Convert eligible ref-mirror effects to useEffectEvent (bd-t18384ia)

claude-notes/plans/2026-10-24-hub-client-useeffect-refactor.md

@@ -2,7 +2,7 @@
 title: 'hub-client `useEffect` audit: phased refactor'
 date: 2026-10-24
 description: 'Phased refactor of the refactor-worthy useEffect sites in hub-client: stale-dep fixes, useSyncExternalStore conversions, derived-state elimination, then measured wide-dep performance work.'
-status: in-progress  # Phases 1–4 done and open as stacked PRs (#924, #925, #931, #936). Phase 5 not started.
+status: in-progress  # All 5 phases done, open as stacked PRs (#924, #925, #931, #936, #938).
 ---
 
 ## Overview
@@ -396,16 +396,60 @@ or moving invalidation into the sync-service callback.
 
 ## Phase 5 — `useEffectEvent` sweep (optional, low priority)
 
-- [ ] Replace the ~12 ref-mirror effects
-  (`useEffect(() => { ref.current = handler })`) where the ref exists only to
-  give an effect or listener the latest handler: `useAuthProbe.ts:44`,
-  `useSessionKeepAlive.ts:51`, `useWindowFileDrop.ts:22`, `useRouting.ts:80`,
-  `Editor.tsx:379`, `Editor.tsx:569`, `Editor.tsx:1400`, `Preview.tsx:198`,
-  `ReactPreview.tsx:514`, `useSlideThumbnails.tsx:182`,
-  `useDownloadAs.ts:43` and `:47`.
-- [ ] Leave alone any effect that intentionally runs on every render unless
-  `useEffectEvent` semantics match exactly; convert only where the
-  replacement is behavior-preserving.
+**Done 2026-10-27 (bd-t18384ia); open as PR #938, stacked on #936.** The sweep triaged all twelve sites and
+converted three; the other nine fail the `useEffectEvent` contract, which
+eslint-plugin-react-hooks v7 enforces lexically: an effect-event variable may
+only be *referenced* from inside an effect (or another effect event) in the
+same component — "It cannot be assigned to a variable or passed down" — and
+React throws if one is called during render. The nine ref-mirrors that stay
+are precisely the cases refs exist for: the read site is a render-scope
+callback, a state updater, or a function passed across a hook boundary.
+
+- [x] Converted (call sites lexically inside an effect — semantics
+  identical):
+  - `useAuthProbe.ts:44` — `onAuthRejected` → effect event called from the
+    probe interval.
+  - `useSessionKeepAlive.ts:51` — both callbacks.
+  - `useWindowFileDrop.ts:22` — the drop listener's async ingest
+    continuation. (React 19.2's `mountEvent` keeps the last committed impl
+    after unmount with no guard, so an in-flight `collectDroppedEntries`
+    resolving post-unmount behaves exactly as the ref did.)
+- [x] Left as ref-mirrors (effect-event semantics do not match):
+  - `useRouting.ts:80` — read inside `setRoute` updaters, which run during
+    render; effect events throw when called during render.
+  - `useDownloadAs.ts:43`/`:47` — read inside `start`, a click handler.
+  - `Preview.tsx:198` / `ReactPreview.tsx:514` — `previewState` read inside
+    `doRenderWithStateManagement`, a render-scope `useCallback`.
+  - `useSlideThumbnails.tsx:182` — recursive re-invocation from inside the
+    `captureThumbnails` `useCallback`.
+  - `Editor.tsx:379` (`currentFilePathRef`) — `getCurrentFilePath` is passed
+    down to `useIntelligenceProviders` and other hooks; effect events cannot
+    be passed down.
+  - `Editor.tsx:569` (`getSlideForLineRef`) — read inside
+    `handleEditorMount`, a render-scope callback handed to Monaco's
+    `onMount`.
+  - `Editor.tsx:1400` (`editorDndHandlersRef`) — the stable wrappers are
+    attached/detached inside `handleEditorMount` and an unmount effect, both
+    outside lexical effect scope.
+- [x] Characterization tests pinned before converting:
+  `useAuthProbe`/`useSessionKeepAlive` each gained a "latest callback without
+  re-arming the interval" test (green on the unmodified hooks, still green
+  after); `useWindowFileDrop` already pinned that behavior.
+- Verification: `npm run test:ci` all three legs green (unit + integration +
+  WASM); `npm run build:all` green (production `tsc -b && vite build` plus
+  sw-precache check); eslint clean on the five touched files
+  (`rules-of-hooks` accepts all three effect events). Browser-smoked
+  2026-10-27, both converted paths green end-to-end:
+  - File drop (`useWindowFileDrop`): 13/13 `import-document.spec.ts` tests
+    on chromium, including the four window-fallback drops (top bar, dialog
+    backdrop, preview pane docx, preview pane csv).
+  - Auth probe (`useAuthProbe`): a throwaway Playwright spec on the auth-on
+    e2e stack (mock OIDC, deleted after use) drove the live path — signed
+    in, project open, networking severed: the offline pill appeared, two
+    `/auth/me` probes fired through the converted effect event, no logout
+    occurred (network-error invariant), and reconnecting restored the
+    online pill. `useSessionKeepAlive`\'s 1 h cadence is unobservable in a
+    smoke; its mechanism is identical to the probe\'s and unit-pinned.
 
 ## Out of scope
 

hub-client/src/hooks/useAuthProbe.test.tsx

@@ -120,4 +120,29 @@ describe('useAuthProbe', () => {
     });
     expect(mockFetchAuthMe).toHaveBeenCalledTimes(calls);
   });
+
+  it('calls the latest onAuthRejected without re-arming the interval', async () => {
+    // Characterization for the latest-callback mechanism (ref mirror, later
+    // useEffectEvent): a mid-stream callback swap must neither lose the
+    // strike count (no effect re-run) nor call the stale callback.
+    const first = vi.fn();
+    const second = vi.fn();
+    mockFetchAuthMe.mockResolvedValue(null);
+    const { rerender } = renderHook(
+      ({ cb }: { cb: () => void }) => useAuthProbe({ enabled: true, onAuthRejected: cb }),
+      { initialProps: { cb: first } },
+    );
+    await act(async () => {
+      await vi.advanceTimersByTimeAsync(0);
+    });
+    // Strike 1 recorded. Swap the callback; the strike count must survive
+    // (a re-armed interval would reset it and probe immediately).
+    rerender({ cb: second });
+    await act(async () => {
+      await vi.advanceTimersByTimeAsync(AUTH_PROBE_INTERVAL_MS);
+    });
+    expect(mockFetchAuthMe).toHaveBeenCalledTimes(2);
+    expect(first).not.toHaveBeenCalled();
+    expect(second).toHaveBeenCalledTimes(1);
+  });
 });

hub-client/src/hooks/useAuthProbe.ts

@@ -24,7 +24,7 @@
  * the first strike once triggered was retired in bd-s042qcxj.
  */
 
-import { useEffect, useRef } from 'react';
+import { useEffect, useEffectEvent } from 'react';
 import { fetchAuthMe } from '../services/authService';
 
 /** Interval between probes while disconnected. */
@@ -38,12 +38,9 @@ interface AuthProbeOpts {
 }
 
 export function useAuthProbe({ enabled, onAuthRejected }: AuthProbeOpts) {
-  // Keep the latest callback in a ref so the probe effect can key on
-  // `enabled` alone without re-arming the interval when it changes.
-  const onAuthRejectedRef = useRef(onAuthRejected);
-  useEffect(() => {
-    onAuthRejectedRef.current = onAuthRejected;
-  });
+  // The probe effect keys on `enabled` alone; the effect event reaches the
+  // latest callback without re-arming the interval when it changes.
+  const onAuthRejectedEvent = useEffectEvent(onAuthRejected);
 
   useEffect(() => {
     if (!enabled) return;
@@ -61,7 +58,7 @@ export function useAuthProbe({ enabled, onAuthRejected }: AuthProbeOpts) {
         }
         strikes += 1;
         if (strikes >= 2) {
-          onAuthRejectedRef.current();
+          onAuthRejectedEvent();
         }
         // strike 1 is a no-op: record it and give the next cycle a chance
         // to confirm (or clear) the rejection before logging out.

hub-client/src/hooks/useSessionKeepAlive.test.tsx

@@ -121,4 +121,31 @@ describe('useSessionKeepAlive', () => {
     });
     expect(mockFetchAuthMe).toHaveBeenCalledTimes(1);
   });
+
+  it('calls the latest onAuthState without re-arming the interval', async () => {
+    // Characterization for the latest-callback mechanism (ref mirror, later
+    // useEffectEvent): a mid-stream callback swap must hit the new callback
+    // and must not re-run the effect (a re-arm would probe immediately,
+    // adding a third fetch).
+    const firstState = vi.fn();
+    const secondState = vi.fn();
+    mockFetchAuthMe.mockResolvedValue(me(1_000_000));
+    const { rerender } = renderHook(
+      ({ cb }: { cb: typeof onAuthState }) =>
+        useSessionKeepAlive({ enabled: true, onAuthState: cb, onAuthRejected }),
+      { initialProps: { cb: firstState } },
+    );
+    await act(async () => {
+      await vi.advanceTimersByTimeAsync(0);
+    });
+    expect(firstState).toHaveBeenCalledTimes(1);
+
+    rerender({ cb: secondState });
+    await act(async () => {
+      await vi.advanceTimersByTimeAsync(SESSION_KEEP_ALIVE_INTERVAL_MS);
+    });
+    expect(mockFetchAuthMe).toHaveBeenCalledTimes(2);
+    expect(firstState).toHaveBeenCalledTimes(1);
+    expect(secondState).toHaveBeenCalledTimes(1);
+  });
 });

hub-client/src/hooks/useSessionKeepAlive.ts

@@ -23,7 +23,7 @@
  * instead.
  */
 
-import { useEffect, useRef } from 'react';
+import { useEffect, useEffectEvent } from 'react';
 import type { AuthState } from '../services/authService';
 import { fetchAuthMe } from '../services/authService';
 
@@ -45,13 +45,10 @@ interface SessionKeepAliveOpts {
 }
 
 export function useSessionKeepAlive({ enabled, onAuthState, onAuthRejected }: SessionKeepAliveOpts) {
-  // Latest callbacks in refs so the interval keys on `enabled` alone.
-  const onAuthStateRef = useRef(onAuthState);
-  const onAuthRejectedRef = useRef(onAuthRejected);
-  useEffect(() => {
-    onAuthStateRef.current = onAuthState;
-    onAuthRejectedRef.current = onAuthRejected;
-  });
+  // The interval keys on `enabled` alone; the effect events reach the latest
+  // callbacks without re-arming it.
+  const onAuthStateEvent = useEffectEvent(onAuthState);
+  const onAuthRejectedEvent = useEffectEvent(onAuthRejected);
 
   useEffect(() => {
     if (!enabled) return;
@@ -63,9 +60,9 @@ export function useSessionKeepAlive({ enabled, onAuthState, onAuthRejected }: Se
         const me = await fetchAuthMe();
         if (cancelled) return;
         if (me) {
-          onAuthStateRef.current(me);
+          onAuthStateEvent(me);
         } else {
-          onAuthRejectedRef.current();
+          onAuthRejectedEvent();
         }
       } catch {
         // Network error / unreachable hub — no evidence, no action.

hub-client/src/hooks/useWindowFileDrop.ts

@@ -11,17 +11,15 @@
  * drags are left alone, and it skips events a component handler already took (`defaultPrevented`).
  * It registers nothing unless `enabled` (import is offered: not the `q2 preview` embed).
  */
-import { useEffect, useRef } from 'react';
+import { useEffect, useEffectEvent } from 'react';
 import { collectDroppedEntries, type DroppedEntries } from '../utils/droppedEntries';
 
 const carriesFiles = (e: DragEvent): boolean => !e.defaultPrevented && !!e.dataTransfer && Array.from(e.dataTransfer.types).includes('Files');
 
 export function useWindowFileDrop({ enabled, onDrop }: { enabled: boolean; onDrop: (entries: DroppedEntries) => void }): void {
-  // The listeners are attached once per `enabled`; they call the latest handler.
-  const handler = useRef(onDrop);
-  useEffect(() => {
-    handler.current = onDrop;
-  }, [onDrop]);
+  // The listeners are attached once per `enabled`; the effect event reaches
+  // the latest handler.
+  const onDropLatest = useEffectEvent(onDrop);
 
   useEffect(() => {
     if (!enabled) return;
@@ -36,7 +34,7 @@ export function useWindowFileDrop({ enabled, onDrop }: { enabled: boolean; onDro
       e.preventDefault();
       // The DataTransfer is only readable during the event: start walking it now.
       void collectDroppedEntries(e.dataTransfer!).then((entries) => {
-        if (entries.files.length > 0 || entries.folders.length > 0) handler.current(entries);
+        if (entries.files.length > 0 || entries.folders.length > 0) onDropLatest(entries);
       });
     };
     document.addEventListener('dragover', onDragOver);