fix: Improve initial file tree load performance #739
Conversation
… & files that are visible
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR implements lazy-loading for the file tree feature by introducing path-specific API fetching, path validation utilities, and converting file tree components to use controlled expansion state management instead of internal state. Changes
Sequence DiagramsequenceDiagram
participant User
participant FileTreePanel
participant API
participant Utils
participant UI as PureFileTreePanel
User->>FileTreePanel: Navigate to repo/revision
FileTreePanel->>FileTreePanel: Initialize openPaths (empty)
User->>FileTreePanel: Click to expand node
FileTreePanel->>FileTreePanel: Add node path to openPaths
FileTreePanel->>API: getTree({repoName, revisionName, paths: [...]})
API->>Utils: Validate paths (isPathValid)
Utils-->>API: Paths valid
API->>Utils: Normalize paths
Utils-->>API: Normalized paths
API->>API: Execute git ls-tree with specific paths
API-->>FileTreePanel: Return FileTreeNode hierarchy
FileTreePanel->>Utils: buildFileTree(flatList)
Utils-->>FileTreePanel: Hierarchical tree
FileTreePanel->>FileTreePanel: Measure fetch duration
FileTreePanel->>FileTreePanel: Capture wa_file_tree_loaded event
FileTreePanel->>UI: Render with openPaths, onTreeNodeClicked
UI->>UI: Collapse/expand based on openPaths.has(path)
UI-->>User: Display tree
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
brendan-kellam
changed the title
brendan-kellam
changed the title
This comment has been minimized.
This comment has been minimized.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on February 14
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
packages/web/src/features/fileTree/components/fileTreePanel.tsx
Outdated
Show resolved
Hide resolved
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/web/src/features/fileTree/utils.test.ts`:
- Around line 33-41: The test description in utils.test.ts for the buildFileTree
unit test contains a grammar typo ("a empty flat list"); update the test title
string in the test(...) call to read "an empty flat list" so the test reads
test('buildFileTree handles an empty flat list', ...) referring to the existing
buildFileTree test case.
In `@packages/web/src/features/fileTree/utils.ts`:
- Around line 43-60: In buildFileTree, intermediate directory nodes are being
created with the full file path (item.path) causing incorrect navigation;
instead compute the node's path incrementally using the parts array and current
index (e.g., join parts.slice(0, i+1) with '/'), assign that computedPath when
creating a new FileTreeNode (use item.path only for leaf nodes where isLeaf is
true), and keep the rest of the logic (variables parts, isLeaf, nodeType, next,
current.children.push) the same so directory nodes have correct cumulative paths
for open-state and navigation matching.
♻️ Duplicate comments (2)
packages/web/src/features/fileTree/components/pureFileTreePanel.tsx (1)
71-77: Indent loading skeleton as a child node.
It should render atdepth + 1to align with the children that will appear there.🧩 Suggested tweak
- {node.type === 'tree' && node.children.length === 0 && openPaths.has(node.path) && renderLoadingSkeleton(depth)} + {node.type === 'tree' && node.children.length === 0 && openPaths.has(node.path) && renderLoadingSkeleton(depth + 1)}packages/web/src/features/fileTree/components/fileTreePanel.tsx (1)
185-199: LGTM!The rendering logic correctly handles the three states: pending (skeleton), error (error message), and success (tree panel). The error display issue mentioned in past reviews appears to have been addressed - the current
isPendingcheck properly allows errors to display when the initial load fails.
🧹 Nitpick comments (2)
packages/web/src/features/fileTree/api.ts (1)
11-13: Consider consolidating imports from the same module.The imports from
./utilsare split across two lines and could be consolidated for cleaner code.Suggested fix
import { FileTreeItem } from './types'; -import { buildFileTree, isPathValid, normalizePath } from './utils'; -import { compareFileTreeItems } from './utils'; +import { buildFileTree, compareFileTreeItems, isPathValid, normalizePath } from './utils';packages/web/src/features/fileTree/components/fileTreePanel.tsx (1)
99-113: Consider removingopenPathsfrom the dependency array.The callback re-creates on every
openPathschange. Since thehascheck can be done inside the functionalsetState, you can eliminate the dependency and improve memoization:Suggested fix
- const onTreeNodeClicked = useCallback((node: FileTreeNode) => { - if (!openPaths.has(node.path)) { - setOpenPaths(current => { - const next = new Set(current); - next.add(node.path); - return next; - }) - } else { - setOpenPaths(current => { - const next = new Set(current); - next.delete(node.path); - return next; - }) - } - }, [openPaths]); + const onTreeNodeClicked = useCallback((node: FileTreeNode) => { + setOpenPaths(current => { + const next = new Set(current); + if (current.has(node.path)) { + next.delete(node.path); + } else { + next.add(node.path); + } + return next; + }); + }, []);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
CHANGELOG.mdpackages/web/src/app/api/(client)/client.tspackages/web/src/features/fileTree/api.tspackages/web/src/features/fileTree/components/fileTreePanel.tsxpackages/web/src/features/fileTree/components/pureFileTreePanel.tsxpackages/web/src/features/fileTree/types.tspackages/web/src/features/fileTree/utils.test.tspackages/web/src/features/fileTree/utils.tspackages/web/src/lib/posthogEvents.ts
🧰 Additional context used
📓 Path-based instructions (1)
CHANGELOG.md
📄 CodeRabbit inference engine (.cursor/rules/cloud_agent.mdc)
After creating a GitHub PR, create a follow-up commit with a Changelog entry in CHANGELOG.md with a short description of the change. Follow the existing conventions: entries must be parented under a header (Added, Changed, Deprecated, Removed, Fixed, or Security), and entries must include the GitHub pull request id at the end of the line, formatted as #
Files:
CHANGELOG.md
🧠 Learnings (1)
📚 Learning: 2025-12-29T18:02:55.419Z
Learnt from: CR
Repo: sourcebot-dev/sourcebot PR: 0
File: .cursor/rules/cloud_agent.mdc:0-0
Timestamp: 2025-12-29T18:02:55.419Z
Learning: Applies to CHANGELOG.md : After creating a GitHub PR, create a follow-up commit with a Changelog entry in CHANGELOG.md with a short description of the change. Follow the existing conventions: entries must be parented under a header (Added, Changed, Deprecated, Removed, Fixed, or Security), and entries must include the GitHub pull request id at the end of the line, formatted as [#<id>](<url>)
Applied to files:
CHANGELOG.md
🧬 Code graph analysis (5)
packages/web/src/features/fileTree/utils.test.ts (1)
packages/web/src/features/fileTree/utils.ts (3)
normalizePath(3-22)isPathValid(26-33)buildFileTree(35-82)
packages/web/src/features/fileTree/utils.ts (1)
packages/web/src/features/fileTree/types.ts (2)
FileTreeNode(43-43)FileTreeItem(28-28)
packages/web/src/features/fileTree/api.ts (1)
packages/web/src/features/fileTree/utils.ts (3)
isPathValid(26-33)normalizePath(3-22)compareFileTreeItems(84-89)
packages/web/src/features/fileTree/components/fileTreePanel.tsx (4)
packages/backend/src/utils.ts (1)
measure(12-20)packages/web/src/lib/utils.ts (1)
unwrapServiceError(547-554)packages/web/src/features/fileTree/api.ts (1)
getTree(22-84)packages/web/src/features/fileTree/types.ts (1)
FileTreeNode(43-43)
packages/web/src/features/fileTree/components/pureFileTreePanel.tsx (1)
packages/web/src/features/fileTree/types.ts (1)
FileTreeNode(43-43)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: Cursor Bugbot
🔇 Additional comments (12)
packages/web/src/lib/posthogEvents.ts (1)
184-186: LGTM: new event shape is clear and typed.packages/web/src/features/fileTree/types.ts (1)
3-7: Type additions look consistent with the new API surface.Also applies to: 16-21, 53-54
CHANGELOG.md (1)
13-17: LGTM!The changelog entries follow the existing conventions with proper headers and include the required links. Note that the "Fixed" entry references the issue (
#531) rather than the PR (#739), which is acceptable since it directly addresses that issue.packages/web/src/features/fileTree/utils.test.ts (3)
4-14: LGTM!The
normalizePathtests correctly validate the core behaviors: stripping leading slashes, adding trailing slashes, and the edge case for root paths.
16-31: Good coverage of edge cases for path validation.The tests effectively distinguish between malicious directory traversal patterns (
..) and legitimate paths containing dot sequences (like...), which directly addresses issue#531.
43-93: LGTM!The test correctly validates tree construction and sorting behavior, ensuring directories appear before files and items are sorted alphabetically within each type.
packages/web/src/features/fileTree/api.ts (4)
39-41: Good security practice: validating paths before git operations.Path validation correctly happens before any git commands are executed, preventing directory traversal attacks.
48-62: LGTM!The git command construction properly uses
--to separate paths from options, includes the-tflag to capture directory nodes, and handles the path specifications correctly.
107-110: LGTM!Path validation and normalization in
getFolderContentsmirrors the pattern ingetTree, ensuring consistent security handling.
141-143: LGTM!Using the shared
compareFileTreeItemscomparator ensures consistent sorting behavior across the codebase.packages/web/src/features/fileTree/components/fileTreePanel.tsx (2)
74-76: LGTM!Correctly resets the open paths when switching repositories or revisions, preventing stale state from persisting.
80-95: LGTM!The logic correctly computes and opens all ancestor directories for the current path, ensuring the file is visible in the tree. For blob paths, it properly excludes the filename to open only the containing directory hierarchy.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| return next; | ||
| }) | ||
| } | ||
| }, [openPaths]); |
There was a problem hiding this comment.
Stale closure prevents folder toggle on rapid clicks
Low Severity
The onTreeNodeClicked callback checks openPaths.has(node.path) using the closure value of openPaths, but the state updates inside use functional updates with current. When a user clicks the same folder rapidly to toggle it, both clicks may see the stale openPaths from the closure and take the same branch (both try to add or both try to delete), preventing the expected toggle behavior. The conditional check needs to happen inside the functional update to use the actual current state.
Problem
For large repositories (100K+ files), the file tree could take a long time (10s+) to load. The primary bottleneck was because we were loading the entire file tree for the repository, even though a small subset would actually be rendered.
Solution
This PR improves things by only loading the subset of the file tree that is actually opened*, rather than loading everything. The tradeoff with this approach is that subsequent requests need to be made whenever a new folder is opened. When this happens, a "loading..." placeholder is rendered below the folder. Upon testing on a production build, this experience is still pretty snappy (was seeing 50ms load times). I've also added a
wa_file_tree_loadedposthog event that includes adurationMsso we can track the performance in the wild.*opened in this context doesn't necessarily mean rendered. If you open a nested directory and then collapse some parent directory, the nested directory will still be considered open but not rendered. This is needed since if you were to then expand the parent directory, the state of the opened directories should still be preserved.
Before
Screen.Recording.2026-01-15.at.2.06.59.PM.mov
After
Screen.Recording.2026-01-15.at.2.08.09.PM.mov
Fixes #651
Note
Speeds up initial file tree rendering by fetching only opened directories and deferring the rest; adds basic telemetry to measure load times.
getTreeAPI to acceptpathsand return a union tree; addgetFolderContents, plusnormalizePath/isPathValid/buildFileTreehelpers andcompareFileTreeItemssorter (with tests)FileTreePaneltracksopenPaths, queries tree on expand (React Query withstaleTime: Infinity, placeholder data), emitswa_file_tree_loadedwithdurationMs;PureFileTreePanelusesopenPathsand shows a loading skeleton for unopened/empty childrenwa_file_tree_loaded; update types/schemas accordingly; note performance improvement inCHANGELOG.mdWritten by Cursor Bugbot for commit 279e044. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.