fix(onboarding): clean up setTimeout timers on unmount#980
Closed
tysoncung wants to merge 1 commit intoMODSetter:mainfrom
Closed
fix(onboarding): clean up setTimeout timers on unmount#980tysoncung wants to merge 1 commit intoMODSetter:mainfrom
tysoncung wants to merge 1 commit intoMODSetter:mainfrom
Conversation
Two setTimeout patterns in the onboarding tour could fire after component unmount: 1. checkAndStartTour recursion: Only the initial setTimeout was cleaned up. Inner retries continued after unmount. Fixed by adding a cancelled flag checked before each retry. 2. updateTarget retry: The retry setTimeout was never tracked or cleaned. Fixed by storing it in a ref and clearing it on subsequent calls. Closes MODSetter#950
|
Someone is attempting to deploy a commit to the Rohan Verma's projects Team on Vercel. A member of the Team first needs to authorize it. |
![recurseml[bot]](https://avatars.githubusercontent.com/in/1054195?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by RecurseML
🔍 Review performed on a474c46..b8f9ed0
✨ No bugs found, your code is sparkling clean
Owner
|
@tysoncung, please raise this PR on the "dev" branch. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The onboarding tour component has two
setTimeoutpatterns that can fire after component unmount, potentially causing state updates on unmounted components:checkAndStartTourrecursion (line ~575): Uses recursivesetTimeoutto retry finding target DOM elements, but only the initialsetTimeoutis cleaned up in the effect's teardown. Inner retries keep firing after unmount.updateTargetretry (line ~480): Schedules a retrysetTimeoutthat is never tracked or cancelled.Solution
Added a
cancelledflag that's set totruein the cleanup function. The recursivecheckAndStartTourchecks this flag before executing, preventing state updates after unmount.Added a
retryTimerRefto track the retry timer inupdateTarget. The timer is cleared before scheduling a new one, preventing stale callbacks.Changes
surfsense_web/components/onboarding-tour.tsx:cancelledguard tocheckAndStartTourrecursionretryTimerRefto track and clean upupdateTargetretry timerCloses #950
High-level PR Summary
This PR fixes memory leaks in the onboarding tour component by properly cleaning up
setTimeouttimers when the component unmounts. It introduces acancelledflag to prevent recursivecheckAndStartTourcalls from executing after unmount, and adds aretryTimerRefto track and clear retry timers in theupdateTargetfunction. These changes prevent state updates on unmounted components.⏱️ Estimated Review Time: 5-15 minutes
💡 Review Order Suggestion
surfsense_web/components/onboarding-tour.tsx