fix: keep the pagination toggle on the last write across a server switch (#2095)

The displayed `paginatedLists` value falls back to the active `servers`
entry, and that list only advances when a `GET /api/servers` succeeds — so
once a list read fails it keeps describing disk as it was before the writes
made since. The optimistic override papered over that, but it was a single
app-wide slot cleared on every change of the *active* entry, so switching to
another server and back dropped it and the UI fell back to the stale entry.

Two things then disagreed, and the one the user could see was the wrong one:
the toggle read `off` while disk, the write tracker, and the client just
built from it all said `on`; and because the client skipped its connect-time
aggregate walk, the all-pages UI rendered an aggregate that was never
fetched, with no Load-next-page control to fill it.

Hold the override per server instead, in `usePaginatedListsOverride`, paired
with that server's entry as of when it was recorded and believed only while
the list still carries that same entry object — identity, not equality, so a
read reporting the value the override replaced supersedes it too. A server's
override then survives a switch away and back, and is still superseded by
the first successful read that rebuilds its entry.

The read cannot instead move to `lastPersistedSettings.resolve(id)`: that is
called during render, and it both reads refs and prunes superseded records.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh
Signed-off-by: cliffhall <cliff@futurescale.com>

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

@@ -0,0 +1,148 @@
+import { describe, it, expect } from "vitest";
+import { act } from "react";
+import type { ServerEntry } from "@inspector/core/mcp/types.js";
+import { renderWithMantine } from "../test/renderWithMantine";
+import {
+  usePaginatedListsOverride,
+  type PaginatedListsOverride,
+} from "./usePaginatedListsOverride";
+
+/**
+ * One `servers` entry, a fresh object per call — every test here turns 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 = (id: string): ServerEntry => ({
+  id,
+  name: id,
+  config: { type: "stdio", command: "node" },
+  connection: { status: "disconnected" },
+});
+
+interface Harness {
+  /** The hook's API as of the latest render. */
+  api: () => PaginatedListsOverride;
+  /** Deliver a new server list, as a successful list read would. */
+  setServers: (next: ServerEntry[]) => void;
+}
+
+function harness(initial: ServerEntry[]): Harness {
+  let latest: PaginatedListsOverride | undefined;
+  function Probe({ servers }: { servers: ServerEntry[] }) {
+    latest = usePaginatedListsOverride(servers);
+    return null;
+  }
+  const { rerender } = renderWithMantine(<Probe servers={initial} />);
+  return {
+    api: () => {
+      if (!latest) throw new Error("hook did not render");
+      return latest;
+    },
+    setServers: (next) => {
+      act(() => rerender(<Probe servers={next} />));
+    },
+  };
+}
+
+describe("usePaginatedListsOverride", () => {
+  it("reports no override before anything is recorded", () => {
+    const { api } = harness([entry("A")]);
+    expect(api().valueFor("A")).toBeUndefined();
+  });
+
+  it("reports nothing for an undefined server id", () => {
+    // No server is active before the first connect, and the composition root
+    // asks for the display value on every render regardless.
+    const { api } = harness([entry("A")]);
+    act(() => api().record("A", true));
+    expect(api().valueFor(undefined)).toBeUndefined();
+  });
+
+  it("returns what was recorded for that server", () => {
+    const { api } = harness([entry("A")]);
+    act(() => api().record("A", true));
+    expect(api().valueFor("A")).toBe(true);
+  });
+
+  it("does not apply one server's override to another", () => {
+    // The override is the *display* value, and the settings modal can be saved
+    // for a server that is not the connected one — so a record made for B must
+    // not change what A renders.
+    const { api } = harness([entry("A"), entry("B")]);
+    act(() => api().record("B", true));
+    expect(api().valueFor("A")).toBeUndefined();
+    expect(api().valueFor("B")).toBe(true);
+  });
+
+  it("survives a server switch away and back (#2095)", () => {
+    // Nothing about the list changes here — only which server is being asked
+    // about, which is all a switch is. The previous app-wide slot was cleared
+    // on every change of the active entry, which is what dropped A's value.
+    const { api } = harness([entry("A"), entry("B")]);
+    act(() => api().record("A", true));
+    expect(api().valueFor("B")).toBeUndefined();
+    expect(api().valueFor("A")).toBe(true);
+  });
+
+  it("keeps a record for each server", () => {
+    const { api } = harness([entry("A"), entry("B")]);
+    act(() => api().record("A", true));
+    act(() => api().record("B", false));
+    expect(api().valueFor("A")).toBe(true);
+    expect(api().valueFor("B")).toBe(false);
+  });
+
+  it("replaces its own record for the same server", () => {
+    const { api } = harness([entry("A")]);
+    act(() => api().record("A", true));
+    act(() => api().record("A", false));
+    expect(api().valueFor("A")).toBe(false);
+  });
+
+  it("stops applying once a fresh list read replaces that entry", () => {
+    const { api, setServers } = harness([entry("A")]);
+    act(() => api().record("A", true));
+    // A successful `GET /api/servers` rebuilds the list, so the entry is a new
+    // object even when the values are unchanged — which is exactly the case
+    // that has to supersede the override, since an edit made outside the
+    // Inspector can report back the value the override replaced.
+    setServers([entry("A")]);
+    expect(api().valueFor("A")).toBeUndefined();
+  });
+
+  it("keeps a record whose own entry was not re-read", () => {
+    // A read that rebuilds B says nothing about A. In practice the list is
+    // rebuilt wholesale, but the check is per entry and must not be widened to
+    // "any list change" — a reorder replaces the array while keeping every
+    // entry object, and is not evidence of a fresher read.
+    const a = entry("A");
+    const { api, setServers } = harness([a, entry("B")]);
+    act(() => api().record("A", true));
+    setServers([entry("B"), a]);
+    expect(api().valueFor("A")).toBe(true);
+  });
+
+  it("records against the list as it stands when the record is written", () => {
+    // A rollback records from a continuation that may be several renders old;
+    // pairing with the list captured then would make the record look superseded
+    // the moment it was written.
+    const { api, setServers } = harness([entry("A")]);
+    const stale = api();
+    setServers([entry("A")]);
+    act(() => stale.record("A", true));
+    expect(api().valueFor("A")).toBe(true);
+  });
+
+  it("drops superseded records rather than accumulating them", () => {
+    // Housekeeping, asserted through the only surface it has: a record dropped
+    // on the way past cannot come back, so re-reading after the entry is
+    // restored to the list still reports nothing.
+    const a = entry("A");
+    const { api, setServers } = harness([a, entry("B")]);
+    act(() => api().record("A", true));
+    setServers([entry("A"), entry("B")]);
+    act(() => api().record("B", true));
+    setServers([a, entry("B")]);
+    expect(api().valueFor("A")).toBeUndefined();
+  });
+});

clients/web/src/hooks/usePaginatedListsOverride.ts

@@ -0,0 +1,115 @@
+import { useCallback, useEffect, useMemo, useRef, useState } from "react";
+import type { ServerEntry } from "@inspector/core/mcp/types.js";
+
+/**
+ * Holds the optimistic `paginatedLists` value **per server**, so the sidebar
+ * toggle keeps showing what the last write put on disk even after the user
+ * switches to another server and back (#2095).
+ *
+ * The displayed value normally comes from the active `servers` entry, and that
+ * list only advances when a `GET /api/servers` succeeds. Once a list read has
+ * failed, every entry keeps describing disk *as of the last successful read*,
+ * while the writes made since have changed it — the same staleness
+ * `useLastPersistedSettings` exists to survive on the write side (#2089). The
+ * optimistic override papers over it for the display, so it has to outlive the
+ * things that are not evidence of a fresher read. Switching servers is one of
+ * them: the previous single app-wide slot was cleared whenever the *active*
+ * entry changed, so an A → B → A round trip dropped A's override and the
+ * toggle fell back to the stale entry — reading `off` while disk, the tracker,
+ * and the live client all said `on`, with the lists rendered in all-pages mode
+ * showing an aggregate the client never fetched and no Load-next-page control
+ * to fill them.
+ *
+ * A record is therefore keyed by server id and paired with that server's
+ * `servers` entry as of when it was recorded, and is believed only while the
+ * list still carries that same entry object. Identity, not deep equality: the
+ * question is whether the list has been re-read since, not whether the values
+ * happen to agree — a successful read that reports the value the override
+ * replaced (an edit made outside the Inspector overtaking the write) has to
+ * supersede it too, and its boolean is unchanged.
+ *
+ * Records are per server for the reason `useLastPersistedSettings` keeps its
+ * own that way: the settings modal can be opened for any server, independently
+ * of which one is connected, so a single slot lets a save on B discard the
+ * value A is still displaying from.
+ *
+ * `valueFor` is pure over the list passed in and is meant to be called during
+ * render. It deliberately does **not** prune superseded records — that is a
+ * write, and this repo's rules put neither a ref mutation nor a state update in
+ * a render path. Pruning happens on the next `record` instead, where a stale
+ * entry is only ever read as "no override" in the meantime.
+ */
+export interface PaginatedListsOverride {
+  /**
+   * Record an optimistic value for `serverId`, pairing it with that server's
+   * entry as the list stands **now** — not as it stood when the write that
+   * produced this value was issued, which a read landing mid-flight would
+   * otherwise make look newer than the write.
+   */
+  record: (serverId: string, value: boolean) => void;
+  /**
+   * The override for `serverId`, or `undefined` when there is none or the list
+   * has moved on from the entry it was recorded against. Pure over the current
+   * list — safe to call during render.
+   */
+  valueFor: (serverId: string | undefined) => boolean | undefined;
+}
+
+interface OverrideRecord {
+  value: boolean;
+  entry: ServerEntry | undefined;
+}
+
+/**
+ * @param servers the current server list, as rendered
+ */
+export function usePaginatedListsOverride(
+  servers: ServerEntry[],
+): PaginatedListsOverride {
+  const [records, setRecords] = useState<Map<string, OverrideRecord>>(
+    () => new Map(),
+  );
+  // Read the list through a ref so a record pairs with the entry as it stands
+  // when it is written, rather than the one captured when the enclosing
+  // handler was created — a settled or rejected write records from a
+  // continuation that may be several list reads old. Mirrored in an effect
+  // rather than assigned during render: writing a ref mid-render is an error
+  // under `react-hooks/refs`, and `record` only ever runs from an event
+  // handler or a settled promise, long after the commit.
+  const serversRef = useRef(servers);
+  useEffect(() => {
+    serversRef.current = servers;
+  }, [servers]);
+
+  const record = useCallback((serverId: string, value: boolean) => {
+    setRecords((previous) => {
+      const list = serversRef.current;
+      const next = new Map<string, OverrideRecord>();
+      // Carry forward only the records the list has not moved past, so a
+      // session that toggles across many servers does not accumulate a record
+      // (and a reference to a long-dead entry object) for each one.
+      for (const [id, held] of previous) {
+        if (id !== serverId && held.entry === list.find((s) => s.id === id)) {
+          next.set(id, held);
+        }
+      }
+      next.set(serverId, { value, entry: list.find((s) => s.id === serverId) });
+      return next;
+    });
+  }, []);
+
+  const valueFor = useMemo(
+    () =>
+      (serverId: string | undefined): boolean | undefined => {
+        if (serverId === undefined) return undefined;
+        const held = records.get(serverId);
+        if (!held) return undefined;
+        return held.entry === servers.find((s) => s.id === serverId)
+          ? held.value
+          : undefined;
+      },
+    [records, servers],
+  );
+
+  return useMemo(() => ({ record, valueFor }), [record, valueFor]);
+}