From 3e105f50270e6db6cd0b4f6542820b9c058e0836 Mon Sep 17 00:00:00 2001 From: Johannes du Plessis Date: Thu, 11 Jun 2026 17:52:20 -0700 Subject: [PATCH] =?UTF-8?q?fix:=20Reviews=20tab=20=E2=80=94=20anchored=20f?= =?UTF-8?q?inding=20card,=20paginated=20list,=20file=20tree=20truncation?= =?UTF-8?q?=20(#1507)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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] --- agent/dashboard/review_api.py | 34 ++- agent/dashboard/routes.py | 16 +- agent/reviewer_findings.py | 1 + agent/webapp.py | 3 + ui/src/components/agents/ReviewSidebar.tsx | 4 +- ui/src/lib/api.ts | 10 +- .../agents/reviews/$owner.$repo.$number.tsx | 196 +++++++++++++----- ui/src/routes/agents/reviews/index.tsx | 82 +++++++- 8 files changed, 268 insertions(+), 78 deletions(-) diff --git a/agent/dashboard/review_api.py b/agent/dashboard/review_api.py index a6672961..fb51fd70 100644 --- a/agent/dashboard/review_api.py +++ b/agent/dashboard/review_api.py @@ -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: diff --git a/agent/dashboard/routes.py b/agent/dashboard/routes.py index 423ddcce..4369218d 100644 --- a/agent/dashboard/routes.py +++ b/agent/dashboard/routes.py @@ -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}") diff --git a/agent/reviewer_findings.py b/agent/reviewer_findings.py index d8cfa59b..7adc23b2 100644 --- a/agent/reviewer_findings.py +++ b/agent/reviewer_findings.py @@ -171,6 +171,7 @@ class ReviewerPRMeta(TypedDict, total=False): title: str head_ref: str base_ref: str + author: str class ReviewerSlackThread(TypedDict, total=False): diff --git a/agent/webapp.py b/agent/webapp.py index be42640a..4fe23385 100644 --- a/agent/webapp.py +++ b/agent/webapp.py @@ -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) diff --git a/ui/src/components/agents/ReviewSidebar.tsx b/ui/src/components/agents/ReviewSidebar.tsx index d04f37bb..db89e669 100644 --- a/ui/src/components/agents/ReviewSidebar.tsx +++ b/ui/src/components/agents/ReviewSidebar.tsx @@ -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 } /> diff --git a/ui/src/lib/api.ts b/ui/src/lib/api.ts index 33d6c6f0..003c6d69 100644 --- a/ui/src/lib/api.ts +++ b/ui/src/lib/api.ts @@ -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; + 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>("/reviews"), + listReviews: (page: number, mine: boolean) => + request(`/reviews?page=${page}&mine=${mine}`), getReview: (owner: string, repo: string, number: number) => request( `/reviews/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/${number}`, diff --git a/ui/src/routes/agents/reviews/$owner.$repo.$number.tsx b/ui/src/routes/agents/reviews/$owner.$repo.$number.tsx index 41c34a6d..2eeb32a7 100644 --- a/ui/src/routes/agents/reviews/$owner.$repo.$number.tsx +++ b/ui/src/routes/agents/reviews/$owner.$repo.$number.tsx @@ -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(null) - const [cardPos, setCardPos] = useState<{ top: number; left: number | null }>( - { top: 96, left: null } - ) + const [anchorEl, setAnchorEl] = useState(null) + const scrollRef = useRef(null) const viewedStorageKey = `open-swe.review.viewed.${detail.owner}/${detail.repo}/${detail.number}.${detail.head_sha}` const [viewed, setViewed] = useState>(() => { @@ -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 ( -
-
+
+
@@ -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 && ( - - )} + {focused && + (isAnchored(focused) && anchorEl ? ( + + ) : ( +
+ +
+ ))}
) } @@ -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 + onClose: () => void +}) { + const cardRef = useRef(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 ( +
+ +
+ ) +} + +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 ( -
+ <>
@@ -797,7 +885,7 @@ function FindingFloatingCard({ )}
-
+ ) } @@ -844,7 +932,7 @@ function SidePanel({ return (