Compare commits

...

1 Commits

Author SHA1 Message Date
claude_code 882a6bb032 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>
2026-07-03 16:49:13 +03:00
3 changed files with 165 additions and 116 deletions
@@ -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", () => {
@@ -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]);
@@ -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" });
});