modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 2

fix(web): apply settled settings to the live client, and reconcile on modal failure

Browse source 
Review round 7 on #2091, both findings.

Every settings continuation pushed through the `inspectorClient` it
closed over, and `onToggleConnection` rebuilds the client on each
(re)connect — so a write outliving a reconnect passed the server-id
guard and then wrote to a destroyed instance, while the replacement,
built from the stale list entry, kept the value the write replaced. The
live client is now mirrored in a ref alongside the active server id, and
all three continuations apply through it.

The modal save's failure path also stopped at taking itself out of
flight. A write that landed while it was pending had been told it was
not settled and applied nothing, so with the save failing there was
nothing left to apply the value that is actually on disk. It now
resolves and re-applies the baseline, the same as the toggle's failure
path.

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:
cliffhall 2026-08-23 13:16:51 -04:00
commit 38d2fd279d
2 changed files with 118 additions and 4 deletions
Download patch file Download diff file Expand all files Collapse all files

89
clients/web/src/App.test.tsx
View file

@ -2624,6 +2624,95 @@ describe("App paginated list pagination toggle (#1721)", () => {
expect(lastPush?.paginatedLists).toBe(true);
});
it("applies a settled write to the current client, not the one it started on (#2089)", async () => {
// A write can outlive a disconnect/reconnect to the *same* server: the id
// guard still passes, but the client captured when the write was issued has
// been destroyed and replaced by one built from the stale list entry. The
// settled value has to reach the live instance.
const user = userEvent.setup();
let landWrite: (() => void) | undefined;
const write = new Promise<void>((resolve) => {
landWrite = resolve;
});
updateServerSettingsSpy.mockImplementationOnce(() => write);
renderWithMantine(<App />);
await user.click(screen.getByText("connect"));
await waitFor(() => expect(clientInstances).toHaveLength(1));
await user.click(screen.getByText("paginated-on"));
// Reconnect to the same server while that write is in flight. The transport
// drops, then the user reconnects — and `onToggleConnection` always
// rebuilds the client so the latest saved settings are picked up.
act(() => {
clientInstances[0].dispatchEvent(new Event("disconnect"));
});
await user.click(screen.getByText("connect"));
await waitFor(() => expect(clientInstances).toHaveLength(2));
const replacement = clientInstances[1] as EventTarget & {
setServerSettings: ReturnType<typeof vi.fn>;
};
replacement.setServerSettings.mockClear();
await act(async () => {
landWrite?.();
await write;
});
await waitFor(() =>
expect(replacement.setServerSettings).toHaveBeenCalledWith(
expect.objectContaining({ paginatedLists: true }),
),
);
});
it("reconciles after a modal save fails onto a write that landed first (#2089)", async () => {
// Reverse of the mixed-writer case above: the toggle lands while the modal
// save is still pending, so it is told it is not settled and applies
// nothing. The modal save then fails, which makes the toggle's value the
// account of disk — and nothing else is left to apply it.
const user = userEvent.setup();
let failModal: ((err: Error) => void) | undefined;
const modalWrite = new Promise<void>((_resolve, reject) => {
failModal = (err) => {
reject(err);
};
});
renderWithMantine(<App />);
await user.click(screen.getByText("connect"));
await waitFor(() => expect(clientInstances).toHaveLength(1));
const draftOptions = vi.mocked(useSettingsDraft).mock.calls.at(-1)?.[0];
if (!draftOptions) throw new Error("useSettingsDraft was never called");
updateServerSettingsSpy.mockImplementationOnce(() => modalWrite);
const persisted = draftOptions
.onPersist("A", { ...settingsWithRoots([]), paginatedLists: false })
.catch(() => undefined);
// The toggle lands `true` while the modal save is still in flight.
await user.click(screen.getByText("paginated-on"));
await waitFor(() =>
expect(updateServerSettingsSpy).toHaveBeenCalledTimes(2),
);
const client = clientInstances[0] as EventTarget & {
setServerSettings: ReturnType<typeof vi.fn>;
};
client.setServerSettings.mockClear();
await act(async () => {
failModal?.(new Error("disk full"));
await persisted;
});
// The failure is what reconciles: a push carrying the landed value arrives
// only after it, since the landed write itself was not settled at the time.
await waitFor(() =>
expect(client.setServerSettings).toHaveBeenCalledWith(
expect.objectContaining({ paginatedLists: true }),
),
);
});
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

33
clients/web/src/App.tsx
View file

@ -1732,11 +1732,18 @@ function App() {
const activeServerNameRef = useRef<string | undefined>(undefined);
const activeServerIdRef = useRef<string | undefined>(activeServerId);
const serversRef = useRef(servers);
// The live client, mirrored for the same reason: a settings write that
// outlives a disconnect/reconnect to the *same* server would otherwise push
// its value into the instance captured when it was issued — which has since
// been destroyed — while the replacement client, built from the stale list
// entry, keeps the value the write replaced (#2089).
const inspectorClientRef = useRef<InspectorClient | null>(inspectorClient);
useEffect(() => {
if (activeServer) activeServerNameRef.current = activeServer.name;
activeServerIdRef.current = activeServerId;
serversRef.current = servers;
}, [activeServer, activeServerId, servers]);
inspectorClientRef.current = inspectorClient;
}, [activeServer, activeServerId, servers, inspectorClient]);
// Last connection-level error message, surfaced as `data-error-message` on
// the InspectorView header so an automated driver can read *why* a connect
@ -3795,10 +3802,13 @@ function App() {
// toggle that failed *first* rolled the UI and the live client back
// to a baseline this write has since replaced, and if the list read
// behind this write failed too, nothing else would ever correct them.
// Through the *current* client, not this continuation's closure: a
// reconnect to the same server passes the id check while the captured
// instance is already destroyed.
const settled = write.landed(next);
if (settled && activeServerIdRef.current === activeServerId) {
setPaginatedListsOverride(next.paginatedLists ?? false);
inspectorClient?.setServerSettings(next);
inspectorClientRef.current?.setServerSettings(next);
}
})
.catch((err: unknown) => {
@ -3822,11 +3832,14 @@ function App() {
// override is a single app-wide boolean and the live client belongs to
// whichever server is connected now, so a rejection arriving after the
// user switched would render this server's value on another one.
// The client is taken from the ref for the same reason the success
// path does: a reconnect to the same server passes the id check while
// this continuation's captured instance is already destroyed.
const baseline =
lastPersistedSettings.resolve(activeServerId) ?? EMPTY_SETTINGS;
if (activeServerIdRef.current === activeServerId) {
setPaginatedListsOverride(baseline.paginatedLists ?? false);
inspectorClient?.setServerSettings(baseline);
inspectorClientRef.current?.setServerSettings(baseline);
}
notifications.show({
title: "Failed to save pagination setting",
@ -4220,6 +4233,15 @@ function App() {
// `onError` — an earlier write still running settles the state once it
// lands, and cannot know that while this one looks pending (#2089).
write.failed();
// A write that landed while this one was pending was told it was not
// settled and so applied nothing; this save never reached disk, so that
// write is the account of it and nothing else will re-apply it. Same
// reconciliation the toggle's own failure path does.
const baseline = lastPersistedSettings.resolve(id);
if (baseline && activeServerIdRef.current === id) {
setPaginatedListsOverride(baseline.paginatedLists ?? false);
inspectorClientRef.current?.setServerSettings(baseline);
}
throw err;
}
// Recorded for the same reason the pagination toggle records its own
@ -4236,7 +4258,10 @@ function App() {
const settled = write.landed(value);
if (settled && activeServerIdRef.current === id) {
setPaginatedListsOverride(value.paginatedLists ?? false);
inspectorClient?.setServerSettings(value);
// The current client, not the one captured when this save began — a
// reconnect to the same server would otherwise be written to a
// destroyed instance while the replacement keeps the stale value.
inspectorClientRef.current?.setServerSettings(value);
}
},
// Surface failures via toast — the modal usually closes