mirror of
https://github.com/Sea-Haven-Industries/open-swe.git
synced 2026-10-04 03:13:20 +00:00
perf: speed up Reviews list (My PRs + All PRs) (#1518)
* 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] <open-swe@users.noreply.github.com>
This commit is contained in:
parent
e8bb6b497b
commit
67239bb2c3
4 changed files with 167 additions and 24 deletions
|
|
@ -187,19 +187,25 @@ async def list_reviews(
|
||||||
``offset`` (counted in accessible, filter-matching records) and
|
``offset`` (counted in accessible, filter-matching records) and
|
||||||
``has_more`` says whether at least one more record exists past it.
|
``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
|
When ``is_accessible`` is given, keeps paging through reviewer threads
|
||||||
until enough accessible summaries are collected (or ``max_scan`` threads
|
until enough accessible summaries are collected (or ``max_scan`` threads
|
||||||
have been examined), so inaccessible records don't crowd accessible ones
|
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
|
out of a single fixed-size page.
|
||||||
GitHub login stored in thread metadata.
|
|
||||||
"""
|
"""
|
||||||
client = langgraph_client()
|
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
|
needed = offset + limit + 1
|
||||||
summaries: list[dict[str, Any]] = []
|
summaries: list[dict[str, Any]] = []
|
||||||
scan_offset = 0
|
scan_offset = 0
|
||||||
while len(summaries) < needed and scan_offset < max_scan:
|
while len(summaries) < needed and scan_offset < max_scan:
|
||||||
threads = await client.threads.search(
|
threads = await client.threads.search(
|
||||||
metadata={"kind": REVIEWER_THREAD_KIND},
|
metadata=search_metadata,
|
||||||
limit=page_size,
|
limit=page_size,
|
||||||
offset=scan_offset,
|
offset=scan_offset,
|
||||||
sort_by="updated_at",
|
sort_by="updated_at",
|
||||||
|
|
@ -213,8 +219,6 @@ async def list_reviews(
|
||||||
summary = _thread_review_summary(thread)
|
summary = _thread_review_summary(thread)
|
||||||
if not summary:
|
if not summary:
|
||||||
continue
|
continue
|
||||||
if author is not None and summary["author"] != author:
|
|
||||||
continue
|
|
||||||
if is_accessible is not None and not await is_accessible(summary):
|
if is_accessible is not None and not await is_accessible(summary):
|
||||||
continue
|
continue
|
||||||
summaries.append(summary)
|
summaries.append(summary)
|
||||||
|
|
|
||||||
|
|
@ -620,17 +620,16 @@ async def _paginate(
|
||||||
return out
|
return out
|
||||||
|
|
||||||
|
|
||||||
@router.get("/repos")
|
async def _fetch_user_installations_and_repos(
|
||||||
async def list_repos(
|
login: str,
|
||||||
session: dict[str, Any] = _SESSION_DEP,
|
) -> tuple[list[dict[str, Any]], list[dict[str, Any]]]:
|
||||||
) -> dict[str, Any]:
|
"""Resolve the installations and repos a user can access via the GitHub App.
|
||||||
"""List repos where open-swe is installed and the user has access.
|
|
||||||
|
|
||||||
Paginates both ``/user/installations`` and per-installation
|
Paginates both ``/user/installations`` and per-installation
|
||||||
``/user/installations/{id}/repositories`` so users with multiple
|
``/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)
|
token = await get_valid_access_token(login)
|
||||||
if not token:
|
if not token:
|
||||||
raise HTTPException(401, "github token unavailable, re-login required")
|
raise HTTPException(401, "github token unavailable, re-login required")
|
||||||
|
|
@ -680,6 +679,30 @@ async def list_repos(
|
||||||
continue
|
continue
|
||||||
raise
|
raise
|
||||||
repositories.extend(repos)
|
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 {
|
return {
|
||||||
"installations": [
|
"installations": [
|
||||||
{
|
{
|
||||||
|
|
@ -722,19 +745,10 @@ async def api_list_reviews(
|
||||||
session: dict[str, Any] = _SESSION_DEP,
|
session: dict[str, Any] = _SESSION_DEP,
|
||||||
) -> dict[str, Any]:
|
) -> dict[str, Any]:
|
||||||
login = session["sub"]
|
login = session["sub"]
|
||||||
access_cache: dict[str, bool] = {}
|
accessible = await accessible_repo_full_names(login)
|
||||||
|
|
||||||
async def is_accessible(summary: dict[str, Any]) -> bool:
|
async def is_accessible(summary: dict[str, Any]) -> bool:
|
||||||
full_name = summary["full_name"]
|
return summary["full_name"].lower() in accessible
|
||||||
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]
|
|
||||||
|
|
||||||
page = max(page, 0)
|
page = max(page, 0)
|
||||||
reviews, has_more = await list_reviews(
|
reviews, has_more = await list_reviews(
|
||||||
|
|
|
||||||
108
tests/test_dashboard_reviews.py
Normal file
108
tests/test_dashboard_reviews.py
Normal file
|
|
@ -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
|
||||||
|
|
@ -1,5 +1,9 @@
|
||||||
import { Link, createFileRoute } from "@tanstack/react-router"
|
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 { useState } from "react"
|
||||||
import {
|
import {
|
||||||
BugBeetleIcon,
|
BugBeetleIcon,
|
||||||
|
|
@ -35,6 +39,7 @@ function statusBadge(review: ReviewSummary) {
|
||||||
|
|
||||||
function ReviewsPage() {
|
function ReviewsPage() {
|
||||||
const session = useSession()
|
const session = useSession()
|
||||||
|
const queryClient = useQueryClient()
|
||||||
const [mine, setMine] = useState(true)
|
const [mine, setMine] = useState(true)
|
||||||
const [page, setPage] = useState(0)
|
const [page, setPage] = useState(0)
|
||||||
const reviews = useQuery({
|
const reviews = useQuery({
|
||||||
|
|
@ -48,6 +53,14 @@ function ReviewsPage() {
|
||||||
: false,
|
: 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 ?? []
|
const items = reviews.data?.reviews ?? []
|
||||||
|
|
||||||
return (
|
return (
|
||||||
|
|
@ -75,6 +88,8 @@ function ReviewsPage() {
|
||||||
setMine(value)
|
setMine(value)
|
||||||
setPage(0)
|
setPage(0)
|
||||||
}}
|
}}
|
||||||
|
onPointerEnter={() => prefetch(value, 0)}
|
||||||
|
onFocus={() => prefetch(value, 0)}
|
||||||
className={cn(
|
className={cn(
|
||||||
"rounded-md px-2.5 py-1 text-xs transition-colors",
|
"rounded-md px-2.5 py-1 text-xs transition-colors",
|
||||||
mine === value
|
mine === value
|
||||||
|
|
@ -167,6 +182,7 @@ function ReviewsPage() {
|
||||||
size="sm"
|
size="sm"
|
||||||
variant="outline"
|
variant="outline"
|
||||||
disabled={page === 0}
|
disabled={page === 0}
|
||||||
|
onPointerEnter={() => prefetch(mine, page - 1)}
|
||||||
onClick={() => setPage((p) => Math.max(0, p - 1))}
|
onClick={() => setPage((p) => Math.max(0, p - 1))}
|
||||||
>
|
>
|
||||||
Prev
|
Prev
|
||||||
|
|
@ -175,6 +191,7 @@ function ReviewsPage() {
|
||||||
size="sm"
|
size="sm"
|
||||||
variant="outline"
|
variant="outline"
|
||||||
disabled={!reviews.data?.has_more}
|
disabled={!reviews.data?.has_more}
|
||||||
|
onPointerEnter={() => prefetch(mine, page + 1)}
|
||||||
onClick={() => setPage((p) => p + 1)}
|
onClick={() => setPage((p) => p + 1)}
|
||||||
>
|
>
|
||||||
Next
|
Next
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue