diff --git a/README.md b/README.md index 0abe3cb..42aa987 100644 --- a/README.md +++ b/README.md @@ -168,9 +168,13 @@ crypto Web Worker (that's the correct cleanup when the element is genuinely being removed). If your host app re-renders its own DOM by resetting an ancestor's `innerHTML` — a common pattern in hand-rolled UI without a framework's reconciler — and the widget happens to be a descendant of that -subtree, this **silently kills the in-progress session**: `getPublicKey()` +subtree, this **kills the in-progress session**: `getPublicKey()` that already resolved before the render still looks fine, but every call after -that hangs forever, because the worker is gone and nothing ever replies. +that fails. As of the unmount guardrail, that failure is now loud rather +than silent — the element logs a `console.error` on disconnect-while-active, +and every later `signEvent`/`getPublicKey` **rejects** with a message naming +the unmount as the cause, instead of hanging forever. It's still a broken +session, though; the fix below is unchanged. Mount `` into a container that your own re-renders never replace (toggle visibility with the `hidden` attribute instead of removing it from the DOM), not into a template string that gets thrown away and @@ -191,8 +195,12 @@ a genuine remove-and-recreate. The worker still gets terminated, and because `connectedCallback` doesn't construct a new `WorkerClient` (that only happens once, in the constructor), the element is left reusing a now-dead worker: every subsequent message — `configure`, `login`, -everything — silently hangs forever with no error, since `postMessage` on -a terminated `Worker` is a no-op. BitUnlock hit exactly this while trying +everything — fails, since `postMessage` on a terminated `Worker` is a +no-op. The client now marks itself torn-down on `terminate()`, so those +calls **reject immediately** with a clear teardown message (and the removal +logged a `console.error`) rather than hanging forever with no error — but +the element is still stuck on a dead worker, so this is a diagnosis, not a +recovery. BitUnlock hit exactly this while trying to work around the innerHTML gotcha above: it kept `#bitloginMount` static but used `replaceWith()` to relocate it into a per-step placeholder, believing a move (vs. removal) would spare the worker. It doesn't. The fix diff --git a/docs/TODO.md b/docs/TODO.md index e9907d7..65b919e 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -386,12 +386,17 @@ how much they cost. ### Medium -- **The unmount-kills-the-worker footfooter deserves a guardrail.** The docs - warn that removing `` tears down the crypto worker and hangs - every later signing call with no error. I hit exactly this. Consider: (a) a - console.error when the element is disconnected while a session is active, and - (b) `signEvent` rejecting with a clear "widget was unmounted" error instead of - hanging forever. Silent infinite-hang is the worst failure mode. +- **[DONE 2026-08-11] The unmount-kills-the-worker footgun now has a guardrail.** + The docs warned that removing `` tears down the crypto worker + and hangs every later signing call with no error. **Shipped both asks:** + (a) `disconnectedCallback` now logs a `console.error` when the element is + removed while a session is active, naming the unmount and pointing at the + README "Integration gotcha"; and (b) `WorkerClient.terminate(reason)` records + the reason and `signEvent`/`getPublicKey`/etc. — whether in-flight at teardown + or issued afterward — reject with that message instead of hanging (the + `dead`-flag path already prevented the infinite hang; this makes the *cause* + legible and unifies the message). Covered by `worker/workerClient.test.ts`; + README gotcha paragraphs updated to say "rejects loudly," not "hangs forever." - **Styling hooks / CSS parts.** The widget renders in a shadow root, so a host can only frame it, not theme it. A few `::part()` hooks (button, panel, text) would let it match a host design system instead of looking pasted in. Tessera diff --git a/packages/widget/src/element.ts b/packages/widget/src/element.ts index 7212821..d831850 100644 --- a/packages/widget/src/element.ts +++ b/packages/widget/src/element.ts @@ -238,9 +238,30 @@ export class BitLoginAuthElement extends HTMLElement { disconnectedCallback(): void { if (this.successDismissTimer !== null) clearTimeout(this.successDismissTimer); + // Captured before clearActiveSession() below wipes the signal. + const hadActiveSession = Boolean(this.session || this.altSigner); clearActiveSession(this); this.releaseSigner(); - this.worker.terminate(); + // Removing from the DOM tears down its crypto worker. That's + // correct on a genuine teardown, but a host that re-renders its surrounding + // markup (a framework diff, an htmx swap, an innerHTML assignment) can remove + // and re-insert the element incidentally -- and then every later + // signEvent/getPublicKey call rejects, which is baffling without the cause. + // Say so loudly rather than letting the app hit silent rejections. The fix is + // to mount the element into a container the host's re-renders never replace + // (README "Integration gotcha"). + if (hadActiveSession) { + console.error( + "[BitLogin] was removed from the DOM while a session was active. " + + "Its crypto worker is now torn down, so later signEvent/getPublicKey calls will reject. " + + "If this was an incidental re-render, mount the element in a stable container your re-renders " + + "don't replace; see the BitLogin README, \"Integration gotcha\"." + ); + } + this.worker.terminate( + "BitLogin's element was removed from the DOM, tearing down its signer. " + + "Re-mount the widget or reload the page to sign in again." + ); } // ---- Public API mirroring window.nostr, scoped to this element instance ---- diff --git a/packages/widget/src/worker/workerClient.test.ts b/packages/widget/src/worker/workerClient.test.ts new file mode 100644 index 0000000..7302241 --- /dev/null +++ b/packages/widget/src/worker/workerClient.test.ts @@ -0,0 +1,55 @@ +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { WorkerClient } from "./workerClient.js"; + +/** Minimal stand-in for a real Worker: records posted messages but never + * answers, so a call stays pending until the client tears it down -- exactly + * the state an unmounted leaves behind. */ +class FakeWorker { + static instances: FakeWorker[] = []; + listeners: Record void)[]> = {}; + posted: unknown[] = []; + terminated = false; + constructor(_url: URL, _opts?: unknown) { + FakeWorker.instances.push(this); + } + addEventListener(type: string, cb: (event: unknown) => void): void { + (this.listeners[type] ??= []).push(cb); + } + postMessage(msg: unknown): void { + this.posted.push(msg); + } + terminate(): void { + this.terminated = true; + } +} + +describe("WorkerClient teardown", () => { + beforeEach(() => { + FakeWorker.instances = []; + (globalThis as unknown as { Worker: unknown }).Worker = FakeWorker; + }); + afterEach(() => { + delete (globalThis as unknown as { Worker?: unknown }).Worker; + }); + + it("rejects an in-flight call with the teardown reason instead of hanging", async () => { + const client = new WorkerClient(); + const pending = client.getPublicKey(); + client.terminate("widget was unmounted"); + await expect(pending).rejects.toThrow("widget was unmounted"); + expect(FakeWorker.instances[0]?.terminated).toBe(true); + }); + + it("rejects calls made after teardown immediately, with the same reason", async () => { + const client = new WorkerClient(); + client.terminate("widget was unmounted"); + await expect(client.getPublicKey()).rejects.toThrow("widget was unmounted"); + }); + + it("falls back to a clear default reason when terminate() is given none", async () => { + const client = new WorkerClient(); + const pending = client.signEvent({ kind: 1, content: "hi", tags: [] } as never); + client.terminate(); + await expect(pending).rejects.toThrow(/torn down|reload the page/i); + }); +}); diff --git a/packages/widget/src/worker/workerClient.ts b/packages/widget/src/worker/workerClient.ts index 7284a3e..010c534 100644 --- a/packages/widget/src/worker/workerClient.ts +++ b/packages/widget/src/worker/workerClient.ts @@ -53,6 +53,10 @@ export class WorkerClient { >(); private counter = 0; private dead = false; + /** Why the worker is gone, surfaced on every post-teardown rejection so a + * developer sees the CAUSE (usually an unmounted element), not just a bare + * failure. Overridable via terminate(reason). */ + private deadReason = "This BitLogin session was torn down; reload the page to sign in again."; /** Set by the element to receive unsolicited worker frames (auth_url). */ onNotification: ((notification: WorkerNotification) => void) | null = null; @@ -120,7 +124,7 @@ export class WorkerClient { timeoutMs = CALL_TIMEOUT_MS ): Promise { if (this.dead) { - return Promise.reject(new Error("This BitLogin session was torn down; reload the page to sign in again.")); + return Promise.reject(new Error(this.deadReason)); } const id = `${Date.now().toString(36)}-${(this.counter++).toString(36)}`; return new Promise((resolve, reject) => { @@ -248,11 +252,14 @@ export class WorkerClient { return this.call("logout", {}); } - terminate(): void { + terminate(reason?: string): void { this.dead = true; + if (reason) this.deadReason = reason; this.worker.terminate(); // terminate() silently kills in-flight work; without this every caller - // waits forever on a worker that no longer exists. - this.failAll("The BitLogin crypto worker was terminated."); + // waits forever on a worker that no longer exists. Reject with the same + // reason later calls get, so an in-flight signEvent and a subsequent one + // fail identically and legibly. + this.failAll(this.deadReason); } }