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

Merge remote-tracking branch 'origin/v2/chore/2086-cross-browser-app-smokes' into v2/chore/2086-cross-browser-app-smokes

Browse source
This commit is contained in:
cliffhall 2026-08-25 21:00:53 -04:00
commit 1404dd9980
4 changed files with 388 additions and 36 deletions
Download patch file Download diff file Expand all files Collapse all files

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

@ -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

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

@ -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<
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 rather than on the boolean it carries: the
// override can hold a *concrete* value (a rollback sets one instead of
// clearing), and if a later successful read reports the value that override
// 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 optimistic override, held per server and superseded by the first
// successful list read that rebuilds that server's entry. Per server rather
// than one app-wide slot cleared on every active-entry change: the previous
// shape dropped the override on a plain server switch, so an A → B → A round
// trip fell back to A's stale entry and showed a value neither disk nor the
// live client held (#2095).
const paginatedListsOverride = usePaginatedListsOverride(servers);
const paginatedLists =
paginatedListsOverride.valueFor(activeServerId) ?? 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) {
setPaginatedListsOverride(next.paginatedLists ?? false);
if (!settled) return;
// 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
// 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.
// The client 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.
// The override is recorded whatever is active by the time this
// rejection arrives: it is keyed by this write's server and is only
// ever displayed while that server is the active one, so a switch in
// between costs nothing and dropping it would leave the stale entry
// to answer for A the next time it comes back (#2095).
//
// 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) {
setPaginatedListsOverride(value.paginatedLists ?? false);
if (!settled) return;
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) {
setPaginatedListsOverride(baseline.paginatedLists ?? false);
applyLiveServerSettings(baseline);
if (baseline) {
paginatedListsOverride.record(id, baseline.paginatedLists ?? false);
if (activeServerIdRef.current === id) {
applyLiveServerSettings(baseline);
}
}
throw err;
}

148
clients/web/src/hooks/usePaginatedListsOverride.test.tsx Normal file
View file

@ -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();
});
});

120
clients/web/src/hooks/usePaginatedListsOverride.ts Normal file
View file

@ -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]);
}