feat(FN-1584): add step-change dependency and polling to useTaskDiffStats

- Add step-based dependency to useTaskDiffStats hook to trigger refetch when task steps change - Implement 5-second polling interval for live diff statistics updates - Add comprehensive test coverage for polling behavior and step-change detection - Update TaskCard to pass step information to useTaskDiffStats - Add TaskCard unit tests for diff statistics display
2026-04-11 11:27:32 -07:00
parent fee27abb56
commit 98dc20b3d3
4 changed files with 464 additions and 19 deletions
--- a/packages/dashboard/app/components/TaskCard.tsx
+++ b/packages/dashboard/app/components/TaskCard.tsx
@@ -491,13 +491,25 @@ function TaskCardComponent({

  const liveBadgeData = badgeUpdates.get(task.id);

+  // Compute step version for diff stats refresh when steps change
+  const isActiveColumn = task.column === "in-progress" || task.column === "in-review";
+  const stepVersion = useMemo(
+    () => task.steps.map((s) => `${s.name}:${s.status}`).join("|"),
+    [task.steps],
+  );
+
  // Viewport-gated diff stats fetching - only fetch when card is visible
  const { stats: diffStats } = useTaskDiffStats(
    task.id,
    task.column,
    task.mergeDetails?.commitSha,
    projectId,
-    { enabled: isInViewport, worktree: task.worktree },
+    {
+      enabled: isInViewport,
+      worktree: task.worktree,
+      stepVersion: isActiveColumn ? stepVersion : undefined,
+      pollIntervalMs: isActiveColumn ? 30_000 : undefined,
+    },
  );

  // Get fresh batch data if available
--- a/packages/dashboard/app/components/tests/TaskCard.test.tsx
+++ b/packages/dashboard/app/components/tests/TaskCard.test.tsx
@@ -3711,4 +3711,105 @@ describe("TaskCard send-back functionality", () => {
    // Dropdown should be closed
    expect(screen.queryByRole("menu")).not.toBeInTheDocument();
  });
+
+  describe("useTaskDiffStats integration", () => {
+    beforeEach(() => {
+      mockUseTaskDiffStats.mockClear();
+      mockUseTaskDiffStats.mockReturnValue({ stats: null, loading: false });
+    });
+
+    it("passes stepVersion and pollIntervalMs for in-progress tasks", () => {
+      const task = createTask({
+        column: "in-progress",
+        steps: [
+          { name: "Step 1", status: "pending" },
+          { name: "Step 2", status: "done" },
+        ],
+        worktree: "/repo/.worktrees/fn-001",
+      });
+
+      render(<TaskCard task={task} onOpenDetail={vi.fn()} addToast={vi.fn()} />);
+
+      // Verify useTaskDiffStats was called
+      expect(mockUseTaskDiffStats).toHaveBeenCalled();
+
+      // Get the options argument (5th argument)
+      const callArgs = mockUseTaskDiffStats.mock.calls[0];
+      const options = callArgs[4] as Record<string, unknown>;
+
+      // Should pass stepVersion for in-progress
+      expect(options.stepVersion).toBe("Step 1:pending|Step 2:done");
+
+      // Should pass pollIntervalMs for in-progress
+      expect(options.pollIntervalMs).toBe(30_000);
+    });
+
+    it("passes stepVersion and pollIntervalMs for in-review tasks", () => {
+      const task = createTask({
+        column: "in-review",
+        steps: [{ name: "Verify", status: "in-progress" }],
+        worktree: "/repo/.worktrees/fn-001",
+      });
+
+      render(<TaskCard task={task} onOpenDetail={vi.fn()} addToast={vi.fn()} />);
+
+      const callArgs = mockUseTaskDiffStats.mock.calls[0];
+      const options = callArgs[4] as Record<string, unknown>;
+
+      expect(options.stepVersion).toBe("Verify:in-progress");
+      expect(options.pollIntervalMs).toBe(30_000);
+    });
+
+    it("does not pass stepVersion or pollIntervalMs for done tasks", () => {
+      const task = createTask({
+        column: "done",
+        steps: [{ name: "Step 1", status: "done" }],
+        mergeDetails: { commitSha: "abc123" },
+      });
+
+      render(<TaskCard task={task} onOpenDetail={vi.fn()} addToast={vi.fn()} />);
+
+      const callArgs = mockUseTaskDiffStats.mock.calls[0];
+      const options = callArgs[4] as Record<string, unknown>;
+
+      // done tasks should not have stepVersion or pollIntervalMs
+      expect(options.stepVersion).toBeUndefined();
+      expect(options.pollIntervalMs).toBeUndefined();
+    });
+
+    it("updates stepVersion when step status changes on in-progress task", () => {
+      const onOpenDetail = vi.fn();
+      const addToast = vi.fn();
+
+      const task1 = createTask({
+        column: "in-progress",
+        steps: [{ name: "Step 1", status: "pending" }],
+        worktree: "/repo/.worktrees/fn-001",
+      });
+
+      const { rerender } = render(
+        <TaskCard task={task1} onOpenDetail={onOpenDetail} addToast={addToast} />,
+      );
+
+      // First call should have the initial stepVersion
+      const firstCallArgs = mockUseTaskDiffStats.mock.calls[0];
+      const firstOptions = firstCallArgs[4] as Record<string, unknown>;
+      expect(firstOptions.stepVersion).toBe("Step 1:pending");
+
+      // Update task with changed step
+      mockUseTaskDiffStats.mockClear();
+      const task2 = createTask({
+        column: "in-progress",
+        steps: [{ name: "Step 1", status: "done" }],
+        worktree: "/repo/.worktrees/fn-001",
+      });
+
+      rerender(<TaskCard task={task2} onOpenDetail={onOpenDetail} addToast={addToast} />);
+
+      // Second call should have updated stepVersion
+      const secondCallArgs = mockUseTaskDiffStats.mock.calls[0];
+      const secondOptions = secondCallArgs[4] as Record<string, unknown>;
+      expect(secondOptions.stepVersion).toBe("Step 1:done");
+    });
+  });
 });
--- a/packages/dashboard/app/hooks/tests/useTaskDiffStats.test.ts
+++ b/packages/dashboard/app/hooks/tests/useTaskDiffStats.test.ts
@@ -1,5 +1,5 @@
 import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
-import { renderHook, waitFor } from "@testing-library/react";
+import { renderHook, waitFor, act } from "@testing-library/react";
 import { useTaskDiffStats, __test_clearDiffStatsCache } from "../useTaskDiffStats";
 import * as api from "../../api";

@@ -451,4 +451,312 @@ describe("useTaskDiffStats", () => {
      expect(mockFetchTaskDiff).toHaveBeenCalled();
    });
  });
+
+  describe("stepVersion", () => {
+    beforeEach(() => {
+      __test_clearDiffStatsCache();
+      mockFetchTaskDiff.mockClear();
+    });
+
+    it("re-fetches when stepVersion changes", async () => {
+      // Initial fetch
+      mockFetchTaskDiff.mockResolvedValueOnce({
+        files: [],
+        stats: { filesChanged: 1, additions: 5, deletions: 1 },
+      });
+
+      const { result, rerender } = renderHook(
+        ({ stepVersion }) => useTaskDiffStats(
+          "FN-STEP",
+          "done",
+          "abc1234",
+          undefined,
+          { stepVersion },
+        ),
+        { initialProps: { stepVersion: 1 as number | string } },
+      );
+
+      await waitFor(() => expect(result.current.loading).toBe(false));
+      expect(result.current.stats).toEqual({ filesChanged: 1, additions: 5, deletions: 1 });
+      expect(mockFetchTaskDiff).toHaveBeenCalledTimes(1);
+
+      // Change stepVersion - should trigger re-fetch
+      mockFetchTaskDiff.mockResolvedValueOnce({
+        files: [],
+        stats: { filesChanged: 3, additions: 10, deletions: 2 },
+      });
+
+      rerender({ stepVersion: 2 as number | string });
+
+      await waitFor(() => expect(result.current.loading).toBe(false));
+      expect(result.current.stats).toEqual({ filesChanged: 3, additions: 10, deletions: 2 });
+      expect(mockFetchTaskDiff).toHaveBeenCalledTimes(2);
+    });
+
+    it("caches stats separately per stepVersion", async () => {
+      // Initial fetch with stepVersion 1
+      mockFetchTaskDiff.mockResolvedValueOnce({
+        files: [],
+        stats: { filesChanged: 5, additions: 20, deletions: 3 },
+      });
+
+      const { result: first } = renderHook(() =>
+        useTaskDiffStats("FN-STEP-CACHE", "done", "abc1234", undefined, { stepVersion: "v1" }),
+      );
+
+      await waitFor(() => expect(first.current.loading).toBe(false));
+      expect(first.current.stats).toEqual({ filesChanged: 5, additions: 20, deletions: 3 });
+
+      // Same task, different stepVersion - should fetch separately
+      mockFetchTaskDiff.mockResolvedValueOnce({
+        files: [],
+        stats: { filesChanged: 10, additions: 50, deletions: 8 },
+      });
+
+      const { result: second } = renderHook(() =>
+        useTaskDiffStats("FN-STEP-CACHE", "done", "abc1234", undefined, { stepVersion: "v2" }),
+      );
+
+      await waitFor(() => expect(second.current.loading).toBe(false));
+      expect(second.current.stats).toEqual({ filesChanged: 10, additions: 50, deletions: 8 });
+
+      // Both should have been fetched
+      expect(mockFetchTaskDiff).toHaveBeenCalledTimes(2);
+
+      // Cache should have both entries
+      mockFetchTaskDiff.mockClear();
+
+      const { result: cached1 } = renderHook(() =>
+        useTaskDiffStats("FN-STEP-CACHE", "done", "abc1234", undefined, { stepVersion: "v1" }),
+      );
+      const { result: cached2 } = renderHook(() =>
+        useTaskDiffStats("FN-STEP-CACHE", "done", "abc1234", undefined, { stepVersion: "v2" }),
+      );
+
+      expect(cached1.current.stats).toEqual({ filesChanged: 5, additions: 20, deletions: 3 });
+      expect(cached2.current.stats).toEqual({ filesChanged: 10, additions: 50, deletions: 8 });
+      expect(mockFetchTaskDiff).not.toHaveBeenCalled();
+    });
+  });
+
+  describe("polling", () => {
+    beforeEach(() => {
+      vi.useFakeTimers();
+      __test_clearDiffStatsCache();
+      mockFetchTaskDiff.mockClear();
+    });
+
+    afterEach(() => {
+      vi.useRealTimers();
+    });
+
+    it("sets up polling interval for in-progress column", async () => {
+      mockFetchTaskDiff.mockResolvedValue({
+        files: [],
+        stats: { filesChanged: 1, additions: 5, deletions: 1 },
+      });
+
+      const { result } = renderHook(() =>
+        useTaskDiffStats(
+          "FN-POLL-IP",
+          "in-progress",
+          undefined,
+          undefined,
+          { worktree: "/repo/.worktrees/fn-poll-ip", pollIntervalMs: 30_000 },
+        ),
+      );
+
+      // Initial fetch
+      await act(async () => {
+        vi.advanceTimersByTime(1);
+      });
+      expect(result.current.loading).toBe(false);
+      expect(result.current.stats).toEqual({ filesChanged: 1, additions: 5, deletions: 1 });
+      expect(mockFetchTaskDiff).toHaveBeenCalledTimes(1);
+
+      // Advance timer by 30 seconds (poll interval)
+      await act(async () => {
+        vi.advanceTimersByTime(30_000);
+      });
+      expect(mockFetchTaskDiff).toHaveBeenCalledTimes(2);
+
+      // Advance timer again
+      await act(async () => {
+        vi.advanceTimersByTime(30_000);
+      });
+      expect(mockFetchTaskDiff).toHaveBeenCalledTimes(3);
+    });
+
+    it("sets up polling interval for in-review column", async () => {
+      mockFetchTaskDiff.mockResolvedValue({
+        files: [],
+        stats: { filesChanged: 2, additions: 10, deletions: 2 },
+      });
+
+      const { result } = renderHook(() =>
+        useTaskDiffStats(
+          "FN-POLL-IR",
+          "in-review",
+          undefined,
+          undefined,
+          { worktree: "/repo/.worktrees/fn-poll-ir", pollIntervalMs: 30_000 },
+        ),
+      );
+
+      // Initial fetch
+      await act(async () => {
+        vi.advanceTimersByTime(1);
+      });
+      expect(result.current.loading).toBe(false);
+      expect(result.current.stats).toEqual({ filesChanged: 2, additions: 10, deletions: 2 });
+      expect(mockFetchTaskDiff).toHaveBeenCalledTimes(1);
+
+      // Advance timer by 30 seconds
+      await act(async () => {
+        vi.advanceTimersByTime(30_000);
+      });
+      expect(mockFetchTaskDiff).toHaveBeenCalledTimes(2);
+    });
+
+    it("does not poll for done column", async () => {
+      mockFetchTaskDiff.mockResolvedValue({
+        files: [],
+        stats: { filesChanged: 3, additions: 15, deletions: 3 },
+      });
+
+      const { result } = renderHook(() =>
+        useTaskDiffStats(
+          "FN-POLL-DONE",
+          "done",
+          "abc1234",
+          undefined,
+          { pollIntervalMs: 30_000 },
+        ),
+      );
+
+      // Initial fetch
+      await act(async () => {
+        vi.advanceTimersByTime(1);
+      });
+      expect(result.current.loading).toBe(false);
+      expect(result.current.stats).toEqual({ filesChanged: 3, additions: 15, deletions: 3 });
+      expect(mockFetchTaskDiff).toHaveBeenCalledTimes(1);
+
+      // Advance timer significantly (would trigger poll if done column was active)
+      await act(async () => {
+        vi.advanceTimersByTime(60_000);
+      });
+
+      // Should NOT have refetched for done column
+      expect(mockFetchTaskDiff).toHaveBeenCalledTimes(1);
+    });
+
+    it("cleans up interval on unmount", async () => {
+      mockFetchTaskDiff.mockResolvedValue({
+        files: [],
+        stats: { filesChanged: 1, additions: 5, deletions: 1 },
+      });
+
+      const { result, unmount } = renderHook(() =>
+        useTaskDiffStats(
+          "FN-POLL-UNMOUNT",
+          "in-progress",
+          undefined,
+          undefined,
+          { worktree: "/repo/.worktrees/fn-poll-unmount", pollIntervalMs: 30_000 },
+        ),
+      );
+
+      // Initial fetch
+      await act(async () => {
+        vi.advanceTimersByTime(1);
+      });
+      expect(result.current.loading).toBe(false);
+      expect(mockFetchTaskDiff).toHaveBeenCalledTimes(1);
+
+      // Unmount the hook (this should clear the interval)
+      unmount();
+
+      // Advance timer past the poll interval
+      await act(async () => {
+        vi.advanceTimersByTime(60_000);
+      });
+
+      // No additional fetches should have occurred after unmount
+      expect(mockFetchTaskDiff).toHaveBeenCalledTimes(1);
+    });
+
+    it("forceRefresh bypasses cache on poll", async () => {
+      // Pre-populate cache with a value
+      mockFetchTaskDiff.mockResolvedValueOnce({
+        files: [],
+        stats: { filesChanged: 5, additions: 25, deletions: 5 },
+      });
+
+      const { result } = renderHook(() =>
+        useTaskDiffStats(
+          "FN-POLL-FORCE",
+          "in-progress",
+          undefined,
+          undefined,
+          { worktree: "/repo/.worktrees/fn-poll-force", pollIntervalMs: 30_000 },
+        ),
+      );
+
+      await act(async () => {
+        vi.advanceTimersByTime(1);
+      });
+      expect(result.current.stats).toEqual({ filesChanged: 5, additions: 25, deletions: 5 });
+      expect(mockFetchTaskDiff).toHaveBeenCalledTimes(1);
+
+      // Update mock to return different value
+      mockFetchTaskDiff.mockResolvedValueOnce({
+        files: [],
+        stats: { filesChanged: 10, additions: 50, deletions: 10 },
+      });
+
+      // Advance timer - should force refresh and bypass cache
+      await act(async () => {
+        vi.advanceTimersByTime(30_000);
+      });
+
+      expect(result.current.stats).toEqual({ filesChanged: 10, additions: 50, deletions: 10 });
+      expect(mockFetchTaskDiff).toHaveBeenCalledTimes(2);
+    });
+
+    it("polls with custom interval", async () => {
+      mockFetchTaskDiff.mockResolvedValue({
+        files: [],
+        stats: { filesChanged: 1, additions: 5, deletions: 1 },
+      });
+
+      const { result } = renderHook(() =>
+        useTaskDiffStats(
+          "FN-POLL-CUSTOM",
+          "in-progress",
+          undefined,
+          undefined,
+          { worktree: "/repo/.worktrees/fn-poll-custom", pollIntervalMs: 10_000 },
+        ),
+      );
+
+      // Initial fetch
+      await act(async () => {
+        vi.advanceTimersByTime(1);
+      });
+      expect(mockFetchTaskDiff).toHaveBeenCalledTimes(1);
+
+      // Advance by 10 seconds (custom interval)
+      await act(async () => {
+        vi.advanceTimersByTime(10_000);
+      });
+      expect(mockFetchTaskDiff).toHaveBeenCalledTimes(2);
+
+      // Advance by another 10 seconds
+      await act(async () => {
+        vi.advanceTimersByTime(10_000);
+      });
+      expect(mockFetchTaskDiff).toHaveBeenCalledTimes(3);
+    });
+  });
 });
--- a/packages/dashboard/app/hooks/useTaskDiffStats.ts
+++ b/packages/dashboard/app/hooks/useTaskDiffStats.ts
@@ -17,6 +17,10 @@ interface UseTaskDiffStatsOptions {
  enabled?: boolean;
  /** Worktree path for active task columns. */
  worktree?: string;
+  /** Version identifier that changes when steps update. Forces cache invalidation when changed. */
+  stepVersion?: number | string;
+  /** Poll interval in ms for active columns (in-progress, in-review). Forces re-fetch bypassing cache. */
+  pollIntervalMs?: number;
 }

 /**
@@ -27,12 +31,12 @@ interface UseTaskDiffStatsOptions {
 const diffStatsCache = new Map<string, { stats: DiffStats; expiresAt: number }>();
 const CACHE_TTL_MS = 30_000; // 30 seconds

-function getCacheKey(taskId: string, projectId?: string, worktree?: string): string {
-  return `${taskId}:${projectId ?? ""}:${worktree ?? ""}`;
+function getCacheKey(taskId: string, projectId?: string, worktree?: string, stepVersion?: string): string {
+  return `${taskId}:${projectId ?? ""}:${worktree ?? ""}:${stepVersion ?? ""}`;
 }

-function getCachedStats(taskId: string, projectId?: string, worktree?: string): DiffStats | null {
-  const key = getCacheKey(taskId, projectId, worktree);
+function getCachedStats(taskId: string, projectId?: string, worktree?: string, stepVersion?: string): DiffStats | null {
+  const key = getCacheKey(taskId, projectId, worktree, stepVersion);
  const entry = diffStatsCache.get(key);

  if (!entry) return null;
@@ -46,8 +50,8 @@ function getCachedStats(taskId: string, projectId?: string, worktree?: string):
  return entry.stats;
 }

-function setCachedStats(taskId: string, projectId: string | undefined, worktree: string | undefined, stats: DiffStats): void {
-  const key = getCacheKey(taskId, projectId, worktree);
+function setCachedStats(taskId: string, projectId: string | undefined, worktree: string | undefined, stepVersion: string | undefined, stats: DiffStats): void {
+  const key = getCacheKey(taskId, projectId, worktree, stepVersion);
  diffStatsCache.set(key, {
    stats,
    expiresAt: Date.now() + CACHE_TTL_MS,
@@ -85,6 +89,8 @@ export function useTaskDiffStats(
 ): UseTaskDiffStatsResult {
  const enabled = options.enabled ?? true;
  const worktree = options.worktree;
+  const stepVersion = options.stepVersion;
+  const pollIntervalMs = options.pollIntervalMs;
  const [stats, setStats] = useState<DiffStats | null>(null);
  const [loading, setLoading] = useState(false);

@@ -105,25 +111,30 @@ export function useTaskDiffStats(
      return;
    }

-    // Check cache first - return immediately without loading flicker
    const activeWorktree = shouldFetchActiveTask ? worktree : undefined;
-    const cached = getCachedStats(taskId, projectId, activeWorktree);
-    if (cached) {
-      setStats(cached);
-      setLoading(false);
-      return;
-    }
-
+    const stepVersionStr = stepVersion !== undefined ? String(stepVersion) : undefined;
    let cancelled = false;

-    async function load() {
+    async function load(forceRefresh = false) {
+      // Check cache first - return immediately without loading flicker (unless force refresh)
+      if (!forceRefresh) {
+        const cached = getCachedStats(taskId, projectId, activeWorktree, stepVersionStr);
+        if (cached) {
+          if (!cancelled) {
+            setStats(cached);
+            setLoading(false);
+          }
+          return;
+        }
+      }
+
      setLoading(true);
      try {
        const data = await fetchTaskDiff(taskId, activeWorktree, projectId);
        if (!cancelled) {
          setStats(data.stats);
          // Store in cache
-          setCachedStats(taskId, projectId, activeWorktree, data.stats);
+          setCachedStats(taskId, projectId, activeWorktree, stepVersionStr, data.stats);
        }
      } catch {
        if (!cancelled) {
@@ -136,12 +147,25 @@ export function useTaskDiffStats(
      }
    }

+    // Initial fetch
    void load();

+    // Set up polling for active columns
+    let timer: ReturnType<typeof setInterval> | undefined;
+    if (pollIntervalMs && shouldFetchActiveTask) {
+      timer = setInterval(() => {
+        // Force refresh on poll - bypass cache
+        void load(true);
+      }, pollIntervalMs);
+    }
+
    return () => {
      cancelled = true;
+      if (timer) {
+        clearInterval(timer);
+      }
    };
-  }, [taskId, column, commitSha, projectId, enabled, worktree]);
+  }, [taskId, column, commitSha, projectId, enabled, worktree, stepVersion, pollIntervalMs]);

  return { stats, loading };
 }