Repository navigation
Fix BrowserOmnibarSuggestionsUITests e2e lane - #6940
Closed
austinywang wants to merge 46 commits into
Closed
austinywang wants to merge 46 commits into
austinywang wants to merge 46 commits into
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:
📝 WalkthroughWalkthroughThe PR updates macOS runner selection, adds a persistent virtual-display check before omnibar UI regressions, and changes omnibar inline backspace handling with updated unit and UI test synchronization. ChangesOmnibar regression CI and test flow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (22 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 |
…romnibarsuggestionsuitests-cannot-r # Conflicts: # .github/swift-file-length-budget.tsv
Contributor
Greptile SummaryThis PR stabilizes the
Confidence Score: 5/5Safe to merge — the production Swift change is narrow, well-tested by new unit tests, and the UITest and CI changes are all in test/infra code. The production change in BrowserPanelView.swift is a small, focused expansion of the backspace-intercept predicate, covered by three new Swift Testing unit tests. The UITest improvements remove a known flaky pattern (XCUITest key-event backspace) and replace it with a socket-driven command with an explicit readiness gate. The CI changes correctly fail closed to a display-capable Depot runner and add the two new tests before the display-churn step. No unguarded paths, no dropped state, no timing races introduced. No files require special attention. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant CI as ci.yml (ui-regressions)
participant Depot as Depot macOS Runner
participant XCTest as XCUITest
participant Socket as Control Socket
participant App as cmux App
CI->>Depot: "run on depot-macos-* (fail-closed guard)"
Depot->>XCTest: xcodebuild test-without-building
XCTest->>App: launch with CMUX_SOCKET_PATH + CMUX_UI_TEST_GOTO_SPLIT_SETUP
App-->>XCTest: "webViewFocused=true (goto_split data file)"
XCTest->>App: typeQueryAndWaitForSuggestions("exam")
App-->>XCTest: BrowserOmnibarSuggestions visible
XCTest->>XCTest: waitForInlineCompletion("example.com")
XCTest->>Socket: simulate_shortcut backspace
Socket->>App: AppKit deleteBackward event
App->>App: inlineCompletionSelectionIsActive() → onDeleteBackwardWithInlineSelection()
App-->>XCTest: "omnibar.value == "exa""
XCTest->>XCTest: waitForCondition → assert "exa"
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant CI as ci.yml (ui-regressions)
participant Depot as Depot macOS Runner
participant XCTest as XCUITest
participant Socket as Control Socket
participant App as cmux App
CI->>Depot: "run on depot-macos-* (fail-closed guard)"
Depot->>XCTest: xcodebuild test-without-building
XCTest->>App: launch with CMUX_SOCKET_PATH + CMUX_UI_TEST_GOTO_SPLIT_SETUP
App-->>XCTest: "webViewFocused=true (goto_split data file)"
XCTest->>App: typeQueryAndWaitForSuggestions("exam")
App-->>XCTest: BrowserOmnibarSuggestions visible
XCTest->>XCTest: waitForInlineCompletion("example.com")
XCTest->>Socket: simulate_shortcut backspace
Socket->>App: AppKit deleteBackward event
App->>App: inlineCompletionSelectionIsActive() → onDeleteBackwardWithInlineSelection()
App-->>XCTest: "omnibar.value == "exa""
XCTest->>XCTest: waitForCondition → assert "exa"
Reviews (34): Last reviewed commit: "test: use socket shortcut for omnibar ba..." | Re-trigger Greptile |
This comment has been minimized.
This comment has been minimized.
added 10 commits
June 26, 2026 19:20
…romnibarsuggestionsuitests-cannot-r
…romnibarsuggestionsuitests-cannot-r # Conflicts: # .github/swift-file-length-budget.tsv
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxUITests/BrowserOmnibarSuggestionsUITests.swift`:
- Line 594: The test in BrowserOmnibarSuggestionsUITests is using a fixed
RunLoop.current.run delay before the socket backspace assertion, which should be
removed. Rely on the existing waitForInlineCompletion state instead, or replace
the sleep with a direct wait on the specific readiness predicate that guarantees
the socket is ready. Update the backspace test flow so the assertion is driven
by observable causality rather than wall-clock timing.
🪄 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: a7567668-3b9b-438e-8835-0cc7e2ca58d8
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (5)
.github/workflows/ci.yml.github/workflows/test-e2e.ymlSources/TerminalController.swiftcmuxUITests/BrowserOmnibarSuggestionsUITests.swifttests/test_ci_self_hosted_guard.sh
austinywang
force-pushed
the
issue-5934-e2e-browseromnibarsuggestionsuitests-cannot-r
branch
from
June 27, 2026 05:55
a53e8f4 to
6bf81e2
Compare
austinywang
force-pushed
the
issue-5934-e2e-browseromnibarsuggestionsuitests-cannot-r
branch
from
June 27, 2026 06:09
6bf81e2 to
0c34b81
Compare
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/Panels/BrowserPanelView.swift (1)
4619-4627: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winKeep
inlineCompletionSelectionIsActive(...)boundary-only
deleteBackwardnow routes throughonDeleteBackwardWithInlineSelection()whenever this helper is true. Keep that helper limited to the typed-prefix boundary or a selected suffix; if it ever matches a mid-prefix caret, Backspace will delete the trailing character instead of the character before the cursor.🤖 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/Panels/BrowserPanelView.swift` around lines 4619 - 4627, Keep inlineCompletionSelectionIsActive(textView, inline:) limited to the typed-prefix boundary or a selected suffix so deleteBackward only uses onDeleteBackwardWithInlineSelection() in those cases. Review the helper’s matching logic in BrowserPanelView and ensure a mid-prefix caret does not satisfy it; otherwise Backspace will remove the wrong character. Preserve the existing deleteBackward selector flow and adjust the helper’s boundary checks rather than the caller.
🤖 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/Panels/BrowserPanelView.swift`:
- Around line 4619-4627: Keep inlineCompletionSelectionIsActive(textView,
inline:) limited to the typed-prefix boundary or a selected suffix so
deleteBackward only uses onDeleteBackwardWithInlineSelection() in those cases.
Review the helper’s matching logic in BrowserPanelView and ensure a mid-prefix
caret does not satisfy it; otherwise Backspace will remove the wrong character.
Preserve the existing deleteBackward selector flow and adjust the helper’s
boundary checks rather than the caller.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 75f93372-8a46-4919-b01d-d1e629baf1dc
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (3)
Sources/Panels/BrowserPanelView.swiftcmuxTests/OmnibarAndToolsTests.swiftcmuxUITests/BrowserOmnibarSuggestionsUITests.swift
…s-cannot-r Resolve .github/swift-file-length-budget.tsv by regenerating it with scripts/swift_file_length_budget.py --write-budget (per repo policy; not hand-edited). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
testInlineAutocompleteBackspaceDeletesTypedPrefixCharacter dispatches Ctrl+H through the control socket (simulate_shortcut), which runs synchronously on the main thread (v2MainSync -> AppKit sendEvent -> the omnibar deleteBackward) and only acks after that hop returns. Right after the omnibar renders inline suggestions the main thread can stay busy longer than the default 2s netcat read window, so nc closes before the ack lands and socketCommand returns nil even though the dispatch is healthy -- the failure diagnostics captured right after still report socketPingResponse=PONG, socketAcceptLoopAlive=1, and 5 live windows. Give this single call a generous 10s budget instead of retrying: Ctrl+H mutates the buffer, so re-dispatching after a lost ack would delete a second character and corrupt the assertion. Scoped via a new responseTimeout parameter on socketCommand (default 2s leaves the ping readiness probe unchanged). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
The ui-regressions job runs browser XCUITests directly against the runner's native WindowServer display instead of self-provisioning a virtual display: the job is pinned to a Depot display runner via repo variable MACOS_RUNNER_DISPLAY, and the existing 'Validate display runner identity' step fails the job if a requested depot-* runner resolves outside Depot. That invariant is why this lane dropped its old persistent virtual display, and why the later display-churn step still creates its own scoped virtual display (it exercises display add/remove churn). The prior green run on this branch confirmed both browser UI steps pass on the pinned display runner without a virtual display. Documenting inline so the design is legible to future reviewers. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The ui-regressions lane failed in testInlineAutocompleteBackspaceDeletesTypedPrefixCharacter: the control socket's `simulate_shortcut ctrl+h` returned nil even with a 10s netcat read window, while the app was demonstrably up (ping->PONG, main window visible, omnibar.value readable both before and after the call). Root cause: `simulate_shortcut` dispatches onto the main thread via `v2MainSync` (DispatchQueue.main.sync) and only writes its "OK" ack after that hop returns. `ping` never hops to main, so it always answers -- which let `waitForSocketPong` pass via its ping/diagnostics fallback and masked that the mutating command's main-thread hop was being starved by concurrent XCUITest main-thread traffic right after the suggestions render. Widening the netcat timeout cannot fix a hop that never gets serviced (proven: 2s and 10s both return nil). Drive Backspace as a real HID key event through XCUITest instead (`app.typeKey(.delete)`), which reaches the focused omnibar and invokes `deleteBackward(_:)` -- the exact command BrowserPanelView intercepts for inline-completion boundary deletion -- over the always-serviced event path the sibling Cmd+A test already relies on. Remove the now-unused per-test socket scaffolding (launch env, pong gate, diagnostics + netcat helpers); every other test in this file already launches without it. Side effect: file drops from 882 to 832 lines, back under its swift-file-length budget, fixing the workflow-guard-tests "Validate Swift file length budget" failure without a budget bump. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…romnibarsuggestionsuitests-cannot-r
…romnibarsuggestionsuitests-cannot-r # Conflicts: # .github/swift-file-length-budget.tsv
…romnibarsuggestionsuitests-cannot-r
…romnibarsuggestionsuitests-cannot-r
lawrencecchen
added
the
stale-revisit
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
3 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.
Fixes #5934.
Summary
autoruns through the display-capable macOS runner variable and remove the known-bad Warp GUI option.Local validation
./tests/test_ci_self_hosted_guard.shpython3 scripts/swift_file_length_budget.pypython3 tests/test_ci_change_areas.pygit diff --check.github/workflows/ci.ymland.github/workflows/test-e2e.ymlNote: I did not run
reload.shor localxcodebuild, per issue instructions. The depot failures are GUI timing/focus-sensitive and are not cleanly reproducible locally under that constraint; the existing UITests plus the added PR UI regression lane are the executable signals.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Stabilizes the BrowserOmnibarSuggestions e2e lane by pinning UI tests to a display-capable Depot macOS runner, driving Backspace through the app’s control socket to invoke a real deleteBackward event, and fixing inline-completion deletion at typed/display boundaries so the typed prefix updates correctly. CI now runs browser UI regressions before display-churn and removes the persistent virtual-display helper.
MACOS_RUNNER_DISPLAY; validate runner identity; run omnibar and browser-find regressions before display-churn; remove Warp choices; update run-name/concurrency/cache; add guard enforcing the display-runner invariant.goto_splitsetup and ensure the app is foreground; focus by clicking the omnibar pill/field; type via the app and wait for suggestions/inline completion; send Backspace via the control socket (simulate_shortcut backspace) and assert the typed prefix is revealed before Escape; clean temp data on teardown.deleteBackward(_:)only when the inline selection is active and skip mid-prefix carets; treat carets at typed or display inline boundaries as active; rename predicate toinlineCompletionSelectionIsActive; add Swift Testing unit coverage for boundary and mid-prefix deletion.Written for commit dd8e1ea. Summary will update on new commits.
Summary by CodeRabbit