perf(host-service): listBranches single git spawn instead of 4×N#4160
perf(host-service): listBranches single git spawn instead of 4×N#4160saddlepaddle merged 2 commits intomainfrom
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughTwo performance-related changes: the ChangeslistBranches Performance and Query Caching Integration
Sequence Diagram(s)sequenceDiagram
participant Hook as useDiffStats
participant Query as workspaceTrpc.git.getStatus
participant Cache as QueryCache
participant Event as git:changed
Hook->>Query: useQuery(refetchOnWindowFocus: false)
Query->>Cache: store status
Cache-->>Hook: return data or null
Event->>Cache: invalidate getStatus
Cache->>Query: auto-refetch
Query-->>Hook: updated status
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
`listBranches` was running 4 git child processes per local branch (`git config remote`, `git config merge`, `git rev-list --left-right`, `git log -1`) for an O(N) fanout, but the only renderer consumer (`BaseBranchSelector`) reads `branch.name` plus a UI check on `isHead`. With ~1k local branches that was ~3-4k spawns per call, sustained near-100% CPU on the host-service event loop. Replaced with a single `git for-each-ref refs/heads/ --format='%(HEAD)\t%(refname:short)'` call. Cost now O(1). `useDiffStats` was bypassing React Query — calling the imperative `client.git.getStatus.query()` per `git:changed` event, so each sibling consumer (every sidebar tile) issued its own uncached `getStatus`. Switched to `workspaceTrpc.git.getStatus.useQuery()` + invalidate-on-event so concurrent invalidations collapse to one refetch. Same update semantics, shared cache. Plan doc captures the diagnostic methodology (sample(1) → realising PID 2182 was the host-service via ELECTRON_RUN_AS_NODE → in-process spawn tracer → root cause).
saddlepaddle
force-pushed
the
debug-cpu-performance
branch
from
fc254aa to
89ef65a
Compare
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Greptile SummaryThis PR fixes a sustained near-100% CPU storm on the host-service caused by
Confidence Score: 3/5The listBranches fix is safe to merge; the useDiffStats migration introduces an unintended refetch-on-focus behaviour that partially undermines the load reduction. The host-service router change is clean and well-reasoned. The useDiffStats hook omits refetchOnWindowFocus: false, silently reintroducing refetch-on-focus for getStatus across every visible workspace tile. With multiple workspaces open, sustained app-focus events can produce the same concurrent git spawns the PR aims to eliminate. apps/desktop/src/renderer/hooks/host-service/useDiffStats/useDiffStats.ts — the missing refetchOnWindowFocus: false option needs a second look before merging.
|
| Filename | Overview |
|---|---|
| packages/host-service/src/trpc/router/git/git.ts | Replaces the 4×N-spawn buildBranch fan-out in listBranches with a single git for-each-ref call, correctly parsing %(HEAD)\t%(refname:short) to populate { name, isHead }. The narrowed return type flows correctly through tRPC inference to all consumers. |
| apps/desktop/src/renderer/hooks/host-service/useDiffStats/useDiffStats.ts | Migrates from a manual fetch/useState pattern to React Query via workspaceTrpc.git.getStatus.useQuery. The shared cache and invalidation approach is correct, but refetchOnWindowFocus is not explicitly disabled, so the hook now refetches getStatus on every window-focus event — behaviour that didn't exist before. |
| apps/desktop/plans/20260506-host-service-listbranches-cpu-storm.md | New investigation document with thorough diagnostic methodology. Status line and artifact-cleanup warning are stale: the fix is implemented in this PR and the debug files are already absent from the branch. |
Comments Outside Diff (2)
-
apps/desktop/plans/20260506-host-service-listbranches-cpu-storm.md, line 7 (link)Outdated status line and stale artifact warning
The header reads
Status: root cause confirmed, mitigation applied, fix not yet implemented, but this PR is the implementation. Readers who find this file after the merge will be misled into thinking the code fix is still pending. Similarly, the "Lingering artifacts in this branch" section (lines 141–148) warns thatspawn-trace.tsand theimport "./spawn-trace"line inindex.tsmust be removed before merging — but both are already absent from the branch.Prompt To Fix With AI
This is a comment left during a code review. Path: apps/desktop/plans/20260506-host-service-listbranches-cpu-storm.md Line: 7 Comment: **Outdated status line and stale artifact warning** The header reads `Status: root cause confirmed, mitigation applied, fix not yet implemented`, but this PR is the implementation. Readers who find this file after the merge will be misled into thinking the code fix is still pending. Similarly, the "Lingering artifacts in this branch" section (lines 141–148) warns that `spawn-trace.ts` and the `import "./spawn-trace"` line in `index.ts` must be removed before merging — but both are already absent from the branch. How can I resolve this? If you propose a fix, please make it concise.
-
packages/host-service/test/integration/git.integration.test.ts, line 20-31 (link)isHeadfield not covered by the integration testThe existing test only asserts that
branch.namevalues are present; it never checksisHead. The implementation now derivesisHeadfrom%(HEAD)in thefor-each-refoutput, and the parse logic has a fallback path (tab < 0 → isHead: false). A test asserting that the checked-out branch hasisHead: trueand"feature/x"hasisHead: falsewould pin the new parsing behaviour against future format-string changes.Prompt To Fix With AI
This is a comment left during a code review. Path: packages/host-service/test/integration/git.integration.test.ts Line: 20-31 Comment: **`isHead` field not covered by the integration test** The existing test only asserts that `branch.name` values are present; it never checks `isHead`. The implementation now derives `isHead` from `%(HEAD)` in the `for-each-ref` output, and the parse logic has a fallback path (`tab < 0 → isHead: false`). A test asserting that the checked-out branch has `isHead: true` and `"feature/x"` has `isHead: false` would pin the new parsing behaviour against future format-string changes. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
apps/desktop/src/renderer/hooks/host-service/useDiffStats/useDiffStats.ts:18-21
**Implicit `refetchOnWindowFocus` behavior change**
The old implementation only fetched on mount and on `git:changed` events — there was no window-focus refetch. `workspaceTrpc.git.getStatus.useQuery()` without an explicit `refetchOnWindowFocus: false` inherits React Query's default of `true`, so every time the user switches back to the app, each visible workspace tile issues its own `getStatus` call. `getStatus` spawns several git processes (status, numstat, diff, ls-files), so on window focus with N workspace tiles open the host-service still sees N concurrent `getStatus` spawns — exactly the multi-consumer refetch pattern the PR set out to fix. Passing `{ refetchOnWindowFocus: false }` alongside `{ enabled: Boolean(workspaceId) }` restores the previous event-only update semantics while keeping the shared-cache benefit.
### Issue 2 of 3
apps/desktop/plans/20260506-host-service-listbranches-cpu-storm.md:7
**Outdated status line and stale artifact warning**
The header reads `Status: root cause confirmed, mitigation applied, fix not yet implemented`, but this PR is the implementation. Readers who find this file after the merge will be misled into thinking the code fix is still pending. Similarly, the "Lingering artifacts in this branch" section (lines 141–148) warns that `spawn-trace.ts` and the `import "./spawn-trace"` line in `index.ts` must be removed before merging — but both are already absent from the branch.
### Issue 3 of 3
packages/host-service/test/integration/git.integration.test.ts:20-31
**`isHead` field not covered by the integration test**
The existing test only asserts that `branch.name` values are present; it never checks `isHead`. The implementation now derives `isHead` from `%(HEAD)` in the `for-each-ref` output, and the parse logic has a fallback path (`tab < 0 → isHead: false`). A test asserting that the checked-out branch has `isHead: true` and `"feature/x"` has `isHead: false` would pin the new parsing behaviour against future format-string changes.
Reviews (1): Last reviewed commit: "perf(host-service): listBranches single ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const { data: status } = workspaceTrpc.git.getStatus.useQuery( | ||
| { workspaceId }, | ||
| { enabled: Boolean(workspaceId) }, | ||
| ); |
There was a problem hiding this comment.
refetchOnWindowFocus behavior change
The old implementation only fetched on mount and on git:changed events — there was no window-focus refetch. workspaceTrpc.git.getStatus.useQuery() without an explicit refetchOnWindowFocus: false inherits React Query's default of true, so every time the user switches back to the app, each visible workspace tile issues its own getStatus call. getStatus spawns several git processes (status, numstat, diff, ls-files), so on window focus with N workspace tiles open the host-service still sees N concurrent getStatus spawns — exactly the multi-consumer refetch pattern the PR set out to fix. Passing { refetchOnWindowFocus: false } alongside { enabled: Boolean(workspaceId) } restores the previous event-only update semantics while keeping the shared-cache benefit.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/hooks/host-service/useDiffStats/useDiffStats.ts
Line: 18-21
Comment:
**Implicit `refetchOnWindowFocus` behavior change**
The old implementation only fetched on mount and on `git:changed` events — there was no window-focus refetch. `workspaceTrpc.git.getStatus.useQuery()` without an explicit `refetchOnWindowFocus: false` inherits React Query's default of `true`, so every time the user switches back to the app, each visible workspace tile issues its own `getStatus` call. `getStatus` spawns several git processes (status, numstat, diff, ls-files), so on window focus with N workspace tiles open the host-service still sees N concurrent `getStatus` spawns — exactly the multi-consumer refetch pattern the PR set out to fix. Passing `{ refetchOnWindowFocus: false }` alongside `{ enabled: Boolean(workspaceId) }` restores the previous event-only update semantics while keeping the shared-cache benefit.
How can I resolve this? If you propose a fix, please make it concise.The pre-RQ `useDiffStats` only fetched on mount and on `git:changed` events. Switching to `useQuery` without an explicit `refetchOnWindowFocus: false` inherited React Query's default of `true`, so every window focus would issue one `getStatus` per visible sidebar tile (each tile has its own `workspaceId` query key, so RQ won't dedupe across tiles). With 8 tiles open that's ~112 git spawns per focus event — exactly the multi-consumer fanout this hook was supposed to eliminate.
#4160 swapped useDiffStats from imperative client.git.getStatus.query() to workspaceTrpc.git.getStatus.useQuery(). That broke the v2 dashboard sidebar: workspaceTrpc.Provider only mounts inside v2-workspace/$id, but the sidebar is a sibling of that Outlet — so its useQuery calls had no provider, and even with one couldn't fan out to workspaces on different hosts. Resolve hostUrl per-workspaceId, call the imperative client inside a react-query queryFn keyed on (hostUrl, workspaceId). Concurrent callers (tile + hover overlay for the same workspace) still dedup via the global QueryClient. staleTime: Infinity since git:changed is push-authoritative.
1 participant
Summary
listBrancheswas running 4 sequentialgitchild processes per local branch (git config remote,git config merge,git rev-list --left-right,git log -1) for an O(N) fanout, but the only renderer consumer (BaseBranchSelectorin the v2 sidebar) readsbranch.nameplus a UI check onisHead. Replaced with a singlegit for-each-ref refs/heads/ --format='%(HEAD)\t%(refname:short)'call — O(1) regardless of branch count.useDiffStatswas bypassing React Query (calledclient.git.getStatus.query()directly), so each sidebar workspace tile issued its own uncachedgetStatuspergit:changedevent with no possibility of cache sharing. Switched toworkspaceTrpc.git.getStatus.useQuery()+ invalidate-on-event so concurrent invalidations from sibling consumers collapse to one refetch. Same update semantics, shared cache.ELECTRON_RUN_AS_NODE, in-process spawn tracer, root-cause).For a repo with ~1k local branches: previously ~3700 spawns per
listBranchescall, now 1. With the v2 sidebar polling every 30s and invalidating on git events, this was sustained near-100% CPU on the host-service event loop.Test plan
BaseBranchSelectorlists all local branchesgit:changedevent without N independent refetches (verify with network tab or spawn-trace-style instrumentation)Summary by cubic
Cuts host-service CPU by collapsing
git.listBranchesto one spawn and moving diff stats to a shared React Query cache. Prevents multi-tile refetches on window focus and keeps the sidebar responsive.git.listBranchesnow usesgit for-each-ref refs/heads/ --format='%(HEAD)\t%(refname:short)'to return{ name, isHead }in one spawn.useDiffStatsswitches toworkspaceTrpc.git.getStatus.useQuery()with invalidate ongit:changed, so sibling consumers share one refetch.refetchOnWindowFocusto match previous behavior and avoid per-tile refetch storms.Written for commit 7586b45. Summary will update on new commits.
Summary by CodeRabbit