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>

Author: cliffhall

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
+    // <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);
+  });
 });

core/react/useSettingsDraft.ts

@@ -73,17 +73,23 @@ export function useSettingsDraft<T>({
   const [draft, setDraft] = useState<T | null>(null);
   const timerRef = useRef<ReturnType<typeof setTimeout> | 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<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) {
@@ -118,6 +124,11 @@ export function useSettingsDraft<T>({
       // 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<T>({
     [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 };
 }