Fix background new-workspace commands - #4137
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. |
📝 WalkthroughWalkthroughPort ordinal is now immutable and set at construction; TerminalSurface registers with the registry before hosted attachment; background/off-window surface start is exposed via ChangesUnfocused terminal command execution
Sequence Diagram(s)sequenceDiagram
participant Workspace
participant TerminalPanel.surface as Panel.surface
participant TerminalSurface
participant TerminalSurfaceRegistry
Workspace->>Panel.surface: requestBackgroundSurfaceStartIfNeeded(allowOffWindow: true)
Panel.surface->>TerminalSurface: requestBackgroundSurfaceStartIfNeeded(allowOffWindow: true)
TerminalSurface->>TerminalSurfaceRegistry: register(surface)
TerminalSurface->>TerminalSurface: hostedView.attachSurface(self) (visual attach)
TerminalSurface->>TerminalSurface: create runtime PTY (allowed off-window when allowOffWindow==true)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Possibly related issues
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 2 warnings)
✅ Passed checks (12 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@tests_v2/test_cli_new_workspace_layout_command_queue.py`:
- Line 98: The test currently asserts a hard latency bound using _must(elapsed <
1.5, ...) which is CI-flaky; change it to use a configurable threshold (e.g.,
read float from an env var like NEW_WORKSPACE_LAYOUT_TIMEOUT with a sensible
default such as 3.0) and assert _must(elapsed < timeout, f"... took
{elapsed:.2f}s") so the threshold can be increased in noisy environments; update
the call site in tests_v2/test_cli_new_workspace_layout_command_queue.py around
the elapsed check and ensure the env var parsing uses a safe float fallback.
- Around line 53-61: The subprocess.run call in
test_cli_new_workspace_layout_command_queue.py should be bounded to avoid hung
tests: add a timeout parameter (e.g., timeout=30) to the subprocess.run
invocation and wrap the call in a try/except subprocess.TimeoutExpired block; on
timeout raise cmuxError with a clear message that includes the command, the
timeout value and the TimeoutExpired details (e.output / e.stderr or str(e)) so
CI fails fast and reports useful diagnostics for the failing call to
subprocess.run.
🪄 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: c2324fa8-23a9-4d14-bb45-2e26862adcd7
📒 Files selected for processing (4)
Sources/GhosttyTerminalView.swiftSources/Panels/TerminalPanel.swiftSources/Workspace.swifttests_v2/test_cli_new_workspace_layout_command_queue.py
Contributor
Greptile SummaryFixes the background
Confidence Score: 5/5Safe to merge. The changes are well-scoped: portOrdinal is now immutable, the allowOffWindow accumulator is cleanly self-contained within TerminalSurface, and the Workspace call sites explicitly opt in to off-window startup only for socket/API-triggered and background-prime paths. All three Swift files make targeted, locally-bounded changes with clear ownership. The portOrdinal fix eliminates a real initialization-ordering gap. The queuedBackgroundSurfaceStartAllowsOffWindow flag is private, reset before every createSurface call, and serialized on the main queue — no new shared mutable state escapes TerminalSurface. Sources/GhosttyTerminalView.swift — the off-window startup path still falls back to NSScreen.main backing scale when the view has no window; scale is only partially reconciled on window presentation (display ID update only, no scale push). Worth revisiting if multi-display rendering artifacts appear. Important Files Changed
Sequence DiagramsequenceDiagram
participant API as Socket/API Caller
participant WS as Workspace
participant TP as TerminalPanel
participant TS as TerminalSurface
participant MQ as Main Queue
API->>WS: addPendingTerminalInputObserver(panelId, text)
WS->>WS: append registration to pendingTerminalInputObserversByPanelId
WS->>TS: requestBackgroundSurfaceStartIfNeeded(allowOffWindow: true)
TS->>TS: "queuedBackgroundSurfaceStartAllowsOffWindow |= true"
TS->>TS: "backgroundSurfaceStartQueued = true"
TS->>MQ: async createSurface for attachedView
note over TS,MQ: surface starts even with no window
MQ-->>TS: fire reads and clears queuedBackgroundSurfaceStartAllowsOffWindow
TS->>TS: createSurface for view off-window allowed
TS-->>WS: surface ready callback fires
WS->>TP: sendInput text pending observer fires
Reviews (5): Last reviewed commit: "Preserve explicit background surface sta..." | Re-trigger Greptile |
…ace-command-unfocused
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/GhosttyTerminalView.swift (1)
5690-5724:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the strongest queued off-window start request.
Once
backgroundSurfaceStartQueuedis set, later calls are ignored, so a queuedallowOffWindow == falserequest can suppress a subsequent explicitallowOffWindow == truerequest. The async block then still hits the Line 5708 no-window guard, leaving background CLI/socket startup deferred until some later retry. This regresses the new explicit off-window path whenever the weaker request wins the race.💡 Proposed fix
private var backgroundSurfaceStartQueued = false + private var queuedBackgroundSurfaceStartAllowOffWindow = false @@ func requestBackgroundSurfaceStartIfNeeded(allowOffWindow: Bool = false) { if !Thread.isMainThread { DispatchQueue.main.async { [weak self] in self?.requestBackgroundSurfaceStartIfNeeded(allowOffWindow: allowOffWindow) } return } guard allowsRuntimeSurfaceCreation() else { return } guard surface == nil, attachedView != nil else { return } - guard !backgroundSurfaceStartQueued else { return } + queuedBackgroundSurfaceStartAllowOffWindow = + queuedBackgroundSurfaceStartAllowOffWindow || allowOffWindow + guard !backgroundSurfaceStartQueued else { return } backgroundSurfaceStartQueued = true DispatchQueue.main.async { [weak self] in guard let self else { return } + let allowOffWindow = self.queuedBackgroundSurfaceStartAllowOffWindow + self.queuedBackgroundSurfaceStartAllowOffWindow = false self.backgroundSurfaceStartQueued = false guard self.allowsRuntimeSurfaceCreation() else { return } guard self.surface == nil, let view = self.attachedView else { return } guard allowOffWindow || view.window != nil else {🤖 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/GhosttyTerminalView.swift` around lines 5690 - 5724, The backgroundSurfaceStartQueued boolean currently suppresses later stronger requests (allowOffWindow == true); change the logic to preserve the strongest queued request by introducing a companion stored flag (e.g. queuedAllowOffWindow: Bool) or an enum, set queuedAllowOffWindow = queuedAllowOffWindow || allowOffWindow when you mark backgroundSurfaceStartQueued = true inside requestBackgroundSurfaceStartIfNeeded, and in the DispatchQueue.main.async closure read and reset queuedAllowOffWindow (and backgroundSurfaceStartQueued) and use that value instead of the original allowOffWindow parameter when checking the no-window guard and calling createSurface(for:). Ensure all references to allowOffWindow inside the async block use the preserved queuedAllowOffWindow and that both flags are cleared appropriately.
🤖 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/GhosttyTerminalView.swift`:
- Around line 5690-5724: The backgroundSurfaceStartQueued boolean currently
suppresses later stronger requests (allowOffWindow == true); change the logic to
preserve the strongest queued request by introducing a companion stored flag
(e.g. queuedAllowOffWindow: Bool) or an enum, set queuedAllowOffWindow =
queuedAllowOffWindow || allowOffWindow when you mark
backgroundSurfaceStartQueued = true inside
requestBackgroundSurfaceStartIfNeeded, and in the DispatchQueue.main.async
closure read and reset queuedAllowOffWindow (and backgroundSurfaceStartQueued)
and use that value instead of the original allowOffWindow parameter when
checking the no-window guard and calling createSurface(for:). Ensure all
references to allowOffWindow inside the async block use the preserved
queuedAllowOffWindow and that both flags are cleared appropriately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6ac417cb-ad19-407e-843c-d375c6829ca5
📒 Files selected for processing (2)
Sources/GhosttyTerminalView.swiftSources/Workspace.swift
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6d72f9b. Configure here.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
This was referenced May 16, 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.
Closes #4090
Summary:
cmux new-workspace --layoutcommands do not expire in the background.Red failure:
CMUXTERM_CLI="$(cat /tmp/cmux-last-cli-path)" CMUX_SOCKET_PATH=/tmp/cmux-debug-i4090.sock python3 tests_v2/test_cli_new_workspace_layout_command_queue.pyfailed before the fix:Layout command marker file was not created.test_cli_new_workspace_command_queue.pydid not create its command marker file.Green verification:
./scripts/reload.sh --tag i4090f --launchCMUXTERM_CLI="$(cat /tmp/cmux-last-cli-path)" CMUX_SOCKET_PATH=/tmp/cmux-debug-i4090f.sock python3 tests_v2/test_cli_new_workspace_command_queue.pypassed.CMUXTERM_CLI="$(cat /tmp/cmux-last-cli-path)" CMUX_SOCKET_PATH=/tmp/cmux-debug-i4090f.sock python3 tests_v2/test_cli_new_workspace_layout_command_queue.pypassed.CMUX_SOCKET_PATH=/tmp/cmux-debug-i4090f.sock python3 tests_v2/test_workspace_create_background_starts_terminal.pypassed.git diff --checkpassed.Cloud proof:
cloud-mac-25835173638.cua-ssh setup --with-permissions --no-verify; I stopped the stuck setup process. No cloud video was produced.Note
Medium Risk
Changes terminal surface startup conditions to allow off-window runtime creation for socket/API-triggered commands, which could affect PTY lifecycle and resource usage if mis-triggered. Mitigated by keeping the normal UI-driven attach path window-gated and adding regression tests for
new-workspacecommand/layout queuing.Overview
Fixes background
new-workspacecommand execution by letting explicit socket/API requests start aTerminalSurfaceruntime from an attached view even when it is not yet in a window (viarequestBackgroundSurfaceStartIfNeeded(allowOffWindow:)), while keeping the normal visual attach path window-gated.Makes per-workspace port range selection deterministic by capturing
portOrdinalatTerminalSurfaceconstruction (and wiring it throughTerminalPanel). Updates workspace pending-input wiring to trigger background surface starts when observers are installed, and adds/extends Python regression tests with configurable CLI timeouts plus a new--layoutcommand-queue test.Reviewed by Cursor Bugbot for commit 735510d. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Bug Fixes
Behavior
Tests