From ba633fc8d2d587c7cca359d56685aa67c6fc238f Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 23 Aug 2026 13:36:21 -0400 Subject: [PATCH] fix(web): seed the settings draft from the tracker, and don't re-apply a rejected one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 8 on #2091, both findings — the two remaining places the stale `servers` entry and the unpersisted draft still leaked. The modal draft was seeded straight from `servers`, and the draft is the input to the *next* write: after a write landed whose list reload failed, opening the modal and saving any unrelated field sent the superseded value back to disk. It now seeds through the tracker, which falls back to the entry when nothing has landed, so the ordinary case is unchanged. `useSettingsDraft` also keeps the draft when a save rejects, and closing the modal unconditionally pushed that draft into the live client — so the client returned to the value that had just failed to reach disk, undoing the rollback that reported the failure. The server whose last modal save rejected is now remembered, and close applies what landed for it instead (the roots diff and the fetch-log resize included). The flag clears on the next save for that server that lands, and on the close that acts on it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4 Signed-off-by: cliffhall --- clients/web/src/App.test.tsx | 70 ++++++++++++++++++++++++++++++++++++ clients/web/src/App.tsx | 42 +++++++++++++++++++--- 2 files changed, 107 insertions(+), 5 deletions(-) diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index 23204d97..7d1a1503 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -1628,6 +1628,9 @@ describe("App task wiring", () => { type RootsFakeClient = EventTarget & { setRoots: ReturnType; getRoots: ReturnType; + // Close also pushes the settings it decided to apply, which is what the + // failed-save case asserts on (#2089). + setServerSettings: ReturnType; }; const settingsWithRoots = ( @@ -1718,6 +1721,47 @@ describe("App roots live-apply on settings-dialog close", () => { ); expect(client.setRoots).not.toHaveBeenCalled(); }); + + it("applies the last persisted settings on close after a save failed (#2089)", async () => { + // `useSettingsDraft` keeps the draft when a save rejects, so closing the + // modal would push a value that never reached disk into the live client — + // undoing the rollback that reported the failure. Close has to apply what + // landed instead. + const user = userEvent.setup(); + updateServerSettingsSpy.mockClear(); + const failedDraft: InspectorServerSettings = { + ...settingsWithRoots([{ uri: "file:///rejected" }]), + paginatedLists: true, + }; + const client = await openSettingsForConnectedServer(failedDraft); + client.getRoots.mockReturnValue([]); + + const draftOptions = vi.mocked(useSettingsDraft).mock.calls.at(-1)?.[0]; + if (!draftOptions) throw new Error("useSettingsDraft was never called"); + // One save lands, so the tracker has an account of disk… + const persistedSettings: InspectorServerSettings = { + ...settingsWithRoots([]), + paginatedLists: false, + }; + await act(async () => { + await draftOptions.onPersist("A", persistedSettings); + }); + // …then the next one rejects, leaving the draft holding an unsaved edit. + updateServerSettingsSpy.mockRejectedValueOnce(new Error("disk full")); + await act(async () => { + await draftOptions.onPersist("A", failedDraft).catch(() => undefined); + }); + client.setServerSettings.mockClear(); + + await closeModal(user); + + await waitFor(() => expect(client.setServerSettings).toHaveBeenCalled()); + expect(client.setServerSettings).toHaveBeenLastCalledWith( + persistedSettings, + ); + // …and the rejected roots edit is not advertised either. + expect(client.setRoots).not.toHaveBeenCalled(); + }); }); describe("App history pin/replay", () => { @@ -2713,6 +2757,32 @@ describe("App paginated list pagination toggle (#1721)", () => { ); }); + it("seeds the settings modal from the last write, not the stale entry (#2089)", async () => { + // The modal draft is the input to the *next* write, so seeding it from a + // `servers` entry that a failed list reload froze would send the superseded + // value back to disk the moment the user saves any unrelated field. + 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 }), + ), + ); + + // `servers` is mocked to a fixed entry here, so it still reports the + // pre-toggle value — exactly what a failed reload leaves behind. + const draftOptions = vi.mocked(useSettingsDraft).mock.calls.at(-1)?.[0]; + if (!draftOptions) throw new Error("useSettingsDraft was never called"); + expect(draftOptions.resolveInitial("A")).toEqual( + 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 diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index af692c73..c96f905f 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -1738,6 +1738,12 @@ 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; @@ -4209,8 +4215,12 @@ function App() { flush: flushSettingsDraft, } = useSettingsDraft({ targetId: settingsModalTargetId, - resolveInitial: (id) => - servers.find((s) => s.id === id)?.settings ?? EMPTY_SETTINGS, + // Through the tracker rather than off `servers` directly: after a write + // lands whose list reload failed, that entry still describes the previous + // value, so seeding from it and saving an unrelated field would write the + // superseded value straight back to disk (#2089). `resolve` falls back to + // the entry when nothing has landed, so the ordinary case is unchanged. + resolveInitial: (id) => lastPersistedSettings.resolve(id) ?? EMPTY_SETTINGS, // The saved settings may include an OAuth client secret or a stdio `env:` // value, and writing one can change what the secrets file *is* — the first // save under a newly-set passphrase re-encrypts a pre-existing plaintext @@ -4233,6 +4243,11 @@ 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,6 +4270,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); @@ -4396,17 +4415,29 @@ function App() { settingsModalTargetId === activeServerId && 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(settingsDraft); + 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(settingsDraft.maxFetchRequests); - const nextRoots = cleanRoots(settingsDraft.roots); + 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(() => { @@ -4423,6 +4454,7 @@ function App() { settingsModalTargetId, activeServerId, settingsDraft, + lastPersistedSettings, ]); // The Resources screen needs `isSubscribed` to flip the Subscribe button