Fix command palette focus after terminal find#2089
Merged
lawrencecchen merged 10 commits intomainfrom Mar 26, 2026
Merged
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughRefactors command-palette focus detection into two module-level helpers that traverse responder/delegate and view/superview chains, expands view-level detection to include Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CommandPalette
participant TabManager
participant Tab
participant AppDelegate
participant Panel
participant View
User->>CommandPalette: open + create "New Workspace"
CommandPalette->>TabManager: request create & switch
TabManager->>Tab: focusSelectedTabPanel(previousTabId)
Tab->>TabManager: resolve panelId (lastFocusedPanelByTab or focusedPanelId)
TabManager->>Tab: tab.focusPanel(panelId)
Tab->>Panel: activate panel
Panel->>AppDelegate: ask current responder
AppDelegate->>AppDelegate: isCommandPaletteFocusStealingTerminalOrBrowserResponder(responder)
AppDelegate->>View: isCommandPaletteFocusStealingTerminalOrBrowserView(view) (may traverse delegate/superview)
alt focus stealer detected
Panel->>View: avoid stealing focus / restore previous field
else not a stealer
Panel->>View: allow focus to requested field
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
Greptile SummaryThis PR fixes a focus regression where the terminal find bar (
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[NSResponder arrives while\ncommand palette is visible] --> B{is GhosttyNSView\nor WKWebView?}
B -- Yes --> BLOCK[Return true: block focus steal]
B -- No --> C{is NSTextView\nand NOT field editor\nwith NSView delegate?}
C -- Yes --> D[Check delegate view hierarchy]
D --> E{delegate or ancestor\nis GhosttyNSView /\nGhosttySurfaceScrollView /\nWKWebView?}
E -- Yes --> BLOCK
E -- No --> ALLOW[Return false: allow focus]
C -- No --> F{is NSView?}
F -- Yes --> G[Check self + superview chain]
G --> H{self or ancestor\nis GhosttyNSView /\nGhosttySurfaceScrollView /\nWKWebView?}
H -- Yes --> BLOCK
H -- No --> ALLOW
F -- No --> ALLOW
style BLOCK fill:#f66,color:#fff
style ALLOW fill:#6a6,color:#fff
Reviews (1): Last reviewed commit: "fix: block terminal find from stealing p..." | Re-trigger Greptile |
![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.
Actionable comments posted: 1
🤖 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/AppDelegate.swift`:
- Around line 45-53: The current logic returns early for NSTextView responders
using textView.delegate as NSView which can skip classifying the NSTextView
itself; change the flow in the responder handling so you first attempt to
classify via the delegate path (call
isCommandPaletteFocusStealingTerminalOrBrowserView(delegateView) only if
delegateView exists) but do not return immediately—if that check fails, then
fall back to classifying the text view itself by calling
isCommandPaletteFocusStealingTerminalOrBrowserView(textView) (still skipping
field editors via textView.isFieldEditor). Ensure both paths are evaluated in
order for responder instances of NSTextView and preserve the existing NSView
branch for non-textView responders.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f3fabc99-76af-4084-a20e-ef74677ac87c
📒 Files selected for processing (2)
Sources/AppDelegate.swiftcmuxTests/ShortcutAndCommandPaletteTests.swift
Comment on lines
+45
to
+53
| if let textView = responder as? NSTextView, | ||
| !textView.isFieldEditor, | ||
| let delegateView = textView.delegate as? NSView { | ||
| return isCommandPaletteFocusStealingTerminalOrBrowserView(delegateView) | ||
| } | ||
|
|
||
| if let view = responder as? NSView { | ||
| return isCommandPaletteFocusStealingTerminalOrBrowserView(view) | ||
| } |
There was a problem hiding this comment.
Avoid delegate-only classification for NSTextView responders.
The early return on Line 48 can miss terminal/browser-hosted text views when delegate is an unrelated NSView. Check delegate path first, then fall back to classifying the text view itself.
Proposed fix
- if let textView = responder as? NSTextView,
- !textView.isFieldEditor,
- let delegateView = textView.delegate as? NSView {
- return isCommandPaletteFocusStealingTerminalOrBrowserView(delegateView)
- }
+ if let textView = responder as? NSTextView, !textView.isFieldEditor {
+ if let delegateView = textView.delegate as? NSView,
+ isCommandPaletteFocusStealingTerminalOrBrowserView(delegateView) {
+ return true
+ }
+ return isCommandPaletteFocusStealingTerminalOrBrowserView(textView)
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/AppDelegate.swift` around lines 45 - 53, The current logic returns
early for NSTextView responders using textView.delegate as NSView which can skip
classifying the NSTextView itself; change the flow in the responder handling so
you first attempt to classify via the delegate path (call
isCommandPaletteFocusStealingTerminalOrBrowserView(delegateView) only if
delegateView exists) but do not return immediately—if that check fails, then
fall back to classifying the text view itself by calling
isCommandPaletteFocusStealingTerminalOrBrowserView(textView) (still skipping
field editors via textView.isFieldEditor). Ensure both paths are evaluated in
order for responder instances of NSTextView and preserve the existing NSView
branch for non-textView responders.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxUITests/BrowserPaneNavigationKeybindUITests.swift`:
- Around line 920-923: The test assumes Cmd+1 returns to the original workspace;
instead either seed the placement explicitly for the UI test or navigate back to
the original workspace by identity: capture the original workspace identifier
before creating the new workspace, then restore focus by selecting that
workspace (rather than calling app.typeKey("1", modifierFlags: [.command]) ),
and assert the BrowserFindSearchTextField (restoredFindField) appears;
alternatively set WorkspacePlacementSettings.current() to a deterministic
placement at test setup so index-based switching is stable.
- Around line 909-913: The test is pressing Return before the command-palette
results have converged to the new "New Workspace" action; update the assertion
to explicitly wait for the result row that represents the "New Workspace" item
rather than just CommandPaletteResultRow.0 (which may be a seeded row). After
typing into paletteSearchField, wait for a result whose label/text contains "New
Workspace" (e.g., query the descendants for a cell/staticText with that string
or use a predicate on the element returned from CommandPaletteResultRow.0 to
check its label) and assert its existence with a timeout before sending the
Return key; keep using the shared
scheduleCommandPaletteResultsRefresh(forceSearchCorpusRefresh:) behavior but
ensure the test verifies the specific "New Workspace" result is present/selected
first.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f5abc0c8-89cb-4c69-90f0-f8e7192aaac5
📒 Files selected for processing (4)
Sources/Panels/BrowserPanel.swiftSources/TabManager.swiftcmuxTests/BrowserConfigTests.swiftcmuxUITests/BrowserPaneNavigationKeybindUITests.swift
Comment on lines
+909
to
+913
| paletteSearchField.typeText("New Workspace") | ||
|
|
||
| let firstResultRow = app.descendants(matching: .any).matching(identifier: "CommandPaletteResultRow.0").firstMatch | ||
| XCTAssertTrue(firstResultRow.waitForExistence(timeout: 5.0), "Expected command palette results for New Workspace") | ||
| app.typeKey(XCUIKeyboardKey.return.rawValue, modifierFlags: []) |
There was a problem hiding this comment.
Wait for the actual “New Workspace” result before submitting.
CommandPaletteResultRow.0 can already exist from the palette’s initial seed, so waitForExistence alone does not prove the query has converged to the new-workspace action. Pressing Return here can intermittently execute whatever stale row is still selected instead.
Based on learnings: scheduleCommandPaletteResultsRefresh(forceSearchCorpusRefresh:) is shared command‑palette infrastructure across all submenus; brief initial flashes are expected and should not be changed in a feature-scoped PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmuxUITests/BrowserPaneNavigationKeybindUITests.swift` around lines 909 -
913, The test is pressing Return before the command-palette results have
converged to the new "New Workspace" action; update the assertion to explicitly
wait for the result row that represents the "New Workspace" item rather than
just CommandPaletteResultRow.0 (which may be a seeded row). After typing into
paletteSearchField, wait for a result whose label/text contains "New Workspace"
(e.g., query the descendants for a cell/staticText with that string or use a
predicate on the element returned from CommandPaletteResultRow.0 to check its
label) and assert its existence with a timeout before sending the Return key;
keep using the shared
scheduleCommandPaletteResultsRefresh(forceSearchCorpusRefresh:) behavior but
ensure the test verifies the specific "New Workspace" result is present/selected
first.
Comment on lines
+920
to
+923
| app.typeKey("1", modifierFlags: [.command]) | ||
|
|
||
| let restoredFindField = app.textFields["BrowserFindSearchTextField"].firstMatch | ||
| XCTAssertTrue(restoredFindField.waitForExistence(timeout: 6.0), "Expected browser find field after returning to workspace 1") |
There was a problem hiding this comment.
Don’t assume the original workspace is always on Cmd+1.
This only returns to the original browser when new workspaces insert after the current one. WorkspacePlacementSettings.current() also allows .top, in which case Cmd+1 stays on the newly created workspace and this test fails even though focus restoration is correct. Please seed placement explicitly for the UI test or navigate back by workspace identity instead of a fixed index.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmuxUITests/BrowserPaneNavigationKeybindUITests.swift` around lines 920 -
923, The test assumes Cmd+1 returns to the original workspace; instead either
seed the placement explicitly for the UI test or navigate back to the original
workspace by identity: capture the original workspace identifier before creating
the new workspace, then restore focus by selecting that workspace (rather than
calling app.typeKey("1", modifierFlags: [.command]) ), and assert the
BrowserFindSearchTextField (restoredFindField) appears; alternatively set
WorkspacePlacementSettings.current() to a deterministic placement at test setup
so index-based switching is stable.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
cmuxUITests/BrowserPaneNavigationKeybindUITests.swift (2)
915-920:⚠️ Potential issue | 🟠 MajorDon’t use
Cmd+1as the round-trip back to the original workspace.These tests still depend on workspace insertion order. If new workspaces are placed at the top,
Cmd+1keeps the test on the newly created workspace, so the assertions fail for the wrong reason. Waiting only for the palette to dismiss also does not prove the midpoint switch completed. Please seed placement explicitly for this UI test, or navigate back by the original workspace’s identity after the new workspace becomes active.Also applies to: 1141-1143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxUITests/BrowserPaneNavigationKeybindUITests.swift` around lines 915 - 920, Don’t rely on app.typeKey("1", modifierFlags: [.command]) to return to the original workspace; instead seed workspace placement or navigate by workspace identity: capture the original workspace’s accessibility identifier before creating the new workspace, wait until the new workspace is active (e.g., assert the new workspace element is selected/exists), then programmatically activate the original workspace element (tap or call .click()) and assert it becomes active; replace the app.typeKey("1", modifierFlags: [.command]) call and the simple palette dismissal check (waitForNonExistence(paletteSearchField)) with these identity-based waits and activation steps so the test doesn’t depend on workspace insertion order.
904-913:⚠️ Potential issue | 🟡 MinorWait for the actual “New Workspace” result before pressing Return.
CommandPaletteResultRow.0can already exist from the palette’s seeded results, sowaitForExistencealone does not prove the query has converged. Return can still execute a stale selection here. Please makeopenCommandPaletteForNewWorkspace(_:)wait until row 0 actually resolves to “New Workspace”, and routetestBrowserFindFieldKeepsFocusAfterNewWorkspaceRoundTrip()through that helper so the fix lives in one place.🛠️ Suggested consolidation
- app.typeKey("p", modifierFlags: [.command, .shift]) - - let paletteSearchField = app.textFields["CommandPaletteSearchField"].firstMatch - XCTAssertTrue(paletteSearchField.waitForExistence(timeout: 5.0), "Expected command palette search field") - paletteSearchField.click() - paletteSearchField.typeText("New Workspace") - - let firstResultRow = app.descendants(matching: .any).matching(identifier: "CommandPaletteResultRow.0").firstMatch - XCTAssertTrue(firstResultRow.waitForExistence(timeout: 5.0), "Expected command palette results for New Workspace") - app.typeKey(XCUIKeyboardKey.return.rawValue, modifierFlags: []) + openCommandPaletteForNewWorkspace(app)- let firstResultRow = app.descendants(matching: .any).matching(identifier: "CommandPaletteResultRow.0").firstMatch - XCTAssertTrue(firstResultRow.waitForExistence(timeout: 5.0), "Expected command palette results for New Workspace") + XCTAssertTrue( + waitForCondition(timeout: 5.0) { + let firstResultRow = app.descendants(matching: .any) + .matching(identifier: "CommandPaletteResultRow.0") + .firstMatch + return firstResultRow.exists && + firstResultRow.label.localizedCaseInsensitiveContains("New Workspace") + }, + "Expected command palette to converge to the New Workspace result" + ) app.typeKey(XCUIKeyboardKey.return.rawValue, modifierFlags: [])Based on learnings:
scheduleCommandPaletteResultsRefresh(forceSearchCorpusRefresh:)is shared command-palette infrastructure across all submenus, so brief seeded flashes are expected here.Also applies to: 1177-1187
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxUITests/BrowserPaneNavigationKeybindUITests.swift` around lines 904 - 913, The test is pressing Return before the command-palette results have converged (seeded rows can make CommandPaletteResultRow.0 appear stale); update openCommandPaletteForNewWorkspace(_:) to wait until the first result row's label/text exactly equals "New Workspace" (polling the firstMatch's value/label or an accessibility attribute) before sending the Return key, and change testBrowserFindFieldKeepsFocusAfterNewWorkspaceRoundTrip() to call that helper so the wait logic is centralized; keep existing use of CommandPaletteResultRow.0 and the shared refresh behavior (scheduleCommandPaletteResultsRefresh(forceSearchCorpusRefresh:)) but gate the Return on the content match rather than mere existence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmuxUITests/BrowserPaneNavigationKeybindUITests.swift`:
- Around line 915-920: Don’t rely on app.typeKey("1", modifierFlags: [.command])
to return to the original workspace; instead seed workspace placement or
navigate by workspace identity: capture the original workspace’s accessibility
identifier before creating the new workspace, wait until the new workspace is
active (e.g., assert the new workspace element is selected/exists), then
programmatically activate the original workspace element (tap or call .click())
and assert it becomes active; replace the app.typeKey("1", modifierFlags:
[.command]) call and the simple palette dismissal check
(waitForNonExistence(paletteSearchField)) with these identity-based waits and
activation steps so the test doesn’t depend on workspace insertion order.
- Around line 904-913: The test is pressing Return before the command-palette
results have converged (seeded rows can make CommandPaletteResultRow.0 appear
stale); update openCommandPaletteForNewWorkspace(_:) to wait until the first
result row's label/text exactly equals "New Workspace" (polling the firstMatch's
value/label or an accessibility attribute) before sending the Return key, and
change testBrowserFindFieldKeepsFocusAfterNewWorkspaceRoundTrip() to call that
helper so the wait logic is centralized; keep existing use of
CommandPaletteResultRow.0 and the shared refresh behavior
(scheduleCommandPaletteResultsRefresh(forceSearchCorpusRefresh:)) but gate the
Return on the content match rather than mere existence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c74b8713-7bb3-4eea-97ec-d6bea96ebad0
📒 Files selected for processing (1)
cmuxUITests/BrowserPaneNavigationKeybindUITests.swift
3 tasks
Jesssullivan
added a commit
to Jesssullivan/cmux
that referenced
this pull request
Mar 26, 2026
Ingests all upstream fixes since 2026-03-22 including: - Fix Cmd+N crash: retain snapshot workspaces (manaflow-ai#2183, manaflow-ai#2181, manaflow-ai#2178, manaflow-ai#2173) - Fix browser pane restore after reopen (manaflow-ai#2141) - Fix Ghostty resize_split keybind (manaflow-ai#1899) - Reduce shell integration prompt latency (manaflow-ai#2109) - Fix command palette focus after terminal find (manaflow-ai#2089) - Add Codex CLI hooks (manaflow-ai#2103) - Add cmux.json custom commands (manaflow-ai#2011) - Fix window position restore on relaunch (manaflow-ai#2129) Conflict resolution: - BrowserPanel.swift: accepted upstream configureWebViewConfiguration() refactor (already includes our forMainFrameOnly:true CAPTCHA fix from PR manaflow-ai#1877) Fork-specific files preserved: - Sources/Panels/WebAuthn{Coordinator,BridgeJavaScript}.swift - Sources/FIDO2/module.modulemap - vendor/ctap2 submodule - cmux.entitlements (with camera/audio-input removed) - cmux.embedded.entitlements - .github/workflows/fork-{ci,release}.yml
3 tasks
This was referenced Mar 28, 2026
bn-l
pushed a commit
to bn-l/cmux
that referenced
this pull request
Apr 3, 2026
* test: cover command palette focus guard * fix: block terminal find from stealing palette focus * test: cover text view focus-stealer fallback * Add regression for hidden DevTools sync republish loop * Avoid redundant DevTools visibility publishes * test: cover browser find focus after workspace round-trip * fix: restore browser find focus after workspace round-trip * fix: keep browser find caret on workspace return * Add workspace round-trip split find regressions * Keep inactive find overlays from stealing focus --------- Co-authored-by: Lawrence Chen <lawrencecchen@users.noreply.github.com>
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
GhosttySurfaceScrollViewas blocked while the command palette is visibleCmd+F, thenCmd+PorCmd+Shift+PTesting
./scripts/reload.sh --tag task-find-command-palette-focuscmux-unitcoverage incmuxTests/ShortcutAndCommandPaletteTests.swiftxcodebuild -scheme cmux-unitruns in this environment hit an existing SwiftUI/AppKit window-layout crash before the old window-based harness can complete, so rely on PR CI for the new regressionTask
Summary by cubic
Prevents terminal and browser views from stealing focus when the command palette is open, and restores the browser find field (with caret) after a workspace round‑trip. Also avoids redundant DevTools visibility publishes when the inspector is already hidden.
GhosttySurfaceScrollView,GhosttyNSView,WKWebView, and terminal-owned text inputs as blocked while the palette is visible.isCommandPaletteFocusStealingTerminalOrBrowserResponderandisCommandPaletteFocusStealingTerminalOrBrowserView.TabManager.focusPaneland focusing the native field at the end of the query.cmux-unittests for focus classification (includingNSTextViewdelegate-not-a-view fallback) and the Cmd+F → Cmd+P regression.setPreferredDeveloperToolsVisible; add a regression test.Written for commit d6313ce. Summary will update on new commits.
Summary by CodeRabbit
Refactor
Stability
Tests