fix(web): apply the whole live surface, and track write outcomes per server

Review round 9 on #2091, all three findings.

Every live-apply now goes through one `applyLiveServerSettings`: the
settings object, the Network log size, and the roots diff together. A
close with a pending debounce applies the draft to all three, so
reconciling only `setServerSettings` when that save rejected left the
server advertising roots that were never persisted and the log sized for
them.

The "this server's draft is unpersisted" flag moves from a single id in
`App.tsx` into the tracker, which already orders writes by issue and
keeps state per server. One id could be overwritten by a failure for a
different server, or cleared by a stale success, either of which made a
close apply a rejected draft.

And when nothing has ever landed for a server, close falls back to the
empty settings the toggle rollback uses rather than to the draft — the
old fallback re-applied the very values that had just failed.

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

@@ -1762,6 +1762,50 @@ describe("App roots live-apply on settings-dialog close", () => {
     // …and the rejected roots edit is not advertised either.
     expect(client.setRoots).not.toHaveBeenCalled();
   });
+
+  it("restores the roots and log size too when a flushed save rejects (#2089)", async () => {
+    // Close flushes a pending save and live-applies the draft immediately —
+    // roots and the Network log size, not just the settings object. If that
+    // save then rejects, reconciling only `setServerSettings` would leave the
+    // server advertising roots that were never persisted.
+    const user = userEvent.setup();
+    updateServerSettingsSpy.mockClear();
+    const rejectedDraft: InspectorServerSettings = {
+      ...settingsWithRoots([{ uri: "file:///rejected" }]),
+      maxFetchRequests: 10,
+    };
+    const client = await openSettingsForConnectedServer(rejectedDraft);
+    client.getRoots.mockReturnValue([]);
+
+    const draftOptions = vi.mocked(useSettingsDraft).mock.calls.at(-1)?.[0];
+    if (!draftOptions) throw new Error("useSettingsDraft was never called");
+    const persistedSettings = settingsWithRoots([]);
+    await act(async () => {
+      await draftOptions.onPersist("A", persistedSettings);
+    });
+
+    // Close applies the draft live — the state the flush leaves behind.
+    await closeModal(user);
+    await waitFor(() =>
+      expect(client.setRoots).toHaveBeenCalledWith([
+        { uri: "file:///rejected" },
+      ]),
+    );
+    client.getRoots.mockReturnValue([{ uri: "file:///rejected" }]);
+    client.setRoots.mockClear();
+
+    // …and only now does the flushed save reject.
+    updateServerSettingsSpy.mockRejectedValueOnce(new Error("disk full"));
+    await act(async () => {
+      await draftOptions.onPersist("A", rejectedDraft).catch(() => undefined);
+    });
+
+    // The whole live surface goes back to what landed, roots included.
+    await waitFor(() => expect(client.setRoots).toHaveBeenCalledWith([]));
+    expect(client.setServerSettings).toHaveBeenLastCalledWith(
+      persistedSettings,
+    );
+  });
 });
 
 describe("App history pin/replay", () => {

clients/web/src/App.tsx

@@ -1738,12 +1738,6 @@ function App() {
   // been destroyed — while the replacement client, built from the stale list
   // entry, keeps the value the write replaced (#2089).
   const inspectorClientRef = useRef<InspectorClient | null>(inspectorClient);
-  // The server whose last settings-modal save rejected, if any. `useSettingsDraft`
-  // keeps the edited draft on failure, and closing the modal pushes that draft
-  // into the live client — which would put back a value that never reached disk
-  // and undo the rollback. Cleared by the next save for that server that lands,
-  // and by the close that acts on it (#2089).
-  const failedSettingsSaveRef = useRef<string | undefined>(undefined);
   useEffect(() => {
     if (activeServer) activeServerNameRef.current = activeServer.name;
     activeServerIdRef.current = activeServerId;
@@ -1936,6 +1930,43 @@ function App() {
   // rebound every time the active server (and its log state) changes.
   const fetchLogRef = useRef<FetchRequestLogState | null>(null);
 
+  // Make the live client reflect one settings value, in full. Every path that
+  // decides what the active server's settings *are* goes through this: the
+  // settings modal's close (#1444), and each write's settled / rolled-back
+  // reconciliation (#2089). It is one function because the live surface is more
+  // than `setServerSettings` — a partial re-application would leave the Network
+  // log sized for, and the server advertised roots from, a value that is not on
+  // disk. Reads the client through its ref so a write outliving a reconnect
+  // applies to the instance that exists now.
+  const applyLiveServerSettings = useCallback(
+    (settings: InspectorServerSettings) => {
+      const client = inspectorClientRef.current;
+      if (!client) return;
+      // Settings the managed state reads at notification time
+      // (auto-refresh-on-list-changed) take effect without a reconnect (#1444).
+      // Connection-time inputs (transport, OAuth, timeouts) still only apply on
+      // the next connect.
+      client.setServerSettings(settings);
+      // Resize the Network log buffer live so a maxFetchRequests edit takes
+      // effect without a reconnect (shrinking trims immediately).
+      fetchLogRef.current?.setMaxFetchRequests(settings.maxFetchRequests);
+      // Root edits are diffed against what the client currently advertises
+      // (both cleaned) and notified only when they differ: `setRoots` fires
+      // `notifications/roots/list_changed`, which makes the server re-request
+      // `roots/list`.
+      const nextRoots = cleanRoots(settings.roots);
+      const currentRoots = cleanRoots(client.getRoots());
+      if (JSON.stringify(nextRoots) !== JSON.stringify(currentRoots)) {
+        void client.setRoots(nextRoots).catch(() => {
+          // setRoots swallows notification failures internally; a throw here
+          // only means the client is mid-teardown — the persisted roots will
+          // re-advertise on the next connect, so nothing to surface.
+        });
+      }
+    },
+    [],
+  );
+
   // Flush the pre-redirect Network log to backend storage, keyed by the OAuth
   // authId carried in the authorization URL's `state`. Runs synchronously from
   // `BrowserNavigation` right before `window.location.href`, so the keepalive
@@ -3814,7 +3845,7 @@ function App() {
           const settled = write.landed(next);
           if (settled && activeServerIdRef.current === activeServerId) {
             setPaginatedListsOverride(next.paginatedLists ?? false);
-            inspectorClientRef.current?.setServerSettings(next);
+            applyLiveServerSettings(next);
           }
         })
         .catch((err: unknown) => {
@@ -3845,7 +3876,7 @@ function App() {
             lastPersistedSettings.resolve(activeServerId) ?? EMPTY_SETTINGS;
           if (activeServerIdRef.current === activeServerId) {
             setPaginatedListsOverride(baseline.paginatedLists ?? false);
-            inspectorClientRef.current?.setServerSettings(baseline);
+            applyLiveServerSettings(baseline);
           }
           notifications.show({
             title: "Failed to save pagination setting",
@@ -3858,6 +3889,7 @@ function App() {
       servers,
       activeServerId,
       lastPersistedSettings,
+      applyLiveServerSettings,
       inspectorClient,
       updateServerSettings,
       refreshInitialConfig,
@@ -4243,11 +4275,6 @@ function App() {
         // `onError` — an earlier write still running settles the state once it
         // lands, and cannot know that while this one looks pending (#2089).
         write.failed();
-        // The draft keeps the values this save was carrying, and closing the
-        // modal pushes the draft into the live client — which would put back
-        // the value that just failed to reach disk. Remember that this
-        // server's draft is unpersisted so close can apply disk instead.
-        failedSettingsSaveRef.current = id;
         // A write that landed while this one was pending was told it was not
         // settled and so applied nothing; this save never reached disk, so that
         // write is the account of it and nothing else will re-apply it. Same
@@ -4255,7 +4282,7 @@ function App() {
         const baseline = lastPersistedSettings.resolve(id);
         if (baseline && activeServerIdRef.current === id) {
           setPaginatedListsOverride(baseline.paginatedLists ?? false);
-          inspectorClientRef.current?.setServerSettings(baseline);
+          applyLiveServerSettings(baseline);
         }
         throw err;
       }
@@ -4270,17 +4297,10 @@ function App() {
       // toggle failing while this save is in flight rolls the UI and the live
       // client back to a baseline this save then replaces on disk, so a settled
       // save has to re-apply itself exactly as a settled toggle does (#2089).
-      // This save reached disk, so the draft it came from is persisted again.
-      if (failedSettingsSaveRef.current === id) {
-        failedSettingsSaveRef.current = undefined;
-      }
       const settled = write.landed(value);
       if (settled && activeServerIdRef.current === id) {
         setPaginatedListsOverride(value.paginatedLists ?? false);
-        // The current client, not the one captured when this save began — a
+        applyLiveServerSettings(value);
-        // reconnect to the same server would otherwise be written to a
-        // destroyed instance while the replacement keeps the stale value.
-        inspectorClientRef.current?.setServerSettings(value);
       }
     },
     // Surface failures via toast — the modal usually closes
@@ -4416,36 +4436,19 @@ function App() {
       settingsDraft
     ) {
       // Normally the draft is what the user just saved, so it is also what is
-      // on disk. When this server's last save *rejected* it is not: the draft
+      // on disk. When this server's last write *rejected* it is not: the draft
-      // still holds the failed edit, and pushing it here would contradict disk
+      // still holds the failed edit, and applying it here would contradict disk
-      // and undo the rollback that reported the failure. Apply what landed
+      // and undo the rollback that reported the failure — so apply what landed
-      // instead, and drop the flag — the stale draft goes with the modal, and
+      // instead. `EMPTY_SETTINGS`, not the draft, when nothing has ever landed
-      // reopening seeds from the tracker (#2089).
+      // for this server: falling back to the draft would re-apply the very
-      let applied = settingsDraft;
+      // values that failed (#2089).
-      if (failedSettingsSaveRef.current === settingsModalTargetId) {
+      const applied = lastPersistedSettings.lastWriteFailed(
-        applied =
+        settingsModalTargetId,
-          lastPersistedSettings.resolve(settingsModalTargetId) ?? settingsDraft;
+      )
-        failedSettingsSaveRef.current = undefined;
+        ? (lastPersistedSettings.resolve(settingsModalTargetId) ??
-      }
+          EMPTY_SETTINGS)
-      // Push the edited settings onto the live client so settings the managed
+        : settingsDraft;
-      // state reads at notification time (auto-refresh-on-list-changed) take
+      applyLiveServerSettings(applied);
-      // effect without a reconnect (#1444). Connection-time inputs (transport,
-      // OAuth, timeouts) still only apply on the next connect.
-      inspectorClient.setServerSettings(applied);
-      // Resize the Network log buffer live so a maxFetchRequests edit takes
-      // effect without a reconnect (shrinking trims immediately). Connect-time
-      // construction also reads this, so a reconnect would apply it anyway —
-      // this just makes the toast→adjust flow responsive.
-      fetchLogRef.current?.setMaxFetchRequests(applied.maxFetchRequests);
-      const nextRoots = cleanRoots(applied.roots);
-      const currentRoots = cleanRoots(inspectorClient.getRoots());
-      if (JSON.stringify(nextRoots) !== JSON.stringify(currentRoots)) {
-        void inspectorClient.setRoots(nextRoots).catch(() => {
-          // setRoots swallows notification failures internally; a throw here
-          // only means the client is mid-teardown — the persisted roots will
-          // re-advertise on the next connect, so nothing to surface.
-        });
-      }
     }
     setSettingsModalTargetId(undefined);
   }, [
@@ -4455,6 +4458,7 @@ function App() {
     activeServerId,
     settingsDraft,
     lastPersistedSettings,
+    applyLiveServerSettings,
   ]);
 
   // The Resources screen needs `isSubscribed` to flip the Subscribe button

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

@@ -191,6 +191,31 @@ describe("useLastPersistedSettings", () => {
     expect(api.resolve("A")).toBe(fresh);
   });
 
+  it("reports a failed last write per server, ordered by issue", () => {
+    // The settings modal's draft survives a rejected save, so its owner has to
+    // know whether what the user last tried to save is on disk. A failure on B
+    // says nothing about A, and a straggling failure from an older write does
+    // not overrule a newer write that succeeded.
+    const { api } = harness([entry("A", settings()), entry("B", settings())]);
+    expect(api.lastWriteFailed("A")).toBe(false);
+
+    const firstA = api.begin("A");
+    const secondA = api.begin("A");
+    secondA.landed(settings({ paginatedLists: true }));
+    firstA.failed();
+    expect(api.lastWriteFailed("A")).toBe(false);
+
+    api.begin("B").failed();
+    expect(api.lastWriteFailed("B")).toBe(true);
+    expect(api.lastWriteFailed("A")).toBe(false);
+
+    // …and a newer failure on A does count, until a newer write lands.
+    api.begin("A").failed();
+    expect(api.lastWriteFailed("A")).toBe(true);
+    api.begin("A").landed(settings());
+    expect(api.lastWriteFailed("A")).toBe(false);
+  });
+
   it("returns nothing when the server has left the list entirely", () => {
     const { api, setServers } = harness([entry("A", settings())]);
     api.begin("A").landed(settings({ paginatedLists: true }));

clients/web/src/hooks/useLastPersistedSettings.ts

@@ -81,6 +81,19 @@ export interface LastPersistedSettings {
    * same class of bug as reading the baseline from a stale `servers` entry.
    */
   resolve: (serverId: string) => InspectorServerSettings | undefined;
+  /**
+   * Whether the most recently **issued** write that has reported for this
+   * server ended in failure — i.e. whatever the user last tried to save is not
+   * what disk holds. A caller holding an edit buffer for that server (the
+   * settings modal's draft) uses it to know its buffer is unpersisted, and to
+   * apply `resolve` instead of the buffer.
+   *
+   * Ordered by issue, not by arrival, for the same reason the record is: a
+   * straggling failure from an older write does not overwrite the outcome of a
+   * newer one, and outcomes are kept per server so a failure on B says nothing
+   * about A.
+   */
+  lastWriteFailed: (serverId: string) => boolean;
 }
 
 export interface SettingsWrite {
@@ -126,6 +139,12 @@ export function useLastPersistedSettings(
   // 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());
+  // Outcome of the latest-issued write that has reported, per server. Kept
+  // apart from the record because a *failed* write leaves no record but is
+  // still the answer to "is the user's last attempt on disk?".
+  const outcomesRef = useRef<Map<string, { sequence: number; ok: boolean }>>(
+    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
@@ -141,12 +160,18 @@ export function useLastPersistedSettings(
     const pending = pendingRef.current.get(serverId) ?? new Set<number>();
     pending.add(sequence);
     pendingRef.current.set(serverId, pending);
-    const settle = () => {
+    const settle = (ok: boolean) => {
       pending.delete(sequence);
+      // Keep the outcome of the latest-issued write that has reported, so an
+      // older straggler cannot overwrite a newer one's verdict.
+      const previous = outcomesRef.current.get(serverId);
+      if (!previous || previous.sequence <= sequence) {
+        outcomesRef.current.set(serverId, { sequence, ok });
+      }
     };
     return {
       landed: (written: InspectorServerSettings) => {
-        settle();
+        settle(true);
         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.
@@ -160,10 +185,15 @@ export function useLastPersistedSettings(
         for (const other of pending) if (other > sequence) return false;
         return true;
       },
-      failed: settle,
+      failed: () => settle(false),
     };
   }, []);
 
+  const lastWriteFailed = useCallback(
+    (serverId: string) => outcomesRef.current.get(serverId)?.ok === false,
+    [],
+  );
+
   const resolve = useCallback(
     (serverId: string): InspectorServerSettings | undefined => {
       const entry = serversRef.current.find((s) => s.id === serverId);
@@ -182,5 +212,5 @@ export function useLastPersistedSettings(
     [],
   );
 
-  return { begin, resolve };
+  return { begin, resolve, lastWriteFailed };
 }