diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index c4bc832a..23204d97 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -2624,6 +2624,95 @@ describe("App paginated list pagination toggle (#1721)", () => { expect(lastPush?.paginatedLists).toBe(true); }); + it("applies a settled write to the current client, not the one it started on (#2089)", async () => { + // A write can outlive a disconnect/reconnect to the *same* server: the id + // guard still passes, but the client captured when the write was issued has + // been destroyed and replaced by one built from the stale list entry. The + // settled value has to reach the live instance. + const user = userEvent.setup(); + let landWrite: (() => void) | undefined; + const write = new Promise((resolve) => { + landWrite = resolve; + }); + updateServerSettingsSpy.mockImplementationOnce(() => write); + + renderWithMantine(); + await user.click(screen.getByText("connect")); + await waitFor(() => expect(clientInstances).toHaveLength(1)); + + await user.click(screen.getByText("paginated-on")); + // Reconnect to the same server while that write is in flight. The transport + // drops, then the user reconnects — and `onToggleConnection` always + // rebuilds the client so the latest saved settings are picked up. + act(() => { + clientInstances[0].dispatchEvent(new Event("disconnect")); + }); + await user.click(screen.getByText("connect")); + await waitFor(() => expect(clientInstances).toHaveLength(2)); + const replacement = clientInstances[1] as EventTarget & { + setServerSettings: ReturnType; + }; + replacement.setServerSettings.mockClear(); + + await act(async () => { + landWrite?.(); + await write; + }); + + await waitFor(() => + expect(replacement.setServerSettings).toHaveBeenCalledWith( + expect.objectContaining({ paginatedLists: true }), + ), + ); + }); + + it("reconciles after a modal save fails onto a write that landed first (#2089)", async () => { + // Reverse of the mixed-writer case above: the toggle lands while the modal + // save is still pending, so it is told it is not settled and applies + // nothing. The modal save then fails, which makes the toggle's value the + // account of disk — and nothing else is left to apply it. + const user = userEvent.setup(); + let failModal: ((err: Error) => void) | undefined; + const modalWrite = new Promise((_resolve, reject) => { + failModal = (err) => { + reject(err); + }; + }); + 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"); + updateServerSettingsSpy.mockImplementationOnce(() => modalWrite); + const persisted = draftOptions + .onPersist("A", { ...settingsWithRoots([]), paginatedLists: false }) + .catch(() => undefined); + + // The toggle lands `true` while the modal save is still in flight. + await user.click(screen.getByText("paginated-on")); + await waitFor(() => + expect(updateServerSettingsSpy).toHaveBeenCalledTimes(2), + ); + const client = clientInstances[0] as EventTarget & { + setServerSettings: ReturnType; + }; + client.setServerSettings.mockClear(); + + await act(async () => { + failModal?.(new Error("disk full")); + await persisted; + }); + + // The failure is what reconciles: a push carrying the landed value arrives + // only after it, since the landed write itself was not settled at the time. + await waitFor(() => + expect(client.setServerSettings).toHaveBeenCalledWith( + 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 80973768..af692c73 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -1732,11 +1732,18 @@ function App() { const activeServerNameRef = useRef(undefined); const activeServerIdRef = useRef(activeServerId); const serversRef = useRef(servers); + // The live client, mirrored for the same reason: a settings write that + // outlives a disconnect/reconnect to the *same* server would otherwise push + // its value into the instance captured when it was issued — which has since + // been destroyed — while the replacement client, built from the stale list + // entry, keeps the value the write replaced (#2089). + const inspectorClientRef = useRef(inspectorClient); useEffect(() => { if (activeServer) activeServerNameRef.current = activeServer.name; activeServerIdRef.current = activeServerId; serversRef.current = servers; - }, [activeServer, activeServerId, servers]); + inspectorClientRef.current = inspectorClient; + }, [activeServer, activeServerId, servers, inspectorClient]); // Last connection-level error message, surfaced as `data-error-message` on // the InspectorView header so an automated driver can read *why* a connect @@ -3795,10 +3802,13 @@ function App() { // 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. + // Through the *current* client, not this continuation's closure: a + // reconnect to the same server passes the id check while the captured + // instance is already destroyed. const settled = write.landed(next); if (settled && activeServerIdRef.current === activeServerId) { setPaginatedListsOverride(next.paginatedLists ?? false); - inspectorClient?.setServerSettings(next); + inspectorClientRef.current?.setServerSettings(next); } }) .catch((err: unknown) => { @@ -3822,11 +3832,14 @@ function App() { // 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. + // The client is taken from the ref for the same reason the success + // path does: a reconnect to the same server passes the id check while + // this continuation's captured instance is already destroyed. const baseline = lastPersistedSettings.resolve(activeServerId) ?? EMPTY_SETTINGS; if (activeServerIdRef.current === activeServerId) { setPaginatedListsOverride(baseline.paginatedLists ?? false); - inspectorClient?.setServerSettings(baseline); + inspectorClientRef.current?.setServerSettings(baseline); } notifications.show({ title: "Failed to save pagination setting", @@ -4220,6 +4233,15 @@ 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(); + // 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 + // reconciliation the toggle's own failure path does. + const baseline = lastPersistedSettings.resolve(id); + if (baseline && activeServerIdRef.current === id) { + setPaginatedListsOverride(baseline.paginatedLists ?? false); + inspectorClientRef.current?.setServerSettings(baseline); + } throw err; } // Recorded for the same reason the pagination toggle records its own @@ -4236,7 +4258,10 @@ function App() { const settled = write.landed(value); if (settled && activeServerIdRef.current === id) { setPaginatedListsOverride(value.paginatedLists ?? false); - inspectorClient?.setServerSettings(value); + // 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); } }, // Surface failures via toast — the modal usually closes