feat: Implement detailed Review Findings UI with filtering and recommendations

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\)/);
     }
   });
 });

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 (
+    <div
+      role="button"
+      tabIndex={0}
+      aria-expanded={isExpanded}
+      aria-label={`${finding.severity} severity finding in ${finding.file_path}${finding.line_number ? ` line ${finding.line_number}` : ''}`}
+      className={`finding-card border-2 rounded-lg p-4 cursor-pointer transition-all hover:shadow-md focus:ring-2 focus:ring-blue-500 focus:outline-none ${severityColor}`}
+      onClick={() => onToggle(findingId)}
+      onKeyDown={handleKeyDown}
+      data-testid={`review-finding-${findingId}`}
+    >
+      {/* Finding Header */}
+      <div className="flex items-start justify-between">
+        <div className="flex-1">
+          <div className="flex items-center gap-2 mb-2">
+            <code className="text-sm font-mono bg-white bg-opacity-50 px-2 py-1 rounded">
+              {finding.file_path}
+              {finding.line_number && `:${finding.line_number}`}
+            </code>
+          </div>
+          <p className="text-sm font-medium">{finding.message}</p>
+        </div>
+        <div className="flex items-center gap-2 ml-4">
+          <span className="text-lg" title={finding.category} aria-hidden="true">
+            {categoryIcon}
+          </span>
+          <span
+            className="text-xs font-semibold uppercase px-2 py-1 bg-white bg-opacity-70 rounded border"
+            data-testid="severity-badge"
+          >
+            {finding.severity}
+          </span>
+        </div>
+      </div>
+
+      {/* Expanded Details */}
+      {isExpanded && (
+        <div className="finding-details mt-4 space-y-3" data-testid="finding-details">
+          {/* Full Message (if needed) */}
+          {finding.message && (
+            <div className="bg-white bg-opacity-50 rounded p-3">
+              <p className="text-xs font-semibold text-gray-600 mb-1">Details:</p>
+              <p className="text-sm">{finding.message}</p>
+            </div>
+          )}
+
+          {/* Recommendation */}
+          {finding.recommendation && (
+            <div
+              className="bg-blue-50 border border-blue-200 rounded p-3"
+              data-testid="finding-recommendation"
+            >
+              <div className="flex items-start gap-2">
+                <span className="text-blue-600 text-lg" aria-hidden="true">💡</span>
+                <div>
+                  <p className="text-xs font-semibold text-blue-800 mb-1">
+                    Recommendation:
+                  </p>
+                  <p className="text-sm text-blue-900">{finding.recommendation}</p>
+                </div>
+              </div>
+            </div>
+          )}
+
+          {/* Code Snippet */}
+          {finding.code_snippet && (
+            <div className="bg-gray-900 text-gray-100 rounded p-3 overflow-x-auto">
+              <p className="text-xs font-semibold text-gray-400 mb-2">Code:</p>
+              <pre className="text-xs font-mono">{finding.code_snippet}</pre>
+            </div>
+          )}
+
+          {/* File Details */}
+          <div className="text-xs text-gray-600 bg-white bg-opacity-50 rounded p-2">
+            <span className="font-semibold">File:</span> {finding.file_path}
+            {finding.line_number && (
+              <>
+                {' '}
+                <span className="font-semibold">Line:</span> {finding.line_number}
+              </>
+            )}
+            {' '}
+            <span className="font-semibold">Category:</span>{' '}
+            <span className="capitalize">{finding.category}</span>
+          </div>
+        </div>
+      )}
+    </div>
+  );
+});
+
+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<Set<number>>(new Set());
+
+  // State for severity filter
+  const [severityFilter, setSeverityFilter] = useState<Severity | 'all'>('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 (
       <div className="review-summary" data-testid="review-summary">
         <h3 className="text-lg font-semibold mb-4">Review Summary</h3>
-        <div className="text-gray-500 bg-gray-50 p-4 rounded">
+        <div className="text-gray-500 bg-gray-50 p-4 rounded mb-6">
           No review data available. Trigger a code review to see results.
         </div>
+        {/* Always render review-findings-list container for test consistency */}
+        <div className="review-findings-list" data-testid="review-findings-list">
+          <div className="text-gray-500 bg-gray-50 p-4 rounded text-center">
+            No review findings yet.
+          </div>
+        </div>
       </div>
     );
   }
@@ -146,8 +300,8 @@ export function ReviewSummary({
         )}
       </div>
 
-      {/* Review Findings List (severity breakdown serves as findings list) */}
-      <div className="severity-breakdown mb-6" data-testid="review-findings-list">
+      {/* Severity Breakdown */}
+      <div className="severity-breakdown mb-6">
         <h4 className="text-md font-semibold mb-3">By Severity</h4>
         <div className="space-y-2">
           {(['critical', 'high', 'medium', 'low', 'info'] as Severity[]).map(
@@ -194,7 +348,7 @@ export function ReviewSummary({
       </div>
 
       {/* Category Breakdown */}
-      <div className="category-breakdown">
+      <div className="category-breakdown mb-6">
         <h4 className="text-md font-semibold mb-3">By Category</h4>
         <div className="grid grid-cols-2 gap-2">
           {(
@@ -229,6 +383,60 @@ export function ReviewSummary({
           })}
         </div>
       </div>
+
+      {/* Individual Findings Section */}
+      <div className="individual-findings">
+        {/* Severity Filter - only show if there are findings */}
+        {reviewResult.findings.length > 0 && (
+          <div className="mb-4">
+            <label htmlFor="severity-filter" className="text-sm font-medium mr-2">
+              Filter by severity:
+            </label>
+            <select
+              id="severity-filter"
+              value={severityFilter}
+              onChange={(e) => setSeverityFilter(e.target.value as Severity | 'all')}
+              className="border border-gray-300 rounded px-3 py-1 text-sm"
+              data-testid="severity-filter"
+            >
+              <option value="all">All</option>
+              <option value="critical">Critical</option>
+              <option value="high">High</option>
+              <option value="medium">Medium</option>
+              <option value="low">Low</option>
+              <option value="info">Info</option>
+            </select>
+          </div>
+        )}
+
+        {/* Findings List - always rendered */}
+        <div className="review-findings-list space-y-3" data-testid="review-findings-list">
+          {reviewResult.findings.length === 0 ? (
+            <div className="text-gray-500 bg-gray-50 p-4 rounded text-center">
+              No review findings. All code reviews will appear here.
+            </div>
+          ) : filteredFindings.length === 0 ? (
+            <div className="text-gray-500 bg-gray-50 p-4 rounded text-center">
+              No findings match the selected filter.
+            </div>
+          ) : (
+            filteredFindings.map((finding, index) => {
+              const findingId = finding.id ?? index;
+              const isExpanded = expandedFindings.has(findingId);
+
+              return (
+                <FindingCard
+                  key={findingId}
+                  finding={finding}
+                  index={index}
+                  isExpanded={isExpanded}
+                  onToggle={toggleFinding}
+                />
+              );
+            })
+          )}
+        </div>
+      </div>
     </div>
   );
 }