fix(deck): prevent AppRefresherIcon scheduler/listener leak on re-render#128
Open
scuba10steve wants to merge 2 commits into
Open
fix(deck): prevent AppRefresherIcon scheduler/listener leak on re-render#128scuba10steve wants to merge 2 commits into
scuba10steve wants to merge 2 commits into
Conversation
AppRefresherIcon called SchedulerFactory.createScheduler(2000) in the render body, but the useEffect that subscribes/tears it down is keyed on [lastRefresh]. Every re-render that did not change lastRefresh (refresh click toggling iconPulsing, refreshing prop changes, parent re-renders) created a fresh scheduler whose rxjs timer and visibilitychange/online/offline listeners were never unsubscribed, leaking them for the session on an always-mounted header component. Move createScheduler into the effect so exactly one scheduler exists per lastRefresh value and is disposed on cleanup. Behavior is unchanged. Adds a spec asserting no new scheduler is created on a lastRefresh-stable re-render.
Switch the spec from shallow to mount so the useEffect actually runs, and assert the real guarantees: exactly one scheduler is created on mount, none is created on a lastRefresh-stable re-render, and the scheduler is torn down on unmount. Verified red on the pre-fix code (createScheduler called twice) and green on the fix.
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
1 participant
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
AppRefresherIconis mounted persistently in the application header. It calledSchedulerFactory.createScheduler(2000)in the render body, while theuseEffectthat subscribes to and tears down that scheduler is keyed on[lastRefresh].Each call to
createSchedulereagerly starts an RxJStimerand registersdocumentvisibilitychangepluswindowonline/offlinelisteners, whichare only removed via
unsubscribe(). Because the scheduler was created on everyrender but the effect only re-ran (and cleaned up) when
lastRefreshchanged,any re-render that did not change
lastRefresh— clicking refresh (togglesiconPulsing), arefreshingprop change, or a parent re-render — created afresh, fully-wired scheduler that was never subscribed and never torn down. Its
timer and three global listeners leaked for the rest of the session.
Fix
Move
createSchedulerinside theuseEffectso exactly one scheduler exists permount/
lastRefreshvalue and is disposed in the effect cleanup. Behavior isotherwise unchanged (the relative-time tooltip still updates on the same 2s
cadence and re-evaluates when
lastRefreshchanges).Test
Adds
AppRefresherIcon.spec.tsxasserting that alastRefresh-stable re-render(changing the
refreshingprop) creates no additional scheduler. The testfails on the old code and passes with the fix.
Verification
fix, passes after).
eslintclean on the changed source;prettier --checkclean.