diff --git a/apps/client/src/features/editor/hooks/use-scroll-position.test.ts b/apps/client/src/features/editor/hooks/use-scroll-position.test.ts index 2008625b..1f0f1e2b 100644 --- a/apps/client/src/features/editor/hooks/use-scroll-position.test.ts +++ b/apps/client/src/features/editor/hooks/use-scroll-position.test.ts @@ -74,6 +74,51 @@ describe("useScrollPosition", () => { unmount(); }); + it("(a2) the restore target is captured at mount and survives a fresh scroll@0 clobber", () => { + vi.useFakeTimers(); + // A previous session saved 500. + window.sessionStorage.setItem(`${KEY_PREFIX}clob`, "500"); + + const { result } = renderHook(() => useScrollPosition("clob")); + + // On load the page is at the top; a scroll@0 fires and overwrites storage + // with 0. This is exactly the clobber the synchronous mount-capture defends + // against: the stored value becomes "0", but the target was already captured. + setScrollY(0); + act(() => { + window.dispatchEvent(new Event("scroll")); + }); + expect(window.sessionStorage.getItem(`${KEY_PREFIX}clob`)).toBe("0"); + + // Restore still scrolls to 500 (the captured target), NOT the clobbered 0. + // If the capture were moved into an effect (after handlers register), it + // would read the clobbered 0 and this assertion would fail. + setScrollHeight(2000); // maxScroll = 1200 >= 500 + act(() => { + result.current.restoreScrollPosition(); + }); + expect(window.scrollTo).toHaveBeenCalledWith({ top: 500, behavior: "auto" }); + }); + + it("(a3) restores at most once per mount even if called again", () => { + vi.useFakeTimers(); + window.sessionStorage.setItem(`${KEY_PREFIX}once`, "500"); + setScrollHeight(2000); // tall enough to restore synchronously + + const { result } = renderHook(() => useScrollPosition("once")); + act(() => { + result.current.restoreScrollPosition(); + }); + expect(window.scrollTo).toHaveBeenCalledTimes(1); + + // A second call (e.g. the wiring effect re-running on [showStatic, editor, + // restoreScrollPosition]) must NOT scroll again and yank the reader. + act(() => { + result.current.restoreScrollPosition(); + }); + expect(window.scrollTo).toHaveBeenCalledTimes(1); + }); + it("(b) does not restore when the URL has a #hash anchor", () => { vi.useFakeTimers(); window.sessionStorage.setItem(`${KEY_PREFIX}p2`, "500"); diff --git a/apps/client/src/features/editor/hooks/use-scroll-position.ts b/apps/client/src/features/editor/hooks/use-scroll-position.ts index 88e3894e..6a72f754 100644 --- a/apps/client/src/features/editor/hooks/use-scroll-position.ts +++ b/apps/client/src/features/editor/hooks/use-scroll-position.ts @@ -25,7 +25,11 @@ function readStorage(pageId: string): number | null { if (raw === null) return null; const value = Number.parseInt(raw, 10); return Number.isFinite(value) ? value : null; - } catch { + } catch (err) { + // Best-effort feature: storage may be unavailable (private mode / quota). + // No user-facing notification (a missed scroll restore is not actionable), + // but log per the AGENTS.md "errors must never be swallowed" rule. + console.warn("[useScrollPosition] sessionStorage read failed", err); return null; } } @@ -33,8 +37,10 @@ function readStorage(pageId: string): number | null { function writeStorage(pageId: string, scrollY: number): void { try { window.sessionStorage.setItem(storageKey(pageId), String(Math.round(scrollY))); - } catch { - // Silently ignore: storage unavailable (private mode / quota exceeded). + } catch (err) { + // Storage unavailable (private mode / quota). Non-actionable for the user, + // but log it rather than swallow silently (AGENTS.md error-handling rule). + console.warn("[useScrollPosition] sessionStorage write failed", err); } } @@ -50,6 +56,14 @@ function writeStorage(pageId: string, scrollY: number): void { export function useScrollPosition(pageId: string): { restoreScrollPosition: () => void; } { + // CONTRACT: this hook assumes PageEditor REMOUNTS per page — page.tsx renders + // ``, so switching pages creates a fresh + // hook instance with fresh refs. 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. + // // 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".