diff --git a/packages/runtime-tests/unit/focus-subscriber-leak.test.ts b/packages/runtime-tests/unit/focus-subscriber-leak.test.ts new file mode 100644 index 0000000..23e0e1e --- /dev/null +++ b/packages/runtime-tests/unit/focus-subscriber-leak.test.ts @@ -0,0 +1,112 @@ +import { expect, test, vi } from "vite-plus/test"; +import { createFocusController } from "@vue-tui/runtime/internal"; + +// Regression: createFocusController()'s subscribe() returned an unsubscribe that +// did `set.delete(fn)` but never removed the now-empty Set from the `subs` Map. +// useFocus() with no explicit id mints a fresh `__auto-N` id per mount, so every +// mount/unmount of a no-id focusable permanently leaked one empty-Set Map entry — +// unbounded growth over a long session. The fix drops the empty Set on the last +// unsubscribe. These tests reach the internal Map via the `__subscriberMapSize` +// probe on the controller (test-only; see internal.ts / render.ts). +// +// Imported from the built `@vue-tui/runtime/internal` dist (not source): render.ts +// transitively imports .vue SFCs, which the runtime-tests vitest config does not +// compile (no @vitejs/plugin-vue), so this must reach the factory via the dist. + +test("subscribing/unsubscribing many auto-id focusables does not grow the subscriber Map", () => { + const ctx = createFocusController(); + const baseline = ctx.__subscriberMapSize(); + expect(baseline).toBe(0); + + // Simulate 300 mount/unmount cycles of a no-id useFocus(): each mints a unique + // `__auto-N` id, subscribes once, then unsubscribes on unmount. + for (let i = 0; i < 300; i++) { + const unsubscribe = ctx.subscribe(`__auto-${i}`, () => {}); + unsubscribe(); + } + + // After the fix the Map returns to baseline; before the fix it had grown by 300. + expect(ctx.__subscriberMapSize()).toBe(baseline); +}); + +test("focus delivery still works: subscribe receives notify, and re-subscribe after the Set was dropped re-creates it", () => { + const ctx = createFocusController(); + const received: boolean[] = []; + + // Register a focusable and subscribe; activate so Tab/programmatic focus can land. + ctx.add("only", { autoFocus: false }); + ctx.activate("only"); + const unsubscribe = ctx.subscribe("only", (focused) => received.push(focused)); + expect(ctx.__subscriberMapSize()).toBe(1); + + // Programmatic focus delivers a `true` notification. + ctx.focus("only"); + expect(received).toEqual([true]); + + // Unsubscribe drops the empty Set (last subscriber gone). Blur first so the + // controller is back to no-active-focus before we re-subscribe (otherwise the + // re-subscriber would later see a `false` blur notification for the prior focus). + ctx.blur(); + unsubscribe(); + expect(ctx.__subscriberMapSize()).toBe(0); + + // Re-subscribing for the same id must re-create the Set and still deliver. + const received2: boolean[] = []; + ctx.subscribe("only", (focused) => received2.push(focused)); + expect(ctx.__subscriberMapSize()).toBe(1); + + // Re-focus to drive a fresh notification to the new subscriber. + ctx.focus("only"); + expect(received2).toEqual([true]); +}); + +test("a stale double-unsubscribe after re-subscribe does not drop the fresh subscriber's Set", () => { + // The empty-Set cleanup must only delete the Set the closure actually owns. + // Without the `subs.get(id) === set` identity guard, calling an old unsubscribe + // a second time (after a new subscribe(id) re-created the Set) would delete the + // FRESH Map entry and silently kill the new subscriber's notifications. + const ctx = createFocusController(); + + const unsubscribeOld = ctx.subscribe("x", () => {}); + unsubscribeOld(); // drops the old (now-empty) Set from the Map + expect(ctx.__subscriberMapSize()).toBe(0); + + const received: boolean[] = []; + ctx.subscribe("x", (focused) => received.push(focused)); // fresh Set + expect(ctx.__subscriberMapSize()).toBe(1); + + // Stale call to the OLD unsubscribe — must be a no-op for the fresh Set. + unsubscribeOld(); + expect(ctx.__subscriberMapSize()).toBe(1); + + // The fresh subscriber still receives notifications. + ctx.add("x", { autoFocus: false }); + ctx.activate("x"); + ctx.focus("x"); + expect(received).toEqual([true]); +}); + +test("a Set with multiple live subscribers is kept until the last one unsubscribes", () => { + // Duplicate-id case: two consumers share one id's Set. Removing one must not + // silence the survivor, and the Map entry must persist while either is live. + const ctx = createFocusController(); + const a = vi.fn(); + const b = vi.fn(); + + const unsubA = ctx.subscribe("dup", a); + const unsubB = ctx.subscribe("dup", b); + expect(ctx.__subscriberMapSize()).toBe(1); + + unsubA(); + // b is still live → Set (and Map entry) must remain. + expect(ctx.__subscriberMapSize()).toBe(1); + + ctx.add("dup", { autoFocus: false }); + ctx.activate("dup"); + ctx.focus("dup"); + expect(a).not.toHaveBeenCalled(); + expect(b).toHaveBeenCalledWith(true); + + unsubB(); + expect(ctx.__subscriberMapSize()).toBe(0); +}); diff --git a/packages/runtime/src/internal.ts b/packages/runtime/src/internal.ts index ddfdf5c..44fb33e 100644 --- a/packages/runtime/src/internal.ts +++ b/packages/runtime/src/internal.ts @@ -26,3 +26,7 @@ export { INTERNAL_FRAME_SINK, type FrameSink } from "./io/frame-sink.ts"; // runtime-tests vitest config does not compile (no @vitejs/plugin-vue), so a // pure-function test of this helper must reach it through the built dist. export { messageForNonError } from "./components/error-overview.ts"; +// Exposed for unit testing the focus-subscriber leak fix: the returned controller +// carries a `__subscriberMapSize()` probe so a test can assert empty subscriber +// Sets are dropped on unsubscribe. See render.ts createFocusController. +export { createFocusController, type FocusControllerForTest } from "./render.ts"; diff --git a/packages/runtime/src/render.ts b/packages/runtime/src/render.ts index d2aca56..5bff82b 100644 --- a/packages/runtime/src/render.ts +++ b/packages/runtime/src/render.ts @@ -1530,7 +1530,14 @@ interface Focusable { isActive: boolean; } -function createFocusController(): FocusContext { +// Test-only probe: lets a unit test observe the internal `subs` Map size to prove +// auto-id focusables don't leak empty Sets (see focus-subs-leak test). The double +// underscore + this comment mark it internal; nothing in production reads it. +export interface FocusControllerForTest extends FocusContext { + __subscriberMapSize: () => number; +} + +export function createFocusController(): FocusControllerForTest { const focusables: Focusable[] = []; const subs = new Map void>>(); let activeFocusable: Focusable | null = null; @@ -1638,6 +1645,11 @@ function createFocusController(): FocusContext { setActiveFocusable(entry); } }, + // remove() intentionally does NOT touch `subs`: a consumer's unsubscribe + // (see subscribe()) is what frees its Set entry, and useFocus unsubscribes + // before calling remove(). Deleting the Set here would also break duplicate + // ids — two components sharing one id share one Set, and removing one entry + // must not silence the survivor's focus notifications. remove(id) { const removingActive = activeFocusable?.id === id; for (let i = focusables.length - 1; i >= 0; i--) { @@ -1669,11 +1681,27 @@ function createFocusController(): FocusContext { subs.set(id, set); } set.add(fn); - return () => set!.delete(fn); + return () => { + set!.delete(fn); + // Drop the now-empty Set so the Map doesn't accumulate dead entries. + // useFocus() with no explicit id mints a fresh `__auto-N` id per mount, + // so without this every mount/unmount cycle would leak one empty Set — + // unbounded growth over a long session. A later subscribe() for the same + // id re-creates the Set (the `if (!set)` branch above), so re-subscription + // still works. + // + // `subs.get(id) === set` keeps this idempotent across a re-subscribe: a + // stale call to THIS closure (e.g. unsubscribe invoked twice) must not + // delete a fresh Set created by a later subscribe(id) for the same id — + // only the Set this closure actually owns is eligible for removal. + if (set!.size === 0 && subs.get(id) === set) subs.delete(id); + }; }, }; - return ctx; + return Object.assign(ctx, { + __subscriberMapSize: () => subs.size, + }); } // --- Stdin controller ----------------------------------------------------