diff --git a/state.db b/state.db index 3eca06d5..4411bc2e 100644 Binary files a/state.db and b/state.db differ diff --git a/tests/e2e/test_review_ui.spec.ts b/tests/e2e/test_review_ui.spec.ts index 1c94dd70..9bb6e56e 100644 --- a/tests/e2e/test_review_ui.spec.ts +++ b/tests/e2e/test_review_ui.spec.ts @@ -56,9 +56,8 @@ test.describe('Review Findings UI', () => { } }); - test.skip('should expand/collapse review finding details', async ({ page }) => { - // SKIP: Individual review findings with expand/collapse not implemented in current ReviewSummary - // Current implementation shows severity counts, not individual findings + test('should expand/collapse review finding details', async ({ page }) => { + // Individual review findings with expand/collapse now implemented in ReviewSummary const findingsList = page.locator('[data-testid="review-findings-list"]'); @@ -79,9 +78,8 @@ test.describe('Review Findings UI', () => { } }); - test.skip('should filter findings by severity', async ({ page }) => { - // SKIP: Severity filter not implemented in current ReviewSummary - // Current implementation shows all severity counts without filtering + test('should filter findings by severity', async ({ page }) => { + // Severity filter now implemented in ReviewSummary const severityFilter = page.locator('[data-testid="severity-filter"]'); @@ -108,9 +106,8 @@ test.describe('Review Findings UI', () => { } }); - test.skip('should display actionable recommendations', async ({ page }) => { - // SKIP: Individual finding recommendations not implemented in current ReviewSummary - // Current implementation shows aggregate severity/category counts + test('should display actionable recommendations', async ({ page }) => { + // Individual finding recommendations now implemented in ReviewSummary const findingsList = page.locator('[data-testid="review-findings-list"]'); const firstFinding = findingsList.locator('[data-testid^="review-finding-"]').first(); @@ -126,6 +123,18 @@ test.describe('Review Findings UI', () => { const recText = await recommendation.textContent(); expect(recText).toBeTruthy(); expect(recText!.length).toBeGreaterThan(10); + + // Verify lightbulb icon is present (Issue #6 - Enhanced test coverage) + const icon = recommendation.locator('span[aria-hidden="true"]').filter({ hasText: '💡' }); + await expect(icon).toBeVisible(); + + // Verify blue background styling is applied (Issue #6 - Enhanced test coverage) + const bgColor = await recommendation.evaluate((el) => { + const styles = window.getComputedStyle(el); + return styles.backgroundColor; + }); + // Should have blue-ish background (rgb values for bg-blue-50) + expect(bgColor).toMatch(/rgb\(239,\s*246,\s*255\)/); } }); }); diff --git a/web-ui/src/components/reviews/ReviewSummary.tsx b/web-ui/src/components/reviews/ReviewSummary.tsx index 7915abe2..55ba10bf 100644 --- a/web-ui/src/components/reviews/ReviewSummary.tsx +++ b/web-ui/src/components/reviews/ReviewSummary.tsx @@ -9,9 +9,9 @@ * Tasks: T037 */ -import React, { useMemo } from 'react'; -import type { ReviewResult, Severity, ReviewCategory } from '../../types/reviews'; -import { CATEGORY_ICONS } from '../../types/reviews'; +import React, { useMemo, useState } from 'react'; +import type { ReviewResult, Severity, ReviewCategory, CodeReview } from '../../types/reviews'; +import { CATEGORY_ICONS, SEVERITY_COLORS } from '../../types/reviews'; interface ReviewSummaryProps { /** Review result data */ @@ -24,6 +24,128 @@ interface ReviewSummaryProps { error?: string | null; } +/** + * Individual Finding Card Component (Memoized for performance) + * Addresses Issue #1 (Performance) and #2 (Accessibility) + */ +interface FindingCardProps { + finding: CodeReview; + index: number; + isExpanded: boolean; + onToggle: (id: number) => void; +} + +const FindingCard = React.memo(({ finding, index, isExpanded, onToggle }: FindingCardProps) => { + // Use ID if available, fallback to index to avoid collisions (Issue #3) + const findingId = finding.id ?? index; + + // Defensive check for severity color (Issue #5) + const severityColor = SEVERITY_COLORS[finding.severity as Severity] || 'bg-gray-100 text-gray-800 border-gray-300'; + + // Defensive check for category icon (Issue #5) + const categoryIcon = CATEGORY_ICONS[finding.category as ReviewCategory] || '📄'; + + // Keyboard event handler for accessibility (Issue #2) + const handleKeyDown = (e: React.KeyboardEvent) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + onToggle(findingId); + } + }; + + return ( +
onToggle(findingId)} + onKeyDown={handleKeyDown} + data-testid={`review-finding-${findingId}`} + > + {/* Finding Header */} +
+
+
+ + {finding.file_path} + {finding.line_number && `:${finding.line_number}`} + +
+

{finding.message}

+
+
+ + + {finding.severity} + +
+
+ + {/* Expanded Details */} + {isExpanded && ( +
+ {/* Full Message (if needed) */} + {finding.message && ( +
+

Details:

+

{finding.message}

+
+ )} + + {/* Recommendation */} + {finding.recommendation && ( +
+
+ +
+

+ Recommendation: +

+

{finding.recommendation}

+
+
+
+ )} + + {/* Code Snippet */} + {finding.code_snippet && ( +
+

Code:

+ 
{finding.code_snippet}
+
+ )} + + {/* File Details */} +
+ File: {finding.file_path} + {finding.line_number && ( + <> + {' '} + Line: {finding.line_number} + + )} + {' '} + Category:{' '} + {finding.category} +
+
+ )} +
+ ); +}); + +FindingCard.displayName = 'FindingCard'; + /** * Display review summary statistics and blocking status */ @@ -46,6 +168,32 @@ export function ReviewSummary({ ); }, [reviewResult]); + // State for expand/collapse individual findings + const [expandedFindings, setExpandedFindings] = useState>(new Set()); + + // State for severity filter + const [severityFilter, setSeverityFilter] = useState('all'); + + // Filter findings based on selected severity + const filteredFindings = useMemo(() => { + if (!reviewResult) return []; + if (severityFilter === 'all') return reviewResult.findings; + return reviewResult.findings.filter((finding) => finding.severity === severityFilter); + }, [reviewResult, severityFilter]); + + // Toggle finding expansion + const toggleFinding = (findingId: number) => { + setExpandedFindings((prev) => { + const newSet = new Set(prev); + if (newSet.has(findingId)) { + newSet.delete(findingId); + } else { + newSet.add(findingId); + } + return newSet; + }); + }; + // Loading state if (loading) { return ( @@ -68,14 +216,20 @@ export function ReviewSummary({ ); } - // Empty state (no review data) + // Empty state (no review data) - still render container with findings list placeholder if (!reviewResult) { return (

Review Summary

-
+
No review data available. Trigger a code review to see results.
+ {/* Always render review-findings-list container for test consistency */} +
+
+ No review findings yet. +
+
); } @@ -146,8 +300,8 @@ export function ReviewSummary({ )}
- {/* Review Findings List (severity breakdown serves as findings list) */} -
+ {/* Severity Breakdown */} +

By Severity

{(['critical', 'high', 'medium', 'low', 'info'] as Severity[]).map( @@ -194,7 +348,7 @@ export function ReviewSummary({
{/* Category Breakdown */} -
+

By Category

{( @@ -229,6 +383,60 @@ export function ReviewSummary({ })}
+ + {/* Individual Findings Section */} +
+ {/* Severity Filter - only show if there are findings */} + {reviewResult.findings.length > 0 && ( +
+ + +
+ )} + + {/* Findings List - always rendered */} +
+ {reviewResult.findings.length === 0 ? ( +
+ No review findings. All code reviews will appear here. +
+ ) : filteredFindings.length === 0 ? ( +
+ No findings match the selected filter. +
+ ) : ( + filteredFindings.map((finding, index) => { + const findingId = finding.id ?? index; + const isExpanded = expandedFindings.has(findingId); + + return ( + + ); + }) + )} +
+
); }