chore: address Copilot review round 3 (#2192)

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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev
Signed-off-by: cliffhall <cliff@futurescale.com>

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(<Probe servers={initial} />);
-  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(<Probe servers={next} />),
   };
 }
@@ -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<InspectorServerSettings | undefined> = [];
+
+    function Probe({ servers }: { servers: ServerEntry[] }) {
+      const api = useLastPersistedSettings(servers);
+      answers.push(api.resolve("A"));
+      return null;
+    }
+
+    const { rerender } = renderWithMantine(<Probe servers={[before]} />);
+    expect(answers.at(-1)).toBe(before.settings);
+
+    answers.length = 0;
+    rerender(<Probe servers={[after]} />);
+
+    expect(answers.length).toBeGreaterThan(0);
+    for (const answer of answers) expect(answer).toBe(after.settings);
+  });
 });

clients/web/src/hooks/useLastPersistedSettings.ts

@@ -145,16 +145,33 @@ export function useLastPersistedSettings(
   const outcomesRef = useRef<Map<string, { sequence: number; ok: boolean }>>(
     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<number>();
@@ -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 };