fix: Reviews tab — anchored finding card, paginated list, file tree truncation (#1507)

* fix: Reviews tab — anchored finding card, paginated list, file tree truncation

- Finding card now tracks the diff anchor while scrolling instead of staying
  frozen in the viewport; auto-hides when its diff card collapses (including
  collapse via mark-as-viewed) and on click outside
- Checks section capped with max height + scroll
- /reviews paginated (page size 20) with has_more, filtered to the current
  user's PRs by default with an All toggle; PR author login now stored in
  reviewer thread metadata
- File tree truncation marker overlapped filenames because the sidebar bg
  was transparent; use the opaque sidebar color

* fix: finding card tracks anchor 1:1 while scrolling

Drop the vertical viewport clamp — it pinned the card at the clamp
boundary while the highlighted lines kept scrolling, breaking the
attachment.

* fix: anchor finding card with Base UI popover

Replace manual fixed-position tracking (laggy: setState per scroll
frame) with a Popover anchored to the finding's diff row. Floating UI
tracks the anchor outside React renders, so the card moves 1:1 with
the content and scrolls out of view with it. Unanchored findings keep
the fixed top-right card.

* fix: lock finding card to diff scroll

Replace the Base UI popover (async repositioning, paints a frame behind
native scroll) with a card absolutely positioned inside the scroll
container, so it scrolls with the diff in the same compositor frame.
Scroll moves to the ReviewBody root, side panel becomes sticky.
Position recomputes only on layout shifts via ResizeObserver.

---------

Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com>
This commit is contained in:
Johannes du Plessis 2026-06-11 17:52:20 -07:00 • committed by GitHub
parent 3724014d80
commit 3e105f5027
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
8 changed files with 268 additions and 78 deletions

View file

@ -170,6 +170,7 @@ def _thread_review_summary(thread: dict[str, Any]) -> dict[str, Any] | None:
"url": pr.get("url") or f"https://github.com/{owner}/{name}/pull/{number}",
"head_ref": pr.get("head_ref") or "",
"base_ref": pr.get("base_ref") or "",
"author": pr.get("author") if isinstance(pr.get("author"), str) else "",
"head_sha": metadata.get("head_sha") or "",
"watch": bool(metadata.get("watch")),
"status": _run_status(thread, metadata),
@ -179,27 +180,35 @@ def _thread_review_summary(thread: dict[str, Any]) -> dict[str, Any] | None:
async def list_reviews(
limit: int = 100,
limit: int = 20,
*,
offset: int = 0,
author: str | None = None,
is_accessible: Callable[[dict[str, Any]], Awaitable[bool]] | None = None,
page_size: int = 100,
max_scan: int = 1000,
) -> list[dict[str, Any]]:
) -> tuple[list[dict[str, Any]], bool]:
"""List review summaries, newest first.
Returns ``(summaries, has_more)`` where the summaries are the page at
``offset`` (counted in accessible, filter-matching records) and
``has_more`` says whether at least one more record exists past it.
When ``is_accessible`` is given, keeps paging through reviewer threads
until ``limit`` accessible summaries are collected (or ``max_scan``
threads have been examined), so inaccessible records don't crowd
accessible ones out of a single fixed-size page.
until enough accessible summaries are collected (or ``max_scan`` threads
have been examined), so inaccessible records don't crowd accessible ones
out of a single fixed-size page. ``author`` filters on the PR author's
GitHub login stored in thread metadata.
"""
client = langgraph_client()
needed = offset + limit + 1
summaries: list[dict[str, Any]] = []
offset = 0
while len(summaries) < limit and offset < max_scan:
scan_offset = 0
while len(summaries) < needed and scan_offset < max_scan:
threads = await client.threads.search(
metadata={"kind": REVIEWER_THREAD_KIND},
limit=page_size,
offset=offset,
offset=scan_offset,
sort_by="updated_at",
sort_order="desc",
)
@ -211,15 +220,18 @@ async def list_reviews(
summary = _thread_review_summary(thread)
if not summary:
continue
if author is not None and summary["author"] != author:
continue
if is_accessible is not None and not await is_accessible(summary):
continue
summaries.append(summary)
if len(summaries) >= limit:
if len(summaries) >= needed:
break
if len(threads) < page_size:
break
offset += page_size
return summaries
scan_offset += page_size
page = summaries[offset : offset + limit]
return page, len(summaries) > offset + limit
def _user_ref(value: Any) -> dict[str, Any] | None:

View file

@ -709,10 +709,15 @@ async def api_list_review_styles(
return out
REVIEWS_PAGE_SIZE = 20
@router.get("/reviews")
async def api_list_reviews(
page: int = 0,
mine: bool = True,
session: dict[str, Any] = _SESSION_DEP,
) -> list[dict[str, Any]]:
) -> dict[str, Any]:
login = session["sub"]
access_cache: dict[str, bool] = {}
@ -728,7 +733,14 @@ async def api_list_reviews(
access_cache[full_name] = False
return access_cache[full_name]
return await list_reviews(is_accessible=is_accessible)
page = max(page, 0)
reviews, has_more = await list_reviews(
REVIEWS_PAGE_SIZE,
offset=page * REVIEWS_PAGE_SIZE,
author=login if mine else None,
is_accessible=is_accessible,
)
return {"reviews": reviews, "page": page, "has_more": has_more}
@router.get("/reviews/{owner}/{repo}/{pr_number}")

View file

@ -171,6 +171,7 @@ class ReviewerPRMeta(TypedDict, total=False):
title: str
head_ref: str
base_ref: str
author: str
class ReviewerSlackThread(TypedDict, total=False):

View file

@ -1865,6 +1865,7 @@ async def trigger_pr_review_from_ref(
"title": pr_title,
"head_ref": branch_name,
"base_ref": base_ref,
"author": (pr_metadata.get("user") or {}).get("login", ""),
}
slack_thread_meta: ReviewerSlackThread | None = None
if slack_channel_id and slack_thread_ts:
@ -2020,6 +2021,7 @@ async def _dispatch_first_review_from_pr_payload(payload: dict[str, Any], *, sou
"title": pr_title,
"head_ref": branch_name,
"base_ref": base_ref,
"author": (pull_request.get("user") or {}).get("login", ""),
}
last_reviewed_sha = ""
if payload.get("action") == "ready_for_review":
@ -2496,6 +2498,7 @@ async def process_github_push_event(payload: dict[str, Any]) -> None:
"title": pr_title,
"head_ref": head_ref,
"base_ref": base_ref,
"author": (pr.get("user") or {}).get("login", ""),
}
await set_reviewer_thread_metadata(thread_id, pr=pr_meta, watch=True, head_sha=head_sha)

View file

@ -122,7 +122,9 @@ function ReviewFileTreeExplorer({
{
height: "100%",
...treeThemeStyle(),
"--trees-theme-sidebar-bg": "transparent",
// Must stay opaque: the tree's truncation marker ("…") paints
// this color behind itself to hide the overflowing filename.
"--trees-theme-sidebar-bg": "var(--ui-sidebar)",
} as React.CSSProperties
}
/>

View file

@ -311,6 +311,7 @@ export interface ReviewSummary {
url: string;
head_ref: string;
base_ref: string;
author: string;
head_sha: string;
watch: boolean;
status: "running" | "error" | "idle";
@ -319,6 +320,12 @@ export interface ReviewSummary {
full_name?: string;
}
export interface ReviewListPayload {
reviews: Array<ReviewSummary>;
page: number;
has_more: boolean;
}
export interface ReviewUserRef {
login: string;
avatar_url?: string | null;
@ -470,7 +477,8 @@ export const api = {
`/admin/user-mappings/${encodeURIComponent(github_login)}`,
{ method: "DELETE" },
),
listReviews: () => request<Array<ReviewSummary>>("/reviews"),
listReviews: (page: number, mine: boolean) =>
request<ReviewListPayload>(`/reviews?page=${page}&mine=${mine}`),
getReview: (owner: string, repo: string, number: number) =>
request<ReviewDetail>(
`/reviews/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/${number}`,

View file

@ -1,6 +1,13 @@
import { Link, Navigate, createFileRoute } from "@tanstack/react-router"
import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"
import { useCallback, useEffect, useMemo, useRef, useState } from "react"
import {
useCallback,
useEffect,
useLayoutEffect,
useMemo,
useRef,
useState,
} from "react"
import {
ArrowClockwiseIcon,
ArrowLeftIcon,
@ -213,9 +220,8 @@ function ReviewBody({
{}
)
const [focused, setFocused] = useState<ReviewFinding | null>(null)
const [cardPos, setCardPos] = useState<{ top: number; left: number | null }>(
{ top: 96, left: null }
)
const [anchorEl, setAnchorEl] = useState<HTMLDivElement | null>(null)
const scrollRef = useRef<HTMLDivElement | null>(null)
const viewedStorageKey = `open-swe.review.viewed.${detail.owner}/${detail.repo}/${detail.number}.${detail.head_sha}`
const [viewed, setViewed] = useState<Set<string>>(() => {
@ -307,7 +313,7 @@ function ReviewBody({
markRead(finding.id)
setFocused(finding)
if (!isAnchored(finding)) {
setCardPos({ top: 96, left: null })
setAnchorEl(null)
return
}
setExpandedFiles((prev) => ({ ...prev, [finding.file]: true }))
@ -315,20 +321,7 @@ function ReviewBody({
const node =
anchorRefs.current[finding.id] ?? fileRefs.current[finding.file]
node?.scrollIntoView({ block: "center" })
requestAnimationFrame(() => {
const rect = anchorRefs.current[finding.id]?.getBoundingClientRect()
if (rect) {
setCardPos({
top: Math.min(
Math.max(rect.top, 64),
window.innerHeight - 360
),
left: Math.min(rect.right + 12, window.innerWidth - 412 - 8),
})
} else {
setCardPos({ top: 96, left: null })
}
})
setAnchorEl(anchorRefs.current[finding.id] ?? null)
})
},
[markRead]
@ -353,13 +346,26 @@ function ReviewBody({
const onKeyDown = (event: KeyboardEvent) => {
if (event.key === "Escape") setFocused(null)
}
const onPointerDown = (event: PointerEvent) => {
const target = event.target
if (target instanceof Element && target.closest("[data-finding-card]"))
return
setFocused(null)
}
window.addEventListener("keydown", onKeyDown)
return () => window.removeEventListener("keydown", onKeyDown)
window.addEventListener("pointerdown", onPointerDown)
return () => {
window.removeEventListener("keydown", onKeyDown)
window.removeEventListener("pointerdown", onPointerDown)
}
}, [focused])
return (
<div className="relative flex min-h-0 flex-1">
<main className="min-w-0 flex-1 overflow-y-auto">
<div
ref={scrollRef}
className="relative flex min-h-0 flex-1 overflow-y-auto"
>
<main className="min-w-0 flex-1">
<div className="mx-auto max-w-6xl px-6 py-6">
<PrHeader detail={detail} />
<div className="mt-4 rounded-lg border border-border bg-card p-4">
@ -398,18 +404,27 @@ function ReviewBody({
findings={findingsByFile.get(file.path) ?? []}
focused={focused}
viewed={viewed.has(file.path)}
onToggleViewed={() => toggleViewed(file.path)}
onToggleViewed={() => {
const collapses =
!viewed.has(file.path) &&
expandedFiles[file.path] === undefined
if (collapses && focused?.file === file.path)
setFocused(null)
toggleViewed(file.path)
}}
expanded={
expandedFiles[file.path] ?? !viewed.has(file.path)
}
onToggleExpanded={() =>
onToggleExpanded={() => {
const next = !(
expandedFiles[file.path] ?? !viewed.has(file.path)
)
if (!next && focused?.file === file.path) setFocused(null)
setExpandedFiles((prev) => ({
...prev,
[file.path]: !(
prev[file.path] ?? !viewed.has(file.path)
),
[file.path]: next,
}))
}
}}
onFindingClick={openFinding}
sectionRef={(node) => {
fileRefs.current[file.path] = node
@ -435,15 +450,30 @@ function ReviewBody({
onFindingClick={openFinding}
/>
{focused && (
<FindingFloatingCard
detail={detail}
finding={focused}
top={cardPos.top}
left={cardPos.left}
onClose={closeFinding}
/>
)}
{focused &&
(isAnchored(focused) && anchorEl ? (
<AnchoredFindingCard
key={focused.id}
detail={detail}
finding={focused}
anchorEl={anchorEl}
scrollRef={scrollRef}
onClose={closeFinding}
/>
) : (
<div
data-finding-card
role="dialog"
aria-label={focused.title}
className={cn(FINDING_CARD_CLASS, "fixed top-24 right-1 z-50")}
>
<FindingCardContent
detail={detail}
finding={focused}
onClose={closeFinding}
/>
</div>
))}
</div>
)
}
@ -698,17 +728,83 @@ function DiffLineRow({
)
}
function FindingFloatingCard({
const FINDING_CARD_WIDTH = 412
const FINDING_CARD_GAP = 12
const FINDING_CARD_CLASS =
"flex max-h-[70vh] w-[412px] flex-col overflow-hidden rounded-lg border border-border bg-background shadow-2xl"
// Positioned absolutely inside the scroll container so it scrolls natively
// with the diff — no per-scroll JS repositioning, hence no lag. Position is
// recomputed only when layout shifts (files expand/collapse, resize).
function AnchoredFindingCard({
detail,
finding,
anchorEl,
scrollRef,
onClose,
}: {
detail: ReviewDetail
finding: ReviewFinding
anchorEl: HTMLDivElement
scrollRef: React.RefObject<HTMLDivElement | null>
onClose: () => void
}) {
const cardRef = useRef<HTMLDivElement | null>(null)
useLayoutEffect(() => {
const card = cardRef.current
const scroller = scrollRef.current
if (!card || !scroller) return
const position = () => {
if (!anchorEl.isConnected) return
const scrollerRect = scroller.getBoundingClientRect()
const anchorRect = anchorEl.getBoundingClientRect()
const top = anchorRect.top - scrollerRect.top + scroller.scrollTop
const left = Math.max(
FINDING_CARD_GAP,
Math.min(
anchorRect.right -
scrollerRect.left +
scroller.scrollLeft +
FINDING_CARD_GAP,
scroller.clientWidth - FINDING_CARD_WIDTH - FINDING_CARD_GAP
)
)
card.style.top = `${top}px`
card.style.left = `${left}px`
}
position()
const observer = new ResizeObserver(position)
observer.observe(scroller)
for (const child of scroller.children) {
if (child !== card) observer.observe(child)
}
return () => observer.disconnect()
}, [anchorEl, scrollRef])
return (
<div
ref={cardRef}
data-finding-card
role="dialog"
aria-label={finding.title}
className={cn(FINDING_CARD_CLASS, "absolute z-50")}
>
<FindingCardContent detail={detail} finding={finding} onClose={onClose} />
</div>
)
}
function FindingCardContent({
detail,
finding,
top,
left,
onClose,
}: {
detail: ReviewDetail
finding: ReviewFinding
top: number
left: number | null
onClose: () => void
}) {
const [copied, setCopied] = useState(false)
@ -737,15 +833,7 @@ function FindingFloatingCard({
}
return (
<div
className={cn(
"fixed z-50 flex max-h-[70vh] w-[412px] flex-col overflow-hidden rounded-lg border border-border bg-background shadow-2xl",
left === null && "right-1"
)}
style={left === null ? { top } : { top, left }}
role="dialog"
aria-label={finding.title}
>
<>
<div className="flex items-center gap-2 border-b border-border px-4 py-2.5 text-xs">
<Icon className={cn("size-3.5", style.className)} />
<span className={cn("font-medium", style.className)}>
@ -797,7 +885,7 @@ function FindingFloatingCard({
</a>
)}
</div>
</div>
</>
)
}
@ -844,7 +932,7 @@ function SidePanel({
return (
<aside
className={cn(
"hidden w-[420px] shrink-0 flex-col overflow-y-auto border-l border-border transition-opacity xl:flex",
"sticky top-0 hidden h-full w-[420px] shrink-0 flex-col overflow-y-auto border-l border-border transition-opacity xl:flex",
dimmed && "pointer-events-none opacity-30"
)}
>
@ -1062,7 +1150,7 @@ function ChecksSection({ checks }: { checks: Array<ReviewCheckRun> }) {
{checks.length === 0 ? (
<p className="text-[11px] text-muted-foreground">No checks reported.</p>
) : (
<div className="space-y-1">
<div className="max-h-56 space-y-1 overflow-y-auto">
{checks.map((check, index) =>
check.url ? (
<a

View file

@ -1,5 +1,6 @@
import { Link, createFileRoute } from "@tanstack/react-router"
import { useQuery } from "@tanstack/react-query"
import { keepPreviousData, useQuery } from "@tanstack/react-query"
import { useState } from "react"
import {
BugBeetleIcon,
FlagIcon,
@ -7,6 +8,7 @@ import {
} from "@phosphor-icons/react"
import type { ReviewSummary } from "@/lib/api"
import { Button } from "@/components/ui/button"
import { Skeleton } from "@/components/ui/skeleton"
import { api } from "@/lib/api"
import { useSession } from "@/lib/session"
@ -33,14 +35,21 @@ function statusBadge(review: ReviewSummary) {
function ReviewsPage() {
const session = useSession()
const [mine, setMine] = useState(true)
const [page, setPage] = useState(0)
const reviews = useQuery({
queryKey: ["reviews"],
queryFn: api.listReviews,
queryKey: ["reviews", mine, page],
queryFn: () => api.listReviews(page, mine),
enabled: !!session.data,
placeholderData: keepPreviousData,
refetchInterval: (query) =>
query.state.data?.some((r) => r.status === "running") ? 5000 : false,
query.state.data?.reviews.some((r) => r.status === "running")
? 5000
: false,
})
const items = reviews.data?.reviews ?? []
return (
<main className="min-w-0 flex-1 overflow-y-auto">
<div className="mx-auto max-w-3xl px-6 py-8">
@ -52,7 +61,33 @@ function ReviewsPage() {
analysis.
</p>
<div className="mt-6 overflow-hidden rounded-lg border border-[var(--ui-border)] bg-[var(--ui-panel)]">
<div className="mt-6 flex items-center gap-1">
{(
[
[true, "My PRs"],
[false, "All"],
] as const
).map(([value, label]) => (
<button
key={label}
type="button"
onClick={() => {
setMine(value)
setPage(0)
}}
className={cn(
"rounded-md px-2.5 py-1 text-xs transition-colors",
mine === value
? "bg-[var(--ui-sidebar-hover)] font-medium text-[var(--ui-text)]"
: "text-[var(--ui-text-muted)] hover:bg-[var(--ui-sidebar-hover)]"
)}
>
{label}
</button>
))}
</div>
<div className="mt-3 overflow-hidden rounded-lg border border-[var(--ui-border)] bg-[var(--ui-panel)]">
{reviews.isLoading && (
<div className="p-4">
<Skeleton className="h-24 w-full" />
@ -63,14 +98,15 @@ function ReviewsPage() {
{reviews.error.message}
</p>
)}
{reviews.data && reviews.data.length === 0 && (
{reviews.data && items.length === 0 && (
<p className="px-4 py-3 text-xs text-[var(--ui-text-muted)]">
No reviews yet. Enable repositories under Open SWE Review settings
and open a PR.
{mine
? "No reviews on your PRs yet. Switch to All to see every review you have access to."
: "No reviews yet. Enable repositories under Open SWE Review settings and open a PR."}
</p>
)}
<div className="divide-y divide-[var(--ui-border)]">
{(reviews.data ?? []).map((review) => (
{items.map((review) => (
<Link
key={review.thread_id}
to="/agents/reviews/$owner/$repo/$number"
@ -89,6 +125,9 @@ function ReviewsPage() {
</div>
<div className="mt-0.5 text-xs text-[var(--ui-text-muted)]">
{review.owner}/{review.repo}#{review.number}
{review.author && !mine && (
<span className="ml-2">by {review.author}</span>
)}
{review.head_ref && (
<span className="ml-2 font-mono text-[11px]">
{review.head_ref}
@ -118,6 +157,31 @@ function ReviewsPage() {
</Link>
))}
</div>
{(page > 0 || reviews.data?.has_more) && (
<div className="flex items-center justify-between gap-4 border-t border-[var(--ui-border)] px-4 py-2 text-xs">
<span className="text-[var(--ui-text-muted)]">
Page {page + 1}
</span>
<div className="flex items-center gap-2">
<Button
size="sm"
variant="outline"
disabled={page === 0}
onClick={() => setPage((p) => Math.max(0, p - 1))}
>
Prev
</Button>
<Button
size="sm"
variant="outline"
disabled={!reviews.data?.has_more}
onClick={() => setPage((p) => p + 1)}
>
Next
</Button>
</div>
</div>
)}
</div>
</div>
</main>