diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index 0394c4e8..ed1a83c8 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -2522,6 +2522,61 @@ describe("App paginated list pagination toggle (#1721)", () => { expect(lastPush?.paginatedLists).toBe(true); }); + it("re-applies a write that lands after an overlapping one failed (#2089)", async () => { + // The mirror of the test above, in the other settlement order: toggle 1 + // (`true`) is still in flight when toggle 2 (`false`) fails, so the rollback + // resolves to the stale entry's `false` — correct at that instant, since + // nothing had landed. Toggle 1 then lands `true`. If its list reload failed + // too, no render will ever correct the UI, so the write that settles has to + // re-apply itself. + const user = userEvent.setup(); + let landFirst: (() => void) | undefined; + let failSecond: ((err: Error) => void) | undefined; + const first = new Promise((resolve) => { + landFirst = resolve; + }); + const second = new Promise((_resolve, reject) => { + failSecond = (err) => { + reject(err); + }; + }); + updateServerSettingsSpy + .mockImplementationOnce(() => first) + .mockImplementationOnce(() => second); + + renderWithMantine(); + await user.click(screen.getByText("connect")); + await waitFor(() => expect(clientInstances).toHaveLength(1)); + + await user.click(screen.getByText("paginated-on")); + await user.click(screen.getByText("paginated-off")); + await waitFor(() => + expect(updateServerSettingsSpy).toHaveBeenCalledTimes(2), + ); + + // Fail the second write first, then land the first. + await act(async () => { + failSecond?.(new Error("disk full")); + await second.catch(() => undefined); + }); + expect(screen.getByTestId("tools-paginated")).toHaveTextContent("false"); + await act(async () => { + landFirst?.(); + await first; + }); + + 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); + }); + 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 e31979e8..e41a71f0 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -111,6 +111,7 @@ 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 { useValueChange } from "./hooks/useValueChange"; import type { ListPaginationControlsProps } from "./components/elements/ListPaginationControls/ListPaginationControls"; import { useManagedResourceTemplates } from "@inspector/core/react/useManagedResourceTemplates.js"; import { useManagedRequestorTasks } from "@inspector/core/react/useManagedRequestorTasks.js"; @@ -1175,15 +1176,19 @@ function App() { // Drop the optimistic override once the persisted value catches up (or the // active server changes), so the persisted setting is the resting source. // - // Keyed on the entry object as well as the value it carries: a rollback can - // set the override to a *concrete* value rather than clear it, and if a later - // successful read then reports the value the override replaced — an edit made - // outside the Inspector overtaking the write — neither the boolean nor the id - // changes and the UI stays stuck on the override. A read rebuilds the list, so - // a fresh entry is what makes every authoritative read supersede it (#2089). - useEffect(() => { - setPaginatedListsOverride(null); - }, [persistedPaginatedLists, activeServerId, activeServerEntry]); + // Keyed on the entry object rather than on the boolean it carries: the + // override can hold a *concrete* value (a rollback sets one instead of + // clearing), and if a later successful read reports the value that override + // replaced — an outside edit overtaking the write — the boolean and the + // server id are both unchanged and the UI stays stuck on it. Every successful + // read rebuilds the list, so the entry is the signal that supersedes it, and + // it subsumes the other two: the value is read off this entry, and switching + // servers selects a different one (#2089). + // + // Adjusted during render rather than in an effect, per this repo's rule + // against resetting state from render data in `useEffect` — the entry is + // referentially stable between reads, which is what `useValueChange` needs. + useValueChange(activeServerEntry, () => setPaginatedListsOverride(null)); const paginatedLists = paginatedListsOverride ?? persistedPaginatedLists; // The malformed-entry report is written by the aggregate walk's salvage. In // paginated mode the tools/prompts/resources panels render the paged stores @@ -3785,9 +3790,23 @@ function App() { // 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). - write.landed(next); + // + // Re-apply it when this write is the settled one: an overlapping + // 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. + const settled = write.landed(next); + if (settled && activeServerIdRef.current === activeServerId) { + setPaginatedListsOverride(next.paginatedLists ?? false); + inspectorClient?.setServerSettings(next); + } }) .catch((err: unknown) => { + // This write is over and never reached disk, so it stops counting as + // in flight: an earlier write still running is the settled state once + // it lands, and is what re-applies the UI this rollback is about to + // set (#2089). + write.failed(); // 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). @@ -4191,10 +4210,18 @@ function App() { // flush can put two saves for one server in flight, and the later-issued // one describes disk however the two happen to finish (#2089). const write = lastPersistedSettings.begin(id); - await refreshingPersist(updateServerSettings, refreshInitialConfig)( - id, - value, - ); + try { + await refreshingPersist(updateServerSettings, refreshInitialConfig)( + id, + value, + ); + } catch (err) { + // Stop counting as in flight before the rejection goes on to + // `onError` — an earlier write still running settles the state once it + // lands, and cannot know that while this one looks pending (#2089). + write.failed(); + throw err; + } // Recorded for the same reason the pagination toggle records its own // write: 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 diff --git a/clients/web/src/hooks/useLastPersistedSettings.test.tsx b/clients/web/src/hooks/useLastPersistedSettings.test.tsx index b7ea2308..7231d56c 100644 --- a/clients/web/src/hooks/useLastPersistedSettings.test.tsx +++ b/clients/web/src/hooks/useLastPersistedSettings.test.tsx @@ -149,6 +149,35 @@ describe("useLastPersistedSettings", () => { expect(api.resolve("A")).toBe(newest); }); + it("reports settled only for the last write issued", () => { + // The flag is what tells a caller it is safe to re-apply the value to the + // UI: while a later write is still in flight, that write will describe disk + // when it reports, and re-applying now would fight it. + const { api } = harness([entry("A", settings())]); + const first = api.begin("A"); + expect(first.landed(settings({ paginatedLists: true }))).toBe(true); + const second = api.begin("A"); + const third = api.begin("A"); + expect(second.landed(settings({ paginatedLists: false }))).toBe(false); + expect(third.landed(settings({ paginatedLists: true }))).toBe(true); + // A superseded straggler is neither recorded nor settled. + expect(second.landed(settings({ paginatedLists: false }))).toBe(false); + }); + + it("treats an earlier write as settled once the later one has failed", () => { + // The order that leaves the UI wrong: the later write fails first, rolling + // the UI back to a baseline the earlier write then replaces on disk. Once + // the failed write stops counting as in flight, the earlier one settles and + // its caller can re-apply the value. + const { api } = harness([entry("A", settings())]); + const first = api.begin("A"); + const second = api.begin("A"); + second.failed(); + const written = settings({ paginatedLists: true }); + expect(first.landed(written)).toBe(true); + expect(api.resolve("A")).toBe(written); + }); + it("does not let a straggler resurrect a record a fresh read dropped", () => { // The high-water mark outlives the record, so an earlier write reporting // after `resolve` invalidated things cannot reinstate a superseded value. diff --git a/clients/web/src/hooks/useLastPersistedSettings.ts b/clients/web/src/hooks/useLastPersistedSettings.ts index dde1b6c4..42fa6ae4 100644 --- a/clients/web/src/hooks/useLastPersistedSettings.ts +++ b/clients/web/src/hooks/useLastPersistedSettings.ts @@ -87,8 +87,22 @@ export interface SettingsWrite { /** * Record that this write reached disk. A no-op if a write issued after this * one has already been confirmed for the same server. + * + * Returns whether this write is now the **settled** state for that server — + * recorded, with no later write still in flight to describe disk after it. + * Callers that own UI derived from the setting use it to re-apply the value: + * an overlapping write that failed *first* will have rolled that UI back to a + * baseline this write has since replaced, and if the list read behind this + * one failed too, nothing else would ever correct it. */ - landed: (written: InspectorServerSettings) => void; + landed: (written: InspectorServerSettings) => boolean; + /** + * Report that this write is over without reaching disk, so it stops counting + * as in flight. An earlier write that lands afterwards is then the settled + * state — which is exactly the order that leaves the UI on a rolled-back + * value nothing on disk holds. + */ + failed: () => void; } interface WriteRecord { @@ -108,6 +122,10 @@ export function useLastPersistedSettings( // record dropped by `resolve` can't be resurrected by a straggler that was // issued earlier: the high-water mark outlives the record it produced. const confirmedRef = useRef>(new Map()); + // Issue orders still in flight per server, so a write that lands can tell + // whether it is the last word or whether something later will still report. + // A write leaves this set through `landed` or `failed` alike. + const pendingRef = useRef>>(new Map()); // Read the list through a ref so a write pairs with the entry as it stands // when it finishes, not the one captured when it was issued. Mirrored in an // effect rather than assigned during render: writing a ref mid-render is an @@ -120,16 +138,29 @@ export function useLastPersistedSettings( const begin = useCallback((serverId: string): SettingsWrite => { const sequence = ++nextSequenceRef.current; + const pending = pendingRef.current.get(serverId) ?? new Set(); + pending.add(sequence); + pendingRef.current.set(serverId, pending); + const settle = () => { + pending.delete(sequence); + }; return { landed: (written: InspectorServerSettings) => { + settle(); const confirmed = confirmedRef.current.get(serverId) ?? 0; // A write issued after this one already reported; it describes disk // more recently, whichever of the two happened to finish first. - if (confirmed > sequence) return; + if (confirmed > sequence) return false; confirmedRef.current.set(serverId, sequence); const entry = serversRef.current.find((s) => s.id === serverId); recordsRef.current.set(serverId, { written, entry }); + // Settled only while nothing later is still in flight — such a write + // will describe disk once it reports, so re-applying this value in the + // meantime would fight it. + for (const other of pending) if (other > sequence) return false; + return true; }, + failed: settle, }; }, []);