fix: wait for PR status before inactivity auto-settle - #5187
Conversation
📝 WalkthroughWalkthroughThe change adds branch-aware change-request settlement states, propagates VCS loading and lookup failures, centralizes mobile lookups, and updates web and mobile thread settlement behavior. Tests cover failures, stale refreshes, branch changes, lookup windows, and unresolved states. ChangesChange-request settlement
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GitManager
participant VcsStatusBroadcaster
participant ClientVcsState
participant ThreadSettlement
GitManager->>VcsStatusBroadcaster: return PR status and lookup failure state
VcsStatusBroadcaster->>ClientVcsState: publish remoteLoaded status
ClientVcsState->>ThreadSettlement: resolve branch-aware change-request state
ThreadSettlement-->>ClientVcsState: return none, unknown, open, closed, or merged
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
github-actions
Bot
added
vouch:unvouched
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
9d94d18 to
0bf23d2
Compare
github-actions
Bot
added
size:XL
|
@coderabbitai review |
✅ Action performedReview finished.
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3c7a1ad. Configure here.
ApprovabilityVerdict: Needs human review This PR introduces significant runtime behavior changes to thread auto-settlement, keeping branch-backed threads active until their PR status is definitively resolved. It adds new state management across web, mobile, and server components, introduces a pooled PR lookup system, and modifies server-side VCS status caching. The cross-cutting scope and behavioral impact warrant human review. You can customize Macroscope's approvability policy. Learn more. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/client-runtime/src/state/vcs.test.ts (1)
116-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the omitted-option case to protect the conditional spread.
The test proves that
statusIdleTtlMs: 0reachesidleTTL. It does not prove that omitting the option keeps the previous default. The conditional spread inpackages/client-runtime/src/state/vcs.tsat Line 275 exists only for that case. A regression that passesidleTtlMs: undefinedunconditionally would not fail any test.Assert the default TTL for a call without options.
💚 Proposed additional assertion
const atoms = createVcsEnvironmentAtoms(runtime, { statusIdleTtlMs: 0 }); expect( atoms.status({ environmentId: TARGET.environmentId, input: { cwd: "/repo" }, }).idleTTL, ).toBe(0); + + const defaultAtoms = createVcsEnvironmentAtoms(runtime); + expect( + defaultAtoms.status({ + environmentId: TARGET.environmentId, + input: { cwd: "/repo" }, + }).idleTTL, + ).not.toBe(0); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/client-runtime/src/state/vcs.test.ts` around lines 116 - 128, Add a second assertion in the existing “allows clients to dispose VCS status streams immediately” test that calls createVcsEnvironmentAtoms without statusIdleTtlMs and verifies atoms.status(...).idleTTL retains the default TTL. Keep the current zero-value assertion unchanged.apps/web/src/components/SidebarV2.tsx (1)
426-426: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the
threadKeyparameter tostateKey.The value passed to
onChangeRequestStateis now the branch-aware state key fromthreadChangeRequestStateKey(line 557), not a scoped thread key. Line 451 in the same file bindsthreadKeytoscopedThreadKey(threadRef), so one name now carries two different key formats. A future caller could pass the line 451 value and silently break the partition lookup at line 1588. The mobile prop already usesstateKey(apps/mobile/src/features/threads/thread-list-v2-items.tsxline 318).♻️ Proposed rename
- onChangeRequestState: (threadKey: string, state: ChangeRequestSettlementState) => void; + onChangeRequestState: (stateKey: string, state: ChangeRequestSettlementState) => void;const handleChangeRequestState = useCallback( - (threadKey: string, state: ChangeRequestSettlementState) => { + (stateKey: string, state: ChangeRequestSettlementState) => { setChangeRequestStateByKey((current) => - updateChangeRequestSettlementState(current, threadKey, state), + updateChangeRequestSettlementState(current, stateKey, state), ); }, [], );Also applies to: 1364-1365
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/components/SidebarV2.tsx` at line 426, Rename the onChangeRequestState parameter from threadKey to stateKey throughout its declaration and all related call sites, including the handler around line 1364, so it consistently represents the branch-aware key from threadChangeRequestStateKey. Keep the separate threadKey binding derived from scopedThreadKey(threadRef) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/server/src/vcs/VcsStatusBroadcaster.ts`:
- Around line 505-509: Update the remote status refresh flow around
getCachedStatus, updateCachedRemoteStatus, and consecutiveFailuresRef so results
with prLookupFailed: true or loaded: false increment the consecutive failure
count instead of resetting it. Apply remoteRefreshFailureDelay using that count,
allowing unresolved lookups to back off toward
VCS_STATUS_REFRESH_FAILURE_MAX_DELAY while preserving recovery behavior when a
lookup succeeds.
---
Nitpick comments:
In `@apps/web/src/components/SidebarV2.tsx`:
- Line 426: Rename the onChangeRequestState parameter from threadKey to stateKey
throughout its declaration and all related call sites, including the handler
around line 1364, so it consistently represents the branch-aware key from
threadChangeRequestStateKey. Keep the separate threadKey binding derived from
scopedThreadKey(threadRef) unchanged.
In `@packages/client-runtime/src/state/vcs.test.ts`:
- Around line 116-128: Add a second assertion in the existing “allows clients to
dispose VCS status streams immediately” test that calls
createVcsEnvironmentAtoms without statusIdleTtlMs and verifies
atoms.status(...).idleTTL retains the default TTL. Keep the current zero-value
assertion unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a5129ee6-4901-45fa-8a2e-12c81b642bab
📒 Files selected for processing (21)
apps/mobile/src/features/home/HomeScreen.tsxapps/mobile/src/features/threads/ThreadNavigationSidebar.tsxapps/mobile/src/features/threads/thread-list-v2-items.tsxapps/mobile/src/features/threads/threadListV2.test.tsapps/mobile/src/features/threads/threadListV2.tsapps/mobile/src/state/use-thread-pr.tsapps/mobile/src/state/vcs.tsapps/server/src/git/GitManager.test.tsapps/server/src/git/GitManager.tsapps/server/src/vcs/VcsStatusBroadcaster.test.tsapps/server/src/vcs/VcsStatusBroadcaster.tsapps/web/src/components/ChatView.tsxapps/web/src/components/SidebarV2.tsxdocs/user/source-control.mdpackages/client-runtime/src/state/threadSettled.test.tspackages/client-runtime/src/state/threadSettled.tspackages/client-runtime/src/state/vcs.test.tspackages/client-runtime/src/state/vcs.tspackages/contracts/src/git.tspackages/shared/src/git.test.tspackages/shared/src/git.ts
| const cachedRemoteLoaded = (yield* getCachedStatus(cwd))?.remoteLoaded ?? false; | ||
| const needsInitialRefresh = (yield* Ref.get(needsInitialRefreshRef)) || !cachedRemoteLoaded; | ||
| if (needsInitialRefresh) { | ||
| yield* Ref.set(needsInitialRefreshRef, true); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Apply backoff when a lookup stays unresolved without failing.
A provider lookup can return successfully and still report prLookupFailed: true. updateCachedRemoteStatus then returns loaded: false, so Line 534 keeps needsInitialRefreshRef true and Line 535 resets consecutiveFailuresRef to 0. The failure backoff in remoteRefreshFailureDelay never applies to this path.
The retry cadence therefore stays at activeInterval indefinitely. For a repository with missing or revoked hosting-provider credentials, prLookupFailed is persistent, so the server polls the provider API at the base interval forever. Lines 505-509 also keep this retry active when automatic fetching is disabled, which widens the affected set to users who opted out of periodic fetching.
The existing test at Lines 787-827 covers recovery after one unresolved result. It does not cover a persistently unresolved lookup.
Count consecutive unresolved results the same way as failures, so the delay grows toward VCS_STATUS_REFRESH_FAILURE_MAX_DELAY.
♻️ Proposed backoff for unresolved results
if (Exit.isSuccess(exit)) {
- yield* Ref.set(needsInitialRefreshRef, !exit.value.loaded);
- yield* Ref.set(consecutiveFailuresRef, 0);
- return activeInterval;
+ yield* Ref.set(needsInitialRefreshRef, !exit.value.loaded);
+ if (exit.value.loaded) {
+ yield* Ref.set(consecutiveFailuresRef, 0);
+ return activeInterval;
+ }
+ const unresolvedAttempts = yield* Ref.updateAndGet(
+ consecutiveFailuresRef,
+ (count) => count + 1,
+ );
+ return remoteRefreshFailureDelay(unresolvedAttempts, activeInterval);
}Also applies to: 534-534
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/server/src/vcs/VcsStatusBroadcaster.ts` around lines 505 - 509, Update
the remote status refresh flow around getCachedStatus, updateCachedRemoteStatus,
and consecutiveFailuresRef so results with prLookupFailed: true or loaded: false
increment the consecutive failure count instead of resetting it. Apply
remoteRefreshFailureDelay using that count, allowing unresolved lookups to back
off toward VCS_STATUS_REFRESH_FAILURE_MAX_DELAY while preserving recovery
behavior when a lookup succeeds.
1 participant
What Changed
Why
#5151 correctly keeps threads with open PRs active, but the client previously flattened “remote PR lookup has not resolved” into
pr: null. During initial loading, after a stream error, or after a branch change, a stale thread could therefore pass the inactivity window before the client knew whether it had an open PR.This follow-up makes “unknown” an explicit conservative state. Confirmed no-PR branches still use normal inactivity settlement, open PRs remain active, and merged/closed PR behavior is unchanged.
This is independent of #5141: that PR controls whether confirmed merged/closed PRs settle, while this PR prevents unresolved status from being mistaken for a confirmed no-PR result.
Validation
pnpm exec vp test run apps/server/src/vcs/VcsStatusBroadcaster.test.ts apps/server/src/git/GitManager.test.ts packages/shared/src/git.test.ts packages/client-runtime/src/state/threadSettled.test.ts packages/client-runtime/src/state/vcs.test.ts apps/mobile/src/features/threads/threadListV2.test.ts --reporter=dot— 337 passedgit diff --checkpassed for all changed filesChecklist
Built with GPT-5.6-Sol via the Codex harness in T3 Code.
Note
Medium Risk
Touches thread settlement classification, VCS status caching/stream semantics, and background PR polling across server, web, and mobile; incorrect
remoteLoadedhandling could keep threads active too long or settle them early.Overview
Threads with a branch were sometimes auto-settled on inactivity while remote PR status was still loading or failed, because unresolved lookups were treated like no PR. This PR introduces an explicit
unknownchange-request state and blocks inactivity auto-settle until PR status is confirmed.Server & stream: VCS status streams now carry
remoteLoaded(andprLookupFailedon remote results).VcsStatusBroadcastertracks ref generation, marks remote as unloaded on branch changes, refresh failures, and provider lookup fallbacks, and ignores stale remote updates after a ref change.Clients: Shared
resolveChangeRequestSettlementStateand branch-keyed state (threadChangeRequestStateKey) feedeffectiveSettledon web (Sidebar v2, ChatView) and mobile thread list v2. Off-screen branches get bounded rotating lookup pools (16 targets / 30s) withthreadVcsEnvironment(statusIdleTtlMs: 0) so subscriptions do not pile up.Confirmed no-PR, open, merged, and closed behavior is unchanged once status is resolved.
Reviewed by Cursor Bugbot for commit 96dc224. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Keep threads with unresolved PR status active instead of auto-settling
remoteLoadedflag to VCS status stream events and cached state so consumers can distinguish "remote not yet fetched" from "no PR"; threads with a branch default tounknownuntil their PR state is confirmed.effectiveSettledto treatunknownchange request state as active (not settled), preventing premature auto-settlement while PR status loads.resolveChangeRequestSettlementStateto deriveopen|closed|merged|none|unknownby correlating thread branch, git status, andremoteLoaded.ThreadListV2ChangeRequestLookupPool/SidebarV2ChangeRequestStateReporter) that rotates through off-screen threads in bounded windows (limit 16, 30s interval) to refresh PR state independently of row virtualization.threadVcsEnvironmentwithstatusIdleTtlMs: 0so status subscriptions are disposed immediately when idle, avoiding lingering background polling.Macroscope summarized 96dc224.
Summary by CodeRabbit
Bug Fixes
Documentation