From 18b4b1c480d5e056f4b2dbb378395535f2269978 Mon Sep 17 00:00:00 2001 From: Kevin Rajan Date: Tue, 29 Sep 2026 18:47:40 -0500 Subject: [PATCH] [muse] fix(web): prune useLiveRefresh fallback view entries on unmount useLiveRefresh keeps a module-level lastRefreshedAtByView map keyed by view identity. When the caller passes no key, the identity is React's useId(), which is fresh on every remount, so each remount of an unkeyed consumer (e.g. the pull-requests route) leaked one entry that no later mount could ever look up again. The map's own comment called entries "never pruned", treating the useId as a stable view identity. Drop the fallback entry when its mount unmounts. Caller-supplied keys (e.g. pull-request::) name views that outlive their mounts, so those entries keep the existing behavior. Authored with AI assistance (Muse, Meta's Muse Spark) under the contributor's direction. --- .../src/hooks/useLiveRefresh.prune.test.tsx | 59 +++++++++++++++++++ apps/web/src/hooks/useLiveRefresh.ts | 23 +++++++- 2 files changed, 80 insertions(+), 2 deletions(-) create mode 100644 apps/web/src/hooks/useLiveRefresh.prune.test.tsx diff --git a/apps/web/src/hooks/useLiveRefresh.prune.test.tsx b/apps/web/src/hooks/useLiveRefresh.prune.test.tsx new file mode 100644 index 000000000000..9a454af8161e --- /dev/null +++ b/apps/web/src/hooks/useLiveRefresh.prune.test.tsx @@ -0,0 +1,59 @@ +// @vitest-environment jsdom +import { act } from "react"; +import { create, type ReactTestRenderer } from "react-test-renderer"; +import { afterEach, describe, expect, it } from "vite-plus/test"; + +import { getLiveRefreshTrackedViewCount, useLiveRefresh } from "./useLiveRefresh"; + +function UnkeyedProbe() { + useLiveRefresh(() => {}); + return null; +} + +function KeyedProbe({ viewKey }: { readonly viewKey: string }) { + useLiveRefresh(() => {}, { key: viewKey }); + return null; +} + +describe("useLiveRefresh view bookkeeping", () => { + let renderer: ReactTestRenderer | undefined; + afterEach(() => { + act(() => { + renderer?.unmount(); + }); + renderer = undefined; + }); + + it("drops the useId fallback entry when its mount unmounts", () => { + const before = getLiveRefreshTrackedViewCount(); + act(() => { + renderer = create(); + }); + expect(getLiveRefreshTrackedViewCount()).toBe(before + 1); + act(() => { + renderer?.unmount(); + }); + renderer = undefined; + // A useId names one mount: no later mount can look it up again, so keeping + // it would leak one entry per remount (e.g. every visit to the + // pull-requests route). + expect(getLiveRefreshTrackedViewCount()).toBe(before); + }); + + it("keeps a caller-supplied key across unmounts", () => { + const viewKey = `test-view-${Date.now()}`; + const before = getLiveRefreshTrackedViewCount(); + act(() => { + renderer = create(); + }); + expect(getLiveRefreshTrackedViewCount()).toBe(before + 1); + act(() => { + renderer?.unmount(); + }); + renderer = undefined; + // A caller-supplied key names a view that outlives its mounts (one panel + // showing a different pull request each time it opens), so its timestamp + // stays for the next arrival. + expect(getLiveRefreshTrackedViewCount()).toBe(before + 1); + }); +}); diff --git a/apps/web/src/hooks/useLiveRefresh.ts b/apps/web/src/hooks/useLiveRefresh.ts index 5c3fc3c81a2d..8e67d6821624 100644 --- a/apps/web/src/hooks/useLiveRefresh.ts +++ b/apps/web/src/hooks/useLiveRefresh.ts @@ -79,11 +79,21 @@ export function shouldRefreshOnInterval(input: { * When each view last read, kept outside React because a mount is exactly what it has to outlive: * a reader who navigates away and straight back mounts a fresh hook, and a timestamp that started * at zero would call that first read due. Keyed by whatever the caller calls the view, because two - * views on screen at once each owe their own reader an answer. Entries are one number and never - * pruned; there is one per view the session ever read. + * views on screen at once each owe their own reader an answer. Entries are one number; a + * caller-supplied key names a view that outlives its mounts, so those entries are never pruned. + * The useId fallback names one mount only, so its entry is dropped when that mount unmounts — + * no later mount can look it up again, and keeping it would leak one entry per remount. */ const lastRefreshedAtByView = new Map(); +/** + * How many view identities currently hold a last-read timestamp. The map is + * module state, so this is exposed for tests to observe pruning. + */ +export function getLiveRefreshTrackedViewCount(): number { + return lastRefreshedAtByView.size; +} + /** * When the reader last did anything. Shared rather than per view: a person is present in the * window, not in one component of it. Only tracked while a live view is mounted, and the handler @@ -130,6 +140,15 @@ export function useLiveRefresh( const fallbackViewId = useId(); const viewId = key ?? fallbackViewId; + // A useId fallback identifies this mount only: unlike a caller-supplied key it + // can never be looked up again after unmount, so its entry is dropped when the + // mount dies instead of accumulating one stale number per remount. + useEffect(() => { + return () => { + if (key === undefined) lastRefreshedAtByView.delete(fallbackViewId); + }; + }, [fallbackViewId, key]); + useEffect(() => { if (!enabled) return; const read = (now: number) => {