fix: refresh diff view when untracked files change (#372)
This commit is contained in:
@@ -36,12 +36,16 @@ func emitPredefinedScenario(enc *json.Encoder, scanner *bufio.Scanner, name stri
|
||||
scenarioDiffUpdateSetup(enc)
|
||||
case "diff-update-modify":
|
||||
scenarioDiffUpdateModify(enc)
|
||||
case "untracked-file-setup":
|
||||
scenarioUntrackedFileSetup(enc)
|
||||
case "untracked-file-modify":
|
||||
scenarioUntrackedFileModify(enc)
|
||||
case "clarification":
|
||||
scenarioClarification(enc)
|
||||
case "clarification-timeout":
|
||||
scenarioClarificationTimeout(enc)
|
||||
default:
|
||||
emitTextBlock(enc, "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, clarification, clarification-timeout", "")
|
||||
emitTextBlock(enc, "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", "")
|
||||
}
|
||||
}
|
||||
|
||||
@@ -545,6 +549,44 @@ func scenarioDiffUpdateModify(enc *json.Encoder) {
|
||||
emitTextBlock(enc, "diff-update-modify complete: diff_update_test.txt now has SECOND_MODIFICATION", "")
|
||||
}
|
||||
|
||||
// scenarioUntrackedFileSetup creates a new untracked file (not staged, not committed).
|
||||
// This tests that the UI shows diffs for untracked files.
|
||||
func scenarioUntrackedFileSetup(enc *json.Encoder) {
|
||||
fixedDelay(50)
|
||||
|
||||
filePath := "untracked_test.txt"
|
||||
content := "line 1: INITIAL_CONTENT\nline 2: some text\n"
|
||||
|
||||
// Remove any existing file first
|
||||
_ = os.Remove(filePath)
|
||||
|
||||
// Write new untracked file
|
||||
if err := os.WriteFile(filePath, []byte(content), 0o644); err != nil {
|
||||
emitTextBlock(enc, "untracked-file-setup: write failed: "+err.Error(), "")
|
||||
return
|
||||
}
|
||||
|
||||
fixedDelay(100)
|
||||
emitTextBlock(enc, "untracked-file-setup complete: untracked_test.txt has INITIAL_CONTENT", "")
|
||||
}
|
||||
|
||||
// scenarioUntrackedFileModify modifies the untracked file created by untracked-file-setup.
|
||||
// This tests that the UI updates the diff when an untracked file is modified.
|
||||
func scenarioUntrackedFileModify(enc *json.Encoder) {
|
||||
fixedDelay(50)
|
||||
|
||||
filePath := "untracked_test.txt"
|
||||
content := "line 1: MODIFIED_CONTENT\nline 2: some text\nline 3: NEW_LINE\n"
|
||||
|
||||
if err := os.WriteFile(filePath, []byte(content), 0o644); err != nil {
|
||||
emitTextBlock(enc, "untracked-file-modify: write failed: "+err.Error(), "")
|
||||
return
|
||||
}
|
||||
|
||||
fixedDelay(100)
|
||||
emitTextBlock(enc, "untracked-file-modify complete: untracked_test.txt now has MODIFIED_CONTENT", "")
|
||||
}
|
||||
|
||||
// clarificationQuestionArgs returns the MCP arguments for the clarification question.
|
||||
func clarificationQuestionArgs() map[string]any {
|
||||
return map[string]any{
|
||||
|
||||
@@ -579,18 +579,19 @@ func (wt *WorkspaceTracker) GetFileContentAtRef(ctx context.Context, reqPath str
|
||||
gitRef := fmt.Sprintf("%s:%s", ref, cleanPath)
|
||||
|
||||
// Preflight: check blob size before materializing content to avoid memory spikes.
|
||||
// Use CombinedOutput to capture stderr for error detection (git writes errors to stderr).
|
||||
// Set LC_ALL=C to ensure English error messages for reliable parsing.
|
||||
sizeCmd := exec.CommandContext(ctx, "git", "cat-file", "-s", gitRef)
|
||||
sizeCmd.Dir = wt.workDir
|
||||
sizeOut, err := sizeCmd.Output()
|
||||
sizeCmd.Env = append(os.Environ(), "LC_ALL=C")
|
||||
sizeOut, err := sizeCmd.CombinedOutput()
|
||||
if err != nil {
|
||||
if exitErr, ok := err.(*exec.ExitError); ok {
|
||||
stderr := string(exitErr.Stderr)
|
||||
if strings.Contains(stderr, "does not exist") ||
|
||||
strings.Contains(stderr, "Not a valid object") ||
|
||||
strings.Contains(stderr, "fatal: path") ||
|
||||
strings.Contains(stderr, "fatal: not a valid") {
|
||||
return "", 0, false, fmt.Errorf("file not found at ref %s: %s", ref, cleanPath)
|
||||
}
|
||||
output := string(sizeOut)
|
||||
if strings.Contains(output, "does not exist") ||
|
||||
strings.Contains(output, "Not a valid object") ||
|
||||
strings.Contains(output, "fatal: path") ||
|
||||
strings.Contains(output, "fatal: not a valid") {
|
||||
return "", 0, false, fmt.Errorf("file not found at ref %s: %s", ref, cleanPath)
|
||||
}
|
||||
return "", 0, false, fmt.Errorf("failed to stat file at ref: %w", err)
|
||||
}
|
||||
|
||||
@@ -5,6 +5,8 @@ import (
|
||||
"fmt"
|
||||
"os"
|
||||
"os/exec"
|
||||
"path/filepath"
|
||||
"strings"
|
||||
"time"
|
||||
|
||||
"github.com/kandev/kandev/internal/agentctl/types"
|
||||
@@ -61,11 +63,12 @@ func (wt *WorkspaceTracker) monitorLoop(ctx context.Context) {
|
||||
// workspaceState holds quick-check state for detecting workspace changes.
|
||||
type workspaceState struct {
|
||||
indexMtime time.Time // index mtime - changes on stage/unstage/commit
|
||||
diffFilesID string // hash of git diff-files output - changes on any file modification
|
||||
diffFilesID string // hash of git diff-files output - changes on tracked file modification
|
||||
untrackedID string // hash of untracked files list + mtimes - changes on untracked file changes
|
||||
}
|
||||
|
||||
// getWorkspaceState returns the current workspace state using fast checks.
|
||||
// Uses index mtime + git diff-files output which includes file content hashes.
|
||||
// Uses index mtime + git diff-files output + untracked file mtimes.
|
||||
func (wt *WorkspaceTracker) getWorkspaceState(ctx context.Context) workspaceState {
|
||||
var state workspaceState
|
||||
|
||||
@@ -87,12 +90,82 @@ func (wt *WorkspaceTracker) getWorkspaceState(ctx context.Context) workspaceStat
|
||||
state.diffFilesID = fmt.Sprintf("%d:%x:%x", len(out), out[0], out[len(out)-1])
|
||||
}
|
||||
|
||||
// Check untracked files - git diff-files doesn't include them
|
||||
// Use git ls-files to get untracked files, then check their mtimes
|
||||
state.untrackedID = wt.getUntrackedFilesID(ctx)
|
||||
|
||||
return state
|
||||
}
|
||||
|
||||
// getUntrackedFilesID returns a hash identifying the current state of untracked files.
|
||||
// Uses file list + mtimes to detect when untracked files are added, removed, or modified.
|
||||
func (wt *WorkspaceTracker) getUntrackedFilesID(ctx context.Context) string {
|
||||
// Get list of untracked files (excluding ignored)
|
||||
cmd := exec.CommandContext(ctx, "git", "ls-files", "--others", "--exclude-standard")
|
||||
cmd.Dir = wt.workDir
|
||||
out, err := cmd.Output()
|
||||
if err != nil || len(out) == 0 {
|
||||
return ""
|
||||
}
|
||||
|
||||
// Build a simple hash from file paths + mtimes
|
||||
// This is faster than hashing file contents
|
||||
lines := strings.Split(strings.TrimSpace(string(out)), "\n")
|
||||
var hashInput strings.Builder
|
||||
for _, file := range lines {
|
||||
if file == "" {
|
||||
continue
|
||||
}
|
||||
hashInput.WriteString(file)
|
||||
// Include mtime so we detect content changes
|
||||
// Sanitize path to prevent directory traversal attacks (CodeQL security fix)
|
||||
safePath, err := wt.sanitizePath(file)
|
||||
if err != nil {
|
||||
continue // Skip files with invalid paths
|
||||
}
|
||||
if info, err := os.Stat(safePath); err == nil {
|
||||
hashInput.WriteString(fmt.Sprintf(":%d", info.ModTime().UnixNano()))
|
||||
}
|
||||
hashInput.WriteString(";")
|
||||
}
|
||||
|
||||
// Return length + first/last bytes as cheap identity (same pattern as diffFilesID)
|
||||
s := hashInput.String()
|
||||
if len(s) > 0 {
|
||||
return fmt.Sprintf("%d:%x:%x", len(s), s[0], s[len(s)-1])
|
||||
}
|
||||
return ""
|
||||
}
|
||||
|
||||
// sanitizePath validates that the given relative file path stays within the workspace directory.
|
||||
// Returns the absolute path if valid, or an error if the path escapes the workspace.
|
||||
func (wt *WorkspaceTracker) sanitizePath(relPath string) (string, error) {
|
||||
// Clean the path to resolve any . or .. components
|
||||
joined := filepath.Join(wt.workDir, relPath)
|
||||
absPath, err := filepath.Abs(joined)
|
||||
if err != nil {
|
||||
return "", err
|
||||
}
|
||||
|
||||
// Ensure the resolved path is within the workspace directory
|
||||
workDirAbs, err := filepath.Abs(wt.workDir)
|
||||
if err != nil {
|
||||
return "", err
|
||||
}
|
||||
|
||||
// Check that absPath starts with workDirAbs (with proper separator handling)
|
||||
if !strings.HasPrefix(absPath, workDirAbs+string(os.PathSeparator)) && absPath != workDirAbs {
|
||||
return "", fmt.Errorf("path escapes workspace: %s", relPath)
|
||||
}
|
||||
|
||||
return absPath, nil
|
||||
}
|
||||
|
||||
// changed returns true if the workspace state has changed.
|
||||
func (s workspaceState) changed(other workspaceState) bool {
|
||||
return s.indexMtime != other.indexMtime || s.diffFilesID != other.diffFilesID
|
||||
return s.indexMtime != other.indexMtime ||
|
||||
s.diffFilesID != other.diffFilesID ||
|
||||
s.untrackedID != other.untrackedID
|
||||
}
|
||||
|
||||
// emitFileChanges sends accumulated file changes to subscribers.
|
||||
|
||||
@@ -4,24 +4,32 @@ import type { ApiClient } from "../helpers/api-client";
|
||||
import type { SeedData } from "../fixtures/test-base";
|
||||
import type { Page } from "@playwright/test";
|
||||
|
||||
// Shared selector constant for diff container
|
||||
const DIFFS_CONTAINER = "diffs-container";
|
||||
|
||||
/** Options for seeding a task with an agent. */
|
||||
type SeedTaskOptions = {
|
||||
title: string;
|
||||
scenarioCommand: string;
|
||||
completionText: string;
|
||||
};
|
||||
|
||||
/**
|
||||
* Seed a task using the diff-update-setup mock scenario and navigate to
|
||||
* Generic helper to seed a task using a mock scenario and navigate to
|
||||
* its session page, waiting for the agent turn to complete.
|
||||
*
|
||||
* The scenario creates a simple text file, commits it, then modifies line 1
|
||||
* to contain "FIRST_MODIFICATION", leaving an uncommitted diff.
|
||||
*/
|
||||
async function seedDiffUpdateTask(
|
||||
async function seedTaskWithScenario(
|
||||
testPage: Page,
|
||||
apiClient: ApiClient,
|
||||
seedData: SeedData,
|
||||
options: SeedTaskOptions,
|
||||
): Promise<{ session: SessionPage; sessionId: string }> {
|
||||
const task = await apiClient.createTaskWithAgent(
|
||||
seedData.workspaceId,
|
||||
"Diff Update E2E",
|
||||
options.title,
|
||||
seedData.agentProfileId,
|
||||
{
|
||||
description: "/e2e:diff-update-setup",
|
||||
description: options.scenarioCommand,
|
||||
workflow_id: seedData.workflowId,
|
||||
workflow_step_id: seedData.startStepId,
|
||||
repository_ids: [seedData.repositoryId],
|
||||
@@ -36,13 +44,31 @@ async function seedDiffUpdateTask(
|
||||
await session.waitForLoad();
|
||||
|
||||
// Wait for the first turn to complete
|
||||
await expect(session.chat.getByText("diff-update-setup complete", { exact: false })).toBeVisible({
|
||||
await expect(session.chat.getByText(options.completionText, { exact: false })).toBeVisible({
|
||||
timeout: 45_000,
|
||||
});
|
||||
|
||||
return { session, sessionId: task.session_id };
|
||||
}
|
||||
|
||||
/** Seed a task for untracked file testing. */
|
||||
function seedUntrackedFileTask(testPage: Page, apiClient: ApiClient, seedData: SeedData) {
|
||||
return seedTaskWithScenario(testPage, apiClient, seedData, {
|
||||
title: "Untracked File E2E",
|
||||
scenarioCommand: "/e2e:untracked-file-setup",
|
||||
completionText: "untracked-file-setup complete",
|
||||
});
|
||||
}
|
||||
|
||||
/** Seed a task for diff update testing. */
|
||||
function seedDiffUpdateTask(testPage: Page, apiClient: ApiClient, seedData: SeedData) {
|
||||
return seedTaskWithScenario(testPage, apiClient, seedData, {
|
||||
title: "Diff Update E2E",
|
||||
scenarioCommand: "/e2e:diff-update-setup",
|
||||
completionText: "diff-update-setup complete",
|
||||
});
|
||||
}
|
||||
|
||||
/** Click the Changes dockview tab. */
|
||||
async function openChangesTab(testPage: Page) {
|
||||
const changesTab = testPage.locator(".dv-default-tab", { hasText: "Changes" });
|
||||
@@ -50,26 +76,31 @@ async function openChangesTab(testPage: Page) {
|
||||
await changesTab.click();
|
||||
}
|
||||
|
||||
/** Click the file row for diff_update_test.txt to open its diff view. */
|
||||
async function openDiffUpdateFileDiff(testPage: Page) {
|
||||
/** Click a file row by name to open its diff view. */
|
||||
async function openFileDiff(testPage: Page, fileName: string) {
|
||||
const fileRow = testPage
|
||||
.locator("button, [role='button'], [class*='file']")
|
||||
.filter({ hasText: "diff_update_test.txt" })
|
||||
.filter({ hasText: fileName })
|
||||
.first();
|
||||
await expect(fileRow).toBeVisible({ timeout: 10_000 });
|
||||
await fileRow.click();
|
||||
}
|
||||
|
||||
/** Helper to get the diffs container locator. */
|
||||
function getDiffsContainer(testPage: Page) {
|
||||
return testPage.locator(DIFFS_CONTAINER);
|
||||
}
|
||||
|
||||
test.describe("Diff update on file change", () => {
|
||||
test.describe.configure({ retries: 2, timeout: 120_000 });
|
||||
|
||||
test("shows initial diff with FIRST_MODIFICATION", async ({ testPage, apiClient, seedData }) => {
|
||||
await seedDiffUpdateTask(testPage, apiClient, seedData);
|
||||
await openChangesTab(testPage);
|
||||
await openDiffUpdateFileDiff(testPage);
|
||||
await openFileDiff(testPage, "diff_update_test.txt");
|
||||
|
||||
// The Pierre Diffs viewer should show the initial modification
|
||||
const diffsContainer = testPage.locator("diffs-container");
|
||||
const diffsContainer = getDiffsContainer(testPage);
|
||||
await expect(diffsContainer).toBeVisible({ timeout: 15_000 });
|
||||
await expect(diffsContainer.getByText("FIRST_MODIFICATION", { exact: true })).toBeVisible({
|
||||
timeout: 15_000,
|
||||
@@ -79,10 +110,10 @@ test.describe("Diff update on file change", () => {
|
||||
test("diff updates when agent modifies file again", async ({ testPage, apiClient, seedData }) => {
|
||||
const { session } = await seedDiffUpdateTask(testPage, apiClient, seedData);
|
||||
await openChangesTab(testPage);
|
||||
await openDiffUpdateFileDiff(testPage);
|
||||
await openFileDiff(testPage, "diff_update_test.txt");
|
||||
|
||||
// Verify initial diff content (scoped to diffs-container to avoid matching chat text)
|
||||
const diffsContainer = testPage.locator("diffs-container");
|
||||
const diffsContainer = getDiffsContainer(testPage);
|
||||
await expect(diffsContainer).toBeVisible({ timeout: 15_000 });
|
||||
await expect(diffsContainer.getByText("FIRST_MODIFICATION", { exact: true })).toBeVisible({
|
||||
timeout: 15_000,
|
||||
@@ -103,10 +134,10 @@ test.describe("Diff update on file change", () => {
|
||||
// The git status (with diff data) should have been updated via polling when
|
||||
// the file changed - this is the bug we're testing for.
|
||||
await openChangesTab(testPage);
|
||||
await openDiffUpdateFileDiff(testPage);
|
||||
await openFileDiff(testPage, "diff_update_test.txt");
|
||||
|
||||
// Re-query the diffs container since the DOM may have changed after tab switch
|
||||
const updatedDiffsContainer = testPage.locator("diffs-container");
|
||||
const updatedDiffsContainer = getDiffsContainer(testPage);
|
||||
await expect(updatedDiffsContainer).toBeVisible({ timeout: 15_000 });
|
||||
|
||||
// The diff should now show SECOND_MODIFICATION instead of FIRST_MODIFICATION.
|
||||
@@ -126,3 +157,61 @@ test.describe("Diff update on file change", () => {
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
test.describe("Untracked file diff update", () => {
|
||||
test.describe.configure({ retries: 2, timeout: 120_000 });
|
||||
|
||||
test("untracked file diff updates when modified", async ({ testPage, apiClient, seedData }) => {
|
||||
// This test verifies that modifying an untracked file triggers a git status update
|
||||
// and the diff viewer shows the updated content. This was a bug where the polling
|
||||
// mechanism didn't detect untracked file changes (git diff-files only shows tracked files).
|
||||
const { session } = await seedUntrackedFileTask(testPage, apiClient, seedData);
|
||||
await openChangesTab(testPage);
|
||||
await openFileDiff(testPage, "untracked_test.txt");
|
||||
|
||||
// Verify initial diff content shows INITIAL_CONTENT
|
||||
// Note: exact match is false because the line shows "line 1: INITIAL_CONTENT"
|
||||
const diffsContainer = getDiffsContainer(testPage);
|
||||
await expect(diffsContainer).toBeVisible({ timeout: 15_000 });
|
||||
await expect(diffsContainer.getByText("INITIAL_CONTENT")).toBeVisible({
|
||||
timeout: 15_000,
|
||||
});
|
||||
|
||||
// Click on the Agent tab to make the chat input visible again
|
||||
await session.clickTab("Agent");
|
||||
|
||||
// Send another message to trigger the modification
|
||||
await session.sendMessage("/e2e:untracked-file-modify");
|
||||
|
||||
// Wait for the second turn to complete
|
||||
await expect(
|
||||
session.chat.getByText("untracked-file-modify complete", { exact: false }),
|
||||
).toBeVisible({ timeout: 45_000 });
|
||||
|
||||
// Wait for git polling to detect the file change (polling interval is ~1-2s)
|
||||
await testPage.waitForTimeout(3_000);
|
||||
|
||||
// Switch back to Changes tab and click on the diff file again
|
||||
await openChangesTab(testPage);
|
||||
await openFileDiff(testPage, "untracked_test.txt");
|
||||
|
||||
// Re-query the diffs container
|
||||
const updatedDiffsContainer = getDiffsContainer(testPage);
|
||||
await expect(updatedDiffsContainer).toBeVisible({ timeout: 15_000 });
|
||||
|
||||
// The diff should now show MODIFIED_CONTENT instead of INITIAL_CONTENT
|
||||
// Note: exact match is false because the line includes prefix text
|
||||
// Give extra time for git polling to detect and refresh the diff view
|
||||
await expect(updatedDiffsContainer.getByText("MODIFIED_CONTENT")).toBeVisible({
|
||||
timeout: 45_000,
|
||||
});
|
||||
|
||||
// Verify INITIAL_CONTENT is no longer shown
|
||||
await expect(updatedDiffsContainer.getByText("INITIAL_CONTENT")).toHaveCount(0);
|
||||
|
||||
// Also verify the new line was added
|
||||
await expect(updatedDiffsContainer.getByText("NEW_LINE")).toBeVisible({
|
||||
timeout: 15_000,
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user