Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ui: Fixed the bug that was resetting the profile selection state on page load #4476

Merged
merged 2 commits into from
Apr 2, 2024

Conversation

manojVivek
Copy link
Contributor

@manojVivek manojVivek commented Apr 2, 2024

This useEffect was causing the state reset of the selected profile on page load.

Also, this effect seems unnecessary as it was just causing a new search effect, we could have added this as a workaround to some edge case, but the app works fine without this. So we can re-visit if any problem surfaces.

@manojVivek manojVivek requested a review from a team as a code owner April 2, 2024 07:27
setIsDataLoading(false);

// eslint-disable-next-line react-hooks/exhaustive-deps
}, [timeRangeSelection]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems like we had an intention behind this, are we sure we're not breaking something else with this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just updated the PR description about this. Basically, this effect feels like a hack as it watches on the timeRangeSelection and resets the query state, which is only necessary as any timeRangeSelection would do reset the state by itself.

For now, the app works fine without this on all the common use cases I tried, but we can watch out for any other edge case issue and see how to properly handle that.

Copy link

alwaysmeticulous bot commented Apr 2, 2024

🤖 Meticulous spotted visual differences in 110 of 362 screens tested: view and approve differences detected.

Last updated for commit f1240d9. This comment will update as new commits are pushed.

Copy link
Member

@brancz brancz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, played around with it a bunch and can't find any regressions in the interactions, probably became obsolete through some other change.

Copy link
Contributor

@yomete yomete left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah this lgtm too. I could have sworn that useEffect helped to fix a bug, but like frederic said, we probably solved with some other change.

@manojVivek manojVivek merged commit 80c6b3e into main Apr 2, 2024
38 checks passed
@manojVivek manojVivek deleted the profile-selection-state-bug branch April 2, 2024 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants