* 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>
158 lines
6.2 KiB
TypeScript
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 };
|
|
}
|