Repository navigation
Keep pinned workspaces inside groups - #5541
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAllow pinned workspaces as group members and update related logic: localization, UI eligibility, ContentView→SidebarDropPlanner plumbing, TabManager reorder/pin and drag-inference, TerminalController validation, and tests. ChangesPinned Workspaces in Groups
Sequence DiagramsequenceDiagram
participant User
participant ContentView
participant SidebarDropPlanner
participant TabManager
User->>ContentView: drag tab (draggedId, targetId)
ContentView->>TabManager: sidebarReorderLegalInsertionRange(draggedId,targetId,usesTopLevelRows)
ContentView->>SidebarDropPlanner: indicator(..., legalInsertionRange)
SidebarDropPlanner->>TabManager: compute target index / clamped insertion position
TabManager->>TabManager: reorder/pin inference, apply group membership
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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, 1 inconclusive)
✅ Passed checks (14 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 |
|
@codex review |
![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: 2c6e4836d4
ℹ️ 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 tab.groupId != nil { | ||
| normalizeWorkspaceGroupContiguity() | ||
| return |
There was a problem hiding this comment.
When this branch keeps a pinned workspace inside its group, the grouped child still has isPinned == true, but batchWorkspaceReorderFinalIds still buckets every isPinned workspace into the global pinned segment before normalizing group runs. In a state like A, group(anchor, pinned child), C, a workspace.reorder_many / CLI reorder of just C will first hoist the pinned child, then normalization emits the whole unpinned group before C, so an unrelated group jumps ahead of the requested item. The batch reorder path needs to treat grouped member pins like local group ordering, not global pinned rows.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR keeps pinned workspaces inside their workspace group instead of ejecting them, normalizes group member order to anchor → pinned children → unpinned children, and gates group-creation/membership eligibility on anchor status alone (not pin status).
Confidence Score: 5/5Safe to merge; the new group-local pin semantics are well-isolated and covered by regression tests. All eligibility checks, reorder paths, drag-clamp logic, and normalisation are internally consistent. The index alignment between sidebarReorderLegalInsertionRange (tabs indices) and SidebarDropPlanner (tabIds) is guaranteed because sidebarReorderWorkspaceIds returns tabs.map(.id) in the non-top-level-rows mode that the range function also requires. The three new tests cover the main new invariants. The only open items — missing locale translations for two new strings and the per-workspace normalise passes in batch-pin — were flagged in previous review rounds. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[setPinned called] --> B{tab.groupId != nil?}
B -- Yes --> C[normalizeWorkspaceGroupContiguity]
C --> D[anchorFirst: anchor → pinned members → unpinned members]
B -- No --> E[reorderTabForPinnedState ungrouped path]
E --> F[leadingGlobalPinnedRowCount]
F --> G{isGlobalPinnedRow per tab}
G -- groupId set --> H[return group.isPinned]
G -- no groupId --> I[return tab.isPinned]
H & I --> J[insert at boundary]
subgraph Drag Drop Clamping
K[sidebarReorderLegalInsertionRange] --> L{dragged member is pinned?}
L -- Yes --> M[range: firstIndex+1 .. firstIndex+1+pinnedCount]
L -- No --> N[range: firstIndex+1+pinnedCount .. lastIndex+1]
M & N --> O[SidebarDropPlanner clamps insertion to range]
end
subgraph Eligibility
P[createWorkspaceGroup / addWorkspaceToGroup / shortcuts] --> Q{workspace is anchor of another group?}
Q -- Yes --> R[reject / error]
Q -- No --> S[allow, regardless of isPinned]
end
Reviews (3): Last reviewed commit: "docs: update grouped drag pin comment" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| "workspaceGroup.error.allChildrenAreAnchors": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "All requested children are ineligible because they are already group anchors; ungroup them first" | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "要求された子ワークスペースはすべて既にグループのアンカーのため対象外です。先にグループ解除してください" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "workspaceGroup.error.workspaceIsOtherGroupAnchor": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Workspace is the anchor of another group; ungroup it first" | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "このワークスペースは別のグループのアンカーです。先にグループ解除してください" | ||
| } | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
Both workspaceGroup.error.allChildrenAreAnchors and workspaceGroup.error.workspaceIsOtherGroupAnchor only ship en and ja translations, but the catalog already supplies zh-Hans, zh-Hant, ko, de, es, fr, it, da, nb, pl, ru, pt-BR, ar, bs, th, tr, uk, and km for comparable workspace-group strings (e.g. contextMenu.workspaceGroup.moveTo carries all of those locales). Users running cmux in any of those other locales will receive the raw English text from the socket/CLI API instead of a localized message.
Rule Used: Flag production user-facing text that is not fully... (source)
| if !workspaceGroups.isEmpty { | ||
| for id in changedIds { | ||
| if let workspace = workspacesById[id] { | ||
| reorderTabForPinnedState(workspace) | ||
| } | ||
| } | ||
| postWorkspaceOrderDidChange(movedWorkspaceIds: changedIds) | ||
| return changedIds | ||
| } |
There was a problem hiding this comment.
normalizeWorkspaceGroupContiguity once per changed workspace
When workspaceGroups is non-empty, setPinnedWorkspaces now calls reorderTabForPinnedState in a loop; for any grouped member, that resolves to normalizeWorkspaceGroupContiguity(), which rebuilds the full tabs order. Pinning N grouped workspaces in one call therefore runs O(N) full-collection rebuild passes instead of the single pass the old code used. For typical usage this is imperceptible, but bulk socket/CLI commands that pin many grouped members at once will see linear slowdown relative to the batch size.
Rule Used: Flag production code that adds nested full-collect... (source)
2c6e483 to
e70ee31
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?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: e70ee31529
ℹ️ 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 Set(tabs.filter(\.isPinned).map(\.id)) | ||
| return Set(tabs.filter { $0.groupId == nil && $0.isPinned }.map(\.id)) | ||
| } | ||
| return sidebarTopLevelPinnedWorkspaceIds() |
There was a problem hiding this comment.
When a pinned child of an unpinned group is dragged over a group header, usesTopLevelRows promotes that child into the top-level reorder list, but this still returns only the already-visible top-level pins. Because the promoted child is still grouped at this point, it is omitted from the pin set, so the planner and reorderTopLevelWorkspaceItem can accept a bottom/unpinned target; after assignGroup(..., nil) runs, the workspace is now a standalone pinned row below unpinned rows, breaking the pin boundary. Include the promoted dragged workspace when it is pinned before computing the top-level pinned ids.
Useful? React with 👍 / 👎.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/TabManager.swift (1)
3818-3825: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUpdate the drag-inference contract comment.
The comment still describes the old "pinned workspaces never gain a group via drag" rule, but this implementation now intentionally allows pinned members inside groups.
🤖 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 3818 - 3825, Update the documentation comment that begins "After a drag-driven reorder, infer the dragged workspace's group membership..." to reflect the new behavior where pinned workspaces may be placed inside groups via drag; remove or revise the sentence "Pinned workspaces never gain a group via drag" and instead state that pinned members can be added to groups when the drag-inference rules determine they belong inside a group's section, keeping the rest of the neighbor-based rules unchanged so readers can locate this logic in TabManager.swift around the drag-inference implementation.
🤖 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 3715-3751: The grouped-member bounds and pinned-tier calculations
in sidebarReorderLegalInsertionRange are duplicated across the pinned and
unpinned branches, so extract a helper (e.g.,
computeGroupMemberBounds(forGroupId:draggedWorkspaceId:)) that returns
(firstIndex: Int, lastIndex: Int, pinnedMemberCount: Int) by scanning tabs and
workspaceGroups using groupId, memberIndices and pinned checks; then use that
single helper to compute lower/upper in both the draggedWorkspace.isPinned
branch and the unpinned branch, and call the same helper from the other
duplicate code path referenced (lines ~5018-5044) so both reorderSidebar logic
and reorderWorkspace use identical bounds logic.
---
Outside diff comments:
In `@Sources/TabManager.swift`:
- Around line 3818-3825: Update the documentation comment that begins "After a
drag-driven reorder, infer the dragged workspace's group membership..." to
reflect the new behavior where pinned workspaces may be placed inside groups via
drag; remove or revise the sentence "Pinned workspaces never gain a group via
drag" and instead state that pinned members can be added to groups when the
drag-inference rules determine they belong inside a group's section, keeping the
rest of the neighbor-based rules unchanged so readers can locate this logic in
TabManager.swift around the drag-inference implementation.
🪄 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: 2103b96d-0ffc-439a-a263-97200a24d06c
📒 Files selected for processing (8)
Resources/Localizable.xcstringsSources/AppDelegate.swiftSources/ContentView.swiftSources/Sidebar/SidebarDropPlanner.swiftSources/TabItemView+WorkspaceGroups.swiftSources/TabManager.swiftSources/TerminalController.swiftcmuxTests/WorkspaceGroupTests.swift
| func sidebarReorderLegalInsertionRange( | ||
| forDraggedWorkspaceId draggedWorkspaceId: UUID?, | ||
| targetWorkspaceId: UUID? = nil, | ||
| usesTopLevelRows: Bool = false | ||
| ) -> ClosedRange<Int>? { | ||
| guard !usesTopLevelRows, | ||
| !sidebarReorderUsesTopLevelRows( | ||
| forDraggedWorkspaceId: draggedWorkspaceId, | ||
| targetWorkspaceId: targetWorkspaceId | ||
| ), | ||
| let draggedWorkspaceId, | ||
| let draggedWorkspace = tabs.first(where: { $0.id == draggedWorkspaceId }), | ||
| let groupId = draggedWorkspace.groupId, | ||
| let group = workspaceGroups.first(where: { $0.id == groupId }), | ||
| draggedWorkspace.id != group.anchorWorkspaceId else { | ||
| return nil | ||
| } | ||
| let memberIndices = tabs.indices.filter { tabs[$0].groupId == groupId } | ||
| guard let firstIndex = memberIndices.first, | ||
| let lastIndex = memberIndices.last else { | ||
| return nil | ||
| } | ||
| let pinnedMemberCount = memberIndices.reduce(into: 0) { count, index in | ||
| let member = tabs[index] | ||
| if member.id != group.anchorWorkspaceId, member.isPinned { | ||
| count += 1 | ||
| } | ||
| } | ||
| if draggedWorkspace.isPinned { | ||
| let lower = min(firstIndex + 1, tabs.count) | ||
| let upper = min(firstIndex + 1 + pinnedMemberCount, tabs.count) | ||
| return lower...max(lower, upper) | ||
| } | ||
| let lower = min(firstIndex + 1 + pinnedMemberCount, tabs.count) | ||
| let upper = min(lastIndex + 1, tabs.count) | ||
| return min(lower, upper)...max(lower, upper) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Extract the grouped-member bounds calculation into one helper.
These two paths now carry the same group scan and pinned-tier math. If one changes without the other, the sidebar will advertise a legal drop that reorderWorkspace cannot actually honor.
Also applies to: 5018-5044
🤖 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 3715 - 3751, The grouped-member bounds
and pinned-tier calculations in sidebarReorderLegalInsertionRange are duplicated
across the pinned and unpinned branches, so extract a helper (e.g.,
computeGroupMemberBounds(forGroupId:draggedWorkspaceId:)) that returns
(firstIndex: Int, lastIndex: Int, pinnedMemberCount: Int) by scanning tabs and
workspaceGroups using groupId, memberIndices and pinned checks; then use that
single helper to compute lower/upper in both the draggedWorkspace.isPinned
branch and the unpinned branch, and call the same helper from the other
duplicate code path referenced (lines ~5018-5044) so both reorderSidebar logic
and reorderWorkspace use identical bounds logic.
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 c079115. Configure here.
| } | ||
| tabs.remove(at: index) | ||
| let pinnedCount = tabs.filter { $0.isPinned }.count | ||
| let pinnedCount = leadingGlobalPinnedRowCount() |
There was a problem hiding this comment.
Pin drops group normalize call
Medium Severity
Pinning or unpinning an ungrouped workspace no longer runs normalizeWorkspaceGroupContiguity when any workspace groups exist. reorderTabForPinnedState only inserts by leadingGlobalPinnedRowCount, which can place that workspace inside a group’s tabs[] run and break anchor-first contiguity until some later operation normalizes.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit c079115. 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 (1)
Sources/TabManager.swift (1)
3818-3826:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign the summary bullets with the actual membership inference logic.
At Line 3821-Line 3826, the comment says a single grouped neighbor can cause a join and “otherwise” the
groupIdis cleared, but the implementation preservescurrentGroupwhen neighbors differ (Line 3863-Line 3867). Please keep the header bullets consistent with the real branch behavior.Suggested doc-only fix
- /// - If only one neighbor is in a group, join that neighbor's group when - /// that group's anchor is the neighbor or another existing member - /// (i.e. the dragged workspace sits "inside" the section). - /// - Otherwise, clear groupId. + /// - If both neighbors resolve to the same `groupId` (including `nil`), + /// adopt that membership. + /// - If neighbors differ, keep the current membership (ambiguous edge drop).🤖 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 3818 - 3826, Update the header comment for the "After a drag-driven reorder" membership inference to match the implementation: state that when both neighbors share a non-nil groupId the dragged workspace joins that group, when only one neighbor is in a group it may join that neighbor's group under the existing "anchor/inside section" condition, and in the case where neighbors differ the code preserves the existing currentGroup (does not clear groupId). Reference the variables/logic named currentGroup and the neighbor-based branch that decides joining vs preserving so the bullets mirror the actual behavior; leave the pinned-workspaces note as-is.
🤖 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/TabManager.swift`:
- Around line 3818-3826: Update the header comment for the "After a drag-driven
reorder" membership inference to match the implementation: state that when both
neighbors share a non-nil groupId the dragged workspace joins that group, when
only one neighbor is in a group it may join that neighbor's group under the
existing "anchor/inside section" condition, and in the case where neighbors
differ the code preserves the existing currentGroup (does not clear groupId).
Reference the variables/logic named currentGroup and the neighbor-based branch
that decides joining vs preserving so the bullets mirror the actual behavior;
leave the pinned-workspaces note as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 585427a2-0be8-491a-8dcf-063c7cdddd46
📒 Files selected for processing (1)
Sources/TabManager.swift
This branch was successfully deployed
1 participant
Summary
Tests
Dogfood
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Touches core TabManager ordering, pinning, and sidebar drag logic plus socket group APIs; behavior changes are broad but covered by new regression tests.
Overview
Pinned workspaces can remain group members instead of being ejected or blocking group actions. Pinning a grouped workspace reorders it to the pinned tier inside that group (anchor first, then pinned children, then unpinned), and drag/reorder paths clamp insertion with
sidebarReorderLegalInsertionRangeso members don’t cross anchor/pinned/unpinned boundaries—invalid drops suppress the indicator.Group eligibility is aligned across the ⌘⇧G shortcut, row context menu, and socket/CLI: only existing group anchors are excluded (pinned is allowed). Socket failures use new localized strings when all explicit children are anchors or when adding a workspace that anchors another group.
Top-level sidebar pinning now treats only ungrouped pinned rows (and group-level pin state via
isGlobalPinnedRow) as the global pinned segment; pinned members inside a group no longer count toward that leading block.Reviewed by Cursor Bugbot for commit c079115. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Pinned workspaces now stay inside their workspace group and reorder within it. Drag and drop is clamped so members can’t cross anchor/pinned/unpinned boundaries; top-level pinned rows ignore pinned group members.
New Features
Bug Fixes
Written for commit c079115. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Documentation / Localization
Tests