Repository navigation
Add one-step grouped workspace creation - #6657
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds optional workspace group placement ( ChangesWorkspace Group Placement
Sequence Diagram(s)sequenceDiagram
participant CLI as cmux CLI
participant TerminalController as TerminalController<br/>(v2WorkspaceCreate)
participant TabManager as TabManager
participant ControlWorkspaceGroupContext as ControlWorkspaceGroupContext
CLI->>TerminalController: new-workspace --group G --group-placement after-current --group-reference R
TerminalController->>TerminalController: parse & validate group_id, group_placement, group_reference_workspace_id
TerminalController->>TabManager: addWorkspace(...)
TabManager-->>TerminalController: workspaceID
TerminalController->>TabManager: validate group G exists
TerminalController->>ControlWorkspaceGroupContext: controlAddWorkspaceToGroup(groupID, workspaceID, placement, referenceWorkspaceID)
ControlWorkspaceGroupContext->>TabManager: addWorkspaceToGroup(workspaceId, groupId, placement, referenceWorkspaceId)
TabManager-->>ControlWorkspaceGroupContext: result
ControlWorkspaceGroupContext-->>TerminalController: resolution
TerminalController-->>CLI: {workspace_id, group_id, group_ref}
sequenceDiagram
participant CLI as cmux CLI
participant Coordinator as Coordinator<br/>(workspaceGroupAdd)
participant TerminalController as TerminalController<br/>(controlAddWorkspaceToGroup)
CLI->>Coordinator: workspace.group.add {placement, reference_workspace_id}
Coordinator->>Coordinator: parse placement to WorkspaceGroupNewPlacement
Coordinator->>Coordinator: validate reference_workspace_id as UUID
Coordinator->>TerminalController: controlAddWorkspaceToGroup(groupID, workspaceID, placement?, referenceWorkspaceID?)
TerminalController-->>Coordinator: ControlWorkspaceGroupAddResolution
Coordinator-->>CLI: response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning, 1 inconclusive)
✅ Passed checks (19 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
Greptile SummaryThis PR wires up one-step grouped workspace creation across the CLI, socket API, and mobile create path.
Confidence Score: 4/5Safe to merge with the localization gap addressed; group creation and validation logic is correct and well-tested. The new workspaceGroup.error.invalidReferenceWorkspace string is added to the catalog with only en and ja entries while the catalog already carries 10+ additional locales for the same domain. Users in Korean, German, French, Russian, Thai, and other supported locales will see the English fallback for this error. The rest of the change — parameter parsing, validation, protocol updates, and new tests — is clean. Resources/Localizable.xcstrings — the new error string needs locale entries for all catalog-supported locales beyond en/ja. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant CLI as CLI / socket caller
participant Coord as ControlCommandCoordinator
participant TC as TerminalController
participant TM as TabManager
CLI->>Coord: workspace.create(group_id, group_placement, group_reference_workspace_id)
Coord->>TC: v2WorkspaceCreate(params)
TC->>TM: v2MainSync: workspaceGroups.contains(group_id) + tabs.contains(referenceId)
TM-->>TC: (groupExists, referenceIsMember)
alt group not found
TC-->>Coord: .err not_found
else invalid reference
TC-->>Coord: .err invalid_params
end
TC->>TM: v2MainSync: addWorkspace(...) + addWorkspaceToGroup(placement, reference)
TM-->>TC: ws.id
TC-->>Coord: .ok workspace_id, group_id, group_ref
Coord-->>CLI: response
CLI->>Coord: workspace.group.add(group_id, workspace_id, placement, reference_workspace_id)
Coord->>TC: controlAddWorkspaceToGroup(groupID, workspaceID, placement, referenceWorkspaceID)
TC->>TM: tabs.contains(referenceId in group)
alt invalid reference
TC-->>Coord: .invalidReferenceWorkspace
Coord-->>CLI: .err invalid_params
end
TC->>TM: addWorkspaceToGroup(workspaceId, groupId, placement, referenceWorkspaceId)
TM-->>TC: tab.groupId updated
TC-->>Coord: .added
Coord-->>CLI: .ok group_id, workspace_id
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant CLI as CLI / socket caller
participant Coord as ControlCommandCoordinator
participant TC as TerminalController
participant TM as TabManager
CLI->>Coord: workspace.create(group_id, group_placement, group_reference_workspace_id)
Coord->>TC: v2WorkspaceCreate(params)
TC->>TM: v2MainSync: workspaceGroups.contains(group_id) + tabs.contains(referenceId)
TM-->>TC: (groupExists, referenceIsMember)
alt group not found
TC-->>Coord: .err not_found
else invalid reference
TC-->>Coord: .err invalid_params
end
TC->>TM: v2MainSync: addWorkspace(...) + addWorkspaceToGroup(placement, reference)
TM-->>TC: ws.id
TC-->>Coord: .ok workspace_id, group_id, group_ref
Coord-->>CLI: response
CLI->>Coord: workspace.group.add(group_id, workspace_id, placement, reference_workspace_id)
Coord->>TC: controlAddWorkspaceToGroup(groupID, workspaceID, placement, referenceWorkspaceID)
TC->>TM: tabs.contains(referenceId in group)
alt invalid reference
TC-->>Coord: .invalidReferenceWorkspace
Coord-->>CLI: .err invalid_params
end
TC->>TM: addWorkspaceToGroup(workspaceId, groupId, placement, referenceWorkspaceId)
TM-->>TC: tab.groupId updated
TC-->>Coord: .added
Coord-->>CLI: .ok group_id, workspace_id
Reviews (5): Last reviewed commit: "Localize invalid group reference errors" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+13301
to
+13335
| @@ -13286,6 +13325,14 @@ class TerminalController { | |||
| if let layoutNode { | |||
| ws.applyCustomLayout(layoutNode, baseCwd: cwd ?? ws.currentDirectory) | |||
| } | |||
| if let groupId { | |||
| tabManager.addWorkspaceToGroup( | |||
| workspaceId: ws.id, | |||
| groupId: groupId, | |||
| placement: groupPlacement ?? .top, | |||
| referenceWorkspaceId: groupReferenceWorkspaceId | |||
| ) | |||
| } | |||
Contributor
There was a problem hiding this comment.
The group is validated in one v2MainSync call (line 13301–13312) and then the workspace is created and grouped in a separate v2MainSync call (line 13313–13338). Because v2MainSync serializes to the main thread one dispatch at a time, another concurrent socket command (e.g. workspace.group.delete) can run between the two blocks, deleting the group after groupExists = true is set. When that happens, addWorkspaceToGroup silently no-ops (as the existing comment notes), so the workspace is created but never added to the group — yet the response returns the group_id as if the association succeeded, misleading the caller.
The group-existence guard and the addWorkspaceToGroup call should both live inside the same v2MainSync closure so the check and the mutation are atomic on the main actor.
Comment on lines
+13272
to
+13275
| } else if v2HasNonNullParam(params, "reference_workspace_id") { | ||
| guard let parsed = v2UUID(params, "reference_workspace_id") else { | ||
| return .err(code: "invalid_params", message: "Missing or invalid group_reference_workspace_id", data: nil) | ||
| } |
Contributor
There was a problem hiding this comment.
reference_workspace_id with a non-UUID value, the error message says "Missing or invalid group_reference_workspace_id" — referencing the wrong parameter name. A caller who passed reference_workspace_id will be confused about which field to fix.
Suggested change
| } else if v2HasNonNullParam(params, "reference_workspace_id") { | |
| guard let parsed = v2UUID(params, "reference_workspace_id") else { | |
| return .err(code: "invalid_params", message: "Missing or invalid group_reference_workspace_id", data: nil) | |
| } | |
| } else if v2HasNonNullParam(params, "reference_workspace_id") { | |
| guard let parsed = v2UUID(params, "reference_workspace_id") else { | |
| return .err(code: "invalid_params", message: "Missing or invalid reference_workspace_id", data: nil) | |
| } |
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!
lawrencecchen
force-pushed
the
feat-workspace-create-group-placement
branch
from
June 23, 2026 05:12
187a315 to
42f7467
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 14775-14777: The help text for the workspace create command is
missing documentation for the --group-reference flag. Locate the create command
help text (the multi-line description starting with "Create a workspace") and
add --group-reference to the list of inherited group-related flags. The help
text currently lists --env, --env-file, --group, and --group-placement, but
should also include --group-reference to provide complete documentation of all
supported flags for this command.
In
`@Packages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/WorkspaceGroup/ControlCommandCoordinator`+WorkspaceGroup.swift:
- Around line 275-277: The presence check for reference_workspace_id incorrectly
treats an explicit JSON null as invalid. Modify the condition in the if
statement to distinguish between an absent field and an explicit null value.
Instead of only checking if params["reference_workspace_id"] != nil, also verify
that the value is not a null/NSNull type before determining it as an invalid
parameter. This way, both missing and explicitly null reference_workspace_id
will be treated as valid (absent optional), while only invalid non-null values
will trigger the error.
In `@Sources/TerminalController.swift`:
- Around line 13256-13264: The error message strings "Missing or invalid
group_id" and "Invalid group_placement" are user-facing strings that must be
localized according to coding guidelines. Wrap each error message with
String(localized:defaultValue:) using an appropriate localization key, then add
matching entries to Resources/Localizable.xcstrings for all supported locales.
Apply the same localization pattern to all error messages in the affected
sections (lines 13256-13264, 13268-13275, and 13306-13309) to ensure consistent
internationalization throughout the socket error responses.
- Around line 13301-13312: Combine the group existence validation (currently in
the v2MainSync block checking if groupId exists and if
tabManager.workspaceGroups contains it) with the workspace creation and
attachment logic that occurs in a separate v2MainSync call around lines
13328-13335 into a single main-actor transaction. This ensures that the group
cannot be deleted or invalidated between the validation check and the actual
attachment, preventing the race condition where an ungrouped workspace could be
created while returning successful group metadata.
- Around line 13254-13279: The code is parsing group-scoped parameters like
group_placement, group_reference_workspace_id, and reference_workspace_id
without validating that group_id is also provided. Add validation logic after
parsing these parameters to check if any of them (rawGroupPlacement,
groupReferenceWorkspaceId, or the reference_workspace_id param) are non-null
when groupId is nil, and return an error response in such cases. This ensures
that group-scoped options are only accepted when group_id is actually provided,
preventing silent ignoring of the caller's grouping intent.
In `@Sources/TerminalController`+ControlWorkspaceGroupContext.swift:
- Around line 207-212: The call to tabManager.addWorkspaceToGroup with
referenceWorkspaceId returns success based only on checking if tab.groupId
equals groupID, but does not validate that the referenceWorkspaceID is valid and
belongs to the target group. This means the method could report success while
the workspace is not actually moved. Either add validation to confirm that
referenceWorkspaceID belongs to the target group before returning success, or
refactor addWorkspaceToGroup to return an explicit placement result that
indicates whether the operation actually succeeded rather than inferring success
from membership alone.
🪄 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: 1f871cad-d741-4d67-9b1e-a3a3f3f1fe87
📒 Files selected for processing (8)
CLI/cmux.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/WorkspaceGroup/ControlCommandCoordinator+WorkspaceGroup.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/WorkspaceGroup/ControlWorkspaceGroupContext.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandContextTestStubs.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandCoordinatorWorkspaceTests.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/FakeWorkspaceControlCommandContext.swiftSources/TerminalController+ControlWorkspaceGroupContext.swiftSources/TerminalController.swift
Comment on lines
+13254
to
+13279
| let groupId = v2UUID(params, "group_id") | ||
| if v2HasNonNullParam(params, "group_id"), groupId == nil { | ||
| return .err(code: "invalid_params", message: "Missing or invalid group_id", data: nil) | ||
| } | ||
| let rawGroupPlacement = v2RawString(params, "group_placement") | ||
| ?? (groupId == nil ? nil : v2RawString(params, "placement")) | ||
| let groupPlacement = WorkspaceGroupNewPlacement(rawString: rawGroupPlacement) | ||
| if let raw = rawGroupPlacement, | ||
| !raw.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty, | ||
| groupPlacement == nil { | ||
| return .err(code: "invalid_params", message: "Invalid group_placement", data: ["group_placement": raw]) | ||
| } | ||
| let groupReferenceWorkspaceId: UUID? | ||
| if v2HasNonNullParam(params, "group_reference_workspace_id") { | ||
| guard let parsed = v2UUID(params, "group_reference_workspace_id") else { | ||
| return .err(code: "invalid_params", message: "Missing or invalid group_reference_workspace_id", data: nil) | ||
| } | ||
| groupReferenceWorkspaceId = parsed | ||
| } else if v2HasNonNullParam(params, "reference_workspace_id") { | ||
| guard let parsed = v2UUID(params, "reference_workspace_id") else { | ||
| return .err(code: "invalid_params", message: "Missing or invalid group_reference_workspace_id", data: nil) | ||
| } | ||
| groupReferenceWorkspaceId = parsed | ||
| } else { | ||
| groupReferenceWorkspaceId = nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject group-scoped options when group_id is missing.
group_placement, group_reference_workspace_id, and reference_workspace_id are parsed even without group_id, but Line 13328 only applies grouping when groupId exists. A request can therefore create an ungrouped workspace successfully while silently ignoring the caller’s group placement/reference intent.
Suggested direction
+ let hasGroupScopedOptions =
+ v2HasNonNullParam(params, "group_placement")
+ || v2HasNonNullParam(params, "group_reference_workspace_id")
+ || v2HasNonNullParam(params, "reference_workspace_id")
let groupId = v2UUID(params, "group_id")
if v2HasNonNullParam(params, "group_id"), groupId == nil {
return .err(code: "invalid_params", message: "Missing or invalid group_id", data: nil)
}
+ if groupId == nil, hasGroupScopedOptions {
+ return .err(code: "invalid_params", message: "group_id is required for group placement options", data: nil)
+ }🤖 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 13254 - 13279, The code is
parsing group-scoped parameters like group_placement,
group_reference_workspace_id, and reference_workspace_id without validating that
group_id is also provided. Add validation logic after parsing these parameters
to check if any of them (rawGroupPlacement, groupReferenceWorkspaceId, or the
reference_workspace_id param) are non-null when groupId is nil, and return an
error response in such cases. This ensures that group-scoped options are only
accepted when group_id is actually provided, preventing silent ignoring of the
caller's grouping intent.
lawrencecchen
force-pushed
the
feat-workspace-create-group-placement
branch
from
June 23, 2026 05:23
42f7467 to
cc839f8
Compare
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
`@Packages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/WorkspaceGroup/ControlCommandCoordinator`+WorkspaceGroup.swift:
- Around line 272-273: The error messages "Invalid placement" and "Missing or
invalid reference_workspace_id" in the workspace group error responses are
hardcoded English strings that violate localization requirements. Replace these
bare string literals with localization function calls (using the appropriate
localization system for this codebase) and create corresponding translation keys
in all supported locale files. Ensure both error messages at line 272 and line
277 are properly localized through the same mechanism used elsewhere in the
codebase for user-facing API/command output strings.
In `@Sources/TerminalController`+WorkspaceCreate.swift:
- Around line 52-60: The error messages being returned in the workspace creation
error handling (specifically the "Invalid group_placement" message and similar
messages at the referenced line ranges) are not localized for user-facing
output. Wrap each message string with String(localized: "key.description",
defaultValue: "English message") using appropriate localization keys, and add
corresponding entries to Resources/Localizable.xcstrings for all supported
locales. Apply this fix to the error return statements throughout the method,
including the ones at lines 52-60, 65-70, and 102-105.
- Around line 50-75: The code currently accepts group-related parameters
(group_reference_workspace_id, reference_workspace_id, groupPlacement) even when
group_id is not provided, and it returns group fields without verifying that
addWorkspaceToGroup actually succeeded. Add validation to reject
group_reference_workspace_id or reference_workspace_id if group_id is nil,
validate that any provided reference workspace actually belongs to the target
group before creating the workspace, and after the workspace creation is
persisted, only include group fields in the response if ws.groupId == groupId is
confirmed. This validation logic needs to be added around the parameter parsing
section (where group_id, group_placement, and reference parameters are
processed) and the persistence section (where addWorkspaceToGroup is called and
results are returned).
🪄 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: 36d6859c-3f1a-402c-800c-dfe7c4ebcf6f
📒 Files selected for processing (10)
CLI/cmux.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/WorkspaceGroup/ControlCommandCoordinator+WorkspaceGroup.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/WorkspaceGroup/ControlWorkspaceGroupContext.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandContextTestStubs.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandCoordinatorWorkspaceTests.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/FakeWorkspaceControlCommandContext.swiftSources/TerminalController+ControlWorkspaceGroupContext.swiftSources/TerminalController+WorkspaceCreate.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxproj
💤 Files with no reviewable changes (1)
- Sources/TerminalController.swift
Comment on lines
+50
to
+75
| let groupId = v2UUID(params, "group_id") | ||
| if v2HasNonNullParam(params, "group_id"), groupId == nil { | ||
| return .err(code: "invalid_params", message: "Missing or invalid group_id", data: nil) | ||
| } | ||
| let rawGroupPlacement = v2RawString(params, "group_placement") | ||
| ?? (groupId == nil ? nil : v2RawString(params, "placement")) | ||
| let groupPlacement = WorkspaceGroupNewPlacement(rawString: rawGroupPlacement) | ||
| if let raw = rawGroupPlacement, | ||
| !raw.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty, | ||
| groupPlacement == nil { | ||
| return .err(code: "invalid_params", message: "Invalid group_placement", data: ["group_placement": raw]) | ||
| } | ||
| let groupReferenceWorkspaceId: UUID? | ||
| if v2HasNonNullParam(params, "group_reference_workspace_id") { | ||
| guard let parsed = v2UUID(params, "group_reference_workspace_id") else { | ||
| return .err(code: "invalid_params", message: "Missing or invalid group_reference_workspace_id", data: nil) | ||
| } | ||
| groupReferenceWorkspaceId = parsed | ||
| } else if v2HasNonNullParam(params, "reference_workspace_id") { | ||
| guard let parsed = v2UUID(params, "reference_workspace_id") else { | ||
| return .err(code: "invalid_params", message: "Missing or invalid group_reference_workspace_id", data: nil) | ||
| } | ||
| groupReferenceWorkspaceId = parsed | ||
| } else { | ||
| groupReferenceWorkspaceId = nil | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate group targeting before persisting the workspace.
group_reference_workspace_id / reference_workspace_id are accepted without group_id and then ignored, and the create path reports group_id/group_ref without confirming addWorkspaceToGroup actually attached the new workspace. Reject orphaned group-placement/reference params, validate the reference belongs to the target group before creation, and only return group fields after ws.groupId == groupId is confirmed.
Suggested direction
let groupId = v2UUID(params, "group_id")
if v2HasNonNullParam(params, "group_id"), groupId == nil {
return .err(code: "invalid_params", message: "Missing or invalid group_id", data: nil)
}
+ if groupId == nil,
+ v2HasNonNullParam(params, "group_placement")
+ || v2HasNonNullParam(params, "group_reference_workspace_id")
+ || v2HasNonNullParam(params, "reference_workspace_id") {
+ return .err(
+ code: "invalid_params",
+ message: "group_id is required when using group placement parameters",
+ data: nil
+ )
+ }Also validate any non-nil reference against the target group’s anchor/member set before tabManager.addWorkspace(...), and capture a post-add failure instead of unconditionally returning group_id.
Also applies to: 97-131, 140-146
🤖 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`+WorkspaceCreate.swift around lines 50 - 75, The
code currently accepts group-related parameters (group_reference_workspace_id,
reference_workspace_id, groupPlacement) even when group_id is not provided, and
it returns group fields without verifying that addWorkspaceToGroup actually
succeeded. Add validation to reject group_reference_workspace_id or
reference_workspace_id if group_id is nil, validate that any provided reference
workspace actually belongs to the target group before creating the workspace,
and after the workspace creation is persisted, only include group fields in the
response if ws.groupId == groupId is confirmed. This validation logic needs to
be added around the parameter parsing section (where group_id, group_placement,
and reference parameters are processed) and the persistence section (where
addWorkspaceToGroup is called and results are returned).
Comment on lines
+52
to
+60
| return .err(code: "invalid_params", message: "Missing or invalid group_id", data: nil) | ||
| } | ||
| let rawGroupPlacement = v2RawString(params, "group_placement") | ||
| ?? (groupId == nil ? nil : v2RawString(params, "placement")) | ||
| let groupPlacement = WorkspaceGroupNewPlacement(rawString: rawGroupPlacement) | ||
| if let raw = rawGroupPlacement, | ||
| !raw.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty, | ||
| groupPlacement == nil { | ||
| return .err(code: "invalid_params", message: "Invalid group_placement", data: ["group_placement": raw]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Localize the new group-create error messages.
These message: values are returned through user-facing socket/CLI/mobile flows, but they are bare English strings. Wrap the new group errors with String(localized: "…", defaultValue: "…") and add matching Resources/Localizable.xcstrings entries for every supported locale.
As per coding guidelines, “All user-facing strings must be localized using String(localized: "key.name", defaultValue: "English text").” As per path instructions, apply .github/review-bot-rules/full-internationalization.md for production user-facing text.
Also applies to: 65-70, 102-105
🤖 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`+WorkspaceCreate.swift around lines 52 - 60, The
error messages being returned in the workspace creation error handling
(specifically the "Invalid group_placement" message and similar messages at the
referenced line ranges) are not localized for user-facing output. Wrap each
message string with String(localized: "key.description", defaultValue: "English
message") using appropriate localization keys, and add corresponding entries to
Resources/Localizable.xcstrings for all supported locales. Apply this fix to the
error return statements throughout the method, including the ones at lines
52-60, 65-70, and 102-105.
Sources: Coding guidelines, Path instructions
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit bf83e6a. Configure here.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/TerminalController+WorkspaceCreate.swift (2)
88-100: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTreat
layout: nullas omitted.
layoutis optional, but an explicit JSONnullstill enters the decode branch and fails the request instead of using the default layout.Suggested fix
- if let rawLayout = params["layout"] { - guard JSONSerialization.isValidJSONObject(rawLayout), + if v2HasNonNullParam(params, "layout") { + guard let rawLayout = params["layout"], + JSONSerialization.isValidJSONObject(rawLayout), let layoutData = try? JSONSerialization.data(withJSONObject: rawLayout) else { return .err(code: "invalid_params", message: "layout must be a valid JSON object", data: nil) }🤖 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`+WorkspaceCreate.swift around lines 88 - 100, The current check `if let rawLayout = params["layout"]` evaluates to true even when the JSON value is explicitly null, causing the decode logic to attempt validation and fail. Add an additional guard condition to check that rawLayout is not an instance of NSNull before proceeding with JSONSerialization validation and JSONDecoder decoding. This will ensure that an explicit null value is treated the same as an omitted layout parameter, allowing the default layout to be used without error.
94-99: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winLocalize the new non-group workspace-create errors.
These errors are returned through socket/mobile user-facing flows but remain bare English strings. Route them through
String(localized:defaultValue:)and add matchingResources/Localizable.xcstringsentries for all supported locales. As per coding guidelines, “All user-facing strings must be localized usingString(localized: "key.name", defaultValue: "English text").” As per path instructions, apply.github/review-bot-rules/full-internationalization.mdfor production user-facing text.Also applies to: 147-148, 163-166
🤖 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`+WorkspaceCreate.swift around lines 94 - 99, The error messages in the workspace-create flow, including the "invalid layout" error in the JSONDecoder catch block and other error returns mentioned at the specified line ranges, are currently bare English strings that need to be localized. Wrap each user-facing error message string with the String(localized:defaultValue:) pattern, providing a descriptive key and the English text as the default value. Then add corresponding entries to the Resources/Localizable.xcstrings file for all supported locales to provide translations for each localized key, ensuring all error messages across the workspace-create operation are properly internationalized.Sources: Coding guidelines, Path instructions
🤖 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.
Outside diff comments:
In `@Sources/TerminalController`+WorkspaceCreate.swift:
- Around line 88-100: The current check `if let rawLayout = params["layout"]`
evaluates to true even when the JSON value is explicitly null, causing the
decode logic to attempt validation and fail. Add an additional guard condition
to check that rawLayout is not an instance of NSNull before proceeding with
JSONSerialization validation and JSONDecoder decoding. This will ensure that an
explicit null value is treated the same as an omitted layout parameter, allowing
the default layout to be used without error.
- Around line 94-99: The error messages in the workspace-create flow, including
the "invalid layout" error in the JSONDecoder catch block and other error
returns mentioned at the specified line ranges, are currently bare English
strings that need to be localized. Wrap each user-facing error message string
with the String(localized:defaultValue:) pattern, providing a descriptive key
and the English text as the default value. Then add corresponding entries to the
Resources/Localizable.xcstrings file for all supported locales to provide
translations for each localized key, ensuring all error messages across the
workspace-create operation are properly internationalized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 030d96b4-d377-429c-9953-e5f983452eba
📒 Files selected for processing (4)
Packages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/WorkspaceGroup/ControlCommandCoordinator+WorkspaceGroup.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandCoordinatorWorkspaceTests.swiftResources/Localizable.xcstringsSources/TerminalController+WorkspaceCreate.swift
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
Dogfood tag: http://127.0.0.1:17320/grpcrt
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Changes workspace creation and sidebar group ordering paths used by CLI and mobile create; invalid group/reference now errors before commit, but wrong placement could still surprise users if callers pass inconsistent params.
Overview
Enables creating a workspace and joining a workspace group in one call, instead of create-then-
workspace.group.add.CLI (
cmux new-workspace/cmux workspace create) gains--group,--group-placement(afterCurrent|top|end), and--group-reference, mapped togroup_id,group_placement, andgroup_reference_workspace_idon the control API.v2WorkspaceCreate(moved toTerminalController+WorkspaceCreate.swift) validates group existence, requiresgroup_idwhen placement/reference is set, rejects bad placement and references that are not in the target group, then callsaddWorkspaceToGroupwith placement (default top). Success responses includegroup_id/group_ref.workspace.group.addnow accepts optionalplacementandreference_workspace_id(including explicitnull), forwards them through the control seam toTabManager, and returnsinvalid_paramswhen the reference workspace is not a group member (new localized string).Coordinator tests cover forwarding, null reference, invalid placement, and invalid reference handling.
Reviewed by Cursor Bugbot for commit 8dd2d30. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Create a workspace and place it into a group in one step. Adds placement/reference across CLI and API with strict validation and localized errors; responses include group info when grouping is used.
New Features
cmux new-workspaceandcmux workspace createadd--group <id|ref>,--group-placement <afterCurrent|top|end>, and--group-reference <workspace>.workspace.createacceptsgroup_id,group_placement(defaulttop), andgroup_reference_workspace_id; requiresgroup_idwhen placement/reference is provided; validates group existence and that the reference is in the target group; returnsgroup_id/group_ref. Also accepts aliasesplacementandreference_workspace_id.workspace.group.addacceptsplacementandreference_workspace_id(nullable); rejects invalid placement, non-UUID references, and references not in the target group withinvalid_params; forwards placement/reference to the controller; invalid-reference errors are localized.Migration
group_id; if you send a reference, it must be in that group.Written for commit 8dd2d30. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
New Features
new-workspace:--group,--group-placement, and--group-reference(and updated examples/help accordingly).Bug Fixes
invalid_paramsinstead of creating the workspace without grouping.Documentation
new-workspacehelp/usage text and shortened relatedlist-workspaces → createhelp to match the same flags.