fix(ui): add Post Clean Review button for clean PR reviews (ACS-201) (#894)
* fix(ui): add Post Clean Review button for clean PR reviews (ACS-201) When a PR review completes with no findings or only LOW severity findings, users can now post a clean review comment to GitHub. This posts a COMMENT (not APPROVE/REQUEST_CHANGES) to document the review without changing the PR's review status. Changes: - Add "Post Clean Review" button when review is clean and no findings selected - Add translation keys for clean review button (en/fr) - Add 29 unit tests for clean review functionality - Reset clean review posted state when PR changes Affected files: - src/renderer/components/github-prs/components/PRDetail.tsx - src/shared/i18n/locales/en/common.json - src/shared/i18n/locales/fr/common.json - src/renderer/components/github-prs/components/__tests__/PRDetail.cleanReview.test.ts * test: improve clean review tests with integration tests and error handling - Add error handling (try/catch) to handlePostCleanReview function - Add PRDetail.integration.test.tsx with React Testing Library integration tests - Update PRDetail.cleanReview.test.ts with improved state reset tests - Tests verify cleanReviewPosted state resets when pr.number changes - Tests verify button visibility based on review cleanliness * test: fix TypeScript errors in integration tests - Add required prNumber and repo to PRReviewResult mocks - Add required title and fixable to PRReviewFinding mocks * fix: add error handling and improve integration tests PRDetail.tsx: - Add cleanReviewError state for tracking posting errors - Update handlePostCleanReview with proper try/catch/finally - Set error message on failure, clear on success - Display error in UI (red text with XCircle icon) - Reset error state when PR changes PRDetail.integration.test.tsx: - Add renderPRDetail helper function to reduce code duplication - Update first test to actually click button and verify success message - Add error handling test for failed clean review posts - Use fireEvent instead of userEvent (not installed) - Tests verify full flow: click → wait for success → verify state reset * fix: resolve TypeScript errors in integration test - Import PRReviewResult from correct path (useGitHubPRs) - Fix mock type to avoid explicit any warning * fix(tests): resolve TypeScript errors in PRDetail integration test Fixed Mock type assignment errors by: - Defining PostCommentFn type alias matching (body: string) => void | Promise<void> - Using vi.fn<PostCommentFn>() for properly typed mock - Updating overrides.onPostComment type in renderPRDetail helper Resolves CI TypeScript errors on lines 108, 171, 283. * i18n: replace hardcoded clean review message with translation keys Replace the inline cleanReviewMessage construction in handlePostCleanReview with i18n translation keys for better localization support. Changes: - Added cleanReviewMessageTitle, cleanReviewMessageStatus, and cleanReviewMessageFooter keys to en/common.json - Added corresponding French translations to fr/common.json - Updated handlePostCleanReview to compose message from translation keys - Removed comment about English being lingua franca (now translatable) Error handling and behavior remain unchanged. * fix: improve clean review error handling and prevent state leaks This commit addresses several issues with the clean review functionality: Error Display (line 832-860): - Replace raw error display with normalized/friendly message - Add "View details"/"Hide details" toggle for full error text - Use translation key 'failedPostCleanReview' for user-facing message - Log full error to console before rendering for debugging Race Condition Prevention (line 737, 760): - Post Clean Review button now checks (isPostingCleanReview || isPosting) - Approve button now checks (isPosting || isPostingCleanReview) - Both buttons disable during either posting operation State Leak Prevention (line 542-645): - Capture current pr.number at start of all posting handlers - Guard all setState calls with pr.number === currentPr check - Reset isPostingCleanReview in PR change effect (line 266) - Use Promise.resolve() for onPostComment to handle non-Promise implementations Locale Updates: - Keep GitHub PR comments in English-only (lingua franca policy) - French locale now uses same English text for comment messages - Add French translations for UI error messages Test Updates: - Update integration test to check for normalized error message - Add assertion for "View details" button presence All posting handlers now consistently: - Capture PR number before async operations - Guard state updates against PR changes - Clear loading state only if PR hasn't changed * test: fix test issues and add accessibility attributes Fixes for code review findings: Test Fixes: - Fix test 'should show clean review success message after posting clean review' to actually click the button and verify the success message appears - Add documentation to unit tests clarifying they test algorithm, not React behavior - Add JSDoc comment explaining algorithm tests vs integration tests Accessibility: - Add useId import and generate stable ID for error details - Add aria-expanded and aria-controls attributes to error toggle button - Add corresponding id to error details div for proper accessibility Documentation: - Add comment explaining inline error pattern vs Card-based pattern - Document why action bar errors use inline layout for consistency All 35 tests pass. * feat: add startedAt prop to match upstream develop branch Add startedAt: string | null property throughout the PR review state chain: - PRDetailProps interface - PRReviewState interface in pr-review-store.ts - All store actions (startPRReview, startFollowupReview, setPRReviewProgress, setPRReviewResult, setPRReviewError, setNewCommitsCheck) - UseGitHubPRsResult interface and hook extraction/return - GitHubPRs.tsx destructuring and JSX props - All PRDetail renders in integration tests This aligns with upstream develop branch changes. * fix: remove duplicate startedAt declarations --------- Co-authored-by: StillKnotKnown <stillknotknown@users.noreply.github.com> Co-authored-by: Andy <119136210+AndyMik90@users.noreply.github.com>
This commit is contained in:
StillKnotKnown
@@ -64,9 +64,9 @@ export function GitHubPRs({ onOpenSettings, isActive = false }: GitHubPRsProps)
|
||||
selectedPRNumber,
|
||||
reviewResult,
|
||||
reviewProgress,
|
||||
startedAt,
|
||||
isReviewing,
|
||||
previousReviewResult,
|
||||
startedAt,
|
||||
hasMore,
|
||||
selectPR,
|
||||
runReview,
|
||||
|
||||
@@ -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<string | null>(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<NewCommitsCheck | null>(initialNewCommitsCheck ?? null);
|
||||
@@ -116,6 +122,9 @@ export function PRDetail({
|
||||
const [isApprovingWorkflow, setIsApprovingWorkflow] = useState<number | null>(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({
|
||||
</Button>
|
||||
)}
|
||||
|
||||
{/* Post Clean Review button - shows when review is clean and no findings are selected */}
|
||||
{selectedCount === 0 && isCleanReview && !hasPostedFindings && !cleanReviewPosted && reviewResult?.overallStatus !== 'request_changes' && (
|
||||
<Button
|
||||
onClick={handlePostCleanReview}
|
||||
disabled={isPostingCleanReview || isPosting}
|
||||
variant="secondary"
|
||||
className="flex-1 sm:flex-none"
|
||||
>
|
||||
{isPostingCleanReview ? (
|
||||
<>
|
||||
<Loader2 className="h-4 w-4 mr-2 animate-spin" />
|
||||
{t('prReview.postingCleanReview')}
|
||||
</>
|
||||
) : (
|
||||
<>
|
||||
<MessageSquare className="h-4 w-4 mr-2" />
|
||||
{t('prReview.postCleanReview')}
|
||||
</>
|
||||
)}
|
||||
</Button>
|
||||
)}
|
||||
|
||||
{/* 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' && (
|
||||
<Button
|
||||
onClick={handleAutoApprove}
|
||||
disabled={isPosting}
|
||||
disabled={isPosting || isPostingCleanReview}
|
||||
variant="default"
|
||||
className="flex-1 sm:flex-none bg-emerald-600 hover:bg-emerald-700 text-white"
|
||||
>
|
||||
@@ -746,6 +851,52 @@ export function PRDetail({
|
||||
{t('prReview.postedFindings', { count: postSuccess.count })}
|
||||
</div>
|
||||
)}
|
||||
|
||||
{cleanReviewPosted && !postSuccess && (
|
||||
<div className="ml-auto flex items-center gap-2 text-emerald-600 text-sm font-medium animate-pulse">
|
||||
<CheckCircle className="h-4 w-4" />
|
||||
{t('prReview.cleanReviewPosted')}
|
||||
</div>
|
||||
)}
|
||||
|
||||
{/* Clean review error display - inline pattern for action bar context */}
|
||||
{/* Note: Uses inline layout (not Card) to match other action bar status messages.
|
||||
Separate Card-based error at line 972 handles review result errors. */}
|
||||
{cleanReviewError && (
|
||||
<div className="ml-auto flex items-center gap-2">
|
||||
<div className="flex items-center gap-2 text-destructive text-sm font-medium">
|
||||
<XCircle className="h-4 w-4" />
|
||||
{t('prReview.failedPostCleanReview')}
|
||||
</div>
|
||||
<button
|
||||
onClick={() => setShowCleanReviewErrorDetails(!showCleanReviewErrorDetails)}
|
||||
aria-expanded={showCleanReviewErrorDetails}
|
||||
aria-controls={cleanReviewErrorDetailsId}
|
||||
className="flex items-center gap-1 text-xs text-muted-foreground hover:text-foreground transition-colors"
|
||||
>
|
||||
{showCleanReviewErrorDetails ? (
|
||||
<>
|
||||
{t('prReview.hideErrorDetails')}
|
||||
<ChevronUp className="h-3 w-3" />
|
||||
</>
|
||||
) : (
|
||||
<>
|
||||
{t('prReview.viewErrorDetails')}
|
||||
<ChevronDown className="h-3 w-3" />
|
||||
</>
|
||||
)}
|
||||
</button>
|
||||
</div>
|
||||
)}
|
||||
{cleanReviewError && showCleanReviewErrorDetails && (
|
||||
<div
|
||||
id={cleanReviewErrorDetailsId}
|
||||
className="ml-auto text-xs text-muted-foreground max-w-md truncate"
|
||||
title={cleanReviewError}
|
||||
>
|
||||
{cleanReviewError}
|
||||
</div>
|
||||
)}
|
||||
</div>
|
||||
)}
|
||||
|
||||
|
||||
+658
@@ -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> = {}): 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);
|
||||
});
|
||||
});
|
||||
});
|
||||
+347
@@ -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<void>;
|
||||
const mockOnPostComment = vi.fn<PostCommentFn>();
|
||||
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> = {}): 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> = {}): 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 <I18nextProvider i18n={i18n}>{children}</I18nextProvider>;
|
||||
}
|
||||
|
||||
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(
|
||||
<I18nWrapper>
|
||||
<PRDetail
|
||||
pr={overrides.pr ?? defaultPR}
|
||||
projectId={mockProjectId}
|
||||
reviewResult={overrides.reviewResult ?? defaultReviewResult}
|
||||
previousReviewResult={null}
|
||||
reviewProgress={null}
|
||||
startedAt={null}
|
||||
isReviewing={false}
|
||||
onRunReview={mockOnRunReview}
|
||||
onRunFollowupReview={mockOnRunFollowupReview}
|
||||
onCheckNewCommits={mockOnCheckNewCommits}
|
||||
onCancelReview={mockOnCancelReview}
|
||||
onPostReview={mockOnPostReview}
|
||||
onPostComment={overrides.onPostComment ?? mockOnPostComment}
|
||||
onMergePR={mockOnMergePR}
|
||||
onAssignPR={mockOnAssignPR}
|
||||
onGetLogs={mockOnGetLogs}
|
||||
/>
|
||||
</I18nWrapper>
|
||||
);
|
||||
}
|
||||
|
||||
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(
|
||||
<I18nWrapper>
|
||||
<PRDetail
|
||||
pr={differentPR}
|
||||
projectId={mockProjectId}
|
||||
reviewResult={cleanReviewResult}
|
||||
previousReviewResult={null}
|
||||
reviewProgress={null}
|
||||
startedAt={null}
|
||||
isReviewing={false}
|
||||
onRunReview={mockOnRunReview}
|
||||
onRunFollowupReview={mockOnRunFollowupReview}
|
||||
onCheckNewCommits={mockOnCheckNewCommits}
|
||||
onCancelReview={mockOnCancelReview}
|
||||
onPostReview={mockOnPostReview}
|
||||
onPostComment={mockOnPostComment}
|
||||
onMergePR={mockOnMergePR}
|
||||
onAssignPR={mockOnAssignPR}
|
||||
onGetLogs={mockOnGetLogs}
|
||||
/>
|
||||
</I18nWrapper>
|
||||
);
|
||||
|
||||
// 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(
|
||||
<I18nWrapper>
|
||||
<PRDetail
|
||||
pr={initialPR}
|
||||
projectId={mockProjectId}
|
||||
reviewResult={dirtyReviewResult}
|
||||
previousReviewResult={null}
|
||||
reviewProgress={null}
|
||||
startedAt={null}
|
||||
isReviewing={false}
|
||||
onRunReview={mockOnRunReview}
|
||||
onRunFollowupReview={mockOnRunFollowupReview}
|
||||
onCheckNewCommits={mockOnCheckNewCommits}
|
||||
onCancelReview={mockOnCancelReview}
|
||||
onPostReview={mockOnPostReview}
|
||||
onPostComment={mockOnPostComment}
|
||||
onMergePR={mockOnMergePR}
|
||||
onAssignPR={mockOnAssignPR}
|
||||
onGetLogs={mockOnGetLogs}
|
||||
/>
|
||||
</I18nWrapper>
|
||||
);
|
||||
|
||||
// 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();
|
||||
});
|
||||
});
|
||||
@@ -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,
|
||||
|
||||
@@ -129,7 +129,7 @@ export const usePRReviewStore = create<PRReviewStoreState>((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<PRReviewStoreState>((set, get) => ({
|
||||
prNumber,
|
||||
projectId,
|
||||
isReviewing: false,
|
||||
startedAt: null,
|
||||
startedAt: existing?.startedAt ?? null,
|
||||
progress: null,
|
||||
result: existing?.result ?? null,
|
||||
previousResult: existing?.previousResult ?? null,
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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",
|
||||
|
||||
Reference in New Issue
Block a user