From 0618831e92a0c94682b6125db0a15a955289fd40 Mon Sep 17 00:00:00 2001 From: Caroline di Vittorio <43390382+carolinedivittorio@users.noreply.github.com> Date: Tue, 30 Jun 2026 17:00:34 -0700 Subject: [PATCH] fix: jump + ResizeObserver settle for review scroll-to (#1655) Replace smooth-scroll plus frame-count correction loops on the PR reviews page with an instant jump that re-asserts its target via a ResizeObserver (the real "layout settled" signal). Block/file navigation and finding/comment centering now land deterministically as off-screen cards mount, files expand, and annotation cards measure, instead of racing a smooth-scroll animation against height reconciliation. Holds bail on user wheel/touch input and after a short ceiling, and a new navigation cancels the previous hold. Co-authored-by: open-swe[bot] Co-authored-by: Johannes du Plessis (cherry picked from commit 7530653bba7774d66a54b8bef0d2bbc25f519942) --- ui/src/components/agents/ReviewMainBody.tsx | 181 +++++++++++--------- 1 file changed, 101 insertions(+), 80 deletions(-) diff --git a/ui/src/components/agents/ReviewMainBody.tsx b/ui/src/components/agents/ReviewMainBody.tsx index 4fae0974..c4242ef7 100644 --- a/ui/src/components/agents/ReviewMainBody.tsx +++ b/ui/src/components/agents/ReviewMainBody.tsx @@ -217,46 +217,27 @@ function selectedRangeFromDiff( } } -// Scroll a file card / group flush to the top of the diff scroller. Under -// virtualization, scrollIntoView computes its target against estimated row -// heights; scrolling past unmeasured files reconciles their real heights -// mid-animation and the Virtualizer re-pins its scroll anchor, which leaves the -// target off the top. Once the smooth scroll settles, re-assert alignment (now -// against measured heights) until the target sits at the top or the budget runs -// out. Respects the element's scroll-margin-top. -function scrollCardToTop(el: HTMLElement, scroller: HTMLElement | null): void { - el.scrollIntoView({ block: "start", behavior: "smooth" }) - if (!scroller) return - let frames = 0 - let lastTop = Number.NaN - let stableFrames = 0 - let corrections = 0 - const align = () => { - if (frames++ > 240) return - const top = scroller.scrollTop - if (top === lastTop) stableFrames++ - else { - stableFrames = 0 - lastTop = top - } - // Wait for the smooth scroll + height reconciliation to settle. - if (stableFrames < 3) { - requestAnimationFrame(align) - return - } +// Scroll a file card / group flush to the top of the diff scroller (fallback +// when no virtualizer geometry is available). Jumps instantly to a bounding-rect +// target — respecting the element's scroll-margin-top — then holds that target +// as content above reflows, so no smooth-scroll animation races the height +// reconciliation. Returns a stop fn to cancel the hold. +function scrollCardToTop( + el: HTMLElement, + scroller: HTMLElement | null +): () => void { + if (!scroller) { + el.scrollIntoView({ block: "start" }) + return () => {} + } + return jumpAndHold(scroller, () => { const marginTop = parseFloat(getComputedStyle(el).scrollMarginTop) || 0 const delta = el.getBoundingClientRect().top - scroller.getBoundingClientRect().top - marginTop - if (Math.abs(delta) > 1 && corrections++ < 5) { - el.scrollIntoView({ block: "start", behavior: "smooth" }) - stableFrames = 0 - lastTop = Number.NaN - requestAnimationFrame(align) - } - } - requestAnimationFrame(align) + return clampScrollTop(scroller, scroller.scrollTop + delta) + }) } // The virtualizer instance returned by useVirtualizer(); exposes @@ -269,42 +250,21 @@ const SCROLL_TOP_GAP = 8 // Scroll a block / file card flush to the top of the diff scroller using the // virtualizer's own geometry. getOffsetInScrollContainer returns the element's // absolute offset within the scroll content; with uniform fixed-height rows -// (see diffUtils) that offset is stable, so a single smooth scroll lands -// precisely. As any not-yet-measured rows above the target reconcile during the -// scroll the offset can shift slightly, so once the scroll settles we re-read it -// and re-assert exactly once — no 240-frame correction loop needed. +// (see diffUtils) that offset is stable, so an instant jump lands precisely. +// jumpAndHold then re-reads the offset whenever the content reflows (rows above +// measuring/expanding) and re-asserts it, so the target stays pinned to the top. +// Returns a stop fn to cancel the hold. function scrollCardToTopVirtual( el: HTMLElement, scroller: HTMLElement, virtualizer: DiffVirtualizer -): void { - const targetTop = () => +): () => void { + return jumpAndHold(scroller, () => clampScrollTop( scroller, virtualizer.getOffsetInScrollContainer(el) - SCROLL_TOP_GAP ) - scroller.scrollTo({ top: targetTop(), behavior: "smooth" }) - let frames = 0 - let lastTop = Number.NaN - let stableFrames = 0 - const settle = () => { - if (frames++ > 120) return - const top = scroller.scrollTop - if (top === lastTop) stableFrames++ - else { - stableFrames = 0 - lastTop = top - } - if (stableFrames < 3) { - requestAnimationFrame(settle) - return - } - const desired = targetTop() - if (Math.abs(desired - scroller.scrollTop) > 1) { - scroller.scrollTo({ top: desired, behavior: "auto" }) - } - } - requestAnimationFrame(settle) + ) } // Older stored summaries embed `[label](#loc=path:line)` diff links; render the @@ -341,16 +301,68 @@ function clampScrollTop(scroller: HTMLElement, top: number): number { ) } -function scrollElementToCenter(el: HTMLElement, scroller: HTMLElement): number { +// How long to keep re-asserting a scroll target after the initial jump. +const SCROLL_HOLD_TIMEOUT_MS = 700 + +// Jump the scroller to getTarget() instantly, then re-assert that target each +// time the scroll content reflows (off-screen cards mounting, files expanding, +// annotation cards measuring) — a ResizeObserver is the real "layout settled" +// signal, replacing fixed frame-budget correction loops. Bails the moment the +// user scrolls so we never fight them, and disconnects after a short ceiling. +function jumpAndHold( + scroller: HTMLElement, + getTarget: () => number, + timeout = SCROLL_HOLD_TIMEOUT_MS +): () => void { + let raf = 0 + let stopped = false + let timer = 0 + let ro: ResizeObserver | null = null + const stop = () => { + if (stopped) return + stopped = true + ro?.disconnect() + if (raf) cancelAnimationFrame(raf) + scroller.removeEventListener("wheel", stop) + scroller.removeEventListener("touchstart", stop) + window.clearTimeout(timer) + } + const reassert = () => { + raf = 0 + if (stopped) return + const desired = getTarget() + if (Math.abs(desired - scroller.scrollTop) > 1) { + scroller.scrollTo({ top: desired, behavior: "auto" }) + } + } + const schedule = () => { + if (!raf && !stopped) raf = requestAnimationFrame(reassert) + } + scroller.scrollTo({ top: getTarget(), behavior: "auto" }) + ro = new ResizeObserver(schedule) + ro.observe(scroller.firstElementChild ?? scroller) + scroller.addEventListener("wheel", stop, { passive: true }) + scroller.addEventListener("touchstart", stop, { passive: true }) + timer = window.setTimeout(stop, timeout) + return stop +} + +// Absolute scrollTop that centers el within the scroller's viewport. +function elementCenterTarget(el: HTMLElement, scroller: HTMLElement): number { const elementRect = el.getBoundingClientRect() const scrollerRect = scroller.getBoundingClientRect() const delta = elementRect.top - scrollerRect.top - (scroller.clientHeight - elementRect.height) / 2 - const targetTop = clampScrollTop(scroller, scroller.scrollTop + delta) + return clampScrollTop(scroller, scroller.scrollTop + delta) +} + +function scrollElementToCenter(el: HTMLElement, scroller: HTMLElement): number { + const before = scroller.scrollTop + const targetTop = elementCenterTarget(el, scroller) scroller.scrollTo({ top: targetTop, behavior: "auto" }) - return Math.abs(delta) + return Math.abs(targetTop - before) } function scrollDiffLineToCenter( @@ -621,6 +633,9 @@ function ReviewBodyInner({ const diffScrollElRef = useRef(null) const virtualizerRef = useRef(null) const findingScrollRequestRef = useRef(0) + // Cancels the in-flight scroll "hold" (see jumpAndHold) when a new navigation + // begins or the component unmounts, so holds never fight each other. + const scrollHoldStopRef = useRef<(() => void) | null>(null) const groupRefs = useRef>({}) // The block pinned at the top of the diff (scroll-spy), highlighted in the // agenda sidebar. @@ -814,27 +829,31 @@ function ReviewBodyInner({ const scrollToFile = useCallback((path: string) => { setSelectedFile(path) setExpandedFiles((prev) => ({ ...prev, [path]: true })) + scrollHoldStopRef.current?.() requestAnimationFrame(() => { const el = fileRefs.current[path] const scroller = diffScrollElRef.current if (!el || !scroller) return - if (virtualizerRef.current) - scrollCardToTopVirtual(el, scroller, virtualizerRef.current) - else scrollCardToTop(el, scroller) + scrollHoldStopRef.current = virtualizerRef.current + ? scrollCardToTopVirtual(el, scroller, virtualizerRef.current) + : scrollCardToTop(el, scroller) }) }, []) const scrollToGroup = useCallback((index: number) => { + scrollHoldStopRef.current?.() requestAnimationFrame(() => { const el = groupRefs.current[index] const scroller = diffScrollElRef.current if (!el || !scroller) return - if (virtualizerRef.current) - scrollCardToTopVirtual(el, scroller, virtualizerRef.current) - else scrollCardToTop(el, scroller) + scrollHoldStopRef.current = virtualizerRef.current + ? scrollCardToTopVirtual(el, scroller, virtualizerRef.current) + : scrollCardToTop(el, scroller) }) }, []) + useEffect(() => () => scrollHoldStopRef.current?.(), []) + // Scroll-spy: track which block's header is currently pinned at the top of the // diff scroller and surface it as the active agenda row (Google-Docs outline). useEffect(() => { @@ -1003,6 +1022,7 @@ function ReviewBodyInner({ if (!willExpand || !isAnchored(finding)) return setSelectedFile(finding.file) setExpandedFiles((prev) => ({ ...prev, [finding.file]: true })) + scrollHoldStopRef.current?.() let frames = 0 let lineScrollDone = false const snap = () => { @@ -1010,12 +1030,13 @@ function ReviewBodyInner({ const scroller = diffScrollElRef.current if (!scroller) return + // Once the finding's inline card has mounted (its diff rows window in + // under virtualization), center it and hold as the card settles. const annotation = annotationRefs.current[finding.id] if (annotation?.isConnected && annotation.getClientRects().length > 0) { - const delta = scrollElementToCenter(annotation, scroller) - if (delta <= 1 || frames >= FINDING_SCROLL_MAX_FRAMES) return - frames += 1 - requestAnimationFrame(snap) + scrollHoldStopRef.current = jumpAndHold(scroller, () => + elementCenterTarget(annotation, scroller) + ) return } @@ -1061,6 +1082,7 @@ function ReviewBodyInner({ } setSelectedFile(path) setExpandedFiles((prev) => ({ ...prev, [path]: true })) + scrollHoldStopRef.current?.() const requestId = ++findingScrollRequestRef.current const side: SelectionSide = openComment.side === "LEFT" ? "deletions" : "additions" @@ -1075,10 +1097,9 @@ function ReviewBodyInner({ const annotation = annotationRefs.current[key] if (annotation?.isConnected && annotation.getClientRects().length > 0) { mounted = true - const delta = scrollElementToCenter(annotation, scroller) - if (delta <= 1 || frames >= FINDING_SCROLL_MAX_FRAMES) return - frames += 1 - requestAnimationFrame(snap) + scrollHoldStopRef.current = jumpAndHold(scroller, () => + elementCenterTarget(annotation, scroller) + ) return } const diffTarget = diffInstanceRefs.current[path]