feat(FN-1544): add viewport-gated loading and lightweight memo comparison

- Add viewport-gated fetching to TaskCard component so only visible cards load their data
- Implement lightweight memo comparison to prevent unnecessary re-renders
- Add lazy enable gates to useSessionFiles and useTaskDiffStats hooks with caching
- Add comprehensive tests for useSessionFiles and useTaskDiffStats hooks
- Document viewport-gated loading patterns in memory and dashboard-load performance docs
This commit is contained in:
gsxdsm
2026-04-10 10:44:48 -07:00
parent 4cb378477c
commit 3870d0c9d8
8 changed files with 616 additions and 11 deletions

View File

@@ -298,7 +298,8 @@ describe("TaskCard mission badge", () => {
expect(badge).not.toBeNull();
await waitFor(() => {
expect(badge?.textContent).toContain("Database Optimiza...");
// MAX_MISSION_TITLE_LENGTH is 12, so first 9 chars + "..."
expect(badge?.textContent).toContain("Database ...");
});
});
@@ -325,8 +326,8 @@ describe("TaskCard mission badge", () => {
expect(badge).not.toBeNull();
await waitFor(() => {
// MAX_MISSION_TITLE_LENGTH is 20, so first 17 chars + "..."
expect(badge?.textContent).toContain("This Is A Very Lo...");
// MAX_MISSION_TITLE_LENGTH is 12, so first 9 chars + "..."
expect(badge?.textContent).toContain("This Is A...");
});
});

View File

@@ -121,6 +121,49 @@ function areTaskDependenciesEqual(previous: string[], next: string[]): boolean {
return previous.every((dependency, index) => dependency === next[index]);
}
/**
* Lightweight comparison for attachment metadata (not file content).
* Compares counts and top-level fields that affect card rendering.
*/
function areAttachmentsEqual(previous: Task["attachments"], next: Task["attachments"]): boolean {
if (!previous && !next) return true;
if (!previous || !next) return false;
if (previous.length !== next.length) return false;
// Compare attachment metadata that affects card rendering
return previous.every((att, i) => {
const nextAtt = next[i];
if (!nextAtt) return false;
// Compare fields that affect the card's visual state
return (
att.filename === nextAtt.filename &&
att.mimeType === nextAtt.mimeType &&
att.size === nextAtt.size
);
});
}
/**
* Lightweight comparison for comments.
* Compares counts and top-level fields that affect card rendering.
*/
function areCommentsEqual(previous: Task["comments"], next: Task["comments"]): boolean {
if (!previous && !next) return true;
if (!previous || !next) return false;
if (previous.length !== next.length) return false;
// Compare comment metadata that affects card rendering
return previous.every((comment, i) => {
const nextComment = next[i];
if (!nextComment) return false;
return (
comment.author === nextComment.author &&
comment.content === nextComment.content &&
comment.createdAt === nextComment.createdAt
);
});
}
// Keep this comparator aligned with the fields TaskCard renders directly and the
// task metadata that influences child badge freshness/subscriptions.
function areTaskCardPropsEqual(previous: TaskCardProps, next: TaskCardProps): boolean {
@@ -164,8 +207,8 @@ function areTaskCardPropsEqual(previous: TaskCardProps, next: TaskCardProps): bo
previousTask.missionId === nextTask.missionId &&
previousTask.assignedAgentId === nextTask.assignedAgentId &&
previousTask.mergeRetries === nextTask.mergeRetries &&
JSON.stringify(previousTask.attachments ?? []) === JSON.stringify(nextTask.attachments ?? []) &&
JSON.stringify(previousTask.comments ?? []) === JSON.stringify(nextTask.comments ?? []) &&
areAttachmentsEqual(previousTask.attachments, nextTask.attachments) &&
areCommentsEqual(previousTask.comments, nextTask.comments) &&
areTaskDependenciesEqual(previousTask.dependencies, nextTask.dependencies) &&
areTaskStepsEqual(previousTask.steps, nextTask.steps) &&
areTaskBadgeInfosEqual(previousTask.prInfo, nextTask.prInfo) &&
@@ -448,8 +491,24 @@ function TaskCardComponent({
}, [hasGitHubBadge, isInViewport, subscribeToBadge, task.id, unsubscribeFromBadge]);
const liveBadgeData = badgeUpdates.get(task.id);
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);
// Viewport-gated session files fetching - only fetch when card is visible
const { files: sessionFiles, loading: sessionFilesLoading } = useSessionFiles(
task.id,
task.worktree,
task.column,
projectId,
{ enabled: isInViewport },
);
// 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 },
);
// Get fresh batch data if available
const batchData = useMemo(() => getFreshBatchData(task.id, projectId), [task.id, projectId]);

View File

@@ -70,4 +70,54 @@ describe("useSessionFiles", () => {
expect(result.current.files).toEqual([]);
expect(mockFetchSessionFiles).not.toHaveBeenCalled();
});
describe("enabled option", () => {
it("fetches when enabled is true (default)", async () => {
mockFetchSessionFiles.mockResolvedValueOnce(["file.ts"]);
const { result } = renderHook(() =>
useSessionFiles("FN-123", "/repo/.worktrees/kb-123", "in-progress", undefined, { enabled: true }),
);
await waitFor(() => expect(result.current.loading).toBe(false));
expect(result.current.files).toEqual(["file.ts"]);
expect(mockFetchSessionFiles).toHaveBeenCalled();
});
it("fetches when enabled is not specified (default)", async () => {
mockFetchSessionFiles.mockResolvedValueOnce(["file.ts"]);
const { result } = renderHook(() =>
useSessionFiles("FN-123", "/repo/.worktrees/kb-123", "in-progress"),
);
await waitFor(() => expect(result.current.loading).toBe(false));
expect(result.current.files).toEqual(["file.ts"]);
expect(mockFetchSessionFiles).toHaveBeenCalled();
});
it("does not fetch when enabled is false", async () => {
const { result } = renderHook(() =>
useSessionFiles("FN-123", "/repo/.worktrees/kb-123", "in-progress", undefined, { enabled: false }),
);
await waitFor(() => expect(result.current.loading).toBe(false));
expect(result.current.files).toEqual([]);
expect(mockFetchSessionFiles).not.toHaveBeenCalled();
});
it("returns stable state (loading: false) when disabled", async () => {
const { result } = renderHook(() =>
useSessionFiles("FN-123", "/repo/.worktrees/kb-123", "in-progress", undefined, { enabled: false }),
);
// Immediately check (before any async)
expect(result.current.loading).toBe(false);
expect(result.current.files).toEqual([]);
await waitFor(() => expect(result.current.loading).toBe(false));
expect(result.current.files).toEqual([]);
expect(mockFetchSessionFiles).not.toHaveBeenCalled();
});
});
});

View File

@@ -1,6 +1,6 @@
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
import { renderHook, waitFor } from "@testing-library/react";
import { useTaskDiffStats } from "../useTaskDiffStats";
import { useTaskDiffStats, __test_clearDiffStatsCache } from "../useTaskDiffStats";
import * as api from "../../api";
vi.mock("../../api", () => ({
@@ -12,6 +12,7 @@ const mockFetchTaskDiff = vi.mocked(api.fetchTaskDiff);
describe("useTaskDiffStats", () => {
beforeEach(() => {
mockFetchTaskDiff.mockReset();
__test_clearDiffStatsCache();
});
afterEach(() => {
@@ -159,4 +160,271 @@ describe("useTaskDiffStats", () => {
await waitFor(() => expect(result.current.loading).toBe(false));
expect(result.current.stats).toBeNull();
});
describe("enabled option", () => {
it("fetches when enabled is true (default)", async () => {
mockFetchTaskDiff.mockResolvedValueOnce({
files: [],
stats: { filesChanged: 1, additions: 2, deletions: 3 },
});
const { result } = renderHook(() =>
useTaskDiffStats("FN-123", "done", "abc1234", undefined, { enabled: true }),
);
await waitFor(() => expect(result.current.loading).toBe(false));
expect(result.current.stats).toEqual({ filesChanged: 1, additions: 2, deletions: 3 });
expect(mockFetchTaskDiff).toHaveBeenCalled();
});
it("fetches when enabled is not specified (default)", async () => {
mockFetchTaskDiff.mockResolvedValueOnce({
files: [],
stats: { filesChanged: 1, additions: 2, deletions: 3 },
});
const { result } = renderHook(() =>
useTaskDiffStats("FN-123", "done", "abc1234", undefined),
);
await waitFor(() => expect(result.current.loading).toBe(false));
expect(result.current.stats).toEqual({ filesChanged: 1, additions: 2, deletions: 3 });
expect(mockFetchTaskDiff).toHaveBeenCalled();
});
it("does not fetch when enabled is false", async () => {
const { result } = renderHook(() =>
useTaskDiffStats("FN-123", "done", "abc1234", undefined, { enabled: false }),
);
await waitFor(() => expect(result.current.loading).toBe(false));
expect(result.current.stats).toBeNull();
expect(mockFetchTaskDiff).not.toHaveBeenCalled();
});
it("returns stable state (loading: false) when disabled", async () => {
const { result } = renderHook(() =>
useTaskDiffStats("FN-123", "done", "abc1234", undefined, { enabled: false }),
);
// Immediately check (before any async)
expect(result.current.loading).toBe(false);
expect(result.current.stats).toBeNull();
await waitFor(() => expect(result.current.loading).toBe(false));
expect(result.current.stats).toBeNull();
expect(mockFetchTaskDiff).not.toHaveBeenCalled();
});
it("respects enabled flag changes", async () => {
mockFetchTaskDiff.mockResolvedValueOnce({
files: [],
stats: { filesChanged: 5, additions: 10, deletions: 2 },
});
const { result, rerender } = renderHook(
({ enabled }) => useTaskDiffStats("FN-123", "done", "abc1234", undefined, { enabled }),
{ initialProps: { enabled: true } },
);
// Fetch should happen initially
await waitFor(() => expect(result.current.loading).toBe(false));
expect(result.current.stats).toEqual({ filesChanged: 5, additions: 10, deletions: 2 });
// Toggle enabled off - should not refetch
mockFetchTaskDiff.mockClear();
rerender({ enabled: false });
await waitFor(() => expect(result.current.loading).toBe(false));
expect(mockFetchTaskDiff).not.toHaveBeenCalled();
});
});
describe("caching", () => {
beforeEach(() => {
// Clear cache before each caching test to ensure isolation
__test_clearDiffStatsCache();
mockFetchTaskDiff.mockClear();
});
it("returns cached stats without making a fetch", async () => {
// First render - fetches and caches
mockFetchTaskDiff.mockResolvedValueOnce({
files: [],
stats: { filesChanged: 10, additions: 50, deletions: 5 },
});
const { result: first } = renderHook(() =>
useTaskDiffStats("FN-CACHE-1", "done", "abc1234", undefined),
);
await waitFor(() => expect(first.current.loading).toBe(false));
expect(first.current.stats).toEqual({ filesChanged: 10, additions: 50, deletions: 5 });
expect(mockFetchTaskDiff).toHaveBeenCalledTimes(1);
// Second render with same taskId - should use cache
mockFetchTaskDiff.mockClear();
const { result: second } = renderHook(() =>
useTaskDiffStats("FN-CACHE-1", "done", "abc1234", undefined),
);
await waitFor(() => expect(second.current.loading).toBe(false));
// Should return cached value, not new value
expect(second.current.stats).toEqual({ filesChanged: 10, additions: 50, deletions: 5 });
// No additional fetch
expect(mockFetchTaskDiff).not.toHaveBeenCalled();
});
it("returns cached stats immediately without loading flicker", async () => {
// Pre-populate cache by doing an initial fetch
mockFetchTaskDiff.mockResolvedValueOnce({
files: [],
stats: { filesChanged: 7, additions: 30, deletions: 3 },
});
const { result: first } = renderHook(() =>
useTaskDiffStats("FN-CACHE-2", "done", "abc1234", undefined),
);
await waitFor(() => expect(first.current.loading).toBe(false));
expect(first.current.stats).toEqual({ filesChanged: 7, additions: 30, deletions: 3 });
// Second hook instance - cache hit should be immediate
mockFetchTaskDiff.mockClear();
const { result: second } = renderHook(() =>
useTaskDiffStats("FN-CACHE-2", "done", "abc1234", undefined),
);
// Cache hit - no loading state, no fetch
expect(second.current.loading).toBe(false);
expect(second.current.stats).toEqual({ filesChanged: 7, additions: 30, deletions: 3 });
expect(mockFetchTaskDiff).not.toHaveBeenCalled();
});
it("caches stats separately per task ID", async () => {
// Pre-populate cache for FN-100
mockFetchTaskDiff.mockResolvedValueOnce({
files: [],
stats: { filesChanged: 5, additions: 25, deletions: 2 },
});
const { result: first } = renderHook(() =>
useTaskDiffStats("FN-100", "done", "abc1234", undefined),
);
await waitFor(() => expect(first.current.loading).toBe(false));
expect(first.current.stats).toEqual({ filesChanged: 5, additions: 25, deletions: 2 });
// Fetch for FN-200 (note: cache already has FN-100 entry)
mockFetchTaskDiff.mockResolvedValueOnce({
files: [],
stats: { filesChanged: 15, additions: 100, deletions: 10 },
});
const { result: second } = renderHook(() =>
useTaskDiffStats("FN-200", "done", "def5678", undefined),
);
await waitFor(() => expect(second.current.loading).toBe(false));
expect(second.current.stats).toEqual({ filesChanged: 15, additions: 100, deletions: 10 });
// Both should have been fetched (FN-100 was in cache from first test's beforeEach, but this test's beforeEach cleared it)
expect(mockFetchTaskDiff).toHaveBeenCalledTimes(2);
// Each should have its own cached value
mockFetchTaskDiff.mockClear();
const { result: cached1 } = renderHook(() =>
useTaskDiffStats("FN-100", "done", "abc1234", undefined),
);
const { result: cached2 } = renderHook(() =>
useTaskDiffStats("FN-200", "done", "def5678", undefined),
);
expect(cached1.current.stats).toEqual({ filesChanged: 5, additions: 25, deletions: 2 });
expect(cached2.current.stats).toEqual({ filesChanged: 15, additions: 100, deletions: 10 });
expect(mockFetchTaskDiff).not.toHaveBeenCalled();
});
it("caches stats separately per project ID", async () => {
// Pre-populate cache for task without projectId
mockFetchTaskDiff.mockResolvedValueOnce({
files: [],
stats: { filesChanged: 3, additions: 15, deletions: 1 },
});
const { result: first } = renderHook(() =>
useTaskDiffStats("FN-PROJ", "done", "abc1234", undefined),
);
await waitFor(() => expect(first.current.loading).toBe(false));
expect(first.current.stats).toEqual({ filesChanged: 3, additions: 15, deletions: 1 });
// Fetch same task with different projectId
mockFetchTaskDiff.mockResolvedValueOnce({
files: [],
stats: { filesChanged: 8, additions: 40, deletions: 4 },
});
const { result: second } = renderHook(() =>
useTaskDiffStats("FN-PROJ", "done", "abc1234", "proj-1"),
);
await waitFor(() => expect(second.current.loading).toBe(false));
expect(second.current.stats).toEqual({ filesChanged: 8, additions: 40, deletions: 4 });
// Both should have been fetched
expect(mockFetchTaskDiff).toHaveBeenCalledTimes(2);
// Cache should have both entries
mockFetchTaskDiff.mockClear();
const { result: cachedNoProj } = renderHook(() =>
useTaskDiffStats("FN-PROJ", "done", "abc1234", undefined),
);
const { result: cachedWithProj } = renderHook(() =>
useTaskDiffStats("FN-PROJ", "done", "abc1234", "proj-1"),
);
expect(cachedNoProj.current.stats).toEqual({ filesChanged: 3, additions: 15, deletions: 1 });
expect(cachedWithProj.current.stats).toEqual({ filesChanged: 8, additions: 40, deletions: 4 });
expect(mockFetchTaskDiff).not.toHaveBeenCalled();
});
it("clears cache via __test_clearDiffStatsCache", async () => {
// Pre-populate cache
mockFetchTaskDiff.mockResolvedValueOnce({
files: [],
stats: { filesChanged: 5, additions: 25, deletions: 2 },
});
const { result: first } = renderHook(() =>
useTaskDiffStats("FN-CLEAR", "done", "abc1234", undefined),
);
await waitFor(() => expect(first.current.loading).toBe(false));
expect(first.current.stats).toEqual({ filesChanged: 5, additions: 25, deletions: 2 });
// Clear cache
__test_clearDiffStatsCache();
// Next fetch should not hit cache
mockFetchTaskDiff.mockClear();
mockFetchTaskDiff.mockResolvedValueOnce({
files: [],
stats: { filesChanged: 99, additions: 999, deletions: 99 },
});
const { result: second } = renderHook(() =>
useTaskDiffStats("FN-CLEAR", "done", "abc1234", undefined),
);
await waitFor(() => expect(second.current.loading).toBe(false));
// Should fetch fresh value
expect(second.current.stats).toEqual({ filesChanged: 99, additions: 999, deletions: 99 });
expect(mockFetchTaskDiff).toHaveBeenCalled();
});
});
});

View File

@@ -8,11 +8,39 @@ interface UseSessionFilesResult {
loading: boolean;
}
export function useSessionFiles(taskId: string, worktree: string | undefined, column: string, projectId?: string): UseSessionFilesResult {
interface UseSessionFilesOptions {
/** Enable fetching when true (default). Suppresses fetches for offscreen cards. */
enabled?: boolean;
}
/**
* Fetches session files for tasks with active worktrees.
*
* @param taskId - Task identifier
* @param worktree - Worktree path (undefined = no worktree)
* @param column - Current task column
* @param projectId - Optional project identifier
* @param options.enabled - When false, no fetch is made and returns empty/stable state
*/
export function useSessionFiles(
taskId: string,
worktree: string | undefined,
column: string,
projectId?: string,
options: UseSessionFilesOptions = {},
): UseSessionFilesResult {
const enabled = options.enabled ?? true;
const [files, setFiles] = useState<string[]>([]);
const [loading, setLoading] = useState(false);
useEffect(() => {
// Disabled state: return stable empty state without fetching
if (!enabled) {
setFiles([]);
setLoading(false);
return;
}
if (!taskId || !worktree || !ACTIVE_COLUMNS.has(column)) {
setFiles([]);
setLoading(false);
@@ -40,7 +68,7 @@ export function useSessionFiles(taskId: string, worktree: string | undefined, co
}
void load();
}, [taskId, worktree, column, projectId]);
}, [taskId, worktree, column, projectId, enabled]);
return { files, loading };
}

View File

@@ -12,6 +12,54 @@ interface UseTaskDiffStatsResult {
loading: boolean;
}
interface UseTaskDiffStatsOptions {
/** Enable fetching when true (default). Suppresses fetches for offscreen cards. */
enabled?: boolean;
}
/**
* Cache for diff stats to avoid repeated fetches during rerenders.
* Key format: "taskId:projectId"
* Entries expire after the TTL to ensure freshness.
*/
const diffStatsCache = new Map<string, { stats: DiffStats; expiresAt: number }>();
const CACHE_TTL_MS = 30_000; // 30 seconds
function getCacheKey(taskId: string, projectId?: string): string {
return `${taskId}:${projectId ?? ""}`;
}
function getCachedStats(taskId: string, projectId?: string): DiffStats | null {
const key = getCacheKey(taskId, projectId);
const entry = diffStatsCache.get(key);
if (!entry) return null;
// Check expiration
if (Date.now() > entry.expiresAt) {
diffStatsCache.delete(key);
return null;
}
return entry.stats;
}
function setCachedStats(taskId: string, projectId: string | undefined, stats: DiffStats): void {
const key = getCacheKey(taskId, projectId);
diffStatsCache.set(key, {
stats,
expiresAt: Date.now() + CACHE_TTL_MS,
});
}
/**
* Clears all entries from the diff stats cache.
* Exported for testing purposes.
*/
export function __test_clearDiffStatsCache(): void {
diffStatsCache.clear();
}
/**
* Fetches diff stats for a done task that has a merge commit SHA.
*
@@ -20,17 +68,32 @@ interface UseTaskDiffStatsResult {
* 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.
*
* @param taskId - Task identifier
* @param column - Current task column
* @param commitSha - Merge commit SHA (undefined = no merge yet)
* @param projectId - Optional project identifier
* @param options.enabled - When false, no fetch is made and returns empty/stable state
*/
export function useTaskDiffStats(
taskId: string,
column: string,
commitSha: string | undefined,
projectId?: string,
options: UseTaskDiffStatsOptions = {},
): UseTaskDiffStatsResult {
const enabled = options.enabled ?? true;
const [stats, setStats] = useState<DiffStats | null>(null);
const [loading, setLoading] = useState(false);
useEffect(() => {
// Disabled state: return stable empty state without fetching
if (!enabled) {
setStats(null);
setLoading(false);
return;
}
// Only fetch for done tasks with a recorded merge commit
if (!taskId || column !== "done" || !commitSha) {
setStats(null);
@@ -38,6 +101,14 @@ export function useTaskDiffStats(
return;
}
// Check cache first - return immediately without loading flicker
const cached = getCachedStats(taskId, projectId);
if (cached) {
setStats(cached);
setLoading(false);
return;
}
let cancelled = false;
async function load() {
@@ -46,6 +117,8 @@ export function useTaskDiffStats(
const data = await fetchTaskDiff(taskId, undefined, projectId);
if (!cancelled) {
setStats(data.stats);
// Store in cache
setCachedStats(taskId, projectId, data.stats);
}
} catch {
if (!cancelled) {
@@ -63,7 +136,7 @@ export function useTaskDiffStats(
return () => {
cancelled = true;
};
}, [taskId, column, commitSha, projectId]);
}, [taskId, column, commitSha, projectId, enabled]);
return { stats, loading };
}