Repository navigation
Add failing regression test for browser find focus - #1891
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 a UI test verifying Cmd+F focuses the in-page find field after creating a split and navigating via the omnibar; changes find-overlay focus handling (AppDelegate, BrowserPanel, BrowserPanelView, BrowserSearchOverlay) to avoid treating the overlay as web content and to manage programmatic focus via an AppKit-backed native text field; updates CI/workflow and guard script to run the new test. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant App as App/UI
participant Window as NSWindow
participant Panel as BrowserPanel
participant Overlay as BrowserSearchOverlay
participant WebView as CmuxWebView
rect rgba(200,200,255,0.5)
User->>App: Cmd+D (split)
App->>Panel: create right split
end
rect rgba(200,255,200,0.5)
User->>App: Cmd+L (omnibar)
App->>Panel: focus omnibar / navigate to example.com
Panel->>WebView: load URL
end
rect rgba(255,200,200,0.5)
User->>App: Cmd+F
App->>Panel: startFind()
Panel->>Panel: clear pendingAddressBarFocusRequestId
Panel->>App: post .browserDidBlurAddressBar(panel.id)
Panel->>Overlay: beginSearchFocusRequest
Overlay->>Window: request window.makeFirstResponder(nativeTextField) if allowed
Window->>Overlay: native field becomes first responder
Overlay->>User: accepts typed input into find field
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 396aeb99db
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Greptile SummaryThis PR adds a single failing XCUITest (
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant T as XCUITest
participant A as cmux App
participant DB as Data File (JSON)
T->>A: launch (RECORD_ONLY=1, SOCKET_PATH, DATA_PATH)
A-->>T: runningForeground
T->>A: typeKey("d", .command) [Cmd+D → split right]
T->>DB: waitForDataMatch(timeout:6s) lastSplitDirection=="right" && paneCount>=2
DB-->>T: split confirmed
T->>A: typeKey("l", .command) [Cmd+L → open omnibar]
T->>A: waitForExistence("BrowserOmnibarTextField", timeout:8s)
A-->>T: omnibar visible
T->>A: typeKey("a", .command) [select all]
T->>A: typeKey(delete)
T->>A: typeText("example.com")
T->>A: typeKey(return)
T->>A: waitForCondition(timeout:8s) omnibar contains "example.com"
A-->>T: navigation confirmed
T->>A: typeKey("f", .command) [Cmd+F → open find]
T->>A: waitForExistence("BrowserFindSearchTextField", timeout:6s)
A-->>T: find field visible
Note over T: Capture omnibarValueBeforeFindTyping
T->>A: typeText("needle")
T->>A: waitForCondition(timeout:4s) findField.value == "needle"
Note over T,A: BUG: focus lands in omnibar → findField stays empty → test FAILS
T->>T: XCTAssertEqual(omnibar.value, omnibarValueBeforeFindTyping)
Note over T,A: BUG: omnibar receives "needle" → value changed → test FAILS
Last reviewed commit: "test: add browser fi..." |
![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
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
551-554: Harden virtual-display lifecycle handling.The helper is started in the background but this step doesn’t verify it stayed alive or clean it up. Add a quick liveness check and an
if: always()cleanup step to reduce flakiness.Suggested hardening
- name: Create virtual display run: | set -euo pipefail @@ /tmp/create-virtual-display & VDISPLAY_PID=$! echo "VDISPLAY_PID=$VDISPLAY_PID" >> "$GITHUB_ENV" sleep 3 + kill -0 "$VDISPLAY_PID" echo "=== Display after ===" system_profiler SPDisplaysDataType 2>/dev/null || echo "(none)" + + - name: Cleanup virtual display + if: always() + run: | + set -euo pipefail + if [ -n "${VDISPLAY_PID:-}" ]; then + kill "$VDISPLAY_PID" >/dev/null 2>&1 || true + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 551 - 554, Start the background virtual-display helper (/tmp/create-virtual-display) and immediately verify it stayed alive by checking the process id stored in VDISPLAY_PID (ensure VDISPLAY_PID is exported to GITHUB_ENV as shown), e.g., test the process with kill -0 or wait -n to detect early failure and fail the job if it died; then add a separate cleanup job/step that runs with if: always() which checks for VDISPLAY_PID and kills/cleans the process (and removes any temp files) to guarantee teardown. Ensure the liveness check references VDISPLAY_PID and the cleanup step references the same VDISPLAY_PID variable so the lifecycle is robust.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Line 497: The workflow uses a custom runner label "warp-macos-15-arm64-6x"
which actionlint doesn't know about; add an actionlint config file
(.actionlintrc.yaml or .actionlint.yaml) at the repository root and declare that
custom self-hosted runner label so actionlint validation succeeds (ensure the
config lists the label used in the runs-on: warp-macos-15-arm64-6x entry).
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 551-554: Start the background virtual-display helper
(/tmp/create-virtual-display) and immediately verify it stayed alive by checking
the process id stored in VDISPLAY_PID (ensure VDISPLAY_PID is exported to
GITHUB_ENV as shown), e.g., test the process with kill -0 or wait -n to detect
early failure and fail the job if it died; then add a separate cleanup job/step
that runs with if: always() which checks for VDISPLAY_PID and kills/cleans the
process (and removes any temp files) to guarantee teardown. Ensure the liveness
check references VDISPLAY_PID and the cleanup step references the same
VDISPLAY_PID variable so the lifecycle is robust.
|
|
||
| ui-browser-find-focus-regression: | ||
| if: github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository | ||
| runs-on: warp-macos-15-arm64-6x |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locating actionlint configuration..."
CONFIGS="$(fd -a 'actionlint\\.ya?ml$' || true)"
if [ -z "$CONFIGS" ]; then
echo "No actionlint config found. Add one and declare custom self-hosted labels."
exit 0
fi
echo "$CONFIGS"
CFG="$(echo "$CONFIGS" | head -n1)"
echo "Inspecting: $CFG"
rg -n 'self-hosted-runner|labels|warp-macos-15-arm64-6x|runner' "$CFG" || trueRepository: manaflow-ai/cmux
Length of output: 171
Add actionlint configuration to declare custom Warp runner labels.
The custom runner label warp-macos-15-arm64-6x is not registered with actionlint. Create an .actionlintrc.yaml or .actionlint.yaml file at the repository root with the self-hosted runner labels to prevent workflow validation failures.
🧰 Tools
🪛 actionlint (1.7.11)
[error] 497-497: label "warp-macos-15-arm64-6x" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2025-vs2026", "windows-2022", "windows-11-arm", "ubuntu-slim", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "macos-latest", "macos-latest-xlarge", "macos-latest-large", "macos-26-xlarge", "macos-26-large", "macos-26", "macos-15-intel", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xlarge", "macos-14-large", "macos-14", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml at line 497, The workflow uses a custom runner
label "warp-macos-15-arm64-6x" which actionlint doesn't know about; add an
actionlint config file (.actionlintrc.yaml or .actionlint.yaml) at the
repository root and declare that custom self-hosted runner label so actionlint
validation succeeds (ensure the config lists the label used in the runs-on:
warp-macos-15-arm64-6x entry).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
493-494: Minor: Add blank line between steps for consistency.The rest of the workflow has blank lines between steps for readability.
📝 Suggested formatting
-only-testing:cmuxUITests/DisplayResolutionRegressionUITests \ test + - name: Run browser find focus UI regression🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 493 - 494, Add a blank line to match the workflow's step separation style by inserting an empty line before the step with name "Run browser find focus UI regression" so there is a blank line between the preceding "test" step and this step; update the CI YAML so each top-level job step maintains the same blank-line spacing for readability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 493-494: Add a blank line to match the workflow's step separation
style by inserting an empty line before the step with name "Run browser find
focus UI regression" so there is a blank line between the preceding "test" step
and this step; update the CI YAML so each top-level job step maintains the same
blank-line spacing for readability.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af1175ac81
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/Find/BrowserSearchOverlay.swift (1)
105-110: Duplicate notification handling for.browserSearchFocus.This
.onReceivehandler setsisSearchFieldFocused = true, which triggersupdateNSViewto callmakeFirstResponder. However,BrowserSearchTextFieldRepresentablealso observes the same notification at lines 288-304 and directly callswindow.makeFirstResponder(field).Both paths guard against already-focused state, so this won't cause functional issues, but it's redundant. Consider removing one of the handlers—the Coordinator's observer (lines 288-304) is more direct and doesn't rely on SwiftUI state propagation timing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Find/BrowserSearchOverlay.swift` around lines 105 - 110, Remove the duplicate `.onReceive` handler that listens for `.browserSearchFocus` and sets `isSearchFieldFocused = true`; instead rely on the existing direct observer in `BrowserSearchTextFieldRepresentable.Coordinator` (lines handling `window.makeFirstResponder(field)`). Specifically, delete the `.onReceive(NotificationCenter.default.publisher(for: .browserSearchFocus)) { ... }` block in `BrowserSearchOverlay` (the code that checks `notifiedPanelId == panelId`, `canApplyFocusRequest(focusRequestGeneration)`, and sets `isSearchFieldFocused = true`) so focus is handled only by the Coordinator's observer which uses `makeFirstResponder` directly. Ensure no other logic depends on `isSearchFieldFocused` being toggled by this notification before removing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/Find/BrowserSearchOverlay.swift`:
- Around line 105-110: Remove the duplicate `.onReceive` handler that listens
for `.browserSearchFocus` and sets `isSearchFieldFocused = true`; instead rely
on the existing direct observer in
`BrowserSearchTextFieldRepresentable.Coordinator` (lines handling
`window.makeFirstResponder(field)`). Specifically, delete the
`.onReceive(NotificationCenter.default.publisher(for: .browserSearchFocus)) {
... }` block in `BrowserSearchOverlay` (the code that checks `notifiedPanelId ==
panelId`, `canApplyFocusRequest(focusRequestGeneration)`, and sets
`isSearchFieldFocused = true`) so focus is handled only by the Coordinator's
observer which uses `makeFirstResponder` directly. Ensure no other logic depends
on `isSearchFieldFocused` being toggled by this notification before removing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7b272686-1ae3-4e6e-ac9e-11ff7719ade5
📒 Files selected for processing (3)
Sources/AppDelegate.swiftSources/Find/BrowserSearchOverlay.swiftSources/Panels/BrowserPanelView.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 717ac6df29
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97b2202b0b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let alreadyFocused = fr === field || | ||
| field.currentEditor() != nil || | ||
| ((fr as? NSTextView)?.delegate as? NSTextField) === field |
There was a problem hiding this comment.
The alreadyFocused check dereferences NSTextView.delegate (((fr as? NSTextView)?.delegate as? NSTextField)) to detect field-editor ownership, but AppKit stores that delegate as unsafe-unretained; if responder teardown has already released the backing text field, this access can trap with an unowned-reference crash. This path runs in both the .browserSearchFocus observer and async focus retries, so closing/churning panes while Cmd+F focus handoff is in flight can crash the app; use a safe ownership check (e.g., currentEditor()/responder-chain walk) instead of touching delegate directly.
Useful? React with 👍 / 👎.
| if BrowserWindowPortalRegistry.searchOverlayPanelId(for: responder, in: window) != nil { | ||
| return nil |
There was a problem hiding this comment.
This new early check runs before any responder-type filtering, so every cmuxOwningWebView(for:in:event:) call now performs a portal-wide searchOverlayPanelId scan even when the first responder is unrelated (e.g., terminal/web view). Because this helper is called from cmux_performKeyEquivalent on key events, the change adds avoidable O(number of browser slots) work to a typing hot path and can regress input latency in sessions with many panes; limit the lookup to field-editor responders (the only case that needs special-casing).
Useful? React with 👍 / 👎.
* test: add browser find focus regression * ci: run browser find focus regression on prs * ci: merge pr ui regressions into one job * fix: focus browser find after navigation * test: rename ui regressions workflow guard * Fix browser find focus after address bar navigation * Fix test-e2e workflow manifest script * Address PR review feedback * Stabilize browser focus regression on CI --------- Co-authored-by: Lawrence Chen <lawrencecchen@users.noreply.github.com>
This branch was successfully deployed
1 participant
Summary
Cmd+D,Cmd+L, navigate toexample.com,Cmd+Frepro.Cmd+Flands inBrowserFindSearchTextField, not the omnibar.Testing
test-depot.yml: https://github.com/manaflow-ai/cmux/actions/runs/23369104453test-e2e.ymlcurrently exits before running tests because the workflow shell script has a syntax error: https://github.com/manaflow-ai/cmux/actions/runs/23369047528Task
Cmd+D,Cmd+L, navigate toexample.com,Cmd+Fshould focus the browser find input.Summary by CodeRabbit
Bug Fixes
Tests
Chores