From 269ff5fefcfcfa9a13bbe823df9bdf745d096ded Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 23 Aug 2026 09:58:56 -0400 Subject: [PATCH] fix(web): follow a rename whose list reload failed, and drop a double cast MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review r2. `onConfigSubmit` moved `activeServerId` to the renamed id only after `updateServer` resolved. Once a failed list reload rejects, the rename is still on disk — so the active id stayed on the old one and the next successful refresh left it pointing at a row that no longer exists. Apply the rename from the catch when the rejection is a `ServerListReloadError`, then rethrow so the modal still shows the error. `addServerHighlighted` gets the same treatment: the row is on disk, so it belongs in the highlight batch the next successful refresh renders. `activeServerId` is not rendered anywhere, so the test reads it back through the one handler that hands it to a spy — the pagination toggle's `updateServerSettings(activeServerId, …)` — after letting the renamed row land as a later refresh would. It fails without the catch-side call. The new describe installs its own `useServers` return value and restores the previous implementation afterwards, since earlier describes' overrides outlive them (which is why the first draft's spy was never called). Also replaced the `as unknown as` in the new test with a single `EventTarget & {…}` intersection, per the TypeScript rules. The other 19 in this file are the same pattern and are left for a separate sweep. The third comment — that the pagination toggle's rollback baseline goes stale after a failed list read — is a real defect but pre-existing and unchanged by this PR; the end state is identical before and after it. Filed as #2089 rather than widening this fix. 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 | 103 ++++++++++++++++++++++++++++++++++- clients/web/src/App.tsx | 36 ++++++++++-- 2 files changed, 132 insertions(+), 7 deletions(-) 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