adam/open-swe
1
0
Fork 0
mirror of https://github.com/Sea-Haven-Industries/open-swe.git synced 2026-10-02 16:43:16 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

fix: jump + ResizeObserver settle for review scroll-to (#1655)

Browse source 
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] <open-swe@users.noreply.github.com>
Co-authored-by: Johannes du Plessis <johannes@langchain.dev>
(cherry picked from commit 7530653bba7774d66a54b8bef0d2bbc25f519942)
This commit is contained in:
Caroline di Vittorio 2026-06-30 17:00:34 -07:00 • committed by Adam Moussa
parent 4c567d8b29
commit 0618831e92
No known key found for this signature in database
1 changed files with 101 additions and 80 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

181
ui/src/components/agents/ReviewMainBody.tsx
View file

@ -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<HTMLDivElement | null>(null)
const virtualizerRef = useRef<DiffVirtualizer | null>(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<Record<number, HTMLDivElement | null>>({})
// 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]