fix(file-explorer): batch dirCache writes on tree refresh to remove O(N²) freeze#6321
fix(file-explorer): batch dirCache writes on tree refresh to remove O(N²) freeze#6321wolfiesch wants to merge 3 commits into
Conversation
…(N²) freeze
refreshTree() reloaded every expanded directory via Promise.all(expanded.map(loadDir)),
and each loadDir did two full { ...prev } spreads of the whole dirCache (a loading
commit and a result commit). With N expanded dirs that is 2N shallow-copies of a
growing N-key object — O(N²). A renderer CPU profile during a watcher-overflow
refresh measured ~7.3s of contiguous main-thread block at N≈6000, entirely in those
two setDirCache spread closures.
Route the expanded-dir refresh through refreshFileExplorerExpandedDirs(), which loads
all dirs concurrently and commits exactly two batched setDirCache updates (one loading,
one result) regardless of N — O(N). Root load and the public hook surface are unchanged.
Add a unit test asserting the batched two-commit behavior.
There was a problem hiding this comment.
Pull request overview
Refactors the File Explorer tree refresh path to eliminate an O(N²) renderer freeze caused by repeated dirCache object spreads when reloading many expanded directories, by batching dirCache updates during refresh.
Changes:
- Introduces
refreshFileExplorerExpandedDirs()to concurrently reload all expanded directories while committing exactly twosetDirCacheupdates (loading + results). - Extracts entry→node conversion into a shared
entriesToTreeNodes()helper used by bothloadDir()and the batched refresh. - Adds a unit test covering the “two cache commits” batching behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/renderer/src/components/right-sidebar/useFileExplorerTree.ts | Routes expanded-dir refresh through a batched loader to avoid per-directory dirCache spreads; shares node-building logic via entriesToTreeNodes. |
| src/renderer/src/components/right-sidebar/useFileExplorerTree.test.ts | Adds a unit test asserting the expanded-dir refresh does one loading cache commit and one result cache commit. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe file explorer tree refresh flow now uses a shared helper to convert directory entries into tree nodes, refreshes expanded directories concurrently, and preserves loading cache state while reads are in progress. The refresh path now calls the new helper with runtime-backed directory reads, and a test covers the cache commit sequence and directory read counts. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/renderer/src/components/right-sidebar/useFileExplorerTree.ts (2)
83-136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a short why-comment for the two batched cache commits.
This helper intentionally waits and commits loading/results in two batches to avoid the prior O(N²) cache-copy path; that constraint is non-obvious from the code alone.
Proposed comment
+ // Why: expanded refresh can touch many directories; keep cache writes batched + // so refreshing large worktrees stays O(N) instead of copying per directory. setDirCache((prev) => {As per coding guidelines, “When writing or modifying code driven by a design doc or non-obvious constraint, add a comment explaining why the code behaves the way it does.”
Source: Coding guidelines
260-264: 🚀 Performance & Scalability | 🔵 TrivialReduce redundant root re-scan when refreshing the file explorer.
Line 256 explicitly force-loads the root directory (
worktreePath). Lines 260–264 then map the fullexpandedset intoexpandedDirs, which triggersrefreshFileExplorerExpandedDirsto queue a second load for the same root path if it happens to be present inexpanded.Filter
worktreePathout before mapping to prevent the duplicated read operation.Diff context
const expandedDirs = Array.from(expanded).map((dirPath) => ({ dirPath, depth: splitPathSegments(dirPath.slice(worktreePath.length + 1)).length - 1 }))
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 66568433-27d0-44c4-ba68-778d6fc2e284
📒 Files selected for processing (2)
src/renderer/src/components/right-sidebar/useFileExplorerTree.test.tssrc/renderer/src/components/right-sidebar/useFileExplorerTree.ts
Summary
The File Explorer freezes for several seconds after a filesystem watcher overflow on large worktrees.
refreshTree()reloaded every expanded directory viaPromise.all(expanded.map(loadDir)), and eachloadDirdoes two full{ ...prev }spreads of the entiredirCache(aloadingcommit and a result commit). With N expanded dirs that is 2N shallow-copies of a growing N-key object — O(N²).This routes the expanded-dir refresh through
refreshFileExplorerExpandedDirs(), which loads all expanded dirs concurrently and commits exactly two batchedsetDirCacheupdates (one loading, one result) regardless of N — O(N). Root load and the public hook surface are unchanged.Screenshots
No visual change — refresh behavior is identical; only the
dirCachestate-update batching changed.Testing
pnpm typecheck(all three configs, clean)pnpm test— newuseFileExplorerTree.test.tsgreen; the right-sidebar suite shows no new failures (8 pre-existingpr-comment-*failures were confirmed present on a cleanmaincheckout and are unrelated to this change)pnpm build(desktop + native, clean)Evidence
A renderer CPU profile (V8
Profilervia CDP) captured during a real watcher-overflow refresh measured ~7.3 s of contiguous renderer-main-thread block at N≈6000 expanded dirs, entirely inside the twosetDirCachespread closures ofloadDir. The flattened-rows reprojection (visitChildren) measured ~10 ms (noise) and is not a contributor at this scale.This freeze is renderer-side; the off-thread stat fanout work (PR #5310 / #5527) does not touch this code path. (A separate standalone watcher benchmark exists as related evidence for moving watcher work off the main thread — it is not the justification for this change.)
AI Review Report
Reviewed with an AI coding agent. Checks run: the change is a pure renderer-side refactor of React state batching with no IPC, path-handling, command-execution, auth, secrets, or dependency surface touched. Cross-platform: no platform-specific code, shortcuts, labels, shell behavior, or Electron platform differences are involved (renderer state only). Verified the public hook return surface (
refreshTree/loadDir/setDirCache/refreshDir) is unchanged and both consumers (FileExplorer.tsx,file-explorer-watcher-reconcile.ts) are unaffected. The load-token/session cancellation semantics from the originalloadDirare preserved in the batched path.Security Audit
No new input handling, command execution, path handling, auth, secrets, dependency, or IPC surface. The change only alters how directory-listing results (already fetched by the existing
readRuntimeDirectorypath) are committed to component-local React state. No follow-up needed.Notes
visitChildren(flattened-row reprojection) was profiled as ~10 ms noise at N≈6000 and is intentionally left unchanged; if a much larger tree ever makes it a contributor, diffing the changed subtree instead of re-walking would address it, but there is no current evidence it matters.