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