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 bc3a694215..b389d7b70a 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 f7ec9bf7c8..6e409c7e81 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 d095bfbfd3..1b6f406d41 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 64dcb5b61e..2edfab2009 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 ba1ce43357..8f3c7d2739 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 8346bf1bce..c2e6ec1ab7 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 5db98a67a0..1479a5bb6d 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 45181c9823..7bed40cb74 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)