diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index d5dc2ce7..0394c4e8 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -2521,6 +2521,102 @@ describe("App paginated list pagination toggle (#1721)", () => { }; 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 + // later read reports the value the override replaced — an edit made outside + // the Inspector overtaking the write — neither that boolean nor the server + // id changes, and the UI would stay stuck on the override forever. A read + // rebuilds the list, so the entry object is the signal that supersedes it. + const user = userEvent.setup(); + const previousUseServers = vi.mocked(useServers).getMockImplementation(); + try { + const { rerender } = renderWithMantine(); + await user.click(screen.getByText("connect")); + await waitFor(() => expect(clientInstances).toHaveLength(1)); + + // A write lands `true` while list reads are stale, then a toggle fails — + // leaving the override on `true` with the entry still reporting `false`. + const draftOptions = vi.mocked(useSettingsDraft).mock.calls.at(-1)?.[0]; + if (!draftOptions) throw new Error("useSettingsDraft was never called"); + await act(async () => { + await draftOptions.onPersist("A", { + ...settingsWithRoots([]), + paginatedLists: true, + }); + }); + updateServerSettingsSpy.mockRejectedValueOnce(new Error("disk full")); + await user.click(screen.getByText("paginated-off")); + await waitFor(() => + expect(screen.getByTestId("tools-paginated")).toHaveTextContent("true"), + ); + + // Now a list read succeeds and still says `false` — same boolean, same + // server id, new entry object. It is authoritative and must win. + vi.mocked(useServers).mockReturnValue({ + servers: [{ ...SERVER_A } as ServerEntry], + loading: false, + error: undefined, + refresh: vi.fn().mockResolvedValue(undefined), + addServer: vi.fn(), + updateServer: vi.fn(), + updateServerSettings: updateServerSettingsSpy, + removeServer: vi.fn(), + reorderServers: vi.fn(), + importSource: vi.fn().mockResolvedValue({ servers: {} }), + }); + rerender(); + await waitFor(() => + expect(screen.getByTestId("tools-paginated")).toHaveTextContent( + "false", + ), + ); + } finally { + if (previousUseServers) { + vi.mocked(useServers).mockImplementation(previousUseServers); + } + } + }); + + it("rolls back to a settings-modal save that landed, not just a toggle (#2089)", async () => { + // The two settings writers feed one record, and this is the half the + // toggle-driven tests cannot reach: `useSettingsDraft` is mocked for this + // file, so its `onPersist` never runs on its own and a regression that + // dropped `begin`/`landed` from the modal path would go unnoticed. The + // callback the App handed the hook is invoked directly instead. + const user = userEvent.setup(); + renderWithMantine(); + await user.click(screen.getByText("connect")); + await waitFor(() => expect(clientInstances).toHaveLength(1)); + + const draftOptions = vi.mocked(useSettingsDraft).mock.calls.at(-1)?.[0]; + if (!draftOptions) throw new Error("useSettingsDraft was never called"); + const saved: InspectorServerSettings = { + ...settingsWithRoots([]), + paginatedLists: true, + }; + // The save lands; the list read behind it is what stays stale, so `servers` + // (mocked to a fixed entry here) never reports it. + await act(async () => { + await draftOptions.onPersist("A", saved); + }); + expect(updateServerSettingsSpy).toHaveBeenCalledWith("A", saved); + + updateServerSettingsSpy.mockRejectedValueOnce(new Error("disk full")); + await user.click(screen.getByText("paginated-off")); + + 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 bf72364c..e31979e8 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -1166,17 +1166,24 @@ function App() { const lastPersistedSettings = useLastPersistedSettings(servers); // The active server's persisted paginated setting drives the display mode. // The sidebar toggle edits it (optimistically, below) and persists it. + const activeServerEntry = servers.find((s) => s.id === activeServerId); const persistedPaginatedLists = - servers.find((s) => s.id === activeServerId)?.settings?.paginatedLists ?? - false; + activeServerEntry?.settings?.paginatedLists ?? false; const [paginatedListsOverride, setPaginatedListsOverride] = useState< boolean | null >(null); // 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]); + }, [persistedPaginatedLists, activeServerId, activeServerEntry]); 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 @@ -3791,10 +3798,17 @@ function App() { // baseline rather than cleared, because clearing it falls back to // `persistedPaginatedLists` — read from a `servers` entry that may be // stale, showing the same wrong value from the other side (#2089). + // + // Applied only while this write's server is still the active one: the + // override is a single app-wide boolean and the live client belongs to + // whichever server is connected now, so a rejection arriving after the + // user switched would render this server's value on another one. const baseline = lastPersistedSettings.resolve(activeServerId) ?? EMPTY_SETTINGS; - setPaginatedListsOverride(baseline.paginatedLists ?? false); - inspectorClient?.setServerSettings(baseline); + if (activeServerIdRef.current === activeServerId) { + setPaginatedListsOverride(baseline.paginatedLists ?? false); + inspectorClient?.setServerSettings(baseline); + } notifications.show({ title: "Failed to save pagination setting", message: err instanceof Error ? err.message : String(err),