From e359368892be64918b70ca42a206816eddc2c760 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 23 Aug 2026 11:18:34 -0400 Subject: [PATCH] fix(web): key the rollback baseline per server, and pair it at completion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 1 on #2091. A single record slot let a save on one server discard the baseline a later failed write on another still needed — the settings modal opens for any server, connected or not, so that sequence reproduces #2089 through the very mechanism meant to prevent it. Records are now kept per server id. The record also paired with the `servers` entry captured when the write was *issued*, so a list read landing while the write was in flight was mistaken for one that happened after it and discarded a live record. It now pairs with the entry as of completion, read through a ref (mirrored in an effect — writing a ref during render is a `react-hooks/refs` error). The residual — a read whose response predates the write but arrives after it — needs a revision or ETag `useServers` does not expose, and is documented on the hook. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4 Signed-off-by: cliffhall --- clients/web/src/App.tsx | 16 +-- .../hooks/useLastPersistedSettings.test.tsx | 106 +++++++++++++----- .../web/src/hooks/useLastPersistedSettings.ts | 104 ++++++++++------- 3 files changed, 144 insertions(+), 82 deletions(-) diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 53d64e10..6855faed 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -1163,7 +1163,7 @@ function App() { // What every settings write that landed on disk actually wrote, so a later // failed write can be rolled back to that rather than to a `servers` entry // frozen at the last successful list read (#2089). - const lastPersistedSettings = useLastPersistedSettings(); + const lastPersistedSettings = useLastPersistedSettings(servers); // The active server's persisted paginated setting drives the display mode. // The sidebar toggle edits it (optimistically, below) and persists it. const persistedPaginatedLists = @@ -3741,7 +3741,6 @@ function App() { // to have landed while that is the fresher account (#2089). const prevSettings = lastPersistedSettings.resolve( activeServerId, - current, current.settings ?? EMPTY_SETTINGS, ); const next: InspectorServerSettings = { @@ -3779,7 +3778,7 @@ function App() { // whatever is written next, since the `servers` entry it was derived // from will keep describing the old value if the reload behind this // write — or any later one — fails (#2089). - lastPersistedSettings.record(activeServerId, next, current); + lastPersistedSettings.record(activeServerId, next); }) .catch((err: unknown) => { // Persist failed: revert the optimistic override and roll the live @@ -4169,7 +4168,6 @@ function App() { // outside the coverage gate, so wiring written here is tested by nothing // (#1950 review r17). onPersist: async (id: string, value: InspectorServerSettings) => { - const entry = servers.find((s) => s.id === id); await refreshingPersist(updateServerSettings, refreshInitialConfig)( id, value, @@ -4177,12 +4175,10 @@ function App() { // Recorded for the same reason the pagination toggle records its own // write (#2089): this is what disk holds now, and the `servers` entry it // was edited from will keep describing the previous value for as long as - // list reads keep failing. Both settings writers feed the one record, so - // a rollback in either takes the most recent write, not just the most - // recent write *of its own kind*. No entry means the modal was opened - // against a server the list doesn't have, and there is nothing for a - // later read to supersede. - if (entry) lastPersistedSettings.record(id, value, entry); + // list reads keep failing. Both settings writers feed the same per-server + // record, so a rollback in either takes the most recent write for that + // server, not just the most recent write *of its own kind*. + lastPersistedSettings.record(id, value); }, // Surface failures via toast — the modal usually closes // immediately on user dismiss, so a silent fail-on-flush would diff --git a/clients/web/src/hooks/useLastPersistedSettings.test.tsx b/clients/web/src/hooks/useLastPersistedSettings.test.tsx index f9e3251a..40daa2da 100644 --- a/clients/web/src/hooks/useLastPersistedSettings.test.tsx +++ b/clients/web/src/hooks/useLastPersistedSettings.test.tsx @@ -26,71 +26,115 @@ const settings = ( }); /** - * One `servers` entry, as a fresh object each call — the point of most of these - * tests is object identity, so a shared fixture would defeat them. + * One `servers` entry, a fresh object per call — most of these tests turn on + * object identity, so a shared fixture would defeat them. A list rebuilt from + * fresh calls is what a successful `GET /api/servers` produces. */ -const entry = (settingsNode?: InspectorServerSettings): ServerEntry => ({ - id: "A", - name: "PlotRocket", +const entry = ( + id: string, + settingsNode?: InspectorServerSettings, +): ServerEntry => ({ + id, + name: id, config: { type: "stdio", command: "node" }, connection: { status: "disconnected" }, settings: settingsNode, }); +interface Harness { + api: LastPersistedSettings; + /** Deliver a new server list, as a successful list read would. */ + setServers: (next: ServerEntry[]) => void; +} + /** - * Hands the hook's API back to the test. The hook holds a ref, so the value has - * to be read from inside a rendered component. + * 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. */ -function harness(): LastPersistedSettings { +function harness(initial: ServerEntry[]): Harness { let api: LastPersistedSettings | undefined; - function Probe() { - api = useLastPersistedSettings(); + function Probe({ servers }: { servers: ServerEntry[] }) { + api = useLastPersistedSettings(servers); return null; } - renderWithMantine(); + const { rerender } = renderWithMantine(); if (!api) throw new Error("hook did not render"); - return api; + return { + api, + setServers: (next) => rerender(), + }; } describe("useLastPersistedSettings", () => { it("falls back to the caller's value when no write has been recorded", () => { - const { resolve } = harness(); + const { api } = harness([entry("A")]); const fallback = settings({ paginatedLists: true }); - expect(resolve("A", entry(), fallback)).toBe(fallback); + expect(api.resolve("A", fallback)).toBe(fallback); }); it("returns the recorded write while the entry has not been re-read", () => { - const { record, resolve } = harness(); - const seen = entry(); + const { api } = harness([entry("A")]); const written = settings({ paginatedLists: true }); - record("A", written, seen); - expect(resolve("A", seen, settings())).toBe(written); + api.record("A", written); + expect(api.resolve("A", settings())).toBe(written); }); it("ignores a record made for a different server", () => { - const { record, resolve } = harness(); - const seen = entry(); - record("A", settings({ paginatedLists: true }), seen); + const { api } = harness([entry("A"), entry("B")]); + api.record("A", settings({ paginatedLists: true })); const fallback = settings(); - expect(resolve("B", seen, fallback)).toBe(fallback); + expect(api.resolve("B", fallback)).toBe(fallback); + }); + + it("keeps each server's record, so a save on B doesn't discard A's", () => { + // The settings modal can be opened for any server, connected or not, so + // this sequence is reachable: A's write lands while list reads are failing, + // B is saved from the modal, and only then does a write on A fail. A single + // slot would have lost A's baseline and reproduced #2089. + const { api } = harness([entry("A"), entry("B")]); + const writtenA = settings({ paginatedLists: true }); + api.record("A", writtenA); + api.record("B", settings({ autoRefreshOnListChanged: true })); + expect(api.resolve("A", settings())).toBe(writtenA); }); it("stops trusting the record once a fresh list read replaces the entry", () => { - const { record, resolve } = harness(); - record("A", settings({ paginatedLists: true }), entry()); + const { api, setServers } = harness([entry("A")]); + api.record("A", settings({ paginatedLists: true })); // A successful `GET /api/servers` rebuilds the list, so the entry is a new // object even when its values are unchanged. That is the signal the list // has caught up, and it is authoritative again from here. const fresh = settings(); - expect(resolve("A", entry(fresh), fresh)).toBe(fresh); + setServers([entry("A", fresh)]); + expect(api.resolve("A", fresh)).toBe(fresh); }); - it("keeps only the most recent write", () => { - const { record, resolve } = harness(); - const seen = entry(); - record("A", settings({ paginatedLists: true }), seen); + it("pairs the record with the entry as of the write, not of the render", () => { + // A list read that lands between a write being issued and completing must + // not be mistaken for one that happened after it. `record` is called on + // completion and pairs with the list as it stands then, so the read below + // — which precedes the write — does not invalidate the record. + const { api, setServers } = harness([entry("A")]); + setServers([entry("A")]); + const written = settings({ paginatedLists: true }); + api.record("A", written); + expect(api.resolve("A", settings())).toBe(written); + }); + + it("keeps only the most recent write for a server", () => { + const { api } = harness([entry("A")]); + api.record("A", settings({ paginatedLists: true })); const later = settings({ autoRefreshOnListChanged: true }); - record("A", later, seen); - expect(resolve("A", seen, settings())).toBe(later); + api.record("A", later); + expect(api.resolve("A", settings())).toBe(later); + }); + + it("falls back when the server has left the list entirely", () => { + const { api, setServers } = harness([entry("A")]); + api.record("A", settings({ paginatedLists: true })); + setServers([]); + const fallback = settings(); + expect(api.resolve("A", fallback)).toBe(fallback); }); }); diff --git a/clients/web/src/hooks/useLastPersistedSettings.ts b/clients/web/src/hooks/useLastPersistedSettings.ts index f8ca03c2..340a399d 100644 --- a/clients/web/src/hooks/useLastPersistedSettings.ts +++ b/clients/web/src/hooks/useLastPersistedSettings.ts @@ -1,12 +1,12 @@ -import { useCallback, useRef } from "react"; +import { useCallback, useEffect, useRef } from "react"; import type { InspectorServerSettings, ServerEntry, } from "@inspector/core/mcp/types.js"; /** - * Remembers the last server-settings write that is known to have reached disk, - * so a *later* failed write can be rolled back to what is actually persisted + * Remembers, per server, the last settings write known to have reached disk, so + * a *later* failed write can be rolled back to what is actually persisted * rather than to a `servers` entry that has gone stale (#2089). * * The `servers` list only advances when a `GET /api/servers` succeeds. Once a @@ -18,60 +18,77 @@ import type { * its list reload fails, the second write fails outright and reverts to the * pre-first-toggle value. * - * The record is not trusted indefinitely. It is paired with the `servers` entry - * the write was derived from, and a successful list read replaces that entry - * (the list is re-parsed from the response, so a fresh read always yields fresh - * objects) — which is the signal that the list has caught up with, or been - * overtaken by (an edit made outside the Inspector), our write. From there the - * entry is authoritative again and the record is ignored. The *entry* is the - * token rather than its `settings` node because a server that has never been - * configured carries no `settings` at all, and `undefined` cannot distinguish - * a re-read from a stale one. + * Records are kept **per server id**, not in a single slot. The settings modal + * can be opened for any server, independently of which one is connected, so a + * single slot would let a save on B discard the baseline a later failed write + * on A still needs — reproducing the very bug this hook exists to prevent. * - * Kept in a ref rather than state deliberately: nothing renders from it, it is - * read only inside callbacks at the moment a write is issued, and holding it in - * state would re-render the whole composition root on every settings save. + * A record is not trusted indefinitely. It is paired with the `servers` entry + * for that server **as of when the write completed**, and is dropped once the + * list carries a different entry object: a successful read rebuilds the list, + * so a fresh object is the signal that the entry has caught up with — or been + * overtaken by (an edit made outside the Inspector) — our write, and from there + * the entry is authoritative again. The pairing is taken at completion rather + * than at scheduling because a read that lands *while the write is in flight* + * would otherwise be mistaken for one that happened after it. + * + * One case the client cannot decide is left: a background read whose response + * was generated before the write landed but which arrives after it still looks + * newer here, and the record is dropped in its favour. Ordering that reliably + * needs a revision or ETag on `GET /api/servers` that `useServers` does not + * expose today. What the fallback yields in that case is the freshest + * successful read — the same value the rest of the UI renders from — rather + * than an arbitrary one. + * + * The list and the records are held in refs rather than state deliberately: + * nothing renders from either, they are read only inside callbacks at the + * moment a write completes or fails, and holding them in state would re-render + * the whole composition root on every settings save. */ export interface LastPersistedSettings { /** - * Record a settings write that reached disk. `entry` is the `servers` entry - * the write was derived from. + * Record a settings write that reached disk. Call it when the write + * *completes*, not when it is issued. */ - record: ( - serverId: string, - written: InspectorServerSettings, - entry: ServerEntry, - ) => void; + record: (serverId: string, written: InspectorServerSettings) => void; /** * The best available answer to "what is on disk for this server". Returns the - * recorded write while `entry` is still the object the write was derived - * from, and `fallback` (the caller's read of the entry) once a fresh list - * read has replaced it, or when no write has landed for that server this - * session. + * recorded write while the list still carries the entry that write was paired + * with, and `fallback` (the caller's read of the entry) once a fresh list read + * has replaced it, or when no write has landed for that server this session. */ resolve: ( serverId: string, - entry: ServerEntry, fallback: InspectorServerSettings, ) => InspectorServerSettings; } interface WriteRecord { - serverId: string; written: InspectorServerSettings; - entry: ServerEntry; + entry: ServerEntry | undefined; } -export function useLastPersistedSettings(): LastPersistedSettings { - const recordRef = useRef(null); +/** + * @param servers the current server list, as rendered + */ +export function useLastPersistedSettings( + servers: ServerEntry[], +): LastPersistedSettings { + const recordsRef = useRef>(new Map()); + // Read the list through a ref so `record` pairs with the entry as it stands + // when the write finishes, not the one captured when the write was issued. + // Mirrored in an effect rather than assigned during render: writing a ref + // mid-render is an error under `react-hooks/refs`, and the callbacks that + // read it all run from settled promises, long after the commit. + const serversRef = useRef(servers); + useEffect(() => { + serversRef.current = servers; + }, [servers]); const record = useCallback( - ( - serverId: string, - written: InspectorServerSettings, - entry: ServerEntry, - ) => { - recordRef.current = { serverId, written, entry }; + (serverId: string, written: InspectorServerSettings) => { + const entry = serversRef.current.find((s) => s.id === serverId); + recordsRef.current.set(serverId, { written, entry }); }, [], ); @@ -79,14 +96,19 @@ export function useLastPersistedSettings(): LastPersistedSettings { const resolve = useCallback( ( serverId: string, - entry: ServerEntry, fallback: InspectorServerSettings, ): InspectorServerSettings => { - const current = recordRef.current; - if (!current || current.serverId !== serverId) return fallback; + const current = recordsRef.current.get(serverId); + if (!current) return fallback; + const entry = serversRef.current.find((s) => s.id === serverId); // 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) return fallback; + 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 fallback; + } return current.written; }, [],