From f06577e98911957b1384ecb74b1c9070f20cadc4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Almeida?= <57680069+zeval@users.noreply.github.com> Date: Wed, 18 Mar 2026 00:24:10 +0000 Subject: [PATCH] fix: include uncommitted changes in review dialog cumulative diff (#447) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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. --- apps/backend/cmd/mock-agent/scenarios.go | 96 ++++++++++++++++--- .../agentctl/server/process/git_log.go | 7 +- .../orchestrator/executor/executor.go | 6 +- .../internal/orchestrator/task_operations.go | 3 +- apps/web/components/review/review-dialog.tsx | 43 +++++---- .../e2e/tests/review-cumulative-diff.spec.ts | 86 +++++++++++++++++ 6 files changed, 202 insertions(+), 39 deletions(-) create mode 100644 apps/web/e2e/tests/review-cumulative-diff.spec.ts diff --git a/apps/backend/cmd/mock-agent/scenarios.go b/apps/backend/cmd/mock-agent/scenarios.go index 375e9db1a8..8c0c9f2d78 100644 --- a/apps/backend/cmd/mock-agent/scenarios.go +++ b/apps/backend/cmd/mock-agent/scenarios.go @@ -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(), diff --git a/apps/backend/internal/agentctl/server/process/git_log.go b/apps/backend/internal/agentctl/server/process/git_log.go index 434561a0ce..f1ea20bb38 100644 --- a/apps/backend/internal/agentctl/server/process/git_log.go +++ b/apps/backend/internal/agentctl/server/process/git_log.go @@ -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 diff --git a/apps/backend/internal/orchestrator/executor/executor.go b/apps/backend/internal/orchestrator/executor/executor.go index 91e4ee78db..fbc6e9f159 100644 --- a/apps/backend/internal/orchestrator/executor/executor.go +++ b/apps/backend/internal/orchestrator/executor/executor.go @@ -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. diff --git a/apps/backend/internal/orchestrator/task_operations.go b/apps/backend/internal/orchestrator/task_operations.go index ba740a5c1f..4758a4b911 100644 --- a/apps/backend/internal/orchestrator/task_operations.go +++ b/apps/backend/internal/orchestrator/task_operations.go @@ -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 { diff --git a/apps/web/components/review/review-dialog.tsx b/apps/web/components/review/review-dialog.tsx index 7f8086af35..bdd9e8f00d 100644 --- a/apps/web/components/review/review-dialog.tsx +++ b/apps/web/components/review/review-dialog.tsx @@ -16,11 +16,35 @@ import { ReviewDiffList } from "./review-diff-list"; import type { ReviewFile } from "./types"; import { hashDiff, normalizeDiffContent } from "./types"; +function addCumulativeDiffFiles( + fileMap: Map, + files: CumulativeDiff["files"], + gitStatusFiles: Record | 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, gitStatusFiles: Record, ) { 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, 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(); 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)); } diff --git a/apps/web/e2e/tests/review-cumulative-diff.spec.ts b/apps/web/e2e/tests/review-cumulative-diff.spec.ts new file mode 100644 index 0000000000..812467adc4 --- /dev/null +++ b/apps/web/e2e/tests/review-cumulative-diff.spec.ts @@ -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 }); + }); +});