fix(web): roll a failed settings write back to what actually landed

The pagination toggle read its rollback baseline off the `servers` entry,
which only advances on a successful `GET /api/servers`. Once a list read
fails that entry describes disk as it was before every write made since,
so the #1721 rollback reverted to a value nothing on disk holds — and
clearing the optimistic override fell back to the same stale entry, so
the UI was wrong from both directions.

Remember the last settings write known to have landed (a new gated hook,
`useLastPersistedSettings`) and roll back to that instead, keyed to the
`servers` entry it was derived from so a fresh list read supersedes it.
Both settings writers — the toggle and the settings modal's debounced
persist — feed the one record.

Closes #2089

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>

clients/web/src/App.test.tsx

@@ -2423,6 +2423,48 @@ describe("App paginated list pagination toggle (#1721)", () => {
     };
     expect(lastPush?.paginatedLists).toBeFalsy();
   });
+
+  it("rolls back to the last write that landed, not to a stale list entry (#2089)", async () => {
+    // Two toggles. The first PUT lands but the list reload behind it fails, so
+    // the `servers` entry keeps describing disk as it was *before* it — which
+    // this suite models exactly, because `useServers` is mocked to return the
+    // same entry forever. The second PUT fails outright and rolls back.
+    //
+    // Reverting to the `servers` entry there lands on `false`, the value from
+    // before the first toggle, contradicting what is on disk. The baseline has
+    // to come from the write that landed instead.
+    //
+    // One failed toggle cannot distinguish the two baselines — they only differ
+    // once a write has landed that the list never caught up with — so the test
+    // needs the full sequence.
+    const user = userEvent.setup();
+    renderWithMantine(<App />);
+    await user.click(screen.getByText("connect"));
+    await waitFor(() => expect(clientInstances).toHaveLength(1));
+
+    await user.click(screen.getByText("paginated-on"));
+    await waitFor(() =>
+      expect(updateServerSettingsSpy).toHaveBeenCalledWith(
+        "A",
+        expect.objectContaining({ paginatedLists: true }),
+      ),
+    );
+
+    updateServerSettingsSpy.mockRejectedValueOnce(new Error("disk full"));
+    await user.click(screen.getByText("paginated-off"));
+
+    // Back to the value the first toggle put on disk, not to the pre-toggle one.
+    await waitFor(() =>
+      expect(screen.getByTestId("tools-paginated")).toHaveTextContent("true"),
+    );
+    const client = clientInstances[0] as EventTarget & {
+      setServerSettings: ReturnType<typeof vi.fn>;
+    };
+    const lastPush = client.setServerSettings.mock.calls.at(-1)?.[0] as {
+      paginatedLists?: boolean;
+    };
+    expect(lastPush?.paginatedLists).toBe(true);
+  });
 });
 
 // A background command runs through `runCommandInBackground`, which owns the

clients/web/src/App.tsx

@@ -110,6 +110,7 @@ import { usePagedTools } from "@inspector/core/react/usePagedTools.js";
 import { usePagedPrompts } from "@inspector/core/react/usePagedPrompts.js";
 import { usePagedResources } from "@inspector/core/react/usePagedResources.js";
 import { usePaginatedList } from "./hooks/usePaginatedList";
+import { useLastPersistedSettings } from "./hooks/useLastPersistedSettings";
 import type { ListPaginationControlsProps } from "./components/elements/ListPaginationControls/ListPaginationControls";
 import { useManagedResourceTemplates } from "@inspector/core/react/useManagedResourceTemplates.js";
 import { useManagedRequestorTasks } from "@inspector/core/react/useManagedRequestorTasks.js";
@@ -1159,6 +1160,10 @@ function App() {
     error: pagedResourcesLoadError,
     loadPage: loadResourcesPage,
   } = usePagedResources(inspectorClient, pagedResourcesState);
+  // 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();
   // The active server's persisted paginated setting drives the display mode.
   // The sidebar toggle edits it (optimistically, below) and persists it.
   const persistedPaginatedLists =
@@ -3730,7 +3735,15 @@ function App() {
       setPaginatedListsOverride(value);
       const current = servers.find((s) => s.id === activeServerId);
       if (!current || activeServerId === undefined) return;
-      const prevSettings = current.settings ?? EMPTY_SETTINGS;
+      // Not `current.settings` directly: that entry only advances on a
+      // successful list read, so once one has failed it describes disk as it
+      // was *before* the writes made since. Roll back to the last write known
+      // to have landed while that is the fresher account (#2089).
+      const prevSettings = lastPersistedSettings.resolve(
+        activeServerId,
+        current,
+        current.settings ?? EMPTY_SETTINGS,
+      );
       const next: InspectorServerSettings = {
         ...prevSettings,
         paginatedLists: value,
@@ -3760,23 +3773,35 @@ function App() {
       void refreshingPersist(updateServerSettings, refreshInitialConfig)(
         activeServerId,
         next,
-      ).catch((err: unknown) => {
-        // Persist failed: revert the optimistic override (the effect only
-        // clears it when the persisted value changes, which won't happen here)
-        // and roll the live client setting back, so the UI and client reflect
-        // the value that's actually on disk rather than the failed edit (#1721).
-        setPaginatedListsOverride(null);
-        inspectorClient?.setServerSettings(prevSettings);
-        notifications.show({
-          title: "Failed to save pagination setting",
-          message: err instanceof Error ? err.message : String(err),
-          color: "red",
+      )
+        .then(() => {
+          // This value is on disk now. Remember it as the rollback baseline for
+          // 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);
+        })
+        .catch((err: unknown) => {
+          // Persist failed: revert the optimistic override and roll the live
+          // client setting back, so the UI and client reflect the value that's
+          // actually on disk rather than the failed edit (#1721). The override
+          // is set to the baseline rather than cleared: clearing it falls back
+          // to `persistedPaginatedLists`, read from the same `servers` entry
+          // that may be stale, which would show the same wrong value from the
+          // other direction (#2089).
+          setPaginatedListsOverride(prevSettings.paginatedLists ?? false);
+          inspectorClient?.setServerSettings(prevSettings);
+          notifications.show({
+            title: "Failed to save pagination setting",
+            message: err instanceof Error ? err.message : String(err),
+            color: "red",
+          });
         });
-      });
     },
     [
       servers,
       activeServerId,
+      lastPersistedSettings,
       inspectorClient,
       updateServerSettings,
       refreshInitialConfig,
@@ -4143,7 +4168,22 @@ function App() {
     // unit rather than an inline `await …; refresh()` because this file is
     // outside the coverage gate, so wiring written here is tested by nothing
     // (#1950 review r17).
-    onPersist: refreshingPersist(updateServerSettings, refreshInitialConfig),
+    onPersist: async (id: string, value: InspectorServerSettings) => {
+      const entry = servers.find((s) => s.id === id);
+      await refreshingPersist(updateServerSettings, refreshInitialConfig)(
+        id,
+        value,
+      );
+      // 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);
+    },
     // Surface failures via toast — the modal usually closes
     // immediately on user dismiss, so a silent fail-on-flush would
     // leave the user thinking their last edits saved when they

clients/web/src/hooks/useLastPersistedSettings.test.tsx

@@ -0,0 +1,96 @@
+import { describe, it, expect } from "vitest";
+import type {
+  InspectorServerSettings,
+  ServerEntry,
+} from "@inspector/core/mcp/types.js";
+import { renderWithMantine } from "../test/renderWithMantine";
+import {
+  useLastPersistedSettings,
+  type LastPersistedSettings,
+} from "./useLastPersistedSettings";
+
+const settings = (
+  overrides: Partial<InspectorServerSettings> = {},
+): InspectorServerSettings => ({
+  headers: [],
+  env: [],
+  metadata: [],
+  connectionTimeout: 0,
+  requestTimeout: 0,
+  taskTtl: 0,
+  autoRefreshOnListChanged: false,
+  paginatedLists: false,
+  maxFetchRequests: 0,
+  roots: [],
+  ...overrides,
+});
+
+/**
+ * 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.
+ */
+const entry = (settingsNode?: InspectorServerSettings): ServerEntry => ({
+  id: "A",
+  name: "PlotRocket",
+  config: { type: "stdio", command: "node" },
+  connection: { status: "disconnected" },
+  settings: settingsNode,
+});
+
+/**
+ * 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.
+ */
+function harness(): LastPersistedSettings {
+  let api: LastPersistedSettings | undefined;
+  function Probe() {
+    api = useLastPersistedSettings();
+    return null;
+  }
+  renderWithMantine(<Probe />);
+  if (!api) throw new Error("hook did not render");
+  return api;
+}
+
+describe("useLastPersistedSettings", () => {
+  it("falls back to the caller's value when no write has been recorded", () => {
+    const { resolve } = harness();
+    const fallback = settings({ paginatedLists: true });
+    expect(resolve("A", entry(), fallback)).toBe(fallback);
+  });
+
+  it("returns the recorded write while the entry has not been re-read", () => {
+    const { record, resolve } = harness();
+    const seen = entry();
+    const written = settings({ paginatedLists: true });
+    record("A", written, seen);
+    expect(resolve("A", seen, 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 fallback = settings();
+    expect(resolve("B", seen, fallback)).toBe(fallback);
+  });
+
+  it("stops trusting the record once a fresh list read replaces the entry", () => {
+    const { record, resolve } = harness();
+    record("A", settings({ paginatedLists: true }), entry());
+    // 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);
+  });
+
+  it("keeps only the most recent write", () => {
+    const { record, resolve } = harness();
+    const seen = entry();
+    record("A", settings({ paginatedLists: true }), seen);
+    const later = settings({ autoRefreshOnListChanged: true });
+    record("A", later, seen);
+    expect(resolve("A", seen, settings())).toBe(later);
+  });
+});

clients/web/src/hooks/useLastPersistedSettings.ts

@@ -0,0 +1,96 @@
+import { useCallback, 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
+ * rather than to a `servers` entry that has gone stale (#2089).
+ *
+ * The `servers` list only advances when a `GET /api/servers` succeeds. Once a
+ * list read fails, every entry in it keeps describing disk *as of the last
+ * successful read* — while writes issued since then have changed disk. A
+ * rollback that reads its baseline from such an entry reverts to a value
+ * nothing on disk holds, and the UI contradicts the file until the next
+ * successful read. The reproduction is two toggles: the first write lands but
+ * 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.
+ *
+ * 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.
+ */
+export interface LastPersistedSettings {
+  /**
+   * Record a settings write that reached disk. `entry` is the `servers` entry
+   * the write was derived from.
+   */
+  record: (
+    serverId: string,
+    written: InspectorServerSettings,
+    entry: ServerEntry,
+  ) => 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.
+   */
+  resolve: (
+    serverId: string,
+    entry: ServerEntry,
+    fallback: InspectorServerSettings,
+  ) => InspectorServerSettings;
+}
+
+interface WriteRecord {
+  serverId: string;
+  written: InspectorServerSettings;
+  entry: ServerEntry;
+}
+
+export function useLastPersistedSettings(): LastPersistedSettings {
+  const recordRef = useRef<WriteRecord | null>(null);
+
+  const record = useCallback(
+    (
+      serverId: string,
+      written: InspectorServerSettings,
+      entry: ServerEntry,
+    ) => {
+      recordRef.current = { serverId, written, entry };
+    },
+    [],
+  );
+
+  const resolve = useCallback(
+    (
+      serverId: string,
+      entry: ServerEntry,
+      fallback: InspectorServerSettings,
+    ): InspectorServerSettings => {
+      const current = recordRef.current;
+      if (!current || current.serverId !== serverId) return fallback;
+      // 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;
+      return current.written;
+    },
+    [],
+  );
+
+  return { record, resolve };
+}