perf(desktop): key sidebar git-watch off agent activity, not every row - #6856
Open
AviPeltz wants to merge 8 commits into
Open
perf(desktop): key sidebar git-watch off agent activity, not every row#6856AviPeltz wants to merge 8 commits into
AviPeltz wants to merge 8 commits into
Conversation
DashboardSidebarWorkspaceStatusProvider called watchGit for every sidebar row to keep diff-stat caches warm. Verified live against real data in #6848: opening the dashboard alone (no workspace opened) caused 79 of 90 non-archived workspaces to become watched, since the sidebar is normally mounted for the whole session — the load and terminal-lag scenario #6729 set out to fix, reintroduced through a different subscriber. An idle workspace's git state can't change without user interaction, so its diff count doesn't need to stay live — it only needs to be correct the next time it's opened, which useDiffStats' normal query fetch already handles. Non-active rows don't render a diff count today anyway (only the active row does, per the existing `entries` memo), so losing liveness for an idle row costs a first-open loading flash, not a wrong number on screen. Now only workspaces with a currently running/blocked/attention-needing agent (working/permission/review/failed — any non-idle PaneStatus) or the active workspace hold live git-watch interest. The signal is already computed here (bindingRowsByIndex + deriveTerminalAgentStatus, the same data driving the status dot) — no new plumbing. The full lifecycle-listener pass (agent:lifecycle/terminal:lifecycle/git:changed) still covers every row regardless, since that's what detects a row transitioning into activity in the first place; only the watchGit/unwatchGit calls — the ones with real host-side cost — are now gated. Split into two effects so the (still full-coverage, cheap) listener registration doesn't churn every time an unrelated workspace's terminal status flips: one for listeners (unchanged, keyed on the full target list), one for git-watch diffing (new, keyed on a fingerprint-stabilized "currently worth watching" set), plus the existing final-unmount cleanup. Verified: typecheck and biome clean; no automated test exists for this component (none did before this change either). Logic re-reviewed by hand against the exact structure verified live via CDP against real production data in #6848, but not independently re-verified live in this follow-up — happy to spin up a fresh dev session and confirm the watched-workspace count actually drops if useful before merge. Follow-up to #6848 / #6729. Claude-Session: https://claude.ai/code/session_01NmLFihnhebmL9bbbojGYCR
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
…ained connection Found by a review of #6856's cleanup-effect ordering, but pre-existing — affects fs:watch too, and predates both #6848 and #6856. getEventBus(hostUrl) unconditionally (re)creates a ConnectionState if none exists in the shared `connections` map. unwatchGit/unwatchFs/unwatchFsFile never called maybeCleanupConnection, unlike on()'s and retain()'s cleanups — so a caller that only ever intends to *release* interest (never establish it) could, if the connection's last retainer/listener already tore it down moments earlier, silently mint a brand-new WebSocket connection just to send an unwatch command, then leave it dangling forever: nothing else would ever call maybeCleanupConnection for it again. This is a real, reachable ordering hazard in a multi-effect component like DashboardSidebarWorkspaceStatusProvider: React runs effect cleanups in declaration order (not reversed), so a "listener registration" effect's cleanup releasing a connection's last retain/listener, followed by a separate "release git-watch interest" effect's cleanup (or body, on a mid-session workspace removal) calling unwatchGit for the same host, hits this exactly. Fix: unwatchGit/unwatchFs/unwatchFsFile now call maybeCleanupConnection after releasing their own interest, mirroring on()'s and retain()'s cleanups. Verified with a real WS server (no mocks): before the fix, a release-only call after the connection's teardown opens a second real upgrade that never closes; after the fix, maybeCleanupConnection fires synchronously and fast enough that the stray connection never even dials out — confirmed via a standalone diagnostic script before writing the regression test, so the test's "no upgrade" assertion isn't guesswork. Claude-Session: https://claude.ai/code/session_01NmLFihnhebmL9bbbojGYCR
…tch gating Two smaller findings from reviewing #6856: - computedGitWatchTargets re-derived "is this workspace worth a live git watch" via an ad hoc `status !== "idle"` check, duplicating the file's own canonical predicate (getHighestPriorityStatus(...) !== null, already used two lines below for the UI's own attention indicator). Reusing it means a future PaneStatus change can't silently drift the two decisions apart — and it drops an unnecessary array-spread allocation in the same motion (getHighestPriorityStatus takes the Map's values() iterator directly). - gitWatchTargets's fingerprint was computed over an array in `targets`' order, even though the actual watched set is order-independent (Effect B's diff is keyed by workspaceId) — a pure reorder of `workspaces` with no membership change produced a different fingerprint and triggered a needless diffing pass. Now sorted by workspaceId before returning. Claude-Session: https://claude.ai/code/session_01NmLFihnhebmL9bbbojGYCR
Contributor
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
AviPeltz
added a commit
that referenced
this pull request
Aug 25, 2026
…ained connection Found by a review of #6856's cleanup-effect ordering, but pre-existing — affects fs:watch too, and predates both #6848 and #6856. getEventBus(hostUrl) unconditionally (re)creates a ConnectionState if none exists in the shared `connections` map. unwatchGit/unwatchFs/unwatchFsFile never called maybeCleanupConnection, unlike on()'s and retain()'s cleanups — so a caller that only ever intends to *release* interest (never establish it) could, if the connection's last retainer/listener already tore it down moments earlier, silently mint a brand-new WebSocket connection just to send an unwatch command, then leave it dangling forever: nothing else would ever call maybeCleanupConnection for it again. This is a real, reachable ordering hazard in a multi-effect component like DashboardSidebarWorkspaceStatusProvider: React runs effect cleanups in declaration order (not reversed), so a "listener registration" effect's cleanup releasing a connection's last retain/listener, followed by a separate "release git-watch interest" effect's cleanup (or body, on a mid-session workspace removal) calling unwatchGit for the same host, hits this exactly. Fix: unwatchGit/unwatchFs/unwatchFsFile now call maybeCleanupConnection after releasing their own interest, mirroring on()'s and retain()'s cleanups. Verified with a real WS server (no mocks): before the fix, a release-only call after the connection's teardown opens a second real upgrade that never closes; after the fix, maybeCleanupConnection fires synchronously and fast enough that the stray connection never even dials out — confirmed via a standalone diagnostic script before writing the regression test, so the test's "no upgrade" assertion isn't guesswork. Claude-Session: https://claude.ai/code/session_01NmLFihnhebmL9bbbojGYCR
…r-activity-based-git-watch
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.
Links
main— this only makes sense once the lazyGitWatcherregistration it builds on has landed)Summary
DashboardSidebarWorkspaceStatusProvidernow only holds livegit:watchinterest for the active workspace or ones with a currently running/blocked/attention-needing agent (any non-idlePaneStatus), instead of every sidebar row.Why / Context
#6848 made
GitWatcherrefcounted and lazy, but live-tested against real production data (90 non-archived workspaces) showed the fix's practical benefit was capped: merely opening the dashboard caused 79 of those 90 to become watched anyway, because this provider calledwatchGitfor every row to keep diff-stat caches warm. For a user who keeps the dashboard open — the common case, and the scenario in the original terminal-input-lag report — that's most of the population #6848 was trying to stop watching, reintroduced through a different subscriber. This PR is the flagged follow-up that actually unlocks the benefit for that case.How It Works
useDiffStats' normal query fetch already handles. Non-active rows don't render a diff count today anyway (only the active row does, per the existingentriesmemo), so losing liveness for an idle row costs a first-open loading flash, not a wrong number ever shown on screen.bindingRowsByIndex+deriveTerminalAgentStatus— the same data driving the status dot), so no new plumbing was needed.agent:lifecycle/terminal:lifecycle/git:changed), unchanged, keyed on the full target list — this still covers every row, since it's what detects a row transitioning into activity in the first place. Cheap to tear down and redo every render.watchGit/unwatchGit, new, keyed on a fingerprint-stabilized "currently worth watching" set — these calls carry real host-side cost (DB lookup, git subprocess, livefs.watchattach/teardown), so only they're gated.Manual QA Checklist
bun devboot on an already-loaded machine.Testing
bunx tsc --noEmit(desktop) — clean for this filebunx biome check— cleanKnown Limitations
Follow-ups
Summary by cubic
Gate the dashboard sidebar’s git-watch to only the active workspace and workspaces with non‑idle agents, instead of every sidebar row, to cut host load and stop terminal lag. Old behavior watched all rows; new behavior watches only active/in‑play rows and keeps full lifecycle listeners for all rows to detect transitions; idle, non‑active rows no longer live‑update diff counts and instead refetch on open (a brief first‑open loading is expected).
Bug Fixes
workspace-client:unwatchGit,unwatchFs, and file unwatch now callmaybeCleanupConnectionso a release‑only call can’t create and strand a new, unretained connection.Refactors
status !== "idle".workspaceIdto stabilize the fingerprint and avoid needless watch/unwatch churn on pure reorders.Written for commit a9edaf0. Summary will update on new commits.