inspector/core/react/useSettingsDraft.ts
Cliff Hall f6ac075f24
fix(servers): make settings form re-render on every keystroke (#1361) (#1362)
* fix(servers): make settings form re-render on every keystroke (#1361)

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) <noreply@anthropic.com>

* chore(servers): extract useSettingsDraft hook + tests; review polish (#1361)

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) <noreply@anthropic.com>

* chore(servers): stabilize flush identity + add unmount-cleanup test (#1361)

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
   <debounceMs window on unmount" trade-off — flushing on unmount
   could fire a PUT against an unmounting component, which is a
   worse footgun than losing a few hundred ms of typing on route
   change / HMR.

Validate clean: 144 files, 1758 tests pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-26 13:42:14 -04:00

158 lines
6.2 KiB
TypeScript

/**
* 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<T> {
/** 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<void>;
/** 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<T> {
/** 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<T>({
targetId,
resolveInitial,
onPersist,
onError,
debounceMs = 300,
}: UseSettingsDraftOptions<T>): UseSettingsDraftResult<T> {
const [draft, setDraft] = useState<T | null>(null);
const timerRef = useRef<ReturnType<typeof setTimeout> | 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<T | null>(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 };
}