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] <open-swe@users.noreply.github.com> (cherry picked from commit 0b76afdc955e33805c7623d1502a75a9c7c9c1b7)
2026-09-30 10:23:14 +00:00 · 2026-06-30 15:45:32 -07:00 · 2026-06-30 15:45:32 -07:00 · 4c567d8b29
commit 4c567d8b29
parent e3c3ce3f8a
3 changed files with 211 additions and 141 deletions
--- 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<ReturnType<typeof useVirtualizer>>
+
+// 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<HTMLDivElement | null>(null)
+  const virtualizerRef = useRef<DiffVirtualizer | null>(null)
  const findingScrollRequestRef = useRef(0)
  const groupRefs = useRef<Record<number, HTMLDivElement | null>>({})
+  // The block pinned at the top of the diff (scroll-spy), highlighted in the
+  // agenda sidebar.
+  const [activeGroup, setActiveGroup] = useState<number | null>(null)
  const [diffStyle, setDiffStyleState] = useState<DiffStyle>(() =>
    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}
              >
-                <div ref={scrollerProbe} aria-hidden className="hidden" />
+                <VirtualizerBridge
+                  probeRef={scrollerProbe}
+                  instanceRef={virtualizerRef}
+                />
                <PrHeader
                  url={detail.url}
                  title={detail.pr.title}
@ -1271,21 +1376,54 @@ function DiffStyleButton({
  )
 }

+// Grabs the virtualizer instance from context (only available inside
+// <Virtualizer>) 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<DiffVirtualizer | null>
+}) {
+  const virtualizer = useVirtualizer()
+  useEffect(() => {
+    instanceRef.current = virtualizer ?? null
+  }, [virtualizer, instanceRef])
+  return <div ref={probeRef} aria-hidden className="hidden" />
+}
+
+// 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 (
-    <div className="flex items-center gap-2">
-      <span className="flex size-5 shrink-0 items-center justify-center rounded bg-[var(--ui-panel-2)] text-[11px] font-medium text-muted-foreground">
-        {group.index}
-      </span>
-      <h3 className="min-w-0 truncate text-sm font-medium">{group.title}</h3>
-      <span className="flex shrink-0 items-center gap-1.5 font-mono text-[11px]">
-        {group.additions > 0 && (
-          <span className="text-emerald-500">+{group.additions}</span>
-        )}
-        {group.deletions > 0 && (
-          <span className="text-red-500">-{group.deletions}</span>
-        )}
-      </span>
+    <div className="sticky top-0 z-[5] border-b border-border bg-background pb-2">
+      <div className="flex items-center gap-2">
+        <span className="flex size-5 shrink-0 items-center justify-center rounded bg-[var(--ui-panel-2)] text-[11px] font-medium text-muted-foreground">
+          {group.index}
+        </span>
+        <h3 className="min-w-0 flex-1 truncate text-sm font-medium">{title}</h3>
+        <span className="flex shrink-0 items-center gap-1.5 font-mono text-[11px]">
+          {group.additions > 0 && (
+            <span className="text-emerald-500">+{group.additions}</span>
+          )}
+          {group.deletions > 0 && (
+            <span className="text-red-500">-{group.deletions}</span>
+          )}
+        </span>
+      </div>
+      {summary && (
+        <div className="mt-2 max-h-40 overflow-y-auto text-xs text-muted-foreground">
+          <Markdown content={summary} />
+        </div>
+      )}
    </div>
  )
 }
--- 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<string>
 }

 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 ? (
        <ReviewGroupList
          groups={data.groups ?? []}
+          activeGroup={data.activeGroup}
          onSelectGroup={data.onSelectGroup}
-          onSelectFile={data.onSelect}
        />
      ) : !data.files ? (
        <div className="px-4 pt-1">
@ -150,43 +143,31 @@ function ReviewViewToggleButton({

 function ReviewGroupList({
  groups,
+  activeGroup,
  onSelectGroup,
-  onSelectFile,
 }: {
  groups: Array<ReviewSidebarGroup>
+  activeGroup: number | null
  onSelectGroup: (index: number) => void
-  onSelectFile: (path: string) => void
 }) {
  return (
-    <div className="min-h-0 flex-1 divide-y divide-[var(--ui-border-subtle)] overflow-y-auto">
+    <div className="min-h-0 flex-1 overflow-y-auto py-1">
      {groups.map((group) => (
        <ReviewGroupRow
          key={group.index}
          group={group}
+          active={group.index === activeGroup}
          onSelectGroup={onSelectGroup}
-          onSelectFile={onSelectFile}
        />
      ))}
    </div>
  )
 }

-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<ReactNode> {
+export function renderInlineCode(text: string): Array<ReactNode> {
  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<ReactNode> {
  })
 }

-// 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({
    <div
      role="button"
      tabIndex={0}
+      aria-current={active ? "true" : undefined}
      onClick={selectGroup}
      onKeyDown={onKeyDown}
-      className="cursor-pointer px-3 py-3 transition-colors hover:bg-[var(--ui-sidebar-hover)]"
+      className={cn(
+        "flex cursor-pointer items-start gap-2 border-l-2 px-3 py-1.5 text-left transition-colors",
+        active
+          ? "border-[var(--ui-accent)] bg-[var(--ui-sidebar-hover)]"
+          : "border-transparent hover:bg-[var(--ui-sidebar-hover)]"
+      )}
    >
-      <div className="flex w-full items-start gap-2 text-left">
-        <span className="mt-0.5 flex size-5 shrink-0 items-center justify-center rounded bg-[var(--ui-panel-2)] text-[11px] font-medium text-[var(--ui-text-dim)]">
-          {group.index}
-        </span>
-        <span className="min-w-0 flex-1">
-          <span className="block text-xs leading-5 font-medium text-[var(--ui-text)]">
-            {title}
-          </span>
-          <span className="mt-1 flex flex-wrap items-center gap-1.5 text-[11px] text-[var(--ui-text-dim)]">
-            <span>
-              {group.fileCount} file{group.fileCount === 1 ? "" : "s"}
-            </span>
-            {group.additions > 0 && (
-              <span className="text-emerald-500">+{group.additions}</span>
-            )}
-            {group.deletions > 0 && (
-              <span className="text-red-500">-{group.deletions}</span>
-            )}
-          </span>
-        </span>
-      </div>
-
-      {group.files.length > 0 && (
-        <div className="mt-2 space-y-0.5 pl-7">
-          {group.files.map((path) => {
-            const { dir, base } = splitPath(path)
-            return (
-              <button
-                key={path}
-                type="button"
-                onClick={(event) => {
-                  event.stopPropagation()
-                  onSelectFile(path)
-                }}
-                title={path}
-                className="flex w-full items-baseline gap-1.5 text-left text-[11px] hover:text-[var(--ui-accent)]"
-              >
-                <span className="shrink-0 font-medium text-[var(--ui-text-muted)]">
-                  {base}
-                </span>
-                {dir && (
-                  <span className="min-w-0 truncate text-[var(--ui-text-dim)]">
-                    {dir}
-                  </span>
-                )}
-              </button>
-            )
-          })}
-        </div>
-      )}
-
-      {group.summary && (
-        <div className="mt-2">
-          <button
-            type="button"
-            onClick={(event) => {
-              event.stopPropagation()
-              setExpanded((value) => !value)
-            }}
-            className="inline-flex items-center gap-1 text-[11px] font-medium text-[var(--ui-accent)]"
-          >
-            <CaretRightIcon
-              className={cn(
-                "size-3 transition-transform",
-                expanded && "rotate-90"
-              )}
-            />
-            Read explanation
-          </button>
-          {expanded && (
-            <div className="mt-1.5">
-              <Markdown content={summary} />
-            </div>
-          )}
-        </div>
-      )}
+      <span className="mt-px shrink-0 text-[11px] font-medium text-[var(--ui-text-dim)] tabular-nums">
+        {group.index}.
+      </span>
+      <span
+        className={cn(
+          "min-w-0 text-xs leading-5",
+          active
+            ? "font-medium text-[var(--ui-text)]"
+            : "text-[var(--ui-text-muted)]"
+        )}
+      >
+        {title}
+      </span>
    </div>
  )
 })
--- 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,