From 30cdd65b929f24135c53adda9b133bc82aa96619 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Tue, 30 Jun 2026 12:13:44 +0300 Subject: [PATCH] test/refactor(#266): cover anti-clobber capture + once-guard; log storage errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 1 on the scroll-position feature: - F1: add two tests for the hook's subtlest invariants — (a2) the restore target is captured synchronously at mount and survives a fresh scroll@0 overwriting storage on load (a regression moving the capture into an effect would now fail); (a3) restore runs at most once per mount even when called again (the wiring effect can re-run). - F2: log instead of silently swallowing sessionStorage errors in readStorage/writeStorage (AGENTS.md "errors must never be swallowed" rule); no user notification since a missed scroll restore is not actionable. - F3: document the hard dependency on PageEditor remounting per page (key={page.id}) at the refs declaration — the per-mount refs are not reset on an in-place pageId change, so removing that key would break restore on the 2nd page. vitest 9/9, tsc 0, eslint 0. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../editor/hooks/use-scroll-position.test.ts | 45 +++++++++++++++++++ .../editor/hooks/use-scroll-position.ts | 20 +++++++-- 2 files changed, 62 insertions(+), 3 deletions(-) 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".