Skip to content

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

Merged
juliusmarminge merged 7 commits into
pingdotgg:mainfrom
Yuruzuu:perf/status-burst-branch-totals
Nov 15, 2026
Merged

juliusmarminge merged 7 commits into
pingdotgg:mainfrom
Yuruzuu:perf/status-burst-branch-totals

Conversation

@Yuruzuu

@Yuruzuu Yuruzuu commented Nov 7, 2026 •

Copy link
Copy Markdown
Contributor

Problem

The command palette's thread rows (ThreadRowLeadingStatus) subscribe to local git status with includeRemote: false to show each thread's PR state. Every one of those subscriptions also paid for the branch-wide Changes totals, which only the thread header shows. Opening the palette over many threads ran 16 git processes per row instead of 3, competing for CPU and disk with whatever the user was waiting on, including the turn-end checkpoint capture.

#32947 moved the sidebar's branch labels to a branch-only stream, so this PR now covers only the palette rows.

Change

branchChanges is already optional in the contract, so includeRemote: false subscribers 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, including the detailed path of refreshBranchStatus, 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 rereads 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. No client or contract changes.

Scope and approval

Cut down at a maintainer's request in review. It stops computing data the requesting client never displays, without changing what any client shows, product defaults, or contracts.

Verification

  • vp test run on the VcsStatusBroadcaster, GitManager, and GitWorkflowService tests: all pass apart from 2 skips that also skip on main.
  • "reads Changes totals only while a subscriber shows them" fails on main, because the light snapshot carries branchChanges.
  • "a light refresh that a header joins mid-read publishes Changes totals" fails when the repeated read is removed.
  • "serializes branch reads with detailed refresh publication" (from perf(sidebar): avoid full git status for branch labels #32947) now reads through a light subscriber, because a header after a light refresh correctly rereads for totals.
  • Server typecheck, lint, and format are clean.
  • Git processes per light status (GIT_TRACE2, warm): 16 → 3.

Measured before the rebase, with the real GitVcsDriver and VcsProcess on Windows 11: a 31,771-file clone, N concurrent light statuses on one cwd plus one checkpoint capture, medians. The harness calls the git driver directly and leaves out GitManager's 1 s result cache.

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

Not checked: a run in a real client.

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

🤖 Generated with Claude Code

@github-actions github-actions Bot added vouch:unvouched 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 →

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • Resume reviews
  • Trigger review

No actionable comments were generated in the recent review.

Walkthrough

Local status reads can omit branch-change totals. The broadcaster tracks when totals are required and adjusts reads, cache reuse, and refresh-result 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 scoped demand for branch-change totals, uses demand-aware reads, and checks cached and shared results against current demand. Tests cover subscriber changes, demand arriving during a read, and status snapshots after serialized refreshes.

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 using current demand
  GitWorkflowService->>GitManager: Forward local-status options
  GitManager->>GitManager: Select cache with or without branch-change totals
  GitManager-->>VcsStatusBroadcaster: Return local status
  VcsStatusBroadcaster-->>StatusCaller: Return status
Loading

Suggested reviewers: juliusmarminge, t3dotgg, yash-singh1

Merge Risk: Minimal · previous review f8d5e

Command-palette status rows skip the expensive branch-change totals, while headers that show them still receive them. I found no concrete correctness or availability risk in this change, so it looks ready to merge after normal checks.

Pre-merge checks: Passed 4

Check name Status Explanation
Title check Passed The title names the main change: command palette status rows now skip the Changes branch totals they never display. It uses the conventional commit format and matches the description and the objective…
Description check Passed The description covers the problem, the change, scope and approval, and verification. It follows the template headings and gives specific test names, results, and benchmark numbers. The Problem and Ch…
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: command palette status rows now skip the Changes branch totals they never display. It uses the conventional commit format and matches the description and the objectives. It is specific, so a teammate scanning history can identify the primary change.

Full details: Description check

Explanation

The description covers the problem, the change, scope and approval, and verification. It follows the template headings and gives specific test names, results, and benchmark numbers. The Problem and Change sections state the cause and the fix. Scope explains that the change was narrowed at a maintainer's request. Verification lists focused tests and measured git process counts. It does not include screenshots, which the template requires only for UI changes, and it does not state the model and harness in the required closing line. Its Problem section also omits a reproduction environment, though it names the environment (Windows 11) in the verification section. These gaps are minor, so the description is mostly complete.

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 7d21a42 and f8d5ef4.

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; 7 remain after this review.

@juliusmarminge juliusmarminge left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Yuruzuu for the careful performance work! #32947 (0d809db501) now avoids full status reads for the main sidebar branch labels. Could you cut this PR down to the remaining ThreadRowLeadingStatus / command-palette status consumer, which still requests full local status, and retain the coverage needed for that path?

…y never show

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Yuruzuu
Yuruzuu force-pushed the perf/status-burst-branch-totals branch from e1dd022 to c3a534c Compare November 9, 2026 10:29
@Yuruzuu Yuruzuu changed the title perf(server): sidebar status rows skip the Changes totals they never show perf(server): command palette status rows skip the Changes totals they never show Nov 9, 2026
@Yuruzuu

Yuruzuu commented Nov 9, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @juliusmarminge. Rebased onto #32947 and cut down to the ThreadRowLeadingStatus / command-palette consumer. The server change stays, since that row still subscribes with includeRemote: false. Its light reads now skip the Changes totals, and headers still always get them. Reads now run under #32947's localRefreshLocks, so a cached status that's missing totals is reread inline instead of going through refreshLocalStatusCore (that would deadlock on the same lock). The detailed path of refreshBranchStatus goes through the same demand check. Coverage: the two totals tests are kept (one now registers the joining header before releasing the read, because the lock serializes it). Your 'serializes branch reads' test now asserts through a light subscriber, because a header that finds a light cached status correctly rereads for totals.

Yuruzuu and others added 3 commits November 10, 2026 18:30
# Conflicts:
#	apps/server/src/vcs/VcsStatusBroadcaster.ts
# Conflicts:
#	apps/server/src/vcs/VcsStatusBroadcaster.test.ts
#	apps/server/src/vcs/VcsStatusBroadcaster.ts
@Yuruzuu

Yuruzuu commented Nov 13, 2026

Copy link
Copy Markdown
Contributor Author

@juliusmarminge Ready for another look. As described above, this is cut down to the ThreadRowLeadingStatus / command-palette consumer on top of #32947, and it's merged with main since. GitHub won't let me re-request your review from a fork, hence the ping.

# Conflicts:
#	apps/server/src/vcs/VcsStatusBroadcaster.ts
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@juliusmarminge juliusmarminge added the takeover:in-progress A takeover lead is working on this PR label Nov 15, 2026
…nch-totals

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@juliusmarminge
juliusmarminge force-pushed the perf/status-burst-branch-totals branch from ef40392 to 3910b3a Compare November 15, 2026 04:53
@juliusmarminge

Copy link
Copy Markdown
Member

Evidence

On the same Windows Server 2025 machine, seed ten Replay threads in separate sparse worktrees retaining a real 30-commit branch difference, one modified README.md and one untracked file. Restart the real app per trial, open General settings, press Ctrl+K and search Palette row. The normal 1440×1080 dark viewport subscribes six rows. Measure first local-only WebSocket status request → last initial snapshot. Eight paired trials, with order alternating within two batches.

Before After
e9d8ac5673 3910b3ad6b
Before: measured delivery and real palette After: measured delivery and real palette
Median 4,980 ms; range 4,835–5,859 ms Median 2,119 ms; range 1,952–2,416 ms
Keyboard → snapshots median 5,694 ms; 135 Git starts per delivery window Keyboard → snapshots median 2,818 ms; 57 Git starts per delivery window
Before recording After recording

Delivery improves 57.4%. The 2,860 ms median improvement exceeds both full run ranges (1,024 ms and 464 ms); the ranges do not overlap. All sixteen normalized branch/dirty/working-tree results match. Displayed Completed labels and branch names are unchanged. This measures status delivery, not palette first-paint latency.

Seed/start/stop driver · Browser measurement script · Capture renderer · All samples · Summary · Reproduction notes

Not shown:

  • Concurrent checkpoint capture during a Replay turn and the N=10/N=30 direct-driver medians were not measured; this proof measures six real palette subscriptions with completed Replay-seeded threads.
  • The claimed 3-versus-16 processes per warm light read is unverified: measured 135-versus-57 whole-window app counts include overhead, with a fresh server per trial.
  • Original full-checkout timings were not reproduced; README-only sparse checkouts retained the real branch history.

@juliusmarminge juliusmarminge added takeover:ready Takeover done; auto-lands when gates pass and removed takeover:in-progress A takeover lead is working on this PR labels Nov 15, 2026
@juliusmarminge
juliusmarminge merged commit a5d21a3 into pingdotgg:main Nov 15, 2026
49 checks passed
@juliusmarminge juliusmarminge removed the takeover:ready Takeover done; auto-lands when gates pass label Nov 15, 2026
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 Author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants