From 8f95c5808e2be9b86633a9f0110d1a6ea3d4cd0b Mon Sep 17 00:00:00 2001 From: claude_code Date: Fri, 3 Jul 2026 17:15:37 +0300 Subject: [PATCH 1/2] =?UTF-8?q?fix(editor):=20reserve=20document=20height?= =?UTF-8?q?=20across=20the=20static=E2=86=92live=20swap=20so=20scroll=20su?= =?UTF-8?q?rvives=20(#266)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause (confirmed via Chrome DevTools on the live app, and the fix validated there too): after the reading-position restore lands correctly, the static→live editor swap momentarily SHRINKS the document (the live editor lays out its content over a few frames — measured height 32005 → 22050), so the browser CLAMPS window scroll to the top. That is what produced all of: - "lands correct → jumps to top → back down" (restore#2 recovering from the clamp), - the final position overshooting (~6000px) via scroll-anchoring during recovery, - "scroll a little → jumps to 0" (the clamp catching the reader mid-scroll). Fixing the restore logic was chasing symptoms. This reserves the pre-swap content height (a min-height on a wrapper around the static/live editor) until the live editor has laid out (or a short safety cap), so the document never collapses and window scroll simply survives the swap. Validated live: with the height pinned the restore fires ONCE and the position stays put (no reset, no jitter, no overshoot); the existing post-swap re-assert becomes a silent no-op. No change to the restore hook or its tests. Co-Authored-By: Claude Opus 4.8 --- .../src/features/editor/page-editor.tsx | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/apps/client/src/features/editor/page-editor.tsx b/apps/client/src/features/editor/page-editor.tsx index b001051f..cbc6f6bc 100644 --- a/apps/client/src/features/editor/page-editor.tsx +++ b/apps/client/src/features/editor/page-editor.tsx @@ -449,6 +449,16 @@ 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(null); + const [reservedHeight, setReservedHeight] = useState(null); + useEffect(() => { const timeout = setTimeout(() => { if (yjsConnectionStatus === WebSocketStatus.Connecting || !isSynced) { @@ -477,10 +487,40 @@ 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. + setReservedHeight(swapWrapperRef.current?.offsetHeight ?? null); setShowStatic(false); } }, [yjsConnectionStatus, isSynced]); + // 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. The cap 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. 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 RELEASE_CAP_MS = 4000; + const check = () => { + const liveHeight = + (menuContainerRef.current as HTMLElement | null)?.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]); + // 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. @@ -490,6 +530,12 @@ export default function PageEditor({ +
{showStatic ? (
{/* Surface the pre-sync read-only window so edits typed before the @@ -577,6 +623,7 @@ export default function PageEditor({ >
)} + From 17003fbbc1628caea77ef9f6c63c63c480491e93 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Fri, 3 Jul 2026 18:13:30 +0300 Subject: [PATCH 2/2] test(editor): extract swap height-reservation into a tested hook (#308 F1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The +42-line height-reservation logic lived inline in PageEditor on the risky static→live swap path with zero tests (F1). Extract it into useSwapHeightReservation(showStatic, menuContainerRef) → { reservedHeight, captureReservation }, mirroring the sibling useScrollRestoreOnSwap extraction, so its release guard and cap are directly unit-testable. Pure extraction — behavior identical. The capture stays a synchronous callback the editor invokes in the collab-sync effect (reading swapWrapperRef.offsetHeight while the static content is still mounted, before setShowStatic(false)); a post-transition effect inside the hook would read the collapsed live height and be wrong. The rAF release loop (release at scrollHeight >= reserved, or the RELEASE_CAP_MS=4000 cap) and cancelAnimationFrame cleanup moved verbatim. Tests (use-swap-height-reservation.test.ts) cover 4 branches, mutation-verified: (a) capture → reservedHeight; (b) release when live content reaches reserved; (c) release at the cap when it never does; (d) non-swap → stays null. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../hooks/use-swap-height-reservation.test.ts | 164 ++++++++++++++++++ .../hooks/use-swap-height-reservation.ts | 79 +++++++++ .../src/features/editor/page-editor.tsx | 37 +--- 3 files changed, 252 insertions(+), 28 deletions(-) create mode 100644 apps/client/src/features/editor/hooks/use-swap-height-reservation.test.ts create mode 100644 apps/client/src/features/editor/hooks/use-swap-height-reservation.ts diff --git a/apps/client/src/features/editor/hooks/use-swap-height-reservation.test.ts b/apps/client/src/features/editor/hooks/use-swap-height-reservation.test.ts new file mode 100644 index 00000000..271e62e4 --- /dev/null +++ b/apps/client/src/features/editor/hooks/use-swap-height-reservation.test.ts @@ -0,0 +1,164 @@ +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; + setScrollHeight: (h: number) => void; +} { + const el = { scrollHeight: 0 }; + return { + ref: { current: el } as unknown as RefObject, + 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(); + }); +}); diff --git a/apps/client/src/features/editor/hooks/use-swap-height-reservation.ts b/apps/client/src/features/editor/hooks/use-swap-height-reservation.ts new file mode 100644 index 00000000..2940df63 --- /dev/null +++ b/apps/client/src/features/editor/hooks/use-swap-height-reservation.ts @@ -0,0 +1,79 @@ +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, +): { + reservedHeight: number | null; + captureReservation: (height: number | null) => void; +} { + const [reservedHeight, setReservedHeight] = useState(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 }; +} diff --git a/apps/client/src/features/editor/page-editor.tsx b/apps/client/src/features/editor/page-editor.tsx index cbc6f6bc..7b4686c4 100644 --- a/apps/client/src/features/editor/page-editor.tsx +++ b/apps/client/src/features/editor/page-editor.tsx @@ -79,6 +79,7 @@ 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"; @@ -457,7 +458,13 @@ export default function PageEditor({ // 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(null); - const [reservedHeight, setReservedHeight] = useState(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(() => { @@ -490,37 +497,11 @@ export default function PageEditor({ // 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. - setReservedHeight(swapWrapperRef.current?.offsetHeight ?? null); + captureReservation(swapWrapperRef.current?.offsetHeight ?? null); setShowStatic(false); } }, [yjsConnectionStatus, isSynced]); - // 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. The cap 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. 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 RELEASE_CAP_MS = 4000; - const check = () => { - const liveHeight = - (menuContainerRef.current as HTMLElement | null)?.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]); - // 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.