From c2be1960b942e6b2fc8d93c148e44d66d8a8a7c9 Mon Sep 17 00:00:00 2001
From: Ammar <ammar+ai@ammar.io>
Date: Sat, 18 Oct 2025 22:08:35 -0500
Subject: [PATCH 1/6] =?UTF-8?q?=F0=9F=A4=96=20Fix=20uncommitted=20changes?=
 =?UTF-8?q?=20not=20showing=20with=20'Dirty'=20checkbox?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

When 'Dirty' checkbox is enabled, uncommitted filesystem changes
were not appearing in the code review panel. Root cause: combining
git commands with && (e.g., 'git diff base...HEAD && git diff HEAD')
produces duplicate FileDiff entries for the same file.

The parser was creating separate FileDiff objects for each, meaning:
- File tree showed files twice (or one overwrote the other)
- Hunks from both diffs contributed to the list
- But file filtering only matched hunks from the FIRST entry

Solution: Deduplicate FileDiff entries by file path after parsing,
merging hunks from duplicate entries into a single canonical entry.

This makes the parser more robust and ensures all hunks (committed
and uncommitted) appear when filtering by file.

Test approach (TDD):
1. Added failing test demonstrating duplicate FileDiff bug
2. Implemented deduplication logic in parseDiff()
3. Verified all tests pass (12 diffParser tests, 628 total tests)

_Generated with `cmux`_
---
 .../RightSidebar/CodeReview/ReviewPanel.tsx   |  3 +-
 src/utils/git/diffParser.test.ts              | 61 +++++++++++++++++++
 src/utils/git/diffParser.ts                   | 16 ++++-
 3 files changed, 78 insertions(+), 2 deletions(-)

diff --git a/src/components/RightSidebar/CodeReview/ReviewPanel.tsx b/src/components/RightSidebar/CodeReview/ReviewPanel.tsx
index 6c255ebc9..012f432f8 100644
--- a/src/components/RightSidebar/CodeReview/ReviewPanel.tsx
+++ b/src/components/RightSidebar/CodeReview/ReviewPanel.tsx
@@ -239,8 +239,9 @@ 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
  */
-function buildGitDiffCommand(
+export function buildGitDiffCommand(
   diffBase: string,
   includeUncommitted: boolean,
   pathFilter: string,
diff --git a/src/utils/git/diffParser.test.ts b/src/utils/git/diffParser.test.ts
index 570fe95fc..50ca251db 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,64 @@ function greet(name) {
     // Pure directory renames should have NO hunks (files are identical)
     expect(allHunks.length).toBe(0);
   });
+
+  it("should deduplicate files when combining committed and uncommitted diffs", () => {
+    // This test replicates the bug: when using "includeDirty" flag,
+    // we run: git diff base...HEAD && git diff HEAD
+    // which produces TWO separate diffs for the same file
+
+    // Reset and setup: create a branch with committed changes
+    execSync("git reset --hard HEAD", { cwd: testRepoPath });
+    execSync("git checkout -b dedup-test", { cwd: testRepoPath });
+
+    // Make and commit a 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 });
+
+    // Now make an uncommitted change
+    writeFileSync(join(testRepoPath, "test-file.txt"), "Line 1\nLine 2 modified\nLine 3\nLine 4\n");
+
+    // Simulate what ReviewPanel does: use buildGitDiffCommand with includeUncommitted=true
+    const baseBranch = execSync("git rev-parse --abbrev-ref HEAD@{u} 2>/dev/null || echo main", {
+      cwd: testRepoPath,
+      encoding: "utf-8",
+    }).trim();
+
+    // Use the same command builder as ReviewPanel (tests the actual integration)
+    const gitCommand = buildGitDiffCommand(baseBranch, true, "", "diff");
+
+    // Execute the command and get the combined output
+    const combinedDiff = execSync(gitCommand, {
+      cwd: testRepoPath,
+      encoding: "utf-8",
+    });
+
+    // Parse the combined diff
+    const fileDiffs = parseDiff(combinedDiff);
+
+    // BUG: Without deduplication, we get 2 FileDiff entries for the same file
+    // FIX: After deduplication, we should get 1 FileDiff with hunks from both diffs
+
+    // This should FAIL before the fix (fileDiffs.length === 2)
+    // This should PASS after the fix (fileDiffs.length === 1)
+    expect(fileDiffs.length).toBe(1);
+    expect(fileDiffs[0].filePath).toBe("test-file.txt");
+
+    // Should have hunks from BOTH diffs (committed + uncommitted)
+    const allHunks = extractAllHunks(fileDiffs);
+    expect(allHunks.length).toBeGreaterThan(0);
+
+    // All hunks should reference the same file
+    expect(allHunks.every((h) => h.filePath === "test-file.txt")).toBe(true);
+
+    // The hunks should include both the committed and uncommitted changes
+    // - Committed: "Line 1\nLine 2\nLine 3\n" (initial commit)
+    // - Uncommitted: "Line 2 modified\nLine 4\n" (working directory)
+    const allContent = allHunks.map((h) => h.content).join("\n");
+    expect(allContent.includes("Line 2 modified") || allContent.includes("Line 4")).toBe(true);
+
+    // Clean up: reset and go back to previous branch
+    execSync("git reset --hard HEAD && git checkout -", { cwd: testRepoPath });
+  });
+
 });
diff --git a/src/utils/git/diffParser.ts b/src/utils/git/diffParser.ts
index 510771ee7..790914000 100644
--- a/src/utils/git/diffParser.ts
+++ b/src/utils/git/diffParser.ts
@@ -164,7 +164,21 @@ export function parseDiff(diffOutput: string): FileDiff[] {
   // Finish last file
   finishFile();
 
-  return files;
+  // Deduplicate files by path - merge hunks from duplicate entries
+  // This happens when combining multiple git diff commands (e.g., base...HEAD && HEAD)
+  // which produces separate FileDiff entries for the same file
+  const fileMap = new Map<string, FileDiff>();
+  for (const file of files) {
+    const existing = fileMap.get(file.filePath);
+    if (existing) {
+      // Merge hunks from duplicate entry into the first one
+      existing.hunks.push(...file.hunks);
+    } else {
+      fileMap.set(file.filePath, file);
+    }
+  }
+
+  return Array.from(fileMap.values());
 }
 
 /**

From 3ad6230bbdb681896d9f4dd2ddadea0f09c202a2 Mon Sep 17 00:00:00 2001
From: Ammar <ammar+ai@ammar.io>
Date: Sat, 18 Oct 2025 22:17:15 -0500
Subject: [PATCH 2/6] =?UTF-8?q?=F0=9F=A4=96=20Fix=20command=20generation?=
 =?UTF-8?q?=20for=20uncommitted=20changes=20(cleaner=20approach)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Instead of deduplicating FileDiff entries after parsing, fix the root
cause by generating better git commands.

**Problem with old approach:**
- Used `git diff base...HEAD && git diff HEAD` when includeUncommitted=true
- Created TWO separate diffs for same file with different base hashes
- Resulted in confusing UX (separate hunks with different contexts)

**New approach:**
- When includeUncommitted=true for branch diffs: use TWO-DOT diff
  (`git diff base`) to show unified changes from base to working directory
- For --staged: keep && behavior (staged + unstaged shown separately)
- For HEAD: no change needed (already shows uncommitted)

**Result:**
- Single unified diff showing all changes cleanly
- No duplicate FileDiff entries
- Better UX - reviewers see cohesive changes, not fragmented hunks
- Net -5 LoC (simpler codebase)

**Testing:**
- Updated test to verify single FileDiff with unified changes
- Added test for --staged edge case (should still use &&)
- All 629 tests pass ✅
- Types check ✅

_Generated with `cmux`_
---
 .../RightSidebar/CodeReview/ReviewPanel.tsx   | 19 ++--
 src/utils/git/diffParser.test.ts              | 87 ++++++++++++++-----
 src/utils/git/diffParser.ts                   | 16 +---
 3 files changed, 78 insertions(+), 44 deletions(-)

diff --git a/src/components/RightSidebar/CodeReview/ReviewPanel.tsx b/src/components/RightSidebar/CodeReview/ReviewPanel.tsx
index 012f432f8..ac897be00 100644
--- a/src/components/RightSidebar/CodeReview/ReviewPanel.tsx
+++ b/src/components/RightSidebar/CodeReview/ReviewPanel.tsx
@@ -253,16 +253,23 @@ export function buildGitDiffCommand(
   let cmd: string;
 
   if (diffBase === "--staged") {
+    // Staged: show staged changes, optionally append unstaged
     cmd = `git diff --staged${flag}${pathFilter}`;
+    if (includeUncommitted) {
+      cmd += ` && git diff HEAD${flag}${pathFilter}`;
+    }
   } else if (diffBase === "HEAD") {
+    // HEAD: already shows uncommitted changes (working directory vs HEAD)
     cmd = `git diff HEAD${flag}${pathFilter}`;
   } else {
-    cmd = `git diff ${diffBase}...HEAD${flag}${pathFilter}`;
-  }
-
-  // Append uncommitted changes if requested
-  if (includeUncommitted) {
-    cmd += ` && git diff HEAD${flag}${pathFilter}`;
+    // Branch diff: use three-dot for committed only, two-dot for all changes
+    if (includeUncommitted) {
+      // Two-dot: shows all changes from base to working directory (unified)
+      cmd = `git diff ${diffBase}${flag}${pathFilter}`;
+    } else {
+      // Three-dot: shows only committed changes (base...HEAD)
+      cmd = `git diff ${diffBase}...HEAD${flag}${pathFilter}`;
+    }
   }
 
   return cmd;
diff --git a/src/utils/git/diffParser.test.ts b/src/utils/git/diffParser.test.ts
index 50ca251db..2d768013f 100644
--- a/src/utils/git/diffParser.test.ts
+++ b/src/utils/git/diffParser.test.ts
@@ -368,63 +368,104 @@ function greet(name) {
     expect(allHunks.length).toBe(0);
   });
 
-  it("should deduplicate files when combining committed and uncommitted diffs", () => {
-    // This test replicates the bug: when using "includeDirty" flag,
-    // we run: git diff base...HEAD && git diff HEAD
-    // which produces TWO separate diffs for the same file
+  it("should show unified diff when includeUncommitted is true", () => {
+    // This test verifies that when using "includeUncommitted" flag,
+    // we get a single unified diff from base to working directory
+    // (not separate diffs for committed and uncommitted)
 
     // Reset and setup: create a branch with committed changes
     execSync("git reset --hard HEAD", { cwd: testRepoPath });
-    execSync("git checkout -b dedup-test", { cwd: testRepoPath });
+    execSync("git checkout -b unified-test", { cwd: testRepoPath });
 
     // Make and commit a 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 });
 
-    // Now make an uncommitted change
-    writeFileSync(join(testRepoPath, "test-file.txt"), "Line 1\nLine 2 modified\nLine 3\nLine 4\n");
+    // Now make an uncommitted change (different line)
+    writeFileSync(join(testRepoPath, "test-file.txt"), "Line 1\nLine 2\nLine 3 modified\nLine 4\n");
 
-    // Simulate what ReviewPanel does: use buildGitDiffCommand with includeUncommitted=true
+    // Get base branch
     const baseBranch = execSync("git rev-parse --abbrev-ref HEAD@{u} 2>/dev/null || echo main", {
       cwd: testRepoPath,
       encoding: "utf-8",
     }).trim();
 
-    // Use the same command builder as ReviewPanel (tests the actual integration)
+    // Use buildGitDiffCommand with includeUncommitted=true
     const gitCommand = buildGitDiffCommand(baseBranch, true, "", "diff");
 
-    // Execute the command and get the combined output
-    const combinedDiff = execSync(gitCommand, {
+    // Execute the command
+    const diffOutput = execSync(gitCommand, {
       cwd: testRepoPath,
       encoding: "utf-8",
     });
 
-    // Parse the combined diff
-    const fileDiffs = parseDiff(combinedDiff);
+    // Parse the diff
+    const fileDiffs = parseDiff(diffOutput);
 
-    // BUG: Without deduplication, we get 2 FileDiff entries for the same file
-    // FIX: After deduplication, we should get 1 FileDiff with hunks from both diffs
-
-    // This should FAIL before the fix (fileDiffs.length === 2)
-    // This should PASS after the fix (fileDiffs.length === 1)
+    // Should get a single FileDiff (no duplicates)
     expect(fileDiffs.length).toBe(1);
     expect(fileDiffs[0].filePath).toBe("test-file.txt");
 
-    // Should have hunks from BOTH diffs (committed + uncommitted)
+    // Should have hunks showing unified changes
     const allHunks = extractAllHunks(fileDiffs);
     expect(allHunks.length).toBeGreaterThan(0);
 
     // All hunks should reference the same file
     expect(allHunks.every((h) => h.filePath === "test-file.txt")).toBe(true);
 
-    // The hunks should include both the committed and uncommitted changes
-    // - Committed: "Line 1\nLine 2\nLine 3\n" (initial commit)
-    // - Uncommitted: "Line 2 modified\nLine 4\n" (working directory)
+    // The diff should include BOTH the committed and uncommitted changes
+    // in a single unified view (not as separate hunks with different base states)
     const allContent = allHunks.map((h) => h.content).join("\n");
-    expect(allContent.includes("Line 2 modified") || allContent.includes("Line 4")).toBe(true);
+    expect(allContent.includes("Line 3 modified") || allContent.includes("Line 4")).toBe(true);
 
     // Clean up: reset and go back to previous branch
     execSync("git reset --hard HEAD && git checkout -", { cwd: testRepoPath });
   });
 
+  it("should handle staged + uncommitted when diffBase is --staged", () => {
+    // When diffBase="--staged" with includeUncommitted=true,
+    // we want TWO separate diffs: staged changes AND unstaged changes
+    // This is the only case where we keep the && behavior
+
+    // Reset
+    execSync("git reset --hard HEAD", { cwd: testRepoPath });
+
+    // Create a file with changes
+    writeFileSync(join(testRepoPath, "staged-test.txt"), "Line 1\nLine 2\nLine 3\n");
+    execSync("git add staged-test.txt", { cwd: testRepoPath });
+
+    // Stage some changes
+    writeFileSync(join(testRepoPath, "staged-test.txt"), "Line 1\nLine 2 staged\nLine 3\n");
+    execSync("git add staged-test.txt", { cwd: testRepoPath });
+
+    // Make additional unstaged changes
+    writeFileSync(join(testRepoPath, "staged-test.txt"), "Line 1\nLine 2 staged\nLine 3 unstaged\n");
+
+    // Build command with --staged and includeUncommitted
+    const gitCommand = buildGitDiffCommand("--staged", true, "", "diff");
+
+    // Execute
+    const diffOutput = execSync(gitCommand, {
+      cwd: testRepoPath,
+      encoding: "utf-8",
+    });
+
+    // Parse
+    const fileDiffs = parseDiff(diffOutput);
+
+    // Should get TWO FileDiff entries for the same file (staged + unstaged)
+    expect(fileDiffs.length).toBe(2);
+    expect(fileDiffs.every((f) => f.filePath === "staged-test.txt")).toBe(true);
+
+    // Should have hunks from both diffs
+    const allHunks = extractAllHunks(fileDiffs);
+    expect(allHunks.length).toBe(2);
+
+    // Content should show both staged and unstaged changes
+    const allContent = allHunks.map((h) => h.content).join("\n");
+    expect(allContent.includes("staged")).toBe(true);
+    expect(allContent.includes("unstaged")).toBe(true);
+  });
+
+
 });
diff --git a/src/utils/git/diffParser.ts b/src/utils/git/diffParser.ts
index 790914000..510771ee7 100644
--- a/src/utils/git/diffParser.ts
+++ b/src/utils/git/diffParser.ts
@@ -164,21 +164,7 @@ export function parseDiff(diffOutput: string): FileDiff[] {
   // Finish last file
   finishFile();
 
-  // Deduplicate files by path - merge hunks from duplicate entries
-  // This happens when combining multiple git diff commands (e.g., base...HEAD && HEAD)
-  // which produces separate FileDiff entries for the same file
-  const fileMap = new Map<string, FileDiff>();
-  for (const file of files) {
-    const existing = fileMap.get(file.filePath);
-    if (existing) {
-      // Merge hunks from duplicate entry into the first one
-      existing.hunks.push(...file.hunks);
-    } else {
-      fileMap.set(file.filePath, file);
-    }
-  }
-
-  return Array.from(fileMap.values());
+  return files;
 }
 
 /**

From bc5418427b690278d6bc027915fa8fe35367c837 Mon Sep 17 00:00:00 2001
From: Ammar <ammar+ai@ammar.io>
Date: Sat, 18 Oct 2025 22:18:28 -0500
Subject: [PATCH 3/6] =?UTF-8?q?=F0=9F=A4=96=20Fix=20test=20to=20work=20wit?=
 =?UTF-8?q?h=20any=20default=20branch=20name?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The test was falling back to hardcoded 'main' which fails on systems
where git init creates 'master' by default.

Fix: Get the current branch name before checking out the new branch,
avoiding reliance on upstream tracking or hardcoded branch names.

_Generated with `cmux`_
---
 src/utils/git/diffParser.test.ts | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/src/utils/git/diffParser.test.ts b/src/utils/git/diffParser.test.ts
index 2d768013f..177647309 100644
--- a/src/utils/git/diffParser.test.ts
+++ b/src/utils/git/diffParser.test.ts
@@ -375,6 +375,13 @@ function greet(name) {
 
     // Reset and setup: create a branch with committed changes
     execSync("git reset --hard HEAD", { cwd: testRepoPath });
+    
+    // Get the current branch name (will be our base branch)
+    const baseBranch = execSync("git rev-parse --abbrev-ref HEAD", {
+      cwd: testRepoPath,
+      encoding: "utf-8",
+    }).trim();
+    
     execSync("git checkout -b unified-test", { cwd: testRepoPath });
 
     // Make and commit a change
@@ -384,12 +391,6 @@ function greet(name) {
     // Now make an uncommitted change (different line)
     writeFileSync(join(testRepoPath, "test-file.txt"), "Line 1\nLine 2\nLine 3 modified\nLine 4\n");
 
-    // Get base branch
-    const baseBranch = execSync("git rev-parse --abbrev-ref HEAD@{u} 2>/dev/null || echo main", {
-      cwd: testRepoPath,
-      encoding: "utf-8",
-    }).trim();
-
     // Use buildGitDiffCommand with includeUncommitted=true
     const gitCommand = buildGitDiffCommand(baseBranch, true, "", "diff");
 

From 996b14e1e34e817b3bc5636dd7c58b8ab6202deb Mon Sep 17 00:00:00 2001
From: Ammar <ammar+ai@ammar.io>
Date: Sat, 18 Oct 2025 22:21:42 -0500
Subject: [PATCH 4/6] =?UTF-8?q?=F0=9F=A4=96=20Run=20prettier=20formatting?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

_Generated with `cmux`_
---
 src/utils/git/diffParser.test.ts | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/src/utils/git/diffParser.test.ts b/src/utils/git/diffParser.test.ts
index 177647309..d221f2a94 100644
--- a/src/utils/git/diffParser.test.ts
+++ b/src/utils/git/diffParser.test.ts
@@ -375,13 +375,13 @@ function greet(name) {
 
     // Reset and setup: create a branch with committed changes
     execSync("git reset --hard HEAD", { cwd: testRepoPath });
-    
+
     // Get the current branch name (will be our base branch)
     const baseBranch = execSync("git rev-parse --abbrev-ref HEAD", {
       cwd: testRepoPath,
       encoding: "utf-8",
     }).trim();
-    
+
     execSync("git checkout -b unified-test", { cwd: testRepoPath });
 
     // Make and commit a change
@@ -440,7 +440,10 @@ function greet(name) {
     execSync("git add staged-test.txt", { cwd: testRepoPath });
 
     // Make additional unstaged changes
-    writeFileSync(join(testRepoPath, "staged-test.txt"), "Line 1\nLine 2 staged\nLine 3 unstaged\n");
+    writeFileSync(
+      join(testRepoPath, "staged-test.txt"),
+      "Line 1\nLine 2 staged\nLine 3 unstaged\n"
+    );
 
     // Build command with --staged and includeUncommitted
     const gitCommand = buildGitDiffCommand("--staged", true, "", "diff");
@@ -467,6 +470,4 @@ function greet(name) {
     expect(allContent.includes("staged")).toBe(true);
     expect(allContent.includes("unstaged")).toBe(true);
   });
-
-
 });

From afe157fb434e573947176569bb2c570321b01d33 Mon Sep 17 00:00:00 2001
From: Ammar <ammar+ai@ammar.io>
Date: Sat, 18 Oct 2025 22:22:54 -0500
Subject: [PATCH 5/6] =?UTF-8?q?=F0=9F=A4=96=20Add=20detailed=20comments=20?=
 =?UTF-8?q?explaining=20git=20diff=20semantics?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Added comprehensive documentation to buildGitDiffCommand explaining:
- Three-dot vs two-dot diff behavior
- Why each case uses different git commands
- What includeUncommitted means in each context
- Examples for each scenario

This makes the logic clearer for future maintainers.

_Generated with `cmux`_
---
 .../RightSidebar/CodeReview/ReviewPanel.tsx   | 31 ++++++++++++++++---
 1 file changed, 26 insertions(+), 5 deletions(-)

diff --git a/src/components/RightSidebar/CodeReview/ReviewPanel.tsx b/src/components/RightSidebar/CodeReview/ReviewPanel.tsx
index ac897be00..d8abdfbdd 100644
--- a/src/components/RightSidebar/CodeReview/ReviewPanel.tsx
+++ b/src/components/RightSidebar/CodeReview/ReviewPanel.tsx
@@ -240,6 +240,19 @@ 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
+ *
+ * Git diff semantics:
+ * - `git diff A...B` (three-dot): Shows changes committed on B since branching from A
+ *   → Useful for reviewing "what did I commit on this branch?"
+ * - `git diff A` (two-dot/implicit): Shows all differences from A to working directory
+ *   → Useful for "what changed since I branched, including uncommitted?"
+ * - `git diff HEAD`: Shows uncommitted changes only (working vs HEAD)
+ * - `git diff --staged`: Shows staged changes only (index vs HEAD)
+ *
+ * @param diffBase - The base reference (e.g., "main", "HEAD", "--staged")
+ * @param includeUncommitted - Whether to include uncommitted working directory changes
+ * @param pathFilter - Optional file path filter (e.g., ' -- "src/foo.ts"')
+ * @param command - "diff" for unified diffs, "numstat" for file stats
  */
 export function buildGitDiffCommand(
   diffBase: string,
@@ -253,21 +266,29 @@ export function buildGitDiffCommand(
   let cmd: string;
 
   if (diffBase === "--staged") {
-    // Staged: show staged changes, optionally append unstaged
+    // Special case: reviewing staged changes
+    // Always show staged changes, optionally append unstaged as a separate diff
     cmd = `git diff --staged${flag}${pathFilter}`;
     if (includeUncommitted) {
+      // Append unstaged changes as second diff (uses &&)
+      // This is intentionally separate - user wants to see staged vs unstaged distinctly
       cmd += ` && git diff HEAD${flag}${pathFilter}`;
     }
   } else if (diffBase === "HEAD") {
-    // HEAD: already shows uncommitted changes (working directory vs HEAD)
+    // HEAD as base: always shows uncommitted changes (working vs HEAD)
+    // includeUncommitted is redundant here but doesn't hurt
     cmd = `git diff HEAD${flag}${pathFilter}`;
   } else {
-    // Branch diff: use three-dot for committed only, two-dot for all changes
+    // Branch diff (e.g., diffBase = "main", "origin/develop")
     if (includeUncommitted) {
-      // Two-dot: shows all changes from base to working directory (unified)
+      // Two-dot (implicit): Compare base to working directory
+      // Shows ALL changes: committed on this branch + uncommitted working changes
+      // Example: `git diff main` shows everything changed since branching from main
       cmd = `git diff ${diffBase}${flag}${pathFilter}`;
     } else {
-      // Three-dot: shows only committed changes (base...HEAD)
+      // Three-dot: Compare base merge-base to HEAD
+      // Shows ONLY committed changes (excludes working directory)
+      // Example: `git diff main...HEAD` shows commits on current branch, not dirty files
       cmd = `git diff ${diffBase}...HEAD${flag}${pathFilter}`;
     }
   }

From e24f9a5eeecc4d1b391712f9cc9c3eddcccbb03d Mon Sep 17 00:00:00 2001
From: Ammar <ammar+ai@ammar.io>
Date: Sat, 18 Oct 2025 22:31:43 -0500
Subject: [PATCH 6/6] =?UTF-8?q?=F0=9F=A4=96=20Cleanup:=20Simplify=20buildG?=
 =?UTF-8?q?itDiffCommand=20and=20add=20test=20coverage?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

- Condensed buildGitDiffCommand logic (removed redundant comments)
- Simplified test assertions and comments
- Added test for includeUncommitted=false (three-dot diff) case
- Net -30 lines, clearer code
---
 .../RightSidebar/CodeReview/ReviewPanel.tsx   | 58 ++++--------
 src/utils/git/diffParser.test.ts              | 88 +++++++++----------
 2 files changed, 59 insertions(+), 87 deletions(-)

diff --git a/src/components/RightSidebar/CodeReview/ReviewPanel.tsx b/src/components/RightSidebar/CodeReview/ReviewPanel.tsx
index d8abdfbdd..d2cac082e 100644
--- a/src/components/RightSidebar/CodeReview/ReviewPanel.tsx
+++ b/src/components/RightSidebar/CodeReview/ReviewPanel.tsx
@@ -241,18 +241,10 @@ interface DiagnosticInfo {
  * Shared logic between numstat (file tree) and diff (hunks) commands
  * Exported for testing
  *
- * Git diff semantics:
- * - `git diff A...B` (three-dot): Shows changes committed on B since branching from A
- *   → Useful for reviewing "what did I commit on this branch?"
- * - `git diff A` (two-dot/implicit): Shows all differences from A to working directory
- *   → Useful for "what changed since I branched, including uncommitted?"
- * - `git diff HEAD`: Shows uncommitted changes only (working vs HEAD)
- * - `git diff --staged`: Shows staged changes only (index vs HEAD)
- *
- * @param diffBase - The base reference (e.g., "main", "HEAD", "--staged")
- * @param includeUncommitted - Whether to include uncommitted working directory changes
- * @param pathFilter - Optional file path filter (e.g., ' -- "src/foo.ts"')
- * @param command - "diff" for unified diffs, "numstat" for file stats
+ * @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)
  */
 export function buildGitDiffCommand(
   diffBase: string,
@@ -260,40 +252,22 @@ export function buildGitDiffCommand(
   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") {
-    // Special case: reviewing staged changes
-    // Always show staged changes, optionally append unstaged as a separate diff
-    cmd = `git diff --staged${flag}${pathFilter}`;
-    if (includeUncommitted) {
-      // Append unstaged changes as second diff (uses &&)
-      // This is intentionally separate - user wants to see staged vs unstaged distinctly
-      cmd += ` && git diff HEAD${flag}${pathFilter}`;
-    }
-  } else if (diffBase === "HEAD") {
-    // HEAD as base: always shows uncommitted changes (working vs HEAD)
-    // includeUncommitted is redundant here but doesn't hurt
-    cmd = `git diff HEAD${flag}${pathFilter}`;
-  } else {
-    // Branch diff (e.g., diffBase = "main", "origin/develop")
-    if (includeUncommitted) {
-      // Two-dot (implicit): Compare base to working directory
-      // Shows ALL changes: committed on this branch + uncommitted working changes
-      // Example: `git diff main` shows everything changed since branching from main
-      cmd = `git diff ${diffBase}${flag}${pathFilter}`;
-    } else {
-      // Three-dot: Compare base merge-base to HEAD
-      // Shows ONLY committed changes (excludes working directory)
-      // Example: `git diff main...HEAD` shows commits on current branch, not dirty files
-      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;
+  }
+
+  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<ReviewPanelProps> = ({
diff --git a/src/utils/git/diffParser.test.ts b/src/utils/git/diffParser.test.ts
index d221f2a94..f7e2c93fb 100644
--- a/src/utils/git/diffParser.test.ts
+++ b/src/utils/git/diffParser.test.ts
@@ -369,14 +369,9 @@ function greet(name) {
   });
 
   it("should show unified diff when includeUncommitted is true", () => {
-    // This test verifies that when using "includeUncommitted" flag,
-    // we get a single unified diff from base to working directory
-    // (not separate diffs for committed and uncommitted)
-
-    // Reset and setup: create a branch with committed changes
+    // Verify includeUncommitted produces single unified diff (not separate committed + uncommitted)
     execSync("git reset --hard HEAD", { cwd: testRepoPath });
 
-    // Get the current branch name (will be our base branch)
     const baseBranch = execSync("git rev-parse --abbrev-ref HEAD", {
       cwd: testRepoPath,
       encoding: "utf-8",
@@ -384,88 +379,91 @@ function greet(name) {
 
     execSync("git checkout -b unified-test", { cwd: testRepoPath });
 
-    // Make and commit a change
+    // 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 });
-
-    // Now make an uncommitted change (different line)
     writeFileSync(join(testRepoPath, "test-file.txt"), "Line 1\nLine 2\nLine 3 modified\nLine 4\n");
 
-    // Use buildGitDiffCommand with includeUncommitted=true
     const gitCommand = buildGitDiffCommand(baseBranch, true, "", "diff");
+    const diffOutput = execSync(gitCommand, { cwd: testRepoPath, encoding: "utf-8" });
+    const fileDiffs = parseDiff(diffOutput);
 
-    // Execute the command
-    const diffOutput = execSync(gitCommand, {
+    // 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,
     });
 
-    // Parse the diff
+    // 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 a single FileDiff (no duplicates)
+    // Should get FileDiff showing only committed changes
     expect(fileDiffs.length).toBe(1);
-    expect(fileDiffs[0].filePath).toBe("test-file.txt");
+    expect(fileDiffs[0].filePath).toBe("committed-file.txt");
 
-    // Should have hunks showing unified changes
     const allHunks = extractAllHunks(fileDiffs);
-    expect(allHunks.length).toBeGreaterThan(0);
-
-    // All hunks should reference the same file
-    expect(allHunks.every((h) => h.filePath === "test-file.txt")).toBe(true);
-
-    // The diff should include BOTH the committed and uncommitted changes
-    // in a single unified view (not as separate hunks with different base states)
     const allContent = allHunks.map((h) => h.content).join("\n");
-    expect(allContent.includes("Line 3 modified") || allContent.includes("Line 4")).toBe(true);
 
-    // Clean up: reset and go back to previous branch
+    // 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", () => {
-    // When diffBase="--staged" with includeUncommitted=true,
-    // we want TWO separate diffs: staged changes AND unstaged changes
-    // This is the only case where we keep the && behavior
-
-    // Reset
+    // Verify --staged with includeUncommitted produces TWO diffs (staged + unstaged)
     execSync("git reset --hard HEAD", { cwd: testRepoPath });
 
-    // Create a file with changes
     writeFileSync(join(testRepoPath, "staged-test.txt"), "Line 1\nLine 2\nLine 3\n");
     execSync("git add staged-test.txt", { cwd: testRepoPath });
 
-    // Stage some changes
     writeFileSync(join(testRepoPath, "staged-test.txt"), "Line 1\nLine 2 staged\nLine 3\n");
     execSync("git add staged-test.txt", { cwd: testRepoPath });
 
-    // Make additional unstaged changes
     writeFileSync(
       join(testRepoPath, "staged-test.txt"),
       "Line 1\nLine 2 staged\nLine 3 unstaged\n"
     );
 
-    // Build command with --staged and includeUncommitted
     const gitCommand = buildGitDiffCommand("--staged", true, "", "diff");
-
-    // Execute
-    const diffOutput = execSync(gitCommand, {
-      cwd: testRepoPath,
-      encoding: "utf-8",
-    });
-
-    // Parse
+    const diffOutput = execSync(gitCommand, { cwd: testRepoPath, encoding: "utf-8" });
     const fileDiffs = parseDiff(diffOutput);
 
-    // Should get TWO FileDiff entries for the same file (staged + unstaged)
+    // Two FileDiff entries (staged + unstaged)
     expect(fileDiffs.length).toBe(2);
     expect(fileDiffs.every((f) => f.filePath === "staged-test.txt")).toBe(true);
 
-    // Should have hunks from both diffs
     const allHunks = extractAllHunks(fileDiffs);
     expect(allHunks.length).toBe(2);
 
-    // Content should show both staged and unstaged changes
     const allContent = allHunks.map((h) => h.content).join("\n");
     expect(allContent.includes("staged")).toBe(true);
     expect(allContent.includes("unstaged")).toBe(true);