From 197e7aa4013a1a9599728b35a1c68d3169561ac5 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 23 Aug 2026 14:12:50 -0400 Subject: [PATCH] fix(web): configure the client from the tracker at connect time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 10 on #2091. Connecting is where the whole connection is configured, and it read `server.settings` straight off the list entry — so every connect undid what the tracker exists to preserve. A save that landed while list reads were failing, including one made from the settings modal for a server that was not connected at the time, was silently reverted by the next connect: headers, roots, timeouts, the Network log size, all rebuilt from the frozen entry. `setupClientForServer` now resolves through `lastPersistedSettings`, which covers every construction path (connect, reconnect, OAuth resume) since they all pass through it. The round-9 test also claimed to cover the log size and did not: it asserted the settings object and the roots only, so a regression that stopped restoring `maxFetchRequests` would have passed. It now asserts the tracked fetch-log instance on both sides of the rejection. 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 | 43 +++++++++++++++++++++++++++++++++++- clients/web/src/App.tsx | 12 +++++++++- 2 files changed, 53 insertions(+), 2 deletions(-) diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index 631cb654..2561c308 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -1785,12 +1785,16 @@ describe("App roots live-apply on settings-dialog close", () => { }); // Close applies the draft live — the state the flush leaves behind. + const fetchLog = fetchLogInstances.at(-1) as EventTarget & { + setMaxFetchRequests: ReturnType; + }; await closeModal(user); await waitFor(() => expect(client.setRoots).toHaveBeenCalledWith([ { uri: "file:///rejected" }, ]), ); + expect(fetchLog.setMaxFetchRequests).toHaveBeenLastCalledWith(10); client.getRoots.mockReturnValue([{ uri: "file:///rejected" }]); client.setRoots.mockClear(); @@ -1800,11 +1804,15 @@ describe("App roots live-apply on settings-dialog close", () => { await draftOptions.onPersist("A", rejectedDraft).catch(() => undefined); }); - // The whole live surface goes back to what landed, roots included. + // The whole live surface goes back to what landed — settings, roots, and + // the log size, each of which close had moved to the rejected draft. await waitFor(() => expect(client.setRoots).toHaveBeenCalledWith([])); expect(client.setServerSettings).toHaveBeenLastCalledWith( persistedSettings, ); + expect(fetchLog.setMaxFetchRequests).toHaveBeenLastCalledWith( + persistedSettings.maxFetchRequests, + ); }); }); @@ -2827,6 +2835,39 @@ describe("App paginated list pagination toggle (#1721)", () => { ); }); + it("builds the next client from the last write, not the stale entry (#2089)", async () => { + // Connecting is where the whole connection is configured, and it read the + // `servers` entry directly — so a save that landed while list reads were + // failing (including one made from the modal for a server that was not + // connected at the time) was undone by the next connect, which rebuilt the + // client from the frozen entry. + const user = userEvent.setup(); + renderWithMantine(); + + const draftOptions = vi.mocked(useSettingsDraft).mock.calls.at(-1)?.[0]; + if (!draftOptions) throw new Error("useSettingsDraft was never called"); + const saved: InspectorServerSettings = { + ...settingsWithRoots([{ uri: "file:///saved" }]), + maxFetchRequests: 42, + }; + await act(async () => { + await draftOptions.onPersist("A", saved); + }); + + // `servers` still reports the entry with no settings at all — what a failed + // reload leaves. The client must be built from the write that landed. + await user.click(screen.getByText("connect")); + await waitFor(() => expect(clientInstances).toHaveLength(1)); + const clientOptions = vi + .mocked(McpIndex.InspectorClient) + .mock.calls.at(-1)?.[1] as { roots?: { uri: string }[] } | undefined; + expect(clientOptions?.roots).toEqual([{ uri: "file:///saved" }]); + const fetchLogOptions = vi + .mocked(FetchLogModule.FetchRequestLogState) + .mock.calls.at(-1)?.[1] as { maxFetchRequests?: number } | undefined; + expect(fetchLogOptions?.maxFetchRequests).toBe(42); + }); + it("lets a successful list read supersede a concrete rollback override (#2089)", async () => { // A rollback sets the override to a value rather than clearing it, so the // effect that drops it cannot key on the persisted boolean alone: if a diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index f32ef321..729eb4d2 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -2522,7 +2522,16 @@ function App() { ); // The settings node persisted in mcp.json for this server — distinct // from the InspectorClient options we're about to derive from it. - const savedSettings = server.settings; + // + // Through the tracker, not `server.settings` directly: the entry only + // advances on a successful list read, so a write that landed while reads + // were failing — including one made from the settings modal for a server + // that was not connected at the time — would otherwise be undone at the + // next connect, which builds the client from that frozen entry. This is + // the one place the whole connection is configured, so every construction + // path (connect, reconnect, OAuth resume) goes through it (#2089). + const savedSettings = + lastPersistedSettings.resolve(server.id) ?? server.settings; const activeIdp = getActiveEnterpriseManagedAuthIdp(clientConfig); const activeCimdUrl = getActiveCimdClientMetadataUrl(clientConfig); // Flatten the persisted settings into the InspectorClient options shape. @@ -2694,6 +2703,7 @@ function App() { onBeforeOAuthRedirect, clientConfig, newAppElicitationSession, + lastPersistedSettings, ], );