From 4916232e084b7e23334b7fcccfb5d58119e54a62 Mon Sep 17 00:00:00 2001 From: Rivet Date: Mon, 24 Aug 2026 14:36:44 -0700 Subject: [PATCH] Stabilize virtualized route target settlement Co-authored-by: Rivet Signed-off-by: Rivet --- .../features/messages/ui/MessageTimeline.tsx | 1 + .../messages/ui/anchoredScrollTypes.ts | 1 + .../messages/ui/targetRowCentering.ts | 37 ++++--- .../ui/useAnchoredScroll.lifecycle.test.mjs | 97 +++++++++++++++++-- .../features/messages/ui/useAnchoredScroll.ts | 49 ++++++---- 5 files changed, 146 insertions(+), 39 deletions(-) diff --git a/desktop/src/features/messages/ui/MessageTimeline.tsx b/desktop/src/features/messages/ui/MessageTimeline.tsx index dfbe4bd390..e47d3f77d0 100644 --- a/desktop/src/features/messages/ui/MessageTimeline.tsx +++ b/desktop/src/features/messages/ui/MessageTimeline.tsx @@ -361,6 +361,7 @@ const MessageTimelineBase = React.forwardRef< scrollContainerRef: activeScrollContainerRef, splitPanelOpen: splitThreadPanelOpen, targetMessageId, + topBoundaryReached: renderedHistoryExhausted, virtualCancelBottomIntent: timelineVirtualizerApi?.cancelBottomIntent, virtualScrollBy: timelineVirtualizerApi?.scrollBy, virtualScrollToMessage: timelineVirtualizerApi?.scrollToMessage, diff --git a/desktop/src/features/messages/ui/anchoredScrollTypes.ts b/desktop/src/features/messages/ui/anchoredScrollTypes.ts index cc192fb79a..9867008f7d 100644 --- a/desktop/src/features/messages/ui/anchoredScrollTypes.ts +++ b/desktop/src/features/messages/ui/anchoredScrollTypes.ts @@ -17,6 +17,7 @@ export type UseAnchoredScrollOptions = { targetMessageId?: string | null; highlightTargetMessage?: boolean; pinTargetCentered?: boolean; + topBoundaryReached?: boolean; onTargetReached?: (messageId: string) => void; onTargetSettled?: (messageId: string) => void; virtualCancelBottomIntent?: () => void; diff --git a/desktop/src/features/messages/ui/targetRowCentering.ts b/desktop/src/features/messages/ui/targetRowCentering.ts index e949e32bf2..a078f7383a 100644 --- a/desktop/src/features/messages/ui/targetRowCentering.ts +++ b/desktop/src/features/messages/ui/targetRowCentering.ts @@ -9,34 +9,41 @@ function resolveCssLength(value: string) { : parsed; } +function getUsableViewportBounds(container: HTMLDivElement) { + const containerRect = container.getBoundingClientRect(); + const styles = getComputedStyle(container); + return { + bottom: + containerRect.bottom - + resolveCssLength(styles.getPropertyValue("--composer-overlay-height")), + top: + containerRect.top + + resolveCssLength(styles.getPropertyValue("--channel-top-chrome-height")), + }; +} + export function getTargetRowCenterOffset( row: Element, container: HTMLDivElement, ) { const rowRect = row.getBoundingClientRect(); - const containerRect = container.getBoundingClientRect(); - const styles = getComputedStyle(container); - const viewportTop = - containerRect.top + - resolveCssLength(styles.getPropertyValue("--channel-top-chrome-height")); - const viewportBottom = - containerRect.bottom - - resolveCssLength(styles.getPropertyValue("--composer-overlay-height")); + const viewport = getUsableViewportBounds(container); return ( - (rowRect.top + rowRect.bottom) / 2 - (viewportTop + viewportBottom) / 2 + (rowRect.top + rowRect.bottom) / 2 - (viewport.top + viewport.bottom) / 2 ); } /** * A virtualized jump is complete only when the row's midpoint reaches the * viewport midpoint. Two pixels absorb fractional layout and Virtua's rounded - * scroll offsets. The newest row is the one intentional exception: the list - * clamps it to the physical floor, where exact centering is impossible. + * scroll offsets. Boundary rows are the intentional exceptions: the list + * clamps the oldest row to the physical ceiling and the newest row to the + * physical floor, where exact centering is impossible. */ export function isTargetRowCentered( row: Element, container: HTMLDivElement, - allowBottomClamp: boolean, + boundary: "none" | "top" | "bottom", isAtBottom: (container: HTMLDivElement) => boolean, ) { const rowRect = row.getBoundingClientRect(); @@ -47,7 +54,11 @@ export function isTargetRowCentered( ) { return true; } - return allowBottomClamp && isAtBottom(container); + const viewport = getUsableViewportBounds(container); + const rowIsVisible = + rowRect.bottom > viewport.top && rowRect.top < viewport.bottom; + if (boundary === "top") return rowIsVisible && container.scrollTop <= 0; + return boundary === "bottom" && rowIsVisible && isAtBottom(container); } export function targetRowNeedsCenterCorrection(offset: number) { diff --git a/desktop/src/features/messages/ui/useAnchoredScroll.lifecycle.test.mjs b/desktop/src/features/messages/ui/useAnchoredScroll.lifecycle.test.mjs index 709db9dda6..d7ff4fa3de 100644 --- a/desktop/src/features/messages/ui/useAnchoredScroll.lifecycle.test.mjs +++ b/desktop/src/features/messages/ui/useAnchoredScroll.lifecycle.test.mjs @@ -157,6 +157,7 @@ import React from "react"; import { act } from "react"; import { createRoot } from "react-dom/client"; +import { isTargetRowCentered } from "./targetRowCentering.ts"; import { useAnchoredScroll } from "./useAnchoredScroll.ts"; import { useVirtualizedBottomSettle } from "./useVirtualizedBottomSettle.ts"; @@ -270,13 +271,16 @@ function BottomStateHarness({ return null; } -function VirtualScrollBehaviorHarness({ refs }) { - const didRun = React.useRef(false); +function VirtualScrollBehaviorHarness({ + messages = [{ id: "selected" }], + refs, +}) { + const lastRunMessageCount = React.useRef(null); const anchored = useAnchoredScroll({ channelId: "conversation", contentRef: refs.content, isLoading: false, - messages: [{ id: "selected" }], + messages, scrollContainerRef: refs.scroller, virtualizerOwnsPrependAnchoring: true, virtualScrollBy: (offset) => { @@ -289,12 +293,12 @@ function VirtualScrollBehaviorHarness({ refs }) { }, }); React.useLayoutEffect(() => { - if (didRun.current) return; - didRun.current = true; + if (lastRunMessageCount.current === messages.length) return; + lastRunMessageCount.current = messages.length; refs.targetResult = anchored.scrollToMessage("selected", { behavior: "smooth", }); - }, [anchored.scrollToMessage, refs]); + }, [anchored.scrollToMessage, messages.length, refs]); return null; } @@ -557,6 +561,42 @@ test("user interaction releases and retires a pending pinned target", async () = await act(async () => root.unmount()); }); +test("boundary-clamped targets settle only at their matching physical edge", () => { + const container = document.createElement("div"); + container.scrollTop = 0; + container.getBoundingClientRect = () => ({ bottom: 400, top: 0 }); + const row = { + getBoundingClientRect: () => ({ bottom: 40, height: 40, top: 0 }), + }; + const isAtBottom = () => false; + + assert.equal(isTargetRowCentered(row, container, "top", isAtBottom), true); + assert.equal(isTargetRowCentered(row, container, "none", isAtBottom), false); + assert.equal( + isTargetRowCentered(row, container, "bottom", isAtBottom), + false, + ); + + row.getBoundingClientRect = () => ({ + bottom: 2_740, + height: 40, + top: 2_700, + }); + assert.equal( + isTargetRowCentered(row, container, "top", isAtBottom), + false, + "an unrendered indexed jump can report scrollTop zero before the row arrives", + ); + + row.getBoundingClientRect = () => ({ bottom: 40, height: 40, top: 0 }); + container.scrollTop = 1; + assert.equal(isTargetRowCentered(row, container, "top", isAtBottom), false); + assert.equal( + isTargetRowCentered(row, container, "bottom", () => true), + true, + ); +}); + test("virtual search scrolling stays smooth only for an already-rendered target", async () => { for (const rendered of [true, false]) { const content = document.createElement("div"); @@ -600,6 +640,51 @@ test("virtual search scrolling stays smooth only for an already-rendered target" } }); +test("a pending virtual jump is retried when the indexed message model grows", async () => { + const content = document.createElement("div"); + const scroller = document.createElement("div"); + scroller.clientHeight = 400; + scroller.scrollHeight = 1_000; + scroller.scrollTop = 0; + scroller.getBoundingClientRect = () => ({ bottom: 400, top: 0 }); + scroller.querySelector = () => null; + scroller.querySelectorAll = () => []; + scroller.appendChild(content); + const refs = { + content: { current: content }, + scroller: { current: scroller }, + rowTop: 1_000, + scrollOffsets: [], + targetJumps: [], + targetResult: null, + }; + const root = createRoot(document.createElement("div")); + + await act(async () => { + root.render( + React.createElement(VirtualScrollBehaviorHarness, { + messages: [{ id: "selected" }], + refs, + }), + ); + }); + await act(async () => { + root.render( + React.createElement(VirtualScrollBehaviorHarness, { + messages: [{ id: "selected" }, { id: "later" }], + refs, + }), + ); + }); + + assert.equal(refs.targetJumps.length, 2); + assert.deepEqual( + refs.targetJumps.map(({ messageId }) => messageId), + ["selected", "selected"], + ); + await act(async () => root.unmount()); +}); + test("virtual centering corrects rendered geometry on the following frame", async () => { const previousGetComputedStyle = globalThis.getComputedStyle; globalThis.getComputedStyle = (element) => ({ diff --git a/desktop/src/features/messages/ui/useAnchoredScroll.ts b/desktop/src/features/messages/ui/useAnchoredScroll.ts index c1d3f9ea34..f307ce6ae3 100644 --- a/desktop/src/features/messages/ui/useAnchoredScroll.ts +++ b/desktop/src/features/messages/ui/useAnchoredScroll.ts @@ -98,6 +98,7 @@ export function useAnchoredScroll({ targetMessageId = null, highlightTargetMessage = true, pinTargetCentered = false, + topBoundaryReached = false, onTargetReached, onTargetSettled, virtualCancelBottomIntent, @@ -151,7 +152,10 @@ export function useAnchoredScroll({ const programmaticScrollRafRef = React.useRef(null); const targetSettleRafRef = React.useRef(null); const targetRetryRafRef = React.useRef(null); - const virtualTargetJumpRef = React.useRef(null); + const virtualTargetJumpRef = React.useRef<{ + messageId: string; + messageCount: number; + } | null>(null); const virtualTargetCorrectionAppliedRef = React.useRef(false); const targetCorrectionRafRef = React.useRef(null); const [targetRetryVersion, setTargetRetryVersion] = React.useState(0); @@ -394,14 +398,11 @@ export function useAnchoredScroll({ const el = container.querySelector( `[data-message-id="${messageId}"]`, ); + if (virtualizerOwnsPrependAnchoring && !virtualScrollToMessage) + return "pending"; // Wait for Virtua's imperative API. if (virtualizerOwnsPrependAnchoring && virtualScrollToMessage) { - // The virtualizer is the only scroll writer here. Its imperative jump - // keeps correcting `scrollTop` for several frames while unmeasured - // rows commit, so a direct `container.scrollTo` would be overwritten — - // the list's own first-commit bottom settle is exactly such a jump. A - // new jump cancels the pending one, which is how target navigation - // takes the viewport away from that settle; disarming the durable - // bottom intent stops the ResizeObserver from re-pinning later. + // Virtua is the sole scroll writer; a new jump cancels its initial + // bottom settle and the cancel call prevents later re-pinning. virtualCancelBottomIntent?.(); const virtualScrollBehavior = el ? (options.behavior ?? "auto") @@ -416,7 +417,10 @@ export function useAnchoredScroll({ ); })() : false; - if (virtualTargetJumpRef.current !== messageId || !rowIsVisible) { + const jumpMatchesCurrentModel = + virtualTargetJumpRef.current?.messageId === messageId && + virtualTargetJumpRef.current.messageCount === messages.length; + if (!jumpMatchesCurrentModel) { if ( !virtualScrollToMessage(messageId, { behavior: virtualScrollBehavior, @@ -424,7 +428,10 @@ export function useAnchoredScroll({ ) { return "missing"; } - virtualTargetJumpRef.current = messageId; + virtualTargetJumpRef.current = { + messageId, + messageCount: messages.length, + }; virtualTargetCorrectionAppliedRef.current = false; } if ( @@ -466,18 +473,19 @@ export function useAnchoredScroll({ }); } anchorRef.current = { kind: "message", messageId, topOffset: 0 }; - // The channel reset seeds `virtualizerAtBottomRef` to true as a - // default, and both the viewport-resize and append settles trust it. - // A ResizeObserver fires once on observe, so leaving that default in - // place lets the first resize callback after this jump pull the view - // straight back to the floor. Record what the jump knows instead; the - // virtualizer's next real bottom report refines it. - // Handled only once the row is rendered and actually in view. Until - // then the jump is in flight; the caller retries on range change. - const isNewestTarget = messages.at(-1)?.id === messageId; + // Completion requires midpoint alignment or a confirmed boundary. + const targetIndex = messages.findIndex( + (message) => message.id === messageId, + ); + const targetBoundary = + targetIndex === 0 && topBoundaryReached + ? "top" + : targetIndex === messages.length - 1 + ? "bottom" + : "none"; if ( !el || - !isTargetRowCentered(el, container, isNewestTarget, isAtBottomNow) + !isTargetRowCentered(el, container, targetBoundary, isAtBottomNow) ) { virtualizerAtBottomRef.current = false; setIsAtBottom(false); @@ -550,6 +558,7 @@ export function useAnchoredScroll({ highlightMessage, messages, pinTargetCentered, + topBoundaryReached, scrollContainerRef, virtualCancelBottomIntent, virtualScrollBy,