Skip to content

Commit ca2846d

Browse files
authored
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
2 parents 897f21c + bbffafd commit ca2846d

4 files changed

Lines changed: 388 additions & 36 deletions

File tree

‎clients/web/src/App.test.tsx‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -555,6 +555,8 @@ vi.mock("./components/views/InspectorView/InspectorView", () => ({
555555
switch-servers-tab
556556
</button>
557557
<button onClick={() => props.onToggleConnection("A")}>connect</button>
558+
{/* A second target, so a test can drive an A -> B -> A switch (#2095). */}
559+
<button onClick={() => props.onToggleConnection("B")}>connect-b</button>
558560
<button onClick={() => props.onConnectionInfo()}>
559561
open-connection-info
560562
</button>
@@ -3538,6 +3540,74 @@ describe("App paginated list pagination toggle (#1721)", () => {
35383540
};
35393541
expect(lastPush?.paginatedLists).toBe(true);
35403542
});
3543+
3544+
it("keeps the toggle on the last write after switching servers and back (#2095)", async () => {
3545+
// The override is what holds the display up while list reads are failing --
3546+
// the `servers` entry it would otherwise be read from is frozen at the last
3547+
// successful read, which this suite models exactly by mocking `useServers`
3548+
// to a fixed list.
3549+
//
3550+
// Held app-wide and cleared on every change of the *active* entry, it did
3551+
// not survive a plain server switch: A to B and back dropped it and the
3552+
// toggle fell back to that stale entry, reading `off` while disk, the
3553+
// tracker, and the client just built from it all said `on`. The lists were
3554+
// then rendered in all-pages mode showing an aggregate the client never
3555+
// fetched, with no Load-next-page control to fill them.
3556+
//
3557+
// Nothing here fails a list read explicitly: the fixed mock *is* that
3558+
// state, which is why one write is enough to reproduce it.
3559+
const user = userEvent.setup();
3560+
const previousUseServers = vi.mocked(useServers).getMockImplementation();
3561+
try {
3562+
vi.mocked(useServers).mockReturnValue({
3563+
servers: [
3564+
SERVER_A,
3565+
{ ...SERVER_A, id: "B", name: "Other" },
3566+
] as ServerEntry[],
3567+
loading: false,
3568+
error: undefined,
3569+
refresh: vi.fn().mockResolvedValue(undefined),
3570+
addServer: addServerSpy,
3571+
updateServer: updateServerSpy,
3572+
updateServerSettings: updateServerSettingsSpy,
3573+
removeServer: vi.fn(),
3574+
reorderServers: vi.fn(),
3575+
importSource: vi.fn().mockResolvedValue({ servers: {} }),
3576+
});
3577+
renderWithMantine(<App />);
3578+
await user.click(screen.getByText("connect"));
3579+
await waitFor(() => expect(clientInstances).toHaveLength(1));
3580+
3581+
await user.click(screen.getByText("paginated-on"));
3582+
await waitFor(() =>
3583+
expect(updateServerSettingsSpy).toHaveBeenCalledWith(
3584+
"A",
3585+
expect.objectContaining({ paginatedLists: true }),
3586+
),
3587+
);
3588+
await waitFor(() =>
3589+
expect(screen.getByTestId("tools-paginated")).toHaveTextContent("true"),
3590+
);
3591+
3592+
// Away to B, whose own entry has never been written and so reads false.
3593+
await user.click(screen.getByText("connect-b"));
3594+
await waitFor(() =>
3595+
expect(screen.getByTestId("active-server")).toHaveTextContent("B"),
3596+
);
3597+
expect(screen.getByTestId("tools-paginated")).toHaveTextContent("false");
3598+
3599+
// ...and back to A, which is still paginated on disk.
3600+
await user.click(screen.getByText("connect"));
3601+
await waitFor(() =>
3602+
expect(screen.getByTestId("active-server")).toHaveTextContent("A"),
3603+
);
3604+
expect(screen.getByTestId("tools-paginated")).toHaveTextContent("true");
3605+
} finally {
3606+
if (previousUseServers) {
3607+
vi.mocked(useServers).mockImplementation(previousUseServers);
3608+
}
3609+
}
3610+
});
35413611
});
35423612

35433613
// A post-write reload failure rejects *after* the write landed, so every piece

‎clients/web/src/App.tsx‎

Lines changed: 50 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,7 @@ import { usePagedPrompts } from "@inspector/core/react/usePagedPrompts.js";
116116
import { usePagedResources } from "@inspector/core/react/usePagedResources.js";
117117
import { usePaginatedList } from "./hooks/usePaginatedList";
118118
import { useLastPersistedSettings } from "./hooks/useLastPersistedSettings";
119+
import { usePaginatedListsOverride } from "./hooks/usePaginatedListsOverride";
119120
import { useValueChange } from "./hooks/useValueChange";
120121
import type { ListPaginationControlsProps } from "./components/elements/ListPaginationControls/ListPaginationControls";
121122
import { useManagedResourceTemplates } from "@inspector/core/react/useManagedResourceTemplates.js";
@@ -1234,26 +1235,15 @@ function App() {
12341235
const activeServerEntry = servers.find((s) => s.id === activeServerId);
12351236
const persistedPaginatedLists =
12361237
activeServerEntry?.settings?.paginatedLists ?? false;
1237-
const [paginatedListsOverride, setPaginatedListsOverride] = useState<
1238-
boolean | null
1239-
>(null);
1240-
// Drop the optimistic override once the persisted value catches up (or the
1241-
// active server changes), so the persisted setting is the resting source.
1242-
//
1243-
// Keyed on the entry object rather than on the boolean it carries: the
1244-
// override can hold a *concrete* value (a rollback sets one instead of
1245-
// clearing), and if a later successful read reports the value that override
1246-
// replaced — an outside edit overtaking the write — the boolean and the
1247-
// server id are both unchanged and the UI stays stuck on it. Every successful
1248-
// read rebuilds the list, so the entry is the signal that supersedes it, and
1249-
// it subsumes the other two: the value is read off this entry, and switching
1250-
// servers selects a different one (#2089).
1251-
//
1252-
// Adjusted during render rather than in an effect, per this repo's rule
1253-
// against resetting state from render data in `useEffect` — the entry is
1254-
// referentially stable between reads, which is what `useValueChange` needs.
1255-
useValueChange(activeServerEntry, () => setPaginatedListsOverride(null));
1256-
const paginatedLists = paginatedListsOverride ?? persistedPaginatedLists;
1238+
// The optimistic override, held per server and superseded by the first
1239+
// successful list read that rebuilds that server's entry. Per server rather
1240+
// than one app-wide slot cleared on every active-entry change: the previous
1241+
// shape dropped the override on a plain server switch, so an A → B → A round
1242+
// trip fell back to A's stale entry and showed a value neither disk nor the
1243+
// live client held (#2095).
1244+
const paginatedListsOverride = usePaginatedListsOverride(servers);
1245+
const paginatedLists =
1246+
paginatedListsOverride.valueFor(activeServerId) ?? persistedPaginatedLists;
12571247
// The malformed-entry report is written by the aggregate walk's salvage. In
12581248
// paginated mode the tools/prompts/resources panels render the paged stores
12591249
// instead, which never write or clear it — so it would linger above a page it
@@ -3968,9 +3958,13 @@ function App() {
39683958
// gating reads it now, and a persisted PUT so it survives reconnects (#1721).
39693959
const onTogglePaginatedLists = useCallback(
39703960
(value: boolean) => {
3971-
setPaginatedListsOverride(value);
39723961
const current = servers.find((s) => s.id === activeServerId);
39733962
if (!current || activeServerId === undefined) return;
3963+
// Recorded against this server rather than app-wide, so it survives a
3964+
// switch away and back (#2095). Ordered after the id guard because the
3965+
// record is keyed by that id; with no active server there is nothing the
3966+
// toggle could have been flipped for.
3967+
paginatedListsOverride.record(activeServerId, value);
39743968
// Not `current.settings` directly: that entry only advances on a
39753969
// successful list read, so once one has failed it describes disk as it
39763970
// was *before* the writes made since. Build on the last write known to
@@ -4019,8 +4013,16 @@ function App() {
40194013
// already destroyed.
40204014
const settlePaginationWrite = () => {
40214015
const settled = write.landed(next);
4022-
if (settled && activeServerIdRef.current === activeServerId) {
4023-
setPaginatedListsOverride(next.paginatedLists ?? false);
4016+
if (!settled) return;
4017+
// The override is keyed by server, so it is re-applied whatever is
4018+
// active now — it is this server's value and is only ever displayed
4019+
// while this server is the active one. The live client is not: it
4020+
// belongs to whichever server is connected (#2095).
4021+
paginatedListsOverride.record(
4022+
activeServerId,
4023+
next.paginatedLists ?? false,
4024+
);
4025+
if (activeServerIdRef.current === activeServerId) {
40244026
applyLiveServerSettings(next);
40254027
}
40264028
};
@@ -4061,17 +4063,25 @@ function App() {
40614063
// `persistedPaginatedLists` — read from a `servers` entry that may be
40624064
// stale, showing the same wrong value from the other side (#2089).
40634065
//
4064-
// Applied only while this write's server is still the active one: the
4065-
// override is a single app-wide boolean and the live client belongs to
4066-
// whichever server is connected now, so a rejection arriving after the
4067-
// user switched would render this server's value on another one.
4068-
// The client is taken from the ref for the same reason the success
4069-
// path does: a reconnect to the same server passes the id check while
4070-
// this continuation's captured instance is already destroyed.
4066+
// The override is recorded whatever is active by the time this
4067+
// rejection arrives: it is keyed by this write's server and is only
4068+
// ever displayed while that server is the active one, so a switch in
4069+
// between costs nothing and dropping it would leave the stale entry
4070+
// to answer for A the next time it comes back (#2095).
4071+
//
4072+
// The live client is the half that stays gated — it belongs to
4073+
// whichever server is connected now, so pushing this server's value
4074+
// into it after a switch would apply it to another one. It is taken
4075+
// from the ref for the same reason the success path does: a reconnect
4076+
// to the same server passes the id check while this continuation's
4077+
// captured instance is already destroyed.
40714078
const baseline =
40724079
lastPersistedSettings.resolve(activeServerId) ?? EMPTY_SETTINGS;
4080+
paginatedListsOverride.record(
4081+
activeServerId,
4082+
baseline.paginatedLists ?? false,
4083+
);
40734084
if (activeServerIdRef.current === activeServerId) {
4074-
setPaginatedListsOverride(baseline.paginatedLists ?? false);
40754085
applyLiveServerSettings(baseline);
40764086
}
40774087
notifications.show({
@@ -4085,6 +4095,7 @@ function App() {
40854095
servers,
40864096
activeServerId,
40874097
lastPersistedSettings,
4098+
paginatedListsOverride,
40884099
applyLiveServerSettings,
40894100
inspectorClient,
40904101
updateServerSettings,
@@ -4513,8 +4524,9 @@ function App() {
45134524
const write = lastPersistedSettings.begin(id);
45144525
const settleSettingsWrite = () => {
45154526
const settled = write.landed(value);
4516-
if (settled && activeServerIdRef.current === id) {
4517-
setPaginatedListsOverride(value.paginatedLists ?? false);
4527+
if (!settled) return;
4528+
paginatedListsOverride.record(id, value.paginatedLists ?? false);
4529+
if (activeServerIdRef.current === id) {
45184530
applyLiveServerSettings(value);
45194531
}
45204532
};
@@ -4541,9 +4553,11 @@ function App() {
45414553
// write is the account of it and nothing else will re-apply it. Same
45424554
// reconciliation the toggle's own failure path does.
45434555
const baseline = lastPersistedSettings.resolve(id);
4544-
if (baseline && activeServerIdRef.current === id) {
4545-
setPaginatedListsOverride(baseline.paginatedLists ?? false);
4546-
applyLiveServerSettings(baseline);
4556+
if (baseline) {
4557+
paginatedListsOverride.record(id, baseline.paginatedLists ?? false);
4558+
if (activeServerIdRef.current === id) {
4559+
applyLiveServerSettings(baseline);
4560+
}
45474561
}
45484562
throw err;
45494563
}
Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,148 @@
1+
import { describe, it, expect } from "vitest";
2+
import { act } from "react";
3+
import type { ServerEntry } from "@inspector/core/mcp/types.js";
4+
import { renderWithMantine } from "../test/renderWithMantine";
5+
import {
6+
usePaginatedListsOverride,
7+
type PaginatedListsOverride,
8+
} from "./usePaginatedListsOverride";
9+
10+
/**
11+
* One `servers` entry, a fresh object per call — every test here turns on
12+
* object identity, so a shared fixture would defeat them. A list rebuilt from
13+
* fresh calls is what a successful `GET /api/servers` produces.
14+
*/
15+
const entry = (id: string): ServerEntry => ({
16+
id,
17+
name: id,
18+
config: { type: "stdio", command: "node" },
19+
connection: { status: "disconnected" },
20+
});
21+
22+
interface Harness {
23+
/** The hook's API as of the latest render. */
24+
api: () => PaginatedListsOverride;
25+
/** Deliver a new server list, as a successful list read would. */
26+
setServers: (next: ServerEntry[]) => void;
27+
}
28+
29+
function harness(initial: ServerEntry[]): Harness {
30+
let latest: PaginatedListsOverride | undefined;
31+
function Probe({ servers }: { servers: ServerEntry[] }) {
32+
latest = usePaginatedListsOverride(servers);
33+
return null;
34+
}
35+
const { rerender } = renderWithMantine(<Probe servers={initial} />);
36+
return {
37+
api: () => {
38+
if (!latest) throw new Error("hook did not render");
39+
return latest;
40+
},
41+
setServers: (next) => {
42+
act(() => rerender(<Probe servers={next} />));
43+
},
44+
};
45+
}
46+
47+
describe("usePaginatedListsOverride", () => {
48+
it("reports no override before anything is recorded", () => {
49+
const { api } = harness([entry("A")]);
50+
expect(api().valueFor("A")).toBeUndefined();
51+
});
52+
53+
it("reports nothing for an undefined server id", () => {
54+
// No server is active before the first connect, and the composition root
55+
// asks for the display value on every render regardless.
56+
const { api } = harness([entry("A")]);
57+
act(() => api().record("A", true));
58+
expect(api().valueFor(undefined)).toBeUndefined();
59+
});
60+
61+
it("returns what was recorded for that server", () => {
62+
const { api } = harness([entry("A")]);
63+
act(() => api().record("A", true));
64+
expect(api().valueFor("A")).toBe(true);
65+
});
66+
67+
it("does not apply one server's override to another", () => {
68+
// The override is the *display* value, and the settings modal can be saved
69+
// for a server that is not the connected one — so a record made for B must
70+
// not change what A renders.
71+
const { api } = harness([entry("A"), entry("B")]);
72+
act(() => api().record("B", true));
73+
expect(api().valueFor("A")).toBeUndefined();
74+
expect(api().valueFor("B")).toBe(true);
75+
});
76+
77+
it("survives a server switch away and back (#2095)", () => {
78+
// Nothing about the list changes here — only which server is being asked
79+
// about, which is all a switch is. The previous app-wide slot was cleared
80+
// on every change of the active entry, which is what dropped A's value.
81+
const { api } = harness([entry("A"), entry("B")]);
82+
act(() => api().record("A", true));
83+
expect(api().valueFor("B")).toBeUndefined();
84+
expect(api().valueFor("A")).toBe(true);
85+
});
86+
87+
it("keeps a record for each server", () => {
88+
const { api } = harness([entry("A"), entry("B")]);
89+
act(() => api().record("A", true));
90+
act(() => api().record("B", false));
91+
expect(api().valueFor("A")).toBe(true);
92+
expect(api().valueFor("B")).toBe(false);
93+
});
94+
95+
it("replaces its own record for the same server", () => {
96+
const { api } = harness([entry("A")]);
97+
act(() => api().record("A", true));
98+
act(() => api().record("A", false));
99+
expect(api().valueFor("A")).toBe(false);
100+
});
101+
102+
it("stops applying once a fresh list read replaces that entry", () => {
103+
const { api, setServers } = harness([entry("A")]);
104+
act(() => api().record("A", true));
105+
// A successful `GET /api/servers` rebuilds the list, so the entry is a new
106+
// object even when the values are unchanged — which is exactly the case
107+
// that has to supersede the override, since an edit made outside the
108+
// Inspector can report back the value the override replaced.
109+
setServers([entry("A")]);
110+
expect(api().valueFor("A")).toBeUndefined();
111+
});
112+
113+
it("keeps a record whose own entry was not re-read", () => {
114+
// A read that rebuilds B says nothing about A. In practice the list is
115+
// rebuilt wholesale, but the check is per entry and must not be widened to
116+
// "any list change" — a reorder replaces the array while keeping every
117+
// entry object, and is not evidence of a fresher read.
118+
const a = entry("A");
119+
const { api, setServers } = harness([a, entry("B")]);
120+
act(() => api().record("A", true));
121+
setServers([entry("B"), a]);
122+
expect(api().valueFor("A")).toBe(true);
123+
});
124+
125+
it("records against the list as it stands when the record is written", () => {
126+
// A rollback records from a continuation that may be several renders old;
127+
// pairing with the list captured then would make the record look superseded
128+
// the moment it was written.
129+
const { api, setServers } = harness([entry("A")]);
130+
const stale = api();
131+
setServers([entry("A")]);
132+
act(() => stale.record("A", true));
133+
expect(api().valueFor("A")).toBe(true);
134+
});
135+
136+
it("drops superseded records rather than accumulating them", () => {
137+
// Housekeeping, asserted through the only surface it has: a record dropped
138+
// on the way past cannot come back, so re-reading after the entry is
139+
// restored to the list still reports nothing.
140+
const a = entry("A");
141+
const { api, setServers } = harness([a, entry("B")]);
142+
act(() => api().record("A", true));
143+
setServers([entry("A"), entry("B")]);
144+
act(() => api().record("B", true));
145+
setServers([a, entry("B")]);
146+
expect(api().valueFor("A")).toBeUndefined();
147+
});
148+
});

0 commit comments

Comments
 (0)