Repository navigation
Keep selected workspace visible after sidebar reorders - #4083
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements deferred scrolling of the selected workspace when it is moved to a new position via reordering. TabManager posts workspace-order-change notifications with moved workspace IDs; a new scroll-policy enum determines when scrolling is needed; ContentView integrates the policy and defers scroll targets when workspaces are reordered. Comprehensive unit and UI tests validate the flow. ChangesWorkspace Reorder and Sidebar Scroll Deferral
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (13 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 SummaryIntroduces scroll-reveal behavior for the selected workspace row after sidebar reordering, by adding a
Confidence Score: 5/5Safe to merge — the reorder paths are guarded correctly, all three issues from the prior review round are resolved, and both unit and UI test coverage is thorough. The three previously flagged issues — notification filter inversion, passive-shift over-scrolling, and missing nil-selection test — are all addressed cleanly in this revision. moveTabToTop's early-return now correctly computes the pinned-aware target index, moveTabsToTop suppresses the notification when the order is unchanged, and the onReceive guard requires identity equality with the owning tabManager. No new isolation, blocking, or state-ownership problems were introduced. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as UI Action (e.g. Move to Top)
participant TM as TabManager
participant NC as NotificationCenter
participant SB as VerticalTabsSidebar
participant SV as ScrollViewProxy
UI->>TM: moveTabToTop / moveTabsToTop / reorderWorkspace
TM->>TM: mutate tabs (guard: order actually changes)
TM->>NC: post .workspaceOrderDidChange (movedWorkspaceIds: [UUID])
NC-->>SB: onReceive(.workspaceOrderDidChange)
SB->>SB: "guard object === tabManager"
SB->>SB: guard selectedId in movedWorkspaceIds
SB->>SB: "pendingSelectedWorkspaceScrollId = selectedId"
TM-->>SB: onChange(renderContext.workspaceIds)
alt selected workspace index changed
SB->>SB: requestSelectedWorkspaceScroll() sets pendingSelectedWorkspaceScrollId
SB->>SV: flushPendingSelectedWorkspaceScroll() proxy.scrollTo(selectedId)
else selected workspace index unchanged
SB->>SV: flushPendingSelectedWorkspaceScroll() flushes notification-set pending ID
end
SB-->>SB: onPreferenceChange(SidebarWorkspaceRowIdsPreferenceKey) flushPendingSelectedWorkspaceScroll() if row now laid out
Reviews (8): Last reviewed commit: "Scroll selected workspace on index shift..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return true | ||
| } | ||
|
|
||
| return oldIndex != newIndex |
There was a problem hiding this comment.
oldIndex != newIndex fires even when a completely unrelated workspace is reordered and merely bumps the selected workspace one position. If the selected workspace is already fully visible when the reorder happens, proxy.scrollTo will still be enqueued. SwiftUI's minimum-reveal scroll usually makes this a no-op, but the scroll is applied after any reorder animation completes — so during an in-flight animation the row can momentarily appear outside the laid-out bounds, causing a small visible jump. The PR description flags this as a known risk, so flagging for visibility rather than blocking.
There was a problem hiding this comment.
Fixed by making passive selected-index shifts a no-scroll path and adding an explicit workspace-order notification for workspaces that were actually reordered.
— Claude Code
| final class SidebarSelectedWorkspaceScrollPolicyTests: XCTestCase { | ||
| func testRequestsScrollWhenSelectedWorkspaceFirstAppears() { | ||
| XCTAssertTrue( | ||
| SidebarSelectedWorkspaceScrollPolicy.shouldScrollSelectedWorkspace( | ||
| selectedWorkspaceId: "b", | ||
| oldWorkspaceIds: ["a"], | ||
| newWorkspaceIds: ["a", "b"] | ||
| ) | ||
| ) | ||
| } | ||
|
|
||
| func testRequestsScrollWhenSelectedWorkspaceMovesToTop() { | ||
| XCTAssertTrue( | ||
| SidebarSelectedWorkspaceScrollPolicy.shouldScrollSelectedWorkspace( | ||
| selectedWorkspaceId: "c", | ||
| oldWorkspaceIds: ["a", "b", "c"], | ||
| newWorkspaceIds: ["c", "a", "b"] | ||
| ) | ||
| ) | ||
| } | ||
|
|
||
| func testRequestsScrollWhenAnotherReorderMovesSelectedWorkspaceIndex() { | ||
| XCTAssertTrue( | ||
| SidebarSelectedWorkspaceScrollPolicy.shouldScrollSelectedWorkspace( | ||
| selectedWorkspaceId: "b", | ||
| oldWorkspaceIds: ["a", "b", "c"], | ||
| newWorkspaceIds: ["c", "a", "b"] | ||
| ) | ||
| ) | ||
| } | ||
|
|
||
| func testSkipsScrollWhenReorderLeavesSelectedWorkspaceIndexUnchanged() { | ||
| XCTAssertFalse( | ||
| SidebarSelectedWorkspaceScrollPolicy.shouldScrollSelectedWorkspace( | ||
| selectedWorkspaceId: "a", | ||
| oldWorkspaceIds: ["a", "b", "c"], | ||
| newWorkspaceIds: ["a", "c", "b"] | ||
| ) | ||
| ) | ||
| } | ||
|
|
||
| func testSkipsScrollWhenSelectedWorkspaceIsMissing() { | ||
| XCTAssertFalse( | ||
| SidebarSelectedWorkspaceScrollPolicy.shouldScrollSelectedWorkspace( | ||
| selectedWorkspaceId: "b", | ||
| oldWorkspaceIds: ["a", "b"], | ||
| newWorkspaceIds: ["a", "c"] | ||
| ) | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
nil selected workspace
The new SidebarSelectedWorkspaceScrollPolicyTests suite exercises first-appearance, move-to-top, passive shift, unchanged index, and removed-workspace paths — but not the selectedWorkspaceId: nil branch. The policy correctly short-circuits via guard let selectedWorkspaceId and returns false, but an explicit test would lock in the contract that a nil selection never triggers a scroll, which matters if this function is later inlined or refactored.
There was a problem hiding this comment.
Fixed by adding explicit nil-selection coverage for SidebarSelectedWorkspaceScrollPolicy.
— Claude Code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/TabManager.swift (1)
3869-3877:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid emitting
workspaceOrderDidChangewhenmoveTabToTopis a no-op.Line 3876 can fire even when effective order is unchanged (e.g., moving an unpinned workspace already at the first unpinned slot). That creates false reorder events and can trigger unnecessary sidebar reveal/scroll behavior.
Suggested fix
func moveTabToTop(_ tabId: UUID) { guard let index = tabs.firstIndex(where: { $0.id == tabId }) else { return } guard index != 0 else { return } + let previousOrder = tabs.map(\.id) let tab = tabs.remove(at: index) let pinnedCount = tabs.filter { $0.isPinned }.count let insertIndex = tab.isPinned ? 0 : pinnedCount tabs.insert(tab, at: insertIndex) - postWorkspaceOrderDidChange(movedWorkspaceIds: [tabId]) + if tabs.map(\.id) != previousOrder { + postWorkspaceOrderDidChange(movedWorkspaceIds: [tabId]) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/TabManager.swift` around lines 3869 - 3877, The moveTabToTop implementation currently removes the tab then computes pinnedCount/insertIndex and always calls postWorkspaceOrderDidChange even when the effective position doesn't change; to fix, compute pinnedCount and target insertIndex before mutating tabs (use tabs.filter { $0.isPinned }.count and let insertIndex = tab.isPinned ? 0 : pinnedCount), if insertIndex == index just return (no-op) otherwise perform the remove(at: index), insert(tab, at: insertIndex) and then call postWorkspaceOrderDidChange(movedWorkspaceIds: [tabId]) so events are only emitted when the visible order actually changes (refer to moveTabToTop, tabs, pinnedCount, insertIndex, postWorkspaceOrderDidChange).
🤖 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.
Outside diff comments:
In `@Sources/TabManager.swift`:
- Around line 3869-3877: The moveTabToTop implementation currently removes the
tab then computes pinnedCount/insertIndex and always calls
postWorkspaceOrderDidChange even when the effective position doesn't change; to
fix, compute pinnedCount and target insertIndex before mutating tabs (use
tabs.filter { $0.isPinned }.count and let insertIndex = tab.isPinned ? 0 :
pinnedCount), if insertIndex == index just return (no-op) otherwise perform the
remove(at: index), insert(tab, at: insertIndex) and then call
postWorkspaceOrderDidChange(movedWorkspaceIds: [tabId]) so events are only
emitted when the visible order actually changes (refer to moveTabToTop, tabs,
pinnedCount, insertIndex, postWorkspaceOrderDidChange).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 03859eed-b070-419a-8868-d91e0703ed74
📒 Files selected for processing (7)
Sources/ContentView.swiftSources/Search/GlobalSearchDocuments.swiftSources/Sidebar/SidebarState.swiftSources/TabManager.swiftcmuxTests/SidebarWorkspaceSnapshotRefreshPolicyTests.swiftcmuxTests/WorkspaceUnitTests.swiftcmuxUITests/WorkspaceSidebarScrollUITests.swift
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 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 0ad6f29. Configure here.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/WorkspaceUnitTests.swift`:
- Around line 3178-3194: Add an assertion to verify the tab order is unchanged
after the no-op move in
testMoveTabsToTopSkipsNotificationWhenOrderDoesNotChange: after calling
manager.moveTabsToTop([first.id]) assert the ids remain the same (e.g. compare
manager.tabs.map(\.id) to the expected order containing first.id) so the test
not only checks notificationCount but also confirms the workspace order did not
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: 0d563c9a-940c-4c52-a64e-45b7f4a0c33e
📒 Files selected for processing (2)
Sources/TabManager.swiftcmuxTests/WorkspaceUnitTests.swift
| if let manager = notification.object as? TabManager, manager !== tabManager { | ||
| return | ||
| } |
There was a problem hiding this comment.
if let … !== tabManager pattern only rejects notifications whose object is a TabManager that is not our manager; it silently accepts notifications with a nil object or a non-TabManager object (neither of which can be cast). In a multi-window app, any future code that posts .workspaceOrderDidChange without setting object: would be processed by every sidebar instance, setting a spurious pendingSelectedWorkspaceScrollId and triggering an unexpected scroll. The correct guard requires the object to be exactly our tabManager.
| if let manager = notification.object as? TabManager, manager !== tabManager { | |
| return | |
| } | |
| guard let manager = notification.object as? TabManager, manager === tabManager else { | |
| return | |
| } |
There was a problem hiding this comment.
Fixed by requiring the notification object to be a TabManager and identical to this sidebar's tabManager before enqueueing a selected-workspace scroll. Notifications with nil or unrelated objects now return immediately.
— Claude Code
…r-scroll # Conflicts: # cmuxTests/SidebarWorkspaceSnapshotRefreshPolicyTests.swift
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/SidebarWorkspaceSnapshotRefreshPolicyTests.swift`:
- Around line 177-185: The test
testSkipsScrollWhenAnotherReorderOnlyShiftsSelectedWorkspaceIndex is asserting
false while the selected workspace "b" moves index (1 → 2), which contradicts
the scroll-policy contract; update the expectation in the call to
SidebarSelectedWorkspaceScrollPolicy.shouldScrollSelectedWorkspace to assert
true (i.e., the test should expect scrolling) so it matches the intended
behavior for reorders that shift the selected index.
🪄 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: 2357b30e-2e82-4d34-807c-3f9050b4e0b5
📒 Files selected for processing (4)
Sources/ContentView.swiftSources/TabManager.swiftcmuxTests/SidebarWorkspaceSnapshotRefreshPolicyTests.swiftcmuxTests/WorkspaceUnitTests.swift
Addressed by adding the no-op order assertion and CodeRabbit confirmed the inline thread as resolved.
Hotfix release for the 0.64.5 cmux ssh stdin regression. Includes: - #4135 SSH stdin fix (community contribution by @kays0x) - #4154 follow-up Swift interpolation fix in the regression test - #4088 transparent backgrounds for file preview panels - #4083 sidebar reorder visibility - #4094 Cloud VM error guidance - #4120 Pi Vault icon and JSONL titles - f85cc56 command palette settings toggles Co-authored-by: Lawrence Chen <lawrencecchen@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This branch was successfully deployed
1 participant
Summary:\n- Add a UI regression for Cmd+Shift+P Move to Top keeping the moved workspace visible.\n- Reveal the selected workspace when workspace order changes move its index.\n- Cover selected-workspace scroll policy for selected moves and other reorders that shift the selected index.\n- Handle right-sidebar tool panels in global search as title-only so the branch builds on current main.\n\nTests:\n- Passed: ./scripts/reload.sh --tag scrolltop\n- Passed: https://github.com/manaflow-ai/cmux/actions/runs/25789124112\n- PR checks: pending CircleCI and Vercel
Note
Medium Risk
Changes workspace reordering behavior and introduces new order-change notifications that drive sidebar scrolling, which could affect tab ordering and selection UX if edge cases are missed.
Overview
Ensures the sidebar keeps the currently selected workspace visible when the workspace list changes order, by centralizing the scroll decision in
SidebarSelectedWorkspaceScrollPolicyand triggering scroll on explicit order-change events.TabManagernow posts a.workspaceOrderDidChangenotification (withmovedWorkspaceIds) from reorder/move-to-top operations, andContentViewlistens for it to request a scroll when the selected workspace was moved. Adds unit tests for the scroll policy and reorder notifications, plus a UI test covering Cmd+Shift+P “Move to Top” keeping the moved workspace visible.Reviewed by Cursor Bugbot for commit 6705ff2. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Improvements
Tests