fix(FN-1024): fix done-task changes tab overcounting and add diff fallback
- Fix TaskChangesTab overcounting changes for done tasks by handling missing commit SHA gracefully - Add /api/tasks/:id/diff endpoint with fallback for done tasks without merge commit info - Add comprehensive tests for both TaskChangesTab component and diff route edge cases - Document done-task Changes tab fallback behavior in README
This commit is contained in:
@@ -4042,6 +4042,204 @@ describe("POST /tasks/:id/reject-plan", () => {
|
||||
});
|
||||
});
|
||||
|
||||
// --- Task diff route tests ---
|
||||
|
||||
describe("GET /tasks/:id/diff", () => {
|
||||
let store: TaskStore;
|
||||
|
||||
beforeEach(() => {
|
||||
store = createMockStore();
|
||||
});
|
||||
|
||||
function buildApp() {
|
||||
const app = express();
|
||||
app.use(express.json());
|
||||
app.use("/api", createApiRoutes(store));
|
||||
return app;
|
||||
}
|
||||
|
||||
it("returns 404 when task not found", async () => {
|
||||
(store.getTask as ReturnType<typeof vi.fn>).mockResolvedValue(undefined);
|
||||
|
||||
const res = await GET(buildApp(), "/api/tasks/FN-999/diff");
|
||||
|
||||
expect(res.status).toBe(404);
|
||||
expect(res.body.error).toContain("Task not found");
|
||||
});
|
||||
|
||||
describe("done tasks without commit SHA", () => {
|
||||
it("returns safe empty file list with merge summary stats", async () => {
|
||||
const doneTask = {
|
||||
...FAKE_TASK_DETAIL,
|
||||
id: "FN-001",
|
||||
column: "done",
|
||||
mergeDetails: {
|
||||
filesChanged: 3,
|
||||
insertions: 10,
|
||||
deletions: 2,
|
||||
},
|
||||
};
|
||||
(store.getTask as ReturnType<typeof vi.fn>).mockResolvedValue(doneTask);
|
||||
|
||||
const res = await GET(buildApp(), "/api/tasks/FN-001/diff");
|
||||
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.body.files).toEqual([]);
|
||||
expect(res.body.stats).toEqual({
|
||||
filesChanged: 3,
|
||||
additions: 10,
|
||||
deletions: 2,
|
||||
});
|
||||
});
|
||||
|
||||
it("returns zeros when mergeDetails has no summary numbers", async () => {
|
||||
const doneTask = {
|
||||
...FAKE_TASK_DETAIL,
|
||||
id: "FN-001",
|
||||
column: "done",
|
||||
mergeDetails: {},
|
||||
};
|
||||
(store.getTask as ReturnType<typeof vi.fn>).mockResolvedValue(doneTask);
|
||||
|
||||
const res = await GET(buildApp(), "/api/tasks/FN-001/diff");
|
||||
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.body.files).toEqual([]);
|
||||
expect(res.body.stats).toEqual({
|
||||
filesChanged: 0,
|
||||
additions: 0,
|
||||
deletions: 0,
|
||||
});
|
||||
});
|
||||
|
||||
it("returns zeros when mergeDetails is undefined", async () => {
|
||||
const doneTask = {
|
||||
...FAKE_TASK_DETAIL,
|
||||
id: "FN-001",
|
||||
column: "done",
|
||||
mergeDetails: undefined,
|
||||
};
|
||||
(store.getTask as ReturnType<typeof vi.fn>).mockResolvedValue(doneTask);
|
||||
|
||||
const res = await GET(buildApp(), "/api/tasks/FN-001/diff");
|
||||
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.body.files).toEqual([]);
|
||||
expect(res.body.stats).toEqual({
|
||||
filesChanged: 0,
|
||||
additions: 0,
|
||||
deletions: 0,
|
||||
});
|
||||
});
|
||||
|
||||
it("response is schema-compatible with TaskDiff type", async () => {
|
||||
const doneTask = {
|
||||
...FAKE_TASK_DETAIL,
|
||||
id: "FN-001",
|
||||
column: "done",
|
||||
mergeDetails: { filesChanged: 5, insertions: 20, deletions: 3 },
|
||||
};
|
||||
(store.getTask as ReturnType<typeof vi.fn>).mockResolvedValue(doneTask);
|
||||
|
||||
const res = await GET(buildApp(), "/api/tasks/FN-001/diff");
|
||||
|
||||
// Must have both `files` array and `stats` object
|
||||
expect(res.status).toBe(200);
|
||||
expect(Array.isArray(res.body.files)).toBe(true);
|
||||
expect(res.body.stats).toHaveProperty("filesChanged");
|
||||
expect(res.body.stats).toHaveProperty("additions");
|
||||
expect(res.body.stats).toHaveProperty("deletions");
|
||||
});
|
||||
});
|
||||
|
||||
describe("done tasks with commit SHA", () => {
|
||||
it("attempts git diff when commitSha is present", async () => {
|
||||
// Use a real git repo to test the commit-backed path
|
||||
const testDir = mkdtempSync(join(tmpdir(), "kb-diff-test-"));
|
||||
try {
|
||||
execFileSync("git", ["init", testDir]);
|
||||
execFileSync("git", ["-C", testDir, "config", "user.email", "test@test.com"]);
|
||||
execFileSync("git", ["-C", testDir, "config", "user.name", "Test"]);
|
||||
writeFileSync(join(testDir, "a.txt"), "initial\n");
|
||||
execFileSync("git", ["-C", testDir, "add", "a.txt"]);
|
||||
execFileSync("git", ["-C", testDir, "commit", "-m", "init"]);
|
||||
|
||||
const headSha = execFileSync("git", ["-C", testDir, "rev-parse", "HEAD"], { encoding: "utf-8" }).trim();
|
||||
|
||||
const localStore = createMockStore({
|
||||
getRootDir: vi.fn().mockReturnValue(testDir),
|
||||
});
|
||||
const doneTask = {
|
||||
...FAKE_TASK_DETAIL,
|
||||
id: "FN-001",
|
||||
column: "done",
|
||||
mergeDetails: { commitSha: headSha },
|
||||
};
|
||||
(localStore.getTask as ReturnType<typeof vi.fn>).mockResolvedValue(doneTask);
|
||||
|
||||
const app = express();
|
||||
app.use(express.json());
|
||||
app.use("/api", createApiRoutes(localStore));
|
||||
|
||||
const res = await GET(app, "/api/tasks/FN-001/diff");
|
||||
expect(res.status).toBe(200);
|
||||
// The diff should be schema-compatible even if it returns empty
|
||||
expect(Array.isArray(res.body.files)).toBe(true);
|
||||
expect(res.body.stats).toHaveProperty("filesChanged");
|
||||
} finally {
|
||||
rmSync(testDir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe("GET /tasks/:id/file-diffs", () => {
|
||||
let store: TaskStore;
|
||||
|
||||
beforeEach(() => {
|
||||
store = createMockStore();
|
||||
});
|
||||
|
||||
function buildApp() {
|
||||
const app = express();
|
||||
app.use(express.json());
|
||||
app.use("/api", createApiRoutes(store));
|
||||
return app;
|
||||
}
|
||||
|
||||
describe("done tasks without commit SHA", () => {
|
||||
it("returns empty array instead of scanning repository", async () => {
|
||||
const doneTask = {
|
||||
...FAKE_TASK_DETAIL,
|
||||
id: "FN-001",
|
||||
column: "done",
|
||||
mergeDetails: { filesChanged: 3 },
|
||||
};
|
||||
(store.getTask as ReturnType<typeof vi.fn>).mockResolvedValue(doneTask);
|
||||
|
||||
const res = await GET(buildApp(), "/api/tasks/FN-001/file-diffs");
|
||||
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.body).toEqual([]);
|
||||
});
|
||||
|
||||
it("returns empty array when mergeDetails is undefined", async () => {
|
||||
const doneTask = {
|
||||
...FAKE_TASK_DETAIL,
|
||||
id: "FN-001",
|
||||
column: "done",
|
||||
mergeDetails: undefined,
|
||||
};
|
||||
(store.getTask as ReturnType<typeof vi.fn>).mockResolvedValue(doneTask);
|
||||
|
||||
const res = await GET(buildApp(), "/api/tasks/FN-001/file-diffs");
|
||||
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.body).toEqual([]);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
// --- Git Management route tests ---
|
||||
// These are integration tests that run against the actual git repository
|
||||
|
||||
|
||||
@@ -7857,6 +7857,22 @@ Output ONLY the prompt text (no markdown, no explanations).`;
|
||||
return;
|
||||
}
|
||||
|
||||
// Done tasks without a commit SHA: return safe, deterministic response.
|
||||
// Do NOT fall through to the worktree-based diff logic, which would use
|
||||
// the repo root as cwd and return an inflated repository-wide diff.
|
||||
if (task.column === "done") {
|
||||
const md = task.mergeDetails;
|
||||
res.json({
|
||||
files: [],
|
||||
stats: {
|
||||
filesChanged: md?.filesChanged ?? 0,
|
||||
additions: md?.insertions ?? 0,
|
||||
deletions: md?.deletions ?? 0,
|
||||
},
|
||||
});
|
||||
return;
|
||||
}
|
||||
|
||||
const worktree = typeof req.query.worktree === "string" ? req.query.worktree : undefined;
|
||||
const cwd = worktree || task.worktree || scopedStore.getRootDir();
|
||||
|
||||
@@ -8024,6 +8040,14 @@ Output ONLY the prompt text (no markdown, no explanations).`;
|
||||
return;
|
||||
}
|
||||
|
||||
// Done tasks without a commit SHA: return safe, empty response.
|
||||
// Do NOT fall through to worktree-based logic that could scan the
|
||||
// entire repository when the worktree has been cleaned up.
|
||||
if (task.column === "done") {
|
||||
res.json([]);
|
||||
return;
|
||||
}
|
||||
|
||||
if (!task.worktree || !nodeFs.existsSync(task.worktree)) {
|
||||
res.json([]);
|
||||
return;
|
||||
|
||||
Reference in New Issue
Block a user