Fix PR List Update on Post Status Click #1207
Conversation
…PRs hook Fix PR list not updating when "Post Status" button is clicked for blocked PRs. Changes: - Add markReviewPosted function to useGitHubPRs hook that updates the store with hasPostedFindings: true - Pass markReviewPosted through GitHubPRs.tsx to PRDetail component - Call onMarkReviewPosted in handlePostBlockedStatus after successful post This ensures the PR list status display updates immediately when posting blocked status (BLOCKED/NEEDS_REVISION verdicts with no findings). Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
Test User seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (7)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a new markReviewPosted flow: IPC channel and main handler persist a PR review as "posted"; preload API exposes markReviewPosted; useGitHubPRs hook exposes markReviewPosted and updates in-memory state; GitHubPRs and PRDetail are wired to invoke it after a successful blocked-status comment post. Changes
Sequence DiagramsequenceDiagram
participant User
participant PRDetail
participant GitHubPRs
participant Hook as useGitHubPRs
participant Preload as PreloadAPI
participant Main as MainProcess
participant Disk as FileSystem
User->>PRDetail: Click "Post blocked-status" (comment)
PRDetail->>PRDetail: call onPostComment(body)
PRDetail->>GitHubPRs: onMarkReviewPosted(prNumber)
GitHubPRs->>Hook: markReviewPosted(prNumber)
Hook->>Preload: invoke markReviewPosted(projectId, prNumber)
Preload->>Main: IPC GITHUB_PR_MARK_REVIEW_POSTED(projectId, prNumber)
Main->>Disk: read PR review file
Disk-->>Main: file contents / ENOENT
Main->>Disk: write updated review (has_posted_findings, posted_at)
Disk-->>Main: write result
Main-->>Preload: return success boolean
Preload-->>Hook: resolve promise
Hook-->>GitHubPRs: resolve
GitHubPRs-->>PRDetail: callback complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @AndyMik90, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a UI synchronization bug where the PR list failed to update its display after a user clicked the 'Post Status' button within the PR detail view. The fix involves introducing a new mechanism to explicitly update the local PR review store, ensuring that changes made in the detail view are immediately reflected in the main PR list, providing a consistent user experience. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
github-actions
bot
added
area/frontend
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively resolves the issue where the PR list status was not updating after posting a 'blocked' status. The solution introduces a new markReviewPosted function in the useGitHubPRs hook to update the store, which is then correctly wired through the component hierarchy to be called from handlePostBlockedStatus. The implementation is clean and follows the existing patterns in the codebase.
I've included a few suggestions to refactor the new logic to be synchronous, as the underlying store update is a synchronous operation. This would remove unnecessary async/await and make the code's intent clearer. Overall, great work on this fix!
| const handleMarkReviewPosted = useCallback(async () => { | ||
| if (selectedPRNumber) { | ||
| await markReviewPosted(selectedPRNumber); | ||
| } | ||
| }, [selectedPRNumber, markReviewPosted]); |
There was a problem hiding this comment.
This handler can be simplified to be synchronous. The underlying markReviewPosted function performs a synchronous state update, so there's no need for async/await here. This change improves clarity by accurately reflecting the synchronous nature of the operation.
| const handleMarkReviewPosted = useCallback(async () => { | |
| if (selectedPRNumber) { | |
| await markReviewPosted(selectedPRNumber); | |
| } | |
| }, [selectedPRNumber, markReviewPosted]); | |
| const handleMarkReviewPosted = useCallback(() => { | |
| if (selectedPRNumber) { | |
| markReviewPosted(selectedPRNumber); | |
| } | |
| }, [selectedPRNumber, markReviewPosted]); |
| onMergePR: (mergeMethod?: 'merge' | 'squash' | 'rebase') => void; | ||
| onAssignPR: (username: string) => void; | ||
| onGetLogs: () => Promise<PRLogsType | null>; | ||
| onMarkReviewPosted?: () => Promise<void>; |
There was a problem hiding this comment.
To align with making markReviewPosted a synchronous function, this prop type should be updated to not return a Promise. The operation is a synchronous state update.
| onMarkReviewPosted?: () => Promise<void>; | |
| onMarkReviewPosted?: () => void; |
| setBlockedStatusPosted(true); | ||
| setBlockedStatusError(null); | ||
| // Update the store to mark review as posted so PR list reflects the change | ||
| await onMarkReviewPosted?.(); |
There was a problem hiding this comment.
Since onMarkReviewPosted should be a synchronous function that doesn't return a promise, the await keyword is unnecessary and should be removed.
| await onMarkReviewPosted?.(); | |
| onMarkReviewPosted?.(); |
| postComment: (prNumber: number, body: string) => Promise<boolean>; | ||
| mergePR: (prNumber: number, mergeMethod?: "merge" | "squash" | "rebase") => Promise<boolean>; | ||
| assignPR: (prNumber: number, username: string) => Promise<boolean>; | ||
| markReviewPosted: (prNumber: number) => Promise<void>; |
There was a problem hiding this comment.
The markReviewPosted function performs a synchronous state update. To make the code clearer and avoid unnecessary asynchronicity, its type should be (prNumber: number) => void; instead of returning a Promise.
| markReviewPosted: (prNumber: number) => Promise<void>; | |
| markReviewPosted: (prNumber: number) => void; |
| const markReviewPosted = useCallback( | ||
| async (prNumber: number): Promise<void> => { | ||
| if (!projectId) return; | ||
|
|
||
| const existingState = getPRReviewState(projectId, prNumber); | ||
| if (existingState?.result) { | ||
| // Update the store with hasPostedFindings: true | ||
| usePRReviewStore.getState().setPRReviewResult( | ||
| projectId, | ||
| { ...existingState.result, hasPostedFindings: true }, | ||
| { preserveNewCommitsCheck: true } | ||
| ); | ||
| } | ||
| }, | ||
| [projectId, getPRReviewState] | ||
| ); |
There was a problem hiding this comment.
This function is marked as async and returns a Promise, but its body only contains synchronous code (updating the Zustand store). This introduces unnecessary asynchronicity. The function should be made synchronous to more accurately reflect its behavior. This also requires updating its type definition and all call sites to remove async/await.
| const markReviewPosted = useCallback( | |
| async (prNumber: number): Promise<void> => { | |
| if (!projectId) return; | |
| const existingState = getPRReviewState(projectId, prNumber); | |
| if (existingState?.result) { | |
| // Update the store with hasPostedFindings: true | |
| usePRReviewStore.getState().setPRReviewResult( | |
| projectId, | |
| { ...existingState.result, hasPostedFindings: true }, | |
| { preserveNewCommitsCheck: true } | |
| ); | |
| } | |
| }, | |
| [projectId, getPRReviewState] | |
| ); | |
| const markReviewPosted = useCallback( | |
| (prNumber: number): void => { | |
| if (!projectId) return; | |
| const existingState = getPRReviewState(projectId, prNumber); | |
| if (existingState?.result) { | |
| // Update the store with hasPostedFindings: true | |
| usePRReviewStore.getState().setPRReviewResult( | |
| projectId, | |
| { ...existingState.result, hasPostedFindings: true }, | |
| { preserveNewCommitsCheck: true } | |
| ); | |
| } | |
| }, | |
| [projectId, getPRReviewState] | |
| ); |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| const handleMarkReviewPosted = useCallback(async () => { | ||
| if (selectedPRNumber) { | ||
| await markReviewPosted(selectedPRNumber); | ||
| } | ||
| }, [selectedPRNumber, markReviewPosted]); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🟠 NEEDS REVISION
🟠 Needs revision - 2 issue(s) require attention.
2 issue(s) must be addressed (0 required, 2 recommended), 1 suggestions
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | Low | Based on lines changed |
| Security Impact | None | Based on security findings |
| Scope Coherence | Good | Based on structural review |
Findings Summary
- Medium: 2 issue(s)
- Low: 1 issue(s)
Generated by Auto Claude PR Review
Findings (3 selected of 3 total)
🟡 [36262582bbce] [MEDIUM] Silent failure when existingState.result is null
📁 apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts:506
The markReviewPosted function silently returns when existingState?.result is null/undefined without any logging or feedback. While this edge case is unlikely in normal flow (called after successful GitHub post for existing review), if it occurs the user would see the GitHub post succeed but the PR list wouldn't update, with no indication of why.
Suggested fix:
Add console.warn for debugging: `if (!existingState?.result) { console.warn('[markReviewPosted] No review result to update for PR #' + prNumber); return; }`
🟡 [38a76dd2af4c] [MEDIUM] Return type inconsistency with sibling functions
📁 apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts:55
The markReviewPosted function returns Promise<void> while all other action functions in the same interface return Promise<boolean> (postComment, mergePR, assignPR, cancelReview, postReview). This inconsistency means callers cannot determine if the operation succeeded, unlike all sibling functions.
Suggested fix:
Change return type to `Promise<boolean>` and update implementation to return `true` on success, `false` when `!projectId` or `!existingState?.result`. Update prop type in PRDetail.tsx accordingly.
🔵 [a343a5b553e5] [LOW] Optional callback inconsistent with sibling props
📁 apps/frontend/src/renderer/components/github-prs/components/PRDetail.tsx:58
The onMarkReviewPosted prop is optional (?) while similar action callbacks (onPostComment, onMergePR, onAssignPR) are required. This minor inconsistency could lead to confusion about which callbacks are expected.
Suggested fix:
Consider making `onMarkReviewPosted` required to match sibling callbacks, or document why it should remain optional.
This review was generated by Auto Claude.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🟠 Follow-up Review: Needs Revision
🟠 Needs revision - 1 blocking issue(s) require fixes.
Resolution Status
- ✅ Resolved: 3 previous findings addressed
- ❌ Unresolved: 0 previous findings remain
- 🆕 New Issues: 1 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 3 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 0 findings verified as genuine issues
- 👤 Needs Human Review: 0 findings require manual verification
🚨 Blocking Issues
- quality: markReviewPosted only updates in-memory store without disk persistence
Verdict
CI Status: ⏳ 1 check pending - cannot approve until CI completes.
Previous Findings: All 3 previous findings (2 medium, 1 low) were dismissed as FALSE POSITIVES after finding-validator investigated the actual code:
- Silent return pattern is intentional and consistent with codebase
- Void return type is semantically correct for local store operations
- Optional callback is intentional for backward compatibility
New Finding: 1 MEDIUM severity issue identified - markReviewPosted only updates in-memory store without disk persistence. After app restart, the hasPostedFindings flag will be lost. This is an incomplete implementation that should be addressed.
Verdict Rationale: NEEDS_REVISION due to:
- CI check still pending (cannot approve until passing)
- New medium-severity persistence issue needs to be fixed
Once CI passes and the persistence issue is addressed, this PR should be READY_TO_MERGE.
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, comment-analyzer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (1 selected of 1 total)
🟡 [NEW-001] [MEDIUM] markReviewPosted only updates in-memory store without disk persistence
📁 apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts:502
The markReviewPosted function updates the Zustand store with hasPostedFindings: true, but this change is not persisted to the review JSON file on disk. When handlePostBlockedStatus posts a blocked status comment and calls markReviewPosted, the hasPostedFindings flag is only updated in memory. After an app restart, the flag will be lost because the review file on disk still has has_posted_findings: false. This is inconsistent with postReview which properly persists the flag.
Suggested fix:
Add a new IPC handler that updates the has_posted_findings flag in the review JSON file on disk, similar to how postPRReview does. Call this IPC handler from markReviewPosted in addition to updating the store.
This review was generated by Auto Claude.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
apps/frontend/src/renderer/components/github-prs/components/PRDetail.tsx
Outdated
Show resolved
Hide resolved
The markReviewPosted function was only updating the in-memory Zustand store without persisting the has_posted_findings flag to the review JSON file on disk. After an app restart, the flag would be lost. This commit adds: - New IPC handler GITHUB_PR_MARK_REVIEW_POSTED that updates the review JSON file on disk with has_posted_findings=true and posted_at timestamp - New API method markReviewPosted in github-api.ts - Updated markReviewPosted in useGitHubPRs.ts to call the IPC handler first, then update the in-memory store Co-Authored-By: Claude Opus 4.5 <[email protected]>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if (existingState?.result) { | ||
| usePRReviewStore.getState().setPRReviewResult( | ||
| projectId, | ||
| { ...existingState.result, hasPostedFindings: true }, | ||
| { preserveNewCommitsCheck: true } | ||
| ); | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
Fixes several issues identified in the follow-up PR review: 1. Race condition with prNumber: onMarkReviewPosted callback now accepts prNumber as a parameter instead of relying on closure state, preventing wrong PR updates when user switches PRs during async operations. 2. Silent failure handling: handlePostBlockedStatus now checks the return value of onPostComment and only marks review as posted on success. 3. Missing postedAt timestamp: markReviewPosted now includes postedAt timestamp in the store update for consistency with disk state. 4. Store not updated when result not loaded: If the review result hasn't been loaded yet (race condition), markReviewPosted now reloads it from disk after persistence to ensure the UI reflects the correct state. Co-Authored-By: Claude Opus 4.5 <[email protected]>
github-actions
bot
added
size/M
Update the mock type to return Promise<boolean> instead of void | Promise<void> to match the updated onPostComment interface that now returns success status. Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
apps/frontend/src/renderer/components/github-prs/components/PRDetail.tsx
Show resolved
Hide resolved
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🔴 Follow-up Review: Blocked
🔴 Blocked - 5 CI check(s) failing. Fix CI before merge.
Resolution Status
- ✅ Resolved: 1 previous findings addressed
- ❌ Unresolved: 0 previous findings remain
- 🆕 New Issues: 3 new findings in recent changes
Verdict
BLOCKED due to 5 failing CI checks (CI Complete, test-frontend on ubuntu/windows/macos, CodeQL). The PR cannot be merged until CI passes. From a code perspective: the previous finding (NEW-001 - disk persistence) has been RESOLVED with a well-implemented solution. 3 new LOW-severity suggestions were identified but none are blocking. However, CI failures take precedence - fix the failing frontend tests and CodeQL issues before this PR can proceed.
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, comment-analyzer
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (3 selected of 3 total)
🔵 [QUAL-001] [LOW] Missing validation of prNumber parameter in markReviewPosted handler
📁 apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts:1783
The IPC handler accepts a prNumber parameter from the renderer process but does not validate that it is a positive integer before using it in file path construction. While mitigated by Electron security model and path normalization, input validation is a defense-in-depth practice.
Suggested fix:
Add validation: if (typeof prNumber !== 'number' || !Number.isInteger(prNumber) || prNumber <= 0) { return false; }
🔵 [QUAL-002] [LOW] Missing try-catch error handling in markReviewPosted
📁 apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts:502
The markReviewPosted function does not wrap its body in try-catch unlike other similar methods in the hook (postComment, mergePR). While PRDetail.tsx does catch errors, consistency with codebase patterns would be better.
Suggested fix:
Wrap function body in try-catch and call setError on failure for consistency with other hook methods
🔵 [QUAL-003] [LOW] Timestamp mismatch between in-memory store and persisted file
📁 apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts:510
After IPC call succeeds, the code creates a new timestamp locally for the in-memory store, but the IPC handler also creates its own timestamp. This results in slightly different postedAt values between memory and disk.
Suggested fix:
Either have IPC handler return the timestamp it used, or reload from disk after successful mark
This review was generated by Auto Claude.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
apps/frontend/src/renderer/components/github-prs/components/PRDetail.tsx
Show resolved
Hide resolved
| ${t('prReview.blockedStatusMessageFooter')}`; | ||
|
|
||
| await Promise.resolve(onPostComment(blockedStatusMessage)); |
There was a problem hiding this comment.
Approve handler silently fails on post error
Low Severity
The handleApprove function also doesn't check the success return value from onPostComment after the type change from void to Promise<boolean>. When posting the approval comment fails and returns false, the function silently completes with no error feedback to the user. The button loading state ends but the user has no indication that the approval wasn't actually posted to GitHub. This is the same incomplete pattern as handlePostCleanReview, though with less severe user impact since there's no false success message displayed.
The mockOnPostComment.mockResolvedValue() was passing undefined instead of boolean, causing TypeScript type check failures in CI. PostCommentFn returns Promise<boolean>, so the mock must also return a boolean value.
| ); | ||
| } | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…ndler Remove separate fs.existsSync() check before fs.readFileSync() to prevent time-of-check to time-of-use (TOCTOU) race condition flagged by CodeQL. Instead, let readFileSync throw ENOENT if file doesn't exist and handle it in the catch block with specific error code checking.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🟠 Follow-up Review: Needs Revision
🟠 Needs revision - 3 unresolved finding(s) from previous review.
Resolution Status
- ✅ Resolved: 0 previous findings addressed
- ❌ Unresolved: 3 previous findings remain
- 🆕 New Issues: 2 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 0 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 3 findings verified as genuine issues
- 👤 Needs Human Review: 0 findings require manual verification
🚨 Blocking Issues
- test: Test helper missing onMarkReviewPosted prop and test coverage
- quality: [FROM COMMENTS] Cursor Bugbot issues require investigation
Verdict
CI Status: 1 check pending - must wait for completion before merge.
Findings Summary:
- 3 previous LOW severity findings confirmed valid but unresolved (QUAL-001, QUAL-002, QUAL-003) - these are code quality/consistency issues, not bugs
- 1 new MEDIUM severity finding (NEW-002) - missing test coverage for onMarkReviewPosted
- Cursor Bugbot reported 2 issues requiring investigation
Why NEEDS_REVISION (not BLOCKED):
- CI is pending, not failing
- All unresolved findings are LOW severity consistency issues (validation pattern, try-catch pattern, timestamp precision)
- The MEDIUM finding (NEW-002) is test coverage, not a runtime bug
- No CRITICAL/HIGH severity issues
Recommended Actions:
- Wait for CI to complete
- Investigate Cursor Bugbot's 2 reported issues
- Consider adding test coverage for onMarkReviewPosted (medium priority)
- Low-priority: address consistency issues in future commits
Note: The LOW severity findings (QUAL-001/002/003) are valid but represent code style consistency rather than functional bugs. They should not block merge once CI passes, but are worth addressing for maintainability.
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, comment-analyzer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (5 selected of 5 total)
🔵 [QUAL-001] [LOW] [UNRESOLVED] Missing validation of prNumber parameter in markReviewPosted handler
📁 apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts:1783
The IPC handler accepts a prNumber parameter from the renderer process but does not validate that it is a positive integer before using it in file path construction. While mitigated by Electron security model and path normalization, input validation is a defense-in-depth practice.
Resolution note: async (_, projectId: string, prNumber: number): Promise => {
const reviewPath = path.join(getGitHubDir(project), "pr", review_${prNumber}.json);
Suggested fix:
Add validation: if (typeof prNumber !== 'number' || !Number.isInteger(prNumber) || prNumber <= 0) { return false; }
🔵 [QUAL-002] [LOW] [UNRESOLVED] Missing try-catch error handling in markReviewPosted
📁 apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts:502
The markReviewPosted function does not wrap its body in try-catch unlike other similar methods in the hook (postComment, mergePR). While PRDetail.tsx does catch errors, consistency with codebase patterns would be better.
Resolution note: const markReviewPosted = useCallback(
async (prNumber: number): Promise => {
if (!projectId) return;
const success = await window.electronAPI.github.markReviewPosted(projectId, prNumber);
// No try-catch wrapper
Suggested fix:
Wrap function body in try-catch and call setError on failure for consistency with other hook methods
🔵 [QUAL-003] [LOW] [UNRESOLVED] Timestamp mismatch between in-memory store and persisted file
📁 apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts:510
After IPC call succeeds, the code creates a new timestamp locally for the in-memory store, but the IPC handler also creates its own timestamp. This results in slightly different postedAt values between memory and disk.
Resolution note: // IPC handler: data.posted_at = new Date().toISOString();
// Renderer: const postedAt = new Date().toISOString();
Suggested fix:
Either have IPC handler return the timestamp it used, or reload from disk after successful mark
🟡 [NEW-002] [MEDIUM] Test helper missing onMarkReviewPosted prop and test coverage
📁 apps/frontend/src/renderer/components/github-prs/components/__tests__/PRDetail.integration.test.tsx:97
The renderPRDetail helper function does not include the new optional onMarkReviewPosted prop, and no test cases verify that markReviewPosted is called when appropriate after successful comment posting in the blocked status flow.
Suggested fix:
Add a mock for onMarkReviewPosted and include it in the renderPRDetail helper. Add test cases to verify onMarkReviewPosted is called after successful onPostComment calls.
🟡 [CMT-FINDING-001] [MEDIUM] [FROM COMMENTS] Cursor Bugbot issues require investigation
📁 unknown:0
Cursor Bugbot flagged 2 potential issues in this PR, but the specific details were not included in the review context. These issues need to be retrieved from the PR's inline comments or Cursor dashboard before confident approval.
Suggested fix:
Locate and review the 2 issues reported by Cursor Bugbot in PR inline comments or Cursor dashboard
This review was generated by Auto Claude.
Resolve merge conflict in ipc.ts by keeping both new IPC channels: - GITHUB_PR_MARK_REVIEW_POSTED (from this branch) - GITHUB_PR_UPDATE_BRANCH (from develop) Fix second occurrence of mockOnPostComment.mockResolvedValue(undefined) type error in PRDetail integration tests.
| setBlockedStatusError(null); | ||
| // Update the store to mark review as posted so PR list reflects the change | ||
| // Pass prNumber explicitly to avoid race conditions with PR selection changes | ||
| await onMarkReviewPosted?.(currentPr); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Resolve merge conflicts: - ipc.ts: keep both GITHUB_PR_MARK_REVIEW_POSTED and GITHUB_PR_UPDATE_BRANCH - PRDetail.integration.test.tsx: use mockResolvedValue(true) for correct typing
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🟠 Follow-up Review: Needs Revision
🟠 Needs revision - 4 unresolved finding(s) from previous review.
Resolution Status
- ✅ Resolved: 1 previous findings addressed
- ❌ Unresolved: 4 previous findings remain
- 🆕 New Issues: 1 new findings in recent changes
Finding Validation
- 🔍 Dismissed as False Positives: 1 findings were re-investigated and found to be incorrect
- ✓ Confirmed Valid: 3 findings verified as genuine issues
- 👤 Needs Human Review: 0 findings require manual verification
🚨 Blocking Issues
- test: [UNRESOLVED] Test helper missing onMarkReviewPosted prop and test coverage
- quality: [UNRESOLVED] [FROM COMMENTS] Cursor Bugbot issues require investigation
Verdict
CI Status: 1 check pending - must wait for completion before merge.
Validation Results: 1 finding dismissed as false positive (QUAL-002 - early return pattern is intentional), reducing blocking issues.
Remaining Issues:
- 1 MEDIUM severity (NEW-002): Missing test coverage for onMarkReviewPosted callback - tests should verify the new functionality
- 3 LOW severity confirmed valid: QUAL-001 (prNumber validation inconsistency), QUAL-003 (timestamp mismatch - minimal impact), NEW-004 (hardcoded error message)
- 1 cannot verify (CMT-FINDING-001): Cursor Bugbot issues - no details provided
Recommendation: Address the MEDIUM severity test coverage gap (NEW-002) before merge. The LOW severity issues are code quality improvements that could be addressed in this PR or a follow-up. Wait for CI to complete.
Review Process
Agents invoked: resolution-verifier, new-code-reviewer, comment-analyzer, finding-validator
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (4 selected of 5 total)
🔵 [QUAL-001] [LOW] [UNRESOLVED] [UNRESOLVED] Missing validation of prNumber parameter in markReviewPosted handler
📁 apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts:1783
The IPC handler accepts a prNumber parameter from the renderer process but does not validate that it is a positive integer before using it in file path construction. While mitigated by Electron security model and path normalization, input validation is a defense-in-depth practice.
Resolution note: async (_, projectId: string, prNumber: number): Promise => {
const reviewPath = path.join(getGitHubDir(project), "pr", review_${prNumber}.json);
Resolution note: async (_, projectId: string, prNumber: number): Promise => {
debugLog("markReviewPosted handler called", { projectId, prNumber });
const result = await withProjectOrNull(projectId, async (project) => {
try {
const reviewPath = path.join(getGitHubDir(project), "pr", review_${prNumber}.json);
Suggested fix:
Add validation: if (typeof prNumber !== 'number' || !Number.isInteger(prNumber) || prNumber <= 0) { return false; }
🔵 [QUAL-003] [LOW] [UNRESOLVED] [UNRESOLVED] Timestamp mismatch between in-memory store and persisted file
📁 apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts:510
After IPC call succeeds, the code creates a new timestamp locally for the in-memory store, but the IPC handler also creates its own timestamp. This results in slightly different postedAt values between memory and disk.
Resolution note: // IPC handler: data.posted_at = new Date().toISOString();
// Renderer: const postedAt = new Date().toISOString();
Resolution note: // pr-handlers.ts:1797-1798
data.posted_at = new Date().toISOString();
// useGitHubPRs.ts:570
const postedAt = new Date().toISOString();
Suggested fix:
Either have IPC handler return the timestamp it used, or reload from disk after successful mark
🟡 [CMT-FINDING-001] [MEDIUM] [UNRESOLVED] [FROM COMMENTS] Cursor Bugbot issues require investigation
📁 unknown:0
Cursor Bugbot flagged 2 potential issues in this PR, but the specific details were not included in the review context. These issues need to be retrieved from the PR's inline comments or Cursor dashboard before confident approval.
Resolution note: N/A - File listed as 'unknown:0' with no code location
Suggested fix:
Locate and review the 2 issues reported by Cursor Bugbot in PR inline comments or Cursor dashboard
🔵 [NEW-004] [LOW] Hardcoded error message instead of using i18n translation
📁 apps/frontend/src/renderer/components/github-prs/components/PRDetail.tsx:795
The error message 'Failed to post comment' is hardcoded in English instead of using the i18n translation system. Other error messages and UI text in this file use translation keys (e.g., t('prReview.blockedStatusMessageTitle')). This inconsistency could cause issues for non-English users.
Suggested fix:
Use a translation key: setBlockedStatusError(t('prReview.failedToPostComment')) and add the key to translation files.
This review was generated by Auto Claude.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| setBlockedStatusError(null); | ||
| // Update the store to mark review as posted so PR list reflects the change | ||
| // Pass prNumber explicitly to avoid race conditions with PR selection changes | ||
| await onMarkReviewPosted?.(currentPr); |
There was a problem hiding this comment.
Store update skipped when user switches PRs during posting
Medium Severity
The onMarkReviewPosted call is placed inside the pr.number === currentPr check, which means if the user switches to a different PR while the comment is being posted, the store and disk are never updated with has_posted_findings = true - even though the comment was successfully posted to GitHub. The code comment explicitly mentions "Pass prNumber explicitly to avoid race conditions with PR selection changes," but the guard prevents this from working. The result is stale data: the PR list shows "not posted" even after a successful post, and this persists through app restarts since the disk file isn't updated.
AndyMik90
deleted the
auto-claude/125-fix-pr-list-update-on-post-status-click
branch
Fix bug where "Post Status" button on PR detail doesn't update the PR list status display. The
handlePostBlockedStatusfunction in PRDetail.tsx posts to GitHub successfully but does not update the PR review store, causing the PR list to show stale data.Working comparison:
handlePostReviewcallsonPostReviewwhich correctly updates the store after posting.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.