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