Repository navigation
Fix up/down arrow keys in browser surface - #2780
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
To use Codex here, create a Codex account and connect to github. |
📝 WalkthroughWalkthroughAdded conditional Up/Down arrow forwarding to the browser first responder with IME/composition and re-entrancy guards; tightened omnibar shortcut focus detection; assigned a stable AppKit identifier to the omnibar text field; updated docs and bumped a vendor submodule pointer. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Window as NSWindow / AppDelegate
participant Browser as Browser First Responder
participant WebView as WebView
User->>Window: Press Up/Down arrow
Window->>Window: cmux_performKeyEquivalent(event)
Window->>Window: shouldDispatchBrowserArrowViaFirstResponderKeyDown?
alt Dispatch to browser (true) and depth == 0
Window->>Window: Increment cmuxBrowserArrowForwardingDepth
Window->>Browser: firstResponder?.keyDown(with: event)
Browser->>WebView: Deliver to web content / field-editor
WebView->>User: Handle arrow navigation/scroll
Window->>Window: Decrement cmuxBrowserArrowForwardingDepth
Window-->>User: Return handled (true)
else Skip forwarding (false or re-entry)
Window-->>User: Fall back to normal dispatch (false)
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 fixes a longstanding issue (#1146) where plain Up/Down arrow keys in browser content (e.g. Google Docs) were silently consumed by Confidence Score: 5/5Safe to merge — the arrow-forwarding change mirrors the well-tested Return/Enter pattern, and the omnibar tightening fixes a real bypass regression with no regressions. All identified findings are P2: one is an edge-case suppression-cleanup gap in multi-panel/multi-window stale state (the common same-panel path is correct), and one is a minor view-traversal direction note with no practical consequence given the unique identifier. No P0/P1 logic errors exist. Sources/AppDelegate.swift — specifically the new browserWebViewFirstResponderObserver handler's suppression cleanup path for cross-panel scenarios. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant NSWindow
participant AppDelegate
participant WebView as CmuxWebView
User->>NSWindow: Press Up/Down (plain)
NSWindow->>NSWindow: performKeyEquivalent(event)
NSWindow->>NSWindow: shouldDispatchBrowserArrowViaFirstResponderKeyDown?
Note over NSWindow: firstResponderIsBrowser=true, no marked text, no modifier flags
NSWindow->>NSWindow: cmuxBrowserArrowForwardingDepth += 1
NSWindow->>WebView: firstResponder.keyDown(with: event)
WebView-->>NSWindow: handled by web content
NSWindow->>NSWindow: cmuxBrowserArrowForwardingDepth -= 1
NSWindow-->>User: return true
Note over AppDelegate,WebView: Address bar focus state sync
User->>WebView: Click web content
WebView->>WebView: becomeFirstResponder()
WebView->>AppDelegate: post .browserDidBecomeFirstResponderWebView
AppDelegate->>AppDelegate: browserPanelOwning(webView)
AppDelegate->>WebView: panel.endSuppressWebViewFocusForAddressBar()
AppDelegate->>AppDelegate: browserAddressBarFocusedPanelId = nil
Note over AppDelegate: Omnibar shortcut tightening
User->>NSWindow: Cmd+N with address bar focused
NSWindow->>AppDelegate: focusedBrowserAddressBarPanelIdForShortcutEvent
AppDelegate->>AppDelegate: isBrowserOmnibarResponder(firstResponder)?
alt omnibar IS first responder
AppDelegate-->>NSWindow: panelId
else omnibar is NOT first responder
AppDelegate-->>NSWindow: nil
end
Reviews (1): Last reviewed commit: "Route browser up/down arrows through key..." | 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 12743-12762: The observer closure for
browserWebViewFirstResponderObserver only calls
endSuppressWebViewFocusForAddressBar() on the panel that became first responder
(panel B) but doesn't clear suppression on a previously tracked omnibar panel
(panel A) referenced by browserAddressBarFocusedPanelId; update the closure so
that if browserAddressBarFocusedPanelId is non-nil you locate the previously
tracked panel (using the existing browserPanelOwning/_lookup that can find a
panel by id) and call endSuppressWebViewFocusForAddressBar() on it before
setting browserAddressBarFocusedPanelId = nil and calling
stopBrowserOmnibarSelectionRepeat(), ensuring you still call
panel.endSuppressWebViewFocusForAddressBar() for the new panel and preserve the
current DEBUG dlog behavior.
🪄 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: 63a99854-f5e8-49e8-adea-edc9938f4ed1
📒 Files selected for processing (2)
Sources/AppDelegate.swiftSources/Panels/BrowserPanelView.swift
![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="Sources/AppDelegate.swift">
<violation number="1" location="Sources/AppDelegate.swift:12751">
P2: When focus jumps directly from panel A's address bar to panel B's web view, `endSuppressWebViewFocusForAddressBar()` is only called on panel B (the notification sender). The previously-tracked panel (referenced by `browserAddressBarFocusedPanelId`) never has its suppression ended before the ID is nilled out, leaving it stuck in a suppressed state. Look up and clear the tracked panel's suppression before clearing `browserAddressBarFocusedPanelId`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
Pushed . The review findings came from dirty submodule checkouts rather than missing fixes in the committed parent state:
This is the Ghostty helper CLI that accompanies the graphical Ghostty app. On macOS, the terminal can also be launched using We don't have proper help output yet, sorry! Please refer to the
This update refreshes to the actual Ghostty pin and records the APC handling patch. Build verified with ==> Ghostty build key: 3b684a0 App path: Tag cleanup status: CLI path: Build complete. Pass --launch to open the app, or cmd-click the path above.. |
|
Pushed 52c83fd. The review findings came from dirty submodule checkouts rather than missing fixes in the committed parent state:
This update refreshes docs/ghostty-fork.md to the actual Ghostty pin and records the TerminalStream APC handling patch. Build verified with ./scripts/reload.sh --tag review-comments. |
|
Pushed 224c01d for the latest browser focus-routing review. Changes:
Build verified with ./scripts/reload.sh --tag fix-browser-focus-routing. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/AppDelegate.swift (1)
12752-12781:⚠️ Potential issue | 🟠 MajorClear the previously tracked omnibar panel here too.
Line 12771 only clears the tracked address-bar state when the web view belongs to that same panel. If focus jumps from panel A’s omnibar straight into panel B’s web view, panel A stays suppressed,
browserAddressBarFocusedPanelIdstays stale, and the omnibar repeat state is never reset.💡 Suggested fix
) { [weak self] notification in guard let self else { return } guard let webView = notification.object as? CmuxWebView, let panel = self.browserPanelOwning(webView) else { return } - guard panel.pendingAddressBarFocusRequestId == nil || - self.browserAddressBarFocusedPanelId != panel.id else { + let trackedPanelId = self.browserAddressBarFocusedPanelId + if let trackedPanelId, + trackedPanelId != panel.id, + let trackedPanel = self.browserPanel(for: trackedPanelId) { + trackedPanel.endSuppressWebViewFocusForAddressBar() + } + guard panel.pendingAddressBarFocusRequestId == nil || + trackedPanelId != panel.id else { `#if` DEBUG dlog( "addressBar CLEAR panelId=\(panel.id.uuidString.prefix(8)) " + "reason=skip_pending_focus_handoff" ) `#endif` return } panel.endSuppressWebViewFocusForAddressBar() - if self.browserAddressBarFocusedPanelId == panel.id { + if trackedPanelId != nil { self.browserAddressBarFocusedPanelId = nil self.stopBrowserOmnibarSelectionRepeat() `#if` DEBUG dlog( "addressBar CLEAR panelId=\(panel.id.uuidString.prefix(8)) " +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 12752 - 12781, The closure handling .browserDidBecomeFirstResponderWebView must also clear any previously tracked omnibar panel when focus moves to a different panel's web view: if self.browserAddressBarFocusedPanelId != nil and != panel.id, locate the old panel (e.g. with a helper like browserPanel(withId:)) and call endSuppressWebViewFocusForAddressBar() on it (if found), then set browserAddressBarFocusedPanelId = nil and call stopBrowserOmnibarSelectionRepeat(); otherwise keep the existing logic for the current panel. This ensures stale focused-panel state is reset when focus jumps between panels.
🧹 Nitpick comments (1)
docs/ghostty-fork.md (1)
15-15: Optional wording cleanup for date punctuationConsider adding a comma after the year for readability: “March 30, 2026, rebase …”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/ghostty-fork.md` at line 15, Edit the sentence "Fork main has advanced beyond the March 30, 2026 rebase onto upstream `main`" to add the missing comma after the year so it reads "March 30, 2026, rebase onto upstream `main`" for correct date punctuation and improved readability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/AppDelegate.swift`:
- Around line 12752-12781: The closure handling
.browserDidBecomeFirstResponderWebView must also clear any previously tracked
omnibar panel when focus moves to a different panel's web view: if
self.browserAddressBarFocusedPanelId != nil and != panel.id, locate the old
panel (e.g. with a helper like browserPanel(withId:)) and call
endSuppressWebViewFocusForAddressBar() on it (if found), then set
browserAddressBarFocusedPanelId = nil and call
stopBrowserOmnibarSelectionRepeat(); otherwise keep the existing logic for the
current panel. This ensures stale focused-panel state is reset when focus jumps
between panels.
---
Nitpick comments:
In `@docs/ghostty-fork.md`:
- Line 15: Edit the sentence "Fork main has advanced beyond the March 30, 2026
rebase onto upstream `main`" to add the missing comma after the year so it reads
"March 30, 2026, rebase onto upstream `main`" for correct date punctuation and
improved readability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0c4e48c9-0d81-4a94-be64-15293e2a464c
📒 Files selected for processing (3)
Sources/AppDelegate.swiftdocs/ghostty-fork.mdvendor/bonsplit
✅ Files skipped from review due to trivial changes (1)
- vendor/bonsplit
|
Pushed Changes in this update:
Verified with |
|
Pushed This update does two things:
Verified with |
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 `@Sources/AppDelegate.swift`:
- Around line 11729-11740: The shortcut gate currently only checks
isBrowserOmnibarResponder(shortcutResponder) and rejects when the omnibar
handoff is in-flight; update the guard logic in the AppDelegate shortcut
handling so that it treats a panel with an active
pendingAddressBarFocusRequestId as an allowed omnibar context (i.e., if
shortcutResponder is the omnibar OR panel.pendingAddressBarFocusRequestId !=
nil, allow the shortcut to be handled by the browser omnibar path); adjust the
condition around isBrowserOmnibarResponder(shortcutResponder) and the early
return so that pendingAddressBarFocusRequestId prevents leaking Cmd/Ctrl+N/P
into app-level shortcuts and keep the existing debug dlog branch unchanged.
- Around line 11755-11759: The current omnibar owner resolution reads
textView.delegate directly (unsafe-unretained) which can crash; replace that
dereference by calling cmuxFieldEditorOwnerView(textView) and check its
identifier against browserOmnibarTextFieldIdentifier instead. Locate the block
that checks "if let textView = responder as? NSTextView, textView.isFieldEditor,
let delegateView = textView.delegate as? NSView, delegateView.identifier ==
browserOmnibarTextFieldIdentifier" and change it to use
cmuxFieldEditorOwnerView(_:) for the owner lookup (same pattern used by
keyRoutingOwnerView) and compare the returned view’s identifier to
browserOmnibarTextFieldIdentifier.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| guard isBrowserOmnibarResponder(shortcutResponder) else { | ||
| #if DEBUG | ||
| let focusedPanel = workspace.focusedPanelId.map { String($0.uuidString.prefix(5)) } ?? "nil" | ||
| dlog( | ||
| "browser.focus.addressBar.shortcutContext panel=\(panelId.uuidString.prefix(5)) " + | ||
| "accepted=0 reason=responder_not_omnibar responder=\(shortcutResponder.map { String(describing: type(of: $0)) } ?? "nil") " + | ||
| "pending=\(panel.pendingAddressBarFocusRequestId != nil ? 1 : 0) focusedPanel=\(focusedPanel) " + | ||
| "event=\(NSWindow.keyDescription(event))" | ||
| ) | ||
| #endif | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Pending address-bar handoff path is currently rejected for shortcut context.
This gate now requires isBrowserOmnibarResponder(...) only. During async omnibar handoff, first responder may still be web content while panel.pendingAddressBarFocusRequestId is active, so Cmd/Ctrl+N/P can leak into app-level shortcut handling.
💡 Suggested direction
+ let allowPendingAddressBarHandoff =
+ panel.pendingAddressBarFocusRequestId != nil &&
+ workspace.focusedPanelId == panel.id &&
+ panel.preferredFocusIntent == .addressBar
+
- guard isBrowserOmnibarResponder(shortcutResponder) else {
+ guard isBrowserOmnibarResponder(shortcutResponder) || allowPendingAddressBarHandoff else {
...
return nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/AppDelegate.swift` around lines 11729 - 11740, The shortcut gate
currently only checks isBrowserOmnibarResponder(shortcutResponder) and rejects
when the omnibar handoff is in-flight; update the guard logic in the AppDelegate
shortcut handling so that it treats a panel with an active
pendingAddressBarFocusRequestId as an allowed omnibar context (i.e., if
shortcutResponder is the omnibar OR panel.pendingAddressBarFocusRequestId !=
nil, allow the shortcut to be handled by the browser omnibar path); adjust the
condition around isBrowserOmnibarResponder(shortcutResponder) and the early
return so that pendingAddressBarFocusRequestId prevents leaking Cmd/Ctrl+N/P
into app-level shortcuts and keep the existing debug dlog branch unchanged.
| if let textView = responder as? NSTextView, | ||
| textView.isFieldEditor, | ||
| let delegateView = textView.delegate as? NSView, | ||
| delegateView.identifier == browserOmnibarTextFieldIdentifier { | ||
| return delegateView |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, examine the exact code at the specified lines
sed -n '11750,11765p' Sources/AppDelegate.swift | cat -nRepository: manaflow-ai/cmux
Length of output: 758
🏁 Script executed:
# Find the cmuxFieldEditorOwnerView function to verify it exists and understand its signature
rg -n 'func cmuxFieldEditorOwnerView' Sources/AppDelegate.swiftRepository: manaflow-ai/cmux
Length of output: 137
🏁 Script executed:
# Check where cmuxFieldEditorOwnerView is used in the codebase
rg -n 'cmuxFieldEditorOwnerView' Sources/AppDelegate.swift | head -20Repository: manaflow-ai/cmux
Length of output: 272
🏁 Script executed:
# Look at the broader context of the function containing lines 11755-11759
sed -n '11740,11780p' Sources/AppDelegate.swift | cat -nRepository: manaflow-ai/cmux
Length of output: 1893
🏁 Script executed:
# Check the implementation of cmuxFieldEditorOwnerView to understand what it does
sed -n '2058,2080p' Sources/AppDelegate.swift | cat -nRepository: manaflow-ai/cmux
Length of output: 889
🏁 Script executed:
# Also check the keyRoutingOwnerView function that is used as a fallback
rg -n 'func keyRoutingOwnerView' Sources/AppDelegate.swiftRepository: manaflow-ai/cmux
Length of output: 143
🏁 Script executed:
# Look at how cmuxFieldEditorOwnerView is used in other parts
sed -n '2025,2035p' Sources/AppDelegate.swift | cat -nRepository: manaflow-ai/cmux
Length of output: 488
🏁 Script executed:
# Verify the keyRoutingOwnerView fallback path to understand the complete flow
sed -n '5664,5685p' Sources/AppDelegate.swift | cat -nRepository: manaflow-ai/cmux
Length of output: 1024
Replace unsafe NSTextView.delegate dereference with cmuxFieldEditorOwnerView in omnibar owner resolution.
At line 11757, direct access to textView.delegate is unsafe-unretained and can crash during responder-chain teardown or reparenting. Use cmuxFieldEditorOwnerView(_:) instead, consistent with the pattern already established at line 5664 in keyRoutingOwnerView.
Suggested fix
- if let textView = responder as? NSTextView,
- textView.isFieldEditor,
- let delegateView = textView.delegate as? NSView,
- delegateView.identifier == browserOmnibarTextFieldIdentifier {
- return delegateView
+ if let textView = responder as? NSTextView,
+ textView.isFieldEditor,
+ let ownerView = cmuxFieldEditorOwnerView(textView),
+ ownerView.identifier == browserOmnibarTextFieldIdentifier {
+ return ownerView
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/AppDelegate.swift` around lines 11755 - 11759, The current omnibar
owner resolution reads textView.delegate directly (unsafe-unretained) which can
crash; replace that dereference by calling cmuxFieldEditorOwnerView(textView)
and check its identifier against browserOmnibarTextFieldIdentifier instead.
Locate the block that checks "if let textView = responder as? NSTextView,
textView.isFieldEditor, let delegateView = textView.delegate as? NSView,
delegateView.identifier == browserOmnibarTextFieldIdentifier" and change it to
use cmuxFieldEditorOwnerView(_:) for the owner lookup (same pattern used by
keyRoutingOwnerView) and compare the returned view’s identifier to
browserOmnibarTextFieldIdentifier.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
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="Sources/AppDelegate.swift">
<violation number="1" location="Sources/AppDelegate.swift:11757">
P1: Use `cmuxFieldEditorOwnerView` instead of directly accessing `textView.delegate` to resolve the omnibar owner view. `NSTextView.delegate` is unsafe-unretained in AppKit and can be a dangling pointer during responder-chain teardown, risking a crash. The codebase already has `cmuxFieldEditorOwnerView(_:)` for exactly this purpose (used in `keyRoutingOwnerView`).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| if let textView = responder as? NSTextView, | ||
| textView.isFieldEditor, | ||
| let delegateView = textView.delegate as? NSView, |
There was a problem hiding this comment.
P1: Use cmuxFieldEditorOwnerView instead of directly accessing textView.delegate to resolve the omnibar owner view. NSTextView.delegate is unsafe-unretained in AppKit and can be a dangling pointer during responder-chain teardown, risking a crash. The codebase already has cmuxFieldEditorOwnerView(_:) for exactly this purpose (used in keyRoutingOwnerView).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/AppDelegate.swift, line 11757:
<comment>Use `cmuxFieldEditorOwnerView` instead of directly accessing `textView.delegate` to resolve the omnibar owner view. `NSTextView.delegate` is unsafe-unretained in AppKit and can be a dangling pointer during responder-chain teardown, risking a crash. The codebase already has `cmuxFieldEditorOwnerView(_:)` for exactly this purpose (used in `keyRoutingOwnerView`).</comment>
<file context>
@@ -11749,8 +11749,38 @@ final class AppDelegate: NSObject, NSApplicationDelegate, UNUserNotificationCent
+
+ if let textView = responder as? NSTextView,
+ textView.isFieldEditor,
+ let delegateView = textView.delegate as? NSView,
+ delegateView.identifier == browserOmnibarTextFieldIdentifier {
+ return delegateView
</file context>
| let delegateView = textView.delegate as? NSView, | |
| let delegateView = cmuxFieldEditorOwnerView(textView), |
|
Stable desperately needs this fix. I just tried cmux (stable) for the first time and was annoyed I could not up/down in a github textarea ... Nightly has the fix and works better. Please release. |
PR #2780 added a browser-specific performKeyEquivalent bypass for plain Up/Down arrows but left Left/Right on the normal key-equivalent path, where AppKit can consume them before WebKit text inputs see keyDown. Extend the same narrow plain-arrow forwarding path to all four arrow key codes and keep modified-arrow shortcuts excluded. Constraint: Fix must remain symmetric with the existing browser Up/Down routing and avoid changing modified-arrow shortcut handling Rejected: Add a broader responder-chain rewrite | the regression is isolated to the plain-arrow allowlist Confidence: high Scope-risk: narrow Tested: Not run locally per task and repo policy Not-tested: CI and manual launched-app verification pending
PR #2780 added a browser-specific performKeyEquivalent bypass for plain Up/Down arrows but left Left/Right on the normal key-equivalent path, where AppKit can consume them before WebKit text inputs see keyDown. Extend the same narrow plain-arrow forwarding path to all four arrow key codes and keep modified-arrow shortcuts excluded. Constraint: Fix must remain symmetric with the existing browser Up/Down routing and avoid changing modified-arrow shortcut handling Rejected: Add a broader responder-chain rewrite | the regression is isolated to the plain-arrow allowlist Confidence: high Scope-risk: narrow Tested: Not run locally per task and repo policy Not-tested: CI and manual launched-app verification pending
PR #2780 added a browser-specific performKeyEquivalent bypass for plain Up/Down arrows but left Left/Right on the normal key-equivalent path, where AppKit can consume them before WebKit text inputs see keyDown. Extend the same narrow plain-arrow forwarding path to all four arrow key codes and keep modified-arrow shortcuts excluded. Constraint: Fix must remain symmetric with the existing browser Up/Down routing and avoid changing modified-arrow shortcut handling Rejected: Add a broader responder-chain rewrite | the regression is isolated to the plain-arrow allowlist Confidence: high Scope-risk: narrow Tested: Not run locally per task and repo policy Not-tested: CI and manual launched-app verification pending
PR #2780 added a browser-specific performKeyEquivalent bypass for plain Up/Down arrows but left Left/Right on the normal key-equivalent path, where AppKit can consume them before WebKit text inputs see keyDown. Extend the same narrow plain-arrow forwarding path to all four arrow key codes and keep modified-arrow shortcuts excluded. Constraint: Fix must remain symmetric with the existing browser Up/Down routing and avoid changing modified-arrow shortcut handling Rejected: Add a broader responder-chain rewrite | the regression is isolated to the plain-arrow allowlist Confidence: high Scope-risk: narrow Tested: Not run locally per task and repo policy Not-tested: CI and manual launched-app verification pending
* Prove browser horizontal arrows need keyDown forwarding The regression test adds browser key-routing helper coverage so Left, Right, Down, and Up are treated as one plain-arrow class when a browser responder is focused. On the pre-fix implementation, the Left and Right cases fail because the forwarding predicate only accepts Up and Down. Constraint: Tests must verify observable routing behavior rather than source text.\nRejected: Add an AppKit-only integration test first | the shared pure routing predicate is the smallest stable seam for the bug.\nConfidence: high\nScope-risk: narrow\nDirective: Keep horizontal and vertical browser arrows in one routing predicate.\nTested: Not run locally per task instruction and repo policy.\nNot-tested: CI execution pending. * Forward all plain browser arrows through keyDown Browser surfaces already bypass NSWindow.performKeyEquivalent for plain Up and Down after PR #2780. The same AppKit path can claim Left and Right before WebKit receives keyDown, so the browser responder now owns the whole plain-arrow key-code range while modified arrows and IME marked-text composition continue to stay out of the forced forwarding path. Constraint: Preserve browser shortcut handling for modified arrow chords and marked-text input.\nRejected: Add separate Left/Right branches in AppDelegate | the shared predicate should define the single browser-arrow class.\nConfidence: high\nScope-risk: narrow\nDirective: Do not split browser arrow forwarding by direction; all plain arrows must share this gate.\nTested: python3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv\nNot-tested: Local unit tests and app launch per task instruction; CI and final tagged reload pending. * Retry CI after CircleCI Zig download outage The previous CircleCI run failed in the shared Install zig setup step before cmux code compiled or tests executed. Rerun controls are unavailable to this account, so this empty commit retriggers CI without changing the reviewed patch. Constraint: CircleCI rerun API returned 403 and the UI reports missing write permission for reruns Rejected: Modify app code to retrigger CI | no code evidence supported a source change Confidence: high Scope-risk: narrow Tested: Inspected failed CircleCI step output for all macOS jobs Not-tested: Local tests/builds intentionally not run per task constraints * Make CircleCI Zig downloads tolerate transient resets CircleCI macOS jobs failed twice in the shared Install zig setup step with curl exit 56 while downloading the Zig tarball from ziglang.org. Retrying those downloads keeps the existing verified tarball path and minisign verification while avoiding source-free PR failures from transient network resets. Constraint: Do not run local xcodebuild or local tests; CI is the verification boundary for this PR Rejected: Keep pushing no-op retry commits | repeated failures showed the setup step itself needed retry behavior Confidence: medium Scope-risk: narrow Directive: Keep minisign verification in this install path; retrying the download must not bypass signature checks Tested: ruby YAML parse for .circleci/config.yml; git diff --check; Swift file length budget script Not-tested: CircleCI macOS rerun still pending after this commit * Bound CI Zig downloads against stalls CircleCI macOS jobs were spending more than 30 minutes in the Zig install step after transient network failures. The installer already retries hard curl failures; this also makes no-progress transfers time out so the retry policy can actually recover.\n\nConstraint: CI must pass before the required tagged app reload can run.\nRejected: Manual CircleCI rerun | CircleCI rejected rerun permissions for this app session.\nConfidence: medium\nScope-risk: narrow\nDirective: Keep CI download timeouts explicit so transient stalls do not block all macOS jobs.\nTested: ruby YAML parse for .circleci/config.yml; git diff --check; Swift file length budget script.\nNot-tested: CircleCI-only behavior pending remote run. * Remove PR-specific CircleCI edits The browser arrow-key fix should not carry CI workaround changes. Restore the CircleCI config to the current base branch version before merging main so the PR diff stays focused on the browser routing regression.\n\nConstraint: User requested removing YAML edits from this PR.\nRejected: Keep CI hardening in this branch | unrelated to issue #3622 and caused PR conflicts.\nConfidence: high\nScope-risk: narrow\nDirective: Keep CI infrastructure changes out of this browser-key routing PR.\nTested: Not run; config restored from origin/main.\nNot-tested: CI rerun pending after push.
…er-arrow-keys Fix up/down arrow keys in browser surface
This branch was successfully deployed
2 participants
Summary
firstResponder.keyDownso WebKit pages like Google Docs receive them instead of losing them inperformKeyEquivalent.Testing
Demo Video
For UI or behavior changes, include a short demo video (GitHub upload, Loom, or other direct link).
Review Trigger (Copy/Paste as PR comment)
Checklist
Note
Medium Risk
Touches macOS key-equivalent routing and focus/shortcut tracking, which can subtly affect global shortcuts and text input behavior across browser/terminal panes. Changes are scoped to browser-specific paths but should be validated across common shortcuts and IME scenarios.
Overview
Fixes browser panels losing plain Up/Down arrow key presses (e.g., in Google Docs) by forwarding unmodified up/down arrows from
NSWindow.performKeyEquivalenttofirstResponder.keyDown, with a re-entrancy guard similar to existing Return/Enter forwarding.Tightens omnibar shortcut behavior by only treating the address bar as focused when the actual omnibar text field is the current responder (via
browserOmnibarTextFieldIdentifier), and keepsbrowserAddressBarFocusedPanelIdin sync by clearing stale tracking when a web view becomes first responder (including cross-panel transitions).Updates
docs/ghostty-fork.mdto reflect the new pinned Ghostty fork head and document an added TerminalStream APC/kitty-graphics handling patch.Reviewed by Cursor Bugbot for commit 1ffe486. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes lost Up/Down arrow handling in the browser surface so WebKit pages (like Google Docs) receive plain arrows. Tightens omnibar shortcut focus detection with field‑editor awareness and keeps address‑bar focus state in sync. Fixes #1146.
Bug Fixes
firstResponder.keyDownwhen the browser web view is first responder, no marked text, and no modifiers; adds a re‑entrancy guard and preventsperformKeyEquivalentfrom swallowing them.browserOmnibarTextFieldIdentifier), or during a pending address‑bar handoff; uses a unified shortcut context for command and arrow navigation.Dependencies
vendor/bonsplit; refreshdocs/ghostty-fork.mdwith the new fork head and APC/kitty‑graphics stream handler notes.Written for commit 1ffe486. Summary will update on new commits.
Summary by CodeRabbit