Repository navigation
Fix background new-workspace command startup - #4115
austinywang wants to merge 11 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR removes the explicit observer deregistration call from the workspace panel discard path and expands CLI integration tests to validate queued command execution under both ChangesQueued Input Recovery for Unfocused Surfaces
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (14 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 |
Greptile SummaryFixes background
Confidence Score: 5/5Safe to merge; the change removes dead observer bookkeeping and delegates to an already-tested durable input queue with no new timing dependencies. The three call sites that previously used sendInputWhenReady now call panel.sendInput directly. GhosttyTerminalView.sendInputResult already handles the nil-surface case by enqueuing text and calling requestBackgroundSurfaceStartIfNeeded, so no delivery guarantee is lost. The hasBackgroundSurfaceStartWork simplification is correct because pendingSocketInputBytes > 0 captures exactly the panels that have queued work. All deleted state (observer registrations, timeouts, panel-close cleanup) is unreachable after the routing change. The regression test covers both --command and --layout without focus change, which directly validates the bug fix. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as cmux CLI
participant WS as Workspace
participant TP as TerminalPanel
participant TS as TerminalSurface (GhosttyTerminalView)
participant Q as pendingSocketInputQueue
CLI->>WS: new-workspace --command / --layout
WS->>TP: newTerminalSurface(focus: false)
TP-->>WS: "panel (surface == nil)"
WS->>TP: panel.sendInput(command + newline)
TP->>TS: surface.sendInput(text)
TS->>TS: "surface == nil, allowsRuntimeSurfaceCreation() == true"
TS->>Q: enqueuePendingSocketInput(text)
TS->>TS: requestBackgroundSurfaceStartIfNeeded()
Note over TS: DispatchQueue.main.async: attachedView present but no window, scheduleHeadlessRuntimeStartIfNeeded
TS->>TS: createSurface(headless window)
TS->>Q: flush pendingSocketInputQueue to PTY
Note over Q: Command executes in background terminal
Reviews (12): Last reviewed commit: "merge: sync main into issue-4090-new-wor..." | Re-trigger Greptile |
![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: 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_command_queue.py`:
- Around line 172-181: Replace the bare try/except blocks used around
c.close_workspace calls with contextlib.suppress(Exception) to make the "ignore
errors during cleanup" intent explicit: import suppress from contextlib, then
wrap the conditional close calls for created_layout_ws_id and created_ws_id each
in a with suppress(Exception): block calling c.close_workspace(id) so that
cleanup remains best-effort but is clearer; target the existing
created_layout_ws_id and created_ws_id variables and the c.close_workspace(...)
calls.
- Around line 89-93: Replace the try/except blocks around the cleanup loop that
calls path.unlink(missing_ok=True) (iterating over marker, layout_left_marker,
layout_right_marker) with contextlib.suppress(OSError) to make the intent
explicit and the code more concise; import suppress from contextlib and wrap the
unlink call in a with suppress(OSError): block in both occurrences (the loop
around marker/layout_left_marker/layout_right_marker and the similar block at
the second location) so only OSError is suppressed while keeping other behavior
unchanged.
🪄 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: e77cdbe6-8ba5-41cb-a41e-115b5210071b
📒 Files selected for processing (3)
Sources/GhosttyTerminalView.swiftSources/Workspace.swifttests_v2/test_cli_new_workspace_command_queue.py
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
44dd7fd to
5ec29db
Compare
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 `@tests_v2/test_cli_new_workspace_command_queue.py`:
- Around line 73-74: The polling in _wait_for_marker currently breaks on any
non-empty observed content which can catch a file mid-write; change the loop to
continue polling until observed == token (or timeout) instead of breaking on
truthy observed, i.e., only exit when the observed string exactly equals the
expected token (preserving the existing timeout behavior and any sleep/retry
logic).
🪄 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: 1b45f147-a7d1-4aef-a3a8-768f00de6dc1
📒 Files selected for processing (4)
Sources/GhosttyTerminalView.swiftSources/Workspace+PanelLifecycle.swiftSources/Workspace.swifttests_v2/test_cli_new_workspace_command_queue.py
💤 Files with no reviewable changes (1)
- Sources/Workspace+PanelLifecycle.swift
Stale CodeRabbit review on an older commit. The marker polling issue was fixed in 7f0a094 and acknowledged on the review thread; latest CodeRabbit check is passing.
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 `@tests_v2/test_cli_new_workspace_command_queue.py`:
- Line 181: Replace the hardcoded timeout check in
tests_v2/test_cli_new_workspace_command_queue.py that uses _must(elapsed < 1.5,
...) with a configurable threshold obtained via the existing _float_env helper
(e.g. timeout = _float_env("NEW_WORKSPACE_LAYOUT_TIMEOUT", 1.5)); then use
_must(elapsed < timeout, f"new-workspace --layout should return quickly, took
{elapsed:.2f}s (threshold {timeout}s)"). Update the assertion site where _must
is called and ensure the env var name matches naming convention used for other
test timeouts (it should be referenced where --command timeout is already
handled).
🪄 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: 26358688-5784-4ef6-a70f-9498644591ed
📒 Files selected for processing (1)
tests_v2/test_cli_new_workspace_command_queue.py
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 014dc07. Configure here.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Superseded by the current CodeRabbit approval on the latest commit. The layout timeout feedback was addressed by replacing the hardcoded threshold with NEW_WORKSPACE_LAYOUT_TIMEOUT in the current code.
…ace-command-dropped
…ace-command-dropped
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
3 participants
Summary
cmux new-workspacewith--commandor--layoutsilently drops the command when invoked unfocused #4090.view.window != nil.TerminalSurface.sendInput, so they are retained on the surface pending-input queue instead of being dropped after a 3 second readiness timeout.tests_v2/test_cli_new_workspace_command_queue.pyto cover bothcmux new-workspace --commandandcmux new-workspace --layoutper-pane commands without selecting the created workspace.Repro Steps Run
From this cmux workspace terminal, using the currently running app's bundled CLI:
I closed the temporary
_repro_4090*workspaces after the repro.Observed Baseline Behavior
On current
mainat278b31209, the unfocused--commandworkspace createdworkspace:53/surface:105, but the surface stayed titledTerminal, hadtty=<nil>, andread-screenreturned:The same error persisted after 3 seconds. The layout repro created
workspace:54withsurface:106andsurface:107; both surfaces hadtty=<nil>and bothread-screencalls returnedTerminal surface not found.I also ran the
--focus truecontrol from the issue. In this already-running 0.64.5 app/session it also produced a nil-tty surface, so I did not use that as the regression assertion. The unfocused command/layout failure reproduced deterministically and matches #4090.Observed After Fix
Pending CI verification on this PR. The expected observable behavior after this fix is that the marker-file commands in
tests_v2/test_cli_new_workspace_command_queue.pyare created from both--commandand--layoutwithout changing the selected workspace.Regression Test
Failing-then-passing regression test:
tests_v2/test_cli_new_workspace_command_queue.py.Before the fix commit, I ran it against the baseline running app with:
It failed waiting for the
--commandmarker file:Relationship To #3798 And #3876
#3798 and #4090 share the same lifecycle root cause: background terminal work is requested while the SwiftUI/AppKit view is attached but not in an
NSWindow, andrequestBackgroundSurfaceStartIfNeeded()used to return before creating the Ghostty runtime surface/PTY.PR #3876 is still open and fixes the #3798
surface.send_textqueue by removing that background-startview.window != nilgate. This PR applies the same background-start principle for the new-workspace command case and adds the missing #4090-specific piece: layout per-panecommandfields were not using the durable surface input queue and could be silently dropped byWorkspace.sendInputWhenReadyafter 3 seconds. This branch removes that timeout path and queues those inputs on theTerminalSurfaceinstead.Note
Medium Risk
Changes terminal startup-command delivery by removing the Workspace-level readiness observer/timeout mechanism and sending commands directly to
TerminalPanel.sendInput, which could affect when/if commands execute during surface initialization. Coverage is improved with expanded CLI regression tests, but the behavior touches workspace/terminal lifecycle paths.Overview
Fixes background
new-workspacestartup commands by removing Workspace’s pending-terminal-input observer/3s timeout path and routing both--commandand per-pane--layoutcommandfields throughTerminalPanel.sendInput(so input is queued on the surface rather than dropped when the runtime surface isn’t ready).Cleans up associated lifecycle bookkeeping (pending observer maps, deinit/prune/close cleanup hooks, and background-start “work pending” checks) and extends
tests_v2/test_cli_new_workspace_command_queue.pyto assert--commandand--layoutcommands execute without switching focus and that the CLI returns quickly.Reviewed by Cursor Bugbot for commit 0de8b11. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes #4090 by making background
cmux new-workspace --commandand per-pane--layoutcommands run reliably without focusing the new workspace or dropping input.panel.sendInputso they queue on the terminal surface; remove readiness observers, the 3s drop timeout, all pending-input bookkeeping, and the stale observer gate in background-start checks.tests_v2/test_cli_new_workspace_command_queue.pyto cover--commandand two-pane--layout; assert quick CLI return (NEW_WORKSPACE_LAYOUT_TIMEOUT), exact per-pane tokens and OK response, and focus preservation.Written for commit 0de8b11. Summary will update on new commits. Review in cubic
Summary by CodeRabbit