fix(react-ui,react-native-ui): stop the remaining embed listeners leaking past unmount - #2015
Open
AngelPaella wants to merge 3 commits into
Open
Conversation
`CrossmintPaymentMethodManagementIFrame`, `EmbeddedCheckoutV3IFrame` and `EmbeddedCheckoutV3WebView` passed event names to `off()`. Listener ids are 13-char random strings, so nothing matched and nothing was removed. All three set their emitter once behind a guard, so cleanup only fires on unmount and the listeners simply outlived the component. Capturing the id `on()` already returns is the whole fix. None of these sites has async work in flight, so there is no in-flight state to lose by tearing down for real. `useOAuthWindowListener` was the only site that did, and it shipped separately in #2014. `CrossmintPaymentMethodManagementIFrame` also read its callbacks from the render that mounted it, so a callback replaced after mount never fired, and it called `off("agentic-enrollment:created")` for an event it never subscribed to. The latest-props pattern it shares with `CrossmintIdentityVerificationIFrame` moves into a `useLatest` hook that assigns during render rather than in a passive effect, closing the window where a message delivered between commit and the effect flush hit the previous render's callback. Tests: `EmbeddedCheckoutV3IFrame` had none and now has two. The ~40 lines of iframe emitter harness duplicated between the card-management and identity-verification suites move to tests/shared. Reverting any of these cleanups to an event name fails an unmount test.
🦋 Changeset detectedLatest commit: 8b78baf The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
Prompt To Fix All With AI### Issue 1
packages/client/ui/react-ui/src/hooks/useLatest.ts:11
**Render exposes uncommitted callbacks**
When a concurrent render with replacement callbacks is interrupted before commit, assigning `ref.current` during render exposes those uncommitted callbacks to listeners owned by the currently committed tree, causing iframe events to invoke the wrong callback while skipping the callback associated with the visible UI.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix(react-ui): stop the remaining iframe..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| */ | ||
| export function useLatest<T>(value: T) { | ||
| const ref = useRef(value); | ||
| ref.current = value; |
Contributor
There was a problem hiding this comment.
When a concurrent render with replacement callbacks is interrupted before commit, assigning ref.current during render exposes those uncommitted callbacks to listeners owned by the currently committed tree, causing iframe events to invoke the wrong callback while skipping the callback associated with the visible UI.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/client/ui/react-ui/src/hooks/useLatest.ts
Line: 11
Comment:
**Render exposes uncommitted callbacks**
When a concurrent render with replacement callbacks is interrupted before commit, assigning `ref.current` during render exposes those uncommitted callbacks to listeners owned by the currently committed tree, causing iframe events to invoke the wrong callback while skipping the callback associated with the visible UI.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.It was not leaking, but it shares the latest-callback pattern that moves into useLatest, so its ref assignment moves from a passive effect to render. A consumer reading the changelog should see that the component changed.
AngelPaella
changed the title
Contributor
|
Reviews (2): Last reviewed commit: "docs(changeset): note the identity-verif..." | Re-trigger Greptile |
…all-sites Keeps the shared iframeEmitter helper this branch introduced, on main's identity-verification route: #2011 renamed it from kyc-verification.
Contributor
|
Reviews (3): Last reviewed commit: "Merge fix/eng4-360-oauth-popup-ownership..." | Re-trigger Greptile |
Contributor
🔥 Smoke Test Results❌ Status: Failed Statistics
Test DetailsThis is a non-blocking smoke test. Full regression tests run separately. |
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.
Split out of #2007. Stacked on #2014, but touches disjoint files from it, so review order does not matter. Patch release, no API change.
The bug
Three components passed event names to
off():Listener ids are 13-char strings from
generateRandomString(), so this looked up an event name in a map keyed by ids, found nothing, and removed nothing. Capturing the idon()already returns is the whole fix, and it needs no type change.CrossmintPaymentMethodManagementIFrameEmbeddedCheckoutV3IFrameEmbeddedCheckoutV3WebViewAll three set their emitter once behind a guard (
if (!iframe || iframeClient) return), so the cleanup only ever fires on unmount and the listeners simply outlived the component. None has async work in flight, so there is no in-flight state to lose by tearing down for real.useOAuthWindowListenerwas the only site that did, which is why it shipped separately in #2014.EmbeddedCheckoutV3WebViewadditionally letsRNWebViewTransportdetach its globalmessagehandler now that the listener map actually drains.EventEmitter.sendAction()/onAction()always passed real ids, so they were never leaking. The blast radius is exactly the four hand-written sites.Two extras in
CrossmintPaymentMethodManagementIFrameoff("agentic-enrollment:created")for an event it never subscribed to. Removed.The latest-props pattern it shares with
CrossmintIdentityVerificationIFramemoves into auseLatesthook that assigns during render rather than in a passive effect, closing the window where a message delivered between commit and the effect flush hit the previous render's callback. Reverting it to the effect form failsuse-latest.test.ts.Tests
EmbeddedCheckoutV3IFramehad no coverage at all and now has two tests. The ~40 lines of iframe emitter harness duplicated between the card-management and identity-verification suites move totests/shared/iframeEmitter.ts(the duplicate had already drifted on its first day).Mutation-checked: reverting any of these cleanups to an event name fails an unmount test.
pnpm build:libs16/16,pnpm test:vitest11/11 packages, react-ui 42 tests,pnpm lintclean.Known gap
EmbeddedCheckoutV3WebViewstill has no test:react-native-uihas avitest.config.tsand a declared turbo task but notest:vitestscript, so CI skips the package. Happy to wire it up here or take it as a separate ticket.