Repository navigation
Add nested workspace folders - #6611
Closed
austinywang wants to merge 54 commits into
Closed
austinywang wants to merge 54 commits into
austinywang wants to merge 54 commits into
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR adds nested workspace-group parent metadata, updates ordering and persistence to preserve parent chains, rewrites sidebar rendering and reorder behavior for hierarchical trees, and propagates the new structure through control, mobile, and test surfaces. ChangesNested Workspace Groups
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (22 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 |
Contributor
Greptile SummaryThis PR generalizes workspace groups into an arbitrary-depth parent-pointer folder tree.
Confidence Score: 5/5Safe to merge. The nested folder tree is a large, well-scoped model change; cycle prevention, restore resilience, and subtree-aware actions are consistently applied across all relevant paths. No correctness bugs found after tracing the DFS rendering path, the iterative normalization rewrite, the dual-redundant snapshot restore, crash-pruning parent re-link, and all four cycle-detection sites. The previously-flagged O(G²) normalization hot path is confirmed fixed. Only minor style observations filed. The Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant User
participant SidebarDragDelegate
participant DropResolver
participant WorkspaceGroupCoordinator
participant WorkspacesModel
User->>SidebarDragDelegate: center-drop group A header onto group B header
SidebarDragDelegate->>DropResolver: resolve(request, context)
DropResolver->>DropResolver: groupReparentPlan() isCenterGroupHeaderDrop and canReparent check
DropResolver-->>SidebarDragDelegate: "plan(.reparentGroup(A, B), indicator=nil, scope=.group(B))"
SidebarDragDelegate->>WorkspaceGroupCoordinator: setWorkspaceGroupParent(A, B)
WorkspaceGroupCoordinator->>WorkspacesModel: canSetWorkspaceGroupParent(A, B)
WorkspacesModel-->>WorkspaceGroupCoordinator: true
WorkspaceGroupCoordinator->>WorkspacesModel: "workspaceGroups[A].parentGroupId = B"
WorkspaceGroupCoordinator->>WorkspacesModel: normalizeWorkspaceGroupContiguity()
WorkspaceGroupCoordinator->>WorkspaceGroupCoordinator: workspaceGroupSubtreeWorkspaceIds(A)
WorkspaceGroupCoordinator-->>SidebarDragDelegate: workspaceOrderDidChange(subtreeIds)
Note over User,WorkspacesModel: Sidebar re-renders A nested under B at depth+1
%%{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 User
participant SidebarDragDelegate
participant DropResolver
participant WorkspaceGroupCoordinator
participant WorkspacesModel
User->>SidebarDragDelegate: center-drop group A header onto group B header
SidebarDragDelegate->>DropResolver: resolve(request, context)
DropResolver->>DropResolver: groupReparentPlan() isCenterGroupHeaderDrop and canReparent check
DropResolver-->>SidebarDragDelegate: "plan(.reparentGroup(A, B), indicator=nil, scope=.group(B))"
SidebarDragDelegate->>WorkspaceGroupCoordinator: setWorkspaceGroupParent(A, B)
WorkspaceGroupCoordinator->>WorkspacesModel: canSetWorkspaceGroupParent(A, B)
WorkspacesModel-->>WorkspaceGroupCoordinator: true
WorkspaceGroupCoordinator->>WorkspacesModel: "workspaceGroups[A].parentGroupId = B"
WorkspaceGroupCoordinator->>WorkspacesModel: normalizeWorkspaceGroupContiguity()
WorkspaceGroupCoordinator->>WorkspaceGroupCoordinator: workspaceGroupSubtreeWorkspaceIds(A)
WorkspaceGroupCoordinator-->>SidebarDragDelegate: workspaceOrderDidChange(subtreeIds)
Note over User,WorkspacesModel: Sidebar re-renders A nested under B at depth+1
Reviews (20): Last reviewed commit: "Ignore stale group ids for parent infere..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Packages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Coordinators/WorkspaceGroupCoordinator.swift (1)
557-570:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix the stale
groupsByAnchorIdreference.Line 557 now declares
rootGroupsByAnchorId, but Line 570 still readsgroupsByAnchorId, which is undefined in this scope and will fail compilation.Proposed fix
- let isPinned = groupsByAnchorId[id]?.isPinned ?? (tabsById[id]?.isPinned == true) + let isPinned = rootGroupsByAnchorId[id]?.isPinned ?? (tabsById[id]?.isPinned == 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 `@Packages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Coordinators/WorkspaceGroupCoordinator.swift` around lines 557 - 570, The variable created at the beginning is named rootGroupsByAnchorId, but the code on line 570 references an undefined variable groupsByAnchorId. In the line where isPinned is calculated within the for loop that iterates over topLevelIds, replace the reference to groupsByAnchorId with rootGroupsByAnchorId to match the variable name that was actually declared.Packages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Coordinators/WorkspaceReorderCoordinator.swift (1)
249-294:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPublish subtree moved IDs for root-folder sidebar reorders.
Line 249 sends root folder anchors through
reorderTopLevelWorkspaceItem, but Lines 288-290 still publish only tabs whosegroupId == group.id. When a root folder contains nested folders, the reorder moves the whole subtree while downstream consumers receive an incompletemovedWorkspaceIdspayload.Proposed fix
let movedWorkspaceIds: [UUID] if let group = model.workspaceGroups.first(where: { $0.anchorWorkspaceId == tabId }) { - movedWorkspaceIds = model.tabs.filter { $0.groupId == group.id }.map(\.id) + movedWorkspaceIds = model.workspaceGroupSubtreeWorkspaceIds(groupId: group.id) } else { movedWorkspaceIds = [tabId] }🤖 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 `@Packages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Coordinators/WorkspaceReorderCoordinator.swift` around lines 249 - 294, The movedWorkspaceIds calculation in reorderTopLevelWorkspaceItem does not include all descendants when a root folder anchor with nested folders is reordered. The current logic at lines 288-290 filters only tabs whose groupId matches the direct group, but when a root folder anchor is moved, the entire subtree of descendants (including nested folders and their children) should be included in the movedWorkspaceIds published to the host. Update the movedWorkspaceIds calculation to recursively collect all descendant workspace IDs when the tabId is an anchor with a group, ensuring the complete moved subtree is communicated via the host?.workspaceOrderDidChange call.
🤖 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/CmuxWorkspaces/Sources/CmuxWorkspaces/Coordinators/WorkspaceGroupCoordinator.swift`:
- Around line 51-53: The resolvedParentGroupId calculation using flatMap
silently converts an invalid parentGroupId to nil instead of rejecting it,
violating the documented contract that requires either an existing folder or nil
root. Replace the current flatMap logic in the resolvedParentGroupId assignment
with explicit validation that throws an error or returns early when
parentGroupId is provided but does not exist in model.workspaceGroups, ensuring
invalid parent references are rejected rather than silently treated as root
folder requests.
In
`@Packages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Model/WorkspacesModel`+GroupInvariants.swift:
- Around line 118-122: The while loop that walks ancestors in the selection
didSet path rescans workspaceGroups on each iteration using firstIndex(where:),
which becomes inefficient for deep trees. Create a dictionary mapping group ids
to their indices in workspaceGroups before the while loop starts (before cursor
is initialized), then replace the firstIndex(where:) call inside the loop with a
simple dictionary lookup using the current UUID. This ensures the ancestor walk
remains linear time regardless of tree depth.
- Around line 45-53: The appendGroupSubtree function uses recursion to traverse
nested workspace groups, which can cause stack overflow with arbitrary-depth
folder hierarchies. Convert this to an iterative approach by introducing an
explicit stack variable to manage the traversal instead of using recursive
calls. Initialize the stack with the initial group parameter, then loop while
the stack is not empty, processing one group at a time. When you encounter child
groups via childGroupsByParentId, push them onto the stack in reverse order (so
they are processed in the original sibling order when popped from the stack).
Keep all the existing logic for tracking emitted group and workspace IDs and
appending members in the same order.
- Around line 92-95: The loop iterating over workspaceGroups.indices repeatedly
calls canSetWorkspaceGroupParent, which rebuilds internal lookup structures on
each iteration, creating O(groups²) complexity. Build a groupsById map once
before the loop, add an overload of canSetWorkspaceGroupParent that accepts this
map as a parameter, and update the loop to call the new overload instead of the
original. Update the groupsById map when a parent is cleared during
normalization to keep the map current for subsequent validation calls.
In
`@Packages/macOS/CmuxWorkspaces/Tests/CmuxWorkspacesTests/WorkspaceCoordinatorTests.swift`:
- Around line 327-347: The test function reparentWorkspaceGroupRejectsCycles()
and the other related test functions mentioned in the comment contain force-try
operators in the try! `#require` calls, which triggers SwiftLint errors. To fix
this, convert each test function to a throwing function by adding throws to the
function signature (for example, change func
reparentWorkspaceGroupRejectsCycles() to func
reparentWorkspaceGroupRejectsCycles() throws), then replace all instances of
try! `#require` with try `#require` throughout these test methods. Apply this change
to reparentWorkspaceGroupRejectsCycles() and all other test functions mentioned
in the also applies to comment.
In `@Sources/SessionPersistencePolicy`+CrashStorage.swift:
- Around line 295-313: The survivingParentId function currently only walks
ancestors using parentGroupId, but it should also respect the parentGroupIndex
fallback that restoreSessionSnapshot uses to reconstruct parent relationships.
Modify the survivingParentId function to check parentGroupIndex first (to find
parents the same way restore does), and only use parentGroupId as a fallback.
This prevents crash pruning from flattening folder hierarchies that depend on
the index-based parent resolution before restoreSessionSnapshot can reconstruct
them properly.
In `@Sources/SidebarWorkspaceRenderItem.swift`:
- Around line 123-139: The appendChildren function performs a full scan of the
tabs collection for each parent group, causing O(n²) complexity in nested trees.
Precompute a dictionary once that groups tabs by their normalized parent group
id using normalizedParentGroupId(for:), storing the result in a property similar
to how anchorGroupByWorkspaceId is structured. Then modify appendChildren to
directly access the precomputed bucket for the given parentGroupId instead of
looping through all tabs each time, eliminating the redundant scans over the
entire collection.
In `@Sources/TabManager.swift`:
- Around line 1873-1888: The commonWorkspaceGroupId method rescans the entire
tabs collection for each workspace ID in the input array, creating O(n*m)
complexity. Build a dictionary once at the start of the method that maps
workspace IDs to their corresponding tab objects using tabs.reduce or a similar
approach, then use this dictionary for constant-time lookups within the loop
instead of calling tabs.first(where:) repeatedly for each workspaceId.
- Around line 6096-6114: The parentByGroupId dictionary is being rebuilt on
every iteration inside the loop over restored.indices, causing O(groups²)
complexity. Move the initialization of parentByGroupId outside and before the
loop that processes each group, then pass it as a parameter to the
wouldCreateParentCycle function instead of rebuilding it internally. Update the
parentByGroupId dictionary with the new parent assignment only when the cycle
check passes (inside the if block where restored[index].parentGroupId is being
set), so the dictionary reflects the currently accepted state.
---
Outside diff comments:
In
`@Packages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Coordinators/WorkspaceGroupCoordinator.swift`:
- Around line 557-570: The variable created at the beginning is named
rootGroupsByAnchorId, but the code on line 570 references an undefined variable
groupsByAnchorId. In the line where isPinned is calculated within the for loop
that iterates over topLevelIds, replace the reference to groupsByAnchorId with
rootGroupsByAnchorId to match the variable name that was actually declared.
In
`@Packages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Coordinators/WorkspaceReorderCoordinator.swift`:
- Around line 249-294: The movedWorkspaceIds calculation in
reorderTopLevelWorkspaceItem does not include all descendants when a root folder
anchor with nested folders is reordered. The current logic at lines 288-290
filters only tabs whose groupId matches the direct group, but when a root folder
anchor is moved, the entire subtree of descendants (including nested folders and
their children) should be included in the movedWorkspaceIds published to the
host. Update the movedWorkspaceIds calculation to recursively collect all
descendant workspace IDs when the tabId is an anchor with a group, ensuring the
complete moved subtree is communicated via the host?.workspaceOrderDidChange
call.
🪄 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: 61ec573c-f0df-447d-8aa6-355b27c44835
📒 Files selected for processing (25)
Packages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/WorkspaceGroup/ControlCommandCoordinator+WorkspaceGroup.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/WorkspaceGroup/ControlWorkspaceGroupSnapshot.swiftPackages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Coordinators/WorkspaceGroupCoordinator.swiftPackages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Coordinators/WorkspaceReorderCoordinator.swiftPackages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Model/WorkspacesModel+GroupInvariants.swiftPackages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Model/WorkspacesModel+Ordering.swiftPackages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Values/WorkspaceGroup.swiftPackages/macOS/CmuxWorkspaces/Tests/CmuxWorkspacesTests/WorkspaceCoordinatorTests.swiftResources/Localizable.xcstringsSources/AppDelegate.swiftSources/ContentView.swiftSources/Mobile/MobileWorkspaceListObserver.swiftSources/SessionPersistence.swiftSources/SessionPersistencePolicy+CrashStorage.swiftSources/SidebarWorkspaceGroupHeaderDropDelegate.swiftSources/SidebarWorkspaceGroupHeaderView.swiftSources/SidebarWorkspaceRenderItem.swiftSources/TabItemView+WorkspaceGroups.swiftSources/TabManager.swiftSources/TerminalController+ControlWorkspaceGroupContext.swiftSources/TerminalController+MobileWorkspaceList.swiftSources/VerticalTabsSidebar+WorkspaceGroups.swiftcmuxTests/MobileWorkspaceListFidelityTests.swiftcmuxTests/SidebarWorkspaceDropPlannerTests.swiftcmuxTests/WorkspaceGroupTests.swift
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)
Packages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Model/WorkspacesModel+Ordering.swift (1)
140-146: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep promoted pinned items in the pinned tier.
sidebarTopLevelWorkspaceIds(promotingWorkspaceId:)can insert a nested group anchor or grouped workspace before it is actually top-level, butclampedTopLevelReorderIndexstill derivespinnedIdsfrom the pre-promotion top-level rows. A pinned nested folder/member dragged out can therefore be clamped as unpinned and persisted below unpinned roots via the top-level reorder path.Proposed fix
let pinnedCount = topLevelIds.reduce(into: 0) { count, id in if pinnedIds.contains(id) { count += 1 } } - if pinnedIds.contains(workspaceId) { - return min(clamped, max(0, pinnedCount - 1)) + let alreadyPinnedTopLevel = pinnedIds.contains(workspaceId) + let promotedPinnedTopLevel: Bool + if let group = workspaceGroups.first(where: { $0.anchorWorkspaceId == workspaceId }) { + promotedPinnedTopLevel = group.parentGroupId != nil && group.isPinned + } else if let tab = tabs.first(where: { $0.id == workspaceId }) { + promotedPinnedTopLevel = tab.groupId != nil && tab.isPinned + } else { + promotedPinnedTopLevel = false + } + if alreadyPinnedTopLevel || promotedPinnedTopLevel { + let lastPinnedIndex = pinnedCount - (alreadyPinnedTopLevel ? 1 : 0) + return min(clamped, max(0, lastPinnedIndex)) } return max(clamped, pinnedCount)🤖 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 `@Packages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Model/WorkspacesModel`+Ordering.swift around lines 140 - 146, The issue is that when promoting a pinned nested group or workspace in the sidebarTopLevelWorkspaceIds(promotingWorkspaceId:) method, the insertion logic does not preserve its pinned status. When inserting the promotedWorkspaceId at the calculated position using min(groupIndex + 1, ids.count), the method can place a pinned item below the pinned tier boundary, causing it to be treated as unpinned. To fix this, check whether the promotedWorkspaceId was originally in the pinned items, and if so, constrain the insertion index to not exceed the pinnedCount boundary to ensure promoted pinned items remain in the pinned tier during reordering.
🤖 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
`@Packages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Model/WorkspacesModel`+Ordering.swift:
- Around line 140-146: The issue is that when promoting a pinned nested group or
workspace in the sidebarTopLevelWorkspaceIds(promotingWorkspaceId:) method, the
insertion logic does not preserve its pinned status. When inserting the
promotedWorkspaceId at the calculated position using min(groupIndex + 1,
ids.count), the method can place a pinned item below the pinned tier boundary,
causing it to be treated as unpinned. To fix this, check whether the
promotedWorkspaceId was originally in the pinned items, and if so, constrain the
insertion index to not exceed the pinnedCount boundary to ensure promoted pinned
items remain in the pinned tier during reordering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 03de335b-d96f-40ed-a6e7-196b6d73692a
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (15)
Packages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileSyncWorkspaceListResponse.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileWorkspacePreview+RemoteMapping.swiftPackages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/MobileCoreRPCClientTests.swiftPackages/iOS/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileWorkspaceGroupPreview.swiftPackages/iOS/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileWorkspaceListItem.swiftPackages/iOS/CmuxMobileShellModel/Tests/CmuxMobileShellModelTests/MobileWorkspaceListItemTests.swiftPackages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Coordinators/WorkspaceGroupCoordinator.swiftPackages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Coordinators/WorkspaceReorderCoordinator.swiftPackages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Model/WorkspacesModel+GroupInvariants.swiftPackages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Model/WorkspacesModel+Ordering.swiftPackages/macOS/CmuxWorkspaces/Tests/CmuxWorkspacesTests/WorkspaceCoordinatorTests.swiftResources/Localizable.xcstringsSources/SessionPersistencePolicy+CrashStorage.swiftSources/SidebarWorkspaceRenderItem.swiftSources/TabManager.swift
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
`@Packages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Coordinators/WorkspaceReorderCoordinator.swift`:
- Around line 275-282: Remove the else if branch that checks
promotedTab?.isPinned when promotedGroupIndex is nil. Root groups should only be
classified as pinned based on their own WorkspaceGroup.isPinned state, not by
their anchor tab's pin state. This prevents unpinned root groups whose anchor
workspace is pinned from being incorrectly placed in the pinned tier during drag
reorder. Apply the same fix to all similar occurrences where this condition is
present.
🪄 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: e7652931-ed26-4dc9-a342-98dec7775157
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (6)
Packages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Coordinators/WorkspaceReorderCoordinator.swiftPackages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Model/WorkspacesModel+Ordering.swiftPackages/macOS/CmuxWorkspaces/Tests/CmuxWorkspacesTests/WorkspaceCoordinatorTests.swiftSources/SidebarWorkspaceRenderItem.swiftSources/VerticalTabsSidebar+WorkspaceGroups.swiftcmuxTests/WorkspaceGroupTests.swift
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 (4)
Packages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Coordinators/WorkspaceGroupCoordinator.swift (2)
355-357: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winIncrement the count for the last-workspace holdout.
This branch removes the last remaining workspace from the deleted group but returns without incrementing
closed, despite the comment saying it should be reported as removed. A single-workspace group deletion can therefore return0.Suggested fix
if model.tabs.count <= 1 { model.assignGroup(workspaceId: tab.id, groupId: nil) + closed += 1 continue }🤖 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 `@Packages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Coordinators/WorkspaceGroupCoordinator.swift` around lines 355 - 357, In the WorkspaceGroupCoordinator.swift file, the branch handling when model.tabs.count <= 1 removes a workspace from the deleted group by calling model.assignGroup(workspaceId: tab.id, groupId: nil) but fails to increment the closed counter before continuing. Add an increment to the closed counter after the assignGroup call and before the continue statement to properly track that this workspace removal should be reported as removed in the final count.
531-612: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftProject nested group slot moves, or reject them.
moveWorkspaceGroupSlotcan reorder groups with a non-nilparentGroupId, butapplyWorkspaceGroupSlotOrderToTabsonly builds a root-group projection. For nested sibling moves, normalization ranks child rows by the old tab order, thensyncWorkspaceGroupsOrderToAnchorOrder()restores the oldworkspaceGroupsorder, so the move can report success without moving the folder.Either make the slot projection include nested sibling anchors in the reordered parent’s child order, or explicitly return
falsefor non-root group slot moves until that projection exists.🤖 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 `@Packages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Coordinators/WorkspaceGroupCoordinator.swift` around lines 531 - 612, The issue is that moveWorkspaceGroupSlot allows reordering groups with non-nil parentGroupId (nested groups), but applyWorkspaceGroupSlotOrderToTabs only builds a projection for root groups, causing nested moves to report success without actually reordering. Fix this by adding a guard check at the beginning of moveWorkspaceGroupSlot that returns false when the source group has a non-nil parentGroupId, explicitly rejecting nested group slot moves until the nested projection logic is implemented in applyWorkspaceGroupSlotOrderToTabs.Packages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Coordinators/WorkspaceReorderCoordinator.swift (1)
186-198: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMirror promotion-aware pinning in the drag planner.
The commit path adds a promoted pinned nested group/workspace to
pinnedTopLevelIds, but this planner helper returns only currently top-level pinned rows.SidebarDropPlannercan therefore compute an unpinned-tier target for a dragged pinned nested folder/workspace before commit clamps it elsewhere.Suggested fix
guard usesTopLevelRows || sidebarReorderUsesTopLevelRows( forDraggedWorkspaceId: draggedWorkspaceId, targetWorkspaceId: targetWorkspaceId ) else { return Set(model.tabs.filter { $0.groupId == nil && $0.isPinned }.map(\.id)) } - return model.sidebarTopLevelPinnedWorkspaceIds() + var pinnedIds = model.sidebarTopLevelPinnedWorkspaceIds() + if let draggedWorkspaceId, + !pinnedIds.contains(draggedWorkspaceId) { + if let promotedGroup = model.workspaceGroups.first(where: { + $0.anchorWorkspaceId == draggedWorkspaceId && $0.parentGroupId != nil + }) { + if promotedGroup.isPinned { + pinnedIds.insert(draggedWorkspaceId) + } + } else if let tab = model.tabs.first(where: { $0.id == draggedWorkspaceId }), + tab.groupId != nil, + !model.isWorkspaceGroupAnchor(draggedWorkspaceId), + tab.isPinned { + pinnedIds.insert(draggedWorkspaceId) + } + } + return pinnedIds🤖 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 `@Packages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Coordinators/WorkspaceReorderCoordinator.swift` around lines 186 - 198, The sidebarReorderPinnedWorkspaceIds method currently returns only currently top-level pinned rows but does not account for promoted pinned nested workspaces/groups, which causes the drag planner to compute incorrect target positions. Modify the logic in sidebarReorderPinnedWorkspaceIds to include promoted pinned nested items in addition to the currently top-level pinned ones, ensuring that both the unpinned-tier return path (in the guard else block) and the usesTopLevelRows path mirror the promotion-aware pinning behavior that the commit operation applies.Packages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Model/WorkspacesModel+GroupInvariants.swift (1)
306-329: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the dissolve comment to match parent promotion.
Lines 320-329 now promote direct members and child groups to the dissolved group’s former parent, but the doc comment still says members become ungrouped. That documents the old flat-group invariant.
Suggested comment update
- /// remaining members lose their `groupId` and stay in `tabs` as ungrouped - /// workspaces. Caller is responsible for having already removed the closed - /// workspace from `tabs`. + /// remaining members and direct child groups are promoted to the dissolved + /// group's former parent, or to the sidebar root when there is no parent. + /// Caller is responsible for having already removed the closed workspace + /// from `tabs`.🤖 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 `@Packages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Model/WorkspacesModel`+GroupInvariants.swift around lines 306 - 329, The documentation comment for the dissolveGroupsAnchoredBy method states that remaining members become ungrouped, but the actual implementation (lines 320-329) promotes them to the dissolved group's parent instead using parentByDissolvedGroupId. Update the doc comment above the method to accurately reflect that when a group is dissolved, its direct members and child groups are promoted to the dissolved group's former parent group, rather than becoming ungrouped. This will ensure the documentation matches the current behavior of the promotion logic.
🤖 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
`@Packages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Coordinators/WorkspaceGroupCoordinator.swift`:
- Around line 355-357: In the WorkspaceGroupCoordinator.swift file, the branch
handling when model.tabs.count <= 1 removes a workspace from the deleted group
by calling model.assignGroup(workspaceId: tab.id, groupId: nil) but fails to
increment the closed counter before continuing. Add an increment to the closed
counter after the assignGroup call and before the continue statement to properly
track that this workspace removal should be reported as removed in the final
count.
- Around line 531-612: The issue is that moveWorkspaceGroupSlot allows
reordering groups with non-nil parentGroupId (nested groups), but
applyWorkspaceGroupSlotOrderToTabs only builds a projection for root groups,
causing nested moves to report success without actually reordering. Fix this by
adding a guard check at the beginning of moveWorkspaceGroupSlot that returns
false when the source group has a non-nil parentGroupId, explicitly rejecting
nested group slot moves until the nested projection logic is implemented in
applyWorkspaceGroupSlotOrderToTabs.
In
`@Packages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Coordinators/WorkspaceReorderCoordinator.swift`:
- Around line 186-198: The sidebarReorderPinnedWorkspaceIds method currently
returns only currently top-level pinned rows but does not account for promoted
pinned nested workspaces/groups, which causes the drag planner to compute
incorrect target positions. Modify the logic in sidebarReorderPinnedWorkspaceIds
to include promoted pinned nested items in addition to the currently top-level
pinned ones, ensuring that both the unpinned-tier return path (in the guard else
block) and the usesTopLevelRows path mirror the promotion-aware pinning behavior
that the commit operation applies.
In
`@Packages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Model/WorkspacesModel`+GroupInvariants.swift:
- Around line 306-329: The documentation comment for the
dissolveGroupsAnchoredBy method states that remaining members become ungrouped,
but the actual implementation (lines 320-329) promotes them to the dissolved
group's parent instead using parentByDissolvedGroupId. Update the doc comment
above the method to accurately reflect that when a group is dissolved, its
direct members and child groups are promoted to the dissolved group's former
parent group, rather than becoming ungrouped. This will ensure the documentation
matches the current behavior of the promotion logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e28524bf-50eb-42c9-bf30-121d1b2c73e8
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (6)
Packages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Coordinators/WorkspaceGroupCoordinator.swiftPackages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Coordinators/WorkspaceReorderCoordinator.swiftPackages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Model/WorkspacesModel+GroupInvariants.swiftPackages/macOS/CmuxWorkspaces/Tests/CmuxWorkspacesTests/WorkspaceCoordinatorTests.swiftSources/ContentView.swiftcmuxTests/WorkspaceGroupTests.swift
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: 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
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swift`:
- Around line 61-63: The SwiftUI state backing the chat-refresh ignore logic is
currently exposed as non-private; update the `WorkspaceDetailView` `@State`
properties `ignoredChatSessionRefreshKey`, `ignoredChatSessionRefreshID`, and
`ignoredChatSessionRefreshTask` to be private. Keep the
`Task<[ChatSessionDescriptor]?, Never>` type unchanged, and only adjust access
control for these internal state members since they are not used as external
bindings.
🪄 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: 033a12d0-922f-46ff-924f-c33253918037
📒 Files selected for processing (13)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swiftPackages/macOS/CmuxFoundation/Sources/CmuxFoundation/SidebarDrop/SidebarWorkspaceReorderDropResolver.swiftPackages/macOS/CmuxFoundation/Tests/CmuxFoundationTests/SidebarDropPlannerTests.swiftPackages/macOS/CmuxSidebar/Sources/CmuxSidebar/Drag/SidebarDragState.swiftPackages/macOS/CmuxSidebar/Tests/CmuxSidebarTests/SidebarDragStateTests.swiftPackages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Coordinators/WorkspaceGroupCoordinator.swiftPackages/macOS/CmuxWorkspaces/Tests/CmuxWorkspacesTests/WorkspaceGroupUngroupPromotionTests.swiftSources/SidebarWorkspaceRenderItem.swiftSources/TerminalController+ControlWorkspaceGroupContext.swiftcmux.xcodeproj/project.pbxprojcmuxTests/ControlWorkspaceGroupCreateParentInferenceTests.swiftcmuxTests/SidebarWorkspaceRenderItemStaleAnchorTests.swift
🛑 Comments failed to post (1)
Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swift (1)
61-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Mark new
@Statepropertiesprivate.SwiftLint flags
ignoredChatSessionRefreshKey,ignoredChatSessionRefreshID, andignoredChatSessionRefreshTaskas non-private SwiftUI state. Since these back internal chat-refresh-ignoring logic (not exposed as bindings elsewhere in this view), they should beprivate.🧹 Proposed fix
- `@State` var ignoredChatSessionRefreshKey: String? - `@State` var ignoredChatSessionRefreshID: UUID? - `@State` var ignoredChatSessionRefreshTask: Task<[ChatSessionDescriptor]?, Never>? + `@State` private var ignoredChatSessionRefreshKey: String? + `@State` private var ignoredChatSessionRefreshID: UUID? + `@State` private var ignoredChatSessionRefreshTask: Task<[ChatSessionDescriptor]?, Never>?The
discouraged_optional_collectionhint on theTaskresult type appears to be a false positive here (the optional distinguishes "no update produced" from "empty list"); no action needed there.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.`@State` private var ignoredChatSessionRefreshKey: String? `@State` private var ignoredChatSessionRefreshID: UUID? `@State` private var ignoredChatSessionRefreshTask: Task<[ChatSessionDescriptor]?, Never>?🧰 Tools
🪛 SwiftLint (0.65.0)
[Warning] 63-63: Prefer empty collection over optional collection
(discouraged_optional_collection)
[Warning] 61-61: SwiftUI state properties should be private
(private_swiftui_state)
[Warning] 62-62: SwiftUI state properties should be private
(private_swiftui_state)
[Warning] 63-63: SwiftUI state properties should be private
(private_swiftui_state)
🤖 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 `@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swift` around lines 61 - 63, The SwiftUI state backing the chat-refresh ignore logic is currently exposed as non-private; update the `WorkspaceDetailView` `@State` properties `ignoredChatSessionRefreshKey`, `ignoredChatSessionRefreshID`, and `ignoredChatSessionRefreshTask` to be private. Keep the `Task<[ChatSessionDescriptor]?, Never>` type unchanged, and only adjust access control for these internal state members since they are not used as external bindings.Source: Linters/SAST tools
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Coordinators/WorkspaceGroupCoordinator.swift (1)
331-356: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the delete count for the last-workspace holdout.
The
tabs.count <= 1branch removes the workspace from the deleted group but never incrementsclosed, despite the comment saying it is still reported as removed. Either increment the count there or update the contract/comment so socket/CLI responses don’t misreport the operation.Proposed fix if the returned count is “removed from group”
if model.tabs.count <= 1 { model.assignGroup(workspaceId: tab.id, groupId: nil) + closed += 1 continue }🤖 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 `@Packages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Coordinators/WorkspaceGroupCoordinator.swift` around lines 331 - 356, The deleted-group cleanup in WorkspaceGroupCoordinator’s group-removal flow is undercounting the last-workspace holdout: the `tabs.count <= 1` branch clears the workspace’s `groupId` but never updates the returned `closed` count. Fix the `closeWorkspaceForGroupDeletion`-driven loop so this path either increments `closed` when `model.assignGroup(workspaceId:groupId:)` detaches the holdout, or adjust the surrounding contract/comment to match the actual meaning of the returned count. Keep the behavior aligned with the `closed` accumulator and the final `return closed` so socket/CLI responses stay accurate.
🤖 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/CmuxFoundation/Sources/CmuxFoundation/SidebarDrop/SidebarWorkspaceReorderDropResolver.swift`:
- Around line 172-177: The parent-group remap in
SidebarWorkspaceReorderDropResolver should not apply to normal workspace drops
on nested folder headers, since it can override the intended target group.
Update the target-resolution logic around the group-header branch and the
subsequent target.groupId handling so that the parentGroupId fallback is only
used for group-anchor sibling reorders, while regular workspace drops keep the
original target.groupId. Use the existing symbols target.isGroupHeader,
target.groupId, groupsById, and parentGroupId to locate and constrain this
branch.
- Around line 143-148: The dragged group check in
SidebarWorkspaceReorderDropResolver should also reject descendant targets, not
just the dragged group itself. Update the candidate validation inside the
draggedGroup branch to walk candidate.groupId’s parentGroupId chain and return
nil if it ever matches draggedGroup.id, preventing explicitGroupId from
resolving back into the dragged subtree.
In
`@Packages/macOS/CmuxWorkspaces/Tests/CmuxWorkspacesTests/WorkspaceGroupCreationPlacementRegressionTests.swift`:
- Line 9: `makeWorld()` is being called through a throwaway
`WorkspaceCoordinatorTests()` instance even though it does not use instance
state. Update `WorkspaceCoordinatorTests.makeWorld` to be `static` (or move it
into a shared test-support helper), then change
`WorkspaceGroupCreationPlacementRegressionTests` and any other callers to use
the new static/helper entrypoint so they no longer need to instantiate the test
suite just to access it.
---
Outside diff comments:
In
`@Packages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Coordinators/WorkspaceGroupCoordinator.swift`:
- Around line 331-356: The deleted-group cleanup in WorkspaceGroupCoordinator’s
group-removal flow is undercounting the last-workspace holdout: the `tabs.count
<= 1` branch clears the workspace’s `groupId` but never updates the returned
`closed` count. Fix the `closeWorkspaceForGroupDeletion`-driven loop so this
path either increments `closed` when `model.assignGroup(workspaceId:groupId:)`
detaches the holdout, or adjust the surrounding contract/comment to match the
actual meaning of the returned count. Keep the behavior aligned with the
`closed` accumulator and the final `return closed` so socket/CLI responses stay
accurate.
🪄 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: 9adb2b90-f068-4903-87dc-dc7d14d79bec
📒 Files selected for processing (15)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileWorkspaceListItem.swiftPackages/iOS/CmuxMobileShellModel/Tests/CmuxMobileShellModelTests/MobileWorkspaceListItemTests.swiftPackages/macOS/CmuxFoundation/Sources/CmuxFoundation/SidebarDrop/SidebarWorkspaceReorderDropResolver+GroupLegalRange.swiftPackages/macOS/CmuxFoundation/Sources/CmuxFoundation/SidebarDrop/SidebarWorkspaceReorderDropResolver.swiftPackages/macOS/CmuxFoundation/Tests/CmuxFoundationTests/SidebarDropPlannerTests.swiftPackages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Coordinators/WorkspaceGroupCoordinator.swiftPackages/macOS/CmuxWorkspaces/Tests/CmuxWorkspacesTests/WorkspaceCoordinatorTests.swiftPackages/macOS/CmuxWorkspaces/Tests/CmuxWorkspacesTests/WorkspaceGroupCreationPlacementRegressionTests.swiftSources/ContentView.swiftSources/TabManager.swiftSources/TerminalController+ControlWorkspaceGroupContext.swiftcmux.xcodeproj/project.pbxprojcmuxTests/ControlWorkspaceGroupCreateParentInferenceTests.swiftcmuxTests/SessionWorkspaceGroupSnapshotParentChainTests.swift
| struct WorkspaceGroupCreationPlacementRegressionTests { | ||
| @Test | ||
| func emptyNestedWorkspaceGroupPreservesParentTopLevelPosition() throws { | ||
| let (model, host, groups, _) = WorkspaceCoordinatorTests().makeWorld() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Instantiating WorkspaceCoordinatorTests() just to reach makeWorld().
makeWorld() doesn't touch self state; consider making it static on WorkspaceCoordinatorTests (or extracting it to a shared test-support helper) so callers don't need to construct a throwaway instance of the suite.
🤖 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
`@Packages/macOS/CmuxWorkspaces/Tests/CmuxWorkspacesTests/WorkspaceGroupCreationPlacementRegressionTests.swift`
at line 9, `makeWorld()` is being called through a throwaway
`WorkspaceCoordinatorTests()` instance even though it does not use instance
state. Update `WorkspaceCoordinatorTests.makeWorld` to be `static` (or move it
into a shared test-support helper), then change
`WorkspaceGroupCreationPlacementRegressionTests` and any other callers to use
the new static/helper entrypoint so they no longer need to instantiate the test
suite just to access it.
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
`@Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/SidebarDrop/SidebarWorkspaceReorderDropResolver.swift`:
- Around line 191-201: The drop-resolution logic in
SidebarWorkspaceReorderDropResolver should not let ordinary workspace drops fall
back to the parent folder when reordering inside a nested child group. Update
the branch that returns from the parentGroupId fallback so it only applies to
group-anchor sibling reorders, or requires an explicit outdent condition, and
keep workspace drops scoped to the current child group. Use the existing symbols
nextIsSameGroup, parentGroupId, groupIsInSubtree, and context.nextTarget to
locate and guard the fallback path.
In
`@Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/SidebarDrop/SidebarWorkspaceReorderDropResolver`+GroupLegalRange.swift:
- Around line 170-186: The directChildGroupId helper currently keeps walking
past the case where workspace.groupId already equals rootGroupId, which adds
unnecessary ancestor traversal and obscures the intended “child subgroup of
root” check. Update directChildGroupId(containing:rootGroupId:groupsById:) to
early-return nil when the workspace is directly in rootGroupId, while preserving
the existing loop for deeper nesting and the visited-cycle guard.
🪄 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: 56f398c1-4b24-4385-b137-32f51fba3304
📒 Files selected for processing (8)
Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/SidebarDrop/SidebarWorkspaceReorderDropResolver+GroupLegalRange.swiftPackages/macOS/CmuxFoundation/Sources/CmuxFoundation/SidebarDrop/SidebarWorkspaceReorderDropResolver.swiftPackages/macOS/CmuxFoundation/Tests/CmuxFoundationTests/SidebarDropLegalRangeTests.swiftPackages/macOS/CmuxFoundation/Tests/CmuxFoundationTests/SidebarDropNestedBoundaryTests.swiftSources/ContentView.swiftSources/SidebarDropIndicatorRowScopeResolver.swiftcmux.xcodeproj/project.pbxprojcmuxTests/SidebarDropIndicatorRowScopeResolverTests.swift
Comment on lines
+170
to
+186
| private func directChildGroupId( | ||
| containing workspace: SidebarWorkspaceReorderWorkspaceSnapshot, | ||
| rootGroupId: UUID, | ||
| groupsById: [UUID: SidebarWorkspaceReorderGroupSnapshot] | ||
| ) -> UUID? { | ||
| guard var currentGroupId = workspace.groupId else { return nil } | ||
| var visited: Set<UUID> = [] | ||
| while let group = groupsById[currentGroupId], | ||
| let parentGroupId = group.parentGroupId { | ||
| guard visited.insert(currentGroupId).inserted else { return nil } | ||
| if parentGroupId == rootGroupId { | ||
| return currentGroupId | ||
| } | ||
| currentGroupId = parentGroupId | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
Consider an early exit when the workspace is a direct member of rootGroupId itself.
When workspace.groupId == rootGroupId, the loop doesn't stop there — it climbs into rootGroupId's own ancestors (outside the subtree) before eventually returning nil. The final result is correct (given the acyclic-tree invariant enforced elsewhere), but the extra climbing is wasted work and makes the intent ("is this workspace nested under a child subgroup of root?") less obvious.
♻️ Proposed early-exit
private func directChildGroupId(
containing workspace: SidebarWorkspaceReorderWorkspaceSnapshot,
rootGroupId: UUID,
groupsById: [UUID: SidebarWorkspaceReorderGroupSnapshot]
) -> UUID? {
guard var currentGroupId = workspace.groupId else { return nil }
+ guard currentGroupId != rootGroupId else { return nil }
var visited: Set<UUID> = []
while let group = groupsById[currentGroupId],
let parentGroupId = group.parentGroupId {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private func directChildGroupId( | |
| containing workspace: SidebarWorkspaceReorderWorkspaceSnapshot, | |
| rootGroupId: UUID, | |
| groupsById: [UUID: SidebarWorkspaceReorderGroupSnapshot] | |
| ) -> UUID? { | |
| guard var currentGroupId = workspace.groupId else { return nil } | |
| var visited: Set<UUID> = [] | |
| while let group = groupsById[currentGroupId], | |
| let parentGroupId = group.parentGroupId { | |
| guard visited.insert(currentGroupId).inserted else { return nil } | |
| if parentGroupId == rootGroupId { | |
| return currentGroupId | |
| } | |
| currentGroupId = parentGroupId | |
| } | |
| return nil | |
| } | |
| private func directChildGroupId( | |
| containing workspace: SidebarWorkspaceReorderWorkspaceSnapshot, | |
| rootGroupId: UUID, | |
| groupsById: [UUID: SidebarWorkspaceReorderGroupSnapshot] | |
| ) -> UUID? { | |
| guard var currentGroupId = workspace.groupId else { return nil } | |
| guard currentGroupId != rootGroupId else { return nil } | |
| var visited: Set<UUID> = [] | |
| while let group = groupsById[currentGroupId], | |
| let parentGroupId = group.parentGroupId { | |
| guard visited.insert(currentGroupId).inserted else { return nil } | |
| if parentGroupId == rootGroupId { | |
| return currentGroupId | |
| } | |
| currentGroupId = parentGroupId | |
| } | |
| return 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
`@Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/SidebarDrop/SidebarWorkspaceReorderDropResolver`+GroupLegalRange.swift
around lines 170 - 186, The directChildGroupId helper currently keeps walking
past the case where workspace.groupId already equals rootGroupId, which adds
unnecessary ancestor traversal and obscures the intended “child subgroup of
root” check. Update directChildGroupId(containing:rootGroupId:groupsById:) to
early-return nil when the workspace is directly in rootGroupId, while preserving
the existing loop for deeper nesting and the visited-cycle guard.
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
`@Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/SidebarDrop/SidebarWorkspaceReorderDropResolver.swift`:
- Around line 193-202: The reordering logic in
SidebarWorkspaceReorderDropResolver is only checking the immediate next target’s
groupId, so descendant rows can be misclassified as leaving the current group.
Update the decision in the affected branch to treat any next target that belongs
to the current group’s subtree as still inside that group by using the
structured hierarchy from context and groupsById/parentGroupId, not just direct
equality on nextTarget?.groupId. Keep the return path in the same reorder
resolver flow, but make the inside/outside group check recursive or
ancestry-based so child-folder headers don’t make the divider
rootable/ambiguous.
🪄 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: 9473b0df-70aa-48e2-a869-0f1cbf042860
📒 Files selected for processing (2)
Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/SidebarDrop/SidebarWorkspaceReorderDropResolver.swiftPackages/macOS/CmuxFoundation/Tests/CmuxFoundationTests/SidebarDropNestedBoundaryTests.swift
lawrencecchen
added
the
stale-revisit
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
3 participants
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
WorkspaceGroup.parentGroupIdand restore-stableSessionWorkspaceGroupSnapshot.parentGroupIndexfallback.New Foldercontext-menu action.Design Notes
cmux.jsonand richer keyboard tree navigation. Existing config-driven group styling/placement remains flat/per-cwd in this PR.Validation
git diff --checkpython3 -m json.tool Resources/Localizable.xcstrings >/dev/nullpython3 scripts/check-package-resolved-policy.pypython3 scripts/check-workspace-package-groups.py --check./scripts/lint-pbxproj-test-wiring.shPer issue instructions, I did not run a local build,
reload.sh, orxcodebuild.Closes #6610
Refs #6514
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Adds nested workspace folders with arbitrary depth on Mac and mobile, with subfolder creation, drag-to-reparent, depth-aware rendering with unread aggregation, and full persistence/restore. Refines nested drop planning and pin classification, improves mobile payloads/indentation, and aligns with #6610.
New Features
WorkspaceGroup.parentGroupId; snapshots storeparentGroupIdand restore-stableparentGroupIndex; restore walks up to the nearest surviving ancestor; create infers parent from selected children or accepts explicitparentGroupId.parent_group_id(control also sendsparent_group_ref); mobile list hashing includes parent chain.depthand collapsed-unread aggregation; header menu adds “New Folder”; mobile rows carrydepthfor indentation; selecting a child scrolls to the nearest collapsed ancestor.Bug Fixes
Written for commit 287c753. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests