fix(editor): плавное восстановление позиции чтения без рывка (#266) #289

Merged
vvzvlad merged 3 commits from feat/scroll-restore-ux into develop 2026-07-03 00:33:19 +03:00
4 changed files with 402 additions and 28 deletions
@@ -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"));
@@ -221,6 +303,55 @@ describe("useScrollPosition", () => {
expect(window.scrollTo).toHaveBeenCalledWith({ top: 200, behavior: "auto" });
});
it("(k) shares ONE timeout budget across re-triggers (does not restart the clock)", () => {
// The static->live editor swap re-invokes restore. The shared budget
// (restoreStartRef) must measure the MAX_RESTORE_WAIT_MS (5000) deadline
// from the FIRST trigger, not restart it on every re-trigger. This pins
// the `if (restoreStartRef.current === null)` guard: a mutant that resets
// `restoreStartRef.current = Date.now()` on every trigger would push the
// deadline out to t=8000 (3000 + 5000) and fail the t=5000 assertion below.
vi.useFakeTimers();
vi.setSystemTime(0);
window.sessionStorage.setItem(`${KEY_PREFIX}k1`, "5000");
setInnerHeight(800);
setScrollHeight(1000); // maxScroll = 200, never reaches 5000 -> it polls.
const { result } = renderHook(() => useScrollPosition("k1"));
// First trigger at t=0: starts the shared budget and begins polling.
act(() => {
result.current.restoreScrollPosition();
});
expect(window.scrollTo).not.toHaveBeenCalled();
// Advance to t=3000 (still polling: content short, not yet timed out).
act(() => {
vi.advanceTimersByTime(3000);
});
expect(window.scrollTo).not.toHaveBeenCalled();
// Second trigger at t=3000 (the swap re-assert). Under the real code the
// budget is shared, so `start` stays 0; under the reset-mutant it becomes 3000.
act(() => {
result.current.restoreScrollPosition();
});
// At t=4900 the FIRST budget has not yet elapsed (4900 - 0 < 5000): no clamp.
act(() => {
vi.advanceTimersByTime(1900);
});
expect(window.scrollTo).not.toHaveBeenCalled();
// At t=5000 the shared budget (measured from t=0) times out and clamps to the
// furthest reachable position (maxScroll = 200). The reset-mutant, measuring
// from t=3000, would still be waiting (5000 - 3000 = 2000 < 5000) and would
// NOT have scrolled here -> this assertion fails against that mutant.
act(() => {
vi.advanceTimersByTime(100);
});
expect(window.scrollTo).toHaveBeenCalledWith({ top: 200, behavior: "auto" });
});
it("(e) never throws when storage access throws", () => {
const err = new Error("storage denied");
vi.spyOn(window.sessionStorage, "getItem").mockImplementation(() => {
@@ -1,4 +1,5 @@
import { useCallback, useEffect, useRef } from "react";
import { useCallback, useEffect, useLayoutEffect, useRef } from "react";
import type { Editor } from "@tiptap/react";
// Throttle interval for persisting the scroll position while the user reads.
const SAVE_THROTTLE_MS = 250;
@@ -13,6 +14,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 +61,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 +136,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 +188,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 +198,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 +225,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;
}
@@ -175,3 +244,37 @@ export function useScrollPosition(pageId: string): {
return { restoreScrollPosition };
}
/**
* Wires `useScrollPosition` to the page editor's static->live swap lifecycle.
*
* Extracted from PageEditor so the exact restore triggers (their deps and the
* post-swap `&& editor` guard) are directly unit-testable rather than mirrored.
* Behaviour is unchanged: `restoreScrollPosition` is idempotent, so re-asserting
* the same target from either trigger is a no-op.
*
* @param pageId the page whose scroll position is persisted/restored.
* @param editor the tiptap editor instance, or `null` until it is ready.
* @param showStatic whether the static (cached) content is still shown.
*/
export function useScrollRestoreOnSwap(
pageId: string,
editor: Editor | null,
showStatic: boolean,
): void {
const { restoreScrollPosition } = useScrollPosition(pageId);
// Restore as early as the static (cached) content is laid out, before paint,
// so the reader's position is applied without a visible jump. Aborts itself if
// the reader has already started scrolling (handled inside the hook).
useLayoutEffect(() => {
restoreScrollPosition();
}, [restoreScrollPosition]);
// Re-assert once after the static -> live editor swap in case the swap reset
// the window scroll. Idempotent: a no-op when the position is already correct,
// and a no-op after the reader has interacted.
useLayoutEffect(() => {
if (!showStatic && editor) restoreScrollPosition();
}, [showStatic, editor, restoreScrollPosition]);
}
@@ -0,0 +1,141 @@
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
import { render, act } from "@testing-library/react";
import type { Editor } from "@tiptap/react";
import { useScrollRestoreOnSwap } from "./hooks/use-scroll-position";
const KEY_PREFIX = "gitmost:scroll-position:";
// NOTE ON SCOPE (F2 — reviewer-approved lighter variant).
//
// The real UX wiring lives in 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.
//
// So this file tests the REAL `useScrollRestoreOnSwap` hook — the exact code
// PageEditor imports and calls — driving its `showStatic`/`editor` inputs the way
// the swap does. Because it exercises the real hook (not a copy), dropping the
// `&& editor` guard or changing the effect deps makes these tests fail; they
// guard the production code directly (verified: removing `&& editor` reddens the
// first test).
//
// Both tests observe the real effect via `window.scrollTo`. The stubbed
// `window.scrollTo` never mutates `window.scrollY`, and the target is left
// unreached, so every restore invocation that passes the guard yields exactly one
// `scrollTo` call — making the call count a faithful proxy for restore invocations.
function setScrollY(value: number): void {
Object.defineProperty(window, "scrollY", { configurable: true, value });
}
function setScrollHeight(value: number): void {
Object.defineProperty(document.documentElement, "scrollHeight", {
configurable: true,
value,
});
}
function setInnerHeight(value: number): void {
Object.defineProperty(window, "innerHeight", { configurable: true, value });
}
// Minimal stand-in for the tiptap editor: the hook only truthiness-checks it.
const fakeEditor = { id: "editor" } as unknown as Editor;
// Thin host that calls the REAL hook so a rerender drives showStatic/editor
// exactly like the page-editor swap does.
function Host({
pageId,
showStatic,
editor,
}: {
pageId: string;
showStatic: boolean;
editor: Editor | null;
}) {
useScrollRestoreOnSwap(pageId, editor, showStatic);
return null;
}
describe("PageEditor scroll-restore wiring (useScrollRestoreOnSwap)", () => {
beforeEach(() => {
window.sessionStorage.clear();
setScrollY(0);
setScrollHeight(0);
setInnerHeight(800);
window.scrollTo = vi.fn();
window.location.hash = "";
});
afterEach(() => {
vi.restoreAllMocks();
vi.useRealTimers();
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.
window.sessionStorage.setItem(`${KEY_PREFIX}guard`, "500");
setInnerHeight(800);
setScrollHeight(2000); // maxScroll = 1200 >= 500: reachable, no polling.
// Pre-swap: static content shown, live editor not ready. Only the early
// pre-paint restore fires; the post-swap effect's guard (!showStatic) blocks it.
const { rerender } = render(
<Host pageId="guard" showStatic={true} editor={null} />,
);
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.)
rerender(<Host pageId="guard" showStatic={false} editor={null} />);
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.
rerender(<Host pageId="guard" showStatic={false} editor={fakeEditor} />);
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).
vi.useFakeTimers();
window.sessionStorage.setItem(`${KEY_PREFIX}peg`, "500");
setInnerHeight(800);
setScrollHeight(100); // maxScroll = -700: target not reachable yet -> polls.
// 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(
<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(() => {
rerender(<Host pageId="peg" showStatic={false} editor={fakeEditor} />);
});
expect(window.scrollTo).toHaveBeenCalledWith({ top: 500, behavior: "auto" });
});
});
@@ -78,7 +78,7 @@ import { PageEditMode } from "@/features/user/types/user.types.ts";
import { jwtDecode } from "jwt-decode";
import { searchSpotlight } from "@/features/search/constants.ts";
import { useEditorScroll } from "./hooks/use-editor-scroll";
import { useScrollPosition } from "./hooks/use-scroll-position";
import { useScrollRestoreOnSwap } from "./hooks/use-scroll-position";
import { EditorLinkMenu } from "@/features/editor/components/link/link-menu";
import ColumnsMenu from "@/features/editor/components/columns/columns-menu.tsx";
import { TransclusionLookupProvider } from "@/features/editor/components/transclusion/transclusion-lookup-context";
@@ -143,7 +143,6 @@ export default function PageEditor({
[isComponentMounted],
);
const { handleScrollTo } = useEditorScroll({ canScroll });
const { restoreScrollPosition } = useScrollPosition(pageId);
// Providers only created once per pageId
const providersRef = useRef<{
local: IndexeddbPersistence;
@@ -482,10 +481,10 @@ export default function PageEditor({
}
}, [yjsConnectionStatus, isSynced]);
// Restore the saved reading position once the live content is laid out.
useEffect(() => {
if (!showStatic && editor) restoreScrollPosition();
}, [showStatic, editor, restoreScrollPosition]);
// Restore the reader's scroll position across the static -> live editor swap.
// The wiring (early pre-paint restore + post-swap re-assert) lives in the hook
// so its triggers/guard are directly unit-testable.
useScrollRestoreOnSwap(pageId, editor, showStatic);
return (
<TransclusionLookupProvider>