diff --git a/src/main/ipc/gitIpc.ts b/src/main/ipc/gitIpc.ts index 9a36a0b75..457ab8f3f 100644 --- a/src/main/ipc/gitIpc.ts +++ b/src/main/ipc/gitIpc.ts @@ -747,19 +747,22 @@ export function registerGitIpc() { // Git: Status (moved from Codex IPC) ipcMain.handle( 'git:get-status', - async (_, arg: string | { taskPath: string; taskId?: string }) => { + async (_, arg: string | { taskPath: string; taskId?: string; includeUntracked?: boolean }) => { const taskPath = typeof arg === 'string' ? arg : arg.taskPath; const taskId = typeof arg === 'string' ? undefined : arg.taskId; + const includeUntracked = typeof arg === 'string' ? true : (arg.includeUntracked ?? true); try { const remote = await resolveRemoteContext(taskPath, taskId); if (remote) { const changes = await remoteGitService.getStatusDetailed( remote.connectionId, - remote.remotePath + remote.remotePath, + { includeUntracked } ); return { success: true, changes }; } - const changes = await gitGetStatus(taskPath); + + const changes = await gitGetStatus(taskPath, { includeUntracked }); return { success: true, changes }; } catch (error) { log.error('git:get-status error', error); diff --git a/src/main/preload.ts b/src/main/preload.ts index f50116e83..1c0c7da70 100644 --- a/src/main/preload.ts +++ b/src/main/preload.ts @@ -383,7 +383,7 @@ contextBridge.exposeInMainWorld('electronAPI', { fetchProjectBaseRef: (args: { projectId: string; projectPath: string }) => ipcRenderer.invoke('projectSettings:fetchBaseRef', args), getGitInfo: (projectPath: string) => ipcRenderer.invoke('git:getInfo', projectPath), - getGitStatus: (arg: string | { taskPath: string; taskId?: string }) => + getGitStatus: (arg: string | { taskPath: string; taskId?: string; includeUntracked?: boolean }) => ipcRenderer.invoke('git:get-status', arg), getDeleteRisks: (args: { targets: Array<{ id: string; taskPath: string }>; diff --git a/src/main/services/GitService.ts b/src/main/services/GitService.ts index c06e39a08..05986c2dd 100644 --- a/src/main/services/GitService.ts +++ b/src/main/services/GitService.ts @@ -144,7 +144,18 @@ async function resolveReviewBaseRef(taskPath: string, baseRef: string): Promise< return baseRef; } -export async function getStatus(taskPath: string): Promise { +/** + * Returns the list of changed files in the worktree. + * + * @param taskPath Absolute path to the worktree. + * @param [options.includeUntracked=true] Whether to include untracked files. + * Disabling this avoids a full directory walk which can be slow in large repos. + */ +export async function getStatus( + taskPath: string, + options?: { includeUntracked?: boolean } +): Promise { + const includeUntracked = options?.includeUntracked ?? true; try { try { await execFileAsync('git', ['rev-parse', '--is-inside-work-tree'], { @@ -157,6 +168,9 @@ export async function getStatus(taskPath: string): Promise { // Run git commands in parallel with flags tuned for performance: // --no-optional-locks: avoid blocking on concurrent git processes // --no-ahead-behind: skip commit-graph walk for tracking info + // --untracked-files: 'no' for fast pass (~50ms), 'all' for full pass (~8s+) + // The untracked directory walk is the main bottleneck in large monorepos. + const untrackedMode = includeUntracked ? 'all' : 'no'; const statusPromise = (async () => { try { const { stdout } = await execFileAsync( @@ -167,7 +181,7 @@ export async function getStatus(taskPath: string): Promise { '--porcelain=v2', '-z', '--no-ahead-behind', - '--untracked-files=all', + `--untracked-files=${untrackedMode}`, ], { cwd: taskPath, @@ -176,10 +190,9 @@ export async function getStatus(taskPath: string): Promise { ); return stdout; } catch { - // Fallback for older git versions that do not support porcelain v2. const { stdout } = await execFileAsync( 'git', - ['--no-optional-locks', 'status', '--porcelain', '--untracked-files=all'], + ['--no-optional-locks', 'status', '--porcelain', `--untracked-files=${untrackedMode}`], { cwd: taskPath, maxBuffer: MAX_DIFF_OUTPUT_BYTES, diff --git a/src/main/services/RemoteGitService.ts b/src/main/services/RemoteGitService.ts index 623194df7..40b1ef843 100644 --- a/src/main/services/RemoteGitService.ts +++ b/src/main/services/RemoteGitService.ts @@ -340,7 +340,13 @@ export class RemoteGitService { * Detailed git status matching the shape returned by local GitService.getStatus(). * Parses porcelain output, numstat diffs, and untracked file line counts. */ - async getStatusDetailed(connectionId: string, worktreePath: string): Promise { + async getStatusDetailed( + connectionId: string, + worktreePath: string, + options?: { includeUntracked?: boolean } + ): Promise { + const includeUntracked = options?.includeUntracked ?? true; + const untrackedMode = includeUntracked ? 'all' : 'no'; const cwd = this.normalizeRemotePath(worktreePath); // Verify git repo const verifyResult = await this.sshService.executeCommand( @@ -355,16 +361,15 @@ export class RemoteGitService { let statusOutput = ''; const statusV2Result = await this.sshService.executeCommand( connectionId, - 'git status --porcelain=v2 -z --untracked-files=all', + `git status --porcelain=v2 -z --untracked-files=${untrackedMode}`, cwd ); if (statusV2Result.exitCode === 0) { statusOutput = statusV2Result.stdout || ''; } else { - // Fallback for older remote git versions. const statusV1Result = await this.sshService.executeCommand( connectionId, - 'git status --porcelain --untracked-files=all', + `git status --porcelain --untracked-files=${untrackedMode}`, cwd ); if (statusV1Result.exitCode !== 0) { diff --git a/src/main/services/__tests__/RemoteGitService.test.ts b/src/main/services/__tests__/RemoteGitService.test.ts index cf8c0b749..a7bfdbfac 100644 --- a/src/main/services/__tests__/RemoteGitService.test.ts +++ b/src/main/services/__tests__/RemoteGitService.test.ts @@ -574,6 +574,19 @@ describe('RemoteGitService', () => { expect(result[1].additions).toBe(100); }); + it('should use --untracked-files=no when includeUntracked is false', async () => { + mockExecuteCommand + .mockResolvedValueOnce({ stdout: 'true', stderr: '', exitCode: 0 } as ExecResult) // rev-parse + .mockResolvedValueOnce({ stdout: '', stderr: '', exitCode: 0 } as ExecResult); // status + + await service.getStatusDetailed('conn-1', '/home/user/project', { + includeUntracked: false, + }); + + const statusCall = mockExecuteCommand.mock.calls[1]; + expect(statusCall[1]).toContain('--untracked-files=no'); + }); + it('should handle renamed files', async () => { mockExecuteCommand .mockResolvedValueOnce({ stdout: 'true', stderr: '', exitCode: 0 } as ExecResult) diff --git a/src/renderer/components/FileChangesPanel.tsx b/src/renderer/components/FileChangesPanel.tsx index 2bfdaa3a7..6f6463c13 100644 --- a/src/renderer/components/FileChangesPanel.tsx +++ b/src/renderer/components/FileChangesPanel.tsx @@ -224,7 +224,7 @@ const FileChangesPanelComponent: React.FC = ({ } }; - const { fileChanges, isLoading, refreshChanges } = useFileChanges(safeTaskPath, { + const { fileChanges, isLoading, isRevalidating, refreshChanges } = useFileChanges(safeTaskPath, { taskId: resolvedTaskId, }); const { toast } = useToast(); @@ -664,6 +664,9 @@ const FileChangesPanelComponent: React.FC = ({ -{formatDiffCount(totalChanges.deletions)} + {isRevalidating && ( + + )} {hasStagedChanges && ( diff --git a/src/renderer/components/FileExplorer/CodeEditor.tsx b/src/renderer/components/FileExplorer/CodeEditor.tsx index bb7da6f00..f65e38793 100644 --- a/src/renderer/components/FileExplorer/CodeEditor.tsx +++ b/src/renderer/components/FileExplorer/CodeEditor.tsx @@ -97,7 +97,7 @@ export default function CodeEditor({ } = useFileManager({ taskId, taskPath, connectionId, remotePath }); // Get file changes status from git - const { fileChanges } = useFileChanges(taskPath); + const { fileChanges } = useFileChanges(taskPath, { taskId }); // UI state const [explorerWidth, setExplorerWidth] = useState(EXPLORER_WIDTH.DEFAULT); diff --git a/src/renderer/components/diff-viewer/DiffViewer.tsx b/src/renderer/components/diff-viewer/DiffViewer.tsx index d6bc16789..2c1b1f557 100644 --- a/src/renderer/components/diff-viewer/DiffViewer.tsx +++ b/src/renderer/components/diff-viewer/DiffViewer.tsx @@ -25,7 +25,7 @@ export const DiffViewer: React.FC = ({ const isPrReview = Boolean(prNumber && (taskPath || scopedTaskPath)); const [activeTab, setActiveTab] = useState('changes'); - const { fileChanges: localFileChanges, refreshChanges } = useFileChanges(taskPath); + const { fileChanges: localFileChanges, refreshChanges } = useFileChanges(taskPath, { taskId }); // PR review mode: fetch PR diff changes and base branch const [prFileChanges, setPrFileChanges] = useState([]); diff --git a/src/renderer/hooks/useFileChanges.ts b/src/renderer/hooks/useFileChanges.ts index 32f8ba3a8..149bdfb4f 100644 --- a/src/renderer/hooks/useFileChanges.ts +++ b/src/renderer/hooks/useFileChanges.ts @@ -1,6 +1,12 @@ import { useCallback, useEffect, useRef, useState } from 'react'; import { subscribeToFileChanges } from '@/lib/fileChangeEvents'; -import { getCachedGitStatus } from '@/lib/gitStatusCache'; +import { + buildCacheKey, + getCachedGitStatus, + getCachedResult, + onCacheRevalidated, +} from '@/lib/gitStatusCache'; +import type { GitStatusChange } from '@/lib/gitStatusCache'; import { filterVisibleGitStatusChanges } from '@/lib/gitStatusFilters'; import { useGitStatusAutoRefresh } from '@/hooks/internal/useGitStatusAutoRefresh'; @@ -13,6 +19,17 @@ export interface FileChange { diff?: string; } +function toFileChange(change: GitStatusChange): FileChange { + return { + path: change.path, + status: change.status as 'added' | 'modified' | 'deleted' | 'renamed', + additions: change.additions ?? null, + deletions: change.deletions ?? null, + isStaged: change.isStaged || false, + diff: change.diff, + }; +} + interface UseFileChangesOptions { isActive?: boolean; idleIntervalMs?: number; @@ -30,6 +47,7 @@ export function shouldRefreshFileChanges( export function useFileChanges(taskPath?: string, options: UseFileChangesOptions = {}) { const [fileChanges, setFileChanges] = useState([]); const [isLoading, setIsLoading] = useState(false); + const [isRevalidating, setIsRevalidating] = useState(false); const [error, setError] = useState(null); const [isDocumentVisible, setIsDocumentVisible] = useState(() => { if (typeof document === 'undefined') return true; @@ -38,6 +56,7 @@ export function useFileChanges(taskPath?: string, options: UseFileChangesOptions const { isActive = true, idleIntervalMs = 60000, taskId } = options; const taskPathRef = useRef(taskPath); + const taskIdRef = useRef(taskId); const inFlightRef = useRef(false); const hasLoadedRef = useRef(false); const mountedRef = useRef(true); @@ -52,13 +71,36 @@ export function useFileChanges(taskPath?: string, options: UseFileChangesOptions }, []); useEffect(() => { - setFileChanges([]); // Clear stale state immediately - if (taskPath) { - setIsLoading(true); - } taskPathRef.current = taskPath; + taskIdRef.current = taskId; hasLoadedRef.current = false; - }, [taskPath]); + + setIsRevalidating(false); + + if (!taskPath) { + setFileChanges([]); + return; + } + + const cached = getCachedResult(taskPath, taskId); + if (cached) { + // Show cached data immediately (even if stale) + if (cached.result?.success && cached.result.changes && cached.result.changes.length > 0) { + setFileChanges(filterVisibleGitStatusChanges(cached.result.changes).map(toFileChange)); + } else { + setFileChanges([]); + } + setIsRevalidating(cached.isStale); + } else { + setFileChanges([]); + setIsLoading(true); + } + + // Kick off a fetch immediately so same-path/taskId changes don't wait + // for the idle poll or a watcher event to trigger a load. + void fetchFileChanges(true); + // eslint-disable-next-line react-hooks/exhaustive-deps -- fetchFileChanges is stable (deps: [queueRefresh]) and reads taskPath/taskId from refs + }, [taskPath, taskId]); useEffect(() => { if (typeof document === 'undefined' || typeof window === 'undefined') return; @@ -78,7 +120,7 @@ export function useFileChanges(taskPath?: string, options: UseFileChangesOptions pendingRefreshRef.current = true; if (shouldSetLoading) { pendingInitialLoadRef.current = true; - if (mountedRef.current) { + if (mountedRef.current && !getCachedResult(taskPathRef.current ?? '', taskIdRef.current)) { setIsLoading(true); setError(null); } @@ -98,48 +140,54 @@ export function useFileChanges(taskPath?: string, options: UseFileChangesOptions } inFlightRef.current = true; - if (isInitialLoad && mountedRef.current) { + if (isInitialLoad && mountedRef.current && !getCachedResult(currentPath, taskIdRef.current)) { setIsLoading(true); setError(null); } const requestPath = currentPath; + const requestTaskId = taskIdRef.current; + + const isStale = () => + requestPath !== taskPathRef.current || requestTaskId !== taskIdRef.current; try { - const result = await getCachedGitStatus(requestPath, { force: options?.force, taskId }); + const result = await getCachedGitStatus(requestPath, { + force: options?.force, + taskId: requestTaskId, + }); if (!mountedRef.current) return; - if (requestPath !== taskPathRef.current) { + if (isStale()) { queueRefresh(true); return; } - if (result?.success && result.changes && result.changes.length > 0) { - const visibleChanges = filterVisibleGitStatusChanges(result.changes); - const changes: FileChange[] = visibleChanges.map((change) => ({ - path: change.path, - status: change.status as 'added' | 'modified' | 'deleted' | 'renamed', - additions: change.additions ?? null, - deletions: change.deletions ?? null, - isStaged: change.isStaged || false, - diff: change.diff, - })); - setFileChanges(changes); + if (result?.success) { + setFileChanges( + result.changes?.length + ? filterVisibleGitStatusChanges(result.changes).map(toFileChange) + : [] + ); + setError(null); + } else if (hasLoadedRef.current) { + setError(result?.error || 'Failed to refresh file changes'); } else { setFileChanges([]); + setError(result?.error || 'Failed to load file changes'); } } catch (err) { if (!mountedRef.current) return; - if (requestPath !== taskPathRef.current) { + if (isStale()) { queueRefresh(true); return; } console.error('Failed to fetch file changes:', err); - if (isInitialLoad) { - setError('Failed to load file changes'); + setError('Failed to load file changes'); + if (!hasLoadedRef.current) { + setFileChanges([]); } - setFileChanges([]); } finally { const isCurrentPath = requestPath === taskPathRef.current; if (mountedRef.current && isInitialLoad && !pendingInitialLoadRef.current) { @@ -171,6 +219,35 @@ export function useFileChanges(taskPath?: string, options: UseFileChangesOptions fetchChanges: fetchFileChanges, }); + useEffect(() => { + if (!taskPath) return undefined; + const expectedKey = buildCacheKey(taskPath, taskId); + + const unsubRevalidate = onCacheRevalidated((key, result) => { + if (!mountedRef.current || key !== expectedKey) return; + + setIsLoading(false); + setIsRevalidating(false); + + if (result.success) { + setError(null); + setFileChanges( + result.changes?.length + ? filterVisibleGitStatusChanges(result.changes).map(toFileChange) + : [] + ); + return; + } + + // Preserve existing fileChanges on background refresh failure + setError(result.error || 'Failed to refresh file changes'); + }); + + return () => { + unsubRevalidate(); + }; + }, [taskPath, taskId]); + useEffect(() => { if (!taskPath) return undefined; @@ -193,6 +270,7 @@ export function useFileChanges(taskPath?: string, options: UseFileChangesOptions return { fileChanges, isLoading, + isRevalidating, error, refreshChanges, }; diff --git a/src/renderer/lib/gitStatusCache.ts b/src/renderer/lib/gitStatusCache.ts index 37c958e31..ff9d0381c 100644 --- a/src/renderer/lib/gitStatusCache.ts +++ b/src/renderer/lib/gitStatusCache.ts @@ -20,16 +20,27 @@ const inFlight = new Map const latestRequestId = new Map(); let requestCounter = 0; +type RevalidateListener = (cacheKey: string, result: GitStatusResult) => void; +const revalidateListeners = new Set(); + +/** + * Returns git status for a worktree with caching and stale-while-revalidate. + * + * - Fresh cache (< TTL): returns immediately, no IPC call. + * - Stale cache (>= TTL): returns stale data immediately, starts a background + * refresh that updates the cache and notifies listeners when done. + * - No cache: blocks on the IPC call. + * - force: true: bypasses cache and always makes an IPC call. + */ export async function getCachedGitStatus( taskPath: string, - options?: { force?: boolean; taskId?: string } + options?: { force?: boolean; taskId?: string; includeUntracked?: boolean } ): Promise { if (!taskPath) return { success: false, error: 'workspace-unavailable' }; const force = options?.force ?? false; const taskId = options?.taskId; - // Use a composite key when taskId is present so workspace status - // doesn't collide with local project status for the same taskPath. - const cacheKey = taskId ? `${taskPath}::${taskId}` : taskPath; + const includeUntracked = options?.includeUntracked ?? true; + const cacheKey = buildCacheKey(taskPath, taskId, includeUntracked); const now = Date.now(); if (!force) { @@ -37,22 +48,89 @@ export async function getCachedGitStatus( if (cached && now - cached.timestamp < CACHE_TTL_MS) { return cached.result; } + + // Stale: return immediately, refresh in background. + // Bump timestamp to prevent idle-loop spinning. + if (cached) { + cache.set(cacheKey, { timestamp: now, result: cached.result }); + if (!inFlight.has(cacheKey)) { + startFetch(cacheKey, taskPath, taskId, includeUntracked, true); + } + return cached.result; + } } const existing = inFlight.get(cacheKey); if (!force && existing) return existing.promise; + return startFetch(cacheKey, taskPath, taskId, includeUntracked); +} + +/** + * Subscribe to background cache revalidation completions. + * Called when a stale-while-revalidate background fetch finishes (success or + * failure), allowing the UI to update without polling. + */ +export function onCacheRevalidated(listener: RevalidateListener): () => void { + revalidateListeners.add(listener); + return () => { + revalidateListeners.delete(listener); + }; +} + +function notifyRevalidated(cacheKey: string, result: GitStatusResult) { + for (const listener of revalidateListeners) { + listener(cacheKey, result); + } +} + +export function buildCacheKey( + taskPath: string, + taskId?: string, + includeUntracked?: boolean +): string { + let key = taskId ? `${taskPath}::${taskId}` : taskPath; + if (includeUntracked === false) key += '::tracked'; + return key; +} + +export function getCachedResult( + taskPath: string, + taskId?: string +): { result: GitStatusResult; isStale: boolean } | undefined { + const cacheKey = buildCacheKey(taskPath, taskId); + const entry = cache.get(cacheKey); + if (!entry) return undefined; + return { result: entry.result, isStale: Date.now() - entry.timestamp >= CACHE_TTL_MS }; +} + +/** + * @param notify When true (stale-while-revalidate path), notifies revalidation + * listeners on completion. On failure, preserves the existing cache entry. + */ +function startFetch( + cacheKey: string, + taskPath: string, + taskId?: string, + includeUntracked?: boolean, + notify = false +): Promise { const requestId = (requestCounter += 1); latestRequestId.set(cacheKey, requestId); const promise = (async () => { try { - const res = await window.electronAPI.getGitStatus(taskId ? { taskPath, taskId } : taskPath); + const res = await window.electronAPI.getGitStatus({ + taskPath, + taskId, + includeUntracked, + }); const result = res ?? { success: false, error: 'Failed to load git status', }; if (latestRequestId.get(cacheKey) === requestId) { cache.set(cacheKey, { timestamp: Date.now(), result }); + if (notify) notifyRevalidated(cacheKey, result); } return result; } catch (error) { @@ -61,7 +139,13 @@ export async function getCachedGitStatus( error: error instanceof Error ? error.message : 'Failed to load git status', }; if (latestRequestId.get(cacheKey) === requestId) { - cache.set(cacheKey, { timestamp: Date.now(), result }); + // Never overwrite a known-good cache entry with a failure — + // stale data is more useful than an error on remount. + const existing = cache.get(cacheKey); + if (!existing?.result?.success) { + cache.set(cacheKey, { timestamp: Date.now(), result }); + } + if (notify) notifyRevalidated(cacheKey, result); } return result; } finally { diff --git a/src/renderer/types/electron-api.d.ts b/src/renderer/types/electron-api.d.ts index 2dd571332..c24d84763 100644 --- a/src/renderer/types/electron-api.d.ts +++ b/src/renderer/types/electron-api.d.ts @@ -357,7 +357,9 @@ declare global { rootPath?: string; error?: string; }>; - getGitStatus: (arg: string | { taskPath: string; taskId?: string }) => Promise<{ + getGitStatus: ( + arg: string | { taskPath: string; taskId?: string; includeUntracked?: boolean } + ) => Promise<{ success: boolean; changes?: Array<{ path: string; diff --git a/src/test/main/GitServiceStatus.test.ts b/src/test/main/GitServiceStatus.test.ts index 291654735..f41fccc39 100644 --- a/src/test/main/GitServiceStatus.test.ts +++ b/src/test/main/GitServiceStatus.test.ts @@ -81,4 +81,24 @@ describe('GitService.getStatus', () => { expect(change?.status).toBe('renamed'); expect(change?.isStaged).toBe(true); }); + + it('excludes untracked files when includeUntracked is false', async () => { + await commitFile(repo, 'tracked.txt', 'content\n', 'init'); + + // Create an untracked file + await fs.promises.writeFile(path.join(repo, 'untracked.txt'), 'new file\n'); + // Modify a tracked file + await fs.promises.writeFile(path.join(repo, 'tracked.txt'), 'modified\n'); + + const withUntracked = await getStatus(repo, { includeUntracked: true }); + const withoutUntracked = await getStatus(repo, { includeUntracked: false }); + + expect(withUntracked.some((e) => e.path === 'untracked.txt')).toBe(true); + expect(withUntracked.some((e) => e.path === 'tracked.txt')).toBe(true); + + expect(withoutUntracked.some((e) => e.path === 'untracked.txt')).toBe(false); + const tracked = withoutUntracked.find((e) => e.path === 'tracked.txt'); + expect(tracked).toBeDefined(); + expect(tracked?.status).toBe('modified'); + }); }); diff --git a/src/test/renderer/gitStatusCache.test.ts b/src/test/renderer/gitStatusCache.test.ts new file mode 100644 index 000000000..1a0277b99 --- /dev/null +++ b/src/test/renderer/gitStatusCache.test.ts @@ -0,0 +1,266 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +const mockGetGitStatus = vi.fn(); +vi.stubGlobal('window', { + electronAPI: { + getGitStatus: mockGetGitStatus, + }, +}); + +// Reset module state between tests so cache Maps are fresh +let buildCacheKey: typeof import('../../renderer/lib/gitStatusCache').buildCacheKey; +let getCachedGitStatus: typeof import('../../renderer/lib/gitStatusCache').getCachedGitStatus; +let getCachedResult: typeof import('../../renderer/lib/gitStatusCache').getCachedResult; +let onCacheRevalidated: typeof import('../../renderer/lib/gitStatusCache').onCacheRevalidated; + +beforeEach(async () => { + vi.clearAllMocks(); + vi.resetModules(); + const mod = await import('../../renderer/lib/gitStatusCache'); + buildCacheKey = mod.buildCacheKey; + getCachedGitStatus = mod.getCachedGitStatus; + getCachedResult = mod.getCachedResult; + onCacheRevalidated = mod.onCacheRevalidated; +}); + +describe('gitStatusCache', () => { + describe('buildCacheKey', () => { + it('uses taskPath alone when no taskId', () => { + expect(buildCacheKey('/repo')).toBe('/repo'); + }); + + it('appends taskId', () => { + expect(buildCacheKey('/repo', 'task-1')).toBe('/repo::task-1'); + }); + + it('appends ::tracked when includeUntracked is false', () => { + expect(buildCacheKey('/repo', undefined, false)).toBe('/repo::tracked'); + expect(buildCacheKey('/repo', 'task-1', false)).toBe('/repo::task-1::tracked'); + }); + + it('does not append ::tracked when includeUntracked is true or undefined', () => { + expect(buildCacheKey('/repo', undefined, true)).toBe('/repo'); + expect(buildCacheKey('/repo', undefined, undefined)).toBe('/repo'); + }); + }); + + describe('getCachedGitStatus', () => { + it('returns workspace-unavailable for empty taskPath', async () => { + const result = await getCachedGitStatus(''); + expect(result).toEqual({ success: false, error: 'workspace-unavailable' }); + expect(mockGetGitStatus).not.toHaveBeenCalled(); + }); + + it('fetches and caches on first call', async () => { + const changes = [ + { path: 'a.ts', status: 'modified', additions: 1, deletions: 0, isStaged: false }, + ]; + mockGetGitStatus.mockResolvedValueOnce({ success: true, changes }); + + const result = await getCachedGitStatus('/repo', { force: true }); + expect(result).toEqual({ success: true, changes }); + expect(mockGetGitStatus).toHaveBeenCalledOnce(); + }); + + it('returns cached result within TTL without fetching', async () => { + const changes = [ + { path: 'a.ts', status: 'modified', additions: 1, deletions: 0, isStaged: false }, + ]; + mockGetGitStatus.mockResolvedValueOnce({ success: true, changes }); + + await getCachedGitStatus('/repo', { force: true }); + mockGetGitStatus.mockClear(); + + const result = await getCachedGitStatus('/repo'); + expect(result).toEqual({ success: true, changes }); + expect(mockGetGitStatus).not.toHaveBeenCalled(); + }); + + it('deduplicates concurrent requests for the same key', async () => { + let resolve!: (v: unknown) => void; + mockGetGitStatus.mockImplementationOnce( + () => + new Promise((r) => { + resolve = r; + }) + ); + + const p1 = getCachedGitStatus('/repo', { force: true }); + const p2 = getCachedGitStatus('/repo'); + + const changes = [ + { path: 'a.ts', status: 'modified', additions: 1, deletions: 0, isStaged: false }, + ]; + resolve({ success: true, changes }); + + const [r1, r2] = await Promise.all([p1, p2]); + expect(r1).toEqual(r2); + expect(mockGetGitStatus).toHaveBeenCalledOnce(); + }); + + it('separates cache by includeUntracked', async () => { + const fullChanges = [ + { path: 'a.ts', status: 'modified', additions: 1, deletions: 0, isStaged: false }, + { path: 'new.ts', status: 'added', additions: 3, deletions: 0, isStaged: false }, + ]; + const trackedChanges = [ + { path: 'a.ts', status: 'modified', additions: 1, deletions: 0, isStaged: false }, + ]; + + mockGetGitStatus.mockResolvedValueOnce({ success: true, changes: fullChanges }); + mockGetGitStatus.mockResolvedValueOnce({ success: true, changes: trackedChanges }); + + await getCachedGitStatus('/repo', { force: true, includeUntracked: true }); + await getCachedGitStatus('/repo', { force: true, includeUntracked: false }); + + expect(mockGetGitStatus).toHaveBeenCalledTimes(2); + + mockGetGitStatus.mockClear(); + const full = await getCachedGitStatus('/repo', { includeUntracked: true }); + const tracked = await getCachedGitStatus('/repo', { includeUntracked: false }); + + expect(full.changes).toHaveLength(2); + expect(tracked.changes).toHaveLength(1); + expect(mockGetGitStatus).not.toHaveBeenCalled(); + }); + }); + + describe('stale-while-revalidate', () => { + it('returns stale data immediately and caches fresh data after background refresh', async () => { + const staleChanges = [ + { path: 'old.ts', status: 'modified', additions: 1, deletions: 0, isStaged: false }, + ]; + const freshChanges = [ + { path: 'new.ts', status: 'added', additions: 5, deletions: 0, isStaged: false }, + ]; + + mockGetGitStatus.mockResolvedValueOnce({ success: true, changes: staleChanges }); + await getCachedGitStatus('/repo', { force: true }); + + vi.useFakeTimers(); + vi.advanceTimersByTime(31000); + mockGetGitStatus.mockClear(); + mockGetGitStatus.mockResolvedValueOnce({ success: true, changes: freshChanges }); + + // Should return stale data immediately + const staleResult = await getCachedGitStatus('/repo'); + expect(staleResult).toEqual({ success: true, changes: staleChanges }); + expect(mockGetGitStatus).toHaveBeenCalledOnce(); + + // Let background refresh complete + await vi.runAllTimersAsync(); + + // Fresh data should now be cached + mockGetGitStatus.mockClear(); + const freshResult = await getCachedGitStatus('/repo'); + expect(freshResult).toEqual({ success: true, changes: freshChanges }); + expect(mockGetGitStatus).not.toHaveBeenCalled(); + + vi.useRealTimers(); + }); + + it('preserves known-good cache entry on background refresh failure', async () => { + const goodChanges = [ + { path: 'a.ts', status: 'modified', additions: 1, deletions: 0, isStaged: false }, + ]; + + mockGetGitStatus.mockResolvedValueOnce({ success: true, changes: goodChanges }); + await getCachedGitStatus('/repo', { force: true }); + + vi.useFakeTimers(); + vi.advanceTimersByTime(31000); + mockGetGitStatus.mockClear(); + mockGetGitStatus.mockRejectedValueOnce(new Error('network error')); + + // SWR returns stale data + await getCachedGitStatus('/repo'); + + // Let failed background refresh complete + await vi.runAllTimersAsync(); + + // Cache should still contain the good data, not the error + const cached = getCachedResult('/repo'); + expect(cached).toBeDefined(); + expect(cached!.result.success).toBe(true); + expect(cached!.result.changes).toEqual(goodChanges); + + vi.useRealTimers(); + }); + }); + + describe('onCacheRevalidated', () => { + it('notifies listeners with fresh data on background revalidation', async () => { + const staleChanges = [ + { path: 'old.ts', status: 'modified', additions: 1, deletions: 0, isStaged: false }, + ]; + const freshChanges = [ + { path: 'new.ts', status: 'added', additions: 5, deletions: 0, isStaged: false }, + ]; + + mockGetGitStatus.mockResolvedValueOnce({ success: true, changes: staleChanges }); + await getCachedGitStatus('/repo', { force: true }); + + const notifications: Array<{ key: string; changes: number }> = []; + const unsub = onCacheRevalidated((key, result) => { + notifications.push({ key, changes: result.changes?.length ?? 0 }); + }); + + vi.useFakeTimers(); + vi.advanceTimersByTime(31000); + mockGetGitStatus.mockResolvedValueOnce({ success: true, changes: freshChanges }); + await getCachedGitStatus('/repo'); + await vi.runAllTimersAsync(); + + expect(notifications).toHaveLength(1); + expect(notifications[0]).toEqual({ key: '/repo', changes: 1 }); + + unsub(); + vi.useRealTimers(); + }); + + it('notifies listeners on failed background revalidation', async () => { + const goodChanges = [ + { path: 'a.ts', status: 'modified', additions: 1, deletions: 0, isStaged: false }, + ]; + + mockGetGitStatus.mockResolvedValueOnce({ success: true, changes: goodChanges }); + await getCachedGitStatus('/repo', { force: true }); + + const notifications: Array<{ key: string; success: boolean }> = []; + const unsub = onCacheRevalidated((key, result) => { + notifications.push({ key, success: result.success }); + }); + + vi.useFakeTimers(); + vi.advanceTimersByTime(31000); + mockGetGitStatus.mockRejectedValueOnce(new Error('network error')); + await getCachedGitStatus('/repo'); + await vi.runAllTimersAsync(); + + expect(notifications).toHaveLength(1); + expect(notifications[0]).toEqual({ key: '/repo', success: false }); + + unsub(); + vi.useRealTimers(); + }); + + it('cleans up listener on unsubscribe', async () => { + const listener = vi.fn(); + const unsub = onCacheRevalidated(listener); + + mockGetGitStatus.mockResolvedValueOnce({ success: true, changes: [] }); + await getCachedGitStatus('/repo', { force: true }); + + unsub(); + + vi.useFakeTimers(); + vi.advanceTimersByTime(31000); + mockGetGitStatus.mockResolvedValueOnce({ success: true, changes: [] }); + await getCachedGitStatus('/repo'); + await vi.runAllTimersAsync(); + + expect(listener).not.toHaveBeenCalled(); + vi.useRealTimers(); + }); + }); +}); diff --git a/src/test/renderer/useFileChanges.test.ts b/src/test/renderer/useFileChanges.test.ts deleted file mode 100644 index 8558ea866..000000000 --- a/src/test/renderer/useFileChanges.test.ts +++ /dev/null @@ -1,24 +0,0 @@ -import { describe, expect, it } from 'vitest'; -import { shouldRefreshFileChanges } from '../../renderer/hooks/useFileChanges'; - -describe('shouldRefreshFileChanges', () => { - it('refreshes when the task is active and the document is visible', () => { - expect(shouldRefreshFileChanges('/tmp/task', true, true)).toBe(true); - }); - - it('refreshes when the task is active and the document is visible (focus is no longer required)', () => { - expect(shouldRefreshFileChanges('/tmp/task', true, true)).toBe(true); - }); - - it('does not refresh when the task path is missing', () => { - expect(shouldRefreshFileChanges(undefined, true, true)).toBe(false); - }); - - it('does not refresh when the task is inactive', () => { - expect(shouldRefreshFileChanges('/tmp/task', false, true)).toBe(false); - }); - - it('does not refresh when the document is hidden', () => { - expect(shouldRefreshFileChanges('/tmp/task', true, false)).toBe(false); - }); -}); diff --git a/src/test/renderer/useFileChanges.test.tsx b/src/test/renderer/useFileChanges.test.tsx new file mode 100644 index 000000000..97aba8903 --- /dev/null +++ b/src/test/renderer/useFileChanges.test.tsx @@ -0,0 +1,148 @@ +import { describe, expect, it, vi, beforeEach } from 'vitest'; +import { renderHook, act, waitFor } from '@testing-library/react'; +import { shouldRefreshFileChanges } from '../../renderer/hooks/useFileChanges'; + +// Mock external dependencies +const mockGetGitStatus = vi.fn(); +const mockWatchGitStatus = vi.fn().mockResolvedValue({ success: false }); +const mockUnwatchGitStatus = vi.fn().mockResolvedValue({}); +const mockOnGitStatusChanged = vi.fn().mockReturnValue(() => {}); + +Object.defineProperty(window, 'electronAPI', { + value: { + getGitStatus: mockGetGitStatus, + watchGitStatus: mockWatchGitStatus, + unwatchGitStatus: mockUnwatchGitStatus, + onGitStatusChanged: mockOnGitStatusChanged, + }, + writable: true, +}); + +vi.mock('@/lib/fileChangeEvents', () => ({ + subscribeToFileChanges: vi.fn().mockReturnValue(() => {}), +})); + +const { useFileChanges } = await import('../../renderer/hooks/useFileChanges'); + +function makeChanges(paths: string[]) { + return paths.map((p) => ({ + path: p, + status: 'modified', + additions: 1, + deletions: 0, + isStaged: false, + })); +} + +describe('shouldRefreshFileChanges', () => { + it('refreshes when the task is active and the document is visible', () => { + expect(shouldRefreshFileChanges('/tmp/task', true, true)).toBe(true); + }); + + it('refreshes when the task is active and the document is visible (focus is no longer required)', () => { + expect(shouldRefreshFileChanges('/tmp/task', true, true)).toBe(true); + }); + + it('does not refresh when the task path is missing', () => { + expect(shouldRefreshFileChanges(undefined, true, true)).toBe(false); + }); + + it('does not refresh when the task is inactive', () => { + expect(shouldRefreshFileChanges('/tmp/task', false, true)).toBe(false); + }); + + it('does not refresh when the document is hidden', () => { + expect(shouldRefreshFileChanges('/tmp/task', true, false)).toBe(false); + }); +}); + +describe('useFileChanges taskId transitions', () => { + let useFileChangesLocal: typeof useFileChanges; + + beforeEach(async () => { + vi.clearAllMocks(); + vi.resetModules(); + const mod = await import('../../renderer/hooks/useFileChanges'); + useFileChangesLocal = mod.useFileChanges; + }); + + it('fetches with the new taskId when taskId changes on the same taskPath', async () => { + const changesA = makeChanges(['a.ts']); + const changesB = makeChanges(['b.ts']); + + mockGetGitStatus.mockImplementation((arg: { taskId?: string }) => { + if (arg.taskId === 'task-A') return Promise.resolve({ success: true, changes: changesA }); + if (arg.taskId === 'task-B') return Promise.resolve({ success: true, changes: changesB }); + return Promise.resolve({ success: true, changes: [] }); + }); + + const { result, rerender } = renderHook( + ({ taskPath, taskId }: { taskPath: string; taskId: string }) => + useFileChangesLocal(taskPath, { taskId }), + { initialProps: { taskPath: '/repo', taskId: 'task-A' } } + ); + + await waitFor(() => { + expect(result.current.fileChanges).toHaveLength(1); + expect(result.current.fileChanges[0].path).toBe('a.ts'); + }); + + rerender({ taskPath: '/repo', taskId: 'task-B' }); + + await waitFor(() => { + expect(result.current.fileChanges).toHaveLength(1); + expect(result.current.fileChanges[0].path).toBe('b.ts'); + }); + + const calls = mockGetGitStatus.mock.calls; + const lastCall = calls[calls.length - 1][0]; + expect(lastCall.taskId).toBe('task-B'); + }); + + it('discards stale results from old taskId after switching', async () => { + const changesA = makeChanges(['a.ts']); + const changesB = makeChanges(['fresh-b.ts']); + + // Load task A successfully first + mockGetGitStatus.mockResolvedValueOnce({ success: true, changes: changesA }); + + const { result, rerender } = renderHook( + ({ taskPath, taskId }: { taskPath: string; taskId: string }) => + useFileChangesLocal(taskPath, { taskId }), + { initialProps: { taskPath: '/repo', taskId: 'task-A' } } + ); + + await waitFor(() => { + expect(result.current.fileChanges[0]?.path).toBe('a.ts'); + }); + + // Now make A's next fetch hang (simulating a slow refresh) + let resolveStaleA!: (v: unknown) => void; + mockGetGitStatus.mockImplementationOnce( + () => + new Promise((resolve) => { + resolveStaleA = resolve; + }) + ); + + // Force a refresh for A (e.g., watcher event) to start an in-flight request + await act(async () => { + result.current.refreshChanges(); + }); + + // Switch to task B while A's refresh is in-flight + mockGetGitStatus.mockResolvedValue({ success: true, changes: changesB }); + rerender({ taskPath: '/repo', taskId: 'task-B' }); + + // Resolve A's stale response + await act(async () => { + resolveStaleA({ success: true, changes: makeChanges(['stale-a.ts']) }); + }); + + // Wait for B to load + await waitFor(() => { + expect(result.current.fileChanges).toHaveLength(1); + expect(result.current.fileChanges[0].path).toBe('fresh-b.ts'); + }); + }); +});