From bcedcccb29048fa79f2bd3d03f4d7301e3dcbf7b Mon Sep 17 00:00:00 2001 From: cliffhall Date: Tue, 26 May 2026 11:23:10 -0400 Subject: [PATCH 1/3] fix(servers): make settings form re-render on every keystroke (#1361) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The settings modal is fully controlled by App.tsx, but pending edits were stashed in a useRef — refs don't trigger re-renders, so the input's `value` prop stayed stale until the 300ms debounce + PUT + refetch round-tripped. Clicking "Add Header" / "Add Metadata" showed no row, and typing in OAuth fields had no character echo, until that round-trip completed (~500ms). The bug landed in #1353 with the debounced-flush pattern. It surfaced after #1356 because per-keystroke PUTs now also write the keychain, making the round-trip latency more noticeable. Lift the draft into `useState`. Every change re-renders the modal immediately. Keep the 300ms debounce on the PUT side. Initialize the draft from the server-list entry only when the modal opens to a new target (not on every `servers` change) so background refreshes don't clobber the user's in-progress edits. Flush on close to preserve final keystrokes. Verified live against the dev server: header/metadata rows appear on click, OAuth Client ID / Client Secret typing echoes without lag, and the values persist correctly (clientId in mcp.json, clientSecret in the macOS Keychain). Co-Authored-By: Claude Opus 4.7 (1M context) --- clients/web/src/App.tsx | 147 ++++++++++++++++++++++++---------------- 1 file changed, 89 insertions(+), 58 deletions(-) diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index d15610af5..d389875bf 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -763,77 +763,108 @@ function App() { return servers.find((s) => s.id === configModal.targetId); }, [configModal, servers]); - const settingsModalTarget = useMemo(() => { - if (!settingsModalTargetId) return undefined; - return servers.find((s) => s.id === settingsModalTargetId); - }, [settingsModalTargetId, servers]); - - // Stable starting shape for entries that have no `settings` node yet — the - // form needs concrete arrays / numbers to render. - const settingsModalValue = useMemo(() => { - return ( - settingsModalTarget?.settings ?? { + // The settings modal is a "dumb" component fully driven by the + // `settings` prop we pass it — every keystroke fires `onSettingsChange` + // back up here for us to render the next value. That round-trip needs + // to be state, not a ref: a ref doesn't cause a re-render, so the + // displayed input value can only change after the debounced PUT + // completes and `servers` refetches, which feels broken (the user + // types and nothing shows for ~half a second). Hold the in-progress + // draft in `settingsDraft` so every keystroke re-renders immediately; + // background refetches of `servers` don't reset it because the draft + // is only (re)initialized when the modal opens to a new target id + // (see effect below). + const [settingsDraft, setSettingsDraft] = + useState(null); + const settingsTimerRef = useRef | undefined>( + undefined, + ); + + // Initialize / tear down the draft when the modal opens or closes + // against a different target. We intentionally do NOT depend on + // `servers` here — if the user is mid-edit and a background refresh + // arrives, we don't want to clobber their in-progress changes. + useEffect(() => { + if (!settingsModalTargetId) { + setSettingsDraft(null); + return; + } + const target = servers.find((s) => s.id === settingsModalTargetId); + setSettingsDraft( + target?.settings ?? { headers: [], metadata: [], connectionTimeout: 0, requestTimeout: 0, - } + }, ); - }, [settingsModalTarget]); - - // The settings modal fires onSettingsChange on every keystroke. Persisting - // each character would run a tight PUT-then-full-refresh loop against the - // backend (and the in-memory `servers` state could flicker mid-typing as - // each refresh resets the modal's prop). Debounce so a burst of edits - // coalesces into a single PUT, and flush on modal close so nothing is lost. - const pendingSettingsRef = useRef<{ - id: string; - settings: InspectorServerSettings; - } | null>(null); - const pendingSettingsTimerRef = useRef< - ReturnType | undefined - >(undefined); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [settingsModalTargetId]); + + // Stable fallback so the modal always has a valid `settings` prop, + // even in the brief window where it's closed (Mantine renders the + // component shell regardless of `opened`). + const settingsModalValue = useMemo( + () => + settingsDraft ?? { + headers: [], + metadata: [], + connectionTimeout: 0, + requestTimeout: 0, + }, + [settingsDraft], + ); - const flushPendingSettings = useCallback(() => { - if (pendingSettingsTimerRef.current) { - clearTimeout(pendingSettingsTimerRef.current); - pendingSettingsTimerRef.current = undefined; - } - const pending = pendingSettingsRef.current; - if (!pending) return; - pendingSettingsRef.current = null; - // Fire-and-forget — but surface failures via toast. The modal closes - // immediately on user dismiss, so a silent fail-on-flush would leave - // the user thinking their last edits saved when they didn't (especially - // painful for the OAuth client secret). - updateServerSettings(pending.id, pending.settings).catch((err) => { - notifications.show({ - title: `Failed to save settings for "${pending.id}"`, - message: err instanceof Error ? err.message : String(err), - color: "red", + // Fire the PUT. Surface failures via toast — the modal usually closes + // immediately on user dismiss, so a silent fail-on-flush would leave + // the user thinking their last edits saved when they didn't + // (especially painful for the OAuth client secret). + const sendSettingsUpdate = useCallback( + (id: string, settings: InspectorServerSettings) => { + updateServerSettings(id, settings).catch((err) => { + notifications.show({ + title: `Failed to save settings for "${id}"`, + message: err instanceof Error ? err.message : String(err), + color: "red", + }); }); - }); - }, [updateServerSettings]); + }, + [updateServerSettings], + ); const onSettingsChange = useCallback( (next: InspectorServerSettings) => { if (!settingsModalTargetId) return; - pendingSettingsRef.current = { - id: settingsModalTargetId, - settings: next, - }; - if (pendingSettingsTimerRef.current) { - clearTimeout(pendingSettingsTimerRef.current); - } - pendingSettingsTimerRef.current = setTimeout(flushPendingSettings, 300); + // Update the displayed draft synchronously — this is the + // re-render that makes typed characters and added rows appear. + setSettingsDraft(next); + // Debounce the PUT. Persisting each character would run a tight + // PUT-then-full-refresh loop against the backend; coalesce a + // burst of edits into one PUT and flush on modal close. + if (settingsTimerRef.current) clearTimeout(settingsTimerRef.current); + const id = settingsModalTargetId; + settingsTimerRef.current = setTimeout(() => { + settingsTimerRef.current = undefined; + sendSettingsUpdate(id, next); + }, 300); }, - [settingsModalTargetId, flushPendingSettings], + [settingsModalTargetId, sendSettingsUpdate], ); const onSettingsModalClose = useCallback(() => { - flushPendingSettings(); + // Flush any pending debounce synchronously so the last edits land + // even if the user dismisses before the 300ms timer fires. The + // useEffect above will null out the draft once the target id + // clears, so we don't need to touch it here. + if (settingsTimerRef.current) { + clearTimeout(settingsTimerRef.current); + settingsTimerRef.current = undefined; + if (settingsDraft && settingsModalTargetId) { + sendSettingsUpdate(settingsModalTargetId, settingsDraft); + } + } setSettingsModalTargetId(undefined); - }, [flushPendingSettings]); + }, [settingsDraft, settingsModalTargetId, sendSettingsUpdate]); // Cancel any pending debounce on unmount (route change / HMR). Without // this a stale setTimeout could still fire one final `updateServerSettings` @@ -841,9 +872,9 @@ function App() { // the final payload) but the cleanest pattern is to clear the timer. useEffect(() => { return () => { - if (pendingSettingsTimerRef.current) { - clearTimeout(pendingSettingsTimerRef.current); - pendingSettingsTimerRef.current = undefined; + if (settingsTimerRef.current) { + clearTimeout(settingsTimerRef.current); + settingsTimerRef.current = undefined; } }; }, []); From f45379092df40403c52b0a463947f90c2600a876 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Tue, 26 May 2026 11:44:09 -0400 Subject: [PATCH 2/3] chore(servers): extract useSettingsDraft hook + tests; review polish (#1361) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Picks up the three review observations on #1362: 1. Extract the draft state + debounce + flush logic from App.tsx into `core/react/useSettingsDraft`. Same behavior; thinner App.tsx; testable in isolation with `renderHook`. Tests pin the regression #1361 was about (synchronous onChange → draft update) plus the debounce window, burst-collapse, flush-on-close, error propagation, and the "background refresh must not clobber in-progress edits" contract via a `resolveInitial` closure swap. 2. Hoist the empty `InspectorServerSettings` fallback to a module-scope `EMPTY_SETTINGS` const. Both the hook's init path and the modal's closed-state fallback share the same object identity; the inline `useMemo` that previously re-derived it is gone. 3. Add a comment on the `resolveInitial` callback noting that we read `servers` from this render's closure under the assumption that the entry point is the "Settings" button on a rendered server card. Documents the contract for future callers (e.g. a keyboard shortcut that opens the modal pre-hydration). Validate clean: 1756 tests pass. New useSettingsDraft.test.tsx adds 13 test cases. Co-Authored-By: Claude Opus 4.7 (1M context) --- clients/web/src/App.tsx | 163 +++------ .../test/core/react/useSettingsDraft.test.tsx | 326 ++++++++++++++++++ core/react/useSettingsDraft.ts | 141 ++++++++ 3 files changed, 521 insertions(+), 109 deletions(-) create mode 100644 clients/web/src/test/core/react/useSettingsDraft.test.tsx create mode 100644 core/react/useSettingsDraft.ts diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index d389875bf..68edb7248 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -30,6 +30,7 @@ import { StderrLogState } from "@inspector/core/mcp/state/stderrLogState.js"; import type { RedirectUrlProvider } from "@inspector/core/auth/index.js"; import { useInspectorClient } from "@inspector/core/react/useInspectorClient.js"; import { useServers } from "@inspector/core/react/useServers.js"; +import { useSettingsDraft } from "@inspector/core/react/useSettingsDraft.js"; import { useManagedTools } from "@inspector/core/react/useManagedTools.js"; import { useManagedPrompts } from "@inspector/core/react/useManagedPrompts.js"; import { useManagedResources } from "@inspector/core/react/useManagedResources.js"; @@ -126,6 +127,19 @@ function messagesToLogEntries(messages: MessageEntry[]): LogEntryData[] { return out; } +// Stable empty-shell for `InspectorServerSettings`. Used both as the +// initial draft for a server entry that hasn't been touched yet, and as +// the fallback the settings modal renders against when it's closed +// (Mantine renders the dialog shell regardless of `opened`). Hoisted to +// module scope so both call sites share the same object identity and so +// React doesn't re-allocate on every render. +const EMPTY_SETTINGS: InspectorServerSettings = { + headers: [], + metadata: [], + connectionTimeout: 0, + requestTimeout: 0, +}; + function App() { // Theme toggle plumbing (preserved from the pre-wire placeholder). const { setColorScheme } = useMantineColorScheme(); @@ -763,121 +777,52 @@ function App() { return servers.find((s) => s.id === configModal.targetId); }, [configModal, servers]); - // The settings modal is a "dumb" component fully driven by the - // `settings` prop we pass it — every keystroke fires `onSettingsChange` - // back up here for us to render the next value. That round-trip needs - // to be state, not a ref: a ref doesn't cause a re-render, so the - // displayed input value can only change after the debounced PUT - // completes and `servers` refetches, which feels broken (the user - // types and nothing shows for ~half a second). Hold the in-progress - // draft in `settingsDraft` so every keystroke re-renders immediately; - // background refetches of `servers` don't reset it because the draft - // is only (re)initialized when the modal opens to a new target id - // (see effect below). - const [settingsDraft, setSettingsDraft] = - useState(null); - const settingsTimerRef = useRef | undefined>( - undefined, - ); - - // Initialize / tear down the draft when the modal opens or closes - // against a different target. We intentionally do NOT depend on - // `servers` here — if the user is mid-edit and a background refresh - // arrives, we don't want to clobber their in-progress changes. - useEffect(() => { - if (!settingsModalTargetId) { - setSettingsDraft(null); - return; - } - const target = servers.find((s) => s.id === settingsModalTargetId); - setSettingsDraft( - target?.settings ?? { - headers: [], - metadata: [], - connectionTimeout: 0, - requestTimeout: 0, - }, - ); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [settingsModalTargetId]); - - // Stable fallback so the modal always has a valid `settings` prop, - // even in the brief window where it's closed (Mantine renders the - // component shell regardless of `opened`). - const settingsModalValue = useMemo( - () => - settingsDraft ?? { - headers: [], - metadata: [], - connectionTimeout: 0, - requestTimeout: 0, - }, - [settingsDraft], - ); - - // Fire the PUT. Surface failures via toast — the modal usually closes - // immediately on user dismiss, so a silent fail-on-flush would leave - // the user thinking their last edits saved when they didn't - // (especially painful for the OAuth client secret). - const sendSettingsUpdate = useCallback( - (id: string, settings: InspectorServerSettings) => { - updateServerSettings(id, settings).catch((err) => { - notifications.show({ - title: `Failed to save settings for "${id}"`, - message: err instanceof Error ? err.message : String(err), - color: "red", - }); + // The settings modal is fully controlled — every input change fires + // `onSettingsChange` back up here, and the input's `value` prop only + // updates when this component re-renders with a new `settings` prop. + // We hold the in-progress draft in `useSettingsDraft` so every change + // re-renders synchronously; the hook also debounces the PUT and + // exposes `flush` for the close handler to call. The draft is + // (re)initialized only when the modal opens to a *different* server, + // which is why a background refresh of `servers` can run without + // clobbering in-progress edits. + // + // `resolveInitial` reads `servers` from this render's closure — that + // works because the settings entry point is the "Settings" button on + // a rendered server card, so `servers` is always non-empty by the + // time this hook is called. A future caller that opens the modal + // from elsewhere (e.g. a keyboard shortcut on initial load) would + // need a different initialization path; the empty-shell fallback at + // least keeps the form renderable while `servers` hydrates. + const { + draft: settingsDraft, + onChange: onSettingsChange, + flush: flushSettingsDraft, + } = useSettingsDraft({ + targetId: settingsModalTargetId, + resolveInitial: (id) => + servers.find((s) => s.id === id)?.settings ?? EMPTY_SETTINGS, + onPersist: updateServerSettings, + // Surface failures via toast — the modal usually closes + // immediately on user dismiss, so a silent fail-on-flush would + // leave the user thinking their last edits saved when they + // didn't (especially painful for the OAuth client secret). + onError: (id, err) => { + notifications.show({ + title: `Failed to save settings for "${id}"`, + message: err instanceof Error ? err.message : String(err), + color: "red", }); }, - [updateServerSettings], - ); + }); - const onSettingsChange = useCallback( - (next: InspectorServerSettings) => { - if (!settingsModalTargetId) return; - // Update the displayed draft synchronously — this is the - // re-render that makes typed characters and added rows appear. - setSettingsDraft(next); - // Debounce the PUT. Persisting each character would run a tight - // PUT-then-full-refresh loop against the backend; coalesce a - // burst of edits into one PUT and flush on modal close. - if (settingsTimerRef.current) clearTimeout(settingsTimerRef.current); - const id = settingsModalTargetId; - settingsTimerRef.current = setTimeout(() => { - settingsTimerRef.current = undefined; - sendSettingsUpdate(id, next); - }, 300); - }, - [settingsModalTargetId, sendSettingsUpdate], - ); + const settingsModalValue: InspectorServerSettings = + settingsDraft ?? EMPTY_SETTINGS; const onSettingsModalClose = useCallback(() => { - // Flush any pending debounce synchronously so the last edits land - // even if the user dismisses before the 300ms timer fires. The - // useEffect above will null out the draft once the target id - // clears, so we don't need to touch it here. - if (settingsTimerRef.current) { - clearTimeout(settingsTimerRef.current); - settingsTimerRef.current = undefined; - if (settingsDraft && settingsModalTargetId) { - sendSettingsUpdate(settingsModalTargetId, settingsDraft); - } - } + flushSettingsDraft(); setSettingsModalTargetId(undefined); - }, [settingsDraft, settingsModalTargetId, sendSettingsUpdate]); - - // Cancel any pending debounce on unmount (route change / HMR). Without - // this a stale setTimeout could still fire one final `updateServerSettings` - // against an unmounted component — harmless today (just an extra PUT of - // the final payload) but the cleanest pattern is to clear the timer. - useEffect(() => { - return () => { - if (settingsTimerRef.current) { - clearTimeout(settingsTimerRef.current); - settingsTimerRef.current = undefined; - } - }; - }, []); + }, [flushSettingsDraft]); // The Resources screen needs `isSubscribed` to flip the Subscribe button // label to "Unsubscribe". Derive it from the live subscriptions list rather diff --git a/clients/web/src/test/core/react/useSettingsDraft.test.tsx b/clients/web/src/test/core/react/useSettingsDraft.test.tsx new file mode 100644 index 000000000..fff981725 --- /dev/null +++ b/clients/web/src/test/core/react/useSettingsDraft.test.tsx @@ -0,0 +1,326 @@ +/** + * Tests for `useSettingsDraft` — the draft-state hook the settings + * modal in App.tsx is built on top of. The behavior we pin here is the + * regression that #1361 fixed: every `onChange` must update the + * displayed value synchronously so a controlled `` doesn't + * appear to eat keystrokes. The PUT itself debounces. + */ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { act, renderHook } from "@testing-library/react"; +import { useSettingsDraft } from "@inspector/core/react/useSettingsDraft"; + +interface SettingsShape { + text: string; + rows: string[]; +} + +const EMPTY: SettingsShape = { text: "", rows: [] }; + +describe("useSettingsDraft", () => { + beforeEach(() => { + vi.useFakeTimers(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + it("starts with a null draft when no target id is set", () => { + const { result } = renderHook(() => + useSettingsDraft({ + targetId: undefined, + resolveInitial: () => EMPTY, + onPersist: vi.fn(), + onError: vi.fn(), + }), + ); + expect(result.current.draft).toBe(null); + }); + + it("seeds the draft via resolveInitial when target id appears", () => { + const resolveInitial = vi.fn((id: string) => ({ + text: `seed-${id}`, + rows: [], + })); + const { result, rerender } = renderHook( + ({ targetId }: { targetId: string | undefined }) => + useSettingsDraft({ + targetId, + resolveInitial, + onPersist: vi.fn(), + onError: vi.fn(), + }), + { initialProps: { targetId: undefined as string | undefined } }, + ); + expect(resolveInitial).not.toHaveBeenCalled(); + + rerender({ targetId: "alpha" }); + expect(resolveInitial).toHaveBeenCalledExactlyOnceWith("alpha"); + expect(result.current.draft).toEqual({ text: "seed-alpha", rows: [] }); + }); + + it("re-seeds when the target id changes (modal reopens to a different server)", () => { + const resolveInitial = vi.fn((id: string) => ({ + text: `seed-${id}`, + rows: [], + })); + const { result, rerender } = renderHook( + ({ targetId }: { targetId: string | undefined }) => + useSettingsDraft({ + targetId, + resolveInitial, + onPersist: vi.fn(), + onError: vi.fn(), + }), + { initialProps: { targetId: "alpha" as string | undefined } }, + ); + expect(result.current.draft?.text).toBe("seed-alpha"); + + rerender({ targetId: "beta" }); + expect(result.current.draft?.text).toBe("seed-beta"); + }); + + it("does NOT re-seed when only resolveInitial's closure changes mid-edit (no clobber on background refresh)", () => { + // This is the regression case the bug fix relies on: a background + // refresh of the server list re-renders the parent with a new + // `resolveInitial` closure (because `servers` changed), but the + // user's in-progress draft must not be reset. + let serversSnapshot: Record = { + alpha: { text: "from-server", rows: [] }, + }; + const { result, rerender } = renderHook(() => + useSettingsDraft({ + targetId: "alpha", + resolveInitial: (id) => serversSnapshot[id] ?? EMPTY, + onPersist: vi.fn(), + onError: vi.fn(), + }), + ); + expect(result.current.draft?.text).toBe("from-server"); + + // User types — draft diverges from server. + act(() => { + result.current.onChange({ text: "user-edit", rows: [] }); + }); + expect(result.current.draft?.text).toBe("user-edit"); + + // Background refresh: server's snapshot changes, parent re-renders. + serversSnapshot = { alpha: { text: "server-changed", rows: [] } }; + rerender(); + expect(result.current.draft?.text).toBe("user-edit"); + }); + + it("nulls the draft when the target id clears (modal closed)", () => { + const { result, rerender } = renderHook( + ({ targetId }: { targetId: string | undefined }) => + useSettingsDraft({ + targetId, + resolveInitial: () => EMPTY, + onPersist: vi.fn(), + onError: vi.fn(), + }), + { initialProps: { targetId: "alpha" as string | undefined } }, + ); + expect(result.current.draft).not.toBe(null); + + rerender({ targetId: undefined }); + expect(result.current.draft).toBe(null); + }); + + it("onChange updates draft synchronously (the regression #1361 was about)", () => { + const { result } = renderHook(() => + useSettingsDraft({ + targetId: "alpha", + resolveInitial: () => EMPTY, + onPersist: vi.fn(), + onError: vi.fn(), + }), + ); + expect(result.current.draft).toEqual(EMPTY); + + act(() => { + result.current.onChange({ text: "a", rows: [] }); + }); + expect(result.current.draft).toEqual({ text: "a", rows: [] }); + + act(() => { + result.current.onChange({ text: "ab", rows: [] }); + }); + expect(result.current.draft).toEqual({ text: "ab", rows: [] }); + }); + + it("debounces onPersist by the configured window", () => { + const onPersist = vi.fn(async () => {}); + const { result } = renderHook(() => + useSettingsDraft({ + targetId: "alpha", + resolveInitial: () => EMPTY, + onPersist, + onError: vi.fn(), + debounceMs: 300, + }), + ); + act(() => { + result.current.onChange({ text: "a", rows: [] }); + }); + expect(onPersist).not.toHaveBeenCalled(); + + act(() => { + vi.advanceTimersByTime(299); + }); + expect(onPersist).not.toHaveBeenCalled(); + + act(() => { + vi.advanceTimersByTime(1); + }); + expect(onPersist).toHaveBeenCalledExactlyOnceWith("alpha", { + text: "a", + rows: [], + }); + }); + + it("collapses a burst of onChange calls into one persist with the final value", () => { + const onPersist = vi.fn(async () => {}); + const { result } = renderHook(() => + useSettingsDraft({ + targetId: "alpha", + resolveInitial: () => EMPTY, + onPersist, + onError: vi.fn(), + }), + ); + act(() => { + result.current.onChange({ text: "a", rows: [] }); + vi.advanceTimersByTime(100); + result.current.onChange({ text: "ab", rows: [] }); + vi.advanceTimersByTime(100); + result.current.onChange({ text: "abc", rows: [] }); + }); + act(() => { + vi.advanceTimersByTime(300); + }); + expect(onPersist).toHaveBeenCalledExactlyOnceWith("alpha", { + text: "abc", + rows: [], + }); + }); + + it("ignores onChange when no target id is set", () => { + const onPersist = vi.fn(async () => {}); + const { result } = renderHook(() => + useSettingsDraft({ + targetId: undefined, + resolveInitial: () => EMPTY, + onPersist, + onError: vi.fn(), + }), + ); + act(() => { + result.current.onChange({ text: "stray", rows: [] }); + }); + act(() => { + vi.advanceTimersByTime(1000); + }); + expect(onPersist).not.toHaveBeenCalled(); + expect(result.current.draft).toBe(null); + }); + + it("flush() fires the pending persist synchronously and clears the timer", () => { + const onPersist = vi.fn(async () => {}); + const { result } = renderHook(() => + useSettingsDraft({ + targetId: "alpha", + resolveInitial: () => EMPTY, + onPersist, + onError: vi.fn(), + }), + ); + act(() => { + result.current.onChange({ text: "almost", rows: [] }); + }); + act(() => { + result.current.flush(); + }); + expect(onPersist).toHaveBeenCalledExactlyOnceWith("alpha", { + text: "almost", + rows: [], + }); + // The timer should be cleared — advancing past the debounce window + // must not produce a second persist. + act(() => { + vi.advanceTimersByTime(1000); + }); + expect(onPersist).toHaveBeenCalledTimes(1); + }); + + it("flush() is a no-op when nothing is pending", () => { + const onPersist = vi.fn(async () => {}); + const { result } = renderHook(() => + useSettingsDraft({ + targetId: "alpha", + resolveInitial: () => EMPTY, + onPersist, + onError: vi.fn(), + }), + ); + act(() => { + result.current.flush(); + }); + expect(onPersist).not.toHaveBeenCalled(); + }); + + it("invokes onError when onPersist rejects (modal-close failure path)", async () => { + const err = new Error("kaboom"); + const onPersist = vi.fn(async () => { + throw err; + }); + const onError = vi.fn(); + const { result } = renderHook(() => + useSettingsDraft({ + targetId: "alpha", + resolveInitial: () => EMPTY, + onPersist, + onError, + }), + ); + act(() => { + result.current.onChange({ text: "x", rows: [] }); + vi.advanceTimersByTime(300); + }); + // Let the rejected promise propagate to the .catch handler. + await act(async () => { + await Promise.resolve(); + }); + expect(onError).toHaveBeenCalledExactlyOnceWith("alpha", err); + }); + + it("a pending PUT for the previous target id still goes to that target after a switch", () => { + // setTimeout closes over `id` at schedule time, so an in-flight + // debounce for `alpha` resolves to `alpha` even if the user has + // since switched to `beta`. (Switching is fired from + // `onSettingsModalClose` which calls `flush()` first — but the + // contract should hold for any pending closure either way.) + const onPersist = vi.fn(async () => {}); + const { result, rerender } = renderHook( + ({ targetId }: { targetId: string | undefined }) => + useSettingsDraft({ + targetId, + resolveInitial: () => EMPTY, + onPersist, + onError: vi.fn(), + }), + { initialProps: { targetId: "alpha" as string | undefined } }, + ); + act(() => { + result.current.onChange({ text: "alpha-edit", rows: [] }); + }); + rerender({ targetId: "beta" }); + act(() => { + vi.advanceTimersByTime(300); + }); + expect(onPersist).toHaveBeenCalledExactlyOnceWith("alpha", { + text: "alpha-edit", + rows: [], + }); + }); +}); diff --git a/core/react/useSettingsDraft.ts b/core/react/useSettingsDraft.ts new file mode 100644 index 000000000..93731292f --- /dev/null +++ b/core/react/useSettingsDraft.ts @@ -0,0 +1,141 @@ +/** + * Draft state for the per-server "settings" modal. + * + * `ServerSettingsModal` is a fully controlled component: every input + * change fires back up to the parent, which must re-render with the new + * value or the input's `value` prop goes stale and characters appear to + * be eaten. We can't drive that re-render from the server-list state + * alone — the PUT round-trip is too slow for per-keystroke updates, and + * background refreshes of the server list would clobber in-progress + * edits. + * + * This hook holds the draft locally, debounces the PUT, and exposes a + * synchronous `flush` for the caller to invoke on modal close so the + * final keystrokes always land. Extracted out of `App.tsx` so the + * behavior is unit-testable in isolation (the App.tsx wiring is hard + * to drive from a React Testing Library harness because of its many + * Mantine + state-manager dependencies). + * + * Initialization is `targetId`-keyed by design: the draft only resets + * when the modal opens to a *different* server, not on every change to + * the underlying entry. That's what lets a background refresh of the + * server list run without losing the user's in-progress edits. + */ +import { useCallback, useEffect, useRef, useState } from "react"; + +export interface UseSettingsDraftOptions { + /** The id of the server whose settings are being edited, or `undefined` when the modal is closed. */ + targetId: string | undefined; + /** + * Resolves the initial draft for a given server id at the moment the + * modal opens. Called once per target — must be stable enough that + * the caller doesn't accidentally provoke re-initialization while + * the user is editing. (We do not include this function in the + * effect deps; it's read through a ref so callers don't have to + * `useCallback` it.) + */ + resolveInitial: (id: string) => T; + /** Persist the draft. Called from the debounced flush and from `flush`. */ + onPersist: (id: string, value: T) => Promise; + /** Invoked when `onPersist` rejects. */ + onError: (id: string, err: unknown) => void; + /** Debounce window in ms between the last `onChange` and the PUT. Defaults to 300. */ + debounceMs?: number; +} + +export interface UseSettingsDraftResult { + /** Current draft, or `null` when no target is selected (modal closed). */ + draft: T | null; + /** Update the draft. Schedules a debounced PUT. */ + onChange: (next: T) => void; + /** + * Flush any pending PUT synchronously (before yielding to the event + * loop). Use from the modal's close handler so a user-dismiss + * doesn't drop edits whose debounce timer hadn't fired yet. + */ + flush: () => void; +} + +/** + * @param targetId the currently selected server id; the draft re-initializes when this changes + * @param resolveInitial called once per `targetId` change to seed the draft + * @param onPersist invoked after the debounce window (and from `flush`) with the latest draft + * @param onError invoked when `onPersist` rejects + * @param debounceMs window between the last onChange and the PUT (default 300) + */ +export function useSettingsDraft({ + targetId, + resolveInitial, + onPersist, + onError, + debounceMs = 300, +}: UseSettingsDraftOptions): UseSettingsDraftResult { + const [draft, setDraft] = useState(null); + const timerRef = useRef | undefined>(undefined); + + // Read callbacks through refs so the consumer doesn't have to + // `useCallback` them. The effect that re-initializes the draft must + // depend on `targetId` only — anything else in its deps risks + // resetting an in-progress edit when the parent re-renders for + // unrelated reasons (e.g. a background server-list refresh). + const resolveInitialRef = useRef(resolveInitial); + const onPersistRef = useRef(onPersist); + const onErrorRef = useRef(onError); + resolveInitialRef.current = resolveInitial; + onPersistRef.current = onPersist; + onErrorRef.current = onError; + + useEffect(() => { + if (!targetId) { + setDraft(null); + return; + } + setDraft(resolveInitialRef.current(targetId)); + }, [targetId]); + + // Unmount cleanup — a stale timer firing after unmount is harmless + // today (the persist call still goes through to the backend) but + // belt-and-suspenders prevents an extra PUT on route change / HMR. + useEffect(() => { + return () => { + if (timerRef.current) { + clearTimeout(timerRef.current); + timerRef.current = undefined; + } + }; + }, []); + + const persist = useCallback((id: string, value: T) => { + onPersistRef.current(id, value).catch((err) => { + onErrorRef.current(id, err); + }); + }, []); + + const onChange = useCallback( + (next: T) => { + if (!targetId) return; + // Synchronous setState so the modal re-renders this tick — the + // bug this hook was extracted to fix was the input's controlled + // `value` prop going stale until the round-trip completed. + setDraft(next); + if (timerRef.current) clearTimeout(timerRef.current); + const id = targetId; + timerRef.current = setTimeout(() => { + timerRef.current = undefined; + persist(id, next); + }, debounceMs); + }, + [targetId, debounceMs, persist], + ); + + const flush = useCallback(() => { + if (!timerRef.current) return; + clearTimeout(timerRef.current); + timerRef.current = undefined; + if (targetId && draft !== null) { + persist(targetId, draft); + } + }, [targetId, draft, persist]); + + return { draft, onChange, flush }; +} From daa90f7b78cb58f5fa9d4de83b50ed797080e48a Mon Sep 17 00:00:00 2001 From: cliffhall Date: Tue, 26 May 2026 12:42:41 -0400 Subject: [PATCH 3/3] chore(servers): stabilize flush identity + add unmount-cleanup test (#1361) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two non-blocking observations from the third review on #1362: 1. `flush` identity churned on every keystroke because the useCallback closed over `draft` + `targetId`. Read both through refs instead so the returned callback identity is stable. The behavior is unchanged — but the consumer's `onSettingsModalClose` (which wraps `flush`) no longer re-allocates per keystroke, which means the modal's `onClose` prop identity also stays stable. A new test pins this. 2. Added a comment in `onChange` calling out that callers switching `targetId` mid-debounce must `flush()` first to preserve the pending PUT. The only switch path today (`onSettingsModalClose` in App.tsx) does that already — the comment is for future refactors. 3. New unmount-cleanup test pins the intentional "drop final --- .../test/core/react/useSettingsDraft.test.tsx | 52 +++++++++++++++++++ core/react/useSettingsDraft.ts | 33 +++++++++--- 2 files changed, 77 insertions(+), 8 deletions(-) diff --git a/clients/web/src/test/core/react/useSettingsDraft.test.tsx b/clients/web/src/test/core/react/useSettingsDraft.test.tsx index fff981725..aa34f2020 100644 --- a/clients/web/src/test/core/react/useSettingsDraft.test.tsx +++ b/clients/web/src/test/core/react/useSettingsDraft.test.tsx @@ -323,4 +323,56 @@ describe("useSettingsDraft", () => { rows: [], }); }); + + it("unmount clears the debounce timer — no PUT after the component is gone", () => { + // Documented contract: unmount mid-debounce drops the final + // {}); + const { result, unmount } = renderHook(() => + useSettingsDraft({ + targetId: "alpha", + resolveInitial: () => EMPTY, + onPersist, + onError: vi.fn(), + }), + ); + act(() => { + result.current.onChange({ text: "doomed", rows: [] }); + }); + unmount(); + act(() => { + vi.advanceTimersByTime(1000); + }); + expect(onPersist).not.toHaveBeenCalled(); + }); + + it("flush identity is stable across keystrokes (no churn on consumer's onClose)", () => { + // Without ref-reading draft + targetId inside flush, every + // `setDraft` would change `flush`'s deps and produce a new + // identity. That cascades to App.tsx's `onSettingsModalClose`, + // which in turn re-allocates the modal's `onClose` prop on every + // character typed. Harmless in practice but the useCallback + // would be doing no work. + const { result } = renderHook(() => + useSettingsDraft({ + targetId: "alpha", + resolveInitial: () => EMPTY, + onPersist: vi.fn(), + onError: vi.fn(), + }), + ); + const flushAfterMount = result.current.flush; + act(() => { + result.current.onChange({ text: "a", rows: [] }); + }); + act(() => { + result.current.onChange({ text: "ab", rows: [] }); + }); + act(() => { + result.current.onChange({ text: "abc", rows: [] }); + }); + expect(result.current.flush).toBe(flushAfterMount); + }); }); diff --git a/core/react/useSettingsDraft.ts b/core/react/useSettingsDraft.ts index 93731292f..186932dac 100644 --- a/core/react/useSettingsDraft.ts +++ b/core/react/useSettingsDraft.ts @@ -73,17 +73,23 @@ export function useSettingsDraft({ const [draft, setDraft] = useState(null); const timerRef = useRef | undefined>(undefined); - // Read callbacks through refs so the consumer doesn't have to - // `useCallback` them. The effect that re-initializes the draft must - // depend on `targetId` only — anything else in its deps risks - // resetting an in-progress edit when the parent re-renders for - // unrelated reasons (e.g. a background server-list refresh). + // Read callbacks (and the current draft) through refs so the consumer + // doesn't have to `useCallback` them, and so the returned `flush` + // identity is stable across keystrokes. The effect that + // re-initializes the draft must depend on `targetId` only — anything + // else in its deps risks resetting an in-progress edit when the + // parent re-renders for unrelated reasons (e.g. a background + // server-list refresh). const resolveInitialRef = useRef(resolveInitial); const onPersistRef = useRef(onPersist); const onErrorRef = useRef(onError); + const draftRef = useRef(null); + const targetIdRef = useRef(targetId); resolveInitialRef.current = resolveInitial; onPersistRef.current = onPersist; onErrorRef.current = onError; + draftRef.current = draft; + targetIdRef.current = targetId; useEffect(() => { if (!targetId) { @@ -118,6 +124,11 @@ export function useSettingsDraft({ // bug this hook was extracted to fix was the input's controlled // `value` prop going stale until the round-trip completed. setDraft(next); + // Cancel any pending debounce — callers that switch `targetId` + // mid-debounce must call `flush()` first if they want the prior + // target's edits to land. The only switch path today + // (`onSettingsModalClose` in App.tsx) does exactly that, so the + // window is closed in practice. if (timerRef.current) clearTimeout(timerRef.current); const id = targetId; timerRef.current = setTimeout(() => { @@ -128,14 +139,20 @@ export function useSettingsDraft({ [targetId, debounceMs, persist], ); + // Read `targetId` and `draft` through refs so this callback's + // identity is stable across keystrokes. Without that, the parent's + // `onClose` prop (which closes over `flush`) churns on every + // `setDraft`, which churns the modal's prop identity. const flush = useCallback(() => { if (!timerRef.current) return; clearTimeout(timerRef.current); timerRef.current = undefined; - if (targetId && draft !== null) { - persist(targetId, draft); + const id = targetIdRef.current; + const value = draftRef.current; + if (id && value !== null) { + persist(id, value); } - }, [targetId, draft, persist]); + }, [persist]); return { draft, onChange, flush }; }