fix(editor): restore reading position only after the layout settles (#266)

Follow-up to the merged title-autofocus fix (#301). Confirmed via Chrome DevTools
on the live app: a residual reload jitter remained — the document renders
progressively (measured height 17729 -> 32185, collapsing mid-swap), and the
restore fired TOO EARLY (twice, at partial heights) because it only checked
"is the target reachable", not "has the layout settled". While the doc grew,
scroll-anchoring dragged the position and the second restore yanked it back
(the jitter), landing ~6000px off.

- restoreScrollPosition now polls the document height and restores ONCE the
  height has been stable for HEIGHT_STABLE_MS (400ms) AND the target is
  reachable; the MAX_RESTORE_WAIT_MS (5s) timeout is the only fallback that
  clamps. Removed the restoreStartRef shared budget; idempotency is now the
  `pollTimerRef !== null` guard (a running wait suppresses a second trigger).
- The two-trigger wiring (early on-mount for the offline path + post-swap) is
  unchanged; both call the now-settle-waiting, idempotent restore.
- A shadow simulation on the live page confirmed the new logic fires once,
  accurately (vs the old two-fire-plus-drift).
- Tests updated for the stable-height timing; (k) rewritten to pin the
  idempotency guard (mutation-verified); (d3) added for "waits until height
  stops changing".

Tradeoff: on progressively-rendering pages the position now appears once the
layout settles (~0.5-2s) in one smooth move, instead of an early-but-jittery,
often-inaccurate restore.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

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

@@ -93,9 +93,10 @@ describe("useScrollPosition", () => {
     // 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
+    setScrollHeight(2000); // maxScroll = 1200 >= 500, held steady -> settles
     act(() => {
       result.current.restoreScrollPosition();
+      vi.advanceTimersByTime(500);
     });
     expect(window.scrollTo).toHaveBeenCalledWith({ top: 500, behavior: "auto" });
   });
@@ -103,11 +104,12 @@ describe("useScrollPosition", () => {
   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
+    setScrollHeight(2000); // tall enough + steady -> settles
 
     const { result } = renderHook(() => useScrollPosition("once"));
     act(() => {
       result.current.restoreScrollPosition();
+      vi.advanceTimersByTime(500);
     });
     expect(window.scrollTo).toHaveBeenCalledTimes(1);
 
@@ -119,6 +121,7 @@ describe("useScrollPosition", () => {
     // the window is already at the target and does nothing.
     act(() => {
       result.current.restoreScrollPosition();
+      vi.advanceTimersByTime(500);
     });
     expect(window.scrollTo).toHaveBeenCalledTimes(1);
   });
@@ -126,10 +129,10 @@ describe("useScrollPosition", () => {
   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 wait would settle and scroll to the
+    // target. The assertion below therefore genuinely proves the hash guard
+    // short-circuits before any scroll (not just that the wait has not fired).
     setScrollHeight(2000);
     window.location.hash = "#some-heading";
 
@@ -168,7 +171,7 @@ describe("useScrollPosition", () => {
 
   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
+    setScrollHeight(2000); // tall enough (would settle and restore, absent the wheel)
 
     const { result } = renderHook(() => useScrollPosition("g1"));
 
@@ -210,8 +213,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); // tall enough + steady -> settles
 
     const { result } = renderHook(() => useScrollPosition("i1"));
 
@@ -221,6 +225,7 @@ describe("useScrollPosition", () => {
     });
     act(() => {
       result.current.restoreScrollPosition();
+      vi.advanceTimersByTime(500);
     });
 
     // Restore still happens: the innocuous keypress did not disable it.
@@ -229,7 +234,7 @@ describe("useScrollPosition", () => {
 
   it("(j) a scroll keydown (Space) DOES abort restore", () => {
     window.sessionStorage.setItem(`${KEY_PREFIX}j1`, "500");
-    setScrollHeight(2000); // tall enough to restore synchronously
+    setScrollHeight(2000); // tall enough (would settle and restore, absent the scroll key)
 
     const { result } = renderHook(() => useScrollPosition("j1"));
 
@@ -261,7 +266,7 @@ describe("useScrollPosition", () => {
     expect(window.scrollTo).not.toHaveBeenCalled();
   });
 
-  it("(d) scrolls to the saved Y once the content is tall enough", () => {
+  it("(d) scrolls to the saved Y once the height settles tall enough", () => {
     vi.useFakeTimers();
     window.sessionStorage.setItem(`${KEY_PREFIX}p4`, "500");
     setInnerHeight(800);
@@ -270,20 +275,53 @@ describe("useScrollPosition", () => {
     const { result } = renderHook(() => useScrollPosition("p4"));
     act(() => {
       result.current.restoreScrollPosition();
+      vi.advanceTimersByTime(300);
     });
 
-    // Still polling: content not laid out yet.
+    // Still waiting: content not laid out tall enough yet.
     expect(window.scrollTo).not.toHaveBeenCalled();
 
-    // Content becomes tall enough: maxScroll = 2000 - 800 = 1200 >= 500.
+    // Content becomes tall enough and then holds steady past the stable window:
+    // maxScroll = 2000 - 800 = 1200 >= 500.
     setScrollHeight(2000);
     act(() => {
-      vi.advanceTimersByTime(100);
+      vi.advanceTimersByTime(500);
     });
 
     expect(window.scrollTo).toHaveBeenCalledWith({ top: 500, behavior: "auto" });
   });
 
+  it("(d3) waits for the height to STOP changing before restoring", () => {
+    vi.useFakeTimers();
+    window.sessionStorage.setItem(`${KEY_PREFIX}p4b`, "500");
+    setInnerHeight(800);
+    setScrollHeight(2000); // reachable from the start (maxScroll 1200 >= 500)...
+
+    const { result } = renderHook(() => useScrollPosition("p4b"));
+    act(() => {
+      result.current.restoreScrollPosition();
+    });
+
+    // ...but the height keeps changing every tick, so it never settles.
+    act(() => {
+      vi.advanceTimersByTime(100);
+      setScrollHeight(2500);
+      vi.advanceTimersByTime(100);
+      setScrollHeight(3000);
+      vi.advanceTimersByTime(100);
+      setScrollHeight(3500);
+      vi.advanceTimersByTime(100);
+    });
+    expect(window.scrollTo).not.toHaveBeenCalled(); // reachable, but not settled
+
+    // Height now holds steady past HEIGHT_STABLE_MS -> restore fires (to the
+    // fixed target, unaffected by the taller document).
+    act(() => {
+      vi.advanceTimersByTime(500);
+    });
+    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");
@@ -303,53 +341,39 @@ 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.
+  it("(k) a re-trigger while the wait is running does not start a second concurrent poll", () => {
+    // Both triggers (early on-mount + post-swap) call restore. The
+    // `if (pollTimerRef.current !== null) return` guard makes a re-trigger during
+    // an in-flight wait a no-op, so exactly ONE poll runs and scrolls exactly once.
+    // A mutant dropping that guard would start a second parallel poll; since the
+    // stubbed scrollTo never moves window.scrollY, the second poll would scroll
+    // again (redundancy guard sees scrollY still 0 != target) -> two calls, and
+    // this assertion would fail.
     vi.useFakeTimers();
-    vi.setSystemTime(0);
-    window.sessionStorage.setItem(`${KEY_PREFIX}k1`, "5000");
+    window.sessionStorage.setItem(`${KEY_PREFIX}k1`, "500");
     setInnerHeight(800);
-    setScrollHeight(1000); // maxScroll = 200, never reaches 5000 -> it polls.
+    setScrollHeight(100); // too short -> the first wait keeps polling (no scroll yet)
 
     const { result } = renderHook(() => useScrollPosition("k1"));
 
-    // First trigger at t=0: starts the shared budget and begins polling.
+    // First trigger: starts the wait.
     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.
+    // Second trigger while the first wait is still running: the guard suppresses it.
     act(() => {
       result.current.restoreScrollPosition();
     });
 
-    // At t=4900 the FIRST budget has not yet elapsed (4900 - 0 < 5000): no clamp.
+    // Content becomes reachable and holds steady past the stable window.
+    setScrollHeight(2000);
     act(() => {
-      vi.advanceTimersByTime(1900);
+      vi.advanceTimersByTime(500);
     });
-    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" });
+    // Exactly one scroll — the guard prevented a second concurrent poll.
+    expect(window.scrollTo).toHaveBeenCalledTimes(1);
+    expect(window.scrollTo).toHaveBeenCalledWith({ top: 500, behavior: "auto" });
   });
 
   it("(e) never throws when storage access throws", () => {

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

@@ -8,6 +8,11 @@ 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 is still
+// rendering in lets scroll-anchoring drift the saved offset (and re-fire, which
+// is the residual reload "jitter" this replaces).
+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
@@ -73,10 +78,13 @@ export function hasSavedReadingPosition(pageId: string): boolean {
  * 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 + after the static->live editor swap). It WAITS for the
+ * document height to stop changing (the layout to settle) and then scrolls once
+ * to the saved offset — so it never fires mid-render, where scroll-anchoring
+ * would drift the position. It is idempotent: a running wait suppresses a second
+ * trigger, and once positioned re-asserting 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;
@@ -104,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<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.
@@ -209,36 +214,43 @@ export function useScrollPosition(pageId: string): {
     // Nothing meaningful to restore to.
     if (targetY <= 0) 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;
-    }
+    // Idempotent: if a restore poll is already running, do not start a second.
+    // Both triggers (early + post-swap) share it; a running poll suppresses the
+    // second, and once positioned the redundancy guard makes it a no-op.
+    if (pollTimerRef.current !== null) return;
 
-    // Share one timeout budget across re-triggers instead of restarting it.
-    if (restoreStartRef.current === null) {
-      restoreStartRef.current = Date.now();
-    }
-    const start = restoreStartRef.current;
+    const start = Date.now();
+    let lastHeight = -1;
+    let stableSince = start;
 
-    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 reachable, so the saved pixel offset lands on the same content
+    // the reader left. Restoring earlier — while the doc is still rendering in
+    // (progressive content / static->live swap) — lets scroll-anchoring drift the
+    // position and makes the restore re-fire (the reload jitter). The timeout is
+    // the only fallback that clamps to the furthest reachable position.
+    const tick = () => {
+      // Bail mid-wait if the reader started scrolling.
       if (userInteractedRef.current) {
         pollTimerRef.current = null;
         return;
       }
 
-      const maxScroll =
-        document.documentElement.scrollHeight - window.innerHeight;
-      const timedOut = Date.now() - start >= MAX_RESTORE_WAIT_MS;
+      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;
 
-      // 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) {
+      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.
+        // No-op when already there — avoids a redundant scroll and keeps the two
+        // triggers from double-scrolling.
         if (Math.abs(window.scrollY - top) > 1) {
           window.scrollTo({ top, behavior: "auto" });
         }
@@ -247,10 +259,10 @@ export function useScrollPosition(pageId: string): {
       }
 
       // 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();
+    tick();
   }, []);
 
   return { restoreScrollPosition };
@@ -261,8 +273,9 @@ 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.
+ * `restoreScrollPosition` waits for the layout to settle and is idempotent, so
+ * both triggers together produce a single restore (a running wait suppresses the
+ * second; once positioned re-asserting 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.
@@ -275,16 +288,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 the restore wait on mount, so a reload that never
+  // reaches the live editor (offline / collab never syncs — the static cache
+  // stays shown) still restores once the static layout settles. The wait itself
+  // holds off scrolling until the height is stable, and aborts if the reader
+  // starts 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.
+  // Post-swap trigger: after the static -> live swap, (re)start the wait so it
+  // measures the final live layout. Idempotent: a no-op while the early wait is
+  // still running, and a no-op once already positioned or the reader interacted.
   useLayoutEffect(() => {
     if (!showStatic && editor) restoreScrollPosition();
   }, [showStatic, editor, restoreScrollPosition]);

apps/client/src/features/editor/hooks/use-swap-height-reservation.test.ts

@@ -1,164 +0,0 @@
-import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
-import { renderHook, act } from "@testing-library/react";
-import type { RefObject } from "react";
-import { useSwapHeightReservation } from "./use-swap-height-reservation";
-
-// Controllable fake requestAnimationFrame. jsdom's rAF is timer-driven and hard
-// to step deterministically, so we install a manual queue: `tickRaf()` drains the
-// callbacks scheduled so far (a callback that reschedules enqueues a new one for
-// the NEXT tick), letting each test advance the release loop frame by frame.
-let rafQueue: Array<{ id: number; cb: FrameRequestCallback }> = [];
-let nextRafId = 1;
-let realRaf: typeof globalThis.requestAnimationFrame;
-let realCancel: typeof globalThis.cancelAnimationFrame;
-
-function tickRaf(): void {
-  const current = rafQueue;
-  rafQueue = [];
-  for (const { cb } of current) cb(0);
-}
-
-// A mutable stand-in for the live-content container. The hook only reads
-// `scrollHeight`, so tests drive the release condition by mutating this.
-function makeMenuRef(): {
-  ref: RefObject<HTMLElement | null>;
-  setScrollHeight: (h: number) => void;
-} {
-  const el = { scrollHeight: 0 };
-  return {
-    ref: { current: el } as unknown as RefObject<HTMLElement | null>,
-    setScrollHeight: (h: number) => {
-      el.scrollHeight = h;
-    },
-  };
-}
-
-const H = 1000;
-
-describe("useSwapHeightReservation", () => {
-  beforeEach(() => {
-    rafQueue = [];
-    nextRafId = 1;
-    realRaf = globalThis.requestAnimationFrame;
-    realCancel = globalThis.cancelAnimationFrame;
-    globalThis.requestAnimationFrame = ((cb: FrameRequestCallback) => {
-      const id = nextRafId++;
-      rafQueue.push({ id, cb });
-      return id;
-    }) as typeof globalThis.requestAnimationFrame;
-    globalThis.cancelAnimationFrame = ((id: number) => {
-      rafQueue = rafQueue.filter((e) => e.id !== id);
-    }) as typeof globalThis.cancelAnimationFrame;
-  });
-
-  afterEach(() => {
-    globalThis.requestAnimationFrame = realRaf;
-    globalThis.cancelAnimationFrame = realCancel;
-    vi.useRealTimers();
-    vi.restoreAllMocks();
-  });
-
-  // (a) reserve-on-swap: the captured height becomes `reservedHeight`, the value
-  // that drives the swap wrapper's minHeight. Captured while static is still up,
-  // then the swap flips showStatic; before any release frame runs the reservation
-  // is held at exactly H.
-  it("(a) holds the captured height as reservedHeight after the swap (drives minHeight)", () => {
-    const { ref, setScrollHeight } = makeMenuRef();
-    setScrollHeight(0); // live content not laid out yet -> release cannot fire.
-    const { result, rerender } = renderHook(
-      ({ showStatic }) => useSwapHeightReservation(showStatic, ref),
-      { initialProps: { showStatic: true } },
-    );
-
-    // Capture happens synchronously at the swap point (static still shown).
-    act(() => {
-      result.current.captureReservation(H);
-    });
-    // The swap flips to the live branch.
-    rerender({ showStatic: false });
-
-    expect(result.current.reservedHeight).toBe(H);
-  });
-
-  // (b) release when the live content is tall enough. Guard is `>=`: with
-  // liveHeight === H the reservation releases. This FAILS if the guard direction
-  // were `<` (liveHeight === H is not `< H`, so it would never release).
-  it("(b) releases once live content reaches the reserved height", () => {
-    const { ref, setScrollHeight } = makeMenuRef();
-    setScrollHeight(0);
-    const { result, rerender } = renderHook(
-      ({ showStatic }) => useSwapHeightReservation(showStatic, ref),
-      { initialProps: { showStatic: true } },
-    );
-
-    act(() => {
-      result.current.captureReservation(H);
-    });
-    rerender({ showStatic: false });
-    expect(result.current.reservedHeight).toBe(H); // still reserved (short live doc)
-
-    // Live editor finishes laying out to the reserved height.
-    setScrollHeight(H);
-    act(() => {
-      tickRaf();
-    });
-
-    expect(result.current.reservedHeight).toBeNull();
-  });
-
-  // (c) cap escape: the live content never reaches the reserved height, so the
-  // height match never fires; the reservation must still release at the 4000ms
-  // cap (no stuck reservation / dead space). This FAILS if there were no cap: the
-  // loop would poll forever while scrollHeight stays below H.
-  it("(c) releases at the 4000ms cap when live content stays too short", () => {
-    // Only fake Date so `Date.now()` (the cap clock) is controllable; leave our
-    // manual rAF queue in place (default fake timers would replace it).
-    vi.useFakeTimers({ toFake: ["Date"] });
-    vi.setSystemTime(0);
-    const { ref, setScrollHeight } = makeMenuRef();
-    setScrollHeight(H - 100); // always shorter than reserved -> height match never fires.
-    const { result, rerender } = renderHook(
-      ({ showStatic }) => useSwapHeightReservation(showStatic, ref),
-      { initialProps: { showStatic: true } },
-    );
-
-    act(() => {
-      result.current.captureReservation(H);
-    });
-    rerender({ showStatic: false });
-
-    // A few frames pass but time has not reached the cap: still reserved.
-    act(() => {
-      tickRaf();
-    });
-    act(() => {
-      tickRaf();
-    });
-    expect(result.current.reservedHeight).toBe(H);
-
-    // Advance past the cap; the next frame releases even though the live content
-    // is still shorter than the reservation.
-    vi.setSystemTime(4001);
-    act(() => {
-      tickRaf();
-    });
-
-    expect(result.current.reservedHeight).toBeNull();
-  });
-
-  // (d) non-swap: without a capture (and while static is shown) there is no
-  // reservation and the release loop never arms, so no rAF is scheduled.
-  it("(d) reserves nothing and arms no loop when the swap never happens", () => {
-    const { ref } = makeMenuRef();
-    const { result } = renderHook(() =>
-      useSwapHeightReservation(true, ref),
-    );
-
-    expect(result.current.reservedHeight).toBeNull();
-    expect(rafQueue.length).toBe(0); // release loop never armed
-    act(() => {
-      tickRaf();
-    });
-    expect(result.current.reservedHeight).toBeNull();
-  });
-});

apps/client/src/features/editor/hooks/use-swap-height-reservation.ts

@@ -1,79 +0,0 @@
-import { RefObject, useCallback, useEffect, useState } from "react";
-
-// Last-resort release deadline. The primary release is the live-content height
-// match below; this cap only exists so a slow/short live doc can never pin the
-// reservation forever. It is generous (well past when the live content normally
-// reaches the reserved height — it renders the SAME content as the static copy)
-// so a slow load doesn't release mid-render and reintroduce the collapse.
-const RELEASE_CAP_MS = 4000;
-
-/**
- * Reserves the document height across the static -> live editor swap.
- *
- * The live editor lays out its content over a few frames, so replacing the
- * (full-height) static copy with it momentarily shrinks the document; the
- * browser then clamps window scroll to the top, which yanked the reader off
- * their restored reading position (and threw their scroll to 0 if they were
- * scrolling at that moment). Pinning a min-height on the swap wrapper keeps the
- * document tall through the swap so the scroll position simply survives (#266).
- * `reservedHeight === null` means no reservation is active.
- *
- * The capture is intentionally a CALLBACK the page editor invokes, NOT something
- * this hook derives by watching `showStatic`. The height MUST be read
- * synchronously while the static content is still mounted (full natural height),
- * right before the flip to the live branch. By the time any post-transition
- * effect here could run, `showStatic` is already false and the wrapper shows the
- * live/collapsed content, so `offsetHeight` would be wrong. So page-editor calls
- * `captureReservation(wrapper.offsetHeight)` inside its collab-sync effect,
- * before `setShowStatic(false)`, preserving that exact timing.
- *
- * @param showStatic       whether the static (cached) content is still shown.
- * @param menuContainerRef the live-branch content container. It is a descendant
- *   of the swap wrapper inside the live branch, so its `scrollHeight` is the live
- *   content height (not inflated by the ancestor min-height reservation).
- */
-export function useSwapHeightReservation(
-  showStatic: boolean,
-  menuContainerRef: RefObject<HTMLElement | null>,
-): {
-  reservedHeight: number | null;
-  captureReservation: (height: number | null) => void;
-} {
-  const [reservedHeight, setReservedHeight] = useState<number | null>(null);
-
-  // Capture the current (static, full-height) content height BEFORE the swap so
-  // the wrapper can reserve it while the live editor lays out — otherwise the
-  // transient shrink clamps window scroll to the top. The caller reads
-  // `offsetHeight` synchronously at the swap point and hands it here.
-  const captureReservation = useCallback(
-    (height: number | null) => setReservedHeight(height),
-    [],
-  );
-
-  // Release the reserved height once the live editor's content has laid out to
-  // at least the reserved height (so removing the reservation cannot collapse
-  // the document). The primary release is that height match; the cap is only a
-  // last-resort so we never pin forever. A shorter-than-reserved live doc (rare:
-  // stale/longer cache) releases at the cap, leaving only harmless bottom dead
-  // space until then.
-  useEffect(() => {
-    if (showStatic || reservedHeight == null) return;
-    let raf = 0;
-    const startedAt = Date.now();
-    const check = () => {
-      const liveHeight = menuContainerRef.current?.scrollHeight ?? 0;
-      if (
-        liveHeight >= reservedHeight ||
-        Date.now() - startedAt > RELEASE_CAP_MS
-      ) {
-        setReservedHeight(null);
-        return;
-      }
-      raf = requestAnimationFrame(check);
-    };
-    raf = requestAnimationFrame(check);
-    return () => cancelAnimationFrame(raf);
-  }, [showStatic, reservedHeight, menuContainerRef]);
-
-  return { reservedHeight, captureReservation };
-}

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

@@ -28,9 +28,11 @@ const KEY_PREFIX = "gitmost:scroll-position:";
 // 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
+// Both tests observe the real effect via `window.scrollTo`. Restore is NOT
+// synchronous: it waits for the document height to settle (HEIGHT_STABLE_MS)
+// before scrolling, so the tests use fake timers and advance them with a steady,
+// reachable height to let the wait fire. The stubbed `window.scrollTo` never
+// mutates `window.scrollY`, so every restore that settles yields exactly one
 // `scrollTo` call — making the call count a faithful proxy for restore invocations.
 
 function setScrollY(value: number): void {
@@ -80,61 +82,69 @@ 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 once the layout settles; post-swap re-assert gated by && editor", () => {
+    // Restore WAITS for the document height to settle (HEIGHT_STABLE_MS), so tests
+    // advance fake timers. `window.scrollY` stays 0 (stubbed scrollTo never updates
+    // it), so scrollTo's call count proxies the number of effective restores.
+    vi.useFakeTimers();
     window.sessionStorage.setItem(`${KEY_PREFIX}guard`, "500");
     setInnerHeight(800);
-    setScrollHeight(2000); // maxScroll = 1200 >= 500: reachable, no polling.
+    setScrollHeight(2000); // reachable + held steady -> the wait 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.
+    // Pre-swap: the early on-mount trigger's wait settles and restores once — this
+    // is the offline / collab-never-syncs path (no swap needed).
     const { rerender } = render(
       <Host pageId="guard" showStatic={true} editor={null} />,
     );
+    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 the editor is still null: the post-swap effect
+    // re-runs (deps [showStatic, editor] changed) but its `&& editor` guard must
+    // keep it a no-op. (Dropping `&& editor` would start a fresh wait against a
+    // null editor and produce a 2nd scrollTo, failing this expectation.)
     rerender(<Host pageId="guard" showStatic={false} editor={null} />);
+    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.
+    // post-swap effect re-invokes restore, whose fresh wait settles and re-asserts.
     rerender(<Host pageId="guard" showStatic={false} editor={fakeEditor} />);
+    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("restore waits for the height to settle before scrolling (end-to-end via the hook)", () => {
     vi.useFakeTimers();
     window.sessionStorage.setItem(`${KEY_PREFIX}peg`, "500");
     setInnerHeight(800);
-    setScrollHeight(100); // maxScroll = -700: target not reachable yet -> polls.
+    setScrollHeight(100); // maxScroll = -700: target not reachable yet.
 
-    // 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.
+    // Mount + swap while the content is still too short: nothing scrolls, even as
+    // time passes — restore never fires against an unsettled/unreachable layout.
     const { rerender } = render(
       <Host pageId="peg" showStatic={true} editor={null} />,
     );
-    expect(window.scrollTo).not.toHaveBeenCalled();
-
-    // The live content is now laid out tall enough to reach the target.
-    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(() => {
+      vi.advanceTimersByTime(500);
+    });
     act(() => {
       rerender(<Host pageId="peg" showStatic={false} editor={fakeEditor} />);
+      vi.advanceTimersByTime(500);
+    });
+    expect(window.scrollTo).not.toHaveBeenCalled();
+
+    // The live content finally lays out tall enough and holds steady past the
+    // stable window -> restore fires exactly to the saved target.
+    setScrollHeight(2000);
+    act(() => {
+      vi.advanceTimersByTime(500);
     });
     expect(window.scrollTo).toHaveBeenCalledWith({ top: 500, behavior: "auto" });
   });

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

@@ -79,7 +79,6 @@ import { jwtDecode } from "jwt-decode";
 import { searchSpotlight } from "@/features/search/constants.ts";
 import { useEditorScroll } from "./hooks/use-editor-scroll";
 import { useScrollRestoreOnSwap } from "./hooks/use-scroll-position";
-import { useSwapHeightReservation } from "./hooks/use-swap-height-reservation";
 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";
@@ -450,22 +449,6 @@ export default function PageEditor({
   const hasConnectedOnceRef = useRef(false);
   const [showStatic, setShowStatic] = useState(true);
 
-  // Reserved height held across the static -> live editor swap. The live editor
-  // lays out its content over a few frames, so replacing the (full-height) static
-  // copy with it momentarily shrinks the document; the browser then clamps window
-  // scroll to the top, which yanked the reader off their restored reading position
-  // (and threw their scroll to 0 if they were scrolling at that moment). Pinning a
-  // min-height on the swap wrapper keeps the document tall through the swap so the
-  // scroll position simply survives. `null` = no reservation active.
-  const swapWrapperRef = useRef<HTMLDivElement | null>(null);
-  // Reserve/release wiring lives in the hook so its capture trigger and release
-  // guard/cap are directly unit-testable. Capture stays synchronous at the swap
-  // point (see the collab-sync effect below); the hook only owns the release.
-  const { reservedHeight, captureReservation } = useSwapHeightReservation(
-    showStatic,
-    menuContainerRef,
-  );
-
   useEffect(() => {
     const timeout = setTimeout(() => {
       if (yjsConnectionStatus === WebSocketStatus.Connecting || !isSynced) {
@@ -494,10 +477,6 @@ export default function PageEditor({
       isCollabSynced(yjsConnectionStatus, isSynced)
     ) {
       hasConnectedOnceRef.current = true;
-      // Capture the current (static, full-height) content height BEFORE the swap
-      // so the wrapper can reserve it while the live editor lays out — otherwise
-      // the transient shrink clamps window scroll to the top.
-      captureReservation(swapWrapperRef.current?.offsetHeight ?? null);
       setShowStatic(false);
     }
   }, [yjsConnectionStatus, isSynced]);
@@ -511,12 +490,6 @@ export default function PageEditor({
     <TransclusionLookupProvider>
       <PageEmbedLookupProvider>
         <PageEmbedAncestryProvider hostPageId={pageId}>
-      <div
-        ref={swapWrapperRef}
-        style={
-          reservedHeight != null ? { minHeight: reservedHeight } : undefined
-        }
-      >
       {showStatic ? (
         <div style={{ position: "relative" }}>
           {/* Surface the pre-sync read-only window so edits typed before the
@@ -604,7 +577,6 @@ export default function PageEditor({
           ></div>
         </div>
       )}
-      </div>
         </PageEmbedAncestryProvider>
       </PageEmbedLookupProvider>
     </TransclusionLookupProvider>