diff --git a/src/components/RightSidebar/CodeReview/ReviewPanel.tsx b/src/components/RightSidebar/CodeReview/ReviewPanel.tsx index 6c255ebc9..d2cac082e 100644 --- a/src/components/RightSidebar/CodeReview/ReviewPanel.tsx +++ b/src/components/RightSidebar/CodeReview/ReviewPanel.tsx @@ -239,32 +239,35 @@ interface DiagnosticInfo { /** * Build git diff command based on diffBase and includeUncommitted flag * Shared logic between numstat (file tree) and diff (hunks) commands + * Exported for testing + * + * @param diffBase - Base reference ("main", "HEAD", "--staged") + * @param includeUncommitted - Include uncommitted working directory changes + * @param pathFilter - Optional path filter (e.g., ' -- "src/foo.ts"') + * @param command - "diff" (unified) or "numstat" (file stats) */ -function buildGitDiffCommand( +export function buildGitDiffCommand( diffBase: string, includeUncommitted: boolean, pathFilter: string, command: "diff" | "numstat" ): string { - const isNumstat = command === "numstat"; - const flag = isNumstat ? " -M --numstat" : " -M"; - - let cmd: string; + const flags = command === "numstat" ? " -M --numstat" : " -M"; if (diffBase === "--staged") { - cmd = `git diff --staged${flag}${pathFilter}`; - } else if (diffBase === "HEAD") { - cmd = `git diff HEAD${flag}${pathFilter}`; - } else { - cmd = `git diff ${diffBase}...HEAD${flag}${pathFilter}`; + // Staged changes, optionally with unstaged appended as separate diff + const base = `git diff --staged${flags}${pathFilter}`; + return includeUncommitted ? `${base} && git diff HEAD${flags}${pathFilter}` : base; } - // Append uncommitted changes if requested - if (includeUncommitted) { - cmd += ` && git diff HEAD${flag}${pathFilter}`; + if (diffBase === "HEAD") { + // Uncommitted changes only (working vs HEAD) + return `git diff HEAD${flags}${pathFilter}`; } - return cmd; + // Branch diff: two-dot includes uncommitted, three-dot excludes + const range = includeUncommitted ? diffBase : `${diffBase}...HEAD`; + return `git diff ${range}${flags}${pathFilter}`; } export const ReviewPanel: React.FC = ({ diff --git a/src/utils/git/diffParser.test.ts b/src/utils/git/diffParser.test.ts index 570fe95fc..f7e2c93fb 100644 --- a/src/utils/git/diffParser.test.ts +++ b/src/utils/git/diffParser.test.ts @@ -10,6 +10,7 @@ import { join } from "path"; import { tmpdir } from "os"; import { execSync } from "child_process"; import { parseDiff, extractAllHunks } from "./diffParser"; +import { buildGitDiffCommand } from "@/components/RightSidebar/CodeReview/ReviewPanel"; describe("git diff parser (real repository)", () => { let testRepoPath: string; @@ -366,4 +367,105 @@ function greet(name) { // Pure directory renames should have NO hunks (files are identical) expect(allHunks.length).toBe(0); }); + + it("should show unified diff when includeUncommitted is true", () => { + // Verify includeUncommitted produces single unified diff (not separate committed + uncommitted) + execSync("git reset --hard HEAD", { cwd: testRepoPath }); + + const baseBranch = execSync("git rev-parse --abbrev-ref HEAD", { + cwd: testRepoPath, + encoding: "utf-8", + }).trim(); + + execSync("git checkout -b unified-test", { cwd: testRepoPath }); + + // Commit a change, then make uncommitted change + writeFileSync(join(testRepoPath, "test-file.txt"), "Line 1\nLine 2\nLine 3\n"); + execSync("git add test-file.txt && git commit -m 'Add test file'", { cwd: testRepoPath }); + writeFileSync(join(testRepoPath, "test-file.txt"), "Line 1\nLine 2\nLine 3 modified\nLine 4\n"); + + const gitCommand = buildGitDiffCommand(baseBranch, true, "", "diff"); + const diffOutput = execSync(gitCommand, { cwd: testRepoPath, encoding: "utf-8" }); + const fileDiffs = parseDiff(diffOutput); + + // Single FileDiff with unified changes (no duplicates) + expect(fileDiffs.length).toBe(1); + expect(fileDiffs[0].filePath).toBe("test-file.txt"); + + const allHunks = extractAllHunks(fileDiffs); + const allContent = allHunks.map((h) => h.content).join("\n"); + expect(allContent.includes("Line 3 modified") || allContent.includes("Line 4")).toBe(true); + + execSync("git reset --hard HEAD && git checkout -", { cwd: testRepoPath }); + }); + + it("should exclude uncommitted changes when includeUncommitted is false", () => { + // Verify includeUncommitted=false uses three-dot (committed only) + execSync("git reset --hard HEAD", { cwd: testRepoPath }); + + const baseBranch = execSync("git rev-parse --abbrev-ref HEAD", { + cwd: testRepoPath, + encoding: "utf-8", + }).trim(); + + execSync("git checkout -b committed-only-test", { cwd: testRepoPath }); + + // Commit a change + writeFileSync(join(testRepoPath, "committed-file.txt"), "Line 1\nLine 2\n"); + execSync("git add committed-file.txt && git commit -m 'Add committed file'", { + cwd: testRepoPath, + }); + + // Make uncommitted change (should NOT appear in diff) + writeFileSync(join(testRepoPath, "committed-file.txt"), "Line 1\nLine 2\nUncommitted line\n"); + + const gitCommand = buildGitDiffCommand(baseBranch, false, "", "diff"); + const diffOutput = execSync(gitCommand, { cwd: testRepoPath, encoding: "utf-8" }); + const fileDiffs = parseDiff(diffOutput); + + // Should get FileDiff showing only committed changes + expect(fileDiffs.length).toBe(1); + expect(fileDiffs[0].filePath).toBe("committed-file.txt"); + + const allHunks = extractAllHunks(fileDiffs); + const allContent = allHunks.map((h) => h.content).join("\n"); + + // Should NOT include uncommitted "Uncommitted line" + expect(allContent.includes("Uncommitted line")).toBe(false); + // Should include committed content + expect(allContent.includes("Line 1") || allContent.includes("Line 2")).toBe(true); + + execSync("git reset --hard HEAD && git checkout -", { cwd: testRepoPath }); + }); + + it("should handle staged + uncommitted when diffBase is --staged", () => { + // Verify --staged with includeUncommitted produces TWO diffs (staged + unstaged) + execSync("git reset --hard HEAD", { cwd: testRepoPath }); + + writeFileSync(join(testRepoPath, "staged-test.txt"), "Line 1\nLine 2\nLine 3\n"); + execSync("git add staged-test.txt", { cwd: testRepoPath }); + + writeFileSync(join(testRepoPath, "staged-test.txt"), "Line 1\nLine 2 staged\nLine 3\n"); + execSync("git add staged-test.txt", { cwd: testRepoPath }); + + writeFileSync( + join(testRepoPath, "staged-test.txt"), + "Line 1\nLine 2 staged\nLine 3 unstaged\n" + ); + + const gitCommand = buildGitDiffCommand("--staged", true, "", "diff"); + const diffOutput = execSync(gitCommand, { cwd: testRepoPath, encoding: "utf-8" }); + const fileDiffs = parseDiff(diffOutput); + + // Two FileDiff entries (staged + unstaged) + expect(fileDiffs.length).toBe(2); + expect(fileDiffs.every((f) => f.filePath === "staged-test.txt")).toBe(true); + + const allHunks = extractAllHunks(fileDiffs); + expect(allHunks.length).toBe(2); + + const allContent = allHunks.map((h) => h.content).join("\n"); + expect(allContent.includes("staged")).toBe(true); + expect(allContent.includes("unstaged")).toBe(true); + }); });