diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index fdf01413..b4e83147 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -2423,6 +2423,48 @@ describe("App paginated list pagination toggle (#1721)", () => { }; expect(lastPush?.paginatedLists).toBeFalsy(); }); + + it("rolls back to the last write that landed, not to a stale list entry (#2089)", async () => { + // Two toggles. The first PUT lands but the list reload behind it fails, so + // the `servers` entry keeps describing disk as it was *before* it — which + // this suite models exactly, because `useServers` is mocked to return the + // same entry forever. The second PUT fails outright and rolls back. + // + // Reverting to the `servers` entry there lands on `false`, the value from + // before the first toggle, contradicting what is on disk. The baseline has + // to come from the write that landed instead. + // + // One failed toggle cannot distinguish the two baselines — they only differ + // once a write has landed that the list never caught up with — so the test + // needs the full sequence. + const user = userEvent.setup(); + renderWithMantine(); + await user.click(screen.getByText("connect")); + await waitFor(() => expect(clientInstances).toHaveLength(1)); + + await user.click(screen.getByText("paginated-on")); + await waitFor(() => + expect(updateServerSettingsSpy).toHaveBeenCalledWith( + "A", + expect.objectContaining({ paginatedLists: true }), + ), + ); + + updateServerSettingsSpy.mockRejectedValueOnce(new Error("disk full")); + await user.click(screen.getByText("paginated-off")); + + // Back to the value the first toggle put on disk, not to the pre-toggle one. + await waitFor(() => + expect(screen.getByTestId("tools-paginated")).toHaveTextContent("true"), + ); + const client = clientInstances[0] as EventTarget & { + setServerSettings: ReturnType; + }; + const lastPush = client.setServerSettings.mock.calls.at(-1)?.[0] as { + paginatedLists?: boolean; + }; + expect(lastPush?.paginatedLists).toBe(true); + }); }); // A background command runs through `runCommandInBackground`, which owns the diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 67f7d0ff..53d64e10 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -110,6 +110,7 @@ import { usePagedTools } from "@inspector/core/react/usePagedTools.js"; import { usePagedPrompts } from "@inspector/core/react/usePagedPrompts.js"; import { usePagedResources } from "@inspector/core/react/usePagedResources.js"; import { usePaginatedList } from "./hooks/usePaginatedList"; +import { useLastPersistedSettings } from "./hooks/useLastPersistedSettings"; import type { ListPaginationControlsProps } from "./components/elements/ListPaginationControls/ListPaginationControls"; import { useManagedResourceTemplates } from "@inspector/core/react/useManagedResourceTemplates.js"; import { useManagedRequestorTasks } from "@inspector/core/react/useManagedRequestorTasks.js"; @@ -1159,6 +1160,10 @@ function App() { error: pagedResourcesLoadError, loadPage: loadResourcesPage, } = usePagedResources(inspectorClient, pagedResourcesState); + // What every settings write that landed on disk actually wrote, so a later + // failed write can be rolled back to that rather than to a `servers` entry + // frozen at the last successful list read (#2089). + const lastPersistedSettings = useLastPersistedSettings(); // The active server's persisted paginated setting drives the display mode. // The sidebar toggle edits it (optimistically, below) and persists it. const persistedPaginatedLists = @@ -3730,7 +3735,15 @@ function App() { setPaginatedListsOverride(value); const current = servers.find((s) => s.id === activeServerId); if (!current || activeServerId === undefined) return; - const prevSettings = current.settings ?? EMPTY_SETTINGS; + // Not `current.settings` directly: that entry only advances on a + // successful list read, so once one has failed it describes disk as it + // was *before* the writes made since. Roll back to the last write known + // to have landed while that is the fresher account (#2089). + const prevSettings = lastPersistedSettings.resolve( + activeServerId, + current, + current.settings ?? EMPTY_SETTINGS, + ); const next: InspectorServerSettings = { ...prevSettings, paginatedLists: value, @@ -3760,23 +3773,35 @@ function App() { void refreshingPersist(updateServerSettings, refreshInitialConfig)( activeServerId, next, - ).catch((err: unknown) => { - // Persist failed: revert the optimistic override (the effect only - // clears it when the persisted value changes, which won't happen here) - // and roll the live client setting back, so the UI and client reflect - // the value that's actually on disk rather than the failed edit (#1721). - setPaginatedListsOverride(null); - inspectorClient?.setServerSettings(prevSettings); - notifications.show({ - title: "Failed to save pagination setting", - message: err instanceof Error ? err.message : String(err), - color: "red", + ) + .then(() => { + // This value is on disk now. Remember it as the rollback baseline for + // whatever is written next, since the `servers` entry it was derived + // from will keep describing the old value if the reload behind this + // write — or any later one — fails (#2089). + lastPersistedSettings.record(activeServerId, next, current); + }) + .catch((err: unknown) => { + // Persist failed: revert the optimistic override and roll the live + // client setting back, so the UI and client reflect the value that's + // actually on disk rather than the failed edit (#1721). The override + // is set to the baseline rather than cleared: clearing it falls back + // to `persistedPaginatedLists`, read from the same `servers` entry + // that may be stale, which would show the same wrong value from the + // other direction (#2089). + setPaginatedListsOverride(prevSettings.paginatedLists ?? false); + inspectorClient?.setServerSettings(prevSettings); + notifications.show({ + title: "Failed to save pagination setting", + message: err instanceof Error ? err.message : String(err), + color: "red", + }); }); - }); }, [ servers, activeServerId, + lastPersistedSettings, inspectorClient, updateServerSettings, refreshInitialConfig, @@ -4143,7 +4168,22 @@ function App() { // unit rather than an inline `await …; refresh()` because this file is // outside the coverage gate, so wiring written here is tested by nothing // (#1950 review r17). - onPersist: refreshingPersist(updateServerSettings, refreshInitialConfig), + onPersist: async (id: string, value: InspectorServerSettings) => { + const entry = servers.find((s) => s.id === id); + await refreshingPersist(updateServerSettings, refreshInitialConfig)( + id, + value, + ); + // Recorded for the same reason the pagination toggle records its own + // write (#2089): this is what disk holds now, and the `servers` entry it + // was edited from will keep describing the previous value for as long as + // list reads keep failing. Both settings writers feed the one record, so + // a rollback in either takes the most recent write, not just the most + // recent write *of its own kind*. No entry means the modal was opened + // against a server the list doesn't have, and there is nothing for a + // later read to supersede. + if (entry) lastPersistedSettings.record(id, value, entry); + }, // Surface failures via toast — the modal usually closes // immediately on user dismiss, so a silent fail-on-flush would // leave the user thinking their last edits saved when they diff --git a/clients/web/src/hooks/useLastPersistedSettings.test.tsx b/clients/web/src/hooks/useLastPersistedSettings.test.tsx new file mode 100644 index 00000000..f9e3251a --- /dev/null +++ b/clients/web/src/hooks/useLastPersistedSettings.test.tsx @@ -0,0 +1,96 @@ +import { describe, it, expect } from "vitest"; +import type { + InspectorServerSettings, + ServerEntry, +} from "@inspector/core/mcp/types.js"; +import { renderWithMantine } from "../test/renderWithMantine"; +import { + useLastPersistedSettings, + type LastPersistedSettings, +} from "./useLastPersistedSettings"; + +const settings = ( + overrides: Partial = {}, +): InspectorServerSettings => ({ + headers: [], + env: [], + metadata: [], + connectionTimeout: 0, + requestTimeout: 0, + taskTtl: 0, + autoRefreshOnListChanged: false, + paginatedLists: false, + maxFetchRequests: 0, + roots: [], + ...overrides, +}); + +/** + * One `servers` entry, as a fresh object each call — the point of most of these + * tests is object identity, so a shared fixture would defeat them. + */ +const entry = (settingsNode?: InspectorServerSettings): ServerEntry => ({ + id: "A", + name: "PlotRocket", + config: { type: "stdio", command: "node" }, + connection: { status: "disconnected" }, + settings: settingsNode, +}); + +/** + * Hands the hook's API back to the test. The hook holds a ref, so the value has + * to be read from inside a rendered component. + */ +function harness(): LastPersistedSettings { + let api: LastPersistedSettings | undefined; + function Probe() { + api = useLastPersistedSettings(); + return null; + } + renderWithMantine(); + if (!api) throw new Error("hook did not render"); + return api; +} + +describe("useLastPersistedSettings", () => { + it("falls back to the caller's value when no write has been recorded", () => { + const { resolve } = harness(); + const fallback = settings({ paginatedLists: true }); + expect(resolve("A", entry(), fallback)).toBe(fallback); + }); + + it("returns the recorded write while the entry has not been re-read", () => { + const { record, resolve } = harness(); + const seen = entry(); + const written = settings({ paginatedLists: true }); + record("A", written, seen); + expect(resolve("A", seen, settings())).toBe(written); + }); + + it("ignores a record made for a different server", () => { + const { record, resolve } = harness(); + const seen = entry(); + record("A", settings({ paginatedLists: true }), seen); + const fallback = settings(); + expect(resolve("B", seen, fallback)).toBe(fallback); + }); + + it("stops trusting the record once a fresh list read replaces the entry", () => { + const { record, resolve } = harness(); + record("A", settings({ paginatedLists: true }), entry()); + // A successful `GET /api/servers` rebuilds the list, so the entry is a new + // object even when its values are unchanged. That is the signal the list + // has caught up, and it is authoritative again from here. + const fresh = settings(); + expect(resolve("A", entry(fresh), fresh)).toBe(fresh); + }); + + it("keeps only the most recent write", () => { + const { record, resolve } = harness(); + const seen = entry(); + record("A", settings({ paginatedLists: true }), seen); + const later = settings({ autoRefreshOnListChanged: true }); + record("A", later, seen); + expect(resolve("A", seen, settings())).toBe(later); + }); +}); diff --git a/clients/web/src/hooks/useLastPersistedSettings.ts b/clients/web/src/hooks/useLastPersistedSettings.ts new file mode 100644 index 00000000..f8ca03c2 --- /dev/null +++ b/clients/web/src/hooks/useLastPersistedSettings.ts @@ -0,0 +1,96 @@ +import { useCallback, useRef } from "react"; +import type { + InspectorServerSettings, + ServerEntry, +} from "@inspector/core/mcp/types.js"; + +/** + * Remembers the last server-settings write that is known to have reached disk, + * so a *later* failed write can be rolled back to what is actually persisted + * rather than to a `servers` entry that has gone stale (#2089). + * + * The `servers` list only advances when a `GET /api/servers` succeeds. Once a + * list read fails, every entry in it keeps describing disk *as of the last + * successful read* — while writes issued since then have changed disk. A + * rollback that reads its baseline from such an entry reverts to a value + * nothing on disk holds, and the UI contradicts the file until the next + * successful read. The reproduction is two toggles: the first write lands but + * its list reload fails, the second write fails outright and reverts to the + * pre-first-toggle value. + * + * The record is not trusted indefinitely. It is paired with the `servers` entry + * the write was derived from, and a successful list read replaces that entry + * (the list is re-parsed from the response, so a fresh read always yields fresh + * objects) — which is the signal that the list has caught up with, or been + * overtaken by (an edit made outside the Inspector), our write. From there the + * entry is authoritative again and the record is ignored. The *entry* is the + * token rather than its `settings` node because a server that has never been + * configured carries no `settings` at all, and `undefined` cannot distinguish + * a re-read from a stale one. + * + * Kept in a ref rather than state deliberately: nothing renders from it, it is + * read only inside callbacks at the moment a write is issued, and holding it in + * state would re-render the whole composition root on every settings save. + */ +export interface LastPersistedSettings { + /** + * Record a settings write that reached disk. `entry` is the `servers` entry + * the write was derived from. + */ + record: ( + serverId: string, + written: InspectorServerSettings, + entry: ServerEntry, + ) => void; + /** + * The best available answer to "what is on disk for this server". Returns the + * recorded write while `entry` is still the object the write was derived + * from, and `fallback` (the caller's read of the entry) once a fresh list + * read has replaced it, or when no write has landed for that server this + * session. + */ + resolve: ( + serverId: string, + entry: ServerEntry, + fallback: InspectorServerSettings, + ) => InspectorServerSettings; +} + +interface WriteRecord { + serverId: string; + written: InspectorServerSettings; + entry: ServerEntry; +} + +export function useLastPersistedSettings(): LastPersistedSettings { + const recordRef = useRef(null); + + const record = useCallback( + ( + serverId: string, + written: InspectorServerSettings, + entry: ServerEntry, + ) => { + recordRef.current = { serverId, written, entry }; + }, + [], + ); + + const resolve = useCallback( + ( + serverId: string, + entry: ServerEntry, + fallback: InspectorServerSettings, + ): InspectorServerSettings => { + const current = recordRef.current; + if (!current || current.serverId !== serverId) return fallback; + // Identity, not deep equality: the question is whether the list has been + // re-read since the write, not whether the values happen to match. + if (current.entry !== entry) return fallback; + return current.written; + }, + [], + ); + + return { record, resolve }; +}