From 55578ac478c6241051b8428fcb552c66c307412c Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 30 Aug 2026 00:38:41 -0400 Subject: [PATCH] chore: address Copilot review round 3 (#2192) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seeding the settings draft during render made `useSettingsDraft` call `resolveInitial` there, and App.tsx passes `lastPersistedSettings.resolve`. That resolver was neither pure nor current: - it could `recordsRef.current.delete(...)`, a shared-state mutation that a render React replays under StrictMode or abandons would run an unpredictable number of times; - it read `serversRef`, which is mirrored in a passive effect and so trails `servers` by one flush — so on exactly the render where a refreshed list arrives, it answered from the previous commit's list. That render is the one that seeds the draft. `resolve` now reads `servers` directly and only reads. Eviction moves to an effect: nothing depended on it happening at a particular moment, it only keeps a dead record from being re-checked on every later write. Reading `servers` also makes the supersession test fire a render earlier, which is the right direction — the record is being checked against a list that has genuinely been re-read. `resolve`'s identity now changes with the list, so the test harness delegates to the current hook result instead of handing back a snapshot from the first render; a snapshot would have closed over the initial list and made `setServers` look like a no-op. New test asserts `resolve` answers from the list of the render it is called in, verified to fail against the ref read. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev Signed-off-by: cliffhall --- .../hooks/useLastPersistedSettings.test.tsx | 46 +++++++++++++++++-- .../web/src/hooks/useLastPersistedSettings.ts | 46 +++++++++++++------ 2 files changed, 75 insertions(+), 17 deletions(-) diff --git a/clients/web/src/hooks/useLastPersistedSettings.test.tsx b/clients/web/src/hooks/useLastPersistedSettings.test.tsx index 5c222b03..87249b6b 100644 --- a/clients/web/src/hooks/useLastPersistedSettings.test.tsx +++ b/clients/web/src/hooks/useLastPersistedSettings.test.tsx @@ -51,17 +51,31 @@ interface Harness { * Hands the hook's API back to the test. The hook holds refs, so it has to be * read from inside a rendered component; `setServers` re-renders it with a new * list the way `useServers` does after a read. + * + * The returned object *delegates* rather than being the API from the first + * render. `resolve` is keyed on the current `servers` (#2192) instead of on a + * ref, so its identity changes with the list — handing back a snapshot would + * give every test a `resolve` closed over the initial list, and `setServers` + * would appear to do nothing. */ function harness(initial: ServerEntry[]): Harness { - let api: LastPersistedSettings | undefined; + let latest: LastPersistedSettings | undefined; function Probe({ servers }: { servers: ServerEntry[] }) { - api = useLastPersistedSettings(servers); + latest = useLastPersistedSettings(servers); return null; } const { rerender } = renderWithMantine(); - if (!api) throw new Error("hook did not render"); + if (!latest) throw new Error("hook did not render"); + const current = () => { + if (!latest) throw new Error("hook did not render"); + return latest; + }; return { - api, + api: { + begin: (serverId) => current().begin(serverId), + resolve: (serverId) => current().resolve(serverId), + lastWriteFailed: (serverId) => current().lastWriteFailed(serverId), + }, setServers: (next) => rerender(), }; } @@ -222,4 +236,28 @@ describe("useLastPersistedSettings", () => { setServers([]); expect(api.resolve("A")).toBeUndefined(); }); + it("answers from the list of the render it is called in", () => { + // `useSettingsDraft` seeds its draft during render (#2192), so `resolve` + // is called there. Reading the effect-mirrored `serversRef` would answer + // from the *previous* commit's list on exactly the render where a + // refreshed list arrives — which is the render that seeds the draft. + const before = entry("A", settings({ paginatedLists: false })); + const after = entry("A", settings({ paginatedLists: true })); + const answers: Array = []; + + function Probe({ servers }: { servers: ServerEntry[] }) { + const api = useLastPersistedSettings(servers); + answers.push(api.resolve("A")); + return null; + } + + const { rerender } = renderWithMantine(); + expect(answers.at(-1)).toBe(before.settings); + + answers.length = 0; + rerender(); + + expect(answers.length).toBeGreaterThan(0); + for (const answer of answers) expect(answer).toBe(after.settings); + }); }); diff --git a/clients/web/src/hooks/useLastPersistedSettings.ts b/clients/web/src/hooks/useLastPersistedSettings.ts index 86e7649c..06f231d8 100644 --- a/clients/web/src/hooks/useLastPersistedSettings.ts +++ b/clients/web/src/hooks/useLastPersistedSettings.ts @@ -145,16 +145,33 @@ export function useLastPersistedSettings( const outcomesRef = useRef>( new Map(), ); - // Read the list through a ref so a write pairs with the entry as it stands - // when it finishes, not the one captured when it was issued. Mirrored in an - // effect rather than assigned during render: writing a ref mid-render is an - // error under `react-hooks/refs`, and every reader runs from a settled - // promise or an event handler, long after the commit. + // `landed` reads the list through a ref so a write pairs with the entry as + // it stands when it *finishes*, not the one captured when it was issued. + // Mirrored in an effect rather than assigned during render: writing a ref + // mid-render is an error under `react-hooks/refs`, and this reader runs from + // a settled promise, long after the commit. `resolve` deliberately does not + // use it — see there. const serversRef = useRef(servers); useEffect(() => { serversRef.current = servers; }, [servers]); + // Drop records the list has moved past. This used to happen lazily inside + // `resolve`, which is no longer allowed: `useSettingsDraft` seeds its draft + // during render (#2192) and so calls `resolve` there, where a ref mutation + // would run an unpredictable number of times — React may replay a render + // under StrictMode or abandon it altogether. Nothing depends on the eviction + // happening at any particular moment; it only keeps a dead record from being + // re-checked on every later write for that server, so an effect is the right + // home for it. + useEffect(() => { + for (const [serverId, record] of recordsRef.current) { + if (record.entry !== servers.find((s) => s.id === serverId)) { + recordsRef.current.delete(serverId); + } + } + }, [servers]); + const begin = useCallback((serverId: string): SettingsWrite => { const sequence = ++nextSequenceRef.current; const pending = pendingRef.current.get(serverId) ?? new Set(); @@ -194,22 +211,25 @@ export function useLastPersistedSettings( [], ); + // Pure, and over `servers` rather than `serversRef`. Both properties are + // required because `useSettingsDraft` calls this during render to seed its + // draft (#2192): a render must not mutate shared state, and the ref trails + // the value it mirrors by one passive-effect flush, so reading it here would + // answer from the previous commit's list on exactly the render where a + // refreshed list arrives. Reading `servers` also makes the supersession test + // fire a render earlier, which is the direction that matters — a list that + // has genuinely been re-read is what the record is being checked against. const resolve = useCallback( (serverId: string): InspectorServerSettings | undefined => { - const entry = serversRef.current.find((s) => s.id === serverId); + const entry = servers.find((s) => s.id === serverId); const current = recordsRef.current.get(serverId); if (!current) return entry?.settings; // Identity, not deep equality: the question is whether the list has been // re-read since the write, not whether the values happen to match. - if (current.entry !== entry) { - // Superseded — drop it rather than re-checking a dead record on every - // later write for this server. - recordsRef.current.delete(serverId); - return entry?.settings; - } + if (current.entry !== entry) return entry?.settings; return current.written; }, - [], + [servers], ); return { begin, resolve, lastWriteFailed };