Repository navigation
Fix offscreen terminal helper PTY startup - #4233
Merged
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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:
📝 WalkthroughWalkthroughAdd headless (off-window) terminal startup, change pending socket input to ordered variants with strict byte-capacity rejection, update send APIs to return structured results and queue input when runtime is absent, centralize socket error messages, and add tests and debug inspection helpers. ChangesTerminal Background Startup and Socket Input Lifecycle
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (4 errors, 1 warning)
✅ Passed checks (11 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
Greptile SummaryThis PR fixes background/offscreen terminal helpers failing to spawn a PTY by bootstrapping Ghostty runtime creation through a hidden headless
Confidence Score: 4/5Safe to merge; the one open gap is only reachable during initial PTY startup when a process exit at that exact moment is exceedingly unlikely in practice. The core headless-bootstrap and typed-result changes are well-covered by nine new unit tests. Actor-isolation corrections flagged in earlier rounds are applied throughout. The only non-trivial asymmetry is that flushPendingSocketInputIfNeeded does not call ghostty_surface_process_exited, while every live-send path does — a real consistency gap but not an immediately actionable defect. Sources/GhosttyTerminalView.swift — specifically the flushPendingSocketInputIfNeeded path, which should mirror the process-exit guard present on all live-send paths. Important Files Changed
Reviews (14): Last reviewed commit: "fix: ignore headless window in force ref..." | 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/GhosttyTerminalView.swift (2)
4627-4721: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftExtract the headless-startup and socket-input queue logic into dedicated types.
This PR adds another large block of AppKit bootstrap/window management and socket-input lifecycle code to a production Swift file that is already far past the repo budget. Keeping this here also deepens the mix of rendering, state ownership, platform bridge, and socket protocol logic in one place.
As per coding guidelines "Do not add more than 250 lines to an existing production Swift file that is already over 800 lines" and "Do not mix UI rendering, state ownership, persistence, networking, parsing, subprocess/socket protocol, and platform bridge code in one Swift file."
Also applies to: 5727-6156
🤖 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 4627 - 4721, The headless-startup and socket-input queue logic should be moved out of GhosttyTerminalView into dedicated helper types: create a HeadlessWindowManager type encapsulating headlessStartupWindow, ensureHeadlessStartupWindowIfNeeded(reason:), releaseHeadlessStartupWindowIfNeeded(for:), and startRuntimeUsingHeadlessWindowIfNeeded(reason:) (use GhosttyTerminalView.hostedView, surfaceView, id, and allowsRuntimeSurfaceCreation() only via well-defined APIs), and create a SocketInputQueue type for the socket/input lifecycle code referenced around lines 5727-6156; update GhosttyTerminalView methods (updateWorkspaceId, reconcileAttachedWindowIfNeeded, and any callers of the moved methods) to delegate to these new types, preserve all existing behaviors (main-thread dispatch, window creation flags, debug logs using id.uuidString.prefix(8), and calling hostedView.attachSurface(self)), and add unit-testable interfaces so UI code no longer mixes socket or lifecycle/state ownership logic in this file.
5648-5659:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
forceRefreshon the live-surface path.
beginPortalCloseLifecycleleavessurfacenon-nil while the lifecycle is.closing. With this guard,forceRefreshcan still reachghostty_surface_set_display_id/ghostty_surface_refreshduring teardown instead of bailing through the existing live-surface quarantine path.Suggested fix
- guard let view = attachedView, - surface != nil, + guard let view = attachedView, view.window != nil, view.bounds.width > 0, view.bounds.height > 0 else { return } `#if` DEBUG recordDebugForceRefresh() `#endif` - guard let currentSurface = self.surface else { return } + guard let currentSurface = liveSurfaceForGhosttyAccess( + reason: "forceRefresh.\(reason)" + ) else { 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 `@Sources/GhosttyTerminalView.swift` around lines 5648 - 5659, The guard that returns early when view/window/bounds are invalid prevents forceRefresh from reaching the live surface during beginPortalCloseLifecycle; update the guard logic in the live-surface path so that when self.surface is non-nil and the lifecycle is .closing (as set by beginPortalCloseLifecycle) you do not bail out if a pending forceRefresh should run—allow the code path that calls ghostty_surface_set_display_id / ghostty_surface_refresh to proceed for forceRefresh. Locate the guard around attachedView/surface/view.window/view.bounds and modify it to short-circuit only when there is no surface or when lifecycle is not .closing (or when forceRefresh is false), ensuring forceRefresh still executes against the live surface during teardown.
🤖 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 `@Sources/TerminalController.swift`:
- Around line 15602-15619: The legacy send-key paths
(terminalPanel.surface.sendNamedKey(...) handling and the similar branch later)
need to trigger a UI refresh when the surface accepted the key; update the code
paths that currently set success = true for the .sent case to also call the same
refresh used by v2SurfaceSendKey (invoke forceRefresh on the relevant
TerminalSurface/terminalPanel.surface) immediately after a .sent result so a
subsequent read-screen sees the updated frame; apply the same change in both the
sendNamedKey handling blocks (the earlier block around
terminalPanel.surface.sendNamedKey and the later similar block) and ensure you
only call forceRefresh when the surface is live/accepted (i.e., in the .sent
branch).
---
Outside diff comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 4627-4721: The headless-startup and socket-input queue logic
should be moved out of GhosttyTerminalView into dedicated helper types: create a
HeadlessWindowManager type encapsulating headlessStartupWindow,
ensureHeadlessStartupWindowIfNeeded(reason:),
releaseHeadlessStartupWindowIfNeeded(for:), and
startRuntimeUsingHeadlessWindowIfNeeded(reason:) (use
GhosttyTerminalView.hostedView, surfaceView, id, and
allowsRuntimeSurfaceCreation() only via well-defined APIs), and create a
SocketInputQueue type for the socket/input lifecycle code referenced around
lines 5727-6156; update GhosttyTerminalView methods (updateWorkspaceId,
reconcileAttachedWindowIfNeeded, and any callers of the moved methods) to
delegate to these new types, preserve all existing behaviors (main-thread
dispatch, window creation flags, debug logs using id.uuidString.prefix(8), and
calling hostedView.attachSurface(self)), and add unit-testable interfaces so UI
code no longer mixes socket or lifecycle/state ownership logic in this file.
- Around line 5648-5659: The guard that returns early when view/window/bounds
are invalid prevents forceRefresh from reaching the live surface during
beginPortalCloseLifecycle; update the guard logic in the live-surface path so
that when self.surface is non-nil and the lifecycle is .closing (as set by
beginPortalCloseLifecycle) you do not bail out if a pending forceRefresh should
run—allow the code path that calls ghostty_surface_set_display_id /
ghostty_surface_refresh to proceed for forceRefresh. Locate the guard around
attachedView/surface/view.window/view.bounds and modify it to short-circuit only
when there is no surface or when lifecycle is not .closing (or when forceRefresh
is false), ensuring forceRefresh still executes against the live surface during
teardown.
🪄 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: 7e56f187-d512-4335-83e7-090816fb7e11
📒 Files selected for processing (5)
Resources/Localizable.xcstringsSources/GhosttyTerminalView.swiftSources/TerminalController.swiftcmuxTests/TerminalAndGhosttyTests.swifttests_v2/test_cli_background_terminal_helpers_start_pty.py
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/TerminalController.swift">
<violation number="1" location="Sources/TerminalController.swift:15607">
P2: Force a refresh after successful live key sends in the legacy socket key paths; otherwise an immediate `read-screen` can return stale terminal content even though the command returned OK.</violation>
</file>
<file name="Sources/GhosttyTerminalView.swift">
<violation number="1" location="Sources/GhosttyTerminalView.swift:6260">
P1: Move headless window teardown onto the main thread; `NSWindow` mutations in `deinit` are not thread-safe when the final release happens off-main.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Re-trigger cubic
lawrencecchen
force-pushed
the
issue-4228-terminal-helper-surfaces-no-pty
branch
2 times, most recently
from
May 16, 2026 01:39
f48f3b3 to
a17258d
Compare
lawrencecchen
force-pushed
the
issue-4228-terminal-helper-surfaces-no-pty
branch
from
May 16, 2026 01:42
a17258d to
d13a450
Compare
lawrencecchen
force-pushed
the
issue-4228-terminal-helper-surfaces-no-pty
branch
from
May 16, 2026 01:53
d13a450 to
37db1dd
Compare
lawrencecchen
force-pushed
the
issue-4228-terminal-helper-surfaces-no-pty
branch
from
May 16, 2026 02:25
37db1dd to
1faa11e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/GhosttyTerminalView.swift (2)
5635-5692: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winKeep
forceRefresh()allocation-free on the hot path.This now builds
viewStatestrings in all builds, and the interpolated"forceRefresh.\(reason)"/"forceRefresh.refresh.\(reason)"tokens also allocate on every refresh. That adds avoidable work to a method that already runs on keystrokes.💡 Suggested tightening
func forceRefresh(reason: String = "unspecified") { if !Thread.isMainThread { DispatchQueue.main.async { [weak self] in self?.forceRefresh(reason: reason) } return } - let hasSurface = surface != nil - let viewState: String - if let view = attachedView { - let inWindow = view.window != nil - let bounds = view.bounds - let metalOK = (view.layer as? CAMetalLayer) != nil - viewState = "inWindow=\(inWindow) bounds=\(bounds) metalOK=\(metalOK) hasSurface=\(hasSurface)" - } else { - viewState = "NO_ATTACHED_VIEW hasSurface=\(hasSurface)" - } `#if` DEBUG + let hasSurface = surface != nil + let viewState: String + if let view = attachedView { + let inWindow = view.window != nil + let bounds = view.bounds + let metalOK = (view.layer as? CAMetalLayer) != nil + viewState = "inWindow=\(inWindow) bounds=\(bounds) metalOK=\(metalOK) hasSurface=\(hasSurface)" + } else { + viewState = "NO_ATTACHED_VIEW hasSurface=\(hasSurface)" + } cmuxDebugLog("forceRefresh: \(id) reason=\(reason) \(viewState)") `#endif` @@ let displayID = (view.window?.screen ?? NSScreen.main)?.displayID let hasLiveSurface = MainActor.assumeIsolated { - guard let currentSurface = liveSurfaceForGhosttyAccess(reason: "forceRefresh.\(reason)") else { + guard let currentSurface = liveSurfaceForGhosttyAccess(reason: "forceRefresh") else { return false } @@ view.forceRefreshSurface() MainActor.assumeIsolated { - guard let surface = liveSurfaceForGhosttyAccess(reason: "forceRefresh.refresh.\(reason)") else { + guard let surface = liveSurfaceForGhosttyAccess(reason: "forceRefresh.refresh") else { return } ghostty_surface_refresh(surface) } }As per coding guidelines:
In TerminalSurface.forceRefresh() in GhosttyTerminalView.swift, do not add allocations, file I/O, or formatting as it's called on every keystroke.🤖 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 5635 - 5692, The method forceRefresh builds strings every call (viewState and the interpolated reasons passed to liveSurfaceForGhosttyAccess), causing allocations on the hot path; fix by moving construction of viewState and any reason interpolation inside `#if` DEBUG so they are only created in debug builds, and in release use constant literals (e.g. "forceRefresh" and "forceRefresh.refresh") when calling liveSurfaceForGhosttyAccess/ghostty_surface_refresh; update the code around viewState, the displayID block that calls liveSurfaceForGhosttyAccess(reason: ...), and the subsequent MainActor.assumeIsolated call to use the debug-only interpolated strings and release-only constants to avoid per-keystroke allocations.
4388-6291: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftExtract the startup/queueing lifecycle out of this file.
This PR adds another large block of runtime bootstrap, socket-queue, and lifecycle code to a production file that is already far beyond the repo’s size budget. It also pushes more subprocess/socket protocol behavior into the same file as AppKit rendering and portal-host code, which makes future changes harder to reason about safely.
As per coding guidelines:
Do not add more than 250 lines to an existing production Swift file that is already over 800 linesandDo not mix UI rendering, state ownership, persistence, networking, parsing, subprocess/socket protocol, and platform bridge code in one Swift file.🤖 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 4388 - 6291, The file mixes UI/portal code with a large startup/socket-queue lifecycle; extract the non-UI runtime/bootstrap and socket queue logic into a new helper (e.g., TerminalRuntimeLifecycle or TerminalSocketQueue) and keep TerminalSurface as the UI/portal owner that delegates lifecycle and queue operations. Move types and functions like PendingKeyEvent, PendingSocketInput, ParsedSocketInput, parsedSocketInputEvents(for:), enqueuePendingSocketInputs(_:), enqueuePendingSocketInput(_:), flushPendingSocketInputIfNeeded(), requestBackgroundSurfaceStartIfNeeded(), startRuntimeUsingHeadlessWindowIfNeeded(reason:), ensureHeadlessStartupWindowIfNeeded(reason:), createSurface(for:), writeTextData(_:to:), sendInput(_:to:), sendText(_:), sendInputResult(_:), sendNamedKey(_:), liveSurfaceForSocketWrite(reason:), and any supporting constants/locks into the new file/class; expose a minimal MainActor-isolated API (enqueue, flush, requestStart, createIfNeeded, liveSurface accessor) that TerminalSurface calls, preserve existing behaviors/annotations (MainActor, debug logs, TerminalSurfaceRegistry interactions) and update all internal calls in TerminalSurface to delegate to the new helper while keeping UI-specific pieces (attachedView, hostedView, view lifecycle, display id, forceRefresh, focus, portal lease/state) in TerminalSurface. Ensure tests and debug-only helpers are updated to use the new helper and keep runtime surface ownership and callback context management consistent when moving createSurface-related code.
🤖 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 `@Sources/GhosttyTerminalView.swift`:
- Around line 5744-5804: The current cold-path enqueues input even after the
surface lifecycle has closed (so requestBackgroundSurfaceStartIfNeeded() is a
no-op), leaving input stranded; update sendText(_:), sendInputResult(_:), and
sendNamedKey(_:) to first check the surface lifecycle/startability before
enqueuing: call whatever predicate is used by
requestBackgroundSurfaceStartIfNeeded (or add a helper like
canStartBackgroundSurface / isSurfaceLifecycleActive) and if it indicates the
surface cannot be started, immediately reject the input (return false for
sendText, .inputQueueFull for sendInputResult, and .inputQueueFull or
.surfaceUnavailable for sendNamedKey as appropriate) instead of
enqueuePendingSocketInput; keep existing behavior when the predicate allows
background start. Ensure references to enqueuePendingSocketInput,
requestBackgroundSurfaceStartIfNeeded, sendText, sendInputResult, and
sendNamedKey are updated accordingly.
---
Outside diff comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 5635-5692: The method forceRefresh builds strings every call
(viewState and the interpolated reasons passed to liveSurfaceForGhosttyAccess),
causing allocations on the hot path; fix by moving construction of viewState and
any reason interpolation inside `#if` DEBUG so they are only created in debug
builds, and in release use constant literals (e.g. "forceRefresh" and
"forceRefresh.refresh") when calling
liveSurfaceForGhosttyAccess/ghostty_surface_refresh; update the code around
viewState, the displayID block that calls liveSurfaceForGhosttyAccess(reason:
...), and the subsequent MainActor.assumeIsolated call to use the debug-only
interpolated strings and release-only constants to avoid per-keystroke
allocations.
- Around line 4388-6291: The file mixes UI/portal code with a large
startup/socket-queue lifecycle; extract the non-UI runtime/bootstrap and socket
queue logic into a new helper (e.g., TerminalRuntimeLifecycle or
TerminalSocketQueue) and keep TerminalSurface as the UI/portal owner that
delegates lifecycle and queue operations. Move types and functions like
PendingKeyEvent, PendingSocketInput, ParsedSocketInput,
parsedSocketInputEvents(for:), enqueuePendingSocketInputs(_:),
enqueuePendingSocketInput(_:), flushPendingSocketInputIfNeeded(),
requestBackgroundSurfaceStartIfNeeded(),
startRuntimeUsingHeadlessWindowIfNeeded(reason:),
ensureHeadlessStartupWindowIfNeeded(reason:), createSurface(for:),
writeTextData(_:to:), sendInput(_:to:), sendText(_:), sendInputResult(_:),
sendNamedKey(_:), liveSurfaceForSocketWrite(reason:), and any supporting
constants/locks into the new file/class; expose a minimal MainActor-isolated API
(enqueue, flush, requestStart, createIfNeeded, liveSurface accessor) that
TerminalSurface calls, preserve existing behaviors/annotations (MainActor, debug
logs, TerminalSurfaceRegistry interactions) and update all internal calls in
TerminalSurface to delegate to the new helper while keeping UI-specific pieces
(attachedView, hostedView, view lifecycle, display id, forceRefresh, focus,
portal lease/state) in TerminalSurface. Ensure tests and debug-only helpers are
updated to use the new helper and keep runtime surface ownership and callback
context management consistent when moving createSurface-related code.
🪄 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: 842ccfb7-0368-42be-8b5b-34389e320860
📒 Files selected for processing (6)
Resources/Localizable.xcstringsSources/GhosttyTerminalView.swiftSources/TerminalController.swiftSources/Workspace.swiftcmuxTests/TerminalAndGhosttyTests.swifttests_v2/test_cli_background_terminal_helpers_start_pty.py
lawrencecchen
force-pushed
the
issue-4228-terminal-helper-surfaces-no-pty
branch
from
May 16, 2026 02:49
1faa11e to
b08ab60
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 `@cmuxTests/TerminalAndGhosttyTests.swift`:
- Around line 1045-1049: The test currently overwrites
TerminalController.shared's active tab manager and unconditionally restores nil;
instead capture the previous manager into a local (e.g., let previousManager =
TerminalController.shared.activeTabManager or similar) before creating the new
TabManager, call TerminalController.shared.setActiveTabManager(manager), and in
the defer restore the original value by calling
TerminalController.shared.setActiveTabManager(previousManager) so the exact
prior state is reinstated; reference the TabManager initializer and
TerminalController.shared.setActiveTabManager when making the change.
In `@Sources/GhosttyTerminalView.swift`:
- Around line 5758-5818: The guard that calls
ghostty_surface_process_exited(runtimeSurface) can dereference a stale surface
pointer; reorder checks in sendText(_:), sendNamedKey(_:), and
sendInputResult(_:) so you first obtain a liveSurface via
liveSurfaceForSocketWrite(reason:...), and only then check runtimeSurface state
(or avoid calling ghostty_surface_process_exited if liveSurface is nil).
Concretely: in sendText, sendNamedKey, and sendInputResult, move the
liveSurfaceForSocketWrite(...) guard ahead of the
ghostty_surface_process_exited(...) guard (or skip the process_exited check when
liveSurface is already nil), ensuring you never call
ghostty_surface_process_exited on a possibly torn-down surface pointer.
🪄 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: 81b92702-21e5-4fdc-8004-84cf9c701d84
📒 Files selected for processing (6)
Resources/Localizable.xcstringsSources/GhosttyTerminalView.swiftSources/TerminalController.swiftSources/Workspace.swiftcmuxTests/TerminalAndGhosttyTests.swifttests_v2/test_cli_background_terminal_helpers_start_pty.py
lawrencecchen
force-pushed
the
issue-4228-terminal-helper-surfaces-no-pty
branch
from
May 16, 2026 03:11
b08ab60 to
1111ddd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
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)
4401-4700: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftExtract the pending-input and bootstrap-window logic out of this file.
This PR adds more terminal lifecycle, socket-input parsing, queue accounting, and AppKit bootstrap-window management to a file that is already far beyond the repo’s size budget. Please move this into dedicated helper types/files instead of deepening
GhosttyTerminalView.swift’s mixed responsibilities.As per coding guidelines "Do not add more than 250 lines to an existing production Swift file that is already over 800 lines..." and "Do not mix UI rendering, state ownership, persistence, networking, parsing, subprocess/socket protocol, and platform bridge code in one Swift file".
Also applies to: 5756-6191, 6234-6308
🤖 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 4401 - 4700, The file is too large and mixes responsibilities; extract the pending-input queue and bootstrap-window logic into separate helper types/files: move the PendingKeyEvent, PendingSocketInput, ParsedSocketInput, Pending queue state (pendingSocketInputQueue, pendingSocketInputBytes, maxPendingSocketInputBytes) and their queue-management logic into a new SocketInputQueue (or similar) type, and move startRuntimeUsingHeadlessWindowIfNeeded, ensureHeadlessStartupWindowIfNeeded, headlessStartupWindow and any headless bootstrap window management into a new HeadlessWindowManager type; update GhosttyTerminalView to hold instances of these helpers, forward calls and state (e.g. rename/keep methods like startRuntimeUsingHeadlessWindowIfNeeded -> HeadlessWindowManager.startIfNeeded) and preserve access control, main-thread assertions, and existing references to surfaceView, hostedView, tabId, and id so behavior is unchanged.
🤖 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 `@Sources/GhosttyTerminalView.swift`:
- Around line 4716-4724: reconcileAttachedWindowIfNeeded currently uses the raw
surface pointer and may call ghostty_surface_set_display_id on a stale surface;
update it to call liveSurfaceForGhosttyAccess(...) (same helper used by
forceRefresh and socket write paths) to revalidate and obtain a live surface
before calling ghostty_surface_set_display_id, i.e. replace the guard-let that
binds surface with a guard-let that binds s from
liveSurfaceForGhosttyAccess(self) (or the appropriate receiver) and then call
ghostty_surface_set_display_id(s, displayID); keep
releaseHeadlessStartupWindowIfNeeded(for:) and the screen/displayID checks
as-is.
---
Outside diff comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 4401-4700: The file is too large and mixes responsibilities;
extract the pending-input queue and bootstrap-window logic into separate helper
types/files: move the PendingKeyEvent, PendingSocketInput, ParsedSocketInput,
Pending queue state (pendingSocketInputQueue, pendingSocketInputBytes,
maxPendingSocketInputBytes) and their queue-management logic into a new
SocketInputQueue (or similar) type, and move
startRuntimeUsingHeadlessWindowIfNeeded, ensureHeadlessStartupWindowIfNeeded,
headlessStartupWindow and any headless bootstrap window management into a new
HeadlessWindowManager type; update GhosttyTerminalView to hold instances of
these helpers, forward calls and state (e.g. rename/keep methods like
startRuntimeUsingHeadlessWindowIfNeeded -> HeadlessWindowManager.startIfNeeded)
and preserve access control, main-thread assertions, and existing references to
surfaceView, hostedView, tabId, and id so behavior is 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: bf8b8bda-71af-4b04-a658-12facc0add59
📒 Files selected for processing (6)
Resources/Localizable.xcstringsSources/GhosttyTerminalView.swiftSources/TerminalController.swiftSources/Workspace.swiftcmuxTests/TerminalAndGhosttyTests.swifttests_v2/test_cli_background_terminal_helpers_start_pty.py
lawrencecchen
force-pushed
the
issue-4228-terminal-helper-surfaces-no-pty
branch
from
May 16, 2026 03:27
1111ddd to
7419925
Compare
lawrencecchen
dismissed stale reviews from coderabbitai[bot], coderabbitai[bot], coderabbitai[bot], and coderabbitai[bot]
May 16, 2026 05:02
Resolved in follow-up commits; latest hosted checks and reviewer checks are green.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 72707b2. Configure here.
| @MainActor | ||
| private func scheduleHeadlessRuntimeStartIfNeeded(reason: String) { | ||
| startRuntimeUsingHeadlessWindowIfNeeded(reason: reason) | ||
| } |
There was a problem hiding this comment.
Redundant single-line wrapper adds unnecessary indirection
Low Severity
scheduleHeadlessRuntimeStartIfNeeded is a one-line wrapper that simply calls startRuntimeUsingHeadlessWindowIfNeeded with the same parameters and identical annotations (@MainActor, private). It adds an unnecessary layer of indirection without any scheduling, debouncing, or gating logic that would justify its existence. Callers could invoke startRuntimeUsingHeadlessWindowIfNeeded directly.
Reviewed by Cursor Bugbot for commit 72707b2. Configure here.
This was referenced May 17, 2026
This was referenced Jun 13, 2026
This was referenced Jul 12, 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.
Fixes #4228.
Summary:
Reproduction:
new-surface --workspace ... --pane pane:123 --type terminal --focus falsereturnedOK surface:232,sendreturned OK, thenread-screen --surface surface:232returnedERROR: Terminal surface not foundandtop --processesshowedsurface:232had no child process.new-pane --workspace ... --type terminal --direction right --focus falsereturnedOK surface:279, thenread-screen --surface surface:279returnedERROR: Terminal surface not foundandtop --processesshowed no child process.Red test:
32b3ee199failed on cloud Mac withTerminalOffscreenStartupTests: cold input hadpending.items == 0, daemon newline input hadkeyEvents == 0, and startup input madecreateAttemptCount == 0./tmp/cmux-4228-red-unit2.xcresultoncloud-mac-25947691246.Green tests:
06f1c0961passedTerminalOffscreenStartupTests: 9 tests, 0 failures./tmp/cmux-4228-actor-helper-unit.xcresult.06f1c0961passed a CircleCI-shaped debug build andscripts/swift_warning_budget.pywith 166 warnings across 75 buckets, under the 225/110 budget.Notes:
cua-ssh doctorpasses SSH, visible windows, and screen-recording preflight, but Skyget_app_statefails withComputer Use server error -10005: cgWindowNotFoundafter setup/reinstall.Note
Medium Risk
Changes terminal runtime startup and socket input delivery paths, including new headless bootstrap windows and queue management; regressions could impact background terminals, focus, or socket automation behavior.
Overview
Fixes offscreen/background terminal helpers by bootstrapping Ghostty runtimes in a hidden, borderless “headless” window when a terminal has startup work (initial command/input) or receives cold socket input before any real UI window is attached.
Refactors terminal socket input sending to return explicit results (
sent/queuedvsinputQueueFull/surfaceUnavailable/processExited), queue parsed input/key events (including control characters like Return/Tab/Escape/Backspace) up to a strict byte cap (rejecting oversize rather than evicting), and surfaces localized error messages via the socket API (and includesqueuedin v2 responses).Updates window/visibility checks across UI and focus paths to use
TerminalSurface.uiWindow/isViewInWindow(excluding the headless bootstrap window), adds debug health reporting for headless hosting, and introduces extensive unit + optional Python regression tests covering offscreen startup, cold input queuing, teardown, and overflow behavior.Reviewed by Cursor Bugbot for commit a96e1ce. Bugbot is set up for automated code reviews on this repo. Configure here.