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: Remove useEffect fn responsible for resetting current icicle path #3173

Merged
merged 2 commits into from
May 25, 2023

Conversation

yomete
Copy link
Contributor

@yomete yomete commented May 25, 2023

This fixes the bug in #1991.

Steps to reproduce bug

  • Pull up any icicle graph
  • Apply a function filter
  • Expand something in the icicle graph
  • Click on root node
  • Remove function filter
  • From now on anything you click on in the icicle graph has this bug.

A couple of things to note:

TL:DR: Removed the navigateTo function as a dependency in the useEffect hook in the in the useURLState hook.

This bug doesn't seem to happen in Parca but only on PSC. After some investigating, I discovered this is because this useEffect keeps firing after a function filter has been removed i.e. setting the curPath to be an array, effectively causing the icicle graph to reset immediately after any clicking interaction with it.

Since this only takes place after the function filter is removed, I was able to isolate (did this by commenting the setStoreValue line out, and manually updating the URL with a filter_by_function query param and then deleting it. The icicle graph did not reset after this manual update) the bug to the setStoreValue function in the FilterByFunctionbutton component. The setStoreValue function is retrieved from the useURLState hook.

Therefore the reason for the infinite re-rendering had to be in the useURLState hook, and probably the useEffect hook, which had a couple of dependencies, which it used to determine whether to re-render. So I thought to remove the navigateTo function as a dependency in the useEffect hook in the in the useURLState hook.

Screen.Recording.2023-05-25.at.17.27.43.480.mov

@yomete
Copy link
Contributor Author

yomete commented May 25, 2023

@manojVivek @monicawoj since y'all worked originally on this hook, do you think this changes anything for the hook's functionality?

@@ -73,7 +73,7 @@ export const useURLState = ({
}
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [value, highlightAfterFilteringEnabled, param, withURLUpdate, navigateTo]);
}, [value, highlightAfterFilteringEnabled, param, withURLUpdate]);
Copy link
Member

Choose a reason for hiding this comment

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

How many developers suffer each day because of this? 😂
Nice find!

@yomete yomete merged commit 16069bf into main May 25, 2023
@yomete yomete deleted the fix-instant-reset-bug branch May 25, 2023 17:30
@monicawoj
Copy link
Contributor

ui/packages/shared/components/src/hooks/useURLState.tsx

@yomete I think this should be ok. The difference between Parca and PSC is in fact what is provided to the navigateTo prop. It's likely creating a new object every time in PSC and thus the prop is changing on every render. Another approach would be to attempt to use a useCallback hook for a shallow comparison of navigateTo but I think your solution should suffice for now - good find!

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