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): follow a rename whose list reload failed, and drop a double cast

Browse source 
Copilot review r2.

`onConfigSubmit` moved `activeServerId` to the renamed id only after
`updateServer` resolved. Once a failed list reload rejects, the rename is
still on disk — so the active id stayed on the old one and the next
successful refresh left it pointing at a row that no longer exists. Apply
the rename from the catch when the rejection is a `ServerListReloadError`,
then rethrow so the modal still shows the error. `addServerHighlighted`
gets the same treatment: the row is on disk, so it belongs in the
highlight batch the next successful refresh renders.

`activeServerId` is not rendered anywhere, so the test reads it back
through the one handler that hands it to a spy — the pagination toggle's
`updateServerSettings(activeServerId, …)` — after letting the renamed row
land as a later refresh would. It fails without the catch-side call. The
new describe installs its own `useServers` return value and restores the
previous implementation afterwards, since earlier describes' overrides
outlive them (which is why the first draft's spy was never called).

Also replaced the `as unknown as` in the new test with a single
`EventTarget & {…}` intersection, per the TypeScript rules. The other 19
in this file are the same pattern and are left for a separate sweep.

The third comment — that the pagination toggle's rollback baseline goes
stale after a failed list read — is a real defect but pre-existing and
unchanged by this PR; the end state is identical before and after it.
Filed as #2089 rather than widening this 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 09:58:56 -04:00
commit 269ff5fefc
2 changed files with 132 additions and 7 deletions
Download patch file Download diff file Expand all files Collapse all files

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

@ -220,6 +220,12 @@ const SERVER_A = {
const { updateServerSettingsSpy } = vi.hoisted(() => ({
updateServerSettingsSpy: vi.fn(() => Promise.resolve()),
}));
// Same idea for the edit-modal rename path (#1914): the test needs to make the
// PUT's *reload* fail, which only a spy it controls can do.
const { updateServerSpy } = vi.hoisted(() => ({
updateServerSpy: vi.fn(() => Promise.resolve()),
}));
// Stable spy for the tools list-changed acknowledgement, so a test can assert
// the paginated Refresh clears the indicator (#1721).
const { clearToolsListChangedSpy } = vi.hoisted(() => ({
@ -235,7 +241,7 @@ vi.mock("@inspector/core/react/useServers.js", async (importOriginal) => ({
useServers: vi.fn(() => ({
servers: [SERVER_A],
addServer: vi.fn(),
updateServer: vi.fn(),
updateServer: updateServerSpy,
updateServerSettings: updateServerSettingsSpy,
removeServer: vi.fn(),
})),
@ -418,6 +424,7 @@ vi.mock("./components/views/InspectorView/InspectorView", () => ({
onClearCompletedTasks: () => void;
onRefreshTasks: () => void;
onServerSettings: (id: string) => void;
onServerEdit: (id: string) => void;
onClearProtocol: () => void;
onReplayProtocol: (id: string) => void;
onTogglePinProtocol: (id: string) => void;
@ -543,6 +550,9 @@ vi.mock("./components/views/InspectorView/InspectorView", () => ({
</button>
<button onClick={() => props.onSetLogLevel("debug")}>set-level</button>
<button onClick={() => props.onServerSettings("A")}>open-settings</button>
{/* The real server grid (and its Edit button) lives inside this mocked
view, so the edit modal is only reachable through its callback. */}
<button onClick={() => props.onServerEdit("A")}>edit-server</button>
<span data-testid="pinned-history">
{Array.from(props.pinnedProtocolIds ?? []).join(",")}
</span>
@ -2458,7 +2468,10 @@ describe("App paginated list pagination toggle (#1721)", () => {
await Promise.resolve();
});
expect(screen.getByTestId("tools-paginated")).toHaveTextContent("true");
const client = clientInstances[0] as unknown as {
// Single intersection assertion rather than the `as unknown as` this file
// uses elsewhere: the fake client really is an EventTarget, and widening
// it keeps that relationship instead of erasing it (#1914 r2).
const client = clientInstances[0] as EventTarget & {
setServerSettings: ReturnType<typeof vi.fn>;
};
const lastPush = client.setServerSettings.mock.calls.at(-1)?.[0] as {
@ -2468,6 +2481,92 @@ describe("App paginated list pagination toggle (#1721)", () => {
});
});
// `onConfigSubmit` moves `activeServerId` to the new id only after
// `updateServer` *resolves*. Once a failed list reload rejects, the rename is
// still on disk — so without the catch-side `followRename` the active id stays
// on the old one, and the next successful refresh leaves it pointing at a row
// that no longer exists (#1914 review r2).
describe("App edit rename with a failed list reload (#1914)", () => {
// Earlier describes install their own `useServers` return value, which
// outlives them — so this block installs its own and puts the previous
// implementation back, rather than trusting whatever leaked in.
let restoreUseServers: (() => void) | undefined;
const serversResult = (ids: string[]): ReturnType<typeof useServers> => ({
servers: ids.map((id) => ({ ...SERVER_A, id }) as ServerEntry),
loading: false,
error: undefined,
refresh: vi.fn().mockResolvedValue(undefined),
addServer: vi.fn().mockResolvedValue(undefined),
updateServer: updateServerSpy,
updateServerSettings: updateServerSettingsSpy,
removeServer: vi.fn().mockResolvedValue(undefined),
reorderServers: vi.fn().mockResolvedValue(undefined),
importSource: vi.fn().mockResolvedValue({ servers: {} }),
});
beforeEach(() => {
clientInstances.length = 0;
const previous = vi.mocked(useServers).getMockImplementation();
restoreUseServers = previous
? () => vi.mocked(useServers).mockImplementation(previous)
: undefined;
updateServerSpy.mockClear();
updateServerSettingsSpy.mockClear();
vi.mocked(useServers).mockReturnValue(serversResult(["A"]));
});
afterEach(() => {
restoreUseServers?.();
});
it("follows the rename so the active id isn't orphaned (#1914)", async () => {
// `activeServerId` isn't rendered anywhere, so it's read back through the
// one handler that passes it straight to a spy: the pagination toggle
// calls `updateServerSettings(activeServerId, …)`.
const user = userEvent.setup();
updateServerSpy.mockRejectedValueOnce(
new ServerListReloadError(
"The server was saved, but the server list could not be reloaded: disk full",
),
);
renderWithMantine(<App />);
await user.click(screen.getByText("connect"));
await waitFor(() => expect(clientInstances).toHaveLength(1));
await user.click(screen.getByText("edit-server"));
const idField = await screen.findByLabelText(/Server ID/);
await user.clear(idField);
await user.type(idField, "A-renamed");
await user.click(screen.getByRole("button", { name: "Save" }));
await waitFor(() =>
expect(updateServerSpy).toHaveBeenCalledWith(
"A",
"A-renamed",
expect.anything(),
),
);
// The rejection still reaches the modal, which stays open showing it.
expect(
await screen.findByText(/could not be reloaded: disk full/),
).toBeInTheDocument();
// Now let the row land, as a later successful refresh would, and read the
// active id back off the toggle.
vi.mocked(useServers).mockReturnValue(serversResult(["A-renamed"]));
await user.click(screen.getByRole("button", { name: "Cancel" }));
await user.click(screen.getByText("paginated-on"));
await waitFor(() =>
expect(updateServerSettingsSpy).toHaveBeenCalledWith(
"A-renamed",
expect.objectContaining({ paginatedLists: true }),
),
);
});
});
// A background command runs through `runCommandInBackground`, which owns the
// rejection. Before #2049 the 17 `void runWithCommandAuthRecovery(...)` sites
// had no handler, so any non-auth failure of the wrapped operation became an

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

@ -4075,8 +4075,20 @@ function App() {
// opens (see the menu handlers).
const addServerHighlighted = useCallback(
async (id: string, config: MCPServerConfig) => {
await addServer(id, config);
setHighlightedServerIds((ids) => (ids.includes(id) ? ids : [...ids, id]));
const markAdded = () =>
setHighlightedServerIds((ids) =>
ids.includes(id) ? ids : [...ids, id],
);
try {
await addServer(id, config);
} catch (err) {
// A failed list reload still means the row is on disk, so it belongs
// in the highlight batch — the next successful refresh is what
// renders it, and it would otherwise arrive unmarked (#1914 r2).
if (err instanceof ServerListReloadError) markAdded();
throw err;
}
markAdded();
},
[addServer],
);
@ -4086,10 +4098,24 @@ function App() {
async (id: string, config: MCPServerConfig) => {
if (configModal?.mode === "edit" && configModal.targetId) {
const originalId = configModal.targetId;
await updateServer(originalId, id, config);
if (originalId === activeServerId && id !== originalId) {
setActiveServerId(id);
const followRename = () => {
if (originalId === activeServerId && id !== originalId) {
setActiveServerId(id);
}
};
try {
await updateServer(originalId, id, config);
} catch (err) {
// A `ServerListReloadError` means the PUT landed and only reading
// the list back failed — the rename IS on disk. The active
// selection has to follow it anyway, or the next successful
// refresh leaves `activeServerId` pointing at an id that no longer
// exists (#1914 r2). Still rethrow, so the modal shows the reload
// error rather than closing as if nothing happened.
if (err instanceof ServerListReloadError) followRename();
throw err;
}
followRename();
return;
}
// add or clone