feat(pr-history): Report Glitch action and REQ→PR back-link (#573)#586
feat(pr-history): Report Glitch action and REQ→PR back-link (#573)#586
Conversation
Add "Report Glitch" button to each merged PR row in the PR History panel.
Clicking it fetches the PR's changed files and opens the Capture Glitch
modal pre-populated with scope, source, and a PR reference note. REQs
created this way store the GitHub PR URL as source_issue, which now
renders as a clickable link on the REQ detail page.
Backend:
- GitHubIntegration.get_pr_files() fetches changed files for a PR
- GET /api/v2/pr/{pr_number}/files endpoint exposes PR changed files
Frontend:
- CaptureGlitchModal accepts optional prNumber/prTitle/prUrl/initialScope
props for pre-population and includes source_issue in the payload
- PRHistoryPanel rows include [Report Glitch] button with loading state
- REQ detail page renders source_issue as a GitHub link when applicable
- PRFilesResponse type and prApi.getFiles() API method added
|
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)
WalkthroughAdds a backend endpoint to fetch filenames changed in a GitHub PR, a GitHub client method, frontend API and UI wiring to fetch those files from PR history, pre-fill the Capture Glitch modal (scope, description, optional source PR), and conditional rendering of a Source PR link on requirement detail pages. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant PRHistory as PR History Panel
participant FrontendAPI as Frontend API (prApi)
participant Backend as Backend Router (/api/v2)
participant GitHub as GitHub API
participant Modal as Capture Glitch Modal
User->>PRHistory: Click "Report Glitch"
PRHistory->>FrontendAPI: prApi.getFiles(workspace, prNumber)
FrontendAPI->>Backend: GET /api/v2/pr/{pr_number}/files
Backend->>GitHub: GET /repos/.../pulls/{pr_number}/files?per_page=100
GitHub-->>Backend: PR file list
Backend-->>FrontendAPI: PRFilesResponse(files)
FrontendAPI-->>PRHistory: string[] (filenames)
PRHistory->>Modal: Open with {prNumber, prTitle, prUrl, initialScope: files}
Modal->>Modal: Pre-fill description and scope
User->>Modal: Submit
Modal->>FrontendAPI: proofApi.capture({... , source_issue?: prUrl})
FrontendAPI-->>Modal: Success
Modal->>PRHistory: Close
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
|
Review posted via file |
|
Review: feat(pr-history): Report Glitch action and REQ-to-PR back-link The PR delivers a clean, well-scoped feature. Architecture follows existing patterns, test coverage is solid, and the CI checklist checks out. A few items worth addressing before merge. What is working well
Issue 1: GitHub API pagination -- get_pr_files() is incomplete for large PRs The GitHub API returns at most 30 files per page by default. A PR touching 31+ files will silently return a truncated list, leaving the pre-populated scope in the Capture Glitch modal incomplete with no user indication. Quick fix: add per_page=100 to the request params. Full pagination via the Link header is more thorough but likely overkill for this use case. Issue 2: Silent error swallowed in handleReportGlitch When the files fetch fails, the modal opens silently with empty scope. The user cannot distinguish a network error from a PR with no changed files. A toast or inline error banner would prevent glitches being captured with empty scope due to transient API failures. Issue 3: Redundant open prop The outer conditional already gates on glitchTarget being truthy, so open={!!glitchTarget} is always true inside that block. Either drop the outer conditional and let the open prop control visibility (the idiomatic pattern for controlled modals), or change it to open={true}. Issue 4: Unused workspace dependency in the /files endpoint The injected workspace is never referenced in the function body. This forces callers to supply a valid workspace_path for a purely GitHub-side operation. If kept intentionally (auth context, future use), a brief comment would clarify intent -- otherwise it can be removed. Minor
Summary
The pagination gap is the only item with correctness impact. The rest is polish. Solid implementation overall -- this cleanly closes the feedback loop between merged PRs and glitch capture. |
- Add per_page=100 to get_pr_files() to avoid silent truncation on PRs
with 30+ changed files (GitHub API default is 30)
- Show fallback note in scope when file fetch fails so user knows to
enter scope manually (was silently empty)
- Simplify redundant open={!!glitchTarget} to open (always true inside
the conditional render block)
- Remove unused ProofRequirement import from PRHistoryPanel
![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
🤖 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 393-395: The current call to self._make_request for endpoint
"/repos/{self.owner}/{self.repo_name}/pulls/{pr_number}/files" only fetches a
single page; update the method (where endpoint and pr_number are used) to
paginate until all files are retrieved by either iterating page numbers with
per_page=100 query param or following the Link header from self._make_request
responses, accumulating filenames from each page into a single list before
returning; ensure you handle empty pages and stop when no next page exists.
In `@tests/unit/test_github_integration.py`:
- Around line 302-347: The new tests in TestGetPrFiles (including
test_returns_list_of_filenames, test_returns_empty_list_for_no_files,
test_propagates_api_error) are missing the required v2 marker; add the marker by
either decorating the class or each test with `@pytest.mark.v2` (or add pytestmark
= pytest.mark.v2 at the module top) so these tests run under the enforced pytest
-m v2 profile; ensure the marker import (pytest) remains available and update
only the test file containing TestGetPrFiles and its test methods.
In `@web-ui/src/app/proof/`[req_id]/page.tsx:
- Around line 264-274: Replace the fragile startsWith check for req.source_issue
with strict URL parsing and host validation: in the conditional that renders the
"Source PR" link (where req.source_issue is used in page.tsx), wrap new
URL(req.source_issue) in a try/catch and only render the link if url.protocol
=== 'https:' and url.hostname === 'github.com'; otherwise treat it as untrusted
(do not render or render plain text). This ensures lookalike hosts like
"github.com.evil.tld" are rejected while preserving valid https://github.com
links.
In `@web-ui/src/components/review/PRHistoryPanel.tsx`:
- Around line 68-77: Concurrent clicks can cause stale getFiles responses to
overwrite glitchTarget; fix handleReportGlitch by associating each fetch with
the PR number and ignoring responses that don't match the latest request:
capture const requestNumber = pr.number, setLoadingFiles(requestNumber), then
after awaiting prApi.getFiles check that the current request equals
requestNumber (use an up-to-date source such as a loadingFilesRef mirrored from
loadingFiles state or compare against loadingFiles state read via a ref) before
calling setGlitchTarget; apply the same guard to the catch branch so only the
most recent request updates setGlitchTarget and ensure finally still clears
loading using setLoadingFiles(null).
🪄 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: 190cae3d-ebf3-41d9-bb51-83be6d53a513
📒 Files selected for processing (12)
codeframe/git/github_integration.pycodeframe/ui/routers/pr_v2.pytests/ui/test_pr_history.pytests/unit/test_github_integration.pyweb-ui/src/__tests__/components/proof/CaptureGlitchModal.test.tsxweb-ui/src/__tests__/components/proof/ProofDetailPage.test.tsxweb-ui/src/__tests__/components/review/PRHistoryPanel.test.tsxweb-ui/src/app/proof/[req_id]/page.tsxweb-ui/src/components/proof/CaptureGlitchModal.tsxweb-ui/src/components/review/PRHistoryPanel.tsxweb-ui/src/lib/api.tsweb-ui/src/types/index.ts
| const handleReportGlitch = async (pr: PRHistoryItem) => { | ||
| setLoadingFiles(pr.number); | ||
| try { | ||
| const files = await prApi.getFiles(workspacePath, pr.number); | ||
| setGlitchTarget({ pr, files }); | ||
| } catch { | ||
| setGlitchTarget({ pr, files: [] }); | ||
| } finally { | ||
| setLoadingFiles(null); | ||
| } |
There was a problem hiding this comment.
Guard against out-of-order getFiles responses (stale modal data).
Concurrent clicks across different rows can race; the slower earlier request can overwrite glitchTarget and open the modal for the wrong PR.
Suggested fix
-import { useState } from 'react';
+import { useRef, useState } from 'react';
@@
export function PRHistoryPanel({ workspacePath }: PRHistoryPanelProps) {
const [expandedPR, setExpandedPR] = useState<number | null>(null);
const [loadingFiles, setLoadingFiles] = useState<number | null>(null);
+ const latestRequestId = useRef(0);
@@
const handleReportGlitch = async (pr: PRHistoryItem) => {
+ const requestId = ++latestRequestId.current;
setLoadingFiles(pr.number);
try {
const files = await prApi.getFiles(workspacePath, pr.number);
- setGlitchTarget({ pr, files });
+ if (requestId === latestRequestId.current) {
+ setGlitchTarget({ pr, files });
+ }
} catch {
- setGlitchTarget({ pr, files: [] });
+ if (requestId === latestRequestId.current) {
+ setGlitchTarget({ pr, files: [] });
+ }
} finally {
- setLoadingFiles(null);
+ if (requestId === latestRequestId.current) {
+ setLoadingFiles(null);
+ }
}
};
@@
- disabled={loadingFiles === pr.number}
+ disabled={loadingFiles !== null}Also applies to: 148-149, 204-214
🤖 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 68 - 77,
Concurrent clicks can cause stale getFiles responses to overwrite glitchTarget;
fix handleReportGlitch by associating each fetch with the PR number and ignoring
responses that don't match the latest request: capture const requestNumber =
pr.number, setLoadingFiles(requestNumber), then after awaiting prApi.getFiles
check that the current request equals requestNumber (use an up-to-date source
such as a loadingFilesRef mirrored from loadingFiles state or compare against
loadingFiles state read via a ref) before calling setGlitchTarget; apply the
same guard to the catch branch so only the most recent request updates
setGlitchTarget and ensure finally still clears loading using
setLoadingFiles(null).
|
Follow-up correction to my previous review On re-reading the diff more carefully, two items in my prior review were inaccurate and I want to correct them: Issue 1 (Pagination) — correction endpoint = f"/repos/{self.owner}/{self.repo_name}/pulls/{pr_number}/files?per_page=100"The actual concern is that GitHub's max for this endpoint is 300 files across multiple pages, so a PR touching more than 100 files will be silently truncated. The current code handles the common case (≤100 files) correctly. For completeness, full pagination via the Issue 2 (Error handling) — correction setGlitchTarget({ pr, files: ['# Could not load changed files — enter scope manually'] });This surfaces in the scope textarea and gives the user actionable instruction. It is reasonable error handling. A toast could complement it, but the current implementation is not silent. The remaining items (redundant |
- Paginate get_pr_files() to handle PRs with 100+ changed files (loop with per_page=100 until all pages exhausted) - Add @pytest.mark.v2 to TestGetPrFiles so tests run in enforced v2 profile - Use URL() constructor for strict host validation on source_issue links (rejects lookalike hosts like github.com.evil.tld) - Disable all Report Glitch buttons during any file fetch to prevent race conditions from concurrent clicks
|
Follow-up review (addressing prior feedback) The commit at 111cb3e picks up all three of the substantive issues raised in my first review (after I corrected myself) and by CodeRabbit. Summary of what landed: All critical items resolved
Two nits still open from the original review These were flagged before and are unchanged. Neither blocks merge, but they're worth a quick note for housekeeping:
Minor coverage gap worth knowing
Overall this is a clean, well-scoped implementation. The pagination fix is correct, the URL validation is solid, and the test coverage across backend, router, and frontend is thorough. The two open nits are minor polish items. |
1 participant
Summary
GET /api/v2/pr/{number}/filesendpoint, then opens the Capture Glitch modal pre-populated with scope, source, and a PR reference notesource_issue, which now renders as a clickable link on the REQ detail pageCloses #573
Changes
Backend (Python)
GitHubIntegration.get_pr_files(pr_number)— fetches changed filenames from GitHub APIGET /api/v2/pr/{pr_number}/files— new endpoint returningPRFilesResponse { files: string[] }Frontend (TypeScript)
CaptureGlitchModal— new optional propsprNumber,prTitle,prUrl,initialScopefor pre-population; includessource_issuein submission payloadPRHistoryPanel— [Report Glitch] button on each row with loading spinner during file fetchproof/[req_id]/page.tsx—source_issuerenders as clickable GitHub link when applicableprApi.getFiles()andPRFilesResponsetype addedTest plan
get_pr_files(pass, empty, error propagation)GET /api/v2/pr/{number}/files(200, empty, 404, close)uv run pytest)npm test)npm run buildsucceedsruff check .passesSummary by CodeRabbit