fix(CDropdown): prevent window listener and popper leaks on unmount#464
Merged
Conversation
- manage keydown/click/keyup listeners declaratively in an effect keyed on visibility, so cleanup always removes the exact references it added (also when handlers or the toggle element change while open) - destroy the popper instance on unmount; the previous cleanup effect captured the initial visibility in a stale closure and never ran - skip handleHide on mount so onHide is not called for a never-opened dropdown - memoize usePopper callbacks and destroy the previous popper instance on re-init to keep stable identities and avoid leaking scroll/resize listeners - add a regression test covering popper re-initialization when the toggler element is replaced while the dropdown is open
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
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
_visiblevalue in a stale closure (useEffect(() => () => { if (_visible) handleHide() }, [])), so it never ran for a dropdown that was opened after mount. Unmounting an open dropdown leftclick/keyuplisteners onwindowand a live Popper instance.handleShowand removed inhandleHide. When anyuseCallbackdependency changed while the dropdown was open (e.g. a newonHideprop or a replaced toggler element),removeEventListenerreceived a different function reference and the original listener stayed attached forever.onHideon mount. The[visible]sync effect ranhandleHide()on first render with the defaultvisible={false}, firing theonHidecallback for a dropdown that was never opened.initPopperoverwrote_popper.currentwithout destroying the previous instance, leaking its scroll/resize listeners whenever the toggler element was replaced while open.Changes
_visible(the same patternCModalalready uses). React guarantees the cleanup removes exactly the references that were added, including on unmount and when the toggler element changes mid-open.[visible]sync effect sohandleHideonly runs when the dropdown is actually open.usePoppercallbacks (useCallback) — they only touch refs, so stable identities are safe; this stopshandleHide/context value churn on every parent render.initPopperbefore creating a new one.Tests
key) while the dropdown is open and asserts the menu keepsposition: absolute+data-popper-placement, Popper is re-created against the new toggler node, and the old instance is destroyed. Verified it fails against the previoususePopper.Note: the
react-hooks/refslint error onusePopper'spopper: _popper.currentreturn pre-exists onmainand is left out of scope — fixing it changes the hook's public API.