Repository navigation
Fix Left/Right arrow keys in browser surface - #3627
Closed
austinywang wants to merge 8 commits into
Closed
austinywang wants to merge 8 commits into
austinywang wants to merge 8 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughExtends browser-arrow routing from Up/Down to all four arrow keys, updates related logs/comments, adds test helpers and a functional test verifying Left/Right caret movement in browser inputs, and records the change in CHANGELOG.md. Browser Horizontal Arrow Fix
Sequence Diagram(s)sequenceDiagram
participant TestClient as Test Client
participant App as cmux App
participant WebView as Browser Surface / WebView
TestClient->>App: create browser pane & focus
TestClient->>WebView: inject input & set caret via browser.eval
TestClient->>App: send Left/Right arrow shortcut
App->>App: shouldDispatchBrowserArrowViaFirstResponderKeyDown(keyCode)
alt dispatch to first responder
App->>WebView: forward keyDown via firstResponder.keyDown
else normal handling
App->>App: normal shortcut dispatch path
end
WebView->>TestClient: return selectionStart via browser.eval
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 11 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (11 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 |
Contributor
Greptile SummaryThis PR restores plain Left/Right arrow caret navigation in browser panes by extending the
Confidence Score: 5/5Test-only change that follows established patterns in the file; cleanup is properly guarded and browser load is awaited with a real signal. The diff is entirely test scaffolding: new helpers and one regression test. The test uses browser.wait and wait_for_webview_focus for readiness rather than bare sleeps. Workspace cleanup is correctly handled in a finally block. No production logic is touched. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant T as Test
participant C as cmux client
participant W as WKWebView
participant JS as DOM (browser.eval)
T->>C: new_workspace() + select_workspace()
T->>C: new_pane(browser, about:blank)
T->>C: "v2_call(browser.wait, load_state=complete)"
C-->>T: ok
T->>C: focus_webview() + wait_for_webview_focus()
C-->>T: ok
T->>C: browser_eval(inject input, setSelectionRange(2,2))
C->>W: evaluate JS
W->>JS: "create #cmux-arrow-probe, focus, position caret"
JS-->>C: "{active:true, start:2}"
C-->>T: initial dict
T->>C: simulate_shortcut(left)
C->>W: key event Left arrow
W->>JS: caret moves to 1
T->>C: "wait_for_selection(expected=1)"
C->>W: browser_eval(selectionStart)
W-->>C: 1
C-->>T: 1 ok
T->>C: simulate_shortcut(right)
C->>W: key event Right arrow
W->>JS: caret moves to 2
T->>C: "wait_for_selection(expected=2)"
C-->>T: 2 ok
T->>C: select_workspace(previous) + close_workspace(ws_id)
Reviews (9): Last reviewed commit: "Address browser keybind test review feed..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
austinywang
force-pushed
the
issue-3622-browser-horizontal-arrows
branch
2 times, most recently
from
May 6, 2026 07:49
f8daaad to
8f5b5cd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@CHANGELOG.md`:
- Around line 7-8: Add a blank line after the "### Fixed" heading in
CHANGELOG.md so the heading is separated from the list (to satisfy markdownlint
rule MD022); locate the "### Fixed" heading and insert a single empty line
before the "- Fixed: Left/Right arrow keys..." list item.
In `@tests/test_browser_custom_keybinds.py`:
- Around line 26-42: In v2_call, the expression "params or {}" masks an
explicitly passed empty dict; update the logic to check for None explicitly
(e.g., use "params if params is not None else {}") so callers can distinguish
None (no params) from {} (empty params); modify the payload construction in the
v2_call function to use this explicit None check for params.
- Around line 26-42: v2_call currently masks any non-dict JSON "result" by
returning {} which silently loses data; update the v2_call function to return
the parsed result value as-is (i.e., return result without forcing a dict) so
callers receive the actual JSON type (including None, int, list, etc.), and
adjust callers if they rely on dict semantics (e.g., browser_eval and
wait_for_selection) to handle None or other types appropriately; alternatively,
if you prefer strictness, replace the silent substitution with an explicit
error/TypeError when result is not a dict so callers cannot unknowingly lose
data.
- Around line 185-232: The test
test_plain_horizontal_arrows_move_caret_in_browser_input creates a workspace and
browser pane but lacks cleanup; wrap the setup and assertions in a try/finally
and in the finally block remove/close the created pane/workspace (or restore the
previous workspace) using the same client methods you used to create them
(client.new_workspace, client.new_pane, client.select_workspace) so the
workspace/pane is always torn down even on failure; ensure you capture the
created ws_id and browser_id before the try and call the appropriate client
teardown (close pane or delete workspace or re-select the prior workspace) in
the finally to avoid leaking panes between tests.
- Line 86: In wait_for_selection, the polling loop currently uses a constant
request_id value via request_id=f"selection-{expected}" which can collide across
iterations; change it to generate a unique ID per request (e.g., include an
incrementing counter or uuid per loop iteration) so each poll uses a distinct
request_id (replace the literal request_id=f"selection-{expected}" with a
generated value such as f"selection-{expected}-{counter}" or
f"selection-{expected}-{uuid4()}").
- Line 20: The file mixes PEP 604 union syntax with typing.Optional (e.g.,
occurrences of dict[str, Any] | None and int | None at the places referenced and
Optional[str] on line 55); fix by choosing one approach: either add from
__future__ import annotations at the top of the module to allow PEP 604 style
annotations on older Python versions, or replace all usages of X | Y (e.g.,
dict[str, Any] | None, int | None) with typing.Optional[X] or typing.Union[X, Y]
to match the existing Optional usage (update the import to include Union if
needed); ensure consistent annotation style across the functions/classes that
declare those types.
🪄 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: 1eb94975-b36a-422b-bbd9-7b252c3fef1c
📒 Files selected for processing (4)
CHANGELOG.mdSources/App/ShortcutRoutingSupport.swiftSources/AppDelegate.swifttests/test_browser_custom_keybinds.py
The existing browser keybind socket test already exercises key delivery while WKWebView is first responder, so extend it with a focused input caret check for plain Left/Right arrows. This commit intentionally contains no production fix so the regression test can fail against the current routing behavior. Constraint: Existing browser-key socket harness can exercise WKWebView focus and synthetic key events without adding a new XCUITest harness Rejected: Add a new UI test harness | outside the requested scope and unnecessary for this focused regression Confidence: medium Scope-risk: narrow Tested: Not run locally per task and repo policy Not-tested: CI has not yet run this test-only commit
austinywang
force-pushed
the
issue-3622-browser-horizontal-arrows
branch
from
May 6, 2026 07:59
8f5b5cd to
4620dfb
Compare
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
austinywang
force-pushed
the
issue-3622-browser-horizontal-arrows
branch
from
May 6, 2026 08:27
4620dfb to
6b09cb0
Compare
Both the activation workflow and CircleCI macOS unit job failed before running cmux code because ziglang.org returned transient download errors or reset slow tarball transfers. Add bounded retry wrappers around the active Zig download steps so CI can absorb those external failures without changing app behavior. Constraint: CI must pass before the requested tagged app launch Rejected: Force-push the same tree repeatedly | repeated Zig download failures showed this was likely to keep wasting CI cycles Confidence: medium Scope-risk: narrow Directive: Keep this retry local to Zig asset downloads unless other CI dependency fetches show the same failure pattern Tested: git diff --check Not-tested: CI retry behavior locally; local tests and local builds are prohibited for this task
The release build can hang inside the Zig install step when the remote tarball connection stalls without returning an HTTP failure. Keep the existing retry helper but add curl connection, total-transfer, and low-speed bounds so stalled transfers become retryable failures. Constraint: CI must be green before the requested tagged app launch Rejected: Wait indefinitely on the existing run | it was stuck in Install zig with no app build progress Confidence: high Scope-risk: narrow Tested: git diff --check Not-tested: local tests/builds per user instruction
The previous timeout still allowed curl's internal retry loop to keep the Install zig step alive too long. Keep one retry owner by relying on the existing shell attempt loop and bounding each transfer directly. Constraint: CI must finish before the requested tagged app launch Rejected: Nested curl retries plus outer retries | total wall-clock budget was not obvious and still stalled CircleCI Confidence: high Scope-risk: narrow Directive: Keep Zig download retry budgeting in one layer; do not reintroduce nested curl retries without a total retry-max-time Tested: git diff --check Not-tested: local tests/builds per user instruction
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 3
🤖 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 @.github/workflows/perf-activation.yml:
- Around line 89-113: The download_zig_asset function currently saves the Zig
tarball to /tmp/zig.tar.xz without integrity checks; after a successful curl in
download_zig_asset (and before the caller extracts it), download the
corresponding ${ZIG_URL}.minisig and verify the tarball with minisign using the
repository's known Zig public key (fall back to verifying the published SHA256
manifest if minisign is not installed), failing the job if verification fails;
update references to ZIG_URL and download_zig_asset so the caller only proceeds
when the signature or checksum verification succeeds.
In `@tests/test_browser_custom_keybinds.py`:
- Around line 16-20: Add "from __future__ import annotations" at the very top of
the file, remove Dict and Optional from the typing import list, and update
annotations to use built-in generics and PEP 604 unions: replace uses of
typing.Dict[...] with dict[...] and Optional[T] with T | None (or use | None in
signatures/return annotations). Ensure imports now read only the names still
required (e.g., Any) and keep the existing runtime annotations like tuple[bool,
str] unchanged.
- Around line 38-41: The try/except around json.loads(raw) misses TypeError when
raw is None (e.g., _send_command returned None); update the error handling in
the block that calls json.loads(raw) (referencing variables raw, method and the
call site where _send_command is used) to either pre-check raw (if raw is None
or not a str/bytes, raise RuntimeError(f"Invalid v2 JSON response for {method}:
{raw}") ) or broaden the except to catch (json.JSONDecodeError, TypeError) and
re-raise the same RuntimeError from exc so the error surface stays consistent.
🪄 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: 098557ac-e61f-4bbd-b58e-b0304ecda393
📒 Files selected for processing (6)
.circleci/config.yml.github/workflows/perf-activation.ymlCHANGELOG.mdSources/App/ShortcutRoutingSupport.swiftSources/AppDelegate.swifttests/test_browser_custom_keybinds.py
CircleCI repeatedly failed before app code because ziglang.org served the 0.15.2 tarball too slowly for reliable CI. Homebrew publishes zig@0.15 as 0.15.2 and serves bottles through the package manager path already used on macOS runners, so use that pinned formula instead of downloading and verifying the tarball ourselves in PR CI. Constraint: CI must pass before the requested tagged app launch Rejected: Keep retrying ziglang.org tarballs | observed downloads stalled around 40-50 KB/s and failed all three macOS jobs Rejected: Raise tarball timeout | would only make failures slower and keep CI dependent on the degraded endpoint Confidence: high Scope-risk: narrow Directive: Keep PR CI on the pinned zig@0.15 formula unless release packaging specifically requires upstream tarball verification Tested: git diff --check Not-tested: local tests/builds per user instruction
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 (2)
.circleci/config.yml (1)
58-70:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce exact Zig version after Homebrew install and use the installed binary path
Line 59 uses a prefix match (
grep "^${ZIG_REQUIRED}") that allows version mismatches (e.g., "0.15.2-dev" would pass). More critically, after installing with Homebrew at line 67, the script capturesZIG_BINbut never verifies it matches the required version, then callszig versionat line 70 which may resolve a different binary from PATH. This creates CI nondeterminism when Homebrew formula versions drift or another Zig is earlier in PATH.Fix by using exact version matching on line 59, verifying the installed binary version, prioritizing it in PATH, and clearing bash's command cache:
Proposed fix
ZIG_REQUIRED="0.15.2" - if command -v zig >/dev/null 2>&1 && zig version 2>/dev/null | grep -q "^${ZIG_REQUIRED}"; then + if command -v zig >/dev/null 2>&1 && [ "$(zig version 2>/dev/null)" = "${ZIG_REQUIRED}" ]; then echo "zig ${ZIG_REQUIRED} already installed" exit 0 fi ZIG_FORMULA="zig@0.15" echo "Installing zig ${ZIG_REQUIRED} from Homebrew formula ${ZIG_FORMULA}" HOMEBREW_NO_INSTALL_CLEANUP=1 brew install "${ZIG_FORMULA}" ZIG_BIN="$(brew --prefix "${ZIG_FORMULA}")/bin/zig" + INSTALLED_ZIG_VERSION="$("${ZIG_BIN}" version 2>/dev/null || true)" + if [ "${INSTALLED_ZIG_VERSION}" != "${ZIG_REQUIRED}" ]; then + echo "Expected zig ${ZIG_REQUIRED}, got ${INSTALLED_ZIG_VERSION:-<none>} from ${ZIG_BIN}" >&2 + exit 1 + fi + export PATH="$(dirname "${ZIG_BIN}"):${PATH}" + hash -r sudo mkdir -p /usr/local/bin sudo ln -sf "${ZIG_BIN}" /usr/local/bin/zig zig version🤖 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 @.circleci/config.yml around lines 58 - 70, The script currently uses a prefix grep and may call a different zig; change the version check to require an exact match against ZIG_REQUIRED (not prefix), after installing with ZIG_FORMULA verify the installed binary at ZIG_BIN reports the exact ZIG_REQUIRED version, then ensure that ZIG_BIN is prioritized (prepend its parent directory to PATH) before invoking any zig commands and clear the shell command cache (e.g., run hash -r) so subsequent calls like `zig version` use the installed binary..github/workflows/perf-activation.yml (1)
75-86:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSame Zig toolchain drift risk exists in perf workflow install step
Line 76 uses prefix matching (
grep -q "^${ZIG_REQUIRED}") which would accept "0.15.2.1" or other patch versions when "0.15.2" is required. Additionally, lines 82-84 don't guarantee that the subsequentzig versioncall resolves to the just-installed binary—bash may have cached an older location or anotherzigmay appear earlier in PATH. This can silently run the wrong Zig version in benchmark CI.🔧 Proposed fix
ZIG_REQUIRED="0.15.2" - if command -v zig >/dev/null 2>&1 && zig version 2>/dev/null | grep -q "^${ZIG_REQUIRED}"; then + if command -v zig >/dev/null 2>&1 && [ "$(zig version 2>/dev/null)" = "${ZIG_REQUIRED}" ]; then echo "zig ${ZIG_REQUIRED} already installed" else ZIG_FORMULA="zig@0.15" echo "Installing zig ${ZIG_REQUIRED} from Homebrew formula ${ZIG_FORMULA}" HOMEBREW_NO_INSTALL_CLEANUP=1 brew install "${ZIG_FORMULA}" ZIG_BIN="$(brew --prefix "${ZIG_FORMULA}")/bin/zig" + INSTALLED_ZIG_VERSION="$("${ZIG_BIN}" version 2>/dev/null || true)" + if [ "${INSTALLED_ZIG_VERSION}" != "${ZIG_REQUIRED}" ]; then + echo "Expected zig ${ZIG_REQUIRED}, got ${INSTALLED_ZIG_VERSION:-<none>} from ${ZIG_BIN}" >&2 + exit 1 + fi + export PATH="$(dirname "${ZIG_BIN}"):${PATH}" + hash -r sudo mkdir -p /usr/local/bin sudo ln -sf "${ZIG_BIN}" /usr/local/bin/zig zig version fi🤖 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 @.github/workflows/perf-activation.yml around lines 75 - 86, The current check uses prefix matching (grep -q "^${ZIG_REQUIRED}") and later runs `zig version` which may resolve to a different binary; change the version check to require an exact match (e.g., grep -q "^${ZIG_REQUIRED}$" or compare the full output string) and, after installing, verify the installed binary explicitly by invoking the resolved path (use the ZIG_BIN variable or /usr/local/bin/zig) rather than an unqualified `zig`; also clear the shell command hash (e.g., `hash -r`) or ensure PATH ordering before invoking the verification to guarantee the just-installed Zig is the one being checked.
🤖 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 @.circleci/config.yml:
- Around line 58-70: The script currently uses a prefix grep and may call a
different zig; change the version check to require an exact match against
ZIG_REQUIRED (not prefix), after installing with ZIG_FORMULA verify the
installed binary at ZIG_BIN reports the exact ZIG_REQUIRED version, then ensure
that ZIG_BIN is prioritized (prepend its parent directory to PATH) before
invoking any zig commands and clear the shell command cache (e.g., run hash -r)
so subsequent calls like `zig version` use the installed binary.
In @.github/workflows/perf-activation.yml:
- Around line 75-86: The current check uses prefix matching (grep -q
"^${ZIG_REQUIRED}") and later runs `zig version` which may resolve to a
different binary; change the version check to require an exact match (e.g., grep
-q "^${ZIG_REQUIRED}$" or compare the full output string) and, after installing,
verify the installed binary explicitly by invoking the resolved path (use the
ZIG_BIN variable or /usr/local/bin/zig) rather than an unqualified `zig`; also
clear the shell command hash (e.g., `hash -r`) or ensure PATH ordering before
invoking the verification to guarantee the just-installed Zig is the one being
checked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 28c8aaee-64b1-4b93-85c7-d26f9f87cdd9
📒 Files selected for processing (2)
.circleci/config.yml.github/workflows/perf-activation.yml
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
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
Fixes #3622
References #2780
Testing
Note
Low Risk
Low risk because changes are isolated to a Python regression test harness and do not modify production keybinding or browser behavior.
Overview
Adds a new regression test ensuring plain Left/Right arrow key events are delivered to focused WKWebView text inputs by verifying caret movement inside an injected
<input>.Refactors the test harness to support JSON-RPC style browser commands via
v2_call/browser_eval, and adds polling helperwait_for_selectionto make the caret assertions less timing-sensitive.Reviewed by Cursor Bugbot for commit 6849b23. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Restores plain Left/Right/Up/Down caret navigation in browser panes by sending unmodified arrows to the WebKit first responder. Switches macOS CI to pinned
zig@0.15via Homebrew for more reliable builds; fixes #3622.Bug Fixes
browser.evalwith caret-position polling to verify Left/Right move the caret.Dependencies
zig@0.15via Homebrew in CircleCI and the perf-activation workflow; remove tarball/minisign downloads and custom retry logic.Written for commit 6849b23. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
Bug Fixes
Tests
Documentation
Chores