refactor: share a single UI subscription per messenger event type - #42823
Merged
cryptodev-2s merged 29 commits intoJun 17, 2026
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. |
cryptodev-2s
force-pushed
the
cryptodev-2s/wpc-1005-shared-subscription-per-event-type
branch
from
May 20, 2026 13:28
1ac25f5 to
872d338
Compare
Contributor
Builds ready [872d338]
⚡ Performance Benchmarks (Total: 🟢 13 pass · 🟡 11 warn · 🔴 0 fail)
Bundle size diffs
|
Contributor
Builds ready [31c928c] [reused from 872d338]
⚡ Performance Benchmarks (Total: 🟢 13 pass · 🟡 11 warn · 🔴 0 fail)
Bundle size diffs
|
Deduplicate background-event subscriptions in subscribeToMessengerEvent so the UI registers at most one upstream messengerSubscribe IPC and one background.onNotification listener per event type. Multiple local subscribers share the in-flight subscribe promise; only the last unsubscribe sends the upstream messengerUnsubscribe. Per the conclusion of the PR #42573 discussion (r3259805738). WPC-1005
…mock Adds two short comments above the `eventName as NamespacedName` and `callback as (data: Json) => void` casts to explain why each is safe. Removes the now-unused `removeOnNotification` jest mock from the test setup since the global notification router is never detached. WPC-1005
Use object destructuring for notification.params and flip the entry-check branch order to avoid a negated condition with else clause. WPC-1005
Documents the Set semantics of the callbacks field so future maintainers know that subscribing the same function twice collapses to a single slot. WPC-1005
Verifies that replacing the background connection clears eventEntries and resets the notification router, so a subsequent subscribe sends a fresh upstream IPC against the new connection. WPC-1005
Contributor
Builds ready [26fac36]
⚡ Performance Benchmarks (Total: 🟢 18 pass · 🟡 7 warn · 🔴 0 fail)
Bundle size diffs
|
FrederikBolding
previously approved these changes
Jun 16, 2026
cryptodev-2s
enabled auto-merge
June 16, 2026 12:41
GuillaumeRx
reviewed
Jun 16, 2026
The unsubscribe path just awaits the result, so a plain jest.fn works. Only messengerSubscribe needs to return a promise because the code calls .catch on it. WPC-1005
Contributor
Builds ready [2799aef]
⚡ Performance Benchmarks (Total: 🟢 14 pass · 🟡 11 warn · 🔴 0 fail)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Key each subscription by a unique symbol instead of deduping callbacks in a Set. Two subscribe calls that share the same function reference are now independent registrations, so unsubscribing one no longer tears down the other. This avoids a React footgun where two mounted components subscribing with a stable handler would lose events when the first unmounts. A shared callback now fires once per registration rather than once total, which is the correct per subscription semantics. WPC-1005
Contributor
Builds ready [d9f75da]
⚡ Performance Benchmarks (Total: 🟢 15 pass · 🟡 10 warn · 🔴 0 fail)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
GuillaumeRx
requested changes
Jun 17, 2026
GuillaumeRx
approved these changes
Jun 17, 2026
FrederikBolding
approved these changes
Jun 17, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Jun 17, 2026
cryptodev-2s
deleted the
cryptodev-2s/wpc-1005-shared-subscription-per-event-type
branch
June 17, 2026 17:12
metamaskbot
added
the
release-13.37.0
Contributor
🧪 Validation RunVerdict: Note Trial run of the MetaMask evidence skills, Every retention primitive the diff adds, paired against a release in the same file, run in CI at the merge commit: Ran to completion (exit 0) — read the output, no verdict assertedClaim under test: every retention primitive this diff introduces has a paired release $ sh -c git diff 8bd65b126ccaf4ce2a96cd8abb7d31ac52f3f4ac^ 8bd65b126ccaf4ce2a96cd8abb7d31ac52f3f4ac > d.patch && python3 .evidence-skills/domains/stability/skills/memory-leak/scripts/retention-scan.py ui/store/background-connection.ts:d.patch
RETENTION REVIEW — scoped to the supplied diff (re-run: retention-scan.py <file>:<patch> [...])
==========================================================================
background-connection.ts
[NEW ] OPEN L154: background.onNotification(routeMessengerEventNotification)
-> no removeOnNotification/offNotification in file
VERDICT: 1 NEW un-paired primitive(s) → escalate to a heap snapshot (Phase 2)Produced by Follows from the scan above
Open for review: whether that listener's lifetime is bounded by something outside this file — a port teardown, a page unload, a controller disposal. A static pass sees a file, not a lifecycle, so it can say the pair is absent here and not that the listener leaks. If the connection is per-session and the page owns it, this is correct as written; if a connection can be replaced while the page lives, each replacement adds a listener. |
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.
6 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
Follows up on the discussion in PR #42573 (r3259805738). After that PR, every route messenger that subscribes to a background event called
subscribeToMessengerEventindependently — so N route messengers subscribing to the same event produced N upstreammessengerSubscribeIPCs and Nbackground.onNotificationlisteners, each running a filter check on every incoming notification.This PR deduplicates at the
subscribeToMessengerEventlayer (the location the thread converged on — notUIMessenger, which still needs per-messenger bookkeeping forrevoke):Map<event, { callbacks, subscribePromise }>refcounts local subscribers.notificationRouterlistens onbackground.onNotificationand dispatches by event-name lookup — no more per-listener filter scan.subscribePromise; the upstreammessengerUnsubscribeonly fires when the last subscriber leaves.setBackgroundConnectionclears the in-memory state, so a replaced background connection starts with a clean slate.Public signature of
subscribeToMessengerEventis unchanged; both existing callers (UIMessenger.delegateandui/contexts/query-client.ts) benefit transparently.How it works
Two subscribers to the same event share one upstream IPC and one notification listener. When a notification arrives, the router does a single map lookup and fans out locally.
sequenceDiagram autonumber participant A as UI caller A participant B as UI caller B participant Sub as subscribeToMessengerEvent participant Map as eventEntries (Map) participant Router as notificationRouter participant BG as background (IPC) A->>Sub: subscribe('EventX', cbA) Sub->>Map: get('EventX') -> undefined Sub->>BG: onNotification(router) [attach once] Sub->>BG: messengerSubscribe('EventX') Sub->>Map: set('EventX', { {cbA}, subscribePromise }) Sub-->>A: unsubscribeA B->>Sub: subscribe('EventX', cbB) Sub->>Map: get('EventX') -> entry Sub->>Map: entry.callbacks.add(cbB) Note over Sub,BG: no new IPC, no new listener Sub-->>B: unsubscribeB BG-->>Router: notification('EventX', payload) Router->>Map: get('EventX') -> entry Router->>A: cbA(payload) Router->>B: cbB(payload) A->>Sub: unsubscribeA() Sub->>Map: entry.callbacks.delete(cbA) -> size=1 Note over Sub,BG: no IPC - others still subscribed B->>Sub: unsubscribeB() Sub->>Map: entry.callbacks.delete(cbB) -> size=0 Sub->>Map: delete('EventX') Sub->>BG: messengerUnsubscribe('EventX')Out of scope (deliberate)
UIMessengeris not modified — per the thread's conclusion.ui/contexts/query-client.tsis not modified. It has a latent unsubscribe-before-subscribe-resolves race, but that's a separate concern.setBackgroundConnectioncalled a second time mid-session) was already not handled by the previous code; this PR matches that — though it does now clear in-memory state on replace, which is hygiene the previous code lacked.Changelog
CHANGELOG entry: null
Related issues
Manual testing steps
The behavioral change is invisible to end users — the cost being optimized is per-notification CPU and number of IPC round-trips. The 15 unit tests in
ui/store/background-connection.test.tscover the observable contract. For local smoke:To observe the optimization itself, inspect
background.onNotificationlisteners or the count ofmessengerSubscribecalls in DevTools — there should now be at most one per distinct event type for the lifetime of the UI session.Screenshots/Recordings
N/A — no UI changes.
Pre-merge author checklist
Pre-merge reviewer checklist