fix(editor): smooth scroll-position restore, no yank on early scroll (#266)

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 <noreply@anthropic.com>

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"));

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
   // `<MemoizedFullEditor key={page.id} ...>`, 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<number | null>(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<number | null>(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<number | null>(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;
       }

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]);