fix(web): configure the client from the tracker at connect time
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4 Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
db84823262
commit
197e7aa401
2 changed files with 53 additions and 2 deletions
|
|
@ -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<typeof vi.fn>;
|
||||
};
|
||||
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(<App />);
|
||||
|
||||
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
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
],
|
||||
);
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue