diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index 7d1a1503..631cb654 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -1762,6 +1762,50 @@ describe("App roots live-apply on settings-dialog close", () => { // …and the rejected roots edit is not advertised either. expect(client.setRoots).not.toHaveBeenCalled(); }); + + it("restores the roots and log size too when a flushed save rejects (#2089)", async () => { + // Close flushes a pending save and live-applies the draft immediately — + // roots and the Network log size, not just the settings object. If that + // save then rejects, reconciling only `setServerSettings` would leave the + // server advertising roots that were never persisted. + const user = userEvent.setup(); + updateServerSettingsSpy.mockClear(); + const rejectedDraft: InspectorServerSettings = { + ...settingsWithRoots([{ uri: "file:///rejected" }]), + maxFetchRequests: 10, + }; + const client = await openSettingsForConnectedServer(rejectedDraft); + client.getRoots.mockReturnValue([]); + + const draftOptions = vi.mocked(useSettingsDraft).mock.calls.at(-1)?.[0]; + if (!draftOptions) throw new Error("useSettingsDraft was never called"); + const persistedSettings = settingsWithRoots([]); + await act(async () => { + await draftOptions.onPersist("A", persistedSettings); + }); + + // Close applies the draft live — the state the flush leaves behind. + await closeModal(user); + await waitFor(() => + expect(client.setRoots).toHaveBeenCalledWith([ + { uri: "file:///rejected" }, + ]), + ); + client.getRoots.mockReturnValue([{ uri: "file:///rejected" }]); + client.setRoots.mockClear(); + + // …and only now does the flushed save reject. + updateServerSettingsSpy.mockRejectedValueOnce(new Error("disk full")); + await act(async () => { + await draftOptions.onPersist("A", rejectedDraft).catch(() => undefined); + }); + + // The whole live surface goes back to what landed, roots included. + await waitFor(() => expect(client.setRoots).toHaveBeenCalledWith([])); + expect(client.setServerSettings).toHaveBeenLastCalledWith( + persistedSettings, + ); + }); }); describe("App history pin/replay", () => { diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index c96f905f..f32ef321 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -1738,12 +1738,6 @@ function App() { // been destroyed — while the replacement client, built from the stale list // entry, keeps the value the write replaced (#2089). const inspectorClientRef = useRef(inspectorClient); - // The server whose last settings-modal save rejected, if any. `useSettingsDraft` - // keeps the edited draft on failure, and closing the modal pushes that draft - // into the live client — which would put back a value that never reached disk - // and undo the rollback. Cleared by the next save for that server that lands, - // and by the close that acts on it (#2089). - const failedSettingsSaveRef = useRef(undefined); useEffect(() => { if (activeServer) activeServerNameRef.current = activeServer.name; activeServerIdRef.current = activeServerId; @@ -1936,6 +1930,43 @@ function App() { // rebound every time the active server (and its log state) changes. const fetchLogRef = useRef(null); + // Make the live client reflect one settings value, in full. Every path that + // decides what the active server's settings *are* goes through this: the + // settings modal's close (#1444), and each write's settled / rolled-back + // reconciliation (#2089). It is one function because the live surface is more + // than `setServerSettings` — a partial re-application would leave the Network + // log sized for, and the server advertised roots from, a value that is not on + // disk. Reads the client through its ref so a write outliving a reconnect + // applies to the instance that exists now. + const applyLiveServerSettings = useCallback( + (settings: InspectorServerSettings) => { + const client = inspectorClientRef.current; + if (!client) return; + // Settings the managed state reads at notification time + // (auto-refresh-on-list-changed) take effect without a reconnect (#1444). + // Connection-time inputs (transport, OAuth, timeouts) still only apply on + // the next connect. + client.setServerSettings(settings); + // Resize the Network log buffer live so a maxFetchRequests edit takes + // effect without a reconnect (shrinking trims immediately). + fetchLogRef.current?.setMaxFetchRequests(settings.maxFetchRequests); + // Root edits are diffed against what the client currently advertises + // (both cleaned) and notified only when they differ: `setRoots` fires + // `notifications/roots/list_changed`, which makes the server re-request + // `roots/list`. + const nextRoots = cleanRoots(settings.roots); + const currentRoots = cleanRoots(client.getRoots()); + if (JSON.stringify(nextRoots) !== JSON.stringify(currentRoots)) { + void client.setRoots(nextRoots).catch(() => { + // setRoots swallows notification failures internally; a throw here + // only means the client is mid-teardown — the persisted roots will + // re-advertise on the next connect, so nothing to surface. + }); + } + }, + [], + ); + // Flush the pre-redirect Network log to backend storage, keyed by the OAuth // authId carried in the authorization URL's `state`. Runs synchronously from // `BrowserNavigation` right before `window.location.href`, so the keepalive @@ -3814,7 +3845,7 @@ function App() { const settled = write.landed(next); if (settled && activeServerIdRef.current === activeServerId) { setPaginatedListsOverride(next.paginatedLists ?? false); - inspectorClientRef.current?.setServerSettings(next); + applyLiveServerSettings(next); } }) .catch((err: unknown) => { @@ -3845,7 +3876,7 @@ function App() { lastPersistedSettings.resolve(activeServerId) ?? EMPTY_SETTINGS; if (activeServerIdRef.current === activeServerId) { setPaginatedListsOverride(baseline.paginatedLists ?? false); - inspectorClientRef.current?.setServerSettings(baseline); + applyLiveServerSettings(baseline); } notifications.show({ title: "Failed to save pagination setting", @@ -3858,6 +3889,7 @@ function App() { servers, activeServerId, lastPersistedSettings, + applyLiveServerSettings, inspectorClient, updateServerSettings, refreshInitialConfig, @@ -4243,11 +4275,6 @@ 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(); - // The draft keeps the values this save was carrying, and closing the - // modal pushes the draft into the live client — which would put back - // the value that just failed to reach disk. Remember that this - // server's draft is unpersisted so close can apply disk instead. - failedSettingsSaveRef.current = id; // 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 @@ -4255,7 +4282,7 @@ function App() { const baseline = lastPersistedSettings.resolve(id); if (baseline && activeServerIdRef.current === id) { setPaginatedListsOverride(baseline.paginatedLists ?? false); - inspectorClientRef.current?.setServerSettings(baseline); + applyLiveServerSettings(baseline); } throw err; } @@ -4270,17 +4297,10 @@ function App() { // toggle failing while this save is in flight rolls the UI and the live // client back to a baseline this save then replaces on disk, so a settled // save has to re-apply itself exactly as a settled toggle does (#2089). - // This save reached disk, so the draft it came from is persisted again. - if (failedSettingsSaveRef.current === id) { - failedSettingsSaveRef.current = undefined; - } const settled = write.landed(value); if (settled && activeServerIdRef.current === id) { setPaginatedListsOverride(value.paginatedLists ?? false); - // 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); + applyLiveServerSettings(value); } }, // Surface failures via toast — the modal usually closes @@ -4416,36 +4436,19 @@ function App() { settingsDraft ) { // Normally the draft is what the user just saved, so it is also what is - // on disk. When this server's last save *rejected* it is not: the draft - // still holds the failed edit, and pushing it here would contradict disk - // and undo the rollback that reported the failure. Apply what landed - // instead, and drop the flag — the stale draft goes with the modal, and - // reopening seeds from the tracker (#2089). - let applied = settingsDraft; - if (failedSettingsSaveRef.current === settingsModalTargetId) { - applied = - lastPersistedSettings.resolve(settingsModalTargetId) ?? settingsDraft; - failedSettingsSaveRef.current = undefined; - } - // Push the edited settings onto the live client so settings the managed - // state reads at notification time (auto-refresh-on-list-changed) take - // effect without a reconnect (#1444). Connection-time inputs (transport, - // OAuth, timeouts) still only apply on the next connect. - inspectorClient.setServerSettings(applied); - // Resize the Network log buffer live so a maxFetchRequests edit takes - // effect without a reconnect (shrinking trims immediately). Connect-time - // construction also reads this, so a reconnect would apply it anyway — - // this just makes the toast→adjust flow responsive. - fetchLogRef.current?.setMaxFetchRequests(applied.maxFetchRequests); - const nextRoots = cleanRoots(applied.roots); - const currentRoots = cleanRoots(inspectorClient.getRoots()); - if (JSON.stringify(nextRoots) !== JSON.stringify(currentRoots)) { - void inspectorClient.setRoots(nextRoots).catch(() => { - // setRoots swallows notification failures internally; a throw here - // only means the client is mid-teardown — the persisted roots will - // re-advertise on the next connect, so nothing to surface. - }); - } + // on disk. When this server's last write *rejected* it is not: the draft + // still holds the failed edit, and applying it here would contradict disk + // and undo the rollback that reported the failure — so apply what landed + // instead. `EMPTY_SETTINGS`, not the draft, when nothing has ever landed + // for this server: falling back to the draft would re-apply the very + // values that failed (#2089). + const applied = lastPersistedSettings.lastWriteFailed( + settingsModalTargetId, + ) + ? (lastPersistedSettings.resolve(settingsModalTargetId) ?? + EMPTY_SETTINGS) + : settingsDraft; + applyLiveServerSettings(applied); } setSettingsModalTargetId(undefined); }, [ @@ -4455,6 +4458,7 @@ function App() { activeServerId, settingsDraft, lastPersistedSettings, + applyLiveServerSettings, ]); // The Resources screen needs `isSubscribed` to flip the Subscribe button diff --git a/clients/web/src/hooks/useLastPersistedSettings.test.tsx b/clients/web/src/hooks/useLastPersistedSettings.test.tsx index 7231d56c..4141fe22 100644 --- a/clients/web/src/hooks/useLastPersistedSettings.test.tsx +++ b/clients/web/src/hooks/useLastPersistedSettings.test.tsx @@ -191,6 +191,31 @@ describe("useLastPersistedSettings", () => { expect(api.resolve("A")).toBe(fresh); }); + it("reports a failed last write per server, ordered by issue", () => { + // The settings modal's draft survives a rejected save, so its owner has to + // know whether what the user last tried to save is on disk. A failure on B + // says nothing about A, and a straggling failure from an older write does + // not overrule a newer write that succeeded. + const { api } = harness([entry("A", settings()), entry("B", settings())]); + expect(api.lastWriteFailed("A")).toBe(false); + + const firstA = api.begin("A"); + const secondA = api.begin("A"); + secondA.landed(settings({ paginatedLists: true })); + firstA.failed(); + expect(api.lastWriteFailed("A")).toBe(false); + + api.begin("B").failed(); + expect(api.lastWriteFailed("B")).toBe(true); + expect(api.lastWriteFailed("A")).toBe(false); + + // …and a newer failure on A does count, until a newer write lands. + api.begin("A").failed(); + expect(api.lastWriteFailed("A")).toBe(true); + api.begin("A").landed(settings()); + expect(api.lastWriteFailed("A")).toBe(false); + }); + it("returns nothing when the server has left the list entirely", () => { const { api, setServers } = harness([entry("A", settings())]); api.begin("A").landed(settings({ paginatedLists: true })); diff --git a/clients/web/src/hooks/useLastPersistedSettings.ts b/clients/web/src/hooks/useLastPersistedSettings.ts index 42fa6ae4..86e7649c 100644 --- a/clients/web/src/hooks/useLastPersistedSettings.ts +++ b/clients/web/src/hooks/useLastPersistedSettings.ts @@ -81,6 +81,19 @@ export interface LastPersistedSettings { * same class of bug as reading the baseline from a stale `servers` entry. */ resolve: (serverId: string) => InspectorServerSettings | undefined; + /** + * Whether the most recently **issued** write that has reported for this + * server ended in failure — i.e. whatever the user last tried to save is not + * what disk holds. A caller holding an edit buffer for that server (the + * settings modal's draft) uses it to know its buffer is unpersisted, and to + * apply `resolve` instead of the buffer. + * + * Ordered by issue, not by arrival, for the same reason the record is: a + * straggling failure from an older write does not overwrite the outcome of a + * newer one, and outcomes are kept per server so a failure on B says nothing + * about A. + */ + lastWriteFailed: (serverId: string) => boolean; } export interface SettingsWrite { @@ -126,6 +139,12 @@ export function useLastPersistedSettings( // 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()); + // Outcome of the latest-issued write that has reported, per server. Kept + // apart from the record because a *failed* write leaves no record but is + // still the answer to "is the user's last attempt on disk?". + const outcomesRef = 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 @@ -141,12 +160,18 @@ export function useLastPersistedSettings( const pending = pendingRef.current.get(serverId) ?? new Set(); pending.add(sequence); pendingRef.current.set(serverId, pending); - const settle = () => { + const settle = (ok: boolean) => { pending.delete(sequence); + // Keep the outcome of the latest-issued write that has reported, so an + // older straggler cannot overwrite a newer one's verdict. + const previous = outcomesRef.current.get(serverId); + if (!previous || previous.sequence <= sequence) { + outcomesRef.current.set(serverId, { sequence, ok }); + } }; return { landed: (written: InspectorServerSettings) => { - settle(); + settle(true); 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. @@ -160,10 +185,15 @@ export function useLastPersistedSettings( for (const other of pending) if (other > sequence) return false; return true; }, - failed: settle, + failed: () => settle(false), }; }, []); + const lastWriteFailed = useCallback( + (serverId: string) => outcomesRef.current.get(serverId)?.ok === false, + [], + ); + const resolve = useCallback( (serverId: string): InspectorServerSettings | undefined => { const entry = serversRef.current.find((s) => s.id === serverId); @@ -182,5 +212,5 @@ export function useLastPersistedSettings( [], ); - return { begin, resolve }; + return { begin, resolve, lastWriteFailed }; }