From fd21d0d52801c0dcad3ecf8803957db8a0bdbef7 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 23 Aug 2026 09:39:47 -0400 Subject: [PATCH] fix(web): don't roll back an optimistic setting a failed reload didn't undo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4 Signed-off-by: cliffhall --- clients/web/src/App.test.tsx | 47 ++++++++++++++++++- clients/web/src/App.tsx | 28 +++++++---- .../src/test/core/react/useServers.test.tsx | 34 +++++++++++++- core/react/useServers.ts | 25 +++++++++- 4 files changed, 122 insertions(+), 12 deletions(-) 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 }, );