Repository navigation
Fix omnibar arrow key focus races - #4183
Merged
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:
📝 WalkthroughWalkthroughAdds an omnibar-native-field registry and focused-field APIs, introduces BrowserAddressBarTrackingContext and a preservation decision helper wired to pointer-initiated WebView focus, restores omnibar focus before forwarding arrow keys, refactors omnibar selection-repeat state, and adds tests for field resolution and arrow-key routing. ChangesBrowser omnibar arrow-key fix with resilient field registry
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 1 warning, 1 inconclusive)
✅ Passed checks (12 passed)
✨ 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 |
austinywang
force-pushed
the
issue-4141-omnibar-arrow-keys
branch
from
May 14, 2026 23:52
5071acc to
23af49a
Compare
Contributor
Greptile SummaryFixes omnibar arrow-key focus races by making the panel-ID resolution path in
Confidence Score: 5/5Safe to merge; the changes are well-scoped to omnibar arrow routing and focus-tracking, backed by new unit tests covering the stale-responder, transient-responder, and restore-before-dispatch scenarios. All modified paths are on MainActor, the repeat machinery now carries an explicit panelId rather than reading mutable shared state at tick time, and the new tracking-preservation logic is fully unit-tested as a pure function. The only nit is unnecessary WeakOmnibarNativeTextField allocation on every SwiftUI update in updateNSView. Sources/Panels/BrowserPanelView.swift — the updateNSView re-registration pattern; everything else is clean. Important Files Changed
Sequence DiagramsequenceDiagram
participant W as NSWindow.performKeyEquivalent
participant D as AppDelegate
participant R as BrowserOmnibarNativeFieldRegistry
participant FE as OmnibarFieldEditor (NSTextView)
W->>W: browserOmnibarPanelId(firstResponder)
alt firstResponder IS omnibar field editor
W->>W: shouldDispatchBrowserOmnibarArrowViaFirstResponderKeyDown → true
W->>FE: keyDown(arrowEvent)
else firstResponder is WebView / other
W->>W: shouldDispatchBrowserArrowViaFirstResponderKeyDown → true
W->>D: focusedBrowserOmnibarField(event, window)
D->>D: focusedBrowserAddressBarPanelIdForShortcutEvent(responder → tracked → intent)
D->>R: field(for: panelId, in: window)
R-->>D: OmnibarNativeTextField?
D-->>W: focusedOmnibarField?
alt focusedOmnibarField exists and not first responder
W->>W: makeFirstResponder(focusedOmnibarField)
W->>FE: keyDown(arrowEvent)
else no focused omnibar
W->>W: normal browser arrow forward
end
end
Reviews (11): Last reviewed commit: "fix: route omnibar arrows through active..." | 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}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/App/ShortcutRoutingSupport.swift`:
- Around line 95-108: Refactor the boolean-heavy function
shouldPreserveBrowserAddressBarTrackingDuringWebViewFocus by introducing a
single parameter struct (e.g., BrowserAddressBarTrackingContext) that contains
the six Bool properties, replace the function signature to accept that context,
update all call sites to construct and pass the struct, and add a concise doc
comment above the function that lists the decision steps
(trackedPanelMatchesWebView check, omnibarResponderActive shortcut,
preferredFocusIntentIsAddressBar and pointerInitiatedWebFocus gates, then
suppressesWebViewFocus || liveOmnibarFieldExists). Ensure property names in the
struct match the existing parameter names so the internal logic (guards and
final return) can be used without behavioral changes.
In `@Sources/AppDelegate.swift`:
- Around line 12379-12393: The helper shouldPreserveBrowserAddressBarTracking is
always passing trackedPanelMatchesWebView: true which allows a stale tracked
panel to be preserved when a different panel's web view actually took
first-responder; change the call site to compute trackedPanelMatchesWebView by
comparing the panel's actual webView against the window's current firstResponder
(or the webView instance passed into the browserDidBecomeFirstResponderWebView
observer) instead of hardcoding true, e.g. determine whether
resolvedWindow?.firstResponder (or the observer-supplied webView) is the same
webView instance as panel.webView and pass that boolean into
shouldPreserveBrowserAddressBarTrackingDuringWebViewFocus so stale
browserAddressBarFocusedPanelId values are not preserved for the wrong panel.
In `@Sources/Panels/BrowserPanelView.swift`:
- Around line 53-62: The selection logic in field(for:in:) should prefer a live
OmnibarNativeTextField that is attached to a window before falling back to
detached registry entries; keep calling pruneDeadEntries(for:) and the existing
behavior when a specific window is provided, but when window is nil change the
fallback to first where field.window != nil (attached) and only then return
liveFields.first (detached) so transient registered-but-not-yet-attached fields
don't get chosen; update the logic in field(for:in:) accordingly and keep
references to the fields dictionary, pruneDeadEntries(for:), and
OmnibarTextFieldRepresentable.makeNSView/updateNSView semantics in mind.
In `@Sources/Panels/CmuxWebView.swift`:
- Around line 430-434: Extract the literal "pointerInitiated" into a shared
constant and use it when posting and reading the notification: add a static
constant (e.g. BrowserFirstResponderKeys.pointerInitiated) alongside the
existing Notification.Name.browserDidBecomeFirstResponderWebView declaration,
replace the inline userInfo key in the NotificationCenter.post call with that
constant, and update any consumers to read userInfo[.pointerInitiatedConstant]
(casting to Bool) so producer and consumers share a single typed key.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c4b4032e-db50-4262-83f0-8dffdfd19c14
📒 Files selected for processing (6)
Sources/App/ShortcutRoutingSupport.swiftSources/AppDelegate.swiftSources/Panels/BrowserPanelView.swiftSources/Panels/CmuxWebView.swiftcmuxTests/BrowserConfigTests.swiftcmuxTests/OmnibarAndToolsTests.swift
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/AppDelegate.swift`:
- Around line 13741-13750: The preserve-check is using a hardcoded false for
trackedPanelMatchesWebView, ignoring the current webview-focus context; update
the call to
shouldPreserveBrowserAddressBarTracking(for:trackedPanelMatchesWebView:) to pass
the pointerInitiated boolean (the local pointerInitiated variable) instead of
false so the preservation logic receives the actual pointer-initiated
webview-focus context when evaluating browserAddressBarFocusedPanelId and
browserPanel(for:).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 25708bcc-6ed0-4169-9f5d-00a2e146aecd
📒 Files selected for processing (6)
Sources/App/ShortcutRoutingSupport.swiftSources/AppDelegate.swiftSources/Panels/BrowserPanelView.swiftSources/Panels/CmuxWebView.swiftSources/TabManager.swiftcmuxTests/BrowserConfigTests.swift
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/AppDelegate.swift (1)
12274-12320:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve the omnibar panel from the current responder before falling back to tracked state.
This still returns
nilwhen the omnibar is first responder butbrowserAddressBarFocusedPanelIdwas cleared, and it can also return the wrong panel when the tracked id lags behind the responder. The fast-path should derive the panel withbrowserOmnibarPanelId(for: shortcutResponder)before consulting tracked state.Suggested fix
func focusedBrowserAddressBarPanelIdForShortcutEvent(_ event: NSEvent) -> UUID? { - guard let panelId = browserAddressBarFocusedPanelId else { return nil } + let shortcutWindow = resolvedShortcutEventWindow(event) ?? NSApp.keyWindow ?? NSApp.mainWindow + let shortcutResponder = shortcutWindow?.firstResponder + + if let omnibarPanelId = browserOmnibarPanelId(for: shortcutResponder), + isBrowserOmnibarResponder(shortcutResponder) { + return omnibarPanelId + } + + guard let panelId = browserAddressBarFocusedPanelId else { return nil } @@ - let shortcutWindow = resolvedShortcutEventWindow(event) ?? NSApp.keyWindow ?? NSApp.mainWindow - let shortcutResponder = shortcutWindow?.firstResponder - - if isBrowserOmnibarResponder(shortcutResponder) { + if isBrowserOmnibarResponder(shortcutResponder) { `#if` DEBUG cmuxDebugLog( "browser.focus.addressBar.shortcutContext panel=\(panelId.uuidString.prefix(5)) " + "accepted=1 reason=omnibar_responder workspace=\(workspace.id.uuidString.prefix(5)) " + "event=\(NSWindow.keyDescription(event))" ) `#endif` - return panelId + return browserOmnibarPanelId(for: shortcutResponder) ?? panelId }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/AppDelegate.swift` around lines 12274 - 12320, The fast-path should resolve the omnibar's panel from the current responder before relying on tracked state: in focusedBrowserAddressBarPanelIdForShortcutEvent(_:) call browserOmnibarPanelId(for: shortcutResponder) (or equivalent) immediately after computing shortcutResponder and use that derived panelId if non-nil; only if that returns nil fall back to browserAddressBarFocusedPanelId. Update subsequent guard checks and DEBUG cmuxDebugLog messages to reference the derived panelId (or the fallback) so the accepted/rejected logging and workspace/panel lookup use the responder-derived panel id instead of stale tracked state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Sources/AppDelegate.swift`:
- Around line 12274-12320: The fast-path should resolve the omnibar's panel from
the current responder before relying on tracked state: in
focusedBrowserAddressBarPanelIdForShortcutEvent(_:) call
browserOmnibarPanelId(for: shortcutResponder) (or equivalent) immediately after
computing shortcutResponder and use that derived panelId if non-nil; only if
that returns nil fall back to browserAddressBarFocusedPanelId. Update subsequent
guard checks and DEBUG cmuxDebugLog messages to reference the derived panelId
(or the fallback) so the accepted/rejected logging and workspace/panel lookup
use the responder-derived panel id instead of stale tracked state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 30ffb542-0950-4dd9-89ab-ba34cc2fea86
📒 Files selected for processing (1)
Sources/AppDelegate.swift
lawrencecchen
dismissed stale reviews from coderabbitai[bot] and coderabbitai[bot]
May 15, 2026 01:14
Stale CodeRabbit changes-request review. All four actionable threads were fixed, replied to, and resolved; CodeRabbit subsequently passed on the updated head.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/AppDelegate.swift`:
- Around line 12352-12361: The code hard-codes trackedPanelMatchesWebView: true
when constructing BrowserAddressBarTrackingContext; instead compute it from the
live responder so it reflects whether the current first responder actually
belongs to this browser panel. Replace the literal true with a boolean derived
by checking the live omnibar/first-responder state (e.g., use the previously
computed liveOmnibarFieldExists combined with a check that the current first
responder’s browser panel ID equals panelId — determine that panel ID via the
same responder lookup you use elsewhere), so trackedPanelMatchesWebView
accurately reflects the live responder before calling
shouldPreserveBrowserAddressBarTrackingDuringWebViewFocus.
- Around line 14894-14908: The code forwards the arrow key even when
makeFirstResponder(focusedOmnibarField) fails, which can swallow the event;
update the logic around makeFirstResponder, currentEditor() and
self.firstResponder so you only call omnibarResponder.keyDown(with:) and return
true when focusedOmnibarField actually became first responder (i.e.,
makeFirstResponder returned true and currentEditorResponder equals
self.firstResponder or otherwise confirms focus); if the focus change was
rejected, do not call keyDown on focusedOmnibarField and return false so the
browser/webview retains the event. Ensure you reference
makeFirstResponder(focusedOmnibarField), focusedOmnibarField.currentEditor(),
self.firstResponder, browserResponderHasMarkedText(_:), and
omnibarResponder.keyDown(with:) in your conditional checks.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4c1ad567-0b38-4213-89ce-47840ae208d5
📒 Files selected for processing (2)
Sources/AppDelegate.swiftcmuxTests/AppDelegateShortcutRoutingTests.swift
Contributor
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/AppDelegate.swift`:
- Around line 12269-12272: The function focusedBrowserOmnibarField(for:in:)
currently falls back to browserAddressBarFocusedPanelId which allows stale
omnibar focus to be used; change it to only use the result of
focusedBrowserAddressBarPanelIdForShortcutEvent(_:) and return nil when that
call returns nil so shortcut-context resolution is respected (i.e., remove the
sticky fallback to browserAddressBarFocusedPanelId), leaving the rest of the
call to browserOmnibarField(panelId:in:) intact; this prevents
cmux_performKeyEquivalent's arrow-key restore from re-focusing a stale omnibar.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 31b143be-406f-4b4e-b024-7fe1d9f6a1dd
📒 Files selected for processing (1)
Sources/AppDelegate.swift
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 41df02b. Configure here.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
This was referenced May 16, 2026
Merged
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
2 participants
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
Fixes #4141
Testing
Note
Medium Risk
Touches AppKit first-responder tracking and keyboard event routing for the browser omnibar, which is easy to regress and can affect typing/navigation behavior across windows and panels.
Overview
Fixes browser omnibar arrow-key race conditions by resolving the focused panel from the current omnibar responder/intent (not just stale tracked state) and by making omnibar selection repeat state panel-specific.
Adds a
BrowserOmnibarNativeFieldRegistryandbrowserOmnibarField(panelId:in:)fallback lookup to reliably find the live omnibar field/field-editor even when AppKit leaves stale responder chains, and updates arrow-key forwarding to route plain arrows throughkeyDownfor omnibar responders (including restoring omnibar focus before forwarding).Refactors address-bar tracking preservation into
shouldPreserveBrowserAddressBarTrackingDuringWebViewFocuswith pointer-initiated WebView focus signals (propagated via.browserDidBecomeFirstResponderWebViewuserInfo), and adds regression tests covering stale field-editor ownership, transient first-responder states, and arrow routing.Reviewed by Cursor Bugbot for commit 0dd9e94. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Tests