fix(web): re-apply the write that settles after an overlapping one failed

Review round 5 on #2091, both findings.

The other overlap order was still wrong. Write 1 (`true`) is in flight
when write 2 (`false`) fails, so the rollback resolves to the stale
entry's `false` — correct at that instant, nothing had landed. Write 1
then lands `true` and only touched refs, so if its own list reload had
failed too, no render would ever correct the UI or the live client while
disk held `true`.

Writes now report both outcomes: `landed` returns whether this write is
the settled state for its server — recorded, with nothing later still in
flight — and `failed` takes a write out of flight so an earlier one that
lands afterwards settles. The toggle re-applies the settled value to the
override and the client, gated on its server still being active.

Serializing per server was the alternative and was not taken: it makes
the modal's debounced saves queue behind each other and changes the write
path to fix a UI-reconciliation concern, and it still could not order two
requests already in flight.

The override reset also moves from a `useEffect` to `useValueChange`,
which is what this repo requires for resetting state from render data —
the entry is referentially stable between reads, and keying on it alone
subsumes the persisted boolean and the server id.

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

@@ -2522,6 +2522,61 @@ describe("App paginated list pagination toggle (#1721)", () => {
     expect(lastPush?.paginatedLists).toBe(true);
   });
 
+  it("re-applies a write that lands after an overlapping one failed (#2089)", async () => {
+    // The mirror of the test above, in the other settlement order: toggle 1
+    // (`true`) is still in flight when toggle 2 (`false`) fails, so the rollback
+    // resolves to the stale entry's `false` — correct at that instant, since
+    // nothing had landed. Toggle 1 then lands `true`. If its list reload failed
+    // too, no render will ever correct the UI, so the write that settles has to
+    // re-apply itself.
+    const user = userEvent.setup();
+    let landFirst: (() => void) | undefined;
+    let failSecond: ((err: Error) => void) | undefined;
+    const first = new Promise<void>((resolve) => {
+      landFirst = resolve;
+    });
+    const second = new Promise<void>((_resolve, reject) => {
+      failSecond = (err) => {
+        reject(err);
+      };
+    });
+    updateServerSettingsSpy
+      .mockImplementationOnce(() => first)
+      .mockImplementationOnce(() => second);
+
+    renderWithMantine(<App />);
+    await user.click(screen.getByText("connect"));
+    await waitFor(() => expect(clientInstances).toHaveLength(1));
+
+    await user.click(screen.getByText("paginated-on"));
+    await user.click(screen.getByText("paginated-off"));
+    await waitFor(() =>
+      expect(updateServerSettingsSpy).toHaveBeenCalledTimes(2),
+    );
+
+    // Fail the second write first, then land the first.
+    await act(async () => {
+      failSecond?.(new Error("disk full"));
+      await second.catch(() => undefined);
+    });
+    expect(screen.getByTestId("tools-paginated")).toHaveTextContent("false");
+    await act(async () => {
+      landFirst?.();
+      await first;
+    });
+
+    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);
+  });
+
   it("lets a successful list read supersede a concrete rollback override (#2089)", async () => {
     // A rollback sets the override to a value rather than clearing it, so the
     // effect that drops it cannot key on the persisted boolean alone: if a

clients/web/src/App.tsx

@@ -111,6 +111,7 @@ 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 { useValueChange } from "./hooks/useValueChange";
 import type { ListPaginationControlsProps } from "./components/elements/ListPaginationControls/ListPaginationControls";
 import { useManagedResourceTemplates } from "@inspector/core/react/useManagedResourceTemplates.js";
 import { useManagedRequestorTasks } from "@inspector/core/react/useManagedRequestorTasks.js";
@@ -1175,15 +1176,19 @@ function App() {
   // Drop the optimistic override once the persisted value catches up (or the
   // active server changes), so the persisted setting is the resting source.
   //
-  // Keyed on the entry object as well as the value it carries: a rollback can
-  // set the override to a *concrete* value rather than clear it, and if a later
-  // successful read then reports the value the override replaced — an edit made
-  // outside the Inspector overtaking the write — neither the boolean nor the id
-  // changes and the UI stays stuck on the override. A read rebuilds the list, so
-  // a fresh entry is what makes every authoritative read supersede it (#2089).
-  useEffect(() => {
-    setPaginatedListsOverride(null);
-  }, [persistedPaginatedLists, activeServerId, activeServerEntry]);
+  // Keyed on the entry object rather than on the boolean it carries: the
+  // override can hold a *concrete* value (a rollback sets one instead of
+  // clearing), and if a later successful read reports the value that override
+  // replaced — an outside edit overtaking the write — the boolean and the
+  // server id are both unchanged and the UI stays stuck on it. Every successful
+  // read rebuilds the list, so the entry is the signal that supersedes it, and
+  // it subsumes the other two: the value is read off this entry, and switching
+  // servers selects a different one (#2089).
+  //
+  // Adjusted during render rather than in an effect, per this repo's rule
+  // against resetting state from render data in `useEffect` — the entry is
+  // referentially stable between reads, which is what `useValueChange` needs.
+  useValueChange(activeServerEntry, () => setPaginatedListsOverride(null));
   const paginatedLists = paginatedListsOverride ?? persistedPaginatedLists;
   // The malformed-entry report is written by the aggregate walk's salvage. In
   // paginated mode the tools/prompts/resources panels render the paged stores
@@ -3785,9 +3790,23 @@ 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).
-          write.landed(next);
+          //
+          // Re-apply it when this write is the settled one: an overlapping
+          // toggle that failed *first* rolled the UI and the live client back
+          // to a baseline this write has since replaced, and if the list read
+          // behind this write failed too, nothing else would ever correct them.
+          const settled = write.landed(next);
+          if (settled && activeServerIdRef.current === activeServerId) {
+            setPaginatedListsOverride(next.paginatedLists ?? false);
+            inspectorClient?.setServerSettings(next);
+          }
         })
         .catch((err: unknown) => {
+          // This write is over and never reached disk, so it stops counting as
+          // in flight: an earlier write still running is the settled state once
+          // it lands, and is what re-applies the UI this rollback is about to
+          // set (#2089).
+          write.failed();
           // 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).
@@ -4191,10 +4210,18 @@ function App() {
       // flush can put two saves for one server in flight, and the later-issued
       // one describes disk however the two happen to finish (#2089).
       const write = lastPersistedSettings.begin(id);
-      await refreshingPersist(updateServerSettings, refreshInitialConfig)(
-        id,
-        value,
-      );
+      try {
+        await refreshingPersist(updateServerSettings, refreshInitialConfig)(
+          id,
+          value,
+        );
+      } catch (err) {
+        // Stop counting as in flight before the rejection goes on to
+        // `onError` — an earlier write still running settles the state once it
+        // lands, and cannot know that while this one looks pending (#2089).
+        write.failed();
+        throw err;
+      }
       // Recorded for the same reason the pagination toggle records its own
       // write: 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

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

@@ -149,6 +149,35 @@ describe("useLastPersistedSettings", () => {
     expect(api.resolve("A")).toBe(newest);
   });
 
+  it("reports settled only for the last write issued", () => {
+    // The flag is what tells a caller it is safe to re-apply the value to the
+    // UI: while a later write is still in flight, that write will describe disk
+    // when it reports, and re-applying now would fight it.
+    const { api } = harness([entry("A", settings())]);
+    const first = api.begin("A");
+    expect(first.landed(settings({ paginatedLists: true }))).toBe(true);
+    const second = api.begin("A");
+    const third = api.begin("A");
+    expect(second.landed(settings({ paginatedLists: false }))).toBe(false);
+    expect(third.landed(settings({ paginatedLists: true }))).toBe(true);
+    // A superseded straggler is neither recorded nor settled.
+    expect(second.landed(settings({ paginatedLists: false }))).toBe(false);
+  });
+
+  it("treats an earlier write as settled once the later one has failed", () => {
+    // The order that leaves the UI wrong: the later write fails first, rolling
+    // the UI back to a baseline the earlier write then replaces on disk. Once
+    // the failed write stops counting as in flight, the earlier one settles and
+    // its caller can re-apply the value.
+    const { api } = harness([entry("A", settings())]);
+    const first = api.begin("A");
+    const second = api.begin("A");
+    second.failed();
+    const written = settings({ paginatedLists: true });
+    expect(first.landed(written)).toBe(true);
+    expect(api.resolve("A")).toBe(written);
+  });
+
   it("does not let a straggler resurrect a record a fresh read dropped", () => {
     // The high-water mark outlives the record, so an earlier write reporting
     // after `resolve` invalidated things cannot reinstate a superseded value.

clients/web/src/hooks/useLastPersistedSettings.ts

@@ -87,8 +87,22 @@ export interface SettingsWrite {
   /**
    * Record that this write reached disk. A no-op if a write issued after this
    * one has already been confirmed for the same server.
+   *
+   * Returns whether this write is now the **settled** state for that server —
+   * recorded, with no later write still in flight to describe disk after it.
+   * Callers that own UI derived from the setting use it to re-apply the value:
+   * an overlapping write that failed *first* will have rolled that UI back to a
+   * baseline this write has since replaced, and if the list read behind this
+   * one failed too, nothing else would ever correct it.
    */
-  landed: (written: InspectorServerSettings) => void;
+  landed: (written: InspectorServerSettings) => boolean;
+  /**
+   * Report that this write is over without reaching disk, so it stops counting
+   * as in flight. An earlier write that lands afterwards is then the settled
+   * state — which is exactly the order that leaves the UI on a rolled-back
+   * value nothing on disk holds.
+   */
+  failed: () => void;
 }
 
 interface WriteRecord {
@@ -108,6 +122,10 @@ export function useLastPersistedSettings(
   // record dropped by `resolve` can't be resurrected by a straggler that was
   // issued earlier: the high-water mark outlives the record it produced.
   const confirmedRef = useRef<Map<string, number>>(new Map());
+  // Issue orders still in flight per server, so a write that lands can tell
+  // whether it is the last word or whether something later will still report.
+  // A write leaves this set through `landed` or `failed` alike.
+  const pendingRef = useRef<Map<string, Set<number>>>(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
@@ -120,16 +138,29 @@ export function useLastPersistedSettings(
 
   const begin = useCallback((serverId: string): SettingsWrite => {
     const sequence = ++nextSequenceRef.current;
+    const pending = pendingRef.current.get(serverId) ?? new Set<number>();
+    pending.add(sequence);
+    pendingRef.current.set(serverId, pending);
+    const settle = () => {
+      pending.delete(sequence);
+    };
     return {
       landed: (written: InspectorServerSettings) => {
+        settle();
         const confirmed = confirmedRef.current.get(serverId) ?? 0;
         // A write issued after this one already reported; it describes disk
         // more recently, whichever of the two happened to finish first.
-        if (confirmed > sequence) return;
+        if (confirmed > sequence) return false;
         confirmedRef.current.set(serverId, sequence);
         const entry = serversRef.current.find((s) => s.id === serverId);
         recordsRef.current.set(serverId, { written, entry });
+        // Settled only while nothing later is still in flight — such a write
+        // will describe disk once it reports, so re-applying this value in the
+        // meantime would fight it.
+        for (const other of pending) if (other > sequence) return false;
+        return true;
       },
+      failed: settle,
     };
   }, []);