diff --git a/src/components/RightSidebar/CodeReview/ExpanderArrow.tsx b/src/components/RightSidebar/CodeReview/ExpanderArrow.tsx new file mode 100644 index 000000000..d6799ad0e --- /dev/null +++ b/src/components/RightSidebar/CodeReview/ExpanderArrow.tsx @@ -0,0 +1,72 @@ +/** + * ExpanderArrow - GitHub-style blue arrow for expanding/collapsing context + */ + +import React from "react"; +import { cn } from "@/lib/utils"; + +interface ExpanderArrowProps { + /** Direction of the arrow */ + direction: "up" | "down"; + /** Mode: expand adds lines, collapse removes lines */ + mode: "expand" | "collapse"; + /** Is expansion/collapse in progress? */ + isLoading: boolean; + /** Click handler */ + onClick: (e: React.MouseEvent) => void; + /** Optional marker text to show (e.g., "Beginning of file") */ + markerText?: string; +} + +export const ExpanderArrow = React.memo( + ({ direction, mode, isLoading, onClick, markerText }) => { + // Arrow symbol based on direction and mode + // Expand: always points toward direction (▲ for up, ▼ for down) + // Collapse: always points away from direction (▼ for up, ▲ for down) + const arrow = + mode === "expand" ? (direction === "up" ? "▲" : "▼") : direction === "up" ? "▼" : "▲"; + + // Collapse arrows are more muted + const opacity = mode === "collapse" ? 0.5 : 1; + + return ( +
+
+ {/* Indicator column - matches diff line structure */} + · + + {/* Line number column - matches diff line structure */} + + {isLoading ? ( + ... + ) : ( + {arrow} + )} + + + {/* Content area - matches diff line structure */} + {markerText ? ( + + • + {markerText} + + ) : ( + {isLoading ? "Loading..." : ""} + )} +
+
+ ); + } +); + +ExpanderArrow.displayName = "ExpanderArrow"; diff --git a/src/components/RightSidebar/CodeReview/HunkContent.tsx b/src/components/RightSidebar/CodeReview/HunkContent.tsx new file mode 100644 index 000000000..b87136538 --- /dev/null +++ b/src/components/RightSidebar/CodeReview/HunkContent.tsx @@ -0,0 +1,167 @@ +/** + * HunkContent - Main content area for a hunk with read-more functionality + */ + +import React, { useMemo } from "react"; +import type { DiffHunk, HunkReadMoreState } from "@/types/review"; +import type { SearchHighlightConfig } from "@/utils/highlighting/highlightSearchTerms"; +import { SelectableDiffRenderer } from "../../shared/DiffRenderer"; +import { ExpanderArrow } from "./ExpanderArrow"; +import { calculateUpwardExpansion } from "@/utils/review/readFileLines"; + +interface ExpansionState { + content: string; + isLoading: boolean; + onExpand: (e: React.MouseEvent) => void; + onCollapse: (e: React.MouseEvent) => void; + isExpanded: boolean; + canExpand: boolean; +} + +interface HunkContentProps { + /** The hunk to display */ + hunk: DiffHunk; + /** Hunk ID for event handling */ + hunkId: string; + /** Read-more expansion state */ + readMoreState: HunkReadMoreState; + /** Upward expansion state */ + upExpansion: ExpansionState; + /** Downward expansion state */ + downExpansion: ExpansionState; + /** Handler for line clicks (triggers parent onClick) */ + onClick?: (e: React.MouseEvent) => void; + /** Handler for review notes */ + onReviewNote?: (note: string) => void; + /** Search configuration for highlighting */ + searchConfig?: SearchHighlightConfig; +} + +export const HunkContent = React.memo( + ({ + hunk, + hunkId, + readMoreState, + upExpansion, + downExpansion, + onClick, + onReviewNote, + searchConfig, + }) => { + // Calculate expansion metadata + const upwardExpansion = calculateUpwardExpansion(hunk.oldStart, readMoreState.up); + const canExpandUp = upwardExpansion.startLine >= 1 && upwardExpansion.numLines > 0; + + // Check if we've reached beginning of file (line 1) + const atBeginningOfFile = upExpansion.isExpanded && upwardExpansion.startLine === 1; + + // Detect EOF: multiple scenarios + // 1. Expanded down and got fewer lines than requested + // 2. Expanded down and got empty/no content (hunk was already at EOF) + const atEndOfFile = useMemo(() => { + // If we've never expanded, we don't know if we're at EOF yet + if (readMoreState.down === 0) return false; + + // If we expanded but got no content, we're at EOF + if (!downExpansion.content?.trim().length) { + return true; + } + + const lines = downExpansion.content.split("\n").filter((l) => l.length > 0); + // If we got fewer lines than requested, we're at EOF + return lines.length < readMoreState.down; + }, [downExpansion.content, readMoreState.down]); + + // Combine all content into single unified diff for proper syntax highlighting + // This ensures grammar state (multi-line comments, strings, etc.) spans correctly + const combinedContent = useMemo(() => { + const parts: string[] = []; + + if (upExpansion.content) { + parts.push(upExpansion.content); + } + + parts.push(hunk.content); + + if (downExpansion.content) { + parts.push(downExpansion.content); + } + + return parts.join("\n"); + }, [upExpansion.content, hunk.content, downExpansion.content]); + + // Calculate starting line number for combined content + const combinedStartLine = upExpansion.content ? upwardExpansion.startLine : hunk.oldStart; + + return ( +
+ { + // Create synthetic event with data-hunk-id for parent handler + const syntheticEvent = { + currentTarget: { dataset: { hunkId } }, + } as unknown as React.MouseEvent; + onClick?.(syntheticEvent); + }} + searchConfig={searchConfig} + expanderTop={ + <> + {/* Collapse arrow - show if currently expanded, with BOF marker if at beginning */} + {upExpansion.isExpanded && ( + + )} + + {/* Expand arrow - show if can expand more */} + {canExpandUp && !atBeginningOfFile && ( + + )} + + } + expanderBottom={ + <> + {/* Expand arrow - show if can expand more */} + {downExpansion.canExpand && !atEndOfFile && ( + + )} + + {/* Collapse arrow - show if currently expanded, with EOF marker if at end */} + {downExpansion.isExpanded && ( + + )} + + } + /> +
+ ); + } +); + +HunkContent.displayName = "HunkContent"; diff --git a/src/components/RightSidebar/CodeReview/HunkHeader.tsx b/src/components/RightSidebar/CodeReview/HunkHeader.tsx new file mode 100644 index 000000000..f99712d92 --- /dev/null +++ b/src/components/RightSidebar/CodeReview/HunkHeader.tsx @@ -0,0 +1,89 @@ +/** + * HunkHeader - Header section of a diff hunk showing file path, stats, and controls + */ + +import React from "react"; +import { Tooltip, TooltipWrapper } from "../../Tooltip"; +import { KEYBINDS, formatKeybind } from "@/utils/ui/keybinds"; + +interface HunkHeaderProps { + /** File path (may contain HTML from search highlighting) */ + highlightedFilePath: string; + /** Whether the hunk is marked as read */ + isRead: boolean; + /** Number of additions in the hunk */ + additions: number; + /** Number of deletions in the hunk */ + deletions: number; + /** Total line count */ + lineCount: number; + /** Whether this is a pure rename (no content changes) */ + isPureRename: boolean; + /** Hunk ID for event handling */ + hunkId: string; + /** Callback when toggle read button is clicked */ + onToggleRead?: (e: React.MouseEvent) => void; +} + +export const HunkHeader = React.memo( + ({ + highlightedFilePath, + isRead, + additions, + deletions, + lineCount, + isPureRename, + hunkId, + onToggleRead, + }) => { + return ( +
+ {isRead && ( + + + ✓ + + + Marked as read + + + )} +
+
+ {!isPureRename && ( + + {additions > 0 && +{additions}} + {deletions > 0 && -{deletions}} + + )} + + ({lineCount} {lineCount === 1 ? "line" : "lines"}) + + {onToggleRead && ( + + + + Mark as read ({formatKeybind(KEYBINDS.TOGGLE_HUNK_READ)}) + + + )} +
+
+ ); + } +); + +HunkHeader.displayName = "HunkHeader"; diff --git a/src/components/RightSidebar/CodeReview/HunkViewer.tsx b/src/components/RightSidebar/CodeReview/HunkViewer.tsx index a8a4b6e34..fef3ed683 100644 --- a/src/components/RightSidebar/CodeReview/HunkViewer.tsx +++ b/src/components/RightSidebar/CodeReview/HunkViewer.tsx @@ -3,17 +3,22 @@ */ import React, { useState, useMemo } from "react"; -import type { DiffHunk } from "@/types/review"; -import { SelectableDiffRenderer } from "../../shared/DiffRenderer"; -import { - type SearchHighlightConfig, - highlightSearchInText, -} from "@/utils/highlighting/highlightSearchTerms"; -import { Tooltip, TooltipWrapper } from "../../Tooltip"; +import type { DiffHunk, HunkReadMoreState } from "@/types/review"; +import type { SearchHighlightConfig } from "@/utils/highlighting/highlightSearchTerms"; +import { highlightSearchInText } from "@/utils/highlighting/highlightSearchTerms"; import { usePersistedState } from "@/hooks/usePersistedState"; -import { getReviewExpandStateKey } from "@/constants/storage"; +import { getReviewExpandStateKey, getReviewReadMoreStateKey } from "@/constants/storage"; import { KEYBINDS, formatKeybind } from "@/utils/ui/keybinds"; import { cn } from "@/lib/utils"; +import { + readFileLines, + calculateUpwardExpansion, + calculateDownwardExpansion, + formatAsContextLines, + getOldFileRef, +} from "@/utils/review/readFileLines"; +import { HunkHeader } from "./HunkHeader"; +import { HunkContent } from "./HunkContent"; interface HunkViewerProps { hunk: DiffHunk; @@ -26,6 +31,10 @@ interface HunkViewerProps { onRegisterToggleExpand?: (hunkId: string, toggleFn: () => void) => void; onReviewNote?: (note: string) => void; searchConfig?: SearchHighlightConfig; + /** Diff base for determining which git ref to read from */ + diffBase: string; + /** Whether uncommitted changes are included in the diff */ + includeUncommitted: boolean; } export const HunkViewer = React.memo( @@ -40,6 +49,8 @@ export const HunkViewer = React.memo( onRegisterToggleExpand, onReviewNote, searchConfig, + diffBase, + includeUncommitted, }) => { // Parse diff lines (memoized - only recompute if hunk.content changes) // Must be done before state initialization to determine initial collapse state @@ -105,6 +116,82 @@ export const HunkViewer = React.memo( } }, [hasManualState, manualExpandState]); + // Read-more state: tracks expanded lines up/down per hunk + const [readMoreStateMap, setReadMoreStateMap] = usePersistedState< + Record + >(getReviewReadMoreStateKey(workspaceId), {}, { listener: true }); + + const readMoreState = useMemo( + () => readMoreStateMap[hunkId] || { up: 0, down: 0 }, + [readMoreStateMap, hunkId] + ); + + // State for expanded content + const [expandedContentUp, setExpandedContentUp] = useState(""); + const [expandedContentDown, setExpandedContentDown] = useState(""); + const [isLoadingUp, setIsLoadingUp] = useState(false); + const [isLoadingDown, setIsLoadingDown] = useState(false); + + // Determine which git ref to read the old file from + const gitRef = useMemo( + () => getOldFileRef(diffBase, includeUncommitted), + [diffBase, includeUncommitted] + ); + + // Load expanded content when read-more state changes + React.useEffect(() => { + if (readMoreState.up > 0) { + const expansion = calculateUpwardExpansion(hunk.oldStart, readMoreState.up); + if (expansion.numLines > 0) { + setIsLoadingUp(true); + void readFileLines( + workspaceId, + hunk.filePath, + expansion.startLine, + expansion.endLine, + gitRef + ) + .then((lines) => { + if (lines) { + setExpandedContentUp(formatAsContextLines(lines)); + } + }) + .finally(() => setIsLoadingUp(false)); + } + } else { + setExpandedContentUp(""); + } + }, [readMoreState.up, hunk.oldStart, hunk.filePath, workspaceId, gitRef]); + + React.useEffect(() => { + if (readMoreState.down > 0) { + const expansion = calculateDownwardExpansion( + hunk.oldStart, + hunk.oldLines, + readMoreState.down + ); + setIsLoadingDown(true); + void readFileLines( + workspaceId, + hunk.filePath, + expansion.startLine, + expansion.endLine, + gitRef + ) + .then((lines) => { + if (lines) { + setExpandedContentDown(formatAsContextLines(lines)); + } else { + // No lines returned - at EOF + setExpandedContentDown(""); + } + }) + .finally(() => setIsLoadingDown(false)); + } else { + setExpandedContentDown(""); + } + }, [readMoreState.down, hunk.oldStart, hunk.oldLines, hunk.filePath, workspaceId, gitRef]); + const handleToggleExpand = React.useCallback( (e?: React.MouseEvent) => { e?.stopPropagation(); @@ -131,9 +218,72 @@ export const HunkViewer = React.memo( onToggleRead?.(e); }; + const handleExpandUp = React.useCallback( + (e: React.MouseEvent) => { + e.stopPropagation(); + const expansion = calculateUpwardExpansion(hunk.oldStart, readMoreState.up); + if (expansion.startLine < 1 || expansion.numLines <= 0) { + // Already at beginning of file + return; + } + setReadMoreStateMap((prev) => ({ + ...prev, + [hunkId]: { + ...readMoreState, + up: readMoreState.up + 30, + }, + })); + }, + [hunkId, hunk.oldStart, readMoreState, setReadMoreStateMap] + ); + + const handleCollapseUp = React.useCallback( + (e: React.MouseEvent) => { + e.stopPropagation(); + const newExpansion = Math.max(0, readMoreState.up - 30); + setReadMoreStateMap((prev) => ({ + ...prev, + [hunkId]: { + ...readMoreState, + up: newExpansion, + }, + })); + }, + [hunkId, readMoreState, setReadMoreStateMap] + ); + + const handleExpandDown = React.useCallback( + (e: React.MouseEvent) => { + e.stopPropagation(); + setReadMoreStateMap((prev) => ({ + ...prev, + [hunkId]: { + ...readMoreState, + down: readMoreState.down + 30, + }, + })); + }, + [hunkId, readMoreState, setReadMoreStateMap] + ); + + const handleCollapseDown = React.useCallback( + (e: React.MouseEvent) => { + e.stopPropagation(); + const newExpansion = Math.max(0, readMoreState.down - 30); + setReadMoreStateMap((prev) => ({ + ...prev, + [hunkId]: { + ...readMoreState, + down: newExpansion, + }, + })); + }, + [hunkId, readMoreState, setReadMoreStateMap] + ); + // Detect pure rename: if renamed and content hasn't changed (zero additions and deletions) const isPureRename = - hunk.changeType === "renamed" && hunk.oldPath && additions === 0 && deletions === 0; + hunk.changeType === "renamed" && !!hunk.oldPath && additions === 0 && deletions === 0; return (
( tabIndex={0} data-hunk-id={hunkId} > -
- {isRead && ( - - - ✓ - - - Marked as read - - - )} -
-
- {!isPureRename && ( - - {additions > 0 && +{additions}} - {deletions > 0 && -{deletions}} - - )} - - ({lineCount} {lineCount === 1 ? "line" : "lines"}) - - {onToggleRead && ( - - - - Mark as read ({formatKeybind(KEYBINDS.TOGGLE_HUNK_READ)}) - - - )} -
-
+ {isPureRename ? (
Renamed from {hunk.oldPath}
) : isExpanded ? ( -
- { - // Create synthetic event with data-hunk-id for parent handler - const syntheticEvent = { - currentTarget: { dataset: { hunkId } }, - } as unknown as React.MouseEvent; - onClick?.(syntheticEvent); - }} - searchConfig={searchConfig} - /> -
+ 0, + canExpand: calculateUpwardExpansion(hunk.oldStart, readMoreState.up).numLines > 0, + }} + downExpansion={{ + content: expandedContentDown, + isLoading: isLoadingDown, + onExpand: handleExpandDown, + onCollapse: handleCollapseDown, + isExpanded: readMoreState.down > 0, + canExpand: true, // Always allow expanding downward + }} + onClick={onClick} + onReviewNote={onReviewNote} + searchConfig={searchConfig} + /> ) : (
void; +} + +export const ReadMoreButton = React.memo( + ({ direction, action, numLines, isLoading, disabled = false, onClick }) => { + const arrow = direction === "up" ? "↑" : "↓"; + const verb = action === "expand" ? "Read" : "Show"; + const qualifier = action === "expand" ? "more" : "fewer"; + + const label = isLoading ? "Loading..." : `${verb} ${numLines} ${qualifier} lines ${arrow}`; + + return ( +
+ +
+ ); + } +); + +ReadMoreButton.displayName = "ReadMoreButton"; diff --git a/src/components/RightSidebar/CodeReview/ReviewPanel.tsx b/src/components/RightSidebar/CodeReview/ReviewPanel.tsx index 2b1357130..324594271 100644 --- a/src/components/RightSidebar/CodeReview/ReviewPanel.tsx +++ b/src/components/RightSidebar/CodeReview/ReviewPanel.tsx @@ -29,13 +29,18 @@ import { FileTree } from "./FileTree"; import { usePersistedState } from "@/hooks/usePersistedState"; import { useReviewState } from "@/hooks/useReviewState"; import { parseDiff, extractAllHunks } from "@/utils/git/diffParser"; -import { getReviewSearchStateKey } from "@/constants/storage"; +import { getReviewSearchStateKey, getReviewReadMoreStateKey } from "@/constants/storage"; import { Tooltip, TooltipWrapper } from "@/components/Tooltip"; import { parseNumstat, buildFileTree, extractNewPath } from "@/utils/git/numstatParser"; -import type { DiffHunk, ReviewFilters as ReviewFiltersType } from "@/types/review"; +import type { + DiffHunk, + ReviewFilters as ReviewFiltersType, + HunkReadMoreState, +} from "@/types/review"; import type { FileTreeNode } from "@/utils/git/numstatParser"; import { matchesKeybind, KEYBINDS, formatKeybind } from "@/utils/ui/keybinds"; import { applyFrontendFilters } from "@/utils/review/filterHunks"; +import { combineOverlappingHunks } from "@/utils/review/combineHunks"; import { cn } from "@/lib/utils"; interface ReviewPanelProps { @@ -371,6 +376,23 @@ export const ReviewPanel: React.FC = ({ searchState.matchCase, ]); + // Load read-more state for combining logic + const [readMoreStateMap] = usePersistedState>( + getReviewReadMoreStateKey(workspaceId), + {}, + { listener: true } + ); + + // Combine overlapping hunks based on expansion state + const combinedHunks = useMemo(() => { + const hunksWithExpansion = filteredHunks.map((hunk) => ({ + hunk, + hunkId: hunk.id, + expansion: readMoreStateMap[hunk.id] || { up: 0, down: 0 }, + })); + return combineOverlappingHunks(hunksWithExpansion); + }, [filteredHunks, readMoreStateMap]); + // Memoize search config to prevent re-creating object on every render // This allows React.memo on HunkViewer to work properly const searchConfig = useMemo( @@ -425,14 +447,6 @@ export const ReviewPanel: React.FC = ({ if (hunkId) setSelectedHunkId(hunkId); }, []); - const handleHunkToggleRead = useCallback( - (e: React.MouseEvent) => { - const hunkId = e.currentTarget.dataset.hunkId; - if (hunkId) handleToggleRead(hunkId); - }, - [handleToggleRead] - ); - const handleRegisterToggleExpand = useCallback((hunkId: string, toggleFn: () => void) => { toggleExpandFnsRef.current.set(hunkId, toggleFn); }, []); @@ -674,7 +688,7 @@ export const ReviewPanel: React.FC = ({ )}
- ) : filteredHunks.length === 0 ? ( + ) : combinedHunks.length === 0 ? (
{debouncedSearchTerm.trim() @@ -685,23 +699,30 @@ export const ReviewPanel: React.FC = ({
) : ( - filteredHunks.map((hunk) => { - const isSelected = hunk.id === selectedHunkId; - const hunkIsRead = isRead(hunk.id); + combinedHunks.map((combined) => { + const isSelected = combined.combinedId === selectedHunkId; + // Combined hunk is "read" if all source hunks are read + const hunkIsRead = combined.sourceHunks.every((h) => isRead(h.hunkId)); return ( { + // When toggling a combined hunk, apply to all source hunks + e.stopPropagation(); + combined.sourceHunks.forEach((h) => handleToggleRead(h.hunkId)); + }} onRegisterToggleExpand={handleRegisterToggleExpand} onReviewNote={onReviewNote} searchConfig={searchConfig} + diffBase={filters.diffBase} + includeUncommitted={filters.includeUncommitted} /> ); }) diff --git a/src/components/shared/DiffRenderer.tsx b/src/components/shared/DiffRenderer.tsx index ebd375f57..c5fd521ec 100644 --- a/src/components/shared/DiffRenderer.tsx +++ b/src/components/shared/DiffRenderer.tsx @@ -260,6 +260,10 @@ interface SelectableDiffRendererProps extends Omit void; /** Search highlight configuration (optional) */ searchConfig?: SearchHighlightConfig; + /** Optional expander component to render at top of grid */ + expanderTop?: React.ReactNode; + /** Optional expander component to render at bottom of grid */ + expanderBottom?: React.ReactNode; } interface LineSelection { @@ -386,6 +390,8 @@ export const SelectableDiffRenderer = React.memo( onReviewNote, onLineClick, searchConfig, + expanderTop, + expanderBottom, }) => { const [selection, setSelection] = React.useState(null); @@ -506,6 +512,7 @@ export const SelectableDiffRenderer = React.memo( gridTemplateColumns: "minmax(min-content, 1fr)", }} > + {expanderTop} {highlightedLineData.map((lineInfo, displayIndex) => { const isSelected = isLineSelected(displayIndex); const indicator = lineInfo.type === "add" ? "+" : lineInfo.type === "remove" ? "-" : " "; @@ -603,6 +610,7 @@ export const SelectableDiffRenderer = React.memo( ); })} + {expanderBottom}
); } diff --git a/src/constants/storage.ts b/src/constants/storage.ts index 0dbf81318..46817d2d3 100644 --- a/src/constants/storage.ts +++ b/src/constants/storage.ts @@ -111,6 +111,15 @@ export function getReviewSearchStateKey(workspaceId: string): string { return `reviewSearchState:${workspaceId}`; } +/** + * Get the localStorage key for read-more expansion state in Review tab + * Stores user's expanded context preferences per hunk + * Format: "reviewReadMoreState:{workspaceId}" + */ +export function getReviewReadMoreStateKey(workspaceId: string): string { + return `reviewReadMoreState:${workspaceId}`; +} + /** * List of workspace-scoped key functions that should be copied on fork and deleted on removal * Note: Excludes ephemeral keys like getCompactContinueMessageKey @@ -125,6 +134,7 @@ const PERSISTENT_WORKSPACE_KEY_FUNCTIONS: Array<(workspaceId: string) => string> getReviewExpandStateKey, getFileTreeExpandStateKey, getReviewSearchStateKey, + getReviewReadMoreStateKey, ]; /** diff --git a/src/types/review.ts b/src/types/review.ts index 2a4e2fe9b..34c36a85f 100644 --- a/src/types/review.ts +++ b/src/types/review.ts @@ -93,3 +93,14 @@ export interface ReviewStats { /** Number of unread hunks */ unread: number; } + +/** + * Read-more expansion state for a hunk + * Tracks how many lines have been expanded in each direction + */ +export interface HunkReadMoreState { + /** Lines expanded upward from the hunk start */ + up: number; + /** Lines expanded downward from the hunk end */ + down: number; +} diff --git a/src/utils/review/combineHunks.test.ts b/src/utils/review/combineHunks.test.ts new file mode 100644 index 000000000..a6aff6e30 --- /dev/null +++ b/src/utils/review/combineHunks.test.ts @@ -0,0 +1,247 @@ +import { describe, test, expect } from "bun:test"; +import { combineOverlappingHunks, type HunkWithExpansion } from "./combineHunks"; +import type { DiffHunk } from "@/types/review"; + +describe("combineOverlappingHunks", () => { + // Helper to create a simple hunk + const createHunk = (oldStart: number, oldLines: number, content = "test"): DiffHunk => ({ + id: `hunk-${oldStart}`, + oldStart, + oldLines, + newStart: oldStart, + newLines: oldLines, + content, + filePath: "test.ts", + changeType: "modified", + header: `@@ -${oldStart},${oldLines} +${oldStart},${oldLines} @@`, + }); + + test("returns single hunk unchanged", () => { + const hunks: HunkWithExpansion[] = [ + { + hunk: createHunk(10, 5), + hunkId: "hunk-1", + expansion: { up: 0, down: 0 }, + }, + ]; + + const result = combineOverlappingHunks(hunks); + + expect(result).toHaveLength(1); + expect(result[0].combinedId).toBe("hunk-1"); + expect(result[0].sourceHunks).toHaveLength(1); + }); + + test("keeps non-overlapping hunks separate", () => { + const hunks: HunkWithExpansion[] = [ + { + hunk: createHunk(10, 5), // lines 10-14 + hunkId: "hunk-1", + expansion: { up: 0, down: 0 }, + }, + { + hunk: createHunk(50, 5), // lines 50-54 (far away) + hunkId: "hunk-2", + expansion: { up: 0, down: 0 }, + }, + ]; + + const result = combineOverlappingHunks(hunks); + + expect(result).toHaveLength(2); + expect(result[0].combinedId).toBe("hunk-1"); + expect(result[1].combinedId).toBe("hunk-2"); + }); + + test("combines hunks that overlap with expansion", () => { + const hunks: HunkWithExpansion[] = [ + { + hunk: createHunk(10, 5, "hunk1"), // lines 10-14 + hunkId: "hunk-1", + expansion: { up: 0, down: 30 }, // expand down to line 44 + }, + { + hunk: createHunk(40, 5, "hunk2"), // lines 40-44 (overlaps!) + hunkId: "hunk-2", + expansion: { up: 0, down: 0 }, + }, + ]; + + const result = combineOverlappingHunks(hunks); + + expect(result).toHaveLength(1); + expect(result[0].combinedId).toBe("hunk-1+hunk-2"); + expect(result[0].sourceHunks).toHaveLength(2); + expect(result[0].sourceHunks[0].hunkId).toBe("hunk-1"); + expect(result[0].sourceHunks[1].hunkId).toBe("hunk-2"); + }); + + test("combines adjacent hunks (within 3 lines)", () => { + const hunks: HunkWithExpansion[] = [ + { + hunk: createHunk(10, 5), // lines 10-14 + hunkId: "hunk-1", + expansion: { up: 0, down: 0 }, + }, + { + hunk: createHunk(17, 5), // lines 17-21 (gap of 2 lines) + hunkId: "hunk-2", + expansion: { up: 0, down: 0 }, + }, + ]; + + const result = combineOverlappingHunks(hunks); + + expect(result).toHaveLength(1); + expect(result[0].combinedId).toBe("hunk-1+hunk-2"); + }); + + test("does not combine hunks with gap > 3 lines", () => { + const hunks: HunkWithExpansion[] = [ + { + hunk: createHunk(10, 5), // lines 10-14 + hunkId: "hunk-1", + expansion: { up: 0, down: 0 }, + }, + { + hunk: createHunk(19, 5), // lines 19-23 (gap of 4 lines) + hunkId: "hunk-2", + expansion: { up: 0, down: 0 }, + }, + ]; + + const result = combineOverlappingHunks(hunks); + + expect(result).toHaveLength(2); + }); + + test("combines multiple overlapping hunks into one", () => { + const hunks: HunkWithExpansion[] = [ + { + hunk: createHunk(10, 5), + hunkId: "hunk-1", + expansion: { up: 0, down: 30 }, // expand to line 44 + }, + { + hunk: createHunk(40, 5), + hunkId: "hunk-2", + expansion: { up: 0, down: 20 }, // expand to line 64 + }, + { + hunk: createHunk(60, 5), + hunkId: "hunk-3", + expansion: { up: 0, down: 0 }, + }, + ]; + + const result = combineOverlappingHunks(hunks); + + expect(result).toHaveLength(1); + expect(result[0].combinedId).toBe("hunk-1+hunk-2+hunk-3"); + expect(result[0].sourceHunks).toHaveLength(3); + }); + + test("preserves max expansion state", () => { + const hunks: HunkWithExpansion[] = [ + { + hunk: createHunk(10, 5), + hunkId: "hunk-1", + expansion: { up: 30, down: 60 }, + }, + { + hunk: createHunk(12, 5), + hunkId: "hunk-2", + expansion: { up: 60, down: 30 }, // higher up, lower down + }, + ]; + + const result = combineOverlappingHunks(hunks); + + expect(result).toHaveLength(1); + expect(result[0].expansion).toEqual({ up: 60, down: 60 }); // max of each + }); + + test("handles hunks in any order", () => { + const hunks: HunkWithExpansion[] = [ + { + hunk: createHunk(40, 5), + hunkId: "hunk-2", + expansion: { up: 0, down: 0 }, + }, + { + hunk: createHunk(10, 5), + hunkId: "hunk-1", + expansion: { up: 0, down: 30 }, + }, + ]; + + const result = combineOverlappingHunks(hunks); + + expect(result).toHaveLength(1); + // Should be sorted by line number + expect(result[0].sourceHunks[0].hunkId).toBe("hunk-1"); + expect(result[0].sourceHunks[1].hunkId).toBe("hunk-2"); + }); + + test("handles upward expansion causing overlap", () => { + const hunks: HunkWithExpansion[] = [ + { + hunk: createHunk(10, 5), + hunkId: "hunk-1", + expansion: { up: 0, down: 0 }, + }, + { + hunk: createHunk(40, 5), + hunkId: "hunk-2", + expansion: { up: 30, down: 0 }, // expand up to line 10 (overlaps!) + }, + ]; + + const result = combineOverlappingHunks(hunks); + + expect(result).toHaveLength(1); + expect(result[0].combinedId).toBe("hunk-1+hunk-2"); + }); +}); + +test("does not combine hunks from different files", () => { + const hunks: HunkWithExpansion[] = [ + { + hunk: { + id: "hunk-1", + oldStart: 10, + oldLines: 5, + newStart: 10, + newLines: 5, + content: "test", + filePath: "file1.ts", + changeType: "modified", + header: "@@ -10,5 +10,5 @@", + }, + hunkId: "hunk-1", + expansion: { up: 0, down: 30 }, // Expands to line 44 + }, + { + hunk: { + id: "hunk-2", + oldStart: 40, + oldLines: 5, + newStart: 40, + newLines: 5, + content: "test", + filePath: "file2.ts", // DIFFERENT FILE + changeType: "modified", + header: "@@ -40,5 +40,5 @@", + }, + hunkId: "hunk-2", + expansion: { up: 0, down: 0 }, + }, + ]; + + const result = combineOverlappingHunks(hunks); + + // Should keep hunks separate because they're from different files + expect(result).toHaveLength(2); + expect(result[0].sourceHunks[0].hunk.filePath).toBe("file1.ts"); + expect(result[1].sourceHunks[0].hunk.filePath).toBe("file2.ts"); +}); diff --git a/src/utils/review/combineHunks.ts b/src/utils/review/combineHunks.ts new file mode 100644 index 000000000..9f5c50305 --- /dev/null +++ b/src/utils/review/combineHunks.ts @@ -0,0 +1,167 @@ +/** + * combineHunks - Combines hunks that overlap when expansion is applied + * + * When hunks are expanded, their context may overlap. This function detects + * overlaps and combines hunks into composite hunks for display. + */ + +import type { DiffHunk, HunkReadMoreState } from "@/types/review"; +import { calculateUpwardExpansion, calculateDownwardExpansion } from "./readFileLines"; + +export interface HunkWithExpansion { + hunk: DiffHunk; + hunkId: string; + expansion: HunkReadMoreState; +} + +export interface CombinedHunk { + /** Combined hunk for display (merged content) */ + displayHunk: DiffHunk; + /** ID for the combined hunk */ + combinedId: string; + /** Original hunks that were combined */ + sourceHunks: HunkWithExpansion[]; + /** Expansion state for display (max of all source hunks) */ + expansion: HunkReadMoreState; +} + +/** + * Calculate the effective line range of a hunk after expansion + */ +function getEffectiveRange( + hunk: DiffHunk, + expansion: HunkReadMoreState +): { start: number; end: number } { + const upwardExp = calculateUpwardExpansion(hunk.oldStart, expansion.up); + const downwardExp = calculateDownwardExpansion(hunk.oldStart, hunk.oldLines, expansion.down); + + const start = upwardExp.startLine >= 1 ? upwardExp.startLine : hunk.oldStart; + const end = downwardExp.endLine; + + return { start, end }; +} + +/** + * Check if two hunks overlap based on their expanded ranges + * Only hunks from the same file can overlap + */ +function hunksOverlap( + hunk1: HunkWithExpansion, + hunk2: HunkWithExpansion, + range1: { start: number; end: number }, + range2: { start: number; end: number } +): boolean { + // Different files cannot overlap + if (hunk1.hunk.filePath !== hunk2.hunk.filePath) { + return false; + } + + // Check if ranges overlap or are adjacent (within 3 lines) + // Adjacent hunks should be combined for cleaner display + const gap = range2.start - range1.end; + return gap <= 3; +} + +/** + * Combine multiple hunks into a single display hunk + */ +function mergeHunks(hunksToMerge: HunkWithExpansion[]): CombinedHunk { + if (hunksToMerge.length === 0) { + throw new Error("Cannot merge empty hunk list"); + } + + if (hunksToMerge.length === 1) { + const { hunk, hunkId, expansion } = hunksToMerge[0]; + return { + displayHunk: hunk, + combinedId: hunkId, + sourceHunks: hunksToMerge, + expansion, + }; + } + + // Sort by line number + const sorted = [...hunksToMerge].sort((a, b) => a.hunk.oldStart - b.hunk.oldStart); + + const first = sorted[0]; + const last = sorted[sorted.length - 1]; + + // Calculate combined range + const firstRange = getEffectiveRange(first.hunk, first.expansion); + const lastRange = getEffectiveRange(last.hunk, last.expansion); + + // Combine content (we'll just concatenate for now - the actual rendering + // will handle showing the expanded content correctly) + const combinedContent = sorted.map((h) => h.hunk.content).join("\n"); + + // Create combined hunk + const displayHunk: DiffHunk = { + ...first.hunk, + oldStart: firstRange.start, + oldLines: lastRange.end - firstRange.start + 1, + content: combinedContent, + }; + + // Calculate max expansion (we want to preserve the most expanded state) + const maxExpansion: HunkReadMoreState = { + up: Math.max(...sorted.map((h) => h.expansion.up)), + down: Math.max(...sorted.map((h) => h.expansion.down)), + }; + + // Generate combined ID from all source IDs + const combinedId = sorted.map((h) => h.hunkId).join("+"); + + return { + displayHunk, + combinedId, + sourceHunks: sorted, + expansion: maxExpansion, + }; +} + +/** + * Combine hunks that overlap after expansion is applied + * + * @param hunks - Array of hunks with their expansion states + * @returns Array of combined hunks (some may be single hunks, some combined) + */ +export function combineOverlappingHunks(hunks: HunkWithExpansion[]): CombinedHunk[] { + if (hunks.length === 0) { + return []; + } + + // Sort by file path first, then by starting line number + // This ensures we process files separately + const sorted = [...hunks].sort((a, b) => { + const fileCompare = a.hunk.filePath.localeCompare(b.hunk.filePath); + if (fileCompare !== 0) return fileCompare; + return a.hunk.oldStart - b.hunk.oldStart; + }); + + const result: CombinedHunk[] = []; + let currentGroup: HunkWithExpansion[] = [sorted[0]]; + let currentRange = getEffectiveRange(sorted[0].hunk, sorted[0].expansion); + + for (let i = 1; i < sorted.length; i++) { + const current = sorted[i]; + const currentHunkRange = getEffectiveRange(current.hunk, current.expansion); + + // Pass the hunk objects to check file path matching + if (hunksOverlap(currentGroup[0], current, currentRange, currentHunkRange)) { + // Overlaps - add to current group + currentGroup.push(current); + // Extend the range to include this hunk + currentRange.end = Math.max(currentRange.end, currentHunkRange.end); + } else { + // No overlap - finalize current group and start new one + result.push(mergeHunks(currentGroup)); + currentGroup = [current]; + currentRange = currentHunkRange; + } + } + + // Don't forget the last group + result.push(mergeHunks(currentGroup)); + + return result; +} diff --git a/src/utils/review/readFileLines.test.ts b/src/utils/review/readFileLines.test.ts new file mode 100644 index 000000000..7fbf5a324 --- /dev/null +++ b/src/utils/review/readFileLines.test.ts @@ -0,0 +1,154 @@ +import { describe, it, expect } from "bun:test"; +import { calculateUpwardExpansion, calculateDownwardExpansion } from "./readFileLines"; + +describe("calculateUpwardExpansion", () => { + it("returns cumulative range from start to hunk", () => { + const oldStart = 100; + + // First expansion: 30 lines + const exp1 = calculateUpwardExpansion(oldStart, 30); + expect(exp1.startLine).toBe(70); + expect(exp1.endLine).toBe(99); + expect(exp1.numLines).toBe(30); + + // Second expansion: 60 lines total (cumulative) + const exp2 = calculateUpwardExpansion(oldStart, 60); + expect(exp2.startLine).toBe(40); + expect(exp2.endLine).toBe(99); // Same endLine - always up to hunk + expect(exp2.numLines).toBe(60); + + // Third expansion: 90 lines total (cumulative) + const exp3 = calculateUpwardExpansion(oldStart, 90); + expect(exp3.startLine).toBe(10); + expect(exp3.endLine).toBe(99); + expect(exp3.numLines).toBe(90); + }); + + it("stops at line 1 (beginning of file)", () => { + const oldStart = 20; + const exp = calculateUpwardExpansion(oldStart, 100); + + expect(exp.startLine).toBe(1); // Can't go below 1 + expect(exp.endLine).toBe(19); + expect(exp.numLines).toBe(19); // Less than requested 100 + }); + + it("returns zero lines when hunk is at line 1", () => { + const oldStart = 1; + const exp = calculateUpwardExpansion(oldStart, 30); + + expect(exp.startLine).toBe(1); + expect(exp.endLine).toBe(0); // endLine < startLine + expect(exp.numLines).toBe(0); // No lines before hunk + }); + + it("handles expansion equal to available lines", () => { + const oldStart = 31; + const exp = calculateUpwardExpansion(oldStart, 30); + + expect(exp.startLine).toBe(1); + expect(exp.endLine).toBe(30); + expect(exp.numLines).toBe(30); + }); +}); + +describe("calculateDownwardExpansion", () => { + it("returns cumulative range from hunk end downward", () => { + const oldStart = 100; + const oldLines = 10; // Hunk is lines 100-109 + + // First expansion: 30 lines + const exp1 = calculateDownwardExpansion(oldStart, oldLines, 30); + expect(exp1.startLine).toBe(110); // Right after hunk + expect(exp1.endLine).toBe(139); + expect(exp1.numLines).toBe(30); + + // Second expansion: 60 lines total (cumulative) + const exp2 = calculateDownwardExpansion(oldStart, oldLines, 60); + expect(exp2.startLine).toBe(110); // Same startLine - always after hunk + expect(exp2.endLine).toBe(169); + expect(exp2.numLines).toBe(60); + + // Third expansion: 90 lines total (cumulative) + const exp3 = calculateDownwardExpansion(oldStart, oldLines, 90); + expect(exp3.startLine).toBe(110); + expect(exp3.endLine).toBe(199); + expect(exp3.numLines).toBe(90); + }); + + it("handles single-line hunks", () => { + const oldStart = 50; + const oldLines = 1; + + const exp = calculateDownwardExpansion(oldStart, oldLines, 30); + expect(exp.startLine).toBe(51); + expect(exp.endLine).toBe(80); + expect(exp.numLines).toBe(30); + }); + + it("handles large hunks", () => { + const oldStart = 100; + const oldLines = 50; // Hunk is lines 100-149 + + const exp = calculateDownwardExpansion(oldStart, oldLines, 30); + expect(exp.startLine).toBe(150); + expect(exp.endLine).toBe(179); + expect(exp.numLines).toBe(30); + }); +}); + +describe("collapse behavior", () => { + it("collapses upward expansion correctly", () => { + const oldStart = 100; + + // Expanded to 60 lines + const exp1 = calculateUpwardExpansion(oldStart, 60); + expect(exp1.numLines).toBe(60); + expect(exp1.startLine).toBe(40); + expect(exp1.endLine).toBe(99); + + // Collapse back to 30 lines + const exp2 = calculateUpwardExpansion(oldStart, 30); + expect(exp2.numLines).toBe(30); + expect(exp2.startLine).toBe(70); + expect(exp2.endLine).toBe(99); // Same boundary + + // Collapse to zero (hide all) + const exp3 = calculateUpwardExpansion(oldStart, 0); + expect(exp3.numLines).toBe(0); + }); + + it("collapses downward expansion correctly", () => { + const oldStart = 100; + const oldLines = 10; + + // Expanded to 60 lines + const exp1 = calculateDownwardExpansion(oldStart, oldLines, 60); + expect(exp1.numLines).toBe(60); + expect(exp1.startLine).toBe(110); + expect(exp1.endLine).toBe(169); + + // Collapse back to 30 lines + const exp2 = calculateDownwardExpansion(oldStart, oldLines, 30); + expect(exp2.numLines).toBe(30); + expect(exp2.startLine).toBe(110); // Same boundary + expect(exp2.endLine).toBe(139); + + // Collapse to zero (hide all) + const exp3 = calculateDownwardExpansion(oldStart, oldLines, 0); + expect(exp3.numLines).toBe(0); + }); + + it("handles partial collapse at file boundary", () => { + const oldStart = 20; + + // Expanded to 30 but only 19 lines available + const exp1 = calculateUpwardExpansion(oldStart, 30); + expect(exp1.numLines).toBe(19); + + // Simulate collapse by 30 → should go to 0, not negative + const newExpansion = Math.max(0, 30 - 30); + const exp2 = calculateUpwardExpansion(oldStart, newExpansion); + expect(exp2.numLines).toBe(0); + }); +}); diff --git a/src/utils/review/readFileLines.ts b/src/utils/review/readFileLines.ts new file mode 100644 index 000000000..61ef2d778 --- /dev/null +++ b/src/utils/review/readFileLines.ts @@ -0,0 +1,135 @@ +/** + * Utility functions for reading file lines using sed + * Used by the read-more feature in code review + */ + +/** Number of lines to expand per click (used by HunkViewer component) */ +export const LINES_PER_EXPANSION = 30; + +/** + * Determine which git ref to use for reading file context based on diffBase + * @param diffBase - The diff base from review filters + * @param includeUncommitted - Whether uncommitted changes are included + * @returns Git ref to read old file from (empty string for working tree) + */ +export function getOldFileRef(diffBase: string, includeUncommitted: boolean): string { + // For staged changes, old version is HEAD + if (diffBase === "--staged") { + return "HEAD"; + } + + // For uncommitted-only diffs, old version is HEAD + if (diffBase === "HEAD") { + return "HEAD"; + } + + // For branch diffs with uncommitted, we use merge-base as the old version + if (includeUncommitted) { + // Note: This would need to be computed dynamically via git merge-base + // For simplicity, we'll use the diffBase ref itself + return diffBase; + } + + // For branch diffs without uncommitted (three-dot), old is at merge-base + // But since three-dot shows merge-base..HEAD, we use HEAD as the old version + return diffBase; +} + +/** + * Read lines from a file at a specific git ref using sed + * @param workspaceId - The workspace ID + * @param filePath - Path to the file relative to workspace root + * @param startLine - Starting line number (1-indexed) + * @param endLine - Ending line number (inclusive) + * @param gitRef - Git reference to read from (e.g., "HEAD", "origin/main", or empty string for working tree) + * @returns Array of lines or null if error + */ +export async function readFileLines( + workspaceId: string, + filePath: string, + startLine: number, + endLine: number, + gitRef: string +): Promise { + // Ensure valid line range + if (startLine < 1 || endLine < startLine) { + return null; + } + + // Build command: either read from git ref or working tree + const script = gitRef + ? // Read from git object database and pipe to sed for line range + `git show "${gitRef}:${filePath.replace(/"/g, '\\"')}" | sed -n '${startLine},${endLine}p'` + : // Read from working tree file + `sed -n '${startLine},${endLine}p' "${filePath.replace(/"/g, '\\"')}"`; + + const result = await window.api.workspace.executeBash(workspaceId, script, { + timeout_secs: 3, + }); + + if (!result.success) { + console.error("Failed to read file lines:", result.error); + return null; + } + + // When success is true, output is always a string (type narrowing) + const bashResult = result.data; + if (!bashResult.output) { + console.error("No output from bash command"); + return null; + } + + // Split output into lines + const lines = bashResult.output.split("\n"); + // Remove trailing empty line if present + if (lines.length > 0 && lines[lines.length - 1] === "") { + lines.pop(); + } + + return lines; +} + +/** + * Calculate line range for expanding context upward + * @param oldStart - Starting line number of the hunk in the old file + * @param currentExpansion - Total number of lines to show above hunk (cumulative) + * @returns Object with startLine and endLine for the expansion + */ +export function calculateUpwardExpansion( + oldStart: number, + currentExpansion: number +): { startLine: number; endLine: number; numLines: number } { + // currentExpansion is the total lines to show, not the delta + const startLine = Math.max(1, oldStart - currentExpansion); + const endLine = oldStart - 1; // Always end right before hunk starts + const numLines = Math.max(0, endLine - startLine + 1); + + return { startLine, endLine, numLines }; +} + +/** + * Calculate line range for expanding context downward + * @param oldStart - Starting line number of the hunk in the old file + * @param oldLines - Number of lines in the hunk + * @param currentExpansion - Total number of lines to show below hunk (cumulative) + * @returns Object with startLine and endLine for the expansion + */ +export function calculateDownwardExpansion( + oldStart: number, + oldLines: number, + currentExpansion: number +): { startLine: number; endLine: number; numLines: number } { + const hunkEnd = oldStart + oldLines - 1; + const startLine = hunkEnd + 1; // Always start right after hunk ends + const endLine = hunkEnd + currentExpansion; // Extend by currentExpansion lines + const numLines = Math.max(0, endLine - startLine + 1); + + return { startLine, endLine, numLines }; +} + +/** + * Format expanded lines as diff context lines (prefix with space) + */ +export function formatAsContextLines(lines: string[]): string { + return lines.map((line) => ` ${line}`).join("\n"); +}