Repository navigation
Make group new workspace placement configurable - #5018
Merged
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughAdds an afterCurrent placement option and threads placement plus optional referenceWorkspaceId through TabManager, AppDelegate, CloudVM launcher, settings parsing/UI, CLI/docs/schema, localization, and tests. ChangesWorkspace Group Placement Enhancement
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (3 errors, 1 warning, 1 inconclusive)
✅ Passed checks (13 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
Contributor
Greptile SummaryThis PR makes the placement of new workspaces inside groups fully configurable (
Confidence Score: 5/5Safe to merge; changes are isolated to group workspace creation ordering and have no impact on data integrity or security. The placement logic is well-guarded with proper fallbacks at every branch, actor isolation is consistent throughout, the old timing-based Task.sleep is correctly replaced by a process-exit signal, localization is complete across all 20 supported locales on both Swift and web surfaces, and the new test suite exercises placement, fallback, and config-parsing paths. Sources/AppDelegate.swift — the finish(workspaceId:) early-return path silently disposes the observer; worth a log line for diagnosability. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant AppDelegate
participant TabManager
participant Observer as ConfiguredGroupActionAsyncWorkspaceObserver
participant CloudVM as CloudVMActionLauncher
Note over User,CloudVM: Cmd-N on group member (afterCurrent default)
User->>AppDelegate: Cmd-N keypress
AppDelegate->>AppDelegate: workspaceGroupNewWorkspaceTarget(in:)
Note right of AppDelegate: resolves placement per-cwd then global then default
alt configured action is builtIn(.newWorkspace)
AppDelegate->>TabManager: createWorkspaceInGroup(groupId, placement, referenceWorkspaceId)
TabManager->>TabManager: addWorkspace()
TabManager->>TabManager: assignGroup()
TabManager->>TabManager: placeWithinGroup(.afterCurrent, referenceId)
else configured action is builtIn(.cloudVM)
AppDelegate->>CloudVM: performCloudVMAction(onCompletion:)
CloudVM->>CloudVM: launch cmux --id-format uuids vm new
AppDelegate->>AppDelegate: onExecuted() snapshot beforeIds
AppDelegate->>Observer: install(tabManager, groupId, placement, referenceId)
Note over Observer: subscribes to tabManager.$tabs
alt workspace appears before process exits
TabManager-->>Observer: $tabs fires
Observer->>TabManager: addWorkspaceToGroup(placement, referenceId)
Observer->>Observer: dispose()
end
CloudVM-->>AppDelegate: terminationHandler onCompletion(Completion)
AppDelegate->>Observer: finishPending(observerId, workspaceId)
alt observer still alive and workspace in tabs
Observer->>TabManager: addWorkspaceToGroup(placement, referenceId)
Observer->>Observer: dispose()
end
else no configured action
AppDelegate->>TabManager: createWorkspaceInGroup(groupId, placement, referenceId)
end
Reviews (4): Last reviewed commit: "Fix group Cloud VM placement completion" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+6272
to
+6283
| case .afterCurrent: | ||
| if let referenceWorkspaceId, | ||
| referenceWorkspaceId != workspaceId, | ||
| let referenceIndex = tabs.firstIndex(where: { $0.id == referenceWorkspaceId && $0.groupId == groupId }) { | ||
| targetIndex = referenceIndex + 1 | ||
| } else if let anchorIndex = tabs.firstIndex(where: { $0.id == group.anchorWorkspaceId }) { | ||
| targetIndex = anchorIndex + 1 | ||
| } else if let firstMember = memberIndices.first { | ||
| targetIndex = firstMember | ||
| } else { | ||
| return | ||
| } |
Contributor
There was a problem hiding this comment.
afterCurrent fallback silently no-ops when group has no members and no anchor
When referenceWorkspaceId is nil (or not found in the group), group.anchorWorkspaceId is absent, and memberIndices is empty, the function returns early without placing the workspace at all. The workspace is left in whatever position addWorkspace inserted it, which can land it outside the group's contiguous block. The .top and .end cases have the same silent-return, so this is pre-existing behaviour, but the new default makes this path reachable more often. A log at the .logger level on the otherwise-dead return would make the failure visible in diagnostics.
![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
🤖 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 13278-13280: Rewrite the ambiguous sentence to explicitly state
the resolution order: indicate that placement is determined first from the
per-cwd cmux.json value named newWorkspacePlacement, and if that is not present
the global default is used; update the comment containing "Placement follows
per-cwd cmux.json `newWorkspacePlacement`, then the global default" to a clearer
form that lists the two steps in that order so readers can unambiguously find
`newWorkspacePlacement` in the per-directory cmux.json.
In `@cmuxTests/WorkspaceGroupTests.swift`:
- Around line 503-510: Replace the manual multiple `#expect` assertions in the
test function workspaceGroupNewPlacementParsesConfigSpellings with a
parameterized `@Test`(arguments:) data-driven test: define an array of
input/expected pairs for WorkspaceGroupNewPlacement(rawString:) (including
"afterCurrent"/"after-current"/"after_current" -> .afterCurrent, "top" -> .top,
"end" -> .end, "middle" -> nil) and convert the test to accept parameters and
assert the parsed result equals the expected value; this keeps the test concise
and lets you add more spellings easily while still exercising
WorkspaceGroupNewPlacement(rawString:).
In `@web/data/cmux.schema.json`:
- Around line 168-173: The schema property newWorkspacePlacement in
web/data/cmux.schema.json is missing a descriptionKey for i18n; add a
descriptionKey (e.g.,
"schemaDescriptions.workspaceGroups.newWorkspacePlacement") alongside the
existing description so page.tsx's t(property.descriptionKey) can resolve a
localized string, then add the matching entry under
"schemaDescriptions.workspaceGroups.newWorkspacePlacement" in
web/messages/en.json and all other locale files with translated text matching
the current description. Ensure the key name matches exactly between the JSON
schema property and the entries in web/messages/* so page.tsx (which reads
property.descriptionKey) can load localized descriptions.
- Around line 199-203: The schema property newWorkspacePlacement in
cmux.schema.json needs internationalization: add a descriptionKey (matching the
pattern used elsewhere, e.g., descriptionKey:
"schemaDescriptions.workspaceGroups.byCwd.newWorkspacePlacement") alongside the
existing description, and then add the corresponding entry to
web/messages/en.json (and all other locale files) under
schemaDescriptions.workspaceGroups.byCwd.newWorkspacePlacement with the full
user-facing string from the review; ensure translations are added for Japanese
and each supported locale to keep parity with other fields.
🪄 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: 32a338c5-72c3-4ac5-b5ac-00578e655f7c
📒 Files selected for processing (17)
CLI/cmux.swiftResources/Localizable.xcstringsSources/AppDelegate.swiftSources/CmuxConfig.swiftSources/CmuxSettingsJSONPathSupport.swiftSources/KeyboardShortcutSettingsFileStore+Template.swiftSources/KeyboardShortcutSettingsFileStore.swiftSources/SettingsNavigation.swiftSources/SettingsSearchAliases.swiftSources/TabManager.swiftSources/TerminalController.swiftSources/cmuxApp.swiftcmuxTests/WorkspaceGroupTests.swiftcmuxTests/WorkspaceUnitTests.swiftdocs/workspace-groups.mdweb/app/[locale]/docs/configuration/page.tsxweb/data/cmux.schema.json
![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 1b85d69. Configure here.
hhsw2015
pushed a commit
to hhsw2015/cmux
that referenced
this pull request
May 30, 2026
- Keep workspace group creation in place (manaflow-ai#4989) - Allow narrower sidebar with titlebar accessories (manaflow-ai#5013) - Add beta TextBox defaults settings (manaflow-ai#4773) - Add default terminal registration (manaflow-ai#4935) - Make group new workspace placement configurable (manaflow-ai#5018) - Fix nightly publishing runner selection (manaflow-ai#5022) Conflicts resolved: - ContentView: take upstream's SidebarWorkspaceTopDropIndicator extraction. - cmuxApp: keep fork's QuickTerminal/WorkspaceTopTabsVisibility @AppStorage + TerminalCopyOnSelectSettings; adopt upstream's Setting(\.terminal.*) for textBoxMaxLines + showTextBoxOnNewTerminals + focusTextBoxOnNewTerminals. - xcstrings: keep both fork's settings.app.workspaceTopTabsVisibility[.*] keys and upstream's settings.app.workspaceGroupNewWorkspacePlacement. - TabManagerUnitTests: drop fork's testNewSurfaceCreatesAndFocusesTopLevelTab (upstream renames + retains coverage via testNewSurfaceFocusesCreatedSurface).
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:
Checks:
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Changes default insert order for group workspaces and Cmd-N routing; users who relied on top placement need an explicit setting. Cloud VM group joining now depends on CLI UUID output rather than a timeout window.
Overview
Group new workspace placement is now configurable with
afterCurrent(default),top, andend, resolved per-cwd incmux.jsonthen from the globalworkspaceGroups.newWorkspacePlacementsetting (new Settings row andcmux.jsonparsing).Cmd-N and configured “new workspace” flows from a group member create inside that group using the active member as the insert reference; the group
+and CLInew-workspace --placementdocument the same options, withafterCurrentfalling back to top when there is no in-group reference.Async group actions (e.g. Cloud VM) pass placement and reference into group join logic; the watcher drops the fixed timeout and instead finishes when the VM process reports success and a
workspace=UUID fromcmux vm new --id-format uuids.Localization adds strings for group placement labels/descriptions and reformats several TextBox catalog entries in
Localizable.xcstrings.Reviewed by Cursor Bugbot for commit b1220c2. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Makes new workspace placement inside a group configurable and routes Cmd‑N from group anchors and members to create inside that group. The default is now After current; placement resolves from per‑cwd first, then global, and falls back to top when no in‑group reference exists.
New Features
workspaceGroups.newWorkspacePlacementwith Settings UI (“Group New Workspace Placement”); settable in globalcmux.jsonwith per‑cwd override.+and CLI use the anchor as the reference so afterCurrent behaves like top when no member is active.cmux workspace-group new-workspace <group> [--placement afterCurrent|top|end]with validation and the above resolution order (per‑cwd → global).Migration
"workspaceGroups": { "newWorkspacePlacement": "top" }incmux.json.--placementin scripts if you need a specific order.Written for commit b1220c2. Summary will update on new commits.
Summary by CodeRabbit
New Features
Behavior
Documentation
Localization
Tests