perf(desktop): fix diff sidebar performance with many commits#1531
Merged
Conversation
…owth The custom rangeExtractor accumulated all previously rendered indices in a ref and never removed them, causing every file diff section to stay mounted permanently after scrolling past it. This defeated virtualization entirely — with many commits/files, the DOM grew unboundedly with IntersectionObservers, tRPC queries, and diff viewers for every item. Switch to defaultRangeExtractor with a fixed overscan count so items outside the visible window are properly unmounted.
Each poll runs ~8 git commands (status, rev-list, log, diff name-status, diff numstat x3, plus reading untracked files). At 2.5s intervals this saturates the Node process and blocks the UI. 10s is sufficient since the user is reading diffs, not actively making changes.
With many commits, expanding all by default creates a VirtualizedFileList and getCommitFiles query per commit on mount. Collapse by default so only the sections the user clicks into are loaded.
Structural sharing in React Query prevents re-renders when data hasn't changed, so the polling interval isn't the perf bottleneck — the broken virtualization was. Keep 2.5s for snappy updates.
Multiple consumers poll getStatus (ChangesContent at 2.5s, sidebar on hover). Each call runs ~8 git commands. Add a 2s TTL cache keyed by worktreePath+defaultBranch so duplicate calls within the window return the cached result without spawning any git processes.
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on March 14. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
Contributor
📝 WalkthroughWalkthroughThis PR optimizes data fetching and state management across the desktop application. It adds server-side caching to Git status operations with TTL-based invalidation, defers commit file fetching until the commit is expanded, and simplifies virtualizer range extraction by using the react-virtual library's default behavior instead of custom logic. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/lib/trpc/routers/changes/status.ts`:
- Around line 14-19: statusCache never evicts entries causing unbounded growth;
when writing new entries (the code that sets statusCache for a
worktreePath:defaultBranch key) add eviction: remove any entries with timestamp
older than STATUS_CACHE_TTL_MS and enforce a cap (e.g. introduce
MAX_STATUS_CACHE_ENTRIES = 100) by deleting the oldest timestamped entries until
size <= cap. Implement this pruning logic in the same place that sets
statusCache so you call it before/after inserting the new {result,timestamp},
referencing STATUS_CACHE_TTL_MS and statusCache to locate where to add the
cleanup.
- Around line 34-38: Export a cache invalidation helper and call it from each
status-mutating RPC: add and export a function clearStatusCache() that clears
the in-memory statusCache (the Map used alongside STATUS_CACHE_TTL_MS and
cacheKey logic that reads cached.result) and then invoke clearStatusCache() at
the end of each mutation handler (stage, unstage, discard, commit) so the next
status() call bypasses stale entries; ensure the helper is imported where those
mutation resolvers are defined and call it after the mutation completes (success
path).
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/InfiniteScrollView/components/CommitSection/CommitSection.tsx`:
- Around line 24-30: The commit files query currently leaves the expanded commit
area empty because commitFiles is undefined while loading; update the component
that calls electronTrpc.changes.getCommitFiles.useQuery (referencing
commitFiles, isCommitExpanded and the local files variable used in the
files.length > 0 guard) to render a small spinner/skeleton when the query is
loading and the commit is expanded (i.e., when isLoading && isCommitExpanded)
instead of letting the area stay blank; ensure the spinner is shown only during
loading and that normal files rendering remains unchanged once commitFiles
resolves.
🧹 Nitpick comments (3)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/changes/status.ts`: - Around line 14-19: statusCache never evicts entries causing unbounded growth; when writing new entries (the code that sets statusCache for a worktreePath:defaultBranch key) add eviction: remove any entries with timestamp older than STATUS_CACHE_TTL_MS and enforce a cap (e.g. introduce MAX_STATUS_CACHE_ENTRIES = 100) by deleting the oldest timestamped entries until size <= cap. Implement this pruning logic in the same place that sets statusCache so you call it before/after inserting the new {result,timestamp}, referencing STATUS_CACHE_TTL_MS and statusCache to locate where to add the cleanup. - Around line 34-38: Export a cache invalidation helper and call it from each status-mutating RPC: add and export a function clearStatusCache() that clears the in-memory statusCache (the Map used alongside STATUS_CACHE_TTL_MS and cacheKey logic that reads cached.result) and then invoke clearStatusCache() at the end of each mutation handler (stage, unstage, discard, commit) so the next status() call bypasses stale entries; ensure the helper is imported where those mutation resolvers are defined and call it after the mutation completes (success path). In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/InfiniteScrollView/components/CommitSection/CommitSection.tsx`: - Around line 24-30: The commit files query currently leaves the expanded commit area empty because commitFiles is undefined while loading; update the component that calls electronTrpc.changes.getCommitFiles.useQuery (referencing commitFiles, isCommitExpanded and the local files variable used in the files.length > 0 guard) to render a small spinner/skeleton when the query is loading and the commit is expanded (i.e., when isLoading && isCommitExpanded) instead of letting the area stay blank; ensure the spinner is shown only during loading and that normal files rendering remains unchanged once commitFiles resolves.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/InfiniteScrollView/components/CommitSection/CommitSection.tsx (1)
24-30: Minor UX gap: no loading indicator while commit files are being fetched.When the user expands a commit,
commitFilesisundefineduntil the query resolves, sofilesis[]and thefiles.length > 0guard on Line 54 renders nothing. The section appears expanded (chevron rotates) but shows an empty area until data arrives. Consider showing a small spinner or skeleton whileisLoading && isCommitExpandedto provide feedback.This is low priority given the perf focus of this PR.
Also applies to: 54-66
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/InfiniteScrollView/components/CommitSection/CommitSection.tsx` around lines 24 - 30, The commit files query currently leaves the expanded commit area empty because commitFiles is undefined while loading; update the component that calls electronTrpc.changes.getCommitFiles.useQuery (referencing commitFiles, isCommitExpanded and the local files variable used in the files.length > 0 guard) to render a small spinner/skeleton when the query is loading and the commit is expanded (i.e., when isLoading && isCommitExpanded) instead of letting the area stay blank; ensure the spinner is shown only during loading and that normal files rendering remains unchanged once commitFiles resolves.apps/desktop/src/lib/trpc/routers/changes/status.ts (2)
14-19: Cache entries are never evicted — minor unbounded growth.The
statusCacheMap accumulates entries for every uniqueworktreePath:defaultBranchpair and never removes them. For a desktop app this is practically bounded (few worktrees), but for correctness consider pruning stale entries periodically or capping the map size.♻️ Optional: prune stale entries on cache write
statusCache.set(cacheKey, { result, timestamp: Date.now() }); + + // Prune expired entries to prevent unbounded growth + for (const [key, entry] of statusCache) { + if (Date.now() - entry.timestamp >= STATUS_CACHE_TTL_MS) { + statusCache.delete(key); + } + } + return result;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/changes/status.ts` around lines 14 - 19, statusCache never evicts entries causing unbounded growth; when writing new entries (the code that sets statusCache for a worktreePath:defaultBranch key) add eviction: remove any entries with timestamp older than STATUS_CACHE_TTL_MS and enforce a cap (e.g. introduce MAX_STATUS_CACHE_ENTRIES = 100) by deleting the oldest timestamped entries until size <= cap. Implement this pruning logic in the same place that sets statusCache so you call it before/after inserting the new {result,timestamp}, referencing STATUS_CACHE_TTL_MS and statusCache to locate where to add the cleanup.
34-38: Add server-side cache invalidation for status mutations (stage, unstage, discard, commit).The server-side
statusCache(2s TTL) is not invalidated when mutations occur. With a 2.5s polling interval, a user staging a file can see stale unstaged status until the cache expires. Export aclearStatusCachehelper function and call it after each mutation (stage, unstage, discard, commit) to ensure the next poll always fetches fresh data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/changes/status.ts` around lines 34 - 38, Export a cache invalidation helper and call it from each status-mutating RPC: add and export a function clearStatusCache() that clears the in-memory statusCache (the Map used alongside STATUS_CACHE_TTL_MS and cacheKey logic that reads cached.result) and then invoke clearStatusCache() at the end of each mutation handler (stage, unstage, discard, commit) so the next status() call bypasses stale entries; ensure the helper is imported where those mutation resolvers are defined and call it after the mutation completes (success path).
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
rangeExtractorinVirtualizedFileListaccumulated all rendered indices and never released them, defeating virtualization entirelyChanges
rangeExtractor(which grew unboundedly) withdefaultRangeExtractor+ fixed overscan, so items outside the viewport are properly unmountedVirtualizedFileList+getCommitFilesquery per commit immediatelygetStatusto debounce overlapping git status polls from multiple consumers (ChangesContent polling + sidebar hover)Test Plan
Summary by CodeRabbit
Release Notes
Performance
Changes