Repository navigation
Add new workspace above and below actions - #5644
lawrencecchen wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR adds "New Workspace Above" and "New Workspace Below" functionality across the application. It introduces core placement logic in TabManager via a new ChangesAdjacent Workspace Creation Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 (15 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}
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 4e0f350. Configure here.
| case .below: | ||
| _ = reorderWorkspace(tabId: workspaceId, after: referenceWorkspaceId) | ||
| } | ||
| } |
There was a problem hiding this comment.
Pinned adjacent insert misplaces workspace
Medium Severity
Adjacent workspace creation appends via addWorkspace, then reorderCreatedWorkspace calls reorderWorkspace with before/after the reference. That path uses clampedReorderIndex, which keeps unpinned tabs out of the pinned prefix (and applies grouped pin bounds). For a pinned reference, “above” or “below” can clamp to the pin-tier edge instead of the slot next to that workspace, while the action still succeeds.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 4e0f350. Configure here.
![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: 4e0f3501ad
ℹ️ 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".
| let cwd = tabs.first(where: { $0.id == group.anchorWorkspaceId })?.currentDirectory | ||
| let newWorkspace = addWorkspace( | ||
| workingDirectory: cwd, | ||
| inheritWorkingDirectory: cwd == nil, |
There was a problem hiding this comment.
When this action is invoked from a workspace row inside a group whose current directory differs from the group anchor, the new workspace is created with the anchor's currentDirectory because workingDirectory: cwd is passed explicitly; that prevents sourceWorkspaceOverride: referenceWorkspace from supplying the clicked workspace's directory. The ungrouped adjacent path inherits from the reference workspace, so grouped rows behave inconsistently and “New Workspace Above/Below” can open in the wrong folder for mixed-directory groups.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR adds "New Workspace Above" and "New Workspace Below" actions wired through a new
Confidence Score: 3/5Safe to merge for English and Japanese users; all other supported locales will display raw English fallback strings for the four new menu and palette entries until translations are added. The insertion logic itself is well-structured and the three new tests cover the core branching paths. The only outstanding defect is in Localizable.xcstrings: four new string keys ship translations for only 2 of the 20 locales in the catalog, leaving ar, bs, da, de, es, fr, it, km, ko, nb, pl, pt-BR, ru, th, tr, uk, zh-Hans, and zh-Hant without localized text for the new actions. Resources/Localizable.xcstrings needs translation entries for the 18 missing locales before shipping to non-English/non-Japanese users. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User triggers action] --> B{Entry point}
B --> C[Command palette]
B --> D[TabItemView context menu]
B --> E[GroupHeader context menu]
C --> F[selectedTabId]
D --> G[tab.id]
E --> H[anchorWorkspaceId]
F --> I[createWorkspaceAdjacent]
G --> I
H --> I
I --> J{In a group?}
J -- No --> K[addWorkspace .end + reorderCreatedWorkspace]
J -- Yes, is anchor --> L[createWorkspaceAdjacentToGroup — outside group]
J -- Yes, non-anchor --> M[createWorkspaceAdjacentWithinGroup — inside group]
K --> N[reorderWorkspace: normalizeContiguity + postOrderChanged]
L --> N
M --> O[assignGroup + reorderCreatedWorkspace + expandGroupIfNeeded]
O --> N
Reviews (1): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
| }, | ||
| "command.newWorkspaceAbove.title": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "New Workspace Above" | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "上に新規ワークスペース" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "command.newWorkspaceBelow.title": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "New Workspace Below" | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "下に新規ワークスペース" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "command.nextTabInPane.subtitle": { | ||
| "extractionState": "manual", | ||
| "localizations": { |
There was a problem hiding this comment.
The catalog already contains 20 locales — ar, bs, da, de, es, fr, it, km, ko, nb, pl, pt-BR, ru, th, tr, uk, zh-Hans, and zh-Hant in addition to en and ja — but all four new keys (command.newWorkspaceAbove.title, command.newWorkspaceBelow.title, contextMenu.newWorkspaceAbove, contextMenu.newWorkspaceBelow) only supply en and ja entries. Every other supported locale will fall back to the en default value at runtime, leaving users of the other 18 locales with untranslated strings.
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!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@Sources/TabManager.swift`:
- Around line 4309-4318: When creating then immediately reordering a new
workspace (via addWorkspace and reorderCreatedWorkspace), the new workspace is
always unpinned so reorderWorkspace's pin-tier clamping can place it
non-adjacent to a pinned reference; modify the flow so the new workspace
inherits the reference workspace's pin tier (or pass an explicit
pinTier/placementHint into addWorkspace or reorderCreatedWorkspace) before
calling reorderWorkspace/reorderCreatedWorkspace so pin-tier clamping uses the
correct tier; update calls around addWorkspace/select: select,
placementOverride: .end, sourceWorkspaceOverride: referenceWorkspace and the
subsequent reorderCreatedWorkspace(newWorkspace.id, adjacentTo:
referenceWorkspaceId, position: position) (and the other similar blocks at the
noted ranges) to preserve the referenceWorkspace.pin state when reordering.
🪄 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: 51143161-ec4a-457e-a50d-28fa14fe3451
📒 Files selected for processing (6)
Resources/Localizable.xcstringsSources/ContentView.swiftSources/SidebarWorkspaceGroupHeaderView.swiftSources/TabManager.swiftSources/VerticalTabsSidebar+WorkspaceGroups.swiftcmuxTests/WorkspaceGroupTests.swift
| let newWorkspace = addWorkspace( | ||
| select: select, | ||
| placementOverride: .end, | ||
| sourceWorkspaceOverride: referenceWorkspace | ||
| ) | ||
| reorderCreatedWorkspace( | ||
| newWorkspace.id, | ||
| adjacentTo: referenceWorkspaceId, | ||
| position: position | ||
| ) |
There was a problem hiding this comment.
Preserve pin tier for adjacent insertion targets.
Line 4314 / Line 4339 / Line 4366 reorder a newly created workspace that is always unpinned. Because reorderWorkspace clamps by pin tier, “above/below” against pinned references (or pinned groups/pinned group members) can land non-adjacent.
Suggested fix
@@
func createWorkspaceAdjacent(
to referenceWorkspaceId: UUID,
position: WorkspaceAdjacentInsertionPosition,
select: Bool = true
) -> Workspace? {
@@
let newWorkspace = addWorkspace(
select: select,
placementOverride: .end,
sourceWorkspaceOverride: referenceWorkspace
)
+ newWorkspace.isPinned = referenceWorkspace.isPinned
reorderCreatedWorkspace(
newWorkspace.id,
adjacentTo: referenceWorkspaceId,
position: position
)
return newWorkspace
}
@@
private func createWorkspaceAdjacentWithinGroup(
to referenceWorkspace: Workspace,
groupId: UUID,
position: WorkspaceAdjacentInsertionPosition,
select: Bool
@@
let newWorkspace = addWorkspace(
workingDirectory: cwd,
inheritWorkingDirectory: cwd == nil,
select: select,
placementOverride: .end,
sourceWorkspaceOverride: referenceWorkspace,
autoWelcomeIfNeeded: false
)
+ newWorkspace.isPinned = referenceWorkspace.isPinned
assignGroup(workspaceId: newWorkspace.id, groupId: groupId)
reorderCreatedWorkspace(
newWorkspace.id,
adjacentTo: referenceWorkspace.id,
position: position
)
@@
private func createWorkspaceAdjacentToGroup(
groupId: UUID,
position: WorkspaceAdjacentInsertionPosition,
select: Bool
) -> Workspace? {
@@
let newWorkspace = addWorkspace(
select: select,
placementOverride: .end,
sourceWorkspaceOverride: anchorWorkspace
)
+ newWorkspace.isPinned = group.isPinned
let groupMemberIds = tabs.filter { $0.groupId == groupId && $0.id != newWorkspace.id }.map(\.id)
switch position {Also applies to: 4330-4343, 4357-4371
🤖 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 4309 - 4318, When creating then
immediately reordering a new workspace (via addWorkspace and
reorderCreatedWorkspace), the new workspace is always unpinned so
reorderWorkspace's pin-tier clamping can place it non-adjacent to a pinned
reference; modify the flow so the new workspace inherits the reference
workspace's pin tier (or pass an explicit pinTier/placementHint into
addWorkspace or reorderCreatedWorkspace) before calling
reorderWorkspace/reorderCreatedWorkspace so pin-tier clamping uses the correct
tier; update calls around addWorkspace/select: select, placementOverride: .end,
sourceWorkspaceOverride: referenceWorkspace and the subsequent
reorderCreatedWorkspace(newWorkspace.id, adjacentTo: referenceWorkspaceId,
position: position) (and the other similar blocks at the noted ranges) to
preserve the referenceWorkspace.pin state when reordering.
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Summary
Testing
node -e 'JSON.parse(require("fs").readFileSync("Resources/Localizable.xcstrings","utf8")); console.log("Localizable.xcstrings JSON OK")'git diff --check./scripts/reload-cloud.sh --tag wspabvxcodebuild test -project cmux.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS' -derivedDataPath /tmp/cmux-workspace-position-actions-rerun -only-testing:cmuxTests/WorkspaceGroupTestsbuilt and ran the suite locally. The three new adjacent-insertion tests passed. The run still failed existing WorkspaceGroupTests expectations unrelated to these new actions, including pinned/group reorder cases.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Touches workspace ordering, group membership, and
addWorkspaceinheritance paths; mistakes could mis-order tabs or break group contiguity, though behavior is covered by new unit tests.Overview
Adds New Workspace Above and New Workspace Below so users can insert a workspace next to the current one instead of only using generic “new workspace” placement.
Command palette registers two new commands (with keywords above/below) that require a selected workspace and call
TabManager.createWorkspaceAdjacent. Workspace row and workspace group header context menus get the same two actions; group headers wire throughonNewWorkspaceAbove/onNewWorkspaceBelowinVerticalTabsSidebar+WorkspaceGroups.TabManagerintroducesWorkspaceAdjacentInsertionPositionandcreateWorkspaceAdjacent(to:position:), plussourceWorkspaceOverrideonaddWorkspaceso inheritance follows the reference row, not only the current selection. Ungrouped tabs insert via create-then-reorder; grouped members stay in the group and expand collapsed groups when selecting; acting on the group anchor inserts outside the whole group (before first / after last member). English and Japanese strings cover palette and context menu labels.Tests:
WorkspaceGroupTestsis marked.serializedand adds three tests for ungrouped, in-group, and group-header adjacent insertion.Reviewed by Cursor Bugbot for commit 4e0f350. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Add actions to create a new workspace above or below the current selection. Uses a shared
TabManageradjacent-insertion API to place new workspaces inside groups or outside the group when triggered from a group header.palette.newWorkspaceAboveandpalette.newWorkspaceBelowwith localized titles.TabManager.createWorkspaceAdjacent(to:position:)ensures:Written for commit 4e0f350. Summary will update on new commits.
Summary by CodeRabbit