refactor(#289 review): extract useScrollRestoreOnSwap so the test guards real code (F2)

The prior test guarded a verbatim MIRROR of the two scroll-restore useLayoutEffect
blocks — the reviewer proved removing '&& editor' from the real page-editor.tsx left
the test green (a copy, not the original). Extract the wiring into an exported
useScrollRestoreOnSwap(pageId, editor, showStatic) hook (the two effects verbatim +
useScrollPosition inside; F1 budget logic untouched), call it once from page-editor.tsx
(replacing the removed useScrollPosition call + both effects), and rewrite the test to
render the REAL hook — deleting the mirror and the false 'regresses in lockstep' comment
(F2-doc). Non-vacuity proven: removing '&& editor' from the real hook reddens the guard
test.

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

apps/client/src/features/editor/hooks/use-scroll-position.ts

@@ -1,4 +1,5 @@
-import { useCallback, useEffect, useRef } from "react";
+import { useCallback, useEffect, useLayoutEffect, useRef } from "react";
+import type { Editor } from "@tiptap/react";
 
 // Throttle interval for persisting the scroll position while the user reads.
 const SAVE_THROTTLE_MS = 250;
@@ -243,3 +244,37 @@ export function useScrollPosition(pageId: string): {
 
   return { restoreScrollPosition };
 }
+
+/**
+ * Wires `useScrollPosition` to the page editor's static->live swap lifecycle.
+ *
+ * Extracted from PageEditor so the exact restore triggers (their deps and the
+ * post-swap `&& editor` guard) are directly unit-testable rather than mirrored.
+ * Behaviour is unchanged: `restoreScrollPosition` is idempotent, so re-asserting
+ * the same target from either trigger is a no-op.
+ *
+ * @param pageId      the page whose scroll position is persisted/restored.
+ * @param editor      the tiptap editor instance, or `null` until it is ready.
+ * @param showStatic  whether the static (cached) content is still shown.
+ */
+export function useScrollRestoreOnSwap(
+  pageId: string,
+  editor: Editor | null,
+  showStatic: boolean,
+): void {
+  const { restoreScrollPosition } = useScrollPosition(pageId);
+
+  // Restore as early as the static (cached) content is laid out, before paint,
+  // so the reader's position is applied without a visible jump. Aborts itself if
+  // the reader has already started scrolling (handled inside the hook).
+  useLayoutEffect(() => {
+    restoreScrollPosition();
+  }, [restoreScrollPosition]);
+
+  // Re-assert once after the static -> live editor swap in case the swap reset
+  // the window scroll. Idempotent: a no-op when the position is already correct,
+  // and a no-op after the reader has interacted.
+  useLayoutEffect(() => {
+    if (!showStatic && editor) restoreScrollPosition();
+  }, [showStatic, editor, restoreScrollPosition]);
+}

apps/client/src/features/editor/page-editor.test.tsx

@@ -1,15 +1,16 @@
 import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
 import { render, act } from "@testing-library/react";
-import { useLayoutEffect, useState } from "react";
+import type { Editor } from "@tiptap/react";
-import { useScrollPosition } from "./hooks/use-scroll-position";
+import { useScrollRestoreOnSwap } from "./hooks/use-scroll-position";
 
 const KEY_PREFIX = "gitmost:scroll-position:";
 
 // NOTE ON SCOPE (F2 — reviewer-approved lighter variant).
 //
-// The real UX wiring lives in page-editor.tsx as two useLayoutEffects around the
+// The real UX wiring lives in the exported `useScrollRestoreOnSwap` hook (two
-// useScrollPosition hook. A FULL PageEditor component test is impractical here and
+// useLayoutEffects around useScrollPosition), which PageEditor calls with the
-// has no precedent in this client: PageEditor directly constructs a
+// same signature. A FULL PageEditor component test is impractical here and has no
+// precedent in this client: PageEditor directly constructs a
 // HocuspocusProviderWebsocket + IndexeddbPersistence, a tiptap `useEditor` with
 // collab extensions, reads jotai atoms, react-router params, the shared
 // `queryClient` from main.tsx, i18n, and mounts ~12 editor menu children. Worse,
@@ -20,35 +21,17 @@ const KEY_PREFIX = "gitmost:scroll-position:";
 // single leaf component with ONE mocked query; nothing mounts a feature root of
 // this weight. Reproducing all of that would test the mocks, not the wiring.
 //
-// So this file tests the same integration at the level that carries the real
+// So this file tests the REAL `useScrollRestoreOnSwap` hook — the exact code
-// contract: the two useLayoutEffect blocks are reproduced VERBATIM from
+// PageEditor imports and calls — driving its `showStatic`/`editor` inputs the way
-// page-editor.tsx (the early pre-paint restore, and the post-swap re-assert with
+// the swap does. Because it exercises the real hook (not a copy), dropping the
-// deps [showStatic, editor]) and exercised against the REAL useScrollPosition
+// `&& editor` guard or changing the effect deps makes these tests fail; they
-// hook. If page-editor's wiring regresses (e.g. the swap effect drops the
+// guard the production code directly (verified: removing `&& editor` reddens the
-// `&& editor` guard or its deps), the mirror below regresses in lockstep.
+// first test).
-
+//
-// Mirror of page-editor.tsx lines ~489-498 (the two scroll-restore useLayoutEffects).
+// Both tests observe the real effect via `window.scrollTo`. The stubbed
-function ScrollRestoreWiring({
+// `window.scrollTo` never mutates `window.scrollY`, and the target is left
-  restoreScrollPosition,
+// unreached, so every restore invocation that passes the guard yields exactly one
-  showStatic,
+// `scrollTo` call — making the call count a faithful proxy for restore invocations.
-  editor,
-}: {
-  restoreScrollPosition: () => void;
-  showStatic: boolean;
-  editor: unknown | null;
-}) {
-  // Restore as early as the static (cached) content is laid out, before paint.
-  useLayoutEffect(() => {
-    restoreScrollPosition();
-  }, [restoreScrollPosition]);
-
-  // Re-assert once after the static -> live editor swap.
-  useLayoutEffect(() => {
-    if (!showStatic && editor) restoreScrollPosition();
-  }, [showStatic, editor, restoreScrollPosition]);
-
-  return null;
-}
 
 function setScrollY(value: number): void {
   Object.defineProperty(window, "scrollY", { configurable: true, value });
@@ -63,7 +46,25 @@ function setInnerHeight(value: number): void {
   Object.defineProperty(window, "innerHeight", { configurable: true, value });
 }
 
-describe("PageEditor scroll-restore wiring (two useLayoutEffects)", () => {
+// Minimal stand-in for the tiptap editor: the hook only truthiness-checks it.
+const fakeEditor = { id: "editor" } as unknown as Editor;
+
+// Thin host that calls the REAL hook so a rerender drives showStatic/editor
+// exactly like the page-editor swap does.
+function Host({
+  pageId,
+  showStatic,
+  editor,
+}: {
+  pageId: string;
+  showStatic: boolean;
+  editor: Editor | null;
+}) {
+  useScrollRestoreOnSwap(pageId, editor, showStatic);
+  return null;
+}
+
+describe("PageEditor scroll-restore wiring (useScrollRestoreOnSwap)", () => {
   beforeEach(() => {
     window.sessionStorage.clear();
     setScrollY(0);
@@ -79,81 +80,52 @@ describe("PageEditor scroll-restore wiring (two useLayoutEffects)", () => {
     window.location.hash = "";
   });
 
-  it("re-invokes restoreScrollPosition after the swap, with the [showStatic, editor] deps", () => {
+  it("re-invokes restore after the swap, with the [showStatic, editor] deps/guard", () => {
-    // A referentially STABLE spy, mirroring page-editor where restoreScrollPosition
+    // Target is immediately reachable, so each restore that passes the guard
-    // is a useCallback([]) — so the early effect (dep [restoreScrollPosition]) runs
+    // scrolls synchronously. `window.scrollY` stays 0 (stubbed scrollTo never
-    // exactly once and does NOT re-fire on every render.
+    // updates it), so scrollTo is called once per effective restore — a proxy for
-    const restore = vi.fn();
+    // the restore invocation count.
-
+    window.sessionStorage.setItem(`${KEY_PREFIX}guard`, "500");
-    const editor = { id: "editor" };
+    setInnerHeight(800);
-
+    setScrollHeight(2000); // maxScroll = 1200 >= 500: reachable, no polling.
-    // Host owns the swap state so we can drive showStatic/editor like page-editor.
-    function Host({
-      showStatic,
-      editorValue,
-    }: {
-      showStatic: boolean;
-      editorValue: unknown | null;
-    }) {
-      return (
-        <ScrollRestoreWiring
-          restoreScrollPosition={restore}
-          showStatic={showStatic}
-          editor={editorValue}
-        />
-      );
-    }
 
     // Pre-swap: static content shown, live editor not ready. Only the early
     // pre-paint restore fires; the post-swap effect's guard (!showStatic) blocks it.
-    const { rerender } = render(<Host showStatic={true} editorValue={null} />);
+    const { rerender } = render(
-    expect(restore).toHaveBeenCalledTimes(1);
+      <Host pageId="guard" showStatic={true} editor={null} />,
+    );
+    expect(window.scrollTo).toHaveBeenCalledTimes(1);
 
     // Collab reports synced (showStatic flips false) but the editor is not ready
-    // yet: the swap effect runs but the `&& editor` guard must keep it a no-op.
+    // yet: the swap effect re-runs (deps [showStatic, editor] changed) but the
-    // (Pins the guard: dropping `&& editor` would restore against a null editor.)
+    // `&& editor` guard must keep it a no-op. The early effect does NOT re-fire
-    rerender(<Host showStatic={false} editorValue={null} />);
+    // (its dep [restoreScrollPosition] is a stable useCallback([])).
-    expect(restore).toHaveBeenCalledTimes(1);
+    // (Pins the guard: dropping `&& editor` would restore against a null editor,
+    // producing a 2nd scrollTo and failing this expectation.)
+    rerender(<Host pageId="guard" showStatic={false} editor={null} />);
+    expect(window.scrollTo).toHaveBeenCalledTimes(1);
 
     // The static -> live swap completes (showStatic false AND editor present): the
-    // post-swap effect re-asserts the restore exactly once more. The early effect
+    // post-swap effect re-asserts the restore exactly once more, driven solely by
-    // does NOT re-fire (restore identity is stable), so this second call is driven
+    // the [showStatic, editor] deps changing.
-    // solely by the [showStatic, editor] deps changing.
+    rerender(<Host pageId="guard" showStatic={false} editor={fakeEditor} />);
-    rerender(<Host showStatic={false} editorValue={editor} />);
+    expect(window.scrollTo).toHaveBeenCalledTimes(2);
-    expect(restore).toHaveBeenCalledTimes(2);
   });
 
   it("the post-swap re-assert drives a REAL restore (window.scrollTo) via the hook", () => {
-    // End-to-end through the real useScrollPosition: the swap re-invocation is the
+    // End-to-end through the real useScrollPosition (inside the hook): the swap
-    // CAUSE of the scroll (nothing scrolls before it).
+    // re-invocation is the CAUSE of the scroll (nothing scrolls before it).
     vi.useFakeTimers();
     window.sessionStorage.setItem(`${KEY_PREFIX}peg`, "500");
     setInnerHeight(800);
     setScrollHeight(100); // maxScroll = -700: target not reachable yet -> polls.
 
-    const editor = { id: "editor" };
-
-    function Host({
-      showStatic,
-      editorValue,
-    }: {
-      showStatic: boolean;
-      editorValue: unknown | null;
-    }) {
-      const { restoreScrollPosition } = useScrollPosition("peg");
-      return (
-        <ScrollRestoreWiring
-          restoreScrollPosition={restoreScrollPosition}
-          showStatic={showStatic}
-          editor={editorValue}
-        />
-      );
-    }
-
     // Pre-swap: the early restore runs but content is too short, so it starts
     // polling (a pending timer) without scrolling. We never advance timers, so the
     // early poll cannot fire on its own — isolating the swap as the sole cause.
-    const { rerender } = render(<Host showStatic={true} editorValue={null} />);
+    const { rerender } = render(
+      <Host pageId="peg" showStatic={true} editor={null} />,
+    );
     expect(window.scrollTo).not.toHaveBeenCalled();
 
     // The live content is now laid out tall enough to reach the target.
@@ -162,7 +134,7 @@ describe("PageEditor scroll-restore wiring (two useLayoutEffects)", () => {
     // The static -> live swap: the post-swap useLayoutEffect re-invokes the real
     // hook, whose synchronous tryRestore now reaches the target and scrolls.
     act(() => {
-      rerender(<Host showStatic={false} editorValue={editor} />);
+      rerender(<Host pageId="peg" showStatic={false} editor={fakeEditor} />);
     });
     expect(window.scrollTo).toHaveBeenCalledWith({ top: 500, behavior: "auto" });
   });

apps/client/src/features/editor/page-editor.tsx

@@ -2,7 +2,6 @@ import "@/features/editor/styles/index.css";
 import React, {
   useCallback,
   useEffect,
-  useLayoutEffect,
   useMemo,
   useRef,
   useState,
@@ -79,7 +78,7 @@ import { PageEditMode } from "@/features/user/types/user.types.ts";
 import { jwtDecode } from "jwt-decode";
 import { searchSpotlight } from "@/features/search/constants.ts";
 import { useEditorScroll } from "./hooks/use-editor-scroll";
-import { useScrollPosition } from "./hooks/use-scroll-position";
+import { useScrollRestoreOnSwap } from "./hooks/use-scroll-position";
 import { EditorLinkMenu } from "@/features/editor/components/link/link-menu";
 import ColumnsMenu from "@/features/editor/components/columns/columns-menu.tsx";
 import { TransclusionLookupProvider } from "@/features/editor/components/transclusion/transclusion-lookup-context";
@@ -144,7 +143,6 @@ export default function PageEditor({
     [isComponentMounted],
   );
   const { handleScrollTo } = useEditorScroll({ canScroll });
-  const { restoreScrollPosition } = useScrollPosition(pageId);
   // Providers only created once per pageId
   const providersRef = useRef<{
     local: IndexeddbPersistence;
@@ -483,19 +481,10 @@ export default function PageEditor({
     }
   }, [yjsConnectionStatus, isSynced]);
 
-  // Restore as early as the static (cached) content is laid out, before paint,
+  // Restore the reader's scroll position across the static -> live editor swap.
-  // so the reader's position is applied without a visible jump. Aborts itself if
+  // The wiring (early pre-paint restore + post-swap re-assert) lives in the hook
-  // the reader has already started scrolling (handled inside the hook).
+  // so its triggers/guard are directly unit-testable.
-  useLayoutEffect(() => {
+  useScrollRestoreOnSwap(pageId, editor, showStatic);
-    restoreScrollPosition();
-  }, [restoreScrollPosition]);
-
-  // Re-assert once after the static -> live editor swap in case the swap reset
-  // the window scroll. Idempotent: a no-op when the position is already correct,
-  // and a no-op after the reader has interacted.
-  useLayoutEffect(() => {
-    if (!showStatic && editor) restoreScrollPosition();
-  }, [showStatic, editor, restoreScrollPosition]);
 
   return (
     <TransclusionLookupProvider>