From 3085e392d203e974f6bb3514afa77f3adf9cb7a5 Mon Sep 17 00:00:00 2001 From: Andy <119136210+AndyMik90@users.noreply.github.com> Date: Sat, 17 Jan 2026 20:51:48 +0100 Subject: [PATCH] Fix PR List Update on Post Status Click (#1207) * auto-claude: subtask-1-1 - Add markReviewPosted function to useGitHubPRs 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 * fix: persist hasPostedFindings flag to disk in markReviewPosted 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 * fix: address follow-up review findings for markReviewPosted 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 * fix: update PostCommentFn type in PRDetail tests to match new signature Update the mock type to return Promise instead of void | Promise to match the updated onPostComment interface that now returns success status. Co-Authored-By: Claude Opus 4.5 * fix: correct mock return value type in PRDetail integration test The mockOnPostComment.mockResolvedValue() was passing undefined instead of boolean, causing TypeScript type check failures in CI. PostCommentFn returns Promise, so the mock must also return a boolean value. * fix(security): eliminate TOCTOU race condition in markReviewPosted handler 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. * chore: merge develop and fix additional test type error 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. --------- Co-authored-by: Test User Co-authored-by: Claude Opus 4.5 --- .../main/ipc-handlers/github/pr-handlers.ts | 41 +++++++++++++++++++ .../src/preload/api/modules/github-api.ts | 4 ++ .../components/github-prs/GitHubPRs.tsx | 11 ++++- .../github-prs/components/PRDetail.tsx | 15 +++++-- .../__tests__/PRDetail.integration.test.tsx | 8 ++-- .../github-prs/hooks/useGitHubPRs.ts | 37 +++++++++++++++++ .../frontend/src/renderer/lib/browser-mock.ts | 1 + apps/frontend/src/shared/constants/ipc.ts | 1 + 8 files changed, 108 insertions(+), 10 deletions(-) diff --git a/apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts b/apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts index bc3a6942..b389d7b7 100644 --- a/apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts +++ b/apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts @@ -1777,6 +1777,47 @@ export function registerPRHandlers(getMainWindow: () => BrowserWindow | null): v } ); + // Mark review as posted (persists has_posted_findings to disk) + ipcMain.handle( + IPC_CHANNELS.GITHUB_PR_MARK_REVIEW_POSTED, + 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`); + + // Read file directly without separate existence check to avoid TOCTOU race condition + // If file doesn't exist, readFileSync will throw ENOENT which we handle below + const rawData = fs.readFileSync(reviewPath, "utf-8"); + // Sanitize data before parsing (review may contain data from GitHub API) + const sanitizedData = sanitizeNetworkData(rawData); + const data = JSON.parse(sanitizedData); + + // Mark as posted + data.has_posted_findings = true; + data.posted_at = new Date().toISOString(); + + fs.writeFileSync(reviewPath, JSON.stringify(data, null, 2), "utf-8"); + debugLog("Marked review as posted", { prNumber }); + + return true; + } catch (error) { + // Handle file not found (ENOENT) separately for clearer logging + if (error instanceof Error && "code" in error && error.code === "ENOENT") { + debugLog("Review file not found", { prNumber }); + return false; + } + debugLog("Failed to mark review as posted", { + prNumber, + error: error instanceof Error ? error.message : error, + }); + return false; + } + }); + return result ?? false; + } + ); + // Post comment to PR ipcMain.handle( IPC_CHANNELS.GITHUB_PR_POST_COMMENT, diff --git a/apps/frontend/src/preload/api/modules/github-api.ts b/apps/frontend/src/preload/api/modules/github-api.ts index f7ec9bf7..6e409c7e 100644 --- a/apps/frontend/src/preload/api/modules/github-api.ts +++ b/apps/frontend/src/preload/api/modules/github-api.ts @@ -272,6 +272,7 @@ export interface GitHubAPI { postPRComment: (projectId: string, prNumber: number, body: string) => Promise; mergePR: (projectId: string, prNumber: number, mergeMethod?: 'merge' | 'squash' | 'rebase') => Promise; assignPR: (projectId: string, prNumber: number, username: string) => Promise; + markReviewPosted: (projectId: string, prNumber: number) => Promise; getPRReview: (projectId: string, prNumber: number) => Promise; getPRReviewsBatch: (projectId: string, prNumbers: number[]) => Promise>; @@ -678,6 +679,9 @@ export const createGitHubAPI = (): GitHubAPI => ({ assignPR: (projectId: string, prNumber: number, username: string): Promise => invokeIpc(IPC_CHANNELS.GITHUB_PR_ASSIGN, projectId, prNumber, username), + markReviewPosted: (projectId: string, prNumber: number): Promise => + invokeIpc(IPC_CHANNELS.GITHUB_PR_MARK_REVIEW_POSTED, projectId, prNumber), + getPRReview: (projectId: string, prNumber: number): Promise => invokeIpc(IPC_CHANNELS.GITHUB_PR_GET_REVIEW, projectId, prNumber), diff --git a/apps/frontend/src/renderer/components/github-prs/GitHubPRs.tsx b/apps/frontend/src/renderer/components/github-prs/GitHubPRs.tsx index d095bfbf..1b6f406d 100644 --- a/apps/frontend/src/renderer/components/github-prs/GitHubPRs.tsx +++ b/apps/frontend/src/renderer/components/github-prs/GitHubPRs.tsx @@ -77,6 +77,7 @@ export function GitHubPRs({ onOpenSettings, isActive = false }: GitHubPRsProps) postComment, mergePR, assignPR, + markReviewPosted, refresh, loadMore, isConnected, @@ -140,10 +141,11 @@ export function GitHubPRs({ onOpenSettings, isActive = false }: GitHubPRsProps) ); const handlePostComment = useCallback( - async (body: string) => { + async (body: string): Promise => { if (selectedPRNumber) { - await postComment(selectedPRNumber, body); + return await postComment(selectedPRNumber, body); } + return false; }, [selectedPRNumber, postComment] ); @@ -173,6 +175,10 @@ export function GitHubPRs({ onOpenSettings, isActive = false }: GitHubPRsProps) return null; }, [selectedProjectId, selectedPRNumber]); + const handleMarkReviewPosted = useCallback(async (prNumber: number) => { + await markReviewPosted(prNumber); + }, [markReviewPosted]); + // Not connected state if (!isConnected) { return ; @@ -259,6 +265,7 @@ export function GitHubPRs({ onOpenSettings, isActive = false }: GitHubPRsProps) onMergePR={handleMergePR} onAssignPR={handleAssignPR} onGetLogs={handleGetLogs} + onMarkReviewPosted={handleMarkReviewPosted} /> ) : ( diff --git a/apps/frontend/src/renderer/components/github-prs/components/PRDetail.tsx b/apps/frontend/src/renderer/components/github-prs/components/PRDetail.tsx index 64dcb5b6..2edfab20 100644 --- a/apps/frontend/src/renderer/components/github-prs/components/PRDetail.tsx +++ b/apps/frontend/src/renderer/components/github-prs/components/PRDetail.tsx @@ -52,10 +52,11 @@ interface PRDetailProps { onCheckNewCommits: () => Promise; onCancelReview: () => void; onPostReview: (selectedFindingIds?: string[], options?: { forceApprove?: boolean }) => Promise; - onPostComment: (body: string) => void; + onPostComment: (body: string) => Promise; onMergePR: (mergeMethod?: 'merge' | 'squash' | 'rebase') => void; onAssignPR: (username: string) => void; onGetLogs: () => Promise; + onMarkReviewPosted?: (prNumber: number) => Promise; } function getStatusColor(status: PRReviewResult['overallStatus']): string { @@ -89,6 +90,7 @@ export function PRDetail({ onMergePR, onAssignPR: _onAssignPR, onGetLogs, + onMarkReviewPosted, }: PRDetailProps) { const { t } = useTranslation('common'); // Selection state for findings @@ -780,12 +782,17 @@ ${reviewResult.summary} ${t('prReview.blockedStatusMessageFooter')}`; - await Promise.resolve(onPostComment(blockedStatusMessage)); + const success = await onPostComment(blockedStatusMessage); - // Only mark as posted on success if PR hasn't changed - if (pr.number === currentPr) { + // Only mark as posted on success if PR hasn't changed AND comment was posted successfully + if (success && pr.number === currentPr) { setBlockedStatusPosted(true); 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); + } else if (!success && pr.number === currentPr) { + setBlockedStatusError('Failed to post comment'); } } catch (err) { console.error('Failed to post blocked status comment:', err); diff --git a/apps/frontend/src/renderer/components/github-prs/components/__tests__/PRDetail.integration.test.tsx b/apps/frontend/src/renderer/components/github-prs/components/__tests__/PRDetail.integration.test.tsx index ba1ce433..8f3c7d27 100644 --- a/apps/frontend/src/renderer/components/github-prs/components/__tests__/PRDetail.integration.test.tsx +++ b/apps/frontend/src/renderer/components/github-prs/components/__tests__/PRDetail.integration.test.tsx @@ -14,8 +14,8 @@ import type { PRData, PRReviewResult } from '../../hooks/useGitHubPRs'; import type { NewCommitsCheck } from '../../../../../preload/api/modules/github-api'; // Mock window.electronAPI -type PostCommentFn = (body: string) => void | Promise; -const mockOnPostComment = vi.fn(); +type PostCommentFn = (body: string) => Promise; +const mockOnPostComment = vi.fn().mockResolvedValue(true); const mockOnPostReview = vi.fn(); const mockOnRunReview = vi.fn(); const mockOnRunFollowupReview = vi.fn(); @@ -128,7 +128,7 @@ describe('PRDetail - Clean Review State Reset Integration', () => { newCommitCount: 0 }); // Resolve successfully by default - mockOnPostComment.mockResolvedValue(undefined); + mockOnPostComment.mockResolvedValue(true); }); it('should reset cleanReviewPosted state when pr.number changes', async () => { @@ -432,7 +432,7 @@ describe('PRDetail - Follow-up Review Trigger Integration', () => { hasCommitsAfterPosting: false, newCommitCount: 0 }); - mockOnPostComment.mockResolvedValue(undefined); + mockOnPostComment.mockResolvedValue(true); }); it('should display "Ready for Follow-up" status when new commits exist after posting', async () => { diff --git a/apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts b/apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts index 8346bf1b..c2e6ec1a 100644 --- a/apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts +++ b/apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts @@ -52,6 +52,7 @@ interface UseGitHubPRsResult { postComment: (prNumber: number, body: string) => Promise; mergePR: (prNumber: number, mergeMethod?: "merge" | "squash" | "rebase") => Promise; assignPR: (prNumber: number, username: string) => Promise; + markReviewPosted: (prNumber: number) => Promise; getReviewStateForPR: (prNumber: number) => { isReviewing: boolean; startedAt: string | null; @@ -557,6 +558,41 @@ export function useGitHubPRs( [projectId, fetchPRs] ); + const markReviewPosted = useCallback( + async (prNumber: number): Promise => { + if (!projectId) return; + + // Persist to disk first + const success = await window.electronAPI.github.markReviewPosted(projectId, prNumber); + if (!success) return; + + // Get the current timestamp for consistent update + const postedAt = new Date().toISOString(); + + // Update the in-memory store + const existingState = getPRReviewState(projectId, prNumber); + if (existingState?.result) { + // If we have the result loaded, update it with hasPostedFindings and postedAt + usePRReviewStore.getState().setPRReviewResult( + projectId, + { ...existingState.result, hasPostedFindings: true, postedAt }, + { preserveNewCommitsCheck: true } + ); + } else { + // If result not loaded yet (race condition), reload from disk to get updated state + const result = await window.electronAPI.github.getPRReview(projectId, prNumber); + if (result) { + usePRReviewStore.getState().setPRReviewResult( + projectId, + result, + { preserveNewCommitsCheck: true } + ); + } + } + }, + [projectId, getPRReviewState] + ); + return { prs, isLoading, @@ -585,6 +621,7 @@ export function useGitHubPRs( postComment, mergePR, assignPR, + markReviewPosted, getReviewStateForPR, }; } diff --git a/apps/frontend/src/renderer/lib/browser-mock.ts b/apps/frontend/src/renderer/lib/browser-mock.ts index 5db98a67..1479a5bb 100644 --- a/apps/frontend/src/renderer/lib/browser-mock.ts +++ b/apps/frontend/src/renderer/lib/browser-mock.ts @@ -204,6 +204,7 @@ const browserMockAPI: ElectronAPI = { postPRComment: async () => true, mergePR: async () => true, assignPR: async () => true, + markReviewPosted: async () => true, getPRReview: async () => null, getPRReviewsBatch: async () => ({}), deletePRReview: async () => true, diff --git a/apps/frontend/src/shared/constants/ipc.ts b/apps/frontend/src/shared/constants/ipc.ts index 45181c98..7bed40cb 100644 --- a/apps/frontend/src/shared/constants/ipc.ts +++ b/apps/frontend/src/shared/constants/ipc.ts @@ -374,6 +374,7 @@ export const IPC_CHANNELS = { GITHUB_PR_FOLLOWUP_REVIEW: 'github:pr:followupReview', GITHUB_PR_CHECK_NEW_COMMITS: 'github:pr:checkNewCommits', GITHUB_PR_CHECK_MERGE_READINESS: 'github:pr:checkMergeReadiness', + GITHUB_PR_MARK_REVIEW_POSTED: 'github:pr:markReviewPosted', GITHUB_PR_UPDATE_BRANCH: 'github:pr:updateBranch', // GitHub PR Review events (main -> renderer)