From 67239bb2c34418ddd97eef2f727e5ae2f6da6919 Mon Sep 17 00:00:00 2001 From: Johannes du Plessis Date: Fri, 12 Jun 2026 14:14:50 -0700 Subject: [PATCH] perf: speed up Reviews list (My PRs + All PRs) (#1518) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * perf: speed up Reviews list (My PRs + All PRs) Push the "My PRs" author filter into the threads.search metadata (pr.author containment) instead of scanning up to 1000 reviewer threads in Python, and replace the per-repo GitHub access check (an N+1 of sequential GET /repos calls) with a single per-login accessible-repo set, cached for 60s. Detail endpoints still re-validate access live. Frontend: prefetch the inactive tab and adjacent page on hover/focus so tab switches and pagination are instant. * fix: don't cache repo access for the reviews list The /reviews list is an authorization boundary for private PR metadata (repo/PR titles, branches, authors, finding counts). A cross-request TTL cache on the accessible-repo set could surface that metadata for up to 60s after a user lost repo access. Resolve the set fresh per request instead — still a fixed, repo-count-independent burst of GitHub calls (no per-repo N+1), with no staleness. --------- Co-authored-by: open-swe[bot] --- agent/dashboard/review_api.py | 14 ++-- agent/dashboard/routes.py | 50 +++++++----- tests/test_dashboard_reviews.py | 108 +++++++++++++++++++++++++ ui/src/routes/agents/reviews/index.tsx | 19 ++++- 4 files changed, 167 insertions(+), 24 deletions(-) create mode 100644 tests/test_dashboard_reviews.py diff --git a/agent/dashboard/review_api.py b/agent/dashboard/review_api.py index e280443e..95d34a4b 100644 --- a/agent/dashboard/review_api.py +++ b/agent/dashboard/review_api.py @@ -187,19 +187,25 @@ async def list_reviews( ``offset`` (counted in accessible, filter-matching records) and ``has_more`` says whether at least one more record exists past it. + ``author`` is pushed into the ``threads.search`` metadata filter + (``pr.author`` containment), so the "My PRs" tab only fetches the user's + own reviewer threads instead of scanning the whole population in Python. + When ``is_accessible`` is given, keeps paging through reviewer threads 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. + out of a single fixed-size page. """ client = langgraph_client() + search_metadata: dict[str, Any] = {"kind": REVIEWER_THREAD_KIND} + if author is not None: + search_metadata["pr"] = {"author": author} needed = offset + limit + 1 summaries: list[dict[str, Any]] = [] scan_offset = 0 while len(summaries) < needed and scan_offset < max_scan: threads = await client.threads.search( - metadata={"kind": REVIEWER_THREAD_KIND}, + metadata=search_metadata, limit=page_size, offset=scan_offset, sort_by="updated_at", @@ -213,8 +219,6 @@ 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) diff --git a/agent/dashboard/routes.py b/agent/dashboard/routes.py index 68e89cb5..76294114 100644 --- a/agent/dashboard/routes.py +++ b/agent/dashboard/routes.py @@ -620,17 +620,16 @@ async def _paginate( return out -@router.get("/repos") -async def list_repos( - session: dict[str, Any] = _SESSION_DEP, -) -> dict[str, Any]: - """List repos where open-swe is installed and the user has access. +async def _fetch_user_installations_and_repos( + login: str, +) -> tuple[list[dict[str, Any]], list[dict[str, Any]]]: + """Resolve the installations and repos a user can access via the GitHub App. Paginates both ``/user/installations`` and per-installation ``/user/installations/{id}/repositories`` so users with multiple - installations or >30 accessible repos get the complete set. + installations or >30 accessible repos get the complete set. Shared by the + ``/repos`` endpoint and the reviews access filter. """ - login = session["sub"] token = await get_valid_access_token(login) if not token: raise HTTPException(401, "github token unavailable, re-login required") @@ -680,6 +679,30 @@ async def list_repos( continue raise repositories.extend(repos) + return installations, repositories + + +async def accessible_repo_full_names(login: str) -> frozenset[str]: + """Lowercased ``owner/name`` of repos the user can currently access. + + Resolved fresh on every call (a fixed, repo-count-independent burst of + GitHub calls) rather than cached. ``/reviews`` uses this set to decide + which private PR metadata a user may see, so it's an authorization + boundary: a stale set would leak repo/PR titles, branches, authors and + finding counts for repos the user just lost access to. + """ + _, repositories = await _fetch_user_installations_and_repos(login) + return frozenset( + repo["full_name"].lower() for repo in repositories if isinstance(repo.get("full_name"), str) + ) + + +@router.get("/repos") +async def list_repos( + session: dict[str, Any] = _SESSION_DEP, +) -> dict[str, Any]: + """List repos where open-swe is installed and the user has access.""" + installations, repositories = await _fetch_user_installations_and_repos(session["sub"]) return { "installations": [ { @@ -722,19 +745,10 @@ async def api_list_reviews( session: dict[str, Any] = _SESSION_DEP, ) -> dict[str, Any]: login = session["sub"] - access_cache: dict[str, bool] = {} + accessible = await accessible_repo_full_names(login) async def is_accessible(summary: dict[str, Any]) -> bool: - full_name = summary["full_name"] - if full_name not in access_cache: - try: - await require_repo_access_for_user(login, full_name) - access_cache[full_name] = True - except HTTPException as exc: - if exc.status_code not in {403, 404}: - raise - access_cache[full_name] = False - return access_cache[full_name] + return summary["full_name"].lower() in accessible page = max(page, 0) reviews, has_more = await list_reviews( diff --git a/tests/test_dashboard_reviews.py b/tests/test_dashboard_reviews.py new file mode 100644 index 00000000..2e59cf70 --- /dev/null +++ b/tests/test_dashboard_reviews.py @@ -0,0 +1,108 @@ +from __future__ import annotations + +from types import SimpleNamespace +from typing import Any +from unittest.mock import AsyncMock + +import pytest + +from agent.dashboard import review_api, routes +from agent.reviewer_findings import REVIEWER_THREAD_KIND + + +def _thread(owner: str, name: str, number: int, author: str) -> dict[str, Any]: + return { + "thread_id": f"{owner}/{name}/{number}", + "status": "idle", + "updated_at": "2026-06-12T00:00:00Z", + "metadata": { + "kind": REVIEWER_THREAD_KIND, + "pr": {"owner": owner, "name": name, "number": number, "author": author}, + }, + } + + +def _fake_client(pages: list[list[dict[str, Any]]]) -> tuple[Any, dict[str, Any]]: + captured: dict[str, Any] = {"calls": []} + queue = list(pages) + + async def search(**kwargs: Any) -> list[dict[str, Any]]: + captured["calls"].append(kwargs) + return queue.pop(0) if queue else [] + + return SimpleNamespace(threads=SimpleNamespace(search=search)), captured + + +@pytest.mark.asyncio +async def test_list_reviews_pushes_author_into_metadata_filter(monkeypatch) -> None: + client, captured = _fake_client([[]]) + monkeypatch.setattr(review_api, "langgraph_client", lambda: client) + + reviews, has_more = await review_api.list_reviews(20, author="octocat") + + assert captured["calls"][0]["metadata"] == { + "kind": REVIEWER_THREAD_KIND, + "pr": {"author": "octocat"}, + } + assert reviews == [] + assert has_more is False + + +@pytest.mark.asyncio +async def test_list_reviews_no_author_filters_kind_only(monkeypatch) -> None: + client, captured = _fake_client([[]]) + monkeypatch.setattr(review_api, "langgraph_client", lambda: client) + + await review_api.list_reviews(20, author=None) + + assert captured["calls"][0]["metadata"] == {"kind": REVIEWER_THREAD_KIND} + + +@pytest.mark.asyncio +async def test_list_reviews_applies_accessibility_and_has_more(monkeypatch) -> None: + page = [ + _thread("acme", "a", 1, "octocat"), + _thread("acme", "b", 2, "octocat"), + _thread("acme", "a", 3, "octocat"), + ] + client, _ = _fake_client([page]) + monkeypatch.setattr(review_api, "langgraph_client", lambda: client) + + async def is_accessible(summary: dict[str, Any]) -> bool: + return summary["full_name"] == "acme/a" + + reviews, has_more = await review_api.list_reviews(1, offset=0, is_accessible=is_accessible) + + assert [r["number"] for r in reviews] == [1] + # two accessible records exist (1 and 3); page of 1 leaves more. + assert has_more is True + + +@pytest.mark.asyncio +async def test_accessible_repo_full_names_lowercases(monkeypatch) -> None: + fetch = AsyncMock(return_value=([], [{"full_name": "Acme/Repo"}, {"full_name": "Acme/Other"}])) + monkeypatch.setattr(routes, "_fetch_user_installations_and_repos", fetch) + + names = await routes.accessible_repo_full_names("octocat") + + assert names == frozenset({"acme/repo", "acme/other"}) + + +@pytest.mark.asyncio +async def test_accessible_repo_full_names_resolves_fresh_each_call(monkeypatch) -> None: + # Access is an authorization boundary, so it must not be cached across calls: + # a second call re-fetches and reflects revoked access. + fetch = AsyncMock( + side_effect=[ + ([], [{"full_name": "acme/repo"}]), + ([], []), + ] + ) + monkeypatch.setattr(routes, "_fetch_user_installations_and_repos", fetch) + + first = await routes.accessible_repo_full_names("octocat") + second = await routes.accessible_repo_full_names("octocat") + + assert first == frozenset({"acme/repo"}) + assert second == frozenset() + assert fetch.await_count == 2 diff --git a/ui/src/routes/agents/reviews/index.tsx b/ui/src/routes/agents/reviews/index.tsx index 170f2eef..c4a12d21 100644 --- a/ui/src/routes/agents/reviews/index.tsx +++ b/ui/src/routes/agents/reviews/index.tsx @@ -1,5 +1,9 @@ import { Link, createFileRoute } from "@tanstack/react-router" -import { keepPreviousData, useQuery } from "@tanstack/react-query" +import { + keepPreviousData, + useQuery, + useQueryClient, +} from "@tanstack/react-query" import { useState } from "react" import { BugBeetleIcon, @@ -35,6 +39,7 @@ function statusBadge(review: ReviewSummary) { function ReviewsPage() { const session = useSession() + const queryClient = useQueryClient() const [mine, setMine] = useState(true) const [page, setPage] = useState(0) const reviews = useQuery({ @@ -48,6 +53,14 @@ function ReviewsPage() { : false, }) + const prefetch = (nextMine: boolean, nextPage: number) => { + if (nextPage < 0) return + void queryClient.prefetchQuery({ + queryKey: ["reviews", nextMine, nextPage], + queryFn: () => api.listReviews(nextPage, nextMine), + }) + } + const items = reviews.data?.reviews ?? [] return ( @@ -75,6 +88,8 @@ function ReviewsPage() { setMine(value) setPage(0) }} + onPointerEnter={() => prefetch(value, 0)} + onFocus={() => prefetch(value, 0)} className={cn( "rounded-md px-2.5 py-1 text-xs transition-colors", mine === value @@ -167,6 +182,7 @@ function ReviewsPage() { size="sm" variant="outline" disabled={page === 0} + onPointerEnter={() => prefetch(mine, page - 1)} onClick={() => setPage((p) => Math.max(0, p - 1))} > Prev @@ -175,6 +191,7 @@ function ReviewsPage() { size="sm" variant="outline" disabled={!reviews.data?.has_more} + onPointerEnter={() => prefetch(mine, page + 1)} onClick={() => setPage((p) => p + 1)} > Next