modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 2

fix(web): key the rollback baseline per server, and pair it at completion

Browse source 
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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-23 11:18:34 -04:00
commit e359368892
3 changed files with 144 additions and 82 deletions
Download patch file Download diff file Expand all files Collapse all files

16
clients/web/src/App.tsx
View file

@ -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

106
clients/web/src/hooks/useLastPersistedSettings.test.tsx
View file

@ -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(<Probe />);
const { rerender } = renderWithMantine(<Probe servers={initial} />);
if (!api) throw new Error("hook did not render");
return api;
return {
api,
setServers: (next) => rerender(<Probe servers={next} />),
};
}
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);
});
});

104
clients/web/src/hooks/useLastPersistedSettings.ts
View file

@ -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<WriteRecord | null>(null);
/**
* @param servers the current server list, as rendered
*/
export function useLastPersistedSettings(
servers: ServerEntry[],
): LastPersistedSettings {
const recordsRef = useRef<Map<string, WriteRecord>>(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;
},
[],