diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index d15610af5..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,90 +777,52 @@ 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 ?? { - 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); - - 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) => { + // 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 "${pending.id}"`, + 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; - pendingSettingsRef.current = { - id: settingsModalTargetId, - settings: next, - }; - if (pendingSettingsTimerRef.current) { - clearTimeout(pendingSettingsTimerRef.current); - } - pendingSettingsTimerRef.current = setTimeout(flushPendingSettings, 300); }, - [settingsModalTargetId, flushPendingSettings], - ); + }); + + const settingsModalValue: InspectorServerSettings = + settingsDraft ?? EMPTY_SETTINGS; const onSettingsModalClose = useCallback(() => { - flushPendingSettings(); + flushSettingsDraft(); setSettingsModalTargetId(undefined); - }, [flushPendingSettings]); - - // 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 (pendingSettingsTimerRef.current) { - clearTimeout(pendingSettingsTimerRef.current); - pendingSettingsTimerRef.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..aa34f2020 --- /dev/null +++ b/clients/web/src/test/core/react/useSettingsDraft.test.tsx @@ -0,0 +1,378 @@ +/** + * 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: [], + }); + }); + + 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 new file mode 100644 index 000000000..186932dac --- /dev/null +++ b/core/react/useSettingsDraft.ts @@ -0,0 +1,158 @@ +/** + * 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 (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) { + 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); + // 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(() => { + timerRef.current = undefined; + persist(id, next); + }, debounceMs); + }, + [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; + const id = targetIdRef.current; + const value = draftRef.current; + if (id && value !== null) { + persist(id, value); + } + }, [persist]); + + return { draft, onChange, flush }; +}