Repository navigation
Keep workspace group creation in place - #4989
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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:
📝 WalkthroughWalkthroughaddWorkspace gains a flag to skip post-insert normalization. createWorkspaceGroup captures pre-insert tab order, creates the anchor without normalizing, then repositions the new group via runs-preserving placement helpers that rebuild tabs as anchor-first contiguous runs. Sidebar drag/drop and indicator logic now use reorder-scoped id providers and header-measured drop delegates. ChangesWorkspace Group Ordering
🎯 4 (Complex) | ⏱️ ~45 minutes
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 2 warnings)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmuxTests/WorkspaceGroupTests.swift (1)
43-59: ⚡ Quick winConsider adding a count assertion for test completeness.
The test verifies specific positions 0-4 but doesn't assert that
reorderedIds.count == 5. Adding this check would catch regressions where unexpected tabs are created during group creation.🧪 Suggested assertion
let reorderedIds = manager.tabs.map(\.id) +#expect(reorderedIds.count == 5) `#expect`(reorderedIds[0] == originalIds[0])🤖 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 `@cmuxTests/WorkspaceGroupTests.swift` around lines 43 - 59, The test createGroupKeepsFirstChildPosition is missing an assertion on the total number of tabs after grouping; update the test to assert the tab count by checking reorderedIds.count equals 5 (use the existing reorderedIds variable from the test on manager.tabs) before the position checks so the test will fail if extra or missing tabs are present.
🤖 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.
Nitpick comments:
In `@cmuxTests/WorkspaceGroupTests.swift`:
- Around line 43-59: The test createGroupKeepsFirstChildPosition is missing an
assertion on the total number of tabs after grouping; update the test to assert
the tab count by checking reorderedIds.count equals 5 (use the existing
reorderedIds variable from the test on manager.tabs) before the position checks
so the test will fail if extra or missing tabs are present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a5d1c826-3e5e-4baf-add5-c6966661db2d
📒 Files selected for processing (2)
Sources/TabManager.swiftcmuxTests/WorkspaceGroupTests.swift
Greptile SummaryThis PR fixes workspace group creation jumping to the top of the sidebar by inserting the new group anchor at the first selected workspace's original position (
Confidence Score: 5/5Safe to merge; ordering logic is well-tested at the model layer and drop-delegate changes are encapsulated behind static policy helpers. No functional bugs found. Ordering algorithms are correct: orphaned group IDs are cleaned before dictionary lookups, the fallback loop in normalizeWorkspaceGroupRunsPreservingOrder ensures no tabs are silently dropped, clampedTopLevelReorderIndex correctly enforces pin-tier boundaries. The dropExited-without-dropEntered imbalance in SidebarWorkspaceGroupHeaderDropDelegate is harmless because the reorder delegate only clears the indicator when its own tabId matches. dropIndicatorUsesTopLevelRows is set and cleared consistently through setDropIndicator. Sources/TabManager.swift continues to grow past an already substantial line count; the new ordering helpers have no UI dependencies and would benefit from extraction into a dedicated module over time. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[createWorkspaceGroup] -->|capture originalTabOrder| B[addWorkspace normalizeAfterInsert:false]
B --> C[assignGroup to children + anchor]
C --> D[placeNewWorkspaceGroupAtCreationPosition]
D --> E{eligible children found?}
E -- No --> F[normalizeWorkspaceGroupContiguity default order]
E -- Yes --> G[build desiredIds: anchor+children at first-child slot]
G --> H[topLevelWorkspaceIdsPreservingOrder]
H --> I[normalizeWorkspaceGroupContiguity preservingTopLevelIds]
I --> J[syncWorkspaceGroupsOrderToAnchorOrder]
K[reorderSidebarWorkspace] --> L{usesTopLevelRows or dragged is anchor?}
L -- Yes --> M[reorderTopLevelWorkspaceItem]
L -- No --> N[reorderWorkspace flat path]
M --> O[clampedTopLevelReorderIndex]
O --> P[normalizeWorkspaceGroupRunsPreservingOrder]
P --> Q[syncWorkspaceGroupsOrderToAnchorOrder]
R[GroupHeader drop] --> S{center zone?}
S -- Yes --> T[addWorkspaceToGroup]
S -- No edge --> U[reorderDelegate.performDrop]
S -- No-op edge --> V[consume drop clearDrag]
Reviews (18): Last reviewed commit: "Keep same-group header edge drops in gro..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| autoWelcomeIfNeeded: Bool = true, | ||
| normalizeWorkspaceGroupsAfterInsert: Bool = true |
There was a problem hiding this comment.
addWorkspace
normalizeWorkspaceGroupsAfterInsert is a caller-context flag that changes addWorkspace's internal contract: normalization only fires when the caller says so. The cmux-swift-architectural-rethink rule flags new Boolean flags wired through multi-purpose entrypoints because they make the function's single-invocation semantics implicit. The only current false call site is createWorkspaceGroup, which defers normalization to placeNewWorkspaceGroupAtCreationPosition. If a future caller forgets to pass false or forgets to normalize afterward, the group's tab order could end up in an inconsistent state. Consider encapsulating the deferred-normalization sequence inside createWorkspaceGroup without exposing the flag on the general addWorkspace surface.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (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!
| if workspaceGroups.contains(where: { $0.id == groupId }) { | ||
| syncWorkspaceGroupsOrderToAnchorOrder() | ||
| } |
There was a problem hiding this comment.
After normalizeWorkspaceGroupRunsPreservingOrder rebuilds tabs[], an ungrouped workspace positioned before the first selected child will remain between ungrouped workspaces and the new group anchor, violating the unpinned-groups-before-ungrouped-unpinned tier until the next normalizeWorkspaceGroupContiguity call. Workspace-number shortcuts and next/previous navigation read tabs[] directly in this window. If the deviation is intentional, a comment here explaining it would help future readers; otherwise a guarded follow-up call to normalizeWorkspaceGroupContiguity() after the sync would close the gap.
There was a problem hiding this comment.
Documented this invariant at the creation-position helper. The stable-position path intentionally preserves the outer row order instead of applying the older all-groups-before-ungrouped tier normalization; callers that need tier ordering still use normalizeWorkspaceGroupContiguity().
— Claude Code
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/ContentView.swift`:
- Around line 11569-11576: Compute sidebarReorderIds once in the parent view
instead of calling tabManager.sidebarReorderWorkspaceIds(forDraggedWorkspaceId:)
inside each row: when rendering the sidebar, compute let sidebarReorderIds =
tabManager.sidebarReorderWorkspaceIds(forDraggedWorkspaceId:
dragState.draggedTabId) (keyed by dragState.draggedTabId so it updates only when
the dragged id changes) and pass that array into each row; update usages that
currently call sidebarReorderWorkspaceIds (e.g., the code around
SidebarTabDropIndicatorPredicate.topVisible(forTabId:draggedTabId:dropIndicator:tabIds:))
to use the passed-in sidebarReorderIds so you avoid per-row recomputation and
the O(N²) behavior.
In `@Sources/SidebarWorkspaceGroupHeaderView.swift`:
- Around line 56-66: The onChange call in the rowHeightProbe GeometryReader uses
the deprecated single-parameter overload; update the call for macOS 14+ to use
onChange(of:initial:_:) by supplying the initial value and the two-parameter
closure form so changes to proxy.size.height update rowHeight correctly; locate
the GeometryReader in rowHeightProbe and replace .onChange(of:
proxy.size.height) { newHeight in ... } with the onChange(of: proxy.size.height,
initial: proxy.size.height) { _, newHeight in rowHeight = max(newHeight, 1) }
(ensuring rowHeight assignment logic remains the same).
In `@Sources/TabManager.swift`:
- Around line 6438-6448: The current projection building for desiredIds uses
removeFirst() on pinnedAnchors and unpinnedAnchors (pinnedAnchors,
unpinnedAnchors, desiredIds, topLevelIds, groupsByAnchorId, group.isPinned)
which yields O(n²) behavior; replace removeFirst() with simple index cursors
(e.g., pinnedIndex and unpinnedIndex) or Array.Iterator to read sequentially
from pinnedAnchors/unpinnedAnchors and increment the cursor when consuming an
element so desiredIds is built in linear time and moveWorkspaceGroup no longer
suffers quadratic behavior.
🪄 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: e977bba2-111c-4985-a556-7b2b9c64add4
📒 Files selected for processing (10)
Resources/Info.plistSources/ContentView.swiftSources/SidebarTabDragPayload+WorkspaceGroups.swiftSources/SidebarWorkspaceGroupDragPayload.swiftSources/SidebarWorkspaceGroupHeaderView.swiftSources/TabManager.swiftSources/VerticalTabsSidebar+WorkspaceGroups.swiftcmux.xcodeproj/project.pbxprojcmuxTests/WorkspaceGroupTests.swiftdocs/workspace-groups.md
💤 Files with no reviewable changes (4)
- Resources/Info.plist
- cmux.xcodeproj/project.pbxproj
- Sources/SidebarWorkspaceGroupDragPayload.swift
- Sources/SidebarTabDragPayload+WorkspaceGroups.swift
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
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)
6458-6475:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRe-tier top-level ids before projecting anchors.
Line 6460 builds the projection from the transient
tabsorder, but callers likemoveTabToTop/moveTabsToToptemporarily bubble grouped members usingWorkspace.isPinned, notWorkspaceGroup.isPinned. That lets an unpinned group slot stay ahead of pinned groups after normalization.♻️ Proposed fix
private func applyWorkspaceGroupSlotOrderToTabs() { let groupsByAnchorId = Dictionary(uniqueKeysWithValues: workspaceGroups.map { ($0.anchorWorkspaceId, $0) }) - let topLevelIds = sidebarTopLevelWorkspaceIds() + let topLevelIds = sidebarTopLevelWorkspaceIds() + let pinnedTopLevelIds = sidebarTopLevelPinnedWorkspaceIds() + let tierOrderedTopLevelIds = + topLevelIds.filter { pinnedTopLevelIds.contains($0) } + + topLevelIds.filter { !pinnedTopLevelIds.contains($0) } var pinnedAnchors = workspaceGroups.filter(\.isPinned).map(\.anchorWorkspaceId) var unpinnedAnchors = workspaceGroups.filter { !$0.isPinned }.map(\.anchorWorkspaceId) - let desiredIds = topLevelIds.map { id -> UUID in + let desiredIds = tierOrderedTopLevelIds.map { id -> UUID in guard let group = groupsByAnchorId[id] else { return id } if group.isPinned, !pinnedAnchors.isEmpty { return pinnedAnchors.removeFirst()🤖 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 6458 - 6475, The current applyWorkspaceGroupSlotOrderToTabs() projects topLevelIds from the transient tabs order without re-tiering pinned status, so grouped members bubbled via Workspace.isPinned (by callers like moveTabToTop/moveTabsToTop) aren't reflected and an unpinned WorkspaceGroup slot can remain ahead of pinned slots; fix by re-tiering top-level IDs before projecting anchors: compute topLevelIds so that you group/reorder top-level workspaces by the workspace-level pinned flag (Workspace.isPinned) or by checking if any member in a WorkspaceGroup is pinned, then build desiredIds from that re-tiered topLevelIds (update the variable topLevelIds or its producer sidebarTopLevelWorkspaceIds() usage inside applyWorkspaceGroupSlotOrderToTabs), then proceed with normalizeWorkspaceGroupRunsPreservingOrder(desiredIds) and syncWorkspaceGroupsOrderToAnchorOrder().
🤖 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 `@cmuxTests/WorkspaceGroupTests.swift`:
- Around line 93-110: The test
movingGroupedChildToTopKeepsAnchorFirstWhenGroupIsAlreadyFirst assumes three
tabs but makeTabManager() only creates two, causing an out-of-bounds access on
originalIds[2]; update the expected array in the `#expect` assertion to only
reference existing IDs (originalIds[0] and originalIds[1]) and the group's
anchorWorkspaceId so the assertion matches the two-tab setup after calling
manager.moveTabToTop(originalIds[1]); ensure you update the expected ordering in
the `#expect` check to remove originalIds[2] and reflect the correct reordered
sequence for manager.tabs.map(\.id).
In `@Sources/TabManager.swift`:
- Around line 5555-5557: The postWorkspaceOrderDidChange call is only sending
the explicitly selected tabId after moveWorkspaceGroupSlotToTierStart(...) which
moves the entire group slot; update both places (the block comparing
tabs.map(\.id) to previousOrder and the similar branch around
moveWorkspaceGroupSlotToTierStart at lines ~5586-5587) to compute and publish
all moved workspace ids (e.g., the ids of the entire group/slot that were
relocated) instead of just [tabId], by deriving the moved ids from the
group/slot being moved and passing that array into
postWorkspaceOrderDidChange(movedWorkspaceIds:).
---
Outside diff comments:
In `@Sources/TabManager.swift`:
- Around line 6458-6475: The current applyWorkspaceGroupSlotOrderToTabs()
projects topLevelIds from the transient tabs order without re-tiering pinned
status, so grouped members bubbled via Workspace.isPinned (by callers like
moveTabToTop/moveTabsToTop) aren't reflected and an unpinned WorkspaceGroup slot
can remain ahead of pinned slots; fix by re-tiering top-level IDs before
projecting anchors: compute topLevelIds so that you group/reorder top-level
workspaces by the workspace-level pinned flag (Workspace.isPinned) or by
checking if any member in a WorkspaceGroup is pinned, then build desiredIds from
that re-tiered topLevelIds (update the variable topLevelIds or its producer
sidebarTopLevelWorkspaceIds() usage inside applyWorkspaceGroupSlotOrderToTabs),
then proceed with normalizeWorkspaceGroupRunsPreservingOrder(desiredIds) and
syncWorkspaceGroupsOrderToAnchorOrder().
🪄 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: 04e02da9-b073-4abb-a055-b648743ae035
📒 Files selected for processing (3)
Sources/SidebarWorkspaceGroupHeaderView.swiftSources/TabManager.swiftcmuxTests/WorkspaceGroupTests.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 `@Sources/TabManager.swift`:
- Around line 6461-6465: The current implementation rescans topLevelIds and
workspaceGroups multiple times (calls to sidebarTopLevelPinnedWorkspaceIds(),
two topLevelIds.filter(...) and two workspaceGroups.filter(...)) inside
applyWorkspaceGroupSlotOrderToTabs(), causing extra work on the reorder hot
path; refactor to do a single pass that partitions topLevelIds into
pinnedTopLevelIds and unpinnedTopLevelIds and simultaneously partitions
workspaceGroups into pinnedAnchors and unpinnedAnchors (or build a Set of pinned
IDs from sidebarTopLevelPinnedWorkspaceIds() once and then loop
topLevelIds/workspaceGroups once) and then use those two partitioned collections
for the final projection so you avoid multiple full-array filters.
🪄 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: c9ed6d96-6f80-4b5b-9f85-2f1cc43c9faa
📒 Files selected for processing (5)
Sources/ContentView.swiftSources/SidebarWorkspaceGroupHeaderView.swiftSources/TabManager.swiftSources/VerticalTabsSidebar+WorkspaceGroups.swiftcmuxTests/WorkspaceGroupTests.swift
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 cf0804e. Configure here.
- Keep workspace group creation in place (manaflow-ai#4989) - Allow narrower sidebar with titlebar accessories (manaflow-ai#5013) - Add beta TextBox defaults settings (manaflow-ai#4773) - Add default terminal registration (manaflow-ai#4935) - Make group new workspace placement configurable (manaflow-ai#5018) - Fix nightly publishing runner selection (manaflow-ai#5022) Conflicts resolved: - ContentView: take upstream's SidebarWorkspaceTopDropIndicator extraction. - cmuxApp: keep fork's QuickTerminal/WorkspaceTopTabsVisibility @AppStorage + TerminalCopyOnSelectSettings; adopt upstream's Setting(\.terminal.*) for textBoxMaxLines + showTextBoxOnNewTerminals + focusTextBoxOnNewTerminals. - xcstrings: keep both fork's settings.app.workspaceTopTabsVisibility[.*] keys and upstream's settings.app.workspaceGroupNewWorkspacePlacement. - TabManagerUnitTests: drop fork's testNewSurfaceCreatesAndFocusesTopLevelTab (upstream renames + retains coverage via testNewSurfaceFocusesCreatedSurface).
This branch was successfully deployed
2 participants
Summary
Testing
Issues
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Touches core
TabManagertab/group ordering and sidebar drag-drop paths used on every reorder; behavior is broad but heavily covered by new unit tests.Overview
New groups stay where you grouped them instead of jumping above ungrouped workspaces. Creation snapshots the pre-group tab order, inserts the anchor at the first selected child, and normalizes contiguous runs without reshuffling unrelated top-level rows (
placeNewWorkspaceGroupAtCreationPosition, optionalnormalizeWorkspaceGroupsAfterInsertonaddWorkspace).Sidebar ordering is modeled as top-level rows (solo workspaces + group anchors).
TabManageradds scoped reorder ID/pinned helpers,reorderSidebarWorkspace/ top-level moves, and rebuildstabs[]vianormalizeWorkspaceGroupRunsPreservingOrderwhile syncingworkspaceGroupsto anchor positions. Move-to-top, add-to-group, and group-slot moves were updated to preserve visible slots and pinned vs unpinned tiers.Drag-and-drop routes drops through those scopes:
SidebarDragStatecentralizes begin/clear/set indicator (including top-level mode), drop validation/planners usesidebarReorderWorkspaceIds, and commits callreorderSidebarWorkspace. Group headers reuse the tab drag payload—edge = reorder, center = add to group—with shared top indicators, anEquatableheader, and removal of the separate group-reorder UTI/files.Regression tests cover creation position, header/member drags, collapsed headers, and pin-tier behavior; workspace-groups docs describe the simplified two-tier sidebar layout.
Reviewed by Cursor Bugbot for commit 1c1771b. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Keep newly created workspace groups at the first selected workspace’s position and make sidebar reordering operate on top‑level rows. Group headers now drag like rows (edge = reorder, center = add) with scoped indicators and lazy, Equatable headers to reduce sidebar churn.
Bug Fixes
New Features
TabManager(sidebarReorderWorkspaceIds,reorderSidebarWorkspace, pinned‑set helpers, clamped indices, projection back intotabs[]).placeNewWorkspaceGroupAtCreationPosition;addWorkspace(..., normalizeWorkspaceGroupsAfterInsert: Bool)keeps sections contiguous.SidebarDragStateaddsbeginDragging/setDropIndicatorand tracks top‑level indicator usage.Written for commit 1c1771b. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation