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): don't roll back an optimistic setting a failed reload didn't undo

Browse source 
Copilot review r1. `updateServerSettings` is also driven by the pagination
toggle, which updates optimistically and — on any rejection — reverts the
override and rolls the live client back to `prevSettings` (#1721). Making
the post-write reload throw handed that catch a rejection whose PUT had
*succeeded*, so it reverted the UI and the client to a value that no
longer matched disk.

Give the reload failure its own type, `ServerListReloadError`, and branch
on it at that call site: report it, leave the optimistic state alone. The
distinction is what the callers need — a failed write must roll back, a
failed read-back must not — and a type carries it without the call site
matching on a message.

The `useServers` mock in App.test.tsx now spreads `importOriginal()` so
the class is the genuine one; a stub would make the `instanceof` check
vacuously false and the branch untestable.

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:39:47 -04:00
commit fd21d0d528
4 changed files with 122 additions and 12 deletions
Download patch file Download diff file Expand all files Collapse all files

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

@ -225,7 +225,13 @@ const { updateServerSettingsSpy } = vi.hoisted(() => ({
const { clearToolsListChangedSpy } = vi.hoisted(() => ({
clearToolsListChangedSpy: vi.fn(),
}));
vi.mock("@inspector/core/react/useServers.js", () => ({
vi.mock("@inspector/core/react/useServers.js", async (importOriginal) => ({
// `ServerListReloadError` is a real class the pagination toggle branches on
// with `instanceof`, so it must be the genuine one — a stub would make the
// check vacuously false and the #1914 branch untestable (#1914 review r1).
...(await importOriginal<
typeof import("@inspector/core/react/useServers.js")
>()),
useServers: vi.fn(() => ({
servers: [SERVER_A],
addServer: vi.fn(),
@ -584,7 +590,10 @@ import { usePagedTools } from "@inspector/core/react/usePagedTools.js";
import { useMessageLog } from "@inspector/core/react/useMessageLog.js";
import { useInspectorClient } from "@inspector/core/react/useInspectorClient.js";
import { useSettingsDraft } from "@inspector/core/react/useSettingsDraft.js";
import { useServers } from "@inspector/core/react/useServers.js";
import {
ServerListReloadError,
useServers,
} from "@inspector/core/react/useServers.js";
import type {
InspectorServerSettings,
MessageEntry,
@ -2423,6 +2432,40 @@ describe("App paginated list pagination toggle (#1721)", () => {
};
expect(lastPush?.paginatedLists).toBeFalsy();
});
it("keeps the optimistic toggle when only the list reload failed (#1914)", async () => {
// The mirror of the test above. `refreshAfterWrite` rejects with a
// `ServerListReloadError` when the PUT landed and only reading the list
// back failed — the setting IS on disk, so the #1721 rollback would put
// the UI and the live client on the value the user just changed away
// from, and contradict what a reload would show.
const user = userEvent.setup();
updateServerSettingsSpy.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("paginated-on"));
await waitFor(() =>
expect(screen.getByTestId("tools-paginated")).toHaveTextContent("true"),
);
// Give the rejection a turn to land, then confirm nothing reverted it.
await act(async () => {
await Promise.resolve();
});
expect(screen.getByTestId("tools-paginated")).toHaveTextContent("true");
const client = clientInstances[0] as unknown as {
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

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

@ -98,7 +98,10 @@ import {
} from "@inspector/core/auth/index.js";
import { RemoteInspectorClientStorage } from "@inspector/core/mcp/remote/index.js";
import { useInspectorClient } from "@inspector/core/react/useInspectorClient.js";
import { useServers } from "@inspector/core/react/useServers.js";
import {
ServerListReloadError,
useServers,
} from "@inspector/core/react/useServers.js";
import { useSettingsDraft } from "@inspector/core/react/useSettingsDraft.js";
import { useClientSettingsDraft } from "@inspector/core/react/useClientSettingsDraft.js";
import { useEmaIdpLoginState } from "@inspector/core/react/useEmaIdpLoginState.js";
@ -3761,14 +3764,23 @@ function App() {
activeServerId,
next,
).catch((err: unknown) => {
// Persist failed: revert the optimistic override (the effect only
// clears it when the persisted value changes, which won't happen here)
// and roll the live client setting back, so the UI and client reflect
// the value that's actually on disk rather than the failed edit (#1721).
setPaginatedListsOverride(null);
inspectorClient?.setServerSettings(prevSettings);
// A ServerListReloadError means the PUT landed and only reading the
// list back failed, so the new setting IS on disk (#1914). Rolling
// back here would put the UI and the live client on the *old* value
// and contradict it — report it and leave the optimistic state alone.
const reloadFailed = err instanceof ServerListReloadError;
if (!reloadFailed) {
// Persist failed: revert the optimistic override (the effect only
// clears it when the persisted value changes, which won't happen here)
// and roll the live client setting back, so the UI and client reflect
// the value that's actually on disk rather than the failed edit (#1721).
setPaginatedListsOverride(null);
inspectorClient?.setServerSettings(prevSettings);
}
notifications.show({
title: "Failed to save pagination setting",
title: reloadFailed
? "Pagination setting saved, but the server list did not reload"
: "Failed to save pagination setting",
message: err instanceof Error ? err.message : String(err),
color: "red",
});

34
clients/web/src/test/core/react/useServers.test.tsx
View file

@ -10,7 +10,10 @@ import { act, renderHook, waitFor } from "@testing-library/react";
import { mkdtempSync, rmSync, writeFileSync, readFileSync } from "node:fs";
import { tmpdir } from "node:os";
import { join } from "node:path";
import { useServers } from "@inspector/core/react/useServers";
import {
ServerListReloadError,
useServers,
} from "@inspector/core/react/useServers";
import { createRemoteApp } from "@inspector/core/mcp/remote/node/server";
import { DEFAULT_SEED_CONFIG } from "@inspector/core/mcp/serverList";
import { InMemorySecretStore } from "@inspector/core/auth/node/secret-store";
@ -1316,6 +1319,35 @@ describe("useServers", () => {
});
});
it("tags a post-write reload failure as ServerListReloadError, not a failed write (#1914)", async () => {
// The pagination toggle updates optimistically and reverts the live
// client on a rejection. That is right for a failed PUT and wrong for a
// failed reload — the setting IS on disk — so the two must be tellable
// apart by type, not by message matching.
const { fetchFn, startFailing } = failGetAfterWrite("disk full");
const { result } = renderHook(() =>
useServers({ baseUrl: "http://test.local", fetchFn }),
);
await waitFor(() => expect(result.current.loading).toBe(false));
startFailing();
await act(async () => {
await expect(
result.current.addServer("alpha", { type: "stdio", command: "node" }),
).rejects.toBeInstanceOf(ServerListReloadError);
});
// A rejected *write* stays a plain Error, so a caller branching on the
// type still rolls back for the case the rollback was written for.
await act(async () => {
const err = await result.current
.addServer("alpha", { type: "stdio", command: "node" })
.catch((e: unknown) => e);
expect(err).toBeInstanceOf(Error);
expect(err).not.toBeInstanceOf(ServerListReloadError);
});
});
it("removeServer rejects when the post-write list reload fails (#1914)", async () => {
const { fetchFn, startFailing } = failGetAfterWrite("disk full");
const { result } = renderHook(() =>

25
core/react/useServers.ts
View file

@ -62,6 +62,25 @@ export interface UseServersResult {
importSource: (type: string) => Promise<ImportSourceResult>;
}
/**
* Thrown when a mutation's *write* landed but reading the list back
* afterwards failed — see `refreshAfterWrite`.
*
* Distinct from a rejected write because the two call for opposite handling
* in an optimistic caller. The pagination toggle in `App.tsx` reverts its
* optimistic override and rolls the live client back on a rejection, which is
* right for a failed PUT and wrong here: the setting *is* on disk, so the
* rollback would show the user a value that no longer matches it. Callers
* that update optimistically must branch on this; callers that only report
* (the modals) can treat it as any other error.
*/
export class ServerListReloadError extends Error {
constructor(message: string, options?: ErrorOptions) {
super(message, options);
this.name = "ServerListReloadError";
}
}
function buildHeaders(
authToken: string | undefined,
includeJsonBody: boolean,
@ -182,6 +201,10 @@ export function useServers(opts: UseServersOptions): UseServersResult {
// back failed. A bare "could not add server" would send the user straight
// into a retry that trips the duplicate-id check, which is the dead end the
// original report described.
//
// The rejection is a `ServerListReloadError` rather than a bare `Error` so
// an optimistic caller can tell it from a failed write and skip its
// rollback — reverting here would contradict what is actually on disk.
const refreshAfterWrite = useCallback(
async (verb: string): Promise<void> => {
setLoading(true);
@ -191,7 +214,7 @@ export function useServers(opts: UseServersOptions): UseServersResult {
} catch (err) {
const message = err instanceof Error ? err.message : String(err);
setError(message);
throw new Error(
throw new ServerListReloadError(
`The server was ${verb}, but the server list could not be reloaded: ${message}`,
{ cause: err },
);