From 4c567d8b29ada18134a16e11cacc8d60996d4231 Mon Sep 17 00:00:00 2001 From: Caroline di Vittorio <43390382+carolinedivittorio@users.noreply.github.com> Date: Tue, 30 Jun 2026 15:45:32 -0700 Subject: [PATCH] feat: reviews block agenda, sticky headers, accurate diff scroll (#1653) Rework the AI-sorted blocks experience on the PR reviews page into a Google-Docs-style outline: the left sidebar is now a clean number+title agenda with scroll-spy highlighting of the active block; each block shows its title + description (sticky) above its diff; and diff rows are pinned to a uniform height so scroll-to lands precisely via the virtualizer's own geometry instead of an estimate-driven correction loop. Co-authored-by: open-swe[bot] (cherry picked from commit 0b76afdc955e33805c7623d1502a75a9c7c9c1b7) --- ui/src/components/agents/ReviewMainBody.tsx | 182 +++++++++++++++++--- ui/src/components/agents/ReviewSidebar.tsx | 156 ++++------------- ui/src/components/agents/utils/diffUtils.ts | 14 ++ 3 files changed, 211 insertions(+), 141 deletions(-) diff --git a/ui/src/components/agents/ReviewMainBody.tsx b/ui/src/components/agents/ReviewMainBody.tsx index f68c3349..4fae0974 100644 --- a/ui/src/components/agents/ReviewMainBody.tsx +++ b/ui/src/components/agents/ReviewMainBody.tsx @@ -41,6 +41,7 @@ import { MultiFileDiff, Virtualizer, WorkerPoolContextProvider, + useVirtualizer, } from "@pierre/diffs/react" import type { Icon } from "@phosphor-icons/react" import type { FileContents } from "@pierre/diffs/react" @@ -73,7 +74,10 @@ import { ReviewChatComposerProvider, useReviewChatComposer, } from "@/components/agents/ReviewChat" -import { ReviewSidebarPanel } from "@/components/agents/ReviewSidebar" +import { + ReviewSidebarPanel, + renderInlineCode, +} from "@/components/agents/ReviewSidebar" import { DIFF_VIRTUALIZER_CONFIG, DIFF_VIRTUAL_METRICS, @@ -255,6 +259,60 @@ function scrollCardToTop(el: HTMLElement, scroller: HTMLElement | null): void { requestAnimationFrame(align) } +// The virtualizer instance returned by useVirtualizer(); exposes +// getOffsetInScrollContainer for accurate scroll targeting. +type DiffVirtualizer = NonNullable> + +// Breathing room left above a block/file when it's scrolled to the top. +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. +function scrollCardToTopVirtual( + el: HTMLElement, + scroller: HTMLElement, + virtualizer: DiffVirtualizer +): void { + const targetTop = () => + 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 +// label as inline code instead so no stale jump-links leak into the block body. +function stripLocationLinks(summary: string): string { + return summary.replace(/\[([^\]]+)\]\(#loc=[^)]*\)/g, "`$1`") +} + interface PositionedDiffInstance { getLinePosition: ( lineNumber: number, @@ -561,8 +619,12 @@ function ReviewBodyInner({ range: SelectedLineRange } | null>(null) const diffScrollElRef = useRef(null) + const virtualizerRef = useRef(null) const findingScrollRequestRef = useRef(0) const groupRefs = useRef>({}) + // The block pinned at the top of the diff (scroll-spy), highlighted in the + // agenda sidebar. + const [activeGroup, setActiveGroup] = useState(null) const [diffStyle, setDiffStyleState] = useState(() => readStoredDiffStyle() ) @@ -726,11 +788,6 @@ function ReviewBodyInner({ return groupedView.map((group) => ({ index: group.index, title: group.title, - summary: group.summary, - additions: group.additions, - deletions: group.deletions, - fileCount: group.files.length, - files: group.files.map((file) => file.path), })) }, [groupedView]) @@ -759,17 +816,60 @@ function ReviewBodyInner({ setExpandedFiles((prev) => ({ ...prev, [path]: true })) requestAnimationFrame(() => { const el = fileRefs.current[path] - if (el) scrollCardToTop(el, diffScrollElRef.current) + const scroller = diffScrollElRef.current + if (!el || !scroller) return + if (virtualizerRef.current) + scrollCardToTopVirtual(el, scroller, virtualizerRef.current) + else scrollCardToTop(el, scroller) }) }, []) const scrollToGroup = useCallback((index: number) => { requestAnimationFrame(() => { const el = groupRefs.current[index] - if (el) scrollCardToTop(el, diffScrollElRef.current) + const scroller = diffScrollElRef.current + if (!el || !scroller) return + if (virtualizerRef.current) + scrollCardToTopVirtual(el, scroller, virtualizerRef.current) + else scrollCardToTop(el, scroller) }) }, []) + // 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(() => { + if (view !== "ai" || !groupedView || groupedView.length === 0) { + setActiveGroup(null) + return + } + const scroller = diffScrollElRef.current + if (!scroller) return + let raf = 0 + const compute = () => { + raf = 0 + const top = scroller.getBoundingClientRect().top + let current = groupedView[0]?.index ?? null + for (const group of groupedView) { + const el = groupRefs.current[group.index] + if (!el) continue + if (el.getBoundingClientRect().top - top <= SCROLL_TOP_GAP + 2) + current = group.index + else break + } + setActiveGroup(current) + } + const onScroll = () => { + if (raf) return + raf = requestAnimationFrame(compute) + } + compute() + scroller.addEventListener("scroll", onScroll, { passive: true }) + return () => { + scroller.removeEventListener("scroll", onScroll) + if (raf) cancelAnimationFrame(raf) + } + }, [view, groupedView]) + const filesByPath = useMemo( () => new Map((diffFiles ?? []).map((file) => [file.path, file])), [diffFiles] @@ -1055,6 +1155,7 @@ function ReviewBodyInner({ view, onViewChange: setView, onSelectGroup: scrollToGroup, + activeGroup, }), [ detail.number, @@ -1066,6 +1167,7 @@ function ReviewBodyInner({ view, setView, scrollToGroup, + activeGroup, ] ) @@ -1122,7 +1224,10 @@ function ReviewBodyInner({ )} config={DIFF_VIRTUALIZER_CONFIG} > -
+ ) and lifts it to the parent ref so scroll-to can read accurate +// offsets. Doubles as the hidden scroll-element probe. +function VirtualizerBridge({ + probeRef, + instanceRef, +}: { + probeRef: (node: HTMLDivElement | null) => void + instanceRef: React.MutableRefObject +}) { + const virtualizer = useVirtualizer() + useEffect(() => { + instanceRef.current = virtualizer ?? null + }, [virtualizer, instanceRef]) + return
+} + +// The block header: number + title + stats, then the block description. Pinned +// at the top of the diff scroller while scrolling the block (Google-Docs feel), +// stacked above Pierre's in-diff sticky header (z-index 4). A long description +// scrolls within the pinned header instead of consuming the viewport. function GroupHeader({ group }: { group: ResolvedGroup }) { + const title = useMemo(() => renderInlineCode(group.title), [group.title]) + const summary = useMemo( + () => (group.summary ? stripLocationLinks(group.summary) : ""), + [group.summary] + ) return ( -
- - {group.index} - -

{group.title}

- - {group.additions > 0 && ( - +{group.additions} - )} - {group.deletions > 0 && ( - -{group.deletions} - )} - +
+
+ + {group.index} + +

{title}

+ + {group.additions > 0 && ( + +{group.additions} + )} + {group.deletions > 0 && ( + -{group.deletions} + )} + +
+ {summary && ( +
+ +
+ )}
) } diff --git a/ui/src/components/agents/ReviewSidebar.tsx b/ui/src/components/agents/ReviewSidebar.tsx index d9b97c22..08397e75 100644 --- a/ui/src/components/agents/ReviewSidebar.tsx +++ b/ui/src/components/agents/ReviewSidebar.tsx @@ -1,14 +1,10 @@ -import { memo, useCallback, useEffect, useMemo, useState } from "react" +import { memo, useCallback, useEffect, useMemo } from "react" import { FileTree, useFileTree, useFileTreeSelection, } from "@pierre/trees/react" -import { - CaretRightIcon, - ListBulletsIcon, - TreeViewIcon, -} from "@phosphor-icons/react" +import { ListBulletsIcon, TreeViewIcon } from "@phosphor-icons/react" import type { ReactNode } from "react" import type { @@ -17,7 +13,6 @@ import type { GitStatusEntry, } from "@pierre/trees" import type { ReviewDiffFile } from "@/lib/api" -import { Markdown } from "@/components/agents/ported" import { Skeleton } from "@/components/ui/skeleton" import { TREE_UNSAFE_CSS, @@ -37,11 +32,6 @@ export type ReviewSidebarView = "ai" | "files" export interface ReviewSidebarGroup { index: number title: string - summary: string - additions: number - deletions: number - fileCount: number - files: Array } export interface ReviewSidebarData { @@ -54,6 +44,9 @@ export interface ReviewSidebarData { view: ReviewSidebarView onViewChange: (view: ReviewSidebarView) => void onSelectGroup: (index: number) => void + // The block currently pinned at the top of the diff (scroll-spy), highlighted + // in the agenda. null when no block is active or the AI view isn't shown. + activeGroup: number | null } export function ReviewSidebarPanel({ data }: { data: ReviewSidebarData }) { @@ -73,8 +66,8 @@ export function ReviewSidebarPanel({ data }: { data: ReviewSidebarData }) { {showAi ? ( ) : !data.files ? (
@@ -150,43 +143,31 @@ function ReviewViewToggleButton({ function ReviewGroupList({ groups, + activeGroup, onSelectGroup, - onSelectFile, }: { groups: Array + activeGroup: number | null onSelectGroup: (index: number) => void - onSelectFile: (path: string) => void }) { return ( -
+
{groups.map((group) => ( ))}
) } -function splitPath(path: string): { dir: string; base: string } { - const idx = path.lastIndexOf("/") - if (idx === -1) return { dir: "", base: path } - return { dir: path.slice(0, idx), base: path.slice(idx + 1) } -} - -// Older stored summaries embed `[label](#loc=path:line)` diff links. Render the -// label as inline code instead so no stale jump-links leak into the explanation. -function stripLocationLinks(summary: string): string { - return summary.replace(/\[([^\]]+)\]\(#loc=[^)]*\)/g, "`$1`") -} - // Render a title with `backtick`-delimited spans as inline code chips, matching // the Markdown component's inline-code styling, without pulling in the full // block renderer for a single line. -function renderInlineCode(text: string): Array { +export function renderInlineCode(text: string): Array { return text.split(/(`[^`]+`)/g).map((part, i) => { if (part.length >= 2 && part.startsWith("`") && part.endsWith("`")) { return ( @@ -202,26 +183,20 @@ function renderInlineCode(text: string): Array { }) } -// The whole card is the scroll-to-group target so clicks anywhere (including -// the expanded explanation body) focus the diff. Nested controls — the file -// links and the "Read explanation" toggle — stop propagation so they keep -// their own behavior. memo'd + memoized string processing so re-renders from -// sibling state don't re-run Markdown/inline-code work. +// A single agenda entry: just the block number + title, like a Google-Docs +// outline. Clicking (or Enter/Space) scrolls the diff to that block. The active +// block (scroll-spy) gets an accent rule + emphasis. memo'd so scroll-spy +// re-renders only repaint the rows whose active state actually changed. const ReviewGroupRow = memo(function ReviewGroupRow({ group, + active, onSelectGroup, - onSelectFile, }: { group: ReviewSidebarGroup + active: boolean onSelectGroup: (index: number) => void - onSelectFile: (path: string) => void }) { - const [expanded, setExpanded] = useState(false) const title = useMemo(() => renderInlineCode(group.title), [group.title]) - const summary = useMemo( - () => stripLocationLinks(group.summary), - [group.summary] - ) const selectGroup = useCallback( () => onSelectGroup(group.index), [onSelectGroup, group.index] @@ -240,86 +215,29 @@ const ReviewGroupRow = memo(function ReviewGroupRow({
-
- - {group.index} - - - - {title} - - - - {group.fileCount} file{group.fileCount === 1 ? "" : "s"} - - {group.additions > 0 && ( - +{group.additions} - )} - {group.deletions > 0 && ( - -{group.deletions} - )} - - -
- - {group.files.length > 0 && ( -
- {group.files.map((path) => { - const { dir, base } = splitPath(path) - return ( - - ) - })} -
- )} - - {group.summary && ( -
- - {expanded && ( -
- -
- )} -
- )} + + {group.index}. + + + {title} +
) }) diff --git a/ui/src/components/agents/utils/diffUtils.ts b/ui/src/components/agents/utils/diffUtils.ts index c502f86e..f4f2dd4c 100644 --- a/ui/src/components/agents/utils/diffUtils.ts +++ b/ui/src/components/agents/utils/diffUtils.ts @@ -70,6 +70,18 @@ export const DIFF_UNSAFE_CSS = ` [data-gutter-buffer="annotation"][data-selected-line] { --diffs-line-bg: var(--ui-panel) !important; } + +/* Pin every code row to one exact, uniform height (kept in sync with + DIFF_VIRTUAL_METRICS.lineHeight below). In scroll mode code never wraps, so a + hard height won't clip content — it just makes the virtualizer's per-line + estimate match measured layout, so scroll-to lands precisely instead of + over/under-shooting as off-estimate rows reconcile while scrolling. */ +[data-line] { + height: 18px !important; + min-height: 18px !important; + max-height: 18px !important; + line-height: 18px !important; +} ` export const diffOptions = { @@ -104,6 +116,8 @@ export const DIFF_VIRTUALIZER_CONFIG = { export const DIFF_VIRTUAL_METRICS = { hunkLineCount: 80, + // Must match the hard `[data-line]` height pinned in DIFF_UNSAFE_CSS so the + // virtualizer's pre-measurement estimate equals the measured row height. lineHeight: 18, diffHeaderHeight: 0, spacing: 8,