fix(react-ui): make each OAuth flow own the popup it opened - #2014
Open
AngelPaella wants to merge 2 commits into
Open
fix(react-ui): make each OAuth flow own the popup it opened#2014AngelPaella wants to merge 2 commits into
AngelPaella wants to merge 2 commits into
Conversation
`useOAuthWindowListener` passed event names to `off()`, which never matched a listener id and removed nothing, so every provider click stacked another listener pair on the same `ChildWindow`. One popup callback then redeemed the same one-time secret once per pair, and the second redemption fails server-side, so the user saw a login error after appearing to succeed. Capturing the ids `on()` already returns is enough to fix that. It also exposes the real problem underneath: all providers share one named popup, and the flow resumes at three points after handing that popup over (the URL await, the refresh await, the 2.5s closure poller). The previous `cancelled` flag was consulted at one of them. Each flow now claims `flowIdRef` and rechecks the claim at all three plus the catch path, so a superseded flow can no longer close the popup, publish its error, or clear the loading state belonging to the flow that replaced it. Two teardown-ordering fixes follow from `off()` finally working: - Teardown moved from the top of the call to the moment of takeover. Earlier left a still-live popup with nothing listening across the URL await, dropping a login that had already completed. - The closure poller no longer removes listeners. The callback page posts its secret and then closes itself, so a tick landing in that gap dropped the material. Takeover and unmount both remove them, so at most one pair is ever alive. Unmounting mid-flow now closes the popup it opened rather than leaving a window on screen that nothing will close. Restoring the old behaviour where the login still landed after unmount would mean keeping the listener leak on purpose. Six tests cover supersede in-flight, supersede after setup, auth-material redemption, and both unmount paths. Verified by mutation: removing the takeover cleanup, the catch guard, or the unmount close each fail a test.
🦋 Changeset detectedLatest commit: 532b72d The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 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/useOAuthWindowListener.ts:112-114
**Ownership checked after auth mutation**
When a popup's redemption remains pending and the user starts another provider flow, `handleRefreshAuthMaterial` stores the superseded flow's tokens and invokes its refresh callback before `ownsPopup()` runs, causing the abandoned provider to publish shared authentication state.
### Issue 2
packages/client/ui/react-ui/src/hooks/useOAuthWindowListener.ts:139-145
**Closed popup poller stays active**
After the user manually closes the popup, this branch clears only `activeOAuthProvider`; the interval, closed popup reference, and message listeners remain alive until another flow starts or the provider unmounts, causing recurring wakeups and retained resources for the remaining provider lifetime.
### Issue 3
packages/client/ui/react-ui/src/hooks/useOAuthWindowListener.ts:41-153
**OAuth callback exceeds size limit**
`createPopupAndSetupListeners` now exceeds 80 lines and combines ownership, popup setup, URL resolution, takeover, event handling, polling, and cleanup. Extracting descriptive helpers would make these race-sensitive lifecycle transitions easier to audit and safely maintain.
---
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): make each OAuth flow own ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
112
to
+114
| const handleAuthMaterial = async (data: { oneTimeSecret: string }) => { | ||
| await crossmintAuth?.handleRefreshAuthMaterial(data.oneTimeSecret); | ||
| childRef.current?.off("authMaterialFromPopupCallback"); | ||
| if (!ownsPopup()) { |
Contributor
There was a problem hiding this comment.
When a popup's redemption remains pending and the user starts another provider flow, handleRefreshAuthMaterial stores the superseded flow's tokens and invokes its refresh callback before ownsPopup() runs, causing the abandoned provider to publish shared authentication state.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/client/ui/react-ui/src/hooks/useOAuthWindowListener.ts
Line: 112-114
Comment:
**Ownership checked after auth mutation**
When a popup's redemption remains pending and the user starts another provider flow, `handleRefreshAuthMaterial` stores the superseded flow's tokens and invokes its refresh callback before `ownsPopup()` runs, causing the abandoned provider to publish shared authentication state.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
139
to
145
| const checkWindowClosure = setInterval(() => { | ||
| if (popup.window?.closed) { | ||
| clearInterval(checkWindowClosure); | ||
| setActiveOAuthProvider(null); | ||
| childRef.current?.off("authMaterialFromPopupCallback"); | ||
| if (ownsPopup()) { | ||
| setActiveOAuthProvider(null); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
After the user manually closes the popup, this branch clears only activeOAuthProvider; the interval, closed popup reference, and message listeners remain alive until another flow starts or the provider unmounts, causing recurring wakeups and retained resources for the remaining provider lifetime.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/client/ui/react-ui/src/hooks/useOAuthWindowListener.ts
Line: 139-145
Comment:
**Closed popup poller stays active**
After the user manually closes the popup, this branch clears only `activeOAuthProvider`; the interval, closed popup reference, and message listeners remain alive until another flow starts or the provider unmounts, causing recurring wakeups and retained resources for the remaining provider lifetime.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
AngelPaella
added a commit
that referenced
this pull request
Aug 10, 2026
`EventEmitter.on()` returns a listener id and `off()` takes one, but both were `string`, so passing an event name type-checked and silently removed nothing. Four call sites did exactly that. `on()` now returns a branded `ListenerId` and `off()` only accepts one, making the mistake a compile error. The brand sits on `Transport.addMessageListener()` / `removeMessageListener()` too, so ids are branded where they are minted rather than laundered with a cast in `on()`, and the publicly exported `SignersWindowTransport` / `RNWebViewTransport` no longer accept an event name either. Call sites fixed here: `CrossmintPaymentMethodManagementIFrame`, `EmbeddedCheckoutV3IFrame` and `EmbeddedCheckoutV3WebView`. All three set their emitter once, so cleanup only runs on unmount and the listeners simply outlived the component; none has async work in flight. The fourth, `useOAuthWindowListener`, has three async resumption points and 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, closing the window where a message delivered between commit and the effect flush hit the previous render's callback. client-sdk-base and client-sdk-rn-window both re-expose the narrowed `off()` signature through their emitter types, so they are majors alongside client-sdk-window instead of taking an automatic patch that would break consumers' builds with no semver signal. Two `AssertTrue` lines pin the brand at build time: widening `off()` back to `string`, or dropping the brand from `ListenerId`, fails the dts build. The duplicated iframe emitter harness moves to tests/shared, and `EmbeddedCheckoutV3IFrame` gets the coverage it never had.
AngelPaella
added a commit
that referenced
this pull request
Aug 10, 2026
`on()` returns a listener id and `off()` takes one, but both were `string`, so
`off("ui:height.changed")` type-checked, looked up an event name in a map keyed
by random ids, and silently removed nothing. Four call sites did exactly that.
`ListenerId` is now branded and `off()` only accepts one, making the mistake a
compile error. The brand sits on `Transport.addMessageListener()` /
`removeMessageListener()` rather than only on `EventEmitter`, so ids are branded
where they are minted instead of laundered with a cast in `on()`. That also
closes a hole: `SignersWindowTransport` and `RNWebViewTransport` are publicly
exported, so a consumer calling `removeMessageListener(eventName)` directly got
the same silent no-op.
Every in-repo call site already passes real ids after #2014 and the call-site PR,
so this commit touches no call sites. Consumers holding an id in a `string`-typed
variable have to switch to the exported `ListenerId`.
client-sdk-base (`PaymentMethodManagementIFrameEmitter`,
`EmbeddedCheckoutV3IFrameEmitter`, `IdentityVerificationIFrameEmitter`) and
client-sdk-rn-window (`WebViewParent`) re-expose the narrowed signature through
their public types, so they are majors too rather than taking an automatic patch
that would break consumer builds with no semver signal.
Two AssertTrue lines pin the guarantee at build time: widening `off()` back to
`string`, or dropping the brand from `ListenerId`, fails the dts build.
AngelPaella
added a commit
that referenced
this pull request
Aug 10, 2026
`on()` returns a listener id and `off()` takes one, but both were `string`, so
`off("ui:height.changed")` type-checked, looked up an event name in a map keyed
by random ids, and silently removed nothing. Four call sites did exactly that.
`ListenerId` is now branded and `off()` only accepts one, making the mistake a
compile error. The brand sits on `Transport.addMessageListener()` /
`removeMessageListener()` rather than only on `EventEmitter`, so ids are branded
where they are minted instead of laundered with a cast in `on()`. That also
closes a hole: `SignersWindowTransport` and `RNWebViewTransport` are publicly
exported, so a consumer calling `removeMessageListener(eventName)` directly got
the same silent no-op.
Every in-repo call site already passes real ids after #2014 and the call-site PR,
so this commit touches no call sites. Consumers holding an id in a `string`-typed
variable have to switch to the exported `ListenerId`.
client-sdk-base (`PaymentMethodManagementIFrameEmitter`,
`EmbeddedCheckoutV3IFrameEmitter`, `IdentityVerificationIFrameEmitter`) and
client-sdk-rn-window (`WebViewParent`) re-expose the narrowed signature through
their public types, so they are majors too rather than taking an automatic patch
that would break consumer builds with no semver signal.
Two AssertTrue lines pin the guarantee at build time: widening `off()` back to
`string`, or dropping the brand from `ListenerId`, fails the dts build.
#2005 landed the same hook's move onto the popup it opens, with listener ids instead of event names, so this branch keeps main's popup client and layers the flow-ownership claim on top. ChildWindow is gone from the hook. Every flow wraps the one named popup in a client of its own, so takeover teardown still matters: a superseded flow's client is subscribed to the same window. The test file merges both sides against real transports, dropping the module mock so the source check stays exercised.
Contributor
|
Reviews (2): Last reviewed commit: "Merge origin/main into fix/eng4-360-oaut..." | Re-trigger Greptile |
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 so the behavioural fix gets its own review. No API change, no type change, patch release.
Rebased on main, and half of the original diff is gone. #2005 landed the same hook's move off
ChildWindowand onto the popup it opens, with the idson()returns instead of event names, so theoff(eventName)half of this PR is already inmain. What is left is the flow-ownership problem underneath it, which #2005 did not address.The problem
All providers share one named popup (
PopupWindow.initEmptyopenswindow.open(..., "popupWindow")), so a second click wraps the window a first flow is still using in aPopupWindowclient of its own. Both clients are subscribed to the same peer. The flow then resumes at three points after handing that popup over:await getOAuthUrl(provider)cancelledawait handleRefreshAuthMaterial()So with no prefetched URLs, clicking Google then Twitter and having Google reject closed the Twitter popup mid-login, set the auth-form error to Google's message, and cleared
activeOAuthProviderwhile Twitter's listeners were still live.Each flow now claims
flowIdRefand rechecks the claim at every resume point:Two teardown-ordering fixes that follow
One deliberate behaviour change
Unmounting mid-flow now closes the popup it opened. Before, the leaked listener meant an unmounted provider's login still landed. Preserving that would mean keeping the leak on purpose, so the abort is explicit instead: no stranded window the SDK has no handle to close.
Verification
Seven tests: source matching (kept from #2005), supersede in-flight, supersede after setup, auth-material redemption, and both unmount paths. They run against the real
WindowTransportrather than a mocked@crossmint/client-sdk-window, so #2005'sevent.sourcecheck stays exercised and the duplicate-listener assertions are behavioural (the secret is redeemed once) instead of countingoff()calls.setErroris hoisted out of the render callback so a superseded flow's writes are observable.Mutation-tested rather than trusted green. Each of these still fails a test after the rebase:
cleanupRef.current?.()(1 failure)ownsPopup()guard from the catch path (2 failures)pnpm lintclean, react-ui suite 43 tests green,pnpm turbo build --filter @crossmint/client-sdk-react-uiclean.