diff --git a/src/main/ipc/filesystem-watcher-local-events.test.ts b/src/main/ipc/filesystem-watcher-local-events.test.ts index 15a6fd0e956..e08145e7ad2 100644 --- a/src/main/ipc/filesystem-watcher-local-events.test.ts +++ b/src/main/ipc/filesystem-watcher-local-events.test.ts @@ -132,6 +132,44 @@ describe('local filesystem watcher flush serialization', () => { expect(sender.send).not.toHaveBeenCalled() }) + it('caps concurrent stats at eight for a full batch and keeps result order', async () => { + const eventCount = 5_000 + const paths = Array.from({ length: eventCount }, (_, index) => `/repo/file-${index}.ts`) + let inFlight = 0 + let peakInFlight = 0 + statMock.mockImplementation(async (statPath: string) => { + inFlight++ + peakInFlight = Math.max(peakInFlight, inFlight) + await Promise.resolve() + inFlight-- + return { isDirectory: () => statPath.endsWith('-0.ts') } + }) + const root = await createLocalWatcher('/repo', '/repo') + root.listeners.set(1, sender as never) + + watcherCallback?.( + null, + paths.map((path) => ({ type: 'update' as const, path })) + ) + vi.advanceTimersByTime(WATCH_BATCH_TRAILING_MS) + // Why a loop, not a fixed microtask count: 5,000 stats through 8 lanes take many turns. + for (let i = 0; i < eventCount * 4 && sender.send.mock.calls.length === 0; i++) { + await Promise.resolve() + } + + expect(statMock).toHaveBeenCalledTimes(eventCount) + expect(peakInFlight).toBe(8) + expect(sender.send).toHaveBeenCalledTimes(1) + const { events } = sender.send.mock.calls[0][1] as FsChangedPayload + expect(events).toEqual( + paths.map((path) => ({ + kind: 'update', + absolutePath: path, + isDirectory: path.endsWith('-0.ts') + })) + ) + }) + it('leaves an open debounce window to the armed timer instead of draining early', async () => { const firstStat = deferred<{ isDirectory: () => boolean }>() const secondStat = deferred<{ isDirectory: () => boolean }>() diff --git a/src/main/ipc/filesystem-watcher-local-events.ts b/src/main/ipc/filesystem-watcher-local-events.ts index 7d5b383176f..57ef7da4e1f 100644 --- a/src/main/ipc/filesystem-watcher-local-events.ts +++ b/src/main/ipc/filesystem-watcher-local-events.ts @@ -17,6 +17,10 @@ import { trackDetachedLocalUnsubscribe } from './filesystem-watcher-listener-lifecycle' import { createDebouncedBatch } from './filesystem-watcher-batch-control' +import { mapWithConcurrency } from '../../shared/map-with-concurrency' + +// Why: matches the watcher subprocess budget in parcel-watcher-event-delivery.ts. +const DIRECTORY_STAT_CONCURRENCY = 8 // ── Event coalescing ───────────────────────────────────────────────── // Why: keep the last event per path in a flush window; delete→create emits both (delete cleans the subtree, create refreshes the parent), create→delete is dropped (§4.4). @@ -128,8 +132,12 @@ async function flushBatch(root: WatchedRoot): Promise { const coalesced = coalesceEvents(rawEvents) - const events: FsChangeEvent[] = await Promise.all( - coalesced.map(async (evt) => { + // Why: a full batch is up to MAX_BATCHED_WATCHER_EVENTS paths; unbounded stat() would swamp + // libuv's 4-thread pool, which also serves git reads and persistence writes. + const events: FsChangeEvent[] = await mapWithConcurrency( + coalesced, + DIRECTORY_STAT_CONCURRENCY, + async (evt) => { // Why: a deleted path can't be stat'd; leave isDirectory undefined and let the renderer infer from dirCache. const isDirectory = evt.type === 'delete' ? undefined : await tryStatIsDirectory(evt.path) @@ -138,7 +146,7 @@ async function flushBatch(root: WatchedRoot): Promise { absolutePath: evt.path, isDirectory } - }) + } ) if (root.batch.cancelled || root.listeners.size === 0) { diff --git a/src/renderer/src/components/right-sidebar/FileExplorerFilesTreePane.tsx b/src/renderer/src/components/right-sidebar/FileExplorerFilesTreePane.tsx index 992a3ffecec..b6fb168245f 100644 --- a/src/renderer/src/components/right-sidebar/FileExplorerFilesTreePane.tsx +++ b/src/renderer/src/components/right-sidebar/FileExplorerFilesTreePane.tsx @@ -59,7 +59,7 @@ export function FileExplorerFilesTreePane({ handleExplorerBackgroundContextMenuCapture, handleExplorerBackgroundDoubleClick }: FileExplorerFilesTreePaneProps): React.JSX.Element { - const { dirCache, rootCache, rootError } = tree + const { loadingDirPaths, rootCache, rootError } = tree const { selectedPaths, preserveSelectionForContextMenu, copyPathsForNode } = selection const { scrollRef, @@ -111,8 +111,8 @@ export function FileExplorerFilesTreePane({ // when the tree is empty, still loading, or showing a read error. const isEmptyState = visibleRowCount === 0 && !inlineInput const isNameFilterLoading = nameFilterSource?.relativePaths === null - const isLoading = - isEmptyState && (hasNameFilter ? isNameFilterLoading : (rootCache?.loading ?? true)) + const isRootLoading = !rootCache || (!!worktreePath && loadingDirPaths.has(worktreePath)) + const isLoading = isEmptyState && (hasNameFilter ? isNameFilterLoading : isRootLoading) const treeError = hasNameFilter ? nameFilterFiles.loadError : rootError const hasError = isEmptyState && !isLoading && !!treeError const showTree = !isEmptyState @@ -177,7 +177,7 @@ export function FileExplorerFilesTreePane({ ignoredByRelativePath={ignoredByRelativePath} expanded={rowExpandedPaths} canCollapseFolderSubtree={!hasNameFilter} - dirCache={dirCache} + loadingDirPaths={loadingDirPaths} selectedPaths={selectedPaths} activeFileId={activeFileId} flashingPath={flashingPath} diff --git a/src/renderer/src/components/right-sidebar/FileExplorerVirtualRows.row-handlers.test.tsx b/src/renderer/src/components/right-sidebar/FileExplorerVirtualRows.row-handlers.test.tsx index 2906a481886..0da560cd139 100644 --- a/src/renderer/src/components/right-sidebar/FileExplorerVirtualRows.row-handlers.test.tsx +++ b/src/renderer/src/components/right-sidebar/FileExplorerVirtualRows.row-handlers.test.tsx @@ -36,7 +36,7 @@ describe('FileExplorerRow collapse folder action', () => { statusByRelativePath: new Map(), ignoredByRelativePath: new Set(), expanded: new Set([directoryNode.path]), - dirCache: {}, + loadingDirPaths: new Set(), selectedPaths: new Set(), activeFileId: null, flashingPath: null, @@ -89,7 +89,7 @@ describe('FileExplorerRow collapse folder action', () => { statusByRelativePath: new Map(), ignoredByRelativePath: new Set(), expanded: new Set([directoryNode.path]), - dirCache: {}, + loadingDirPaths: new Set(), selectedPaths: new Set(), activeFileId: null, flashingPath: null, @@ -141,7 +141,7 @@ describe('FileExplorerRow collapse folder action', () => { statusByRelativePath: new Map(), ignoredByRelativePath: new Set(), expanded: new Set(), - dirCache: {}, + loadingDirPaths: new Set(), selectedPaths: new Set(), activeFileId: null, flashingPath: null, @@ -194,7 +194,7 @@ describe('FileExplorerRow collapse folder action', () => { statusByRelativePath: new Map(), ignoredByRelativePath: new Set(), expanded: new Set([directoryNode.path]), - dirCache: {}, + loadingDirPaths: new Set(), selectedPaths: new Set(), activeFileId: null, flashingPath: null, @@ -247,7 +247,7 @@ describe('FileExplorerRow collapse folder action', () => { statusByRelativePath: new Map(), ignoredByRelativePath: new Set(), expanded: new Set(), - dirCache: {}, + loadingDirPaths: new Set(), selectedPaths: new Set(), activeFileId: null, flashingPath: null, diff --git a/src/renderer/src/components/right-sidebar/FileExplorerVirtualRows.tsx b/src/renderer/src/components/right-sidebar/FileExplorerVirtualRows.tsx index 0bb1d0d5d11..11bf2c65597 100644 --- a/src/renderer/src/components/right-sidebar/FileExplorerVirtualRows.tsx +++ b/src/renderer/src/components/right-sidebar/FileExplorerVirtualRows.tsx @@ -6,7 +6,7 @@ import type { GitFileStatus } from '../../../../shared/git-status-types' import { FileExplorerRow } from './FileExplorerRow' import { InlineInputRow, type InlineInput } from './file-explorer-inline-input-row' import { shouldShowIgnoredDecoration, STATUS_COLORS } from './status-display' -import type { DirCache, TreeNode } from './file-explorer-types' +import type { TreeNode } from './file-explorer-types' import type { FileExplorerRowProjection } from './file-explorer-row-projection' import type { RuntimeFileOperationArgs } from '@/runtime/runtime-file-client' @@ -22,7 +22,7 @@ type FileExplorerVirtualRowsProps = { ignoredByRelativePath: Set expanded: Set canCollapseFolderSubtree?: boolean - dirCache: Record + loadingDirPaths: ReadonlySet selectedPaths: Set activeFileId: string | null flashingPath: string | null @@ -69,7 +69,7 @@ export function FileExplorerVirtualRows(props: FileExplorerVirtualRowsProps): Re ignoredByRelativePath, expanded, canCollapseFolderSubtree = true, - dirCache, + loadingDirPaths, selectedPaths, activeFileId, flashingPath, @@ -171,7 +171,7 @@ export function FileExplorerVirtualRows(props: FileExplorerVirtualRowsProps): Re { statusByRelativePath: new Map(), ignoredByRelativePath: new Set(), expanded: new Set([directoryNode.path]), - dirCache: {}, + loadingDirPaths: new Set(), selectedPaths: new Set(), activeFileId: null, flashingPath: null, diff --git a/src/renderer/src/components/right-sidebar/file-explorer-dir-load-state.ts b/src/renderer/src/components/right-sidebar/file-explorer-dir-load-state.ts new file mode 100644 index 00000000000..37868267b08 --- /dev/null +++ b/src/renderer/src/components/right-sidebar/file-explorer-dir-load-state.ts @@ -0,0 +1,76 @@ +import type { DirCache } from './file-explorer-types' + +/** + * Directories with a directory read in flight. + * + * Why this is not a `dirCache` field: every `dirCache` identity change re-walks and re-flattens the + * whole visible tree (the ignored-path query plus the row projection). A read that starts and a read + * that lands would each pay for that, and the first one commits a byte-identical row set because the + * spinner is the only thing that changed. Keeping the flag in a sibling set lets `dirCache` change + * only when `children` do. + */ +export const EMPTY_FILE_EXPLORER_LOADING_DIRS: ReadonlySet = new Set() + +/** + * Applies one mark/clear to the loading set. + * + * Why not `Dispatch>`: the owner keeps the set in a ref as well as in state, and + * the ref must be current the moment a mark is made — a refresh wave marks every expanded dir + * before the render that would refresh a mirrored copy. + */ +export type FileExplorerLoadingDirsUpdater = ( + update: (prev: ReadonlySet) => ReadonlySet +) => void + +/** Returns `prev` unchanged when every path is already marked, so subscribers do not re-render. */ +export function markFileExplorerDirsLoading( + prev: ReadonlySet, + dirPaths: readonly string[] +): ReadonlySet { + if (dirPaths.every((dirPath) => prev.has(dirPath))) { + return prev + } + const next = new Set(prev) + for (const dirPath of dirPaths) { + next.add(dirPath) + } + return next +} + +/** Returns `prev` unchanged when no path was marked, so subscribers do not re-render. */ +export function clearFileExplorerDirsLoading( + prev: ReadonlySet, + dirPaths: readonly string[] +): ReadonlySet { + if (!dirPaths.some((dirPath) => prev.has(dirPath))) { + return prev + } + const next = new Set(prev) + for (const dirPath of dirPaths) { + next.delete(dirPath) + } + return next.size === 0 ? EMPTY_FILE_EXPLORER_LOADING_DIRS : next +} + +/** + * Adds an empty listing for dirs a read is about to populate for the first time. + * + * Why: a `dirCache` key is what tells the watcher reconciler that a path is a directory the + * Explorer tracks. Without the placeholder, a create/delete arriving while the very first read of + * that dir is still in flight resolves to no cached dir and is dropped, leaving the listing stale. + * Returns `prev` unchanged once every dir is known, which is the common case. + */ +export function withPendingFileExplorerDirCacheEntries( + prev: Record, + dirPaths: readonly string[] +): Record { + const missing = dirPaths.filter((dirPath) => prev[dirPath] === undefined) + if (missing.length === 0) { + return prev + } + const next = { ...prev } + for (const dirPath of missing) { + next[dirPath] = { children: [] } + } + return next +} diff --git a/src/renderer/src/components/right-sidebar/file-explorer-drag-scroll-marker.test.tsx b/src/renderer/src/components/right-sidebar/file-explorer-drag-scroll-marker.test.tsx index 7516546735d..0bd61cb0dd5 100644 --- a/src/renderer/src/components/right-sidebar/file-explorer-drag-scroll-marker.test.tsx +++ b/src/renderer/src/components/right-sidebar/file-explorer-drag-scroll-marker.test.tsx @@ -64,7 +64,7 @@ function virtualRowsElement(nodes: TreeNode[]): React.JSX.Element { statusByRelativePath: new Map(), ignoredByRelativePath: new Set(), expanded: new Set(), - dirCache: {}, + loadingDirPaths: new Set(), selectedPaths: new Set(), activeFileId: null, flashingPath: null, diff --git a/src/renderer/src/components/right-sidebar/file-explorer-expanded-dirs-refresh.test.ts b/src/renderer/src/components/right-sidebar/file-explorer-expanded-dirs-refresh.test.ts index cbb6b6bb25f..a46014d23f2 100644 --- a/src/renderer/src/components/right-sidebar/file-explorer-expanded-dirs-refresh.test.ts +++ b/src/renderer/src/components/right-sidebar/file-explorer-expanded-dirs-refresh.test.ts @@ -4,30 +4,49 @@ import type { DirEntry } from '../../../../shared/filesystem-entry-types' import type { DirCache } from './file-explorer-types' import { createFileExplorerDirLoadTracker } from './file-explorer-dir-load-tracker' import { refreshFileExplorerExpandedDirs } from './file-explorer-expanded-dirs-refresh' +import type { FileExplorerLoadingDirsUpdater } from './file-explorer-dir-load-state' type CacheUpdate = SetStateAction> +function createLoadingDirPathsRecorder(): { + updateLoadingDirPaths: FileExplorerLoadingDirsUpdater + isLoading: (dirPath: string) => boolean +} { + let loadingDirPaths: ReadonlySet = new Set() + return { + updateLoadingDirPaths: (update) => { + loadingDirPaths = update(loadingDirPaths) + }, + isLoading: (dirPath: string) => loadingDirPaths.has(dirPath) + } +} + function entry(name: string, isDirectory = false): DirEntry { return { name, isDirectory, isSymlink: false } } describe('refreshFileExplorerExpandedDirs', () => { - it('reloads expanded directories with one loading cache commit and one result cache commit', async () => { + it('rebuilds the dirCache identity once per refresh of already-cached dirs', async () => { let cache: Record = { '/repo': { children: [ { name: 'old', path: '/repo/old', relativePath: 'old', isDirectory: false, depth: 0 } - ], - loading: false + ] }, - '/repo/src': { children: [], loading: false }, - '/repo/docs': { children: [], loading: false } + '/repo/src': { children: [] }, + '/repo/docs': { children: [] } } + // Why identities, not calls: React skips the re-render (and the row-projection rebuild) when a + // setState produces the same value, so only a new identity costs a full tree walk. const committedCaches: Record[] = [] const setDirCache = vi.fn((update: CacheUpdate) => { - cache = typeof update === 'function' ? update(cache) : update - committedCaches.push(cache) + const next = typeof update === 'function' ? update(cache) : update + if (next !== cache) { + committedCaches.push(next) + } + cache = next }) + const { updateLoadingDirPaths, isLoading } = createLoadingDirPathsRecorder() const readDirectory = vi.fn(async (dirPath: string) => { const entriesByPath: Record = { '/repo/src': [entry('index.ts')], @@ -44,22 +63,19 @@ describe('refreshFileExplorerExpandedDirs', () => { worktreePath: '/repo', dirLoadTracker: createFileExplorerDirLoadTracker(), setDirCache, + updateLoadingDirPaths, readDirectory, // A limit at or above the dir count keeps one result batch. maxConcurrentReads: 16 }) expect(refreshed).toBe(true) - expect(setDirCache).toHaveBeenCalledTimes(2) + expect(committedCaches).toHaveLength(1) + expect(isLoading('/repo/src')).toBe(false) + expect(isLoading('/repo/docs')).toBe(false) expect(committedCaches[0]).toMatchObject({ - '/repo': { loading: false, children: [{ name: 'old' }] }, - '/repo/src': { loading: true }, - '/repo/docs': { loading: true } - }) - expect(committedCaches[1]).toMatchObject({ - '/repo': { loading: false, children: [{ name: 'old' }] }, + '/repo': { children: [{ name: 'old' }] }, '/repo/src': { - loading: false, children: [ { name: 'index.ts', @@ -71,7 +87,6 @@ describe('refreshFileExplorerExpandedDirs', () => { ] }, '/repo/docs': { - loading: false, children: [ { name: 'guide.md', @@ -89,14 +104,14 @@ describe('refreshFileExplorerExpandedDirs', () => { it('drops a superseded directory result so a newer concurrent load is not clobbered', async () => { const tracker = createFileExplorerDirLoadTracker() let cache: Record = { - '/repo/src': { children: [], loading: false }, - '/repo/docs': { children: [], loading: false } + '/repo/src': { children: [] }, + '/repo/docs': { children: [] } } const setDirCache = vi.fn((update: CacheUpdate) => { cache = typeof update === 'function' ? update(cache) : update }) + const { updateLoadingDirPaths } = createLoadingDirPathsRecorder() const newerSrcCache: DirCache = { - loading: true, children: [ { name: 'fresh.ts', @@ -130,6 +145,7 @@ describe('refreshFileExplorerExpandedDirs', () => { worktreePath: '/repo', dirLoadTracker: tracker, setDirCache, + updateLoadingDirPaths, readDirectory, maxConcurrentReads: 16 }) @@ -140,21 +156,19 @@ describe('refreshFileExplorerExpandedDirs', () => { // dropped from the batched commit instead of clobbering fresher data. expect(cache['/repo/src']).toEqual(newerSrcCache) // The still-current dir is committed normally. - expect(cache['/repo/docs']).toMatchObject({ - loading: false, - children: [{ name: 'guide.md' }] - }) + expect(cache['/repo/docs']).toMatchObject({ children: [{ name: 'guide.md' }] }) }) it('drops a result superseded after its read resolved but before the batch commit', async () => { const tracker = createFileExplorerDirLoadTracker() let cache: Record = { - '/repo/src': { children: [], loading: false }, - '/repo/docs': { children: [], loading: false } + '/repo/src': { children: [] }, + '/repo/docs': { children: [] } } const setDirCache = vi.fn((update: CacheUpdate) => { cache = typeof update === 'function' ? update(cache) : update }) + const { updateLoadingDirPaths } = createLoadingDirPathsRecorder() let releaseDocs!: () => void const docsGate = new Promise((resolve) => { releaseDocs = resolve @@ -175,6 +189,7 @@ describe('refreshFileExplorerExpandedDirs', () => { worktreePath: '/repo', dirLoadTracker: tracker, setDirCache, + updateLoadingDirPaths, readDirectory, maxConcurrentReads: 16 }) @@ -187,7 +202,6 @@ describe('refreshFileExplorerExpandedDirs', () => { // the window between its resolved read and the final batched commit. tracker.begin('/repo/src') const newerSrcCache: DirCache = { - loading: false, children: [ { name: 'fresh.ts', @@ -206,10 +220,7 @@ describe('refreshFileExplorerExpandedDirs', () => { expect(refreshed).toBe(false) // The stale /repo/src read must not clobber the newer committed cache. expect(cache['/repo/src']).toEqual(newerSrcCache) - expect(cache['/repo/docs']).toMatchObject({ - loading: false, - children: [{ name: 'guide.md' }] - }) + expect(cache['/repo/docs']).toMatchObject({ children: [{ name: 'guide.md' }] }) }) it('never exceeds maxConcurrentReads in flight and still commits every directory', async () => { @@ -221,6 +232,7 @@ describe('refreshFileExplorerExpandedDirs', () => { const setDirCache = vi.fn((update: CacheUpdate) => { cache = typeof update === 'function' ? update(cache) : update }) + const { updateLoadingDirPaths, isLoading } = createLoadingDirPathsRecorder() let inFlight = 0 let peakInFlight = 0 const readDirectory = vi.fn(async (dirPath: string) => { @@ -239,6 +251,7 @@ describe('refreshFileExplorerExpandedDirs', () => { worktreePath: '/repo', dirLoadTracker: createFileExplorerDirLoadTracker(), setDirCache, + updateLoadingDirPaths, readDirectory, maxConcurrentReads: 4 }) @@ -249,10 +262,8 @@ describe('refreshFileExplorerExpandedDirs', () => { // One up-front loading write plus one result write per completed group of four. expect(setDirCache).toHaveBeenCalledTimes(6) for (const { dirPath } of dirs) { - expect(cache[dirPath]).toMatchObject({ - loading: false, - children: [{ name: expect.any(String) }] - }) + expect(cache[dirPath]).toMatchObject({ children: [{ name: expect.any(String) }] }) + expect(isLoading(dirPath)).toBe(false) } }) @@ -265,6 +276,7 @@ describe('refreshFileExplorerExpandedDirs', () => { const setDirCache = vi.fn((update: CacheUpdate) => { cache = typeof update === 'function' ? update(cache) : update }) + const { updateLoadingDirPaths, isLoading } = createLoadingDirPathsRecorder() let releaseInitialReads!: () => void const initialReadsGate = new Promise((resolve) => { releaseInitialReads = resolve @@ -281,22 +293,23 @@ describe('refreshFileExplorerExpandedDirs', () => { worktreePath: '/repo', dirLoadTracker: createFileExplorerDirLoadTracker(), setDirCache, + updateLoadingDirPaths, readDirectory, maxConcurrentReads: 3 }) await Promise.resolve() - expect(cache['/repo/d0']).toMatchObject({ loading: true }) + expect(isLoading('/repo/d0')).toBe(true) // A queued dir must already advertise loading:true, or FileExplorer's // auto-load effect fans out an unbounded loadDir for it on the next // `expanded` change — the reads this cap exists to bound. - expect(cache['/repo/d6']).toMatchObject({ loading: true }) + expect(isLoading('/repo/d6')).toBe(true) expect(readDirectory).toHaveBeenCalledTimes(3) releaseInitialReads() await refreshPromise - expect(cache['/repo/d6']).toMatchObject({ loading: false }) + expect(isLoading('/repo/d6')).toBe(false) }) it('starts later reads as slots free without waiting for the slowest initial read', async () => { @@ -308,6 +321,7 @@ describe('refreshFileExplorerExpandedDirs', () => { const setDirCache = vi.fn((update: CacheUpdate) => { cache = typeof update === 'function' ? update(cache) : update }) + const { updateLoadingDirPaths, isLoading } = createLoadingDirPathsRecorder() let releaseSlowRead!: () => void const slowRead = new Promise((resolve) => { releaseSlowRead = resolve @@ -324,6 +338,7 @@ describe('refreshFileExplorerExpandedDirs', () => { worktreePath: '/repo', dirLoadTracker: createFileExplorerDirLoadTracker(), setDirCache, + updateLoadingDirPaths, readDirectory, maxConcurrentReads: 2 }) @@ -336,12 +351,12 @@ describe('refreshFileExplorerExpandedDirs', () => { '/repo/d3', '/repo/d4' ]) - expect(cache['/repo/d1']).toMatchObject({ loading: false }) - expect(cache['/repo/d0']).toMatchObject({ loading: true }) + expect(isLoading('/repo/d1')).toBe(false) + expect(isLoading('/repo/d0')).toBe(true) releaseSlowRead() await expect(refreshPromise).resolves.toBe(true) - expect(cache['/repo/d0']).toMatchObject({ loading: false }) + expect(isLoading('/repo/d0')).toBe(false) }) it('does not turn a commit callback failure into an empty directory result', async () => { @@ -349,6 +364,7 @@ describe('refreshFileExplorerExpandedDirs', () => { const setDirCache = vi.fn((update: CacheUpdate) => { cache = typeof update === 'function' ? update(cache) : update }) + const { updateLoadingDirPaths } = createLoadingDirPathsRecorder() const commitError = new Error('commit failed') await expect( @@ -357,6 +373,7 @@ describe('refreshFileExplorerExpandedDirs', () => { worktreePath: '/repo', dirLoadTracker: createFileExplorerDirLoadTracker(), setDirCache, + updateLoadingDirPaths, readDirectory: async () => ({ entries: [entry('index.ts')], operationOwner: { kind: 'local' as const } @@ -369,10 +386,7 @@ describe('refreshFileExplorerExpandedDirs', () => { ).rejects.toBe(commitError) expect(setDirCache).toHaveBeenCalledTimes(2) - expect(cache['/repo/src']).toMatchObject({ - loading: false, - children: [{ name: 'index.ts' }] - }) + expect(cache['/repo/src']).toMatchObject({ children: [{ name: 'index.ts' }] }) }) it('still notifies the rest of a commit batch after one commit callback throws', async () => { @@ -380,6 +394,7 @@ describe('refreshFileExplorerExpandedDirs', () => { const setDirCache = vi.fn((update: CacheUpdate) => { cache = typeof update === 'function' ? update(cache) : update }) + const { updateLoadingDirPaths } = createLoadingDirPathsRecorder() const commitError = new Error('commit failed') const onDirCommitted = vi.fn((dirPath: string) => { if (dirPath === '/repo/a') { @@ -396,6 +411,7 @@ describe('refreshFileExplorerExpandedDirs', () => { worktreePath: '/repo', dirLoadTracker: createFileExplorerDirLoadTracker(), setDirCache, + updateLoadingDirPaths, readDirectory: async () => ({ entries: [entry('index.ts')], operationOwner: { kind: 'local' as const } @@ -410,7 +426,7 @@ describe('refreshFileExplorerExpandedDirs', () => { '/repo/a', '/repo/b' ]) - expect(cache['/repo/b']).toMatchObject({ loading: false, children: [{ name: 'index.ts' }] }) + expect(cache['/repo/b']).toMatchObject({ children: [{ name: 'index.ts' }] }) }) it('stops later batches after a commit callback throws', async () => { @@ -418,6 +434,7 @@ describe('refreshFileExplorerExpandedDirs', () => { const setDirCache = vi.fn((update: CacheUpdate) => { cache = typeof update === 'function' ? update(cache) : update }) + const { updateLoadingDirPaths, isLoading } = createLoadingDirPathsRecorder() const commitError = new Error('commit failed') const onDirCommitted = vi.fn((dirPath: string) => { if (dirPath === '/repo/a') { @@ -431,6 +448,7 @@ describe('refreshFileExplorerExpandedDirs', () => { worktreePath: '/repo', dirLoadTracker: createFileExplorerDirLoadTracker(), setDirCache, + updateLoadingDirPaths, readDirectory: async () => ({ entries: [entry('index.ts')], operationOwner: { kind: 'local' as const } @@ -447,10 +465,11 @@ describe('refreshFileExplorerExpandedDirs', () => { '/repo/a', '/repo/b' ]) - // One up-front loading write plus the single failed batch's result write. + // One up-front placeholder write plus the single failed batch's result write. expect(setDirCache).toHaveBeenCalledTimes(2) - expect(cache['/repo/c']).toMatchObject({ loading: true }) - expect(cache['/repo/d']).toMatchObject({ loading: true }) + // Why not still loading: no read was ever started for these, so a spinner would never clear. + expect(isLoading('/repo/c')).toBe(false) + expect(isLoading('/repo/d')).toBe(false) }) it('drops a queued directory superseded while an earlier read is blocked', async () => { @@ -459,6 +478,7 @@ describe('refreshFileExplorerExpandedDirs', () => { const setDirCache = vi.fn((update: CacheUpdate) => { cache = typeof update === 'function' ? update(cache) : update }) + const { updateLoadingDirPaths } = createLoadingDirPathsRecorder() let releaseFirst!: () => void const firstGate = new Promise((resolve) => { releaseFirst = resolve @@ -478,6 +498,7 @@ describe('refreshFileExplorerExpandedDirs', () => { worktreePath: '/repo', dirLoadTracker: tracker, setDirCache, + updateLoadingDirPaths, readDirectory, maxConcurrentReads: 1 }) @@ -485,14 +506,14 @@ describe('refreshFileExplorerExpandedDirs', () => { // A watcher-driven refreshDir supersedes the queued dir before it starts reading. tracker.begin('/repo/b') - const newerBCache: DirCache = { loading: false, children: [] } + const newerBCache: DirCache = { children: [] } setDirCache((prev) => ({ ...prev, '/repo/b': newerBCache })) releaseFirst() const refreshed = await refreshPromise expect(refreshed).toBe(false) - expect(cache['/repo/a']).toMatchObject({ loading: false, children: [{ name: 'x.ts' }] }) + expect(cache['/repo/a']).toMatchObject({ children: [{ name: 'x.ts' }] }) // The queued task must neither read nor commit the superseded dir. expect(readDirectory).toHaveBeenCalledTimes(1) expect(cache['/repo/b']).toEqual(newerBCache) diff --git a/src/renderer/src/components/right-sidebar/file-explorer-expanded-dirs-refresh.ts b/src/renderer/src/components/right-sidebar/file-explorer-expanded-dirs-refresh.ts index 64dc0d587bb..6f172ee5d11 100644 --- a/src/renderer/src/components/right-sidebar/file-explorer-expanded-dirs-refresh.ts +++ b/src/renderer/src/components/right-sidebar/file-explorer-expanded-dirs-refresh.ts @@ -6,6 +6,12 @@ import { type FileExplorerDirectoryListing } from './file-explorer-directory-listing' import { forEachWithConcurrency } from '../../../../shared/map-with-concurrency' +import { + clearFileExplorerDirsLoading, + markFileExplorerDirsLoading, + withPendingFileExplorerDirCacheEntries, + type FileExplorerLoadingDirsUpdater +} from './file-explorer-dir-load-state' export type RefreshFileExplorerTreeDir = { dirPath: string @@ -17,6 +23,7 @@ export type RefreshFileExplorerExpandedDirsParams = { worktreePath: string dirLoadTracker: FileExplorerDirLoadTracker setDirCache: Dispatch>> + updateLoadingDirPaths: FileExplorerLoadingDirsUpdater readDirectory: (dirPath: string) => Promise maxConcurrentReads: number /** Called per dir whose fresh listing was committed, so callers can clear a staleness mark. */ @@ -28,6 +35,7 @@ export async function refreshFileExplorerExpandedDirs({ worktreePath, dirLoadTracker, setDirCache, + updateLoadingDirPaths, readDirectory, maxConcurrentReads, onDirCommitted @@ -55,19 +63,22 @@ export async function refreshFileExplorerExpandedDirs({ // workers itself — otherwise a later batch commits after the caller already saw this reject. let stopped = false + const uniqueDirPaths = uniqueDirs.map((dir) => dir.dirPath) // Why: mark every dir loading up front — FileExplorer's auto-load // effect re-runs on any `expanded` change and fans out an unbounded loadDir per // dir that is neither cached nor loading, which would defeat the concurrency cap. - setDirCache((prev) => { - const next = { ...prev } - for (const { dirPath } of uniqueDirs) { - next[dirPath] = { - children: prev[dirPath]?.children ?? [], - loading: true - } + // Why this no longer touches dirCache for known dirs: the pre-mark used to rebuild the whole + // visible tree once per refresh before a single fresh listing existed. + setDirCache((prev) => withPendingFileExplorerDirCacheEntries(prev, uniqueDirPaths)) + updateLoadingDirPaths((prev) => markFileExplorerDirsLoading(prev, uniqueDirPaths)) + + // Why: only dirs this refresh still owns — a superseding load owns the flag for the rest. + const clearOwnedLoadingMarks = (dirPaths: readonly string[]): void => { + const owned = dirPaths.filter((dirPath) => dirLoadTracker.isCurrent(loadTokens.get(dirPath)!)) + if (owned.length > 0) { + updateLoadingDirPaths((prev) => clearFileExplorerDirsLoading(prev, owned)) } - return next - }) + } const commitPendingResults = (): void => { if (stopped) { @@ -88,6 +99,7 @@ export async function refreshFileExplorerExpandedDirs({ } return next }) + clearOwnedLoadingMarks(currentResults.map((result) => result.dirPath)) committedDirs += currentResults.length // Why: the cache write above already landed for every result, so a throwing callback must not // strand the rest of the batch with a staleness mark no later commit will clear. @@ -119,41 +131,46 @@ export async function refreshFileExplorerExpandedDirs({ } } - await forEachWithConcurrency(uniqueDirs, maxConcurrentReads, async ({ dirPath, depth }) => { - if (stopped) { - return - } - const loadToken = loadTokens.get(dirPath)! - // A superseding load owns this dir now; do not spend a round trip on a result we must drop. - if (!dirLoadTracker.isCurrent(loadToken)) { - settleRead() - return - } - let cache: DirCache | undefined - try { - const listing = await readDirectory(dirPath) - if (dirLoadTracker.isCurrent(loadToken)) { - cache = { - children: fileExplorerEntriesToTreeNodes( - listing.entries, - dirPath, - depth, - worktreePath, - listing.operationOwner - ), - loading: false, - operationOwner: listing.operationOwner + try { + await forEachWithConcurrency(uniqueDirs, maxConcurrentReads, async ({ dirPath, depth }) => { + if (stopped) { + return + } + const loadToken = loadTokens.get(dirPath)! + // A superseding load owns this dir now; do not spend a round trip on a result we must drop. + if (!dirLoadTracker.isCurrent(loadToken)) { + settleRead() + return + } + let cache: DirCache | undefined + try { + const listing = await readDirectory(dirPath) + if (dirLoadTracker.isCurrent(loadToken)) { + cache = { + children: fileExplorerEntriesToTreeNodes( + listing.entries, + dirPath, + depth, + worktreePath, + listing.operationOwner + ), + operationOwner: listing.operationOwner + } + } + } catch { + if (dirLoadTracker.isCurrent(loadToken)) { + cache = { children: [] } } } - } catch { - if (dirLoadTracker.isCurrent(loadToken)) { - cache = { children: [], loading: false } - } + settleRead(cache ? { dirPath, cache } : undefined) + }) + if (settledSinceCommit > 0) { + commitPendingResults() } - settleRead(cache ? { dirPath, cache } : undefined) - }) - if (settledSinceCommit > 0) { - commitPendingResults() + } finally { + // Why: no dir this refresh still owns may keep a spinner once the wave ends, including the + // ones a failed commit or a superseded read left uncommitted. + clearOwnedLoadingMarks(uniqueDirPaths) } return committedDirs === uniqueDirs.length diff --git a/src/renderer/src/components/right-sidebar/file-explorer-stale-dir-cache.test.ts b/src/renderer/src/components/right-sidebar/file-explorer-stale-dir-cache.test.ts index e0cf1c1eccf..89761eff1e2 100644 --- a/src/renderer/src/components/right-sidebar/file-explorer-stale-dir-cache.test.ts +++ b/src/renderer/src/components/right-sidebar/file-explorer-stale-dir-cache.test.ts @@ -34,20 +34,15 @@ describe('decideExpandedDirLoad', () => { const children = [{ name: 'gone.ts', path: '/repo/src/gone.ts' } as TreeNode] it('re-reads a cached dir whose listing the last full refresh skipped', () => { - expect(decideExpandedDirLoad({ children, loading: false }, true)).toBe('reload') + expect(decideExpandedDirLoad({ children }, true)).toBe('reload') }) it('trusts a cached listing that is not stale', () => { - expect(decideExpandedDirLoad({ children, loading: false }, false)).toBe('skip') + expect(decideExpandedDirLoad({ children }, false)).toBe('skip') }) it('reads a dir that has never been listed', () => { expect(decideExpandedDirLoad(undefined, false)).toBe('load') - expect(decideExpandedDirLoad({ children: [], loading: false }, false)).toBe('load') - }) - - it('never stacks a read on one already in flight, stale or not', () => { - expect(decideExpandedDirLoad({ children, loading: true }, true)).toBe('skip') - expect(decideExpandedDirLoad({ children: [], loading: true }, false)).toBe('skip') + expect(decideExpandedDirLoad({ children: [] }, false)).toBe('load') }) }) diff --git a/src/renderer/src/components/right-sidebar/file-explorer-stale-dir-cache.ts b/src/renderer/src/components/right-sidebar/file-explorer-stale-dir-cache.ts index 007e18960c8..08c1b103cc9 100644 --- a/src/renderer/src/components/right-sidebar/file-explorer-stale-dir-cache.ts +++ b/src/renderer/src/components/right-sidebar/file-explorer-stale-dir-cache.ts @@ -19,14 +19,16 @@ export function collectStaleDirCachePaths( export type ExpandedDirLoadDecision = 'skip' | 'load' | 'reload' -/** What the expansion effect owes a newly expanded dir: nothing, a first read, or a forced re-read. */ +/** + * What the expansion effect owes a newly expanded dir: nothing, a first read, or a forced re-read. + * + * Callers must skip a dir with a read already in flight before asking — that state lives in the + * loading set, not in `dirCache` (see file-explorer-dir-load-state.ts). + */ export function decideExpandedDirLoad( cached: DirCache | undefined, stale: boolean ): ExpandedDirLoadDecision { - if (cached?.loading) { - return 'skip' - } if (!cached?.children.length) { return 'load' } diff --git a/src/renderer/src/components/right-sidebar/file-explorer-types.ts b/src/renderer/src/components/right-sidebar/file-explorer-types.ts index 8fa8a432544..02bd1fd39e4 100644 --- a/src/renderer/src/components/right-sidebar/file-explorer-types.ts +++ b/src/renderer/src/components/right-sidebar/file-explorer-types.ts @@ -17,9 +17,9 @@ export type TreeNode = { operationOwner?: FileExplorerOperationOwner } +/** Why no `loading` here: see file-explorer-loading-dirs.ts — identity changes re-walk the tree. */ export type DirCache = { children: TreeNode[] - loading: boolean operationOwner?: FileExplorerOperationOwner } diff --git a/src/renderer/src/components/right-sidebar/file-explorer-watch-drive-root.test.ts b/src/renderer/src/components/right-sidebar/file-explorer-watch-drive-root.test.ts index d7b1380aefa..86e8931413b 100644 --- a/src/renderer/src/components/right-sidebar/file-explorer-watch-drive-root.test.ts +++ b/src/renderer/src/components/right-sidebar/file-explorer-watch-drive-root.test.ts @@ -12,7 +12,6 @@ function processRootEvent(root: string, event: FsChangeEvent): ReturnType, dirPath: string, worktreePath?: string, - cachePathIndex?: ReadonlyMap + cachePathIndex?: () => ReadonlyMap ): string | null { if (dirPath in cache) { return dirPath } const target = normalizeRuntimePathForComparison(dirPath) - const indexedPath = cachePathIndex?.get(target) + const indexedPath = cachePathIndex?.().get(target) if (indexedPath) { return indexedPath } diff --git a/src/renderer/src/components/right-sidebar/file-explorer-watch-reconcile.test.ts b/src/renderer/src/components/right-sidebar/file-explorer-watch-reconcile.test.ts index 343e33b64a0..7e86bcad3eb 100644 --- a/src/renderer/src/components/right-sidebar/file-explorer-watch-reconcile.test.ts +++ b/src/renderer/src/components/right-sidebar/file-explorer-watch-reconcile.test.ts @@ -14,7 +14,6 @@ function cacheWithChildren(paths: string[]): DirCache { depth: 0, operationOwner: { kind: 'local' } })), - loading: false, operationOwner: { kind: 'local' } } } @@ -432,7 +431,9 @@ describe('processFileExplorerFsPayload update reconciliation', () => { } expect(setDirCache).toHaveBeenCalledOnce() - expect(keyVisits).toBe(entryCount * 2) + // One scan, in purgeDirCacheSubtrees. The casing-fallback index stays unbuilt because every + // lookup here hits `dirPath in cache` directly. + expect(keyVisits).toBe(entryCount) expect(expandedPathReads).toBe(expandedPaths.length) expect(remainingExpanded).toEqual(new Set()) }) diff --git a/src/renderer/src/components/right-sidebar/file-explorer-watch-reconcile.ts b/src/renderer/src/components/right-sidebar/file-explorer-watch-reconcile.ts index 1d3905911ef..f5491562f52 100644 --- a/src/renderer/src/components/right-sidebar/file-explorer-watch-reconcile.ts +++ b/src/renderer/src/components/right-sidebar/file-explorer-watch-reconcile.ts @@ -68,7 +68,9 @@ export function processFileExplorerFsPayload(args: ProcessFileExplorerFsPayloadA const dirsToRefresh = new Set() const childPathIndexes = new Map>() - const cachePathIndex = createCachedDirPathIndex(cache) + let cachedDirPathIndex: ReadonlyMap | undefined + const cachePathIndex = (): ReadonlyMap => + (cachedDirPathIndex ??= createCachedDirPathIndex(cache)) const cachedDirsToPurge = new Set() const reconciledRenameSources = new Set() let needsFullRefresh = false diff --git a/src/renderer/src/components/right-sidebar/use-file-explorer-row-scrolling.ts b/src/renderer/src/components/right-sidebar/use-file-explorer-row-scrolling.ts index e59a05d5748..7fd0f96ed3a 100644 --- a/src/renderer/src/components/right-sidebar/use-file-explorer-row-scrolling.ts +++ b/src/renderer/src/components/right-sidebar/use-file-explorer-row-scrolling.ts @@ -17,6 +17,7 @@ type UseFileExplorerRowScrollingParams = { worktreePath: string | null expanded: Set dirCache: Record + loadingDirPaths: ReadonlySet rootCache: DirCache | undefined loadDir: (dirPath: string, depth: number, options?: { force?: boolean }) => Promise setSelectedPath: (path: string | null) => void @@ -42,6 +43,7 @@ export function useFileExplorerRowScrolling({ worktreePath, expanded, dirCache, + loadingDirPaths, rootCache, loadDir, setSelectedPath, @@ -81,6 +83,7 @@ export function useFileExplorerRowScrolling({ clearPendingExplorerReveal, expanded, dirCache, + loadingDirPaths, rootCache, rowProjection, loadDir, diff --git a/src/renderer/src/components/right-sidebar/use-file-explorer-tree-load-effects.ts b/src/renderer/src/components/right-sidebar/use-file-explorer-tree-load-effects.ts index 161955da697..a583484f68d 100644 --- a/src/renderer/src/components/right-sidebar/use-file-explorer-tree-load-effects.ts +++ b/src/renderer/src/components/right-sidebar/use-file-explorer-tree-load-effects.ts @@ -11,6 +11,7 @@ type UseFileExplorerTreeLoadEffectsParams = { visibleFilesWorktreePath: string | null expanded: Set dirCache: Record + loadingDirPaths: ReadonlySet rootError: string | null isDirStale: (dirPath: string) => boolean loadDir: (dirPath: string, depth: number, options?: { force?: boolean }) => Promise @@ -24,6 +25,7 @@ export function useFileExplorerTreeLoadEffects({ visibleFilesWorktreePath, expanded, dirCache, + loadingDirPaths, rootError, isDirStale, loadDir, @@ -75,6 +77,11 @@ export function useFileExplorerTreeLoadEffects({ return } for (const dirPath of expanded) { + // Why first: a refresh wave marks every dir it owns before its first read lands, and without + // this the effect would fan out an unbounded loadDir per dir on the next `expanded` change. + if (loadingDirPaths.has(dirPath)) { + continue + } // Why: a full refresh (watcher overflow) re-reads only root and the dirs expanded at the time, // so a listing cached while collapsed is unverified — re-read it here instead of trusting it. const decision = decideExpandedDirLoad(dirCache[dirPath], isDirStale(dirPath)) diff --git a/src/renderer/src/components/right-sidebar/use-file-explorer-tree-pane-state.ts b/src/renderer/src/components/right-sidebar/use-file-explorer-tree-pane-state.ts index 3bdb1557ba6..35a18f4fb6c 100644 --- a/src/renderer/src/components/right-sidebar/use-file-explorer-tree-pane-state.ts +++ b/src/renderer/src/components/right-sidebar/use-file-explorer-tree-pane-state.ts @@ -84,6 +84,7 @@ export function useFileExplorerTreePaneState({ const { dirCache, setDirCache, + loadingDirPaths, rootCache, rootError, loadDir, @@ -165,6 +166,7 @@ export function useFileExplorerTreePaneState({ visibleFilesWorktreePath, expanded, dirCache, + loadingDirPaths, rootError, isDirStale, loadDir, @@ -215,6 +217,7 @@ export function useFileExplorerTreePaneState({ worktreePath: visibleFilesWorktreePath, expanded, dirCache, + loadingDirPaths, rootCache, loadDir, setSelectedPath: setSingleSelectedPath, diff --git a/src/renderer/src/components/right-sidebar/useFileExplorerReveal.ts b/src/renderer/src/components/right-sidebar/useFileExplorerReveal.ts index a7311b31569..caac486ccdc 100644 --- a/src/renderer/src/components/right-sidebar/useFileExplorerReveal.ts +++ b/src/renderer/src/components/right-sidebar/useFileExplorerReveal.ts @@ -18,6 +18,7 @@ type UseFileExplorerRevealParams = { clearPendingExplorerReveal: () => void expanded: Set dirCache: Record + loadingDirPaths: ReadonlySet rootCache: DirCache | undefined rowProjection: FileExplorerRowProjection loadDir: (dirPath: string, depth: number, options?: { force?: boolean }) => Promise @@ -34,6 +35,7 @@ export function useFileExplorerReveal({ clearPendingExplorerReveal, expanded, dirCache, + loadingDirPaths, rootCache, rowProjection, loadDir, @@ -154,14 +156,15 @@ export function useFileExplorerReveal({ const missingAncestor = pendingRevealAncestorDirs.find( (dirPath) => !rowProjection.hasPath(dirPath) ) + const rootStillLoading = !rootCache || loadingDirPaths.has(worktreePath) const parentDirStillLoading = parentDirPath === worktreePath - ? (rootCache?.loading ?? true) - : (parentDirCache?.loading ?? true) + ? rootStillLoading + : !parentDirCache || loadingDirPaths.has(parentDirPath) const parentDirKnown = parentDirPath === worktreePath ? !!rootCache : !!parentDirCache if ( - (rootCache?.loading ?? true) || + rootStillLoading || missingExpandedAncestor || missingAncestor || parentDirStillLoading || @@ -210,6 +213,7 @@ export function useFileExplorerReveal({ clearPendingExplorerReveal, dirCache, expanded, + loadingDirPaths, pendingExplorerReveal, pendingRevealAncestorDirs, rowProjection, diff --git a/src/renderer/src/components/right-sidebar/useFileExplorerTree.refresh-projection-churn.test.tsx b/src/renderer/src/components/right-sidebar/useFileExplorerTree.refresh-projection-churn.test.tsx new file mode 100644 index 00000000000..ba37b0603b4 --- /dev/null +++ b/src/renderer/src/components/right-sidebar/useFileExplorerTree.refresh-projection-churn.test.tsx @@ -0,0 +1,248 @@ +// @vitest-environment happy-dom + +import { act, cleanup, renderHook } from '@testing-library/react' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { useAppStore } from '@/store' +import type { AppState } from '@/store/types' +import type { DirEntry } from '../../../../shared/filesystem-entry-types' +import { FileExplorerRow } from './FileExplorerRow' +import { FileExplorerVirtualRows } from './FileExplorerVirtualRows' +import { createFileExplorerRowProjection } from './file-explorer-row-projection' +import { directoryNode } from './file-explorer-tree-node-test-fixtures' +import { visit, type ReactElementLike } from './file-explorer-element-tree-test-harness' +import { useFileExplorerTreeLoadEffects } from './use-file-explorer-tree-load-effects' +import { useFileExplorerTree } from './useFileExplorerTree' +import { useFileExplorerVisibleRowProjection } from './useFileExplorerVisibleRowProjection' + +const readDirectoryMock = vi.hoisted(() => vi.fn()) +vi.mock('./file-explorer-directory-listing', async (importOriginal) => ({ + ...(await importOriginal>()), + readFileExplorerDirectory: readDirectoryMock +})) +vi.mock('./file-explorer-operation-owner', async (importOriginal) => ({ + ...(await importOriginal>()), + getFileExplorerOperationOwner: () => ({ kind: 'local' as const }) +})) +vi.mock('@/runtime/runtime-git-client', () => ({ + getRuntimeGitIgnoredPaths: vi.fn().mockResolvedValue([]) +})) + +const initialAppState = useAppStore.getInitialState() +const WORKTREE_PATH = '/repo' +const SRC_DIR = '/repo/src' + +function entry(name: string, isDirectory = false): DirEntry { + return { name, isDirectory } as DirEntry +} + +function listing(...entries: DirEntry[]) { + return { entries, operationOwner: { kind: 'local' as const } } +} + +function useTreeWithProjection(expanded: Set) { + const tree = useFileExplorerTree(WORKTREE_PATH, expanded, 'wt-1') + const projection = useFileExplorerVisibleRowProjection( + 'wt-1', + WORKTREE_PATH, + tree.dirCache, + expanded, + false, + true, + null + ) + return { tree, rowProjection: projection.rowProjection } +} + +/** Counts how many times the memoized visible-row projection produced a new value. */ +function renderTreeWithProjectionRebuildCounter(expanded: Set): { + result: { current: ReturnType } + rebuilds: () => number +} { + const seen = new Set() + const hook = renderHook(() => { + const value = useTreeWithProjection(expanded) + seen.add(value.rowProjection) + return value + }) + return { result: hook.result, rebuilds: () => seen.size } +} + +/** Holds the next directory read open so the loading commit lands in its own render. */ +function gateNextRead(): { resolve: (value: ReturnType) => void } { + let resolve!: (value: ReturnType) => void + const gate = new Promise>((nextResolve) => { + resolve = nextResolve + }) + readDirectoryMock.mockImplementationOnce(() => gate) + return { resolve } +} + +function findFileExplorerRow(node: unknown): ReactElementLike { + let found: ReactElementLike | null = null + visit(node, (candidate) => { + if (candidate.type === FileExplorerRow) { + found = candidate + } + }) + if (!found) { + throw new Error('file explorer row not found') + } + return found +} + +describe('file explorer directory refresh churn', () => { + beforeEach(() => { + readDirectoryMock.mockReset().mockResolvedValue(listing()) + useAppStore.setState(initialAppState, true) + useAppStore.setState({ + settings: { activeRuntimeEnvironmentId: null } as AppState['settings'] + }) + }) + + afterEach(() => { + cleanup() + useAppStore.setState(initialAppState, true) + }) + + it('rebuilds the visible row projection once per touched-directory refresh', async () => { + const expanded = new Set([SRC_DIR]) + readDirectoryMock.mockResolvedValue(listing(entry('index.ts'))) + const { result, rebuilds } = renderTreeWithProjectionRebuildCounter(expanded) + + await act(async () => { + await result.current.tree.loadDir(WORKTREE_PATH, -1, { force: true }) + }) + await act(async () => { + await result.current.tree.loadDir(SRC_DIR, 0, { force: true }) + }) + const rebuildsBeforeRefresh = rebuilds() + + // One watcher-driven refresh of a directory that is already cached, with the read gated so the + // loading state is committed and painted before the listing lands. + const gatedRead = gateNextRead() + let pendingRefresh!: Promise + await act(async () => { + pendingRefresh = result.current.tree.refreshDir(SRC_DIR) + await Promise.resolve() + }) + expect(result.current.tree.loadingDirPaths.has(SRC_DIR)).toBe(true) + // Why zero here: marking the dir loading used to commit a second dirCache identity carrying a + // byte-identical row set, so every refresh paid for two full tree walks and flattens. + expect(rebuilds()).toBe(rebuildsBeforeRefresh) + + await act(async () => { + gatedRead.resolve(listing(entry('index.ts'))) + await pendingRefresh + }) + expect(rebuilds() - rebuildsBeforeRefresh).toBe(1) + }) + + it('keeps the directory marked loading for the whole of a slow read', async () => { + const expanded = new Set([SRC_DIR]) + const gatedRead = gateNextRead() + const { result, rebuilds } = renderTreeWithProjectionRebuildCounter(expanded) + + let pendingLoad!: Promise + await act(async () => { + pendingLoad = result.current.tree.loadDir(SRC_DIR, 0) + await Promise.resolve() + }) + expect(result.current.tree.loadingDirPaths.has(SRC_DIR)).toBe(true) + const rebuildsWhileLoading = rebuilds() + + await act(async () => { + gatedRead.resolve(listing(entry('index.ts'))) + await pendingLoad + }) + expect(result.current.tree.loadingDirPaths.has(SRC_DIR)).toBe(false) + expect(result.current.tree.dirCache[SRC_DIR].children).toHaveLength(1) + // The spinner rendered without the projection being rebuilt for it. + expect(rebuilds()).toBe(rebuildsWhileLoading + 1) + }) + + it('does not stack a second read on an expanded dir the loading set already owns', () => { + const loadDir = vi.fn().mockResolvedValue(true) + const params = { + visibleFilesWorktreePath: WORKTREE_PATH, + expanded: new Set([SRC_DIR]), + dirCache: {}, + loadingDirPaths: new Set([SRC_DIR]), + rootError: null, + isDirStale: () => false, + loadDir, + resetAndLoad: vi.fn(), + resetSelection: vi.fn(), + setNameFilterQuery: vi.fn() + } + const hook = renderHook((props: typeof params) => useFileExplorerTreeLoadEffects(props), { + initialProps: params + }) + // Why this matters: a refresh wave marks every dir it owns before its first read lands, and the + // effect re-runs on any `expanded` change — without the guard it fans out an unbounded loadDir. + expect(loadDir).not.toHaveBeenCalled() + + // The effect re-runs on `expanded` identity; by then the wave's read has landed and cleared. + hook.rerender({ + ...params, + expanded: new Set([SRC_DIR]), + loadingDirPaths: new Set() + }) + expect(loadDir).toHaveBeenCalledTimes(1) + expect(loadDir).toHaveBeenCalledWith(SRC_DIR, 0, undefined) + }) + + it('renders the folder spinner from the loading dir set', () => { + const rowProps = { + virtualizer: { + getTotalSize: () => 26, + getVirtualItems: () => [{ index: 0, key: 'src', start: 0 }], + measureElement: vi.fn() + } as never, + inlineInputIndex: -1, + rowProjection: createFileExplorerRowProjection([directoryNode]), + inlineInput: null, + handleInlineSubmit: vi.fn(), + dismissInlineInput: vi.fn(), + folderStatusByRelativePath: new Map(), + statusByRelativePath: new Map(), + ignoredByRelativePath: new Set(), + expanded: new Set([directoryNode.path]), + selectedPaths: new Set(), + activeFileId: null, + flashingPath: null, + deleteShortcutLabel: 'Del', + onClick: vi.fn(), + onDoubleClick: vi.fn(), + onViewFile: vi.fn(), + onContextMenuSelect: vi.fn(), + onCopyPaths: vi.fn(), + onStartNew: vi.fn(), + onStartRename: vi.fn(), + onDuplicate: vi.fn(), + onAddFolderAsProject: vi.fn(), + canAddFolderAsProject: () => false, + onOpenInTerminal: vi.fn(), + onRequestDelete: vi.fn(), + onCollapseFolderSubtree: vi.fn(), + onFindInFolder: vi.fn(), + onMoveDrop: vi.fn(), + onDragTargetChange: vi.fn(), + onDragSourceChange: vi.fn(), + onDragExpandDir: vi.fn(), + onNativeDragTargetChange: vi.fn(), + onNativeDragExpandDir: vi.fn(), + dropTargetDir: null, + dragSourcePath: null, + nativeDropTargetDir: null + } + + const loading = FileExplorerVirtualRows({ + ...rowProps, + loadingDirPaths: new Set([directoryNode.path]) + }) + const idle = FileExplorerVirtualRows({ ...rowProps, loadingDirPaths: new Set() }) + + expect(findFileExplorerRow(loading).props.isLoading).toBe(true) + expect(findFileExplorerRow(idle).props.isLoading).toBe(false) + }) +}) diff --git a/src/renderer/src/components/right-sidebar/useFileExplorerTree.ts b/src/renderer/src/components/right-sidebar/useFileExplorerTree.ts index 4c8311f8dfa..a78af9ce931 100644 --- a/src/renderer/src/components/right-sidebar/useFileExplorerTree.ts +++ b/src/renderer/src/components/right-sidebar/useFileExplorerTree.ts @@ -16,10 +16,18 @@ import { import { refreshFileExplorerExpandedDirs } from './file-explorer-expanded-dirs-refresh' import { collectStaleDirCachePaths } from './file-explorer-stale-dir-cache' import { fileExplorerRefreshConcurrency } from './file-explorer-refresh-concurrency' +import { + clearFileExplorerDirsLoading, + EMPTY_FILE_EXPLORER_LOADING_DIRS, + markFileExplorerDirsLoading, + withPendingFileExplorerDirCacheEntries +} from './file-explorer-dir-load-state' type UseFileExplorerTreeResult = { dirCache: Record setDirCache: Dispatch>> + /** Dirs with a read in flight — kept out of dirCache so the row projection does not rebuild. */ + loadingDirPaths: ReadonlySet rootCache: DirCache | undefined rootError: string | null loadDir: ( @@ -42,9 +50,27 @@ export function useFileExplorerTree( activeWorktreeId?: string | null ): UseFileExplorerTreeResult { const [dirCache, setDirCache] = useState>({}) + const [loadingDirPaths, setLoadingDirPaths] = useState>( + EMPTY_FILE_EXPLORER_LOADING_DIRS + ) const [rootError, setRootError] = useState(null) const dirCacheRef = useRef(dirCache) dirCacheRef.current = dirCache + // Why the ref is authoritative rather than a render mirror: writing it during render is unsafe + // (React may discard that render), and a mirror would leave loadDir's in-flight guard reading a + // set one commit stale — long enough for a second read of the same dir to slip through. + const loadingDirPathsRef = useRef>(EMPTY_FILE_EXPLORER_LOADING_DIRS) + const updateLoadingDirPaths = useCallback( + (update: (prev: ReadonlySet) => ReadonlySet) => { + const next = update(loadingDirPathsRef.current) + if (next === loadingDirPathsRef.current) { + return + } + loadingDirPathsRef.current = next + setLoadingDirPaths(next) + }, + [] + ) const dirLoadTrackerRef = useRef>(undefined!) dirLoadTrackerRef.current ??= createFileExplorerDirLoadTracker() // Why: a ref, not state — the expansion effect must read the mark set by a refresh that landed @@ -60,25 +86,23 @@ export function useFileExplorerTree( options?: { force?: boolean; failOnError?: boolean } ) => { const cache = dirCacheRef.current - if (!options?.force && (cache[dirPath]?.children.length > 0 || cache[dirPath]?.loading)) { + if ( + !options?.force && + (cache[dirPath]?.children.length > 0 || loadingDirPathsRef.current.has(dirPath)) + ) { return true } const loadToken = dirLoadTrackerRef.current.begin(dirPath) // Why: this read starts after the refresh that marked the dir, so its result is current. staleDirsRef.current.delete(dirPath) - // Why: when force-reloading a directory (e.g. after a file is created, - // duplicated, or deleted), keep the previous children visible while the - // fresh listing loads. Clearing to [] would momentarily shrink the - // visible projection and make the virtualizer jump to the top. - setDirCache((prev) => ({ - ...prev, - [dirPath]: { - children: prev[dirPath]?.children ?? [], - loading: true - } - })) + // Why: an already-cached dir keeps its children visible for the whole read — clearing to [] + // would momentarily shrink the visible projection and jump the virtualizer to the top. + setDirCache((prev) => withPendingFileExplorerDirCacheEntries(prev, [dirPath])) + updateLoadingDirPaths((prev) => markFileExplorerDirsLoading(prev, [dirPath])) try { const listing = await readFileExplorerDirectory(activeWorktreeId, worktreePath, dirPath) + // Why: only the current owner may clear the flag — a superseded read clearing it would + // drop the spinner while the load that replaced it is still in flight. if (!dirLoadTrackerRef.current.isCurrent(loadToken)) { return false } @@ -94,8 +118,9 @@ export function useFileExplorerTree( ) setDirCache((prev) => ({ ...prev, - [dirPath]: { children, loading: false, operationOwner: listing.operationOwner } + [dirPath]: { children, operationOwner: listing.operationOwner } })) + updateLoadingDirPaths((prev) => clearFileExplorerDirsLoading(prev, [dirPath])) return true } catch (error) { if (!dirLoadTrackerRef.current.isCurrent(loadToken)) { @@ -109,11 +134,12 @@ export function useFileExplorerTree( setRootError(error instanceof Error ? error.message : String(error)) rootReadFailedRef.current = true } - setDirCache((prev) => ({ ...prev, [dirPath]: { children: [], loading: false } })) + setDirCache((prev) => ({ ...prev, [dirPath]: { children: [] } })) + updateLoadingDirPaths((prev) => clearFileExplorerDirsLoading(prev, [dirPath])) return !options?.failOnError } }, - [activeWorktreeId, worktreePath] + [activeWorktreeId, updateLoadingDirPaths, worktreePath] ) const markPathAsDirectory = useCallback((path: string) => { @@ -206,6 +232,7 @@ export function useFileExplorerTree( worktreePath, dirLoadTracker: dirLoadTrackerRef.current, setDirCache, + updateLoadingDirPaths, readDirectory: (dirPath) => readFileExplorerDirectory(activeWorktreeId, worktreePath, dirPath), maxConcurrentReads: fileExplorerRefreshConcurrency( @@ -214,7 +241,7 @@ export function useFileExplorerTree( onDirCommitted: (dirPath) => staleDirsRef.current.delete(dirPath) }) return allDirsCommitted ? 'refreshed' : 'superseded' - }, [activeWorktreeId, expanded, loadDir, worktreePath]) + }, [activeWorktreeId, expanded, loadDir, updateLoadingDirPaths, worktreePath]) const refreshDir = useCallback( async (dirPath: string) => { @@ -240,15 +267,17 @@ export function useFileExplorerTree( dirLoadTrackerRef.current.reset() staleDirsRef.current.clear() setDirCache({}) + updateLoadingDirPaths(() => EMPTY_FILE_EXPLORER_LOADING_DIRS) setRootError(null) if (worktreePath) { void loadDir(worktreePath, -1, { force: true }) } - }, [worktreePath, loadDir]) + }, [worktreePath, loadDir, updateLoadingDirPaths]) return { dirCache, setDirCache, + loadingDirPaths, rootCache, rootError, loadDir, diff --git a/src/renderer/src/components/right-sidebar/useFileExplorerVisibleRowProjection.debounce.test.tsx b/src/renderer/src/components/right-sidebar/useFileExplorerVisibleRowProjection.debounce.test.tsx index 00ffeee274d..04d980b68ba 100644 --- a/src/renderer/src/components/right-sidebar/useFileExplorerVisibleRowProjection.debounce.test.tsx +++ b/src/renderer/src/components/right-sidebar/useFileExplorerVisibleRowProjection.debounce.test.tsx @@ -22,7 +22,8 @@ function useProjection(query: string) { }) } -function treeDirCache(loading: boolean) { +/** A fresh object per call: a wave-batched refresh commits a new dirCache identity per wave. */ +function treeDirCache() { return { '/repo': { children: [ @@ -33,13 +34,12 @@ function treeDirCache(loading: boolean) { isDirectory: true, depth: 0 } - ], - loading + ] } } } -function useTreeProjection(dirCache = treeDirCache(false)) { +function useTreeProjection(dirCache = treeDirCache()) { return useFileExplorerVisibleRowProjection( 'worktree-1', '/repo', @@ -105,12 +105,12 @@ describe('file explorer ignored-path query debounce', () => { // ignored query is an uncancellable remote git check-ignore over the whole // visible tree, so identical contents must not re-issue it. const hook = renderHook(({ dirCache }) => useTreeProjection(dirCache), { - initialProps: { dirCache: treeDirCache(false) } + initialProps: { dirCache: treeDirCache() } }) expect(getRuntimeGitIgnoredPathsMock).toHaveBeenCalledTimes(1) - hook.rerender({ dirCache: treeDirCache(true) }) - hook.rerender({ dirCache: treeDirCache(false) }) + hook.rerender({ dirCache: treeDirCache() }) + hook.rerender({ dirCache: treeDirCache() }) expect(getRuntimeGitIgnoredPathsMock).toHaveBeenCalledTimes(1) }) diff --git a/src/renderer/src/components/right-sidebar/useFileExplorerVisibleRowProjection.test.ts b/src/renderer/src/components/right-sidebar/useFileExplorerVisibleRowProjection.test.ts index 692ffebe729..47294f3657a 100644 --- a/src/renderer/src/components/right-sidebar/useFileExplorerVisibleRowProjection.test.ts +++ b/src/renderer/src/components/right-sidebar/useFileExplorerVisibleRowProjection.test.ts @@ -25,7 +25,7 @@ function row(relativePath: string, isDirectory = false, depth?: number): TreeNod function cache(childrenByPath: Record): Record { const dirCache: Record = {} for (const [path, children] of Object.entries(childrenByPath)) { - dirCache[path] = { children, loading: false } + dirCache[path] = { children } } return dirCache } diff --git a/src/renderer/src/components/right-sidebar/useFileExplorerWatch.pending-refresh.test.tsx b/src/renderer/src/components/right-sidebar/useFileExplorerWatch.pending-refresh.test.tsx index 4bf3de4b551..905e20983e4 100644 --- a/src/renderer/src/components/right-sidebar/useFileExplorerWatch.pending-refresh.test.tsx +++ b/src/renderer/src/components/right-sidebar/useFileExplorerWatch.pending-refresh.test.tsx @@ -74,7 +74,7 @@ describe('useFileExplorerWatch pending refreshes', () => { useFileExplorerWatch({ worktreePath: visiblePath, activeWorktreeId: 'wt-1', - dirCache: { '/repo': { children: [], loading: false } }, + dirCache: { '/repo': { children: [] } }, setDirCache: vi.fn(), expanded: new Set(), setSelectedPath: vi.fn(), diff --git a/src/renderer/src/hooks/editor-external-watch-event-reconciliation.ts b/src/renderer/src/hooks/editor-external-watch-event-reconciliation.ts index 1cb0e3e130b..86bf910a3a8 100644 --- a/src/renderer/src/hooks/editor-external-watch-event-reconciliation.ts +++ b/src/renderer/src/hooks/editor-external-watch-event-reconciliation.ts @@ -164,6 +164,11 @@ export function buildEditorExternalWatchEventHandler( for (const change of batchPaths.changes) { const matching = batchPaths.matchingOpenFiles(change) + // Why: most watched paths match no open file, and the notification below is only ever read + // past this point — building it first allocates (and dictionary-modes) it for nothing. + if (matching.length === 0 && !batchPaths.hasCombinedDiffConsumer) { + continue + } const notification: EditorExternalWatchNotification = { worktreeId: target.worktreeId, worktreePath: target.worktreePath, @@ -177,9 +182,7 @@ export function buildEditorExternalWatchEventHandler( } }) if (matching.length === 0) { - if (batchPaths.hasCombinedDiffConsumer) { - scheduleDebouncedEditorExternalReload(notification) - } + scheduleDebouncedEditorExternalReload(notification) continue } const dirtyMatches = matching.filter((file) => file.isDirty)