Skip to content

Closing a wallet or picker window leaves connect() stuck #49

Description

@fernandomg

Description

sdk.connect() never settles when a window is closed rather than dismissed through the picker's
own UI. useConnect().isConnecting stays true and every control gated on it is disabled until the
page is reloaded.

The SDK's own cancel path works — this is not "all cancellation is broken":

user action outcome
reject in the wallet, then click Cancel in the picker's try-again prompt recovers cleanly; connect() rejects and state resets
close the wallet window without completing (e.g. console wallet, no login) the picker sits in a loading state indefinitely
close the picker window instead of clicking Cancel connect() never settles — dApp bricked until reload

So the gap is window-close detection, on both windows, not the retry flow. Verified with Carpincho
(reject → Cancel → recovers) and with a console wallet (close the wallet window → picker spins), and
first seen with a WalletConnect QR, so it is not adapter-specific.

The SDK does try to handle it. Its picker wait rejects on the popup's beforeunload:

// core-wallet-ui-components@1.6.0/dist/index.js:9662-9677
Sa = (t) => new Promise((e, r) => {
  const n = () => {  r(new Error("User closed the wallet picker")) }
  t.addEventListener("beforeunload", n)
  window.addEventListener("message", i)   // resolves on SPLICE_WALLET_PICKER_RESULT
})

and the retry waiter refuses outright if the window is already gone:

// :9678-9683
Lp = async () => {
  const t = Ht
  if (!t || t.closed) throw new Error("Wallet picker is not open")
  return await Sa(t)
}

So on paper a close rejects, the rejection propagates out of DappSDK.connect()'s retry branch
(dapp-sdk@1.4.0/dist/index.js:1252-1261), and the consumer's catch runs.

In practice nothing settles — confirmed empirically: a harness that logs on both resolve and
reject printed neither.

Measured, not hypothesised. Wrapping window.open and watching the SDK's popup through the
console-wallet flow gives:

21:02:49.107  opened name="wallet-popup" — same-origin (about:blank)
21:02:49.108  beforeunload listener attached
21:02:49.108  beforeunload FIRED                     <-- 1ms after open, nothing was closed
21:02:49.509  navigated: about:blank -> same-origin (http://localhost:3020/<uuid>)
21:03:00.309  window.closed became true — polling detects the close

The popup opens blank and navigates ~400ms later, and beforeunload fires on that navigation. So a
listener attached at open time is consumed before the user can close anything, and nothing is watching
when the real close happens. Polling closed catches it. The popup stays on the dApp's own origin
throughout, so cross-origin access is not the obstacle — an earlier guess of mine that the measurement
disproved.

Whatever the SDK's root cause, our layer has no bound of its own, and a consumer cannot fix it
from outside: connect() returns a promise with no abort, and nothing on the hook surface cancels or
times out.

Steps to reproduce

  1. Run a dApp against the built package with any wallet available in the picker.
  2. Click connect and choose a wallet.
  3. Close the picker window with its window control, without clicking its Cancel button. (Or: pick a
    wallet, close the wallet's own window without completing, then close the picker.)
  4. isConnecting stays true. The connect button never re-enables. Only a reload recovers.

Contrast: rejecting in the wallet and then clicking Cancel in the picker recovers correctly, which
is what makes this a window-close gap rather than a broken cancel path.

Expected vs actual behavior

Expected: a cancelled or failed attempt settles. connectError is set, isConnecting returns to
false, and the user can try again without reloading.

Actual: isConnecting stays true indefinitely, with no error surfaced and no way back.

Severity

High, not critical: there is a working path (the picker's Cancel button), so a user who dismisses the
modal the intended way is fine. But closing a window is an ordinary reflex, it bricks the package's
main entry point when it happens, and a dApp author cannot recover from it — connect() exposes no
abort.

Proposed fix

Ours, and it holds regardless of what the SDK does:

  • Bound the wait. A connectTimeoutMs on CantonConnectConfig with a humane default. On expiry,
    set connectError and run the same sdk.status() probe the cancelled-picker path already uses, so
    a session that survived the failed attempt is kept rather than discarded.
  • Reconcile a late completion. If the user finishes connecting after the timeout, the SDK connects
    its client regardless. Keep a .then on the original promise routing into syncFromStatus, so the
    UI recovers instead of showing disconnected over a live session. This is the subtle half.

Upstream, now measured: the picker should poll popup.closed rather than rely on a beforeunload
listener that the popup's own first navigation consumes. Tracked in
docs/carpincho-enhancements.md.

Explicitly rejected: grabbing the popup handle ourselves via window.open('', 'wallet-popup') to
poll .closed. It depends on an internal window name and would break silently the day it changes.

Acceptance criteria

  • A cancelled or rejected connection settles: connectError set, isConnecting back to false,
    retry possible without a reload — verified in a browser with a real wallet, not only in jsdom
  • connectTimeoutMs exists with a documented default, and connect() never leaves
    isConnecting true indefinitely
  • A session that survived the failed attempt is kept, not discarded
  • A connection completed after the timeout reconciles into connected state with the party
    populated
  • Test: a picker whose promise never settles — assert isConnecting returns to false and
    connectError is set. Fake timers; note waitFor polls on real timers, so fake time has to be
    advanced inside act
  • Test: the reconcile path — settle after the timeout, assert the party appears. Write this one
    first; it is the half most likely to be wrong

Notes

The suite missed this because all 37 tests drive connect() to resolve or reject. "Never settles"
had no coverage at all, which is why it took a browser and a real wallet to surface — and why the
first acceptance criterion above insists on a browser check rather than a unit test alone.

Metadata

Metadata

Assignees

No one assigned

    Labels

    area: connectcanton-connect: hooks, adapters, session, SDK facadebugSomething isn't workingpriority: highMust be addressed in current sprint

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions