Repository navigation
Add optional single-click focus for inactive panes - #1796
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughImplements an optional single-click pane focus when cmux is inactive: adds a persisted setting and localized strings, updates terminal/browser/markdown views to accept first-mouse when enabled, forwards markdown pointer events as needed, and adds tests and Xcode project entries. Changes
Sequence DiagramsequenceDiagram
actor User
participant App as cmux App
participant Settings as PaneFirstClickFocusSettings
participant View as Pane View (Terminal/Browser/Markdown)
participant System as AppKit Event System
User->>System: mouseDown on inactive window
System->>App: deliver event to window
System->>View: calls acceptsFirstMouse(for:)
View->>Settings: isEnabled()?
alt enabled
Settings-->>View: true
View-->>System: accepts first-mouse
System->>View: deliver mouseDown
View->>App: becomeFirstResponder / focus pane (1 click)
else disabled
Settings-->>View: false
View-->>System: reject first-mouse
System->>App: window becomes key (no pane focus)
User->>View: second click required
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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[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)
cmuxTests/InactivePaneFirstClickFocusTests.swift (1)
25-47: Consider adding negative case tests for completeness.The tests verify that
acceptsFirstMouse(for:)returnstruewhen the setting is enabled, which correctly validates runtime behavior. For more robust coverage, consider also testing that the views returnfalse(or the platform default) when the setting is disabled.💡 Example negative case test
func testTerminalViewRejectsFirstMouseWhenSettingDisabled() { UserDefaults.standard.set(false, forKey: settingsKey) let view = GhosttyNSView(frame: .zero) XCTAssertFalse(view.acceptsFirstMouse(for: nil)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/InactivePaneFirstClickFocusTests.swift` around lines 25 - 47, Add negative-case tests mirroring the existing positives: for each of testTerminalViewAcceptsFirstMouseWhenSettingEnabled, testBrowserViewAcceptsFirstMouseWhenSettingEnabled, and testMarkdownPointerObserverAcceptsFirstMouseWhenSettingEnabled set UserDefaults.standard.set(false, forKey: settingsKey) and assert the view's acceptsFirstMouse(for:) returns false (use GhosttyNSView, CmuxWebView with WKWebViewConfiguration(), and MarkdownPanelPointerObserverView respectively) so behavior is verified when the setting is disabled.Sources/Panels/MarkdownPanelView.swift (1)
382-390: TheisHiddentoggle during hit-test is a valid pattern but consider edge cases.Temporarily hiding
selfto perform hit-testing on the underlying content is a reasonable approach. ThedeferensuresisHiddenis restored even if the hit-test throws.One edge case: if the window's content view is
nil(line 384 guard), the function returnsniland no forwarding occurs. This is safe but could silently drop the mouse event. Consider whether a fallback or debug log would help diagnose issues.💡 Optional: Add debug logging for nil content view
private func forwardedTarget(for event: NSEvent) -> NSView? { - guard let window, - let contentView = window.contentView else { return nil } + guard let window, + let contentView = window.contentView else { +#if DEBUG + dlog("markdown.pointer.forwardedTarget window=\(window != nil ? 1 : 0) contentView=nil") +#endif + return nil + } isHidden = true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/MarkdownPanelView.swift` around lines 382 - 390, The guard in forwardedTarget(for:) silently returns nil when window.contentView is nil; add a debug-only log before returning to surface this edge case for diagnostics. Update the function forwardedTarget(for event: NSEvent) to detect the nil contentView case (the existing guard) and emit a concise debug message (e.g., NSLog or os_log inside a `#if` DEBUG block) indicating the window or event context, then return nil as before; leave the isHidden toggle and defer unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/cmuxApp.swift`:
- Around line 3821-3822: resetAllSettings() currently doesn't reset the new
AppStorage-backed property paneFirstClickFocusEnabled; update resetAllSettings()
to explicitly set paneFirstClickFocusEnabled =
PaneFirstClickFocusSettings.defaultEnabled so the persisted toggle is restored
to its default when users choose "Reset All Settings" (ensure you modify the
resetAllSettings function where other settings are restored).
---
Nitpick comments:
In `@cmuxTests/InactivePaneFirstClickFocusTests.swift`:
- Around line 25-47: Add negative-case tests mirroring the existing positives:
for each of testTerminalViewAcceptsFirstMouseWhenSettingEnabled,
testBrowserViewAcceptsFirstMouseWhenSettingEnabled, and
testMarkdownPointerObserverAcceptsFirstMouseWhenSettingEnabled set
UserDefaults.standard.set(false, forKey: settingsKey) and assert the view's
acceptsFirstMouse(for:) returns false (use GhosttyNSView, CmuxWebView with
WKWebViewConfiguration(), and MarkdownPanelPointerObserverView respectively) so
behavior is verified when the setting is disabled.
In `@Sources/Panels/MarkdownPanelView.swift`:
- Around line 382-390: The guard in forwardedTarget(for:) silently returns nil
when window.contentView is nil; add a debug-only log before returning to surface
this edge case for diagnostics. Update the function forwardedTarget(for event:
NSEvent) to detect the nil contentView case (the existing guard) and emit a
concise debug message (e.g., NSLog or os_log inside a `#if` DEBUG block)
indicating the window or event context, then return nil as before; leave the
isHidden toggle and defer unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 631d9b05-9140-4259-b290-af875123f895
📒 Files selected for processing (7)
GhosttyTabs.xcodeproj/project.pbxprojResources/Localizable.xcstringsSources/GhosttyTerminalView.swiftSources/Panels/CmuxWebView.swiftSources/Panels/MarkdownPanelView.swiftSources/cmuxApp.swiftcmuxTests/InactivePaneFirstClickFocusTests.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
bn-l
pushed a commit
to bn-l/cmux
that referenced
this pull request
Apr 3, 2026
* Add failing first-click pane focus tests * Enable optional first-click focus for inactive panes * Address PR review follow-ups --------- Co-authored-by: Lawrence Chen <lawrencecchen@users.noreply.github.com>
This was referenced May 11, 2026
This branch was successfully deployed
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.
Summary
Testing
Issues
Summary by cubic
Adds an optional one-click focus for inactive panes so a single click activates the window and focuses the clicked pane. Closes #1794.
New Features
Migration
Written for commit 673246e. Summary will update on new commits.
Summary by CodeRabbit
New Features
Localization
Tests