Keep cmux browser Find shortcuts authoritative - #2356
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe PR adds keyboard shortcut routing for browser Find commands (Cmd+F/E/G variants), implementing a preflight policy that routes these shortcuts to WebKit first before falling back to cmux menu handling. It includes ownership detection to determine browser find overlay visibility, routing suppression during overlay presence, and expanded unit and UI test coverage for this behavior. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant NSWindow as NSWindow
participant AppDelegate as AppDelegate
participant CmuxWebView as CmuxWebView
participant WebKit as WebKit
participant BrowserPanel as BrowserPanel
User->>NSWindow: Press Cmd+F
NSWindow->>AppDelegate: cmux_performKeyEquivalent()
AppDelegate->>AppDelegate: shouldRouteBrowserFindCommandEquivalentThroughWebContentFirst()?
alt Browser Find Overlay Visible
AppDelegate->>CmuxWebView: performKeyEquivalent(with:)
CmuxWebView->>WebKit: super.performKeyEquivalent()
WebKit-->>CmuxWebView: handled = true
CmuxWebView-->>AppDelegate: true (suppress menu)
else Browser Find Overlay Hidden
AppDelegate->>BrowserPanel: browserFindBarIsVisible()?
BrowserPanel-->>AppDelegate: false
AppDelegate->>AppDelegate: Use fallback routing (menu/cmux)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 |
Greptile SummaryThis PR teaches cmux to let web content see Find-family shortcuts (Cmd+F / Cmd+G / Cmd+Shift+G / Cmd+Shift+F / Cmd+E) before the cmux menu fallback, enabling native web-app shortcuts like VS Code's Cmd+F while ensuring the visible cmux browser find bar keeps ownership of navigation shortcuts. Key design decisions:
Findings:
Confidence Score: 5/5Safe to merge — routing logic is sound, double-dispatch is correctly prevented at both the window and web-view layers, and all remaining feedback is P2. No P0 or P1 issues found. The browser-find preflight logic correctly prevents WebKit from observing the same key equivalent twice (via the No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant AppKit as AppKit Dispatch
participant Win as CmuxWindow.performKeyEquivalent
participant Vis as BrowserFindVisibility Check
participant WV as CmuxWebView.performKeyEquivalent
participant WK as WebKit (super)
participant Menu as NSApp.mainMenu
AppKit->>Win: performKeyEquivalent(Cmd+F / Cmd+G etc.)
Win->>Vis: shouldRouteBrowserFindCommandEquivalentThroughWebContentFirst(event, webView)
alt Find bar IS visible (Cmd+F/G/Shift+G/Shift+F)
Vis-->>Win: false
Win->>Menu: menu.performKeyEquivalent (via cmux_performKeyEquivalent)
Menu-->>Win: true (cmux find bar handles it)
else Find bar NOT visible (or Cmd+E)
Vis-->>Win: true
Win->>WV: firstResponderWebView.performKeyEquivalent(event)
WV->>Vis: shouldRouteBrowserFindCommandEquivalentThroughWebContentFirst (re-check)
Vis-->>WV: true
WV->>WK: super.performKeyEquivalent(event) [first and only WebKit call]
alt Web page claims shortcut (e.g. VS Code Cmd+F)
WK-->>WV: true
WV-->>Win: true
else Web page does not claim shortcut
WK-->>WV: false
WV->>Menu: menu.performKeyEquivalent (cmux find bar opens)
Menu-->>WV: true
WV-->>Win: true
else Nothing claims shortcut (e.g. bare Cmd+E)
WK-->>WV: false
Menu-->>WV: false
Note over WV: replayedBrowserFindShortcutIntoWebContent=true → skip 2nd WebKit call
WV-->>Win: false
end
Note over Win: Always return true to suppress double WebKit replay
Win-->>AppKit: true
end
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Panels/CmuxWebView.swift (1)
251-298:⚠️ Potential issue | 🟠 MajorAdd state tracking to prevent Find shortcut re-delivery through keyDown.
The
replayedBrowserFindShortcutIntoWebContentguard only suppresses the trailingsuper.performKeyEquivalent(with:)at lines 297–298. If WebKit declines the preflighted Find shortcut and cmux also declines it,performKeyEquivalent(with:)still returnsfalse, so the same event flows tokeyDown(with:). There,super.keyDown(with:)re-exposes unclaimed Cmd+E/Find-family events to WebKit a second time.The
replayedBrowserFindShortcutIntoWebContentflag is local toperformKeyEquivalentand inaccessible inkeyDown. Pass this state via a property or event annotation sokeyDowncan suppress Find shortcuts that were already preflighted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/CmuxWebView.swift` around lines 251 - 298, The preflight flag replayedBrowserFindShortcutIntoWebContent is local to performKeyEquivalent so keyDown cannot tell the event was already offered to WebKit; add an instance property (e.g. var didPreflightBrowserFindShortcut: Bool) on the CmuxWebView class, set it true when you preflight in performKeyEquivalent (where replayedBrowserFindShortcutIntoWebContent is currently set), and then in keyDown(with:) check that property to suppress re-delivery to WebKit (and reset it to false once the event is consumed or discarded). Ensure the property is cleared after handling each event to avoid permanently blocking real Find shortcuts.
🧹 Nitpick comments (1)
cmuxUITests/MenuKeyEquivalentRoutingUITests.swift (1)
321-326: Prefer deterministic focus confirmation over fixed 150ms sleep after pane click.Line 325 uses a timing sleep, which can be flaky on slower CI runners. Waiting for a concrete state signal (for example, focused panel change) would make this helper more stable.
♻️ Suggested stabilization
private func clickBrowserPane(app: XCUIApplication, browserPanelId: String) { let browserPane = app.otherElements["BrowserPanelContent.\(browserPanelId)"].firstMatch XCTAssertTrue(browserPane.waitForExistence(timeout: 6.0), "Expected browser pane content for click target") browserPane.coordinate(withNormalizedOffset: CGVector(dx: 0.5, dy: 0.5)).click() - RunLoop.current.run(until: Date().addingTimeInterval(0.15)) + XCTAssertTrue( + waitForGotoSplitMatch(timeout: 2.0) { data in + data["focusedPanelId"] == browserPanelId + }, + "Expected browser pane to become focused after click" + ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxUITests/MenuKeyEquivalentRoutingUITests.swift` around lines 321 - 326, The helper clickBrowserPane currently uses a fixed RunLoop delay after clicking (RunLoop.current.run(until: Date().addingTimeInterval(0.15))) which is flaky; update clickBrowserPane to replace the hard sleep with a deterministic wait that polls for a concrete focus signal after the click (e.g., waitUntil browserPane reports focused/selected or an app-level “focused panel” element/attribute changes), using browserPane (from BrowserPanelContent.\(browserPanelId)) and a reasonable timeout; implement polling (short interval loop) that returns when browserPane.isFocused / isSelected / accessibilityValue indicates focus (or when the app’s focused-panel indicator updates) to stabilize tests on slow CI.
🤖 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/MenuKeyEquivalentRoutingUITests.swift`:
- Around line 132-137: The current test only samples once after
RunLoop.current.run(until: Date().addingTimeInterval(0.5)) which can miss a late
second WebKit replay; change the assertion to observe stability over a window by
polling loadGotoSplit() multiple times (e.g., every 0.05–0.1s for a total of
~0.5–1s) and fail if any sample's ["browserPageTitle"] deviates from "cmde-1".
Update the block around RunLoop.current.run and XCTAssertEqual to loop, call
loadGotoSplit() repeatedly, and assert stability across all samples (reference:
RunLoop.current.run, loadGotoSplit(), XCTAssertEqual).
In `@Sources/AppDelegate.swift`:
- Around line 1877-1884: The matches(_ chars:keyCode:) function currently falls
back to event.keyCode even when KeyboardLayout.normalizedCharacters(for:)
produced a non-matching ASCII character, causing physical-key shortcuts to
misfire on non‑QWERTY layouts; change the logic so you only use the
event.keyCode fallback when the normalizedCharacters result is unavailable/empty
(e.g. normalizedChars.isEmpty) rather than whenever normalizedChars !=
chars—keep the initial normalizedChars == chars fast path, and otherwise if
normalizedChars.isEmpty return event.keyCode == keyCode, else return false.
---
Outside diff comments:
In `@Sources/Panels/CmuxWebView.swift`:
- Around line 251-298: The preflight flag
replayedBrowserFindShortcutIntoWebContent is local to performKeyEquivalent so
keyDown cannot tell the event was already offered to WebKit; add an instance
property (e.g. var didPreflightBrowserFindShortcut: Bool) on the CmuxWebView
class, set it true when you preflight in performKeyEquivalent (where
replayedBrowserFindShortcutIntoWebContent is currently set), and then in
keyDown(with:) check that property to suppress re-delivery to WebKit (and reset
it to false once the event is consumed or discarded). Ensure the property is
cleared after handling each event to avoid permanently blocking real Find
shortcuts.
---
Nitpick comments:
In `@cmuxUITests/MenuKeyEquivalentRoutingUITests.swift`:
- Around line 321-326: The helper clickBrowserPane currently uses a fixed
RunLoop delay after clicking (RunLoop.current.run(until:
Date().addingTimeInterval(0.15))) which is flaky; update clickBrowserPane to
replace the hard sleep with a deterministic wait that polls for a concrete focus
signal after the click (e.g., waitUntil browserPane reports focused/selected or
an app-level “focused panel” element/attribute changes), using browserPane (from
BrowserPanelContent.\(browserPanelId)) and a reasonable timeout; implement
polling (short interval loop) that returns when browserPane.isFocused /
isSelected / accessibilityValue indicates focus (or when the app’s focused-panel
indicator updates) to stabilize tests on slow CI.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 56d632b2-9af7-47b7-9c04-5670c4c89dc7
📒 Files selected for processing (4)
Sources/AppDelegate.swiftSources/Panels/CmuxWebView.swiftcmuxTests/AppDelegateShortcutRoutingTests.swiftcmuxUITests/MenuKeyEquivalentRoutingUITests.swift
| RunLoop.current.run(until: Date().addingTimeInterval(0.5)) | ||
| XCTAssertEqual( | ||
| loadGotoSplit()?["browserPageTitle"], | ||
| "cmde-1", | ||
| "Expected Cmd+E to avoid a second WebKit replay. data=\(loadGotoSplit() ?? [:])" | ||
| ) |
There was a problem hiding this comment.
Single delayed snapshot can miss late double-replay of Cmd+E.
Line 132 and Line 133 only check one point-in-time after 0.5s. A delayed second replay can slip past this check and still pass. Prefer asserting stability across a full observation window.
🔧 Proposed reliability fix
- RunLoop.current.run(until: Date().addingTimeInterval(0.5))
- XCTAssertEqual(
- loadGotoSplit()?["browserPageTitle"],
- "cmde-1",
- "Expected Cmd+E to avoid a second WebKit replay. data=\(loadGotoSplit() ?? [:])"
- )
+ let replayDetected = waitForCondition(timeout: 1.5) {
+ (self.loadGotoSplit()?["browserPageTitle"] ?? "") != "cmde-1"
+ }
+ XCTAssertFalse(
+ replayDetected,
+ "Expected Cmd+E to avoid a second WebKit replay during the observation window. data=\(loadGotoSplit() ?? [:])"
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmuxUITests/MenuKeyEquivalentRoutingUITests.swift` around lines 132 - 137,
The current test only samples once after RunLoop.current.run(until:
Date().addingTimeInterval(0.5)) which can miss a late second WebKit replay;
change the assertion to observe stability over a window by polling
loadGotoSplit() multiple times (e.g., every 0.05–0.1s for a total of ~0.5–1s)
and fail if any sample's ["browserPageTitle"] deviates from "cmde-1". Update the
block around RunLoop.current.run and XCTAssertEqual to loop, call
loadGotoSplit() repeatedly, and assert stability across all samples (reference:
RunLoop.current.run, loadGotoSplit(), XCTAssertEqual).
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
* Route browser Find shortcuts through web content first * Keep cmux browser Find shortcuts authoritative * Add browser Find inspector regression test * Fix browser Find routing follow-ups
This branch was successfully deployed
1 participant
Summary
Verification
Notes
Summary by cubic
Routes the Find shortcut family to web content first so native web apps can handle them, while keeping the cmux browser find bar authoritative when visible and preventing double delivery; skips this preflight when Web Inspector is focused. Addresses Linear issue 2342.
New Features
CMUX_UI_TEST_GOTO_SPLIT_BROWSER_URL.Bug Fixes
Written for commit 2917844. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
New Features
Tests