Skip to content

Animate sidebar workspace reordering - #5034

Closed
lawrencecchen wants to merge 10 commits into
mainfrom
task-sidebar-reordering-animations
Closed

lawrencecchen wants to merge 10 commits into
mainfrom
task-sidebar-reordering-animations

Conversation

@lawrencecchen

@lawrencecchen lawrencecchen commented May 31, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Adds a drag-hover render preview so sidebar rows animate into the pending reorder position before drop.
  • Keeps expanded groups moving as a block in top-level reorder scope while preserving flat member reorders inside a group.
  • Adds coverage for expanded groups, collapsed groups, promoted grouped members, and no-op preview inputs.

Testing

  • git diff --check
  • ./scripts/reload.sh --tag sidebar-anim was attempted locally, but the build was interrupted after a long Xcode LLVM codegen stall in an unrelated large Swift batch; no valid tagged build was produced.

Issues

  • Task: implement animations for sidebar reordering, including groups and workspaces.

View with Codesmith Autofix with Codesmith
Need help on this PR? Tag @codesmith with what you need. Autofix is disabled.


Note

Medium Risk
Large UI interaction changes in sidebar drag/drop and autoscroll with performance-sensitive @observable invalidation; mitigated by tests and guards around preview/drop state, but regressions in grouping or drop targets are possible.

Overview
Sidebar workspace drag-and-drop now previews the final list order while hovering, with snappy animations on row moves and top insertion indicators, instead of only updating on drop.

Reorder preview builds a temporary render list via SidebarWorkspaceRenderItem.dragPreviewItems, including group-center “add to group” previews (groupDropPreview) separate from edge insertion indicators. Expanded groups can move as a single top-level block; in-group reorders stay flat; dragging a member out can show promoted placement via effectiveGroupId.

Drag visuals hide the system drag image (SidebarWorkspaceInvisibleDragPreview), nearly hide the source row, and show a cursor-following overlay sized from a one-time frame snapshot plus live document Y from SidebarDragAutoScrollController (local mouse monitor + scroll tracking).

Drop stability ignores self-target hovers, avoids clearing indicators when the dragged row passes under the pointer during animated preview, and can commit add-to-group from a group-center preview on drop.

Tests in WorkspaceGroupTests cover block moves, in-group reorder, collapsed headers, promotion, and missing-indicator no-ops.

Reviewed by Cursor Bugbot for commit 618ba98. Bugbot is set up for automated code reviews on this repo. Configure here.


Summary by cubic

Adds live drag-hover reorder previews with snappy list animations and a cursor-following drag row that stays pinned through autoscroll. Keeps insertion indicators and group-center join previews stable so layouts don’t snap while items move under the pointer.

  • New Features

    • Cursor-following drag overlay with snapshot sizing; uses a 1×1 invisible drag preview to hide the system image and keep spacing stable during autoscroll.
    • Reorder preview via SidebarWorkspaceRenderItem.dragPreviewItems: expanded groups move as a block at top level, in-group reorders stay flat, collapsed headers behave as single rows, and members can be promoted out of a group; indentation tracked with effectiveGroupId.
    • Animated top insertion indicator and list transitions using snappy timing; drag follower renders non-interactively above the list.
  • Bug Fixes

    • Preserves insertion state when the dragged row moves under the pointer and ignores self-target hovers; keeps previews across row exits.
    • Allows committing add-to-group directly from a group-center preview; separates group-center previews from edge indicators.
    • Continuous local cursor tracking keeps the drag follower visible and aligned during native and manual autoscroll.

Written for commit 618ba98. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • New Features
    • Group-aware sidebar drag-and-drop with explicit group drop previews, promoted-preview handling, cursor-driven auto-scrolling during drag, and snappy show/hide animations for indicators.
    • Invisible drag previews for cleaner visuals and consistent row spacing; improved indentation for effective group placement.
  • Bug Fixes
    • Prevented conflicting/no-op hover updates and unwanted indicator clearing while dragging.
  • Tests
    • Added drag-preview and reorder-preview tests covering grouped, collapsed, and promoted-preview scenarios.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@vercel

vercel Bot commented May 31, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
cmux Ready Ready Preview, Comment Jun 2, 2026 12:10am
cmux-staging Building Building Preview, Comment Jun 2, 2026 12:10am

@coderabbitai

coderabbitai Bot commented May 31, 2026 •

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds group-aware drag-preview computation and integrates it into sidebar rendering: drag state now supports group drop previews and cursor-based autoscroll, preview item lists are computed and animated, invisible drag previews are used for OS visuals, and tests validate preview behaviors.

Changes

Workspace Drag/Drop Preview and Animation

Layer / File(s) Summary
Drag-preview computation and render-item model
Sources/SidebarWorkspaceRenderItem.swift
Adds SidebarWorkspaceGroupDropPreview, makes .workspace carry effectiveGroupId, introduces representedWorkspaceId, withEffectiveGroupId, and dragPreviewItems plus helpers dragPreviewWorkspaceIds and dragPreviewBlocks to compute reordered/grouped preview lists.
Sidebar integration, autoscroll, rows, and drop handling
Sources/ContentView.swift
Adds groupDropPreview and dragLocationInDocument to SidebarDragState, setGroupDropPreview(...), refactors top-edge indicator to opacity+scale, passes preview renderItems to VerticalTabsSidebar.workspaceRows, animates LazyVStack by render IDs, refines workspaceRow role/indentation, updates .onDrag(..., preview:) usage and dragged opacity, enhances SidebarDragAutoScrollController to attach to dragState and record drag location, and updates SidebarTabDropDelegate perform/update/exit logic to handle group previews.
Group header drag handling and invisible preview
Sources/SidebarWorkspaceGroupHeaderView.swift, Sources/ContentView.swift
Adds SidebarWorkspaceInvisibleDragPreview, uses explicit invisible preview closures for header/row/tile drags, adjusts header opacity when dragged, conditions dropExited cleanup on matching groupDropPreview, and routes .addWorkspaceToGroup through tabManager.addWorkspaceToGroup when group preview matches.
Header role wrapping and drag-follower overlay
Sources/VerticalTabsSidebar+WorkspaceGroups.swift, Sources/ContentView.swift
Extends sidebarWorkspaceGroupHeader with a role parameter, records drag location on drag start, tightens isBeingDragged for list role only, conditionally wraps headers for .dragFollower to disable hit-testing/accessibility and reduce preference/anchor wiring, and adds SidebarWorkspaceDragFollowerOverlay.
Drag-preview test cases and helpers
cmuxTests/WorkspaceGroupTests.swift
Adds helpers to build render items and extract represented workspace IDs; new tests assert drag-preview outputs for expanded/collapsed groups, block-level reorders, member promotion without sibling moves, and passthrough when dropIndicator is nil.

Sequence Diagram(s)

sequenceDiagram
  participant DragState
  participant VerticalTabsSidebar
  participant SidebarWorkspaceRenderItem
  participant TabManager
  participant SidebarEmptyArea
  DragState->>VerticalTabsSidebar: provide draggedWorkspaceId, dropIndicator, reorderIds, groupDropPreview
  VerticalTabsSidebar->>SidebarWorkspaceRenderItem: dragPreviewItems(items, draggedId, dropIndicator, reorderIds, groupDropPreview)
  SidebarWorkspaceRenderItem-->>VerticalTabsSidebar: preview renderItems
  VerticalTabsSidebar->>VerticalTabsSidebar: LazyVStack animation keyed by renderItems.ids
  VerticalTabsSidebar->>SidebarEmptyArea: SidebarWorkspaceTopDropIndicator visibility update
  VerticalTabsSidebar->>TabManager: performDrop / addWorkspaceToGroup
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • Ari4ka

Poem

🐇 I nudge a tab, it hops, a quiet show,
A ghostly preview guides where it should go,
Indicators breathe, then scale away with grace,
Groups open arms to welcome a workspace,
— a rabbit cheers as rows find their place.


Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

  • Ignore

❌ Failed checks (4 errors, 1 warning)

Check name Status Explanation Resolution
Cmux Swift Actor Isolation ❌ Error New struct SidebarWorkspaceGroupDropPreview is a pure Equatable value model lacking nonisolated isolation attribute required per swift-actor-isolation.md rules for data models. Mark the struct as: nonisolated struct SidebarWorkspaceGroupDropPreview: Equatable { let draggedWorkspaceId: UUID; let targetGroupId: UUID }
Cmux Algorithmic Complexity ❌ Error Two O(n) scans in hot paths: line 11894 rescans tabManager.tabs.firstIndex when renderContext.tabIndexById is in scope; line 12156 rescans renderItems.first(where:) on every mouse move during drag. Use renderContext.tabIndexById[workspaceId] at 11894. Precompute [UUID:Item] dict and pass to SidebarWorkspaceDragFollowerOverlay to avoid 12156 rescan.
Cmux Swift @Concurrent ❌ Error FeedbackComposerClient.submit() performs network/file I/O without @concurrent annotation, called from MainActor-bound View without explicit actor hop. Add @concurrent to FeedbackComposerClient.submit or move work to background actor before calling from submitFeedback().
Cmux Swift File And Package Boundaries ❌ Error ContentView.swift: +276 lines added to file already at 18,068 lines (budget: 15,956) exceeds >250 line threshold for oversized files; no exemption applies (no 200+ line extraction). Extract drag preview logic (SidebarWorkspaceDragFollowerOverlay and related drag state/autoscroll updates) into a new SwiftPM package or separate the ~200+ lines of drag animation features into a focused file.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (13 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change—animating sidebar workspace reordering—which aligns with the core objective of adding live drag-hover previews.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Cmux Swift Blocking Runtime ✅ Passed PR introduces drag-reorder animation using SwiftUI Animation.snappy() with .animation modifier, not blocking synchronization. Pre-existing Task.sleep in workspace handoff is unrelated to this feature.
Cmux No Hacky Sleeps ✅ Passed PR modifies only Swift files; check applies only to non-Swift production code (TypeScript, JavaScript, shell, build/runtime scripts). Swift sleeps are covered by separate check.
Cmux Swift Concurrency ✅ Passed No new legacy async patterns. Timer callback Task{@MainActor} correctly bridges AppKit boundary; NSItemProvider.registerDataRepresentation follows existing pattern.
Cmux Swift Logging ✅ Passed No logging violations found; the PR adds drag/drop functionality without introducing print, debugPrint, dump, NSLog, or improper Logger declarations in production code.
Cmux User-Facing Error Privacy ✅ Passed PR contains only UI drag-drop logic changes with no user-facing error messages, alerts, or sensitive data exposure. All localized strings use proper String(localized:) pattern.
Cmux Full Internationalization ✅ Passed PR adds 3 new localized strings to SidebarWorkspaceGroupHeaderView.swift with full translations for 17+ locales; no unlocalized user-facing text was introduced in sidebar drag-preview changes.
Cmux Swiftui State Layout ✅ Passed SidebarDragState uses @Observable; new struct SidebarWorkspaceGroupDropPreview; LazyVStack rows receive snapshot-based renderContext; no store refs; GeometryReader for overlay measurement.
Cmux Architecture Rethink ✅ Passed PR introduces Timer for AppKit drag-drop autoscroll (required platform bridge), maintains single dragState owner, adds no duplicate entrypoints/observers/singletons, and preserves clear invariants.
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed PR makes no changes to standalone cmux-owned windows; all NSWindow references are to existing windows being monitored or modified via overlays, not new window creation.
Description check ✅ Passed The pull request description covers the summary of changes and testing performed, but is missing the demo video and incomplete checklist items.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch task-sidebar-reordering-animations

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Comment thread Sources/SidebarWorkspaceRenderItem.swift
Comment thread Sources/ContentView.swift
@greptile-apps

greptile-apps Bot commented May 31, 2026 •

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds live drag-hover reorder previews to the sidebar workspace list, animating rows into their pending drop positions before the user releases the mouse. It introduces cursor-following drag chrome, group-aware block moves, and in-group flat reorders.

  • SidebarWorkspaceRenderItem.dragPreviewItems computes the reshuffled item list from the current dropIndicator or groupDropPreview on every hover update, with dragPreviewBlocks grouping expanded-group rows into moveable units and groupDropPreviewItems handling center-hover join previews.
  • SidebarDragState gains groupDropPreview, dragLocationInDocument, and dragFollowerSnapshot; SidebarDragAutoScrollController gains a local NSEvent monitor and a tracking NSView to feed document-space cursor coordinates into dragLocationInDocument at drag frequency.
  • SidebarWorkspaceDragFollowerOverlay renders the cursor-pinned copy of the dragged row by reading dragState.dragLocationInDocument (updated at mouse-drag frequency) to position the row's Y axis while freezing its X from the initial frame snapshot.

Confidence Score: 4/5

Safe to merge pending resolution of the collapsed-group member leak and the sidebar-body broad invalidation flagged in prior reviews, both of which remain unaddressed in this diff.

The collapsed-group groupDropPreviewItems bug (a visible member row rendered below a visually collapsed header) and the dropIndicator/groupDropPreview reads inside VerticalTabsSidebar.body (which re-evaluates the entire sidebar on every hover-target change during drag) were identified in earlier review rounds and are still present in the updated code. A new O(n) scan pair inside SidebarWorkspaceDragFollowerOverlay.body runs at cursor-drag event frequency and is the only new finding, but it does not change the merge safety picture materially.

Sources/SidebarWorkspaceRenderItem.swift (groupDropPreviewItems isCollapsed guard) and Sources/ContentView.swift (dragState property reads in workspaceRows / SidebarWorkspaceDragFollowerOverlay scan)

Important Files Changed

Filename Overview
Sources/ContentView.swift Extends SidebarDragState with groupDropPreview/dragLocationInDocument/dragFollowerSnapshot; adds SidebarWorkspaceDragFollowerOverlay (reads dragLocationInDocument at 60 fps with two O(n) first(where:) scans); dropIndicator still read in sidebar body (prior P1). SidebarDragAutoScrollController gains NSEvent local monitor and cursor tracking.
Sources/SidebarWorkspaceRenderItem.swift Adds dragPreviewItems, groupDropPreviewItems, dragPreviewWorkspaceIds, and dragPreviewBlocks. groupDropPreviewItems still lacks an isCollapsed guard (flagged in prior review). The blockIds O(n²) filter also already flagged.
Sources/SidebarWorkspaceGroupHeaderView.swift Adds SidebarWorkspaceInvisibleDragPreview; group header dropExited now clears groupDropPreview and calls reorderDelegate.dropExited (which returns early during drags – intentional). performDrop uses placement:.end.
Sources/VerticalTabsSidebar+WorkspaceGroups.swift Adds role parameter to sidebarWorkspaceGroupHeader; dragFollower branch strips identity/preferences/hit-testing; .list branch is unchanged.
cmuxTests/WorkspaceGroupTests.swift Adds six new dragPreviewItems tests covering expanded-group block moves, group-center join, flat in-group reorder, collapsed-group header move, member promotion, and no-op missing indicator.

Sequence Diagram

sequenceDiagram
    participant User
    participant NSEventMonitor as NSEvent Local Monitor
    participant AutoScroll as SidebarDragAutoScrollController
    participant DragState as SidebarDragState
    participant SidebarBody as VerticalTabsSidebar.body
    participant Follower as SidebarWorkspaceDragFollowerOverlay
    participant DropDelegate as SidebarTabDropDelegate

    User->>NSEventMonitor: leftMouseDragged
    NSEventMonitor->>AutoScroll: "recordDragLocation() [Task @MainActor]"
    AutoScroll->>DragState: setDragLocationInDocument(point)
    DragState-->>Follower: invalidate (dragLocationInDocument changed)
    Follower->>Follower: body — O(n) first(where:) scan for dragged item
    Follower->>Follower: position overlay at (frame.midX, cursor.y)

    User->>DropDelegate: dropUpdated (new hover row)
    DropDelegate->>DropDelegate: updateDropIndicator()
    DropDelegate->>DragState: setDropIndicator(indicator, usesTopLevelRows)
    DragState-->>SidebarBody: invalidate (dropIndicator changed)
    SidebarBody->>SidebarBody: dragPreviewItems() — recompute reordered list
    SidebarBody->>Follower: new sourceItems / renderItems

    User->>DropDelegate: performDrop
    alt groupDropPreview set
        DropDelegate->>DropDelegate: addWorkspaceToGroup(.end)
    else normal reorder
        DropDelegate->>DropDelegate: sidebarReorderWorkspaceIds(usesTopLevelRows)
        DropDelegate->>DropDelegate: moveWorkspace(from:to:)
    end
    DropDelegate->>DragState: clearDrag()
Loading

Reviews (10): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile

Comment on lines +91 to +92
let blockIds = blocks.map(\.workspaceId)
let reorderIds = reorderWorkspaceIds.filter { blockIds.contains($0) }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 O(n²) array scan on main thread during drag

blockIds is a plain [UUID] array, so every call to blockIds.contains($0) in the filter closure is an O(n) linear scan, making the overall filter O(|reorderWorkspaceIds| × |blockIds|). In the flat drag scope (reorderWorkspaceIds = tabs.map(\.id)), both sides scale with the full workspace count. At ~1000 workspaces this is ~1 000 000 UUID comparisons on the @MainActor on every body refresh where a drag is active. Wrapping blockIds in a Set before the filter reduces this to O(n + m).

Rule Used: Flag production code that adds nested full-collect... (source)

Comment thread Sources/ContentView.swift Outdated
Comment on lines 15163 to 15166
.opacity(isBeingDragged ? 0.6 : 1)
.animation(.snappy(duration: 0.24, extraBounce: 0.02), value: isBeingDragged)
.overlay {
SidebarWorkspaceRowHoverTracker(rowInteractionState: $rowInteractionState)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 The .snappy(duration: 0.24, extraBounce: 0.02) parameters are hardcoded inline here and in SidebarWorkspaceGroupHeaderView instead of sharing VerticalTabsSidebar.workspaceReorderAnimation. Since those row types can't access the private static let, consider moving the constant to a shared home (e.g., a SidebarWorkspaceListMetrics-style enum) so the three sites stay in sync.

Suggested change
.opacity(isBeingDragged ? 0.6 : 1)
.animation(.snappy(duration: 0.24, extraBounce: 0.02), value: isBeingDragged)
.overlay {
SidebarWorkspaceRowHoverTracker(rowInteractionState: $rowInteractionState)
.opacity(isBeingDragged ? 0.6 : 1)
.animation(SidebarWorkspaceListMetrics.reorderAnimation, value: isBeingDragged)
.overlay {
SidebarWorkspaceRowHoverTracker(rowInteractionState: $rowInteractionState)

Rule Used: Flag SwiftUI changes that can cause stale state, b... (source)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/SidebarWorkspaceRenderItem.swift`:
- Around line 91-92: Replace the O(n×m) lookup by building a Set of workspace
IDs and using O(1) membership checks: instead of creating blockIds as an Array
from blocks.map(\.workspaceId) and calling blockIds.contains($0) in the filter,
create a Set (e.g., blockIdSet = Set(blocks.map(\.workspaceId))) and use
blockIdSet.contains($0) when computing reorderIds (reorderWorkspaceIds.filter {
... }); update any variable names (blockIds → blockIdSet) to reflect the change.
🪄 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: b235f4cb-b273-459b-8760-c7f7721d21b8

📥 Commits

Reviewing files that changed from the base of the PR and between 6154ad3 and 1ca43f1.

📒 Files selected for processing (4)
  • Sources/ContentView.swift
  • Sources/SidebarWorkspaceGroupHeaderView.swift
  • Sources/SidebarWorkspaceRenderItem.swift
  • cmuxTests/WorkspaceGroupTests.swift

Comment on lines +91 to +92
let blockIds = blocks.map(\.workspaceId)
let reorderIds = reorderWorkspaceIds.filter { blockIds.contains($0) }

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Use Set for O(1) lookup instead of Array.contains.

blockIds.contains($0) is O(m) per call, making the filter O(n×m). This runs on the drag-hover path which fires frequently during drag operations.

♻️ Proposed fix
-        let blockIds = blocks.map(\.workspaceId)
-        let reorderIds = reorderWorkspaceIds.filter { blockIds.contains($0) }
+        let blockIdSet = Set(blocks.map(\.workspaceId))
+        let reorderIds = reorderWorkspaceIds.filter { blockIdSet.contains($0) }

As per coding guidelines: algorithmic-complexity.md flags "nested full-collection scans... in hot UI/socket/search/process paths" for "paths expected to handle roughly 1000 workspaces."

🤖 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/SidebarWorkspaceRenderItem.swift` around lines 91 - 92, Replace the
O(n×m) lookup by building a Set of workspace IDs and using O(1) membership
checks: instead of creating blockIds as an Array from blocks.map(\.workspaceId)
and calling blockIds.contains($0) in the filter, create a Set (e.g., blockIdSet
= Set(blocks.map(\.workspaceId))) and use blockIdSet.contains($0) when computing
reorderIds (reorderWorkspaceIds.filter { ... }); update any variable names
(blockIds → blockIdSet) to reflect the change.

Comment thread Sources/SidebarWorkspaceRenderItem.swift

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/SidebarWorkspaceRenderItem.swift`:
- Around line 155-165: The preview logic is creating synthetic reorders and
visible child rows even when the drop would be a no-op or the group is
collapsed; update the .groupHeader and .workspace handling so you only modify
the group's member list and append a visible child row when necessary: in the
.groupHeader branch (case .groupHeader(let group, let memberWorkspaceIds)) only
compute and use nextMemberWorkspaceIds if memberWorkspaceIds does not already
contain preview.draggedWorkspaceId, and set targetHeaderIndex/result.append only
when a real change occurs; in the .workspace branch (case .workspace(let
workspace, _)) avoid calling item.withEffectiveGroupId(preview.targetGroupId) if
workspace.id already belongs to preview.targetGroupId; additionally, when
deciding to append a child row under the header, check the group's expanded
state (e.g., group.isExpanded / !group.isCollapsed) and only insert the visible
child when the target group is expanded. Ensure you reference
nextMemberWorkspaceIds, targetHeaderIndex, result.append(.groupHeader(...)), and
withEffectiveGroupId to locate the spots to change.
🪄 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: 4e436485-b465-4c36-b28c-3dd6fbd0361d

📥 Commits

Reviewing files that changed from the base of the PR and between 1ca43f1 and 8a7e460.

📒 Files selected for processing (4)
  • Sources/ContentView.swift
  • Sources/SidebarWorkspaceGroupHeaderView.swift
  • Sources/SidebarWorkspaceRenderItem.swift
  • cmuxTests/WorkspaceGroupTests.swift

Comment on lines +155 to +165
case .groupHeader(let group, let memberWorkspaceIds)
where group.id == preview.targetGroupId:
let nextMemberWorkspaceIds = memberWorkspaceIds.contains(preview.draggedWorkspaceId)
? memberWorkspaceIds
: memberWorkspaceIds + [preview.draggedWorkspaceId]
targetHeaderIndex = result.count
result.append(.groupHeader(group, memberWorkspaceIds: nextMemberWorkspaceIds))

case .workspace(let workspace, _)
where workspace.id == preview.draggedWorkspaceId:
draggedItem = item.withEffectiveGroupId(preview.targetGroupId)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Avoid synthetic reorder/expansion in no-op group previews.

If the dragged workspace already belongs to preview.targetGroupId, this helper still removes it and re-inserts it at the end of that group. And when the target group is collapsed, Lines 177-184 still insert a visible child row under the header. Both cases make the hover preview change layout even though the drop should be a no-op or the group should remain collapsed.

💡 Minimal fix sketch
     ) -> [SidebarWorkspaceRenderItem]? {
         guard let preview else { return nil }
         var draggedItem: SidebarWorkspaceRenderItem?
+        var draggedWorkspaceGroupId: UUID?
         var targetHeaderIndex: Int?
+        var targetGroupIsCollapsed = false
         var result: [SidebarWorkspaceRenderItem] = []
         result.reserveCapacity(items.count)

         for item in items {
             switch item {
             case .groupHeader(let group, let memberWorkspaceIds)
                 where group.id == preview.targetGroupId:
+                targetGroupIsCollapsed = group.isCollapsed
                 let nextMemberWorkspaceIds = memberWorkspaceIds.contains(preview.draggedWorkspaceId)
                     ? memberWorkspaceIds
                     : memberWorkspaceIds + [preview.draggedWorkspaceId]
                 targetHeaderIndex = result.count
                 result.append(.groupHeader(group, memberWorkspaceIds: nextMemberWorkspaceIds))

             case .workspace(let workspace, _)
                 where workspace.id == preview.draggedWorkspaceId:
+                draggedWorkspaceGroupId = workspace.groupId
                 draggedItem = item.withEffectiveGroupId(preview.targetGroupId)

             case .groupHeader(let group, _)
                 where group.anchorWorkspaceId == preview.draggedWorkspaceId:
                 return nil

             default:
                 result.append(item)
             }
         }

         guard let draggedItem, let targetHeaderIndex else { return nil }
+        if draggedWorkspaceGroupId == preview.targetGroupId {
+            return items
+        }
+        if targetGroupIsCollapsed {
+            return result
+        }
         var insertionIndex = targetHeaderIndex + 1
         while insertionIndex < result.endIndex {
             guard result[insertionIndex].effectiveGroupId == preview.targetGroupId else {
                 break
             }

Also applies to: 176-184

🤖 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/SidebarWorkspaceRenderItem.swift` around lines 155 - 165, The preview
logic is creating synthetic reorders and visible child rows even when the drop
would be a no-op or the group is collapsed; update the .groupHeader and
.workspace handling so you only modify the group's member list and append a
visible child row when necessary: in the .groupHeader branch (case
.groupHeader(let group, let memberWorkspaceIds)) only compute and use
nextMemberWorkspaceIds if memberWorkspaceIds does not already contain
preview.draggedWorkspaceId, and set targetHeaderIndex/result.append only when a
real change occurs; in the .workspace branch (case .workspace(let workspace, _))
avoid calling item.withEffectiveGroupId(preview.targetGroupId) if workspace.id
already belongs to preview.targetGroupId; additionally, when deciding to append
a child row under the header, check the group's expanded state (e.g.,
group.isExpanded / !group.isCollapsed) and only insert the visible child when
the target group is expanded. Ensure you reference nextMemberWorkspaceIds,
targetHeaderIndex, result.append(.groupHeader(...)), and withEffectiveGroupId to
locate the spots to change.

Comment thread Sources/ContentView.swift
Comment thread Sources/SidebarWorkspaceGroupHeaderView.swift
Comment thread Sources/ContentView.swift
Comment on lines +177 to +184
var insertionIndex = targetHeaderIndex + 1
while insertionIndex < result.endIndex {
guard result[insertionIndex].effectiveGroupId == preview.targetGroupId else {
break
}
insertionIndex += 1
}
result.insert(draggedItem, at: insertionIndex)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Collapsed group leaks a visible member row in the preview

groupDropPreviewItems never inspects group.isCollapsed. When a workspace is dragged over a collapsed group's header center, setGroupDropPreview is called (no isCollapsed guard in groupHeaderCenterDropAction), and this function finds targetHeaderIndex, skips the while-loop immediately (no visible member rows exist for a collapsed group), and inserts the dragged item at targetHeaderIndex + 1. The result is a workspace row with memberIndent rendered directly below a header that visually shows no members — an indented row appears inside a group that appears collapsed. The fix is to return nil early when group.isCollapsed, matching renderItems's own skipChildrenUntilNextGroup logic.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@Sources/ContentView.swift`:
- Around line 11892-11894: The closure passed to sidebarIndexForTabId currently
calls tabManager.tabs.firstIndex which does a linear scan on every drag-hover;
change it to use the precomputed O(1) lookup renderContext.tabIndexById (already
in scope) instead of touching tabManager so hover updates don't rescans the
list. Locate the sidebarIndexForTabId closure and replace the body that calls
tabManager.tabs.firstIndex { $0.id == workspaceId } with a lookup into
renderContext.tabIndexById[workspaceId] (or equivalent API) and return that
index (or nil) so the drag overlay uses the precomputed map.
- Around line 12152-12156: The body currently does a repeated O(n) search via
renderItems.first(where:) on every dragLocationInDocument tick; instead, when
you build the preview list (where renderItems is populated) create and store a
dictionary lookup keyed by representedWorkspaceId (e.g.,
renderItemByWorkspaceId: [WorkspaceID: RenderItem]) and replace
renderItems.first(where:) in this view with a constant-time lookup
(renderItemByWorkspaceId[draggedWorkspaceId]); keep using
dragState.draggedTabId, dragState.dragLocationInDocument and
anchors[draggedWorkspaceId] as before.
🪄 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: fa5e4663-b084-4190-b058-d8a9b4248ce9

📥 Commits

Reviewing files that changed from the base of the PR and between 3b0c5ec and b413abd.

📒 Files selected for processing (3)
  • Sources/ContentView.swift
  • Sources/SidebarWorkspaceGroupHeaderView.swift
  • Sources/VerticalTabsSidebar+WorkspaceGroups.swift

Comment thread Sources/ContentView.swift
Comment thread Sources/ContentView.swift Outdated
Comment on lines +12152 to +12156
var body: some View {
if let draggedWorkspaceId = dragState.draggedTabId,
let dragLocation = dragState.dragLocationInDocument,
let item = renderItems.first(where: { $0.representedWorkspaceId == draggedWorkspaceId }),
let anchor = anchors[draggedWorkspaceId] {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Don’t rescan renderItems on every drag-location tick.

dragLocationInDocument changes continuously while dragging, so this body gets reevaluated at mouse-move/autoscroll cadence. renderItems.first(where:) turns that into an O(n) walk on the hot path; precompute a lookup keyed by representedWorkspaceId when the preview list is built and read that here.

♻️ Proposed fix
 `@MainActor`
 private struct SidebarWorkspaceDragFollowerOverlay: View {
     let dragState: SidebarDragState
-    let renderItems: [SidebarWorkspaceRenderItem]
+    let renderItemsByWorkspaceId: [UUID: SidebarWorkspaceRenderItem]
     let anchors: [UUID: Anchor<CGRect>]
     let proxy: GeometryProxy
     let rowContent: (SidebarWorkspaceRenderItem) -> AnyView

     var body: some View {
         if let draggedWorkspaceId = dragState.draggedTabId,
            let dragLocation = dragState.dragLocationInDocument,
-           let item = renderItems.first(where: { $0.representedWorkspaceId == draggedWorkspaceId }),
+           let item = renderItemsByWorkspaceId[draggedWorkspaceId],
            let anchor = anchors[draggedWorkspaceId] {
             let frame = proxy[anchor]
             rowContent(item)

As per coding guidelines, **/*.{swift,ts,tsx,js,jsx,mjs,cjs,sh,zsh} applies .github/review-bot-rules/algorithmic-complexity.md and fails repeated full-collection scans in hot UI paths.

🤖 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 12152 - 12156, The body currently
does a repeated O(n) search via renderItems.first(where:) on every
dragLocationInDocument tick; instead, when you build the preview list (where
renderItems is populated) create and store a dictionary lookup keyed by
representedWorkspaceId (e.g., renderItemByWorkspaceId: [WorkspaceID:
RenderItem]) and replace renderItems.first(where:) in this view with a
constant-time lookup (renderItemByWorkspaceId[draggedWorkspaceId]); keep using
dragState.draggedTabId, dragState.dragLocationInDocument and
anchors[draggedWorkspaceId] as before.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 3 total unresolved issues (including 2 from previous reviews).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 8024587. Configure here.

Comment thread Sources/SidebarWorkspaceGroupHeaderView.swift
Comment thread Sources/ContentView.swift
Comment on lines +11860 to +11866
let renderItems = SidebarWorkspaceRenderItem.dragPreviewItems(
baseRenderItems,
draggedWorkspaceId: dragState.draggedTabId,
dropIndicator: dragState.dropIndicator,
reorderWorkspaceIds: renderContext.sidebarReorderIds,
groupDropPreview: dragState.groupDropPreview
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 dragState.dropIndicator read in sidebar body breaks @Observable isolation invariant

workspaceRows is a plain function called directly from VerticalTabsSidebar.body, not a separate View struct. With @Observable, every property access inside a body evaluation registers on the calling view — so reading dragState.dropIndicator and dragState.groupDropPreview here makes VerticalTabsSidebar itself an observer of those properties. Every hover-target change during drag (each time updateDropIndicator fires with a new target) now re-evaluates the entire VerticalTabsSidebar.body and recomputes dragPreviewItems on the main thread.

This is exactly the broad invalidation that the SidebarDragState comment says was fixed by issue #2586: "never the sidebar body or the LazyVStack itself." The updated comment in this PR softens that claim to "without rebuilding on every drag-location tick," which is accurate for dragLocationInDocument — but dropIndicator still triggers full-body rebuilds on every hover change.

The fix is to move the dragPreviewItems computation into its own @MainActor View struct so @Observable scopes the invalidation to that sub-view and leaves VerticalTabsSidebar.body unaffected during drag.

Rule Used: Flag SwiftUI changes that can cause stale state, b... (source)

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 5 files

Re-trigger cubic

@lawrencecchen lawrencecchen added the stale-revisit Closed after 30+ days without activity; preserved for possible revisit or reopening. label Sep 23, 2026
@github-project-automation github-project-automation Bot moved this from Todo to Done in cmux backlog Sep 23, 2026

This branch was successfully deployed

1 active deployment
Preview – cmux — 618ba98d Deployed Jun 2, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale-revisit Closed after 30+ days without activity; preserved for possible revisit or reopening.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants