From 293348f9dc4e77458d44b66db83f882dd813aebe Mon Sep 17 00:00:00 2001 From: agent_coder Date: Thu, 2 Jul 2026 23:29:01 +0300 Subject: [PATCH] refactor(#289 review): extract useScrollRestoreOnSwap so the test guards real code (F2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../editor/hooks/use-scroll-position.ts | 37 +++- .../src/features/editor/page-editor.test.tsx | 158 +++++++----------- .../src/features/editor/page-editor.tsx | 21 +-- 3 files changed, 106 insertions(+), 110 deletions(-) diff --git a/apps/client/src/features/editor/hooks/use-scroll-position.ts b/apps/client/src/features/editor/hooks/use-scroll-position.ts index e6500b9d..8c2459fb 100644 --- a/apps/client/src/features/editor/hooks/use-scroll-position.ts +++ b/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]); +} diff --git a/apps/client/src/features/editor/page-editor.test.tsx b/apps/client/src/features/editor/page-editor.test.tsx index 123045c6..57d3fab8 100644 --- a/apps/client/src/features/editor/page-editor.test.tsx +++ b/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 { useScrollPosition } from "./hooks/use-scroll-position"; +import type { Editor } from "@tiptap/react"; +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 -// useScrollPosition hook. A FULL PageEditor component test is impractical here and -// has no precedent in this client: PageEditor directly constructs a +// The real UX wiring lives in the exported `useScrollRestoreOnSwap` hook (two +// useLayoutEffects around useScrollPosition), which PageEditor calls with the +// 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 -// contract: the two useLayoutEffect blocks are reproduced VERBATIM from -// page-editor.tsx (the early pre-paint restore, and the post-swap re-assert with -// deps [showStatic, editor]) and exercised against the REAL useScrollPosition -// hook. If page-editor's wiring regresses (e.g. the swap effect drops the -// `&& editor` guard or its deps), the mirror below regresses in lockstep. - -// Mirror of page-editor.tsx lines ~489-498 (the two scroll-restore useLayoutEffects). -function ScrollRestoreWiring({ - restoreScrollPosition, - showStatic, - 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; -} +// So this file tests the REAL `useScrollRestoreOnSwap` hook — the exact code +// PageEditor imports and calls — driving its `showStatic`/`editor` inputs the way +// the swap does. Because it exercises the real hook (not a copy), dropping the +// `&& editor` guard or changing the effect deps makes these tests fail; they +// guard the production code directly (verified: removing `&& editor` reddens the +// first test). +// +// Both tests observe the real effect via `window.scrollTo`. The stubbed +// `window.scrollTo` never mutates `window.scrollY`, and the target is left +// unreached, so every restore invocation that passes the guard yields exactly one +// `scrollTo` call — making the call count a faithful proxy for restore invocations. 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", () => { - // A referentially STABLE spy, mirroring page-editor where restoreScrollPosition - // is a useCallback([]) — so the early effect (dep [restoreScrollPosition]) runs - // exactly once and does NOT re-fire on every render. - const restore = vi.fn(); - - const editor = { id: "editor" }; - - // Host owns the swap state so we can drive showStatic/editor like page-editor. - function Host({ - showStatic, - editorValue, - }: { - showStatic: boolean; - editorValue: unknown | null; - }) { - return ( - - ); - } + it("re-invokes restore after the swap, with the [showStatic, editor] deps/guard", () => { + // Target is immediately reachable, so each restore that passes the guard + // scrolls synchronously. `window.scrollY` stays 0 (stubbed scrollTo never + // updates it), so scrollTo is called once per effective restore — a proxy for + // the restore invocation count. + window.sessionStorage.setItem(`${KEY_PREFIX}guard`, "500"); + setInnerHeight(800); + setScrollHeight(2000); // maxScroll = 1200 >= 500: reachable, no polling. // 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(); - expect(restore).toHaveBeenCalledTimes(1); + const { rerender } = render( + , + ); + 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. - // (Pins the guard: dropping `&& editor` would restore against a null editor.) - rerender(); - expect(restore).toHaveBeenCalledTimes(1); + // yet: the swap effect re-runs (deps [showStatic, editor] changed) but the + // `&& editor` guard must keep it a no-op. The early effect does NOT re-fire + // (its dep [restoreScrollPosition] is a stable useCallback([])). + // (Pins the guard: dropping `&& editor` would restore against a null editor, + // producing a 2nd scrollTo and failing this expectation.) + rerender(); + 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 - // does NOT re-fire (restore identity is stable), so this second call is driven - // solely by the [showStatic, editor] deps changing. - rerender(); - expect(restore).toHaveBeenCalledTimes(2); + // post-swap effect re-asserts the restore exactly once more, driven solely by + // the [showStatic, editor] deps changing. + rerender(); + expect(window.scrollTo).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 - // CAUSE of the scroll (nothing scrolls before it). + // End-to-end through the real useScrollPosition (inside the hook): the swap + // 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 ( - - ); - } - // 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(); + const { rerender } = render( + , + ); 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(); + rerender(); }); expect(window.scrollTo).toHaveBeenCalledWith({ top: 500, behavior: "auto" }); }); diff --git a/apps/client/src/features/editor/page-editor.tsx b/apps/client/src/features/editor/page-editor.tsx index ada24872..b001051f 100644 --- a/apps/client/src/features/editor/page-editor.tsx +++ b/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, - // 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]); + // Restore the reader's scroll position across the static -> live editor swap. + // The wiring (early pre-paint restore + post-swap re-assert) lives in the hook + // so its triggers/guard are directly unit-testable. + useScrollRestoreOnSwap(pageId, editor, showStatic); return (