diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index 3d95bf9d..fabee0b9 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -220,6 +220,12 @@ const SERVER_A = { const { updateServerSettingsSpy } = vi.hoisted(() => ({ updateServerSettingsSpy: vi.fn(() => Promise.resolve()), })); + +// 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(() => ({ + updateServerSpy: vi.fn(() => Promise.resolve()), +})); // Stable spy for the tools list-changed acknowledgement, so a test can assert // the paginated Refresh clears the indicator (#1721). const { clearToolsListChangedSpy } = vi.hoisted(() => ({ @@ -235,7 +241,7 @@ vi.mock("@inspector/core/react/useServers.js", async (importOriginal) => ({ useServers: vi.fn(() => ({ servers: [SERVER_A], addServer: vi.fn(), - updateServer: vi.fn(), + updateServer: updateServerSpy, updateServerSettings: updateServerSettingsSpy, removeServer: vi.fn(), })), @@ -418,6 +424,7 @@ vi.mock("./components/views/InspectorView/InspectorView", () => ({ onClearCompletedTasks: () => void; onRefreshTasks: () => void; onServerSettings: (id: string) => void; + onServerEdit: (id: string) => void; onClearProtocol: () => void; onReplayProtocol: (id: string) => void; onTogglePinProtocol: (id: string) => void; @@ -543,6 +550,9 @@ 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. */} + {Array.from(props.pinnedProtocolIds ?? []).join(",")} @@ -2458,7 +2468,10 @@ describe("App paginated list pagination toggle (#1721)", () => { await Promise.resolve(); }); expect(screen.getByTestId("tools-paginated")).toHaveTextContent("true"); - const client = clientInstances[0] as unknown as { + // Single intersection assertion rather than the `as unknown as` this file + // uses elsewhere: the fake client really is an EventTarget, and widening + // it keeps that relationship instead of erasing it (#1914 r2). + const client = clientInstances[0] as EventTarget & { setServerSettings: ReturnType; }; const lastPush = client.setServerSettings.mock.calls.at(-1)?.[0] as { @@ -2468,6 +2481,92 @@ 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)", () => { + // 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. + let restoreUseServers: (() => void) | undefined; + + const serversResult = (ids: string[]): ReturnType => ({ + servers: ids.map((id) => ({ ...SERVER_A, id }) as ServerEntry), + loading: false, + error: undefined, + refresh: vi.fn().mockResolvedValue(undefined), + addServer: vi.fn().mockResolvedValue(undefined), + updateServer: updateServerSpy, + updateServerSettings: updateServerSettingsSpy, + removeServer: vi.fn().mockResolvedValue(undefined), + reorderServers: vi.fn().mockResolvedValue(undefined), + importSource: vi.fn().mockResolvedValue({ servers: {} }), + }); + + beforeEach(() => { + clientInstances.length = 0; + const previous = vi.mocked(useServers).getMockImplementation(); + restoreUseServers = previous + ? () => vi.mocked(useServers).mockImplementation(previous) + : undefined; + updateServerSpy.mockClear(); + updateServerSettingsSpy.mockClear(); + vi.mocked(useServers).mockReturnValue(serversResult(["A"])); + }); + + afterEach(() => { + restoreUseServers?.(); + }); + + it("follows the rename so the active id isn't orphaned (#1914)", async () => { + // `activeServerId` isn't rendered anywhere, so it's read back through the + // one handler that passes it straight to a spy: the pagination toggle + // calls `updateServerSettings(activeServerId, …)`. + const user = userEvent.setup(); + updateServerSpy.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("edit-server")); + const idField = await screen.findByLabelText(/Server ID/); + await user.clear(idField); + await user.type(idField, "A-renamed"); + await user.click(screen.getByRole("button", { name: "Save" })); + + await waitFor(() => + expect(updateServerSpy).toHaveBeenCalledWith( + "A", + "A-renamed", + expect.anything(), + ), + ); + // The rejection still reaches the modal, which stays open showing it. + expect( + 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. + vi.mocked(useServers).mockReturnValue(serversResult(["A-renamed"])); + await user.click(screen.getByRole("button", { name: "Cancel" })); + await user.click(screen.getByText("paginated-on")); + + await waitFor(() => + expect(updateServerSettingsSpy).toHaveBeenCalledWith( + "A-renamed", + expect.objectContaining({ paginatedLists: true }), + ), + ); + }); +}); + // A background command runs through `runCommandInBackground`, which owns the // rejection. Before #2049 the 17 `void runWithCommandAuthRecovery(...)` sites // had no handler, so any non-auth failure of the wrapped operation became an diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index a79098ca..169fd9c3 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -4075,8 +4075,20 @@ function App() { // opens (see the menu handlers). const addServerHighlighted = useCallback( async (id: string, config: MCPServerConfig) => { - await addServer(id, config); - setHighlightedServerIds((ids) => (ids.includes(id) ? ids : [...ids, id])); + const markAdded = () => + setHighlightedServerIds((ids) => + ids.includes(id) ? ids : [...ids, id], + ); + try { + await addServer(id, config); + } catch (err) { + // A failed list reload still means the row is on disk, so it belongs + // in the highlight batch — the next successful refresh is what + // renders it, and it would otherwise arrive unmarked (#1914 r2). + if (err instanceof ServerListReloadError) markAdded(); + throw err; + } + markAdded(); }, [addServer], ); @@ -4086,10 +4098,24 @@ function App() { async (id: string, config: MCPServerConfig) => { if (configModal?.mode === "edit" && configModal.targetId) { const originalId = configModal.targetId; - await updateServer(originalId, id, config); - if (originalId === activeServerId && id !== originalId) { - setActiveServerId(id); + const followRename = () => { + if (originalId === activeServerId && id !== originalId) { + setActiveServerId(id); + } + }; + try { + await updateServer(originalId, id, config); + } catch (err) { + // A `ServerListReloadError` means the PUT landed and only reading + // the list back failed — the rename IS on disk. The active + // selection has to follow it anyway, or the next successful + // refresh leaves `activeServerId` pointing at an id that no longer + // exists (#1914 r2). Still rethrow, so the modal shows the reload + // error rather than closing as if nothing happened. + if (err instanceof ServerListReloadError) followRename(); + throw err; } + followRename(); return; } // add or clone