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 06debfde..7e3a880a 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 @@ -1,6 +1,9 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import { renderHook, act } from "@testing-library/react"; -import { useScrollPosition } from "./use-scroll-position"; +import { + useScrollPosition, + hasSavedReadingPosition, +} from "./use-scroll-position"; const KEY_PREFIX = "gitmost:scroll-position:"; @@ -90,46 +93,28 @@ describe("useScrollPosition", () => { }); expect(window.sessionStorage.getItem(`${KEY_PREFIX}clob`)).toBe("0"); - // Restore still scrolls to 500 (the captured target), NOT the clobbered 0. - // If the capture were moved into an effect (after handlers register), it - // would read the clobbered 0 and this assertion would fail. - setScrollHeight(2000); // maxScroll = 1200 >= 500 + // Content is tall enough (maxScroll = 2000 - 800 = 1200 >= 500) and stays + // steady, so once the height settles restore scrolls to 500 (the captured + // target), NOT the clobbered 0. If the capture were moved into an effect + // (after handlers register) it would read the clobbered 0 and this fails. + setScrollHeight(2000); act(() => { result.current.restoreScrollPosition(); }); + // Height held steady: after HEIGHT_STABLE_MS (400) the poll settles + scrolls. + act(() => { + vi.advanceTimersByTime(500); + }); expect(window.scrollTo).toHaveBeenCalledWith({ top: 500, behavior: "auto" }); }); - 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 - - const { result } = renderHook(() => useScrollPosition("once")); - act(() => { - result.current.restoreScrollPosition(); - }); - 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: the redundancy guard sees - // the window is already at the target and does nothing. - act(() => { - result.current.restoreScrollPosition(); - }); - expect(window.scrollTo).toHaveBeenCalledTimes(1); - }); - it("(b) does not restore when the URL has a #hash anchor", () => { vi.useFakeTimers(); window.sessionStorage.setItem(`${KEY_PREFIX}p2`, "500"); - // Content is ALREADY tall enough (maxScroll = 2000 - 800 = 1200 >= 500), so - // without the hash guard tryRestore would call scrollTo synchronously on the - // first tick. The assertion below therefore genuinely proves the hash guard - // short-circuits before any scroll (not just that the poll has not fired). + // Content is ALREADY tall enough (maxScroll = 2000 - 800 = 1200 >= 500) and + // steady, so without the hash guard the poll would settle and scroll. The + // assertion below therefore genuinely proves the hash guard short-circuits + // before any timer is even scheduled. setScrollHeight(2000); window.location.hash = "#some-heading"; @@ -142,6 +127,93 @@ describe("useScrollPosition", () => { expect(window.scrollTo).not.toHaveBeenCalled(); }); + it("(c) does nothing when nothing is saved or the saved value is <= 0", () => { + vi.useFakeTimers(); + + // Nothing saved. + const a = renderHook(() => useScrollPosition("nope")); + act(() => { + a.result.current.restoreScrollPosition(); + vi.advanceTimersByTime(5000); + }); + expect(window.scrollTo).not.toHaveBeenCalled(); + + // Saved value <= 0. + window.sessionStorage.setItem(`${KEY_PREFIX}zero`, "0"); + const b = renderHook(() => useScrollPosition("zero")); + act(() => { + b.result.current.restoreScrollPosition(); + vi.advanceTimersByTime(5000); + }); + expect(window.scrollTo).not.toHaveBeenCalled(); + }); + + it("(d) restores once the height has been STABLE for HEIGHT_STABLE_MS and is reachable", () => { + vi.useFakeTimers(); + window.sessionStorage.setItem(`${KEY_PREFIX}p4`, "500"); + setInnerHeight(800); + setScrollHeight(2000); // maxScroll = 1200 >= 500: reachable from the start. + + const { result } = renderHook(() => useScrollPosition("p4")); + act(() => { + result.current.restoreScrollPosition(); + }); + + // Keep flipping the height across a few 100ms ticks: every change resets the + // "stable since" clock, so the layout is never considered settled and restore + // must NOT fire yet even though the target is reachable. + setScrollHeight(2001); + act(() => { + vi.advanceTimersByTime(100); + }); + expect(window.scrollTo).not.toHaveBeenCalled(); + + setScrollHeight(2000); + act(() => { + vi.advanceTimersByTime(100); + }); + expect(window.scrollTo).not.toHaveBeenCalled(); + + setScrollHeight(2001); + act(() => { + vi.advanceTimersByTime(100); + }); + expect(window.scrollTo).not.toHaveBeenCalled(); + + // Now hold the height steady at 2000 and advance past HEIGHT_STABLE_MS (400): + // the layout settles and restore scrolls once to the saved offset. + setScrollHeight(2000); + act(() => { + vi.advanceTimersByTime(500); + }); + expect(window.scrollTo).toHaveBeenCalledWith({ top: 500, behavior: "auto" }); + }); + + it("(d2) settled-but-unreachable does NOT restore until the timeout, then clamps", () => { + vi.useFakeTimers(); + window.sessionStorage.setItem(`${KEY_PREFIX}p5`, "5000"); + setInnerHeight(800); + setScrollHeight(1000); // maxScroll stays 200, never reaches 5000. + + const { result } = renderHook(() => useScrollPosition("p5")); + act(() => { + result.current.restoreScrollPosition(); + }); + + // Past HEIGHT_STABLE_MS the height IS settled, but the target is unreachable + // (maxScroll 200 < 5000), so restore must keep waiting rather than scroll. + act(() => { + vi.advanceTimersByTime(500); + }); + expect(window.scrollTo).not.toHaveBeenCalled(); + + // Only the MAX_RESTORE_WAIT_MS (5000) timeout fires it, clamped to maxScroll. + act(() => { + vi.advanceTimersByTime(4500); + }); + expect(window.scrollTo).toHaveBeenCalledWith({ top: 200, behavior: "auto" }); + }); + it("(f) cancels the in-flight restore poll on unmount (no scroll on the next page)", () => { vi.useFakeTimers(); window.sessionStorage.setItem(`${KEY_PREFIX}p7`, "500"); @@ -151,6 +223,7 @@ describe("useScrollPosition", () => { const { result, unmount } = renderHook(() => useScrollPosition("p7")); act(() => { result.current.restoreScrollPosition(); + vi.advanceTimersByTime(300); }); expect(window.scrollTo).not.toHaveBeenCalled(); // still polling @@ -167,8 +240,9 @@ describe("useScrollPosition", () => { }); it("(g) does not restore if the reader scrolled (wheel) before restore fires", () => { + vi.useFakeTimers(); window.sessionStorage.setItem(`${KEY_PREFIX}g1`, "500"); - setScrollHeight(2000); // tall enough to restore synchronously + setScrollHeight(2000); // reachable + steady: would settle and scroll otherwise. const { result } = renderHook(() => useScrollPosition("g1")); @@ -178,12 +252,13 @@ describe("useScrollPosition", () => { }); act(() => { result.current.restoreScrollPosition(); + vi.advanceTimersByTime(5000); }); expect(window.scrollTo).not.toHaveBeenCalled(); }); - it("(h) aborts an in-flight restore poll when the reader scrolls", () => { + it("(h) aborts an in-flight restore poll when the reader scrolls mid-wait", () => { vi.useFakeTimers(); window.sessionStorage.setItem(`${KEY_PREFIX}h1`, "500"); setInnerHeight(800); @@ -192,6 +267,7 @@ describe("useScrollPosition", () => { const { result } = renderHook(() => useScrollPosition("h1")); act(() => { result.current.restoreScrollPosition(); + vi.advanceTimersByTime(200); }); expect(window.scrollTo).not.toHaveBeenCalled(); // still polling @@ -201,7 +277,7 @@ describe("useScrollPosition", () => { }); // Content of the page grows tall enough and time passes: the cancelled poll - // must NOT resurrect and yank the reader. + // must NOT resurrect and yank the reader, even after the height would settle. setScrollHeight(2000); act(() => { vi.advanceTimersByTime(5000); @@ -210,8 +286,9 @@ describe("useScrollPosition", () => { }); it("(i) a non-scroll keydown does NOT abort restore", () => { + vi.useFakeTimers(); window.sessionStorage.setItem(`${KEY_PREFIX}i1`, "500"); - setScrollHeight(2000); // tall enough to restore synchronously + setScrollHeight(2000); // reachable + steady. const { result } = renderHook(() => useScrollPosition("i1")); @@ -222,14 +299,17 @@ describe("useScrollPosition", () => { act(() => { result.current.restoreScrollPosition(); }); - - // Restore still happens: the innocuous keypress did not disable it. + // Height steady: restore settles and still fires — the keypress did not abort. + act(() => { + vi.advanceTimersByTime(500); + }); expect(window.scrollTo).toHaveBeenCalledWith({ top: 500, behavior: "auto" }); }); it("(j) a scroll keydown (Space) DOES abort restore", () => { + vi.useFakeTimers(); window.sessionStorage.setItem(`${KEY_PREFIX}j1`, "500"); - setScrollHeight(2000); // tall enough to restore synchronously + setScrollHeight(2000); // reachable + steady: would settle and scroll otherwise. const { result } = renderHook(() => useScrollPosition("j1")); @@ -239,117 +319,10 @@ describe("useScrollPosition", () => { }); 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")); - act(() => { - a.result.current.restoreScrollPosition(); - }); - expect(window.scrollTo).not.toHaveBeenCalled(); - - // Saved value <= 0. - window.sessionStorage.setItem(`${KEY_PREFIX}zero`, "0"); - const b = renderHook(() => useScrollPosition("zero")); - act(() => { - b.result.current.restoreScrollPosition(); - }); - expect(window.scrollTo).not.toHaveBeenCalled(); - }); - - it("(d) scrolls to the saved Y once the content is tall enough", () => { - vi.useFakeTimers(); - window.sessionStorage.setItem(`${KEY_PREFIX}p4`, "500"); - setInnerHeight(800); - setScrollHeight(100); // maxScroll = -700, target not yet reachable. - - const { result } = renderHook(() => useScrollPosition("p4")); - act(() => { - result.current.restoreScrollPosition(); - }); - - // Still polling: content not laid out yet. - expect(window.scrollTo).not.toHaveBeenCalled(); - - // Content becomes tall enough: maxScroll = 2000 - 800 = 1200 >= 500. - setScrollHeight(2000); - act(() => { - vi.advanceTimersByTime(100); - }); - - expect(window.scrollTo).toHaveBeenCalledWith({ top: 500, behavior: "auto" }); - }); - - it("(d2) clamps to the max reachable position after the timeout", () => { - vi.useFakeTimers(); - window.sessionStorage.setItem(`${KEY_PREFIX}p5`, "5000"); - setInnerHeight(800); - setScrollHeight(1000); // maxScroll stays 200, never reaches 5000. - - const { result } = renderHook(() => useScrollPosition("p5")); - act(() => { - result.current.restoreScrollPosition(); - }); - - // Advance past the 5s timeout; restore should fire clamped to maxScroll. - act(() => { vi.advanceTimersByTime(5000); }); - 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", () => { @@ -372,3 +345,28 @@ describe("useScrollPosition", () => { }).not.toThrow(); }); }); + +describe("hasSavedReadingPosition", () => { + beforeEach(() => { + window.sessionStorage.clear(); + }); + + afterEach(() => { + vi.restoreAllMocks(); + window.sessionStorage.clear(); + }); + + it("is false when nothing is saved for the page", () => { + expect(hasSavedReadingPosition("absent")).toBe(false); + }); + + it("is false when the saved value is 0 (page is at the top)", () => { + window.sessionStorage.setItem(`${KEY_PREFIX}top`, "0"); + expect(hasSavedReadingPosition("top")).toBe(false); + }); + + it("is true when a positive reading position is saved", () => { + window.sessionStorage.setItem(`${KEY_PREFIX}deep`, "500"); + expect(hasSavedReadingPosition("deep")).toBe(true); + }); +}); 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 8c2459fb..537312ed 100644 --- a/apps/client/src/features/editor/hooks/use-scroll-position.ts +++ b/apps/client/src/features/editor/hooks/use-scroll-position.ts @@ -8,6 +8,10 @@ const SAVE_THROTTLE_MS = 250; const MAX_RESTORE_WAIT_MS = 5000; // How often to re-check the document height while waiting for content to load. const RESTORE_POLL_MS = 100; +// The document height must stay unchanged this long before we treat the layout +// as settled and safe to restore against — restoring while content/images are +// still streaming in would let scroll-anchoring drift the saved offset. +const HEIGHT_STABLE_MS = 400; // sessionStorage key prefix. sessionStorage survives an F5 in the same tab and // is cleared on tab close, which is exactly the lifetime we want for an MVP @@ -57,24 +61,39 @@ function writeStorage(pageId: string, scrollY: number): void { } } +/** + * Whether a positive reading position is saved for this page — i.e. the page + * will be scrolled away from the top on load. Used by the title editor to avoid + * auto-focusing (and thus placing the caret in) the now-off-screen title. + * Returns false when nothing is saved or storage is unavailable. + */ +export function hasSavedReadingPosition(pageId: string): boolean { + const y = readStorage(pageId); + return typeof y === "number" && y > 0; +} + /** * 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 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. + * (early on mount + post-swap, after the static->live editor swap). It waits for + * the document height to settle, then scrolls once to the captured target; it + * aborts if the reader shows scroll intent first. 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. 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. + // hook instance with fresh refs. Restore is called from two triggers (early on + // mount + post-swap) and waits for the document height to settle before + // scrolling once to the captured target; it aborts on reader scroll intent, and + // a `#hash` in the URL wins (restore becomes a no-op). It is idempotent enough + // to be re-invoked safely: an already-running poll is not restarted, and once + // the window is at the target the final scroll is a no-op. // 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]). @@ -93,9 +112,6 @@ export function useScrollPosition(pageId: string): { // 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. @@ -190,56 +206,50 @@ export function useScrollPosition(pageId: string): { const restoreScrollPosition = useCallback(() => { // 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; - const targetY = initialTargetRef.current ?? 0; // Nothing meaningful to restore to. if (targetY <= 0) return; + // Idempotent: if a restore poll is already running, do not start a second. + if (pollTimerRef.current !== null) return; - // 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; - } + const start = Date.now(); + let lastHeight = -1; + let stableSince = start; - // 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. + // Restore ONCE the document height has been stable for HEIGHT_STABLE_MS AND + // the target is actually reachable, so the saved pixel offset lands on the + // same content the reader left (restoring earlier lets scroll-anchoring drift + // it as content streams in above the target). The timeout is the only + // fallback that clamps to the furthest reachable position. + const tick = () => { if (userInteractedRef.current) { pollTimerRef.current = null; return; } - - const maxScroll = - document.documentElement.scrollHeight - window.innerHeight; - const timedOut = Date.now() - start >= MAX_RESTORE_WAIT_MS; - - // 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) { + const now = Date.now(); + const height = document.documentElement.scrollHeight; + if (height !== lastHeight) { + lastHeight = height; + stableSince = now; + } + const maxScroll = height - window.innerHeight; + const settled = now - stableSince >= HEIGHT_STABLE_MS; + const reachable = maxScroll >= targetY; + const timedOut = now - start >= MAX_RESTORE_WAIT_MS; + if ((settled && reachable) || timedOut) { 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; } - - // Stored in a ref so the effect cleanup can cancel it on unmount. - pollTimerRef.current = window.setTimeout(tryRestore, RESTORE_POLL_MS); + pollTimerRef.current = window.setTimeout(tick, RESTORE_POLL_MS); }; - tryRestore(); + pollTimerRef.current = window.setTimeout(tick, RESTORE_POLL_MS); }, []); return { restoreScrollPosition }; @@ -250,8 +260,16 @@ export function useScrollPosition(pageId: string): { * * 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. + * There are TWO triggers: + * - an EARLY one on mount, so a reload that never reaches the live editor + * (offline / collab never syncs — the static cache stays shown) still restores + * the reading position instead of being stuck at the top; and + * - a POST-SWAP one, so once the live editor is mounted the wait re-measures + * against the FINAL layout the reader will see. + * Both are safe because `restoreScrollPosition` is idempotent: a running poll + * suppresses a second start, and once already positioned the final scroll is a + * no-op. The early trigger is now bounce-free because the title-focus root cause + * is fixed (title auto-focus is skipped when a saved position exists). * * @param pageId the page whose scroll position is persisted/restored. * @param editor the tiptap editor instance, or `null` until it is ready. @@ -264,16 +282,18 @@ export function useScrollRestoreOnSwap( ): 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). + // Early trigger: start restoring on mount, so a reload that never reaches the + // live editor (offline / collab never syncs — the static cache stays shown) + // still restores the reading position. Restore waits for the document height to + // settle before scrolling, so this measures whatever layout is current (static, + // then the live layout if the swap happens). 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. + // Post-swap trigger: after the static -> live swap, re-invoke restore so the + // wait re-measures against the final live layout. Idempotent — a no-op while the + // early poll is still running, and a no-op once already positioned. 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 57d3fab8..1be75765 100644 --- a/apps/client/src/features/editor/page-editor.test.tsx +++ b/apps/client/src/features/editor/page-editor.test.tsx @@ -7,31 +7,34 @@ const KEY_PREFIX = "gitmost:scroll-position:"; // NOTE ON SCOPE (F2 — reviewer-approved lighter variant). // -// 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, -// 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. +// The real UX wiring lives in the exported `useScrollRestoreOnSwap` hook (now TWO +// useLayoutEffect triggers around useScrollPosition — an early one on mount plus +// a post-swap one), 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, 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 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). +// post-swap `&& editor` guard or changing the effect deps makes these tests fail; +// they guard the production code directly (verified: removing `&& editor` reddens +// test A's call-count assertion). // -// 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. +// Both tests observe the real effect via `window.scrollTo`. Restore is not +// synchronous: the hook schedules a poll that waits for the document height to +// stay stable for HEIGHT_STABLE_MS, so the tests drive fake timers and assert the +// eventual `scrollTo({ top, behavior: "auto" })`. The stubbed `window.scrollTo` +// never mutates `window.scrollY`, so each effective restore produces exactly one +// call — the test A guard is pinned via that call count. function setScrollY(value: number): void { Object.defineProperty(window, "scrollY", { configurable: true, value }); @@ -80,61 +83,60 @@ describe("PageEditor scroll-restore wiring (useScrollRestoreOnSwap)", () => { window.location.hash = ""; }); - 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. + it("early trigger restores on mount; post-swap re-assert is gated by `&& editor`", () => { + vi.useFakeTimers(); + // Target reachable (maxScroll = 2000 - 800 = 1200 >= 500) and the height is + // held steady, so each restore poll settles and scrolls to 500. window.sessionStorage.setItem(`${KEY_PREFIX}guard`, "500"); setInnerHeight(800); - setScrollHeight(2000); // maxScroll = 1200 >= 500: reachable, no polling. + setScrollHeight(2000); // reachable + held steady -> the poll settles - // 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( , ); + // Early trigger's poll settles (>= HEIGHT_STABLE_MS) and restores once — this + // is the offline / collab-never-syncs path (no swap needed). + act(() => { + vi.advanceTimersByTime(500); + }); expect(window.scrollTo).toHaveBeenCalledTimes(1); - // Collab reports synced (showStatic flips false) but the editor is not ready - // 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.) + // showStatic flips false but editor still null: the post-swap effect re-runs + // but its `&& editor` guard must block it — no new restore. (Dropping + // `&& editor` would start a fresh poll and produce a 2nd call, failing this.) rerender(); + act(() => { + vi.advanceTimersByTime(500); + }); 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, driven solely by - // the [showStatic, editor] deps changing. + // editor present: the post-swap effect re-invokes restore -> a fresh poll + // settles and re-asserts (scrollY stubbed at 0, so it scrolls again). rerender(); + act(() => { + vi.advanceTimersByTime(500); + }); 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 (inside the hook): the swap - // re-invocation is the CAUSE of the scroll (nothing scrolls before it). + it("early trigger restores WITHOUT any swap once the static content settles tall enough", () => { + // Directly covers the offline regression: collab never syncs, so showStatic + // stays true and the editor stays null — yet the reading position is restored + // against the static cache's own layout, not left stuck at the top. vi.useFakeTimers(); - window.sessionStorage.setItem(`${KEY_PREFIX}peg`, "500"); + window.sessionStorage.setItem(`${KEY_PREFIX}offline`, "500"); setInnerHeight(800); - setScrollHeight(100); // maxScroll = -700: target not reachable yet -> polls. + setScrollHeight(100); // too short at first -> the poll keeps waiting - // 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. + render(); act(() => { - rerender(); + vi.advanceTimersByTime(500); + }); + expect(window.scrollTo).not.toHaveBeenCalled(); // still short, no swap + + setScrollHeight(2000); // static content finished laying out + act(() => { + vi.advanceTimersByTime(500); }); 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 b001051f..2c7d8231 100644 --- a/apps/client/src/features/editor/page-editor.tsx +++ b/apps/client/src/features/editor/page-editor.tsx @@ -481,9 +481,10 @@ export default function PageEditor({ } }, [yjsConnectionStatus, isSynced]); - // 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. + // Reading-position restore. The wiring (an early on-mount trigger that also + // covers the offline / collab-never-syncs path, plus a post-swap re-assert for + // the final layout) lives in useScrollRestoreOnSwap; both wait for the document + // height to settle and abort if the reader scrolls first. useScrollRestoreOnSwap(pageId, editor, showStatic); return ( diff --git a/apps/client/src/features/editor/title-editor.tsx b/apps/client/src/features/editor/title-editor.tsx index 0b1fb924..b57dc93f 100644 --- a/apps/client/src/features/editor/title-editor.tsx +++ b/apps/client/src/features/editor/title-editor.tsx @@ -1,5 +1,5 @@ import "@/features/editor/styles/index.css"; -import React, { useCallback, useEffect, useState } from "react"; +import React, { useCallback, useEffect, useRef, useState } from "react"; import { EditorContent, useEditor } from "@tiptap/react"; import { Document } from "@tiptap/extension-document"; import { Heading } from "@tiptap/extension-heading"; @@ -11,6 +11,7 @@ import { pageEditorAtom, titleEditorAtom, } from "@/features/editor/atoms/editor-atoms"; +import { hasSavedReadingPosition } from "@/features/editor/hooks/use-scroll-position"; import { updatePageData, useUpdateTitlePageMutation, @@ -54,6 +55,15 @@ export function TitleEditor({ const [activePageId, setActivePageId] = useState(pageId); const currentPageEditMode = useAtomValue(currentPageEditModeAtom); + // Captured synchronously on first render (before any scroll handler can clobber + // the stored value): does this page have a saved reading position that will be + // restored (scrolling away from the top)? If so we must NOT auto-focus the + // title, or the caret lands in the now-off-screen title. + const hasSavedScrollRef = useRef(null); + if (hasSavedScrollRef.current === null) { + hasSavedScrollRef.current = hasSavedReadingPosition(pageId); + } + const titleEditor = useEditor({ extensions: [ Document.extend({ @@ -168,10 +178,17 @@ export function TitleEditor({ }, [pageId, title, titleEditor]); useEffect(() => { + // Skip title auto-focus when a saved reading position will be restored: the + // viewport scrolls away from the top, so focusing the top-of-page title would + // drop the caret off-screen (the first keystroke would go into the hidden title). + if (hasSavedScrollRef.current) return; setTimeout(() => { // guard against Cannot access view['hasFocus'] error if (!titleEditor?.isInitialized) return; - titleEditor?.commands?.focus("end"); + // Focus WITHOUT scrolling: TipTap's focus scrolls the focused node into + // view by default, and the title sits at the very top of the page, which + // would fight scroll-position restoration. We only want the caret placed. + titleEditor?.commands?.focus("end", { scrollIntoView: false }); }, 300); }, [titleEditor]);