Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 21 additions & 4 deletions Sources/ContentView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -9167,10 +9167,21 @@ struct VerticalTabsSidebar: View {
from oldWorkspaceIds: [UUID],
to newWorkspaceIds: [UUID]
) -> Bool {
guard let selectedWorkspaceId = tabManager.selectedTabId else {
return false
SidebarSelectedWorkspaceScrollPolicy.shouldScrollSelectedWorkspace(
selectedWorkspaceId: tabManager.selectedTabId,
oldWorkspaceIds: oldWorkspaceIds,
newWorkspaceIds: newWorkspaceIds
)
}

private func requestSelectedWorkspaceScrollAfterWorkspaceOrderChange(_ notification: Notification) {
guard let manager = notification.object as? TabManager, manager === tabManager else {
return
}
return !oldWorkspaceIds.contains(selectedWorkspaceId) && newWorkspaceIds.contains(selectedWorkspaceId)
guard let selectedWorkspaceId = tabManager.selectedTabId else { return }
let movedWorkspaceIds = notification.userInfo?[WorkspaceOrderChangeNotificationKey.movedWorkspaceIds] as? [UUID] ?? []
guard movedWorkspaceIds.contains(selectedWorkspaceId) else { return }
pendingSelectedWorkspaceScrollId = selectedWorkspaceId
}

private struct WorkspaceListRenderContext {
Expand Down Expand Up @@ -9405,9 +9416,15 @@ struct VerticalTabsSidebar: View {
guard shouldRequestSelectedWorkspaceScrollAfterWorkspaceIdsChange(
from: oldWorkspaceIds,
to: newWorkspaceIds
) else { return }
) else {
flushPendingSelectedWorkspaceScroll(scrollProxy)
return
}
requestSelectedWorkspaceScroll(scrollProxy, workspaceIds: newWorkspaceIds)
}
.onReceive(NotificationCenter.default.publisher(for: .workspaceOrderDidChange)) { notification in
requestSelectedWorkspaceScrollAfterWorkspaceOrderChange(notification)
}
.onPreferenceChange(SidebarWorkspaceRowIdsPreferenceKey.self) { rowIds in
laidOutWorkspaceRowIds = rowIds
flushPendingSelectedWorkspaceScroll(scrollProxy, laidOutWorkspaceRowIds: rowIds)
Expand Down
23 changes: 23 additions & 0 deletions Sources/Sidebar/SidebarState.swift
Original file line number Diff line number Diff line change
Expand Up @@ -52,3 +52,26 @@ enum SidebarResizeInteraction {
sidebarSideHitWidth + contentSideHitWidth
}
}

enum SidebarSelectedWorkspaceScrollPolicy {
static func shouldScrollSelectedWorkspace<ID: Equatable>(
selectedWorkspaceId: ID?,
oldWorkspaceIds: [ID],
newWorkspaceIds: [ID]
) -> Bool {
guard let selectedWorkspaceId,
let newIndex = newWorkspaceIds.firstIndex(of: selectedWorkspaceId) else {
return false
}

guard let oldIndex = oldWorkspaceIds.firstIndex(of: selectedWorkspaceId) else {
return true
}

guard oldIndex != newIndex else {
return false
}

return true
}
}
27 changes: 25 additions & 2 deletions Sources/TabManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,10 @@ enum WorkspaceAutoReorderSettings {
}
}

enum WorkspaceOrderChangeNotificationKey {
static let movedWorkspaceIds = "movedWorkspaceIds"
}

enum LastSurfaceCloseShortcutSettings {
static let key = "closeWorkspaceOnLastSurfaceShortcut"
// Keep the legacy stored meaning so existing values still map to the same
Expand Down Expand Up @@ -3877,23 +3881,30 @@ class TabManager: ObservableObject {

func moveTabToTop(_ tabId: UUID) {
guard let index = tabs.firstIndex(where: { $0.id == tabId }) else { return }
guard index != 0 else { return }
let tab = tabs.remove(at: index)
let tab = tabs[index]
let targetIndex = tab.isPinned ? 0 : tabs.filter { $0.isPinned }.count
guard index != targetIndex else { return }
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])
Comment thread
cursor[bot] marked this conversation as resolved.
}

func moveTabsToTop(_ tabIds: Set<UUID>) {
guard !tabIds.isEmpty else { return }
let selectedTabs = tabs.filter { tabIds.contains($0.id) }
guard !selectedTabs.isEmpty else { return }
let previousOrder = tabs.map(\.id)
let remainingTabs = tabs.filter { !tabIds.contains($0.id) }
let selectedPinned = selectedTabs.filter { $0.isPinned }
let selectedUnpinned = selectedTabs.filter { !$0.isPinned }
let remainingPinned = remainingTabs.filter { $0.isPinned }
let remainingUnpinned = remainingTabs.filter { !$0.isPinned }
tabs = selectedPinned + remainingPinned + selectedUnpinned + remainingUnpinned
if tabs.map(\.id) != previousOrder {
postWorkspaceOrderDidChange(movedWorkspaceIds: selectedTabs.map(\.id))
}
}

func moveTabToTopForNotification(_ tabId: UUID) {
Expand All @@ -3904,6 +3915,7 @@ class TabManager: ObservableObject {
guard !tab.isPinned else { return }
tabs.remove(at: index)
tabs.insert(tab, at: pinnedCount)
postWorkspaceOrderDidChange(movedWorkspaceIds: [tabId])
}

@discardableResult
Expand All @@ -3917,9 +3929,19 @@ class TabManager: ObservableObject {

tabs.remove(at: currentIndex)
tabs.insert(workspace, at: clamped)
postWorkspaceOrderDidChange(movedWorkspaceIds: [tabId])
return true
}

private func postWorkspaceOrderDidChange(movedWorkspaceIds: [UUID]) {
guard !movedWorkspaceIds.isEmpty else { return }
NotificationCenter.default.post(
name: .workspaceOrderDidChange,
object: self,
userInfo: [WorkspaceOrderChangeNotificationKey.movedWorkspaceIds: movedWorkspaceIds]
)
}

@discardableResult
func reorderWorkspace(tabId: UUID, before beforeId: UUID? = nil, after afterId: UUID? = nil) -> Bool {
guard tabs.contains(where: { $0.id == tabId }) else { return false }
Expand Down Expand Up @@ -7559,4 +7581,5 @@ extension Notification.Name {
static let webViewDidReceiveClick = Notification.Name("webViewDidReceiveClick")
static let terminalPortalVisibilityDidChange = Notification.Name("cmux.terminalPortalVisibilityDidChange")
static let browserPortalRegistryDidChange = Notification.Name("cmux.browserPortalRegistryDidChange")
static let workspaceOrderDidChange = Notification.Name("cmux.workspaceOrderDidChange")
}
62 changes: 62 additions & 0 deletions cmuxTests/SidebarWorkspaceSnapshotRefreshPolicyTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,68 @@ final class SidebarWorkspaceSnapshotRefreshPolicyTests: XCTestCase {
}
}

final class SidebarSelectedWorkspaceScrollPolicyTests: XCTestCase {
func testSkipsScrollWhenSelectedWorkspaceIdIsNil() {
XCTAssertFalse(
SidebarSelectedWorkspaceScrollPolicy.shouldScrollSelectedWorkspace(
selectedWorkspaceId: nil as String?,
oldWorkspaceIds: ["a"],
newWorkspaceIds: ["a"]
)
)
}

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 testRequestsScrollWhenAnotherReorderShiftsSelectedWorkspaceIndex() {
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"]
)
)
}
}
Comment on lines +146 to +206

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 Missing test for 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed by adding explicit nil-selection coverage for SidebarSelectedWorkspaceScrollPolicy.

— Claude Code


final class SidebarTabItemPresentationResolutionPolicyTests: XCTestCase {
func testFrozenContextMenuPresentationDoesNotSuppressLiveNotificationState() {
let tabId = UUID()
Expand Down
106 changes: 106 additions & 0 deletions cmuxTests/WorkspaceUnitTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -3183,6 +3183,112 @@ final class SidebarWorkspaceAuxiliaryDetailVisibilityTests: XCTestCase {


final class WorkspaceReorderTests: XCTestCase {
@MainActor
func testReorderWorkspacePostsMovedWorkspaceId() {
let manager = TabManager()
let second = manager.addWorkspace()
_ = manager.addWorkspace()
var observedMovedIds: [UUID] = []
let token = NotificationCenter.default.addObserver(
forName: .workspaceOrderDidChange,
object: manager,
queue: nil
) { notification in
observedMovedIds = notification.userInfo?[WorkspaceOrderChangeNotificationKey.movedWorkspaceIds] as? [UUID] ?? []
}
defer { NotificationCenter.default.removeObserver(token) }

XCTAssertTrue(manager.reorderWorkspace(tabId: second.id, toIndex: 0))

XCTAssertEqual(observedMovedIds, [second.id])
}

@MainActor
func testMoveTabsToTopPostsMovedWorkspaceIds() {
let manager = TabManager()
let first = manager.tabs[0]
let second = manager.addWorkspace()
let third = manager.addWorkspace()
var observedMovedIds: [UUID] = []
let token = NotificationCenter.default.addObserver(
forName: .workspaceOrderDidChange,
object: manager,
queue: nil
) { notification in
observedMovedIds = notification.userInfo?[WorkspaceOrderChangeNotificationKey.movedWorkspaceIds] as? [UUID] ?? []
}
defer { NotificationCenter.default.removeObserver(token) }

manager.moveTabsToTop([third.id, second.id])

XCTAssertEqual(manager.tabs.map(\.id), [second.id, third.id, first.id])
XCTAssertEqual(observedMovedIds, [second.id, third.id])
}

@MainActor
func testMoveTabsToTopSkipsNotificationWhenOrderDoesNotChange() {
let manager = TabManager()
let first = manager.tabs[0]
var notificationCount = 0
let token = NotificationCenter.default.addObserver(
forName: .workspaceOrderDidChange,
object: manager,
queue: nil
) { _ in
notificationCount += 1
}
defer { NotificationCenter.default.removeObserver(token) }

manager.moveTabsToTop([first.id])

XCTAssertEqual(manager.tabs.map(\.id), [first.id])
XCTAssertEqual(notificationCount, 0)
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

@MainActor
func testMoveTabToTopPostsMovedWorkspaceIdWhenOrderChanges() {
let manager = TabManager()
let first = manager.tabs[0]
let second = manager.addWorkspace()
var observedMovedIds: [UUID] = []
let token = NotificationCenter.default.addObserver(
forName: .workspaceOrderDidChange,
object: manager,
queue: nil
) { notification in
observedMovedIds = notification.userInfo?[WorkspaceOrderChangeNotificationKey.movedWorkspaceIds] as? [UUID] ?? []
}
defer { NotificationCenter.default.removeObserver(token) }

manager.moveTabToTop(second.id)

XCTAssertEqual(manager.tabs.map(\.id), [second.id, first.id])
XCTAssertEqual(observedMovedIds, [second.id])
}

@MainActor
func testMoveTabToTopSkipsNotificationWhenUnpinnedAlreadyFirstBelowPinnedWorkspaces() {
let manager = TabManager()
let pinned = manager.tabs[0]
manager.setPinned(pinned, pinned: true)
let firstUnpinned = manager.addWorkspace()
_ = manager.addWorkspace()
var notificationCount = 0
let token = NotificationCenter.default.addObserver(
forName: .workspaceOrderDidChange,
object: manager,
queue: nil
) { _ in
notificationCount += 1
}
defer { NotificationCenter.default.removeObserver(token) }

manager.moveTabToTop(firstUnpinned.id)

XCTAssertEqual(manager.tabs.map(\.id).prefix(2), [pinned.id, firstUnpinned.id])
XCTAssertEqual(notificationCount, 0)
}

@MainActor
func testReorderWorkspaceMovesWorkspaceToRequestedIndex() {
let manager = TabManager()
Expand Down
57 changes: 57 additions & 0 deletions cmuxUITests/WorkspaceSidebarScrollUITests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,33 @@ final class WorkspaceSidebarScrollUITests: XCTestCase {
)
}

func testCommandPaletteMoveWorkspaceToTopKeepsMovedWorkspaceVisible() {
let app = XCUIApplication()
configureLaunch(app)
launchAndEnsureRunning(app)
XCTAssertTrue(waitForWindowCount(atLeast: 1, app: app, timeout: 8.0), "Expected a main window")
XCTAssertTrue(
waitForWorkspaceRowHittable(index: 1, count: 1, app: app, timeout: 8.0),
"Expected the initial workspace row to be visible"
)

let workspaceCount = 20
for expectedCount in 2...workspaceCount {
app.typeKey("n", modifierFlags: [.command])
XCTAssertTrue(
waitForWorkspaceRowHittable(index: expectedCount, count: expectedCount, app: app, timeout: 6.0),
"Expected the newly selected workspace \(expectedCount) to be visible"
)
}

runCommandPaletteMoveToTop(app: app)

XCTAssertTrue(
waitForWorkspaceRowHittable(index: 1, count: workspaceCount, app: app, timeout: 6.0),
"Expected Cmd+Shift+P Move to Top to scroll the moved workspace back into view"
)
}

func testSidebarScrollerVisibilityFollowsWorkspaceOverflow() {
let app = XCUIApplication()
configureLaunch(app)
Expand Down Expand Up @@ -158,6 +185,36 @@ final class WorkspaceSidebarScrollUITests: XCTestCase {
}
}

private func runCommandPaletteMoveToTop(app: XCUIApplication) {
let searchField = app.textFields["CommandPaletteSearchField"].firstMatch
app.typeKey("p", modifierFlags: [.command, .shift])
XCTAssertTrue(searchField.waitForExistence(timeout: 5.0), "Expected command palette search field")
searchField.click()
searchField.typeText("move to top")

let row = app.descendants(matching: .any)
.matching(
NSPredicate(
format: "identifier BEGINSWITH %@ AND value == %@",
"CommandPaletteResultRow.",
"palette.moveWorkspaceToTop"
)
)
.firstMatch
XCTAssertTrue(row.waitForExistence(timeout: 5.0), "Expected Move to Top command palette row")
row.click()
XCTAssertTrue(
waitForNonExistence(searchField, timeout: 5.0),
"Expected command palette to dismiss after Move to Top"
)
}

private func waitForNonExistence(_ element: XCUIElement, timeout: TimeInterval) -> Bool {
pollUntil(timeout: timeout) {
!element.exists
}
}

private func revealSidebarVerticalScroller(
app: XCUIApplication,
sidebar: XCUIElement,
Expand Down
Loading