Merge pull request #2131 from modelcontextprotocol/v2/fix/2095-pagination-override-by-server

fix: keep the pagination toggle on the last write across a server switch

clients/web/src/App.test.tsx

@@ -555,6 +555,8 @@ vi.mock("./components/views/InspectorView/InspectorView", () => ({
         switch-servers-tab
       </button>
       <button onClick={() => props.onToggleConnection("A")}>connect</button>
+      {/* A second target, so a test can drive an A -> B -> A switch (#2095). */}
+      <button onClick={() => props.onToggleConnection("B")}>connect-b</button>
       <button onClick={() => props.onConnectionInfo()}>
         open-connection-info
       </button>
@@ -3538,6 +3540,74 @@ describe("App paginated list pagination toggle (#1721)", () => {
     };
     expect(lastPush?.paginatedLists).toBe(true);
   });
+
+  it("keeps the toggle on the last write after switching servers and back (#2095)", async () => {
+    // The override is what holds the display up while list reads are failing --
+    // the `servers` entry it would otherwise be read from is frozen at the last
+    // successful read, which this suite models exactly by mocking `useServers`
+    // to a fixed list.
+    //
+    // Held app-wide and cleared on every change of the *active* entry, it did
+    // not survive a plain server switch: A to B and back dropped it and the
+    // toggle fell back to that stale entry, reading `off` while disk, the
+    // tracker, and the client just built from it all said `on`. The lists were
+    // then rendered in all-pages mode showing an aggregate the client never
+    // fetched, with no Load-next-page control to fill them.
+    //
+    // Nothing here fails a list read explicitly: the fixed mock *is* that
+    // state, which is why one write is enough to reproduce it.
+    const user = userEvent.setup();
+    const previousUseServers = vi.mocked(useServers).getMockImplementation();
+    try {
+      vi.mocked(useServers).mockReturnValue({
+        servers: [
+          SERVER_A,
+          { ...SERVER_A, id: "B", name: "Other" },
+        ] as ServerEntry[],
+        loading: false,
+        error: undefined,
+        refresh: vi.fn().mockResolvedValue(undefined),
+        addServer: addServerSpy,
+        updateServer: updateServerSpy,
+        updateServerSettings: updateServerSettingsSpy,
+        removeServer: vi.fn(),
+        reorderServers: vi.fn(),
+        importSource: vi.fn().mockResolvedValue({ servers: {} }),
+      });
+      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 }),
+        ),
+      );
+      await waitFor(() =>
+        expect(screen.getByTestId("tools-paginated")).toHaveTextContent("true"),
+      );
+
+      // Away to B, whose own entry has never been written and so reads false.
+      await user.click(screen.getByText("connect-b"));
+      await waitFor(() =>
+        expect(screen.getByTestId("active-server")).toHaveTextContent("B"),
+      );
+      expect(screen.getByTestId("tools-paginated")).toHaveTextContent("false");
+
+      // ...and back to A, which is still paginated on disk.
+      await user.click(screen.getByText("connect"));
+      await waitFor(() =>
+        expect(screen.getByTestId("active-server")).toHaveTextContent("A"),
+      );
+      expect(screen.getByTestId("tools-paginated")).toHaveTextContent("true");
+    } finally {
+      if (previousUseServers) {
+        vi.mocked(useServers).mockImplementation(previousUseServers);
+      }
+    }
+  });
 });
 
 // A post-write reload failure rejects *after* the write landed, so every piece

clients/web/src/App.tsx

@@ -116,6 +116,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 { usePaginatedListsOverride } from "./hooks/usePaginatedListsOverride";
 import { useValueChange } from "./hooks/useValueChange";
 import type { ListPaginationControlsProps } from "./components/elements/ListPaginationControls/ListPaginationControls";
 import { useManagedResourceTemplates } from "@inspector/core/react/useManagedResourceTemplates.js";
@@ -1234,26 +1235,15 @@ function App() {
   const activeServerEntry = servers.find((s) => s.id === activeServerId);
   const persistedPaginatedLists =
     activeServerEntry?.settings?.paginatedLists ?? false;
-  const [paginatedListsOverride, setPaginatedListsOverride] = useState<
+  // The optimistic override, held per server and superseded by the first
-    boolean | null
+  // successful list read that rebuilds that server's entry. Per server rather
-  >(null);
+  // than one app-wide slot cleared on every active-entry change: the previous
-  // Drop the optimistic override once the persisted value catches up (or the
+  // shape dropped the override on a plain server switch, so an A → B → A round
-  // active server changes), so the persisted setting is the resting source.
+  // trip fell back to A's stale entry and showed a value neither disk nor the
-  //
+  // live client held (#2095).
-  // Keyed on the entry object rather than on the boolean it carries: the
+  const paginatedListsOverride = usePaginatedListsOverride(servers);
-  // override can hold a *concrete* value (a rollback sets one instead of
+  const paginatedLists =
-  // clearing), and if a later successful read reports the value that override
+    paginatedListsOverride.valueFor(activeServerId) ?? persistedPaginatedLists;
-  // 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
   // instead, which never write or clear it — so it would linger above a page it
@@ -3968,9 +3958,13 @@ function App() {
   // gating reads it now, and a persisted PUT so it survives reconnects (#1721).
   const onTogglePaginatedLists = useCallback(
     (value: boolean) => {
-      setPaginatedListsOverride(value);
       const current = servers.find((s) => s.id === activeServerId);
       if (!current || activeServerId === undefined) return;
+      // Recorded against this server rather than app-wide, so it survives a
+      // switch away and back (#2095). Ordered after the id guard because the
+      // record is keyed by that id; with no active server there is nothing the
+      // toggle could have been flipped for.
+      paginatedListsOverride.record(activeServerId, value);
       // 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. Build on the last write known to
@@ -4019,8 +4013,16 @@ function App() {
       // already destroyed.
       const settlePaginationWrite = () => {
         const settled = write.landed(next);
-        if (settled && activeServerIdRef.current === activeServerId) {
+        if (!settled) return;
-          setPaginatedListsOverride(next.paginatedLists ?? false);
+        // The override is keyed by server, so it is re-applied whatever is
+        // active now — it is this server's value and is only ever displayed
+        // while this server is the active one. The live client is not: it
+        // belongs to whichever server is connected (#2095).
+        paginatedListsOverride.record(
+          activeServerId,
+          next.paginatedLists ?? false,
+        );
+        if (activeServerIdRef.current === activeServerId) {
           applyLiveServerSettings(next);
         }
       };
@@ -4061,17 +4063,25 @@ function App() {
           // `persistedPaginatedLists` — read from a `servers` entry that may be
           // stale, showing the same wrong value from the other side (#2089).
           //
-          // Applied only while this write's server is still the active one: the
+          // The override is recorded whatever is active by the time this
-          // override is a single app-wide boolean and the live client belongs to
+          // rejection arrives: it is keyed by this write's server and is only
-          // whichever server is connected now, so a rejection arriving after the
+          // ever displayed while that server is the active one, so a switch in
-          // user switched would render this server's value on another one.
+          // between costs nothing and dropping it would leave the stale entry
-          // The client is taken from the ref for the same reason the success
+          // to answer for A the next time it comes back (#2095).
-          // path does: a reconnect to the same server passes the id check while
+          //
-          // this continuation's captured instance is already destroyed.
+          // The live client is the half that stays gated — it belongs to
+          // whichever server is connected now, so pushing this server's value
+          // into it after a switch would apply it to another one. It is taken
+          // from the ref for the same reason the success path does: a reconnect
+          // to the same server passes the id check while this continuation's
+          // captured instance is already destroyed.
           const baseline =
             lastPersistedSettings.resolve(activeServerId) ?? EMPTY_SETTINGS;
+          paginatedListsOverride.record(
+            activeServerId,
+            baseline.paginatedLists ?? false,
+          );
           if (activeServerIdRef.current === activeServerId) {
-            setPaginatedListsOverride(baseline.paginatedLists ?? false);
             applyLiveServerSettings(baseline);
           }
           notifications.show({
@@ -4085,6 +4095,7 @@ function App() {
       servers,
       activeServerId,
       lastPersistedSettings,
+      paginatedListsOverride,
       applyLiveServerSettings,
       inspectorClient,
       updateServerSettings,
@@ -4513,8 +4524,9 @@ function App() {
       const write = lastPersistedSettings.begin(id);
       const settleSettingsWrite = () => {
         const settled = write.landed(value);
-        if (settled && activeServerIdRef.current === id) {
+        if (!settled) return;
-          setPaginatedListsOverride(value.paginatedLists ?? false);
+        paginatedListsOverride.record(id, value.paginatedLists ?? false);
+        if (activeServerIdRef.current === id) {
           applyLiveServerSettings(value);
         }
       };
@@ -4541,9 +4553,11 @@ function App() {
         // write is the account of it and nothing else will re-apply it. Same
         // reconciliation the toggle's own failure path does.
         const baseline = lastPersistedSettings.resolve(id);
-        if (baseline && activeServerIdRef.current === id) {
+        if (baseline) {
-          setPaginatedListsOverride(baseline.paginatedLists ?? false);
+          paginatedListsOverride.record(id, baseline.paginatedLists ?? false);
-          applyLiveServerSettings(baseline);
+          if (activeServerIdRef.current === id) {
+            applyLiveServerSettings(baseline);
+          }
         }
         throw err;
       }

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,120 @@
+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) => {
+    // Read outside the updater. A functional updater must be pure — React may
+    // defer or replay it — and this one would otherwise resolve the ref at
+    // whatever moment it happened to run. A list read committing in between
+    // would then be paired with the record as its baseline, so the very read
+    // that should have superseded the override would instead certify it.
+    const list = serversRef.current;
+    setRecords((previous) => {
+      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]);
+}