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): surface a failed post-write server-list reload

Browse source 
`useServers`' mutators awaited `refresh()`, whose failure was swallowed
into the hook's `error` state rather than thrown. `ServerConfigModal`
renders `submitError` only for something `onSubmit` threw, so a
successful `POST /api/servers` followed by a failed `GET /api/servers`
closed the Add Server modal as though the whole thing had worked — no
inline error, no toast, and no new row, since the list read is what
would have produced it. That is the silent failure reported in #1914;
#1848 removed the trigger (a keyring-less container 500'ing the list
read) but not the swallow, so any other cause reproduces it.

Split the list read into `loadServers` and give the mutators their own
`refreshAfterWrite`, which records the failure in `error` as before and
then rethrows. `refreshInternal` is unchanged for the mount effect and
the SSE loop — neither has anywhere to put a rejection.

The thrown message says the write landed, because it did; only reading
the list 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.

Every consumer of these mutators already handles a rejection:
ServerConfigModal and ServerRemoveConfirmModal into an inline error,
useSettingsDraft and the pagination toggle into a toast, and both
import flows into their per-entry outcome list.

Closes #1914

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:21:46 -04:00
commit 4379d17f76
2 changed files with 210 additions and 18 deletions
Download patch file Download diff file Expand all files Collapse all files

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

@ -1199,6 +1199,157 @@ describe("useServers", () => {
}
});
/**
* #1914 — a write that lands but whose list reload fails must reject.
*
* `ServerConfigModal` renders `submitError` only for something `onSubmit`
* threw, so while the post-write refresh swallowed its failure into the
* `error` state the Add Server modal closed as though nothing had gone
* wrong. That is the "silent failure" in the report: the POST succeeded,
* the GET 500'd, and the user saw neither the new row nor an error.
*
* `failGetAfterWrite` lets the seed load and the write itself through, then
* fails every subsequent list read — the shape a container with no
* reachable keychain produced before #1848.
*/
function failGetAfterWrite(message: string): {
fetchFn: typeof fetch;
startFailing: () => void;
} {
let failing = false;
const fetchFn: typeof fetch = async (input, init) => {
const req =
input instanceof Request
? input
: new Request(input as string | URL, init);
if (failing && req.method === "GET" && req.url.endsWith("/api/servers")) {
return new Response(JSON.stringify({ error: message }), {
status: 500,
headers: { "Content-Type": "application/json" },
});
}
return h.fetchFn(req);
};
return {
fetchFn,
startFailing: () => {
failing = true;
},
};
}
it("addServer rejects when the post-write list reload fails (#1914)", async () => {
const { fetchFn, startFailing } = failGetAfterWrite("keyring unavailable");
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.toThrow(
/The server was added, but the server list could not be reloaded: keyring unavailable/,
);
});
// The write itself landed, and the message says so — a bare failure would
// send the user into a retry that trips the duplicate-id check.
expect(readConfig(h.configPath).mcpServers.alpha).toEqual({
type: "stdio",
command: "node",
});
// Still recorded in `error` for any list-level UI reading it.
await waitFor(() =>
expect(result.current.error).toBe("keyring unavailable"),
);
expect(result.current.loading).toBe(false);
});
it("updateServer rejects when the post-write list reload fails (#1914)", async () => {
const { fetchFn, startFailing } = failGetAfterWrite("disk full");
const { result } = renderHook(() =>
useServers({ baseUrl: "http://test.local", fetchFn }),
);
await waitFor(() => expect(result.current.loading).toBe(false));
const existing = result.current.servers[0]!.id;
startFailing();
await act(async () => {
await expect(
result.current.updateServer(existing, existing, {
type: "stdio",
command: "node",
}),
).rejects.toThrow(
/The server was saved, but the server list could not be reloaded: disk full/,
);
});
});
it("updateServerSettings rejects when the post-write list reload fails (#1914)", async () => {
const { fetchFn, startFailing } = failGetAfterWrite("disk full");
const { result } = renderHook(() =>
useServers({ baseUrl: "http://test.local", fetchFn }),
);
await waitFor(() => expect(result.current.loading).toBe(false));
const existing = result.current.servers[0]!.id;
startFailing();
await act(async () => {
await expect(
result.current.updateServerSettings(existing, {
headers: [],
env: [],
metadata: [],
roots: [],
connectionTimeout: 5000,
requestTimeout: 30000,
taskTtl: 30000,
maxFetchRequests: 1000,
paginatedLists: true,
}),
).rejects.toThrow(
/The server was saved, but the server list could not be reloaded: disk full/,
);
});
});
it("removeServer rejects when the post-write list reload fails (#1914)", async () => {
const { fetchFn, startFailing } = failGetAfterWrite("disk full");
const { result } = renderHook(() =>
useServers({ baseUrl: "http://test.local", fetchFn }),
);
await waitFor(() => expect(result.current.loading).toBe(false));
const existing = result.current.servers[0]!.id;
startFailing();
await act(async () => {
await expect(result.current.removeServer(existing)).rejects.toThrow(
/The server was removed, but the server list could not be reloaded: disk full/,
);
});
expect(readConfig(h.configPath).mcpServers[existing]).toBeUndefined();
});
it("a plain refresh() still resolves on a failed list read, recording the error (#1914)", async () => {
// The rethrow is scoped to post-write reloads. `refresh()` is awaited by
// the mount effect and the SSE loop, neither of which has anywhere to put
// a rejection, so it must keep swallowing into `error`.
const { fetchFn, startFailing } = failGetAfterWrite("keyring unavailable");
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.refresh()).resolves.toBeUndefined();
});
expect(result.current.error).toBe("keyring unavailable");
});
it("uses DEFAULT_SEED_CONFIG keys on the first load (seed-write contract)", async () => {
const { result } = renderHook(() =>
useServers({ baseUrl: "http://test.local", fetchFn: h.fetchFn }),

77
core/react/useServers.ts
View file

@ -132,33 +132,74 @@ export function useServers(opts: UseServersOptions): UseServersResult {
const [loading, setLoading] = useState<boolean>(true);
const [error, setError] = useState<string | undefined>(undefined);
// The list read itself, throwing on failure. Every refresh path goes
// through it; they differ only in what they do with a rejection.
const loadServers = useCallback(async (): Promise<ServerEntry[]> => {
const res = await doFetch(`${base}/api/servers`, {
method: "GET",
headers: buildHeaders(authToken, false),
});
if (!res.ok) {
throw new Error(await readErrorMessage(res));
}
const body = (await res.json()) as MCPConfig;
return mcpConfigToServerEntries(body);
}, [base, authToken, doFetch]);
// Inner refresh that the public `refresh` and the SSE-triggered background
// refresh both share. `background` skips the loading-state toggle so
// consumers rendering a spinner or skeleton don't flash on every external
// mcp.json edit — the existing list stays on screen while the re-fetch
// resolves. `error` is still reset / set either way so a real error
// surfaces even from a background refresh.
//
// A failure is recorded in `error` and NOT rethrown: the two callers are
// the mount effect and the SSE loop, neither of which has anywhere to put
// a rejection. Post-write reloads use `refreshAfterWrite` instead.
const refreshInternal = useCallback(
async (background: boolean): Promise<void> => {
if (!background) setLoading(true);
setError(undefined);
try {
const res = await doFetch(`${base}/api/servers`, {
method: "GET",
headers: buildHeaders(authToken, false),
});
if (!res.ok) {
throw new Error(await readErrorMessage(res));
}
const body = (await res.json()) as MCPConfig;
setServers(mcpConfigToServerEntries(body));
setServers(await loadServers());
} catch (err) {
setError(err instanceof Error ? err.message : String(err));
} finally {
if (!background) setLoading(false);
}
},
[base, authToken, doFetch],
[loadServers],
);
// Post-write reload, used by every mutator. Unlike `refreshInternal` it
// rethrows, because a mutator's caller learns that something went wrong
// only from a rejection — `ServerConfigModal` renders `submitError` for a
// thrown error and nothing else, so a successful POST followed by a failed
// GET closed the Add Server modal as though the whole thing had worked
// (#1914). The reported failure was silent for exactly this reason.
//
// The message says the write landed, because it did: only reading the list
// 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.
const refreshAfterWrite = useCallback(
async (verb: string): Promise<void> => {
setLoading(true);
setError(undefined);
try {
setServers(await loadServers());
} catch (err) {
const message = err instanceof Error ? err.message : String(err);
setError(message);
throw new Error(
`The server was ${verb}, but the server list could not be reloaded: ${message}`,
{ cause: err },
);
} finally {
setLoading(false);
}
},
[loadServers],
);
const refresh = useCallback(
@ -241,9 +282,9 @@ export function useServers(opts: UseServersOptions): UseServersResult {
if (!res.ok) {
throw new Error(await readErrorMessage(res));
}
await refresh();
await refreshAfterWrite("added");
},
[base, authToken, doFetch, refresh],
[base, authToken, doFetch, refreshAfterWrite],
);
const importSource = useCallback(
@ -282,9 +323,9 @@ export function useServers(opts: UseServersOptions): UseServersResult {
if (!res.ok) {
throw new Error(await readErrorMessage(res));
}
await refresh();
await refreshAfterWrite("saved");
},
[base, authToken, doFetch, refresh],
[base, authToken, doFetch, refreshAfterWrite],
);
const updateServerSettings = useCallback(
@ -307,9 +348,9 @@ export function useServers(opts: UseServersOptions): UseServersResult {
if (!res.ok) {
throw new Error(await readErrorMessage(res));
}
await refresh();
await refreshAfterWrite("saved");
},
[base, authToken, doFetch, refresh],
[base, authToken, doFetch, refreshAfterWrite],
);
const removeServer = useCallback(
@ -324,9 +365,9 @@ export function useServers(opts: UseServersOptions): UseServersResult {
if (!res.ok) {
throw new Error(await readErrorMessage(res));
}
await refresh();
await refreshAfterWrite("removed");
},
[base, authToken, doFetch, refresh],
[base, authToken, doFetch, refreshAfterWrite],
);
const reorderServers = useCallback(