Repository navigation
Move open diff baseline lookup off main thread - #6497
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesAsync agent-context diff viewer launch
Expensive synchronous agent-load review rule
Sequence Diagram(s)sequenceDiagram
participant MainActor as Main Actor
participant InFlightMap as openDiffViewerAgentContextTasks
participant DetachedTask as Detached Task
participant BaselineHelpers as Baseline Helpers
MainActor->>MainActor: compute taskKey = workspaceId:surfaceId:sessionId
MainActor->>InFlightMap: check if taskKey already in flight
alt Already in flight
MainActor->>MainActor: skip duplicate launch
else New launch
MainActor->>InFlightMap: record taskKey as in-flight
MainActor->>DetachedTask: spawn Task.detached
DetachedTask->>BaselineHelpers: latestAgentTurnDiffRepoRoot(storeURL, workspaceId, surfaceId, sessionId)
BaselineHelpers-->>DetachedTask: repoRoot or nil → useLastTurnSource
DetachedTask->>MainActor: openDiffViewerAgentContextShouldFocus(workspaceId, surfaceId, sessionId)
MainActor-->>DetachedTask: still focused?
DetachedTask->>MainActor: launchDiffViewerProcess(cwd, useLastTurnSource, sessionId, focus)
MainActor->>InFlightMap: clear taskKey
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 21 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (21 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![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
🤖 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 `@Sources/AppDelegate.swift`:
- Around line 6166-6170: The guard check in mainWindowContexts.values.contains
is using a cached snapshot from SharedLiveAgentIndex.shared.snapshot() which may
be stale, and it does not verify the window context is still the active one. If
the focus moved or the agent session was replaced while the baseline parse was
running, the old session could still be launched. Add an additional check to
verify the window context is currently active and obtain a fresh (or provably
fresh-enough) snapshot instead of relying on the cached value. If cache
freshness cannot be guaranteed, fall back to using directory diff rather than
the old session's cmux diff command.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 80e6212a-e492-4208-ae2e-7108ecc0eef0
📒 Files selected for processing (1)
Sources/AppDelegate.swift
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@Sources/OpenDiffViewerAgentBaselineLookup.swift`:
- Around line 5-30: The openDiffViewerAgentContextShouldFocus method uses a
tri-state Bool? return type with non-obvious semantics that need documentation
for maintainability. Add a documentation comment above the
openDiffViewerAgentContextShouldFocus function definition explaining the three
return cases: nil means no matching context exists, false means context exists
but focus has moved, and true means context is still focused. This will clarify
the intent at all call sites without requiring developers to reverse-engineer
the logic.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b3ab721a-5735-46ef-9681-fbedb7699a6a
📒 Files selected for processing (2)
Sources/AppDelegate.swiftSources/OpenDiffViewerAgentBaselineLookup.swift
| NSApp.isActive, | ||
| (context.window?.isKeyWindow == true || context.window?.isMainWindow == true) else { | ||
| return false | ||
| } |
There was a problem hiding this comment.
Focus check skips origin window
Medium Severity
In openDiffViewerAgentContextShouldFocus, when a window context matches the workspace and session but its windowId is not originWindowId, the loop returns false instead of continuing. If multiple MainWindowContext entries share the same TabManager, iteration order can pick a non-origin window first and mis-report focus before the origin window is checked.
Reviewed by Cursor Bugbot for commit e75a481. Configure here.
e75a481 to
87d5707
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 87d5707. Configure here.
| sessionId: request.sessionId, | ||
| originWindowId: request.originWindowId | ||
| ) else { | ||
| return |
There was a problem hiding this comment.
Stale async abort silent
Medium Severity
When the detached baseline task finishes, openDiffViewerAgentContextShouldFocus returning nil exits without launching the diff viewer or beeping. The shortcut path already returned true from openDiffViewerForFocusedWorkspace, so callers skip their failure beep and the user gets no feedback after a slow parse.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 87d5707. Configure here.
…essionId/focus (#6497), add AppDelegate launchDiffViewerProcess forwarder + agent-context task/pending dicts (round 78-fix)
This branch was successfully deployed
1 participant
Summary
Testing
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Changes the production keyboard/command-palette diff launch path and adds concurrent task state on AppDelegate; behavior is more async but reduces main-thread hang risk on large baseline files.
Overview
Open Diff Viewer no longer parses
agent-turn-diff-baselines.jsonsynchronously on the main actor when a focused agent session exists. The shortcut path now readsSharedLiveAgentIndex.sharedsnapshots on the main thread, then runs baseline/repo-root resolution in aTask.detachedtask before hopping back to launchcmux diff. Repeated shortcuts for the same workspace/surface/session are deduplicated, and completion re-checks focus so stale launches are skipped or opened without stealing focus. Non–agent-context opens stay on the existing synchronous working-directory fallback.Review automation is updated in parallel: CodeRabbit, Greptile, and
swift-expensive-sync-load.mdnow treat unbounded agent-history JSON/transcript/trajectory loads on interactive paths as failures and document the detached-parser pattern this PR implements.Reviewed by Cursor Bugbot for commit 87d5707. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit