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 2a9e0a1f..64dcb5b6 100644 --- a/apps/frontend/src/renderer/components/github-prs/components/PRDetail.tsx +++ b/apps/frontend/src/renderer/components/github-prs/components/PRDetail.tsx @@ -172,30 +172,48 @@ export function PRDetail({ return; } + // Check for new commits if we have ANY successful review with a commit SHA + // This includes follow-up reviews that resolved all issues (no new findings) + // New commits = new code that needs to be reviewed, regardless of posting status + if (!reviewResult?.success || !reviewResult.reviewedCommitSha) { + return; + } + + // Skip if we already have a fresh newCommitsCheck from initialNewCommitsCheck (store) + // that matches the current review's commit SHA. This prevents redundant API calls + // when the useGitHubPRs hook has already checked for new commits on PR selection. + // The `lastReviewedCommit` field indicates which commit SHA the check was performed against. + if (newCommitsCheck?.lastReviewedCommit === reviewResult.reviewedCommitSha) { + return; + } + + // Additional guard: if we have any newCommitsCheck result but it lacks lastReviewedCommit, + // skip to prevent infinite loops. This handles edge cases where the API returns + // a result without the tracking field. + if (newCommitsCheck && !newCommitsCheck.lastReviewedCommit) { + return; + } + // Cancel any pending check if (checkNewCommitsAbortRef.current) { checkNewCommitsAbortRef.current.abort(); } checkNewCommitsAbortRef.current = new AbortController(); - // Check for new commits if we have ANY successful review with a commit SHA - // This includes follow-up reviews that resolved all issues (no new findings) - // New commits = new code that needs to be reviewed, regardless of posting status - if (reviewResult?.success && reviewResult.reviewedCommitSha) { - isCheckingNewCommitsRef.current = true; - try { - const result = await onCheckNewCommits(); - // Only update state if not aborted - if (!checkNewCommitsAbortRef.current?.signal.aborted) { - setNewCommitsCheck(result); - } - } finally { - if (!checkNewCommitsAbortRef.current?.signal.aborted) { - isCheckingNewCommitsRef.current = false; - } + isCheckingNewCommitsRef.current = true; + try { + const result = await onCheckNewCommits(); + // Only update state if not aborted + if (!checkNewCommitsAbortRef.current?.signal.aborted) { + setNewCommitsCheck(result); } + } finally { + // Always reset the checking ref to allow future checks. + // The abort only determines whether to update STATE, not whether + // the operation tracking should be reset. + isCheckingNewCommitsRef.current = false; } - }, [reviewResult, onCheckNewCommits]); + }, [reviewResult, onCheckNewCommits, newCommitsCheck]); useEffect(() => { checkForNewCommits(); 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 5c47e070..ba1ce433 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 @@ -11,6 +11,7 @@ import { I18nextProvider } from 'react-i18next'; import i18n from '../../../../../shared/i18n'; import { PRDetail } from '../PRDetail'; 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; @@ -345,3 +346,481 @@ describe('PRDetail - Clean Review State Reset Integration', () => { unmount(); }); }); + +/** + * Integration tests for PRDetail follow-up review trigger + * Tests that follow-up review is correctly triggered when new commits are detected + * after findings have been posted to GitHub + */ +describe('PRDetail - Follow-up Review Trigger Integration', () => { + const mockProjectId = 'test-project-id'; + + // Helper function to create a mock review result with posted findings + function createMockPostedReviewResult(overrides: Partial = {}): PRReviewResult { + return { + prNumber: 123, + repo: 'test/repo', + success: true, + overallStatus: 'request_changes', + summary: 'Found issues that need attention.', + findings: [ + { + id: 'finding-1', + severity: 'high', + category: 'security', + title: 'Security Issue', + file: 'src/test.ts', + line: 10, + description: 'High severity security issue', + fixable: true + } + ], + reviewedAt: '2024-01-01T00:00:00Z', + reviewedCommitSha: 'abc123', + postedFindingIds: ['finding-1'], + hasPostedFindings: true, + postedAt: '2024-01-01T01:00:00Z', + ...overrides + }; + } + + // Helper function to render PRDetail with all props for follow-up review tests + function renderPRDetailForFollowup(overrides: { + pr?: PRData; + reviewResult?: PRReviewResult; + initialNewCommitsCheck?: NewCommitsCheck | null; + isReviewing?: boolean; + onRunFollowupReview?: () => void; + } = {}) { + const defaultPR = createMockPR({ number: 123 }); + const defaultReviewResult = createMockPostedReviewResult(); + const defaultNewCommitsCheck = overrides.initialNewCommitsCheck ?? null; + const onRunFollowupReviewMock = overrides.onRunFollowupReview ?? mockOnRunFollowupReview; + + return render( + + + + ); + } + + beforeEach(() => { + vi.clearAllMocks(); + + // Setup default mock return values + mockOnGetLogs.mockResolvedValue(null); + mockOnCheckNewCommits.mockResolvedValue({ + hasNewCommits: false, + hasCommitsAfterPosting: false, + newCommitCount: 0 + }); + mockOnPostComment.mockResolvedValue(undefined); + }); + + it('should display "Ready for Follow-up" status when new commits exist after posting', async () => { + const reviewResult = createMockPostedReviewResult(); + + const { unmount } = renderPRDetailForFollowup({ + reviewResult, + initialNewCommitsCheck: { + hasNewCommits: true, + newCommitCount: 2, + hasCommitsAfterPosting: true, + hasOverlapWithFindings: true + } + }); + + // Wait for the status tree to render with "Ready for Follow-up" status + await waitFor(() => { + expect(screen.getByText(/ready for follow-up/i)).toBeInTheDocument(); + }); + + unmount(); + }); + + it('should show "Run Follow-up" button when new commits overlap with findings', async () => { + const reviewResult = createMockPostedReviewResult(); + + const { unmount } = renderPRDetailForFollowup({ + reviewResult, + initialNewCommitsCheck: { + hasNewCommits: true, + newCommitCount: 3, + hasCommitsAfterPosting: true, + hasOverlapWithFindings: true + } + }); + + // Wait for the "Run Follow-up" button to appear + await waitFor(() => { + expect(screen.getByRole('button', { name: /run follow-up/i })).toBeInTheDocument(); + }); + + unmount(); + }); + + it('should call onRunFollowupReview when "Run Follow-up" button is clicked', async () => { + const reviewResult = createMockPostedReviewResult(); + + // Mock checkNewCommits to return consistent result + mockOnCheckNewCommits.mockResolvedValue({ + hasNewCommits: true, + newCommitCount: 2, + hasCommitsAfterPosting: true, + hasOverlapWithFindings: true, + lastReviewedCommit: 'abc123' // Match reviewedCommitSha to prevent additional API call + }); + + const { unmount } = renderPRDetailForFollowup({ + reviewResult, + initialNewCommitsCheck: { + hasNewCommits: true, + newCommitCount: 2, + hasCommitsAfterPosting: true, + hasOverlapWithFindings: true, + lastReviewedCommit: 'abc123' // Prevents redundant checkNewCommits call + } + }); + + // Wait for the "Run Follow-up" button to appear + const followupButton = await screen.findByRole('button', { name: /run follow-up/i }); + expect(followupButton).toBeInTheDocument(); + + // Click the button + fireEvent.click(followupButton); + + // Verify the callback was called + expect(mockOnRunFollowupReview).toHaveBeenCalledTimes(1); + + unmount(); + }); + + it('should NOT show follow-up prompt when hasCommitsAfterPosting is false', async () => { + const reviewResult = createMockPostedReviewResult(); + + const { unmount } = renderPRDetailForFollowup({ + reviewResult, + initialNewCommitsCheck: { + hasNewCommits: true, + newCommitCount: 2, + hasCommitsAfterPosting: false // New commits exist but before posting + } + }); + + // The "Run Follow-up" button should NOT be visible + await waitFor(() => { + expect(screen.queryByRole('button', { name: /run follow-up/i })).not.toBeInTheDocument(); + }); + + // Should show "Waiting for Changes" instead since blockers are posted but no new commits after posting + await waitFor(() => { + expect(screen.getByText(/waiting for changes/i)).toBeInTheDocument(); + }); + + unmount(); + }); + + it('should NOT show follow-up prompt when findings have not been posted', async () => { + // Review with findings but NOT posted + const reviewResult: PRReviewResult = { + prNumber: 123, + repo: 'test/repo', + success: true, + overallStatus: 'request_changes', + summary: 'Found issues.', + findings: [ + { + id: 'finding-1', + severity: 'high', + category: 'security', + title: 'Security Issue', + file: 'src/test.ts', + line: 10, + description: 'High severity issue', + fixable: true + } + ], + reviewedAt: '2024-01-01T00:00:00Z', + reviewedCommitSha: 'abc123', + hasPostedFindings: false, // NOT posted + postedFindingIds: [] + }; + + const { unmount } = renderPRDetailForFollowup({ + reviewResult, + initialNewCommitsCheck: { + hasNewCommits: true, + newCommitCount: 2, + hasCommitsAfterPosting: true + } + }); + + // The "Run Follow-up" button should NOT be visible since findings weren't posted + await waitFor(() => { + expect(screen.queryByRole('button', { name: /run follow-up/i })).not.toBeInTheDocument(); + }); + + // Should show "Needs Attention" since there are unposted blockers + await waitFor(() => { + expect(screen.getByText(/needs attention/i)).toBeInTheDocument(); + }); + + unmount(); + }); + + it('should update follow-up status when newCommitsCheck changes via props', async () => { + const reviewResult = createMockPostedReviewResult(); + + // Mock checkNewCommits to return no new commits initially + mockOnCheckNewCommits.mockResolvedValue({ + hasNewCommits: false, + newCommitCount: 0, + hasCommitsAfterPosting: false, + lastReviewedCommit: 'abc123' + }); + + // Start without new commits + const { rerender, unmount } = renderPRDetailForFollowup({ + reviewResult, + initialNewCommitsCheck: { + hasNewCommits: false, + newCommitCount: 0, + hasCommitsAfterPosting: false, + lastReviewedCommit: 'abc123' + } + }); + + // Should show "Waiting for Changes" initially + await waitFor(() => { + expect(screen.getByText(/waiting for changes/i)).toBeInTheDocument(); + }); + + // No follow-up button initially + expect(screen.queryByRole('button', { name: /run follow-up/i })).not.toBeInTheDocument(); + + // Update mock before rerender + mockOnCheckNewCommits.mockResolvedValue({ + hasNewCommits: true, + newCommitCount: 3, + hasCommitsAfterPosting: true, + hasOverlapWithFindings: true, + lastReviewedCommit: 'abc123' + }); + + // Rerender with new commits detected + rerender( + + + + ); + + // Now should show "Ready for Follow-up" status + await waitFor(() => { + expect(screen.getByText(/ready for follow-up/i)).toBeInTheDocument(); + }); + + // Follow-up button should now be visible + const followupButton = await screen.findByRole('button', { name: /run follow-up/i }); + expect(followupButton).toBeInTheDocument(); + + unmount(); + }); + + it('should show "Verify" option when new commits have no overlap with findings', async () => { + const reviewResult = createMockPostedReviewResult(); + + // Mock checkNewCommits to return result with no overlap + mockOnCheckNewCommits.mockResolvedValue({ + hasNewCommits: true, + newCommitCount: 2, + hasCommitsAfterPosting: true, + hasOverlapWithFindings: false, + lastReviewedCommit: 'abc123' + }); + + const { unmount } = renderPRDetailForFollowup({ + reviewResult, + initialNewCommitsCheck: { + hasNewCommits: true, + newCommitCount: 2, + hasCommitsAfterPosting: true, + hasOverlapWithFindings: false, // No overlap - safe commits + lastReviewedCommit: 'abc123' + } + }); + + // Should show "Verify" button for optional follow-up (translation key: verifyAnyway) + const verifyButton = await screen.findByRole('button', { name: /^verify$/i }); + expect(verifyButton).toBeInTheDocument(); + + unmount(); + }); + + it('should call onRunFollowupReview when "Verify" button is clicked', async () => { + const reviewResult = createMockPostedReviewResult(); + + // Mock checkNewCommits to return result with no overlap + mockOnCheckNewCommits.mockResolvedValue({ + hasNewCommits: true, + newCommitCount: 2, + hasCommitsAfterPosting: true, + hasOverlapWithFindings: false, + lastReviewedCommit: 'abc123' + }); + + const { unmount } = renderPRDetailForFollowup({ + reviewResult, + initialNewCommitsCheck: { + hasNewCommits: true, + newCommitCount: 2, + hasCommitsAfterPosting: true, + hasOverlapWithFindings: false, + lastReviewedCommit: 'abc123' + } + }); + + // Wait for the "Verify" button and click it + const verifyButton = await screen.findByRole('button', { name: /^verify$/i }); + fireEvent.click(verifyButton); + + // Verify the callback was called + expect(mockOnRunFollowupReview).toHaveBeenCalledTimes(1); + + unmount(); + }); + + it('should NOT show follow-up prompt during active review', async () => { + const reviewResult = createMockPostedReviewResult(); + + // Mock checkNewCommits - won't be called during active review anyway + mockOnCheckNewCommits.mockResolvedValue({ + hasNewCommits: true, + newCommitCount: 2, + hasCommitsAfterPosting: true, + hasOverlapWithFindings: true, + lastReviewedCommit: 'abc123' + }); + + const { unmount } = renderPRDetailForFollowup({ + reviewResult, + initialNewCommitsCheck: { + hasNewCommits: true, + newCommitCount: 2, + hasCommitsAfterPosting: true, + hasOverlapWithFindings: true, + lastReviewedCommit: 'abc123' + }, + isReviewing: true // Review in progress + }); + + // Should show "AI Review in Progress" status - may appear multiple times (title + badge) + // Use getAllByText to handle multiple occurrences + await waitFor(() => { + const reviewingElements = screen.getAllByText(/ai review in progress/i); + expect(reviewingElements.length).toBeGreaterThan(0); + }); + + // Follow-up button should NOT be visible during active review + expect(screen.queryByRole('button', { name: /run follow-up/i })).not.toBeInTheDocument(); + + unmount(); + }); + + it('should reset follow-up state when PR changes', async () => { + const reviewResult = createMockPostedReviewResult(); + + const { rerender, unmount } = renderPRDetailForFollowup({ + pr: createMockPR({ number: 123 }), + reviewResult, + initialNewCommitsCheck: { + hasNewCommits: true, + newCommitCount: 2, + hasCommitsAfterPosting: true, + hasOverlapWithFindings: true + } + }); + + // Should show follow-up status for PR 123 + await waitFor(() => { + expect(screen.getByText(/ready for follow-up/i)).toBeInTheDocument(); + }); + + // Switch to a different PR that hasn't been reviewed + const newPR = createMockPR({ number: 456 }); + rerender( + + + + ); + + // Should now show "Not Reviewed" for the new PR + await waitFor(() => { + expect(screen.getByText(/not reviewed/i)).toBeInTheDocument(); + }); + + // Follow-up button should not be visible for unreviewed PR + expect(screen.queryByRole('button', { name: /run follow-up/i })).not.toBeInTheDocument(); + + unmount(); + }); +}); diff --git a/apps/frontend/src/renderer/components/github-prs/hooks/__tests__/useGitHubPRs.test.ts b/apps/frontend/src/renderer/components/github-prs/hooks/__tests__/useGitHubPRs.test.ts new file mode 100644 index 00000000..87ba198b --- /dev/null +++ b/apps/frontend/src/renderer/components/github-prs/hooks/__tests__/useGitHubPRs.test.ts @@ -0,0 +1,586 @@ +/** + * @vitest-environment jsdom + */ +/** + * Unit tests for useGitHubPRs hook - selectPR triggering checkNewCommits + * + * Key behavior tested: + * - selectPR calls checkNewCommits when review exists in store + * - selectPR calls checkNewCommits after loading review from disk + * - checkNewCommits is NOT called when review is in progress + * - checkNewCommits is NOT called when no reviewedCommitSha exists + * - Race condition prevention with AbortController + */ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import type { PRReviewResult, NewCommitsCheck } from '../../../../../preload/api/modules/github-api'; + +// Mock factory functions +function createMockReviewResult(overrides: Partial = {}): PRReviewResult { + return { + prNumber: 123, + repo: 'test/repo', + success: true, + findings: [], + summary: 'Test summary', + overallStatus: 'approve', + reviewedAt: '2024-01-01T00:00:00Z', + reviewedCommitSha: 'abc123def456', + ...overrides, + }; +} + +function createMockNewCommitsCheck(overrides: Partial = {}): NewCommitsCheck { + return { + hasNewCommits: false, + newCommitCount: 0, + ...overrides, + }; +} + +/** + * Simulate the selectPR logic flow for testing checkNewCommits behavior. + * This is extracted from useGitHubPRs.ts for unit testing without needing + * to render the full hook in a React environment. + */ +interface SelectPRTestParams { + prNumber: number | null; + projectId: string | null; + existingState: { + result: PRReviewResult | null; + isReviewing: boolean; + newCommitsCheck: NewCommitsCheck | null; + } | null; + diskReviewResult: PRReviewResult | null; + mockCheckNewCommits: (projectId: string, prNumber: number) => Promise; + mockGetPRReview: (projectId: string, prNumber: number) => Promise; + mockSetNewCommitsCheck: (projectId: string, prNumber: number, check: NewCommitsCheck) => void; + mockSetPRReviewResult: (projectId: string, result: PRReviewResult) => void; + abortSignal?: AbortSignal; +} + +interface SelectPRTestResult { + checkNewCommitsCalled: boolean; + checkNewCommitsCallArgs: { projectId: string; prNumber: number } | null; + getPRReviewCalled: boolean; + setNewCommitsCheckCalled: boolean; + setPRReviewResultCalled: boolean; +} + +async function simulateSelectPR(params: SelectPRTestParams): Promise { + const { + prNumber, + projectId, + existingState, + diskReviewResult, + mockCheckNewCommits, + mockGetPRReview, + mockSetNewCommitsCheck, + mockSetPRReviewResult, + abortSignal, + } = params; + + const result: SelectPRTestResult = { + checkNewCommitsCalled: false, + checkNewCommitsCallArgs: null, + getPRReviewCalled: false, + setNewCommitsCheckCalled: false, + setPRReviewResultCalled: false, + }; + + // Early return if no prNumber or deselecting + if (prNumber === null || !projectId) { + return result; + } + + // Helper function to check for new commits (matches useGitHubPRs logic) + const checkNewCommitsForPR = async (reviewedCommitSha: string | undefined) => { + // Skip if no commit SHA to compare against + if (!reviewedCommitSha) { + return; + } + + // Skip if aborted + if (abortSignal?.aborted) { + return; + } + + result.checkNewCommitsCalled = true; + result.checkNewCommitsCallArgs = { projectId, prNumber }; + + try { + const newCommitsResult = await mockCheckNewCommits(projectId, prNumber); + + // Check abort signal after async call + if (abortSignal?.aborted) { + return; + } + + mockSetNewCommitsCheck(projectId, prNumber, newCommitsResult); + result.setNewCommitsCheckCalled = true; + } catch { + // Ignore errors in tests + } + }; + + // Case 1: No existing state or no result, and not reviewing - load from disk + if (!existingState?.result && !existingState?.isReviewing) { + result.getPRReviewCalled = true; + const reviewFromDisk = await mockGetPRReview(projectId, prNumber); + + if (reviewFromDisk) { + mockSetPRReviewResult(projectId, reviewFromDisk); + result.setPRReviewResultCalled = true; + + // CRITICAL: Check for new commits AFTER loading review + await checkNewCommitsForPR(reviewFromDisk.reviewedCommitSha); + } + } + // Case 2: Review already in store - check for new commits immediately + else if (existingState?.result) { + await checkNewCommitsForPR(existingState.result.reviewedCommitSha); + } + // Case 3: Review in progress - do NOT check for new commits + // (no action needed, we just don't call checkNewCommits) + + return result; +} + +describe('useGitHubPRs - selectPR triggering checkNewCommits', () => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + let mockCheckNewCommits: any; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + let mockGetPRReview: any; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + let mockSetNewCommitsCheck: any; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + let mockSetPRReviewResult: any; + + beforeEach(() => { + mockCheckNewCommits = vi.fn().mockResolvedValue(createMockNewCommitsCheck()); + mockGetPRReview = vi.fn().mockResolvedValue(null); + mockSetNewCommitsCheck = vi.fn(); + mockSetPRReviewResult = vi.fn(); + }); + + afterEach(() => { + vi.clearAllMocks(); + }); + + describe('checkNewCommits triggered when review exists in store', () => { + it('should call checkNewCommits when selecting PR with existing review in store', async () => { + const existingReview = createMockReviewResult({ + prNumber: 123, + reviewedCommitSha: 'abc123', + }); + + const result = await simulateSelectPR({ + prNumber: 123, + projectId: 'test-project', + existingState: { + result: existingReview, + isReviewing: false, + newCommitsCheck: null, + }, + diskReviewResult: null, + mockCheckNewCommits, + mockGetPRReview, + mockSetNewCommitsCheck, + mockSetPRReviewResult, + }); + + expect(result.checkNewCommitsCalled).toBe(true); + expect(result.checkNewCommitsCallArgs).toEqual({ + projectId: 'test-project', + prNumber: 123, + }); + expect(mockCheckNewCommits).toHaveBeenCalledWith('test-project', 123); + }); + + it('should update store with new commits check result', async () => { + const newCommitsResult = createMockNewCommitsCheck({ + hasNewCommits: true, + newCommitCount: 3, + hasCommitsAfterPosting: true, + }); + mockCheckNewCommits.mockResolvedValue(newCommitsResult); + + const existingReview = createMockReviewResult({ reviewedCommitSha: 'abc123' }); + + await simulateSelectPR({ + prNumber: 123, + projectId: 'test-project', + existingState: { + result: existingReview, + isReviewing: false, + newCommitsCheck: null, + }, + diskReviewResult: null, + mockCheckNewCommits, + mockGetPRReview, + mockSetNewCommitsCheck, + mockSetPRReviewResult, + }); + + expect(mockSetNewCommitsCheck).toHaveBeenCalledWith( + 'test-project', + 123, + newCommitsResult + ); + }); + }); + + describe('checkNewCommits triggered after loading review from disk', () => { + it('should call checkNewCommits after loading review from disk', async () => { + const diskReview = createMockReviewResult({ + prNumber: 456, + reviewedCommitSha: 'def789', + }); + mockGetPRReview.mockResolvedValue(diskReview); + + const result = await simulateSelectPR({ + prNumber: 456, + projectId: 'test-project', + existingState: null, // No existing state + diskReviewResult: diskReview, + mockCheckNewCommits, + mockGetPRReview, + mockSetNewCommitsCheck, + mockSetPRReviewResult, + }); + + // Should load from disk first + expect(result.getPRReviewCalled).toBe(true); + expect(result.setPRReviewResultCalled).toBe(true); + expect(mockSetPRReviewResult).toHaveBeenCalledWith('test-project', diskReview); + + // Then check for new commits + expect(result.checkNewCommitsCalled).toBe(true); + expect(mockCheckNewCommits).toHaveBeenCalledWith('test-project', 456); + }); + + it('should NOT call checkNewCommits if disk review has no reviewedCommitSha', async () => { + const diskReviewWithoutSha = createMockReviewResult({ + prNumber: 789, + reviewedCommitSha: undefined, // No SHA + }); + mockGetPRReview.mockResolvedValue(diskReviewWithoutSha); + + const result = await simulateSelectPR({ + prNumber: 789, + projectId: 'test-project', + existingState: null, + diskReviewResult: diskReviewWithoutSha, + mockCheckNewCommits, + mockGetPRReview, + mockSetNewCommitsCheck, + mockSetPRReviewResult, + }); + + // Should still load from disk + expect(result.getPRReviewCalled).toBe(true); + expect(result.setPRReviewResultCalled).toBe(true); + + // But NOT check for new commits + expect(result.checkNewCommitsCalled).toBe(false); + expect(mockCheckNewCommits).not.toHaveBeenCalled(); + }); + }); + + describe('checkNewCommits NOT triggered during active review', () => { + it('should NOT call checkNewCommits when review is in progress', async () => { + const result = await simulateSelectPR({ + prNumber: 123, + projectId: 'test-project', + existingState: { + result: null, // No result yet + isReviewing: true, // Review in progress + newCommitsCheck: null, + }, + diskReviewResult: null, + mockCheckNewCommits, + mockGetPRReview, + mockSetNewCommitsCheck, + mockSetPRReviewResult, + }); + + // Should NOT check for new commits during active review + expect(result.checkNewCommitsCalled).toBe(false); + expect(mockCheckNewCommits).not.toHaveBeenCalled(); + + // Should also NOT load from disk (review is managed by IPC) + expect(result.getPRReviewCalled).toBe(false); + }); + + it('should still call checkNewCommits when previous result exists during active review', async () => { + const previousReview = createMockReviewResult({ reviewedCommitSha: 'old123' }); + + const result = await simulateSelectPR({ + prNumber: 123, + projectId: 'test-project', + existingState: { + result: previousReview, + isReviewing: true, + newCommitsCheck: null, + }, + diskReviewResult: null, + mockCheckNewCommits, + mockGetPRReview, + mockSetNewCommitsCheck, + mockSetPRReviewResult, + }); + + expect(result.checkNewCommitsCalled).toBe(true); + }); + }); + + describe('checkNewCommits NOT triggered without reviewedCommitSha', () => { + it('should NOT call checkNewCommits if store review has no reviewedCommitSha', async () => { + const reviewWithoutSha = createMockReviewResult({ + reviewedCommitSha: undefined, + }); + + const result = await simulateSelectPR({ + prNumber: 123, + projectId: 'test-project', + existingState: { + result: reviewWithoutSha, + isReviewing: false, + newCommitsCheck: null, + }, + diskReviewResult: null, + mockCheckNewCommits, + mockGetPRReview, + mockSetNewCommitsCheck, + mockSetPRReviewResult, + }); + + expect(result.checkNewCommitsCalled).toBe(false); + expect(mockCheckNewCommits).not.toHaveBeenCalled(); + }); + }); + + describe('Race condition prevention', () => { + it('should abort checkNewCommits when signal is aborted', async () => { + const abortController = new AbortController(); + const existingReview = createMockReviewResult({ reviewedCommitSha: 'abc123' }); + + // Abort before the call + abortController.abort(); + + const result = await simulateSelectPR({ + prNumber: 123, + projectId: 'test-project', + existingState: { + result: existingReview, + isReviewing: false, + newCommitsCheck: null, + }, + diskReviewResult: null, + mockCheckNewCommits, + mockGetPRReview, + mockSetNewCommitsCheck, + mockSetPRReviewResult, + abortSignal: abortController.signal, + }); + + expect(result.checkNewCommitsCalled).toBe(false); + expect(mockSetNewCommitsCheck).not.toHaveBeenCalled(); + }); + + it('should NOT update store if aborted during async operation', async () => { + const abortController = new AbortController(); + const existingReview = createMockReviewResult({ reviewedCommitSha: 'abc123' }); + + // Make checkNewCommits delay and abort during the delay + mockCheckNewCommits.mockImplementation(async () => { + // Simulate abort happening during the async call + abortController.abort(); + return createMockNewCommitsCheck({ hasNewCommits: true }); + }); + + const result = await simulateSelectPR({ + prNumber: 123, + projectId: 'test-project', + existingState: { + result: existingReview, + isReviewing: false, + newCommitsCheck: null, + }, + diskReviewResult: null, + mockCheckNewCommits, + mockGetPRReview, + mockSetNewCommitsCheck, + mockSetPRReviewResult, + abortSignal: abortController.signal, + }); + + // checkNewCommits was called + expect(result.checkNewCommitsCalled).toBe(true); + // But store was NOT updated because abort happened + expect(result.setNewCommitsCheckCalled).toBe(false); + }); + }); + + describe('Edge cases', () => { + it('should NOT trigger anything when prNumber is null (deselecting)', async () => { + const result = await simulateSelectPR({ + prNumber: null, + projectId: 'test-project', + existingState: null, + diskReviewResult: null, + mockCheckNewCommits, + mockGetPRReview, + mockSetNewCommitsCheck, + mockSetPRReviewResult, + }); + + expect(result.checkNewCommitsCalled).toBe(false); + expect(result.getPRReviewCalled).toBe(false); + expect(mockCheckNewCommits).not.toHaveBeenCalled(); + expect(mockGetPRReview).not.toHaveBeenCalled(); + }); + + it('should NOT trigger anything when projectId is null', async () => { + const result = await simulateSelectPR({ + prNumber: 123, + projectId: null, + existingState: null, + diskReviewResult: null, + mockCheckNewCommits, + mockGetPRReview, + mockSetNewCommitsCheck, + mockSetPRReviewResult, + }); + + expect(result.checkNewCommitsCalled).toBe(false); + expect(result.getPRReviewCalled).toBe(false); + }); + + it('should NOT call getPRReview if review already exists in store (not reviewing)', async () => { + const existingReview = createMockReviewResult({ reviewedCommitSha: 'abc123' }); + + const result = await simulateSelectPR({ + prNumber: 123, + projectId: 'test-project', + existingState: { + result: existingReview, + isReviewing: false, + newCommitsCheck: null, + }, + diskReviewResult: null, + mockCheckNewCommits, + mockGetPRReview, + mockSetNewCommitsCheck, + mockSetPRReviewResult, + }); + + // Should NOT load from disk (already in store) + expect(result.getPRReviewCalled).toBe(false); + expect(mockGetPRReview).not.toHaveBeenCalled(); + + // But SHOULD check for new commits + expect(result.checkNewCommitsCalled).toBe(true); + }); + + it('should NOT load from disk if no review exists on disk', async () => { + mockGetPRReview.mockResolvedValue(null); // No review on disk + + const result = await simulateSelectPR({ + prNumber: 123, + projectId: 'test-project', + existingState: null, // No existing state + diskReviewResult: null, + mockCheckNewCommits, + mockGetPRReview, + mockSetNewCommitsCheck, + mockSetPRReviewResult, + }); + + // Should try to load from disk + expect(result.getPRReviewCalled).toBe(true); + // But no result to set + expect(result.setPRReviewResultCalled).toBe(false); + // And no new commits check (no reviewedCommitSha) + expect(result.checkNewCommitsCalled).toBe(false); + }); + }); +}); + +describe('useGitHubPRs - checkNewCommits result handling', () => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + let mockSetNewCommitsCheck: any; + + beforeEach(() => { + mockSetNewCommitsCheck = vi.fn(); + }); + + afterEach(() => { + vi.clearAllMocks(); + }); + + it('should handle hasNewCommits: true correctly', async () => { + const mockCheckNewCommits = vi.fn().mockResolvedValue( + createMockNewCommitsCheck({ + hasNewCommits: true, + newCommitCount: 5, + hasCommitsAfterPosting: true, + }) + ); + + const existingReview = createMockReviewResult({ reviewedCommitSha: 'abc123' }); + + await simulateSelectPR({ + prNumber: 123, + projectId: 'test-project', + existingState: { + result: existingReview, + isReviewing: false, + newCommitsCheck: null, + }, + diskReviewResult: null, + mockCheckNewCommits, + mockGetPRReview: vi.fn(), + mockSetNewCommitsCheck, + mockSetPRReviewResult: vi.fn(), + }); + + expect(mockSetNewCommitsCheck).toHaveBeenCalledWith('test-project', 123, { + hasNewCommits: true, + newCommitCount: 5, + hasCommitsAfterPosting: true, + }); + }); + + it('should handle hasNewCommits: false correctly', async () => { + const mockCheckNewCommits = vi.fn().mockResolvedValue( + createMockNewCommitsCheck({ + hasNewCommits: false, + newCommitCount: 0, + hasCommitsAfterPosting: false, + }) + ); + + const existingReview = createMockReviewResult({ reviewedCommitSha: 'abc123' }); + + await simulateSelectPR({ + prNumber: 123, + projectId: 'test-project', + existingState: { + result: existingReview, + isReviewing: false, + newCommitsCheck: null, + }, + diskReviewResult: null, + mockCheckNewCommits, + mockGetPRReview: vi.fn(), + mockSetNewCommitsCheck, + mockSetPRReviewResult: vi.fn(), + }); + + expect(mockSetNewCommitsCheck).toHaveBeenCalledWith('test-project', 123, { + hasNewCommits: false, + newCommitCount: 0, + hasCommitsAfterPosting: false, + }); + }); +}); 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 00170386..8346bf1b 100644 --- a/apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts +++ b/apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts @@ -86,6 +86,8 @@ export function useGitHubPRs( const hasLoadedRef = useRef(false); // Track the current PR being fetched (for race condition prevention) const currentFetchPRNumberRef = useRef(null); + // AbortController for cancelling pending checkNewCommits calls on rapid PR switching + const checkNewCommitsAbortRef = useRef(null); // Get PR review state from the global store const prReviews = usePRReviewStore((state) => state.prReviews); @@ -249,12 +251,35 @@ export function useGitHubPRs( setPrs([]); setSelectedPRNumber(null); setSelectedPRDetails(null); + currentFetchPRNumberRef.current = null; + // Cancel any pending checkNewCommits request + if (checkNewCommitsAbortRef.current) { + checkNewCommitsAbortRef.current.abort(); + checkNewCommitsAbortRef.current = null; + } }, [projectId]); + // Cleanup abort controller on unmount to prevent memory leaks + // and avoid state updates on unmounted components + useEffect(() => { + return () => { + if (checkNewCommitsAbortRef.current) { + checkNewCommitsAbortRef.current.abort(); + } + }; + }, []); + // No need for local IPC listeners - they're handled globally in github-store const selectPR = useCallback( (prNumber: number | null) => { + // Abort any pending checkNewCommits request from previous PR selection + // This prevents stale data from appearing when user switches PRs rapidly + if (checkNewCommitsAbortRef.current) { + checkNewCommitsAbortRef.current.abort(); + checkNewCommitsAbortRef.current = null; + } + setSelectedPRNumber(prNumber); // Note: Don't reset review result - it comes from the store now // and persists across navigation @@ -290,12 +315,63 @@ export function useGitHubPRs( } }); + // Helper function to check for new commits with race condition protection + // This is called after review state is available (from store or disk) + // Uses AbortController pattern to cancel pending checks when user switches PRs rapidly + const checkNewCommitsForPR = (reviewedCommitSha: string | undefined) => { + // Skip if no commit SHA to compare against + if (!reviewedCommitSha) { + return; + } + + // Skip if user has already switched to a different PR (race condition prevention) + if (prNumber !== currentFetchPRNumberRef.current) { + return; + } + + // Cancel any pending checkNewCommits request before starting a new one + if (checkNewCommitsAbortRef.current) { + checkNewCommitsAbortRef.current.abort(); + } + checkNewCommitsAbortRef.current = new AbortController(); + const currentAbortController = checkNewCommitsAbortRef.current; + + window.electronAPI.github + .checkNewCommits(projectId, prNumber) + .then((newCommitsResult) => { + // Check if request was aborted (user switched PRs) + if (currentAbortController.signal.aborted) { + return; + } + + // Final race condition check before updating store + if (prNumber !== currentFetchPRNumberRef.current) { + return; + } + + setNewCommitsCheckAction(projectId, prNumber, newCommitsResult); + }) + .catch((err) => { + // Don't log errors for aborted requests + if (currentAbortController.signal.aborted) { + return; + } + console.warn(`Failed to check new commits for PR #${prNumber}:`, err); + }); + }; + // Load existing review from disk if not already in store const existingState = getPRReviewState(projectId, prNumber); + // Only fetch from disk if we don't have a result in the store AND no review is running // If a review is in progress, the state is managed by IPC listeners - don't overwrite it if (!existingState?.result && !existingState?.isReviewing) { window.electronAPI.github.getPRReview(projectId, prNumber).then((result) => { + // Race condition check: skip if user switched PRs + if (prNumber !== currentFetchPRNumberRef.current) { + return; + } + if (result) { // Update store with the loaded result // Preserve newCommitsCheck when loading existing review from disk @@ -305,33 +381,16 @@ export function useGitHubPRs( // Always check for new commits when selecting a reviewed PR // This ensures fresh data even if we have a cached check from earlier in the session - const reviewedCommitSha = result.reviewedCommitSha; - if (reviewedCommitSha) { - window.electronAPI.github - .checkNewCommits(projectId, prNumber) - .then((newCommitsResult) => { - setNewCommitsCheckAction(projectId, prNumber, newCommitsResult); - }) - .catch((err) => { - console.warn(`Failed to check new commits for PR #${prNumber}:`, err); - }); - } + // CRITICAL: This runs AFTER store is updated with review result + checkNewCommitsForPR(result.reviewedCommitSha); } }); } else if (existingState?.result) { // Review already in store - always check for new commits to get fresh status - const reviewedCommitSha = existingState.result.reviewedCommitSha; - if (reviewedCommitSha) { - window.electronAPI.github - .checkNewCommits(projectId, prNumber) - .then((newCommitsResult) => { - setNewCommitsCheckAction(projectId, prNumber, newCommitsResult); - }) - .catch((err) => { - console.warn(`Failed to check new commits for PR #${prNumber}:`, err); - }); - } + // CRITICAL: Review state is already available, check for new commits immediately + checkNewCommitsForPR(existingState.result.reviewedCommitSha); } + // If existingState?.isReviewing, state is managed by IPC listeners - do nothing } }, [projectId, getPRReviewState, setNewCommitsCheckAction]