Repository navigation
Conversation
|
@pgbezerra is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a persisted WorkspaceCommands store and settings window, a titlebar split-button menu to pick/launch workspace commands, extends workspace definitions with SSH/program fields, threads initial-terminal command and close-on-exit behavior through workspace creation/execution, and exposes AppDelegate APIs to list/perform/manage commands. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App as App\n(AppDelegate)
participant Store as WorkspaceCommandsStore
participant Executor as CmuxConfigExecutor
participant Workspace as Workspace
participant Remote as RemoteConnection
User->>App: Launch (fresh)
App->>Store: defaultCommand()
Store-->>App: WorkspaceCommandConfig
App->>Executor: executeWorkspaceCommand(config)
Executor->>Executor: ensure restart == .always\ngenerate unique title if collision
Executor->>Workspace: create workspace(initialTerminalCommand,\nclosePanesOnInitialCommandExit)
alt config.remote exists
Executor->>Executor: build SSH startup command
Executor->>Workspace: apply WorkspaceRemoteConfiguration
Workspace->>Remote: configureRemoteConnection(autoConnect: true)
Remote-->>Workspace: connection established
end
Workspace-->>App: workspace ready
App-->>User: window shows workspace
sequenceDiagram
participant User
participant Titlebar as Titlebar\nSplit Button
participant Store as WorkspaceCommandsStore
participant App as App\n(AppDelegate)
participant Executor as CmuxConfigExecutor
User->>Titlebar: Click chevron
Titlebar->>Store: fetch commands
Store-->>Titlebar: command list
User->>Titlebar: Select command "Prod"
Titlebar->>App: performWorkspaceCommand(id:"Prod")
App->>Store: command(id:)
Store-->>App: WorkspaceCommandConfig
App->>Executor: executeWorkspaceCommand(config)
Executor-->>App: workspace created
App-->>User: new workspace opened
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
pgbezerra
force-pushed
the
issue-3257-remote-workspace
branch
2 times, most recently
from
April 29, 2026 17:38
6fbfc90 to
9865dc9
Compare
…grams Adds a Preferences-driven workspace-command system that replaces the deferred picker in manaflow-ai#3257 and the JSON-only `~/.config/cmux/cmux.json` workspace section with a UserDefaults-backed store and a list/detail editor. Closes manaflow-ai#3257. UserDefaults-backed store - New `WorkspaceCommandsStore` (singleton, `cmux.workspaceCommands.v1` key) holds an ordered list of `WorkspaceCommandConfig` entries plus a `defaultCommandID` selection. Posts a `didChange` notification on every persist. - A built-in `Local` workspace at a fixed UUID is always prepended so the list is never empty; it can't be renamed, deleted, or moved, and it falls through as the Cmd+N default when nothing else is selected. - `restoreDefaults()` clears every user-added command and the explicit default selection, leaving only Local. Settings UI (Cmd+,) - New "Workspaces" section opens `WorkspaceCommandsSettingsView` — a list/detail editor (NavigationSplitView). Detail panel covers name, open behavior (always / reuse / replace / ask), default-for- Cmd+N toggle, and: - Local section: optional `Program` field (e.g. `/opt/homebrew/bin/fish`, `/bin/bash -l`). Empty = Ghostty's default shell. - Remote (SSH) section: host, port, identity file, ssh -o options, optional startup command. - "Restore Defaults" button with a confirm alert. Titlebar `+` split button - Replaced the SwiftUI Menu with an `NSViewRepresentable` wrapping an AppKit `NSButton` pair + `NSMenu`. The menu rebuilds via `menuNeedsUpdate(_:)` against the current store every time it opens, so add / edit / remove / restore-defaults propagate immediately. (SwiftUI Menu inside the titlebar's detached NSHostingView did not reliably re-render on store mutations.) - Menu has a separator and a "Manage Workspaces…" entry that routes through a `cmuxOpenWorkspaceCommandsWindow` notification → a `WorkspaceCommandsWindowOpener` view modifier on the main WindowGroup → SwiftUI `openWindow(id:)`. Executor + workspace plumbing - `CmuxRestartBehavior` gains `.always` (now the default), which skips the existing-name match and auto-suffixes the title (`server.internal`, `server.internal 2`, …) so each Cmd+N spawns a fresh tab. `.ignore`, `.recreate`, and `.confirm` remain available. - `CmuxWorkspaceDefinition.program` carries the optional non-remote program through to `addWorkspace(initialTerminalCommand:)`. SSH invocation still wins when both are present. - New `Workspace.closePanesOnInitialCommandExit` flag, threaded through `TabManager.addWorkspace`, inverts `wait_after_command` for the initial pane and any panes spawned by Cmd+T / splits in that workspace. The `CmuxConfigExecutor` sets it whenever the workspace has a remote block or a program, so SSH/program panes auto-close on exit instead of holding the PTY with a "Process exited" prompt. The cmux ssh / vm new flows leave the flag false and keep their existing wait behavior. AppDelegate + supporting - `availableWorkspaceCommands()`, `performWorkspaceCommand(id:)`, `performDefaultWorkspaceCommand(debugSource:)`, and `openWorkspaceCommandsWindow(debugSource:)` route the new flows through the executor pipeline. Note: a future ghostty fork patch is needed to honor `opts.wait_after_command = false` when a `command` is set (apprt/embedded.zig:534 currently forces it to true), which would let the auto-close behavior take effect end-to-end. The Swift-side flag is already in place for that day. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
pgbezerra
force-pushed
the
issue-3257-remote-workspace
branch
from
April 30, 2026 02:22
9865dc9 to
f59eda3
Compare
pgbezerra
changed the title
pgbezerra
marked this pull request as ready for review
April 30, 2026 02:29
Contributor
Greptile SummaryThis PR replaces the JSON-file-only workspace configuration with a full
Confidence Score: 4/5Safe to merge with minor fixes; the P1 empty-state dead code affects only the Settings summary label, not data correctness. One P1 (unreachable empty-state branch in the summary label) and one P2 (missing Default badge for Local in the sidebar list). No data loss, crashes, or security issues. Core flows — launching workspaces via Cmd-N, the titlebar split button, and SSH — are correct. Sources/cmuxApp.swift (dead isEmpty check) and Sources/Settings/WorkspaceCommandsSettingsView.swift (Default badge logic for built-in Local) Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant TitlebarButton as NewWorkspaceSplitButtonView
participant AppDelegate
participant Store as WorkspaceCommandsStore
participant Executor as CmuxConfigExecutor
participant TabManager
User->>TitlebarButton: Click +
TitlebarButton->>AppDelegate: onNewTab()
AppDelegate->>Store: defaultCommand()
Store-->>AppDelegate: WorkspaceCommandConfig
AppDelegate->>Executor: execute(command.asCmuxCommandDefinition(), ...)
Executor->>Executor: buildRemoteTerminalStartupCommand() [if SSH]
Executor->>TabManager: addWorkspace(initialTerminalCommand:, closePanesOnInitialCommandExit:)
TabManager-->>AppDelegate: Workspace
User->>TitlebarButton: Click chevron
TitlebarButton->>TitlebarButton: menuNeedsUpdate() — rebuild NSMenu
TitlebarButton->>Store: commands
Store-->>TitlebarButton: [Local, ...userCommands]
User->>TitlebarButton: Select workspace from menu
TitlebarButton->>AppDelegate: performWorkspaceCommand(id:)
AppDelegate->>Store: command(id:)
Store-->>AppDelegate: WorkspaceCommandConfig
AppDelegate->>Executor: execute(...)
Executor->>TabManager: addWorkspace(...)
Reviews (1): Last reviewed commit: "Add UI-driven workspace command picker w..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (2)
Sources/CmuxWorkspaceDefinition.swift (1)
35-40: ⚡ Quick winFail fast when
remote/programappear in decoded JSON.Right now those keys are silently dropped. Rejecting them explicitly would make migration failures obvious instead of quietly changing behavior.
🧭 Proposed fix
init(from decoder: Decoder) throws { let container = try decoder.container(keyedBy: CodingKeys.self) name = try container.decodeIfPresent(String.self, forKey: .name) cwd = try container.decodeIfPresent(String.self, forKey: .cwd) layout = try container.decodeIfPresent(CmuxLayoutNode.self, forKey: .layout) - // `remote` and `program` are runtime-only — they're populated by the - // `WorkspaceCommandsStore` projection when the executor runs a - // workspace command. Workspace commands aren't authored in cmux.json - // anymore, so the JSON decoder does not accept these keys. + // `remote` and `program` are runtime-only. + if container.contains(.remote) || container.contains(.program) { + throw DecodingError.dataCorrupted( + DecodingError.Context( + codingPath: decoder.codingPath, + debugDescription: "`workspace.remote` and `workspace.program` are not supported in cmux.json; configure them in Settings → Workspaces." + ) + ) + } remote = nil program = nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/CmuxWorkspaceDefinition.swift` around lines 35 - 40, The decoder currently silently drops runtime-only keys `remote` and `program` by setting them to nil; instead, modify the Decodable implementation (the init(from decoder:)) and/or the CodingKeys in CmuxWorkspaceDefinition so that if the keyed container contains the keys "remote" or "program" you throw a DecodingError (e.g., dataCorrupted or keyNotFound) with a clear message; locate the custom init(from:) and add explicit checks using container.contains(.remote)/container.contains(.program) and raise an error pointing out that those keys are invalid in cmux.json.Sources/cmuxApp.swift (1)
6413-6416: ⚡ Quick winAdd Settings search/navigation anchors for the new Workspaces section.
Every other top-level Settings section in this view registers
SettingsSearchIndexanchors, but this one does not. That leaves sidebar search/deep-link navigation with nothing stable to scroll to for the new section.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 6413 - 6416, The Workspaces settings block (SettingsSectionHeader / SettingsCard / WorkspaceCommandsSettingsRow) lacks the SettingsSearchIndex anchors used elsewhere; add the same SettingsSearchIndex registration calls for the "Workspaces" section (attach the anchor(s) to the SettingsSectionHeader and/or the containing SettingsCard or WorkspaceCommandsSettingsRow) so sidebar search/deep-link navigation can scroll to this section just like other top-level sections.
🤖 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/AppDelegate.swift`:
- Around line 6046-6050: The current logic falls back to
mainWindowContexts.values.first when
preferredMainWindowContextForWorkspaceCreation(...) returns nil, which can pick
a stale/orphaned context; update the guard in the workspace-creation path (the
call to preferredMainWindowContextForWorkspaceCreation and usage of
mainWindowContexts) to NOT fallback to mainWindowContexts.values.first and
instead return false immediately when
preferredMainWindowContextForWorkspaceCreation(...) is nil so callers can apply
their own fallback behavior.
- Around line 5859-5873: isFreshLaunch is computed too early (before
ensureInitialMainWindowIfNeeded) causing restored sessions to be treated as
fresh; move the isFreshLaunch calculation to after the call to
ensureInitialMainWindowIfNeeded(shouldActivate:) (or otherwise derive "fresh"
from post-ensure state) so that you check mainWindowContexts.isEmpty only after
any initial window/context creation, then proceed with the existing conditional
that references WorkspaceCommandsStore.shared.defaultCommandID and the context
lookup (mainWindowContexts.values.first(where: { $0.windowId == windowId })).
Ensure no other logic assumes the old pre-ensure isFreshLaunch value.
In `@Sources/cmuxApp.swift`:
- Around line 762-769: The Workspaces Window scene currently just creates a
Window with WorkspaceCommandsSettingsView and never captures or configures its
NSWindow, so it cannot reuse/de-miniaturize an existing window or register it as
an auxiliary window; update the Window creation to accept a configure(window:)
closure that captures the NSWindow and calls
WorkspaceCommandsWindowPresenter.show() (mirroring
SettingsWindowPresenter.show(navigationTarget:)) so the presenter can
de-duplicate and focus an existing window, then register the window identifier
returned with cmuxAuxiliaryWindowIdentifiers; ensure you route the button action
to WorkspaceCommandsWindowPresenter.show() and use
WorkspaceCommandsSettingsView.windowID as the id when registering.
In `@Sources/CmuxConfigExecutor.swift`:
- Around line 533-537: The SSH option "-t" is currently appended after the
destination so it becomes part of the remote command; modify the
argument-building logic in CmuxConfigExecutor (where args, remote.host,
remote.startupCommand and shellQuoteArgument are used) so that when a non-empty
startupCommand exists you append "-t" to args before appending the quoted
remote.host, then append the quoted startupCommand; ensure the "-t" insertion
happens only when startupCommand is present so other call sites keep the
original ordering.
In `@Sources/Settings/WorkspaceCommandsSettingsView.swift`:
- Around line 89-94: The list row doesn't mark the Local/built-in profile as
default when store.defaultCommandID is nil; in WorkspaceCommandsSettingsView
update the isDefault argument passed to WorkspaceCommandListRow (inside the
ForEach over store.commands) so it is true when store.defaultCommandID ==
command.id OR when store.defaultCommandID is nil and the command represents the
local/built-in profile (e.g., command.remote == nil), ensuring the list shows
Local as default after restore/defaults or first launch.
- Around line 368-389: The UI currently leaves command.remote non-nil even when
the host is blank or the port is invalid; update the hostBinding and/or
portBinding handling so that when the host is empty/whitespace or the port is
non-numeric or out of valid SSH range (e.g. not in 1...65535) you clear
command.remote (set it to nil) instead of storing partial/invalid data, and
ensure WorkspaceCommandsStore.asCmuxCommandDefinition() treats a nil
command.remote as non-remote; specifically, after trimming the host in
hostBinding/get-or-set and after parsing/validating the port in portBinding/set,
set command.remote = nil when validation fails, otherwise create/populate
command.remote with the validated host and port.
In `@Sources/Settings/WorkspaceCommandsStore.swift`:
- Around line 264-271: The CmuxRemoteDefinition builder is passing through
whitespace-only remote string fields and sshOptions; trim each string (use
.trimmingCharacters(in: .whitespacesAndNewlines)) and convert empty results to
nil for host, identityFile, and startupCommand when constructing
CmuxRemoteDefinition, and normalize sshOptions by mapping/trimming each entry
then filtering out empty strings and passing nil if the resulting array is empty
(preserve port as-is); apply this normalization in the remote.map block where
CmuxRemoteDefinition(...) is created.
- Around line 164-170: The load() method currently assigns snapshot.userCommands
and snapshot.defaultCommandID directly which can introduce duplicate IDs or an
invalid default; instead, decode StoredSnapshot into a temporary value, sanitize
userCommands by removing or remapping any IDs that conflict with built-in
command IDs and deduplicate entries (preserving later/most-recent items),
validate that the resulting defaultCommandID exists in the sanitized
userCommands (or clear/fallback it to nil/first valid ID), and only then set
suppressPersist = true and assign userCommands and defaultCommandID from these
sanitized values before resetting suppressPersist; reference StoredSnapshot,
load(), userCommands, defaultCommandID and suppressPersist when implementing the
checks.
In `@Sources/Update/UpdateTitlebarAccessory.swift`:
- Around line 399-413: Replace the hard-coded accessibilityDescription strings
on primaryButton and chevronButton with localized strings using
String(localized:defaultValue:); specifically update primaryButton.image
creation to use accessibilityDescription: String(localized:
"titlebar.newWorkspace.accessibility", defaultValue: "New workspace") and update
chevronButton.image creation to use accessibilityDescription: String(localized:
"titlebar.pickWorkspace.accessibility", defaultValue: "Pick workspace command"),
leaving the rest of primaryButton and chevronButton setup unchanged.
- Around line 368-372: The split-button's layout constraints aren't updated when
style changes because updateNSView assigns applyConfig(_:) but never refreshes
the size-based constraints (so NewWorkspaceSplitButtonView keeps the original
buttonSize); modify updateNSView (and the similar update path around the other
block mentioned) to after calling nsView.applyConfig(config) recompute or
refresh the sizing constraints—e.g., call a method on
NewWorkspaceSplitButtonView that recalculates/updates the buttonSize
constraints, invalidates intrinsic content size, and/or recreates the
NSLayoutConstraints so the control re-lays out with the new symbol sizes; ensure
the helper is invoked wherever applyConfig(_:) is used (including the other
block referenced).
In `@Sources/Workspace.swift`:
- Around line 7832-7836: The code sets template.waitAfterCommand =
!closePanesOnInitialCommandExit when preparing resolvedConfigTemplate for
initialTerminalCommand but didCloseTab still unconditionally calls
createReplacementTerminalPanel(), causing single-pane SSH/custom profiles to
respawn a local shell. Update the teardown path (the function didCloseTab) to
consult the pane/tab's resolvedConfigTemplate.waitAfterCommand (or the stored
closePanesOnInitialCommandExit equivalent on the panel state) and skip calling
createReplacementTerminalPanel() when waitAfterCommand == false (i.e., when
closePanesOnInitialCommandExit was true); ensure the flag is stored with the
panel so didCloseTab can access it reliably for the other call sites (the
similar blocks referenced around didCloseTab).
---
Nitpick comments:
In `@Sources/cmuxApp.swift`:
- Around line 6413-6416: The Workspaces settings block (SettingsSectionHeader /
SettingsCard / WorkspaceCommandsSettingsRow) lacks the SettingsSearchIndex
anchors used elsewhere; add the same SettingsSearchIndex registration calls for
the "Workspaces" section (attach the anchor(s) to the SettingsSectionHeader
and/or the containing SettingsCard or WorkspaceCommandsSettingsRow) so sidebar
search/deep-link navigation can scroll to this section just like other top-level
sections.
In `@Sources/CmuxWorkspaceDefinition.swift`:
- Around line 35-40: The decoder currently silently drops runtime-only keys
`remote` and `program` by setting them to nil; instead, modify the Decodable
implementation (the init(from decoder:)) and/or the CodingKeys in
CmuxWorkspaceDefinition so that if the keyed container contains the keys
"remote" or "program" you throw a DecodingError (e.g., dataCorrupted or
keyNotFound) with a clear message; locate the custom init(from:) and add
explicit checks using container.contains(.remote)/container.contains(.program)
and raise an error pointing out that those keys are invalid in cmux.json.
🪄 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: 6bc598cd-45a1-4043-b994-65c6072825af
📒 Files selected for processing (12)
GhosttyTabs.xcodeproj/project.pbxprojSources/AppDelegate.swiftSources/CmuxConfig.swiftSources/CmuxConfigExecutor.swiftSources/CmuxWorkspaceDefinition.swiftSources/Settings/WorkspaceCommandsSettingsView.swiftSources/Settings/WorkspaceCommandsStore.swiftSources/TabManager.swiftSources/Update/UpdateTitlebarAccessory.swiftSources/Workspace.swiftSources/cmuxApp.swiftcmuxTests/CmuxConfigTests.swift
- Show empty-state summary when no user commands exist (built-in Local always populates store.commands, so check userCommands instead). - Treat nil defaultCommandID as Local being the default in the workspace command sidebar so the "Default" badge renders correctly. - Document why the unreachable .always case in the restart switch exists. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
Sources/cmuxApp.swift (1)
762-769:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWorkspaces window still misses Settings-style lifecycle/focus + auxiliary-window registration.
At Lines 762-769 and Lines 7568-7574, the Workspaces scene/presenter still does not capture/configure an
NSWindow(identifier + reuse/de-miniaturize/focus path). As a result,Cmd+Wcan still fall through to panel/workspace close routing because this window is not recognized bycmuxWindowShouldOwnCloseShortcut(...).Suggested patch
`@MainActor` enum WorkspaceCommandsWindowPresenter { + static let windowIdentifier = "cmux.workspaces" private static var openWindow: (`@MainActor` () -> Void)? + private static weak var workspaceCommandsWindow: NSWindow? private static var shouldOpenWhenConfigured = false @@ + static func configure(window: NSWindow) { + workspaceCommandsWindow = window + window.identifier = NSUserInterfaceItemIdentifier(windowIdentifier) + } + static func show() { + if let window = existingWindow() { + focus(window) + return + } guard let openWindow else { shouldOpenWhenConfigured = true return } openWindow() } + + private static func existingWindow() -> NSWindow? { + if let workspaceCommandsWindow, workspaceCommandsWindow.isVisible || workspaceCommandsWindow.isMiniaturized { + return workspaceCommandsWindow + } + return NSApp.windows.first { + $0.identifier?.rawValue == windowIdentifier && ($0.isVisible || $0.isMiniaturized) + } + } + + private static func focus(_ window: NSWindow) { + if window.isMiniaturized { + window.deminiaturize(nil) + } + NSRunningApplication.current.activate(options: [.activateAllWindows]) + window.makeKeyAndOrderFront(nil) + window.orderFrontRegardless() + } }Window( String(localized: "settings.workspaces.windowTitle", defaultValue: "Workspaces"), id: WorkspaceCommandsSettingsView.windowID ) { WorkspaceCommandsSettingsView() + .background(WindowAccessor { window in + WorkspaceCommandsWindowPresenter.configure(window: window) + }) } .defaultSize(width: 760, height: 500) .windowResizability(.contentMinSize)private let cmuxAuxiliaryWindowIdentifiers: Set<String> = [ "cmux.settings", + "cmux.workspaces", "cmux.about",Based on learnings:
SettingsWindowPresenter.show(navigationTarget:)reuses existing Settings windows, deminiaturizes them when needed, and fronts them instead of duplicating.Also applies to: 7549-7575
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 762 - 769, The Workspaces Window must capture and configure its NSWindow like SettingsWindowPresenter.show(navigationTarget:) does: when creating the Window for WorkspaceCommandsSettingsView, obtain the underlying NSWindow, set its identifier to WorkspaceCommandsSettingsView.windowID, detect/reuse an existing window with that identifier (do not create duplicates), deminiaturize and bring it to front (makeKeyAndOrderFront) when showing, and ensure it is registered/recognized by cmuxWindowShouldOwnCloseShortcut(...) (so Cmd+W is owned). Modify the Window creation for WorkspaceCommandsSettingsView and the presenter/scene code to follow the same reuse/fronting/identifier behavior as SettingsWindowPresenter.show(navigationTarget:).Sources/CmuxConfigExecutor.swift (1)
518-541:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove
-tbefore the SSH destination.
sshstops parsing client options at the destination token, so appending-tafterremote.hostmakes it part of the remote command instead of an option. That breaks SSH-backed workspaces with a startup command, and the malformed command is also what gets stored for reconnects. This is the same parsing issue that was previously called out and it still needs to be fixed.Suggested fix
var args: [String] = ["ssh"] if let identityFile = remote.identityFile, !identityFile.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty { args.append("-i") args.append(shellQuoteArgument(expandTildePath(identityFile))) } if let port = remote.port { args.append("-p") args.append(String(port)) } for option in remote.sshOptions ?? [] { let trimmed = option.trimmingCharacters(in: .whitespacesAndNewlines) guard !trimmed.isEmpty else { continue } args.append("-o") args.append(shellQuoteArgument(trimmed)) } + if let startupCommand = remote.startupCommand?.trimmingCharacters(in: .whitespacesAndNewlines), + !startupCommand.isEmpty { + args.append("-t") + } args.append(shellQuoteArgument(remote.host)) if let startupCommand = remote.startupCommand?.trimmingCharacters(in: .whitespacesAndNewlines), !startupCommand.isEmpty { - args.append("-t") args.append(shellQuoteArgument(startupCommand)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/CmuxConfigExecutor.swift` around lines 518 - 541, In buildRemoteTerminalStartupCommand, the "-t" and its startup command are appended after remote.host so SSH treats them as the remote command instead of an option; move the block that appends "-t" and shellQuoteArgument(startupCommand) to occur before appending shellQuoteArgument(remote.host) (i.e., ensure "-t" and the quoted startupCommand are inserted into the args array prior to appending remote.host) so SSH parses "-t" as a client option and the startupCommand is sent as the remote command.
🤖 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/cmuxApp.swift`:
- Around line 6413-6417: The SettingsSectionHeader for the Workspaces section is
missing a settingsSearchAnchor which breaks Settings sidebar jump-to behavior;
update the SettingsSectionHeader(...) call that renders the "Workspaces" section
(the SettingsSectionHeader instance above SettingsCard and
WorkspaceCommandsSettingsRow) to append a .settingsSearchAnchor("workspaces")
(or similarly unique anchor identifier) so the section is discoverable by the
Settings search sidebar.
In `@Sources/CmuxConfigExecutor.swift`:
- Around line 492-499: The WorkspaceRemoteConfiguration is being created with
identityFile: remote.identityFile.map(expandTildePath) which can produce an
empty string for whitespace-only input; normalize such empty/whitespace results
to nil so saved config matches runtime SSH behavior. Update the construction to
call expandTildePath on remote.identityFile and then set identityFile to nil if
the trimmed result is empty (i.e., treat "" or all-whitespace as nil) before
passing into WorkspaceRemoteConfiguration; refer to expandTildePath,
remote.identityFile and WorkspaceRemoteConfiguration to locate and update this
logic.
---
Duplicate comments:
In `@Sources/cmuxApp.swift`:
- Around line 762-769: The Workspaces Window must capture and configure its
NSWindow like SettingsWindowPresenter.show(navigationTarget:) does: when
creating the Window for WorkspaceCommandsSettingsView, obtain the underlying
NSWindow, set its identifier to WorkspaceCommandsSettingsView.windowID,
detect/reuse an existing window with that identifier (do not create duplicates),
deminiaturize and bring it to front (makeKeyAndOrderFront) when showing, and
ensure it is registered/recognized by cmuxWindowShouldOwnCloseShortcut(...) (so
Cmd+W is owned). Modify the Window creation for WorkspaceCommandsSettingsView
and the presenter/scene code to follow the same reuse/fronting/identifier
behavior as SettingsWindowPresenter.show(navigationTarget:).
In `@Sources/CmuxConfigExecutor.swift`:
- Around line 518-541: In buildRemoteTerminalStartupCommand, the "-t" and its
startup command are appended after remote.host so SSH treats them as the remote
command instead of an option; move the block that appends "-t" and
shellQuoteArgument(startupCommand) to occur before appending
shellQuoteArgument(remote.host) (i.e., ensure "-t" and the quoted startupCommand
are inserted into the args array prior to appending remote.host) so SSH parses
"-t" as a client option and the startupCommand is sent as the remote command.
🪄 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: 0c484b56-cab8-42e5-b938-39bc6a1240f4
📒 Files selected for processing (3)
Sources/CmuxConfigExecutor.swiftSources/Settings/WorkspaceCommandsSettingsView.swiftSources/cmuxApp.swift
✅ Files skipped from review due to trivial changes (1)
- Sources/Settings/WorkspaceCommandsSettingsView.swift
- SSH startup: move `-t` before destination; ssh(1) parses options up to destination, otherwise `-t` was being passed to the remote shell. - Normalize blank identityFile to nil before persisting in remote config (matches the SSH builder's blank-as-absent behavior). - WorkspaceCommandsStore.load: sanitize persisted IDs (drop entries that collide with the built-in Local ID; clear orphan defaultCommandID). - WorkspaceCommandsStore.asCmuxCommandDefinition: trim and drop empty remote string fields (host, identityFile, sshOptions, startupCommand). - Settings: validate SSH port to 1-65535 range; reject otherwise. - Titlebar: localize "New Workspace" / "Pick Workspace Command" accessibility descriptions on the AppKit split button. - AppDelegate.runWorkspaceCommandConfig: drop orphan-context fallback — if no preferred main-window context exists, return false rather than running against a stale context. - AppDelegate.bootstrapInitialMainWindow: tighten isFreshLaunch so the user's default workspace command does not overwrite a restored session on launch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/Settings/WorkspaceCommandsStore.swift`:
- Around line 127-130: setDefault(id:) must sanitize the incoming id before
assigning and persisting: in WorkspaceCommandsStore.setDefault(id:) check
whether id is nil or corresponds to an existing entry in userCommands (the
collection used by defaultCommand()); if the id is absent in userCommands treat
it as nil (clear defaultCommandID) and then call persist(); update the logic
around defaultCommandID assignment so orphan UUIDs are never stored/persisted
and defaultCommand() continues to correctly fall back to Local.
- Around line 133-139: The code in defaultNewCommandName currently builds the
numbered fallback with "\(base) \(n)" which hardcodes English ordering/spacing;
replace that concatenation with a localized formatted string. Add a localization
key (e.g. "settings.workspaces.newCommand.numberedName") whose defaultValue is a
format like "%@ %d" (or locale-appropriate ordering), then when creating the
candidate name use String(localized:
"settings.workspaces.newCommand.numberedName", defaultValue: "...", base, n) (or
the appropriate localized formatting API) instead of "\(base) \(n)"; keep the
rest of defaultNewCommandName (existing Set(commands.map(\.name)), loop, etc.)
the same.
🪄 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: 745b8596-af96-4e27-bff0-6c4bd04e7d4b
📒 Files selected for processing (5)
Sources/AppDelegate.swiftSources/CmuxConfigExecutor.swiftSources/Settings/WorkspaceCommandsSettingsView.swiftSources/Settings/WorkspaceCommandsStore.swiftSources/Update/UpdateTitlebarAccessory.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/Update/UpdateTitlebarAccessory.swift
- setDefault(id:) now drops orphan UUIDs that aren't in userCommands (or the built-in Local ID), so AppDelegate's startup gate stops treating stale IDs as an explicit default. - defaultNewCommandName() builds the numbered fallback through a new localized format key (settings.workspaces.newCommand.numberedName) instead of hardcoding English ordering. Addresses CodeRabbit review on PR manaflow-ai#3307. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Author
|
the code changed a lot, I'll have to recreate the PR using a different strategy |
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
Adds a Workspaces section to Settings so you can define named workspace profiles — local shells, custom programs (fish, bash with flags, etc.), or remote SSH targets — and launch them from Cmd-N, the titlebar
+, or the command palette. Closes #3257.Previously the only way to configure these was hand-editing
~/.config/cmux/cmux.json. That file is no longer read for workspace commands; the new UI is the single entry point.What you can do
Localprofile is always pinned at the top.+runs the default; its chevron opens a menu of every profile plus a shortcut to the editor.-ooptions, and an optional startup command./opt/homebrew/bin/fish,/bin/bash -l,/usr/bin/env zsh --no-rcs. Empty falls back to your default shell.server,server 2,server 3, …) by default. The previous "Reuse / Replace / Ask" behaviors remain available per profile.Local.Migration
Workspace commands previously held in
~/.config/cmux/cmux.jsonaren't migrated. Re-enter them in Settings → Workspaces and pick a default.I recorded a youtube video to show it live: https://youtu.be/CktpcczHTww
🤖 Generated with Claude Code
Summary by cubic
Adds a Preferences-based workspace command picker with SSH and custom programs. Replaces the JSON
~/.config/cmux/cmux.jsonworkspace section with aUserDefaultsstore, wired into Cmd‑N, the titlebar +, and first‑window launch. Closes #3257.New Features
Localworkspace is pinned and acts as the fallback default; the store sanitizes persisted IDs and clears orphan defaults.-ooptions, startup command), and a local Program for non-remote shells.+is a split button: primary runs your default; the chevron opens a liveNSMenuof commands plus “Manage Workspaces…”, rebuilt on open with localized accessibility labels.Local) instead of a bare local tab.server,server 2, …); “Reuse”, “Replace”, and “Ask” remain.~, places-tbefore the host) and trims/drops empty remote fields.Local; Settings shows an empty‑state summary when onlyLocalexists.Migration
UserDefaultsundercmux.workspaceCommands.v1; entries in~/.config/cmux/cmux.jsonare not migrated. Re-enter them in Settings → Workspaces and set a default for Cmd‑N/titlebar+.Written for commit e81c721. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Tests