diff --git a/apps/frontend/src/renderer/components/github-prs/GitHubPRs.tsx b/apps/frontend/src/renderer/components/github-prs/GitHubPRs.tsx index c5df0e01..d095bfbf 100644 --- a/apps/frontend/src/renderer/components/github-prs/GitHubPRs.tsx +++ b/apps/frontend/src/renderer/components/github-prs/GitHubPRs.tsx @@ -64,9 +64,9 @@ export function GitHubPRs({ onOpenSettings, isActive = false }: GitHubPRsProps) selectedPRNumber, reviewResult, reviewProgress, + startedAt, isReviewing, previousReviewResult, - startedAt, hasMore, selectPR, runReview, 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 5709742e..5a9b0fe5 100644 --- a/apps/frontend/src/renderer/components/github-prs/components/PRDetail.tsx +++ b/apps/frontend/src/renderer/components/github-prs/components/PRDetail.tsx @@ -1,4 +1,4 @@ -import { useState, useEffect, useMemo, useCallback, useRef } from 'react'; +import { useState, useEffect, useMemo, useCallback, useRef, useId } from 'react'; import { useTranslation } from 'react-i18next'; import { Bot, @@ -16,6 +16,8 @@ import { ExternalLink, Play, Clock, + ChevronDown, + ChevronUp, } from 'lucide-react'; import { Badge } from '../../ui/badge'; import { Button } from '../../ui/button'; @@ -94,6 +96,10 @@ export function PRDetail({ const [isPostingFindings, setIsPostingFindings] = useState(false); const [postSuccess, setPostSuccess] = useState<{ count: number; timestamp: number } | null>(null); const [isPosting, setIsPosting] = useState(false); + const [isPostingCleanReview, setIsPostingCleanReview] = useState(false); + const [cleanReviewPosted, setCleanReviewPosted] = useState(false); + const [cleanReviewError, setCleanReviewError] = useState(null); + const [showCleanReviewErrorDetails, setShowCleanReviewErrorDetails] = useState(false); const [isMerging, setIsMerging] = useState(false); // Initialize with store value, then sync and update via local checks const [newCommitsCheck, setNewCommitsCheck] = useState(initialNewCommitsCheck ?? null); @@ -116,6 +122,9 @@ export function PRDetail({ const [isApprovingWorkflow, setIsApprovingWorkflow] = useState(null); const [workflowsExpanded, setWorkflowsExpanded] = useState(true); + // Generate stable IDs for accessibility + const cleanReviewErrorDetailsId = useId(); + // Sync with store's newCommitsCheck when it changes (e.g., when switching PRs or after refresh) // Always sync to keep local state in sync with store, including null values useEffect(() => { @@ -257,6 +266,10 @@ export function PRDetail({ logsLoadedRef.current = false; setPrLogs(null); setLogsExpanded(false); + setCleanReviewPosted(false); + setCleanReviewError(null); + setIsPostingCleanReview(false); + setShowCleanReviewErrorDetails(false); }, [pr.number]); // Check for workflows awaiting approval (fork PRs) when PR changes or review completes @@ -543,11 +556,14 @@ export function PRDetail({ const idsToPost = Array.from(selectedFindingIds); if (idsToPost.length === 0) return; + // Capture current PR number to prevent state leaks across PR switches + const currentPr = pr.number; + setIsPostingFindings(true); try { const success = await onPostReview(idsToPost); - if (success) { - // Mark these findings as posted + if (success && pr.number === currentPr) { + // Mark these findings as posted only if PR hasn't changed setPostedFindingIds(prev => new Set([...prev, ...idsToPost])); // Clear selection setSelectedFindingIds(new Set()); @@ -558,21 +574,30 @@ export function PRDetail({ setTimeout(() => checkForNewCommits(), 500); } } finally { - setIsPostingFindings(false); + // Clear loading state if PR hasn't changed + if (pr.number === currentPr) { + setIsPostingFindings(false); + } } }; const handleApprove = async () => { if (!reviewResult) return; + // Capture current PR number to prevent state leaks across PR switches + const currentPr = pr.number; + setIsPosting(true); try { // Auto-assign current user (you can get from GitHub config) // For now, we'll just post the comment const approvalMessage = `## ✅ Auto Claude PR Review - APPROVED\n\n${reviewResult.summary}\n\n---\n*This approval was generated by Auto Claude.*`; - await onPostComment(approvalMessage); + await Promise.resolve(onPostComment(approvalMessage)); } finally { - setIsPosting(false); + // Clear loading state if PR hasn't changed + if (pr.number === currentPr) { + setIsPosting(false); + } } }; @@ -582,6 +607,10 @@ export function PRDetail({ // content is meant to be read by contributors who may have different locales. const handleAutoApprove = async () => { if (!reviewResult) return; + + // Capture current PR number to prevent state leaks across PR switches + const currentPr = pr.number; + setIsPosting(true); try { // Post approval with suggestions in a single review comment @@ -589,12 +618,66 @@ export function PRDetail({ const lowFindingIds = lowSeverityFindings.map(f => f.id); const success = await onPostReview(lowFindingIds, { forceApprove: true }); - if (success && lowFindingIds.length > 0) { - // Mark findings as posted locally + if (success && lowFindingIds.length > 0 && pr.number === currentPr) { + // Mark findings as posted locally only if PR hasn't changed setPostedFindingIds(prev => new Set([...prev, ...lowFindingIds])); } } finally { - setIsPosting(false); + // Clear loading state if PR hasn't changed + if (pr.number === currentPr) { + setIsPosting(false); + } + } + }; + + // Post clean review as a comment (does not change PR review status) + // This is for when a review has no findings or only LOW severity findings + // NOTE: GitHub PR comments are intentionally in English as it's the lingua franca + // for code reviews and GitHub's international developer community. + const handlePostCleanReview = async () => { + if (!reviewResult) return; + + // Capture current PR number to prevent state leaks across PR switches + const currentPr = pr.number; + + setIsPostingCleanReview(true); + setCleanReviewError(null); // Clear previous error + setShowCleanReviewErrorDetails(false); // Reset error details visibility + try { + // Format the clean review comment using i18n translations + const cleanReviewMessage = `${t('prReview.cleanReviewMessageTitle')} + +${t('prReview.cleanReviewMessageStatus')} + +${reviewResult.summary} + +--- + +${t('prReview.cleanReviewMessageFooter')}`; + + // Use Promise.resolve to handle both Promise and non-Promise implementations + await Promise.resolve(onPostComment(cleanReviewMessage)); + + // Only mark as posted on success if PR hasn't changed + if (pr.number === currentPr) { + setCleanReviewPosted(true); + setCleanReviewError(null); + } + } catch (err) { + // Log full error to console for debugging before rendering + console.error('Failed to post clean review comment:', err); + + // Set user-friendly error message using translation key + const fullError = err instanceof Error ? err.message : String(err); + if (pr.number === currentPr) { + setCleanReviewError(fullError); + } + // Do NOT set cleanReviewPosted on failure + } finally { + // Clear loading state if PR hasn't changed + if (pr.number === currentPr) { + setIsPostingCleanReview(false); + } } }; @@ -677,12 +760,34 @@ export function PRDetail({ )} + {/* Post Clean Review button - shows when review is clean and no findings are selected */} + {selectedCount === 0 && isCleanReview && !hasPostedFindings && !cleanReviewPosted && reviewResult?.overallStatus !== 'request_changes' && ( + + )} + {/* Approve button - consolidated logic to avoid duplicate buttons */} {/* Don't show when overallStatus is 'request_changes' (e.g., workflows blocked, or other issues) */} {isCleanReview && !hasPostedFindings && reviewResult?.overallStatus !== 'request_changes' && ( + + )} + {cleanReviewError && showCleanReviewErrorDetails && ( +
+ {cleanReviewError} +
+ )} )} diff --git a/apps/frontend/src/renderer/components/github-prs/components/__tests__/PRDetail.cleanReview.test.ts b/apps/frontend/src/renderer/components/github-prs/components/__tests__/PRDetail.cleanReview.test.ts new file mode 100644 index 00000000..9d1b2ec9 --- /dev/null +++ b/apps/frontend/src/renderer/components/github-prs/components/__tests__/PRDetail.cleanReview.test.ts @@ -0,0 +1,658 @@ +/** + * Unit tests for PRDetail clean review functionality + * Tests the "Post Clean Review" button visibility and behavior + * + * @vitest-environment jsdom + */ +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +// Types for PR review data +interface PRReviewFinding { + id: string; + severity: 'critical' | 'high' | 'medium' | 'low'; + category: string; + file: string; + line: number; + description: string; + suggestedFix?: string; +} + +interface PRReviewResult { + success: boolean; + overallStatus: 'approve' | 'request_changes' | 'comment'; + summary: string; + findings: PRReviewFinding[]; + reviewedCommitSha: string | null; + postedAt?: string; + postedFindingIds?: string[]; + hasPostedFindings?: boolean; + isFollowupReview?: boolean; + resolvedFindings?: PRReviewFinding[]; + unresolvedFindings?: PRReviewFinding[]; + newFindingsSinceLastReview?: PRReviewFinding[]; +} + +// Helper to create test review results +function createReviewResult(overrides: Partial = {}): PRReviewResult { + return { + success: true, + overallStatus: 'approve', + summary: 'Code review completed successfully. No issues found.', + findings: [], + reviewedCommitSha: 'abc123', + ...overrides + }; +} + +function createTestFinding(severity: PRReviewFinding['severity'], id: string = 'finding-1'): PRReviewFinding { + return { + id, + severity, + category: 'quality', + file: 'src/test.ts', + line: 10, + description: `Test ${severity} severity issue` + }; +} + +describe('PRDetail Clean Review Functionality', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + /** + * NOTE: These tests verify the core ALGORITHM (isCleanReview logic, button visibility conditions). + * + * The actual React component behavior (rendering, user interactions, useEffect hooks) is tested + * in PRDetail.integration.test.tsx. These algorithm tests provide rapid feedback on logic + * changes but duplicate the implementation expressions. If the component logic changes, + * these tests must be updated to match. + * + * Alternative: Export isCleanReview as a pure function for direct testing. + */ + describe('isCleanReview Logic', () => { + it('should return true for review with no findings', () => { + const reviewResult = createReviewResult({ + findings: [] + }); + + // isCleanReview logic: success && no critical/high/medium findings + const isCleanReview = reviewResult.success && + !reviewResult.findings.some(f => + f.severity === 'critical' || f.severity === 'high' || f.severity === 'medium' + ); + + expect(isCleanReview).toBe(true); + expect(reviewResult.findings).toHaveLength(0); + }); + + it('should return true for review with only LOW severity findings', () => { + const reviewResult = createReviewResult({ + findings: [ + createTestFinding('low', 'low-1'), + createTestFinding('low', 'low-2'), + createTestFinding('low', 'low-3') + ] + }); + + const isCleanReview = reviewResult.success && + !reviewResult.findings.some(f => + f.severity === 'critical' || f.severity === 'high' || f.severity === 'medium' + ); + + expect(isCleanReview).toBe(true); + expect(reviewResult.findings).toHaveLength(3); + }); + + it('should return false for review with MEDIUM severity findings', () => { + const reviewResult = createReviewResult({ + findings: [ + createTestFinding('low', 'low-1'), + createTestFinding('medium', 'medium-1') + ] + }); + + const isCleanReview = reviewResult.success && + !reviewResult.findings.some(f => + f.severity === 'critical' || f.severity === 'high' || f.severity === 'medium' + ); + + expect(isCleanReview).toBe(false); + }); + + it('should return false for review with HIGH severity findings', () => { + const reviewResult = createReviewResult({ + findings: [ + createTestFinding('low', 'low-1'), + createTestFinding('high', 'high-1') + ] + }); + + const isCleanReview = reviewResult.success && + !reviewResult.findings.some(f => + f.severity === 'critical' || f.severity === 'high' || f.severity === 'medium' + ); + + expect(isCleanReview).toBe(false); + }); + + it('should return false for review with CRITICAL severity findings', () => { + const reviewResult = createReviewResult({ + findings: [ + createTestFinding('critical', 'critical-1') + ] + }); + + const isCleanReview = reviewResult.success && + !reviewResult.findings.some(f => + f.severity === 'critical' || f.severity === 'high' || f.severity === 'medium' + ); + + expect(isCleanReview).toBe(false); + }); + + it('should return false for failed review', () => { + const reviewResult = createReviewResult({ + success: false, + findings: [] + }); + + const isCleanReview = reviewResult.success && + !reviewResult.findings.some(f => + f.severity === 'critical' || f.severity === 'high' || f.severity === 'medium' + ); + + expect(isCleanReview).toBe(false); + }); + }); + + describe('Post Clean Review Button Visibility', () => { + it('should show button when: review success, no findings selected, clean review, not posted, not request_changes', () => { + const reviewResult = createReviewResult({ + findings: [] + }); + + const selectedCount = 0; + const hasPostedFindings = false; + const cleanReviewPosted = false; + + // Button visibility conditions + const shouldShowButton = + selectedCount === 0 && + reviewResult.success && + !reviewResult.findings.some(f => + f.severity === 'critical' || f.severity === 'high' || f.severity === 'medium' + ) && + !hasPostedFindings && + !cleanReviewPosted && + reviewResult.overallStatus !== 'request_changes'; + + expect(shouldShowButton).toBe(true); + }); + + it('should NOT show button when findings are selected', () => { + const reviewResult = createReviewResult({ + findings: [createTestFinding('low')] + }); + + const selectedCount: number = 1; // Finding selected + const hasPostedFindings = false; + const cleanReviewPosted = false; + + const shouldShowButton = + selectedCount === 0 && + reviewResult.success && + !reviewResult.findings.some(f => + f.severity === 'critical' || f.severity === 'high' || f.severity === 'medium' + ) && + !hasPostedFindings && + !cleanReviewPosted && + reviewResult.overallStatus !== 'request_changes'; + + expect(shouldShowButton).toBe(false); + }); + + it('should NOT show button when review has MEDIUM severity findings', () => { + const reviewResult = createReviewResult({ + findings: [createTestFinding('medium')] + }); + + const selectedCount = 0; + const hasPostedFindings = false; + const cleanReviewPosted = false; + + const shouldShowButton = + selectedCount === 0 && + reviewResult.success && + !reviewResult.findings.some(f => + f.severity === 'critical' || f.severity === 'high' || f.severity === 'medium' + ) && + !hasPostedFindings && + !cleanReviewPosted && + reviewResult.overallStatus !== 'request_changes'; + + expect(shouldShowButton).toBe(false); + }); + + it('should NOT show button when findings have already been posted', () => { + const reviewResult = createReviewResult({ + findings: [] + }); + + const selectedCount = 0; + const hasPostedFindings = true; // Already posted + const cleanReviewPosted = false; + + const shouldShowButton = + selectedCount === 0 && + reviewResult.success && + !reviewResult.findings.some(f => + f.severity === 'critical' || f.severity === 'high' || f.severity === 'medium' + ) && + !hasPostedFindings && + !cleanReviewPosted && + reviewResult.overallStatus !== 'request_changes'; + + expect(shouldShowButton).toBe(false); + }); + + it('should NOT show button when clean review has been posted', () => { + const reviewResult = createReviewResult({ + findings: [] + }); + + const selectedCount = 0; + const hasPostedFindings = false; + const cleanReviewPosted = true; // Already posted clean review + + const shouldShowButton = + selectedCount === 0 && + reviewResult.success && + !reviewResult.findings.some(f => + f.severity === 'critical' || f.severity === 'high' || f.severity === 'medium' + ) && + !hasPostedFindings && + !cleanReviewPosted && + reviewResult.overallStatus !== 'request_changes'; + + expect(shouldShowButton).toBe(false); + }); + + it('should NOT show button when overallStatus is request_changes', () => { + const reviewResult = createReviewResult({ + findings: [], + overallStatus: 'request_changes' + }); + + const selectedCount = 0; + const hasPostedFindings = false; + const cleanReviewPosted = false; + + const shouldShowButton = + selectedCount === 0 && + reviewResult.success && + !reviewResult.findings.some(f => + f.severity === 'critical' || f.severity === 'high' || f.severity === 'medium' + ) && + !hasPostedFindings && + !cleanReviewPosted && + reviewResult.overallStatus !== 'request_changes'; + + expect(shouldShowButton).toBe(false); + }); + + it('should NOT show button when review failed', () => { + const reviewResult = createReviewResult({ + success: false, + findings: [] + }); + + const selectedCount = 0; + const hasPostedFindings = false; + const cleanReviewPosted = false; + + const shouldShowButton = + selectedCount === 0 && + reviewResult.success && + !reviewResult.findings.some(f => + f.severity === 'critical' || f.severity === 'high' || f.severity === 'medium' + ) && + !hasPostedFindings && + !cleanReviewPosted && + reviewResult.overallStatus !== 'request_changes'; + + expect(shouldShowButton).toBe(false); + }); + }); + + describe('Post Clean Review vs Post Findings Button Mutual Exclusivity', () => { + it('should show "Post Findings" button when findings are selected', () => { + const reviewResult = createReviewResult({ + findings: [createTestFinding('low')] + }); + + const selectedCount: number = 1; + + // Post Findings button: selectedCount > 0 + const showPostFindings = selectedCount > 0; + + // Post Clean Review button: selectedCount === 0 && other conditions + const showPostCleanReview = + selectedCount === 0 && + reviewResult.success && + !reviewResult.findings.some(f => + f.severity === 'critical' || f.severity === 'high' || f.severity === 'medium' + ); + + expect(showPostFindings).toBe(true); + expect(showPostCleanReview).toBe(false); + }); + + it('should show "Post Clean Review" button when no findings are selected and review is clean', () => { + const reviewResult = createReviewResult({ + findings: [] + }); + + const selectedCount = 0; + + const showPostFindings = selectedCount > 0; + + const showPostCleanReview = + selectedCount === 0 && + reviewResult.success && + !reviewResult.findings.some(f => + f.severity === 'critical' || f.severity === 'high' || f.severity === 'medium' + ); + + expect(showPostFindings).toBe(false); + expect(showPostCleanReview).toBe(true); + }); + + it('should show neither button when no findings exist and none selected but review is not clean', () => { + const reviewResult = createReviewResult({ + findings: [createTestFinding('high')] + }); + + const selectedCount = 0; + + const showPostFindings = selectedCount > 0; + + const showPostCleanReview = + selectedCount === 0 && + reviewResult.success && + !reviewResult.findings.some(f => + f.severity === 'critical' || f.severity === 'high' || f.severity === 'medium' + ); + + expect(showPostFindings).toBe(false); + expect(showPostCleanReview).toBe(false); + }); + }); + + describe('Clean Review Comment Format', () => { + it('should format clean review comment correctly', () => { + const reviewResult = createReviewResult({ + summary: 'All code passes review. No issues found.' + }); + + const cleanReviewMessage = `## ✅ Auto Claude PR Review - PASSED + +**Status:** All code is good + +${reviewResult.summary} + +--- + +*This automated review found no issues. Generated by Auto Claude.*`; + + expect(cleanReviewMessage).toContain('## ✅ Auto Claude PR Review - PASSED'); + expect(cleanReviewMessage).toContain('**Status:** All code is good'); + expect(cleanReviewMessage).toContain(reviewResult.summary); + expect(cleanReviewMessage).toContain('*This automated review found no issues. Generated by Auto Claude.*'); + }); + + it('should include custom summary in clean review comment', () => { + const customSummary = 'Review completed: 5 files checked, 0 issues found. Code follows best practices.'; + const reviewResult = createReviewResult({ + summary: customSummary + }); + + const cleanReviewMessage = `## ✅ Auto Claude PR Review - PASSED + +**Status:** All code is good + +${reviewResult.summary} + +--- + +*This automated review found no issues. Generated by Auto Claude.*`; + + expect(cleanReviewMessage).toContain(customSummary); + }); + + it('should handle empty summary gracefully', () => { + const reviewResult = createReviewResult({ + summary: '' + }); + + const cleanReviewMessage = `## ✅ Auto Claude PR Review - PASSED + +**Status:** All code is good + +${reviewResult.summary} + +--- + +*This automated review found no issues. Generated by Auto Claude.*`; + + expect(cleanReviewMessage).toBeDefined(); + expect(cleanReviewMessage).toContain('All code is good'); + }); + }); + + describe('Follow-up Review Scenarios', () => { + it('should show clean review button for follow-up with all issues resolved', () => { + const reviewResult = createReviewResult({ + isFollowupReview: true, + findings: [], // No new issues from follow-up + resolvedFindings: [createTestFinding('high', 'resolved-1')], + unresolvedFindings: [], + newFindingsSinceLastReview: [] + }); + + const selectedCount = 0; + const hasPostedFindings = false; + const cleanReviewPosted = false; + + const shouldShowButton = + selectedCount === 0 && + reviewResult.success && + !reviewResult.findings.some(f => + f.severity === 'critical' || f.severity === 'high' || f.severity === 'medium' + ) && + !hasPostedFindings && + !cleanReviewPosted && + reviewResult.overallStatus !== 'request_changes'; + + expect(shouldShowButton).toBe(true); + }); + + it('should not show clean review button for follow-up with unresolved HIGH issues', () => { + const reviewResult = createReviewResult({ + isFollowupReview: true, + findings: [createTestFinding('high', 'unresolved-1')], + unresolvedFindings: [createTestFinding('high', 'unresolved-1')], + newFindingsSinceLastReview: [] + }); + + const selectedCount = 0; + const hasPostedFindings = false; + const cleanReviewPosted = false; + + const shouldShowButton = + selectedCount === 0 && + reviewResult.success && + !reviewResult.findings.some(f => + f.severity === 'critical' || f.severity === 'high' || f.severity === 'medium' + ) && + !hasPostedFindings && + !cleanReviewPosted && + reviewResult.overallStatus !== 'request_changes'; + + expect(shouldShowButton).toBe(false); + }); + + it('should not show clean review button for follow-up with new issues found', () => { + const reviewResult = createReviewResult({ + isFollowupReview: true, + findings: [createTestFinding('high', 'new-1')], + newFindingsSinceLastReview: [createTestFinding('high', 'new-1')] + }); + + const selectedCount = 0; + const hasPostedFindings = false; + const cleanReviewPosted = false; + + const shouldShowButton = + selectedCount === 0 && + reviewResult.success && + !reviewResult.findings.some(f => + f.severity === 'critical' || f.severity === 'high' || f.severity === 'medium' + ) && + !hasPostedFindings && + !cleanReviewPosted && + reviewResult.overallStatus !== 'request_changes'; + + expect(shouldShowButton).toBe(false); + }); + }); + + /** + * NOTE: These tests verify the STATE RESET ALGORITHM with plain JavaScript if-statements. + * + * The actual React useEffect hook behavior is tested in PRDetail.integration.test.tsx + * via component rerendering. These algorithm tests provide rapid feedback on the + * reset logic but don't verify the hook triggers correctly on PR changes. + */ + describe('State Reset on PR Change', () => { + it('should reset cleanReviewPosted state when pr.number changes', () => { + // Simplified logic test - actual useEffect behavior tested in integration tests + let prNumber = 123; + let cleanReviewPosted = true; + + // Simulate the useEffect reset when pr.number changes + const currentPrNumber = prNumber; + const newPrNumber = 456; + + if (currentPrNumber !== newPrNumber) { + cleanReviewPosted = false; + prNumber = newPrNumber; + } + + expect(cleanReviewPosted).toBe(false); + expect(prNumber).toBe(456); + }); + + it('should not reset cleanReviewPosted state when pr.number stays the same', () => { + let prNumber = 123; + let cleanReviewPosted = true; + + // Simulate no change in pr.number + const currentPrNumber = prNumber; + const newPrNumber = 123; + + if (currentPrNumber !== newPrNumber) { + cleanReviewPosted = false; + prNumber = newPrNumber; + } + + expect(cleanReviewPosted).toBe(true); + expect(prNumber).toBe(123); + }); + }); + + describe('Success Message Display', () => { + it('should show clean review posted message when cleanReviewPosted is true', () => { + const postSuccess = null; + const cleanReviewPosted = true; + + // From implementation: {cleanReviewPosted && !postSuccess && (...)} + const showCleanReviewMessage = cleanReviewPosted && !postSuccess; + // From implementation: {postSuccess && (...)} + const showPostFindingsMessage = !!postSuccess; + + expect(showCleanReviewMessage).toBe(true); + expect(showPostFindingsMessage).toBe(false); + }); + + it('should show posted findings message when postSuccess is set', () => { + const postSuccess = { count: 3, timestamp: Date.now() }; + const cleanReviewPosted = false; + + const showCleanReviewMessage = cleanReviewPosted && !postSuccess; + const showPostFindingsMessage = !!postSuccess; + + expect(showCleanReviewMessage).toBe(false); + expect(showPostFindingsMessage).toBe(true); + }); + + it('should prioritize posted findings message when both are set', () => { + const postSuccess = { count: 2, timestamp: Date.now() }; + const cleanReviewPosted = true; + + // Implementation: postSuccess takes priority (first condition checked) + const showCleanReviewMessage = cleanReviewPosted && !postSuccess; + const showPostFindingsMessage = !!postSuccess; + + expect(showCleanReviewMessage).toBe(false); + expect(showPostFindingsMessage).toBe(true); + }); + }); + + describe('Edge Cases', () => { + it('should handle review with only LOW findings as clean', () => { + const reviewResult = createReviewResult({ + findings: [ + createTestFinding('low', 'low-1'), + createTestFinding('low', 'low-2') + ] + }); + + const isCleanReview = reviewResult.success && + !reviewResult.findings.some(f => + f.severity === 'critical' || f.severity === 'high' || f.severity === 'medium' + ); + + expect(isCleanReview).toBe(true); + expect(reviewResult.findings.length).toBe(2); + }); + + it('should handle mixed severity with LOW and MEDIUM as not clean', () => { + const reviewResult = createReviewResult({ + findings: [ + createTestFinding('low', 'low-1'), + createTestFinding('medium', 'medium-1') + ] + }); + + const isCleanReview = reviewResult.success && + !reviewResult.findings.some(f => + f.severity === 'critical' || f.severity === 'high' || f.severity === 'medium' + ); + + expect(isCleanReview).toBe(false); + }); + + it('should handle overallStatus comment correctly', () => { + const reviewResult = createReviewResult({ + overallStatus: 'comment', + findings: [] + }); + + const shouldShowButton = + reviewResult.overallStatus !== 'request_changes'; + + expect(shouldShowButton).toBe(true); + }); + }); +}); 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 new file mode 100644 index 00000000..5c47e070 --- /dev/null +++ b/apps/frontend/src/renderer/components/github-prs/components/__tests__/PRDetail.integration.test.tsx @@ -0,0 +1,347 @@ +/** + * Integration tests for PRDetail clean review state reset on PR change + * Tests that cleanReviewPosted state resets when pr.number changes + * + * @vitest-environment jsdom + */ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { render, screen, waitFor, fireEvent } from '@testing-library/react'; +import '@testing-library/jest-dom/vitest'; +import { I18nextProvider } from 'react-i18next'; +import i18n from '../../../../../shared/i18n'; +import { PRDetail } from '../PRDetail'; +import type { PRData, PRReviewResult } from '../../hooks/useGitHubPRs'; + +// Mock window.electronAPI +type PostCommentFn = (body: string) => void | Promise; +const mockOnPostComment = vi.fn(); +const mockOnPostReview = vi.fn(); +const mockOnRunReview = vi.fn(); +const mockOnRunFollowupReview = vi.fn(); +const mockOnCheckNewCommits = vi.fn(); +const mockOnCancelReview = vi.fn(); +const mockOnMergePR = vi.fn(); +const mockOnAssignPR = vi.fn(); +const mockOnGetLogs = vi.fn(); + +Object.defineProperty(window, 'electronAPI', { + value: { + github: { + getWorkflowsAwaitingApproval: vi.fn().mockResolvedValue({ + awaiting_approval: 0, + workflow_runs: [] + }), + checkMergeReadiness: vi.fn().mockResolvedValue({ + blockers: [] + }) + } + } +}); + +// Create a mock PR data +function createMockPR(overrides: Partial = {}): PRData { + return { + number: 123, + title: 'Test PR', + body: 'Test PR body', + state: 'open', + author: { login: 'testuser' }, + headRefName: 'feature-branch', + baseRefName: 'main', + additions: 100, + deletions: 50, + changedFiles: 5, + assignees: [], + files: [], + createdAt: '2024-01-01T00:00:00Z', + updatedAt: '2024-01-01T00:00:00Z', + htmlUrl: 'https://github.com/test/repo/pull/123', + ...overrides + }; +} + +// Create a mock clean review result +function createMockCleanReviewResult(overrides: Partial = {}): PRReviewResult { + return { + prNumber: 123, + repo: 'test/repo', + success: true, + overallStatus: 'approve', + summary: 'All code passes review. No issues found.', + findings: [], + reviewedAt: '2024-01-01T00:00:00Z', + reviewedCommitSha: 'abc123', + ...overrides + }; +} + +// Wrapper component for i18n +function I18nWrapper({ children }: { children: React.ReactNode }) { + return {children}; +} + +describe('PRDetail - Clean Review State Reset Integration', () => { + const mockProjectId = 'test-project-id'; + + // Helper function to render PRDetail with common default props + function renderPRDetail(overrides: { + pr?: PRData; + reviewResult?: PRReviewResult; + onPostComment?: PostCommentFn; + } = {}) { + const defaultPR = createMockPR({ number: 123 }); + const defaultReviewResult = createMockCleanReviewResult(); + + return render( + + + + ); + } + + beforeEach(() => { + vi.clearAllMocks(); + + // Setup default mock return values + mockOnGetLogs.mockResolvedValue(null); + mockOnCheckNewCommits.mockResolvedValue({ + hasNewCommits: false, + hasCommitsAfterPosting: false, + newCommitCount: 0 + }); + // Resolve successfully by default + mockOnPostComment.mockResolvedValue(undefined); + }); + + it('should reset cleanReviewPosted state when pr.number changes', async () => { + const initialPR = createMockPR({ number: 123 }); + const cleanReviewResult = createMockCleanReviewResult(); + + const { rerender, unmount } = renderPRDetail({ + pr: initialPR, + reviewResult: cleanReviewResult + }); + + // The "Post Clean Review" button should be visible initially + const postCleanReviewButton = screen.getByRole('button', { name: /post clean review/i }); + expect(postCleanReviewButton).toBeInTheDocument(); + + // Click the button to post clean review + fireEvent.click(postCleanReviewButton); + + // Wait for success message to appear (confirms cleanReviewPosted is true) + await waitFor(() => { + expect(screen.getByText(/clean review posted/i)).toBeInTheDocument(); + }); + + // Button should be hidden after posting + expect(screen.queryByRole('button', { name: /post clean review/i })).not.toBeInTheDocument(); + + // Rerender with a different PR (number 456) + const differentPR = createMockPR({ number: 456 }); + rerender( + + + + ); + + // After PR change, the "Post Clean Review" button should be visible again + // because cleanReviewPosted state was reset by useEffect when pr.number changed + const postCleanReviewButtonAfterChange = screen.queryByRole('button', { name: /post clean review/i }); + expect(postCleanReviewButtonAfterChange).toBeInTheDocument(); + unmount(); + }); + + it('should show clean review success message after posting clean review', async () => { + const { unmount } = renderPRDetail(); + + // Initially, the success message should not be present + const successMessage = screen.queryByText(/clean review posted/i); + expect(successMessage).not.toBeInTheDocument(); + + // The "Post Clean Review" button should be visible + const postCleanReviewButton = screen.getByRole('button', { name: /post clean review/i }); + expect(postCleanReviewButton).toBeInTheDocument(); + + // Click the button to post clean review + fireEvent.click(postCleanReviewButton); + + // Wait for success message to appear + await waitFor(() => { + expect(screen.getByText(/clean review posted/i)).toBeInTheDocument(); + }); + + // Button should be hidden after posting + expect(screen.queryByRole('button', { name: /post clean review/i })).not.toBeInTheDocument(); + + unmount(); + }); + + it('should not show Post Clean Review button when review has HIGH severity findings', async () => { + const reviewWithHighFindings: PRReviewResult = { + prNumber: 123, + repo: 'test/repo', + success: true, + overallStatus: 'request_changes', + summary: 'Found high severity issues.', + reviewedAt: '2024-01-01T00:00:00Z', + findings: [ + { + id: 'finding-1', + severity: 'high', + category: 'security', + title: 'Security Issue', + file: 'src/test.ts', + line: 10, + description: 'High severity issue', + fixable: true + } + ], + reviewedCommitSha: 'abc123' + }; + + const { unmount } = renderPRDetail({ reviewResult: reviewWithHighFindings }); + + // The "Post Clean Review" button should NOT be visible for dirty reviews + const postCleanReviewButton = screen.queryByRole('button', { name: /post clean review/i }); + expect(postCleanReviewButton).not.toBeInTheDocument(); + + unmount(); + }); + + it('should show correct button state based on review cleanliness', async () => { + const cleanReviewResult = createMockCleanReviewResult(); + const initialPR = createMockPR({ number: 123 }); + + // Test 1: Clean review (no findings) + const { rerender, unmount } = renderPRDetail({ + pr: initialPR, + reviewResult: cleanReviewResult + }); + + // Clean review: Post Clean Review button should be visible + const postCleanReviewButton = screen.queryByRole('button', { name: /post clean review/i }); + expect(postCleanReviewButton).toBeInTheDocument(); + + // Test 2: Dirty review (HIGH findings) + const dirtyReviewResult: PRReviewResult = { + prNumber: 123, + repo: 'test/repo', + success: true, + overallStatus: 'request_changes', + summary: 'Found issues.', + reviewedAt: '2024-01-01T00:00:00Z', + findings: [ + { + id: 'finding-1', + severity: 'high', + category: 'security', + title: 'Security Issue', + file: 'src/test.ts', + line: 10, + description: 'High severity issue', + fixable: true + } + ], + reviewedCommitSha: 'abc123' + }; + + rerender( + + + + ); + + // Dirty review: Post Clean Review button should NOT be visible + const postCleanReviewButtonDirty = screen.queryByRole('button', { name: /post clean review/i }); + expect(postCleanReviewButtonDirty).not.toBeInTheDocument(); + + unmount(); + }); + + it('should show error message when posting clean review fails', async () => { + // Mock onPostComment to reject + const testError = new Error('Failed to post comment: Rate limit exceeded'); + mockOnPostComment.mockRejectedValue(testError); + + const { unmount } = renderPRDetail({ + onPostComment: mockOnPostComment + }); + + // The "Post Clean Review" button should be visible initially + const postCleanReviewButton = screen.getByRole('button', { name: /post clean review/i }); + expect(postCleanReviewButton).toBeInTheDocument(); + + // Click the button to attempt posting clean review + fireEvent.click(postCleanReviewButton); + + // Wait for normalized error message to appear (shows friendly message, not raw error) + await waitFor(() => { + expect(screen.getByText(/Failed to post clean review/i)).toBeInTheDocument(); + }); + + // "View details" button should be available + await waitFor(() => { + expect(screen.getByText(/View details/i)).toBeInTheDocument(); + }); + + // Button should still be visible for retry after error + expect(screen.queryByRole('button', { name: /post clean review/i })).toBeInTheDocument(); + + // Success message should NOT be shown + expect(screen.queryByText(/clean review posted/i)).not.toBeInTheDocument(); + + unmount(); + }); +}); 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 634ec214..8ddadb99 100644 --- a/apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts +++ b/apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts @@ -30,9 +30,9 @@ interface UseGitHubPRsResult { selectedPRNumber: number | null; reviewResult: PRReviewResult | null; reviewProgress: PRReviewProgress | null; + startedAt: string | null; isReviewing: boolean; previousReviewResult: PRReviewResult | null; - startedAt: string | null; isConnected: boolean; repoFullName: string | null; activePRReviews: number[]; // PR numbers currently being reviewed @@ -506,9 +506,9 @@ export function useGitHubPRs( selectedPRNumber, reviewResult, reviewProgress, + startedAt, isReviewing, previousReviewResult, - startedAt, isConnected, repoFullName, activePRReviews, diff --git a/apps/frontend/src/renderer/stores/github/pr-review-store.ts b/apps/frontend/src/renderer/stores/github/pr-review-store.ts index e3a78eb3..a592c9df 100644 --- a/apps/frontend/src/renderer/stores/github/pr-review-store.ts +++ b/apps/frontend/src/renderer/stores/github/pr-review-store.ts @@ -129,7 +129,7 @@ export const usePRReviewStore = create((set, get) => ({ prNumber: result.prNumber, projectId, isReviewing: false, - startedAt: null, + startedAt: existing?.startedAt ?? null, progress: null, result, previousResult: existing?.previousResult ?? null, @@ -152,7 +152,7 @@ export const usePRReviewStore = create((set, get) => ({ prNumber, projectId, isReviewing: false, - startedAt: null, + startedAt: existing?.startedAt ?? null, progress: null, result: existing?.result ?? null, previousResult: existing?.previousResult ?? null, diff --git a/apps/frontend/src/shared/i18n/locales/en/common.json b/apps/frontend/src/shared/i18n/locales/en/common.json index 377c7e93..2cde7094 100644 --- a/apps/frontend/src/shared/i18n/locales/en/common.json +++ b/apps/frontend/src/shared/i18n/locales/en/common.json @@ -294,7 +294,16 @@ "workflowsAwaitingDescription": "This PR is from a fork and requires workflow approval before CI checks can run. Approve the workflows to continue.", "viewOnGitHub": "View", "approveWorkflow": "Approve", - "approveAllWorkflows": "Approve All Workflows" + "approveAllWorkflows": "Approve All Workflows", + "postCleanReview": "Post Clean Review", + "postingCleanReview": "Posting...", + "cleanReviewPosted": "Clean review posted", + "cleanReviewMessageTitle": "## ✅ Auto Claude PR Review - PASSED", + "cleanReviewMessageStatus": "**Status:** All code is good", + "cleanReviewMessageFooter": "*This automated review found no issues. Generated by Auto Claude.*", + "failedPostCleanReview": "Failed to post clean review", + "viewErrorDetails": "View details", + "hideErrorDetails": "Hide details" }, "downloads": { "toggleExpand": "Toggle download details", diff --git a/apps/frontend/src/shared/i18n/locales/fr/common.json b/apps/frontend/src/shared/i18n/locales/fr/common.json index 623b260e..7371ec4b 100644 --- a/apps/frontend/src/shared/i18n/locales/fr/common.json +++ b/apps/frontend/src/shared/i18n/locales/fr/common.json @@ -294,7 +294,16 @@ "workflowsAwaitingDescription": "Cette PR provient d'un fork et nécessite l'approbation des workflows avant que les vérifications CI puissent s'exécuter. Approuvez les workflows pour continuer.", "viewOnGitHub": "Voir", "approveWorkflow": "Approuver", - "approveAllWorkflows": "Approuver tous les workflows" + "approveAllWorkflows": "Approuver tous les workflows", + "postCleanReview": "Publier révision propre", + "postingCleanReview": "Publication...", + "cleanReviewPosted": "Révision propre publiée", + "cleanReviewMessageTitle": "## ✅ Auto Claude PR Review - PASSED", + "cleanReviewMessageStatus": "**Status:** All code is good", + "cleanReviewMessageFooter": "*This automated review found no issues. Generated by Auto Claude.*", + "failedPostCleanReview": "Échec de la publication de la révision", + "viewErrorDetails": "Voir les détails", + "hideErrorDetails": "Masquer les détails" }, "downloads": { "toggleExpand": "Afficher/masquer les détails",