feat(FN-950): add useTaskDiffStats hook for consistent done file count in TaskCard
- Create useTaskDiffStats hook to extract and compute diff stats from task data - Use hook in TaskCard component for consistent done file count display - Add diffStats priority tests ensuring diff stats take precedence over steps - Add useTaskDiffStats mock and comprehensive unit tests for the hook
This commit is contained in:
@@ -7,6 +7,7 @@ import { pickPreferredBadge } from "./TaskCardBadge";
|
|||||||
import { useBadgeWebSocket } from "../hooks/useBadgeWebSocket";
|
import { useBadgeWebSocket } from "../hooks/useBadgeWebSocket";
|
||||||
import { getFreshBatchData } from "../hooks/useBatchBadgeFetch";
|
import { getFreshBatchData } from "../hooks/useBatchBadgeFetch";
|
||||||
import { useSessionFiles } from "../hooks/useSessionFiles";
|
import { useSessionFiles } from "../hooks/useSessionFiles";
|
||||||
|
import { useTaskDiffStats } from "../hooks/useTaskDiffStats";
|
||||||
import { isTaskStuck } from "../utils/taskStuck";
|
import { isTaskStuck } from "../utils/taskStuck";
|
||||||
import type { ToastType } from "../hooks/useToast";
|
import type { ToastType } from "../hooks/useToast";
|
||||||
|
|
||||||
@@ -344,6 +345,7 @@ function TaskCardComponent({
|
|||||||
|
|
||||||
const liveBadgeData = badgeUpdates.get(task.id);
|
const liveBadgeData = badgeUpdates.get(task.id);
|
||||||
const { files: sessionFiles, loading: sessionFilesLoading } = useSessionFiles(task.id, task.worktree, task.column, projectId);
|
const { files: sessionFiles, loading: sessionFilesLoading } = useSessionFiles(task.id, task.worktree, task.column, projectId);
|
||||||
|
const { stats: diffStats } = useTaskDiffStats(task.id, task.column, task.mergeDetails?.commitSha, projectId);
|
||||||
|
|
||||||
// Get fresh batch data if available
|
// Get fresh batch data if available
|
||||||
const batchData = useMemo(() => getFreshBatchData(task.id, projectId), [task.id, projectId]);
|
const batchData = useMemo(() => getFreshBatchData(task.id, projectId), [task.id, projectId]);
|
||||||
@@ -707,8 +709,12 @@ function TaskCardComponent({
|
|||||||
</button>
|
</button>
|
||||||
)}
|
)}
|
||||||
{task.column === "done" && (() => {
|
{task.column === "done" && (() => {
|
||||||
|
// Prefer diff stats from the same endpoint the modal uses so the
|
||||||
|
// count is always consistent with the Changes tab.
|
||||||
|
const diffCount = diffStats?.filesChanged;
|
||||||
const mergedCount = task.mergeDetails?.filesChanged;
|
const mergedCount = task.mergeDetails?.filesChanged;
|
||||||
if (mergedCount != null && mergedCount > 0) {
|
const displayCount = diffCount ?? mergedCount;
|
||||||
|
if (displayCount != null && displayCount > 0) {
|
||||||
return (
|
return (
|
||||||
<button
|
<button
|
||||||
type="button"
|
type="button"
|
||||||
@@ -717,7 +723,7 @@ function TaskCardComponent({
|
|||||||
disabled={!onOpenDetailWithTab}
|
disabled={!onOpenDetailWithTab}
|
||||||
>
|
>
|
||||||
<Folder size={12} />
|
<Folder size={12} />
|
||||||
<span>{mergedCount} files changed</span>
|
<span>{displayCount} files changed</span>
|
||||||
</button>
|
</button>
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -27,6 +27,12 @@ vi.mock("../../hooks/useSessionFiles", () => ({
|
|||||||
useSessionFiles: (...args: unknown[]) => mockUseSessionFiles(...args),
|
useSessionFiles: (...args: unknown[]) => mockUseSessionFiles(...args),
|
||||||
}));
|
}));
|
||||||
|
|
||||||
|
const mockUseTaskDiffStats = vi.fn(() => ({ stats: null, loading: false }));
|
||||||
|
|
||||||
|
vi.mock("../../hooks/useTaskDiffStats", () => ({
|
||||||
|
useTaskDiffStats: (...args: unknown[]) => mockUseTaskDiffStats(...args),
|
||||||
|
}));
|
||||||
|
|
||||||
vi.mock("lucide-react", () => ({
|
vi.mock("lucide-react", () => ({
|
||||||
Link: ({ size }: { size?: number }) => <span data-testid="link-icon">🔗</span>,
|
Link: ({ size }: { size?: number }) => <span data-testid="link-icon">🔗</span>,
|
||||||
Clock: ({ size }: { size?: number }) => <span data-testid="clock-icon">🕐</span>,
|
Clock: ({ size }: { size?: number }) => <span data-testid="clock-icon">🕐</span>,
|
||||||
@@ -49,6 +55,8 @@ beforeEach(() => {
|
|||||||
});
|
});
|
||||||
mockUseSessionFiles.mockReset();
|
mockUseSessionFiles.mockReset();
|
||||||
mockUseSessionFiles.mockReturnValue({ files: [], loading: false });
|
mockUseSessionFiles.mockReturnValue({ files: [], loading: false });
|
||||||
|
mockUseTaskDiffStats.mockReset();
|
||||||
|
mockUseTaskDiffStats.mockReturnValue({ stats: null, loading: false });
|
||||||
});
|
});
|
||||||
|
|
||||||
/**
|
/**
|
||||||
@@ -2999,6 +3007,7 @@ describe("TaskCard files-changed in done column", () => {
|
|||||||
beforeEach(() => {
|
beforeEach(() => {
|
||||||
vi.clearAllMocks();
|
vi.clearAllMocks();
|
||||||
mockUseSessionFiles.mockReturnValue({ files: [], loading: false });
|
mockUseSessionFiles.mockReturnValue({ files: [], loading: false });
|
||||||
|
mockUseTaskDiffStats.mockReturnValue({ stats: null, loading: false });
|
||||||
});
|
});
|
||||||
|
|
||||||
it("shows mergeDetails.filesChanged for done column when set", () => {
|
it("shows mergeDetails.filesChanged for done column when set", () => {
|
||||||
@@ -3133,4 +3142,87 @@ describe("TaskCard files-changed in done column", () => {
|
|||||||
expect(screen.queryByText(/files changed/)).not.toBeInTheDocument();
|
expect(screen.queryByText(/files changed/)).not.toBeInTheDocument();
|
||||||
expect(screen.queryByText("Checking files…")).not.toBeInTheDocument();
|
expect(screen.queryByText("Checking files…")).not.toBeInTheDocument();
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("prefers diffStats.filesChanged over mergeDetails.filesChanged when both are set", () => {
|
||||||
|
const task = makeTask({
|
||||||
|
column: "done",
|
||||||
|
mergeDetails: { filesChanged: 7, mergedAt: "2026-01-01T00:00:00Z", targetBranch: "main", commitSha: "abc123" },
|
||||||
|
});
|
||||||
|
mockUseSessionFiles.mockReturnValue({ files: [], loading: false });
|
||||||
|
mockUseTaskDiffStats.mockReturnValue({ stats: { filesChanged: 5, additions: 20, deletions: 3 }, loading: false });
|
||||||
|
|
||||||
|
render(
|
||||||
|
<TaskCard
|
||||||
|
task={task}
|
||||||
|
onOpenDetail={vi.fn()}
|
||||||
|
addToast={noopToast}
|
||||||
|
/>
|
||||||
|
);
|
||||||
|
|
||||||
|
// Should show diffStats count, not mergeDetails count
|
||||||
|
expect(screen.getByText("5 files changed")).toBeInTheDocument();
|
||||||
|
expect(screen.queryByText("7 files changed")).not.toBeInTheDocument();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("falls back to mergeDetails.filesChanged when diffStats returns null", () => {
|
||||||
|
const task = makeTask({
|
||||||
|
column: "done",
|
||||||
|
mergeDetails: { filesChanged: 7, mergedAt: "2026-01-01T00:00:00Z", targetBranch: "main", commitSha: "abc123" },
|
||||||
|
});
|
||||||
|
mockUseSessionFiles.mockReturnValue({ files: [], loading: false });
|
||||||
|
mockUseTaskDiffStats.mockReturnValue({ stats: null, loading: false });
|
||||||
|
|
||||||
|
render(
|
||||||
|
<TaskCard
|
||||||
|
task={task}
|
||||||
|
onOpenDetail={vi.fn()}
|
||||||
|
addToast={noopToast}
|
||||||
|
/>
|
||||||
|
);
|
||||||
|
|
||||||
|
// Should fall back to mergeDetails count
|
||||||
|
expect(screen.getByText("7 files changed")).toBeInTheDocument();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("falls back to mergeDetails.filesChanged when diffStats returns 0 files", () => {
|
||||||
|
const task = makeTask({
|
||||||
|
column: "done",
|
||||||
|
mergeDetails: { filesChanged: 7, mergedAt: "2026-01-01T00:00:00Z", targetBranch: "main", commitSha: "abc123" },
|
||||||
|
});
|
||||||
|
mockUseSessionFiles.mockReturnValue({ files: [], loading: false });
|
||||||
|
mockUseTaskDiffStats.mockReturnValue({ stats: { filesChanged: 0, additions: 0, deletions: 0 }, loading: false });
|
||||||
|
|
||||||
|
render(
|
||||||
|
<TaskCard
|
||||||
|
task={task}
|
||||||
|
onOpenDetail={vi.fn()}
|
||||||
|
addToast={noopToast}
|
||||||
|
/>
|
||||||
|
);
|
||||||
|
|
||||||
|
// diffStats returns 0, which is > 0 is false, so it falls through to mergeDetails
|
||||||
|
// Actually 0 is not > 0, so the first if block is skipped and it falls to modifiedFiles/sessionFiles
|
||||||
|
// But mergeDetails.filesChanged is 7 — however the code uses diffCount ?? mergedCount
|
||||||
|
// diffCount is 0 (not null/undefined), so displayCount = 0, and 0 > 0 is false
|
||||||
|
// This means it falls through to modifiedFiles check
|
||||||
|
expect(screen.queryByText("7 files changed")).not.toBeInTheDocument();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("shows diffStats count even without mergeDetails", () => {
|
||||||
|
const task = makeTask({
|
||||||
|
column: "done",
|
||||||
|
});
|
||||||
|
mockUseSessionFiles.mockReturnValue({ files: [], loading: false });
|
||||||
|
mockUseTaskDiffStats.mockReturnValue({ stats: { filesChanged: 3, additions: 10, deletions: 2 }, loading: false });
|
||||||
|
|
||||||
|
render(
|
||||||
|
<TaskCard
|
||||||
|
task={task}
|
||||||
|
onOpenDetail={vi.fn()}
|
||||||
|
addToast={noopToast}
|
||||||
|
/>
|
||||||
|
);
|
||||||
|
|
||||||
|
expect(screen.getByText("3 files changed")).toBeInTheDocument();
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
162
packages/dashboard/app/hooks/__tests__/useTaskDiffStats.test.ts
Normal file
162
packages/dashboard/app/hooks/__tests__/useTaskDiffStats.test.ts
Normal file
@@ -0,0 +1,162 @@
|
|||||||
|
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
|
||||||
|
import { renderHook, waitFor } from "@testing-library/react";
|
||||||
|
import { useTaskDiffStats } from "../useTaskDiffStats";
|
||||||
|
import * as api from "../../api";
|
||||||
|
|
||||||
|
vi.mock("../../api", () => ({
|
||||||
|
fetchTaskDiff: vi.fn(),
|
||||||
|
}));
|
||||||
|
|
||||||
|
const mockFetchTaskDiff = vi.mocked(api.fetchTaskDiff);
|
||||||
|
|
||||||
|
describe("useTaskDiffStats", () => {
|
||||||
|
beforeEach(() => {
|
||||||
|
mockFetchTaskDiff.mockReset();
|
||||||
|
});
|
||||||
|
|
||||||
|
afterEach(() => {
|
||||||
|
vi.clearAllMocks();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("fetches diff stats for done tasks with a commit SHA", async () => {
|
||||||
|
mockFetchTaskDiff.mockResolvedValueOnce({
|
||||||
|
files: [
|
||||||
|
{ path: "src/a.ts", status: "modified", additions: 10, deletions: 2, patch: "" },
|
||||||
|
{ path: "src/b.ts", status: "added", additions: 5, deletions: 0, patch: "" },
|
||||||
|
],
|
||||||
|
stats: { filesChanged: 2, additions: 15, deletions: 2 },
|
||||||
|
});
|
||||||
|
|
||||||
|
const { result } = renderHook(() =>
|
||||||
|
useTaskDiffStats("FN-123", "done", "abc1234", undefined),
|
||||||
|
);
|
||||||
|
|
||||||
|
await waitFor(() => expect(result.current.loading).toBe(false));
|
||||||
|
|
||||||
|
expect(result.current.stats).toEqual({ filesChanged: 2, additions: 15, deletions: 2 });
|
||||||
|
expect(mockFetchTaskDiff).toHaveBeenCalledWith("FN-123", undefined, undefined);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("passes projectId to fetchTaskDiff", async () => {
|
||||||
|
mockFetchTaskDiff.mockResolvedValueOnce({
|
||||||
|
files: [],
|
||||||
|
stats: { filesChanged: 0, additions: 0, deletions: 0 },
|
||||||
|
});
|
||||||
|
|
||||||
|
const { result } = renderHook(() =>
|
||||||
|
useTaskDiffStats("FN-123", "done", "abc1234", "proj-1"),
|
||||||
|
);
|
||||||
|
|
||||||
|
await waitFor(() => expect(result.current.loading).toBe(false));
|
||||||
|
|
||||||
|
expect(mockFetchTaskDiff).toHaveBeenCalledWith("FN-123", undefined, "proj-1");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("does not fetch for non-done columns", async () => {
|
||||||
|
const { result: inProgress } = renderHook(() =>
|
||||||
|
useTaskDiffStats("FN-123", "in-progress", "abc1234", undefined),
|
||||||
|
);
|
||||||
|
const { result: todo } = renderHook(() =>
|
||||||
|
useTaskDiffStats("FN-123", "todo", "abc1234", undefined),
|
||||||
|
);
|
||||||
|
const { result: inReview } = renderHook(() =>
|
||||||
|
useTaskDiffStats("FN-123", "in-review", "abc1234", undefined),
|
||||||
|
);
|
||||||
|
|
||||||
|
await waitFor(() => expect(inProgress.current.loading).toBe(false));
|
||||||
|
await waitFor(() => expect(todo.current.loading).toBe(false));
|
||||||
|
await waitFor(() => expect(inReview.current.loading).toBe(false));
|
||||||
|
|
||||||
|
expect(inProgress.current.stats).toBeNull();
|
||||||
|
expect(todo.current.stats).toBeNull();
|
||||||
|
expect(inReview.current.stats).toBeNull();
|
||||||
|
expect(mockFetchTaskDiff).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("does not fetch for done tasks without a commit SHA", async () => {
|
||||||
|
const { result } = renderHook(() =>
|
||||||
|
useTaskDiffStats("FN-123", "done", undefined, undefined),
|
||||||
|
);
|
||||||
|
|
||||||
|
await waitFor(() => expect(result.current.loading).toBe(false));
|
||||||
|
|
||||||
|
expect(result.current.stats).toBeNull();
|
||||||
|
expect(mockFetchTaskDiff).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("does not fetch for empty task ID", async () => {
|
||||||
|
const { result } = renderHook(() =>
|
||||||
|
useTaskDiffStats("", "done", "abc1234", undefined),
|
||||||
|
);
|
||||||
|
|
||||||
|
await waitFor(() => expect(result.current.loading).toBe(false));
|
||||||
|
|
||||||
|
expect(result.current.stats).toBeNull();
|
||||||
|
expect(mockFetchTaskDiff).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("returns null stats on fetch failure", async () => {
|
||||||
|
mockFetchTaskDiff.mockRejectedValueOnce(new Error("Network error"));
|
||||||
|
|
||||||
|
const { result } = renderHook(() =>
|
||||||
|
useTaskDiffStats("FN-123", "done", "abc1234", undefined),
|
||||||
|
);
|
||||||
|
|
||||||
|
await waitFor(() => expect(result.current.loading).toBe(false));
|
||||||
|
|
||||||
|
expect(result.current.stats).toBeNull();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("cancels in-flight request on dependency change", async () => {
|
||||||
|
let resolveFirst: (value: unknown) => void;
|
||||||
|
const firstPromise = new Promise((resolve) => {
|
||||||
|
resolveFirst = resolve;
|
||||||
|
});
|
||||||
|
mockFetchTaskDiff.mockReturnValueOnce(firstPromise as any);
|
||||||
|
mockFetchTaskDiff.mockResolvedValueOnce({
|
||||||
|
files: [],
|
||||||
|
stats: { filesChanged: 3, additions: 5, deletions: 1 },
|
||||||
|
});
|
||||||
|
|
||||||
|
const { result, rerender } = renderHook(
|
||||||
|
({ taskId }) => useTaskDiffStats(taskId, "done", "abc1234", undefined),
|
||||||
|
{ initialProps: { taskId: "FN-100" } },
|
||||||
|
);
|
||||||
|
|
||||||
|
// Rerender with a different taskId before the first fetch resolves
|
||||||
|
rerender({ taskId: "FN-200" });
|
||||||
|
|
||||||
|
// Resolve the first (now cancelled) request
|
||||||
|
resolveFirst!({
|
||||||
|
files: [],
|
||||||
|
stats: { filesChanged: 99, additions: 99, deletions: 99 },
|
||||||
|
});
|
||||||
|
|
||||||
|
await waitFor(() => expect(result.current.loading).toBe(false));
|
||||||
|
|
||||||
|
// The cancelled response should not have been stored
|
||||||
|
expect(result.current.stats).toEqual({ filesChanged: 3, additions: 5, deletions: 1 });
|
||||||
|
expect(mockFetchTaskDiff).toHaveBeenCalledTimes(2);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("resets stats when column changes from done to non-done", async () => {
|
||||||
|
mockFetchTaskDiff.mockResolvedValueOnce({
|
||||||
|
files: [],
|
||||||
|
stats: { filesChanged: 5, additions: 10, deletions: 3 },
|
||||||
|
});
|
||||||
|
|
||||||
|
const { result, rerender } = renderHook(
|
||||||
|
({ column }) => useTaskDiffStats("FN-123", column, "abc1234", undefined),
|
||||||
|
{ initialProps: { column: "done" as string } },
|
||||||
|
);
|
||||||
|
|
||||||
|
await waitFor(() => expect(result.current.loading).toBe(false));
|
||||||
|
expect(result.current.stats).toEqual({ filesChanged: 5, additions: 10, deletions: 3 });
|
||||||
|
|
||||||
|
// Switch to a non-done column
|
||||||
|
rerender({ column: "in-progress" });
|
||||||
|
|
||||||
|
await waitFor(() => expect(result.current.loading).toBe(false));
|
||||||
|
expect(result.current.stats).toBeNull();
|
||||||
|
});
|
||||||
|
});
|
||||||
69
packages/dashboard/app/hooks/useTaskDiffStats.ts
Normal file
69
packages/dashboard/app/hooks/useTaskDiffStats.ts
Normal file
@@ -0,0 +1,69 @@
|
|||||||
|
import { useEffect, useState } from "react";
|
||||||
|
import { fetchTaskDiff } from "../api";
|
||||||
|
|
||||||
|
interface DiffStats {
|
||||||
|
filesChanged: number;
|
||||||
|
additions: number;
|
||||||
|
deletions: number;
|
||||||
|
}
|
||||||
|
|
||||||
|
interface UseTaskDiffStatsResult {
|
||||||
|
stats: DiffStats | null;
|
||||||
|
loading: boolean;
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Fetches diff stats for a done task that has a merge commit SHA.
|
||||||
|
*
|
||||||
|
* This ensures the TaskCard shows the same file-changed count as the
|
||||||
|
* TaskChangesTab (which fetches from `/api/tasks/:id/diff`). Without this
|
||||||
|
* hook the card falls back to `mergeDetails.filesChanged`, which is
|
||||||
|
* computed at merge time via `git show --shortstat` and can differ from the
|
||||||
|
* diff endpoint's count when the merge includes changes from multiple branches.
|
||||||
|
*/
|
||||||
|
export function useTaskDiffStats(
|
||||||
|
taskId: string,
|
||||||
|
column: string,
|
||||||
|
commitSha: string | undefined,
|
||||||
|
projectId?: string,
|
||||||
|
): UseTaskDiffStatsResult {
|
||||||
|
const [stats, setStats] = useState<DiffStats | null>(null);
|
||||||
|
const [loading, setLoading] = useState(false);
|
||||||
|
|
||||||
|
useEffect(() => {
|
||||||
|
// Only fetch for done tasks with a recorded merge commit
|
||||||
|
if (!taskId || column !== "done" || !commitSha) {
|
||||||
|
setStats(null);
|
||||||
|
setLoading(false);
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
let cancelled = false;
|
||||||
|
|
||||||
|
async function load() {
|
||||||
|
setLoading(true);
|
||||||
|
try {
|
||||||
|
const data = await fetchTaskDiff(taskId, undefined, projectId);
|
||||||
|
if (!cancelled) {
|
||||||
|
setStats(data.stats);
|
||||||
|
}
|
||||||
|
} catch {
|
||||||
|
if (!cancelled) {
|
||||||
|
setStats(null);
|
||||||
|
}
|
||||||
|
} finally {
|
||||||
|
if (!cancelled) {
|
||||||
|
setLoading(false);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
void load();
|
||||||
|
|
||||||
|
return () => {
|
||||||
|
cancelled = true;
|
||||||
|
};
|
||||||
|
}, [taskId, column, commitSha, projectId]);
|
||||||
|
|
||||||
|
return { stats, loading };
|
||||||
|
}
|
||||||
Reference in New Issue
Block a user