Repository navigation
Turn a single workspace into a group (promote in place) - #5633
lawrencecchen wants to merge 7 commits into
Conversation
Grouping previously always synthesized a fresh empty anchor workspace, so "New Group from Workspace" on a single workspace produced a junk empty anchor with the original as a child, and ungrouping left two workspaces. Add a promote path: an existing ungrouped workspace becomes the anchor of a new single-member group in place (no extra workspace), inheriting its title as the group name and its pinned state to keep its sidebar position. The existing ungroup is the exact inverse, so workspace -> group -> ungroup round-trips back to the same single normal workspace. - TabManager.makeWorkspaceGroupFromWorkspace(anchorWorkspaceId:name:) - Sidebar "New Group from Workspace" (single, ungrouped) routes here; multi-select "New Group from Selection" / Cmd-Shift-G keep the documented fresh-anchor create contract; grouped rows still show Move/Remove only. - Socket workspace.group.from_workspace + CLI `workspace group from-workspace` (non-focus, mirrors create's payload). - Round-trip + no-op + header-render unit tests; docs updated. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPromotes an existing ungrouped workspace into a single-member workspace group anchored on that workspace. Adds TabManager API, Terminal v2 handler, cmux CLI subcommand, sidebar/context-menu and shortcut wiring, tests, docs, schema, and localization. ChangesWorkspace-to-Group Promotion
Sequence DiagramsequenceDiagram
participant User
participant CLI
participant Client
participant TerminalController
participant TabManager
User->>CLI: cmux workspace-group from-workspace --workspace <id> [--name <name>]
CLI->>Client: sendV2(workspace.group.from_workspace, params)
Client->>TerminalController: Deliver v2 command
TerminalController->>TabManager: makeWorkspaceGroupFromWorkspace(workspace_id, name)
alt workspace missing
TerminalController-->>Client: error not_found
else already grouped
TerminalController-->>Client: error workspace_already_grouped
else success
TerminalController->>Client: group payload
Client-->>CLI: payload
alt jsonOutput
CLI-->>User: formatted JSON
else
CLI-->>User: OK <group_name> (if present) or OK
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (3 errors, 1 warning)
✅ Passed checks (17 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 468315f489
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return (true, tab.groupId != nil) | ||
| } | ||
| guard knownState.exists else { | ||
| return .err(code: "not_found", message: "Workspace not found in target window", data: [ |
There was a problem hiding this comment.
This new error path returns a bare English message to CLI/socket callers. The repository's AGENTS.md explicitly requires String(localized:defaultValue:) for every user-facing error message, so Japanese users will see this new from-workspace failure untranslated even though the adjacent workspaceAlreadyGrouped case was localized.
Useful? React with 👍 / 👎.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Greptile SummaryThis PR adds promote-in-place workspace grouping: an existing ungrouped workspace becomes the group's anchor directly (no phantom empty workspace created), inheriting its title and pinned state.
Confidence Score: 4/5Safe to merge with one localization gap to address before shipping to non-English users. The feature logic is well-structured and backed by tests. The one outstanding issue is that Resources/Localizable.xcstrings — the new Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant AppDelegate
participant TabManager
participant TerminalController
Note over User,TabManager: ⌘⇧G on single terminal workspace
User->>AppDelegate: ⌘⇧G keypress
AppDelegate->>TabManager: reactGrabWouldHandleCurrentFocus()
TabManager-->>AppDelegate: false (no browser focus)
AppDelegate->>TabManager: makeWorkspaceGroupFromWorkspace(anchorWorkspaceId:)
TabManager->>TabManager: "guard groupId == nil"
TabManager->>TabManager: inherit title + isPinned
TabManager->>TabManager: workspaceGroups.append(group)
TabManager->>TabManager: assignGroup + normalizeContiguity
TabManager-->>AppDelegate: groupId (UUID)
AppDelegate-->>User: return true (chord consumed)
Note over User,TerminalController: CLI / socket path
User->>TerminalController: workspace.group.from_workspace
TerminalController->>TerminalController: v2MainSync — check exists/grouped
TerminalController->>TabManager: makeWorkspaceGroupFromWorkspace(anchorWorkspaceId:name:)
TabManager-->>TerminalController: groupId
TerminalController->>TerminalController: v2MainSync — read group payload
TerminalController-->>User: "{ group: { ... } }"
Note over User,TabManager: ⌘⇧G toggle-off (solo group)
User->>AppDelegate: ⌘⇧G keypress
AppDelegate->>TabManager: "focused.groupId != nil, anchorWorkspaceId == focusedId, count == 1"
AppDelegate->>TabManager: ungroupWorkspaceGroup(groupId:)
TabManager-->>AppDelegate: dissolved
AppDelegate-->>User: return true (chord consumed)
Reviews (5): Last reviewed commit: "Group shortcuts: ⌘⇧G toggle, ⌘⇧P rename ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| "workspaceGroup.error.workspaceAlreadyGrouped": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Workspace is already in a group; remove it first" | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "このワークスペースはすでにグループに属しています。先に削除してください" | ||
| } | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
The new workspaceGroup.error.workspaceAlreadyGrouped key ships with only en and ja, but the catalog already carries translations for ar, bs, da, de, es, fr, it, km, ko, nb, pl, ru, th, tr, uk, zh-Hans, and zh-Hant. Users on any of those locales will fall back to the English default — surfacing raw English in an otherwise-localized UI. Adjacent keys like workspaceGroup.error.workspaceIsOtherGroupAnchor follow the same two-locale pattern, but those are pre-existing; this one is new to the PR.
Rule Used: Flag production user-facing text that is not fully... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| let knownState: (exists: Bool, grouped: Bool) = v2MainSync { | ||
| guard let tab = tabManager.tabs.first(where: { $0.id == wsId }) else { | ||
| return (false, false) | ||
| } | ||
| return (true, tab.groupId != nil) | ||
| } | ||
| guard knownState.exists else { | ||
| return .err(code: "not_found", message: "Workspace not found in target window", data: [ | ||
| "workspace_id": wsId.uuidString | ||
| ]) | ||
| } | ||
| guard !knownState.grouped else { | ||
| return .err( | ||
| code: "invalid_state", | ||
| message: String( | ||
| localized: "workspaceGroup.error.workspaceAlreadyGrouped", | ||
| defaultValue: "Workspace is already in a group; remove it first" | ||
| ), | ||
| data: ["workspace_id": wsId.uuidString] | ||
| ) | ||
| } | ||
| var createdGroupId: UUID? | ||
| v2MainSync { | ||
| createdGroupId = tabManager.makeWorkspaceGroupFromWorkspace(anchorWorkspaceId: wsId, name: name) | ||
| } |
There was a problem hiding this comment.
The workspace's exists/grouped state is read in one v2MainSync block and acted on in a second block two calls later. If another operation (e.g. workspace.group.create or workspace.group.add) runs on the same workspace between the two blocks, makeWorkspaceGroupFromWorkspace will safely return nil (its own guard), but the caller will respond with the generic not_created error instead of the more actionable not_found or invalid_state the pre-check was meant to produce. The v2WorkspaceGroupUngroup handler avoids this by merging its check and mutate into a single v2MainSync closure — a similar consolidation here would keep the error codes correct under concurrent access.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/TerminalController.swift`:
- Around line 4900-4905: Replace raw English error messages and the TabManager
type leak with localized keys and user-facing wording: when
v2ResolveTabManager(params:) returns nil, return a localized error key (e.g.
"window_unavailable" or similar existing localization used around the
grouped-workspace case) and a user-facing message that uses "window" or
"workspace" rather than exposing "TabManager"; likewise when v2UUID(params,
"workspace_id") fails, return a localized key (e.g. "invalid_workspace_id") and
a localized user-facing message. Update the three matching sites that use the
same patterns (the v2ResolveTabManager nil checks and v2UUID workspace_id
failure checks) to use the same localization keys and avoid exposing internal
type names, matching the style used in the grouped-workspace error handling
already present in this file.
- Around line 4909-4936: The existence/grouped checks, creation call and group
lookup must be executed atomically on the main thread to avoid races; replace
the separate v2MainSync hops with a single v2MainSync closure that (using wsId)
finds the tab in tabManager, verifies it is not grouped, calls
tabManager.makeWorkspaceGroupFromWorkspace(anchorWorkspaceId:name:) to create
the group, and returns both the created group id and the created group (or an
explicit error state) so the subsequent guards can inspect a single, consistent
result instead of relying on knownState/createdGroupId across multiple
v2MainSync calls and workspaceGroups lookups.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3e2618f5-ab29-4d59-a568-d591cd31df6b
📒 Files selected for processing (7)
CLI/cmux.swiftResources/Localizable.xcstringsSources/TabItemView+WorkspaceGroups.swiftSources/TabManager.swiftSources/TerminalController.swiftcmuxTests/WorkspaceGroupTests.swiftdocs/workspace-groups.md
| guard let tabManager = v2ResolveTabManager(params: params) else { | ||
| return .err(code: "unavailable", message: "TabManager not available", data: nil) | ||
| } | ||
| guard let wsId = v2UUID(params, "workspace_id") else { | ||
| return .err(code: "invalid_params", message: "Missing or invalid workspace_id", data: nil) | ||
| } |
There was a problem hiding this comment.
Localize these new API errors and remove the TabManager leak.
This route is surfaced to CLI/API users, but the new failure paths still return raw English literals, and "TabManager not available" exposes an internal type name. Please switch these messages to localized keys and user-facing window/workspace wording, the same way Line 4923 already does for the grouped-workspace case.
As per coding guidelines, “All user-facing strings must be localized” and “API error bodies ... must not expose ... internal provider names” or other implementation details.
Also applies to: 4915-4918, 4936-4936
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/TerminalController.swift` around lines 4900 - 4905, Replace raw
English error messages and the TabManager type leak with localized keys and
user-facing wording: when v2ResolveTabManager(params:) returns nil, return a
localized error key (e.g. "window_unavailable" or similar existing localization
used around the grouped-workspace case) and a user-facing message that uses
"window" or "workspace" rather than exposing "TabManager"; likewise when
v2UUID(params, "workspace_id") fails, return a localized key (e.g.
"invalid_workspace_id") and a localized user-facing message. Update the three
matching sites that use the same patterns (the v2ResolveTabManager nil checks
and v2UUID workspace_id failure checks) to use the same localization keys and
avoid exposing internal type names, matching the style used in the
grouped-workspace error handling already present in this file.
Source: Coding guidelines
| let knownState: (exists: Bool, grouped: Bool) = v2MainSync { | ||
| guard let tab = tabManager.tabs.first(where: { $0.id == wsId }) else { | ||
| return (false, false) | ||
| } | ||
| return (true, tab.groupId != nil) | ||
| } | ||
| guard knownState.exists else { | ||
| return .err(code: "not_found", message: "Workspace not found in target window", data: [ | ||
| "workspace_id": wsId.uuidString | ||
| ]) | ||
| } | ||
| guard !knownState.grouped else { | ||
| return .err( | ||
| code: "invalid_state", | ||
| message: String( | ||
| localized: "workspaceGroup.error.workspaceAlreadyGrouped", | ||
| defaultValue: "Workspace is already in a group; remove it first" | ||
| ), | ||
| data: ["workspace_id": wsId.uuidString] | ||
| ) | ||
| } | ||
| var createdGroupId: UUID? | ||
| v2MainSync { | ||
| createdGroupId = tabManager.makeWorkspaceGroupFromWorkspace(anchorWorkspaceId: wsId, name: name) | ||
| } | ||
| guard let gid = createdGroupId, | ||
| let group = v2MainSync({ tabManager.workspaceGroups.first(where: { $0.id == gid }) }) else { | ||
| return .err(code: "not_created", message: "Group was not created", data: nil) |
There was a problem hiding this comment.
Make the preflight and promotion atomic on the main thread.
Lines 4909-4936 split existence/grouped checks, creation, and group lookup across separate v2MainSync hops. Another socket/UI mutation can change the workspace between those hops, so a request that should return not_found or invalid_state instead falls through to the generic not_created branch.
Suggested structure
- let knownState: (exists: Bool, grouped: Bool) = v2MainSync {
- guard let tab = tabManager.tabs.first(where: { $0.id == wsId }) else {
- return (false, false)
- }
- return (true, tab.groupId != nil)
- }
- guard knownState.exists else {
- return .err(code: "not_found", message: "Workspace not found in target window", data: [
- "workspace_id": wsId.uuidString
- ])
- }
- guard !knownState.grouped else {
- return .err(...)
- }
- var createdGroupId: UUID?
- v2MainSync {
- createdGroupId = tabManager.makeWorkspaceGroupFromWorkspace(anchorWorkspaceId: wsId, name: name)
- }
- guard let gid = createdGroupId,
- let group = v2MainSync({ tabManager.workspaceGroups.first(where: { $0.id == gid }) }) else {
- return .err(code: "not_created", message: "Group was not created", data: nil)
- }
+ enum PromotionResult {
+ case notFound
+ case alreadyGrouped
+ case notCreated
+ case created(WorkspaceGroup)
+ }
+
+ let result: PromotionResult = v2MainSync {
+ guard let tab = tabManager.tabs.first(where: { $0.id == wsId }) else {
+ return .notFound
+ }
+ guard tab.groupId == nil else {
+ return .alreadyGrouped
+ }
+ guard let gid = tabManager.makeWorkspaceGroupFromWorkspace(anchorWorkspaceId: wsId, name: name),
+ let group = tabManager.workspaceGroups.first(where: { $0.id == gid }) else {
+ return .notCreated
+ }
+ return .created(group)
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/TerminalController.swift` around lines 4909 - 4936, The
existence/grouped checks, creation call and group lookup must be executed
atomically on the main thread to avoid races; replace the separate v2MainSync
hops with a single v2MainSync closure that (using wsId) finds the tab in
tabManager, verifies it is not grouped, calls
tabManager.makeWorkspaceGroupFromWorkspace(anchorWorkspaceId:name:) to create
the group, and returns both the created group id and the created group (or an
explicit error state) so the subsequent guards can inspect a single, consistent
result instead of relying on knownState/createdGroupId across multiple
v2MainSync calls and workspaceGroups lookups.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/WorkspaceGroupTests.swift`:
- Around line 922-923: Replace the direct property mutations of
manager.tabs[0].isPinned and manager.tabs[1].isPinned with the public API
manager.setPinned(workspace, pinned: true) so the test triggers the same side
effects (reordering/notifications) as production; locate the workspace objects
corresponding to tabs[0] and tabs[1] and call manager.setPinned(theWorkspace,
pinned: true) for each instead of assigning isPinned directly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 203cbc43-d931-4c43-a923-c74dfd8081b9
📒 Files selected for processing (1)
cmuxTests/WorkspaceGroupTests.swift
| manager.tabs[0].isPinned = true | ||
| manager.tabs[1].isPinned = true |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use setPinned API instead of direct mutation.
Lines 922-923 directly mutate isPinned, but the rest of this file consistently uses manager.setPinned(workspace, pinned: true) (see lines 147, 316, 339, 370, 387, 396, 421, 712). The setPinned method likely handles reordering, notification posting, or other side effects that direct mutation bypasses, which could make the test's initial state inconsistent with real usage.
♻️ Proposed fix
- manager.tabs[0].isPinned = true
- manager.tabs[1].isPinned = true
+ let firstWorkspace = try `#require`(manager.tabs.first { $0.id == ids[0] })
+ let secondWorkspace = try `#require`(manager.tabs.first { $0.id == ids[1] })
+ manager.setPinned(firstWorkspace, pinned: true)
+ manager.setPinned(secondWorkspace, pinned: true)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmuxTests/WorkspaceGroupTests.swift` around lines 922 - 923, Replace the
direct property mutations of manager.tabs[0].isPinned and
manager.tabs[1].isPinned with the public API manager.setPinned(workspace,
pinned: true) so the test triggers the same side effects
(reordering/notifications) as production; locate the workspace objects
corresponding to tabs[0] and tabs[1] and call manager.setPinned(theWorkspace,
pinned: true) for each instead of assigning isPinned directly.
When there's no 2+ sidebar selection, Cmd-Shift-G now turns the single focused workspace into a group in place, instead of doing nothing. It defers to React Grab whenever React Grab would act on the current focus (browser focused, or a single browser reachable from the focused terminal), so React Grab keeps the chord on browser-bearing workspaces; in a plain terminal (where it previously just beeped) it promotes. Extracts a shared reactGrabShortcutRouteForCurrentFocus() so the gate and the React Grab action use one route-resolution path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…in place Turning the focused workspace into a single-member group (and ungrouping it) changed the data model correctly but the sidebar didn't update: the group header never appeared on create, and a stale header lingered after ungroup. The sidebar list is a LazyVStack + ForEach over render items where a top-level slot switches between a `.workspace` row and a `.groupHeader` (different view types). LazyVStack mis-diffs that in-place type change, so the row view wasn't swapped. Multi-select grouping never hit this because it inserts a brand-new anchor workspace rather than transforming an existing row. Key the rows on a signature of the group-anchor SET so the list rebuilds cleanly whenever a workspace starts or stops being a group header. It is sorted, so reorders and renames (which don't change the anchor set) don't trigger a rebuild and drag-reorder is unaffected. Verified visually via cua screenshots: keyboard Cmd-Shift-G on the focused workspace now renders the header, and ungroup reverts it to a plain row. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Merge with main raised these mega-files; the feature's small additions (socket handler, CLI subcommand, TabManager primitive, sidebar render fix, tests) push them a little further. Splitting them is out of scope. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
⌘⇧G now toggles for a single focused workspace: first press makes it a single-member group, pressing again on that solo group ungroups it. A workspace inside a multi-member group (its anchor or a child) is left untouched so the toggle never dissolves a group with other members. ⌘⇧P (new .renameFocusedWorkspaceGroup action) renames the focused workspace's group. It shares the chord with commandPalettePrevious, which only acts while the palette is open (in-palette nav, dispatched earlier), so they don't collide in practice. Full shortcut wiring: KeyboardShortcut Settings case/label/default, dispatch + handler, cmux.json schema, docs, localized label. ⌘⇧R (rename workspace) now also renames the group when the focused workspace is a group anchor, since the group name is what the header displays — hooked once in applyRenameFlow so every rename entrypoint (palette, context menu, shortcut) renames both. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7bcf7fd. Configure here.
| // in-palette navigation, dispatched earlier), so this fires | ||
| // only when the palette is closed. No-ops gracefully when the | ||
| // focused workspace isn't in a group. Rebind in Settings. | ||
| return StoredShortcut(key: "p", command: true, shift: true, option: false, control: false) |
There was a problem hiding this comment.
Rename group shortcut blocked
Medium Severity
The new renameFocusedWorkspaceGroup action uses the same default chord as commandPalette (⌘⇧P). Shortcut dispatch handles commandPalette first and always consumes that keypress, so the group-rename handler never runs with factory defaults despite docs describing ⌘⇧P for renaming a focused workspace’s group.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 7bcf7fd. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7bcf7fdcdc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let normalizedName, | ||
| let anchoredGroup = tabManager.workspaceGroups.first(where: { $0.anchorWorkspaceId == workspaceId }) { | ||
| tabManager.renameWorkspaceGroup(groupId: anchoredGroup.id, name: normalizedName) |
There was a problem hiding this comment.
When a promoted workspace is the group anchor, this special-case only fixes the command-palette rename flow. Other rename entrypoints still call setCustomTitle directly (for example socket/CLI workspace.rename in Sources/TerminalController.swift:5446 and the workspace rename alert in Sources/AppDelegate.swift:12436), but anchor display is sourced from group.name (TabManager.resolvedWorkspaceDisplayTitle), so those renames report success while the sidebar header/window title keep the old group name. Please move this anchor-aware behavior into a shared TabManager action used by every workspace rename path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 19234-19241: The catch block that rethrows validation failures
wraps the error with error.localizedDescription which can lose detail; update
the catch in the invocation of CodexTeamsApprovalBridge.validateWorkingDirectory
to construct CLIError using String(describing: error) instead of
error.localizedDescription so full error information is preserved when creating
the CLIError instance.
In `@Sources/KeyboardShortcutSettings.swift`:
- Around line 371-377: Update the misleading comment on the
.renameFocusedWorkspaceGroup shortcut: replace the reference to
commandPalettePrevious with commandPalette (Cmd+Shift+P is shared with
.commandPalette, not Ctrl+P), and mirror the same correction to the related
comment in AppDelegate where renameFocusedWorkspaceGroup is discussed so both
files consistently state the chord is shared with .commandPalette and explain
the palette-open vs closed dispatch behavior.
In `@Sources/TabManager.swift`:
- Around line 5336-5343: The snapshot path currently uses
SharedLiveAgentIndex.shared.currentIndexSchedulingRefresh() without checking
freshness, risking persisting stale resume metadata; update the logic used by
workspace.sessionSnapshot(includeScrollback:restorableAgentIndex:) so it only
supplies the cached index when the cache is both loaded and fresh (e.g., expose
or check a freshness/loaded flag on SharedLiveAgentIndex or compare a
version/timestamp), and otherwise perform the authoritative
RestorableAgentSessionIndex.load() before creating the snapshot (or skip
persisting until a fresh index is available); ensure you change the call site
that now passes currentIndexSchedulingRefresh() to validate freshness and fall
back to RestorableAgentSessionIndex.load() when stale or cold.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c30272a8-b1ee-4e35-850e-3cc950385cf1
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (9)
CLI/cmux.swiftResources/Localizable.xcstringsSources/AppDelegate.swiftSources/ContentView.swiftSources/KeyboardShortcutSettings.swiftSources/TabManager.swiftSources/TerminalController.swiftdocs/workspace-groups.mdweb/data/cmux.schema.json
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 19234-19241: The catch block that rethrows validation failures
wraps the error with error.localizedDescription which can lose detail; update
the catch in the invocation of CodexTeamsApprovalBridge.validateWorkingDirectory
to construct CLIError using String(describing: error) instead of
error.localizedDescription so full error information is preserved when creating
the CLIError instance.
In `@Sources/KeyboardShortcutSettings.swift`:
- Around line 371-377: Update the misleading comment on the
.renameFocusedWorkspaceGroup shortcut: replace the reference to
commandPalettePrevious with commandPalette (Cmd+Shift+P is shared with
.commandPalette, not Ctrl+P), and mirror the same correction to the related
comment in AppDelegate where renameFocusedWorkspaceGroup is discussed so both
files consistently state the chord is shared with .commandPalette and explain
the palette-open vs closed dispatch behavior.
In `@Sources/TabManager.swift`:
- Around line 5336-5343: The snapshot path currently uses
SharedLiveAgentIndex.shared.currentIndexSchedulingRefresh() without checking
freshness, risking persisting stale resume metadata; update the logic used by
workspace.sessionSnapshot(includeScrollback:restorableAgentIndex:) so it only
supplies the cached index when the cache is both loaded and fresh (e.g., expose
or check a freshness/loaded flag on SharedLiveAgentIndex or compare a
version/timestamp), and otherwise perform the authoritative
RestorableAgentSessionIndex.load() before creating the snapshot (or skip
persisting until a fresh index is available); ensure you change the call site
that now passes currentIndexSchedulingRefresh() to validate freshness and fall
back to RestorableAgentSessionIndex.load() when stale or cold.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c30272a8-b1ee-4e35-850e-3cc950385cf1
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (9)
CLI/cmux.swiftResources/Localizable.xcstringsSources/AppDelegate.swiftSources/ContentView.swiftSources/KeyboardShortcutSettings.swiftSources/TabManager.swiftSources/TerminalController.swiftdocs/workspace-groups.mdweb/data/cmux.schema.json
🛑 Comments failed to post (3)
CLI/cmux.swift (1)
19234-19241:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
String(describing:)when wrapping validation errors intoCLIError.At Line 19240,
error.localizedDescriptioncan collapse important details from non-LocalizedErrorfailures. Wrap withString(describing: error)so CLI diagnostics keep full context.Suggested patch
do { try CodexTeamsApprovalBridge.validateWorkingDirectory( commandArgs: commandArgs, baseDirectory: baseDirectory ) } catch { - throw CLIError(message: error.localizedDescription) + throw CLIError(message: String(describing: error)) }Based on learnings, CLI error formatting in this repo should use
String(describing: error)instead oferror.localizedDescriptionto preserve full error detail.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CLI/cmux.swift` around lines 19234 - 19241, The catch block that rethrows validation failures wraps the error with error.localizedDescription which can lose detail; update the catch in the invocation of CodexTeamsApprovalBridge.validateWorkingDirectory to construct CLIError using String(describing: error) instead of error.localizedDescription so full error information is preserved when creating the CLIError instance.Source: Learnings
Sources/KeyboardShortcutSettings.swift (1)
371-377:
⚠️ Potential issue | 🟡 Minor🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Verify shortcut conflict handling for Cmd+Shift+P between commandPalette and renameFocusedWorkspaceGroup echo "Searching for dispatch logic that handles Cmd+Shift+P priority between commandPalette and renameFocusedWorkspaceGroup..." rg -n -C5 'renameFocusedWorkspaceGroup|commandPalette' Sources/AppDelegate.swift | head -100Repository: manaflow-ai/cmux
Length of output: 4873
🏁 Script executed:
#!/bin/bash set -euo pipefail # 1) Confirm bindings for commandPalettePrevious vs commandPalette and the renameFocusedWorkspaceGroup comment context FILE="Sources/KeyboardShortcutSettings.swift" echo "== Relevant shortcut cases in $FILE ==" rg -n "case \.(commandPalettePrevious|commandPalette|renameFocusedWorkspaceGroup)" "$FILE" -n echo echo "== Neighborhood around renameFocusedWorkspaceGroup (approx 320-420) ==" sed -n '330,410p' "$FILE" | nl -ba | sed -n '1,90p' echo echo "== Neighborhood around commandPalettePrevious/commandPalette (approx 260-340) ==" sed -n '260,340p' "$FILE" | nl -ba | sed -n '1,90p'Repository: manaflow-ai/cmux
Length of output: 1037
🏁 Script executed:
#!/bin/bash set -euo pipefail # 2) Find where AppDelegate routes keyboard shortcut actions for commandPalette vs renameFocusedWorkspaceGroup FILE="Sources/AppDelegate.swift" echo "== References to renameFocusedWorkspaceGroup in $FILE ==" rg -n "renameFocusedWorkspaceGroup" "$FILE" || true echo echo "== References to commandPalette (routing/shortcut handling) in $FILE ==" rg -n "commandPalette" "$FILE" | head -200 echo echo "== Look for key/shortcut dispatch around the command palette rename workspace flows ==" rg -n -C6 "(renameFocusedWorkspaceGroup|commandPalette)(Opened|open|visible|isOpen|pending|dispatch|shortcut|route|performKeyEquivalent|keyEquivalent|handle)" "$FILE" | head -200Repository: manaflow-ai/cmux
Length of output: 15801
🏁 Script executed:
#!/bin/bash set -euo pipefail FILE="Sources/KeyboardShortcutSettings.swift" echo "== KeyboardShortcutSettings.swift around renameFocusedWorkspaceGroup (lines ~350-390) ==" sed -n '350,390p' "$FILE" | cat -n echo echo "== KeyboardShortcutSettings.swift around commandPalette/commandPalettePrevious (lines ~280-330) ==" sed -n '280,330p' "$FILE" | cat -nRepository: manaflow-ai/cmux
Length of output: 7526
🏁 Script executed:
#!/bin/bash set -euo pipefail FILE="Sources/AppDelegate.swift" echo "== AppDelegate.swift around renameFocusedWorkspaceGroup match (lines ~13180-13260) ==" sed -n '13160,13260p' "$FILE" | cat -n echo echo "== AppDelegate.swift around commandPalette handler (lines ~12730-12840) ==" sed -n '12720,12860p' "$FILE" | cat -nRepository: manaflow-ai/cmux
Length of output: 13581
Fix misleading chord-conflict comment for Cmd+Shift+P (.renameFocusedWorkspaceGroup)
The comment says it shares the chord with
commandPalettePrevious, butcommandPalettePreviousis Ctrl+P; the actual shared Cmd+Shift+P binding is.commandPalette. The “palette open vs closed” behavior described matches howAppDelegate.swiftroutes.commandPalettewhen the palette is effective, so only the conflicting action name needs correction (also update the similar comment inSources/AppDelegate.swiftforrenameFocusedWorkspaceGroup).🔧 Proposed fix
case .renameFocusedWorkspaceGroup: - // Cmd+Shift+P. Shares the chord with commandPalettePrevious, + // Cmd+Shift+P. Shares the chord with commandPalette, // but that only acts while the command palette is OPEN (it's // in-palette navigation, dispatched earlier), so this fires // only when the palette is closed. No-ops gracefully when the // focused workspace isn't in a group. Rebind in Settings. return StoredShortcut(key: "p", command: true, shift: true, option: false, control: false)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/KeyboardShortcutSettings.swift` around lines 371 - 377, Update the misleading comment on the .renameFocusedWorkspaceGroup shortcut: replace the reference to commandPalettePrevious with commandPalette (Cmd+Shift+P is shared with .commandPalette, not Ctrl+P), and mirror the same correction to the related comment in AppDelegate where renameFocusedWorkspaceGroup is discussed so both files consistently state the chord is shared with .commandPalette and explain the palette-open vs closed dispatch behavior.Sources/TabManager.swift (1)
5336-5343:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't persist closed-workspace snapshots from an unchecked cache.
This is a history/snapshot path, but the new branch only distinguishes “cache missing” from “cache present.” If
SharedLiveAgentIndexis warm but older than the authoritative restorable-agent index,workspace.sessionSnapshot(...)will persist stale resume metadata for the closed workspace. The nil fallback also means the original synchronousload()is still reachable on the main-actor close path, so the freeze is only avoided when the cache is both loaded and fresh.As per coding guidelines, when a persistence/history/snapshot path substitutes a cached value for an authoritative read, it must explicitly handle both cold and stale caches or document a graceful-degradation rationale.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/TabManager.swift` around lines 5336 - 5343, The snapshot path currently uses SharedLiveAgentIndex.shared.currentIndexSchedulingRefresh() without checking freshness, risking persisting stale resume metadata; update the logic used by workspace.sessionSnapshot(includeScrollback:restorableAgentIndex:) so it only supplies the cached index when the cache is both loaded and fresh (e.g., expose or check a freshness/loaded flag on SharedLiveAgentIndex or compare a version/timestamp), and otherwise perform the authoritative RestorableAgentSessionIndex.load() before creating the snapshot (or skip persisting until a fresh index is available); ensure you change the call site that now passes currentIndexSchedulingRefresh() to validate freshness and fall back to RestorableAgentSessionIndex.load() when stale or cold.Source: Coding guidelines
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Summary
⌘⇧Gturns the focused workspace into a group when there's no multi-selection. Right-click → New Group from Workspace does the same. CLI/socket too.What changed
TabManager.makeWorkspaceGroupFromWorkspace(anchorWorkspaceId:name:)— promote an existing ungrouped workspace to be the anchor of a new single-member group, in place. No-ops for a missing or already-grouped workspace.⌘⇧Gkeep the documented fresh-anchorcreatecontract.⌘⇧Gpromotes the focused workspace, deferring to React Grab when React Grab would act (browser focused, or a single browser reachable from the focused terminal).workspace.group.from_workspace+ CLIcmux workspace group from-workspace --workspace <ws> [--name <name>].LazyVStack+ForEachwhere a top-level slot switches between a.workspacerow and a.groupHeader(different view types). LazyVStack mis-diffs that in-place type change, so on the focused workspace the header never appeared on create and a stale header lingered after ungroup. The data model was always correct (verified via the socket/logs); only the view didn't update. Fixed by keying the rows on a signature of the group-anchor set, so the list rebuilds cleanly when a workspace starts/stops being a header. Sorted, so reorders/renames don't trigger a rebuild and drag-reorder is unaffected. Multi-select grouping never hit this because it inserts a brand-new anchor rather than transforming an existing row.Mechanism / principled?
Mostly principled. The promote primitive reuses the existing anchor model instead of adding a parallel concept. The render fix is the one pragmatic part: it forces a clean list rebuild on group-structure change rather than fully reworking the heterogeneous-row identity, because LazyVStack's in-place view-type swap is unreliable. Known minor tradeoff: on a group create/ungroup the sidebar scroll resets to top (only visible when the sidebar is actually scrolled). Worth a follow-up to restore scroll-to-selection if it bothers anyone; correctness first.
Testing
cmuxTests/WorkspaceGroupTests.swift: promote-in-place + round-trip, pinned-tier stability, no-op for an already-grouped workspace, header-only rendering. (CItestsjob is green.)w2grp) with real keyboard input + cua window screenshots:⌘⇧Gon the focused terminal workspace renders the group header in place, and ungroup reverts it to a plain row. The data-level round-trip (from_workspace→list→ungroup) is also confirmed via the socket.Issues
⌘⇧Gin a terminal triggers it.Summary by CodeRabbit
New Features
Context Menu
Behavior
Tests
Documentation
Localization
Note
Medium Risk
Touches sidebar rendering, keyboard routing (⌘⇧G vs React Grab), and workspace/group state; changes are localized but affect daily navigation UX.
Overview
Adds in-place “promote workspace to group”: an ungrouped workspace becomes the group anchor with no extra anchor tab, inheriting its title and pin tier.
⌘⇧Gstill groups 2+ sidebar selections under a fresh anchor; with ≤1 selection it promotes the focused workspace when React Grab would not handle the chord, and toggles ungroup for a solo single-member group. Context menu New Group from Workspace and socketworkspace.group.from_workspace/ CLIfrom-workspacecall the same primitive.Sidebar fix: keys the workspace list on the set of group anchor IDs so
LazyVStackrebuilds when a row flips between plain workspace and group header (fixes invisible/stale headers).Rename: renaming a workspace that is a group anchor updates the group name; new
renameFocusedWorkspaceGroupshortcut (⌘⇧P, palette-closed only) opens the group rename prompt. Docs, schema, and tests cover promote/ungroup round-trip, pinned tier, and already-grouped errors.Reviewed by Cursor Bugbot for commit 7bcf7fd. Bugbot is set up for automated code reviews on this repo. Configure here.
Group keyboard shortcuts (follow-up)
⌘⇧Gtoggles for a single focused workspace: first press makes it a single-member group, pressing again on that solo group ungroups it. A workspace inside a multi-member group (anchor or child) is left untouched, so the toggle never dissolves a group with other members.⌘⇧P(new.renameFocusedWorkspaceGroup) renames the focused workspace's group. It shares the chord withcommandPalettePrevious, which only acts while the command palette is open (in-palette navigation, dispatched earlier), so they don't collide in practice. Rebindable in Settings. Fully wired (KeyboardShortcutSettings, dispatch + handler, schema, localized label, docs).⌘⇧R(rename workspace) now also renames the group when the focused workspace is a group anchor — the group name is what the header displays. Hooked once inapplyRenameFlow, so every rename entrypoint (palette, context menu, shortcut) renames both.⌘⇧Gcreate/ungroup toggle and multi-member no-op;⌘⇧Popens the group rename dialog. The rename completion and⌘⇧Rdual-rename use the same model paths as the existing (working) header rename, but the modal/palette text entry wasn't automatable, so they're flagged for dogfood.