fix(web): close file diff tab when uncommitted change is undone (#618)
This commit is contained in:
@@ -0,0 +1,38 @@
|
||||
import { describe, it, expect } from "vitest";
|
||||
import { shouldCloseFileDiffPanel } from "./task-changes-panel";
|
||||
|
||||
const PATH = "src/foo.ts";
|
||||
|
||||
describe("shouldCloseFileDiffPanel", () => {
|
||||
it("returns false when gitStatus is undefined (not loaded yet)", () => {
|
||||
expect(shouldCloseFileDiffPanel(undefined, PATH)).toBe(false);
|
||||
});
|
||||
|
||||
it("returns true when gitStatus.files is undefined (loaded, no changes)", () => {
|
||||
expect(shouldCloseFileDiffPanel({}, PATH)).toBe(true);
|
||||
});
|
||||
|
||||
it("returns true when the file is missing from gitStatus.files (discarded)", () => {
|
||||
expect(shouldCloseFileDiffPanel({ files: {} }, PATH)).toBe(true);
|
||||
});
|
||||
|
||||
it("returns false when the file has a non-empty uncommitted diff", () => {
|
||||
const gitStatus = { files: { [PATH]: { diff: "@@ -1 +1 @@\n-a\n+b\n" } } };
|
||||
expect(shouldCloseFileDiffPanel(gitStatus, PATH)).toBe(false);
|
||||
});
|
||||
|
||||
it("returns true when the file entry exists but diff is an empty string", () => {
|
||||
const gitStatus = { files: { [PATH]: { diff: "" } } };
|
||||
expect(shouldCloseFileDiffPanel(gitStatus, PATH)).toBe(true);
|
||||
});
|
||||
|
||||
it("returns true when the file entry exists but diff is undefined", () => {
|
||||
const gitStatus = { files: { [PATH]: {} } };
|
||||
expect(shouldCloseFileDiffPanel(gitStatus, PATH)).toBe(true);
|
||||
});
|
||||
|
||||
it("is not affected by unrelated files in gitStatus.files", () => {
|
||||
const gitStatus = { files: { "other/file.ts": { diff: "diff content" } } };
|
||||
expect(shouldCloseFileDiffPanel(gitStatus, PATH)).toBe(true);
|
||||
});
|
||||
});
|
||||
@@ -109,6 +109,16 @@ function addPRFiles(fileMap: Map<string, ReviewFile>, files: PRDiffFile[]) {
|
||||
}
|
||||
}
|
||||
|
||||
// Returns true only after gitStatus loads and the file's uncommitted diff is gone.
|
||||
export function shouldCloseFileDiffPanel(
|
||||
gitStatus: { files?: Record<string, { diff?: string }> } | undefined,
|
||||
filePath: string,
|
||||
): boolean {
|
||||
if (!gitStatus) return false;
|
||||
const entry = gitStatus.files?.[filePath];
|
||||
return !entry?.diff;
|
||||
}
|
||||
|
||||
/** Merge PR + uncommitted + committed files into a single sorted list */
|
||||
function mergeReviewFiles(
|
||||
gitStatus: ReturnType<typeof useSessionGitStatus>,
|
||||
@@ -211,6 +221,7 @@ function useChangesData(selectedDiff: SelectedDiff | null, onClearSelected: () =
|
||||
totalCommentCount,
|
||||
fileRefs,
|
||||
cumulativeLoading,
|
||||
gitStatus,
|
||||
};
|
||||
}
|
||||
|
||||
@@ -329,6 +340,40 @@ function useChangesActions(activeSessionId: string | null | undefined, allFiles:
|
||||
};
|
||||
}
|
||||
|
||||
function useAutoCloseWhenEmpty(opts: {
|
||||
mode: "all" | "file";
|
||||
filePath: string | undefined;
|
||||
gitStatus: { files?: Record<string, { diff?: string }> } | undefined;
|
||||
visibleCount: number;
|
||||
onBecameEmpty: (() => void) | undefined;
|
||||
}) {
|
||||
const { mode, filePath, gitStatus, visibleCount, onBecameEmpty } = opts;
|
||||
const prevVisibleCountRef = useRef<number | null>(null);
|
||||
const prevFileSeenRef = useRef<boolean>(false);
|
||||
|
||||
useEffect(() => {
|
||||
if (!onBecameEmpty) return;
|
||||
|
||||
if (mode === "file" && filePath) {
|
||||
// File-mode: close when the file no longer has an uncommitted diff,
|
||||
// regardless of whether it still appears in PR/cumulative diff sources.
|
||||
const shouldClose = shouldCloseFileDiffPanel(gitStatus, filePath);
|
||||
if (prevFileSeenRef.current && shouldClose) {
|
||||
onBecameEmpty();
|
||||
return;
|
||||
}
|
||||
if (!shouldClose) prevFileSeenRef.current = true;
|
||||
return;
|
||||
}
|
||||
|
||||
const prevCount = prevVisibleCountRef.current;
|
||||
if (prevCount !== null && prevCount > 0 && visibleCount === 0) {
|
||||
onBecameEmpty();
|
||||
}
|
||||
prevVisibleCountRef.current = visibleCount;
|
||||
}, [mode, filePath, gitStatus, onBecameEmpty, visibleCount]);
|
||||
}
|
||||
|
||||
const TaskChangesPanel = memo(function TaskChangesPanel({
|
||||
mode = "all",
|
||||
filePath,
|
||||
@@ -349,6 +394,7 @@ const TaskChangesPanel = memo(function TaskChangesPanel({
|
||||
totalCommentCount,
|
||||
fileRefs,
|
||||
cumulativeLoading,
|
||||
gitStatus,
|
||||
} = useChangesData(selectedDiff, onClearSelected);
|
||||
const visibleFiles = useMemo(() => {
|
||||
if (mode === "file" && filePath) {
|
||||
@@ -385,16 +431,13 @@ const TaskChangesPanel = memo(function TaskChangesPanel({
|
||||
);
|
||||
const totalCount = visibleFiles.length;
|
||||
const progressPercent = totalCount > 0 ? (reviewedCount / totalCount) * 100 : 0;
|
||||
const prevVisibleCountRef = useRef<number | null>(null);
|
||||
|
||||
useEffect(() => {
|
||||
if (!onBecameEmpty) return;
|
||||
const prevCount = prevVisibleCountRef.current;
|
||||
if (prevCount !== null && prevCount > 0 && visibleFiles.length === 0) {
|
||||
onBecameEmpty();
|
||||
}
|
||||
prevVisibleCountRef.current = visibleFiles.length;
|
||||
}, [onBecameEmpty, visibleFiles.length]);
|
||||
useAutoCloseWhenEmpty({
|
||||
mode,
|
||||
filePath,
|
||||
gitStatus,
|
||||
visibleCount: visibleFiles.length,
|
||||
onBecameEmpty,
|
||||
});
|
||||
|
||||
if (isArchived) return <ArchivedPanelPlaceholder />;
|
||||
|
||||
|
||||
@@ -206,6 +206,34 @@ test.describe("Diff update on file change", () => {
|
||||
timeout: 15_000,
|
||||
});
|
||||
});
|
||||
|
||||
test("diff panel closes when uncommitted change is undone via hunk Undo", async ({
|
||||
testPage,
|
||||
apiClient,
|
||||
seedData,
|
||||
}) => {
|
||||
// Regression: Undo must close the diff tab even when PR/cumulative diffs keep the file visible.
|
||||
await seedDiffUpdateTask(testPage, apiClient, seedData);
|
||||
await openChangesTab(testPage);
|
||||
await openFileDiff(testPage, "diff_update_test.txt");
|
||||
|
||||
const diffTab = testPage.locator(".dv-default-tab", { hasText: "diff_update_test.txt" });
|
||||
await expect(diffTab).toBeVisible({ timeout: 10_000 });
|
||||
|
||||
const diffsContainer = getDiffsContainer(testPage);
|
||||
await expect(diffsContainer).toBeVisible({ timeout: 15_000 });
|
||||
await expect(diffsContainer.getByText("FIRST_MODIFICATION", { exact: true })).toBeVisible({
|
||||
timeout: 15_000,
|
||||
});
|
||||
|
||||
// Button is CSS-hidden until hover; dispatchEvent bypasses pointer-events:none.
|
||||
const undoBtn = diffsContainer.locator("[data-undo-btn] button").first();
|
||||
await expect(undoBtn).toHaveCount(1, { timeout: 10_000 });
|
||||
await undoBtn.dispatchEvent("click");
|
||||
|
||||
// The Diff tab should close automatically.
|
||||
await expect(diffTab).toHaveCount(0, { timeout: 15_000 });
|
||||
});
|
||||
});
|
||||
|
||||
test.describe("Untracked file diff update", () => {
|
||||
|
||||
Reference in New Issue
Block a user