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 };