* 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>
This commit is contained in:
Cliff Hall
GitHub
parent
037225a9fb
commit
f6ac075f24
1 changed files with 588 additions and 76 deletions
378
clients/web/src/test/core/react/useSettingsDraft.test.tsx
Normal file
378
clients/web/src/test/core/react/useSettingsDraft.test.tsx
Normal file
|
|
@ -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 `<input>` 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<SettingsShape>({
|
||||
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<SettingsShape>({
|
||||
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<SettingsShape>({
|
||||
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<string, SettingsShape> = {
|
||||
alpha: { text: "from-server", rows: [] },
|
||||
};
|
||||
const { result, rerender } = renderHook(() =>
|
||||
useSettingsDraft<SettingsShape>({
|
||||
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<SettingsShape>({
|
||||
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<SettingsShape>({
|
||||
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<SettingsShape>({
|
||||
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<SettingsShape>({
|
||||
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<SettingsShape>({
|
||||
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<SettingsShape>({
|
||||
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<SettingsShape>({
|
||||
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<SettingsShape>({
|
||||
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<SettingsShape>({
|
||||
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
|
||||
// <debounceMs window of edits. Flushing on unmount could fire a
|
||||
// PUT against an unmounting component, which is a worse footgun
|
||||
// than losing a few hundred ms of typing during route change / HMR.
|
||||
const onPersist = vi.fn(async () => {});
|
||||
const { result, unmount } = renderHook(() =>
|
||||
useSettingsDraft<SettingsShape>({
|
||||
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<SettingsShape>({
|
||||
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);
|
||||
});
|
||||
});
|
||||
Loading…
Reference in a new issue