fix(pr-status): address review feedback missed before merge (#570 follow-up)#580
fix(pr-status): address review feedback missed before merge (#570 follow-up)#580
Conversation
Five issues identified in review of #570 that were not addressed before merge: 1. get_pr_review_status: guard against non-list response, filter non-dict items, and exclude DISMISSED/COMMENTED states (only APPROVED and CHANGES_REQUESTED are actionable). 2. pr_v2.py get_pr_status: use safe .get() access on pr_raw (head.sha, html_url, state); raise 502 with a clear message if required fields are absent rather than letting KeyError surface as unhandled 500. 3. pr_v2.py get_pr_status: wrap GitHub calls in try/finally and call await client.close() to prevent httpx AsyncClient file-descriptor leak. 4. PRStatusPanel: encodeURIComponent(workspacePath) in SWR cache key to prevent cache mismatches for paths with spaces or special characters. 5. types/index.ts: narrow CICheck.status/conclusion and PRStatusResponse.review_status/merge_state to union literal types so typos are caught at compile time rather than failing silently at runtime. Tests: +10 backend (502 path, generic 500, client-close assertions, six direct unit tests for get_pr_review_status corner cases). All 20 pass.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughDefensive GitHub PR review parsing and aggregation; ensured GitHub client is closed on all paths; stricter PR payload validation returning HTTP 502 for malformed inputs; added tests for lifecycle and review-status edge cases; URL-encoded workspace keys and narrowed TypeScript PR/CI status types. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Browser / UI
participant Router as pr_v2.get_pr_status
participant GHClient as GitHub Client
participant GitHub as GitHub API
Client->>Router: GET /api/v2/pr/status (pr_raw)
Router->>GHClient: create client
Router->>GHClient: get_pr_review_status(pr_number)
GHClient->>GitHub: GET /repos/.../pulls/{pr}/reviews
GitHub-->>GHClient: reviews (maybe non-list / mixed items)
GHClient->>GHClient: normalize to list, filter actionable dicts, collapse latest per reviewer
GHClient-->>Router: review_status
Router->>GHClient: await client.close()
Router-->>Client: HTTP 200/502/500 with PR status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Review — fix(pr-status): #570 follow-upGood, targeted cleanup. The five issues from the original review are addressed and the test additions are solid. A few observations: What's working well
Frontend Types (index.ts) One thing to trackOther
This PR is scoped to the issues called out before merge, so I understand not fixing everything here. But it should land in a follow-up or tracked in an issue — under load these will accumulate open sockets. Minor observations (non-blocking)
Overall the PR does exactly what it says: five targeted fixes, tests updated to match, CI clean. Ship it when CodeRabbit finishes its run. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/git/github_integration.py`:
- Around line 440-446: The aggregation currently treats every actionable review
independently, so stale reviews from the same reviewer (e.g., CHANGES_REQUESTED
then later APPROVED) still count; update the logic that builds active reviews to
collapse by reviewer and keep only their latest actionable review before
computing has_changes_requested and has_approved. Specifically, group the
reviews (the reviews list and ACTIONABLE filter) by reviewer identity (e.g.,
r.get("user", {}).get("login") or another stable reviewer key), sort/compare by
a timestamp field (e.g., r.get("submitted_at") or r.get("submittedAt")) or
fallback to id to pick the most recent actionable review per reviewer, then
compute has_changes_requested and has_approved from that per-reviewer latest
set; also add a unit test for the sequence "same reviewer CHANGES_REQUESTED then
APPROVED" to ensure regression coverage.
In `@codeframe/ui/routers/pr_v2.py`:
- Around line 176-182: Ensure pr_raw and its nested "head" are dicts before
using .get: validate that pr_raw is an instance of dict and that
pr_raw.get("head") is a dict, then safely extract head_sha, pr_url, and state;
if any of those type checks fail or required fields are missing, raise the
502/invalid-GitHub-response path instead of letting attribute access on
non-dicts raise a 500. Update the logic around pr_raw, head_sha, pr_url, and
state to perform these type checks and early return/raise when the payload shape
is invalid.
In `@web-ui/src/types/index.ts`:
- Around line 277-278: Update the check-run type unions so they match all GitHub
API values: extend the status union on the type that defines "status" (currently
'queued' | 'in_progress' | 'completed') to also include 'waiting', 'requested',
and 'pending', and extend the "conclusion" union (currently 'success' |
'failure' | 'neutral' | 'cancelled' | 'timed_out' | 'action_required' |
'skipped' | null) to also include 'stale'; locate the fields named status and
conclusion in the type declaration in web-ui/src/types/index.ts and add those
missing string literals to the respective unions.
🪄 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: 0045db76-37cf-4daf-977e-8fc2fe823cd6
📒 Files selected for processing (6)
codeframe/git/github_integration.pycodeframe/ui/routers/pr_v2.pytests/ui/test_pr_status.pyweb-ui/src/__tests__/components/review/PRStatusPanel.test.tsxweb-ui/src/components/review/PRStatusPanel.tsxweb-ui/src/types/index.ts
- github_integration.py: collapse per-reviewer review history before aggregating — a reviewer who CHANGES_REQUESTed then APPROVEd now correctly counts as approved (was double-counted before) - pr_v2.py: validate pr_raw is a dict before .get() access; non-dict head field also raises 502 rather than AttributeError - types/index.ts: add missing GitHub API values to CICheck unions (status: waiting/requested/pending; conclusion: stale) - tests: add 4 new cases (same-reviewer sequence ×2, non-dict pr_raw, non-dict head field) — 24/24 pass
|
Follow-up review — latest commit (edf08e9) All three of CodeRabbit actionable items are addressed in the new commit. 1. Per-reviewer deduplication (github_integration.py) — 2. pr_raw type validation (pr_v2.py) — 3. TypeScript union completeness (types/index.ts) — Carry-forward from the first review (still open, not new):
No new concerns. The substantive correctness issues are resolved. Ready to merge after the client-leak issue is filed. |
1 participant
Follow-up to #579 (merged). Five issues from CodeRabbit and claude-review that were not addressed before merge.
What was missed and why
I checked that CI was green and that CodeRabbit passed, but I did not read the inline CodeRabbit comment on
get_pr_review_statusor either of the two claude-review PR comments before merging.Fixes
github_integration.pyget_pr_review_statusincluded DISMISSED and COMMENTED reviews; non-list/non-dict responses would AttributeErrorpr_v2.pypr_raw["head"]["sha"]etc. would KeyError on partial response → unhandled 500.get()with safe fallbacks; raise 502 with a clear message if required fields absentpr_v2.pyhttpx.AsyncClientcreated but never closed → file descriptor leaktry/finally: await client.close()PRStatusPanel.tsxworkspacePath→ cache miss for paths with spacesencodeURIComponent(workspacePath)types/index.tsstatus,conclusion,review_status,merge_statetyped as loosestringTest plan
uv run pytest tests/ui/test_pr_status.py -v— 20/20 (10 new tests: 502 path, generic 500, client-close verified, 6 direct unit tests forget_pr_review_statuscorner cases)npm test -- --testPathPattern=PRStatusPanel— 19/19 (SWR key assertion updated for encoded path)uv run ruff check .— cleannpm run build— succeedsSummary by CodeRabbit
Bug Fixes
Tests
Chores