feat(pr-history): PR history view with proof snapshots (#572)#585
feat(pr-history): PR history view with proof snapshots (#572)#585
Conversation
Add GET /api/v2/pr/history returning recently merged PRs with PROOF9 gate snapshots captured at PR creation time. Includes pr_proof_snapshots SQLite table, author field on PRDetails, and full test coverage.
Add PRHistoryPanel component to the Review page showing recently merged PRs with PROOF9 gate snapshot badges. Includes expandable gate breakdown, TypeScript types, API client method, and 13 Jest tests.
WalkthroughAdds PR-history support: stores PR proof snapshots in SQLite, exposes GET /api/v2/pr/history (joins GitHub merged PRs with stored snapshots), captures snapshots on PR creation, extends GitHub PR model with author, and adds frontend components/tests to display merged PRs with gate breakdowns. Changes
Sequence Diagram(s)sequenceDiagram
actor Browser
participant Frontend as "Web UI (PRHistoryPanel)"
participant API as "Backend /api/v2/pr/history"
participant GH as "GitHub Client"
participant DB as "SQLite (pr_proof_snapshots)"
Browser->>Frontend: load PR History (workspacePath)
Frontend->>API: GET /api/v2/pr/history?workspace=...
API->>GH: list_pull_requests(state=closed)
GH-->>API: list of PRs (with merged_at, user)
API->>DB: get_pr_proof_snapshot(workspace, pr_number) for each merged PR
DB-->>API: snapshot or null
API-->>Frontend: PRHistoryResponse (merged PRs + snapshots)
Frontend-->>Browser: render list, badges, expandable breakdowns
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
PR #585 Review — feat(pr-history): PR history view with proof snapshotsOverall: Well-structured addition that follows project conventions (core-first, thin UI adapter, SWR, Shadcn). Test coverage is solid and the test plan checks out. There is one critical bug and a few lower-priority issues to address. 🔴 Critical: Gate status string mismatchThe backend saves obligation status verbatim from the domain model (which uses
gate_breakdown.append({
\"gate\": ob.gate.value,
\"status\": ob.status, # stores \"passed\" / \"failed\" / ...
})
{gate.status === 'satisfied' ? (
<CheckmarkCircle01Icon className=\"h-3.5 w-3.5 text-green-600\" />
) : (
<Cancel01Icon className=\"h-3.5 w-3.5 text-red-600\" />
)}This means every gate will render the red ✗ icon regardless of actual pass/fail — the green ✓ is unreachable in production. The mismatch is also visible in the tests: Fix options:
🟡 Performance: Full closed-PR fetch then in-memory filter
Consider using 🟡
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
web-ui/src/__tests__/components/review/PRHistoryPanel.test.tsx (1)
139-178: Add an explicit assertion for PR GitHub link rendering.The test data includes
url, but link/href behavior isn’t validated yet.Suggested test addition
describe('PR list rendering', () => { @@ it('renders PR titles', () => { withData(SAMPLE_HISTORY); render(<PRHistoryPanel workspacePath={WORKSPACE} />); @@ expect(screen.getByText('fix: resolve login bug')).toBeInTheDocument(); }); + + it('renders GitHub PR link', () => { + withData(SAMPLE_HISTORY); + render(<PRHistoryPanel workspacePath={WORKSPACE} />); + + expect(screen.getByRole('link', { name: /feat: add user auth/i })) + .toHaveAttribute('href', 'https://github.com/owner/repo/pull/10'); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/__tests__/components/review/PRHistoryPanel.test.tsx` around lines 139 - 178, Add an assertion that the PRHistoryPanel renders the PR GitHub link using the `url` from the fixture (e.g., SAMPLE_HISTORY / ALL_PASSED entries) by locating the PR title element rendered by PRHistoryPanel and asserting its enclosing anchor has the expected href; for example, after render(<PRHistoryPanel workspacePath={WORKSPACE} />) find the title (or use getByRole('link', { name: /feat: add user auth/i })) and assert the link's href equals the corresponding SAMPLE_HISTORY[...].url value so the test verifies link/href behavior.tests/ui/test_pr_history.py (1)
162-178: Strengthenlimittest to verify which PRs are returned, not only count.Right now this passes even if the endpoint returns the wrong two items.
Suggested assertion tightening
assert resp.status_code == 200 - assert len(resp.json()["pull_requests"]) == 2 + items = resp.json()["pull_requests"] + assert len(items) == 2 + assert [pr["number"] for pr in items] == [3, 2]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/ui/test_pr_history.py` around lines 162 - 178, The test only checks the count; update test_limit_parameter to also assert which PRs are returned by extracting numbers from resp.json()["pull_requests"] and comparing them to the expected PR numbers from the prs list (e.g., the two most-recent merged PRs created via _make_pr), ensuring both order and identity match (use the prs fixture values or expected_numbers like [3,2] derived from the prs list) so the endpoint returns the correct items rather than any two.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@codeframe/ui/routers/pr_v2.py`:
- Around line 474-486: The current loop over reqs builds gates_total and
gate_breakdown from every req.obligations entry, causing duplicate gates;
instead aggregate obligations by ob.gate: iterate reqs and req.obligations to
build a dict keyed by ob.gate (use ob.gate.value or ob.gate as key) and compute
a single status per gate (e.g., mark "passed" if any obligation for that gate
has status == "passed", otherwise the most severe status), then set gates_total
= number of unique keys and build gate_breakdown from the aggregated dict;
update the code paths that reference gates_total and gate_breakdown accordingly.
- Around line 344-349: The current logic only inspects the first page returned
by client.list_pull_requests(state="closed") and then sorts that subset, which
can miss recently merged PRs; change list_pull_requests usage to paginate (fetch
subsequent pages) until you've collected at least `limit` PRs with pr.merged_at
not None or there are no more pages, accumulate merged PRs across pages into
`merged`, then sort merged by pr.merged_at descending and truncate to `limit`;
update the code that builds `merged` (the list comprehension and subsequent
sort) and the call site of client.list_pull_requests to support pagination.
- Around line 463-494: The snapshot is being taken immediately after
create_pull_request; instead, remove the proof-capture block (init_proof_tables,
list_requirements, gate counting and save_pr_proof_snapshot) from the create PR
flow and relocate it to the PR merge flow so snapshots reflect final merged
state; implement the same logic (call init_proof_tables(workspace), compute
gates_total/gates_passed/gate_breakdown from list_requirements(workspace) and
call save_pr_proof_snapshot(workspace, pr_number=pr.number, ...)) in the merge
handler (the function that performs PR merge/merge_pull_request) so the proof
snapshot is created/updated at merge time rather than on create.
In `@web-ui/src/components/review/PRHistoryPanel.tsx`:
- Around line 90-124: The GitHub anchor is nested inside the toggle <button>,
which is invalid; refactor the JSX so the toggle button (the element that calls
toggleExpand and uses expandedPR/ pr.number and contains the left-side content
and the Badge using proofBadgeClasses/proofBadgeText) is one sibling and the
external link anchor (using pr.url, rel, target, aria-label and
ArrowUpRight01Icon) is a separate sibling element next to it inside the same
container; keep the button's aria-expanded and focus styles, preserve the
anchor's classes and aria-label, and keep or reapply the onClick stopPropagation
logic on the anchor (or remove it if no longer needed) so clicking the external
link does not toggle the row.
---
Nitpick comments:
In `@tests/ui/test_pr_history.py`:
- Around line 162-178: The test only checks the count; update
test_limit_parameter to also assert which PRs are returned by extracting numbers
from resp.json()["pull_requests"] and comparing them to the expected PR numbers
from the prs list (e.g., the two most-recent merged PRs created via _make_pr),
ensuring both order and identity match (use the prs fixture values or
expected_numbers like [3,2] derived from the prs list) so the endpoint returns
the correct items rather than any two.
In `@web-ui/src/__tests__/components/review/PRHistoryPanel.test.tsx`:
- Around line 139-178: Add an assertion that the PRHistoryPanel renders the PR
GitHub link using the `url` from the fixture (e.g., SAMPLE_HISTORY / ALL_PASSED
entries) by locating the PR title element rendered by PRHistoryPanel and
asserting its enclosing anchor has the expected href; for example, after
render(<PRHistoryPanel workspacePath={WORKSPACE} />) find the title (or use
getByRole('link', { name: /feat: add user auth/i })) and assert the link's href
equals the corresponding SAMPLE_HISTORY[...].url value so the test verifies
link/href behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b59fad2c-5a87-40a1-83ab-7821dbbd15ff
📒 Files selected for processing (11)
codeframe/core/proof/ledger.pycodeframe/git/github_integration.pycodeframe/ui/routers/pr_v2.pytests/core/test_proof_snapshot.pytests/ui/test_pr_history.pyweb-ui/__mocks__/@hugeicons/react.jsweb-ui/src/__tests__/components/review/PRHistoryPanel.test.tsxweb-ui/src/app/review/page.tsxweb-ui/src/components/review/PRHistoryPanel.tsxweb-ui/src/lib/api.tsweb-ui/src/types/index.ts
| prs = await client.list_pull_requests(state="closed") | ||
|
|
||
| # Filter to only merged PRs and sort newest first. | ||
| merged = [pr for pr in prs if pr.merged_at is not None] | ||
| merged.sort(key=lambda pr: pr.merged_at, reverse=True) | ||
| merged = merged[:limit] |
There was a problem hiding this comment.
/history can miss the actual latest merged PRs.
This only examines GitHub's first page of state="closed" PRs and then sorts that subset by merged_at. A long-lived PR merged recently can fall off page 1, so the history view can omit the real latest merged PRs. Paginate until you collect limit merged PRs, or query GitHub by merge time directly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codeframe/ui/routers/pr_v2.py` around lines 344 - 349, The current logic only
inspects the first page returned by client.list_pull_requests(state="closed")
and then sorts that subset, which can miss recently merged PRs; change
list_pull_requests usage to paginate (fetch subsequent pages) until you've
collected at least `limit` PRs with pr.merged_at not None or there are no more
pages, accumulate merged PRs across pages into `merged`, then sort merged by
pr.merged_at descending and truncate to `limit`; update the code that builds
`merged` (the list comprehension and subsequent sort) and the call site of
client.list_pull_requests to support pagination.
| # Capture proof snapshot at PR creation time. | ||
| try: | ||
| from codeframe.core.proof.ledger import ( | ||
| init_proof_tables, | ||
| list_requirements, | ||
| save_pr_proof_snapshot, | ||
| ) | ||
|
|
||
| init_proof_tables(workspace) | ||
| reqs = list_requirements(workspace) | ||
|
|
||
| gates_total = 0 | ||
| gates_passed = 0 | ||
| gate_breakdown: list[dict] = [] | ||
| for req in reqs: | ||
| for ob in req.obligations: | ||
| gates_total += 1 | ||
| passed = ob.status == "passed" | ||
| if passed: | ||
| gates_passed += 1 | ||
| gate_breakdown.append({ | ||
| "gate": ob.gate.value, | ||
| "status": ob.status, | ||
| }) | ||
|
|
||
| save_pr_proof_snapshot( | ||
| workspace, | ||
| pr_number=pr.number, | ||
| gates_passed=gates_passed, | ||
| gates_total=gates_total, | ||
| gate_breakdown=gate_breakdown, | ||
| ) |
There was a problem hiding this comment.
The snapshot is captured at PR creation, not at merge.
This stores proof data immediately after create_pull_request(), but the feature is exposing proof state for merged PR history. Any gates that change while the PR is open will make the history snapshot stale and misleading. Capture or refresh the snapshot in the merge flow instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codeframe/ui/routers/pr_v2.py` around lines 463 - 494, The snapshot is being
taken immediately after create_pull_request; instead, remove the proof-capture
block (init_proof_tables, list_requirements, gate counting and
save_pr_proof_snapshot) from the create PR flow and relocate it to the PR merge
flow so snapshots reflect final merged state; implement the same logic (call
init_proof_tables(workspace), compute gates_total/gates_passed/gate_breakdown
from list_requirements(workspace) and call save_pr_proof_snapshot(workspace,
pr_number=pr.number, ...)) in the merge handler (the function that performs PR
merge/merge_pull_request) so the proof snapshot is created/updated at merge time
rather than on create.
| gates_total = 0 | ||
| gates_passed = 0 | ||
| gate_breakdown: list[dict] = [] | ||
| for req in reqs: | ||
| for ob in req.obligations: | ||
| gates_total += 1 | ||
| passed = ob.status == "passed" | ||
| if passed: | ||
| gates_passed += 1 | ||
| gate_breakdown.append({ | ||
| "gate": ob.gate.value, | ||
| "status": ob.status, | ||
| }) |
There was a problem hiding this comment.
This counts requirement obligations, not unique gates.
gates_total and gate_breakdown are built from every req.obligations entry, so the same PROOF9 gate can appear multiple times across requirements. That inflates the badge and produces repeated rows instead of a gate-by-gate snapshot. Aggregate by ob.gate before persisting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@codeframe/ui/routers/pr_v2.py` around lines 474 - 486, The current loop over
reqs builds gates_total and gate_breakdown from every req.obligations entry,
causing duplicate gates; instead aggregate obligations by ob.gate: iterate reqs
and req.obligations to build a dict keyed by ob.gate (use ob.gate.value or
ob.gate as key) and compute a single status per gate (e.g., mark "passed" if any
obligation for that gate has status == "passed", otherwise the most severe
status), then set gates_total = number of unique keys and build gate_breakdown
from the aggregated dict; update the code paths that reference gates_total and
gate_breakdown accordingly.
- Fix gate status comparison: use "satisfied" (domain model) not "passed" - Fix 0/0 gates edge case rendering as green instead of yellow - Fix nested <a> inside <button> (invalid HTML, accessibility issue) - Align test fixtures to use "satisfied" status string
Review: PR History with Proof Snapshots (#572)Overall this is a well-structured feature — clear layer separation (ledger → router → component), good test coverage across all three layers, and proper handling of loading/empty/error states in the UI. A few things worth addressing before merge, some of which overlap with CodeRabbit's flags: Issues to address1. Gate deduplication (overlaps CodeRabbit flag) In Fix: aggregate by 2. Snapshot captured at PR create, not merge The proof snapshot fires inside 3.
4.
5. Redundant Inside the create PR proof-capture block ( 6. SWR key / fetcher alignment const swrKey = `/api/v2/pr/history?workspace_path=${encodeURIComponent(workspacePath)}`;
const { data } = useSWR<PRHistoryResponse>(
swrKey,
() => prApi.getHistory(workspacePath) // constructs URL differently, ignores limit
);The SWR key and the fetcher construct different URLs. If Nitpicks (non-blocking)
What's working well
The gate deduplication (#1) and status rendering (#4) are the most important to address before this goes in. The rest can be sequenced as follow-ups if needed. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
web-ui/src/components/review/PRHistoryPanel.tsx (1)
138-152: Potential duplicate React keys if backend sends duplicate gates.Using
gate.gateas the key assumes unique gate names ingate_breakdown. If the backend sends duplicate gate entries (related to the obligation counting issue flagged in the backend review), React will warn about duplicate keys and may exhibit rendering issues.Consider adding an index suffix as a fallback or addressing deduplication on the backend.
♻️ Defensive key using index
- {pr.proof_snapshot.gate_breakdown.map((gate: GateBreakdownItem) => ( + {pr.proof_snapshot.gate_breakdown.map((gate: GateBreakdownItem, idx: number) => ( <div - key={gate.gate} + key={`${gate.gate}-${idx}`} className="flex items-center gap-2 text-xs" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web-ui/src/components/review/PRHistoryPanel.tsx` around lines 138 - 152, The current mapping over pr.proof_snapshot.gate_breakdown uses gate.gate as the React key which can collide if duplicate GateBreakdownItem.gate values are returned; change the key to a stable composite or index-fallback (e.g., use `${gate.gate}-${i}` or similar) when rendering the list in the component that maps pr.proof_snapshot.gate_breakdown so each element has a unique key, or deduplicate/normalize entries upstream before mapping to avoid duplicates.codeframe/ui/routers/pr_v2.py (1)
340-353: Synchronous SQLite call in async endpoint may block the event loop.
get_pr_proof_snapshotperforms synchronous database I/O. While the impact is minimal for SQLite on fast storage, this pattern can block other coroutines under load. Consider wrapping inasyncio.to_thread()for consistency with async best practices.♻️ Optional improvement using asyncio.to_thread
for pr in merged: - snapshot = get_pr_proof_snapshot(workspace, pr.number) + snapshot = await asyncio.to_thread(get_pr_proof_snapshot, workspace, pr.number) proof_snapshot = None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codeframe/ui/routers/pr_v2.py` around lines 340 - 353, The call to the synchronous DB helper get_pr_proof_snapshot(workspace, pr.number) in the async handler should be moved off the event loop: import asyncio and replace the direct call with an awaited asyncio.to_thread(...) invocation (e.g., snapshot = await asyncio.to_thread(get_pr_proof_snapshot, workspace, pr.number)) so the synchronous SQLite I/O does not block other coroutines while constructing PRHistoryItem entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@codeframe/ui/routers/pr_v2.py`:
- Around line 340-353: The call to the synchronous DB helper
get_pr_proof_snapshot(workspace, pr.number) in the async handler should be moved
off the event loop: import asyncio and replace the direct call with an awaited
asyncio.to_thread(...) invocation (e.g., snapshot = await
asyncio.to_thread(get_pr_proof_snapshot, workspace, pr.number)) so the
synchronous SQLite I/O does not block other coroutines while constructing
PRHistoryItem entries.
In `@web-ui/src/components/review/PRHistoryPanel.tsx`:
- Around line 138-152: The current mapping over pr.proof_snapshot.gate_breakdown
uses gate.gate as the React key which can collide if duplicate
GateBreakdownItem.gate values are returned; change the key to a stable composite
or index-fallback (e.g., use `${gate.gate}-${i}` or similar) when rendering the
list in the component that maps pr.proof_snapshot.gate_breakdown so each element
has a unique key, or deduplicate/normalize entries upstream before mapping to
avoid duplicates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dbf7014e-607a-4b90-b48b-bfabb3dec451
📒 Files selected for processing (4)
codeframe/ui/routers/pr_v2.pytests/core/test_proof_snapshot.pytests/ui/test_pr_history.pyweb-ui/src/components/review/PRHistoryPanel.tsx
✅ Files skipped from review due to trivial changes (2)
- tests/core/test_proof_snapshot.py
- tests/ui/test_pr_history.py
1 participant
Summary
GET /api/v2/pr/historyendpoint returning recently merged PRs with PROOF9 gate snapshots captured at PR creation time. Newpr_proof_snapshotsSQLite table in the workspace DB,authorfield onPRDetailsdataclass.PRHistoryPanelcomponent on the Review page showing merged PRs with proof badge (gates passed/total), expandable gate-by-gate breakdown, empty/loading/error states.Closes #572
Test plan
uv run pytest tests/ui/test_pr_history.py tests/core/test_proof_snapshot.py— 11 passedcd web-ui && npm test— 773 passed (60 suites), including 13 PRHistoryPanel testscd web-ui && npm run build— succeedsuv run ruff check— all passeduv run pytest— 3158 passed (1 pre-existing flaky stall_monitor timing test)Summary by CodeRabbit