fix: clean up popover capture listeners#42732
Merged
Merged
Conversation
Contributor
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
Contributor
✨ Files requiring CODEOWNER review ✨🎨 @MetaMask/design-system-engineers (2 files, +81 -17)
|
|
Contributor
Builds ready [0ccfb58]
⚡ Performance Benchmarks (Total: 🟢 15 pass · 🟡 9 warn · 🔴 0 fail)
Bundle size diffs
|
kirillzyusko
left a comment
Contributor
There was a problem hiding this comment.
It looks good, but I have 2 questions:
- Do we need to port these changes to DSR repo? (thought popover component may be not there yet)
- Do we need to apply the same fix for
ModalContent? The code there is very similar to what we had inPopovercomponent so decided to ask
Contributor
Author
|
@kirillzyusko thanks for reviewing. Popover is not on DSR yet, only PopoverHeader, so there's no listener cleanup issue there. ModalContent is similar but it does not have the leak fixed here. It already adds/removes keydown and mousedown without capture options in both calls, so listener identity matches. |
kirillzyusko
left a comment
Contributor
There was a problem hiding this comment.
Looks good to me! 🚀
georgewrmarshall
approved these changes
May 21, 2026
kirillzyusko
approved these changes
May 21, 2026
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 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.
Description
Fixes a shared Popover listener leak that retained old route trees during send/settings route churn.
Popover registered document
keydownandclicklisteners with capture enabled, but removed them without matching capture options. SinceremoveEventListenermatches on event type, callback, and capture flag, cleanup failed and leaked document listeners retained their React closure scope.This PR keeps the change scoped to
ui/components/component-library/popover/popover.tsxby:addEventListenerChangelog
CHANGELOG entry: null
Manual testing steps
Build the test extension:
The
yarn llm:memoryprofiler is available on the separate profiler PR. With that tooling available and this branch'sdist/chromebuild, run:Expected post-fix profile from the prior investigation with the Snow WeakMap patch still applied:
Validation
source ~/.nvm/nvm.sh && nvm use && yarn test:unit ui/components/component-library/popover/popover.test.tsxsource ~/.nvm/nvm.sh && nvm use && yarn lint:changed:fixsource ~/.nvm/nvm.sh && nvm use && yarn webpack:tscsource ~/.nvm/nvm.sh && nvm use && yarn build:test:webpackPre-merge author checklist
Pre-merge reviewer checklist
Note
Low Risk
Low risk: scoped to Popover’s document event listener lifecycle and adds a regression test; main risk is behavior change in when listeners are attached (only when open and handlers provided).
Overview
Fixes a Popover document-listener leak by reusing a shared capture options object and ensuring
removeEventListeneris called with the same callback and capture options used foraddEventListener.Listeners for
keydown(Esc) andclick(outside) are now only attached when the popover is open and the corresponding handler prop is provided, and a new unit test asserts the add/remove signatures match on unmount to prevent regressions.Reviewed by Cursor Bugbot for commit 0ccfb58. Bugbot is set up for automated code reviews on this repo. Configure here.