Fix browser up/down arrows after omnibar focus#1784
Fix browser up/down arrows after omnibar focus#1784lawrencecchen wants to merge 48 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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 AppDelegate observer to clear stale address-bar focus when a WKWebView becomes first responder; updates observer setup; adds unit and UI tests verifying arrow/shortcut routing across omnibar↔page focus transitions; minor CI manifest write change. Changes
Sequence DiagramsequenceDiagram
participant Client as Keyboard/EventSource
participant WebView as WKWebView
participant NC as NotificationCenter
participant AppD as AppDelegate
participant AB as AddressBar
WebView->>NC: post browserDidBecomeFirstResponderWebView
NC->>AppD: notify browserWebViewFocusObserver
AppD->>NC: post browserDidBlurAddressBar
NC->>AB: browserDidBlurAddressBar (clear stale focus)
Note right of AppD: Debug log (focus cleared)
Client->>WebView: key event (e.g., Down Arrow)
WebView->>WebView: handle key normally
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
|
I ran into the same issue and did some debugging in #1423 — wanted to share what I found in case it's useful. When I traced the key events, I noticed there are actually two layers where arrow keys get intercepted:
This second layer seems to happen when SwiftUI's internal focus system gets into a broken state after a WKWebView has been in the responder chain — same issue that the terminal path already works around at line 11159 by routing non-Command keys directly to Not sure if you're seeing the same thing on your end, but thought it might be worth flagging. |
![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)
1343-1375: Minor: Pipe reading order afterwaitUntilExit()could block on large outputs.Reading from pipes after
waitUntilExit()(lines 1368-1369) is safe for the small JSON outputs expected here, but could cause a deadlock if the process writes enough to fill the pipe buffer before exiting.For this use case (cmux CLI returning short JSON), the current implementation is fine. If this helper is ever reused for commands with larger outputs, consider reading pipes asynchronously or before waiting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxUITests/BrowserPaneNavigationKeybindUITests.swift` around lines 1343 - 1375, The helper executeCmuxCommand reads stdout/stderr synchronously after waitUntilExit(), which can deadlock if a subprocess produces large output; change it to read the pipes asynchronously (or start background reads on stdoutPipe.fileHandleForReading and stderrPipe.fileHandleForReading using readToEndOfFileInBackgroundAndNotify or Dispatch I/O) before calling waitUntilExit(), capture the completed Data results, convert/trimming them the same way, and then return the terminationStatus, stdout and stderr; ensure you still set process.environment and process.arguments as currently done.
🤖 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 1343-1375: The helper executeCmuxCommand reads stdout/stderr
synchronously after waitUntilExit(), which can deadlock if a subprocess produces
large output; change it to read the pipes asynchronously (or start background
reads on stdoutPipe.fileHandleForReading and stderrPipe.fileHandleForReading
using readToEndOfFileInBackgroundAndNotify or Dispatch I/O) before calling
waitUntilExit(), capture the completed Data results, convert/trimming them the
same way, and then return the terminationStatus, stdout and stderr; ensure you
still set process.environment and process.arguments as currently done.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c499150c-f11a-492f-8535-712ef089c340
📒 Files selected for processing (1)
cmuxUITests/BrowserPaneNavigationKeybindUITests.swift
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmuxUITests/BrowserPaneNavigationKeybindUITests.swift (1)
415-418: Replace the fixed sleep before the click.The
0.15second delay is still a race on slower UI runners; wait forbrowserPaneto be hittable, or another explicit post-Cmd+Lreadiness signal, before clicking.♻️ Suggested change
- RunLoop.current.run(until: Date().addingTimeInterval(0.15)) + XCTAssertTrue( + waitForElementToBecomeHittable(browserPane, timeout: 6.0), + "Expected browser pane to be hittable before clicking the secondary page input" + ) browserPane .coordinate(withNormalizedOffset: CGVector(dx: harness.secondaryCenterX, dy: harness.secondaryCenterY)) .click()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxUITests/BrowserPaneNavigationKeybindUITests.swift` around lines 415 - 418, Replace the hard-coded RunLoop delay with an explicit wait for browserPane to be ready: remove the RunLoop.current.run(until: Date().addingTimeInterval(0.15)) call and instead wait for browserPane to become hittable (e.g., use an NSPredicate "isHittable == true" with expectation(for:evaluatedWith:handler:) and wait(for:timeout:) or XCTWaiter) before calling browserPane.coordinate(...).click(); reference the XCUIElement instance browserPane and the existing harness.secondaryCenterX/secondaryCenterY coordinate usage when implementing the wait.
🤖 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 1359-1368: The browserEval helper currently calls
executeCmuxCommand which can block indefinitely; update browserEval (or
executeCmuxCommand) to enforce a subprocess timeout (e.g., pass a timeout
parameter or implement a DispatchWorkItem/Timer that kills the Process and
returns a timeout result) so the test predicate never hangs, and ensure stderr
is captured and surfaced in the returned result or error logging (include stderr
text when returning nil/timeout) to aid diagnostics; change references inside
browserEval (function name: browserEval, called function: executeCmuxCommand)
accordingly so a non-zero/timeout termination returns promptly with
stdout/stderr available.
---
Nitpick comments:
In `@cmuxUITests/BrowserPaneNavigationKeybindUITests.swift`:
- Around line 415-418: Replace the hard-coded RunLoop delay with an explicit
wait for browserPane to be ready: remove the RunLoop.current.run(until:
Date().addingTimeInterval(0.15)) call and instead wait for browserPane to become
hittable (e.g., use an NSPredicate "isHittable == true" with
expectation(for:evaluatedWith:handler:) and wait(for:timeout:) or XCTWaiter)
before calling browserPane.coordinate(...).click(); reference the XCUIElement
instance browserPane and the existing harness.secondaryCenterX/secondaryCenterY
coordinate usage when implementing the wait.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7a2e4417-4ee7-4ffd-904c-0aadb5c80be9
📒 Files selected for processing (1)
cmuxUITests/BrowserPaneNavigationKeybindUITests.swift
…arrow-keys-merge # Conflicts: # cmuxUITests/BrowserPaneNavigationKeybindUITests.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 (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="cmuxUITests/BrowserPaneNavigationKeybindUITests.swift">
<violation number="1" location="cmuxUITests/BrowserPaneNavigationKeybindUITests.swift:1976">
P2: The counter-stability helper can pass even when no telemetry snapshot is read, so the new arrow-leak tests may report success without actually verifying counters.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
2 participants
Summary
Testing
Task
Summary by cubic
Fixes omnibar focus and key routing so arrows and Return/Enter reach page inputs after leaving the address bar. Web‑view click handoff now uses authoritative omnibar state so focus reliably returns to the page.
Bug Fixes
performKeyEquivalentfor browser content keys; sendkeyDownto the web view or field editor when a browser responder is focused and the omnibar isn’t, then fall back to app/menu shortcuts beforekeyDownso arrows (incl. Cmd/Shift) and Return/Enter reach inputs/contenteditables before AppKit..browserWillBlurAddressBarForWebViewClickand.browserDidBlurAddressBar. Keep web‑view focus policy in sync via newBrowserPanelhelpers.Tests/Tooling
Addresses the Linear task “Browser workspace up/down arrows do not move the cursor inside web text fields after returning focus from the omnibar.”
Written for commit 07b36b9. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests
Chores