Skip to content

perf(server): sidebar status rows skip the Changes totals they never show - #32351

Open
Yuruzuu wants to merge 1 commit into
pingdotgg:mainfrom
Yuruzuu:perf/status-burst-branch-totals
Open

Yuruzuu wants to merge 1 commit into
pingdotgg:mainfrom
Yuruzuu:perf/status-burst-branch-totals

Conversation

@Yuruzuu

@Yuruzuu Yuruzuu commented Nov 7, 2026

Copy link
Copy Markdown
Contributor

Problem

Every visible sidebar thread row subscribes to local git status, and each one paid for the branch-wide Changes totals. Only the thread header shows those totals. On startup, reconnect, or when scrolling the sidebar, a burst of rows ran 16 git processes each instead of 3. That competed for CPU and disk with whatever the user was waiting on, including the turn-end checkpoint capture. On Windows, in a 31K-file repo, 30 concurrent sidebar loads took 3.0 s.

Change

The includeRemote: false subscribers (SidebarThreadRow.tsx twice, ThreadStatusIndicators.tsx once) read only the branch name and PR fields. branchChanges is already optional in the contract, so they can go without it.

  • GitManager.localStatus takes { includeBranchChanges?: boolean }, defaulting to true. Reads with and without totals use separate cache entries, so a light result is never served to a caller that shows totals. Invalidation clears both.
  • VcsStatusBroadcaster counts, per cwd, who needs totals: full subscribers plus in-flight getStatus and refreshStatus calls. Status reads compute totals only while that count is above zero.
  • Each cached status records whether it carries totals. A header that finds a cached status without totals triggers one read with them.
  • If a header subscribes while a light read is running, the read is repeated with totals before it is published, so the header never receives a status without them.

Callers that don't pass the option, including turn start, MCP, and auto-pull, keep the full read. Web, desktop, and mobile need no changes, and no contract changes. #29155, #26036, and #18842 also edit VcsStatusBroadcaster.ts; I expect only small text conflicts.

Scope and approval

This qualifies for the small, focused fix exception in CONTRIBUTING.md. It stops computing data that the requesting client never displays, without changing what any client shows, product defaults, or contracts.

Verification

  • vp test run on the VcsStatusBroadcaster, GitManager, GitWorkflowService and RunFinalizationService tests: 153 passed, 2 skipped. The same 2 skip on main.
  • "reads Changes totals only while a subscriber shows them" fails on main, because the sidebar snapshot carries branchChanges.
  • "a light refresh that a header joins mid-read publishes Changes totals" fails when the repeated read is removed.
  • Server typecheck, lint, and format are clean.
  • Git processes per sidebar status (GIT_TRACE2, warm): 16 → 3.

Measured with the real GitVcsDriver and VcsProcess on Windows 11: a 31,771-file clone, N concurrent local statuses on one cwd plus one checkpoint capture, alternating before and after, medians.

Case Checkpoint capture during burst Whole burst
N=10 (7 runs) 876 → 668 ms 1,046 → 437 ms
N=30, capture starts 1 s in (5 runs) 838 → 480 ms 2,984 → 1,120 ms
N=30, capture starts with burst (7 runs) 1,101 → 1,098 ms 3,005 → 1,176 ms

In the last row the capture overlaps only the first git commands of each status, which run either way. The harness calls the git driver directly. It leaves out GitManager's 1 s result cache and the hosting-provider lookup. It also uses one cwd, whereas real sidebar rows usually point at different worktrees.

Not checked: a run in a real client.

Made with Claude Opus 5.5 via Claude Code in T3 Code.

🤖 Generated with Claude Code

…show

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:L 100-499 changed lines (additions + deletions). labels Nov 7, 2026
@macroscopeapp

macroscopeapp Bot commented Nov 7, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at e1dd022

Macroscope's review found this PR approvable — Passive status rows skip unused Changes totals while full subscriptions and direct reads retain them. The change is limited to read-only status computation and in-memory caching, with no persistent-data or product-default changes.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Nov 7, 2026

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

Local status reads can now omit branch-change totals. The status broadcaster tracks when totals are required and adjusts reads, cache reuse, and refresh sharing to match active demand.

Changes: Local status totals

Layer / File(s) Summary
Local status options and cache
apps/server/src/git/GitManager.ts, apps/server/src/git/GitWorkflowService.ts
GitManager.localStatus accepts an option to omit branch-change totals and maintains separate caches for reads with and without totals. GitWorkflowService.localStatus forwards the option for Git repositories.
Broadcaster demand and refresh
apps/server/src/vcs/VcsStatusBroadcaster.ts, apps/server/src/vcs/VcsStatusBroadcaster.test.ts
The broadcaster acquires and releases branch-change demand for direct reads, full refreshes, and eligible streams. It checks whether cached and shared refresh results include required totals. Tests cover subscriber changes and demand arriving during a read.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Refactor

Sequence Diagram(s)

sequenceDiagram
  participant StatusCaller
  participant VcsStatusBroadcaster
  participant GitWorkflowService
  participant GitManager
  StatusCaller->>VcsStatusBroadcaster: Request status or open eligible stream
  VcsStatusBroadcaster->>VcsStatusBroadcaster: Acquire branch-change demand
  VcsStatusBroadcaster->>GitWorkflowService: Read local status with current demand
  GitWorkflowService->>GitManager: Forward local-status options
  GitManager->>GitManager: Select cache with or without branch-change totals
  GitManager-->>GitWorkflowService: Return local status
  GitWorkflowService-->>VcsStatusBroadcaster: Return local status
  VcsStatusBroadcaster-->>StatusCaller: Return status
Loading

Suggested reviewers: juliusmarminge, yash-singh1, t3dotgg

Merge Risk: Minimal · up to e1dd0

Sidebar-only status reads can skip Changes totals while headers continue to receive them. No merge-blocking issue was identified; merge after normal checks.

Pre-merge checks: Passed 4

Check name Status Explanation
Title check Passed The title names the main change: sidebar status rows stop requesting the branch-wide Changes totals that they never display. It matches the raw summary, which adds an includeBranchChanges option to Gi…
Description check Passed The description follows the template's sections: Problem, Change, Scope and approval, and Verification. The Problem section states the expected behavior and the observed cost (16 git processes per row…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title names the main change: sidebar status rows stop requesting the branch-wide Changes totals that they never display. It matches the raw summary, which adds an includeBranchChanges option to GitManager.localStatus and has VcsStatusBroadcaster compute totals only when a header or in-flight request needs them. The title follows the conventional commit form (perf(server)) and stays under the usual length. It does not mention the caching and repeat-read mechanics, which is acceptable because it is not required to cover every detail.

Full details: Description check

Explanation

The description follows the template's sections: Problem, Change, Scope and approval, and Verification. The Problem section states the expected behavior and the observed cost (16 git processes per row instead of 3, and 3.0 s for 30 concurrent sidebar loads on Windows in a 31K-file repo). The Change section explains the option, the cache separation, the demand counting, and why web, desktop, and mobile need no changes. The Scope and approval section cites the small, focused fix exception and explains why it qualifies. The Verification section lists the test commands, the tests that fail on main, the process-count result, and the Windows benchmark table, and it states what was not checked (a run in a real client). The description includes a conventional commit-style title example from the template as a comment, which is not a problem. The author also states that the change does not alter any contract. Minor gaps: the Scope section does not link a triaged issue or maintainer approval, but the author explicitly claims the small-fix exemption, which the template allows. The description is mostly complete, so the check passes.

Finishing Touches

Generate unit tests (beta)

  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Recent review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between d4247a5 and e1dd022.

Files selected for processing: 4

  • apps/server/src/git/GitManager.ts
  • apps/server/src/git/GitWorkflowService.ts
  • apps/server/src/vcs/VcsStatusBroadcaster.test.ts
  • apps/server/src/vcs/VcsStatusBroadcaster.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L 100-499 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant