fix(runtime): drop empty focus-subscriber sets so auto-id focusables don't leak (#196)
createFocusController()'s subscribe() returned an unsubscribe that did `set.delete(fn)` but never removed the now-empty Set from the `subs` Map, and remove(id) never touched `subs` either. 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 (300 mount/unmount cycles leaked 300 empty Sets). The unsubscribe closure now drops the Set once its last subscriber leaves, guarded by `subs.get(id) === set` so a stale double-unsubscribe after a re-subscribe can't delete the fresh subscriber's Set (idempotency preserved). remove() is left untouched on purpose: useFocus unsubscribes before calling it, and deleting a Set with live subscribers would silence duplicate-id focus delivery. createFocusController + a test-only `__subscriberMapSize()` probe are exposed via the ./internal entry so a unit test can assert the Map stays flat across 300 cycles, focus delivery still works (notify + re-subscribe re-creates the Set), stale double-unsubscribe is a no-op, and multi-subscriber Sets are retained until the last unsubscribe. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -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";
|
||||
|
||||
@@ -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<string, Set<(focused: boolean) => 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 ----------------------------------------------------
|
||||
|
||||
Reference in New Issue
Block a user