fix: include uncommitted changes in review dialog cumulative diff (#447)

* fix: include uncommitted changes in review dialog cumulative diff The review dialog only showed uncommitted changes instead of the full diff from the task's base commit. Two issues combined: 1. Backend used `git diff base..HEAD` (committed only); changed to `git diff base` which compares against the working tree. 2. Frontend gave priority to git status over cumulative diff; reversed the merge order so cumulative diff takes precedence. Adds E2E test with a mock-agent scenario that commits changes then leaves uncommitted modifications, verifying the review dialog shows the complete base→working-tree diff. * fix(e2e): remove incorrect BASE_CONTENT assertion in review cumulative diff test The file is created after the base commit, so the entire file shows as added in the cumulative diff. BASE_CONTENT was replaced and doesn't appear. The key assertion is that COMMITTED_CHANGE is visible — proving the diff includes committed changes (not just the uncommitted portion). * fix: address CodeRabbit review feedback - Update 3 docstrings to reflect working-tree diff (not HEAD) - Generate scenario list dynamically in error message - Use data-testid selector in E2E test for resilience * style: format E2E test file * fix(e2e): use toContainText and click file to ensure diff is scrollable Pierre diff viewer content inside a scroll container may not be considered "visible" by Playwright. Switch to toContainText which checks text presence without element-level visibility. Also click the file in the tree to ensure its diff section is scrolled into view.
2026-03-18 00:24:10 +00:00
parent 6df276e392
commit f06577e989
6 changed files with 202 additions and 39 deletions
@@ -15,20 +15,21 @@ import (

 // scenarioRegistry maps scenario names to their handler functions.
 var scenarioRegistry = map[string]func(e *emitter){
-	"simple-message":        scenarioSimpleMessage,
-	"read-and-edit":         scenarioReadAndEdit,
-	"permission-flow":       scenarioPermissionFlow,
-	"error":                 scenarioError,
-	"subagent":              scenarioSubagent,
-	"all-tools":             scenarioAllTools,
-	"multi-turn":            scenarioMultiTurn,
-	"diff-expansion-setup":  scenarioDiffExpansionSetup,
-	"diff-update-setup":     scenarioDiffUpdateSetup,
-	"diff-update-modify":    scenarioDiffUpdateModify,
-	"untracked-file-setup":  scenarioUntrackedFileSetup,
-	"untracked-file-modify": scenarioUntrackedFileModify,
-	"clarification":         scenarioClarification,
-	"clarification-timeout": scenarioClarificationTimeout,
+	"simple-message":          scenarioSimpleMessage,
+	"read-and-edit":           scenarioReadAndEdit,
+	"permission-flow":         scenarioPermissionFlow,
+	"error":                   scenarioError,
+	"subagent":                scenarioSubagent,
+	"all-tools":               scenarioAllTools,
+	"multi-turn":              scenarioMultiTurn,
+	"diff-expansion-setup":    scenarioDiffExpansionSetup,
+	"diff-update-setup":       scenarioDiffUpdateSetup,
+	"diff-update-modify":      scenarioDiffUpdateModify,
+	"untracked-file-setup":    scenarioUntrackedFileSetup,
+	"untracked-file-modify":   scenarioUntrackedFileModify,
+	"clarification":           scenarioClarification,
+	"clarification-timeout":   scenarioClarificationTimeout,
+	"review-cumulative-setup": scenarioReviewCumulativeSetup,
 }

 // emitPredefinedScenario dispatches to a named e2e scenario.
@@ -36,7 +37,11 @@ func emitPredefinedScenario(e *emitter, name string) {
 	if fn, ok := scenarioRegistry[name]; ok {
 		fn(e)
 	} else {
-		e.text("Unknown e2e scenario: " + name + ". Available: simple-message, read-and-edit, permission-flow, error, subagent, all-tools, multi-turn, diff-expansion-setup, diff-update-setup, diff-update-modify, untracked-file-setup, untracked-file-modify, clarification, clarification-timeout")
+		names := make([]string, 0, len(scenarioRegistry))
+		for k := range scenarioRegistry {
+			names = append(names, k)
+		}
+		e.text("Unknown e2e scenario: " + name + ". Available: " + strings.Join(names, ", "))
 	}
 }

@@ -435,6 +440,67 @@ func scenarioClarificationTimeout(e *emitter) {
 	e.text(fmt.Sprintf("You answered: %s", result))
 }

+// scenarioReviewCumulativeSetup creates a file, commits it, then modifies and
+// commits again, then makes a final uncommitted modification.  This produces a
+// file with both committed and uncommitted changes relative to the session's
+// base commit, exercising the cumulative diff (base → working tree).
+func scenarioReviewCumulativeSetup(e *emitter) {
+	fixedDelay(50)
+
+	wd, err := os.Getwd()
+	if err != nil {
+		e.text("review-cumulative-setup: getwd failed: " + err.Error())
+		return
+	}
+
+	filePath := "review_cumulative_test.txt"
+	baseContent := "line 1: BASE_CONTENT\nline 2: unchanged\nline 3: BASE_CONTENT\n"
+
+	// Clean up any leftover file from a previous run.
+	runGitCmd := makeGitRunner(wd)
+	_ = runGitCmd("rm", "--force", filePath)
+	_ = runGitCmd("commit", "-m", "cleanup "+filePath)
+
+	// Write original content and commit.
+	if err := os.WriteFile(filePath, []byte(baseContent), 0o644); err != nil {
+		e.text("review-cumulative-setup: write base failed: " + err.Error())
+		return
+	}
+	if err := runGitCmd("add", filePath); err != nil {
+		e.text("review-cumulative-setup: git add base failed")
+		return
+	}
+	if err := runGitCmd("commit", "-m", "add "+filePath+" with base content"); err != nil {
+		e.text("review-cumulative-setup: git commit base failed")
+		return
+	}
+
+	// Second commit: modify the file.
+	committedContent := "line 1: COMMITTED_CHANGE\nline 2: unchanged\nline 3: BASE_CONTENT\n"
+	if err := os.WriteFile(filePath, []byte(committedContent), 0o644); err != nil {
+		e.text("review-cumulative-setup: write committed failed: " + err.Error())
+		return
+	}
+	if err := runGitCmd("add", filePath); err != nil {
+		e.text("review-cumulative-setup: git add committed failed")
+		return
+	}
+	if err := runGitCmd("commit", "-m", "modify "+filePath+" with committed change"); err != nil {
+		e.text("review-cumulative-setup: git commit committed failed")
+		return
+	}
+
+	// Leave an uncommitted modification on top.
+	uncommittedContent := "line 1: COMMITTED_CHANGE\nline 2: unchanged\nline 3: UNCOMMITTED_CHANGE\n"
+	if err := os.WriteFile(filePath, []byte(uncommittedContent), 0o644); err != nil {
+		e.text("review-cumulative-setup: write uncommitted failed: " + err.Error())
+		return
+	}
+
+	fixedDelay(100)
+	e.text("review-cumulative-setup complete: " + filePath + " has COMMITTED_CHANGE and UNCOMMITTED_CHANGE")
+}
+
 // makeGitRunner returns a function that runs git commands in the given directory.
 func makeGitRunner(wd string) func(args ...string) error {
 	gitEnv := append(os.Environ(),
@@ -117,7 +117,8 @@ func (g *GitOperator) GetLog(ctx context.Context, baseCommit string, limit int)
 	return result, nil
 }

-// GetCumulativeDiff returns the cumulative diff from baseCommit to HEAD.
+// GetCumulativeDiff returns the cumulative diff from baseCommit to the working tree
+// (including uncommitted/unstaged changes).
 func (g *GitOperator) GetCumulativeDiff(ctx context.Context, baseCommit string) (*CumulativeDiffResult, error) {
 	result := &CumulativeDiffResult{
 		Files: make(map[string]interface{}),
@@ -144,8 +145,8 @@ func (g *GitOperator) GetCumulativeDiff(ctx context.Context, baseCommit string)
 		_, _ = fmt.Sscanf(strings.TrimSpace(countOutput), "%d", &result.TotalCommits)
 	}

-	// Get the cumulative diff
-	diffOutput, err := g.runGitCommand(ctx, "diff", baseCommit+"..HEAD")
+	// Get the cumulative diff (base → working tree, includes uncommitted changes)
+	diffOutput, err := g.runGitCommand(ctx, "diff", baseCommit)
 	if err != nil {
 		result.Error = fmt.Sprintf("failed to get diff: %s", err.Error())
 		return result, nil
@@ -154,8 +154,10 @@ type AgentManagerClient interface {
 	// Used for archive snapshot capture. Returns nil, nil if no execution exists.
 	GetGitLog(ctx context.Context, sessionID, baseCommit string, limit int) (*client.GitLogResult, error)

-	// GetCumulativeDiff retrieves the cumulative diff for a session from baseCommit to HEAD.
-	// Used for archive snapshot capture. Returns nil, nil if no execution exists.
+	// GetCumulativeDiff retrieves the cumulative diff for a session from baseCommit to the
+	// working tree (including uncommitted/unstaged changes). Used for archive snapshot capture.
+	// Note: archive snapshots will capture uncommitted working-tree state.
+	// Returns nil, nil if no execution exists.
 	GetCumulativeDiff(ctx context.Context, sessionID, baseCommit string) (*client.CumulativeDiffResult, error)

 	// GetGitStatus retrieves the current git status for a session.
@@ -1095,7 +1095,8 @@ func (s *Service) captureArchiveCommits(ctx context.Context, sessionID, baseComm
 	return true
 }

-// captureArchiveDiff fetches and saves the cumulative diff from baseCommit to HEAD.
+// captureArchiveDiff fetches and saves the cumulative diff from baseCommit to the working tree
+// (including uncommitted/unstaged changes).
 func (s *Service) captureArchiveDiff(ctx context.Context, sessionID, baseCommit string) {
 	diffResult, err := s.agentManager.GetCumulativeDiff(ctx, sessionID, baseCommit)
 	if err != nil {
@@ -16,11 +16,35 @@ import { ReviewDiffList } from "./review-diff-list";
 import type { ReviewFile } from "./types";
 import { hashDiff, normalizeDiffContent } from "./types";

+function addCumulativeDiffFiles(
+  fileMap: Map<string, ReviewFile>,
+  files: CumulativeDiff["files"],
+  gitStatusFiles: Record<string, FileInfo> | null,
+) {
+  for (const [path, file] of Object.entries(files)) {
+    if (fileMap.has(path)) continue;
+    const diff = file.diff ? normalizeDiffContent(file.diff) : "";
+    if (diff) {
+      const hasUncommitted = gitStatusFiles != null && path in gitStatusFiles;
+      fileMap.set(path, {
+        path,
+        diff,
+        status: file.status || "modified",
+        additions: file.additions ?? 0,
+        deletions: file.deletions ?? 0,
+        staged: gitStatusFiles?.[path]?.staged ?? false,
+        source: hasUncommitted ? "uncommitted" : "committed",
+      });
+    }
+  }
+}
+
 function addUncommittedFiles(
  fileMap: Map<string, ReviewFile>,
  gitStatusFiles: Record<string, FileInfo>,
 ) {
  for (const [path, file] of Object.entries(gitStatusFiles)) {
+    if (fileMap.has(path)) continue;
    const diff = file.diff ? normalizeDiffContent(file.diff) : "";
    if (diff)
      fileMap.set(path, {
@@ -35,23 +59,6 @@ function addUncommittedFiles(
  }
 }

-function addCommittedFiles(fileMap: Map<string, ReviewFile>, files: CumulativeDiff["files"]) {
-  for (const [path, file] of Object.entries(files)) {
-    if (fileMap.has(path)) continue;
-    const diff = file.diff ? normalizeDiffContent(file.diff) : "";
-    if (diff)
-      fileMap.set(path, {
-        path,
-        diff,
-        status: file.status || "modified",
-        additions: file.additions ?? 0,
-        deletions: file.deletions ?? 0,
-        staged: false,
-        source: "committed",
-      });
-  }
-}
-
 function prFileStatus(status: string): "added" | "deleted" | "modified" {
  if (status === "added") return "added";
  if (status === "removed") return "deleted";
@@ -82,8 +89,8 @@ function buildAllFiles(
 ): ReviewFile[] {
  const fileMap = new Map<string, ReviewFile>();
  if (prDiffFiles) addPRFiles(fileMap, prDiffFiles);
+  if (cumulativeDiff?.files) addCumulativeDiffFiles(fileMap, cumulativeDiff.files, gitStatusFiles);
  if (gitStatusFiles) addUncommittedFiles(fileMap, gitStatusFiles);
-  if (cumulativeDiff?.files) addCommittedFiles(fileMap, cumulativeDiff.files);
  return Array.from(fileMap.values()).sort((a, b) => a.path.localeCompare(b.path));
 }

@@ -0,0 +1,86 @@
+import { test, expect } from "../fixtures/test-base";
+import { SessionPage } from "../pages/session-page";
+import type { ApiClient } from "../helpers/api-client";
+import type { SeedData } from "../fixtures/test-base";
+import type { Page } from "@playwright/test";
+
+/**
+ * Seed a task that creates a file with BASE_CONTENT, commits it,
+ * then commits a COMMITTED_CHANGE, then leaves an UNCOMMITTED_CHANGE.
+ */
+async function seedReviewCumulativeTask(
+  testPage: Page,
+  apiClient: ApiClient,
+  seedData: SeedData,
+): Promise<{ session: SessionPage; sessionId: string }> {
+  const task = await apiClient.createTaskWithAgent(
+    seedData.workspaceId,
+    "Review Cumulative Diff E2E",
+    seedData.agentProfileId,
+    {
+      description: "/e2e:review-cumulative-setup",
+      workflow_id: seedData.workflowId,
+      workflow_step_id: seedData.startStepId,
+      repository_ids: [seedData.repositoryId],
+    },
+  );
+
+  if (!task.session_id) throw new Error("createTaskWithAgent did not return a session_id");
+
+  await testPage.goto(`/s/${task.session_id}`);
+
+  const session = new SessionPage(testPage);
+  await session.waitForLoad();
+
+  await expect(
+    session.chat.getByText("review-cumulative-setup complete", { exact: false }),
+  ).toBeVisible({ timeout: 45_000 });
+
+  return { session, sessionId: task.session_id };
+}
+
+/** Open the review dialog via the custom DOM event. */
+async function openReviewDialog(testPage: Page) {
+  await testPage.evaluate(() => window.dispatchEvent(new CustomEvent("open-review-dialog")));
+}
+
+test.describe("Review dialog cumulative diff", () => {
+  test.describe.configure({ retries: 2, timeout: 120_000 });
+
+  test("shows full diff from base commit including committed and uncommitted changes", async ({
+    testPage,
+    apiClient,
+    seedData,
+  }) => {
+    await seedReviewCumulativeTask(testPage, apiClient, seedData);
+
+    // Open the Changes tab first so the review dialog has git status data
+    const changesTab = testPage.locator(".dv-default-tab", { hasText: "Changes" });
+    await expect(changesTab).toBeVisible({ timeout: 10_000 });
+    await changesTab.click();
+
+    // Wait for git status to populate (file should appear in the changes panel)
+    await expect(testPage.getByTestId("file-row-review_cumulative_test.txt")).toBeVisible({
+      timeout: 15_000,
+    });
+
+    // Open the review dialog
+    await openReviewDialog(testPage);
+
+    // The review dialog renders inside a Dialog — find it via the sr-only title
+    const dialog = testPage.getByRole("dialog", { name: "Review Changes" });
+    await expect(dialog).toBeVisible({ timeout: 10_000 });
+
+    // Click on the file in the tree to ensure its diff is scrolled into view
+    await dialog.getByText("review_cumulative_test.txt").first().click();
+
+    // The diff should show COMMITTED_CHANGE — this is the key assertion.
+    // With the old bug (git diff base..HEAD + uncommitted-first priority),
+    // only the HEAD→working-tree diff would appear, so COMMITTED_CHANGE
+    // (which was committed, not in the uncommitted diff) would be missing.
+    await expect(dialog).toContainText("COMMITTED_CHANGE", { timeout: 30_000 });
+
+    // The diff should also show UNCOMMITTED_CHANGE (current working tree state)
+    await expect(dialog).toContainText("UNCOMMITTED_CHANGE", { timeout: 15_000 });
+  });
+});