Dim machines that drop instead of vanishing them, and cut idle background load - #947
Merged
Conversation
A machine used to leave the Work sidebar entirely on any connection blip. The reconnect grace window was 6s while a single connect candidate is allowed 10s and candidates are dialed in sequence, so every wifi hiccup and every sleep/wake yanked a machine's whole lane and chat group out of the list and animated it back a moment later. Users read that as "my machines disappear". Presence is now three verdicts instead of two, decided in one place: - LIVE: connected and still hosting this repository. - DIMMED: its lanes and chats stay on screen, collapsed and inert, with the offline form of the machine marker naming it and every card reading "<machine> is offline". A drop earns this only once a reconnect attempt has run to completion and failed - `connecting` observed while dropped, then a non-connected state - plus a 45s floor, with a 120s ceiling for a dial that never finishes and an immediate verdict for an `idle` target that will not redial at all. `lastAttemptedAt` cannot answer this on its own: a failed RPC over an established connection stamps it too, and that is the event most drops start with. - FORGOTTEN: the only case that deletes rows. A target gone from the connection snapshot, a connected machine that positively reports the repository missing (the #941 fix, preserved), or 24 hours unreachable. A machine we ARE connected to but cannot re-prove the repository on keeps its last verdict. Absence of proof is not proof of absence: a project list that has not caught up after a reconnect must not read as "the repo is gone". The foreign-lane context menu's machine-bound actions are now disabled from live store state rather than a flag captured at right-click time, which was a lie in the one case it looked like it covered. The same file also owned an undisclosed poller. Every ~5.4s, per connected foreign machine, it fired `lane.list` with `includeStatus` (a git status and a worktree probe per lane, plus a state-snapshot row written per lane, on the other machine) alongside `session.list` - and only the Work tab being selected gated it, not whether the window was visible at all. It now stops entirely while the window is hidden and refreshes once on the way back; chats are re-read every 10s and lanes on their own 30s cadence, with an immediate lane read when a chat names a lane that machine has never reported. For one connected foreign machine that is 22.2 to 7.7 calls/min visible, and to zero hidden. Also fixes a latent wedge found while testing: a refresh outlives its own runtime, and bookkeeping from a torn-down run left `refreshInFlight` set for whoever mounted next, which then scheduled nothing at all. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ecover Attention kept three background pollers running regardless of whether anything was on screen, and one RPC path could pin the UI on "syncing" for ten minutes. - `callAttention` inherited the runtime client's 10-minute default while every other sync-domain call uses 30s. It now passes the sync-domain budget, and `callSync` takes the timeout as an option so no other caller changes. - `refreshAttentionSnapshot` deduped on a module-level promise that a wedged call never settled, so every later refresh returned the same dead promise and the UI never left "syncing". A 45s renderer backstop - deliberately above the 30s main-process budget, so real host errors still win - now lands in the existing degraded/retry path and clears the dedupe. - The notch helper polled every 15s from the moment it spawned, including with the screen locked or asleep. It now reconciles its cadence: 15s only while it has an anchored surface and the screen is awake, 60s otherwise, driven by `powerMonitor` lock/suspend in main. - The presence POST ran at a fixed 30s. Hidden windows now report at 120s and send immediately on the way back; `blur` still reports the foreground change at once, so nothing is learned later than before. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Six real defects, four of them in the new code: - A remount re-brightened a machine that was already dimmed. Leaving Work and coming back tears the shared runtime down, taking its drop records with it while the store slice survives, so the next snapshot derived a fresh drop and held the machine live for another floor — group re-expanded, actions re-enabled. The verdict now survives: a dimmed machine is only ever re-brightened by becoming eligible again, and its retention deadline is re-anchored to its last successful read. - The catch-up lane read fired every tick instead of once. `session.list` does not filter on lane status while `lane.list` asks for `includeArchived: false`, so a chat on an archived lane is permanently unresolvable and demanded a fresh `includeStatus` read forever — more expensive than before the cadence existed. Lane ids a completed read did not explain are now remembered until the next one, and both read paths share the helper that owns the rule. - Removal on "the repository is gone" believed a folder-name mismatch. The scope's origin is re-resolved from the bound machine and can be transiently null, and `repoMatchFor` will say "missing" off a name alone — so a healthy machine's rows could be deleted while the bound machine blipped. Removal now requires an origin to prove it by. - A connected machine whose repository could not be re-proven stayed bright and was never read again: eligible for display, ineligible for refresh. It now dims on the same floor. It is still not removed — absence of proof is not proof of absence — but it stops claiming to be live. - The lane cadence could be stamped by a read that resolved after a scope change, suppressing the new scope's first lane read. - The Attention backstop was below the budget it was meant to clear: 15s relay request plus one 401 retry plus the 30s local fallback is 60s, so a 45s race could discard a slow-but-successful snapshot. Raised to 75s. Structural, from the same review: the hold/dim rule is one deadline instead of a double negative plus a matching ternary; `MachineConnectivity` carries the machine option rather than a nullable copy of one field, and the eligible list is derived from it instead of re-deriving; the machine marker moved out of the 2.3k-line session list; the offline-has-no-live-work rule is one predicate instead of two inverted copies. `resume` no longer claims the screen is awake while it is still locked, and a respawned notch helper no longer inherits the previous child's surface. The presence-cadence test now advances the clock instead of asserting that a timer was scheduled, so it would catch a chain that fires once and never re-arms. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… stand Commit 1's doc pass predated the review fixes, so it still said a connected machine that cannot re-prove the repository keeps its last verdict, and still claimed the cross-machine union never polls. Corrects both, and documents what had no coverage at all: the notch helper's surface- and screen-driven cadence, the presence POST's visible/hidden split, and the two bounds on an Attention snapshot read. Also adds a regression test for the one contract from the review round that had none: a catch-up lane read that resolves after a scope change must not stamp the cadence, or the new scope goes a full cadence with no lane list and therefore no rows at all. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Warning Review limit reached
Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (5)
📒 Files selected for processing (16)
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 |
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.
Two families of desktop defects: machines that appeared to vanish from the Work sidebar on any connection blip, and background pollers that ran whether or not anyone was looking.
1. Dim, don't vanish
#941 made an unreachable machine leave the Work sidebar entirely after a 6s grace window. A single connect candidate is allowed 10s and candidates are dialed in sequence, so that window was shorter than one dial: every wifi hiccup and every sleep/wake yanked a machine's whole lane and chat group out of the list and animated it back a moment later. Users read that as "my machines disappear".
Presence is now three verdicts, decided in one place (
applyReachability):idletarget that will not redial)lastAttemptedAtcannot decide this on its own: a failed RPC over an established connection stamps it too (markCallFailure), and that is the event most drops start with. So the state machine watches the lifecycle instead —connectingseen while dropped, then a non-connected state.A machine that is connected but whose repository cannot be re-proven dims on the same floor rather than staying bright: it is not being read, so claiming it is live is a lie. It is not removed, because absence of proof is not proof of absence — a project list that has not caught up after a reconnect must not read as "the repo is gone". #941's real fix (a connected machine that positively reports the repo missing) is preserved, now gated on having an origin to prove it by.
Behavior demo
Simulated by pushing connection snapshots through the real state machine (
crossMachineLanes.test.ts), which is exactly what the remote-runtime service publishes on a blip:connecting→ group stays live, no reflow. Previously: still live (6s window).errorat +3s → still live. Previously: removed at 6s.connectedat any point → instantly live again, and a second drop gets a full window, not the remainder of the first.2. Background load
The 5s foreign-machine poll was undisclosed — #917's commit message said "No polling." Every ~5.4s per connected foreign machine it fired
lane.list {includeStatus:true}(a git status and a worktree probe per lane, plus a state-snapshot row written per lane, on the other machine) alongsidesession.list, gated only on the Work tab being selected, not on whether the window was visible.Now: paused entirely while the window is hidden (one immediate refresh on the way back), 10s for chats, and the expensive lane read on its own 30s cadence — with an immediate lane read when a chat names a lane that machine has never reported, so nothing is invisible while it waits.
Attention:
callAttentioninherited a 10-minute timeout while every other sync-domain call uses 30s, andrefreshAttentionSnapshotdeduped on a promise a wedged call never settled — so the UI could sit on "syncing" indefinitely. Both fixed (30s main-process budget, 75s renderer backstop sized above 15s relay + one 401 retry + the 30s fallback). The notch helper polls 15s only while its surface is anchored and the screen is awake, 60s otherwise; presence POSTs at 30s visible / 120s hidden.Steady-state request math
One idle machine, Work tab open, one connected foreign machine. Counting only what this PR governs (the daemon's own 30s publish is unchanged and excluded):
Expensive
lane.list {includeStatus}calls specifically: 11.1/min → 1.9/min visible, 0 hidden.Targets were ≥2× visible and ≥4× hidden. The relay's 500k/day budget is not currently tripped — this is scaling headroom, not an incident fix.
Review round
/qualityfound six real defects, four in this PR's own new code; each is pinned by a test that fails without the fix:session.listdoes not filter on lane status whilelane.listexcludes archived lanes, so a chat on an archived lane was permanently unresolvable — more expensive than before the cadence existedAlso fixed a latent wedge found while testing: a refresh outlives its own runtime, and bookkeeping from a torn-down run left
refreshInFlightset for whoever mounted next, which then scheduled nothing at all.Tests
751 tests across renderer state, terminals, attention, and localRuntime; shard 2/8 re-run green (1128). 10 new tests covering the reachability state machine (drop → dimmed only after a failed attempt + floor; ceiling; idle; recovery; remount; 24h and registry removal), visibility gating, the lane cadence split, the archived-lane catch-up, and the Attention timeout + presence re-arm.
Parity
iOS,
adeCLI, and the TUI need no changes — none has a cross-machine roster, and each already retains-and-labels rather than hiding (iOS's Attention drawer and the TUI's Attention pane both ship the "offline, last known" form already). No shared type, IPC channel, or action domain changed. Not analytics-applicable: connection blips and poll cadence are indocs/logging.md's do-not-capture list.🤖 Generated with Claude Code