Repository navigation
Revert "Fix workspace group drag drop intent (#6532)" — crashes app-host on main - #6713
Merged
Merged
Conversation
This reverts commit 5cf0557.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughRemoves the NSView-based workspace reorder drop overlay infrastructure (resolver, plan, request, target, pending-drop, and bridge types). Replaces it with a SwiftUI-native ChangesSidebar drag/drop system replacement
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (23 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a8d37a2. Configure here.
| // Hover updates and drag-end own indicator changes. | ||
| if dragState.dropIndicator?.tabId == targetTabId { | ||
| dragState.clearDropIndicator() | ||
| } |
There was a problem hiding this comment.
Drop exit clears indicator early
Medium Severity
SidebarTabDropDelegate again clears the active drop indicator in dropExited when dropIndicator.tabId matches the row delegate. SwiftUI often emits spurious row exits during drag-driven relayout, so the line can vanish while the pointer is still over a valid target until the next dropUpdated.
Reviewed by Cursor Bugbot for commit a8d37a2. Configure here.
| } | ||
| if tab.groupId != inferred { | ||
| model.assignGroup(workspaceId: workspaceId, groupId: inferred) | ||
| tab.groupId = inferred |
There was a problem hiding this comment.
Drag join hides selected workspace
Medium Severity
After a drag-driven reorder, group membership is applied by setting tab.groupId directly, and assignGroup no longer expands collapsed groups for the focused workspace. If the dragged tab is already selected and lands in a collapsed group, selectedTabId does not change, so the sidebar can keep the group folded and hide the active workspace.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit a8d37a2. Configure here.
Contributor
Greptile SummaryThis PR reverts #6532 ("Fix workspace group drag drop intent") to unblock
Confidence Score: 4/5Safe to merge — the crash-inducing overlay is fully removed and the restored drop-delegate path is the previously stable implementation. The bulk of the change is a clean, mechanical revert of the 663-line SwiftUI drop overlay causing reentrant layout segfaults. The restored code was working before #6532 landed. Two new blocks in WorkspaceGroupCoordinator patch a collapsed-group visibility bug correctly but leave a host-sequencing ordering issue unaddressed. The SidebarWorkspaceGroupHeaderView Equatable conformance omits tabDropDelegateFactory, leaving a narrow stale-delegate window on membership-only changes outside a drag. WorkspaceGroupCoordinator.swift (new timing-patch blocks) and SidebarWorkspaceGroupHeaderView.swift (Equatable omits tabDropDelegateFactory) Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Host as TabManager / Host
participant GC as WorkspaceGroupCoordinator
participant Model as WorkspacesModel
participant Sidebar as VerticalTabsSidebar (SwiftUI)
Note over Host,Sidebar: Before revert (#6532 path — crashes)
Host->>GC: createWorkspace(inGroup:)
GC->>Model: assignGroup + normalizeContiguity
Sidebar-->>Sidebar: SidebarWorkspaceReorderDropOverlay body (reentrant layout → SIGSEGV)
Note over Host,Sidebar: After revert (this PR)
Host->>GC: createWorkspace(inGroup:select:)
GC->>Model: assignGroup
GC->>Model: "expand group if select && isCollapsed [NEW]"
GC->>Model: normalizeWorkspaceGroupContiguity
GC-->>Host: newWorkspace
Host->>GC: addWorkspaceToGroup(workspaceId:groupId:)
GC->>Model: assignGroup
GC->>Model: "expand group if selectedTabId==workspaceId && isCollapsed [NEW]"
GC->>Model: normalizeWorkspaceGroupContiguity
Sidebar->>Sidebar: SidebarWorkspaceGroupHeaderDropDelegate.performDrop
Sidebar->>Host: addWorkspaceToGroup via tabManager
Sidebar->>Sidebar: clearDropState (dragState + autoScroll)
%%{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 Host as TabManager / Host
participant GC as WorkspaceGroupCoordinator
participant Model as WorkspacesModel
participant Sidebar as VerticalTabsSidebar (SwiftUI)
Note over Host,Sidebar: Before revert (#6532 path — crashes)
Host->>GC: createWorkspace(inGroup:)
GC->>Model: assignGroup + normalizeContiguity
Sidebar-->>Sidebar: SidebarWorkspaceReorderDropOverlay body (reentrant layout → SIGSEGV)
Note over Host,Sidebar: After revert (this PR)
Host->>GC: createWorkspace(inGroup:select:)
GC->>Model: assignGroup
GC->>Model: "expand group if select && isCollapsed [NEW]"
GC->>Model: normalizeWorkspaceGroupContiguity
GC-->>Host: newWorkspace
Host->>GC: addWorkspaceToGroup(workspaceId:groupId:)
GC->>Model: assignGroup
GC->>Model: "expand group if selectedTabId==workspaceId && isCollapsed [NEW]"
GC->>Model: normalizeWorkspaceGroupContiguity
Sidebar->>Sidebar: SidebarWorkspaceGroupHeaderDropDelegate.performDrop
Sidebar->>Host: addWorkspaceToGroup via tabManager
Sidebar->>Sidebar: clearDropState (dragState + autoScroll)
Reviews (1): Last reviewed commit: "Revert "Fix workspace group drag drop in..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+155
to
162
| if select, | ||
| let idx = model.workspaceGroups.firstIndex(where: { $0.id == groupId }), | ||
| model.workspaceGroups[idx].isCollapsed { | ||
| model.workspaceGroups[idx].isCollapsed = false | ||
| } | ||
| model.normalizeWorkspaceGroupContiguity() | ||
| host.workspaceOrderDidChange(movedWorkspaceIds: [newWorkspace.id]) | ||
| return newWorkspace |
Contributor
There was a problem hiding this comment.
The comment explains the root problem: the host's selectedTabId auto-expand hook fires before assignGroup completes, so group membership isn't visible when expansion is first attempted. These two new blocks compensate by imperatively re-expanding inside the coordinator after assignment. This is the pattern flagged by the architectural-rethink rule — a timing repair that leaves the bad host-sequencing invariant in place. The coordinator now silently absorbs responsibility for a lifecycle ordering guarantee that belongs to the host's workspace-creation call sequence. If the host ever changes the order of selectedTabId assignment relative to group assignment, this second expansion could fire redundantly or conflict with a future expansion guard. Consider documenting the exact caller-sequence constraint this relies on, and track fixing the host's ordering as follow-up work before re-landing #6532.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
Comment on lines
37
to
41
| lhs.rowSpacing == rhs.rowSpacing && | ||
| lhs.isFirstRow == rhs.isFirstRow && | ||
| lhs.isBeingDragged == rhs.isBeingDragged && | ||
| lhs.topDropIndicatorVisible == rhs.topDropIndicatorVisible && | ||
| lhs.bottomDropIndicatorVisible == rhs.bottomDropIndicatorVisible | ||
| lhs.topDropIndicatorVisible == rhs.topDropIndicatorVisible | ||
| } |
Contributor
There was a problem hiding this comment.
tabDropDelegateFactory excluded from Equatable — stale delegate on membership-only changes
The Equatable conformance compares topDropIndicatorVisible and isBeingDragged but not tabDropDelegateFactory. The factory closure captures workspaceGroupIdByWorkspaceId from renderContext. If workspace group membership changes while no drag indicator is active (e.g., a socket command reassigns a workspace to a different group while the sidebar is idle), topDropIndicatorVisible and isBeingDragged are both false and unchanged, so SwiftUI's equatable optimization skips re-rendering the row. The .onDrop(delegate:) modifier then holds a stale delegate with the old workspaceGroupIdByWorkspaceId snapshot, which could route a subsequent drag-drop to the wrong group. In practice this window is narrow, but worth documenting or closing by including a version counter or checksum of the drop-relevant inputs in the equality check.
Rule Used: Flag SwiftUI changes that can cause stale state, b... (source)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/ContentView.swift (1)
15672-15734: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDon’t treat unchanged index as no-op when group membership changes.
After removing
explicitGroupId, a drop can keep the same numerictargetIndexbut still need a top-level ⇄ group membership transition. The guard on Line 15715 skipsreorderSidebarWorkspace(...), so the coordinator never gets a chance to apply the inferred group change. Include the inferred target group/current group in the no-op check, or move the no-op decision into the coordinator where membership inference is authoritative.🤖 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/ContentView.swift` around lines 15672 - 15734, The guard statement checking `fromIndex != targetIndex` treats a drop as a no-op and returns early without calling `reorderSidebarWorkspace()`, but this fails to account for group membership transitions that can occur even when the numeric index remains unchanged. Modify the no-op guard to also check whether the inferred target group membership differs from the current group membership by computing the target group for the drop location and comparing it against the current group, or alternatively move the no-op decision logic into the coordinator where group membership inference is authoritative so that membership changes are properly applied regardless of index position.
🤖 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/SidebarTabDropIndicatorPredicate.swift`:
- Around line 18-24: The predicate containing the guard statement that checks
`tabIds.firstIndex(of: tabId)` performs a full linear scan over the tabIds
collection for every sidebar row render, causing O(n²) performance. Instead of
scanning within the predicate, modify the method signature to accept a
precomputed index lookup (such as a dictionary mapping tabId to its index
position) as a parameter from the render context. Replace the
`tabIds.firstIndex(of: tabId)` call with an O(1) lookup from this precomputed
dictionary, and update the call sites to pass this lookup when invoking the
predicate.
In
`@Packages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Coordinators/WorkspaceReorderCoordinator.swift`:
- Around line 274-283: The guard statement at the beginning of the method
prevents execution from reaching the model.assignGroup call when fromIndex
equals clampedTarget. For grouped workspaces being promoted, the function should
still clear the groupId even when the top-level indices are unchanged, since
sidebarTopLevelWorkspaceIds can synthesize the promoted row at the same index.
Modify the guard condition to allow the function to continue when
promotesGroupedWorkspace is true and fromIndex equals clampedTarget, so the
groupId clearing logic in model.assignGroup still executes while skipping the
unnecessary topLevelIds reordering.
In
`@Packages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Model/WorkspacesModel`+Ordering.swift:
- Around line 97-115: The function clampedTopLevelReorderIndex relies on
sidebarTopLevelPinnedWorkspaceIds which recomputes the pinned set from the
non-promoted top-level view, causing promoted children to not be recognized as
pinned. To fix this, modify clampedTopLevelReorderIndex to accept the dragged
workspace ID as a parameter (the workspace being moved), and when determining
pin tier constraints, check the authoritative pin state of that workspace
directly from the model state (checking both workspace and group isPinned
properties) rather than relying solely on the computed pinnedIds set. This
ensures the promoted child's pin state is preserved during reordering.
In
`@Packages/macOS/CmuxWorkspaces/Tests/CmuxWorkspacesTests/WorkspaceCoordinatorTests.swift`:
- Line 213: The test methods contain multiple instances of try! `#require`(...) at
lines 213, 244, 263, 278, and 317 which cause forced crashes instead of proper
test failure assertions. For each test method containing this pattern, add
throws to the test method signature and replace try! `#require`(...) with try
`#require`(...) so that requirement failures are properly reported as test
assertions rather than hard crashes.
In `@Sources/ContentView.swift`:
- Around line 10620-10635: The top drop zone overlay is gated on both
dragState.draggedTabId != nil and firstWorkspaceId existing, which prevents the
SidebarTabDropDelegate from mounting during foreign (cross-window) drags. Remove
the dragState.draggedTabId != nil check from the if condition so the overlay
remains mounted whenever firstWorkspaceId exists, allowing the delegate to
properly validate the payload and handle cross-window drags through
activateForeignDragIfNeeded().
- Around line 12308-12321: Remove the delayed failsafe mechanism in the
requestClearSoon function that uses DispatchQueue.main.asyncAfter with
SidebarDragFailsafePolicy.clearDelay. Instead of scheduling a delayed clear via
DispatchWorkItem, integrate the clearing logic directly into the drag lifecycle
state machine or trigger the onRequestClear callback from a concrete monitor
event that represents the actual end of the drag operation. This eliminates the
timing-based repair path and makes the state transition explicit and predictable
rather than relying on a delayed dispatch that can hide lifecycle races.
---
Outside diff comments:
In `@Sources/ContentView.swift`:
- Around line 15672-15734: The guard statement checking `fromIndex !=
targetIndex` treats a drop as a no-op and returns early without calling
`reorderSidebarWorkspace()`, but this fails to account for group membership
transitions that can occur even when the numeric index remains unchanged. Modify
the no-op guard to also check whether the inferred target group membership
differs from the current group membership by computing the target group for the
drop location and comparing it against the current group, or alternatively move
the no-op decision logic into the coordinator where group membership inference
is authoritative so that membership changes are properly applied regardless of
index position.
🪄 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: f002dc34-2743-4dde-bd12-efaff43cb391
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (39)
Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/SidebarDrop/SidebarDropIndicator.swiftPackages/macOS/CmuxFoundation/Sources/CmuxFoundation/SidebarDrop/SidebarDropPlanner.swiftPackages/macOS/CmuxFoundation/Sources/CmuxFoundation/SidebarDrop/SidebarTabDropIndicatorPredicate.swiftPackages/macOS/CmuxFoundation/Sources/CmuxFoundation/SidebarDrop/SidebarWorkspaceReorderDropAction.swiftPackages/macOS/CmuxFoundation/Sources/CmuxFoundation/SidebarDrop/SidebarWorkspaceReorderDropIndicatorScope.swiftPackages/macOS/CmuxFoundation/Sources/CmuxFoundation/SidebarDrop/SidebarWorkspaceReorderDropPlan.swiftPackages/macOS/CmuxFoundation/Sources/CmuxFoundation/SidebarDrop/SidebarWorkspaceReorderDropRequest.swiftPackages/macOS/CmuxFoundation/Sources/CmuxFoundation/SidebarDrop/SidebarWorkspaceReorderDropResolver.swiftPackages/macOS/CmuxFoundation/Sources/CmuxFoundation/SidebarDrop/SidebarWorkspaceReorderDropTarget.swiftPackages/macOS/CmuxFoundation/Sources/CmuxFoundation/SidebarDrop/SidebarWorkspaceReorderGroupLayout.swiftPackages/macOS/CmuxFoundation/Sources/CmuxFoundation/SidebarDrop/SidebarWorkspaceReorderGroupSnapshot.swiftPackages/macOS/CmuxFoundation/Sources/CmuxFoundation/SidebarDrop/SidebarWorkspaceReorderHitContext.swiftPackages/macOS/CmuxFoundation/Sources/CmuxFoundation/SidebarDrop/SidebarWorkspaceReorderRootTarget.swiftPackages/macOS/CmuxFoundation/Sources/CmuxFoundation/SidebarDrop/SidebarWorkspaceReorderWorkspaceSnapshot.swiftPackages/macOS/CmuxSidebar/Sources/CmuxSidebar/Drag/SidebarDragState.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.swiftSources/ContentView.swiftSources/SidebarWorkspaceDropTargetWriters.swiftSources/SidebarWorkspaceGroupHeaderDropDelegate.swiftSources/SidebarWorkspaceGroupHeaderMetrics.swiftSources/SidebarWorkspaceGroupHeaderView.swiftSources/SidebarWorkspaceReorderDropOverlay.swiftSources/SidebarWorkspaceReorderDropOverlayTarget.swiftSources/SidebarWorkspaceReorderDropOverlayTargetBridge.swiftSources/SidebarWorkspaceReorderDropView.swiftSources/SidebarWorkspaceReorderPendingDrop.swiftSources/SidebarWorkspaceTopDropIndicator.swiftSources/TabManager.swiftSources/VerticalTabsSidebar+WorkspaceGroups.swiftcmux.xcodeproj/project.pbxprojcmuxTests/SidebarTabDropIndicatorPredicateTests.swiftcmuxTests/SidebarWorkspaceDropPlannerTests.swiftcmuxTests/SidebarWorkspaceGroupHeaderMetricsTests.swiftcmuxTests/SidebarWorkspaceReorderDropOverlayHitTestingTests.swiftcmuxTests/WorkspaceGroupTests.swift
💤 Files with no reviewable changes (21)
- Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/SidebarDrop/SidebarWorkspaceReorderGroupSnapshot.swift
- Sources/SidebarWorkspaceTopDropIndicator.swift
- Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/SidebarDrop/SidebarWorkspaceReorderDropAction.swift
- Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/SidebarDrop/SidebarWorkspaceReorderDropPlan.swift
- Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/SidebarDrop/SidebarWorkspaceReorderDropRequest.swift
- Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/SidebarDrop/SidebarWorkspaceReorderWorkspaceSnapshot.swift
- Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/SidebarDrop/SidebarWorkspaceReorderDropTarget.swift
- Sources/SidebarWorkspaceReorderPendingDrop.swift
- Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/SidebarDrop/SidebarWorkspaceReorderGroupLayout.swift
- Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/SidebarDrop/SidebarWorkspaceReorderDropResolver.swift
- Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/SidebarDrop/SidebarWorkspaceReorderHitContext.swift
- Sources/SidebarWorkspaceReorderDropOverlay.swift
- Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/SidebarDrop/SidebarWorkspaceReorderDropIndicatorScope.swift
- Sources/SidebarWorkspaceReorderDropView.swift
- Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/SidebarDrop/SidebarWorkspaceReorderRootTarget.swift
- cmuxTests/SidebarWorkspaceGroupHeaderMetricsTests.swift
- Sources/SidebarWorkspaceReorderDropOverlayTarget.swift
- Sources/SidebarWorkspaceReorderDropOverlayTargetBridge.swift
- cmuxTests/SidebarWorkspaceReorderDropOverlayHitTestingTests.swift
- Sources/SidebarWorkspaceDropTargetWriters.swift
- Sources/SidebarWorkspaceGroupHeaderMetrics.swift
Comment on lines
+18
to
+24
| guard indicator.edge == .bottom, | ||
| let currentIndex = tabIds.firstIndex(of: tabId), | ||
| currentIndex > 0 | ||
| else { | ||
| return false | ||
| } | ||
| return true | ||
| return tabIds[currentIndex - 1] == indicator.tabId |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Avoid the per-row full scan in the drop-indicator predicate.
Line 19 runs tabIds.firstIndex(of:) from a predicate called by each sidebar row/header render, so a bottom-edge drag indicator turns the render pass into O(n²) over workspace rows. Pass a precomputed predecessor/index lookup from the render context and make this predicate O(1). As per path instructions, “In SwiftUI/body and drag-drop hit-testing/rendering paths, avoid repeated full scans over scalable collections.”
🤖 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/SidebarTabDropIndicatorPredicate.swift`
around lines 18 - 24, The predicate containing the guard statement that checks
`tabIds.firstIndex(of: tabId)` performs a full linear scan over the tabIds
collection for every sidebar row render, causing O(n²) performance. Instead of
scanning within the predicate, modify the method signature to accept a
precomputed index lookup (such as a dictionary mapping tabId to its index
position) as a parameter from the render context. Replace the
`tabIds.firstIndex(of: tabId)` call with an O(1) lookup from this precomputed
dictionary, and update the call sites to pass this lookup when invoking the
predicate.
Source: Path instructions
Comment on lines
+274
to
283
| guard fromIndex != clampedTarget else { return false } | ||
|
|
||
| var desiredTopLevelIds = topLevelIds | ||
| if fromIndex != clampedTarget { | ||
| let movedId = desiredTopLevelIds.remove(at: fromIndex) | ||
| desiredTopLevelIds.insert(movedId, at: clampedTarget) | ||
| } | ||
| if shouldPromoteGroupedWorkspace { | ||
| let movedId = desiredTopLevelIds.remove(at: fromIndex) | ||
| desiredTopLevelIds.insert(movedId, at: clampedTarget) | ||
| if promotesGroupedWorkspace, | ||
| let tab = model.tabs.first(where: { $0.id == tabId }), | ||
| tab.groupId != nil, | ||
| !model.isWorkspaceGroupAnchor(tabId) { | ||
| model.assignGroup(workspaceId: tabId, groupId: nil) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Allow promotion even when the top-level index is unchanged.
For grouped children, sidebarTopLevelWorkspaceIds(promotingWorkspaceId:) can synthesize the promoted row at the same fromIndex; the required change is still clearing groupId. This guard returns before line 283, so dropping a child into its immediate top-level slot is ignored instead of promoting it.
Proposed fix
- guard fromIndex != clampedTarget else { return false }
+ let shouldPromoteGroupedWorkspace = promotesGroupedWorkspace
+ && model.tabs.contains { tab in
+ tab.id == tabId &&
+ tab.groupId != nil &&
+ !model.isWorkspaceGroupAnchor(tabId)
+ }
+ guard fromIndex != clampedTarget || shouldPromoteGroupedWorkspace else { return false }
var desiredTopLevelIds = topLevelIds
- let movedId = desiredTopLevelIds.remove(at: fromIndex)
- desiredTopLevelIds.insert(movedId, at: clampedTarget)
- if promotesGroupedWorkspace,
- let tab = model.tabs.first(where: { $0.id == tabId }),
- tab.groupId != nil,
- !model.isWorkspaceGroupAnchor(tabId) {
+ if fromIndex != clampedTarget {
+ let movedId = desiredTopLevelIds.remove(at: fromIndex)
+ desiredTopLevelIds.insert(movedId, at: clampedTarget)
+ }
+ if shouldPromoteGroupedWorkspace {
model.assignGroup(workspaceId: tabId, groupId: 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/CmuxWorkspaces/Sources/CmuxWorkspaces/Coordinators/WorkspaceReorderCoordinator.swift`
around lines 274 - 283, The guard statement at the beginning of the method
prevents execution from reaching the model.assignGroup call when fromIndex
equals clampedTarget. For grouped workspaces being promoted, the function should
still clear the groupId even when the top-level indices are unchanged, since
sidebarTopLevelWorkspaceIds can synthesize the promoted row at the same index.
Modify the guard condition to allow the function to continue when
promotesGroupedWorkspace is true and fromIndex equals clampedTarget, so the
groupId clearing logic in model.assignGroup still executes while skipping the
unnecessary topLevelIds reordering.
Comment on lines
+97
to
+115
| func sidebarTopLevelPinnedWorkspaceIds() -> Set<UUID> { | ||
| let groupsByAnchorId = Dictionary(uniqueKeysWithValues: workspaceGroups.map { ($0.anchorWorkspaceId, $0) }) | ||
| let tabsById = Dictionary(uniqueKeysWithValues: tabs.map { ($0.id, $0) }) | ||
| return Set(sidebarTopLevelWorkspaceIds(promotingWorkspaceId: promotingWorkspaceId).filter { id in | ||
| topLevelWorkspaceIdIsPinned(id, tabsById: tabsById, groupsByAnchorId: groupsByAnchorId) | ||
| }) | ||
| } | ||
|
|
||
| private func promotedTopLevelInsertionIndex( | ||
| ids: [UUID], | ||
| groupIndex: Int, | ||
| promotedIsPinned: Bool, | ||
| tabsById: [UUID: Tab], | ||
| groupsByAnchorId: [UUID: WorkspaceGroup] | ||
| ) -> Int { | ||
| let desiredIndex = min(groupIndex + 1, ids.count) | ||
| let pinnedCount = ids.reduce(into: 0) { count, id in | ||
| if topLevelWorkspaceIdIsPinned(id, tabsById: tabsById, groupsByAnchorId: groupsByAnchorId) { | ||
| count += 1 | ||
| return Set(sidebarTopLevelWorkspaceIds().filter { id in | ||
| if let group = groupsByAnchorId[id] { | ||
| return group.isPinned | ||
| } | ||
| } | ||
| return promotedIsPinned ? min(desiredIndex, pinnedCount) : max(desiredIndex, pinnedCount) | ||
| } | ||
|
|
||
| private func topLevelWorkspaceIdIsPinned( | ||
| _ id: UUID, | ||
| tabsById: [UUID: Tab], | ||
| groupsByAnchorId: [UUID: WorkspaceGroup] | ||
| ) -> Bool { | ||
| if let group = groupsByAnchorId[id] { | ||
| return group.isPinned | ||
| } | ||
| return tabsById[id]?.isPinned == true | ||
| return tabsById[id]?.isPinned == true | ||
| }) | ||
| } | ||
|
|
||
| /// Clamps a requested top-level reorder index into the mover's pin tier. | ||
| func clampedTopLevelReorderIndex( | ||
| forWorkspaceId workspaceId: UUID, | ||
| targetIndex: Int, | ||
| topLevelIds: [UUID], | ||
| promotingWorkspaceId: UUID? = nil | ||
| topLevelIds: [UUID] | ||
| ) -> Int { | ||
| let clamped = max(0, min(targetIndex, max(0, topLevelIds.count - 1))) | ||
| let pinnedIds = sidebarTopLevelPinnedWorkspaceIds(promotingWorkspaceId: promotingWorkspaceId) | ||
| let pinnedIds = sidebarTopLevelPinnedWorkspaceIds() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve the promoted child’s pin tier.
When topLevelIds includes a promoted grouped child, sidebarTopLevelPinnedWorkspaceIds() recomputes pinned rows from the non-promoted top-level projection, so the promoted child is never in pinnedIds. A pinned child promoted out of a group is then clamped/planned as unpinned and can land below the pinned boundary.
Proposed fix
- func sidebarTopLevelPinnedWorkspaceIds() -> Set<UUID> {
+ func sidebarTopLevelPinnedWorkspaceIds(promotingWorkspaceId promotedWorkspaceId: UUID? = nil) -> Set<UUID> {
let groupsByAnchorId = Dictionary(uniqueKeysWithValues: workspaceGroups.map { ($0.anchorWorkspaceId, $0) })
let tabsById = Dictionary(uniqueKeysWithValues: tabs.map { ($0.id, $0) })
- return Set(sidebarTopLevelWorkspaceIds().filter { id in
+ var pinnedIds = Set(sidebarTopLevelWorkspaceIds(promotingWorkspaceId: promotedWorkspaceId).filter { id in
if let group = groupsByAnchorId[id] {
return group.isPinned
}
return tabsById[id]?.isPinned == true
- })
+ })
+ if let promotedWorkspaceId,
+ tabsById[promotedWorkspaceId]?.isPinned == true {
+ pinnedIds.insert(promotedWorkspaceId)
+ }
+ return pinnedIds
}
/// Clamps a requested top-level reorder index into the mover's pin tier.
@@
- let pinnedIds = sidebarTopLevelPinnedWorkspaceIds()
+ let pinnedIds = sidebarTopLevelPinnedWorkspaceIds(promotingWorkspaceId: workspaceId)Also pass the dragged workspace from the coordinator’s top-level planning path:
- return model.sidebarTopLevelPinnedWorkspaceIds()
+ return model.sidebarTopLevelPinnedWorkspaceIds(promotingWorkspaceId: draggedWorkspaceId)As per path instructions, correctness-critical UI state must have one authoritative source of truth rather than an unreliable fallback; here the authoritative pin state is the workspace/group model state, including the promoted workspace.
🤖 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 97 - 115, The function clampedTopLevelReorderIndex relies on
sidebarTopLevelPinnedWorkspaceIds which recomputes the pinned set from the
non-promoted top-level view, causing promoted children to not be recognized as
pinned. To fix this, modify clampedTopLevelReorderIndex to accept the dragged
workspace ID as a parameter (the workspace being moved), and when determining
pin tier constraints, check the authoritative pin state of that workspace
directly from the model state (checking both workspace and group isPinned
properties) rather than relying solely on the computed pinnedIds set. This
ensures the promoted child's pin state is preserved during reordering.
Source: Path instructions
| ) | ||
|
|
||
| let group = try #require(model.workspaceGroups.first(where: { $0.id == groupId })) | ||
| let group = try! #require(model.workspaceGroups.first(where: { $0.id == groupId })) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove try! in tests and restore throwing test signatures.
try! #require(...) at Line 213, Line 244, Line 263, Line 278, and Line 317 introduces forced-crash paths and triggers the current lint errors. Prefer throws test methods with try #require(...) so failures are reported as assertions instead of hard crashes.
Suggested fix pattern
- func createWorkspaceGroupAdoptsChildrenAndKeepsSectionContiguous() {
+ func createWorkspaceGroupAdoptsChildrenAndKeepsSectionContiguous() throws {
...
- let group = try! `#require`(model.workspaceGroups.first(where: { $0.id == groupId }))
+ let group = try `#require`(model.workspaceGroups.first(where: { $0.id == groupId }))Apply the same pattern to the other changed tests at Lines 239, 258, 274, and 311.
Also applies to: 244-244, 263-263, 278-278, 317-317
🧰 Tools
🪛 SwiftLint (0.64.0)
[Error] 213-213: Force tries should be avoided
(force_try)
🤖 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/WorkspaceCoordinatorTests.swift`
at line 213, The test methods contain multiple instances of try! `#require`(...)
at lines 213, 244, 263, 278, and 317 which cause forced crashes instead of
proper test failure assertions. For each test method containing this pattern,
add throws to the test method signature and replace try! `#require`(...) with try
`#require`(...) so that requirement failures are properly reported as test
assertions rather than hard crashes.
Source: Linters/SAST tools
Comment on lines
+10620
to
+10635
| .overlay(alignment: .top) { | ||
| if dragState.draggedTabId != nil, let firstWorkspaceId = renderContext.workspaceIds.first { | ||
| Color.clear | ||
| .contentShape(Rectangle()) | ||
| .frame(height: scrollInsets.top + 8) | ||
| .onDrop(of: SidebarTabDragPayload.dropContentTypes, delegate: SidebarTabDropDelegate( | ||
| targetTabId: firstWorkspaceId, | ||
| tabManager: tabManager, | ||
| workspaceGroupIdByWorkspaceId: renderContext.workspaceGroupIdByWorkspaceId, | ||
| dragState: dragState, | ||
| selectedTabIds: $selectedTabIds, | ||
| lastSidebarSelectionIndex: $lastSidebarSelectionIndex, | ||
| targetRowHeight: nil, | ||
| dragAutoScrollController: dragAutoScrollController | ||
| )) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep the top drop zone mounted for foreign drags.
Gating this overlay on dragState.draggedTabId != nil means a cross-window drag that first enters the top strip has no delegate mounted to call activateForeignDragIfNeeded(), so dropping above the first workspace can fail until the pointer reaches a row. Mount the drop zone whenever firstWorkspaceId exists and let the delegate validate the payload.
🐛 Proposed fix
- if dragState.draggedTabId != nil, let firstWorkspaceId = renderContext.workspaceIds.first {
+ if let firstWorkspaceId = renderContext.workspaceIds.first {📝 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
| .overlay(alignment: .top) { | |
| if dragState.draggedTabId != nil, let firstWorkspaceId = renderContext.workspaceIds.first { | |
| Color.clear | |
| .contentShape(Rectangle()) | |
| .frame(height: scrollInsets.top + 8) | |
| .onDrop(of: SidebarTabDragPayload.dropContentTypes, delegate: SidebarTabDropDelegate( | |
| targetTabId: firstWorkspaceId, | |
| tabManager: tabManager, | |
| workspaceGroupIdByWorkspaceId: renderContext.workspaceGroupIdByWorkspaceId, | |
| dragState: dragState, | |
| selectedTabIds: $selectedTabIds, | |
| lastSidebarSelectionIndex: $lastSidebarSelectionIndex, | |
| targetRowHeight: nil, | |
| dragAutoScrollController: dragAutoScrollController | |
| )) | |
| } | |
| .overlay(alignment: .top) { | |
| if let firstWorkspaceId = renderContext.workspaceIds.first { | |
| Color.clear | |
| .contentShape(Rectangle()) | |
| .frame(height: scrollInsets.top + 8) | |
| .onDrop(of: SidebarTabDragPayload.dropContentTypes, delegate: SidebarTabDropDelegate( | |
| targetTabId: firstWorkspaceId, | |
| tabManager: tabManager, | |
| workspaceGroupIdByWorkspaceId: renderContext.workspaceGroupIdByWorkspaceId, | |
| dragState: dragState, | |
| selectedTabIds: $selectedTabIds, | |
| lastSidebarSelectionIndex: $lastSidebarSelectionIndex, | |
| targetRowHeight: nil, | |
| dragAutoScrollController: dragAutoScrollController | |
| )) | |
| } | |
| } |
🤖 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/ContentView.swift` around lines 10620 - 10635, The top drop zone
overlay is gated on both dragState.draggedTabId != nil and firstWorkspaceId
existing, which prevents the SidebarTabDropDelegate from mounting during foreign
(cross-window) drags. Remove the dragState.draggedTabId != nil check from the if
condition so the overlay remains mounted whenever firstWorkspaceId exists,
allowing the delegate to properly validate the payload and handle cross-window
drags through activateForeignDragIfNeeded().
Comment on lines
12308
to
+12321
| private func requestClearSoon(reason: String) { | ||
| guard pendingClearTimer == nil else { return } | ||
| guard pendingClearWorkItem == nil else { return } | ||
| #if DEBUG | ||
| cmuxDebugLog("sidebar.dragFailsafe.schedule reason=\(reason)") | ||
| #endif | ||
| let timer = DispatchSource.makeTimerSource(queue: .main) | ||
| pendingClearGeneration &+= 1 | ||
| let generation = pendingClearGeneration | ||
| timer.schedule(deadline: .now() + SidebarDragFailsafePolicy.clearDelay) | ||
| timer.setEventHandler { [weak self] in | ||
| Task { @MainActor [weak self] in | ||
| guard let self, self.pendingClearGeneration == generation else { return } | ||
| let workItem = DispatchWorkItem { [weak self] in | ||
| #if DEBUG | ||
| cmuxDebugLog("sidebar.dragFailsafe.fire reason=\(reason)") | ||
| cmuxDebugLog("sidebar.dragFailsafe.fire reason=\(reason)") | ||
| #endif | ||
| self.pendingClearTimer = nil | ||
| self.onRequestClear?(reason) | ||
| } | ||
| self?.pendingClearWorkItem = nil | ||
| self?.onRequestClear?(reason) | ||
| } | ||
| pendingClearTimer = timer | ||
| timer.resume() | ||
| pendingClearWorkItem = workItem | ||
| DispatchQueue.main.asyncAfter(deadline: .now() + SidebarDragFailsafePolicy.clearDelay, execute: workItem) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Replace the delayed failsafe with an explicit drag-state transition.
DispatchQueue.main.asyncAfter adds a timing repair path in production UI/drag state code, which can hide lifecycle races and create delayed stale-state clears. Clear from the concrete monitor event, or move the transition into the drag lifecycle owner instead of relying on a delay. As per coding guidelines, “Flag DispatchQueue.asyncAfter ... in shipped app/runtime Swift code as failures by default.”
🐛 Proposed direction
private func requestClearSoon(reason: String) {
- guard pendingClearWorkItem == nil else { return }
`#if` DEBUG
- cmuxDebugLog("sidebar.dragFailsafe.schedule reason=\(reason)")
+ cmuxDebugLog("sidebar.dragFailsafe.clear reason=\(reason)")
`#endif`
- let workItem = DispatchWorkItem { [weak self] in
-#if DEBUG
- cmuxDebugLog("sidebar.dragFailsafe.fire reason=\(reason)")
-#endif
- self?.pendingClearWorkItem = nil
- self?.onRequestClear?(reason)
- }
- pendingClearWorkItem = workItem
- DispatchQueue.main.asyncAfter(deadline: .now() + SidebarDragFailsafePolicy.clearDelay, execute: workItem)
+ onRequestClear?(reason)
}🤖 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/ContentView.swift` around lines 12308 - 12321, Remove the delayed
failsafe mechanism in the requestClearSoon function that uses
DispatchQueue.main.asyncAfter with SidebarDragFailsafePolicy.clearDelay. Instead
of scheduling a delayed clear via DispatchWorkItem, integrate the clearing logic
directly into the drag lifecycle state machine or trigger the onRequestClear
callback from a concrete monitor event that represents the actual end of the
drag operation. This eliminates the timing-based repair path and makes the state
transition explicit and predictable rather than relying on a delayed dispatch
that can hide lifecycle races.
Source: Coding guidelines
azooz2003-bit
added a commit
that referenced
this pull request
Jun 27, 2026
* Reapply "Fix workspace group drag drop intent (#6532)" (#6713) This reverts commit f24eeb8. * test: exercise top-level scope explicitly in group bottom-indicator assertion Address CodeRabbit: pass indicatorScope: .topLevel so the assertion validates the top-level branch instead of relying on the default .raw value. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * ci: bump WorkspaceGroupTests.swift length budget 1030->1031 The indicatorScope: .topLevel test assertion fix added one line, pushing the file one over budget. Accept the +1 for the test-correctness improvement. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
2 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.
Reverts #6532.
Why
#6532 broke
mainCI. It was the only commit between the last all-green run and two consecutive red runs:18e4d64a(14:09) — all 4 app-host shards passed clean.5cf0557e, 22:34): app-host crashes on every run.5cf0557e→ ❌ app-host unit tests (shard 1/4)96221935(HEAD) → ❌ app-host unit tests (shard 4/4)Crash
The app-host segfaults during UI-rendering tests:
The crash is nondeterministic (hit shard 1/4 in one run, 4/4 in the next), so it takes down a different batch of otherwise-unrelated tests each run (
CLINotifyProcessIntegrationRegressionTests, browser, keyboard-settings) — which is why it first looked flaky. The reentrant-layout-then-segfault points at the new sidebar drop-overlay rendering (SidebarWorkspaceReorderDropOverlay, +663 lines inContentView.swift).Reverting to unblock
main. @azooz2003-bit can re-land #6532 with the reentrant SwiftUI layout fixed (likely a state write duringbody/layout in the new drop overlay — see the snapshot-boundary / no-state-mutation-in-body rules in CLAUDE.md).🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Large behavioral rollback in sidebar reorder/group drag UX with substantial deleted code; risk is regressing #6532 fixes while fixing CI crashes, not new security surface.
Overview
Reverts #6532 to restore stable sidebar drag/drop and unblock app-host CI after reentrant SwiftUI layout / segfaults tied to the new overlay.
Removed: The
SidebarWorkspaceReorderDropResolverstack in CmuxFoundation (hit testing, group/top-level scopes, cross-window plans),dropIndicatorScopeonSidebarDragState,explicitGroupIdreorder paths, bottom-edge drop lines, and the AppKitSidebarWorkspaceReorderDropOverlaypath inContentView(frame collection bridge, overlay-driven commit). Related coordinator tests for explicit group drops go with it.Restored: Per-row
SidebarTabDropDelegate/ SwiftUIonDrop, simplerSidebarDropPlannerand indicator predicates, and reorder logic without scoped overlay planning.Kept from the reverted work:
SidebarWorkspaceGroupHeaderDropDelegate(center drop on a group header adds a workspace to the group; edges still reorder), plusWorkspaceGroupCoordinatorfixes that expand a collapsed group when the focused workspace is added or created inside it.Reviewed by Cursor Bugbot for commit a8d37a2. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Reverts the workspace group drag/drop intent from #6532 to stop app-host segfaults and unblock main CI. Restores the previous sidebar reorder behavior and removes the SwiftUI drop overlay that triggered reentrant layout.
CmuxFoundationand app code (SidebarWorkspaceReorderDropOverlay, resolver/types), eliminating reentrant SwiftUI layout that crashed tests.WorkspaceReorderCoordinatorandTabManager(no explicit group-scoped drag path); restored legal insertion range API shape.SidebarWorkspaceGroupHeaderDropDelegateto handle center-drops on a group header to add to that group; auto-expands a collapsed group when creating and selecting a new workspace.Written for commit a8d37a2. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Refactor