From 6ef9188c46ab464154cabdd33cf3c0445a6f423d Mon Sep 17 00:00:00 2001 From: frankbria Date: Thu, 4 Dec 2025 22:21:47 -0700 Subject: [PATCH 1/4] feat: Add inline dependency rendering to TaskTreeView MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Replace hover tooltip with inline "Depends on: task-1, task-3" text - Remove .skip from two dependency tests (lines 248, 382) - Update dependency count test to match new format - Simplify implementation from 40 lines to 6 lines Fixes #42 Test Results: - All 38 TaskTreeView tests pass - Full suite: 1096 tests pass - No regressions introduced Visual Change: Before: "↳ 1 dependency" (hover for details) After: "Depends on: task-1, task-3" (inline, immediately visible) --- web-ui/src/components/TaskTreeView.test.tsx | 12 +++---- web-ui/src/components/TaskTreeView.tsx | 40 ++------------------- 2 files changed, 8 insertions(+), 44 deletions(-) diff --git a/web-ui/src/components/TaskTreeView.test.tsx b/web-ui/src/components/TaskTreeView.test.tsx index a9ae1aa8..10a88152 100644 --- a/web-ui/src/components/TaskTreeView.test.tsx +++ b/web-ui/src/components/TaskTreeView.test.tsx @@ -245,8 +245,7 @@ describe('TaskTreeView', () => { expect(humanBadges.length).toBeGreaterThan(0); // Task }); - // TODO: Task dependencies not rendering - see beads issue cf-jf1 - it.skip('should display task dependencies', async () => { + it('should display task dependencies', async () => { const user = userEvent.setup(); render(); @@ -380,8 +379,7 @@ describe('TaskTreeView', () => { expect(titleElement).toBeInTheDocument(); }); - // TODO: Task dependencies not rendering - see beads issue cf-jf1 - it.skip('should handle multiple dependencies correctly', async () => { + it('should handle multiple dependencies correctly', async () => { const user = userEvent.setup(); const multiDepTask: Task = { @@ -614,9 +612,9 @@ describe('TaskTreeView', () => { const expandButton = screen.getAllByRole('button', { name: /expand/i })[0]; await user.click(expandButton); - // Should show dependency count - const depCount = screen.getByText(/1 dependency/i); - expect(depCount).toBeInTheDocument(); + // Should show dependency text + const depText = screen.getByText(/depends on.*task-1/i); + expect(depText).toBeInTheDocument(); }); it('should mark task as blocked when dependencies are not completed', async () => { diff --git a/web-ui/src/components/TaskTreeView.tsx b/web-ui/src/components/TaskTreeView.tsx index 73516e05..37164b37 100644 --- a/web-ui/src/components/TaskTreeView.tsx +++ b/web-ui/src/components/TaskTreeView.tsx @@ -225,44 +225,10 @@ const TaskTreeView = memo(function TaskTreeView({ issues }: TaskTreeViewProps) { )} - {/* Dependency details with hover tooltip */} + {/* Dependency details */} {hasDependencies && ( - - ↳ {task.depends_on.length} {task.depends_on.length === 1 ? 'dependency' : 'dependencies'} - {/* Hover tooltip */} - - Depends on: -
    - {task.depends_on.map((depId) => { - const depTask = allTasks.find( - (t) => t.id === depId || t.task_number === depId - ); - return ( -
  • - {depTask ? ( - - {depTask.task_number}: {depTask.title} - - ({depTask.status}) - - - ) : ( - depId - )} -
    • - ); - })} -
- + + Depends on: {task.depends_on.join(', ')} )} From 3d0ccc639ecf1e872fdefec0ec863552b70582da Mon Sep 17 00:00:00 2001 From: frankbria Date: Thu, 4 Dec 2025 22:38:07 -0700 Subject: [PATCH 2/4] fix: Address code review feedback for dependency rendering 1. Type Safety (CRITICAL): - Added defensive check: hasDependencies && task.depends_on && - Prevents runtime error if depends_on is undefined - TypeScript type narrowing now properly handled 2. UX Improvement - Restore Rich Information: - Added tooltip with task numbers, titles, and status - Tooltip format: "T-001: Implement auth (completed)" - Keeps inline visibility while providing detailed hover info - Addresses UX regression from removing rich tooltip 3. Visual Spacing: - Added ml-2 margin for better badge separation - Added cursor-help for tooltip discoverability Test Results: - All 38 TaskTreeView tests pass - No regressions introduced Visual Result: Inline: "Depends on: task-1, task-3" Hover: "T-001: Implement auth (completed) T-003: Setup database (in_progress)" --- web-ui/src/components/TaskTreeView.tsx | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/web-ui/src/components/TaskTreeView.tsx b/web-ui/src/components/TaskTreeView.tsx index 37164b37..cd3713e3 100644 --- a/web-ui/src/components/TaskTreeView.tsx +++ b/web-ui/src/components/TaskTreeView.tsx @@ -226,8 +226,20 @@ const TaskTreeView = memo(function TaskTreeView({ issues }: TaskTreeViewProps) { )} {/* Dependency details */} - {hasDependencies && ( - + {hasDependencies && task.depends_on && ( + { + const depTask = allTasks.find( + (t) => t.id === depId || t.task_number === depId + ); + return depTask + ? `${depTask.task_number}: ${depTask.title} (${depTask.status})` + : depId; + }) + .join('\n')}`} + > Depends on: {task.depends_on.join(', ')} )} From 6c2aafa09b0a5af4eff59b70e63f6fb983847a4f Mon Sep 17 00:00:00 2001 From: frankbria Date: Fri, 5 Dec 2025 00:07:51 -0700 Subject: [PATCH 3/4] feat: Add checkpoint diff preview and custom delete confirmation dialog Implements expandable checkpoint items with git diff preview and replaces browser confirm dialog with custom DeleteConfirmationDialog component. Backend Changes: - Add CheckpointDiffResponse model for diff API responses - Add GET /api/projects/{id}/checkpoints/{id}/diff endpoint - Parse git numstat output for files changed, insertions, deletions statistics Frontend Changes: - Make checkpoint items clickable to expand and show git diff - Add state management for expanded checkpoints and diff caching - Implement syntax-highlighted diff display (green/red/blue) - Add loading spinner and error handling for diff fetching - Create DeleteConfirmationDialog component following modal pattern - Replace window.confirm() with custom delete confirmation dialog - Add all required testids for E2E testing Testing: - Remove skip decorators from 2 E2E tests in test_checkpoint_ui.spec.ts - All 19 backend checkpoint manager tests pass - Python linting passes (ruff check) Closes #47 --- codeframe/ui/models.py | 9 + codeframe/ui/server.py | 112 +++++++++++ tests/e2e/test_checkpoint_ui.spec.ts | 10 +- .../components/checkpoints/CheckpointList.tsx | 187 ++++++++++++++++-- .../checkpoints/DeleteConfirmationDialog.tsx | 137 +++++++++++++ 5 files changed, 435 insertions(+), 20 deletions(-) create mode 100644 web-ui/src/components/checkpoints/DeleteConfirmationDialog.tsx diff --git a/codeframe/ui/models.py b/codeframe/ui/models.py index c13e8c5d..44a5a077 100644 --- a/codeframe/ui/models.py +++ b/codeframe/ui/models.py @@ -126,6 +126,15 @@ class RestoreCheckpointRequest(BaseModel): ) +class CheckpointDiffResponse(BaseModel): + """Response model for checkpoint diff (Sprint 10 Phase 4).""" + + files_changed: int = Field(description="Number of files changed") + insertions: int = Field(description="Number of lines inserted") + deletions: int = Field(description="Number of lines deleted") + diff: str = Field(description="Git diff output") + + # Multi-Agent Per Project API Models (Phase 3) diff --git a/codeframe/ui/server.py b/codeframe/ui/server.py index 136cad88..f875b403 100644 --- a/codeframe/ui/server.py +++ b/codeframe/ui/server.py @@ -31,6 +31,7 @@ QualityGatesRequest, CheckpointCreateRequest, CheckpointResponse, + CheckpointDiffResponse, RestoreCheckpointRequest, AgentAssignmentRequest, AgentRoleUpdateRequest, @@ -3451,6 +3452,117 @@ async def restore_checkpoint( raise HTTPException(status_code=500, detail=f"Checkpoint restore failed: {str(e)}") +@app.get("/api/projects/{project_id}/checkpoints/{checkpoint_id}/diff", tags=["checkpoints"]) +async def get_checkpoint_diff(project_id: int, checkpoint_id: int) -> CheckpointDiffResponse: + """Get git diff for a checkpoint (Sprint 10 Phase 4). + + Returns the git diff between the checkpoint commit and current HEAD, + including statistics about files changed, insertions, and deletions. + + Args: + project_id: Project ID + checkpoint_id: Checkpoint ID to get diff for + + Returns: + 200 OK: Checkpoint diff with statistics + { + "files_changed": int, + "insertions": int, + "deletions": int, + "diff": str + } + 404 Not Found: Project or checkpoint not found + 500 Internal Server Error: Git operation failed + """ + from codeframe.lib.checkpoint_manager import CheckpointManager + + # Verify project exists + project = app.state.db.get_project_by_id(project_id) + if not project: + raise HTTPException(status_code=404, detail=f"Project {project_id} not found") + + # Get project workspace path + workspace_path = project.get("workspace_path") + if not workspace_path: + raise HTTPException( + status_code=500, + detail=f"Project {project_id} has no workspace path configured", + ) + + # Verify checkpoint exists + checkpoint = app.state.db.get_checkpoint_by_id(checkpoint_id) + if not checkpoint: + raise HTTPException(status_code=404, detail=f"Checkpoint {checkpoint_id} not found") + + # Verify checkpoint belongs to this project + if checkpoint.project_id != project_id: + raise HTTPException( + status_code=404, + detail=f"Checkpoint {checkpoint_id} does not belong to project {project_id}", + ) + + try: + # Create checkpoint manager + checkpoint_mgr = CheckpointManager( + db=app.state.db, + project_root=Path(workspace_path), + project_id=project_id, + ) + + # Get diff output + diff_output = checkpoint_mgr._show_diff(checkpoint.git_commit) + + # Parse diff statistics using git diff --numstat + import subprocess + try: + stats_result = subprocess.run( + ["git", "diff", "--numstat", checkpoint.git_commit, "HEAD"], + cwd=Path(workspace_path), + check=True, + capture_output=True, + text=True + ) + + # Parse numstat output + # Format: \t\t + files_changed = 0 + total_insertions = 0 + total_deletions = 0 + + for line in stats_result.stdout.strip().split('\n'): + if not line: + continue + files_changed += 1 + parts = line.split('\t') + if len(parts) >= 2: + # Handle binary files (marked as '-') + insertions = int(parts[0]) if parts[0] != '-' else 0 + deletions = int(parts[1]) if parts[1] != '-' else 0 + total_insertions += insertions + total_deletions += deletions + + return CheckpointDiffResponse( + files_changed=files_changed, + insertions=total_insertions, + deletions=total_deletions, + diff=diff_output + ) + + except subprocess.CalledProcessError as e: + logger.error(f"Failed to get diff stats: {e.stderr}") + # Return diff without stats if parsing fails + return CheckpointDiffResponse( + files_changed=0, + insertions=0, + deletions=0, + diff=diff_output + ) + + except Exception as e: + logger.error(f"Failed to get checkpoint diff {checkpoint_id}: {e}", exc_info=True) + raise HTTPException(status_code=500, detail=f"Failed to get checkpoint diff: {str(e)}") + + # Sprint 10 Phase 5: Metrics API endpoints (T127-T129) diff --git a/tests/e2e/test_checkpoint_ui.spec.ts b/tests/e2e/test_checkpoint_ui.spec.ts index e26576db..309ce88c 100644 --- a/tests/e2e/test_checkpoint_ui.spec.ts +++ b/tests/e2e/test_checkpoint_ui.spec.ts @@ -119,10 +119,7 @@ test.describe('Checkpoint UI Workflow', () => { } }); - test.skip('should display checkpoint diff preview', async ({ page }) => { - // SKIP: Checkpoint diff preview not implemented in current CheckpointList - // CheckpointList shows metadata but not git diff preview - + test('should display checkpoint diff preview', async ({ page }) => { const checkpointItems = page.locator('[data-testid^="checkpoint-item-"]'); if (await checkpointItems.count() > 0) { @@ -164,10 +161,7 @@ test.describe('Checkpoint UI Workflow', () => { } }); - test.skip('should allow deleting checkpoint', async ({ page }) => { - // SKIP: Delete uses browser confirm dialog, not custom dialog with testids - // The current implementation uses window.confirm() for delete confirmation - + test('should allow deleting checkpoint', async ({ page }) => { const checkpointItems = page.locator('[data-testid^="checkpoint-item-"]'); if (await checkpointItems.count() > 0) { diff --git a/web-ui/src/components/checkpoints/CheckpointList.tsx b/web-ui/src/components/checkpoints/CheckpointList.tsx index 93248ba7..e15c336c 100644 --- a/web-ui/src/components/checkpoints/CheckpointList.tsx +++ b/web-ui/src/components/checkpoints/CheckpointList.tsx @@ -4,9 +4,10 @@ */ import React, { useState, useEffect } from 'react'; -import type { Checkpoint } from '../../types/checkpoints'; -import { listCheckpoints, createCheckpoint, deleteCheckpoint } from '../../api/checkpoints'; +import type { Checkpoint, CheckpointDiff } from '../../types/checkpoints'; +import { listCheckpoints, createCheckpoint, deleteCheckpoint, getCheckpointDiff } from '../../api/checkpoints'; import { CheckpointRestore } from './CheckpointRestore'; +import { DeleteConfirmationDialog } from './DeleteConfirmationDialog'; interface CheckpointListProps { projectId: number; @@ -28,6 +29,17 @@ export const CheckpointList: React.FC = ({ const [selectedCheckpoint, setSelectedCheckpoint] = useState(null); const [showRestoreDialog, setShowRestoreDialog] = useState(false); + // Expandable checkpoint state + const [expandedCheckpointId, setExpandedCheckpointId] = useState(null); + const [checkpointDiffs, setCheckpointDiffs] = useState>(new Map()); + const [loadingDiffs, setLoadingDiffs] = useState>(new Set()); + const [diffErrors, setDiffErrors] = useState>(new Map()); + + // Delete dialog state + const [showDeleteDialog, setShowDeleteDialog] = useState(false); + const [checkpointToDelete, setCheckpointToDelete] = useState<{ id: number; name: string } | null>(null); + const [deleting, setDeleting] = useState(false); + // Load checkpoints const loadCheckpoints = async () => { try { @@ -87,21 +99,37 @@ export const CheckpointList: React.FC = ({ } }; - // Handle delete checkpoint - const handleDeleteCheckpoint = async (checkpointId: number, checkpointName: string) => { - if (!window.confirm(`Are you sure you want to delete checkpoint "${checkpointName}"?`)) { - return; - } + // Handle delete checkpoint - show confirmation dialog + const handleDeleteCheckpoint = (checkpointId: number, checkpointName: string) => { + setCheckpointToDelete({ id: checkpointId, name: checkpointName }); + setShowDeleteDialog(true); + }; + + // Handle confirm delete + const handleConfirmDelete = async () => { + if (!checkpointToDelete) return; + + setDeleting(true); + setError(null); try { - setError(null); - await deleteCheckpoint(projectId, checkpointId); + await deleteCheckpoint(projectId, checkpointToDelete.id); await loadCheckpoints(); + setShowDeleteDialog(false); + setCheckpointToDelete(null); } catch (err) { setError(err instanceof Error ? err.message : 'Failed to delete checkpoint'); + } finally { + setDeleting(false); } }; + // Handle cancel delete + const handleCancelDelete = () => { + setShowDeleteDialog(false); + setCheckpointToDelete(null); + }; + // Handle restore click const handleRestoreClick = (checkpoint: Checkpoint) => { setSelectedCheckpoint(checkpoint); @@ -115,6 +143,44 @@ export const CheckpointList: React.FC = ({ loadCheckpoints(); }; + // Handle checkpoint click to toggle expansion and fetch diff + const handleCheckpointClick = async (checkpointId: number) => { + // Toggle expansion + if (expandedCheckpointId === checkpointId) { + setExpandedCheckpointId(null); + return; + } + + setExpandedCheckpointId(checkpointId); + + // Check if diff is already cached + if (checkpointDiffs.has(checkpointId)) { + return; + } + + // Fetch diff + setLoadingDiffs(prev => new Set(prev).add(checkpointId)); + setDiffErrors(prev => { + const next = new Map(prev); + next.delete(checkpointId); + return next; + }); + + try { + const diff = await getCheckpointDiff(projectId, checkpointId); + setCheckpointDiffs(prev => new Map(prev).set(checkpointId, diff)); + } catch (err) { + const errorMessage = err instanceof Error ? err.message : 'Failed to load diff'; + setDiffErrors(prev => new Map(prev).set(checkpointId, errorMessage)); + } finally { + setLoadingDiffs(prev => { + const next = new Set(prev); + next.delete(checkpointId); + return next; + }); + } + }; + // Format date const formatDate = (dateString: string): string => { const date = new Date(dateString); @@ -256,8 +322,9 @@ export const CheckpointList: React.FC = ({ {checkpoints.map((checkpoint) => (
handleCheckpointClick(checkpoint.id)} >
@@ -321,14 +388,20 @@ export const CheckpointList: React.FC = ({
+ + {/* Diff display section */} + {expandedCheckpointId === checkpoint.id && ( +
+ {loadingDiffs.has(checkpoint.id) && ( +
+
+ Loading diff... +
+ )} + + {diffErrors.has(checkpoint.id) && ( +
+

{diffErrors.get(checkpoint.id)}

+
+ )} + + {checkpointDiffs.has(checkpoint.id) && !loadingDiffs.has(checkpoint.id) && ( +
+ {(() => { + const diff = checkpointDiffs.get(checkpoint.id)!; + + // Check if diff is empty + if (diff.files_changed === 0 && !diff.diff.trim()) { + return ( +
+

No changes detected

+
+ ); + } + + return ( + <> + {/* Diff summary */} +
+
+
+ Files changed:{' '} + {diff.files_changed} +
+
+ Insertions:{' '} + +{diff.insertions} +
+
+ Deletions:{' '} + -{diff.deletions} +
+
+
+ + {/* Diff content */} +
+ 
+                                {diff.diff.split('\n').map((line, idx) => (
+                                  

+                                    {line}
+                                  

+                                ))}
+
+
+ + ); + })()} +
+ )} +
+ )}
))} @@ -353,6 +505,17 @@ export const CheckpointList: React.FC = ({ onRestoreComplete={handleRestoreComplete} /> )} + + {/* Delete confirmation dialog */} + {showDeleteDialog && checkpointToDelete && ( + + )} ); }; diff --git a/web-ui/src/components/checkpoints/DeleteConfirmationDialog.tsx b/web-ui/src/components/checkpoints/DeleteConfirmationDialog.tsx new file mode 100644 index 00000000..cf30766b --- /dev/null +++ b/web-ui/src/components/checkpoints/DeleteConfirmationDialog.tsx @@ -0,0 +1,137 @@ +/** + * DeleteConfirmationDialog Component for Sprint 10 Phase 4 + * Modal dialog for confirming checkpoint deletion + */ + +'use client'; + +import { useEffect } from 'react'; + +interface DeleteConfirmationDialogProps { + isOpen: boolean; + checkpointName: string; + onConfirm: () => void; + onCancel: () => void; + isDeleting: boolean; +} + +export function DeleteConfirmationDialog({ + isOpen, + checkpointName, + onConfirm, + onCancel, + isDeleting, +}: DeleteConfirmationDialogProps) { + // Close modal on Escape key + useEffect(() => { + const handleEscape = (e: KeyboardEvent) => { + if (e.key === 'Escape' && isOpen && !isDeleting) { + onCancel(); + } + }; + + if (isOpen) { + document.addEventListener('keydown', handleEscape); + return () => document.removeEventListener('keydown', handleEscape); + } + }, [isOpen, isDeleting, onCancel]); + + // Handle backdrop click + const handleBackdropClick = (e: React.MouseEvent) => { + if (e.target === e.currentTarget && !isDeleting) { + onCancel(); + } + }; + + if (!isOpen) { + return null; + } + + return ( +
+
+ {/* Header */} +
+

+ Delete Checkpoint +

+ +
+ + {/* Body */} +
+
+ {/* Warning icon */} + + + + + {/* Warning message */} +
+

+ Are you sure you want to delete this checkpoint? +

+

+ "{checkpointName}" +

+

+ This action cannot be undone. The checkpoint backup files will be permanently deleted. +

+
+
+
+ + {/* Footer */} +
+ + +
+
+
+ ); +} + +export default DeleteConfirmationDialog; From 1e69b14c98a5808c6a4caae7c92bbba5edc6f072 Mon Sep 17 00:00:00 2001 From: frankbria Date: Fri, 5 Dec 2025 00:20:34 -0700 Subject: [PATCH 4/4] fix: Address critical security and UX issues from code review Resolves all Critical, High, and Medium priority issues identified in code review. Critical Issues Fixed: 1. Git commit validation - Added SHA format validation (^[a-f0-9]{7,40}$) to prevent command injection 2. Git commit existence check - Verify commit exists before attempting diff operations 3. Subprocess security - Validate all git commit SHAs before use in subprocess calls High Priority Issues Fixed: 4. Race condition prevention - Implemented AbortController to cancel in-flight requests 5. Error handling - Return explicit 500 errors instead of misleading zeros when parsing fails 6. Memory limits - Added 10MB diff size limit and 10-item cache limit Medium Priority Issues Fixed: 7. Component cleanup - Added isMounted ref to prevent state updates on unmounted components 8. Accessibility - Auto-focus Cancel button in DeleteConfirmationDialog for keyboard users 9. Binary file tracking - Count binary files separately (exposed in X-Binary-Files header) Additional Improvements: - Added HTTP caching headers (Cache-Control: immutable) for checkpoint diffs - Added timeouts for git operations (5s verification, 30s diff) - Improved error messages with detailed failure information - LRU cache eviction for checkpoint diffs Backend Changes: - server.py: Enhanced get_checkpoint_diff endpoint with validation and limits - Added git cat-file check to verify commit existence - Added regex validation for git SHA format - Added size limits and truncation for large diffs - Better subprocess error handling with timeouts Frontend Changes: - CheckpointList.tsx: Added AbortController and cleanup logic - api/checkpoints.ts: Added optional AbortSignal parameter - DeleteConfirmationDialog.tsx: Added focus management with useRef - Implemented isMounted pattern to prevent memory leaks - Added cache size limits (max 10 items with LRU eviction) Testing: All 19 backend checkpoint manager tests passing --- codeframe/ui/server.py | 80 ++++++++++++--- web-ui/src/api/checkpoints.ts | 4 +- .../components/checkpoints/CheckpointList.tsx | 98 ++++++++++++++++--- .../checkpoints/DeleteConfirmationDialog.tsx | 16 ++- 4 files changed, 165 insertions(+), 33 deletions(-) diff --git a/codeframe/ui/server.py b/codeframe/ui/server.py index f875b403..91560484 100644 --- a/codeframe/ui/server.py +++ b/codeframe/ui/server.py @@ -3474,6 +3474,8 @@ async def get_checkpoint_diff(project_id: int, checkpoint_id: int) -> Checkpoint 404 Not Found: Project or checkpoint not found 500 Internal Server Error: Git operation failed """ + import re + import subprocess from codeframe.lib.checkpoint_manager import CheckpointManager # Verify project exists @@ -3501,7 +3503,35 @@ async def get_checkpoint_diff(project_id: int, checkpoint_id: int) -> Checkpoint detail=f"Checkpoint {checkpoint_id} does not belong to project {project_id}", ) + # SECURITY: Validate git commit SHA format to prevent command injection + git_sha_pattern = re.compile(r'^[a-f0-9]{7,40}$') + if not git_sha_pattern.match(checkpoint.git_commit): + logger.error(f"Invalid git commit SHA format: {checkpoint.git_commit}") + raise HTTPException( + status_code=500, + detail=f"Invalid git commit format in checkpoint {checkpoint_id}", + ) + try: + # Verify git commit exists before attempting diff + try: + verify_result = subprocess.run( + ["git", "cat-file", "-e", checkpoint.git_commit], + cwd=Path(workspace_path), + check=True, + capture_output=True, + timeout=5 + ) + except subprocess.CalledProcessError: + logger.error(f"Git commit {checkpoint.git_commit} not found in repository") + raise HTTPException( + status_code=404, + detail=f"Checkpoint commit {checkpoint.git_commit[:7]} not found in repository", + ) + except subprocess.TimeoutExpired: + logger.error(f"Git verification timed out for commit {checkpoint.git_commit}") + raise HTTPException(status_code=500, detail="Git operation timed out") + # Create checkpoint manager checkpoint_mgr = CheckpointManager( db=app.state.db, @@ -3509,18 +3539,22 @@ async def get_checkpoint_diff(project_id: int, checkpoint_id: int) -> Checkpoint project_id=project_id, ) - # Get diff output + # Get diff output with size limit (10MB) diff_output = checkpoint_mgr._show_diff(checkpoint.git_commit) + MAX_DIFF_SIZE = 10 * 1024 * 1024 # 10MB + if len(diff_output) > MAX_DIFF_SIZE: + diff_output = diff_output[:MAX_DIFF_SIZE] + "\n\n... [diff truncated - exceeded 10MB limit]" + logger.warning(f"Diff for checkpoint {checkpoint_id} truncated due to size limit") # Parse diff statistics using git diff --numstat - import subprocess try: stats_result = subprocess.run( ["git", "diff", "--numstat", checkpoint.git_commit, "HEAD"], cwd=Path(workspace_path), check=True, capture_output=True, - text=True + text=True, + timeout=30 ) # Parse numstat output @@ -3528,6 +3562,7 @@ async def get_checkpoint_diff(project_id: int, checkpoint_id: int) -> Checkpoint files_changed = 0 total_insertions = 0 total_deletions = 0 + binary_files = 0 for line in stats_result.stdout.strip().split('\n'): if not line: @@ -3536,28 +3571,43 @@ async def get_checkpoint_diff(project_id: int, checkpoint_id: int) -> Checkpoint parts = line.split('\t') if len(parts) >= 2: # Handle binary files (marked as '-') - insertions = int(parts[0]) if parts[0] != '-' else 0 - deletions = int(parts[1]) if parts[1] != '-' else 0 - total_insertions += insertions - total_deletions += deletions - - return CheckpointDiffResponse( + if parts[0] == '-' or parts[1] == '-': + binary_files += 1 + else: + insertions = int(parts[0]) + deletions = int(parts[1]) + total_insertions += insertions + total_deletions += deletions + + response = CheckpointDiffResponse( files_changed=files_changed, insertions=total_insertions, deletions=total_deletions, diff=diff_output ) + # Add cache headers for immutable checkpoint diffs + return JSONResponse( + content=response.model_dump(), + headers={ + "Cache-Control": "public, max-age=31536000, immutable", + "X-Binary-Files": str(binary_files) + } + ) + except subprocess.CalledProcessError as e: logger.error(f"Failed to get diff stats: {e.stderr}") - # Return diff without stats if parsing fails - return CheckpointDiffResponse( - files_changed=0, - insertions=0, - deletions=0, - diff=diff_output + # Return error response when parsing fails (not misleading zeros) + raise HTTPException( + status_code=500, + detail=f"Failed to parse diff statistics: {e.stderr[:200]}" ) + except subprocess.TimeoutExpired: + logger.error(f"Git diff timed out for checkpoint {checkpoint_id}") + raise HTTPException(status_code=500, detail="Diff operation timed out") + except HTTPException: + raise except Exception as e: logger.error(f"Failed to get checkpoint diff {checkpoint_id}: {e}", exc_info=True) raise HTTPException(status_code=500, detail=f"Failed to get checkpoint diff: {str(e)}") diff --git a/web-ui/src/api/checkpoints.ts b/web-ui/src/api/checkpoints.ts index d7d6daeb..bed5d351 100644 --- a/web-ui/src/api/checkpoints.ts +++ b/web-ui/src/api/checkpoints.ts @@ -136,7 +136,8 @@ export async function restoreCheckpoint( */ export async function getCheckpointDiff( projectId: number, - checkpointId: number + checkpointId: number, + signal?: AbortSignal ): Promise { const response = await fetch( `${API_BASE_URL}/api/projects/${projectId}/checkpoints/${checkpointId}/diff`, @@ -145,6 +146,7 @@ export async function getCheckpointDiff( headers: { 'Content-Type': 'application/json', }, + signal, } ); diff --git a/web-ui/src/components/checkpoints/CheckpointList.tsx b/web-ui/src/components/checkpoints/CheckpointList.tsx index e15c336c..33f48c22 100644 --- a/web-ui/src/components/checkpoints/CheckpointList.tsx +++ b/web-ui/src/components/checkpoints/CheckpointList.tsx @@ -3,7 +3,7 @@ * Displays list of checkpoints with create/delete functionality */ -import React, { useState, useEffect } from 'react'; +import React, { useState, useEffect, useRef } from 'react'; import type { Checkpoint, CheckpointDiff } from '../../types/checkpoints'; import { listCheckpoints, createCheckpoint, deleteCheckpoint, getCheckpointDiff } from '../../api/checkpoints'; import { CheckpointRestore } from './CheckpointRestore'; @@ -40,6 +40,21 @@ export const CheckpointList: React.FC = ({ const [checkpointToDelete, setCheckpointToDelete] = useState<{ id: number; name: string } | null>(null); const [deleting, setDeleting] = useState(false); + // Refs for cleanup and race condition prevention + const isMounted = useRef(true); + const abortControllerRef = useRef(null); + + // Cleanup on unmount + useEffect(() => { + return () => { + isMounted.current = false; + // Cancel any in-flight diff requests + if (abortControllerRef.current) { + abortControllerRef.current.abort(); + } + }; + }, []); + // Load checkpoints const loadCheckpoints = async () => { try { @@ -148,6 +163,11 @@ export const CheckpointList: React.FC = ({ // Toggle expansion if (expandedCheckpointId === checkpointId) { setExpandedCheckpointId(null); + // Cancel any in-flight request when collapsing + if (abortControllerRef.current) { + abortControllerRef.current.abort(); + abortControllerRef.current = null; + } return; } @@ -158,26 +178,72 @@ export const CheckpointList: React.FC = ({ return; } + // Cancel any previous in-flight diff request (prevents race condition) + if (abortControllerRef.current) { + abortControllerRef.current.abort(); + } + + // Create new AbortController for this request + const controller = new AbortController(); + abortControllerRef.current = controller; + // Fetch diff - setLoadingDiffs(prev => new Set(prev).add(checkpointId)); - setDiffErrors(prev => { - const next = new Map(prev); - next.delete(checkpointId); - return next; - }); + if (isMounted.current) { + setLoadingDiffs(prev => new Set(prev).add(checkpointId)); + setDiffErrors(prev => { + const next = new Map(prev); + next.delete(checkpointId); + return next; + }); + } try { - const diff = await getCheckpointDiff(projectId, checkpointId); - setCheckpointDiffs(prev => new Map(prev).set(checkpointId, diff)); + const diff = await getCheckpointDiff(projectId, checkpointId, controller.signal); + + // Only update state if component is still mounted and request wasn't aborted + if (isMounted.current && !controller.signal.aborted) { + // Enforce cache size limit (max 10 items) + setCheckpointDiffs(prev => { + const next = new Map(prev); + next.set(checkpointId, diff); + + // Evict oldest entries if cache exceeds 10 items + const MAX_CACHE_SIZE = 10; + if (next.size > MAX_CACHE_SIZE) { + const firstKey = next.keys().next().value; + if (firstKey !== undefined) { + next.delete(firstKey); + } + } + + return next; + }); + } } catch (err) { - const errorMessage = err instanceof Error ? err.message : 'Failed to load diff'; - setDiffErrors(prev => new Map(prev).set(checkpointId, errorMessage)); + // Ignore AbortError (expected when requests are cancelled) + if (err instanceof Error && err.name === 'AbortError') { + return; + } + + // Only update state if component is still mounted + if (isMounted.current) { + const errorMessage = err instanceof Error ? err.message : 'Failed to load diff'; + setDiffErrors(prev => new Map(prev).set(checkpointId, errorMessage)); + } } finally { - setLoadingDiffs(prev => { - const next = new Set(prev); - next.delete(checkpointId); - return next; - }); + // Only update state if component is still mounted + if (isMounted.current) { + setLoadingDiffs(prev => { + const next = new Set(prev); + next.delete(checkpointId); + return next; + }); + } + + // Clear the ref if this was the active request + if (abortControllerRef.current === controller) { + abortControllerRef.current = null; + } } }; diff --git a/web-ui/src/components/checkpoints/DeleteConfirmationDialog.tsx b/web-ui/src/components/checkpoints/DeleteConfirmationDialog.tsx index cf30766b..43b3f8a6 100644 --- a/web-ui/src/components/checkpoints/DeleteConfirmationDialog.tsx +++ b/web-ui/src/components/checkpoints/DeleteConfirmationDialog.tsx @@ -5,7 +5,7 @@ 'use client'; -import { useEffect } from 'react'; +import { useEffect, useRef } from 'react'; interface DeleteConfirmationDialogProps { isOpen: boolean; @@ -22,6 +22,19 @@ export function DeleteConfirmationDialog({ onCancel, isDeleting, }: DeleteConfirmationDialogProps) { + // Ref for focus management + const cancelButtonRef = useRef(null); + + // Auto-focus Cancel button when dialog opens + useEffect(() => { + if (isOpen && cancelButtonRef.current) { + // Small delay to ensure dialog is rendered + setTimeout(() => { + cancelButtonRef.current?.focus(); + }, 100); + } + }, [isOpen]); + // Close modal on Escape key useEffect(() => { const handleEscape = (e: KeyboardEvent) => { @@ -113,6 +126,7 @@ export function DeleteConfirmationDialog({ {/* Footer */}