modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 2

fix(web): let a successful read supersede a rollback, and scope it to its server

Browse source 
Review round 4 on #2091, three findings.

A rollback sets the override to a concrete value rather than clearing
it, so the effect that drops it could not key on the persisted boolean
alone: if a later read reported the value the override replaced — an
edit made outside the Inspector overtaking the write — neither that
boolean nor the server id changed and the UI stayed stuck on the
override. It now keys on the active `servers` entry too, which a
successful read always rebuilds.

The override is also a single app-wide boolean and the live client
belongs to whichever server is connected now, so a rejection arriving
after the user switched servers rendered one server's value on another.
The UI rollback and the client push are now applied only while the
failing write's server is still active; the toast is not gated.

The settings-modal writer had no coverage: `useSettingsDraft` is mocked
for `App.test.tsx`, so its `onPersist` never ran and dropping
`begin`/`landed` there would have gone unnoticed. A test now invokes the
callback the App handed the hook, lands a save, and asserts a failed
toggle rolls back to the modal's value.

Both new tests fail without their fix.

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>
This commit is contained in:
cliffhall 2026-08-23 12:16:22 -04:00
commit 2aa97c1833
2 changed files with 115 additions and 5 deletions
Download patch file Download diff file Expand all files Collapse all files

96
clients/web/src/App.test.tsx
View file

@ -2521,6 +2521,102 @@ describe("App paginated list pagination toggle (#1721)", () => {
};
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
// later read reports the value the override replaced — an edit made outside
// the Inspector overtaking the write — neither that boolean nor the server
// id changes, and the UI would stay stuck on the override forever. A read
// rebuilds the list, so the entry object is the signal that supersedes it.
const user = userEvent.setup();
const previousUseServers = vi.mocked(useServers).getMockImplementation();
try {
const { rerender } = renderWithMantine(<App />);
await user.click(screen.getByText("connect"));
await waitFor(() => expect(clientInstances).toHaveLength(1));
// A write lands `true` while list reads are stale, then a toggle fails —
// leaving the override on `true` with the entry still reporting `false`.
const draftOptions = vi.mocked(useSettingsDraft).mock.calls.at(-1)?.[0];
if (!draftOptions) throw new Error("useSettingsDraft was never called");
await act(async () => {
await draftOptions.onPersist("A", {
...settingsWithRoots([]),
paginatedLists: true,
});
});
updateServerSettingsSpy.mockRejectedValueOnce(new Error("disk full"));
await user.click(screen.getByText("paginated-off"));
await waitFor(() =>
expect(screen.getByTestId("tools-paginated")).toHaveTextContent("true"),
);
// Now a list read succeeds and still says `false` — same boolean, same
// server id, new entry object. It is authoritative and must win.
vi.mocked(useServers).mockReturnValue({
servers: [{ ...SERVER_A } as ServerEntry],
loading: false,
error: undefined,
refresh: vi.fn().mockResolvedValue(undefined),
addServer: vi.fn(),
updateServer: vi.fn(),
updateServerSettings: updateServerSettingsSpy,
removeServer: vi.fn(),
reorderServers: vi.fn(),
importSource: vi.fn().mockResolvedValue({ servers: {} }),
});
rerender(<App />);
await waitFor(() =>
expect(screen.getByTestId("tools-paginated")).toHaveTextContent(
"false",
),
);
} finally {
if (previousUseServers) {
vi.mocked(useServers).mockImplementation(previousUseServers);
}
}
});
it("rolls back to a settings-modal save that landed, not just a toggle (#2089)", async () => {
// The two settings writers feed one record, and this is the half the
// toggle-driven tests cannot reach: `useSettingsDraft` is mocked for this
// file, so its `onPersist` never runs on its own and a regression that
// dropped `begin`/`landed` from the modal path would go unnoticed. The
// callback the App handed the hook is invoked directly instead.
const user = userEvent.setup();
renderWithMantine(<App />);
await user.click(screen.getByText("connect"));
await waitFor(() => expect(clientInstances).toHaveLength(1));
const draftOptions = vi.mocked(useSettingsDraft).mock.calls.at(-1)?.[0];
if (!draftOptions) throw new Error("useSettingsDraft was never called");
const saved: InspectorServerSettings = {
...settingsWithRoots([]),
paginatedLists: true,
};
// The save lands; the list read behind it is what stays stale, so `servers`
// (mocked to a fixed entry here) never reports it.
await act(async () => {
await draftOptions.onPersist("A", saved);
});
expect(updateServerSettingsSpy).toHaveBeenCalledWith("A", saved);
updateServerSettingsSpy.mockRejectedValueOnce(new Error("disk full"));
await user.click(screen.getByText("paginated-off"));
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);
});
});
// A background command runs through `runCommandInBackground`, which owns the

24
clients/web/src/App.tsx
View file

@ -1166,17 +1166,24 @@ function App() {
const lastPersistedSettings = useLastPersistedSettings(servers);
// The active server's persisted paginated setting drives the display mode.
// The sidebar toggle edits it (optimistically, below) and persists it.
const activeServerEntry = servers.find((s) => s.id === activeServerId);
const persistedPaginatedLists =
servers.find((s) => s.id === activeServerId)?.settings?.paginatedLists ??
false;
activeServerEntry?.settings?.paginatedLists ?? false;
const [paginatedListsOverride, setPaginatedListsOverride] = useState<
boolean | null
>(null);
// 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]);
}, [persistedPaginatedLists, activeServerId, activeServerEntry]);
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
@ -3791,10 +3798,17 @@ function App() {
// baseline rather than cleared, because clearing it falls back to
// `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
// override is a single app-wide boolean and the live client belongs to
// whichever server is connected now, so a rejection arriving after the
// user switched would render this server's value on another one.
const baseline =
lastPersistedSettings.resolve(activeServerId) ?? EMPTY_SETTINGS;
setPaginatedListsOverride(baseline.paginatedLists ?? false);
inspectorClient?.setServerSettings(baseline);
if (activeServerIdRef.current === activeServerId) {
setPaginatedListsOverride(baseline.paginatedLists ?? false);
inspectorClient?.setServerSettings(baseline);
}
notifications.show({
title: "Failed to save pagination setting",
message: err instanceof Error ? err.message : String(err),