Harden runtime project switching#453
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
Disabled knowledge base sources:
📝 WalkthroughWalkthroughThis PR improves desktop application resilience during project operations. It tracks in-flight project bindings per window, enables pending roots to authorize runtime requests during switches, increases provisioning timeouts for cold projects, prevents inactive surfaces from rendering stale content, and refactors usage snapshot fetching to rely on cached freshness instead of forced polling. ChangesProject Switch Safety & Timeout Configuration
Usage Control Snapshot Caching
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review
Scope: 14 file(s), +244 / −68
Verdict: Looks good
This PR hardens same-window project switching by temporarily authorizing the target local runtime root on the main process, extending local-runtime IPC/setup timeouts, keeping Work mounted across inactive project tabs while skipping Lanes/other routes until a tab is active, and preventing stale usage cache reads from clobbering fresher onUpdate snapshots. The race and authorization paths are covered by targeted tests; I did not find blocking bugs in the diff.
Notes
- Pending-root authorization (
main.ts+runtimeBridge.ts) is scoped per window with refcounted cleanup infinally, and only paths enteringswitchProjectFromDialogare added—renderer IPC still must matchcollectAuthorizedLocalRuntimeRoots, so this closes the switch race without opening arbitrary cross-project access. - Preload eager binding on
switchToPathaligns rendererrootPathwith main pending auth; failure restores the previous binding. - Header usage correctly drops duplicate
refresh()on mount/focus/interval (the source of the stale-read race) while still subscribing toonUpdateand reading cache once viagetSnapshot(); background polling remains inusageTrackingService(2 min, immediatestart()poll). - App.tsx inactive-tab behavior is intentional: Work stays warm; Lanes and other routes no longer mount against the wrong runtime (see
App.workKeepAlive.test.tsx). - I did not re-run the PR’s full validation matrix in this automation environment; findings are from diff review and tracing control flow only.
Sent by Cursor Automation: BUGBOT in Versic
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function shouldApplyCachedSnapshot( | ||
| nextSnapshot: UsageSnapshot | null, | ||
| currentSnapshot: UsageSnapshot | null, | ||
| ): boolean { | ||
| if (!currentSnapshot) return true; | ||
| if (!nextSnapshot) return false; | ||
| const nextTimestamp = snapshotLastPolledMs(nextSnapshot); | ||
| const currentTimestamp = snapshotLastPolledMs(currentSnapshot); | ||
| return nextTimestamp == null || currentTimestamp == null || nextTimestamp >= currentTimestamp; | ||
| } |
There was a problem hiding this comment.
When nextTimestamp == null (i.e., the cached snapshot has a missing or unparseable lastPolledAt), the guard returns true and applies the cached snapshot even if currentSnapshot is a fresher, valid push update. A scenario where this bites: a push notification arrives with a valid timestamp, snapshotRef.current is set to that data, then readCachedSnapshot resolves with a stale or stripped snapshot that has no lastPolledAt — the guard would overwrite the fresher state with the stale one. Flipping that branch to return false (treat an undatable cached snapshot as unsafe to apply) would prevent stale overwrites while still letting valid older-timestamped snapshots be filtered by the >= comparison.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/components/usage/HeaderUsageControl.tsx
Line: 86-95
Comment:
**Null timestamp treated as "apply" rather than "skip"**
When `nextTimestamp == null` (i.e., the cached snapshot has a missing or unparseable `lastPolledAt`), the guard returns `true` and applies the cached snapshot even if `currentSnapshot` is a fresher, valid push update. A scenario where this bites: a push notification arrives with a valid timestamp, `snapshotRef.current` is set to that data, then `readCachedSnapshot` resolves with a stale or stripped snapshot that has no `lastPolledAt` — the guard would overwrite the fresher state with the stale one. Flipping that branch to `return false` (treat an undatable cached snapshot as unsafe to apply) would prevent stale overwrites while still letting valid older-timestamped snapshots be filtered by the `>=` comparison.
How can I resolve this? If you propose a fix, please make it concise.
1 participant
Summary
Validation
projects.addtimeout, warm switches were ~17-18ms, and ADE Work session state was preserved after switching away and back.git diff --checknpm --prefix apps/desktop run typechecknpm --prefix apps/desktop run lintnpm --prefix apps/desktop run buildnpm --prefix apps/ade-cli run testnpm --prefix apps/ade-cli run typechecknpm --prefix apps/ade-cli run buildnode scripts/validate-docs.mjsSummary by CodeRabbit
New Features
Bug Fixes
Performance
Greptile Summary
This PR hardens the desktop app's runtime project-switching flow by closing three distinct race windows: renderer calls that fire before the main-process binding is established, local runtime IPC calls that time out during large-project cold opens, and stale usage-cache reads that overwrite fresher pushed snapshots.
main.ts,runtimeBridge.ts): a reference-countedwindowPendingProjectRootsmap pre-authorizes both the user-selected path and the resolved git root for the duration ofopenProject, so renderer IPC and event-stream calls that race the binding step are not rejected. Cleanup is guaranteed in afinallyblock.ipcTimeouts.ts,localRuntimeConnectionPool.ts): theprojects.addsocket timeout grows from 3s → 120s and the renderer-side IPC deadline from 30s → 150s, keeping the outer deadline above the inner one so the socket call can always resolve before the IPC layer gives up.App.tsx): the Lanes surface and catch-allRoutesblock are now gated on theactiveflag, preventing background project tabs from mounting their Lanes component against the currently active runtime.HeaderUsageControl.tsx): removes the 120s polling interval and focus-triggered force-refresh in favour of a single cached read on mount plusonUpdatepush subscription, with a timestamp guard to prevent late-resolving stale reads from overwriting fresher data.Confidence Score: 4/5
Safe to merge; the core project-switching hardening is well-structured and test-covered. The usage control refactor has two edge cases worth a follow-up.
The pending-root authorization, timeout alignment, and inactive-tab isolation changes are all clearly scoped, have tests, and do not touch security-sensitive paths. The usage component change removes active polling in favour of push-only updates, and its staleness guard has a subtle gap where a cached snapshot with no lastPolledAt will overwrite a fresher pushed snapshot — neither issue blocks the project-switching fix but both are worth addressing before the usage refactor causes user-visible regression in long sessions.
apps/desktop/src/renderer/components/usage/HeaderUsageControl.tsx — the shouldApplyCachedSnapshot null-timestamp branch and the removal of all periodic refresh paths deserve a second look.
Important Files Changed
Sequence Diagram
sequenceDiagram participant R as Renderer participant P as Preload participant M as Main (openProject) participant B as runtimeBridge participant L as LocalRuntimePool R->>P: switchToPath("/new-repo") P->>P: rememberProjectBinding(nextBinding "/new-repo") P->>M: IPC projectSwitchToPath activate M M->>M: authorizePendingWindowProjectRoot(windowId, "/new-repo") M->>M: resolveRepoRoot → "/new-repo" M->>M: authorizePendingWindowProjectRoot(windowId, repoRoot) [if different] R->>B: "localRuntimeCallAction { rootPath: "/new-repo" }" B->>B: collectAuthorizedLocalRuntimeRoots → includes pendingLocalProjectRoots B->>L: callActionForRoot("/new-repo") [authorized via pending] L-->>B: result B-->>R: result M->>L: "projects.add { rootPath } [timeout: 120s]" L-->>M: ok M->>M: bindWindowToProject(windowId, repoRoot) deactivate M M->>M: finally: pendingRepoRootCleanup() + pendingSelectedRootCleanup() M-->>P: ProjectInfo P->>P: rememberProjectBinding(nextBinding or resolved)Comments Outside Diff (1)
apps/desktop/src/renderer/components/usage/HeaderUsageControl.tsx, line 152-183 (link)The periodic 120-second interval and focus-triggered refresh have been removed. The component now relies entirely on
onUpdatepush notifications for ongoing freshness. In a long-running session where the push mechanism stops delivering (backend process restarts, IPC hiccup, etc.) the usage meter will show stale data indefinitely — there is no automatic recovery path, only the manual refresh button inside the drawer that users must discover. IfonUpdateis guaranteed to be driven by a background polling loop that is independent of this component, the risk is low; but if the subscriber count reaching zero could pause that loop, the component is left dark. Consider keeping at least a coarse background refresh (e.g., every 10 minutes) as a fallback to bound the maximum staleness.Prompt To Fix With AI
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "Harden runtime project switching" | Re-trigger Greptile