Skip to content

fix(window): make off() reject event names so listeners stop leaking - #2007

Open
AngelPaella wants to merge 1 commit into
mainfrom
fix/eng4-360-listener-ids
Open

fix(window): make off() reject event names so listeners stop leaking#2007
AngelPaella wants to merge 1 commit into
mainfrom
fix/eng4-360-listener-ids

Conversation

@AngelPaella

@AngelPaella AngelPaella commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Closes ENG4-360.

The bug

off() removes a listener by the id on() handed back. Ten calls across four files passed the event name instead:

const id = iframeClient.on("ui:height.changed", handler); // id is a random string
iframeClient.off("ui:height.changed"); // no listener is stored under that key

WindowTransport.removeMessageListener looks the argument up in its id map, misses, and returns without throwing. TypeScript allowed it because on() returned string, off() took string, and event names are strings.

The window message listener outlives the component. Mount it again and two listeners parse the same message: the live one, and the one the dead render left behind, which still calls the callbacks it captured. setHeight on a dead component does nothing, but onPaymentMethodSelected fires the integrator's stale callback. In useOAuthWindowListener the leak compounds without any remount, since each provider click stacks another pair.

on() now returns a branded id and off() accepts only that, so passing an event name fails the build:

export type ListenerId = string & { readonly __listenerId: true };

Call sites the compiler caught

File off() calls by name
card-management/CrossmintPaymentMethodManagementIFrame.tsx 3, one of them for a listener nothing registers
embed/v3/EmbeddedCheckoutV3IFrame.tsx 1
react-native embed/v3/EmbeddedCheckoutV3WebView.tsx 2
hooks/useOAuthWindowListener.ts 4

useCrossmintCheckout.tsx calls off("order:updated", handler) on a Node-style local emitter with a different signature, so I left it alone.

CrossmintPaymentMethodManagementIFrame

This component had both bugs #2003 fixed in CrossmintIdentityVerificationIFrame, so I copied that fix. The listener effect depends only on iframeClient, which is set once, so props never refreshed and onPaymentMethodSelected fired the callback captured at mount. Swap the callback after mount and the old one runs. It now reads through a latestProps ref.

The third call, off("agentic-enrollment:created"), is gone. paymentMethodManagementIncomingEvents has no such key and nothing subscribed to it. It compiled because off took a string.

useOAuthWindowListener

Capturing ids fixed one of the two problems here. Both are on main.

Each createPopupAndSetupListeners call registered two listeners and removed none. Click Google then Apple and the Google pair stays attached; unmount mid-flow and both pairs stay, along with a 2.5s interval. One cleanup() closure now drops both ids and clears the interval. It runs when auth material arrives, on error, when the user closes the popup, on unmount, and when a new popup replaces one still open.

Replacing an open popup needed a second change. cleanup only reached the ref after await getOAuthUrl(provider) resolved, so a click landing inside that window found nothing to cancel. Both flows registered listeners, and since PopupWindow.initEmpty opens a window by name they shared one popup, so a single authMaterialFromPopupCallback reached both handlers and redeemed the same one-time secret twice. The hook now claims the flow before the await, and the losing flow returns instead of navigating the popup a second time.

Verification

  • Reverting the four component fixes against the branded type reproduces 8 TS2345 errors, all at the off() calls above, and nothing else in the monorepo. Consumers resolve the package through dist/index.d.ts, so the typecheck runs against built output, not source.
  • crossmint-payment-method-management.test.tsx mirrors the identity-verification suite. Its mock returns listener-id:${event}, distinct from the event name, so the unmount assertion fails if cleanup passes names again. Both behavior assertions fail against the pre-fix component.
  • use-oauth-window-listener.test.ts drives two overlapping clicks with getOAuthUrl held open. Against the pre-fix hook it counts 4 registered listeners instead of 2.
  • pnpm --filter @crossmint/client-sdk-react-ui test:vitest: 34/34.
  • pnpm lint clean. client-sdk-base, react-ui and react-native-ui build.

Breaking

Types only, and only for code outside this repo that holds a listener id in a string-typed variable. Those switch to ListenerId, exported from @crossmint/client-sdk-window. on and off behave the same at runtime. The changeset marks the package major, and changeset status bumps the dependents.

No in-repo consumer needed a change beyond the four above. Nothing subclasses or re-declares on/off, and no listener id is stored in an annotated string, so inference covers the rest.

@changeset-bot

changeset-bot Bot commented Aug 6, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: a949f37

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 18 packages
Name Type
@crossmint/client-sdk-react-native-ui Patch
@crossmint/client-sdk-react-ui Patch
@crossmint/client-sdk-window Major
@crossmint/wallets-playground-expo Patch
@crossmint/auth-ssr-nextjs-demo Patch
@crossmint/client-sdk-nextjs-starter Patch
@crossmint/wallets-quickstart-devkit Patch
@crossmint/wallets-playground-react Patch
@crossmint/client-sdk-base Patch
@crossmint/client-sdk-react-base Patch
@crossmint/client-sdk-rn-window Patch
@crossmint/wallets-sdk Patch
@crossmint/client-sdk-auth Patch
@crossmint/client-sdk-verifiable-credentials Patch
@crossmint/client-sdk-smart-wallet Patch
@crossmint/common-sdk-auth Patch
@crossmint/server-sdk Patch
crossmint-auth-node Patch

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

on() returns a branded ListenerId and off() only takes one. Passing an
event name removed nothing and typechecked, so four components leaked
their listeners on unmount.

useOAuthWindowListener also stacked a listener pair per provider click,
and both pairs redeemed the same one-time secret over the shared popup.
One cleanup closure now runs on every exit path, and a second click
during the OAuth URL fetch cancels the first flow instead of racing it.

Fixes ENG4-360.
@AngelPaella
AngelPaella force-pushed the fix/eng4-360-listener-ids branch from 37d5ccd to a949f37 Compare August 6, 2026 00:42
@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

🔥 Smoke Test Results

Status: Failed

Statistics

  • Total Tests: 5
  • Passed: 3 ✅
  • Failed: 1 ❌
  • Skipped: 1 ⚠️
  • Duration: 4.29 min

Test Details


This is a non-blocking smoke test. Full regression tests run separately.

@AngelPaella
AngelPaella marked this pull request as ready for review August 6, 2026 18:01
@greptile-apps

greptile-apps Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Reviews (1): Last reviewed commit: "fix(window): make off() reject event nam..." | Re-trigger Greptile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant