Repository navigation
Fix sidebar Shift-click range anchor after no-op drag - #5601
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR refactors sidebar workspace selection anchor handling from inline logic into reusable ChangesSidebar Anchor Preservation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 18 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (18 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Greptile SummaryFixes the intermittent sidebar Shift-click range selection bug where a tiny accidental drag could trigger a no-op drop that silently reset
Confidence Score: 5/5Safe to merge — the fix is narrowly scoped to sidebar drag-drop and Shift-click anchor logic, backed by new regression tests that all pass. All three changed paths (no-op drop, real reorder, Shift-click) are well-reasoned and consistent with one another. The no-op early-return is the simplest correct fix; the UUID-based anchor snapshot for real reorders is correctly captured before the list mutates and correctly resolved after; the shiftClickAnchorIndex fallback chain (existing index → single selection → focus) handles all nil cases without crashing. New unit tests exercise each path deterministically, and the xcodebuild run confirms 8/8 pass. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User action in sidebar] --> B{Action type?}
B -->|Drag drop| C{fromIndex == targetIndex?}
C -->|Yes — no-op| D[return true\nno selection mutation]
C -->|No — real reorder| E[Snapshot anchorWorkspaceId\nbefore reorder]
E --> F[tabManager.reorderSidebarWorkspace]
F --> G[syncSidebarSelection\npreferredAnchorWorkspaceId]
G --> H{anchor UUID still\nin new selection?}
H -->|Yes| I[anchorIndex = new position\nof anchor UUID]
H -->|No| J[anchorIndex = focused\nworkspace index]
B -->|Click| K{isShift?}
K -->|Yes| L[shiftClickAnchorIndex\nexistingAnchorIndex → singleSelection → focus]
L --> M{anchor resolved?}
M -->|Yes| N[Select range\nlower…upper]
M -->|No| O[Fall through to\nnormal selection]
N --> P[anchorIndexAfterWorkspaceClick\nkeeps resolved anchor]
O --> P
K -->|No| Q[Regular / Cmd selection]
Q --> R[anchorIndexAfterWorkspaceClick\nsets anchor = clickedIndex]
Reviews (2): Last reviewed commit: "Move sidebar anchor tests to Swift Testi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| static func shiftClickAnchorIndex( | ||
| existingAnchorIndex: Int?, | ||
| selectedWorkspaceIds: Set<UUID>, | ||
| focusedWorkspaceId: UUID?, | ||
| liveWorkspaceIds: [UUID] | ||
| ) -> Int? { | ||
| if let existingAnchorIndex, | ||
| liveWorkspaceIds.indices.contains(existingAnchorIndex) { | ||
| return existingAnchorIndex | ||
| } | ||
| if selectedWorkspaceIds.count == 1, | ||
| let selectedWorkspaceId = selectedWorkspaceIds.first, | ||
| let selectedIndex = liveWorkspaceIds.firstIndex(of: selectedWorkspaceId) { | ||
| return selectedIndex | ||
| } | ||
| if let focusedWorkspaceId { | ||
| return liveWorkspaceIds.firstIndex(of: focusedWorkspaceId) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
shiftClickAnchorIndex returns existingAnchorIndex whenever it falls in-bounds, without verifying the workspace at that position is the one originally anchored. The drag-reorder path is now correctly protected by the pre-reorder UUID snapshot, but any mutation that shifts the list without going through reorderSidebarWorkspace — workspace close, workspace create, or group collapse that renumbers rows — can silently move the anchor to a different workspace. If workspace B at index 0 is closed while the anchor sits at index 2, the next Shift-click anchors to whatever is now at index 2 rather than the original anchor workspace. Converting lastSidebarSelectionIndex from Int? to UUID? would make the anchor invariant to all mutations without special-casing each mutating path.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
This branch was successfully deployed
1 participant
Summary
Fixes the intermittent sidebar Shift-click range selection bug where repeatedly Shift-selecting the next workspace could collapse or slide the selection after a tiny accidental drag.
The log-backed failure path was:
.onDragfor the adjacent row.SidebarTabDropDelegate.performDropsawfromIndex == targetIndexand treated the no-op drop as a selection repair.lastSidebarSelectionIndexto the focused workspace, so the next Shift-click used the wrong anchor.Changes
Verification
git diff --check origin/main...HEADxcodebuild -project cmux.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS' -derivedDataPath /tmp/cmux-shsel2-unit -only-testing:cmuxTests/SidebarWorkspaceSelectionSyncPolicyTests -only-testing:cmuxTests/SidebarWorkspaceSelectionAnchorPolicyTests test** TEST SUCCEEDED **$autoreview: clean, including cmux policyDogfood
Open shsel2. The tagged instance is pre-seeded with five workspaces:
workspace:1selected, thenShift Row 2throughShift Row 5.Starting from
workspace:1, repeatedly Shift-click one row lower: row 2, row 3, row 4, row 5. Before this fix, the range could intermittently collapse or slide to only the last pair when a small accidental row drag/no-op drop happened while holding Shift. Expected behavior: the range stays anchored atworkspace:1and expands downward, even if a tiny accidental no-op drag occurs.Note
Low Risk
Localized sidebar selection/drag-drop UI logic with dedicated unit tests; no auth, data, or network changes.
Overview
Fixes sidebar Shift-click range selection when a tiny accidental drag triggers a no-op drop that used to reset the range anchor to the focused row.
No-op drops (
fromIndex == targetIndex) now return without callingsyncSidebarSelection, solastSidebarSelectionIndexis not overwritten by focus.Shift-click resolves the range anchor via
SidebarWorkspaceSelectionSyncPolicy.shiftClickAnchorIndex(existing anchor index, single selected workspace, or focused workspace) and updates the anchor withanchorIndexAfterWorkspaceClickso the anchor stays fixed while focus moves down the list.Real reorders snapshot the anchor workspace ID before the move and restore it with
anchorIndexAfterWorkspaceReorderinstead of always anchoring on the focused tab aftersyncSidebarSelection(preserving:).Adds
SidebarWorkspaceSelectionAnchorPolicyTestsfor anchor identity, reorder, and shift-click behavior.Reviewed by Cursor Bugbot for commit 9f4dab1. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests