diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index fabee0b9..db5a28d5 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -223,8 +223,9 @@ const { updateServerSettingsSpy } = vi.hoisted(() => ({ // 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(() => ({ +const { updateServerSpy, addServerSpy } = vi.hoisted(() => ({ updateServerSpy: vi.fn(() => Promise.resolve()), + addServerSpy: vi.fn(() => Promise.resolve()), })); // Stable spy for the tools list-changed acknowledgement, so a test can assert // the paginated Refresh clears the indicator (#1721). @@ -240,7 +241,7 @@ vi.mock("@inspector/core/react/useServers.js", async (importOriginal) => ({ >()), useServers: vi.fn(() => ({ servers: [SERVER_A], - addServer: vi.fn(), + addServer: addServerSpy, updateServer: updateServerSpy, updateServerSettings: updateServerSettingsSpy, removeServer: vi.fn(), @@ -425,6 +426,8 @@ vi.mock("./components/views/InspectorView/InspectorView", () => ({ onRefreshTasks: () => void; onServerSettings: (id: string) => void; onServerEdit: (id: string) => void; + onServerAdd: () => void; + highlightedServerIds?: string[]; onClearProtocol: () => void; onReplayProtocol: (id: string) => void; onTogglePinProtocol: (id: string) => void; @@ -550,9 +553,14 @@ vi.mock("./components/views/InspectorView/InspectorView", () => ({ - {/* The real server grid (and its Edit button) lives inside this mocked - view, so the edit modal is only reachable through its callback. */} + {/* The real server grid (and its Add / Edit controls) lives inside this + mocked view, so the config modal is only reachable through these + callbacks — and the highlight batch only observable through this prop. */} + + + {(props.highlightedServerIds ?? []).join(",") || "none"} + {Array.from(props.pinnedProtocolIds ?? []).join(",")} @@ -2481,12 +2489,11 @@ 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)", () => { +// A post-write reload failure rejects *after* the write landed, so every piece +// of state `onConfigSubmit` used to apply on the resolve path has to be applied +// on the reject path too — the rename target and the highlight batch both +// describe a row that really is on disk (#1914 review r2 / r3). +describe("App config submit 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. @@ -2497,7 +2504,7 @@ describe("App edit rename with a failed list reload (#1914)", () => { loading: false, error: undefined, refresh: vi.fn().mockResolvedValue(undefined), - addServer: vi.fn().mockResolvedValue(undefined), + addServer: addServerSpy, updateServer: updateServerSpy, updateServerSettings: updateServerSettingsSpy, removeServer: vi.fn().mockResolvedValue(undefined), @@ -2512,6 +2519,7 @@ describe("App edit rename with a failed list reload (#1914)", () => { ? () => vi.mocked(useServers).mockImplementation(previous) : undefined; updateServerSpy.mockClear(); + addServerSpy.mockClear(); updateServerSettingsSpy.mockClear(); vi.mocked(useServers).mockReturnValue(serversResult(["A"])); }); @@ -2530,7 +2538,7 @@ describe("App edit rename with a failed list reload (#1914)", () => { "The server was saved, but the server list could not be reloaded: disk full", ), ); - renderWithMantine(); + const { rerender } = renderWithMantine(); await user.click(screen.getByText("connect")); await waitFor(() => expect(clientInstances).toHaveLength(1)); @@ -2552,12 +2560,19 @@ describe("App edit rename with a failed list reload (#1914)", () => { 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. + // Now let the row land, as the SSE refresh the write itself triggers + // would. The modal's target ("A") is gone, so it closes rather than + // blanking its own form — `initialId`/`initialConfig` would go undefined + // and ServerConfigModal's reset would wipe the open form and the error + // it is showing (#1914 r3). vi.mocked(useServers).mockReturnValue(serversResult(["A-renamed"])); - await user.click(screen.getByRole("button", { name: "Cancel" })); - await user.click(screen.getByText("paginated-on")); + rerender(); + await waitFor(() => + expect(screen.queryByLabelText(/Server ID/)).not.toBeInTheDocument(), + ); + // With the modal gone, read the active id back off the toggle. + await user.click(screen.getByText("paginated-on")); await waitFor(() => expect(updateServerSettingsSpy).toHaveBeenCalledWith( "A-renamed", @@ -2565,6 +2580,38 @@ describe("App edit rename with a failed list reload (#1914)", () => { ), ); }); + + it("keeps a successful add in the highlight batch when the reload failed (#1914)", async () => { + // `addServerHighlighted` marked the new id only after `addServer` + // resolved. The row is on disk either way, so on a reload failure it has + // to be marked from the catch — the next successful refresh is what + // renders it, and it would otherwise arrive unhighlighted. + const user = userEvent.setup(); + addServerSpy.mockRejectedValueOnce( + new ServerListReloadError( + "The server was added, but the server list could not be reloaded: disk full", + ), + ); + renderWithMantine(); + expect(screen.getByTestId("highlighted-servers")).toHaveTextContent("none"); + + await user.click(screen.getByText("add-server")); + await user.type(await screen.findByLabelText(/Server ID/), "brand-new"); + await user.type(screen.getByLabelText(/^Command/), "node"); + await user.click(screen.getByRole("button", { name: "Add" })); + + await waitFor(() => + expect(addServerSpy).toHaveBeenCalledWith("brand-new", expect.anything()), + ); + expect( + await screen.findByText(/could not be reloaded: disk full/), + ).toBeInTheDocument(); + await waitFor(() => + expect(screen.getByTestId("highlighted-servers")).toHaveTextContent( + "brand-new", + ), + ); + }); }); // A background command runs through `runCommandInBackground`, which owns the diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 169fd9c3..a99f212e 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -113,6 +113,7 @@ import { usePagedTools } from "@inspector/core/react/usePagedTools.js"; import { usePagedPrompts } from "@inspector/core/react/usePagedPrompts.js"; import { usePagedResources } from "@inspector/core/react/usePagedResources.js"; import { usePaginatedList } from "./hooks/usePaginatedList"; +import { useValueChange } from "./hooks/useValueChange"; import type { ListPaginationControlsProps } from "./components/elements/ListPaginationControls/ListPaginationControls"; import { useManagedResourceTemplates } from "@inspector/core/react/useManagedResourceTemplates.js"; import { useManagedRequestorTasks } from "@inspector/core/react/useManagedRequestorTasks.js"; @@ -4140,6 +4141,30 @@ function App() { return servers.find((s) => s.id === configModal.targetId); }, [configModal, servers]); + // An edit/clone modal whose target has left the list can no longer render a + // coherent form: `initialId`/`initialConfig` go undefined, and + // ServerConfigModal's own `useValueChange` reset then blanks the open form + // *and* clears the error it is showing. + // + // Only reachable since #1914 made the modal stay open on a failed post-write + // reload: rename A → B, the PUT lands, the reload fails, and the modal sits + // there targeting A. The write itself is what triggers the backend's change + // broadcast, so the SSE-driven background refresh that lands B (and drops A) + // is the *likely* next event, not a remote one. Closing is right — the edit + // is on disk and the list now agrees — and it covers the ordinary case too, + // where an external mcp.json edit removes the server being edited. + // + // Adjusted during render rather than in an effect (see `useValueChange`), so + // no frame ever paints the blanked form. + useValueChange( + configModal?.mode === "add" || configModal?.targetId === undefined + ? true + : servers.some((s) => s.id === configModal.targetId), + (targetPresent) => { + if (!targetPresent) setConfigModal(null); + }, + ); + const settingsModalTarget = useMemo(() => { if (!settingsModalTargetId) return undefined; return servers.find((s) => s.id === settingsModalTargetId); @@ -4188,7 +4213,14 @@ function App() { // didn't (especially painful for the OAuth client secret). onError: (id, err) => { notifications.show({ - title: `Failed to save settings for "${id}"`, + // A `ServerListReloadError` means the PUT landed and only reading the + // list back failed, so "Failed to save" would be false — and this + // toast is the user's only signal here, since the modal has usually + // closed by the time a flush rejects (#1914 r3). + title: + err instanceof ServerListReloadError + ? `Saved settings for "${id}", but the server list did not reload` + : `Failed to save settings for "${id}"`, message: err instanceof Error ? err.message : String(err), color: "red", }); diff --git a/clients/web/src/hooks/useImportClientConfig.test.tsx b/clients/web/src/hooks/useImportClientConfig.test.tsx index 9900f283..def42e7e 100644 --- a/clients/web/src/hooks/useImportClientConfig.test.tsx +++ b/clients/web/src/hooks/useImportClientConfig.test.tsx @@ -6,6 +6,7 @@ import { } from "./useImportClientConfig"; import type { ImportSourceResult } from "@inspector/core/mcp/import/index.js"; import type { MCPConfig } from "@inspector/core/mcp/types.js"; +import { ServerListReloadError } from "@inspector/core/react/useServers.js"; function config(...ids: string[]): MCPConfig { const mcpServers: MCPConfig["mcpServers"] = {}; @@ -207,6 +208,31 @@ describe("useImportClientConfig", () => { expect(outcome.detail).toBe("add failed"); }); + it("reports a reload failure as imported-with-a-warning, not failed (#1914)", async () => { + // A `ServerListReloadError` rejects *after* the write landed, so the entry + // really was added. A red "Failed" badge would contradict the error's own + // message and invite a retry that then collides with the row it claims + // wasn't created. The reason still shows, as `detail`. + const onAddServer = vi.fn(async () => { + throw new ServerListReloadError( + "The server was added, but the server list could not be reloaded: disk full", + ); + }); + const { result } = setup({ + onFetchSource: vi.fn(async () => foundSource(config("alpha"))), + onAddServer, + }); + await act(async () => { + await result.current.pickSource("claude"); + }); + await act(async () => { + await result.current.runImport(); + }); + const outcome = result.current.outcomes[0]; + expect(outcome.status).toBe("added"); + expect(outcome.detail).toMatch(/could not be reloaded: disk full/); + }); + it("resolves conflicts by overwrite, skip, and rename", async () => { const onAddServer = vi.fn(async () => {}); const onUpdateServer = vi.fn(async () => {}); diff --git a/clients/web/src/hooks/useImportClientConfig.ts b/clients/web/src/hooks/useImportClientConfig.ts index 61a01478..c5e439f8 100644 --- a/clients/web/src/hooks/useImportClientConfig.ts +++ b/clients/web/src/hooks/useImportClientConfig.ts @@ -8,6 +8,7 @@ import { type ImportSourceResult, } from "@inspector/core/mcp/import/index.js"; import { validateStoreId } from "@inspector/core/storage/store-id.js"; +import { ServerListReloadError } from "@inspector/core/react/useServers.js"; import type { MCPConfig, MCPServerConfig } from "@inspector/core/mcp/types.js"; export type ImportPhase = "select" | "loading" | "review" | "summary"; @@ -198,11 +199,17 @@ export function useImportClientConfig({ await write(); return { id, status: outcome }; } catch (err) { - return { - id, - status: "failed", - detail: err instanceof Error ? err.message : String(err), - }; + const detail = err instanceof Error ? err.message : String(err); + // A `ServerListReloadError` means the write landed and only reading the + // list back failed, so this entry really was imported. Reporting it as + // `failed` would contradict the error's own message and invite a retry + // that then collides with the row it says wasn't created (#1914 r3). + // The status is the truth about the write; `detail` renders in red + // beneath it either way, so the reload failure is still visible. + if (err instanceof ServerListReloadError) { + return { id, status: outcome, detail }; + } + return { id, status: "failed", detail }; } }