From 768d135a1981565f823bcbb4471d4ee57bb87d40 Mon Sep 17 00:00:00 2001 From: claude_code Date: Thu, 2 Jul 2026 15:37:49 +0300 Subject: [PATCH 1/3] fix(editor): smooth scroll-position restore, no yank on early scroll (#266) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reading-position restore fired only after collab sync (`!showStatic`), so the page painted at the top and then visibly jumped — and readers who had already started scrolling were rudely yanked back to the saved position. - Abort restore permanently on genuine user scroll intent: `wheel`/`touchstart` unconditionally, and `keydown` only for real scroll keys (Arrow/Page/Home/End/ Space) so shortcuts and typing do not disable it. Our own `window.scrollTo` never emits these, so restore cannot self-abort. - Restore earlier via `useLayoutEffect` (before paint) while the static/cached content is laid out, and re-assert once after the static->live editor swap. - Make `restoreScrollPosition` idempotent with a redundancy guard so the two triggers never double-scroll; share one bounded timeout budget across them. - Add tests for interaction-abort, scroll-key filtering, idempotence. Co-Authored-By: Claude Opus 4.8 --- .../editor/hooks/use-scroll-position.test.ts | 86 +++++++++++++- .../editor/hooks/use-scroll-position.ts | 106 ++++++++++++++---- .../src/features/editor/page-editor.tsx | 14 ++- 3 files changed, 183 insertions(+), 23 deletions(-) diff --git a/apps/client/src/features/editor/hooks/use-scroll-position.test.ts b/apps/client/src/features/editor/hooks/use-scroll-position.test.ts index 1f0f1e2b..d9f3195c 100644 --- a/apps/client/src/features/editor/hooks/use-scroll-position.test.ts +++ b/apps/client/src/features/editor/hooks/use-scroll-position.test.ts @@ -100,7 +100,7 @@ describe("useScrollPosition", () => { expect(window.scrollTo).toHaveBeenCalledWith({ top: 500, behavior: "auto" }); }); - it("(a3) restores at most once per mount even if called again", () => { + it("(a3) is idempotent: re-asserting the same target does not scroll again", () => { vi.useFakeTimers(); window.sessionStorage.setItem(`${KEY_PREFIX}once`, "500"); setScrollHeight(2000); // tall enough to restore synchronously @@ -111,8 +111,12 @@ describe("useScrollPosition", () => { }); expect(window.scrollTo).toHaveBeenCalledTimes(1); + // Simulate the browser now being at the restored position. + setScrollY(500); + // A second call (e.g. the wiring effect re-running on [showStatic, editor, - // restoreScrollPosition]) must NOT scroll again and yank the reader. + // restoreScrollPosition]) must NOT scroll again: the redundancy guard sees + // the window is already at the target and does nothing. act(() => { result.current.restoreScrollPosition(); }); @@ -162,6 +166,84 @@ describe("useScrollPosition", () => { expect(window.scrollTo).not.toHaveBeenCalled(); }); + it("(g) does not restore if the reader scrolled (wheel) before restore fires", () => { + window.sessionStorage.setItem(`${KEY_PREFIX}g1`, "500"); + setScrollHeight(2000); // tall enough to restore synchronously + + const { result } = renderHook(() => useScrollPosition("g1")); + + // The reader shows scroll intent before restore is triggered. + act(() => { + window.dispatchEvent(new Event("wheel")); + }); + act(() => { + result.current.restoreScrollPosition(); + }); + + expect(window.scrollTo).not.toHaveBeenCalled(); + }); + + it("(h) aborts an in-flight restore poll when the reader scrolls", () => { + vi.useFakeTimers(); + window.sessionStorage.setItem(`${KEY_PREFIX}h1`, "500"); + setInnerHeight(800); + setScrollHeight(100); // maxScroll = -700: target not reachable yet, so it polls. + + const { result } = renderHook(() => useScrollPosition("h1")); + act(() => { + result.current.restoreScrollPosition(); + }); + expect(window.scrollTo).not.toHaveBeenCalled(); // still polling + + // The reader takes over mid-poll: this cancels the in-flight poll. + act(() => { + window.dispatchEvent(new Event("wheel")); + }); + + // Content of the page grows tall enough and time passes: the cancelled poll + // must NOT resurrect and yank the reader. + setScrollHeight(2000); + act(() => { + vi.advanceTimersByTime(5000); + }); + expect(window.scrollTo).not.toHaveBeenCalled(); + }); + + it("(i) a non-scroll keydown does NOT abort restore", () => { + window.sessionStorage.setItem(`${KEY_PREFIX}i1`, "500"); + setScrollHeight(2000); // tall enough to restore synchronously + + const { result } = renderHook(() => useScrollPosition("i1")); + + // A non-scroll key (e.g. typing, a shortcut) must NOT count as scroll intent. + act(() => { + window.dispatchEvent(new KeyboardEvent("keydown", { key: "a" })); + }); + act(() => { + result.current.restoreScrollPosition(); + }); + + // Restore still happens: the innocuous keypress did not disable it. + expect(window.scrollTo).toHaveBeenCalledWith({ top: 500, behavior: "auto" }); + }); + + it("(j) a scroll keydown (Space) DOES abort restore", () => { + window.sessionStorage.setItem(`${KEY_PREFIX}j1`, "500"); + setScrollHeight(2000); // tall enough to restore synchronously + + const { result } = renderHook(() => useScrollPosition("j1")); + + // Space scrolls the page: this is real scroll intent and must abort restore. + act(() => { + window.dispatchEvent(new KeyboardEvent("keydown", { key: " " })); + }); + act(() => { + result.current.restoreScrollPosition(); + }); + + expect(window.scrollTo).not.toHaveBeenCalled(); + }); + it("(c) does nothing when nothing is saved or the saved value is <= 0", () => { // Nothing saved. const a = renderHook(() => useScrollPosition("nope")); 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 6a72f754..e6500b9d 100644 --- a/apps/client/src/features/editor/hooks/use-scroll-position.ts +++ b/apps/client/src/features/editor/hooks/use-scroll-position.ts @@ -13,6 +13,18 @@ const RESTORE_POLL_MS = 100; // "remember where I was reading" feature (self-limiting, no cross-tab leak). const STORAGE_PREFIX = "gitmost:scroll-position:"; +// Keys that scroll the window. Only these count as scroll intent for keydown; +// other keys (shortcuts, modifiers, typing) must NOT disable scroll restore. +const SCROLL_KEYS = new Set([ + "ArrowUp", + "ArrowDown", + "PageUp", + "PageDown", + "Home", + "End", + " ", // Space (and Shift+Space) scroll the page +]); + function storageKey(pageId: string): string { return `${STORAGE_PREFIX}${pageId}`; } @@ -48,32 +60,41 @@ function writeStorage(pageId: string, scrollY: number): void { * Persists and restores the window scroll position per page so a reader keeps * their place across a reload (F5) or reopening the document. * - * Returns `restoreScrollPosition`, which the page editor calls once the live - * (non-static) content is laid out. The two scroll mechanisms are mutually - * exclusive: if the URL has a `#hash` anchor, the existing anchor-scroll logic - * wins and restore is a no-op. + * Returns `restoreScrollPosition`, which the page editor calls from two triggers + * (early, while the static/cached content is laid out, and again after the + * static->live editor swap); it is idempotent, so re-asserting the same target is + * a no-op. The two scroll mechanisms are mutually exclusive: if the URL has a + * `#hash` anchor, the existing anchor-scroll logic wins and restore is a no-op. */ export function useScrollPosition(pageId: string): { restoreScrollPosition: () => void; } { // CONTRACT: this hook assumes PageEditor REMOUNTS per page — page.tsx renders // ``, so switching pages creates a fresh - // hook instance with fresh refs. These refs latch per-mount and are NOT reset - // when `pageId` changes in place (only the effect re-runs on [pageId]). If that - // `key={page.id}` is ever removed, restore would silently break on the 2nd page - // (refs would hold the first page's target / already-restored flag) — in that - // case the refs must be reset on a pageId change. + // hook instance with fresh refs. Restore is idempotent and interaction-gated + // (not single-shot): it may be called from several triggers and re-asserts the + // SAME captured target, which is a no-op once the window is already positioned. + // The per-mount refs that latch are `initialTargetRef` (the captured target) + // and `userInteractedRef` (the reader has taken over scrolling). They are NOT + // reset when `pageId` changes in place (only the effect re-runs on [pageId]). + // If that `key={page.id}` is ever removed, restore would silently break on the + // 2nd page (refs would hold the first page's target / interaction flag) — in + // that case the refs must be reset on a pageId change. // // The target Y captured synchronously at mount, BEFORE any scroll/visibility // handler can overwrite the stored value with a fresh 0 (the page starts // scrolled to top on load). `null` means "not yet captured". const initialTargetRef = useRef(null); - // Guards so restore runs at most once per page mount. - const hasRestoredRef = useRef(false); + // Set once the reader shows unambiguous scroll intent; restore must never yank + // a reader who has already started scrolling. + const userInteractedRef = useRef(false); // Holds the in-flight restore poll timer so the cleanup can cancel it: without // this, a fast SPA navigation away mid-poll would let the old page's poll fire // window.scrollTo against the NEW page's document (visible wrong-page scroll). const pollTimerRef = useRef(null); + // Timestamp of the FIRST restore attempt so re-triggers (e.g. the static→live + // editor swap) share ONE bounded timeout budget instead of restarting it. + const restoreStartRef = useRef(null); // Capture the previously-saved value synchronously during render, before the // effect below registers handlers that would persist the current (0) scrollY. @@ -114,14 +135,43 @@ export function useScrollPosition(pageId: string): { } }; + // User scroll-intent signals. wheel and touch are unconditional scroll + // intent; keydown is filtered to actual scroll keys only (SCROLL_KEYS) so + // shortcuts, lone modifiers, and typing do not abort restore. Our own + // window.scrollTo does NOT emit these, so restore can never self-abort via + // them. Once the reader shows intent we mark it and cancel any in-flight + // restore poll so restore can never yank them back. (Scrollbar-drag via + // pointer is an accepted small gap — it is not covered here.) + const onUserIntent = (event: Event) => { + // wheel/touchstart are unambiguous scroll intent; for keydown, only real + // scroll keys count — a shortcut or typing must not abort restore. + if ( + event.type === "keydown" && + !SCROLL_KEYS.has((event as KeyboardEvent).key) + ) { + return; + } + userInteractedRef.current = true; + if (pollTimerRef.current !== null) { + window.clearTimeout(pollTimerRef.current); + pollTimerRef.current = null; + } + }; + window.addEventListener("scroll", onScroll, { passive: true }); window.addEventListener("pagehide", onPageHide); document.addEventListener("visibilitychange", onVisibilityChange); + window.addEventListener("wheel", onUserIntent, { passive: true }); + window.addEventListener("touchstart", onUserIntent, { passive: true }); + window.addEventListener("keydown", onUserIntent); return () => { window.removeEventListener("scroll", onScroll); window.removeEventListener("pagehide", onPageHide); document.removeEventListener("visibilitychange", onVisibilityChange); + window.removeEventListener("wheel", onUserIntent); + window.removeEventListener("touchstart", onUserIntent); + window.removeEventListener("keydown", onUserIntent); if (throttleTimer !== null) { window.clearTimeout(throttleTimer); throttleTimer = null; @@ -137,9 +187,8 @@ export function useScrollPosition(pageId: string): { }, [pageId]); const restoreScrollPosition = useCallback(() => { - // Run at most once per page mount. - if (hasRestoredRef.current) return; - hasRestoredRef.current = true; + // The reader took over — never yank them back. + if (userInteractedRef.current) return; // Anchor priority: a `#hash` in the URL is handled by useEditorScroll. if (window.location.hash) return; @@ -148,9 +197,26 @@ export function useScrollPosition(pageId: string): { // Nothing meaningful to restore to. if (targetY <= 0) return; - const start = Date.now(); + // Cancel any in-flight poll before (re)starting, so overlapping triggers can + // never run two concurrent polls against the same target. + if (pollTimerRef.current !== null) { + window.clearTimeout(pollTimerRef.current); + pollTimerRef.current = null; + } + + // Share one timeout budget across re-triggers instead of restarting it. + if (restoreStartRef.current === null) { + restoreStartRef.current = Date.now(); + } + const start = restoreStartRef.current; const tryRestore = () => { + // Bail mid-poll if the reader started scrolling while we were waiting. + if (userInteractedRef.current) { + pollTimerRef.current = null; + return; + } + const maxScroll = document.documentElement.scrollHeight - window.innerHeight; const timedOut = Date.now() - start >= MAX_RESTORE_WAIT_MS; @@ -158,10 +224,12 @@ export function useScrollPosition(pageId: string): { // Restore once the content is tall enough to reach the target, or bail out // after the timeout and scroll as far as currently possible. if (maxScroll >= targetY || timedOut) { - window.scrollTo({ - top: Math.min(targetY, Math.max(maxScroll, 0)), - behavior: "auto", - }); + const top = Math.min(targetY, Math.max(maxScroll, 0)); + // Redundancy guard: re-asserting the SAME target when already positioned + // is a no-op, so this hook can be called from multiple triggers safely. + if (Math.abs(window.scrollY - top) > 1) { + window.scrollTo({ top, behavior: "auto" }); + } pollTimerRef.current = null; return; } diff --git a/apps/client/src/features/editor/page-editor.tsx b/apps/client/src/features/editor/page-editor.tsx index 10d65a7e..ada24872 100644 --- a/apps/client/src/features/editor/page-editor.tsx +++ b/apps/client/src/features/editor/page-editor.tsx @@ -2,6 +2,7 @@ import "@/features/editor/styles/index.css"; import React, { useCallback, useEffect, + useLayoutEffect, useMemo, useRef, useState, @@ -482,8 +483,17 @@ export default function PageEditor({ } }, [yjsConnectionStatus, isSynced]); - // Restore the saved reading position once the live content is laid out. - useEffect(() => { + // 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]); -- 2.52.0 From 963822bd28aa40a580b2bd6b990e7cf2ee249a35 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Thu, 2 Jul 2026 22:16:22 +0300 Subject: [PATCH 2/3] test(#289 review): cover shared restore budget + scroll-restore wiring (F1/F2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit F1: pin the shared restoreStartRef timeout budget — re-triggers share ONE budget (measured from the first call), not a per-trigger restart. Test drives short content (polls), triggers at t=0 and t=3s, and asserts the clamp fires at t=5s from the FIRST call. Verified non-vacuous: a mutant that resets the budget on each trigger fails it. F2: cover the two useLayoutEffect scroll-restore blocks. A full PageEditor mount has no precedent in the client suite (it builds live Hocuspocus/IndexedDB providers + collab tiptap; the static->live swap gates on isCollabSynced, only reproducible by driving mocked provider callbacks = testing the mocks). Per the reviewer's allowance for a justified lighter variant: page-editor.test.tsx reproduces the two effects and (1) asserts the [showStatic, editor] deps + the '&& editor' guard via a stable spy, (2) drives the REAL useScrollPosition end-to-end so the post-swap re-assert is the sole cause of scrollTo. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../editor/hooks/use-scroll-position.test.ts | 49 +++++ .../src/features/editor/page-editor.test.tsx | 169 ++++++++++++++++++ 2 files changed, 218 insertions(+) create mode 100644 apps/client/src/features/editor/page-editor.test.tsx diff --git a/apps/client/src/features/editor/hooks/use-scroll-position.test.ts b/apps/client/src/features/editor/hooks/use-scroll-position.test.ts index d9f3195c..06debfde 100644 --- a/apps/client/src/features/editor/hooks/use-scroll-position.test.ts +++ b/apps/client/src/features/editor/hooks/use-scroll-position.test.ts @@ -303,6 +303,55 @@ describe("useScrollPosition", () => { expect(window.scrollTo).toHaveBeenCalledWith({ top: 200, behavior: "auto" }); }); + it("(k) shares ONE timeout budget across re-triggers (does not restart the clock)", () => { + // The static->live editor swap re-invokes restore. The shared budget + // (restoreStartRef) must measure the MAX_RESTORE_WAIT_MS (5000) deadline + // from the FIRST trigger, not restart it on every re-trigger. This pins + // the `if (restoreStartRef.current === null)` guard: a mutant that resets + // `restoreStartRef.current = Date.now()` on every trigger would push the + // deadline out to t=8000 (3000 + 5000) and fail the t=5000 assertion below. + vi.useFakeTimers(); + vi.setSystemTime(0); + window.sessionStorage.setItem(`${KEY_PREFIX}k1`, "5000"); + setInnerHeight(800); + setScrollHeight(1000); // maxScroll = 200, never reaches 5000 -> it polls. + + const { result } = renderHook(() => useScrollPosition("k1")); + + // First trigger at t=0: starts the shared budget and begins polling. + act(() => { + result.current.restoreScrollPosition(); + }); + expect(window.scrollTo).not.toHaveBeenCalled(); + + // Advance to t=3000 (still polling: content short, not yet timed out). + act(() => { + vi.advanceTimersByTime(3000); + }); + expect(window.scrollTo).not.toHaveBeenCalled(); + + // Second trigger at t=3000 (the swap re-assert). Under the real code the + // budget is shared, so `start` stays 0; under the reset-mutant it becomes 3000. + act(() => { + result.current.restoreScrollPosition(); + }); + + // At t=4900 the FIRST budget has not yet elapsed (4900 - 0 < 5000): no clamp. + act(() => { + vi.advanceTimersByTime(1900); + }); + expect(window.scrollTo).not.toHaveBeenCalled(); + + // At t=5000 the shared budget (measured from t=0) times out and clamps to the + // furthest reachable position (maxScroll = 200). The reset-mutant, measuring + // from t=3000, would still be waiting (5000 - 3000 = 2000 < 5000) and would + // NOT have scrolled here -> this assertion fails against that mutant. + act(() => { + vi.advanceTimersByTime(100); + }); + expect(window.scrollTo).toHaveBeenCalledWith({ top: 200, behavior: "auto" }); + }); + it("(e) never throws when storage access throws", () => { const err = new Error("storage denied"); vi.spyOn(window.sessionStorage, "getItem").mockImplementation(() => { diff --git a/apps/client/src/features/editor/page-editor.test.tsx b/apps/client/src/features/editor/page-editor.test.tsx new file mode 100644 index 00000000..123045c6 --- /dev/null +++ b/apps/client/src/features/editor/page-editor.test.tsx @@ -0,0 +1,169 @@ +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"; + +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 +// 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, +// the static->live swap (`showStatic` -> false) is gated on +// `isCollabSynced(status, isLocalSynced && isRemoteSynced)`, which can only flip +// by driving the mocked collab provider's async sync callbacks. The heaviest +// component-test precedent in the repo (comment-hover-preview.test.tsx) mounts a +// 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; +} + +function setScrollY(value: number): void { + Object.defineProperty(window, "scrollY", { configurable: true, value }); +} +function setScrollHeight(value: number): void { + Object.defineProperty(document.documentElement, "scrollHeight", { + configurable: true, + value, + }); +} +function setInnerHeight(value: number): void { + Object.defineProperty(window, "innerHeight", { configurable: true, value }); +} + +describe("PageEditor scroll-restore wiring (two useLayoutEffects)", () => { + beforeEach(() => { + window.sessionStorage.clear(); + setScrollY(0); + setScrollHeight(0); + setInnerHeight(800); + window.scrollTo = vi.fn(); + window.location.hash = ""; + }); + + afterEach(() => { + vi.restoreAllMocks(); + vi.useRealTimers(); + 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 ( + + ); + } + + // 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); + + // 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); + + // 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); + }); + + 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). + 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(); + expect(window.scrollTo).not.toHaveBeenCalled(); + + // The live content is now laid out tall enough to reach the target. + setScrollHeight(2000); // maxScroll = 1200 >= 500 + + // The static -> live swap: the post-swap useLayoutEffect re-invokes the real + // hook, whose synchronous tryRestore now reaches the target and scrolls. + act(() => { + rerender(); + }); + expect(window.scrollTo).toHaveBeenCalledWith({ top: 500, behavior: "auto" }); + }); +}); -- 2.52.0 From 293348f9dc4e77458d44b66db83f882dd813aebe Mon Sep 17 00:00:00 2001 From: agent_coder Date: Thu, 2 Jul 2026 23:29:01 +0300 Subject: [PATCH 3/3] 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 ( -- 2.52.0