diff --git a/src/components/RightSidebar/CodeReview/FileTree.tsx b/src/components/RightSidebar/CodeReview/FileTree.tsx index ce214f2b7..89f07f03c 100644 --- a/src/components/RightSidebar/CodeReview/FileTree.tsx +++ b/src/components/RightSidebar/CodeReview/FileTree.tsx @@ -33,14 +33,42 @@ const TreeNode = styled.div<{ depth: number; isSelected: boolean }>` } `; -const FileName = styled.span` +const FileName = styled.span<{ isFullyRead?: boolean; isUnknownState?: boolean }>` color: #ccc; flex: 1; + ${(props) => + props.isFullyRead && + ` + color: #666; + text-decoration: line-through; + text-decoration-color: var(--color-read); + text-decoration-thickness: 2px; + `} + ${(props) => + props.isUnknownState && + !props.isFullyRead && + ` + color: #666; + `} `; -const DirectoryName = styled.span` +const DirectoryName = styled.span<{ isFullyRead?: boolean; isUnknownState?: boolean }>` color: #888; flex: 1; + ${(props) => + props.isFullyRead && + ` + color: #666; + text-decoration: line-through; + text-decoration-color: var(--color-read); + text-decoration-thickness: 2px; + `} + ${(props) => + props.isUnknownState && + !props.isFullyRead && + ` + color: #666; + `} `; const DirectoryStats = styled.span<{ isOpen: boolean }>` @@ -117,13 +145,56 @@ const EmptyState = styled.div` text-align: center; `; +/** + * Compute read status for a directory by recursively checking all descendant files + * Returns "fully-read" if all files are fully read, "unknown" if any file has unknown status, null otherwise + */ +function computeDirectoryReadStatus( + node: FileTreeNode, + getFileReadStatus?: (filePath: string) => { total: number; read: number } | null +): "fully-read" | "unknown" | null { + if (!node.isDirectory || !getFileReadStatus) return null; + + let hasUnknown = false; + let fileCount = 0; + let fullyReadCount = 0; + + const checkNode = (n: FileTreeNode) => { + if (n.isDirectory) { + // Recurse into children + n.children.forEach(checkNode); + } else { + // Check file status + fileCount++; + const status = getFileReadStatus(n.path); + if (status === null) { + hasUnknown = true; + } else if (status.read === status.total && status.total > 0) { + fullyReadCount++; + } + } + }; + + checkNode(node); + + // If any file has unknown state, directory is unknown + if (hasUnknown) return "unknown"; + + // If all files are fully read, directory is fully read + if (fileCount > 0 && fullyReadCount === fileCount) return "fully-read"; + + // Otherwise, directory has partial/no read state + return null; +} + const TreeNodeContent: React.FC<{ node: FileTreeNode; depth: number; selectedPath: string | null; onSelectFile: (path: string | null) => void; commonPrefix: string | null; -}> = ({ node, depth, selectedPath, onSelectFile, commonPrefix }) => { + getFileReadStatus?: (filePath: string) => { total: number; read: number } | null; +}> = ({ node, depth, selectedPath, onSelectFile, commonPrefix, getFileReadStatus }) => { const [isOpen, setIsOpen] = useState(depth < 2); // Auto-expand first 2 levels const handleClick = (e: React.MouseEvent) => { @@ -154,6 +225,20 @@ const TreeNodeContent: React.FC<{ const isSelected = selectedPath === node.path; + // Compute read status for files and directories + let isFullyRead = false; + let isUnknownState = false; + + if (node.isDirectory) { + const dirStatus = computeDirectoryReadStatus(node, getFileReadStatus); + isFullyRead = dirStatus === "fully-read"; + isUnknownState = dirStatus === "unknown"; + } else if (getFileReadStatus) { + const readStatus = getFileReadStatus(node.path); + isFullyRead = readStatus ? readStatus.read === readStatus.total && readStatus.total > 0 : false; + isUnknownState = readStatus === null; + } + return ( <> @@ -162,7 +247,9 @@ const TreeNodeContent: React.FC<{ ▶ - {node.name || "/"} + + {node.name || "/"} + {node.totalStats && (node.totalStats.additions > 0 || node.totalStats.deletions > 0) && ( @@ -184,7 +271,9 @@ const TreeNodeContent: React.FC<{ ) : ( <> - {node.name} + + {node.name} + {node.stats && ( {node.stats.additions > 0 && +{node.stats.additions}} @@ -205,6 +294,7 @@ const TreeNodeContent: React.FC<{ selectedPath={selectedPath} onSelectFile={onSelectFile} commonPrefix={commonPrefix} + getFileReadStatus={getFileReadStatus} /> ))} @@ -217,6 +307,7 @@ interface FileTreeExternalProps { onSelectFile: (path: string | null) => void; isLoading?: boolean; commonPrefix?: string | null; + getFileReadStatus?: (filePath: string) => { total: number; read: number } | null; } export const FileTree: React.FC = ({ @@ -225,6 +316,7 @@ export const FileTree: React.FC = ({ onSelectFile, isLoading = false, commonPrefix = null, + getFileReadStatus, }) => { // Find the node at the common prefix path to start rendering from const startNode = React.useMemo(() => { @@ -262,6 +354,7 @@ export const FileTree: React.FC = ({ selectedPath={selectedPath} onSelectFile={onSelectFile} commonPrefix={commonPrefix} + getFileReadStatus={getFileReadStatus} /> )) ) : ( diff --git a/src/components/RightSidebar/CodeReview/HunkViewer.tsx b/src/components/RightSidebar/CodeReview/HunkViewer.tsx index e5baff1d6..568a9d776 100644 --- a/src/components/RightSidebar/CodeReview/HunkViewer.tsx +++ b/src/components/RightSidebar/CodeReview/HunkViewer.tsx @@ -10,11 +10,13 @@ import { SelectableDiffRenderer } from "../../shared/DiffRenderer"; interface HunkViewerProps { hunk: DiffHunk; isSelected?: boolean; + isRead?: boolean; onClick?: () => void; + onToggleRead?: () => void; onReviewNote?: (note: string) => void; } -const HunkContainer = styled.div<{ isSelected: boolean }>` +const HunkContainer = styled.div<{ isSelected: boolean; isRead: boolean }>` background: #1e1e1e; border: 1px solid #3e3e42; border-radius: 4px; @@ -24,18 +26,21 @@ const HunkContainer = styled.div<{ isSelected: boolean }>` transition: all 0.2s ease; ${(props) => - props.isSelected && + props.isRead && ` - border-color: #007acc; - box-shadow: 0 0 0 1px #007acc; + border-color: var(--color-read); `} - &:hover { - border-color: #007acc; - } + ${(props) => + props.isSelected && + ` + border-color: var(--color-review-accent); + box-shadow: 0 0 0 1px var(--color-review-accent); + `} `; const HunkHeader = styled.div` + /* Keep grayscale to avoid clashing with green/red LoC indicators */ background: #252526; padding: 8px 12px; border-bottom: 1px solid #3e3e42; @@ -44,6 +49,7 @@ const HunkHeader = styled.div` align-items: center; font-family: var(--font-monospace); font-size: 12px; + gap: 8px; `; const FilePath = styled.div` @@ -124,19 +130,64 @@ const RenameInfo = styled.div` } `; +const ReadIndicator = styled.span` + display: inline-flex; + align-items: center; + color: var(--color-read); + font-size: 14px; + margin-right: 4px; +`; + +const ToggleReadButton = styled.button` + background: transparent; + border: 1px solid #3e3e42; + border-radius: 3px; + padding: 2px 6px; + color: #888; + font-size: 11px; + cursor: pointer; + transition: all 0.2s ease; + display: flex; + align-items: center; + gap: 4px; + + &:hover { + background: rgba(255, 255, 255, 0.05); + border-color: var(--color-read); + color: var(--color-read); + } + + &:active { + transform: scale(0.95); + } +`; + export const HunkViewer: React.FC = ({ hunk, isSelected, + isRead = false, onClick, + onToggleRead, onReviewNote, }) => { - const [isExpanded, setIsExpanded] = useState(true); + // Collapse by default if marked as read + const [isExpanded, setIsExpanded] = useState(!isRead); + + // Auto-collapse when marked as read, auto-expand when unmarked + React.useEffect(() => { + setIsExpanded(!isRead); + }, [isRead]); const handleToggleExpand = (e: React.MouseEvent) => { e.stopPropagation(); setIsExpanded(!isExpanded); }; + const handleToggleRead = (e: React.MouseEvent) => { + e.stopPropagation(); + onToggleRead?.(); + }; + // Parse diff lines const diffLines = hunk.content.split("\n").filter((line) => line.length > 0); const lineCount = diffLines.length; @@ -153,9 +204,11 @@ export const HunkViewer: React.FC = ({ return ( { if (e.key === "Enter" || e.key === " ") { e.preventDefault(); @@ -164,6 +217,7 @@ export const HunkViewer: React.FC = ({ }} > + {isRead && ✓} {hunk.filePath} {!isPureRename && ( @@ -175,6 +229,11 @@ export const HunkViewer: React.FC = ({ ({lineCount} {lineCount === 1 ? "line" : "lines"}) + {onToggleRead && ( + + {isRead ? "○" : "◉"} + + )} @@ -190,11 +249,12 @@ export const HunkViewer: React.FC = ({ oldStart={hunk.oldStart} newStart={hunk.newStart} onReviewNote={onReviewNote} + onLineClick={onClick} /> ) : ( - Click to expand ({lineCount} lines) + {isRead && "Hunk marked as read. "}Click to expand ({lineCount} lines) )} diff --git a/src/components/RightSidebar/CodeReview/ReviewControls.tsx b/src/components/RightSidebar/CodeReview/ReviewControls.tsx index d7f6c2587..afe0192c4 100644 --- a/src/components/RightSidebar/CodeReview/ReviewControls.tsx +++ b/src/components/RightSidebar/CodeReview/ReviewControls.tsx @@ -166,6 +166,10 @@ export const ReviewControls: React.FC = ({ onFiltersChange({ ...filters, includeDirty: e.target.checked }); }; + const handleShowReadToggle = (e: React.ChangeEvent) => { + onFiltersChange({ ...filters, showReadHunks: e.target.checked }); + }; + const handleSetDefault = () => { setDefaultBase(filters.diffBase); }; @@ -206,6 +210,11 @@ export const ReviewControls: React.FC = ({ Dirty + + + Show read + + = ({ - {stats.total} {stats.total === 1 ? "hunk" : "hunks"} + {stats.read} read / {stats.total} total ); diff --git a/src/components/RightSidebar/CodeReview/ReviewPanel.tsx b/src/components/RightSidebar/CodeReview/ReviewPanel.tsx index fda0ec552..6536750c8 100644 --- a/src/components/RightSidebar/CodeReview/ReviewPanel.tsx +++ b/src/components/RightSidebar/CodeReview/ReviewPanel.tsx @@ -3,12 +3,13 @@ * Displays diff hunks for viewing changes in the workspace */ -import React, { useState, useEffect, useMemo } from "react"; +import React, { useState, useEffect, useMemo, useCallback } from "react"; import styled from "@emotion/styled"; import { HunkViewer } from "./HunkViewer"; import { ReviewControls } from "./ReviewControls"; import { FileTree } from "./FileTree"; import { usePersistedState } from "@/hooks/usePersistedState"; +import { useReviewState } from "@/hooks/useReviewState"; import { parseDiff, extractAllHunks } from "@/utils/git/diffParser"; import { parseNumstat, @@ -301,9 +302,17 @@ export const ReviewPanel: React.FC = ({ false ); + // Persist showReadHunks flag per workspace + const [showReadHunks, setShowReadHunks] = usePersistedState( + `review-show-read:${workspaceId}`, + true + ); + + // Initialize review state hook + const reviewState = useReviewState(workspaceId); + const [filters, setFilters] = useState({ - showReviewed: true, - statusFilter: "all", + showReadHunks: showReadHunks, diffBase: diffBase, includeDirty: includeDirty, }); @@ -431,6 +440,8 @@ export const ReviewPanel: React.FC = ({ return () => { cancelled = true; }; + // selectedHunkId intentionally omitted - only auto-select on initial load, not on every selection change + // eslint-disable-next-line react-hooks/exhaustive-deps }, [ workspaceId, workspacePath, @@ -450,20 +461,88 @@ export const ReviewPanel: React.FC = ({ setIncludeDirty(filters.includeDirty); }, [filters.includeDirty, setIncludeDirty]); - // For MVP: No review state tracking, just show all hunks - const filteredHunks = hunks; - - // Simple stats for display - const stats = useMemo( - () => ({ - total: hunks.length, - accepted: 0, - rejected: 0, - unreviewed: hunks.length, - }), - [hunks] + // Persist showReadHunks when it changes + useEffect(() => { + setShowReadHunks(filters.showReadHunks); + }, [filters.showReadHunks, setShowReadHunks]); + + // Get read status for a file + const getFileReadStatus = useCallback( + (filePath: string) => { + const fileHunks = hunks.filter((h) => h.filePath === filePath); + if (fileHunks.length === 0) { + return null; // Unknown state - no hunks loaded for this file + } + const total = fileHunks.length; + const read = fileHunks.filter((h) => reviewState.isRead(h.id)).length; + return { total, read }; + }, + [hunks, reviewState] + ); + + // Filter hunks based on read state + const filteredHunks = useMemo(() => { + if (filters.showReadHunks) { + return hunks; + } + return hunks.filter((hunk) => !reviewState.isRead(hunk.id)); + }, [hunks, filters.showReadHunks, reviewState]); + + // Handle toggling read state with auto-navigation + const handleToggleRead = useCallback( + (hunkId: string) => { + const wasRead = reviewState.isRead(hunkId); + reviewState.toggleRead(hunkId); + + // If toggling the selected hunk, check if it will still be visible after toggle + if (hunkId === selectedHunkId) { + // Hunk is visible if: showReadHunks is on OR it will be unread after toggle + const willBeVisible = filters.showReadHunks || wasRead; + + if (!willBeVisible) { + // Hunk will be filtered out - move to next visible hunk + const currentIndex = filteredHunks.findIndex((h) => h.id === hunkId); + if (currentIndex !== -1) { + if (currentIndex < filteredHunks.length - 1) { + setSelectedHunkId(filteredHunks[currentIndex + 1].id); + } else if (currentIndex > 0) { + setSelectedHunkId(filteredHunks[currentIndex - 1].id); + } else { + setSelectedHunkId(null); + } + } + } + } + }, + [reviewState, filters.showReadHunks, filteredHunks, selectedHunkId] ); + // Calculate stats + const stats = useMemo(() => { + const total = hunks.length; + const read = hunks.filter((h) => reviewState.isRead(h.id)).length; + return { + total, + read, + unread: total - read, + }; + }, [hunks, reviewState]); + + // Scroll selected hunk into view + useEffect(() => { + if (!selectedHunkId) return; + + // Find the hunk container element by data attribute + const hunkElement = document.querySelector(`[data-hunk-id="${selectedHunkId}"]`); + if (hunkElement) { + hunkElement.scrollIntoView({ + behavior: "smooth", + block: "nearest", + inline: "nearest", + }); + } + }, [selectedHunkId]); + // Keyboard navigation (j/k or arrow keys) - only when panel is focused useEffect(() => { if (!isPanelFocused) return; @@ -482,7 +561,7 @@ export const ReviewPanel: React.FC = ({ const currentIndex = filteredHunks.findIndex((h) => h.id === selectedHunkId); if (currentIndex === -1) return; - // Navigation only + // Navigation if (e.key === "j" || e.key === "ArrowDown") { e.preventDefault(); if (currentIndex < filteredHunks.length - 1) { @@ -493,12 +572,16 @@ export const ReviewPanel: React.FC = ({ if (currentIndex > 0) { setSelectedHunkId(filteredHunks[currentIndex - 1].id); } + } else if (matchesKeybind(e, KEYBINDS.TOGGLE_HUNK_READ)) { + // Toggle read state of selected hunk + e.preventDefault(); + handleToggleRead(selectedHunkId); } }; window.addEventListener("keydown", handleKeyDown); return () => window.removeEventListener("keydown", handleKeyDown); - }, [isPanelFocused, selectedHunkId, filteredHunks]); + }, [isPanelFocused, selectedHunkId, filteredHunks, handleToggleRead]); // Global keyboard shortcut for refresh (Ctrl+R / Cmd+R) useEffect(() => { @@ -586,13 +669,16 @@ export const ReviewPanel: React.FC = ({ ) : ( filteredHunks.map((hunk) => { const isSelected = hunk.id === selectedHunkId; + const isRead = reviewState.isRead(hunk.id); return ( setSelectedHunkId(hunk.id)} + onToggleRead={() => handleToggleRead(hunk.id)} onReviewNote={onReviewNote} /> ); @@ -610,6 +696,7 @@ export const ReviewPanel: React.FC = ({ onSelectFile={setSelectedFilePath} isLoading={isLoadingTree} commonPrefix={commonPrefix} + getFileReadStatus={getFileReadStatus} /> )} diff --git a/src/components/shared/DiffRenderer.tsx b/src/components/shared/DiffRenderer.tsx index 216c5ddd1..42ce1bf80 100644 --- a/src/components/shared/DiffRenderer.tsx +++ b/src/components/shared/DiffRenderer.tsx @@ -207,6 +207,8 @@ interface SelectableDiffRendererProps extends DiffRendererProps { filePath: string; /** Callback when user submits a review note */ onReviewNote?: (note: string) => void; + /** Callback when user clicks on a line (to activate parent hunk) */ + onLineClick?: () => void; } interface LineSelection { @@ -227,14 +229,14 @@ const SelectableDiffLineWrapper = styled(DiffLineWrapper)<{ ${({ isSelected }) => isSelected && ` - background: rgba(255, 200, 0, 0.2) !important; + background: hsl(from var(--color-review-accent) h s l / 0.2) !important; `} &:hover { ${({ isSelecting }) => isSelecting && ` - background: rgba(255, 200, 0, 0.1); + background: hsl(from var(--color-review-accent) h s l / 0.1); `} } `; @@ -242,7 +244,7 @@ const SelectableDiffLineWrapper = styled(DiffLineWrapper)<{ const InlineNoteContainer = styled.div` padding: 10px 8px 8px 8px; background: #252526; - border-top: 1px solid rgba(255, 200, 0, 0.3); + border-top: 1px solid hsl(from var(--color-review-accent) h s l / 0.3); margin: 0; `; @@ -253,14 +255,14 @@ const NoteTextarea = styled.textarea` font-family: var(--font-sans); font-size: 11px; background: #1e1e1e; - border: 1px solid rgba(255, 200, 0, 0.4); + border: 1px solid hsl(from var(--color-review-accent) h s l / 0.4); border-radius: 2px; color: var(--color-text); resize: vertical; &:focus { outline: none; - border-color: rgba(255, 200, 0, 0.6); + border-color: hsl(from var(--color-review-accent) h s l / 0.6); } &::placeholder { @@ -276,6 +278,7 @@ export const SelectableDiffRenderer: React.FC = ({ newStart = 1, filePath, onReviewNote, + onLineClick, }) => { const [selection, setSelection] = React.useState(null); const [noteText, setNoteText] = React.useState(""); @@ -333,6 +336,9 @@ export const SelectableDiffRenderer: React.FC = ({ }); const handleClick = (lineIndex: number, shiftKey: boolean) => { + // Notify parent that this hunk should become active + onLineClick?.(); + // Shift-click: extend existing selection if (shiftKey && selection && isSelectingMode) { const start = selection.startIndex; @@ -439,7 +445,7 @@ export const SelectableDiffRenderer: React.FC = ({ setNoteText(e.target.value)} onClick={(e) => e.stopPropagation()} diff --git a/src/hooks/useReviewState.test.ts b/src/hooks/useReviewState.test.ts new file mode 100644 index 000000000..1088814fa --- /dev/null +++ b/src/hooks/useReviewState.test.ts @@ -0,0 +1,86 @@ +/** + * Tests for useReviewState hook + * + * Note: Hook integration tests are omitted because they require jsdom setup. + * The eviction logic is the critical piece and is tested here. + * The hook itself is a thin wrapper around usePersistedState with manual testing. + */ + +import { describe, it, expect } from "bun:test"; +import type { HunkReadState } from "@/types/review"; +import { evictOldestReviews } from "./useReviewState"; + +describe("evictOldestReviews", () => { + it("should not evict when under limit", () => { + const readState: Record = { + "hunk-1": { hunkId: "hunk-1", isRead: true, timestamp: 1 }, + "hunk-2": { hunkId: "hunk-2", isRead: true, timestamp: 2 }, + }; + + const result = evictOldestReviews(readState, 10); + expect(Object.keys(result).length).toBe(2); + }); + + it("should evict oldest entries when exceeding limit", () => { + const readState: Record = { + "hunk-1": { hunkId: "hunk-1", isRead: true, timestamp: 1 }, + "hunk-2": { hunkId: "hunk-2", isRead: true, timestamp: 2 }, + "hunk-3": { hunkId: "hunk-3", isRead: true, timestamp: 3 }, + "hunk-4": { hunkId: "hunk-4", isRead: true, timestamp: 4 }, + "hunk-5": { hunkId: "hunk-5", isRead: true, timestamp: 5 }, + }; + + const result = evictOldestReviews(readState, 3); + expect(Object.keys(result).length).toBe(3); + + // Should keep the newest 3 (timestamps 3, 4, 5) + expect(result["hunk-3"]).toBeDefined(); + expect(result["hunk-4"]).toBeDefined(); + expect(result["hunk-5"]).toBeDefined(); + + // Should evict oldest 2 (timestamps 1, 2) + expect(result["hunk-1"]).toBeUndefined(); + expect(result["hunk-2"]).toBeUndefined(); + }); + + it("should handle exactly at limit", () => { + const readState: Record = { + "hunk-1": { hunkId: "hunk-1", isRead: true, timestamp: 1 }, + "hunk-2": { hunkId: "hunk-2", isRead: true, timestamp: 2 }, + "hunk-3": { hunkId: "hunk-3", isRead: true, timestamp: 3 }, + }; + + const result = evictOldestReviews(readState, 3); + expect(Object.keys(result).length).toBe(3); + expect(result).toEqual(readState); + }); + + it("should handle empty state", () => { + const readState: Record = {}; + const result = evictOldestReviews(readState, 10); + expect(Object.keys(result).length).toBe(0); + }); + + it("should evict to exact limit with many entries", () => { + const readState: Record = {}; + // Create 1100 entries + for (let i = 0; i < 1100; i++) { + readState[`hunk-${i}`] = { + hunkId: `hunk-${i}`, + isRead: true, + timestamp: i, + }; + } + + const result = evictOldestReviews(readState, 1024); + expect(Object.keys(result).length).toBe(1024); + + // Should keep newest 1024 (timestamps 76-1099) + expect(result["hunk-1099"]).toBeDefined(); + expect(result["hunk-76"]).toBeDefined(); + + // Should evict oldest (timestamps 0-75) + expect(result["hunk-0"]).toBeUndefined(); + expect(result["hunk-75"]).toBeUndefined(); + }); +}); diff --git a/src/hooks/useReviewState.ts b/src/hooks/useReviewState.ts index 22ffe1182..743071ce7 100644 --- a/src/hooks/useReviewState.ts +++ b/src/hooks/useReviewState.ts @@ -1,76 +1,120 @@ /** - * Hook for managing code review state - * Provides interface for reading/updating hunk reviews with localStorage persistence + * Hook for managing hunk read state + * Provides interface for tracking which hunks have been reviewed with localStorage persistence */ -import { useCallback, useMemo } from "react"; +import { useCallback, useMemo, useEffect, useState } from "react"; import { usePersistedState } from "./usePersistedState"; -import type { ReviewState, HunkReview, ReviewStats, DiffHunk } from "@/types/review"; +import type { ReviewState, HunkReadState } from "@/types/review"; + +/** + * Maximum number of read states to keep per workspace (LRU eviction) + */ +const MAX_READ_STATES = 1024; /** * Get the localStorage key for review state */ function getReviewStateKey(workspaceId: string): string { - return `code-review:${workspaceId}`; + return `review-state:${workspaceId}`; } /** - * Hook for managing code review state for a workspace - * Persists reviews to localStorage and provides helpers for common operations + * Evict oldest read states if count exceeds max + * Keeps the newest MAX_READ_STATES entries + * Exported for testing */ -export function useReviewState(workspaceId: string) { +export function evictOldestReviews( + readState: Record, + maxCount: number +): Record { + const entries = Object.entries(readState); + if (entries.length <= maxCount) return readState; + + // Sort by timestamp descending (newest first) + entries.sort((a, b) => b[1].timestamp - a[1].timestamp); + + // Keep only the newest maxCount + return Object.fromEntries(entries.slice(0, maxCount)); +} + +export interface UseReviewStateReturn { + /** Check if a hunk is marked as read */ + isRead: (hunkId: string) => boolean; + /** Mark one or more hunks as read */ + markAsRead: (hunkIds: string | string[]) => void; + /** Mark a hunk as unread */ + markAsUnread: (hunkId: string) => void; + /** Toggle read state of a hunk */ + toggleRead: (hunkId: string) => void; + /** Clear all read states */ + clearAll: () => void; + /** Number of hunks marked as read */ + readCount: number; +} + +/** + * Hook for managing hunk read state for a workspace + * Persists read states to localStorage with automatic LRU eviction + */ +export function useReviewState(workspaceId: string): UseReviewStateReturn { const [reviewState, setReviewState] = usePersistedState( getReviewStateKey(workspaceId), { workspaceId, - reviews: {}, + readState: {}, lastUpdated: Date.now(), } ); - /** - * Get review for a specific hunk - */ - const getReview = useCallback( - (hunkId: string): HunkReview | undefined => { - return reviewState.reviews[hunkId]; - }, - [reviewState.reviews] - ); + // Apply LRU eviction on initial load + const [hasAppliedEviction, setHasAppliedEviction] = useState(false); + useEffect(() => { + if (!hasAppliedEviction) { + setHasAppliedEviction(true); + const evicted = evictOldestReviews(reviewState.readState, MAX_READ_STATES); + if (Object.keys(evicted).length !== Object.keys(reviewState.readState).length) { + setReviewState((prev) => ({ + ...prev, + readState: evicted, + lastUpdated: Date.now(), + })); + } + } + }, [hasAppliedEviction, reviewState.readState, setReviewState]); /** - * Set or update a review for a hunk + * Check if a hunk is marked as read */ - const setReview = useCallback( - (hunkId: string, status: "accepted" | "rejected", note?: string) => { - setReviewState((prev) => ({ - ...prev, - reviews: { - ...prev.reviews, - [hunkId]: { - hunkId, - status, - note, - timestamp: Date.now(), - }, - }, - lastUpdated: Date.now(), - })); + const isRead = useCallback( + (hunkId: string): boolean => { + return reviewState.readState[hunkId]?.isRead ?? false; }, - [setReviewState] + [reviewState.readState] ); /** - * Delete a review for a hunk + * Mark one or more hunks as read */ - const deleteReview = useCallback( - (hunkId: string) => { + const markAsRead = useCallback( + (hunkIds: string | string[]) => { + const ids = Array.isArray(hunkIds) ? hunkIds : [hunkIds]; + if (ids.length === 0) return; + + const timestamp = Date.now(); setReviewState((prev) => { - const { [hunkId]: _, ...rest } = prev.reviews; + const newReadState = { ...prev.readState }; + for (const hunkId of ids) { + newReadState[hunkId] = { + hunkId, + isRead: true, + timestamp, + }; + } return { ...prev, - reviews: rest, - lastUpdated: Date.now(), + readState: newReadState, + lastUpdated: timestamp, }; }); }, @@ -78,39 +122,15 @@ export function useReviewState(workspaceId: string) { ); /** - * Clear all reviews - */ - const clearAllReviews = useCallback(() => { - setReviewState((prev) => ({ - ...prev, - reviews: {}, - lastUpdated: Date.now(), - })); - }, [setReviewState]); - - /** - * Remove stale reviews (hunks that no longer exist in the diff) + * Mark a hunk as unread */ - const removeStaleReviews = useCallback( - (currentHunkIds: string[]) => { - const currentIdSet = new Set(currentHunkIds); + const markAsUnread = useCallback( + (hunkId: string) => { setReviewState((prev) => { - const cleanedReviews: Record = {}; - let changed = false; - - for (const [hunkId, review] of Object.entries(prev.reviews)) { - if (currentIdSet.has(hunkId)) { - cleanedReviews[hunkId] = review; - } else { - changed = true; - } - } - - if (!changed) return prev; - + const { [hunkId]: _, ...rest } = prev.readState; return { ...prev, - reviews: cleanedReviews, + readState: rest, lastUpdated: Date.now(), }; }); @@ -119,65 +139,43 @@ export function useReviewState(workspaceId: string) { ); /** - * Calculate review statistics - */ - const stats = useMemo((): ReviewStats => { - const reviews = Object.values(reviewState.reviews); - return { - total: reviews.length, - accepted: reviews.filter((r) => r.status === "accepted").length, - rejected: reviews.filter((r) => r.status === "rejected").length, - unreviewed: 0, // Will be calculated by consumer based on total hunks - }; - }, [reviewState.reviews]); - - /** - * Check if there are any stale reviews (reviews for hunks not in current set) + * Toggle read state of a hunk */ - const hasStaleReviews = useCallback( - (currentHunkIds: string[]): boolean => { - const currentIdSet = new Set(currentHunkIds); - return Object.keys(reviewState.reviews).some((hunkId) => !currentIdSet.has(hunkId)); + const toggleRead = useCallback( + (hunkId: string) => { + if (isRead(hunkId)) { + markAsUnread(hunkId); + } else { + markAsRead(hunkId); + } }, - [reviewState.reviews] + [isRead, markAsRead, markAsUnread] ); /** - * Calculate stats for a specific set of hunks + * Clear all read states */ - const calculateStats = useCallback( - (hunks: DiffHunk[]): ReviewStats => { - const total = hunks.length; - let accepted = 0; - let rejected = 0; - - for (const hunk of hunks) { - const review = reviewState.reviews[hunk.id]; - if (review) { - if (review.status === "accepted") accepted++; - else if (review.status === "rejected") rejected++; - } - } + const clearAll = useCallback(() => { + setReviewState((prev) => ({ + ...prev, + readState: {}, + lastUpdated: Date.now(), + })); + }, [setReviewState]); - return { - total, - accepted, - rejected, - unreviewed: total - accepted - rejected, - }; - }, - [reviewState.reviews] - ); + /** + * Calculate number of read hunks + */ + const readCount = useMemo(() => { + return Object.values(reviewState.readState).filter((state) => state.isRead).length; + }, [reviewState.readState]); return { - reviewState, - getReview, - setReview, - deleteReview, - clearAllReviews, - removeStaleReviews, - hasStaleReviews, - stats, - calculateStats, + isRead, + markAsRead, + markAsUnread, + toggleRead, + clearAll, + readCount, }; } diff --git a/src/styles/colors.tsx b/src/styles/colors.tsx index 25de72911..5e45bdfba 100644 --- a/src/styles/colors.tsx +++ b/src/styles/colors.tsx @@ -37,6 +37,10 @@ export const GlobalColors = () => ( --color-edit-mode-alpha: hsl(from var(--color-edit-mode) h s l / 0.1); --color-edit-mode-alpha-hover: hsl(from var(--color-edit-mode) h s l / 0.15); + /* Read State Colors (Blue - reuses plan mode color for consistency) */ + --color-read: var(--color-plan-mode); + --color-read-alpha: var(--color-plan-mode-alpha); + /* Editing Mode Colors */ --color-editing-mode: hsl(30 100% 50%); --color-editing-mode-alpha: hsl(from var(--color-editing-mode) h s l / 0.1); @@ -112,6 +116,14 @@ export const GlobalColors = () => ( --color-interrupted: hsl(38 92% 50%); /* #f59e0b */ --color-interrupted-alpha: hsl(from var(--color-interrupted) h s l / 0.3); + /* Review/Selection Colors (Yellow/Orange) */ + --color-review-accent: hsl( + 48 70% 50% / 0.75 + ); /* Muted yellow/orange - used for review notes and active hunks */ + /* ANTI-PATTERN: Don't create variants like --color-review-accent-alpha-10, -20, -30, etc. + Instead, use inline alpha: hsl(from var(--color-review-accent) h s l / 0.2) directly in components. + This avoids cluttering color definitions with every possible alpha variant. */ + /* Git Dirty/Uncommitted Changes Colors */ --color-git-dirty: hsl(38 92% 50%); /* Same as interrupted - orange warning color */ diff --git a/src/types/review.ts b/src/types/review.ts index ebfe642d3..49c7cebae 100644 --- a/src/types/review.ts +++ b/src/types/review.ts @@ -45,16 +45,14 @@ export interface FileDiff { } /** - * User's review of a hunk + * Read state for a single hunk */ -export interface HunkReview { - /** ID of the hunk being reviewed */ +export interface HunkReadState { + /** ID of the hunk */ hunkId: string; - /** Review status */ - status: "accepted" | "rejected"; - /** Optional comment/note */ - note?: string; - /** Timestamp when review was created/updated */ + /** Whether this hunk has been marked as read */ + isRead: boolean; + /** Timestamp when read state was last updated */ timestamp: number; } @@ -64,8 +62,8 @@ export interface HunkReview { export interface ReviewState { /** Workspace ID this review belongs to */ workspaceId: string; - /** Reviews keyed by hunk ID */ - reviews: Record; + /** Read state keyed by hunk ID */ + readState: Record; /** Timestamp of last update */ lastUpdated: number; } @@ -74,10 +72,8 @@ export interface ReviewState { * Filter options for review panel */ export interface ReviewFilters { - /** Whether to show already-reviewed hunks */ - showReviewed: boolean; - /** Status filter */ - statusFilter: "all" | "accepted" | "rejected" | "unreviewed"; + /** Whether to show hunks marked as read */ + showReadHunks: boolean; /** File path filter (regex or glob pattern) */ filePathFilter?: string; /** Base reference to diff against (e.g., "HEAD", "main", "origin/main") */ @@ -90,8 +86,10 @@ export interface ReviewFilters { * Review statistics */ export interface ReviewStats { + /** Total number of hunks */ total: number; - accepted: number; - rejected: number; - unreviewed: number; + /** Number of hunks marked as read */ + read: number; + /** Number of unread hunks */ + unread: number; } diff --git a/src/utils/git/diffParser.ts b/src/utils/git/diffParser.ts index 692cc0278..510771ee7 100644 --- a/src/utils/git/diffParser.ts +++ b/src/utils/git/diffParser.ts @@ -5,11 +5,17 @@ import type { DiffHunk, FileDiff } from "@/types/review"; /** - * Generate a stable ID for a hunk based on file path and line ranges + * Generate a stable content-based ID for a hunk + * Uses file path + line range + diff content to ensure uniqueness */ -function generateHunkId(filePath: string, oldStart: number, newStart: number): string { - // Simple hash: combine file path with line numbers - const str = `${filePath}:${oldStart}:${newStart}`; +function generateHunkId( + filePath: string, + oldStart: number, + newStart: number, + content: string +): string { + // Hash file path + line range + diff content for uniqueness and rebase stability + const str = `${filePath}:${oldStart}-${newStart}:${content}`; let hash = 0; for (let i = 0; i < str.length; i++) { const char = str.charCodeAt(i); @@ -54,16 +60,18 @@ export function parseDiff(diffOutput: string): FileDiff[] { const finishHunk = () => { if (currentHunk && currentFile && hunkLines.length > 0) { + const content = hunkLines.join("\n"); const hunkId = generateHunkId( currentFile.filePath, currentHunk.oldStart!, - currentHunk.newStart! + currentHunk.newStart!, + content ); currentFile.hunks.push({ ...currentHunk, id: hunkId, filePath: currentFile.filePath, - content: hunkLines.join("\n"), + content, changeType: currentFile.changeType, oldPath: currentFile.oldPath, } as DiffHunk); diff --git a/src/utils/ui/keybinds.ts b/src/utils/ui/keybinds.ts index 6078101d8..ad1aabd0c 100644 --- a/src/utils/ui/keybinds.ts +++ b/src/utils/ui/keybinds.ts @@ -254,4 +254,7 @@ export const KEYBINDS = { /** Refresh diff in Code Review panel */ // macOS: Cmd+R, Win/Linux: Ctrl+R REFRESH_REVIEW: { key: "r", ctrl: true }, + + /** Mark selected hunk as read/unread in Code Review panel */ + TOGGLE_HUNK_READ: { key: "m" }, } as const;