Fix browser focus after dismissing the command palette by click - #990
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR implements comprehensive command palette backdrop focus handling by adding responder discovery mechanisms, focus target derivation helpers across terminal and browser contexts, and focus restoration logic. A new DragGesture replaces the backdrop tap handling to route clicks through the new handler, with accessibility identifiers added for UI testing. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as Command Palette UI
participant Handler as Backdrop Click Handler
participant Responder as Responder Discovery
participant Focus as Focus Target Derivation
participant ContentView as ContentView
User->>UI: Click backdrop
UI->>Handler: DragGesture triggered
Handler->>Responder: underlyingResponder(atWindowPoint:)
Responder->>Responder: Disable container capture, hitTest on theme frame
Responder-->>Handler: NSResponder at click point
Handler->>Focus: commandPaletteBackdropFocusTarget(for:)
Focus->>Focus: Traverse responder chain & superviews
Focus->>Focus: Check terminal/browser/panel contexts
Focus-->>Handler: CommandPaletteRestoreFocusTarget
Handler->>ContentView: dismissCommandPalette(preferredFocusTarget:)
ContentView->>ContentView: restoreCommandPaletteFocus()
ContentView-->>User: Palette dismissed, focus restored
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmuxUITests/BrowserPaneNavigationKeybindUITests.swift">
<violation number="1" location="cmuxUITests/BrowserPaneNavigationKeybindUITests.swift:325">
P2: Avoid fixed `RunLoop` sleeps in this UI test; wait until the rename field actually disappears to reduce flakiness and unnecessary delay.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
Greptile SummaryThis PR fixes a focus regression where dismissing the command palette by clicking a browser or terminal pane behind it would restore focus to the stale pre-palette target instead of the pane the user actually clicked. The fix intercepts backdrop clicks via a Key changes:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Backdrop as CommandPalette Backdrop (DragGesture)
participant CV as ContentView
participant OC as WindowCommandPaletteOverlayController
participant BR as BrowserWindowPortalRegistry
participant TR as TerminalWindowPortalRegistry
User->>Backdrop: click (DragGesture.onEnded)
Backdrop->>CV: handleCommandPaletteBackdropClick(atContentPoint:)
CV->>CV: commandPaletteBackdropFocusTarget(atContentPoint:)
CV->>OC: underlyingResponder(atWindowPoint:)
OC->>OC: capturesMouseEvents = false
OC->>OC: themeFrame.hitTest(pointInTheme)
OC-->>CV: NSResponder?
alt Responder found
CV->>CV: commandPaletteBackdropFocusTarget(for responder)
CV->>CV: commandPaletteOwningWebView(for:) or cmuxOwningGhosttyView(for:)
CV-->>CV: CommandPaletteRestoreFocusTarget?
else No responder hit
CV->>BR: webViewAtWindowPoint(_:in:)
BR-->>CV: WKWebView?
alt WebView found
CV->>CV: commandPaletteBrowserFocusTarget(for:)
else No WebView
CV->>TR: terminalViewAtWindowPoint(_:in:)
TR-->>CV: TerminalView?
end
end
CV->>CV: dismissCommandPalette(restoreFocus: true, preferredFocusTarget:)
CV->>CV: restoreCommandPaletteFocus(target:attemptsRemaining:)
CV-->>User: Focus restored to clicked pane
Last reviewed commit: 82d6086 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?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.
🧹 Nitpick comments (1)
cmuxUITests/BrowserPaneNavigationKeybindUITests.swift (1)
322-326: Consider making the click target more robust.The normalized offset
(dx: 0.82, dy: 0.78)assumes a specific window layout where the browser pane occupies that region. If the layout changes (e.g., different pane ratios, window sizes), this could click the wrong element or miss the browser entirely, causing test flakiness.Consider using an accessibility identifier on the browser pane content area and querying it directly, similar to how
BrowserOmnibarTextFieldandCommandPaletteRenameFieldare used elsewhere in this file.Also, the 0.2s hardcoded delay after clicking may be insufficient or excessive depending on system load. Consider replacing it with a
waitForDataMatchpredicate if the app emits a signal when focus changes, or at minimum document why this delay is necessary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxUITests/BrowserPaneNavigationKeybindUITests.swift` around lines 322 - 326, Replace the brittle coordinate click and fixed delay with a direct hit on the browser pane element by querying it via an accessibility identifier (e.g., add/use an identifier like "BrowserContentArea" and locate it via app.otherElements["BrowserContentArea"]) and call its click; then replace the RunLoop delay with an explicit wait that asserts the command palette rename field disappears (use a wait/predicate against renameField, e.g., XCTAssertFalse(renameField.waitForExistence(timeout: X)) or use NSPredicate-based expectation) so the test targets BrowserPane content reliably and waits deterministically for the focus change instead of relying on coordinates and a hardcoded 0.2s sleep.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmuxUITests/BrowserPaneNavigationKeybindUITests.swift`:
- Around line 322-326: Replace the brittle coordinate click and fixed delay with
a direct hit on the browser pane element by querying it via an accessibility
identifier (e.g., add/use an identifier like "BrowserContentArea" and locate it
via app.otherElements["BrowserContentArea"]) and call its click; then replace
the RunLoop delay with an explicit wait that asserts the command palette rename
field disappears (use a wait/predicate against renameField, e.g.,
XCTAssertFalse(renameField.waitForExistence(timeout: X)) or use
NSPredicate-based expectation) so the test targets BrowserPane content reliably
and waits deterministically for the focus change instead of relying on
coordinates and a hardcoded 0.2s sleep.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 55bf0a7a-a466-45b9-8ceb-5582e790fb3a
📒 Files selected for processing (2)
Sources/ContentView.swiftcmuxUITests/BrowserPaneNavigationKeybindUITests.swift
This was referenced Mar 21, 2026
bn-l
pushed a commit
to bn-l/cmux
that referenced
this pull request
Apr 3, 2026
…-unfocusable-after-cmdr Fix browser focus after dismissing the command palette by click
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
./scripts/reload.sh --tag issue-983-browser-unfocusable-after-cmdrSummary by cubic
Fixes focus after dismissing the command palette by clicking the backdrop. We hit-test the click, map it to the underlying browser or terminal, and restore focus to that exact pane (Linear 983).
Written for commit 44910d0. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements
Accessibility
Tests