diff --git a/packages/ui/src/components/views/useFilePreviewScrollPosition.test.tsx b/packages/ui/src/components/views/useFilePreviewScrollPosition.test.tsx index 47539a63..00859be0 100644 --- a/packages/ui/src/components/views/useFilePreviewScrollPosition.test.tsx +++ b/packages/ui/src/components/views/useFilePreviewScrollPosition.test.tsx @@ -2,13 +2,28 @@ import { afterEach, beforeEach, describe, expect, test } from 'bun:test'; import { Window } from 'happy-dom'; import React, { act, useLayoutEffect } from 'react'; import { createRoot, type Root } from 'react-dom/client'; +import { VirtualizedFile, Virtualizer } from '@pierre/diffs'; import { useFilePreviewScrollPosition } from './useFilePreviewScrollPosition'; +type RestorePreview = ReturnType['restore']; + +class MeasuredPreviewFile extends VirtualizedFile { + lineTop = 1000; + + override getLinePosition() { + return { top: this.lineTop, height: 20 }; + } + + override getNumericScrollAnchor() { + return { lineNumber: 51, top: this.lineTop }; + } +} + function Preview({ positionKey, element, onReady }: { positionKey: string | null; element: HTMLElement; - onReady: (restore: () => void) => void; + onReady: (restore: RestorePreview) => void; }) { const { setScroller, restore } = useFilePreviewScrollPosition(positionKey); useLayoutEffect(() => { @@ -27,10 +42,10 @@ describe('file preview scroll positions', () => { let height: number; let top: number; let left: number; - let restore: () => void; + let restore: RestorePreview; let prefix: string; let sequence = 0; - const onReady = (callback: () => void) => { restore = callback; }; + const onReady = (callback: RestorePreview) => { restore = callback; }; beforeEach(() => { windowInstance = new Window(); @@ -39,6 +54,7 @@ describe('file preview scroll positions', () => { document: windowInstance.document, HTMLElement: windowInstance.HTMLElement, Event: windowInstance.Event, + DOMRect: windowInstance.DOMRect, MutationObserver: windowInstance.MutationObserver, ResizeObserver: windowInstance.ResizeObserver, IS_REACT_ACT_ENVIRONMENT: true, @@ -54,6 +70,7 @@ describe('file preview scroll positions', () => { left = 0; prefix = `preview-test-${sequence++}`; Object.defineProperties(scroller, { + clientHeight: { value: 100 }, scrollTop: { get: () => top, set: (value: number) => { top = Math.max(0, Math.min(value, height - 100)); }, @@ -168,4 +185,33 @@ describe('file preview scroll positions', () => { restore(); expect(top).toBe(50); }); + + test('restores the same virtualized line after Pierre reconciles different height estimates', async () => { + const file = new MeasuredPreviewFile({}, new Virtualizer()); + const node = document.createElement('div'); + const line = document.createElement('div'); + line.dataset.line = ''; + line.dataset.lineIndex = '50'; + node.attachShadow({ mode: 'open' }).append(line); + content.append(node); + line.getBoundingClientRect = () => new DOMRect(0, file.lineTop - top + 8, 100, 20); + + await render('virtual'); + scroll(1000); + restore(node, file); + await Promise.resolve(); + expect(line.getBoundingClientRect().top).toBe(8); + + await render(null); + scroll(0); + await render('virtual'); + restore(node, file); + // onPostRender runs before Pierre's synchronous height reconciliation. + file.lineTop = 1500; + scroll(800); + await Promise.resolve(); + expect(top).toBe(1500); + expect(line.getBoundingClientRect().top).toBe(8); + file.cleanUp(); + }); }); diff --git a/packages/ui/src/components/views/useFilePreviewScrollPosition.ts b/packages/ui/src/components/views/useFilePreviewScrollPosition.ts index 7b0609bc..bd0cbd3b 100644 --- a/packages/ui/src/components/views/useFilePreviewScrollPosition.ts +++ b/packages/ui/src/components/views/useFilePreviewScrollPosition.ts @@ -21,10 +21,18 @@ export function useFilePreviewScrollPosition(positionKey: string | null) { if (node && instance instanceof VirtualizedFile) { virtualFileRef.current = { key: positionKey, file: instance, node }; } - restoreRef.current?.(); - // The scroll event can precede the virtualizer mounting the target lines. - // Capture again after rendering, when a numeric line anchor is available. - rememberRef.current?.(); + const restorePosition = restoreRef.current; + const rememberPosition = rememberRef.current; + const finishRender = () => { + if (restoreRef.current !== restorePosition) return; + restorePosition?.(); + rememberPosition?.(); + }; + // Pierre calls onPostRender before reconciling measured heights and applying + // its own scroll correction. Finish after that synchronous render pass, + // before paint, rather than saving estimates or having our restore undone. + if (instance) queueMicrotask(finishRender); + else finishRender(); }, [positionKey]); useLayoutEffect(() => { @@ -48,11 +56,13 @@ export function useFilePreviewScrollPosition(positionKey: string | null) { const lineElement = target.line && getLineElement(target.line.number); // Estimated heights locate the virtual window; the mounted row supplies // the final visual offset, including wrapping and Pierre's padding. - const targetTop = lineElement && target.line - ? scroller.scrollTop + lineElement.getBoundingClientRect().top - scroller.getBoundingClientRect().top - target.line.offset - : linePosition && target.line - ? (file?.top ?? 0) + linePosition.top - target.line.offset - : target.top; + let targetTop = target.top; + if (target.line && linePosition) { + targetTop = (file?.top ?? 0) + linePosition.top - target.line.offset; + } + if (target.line && lineElement) { + targetTop = scroller.scrollTop + lineElement.getBoundingClientRect().top - scroller.getBoundingClientRect().top - target.line.offset; + } scroller.scrollTop = targetTop; scroller.scrollLeft = target.left; if ((!target.line || lineElement) && Math.abs(scroller.scrollTop - targetTop) < 1 && Math.abs(scroller.scrollLeft - target.left) < 1) { @@ -77,12 +87,13 @@ export function useFilePreviewScrollPosition(positionKey: string | null) { const file = getVirtualFile(); const anchor = file?.getNumericScrollAnchor(scroller.scrollTop - (file.top ?? 0)); const lineElement = anchor && getLineElement(anchor.lineNumber); + const offset = lineElement ? lineElement.getBoundingClientRect().top - scroller.getBoundingClientRect().top : null; positions.delete(positionKey); positions.set(positionKey, { top: scroller.scrollTop, left: scroller.scrollLeft, - line: anchor && lineElement - ? { number: anchor.lineNumber, offset: lineElement.getBoundingClientRect().top - scroller.getBoundingClientRect().top } + line: anchor && offset !== null && offset >= 0 && offset < scroller.clientHeight + ? { number: anchor.lineNumber, offset } : undefined, }); if (positions.size > MAX_POSITIONS) {