diff --git a/src/browser/components/ReviewsBanner.tsx b/src/browser/components/ReviewsBanner.tsx index 71ae9f0c74..2df6347c8b 100644 --- a/src/browser/components/ReviewsBanner.tsx +++ b/src/browser/components/ReviewsBanner.tsx @@ -137,17 +137,20 @@ const ReviewItem: React.FC = ({ [handleSaveEdit, handleCancelEdit] ); - // Format code for diff display - add diff markers if not present + // Prefer selectedDiff (raw diff) when available so reviewers see syntax highlighting consistently. const diffContent = useMemo(() => { + if (review.data.selectedDiff) { + return review.data.selectedDiff; + } + + // Legacy: selectedCode may be plain code or diff-ish text. const lines = review.data.selectedCode.split("\n"); - // Check if lines already have diff markers const hasDiffMarkers = lines.some((l) => /^[+-\s]/.test(l)); if (hasDiffMarkers) { return review.data.selectedCode; } - // Add context markers return lines.map((l) => ` ${l}`).join("\n"); - }, [review.data.selectedCode]); + }, [review.data.selectedCode, review.data.selectedDiff]); const age = formatRelativeTime(review.createdAt); @@ -245,7 +248,13 @@ const ReviewItem: React.FC = ({
{/* Code diff */}
- +
{/* Comment section */} diff --git a/src/browser/components/shared/DiffRenderer.tsx b/src/browser/components/shared/DiffRenderer.tsx index 4b0f0b34cd..3b02316c15 100644 --- a/src/browser/components/shared/DiffRenderer.tsx +++ b/src/browser/components/shared/DiffRenderer.tsx @@ -7,6 +7,7 @@ import React, { useEffect, useState } from "react"; import { cn } from "@/common/lib/utils"; import { getLanguageFromPath } from "@/common/utils/git/languageDetector"; +import { MessageSquare } from "lucide-react"; import { Tooltip, TooltipTrigger, TooltipContent } from "../ui/tooltip"; import { groupDiffLines } from "@/browser/utils/highlighting/diffChunking"; import { useTheme, type ThemeMode } from "@/browser/contexts/ThemeContext"; @@ -23,66 +24,171 @@ import type { ReviewNoteData } from "@/common/types/review"; // Shared type for diff line types export type DiffLineType = "add" | "remove" | "context" | "header"; -// Helper function for getting diff line background color. -// Keep backgrounds subtle so the diff reads like a code block, not a highlighter. +interface DiffLineStyles { + tintBase: string | null; + codeTintTransparentPct: number; + gutterTintTransparentPct: number; + contentColor: string; +} + +const tint = (base: string, transparentPct: number): string => + `color-mix(in srgb, ${base}, transparent ${transparentPct}%)`; + +const DIFF_LINE_STYLES: Record = { + add: { + tintBase: "var(--color-success)", + codeTintTransparentPct: 94, + gutterTintTransparentPct: 86, + contentColor: "var(--color-text)", + }, + remove: { + tintBase: "var(--color-danger)", + codeTintTransparentPct: 94, + gutterTintTransparentPct: 86, + contentColor: "var(--color-text)", + }, + header: { + tintBase: "var(--color-accent)", + codeTintTransparentPct: 95, + gutterTintTransparentPct: 90, + contentColor: "var(--color-accent-light)", + }, + context: { + tintBase: null, + codeTintTransparentPct: 100, + gutterTintTransparentPct: 100, + contentColor: "var(--color-text-secondary)", + }, +}; + +// Helper function for the diff *code* background. This should stay relatively subtle. const getDiffLineBackground = (type: DiffLineType): string => { - switch (type) { - case "add": - return "color-mix(in srgb, var(--color-success), transparent 90%)"; - case "remove": - return "color-mix(in srgb, var(--color-danger), transparent 90%)"; - case "header": - return "color-mix(in srgb, var(--color-accent), transparent 92%)"; - case "context": - default: - return "transparent"; - } + const style = DIFF_LINE_STYLES[type]; + return style.tintBase ? tint(style.tintBase, style.codeTintTransparentPct) : "transparent"; }; -const getDiffLineBorderColor = (type: DiffLineType): string => { - switch (type) { - case "add": - return "color-mix(in srgb, var(--color-success), transparent 35%)"; - case "remove": - return "color-mix(in srgb, var(--color-danger), transparent 35%)"; - case "header": - return "color-mix(in srgb, var(--color-accent), transparent 45%)"; - case "context": - default: - return "transparent"; - } +// Helper function for the diff *gutter* background (line numbers / +/- area). +// This is intentionally more saturated than the code background for contrast. +// Context lines have no special background (same as code area). +const getDiffLineGutterBackground = (type: DiffLineType): string => { + const style = DIFF_LINE_STYLES[type]; + return style.tintBase ? tint(style.tintBase, style.gutterTintTransparentPct) : "transparent"; }; // Helper function for getting line content color. // Only headers/context are tinted; actual code stays the normal foreground color. -const getLineContentColor = (type: DiffLineType): string => { +const getLineContentColor = (type: DiffLineType): string => DIFF_LINE_STYLES[type].contentColor; + +// Split diff into lines while preserving indices. +// We only remove the trailing empty line if the input ends with a newline. +const splitDiffLines = (diff: string): string[] => { + const lines = diff.split("\n"); + if (lines.length > 0 && lines[lines.length - 1] === "") { + lines.pop(); + } + return lines; +}; + +// Line number color - brighter for changed lines, dimmed for context. +const getLineNumberColor = (type: DiffLineType): string => { + return type === "context" + ? "color-mix(in srgb, var(--color-muted) 40%, transparent)" + : "var(--color-text)"; +}; + +// Indicator (+/-/space) character and color +const getIndicatorChar = (type: DiffLineType): string => { switch (type) { - case "header": - return "var(--color-accent-light)"; - case "context": - return "var(--color-text-secondary)"; case "add": + return "+"; case "remove": + return "−"; // Use proper minus sign for aesthetics default: - return "var(--color-text)"; + return " "; } }; -// Used for the +/- marker and line numbers. -const getContrastColor = (type: DiffLineType): string => { +const getIndicatorColor = (type: DiffLineType): string => { switch (type) { case "add": - return "var(--color-success-light)"; + return "var(--color-success)"; case "remove": - return "var(--color-danger-soft)"; - case "header": - return "var(--color-accent-light)"; - case "context": + return "var(--color-danger)"; default: - return "var(--color-text-secondary)"; + return "transparent"; } }; +// Shared line number widths interface +interface LineNumberWidths { + oldWidthCh: number; + newWidthCh: number; +} + +// Shared line gutter component (line numbers) +interface DiffLineGutterProps { + type: DiffLineType; + oldLineNum: number | null; + newLineNum: number | null; + showLineNumbers: boolean; + lineNumberWidths: LineNumberWidths; +} + +const DiffLineGutter: React.FC = ({ + type, + oldLineNum, + newLineNum, + showLineNumbers, + lineNumberWidths, +}) => ( + + {showLineNumbers && ( + <> + + {oldLineNum ?? ""} + + + {newLineNum ?? ""} + + + )} + +); + +// Shared indicator component (+/- with optional hover replacement) +interface DiffIndicatorProps { + type: DiffLineType; + /** Render review button overlay on hover */ + reviewButton?: React.ReactNode; +} + +const DiffIndicator: React.FC = ({ type, reviewButton }) => ( + + + {getIndicatorChar(type)} + + {reviewButton} + +); + /** * Container component for diff rendering - exported for custom diff displays * Used by FileEditToolCall for wrapping custom diff content @@ -214,8 +320,8 @@ function useHighlightedDiff( let cancelled = false; async function highlight() { - // Split into lines - const lines = content.split("\n").filter((line) => line.length > 0); + // Split into lines (preserve indices for selection + rendering) + const lines = splitDiffLines(content); // Group into chunks const diffChunks = groupDiffLines(lines, oldStart, newStart); @@ -272,6 +378,31 @@ export const DiffRenderer: React.FC = ({ const highlightedChunks = useHighlightedDiff(content, language, oldStart, newStart, theme); + const lineNumberWidths = React.useMemo(() => { + if (!showLineNumbers || !highlightedChunks) { + return { oldWidthCh: 2, newWidthCh: 2 }; + } + + let oldWidthCh = 0; + let newWidthCh = 0; + + for (const chunk of highlightedChunks) { + for (const line of chunk.lines) { + if (line.oldLineNumber !== null) { + oldWidthCh = Math.max(oldWidthCh, String(line.oldLineNumber).length); + } + if (line.newLineNumber !== null) { + newWidthCh = Math.max(newWidthCh, String(line.newLineNumber).length); + } + } + } + + return { + oldWidthCh: Math.max(2, oldWidthCh), + newWidthCh: Math.max(2, newWidthCh), + }; + }, [highlightedChunks, showLineNumbers]); + // Show loading state while highlighting if (!highlightedChunks) { return ( @@ -285,47 +416,25 @@ export const DiffRenderer: React.FC = ({ {highlightedChunks.flatMap((chunk) => chunk.lines.map((line) => { - const indicator = chunk.type === "add" ? "+" : chunk.type === "remove" ? "-" : " "; return ( -
-
+ + - - {indicator} - - {showLineNumbers && ( - - {line.lineNumber} - - )} - - - -
+ + +
); }) @@ -351,8 +460,6 @@ interface SelectableDiffRendererProps extends Omit; - lines: string[]; // Original diff lines with +/- prefix + lineData: Array<{ + index: number; + type: DiffLineType; + oldLineNum: number | null; + newLineNum: number | null; + raw: string; // Original line with +/- prefix + }>; filePath: string; + showLineNumbers: boolean; + lineNumberWidths: { oldWidthCh: number; newWidthCh: number }; onSubmit: (data: ReviewNoteData) => void; onCancel: () => void; } const ReviewNoteInput: React.FC = React.memo( - ({ selection, lineData, lines, filePath, onSubmit, onCancel }) => { + ({ selection, lineData, filePath, showLineNumbers, lineNumberWidths, onSubmit, onCancel }) => { const [noteText, setNoteText] = React.useState(""); const textareaRef = React.useRef(null); @@ -390,17 +504,40 @@ const ReviewNoteInput: React.FC = React.memo( const handleSubmit = () => { if (!noteText.trim()) return; - const lineRange = - selection.startLineNum === selection.endLineNum - ? `${selection.startLineNum}` - : `${selection.startLineNum}-${selection.endLineNum}`; - const [start, end] = [selection.startIndex, selection.endIndex].sort((a, b) => a - b); - const allLines = lineData.slice(start, end + 1).map((lineInfo) => { - const line = lines[lineInfo.index]; - const indicator = line[0]; // +, -, or space - const content = line.slice(1); // Remove the indicator - return `${lineInfo.lineNum} ${indicator} ${content}`; + const selectedLineData = lineData.slice(start, end + 1); + + const oldLineNumbers = selectedLineData + .map((lineInfo) => lineInfo.oldLineNum) + .filter((lineNum): lineNum is number => lineNum !== null); + const newLineNumbers = selectedLineData + .map((lineInfo) => lineInfo.newLineNum) + .filter((lineNum): lineNum is number => lineNum !== null); + + const formatRange = (nums: number[]) => { + if (nums.length === 0) return null; + const min = Math.min(...nums); + const max = Math.max(...nums); + return min === max ? `${min}` : `${min}-${max}`; + }; + + const oldRange = formatRange(oldLineNumbers); + const newRange = formatRange(newLineNumbers); + const lineRange = [oldRange ? `-${oldRange}` : null, newRange ? `+${newRange}` : null] + .filter((part): part is string => Boolean(part)) + .join(" "); + + const oldWidth = Math.max(1, ...oldLineNumbers.map((n) => String(n).length)); + const newWidth = Math.max(1, ...newLineNumbers.map((n) => String(n).length)); + + const allLines = selectedLineData.map((lineInfo) => { + const indicator = lineInfo.raw[0] ?? " "; // +, -, or space + const content = lineInfo.raw.slice(1); // Remove the indicator + + const oldStr = lineInfo.oldLineNum === null ? "" : String(lineInfo.oldLineNum); + const newStr = lineInfo.newLineNum === null ? "" : String(lineInfo.newLineNum); + + return `${oldStr.padStart(oldWidth)} ${newStr.padStart(newWidth)} ${indicator} ${content}`; }); // Elide middle lines if more than 20 lines selected (show 10 at start, 10 at end) @@ -418,44 +555,86 @@ const ReviewNoteInput: React.FC = React.memo( ].join("\n"); } + const selectedDiff = selectedLineData.map((lineInfo) => lineInfo.raw).join("\n"); + const oldStart = oldLineNumbers.length ? Math.min(...oldLineNumbers) : 1; + const newStart = newLineNumbers.length ? Math.min(...newLineNumbers) : 1; + // Pass structured data instead of formatted message onSubmit({ filePath, lineRange, selectedCode, + selectedDiff, + oldStart, + newStart, userNote: noteText.trim(), }); }; + // Determine the predominant line type for background matching + const [start, end] = [selection.startIndex, selection.endIndex].sort((a, b) => a - b); + const selectedTypes = lineData.slice(start, end + 1).map((l) => l.type); + // Use the last selected line's type (where the input appears) + const lineType = selectedTypes[selectedTypes.length - 1] ?? "context"; + return ( -
-