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.

Sign up for GitHub

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
Merged

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

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7d8d80f
Fixed the bug that was resetting the profile selection state on page …
manojVivek Apr 2, 2024
f1240d9
Linter fix
manojVivek Apr 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 0 additions & 12 deletions ui/packages/shared/profile/src/ProfileSelector/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@
data: profileTypesData,
error,
} = useProfileTypes(queryClient);
const [isDataLoading, setIsDataLoading] = useState<boolean>(false);

Check failure on line 102 in ui/packages/shared/profile/src/ProfileSelector/index.tsx

View workflow job for this annotation

GitHub Actions / UI Test and Lint 

'setIsDataLoading' is assigned a value but never used. Allowed unused vars must match /_/u
const {heightStyle} = useMetricsGraphDimensions(comparing);
const {viewComponent} = useParcaContext();

Expand All @@ -109,18 +109,6 @@

const [queryExpressionString, setQueryExpressionString] = useState(querySelection.expression);

useEffect(() => {
if (querySelection.expression === undefined) {
return;
}

setIsDataLoading(true);
setQueryExpression();
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.


useEffect(() => {
if (enforcedProfileName !== '') {
const [q, changed] = Query.parse(querySelection.expression).setProfileName(
Expand Down
Loading