diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx
index fdf01413..3d95bf9d 100644
--- a/clients/web/src/App.test.tsx
+++ b/clients/web/src/App.test.tsx
@@ -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();
+ 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;
+ };
+ 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
diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx
index 67f7d0ff..a79098ca 100644
--- a/clients/web/src/App.tsx
+++ b/clients/web/src/App.tsx
@@ -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",
});
diff --git a/clients/web/src/test/core/react/useServers.test.tsx b/clients/web/src/test/core/react/useServers.test.tsx
index 0c011109..2ca1d334 100644
--- a/clients/web/src/test/core/react/useServers.test.tsx
+++ b/clients/web/src/test/core/react/useServers.test.tsx
@@ -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(() =>
diff --git a/core/react/useServers.ts b/core/react/useServers.ts
index bcba4690..3b66a0c8 100644
--- a/core/react/useServers.ts
+++ b/core/react/useServers.ts
@@ -62,6 +62,25 @@ export interface UseServersResult {
importSource: (type: string) => Promise;
}
+/**
+ * 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 => {
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 },
);