Repository navigation
Animate sidebar workspace groups (morph, collapse, rollup) + grouping focus fixes - #5779
lawrencecchen wants to merge 37 commits into
Conversation
The old reorder used .onDrag -> NSItemProvider, which spawns a macOS system drag session whose pointer location only updates coarsely via the drop delegate (and only over a valid target). That is what made dragging feel laggy and the follower drift. Replace it with a custom DragGesture that owns the interaction: a 1:1 floating follower, a live gap that animates open via dragPreviewItems, and hysteresis so the landing slot does not flicker at row midpoints. Workspace rows and group-header (anchor) rows both reorder through one shared path, so groups keep full parity. Per-frame cursor updates touch only the isolated follower overlay; the list reflows only on discrete drop-indicator changes, preserving the issue #2586 snapshot boundary. Committed row frames are frozen during a drag so hit-testing does not feed back on its own preview. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Move the dragged-row placeholder opacity out of TabItemView / SidebarWorkspaceGroupHeaderView and into the parent builders, and drop isBeingDragged from those views (and their == ). The gesture lives inside the equatable row; if isBeingDragged were an input it would flip when the drag starts, re-evaluate the row body, and could tear down the in-flight DragGesture. With the row's inputs constant during a drag, .equatable() skips its body and the gesture survives. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Animate group structure changes and fix two grouping UX issues, built on the promote-in-place workspace→group feature. Animations (shared via SidebarGroupAnimation, so every entrypoint — keyboard, context menu, header buttons, socket/CLI — animates identically): - Promote/ungroup morph: a workspace row now crossfade-morphs into a group header in place (and back). Root fix: key the sidebar ForEach on the anchor/workspace UUID (stable across the morph and also the scrollTo target) and drop the inner per-row .id() that was pinning identity and suppressing the _ConditionalContent transition. Removes the prior .id(groupAnchorSignature) whole-list rebuild hack, which also restores sidebar scroll position on group create/ungroup. - Multi-select grouping (createWorkspaceGroup) wrapped in withAnimation so the fresh anchor fades in and members indent in, matching the single case. - Collapse/expand: members slide+fade, chevron rotates (single rotated glyph instead of swapped images). - Member join/leave (incl. drag-into-group) and drag-reorder drop-settle animate via withAnimation around the TabManager mutations. Group header notification badge now rolls up unread across ALL members (anchor included), collapsed or expanded, instead of only the anchor when expanded. Renamed anchorUnreadCount → groupUnreadCount; animated count via contentTransition(.numericText). Keep focus on the user's current workspace when grouping a multi-selection: interactive ⌘⇧G / "New Group from Selection" pass selectAnchor: false and the collapse step prefers the previously-focused member over the new empty anchor. Socket/CLI path unchanged (non-focus per policy). Removed the blue sidebar drop-indicator line per dogfood feedback. Tests: promote-in-place keeps anchor render identity (round-trips through ungroup); delete removes the header + member rows from render items. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Greptile SummaryThis PR adds animations to sidebar workspace group operations (promote/ungroup morph, collapse/expand, member join/leave, drag-reorder settle) via a shared
Confidence Score: 5/5The change is safe to merge; the one non-blocking note is a bounded, cancellable polling loop that follows an existing pattern in the same class. The morph-identity fix removes a hack rather than adding one, the live-reorder control law is pure and fully unit-tested, the #2586 layout-thrash guard is preserved throughout, and the new applyGestureDragReorder path is well-guarded against anchors and stale group ids. Sources/ContentView.swift — the Escape-key polling task in SidebarDragFailsafeMonitor is worth revisiting to confirm whether the gesture-drag path can use the existing keyDown monitor instead of polling. Important Files Changed
Reviews (33): Last reviewed commit: "WIP: isolated animated group backdrop + ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -4285,14 +4310,21 @@ class TabManager: ObservableObject { | |||
| customColor: nil, | |||
| iconSymbol: nil | |||
| ) | |||
| workspaceGroups.append(group) | |||
| assignGroup(workspaceId: anchorWorkspaceId, groupId: group.id) | |||
| // The anchor is already at its sidebar position; normalize keeps it | |||
| // there (`sidebarTopLevelWorkspaceIds` maps the now-grouped workspace | |||
| // back to its anchor's slot) while syncing the workspaceGroups order | |||
| // to the tabs order. | |||
| normalizeWorkspaceGroupContiguity() | |||
| postWorkspaceOrderDidChange(movedWorkspaceIds: [anchorWorkspaceId]) | |||
| // Animate the workspace row morphing into its group header in place. | |||
| // The anchor keeps its ForEach identity (see | |||
| // `SidebarWorkspaceRenderItem.id`), so this crossfades rather than | |||
| // teleports. Shared path: every entrypoint (⌘⇧G, context menu, socket) | |||
| // routes here, so they all animate identically. | |||
| withAnimation(SidebarGroupAnimation.structure) { | |||
| workspaceGroups.append(group) | |||
| assignGroup(workspaceId: anchorWorkspaceId, groupId: group.id) | |||
| // The anchor is already at its sidebar position; normalize keeps it | |||
| // there (`sidebarTopLevelWorkspaceIds` maps the now-grouped | |||
| // workspace back to its anchor's slot) while syncing the | |||
| // workspaceGroups order to the tabs order. | |||
| normalizeWorkspaceGroupContiguity() | |||
| postWorkspaceOrderDidChange(movedWorkspaceIds: [anchorWorkspaceId]) | |||
| } | |||
| return group.id | |||
| } | |||
|
|
|||
| @@ -4418,28 +4450,32 @@ class TabManager: ObservableObject { | |||
| } | |||
| if isAnchorOfOtherGroup { return } | |||
| let originalTopLevelIds = sidebarTopLevelWorkspaceIds() | |||
| assignGroup(workspaceId: workspaceId, groupId: groupId) | |||
| // selectedTabId may not change here (the workspace was already | |||
| // selected), so the existing didSet hook won't fire. Expand manually | |||
| // when the added workspace is the focused one so it doesn't end up | |||
| // hidden inside a collapsed section. | |||
| if selectedTabId == workspaceId, | |||
| let groupIndex = workspaceGroups.firstIndex(where: { $0.id == groupId }), | |||
| workspaceGroups[groupIndex].isCollapsed { | |||
| workspaceGroups[groupIndex].isCollapsed = false | |||
| } | |||
| normalizeWorkspaceGroupContiguity( | |||
| preservingTopLevelIds: originalTopLevelIds.filter { $0 != workspaceId } | |||
| ) | |||
| if let placement { | |||
| placeWithinGroup( | |||
| workspaceId: workspaceId, | |||
| groupId: groupId, | |||
| placement: placement, | |||
| referenceWorkspaceId: referenceWorkspaceId | |||
| // Animate the workspace sliding under the header and indenting as a | |||
| // member. Covers the drag-into-group drop and the socket/CLI add path. | |||
| withAnimation(SidebarGroupAnimation.structure) { | |||
| assignGroup(workspaceId: workspaceId, groupId: groupId) | |||
| // selectedTabId may not change here (the workspace was already | |||
| // selected), so the existing didSet hook won't fire. Expand | |||
| // manually when the added workspace is the focused one so it | |||
| // doesn't end up hidden inside a collapsed section. | |||
| if selectedTabId == workspaceId, | |||
There was a problem hiding this comment.
withAnimation embedded in model mutations with no opt-out for batch callers
Group structure mutations (createWorkspaceGroup, makeWorkspaceGroupFromWorkspace, addWorkspaceToGroup, removeWorkspaceFromGroup, ungroupWorkspaceGroup) are now unconditionally wrapped in withAnimation inside TabManager. This couples a presentation-layer concern directly into the business-logic layer with no mechanism for a caller to suppress animation — e.g., a future socket-initiated batch resync that creates or dissolves multiple groups in a tight loop would produce a cascade of concurrent .snappy transactions that cannot be turned off from outside the model.
The collapse/expand path already establishes the right pattern: toggleWorkspaceGroupCollapsed (UI path) wraps the call in withAnimation, while setWorkspaceGroupCollapsed (pure-data path) mutates without a transaction, letting any caller opt into animation at the call site. Group create/dissolve/membership mutations don't have this split, so the model owns both the data invariant and the animation timing, making the two concerns hard to separate later.
Consider extracting animation to the call site (like the reorder paths already do) or adding an animated: Bool = true parameter mirroring the collapse split.
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!
There was a problem hiding this comment.
Deliberate: the PR's goal is that every entrypoint (keyboard, context menu, header buttons, socket/CLI) animates identically, so the animation lives with the one shared mutation path instead of each caller opting in (per the repo's shared-entrypoint policy). withAnimation outside a SwiftUI transaction is a no-op for non-UI callers, so batch/socket paths pay nothing. If a caller ever genuinely needs to suppress animation we can add a parameter then.
There was a problem hiding this comment.
Fair point on the shared-entrypoint policy — that's a coherent design. One thing worth double-checking: the "no-op for non-UI callers" claim holds only if those callers aren't on @MainActor. If TabManager is @MainActor (which is typical for ObservableObject), socket/CLI mutations also run on the main actor, so withAnimation inside them would not be a no-op — it would apply the animation transaction to those paths too, which contradicts the PR description's "Socket/CLI path unchanged (non-focus per policy)" note.
If that's not already tested, it's worth verifying that a socket-driven batch resync doesn't accumulate concurrent .snappy transactions visually. The deferred-parameter approach you mentioned (animated: Bool = true) would be a clean escape hatch if that scenario comes up.
There was a problem hiding this comment.
Right, my "no-op" phrasing was wrong — TabManager is @mainactor, so socket/CLI group mutations do get the animation transaction. That is the intent: the PR's "every entrypoint animates identically" includes socket/CLI (one group mutation per command, so no transaction pile-up; a CLI-driven collapse animates the same as a click). The "unchanged (non-focus per policy)" note in the description is about focus/selection, not animation — socket paths still never move focus. If a future bulk resync path shows visual churn, the animated: parameter is the escape hatch.
| /// Shared animation timings for workspace-group structure changes in the | ||
| /// sidebar (promote-in-place, ungroup, collapse/expand, member join/leave, | ||
| /// drag-reorder settle). | ||
| /// | ||
| /// One source of truth so every entrypoint that mutates group structure | ||
| /// (keyboard shortcut, context menu, group-header buttons, drag-and-drop, | ||
| /// socket/CLI) animates identically. The `TabManager` group mutation methods | ||
| /// wrap their structural changes in `withAnimation(SidebarGroupAnimation.<case>)`, | ||
| /// and the sidebar rows carry `.transition(.sidebarGroupRow)` so inserted or | ||
| /// removed rows fade while their siblings reflow within the same transaction. | ||
| enum SidebarGroupAnimation { | ||
| /// Workspace row morphs into a group header (and back), member rows join or | ||
| /// leave a group, drag-reorder drops settle. | ||
| static let structure: Animation = .snappy(duration: 0.26) | ||
|
|
||
| /// Group collapse / expand: member rows slide+fade and the chevron rotates. | ||
| static let collapse: Animation = .snappy(duration: 0.24) | ||
| } | ||
|
|
||
| extension AnyTransition { | ||
| /// Transition for a sidebar row appearing or disappearing because of a | ||
| /// group-structure change. A plain opacity fade keeps the morph between a | ||
| /// workspace row and a group header (same ForEach identity, swapped | ||
| /// `_ConditionalContent` branch) reading as a crossfade, while the | ||
| /// surrounding `withAnimation` transaction animates the list reflow so | ||
| /// collapse/expand and member join/leave feel like a slide for free. | ||
| static var sidebarGroupRow: AnyTransition { | ||
| .opacity | ||
| } | ||
| } |
There was a problem hiding this comment.
SidebarGroupAnimation and the AnyTransition.sidebarGroupRow extension are added to SidebarWorkspaceGroupingMetrics.swift, which was otherwise purely about spatial layout constants (indent, row padding). The file now holds two distinct conceptual concerns. Given that SidebarGroupAnimation is referenced from both view code and TabManager, a dedicated SidebarGroupAnimation.swift would keep each file's responsibility clear and make it easier to find or extend animation settings independently of layout metrics.
Rule Used: Flag Swift changes that add too much unrelated res... (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!
- Escape now cancels an in-progress sidebar drag. The existing escape_cancel handler used a local keyDown monitor, which never fires inside the native NSDraggingSession modal loop. Poll CGEventSource.keyState for Escape instead (same HID-read trick the mouse-button failsafe already uses via buttonState), readable inside the drag loop. Bounded/cancellable task scoped to the live drag. - Removed the end-of-list (empty-area) blue drop-indicator line too, matching the per-row indicator removal. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…cator When a drag would insert above a row, that row opens a row-tall animated gap so the list visibly shifts apart to preview the drop slot, replacing the removed blue insertion line. Purely a layout animation driven by the existing topDropIndicatorVisible signal: the model is only reordered once on drop, so there's no during-drag @published reorder thrash (#2586) or hover oscillation from live model mutation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the greyed-ghost + gap preview with real live reorder: as the drag moves over rows, the dragged workspace is immediately moved to the hovered drop slot (within withAnimation), so the row physically travels through the list and the others reorder around it. The drop commit then no-ops because the order is already correct. - commitLiveReorder reuses the exact drop-commit planner (targetIndex from the freshly-set indicator with the same pinned/group/legal-range inputs), so constraints hold on every step. Only fires when the resolved index changes (once per row crossed), intra-window only. Logs sidebar.liveReorder for diagnosing jank. - Dragged row no longer dims in place (it moves instead). - Escape restores the order captured at drag start (live reorder is undone); a normal release commits via performDrop and keeps the new order. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79798c1478
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let targetIndex = SidebarDropPlanner.targetIndex( | ||
| draggedTabId: draggedTabId, | ||
| targetTabId: targetTabId, | ||
| indicator: dragState.dropIndicator, | ||
| tabIds: reorderTabIds, | ||
| pinnedTabIds: pinnedTabIds, | ||
| legalInsertionRange: legalInsertionRange | ||
| ), |
There was a problem hiding this comment.
When dragging a row past a neighbor, the first live reorder can make the dragged row already sit at the pointer's intended insertion point, so the next updateDropIndicator returns nil; this code then calls targetIndex with a nil indicator, which falls back to preferredEdge instead of the actual pointer edge and moves the row back to the other side of the hovered target. For example, dragging the first workspace to the bottom half of the third row moves it below that row, then the next hover tick over the same bottom half sees dropIndicator == nil and moves it above the row again, causing oscillation/incorrect final order. Treat a nil indicator as “already at the live target” and don't recompute from targetTabId here.
Useful? React with 👍 / 👎.
The first live-reorder pass ping-ponged the dragged row between two adjacent slots while the pointer was held still: each reorder shifts rows, which re-fires the drop delegate with the pointer unmoved, and the planner's targetIndex (which depends on the dragged row's current index) flips back. The debug log showed from=N to=N+1 then N+1 to=N repeating every ~25ms. Loop-breaker: only commit a live reorder when the global pointer has moved since the last committed reorder (>5pt). Self-induced re-fires arrive at the same location and are skipped; genuine drag movement still reorders. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67408932fc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| _ = withAnimation(SidebarGroupAnimation.structure) { | ||
| tabManager.reorderWorkspaces(orderedWorkspaceIds: snapshot) |
There was a problem hiding this comment.
When Escape cancels a drag after commitLiveReorder has already moved a workspace into or out of an expanded group, the live path calls reorderSidebarWorkspace(..., isDragOperation: true), which can mutate Workspace.groupId via applyDragInferredGroupMembership. This restore only replays the saved tab order, and reorderWorkspaces rebuilds the same Workspace objects without restoring their previous group ids, so cancelling can leave the workspace in the wrong group even though the row order snaps back. Capture and restore group membership (and any group ordering state) along with the id order for escape_cancel.
Useful? React with 👍 / 👎.
Review fixes from Cursor Bugbot and Codex on the live-reorder work: - The pre-drag snapshot now restores on EVERY clear request (escape, mouse-up failsafe, app deactivation, outside-sidebar drop, aborted performDrop), not only escape_cancel. The live reorder mutates the model on hover, so any drag end that doesn't commit must roll back. - The snapshot captures group membership alongside row order: reorderSidebarWorkspace(isDragOperation:) can rewrite Workspace.groupId when a row is dragged into or out of an expanded group, so restoring the id order alone left the row in the wrong group. New TabManager.SidebarDragRestoreSnapshot capture/restore API, unit-tested. - commitLiveReorder skips the move when the freshly-updated drop indicator is nil: the planner suppresses the indicator when the dragged row already sits at the pointer's insertion point, and recomputing from preferredEdge bounced the row back across the hovered row. - createWorkspaceGroup(selectAnchor: false) keeps the collapsed sidebar selection on the previously focused workspace even when it isn't a group member, so the sidebar highlight tracks the still-active workspace instead of jumping to the new empty anchor. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
SidebarWorkspaceGroupingMetrics.swift goes back to holding only spatial layout constants (Greptile review note). Also drops the now-dead dropGapHeight: the animated drop gap was replaced by the live reorder. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ef6b60c82
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let last = dragState.lastLiveReorderMouseLocation, | ||
| hypot(mouseLocation.x - last.x, mouseLocation.y - last.y) < 5 { | ||
| return |
There was a problem hiding this comment.
When a drag is held in the sidebar’s top/bottom autoscroll zone after one live reorder, the autoscroll timer scrolls the list under a stationary pointer, but this guard drops subsequent hover updates because NSEvent.mouseLocation has not moved by 5 points. In that common drag-to-edge scenario the row stops following the newly exposed targets and the eventual drop can commit to the stale insertion point instead of the row currently under the cursor; the loop breaker needs to distinguish self-induced row shifts from scroll-origin changes or reset when the scroll view moves.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c4c6513: the loop-breaker now ignores a hover update only when the pointer is unmoved AND the hovered row is unchanged (the self-induced re-fire signature), so rows arriving under a stationary pointer during autoscroll reorder normally. The nil-indicator guard keeps the original ping-pong dead: after a reorder the pointer sits over the dragged row (early return) or the planner reports already-in-place (nil indicator).
Codex review P2: the loop-breaker keyed on pointer movement alone, but autoscroll moves new rows under a held pointer, so the dragged row stopped following once the drag sat in the top/bottom autoscroll zone. The breaker now ignores a hover update only when the pointer is unmoved AND the hovered row is unchanged — the self-induced re-fire signature. A new row arriving under a stationary pointer reorders normally. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4c6513eef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // is usually a no-op (caught by the fromIndex == targetIndex guard | ||
| // above); it only mutates when the last hover tick and the drop | ||
| // disagree, e.g. a fast flick released before the final dropUpdated. | ||
| let didReorder = withAnimation(SidebarGroupAnimation.structure) { |
There was a problem hiding this comment.
When a live reorder has already moved the dragged row into the hovered slot, updateDropIndicator suppresses the indicator as nil, but this drop commit still falls through and recomputes a target from that nil indicator. For example, dragging the first workspace below the third can live-reorder to the correct order; releasing over the same bottom half then makes targetIndex fall back to preferredEdge with the dragged row now after the target, so this commit moves it back before the target. The nil-indicator no-op rule added for hover needs to apply before this final reorder as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 99ca472: performDrop now applies the same nil-indicator rule before computing targetIndex — nil indicator over a row commits as a no-op (the live reorder already produced the final order) instead of falling through to the preferredEdge recompute.
Codex review P2, the drop-time sibling of the hover fix: performDrop
recomputed targetIndex from a nil indicator, falling back to
preferredEdge — releasing over the same row half after the live reorder
had already placed the row moved it back across the target. The hover
path's nil-indicator rule ("planner says already at the pointer's
insertion point") now applies before the final commit too.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99ca472adb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Snapshot order + group membership at drag start so a | ||
| // non-committing drag end can undo the live reorder. Captured | ||
| // before any drop hover mutates the model. | ||
| dragReorderSnapshot = tabManager.captureSidebarDragRestoreSnapshot() |
There was a problem hiding this comment.
When a native drag enters a sidebar row before SwiftUI delivers this .onChange callback, dropEntered now runs commitLiveReorder and mutates tabs before the rollback snapshot is captured. That leaves dragReorderSnapshot either nil or already reflecting the first live reorder, so Escape, outside-sidebar drops, or other non-committing drag ends cannot restore the original order. Capture the snapshot in the drag-start path (before the first hover can run), rather than relying on a later view update.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in beed2dd: the snapshot moved onto SidebarDragState and commitLiveReorder captures it synchronously right before the drag's first model mutation, so the .onChange timing no longer matters. The onChange capture is removed.
Dogfood: the indent/width animation never played. The follower's blanket
.transaction { disablesAnimations = true } (inherited from the original
engine to keep cursor tracking snap-instant) suppresses ALL downstream
animations — disablesAnimations kills .animation(_:value:) too — so the
indent keyed inside it could never run.
The blanket is removed. Cursor tracking stays animation-free by
construction: .position sits OUTSIDE the indent's .animation scope, no
other animation modifier covers it, and followerCursorY writes are
never wrapped in withAnimation. The indent/width now animate on their
0.15s axis.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6efee48e66
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .modifier(SidebarReorderRowModifier( | ||
| enabled: isReorderEnabled, | ||
| workspaceId: tab.id, | ||
| onChanged: onReorderChanged, | ||
| onEnded: onReorderEnded | ||
| )) |
There was a problem hiding this comment.
This replacement removes the SidebarTabDragPayload drop delegate from each main sidebar row, while the extension/browser-stack rows still start native workspace drags with SidebarTabDragPayload.provider and rg "SidebarTabDropDelegate(" now shows the main list only accepts that payload in the empty area/top strip. In that context, dragging one of those native workspace payloads onto a specific main-sidebar row no longer invokes the reorder delegate, so row-targeted moves from those surfaces are lost; keep a row SidebarTabDropDelegate or route native payloads through the new gesture path too.
Useful? React with 👍 / 👎.
…undary hitboxes Dogfood: the indent/width animation still did not play after removing the transaction blanket, and the in/out boundary at a group's end felt wrong. - The follower's indent is now follower-local @State driven by an explicit withAnimation in onChange(of: previewExtraIndent) — the most deterministic animation mechanism available, with no dependence on .animation(_:value:) transaction semantics. Width and X both derive from the animated value. - The in/out boundary at a group's bottom edge moved from the boundary row's bottom pixel to the MIDPOINT of the space between it and the next row: the "last member" hitbox covers the row's lower half plus half the gap, the "first after group" hitbox the other half plus the next row's top half — two comparably-sized zones with 3pt hysteresis. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3bf119633f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| withAnimation(Self.sidebarReorderAnimation) { | ||
| _ = tabManager.applyGestureDragReorder( |
There was a problem hiding this comment.
When a selected workspace is moved with the new gesture path, the reorder commits here but never re-syncs selectedTabIds/lastSidebarSelectionIndex the way the old SidebarTabDropDelegate.performDrop path did after reordering. The selected ID set may still contain the moved row, but the anchor index remains at its pre-drag position, so the next Shift-click range can anchor on whatever workspace now occupies that old index and select the wrong rows. Snapshot the selection anchor before committing and restore it by workspace id after successful gesture reorders.
Useful? React with 👍 / 👎.
…tation site Dogfood pinned an asymmetry: the in-place X nudge animated the indent but a vertical drag into a group snapped instantly — the two flips took different view-update paths and the onChange+@State mechanism only survived one of them. The rendered indent is now a SidebarDragState property (followerRenderedIndent) written ONLY by setPreviewMembership, wrapped in an explicit withAnimation there — the same mutation-site pattern the TabManager group morphs use. Every flip path (slot crossing, X nudge) funnels through that one write, so both animate identically by construction. The follower just renders the value; the @State/onChange plumbing and the parent's indent computation are removed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ed groups Dogfood asks: - The reorder now triggers the moment the dragged row TOUCHES a neighbor instead of when the cursor crosses row midpoints. The slot probe is the follower's leading edge for the current drag direction (top edge moving up, bottom edge moving down) with ~6pt penetration; moving up claims the slot above the touched row, moving down the slot below it. Gaps and the dragged row's own frozen band keep the current slot, which doubles as hysteresis. Still built through SidebarDropPlanner so pinned-tier legality and canonicalization hold, and still hit-tested against frozen frames so there is no feedback. - Spring-loaded collapsed groups: parking the cursor over a collapsed group's header for 600ms mid-drag expands the group (cancellable Task; cancelled when the cursor leaves or the drag ends) so the row can be placed at a specific slot inside, Finder-style. After the expand, the indicator clears and the frozen frame map re-baselines from the expanded committed layout. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…eader join tint Dogfood: dragging into a group still felt non-intuitive. Root cause: the slot flipped on the follower's leading edge (touch probe) while the in/out membership zones still measured the CURSOR — two reference points, so the gap moved before the indent did and the row looked like it would not join even when it would. - The membership zones now use the SAME follower-edge probe as the slots: touching a group's interior indents immediately together with the gap, and the boundary sub-zones flip in lockstep with what the user sees. - New affordance: while the resolved landing membership is a group, that group's header shows a soft accent tint (geometry matched to the header pill — radius 4 inside the 6pt gutter, unlike the removed oversized ring), making "release joins this group" unmistakable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ontrol law Dogfood: "too sensitive, sometimes two workspaces reorder and jump at once". An ultracode design pass traced it to a discontinuous control function: the probe was the follower's TOP edge when dragging up and the BOTTOM edge when dragging down, so a 0.6pt hand reversal flipped the direction bit and teleported the probe ~38pt (a row height), jumping the slot 2-3 positions in one event. The group in/out boundary used the same teleporting probe, which is why it also felt bistable. The whole slot+membership map is now a monotone step function of ONE scalar against ONE strictly-increasing threshold sequence: - probeY = follower CENTER + a slew-limited velocity lean (EMA, capped at ±(H/2 - 6), moved at most 3pt/event). At rest it is the center (classic iOS reference); at drag speed it leans 6pt inside the leading edge so a row still triggers the swap on contact — with no binary direction state, so a jitter moves the probe ≤ |dY|+3pt instead of a row height. - slot = Schmitt-clamped staircase over neighbor midpoints (4pt dead band): clamp(prev, lo, hi). At most one threshold crosses per event, so two rows can never reorder at once; the memory is consulted only inside a dead band, never as sticky state. - membership = Schmitt around the gap midpoint M, read off the SAME probeY, with M strictly interleaved between the two neighbor midpoints, so descending a group bottom crosses prevMid → M → nextMid as three distinct single steps. The in-place X flip survives as an override only at the dragged row's own slot, keyed on the legalized slot so indicator flicker can't erase accumulated X intent. The pure control math moves into SidebarReorderIndicatorResolver (probe / slot / membershipIn) so the live path is exactly what the tests exercise (the old midpoint+sticky resolver was dead, referenced only by tests). A new SidebarDropPlanner.plan(neighborSlot:) overload feeds the staircase slot through the existing pinned-tier legalization. Spring-load resets the velocity/bias/slot memory so the expansion doesn't blink. Tests: monotone staircase, dead-band hold, jitter-never-moves-slot, per-event single-step sweep, slew bound, rest decay, membership Schmitt, and the planner neighbor-slot mapping. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… leave Two dogfood asks: - Group headers now show a subtle hover background (primary.opacity(0.05)), matching how workspace rows highlight, so hovering a group reads as interactive. - Spring-loaded collapsed groups now COLLAPSE BACK when the drag leaves them, if they were collapsed at drag begin (Finder spring-loaded folder behavior). Dragging onto a collapsed group's header still expands it after a short dwell (450ms, down from 600); the group's live span (header + now-visible members) is tracked, and once the cursor leaves that span the group re-collapses and re-arms. On drop, the group stays expanded only if the row landed inside it; otherwise it collapses. gestureEnded returns the landed group so the caller can decide. Geometry/velocity state re-baselines on every collapse toggle so the indicator never blinks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce61759be9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let onReorderEnded: (CGPoint, CGSize) -> Void = { [tabId = tab.id] _, _ in | ||
| sidebarReorderGestureEnded(draggedId: tabId) |
There was a problem hiding this comment.
When the mouse is released after moving since the last onChanged tick, DragGesture.onEnded provides the final translation but this closure discards it and commits whatever dropIndicator/membership was last computed. In a quick flick or coalesced mouse-up over a different row or group boundary, the release can therefore commit the stale slot instead of the row currently under the pointer; update the reorder state with the final startLocation/translation before calling sidebarReorderGestureEnded.
Useful? React with 👍 / 👎.
… just the header The hover/drop highlight now covers the entire group area. A shared highlightedGroupId drives it: during a drag it is the membership the dragged row would commit to (drop-into target), otherwise the hovered group's id (header hover reports up to the parent). The header and every member row draw a matching full-width accent tint (applied after the member indent so it spans the full width, bled past the 2pt inter-row gap so the group reads as one continuous block). Renames the header's isMembershipPreviewTarget to isGroupHighlighted since it is now hover- or drag-driven. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 615bfe77a9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let upperBound = workspace.isPinned | ||
| ? max(firstIndex + pinnedMemberCount, lowerBound) | ||
| : lastIndex | ||
| : lastIndex + 1 |
There was a problem hiding this comment.
When an ungrouped workspace above a group is dragged into a group at the bottom of the sidebar, applyGestureDragReorder writes the new groupId before calling reorderWorkspace, so this helper can return lastIndex + 1 == tabs.count. reorderWorkspace then removes the source row before calling tabs.insert(workspace, at: plan.toIndex), making that index one past the new end and causing a fatal Array precondition failure for a common drop like [workspace, groupHeader] -> drop workspace into group.
Useful? React with 👍 / 👎.
| if let cancelledId = dragState.draggedTabId { | ||
| dragState.cancelledReorderTabId = cancelledId | ||
| } | ||
| dragState.clearDrag() |
There was a problem hiding this comment.
If a drag dwell spring-expands a collapsed group and the drag is then cancelled through Escape, app deactivation, or the mouse-up failsafe, this clear path bypasses sidebarReorderGestureEnded and calls clearDrag() directly. clearDrag() discards springAutoExpandedGroupId, so the group that was only temporarily opened for the drag stays expanded even though no drop landed inside it; the cancel path needs to collapse the recorded spring-loaded group before clearing that state.
Useful? React with 👍 / 👎.
The whole-group highlight previously only triggered from the header's hover. Member workspace rows now report hover up to hoveredGroupId too (guarded so passing ungrouped rows writes nothing), so hovering any row inside a group lights the entire group region. Drag membership still takes precedence over hover. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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 e968159. Configure here.
…t a per-row tint The whole-group highlight was a background tint on each row/header, so it traveled with each workspace during a reorder (gray fragments moving with the dragged/reflowing rows). It is now ONE rounded backdrop drawn behind the rows: each row of the highlighted group emits its bounds under the group id (only when highlighted, so no virtualization cost otherwise), the sidebar unions them into a single rect via backgroundPreferenceValue, and the rows (and the dragged-row follower) sit on top of that fixed region. Reordering moves rows over a stable backdrop instead of dragging tint around. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Backdrop geometry funneled through dragState and read only by the isolated SidebarGroupHighlightBackdropView (sibling background), so per-frame union frames never re-run the sidebar body / LazyVStack (issue #2586). Sooner catch margin when joining a group from outside. Diagnostic grab/membership logs still present (temporary). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Stacked on #5633 (promote-in-place). Adds animations and two grouping UX fixes that came out of dogfood.
Animations
All routed through a shared
SidebarGroupAnimation, so every entrypoint (keyboard, context menu, header buttons, socket/CLI) animates identically.ForEachis now keyed on the anchor/workspace UUID (stable across the morph, and thescrollTotarget), and the inner per-row.id()that was pinning identity is removed, so the_ConditionalContentswap actually runs its transition. This lets me delete the.id(groupAnchorSignature)whole-list rebuild hack from Turn a single workspace into a group (promote in place) #5633, which also fixes the sidebar-scroll-resets-to-top tradeoff that PR called out.createWorkspaceGroup, the fresh-anchor path) wrapped inwithAnimationso it animates like the single case.withAnimationaround theTabManagermutations.Group notification rollup
The group header badge now sums unread across all members (anchor included), collapsed or expanded, instead of showing only the anchor's count when expanded.
anchorUnreadCount→groupUnreadCount, animated withcontentTransition(.numericText).Grouping focus fix
Grouping a multi-selection used to yank focus to the new empty anchor. Interactive ⌘⇧G / "New Group from Selection" now pass
selectAnchor: false, and the collapse step keeps focus on the previously-focused member. Socket/CLI path unchanged (non-focus per policy).Other
Live drag-reorder + cancel (landed after the first dogfood round)
The sidebar drag now live-reorders: the dragged row physically travels through the list as you hover (
commitLiveReorderreuses the exact drop planner the commit uses, so pinned tiers and group boundaries hold on every step), and the drop usually no-ops because the order is already right. Escape cancels the drag via an HID key-state poll (a local keyDown monitor can't see Escape inside the native drag loop). The blue insertion line is gone.Review-round fixes on top (Cursor Bugbot high, Codex P1/P2, Greptile P2s):
Workspace.groupId, so order alone wasn't enough. NewTabManager.SidebarDragRestoreSnapshotcapture/restore API, unit-tested.commitLiveReorderskips when the freshly-updated drop indicator is nil (the planner's "already at the pointer's slot" signal); recomputing frompreferredEdgebounced the row back across the hovered row.createWorkspaceGroup(selectAnchor: false)keeps the collapsed sidebar selection on the previously focused workspace even when it isn't a member, so the sidebar highlight tracks the still-active workspace instead of jumping to the new empty anchor.SidebarGroupAnimationmoved to its own file; deaddropGapHeightremoved.Tests
promoteInPlaceKeepsAnchorRenderIdentity: the morph keeps the anchor's render identity and round-trips through ungroup.deleteRemovesGroupHeaderAndMemberRowsFromRenderItems: delete clears the header + member rows from the render items.dragRestoreSnapshotRestoresOrderAndGroupMembership/...MembershipWhenOrderAlreadyMatches/...SkipsWorkspaceClosedMidDrag/...IsNoOpWhenNothingChanged: cancelled-drag rollback semantics.createGroupKeepsSidebarSelectionOnFocusedNonMember: grouping doesn't move the sidebar selection off the focused workspace.Mechanism / principled?
Principled. The morph fix removes a hack (
.id(groupAnchorSignature)) rather than adding one, and fixes the scroll-reset regression as a side effect. Animations are driven from the shared model mutations so all entrypoints stay consistent.Dogfood status
Single + multi promote/ungroup morph, collapse/expand, notif rollup, focus fix, and blue-line removal dogfood-confirmed on tag
grpanim. The live drag-reorder, Escape-cancel, and cancel-restore round is rebuilt on the same tag and awaiting dogfood.🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
High Risk
Large, interaction-critical changes to sidebar ordering, group membership, pinning, and SwiftUI layout (issue #2586-class thrash was explicitly guarded)—regressions would affect daily navigation across every window.
Overview
Reworks sidebar workspace group structure and reorder so promote/ungroup, collapse, and drag-and-drop feel consistent and keep scroll position stable.
Animations & identity: Introduces shared
SidebarGroupAnimationand wrapsTabManagergroup mutations (create, promote-in-place, add/remove member, ungroup, collapse toggle) in matchingwithAnimation. Rows use.transition(.sidebarGroupRow).SidebarWorkspaceRenderItemnow keys the list on the anchor/workspace UUID so a workspace can morph into a group header in place—replacing the old.id(groupAnchorSignature)full-list rebuild that reset scroll.Gesture reorder (replaces row
.onDrag+ drop delegates for workspace reorder): Rows use a list-coordinateDragGesture, a floatingSidebarReorderFollowerView(only view reading per-framefollowerCursorY), and expandedSidebarDragStatewith velocity-biased slot math (SidebarReorderIndicatorResolver), live preview list order (dragPreviewItems), group membership preview (Y slot + X tuck/pull), spring-loaded collapsed groups, and autoscroll feeding the follower. Drop commits viaTabManager.applyGestureDragReorderwith explicitdesiredGroupId. Blue insertion lines are removed; cancel uses an Escape HID poll and a latch so cleared drags don’t restart mid-gesture. Bonsplit-only drop-target collection avoids tearing down the gesture host mid-drag; scroll-follow is suppressed while dragging.Grouping UX: Multi-select ⌘⇧G / “New Group” passes
selectAnchor: falseand keeps sidebar selection on the previously focused workspace. Group headers show rollup unread across all members (groupUnreadCount). Whole-group highlight (hover + drag membership target) via a unioned backdrop; member row hover propagates to the group.Smaller: Keyboard/context-menu reorders and legacy drop paths get animated settles and nil-indicator no-ops; member-run clamp fixes for gesture joins.
Reviewed by Cursor Bugbot for commit fc4bc75. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Animated the sidebar’s workspace groups and rebuilt reorder around a gesture-driven follower so rows morph, collapse, and join/leave groups smoothly while focus and scroll stay stable. The whole-group highlight is now a single backdrop rendered via an isolated background for better performance, and last-member joins from outside catch sooner.
New Features
SidebarGroupAnimationfor promote/ungroup, collapse/expand, member add/remove, keyboard reorders, and drop-settle; chevrons rotate; unread counts roll up with animated numbers; stable row identity preserves scroll.selectAnchor: false; leaving the pinned tier unpins; joining collapsed groups auto-expands; spring-loaded groups auto-collapse when you leave.Bug Fixes
Written for commit ccb582b. Summary will update on new commits.