Repository navigation
Fix Cloud focus convergence on middle-click - #16513
teamleaderleo wants to merge 4 commits into
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 1 files. (1 skipped: 1 … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely describes the primary change: fixing Cloud focus behavior for middle-click. Description check The description includes a complete summary, testing details, changelog entry, proof statement, and relevant checklist items. The Demo Video heading serves the Proof requirement, and omitted checklist… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Cmux Cloud Persistent Session And Early Input The reviewed diff adds only middle-click focus convergence and a regression test. The changed handler reuses existing focus activation, reparent-suppression clearing, explicit-input, and terminal mous… Cmux Swift Actor Isolation The production diff only adds existing focus calls inside GhosttyNSView.otherMouseDown(with:), an AppKit UI callback. It adds no model, protocol,Sendablereference type, logger, helper, or backgr…Cmux Swift Blocking Runtime The production diff adds no blocking or timing-based synchronization. Sources/GhosttyTerminalView.swiftonly calls focus activation and suppression-clear methods inotherMouseDown; the added lines…Cmux Browser Automation Off-Main PASS: The PR changes only Sources/GhosttyTerminalView.swiftandcmuxTests/SurfacePaneFactoryFocusTests.swift. The changed code handles middle-click focus activation and adds a regression test. It …Cmux Expensive Synchronous Load PASS. The only production change is in GhosttyNSView.otherMouseDown(with:): it callsactivateContainerFocusFromPointerDown()and clears reparent-focus suppression before handling the middle button…Cmux Cache Substitution Correctness PASS. The production diff only adds middle-click pane activation, reparent-focus suppression clearing, and explicit input handling in GhosttyNSView.otherMouseDown(with:). It does not replace an auth…Cmux No Hacky Sleeps PASS: The pull request changes only Swift production code and Swift tests. It adds no TypeScript, JavaScript, shell, or build/runtime-script sleep, timer, polling, delayed dispatch, or wall-clock wait… Cmux Algorithmic Complexity The production diff adds only constant-time control flow and existing method calls in GhosttyNSView.otherMouseDown. It adds no collection traversal, nested scan, repeated sort/filter, join, or batch…Cmux Swift Concurrency PASS. The PR adds synchronous focus calls in the AppKit otherMouseDown(with:)override and a test-only CGEvent/NSEvent middle-click helper. The authoritative diff adds no background Dispatch queue, …Cmux Swift @ConcurrentThe diff adds only synchronous focus handling in GhosttyNSView.otherMouseDownand a synchronous@MainActortest helper. It introduces nononisolated asyncwork, no@concurrentannotation, and …Cmux Swift Package Boundaries PASS: The production diff adds middle-click handling inside GhosttyNSView.otherMouseDown(with:). It usesNSEvent,window,terminalSurface, Ghostty mouse APIs, workspace focus activation, and h…Cmux Swiftpm Lockfiles PASS: The authoritative PR diff changes only Sources/GhosttyTerminalView.swiftandcmuxTests/SurfacePaneFactoryFocusTests.swift. It contains noPackage.swift,Package.resolved, `cmux.xcodeproj…Cmux Swift Logging PASS: The reviewed diff adds focus-handling code and a test helper only. It adds no print,debugPrint,dump,NSLog, file/stdout diagnostics, Logger declarations, or sensitive logging. The chan…Cmux User-Facing Error Privacy The pull request changes focus handling in Sources/GhosttyTerminalView.swiftand adds a regression test. It adds no user-facing errors, alerts, command output, API error bodies, or recovery copy. Th…Cmux Full Internationalization PASS: The diff changes pointer-focus behavior and adds a regression test. It adds no user-facing Swift text, app metadata, string-catalog entry, web UI text, API response, rendered markdown, or change… Cmux Swiftui State Layout PASS. The diff changes GhosttyNSView.otherMouseDown(with:), an AppKit bridge view, and adds AppKit regression-test logic. It introduces noObservableObject,@Published,@Observable, `GeometryR…Cmux Architecture Rethink PASS. The diff adds a small AppKit middle-click correctness path. It has no sleeps, delayed dispatch, polling, locks, observers, new mutable state, or side channels. `activateContainerFocusFromPointer… Cmux Swift Auxiliary Window Close Shortcuts The PR changes only middle-click handling in Sources/GhosttyTerminalView.swiftand a test fixture/helper incmuxTests/SurfacePaneFactoryFocusTests.swift. It adds or materially changes no standalon…Cmux Source Artifacts The PR changes only Sources/GhosttyTerminalView.swiftandcmuxTests/SurfacePaneFactoryFocusTests.swift. The diff adds hand-written Swift product logic and a focused regression test, including a sy…Cmux No Test Or Debug Seam In Production Source The only production change is in Sources/GhosttyTerminalView.swift. It adds normal middle-click focus behavior and calls existing production helpers. It does not add a#if DEBUGtest-only member, …
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 1 files. (1 skipped: 1 too large.)
✨ Finishing Touches 💡 1
-
📝 Generate docstrings 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Autopilot is currently an internal CodeRabbit preview.
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 @coderabbitai help to get the list of available commands.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Yours to fix: compile error in CI failed on
Failures:
Matched log linesNot re-run automatically: Written by |
Dogfood tours of
|
|
Fixed the CI compile error in 73ba38a: AppKit exposes |
|
Follow-up in 5ad497b: the synthetic middle-click now uses a CGEvent bridged to NSEvent and explicitly sets the button-number field to 2, so the test exercises GhosttyNSView.otherMouseDown instead of relying on the generic AppKit constructor. |
|
Updated the PR description to the repository template, including the concrete behavior, testing evidence, changelog, demo note, and checklist. |
95a3a8b to
f21b6e2
Compare
|
Rebased onto current main at f21b6e2; scoped diff check is clean. Parked while hosted CI reruns for exact-head attribution. |
This comment has been minimized.
This comment has been minimized.
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔇 Additional comments (3)
cmuxTests/SurfacePaneFactoryFocusTests.swift (2)
152-163: LGTM!
880-891: LGTM!Sources/GhosttyTerminalView.swift-9534-9541 (1)
9534-9541: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.Extract the shared pointer-down activation to one action path.
focusFromPointerDown()has the same two calls:activateContainerFocusFromPointerDown()andclearReparentFocusSuppressionForPointerFocus().otherMouseDownnow repeats them inline. The two paths can drift apart.otherMouseDowncould reuse the shared helper, butfocusFromPointerDown()also callsmakeFirstResponder. Extract only the activation part into one private method. Call that method from bothfocusFromPointerDown()andotherMouseDown.As per coding guidelines: "The same behavior wired separately through multiple surfaces instead of one shared action path."
Proposed refactor
+ private func activatePaneFocusFromPointerDown() { + guard let terminalSurface else { return } + activateContainerFocusFromPointerDown() + terminalSurface.hostedView.clearReparentFocusSuppressionForPointerFocus() + } + func focusFromPointerDown() { - if let terminalSurface { - activateContainerFocusFromPointerDown() - terminalSurface.hostedView.clearReparentFocusSuppressionForPointerFocus() - } + activatePaneFocusFromPointerDown()- if let terminalSurface { - activateContainerFocusFromPointerDown() - terminalSurface.hostedView.clearReparentFocusSuppressionForPointerFocus() - } + activatePaneFocusFromPointerDown()Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: manaflow-ai/cmux/.coderabbit.yaml
- Review profile: ASSERTIVE
- Plan: Advanced
- Run ID:
5cd97c62-e867-428c-b14e-5c80f4893da2
📒 Files selected for processing (2)
Sources/GhosttyTerminalView.swiftcmuxTests/SurfacePaneFactoryFocusTests.swift
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.
|
Closing in the 10-13 stale sweep: parked stale/conflicting work has no approved classic merge path. Reopen if still wanted. |
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
teamleaderleo
added
the
needs a call
This comment has been minimized.
This comment has been minimized.
|
Found 2 test failures on Blacksmith runners: Failures
|
![Fix with [code]smith](https://pr-comments-assets.blacksmith.sh/codesmith/fix-with-codesmith-light.svg)
This branch has not been deployed
2 participants
Summary
A Cloud manual-mirror pane beside a local pane could receive a middle-click while Bonsplit still reported the local pane as focused. The visible focus ring and keyboard ownership then stayed on the local pane. Middle-click now activates the containing workspace pane before the portal callback runs, clears stale reparent-focus suppression, and forwards the event to the clicked terminal. A mixed local and Cloud regression covers the focus handoff.
Testing
swiftc -parse Sources/GhosttyNSView+PointerFocusActivation.swift Sources/GhosttyTerminalView.swift cmuxTests/SurfacePaneFactoryFocusTests.swiftpython3 scripts/verify-local.py --affected mf/main(3/3)git diff --checkThe synthetic regression uses a CGEvent bridged to NSEvent with the middle-button field set, so it exercises
otherMouseDown. Native app compilation and live dogfood were not run on the MacBook Air.Changelog
Fixed: Middle-clicking a Cloud terminal beside a local pane moves the focus ring and keyboard ownership to the clicked pane.
Demo Video
Checklist
Summary by CodeRabbit