From 801f93b4deac42944ea6330f920d03fc08964126 Mon Sep 17 00:00:00 2001 From: Johannes du Plessis Date: Tue, 16 Jun 2026 13:59:37 -0700 Subject: [PATCH] feat: AI-sorted PR review view with diff grouping (#1544) * feat: AI-sorted PR review view with diff grouping Group a PR's changed files into a logical top-to-bottom walkthrough via a best-effort structured-output LLM pass, kicked off concurrently with the reviewer run (~0 added latency) and persisted on reviewer thread metadata. Results render in the review UI behind an AI sorted / file tree toggle that persists across PRs; the view falls back to the file tree when groups are absent or stale. Adds a grouping-model team default (inherits the Reviewer subagent model when unset) and the admin RolePicker for it. * feat(reviews): richer AI-sorted explanations + sidebar polish Sidebar group rows get Devin-style spacing (dividers, padding), a per-group file list (click a file to jump to its diff), inline-code chips in titles, and an accent Read explanation link. Group explanations are now rich markdown: the grouping prompt feeds per-hunk line ranges and asks for inline code, a short code block, and [path:line](#loc=...) references. The Markdown renderer turns those #loc= links into in-page buttons that scroll the diff to the hunk and highlight the range, reusing the existing selectedLines path. * fix(reviews): drop stale diff groups from the AI-sorted view When groups were generated for a previous head, a persisted "ai" view in localStorage still rendered the outdated walkthrough. groupedView now returns null on diff_groups_stale, so the file-tree fallback is used and the view toggle hides until fresh groups arrive. --------- Co-authored-by: open-swe[bot] --- agent/dashboard/review_api.py | 52 +- agent/dashboard/team_settings.py | 38 + agent/reviewer.py | 64 +- agent/reviewer_groups.py | 241 ++++++ tests/test_review_api.py | 38 + tests/test_reviewer_groups.py | 172 +++++ tests/test_team_settings_grouping.py | 83 +++ ui/src/components/agents/AgentsSidebar.tsx | 4 +- ui/src/components/agents/ReviewSidebar.tsx | 251 ++++++- ui/src/components/agents/ported/Markdown.tsx | 58 +- .../agents/ported/markdownLocation.test.ts | 53 ++ .../agents/ported/markdownLocation.ts | 35 + ui/src/lib/api.ts | 686 +++++++++--------- ui/src/routes/admin.tsx | 32 +- .../agents/reviews/$owner.$repo.$number.tsx | 267 +++++-- 15 files changed, 1684 insertions(+), 390 deletions(-) create mode 100644 agent/reviewer_groups.py create mode 100644 tests/test_reviewer_groups.py create mode 100644 tests/test_team_settings_grouping.py create mode 100644 ui/src/components/agents/ported/markdownLocation.test.ts create mode 100644 ui/src/components/agents/ported/markdownLocation.ts diff --git a/agent/dashboard/review_api.py b/agent/dashboard/review_api.py index 95d34a4b..22d593ba 100644 --- a/agent/dashboard/review_api.py +++ b/agent/dashboard/review_api.py @@ -116,6 +116,47 @@ def classify_finding(finding: dict[str, Any]) -> Literal["bug", "investigate", " return "informational" +def _serialize_diff_groups( + metadata: dict[str, Any], head_sha: str +) -> tuple[list[dict[str, Any]], bool]: + """Serialize the persisted ``diff_groups`` for the AI sorted view. + + Returns ``(groups, stale)`` where each group carries a 1-based ``index`` + and validated fields, and ``stale`` is True when the groups were generated + for a different head SHA than the one currently being rendered. + """ + raw = metadata.get("diff_groups") + if not isinstance(raw, dict): + return [], False + raw_groups = raw.get("groups") + if not isinstance(raw_groups, list): + return [], False + groups: list[dict[str, Any]] = [] + for group in raw_groups: + if not isinstance(group, dict): + continue + title = group.get("title") + if not isinstance(title, str) or not title.strip(): + continue + files = [f for f in (group.get("files") or []) if isinstance(f, str) and f] + if not files: + continue + summary = group.get("summary") + groups.append( + { + "index": len(groups) + 1, + "title": title.strip(), + "summary": summary.strip() if isinstance(summary, str) else "", + "files": files, + } + ) + groups_head = raw.get("head_sha") + stale = bool( + head_sha and isinstance(groups_head, str) and groups_head and groups_head != head_sha + ) + return groups, stale + + def _finding_counts(findings: list[dict[str, Any]]) -> dict[str, int]: counts = {"open": 0, "resolved": 0, "dismissed": 0, "bugs": 0, "flags": 0} for finding in findings: @@ -336,7 +377,16 @@ async def get_review(owner: str, repo: str, pr_number: int) -> dict[str, Any]: for finding in findings: finding["group"] = classify_finding(finding) - return {**summary, "pr": details, "checks": checks, "findings": findings} + diff_groups, diff_groups_stale = _serialize_diff_groups(metadata, head_sha) + + return { + **summary, + "pr": details, + "checks": checks, + "findings": findings, + "diff_groups": diff_groups, + "diff_groups_stale": diff_groups_stale, + } async def get_review_diff(owner: str, repo: str, pr_number: int) -> dict[str, Any]: diff --git a/agent/dashboard/team_settings.py b/agent/dashboard/team_settings.py index ed546609..47af5436 100644 --- a/agent/dashboard/team_settings.py +++ b/agent/dashboard/team_settings.py @@ -52,6 +52,8 @@ class TeamSettingsUpdate(BaseModel): default_reviewer_reasoning_effort: str | None = None default_reviewer_subagent_model: str | None = None default_reviewer_subagent_reasoning_effort: str | None = None + default_grouping_model: str | None = None + default_grouping_reasoning_effort: str | None = None default_chat_model: str | None = None default_chat_reasoning_effort: str | None = None @@ -89,6 +91,11 @@ class TeamSettingsUpdate(BaseModel): self.default_reviewer_subagent_reasoning_effort, "reviewer subagent", ) + _validate_model_effort_pair( + self.default_grouping_model, + self.default_grouping_reasoning_effort, + "review diff grouping", + ) _validate_model_effort_pair( self.default_chat_model, self.default_chat_reasoning_effort, "review chat" ) @@ -144,6 +151,10 @@ def _default_settings() -> dict[str, Any]: "default_reviewer_reasoning_effort": fallback_effort, "default_reviewer_subagent_model": fallback_model, "default_reviewer_subagent_reasoning_effort": fallback_effort, + # No hardcoded grouping default: unset means "inherit the Reviewer + # subagent default". + "default_grouping_model": None, + "default_grouping_reasoning_effort": None, # No hardcoded chat default: unset means "inherit the Agent default". "default_chat_model": None, "default_chat_reasoning_effort": None, @@ -192,6 +203,8 @@ async def upsert_team_settings(update: TeamSettingsUpdate) -> dict[str, Any]: "default_reviewer_reasoning_effort": update.default_reviewer_reasoning_effort, "default_reviewer_subagent_model": update.default_reviewer_subagent_model, "default_reviewer_subagent_reasoning_effort": update.default_reviewer_subagent_reasoning_effort, + "default_grouping_model": update.default_grouping_model, + "default_grouping_reasoning_effort": update.default_grouping_reasoning_effort, "default_chat_model": update.default_chat_model, "default_chat_reasoning_effort": update.default_chat_reasoning_effort, "updated_at": datetime.now(UTC).isoformat(), @@ -268,6 +281,31 @@ async def get_team_default_model_pair( return main, subagent +async def get_team_default_grouping_model() -> tuple[str, str]: + """Return the team-wide default ``(model_id, reasoning_effort)`` for the + review diff-grouping pass. + + When no grouping-specific model is configured (or it's no longer + supported), inherit the team **reviewer subagent** default — the grouping + pass is a cheap, fast companion to the reviewer, so it should track that + cheaper tier rather than the primary reviewer model. + """ + settings = await get_team_settings() + model = settings.get("default_grouping_model") + effort = settings.get("default_grouping_reasoning_effort") + if ( + isinstance(model, str) + and isinstance(effort, str) + and model in SUPPORTED_MODEL_IDS + and model_supports_effort(model, effort) + ): + return _resolve_default_pair(model, effort) + return _resolve_default_pair( + settings.get("default_reviewer_subagent_model"), + settings.get("default_reviewer_subagent_reasoning_effort"), + ) + + async def get_autofix_settings() -> dict[str, Any]: """Return the team-wide auto-fix config: mode, severity threshold, trigger mode.""" settings = await get_team_settings() diff --git a/agent/reviewer.py b/agent/reviewer.py index 536bde8a..ccf1841e 100644 --- a/agent/reviewer.py +++ b/agent/reviewer.py @@ -31,8 +31,13 @@ warnings.filterwarnings("ignore", message=".*Pydantic V1.*", category=UserWarnin from deepagents import create_deep_agent from langchain.agents.middleware import ModelCallLimitMiddleware +from langchain_core.language_models.chat_models import BaseChatModel -from .dashboard.team_settings import get_org_review_guidelines, get_team_default_model_pair +from .dashboard.team_settings import ( + get_org_review_guidelines, + get_team_default_grouping_model, + get_team_default_model_pair, +) from .middleware import ( SanitizeThinkingBlocksMiddleware, SanitizeToolInputsMiddleware, @@ -46,6 +51,7 @@ from .reviewer_diff import compute_diff_line_set, fetch_pr_diff, fetch_pr_metada from .reviewer_findings import ( list_findings as list_findings_async, ) +from .reviewer_groups import maybe_generate_and_store_diff_groups from .reviewer_publish import fetch_pr_review_threads from .reviewer_reconcile import reconcile_findings_with_review_threads from .server import ( @@ -727,6 +733,43 @@ def _format_existing_findings(findings: list[dict]) -> str: return "\n".join(lines) if lines else "_(no open findings)_" +# Strong references to fire-and-forget background tasks (e.g. the AI-sorted +# diff grouping pass) so the event loop doesn't garbage-collect them mid-flight. +_BACKGROUND_TASKS: set[asyncio.Task[None]] = set() + + +def _on_background_task_done(task: asyncio.Task[None]) -> None: + _BACKGROUND_TASKS.discard(task) + if task.cancelled(): + return + exc = task.exception() + if exc is not None: + logger.warning("Background reviewer task failed: %s", exc) + + +async def _resolve_grouping_model(configurable: dict[str, object]) -> BaseChatModel: + """Resolve the model for the diff-grouping pass. + + Per-run override (``grouping_model_id``/``grouping_reasoning_effort``) wins; + otherwise the team default, which itself inherits the reviewer subagent + model when no grouping-specific model is configured. + """ + configured_model_id = configurable.get("grouping_model_id") + configured_effort = configurable.get("grouping_reasoning_effort") + if isinstance(configured_model_id, str) and configured_model_id: + model_id = configured_model_id + effort = configured_effort if isinstance(configured_effort, str) else None + else: + model_id, effort = await get_team_default_grouping_model() + model_kwargs = provider_model_kwargs( + model_id, + effort, + max_tokens=DEFAULT_LLM_MAX_TOKENS, + openai_reasoning_default=DEFAULT_LLM_REASONING, + ) + return make_model(model_id, **model_kwargs) + + async def get_reviewer_agent(config: RunnableConfig) -> Pregel: """Get or create a reviewer agent with a sandbox + prepped repo.""" thread_id = config["configurable"].get("thread_id", None) @@ -1025,6 +1068,25 @@ async def get_reviewer_agent(config: RunnableConfig) -> Pregel: reviewer_model = make_model(model_id, **model_kwargs) reviewer_subagent_model = make_model(subagent_model_id, **subagent_model_kwargs) + + # Kick off the AI-sorted diff grouping pass at run start, concurrently with + # the review, so it adds ~0 latency. First-review and re-review only — a + # finding_reply run doesn't change the diff. Best-effort: the task swallows + # its own errors and the UI falls back to the folder view when groups are + # absent. + if reviewer_event != "finding_reply" and pr_diff_text and thread_id: + grouping_model = await _resolve_grouping_model(config["configurable"]) + grouping_task = asyncio.create_task( + maybe_generate_and_store_diff_groups( + thread_id=thread_id, + head_sha=head_sha, + diff_text=pr_diff_text, + model=grouping_model, + ) + ) + _BACKGROUND_TASKS.add(grouping_task) + grouping_task.add_done_callback(_on_background_task_done) + return create_deep_agent( model=reviewer_model, system_prompt=system_prompt, diff --git a/agent/reviewer_groups.py b/agent/reviewer_groups.py new file mode 100644 index 00000000..36e4c3dc --- /dev/null +++ b/agent/reviewer_groups.py @@ -0,0 +1,241 @@ +"""Diff grouping for the reviewer "AI sorted" view. + +A lightweight, single structured-output LLM pass that partitions a PR's +changed files into ordered, logically-connected groups, each with a short +headline and explanation. The pass is kicked off concurrently with the +reviewer run (so it adds ~0 latency) and its result is stored on the reviewer +thread metadata under ``diff_groups`` for the review UI to render as a +top-to-bottom walkthrough of the PR. + +Everything here is best-effort: any failure logs and degrades to "no groups", +and the UI falls back to the folder/flat view. The grouping never blocks or +breaks the review itself. +""" + +from __future__ import annotations + +import hashlib +import logging +from datetime import UTC, datetime +from typing import TypedDict + +from langchain_core.language_models.chat_models import BaseChatModel +from pydantic import BaseModel, Field + +from .reviewer_diff import parse_unified_diff +from .reviewer_findings import get_thread_metadata, set_reviewer_thread_metadata + +logger = logging.getLogger(__name__) + +# Bound the prompt so a huge PR can't blow up the cost/latency of the grouping +# pass. The full changed-file list is always included (so every file can be +# assigned); only the per-file hunk previews are budgeted. +MAX_PROMPT_CHARS = 48_000 +MAX_FILE_HUNK_CHARS = 4_000 +MAX_GROUPS = 8 + + +class DiffGroup(TypedDict): + """One logical group of changed files for the AI sorted view.""" + + title: str + summary: str + files: list[str] + + +class _DiffGroupModel(BaseModel): + title: str = Field( + description=( + "Short headline naming the logical change, roughly 4-10 words, no " + "trailing punctuation. Wrap code identifiers in `backticks`." + ) + ) + summary: str = Field( + description=( + "GitHub-flavored markdown walkthrough of this group: a few short " + "sentences or bullets, code identifiers in `backticks`, optionally one " + "short fenced code block of the key snippet, and concrete locations " + "linked as [path:start-end](#loc=path:start-end)." + ) + ) + files: list[str] = Field( + description="Changed file paths in this group, copied verbatim, in reading order." + ) + + +class _DiffGroupingResult(BaseModel): + groups: list[_DiffGroupModel] = Field( + description="Ordered groups forming a top-to-bottom walkthrough of the PR." + ) + + +_PROMPT_TEMPLATE = """You are organizing a GitHub pull request's changed files into a clear, \ +top-to-bottom walkthrough for a human reviewer. + +Group the changed files by logical intent — put files that implement one \ +coherent change together (e.g. "the new feature", "the supporting refactor", \ +"the config wiring", "the tests"). Order the groups so a reviewer can read \ +them top to bottom and build a mental model of the PR. + +Rules: +- Assign every changed file to exactly one group. +- Use at most {max_groups} groups. Prefer fewer, larger groups over many tiny ones. +- title: a short headline naming the change, roughly 4-10 words, no trailing \ +punctuation. Wrap code identifiers (symbols, flags, file names) in `backticks`. +- summary: GitHub-flavored markdown explaining what the group changes and why, \ +written as a short walkthrough a reviewer can skim: + - Keep it focused — a few short sentences or bullets. Do not restate the diff line by line. + - Wrap every code identifier, symbol, type, flag, and path in `backticks`. + - When one change is central, include at most ONE short fenced code block \ +(```lang, <= ~8 lines) of the key snippet — not the whole hunk. + - Reference concrete locations as markdown links of the EXACT form \ +[path:start-end](#loc=path:start-end), where path is the verbatim changed-file \ +path and start/end are line numbers from the "lines X-Y" annotations below \ +(use [path:line](#loc=path:line) when start == end). Prefer these links over \ +describing locations in prose — they let the reader jump straight to the hunk. +- files: the exact file paths (copied verbatim from the list below) in this \ +group, in the order a reviewer should read them. + +Changed files: +{file_list} + +Diffs (each hunk is annotated with its line range in the new file): +{diffs} +""" + + +def diff_signature(diff_text: str) -> str: + """Stable content hash of the diff, used to skip regeneration when unchanged.""" + return hashlib.sha256(diff_text.encode("utf-8", "ignore")).hexdigest() + + +def _changed_files(diff_text: str) -> list[str]: + return [fd.file for fd in parse_unified_diff(diff_text)] + + +def _build_prompt(diff_text: str, files: list[str]) -> str: + parts: list[str] = [] + budget = MAX_PROMPT_CHARS + for file_diff in parse_unified_diff(diff_text): + segments: list[str] = [] + for hunk in file_diff.hunks: + body = hunk.body + if len(body) > MAX_FILE_HUNK_CHARS: + body = body[:MAX_FILE_HUNK_CHARS] + "\n... (truncated)" + # Surface the new-file line range so the model can cite accurate + # locations in its summary links. + segments.append(f"lines {hunk.new_start}-{hunk.new_end}:\n```diff\n{body}\n```") + block = f"### {file_diff.file}\n" + "\n".join(segments) + "\n" + if budget - len(block) < 0: + parts.append(f"### {file_diff.file}\n(diff omitted — prompt budget reached)\n") + continue + budget -= len(block) + parts.append(block) + file_list = "\n".join(f"- {path}" for path in files) + return _PROMPT_TEMPLATE.format( + max_groups=MAX_GROUPS, + file_list=file_list, + diffs="\n".join(parts), + ) + + +def _normalize_groups(result: _DiffGroupingResult, files: list[str]) -> list[DiffGroup]: + """Validate the model output into a clean partition. + + Each file is assigned at most once (first group wins), unknown paths are + dropped, empty/untitled groups are dropped, and the group count is capped. + Files the model failed to assign are intentionally left out here — the UI + collects them into a trailing "Other changes" group, which also keeps the + view robust against stale group data. + """ + valid = set(files) + seen: set[str] = set() + groups: list[DiffGroup] = [] + for group in result.groups: + title = (group.title or "").strip() + summary = (group.summary or "").strip() + picked: list[str] = [] + for path in group.files: + if path in valid and path not in seen: + seen.add(path) + picked.append(path) + if not title or not picked: + continue + groups.append({"title": title, "summary": summary, "files": picked}) + if len(groups) >= MAX_GROUPS: + break + return groups + + +async def generate_diff_groups( + *, + diff_text: str, + model: BaseChatModel, +) -> list[DiffGroup] | None: + """Run the single structured-output grouping pass over a unified diff. + + Returns the normalized groups, ``[]`` when there are no changed files, or + ``None`` when the LLM call fails (caller treats ``None`` as "leave existing + groups untouched"). + """ + files = _changed_files(diff_text) + if not files: + return [] + prompt = _build_prompt(diff_text, files) + try: + structured = model.with_structured_output(_DiffGroupingResult) + result = await structured.ainvoke(prompt) + except Exception: # noqa: BLE001 — grouping is best-effort, never break the review + logger.exception("Diff grouping LLM call failed") + return None + if not isinstance(result, _DiffGroupingResult): + logger.warning("Diff grouping returned unexpected type: %s", type(result)) + return None + return _normalize_groups(result, files) + + +async def maybe_generate_and_store_diff_groups( + *, + thread_id: str, + head_sha: str, + diff_text: str, + model: BaseChatModel, +) -> None: + """Generate diff groups and persist them on the reviewer thread metadata. + + Skips regeneration when the persisted signature already matches the current + diff (the cheap no-op path for re-reviews with no file changes). All errors + are logged and swallowed so this can run as a fire-and-forget background + task without ever affecting the review. + """ + try: + if not thread_id or not diff_text: + return + signature = diff_signature(diff_text) + metadata = await get_thread_metadata(thread_id) + existing = metadata.get("diff_groups") if isinstance(metadata, dict) else None + if ( + isinstance(existing, dict) + and existing.get("signature") == signature + and isinstance(existing.get("groups"), list) + and existing.get("groups") + ): + return + groups = await generate_diff_groups(diff_text=diff_text, model=model) + if groups is None: + return + payload = { + "head_sha": head_sha, + "signature": signature, + "generated_at": datetime.now(UTC).isoformat(), + "groups": groups, + } + await set_reviewer_thread_metadata(thread_id, extra={"diff_groups": payload}) + logger.info( + "Stored %d diff group(s) for reviewer thread %s (head %s)", + len(groups), + thread_id, + head_sha[:7] if head_sha else "?", + ) + except Exception: # noqa: BLE001 — best-effort; never break the review + logger.exception("Failed to generate/store diff groups for thread %s", thread_id) diff --git a/tests/test_review_api.py b/tests/test_review_api.py index 47398825..dd917188 100644 --- a/tests/test_review_api.py +++ b/tests/test_review_api.py @@ -1,5 +1,6 @@ from agent.dashboard.review_api import ( _finding_counts, + _serialize_diff_groups, _serialize_finding, _thread_review_summary, classify_finding, @@ -71,3 +72,40 @@ def test_thread_review_summary_requires_pr_meta(): def test_reviewer_thread_id_matches_webapp(): assert reviewer_thread_id("acme", "repo", 7) == generate_reviewer_thread_id("acme", "repo", 7) + + +def test_serialize_diff_groups_assigns_index_and_drops_invalid(): + metadata = { + "diff_groups": { + "head_sha": "abc", + "groups": [ + {"title": "Feature", "summary": "Adds it", "files": ["a.py", "b.py"]}, + {"title": " ", "summary": "x", "files": ["c.py"]}, + {"title": "Empty", "summary": "", "files": []}, + {"title": "Tests", "summary": "Covers it", "files": ["t.py", 5]}, + ], + } + } + groups, stale = _serialize_diff_groups(metadata, "abc") + assert stale is False + assert groups == [ + {"index": 1, "title": "Feature", "summary": "Adds it", "files": ["a.py", "b.py"]}, + {"index": 2, "title": "Tests", "summary": "Covers it", "files": ["t.py"]}, + ] + + +def test_serialize_diff_groups_marks_stale_on_head_mismatch(): + metadata = { + "diff_groups": { + "head_sha": "old", + "groups": [{"title": "T", "summary": "", "files": ["a.py"]}], + } + } + groups, stale = _serialize_diff_groups(metadata, "new") + assert stale is True + assert groups[0]["index"] == 1 + + +def test_serialize_diff_groups_handles_missing(): + assert _serialize_diff_groups({}, "abc") == ([], False) + assert _serialize_diff_groups({"diff_groups": {"groups": "nope"}}, "abc") == ([], False) diff --git a/tests/test_reviewer_groups.py b/tests/test_reviewer_groups.py new file mode 100644 index 00000000..9051b645 --- /dev/null +++ b/tests/test_reviewer_groups.py @@ -0,0 +1,172 @@ +from __future__ import annotations + +from typing import Any +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from agent.reviewer_groups import ( + _build_prompt, + _DiffGroupingResult, + _DiffGroupModel, + diff_signature, + generate_diff_groups, + maybe_generate_and_store_diff_groups, +) + +_DIFF = """diff --git a/foo.py b/foo.py +--- a/foo.py ++++ b/foo.py +@@ -1,2 +1,3 @@ + a ++b + c +diff --git a/bar.py b/bar.py +--- a/bar.py ++++ b/bar.py +@@ -1 +1,2 @@ + x ++y +""" + + +class _FakeStructured: + def __init__(self, result: Any, *, raises: bool = False) -> None: + self._result = result + self._raises = raises + + async def ainvoke(self, _prompt: str) -> Any: + if self._raises: + raise RuntimeError("boom") + return self._result + + +class _FakeModel: + def __init__(self, result: Any, *, raises: bool = False) -> None: + self._structured = _FakeStructured(result, raises=raises) + + def with_structured_output(self, _schema: Any) -> _FakeStructured: + return self._structured + + +@pytest.mark.asyncio +async def test_generate_diff_groups_partitions() -> None: + result = _DiffGroupingResult( + groups=[ + _DiffGroupModel(title="Foo change", summary="Edits foo", files=["foo.py"]), + _DiffGroupModel(title="Bar change", summary="Edits bar", files=["bar.py"]), + ] + ) + groups = await generate_diff_groups(diff_text=_DIFF, model=_FakeModel(result)) + assert groups == [ + {"title": "Foo change", "summary": "Edits foo", "files": ["foo.py"]}, + {"title": "Bar change", "summary": "Edits bar", "files": ["bar.py"]}, + ] + + +@pytest.mark.asyncio +async def test_generate_diff_groups_dedupes_and_drops_unknown() -> None: + result = _DiffGroupingResult( + groups=[ + _DiffGroupModel( + title="First", + summary="", + files=["foo.py", "foo.py", "does-not-exist.py"], + ), + _DiffGroupModel(title="Second", summary="", files=["foo.py", "bar.py"]), + _DiffGroupModel(title="", summary="ignored", files=["bar.py"]), + ] + ) + groups = await generate_diff_groups(diff_text=_DIFF, model=_FakeModel(result)) + # foo.py only in the first group, bar.py only in the second; the untitled + # group and the unknown path are dropped. + assert groups == [ + {"title": "First", "summary": "", "files": ["foo.py"]}, + {"title": "Second", "summary": "", "files": ["bar.py"]}, + ] + + +@pytest.mark.asyncio +async def test_generate_diff_groups_empty_diff_returns_empty() -> None: + assert await generate_diff_groups(diff_text="", model=_FakeModel(None)) == [] + + +@pytest.mark.asyncio +async def test_generate_diff_groups_llm_failure_returns_none() -> None: + groups = await generate_diff_groups(diff_text=_DIFF, model=_FakeModel(None, raises=True)) + assert groups is None + + +def test_build_prompt_annotates_hunk_line_ranges() -> None: + prompt = _build_prompt(_DIFF, ["foo.py", "bar.py"]) + assert "### foo.py" in prompt + assert "### bar.py" in prompt + # New-file line ranges from the @@ headers are surfaced so the model can + # cite accurate locations: foo.py @@ -1,2 +1,3 @@ and bar.py @@ -1 +1,2 @@. + assert "lines 1-3:" in prompt + assert "lines 1-2:" in prompt + # The verbatim file list is included so every file can be assigned. + assert "- foo.py" in prompt + assert "- bar.py" in prompt + + +def test_diff_signature_is_stable_and_content_sensitive() -> None: + assert diff_signature(_DIFF) == diff_signature(_DIFF) + assert diff_signature(_DIFF) != diff_signature(_DIFF + "\n+extra") + + +@pytest.mark.asyncio +async def test_maybe_generate_skips_when_signature_unchanged() -> None: + existing = { + "diff_groups": { + "signature": diff_signature(_DIFF), + "groups": [{"title": "t", "summary": "", "files": ["foo.py"]}], + } + } + with ( + patch( + "agent.reviewer_groups.get_thread_metadata", + new_callable=AsyncMock, + return_value=existing, + ), + patch("agent.reviewer_groups.generate_diff_groups", new_callable=AsyncMock) as gen, + patch( + "agent.reviewer_groups.set_reviewer_thread_metadata", + new_callable=AsyncMock, + ) as set_meta, + ): + await maybe_generate_and_store_diff_groups( + thread_id="t1", head_sha="h", diff_text=_DIFF, model=MagicMock() + ) + gen.assert_not_awaited() + set_meta.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_maybe_generate_stores_when_changed() -> None: + groups = [{"title": "t", "summary": "s", "files": ["foo.py"]}] + with ( + patch( + "agent.reviewer_groups.get_thread_metadata", + new_callable=AsyncMock, + return_value={}, + ), + patch( + "agent.reviewer_groups.generate_diff_groups", + new_callable=AsyncMock, + return_value=groups, + ) as gen, + patch( + "agent.reviewer_groups.set_reviewer_thread_metadata", + new_callable=AsyncMock, + ) as set_meta, + ): + await maybe_generate_and_store_diff_groups( + thread_id="t1", head_sha="head123", diff_text=_DIFF, model=MagicMock() + ) + gen.assert_awaited_once() + set_meta.assert_awaited_once() + payload = set_meta.call_args.kwargs["extra"]["diff_groups"] + assert payload["head_sha"] == "head123" + assert payload["signature"] == diff_signature(_DIFF) + assert payload["groups"] == groups diff --git a/tests/test_team_settings_grouping.py b/tests/test_team_settings_grouping.py new file mode 100644 index 00000000..b8c72abd --- /dev/null +++ b/tests/test_team_settings_grouping.py @@ -0,0 +1,83 @@ +from __future__ import annotations + +from unittest.mock import AsyncMock, patch + +import pytest +from pydantic import ValidationError + +from agent.dashboard.team_settings import ( + TeamSettingsUpdate, + get_team_default_grouping_model, +) + +_REVIEWER_SUBAGENT_PAIR = ("openai:gpt-5.5", "low") +_GROUPING_PAIR = ("google_genai:gemini-3.5-flash", "low") + + +def _settings(**overrides: object) -> dict[str, object]: + base: dict[str, object] = { + "default_reviewer_subagent_model": _REVIEWER_SUBAGENT_PAIR[0], + "default_reviewer_subagent_reasoning_effort": _REVIEWER_SUBAGENT_PAIR[1], + "default_grouping_model": None, + "default_grouping_reasoning_effort": None, + } + base.update(overrides) + return base + + +@pytest.mark.asyncio +async def test_grouping_inherits_reviewer_subagent_when_unset() -> None: + with patch( + "agent.dashboard.team_settings.get_team_settings", + new_callable=AsyncMock, + return_value=_settings(), + ): + assert await get_team_default_grouping_model() == _REVIEWER_SUBAGENT_PAIR + + +@pytest.mark.asyncio +async def test_grouping_uses_configured_model_when_set() -> None: + with patch( + "agent.dashboard.team_settings.get_team_settings", + new_callable=AsyncMock, + return_value=_settings( + default_grouping_model=_GROUPING_PAIR[0], + default_grouping_reasoning_effort=_GROUPING_PAIR[1], + ), + ): + assert await get_team_default_grouping_model() == _GROUPING_PAIR + + +@pytest.mark.asyncio +async def test_grouping_inherits_when_configured_model_invalid() -> None: + with patch( + "agent.dashboard.team_settings.get_team_settings", + new_callable=AsyncMock, + return_value=_settings( + default_grouping_model="bogus:model", + default_grouping_reasoning_effort="high", + ), + ): + assert await get_team_default_grouping_model() == _REVIEWER_SUBAGENT_PAIR + + +def test_team_settings_update_accepts_grouping_pair() -> None: + update = TeamSettingsUpdate( + default_grouping_model=_GROUPING_PAIR[0], + default_grouping_reasoning_effort=_GROUPING_PAIR[1], + ) + assert update.default_grouping_model == _GROUPING_PAIR[0] + assert update.default_grouping_reasoning_effort == _GROUPING_PAIR[1] + + +def test_team_settings_update_rejects_grouping_effort_without_model() -> None: + with pytest.raises(ValidationError): + TeamSettingsUpdate(default_grouping_reasoning_effort="high") + + +def test_team_settings_update_rejects_unsupported_grouping_effort() -> None: + with pytest.raises(ValidationError): + TeamSettingsUpdate( + default_grouping_model=_GROUPING_PAIR[0], + default_grouping_reasoning_effort="max", + ) diff --git a/ui/src/components/agents/AgentsSidebar.tsx b/ui/src/components/agents/AgentsSidebar.tsx index 2138d96d..21aa11aa 100644 --- a/ui/src/components/agents/AgentsSidebar.tsx +++ b/ui/src/components/agents/AgentsSidebar.tsx @@ -27,7 +27,7 @@ import type { SessionUser } from "@/lib/api" import type { AgentSource, AgentThread } from "@/lib/agents/types" import { SidebarUserMenu } from "@/components/SidebarUserMenu" import { - ReviewFileTree, + ReviewSidebarPanel, ReviewSidebarProvider, useReviewSidebarData, } from "@/components/agents/ReviewSidebar" @@ -159,7 +159,7 @@ export function AgentsSidebar({ user, activeThreadId }: AgentsSidebarProps) { {reviewSidebar ? ( - + ) : (
+} + export interface ReviewSidebarData { title: string files: Array | null selected: string | null viewed: Set onSelect: (path: string) => void + groups: Array | null + view: ReviewSidebarView + onViewChange: (view: ReviewSidebarView) => void + onSelectGroup: (index: number) => void + onLocationClick?: (file: string, startLine: number, endLine: number) => void } const ReviewSidebarContext = createContext<{ @@ -61,13 +86,28 @@ export function useRegisterReviewSidebar(data: ReviewSidebarData) { }, [setData, data]) } -export function ReviewFileTree({ data }: { data: ReviewSidebarData }) { +export function ReviewSidebarPanel({ data }: { data: ReviewSidebarData }) { + const hasGroups = data.groups !== null && data.groups.length > 0 + const showAi = data.view === "ai" && hasGroups + return (
-
- {data.title} +
+ + {data.title} + + {hasGroups && ( + + )}
- {!data.files ? ( + {showAi ? ( + + ) : !data.files ? (
@@ -82,6 +122,209 @@ export function ReviewFileTree({ data }: { data: ReviewSidebarData }) { ) } +function ReviewViewToggle({ + view, + onChange, +}: { + view: ReviewSidebarView + onChange: (view: ReviewSidebarView) => void +}) { + return ( +
+ onChange("ai")} + > + + + onChange("files")} + > + + +
+ ) +} + +function ReviewViewToggleButton({ + active, + label, + onClick, + children, +}: { + active: boolean + label: string + onClick: () => void + children: React.ReactNode +}) { + return ( + + ) +} + +function ReviewGroupList({ + groups, + onSelectGroup, + onSelectFile, + onLocationClick, +}: { + groups: Array + onSelectGroup: (index: number) => void + onSelectFile: (path: string) => void + onLocationClick?: (file: string, startLine: number, endLine: number) => void +}) { + return ( +
+ {groups.map((group) => ( + onSelectGroup(group.index)} + onSelectFile={onSelectFile} + onLocationClick={onLocationClick} + /> + ))} +
+ ) +} + +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) } +} + +// 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 { + return text.split(/(`[^`]+`)/g).map((part, i) => { + if (part.length >= 2 && part.startsWith("`") && part.endsWith("`")) { + return ( + + {part.slice(1, -1)} + + ) + } + return {part} + }) +} + +function ReviewGroupRow({ + group, + onSelect, + onSelectFile, + onLocationClick, +}: { + group: ReviewSidebarGroup + onSelect: () => void + onSelectFile: (path: string) => void + onLocationClick?: (file: string, startLine: number, endLine: number) => void +}) { + const [expanded, setExpanded] = useState(false) + return ( +
+ + + {group.files.length > 0 && ( +
+ {group.files.map((path) => { + const { dir, base } = splitPath(path) + return ( + + ) + })} +
+ )} + + {group.summary && ( +
+ + {expanded && ( +
+ +
+ )} +
+ )} +
+ ) +} + function ReviewFileTreeExplorer({ files, selected, diff --git a/ui/src/components/agents/ported/Markdown.tsx b/ui/src/components/agents/ported/Markdown.tsx index 38dff27b..279bb412 100644 --- a/ui/src/components/agents/ported/Markdown.tsx +++ b/ui/src/components/agents/ported/Markdown.tsx @@ -1,5 +1,6 @@ -import { memo } from "react"; +import { memo, useMemo } from "react"; import { Streamdown } from "streamdown"; +import { parseLocationHref } from "./markdownLocation"; import type { ReactNode } from "react"; import "streamdown/styles.css"; @@ -7,6 +8,12 @@ interface MarkdownProps { content: string; /** When true, keep Streamdown in streaming mode for the duration of the run. */ isLive?: boolean; + /** + * When set, `#loc=path:start-end` links render as in-page buttons that invoke + * this callback instead of navigating. Used by the review view to jump the + * diff to a referenced hunk. + */ + onLocationClick?: (file: string, startLine: number, endLine: number) => void; } /** @@ -44,16 +51,6 @@ const STREAMDOWN_COMPONENTS = { {children}

), - a: ({ href, children }: { href?: string; children?: ReactNode }) => ( - - {children} - - ), ul: ({ children }: { children?: ReactNode }) => (
{children}
), @@ -86,7 +83,42 @@ const STREAMDOWN_COMPONENTS = { const SHIKI_THEME: ["github-light", "github-dark"] = ["github-light", "github-dark"]; -export const Markdown = memo(function Markdown({ content, isLive = false }: MarkdownProps) { +export const Markdown = memo(function Markdown({ + content, + isLive = false, + onLocationClick, +}: MarkdownProps) { + const components = useMemo( + () => ({ + ...STREAMDOWN_COMPONENTS, + a: ({ href, children }: { href?: string; children?: ReactNode }) => { + const loc = onLocationClick ? parseLocationHref(href) : null; + if (loc && onLocationClick) { + return ( + + ); + } + return ( + + {children} + + ); + }, + }), + [onLocationClick] + ); + return (
{content} diff --git a/ui/src/components/agents/ported/markdownLocation.test.ts b/ui/src/components/agents/ported/markdownLocation.test.ts new file mode 100644 index 00000000..0e2aa28c --- /dev/null +++ b/ui/src/components/agents/ported/markdownLocation.test.ts @@ -0,0 +1,53 @@ +import { describe, expect, it } from "vitest"; + +import { parseLocationHref } from "./markdownLocation"; + +describe("parseLocationHref", () => { + it("parses a path with a start-end range", () => { + expect(parseLocationHref("#loc=src/foo.rs:231-232")).toEqual({ + file: "src/foo.rs", + startLine: 231, + endLine: 232, + }); + }); + + it("parses a single-line ref (no end)", () => { + expect(parseLocationHref("#loc=mod.rs:42")).toEqual({ + file: "mod.rs", + startLine: 42, + endLine: 42, + }); + }); + + it("tolerates an origin prefix the URL transform may prepend", () => { + expect(parseLocationHref("http://localhost/#loc=a/b.ts:5-9")).toEqual({ + file: "a/b.ts", + startLine: 5, + endLine: 9, + }); + }); + + it("decodes percent-encoded hrefs", () => { + expect(parseLocationHref("#loc=a%20b/c.ts:1-2")).toEqual({ + file: "a b/c.ts", + startLine: 1, + endLine: 2, + }); + }); + + it("clamps an end below the start to a single line", () => { + expect(parseLocationHref("#loc=x.ts:10-3")).toEqual({ + file: "x.ts", + startLine: 10, + endLine: 10, + }); + }); + + it("returns null for non-location and malformed hrefs", () => { + expect(parseLocationHref(undefined)).toBeNull(); + expect(parseLocationHref("https://example.com")).toBeNull(); + expect(parseLocationHref("#loc=noline")).toBeNull(); + expect(parseLocationHref("#loc=:5")).toBeNull(); + expect(parseLocationHref("#loc=x.ts:abc")).toBeNull(); + }); +}); diff --git a/ui/src/components/agents/ported/markdownLocation.ts b/ui/src/components/agents/ported/markdownLocation.ts new file mode 100644 index 00000000..30e85d24 --- /dev/null +++ b/ui/src/components/agents/ported/markdownLocation.ts @@ -0,0 +1,35 @@ +export interface MarkdownLocation { + file: string; + startLine: number; + endLine: number; +} + +/** + * Parse a `#loc=:[-]` sentinel href emitted by the reviewer + * diff-grouping pass into a file + line range. Tolerant of an origin prefix the + * markdown URL transform may prepend, and of percent-encoding. Returns null for + * any href that isn't a location link. + */ +export function parseLocationHref(href?: string): MarkdownLocation | null { + if (!href) return null; + const marker = href.indexOf("#loc="); + if (marker === -1) return null; + let raw = href.slice(marker + "#loc=".length); + try { + raw = decodeURIComponent(raw); + } catch { + // Keep raw as-is if it isn't valid percent-encoding. + } + const colon = raw.lastIndexOf(":"); + if (colon <= 0) return null; + const file = raw.slice(0, colon); + const [startStr, endStr] = raw.slice(colon + 1).split("-"); + const start = Number.parseInt(startStr ?? "", 10); + if (!Number.isFinite(start) || start <= 0) return null; + const end = endStr !== undefined ? Number.parseInt(endStr, 10) : start; + return { + file, + startLine: start, + endLine: Number.isFinite(end) && end >= start ? end : start, + }; +} diff --git a/ui/src/lib/api.ts b/ui/src/lib/api.ts index d806b79e..da19019c 100644 --- a/ui/src/lib/api.ts +++ b/ui/src/lib/api.ts @@ -5,23 +5,29 @@ * cookie set by the OAuth callback rides along on cross-origin calls. */ -const API_BASE = (import.meta.env.VITE_DASHBOARD_API_BASE_URL ?? "").replace(/\/$/, ""); +const API_BASE = (import.meta.env.VITE_DASHBOARD_API_BASE_URL ?? "").replace( + /\/$/, + "" +) if (!API_BASE && typeof window !== "undefined") { - console.warn("VITE_DASHBOARD_API_BASE_URL is not set"); + console.warn("VITE_DASHBOARD_API_BASE_URL is not set") } export class ApiError extends Error { - constructor(public readonly status: number, message: string) { - super(message); - this.name = "ApiError"; + constructor( + public readonly status: number, + message: string + ) { + super(message) + this.name = "ApiError" } } export function isGithubReauthError(error: unknown): boolean { - if (!(error instanceof ApiError)) return false; - if (error.status === 401) return true; - return /github token|re-login required/i.test(error.message); + if (!(error instanceof ApiError)) return false + if (error.status === 401) return true + return /github token|re-login required/i.test(error.message) } async function request(path: string, init: RequestInit = {}): Promise { @@ -32,364 +38,379 @@ async function request(path: string, init: RequestInit = {}): Promise { "Content-Type": "application/json", ...(init.headers ?? {}), }, - }); + }) if (!res.ok) { - let message = res.statusText; + let message = res.statusText try { - const body = await res.json(); - if (body?.detail) message = typeof body.detail === "string" ? body.detail : JSON.stringify(body.detail); + const body = await res.json() + if (body?.detail) + message = + typeof body.detail === "string" + ? body.detail + : JSON.stringify(body.detail) } catch { /* ignore */ } - throw new ApiError(res.status, message); + throw new ApiError(res.status, message) } - if (res.status === 204) return undefined as T; - return (await res.json()) as T; + if (res.status === 204) return undefined as T + return (await res.json()) as T } export interface SessionUser { - login: string; - email: string | null; - avatar_url: string | null; - is_admin: boolean; - slack_oauth_enabled?: boolean; + login: string + email: string | null + avatar_url: string | null + is_admin: boolean + slack_oauth_enabled?: boolean } export interface ModelOption { - id: string; - label: string; - efforts: Array; - default_effort: string; - supports_images: boolean; + id: string + label: string + efforts: Array + default_effort: string + supports_images: boolean } export interface OptionsPayload { - models: Array; - default_agent_model: string; - default_agent_reasoning_effort: string; - default_agent_subagent_model: string; - default_agent_subagent_reasoning_effort: string; + models: Array + default_agent_model: string + default_agent_reasoning_effort: string + default_agent_subagent_model: string + default_agent_subagent_reasoning_effort: string } export interface Profile { - login?: string; - email?: string; - default_model?: string; - reasoning_effort?: string; - default_subagent_model?: string | null; - subagent_reasoning_effort?: string | null; - default_repo?: string | null; - base_branch?: string | null; - branch_prefix?: string | null; - auto_fix_ci?: boolean; - create_prs?: boolean; - review_draft_prs?: boolean | null; - updated_at?: string; + login?: string + email?: string + default_model?: string + reasoning_effort?: string + default_subagent_model?: string | null + subagent_reasoning_effort?: string | null + default_repo?: string | null + base_branch?: string | null + branch_prefix?: string | null + auto_fix_ci?: boolean + create_prs?: boolean + review_draft_prs?: boolean | null + updated_at?: string } export interface ProfileUpdate { - default_model: string; - reasoning_effort: string; - default_subagent_model?: string | null; - subagent_reasoning_effort?: string | null; - default_repo?: string | null; - base_branch?: string | null; - branch_prefix?: string | null; - auto_fix_ci?: boolean; - create_prs?: boolean; - review_draft_prs?: boolean | null; + default_model: string + reasoning_effort: string + default_subagent_model?: string | null + subagent_reasoning_effort?: string | null + default_repo?: string | null + base_branch?: string | null + branch_prefix?: string | null + auto_fix_ci?: boolean + create_prs?: boolean + review_draft_prs?: boolean | null } -export type TriggerMode = "every_push" | "once_per_pr" | "manual"; -export type AutofixMode = "off" | "low" | "medium" | "high"; +export type TriggerMode = "every_push" | "once_per_pr" | "manual" +export type AutofixMode = "off" | "low" | "medium" | "high" export interface TeamSettings { - trigger_mode: TriggerMode; - review_draft_prs: boolean; - pr_summaries: boolean; - review_trace_links: boolean; - autofix_mode: AutofixMode; - autofix_severity_threshold: AutofixMode; - org_guidelines?: string | null; - default_agent_model?: string | null; - default_agent_reasoning_effort?: string | null; - default_agent_subagent_model?: string | null; - default_agent_subagent_reasoning_effort?: string | null; - default_repo?: string | null; - default_reviewer_model?: string | null; - default_reviewer_reasoning_effort?: string | null; - default_reviewer_subagent_model?: string | null; - default_reviewer_subagent_reasoning_effort?: string | null; - default_chat_model?: string | null; - default_chat_reasoning_effort?: string | null; - updated_at?: string | null; + trigger_mode: TriggerMode + review_draft_prs: boolean + pr_summaries: boolean + review_trace_links: boolean + autofix_mode: AutofixMode + autofix_severity_threshold: AutofixMode + org_guidelines?: string | null + default_agent_model?: string | null + default_agent_reasoning_effort?: string | null + default_agent_subagent_model?: string | null + default_agent_subagent_reasoning_effort?: string | null + default_repo?: string | null + default_reviewer_model?: string | null + default_reviewer_reasoning_effort?: string | null + default_reviewer_subagent_model?: string | null + default_reviewer_subagent_reasoning_effort?: string | null + default_grouping_model?: string | null + default_grouping_reasoning_effort?: string | null + default_chat_model?: string | null + default_chat_reasoning_effort?: string | null + updated_at?: string | null } export interface ProviderCredentialStatus { - connected: boolean; - site?: string; - endpoint?: string; - api_key_last4?: string; - updated_at?: string | null; + connected: boolean + site?: string + endpoint?: string + api_key_last4?: string + updated_at?: string | null } export interface TeamCredentialsStatus { - datadog: ProviderCredentialStatus; - langsmith: ProviderCredentialStatus; + datadog: ProviderCredentialStatus + langsmith: ProviderCredentialStatus } export interface DatadogConnectBody { - site: string; - api_key: string; - app_key: string; + site: string + api_key: string + app_key: string } export interface LangSmithConnectBody { - api_key: string; - endpoint?: string | null; + api_key: string + endpoint?: string | null } export interface UserMapping { - github_login: string; - work_email: string; - slack_user_id?: string | null; - source?: string; - status?: string; - created_at?: string; - updated_at?: string; + github_login: string + work_email: string + slack_user_id?: string | null + source?: string + status?: string + created_at?: string + updated_at?: string } export interface UserMappingsPage { - items: Array; - total: number; - page: number; - page_size: number; + items: Array + total: number + page: number + page_size: number } -export type UsageLeaderboardPeriod = "7d" | "30d" | "all"; +export type UsageLeaderboardPeriod = "7d" | "30d" | "all" export interface UsageLeaderboardRow { - rank: number; + rank: number user: { - name: string; - github_login: string | null; - email: string | null; - }; - favorite_model: string; - agent_runs: number; - prs_opened: number; - merged_prs: number; - agent_loc: number; - additions: number; - deletions: number; + name: string + github_login: string | null + email: string | null + } + favorite_model: string + agent_runs: number + prs_opened: number + merged_prs: number + agent_loc: number + additions: number + deletions: number } export interface ReviewerStatsCounterRow { - name: string; - count: number; + name: string + count: number } export interface ReviewerStatsPayload { - period: UsageLeaderboardPeriod; - reviewed_prs: number; - prs_with_findings: number; - findings_recorded: number; - surfaced_findings: number; - addressed_findings: number; - resolved_after_update: number; - dismissed_findings: number; - unresolved_surfaced_findings: number; - resolution_rate: number; - human_replies: number; - severity_counts: Record; - top_categories: Array; - generated_at_ms: number | null; + period: UsageLeaderboardPeriod + reviewed_prs: number + prs_with_findings: number + findings_recorded: number + surfaced_findings: number + addressed_findings: number + resolved_after_update: number + dismissed_findings: number + unresolved_surfaced_findings: number + resolution_rate: number + human_replies: number + severity_counts: Record + top_categories: Array + generated_at_ms: number | null } export interface UsageLeaderboardPayload { - period: UsageLeaderboardPeriod; - rows: Array; - total_members: number; - current_user_rank: number | null; - generated_at_ms: number | null; - reviewer_stats: ReviewerStatsPayload; + period: UsageLeaderboardPeriod + rows: Array + total_members: number + current_user_rank: number | null + generated_at_ms: number | null + reviewer_stats: ReviewerStatsPayload } export interface Repository { - full_name: string; - private: boolean; + full_name: string + private: boolean } export interface Installation { - id: number; - account: string | null; - account_type: string | null; + id: number + account: string | null + account_type: string | null } export interface ReposPayload { - installations: Array; - repositories: Array; + installations: Array + repositories: Array } -export type ReviewStyleStatus = "idle" | "running" | "completed" | "failed"; +export type ReviewStyleStatus = "idle" | "running" | "completed" | "failed" export interface ReviewStyle { - full_name: string; - owner?: string; - name?: string; - status: ReviewStyleStatus; - custom_prompt: string | null; - analysis_summary: string | null; - top_reviewers: Array; - prs_sampled: number; - reviews_sampled: number; - analysis_thread_id: string | null; - analysis_run_id: string | null; - error: string | null; - created_by?: string; - created_at?: string; - updated_at?: string; + full_name: string + owner?: string + name?: string + status: ReviewStyleStatus + custom_prompt: string | null + analysis_summary: string | null + top_reviewers: Array + prs_sampled: number + reviews_sampled: number + analysis_thread_id: string | null + analysis_run_id: string | null + error: string | null + created_by?: string + created_at?: string + updated_at?: string } export interface AgentInstructions { - full_name: string; - owner?: string; - name?: string; - instructions: string; - created_by?: string; - created_at?: string; - updated_at?: string; + full_name: string + owner?: string + name?: string + instructions: string + created_by?: string + created_at?: string + updated_at?: string } -export type FindingSeverity = "low" | "medium" | "high" | "critical"; -export type FindingConfidence = "low" | "medium" | "high"; -export type FindingStatus = "open" | "resolved" | "dismissed"; -export type FindingGroup = "bug" | "investigate" | "informational"; +export type FindingSeverity = "low" | "medium" | "high" | "critical" +export type FindingConfidence = "low" | "medium" | "high" +export type FindingStatus = "open" | "resolved" | "dismissed" +export type FindingGroup = "bug" | "investigate" | "informational" export interface FindingInteraction { - kind: "human_reply" | "bot_reply"; - author?: string; - body?: string; - created_at?: string; + kind: "human_reply" | "bot_reply" + author?: string + body?: string + created_at?: string } export interface ReviewFinding { - id: string; - severity: FindingSeverity; - confidence: FindingConfidence; - category: string; - title: string; - description: string; - suggestion: string | null; - file: string; - start_line: number | null; - end_line: number | null; - side: "LEFT" | "RIGHT"; - in_diff: boolean; - status: FindingStatus; - outdated: boolean; - resolution_note: string | null; - diff_hunk: string | null; - github_thread_resolved: boolean; - github_review_comment_id: number | null; - interactions: Array; - group: FindingGroup; + id: string + severity: FindingSeverity + confidence: FindingConfidence + category: string + title: string + description: string + suggestion: string | null + file: string + start_line: number | null + end_line: number | null + side: "LEFT" | "RIGHT" + in_diff: boolean + status: FindingStatus + outdated: boolean + resolution_note: string | null + diff_hunk: string | null + github_thread_resolved: boolean + github_review_comment_id: number | null + interactions: Array + group: FindingGroup } export interface ReviewCounts { - open: number; - resolved: number; - dismissed: number; - bugs: number; - flags: number; + open: number + resolved: number + dismissed: number + bugs: number + flags: number } export interface ReviewSummary { - thread_id: string; - owner: string; - repo: string; - number: number; - title: string; - url: string; - head_ref: string; - base_ref: string; - author: string; - head_sha: string; - watch: boolean; - status: "running" | "error" | "idle"; - counts: ReviewCounts; - updated_at: string | null; - full_name?: string; + thread_id: string + owner: string + repo: string + number: number + title: string + url: string + head_ref: string + base_ref: string + author: string + head_sha: string + watch: boolean + status: "running" | "error" | "idle" + counts: ReviewCounts + updated_at: string | null + full_name?: string } export interface ReviewListPayload { - reviews: Array; - page: number; - has_more: boolean; + reviews: Array + page: number + has_more: boolean } export interface ReviewUserRef { - login: string; - avatar_url?: string | null; + login: string + avatar_url?: string | null } export interface ReviewCheckRun { - name: string; - status: string; - conclusion: string | null; - url: string | null; + name: string + status: string + conclusion: string | null + url: string | null } export interface ReviewPrDetails { - state: string; - title: string; - body: string; - additions: number; - deletions: number; - changed_files: number; - commits: number; - head_sha: string; - head_ref: string; - base_ref: string; - author: ReviewUserRef | null; - assignees: Array; - requested_reviewers: Array; - labels: Array<{ name: string; color: string | null }>; + state: string + title: string + body: string + additions: number + deletions: number + changed_files: number + commits: number + head_sha: string + head_ref: string + base_ref: string + author: ReviewUserRef | null + assignees: Array + requested_reviewers: Array + labels: Array<{ name: string; color: string | null }> +} + +export interface ReviewDiffGroup { + index: number + title: string + summary: string + files: Array } export interface ReviewDetail extends ReviewSummary { - pr: ReviewPrDetails; - checks: Array; - findings: Array; + pr: ReviewPrDetails + checks: Array + findings: Array + diff_groups: Array + diff_groups_stale: boolean } export interface ReviewDiffFile { - path: string; - previousPath: string | null; - status: "added" | "removed" | "modified" | "renamed"; - additions: number; - deletions: number; - originalContent: string; - modifiedContent: string; - unrenderable?: boolean; + path: string + previousPath: string | null + status: "added" | "removed" | "modified" | "renamed" + additions: number + deletions: number + originalContent: string + modifiedContent: string + unrenderable?: boolean } export interface ReviewDiffPayload { - files: Array; - total_additions: number; - total_deletions: number; - truncated: boolean; + files: Array + total_additions: number + total_deletions: number + truncated: boolean } export interface ReviewChatMeta { - available: boolean; - assistant_id: string; + available: boolean + assistant_id: string } export interface ReviewChatThread { - thread_id: string; - title: string; - updated_at?: string | null; + thread_id: string + title: string + updated_at?: string | null } /** @@ -400,53 +421,53 @@ export interface ReviewChatThread { export function reviewChatApiBase( owner: string, repo: string, - number: number, + number: number ): string { - const path = `${API_BASE}/dashboard/api/reviews/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/${number}/chat`; - if (/^https?:\/\//.test(path)) return path; + const path = `${API_BASE}/dashboard/api/reviews/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/${number}/chat` + if (/^https?:\/\//.test(path)) return path if (typeof window !== "undefined") { - return `${window.location.origin}${path.startsWith("/") ? "" : "/"}${path}`; + return `${window.location.origin}${path.startsWith("/") ? "" : "/"}${path}` } - return path; + return path } -export type ReviewerEvalScoreMode = "all_findings" | "surfaced_findings"; -export type ReviewerEvalSeverity = "low" | "medium" | "high" | "critical"; +export type ReviewerEvalScoreMode = "all_findings" | "surfaced_findings" +export type ReviewerEvalSeverity = "low" | "medium" | "high" | "critical" export interface ReviewerEvalConfig { - dataset_name: string; - experiment_prefix: string; - max_concurrency: number; - langsmith_project: string; - langgraph_url: string; - assistant_id: string; - model_id: string; - reasoning_effort: string; - score_mode: ReviewerEvalScoreMode; - severity_threshold: ReviewerEvalSeverity; - cap: number; + dataset_name: string + experiment_prefix: string + max_concurrency: number + langsmith_project: string + langgraph_url: string + assistant_id: string + model_id: string + reasoning_effort: string + score_mode: ReviewerEvalScoreMode + severity_threshold: ReviewerEvalSeverity + cap: number } export interface ReviewerEvalStartRequest extends ReviewerEvalConfig { - limit: number | null; + limit: number | null } export interface ReviewerEvalStatus { - name: string; - status: "idle" | "running" | "completed" | "failed"; - run_name?: string; - langsmith_project: string; - limit: number | null; - config_snapshot?: ReviewerEvalConfig; - started_at: string | null; - finished_at: string | null; - created_by: string | null; - pid: number | null; - exit_code: number | null; - experiment_url: string | null; - error: string | null; - log_tail: string | null; - updated_at: string; + name: string + status: "idle" | "running" | "completed" | "failed" + run_name?: string + langsmith_project: string + limit: number | null + config_snapshot?: ReviewerEvalConfig + started_at: string | null + finished_at: string | null + created_by: string | null + pid: number | null + exit_code: number | null + experiment_url: string | null + error: string | null + log_tail: string | null + updated_at: string } export const api = { @@ -470,37 +491,52 @@ export const api = { body: JSON.stringify({ custom_prompt }), }), analyzeReviewStyle: (full_name: string) => - request(`/review-styles/${encodeURIComponent(full_name)}/analyze`, { - method: "POST", - }), + request( + `/review-styles/${encodeURIComponent(full_name)}/analyze`, + { + method: "POST", + } + ), cancelReviewStyle: (full_name: string) => - request(`/review-styles/${encodeURIComponent(full_name)}/cancel`, { - method: "POST", - }), + request( + `/review-styles/${encodeURIComponent(full_name)}/cancel`, + { + method: "POST", + } + ), deleteReviewStyle: (full_name: string) => request(`/review-styles/${encodeURIComponent(full_name)}`, { method: "DELETE", }), - listAgentInstructions: () => request>("/agent-instructions"), + listAgentInstructions: () => + request>("/agent-instructions"), createAgentInstructions: (full_name: string) => request("/agent-instructions", { method: "POST", body: JSON.stringify({ full_name }), }), getAgentInstructions: (full_name: string) => - request(`/agent-instructions/${encodeURIComponent(full_name)}`), + request( + `/agent-instructions/${encodeURIComponent(full_name)}` + ), saveAgentInstructions: (full_name: string, instructions: string) => - request(`/agent-instructions/${encodeURIComponent(full_name)}`, { - method: "PUT", - body: JSON.stringify({ instructions }), - }), + request( + `/agent-instructions/${encodeURIComponent(full_name)}`, + { + method: "PUT", + body: JSON.stringify({ instructions }), + } + ), deleteAgentInstructions: (full_name: string) => request(`/agent-instructions/${encodeURIComponent(full_name)}`, { method: "DELETE", }), getTeamSettings: () => request("/team-settings"), saveTeamSettings: (body: TeamSettings) => - request("/team-settings", { method: "PUT", body: JSON.stringify(body) }), + request("/team-settings", { + method: "PUT", + body: JSON.stringify(body), + }), getTeamCredentials: () => request("/team-credentials"), connectDatadog: (body: DatadogConnectBody) => request("/team-credentials/datadog", { @@ -508,14 +544,18 @@ export const api = { body: JSON.stringify(body), }), disconnectDatadog: () => - request("/team-credentials/datadog", { method: "DELETE" }), + request("/team-credentials/datadog", { + method: "DELETE", + }), connectLangSmith: (body: LangSmithConnectBody) => request("/team-credentials/langsmith", { method: "PUT", body: JSON.stringify(body), }), disconnectLangSmith: () => - request("/team-credentials/langsmith", { method: "DELETE" }), + request("/team-credentials/langsmith", { + method: "DELETE", + }), listEnabledReviewRepos: () => request<{ repos: Array }>("/enabled-review-repos"), setEnabledReviewRepo: (full_name: string, enabled: boolean) => @@ -525,58 +565,57 @@ export const api = { }), usageLeaderboard: (period: UsageLeaderboardPeriod = "30d", limit = 10) => request( - `/agent-usage-leaderboard?period=${encodeURIComponent(period)}&limit=${limit}`, + `/agent-usage-leaderboard?period=${encodeURIComponent(period)}&limit=${limit}` ), myMapping: () => request>("/my-mapping"), adminListUserMappings: (page = 1, pageSize = 20) => request( - `/admin/user-mappings?page=${page}&page_size=${pageSize}`, + `/admin/user-mappings?page=${page}&page_size=${pageSize}` ), adminDeleteUserMapping: (github_login: string) => request<{ deleted: boolean }>( `/admin/user-mappings/${encodeURIComponent(github_login)}`, - { method: "DELETE" }, + { method: "DELETE" } ), 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}`, + `/reviews/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/${number}` ), getReviewDiff: (owner: string, repo: string, number: number) => request( - `/reviews/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/${number}/diff`, + `/reviews/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/${number}/diff` ), getReviewChat: (owner: string, repo: string, number: number) => request( - `/reviews/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/${number}/chat`, + `/reviews/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/${number}/chat` ), listReviewChatThreads: (owner: string, repo: string, number: number) => request<{ threads: Array }>( - `/reviews/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/${number}/chat/threads`, + `/reviews/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/${number}/chat/threads` ), deleteReviewChatThread: ( owner: string, repo: string, number: number, - threadId: string, + threadId: string ) => request( `/reviews/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/${number}/chat/threads/${encodeURIComponent(threadId)}`, - { method: "DELETE" }, + { method: "DELETE" } ), reReview: (owner: string, repo: string, number: number) => request<{ - success: boolean; - queued: boolean; - thread_id: string; - pr_url: string; + success: boolean + queued: boolean + thread_id: string + pr_url: string }>( `/reviews/${encodeURIComponent(owner)}/${encodeURIComponent(repo)}/${number}/re-review`, - { method: "POST" }, + { method: "POST" } ), - getReviewerEval: () => - request("/admin/evals/reviewer"), + getReviewerEval: () => request("/admin/evals/reviewer"), startReviewerEval: (body: ReviewerEvalStartRequest) => request("/admin/evals/reviewer", { method: "POST", @@ -585,14 +624,15 @@ export const api = { cancelReviewerEval: () => request("/admin/evals/reviewer", { method: "DELETE" }), logout: () => request("/auth/logout", { method: "POST" }), -}; +} export function loginUrl(redirectTo?: string): string { - const target = redirectTo ?? (typeof window !== "undefined" ? window.location.origin : ""); - const qs = target ? `?redirect_to=${encodeURIComponent(target)}` : ""; - return `${API_BASE}/dashboard/api/auth/login${qs}`; + const target = + redirectTo ?? (typeof window !== "undefined" ? window.location.origin : "") + const qs = target ? `?redirect_to=${encodeURIComponent(target)}` : "" + return `${API_BASE}/dashboard/api/auth/login${qs}` } export function slackConnectUrl(): string { - return `${API_BASE}/dashboard/api/slack/login`; + return `${API_BASE}/dashboard/api/slack/login` } diff --git a/ui/src/routes/admin.tsx b/ui/src/routes/admin.tsx index 96eff351..90cda443 100644 --- a/ui/src/routes/admin.tsx +++ b/ui/src/routes/admin.tsx @@ -61,9 +61,12 @@ function AdminPage() { className="flex items-center justify-between gap-6 px-4 py-3 hover:bg-muted/40" >
- Reviewer eval + + Reviewer eval + - Run the offline reviewer benchmark and watch its output stream live. + Run the offline reviewer benchmark and watch its output stream + live.
@@ -559,6 +562,31 @@ function GlobalDefaultsSection({ models }: { models: Array }) { } disabled={!settings.data || save.isPending} /> + + settings.data && + save.mutate({ + ...settings.data, + default_grouping_model: null, + default_grouping_reasoning_effort: null, + }) + } + onChange={(model, effort) => + settings.data && + save.mutate({ + ...settings.data, + default_grouping_model: model, + default_grouping_reasoning_effort: effort, + }) + } + disabled={!settings.data || save.isPending} + /> + additions: number + deletions: number +} + const GROUP_STYLES = { bug: { label: "Bug", className: "text-destructive", Icon: BugBeetleIcon }, investigate: { @@ -203,6 +218,12 @@ function ReviewBody({ const [focused, setFocused] = useState(null) const [anchorEl, setAnchorEl] = useState(null) const scrollRef = useRef(null) + const groupRefs = useRef>({}) + const [selectedRange, setSelectedRange] = useState<{ + file: string + start: number + end: number + } | null>(null) useEffect(() => { void warmDiffHighlighter() @@ -282,6 +303,85 @@ function ReviewBody({ .reduce((acc, file) => acc + file.additions + file.deletions, 0) }, [diffFiles, viewed]) + // Resolve the AI-sorted groups against the actual diff: drop stale groups + // (generated for a previous head) so the file-tree fallback is used, drop + // paths no longer in the diff and empty groups, and collect any unassigned + // files into a trailing "Other changes" group so nothing ever disappears. + const groupedView = useMemo | null>(() => { + if ( + !diffFiles || + detail.diff_groups_stale || + detail.diff_groups.length === 0 + ) + return null + const byPath = new Map(diffFiles.map((file) => [file.path, file])) + const assigned = new Set() + const resolved: Array> = [] + for (const group of detail.diff_groups) { + const files: Array = [] + for (const path of group.files) { + const file = byPath.get(path) + if (file && !assigned.has(path)) { + assigned.add(path) + files.push(file) + } + } + if (files.length === 0) continue + resolved.push({ + title: group.title, + summary: group.summary, + files, + additions: files.reduce((acc, file) => acc + file.additions, 0), + deletions: files.reduce((acc, file) => acc + file.deletions, 0), + }) + } + const leftover = diffFiles.filter((file) => !assigned.has(file.path)) + if (leftover.length > 0) { + resolved.push({ + title: "Other changes", + summary: "", + files: leftover, + additions: leftover.reduce((acc, file) => acc + file.additions, 0), + deletions: leftover.reduce((acc, file) => acc + file.deletions, 0), + }) + } + if (resolved.length === 0) return null + return resolved.map((group, i) => ({ ...group, index: i + 1 })) + }, [diffFiles, detail.diff_groups, detail.diff_groups_stale]) + + const sidebarGroups = useMemo | null>(() => { + if (!groupedView) return null + 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]) + + // The view follows fresh-group availability until the user explicitly picks + // one, after which the choice persists across PRs. + const hasFreshGroups = + detail.diff_groups.length > 0 && !detail.diff_groups_stale + const [explicitView, setExplicitView] = useState( + () => { + if (typeof window === "undefined") return null + const stored = window.localStorage.getItem(REVIEW_VIEW_STORAGE_KEY) + return stored === "ai" || stored === "files" ? stored : null + } + ) + const view: ReviewSidebarView = + explicitView ?? (hasFreshGroups ? "ai" : "files") + const setView = useCallback((next: ReviewSidebarView) => { + setExplicitView(next) + if (typeof window !== "undefined") { + window.localStorage.setItem(REVIEW_VIEW_STORAGE_KEY, next) + } + }, []) + const scrollToFile = useCallback((path: string) => { setSelectedFile(path) setExpandedFiles((prev) => ({ ...prev, [path]: true })) @@ -293,6 +393,30 @@ function ReviewBody({ }) }, []) + const scrollToGroup = useCallback((index: number) => { + requestAnimationFrame(() => { + groupRefs.current[index]?.scrollIntoView({ + block: "start", + behavior: "smooth", + }) + }) + }, []) + + const scrollToLineRange = useCallback( + (file: string, start: number, end: number) => { + setSelectedFile(file) + setSelectedRange({ file, start, end }) + setExpandedFiles((prev) => ({ ...prev, [file]: true })) + requestAnimationFrame(() => { + fileRefs.current[file]?.scrollIntoView({ + block: "start", + behavior: "smooth", + }) + }) + }, + [] + ) + const openFinding = useCallback( (finding: ReviewFinding) => { markRead(finding.id) @@ -314,6 +438,39 @@ function ReviewBody({ const closeFinding = useCallback(() => setFocused(null), []) + const renderFileCard = (file: ReviewDiffFile) => ( + { + const becomingViewed = !viewed.has(file.path) + if (becomingViewed && focused?.file === file.path) setFocused(null) + toggleViewed(file.path) + setExpandedFiles((prev) => ({ + ...prev, + [file.path]: !becomingViewed, + })) + }} + expanded={expandedFiles[file.path] ?? !viewed.has(file.path)} + onToggleExpanded={() => { + const next = !(expandedFiles[file.path] ?? !viewed.has(file.path)) + if (!next && focused?.file === file.path) setFocused(null) + setExpandedFiles((prev) => ({ ...prev, [file.path]: next })) + }} + onFindingClick={openFinding} + sectionRef={(node) => { + fileRefs.current[file.path] = node + }} + anchorRef={(id, node) => { + anchorRefs.current[id] = node + }} + /> + ) + const sidebarData = useMemo( () => ({ title: `PR #${detail.number}`, @@ -321,8 +478,24 @@ function ReviewBody({ selected: selectedFile, viewed, onSelect: scrollToFile, + groups: sidebarGroups, + view, + onViewChange: setView, + onSelectGroup: scrollToGroup, + onLocationClick: scrollToLineRange, }), - [detail.number, diffFiles, selectedFile, viewed, scrollToFile] + [ + detail.number, + diffFiles, + selectedFile, + viewed, + scrollToFile, + sidebarGroups, + view, + setView, + scrollToGroup, + scrollToLineRange, + ] ) useRegisterReviewSidebar(sidebarData) @@ -380,48 +553,23 @@ function ReviewBody({

No diff available.

- ) : ( -
- {diffFiles.map((file) => ( - { - const becomingViewed = !viewed.has(file.path) - if (becomingViewed && focused?.file === file.path) - setFocused(null) - toggleViewed(file.path) - setExpandedFiles((prev) => ({ - ...prev, - [file.path]: !becomingViewed, - })) + ) : view === "ai" && groupedView ? ( +
+ {groupedView.map((group) => ( +
{ + groupRefs.current[group.index] = node }} - expanded={ - expandedFiles[file.path] ?? !viewed.has(file.path) - } - onToggleExpanded={() => { - const next = !( - expandedFiles[file.path] ?? !viewed.has(file.path) - ) - if (!next && focused?.file === file.path) setFocused(null) - setExpandedFiles((prev) => ({ - ...prev, - [file.path]: next, - })) - }} - onFindingClick={openFinding} - sectionRef={(node) => { - fileRefs.current[file.path] = node - }} - anchorRef={(id, node) => { - anchorRefs.current[id] = node - }} - /> + className="scroll-mt-4 space-y-3" + > + + {group.files.map(renderFileCard)} +
))}
+ ) : ( +
{diffFiles.map(renderFileCard)}
)}
@@ -515,10 +663,30 @@ function PrHeader({ detail }: { detail: ReviewDetail }) { ) } +function GroupHeader({ group }: { group: ResolvedGroup }) { + return ( +
+ + {group.index} + +

{group.title}

+ + {group.additions > 0 && ( + +{group.additions} + )} + {group.deletions > 0 && ( + -{group.deletions} + )} + +
+ ) +} + function FileDiffCard({ file, findings, focused, + selectedRange, viewed, onToggleViewed, expanded, @@ -530,6 +698,7 @@ function FileDiffCard({ file: ReviewDiffFile findings: Array focused: ReviewFinding | null + selectedRange: { file: string; start: number; end: number } | null viewed: boolean onToggleViewed: () => void expanded: boolean @@ -552,10 +721,20 @@ function FileDiffCard({ [findings] ) - const selectedLines = - focused?.file === file.path && isAnchored(focused) - ? findingSelectedRange(focused) - : null + const selectedLines = useMemo(() => { + if (focused?.file === file.path && isAnchored(focused)) { + return findingSelectedRange(focused) + } + if (selectedRange?.file === file.path) { + return { + start: selectedRange.start, + end: selectedRange.end, + side: "additions", + endSide: "additions", + } + } + return null + }, [focused, selectedRange, file.path]) return (