From 90f966e1497f16348e018920497fc664c1e33a80 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 8 May 2026 20:53:32 +0000 Subject: [PATCH 1/3] fix(wallet-sdk): allow popup messenger access before open() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wallet.ts calls bindMessengerListeners(handle) synchronously inside ensureActive(), which means listeners are registered on the popup handle's messenger before the user-gesture click that mounts the popup window. The previous popup factory exposed `messenger` as a getter that threw "Popup messenger not initialised" until open() created the bridge — so the iframe→popup downgrade path threw the moment switchMode("popup") ran (e.g. on browsers without IntersectionObserver v2 where the parent origin isn't on the wallet host's trustedHosts allowlist, which is what the agw-wallet-sdk-demo hit). Replace the lazy getter with a stable proxy that buffers on()/send() calls until open() adopts a real bridge, then replays them. waitForReady() also resolves through the proxy so callers don't need to time their calls around open(). Behaviour after open() is unchanged. --- packages/wallet-sdk/src/core/Dialog.ts | 128 +++++++++++++++++--- packages/wallet-sdk/test/src/Dialog.test.ts | 30 +++++ 2 files changed, 140 insertions(+), 18 deletions(-) create mode 100644 packages/wallet-sdk/test/src/Dialog.test.ts diff --git a/packages/wallet-sdk/src/core/Dialog.ts b/packages/wallet-sdk/src/core/Dialog.ts index 3d97d57..001b935 100644 --- a/packages/wallet-sdk/src/core/Dialog.ts +++ b/packages/wallet-sdk/src/core/Dialog.ts @@ -366,6 +366,30 @@ export function popup(options: PopupOptions = {}): DialogFactory { let win: Window | null = null; let bridge: Messenger.Bridge | null = null; let pollClosed: ReturnType | null = null; + let destroyed = false; + + // Subscriptions registered before the bridge exists. Each captures its + // topic/listener types in a closure so we can replay them once `open()` + // adopts a bridge, without losing per-topic typing. + type PendingSub = { + attach: (b: Messenger.Bridge) => () => void; + off?: (() => void) | undefined; + }; + const pendingSubs: PendingSub[] = []; + + // Sends queued before the bridge exists. Each captures its topic/payload + // types in a closure for the same reason. The synchronous return value + // uses a freshly-minted envelope id; the wallet correlates by the inner + // RPC id, so the envelope id mismatch does not affect request/response + // routing. + type QueuedSend = (b: Messenger.Bridge) => void; + const queuedSends: QueuedSend[] = []; + + type ReadyResolver = { + resolve: (options: Messenger.ReadyOptions) => void; + reject: (reason?: unknown) => void; + }; + const pendingReady: ReadyResolver[] = []; function teardownPoll() { if (pollClosed) { @@ -374,18 +398,86 @@ export function popup(options: PopupOptions = {}): DialogFactory { } } + const messenger: Messenger.Bridge = { + on( + topic: T, + listener: Messenger.Listener, + id?: string | undefined, + ) { + if (bridge) return bridge.on(topic, listener, id); + const sub: PendingSub = { + attach: (b) => b.on(topic, listener, id), + }; + pendingSubs.push(sub); + return () => { + sub.off?.(); + const i = pendingSubs.indexOf(sub); + if (i >= 0) pendingSubs.splice(i, 1); + }; + }, + send( + topic: T, + payload: Messenger.Payload, + target?: string | undefined, + ) { + if (bridge) return bridge.send(topic, payload, target); + queuedSends.push((b) => { + b.send(topic, payload, target); + }); + return { id: Messenger.uuid(), topic, payload }; + }, + destroy() { + destroyed = true; + teardownPoll(); + try { + win?.close(); + } catch { + /* COOP severs the WindowProxy — popup closes itself anyway */ + } + win = null; + bridge?.destroy(); + bridge = null; + for (const sub of pendingSubs) sub.off?.(); + pendingSubs.length = 0; + queuedSends.length = 0; + const err = new Error("Messenger destroyed"); + for (const r of pendingReady) r.reject(err); + pendingReady.length = 0; + }, + ready() { + // dApp side: the wallet host is the one that announces ready, not us. + }, + waitForReady() { + if (bridge) return bridge.waitForReady(); + return new Promise((resolve, reject) => { + pendingReady.push({ resolve, reject }); + }); + }, + }; + + function adoptBridge(b: Messenger.Bridge) { + bridge = b; + for (const sub of pendingSubs) sub.off = sub.attach(b); + for (const flush of queuedSends) flush(b); + queuedSends.length = 0; + if (pendingReady.length > 0) { + const resolvers = pendingReady.splice(0); + b.waitForReady().then( + (opts) => { + for (const r of resolvers) r.resolve(opts); + }, + (err) => { + for (const r of resolvers) r.reject(err); + }, + ); + } + } + const handle: DialogHandle = { mode: "popup", - // Lazily-bound — the bridge isn't constructable until `open()` runs. - // Consumers should always call `open()` before reading `messenger`. - get messenger(): Messenger.Bridge { - if (!bridge) - throw new Error( - "Popup messenger not initialised — call open() before sending", - ); - return bridge; - }, + messenger, open() { + if (destroyed) return; if (win && !win.closed) { win.focus(); return; @@ -409,13 +501,15 @@ export function popup(options: PopupOptions = {}): DialogFactory { ); if (!win) throw new Error("Popup blocked by browser"); - bridge = Messenger.bridge({ - from: Messenger.fromWindow(window, { targetOrigin: hostOrigin }), - to: Messenger.fromWindow(win, { targetOrigin: hostOrigin }), - waitForReady: true, - }); + adoptBridge( + Messenger.bridge({ + from: Messenger.fromWindow(window, { targetOrigin: hostOrigin }), + to: Messenger.fromWindow(win, { targetOrigin: hostOrigin }), + waitForReady: true, + }), + ); - bridge.send("__internal", { + messenger.send("__internal", { type: "init", mode: "popup", referrer: getReferrer(), @@ -441,9 +535,7 @@ export function popup(options: PopupOptions = {}): DialogFactory { win = null; }, destroy() { - this.close(); - bridge?.destroy(); - bridge = null; + messenger.destroy(); }, async secure() { // Popups don't suffer from clickjacking — the wallet runs in its own diff --git a/packages/wallet-sdk/test/src/Dialog.test.ts b/packages/wallet-sdk/test/src/Dialog.test.ts new file mode 100644 index 0000000..605d11a --- /dev/null +++ b/packages/wallet-sdk/test/src/Dialog.test.ts @@ -0,0 +1,30 @@ +import { describe, expect, it, vi } from "vitest"; +import * as Dialog from "../../src/core/Dialog.js"; + +describe("popup factory", () => { + it("does not throw when listeners or sends are issued before open()", () => { + const handle = Dialog.popup()({ host: "https://wallet.test" }); + + const listener = vi.fn(); + const off = handle.messenger.on("rpc-response", listener); + expect(typeof off).toBe("function"); + + const envelope = handle.messenger.send("rpc-request", { + id: 1, + jsonrpc: "2.0", + method: "eth_chainId", + }); + expect(envelope.id).toBeTruthy(); + expect(envelope.topic).toBe("rpc-request"); + + off(); + handle.destroy(); + }); + + it("rejects pre-open waitForReady() callers when the handle is destroyed", async () => { + const handle = Dialog.popup()({ host: "https://wallet.test" }); + const ready = handle.messenger.waitForReady(); + handle.destroy(); + await expect(ready).rejects.toThrow(/destroyed/); + }); +}); From a3051617a65a9124e1fd544d74212c6d83ecb474 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 9 May 2026 00:58:58 +0000 Subject: [PATCH 2/3] refactor(wallet-sdk): align Dialog with Porto's handler-based design Drop the previous buffered-messenger proxy in favour of Porto's inverted dependency: factories own listener wiring internally and consumers route data through callbacks (`DialogHandlers`) plus a narrow outbound surface (`syncRequest`, `waitForReady`). - `DialogHandle` no longer exposes `messenger`; the underlying Messenger.Bridge is closure-local in each factory. - `DialogFactory` now takes a `handlers` parameter; each factory registers `rpc-response` / `__internal` / `close` listeners on its own messenger (the popup factory does it inside `open()` once the bridge is constructable). - `Wallet.ts` builds a single `DialogHandlers` struct and stops re-wiring listeners per handle. `bindMessengerListeners` is gone. - `syncRequest` mirrors Porto's optional-chained `messenger?.send` pattern; pre-open calls are no-ops so a stale request stays in the consumer's pending map until the next mode switch. This removes the original "Popup messenger not initialised" error by construction (no lazy getter exists to throw) and keeps the SDK mechanically close to upstream Porto for future ports. --- packages/wallet-sdk/src/core/Dialog.ts | 217 +++++++------------- packages/wallet-sdk/src/core/Wallet.ts | 50 ++--- packages/wallet-sdk/src/exports/index.ts | 1 + packages/wallet-sdk/test/src/Dialog.test.ts | 57 +++-- 4 files changed, 147 insertions(+), 178 deletions(-) diff --git a/packages/wallet-sdk/src/core/Dialog.ts b/packages/wallet-sdk/src/core/Dialog.ts index 001b935..03bae83 100644 --- a/packages/wallet-sdk/src/core/Dialog.ts +++ b/packages/wallet-sdk/src/core/Dialog.ts @@ -37,14 +37,35 @@ export type SecurityState = { host: boolean; }; +/** + * Inbound callbacks the consumer (e.g. `Wallet.ts`) passes when constructing a + * dialog. The factory wires these into the messenger internally — consumers + * never touch the underlying transport directly. Mirrors Porto's design, + * which uses a shared store + handler functions instead of exposing the + * messenger surface (see `porto/src/core/Dialog.ts`). + */ +export type DialogHandlers = { + /** Fired when the wallet host returns an RPC response. */ + onResponse( + response: Messenger.RpcResponse & { _request: Messenger.RpcRequest }, + ): void; + /** Fired for internal control payloads (e.g. mode switch requests). */ + onInternal(payload: Messenger.InternalPayload): void; + /** Fired when the wallet host (or popup-close watcher) signals close. */ + onClose(): void; +}; + export type DialogHandle = { readonly mode: DialogMode; - readonly messenger: Messenger.Bridge; open(): void; close(): void; destroy(): void; /** Run the security checklist. Resolves once the messenger is ready. */ secure(): Promise; + /** Forward an RPC request to the wallet host. */ + syncRequest(request: Messenger.RpcRequest): void; + /** Resolve once the wallet host announces ready (with its capabilities). */ + waitForReady(): Promise; }; export type DialogFactory = (parameters: { @@ -52,6 +73,8 @@ export type DialogFactory = (parameters: { host: string; /** Theme hint forwarded in the init payload. */ theme?: "light" | "dark" | undefined; + /** Inbound callbacks. Wired into the messenger by the factory. */ + handlers: DialogHandlers; }) => DialogHandle; // ---------- Internals ---------- @@ -112,7 +135,7 @@ export type IframeOptions = { export function iframe(options: IframeOptions = {}): DialogFactory { const { skipProtocolCheck = false } = options; - return ({ host, theme }) => { + return ({ host, theme, handlers }) => { if (typeof window === "undefined") return noopHandle(); const hostOrigin = originOf(host); @@ -199,6 +222,11 @@ export function iframe(options: IframeOptions = {}): DialogFactory { waitForReady: true, }); + // Inbound wiring is internal — the consumer never touches the messenger. + messenger.on("rpc-response", handlers.onResponse); + messenger.on("__internal", handlers.onInternal); + messenger.on("close", handlers.onClose); + const drawerModeQuery = window.matchMedia( `(max-width: ${DRAWER_BREAKPOINT}px)`, ); @@ -303,7 +331,6 @@ export function iframe(options: IframeOptions = {}): DialogFactory { const handle: DialogHandle = { mode: "iframe", - messenger, open() { showDialog(); activateDialog(); @@ -337,6 +364,12 @@ export function iframe(options: IframeOptions = {}): DialogFactory { const frameOk = IO.supported() || trustedHost; return { protocol, frame: frameOk, host: trustedHost }; }, + syncRequest(request) { + messenger.send("rpc-request", request); + }, + waitForReady() { + return messenger.waitForReady(); + }, }; return handle; @@ -354,7 +387,7 @@ export type PopupOptions = { export function popup(options: PopupOptions = {}): DialogFactory { const { type = "auto", size = DEFAULT_POPUP_SIZE } = options; - return ({ host, theme }) => { + return ({ host, theme, handlers }) => { if (typeof window === "undefined") return noopHandle(); const hostOrigin = originOf(host); @@ -364,32 +397,12 @@ export function popup(options: PopupOptions = {}): DialogFactory { : "popup"; let win: Window | null = null; - let bridge: Messenger.Bridge | null = null; + // The messenger is constructed inside `open()` because `Messenger.fromWindow` + // requires the popup's Window reference, which only exists after + // `window.open()`. Mirrors Porto's design — consumers never touch this + // directly; they go through `syncRequest` / `waitForReady` instead. + let messenger: Messenger.Bridge | null = null; let pollClosed: ReturnType | null = null; - let destroyed = false; - - // Subscriptions registered before the bridge exists. Each captures its - // topic/listener types in a closure so we can replay them once `open()` - // adopts a bridge, without losing per-topic typing. - type PendingSub = { - attach: (b: Messenger.Bridge) => () => void; - off?: (() => void) | undefined; - }; - const pendingSubs: PendingSub[] = []; - - // Sends queued before the bridge exists. Each captures its topic/payload - // types in a closure for the same reason. The synchronous return value - // uses a freshly-minted envelope id; the wallet correlates by the inner - // RPC id, so the envelope id mismatch does not affect request/response - // routing. - type QueuedSend = (b: Messenger.Bridge) => void; - const queuedSends: QueuedSend[] = []; - - type ReadyResolver = { - resolve: (options: Messenger.ReadyOptions) => void; - reject: (reason?: unknown) => void; - }; - const pendingReady: ReadyResolver[] = []; function teardownPoll() { if (pollClosed) { @@ -398,86 +411,9 @@ export function popup(options: PopupOptions = {}): DialogFactory { } } - const messenger: Messenger.Bridge = { - on( - topic: T, - listener: Messenger.Listener, - id?: string | undefined, - ) { - if (bridge) return bridge.on(topic, listener, id); - const sub: PendingSub = { - attach: (b) => b.on(topic, listener, id), - }; - pendingSubs.push(sub); - return () => { - sub.off?.(); - const i = pendingSubs.indexOf(sub); - if (i >= 0) pendingSubs.splice(i, 1); - }; - }, - send( - topic: T, - payload: Messenger.Payload, - target?: string | undefined, - ) { - if (bridge) return bridge.send(topic, payload, target); - queuedSends.push((b) => { - b.send(topic, payload, target); - }); - return { id: Messenger.uuid(), topic, payload }; - }, - destroy() { - destroyed = true; - teardownPoll(); - try { - win?.close(); - } catch { - /* COOP severs the WindowProxy — popup closes itself anyway */ - } - win = null; - bridge?.destroy(); - bridge = null; - for (const sub of pendingSubs) sub.off?.(); - pendingSubs.length = 0; - queuedSends.length = 0; - const err = new Error("Messenger destroyed"); - for (const r of pendingReady) r.reject(err); - pendingReady.length = 0; - }, - ready() { - // dApp side: the wallet host is the one that announces ready, not us. - }, - waitForReady() { - if (bridge) return bridge.waitForReady(); - return new Promise((resolve, reject) => { - pendingReady.push({ resolve, reject }); - }); - }, - }; - - function adoptBridge(b: Messenger.Bridge) { - bridge = b; - for (const sub of pendingSubs) sub.off = sub.attach(b); - for (const flush of queuedSends) flush(b); - queuedSends.length = 0; - if (pendingReady.length > 0) { - const resolvers = pendingReady.splice(0); - b.waitForReady().then( - (opts) => { - for (const r of resolvers) r.resolve(opts); - }, - (err) => { - for (const r of resolvers) r.reject(err); - }, - ); - } - } - const handle: DialogHandle = { mode: "popup", - messenger, open() { - if (destroyed) return; if (win && !win.closed) { win.focus(); return; @@ -501,13 +437,17 @@ export function popup(options: PopupOptions = {}): DialogFactory { ); if (!win) throw new Error("Popup blocked by browser"); - adoptBridge( - Messenger.bridge({ - from: Messenger.fromWindow(window, { targetOrigin: hostOrigin }), - to: Messenger.fromWindow(win, { targetOrigin: hostOrigin }), - waitForReady: true, - }), - ); + messenger = Messenger.bridge({ + from: Messenger.fromWindow(window, { targetOrigin: hostOrigin }), + to: Messenger.fromWindow(win, { targetOrigin: hostOrigin }), + waitForReady: true, + }); + + // Wire inbound listeners now that the bridge exists. They live for + // the lifetime of this handle (until `destroy()`). + messenger.on("rpc-response", handlers.onResponse); + messenger.on("__internal", handlers.onInternal); + messenger.on("close", handlers.onClose); messenger.send("__internal", { type: "init", @@ -516,12 +456,12 @@ export function popup(options: PopupOptions = {}): DialogFactory { theme, }); - // If the user closes the popup without acting, treat that as a - // rejection — the consumer's outstanding requests should reject. + // If the user closes the popup without acting, surface a `close` + // event so outstanding requests get rejected as user-rejections. pollClosed = setInterval(() => { if (win?.closed) { teardownPoll(); - bridge?.send("close", undefined); + messenger?.send("close", undefined); } }, 250); }, @@ -535,13 +475,28 @@ export function popup(options: PopupOptions = {}): DialogFactory { win = null; }, destroy() { - messenger.destroy(); + this.close(); + messenger?.destroy(); + messenger = null; }, async secure() { // Popups don't suffer from clickjacking — the wallet runs in its own // top-level window, fully visible to the user. So all gates pass. return { protocol: true, frame: true, host: true }; }, + syncRequest(request) { + // Optional-chained — mirrors Porto's `messenger?.send(...)`. If a + // caller forgets to `open()` first, the send is silently dropped + // rather than throwing; the request stays in the consumer's pending + // map and will be retried on the next mode switch. + messenger?.send("rpc-request", request); + }, + async waitForReady() { + if (!messenger) { + throw new Error("Popup not opened — call open() first"); + } + return messenger.waitForReady(); + }, }; return handle; @@ -551,29 +506,8 @@ export function popup(options: PopupOptions = {}): DialogFactory { // ---------- Noop (SSR / non-browser) ---------- function noopHandle(): DialogHandle { - const noopMessenger: Messenger.Bridge = { - on: () => () => { - /* no-op: SSR / non-browser environments have no message bus */ - }, - send: ( - topic: T, - payload: Messenger.Payload, - ) => ({ id: "", topic, payload }), - destroy: () => { - /* no-op: nothing to tear down */ - }, - ready: () => { - /* no-op: ready handshake never fires in SSR */ - }, - waitForReady: () => - new Promise(() => { - /* never resolves in SSR — caller is expected to abort on its own */ - }), - }; - return { mode: "iframe", - messenger: noopMessenger, open() { /* no-op: cannot mount a dialog without a window */ }, @@ -586,5 +520,12 @@ function noopHandle(): DialogHandle { async secure() { return { protocol: false, frame: false, host: false }; }, + syncRequest() { + /* no-op: no transport in SSR */ + }, + waitForReady() { + // Never resolves in SSR — caller is expected to abort on its own. + return new Promise(() => {}); + }, }; } diff --git a/packages/wallet-sdk/src/core/Wallet.ts b/packages/wallet-sdk/src/core/Wallet.ts index 87e3bc4..0f82262 100644 --- a/packages/wallet-sdk/src/core/Wallet.ts +++ b/packages/wallet-sdk/src/core/Wallet.ts @@ -91,19 +91,13 @@ export function createWallet(config: WalletConfig): Wallet { const pending = new Map(); let nextRequestId = 1; - function ensureActive(targetMode: Dialog.DialogMode): Dialog.DialogHandle { - if (active && activeMode === targetMode) return active; - // Tear down previous mode (if any) before swapping. - active?.destroy(); - const factory = targetMode === "iframe" ? iframeFactory : popupFactory; - active = factory({ host, theme }); - activeMode = targetMode; - bindMessengerListeners(active); - return active; - } - - function bindMessengerListeners(handle: Dialog.DialogHandle) { - handle.messenger.on("rpc-response", (response) => { + // Inbound callbacks the dialog factories wire into their messenger + // internally. We rebuild this struct per-handle so the closures always + // close over the correct `active` reference (after a mode swap, stale + // events from the previous transport are ignored because that handle's + // messenger is destroyed by `previous?.destroy()` in `switchMode`). + const handlers: Dialog.DialogHandlers = { + onResponse(response) { const p = pending.get(response.id); if (!p) return; pending.delete(response.id); @@ -115,13 +109,13 @@ export function createWallet(config: WalletConfig): Wallet { }), ); else p.resolve(response.result); - if (pending.size === 0) handle.close(); - }); - handle.messenger.on("__internal", (payload) => { + if (pending.size === 0) active?.close(); + }, + onInternal(payload) { if (payload.type === "switch") void switchMode(payload.mode); - }); - handle.messenger.on("close", () => { - handle.close(); + }, + onClose() { + active?.close(); // Reject every outstanding request with a user-rejected style error. for (const [id, p] of pending) { pending.delete(id); @@ -131,7 +125,17 @@ export function createWallet(config: WalletConfig): Wallet { }), ); } - }); + }, + }; + + function ensureActive(targetMode: Dialog.DialogMode): Dialog.DialogHandle { + if (active && activeMode === targetMode) return active; + // Tear down previous mode (if any) before swapping. + active?.destroy(); + const factory = targetMode === "iframe" ? iframeFactory : popupFactory; + active = factory({ host, theme, handlers }); + activeMode = targetMode; + return active; } async function switchMode(target: Dialog.DialogMode) { @@ -141,7 +145,7 @@ export function createWallet(config: WalletConfig): Wallet { next.open(); // Re-deliver pending requests against the new transport. for (const p of pending.values()) { - next.messenger.send("rpc-request", p.request); + next.syncRequest(p.request); } previous?.destroy(); } @@ -207,7 +211,7 @@ export function createWallet(config: WalletConfig): Wallet { // safe. If not, transparently downgrade to popup before sending. let targetHandle = handle; if (initialMode === "iframe") { - const ready = await handle.messenger.waitForReady(); + const ready = await handle.waitForReady(); const sec = await handle.secure(); const alwaysHeadless = isAlwaysHeadless(rpc, ready.methodPolicies); if (!alwaysHeadless && (!sec.protocol || !sec.frame)) { @@ -227,7 +231,7 @@ export function createWallet(config: WalletConfig): Wallet { return new Promise((resolve, reject) => { pending.set(id, { request: rpc, resolve, reject }); - targetHandle.messenger.send("rpc-request", rpc); + targetHandle.syncRequest(rpc); }); } diff --git a/packages/wallet-sdk/src/exports/index.ts b/packages/wallet-sdk/src/exports/index.ts index 3ec48ae..84d7418 100644 --- a/packages/wallet-sdk/src/exports/index.ts +++ b/packages/wallet-sdk/src/exports/index.ts @@ -1,6 +1,7 @@ export type { DialogFactory, DialogHandle, + DialogHandlers, DialogMode, IframeOptions, PopupOptions, diff --git a/packages/wallet-sdk/test/src/Dialog.test.ts b/packages/wallet-sdk/test/src/Dialog.test.ts index 605d11a..6df5174 100644 --- a/packages/wallet-sdk/test/src/Dialog.test.ts +++ b/packages/wallet-sdk/test/src/Dialog.test.ts @@ -1,30 +1,53 @@ import { describe, expect, it, vi } from "vitest"; import * as Dialog from "../../src/core/Dialog.js"; +function noopHandlers(): Dialog.DialogHandlers { + return { + onResponse: vi.fn(), + onInternal: vi.fn(), + onClose: vi.fn(), + }; +} + describe("popup factory", () => { - it("does not throw when listeners or sends are issued before open()", () => { - const handle = Dialog.popup()({ host: "https://wallet.test" }); + it("does not throw when constructed; window.open only fires on open()", () => { + // The factory call must not invoke window.open or attempt to mount any + // bridge — those are deferred to open() so the user-gesture click is the + // first time we touch the popup window. + const openSpy = vi.spyOn(window, "open"); + const handle = Dialog.popup()({ + host: "https://wallet.test", + handlers: noopHandlers(), + }); - const listener = vi.fn(); - const off = handle.messenger.on("rpc-response", listener); - expect(typeof off).toBe("function"); + expect(handle.mode).toBe("popup"); + expect(openSpy).not.toHaveBeenCalled(); - const envelope = handle.messenger.send("rpc-request", { - id: 1, - jsonrpc: "2.0", - method: "eth_chainId", - }); - expect(envelope.id).toBeTruthy(); - expect(envelope.topic).toBe("rpc-request"); + handle.destroy(); + openSpy.mockRestore(); + }); - off(); + it("syncRequest before open() is a no-op", () => { + // Mirrors Porto's `messenger?.send(...)` — pre-open syncRequest silently + // drops rather than throwing, so a request still in the consumer's + // pending map gets re-delivered on the next mode switch instead of + // surfacing a noisy error. + const handle = Dialog.popup()({ + host: "https://wallet.test", + handlers: noopHandlers(), + }); + expect(() => + handle.syncRequest({ id: 1, jsonrpc: "2.0", method: "eth_chainId" }), + ).not.toThrow(); handle.destroy(); }); - it("rejects pre-open waitForReady() callers when the handle is destroyed", async () => { - const handle = Dialog.popup()({ host: "https://wallet.test" }); - const ready = handle.messenger.waitForReady(); + it("waitForReady before open() rejects", async () => { + const handle = Dialog.popup()({ + host: "https://wallet.test", + handlers: noopHandlers(), + }); + await expect(handle.waitForReady()).rejects.toThrow(/Popup not opened/); handle.destroy(); - await expect(ready).rejects.toThrow(/destroyed/); }); }); From 06c187d11e3440700ba19305dd4db1f6a31955de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?coffee=20=E2=98=95=EF=B8=8F?= Date: Sat, 9 May 2026 08:30:00 -0400 Subject: [PATCH 3/3] Fix wallet SDK popup bridge cleanup --- packages/wallet-sdk/src/core/Dialog.ts | 14 +-- packages/wallet-sdk/test/src/Dialog.test.ts | 98 +++++++++++++++++++++ 2 files changed, 107 insertions(+), 5 deletions(-) diff --git a/packages/wallet-sdk/src/core/Dialog.ts b/packages/wallet-sdk/src/core/Dialog.ts index 03bae83..2471ed1 100644 --- a/packages/wallet-sdk/src/core/Dialog.ts +++ b/packages/wallet-sdk/src/core/Dialog.ts @@ -419,6 +419,10 @@ export function popup(options: PopupOptions = {}): DialogFactory { return; } + teardownPoll(); + messenger?.destroy(); + messenger = null; + const features = resolvedType === "popup" ? `width=${size.width},height=${size.height},left=${ @@ -456,12 +460,12 @@ export function popup(options: PopupOptions = {}): DialogFactory { theme, }); - // If the user closes the popup without acting, surface a `close` - // event so outstanding requests get rejected as user-rejections. + // If the user closes the popup without acting, surface the close + // locally so outstanding requests get rejected as user-rejections. pollClosed = setInterval(() => { if (win?.closed) { teardownPoll(); - messenger?.send("close", undefined); + handlers.onClose(); } }, 250); }, @@ -473,11 +477,11 @@ export function popup(options: PopupOptions = {}): DialogFactory { /* COOP severs the WindowProxy — popup closes itself anyway */ } win = null; + messenger?.destroy(); + messenger = null; }, destroy() { this.close(); - messenger?.destroy(); - messenger = null; }, async secure() { // Popups don't suffer from clickjacking — the wallet runs in its own diff --git a/packages/wallet-sdk/test/src/Dialog.test.ts b/packages/wallet-sdk/test/src/Dialog.test.ts index 6df5174..91b0eca 100644 --- a/packages/wallet-sdk/test/src/Dialog.test.ts +++ b/packages/wallet-sdk/test/src/Dialog.test.ts @@ -50,4 +50,102 @@ describe("popup factory", () => { await expect(handle.waitForReady()).rejects.toThrow(/Popup not opened/); handle.destroy(); }); + + it("notifies the local close handler when the user closes the popup", () => { + vi.useFakeTimers(); + const popupWindow = { + closed: false, + close: vi.fn(), + focus: vi.fn(), + postMessage: vi.fn(), + } as unknown as Window; + const openSpy = vi.spyOn(window, "open").mockReturnValue(popupWindow); + const handlers = noopHandlers(); + const handle = Dialog.popup()({ + host: "https://wallet.test", + handlers, + }); + + try { + handle.open(); + (popupWindow as Window & { closed: boolean }).closed = true; + vi.advanceTimersByTime(250); + + expect(handlers.onClose).toHaveBeenCalledTimes(1); + } finally { + handle.destroy(); + openSpy.mockRestore(); + vi.useRealTimers(); + } + }); + + it("destroys the current bridge before reopening a new popup", () => { + const removeSpy = vi.spyOn(window, "removeEventListener"); + const popupWindow = { + closed: false, + close: vi.fn(), + focus: vi.fn(), + postMessage: vi.fn(), + } as unknown as Window; + const openSpy = vi.spyOn(window, "open").mockReturnValue(popupWindow); + const handle = Dialog.popup()({ + host: "https://wallet.test", + handlers: noopHandlers(), + }); + + try { + handle.open(); + handle.close(); + const cleanupCountBeforeReopen = removeSpy.mock.calls.filter( + ([type]) => type === "message", + ).length; + + handle.open(); + + expect(cleanupCountBeforeReopen).toBeGreaterThan(0); + } finally { + handle.destroy(); + openSpy.mockRestore(); + removeSpy.mockRestore(); + } + }); + + it("cleans up a closed popup bridge before the poller observes it", () => { + const removeSpy = vi.spyOn(window, "removeEventListener"); + const firstPopup = { + closed: false, + close: vi.fn(), + focus: vi.fn(), + postMessage: vi.fn(), + } as unknown as Window; + const secondPopup = { + closed: false, + close: vi.fn(), + focus: vi.fn(), + postMessage: vi.fn(), + } as unknown as Window; + const openSpy = vi + .spyOn(window, "open") + .mockReturnValueOnce(firstPopup) + .mockReturnValueOnce(secondPopup); + const handle = Dialog.popup()({ + host: "https://wallet.test", + handlers: noopHandlers(), + }); + + try { + handle.open(); + (firstPopup as Window & { closed: boolean }).closed = true; + handle.open(); + + const listenerCleanupCount = removeSpy.mock.calls.filter( + ([type]) => type === "message", + ).length; + expect(listenerCleanupCount).toBeGreaterThan(0); + } finally { + handle.destroy(); + openSpy.mockRestore(); + removeSpy.mockRestore(); + } + }); });