Repository navigation
Conversation
…project management - Workspaces can have child workspaces (max 3 levels deep) - Collapsible hierarchy with color-coded card backgrounds - Templates system for multi-tab project setups with startup commands - Script repository for reusable shell scripts - Scripts menu bar with Run Script, Open Template, Manage Templates/Scripts - Template Manager and Script Manager editor windows - Startup commands persist and re-run on session restore (lightning bolt indicator) - Finder "Open in cmux" service - Context menu: Add Child, Make Child, Raise Level, Run Script - Socket API: group.*, project.open, project.open_template, script.*, template.* - CLI: cmux open command - Prompt-ready detection for reliable command execution via sendInteractiveText Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…tories Names from the socket API and UI are now validated through NameSanitizer before being interpolated into filesystem paths. Rejects directory traversal vectors (/, \, :, ..) to prevent escaping ~/.config/cmux/. Also fixes pre-existing TemplateRepositoryTests to use current WorkspaceTemplate API. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…Only mode Previously fell back to same-UID check when LOCAL_PEERPID returned nil, which is weaker than the descendant-process guarantee cmuxOnly promises. Now rejects outright when ancestry cannot be verified. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace try? with do/catch in save, add, duplicate, and delete operations so users see error messages instead of silent failures. Adds errorMessage property and red error banner to ScriptManagerWindow matching the existing TemplateManagerWindow pattern. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add read-only preferredTabManager to AppDelegate (ported from upstream 6bd9894) to avoid mutation-triggered re-evaluation in menu lookups - Decouple ScriptsMenuContent from activeTabManager parameter; resolve live state only in action closures - Add Equatable conformance + .equatable() to minimize body re-evaluation - Restore .disabled(!isAtPrompt) on Run Script items for proper greyed-out state - Change duplicate icon from doc.on.doc to plus.square.on.square in both Script Manager and Template Manager windows Note: cosmetic highlight flicker in the Run Script submenu persists during active terminal output due to a SwiftUI CommandMenu framework limitation (Scene-level invalidation rebuilds the NSMenu bridge regardless of content stability). Clears up once terminal output settles. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…pacing - Add moveWorkspaceByIndicator to TabManager for cross-level drag-and-drop: top-level reorder, child reorder, promote (child→tier 1), demote (tier 1→child), with depth constraint checks and cycle prevention - Update SidebarTabDropDelegate to use visible workspace list instead of flat tabs array, and call moveWorkspaceByIndicator with indicator info - Add SidebarGroupSpacerDropDelegate for two-zone drop target in the 10px spacer between tier 1 groups (upper half = last child, lower half = tier 1) - Add showsBottomDropIndicator and bottom overlay on TabItemView for correct indicator positioning at group boundaries - Fix showsCenteredTopDropIndicator to suppress top line at group boundaries - Override indicator in updateDropIndicator when planner returns no-op at group boundaries (bottom-of-self at boundary is meaningful) - Add 10px spacing above each tier 1 workspace for visual group separation - Fix shouldShowTopDropIndicator in SidebarEmptyArea to use visible workspaces Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@hirscr 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. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughAdds a parent–child workspace hierarchy with sidebar grouping, template and script repositories and UIs, YAML project config parsing, CLI/socket v2 project/group/script/template commands (including a new Changes
Sequence Diagram(s)sequenceDiagram
participant Finder as User / Finder
participant CLI as CLI (`cmux open`)
participant App as AppDelegate
participant Socket as v2 Socket Server
participant TM as TabManager / WorkspaceGroupManager
participant Template as TemplateRepository
participant Script as StartupScriptRunner
participant UI as Sidebar / TerminalPanel
Finder->>CLI: invoke `cmux open /path`
CLI->>Socket: attempt connect & send `project.open(path)`
alt socket reachable
Socket->>TM: v2ProjectOpen handling
TM->>Template: try parse .cmux.yaml
alt config parsed
Template-->>TM: parsed tab/group defs
TM->>TM: create parent and child workspaces, register hierarchy
TM->>Script: schedule startup scripts (onPromptReady)
else no config
TM->>TM: create single workspace
end
TM-->>Socket: return created workspace ids / dedupe flag
Socket-->>CLI: OK
else socket unreachable
CLI->>App: launch app
CLI->>CLI: wait/retry socket (up to 10s)
CLI->>Socket: retry `project.open`
Socket-->>CLI: OK
end
UI->>Script: terminal becomes prompt-ready
Script->>UI: send interactive startup text
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR adds hierarchical workspace groups (parent-child relationships, up to 3 levels deep), YAML-based project templates, shell scripts, a Key issues found:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as cmux CLI
participant Socket as Socket API
participant TC as TerminalController
participant TM as TabManager
participant GM as WorkspaceGroupManager
participant WS as Workspace
CLI->>Socket: project.open { path }
Socket->>TC: v2ProjectOpen(params)
TC->>TC: check .cmux.yaml / dedupe
TC->>TM: addWorkspace(workingDirectory:)
TM->>GM: registerWorkspaceAsStandalone(id)
TC->>TM: addWorkspace(skipStandaloneRegistration: true) [children]
TM-->>TC: child Workspace
TC->>GM: addChildId(childId, to: parentId)
TC->>WS: startupCommand = command
TC->>TC: scriptRunner.scheduleCommand(command, workspace:, panelId:)
WS-->>WS: panelDidUpdateShellActivityState(.promptIdle)
WS->>WS: onPromptReady[panelId]() → panel.sendInteractiveText(command)
Note over TM,GM: Session Restore
TM->>WS: childWorkspaceIds = restored indices
TM->>GM: items = topLevelWorkspaceIndices
TM->>TC: scriptRunner.scheduleCommand (re-run startup commands)
Reviews (1): Last reviewed commit: "feat: add hierarchy-aware drag-and-drop ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if !child.children.isEmpty { | ||
| createTemplateChildren( | ||
| child.children, | ||
| parentId: ws.id, | ||
| workingDirectory: workingDirectory, | ||
| scriptRunner: scriptRunner | ||
| ) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
createTemplateChildren
createTemplateChildren recurses into arbitrarily deep template hierarchies without checking the max-depth invariant (3 levels). groupManager.addChildId just does parent.childWorkspaceIds.append(childId) — no depth gate. A template YAML with 4+ nesting levels will create workspaces at depth 4+, silently violating the invariant documented on WorkspaceGroupManager.
The interactive path enforces the limit via addChildWorkspace, which guards on depth < 3, but this private helper bypasses that entirely. The recursive v2ProjectOpenTemplate helper in TerminalControllerProjectCommands.swift has the same issue.
Consider adding a depth guard before each recursive call:
if !child.children.isEmpty {
let newDepth = groupManager.depthOf(workspaceId: ws.id)
if newDepth < 3 {
createTemplateChildren(
child.children,
parentId: ws.id,
workingDirectory: workingDirectory,
scriptRunner: scriptRunner
)
}
}| ws.isCollapsed = true | ||
| tm.items = tm.groupManager.items |
There was a problem hiding this comment.
v2GroupCollapse/v2GroupExpand bypass the hasChildren guard
toggleCollapsed in WorkspaceGroupManager checks ws.hasChildren before touching isCollapsed. These socket API handlers set ws.isCollapsed = true/false directly, so they will mark a leaf workspace as collapsed/expanded — inconsistent state that other code inspects (e.g. visibleWorkspaces checks ws.hasChildren && !ws.isCollapsed).
For v2GroupCollapse, consider:
| ws.isCollapsed = true | |
| tm.items = tm.groupManager.items | |
| ws.isCollapsed = ws.hasChildren ? true : ws.isCollapsed | |
| tm.items = tm.groupManager.items |
And likewise for v2GroupExpand:
| ws.isCollapsed = true | |
| tm.items = tm.groupManager.items | |
| ws.isCollapsed = false | |
| tm.items = tm.groupManager.items |
The expand case is harmless, but the collapse case should at minimum skip the mutation for childless workspaces.
| func seedDefaultTemplates() { | ||
| for (name, content) in Self.defaultTemplates { | ||
| guard !hasTemplate(named: name) else { continue } | ||
| try? ensureDirectoryExists() | ||
| let path = directory.appendingPathComponent("\(name).yaml") | ||
| try? content.write(to: path, atomically: true, encoding: .utf8) | ||
| } | ||
| } |
There was a problem hiding this comment.
seedDefaultTemplates bypasses NameSanitizer
Every other method in TemplateRepository routes the name through NameSanitizer.sanitize before building the path. seedDefaultTemplates appends the name directly:
let path = directory.appendingPathComponent("\(name).yaml")The current default names are safe, but this is an inconsistency — if a future default template name ever contained a problematic character, there would be no validation. Use saveTemplate(named:rawYaml:) instead, which already handles sanitization and directory creation:
| func seedDefaultTemplates() { | |
| for (name, content) in Self.defaultTemplates { | |
| guard !hasTemplate(named: name) else { continue } | |
| try? ensureDirectoryExists() | |
| let path = directory.appendingPathComponent("\(name).yaml") | |
| try? content.write(to: path, atomically: true, encoding: .utf8) | |
| } | |
| } | |
| func seedDefaultTemplates() { | |
| for (name, content) in Self.defaultTemplates { | |
| guard !hasTemplate(named: name) else { continue } | |
| try? saveTemplate(named: name, rawYaml: content) | |
| } | |
| } |
| } catch { | ||
| // Fall through to single-workspace fallback on parse error | ||
| } |
There was a problem hiding this comment.
.cmux.yaml parse error discards user-visible failure
When a .cmux.yaml file exists but fails to parse, the error is silently swallowed and the command falls through to create a plain single-workspace. From the user's perspective the project appears to open normally, with no indication that their config file is broken.
At a minimum, the error should be surfaced in the response or logged:
} catch {
// Fall through to single-workspace fallback on parse error
// TODO: surface parse error to the caller so the user knows
// their .cmux.yaml has a problem.
// return .err(code: "config_parse_error", message: error.localizedDescription, data: nil)
}Consider returning a warning field in the success response, e.g. "config_warning": error.localizedDescription, so callers can display it without breaking the open flow.
| /// Scripts are NOT run during session restore to avoid duplicate execution. | ||
| @MainActor |
There was a problem hiding this comment.
The class comment says:
Scripts are NOT run during session restore to avoid duplicate execution.
But TabManager explicitly calls scriptRunner.scheduleCommand(command, workspace:, panelId:) for every workspace with a startupCommand during restoreFromSnapshot — which IS session restore. The PR description also confirms this is intentional ("re-run on session restore"). The stale comment will mislead future readers.
| /// Scripts are NOT run during session restore to avoid duplicate execution. | |
| @MainActor | |
| /// Manages startup script/command execution for workspaces created from project configs or templates. | |
| /// Startup *commands* are re-run on session restore (`startupCommand` field). | |
| /// File-based scripts fetched from `ScriptRepository` respect the `shouldRunScript(isRestore:)` gate. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
27 issues found across 43 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/TerminalControllerProjectCommands.swift">
<violation number="1" location="Sources/TerminalControllerProjectCommands.swift:260">
P2: Project dedupe uses `currentDirectory`, which is a mutable runtime value updated as users `cd`; this can fail to match an existing project root and create duplicate workspaces.</violation>
</file>
<file name="Sources/GroupHeaderView.swift">
<violation number="1" location="Sources/GroupHeaderView.swift:7">
P2: Functional `Color(hex:)` parsing was added to a file marked as temporary/cleanup-only, creating hidden coupling and future regression risk if that placeholder file is removed.</violation>
</file>
<file name="Sources/ScriptsMenu.swift">
<violation number="1" location="Sources/ScriptsMenu.swift:52">
P2: Run Script action lacks a live prompt-state guard and may send script text when the terminal is no longer at prompt.</violation>
</file>
<file name="Sources/ScriptManagerViewModel.swift">
<violation number="1" location="Sources/ScriptManagerViewModel.swift:69">
P2: Unsaved edits can be lost when add/duplicate/reload/select actions call loadScript without checking isDirty, overwriting editorText and resetting the dirty flag.</violation>
<violation number="2" location="Sources/ScriptManagerViewModel.swift:118">
P1: Script load failures are silently coerced to empty content, masking read errors and risking accidental overwrite.</violation>
</file>
<file name="Sources/cmuxApp.swift">
<violation number="1" location="Sources/cmuxApp.swift:984">
P2: activeTabManager no longer synchronizes the active main-window context, so menu actions can target a stale/root TabManager in multi-window sessions.</violation>
</file>
<file name="Sources/StartupScriptRunner.swift">
<violation number="1" location="Sources/StartupScriptRunner.swift:46">
P2: Startup scripts can be sent to the currently focused terminal instead of the scheduled panelId if focus changes before the prompt-ready/fallback callback fires.</violation>
</file>
<file name="Sources/AppDelegate.swift">
<violation number="1" location="Sources/AppDelegate.swift:11376">
P2: Finder service selects the target window via listMainWindowSummaries().first, which is built from Dictionary values and therefore unordered. With multiple windows open, this can route Finder service opens to an arbitrary background window instead of the key/main window.</violation>
<violation number="2" location="Sources/AppDelegate.swift:11383">
P2: Finder Services return immediately after creating a cold-start window, so only the first selected item is opened and template services never apply the template on cold start.</violation>
</file>
<file name="Sources/TemplateYamlParser.swift">
<violation number="1" location="Sources/TemplateYamlParser.swift:217">
P2: Legacy `startupScript` is parsed but discarded when building `TemplateNode` children, so legacy templates lose their startup commands during migration/serialization.</violation>
</file>
<file name="cmuxTests/WorkspaceGroupManagerTests.swift">
<violation number="1" location="cmuxTests/WorkspaceGroupManagerTests.swift:213">
P2: Depth-constraint test does not execute or assert the blocked-indent path, so max-depth enforcement is not actually validated.</violation>
</file>
<file name="Sources/TemplateManagerWindow.swift">
<violation number="1" location="Sources/TemplateManagerWindow.swift:73">
P2: Unsaved-changes flow closes or switches immediately after save without checking for save failure, so a failed save can still discard the user’s edits/context.</violation>
</file>
<file name="Sources/TerminalControllerGroupCommands.swift">
<violation number="1" location="Sources/TerminalControllerGroupCommands.swift:141">
P2: v2GroupAddWorkspace reports max_depth for any addChildWorkspace failure, including missing parent workspaces, which misleads API clients about the actual error.</violation>
</file>
<file name="Sources/ScriptManagerWindow.swift">
<violation number="1" location="Sources/ScriptManagerWindow.swift:41">
P2: Calling viewModel.reload() unconditionally when showing the window can discard unsaved edits if the user reopens the Script Manager while it’s already open. There’s no dirty-state guard or prompt in show(), so in-memory edits may be overwritten by disk content.</violation>
<violation number="2" location="Sources/ScriptManagerWindow.swift:118">
P2: `pendingSelection` drives the List selection, but add/duplicate mutate `selectedName` without updating `pendingSelection`. After add/duplicate, the editor switches to the new script while the list selection stays on the old item, and later selection-change prompts can be out of sync.</violation>
</file>
<file name="Sources/Workspace.swift">
<violation number="1" location="Sources/Workspace.swift:5090">
P2: onPromptReady callbacks are only removed on prompt-idle, but panel close paths clear other per-panel maps without clearing onPromptReady. Closing a panel before prompt-idle leaves stale retained closures.</violation>
</file>
<file name="Sources/WorkspaceGroupManager.swift">
<violation number="1" location="Sources/WorkspaceGroupManager.swift:31">
P2: `addChildId` bypasses documented hierarchy invariants by appending children without depth/structure validation, while callers rely on it for max-depth enforcement.</violation>
</file>
<file name="Sources/CmuxConfigParser.swift">
<violation number="1" location="Sources/CmuxConfigParser.swift:121">
P2: Top-level tabs parsing is not restricted to root indentation, so nested group `tabs:` blocks can be misparsed as root tabs.</violation>
<violation number="2" location="Sources/CmuxConfigParser.swift:233">
P2: Group-level workingDirectory is parsed but never applied to group tab definitions, so the YAML workingDirectory for a group has no effect.</violation>
<violation number="3" location="Sources/CmuxConfigParser.swift:235">
P2: Nested groups are always flagged as max-depth exceeded because parseGroupsList never uses currentDepth or recurses into child groups, so valid depth-2 hierarchies are rejected.</violation>
</file>
<file name="Sources/ContentView.swift">
<violation number="1" location="Sources/ContentView.swift:8369">
P2: TabItemView now receives a visible index, but move/selection logic still treats `index` as a flat `tabManager.tabs` index. When groups are collapsed, visible indices can diverge from flat order, so moveBy and shift-selection can reorder/select the wrong workspaces.</violation>
</file>
<file name="Sources/TerminalControllerScriptTemplateCommands.swift">
<violation number="1" location="Sources/TerminalControllerScriptTemplateCommands.swift:66">
P2: `v2TemplateGet` incorrectly collapses all repository errors into `not_found`, masking parse and I/O failures as missing templates.</violation>
<violation number="2" location="Sources/TerminalControllerScriptTemplateCommands.swift:116">
P2: Template deserialization is unbounded recursive on untrusted request data, allowing excessively deep/large trees to cause stack/memory/CPU exhaustion.</violation>
</file>
<file name="CLI/cmux.swift">
<violation number="1" location="CLI/cmux.swift:2515">
P2: `openProjectPath` bypasses socket authentication, so `cmux open` can fail in password-protected socket mode.</violation>
</file>
<file name="Sources/TabManager.swift">
<violation number="1" location="Sources/TabManager.swift:1941">
P2: Template import recursively creates child workspaces without enforcing the documented 3-level hierarchy limit, so deep templates can bypass the depth constraint enforced elsewhere (e.g., addChildWorkspace/drag-drop).</violation>
<violation number="2" location="Sources/TabManager.swift:2315">
P1: Closing a parent workspace can delete children and then abort closing the parent due to the post-recursion minimum-tab guard.</violation>
<violation number="3" location="Sources/TabManager.swift:5234">
P2: Legacy/no-layout snapshot restore rebuilds sidebar layout by registering workspaces without clearing WorkspaceGroupManager.items, so stale IDs from the pre-restore state can persist and mix with restored IDs. This can leave orphan/duplicate sidebar entries when topLevelWorkspaceIndices is absent.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| // MARK: - Private | ||
|
|
||
| private func loadScript(named name: String) { | ||
| editorText = repo.getScript(named: name) ?? "" |
There was a problem hiding this comment.
P1: Script load failures are silently coerced to empty content, masking read errors and risking accidental overwrite.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/ScriptManagerViewModel.swift, line 118:
<comment>Script load failures are silently coerced to empty content, masking read errors and risking accidental overwrite.</comment>
<file context>
@@ -0,0 +1,122 @@
+ // MARK: - Private
+
+ private func loadScript(named name: String) {
+ editorText = repo.getScript(named: name) ?? ""
+ loadedText = editorText
+ isDirty = false
</file context>
| } | ||
|
|
||
| // After closing children, recheck count | ||
| guard tabs.count > 1 else { return } |
There was a problem hiding this comment.
P1: Closing a parent workspace can delete children and then abort closing the parent due to the post-recursion minimum-tab guard.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/TabManager.swift, line 2315:
<comment>Closing a parent workspace can delete children and then abort closing the parent due to the post-recursion minimum-tab guard.</comment>
<file context>
@@ -2090,6 +2302,18 @@ class TabManager: ObservableObject {
+ }
+
+ // After closing children, recheck count
+ guard tabs.count > 1 else { return }
+
sentryBreadcrumb("workspace.close", data: ["tabCount": tabs.count - 1])
</file context>
| for wsId in tm.items { | ||
| guard let ws = tm.workspace(for: wsId), | ||
| ws.hasChildren else { continue } | ||
| let wsPath = URL(fileURLWithPath: ws.currentDirectory) |
There was a problem hiding this comment.
P2: Project dedupe uses currentDirectory, which is a mutable runtime value updated as users cd; this can fail to match an existing project root and create duplicate workspaces.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/TerminalControllerProjectCommands.swift, line 260:
<comment>Project dedupe uses `currentDirectory`, which is a mutable runtime value updated as users `cd`; this can fail to match an existing project root and create duplicate workspaces.</comment>
<file context>
@@ -0,0 +1,271 @@
+ for wsId in tm.items {
+ guard let ws = tm.workspace(for: wsId),
+ ws.hasChildren else { continue }
+ let wsPath = URL(fileURLWithPath: ws.currentDirectory)
+ .resolvingSymlinksInPath().path
+ if wsPath == canonicalPath {
</file context>
|
|
||
| import SwiftUI | ||
|
|
||
| extension Color { |
There was a problem hiding this comment.
P2: Functional Color(hex:) parsing was added to a file marked as temporary/cleanup-only, creating hidden coupling and future regression risk if that placeholder file is removed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/GroupHeaderView.swift, line 7:
<comment>Functional `Color(hex:)` parsing was added to a file marked as temporary/cleanup-only, creating hidden coupling and future regression risk if that placeholder file is removed.</comment>
<file context>
@@ -0,0 +1,17 @@
+
+import SwiftUI
+
+extension Color {
+ init?(hex: String) {
+ let hex = hex.trimmingCharacters(in: .init(charactersIn: "#"))
</file context>
| let color = dict["color"] as? String | ||
| let command = dict["command"] as? String | ||
| let childDicts = dict["children"] as? [[String: Any]] ?? [] | ||
| let children = childDicts.map { deserializeTemplateNode($0) } |
There was a problem hiding this comment.
P2: Template deserialization is unbounded recursive on untrusted request data, allowing excessively deep/large trees to cause stack/memory/CPU exhaustion.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/TerminalControllerScriptTemplateCommands.swift, line 116:
<comment>Template deserialization is unbounded recursive on untrusted request data, allowing excessively deep/large trees to cause stack/memory/CPU exhaustion.</comment>
<file context>
@@ -0,0 +1,119 @@
+ let color = dict["color"] as? String
+ let command = dict["command"] as? String
+ let childDicts = dict["children"] as? [[String: Any]] ?? []
+ let children = childDicts.map { deserializeTemplateNode($0) }
+ return TemplateNode(title: title, color: color, command: command, children: children)
+ }
</file context>
| let result = serializeTemplateNode(template.root) | ||
| return .ok(["name": name, "root": result]) | ||
| } catch { | ||
| return .err(code: "not_found", message: "Template '\(name)' not found", data: nil) |
There was a problem hiding this comment.
P2: v2TemplateGet incorrectly collapses all repository errors into not_found, masking parse and I/O failures as missing templates.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/TerminalControllerScriptTemplateCommands.swift, line 66:
<comment>`v2TemplateGet` incorrectly collapses all repository errors into `not_found`, masking parse and I/O failures as missing templates.</comment>
<file context>
@@ -0,0 +1,119 @@
+ let result = serializeTemplateNode(template.root)
+ return .ok(["name": name, "root": result])
+ } catch {
+ return .err(code: "not_found", message: "Template '\(name)' not found", data: nil)
+ }
+ }
</file context>
| throw CLIError(message: "Path does not exist: \(resolved)") | ||
| } | ||
|
|
||
| let client = SocketClient(path: socketPath) |
There was a problem hiding this comment.
P2: openProjectPath bypasses socket authentication, so cmux open can fail in password-protected socket mode.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At CLI/cmux.swift, line 2515:
<comment>`openProjectPath` bypasses socket authentication, so `cmux open` can fail in password-protected socket mode.</comment>
<file context>
@@ -2490,6 +2497,45 @@ struct CMUXCLI {
+ throw CLIError(message: "Path does not exist: \(resolved)")
+ }
+
+ let client = SocketClient(path: socketPath)
+ if (try? client.connect()) == nil {
+ client.close()
</file context>
| } | ||
| } | ||
|
|
||
| if !child.children.isEmpty { |
There was a problem hiding this comment.
P2: Template import recursively creates child workspaces without enforcing the documented 3-level hierarchy limit, so deep templates can bypass the depth constraint enforced elsewhere (e.g., addChildWorkspace/drag-drop).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/TabManager.swift, line 1941:
<comment>Template import recursively creates child workspaces without enforcing the documented 3-level hierarchy limit, so deep templates can bypass the depth constraint enforced elsewhere (e.g., addChildWorkspace/drag-drop).</comment>
<file context>
@@ -1817,6 +1862,93 @@ class TabManager: ObservableObject {
+ }
+ }
+
+ if !child.children.isEmpty {
+ createTemplateChildren(
+ child.children,
</file context>
| tabs = newTabs | ||
| selectedTabId = newSelectedId | ||
|
|
||
| // Rebuild sidebar layout from snapshot |
There was a problem hiding this comment.
P2: Legacy/no-layout snapshot restore rebuilds sidebar layout by registering workspaces without clearing WorkspaceGroupManager.items, so stale IDs from the pre-restore state can persist and mix with restored IDs. This can leave orphan/duplicate sidebar entries when topLevelWorkspaceIndices is absent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/TabManager.swift, line 5234:
<comment>Legacy/no-layout snapshot restore rebuilds sidebar layout by registering workspaces without clearing WorkspaceGroupManager.items, so stale IDs from the pre-restore state can persist and mix with restored IDs. This can leave orphan/duplicate sidebar entries when topLevelWorkspaceIndices is absent.</comment>
<file context>
@@ -4963,10 +5212,41 @@ extension TabManager {
tabs = newTabs
selectedTabId = newSelectedId
+
+ // Rebuild sidebar layout from snapshot
+ if let topLevelIndices = snapshot.topLevelWorkspaceIndices {
+ groupManager.items = topLevelIndices.compactMap { index in
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (21)
CLI/cmux.swift-2515-2523 (1)
2515-2523:⚠️ Potential issue | 🟠 Major
openProjectPathskips socket authentication.This helper connects and immediately sends
project.open, but never callsauthenticateClientIfNeeded. On authenticated sockets,cmux open ...will fail even though the rest of the CLI already resolves--password/ saved passwords viaconnectClient(...).🔐 Suggested fix
- private func openProjectPath(_ path: String, socketPath: String, windowId: String?) throws { + private func openProjectPath( + _ path: String, + socketPath: String, + windowId: String?, + explicitPassword: String? + ) throws { let resolved = resolvePath(path) var isDir: ObjCBool = false let exists = FileManager.default.fileExists(atPath: resolved, isDirectory: &isDir) @@ - let client = SocketClient(path: socketPath) - if (try? client.connect()) == nil { - client.close() - try launchApp() - let launchedClient = try SocketClient.waitForConnectableSocket(path: socketPath, timeout: 10) - defer { launchedClient.close() } - var params: [String: Any] = ["path": directory] - if let windowId { params["window_id"] = windowId } - let response = try launchedClient.sendV2(method: "project.open", params: params) - let groupId = (response["group_id"] as? String) ?? "" - if !groupId.isEmpty { print("OK \(groupId)") } - try activateApp() - return - } + let client = try connectClient( + socketPath: socketPath, + explicitPassword: explicitPassword, + launchIfNeeded: true + ) defer { client.close() } var params: [String: Any] = ["path": directory] if let windowId { params["window_id"] = windowId } let response = try client.sendV2(method: "project.open", params: params) let groupId = (response["group_id"] as? String) ?? "" if !groupId.isEmpty { print("OK \(groupId)") } try activateApp() }Callers at Lines 1409 and 1415 should pass
socketPasswordArg.Also applies to: 2531-2533
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 2515 - 2523, The openProjectPath helper is creating a SocketClient and sending "project.open" without performing authentication, so update openProjectPath to call authenticateClientIfNeeded (or reuse connectClient) after obtaining the connected SocketClient (both for the initial client.connect() path and the launchedClient from SocketClient.waitForConnectableSocket) and before calling sendV2; ensure callers that invoke openProjectPath (they currently pass socketPasswordArg at the call sites mentioned) forward the socketPasswordArg through so authenticateClientIfNeeded has the password to authenticate the socket.CLI/cmux.swift-1406-1410 (1)
1406-1410:⚠️ Potential issue | 🟠 MajorParse
openflags before selecting the path.This branch runs before subcommand-help handling and never parses its own args, so
cmux open --helptries to open a literal--helppath, andcmux open /repo --window ...silently drops the trailing--window. ParsecommandArgshere (or move this block below help dispatch) before picking the path.💡 Suggested direction
- // Explicit `open` command: cmux open /path/to/folder [--window <id>] if command == "open" { - let openPath = commandArgs.first ?? "." - try openProjectPath(openPath, socketPath: resolvedSocketPath, windowId: windowId) + if commandArgs.contains("--help") || commandArgs.contains("-h") { + print(subcommandUsage("open") ?? "Usage: cmux open <path> [--window <id>]") + return + } + let (openWindowOpt, rem0) = parseOption(commandArgs, name: "--window") + let positional = rem0.filter { $0 != "--" } + if let extra = positional.dropFirst().first { + throw CLIError(message: "open: unexpected argument '\(extra)'") + } + let openPath = positional.first ?? "." + try openProjectPath( + openPath, + socketPath: resolvedSocketPath, + windowId: openWindowOpt ?? windowId + ) return }You’ll also want a matching
subcommandUsage("open")/usage()entry so the help path stays discoverable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 1406 - 1410, The open-branch handles "command == \"open\"" before parsing flags so commandArgs like "--help" or "--window" are treated as the path; change this to parse the open subcommand's flags from commandArgs first (or move the block below the global help dispatch), extract the path only after flag parsing, and then call openProjectPath(openPath, socketPath: resolvedSocketPath, windowId: windowId); also add a matching subcommandUsage("open") (and a usage() entry) so "cmux open --help" shows the open help instead of attempting to open a literal path.Sources/CmuxConfigParserGroups.swift-49-53 (1)
49-53:⚠️ Potential issue | 🟠 MajorNested list lines can be misparsed as new tabs.
On Line 49, any
-starts a new tab entry regardless of indentation. That breaks valid nested lists within a tab block (e.g., commands arrays), because deeper-indented- ...lines get flushed into separate tabs.Suggested fix
var tabsSectionIndent = 0 var entryIndent = 0 + var tabItemIndent: Int? var currentEntryLines: [String] = [] @@ - if trimmed.hasPrefix("- ") { + if trimmed.hasPrefix("- "), (tabItemIndent == nil || indent == tabItemIndent) { + if tabItemIndent == nil { tabItemIndent = indent } flushEntry() currentEntryLines.append(String(trimmed.dropFirst(2))) entryIndent = indent } else if indent > entryIndent { currentEntryLines.append(trimmed) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/CmuxConfigParserGroups.swift` around lines 49 - 53, The parser currently starts a new tab whenever a line begins with "- " regardless of indentation; update the conditional around trimmed.hasPrefix("- ") in the block that calls flushEntry() so it only starts a new entry when the dash is at the same indentation level as the existing entry (e.g., require indent == entryIndent or otherwise ensure indent is not greater than entryIndent) to avoid treating deeper-indented nested list items as new tabs; adjust the condition that uses trimmed, indent, entryIndent and the flushEntry()/currentEntryLines logic accordingly.Sources/AppDelegate.swift-11400-11412 (1)
11400-11412:⚠️ Potential issue | 🟠 MajorThe dedupe check misses most already-open workspaces.
This loop only scans
tm.itemsand immediately skips anything without children. Reopening an existing plain workspace—or even selecting two files from the same directory—still creates duplicates. The lookup needs to scan every workspace acrossallTabManagers().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 11400 - 11412, The dedupe loop only iterates tm.items and skips workspaces without children (ws.hasChildren), so plain workspaces are missed; change the lookup to iterate every TabManager from allTabManagers() and examine each manager's items (calling workspace(for:) on each) comparing their resolved currentDirectory to canonicalPath, and when found set tm.selectedTabId = ws.id and break; ensure you remove the ws.hasChildren guard so plain workspaces are considered.Sources/AppDelegate.swift-11379-11383 (1)
11379-11383:⚠️ Potential issue | 🟠 MajorCold-start bootstrap returns too early.
When no window exists, both branches return immediately after creating the first window.
openInCmuxdrops the rest of the selection, andopenInCmuxBuilder/Fixeralso skipstm.openTemplate(...)for that first cold-start open.Also applies to: 11451-11458
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 11379 - 11383, The cold-start branch returns immediately after creating the first window, causing openInCmux/openInCmuxBuilder/openInCmuxFixer to drop remaining items and skip tm.openTemplate for the first item; update the logic around resolvedDirectoryPath(for:), createMainWindow(initialWorkingDirectory:), and the surrounding conditional so you do not return after creating the initial window—instead allow the method to continue processing the full items collection (call openInCmux / openInCmuxBuilder / openInCmuxFixer for each item and ensure tm.openTemplate(...) is invoked where appropriate) so the rest of the selection is handled and templates run on cold start.Sources/AppDelegate.swift-11365-11372 (1)
11365-11372:⚠️ Potential issue | 🟠 MajorMark Finder-service opens as explicit startup intents.
Unlike the other external-open entry points in this file, these handlers never call
prepareForExplicitOpenIntentAtStartup(). On a cold launch,registerMainWindow(...)can still restore the previous session and overwrite the folder/template the service was asked to open.Minimal fix
`@objc` func openInCmux( _ pasteboard: NSPasteboard, userData: String, error: AutoreleasingUnsafeMutablePointer<NSString> ) { + prepareForExplicitOpenIntentAtStartup() guard let items = pasteboard.readObjects(forClasses: [NSURL.self], options: [ .urlReadingFileURLsOnly: true ]) as? [URL] else { return }private func openInCmuxWithTemplate( _ pasteboard: NSPasteboard, templateName: String, error: AutoreleasingUnsafeMutablePointer<NSString> ) { + prepareForExplicitOpenIntentAtStartup() guard let items = pasteboard.readObjects(forClasses: [NSURL.self], options: [ .urlReadingFileURLsOnly: true ]) as? [URL] else { return }Also applies to: 11442-11449
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 11365 - 11372, The openInCmux handler (and the other Finder-service handler at the nearby block) must mark these as explicit startup intents by calling prepareForExplicitOpenIntentAtStartup() before any session-restore or window registration occurs; modify openInCmux (and the similar handler around lines 11442-11449) to invoke prepareForExplicitOpenIntentAtStartup() at the start of the method (before reading the pasteboard or calling registerMainWindow/any session restore logic) so the explicit open intent wins over restored state.Sources/CmuxConfigParser.swift-120-124 (1)
120-124:⚠️ Potential issue | 🟠 MajorRestrict
parseTabsListto the top-leveltabs:block.Line 121 currently enters the first
tabs:it sees, even when that key is nested undergroups:. In configs where a group appears before the roottabs:section, those child tabs get parsed as top-level tabs and the real root section is skipped or duplicated.🔧 Suggested fix
- if trimmed == "\(key):" || trimmed.hasPrefix("\(key):") { + if indent == 0 && (trimmed == "\(key):" || trimmed.hasPrefix("\(key):")) { inSection = true sectionIndent = indent }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/CmuxConfigParser.swift` around lines 120 - 124, parseTabsList is entering any "tabs:" it encounters including nested ones (e.g., under groups:); restrict it to only open the top-level tabs block by checking indentation before setting inSection/sectionIndent. Update the logic in parseTabsList (the block that sets inSection and sectionIndent when trimmed == "\(key):" or hasPrefix) to only set inSection = true when the indent indicates a top-level key (e.g., indent == 0 or matches the expected root indent) or when sectionIndent is not already set, so nested "tabs:" under groups are ignored and only the true root "tabs:" starts parsing.Sources/TerminalControllerGroupCommands.swift-153-163 (1)
153-163:⚠️ Potential issue | 🟠 MajorDon’t report success when no workspace was actually promoted.
Line 158 uses
parentWorkspace(of:)as the only existence check, so Line 163 returnsremoved: truefor three different cases: the workspace was promoted, the workspace ID does not exist, or the workspace was already top-level. Please guardtm.workspace(for: wsId)first and return a distinct result when there is no parent to remove.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalControllerGroupCommands.swift` around lines 153 - 163, The current handler in v2MainSync returns removed: true even when the workspace doesn't exist or is already top-level; first validate the workspace exists by calling tm.workspace(for: wsId) (after v2ResolveTabManager) and if it returns nil return V2CallResult.err/code or a distinct result indicating "workspace_not_found"; then check tm.groupManager.parentWorkspace(of: wsId) and only perform tm.groupManager.removeChildId(wsId, from: parent.id), tm.groupManager.registerWorkspaceAsStandalone(wsId) and tm.items = tm.groupManager.items when a parent is present; otherwise return a clear non-success result (e.g., removed: false or a specific error code like "no_parent") instead of .ok(["removed": true]).Sources/TerminalControllerGroupCommands.swift-18-23 (1)
18-23:⚠️ Potential issue | 🟠 MajorValidate and normalize
coloron both create and set-color.Lines 21-22 and 127 accept raw values as-is, and
v2GroupSetColoralso treats a present-but-invalidcolorasnil. That diverges from the existing workspace color contract: malformed input should returninvalid_params, and valid input should be normalized to uppercase#RRGGBBbefore storing.Based on learnings, workspace color APIs in this repo must validate with
WorkspaceTabColorSettings.normalizedHex(...)and reject present-but-invalid values instead of silently accepting them.Also applies to: 114-128
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalControllerGroupCommands.swift` around lines 18 - 23, The workspace color handling must validate and normalize incoming color strings using WorkspaceTabColorSettings.normalizedHex(...) instead of accepting raw values; update the create handler (where ws.customColor is set) and the v2GroupSetColor handler to (1) if "color" is present but WorkspaceTabColorSettings.normalizedHex(color) returns nil, respond with an invalid_params error, and (2) if it returns a value, store that normalized uppercase "#RRGGBB" result into ws.customColor. Ensure you only reject when the field is present and malformed (not when omitted).Sources/ScriptsMenu.swift-31-37 (1)
31-37:⚠️ Potential issue | 🟠 MajorDrive “Run Script” from live prompt state, and re-check it before sending.
Line 31 snapshots
isFocusedPanelAtPromptinto a local during menu construction, but nothing in this view actually observes prompt changes. That means the submenu can stay incorrectly enabled/disabled after the shell state flips, and Line 52 still injects the script without a final prompt check. Please revalidate prompt readiness insiderunScript(named:), and only use.disabledwith state that will actually invalidate this Commands view.Also applies to: 45-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ScriptsMenu.swift` around lines 31 - 37, The menu currently snapshots isFocusedPanelAtPrompt into a local (let isAtPrompt) so the submenu enablement can become stale; instead either bind .disabled to a state that actually updates (e.g. a computed property or observed value from the active tab manager) or remove the disabled guard from the static snapshot, and — critically — add a fresh readiness check at the start of runScript(named:) that queries Self.resolveActiveTabManager()?.selectedTab?.isFocusedPanelAtPrompt and returns (or shows an error) if false before injecting the script; update references: isFocusedPanelAtPrompt, runScript(named:), Menu and the .disabled usage so the UI is reactive and the final call always revalidates the prompt state.Sources/WorkspaceGroupManager.swift-48-55 (1)
48-55:⚠️ Potential issue | 🟠 MajorImplement explicit cascade removal in
removeWorkspaceto match documented invariant.The class invariant (line 11) states "No orphaned children — removing a parent cascades to children," but
removeWorkspaceonly removes the workspace fromitemsand its parent'schildWorkspaceIds. It does not recursively remove descendants.While the current code works in practice (TabManager removes from
tabsfirst, implicitly deallocating the workspace and its children), this relies on caller discipline rather than enforcing the invariant withinremoveWorkspace. For robustness and proper separation of concerns, explicitly implement recursive removal:Proposed fix for recursive removal
/// Remove a workspace from the sidebar entirely (top-level and as child). + /// Also recursively removes all descendants. func removeWorkspace(_ workspaceId: UUID) { + // First, recursively remove all descendants + let descendants = allDescendantIds(of: workspaceId) + for descendantId in descendants { + items.removeAll { $0 == descendantId } + } + items.removeAll { $0 == workspaceId } // Also remove from any parent's childWorkspaceIds if let parent = parentWorkspace(of: workspaceId) { parent.childWorkspaceIds.removeAll { $0 == workspaceId } } }Also add a test case for multi-level cascade:
testRemoveWorkspaceCascadesToGrandchildrento verify descendants are removed fromitemswhen a parent with children is removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/WorkspaceGroupManager.swift` around lines 48 - 55, removeWorkspace currently only deletes the single workspace id from items and the immediate parent's childWorkspaceIds, but does not enforce the class invariant "No orphaned children" by cascading deletions; update removeWorkspace(_ workspaceId: UUID) to recursively collect and remove all descendant workspace IDs (e.g., traverse childWorkspaceIds of the workspace found via parentWorkspace(of:) or a lookup for the Workspace object) and remove them from items and their parent's childWorkspaceIds as well, ensuring childWorkspaceIds are cleared for removed parents; also add a unit test named testRemoveWorkspaceCascadesToGrandchildren that creates a multi-level workspace tree and asserts all descendants are removed from items after removing the ancestor.Sources/ContentView.swift-10655-10657 (1)
10655-10657:⚠️ Potential issue | 🟠 MajorSurface template read/parse failures instead of dropping the action.
Both template-open paths discard
TemplateRepository.shared.getTemplate(...)errors withtry?, so malformed YAML or file I/O problems look like a dead menu item. Please route the failure to visible UI instead of returning silently.🛠️ Minimal fix
- if let template = try? TemplateRepository.shared.getTemplate(named: templateName) { - tabManager.openTemplate(template, directory: url.path) - } + do { + let template = try TemplateRepository.shared.getTemplate(named: templateName) + tabManager.openTemplate(template, directory: url.path) + } catch { + NSAlert(error: error).runModal() + }Apply the same pattern in
openTemplateInWorkspace(named:).Also applies to: 12414-12416
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 10655 - 10657, The current code swallows errors from TemplateRepository.shared.getTemplate(named:) using try?, causing malformed YAML or I/O errors to appear as dead menu items; change both call sites (the snippet that calls tabManager.openTemplate(template, directory:) and openTemplateInWorkspace(named:)) to use do/try/catch, attempt let template = try TemplateRepository.shared.getTemplate(named:), then call tabManager.openTemplate(...) on success, and in catch route the thrown error to the visible UI (e.g., call your app’s existing error reporting/alerting helper or present an NSAlert with the error.localizedDescription) so failures are surfaced instead of being ignored.Sources/ContentView.swift-8369-8374 (1)
8369-8374:⚠️ Potential issue | 🟠 MajorDon't pass visible-order indices into
TabItemView.index.Line 8369 now feeds
visibleIndex, butTabItemViewstill consumesindexas a flattabManager.tabsposition in the shift-range path (Line 11841) and move actions (Line 11817). Once a parent is collapsed, those orders diverge, so Shift-click and Move Up/Down can target hidden or wrong workspaces.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 8369 - 8374, TabItemView.index must receive the absolute position in tabManager.tabs, not the visible-order visibleIndex; compute the full index via the tab manager (e.g., let fullIndex = tabManager.index(of: tab) or tabManager.tabs.firstIndex(where: { $0.id == tab.id })) and pass that as index to TabItemView so its shift-range path and move up/down actions operate on the real tab collection; keep using visibleIndex only for UI-only things like WorkspaceShortcutMapper.commandDigitForWorkspace(at: visibleIndex, workspaceCount: visibleCount) and leave isActive as tabManager.selectedTabId == tab.id.Sources/ContentView.swift-11092-11101 (1)
11092-11101:⚠️ Potential issue | 🟠 MajorUse a real button for collapse/expand.
Image+onTapGestureis not keyboard/VoiceOver actionable, so the new hierarchy control is effectively pointer-only. This should be a plainButtonwith a localized accessibility label/value.♿ Minimal fix
if hasChildren { - Image(systemName: "chevron.right") - .font(.system(size: 9, weight: .bold)) - .rotationEffect(.degrees(isCollapsed ? 0 : 90)) - .foregroundColor(.secondary) - .frame(width: 12, height: 12) - .contentShape(Rectangle()) - .onTapGesture { - tabManager.toggleWorkspaceCollapsed(tab.id) - } + Button { + tabManager.toggleWorkspaceCollapsed(tab.id) + } label: { + Image(systemName: "chevron.right") + .font(.system(size: 9, weight: .bold)) + .rotationEffect(.degrees(isCollapsed ? 0 : 90)) + .foregroundColor(.secondary) + .frame(width: 12, height: 12) + .contentShape(Rectangle()) + } + .buttonStyle(.plain) + .accessibilityLabel( + String( + localized: isCollapsed ? "sidebar.workspace.expand" : "sidebar.workspace.collapse", + defaultValue: isCollapsed ? "Expand workspace" : "Collapse workspace" + ) + ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 11092 - 11101, Replace the tappable Image and onTapGesture with a proper Button that calls tabManager.toggleWorkspaceCollapsed(tab.id) (use the existing tabManager.toggleWorkspaceCollapsed symbol) so the control is keyboard and VoiceOver accessible; keep the Image(systemName: "chevron.right") as the Button label, preserve the rotationEffect/foregroundColor/frame, and add a localized accessibilityLabel and accessibilityValue based on isCollapsed (e.g., "Collapse" / "Expand" or localized equivalents) so the state is announced to assistive technologies.Sources/TabManager.swift-2303-2315 (1)
2303-2315:⚠️ Potential issue | 🟠 MajorClosing the last grouped workspace can strand the root.
After recursively closing children, the second
guard tabs.count > 1can fire before the requested parent is removed. A window containing onlyparent + descendantsends up keeping the parent open instead of closing the whole group/window.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TabManager.swift` around lines 2303 - 2315, The second guard in closeWorkspace(_:) can bail out before removing the requested parent, leaving the root stranded; replace that guard with logic that checks whether the workspace to remove still exists and only prevents removal when it is the very last remaining tab. Specifically, in closeWorkspace(_:), after recursively closing childWorkspaceIds, remove the second `guard tabs.count > 1 else { return }` and instead check if `tabs.contains(where: { $0.id == workspace.id })` and if `tabs.count == 1 && tabs.first?.id == workspace.id` then return; otherwise proceed to remove the workspace (use the workspace.id and tabs collection to locate and remove it).Sources/TabManager.swift-5234-5248 (1)
5234-5248:⚠️ Potential issue | 🟠 MajorClear sidebar state before rebuilding it from a session snapshot.
The legacy/no-
topLevelWorkspaceIndicesbranches only register onto the existingWorkspaceGroupManager.itemsarray. Restoring into a liveTabManagercan therefore leave stale sidebar UUIDs from the pre-restore session.🔧 Suggested fix
// Rebuild sidebar layout from snapshot + groupManager.items.removeAll() if let topLevelIndices = snapshot.topLevelWorkspaceIndices { groupManager.items = topLevelIndices.compactMap { index in guard newTabs.indices.contains(index) else { return nil } return newTabs[index].id }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TabManager.swift` around lines 5234 - 5248, Before rebuilding the sidebar from the snapshot, clear the existing sidebar state so stale UUIDs aren't preserved: inside the TabManager logic that handles snapshot restoration (the block checking snapshot.topLevelWorkspaceIndices and snapshot.sidebarLayout), reset the groupManager's stored items (e.g., clear or assign an empty collection to groupManager.items) before you populate it via topLevelWorkspaceIndices mapping, restoreLegacySidebarLayout(sidebarLayout, workspaces: newTabs), or the loop that calls groupManager.registerWorkspaceAsStandalone(ws.id); ensure items is then re-assigned from groupManager.items after the rebuild.Sources/TerminalControllerProjectCommands.swift-226-243 (1)
226-243:⚠️ Potential issue | 🟠 Major
group.install_templatecurrently truncates hierarchical templates.This loop only materializes
template.root.children. Any nestedchild.childrenare dropped, so installing the same template here produces a flatter tree thanproject.open_template.🔧 Suggested fix
- for child in template.root.children { - let ws = tm.addWorkspace( - workingDirectory: workDir, - select: false, - skipStandaloneRegistration: true - ) - ws.title = child.title - if let color = child.color { - ws.customColor = color - } - tm.groupManager.addChildId(ws.id, to: parentId) - createdIds.append(ws.id.uuidString) - - if let command = child.command, - let panelId = ws.focusedPanelId ?? ws.panels.values.first(where: { $0 is TerminalPanel })?.id { - scriptRunner.scheduleCommand(command, workspace: ws, panelId: panelId) - } - } + createChildWorkspaces( + children: template.root.children, + parentId: parentId, + workingDirectory: workDir, + tabManager: tm, + scriptRunner: scriptRunner, + createdIds: &createdIds + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalControllerProjectCommands.swift` around lines 226 - 243, The installation loop only materializes template.root.children and drops nested child.children, producing a flattened tree; update the logic in the group.install_template flow to recursively traverse the template node tree (starting from template.root) and for each node create a workspace via tm.addWorkspace, set ws.title/ws.customColor, call tm.groupManager.addChildId(ws.id, to: parentId), append to createdIds, and schedule any command with scriptRunner.scheduleCommand using the resolved panelId; implement a helper function (e.g., materializeTemplateNode(node:parentId:)) that handles creating the workspace for a node and then calls itself for each node.children so the original hierarchy is preserved.Sources/TerminalControllerProjectCommands.swift-127-130 (1)
127-130:⚠️ Potential issue | 🟠 MajorPersist
startupCommandbefore scheduling it.
TabManager.restoreSessionSnapshot(...)only replays savedworkspace.startupCommand, but these socket/template paths schedule commands without ever assigning that field. Commands opened over the socket will run once and then disappear after a restore.🔧 Suggested fix
if let command = template.root.command, let panelId = rootWs.focusedPanelId ?? rootWs.panels.values.first(where: { $0 is TerminalPanel })?.id { + rootWs.startupCommand = command scriptRunner.scheduleCommand(command, workspace: rootWs, panelId: panelId) } ... if let command = child.command, let panelId = ws.focusedPanelId ?? ws.panels.values.first(where: { $0 is TerminalPanel })?.id { + ws.startupCommand = command scriptRunner.scheduleCommand(command, workspace: ws, panelId: panelId) } ... if let command = child.command, let panelId = ws.focusedPanelId ?? ws.panels.values.first(where: { $0 is TerminalPanel })?.id { + ws.startupCommand = command scriptRunner.scheduleCommand(command, workspace: ws, panelId: panelId) }Also applies to: 177-179, 239-241
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalControllerProjectCommands.swift` around lines 127 - 130, The scheduled commands from template/socket are not persisted to the workspace startupCommand, so TabManager.restoreSessionSnapshot(...) can’t replay them; before calling scriptRunner.scheduleCommand(command, workspace: rootWs, panelId: panelId) assign the command to the workspace's startupCommand (e.g., rootWs.startupCommand = command) so it is saved for restores; apply the same change at the other occurrences mentioned (the blocks around the spots corresponding to lines 177-179 and 239-241) so any command scheduled via template/socket is persisted prior to scheduling.Sources/TerminalControllerProjectCommands.swift-256-265 (1)
256-265:⚠️ Potential issue | 🟠 MajorProject dedupe skips childless project roots.
The
ws.hasChildrenguard excludes the two creation paths above that intentionally produce a single root workspace: the plain-directory fallback and templates whose root has no children. Reopening those directories will duplicate them instead of focusing the existing workspace.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalControllerProjectCommands.swift` around lines 256 - 265, The dedupe logic currently skips workspaces with no children because of the ws.hasChildren guard, which causes single-root workspaces (plain-directory fallback and certain templates) to be duplicated; update the loop in TerminalControllerProjectCommands (where it iterates appDelegate.allTabManagers() and uses tm.workspace(for: wsId)) to remove the ws.hasChildren requirement — i.e., only guard for the existence of ws (guard let ws = tm.workspace(for: wsId) else { continue }) and then compare ws.currentDirectory (resolved to canonicalPath) as before, keeping the existing actions (tm.selectedTabId = ws.id, tm.window?.makeKeyAndOrderFront(nil), return .ok([...])) so single-root workspaces are matched and focused instead of duplicated.Sources/TerminalControllerProjectCommands.swift-51-63 (1)
51-63:⚠️ Potential issue | 🟠 MajorBackground project tabs need eager terminal startup.
These children are created off-screen with
select: false, but unlikecreateChildWorkspaces(...)they never request background terminal startup. The scheduled script will not reach prompt-ready until the user manually opens the tab.🔧 Suggested fix
for tabDef in result.tabDefinitions { let childWs = tm.addWorkspace( workingDirectory: dirURL.path, select: false, + eagerLoadTerminal: true, skipStandaloneRegistration: true )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalControllerProjectCommands.swift` around lines 51 - 63, The new off-screen child workspace created via tm.addWorkspace (childWs) never requests background terminal startup, so scheduled scripts may not reach prompt-ready; update the flow to request the same background terminal startup used by createChildWorkspaces for childWs (call the equivalent background startup API on childWs or through tm) after tm.groupManager.addChildId(...) and before scriptRunner.scheduleScript(...), ensuring the terminal is started in the background when select: false so the scriptRunner.scheduleScript will run to prompt-ready.Sources/TabManager.swift-1871-1880 (1)
1871-1880:⚠️ Potential issue | 🟠 MajorDon't silently downgrade a failed template open to a plain workspace.
If the selected template cannot be loaded,
try?swallows the error and this method falls through to creating a normal workspace. The caller asked for a hierarchy, so this should surface the failure and abort instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TabManager.swift` around lines 1871 - 1880, The code currently swallows template-loading errors with `try? TemplateRepository.shared.getTemplate(named:)` and falls back to creating a plain workspace; instead, change this to a throwing call so failures are surfaced: replace the `try?` with `try` (or otherwise handle the thrown error) and make the enclosing function propagate the error (add `throws`) so that a failed `TemplateRepository.shared.getTemplate(named:)` aborts rather than falling through to `addWorkspace(workingDirectory:select:)`; ensure callers of this method are updated to handle the thrown error and keep the `openTemplate(_:directory:)` branch unchanged so the requested hierarchy is only created on successful template load.
🟡 Minor comments (8)
Sources/NewWorkspaceDialog.swift-39-41 (1)
39-41:⚠️ Potential issue | 🟡 MinorGuard popup index before template array access.
Line 40 assumes a non-negative selected index. If
indexOfSelectedItemis-1, this becomes an out-of-bounds access.Suggested fix
let selectedIndex = accessory.popup.indexOfSelectedItem - let templateName: String? = selectedIndex == 0 ? nil : templates[selectedIndex - 1] + let templateName: String? + if selectedIndex <= 0 { + templateName = nil + } else if (selectedIndex - 1) < templates.count { + templateName = templates[selectedIndex - 1] + } else { + templateName = nil + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/NewWorkspaceDialog.swift` around lines 39 - 41, The code assumes accessory.popup.indexOfSelectedItem is non-negative and indexes templates with selectedIndex - 1; guard against indexOfSelectedItem being -1 by checking the selectedIndex value before accessing templates (e.g., treat -1 as no selection and map to templateName = nil, or ensure selectedIndex >= 1 and selectedIndex - 1 < templates.count), updating the logic around selectedIndex/templateName in NewWorkspaceDialog (the accessory.popup.indexOfSelectedItem usage) to avoid out‑of‑bounds access.Sources/ScriptManagerViewModel.swift-76-93 (1)
76-93:⚠️ Potential issue | 🟡 MinorLocalize the "Copy" suffix for duplicated scripts.
The
"Copy"and"Copy \(counter)"strings are user-facing (visible in the script manager sidebar) but are not wrapped inString(localized:defaultValue:).🌐 Proposed fix for localization
func duplicateSelected() { errorMessage = nil guard let name = selectedName, let content = repo.getScript(named: name) else { return } - var copyName = "\(name) Copy" + var copyName = String( + localized: "scriptManager.copyScriptName", + defaultValue: "\(name) Copy" + ) var counter = 1 while repo.hasScript(named: copyName) { counter += 1 - copyName = "\(name) Copy \(counter)" + copyName = String( + localized: "scriptManager.copyScriptNameNumbered", + defaultValue: "\(name) Copy \(counter)" + ) }As per coding guidelines: "All user-facing strings must be localized using
String(localized: "key.name", defaultValue: "English text")for every string shown in the UI."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ScriptManagerViewModel.swift` around lines 76 - 93, The duplicated-script name construction in duplicateSelected() uses hardcoded user-facing strings ("Copy" and "Copy \(counter)") — update it to use localized strings: replace the literal "Copy" and the interpolated "Copy \(counter)" with String(localized:..., defaultValue:...) (e.g. use a "script.copy" key for the base suffix and format the numbered variant via String(format:localized: or String(localized:..., defaultValue: "%@ %d") with the script name and counter). Ensure copies created in duplicateSelected() (variables copyName and the while-loop) use the localized suffix before calling repo.saveScript and when calling selectScript/reload.Sources/Workspace.swift-5088-5090 (1)
5088-5090:⚠️ Potential issue | 🟡 MinorClean up stale
onPromptReadycallbacks when panels are removed.Line 5090 introduces per-panel closures, but they’re only removed on Line 5862 when
.promptIdleis reached. If a panel closes before that, callbacks can be retained longer than needed.💡 Suggested cleanup points
// In pruneSurfaceMetadata(validSurfaceIds:) + onPromptReady = onPromptReady.filter { validSurfaceIds.contains($0.key) } // In didCloseTab cleanup path (after resolving panelId) + onPromptReady.removeValue(forKey: panelId) // In didClosePane cleanup loop + onPromptReady.removeValue(forKey: panelId)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 5088 - 5090, The onPromptReady dictionary stores per-panel one-shot closures but is only cleared when a panel reaches .promptIdle; ensure you remove any stored closure when a panel is removed or closed to avoid retaining stale callbacks—add cleanup of onPromptReady[panelID] in the code paths that remove/close panels (e.g., in the panel removal/close handler(s) such as removePanel/closePanel/panelDidClose or any workspace panel-replacement logic) and also when a panel is replaced or its UUID changes; reference the onPromptReady property to locate and delete the entry for the panel's UUID in those code paths.cmuxTests/WorkspaceGroupManagerTests.swift-213-233 (1)
213-233:⚠️ Potential issue | 🟡 MinorIncomplete test: missing assertion for depth constraint blocking.
This test sets up the scenario and comments about depth constraints, but never actually asserts that indentation is blocked when max depth would be exceeded. Lines 229-233 create
ws4at depth 4 but don't attempt an indent operation or verify it fails.Proposed fix to complete the test
// Now ws1 -> ws2 -> ws3 (ws3 is at depth 3) // Create ws4 and try to indent under ws3 — would exceed depth 3 let ws4 = tabManager.addWorkspace() - ws3.childWorkspaceIds = [ws4.id] - // ws4 is now at depth 4 which is the state but indenting ws3 further isn't possible - // since ws3 is a child of ws2 which is a child of ws1 + manager.registerWorkspaceAsStandalone(ws4.id) + + // ws4 has a child, so indenting ws4 under ws3 would put the child at depth 4+ + let ws5 = tabManager.addWorkspace() + ws4.childWorkspaceIds = [ws5.id] + + // Attempt to indent ws4 under ws3 — should fail due to depth constraint + let blockedResult = manager.indentWorkspace(ws4.id) + XCTAssertFalse(blockedResult, "Indent should be blocked when it would exceed max depth") + // ws4 should remain a top-level item + XCTAssertTrue(manager.items.contains(ws4.id)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/WorkspaceGroupManagerTests.swift` around lines 213 - 233, The test testIndentBlockedByDepthConstraint is incomplete: after creating ws4 as a child of ws3 you must attempt the indent and assert it is blocked. Update the test to (1) ensure ws3 and ws4 are registered via manager.registerWorkspaceAsStandalone(...) if needed, (2) call let result2 = manager.indentWorkspace(ws3.id) (or indentWorkspace on the workspace that would push ws4 beyond max depth), and (3) XCTAssertFalse(result2) (and optionally assert the workspace hierarchy didn't change). Use the existing symbols testIndentBlockedByDepthConstraint, ws3, ws4, and manager.indentWorkspace to locate where to add this assertion.Sources/TemplateManagerViewModel.swift-94-99 (1)
94-99:⚠️ Potential issue | 🟡 MinorLocalize the generated duplicate-name suffix.
Lines 94-99 hard-code
Copy, so duplicated templates stay English in every locale even though the “New Template” flow is localized. Please build both the base and numbered variants from localization keys as well.As per coding guidelines, "All user-facing strings must be localized using
String(localized: "key.name", defaultValue: "English text")for every string shown in the UI."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TemplateManagerViewModel.swift` around lines 94 - 99, The duplicate-name generation currently hardcodes "Copy"; change it to use localized strings by building the base suffix and the numbered variant via String(localized:...): replace the literal "Copy" in the copyName construction with a localized base suffix (e.g., a key like "template.copy.suffix") and format the numbered variant using another localized format string (e.g., "template.copy.suffix.number" or an interpolated localized string) while keeping the loop using repo.hasTemplate(named:) and the same copyName variable to ensure uniqueness; ensure you call String(localized: "key", defaultValue: "English text") for both the base and numbered forms in TemplateManagerViewModel where copyName is computed.Sources/WorkspaceGroupManager.swift-149-171 (1)
149-171:⚠️ Potential issue | 🟡 MinorOutdent logic is correct but could fail silently.
If neither the grandparent nor top-level insertion path is taken (e.g., due to stale data), the method returns
truedespite not actually inserting the workspace anywhere. Consider returningfalseif insertion didn't occur.Proposed fix
`@discardableResult` func outdentWorkspace(_ workspaceId: UUID) -> Bool { guard let tabManager else { return false } guard let parent = parentWorkspace(of: workspaceId) else { return false } // Remove from parent's children parent.childWorkspaceIds.removeAll { $0 == workspaceId } // Find where parent lives and insert after it if let grandparent = parentWorkspace(of: parent.id) { // Parent is a child itself — insert in grandparent's children after parent if let parentIdx = grandparent.childWorkspaceIds.firstIndex(of: parent.id) { grandparent.childWorkspaceIds.insert(workspaceId, at: parentIdx + 1) + return true } } else if let parentTopIdx = items.firstIndex(of: parent.id) { // Parent is top-level — insert in items after parent items.insert(workspaceId, at: parentTopIdx + 1) + return true } - return true + // Failed to find insertion point — workspace is now orphaned + return false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/WorkspaceGroupManager.swift` around lines 149 - 171, The outdentWorkspace(_:) flow currently removes workspaceId from parent.childWorkspaceIds then may fail to insert it (so returns true incorrectly); modify it to record the parent's original child index, attempt insertion into grandparent.childWorkspaceIds or items while tracking a Bool inserted, and if insertion never happens, restore parent.childWorkspaceIds by reinserting workspaceId at the recorded index and return false; update references in this logic to parentWorkspace(of:), parent.childWorkspaceIds, grandparent.childWorkspaceIds, items and ensure outdentWorkspace(_:) returns true only when inserted is true.Sources/TemplateRepository.swift-99-106 (1)
99-106:⚠️ Potential issue | 🟡 Minor
seedDefaultTemplatesbypassesNameSanitizerand silently swallows errors.Unlike other methods,
seedDefaultTemplatesusesnamedirectly (line 103) rather than callingNameSanitizer.sanitize(name). While the hardcoded names are safe, this inconsistency could become a problem if the method is later generalized. Additionally, bothtry?usages silently ignore failures, making it hard to diagnose seeding issues.Proposed fix
func seedDefaultTemplates() { for (name, content) in Self.defaultTemplates { guard !hasTemplate(named: name) else { continue } - try? ensureDirectoryExists() - let path = directory.appendingPathComponent("\(name).yaml") - try? content.write(to: path, atomically: true, encoding: .utf8) + do { + try ensureDirectoryExists() + let safeName = try NameSanitizer.sanitize(name) + let path = directory.appendingPathComponent("\(safeName).yaml") + try content.write(to: path, atomically: true, encoding: .utf8) + } catch { + // Log seeding failure for diagnostics + print("[TemplateRepository] Failed to seed template '\(name)': \(error)") + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TemplateRepository.swift` around lines 99 - 106, seedDefaultTemplates currently writes files using the raw template name and swallows failures with try?, so update seedDefaultTemplates to sanitize the filename via NameSanitizer.sanitize(name) before building the path, and replace the silent try? calls around ensureDirectoryExists() and content.write(...) with proper error handling (use do-catch and either log the error or rethrow so failures are visible). Ensure you still check hasTemplate(sanitizedName) and build the path with directory.appendingPathComponent("\(sanitizedName).yaml") so the behavior matches other methods.Sources/TerminalControllerScriptTemplateCommands.swift-61-68 (1)
61-68:⚠️ Potential issue | 🟡 MinorError type conflation in
v2TemplateGet.Catching all errors and returning
not_foundmasks distinct failure modes (parse errors, permission errors, disk I/O errors). Callers cannot distinguish between "template doesn't exist" and "template exists but is malformed."Consider checking file existence separately or mapping different error types to distinct codes.
Proposed fix
func v2TemplateGet(params: [String: Any]) -> V2CallResult { guard let name = params["name"] as? String else { return .err(code: "missing_param", message: "Missing 'name'", data: nil) } + guard TemplateRepository.shared.hasTemplate(named: name) else { + return .err(code: "not_found", message: "Template '\(name)' not found", data: nil) + } do { let template = try TemplateRepository.shared.getTemplate(named: name) let result = serializeTemplateNode(template.root) return .ok(["name": name, "root": result]) } catch { - return .err(code: "not_found", message: "Template '\(name)' not found", data: nil) + return .err(code: "parse_error", message: error.localizedDescription, data: nil) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalControllerScriptTemplateCommands.swift` around lines 61 - 68, The catch-all in v2TemplateGet masks distinct failure modes; update the error handling around TemplateRepository.shared.getTemplate(named:) and serializeTemplateNode(...) to distinguish "not found" from parse/IO/permission errors by first checking existence (e.g., a TemplateRepository.shared.templateExists(named:) or FileManager check) and then mapping thrown errors to appropriate codes (e.g., "not_found" for missing, "parse_error" for deserialization/serializeTemplateNode failures, "io_error"/"permission_denied" for disk access errors). Specifically, change the do/catch so that if templateExists returns false you return .err(code: "not_found", ...), otherwise rethrow or inspect the thrown error and return different .err codes/messages for parsing vs I/O/permission problems, referencing getTemplate(named:) and serializeTemplateNode to locate the relevant code paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 848c8c0b-53ea-411b-809d-97eefad9d387
📒 Files selected for processing (43)
CLI/cmux.swiftGhosttyTabs.xcodeproj/project.pbxprojResources/Info.plistSources/AppDelegate.swiftSources/CmuxConfigError.swiftSources/CmuxConfigParser.swiftSources/CmuxConfigParserGroups.swiftSources/ContentView.swiftSources/GroupHeaderView.swiftSources/GroupItem.swiftSources/MonospaceTextEditor.swiftSources/NameSanitizer.swiftSources/NewWorkspaceDialog.swiftSources/Panels/TerminalPanel.swiftSources/ScriptManagerViewModel.swiftSources/ScriptManagerWindow.swiftSources/ScriptRepository.swiftSources/ScriptsMenu.swiftSources/SessionPersistence.swiftSources/SessionWorkspaceGroupSnapshot.swiftSources/StartupScriptRunner.swiftSources/TabManager.swiftSources/TemplateManagerHelpSidebar.swiftSources/TemplateManagerViewModel.swiftSources/TemplateManagerWindow.swiftSources/TemplateRepository.swiftSources/TemplateYamlParser.swiftSources/TerminalController.swiftSources/TerminalControllerGroupCommands.swiftSources/TerminalControllerProjectCommands.swiftSources/TerminalControllerScriptTemplateCommands.swiftSources/Update/UpdateTitlebarAccessory.swiftSources/Workspace.swiftSources/WorkspaceGroup.swiftSources/WorkspaceGroupManager.swiftSources/cmuxApp.swiftcmuxTests/CmuxConfigParserTests.swiftcmuxTests/NameSanitizerTests.swiftcmuxTests/ScriptRepositoryTests.swiftcmuxTests/SessionGroupPersistenceTests.swiftcmuxTests/TemplateRepositoryTests.swiftcmuxTests/WorkspaceGroupManagerTests.swiftcmuxTests/WorkspaceGroupTests.swift
| func show() { | ||
| viewModel.reload() | ||
| window?.makeKeyAndOrderFront(nil) | ||
| } |
There was a problem hiding this comment.
These code paths can drop a dirty script buffer without a Save/Discard/Cancel decision.
Line 41 reloads the singleton from disk every time the window is shown, and the Add / Duplicate / Delete actions on Lines 118-133 and 192-212 bypass the dirty-state prompt that selection changes already use. If viewModel.isDirty is true, these paths can overwrite the editor buffer, duplicate stale on-disk content, or delete the current script without first resolving the unsaved edits.
Also applies to: 118-133, 192-212
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/ScriptManagerWindow.swift` around lines 40 - 43, The show() path and
the Add/Duplicate/Delete actions call viewModel.reload() or mutate scripts
without checking viewModel.isDirty, which can overwrite or lose unsaved editor
content; before calling viewModel.reload() in show() and before performing the
Add, Duplicate, and Delete handlers (the actions around the Add / Duplicate /
Delete blocks), detect viewModel.isDirty and present the same
Save/Discard/Cancel prompt used by selection-change logic, then act on the
user's choice (save then proceed, discard and proceed, or cancel the operation)
so the editor buffer is not dropped unintentionally.
| alert.beginSheetModal(for: window) { [weak self] response in | ||
| switch response { | ||
| case .alertFirstButtonReturn: | ||
| self?.viewModel.save() | ||
| onDiscard() | ||
| case .alertSecondButtonReturn: | ||
| self?.viewModel.revert() | ||
| onDiscard() |
There was a problem hiding this comment.
Only close or switch after a successful save.
The save branches here continue immediately after viewModel.save(), but save() communicates failure through errorMessage rather than control flow. When validation or disk I/O fails, the switch path still moves away from the current script, and the close path still retries the close flow instead of keeping the user on the editor.
Also applies to: 228-234
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/ScriptManagerWindow.swift` around lines 69 - 76, The current alert
handlers call onDiscard() (or switch/close) immediately after viewModel.save(),
but save() reports failures via viewModel.errorMessage rather than a return
value; change both alert callback sites (the beginSheetModal handlers) to call
viewModel.save() then check viewModel.errorMessage (or a saveSuccess flag on the
viewModel) and only call onDiscard() / proceed with closing/switching if there
is no error; keep the revert path behavior unchanged but apply the same guard
after save in both places mentioned so the UI stays on the editor when save
fails.
| func show() { | ||
| viewModel.reload() | ||
| window?.makeKeyAndOrderFront(nil) | ||
| } |
There was a problem hiding this comment.
These code paths can drop a dirty template buffer without a Save/Discard/Cancel decision.
Line 41 reloads the singleton from disk every time the window is shown, and the Add / Duplicate / Delete actions on Lines 121-136 and 195-215 bypass the dirty-state prompt that list selection already uses. If viewModel.isDirty is true, these paths can overwrite editorText, duplicate stale on-disk YAML, or delete the current template without ever asking what to do with the unsaved edits.
Also applies to: 121-136, 195-215
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TemplateManagerWindow.swift` around lines 40 - 43, The show() path
(calling viewModel.reload()) and the Add/Duplicate/Delete action handlers can
overwrite or remove unsaved edits because they don't respect viewModel.isDirty;
before calling viewModel.reload() in show() and before performing Add,
Duplicate, or Delete you must check viewModel.isDirty and present the same
Save/Discard/Cancel dialog used for list selection: on Save invoke the existing
save routine, on Discard proceed with reload/add/duplicate/delete, on Cancel
abort the operation; ensure editorText and any duplicate logic read from the
in-memory model (not stale on-disk) only after handling the dirty-state decision
so no unsaved edits are lost.
| alert.beginSheetModal(for: window) { [weak self] response in | ||
| switch response { | ||
| case .alertFirstButtonReturn: | ||
| self?.viewModel.save() | ||
| onDiscard() | ||
| case .alertSecondButtonReturn: | ||
| self?.viewModel.revert() | ||
| onDiscard() |
There was a problem hiding this comment.
Only close or switch after a successful save.
The save branches here continue immediately after viewModel.save(), but save() reports failure by mutating errorMessage instead of throwing. A validation or write error therefore still advances the workflow: the switch path discards the current buffer, and the close path just re-enters the close flow. Please make save() return success/failure and gate the follow-up action on that result.
Also applies to: 231-237
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TemplateManagerWindow.swift` around lines 69 - 76, The save branch
currently calls viewModel.save() but proceeds to onDiscard()/close regardless of
whether save actually succeeded (save reports errors via errorMessage), so
change viewModel.save() to return a Bool or Result (e.g., save() -> Bool or
save() -> Result<Void, Error>), update the call sites in
TemplateManagerWindow.swift (the alert completion handler and the similar block
at lines 231-237) to check the returned success value before calling onDiscard()
or continuing the close flow, and only proceed when the save indicates success;
keep viewModel.revert() behavior unchanged for the discard path.
Replace defaultButtonCell/keyEquivalent overrides with hasDestructiveAction in all three close-confirmation alerts. Adds Phase 1 shadow fields for menu-state decoupling and TabItemDisplaySnapshot infrastructure. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…bmenu flicker Move sidebar context menu from SwiftUI .contextMenu to an AppKit NSMenu built at right-click time via NSMenuDelegate.menuNeedsUpdate. The menu is fully static while open, decoupled from the SwiftUI rendering pipeline. Removes ~300 lines of dead SwiftUI menu code from ContentView.swift. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Closing — this fork has diverged significantly and is maintained independently. |
There was a problem hiding this comment.
1 issue found across 12 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/SidebarContextMenuController.swift">
<violation number="1" location="Sources/SidebarContextMenuController.swift:164">
P2: "Clear Color" menu visibility checks only the clicked workspace, hiding valid bulk-clear for selected workspaces.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| let parentItem = NSMenuItem(title: String(localized: "contextMenu.workspaceColor", defaultValue: "Workspace Color"), action: nil, keyEquivalent: "") | ||
| parentItem.submenu = colorMenu | ||
|
|
||
| if workspace.customColor != nil { |
There was a problem hiding this comment.
P2: "Clear Color" menu visibility checks only the clicked workspace, hiding valid bulk-clear for selected workspaces.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/SidebarContextMenuController.swift, line 164:
<comment>"Clear Color" menu visibility checks only the clicked workspace, hiding valid bulk-clear for selected workspaces.</comment>
<file context>
@@ -0,0 +1,421 @@
+ let parentItem = NSMenuItem(title: String(localized: "contextMenu.workspaceColor", defaultValue: "Workspace Color"), action: nil, keyEquivalent: "")
+ parentItem.submenu = colorMenu
+
+ if workspace.customColor != nil {
+ colorMenu.addItem(ActionMenuItem(
+ String(localized: "contextMenu.clearColor", defaultValue: "Clear Color")
</file context>
1 participant
Summary
Adds collapsible, hierarchical workspace groups to the sidebar — workspaces can have child workspaces (max 3 levels), creating project-based grouping with visual separation.
Features
childWorkspaceIds,isCollapsed, collapse/expand via chevron~/.config/cmux/templates/that create multi-tab hierarchies with startup commands~/.config/cmux/scripts/runnable from context menu or Scripts menuonPromptReadycmux open /pathcommandSecurity hardening
/,\,:,..)try?with user-visible errors)UI polish
plus.square.on.squarepreferredTabManagerfor menu lookups (ported from upstream 6bd9894)Test plan
/or..are rejected🤖 Generated with Claude Code
Summary by cubic
Adds collapsible, hierarchical workspace groups (max 3 levels) and a templates/scripts system so projects open with the right tabs and commands faster. Also adds a Scripts menu, project open flow (CLI + Finder), drag-and-drop hierarchy, and tighter socket/security checks.
New Features
~/.config/cmux/templates/and shell scripts in~/.config/cmux/scripts/; Template Manager and Script Manager editors; menu bar “Scripts” with Run Script, Open Template, and managers..cmux.yamlparsing, CLIcmux open /path, and a Finder “Open in cmux” service; adds socket commands for groups, projects, scripts, and templates.sendInteractiveText); skipped on session restore to avoid duplicates.Bug Fixes
/,\,:,..).Written for commit 4614ccc. Summary will update on new commits.
Summary by CodeRabbit