Repository navigation
Add real tmux pane proxy support - #3275
Closed
islee23520 wants to merge 14 commits into
Closed
islee23520 wants to merge 14 commits into
islee23520 wants to merge 14 commits into
Conversation
|
@islee23520 is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
Caution Review failedThe pull request is closed. 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:
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds real‑tmux integration: a tmux‑pane proxy CLI, periodic tmux session reconciliation with persisted session→workspace mapping, UI to list/attach/kill tmux sessions, and routes split/focus/close behavior for tmux‑backed workspaces. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as "Right Sidebar UI"
participant TabManager
participant TMUX as "tmux binary"
participant Proxy as "__real-tmux-pane-proxy"
participant Workspace
TMUX->>TabManager: list-sessions (periodic)
TMUX-->>TabManager: return sessions + pane ids + cwd
TabManager->>TabManager: reconcile with persisted mapping
alt New session
TabManager->>Workspace: create workspace(with session id/name)
TabManager->>Proxy: build proxy command for pane
Workspace-->>UI: session appears in sidebar
end
User->>UI: attach/select session
UI->>TabManager: select workspace
TabManager->>TMUX: select-window/select-pane
TMUX-->>TabManager: confirm pane selected
TabManager->>Workspace: mark focused
User->>Workspace: request split
Workspace->>TabManager: newSplit(allowInRealTmuxWorkspace: true)
TabManager->>TMUX: split-window / capture-pane
TMUX-->>TabManager: new pane id
TabManager->>Proxy: launch proxy attached to new pane
Proxy-->>Workspace: terminal shows proxied pane
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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. Review rate limit: 6/8 reviews remaining, refill in 8 minutes and 46 seconds.Comment |
Contributor
Greptile SummaryThis PR adds real tmux session import into cmux workspaces: a Two P1 threading issues need to be fixed before merge:
Confidence Score: 2/5Not safe to merge — two P1 threading violations will cause repeated UI freezes for any user with tmux running. Two independent P1 findings (main-thread blocking in TabManager sync timer and TmuxSessionListView refresh) together push the score below the P1 ceiling of 4. Sources/TabManager.swift (syncRealTmuxSessions threading) and Sources/RightSidebarPanelView.swift (TmuxSessionListView.refresh threading) Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as TabManager / TmuxSessionListView
participant Main as Main Thread
participant BG as Background Queue
participant Tmux as tmux process
participant Proxy as cmux __real-tmux-pane-proxy
Note over UI,Tmux: Session Sync (every 2 s)
BG->>Main: DispatchQueue.main.async
Main->>Tmux: list-sessions (waitUntilExit) blocks main
Tmux-->>Main: session list
Main->>Tmux: display-message pane_id (waitUntilExit) blocks main
Tmux-->>Main: pane id
Main->>Main: addWorkspace / setCustomTitle
Note over UI,Proxy: Pane Proxy Lifecycle
UI->>Proxy: exec cmux __real-tmux-pane-proxy --pane %N
Proxy->>Tmux: tmux -C attach-session (control mode)
Tmux-->>Proxy: %output %N payload
Proxy->>Proxy: decodeRealTmuxControlPayload (octal unescape)
Proxy-->>UI: raw terminal bytes to stdout
Note over UI,Proxy: Input Forwarding
UI->>Proxy: stdin bytes
Proxy->>Tmux: send-keys -l / -H / C-c / Enter / Escape
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+705
to
+714
| return store | ||
| } | ||
|
|
||
| private func saveRealTmuxStore(_ store: RealTmuxStore) { | ||
| guard let data = try? JSONEncoder().encode(store) else { return } | ||
| try? FileManager.default.createDirectory( | ||
| at: realTmuxStoreURL.deletingLastPathComponent(), | ||
| withIntermediateDirectories: true | ||
| ) | ||
| try? data.write(to: realTmuxStoreURL, options: .atomic) |
Contributor
There was a problem hiding this comment.
refresh() blocks the main actor with subprocess I/O
The .task loop calls await MainActor.run { refresh() }, which runs listRealTmuxSessions(), realTmuxCurrentPaneId(), and realTmuxOutput() — all blocking Process.waitUntilExit() calls — on the main actor. The onAppear and onChange handlers call refresh() directly on the main thread as well. With multiple tmux sessions this will freeze the UI for the duration of each subprocess. The subprocess work needs to be done off-main and only the resulting state updates applied on main.
| // If not (e.g. `ignore` keybind), fall through to interpretKeyEvents | ||
| // so the IME gets a chance to process this event. | ||
| if handled { return } | ||
| if handled { return } |
Contributor
There was a problem hiding this comment.
if handled { return }
This line was de-indented from 12 spaces (inside the if flags.contains(.control) block) to 8 spaces (matching the closing } of that same block). Swift scoping is brace-based so it compiles correctly, but the indentation now falsely implies this return is outside the control-key fast path, making it appear to guard the fall-through path unconditionally. This will mislead future editors of this latency-sensitive code path.
Suggested change
| if handled { return } | |
| if handled { return } |
Comment on lines
5655
to
+5660
| /// Create a new terminal surface in the focused pane of the selected workspace | ||
| func newSurface() { | ||
| // Cmd+T should always focus the newly created surface. | ||
| selectedWorkspace?.clearSplitZoom() | ||
| selectedWorkspace?.newTerminalSurfaceInFocusedPane(focus: true) | ||
| guard let workspace = selectedWorkspace else { return } | ||
| if workspace.isRealTmuxWorkspace, | ||
| let focusedPanelId = workspace.focusedPanelId { |
Contributor
There was a problem hiding this comment.
newSurface(initialInput:) silently drops initialInput for tmux workspaces
When workspace.isRealTmuxWorkspace, the method calls createSplit and returns early, discarding the caller-supplied initialInput without any comment or fallback. If this is intentional (input forwarding isn't supported via splits), a brief comment explaining why would prevent future confusion and accidental reintroduction.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/MainWindowFocusController.swift (1)
313-355:⚠️ Potential issue | 🟡 MinorClear tmux pending-first-item state to avoid false-positive focus success.
For
.tmux, endpoint focus is intentionallyfalse(Line 346 and Line 543), butpendingRightSidebarFirstItemFocusModecan remain set from Line 317. That leaves a sticky pending state and can makefocusRightSidebar(...)returntrueeven when no responder was focused.Suggested fix
@@ - pendingRightSidebarFirstItemFocusMode = focusFirstItem ? mode : nil + pendingRightSidebarFirstItemFocusMode = + (focusFirstItem && mode != .tmux) ? mode : nil @@ - let fallbackResult = modeResult ? false : focusFallbackRightSidebarHost() - let result = modeResult || fallbackResult || pendingRightSidebarFirstItemFocusMode == mode + let fallbackResult = modeResult ? false : focusFallbackRightSidebarHost() + let hasPendingEndpointFocus = mode != .tmux && pendingRightSidebarFirstItemFocusMode == mode + let result = modeResult || fallbackResult || hasPendingEndpointFocusAlso applies to: 523-545
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/MainWindowFocusController.swift` around lines 313 - 355, In focusRightSidebar(mode:focusFirstItem:) clear the sticky pendingRightSidebarFirstItemFocusMode when handling the .tmux branch so the pending-first-item flag cannot make the method return true incorrectly: inside the switch case .tmux set pendingRightSidebarFirstItemFocusMode = nil (and keep modeResult = false) so no stale pending state remains; apply the same change to the corresponding tmux branch in the other function referenced around lines 523-545 to ensure consistent behavior.
🧹 Nitpick comments (4)
Sources/TerminalController.swift (4)
5172-5173: Usews.isRealTmuxWorkspaceproperty for consistency.This hunk computes
isRealTmuxWorkspaceinline, but hunks 5, 6, 7, and 8 use thews.isRealTmuxWorkspaceproperty directly. Using the property ensures the detection logic remains centralized and consistent.♻️ Proposed fix
- let isRealTmuxWorkspace = ws.realTmuxSessionId != nil || ws.realTmuxSessionName != nil - if ws.panels.count <= 1 && !isRealTmuxWorkspace { + if ws.panels.count <= 1 && !ws.isRealTmuxWorkspace {Then update the later usages of
isRealTmuxWorkspacetows.isRealTmuxWorkspace.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalController.swift` around lines 5172 - 5173, Replace the inline computation of isRealTmuxWorkspace with the existing ws.isRealTmuxWorkspace property to keep detection logic centralized: change the condition "let isRealTmuxWorkspace = ws.realTmuxSessionId != nil || ws.realTmuxSessionName != nil" and subsequent usage in the if-statement to use "ws.isRealTmuxWorkspace" so the code consistently relies on the workspace property (refer to the ws.isRealTmuxWorkspace property and the surrounding if that checks ws.panels.count).
16125-16127: Usetab.isRealTmuxWorkspaceproperty for consistency.Same issue as hunk 4 - computes inline instead of using the property. Aligning with the property ensures consistent detection logic.
♻️ Proposed fix
- let isRealTmuxWorkspace = tab.realTmuxSessionId != nil || tab.realTmuxSessionName != nil - // Don't close if it's the only surface, unless the surface represents a real tmux session. - if tab.panels.count <= 1 && !isRealTmuxWorkspace { + // Don't close if it's the only surface, unless the surface represents a real tmux session. + if tab.panels.count <= 1 && !tab.isRealTmuxWorkspace {Then update the later usage of
isRealTmuxWorkspacetotab.isRealTmuxWorkspace.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalController.swift` around lines 16125 - 16127, Replace the inline computation of a tmux workspace boolean with the existing property: change usages that compute isRealTmuxWorkspace as "tab.realTmuxSessionId != nil || tab.realTmuxSessionName != nil" to use "tab.isRealTmuxWorkspace" (and update subsequent references to the variable to use tab.isRealTmuxWorkspace as well) so detection logic is consistent across TerminalController.swift and avoids duplicating the condition.
3428-3433: Consistent with hunk 1 - same redundancy note applies.Same
kind/sourceredundancy as the earlier payload. Consider extracting a helper if this pattern appears in more places.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalController.swift` around lines 3428 - 3433, Redundant construction of the "kind" and "source" fields repeats the same ternary logic; extract a small helper (e.g., a function that returns ["kind": ..., "source": ...] or a tuple) and call it where the payload is built to avoid duplication. Update the payload building here (the block referencing workspace.isRealTmuxWorkspace and real_tmux / realTmuxSessionId/sessionName) to use that helper and keep the real_tmux conditional as-is (referencing workspace.realTmuxSessionId and workspace.realTmuxSessionName) so the produced JSON remains identical but the kind/source logic is centralized.
2996-3001:kindandsourceappear redundant here.Both fields are set to the same value (
"real-tmux"or"native"). If this is intentional for forward compatibility (e.g.,sourcemay diverge later), consider adding a brief comment. Otherwise, one field may suffice.The conditional payload structure with
v2OrNullfor optional fields looks correct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalController.swift` around lines 2996 - 3001, The payload sets both "kind" and "source" to the identical value based on workspace.isRealTmuxWorkspace which is redundant; either remove one field (e.g., drop "source") to avoid duplication or keep both but add a clear comment explaining the forward-compatibility intent so future readers know they may diverge; update the block that builds the dictionary (the keys "kind", "source", and "real_tmux" and the use of workspace.isRealTmuxWorkspace, v2OrNull, realTmuxSessionId, realTmuxSessionName) accordingly to either omit the duplicate key or include the explanatory comment above the two assignments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 17275-17278: realTmuxExecutablePath() currently only checks three
hard-coded paths and should also search the user's PATH; update
realTmuxExecutablePath() to first iterate over PATH components (from
ProcessInfo.processInfo.environment["PATH"] split by ":") and return the first
entry where FileManager.default.isExecutableFile(atPath:) is true for "tmux",
falling back to the existing hard-coded array only if nothing on PATH is
executable, and ensure the function still returns an optional String.
- Around line 17083-17095: The defer restoring terminal attributes is bypassed
when finishProxy() calls exit(), leaving the PTY in raw mode; update
finishProxy() to explicitly restore the original termios before calling exit()
by checking the same hasTermios flag and calling tcsetattr(STDIN_FILENO,
TCSANOW, &originalTermios) (using the originalTermios variable used when setting
raw), then call exit(); apply the same explicit restore at the second occurrence
that mirrors the code using originalTermios/hasTermios so both code paths
restore the tty prior to exiting.
In `@Sources/AppDelegate.swift`:
- Around line 3917-3926: The matching currently uses "realTmuxSessionId ==
sessionId || title == sessionName" which returns wrong workspaces when titles
collide; update mainWindowWorkspaceForRealTmuxSession to first search all
contexts' tabManager.tabs for a workspace where realTmuxSessionId == sessionId
and return it if found, and only if no ID match exists perform a second search
that matches title == sessionName but restricts that fallback to tmux workspaces
(e.g., where realTmuxSessionId != nil or an isTmux flag) to avoid matching
non-tmux workspaces; reference mainWindowWorkspaceForRealTmuxSession,
mainWindowContexts, tabManager.tabs, realTmuxSessionId and title when making the
change.
In `@Sources/GhosttyTerminalView.swift`:
- Around line 7083-7085: The current early return when
shouldBlockRealTmuxShortcut(event, surface: surface) is true swallows Ctrl+B
(tmux prefix) and creates unmatched keyUp events; instead of returning early in
the key handler (where shouldBlockRealTmuxShortcut is called), detect the Ctrl+B
case and forward the event into the tmux input path so tmux receives both press
and release (e.g., call the existing tmux input/forwarding function used
elsewhere) and only suppress local handling for non-tmux shortcuts; update both
keyDown and keyUp paths to use shouldBlockRealTmuxShortcut and the
tmux-forwarding function so events are consistently routed rather than dropped.
In `@Sources/RightSidebarPanelView.swift`:
- Around line 646-649: realTmuxExecutablePath currently only checks three
hard-coded locations and misses tmux installs on PATH (MacPorts, Nix, custom
paths); update realTmuxExecutablePath() to first search PATH for an executable
named "tmux" (e.g., inspect ProcessInfo.processInfo.environment["PATH"] and test
each component with FileManager.isExecutableFile(atPath:) or run `which tmux`
via Process) and return that if found, then fall back to the existing array
["/opt/homebrew/bin/tmux","/usr/local/bin/tmux","/usr/bin/tmux"] as a secondary
lookup; keep the function signature and behavior of returning an optional String
and use the same FileManager check for fallbacks.
- Around line 511-515: The onKill closure currently calls
killRealTmuxSession(session.name) and immediately closes the mapped workspace
via tabManager.closeWorkspaceWithConfirmation; change killRealTmuxSession to
return a Bool (or Result) that reflects the tmux process terminationStatus (or
thrown error), and in onKill only call
tabManager.closeWorkspaceWithConfirmation(workspace) after receiving a
successful result (or after performing a refresh check that confirms the tmux
session no longer exists). Update the killRealTmuxSession signature and all
callers (including the similar pattern used in the other onKill location) to
await/inspect the success flag and handle failures by logging or refreshing
instead of removing the workspace immediately.
- Around line 523-541: The refresh() path currently runs blocking tmux and file
I/O on the MainActor; change it to perform tmux polling,
Process.waitUntilExit(), Data(contentsOf:), JSON encoding/decoding, and any
store reads/writes inside a Task.detached (or otherwise off the MainActor) at
.utility priority to produce a snapshot object (e.g. via a new
collectTmuxSnapshot() or similar function), then marshal back to the MainActor
and apply only the UI mutations (sessions, workspace, and tabManager) in an
apply(snapshot) function; update the recurring task to call the new async
refresh() (which awaits the detached task and then calls MainActor.run to apply)
so the 1.5s polling loop does not hop to MainActor before doing blocking work.
- Around line 549-573: The pruning/import code currently filters and resolves
workspaces using only self.tabManager.tabs, causing cross-window tmux sessions
to be dropped or duplicated; change the lookup to search across all main window
contexts’ tab managers (e.g., aggregate tabs from mainWindowContexts'
tabManager.tabs) when building liveWorkspaceIds and when finding an existing
workspace for a session (instead of tabManager.tabs.first(where: ...)). Ensure
the store.sessionIdToWorkspaceId filter uses that aggregated set and that the
existing-workspace resolution and subsequent tabManager.addWorkspace fallback
remain the same but only run if no workspace exists across any main window
context.
In `@Sources/TabManager.swift`:
- Around line 5477-5482: The early return in focusSurface(tabId:surfaceId:)
drops explicit surface-focus requests for tmux-backed workspaces; instead when
tab.isRealTmuxWorkspace is true call selectRealTmuxPane(tabId:surfaceId:) (or
the appropriate selectRealTmuxPane method) to route the focus to the tmux pane
and also update local selection state (e.g., still call
tab.focusPanel(surfaceId) or update the tab's focusedSurface) so the app
reflects the explicit focus-intent; ensure this code path is used only for
explicit in-app focus commands and does not change global macOS app focus for
socket/CLI-triggered events.
- Around line 5668-5673: The newSurface(initialInput:) branch for real-tmux
workspaces calls createSplit(tabId:surfaceId:direction:) but never passes or
applies the initialInput, causing startup text to be lost for tmux-backed panes;
modify the tmux split path to either accept an initialInput parameter (update
createSplit signature or add a new createSplitWithInitialInput) and thread the
initialInput through to the created pane, or after createSplit returns use the
created pane identifier to send the initialInput into that pane (e.g., via the
same method non-tmux splits use); apply the same fix to the analogous code block
around the other occurrence referenced (the block at the later newSurface call).
- Around line 1221-1228: The current matching falls back to tabs.first {
$0.realTmuxSessionId == session.id || $0.title == session.name }, which can
incorrectly convert a local workspace when its title equals a tmux session name;
remove the title==session.name fallback so we only adopt when a persisted or
already-tmux-backed workspace is found. Change the lookup to use
AppDelegate.shared?.mainWindowWorkspaceForRealTmuxSession(id:name:) or
tabs.first(where: { $0.realTmuxSessionId == session.id }) only, then set
store.sessionIdToWorkspaceId[session.id] and update
existing.realTmuxSessionId/realTmuxSessionName as before (symbols:
mainWindowWorkspaceForRealTmuxSession, tabs, realTmuxSessionId,
store.sessionIdToWorkspaceId, session.id, session.name,
existing.realTmuxSessionName).
- Around line 4902-4905: The current logic gates tmux close routing by
tab.isRealTmuxWorkspace, which fails for browser panels that lack a tmux pane;
change both occurrences (the block around closeRealTmuxPanel(tab: panelId:) and
the similar block at the other occurrence) to check the specific panel's real
tmux pane id by calling tab.terminalPanel(for: panelId)?.realTmuxPaneId and only
call closeRealTmuxPanel(...) when that pane id exists (otherwise fall back to
the non-tmux close path). Ensure you reference and use tab.terminalPanel(for:),
realTmuxPaneId, and closeRealTmuxPanel(tab:panelId:) when making the conditional
change.
- Around line 1207-1212: syncRealTmuxSessions() calls closeWorkspace(...) when a
tmux session is gone but closeWorkspace bails out if tabs.count <= 1, leaving
the real-tmux session stale; fix by detecting when the to-be-closed workspace is
the last tab (using tabs and store.sessionIdToWorkspaceId + liveSessionIds) and
either create a replacement workspace in that window first (via whatever
create/open-new-workspace API your codebase provides) before calling
closeWorkspace, or route this cleanup through the existing user-close path that
can handle removing the last workspace so the real tmux session is cleaned up;
ensure you reference syncRealTmuxSessions and closeWorkspace and handle the
last-tab case explicitly.
- Around line 1186-1197: startRealTmuxSessionSyncTimer currently schedules the
timer to hop onto DispatchQueue.main and calls syncRealTmuxSessions on the main
actor, but syncRealTmuxSessions performs blocking Process.waitUntilExit(), file
reads, and JSON writes; move the tmux probe and store I/O off the main actor by
running the heavy work on a background queue inside the timer handler and only
dispatch back to the main queue for quick workspace/store mutations and UI
updates. Specifically, update startRealTmuxSessionSyncTimer's
timer.setEventHandler to call syncRealTmuxSessions (or a new helper like
probeRealTmuxSessions) on a global/utility queue, ensure syncRealTmuxSessions
does not perform blocking operations on the main thread (extract blocking
Process.waitUntilExit, file reads/writes into background-only code), and when
results are ready dispatch minimal state-updating code back to
DispatchQueue.main to update the workspace/store and assign
realTmuxSessionSyncTimer as before.
- Around line 1230-1239: Compute a single boolean (e.g. hasRealTmux) and only
treat the session as a real‑tmux workspace when both
Self.realTmuxCurrentPaneId(for: session) and the proxy command from
Self.realTmuxPaneProxyCommand(...) are non‑nil; pass
initialTerminalRealTmuxPaneId = hasRealTmux ? initialPaneId : nil and
initialTerminalCommand = hasRealTmux ? initialProxyCommand : nil, otherwise
supply the attach input (Self.realTmuxAttachInput(for: session)) as
initialTerminalInput so addWorkspace(...) is called as a non‑real‑tmux import
when a real tmux pane id/proxy is not available.
In `@Sources/TerminalController.swift`:
- Around line 14644-14648: The code path where tabManager.newSplit(...) returns
nil lacks error handling and leaves result unset; update the branch so that when
newSplit returns nil you set result to an internal error string (matching the V2
pattern, e.g. "internal_error new_split_failed") before returning; locate the
call to tabManager.newSplit(tabId: tabId, surfaceId: focusedPanelId, direction:
direction, focus: focus) and ensure the else path sets result appropriately and
returns.
In `@Sources/Workspace.swift`:
- Around line 6647-6651: sessionSnapshot(...) and restoreSessionSnapshot(...)
currently only persist generic terminal state so real tmux identity
(realTmuxSessionId, realTmuxSessionName) and per-panel realTmuxPaneId are lost
across restarts; update the snapshot model to include realTmuxSessionId and
realTmuxSessionName on the Workspace snapshot and include realTmuxPaneId on each
Panel snapshot, serialize those fields in sessionSnapshot(...) and rehydrate
them in restoreSessionSnapshot(...), and if you prefer opting out add an
explicit flag (e.g., isRealTmuxWorkspace or persistRealTmuxIdentity) to the
snapshot so real tmux workspaces can be excluded from restore when set.
---
Outside diff comments:
In `@Sources/MainWindowFocusController.swift`:
- Around line 313-355: In focusRightSidebar(mode:focusFirstItem:) clear the
sticky pendingRightSidebarFirstItemFocusMode when handling the .tmux branch so
the pending-first-item flag cannot make the method return true incorrectly:
inside the switch case .tmux set pendingRightSidebarFirstItemFocusMode = nil
(and keep modeResult = false) so no stale pending state remains; apply the same
change to the corresponding tmux branch in the other function referenced around
lines 523-545 to ensure consistent behavior.
---
Nitpick comments:
In `@Sources/TerminalController.swift`:
- Around line 5172-5173: Replace the inline computation of isRealTmuxWorkspace
with the existing ws.isRealTmuxWorkspace property to keep detection logic
centralized: change the condition "let isRealTmuxWorkspace =
ws.realTmuxSessionId != nil || ws.realTmuxSessionName != nil" and subsequent
usage in the if-statement to use "ws.isRealTmuxWorkspace" so the code
consistently relies on the workspace property (refer to the
ws.isRealTmuxWorkspace property and the surrounding if that checks
ws.panels.count).
- Around line 16125-16127: Replace the inline computation of a tmux workspace
boolean with the existing property: change usages that compute
isRealTmuxWorkspace as "tab.realTmuxSessionId != nil || tab.realTmuxSessionName
!= nil" to use "tab.isRealTmuxWorkspace" (and update subsequent references to
the variable to use tab.isRealTmuxWorkspace as well) so detection logic is
consistent across TerminalController.swift and avoids duplicating the condition.
- Around line 3428-3433: Redundant construction of the "kind" and "source"
fields repeats the same ternary logic; extract a small helper (e.g., a function
that returns ["kind": ..., "source": ...] or a tuple) and call it where the
payload is built to avoid duplication. Update the payload building here (the
block referencing workspace.isRealTmuxWorkspace and real_tmux /
realTmuxSessionId/sessionName) to use that helper and keep the real_tmux
conditional as-is (referencing workspace.realTmuxSessionId and
workspace.realTmuxSessionName) so the produced JSON remains identical but the
kind/source logic is centralized.
- Around line 2996-3001: The payload sets both "kind" and "source" to the
identical value based on workspace.isRealTmuxWorkspace which is redundant;
either remove one field (e.g., drop "source") to avoid duplication or keep both
but add a clear comment explaining the forward-compatibility intent so future
readers know they may diverge; update the block that builds the dictionary (the
keys "kind", "source", and "real_tmux" and the use of
workspace.isRealTmuxWorkspace, v2OrNull, realTmuxSessionId, realTmuxSessionName)
accordingly to either omit the duplicate key or include the explanatory comment
above the two assignments.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: fc7cdd89-b3ad-40e1-8f8a-e2080f38873f
📒 Files selected for processing (10)
CLI/cmux.swiftSources/AppDelegate.swiftSources/ContentView.swiftSources/GhosttyTerminalView.swiftSources/MainWindowFocusController.swiftSources/Panels/TerminalPanel.swiftSources/RightSidebarPanelView.swiftSources/TabManager.swiftSources/TerminalController.swiftSources/Workspace.swift
Comment on lines
+17083
to
+17095
| setbuf(stdout, nil) | ||
| var originalTermios = termios() | ||
| let hasTermios = tcgetattr(STDIN_FILENO, &originalTermios) == 0 | ||
| if hasTermios { | ||
| var rawTermios = originalTermios | ||
| cfmakeraw(&rawTermios) | ||
| _ = tcsetattr(STDIN_FILENO, TCSANOW, &rawTermios) | ||
| } | ||
| defer { | ||
| if hasTermios { | ||
| _ = tcsetattr(STDIN_FILENO, TCSANOW, &originalTermios) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Swift, does calling exit()terminate the process before enclosingdefer blocks execute?
💡 Result:
No, calling exit in Swift (imported from Darwin) terminates the process immediately without executing enclosing defer blocks.
Citations:
- 1: https://lists.swift.org/pipermail/swift-dev/Week-of-Mon-20170206/003987.html
- 2: https://stackoverflow.com/questions/24102157/seeking-an-exit-equivalent-in-swift
- 3: deinit not called from global scope swiftlang/swift#79701
🏁 Script executed:
# First, locate the file and check its size
find . -name "cmux.swift" -type fRepository: manaflow-ai/cmux
Length of output: 77
🏁 Script executed:
# Check the lines around 17083-17095 to see the defer block
sed -n '17080,17100p' CLI/cmux.swiftRepository: manaflow-ai/cmux
Length of output: 752
🏁 Script executed:
# Check lines 17123-17134 to see the duplicate issue
sed -n '17120,17140p' CLI/cmux.swiftRepository: manaflow-ai/cmux
Length of output: 853
🏁 Script executed:
# Find the finishProxy function to see where exit() is called
rg -A 20 "func finishProxy" CLI/cmux.swiftRepository: manaflow-ai/cmux
Length of output: 875
🏁 Script executed:
# Check if hasTermios is defined at function scope or earlier in main context
rg -B 50 "func finishProxy" CLI/cmux.swift | grep -E "(hasTermios|var|let)" | tail -20Repository: manaflow-ai/cmux
Length of output: 757
Restore the tty before calling exit() in finishProxy().
The defer block restores terminal attributes when the function exits normally, but finishProxy() calls exit() directly, which terminates the process immediately and bypasses all defer blocks. This leaves the pane PTY in raw mode after teardown.
Add explicit terminal restoration before the exit() call in finishProxy():
Suggested fix
func finishProxy(status: Int32) -> Never {
+ if hasTermios {
+ _ = tcsetattr(STDIN_FILENO, TCSANOW, &originalTermios)
+ }
if !didFinish {
didFinish = true
controlOutput.fileHandleForReading.readabilityHandler = nil
controlError.fileHandleForReading.readabilityHandler = nil
if controlProcess.isRunning {
controlInput.fileHandleForWriting.write(Data("detach-client\n".utf8))
controlProcess.terminate()
}
}
exit(status)
}Also applies to the second occurrence at lines 17123-17134.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLI/cmux.swift` around lines 17083 - 17095, The defer restoring terminal
attributes is bypassed when finishProxy() calls exit(), leaving the PTY in raw
mode; update finishProxy() to explicitly restore the original termios before
calling exit() by checking the same hasTermios flag and calling
tcsetattr(STDIN_FILENO, TCSANOW, &originalTermios) (using the originalTermios
variable used when setting raw), then call exit(); apply the same explicit
restore at the second occurrence that mirrors the code using
originalTermios/hasTermios so both code paths restore the tty prior to exiting.
Comment on lines
+17254
to
+17267
| for byte in data { | ||
| switch byte { | ||
| case 0x03: | ||
| flushLiteralBuffer() | ||
| _ = runRealTmux(tmuxPath: tmuxPath, arguments: ["send-keys", "-t", paneId, "C-c"]) | ||
| case 0x04: | ||
| flushLiteralBuffer() | ||
| _ = runRealTmux(tmuxPath: tmuxPath, arguments: ["send-keys", "-t", paneId, "C-d"]) | ||
| case 0x0D, 0x0A: | ||
| flushLiteralBuffer() | ||
| _ = runRealTmux(tmuxPath: tmuxPath, arguments: ["send-keys", "-t", paneId, "Enter"]) | ||
| case 0x1B: | ||
| flushLiteralBuffer() | ||
| _ = runRealTmux(tmuxPath: tmuxPath, arguments: ["send-keys", "-t", paneId, "Escape"]) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In tmux, what happens if an arrow-key sequence (ESC [ A) is forwarded as send-keys Escapeplus literal[andA, instead of preserving the raw byte sequence?
💡 Result:
In tmux, forwarding an arrow-key sequence as separate send-keys Escape followed by literal [ and A (instead of the raw byte sequence ESC [ A) sends three distinct keystrokes to the target pane: first ESC (0x1B), then [ (0x5B), then A (0x41). These are not combined or interpreted as a CSI sequence by tmux itself during sending. The target application (e.g., shell, vim) receives them as individual characters/keystrokes. Whether they form an arrow key depends on the application's input parser and timing: - Many TUIs (like vim, bash with showkey -a) use an escape-time timeout (default ~500ms in tmux/vim) to distinguish lone ESC from escape sequences. If [ and A arrive quickly after ESC (typical in send-keys), it may parse as Up arrow (CSI A). - If delayed (e.g., slow network), or if escape-time is high, it treats them separately: ESC cancels/escapes, then literal [ and A are inserted. This differs from raw ESC [ A (sent via send-keys -l $'\e[ A' or send-keys -H 1b 5b 41), which arrives as a contiguous byte stream more reliably parsed as arrow by applications. tmux send-keys with named keys like Up sends the exact same bytes as raw CSI (per key table matching terminfo), preserving the sequence. Separate sends risk timeout misinterpretation. To send raw bytes literally without key name lookup: send-keys -l "string" or -H for hex.
Citations:
- 1: https://man7.org/linux/man-pages/man1/tmux.1.html
- 2: https://man.openbsd.org/i386/tmux.1
- 3: https://github.com/tmux/tmux/blob/master/cmd-send-keys.c
- 4: https://blog.damonkelley.me/2016/09/07/tmux-send-keys/
- 5: https://stackoverflow.com/questions/19313807/tmux-send-keys-syntax/19330452
- 6: https://superuser.com/questions/1891760/sending-raw-keys-with-tmux
🏁 Script executed:
# Check the file size and examine the context around lines 17254-17267
wc -l CLI/cmux.swiftRepository: manaflow-ai/cmux
Length of output: 81
🏁 Script executed:
# Read the context around the flagged lines, including function signature
sed -n '17200,17300p' CLI/cmux.swift | cat -nRepository: manaflow-ai/cmux
Length of output: 4744
🏁 Script executed:
# Search for the function definition containing these lines
sed -n '17000,17254p' CLI/cmux.swift | tail -100 | cat -nRepository: manaflow-ai/cmux
Length of output: 4747
Don't send ESC as a standalone key—it breaks CSI/SS3 sequences.
Line 66–68 treats every 0x1B byte as a standalone Escape key sent to tmux. This breaks escape sequences like arrow keys (ESC [ A), function keys, and Alt+key combinations that use CSI/SS3 encoding. When ESC is sent separately via send-keys Escape, the following bytes [ and A are sent in subsequent send-keys calls, causing the target application's escape-time timeout (~500 ms in tmux/vim) to expire between them. The application then interprets ESC as a cancel/escape, and [ and A as literal characters instead of part of the arrow-key sequence.
Use send-keys -l (literal mode) or -H (hex mode) to preserve raw byte sequences together, or buffer ESC with lookahead to detect and handle complete CSI/SS3 sequences as atomic units.
Comment on lines
+17275
to
+17278
| private func realTmuxExecutablePath() -> String? { | ||
| ["/opt/homebrew/bin/tmux", "/usr/local/bin/tmux", "/usr/bin/tmux"] | ||
| .first { FileManager.default.isExecutableFile(atPath: $0) } | ||
| } |
There was a problem hiding this comment.
Look up tmux on PATH before failing.
realTmuxExecutablePath() only checks three hard-coded locations. Valid installs from Nix, MacPorts, custom Homebrew prefixes, or wrapper-based setups will hit tmux executable not found even though tmux is available.
♻️ Suggested fix
private func realTmuxExecutablePath() -> String? {
- ["/opt/homebrew/bin/tmux", "/usr/local/bin/tmux", "/usr/bin/tmux"]
- .first { FileManager.default.isExecutableFile(atPath: $0) }
+ if let path = ProcessInfo.processInfo.environment["PATH"] {
+ for entry in path.split(separator: ":") {
+ let candidate = String(entry) + "/tmux"
+ if FileManager.default.isExecutableFile(atPath: candidate) {
+ return candidate
+ }
+ }
+ }
+ return ["/opt/homebrew/bin/tmux", "/usr/local/bin/tmux", "/usr/bin/tmux"]
+ .first { FileManager.default.isExecutableFile(atPath: $0) }
}📝 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
| private func realTmuxExecutablePath() -> String? { | |
| ["/opt/homebrew/bin/tmux", "/usr/local/bin/tmux", "/usr/bin/tmux"] | |
| .first { FileManager.default.isExecutableFile(atPath: $0) } | |
| } | |
| private func realTmuxExecutablePath() -> String? { | |
| if let path = ProcessInfo.processInfo.environment["PATH"] { | |
| for entry in path.split(separator: ":") { | |
| let candidate = String(entry) + "/tmux" | |
| if FileManager.default.isExecutableFile(atPath: candidate) { | |
| return candidate | |
| } | |
| } | |
| } | |
| return ["/opt/homebrew/bin/tmux", "/usr/local/bin/tmux", "/usr/bin/tmux"] | |
| .first { FileManager.default.isExecutableFile(atPath: $0) } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLI/cmux.swift` around lines 17275 - 17278, realTmuxExecutablePath()
currently only checks three hard-coded paths and should also search the user's
PATH; update realTmuxExecutablePath() to first iterate over PATH components
(from ProcessInfo.processInfo.environment["PATH"] split by ":") and return the
first entry where FileManager.default.isExecutableFile(atPath:) is true for
"tmux", falling back to the existing hard-coded array only if nothing on PATH is
executable, and ensure the function still returns an optional String.
Comment on lines
+3917
to
+3926
| func mainWindowWorkspaceForRealTmuxSession(id sessionId: String, name sessionName: String) -> Workspace? { | ||
| for context in mainWindowContexts.values { | ||
| if let workspace = context.tabManager.tabs.first(where: { | ||
| $0.realTmuxSessionId == sessionId || $0.title == sessionName | ||
| }) { | ||
| return workspace | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Tighten real-tmux workspace matching to avoid false positives.
$0.realTmuxSessionId == sessionId || $0.title == sessionName can resolve the wrong workspace when titles collide (including non-tmux workspaces). This can misroute tmux pane actions. Prefer: ID match first, then tmux-only fallback by name.
Proposed fix
func mainWindowWorkspaceForRealTmuxSession(id sessionId: String, name sessionName: String) -> Workspace? {
- for context in mainWindowContexts.values {
- if let workspace = context.tabManager.tabs.first(where: {
- $0.realTmuxSessionId == sessionId || $0.title == sessionName
- }) {
- return workspace
- }
- }
- return nil
+ for context in mainWindowContexts.values {
+ if let workspace = context.tabManager.tabs.first(where: { $0.realTmuxSessionId == sessionId }) {
+ return workspace
+ }
+ }
+
+ guard !sessionName.isEmpty else { return nil }
+ for context in mainWindowContexts.values {
+ if let workspace = context.tabManager.tabs.first(where: {
+ $0.realTmuxSessionId != nil && $0.title == sessionName
+ }) {
+ return workspace
+ }
+ }
+ return nil
}📝 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
| func mainWindowWorkspaceForRealTmuxSession(id sessionId: String, name sessionName: String) -> Workspace? { | |
| for context in mainWindowContexts.values { | |
| if let workspace = context.tabManager.tabs.first(where: { | |
| $0.realTmuxSessionId == sessionId || $0.title == sessionName | |
| }) { | |
| return workspace | |
| } | |
| } | |
| return nil | |
| } | |
| func mainWindowWorkspaceForRealTmuxSession(id sessionId: String, name sessionName: String) -> Workspace? { | |
| for context in mainWindowContexts.values { | |
| if let workspace = context.tabManager.tabs.first(where: { $0.realTmuxSessionId == sessionId }) { | |
| return workspace | |
| } | |
| } | |
| guard !sessionName.isEmpty else { return nil } | |
| for context in mainWindowContexts.values { | |
| if let workspace = context.tabManager.tabs.first(where: { | |
| $0.realTmuxSessionId != nil && $0.title == sessionName | |
| }) { | |
| return workspace | |
| } | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/AppDelegate.swift` around lines 3917 - 3926, The matching currently
uses "realTmuxSessionId == sessionId || title == sessionName" which returns
wrong workspaces when titles collide; update
mainWindowWorkspaceForRealTmuxSession to first search all contexts'
tabManager.tabs for a workspace where realTmuxSessionId == sessionId and return
it if found, and only if no ID match exists perform a second search that matches
title == sessionName but restricts that fallback to tmux workspaces (e.g., where
realTmuxSessionId != nil or an isTmux flag) to avoid matching non-tmux
workspaces; reference mainWindowWorkspaceForRealTmuxSession, mainWindowContexts,
tabManager.tabs, realTmuxSessionId and title when making the change.
Comment on lines
+7083
to
+7085
| if shouldBlockRealTmuxShortcut(event, surface: surface) { | ||
| return | ||
| } |
There was a problem hiding this comment.
Don't swallow Ctrl+B in real tmux workspaces.
This turns tmux's default prefix into a no-op for imported sessions, so users can't type core tmux commands from the keyboard. It also leaves keyUp sending a release without a matching press. If this key needs special handling, route it through the tmux input path instead of returning early here.
💡 Minimal fix
- if shouldBlockRealTmuxShortcut(event, surface: surface) {
- return
- }- private func shouldBlockRealTmuxShortcut(_ event: NSEvent, surface: ghostty_surface_t) -> Bool {
- let flags = event.modifierFlags.intersection(.deviceIndependentFlagsMask)
- guard flags.contains(.control),
- !flags.contains(.command),
- !flags.contains(.option),
- !flags.contains(.shift),
- event.keyCode == kVK_ANSI_B,
- let located = AppDelegate.shared?.locateGhosttySurface(surface),
- let workspace = located.tabManager.tabs.first(where: { $0.id == located.workspaceId }),
- workspace.realTmuxSessionId != nil else {
- return false
- }
-#if DEBUG
- cmuxDebugLog(
- "realTmux.key.block workspace=\(workspace.id.uuidString.prefix(5)) " +
- "session=\(workspace.realTmuxSessionName ?? workspace.realTmuxSessionId ?? "") key=ctrl-b"
- )
-#endif
- return true
- }Also applies to: 7461-7480
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/GhosttyTerminalView.swift` around lines 7083 - 7085, The current
early return when shouldBlockRealTmuxShortcut(event, surface: surface) is true
swallows Ctrl+B (tmux prefix) and creates unmatched keyUp events; instead of
returning early in the key handler (where shouldBlockRealTmuxShortcut is
called), detect the Ctrl+B case and forward the event into the tmux input path
so tmux receives both press and release (e.g., call the existing tmux
input/forwarding function used elsewhere) and only suppress local handling for
non-tmux shortcuts; update both keyDown and keyUp paths to use
shouldBlockRealTmuxShortcut and the tmux-forwarding function so events are
consistently routed rather than dropped.
Comment on lines
+14644
to
+14648
| if let id = tabManager.newSplit(tabId: tabId, surfaceId: focusedPanelId, direction: direction, focus: focus) { | ||
| result = "OK \(id.uuidString) [tmux]" | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
Missing error handling when newSplit fails.
If tabManager.newSplit returns nil, the function returns without setting result, leaving it undefined/default. The V2 equivalent (hunk 6) correctly handles this with an internal_error response.
🐛 Proposed fix
if let id = tabManager.newSplit(tabId: tabId, surfaceId: focusedPanelId, direction: direction, focus: focus) {
result = "OK \(id.uuidString) [tmux]"
+ } else {
+ result = "ERROR: Failed to split real tmux pane"
}
return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TerminalController.swift` around lines 14644 - 14648, The code path
where tabManager.newSplit(...) returns nil lacks error handling and leaves
result unset; update the branch so that when newSplit returns nil you set result
to an internal error string (matching the V2 pattern, e.g. "internal_error
new_split_failed") before returning; locate the call to
tabManager.newSplit(tabId: tabId, surfaceId: focusedPanelId, direction:
direction, focus: focus) and ensure the else path sets result appropriately and
returns.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (10)
Sources/GhosttyTerminalView.swift (1)
7083-7085:⚠️ Potential issue | 🔴 CriticalDon’t swallow
Ctrl+Bin real tmux workspaces.Line 7083 drops the key entirely for real tmux sessions, so tmux never receives its default prefix. It also leaves key handling asymmetric because keyUp still goes through normal release routing.
Minimal safe fix
- if shouldBlockRealTmuxShortcut(event, surface: surface) { - return - }- private func shouldBlockRealTmuxShortcut(_ event: NSEvent, surface: ghostty_surface_t) -> Bool { - let flags = event.modifierFlags.intersection(.deviceIndependentFlagsMask) - guard flags.contains(.control), - !flags.contains(.command), - !flags.contains(.option), - !flags.contains(.shift), - event.keyCode == kVK_ANSI_B, - let located = AppDelegate.shared?.locateGhosttySurface(surface), - let workspace = located.tabManager.tabs.first(where: { $0.id == located.workspaceId }), - workspace.realTmuxSessionId != nil else { - return false - } -#if DEBUG - cmuxDebugLog( - "realTmux.key.block workspace=\(workspace.id.uuidString.prefix(5)) " + - "session=\(workspace.realTmuxSessionName ?? workspace.realTmuxSessionId ?? "") key=ctrl-b" - ) -#endif - return true - }Also applies to: 7461-7480
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 7083 - 7085, The current early-return when shouldBlockRealTmuxShortcut(event, surface: surface) is true swallows Ctrl+B for real tmux workspaces and breaks symmetry with keyUp; instead of returning, allow the event to be forwarded to normal input handling so tmux receives the prefix and keyUp is routed the same way—replace the bare return in the keyDown handling with code that forwards the event (e.g., call the same path used for non-blocked events or surface.handleEvent/nextResponder) only block events for the synthetic/unsupported cases that should genuinely be dropped; ensure the same gating is applied in keyUp handling so press/release are consistent.Sources/TabManager.swift (6)
5707-5714:⚠️ Potential issue | 🟠 Major
newSurface(initialInput:)still loses the startup input on tmux-backed paths.The tmux split path creates a pane, but it still has no way to carry the caller's
initialInput, so commands that rely on startup text behave differently after tmux import.Also applies to: 5877-5893
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TabManager.swift` around lines 5707 - 5714, The newSurface(initialInput:) path for tmux-backed workspaces discards the caller's initialInput when it calls createSplit(tabId:surfaceId:direction:), so startup text is lost; modify the split flow to propagate and apply the initialInput: add an initialInput parameter to createSplit (or otherwise pass/store the text via a pendingInputs map keyed by the created pane/surface id), ensure the tmux split code uses that value to send the input after the proxy/attachment completes (or inject it when the backing pane is ready), and update callers (including newSurface and any other call sites) to pass through the initialInput so tmux-backed panes receive the startup command.
4941-4944:⚠️ Potential issue | 🟠 MajorGate tmux close routing on the addressed panel, not the whole workspace.
A real-tmux workspace can still contain non-terminal panels. Routing every close through
closeRealTmuxPanel(...)makes browser-panel closes no-op because there is norealTmuxPaneIdto kill.Also applies to: 6083-6085
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TabManager.swift` around lines 4941 - 4944, The current check uses tab.isRealTmuxWorkspace to route every close through closeRealTmuxPanel, which incorrectly no-ops for non-terminal panels; instead gate on the addressed panel's tmux identity (e.g., check the panel object for realTmuxPaneId or panel.isRealTmuxPanel) and only call closeRealTmuxPanel(tab:tab, panelId:panelId) when that specific panel has a realTmuxPaneId; otherwise perform the normal non-tmux close path. Apply the same change to the duplicate occurrence around the other snippet (the block at the 6083-6085 location).
1259-1264:⚠️ Potential issue | 🟠 MajorDon't adopt a workspace just because its title matches the tmux session name.
The
title == session.namefallback can still silently convert a normal local workspace into a real-tmux workspace when names collide.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TabManager.swift` around lines 1259 - 1264, The code is incorrectly adopting workspaces by matching tmux session.name against workspace.title; remove the title-based fallback so we only treat a workspace as a real-tmux mapping when AppDelegate.shared?.mainWindowWorkspaceForRealTmuxSession(id:name:) returns a workspace or when a tab already has realTmuxSessionId == session.id. Update the logic in the loop over realSessions that sets store.sessionIdToWorkspaceId[session.id] (references: realSessions, store.sessionIdToWorkspaceId, AppDelegate.shared?.mainWindowWorkspaceForRealTmuxSession(id:name:), tabs, realTmuxSessionId, title) to drop the `title == session.name` check and only use the explicit real-tmux identifiers.
1246-1251:⚠️ Potential issue | 🟠 MajorSync cleanup still can't remove the last workspace in a window.
When a backing tmux session disappears and this is the only workspace in the window,
closeWorkspace(...)returns immediately, so the stale real-tmux workspace stays open forever.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TabManager.swift` around lines 1246 - 1251, The cleanup loop over store.sessionIdToWorkspaceId skips closing a workspace when it's the last in its window because closeWorkspace(...) currently early-returns for last-workspace cases; change closeWorkspace (or add a parameter like forceCloseWhenBackingMissing) so the call from the sync-cleanup loop can force-close a workspace even if it's the final tab in its window when the backing tmux session is gone: detect the missing backing session in the loop (using liveSessionIds) and call closeWorkspace(workspace, killRealTmuxSession: false, force: true) or equivalent, and update closeWorkspace to honor that force flag by bypassing the "last workspace" early-return when force is true.
1269-1283:⚠️ Potential issue | 🟠 MajorAvoid importing a real-tmux workspace without an initial pane id.
This still sets
realTmuxSessionId/realTmuxSessionNameeven when no proxy command is available, so later pane-scoped operations have norealTmuxPaneIdto target.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TabManager.swift` around lines 1269 - 1283, The code sets realTmuxSessionId and realTmuxSessionName on the workspace even if initialProxyCommand is nil, which means no valid initial pane id exists and later pane-related operations will lack a target. To fix this in the block where you add a workspace for a real-tmux session, conditionally assign realTmuxSessionId and realTmuxSessionName only if initialProxyCommand is non-nil so that only workspaces with valid pane proxies are marked as real-tmux sessions.
5516-5520:⚠️ Potential issue | 🟠 MajorExplicit surface-focus requests are still dropped in real-tmux workspaces.
Returning early here means callers cannot focus a specific tmux-backed pane via
focusSurface(...); this should select the tmux pane and still update local panel focus.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TabManager.swift` around lines 5516 - 5520, The early return in focusSurface(tabId:surfaceId:) when tab.isRealTmuxWorkspace drops explicit focus requests; remove the return and instead, for real-tmux tabs, invoke the tmux-pane selection logic (the same code path used elsewhere to select a tmux-backed pane) and then proceed to update the local panel focus/state just like non-tmux tabs so callers can both select the tmux pane and update local UI focus; locate focusSurface and replace the guard/return behavior for tab.isRealTmuxWorkspace with a branch that triggers the tmux selection call and continues to the common focus-update steps.Sources/RightSidebarPanelView.swift (3)
569-576:⚠️ Potential issue | 🟠 MajorCross-window lookup is still missing.
applyRefreshbuildsliveWorkspaceIdsand searches for existing workspaces using only the localtabManager.tabs. If the tmux sidebar is opened in a second window, it will drop mappings for sessions whose workspaces live in the first window, then recreate duplicates on the next refresh.Per retrieved learnings, workspace resolution should search across all
mainWindowContexts(e.g., viaAppDelegate.shared?.workspaceFor(tabId:)or aggregating tabs from all contexts) to avoid active-window bias.Also applies to: 598-598
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/RightSidebarPanelView.swift` around lines 569 - 576, applyRefresh currently builds liveWorkspaceIds and matches sessions only against the local tabManager.tabs which drops mappings for workspaces in other app windows; update applyRefresh to resolve workspaces across all windows by querying global contexts (e.g., use AppDelegate.shared?.workspaceFor(tabId:) or aggregate tabs from all mainWindowContexts) when constructing liveWorkspaceIds and when doing the lookup inside the for session in realSessions loop so store.sessionIdToWorkspaceId retains mappings to workspaces in other windows (ensure you reference liveWorkspaceIds, tabManager.tabs, store.sessionIdToWorkspaceId, and realSessions in the updated logic).
512-516:⚠️ Potential issue | 🟠 Major
killRealTmuxSessionstill ignores termination status before closing workspace.The
onKillclosure unconditionally closes the mapped workspace after callingkillRealTmuxSession, but the function discards the process exit code. Iftmux kill-sessionfails (e.g., session already gone, permission error), the tmux session may remain alive while cmux removes its workspace, desynchronizing state.Return a success indicator and gate the workspace close on it:
🔧 Proposed fix
- private func killRealTmuxSession(_ sessionName: String) { + `@discardableResult` + private func killRealTmuxSession(_ sessionName: String) -> Bool { guard let tmuxPath = realTmuxExecutablePath() else { return } let process = Process() process.executableURL = URL(fileURLWithPath: tmuxPath) process.arguments = ["kill-session", "-t", sessionName] process.standardOutput = Pipe() process.standardError = Pipe() - try? process.run() - process.waitUntilExit() + do { + try process.run() + process.waitUntilExit() + return process.terminationStatus == 0 + } catch { + return false + } }Then in
onKill:onKill: { - killRealTmuxSession(session.name) - if let workspace = tabManager.tabs.first(where: { $0.id == session.workspaceId }) { - _ = tabManager.closeWorkspaceWithConfirmation(workspace) + if killRealTmuxSession(session.name), + let workspace = tabManager.tabs.first(where: { $0.id == session.workspaceId }) { + tabManager.closeWorkspaceWithConfirmation(workspace) } }Also applies to: 745-754
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/RightSidebarPanelView.swift` around lines 512 - 516, The onKill closure calls killRealTmuxSession(session.name) but ignores its exit status, so change killRealTmuxSession to return a Bool (or Result) indicating success/failure (or the process exit code) and update all callers (the onKill closure here and the similar block around lines 745-754) to gate closing the mapped workspace: call killRealTmuxSession(session.name), check the returned success flag (or exit code == 0) before calling tabManager.closeWorkspaceWithConfirmation(workspace), and log/handle failures instead of unconditionally closing the workspace (referencing symbols: killRealTmuxSession, onKill closure, session.name, session.workspaceId, tabManager.closeWorkspaceWithConfirmation, tabManager.tabs).
563-622:⚠️ Potential issue | 🟡 MinorStore I/O (
load/save) still runs on the main thread.
applyRefreshis dispatched toDispatchQueue.main.async(line 557-558), soloadRealTmuxStore()andsaveRealTmuxStore(_:)perform file reads/writes on the main thread. This can stall the sidebar when the disk is slow or the JSON grows.Move the store load into the background task alongside the tmux calls, and write the store asynchronously after applying UI changes (or batch it with the next refresh).
Also applies to: 724-739
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/RightSidebarPanelView.swift` around lines 563 - 622, applyRefresh currently calls loadRealTmuxStore() and saveRealTmuxStore(_:) on the main thread (applyRefresh is dispatched via DispatchQueue.main.async), which can block the UI; move the store I/O off the main thread by loading the store earlier in the background task that fetches tmux data (so replace direct calls to loadRealTmuxStore() in applyRefresh with a store parameter or a preloaded variable), perform all mutation of UI models (workspace updates, sessions array, isRefreshing) on the main thread as you already do, and persist the updated store asynchronously after UI updates (call saveRealTmuxStore(_) from a background queue or batch it with the next refresh); update references to functions/vars: applyRefresh, loadRealTmuxStore, saveRealTmuxStore, tabManager, sessions, and ensure workspace.realTmuxSessionId/name assignments and workspace.setCustomTitle(...) remain on the main thread while file I/O is moved off.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/RightSidebarPanelView.swift`:
- Around line 745-754: The killRealTmuxSession function runs a Process
synchronously and calls process.waitUntilExit(), which blocks the main thread;
update it to run the Process asynchronously (e.g., create and run the Process on
a background queue or use Process.terminationHandler) and remove the blocking
waitUntilExit(); after the Process finishes invoke UI-related follow-up on the
main thread (DispatchQueue.main.async) so callers from the button tap (which use
killRealTmuxSession) no longer block the UI; reference the existing function
killRealTmuxSession and the helper realTmuxExecutablePath() when making the
change.
In `@Sources/TabManager.swift`:
- Around line 1243-1257: The current filter on store.sessionIdToWorkspaceId
wrongly drops mappings for workspaces not present in this TabManager
(refreshedWorkspaceIds), breaking cross-window tmux mappings; update the
filtering logic in the TabManager code that touches store.sessionIdToWorkspaceId
so you only remove entries whose sessionId is no longer live (use
liveSessionIds.contains($0.key)) and do NOT require the workspace id to be in
this TabManager's tabs/refreshedWorkspaceIds; keep the existing
closeWorkspace(session cleanup) behavior (closeWorkspace and liveSessionIds
logic) but remove the refreshedWorkspaceIds.contains($0.value) condition so
mappings for other windows are preserved.
---
Duplicate comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 7083-7085: The current early-return when
shouldBlockRealTmuxShortcut(event, surface: surface) is true swallows Ctrl+B for
real tmux workspaces and breaks symmetry with keyUp; instead of returning, allow
the event to be forwarded to normal input handling so tmux receives the prefix
and keyUp is routed the same way—replace the bare return in the keyDown handling
with code that forwards the event (e.g., call the same path used for non-blocked
events or surface.handleEvent/nextResponder) only block events for the
synthetic/unsupported cases that should genuinely be dropped; ensure the same
gating is applied in keyUp handling so press/release are consistent.
In `@Sources/RightSidebarPanelView.swift`:
- Around line 569-576: applyRefresh currently builds liveWorkspaceIds and
matches sessions only against the local tabManager.tabs which drops mappings for
workspaces in other app windows; update applyRefresh to resolve workspaces
across all windows by querying global contexts (e.g., use
AppDelegate.shared?.workspaceFor(tabId:) or aggregate tabs from all
mainWindowContexts) when constructing liveWorkspaceIds and when doing the lookup
inside the for session in realSessions loop so store.sessionIdToWorkspaceId
retains mappings to workspaces in other windows (ensure you reference
liveWorkspaceIds, tabManager.tabs, store.sessionIdToWorkspaceId, and
realSessions in the updated logic).
- Around line 512-516: The onKill closure calls
killRealTmuxSession(session.name) but ignores its exit status, so change
killRealTmuxSession to return a Bool (or Result) indicating success/failure (or
the process exit code) and update all callers (the onKill closure here and the
similar block around lines 745-754) to gate closing the mapped workspace: call
killRealTmuxSession(session.name), check the returned success flag (or exit code
== 0) before calling tabManager.closeWorkspaceWithConfirmation(workspace), and
log/handle failures instead of unconditionally closing the workspace
(referencing symbols: killRealTmuxSession, onKill closure, session.name,
session.workspaceId, tabManager.closeWorkspaceWithConfirmation,
tabManager.tabs).
- Around line 563-622: applyRefresh currently calls loadRealTmuxStore() and
saveRealTmuxStore(_:) on the main thread (applyRefresh is dispatched via
DispatchQueue.main.async), which can block the UI; move the store I/O off the
main thread by loading the store earlier in the background task that fetches
tmux data (so replace direct calls to loadRealTmuxStore() in applyRefresh with a
store parameter or a preloaded variable), perform all mutation of UI models
(workspace updates, sessions array, isRefreshing) on the main thread as you
already do, and persist the updated store asynchronously after UI updates (call
saveRealTmuxStore(_) from a background queue or batch it with the next refresh);
update references to functions/vars: applyRefresh, loadRealTmuxStore,
saveRealTmuxStore, tabManager, sessions, and ensure
workspace.realTmuxSessionId/name assignments and workspace.setCustomTitle(...)
remain on the main thread while file I/O is moved off.
In `@Sources/TabManager.swift`:
- Around line 5707-5714: The newSurface(initialInput:) path for tmux-backed
workspaces discards the caller's initialInput when it calls
createSplit(tabId:surfaceId:direction:), so startup text is lost; modify the
split flow to propagate and apply the initialInput: add an initialInput
parameter to createSplit (or otherwise pass/store the text via a pendingInputs
map keyed by the created pane/surface id), ensure the tmux split code uses that
value to send the input after the proxy/attachment completes (or inject it when
the backing pane is ready), and update callers (including newSurface and any
other call sites) to pass through the initialInput so tmux-backed panes receive
the startup command.
- Around line 4941-4944: The current check uses tab.isRealTmuxWorkspace to route
every close through closeRealTmuxPanel, which incorrectly no-ops for
non-terminal panels; instead gate on the addressed panel's tmux identity (e.g.,
check the panel object for realTmuxPaneId or panel.isRealTmuxPanel) and only
call closeRealTmuxPanel(tab:tab, panelId:panelId) when that specific panel has a
realTmuxPaneId; otherwise perform the normal non-tmux close path. Apply the same
change to the duplicate occurrence around the other snippet (the block at the
6083-6085 location).
- Around line 1259-1264: The code is incorrectly adopting workspaces by matching
tmux session.name against workspace.title; remove the title-based fallback so we
only treat a workspace as a real-tmux mapping when
AppDelegate.shared?.mainWindowWorkspaceForRealTmuxSession(id:name:) returns a
workspace or when a tab already has realTmuxSessionId == session.id. Update the
logic in the loop over realSessions that sets
store.sessionIdToWorkspaceId[session.id] (references: realSessions,
store.sessionIdToWorkspaceId,
AppDelegate.shared?.mainWindowWorkspaceForRealTmuxSession(id:name:), tabs,
realTmuxSessionId, title) to drop the `title == session.name` check and only use
the explicit real-tmux identifiers.
- Around line 1246-1251: The cleanup loop over store.sessionIdToWorkspaceId
skips closing a workspace when it's the last in its window because
closeWorkspace(...) currently early-returns for last-workspace cases; change
closeWorkspace (or add a parameter like forceCloseWhenBackingMissing) so the
call from the sync-cleanup loop can force-close a workspace even if it's the
final tab in its window when the backing tmux session is gone: detect the
missing backing session in the loop (using liveSessionIds) and call
closeWorkspace(workspace, killRealTmuxSession: false, force: true) or
equivalent, and update closeWorkspace to honor that force flag by bypassing the
"last workspace" early-return when force is true.
- Around line 1269-1283: The code sets realTmuxSessionId and realTmuxSessionName
on the workspace even if initialProxyCommand is nil, which means no valid
initial pane id exists and later pane-related operations will lack a target. To
fix this in the block where you add a workspace for a real-tmux session,
conditionally assign realTmuxSessionId and realTmuxSessionName only if
initialProxyCommand is non-nil so that only workspaces with valid pane proxies
are marked as real-tmux sessions.
- Around line 5516-5520: The early return in focusSurface(tabId:surfaceId:) when
tab.isRealTmuxWorkspace drops explicit focus requests; remove the return and
instead, for real-tmux tabs, invoke the tmux-pane selection logic (the same code
path used elsewhere to select a tmux-backed pane) and then proceed to update the
local panel focus/state just like non-tmux tabs so callers can both select the
tmux pane and update local UI focus; locate focusSurface and replace the
guard/return behavior for tab.isRealTmuxWorkspace with a branch that triggers
the tmux selection call and continues to the common focus-update steps.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: da7edd4f-b65d-4068-acf1-286282d1595d
📒 Files selected for processing (3)
Sources/GhosttyTerminalView.swiftSources/RightSidebarPanelView.swiftSources/TabManager.swift
There was a problem hiding this comment.
♻️ Duplicate comments (16)
Sources/GhosttyTerminalView.swift (1)
7191-7193:⚠️ Potential issue | 🔴 CriticalDon’t swallow
Ctrl+Bin real tmux workspaces.On Line 7192, returning early drops the tmux prefix key press.
keyUpstill follows the normal release path, so real tmux panes lose prefix behavior and key transitions become asymmetric. Route this case to the tmux forwarding path instead of returning early (or remove this block).💡 Minimal fix
- if shouldBlockRealTmuxShortcut(event, surface: surface) { - return - } + // Do not swallow Ctrl+B for real tmux; let normal input path deliver it.Also applies to: 7569-7588
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 7191 - 7193, The early return in the key handling branch that checks shouldBlockRealTmuxShortcut(event, surface: surface) swallows the tmux prefix (Ctrl+B) on keyDown and breaks symmetry with keyUp; instead of returning, route this case into the existing tmux forwarding path (the same flow used for forwarding other tmux events) so the prefix is forwarded to the real tmux workspace and keyUp behavior remains symmetrical—replace the return with a call to the tmux-forwarding handler used elsewhere (or remove the block so execution falls through to forwarding), keeping references to shouldBlockRealTmuxShortcut, the current event, surface, and keyUp handling.Sources/AppDelegate.swift (1)
3918-3927:⚠️ Potential issue | 🟠 MajorAvoid broad title fallback for tmux workspace resolution.
On Line 3921, matching
title == sessionNamecan select the wrong workspace when titles collide (including non-tmux workspaces), which can misroute tmux pane actions.Proposed fix
func mainWindowWorkspaceForRealTmuxSession(id sessionId: String, name sessionName: String) -> Workspace? { for context in mainWindowContexts.values { - if let workspace = context.tabManager.tabs.first(where: { - $0.realTmuxSessionId == sessionId || $0.title == sessionName - }) { + if let workspace = context.tabManager.tabs.first(where: { + $0.realTmuxSessionId == sessionId + }) { return workspace } } + + guard !sessionName.isEmpty else { return nil } + for context in mainWindowContexts.values { + if let workspace = context.tabManager.tabs.first(where: { + $0.isRealTmuxWorkspace && $0.title == sessionName + }) { + return workspace + } + } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 3918 - 3927, The current mainWindowWorkspaceForRealTmuxSession function is matching workspaces by title (title == sessionName) which can pick non-tmux workspaces; change the predicate so it only matches real tmux sessions by realTmuxSessionId. Either remove the title fallback entirely from the closure in context.tabManager.tabs.first(where:) and only compare $0.realTmuxSessionId == sessionId, or if you must keep a title-based fallback, restrict it to workspaces that are already tmux-bound (e.g. only allow $0.title == sessionName when $0.realTmuxSessionId != nil or another tmux-marker is present) so title collisions won't select non-tmux workspaces.Sources/RightSidebarPanelView.swift (4)
572-579:⚠️ Potential issue | 🟠 MajorThe global tmux mapping still has active-window bias.
realTmuxStoreURLis shared across the app, but pruning and existing-workspace resolution only inspecttabManager.tabsfrom the current window. Opening this sidebar in a second window can drop mappings for tmux workspaces hosted in the first window and recreate duplicates locally on the next refresh. Based on learnings, workspace resolution in this repo should search across all main-window contexts rather than the active window to avoid active-window bias.Also applies to: 597-601
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/RightSidebarPanelView.swift` around lines 572 - 579, The prune-and-resolve logic uses tabManager.tabs (active window only) causing active-window bias; change it to aggregate tabs from all main-window contexts before computing liveWorkspaceIds and when resolving existing workspaces for realSessions. Replace uses of tabManager.tabs with a collectedTabs array built by querying the global window registry/main controllers (e.g., allWindowControllers.flatMap { $0.tabs }) and use collectedTabs in the Set creation and in the first(where:) lookups; apply the same change to the other block referenced (lines 597-601) so pruning and resolution search across all windows.
673-676:⚠️ Potential issue | 🟠 MajorSearch
PATHbefore falling back to fixed tmux locations.This will miss valid installs from MacPorts, Nix, and custom PATH setups, so the tmux panel can look empty even though
tmuxis available in the user's environment.#!/bin/bash rg -n -C2 'realTmuxExecutablePath|/opt/homebrew/bin/tmux|/usr/local/bin/tmux|/usr/bin/tmux' Sources/RightSidebarPanelView.swift🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/RightSidebarPanelView.swift` around lines 673 - 676, realTmuxExecutablePath currently only checks a hardcoded list of locations and misses tmux installs on MacPorts, Nix or custom PATHs; update realTmuxExecutablePath to first search the user PATH (use ProcessInfo.processInfo.environment["PATH"] split on ":" and check each candidate with FileManager.default.isExecutableFile(atPath:)) and return the first match, and only if that fails fall back to the existing hardcoded array ["/opt/homebrew/bin/tmux", "/usr/local/bin/tmux", "/usr/bin/tmux"]; keep the function nonisolated and preserve the FileManager checks so callers of realTmuxExecutablePath see the same return semantics.
515-519:⚠️ Potential issue | 🟠 MajorOnly close the workspace after
kill-sessionsucceeds, and don't block the button tap.
onKillremoves the mapped workspace unconditionally, whilekillRealTmuxSessionignoresterminationStatusand waits synchronously. If tmux rejects the target, cmux still drops the workspace; if tmux is slow, the UI blocks until the child exits. Return a success flag asynchronously and only close after a confirmed kill (or after a refresh proves the session is gone).#!/bin/bash rg -n -C2 'onKill:|killRealTmuxSession|waitUntilExit|closeWorkspaceWithConfirmation' Sources/RightSidebarPanelView.swiftAlso applies to: 748-756
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/RightSidebarPanelView.swift` around lines 515 - 519, The onKill handler currently calls killRealTmuxSession(session.name) synchronously and immediately closes the mapped workspace via tabManager.closeWorkspaceWithConfirmation(workspace); change killRealTmuxSession to an asynchronous API that returns a success flag (via completion handler, async/await, or Result) and do not await it on the main thread; instead, kick off the kill asynchronously and only call tabManager.closeWorkspaceWithConfirmation(workspace) after the kill completes successfully (or after a follow-up refresh confirms the tmux session no longer exists); update both occurrences (the onKill closure around killRealTmuxSession(session.name) and the similar block at the later lines) to use the new async success check so the UI button tap is non-blocking and the workspace is only removed on confirmed kill.
560-562:⚠️ Potential issue | 🟠 MajorKeep the tmux-store JSON I/O off the main queue.
The tmux polling moved off-main, but
loadRealTmuxStore()andsaveRealTmuxStore()still run after the main-queue hop insideapplyRefresh. That leaves disk I/O in the 1.5s refresh path and can still hitch the sidebar when the home directory is slow.Also applies to: 566-618, 727-741
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/RightSidebarPanelView.swift` around lines 560 - 562, The tmux JSON disk I/O (calls to loadRealTmuxStore() and saveRealTmuxStore()) must be moved off the main queue: perform those calls on a background queue (e.g., DispatchQueue.global(qos:.utility).async) and only hop back to DispatchQueue.main.async to call UI-updating code such as applyRefresh(...) or to commit results to the view; alternatively, have applyRefresh accept the already-loaded tmux store/result and call saveRealTmuxStore from a background queue before/after UI updates. Ensure you capture any returned data/error from loadRealTmuxStore() on the background thread and pass it into applyRefresh on the main thread, and do not call loadRealTmuxStore() or saveRealTmuxStore() directly from inside DispatchQueue.main.async.Sources/Workspace.swift (2)
7206-7210:⚠️ Potential issue | 🟠 MajorPersist the real-tmux identity, or explicitly opt these workspaces out of restore.
These fields are now part of the workspace model, but Lines 321-336 still serialize only the generic
SessionWorkspaceSnapshot, and Lines 543-557 / 694-779 still omit per-panelrealTmuxPaneIdrehydration. After a relaunch, a real-tmux workspace will come back as plain local terminals with no backing-pane routing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 7206 - 7210, The snapshot serialization/deserialization needs to include the real-Tmux identity and per-panel pane IDs: update the code paths that create and restore SessionWorkspaceSnapshot to persist realTmuxSessionId and realTmuxSessionName when isRealTmuxWorkspace is true, and update the workspace rehydration logic (the code that reconstructs panels/TerminalPanel state) to read and set each panel's realTmuxPaneId during restore; ensure the snapshot model and the restore function that iterates panels both handle these new fields so real-tmux workspaces are rehydrated with backing-pane routing instead of falling back to local terminals.
9875-9888:⚠️ Potential issue | 🟠 Major
newTerminalSurface(...)still bypasses the real-tmux invariant.This guard only covers split creation. Line 12244 still goes through
createTerminalToRight(...) -> newTerminalSurface(...), and Line 13709 keeps that context-menu path reachable, so a real-tmux workspace can still end up with a native cmux terminal that has no backing tmux pane.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 9875 - 9888, The newTerminalSurface(...) function (and any code paths that call it like createTerminalToRight(...) and the context-menu creation path) is still allowed to create a native cmux terminal in a real-tmux workspace; add the same real-tmux invariant check at the top of newTerminalSurface(...) (or make a single helper guard) so it returns nil unless allowInRealTmuxWorkspace is true, and update callers (createTerminalToRight and the context-menu terminal creation) to either pass allowInRealTmuxWorkspace when appropriate or stop calling newTerminalSurface in real-tmux workspaces; reference newTerminalSurface(...), createTerminalToRight(...), and the context-menu creation path when making the change.Sources/TabManager.swift (8)
4941-4944:⚠️ Potential issue | 🟠 MajorGate tmux close routing on the panel, not the whole workspace.
Browser or other non-terminal panels inside a real-tmux workspace do not have a
realTmuxPaneId, so these branches turn close into a no-op whencloseRealTmuxPanel(...)returnsfalse. Checktab.terminalPanel(for: panelId)?.realTmuxPaneIdfirst and only route those panels through tmux.Also applies to: 6096-6098
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TabManager.swift` around lines 4941 - 4944, Currently the code unconditionally routes all panels in a real tmux workspace through closeRealTmuxPanel by checking tab.isRealTmuxWorkspace, which makes non-terminal/browser panels (which lack a realTmuxPaneId) become no-ops when closeRealTmuxPanel returns false; change the gate to check the specific panel’s realTmuxPaneId by calling tab.terminalPanel(for: panelId)?.realTmuxPaneId and only call closeRealTmuxPanel(tab: panelId:) when that optional pane id exists. Apply the same fix to the other occurrence around the code near the 6096–6098 region so only true tmux terminal panels are routed to closeRealTmuxPanel.
5720-5727:⚠️ Potential issue | 🟠 Major
newSurface(initialInput:)still drops the startup input in real-tmux workspaces.The tmux branch creates the split but never forwards
initialInput, so commands that rely on startup text behave differently after tmux import. Thread the input through the tmux split path or send it to the newly created pane immediately after creation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TabManager.swift` around lines 5720 - 5727, In newSurface(initialInput:), when workspace.isRealTmuxWorkspace is true the code currently calls createSplit(tabId:surfaceId:direction:) and returns without forwarding initialInput; modify the tmux branch to either (a) extend createSplit to accept an optional initialInput parameter and pass initialInput through, or (b) capture the created pane/surface id returned by createSplit and immediately call the existing input-forwarding API (e.g. sendInputToSurface / writeToPane / whatever central method your codebase uses) to write initialInput into the new surface; ensure you reference selectedWorkspace, workspace.focusedPanelId and createSplit in the change so the startup input is not dropped.
1238-1242:⚠️ Potential issue | 🟠 MajorReal-tmux sync still does store I/O on the main actor.
The tmux probing moved off-main, but
applyRealTmuxSessionSync()still synchronously loads and saves the JSON store on the main actor every pass. Slow home-directory I/O will still hitch UI/typing here; decode/encode the store in the background phase and only apply the already-loaded result on main.Also applies to: 1304-1304
1269-1283:⚠️ Potential issue | 🟠 MajorDon't import as real-tmux unless the initial pane proxy exists.
This branch still marks the workspace as real-tmux when
currentPaneId/realTmuxPaneProxyCommand(...)is unavailable. The initial terminal then has norealTmuxPaneId, but later pane-scoped operations still route through tmux, so the imported workspace becomes partially unusable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TabManager.swift` around lines 1269 - 1283, The code marks workspaces as real-tmux even when there is no initial pane proxy; change it so workspace.realTmuxSessionId, workspace.realTmuxSessionName and store.sessionIdToWorkspaceId[session.id] are only set when initialProxyCommand (computed via Self.realTmuxPaneProxyCommand from initialPaneId) is non-nil. Leave addWorkspace call as-is but wrap the assignments to workspace.realTmuxSessionId / realTmuxSessionName and the store mapping in a conditional that checks initialProxyCommand != nil so imports without a proxy are not treated as real-tmux.
5529-5534:⚠️ Potential issue | 🟠 MajorExplicit surface-focus requests still no-op in tmux-backed workspaces.
This drops callers that intentionally target a specific pane, so
focusTab(..., surfaceId:)cannot focus a tmux-backed surface after import. Route the request throughselectRealTmuxPane(...)while keeping the local focused panel in sync. Based on learnings: Socket/CLI commands must not steal macOS app focus (no app activation/window raising side effects); only explicit focus-intent commands may mutate in-app focus/selection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TabManager.swift` around lines 5529 - 5534, The focusSurface(tabId:surfaceId:) currently returns early for tmux-backed workspaces and thus ignores explicit surface-focus requests; change it so that when tab.isRealTmuxWorkspace is true you call selectRealTmuxPane(tabId: , surfaceId: ) to route the focus request to tmux and then update the local selection (e.g., call tab.focusPanel(surfaceId) or otherwise sync the in-memory focused panel) so the UI state reflects the change; ensure selectRealTmuxPane and this flow do not trigger macOS app activation/window raising (only mutate in-app selection), and keep focusTab(..., surfaceId:) callers functional after import.
1246-1251:⚠️ Potential issue | 🟠 MajorMissing-session cleanup still can't remove the only workspace in a window.
If the vanished tmux session maps to this window's last tab,
closeWorkspace(..., killRealTmuxSession: false)immediately returns becausetabs.count <= 1, so the stale imported workspace survives forever. This needs the same replacement-workspace fallback used incloseRealTmuxPanel(...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TabManager.swift` around lines 1246 - 1251, The cleanup loop that calls closeWorkspace(sessionIdToWorkspaceId ...) skips closing if tabs.count <= 1, leaving a stale imported workspace; change the logic in the for-loop that iterates (sessionId, workspaceId) to use the same replacement-workspace fallback as closeRealTmuxPanel: when the workspace to close is found but tabs.count <= 1, create/assign a replacement workspace (same behavior used by closeRealTmuxPanel) and then proceed to remove the stale mapping and call closeWorkspace(..., killRealTmuxSession: false) on the replacement or otherwise remove the workspace so the stale imported workspace is not left behind. Ensure you reference and reuse the same replacement creation/removal helpers used by closeRealTmuxPanel to keep behavior consistent.
1254-1257:⚠️ Potential issue | 🟠 MajorDon't prune tmux mappings for other windows.
This filter only keeps workspace ids from the current
TabManager, so a sync in one window can deletesessionIdToWorkspaceIdentries for live tmux workspaces owned by another window. Filter by live session ids only, or use the cross-window workspace-id set you already computed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TabManager.swift` around lines 1254 - 1257, The current filtering of store.sessionIdToWorkspaceId uses refreshedWorkspaceIds (Set(tabs.map { $0.id.uuidString })) which only contains workspace ids for this TabManager and causes removal of tmux mappings belonging to other windows; change the filter to only test live session membership (liveSessionIds.contains($0.key)) or use the previously computed cross-window workspace-id set instead of refreshedWorkspaceIds so you don't prune mappings for other windows—update the closure on store.sessionIdToWorkspaceId.filter to reference liveSessionIds (or the shared workspace-id set) rather than refreshedWorkspaceIds.
1259-1267:⚠️ Potential issue | 🟠 MajorAvoid adopting a local workspace by title match.
The
|| $0.title == session.namefallback can silently convert an unrelated local workspace into a real-tmux workspace when names collide, after which close/focus/split start routing through tmux. Matching should stay limited to persisted mappings or existingrealTmuxSessionId.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TabManager.swift` around lines 1259 - 1267, The code is incorrectly matching local workspaces by title (the "|| $0.title == session.name" clause) which can reassign unrelated workspaces to real tmux sessions; change the lookup in the realSessions loop so you only adopt an existing workspace if AppDelegate.shared?.mainWindowWorkspaceForRealTmuxSession(id:name:) returns one or if a tab already has a matching realTmuxSessionId (tabs.first(where: { $0.realTmuxSessionId == session.id })), removing the title-based fallback and ensuring you only update store.sessionIdToWorkspaceId, existing.realTmuxSessionId, and existing.realTmuxSessionName when a true persisted or realTmuxSessionId match is found.
🧹 Nitpick comments (3)
Sources/TerminalController.swift (3)
5471-5471: Consider cachingv2ResolveWindowIdresult.
v2ResolveWindowId(tabManager: tabManager)is called twice in the same expression. While unlikely to cause issues on the main thread, caching the result would be cleaner.+ let windowId = v2ResolveWindowId(tabManager: tabManager) result = closed - ? .ok(["workspace_id": ws.id.uuidString, "workspace_ref": v2Ref(kind: .workspace, uuid: ws.id), "surface_id": surfaceId.uuidString, "surface_ref": v2Ref(kind: .surface, uuid: surfaceId), "window_id": v2OrNull(v2ResolveWindowId(tabManager: tabManager)?.uuidString), "window_ref": v2Ref(kind: .window, uuid: v2ResolveWindowId(tabManager: tabManager))]) + ? .ok(["workspace_id": ws.id.uuidString, "workspace_ref": v2Ref(kind: .workspace, uuid: ws.id), "surface_id": surfaceId.uuidString, "surface_ref": v2Ref(kind: .surface, uuid: surfaceId), "window_id": v2OrNull(windowId?.uuidString), "window_ref": v2Ref(kind: .window, uuid: windowId)]) : .err(code: "internal_error", message: "Failed to close surface", data: ["surface_id": surfaceId.uuidString])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalController.swift` at line 5471, The expression calls v2ResolveWindowId(tabManager: tabManager) twice; compute it once into a local (e.g., let resolvedWindowId = v2ResolveWindowId(tabManager: tabManager)) and then use resolvedWindowId?.uuidString for the "window_id" value (wrapped with v2OrNull if needed) and pass resolvedWindowId to v2Ref(kind: .window, uuid: resolvedWindowId) for "window_ref" so the lookup is performed once and the returned value reused.
16453-16455: Same inconsistency with inline computation.Similar to hunk 4, this computes
isRealTmuxWorkspaceinline rather than using the property accessor. Consider usingtab.isRealTmuxWorkspacefor consistency with V2 split handlers.- let isRealTmuxWorkspace = tab.realTmuxSessionId != nil || tab.realTmuxSessionName != nil - // Don't close if it's the only surface, unless the surface represents a real tmux session. - if tab.panels.count <= 1 && !isRealTmuxWorkspace { + // Don't close if it's the only surface, unless the surface represents a real tmux session. + if tab.panels.count <= 1 && !tab.isRealTmuxWorkspace {And update line 16461:
- let closed = isRealTmuxWorkspace + let closed = tab.isRealTmuxWorkspace🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalController.swift` around lines 16453 - 16455, Replace the inline computation let isRealTmuxWorkspace = tab.realTmuxSessionId != nil || tab.realTmuxSessionName != nil with the property accessor tab.isRealTmuxWorkspace and update any subsequent conditional that uses isRealTmuxWorkspace (e.g. the if checking tab.panels.count <= 1 && !isRealTmuxWorkspace) to use tab.isRealTmuxWorkspace instead so the logic matches the V2 split handlers; ensure you remove the local variable and reference the existing tab.isRealTmuxWorkspace property wherever this inline check was used.
5460-5461: Use property accessor for consistency.Line 5460 computes
isRealTmuxWorkspaceinline withws.realTmuxSessionId != nil || ws.realTmuxSessionName != nil. Thews.isRealTmuxWorkspaceproperty accessor implements the same logic. Use the property accessor for consistency across the codebase:- let isRealTmuxWorkspace = ws.realTmuxSessionId != nil || ws.realTmuxSessionName != nil - if ws.panels.count <= 1 && !isRealTmuxWorkspace { + if ws.panels.count <= 1 && !ws.isRealTmuxWorkspace {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalController.swift` around lines 5460 - 5461, Replace the inline check that computes isRealTmuxWorkspace using ws.realTmuxSessionId != nil || ws.realTmuxSessionName != nil with the existing property accessor ws.isRealTmuxWorkspace so the condition uses the canonical implementation; update the variable usage in the block that currently defines let isRealTmuxWorkspace = ... and the subsequent if to reference ws.isRealTmuxWorkspace for consistency with the rest of the codebase.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/AppDelegate.swift`:
- Around line 3918-3927: The current mainWindowWorkspaceForRealTmuxSession
function is matching workspaces by title (title == sessionName) which can pick
non-tmux workspaces; change the predicate so it only matches real tmux sessions
by realTmuxSessionId. Either remove the title fallback entirely from the closure
in context.tabManager.tabs.first(where:) and only compare $0.realTmuxSessionId
== sessionId, or if you must keep a title-based fallback, restrict it to
workspaces that are already tmux-bound (e.g. only allow $0.title == sessionName
when $0.realTmuxSessionId != nil or another tmux-marker is present) so title
collisions won't select non-tmux workspaces.
In `@Sources/GhosttyTerminalView.swift`:
- Around line 7191-7193: The early return in the key handling branch that checks
shouldBlockRealTmuxShortcut(event, surface: surface) swallows the tmux prefix
(Ctrl+B) on keyDown and breaks symmetry with keyUp; instead of returning, route
this case into the existing tmux forwarding path (the same flow used for
forwarding other tmux events) so the prefix is forwarded to the real tmux
workspace and keyUp behavior remains symmetrical—replace the return with a call
to the tmux-forwarding handler used elsewhere (or remove the block so execution
falls through to forwarding), keeping references to shouldBlockRealTmuxShortcut,
the current event, surface, and keyUp handling.
In `@Sources/RightSidebarPanelView.swift`:
- Around line 572-579: The prune-and-resolve logic uses tabManager.tabs (active
window only) causing active-window bias; change it to aggregate tabs from all
main-window contexts before computing liveWorkspaceIds and when resolving
existing workspaces for realSessions. Replace uses of tabManager.tabs with a
collectedTabs array built by querying the global window registry/main
controllers (e.g., allWindowControllers.flatMap { $0.tabs }) and use
collectedTabs in the Set creation and in the first(where:) lookups; apply the
same change to the other block referenced (lines 597-601) so pruning and
resolution search across all windows.
- Around line 673-676: realTmuxExecutablePath currently only checks a hardcoded
list of locations and misses tmux installs on MacPorts, Nix or custom PATHs;
update realTmuxExecutablePath to first search the user PATH (use
ProcessInfo.processInfo.environment["PATH"] split on ":" and check each
candidate with FileManager.default.isExecutableFile(atPath:)) and return the
first match, and only if that fails fall back to the existing hardcoded array
["/opt/homebrew/bin/tmux", "/usr/local/bin/tmux", "/usr/bin/tmux"]; keep the
function nonisolated and preserve the FileManager checks so callers of
realTmuxExecutablePath see the same return semantics.
- Around line 515-519: The onKill handler currently calls
killRealTmuxSession(session.name) synchronously and immediately closes the
mapped workspace via tabManager.closeWorkspaceWithConfirmation(workspace);
change killRealTmuxSession to an asynchronous API that returns a success flag
(via completion handler, async/await, or Result) and do not await it on the main
thread; instead, kick off the kill asynchronously and only call
tabManager.closeWorkspaceWithConfirmation(workspace) after the kill completes
successfully (or after a follow-up refresh confirms the tmux session no longer
exists); update both occurrences (the onKill closure around
killRealTmuxSession(session.name) and the similar block at the later lines) to
use the new async success check so the UI button tap is non-blocking and the
workspace is only removed on confirmed kill.
- Around line 560-562: The tmux JSON disk I/O (calls to loadRealTmuxStore() and
saveRealTmuxStore()) must be moved off the main queue: perform those calls on a
background queue (e.g., DispatchQueue.global(qos:.utility).async) and only hop
back to DispatchQueue.main.async to call UI-updating code such as
applyRefresh(...) or to commit results to the view; alternatively, have
applyRefresh accept the already-loaded tmux store/result and call
saveRealTmuxStore from a background queue before/after UI updates. Ensure you
capture any returned data/error from loadRealTmuxStore() on the background
thread and pass it into applyRefresh on the main thread, and do not call
loadRealTmuxStore() or saveRealTmuxStore() directly from inside
DispatchQueue.main.async.
In `@Sources/TabManager.swift`:
- Around line 4941-4944: Currently the code unconditionally routes all panels in
a real tmux workspace through closeRealTmuxPanel by checking
tab.isRealTmuxWorkspace, which makes non-terminal/browser panels (which lack a
realTmuxPaneId) become no-ops when closeRealTmuxPanel returns false; change the
gate to check the specific panel’s realTmuxPaneId by calling
tab.terminalPanel(for: panelId)?.realTmuxPaneId and only call
closeRealTmuxPanel(tab: panelId:) when that optional pane id exists. Apply the
same fix to the other occurrence around the code near the 6096–6098 region so
only true tmux terminal panels are routed to closeRealTmuxPanel.
- Around line 5720-5727: In newSurface(initialInput:), when
workspace.isRealTmuxWorkspace is true the code currently calls
createSplit(tabId:surfaceId:direction:) and returns without forwarding
initialInput; modify the tmux branch to either (a) extend createSplit to accept
an optional initialInput parameter and pass initialInput through, or (b) capture
the created pane/surface id returned by createSplit and immediately call the
existing input-forwarding API (e.g. sendInputToSurface / writeToPane / whatever
central method your codebase uses) to write initialInput into the new surface;
ensure you reference selectedWorkspace, workspace.focusedPanelId and createSplit
in the change so the startup input is not dropped.
- Around line 1269-1283: The code marks workspaces as real-tmux even when there
is no initial pane proxy; change it so workspace.realTmuxSessionId,
workspace.realTmuxSessionName and store.sessionIdToWorkspaceId[session.id] are
only set when initialProxyCommand (computed via Self.realTmuxPaneProxyCommand
from initialPaneId) is non-nil. Leave addWorkspace call as-is but wrap the
assignments to workspace.realTmuxSessionId / realTmuxSessionName and the store
mapping in a conditional that checks initialProxyCommand != nil so imports
without a proxy are not treated as real-tmux.
- Around line 5529-5534: The focusSurface(tabId:surfaceId:) currently returns
early for tmux-backed workspaces and thus ignores explicit surface-focus
requests; change it so that when tab.isRealTmuxWorkspace is true you call
selectRealTmuxPane(tabId: , surfaceId: ) to route the focus request to tmux and
then update the local selection (e.g., call tab.focusPanel(surfaceId) or
otherwise sync the in-memory focused panel) so the UI state reflects the change;
ensure selectRealTmuxPane and this flow do not trigger macOS app
activation/window raising (only mutate in-app selection), and keep focusTab(...,
surfaceId:) callers functional after import.
- Around line 1246-1251: The cleanup loop that calls
closeWorkspace(sessionIdToWorkspaceId ...) skips closing if tabs.count <= 1,
leaving a stale imported workspace; change the logic in the for-loop that
iterates (sessionId, workspaceId) to use the same replacement-workspace fallback
as closeRealTmuxPanel: when the workspace to close is found but tabs.count <= 1,
create/assign a replacement workspace (same behavior used by closeRealTmuxPanel)
and then proceed to remove the stale mapping and call closeWorkspace(...,
killRealTmuxSession: false) on the replacement or otherwise remove the workspace
so the stale imported workspace is not left behind. Ensure you reference and
reuse the same replacement creation/removal helpers used by closeRealTmuxPanel
to keep behavior consistent.
- Around line 1254-1257: The current filtering of store.sessionIdToWorkspaceId
uses refreshedWorkspaceIds (Set(tabs.map { $0.id.uuidString })) which only
contains workspace ids for this TabManager and causes removal of tmux mappings
belonging to other windows; change the filter to only test live session
membership (liveSessionIds.contains($0.key)) or use the previously computed
cross-window workspace-id set instead of refreshedWorkspaceIds so you don't
prune mappings for other windows—update the closure on
store.sessionIdToWorkspaceId.filter to reference liveSessionIds (or the shared
workspace-id set) rather than refreshedWorkspaceIds.
- Around line 1259-1267: The code is incorrectly matching local workspaces by
title (the "|| $0.title == session.name" clause) which can reassign unrelated
workspaces to real tmux sessions; change the lookup in the realSessions loop so
you only adopt an existing workspace if
AppDelegate.shared?.mainWindowWorkspaceForRealTmuxSession(id:name:) returns one
or if a tab already has a matching realTmuxSessionId (tabs.first(where: {
$0.realTmuxSessionId == session.id })), removing the title-based fallback and
ensuring you only update store.sessionIdToWorkspaceId,
existing.realTmuxSessionId, and existing.realTmuxSessionName when a true
persisted or realTmuxSessionId match is found.
In `@Sources/Workspace.swift`:
- Around line 7206-7210: The snapshot serialization/deserialization needs to
include the real-Tmux identity and per-panel pane IDs: update the code paths
that create and restore SessionWorkspaceSnapshot to persist realTmuxSessionId
and realTmuxSessionName when isRealTmuxWorkspace is true, and update the
workspace rehydration logic (the code that reconstructs panels/TerminalPanel
state) to read and set each panel's realTmuxPaneId during restore; ensure the
snapshot model and the restore function that iterates panels both handle these
new fields so real-tmux workspaces are rehydrated with backing-pane routing
instead of falling back to local terminals.
- Around line 9875-9888: The newTerminalSurface(...) function (and any code
paths that call it like createTerminalToRight(...) and the context-menu creation
path) is still allowed to create a native cmux terminal in a real-tmux
workspace; add the same real-tmux invariant check at the top of
newTerminalSurface(...) (or make a single helper guard) so it returns nil unless
allowInRealTmuxWorkspace is true, and update callers (createTerminalToRight and
the context-menu terminal creation) to either pass allowInRealTmuxWorkspace when
appropriate or stop calling newTerminalSurface in real-tmux workspaces;
reference newTerminalSurface(...), createTerminalToRight(...), and the
context-menu creation path when making the change.
---
Nitpick comments:
In `@Sources/TerminalController.swift`:
- Line 5471: The expression calls v2ResolveWindowId(tabManager: tabManager)
twice; compute it once into a local (e.g., let resolvedWindowId =
v2ResolveWindowId(tabManager: tabManager)) and then use
resolvedWindowId?.uuidString for the "window_id" value (wrapped with v2OrNull if
needed) and pass resolvedWindowId to v2Ref(kind: .window, uuid:
resolvedWindowId) for "window_ref" so the lookup is performed once and the
returned value reused.
- Around line 16453-16455: Replace the inline computation let
isRealTmuxWorkspace = tab.realTmuxSessionId != nil || tab.realTmuxSessionName !=
nil with the property accessor tab.isRealTmuxWorkspace and update any subsequent
conditional that uses isRealTmuxWorkspace (e.g. the if checking tab.panels.count
<= 1 && !isRealTmuxWorkspace) to use tab.isRealTmuxWorkspace instead so the
logic matches the V2 split handlers; ensure you remove the local variable and
reference the existing tab.isRealTmuxWorkspace property wherever this inline
check was used.
- Around line 5460-5461: Replace the inline check that computes
isRealTmuxWorkspace using ws.realTmuxSessionId != nil || ws.realTmuxSessionName
!= nil with the existing property accessor ws.isRealTmuxWorkspace so the
condition uses the canonical implementation; update the variable usage in the
block that currently defines let isRealTmuxWorkspace = ... and the subsequent if
to reference ws.isRealTmuxWorkspace for consistency with the rest of the
codebase.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0744242d-ac96-4b23-967c-3d98ff998fee
📒 Files selected for processing (9)
CLI/cmux.swiftSources/AppDelegate.swiftSources/ContentView.swiftSources/GhosttyTerminalView.swiftSources/MainWindowFocusController.swiftSources/RightSidebarPanelView.swiftSources/TabManager.swiftSources/TerminalController.swiftSources/Workspace.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- CLI/cmux.swift
Author
|
Addressed the latest tmux review findings in 5f8ca7f, including cross-window session mapping, async/off-main sidebar tmux operations, stricter real tmux identity matching, tmux panel routing, snapshot persistence, and socket/key handling nits. @coderabbitai review |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (2)
Sources/GhosttyTerminalView.swift (1)
7191-7193:⚠️ Potential issue | 🔴 CriticalStill swallowing tmux's default prefix in real-tmux workspaces.
This still turns
Ctrl+Binto a no-op by returning early on both key down and key up, so imported tmux sessions never receive the prefix sequence. Route the event through the tmux input path instead of dropping it.Also applies to: 7569-7588, 7641-7643
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 7191 - 7193, The early return in the key event handlers when shouldBlockRealTmuxShortcut(event, surface: surface) is true is swallowing tmux's prefix (Ctrl+B); instead of returning, forward the event into the tmux input path so the imported tmux session receives it. Replace the bare return in the blocks that call shouldBlockRealTmuxShortcut (seen in GhosttyTerminalView's key handling methods) with a call to the tmux input forwarder (e.g., routeToTmuxInput(event, surface: surface) or sendEventToTmuxInput(event, surface: surface)) so the event is delivered to real-tmux workspaces; do the same for the other affected locations (the other occurrences around the lines noted) to ensure both keyDown and keyUp are forwarded.Sources/TabManager.swift (1)
5538-5545:⚠️ Potential issue | 🟠 MajorNon-tmux panels in tmux workspaces still no-op on explicit focus.
tab.isRealTmuxWorkspaceis too broad here. IfsurfaceIdis a browser panel,selectRealTmuxPane(...)returnsfalseand the explicit focus request is dropped instead of falling back totab.focusPanel(surfaceId).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TabManager.swift` around lines 5538 - 5545, The current focusSurface(tabId: UUID, surfaceId: UUID) path treats any real tmux workspace as exclusive, dropping focus requests when Self.selectRealTmuxPane(in:tab, panelId:surfaceId) returns false; change the logic so we only call selectRealTmuxPane for panes that are actually tmux panes (or, if selectRealTmuxPane returns false, fall back to tab.focusPanel(surfaceId)). Concretely: in focusSurface use a narrower condition (e.g., check the target panel's type/metadata for tmux pane) before invoking Self.selectRealTmuxPane(in:tab, panelId:surfaceId), and ensure that if selectRealTmuxPane returns false (or the panel is not a tmux pane) you still call tab.focusPanel(surfaceId) so non-tmux/browser panels in tmux workspaces are focused.
🧹 Nitpick comments (1)
Sources/TerminalController.swift (1)
3119-3124:kindandsourcefields are currently identical.Both fields return the same value (
"real-tmux"or"native"). If this is intentional for API forward-compatibility, consider adding a brief comment. Otherwise, one field could be removed to reduce payload redundancy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalController.swift` around lines 3119 - 3124, The JSON payload sets both "kind" and "source" to the same value based on workspace.isRealTmuxWorkspace, causing redundancy; either remove one of the duplicate fields (e.g., drop "source" and keep "kind") or, if the duplication is intentional for API forward-compatibility, add an inline comment next to the dictionary entry explaining that duplication for future-proofing. Update any serialization/consumer code that expects the removed field and keep the real_tmux block (realTmuxSessionId/realTmuxSessionName) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/RightSidebarPanelView.swift`:
- Around line 487-500: The project is missing six localization keys used by
RightSidebarPanelView (Text and help strings) which prevents Japanese
translations from working; add entries for tmux.sessions.title,
tmux.sessions.refresh, tmux.session.current, tmux.session.attach,
tmux.session.kill, and rightSidebar.mode.tmux to Resources/Localizable.xcstrings
with English and Japanese translations (provide the English default and the
Japanese localized string for each key), ensure the keys match exactly the
identifiers used in the String(localized:defaultValue:) calls (e.g., usages in
RightSidebarPanelView and any session action UI), save the updated .xcstrings
files in the appropriate localization folder and rebuild so the localized
strings are picked up.
In `@Sources/TabManager.swift`:
- Around line 5912-5929: When tab.isRealTmuxWorkspace is true and you create a
backend tmux pane, ensure you roll it back if creating the local panel fails:
after obtaining realTmuxPaneId and proxyCommand, call tab.newTerminalSplit(...)
and if it returns nil then invoke Self.runRealTmux(arguments: ["kill-pane",
"-t", realTmuxPaneId]) to remove the orphaned tmux pane and then return nil;
reuse the existing guard that kills the pane on proxy-command failure as a
template and apply the same cleanup when newTerminalSplit returns nil.
- Around line 1209-1235: The probe currently conflates “no sessions” with probe
failure because listRealTmuxSessions() returns an empty array on error; change
the probe API (e.g., make listRealTmuxSessions() return Optional/[Result]/throw)
and update syncRealTmuxSessions() to detect probe failure before calling
applyRealTmuxSessionSync(sessionDetails:); when the probe fails, skip calling
applyRealTmuxSessionSync and avoid pruning/closing mapped workspaces (do not
treat empty result from a failed probe as authoritative), while preserving the
existing retry/rerun semantics (realTmuxSessionSyncInFlight and
realTmuxSessionSyncRerunPending). Ensure the same fix is applied for the
analogous block around applyRealTmuxSessionSync at the later region (the
1308–1329 area).
- Around line 1258-1284: The import path creates a workspace when
initialProxyCommand is nil but does not record
store.sessionIdToWorkspaceId[session.id], causing the same session to be
re-imported on the next sync; update the logic in the loop that calls
addWorkspace (and uses Self.realTmuxPaneProxyCommand / Self.realTmuxAttachInput)
so that after creating the workspace you always persist
store.sessionIdToWorkspaceId[session.id] = workspace.id.uuidString (regardless
of whether initialProxyCommand is nil), and only set workspace.realTmuxSessionId
/ workspace.realTmuxSessionName when initialProxyCommand is non-nil; this
prevents duplicate imports for non-proxied attach paths while preserving
proxy-specific fields.
- Around line 4949-4953: The early return path that calls
closeRealTmuxPanel(...) bypasses the normal confirm-close gate; update the close
flow so that when tab.isRealTmuxWorkspace and Self.normalizedRealTmuxPaneId(...)
!= nil you first invoke the same confirmation/close routine used for regular
panels (the method that handles running-process/dirty confirmation — e.g., the
existing closePanel/confirm-close flow) and only call closeRealTmuxPanel(...)
after confirmation succeeds (or have the normal close routine call
closeRealTmuxPanel as the final teardown), ensuring terminalPanel(for:),
normalizedRealTmuxPaneId(_:), and closeRealTmuxPanel(...) are used but do not
short-circuit the confirmation logic.
- Around line 4500-4510: The last-window close path isn't running the same
real-tmux-store cleanup in closeWorkspace(_:, killRealTmuxSession:), leaving
sessionIdToWorkspaceId entries pointing at dead workspaces; extract the cleanup
logic (the loadRealTmuxStore/filter/save sequence and optional
Self.killRealTmuxSession call) into a small helper like
cleanupRealTmuxMapping(for workspace:killSession:) and invoke that helper from
both closeWorkspace(...) and the code path that directly closes the last window
(or simply call closeWorkspace(...) from the last-window close path), ensuring
you reference and reuse the symbols CloseWorkspace/cleanupRealTmuxMapping,
Self.loadRealTmuxStore, sessionIdToWorkspaceId, Self.saveRealTmuxStore, and
Self.killRealTmuxSession so the stale mapping is removed in all code paths.
In `@Sources/Workspace.swift`:
- Around line 7228-7232: sidebarImmediateObservationPublisher and
sidebarObservationPublisher never observe changes to the `@Published` real-tmux
identity properties, so tmux badge/state can remain stale; update the publisher
arrays in sidebarImmediateObservationPublisher and sidebarObservationPublisher
to include observation signals for $realTmuxSessionId and $realTmuxSessionName
by adding sidebarObservationSignal($realTmuxSessionId) and
sidebarObservationSignal($realTmuxSessionName) to the publishers passed into
Publishers.MergeMany (using the existing sidebarObservationSignal helper) so
those property changes propagate to observers.
- Around line 741-750: Compute the proxy command up front using
Self.realTmuxPaneProxyCommand(snapshot.realTmuxPaneId) and if
snapshot.realTmuxPaneId is non-nil but the proxy command is nil, abort the
restore (do not call newTerminalSurface); otherwise pass that non-optional proxy
command into the initialCommand parameter and set allowInRealTmuxWorkspace only
when both realTmuxPaneId and the proxy command are present. This ensures
newTerminalSurface(...) is only created when the proxy command could be built
and prevents a native cmux terminal from being restored into a real-tmux
workspace.
- Around line 741-750: This call to newTerminalSurface is reconnecting to an
existing tmux pane (realTmuxPaneId != nil) and must not perform restore-time
replay; change the logic so that when realTmuxPaneId is non-nil you do not pass
restoredAgentResumeInput, replayEnvironment, or any restore initialCommand into
newTerminalSurface. Compute conditional values (e.g. let initialCommand =
realTmuxPaneId == nil ? realTmuxPaneProxyCommand(...) : nil; let initialInput =
realTmuxPaneId == nil ? restoredAgentResumeInput : nil; let startupEnvironment =
realTmuxPaneId == nil ? replayEnvironment : nil) and pass those variables into
newTerminalSurface so live tmux reattachments skip replay while keeping
allowInRealTmuxWorkspace and realTmuxPaneId as-is.
---
Duplicate comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 7191-7193: The early return in the key event handlers when
shouldBlockRealTmuxShortcut(event, surface: surface) is true is swallowing
tmux's prefix (Ctrl+B); instead of returning, forward the event into the tmux
input path so the imported tmux session receives it. Replace the bare return in
the blocks that call shouldBlockRealTmuxShortcut (seen in GhosttyTerminalView's
key handling methods) with a call to the tmux input forwarder (e.g.,
routeToTmuxInput(event, surface: surface) or sendEventToTmuxInput(event,
surface: surface)) so the event is delivered to real-tmux workspaces; do the
same for the other affected locations (the other occurrences around the lines
noted) to ensure both keyDown and keyUp are forwarded.
In `@Sources/TabManager.swift`:
- Around line 5538-5545: The current focusSurface(tabId: UUID, surfaceId: UUID)
path treats any real tmux workspace as exclusive, dropping focus requests when
Self.selectRealTmuxPane(in:tab, panelId:surfaceId) returns false; change the
logic so we only call selectRealTmuxPane for panes that are actually tmux panes
(or, if selectRealTmuxPane returns false, fall back to
tab.focusPanel(surfaceId)). Concretely: in focusSurface use a narrower condition
(e.g., check the target panel's type/metadata for tmux pane) before invoking
Self.selectRealTmuxPane(in:tab, panelId:surfaceId), and ensure that if
selectRealTmuxPane returns false (or the panel is not a tmux pane) you still
call tab.focusPanel(surfaceId) so non-tmux/browser panels in tmux workspaces are
focused.
---
Nitpick comments:
In `@Sources/TerminalController.swift`:
- Around line 3119-3124: The JSON payload sets both "kind" and "source" to the
same value based on workspace.isRealTmuxWorkspace, causing redundancy; either
remove one of the duplicate fields (e.g., drop "source" and keep "kind") or, if
the duplication is intentional for API forward-compatibility, add an inline
comment next to the dictionary entry explaining that duplication for
future-proofing. Update any serialization/consumer code that expects the removed
field and keep the real_tmux block (realTmuxSessionId/realTmuxSessionName)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7c30031b-b78d-49fb-bbb6-881804f0550f
📒 Files selected for processing (7)
Sources/AppDelegate.swiftSources/GhosttyTerminalView.swiftSources/RightSidebarPanelView.swiftSources/SessionPersistence.swiftSources/TabManager.swiftSources/TerminalController.swiftSources/Workspace.swift
Author
|
Addressed the latest inline review findings in 6ae8abb: added missing tmux sidebar localizations, hardened real tmux probe/import/close/split rollback lifecycle, updated sidebar observation/restore behavior, and documented the kind/source socket compatibility field. Kept Ctrl+B blocking unchanged because it is an explicit product requirement for imported real-tmux workspaces. @coderabbitai review |
|
✏️ Learnings added
🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
Sources/TerminalController.swift (1)
14974-14977:⚠️ Potential issue | 🟠 MajorStill return an explicit error when
newSplitfails.This branch still exits without setting
resultwhentabManager.newSplit(...)returnsnil, so the V1 caller can get an empty/stale response instead of a failure string.🐛 Proposed fix
if let id = tabManager.newSplit(tabId: tabId, surfaceId: focusedPanelId, direction: direction, focus: focus) { result = "OK \(id.uuidString) [tmux]" + } else { + result = "ERROR: Failed to split real tmux pane" } return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalController.swift` around lines 14974 - 14977, The branch handling the result of tabManager.newSplit(...) currently returns early without setting result when newSplit returns nil; update the logic around the call to tabManager.newSplit(tabId: tabId, surfaceId: focusedPanelId, direction: direction, focus: focus) so that when it returns nil you set result to an explicit error string (e.g., "ERR newSplit failed" or similar) before returning, while preserving the existing success path that sets result = "OK \(id.uuidString) [tmux]".Sources/TabManager.swift (2)
1290-1308:⚠️ Potential issue | 🟠 MajorDon't re-promote attach-fallback imports into real-tmux mode on the next poll.
This path correctly creates a fallback import with no
realTmuxPaneIdwhen proxy startup is unavailable, but the unconditional assignment at Lines 1307-1308 flips that workspace back intoisRealTmuxWorkspaceon the next sync anyway. At that point the existing terminal panel is still not pane-backed, so close/split/focus routing becomes inconsistent again. Only setrealTmuxSessionId/realTmuxSessionNameonce the workspace already owns a pane-backed proxy panel.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TabManager.swift` around lines 1290 - 1308, The loop that syncs realSessions unconditionally assigns workspace.realTmuxSessionId/Name (symbols: realSessions loop, workspace.realTmuxSessionId, workspace.realTmuxSessionName), which re-promotes attach-fallback imports into real-tmux mode; change it so those two properties are only set when the workspace actually owns a pane-backed proxy panel — e.g. check for an existing pane-backed proxy on the workspace (such as workspace.proxyPanel?.isPaneBacked or a helper like workspace.ownsPaneBackedProxy()) and only then assign realTmuxSessionId/realTmuxSessionName; leave them unset when the workspace has a fallback import without a pane-backed proxy.
1216-1227:⚠️ Potential issue | 🟠 MajorTreat “no tmux server” as an authoritative empty result, not a probe failure.
When the last tmux session exits,
tmux list-sessionsreturns a non-zero status.listRealTmuxSessions()currently maps that tonil, andsyncRealTmuxSessions()treatsnilas “skip reconciliation”, so the stale real-tmux workspaces never get closed/pruned. This needs a third state: “probe failed” vs “tmux has no live sessions”.Also applies to: 1318-1340
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TabManager.swift` around lines 1216 - 1227, listRealTmuxSessions currently conflates "no tmux server / zero sessions" with a probe failure by returning nil; update the API and callers so you can distinguish three states: probe error, authoritative empty result, and non-empty sessions. Change listRealTmuxSessions to return a discriminated result (e.g., enum or Result type) that encodes .error(Error), .empty, and .sessions([String]) and update syncRealTmuxSessions (and the DispatchQueue block using realTmuxSessionSyncInFlight/realTmuxSessionSyncRerunPending) to treat .empty as an authoritative set (perform reconciliation/closure of stale workspaces) while only skipping reconciliation on .error. Ensure all call sites that currently check for nil are updated to handle the new cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/TabManager.swift`:
- Around line 4511-4524: The cleanupRealTmuxMapping(for:killSession:) call is
currently executed before the early return guard in
closeWorkspace(_:killRealTmuxSession:), which can remove/kill the tmux backend
while leaving the last workspace onscreen; move the call so that the guard
tabs.count > 1 runs first and only if we will actually close a workspace call
cleanupRealTmuxMapping(for: workspace, killSession: killRealTmuxSession). Update
closeWorkspace(_:killRealTmuxSession:) to perform the guard check before
invoking cleanupRealTmuxMapping and ensure the same function signature and
semantics are preserved.
In `@Sources/TerminalController.swift`:
- Around line 5462-5474: The check/branch is using the workspace-wide flag
isRealTmuxWorkspace but the operation is per-surface; instead use the surface's
tmux identity (realTmuxPaneId or equivalent) to decide routing and last-surface
gating. Change the predicate around TabManager.closeSurface(tabId: ws.id,
surfaceId: surfaceId) and ws.closePanel(surfaceId, force: true) to test the
target panel’s realTmuxPaneId (or a helper that returns whether the specific
surface is tmux-backed) rather than ws.isRealTmuxWorkspace, and ensure the
"cannot close last surface" guard also uses that per-surface predicate; keep
TabManager.closeSurface and ws.closePanel calls intact and leave
v2ResolveWindowId, surfaceId and ws.id usages as-is.
In `@Sources/Workspace.swift`:
- Around line 328-329: The code is enabling "real-tmux" behavior based solely on
isRealTmuxWorkspace (which can be true when only a display name exists); change
those conditionals to require a non-nil realTmuxSessionId instead. Update
occurrences where realTmuxSessionId/realTmuxSessionName are passed (e.g., the
initializer call with realTmuxSessionId: and realTmuxSessionName:) to use
realTmuxSessionId != nil as the gate, leave realTmuxSessionName as display-only
metadata, and likewise change any logic in GhosttyTerminalView and the Ctrl+B
blocking code paths (the blocks around lines ~7238-7242) to only activate when
realTmuxSessionId != nil.
- Around line 9917-9925: The session-drop path bypasses the real-tmux guard
because handleSessionDrop calls splitPaneWithNewTerminal which directly
instantiates TerminalPanel; add the same
isRealTmuxWorkspace/allowInRealTmuxWorkspace check before creating a native
terminal in that path. Concretely, update handleSessionDrop and/or
splitPaneWithNewTerminal to early-return (nil or no-op) when isRealTmuxWorkspace
&& !allowInRealTmuxWorkspace, mirroring the guard used in
newTerminalSplit/newTerminalSurface so TerminalPanel is never instantiated for
real-tmux workspaces.
---
Duplicate comments:
In `@Sources/TabManager.swift`:
- Around line 1290-1308: The loop that syncs realSessions unconditionally
assigns workspace.realTmuxSessionId/Name (symbols: realSessions loop,
workspace.realTmuxSessionId, workspace.realTmuxSessionName), which re-promotes
attach-fallback imports into real-tmux mode; change it so those two properties
are only set when the workspace actually owns a pane-backed proxy panel — e.g.
check for an existing pane-backed proxy on the workspace (such as
workspace.proxyPanel?.isPaneBacked or a helper like
workspace.ownsPaneBackedProxy()) and only then assign
realTmuxSessionId/realTmuxSessionName; leave them unset when the workspace has a
fallback import without a pane-backed proxy.
- Around line 1216-1227: listRealTmuxSessions currently conflates "no tmux
server / zero sessions" with a probe failure by returning nil; update the API
and callers so you can distinguish three states: probe error, authoritative
empty result, and non-empty sessions. Change listRealTmuxSessions to return a
discriminated result (e.g., enum or Result type) that encodes .error(Error),
.empty, and .sessions([String]) and update syncRealTmuxSessions (and the
DispatchQueue block using
realTmuxSessionSyncInFlight/realTmuxSessionSyncRerunPending) to treat .empty as
an authoritative set (perform reconciliation/closure of stale workspaces) while
only skipping reconciliation on .error. Ensure all call sites that currently
check for nil are updated to handle the new cases.
In `@Sources/TerminalController.swift`:
- Around line 14974-14977: The branch handling the result of
tabManager.newSplit(...) currently returns early without setting result when
newSplit returns nil; update the logic around the call to
tabManager.newSplit(tabId: tabId, surfaceId: focusedPanelId, direction:
direction, focus: focus) so that when it returns nil you set result to an
explicit error string (e.g., "ERR newSplit failed" or similar) before returning,
while preserving the existing success path that sets result = "OK
\(id.uuidString) [tmux]".
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 51e9639d-0b95-407f-8c48-2b1efeaf4a11
📒 Files selected for processing (4)
Resources/Localizable.xcstringsSources/TabManager.swiftSources/TerminalController.swiftSources/Workspace.swift
✅ Files skipped from review due to trivial changes (1)
- Resources/Localizable.xcstrings
Comment on lines
+4511
to
4524
| private func cleanupRealTmuxMapping(for workspace: Workspace, killSession: Bool) { | ||
| if let realTmuxSessionId = workspace.realTmuxSessionId { | ||
| var store = Self.loadRealTmuxStore() | ||
| store.sessionIdToWorkspaceId = store.sessionIdToWorkspaceId.filter { $0.value != workspace.id.uuidString } | ||
| Self.saveRealTmuxStore(store) | ||
| if killSession { | ||
| Self.killRealTmuxSession(id: realTmuxSessionId, name: workspace.realTmuxSessionName) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func closeWorkspace(_ workspace: Workspace, killRealTmuxSession: Bool = true) { | ||
| cleanupRealTmuxMapping(for: workspace, killSession: killRealTmuxSession) | ||
| guard tabs.count > 1 else { return } |
There was a problem hiding this comment.
Move tmux cleanup below the last-workspace guard.
cleanupRealTmuxMapping(for:killSession:) now runs before guard tabs.count > 1, so closeWorkspace(...) can remove the mapping and kill the backing tmux session even when the method immediately bails out and leaves the last workspace open. closeCurrentWorkspace()/closeTab(_:) still call this path directly, so this can strand an onscreen workspace with its backend already destroyed.
♻️ Suggested fix
func closeWorkspace(_ workspace: Workspace, killRealTmuxSession: Bool = true) {
- cleanupRealTmuxMapping(for: workspace, killSession: killRealTmuxSession)
guard tabs.count > 1 else { return }
+ cleanupRealTmuxMapping(for: workspace, killSession: killRealTmuxSession)
sentryBreadcrumb("workspace.close", data: ["tabCount": tabs.count - 1])
clearWorkspaceGitProbes(workspaceId: workspace.id)
clearWorkspacePullRequestTracking(workspaceId: workspace.id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TabManager.swift` around lines 4511 - 4524, The
cleanupRealTmuxMapping(for:killSession:) call is currently executed before the
early return guard in closeWorkspace(_:killRealTmuxSession:), which can
remove/kill the tmux backend while leaving the last workspace onscreen; move the
call so that the guard tabs.count > 1 runs first and only if we will actually
close a workspace call cleanupRealTmuxMapping(for: workspace, killSession:
killRealTmuxSession). Update closeWorkspace(_:killRealTmuxSession:) to perform
the guard check before invoking cleanupRealTmuxMapping and ensure the same
function signature and semantics are preserved.
Comment on lines
+5462
to
+5474
| if ws.panels.count <= 1 && !ws.isRealTmuxWorkspace { | ||
| result = .err(code: "invalid_state", message: "Cannot close the last surface", data: nil) | ||
| return | ||
| } | ||
|
|
||
| // Socket API must be non-interactive: bypass close-confirmation gating. | ||
| ws.closePanel(surfaceId, force: true) | ||
| result = .ok(["workspace_id": ws.id.uuidString, "workspace_ref": v2Ref(kind: .workspace, uuid: ws.id), "surface_id": surfaceId.uuidString, "surface_ref": v2Ref(kind: .surface, uuid: surfaceId), "window_id": v2OrNull(v2ResolveWindowId(tabManager: tabManager)?.uuidString), "window_ref": v2Ref(kind: .window, uuid: v2ResolveWindowId(tabManager: tabManager))]) | ||
| let closed = ws.isRealTmuxWorkspace | ||
| ? tabManager.closeSurface(tabId: ws.id, surfaceId: surfaceId) | ||
| : ws.closePanel(surfaceId, force: true) | ||
| let resolvedWindowId = v2ResolveWindowId(tabManager: tabManager) | ||
| result = closed | ||
| ? .ok(["workspace_id": ws.id.uuidString, "workspace_ref": v2Ref(kind: .workspace, uuid: ws.id), "surface_id": surfaceId.uuidString, "surface_ref": v2Ref(kind: .surface, uuid: surfaceId), "window_id": v2OrNull(resolvedWindowId?.uuidString), "window_ref": v2Ref(kind: .window, uuid: resolvedWindowId)]) | ||
| : .err(code: "internal_error", message: "Failed to close surface", data: ["surface_id": surfaceId.uuidString]) |
There was a problem hiding this comment.
Use the target surface’s tmux identity here, not the workspace-wide flag.
These branches are keyed off isRealTmuxWorkspace, but the operation is surface-specific. That means a non-tmux surface inside a real-tmux workspace can bypass the last-surface guard or get routed into tmux split handling even when that panel is not tmux-backed. TabManager.closeSurface(...) already makes this decision per panel via realTmuxPaneId; these call sites should mirror that predicate.
Also applies to: 5500-5516, 6755-6777, 16455-16465
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TerminalController.swift` around lines 5462 - 5474, The check/branch
is using the workspace-wide flag isRealTmuxWorkspace but the operation is
per-surface; instead use the surface's tmux identity (realTmuxPaneId or
equivalent) to decide routing and last-surface gating. Change the predicate
around TabManager.closeSurface(tabId: ws.id, surfaceId: surfaceId) and
ws.closePanel(surfaceId, force: true) to test the target panel’s realTmuxPaneId
(or a helper that returns whether the specific surface is tmux-backed) rather
than ws.isRealTmuxWorkspace, and ensure the "cannot close last surface" guard
also uses that per-surface predicate; keep TabManager.closeSurface and
ws.closePanel calls intact and leave v2ResolveWindowId, surfaceId and ws.id
usages as-is.
Comment on lines
+328
to
+329
| realTmuxSessionId: isRealTmuxWorkspace ? realTmuxSessionId : nil, | ||
| realTmuxSessionName: isRealTmuxWorkspace ? realTmuxSessionName : nil, |
There was a problem hiding this comment.
Key real-tmux mode off realTmuxSessionId only.
isRealTmuxWorkspace currently becomes true when only the display name is present. That pushes partial/legacy snapshots into the tmux-only paths here and in GhosttyTerminalView, even though there is no stable tmux session identity to route against. Gate the mode on realTmuxSessionId != nil and treat realTmuxSessionName as display metadata only.
Suggested fix
`@Published` var realTmuxSessionId: String?
`@Published` var realTmuxSessionName: String?
var isRealTmuxWorkspace: Bool {
- realTmuxSessionId != nil || realTmuxSessionName != nil
+ realTmuxSessionId != nil
}Based on learnings, blocking Ctrl+B is only required for real-tmux workspaces “where realTmuxSessionId is non-nil”.
Also applies to: 7238-7242
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Workspace.swift` around lines 328 - 329, The code is enabling
"real-tmux" behavior based solely on isRealTmuxWorkspace (which can be true when
only a display name exists); change those conditionals to require a non-nil
realTmuxSessionId instead. Update occurrences where
realTmuxSessionId/realTmuxSessionName are passed (e.g., the initializer call
with realTmuxSessionId: and realTmuxSessionName:) to use realTmuxSessionId !=
nil as the gate, leave realTmuxSessionName as display-only metadata, and
likewise change any logic in GhosttyTerminalView and the Ctrl+B blocking code
paths (the blocks around lines ~7238-7242) to only activate when
realTmuxSessionId != nil.
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.
Summary
__real-tmux-pane-proxyhelper that renders a single tmux pane through control-mode%outputstreaming and forwards input withsend-keysfallback.realTmuxPaneIdon terminal panels so split, focus, and close operations stay synchronized with the backing tmux pane.Verification
CONTRIBUTING.mdbefore PR prep./tmp/cmux-cli __real-tmux-pane-proxy --pane %Nwith marker output.lsp_diagnosticsclean onSources/RightSidebarPanelView.swift,Sources/TabManager.swift, andSources/GhosttyTerminalView.swift;git diff --checkclean;PATH="$HOME/.local/zig/0.15.2:$PATH" ./scripts/reload.sh --tag real-tmux-sync4succeeded;review_fix_smoke=pass pane=%75.Related
__tmux-compat: short tmux format codes (#S, #I, #W, ...) are not substituted, breakingcmux omc#3228Notes
send-keysbecause control-mode does not provide a raw stdin byte-write API.Package.swiftandPackage.resolvedwere intentionally left out of these commits.Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Localization