Repository navigation
Wait for E2E virtual display readiness - #4928
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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 a shared UI-test launch helper (cmuxLaunchApp) with a direct-process option, updates many UI tests to use it instead of XCTExpectFailure-wrapped launches, wires the helper into the Xcode project, and makes CI virtual-display startup readiness-based. ChangesUI test launch & CI readiness
Sequence DiagramsequenceDiagram
participant Tester as UI Test
participant Helper as cmuxLaunchApp
participant Process as App Binary
participant App as XCUIApplication
Tester->>Helper: request app launch with args/env
Helper->>Process: spawn binary, redirect stdout/stderr to log
Process->>Helper: write runtime log
Helper->>App: poll app.state until runningForeground/runningBackground or process exits
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 15 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (15 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 SummaryReplaces the fixed
Confidence Score: 5/5Safe to merge — only the CI workflow is touched, replacing a fixed sleep with a deterministic readiness handshake that is already fully implemented in the helper binary. The change is confined to the E2E CI workflow. The No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant WF as Workflow Step
participant H as create-virtual-display
participant FS as Marker Files
WF->>H: "launch (--ready-path, --display-id-path) &"
H-->>FS: write displayIDPath
H-->>FS: write readyPath
H->>H: dispatch_main() (keeps alive)
loop Poll up to 100 × 0.1 s
WF->>FS: "[ -s READY ] && [ -s ID ]"
alt both non-empty
WF-->>WF: break
else helper exited early
WF-->>WF: cat log, exit 1
else still waiting
WF->>WF: sleep 0.1
end
end
WF->>WF: post-loop timeout check (exit 1 if not ready)
WF->>WF: echo display ID, cat log
Reviews (8): Last reviewed commit: "Wait for E2E virtual display readiness" | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
lawrencecchen
force-pushed
the
feat-ui-test-activation-deflake
branch
from
May 28, 2026 06:52
8cef800 to
fe73d5f
Compare
lawrencecchen
force-pushed
the
feat-ui-test-activation-deflake
branch
from
May 28, 2026 07:02
fe73d5f to
0bea59b
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+74
to
+91
| let testBundle = Bundle(for: UITestLaunchSupportBundleToken.self) | ||
| let productsDir = testBundle.bundleURL | ||
| .deletingLastPathComponent() | ||
| .deletingLastPathComponent() | ||
| .deletingLastPathComponent() | ||
| .deletingLastPathComponent() | ||
| let binaryPath = productsDir | ||
| .appendingPathComponent("cmux DEV.app") | ||
| .appendingPathComponent("Contents/MacOS/cmux DEV") | ||
| .path | ||
| if FileManager.default.isExecutableFile(atPath: binaryPath) { | ||
| return binaryPath | ||
| } | ||
|
|
||
| throw NSError(domain: "UITestLaunchSupport", code: 2, userInfo: [ | ||
| NSLocalizedDescriptionKey: "App binary not found at \(binaryPath). testBundle=\(testBundle.bundleURL.path)" | ||
| ]) | ||
| } |
Contributor
There was a problem hiding this comment.
The four deletingLastPathComponent() calls starting from testBundle.bundleURL (which resolves to the .xctest bundle inside the runner) navigate to .../Build/Products/ rather than .../Build/Products/Debug/. Concretely: .xctest → PlugIns → cmuxUITests-Runner.app → Debug → Products. Appending cmux DEV.app then yields Products/cmux DEV.app/... but the actual artifact lives at Products/Debug/cmux DEV.app/.... Because the workflow sets CMUX_UI_TEST_DIRECT_PROCESS_LAUNCH=1 but does NOT set CMUX_UI_TEST_APP_BINARY_PATH, every CI run will immediately fail with "App binary not found" at the incorrect path. Either drop one deletingLastPathComponent() to land on the configuration directory, or set CMUX_UI_TEST_APP_BINARY_PATH explicitly in the workflow step's env.
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 (4)
cmuxUITests/WorkspaceSidebarScrollUITests.swift (1)
248-255:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRequire foreground, not just running/background, before sidebar interaction tests
This helper currently passes when app is backgrounded, but these tests immediately issue keyboard input and visibility assertions that expect an active foreground app.
Suggested fix
private func launchAndEnsureRunning(_ app: XCUIApplication) { cmuxLaunchApp(app) - XCTAssertTrue( - pollUntil(timeout: 10.0) { - app.state == .runningForeground || app.state == .runningBackground - }, - "App failed to launch. state=\(app.state.rawValue)" - ) + XCTAssertTrue( + pollUntil(timeout: 10.0) { + if app.state != .runningForeground { + app.activate() + } + return app.state == .runningForeground + }, + "App did not reach runningForeground before UI interactions. state=\(app.state.rawValue)" + ) }🤖 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 `@cmuxUITests/WorkspaceSidebarScrollUITests.swift` around lines 248 - 255, The helper launchAndEnsureRunning currently treats runningBackground as acceptable; change its readiness check to require the app to be in the .runningForeground state before proceeding so UI and keyboard interactions are safe: update the pollUntil predicate in launchAndEnsureRunning (which calls cmuxLaunchApp and inspects app.state) to return true only when app.state == .runningForeground, and keep the existing timeout/error message behavior.cmuxUITests/UpdatePillUITests.swift (1)
278-289:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce foreground before continuing UI interactions
Both launch helpers can continue even if the app never reaches
.runningForeground, which weakens activation-failure coverage and can mask regressions behind downstream flake.Suggested fix
private func launchAndActivate(_ app: XCUIApplication, activateTimeout: TimeInterval = 2.0) { cmuxLaunchApp(app) let activated = pollUntil(timeout: activateTimeout) { guard app.state != .runningForeground else { return true } app.activate() return app.state == .runningForeground } if !activated { app.activate() } + XCTAssertTrue( + pollUntil(timeout: 2.0) { app.state == .runningForeground }, + "App did not reach runningForeground before UI interactions. state=\(app.state.rawValue)" + ) } @@ - _ = pollUntil(timeout: 2.0) { + let activated = pollUntil(timeout: 2.0) { guard app.state != .runningForeground else { return true } app.activate() return app.state == .runningForeground } + XCTAssertTrue( + activated, + "App did not reach runningForeground before titlebar hint assertions. state=\(app.state.rawValue)" + )Also applies to: 383-391
🤖 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 `@cmuxUITests/UpdatePillUITests.swift` around lines 278 - 289, The helper launchAndActivate currently continues even when activation fails; change it to treat a failed activation as a test failure by replacing the silent fallback with an explicit assertion/termination (e.g., XCTAssertTrue or XCTFail plus return/throw) so the test fails if app.state never becomes .runningForeground after pollUntil; update the same behavior in the other launch helper referenced (the similar helper at lines ~383-391) to enforce foreground before continuing UI interactions.cmuxUITests/BrowserPaneNavigationKeybindUITests.swift (1)
1457-1466:⚠️ Potential issue | 🟠 Major | ⚡ Quick winForeground helper currently succeeds in background state
launchAndEnsureForegroundreturns on.runningBackground, which can let keybind tests proceed without an active app and mask launch/activation regressions.Suggested fix
private func launchAndEnsureForeground(_ app: XCUIApplication, timeout: TimeInterval = 12.0) { cmuxLaunchApp(app) - - if app.state == .runningForeground { return } - - if app.state == .runningBackground { - return - } - - XCTFail("App failed to start. state=\(app.state.rawValue)") + let reachedForeground = waitForCondition(timeout: timeout) { + if app.state != .runningForeground { + app.activate() + } + return app.state == .runningForeground + } + guard reachedForeground else { + XCTFail("App did not reach runningForeground before UI interactions. state=\(app.state.rawValue)") + return + } }🤖 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 `@cmuxUITests/BrowserPaneNavigationKeybindUITests.swift` around lines 1457 - 1466, The helper launchAndEnsureForeground currently returns when app.state == .runningBackground which lets tests proceed with the app in background; update launchAndEnsureForeground to not treat .runningBackground as success: after calling cmuxLaunchApp(app) if app.state != .runningForeground then try to bring it to foreground (call app.activate() and wait/poll for app.state == .runningForeground up to the provided timeout) and only then return; if activation fails within the timeout call XCTFail with the app.state (keep references to launchAndEnsureForeground, cmuxLaunchApp, XCUIApplication, app.activate(), app.state == .runningForeground, .runningBackground, and XCTFail).cmuxUITests/WorkspaceDescriptionUITests.swift (1)
275-282:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
launchAndEnsureForegroundshould not accept background as successThis helper currently returns when state is
.runningBackground, so tests may proceed before the app is interactable in foreground.Suggested fix
private func launchAndEnsureForeground(_ app: XCUIApplication, timeout: TimeInterval = 12.0) { cmuxLaunchApp(app) - - if app.state == .runningForeground { return } - if app.state == .runningBackground { return } - - XCTFail("App failed to start. state=\(app.state.rawValue)") + let reachedForeground = workspaceDescriptionPollUntil(timeout: timeout) { + if app.state != .runningForeground { + app.activate() + } + return app.state == .runningForeground + } + guard reachedForeground else { + XCTFail("App did not reach runningForeground before UI interactions. state=\(app.state.rawValue)") + return + } }🤖 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 `@cmuxUITests/WorkspaceDescriptionUITests.swift` around lines 275 - 282, The helper launchAndEnsureForeground currently treats .runningBackground as a success and can allow tests to continue before the app is interactable; change launchAndEnsureForeground (which calls cmuxLaunchApp) to treat only .runningForeground as success: remove the early return for .runningBackground and instead wait (via a short poll loop or an XCTNSPredicateExpectation on app.state == .runningForeground) up to the timeout, then XCTFail if the app never reaches .runningForeground; ensure any waiting uses the provided timeout parameter and preserves the existing XCTFail message including app.state.rawValue.
🤖 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 `@cmuxUITests/BrowserPaneNavigationKeybindUITests.swift`:
- Around line 1457-1466: The helper launchAndEnsureForeground currently returns
when app.state == .runningBackground which lets tests proceed with the app in
background; update launchAndEnsureForeground to not treat .runningBackground as
success: after calling cmuxLaunchApp(app) if app.state != .runningForeground
then try to bring it to foreground (call app.activate() and wait/poll for
app.state == .runningForeground up to the provided timeout) and only then
return; if activation fails within the timeout call XCTFail with the app.state
(keep references to launchAndEnsureForeground, cmuxLaunchApp, XCUIApplication,
app.activate(), app.state == .runningForeground, .runningBackground, and
XCTFail).
In `@cmuxUITests/UpdatePillUITests.swift`:
- Around line 278-289: The helper launchAndActivate currently continues even
when activation fails; change it to treat a failed activation as a test failure
by replacing the silent fallback with an explicit assertion/termination (e.g.,
XCTAssertTrue or XCTFail plus return/throw) so the test fails if app.state never
becomes .runningForeground after pollUntil; update the same behavior in the
other launch helper referenced (the similar helper at lines ~383-391) to enforce
foreground before continuing UI interactions.
In `@cmuxUITests/WorkspaceDescriptionUITests.swift`:
- Around line 275-282: The helper launchAndEnsureForeground currently treats
.runningBackground as a success and can allow tests to continue before the app
is interactable; change launchAndEnsureForeground (which calls cmuxLaunchApp) to
treat only .runningForeground as success: remove the early return for
.runningBackground and instead wait (via a short poll loop or an
XCTNSPredicateExpectation on app.state == .runningForeground) up to the timeout,
then XCTFail if the app never reaches .runningForeground; ensure any waiting
uses the provided timeout parameter and preserves the existing XCTFail message
including app.state.rawValue.
In `@cmuxUITests/WorkspaceSidebarScrollUITests.swift`:
- Around line 248-255: The helper launchAndEnsureRunning currently treats
runningBackground as acceptable; change its readiness check to require the app
to be in the .runningForeground state before proceeding so UI and keyboard
interactions are safe: update the pollUntil predicate in launchAndEnsureRunning
(which calls cmuxLaunchApp and inspects app.state) to return true only when
app.state == .runningForeground, and keep the existing timeout/error message
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b5d701aa-2e4e-4f89-9d7b-257847153190
📒 Files selected for processing (15)
.github/workflows/test-e2e.ymlcmux.xcodeproj/project.pbxprojcmuxUITests/BonsplitTabDragUITests.swiftcmuxUITests/BrowserPaneNavigationKeybindUITests.swiftcmuxUITests/CommandPaletteIdentifierClipboardUITests.swiftcmuxUITests/FeedSidebarUITests.swiftcmuxUITests/FindSelectionShortcutUITests.swiftcmuxUITests/HelpMenuUITests.swiftcmuxUITests/RightSidebarChromeHeightUITests.swiftcmuxUITests/SidebarHelpMenuUITests.swiftcmuxUITests/TerminalCmdClickUITests.swiftcmuxUITests/UITestLaunchSupport.swiftcmuxUITests/UpdatePillUITests.swiftcmuxUITests/WorkspaceDescriptionUITests.swiftcmuxUITests/WorkspaceSidebarScrollUITests.swift
lawrencecchen
force-pushed
the
feat-ui-test-activation-deflake
branch
from
May 28, 2026 07:15
0bea59b to
25d4620
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
lawrencecchen
force-pushed
the
feat-ui-test-activation-deflake
branch
from
May 28, 2026 07:22
25d4620 to
0c211cb
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/UITestLaunchSupport.swift`:
- Around line 12-15: The current guard uses the wrong env var and makes
direct-process launch the default; update the condition to check
ProcessInfo.processInfo.environment["CMUX_UI_TEST_DIRECT_PROCESS_LAUNCH"] == "1"
and only take the direct path (the code that bypasses app.launch()) when that
opt-in variable is set; otherwise call app.launch() as the default. Ensure you
replace any reference to "CMUX_UI_TEST_XCTEST_LAUNCH" with
"CMUX_UI_TEST_DIRECT_PROCESS_LAUNCH" and keep the calls to app.launch() and the
direct-launch branch intact (refer to ProcessInfo.processInfo.environment and
app.launch()).
- Around line 25-28: The helper currently calls app.terminate() and ignores the
boolean result of cmuxPollUntil(timeout: 3.0) { app.state == .notRunning }, so a
stale app instance can persist; update the helper (the block invoking
app.terminate() and cmuxPollUntil) to check the return value and fail fast when
it returns false—e.g., assert/XCTFail or throw/fatalError with a clear message
if cmuxPollUntil(...) returns false indicating the previous instance did not
exit—so the new launch is aborted instead of proceeding.
🪄 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: 775e4c82-4301-4a3e-86b0-83e2d26bc3dd
📒 Files selected for processing (15)
.github/workflows/test-e2e.ymlcmux.xcodeproj/project.pbxprojcmuxUITests/BonsplitTabDragUITests.swiftcmuxUITests/BrowserPaneNavigationKeybindUITests.swiftcmuxUITests/CommandPaletteIdentifierClipboardUITests.swiftcmuxUITests/FeedSidebarUITests.swiftcmuxUITests/FindSelectionShortcutUITests.swiftcmuxUITests/HelpMenuUITests.swiftcmuxUITests/RightSidebarChromeHeightUITests.swiftcmuxUITests/SidebarHelpMenuUITests.swiftcmuxUITests/TerminalCmdClickUITests.swiftcmuxUITests/UITestLaunchSupport.swiftcmuxUITests/UpdatePillUITests.swiftcmuxUITests/WorkspaceDescriptionUITests.swiftcmuxUITests/WorkspaceSidebarScrollUITests.swift
Comment on lines
+12
to
+15
| guard ProcessInfo.processInfo.environment["CMUX_UI_TEST_XCTEST_LAUNCH"] != "1" else { | ||
| app.launch() | ||
| return | ||
| } |
There was a problem hiding this comment.
Use the documented opt-in flag for direct launch.
Lines 12-15 currently make direct-process launch the default and key it off CMUX_UI_TEST_XCTEST_LAUNCH, but the PR objective says CI should opt in with CMUX_UI_TEST_DIRECT_PROCESS_LAUNCH=1 and local runs should stay on app.launch(). As written, local runs will take the direct path and the documented CI flag is ignored.
Suggested fix
- guard ProcessInfo.processInfo.environment["CMUX_UI_TEST_XCTEST_LAUNCH"] != "1" else {
- app.launch()
- return
- }
+ guard ProcessInfo.processInfo.environment["CMUX_UI_TEST_DIRECT_PROCESS_LAUNCH"] == "1" else {
+ app.launch()
+ return
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| guard ProcessInfo.processInfo.environment["CMUX_UI_TEST_XCTEST_LAUNCH"] != "1" else { | |
| app.launch() | |
| return | |
| } | |
| guard ProcessInfo.processInfo.environment["CMUX_UI_TEST_DIRECT_PROCESS_LAUNCH"] == "1" else { | |
| app.launch() | |
| return | |
| } |
🤖 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 `@cmuxUITests/UITestLaunchSupport.swift` around lines 12 - 15, The current
guard uses the wrong env var and makes direct-process launch the default; update
the condition to check
ProcessInfo.processInfo.environment["CMUX_UI_TEST_DIRECT_PROCESS_LAUNCH"] == "1"
and only take the direct path (the code that bypasses app.launch()) when that
opt-in variable is set; otherwise call app.launch() as the default. Ensure you
replace any reference to "CMUX_UI_TEST_XCTEST_LAUNCH" with
"CMUX_UI_TEST_DIRECT_PROCESS_LAUNCH" and keep the calls to app.launch() and the
direct-launch branch intact (refer to ProcessInfo.processInfo.environment and
app.launch()).
Comment on lines
+25
to
+28
| app.terminate() | ||
| _ = cmuxPollUntil(timeout: 3.0) { | ||
| app.state == .notRunning | ||
| } |
There was a problem hiding this comment.
Fail fast if the previous app instance never exits.
Lines 25-28 ignore the timeout result and continue launching a new process even when the old instance is still running. That keeps the stale-instance race alive in the exact path this helper is supposed to deflake.
Suggested fix
app.terminate()
- _ = cmuxPollUntil(timeout: 3.0) {
+ let didTerminate = cmuxPollUntil(timeout: 3.0) {
app.state == .notRunning
}
+ guard didTerminate else {
+ throw NSError(domain: "UITestLaunchSupport", code: 3, userInfo: [
+ NSLocalizedDescriptionKey: "Existing app instance did not terminate before relaunch."
+ ])
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| app.terminate() | |
| _ = cmuxPollUntil(timeout: 3.0) { | |
| app.state == .notRunning | |
| } | |
| app.terminate() | |
| let didTerminate = cmuxPollUntil(timeout: 3.0) { | |
| app.state == .notRunning | |
| } | |
| guard didTerminate else { | |
| throw NSError(domain: "UITestLaunchSupport", code: 3, userInfo: [ | |
| NSLocalizedDescriptionKey: "Existing app instance did not terminate before relaunch." | |
| ]) | |
| } |
🤖 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 `@cmuxUITests/UITestLaunchSupport.swift` around lines 25 - 28, The helper
currently calls app.terminate() and ignores the boolean result of
cmuxPollUntil(timeout: 3.0) { app.state == .notRunning }, so a stale app
instance can persist; update the helper (the block invoking app.terminate() and
cmuxPollUntil) to check the return value and fail fast when it returns
false—e.g., assert/XCTFail or throw/fatalError with a clear message if
cmuxPollUntil(...) returns false indicating the previous instance did not
exit—so the new launch is aborted instead of proceeding.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0c211cb. Configure here.
There was a problem hiding this comment.
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 `@cmuxUITests/BrowserPaneNavigationKeybindUITests.swift`:
- Around line 1462-1464: The early return in BrowserPaneNavigationKeybindUITests
that does "if app.state == .runningBackground { return }" lets XCTest-launched
runs skip activation and makes keyboard events flaky; update the guard to only
return for direct-process launches by adding a check for XCTest test-run
indicators (e.g.
ProcessInfo.processInfo.environment["XCTestConfigurationFilePath"] or
ProcessInfo.processInfo.arguments contains "XCTest") so the condition becomes:
if app.state == .runningBackground &&
ProcessInfo.processInfo.environment["XCTestConfigurationFilePath"] == nil {
return } (locate and modify this conditional in
BrowserPaneNavigationKeybindUITests.swift where app.state is checked).
In `@cmuxUITests/SidebarHelpMenuUITests.swift`:
- Around line 850-857: The current check only treats .runningForeground as
success inside sidebarHelpPollUntil, which causes spurious failures when the app
transitions to .runningBackground under direct-process launch; update the poll
closure used by sidebarHelpPollUntil so it returns true when app.state is either
.runningForeground or .runningBackground (i.e., treat both states as success),
and keep calling cmuxActivateAppIfUsingXCTestLaunch(app) only when neither state
is present; reference the existing symbols cmuxUsesDirectProcessLaunch(),
app.state, sidebarHelpPollUntil, and cmuxActivateAppIfUsingXCTestLaunch to
locate and change the closure logic accordingly.
In `@cmuxUITests/UITestLaunchSupport.swift`:
- Around line 84-99: The resolver currently only checks for the Debug product
name "cmux DEV.app" (binaryPath) and throws if not found; update the logic in
UITestLaunchSupport (around UITestLaunchSupportBundleToken and the binaryPath
resolution) to also probe the Release product name ("cmux.app" /
"Contents/MacOS/cmux") before failing, and fall back to the existing
CMUX_UI_TEST_APP_BINARY_PATH env var if present; if neither binary is
executable, keep throwing the NSError with the same descriptive message.
🪄 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: 1bc8f528-05ec-4201-aec0-1a4be711d276
📒 Files selected for processing (15)
.github/workflows/test-e2e.ymlcmux.xcodeproj/project.pbxprojcmuxUITests/BonsplitTabDragUITests.swiftcmuxUITests/BrowserPaneNavigationKeybindUITests.swiftcmuxUITests/CommandPaletteIdentifierClipboardUITests.swiftcmuxUITests/FeedSidebarUITests.swiftcmuxUITests/FindSelectionShortcutUITests.swiftcmuxUITests/HelpMenuUITests.swiftcmuxUITests/RightSidebarChromeHeightUITests.swiftcmuxUITests/SidebarHelpMenuUITests.swiftcmuxUITests/TerminalCmdClickUITests.swiftcmuxUITests/UITestLaunchSupport.swiftcmuxUITests/UpdatePillUITests.swiftcmuxUITests/WorkspaceDescriptionUITests.swiftcmuxUITests/WorkspaceSidebarScrollUITests.swift
Comment on lines
1462
to
1464
| if app.state == .runningBackground { | ||
| // App launched but couldn't activate — continue in background. | ||
| // XCUIElement queries and keyboard input work through the | ||
| // accessibility framework regardless of activation state. | ||
| return | ||
| } |
There was a problem hiding this comment.
Don’t return immediately on background state for XCTest launch mode.
Line 1462-1464 exits before attempting activation, so tests can continue while the app is backgrounded and keyboard events become unreliable. Gate this early return to direct-process launch only.
Proposed fix
private func launchAndEnsureForeground(_ app: XCUIApplication, timeout: TimeInterval = 12.0) {
cmuxLaunchApp(app)
if app.state == .runningForeground { return }
if app.state == .runningBackground {
- return
+ if cmuxUsesDirectProcessLaunch() { return }
+ cmuxActivateAppIfUsingXCTestLaunch(app)
+ if app.wait(for: .runningForeground, timeout: 6.0) { return }
}
XCTFail("App failed to start. state=\(app.state.rawValue)")
}🤖 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 `@cmuxUITests/BrowserPaneNavigationKeybindUITests.swift` around lines 1462 -
1464, The early return in BrowserPaneNavigationKeybindUITests that does "if
app.state == .runningBackground { return }" lets XCTest-launched runs skip
activation and makes keyboard events flaky; update the guard to only return for
direct-process launches by adding a check for XCTest test-run indicators (e.g.
ProcessInfo.processInfo.environment["XCTestConfigurationFilePath"] or
ProcessInfo.processInfo.arguments contains "XCTest") so the condition becomes:
if app.state == .runningBackground &&
ProcessInfo.processInfo.environment["XCTestConfigurationFilePath"] == nil {
return } (locate and modify this conditional in
BrowserPaneNavigationKeybindUITests.swift where app.state is checked).
Comment on lines
850
to
857
| if cmuxUsesDirectProcessLaunch(), app.state == .runningForeground || app.state == .runningBackground { | ||
| return | ||
| } | ||
| XCTAssertTrue( | ||
| sidebarHelpPollUntil(timeout: 4.0) { | ||
| guard app.state != .runningForeground else { return true } | ||
| app.activate() | ||
| cmuxActivateAppIfUsingXCTestLaunch(app) | ||
| return app.state == .runningForeground |
There was a problem hiding this comment.
Handle delayed state transitions in direct-process launch path.
Line 850-857 checks direct-process runnable state only once. If state becomes .runningBackground slightly later, the code falls into a foreground-only poll where activation is a no-op and can fail spuriously.
Proposed fix
private func launchAndActivate(_ app: XCUIApplication) {
cmuxLaunchApp(app)
- if cmuxUsesDirectProcessLaunch(), app.state == .runningForeground || app.state == .runningBackground {
- return
+ if cmuxUsesDirectProcessLaunch() {
+ XCTAssertTrue(
+ sidebarHelpPollUntil(timeout: 4.0) {
+ app.state == .runningForeground || app.state == .runningBackground
+ },
+ "App did not reach a runnable state before UI interactions"
+ )
+ return
}
XCTAssertTrue(
sidebarHelpPollUntil(timeout: 4.0) {
guard app.state != .runningForeground else { return true }
cmuxActivateAppIfUsingXCTestLaunch(app)
return app.state == .runningForeground
},
"App did not reach runningForeground before UI interactions"
)
}🤖 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 `@cmuxUITests/SidebarHelpMenuUITests.swift` around lines 850 - 857, The current
check only treats .runningForeground as success inside sidebarHelpPollUntil,
which causes spurious failures when the app transitions to .runningBackground
under direct-process launch; update the poll closure used by
sidebarHelpPollUntil so it returns true when app.state is either
.runningForeground or .runningBackground (i.e., treat both states as success),
and keep calling cmuxActivateAppIfUsingXCTestLaunch(app) only when neither state
is present; reference the existing symbols cmuxUsesDirectProcessLaunch(),
app.state, sidebarHelpPollUntil, and cmuxActivateAppIfUsingXCTestLaunch to
locate and change the closure logic accordingly.
Comment on lines
+84
to
+99
| let testBundle = Bundle(for: UITestLaunchSupportBundleToken.self) | ||
| let productsDir = testBundle.bundleURL | ||
| .deletingLastPathComponent() | ||
| .deletingLastPathComponent() | ||
| .deletingLastPathComponent() | ||
| .deletingLastPathComponent() | ||
| let binaryPath = productsDir | ||
| .appendingPathComponent("cmux DEV.app") | ||
| .appendingPathComponent("Contents/MacOS/cmux DEV") | ||
| .path | ||
| if FileManager.default.isExecutableFile(atPath: binaryPath) { | ||
| return binaryPath | ||
| } | ||
|
|
||
| throw NSError(domain: "UITestLaunchSupport", code: 2, userInfo: [ | ||
| NSLocalizedDescriptionKey: "App binary not found at \(binaryPath). testBundle=\(testBundle.bundleURL.path)" |
There was a problem hiding this comment.
Handle both Debug and Release app product names.
This resolver only probes cmux DEV.app, but cmux.xcodeproj/project.pbxproj builds the app as cmux DEV in Debug and cmux in Release. Direct launch will therefore fail under the cmuxUITests Release config unless CMUX_UI_TEST_APP_BINARY_PATH is set manually.
Suggested fix
- let binaryPath = productsDir
- .appendingPathComponent("cmux DEV.app")
- .appendingPathComponent("Contents/MacOS/cmux DEV")
- .path
- if FileManager.default.isExecutableFile(atPath: binaryPath) {
- return binaryPath
+ let candidates = [
+ ("cmux DEV.app", "cmux DEV"),
+ ("cmux.app", "cmux"),
+ ]
+ for (appName, executableName) in candidates {
+ let binaryPath = productsDir
+ .appendingPathComponent(appName)
+ .appendingPathComponent("Contents/MacOS/\(executableName)")
+ .path
+ if FileManager.default.isExecutableFile(atPath: binaryPath) {
+ return binaryPath
+ }
}
throw NSError(domain: "UITestLaunchSupport", code: 2, userInfo: [🤖 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 `@cmuxUITests/UITestLaunchSupport.swift` around lines 84 - 99, The resolver
currently only checks for the Debug product name "cmux DEV.app" (binaryPath) and
throws if not found; update the logic in UITestLaunchSupport (around
UITestLaunchSupportBundleToken and the binaryPath resolution) to also probe the
Release product name ("cmux.app" / "Contents/MacOS/cmux") before failing, and
fall back to the existing CMUX_UI_TEST_APP_BINARY_PATH env var if present; if
neither binary is executable, keep throwing the NSError with the same
descriptive message.
lawrencecchen
force-pushed
the
feat-ui-test-activation-deflake
branch
from
May 28, 2026 07:41
0c211cb to
6893b1e
Compare
lawrencecchen
force-pushed
the
feat-ui-test-activation-deflake
branch
from
May 28, 2026 08:14
6893b1e to
62b7da7
Compare
lawrencecchen
changed the title
lawrencecchen
force-pushed
the
feat-ui-test-activation-deflake
branch
from
May 28, 2026 08:43
62b7da7 to
0eace82
Compare
lawrencecchen
changed the title
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
This was referenced May 28, 2026
hhsw2015
pushed a commit
to hhsw2015/cmux
that referenced
this pull request
May 28, 2026
- a791621 Reduce browser WebView input latency (manaflow-ai#4863) - f2dbc31 Fix file preview Open With menu (manaflow-ai#4932) - 95d4c2f Wait for E2E virtual display readiness (manaflow-ai#4928) - 86558d1 Add cmux diff CodeView command (manaflow-ai#4451) - 6dcddde Run E2E xcodebuild in GUI bootstrap (manaflow-ai#4940) Conflicts resolved: - CLINotifyProcessIntegrationRegressionTests.swift: kept both fork and upstream test additions. - Workspace.swift: kept fork's per-layoutTab snapshot logic; rawLayout aliases the selected tab's layout to satisfy upstream's downstream uses.
This was referenced May 28, 2026
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.
Replaces the fixed virtual-display sleep in hosted E2E with the helper readiness and display-id markers. The workflow now fails with the helper log if readiness never arrives or the helper exits early.
I tried removing several non-strict UI-test launch expected-failure masks, but the current runner still fails at
app.launch()for CommandPalette, UpdatePill, and FeedSidebar. Those masks stay in place. This PR keeps the deterministic CI timing fix only.Validation:
git diff --checkactionlint -oneline .github/workflows/test-e2e.yml./tests/test_ci_self_hosted_guard.shNote
Low Risk
CI-only workflow timing change; no app runtime, auth, or data-path impact.
Overview
Hosted E2E no longer assumes a 3 second delay after starting
create-virtual-display. The workflow passes--ready-pathand--display-id-path, polls until both marker files exist (or the helper exits), and fails the job with the helper log on timeout or early exit.That ties test startup to the helper’s real “display created” signal instead of a fixed sleep, which should reduce flaky races when the virtual display is slow or fast to come up.
Reviewed by Cursor Bugbot for commit 0eace82. Bugbot is set up for automated code reviews on this repo. Configure here.