From 0761080b569423e7a49fc737f7ac18f886ad7d72 Mon Sep 17 00:00:00 2001 From: Lawrence Chen Date: Mon, 20 Apr 2026 00:05:10 -0700 Subject: [PATCH 1/5] Make workspace sidebar lazy with @Observable drag state Switches the left sidebar to LazyVStack for O(visible) layout instead of O(total) workspaces. Previously the stack had to stay eager because drag mutations (draggedTabId, dropIndicator) were @State on the ancestor, and the 60fps writes during drag fed back through the lazy layout cache and pegged the main thread (https://github.com/manaflow-ai/cmux/issues/2586). Drag transient state now lives on a @Observable @MainActor SidebarDragState class. Reads are tracked per property, so high-frequency writes invalidate only the tiny overlays that depend on them, never the sidebar body or the LazyVStack layout. - Add SidebarDragState (@Observable) owning draggedTabId + dropIndicator. - Replace @State draggedTabId/dropIndicator/frozenTabItemPresentation on VerticalTabsSidebar with a single @State SidebarDragState. - Replace the @Binding chain through TabItemView, SidebarEmptyArea, and both SidebarTabDropDelegate and SidebarBonsplitTabDropDelegate with a direct SidebarDragState reference. - Extract SidebarTabDropIndicatorOverlay and SidebarTabDragOpacityModifier so the per-frame dropIndicator/draggedTabId reads stay inside isolated views and don't invalidate the 1600-line TabItemView body. - Delete SidebarTabItemPresentationSnapshot and the frozen-presentation workflow (context-menu freeze was a workaround for binding churn that @Observable's per-property tracking makes unnecessary). - Flip VStack to LazyVStack inside the sidebar ScrollView. --- Sources/ContentView.swift | 318 ++++++++++++++++++++------------------ 1 file changed, 169 insertions(+), 149 deletions(-) diff --git a/Sources/ContentView.swift b/Sources/ContentView.swift index 1799a8618ed3..79fe4da92f00 100644 --- a/Sources/ContentView.swift +++ b/Sources/ContentView.swift @@ -9571,25 +9571,65 @@ private final class SidebarTabItemSettingsStore: ObservableObject { } } -struct SidebarTabItemPresentationSnapshot: Equatable { +/// Transient sidebar drag/drop state, owned by `VerticalTabsSidebar` and passed +/// by reference into rows and drop delegates. `@Observable` gives per-property +/// tracking: writing `draggedTabId` or `dropIndicator` during drag invalidates +/// only the views that read those properties (the dragged row's opacity and the +/// drop-indicator overlays), never the sidebar body or the `LazyVStack` itself. +/// That invariant is what prevents the layout-invalidation loop that caused +/// https://github.com/manaflow-ai/cmux/issues/2586. +@MainActor +@Observable +final class SidebarDragState { + var draggedTabId: UUID? + var dropIndicator: SidebarDropIndicator? + + init() {} +} + +/// Dedicated view that reads `dragState.dropIndicator` (mutates at ~60fps during +/// drag). Isolating the read here means only this tiny overlay invalidates per +/// frame, not the enclosing `TabItemView` body. +private struct SidebarTabDropIndicatorOverlay: View { + let dragState: SidebarDragState let tabId: UUID - let unreadCount: Int - let latestNotificationText: String? - let showsModifierShortcutHints: Bool + let index: Int + let rowSpacing: CGFloat + let tabManager: TabManager + + var body: some View { + if showsCenteredTopDropIndicator { + Rectangle() + .fill(cmuxAccentColor()) + .frame(height: 2) + .padding(.horizontal, 8) + .offset(y: index == 0 ? 0 : -(rowSpacing / 2)) + } + } + + private var showsCenteredTopDropIndicator: Bool { + guard dragState.draggedTabId != nil, let indicator = dragState.dropIndicator else { return false } + if indicator.tabId == tabId && indicator.edge == .top { + return true + } + guard indicator.edge == .bottom, + let currentIndex = tabManager.tabs.firstIndex(where: { $0.id == tabId }), + currentIndex > 0 + else { + return false + } + return tabManager.tabs[currentIndex - 1].id == indicator.tabId + } } -struct SidebarTabItemPresentationResolutionPolicy { - static func resolved( - live: SidebarTabItemPresentationSnapshot, - frozen: SidebarTabItemPresentationSnapshot? - ) -> SidebarTabItemPresentationSnapshot { - guard let frozen, frozen.tabId == live.tabId else { return live } - return SidebarTabItemPresentationSnapshot( - tabId: live.tabId, - unreadCount: live.unreadCount, - latestNotificationText: live.latestNotificationText, - showsModifierShortcutHints: frozen.showsModifierShortcutHints - ) +/// Dedicated view that reads `dragState.draggedTabId` so only the row's opacity +/// layer invalidates on drag start/end, not the row's complex body. +private struct SidebarTabDragOpacityModifier: ViewModifier { + let dragState: SidebarDragState + let tabId: UUID + + func body(content: Content) -> some View { + content.opacity(dragState.draggedTabId == tabId ? 0.6 : 1) } } @@ -9611,9 +9651,7 @@ struct VerticalTabsSidebar: View { @StateObject private var dragFailsafeMonitor = SidebarDragFailsafeMonitor() @StateObject private var tabItemSettingsStore = SidebarTabItemSettingsStore() @ObservedObject private var keyboardShortcutSettingsObserver = KeyboardShortcutSettingsObserver.shared - @State private var draggedTabId: UUID? - @State private var dropIndicator: SidebarDropIndicator? - @State private var frozenTabItemPresentation: SidebarTabItemPresentationSnapshot? + @State private var dragState = SidebarDragState() @State private var terminalScrollBarVisibilityGeneration: UInt64 = 0 @State private var laidOutWorkspaceRowIds: Set = [] @State private var pendingSelectedWorkspaceScrollId: UUID? @@ -9634,6 +9672,25 @@ struct VerticalTabsSidebar: View { MinimalModeChromeMetrics.titlebarHeight } + /// Adapter binding for unmigrated consumers (extension sidebar drop + /// delegates, bonsplit overlays) that still expect @Binding. Reads + /// flow through `dragState.draggedTabId` so @Observable per-property + /// tracking still applies to whoever calls the binding's get. + private var draggedTabIdBinding: Binding { + Binding( + get: { dragState.draggedTabId }, + set: { dragState.draggedTabId = $0 } + ) + } + + /// Adapter binding mirroring `draggedTabIdBinding`. See its doc comment. + private var dropIndicatorBinding: Binding { + Binding( + get: { dragState.dropIndicator }, + set: { dragState.dropIndicator = $0 } + ) + } + private var sidebarTopScrimHeight: CGFloat { SidebarWorkspaceListMetrics.topScrimHeight } @@ -9784,8 +9841,8 @@ struct VerticalTabsSidebar: View { ) .onAppear { modifierKeyMonitor.start() - draggedTabId = nil - dropIndicator = nil + dragState.draggedTabId = nil + dragState.dropIndicator = nil SidebarDragLifecycleNotification.postStateDidChange( tabId: nil, reason: "sidebar_appear" @@ -9795,14 +9852,14 @@ struct VerticalTabsSidebar: View { modifierKeyMonitor.stop() dragAutoScrollController.stop() dragFailsafeMonitor.stop() - draggedTabId = nil - dropIndicator = nil + dragState.draggedTabId = nil + dragState.dropIndicator = nil SidebarDragLifecycleNotification.postStateDidChange( tabId: nil, reason: "sidebar_disappear" ) } - .onChange(of: draggedTabId) { newDraggedTabId in + .onChange(of: dragState.draggedTabId) { newDraggedTabId in SidebarDragLifecycleNotification.postStateDidChange( tabId: newDraggedTabId, reason: "drag_state_change" @@ -9818,20 +9875,15 @@ struct VerticalTabsSidebar: View { } dragFailsafeMonitor.stop() dragAutoScrollController.stop() - dropIndicator = nil - } - .onChange(of: tabs.map(\.id)) { tabIds in - guard let frozenTabItemPresentation, - !tabIds.contains(frozenTabItemPresentation.tabId) else { return } - self.frozenTabItemPresentation = nil + dragState.dropIndicator = nil } .onReceive(NotificationCenter.default.publisher(for: SidebarDragLifecycleNotification.requestClear)) { notification in - guard draggedTabId != nil else { return } + guard dragState.draggedTabId != nil else { return } let reason = SidebarDragLifecycleNotification.reason(from: notification) #if DEBUG - cmuxDebugLog("sidebar.dragClear tab=\(debugShortSidebarTabId(draggedTabId)) reason=\(reason)") + cmuxDebugLog("sidebar.dragClear tab=\(debugShortSidebarTabId(dragState.draggedTabId)) reason=\(reason)") #endif - draggedTabId = nil + dragState.draggedTabId = nil } .onReceive( NotificationCenter.default.publisher(for: Workspace.terminalScrollBarHiddenDidChangeNotification) @@ -9890,19 +9942,18 @@ struct VerticalTabsSidebar: View { .background(TitlebarDoubleClickMonitorView()) } .overlay(alignment: .top) { - if draggedTabId != nil, let firstWorkspaceId = renderContext.workspaceIds.first { + if dragState.draggedTabId != nil, let firstWorkspaceId = renderContext.workspaceIds.first { Color.clear .contentShape(Rectangle()) .frame(height: scrollInsets.top + 8) .onDrop(of: SidebarTabDragPayload.dropContentTypes, delegate: SidebarTabDropDelegate( targetTabId: firstWorkspaceId, tabManager: tabManager, - draggedTabId: $draggedTabId, + dragState: dragState, selectedTabIds: $selectedTabIds, lastSidebarSelectionIndex: $lastSidebarSelectionIndex, targetRowHeight: nil, - dragAutoScrollController: dragAutoScrollController, - dropIndicator: $dropIndicator + dragAutoScrollController: dragAutoScrollController )) } } @@ -9994,8 +10045,7 @@ struct VerticalTabsSidebar: View { selectedTabIds: $selectedTabIds, lastSidebarSelectionIndex: $lastSidebarSelectionIndex, dragAutoScrollController: dragAutoScrollController, - draggedTabId: $draggedTabId, - dropIndicator: $dropIndicator + dragState: dragState ) .frame(maxWidth: .infinity, minHeight: 48) } @@ -10254,8 +10304,8 @@ struct VerticalTabsSidebar: View { rowSpacing: tabRowSpacing, orderedRows: dropRows, dragAutoScrollController: dragAutoScrollController, - draggedTabId: $draggedTabId, - dropIndicator: $dropIndicator, + draggedTabId: draggedTabIdBinding, + dropIndicator: dropIndicatorBinding, onNewTab: onNewTab, onMove: { move in handleExtensionSidebarMutation(.moveWorkspace(move)) @@ -10345,20 +10395,20 @@ struct VerticalTabsSidebar: View { .buttonStyle(.plain) .frame(maxWidth: .infinity) .safeHelp(row.title) - .opacity(draggedTabId == row.workspaceId ? 0.55 : 1) + .opacity(dragState.draggedTabId == row.workspaceId ? 0.55 : 1) .onDrag { - draggedTabId = row.workspaceId - dropIndicator = nil + dragState.draggedTabId = row.workspaceId + dragState.dropIndicator = nil return SidebarTabDragPayload.provider(for: row.workspaceId) } .internalOnlyTabDrag() .onDrop(of: SidebarTabDragPayload.dropContentTypes, delegate: ExtensionSidebarBrowserStackDropDelegate( targetWorkspaceId: row.workspaceId, orderedRows: dropRows, - draggedTabId: $draggedTabId, + draggedTabId: draggedTabIdBinding, targetRowHeight: targetRowHeight, dragAutoScrollController: dragAutoScrollController, - dropIndicator: $dropIndicator, + dropIndicator: dropIndicatorBinding, onMove: { move in handleExtensionSidebarMutation(.moveWorkspace(move)) } @@ -10424,20 +10474,20 @@ struct VerticalTabsSidebar: View { .contentShape(Rectangle()) } .buttonStyle(.plain) - .opacity(draggedTabId == row.workspaceId ? 0.55 : 1) + .opacity(dragState.draggedTabId == row.workspaceId ? 0.55 : 1) .onDrag { - draggedTabId = row.workspaceId - dropIndicator = nil + dragState.draggedTabId = row.workspaceId + dragState.dropIndicator = nil return SidebarTabDragPayload.provider(for: row.workspaceId) } .internalOnlyTabDrag() .onDrop(of: SidebarTabDragPayload.dropContentTypes, delegate: ExtensionSidebarBrowserStackDropDelegate( targetWorkspaceId: row.workspaceId, orderedRows: dropRows, - draggedTabId: $draggedTabId, + draggedTabId: draggedTabIdBinding, targetRowHeight: targetRowHeight, dragAutoScrollController: dragAutoScrollController, - dropIndicator: $dropIndicator, + dropIndicator: dropIndicatorBinding, onMove: { move in handleExtensionSidebarMutation(.moveWorkspace(move)) } @@ -10468,7 +10518,7 @@ struct VerticalTabsSidebar: View { row: CmuxExtensionSidebarRenderRow, edge: SidebarDropEdge ) -> some View { - if dropIndicator == SidebarDropIndicator(tabId: row.workspaceId, edge: edge) { + if dragState.dropIndicator == SidebarDropIndicator(tabId: row.workspaceId, edge: edge) { Rectangle() .fill(cmuxAccentColor()) .frame(height: 2) @@ -10751,8 +10801,7 @@ struct VerticalTabsSidebar: View { selectedTabIds: $selectedTabIds, lastSidebarSelectionIndex: $lastSidebarSelectionIndex, dragAutoScrollController: dragAutoScrollController, - draggedTabId: $draggedTabId, - dropIndicator: $dropIndicator + dragState: dragState ) .frame(maxWidth: .infinity, maxHeight: .infinity) } @@ -10760,9 +10809,11 @@ struct VerticalTabsSidebar: View { } private func workspaceRows(renderContext: WorkspaceListRenderContext) -> some View { - // Workspaces are bounded, so prefer a non-lazy stack here. - // LazyVStack + drag-state invalidations can recurse through layout. - VStack(spacing: tabRowSpacing) { + // LazyVStack is safe here because `dragState` is @Observable: + // drag mutations at 60fps invalidate only the rows/overlays that + // read them, never this sidebar body. See SidebarDragState and + // https://github.com/manaflow-ai/cmux/issues/2586. + LazyVStack(spacing: tabRowSpacing) { ForEach(renderContext.tabs, id: \.id) { tab in workspaceRow(tab, renderContext: renderContext) } @@ -10808,7 +10859,7 @@ struct VerticalTabsSidebar: View { }, selectedTabIds: $selectedTabIds, lastSidebarSelectionIndex: $lastSidebarSelectionIndex, - dropIndicator: $dropIndicator, + dropIndicator: dropIndicatorBinding, updateAutoscroll: { dragAutoScrollController.updateFromDragLocation() }, @@ -10869,19 +10920,6 @@ struct VerticalTabsSidebar: View { return trimmed.isEmpty ? nil : trimmed }() let liveShowsModifierShortcutHints = modifierKeyMonitor.isModifierPressed - let livePresentation = SidebarTabItemPresentationSnapshot( - tabId: tab.id, - unreadCount: liveUnreadCount, - latestNotificationText: liveLatestNotificationText, - showsModifierShortcutHints: liveShowsModifierShortcutHints - ) - let frozenPresentation = frozenTabItemPresentation?.tabId == tab.id - ? frozenTabItemPresentation - : nil - let resolvedPresentation = SidebarTabItemPresentationResolutionPolicy.resolved( - live: livePresentation, - frozen: frozenPresentation - ) return TabItemView( tabManager: tabManager, @@ -10896,25 +10934,22 @@ struct VerticalTabsSidebar: View { workspaceShortcutModifierSymbol: renderContext.workspaceNumberShortcut.numberedDigitHintPrefix, canCloseWorkspace: renderContext.canCloseWorkspace, accessibilityWorkspaceCount: renderContext.workspaceCount, - unreadCount: resolvedPresentation.unreadCount, - latestNotificationText: resolvedPresentation.latestNotificationText, + unreadCount: liveUnreadCount, + latestNotificationText: liveLatestNotificationText, rowSpacing: tabRowSpacing, setSelectionToTabs: { selection = .tabs }, selectedTabIds: $selectedTabIds, lastSidebarSelectionIndex: $lastSidebarSelectionIndex, - showsModifierShortcutHints: resolvedPresentation.showsModifierShortcutHints, + showsModifierShortcutHints: liveShowsModifierShortcutHints, dragAutoScrollController: dragAutoScrollController, - draggedTabId: $draggedTabId, - dropIndicator: $dropIndicator, + dragState: dragState, contextMenuWorkspaceIds: contextMenuWorkspaceIds, remoteContextMenuWorkspaceIds: remoteContextMenuWorkspaceIds, allRemoteContextMenuTargetsConnecting: allRemoteContextMenuTargetsConnecting, allRemoteContextMenuTargetsDisconnected: allRemoteContextMenuTargetsDisconnected, allContextMenuWorkspacesHideTerminalScrollBar: allContextMenuWorkspacesHideTerminalScrollBar, contextMenuPinState: contextMenuPinState, - settings: renderContext.tabItemSettings, - livePresentation: livePresentation, - frozenPresentation: $frozenTabItemPresentation + settings: renderContext.tabItemSettings ) .equatable() .id(tab.id) @@ -13125,8 +13160,7 @@ private struct SidebarEmptyArea: View { @Binding var selectedTabIds: Set @Binding var lastSidebarSelectionIndex: Int? let dragAutoScrollController: SidebarDragAutoScrollController - @Binding var draggedTabId: UUID? - @Binding var dropIndicator: SidebarDropIndicator? + let dragState: SidebarDragState var body: some View { Color.clear @@ -13143,14 +13177,24 @@ private struct SidebarEmptyArea: View { .onDrop(of: SidebarTabDragPayload.dropContentTypes, delegate: SidebarTabDropDelegate( targetTabId: nil, tabManager: tabManager, - draggedTabId: $draggedTabId, + dragState: dragState, selectedTabIds: $selectedTabIds, lastSidebarSelectionIndex: $lastSidebarSelectionIndex, targetRowHeight: nil, - dragAutoScrollController: dragAutoScrollController, - dropIndicator: $dropIndicator + dragAutoScrollController: dragAutoScrollController )) - .overlay { SidebarBonsplitTabNewWorkspaceDropOverlay(tabManager: tabManager, selectedTabIds: $selectedTabIds, lastSidebarSelectionIndex: $lastSidebarSelectionIndex, dropIndicator: $dropIndicator).frame(maxWidth: .infinity, maxHeight: .infinity) } + .overlay { + SidebarBonsplitTabNewWorkspaceDropOverlay( + tabManager: tabManager, + selectedTabIds: $selectedTabIds, + lastSidebarSelectionIndex: $lastSidebarSelectionIndex, + dropIndicator: Binding( + get: { dragState.dropIndicator }, + set: { dragState.dropIndicator = $0 } + ) + ) + .frame(maxWidth: .infinity, maxHeight: .infinity) + } .overlay(alignment: .top) { if shouldShowTopDropIndicator { Rectangle() @@ -13163,7 +13207,7 @@ private struct SidebarEmptyArea: View { } private var shouldShowTopDropIndicator: Bool { - guard let indicator = dropIndicator else { return false } + guard dragState.draggedTabId != nil, let indicator = dragState.dropIndicator else { return false } if indicator.tabId == nil { return true } @@ -13387,8 +13431,7 @@ private struct TabItemView: View, Equatable { @Binding var lastSidebarSelectionIndex: Int? let showsModifierShortcutHints: Bool let dragAutoScrollController: SidebarDragAutoScrollController - @Binding var draggedTabId: UUID? - @Binding var dropIndicator: SidebarDropIndicator? + let dragState: SidebarDragState let contextMenuWorkspaceIds: [UUID] let remoteContextMenuWorkspaceIds: [UUID] let allRemoteContextMenuTargetsConnecting: Bool @@ -13396,8 +13439,6 @@ private struct TabItemView: View, Equatable { let allContextMenuWorkspacesHideTerminalScrollBar: Bool let contextMenuPinState: WorkspaceActionDispatcher.PinState? let settings: SidebarTabItemSettingsSnapshot - let livePresentation: SidebarTabItemPresentationSnapshot - @Binding var frozenPresentation: SidebarTabItemPresentationSnapshot? @State private var workspaceSnapshotStorage: SidebarWorkspaceSnapshotBuilder.Snapshot? @StateObject private var contextMenuState = SidebarTabItemContextMenuState() @State private var rowInteractionState = SidebarWorkspaceRowInteractionState() @@ -13409,10 +13450,6 @@ private struct TabItemView: View, Equatable { selectedTabIds.contains(tab.id) } - private var isBeingDragged: Bool { - draggedTabId == tab.id - } - private var sidebarShortcutHintXOffset: Double { settings.sidebarShortcutHintXOffset } @@ -13624,6 +13661,18 @@ private struct TabItemView: View, Equatable { } } + private var rowHeightProbe: some View { + GeometryReader { proxy in + Color.clear + .onAppear { + rowHeight = max(proxy.size.height, 1) + } + .onChange(of: proxy.size.height) { newHeight in + rowHeight = max(newHeight, 1) + } + } + } + @ViewBuilder private var remoteWorkspaceSection: some View { let workspaceSnapshot = self.workspaceSnapshot @@ -13998,19 +14047,9 @@ private struct TabItemView: View, Equatable { } .shortcutHintVisibilityAnimation(value: showsWorkspaceShortcutHint) .padding(.horizontal, 6) - .background { - GeometryReader { proxy in - Color.clear - .onAppear { - rowHeight = max(proxy.size.height, 1) - } - .onChange(of: proxy.size.height) { newHeight in - rowHeight = max(newHeight, 1) - } - } - } + .background { rowHeightProbe } .contentShape(Rectangle()) - .opacity(isBeingDragged ? 0.6 : 1) + .modifier(SidebarTabDragOpacityModifier(dragState: dragState, tabId: tab.id)) .overlay { SidebarWorkspaceRowHoverTracker(rowInteractionState: $rowInteractionState) } @@ -14023,13 +14062,13 @@ private struct TabItemView: View, Equatable { } } .overlay(alignment: .top) { - if showsCenteredTopDropIndicator { - Rectangle() - .fill(cmuxAccentColor()) - .frame(height: 2) - .padding(.horizontal, 8) - .offset(y: index == 0 ? 0 : -(rowSpacing / 2)) - } + SidebarTabDropIndicatorOverlay( + dragState: dragState, + tabId: tab.id, + index: index, + rowSpacing: rowSpacing, + tabManager: tabManager + ) } .onAppear { refreshWorkspaceSnapshot(force: true) @@ -14088,20 +14127,19 @@ private struct TabItemView: View, Equatable { #if DEBUG cmuxDebugLog("sidebar.onDrag tab=\(tab.id.uuidString.prefix(5))") #endif - draggedTabId = tab.id - dropIndicator = nil + dragState.draggedTabId = tab.id + dragState.dropIndicator = nil return SidebarTabDragPayload.provider(for: tab.id) } .internalOnlyTabDrag() .onDrop(of: SidebarTabDragPayload.dropContentTypes, delegate: SidebarTabDropDelegate( targetTabId: tab.id, tabManager: tabManager, - draggedTabId: $draggedTabId, + dragState: dragState, selectedTabIds: $selectedTabIds, lastSidebarSelectionIndex: $lastSidebarSelectionIndex, targetRowHeight: rowHeight, - dragAutoScrollController: dragAutoScrollController, - dropIndicator: $dropIndicator + dragAutoScrollController: dragAutoScrollController )) .onDrop(of: BonsplitTabDragPayload.dropContentTypes, delegate: SidebarBonsplitTabDropDelegate( targetWorkspaceId: tab.id, @@ -14128,11 +14166,9 @@ private struct TabItemView: View, Equatable { rowInteractionState.contextMenuDidAppear() contextMenuState.hasDeferredWorkspaceObservationInvalidation = false contextMenuState.pendingWorkspaceSnapshot = nil - frozenPresentation = livePresentation } .onDisappear { rowInteractionState.contextMenuDidDisappear() - frozenPresentation = nil flushDeferredWorkspaceObservationInvalidation() } } @@ -14488,21 +14524,6 @@ private struct TabItemView: View, Equatable { ) ?? NSColor(hex: hex) ?? .gray } - private var showsCenteredTopDropIndicator: Bool { - guard let indicator = dropIndicator else { return false } - if indicator.tabId == tab.id && indicator.edge == .top { - return true - } - - guard indicator.edge == .bottom, - let currentIndex = tabManager.tabs.firstIndex(where: { $0.id == tab.id }), - currentIndex > 0 - else { - return false - } - return tabManager.tabs[currentIndex - 1].id == indicator.tabId - } - private var accessibilityTitle: String { String(localized: "accessibility.workspacePosition", defaultValue: "\(workspaceSnapshot.title), workspace \(index + 1) of \(accessibilityWorkspaceCount)") } @@ -15808,16 +15829,15 @@ private struct SidebarBonsplitTabDropDelegate: DropDelegate { private struct SidebarTabDropDelegate: DropDelegate { let targetTabId: UUID? let tabManager: TabManager - @Binding var draggedTabId: UUID? + let dragState: SidebarDragState @Binding var selectedTabIds: Set @Binding var lastSidebarSelectionIndex: Int? let targetRowHeight: CGFloat? let dragAutoScrollController: SidebarDragAutoScrollController - @Binding var dropIndicator: SidebarDropIndicator? func validateDrop(info: DropInfo) -> Bool { let hasType = info.hasItemsConforming(to: [SidebarTabDragPayload.typeIdentifier]) - let hasDrag = draggedTabId != nil + let hasDrag = dragState.draggedTabId != nil #if DEBUG cmuxDebugLog("sidebar.validateDrop target=\(targetTabId?.uuidString.prefix(5) ?? "end") hasType=\(hasType) hasDrag=\(hasDrag)") #endif @@ -15836,8 +15856,8 @@ private struct SidebarTabDropDelegate: DropDelegate { #if DEBUG cmuxDebugLog("sidebar.dropExited target=\(targetTabId?.uuidString.prefix(5) ?? "end")") #endif - if dropIndicator?.tabId == targetTabId { - dropIndicator = nil + if dragState.dropIndicator?.tabId == targetTabId { + dragState.dropIndicator = nil } } @@ -15847,7 +15867,7 @@ private struct SidebarTabDropDelegate: DropDelegate { #if DEBUG cmuxDebugLog( "sidebar.dropUpdated target=\(targetTabId?.uuidString.prefix(5) ?? "end") " + - "indicator=\(debugIndicator(dropIndicator))" + "indicator=\(debugIndicator(dragState.dropIndicator))" ) #endif return DropProposal(operation: .move) @@ -15855,14 +15875,14 @@ private struct SidebarTabDropDelegate: DropDelegate { func performDrop(info: DropInfo) -> Bool { defer { - draggedTabId = nil - dropIndicator = nil + dragState.draggedTabId = nil + dragState.dropIndicator = nil dragAutoScrollController.stop() } #if DEBUG cmuxDebugLog("sidebar.drop target=\(targetTabId?.uuidString.prefix(5) ?? "end")") #endif - guard let draggedTabId else { + guard let draggedTabId = dragState.draggedTabId else { #if DEBUG cmuxDebugLog("sidebar.drop.abort reason=missingDraggedTab") #endif @@ -15878,14 +15898,14 @@ private struct SidebarTabDropDelegate: DropDelegate { guard let targetIndex = SidebarDropPlanner.targetIndex( draggedTabId: draggedTabId, targetTabId: targetTabId, - indicator: dropIndicator, + indicator: dragState.dropIndicator, tabIds: tabIds, pinnedTabIds: Set(tabManager.tabs.filter(\.isPinned).map(\.id)) ) else { #if DEBUG cmuxDebugLog( "sidebar.drop.abort reason=noTargetIndex tab=\(draggedTabId.uuidString.prefix(5)) " + - "target=\(targetTabId?.uuidString.prefix(5) ?? "end") indicator=\(debugIndicator(dropIndicator))" + "target=\(targetTabId?.uuidString.prefix(5) ?? "end") indicator=\(debugIndicator(dragState.dropIndicator))" ) #endif return false @@ -15917,15 +15937,15 @@ private struct SidebarTabDropDelegate: DropDelegate { let tabIds = tabManager.tabs.map(\.id) let pinnedTabIds = Set(tabManager.tabs.filter(\.isPinned).map(\.id)) let nextIndicator = SidebarDropPlanner.indicator( - draggedTabId: draggedTabId, + draggedTabId: dragState.draggedTabId, targetTabId: targetTabId, tabIds: tabIds, pinnedTabIds: pinnedTabIds, pointerY: targetTabId == nil ? nil : info.location.y, targetHeight: targetRowHeight ) - guard dropIndicator != nextIndicator else { return } - dropIndicator = nextIndicator + guard dragState.dropIndicator != nextIndicator else { return } + dragState.dropIndicator = nextIndicator } private func syncSidebarSelection(preferredSelectedTabId: UUID? = nil) { From 57977ae75e0fe5bfdaa19fe450d8a488a4912c56 Mon Sep 17 00:00:00 2001 From: Aziz Albahar Date: Mon, 25 May 2026 16:33:26 -0700 Subject: [PATCH 2/5] Address PR review: snapshot-boundary, @MainActor, drag-clear - TabItemView and SidebarEmptyArea no longer hold `let dragState: SidebarDragState`. Per the snapshot-boundary rule in CLAUDE.md, rows under a LazyVStack must not hold @Observable store references. Replaced with value snapshots (`isBeingDragged`, `topDropIndicatorVisible`) and closure/delegate bundles that the parent constructs from `dragState` ownership. TabItemView's Equatable conformance now compares the new snapshot fields so unchanged rows still skip re-render. - Deleted SidebarTabDropIndicatorOverlay and SidebarTabDragOpacityModifier (they also held @Observable refs); their work is inlined or moved to a SidebarTabDropIndicatorPredicate helper evaluated in the parent. - Marked SidebarTabDropDelegate @MainActor (Greptile P2): it mutates @MainActor-isolated SidebarDragState properties, so documenting the isolation prevents accidental removal. - requestClear notification handler now also clears `dropIndicator` (CodeRabbit) so render paths keyed on dropIndicator can't linger. - Dropped SidebarTabItemPresentationResolutionPolicyTests, which still referenced the removed presentation-snapshot types and was failing CI. Co-Authored-By: Claude Opus 4.7 (1M context) --- Sources/ContentView.swift | 209 ++++++++++-------- ...rWorkspaceSnapshotRefreshPolicyTests.swift | 67 ------ 2 files changed, 121 insertions(+), 155 deletions(-) diff --git a/Sources/ContentView.swift b/Sources/ContentView.swift index 79fe4da92f00..a0ebb16e3271 100644 --- a/Sources/ContentView.swift +++ b/Sources/ContentView.swift @@ -9587,49 +9587,28 @@ final class SidebarDragState { init() {} } -/// Dedicated view that reads `dragState.dropIndicator` (mutates at ~60fps during -/// drag). Isolating the read here means only this tiny overlay invalidates per -/// frame, not the enclosing `TabItemView` body. -private struct SidebarTabDropIndicatorOverlay: View { - let dragState: SidebarDragState - let tabId: UUID - let index: Int - let rowSpacing: CGFloat - let tabManager: TabManager - - var body: some View { - if showsCenteredTopDropIndicator { - Rectangle() - .fill(cmuxAccentColor()) - .frame(height: 2) - .padding(.horizontal, 8) - .offset(y: index == 0 ? 0 : -(rowSpacing / 2)) - } - } - - private var showsCenteredTopDropIndicator: Bool { +/// Per-row drop-indicator visibility computed by the parent. Same predicate +/// that used to live inside `SidebarTabDropIndicatorOverlay`, but evaluated +/// once per row from `dragState` ownership at the LazyVStack parent so the +/// row's view subtree never reads the `@Observable` store directly. +@MainActor +enum SidebarTabDropIndicatorPredicate { + static func topVisible( + forTabId tabId: UUID, + dragState: SidebarDragState, + tabs: [Tab] + ) -> Bool { guard dragState.draggedTabId != nil, let indicator = dragState.dropIndicator else { return false } if indicator.tabId == tabId && indicator.edge == .top { return true } guard indicator.edge == .bottom, - let currentIndex = tabManager.tabs.firstIndex(where: { $0.id == tabId }), + let currentIndex = tabs.firstIndex(where: { $0.id == tabId }), currentIndex > 0 else { return false } - return tabManager.tabs[currentIndex - 1].id == indicator.tabId - } -} - -/// Dedicated view that reads `dragState.draggedTabId` so only the row's opacity -/// layer invalidates on drag start/end, not the row's complex body. -private struct SidebarTabDragOpacityModifier: ViewModifier { - let dragState: SidebarDragState - let tabId: UUID - - func body(content: Content) -> some View { - content.opacity(dragState.draggedTabId == tabId ? 0.6 : 1) + return tabs[currentIndex - 1].id == indicator.tabId } } @@ -9691,6 +9670,33 @@ struct VerticalTabsSidebar: View { ) } + /// Computed in the parent so `SidebarEmptyArea` can render its top-edge + /// indicator from a value snapshot without holding a `SidebarDragState` + /// reference (snapshot-boundary rule). + private func emptyAreaTopDropIndicatorVisible() -> Bool { + guard dragState.draggedTabId != nil, let indicator = dragState.dropIndicator else { return false } + if indicator.tabId == nil { + return true + } + guard indicator.edge == .bottom, let lastTabId = tabManager.tabs.last?.id else { return false } + return indicator.tabId == lastTabId + } + + /// Constructs the drop delegate for the empty area in the parent scope, + /// so the child view receives a closure-bundle-equivalent value rather + /// than an `@Observable` store. + private func emptyAreaTabDropDelegate() -> SidebarTabDropDelegate { + SidebarTabDropDelegate( + targetTabId: nil, + tabManager: tabManager, + dragState: dragState, + selectedTabIds: $selectedTabIds, + lastSidebarSelectionIndex: $lastSidebarSelectionIndex, + targetRowHeight: nil, + dragAutoScrollController: dragAutoScrollController + ) + } + private var sidebarTopScrimHeight: CGFloat { SidebarWorkspaceListMetrics.topScrimHeight } @@ -9878,12 +9884,13 @@ struct VerticalTabsSidebar: View { dragState.dropIndicator = nil } .onReceive(NotificationCenter.default.publisher(for: SidebarDragLifecycleNotification.requestClear)) { notification in - guard dragState.draggedTabId != nil else { return } + guard dragState.draggedTabId != nil || dragState.dropIndicator != nil else { return } let reason = SidebarDragLifecycleNotification.reason(from: notification) #if DEBUG cmuxDebugLog("sidebar.dragClear tab=\(debugShortSidebarTabId(dragState.draggedTabId)) reason=\(reason)") #endif dragState.draggedTabId = nil + dragState.dropIndicator = nil } .onReceive( NotificationCenter.default.publisher(for: Workspace.terminalScrollBarHiddenDidChangeNotification) @@ -10045,7 +10052,9 @@ struct VerticalTabsSidebar: View { selectedTabIds: $selectedTabIds, lastSidebarSelectionIndex: $lastSidebarSelectionIndex, dragAutoScrollController: dragAutoScrollController, - dragState: dragState + topDropIndicatorVisible: emptyAreaTopDropIndicatorVisible(), + tabDropDelegate: emptyAreaTabDropDelegate(), + bonsplitDropIndicator: dropIndicatorBinding ) .frame(maxWidth: .infinity, minHeight: 48) } @@ -10801,7 +10810,9 @@ struct VerticalTabsSidebar: View { selectedTabIds: $selectedTabIds, lastSidebarSelectionIndex: $lastSidebarSelectionIndex, dragAutoScrollController: dragAutoScrollController, - dragState: dragState + topDropIndicatorVisible: emptyAreaTopDropIndicatorVisible(), + tabDropDelegate: emptyAreaTabDropDelegate(), + bonsplitDropIndicator: dropIndicatorBinding ) .frame(maxWidth: .infinity, maxHeight: .infinity) } @@ -10921,6 +10932,41 @@ struct VerticalTabsSidebar: View { }() let liveShowsModifierShortcutHints = modifierKeyMonitor.isModifierPressed + // Per-row drag/drop snapshots. Reading `dragState` here in the parent + // is intentional: the parent owns the @Observable store, and these + // value snapshots are what get passed to the row. The row's + // Equatable conformance ignores closures, so rows whose snapshot is + // unchanged skip re-render when drag state moves. + let isBeingDragged = dragState.draggedTabId == tab.id + let topDropIndicatorVisible = SidebarTabDropIndicatorPredicate.topVisible( + forTabId: tab.id, + dragState: dragState, + tabs: renderContext.tabs + ) + let onDragStart: () -> NSItemProvider = { [tabId = tab.id] in + #if DEBUG + cmuxDebugLog("sidebar.onDrag tab=\(tabId.uuidString.prefix(5))") + #endif + dragState.draggedTabId = tabId + dragState.dropIndicator = nil + return SidebarTabDragPayload.provider(for: tabId) + } + let tabDropDelegateFactory: (CGFloat) -> SidebarTabDropDelegate = { [ + tabId = tab.id, + selectedTabIds = $selectedTabIds, + lastSidebarSelectionIndex = $lastSidebarSelectionIndex + ] rowHeight in + SidebarTabDropDelegate( + targetTabId: tabId, + tabManager: tabManager, + dragState: dragState, + selectedTabIds: selectedTabIds, + lastSidebarSelectionIndex: lastSidebarSelectionIndex, + targetRowHeight: rowHeight, + dragAutoScrollController: dragAutoScrollController + ) + } + return TabItemView( tabManager: tabManager, notificationStore: notificationStore, @@ -10942,7 +10988,10 @@ struct VerticalTabsSidebar: View { lastSidebarSelectionIndex: $lastSidebarSelectionIndex, showsModifierShortcutHints: liveShowsModifierShortcutHints, dragAutoScrollController: dragAutoScrollController, - dragState: dragState, + isBeingDragged: isBeingDragged, + topDropIndicatorVisible: topDropIndicatorVisible, + onDragStart: onDragStart, + tabDropDelegateFactory: tabDropDelegateFactory, contextMenuWorkspaceIds: contextMenuWorkspaceIds, remoteContextMenuWorkspaceIds: remoteContextMenuWorkspaceIds, allRemoteContextMenuTargetsConnecting: allRemoteContextMenuTargetsConnecting, @@ -13160,7 +13209,11 @@ private struct SidebarEmptyArea: View { @Binding var selectedTabIds: Set @Binding var lastSidebarSelectionIndex: Int? let dragAutoScrollController: SidebarDragAutoScrollController - let dragState: SidebarDragState + // Value snapshot + closure bundles instead of an @Observable store + // reference (snapshot-boundary rule). + let topDropIndicatorVisible: Bool + let tabDropDelegate: SidebarTabDropDelegate + let bonsplitDropIndicator: Binding var body: some View { Color.clear @@ -13174,29 +13227,18 @@ private struct SidebarEmptyArea: View { } selection = .tabs } - .onDrop(of: SidebarTabDragPayload.dropContentTypes, delegate: SidebarTabDropDelegate( - targetTabId: nil, - tabManager: tabManager, - dragState: dragState, - selectedTabIds: $selectedTabIds, - lastSidebarSelectionIndex: $lastSidebarSelectionIndex, - targetRowHeight: nil, - dragAutoScrollController: dragAutoScrollController - )) + .onDrop(of: SidebarTabDragPayload.dropContentTypes, delegate: tabDropDelegate) .overlay { SidebarBonsplitTabNewWorkspaceDropOverlay( tabManager: tabManager, selectedTabIds: $selectedTabIds, lastSidebarSelectionIndex: $lastSidebarSelectionIndex, - dropIndicator: Binding( - get: { dragState.dropIndicator }, - set: { dragState.dropIndicator = $0 } - ) + dropIndicator: bonsplitDropIndicator ) .frame(maxWidth: .infinity, maxHeight: .infinity) } .overlay(alignment: .top) { - if shouldShowTopDropIndicator { + if topDropIndicatorVisible { Rectangle() .fill(cmuxAccentColor()) .frame(height: 2) @@ -13205,15 +13247,6 @@ private struct SidebarEmptyArea: View { } } } - - private var shouldShowTopDropIndicator: Bool { - guard dragState.draggedTabId != nil, let indicator = dragState.dropIndicator else { return false } - if indicator.tabId == nil { - return true - } - guard indicator.edge == .bottom, let lastTabId = tabManager.tabs.last?.id else { return false } - return indicator.tabId == lastTabId - } } private struct ExtensionSidebarBrowserStackEmptyArea: View { @@ -13407,6 +13440,8 @@ private struct TabItemView: View, Equatable { lhs.allRemoteContextMenuTargetsDisconnected == rhs.allRemoteContextMenuTargetsDisconnected && lhs.allContextMenuWorkspacesHideTerminalScrollBar == rhs.allContextMenuWorkspacesHideTerminalScrollBar && lhs.contextMenuPinState == rhs.contextMenuPinState && + lhs.isBeingDragged == rhs.isBeingDragged && + lhs.topDropIndicatorVisible == rhs.topDropIndicatorVisible && lhs.settings == rhs.settings } @@ -13431,7 +13466,19 @@ private struct TabItemView: View, Equatable { @Binding var lastSidebarSelectionIndex: Int? let showsModifierShortcutHints: Bool let dragAutoScrollController: SidebarDragAutoScrollController - let dragState: SidebarDragState + // Row receives precomputed drag/drop snapshot values + action closures + // instead of an `@Observable` store reference. This keeps TabItemView in + // compliance with the snapshot-boundary rule for views under a LazyVStack + // (see CLAUDE.md). When drag state changes, the parent recomputes these + // per-row snapshots and `==` skips re-render for rows whose snapshot is + // unchanged. + let isBeingDragged: Bool + let topDropIndicatorVisible: Bool + let onDragStart: () -> NSItemProvider + /// Factory invoked from `body` with the row's measured `rowHeight`. Closure + /// captures the parent's `dragState`, so TabItemView itself never holds an + /// `@Observable` store reference (snapshot-boundary rule). + let tabDropDelegateFactory: (CGFloat) -> SidebarTabDropDelegate let contextMenuWorkspaceIds: [UUID] let remoteContextMenuWorkspaceIds: [UUID] let allRemoteContextMenuTargetsConnecting: Bool @@ -14049,7 +14096,7 @@ private struct TabItemView: View, Equatable { .padding(.horizontal, 6) .background { rowHeightProbe } .contentShape(Rectangle()) - .modifier(SidebarTabDragOpacityModifier(dragState: dragState, tabId: tab.id)) + .opacity(isBeingDragged ? 0.6 : 1) .overlay { SidebarWorkspaceRowHoverTracker(rowInteractionState: $rowInteractionState) } @@ -14062,13 +14109,13 @@ private struct TabItemView: View, Equatable { } } .overlay(alignment: .top) { - SidebarTabDropIndicatorOverlay( - dragState: dragState, - tabId: tab.id, - index: index, - rowSpacing: rowSpacing, - tabManager: tabManager - ) + if topDropIndicatorVisible { + Rectangle() + .fill(cmuxAccentColor()) + .frame(height: 2) + .padding(.horizontal, 8) + .offset(y: index == 0 ? 0 : -(rowSpacing / 2)) + } } .onAppear { refreshWorkspaceSnapshot(force: true) @@ -14123,24 +14170,9 @@ private struct TabItemView: View, Equatable { .onChange(of: settings) { _ in refreshWorkspaceSnapshot(force: true) } - .onDrag { - #if DEBUG - cmuxDebugLog("sidebar.onDrag tab=\(tab.id.uuidString.prefix(5))") - #endif - dragState.draggedTabId = tab.id - dragState.dropIndicator = nil - return SidebarTabDragPayload.provider(for: tab.id) - } + .onDrag(onDragStart) .internalOnlyTabDrag() - .onDrop(of: SidebarTabDragPayload.dropContentTypes, delegate: SidebarTabDropDelegate( - targetTabId: tab.id, - tabManager: tabManager, - dragState: dragState, - selectedTabIds: $selectedTabIds, - lastSidebarSelectionIndex: $lastSidebarSelectionIndex, - targetRowHeight: rowHeight, - dragAutoScrollController: dragAutoScrollController - )) + .onDrop(of: SidebarTabDragPayload.dropContentTypes, delegate: tabDropDelegateFactory(rowHeight)) .onDrop(of: BonsplitTabDragPayload.dropContentTypes, delegate: SidebarBonsplitTabDropDelegate( targetWorkspaceId: tab.id, tabManager: tabManager, @@ -15826,6 +15858,7 @@ private struct SidebarBonsplitTabDropDelegate: DropDelegate { } } +@MainActor private struct SidebarTabDropDelegate: DropDelegate { let targetTabId: UUID? let tabManager: TabManager diff --git a/cmuxTests/SidebarWorkspaceSnapshotRefreshPolicyTests.swift b/cmuxTests/SidebarWorkspaceSnapshotRefreshPolicyTests.swift index 3dc064cadb9c..3bf9715051d4 100644 --- a/cmuxTests/SidebarWorkspaceSnapshotRefreshPolicyTests.swift +++ b/cmuxTests/SidebarWorkspaceSnapshotRefreshPolicyTests.swift @@ -208,73 +208,6 @@ final class SidebarSelectedWorkspaceScrollPolicyTests: XCTestCase { } } -final class SidebarTabItemPresentationResolutionPolicyTests: XCTestCase { - func testFrozenContextMenuPresentationDoesNotSuppressLiveNotificationState() { - let tabId = UUID() - let frozen = SidebarTabItemPresentationSnapshot( - tabId: tabId, - unreadCount: 0, - latestNotificationText: nil, - showsModifierShortcutHints: true - ) - let live = SidebarTabItemPresentationSnapshot( - tabId: tabId, - unreadCount: 1, - latestNotificationText: "done", - showsModifierShortcutHints: false - ) - - let resolved = SidebarTabItemPresentationResolutionPolicy.resolved( - live: live, - frozen: frozen - ) - - XCTAssertEqual(resolved.unreadCount, 1) - XCTAssertEqual(resolved.latestNotificationText, "done") - XCTAssertTrue(resolved.showsModifierShortcutHints) - } - - func testNoFrozenPresentationUsesLiveSnapshot() { - let live = SidebarTabItemPresentationSnapshot( - tabId: UUID(), - unreadCount: 2, - latestNotificationText: "live", - showsModifierShortcutHints: true - ) - - let resolved = SidebarTabItemPresentationResolutionPolicy.resolved( - live: live, - frozen: nil - ) - - XCTAssertEqual(resolved, live) - } - - func testNonMatchingTabIdUsesLiveShortcutHints() { - let frozen = SidebarTabItemPresentationSnapshot( - tabId: UUID(), - unreadCount: 0, - latestNotificationText: nil, - showsModifierShortcutHints: true - ) - let live = SidebarTabItemPresentationSnapshot( - tabId: UUID(), - unreadCount: 1, - latestNotificationText: "done", - showsModifierShortcutHints: false - ) - - let resolved = SidebarTabItemPresentationResolutionPolicy.resolved( - live: live, - frozen: frozen - ) - - XCTAssertEqual(resolved.unreadCount, 1) - XCTAssertEqual(resolved.latestNotificationText, "done") - XCTAssertFalse(resolved.showsModifierShortcutHints) - } -} - final class SidebarWorkspaceRowInteractionStateTests: XCTestCase { func testHoverRevealIsIndependentFromStaleContextMenuVisibility() { var state = SidebarWorkspaceRowInteractionState() From 5631c6b51a9bec434253c901cf184fda24800220 Mon Sep 17 00:00:00 2001 From: Aziz Albahar Date: Mon, 25 May 2026 16:57:14 -0700 Subject: [PATCH 3/5] Add test coverage for SidebarTabDropIndicatorPredicate + SidebarDragState - Refactored `SidebarTabDropIndicatorPredicate.topVisible` to take pure values (draggedTabId, dropIndicator, tabIds) instead of a SidebarDragState + [Tab] pair, so the per-row indicator predicate is unit-testable without any view-state setup. Extracted `emptyAreaTopVisible` to host the corresponding logic that used to live inline on SidebarEmptyArea. - New `cmuxTests/SidebarTabDropIndicatorPredicateTests.swift`: * `SidebarTabDropIndicatorPredicateTopVisibleTests` (8 cases): no drag, no indicator, indicator on this row's top edge, this row's bottom edge, previous row's bottom edge, unrelated row, first-row bottom edge, stray row id not in tabIds. * `SidebarTabDropIndicatorPredicateEmptyAreaTests` (7 cases): no drag, no indicator, end-of-list (tabId nil), last row bottom edge, last row top edge, non-last row bottom edge, empty list. * `SidebarDragStateTests` (3 cases): initial cleared, independent per-property mutation, clearing both yields idle state. All 18 cases pass against the cmux-unit scheme. Co-Authored-By: Claude Opus 4.7 (1M context) --- Sources/ContentView.swift | 57 ++-- cmux.xcodeproj/project.pbxproj | 4 + ...idebarTabDropIndicatorPredicateTests.swift | 258 ++++++++++++++++++ 3 files changed, 300 insertions(+), 19 deletions(-) create mode 100644 cmuxTests/SidebarTabDropIndicatorPredicateTests.swift diff --git a/Sources/ContentView.swift b/Sources/ContentView.swift index a0ebb16e3271..184be58e2c4f 100644 --- a/Sources/ContentView.swift +++ b/Sources/ContentView.swift @@ -9587,28 +9587,46 @@ final class SidebarDragState { init() {} } -/// Per-row drop-indicator visibility computed by the parent. Same predicate -/// that used to live inside `SidebarTabDropIndicatorOverlay`, but evaluated -/// once per row from `dragState` ownership at the LazyVStack parent so the -/// row's view subtree never reads the `@Observable` store directly. -@MainActor +/// Per-row drop-indicator visibility, computed by the parent from value +/// inputs only. Takes UUIDs (not `Tab` objects or `SidebarDragState`) so it's +/// trivially unit-testable and the row's view subtree never reads the +/// `@Observable` store directly. Same predicate that used to live inside +/// `SidebarTabDropIndicatorOverlay`. enum SidebarTabDropIndicatorPredicate { static func topVisible( forTabId tabId: UUID, - dragState: SidebarDragState, - tabs: [Tab] + draggedTabId: UUID?, + dropIndicator: SidebarDropIndicator?, + tabIds: [UUID] ) -> Bool { - guard dragState.draggedTabId != nil, let indicator = dragState.dropIndicator else { return false } + guard draggedTabId != nil, let indicator = dropIndicator else { return false } if indicator.tabId == tabId && indicator.edge == .top { return true } guard indicator.edge == .bottom, - let currentIndex = tabs.firstIndex(where: { $0.id == tabId }), + let currentIndex = tabIds.firstIndex(of: tabId), currentIndex > 0 else { return false } - return tabs[currentIndex - 1].id == indicator.tabId + return tabIds[currentIndex - 1] == indicator.tabId + } + + /// Convenience used by `SidebarEmptyArea`: the empty area's "top" indicator + /// (drawn above the empty space below all rows) is visible when the drop + /// indicator targets nothing (end-of-list) or the bottom edge of the last + /// row. + static func emptyAreaTopVisible( + draggedTabId: UUID?, + dropIndicator: SidebarDropIndicator?, + lastTabId: UUID? + ) -> Bool { + guard draggedTabId != nil, let indicator = dropIndicator else { return false } + if indicator.tabId == nil { + return true + } + guard indicator.edge == .bottom, let lastTabId else { return false } + return indicator.tabId == lastTabId } } @@ -9672,14 +9690,14 @@ struct VerticalTabsSidebar: View { /// Computed in the parent so `SidebarEmptyArea` can render its top-edge /// indicator from a value snapshot without holding a `SidebarDragState` - /// reference (snapshot-boundary rule). + /// reference (snapshot-boundary rule). Delegates to a pure predicate so + /// the logic is unit-testable in isolation from view state. private func emptyAreaTopDropIndicatorVisible() -> Bool { - guard dragState.draggedTabId != nil, let indicator = dragState.dropIndicator else { return false } - if indicator.tabId == nil { - return true - } - guard indicator.edge == .bottom, let lastTabId = tabManager.tabs.last?.id else { return false } - return indicator.tabId == lastTabId + SidebarTabDropIndicatorPredicate.emptyAreaTopVisible( + draggedTabId: dragState.draggedTabId, + dropIndicator: dragState.dropIndicator, + lastTabId: tabManager.tabs.last?.id + ) } /// Constructs the drop delegate for the empty area in the parent scope, @@ -10940,8 +10958,9 @@ struct VerticalTabsSidebar: View { let isBeingDragged = dragState.draggedTabId == tab.id let topDropIndicatorVisible = SidebarTabDropIndicatorPredicate.topVisible( forTabId: tab.id, - dragState: dragState, - tabs: renderContext.tabs + draggedTabId: dragState.draggedTabId, + dropIndicator: dragState.dropIndicator, + tabIds: renderContext.tabs.map(\.id) ) let onDragStart: () -> NSItemProvider = { [tabId = tab.id] in #if DEBUG diff --git a/cmux.xcodeproj/project.pbxproj b/cmux.xcodeproj/project.pbxproj index 1d1bf1fb37e5..9adeab747ad9 100644 --- a/cmux.xcodeproj/project.pbxproj +++ b/cmux.xcodeproj/project.pbxproj @@ -97,6 +97,7 @@ EA1F00000000000000000003 /* SidebarDirectoryText.swift in Sources */ = {isa = PBXBuildFile; fileRef = EA1F00000000000000000004 /* SidebarDirectoryText.swift */; }; D7AB34300000000000000003 /* SidebarBonsplitTabWorkspaceDropOverlay.swift in Sources */ = {isa = PBXBuildFile; fileRef = D7AB34300000000000000004 /* SidebarBonsplitTabWorkspaceDropOverlay.swift */; }; D7AB34300000000000000005 /* SidebarWorkspaceDropPlannerTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = D7AB34300000000000000006 /* SidebarWorkspaceDropPlannerTests.swift */; }; + D7AB34300000000000000105 /* SidebarTabDropIndicatorPredicateTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = D7AB34300000000000000106 /* SidebarTabDropIndicatorPredicateTests.swift */; }; D7AB34400000000000000001 /* WorkspaceSurfaceConfig.swift in Sources */ = {isa = PBXBuildFile; fileRef = D7AB34400000000000000002 /* WorkspaceSurfaceConfig.swift */; }; D7AB34400000000000000003 /* GhosttyTerminalViewVisibilityPolicyTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = D7AB34400000000000000004 /* GhosttyTerminalViewVisibilityPolicyTests.swift */; }; F4350A110000000000000001 /* AppBundleIconPersistencePolicy.swift in Sources */ = {isa = PBXBuildFile; fileRef = F4350A130000000000000001 /* AppBundleIconPersistencePolicy.swift */; }; @@ -624,6 +625,7 @@ 491751CE2321474474F27DCF /* TerminalControllerSocketSecurityTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TerminalControllerSocketSecurityTests.swift; sourceTree = ""; }; 51D800000000000000000002 /* SidebarIdentifierFormattingTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SidebarIdentifierFormattingTests.swift; sourceTree = ""; }; D7AB34300000000000000006 /* SidebarWorkspaceDropPlannerTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SidebarWorkspaceDropPlannerTests.swift; sourceTree = ""; }; + D7AB34300000000000000106 /* SidebarTabDropIndicatorPredicateTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SidebarTabDropIndicatorPredicateTests.swift; sourceTree = ""; }; D7AB34400000000000000004 /* GhosttyTerminalViewVisibilityPolicyTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = GhosttyTerminalViewVisibilityPolicyTests.swift; sourceTree = ""; }; 9C1BEA3D2E6F49709A71C021 /* TerminalControllerSocketWriteTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TerminalControllerSocketWriteTests.swift; sourceTree = ""; }; E7E000000000000000000004 /* CmuxEventBusTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CmuxEventBusTests.swift; sourceTree = ""; }; @@ -1666,6 +1668,7 @@ C7A509000000000000000001 /* CmuxTopSnapshotScopeTests.swift */, C7A50C000000000000000001 /* CmuxTopProcessCPUTests.swift */, D7AB34300000000000000006 /* SidebarWorkspaceDropPlannerTests.swift */, + D7AB34300000000000000106 /* SidebarTabDropIndicatorPredicateTests.swift */, C3677001000000000000002 /* CmuxSSHURLRequestTests.swift */, 491751CE2321474474F27DCF /* TerminalControllerSocketSecurityTests.swift */, 9C1BEA3D2E6F49709A71C021 /* TerminalControllerSocketWriteTests.swift */, @@ -2464,6 +2467,7 @@ C7A509000000000000000002 /* CmuxTopSnapshotScopeTests.swift in Sources */, C7A50C000000000000000002 /* CmuxTopProcessCPUTests.swift in Sources */, D7AB34300000000000000005 /* SidebarWorkspaceDropPlannerTests.swift in Sources */, + D7AB34300000000000000105 /* SidebarTabDropIndicatorPredicateTests.swift in Sources */, C3677001000000000000001 /* CmuxSSHURLRequestTests.swift in Sources */, 8C4BBF2DEF6DF93F395A9EE7 /* TerminalControllerSocketSecurityTests.swift in Sources */, 9C1BEA3D2E6F49709A71C020 /* TerminalControllerSocketWriteTests.swift in Sources */, diff --git a/cmuxTests/SidebarTabDropIndicatorPredicateTests.swift b/cmuxTests/SidebarTabDropIndicatorPredicateTests.swift new file mode 100644 index 000000000000..b6327059d6cf --- /dev/null +++ b/cmuxTests/SidebarTabDropIndicatorPredicateTests.swift @@ -0,0 +1,258 @@ +import XCTest + +#if canImport(cmux_DEV) +@testable import cmux_DEV +#elseif canImport(cmux) +@testable import cmux +#endif + +/// Tests for `SidebarTabDropIndicatorPredicate.topVisible(forTabId:draggedTabId:dropIndicator:tabIds:)`. +/// +/// This predicate is the snapshot the parent computes for each sidebar row to +/// decide whether to draw the drop-line indicator above it. Lifting it out of +/// the row view subtree (per the snapshot-boundary rule) makes it a pure +/// function — these tests cover the resulting branches end-to-end. +final class SidebarTabDropIndicatorPredicateTopVisibleTests: XCTestCase { + func testReturnsFalseWhenNoDragInProgress() { + let rowId = UUID() + XCTAssertFalse( + SidebarTabDropIndicatorPredicate.topVisible( + forTabId: rowId, + draggedTabId: nil, + dropIndicator: SidebarDropIndicator(tabId: rowId, edge: .top), + tabIds: [rowId] + ), + "An indicator value alone shouldn't trigger the overlay; a drag must be in flight." + ) + } + + func testReturnsFalseWhenNoIndicator() { + let rowId = UUID() + XCTAssertFalse( + SidebarTabDropIndicatorPredicate.topVisible( + forTabId: rowId, + draggedTabId: rowId, + dropIndicator: nil, + tabIds: [rowId] + ) + ) + } + + func testReturnsTrueWhenIndicatorTargetsThisRowTopEdge() { + let rowId = UUID() + let draggedId = UUID() + XCTAssertTrue( + SidebarTabDropIndicatorPredicate.topVisible( + forTabId: rowId, + draggedTabId: draggedId, + dropIndicator: SidebarDropIndicator(tabId: rowId, edge: .top), + tabIds: [rowId, draggedId] + ) + ) + } + + func testReturnsFalseWhenIndicatorTargetsThisRowBottomEdge() { + let rowId = UUID() + let draggedId = UUID() + // A .bottom indicator on this row paints the indicator above the *next* + // row, not above this one. + XCTAssertFalse( + SidebarTabDropIndicatorPredicate.topVisible( + forTabId: rowId, + draggedTabId: draggedId, + dropIndicator: SidebarDropIndicator(tabId: rowId, edge: .bottom), + tabIds: [rowId, draggedId] + ) + ) + } + + func testReturnsTrueWhenIndicatorTargetsPreviousRowBottomEdge() { + let firstId = UUID() + let middleId = UUID() + let draggedId = UUID() + // The visual indicator for "insert between row 0 and row 1" is drawn + // above row 1, even though the indicator semantically points at row 0 + // with .bottom. + XCTAssertTrue( + SidebarTabDropIndicatorPredicate.topVisible( + forTabId: middleId, + draggedTabId: draggedId, + dropIndicator: SidebarDropIndicator(tabId: firstId, edge: .bottom), + tabIds: [firstId, middleId, draggedId] + ) + ) + } + + func testReturnsFalseWhenIndicatorTargetsUnrelatedRow() { + let rowId = UUID() + let otherId = UUID() + let draggedId = UUID() + XCTAssertFalse( + SidebarTabDropIndicatorPredicate.topVisible( + forTabId: rowId, + draggedTabId: draggedId, + dropIndicator: SidebarDropIndicator(tabId: otherId, edge: .top), + tabIds: [rowId, otherId, draggedId] + ) + ) + } + + func testReturnsFalseForFirstRowWithBottomIndicatorAboveIt() { + // The first row has no previous neighbor — a .bottom indicator from a + // hypothetical previous row can't apply. + let firstId = UUID() + let draggedId = UUID() + XCTAssertFalse( + SidebarTabDropIndicatorPredicate.topVisible( + forTabId: firstId, + draggedTabId: draggedId, + dropIndicator: SidebarDropIndicator(tabId: UUID(), edge: .bottom), + tabIds: [firstId, draggedId] + ) + ) + } + + func testReturnsFalseWhenRowIsNotInTabsList() { + // Defensive: if the row id isn't in tabIds (stale snapshot), the + // predicate should return false rather than crashing on the lookup. + let strayId = UUID() + let draggedId = UUID() + XCTAssertFalse( + SidebarTabDropIndicatorPredicate.topVisible( + forTabId: strayId, + draggedTabId: draggedId, + dropIndicator: SidebarDropIndicator(tabId: UUID(), edge: .bottom), + tabIds: [UUID(), draggedId] + ) + ) + } +} + +/// Tests for `SidebarTabDropIndicatorPredicate.emptyAreaTopVisible(...)`. +/// The "empty area" sits below the workspace list and shows an indicator when +/// the drop will append at the end of the list. +final class SidebarTabDropIndicatorPredicateEmptyAreaTests: XCTestCase { + func testReturnsFalseWhenNoDragInProgress() { + XCTAssertFalse( + SidebarTabDropIndicatorPredicate.emptyAreaTopVisible( + draggedTabId: nil, + dropIndicator: SidebarDropIndicator(tabId: nil, edge: .top), + lastTabId: UUID() + ) + ) + } + + func testReturnsFalseWhenNoIndicator() { + XCTAssertFalse( + SidebarTabDropIndicatorPredicate.emptyAreaTopVisible( + draggedTabId: UUID(), + dropIndicator: nil, + lastTabId: UUID() + ) + ) + } + + func testReturnsTrueWhenIndicatorTargetsEndOfList() { + // tabId == nil means "after the last row" — the empty area shows the + // indicator regardless of which row was last. + XCTAssertTrue( + SidebarTabDropIndicatorPredicate.emptyAreaTopVisible( + draggedTabId: UUID(), + dropIndicator: SidebarDropIndicator(tabId: nil, edge: .top), + lastTabId: UUID() + ) + ) + } + + func testReturnsTrueWhenIndicatorTargetsLastRowBottomEdge() { + let lastId = UUID() + XCTAssertTrue( + SidebarTabDropIndicatorPredicate.emptyAreaTopVisible( + draggedTabId: UUID(), + dropIndicator: SidebarDropIndicator(tabId: lastId, edge: .bottom), + lastTabId: lastId + ) + ) + } + + func testReturnsFalseWhenIndicatorTargetsLastRowTopEdge() { + // A .top indicator on the last row draws the line *above* the last + // row, not below — so the empty area below it should stay clear. + let lastId = UUID() + XCTAssertFalse( + SidebarTabDropIndicatorPredicate.emptyAreaTopVisible( + draggedTabId: UUID(), + dropIndicator: SidebarDropIndicator(tabId: lastId, edge: .top), + lastTabId: lastId + ) + ) + } + + func testReturnsFalseWhenIndicatorTargetsNonLastRowBottomEdge() { + let middleId = UUID() + let lastId = UUID() + XCTAssertFalse( + SidebarTabDropIndicatorPredicate.emptyAreaTopVisible( + draggedTabId: UUID(), + dropIndicator: SidebarDropIndicator(tabId: middleId, edge: .bottom), + lastTabId: lastId + ) + ) + } + + func testReturnsFalseWhenListIsEmpty() { + XCTAssertFalse( + SidebarTabDropIndicatorPredicate.emptyAreaTopVisible( + draggedTabId: UUID(), + dropIndicator: SidebarDropIndicator(tabId: UUID(), edge: .bottom), + lastTabId: nil + ) + ) + } +} + +/// Tests for `SidebarDragState` (the @MainActor @Observable bag that owns +/// the per-window drag transient state). +@MainActor +final class SidebarDragStateTests: XCTestCase { + func testInitialStateIsCleared() { + let state = SidebarDragState() + XCTAssertNil(state.draggedTabId) + XCTAssertNil(state.dropIndicator) + } + + func testIndependentMutationOfEachProperty() { + // Per-property invariant the PR depends on: writes to one field must + // not silently disturb the other. Verifies the @Observable container + // doesn't enforce coupled updates. + let state = SidebarDragState() + let tabId = UUID() + let indicator = SidebarDropIndicator(tabId: tabId, edge: .top) + + state.draggedTabId = tabId + XCTAssertEqual(state.draggedTabId, tabId) + XCTAssertNil(state.dropIndicator) + + state.dropIndicator = indicator + XCTAssertEqual(state.draggedTabId, tabId) + XCTAssertEqual(state.dropIndicator, indicator) + + state.draggedTabId = nil + XCTAssertNil(state.draggedTabId) + XCTAssertEqual(state.dropIndicator, indicator) + } + + func testClearingBothLeavesStateIdle() { + // Mirror the `requestClear` notification handler: both fields go to + // nil and the state is back to its initial shape. + let state = SidebarDragState() + state.draggedTabId = UUID() + state.dropIndicator = SidebarDropIndicator(tabId: UUID(), edge: .bottom) + + state.draggedTabId = nil + state.dropIndicator = nil + + XCTAssertNil(state.draggedTabId) + XCTAssertNil(state.dropIndicator) + } +} From 186ff097b08d71617eb0256c474a874a19245e54 Mon Sep 17 00:00:00 2001 From: Aziz Albahar Date: Mon, 25 May 2026 17:44:06 -0700 Subject: [PATCH 4/5] Precompute tabIds once per render to avoid O(n) per row MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit flagged that `workspaceRow(...)` was calling `renderContext.tabs.map(\.id)` once per row to feed `SidebarTabDropIndicatorPredicate.topVisible(...)`, making the per-render cost O(n²) in workspace count. Store the snapshot once on `WorkspaceListRenderContext.tabIds` at the parent body level and reuse it per row. Co-Authored-By: Claude Opus 4.7 (1M context) --- Sources/ContentView.swift | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/Sources/ContentView.swift b/Sources/ContentView.swift index 184be58e2c4f..0c6aeeb87371 100644 --- a/Sources/ContentView.swift +++ b/Sources/ContentView.swift @@ -9790,6 +9790,10 @@ struct VerticalTabsSidebar: View { private struct WorkspaceListRenderContext { let tabs: [Workspace] + /// Stored snapshot of `tabs.map(\.id)` so per-row predicates that need + /// it (e.g. `SidebarTabDropIndicatorPredicate.topVisible`) don't pay + /// O(n) per row. + let tabIds: [UUID] let workspaceCount: Int let canCloseWorkspace: Bool let workspaceNumberShortcut: StoredShortcut @@ -9801,9 +9805,7 @@ struct VerticalTabsSidebar: View { let allSelectedRemoteContextMenuTargetsDisconnected: Bool let workspaceTerminalScrollBarHiddenById: [UUID: Bool] - var workspaceIds: [UUID] { - tabs.map(\.id) - } + var workspaceIds: [UUID] { tabIds } } var body: some View { @@ -9831,6 +9833,7 @@ struct VerticalTabsSidebar: View { selectedRemoteContextMenuTargets.allSatisfy { $0.remoteConnectionState == .disconnected } let renderContext = WorkspaceListRenderContext( tabs: tabs, + tabIds: tabs.map(\.id), workspaceCount: workspaceCount, canCloseWorkspace: canCloseWorkspace, workspaceNumberShortcut: workspaceNumberShortcut, @@ -10960,7 +10963,7 @@ struct VerticalTabsSidebar: View { forTabId: tab.id, draggedTabId: dragState.draggedTabId, dropIndicator: dragState.dropIndicator, - tabIds: renderContext.tabs.map(\.id) + tabIds: renderContext.tabIds ) let onDragStart: () -> NSItemProvider = { [tabId = tab.id] in #if DEBUG From 4ecd35bd230b7a6e66530a98808691cdf0dd25fc Mon Sep 17 00:00:00 2001 From: Aziz Albahar Date: Mon, 25 May 2026 18:13:08 -0700 Subject: [PATCH 5/5] Freeze showsModifierShortcutHints while a context menu is open Removing SidebarTabItemPresentationResolutionPolicy in the lazy-sidebar refactor dropped the per-row freeze that prevented modifier-key transitions from flipping shortcut-hint badges on the row sitting behind an open context menu (Greptile flagged this regression; the PR's claim that rowInteractionState preserves the freeze only covers the workspace snapshot, not the modifier hint flag). Reintroduce the freeze with a small pure policy + per-row closures so the snapshot boundary still holds: the parent owns frozenShortcutHintsTabId/Value, rows only see Bool snapshots and call onContextMenuAppear/Disappear closures. Clear the freeze when the frozen row is removed. Co-Authored-By: Claude Opus 4.7 (1M context) --- Sources/ContentView.swift | 58 ++++++++++++++++- ...idebarTabDropIndicatorPredicateTests.swift | 62 +++++++++++++++++++ 2 files changed, 118 insertions(+), 2 deletions(-) diff --git a/Sources/ContentView.swift b/Sources/ContentView.swift index 0c6aeeb87371..6348551f4816 100644 --- a/Sources/ContentView.swift +++ b/Sources/ContentView.swift @@ -9630,6 +9630,24 @@ enum SidebarTabDropIndicatorPredicate { } } +/// Freezes `showsModifierShortcutHints` for the row whose context menu is open, +/// so pressing/releasing the modifier key while the menu is up does not flip +/// the underlying row's shortcut badges (which would be visible around the +/// open context menu). All other rows transition live. +enum SidebarShortcutHintFreezePolicy { + static func resolved( + live: Bool, + currentTabId: UUID, + frozenTabId: UUID?, + frozenValue: Bool + ) -> Bool { + if frozenTabId == currentTabId { + return frozenValue + } + return live + } +} + struct VerticalTabsSidebar: View { @ObservedObject var updateViewModel: UpdateViewModel @ObservedObject var fileExplorerState: FileExplorerState @@ -9649,6 +9667,12 @@ struct VerticalTabsSidebar: View { @StateObject private var tabItemSettingsStore = SidebarTabItemSettingsStore() @ObservedObject private var keyboardShortcutSettingsObserver = KeyboardShortcutSettingsObserver.shared @State private var dragState = SidebarDragState() + // Freezes `showsModifierShortcutHints` for the workspace whose context menu + // is open. Set on the row's contextMenu.onAppear and cleared on + // .onDisappear so modifier-key transitions don't flip the badges on the + // row sitting behind the open menu. See `SidebarShortcutHintFreezePolicy`. + @State private var frozenShortcutHintsTabId: UUID? + @State private var frozenShortcutHintsValue: Bool = false @State private var terminalScrollBarVisibilityGeneration: UInt64 = 0 @State private var laidOutWorkspaceRowIds: Set = [] @State private var pendingSelectedWorkspaceScrollId: UUID? @@ -9913,6 +9937,11 @@ struct VerticalTabsSidebar: View { dragState.draggedTabId = nil dragState.dropIndicator = nil } + .onChange(of: tabManager.tabs.map(\.id)) { tabIds in + guard let frozenTabId = frozenShortcutHintsTabId, + !tabIds.contains(frozenTabId) else { return } + frozenShortcutHintsTabId = nil + } .onReceive( NotificationCenter.default.publisher(for: Workspace.terminalScrollBarHiddenDidChangeNotification) .receive(on: RunLoop.main) @@ -10952,6 +10981,21 @@ struct VerticalTabsSidebar: View { return trimmed.isEmpty ? nil : trimmed }() let liveShowsModifierShortcutHints = modifierKeyMonitor.isModifierPressed + let resolvedShowsModifierShortcutHints = SidebarShortcutHintFreezePolicy.resolved( + live: liveShowsModifierShortcutHints, + currentTabId: tab.id, + frozenTabId: frozenShortcutHintsTabId, + frozenValue: frozenShortcutHintsValue + ) + let onContextMenuAppear: () -> Void = { [tabId = tab.id, snapshot = resolvedShowsModifierShortcutHints] in + frozenShortcutHintsTabId = tabId + frozenShortcutHintsValue = snapshot + } + let onContextMenuDisappear: () -> Void = { [tabId = tab.id] in + if frozenShortcutHintsTabId == tabId { + frozenShortcutHintsTabId = nil + } + } // Per-row drag/drop snapshots. Reading `dragState` here in the parent // is intentional: the parent owns the @Observable store, and these @@ -11008,7 +11052,7 @@ struct VerticalTabsSidebar: View { setSelectionToTabs: { selection = .tabs }, selectedTabIds: $selectedTabIds, lastSidebarSelectionIndex: $lastSidebarSelectionIndex, - showsModifierShortcutHints: liveShowsModifierShortcutHints, + showsModifierShortcutHints: resolvedShowsModifierShortcutHints, dragAutoScrollController: dragAutoScrollController, isBeingDragged: isBeingDragged, topDropIndicatorVisible: topDropIndicatorVisible, @@ -11020,7 +11064,9 @@ struct VerticalTabsSidebar: View { allRemoteContextMenuTargetsDisconnected: allRemoteContextMenuTargetsDisconnected, allContextMenuWorkspacesHideTerminalScrollBar: allContextMenuWorkspacesHideTerminalScrollBar, contextMenuPinState: contextMenuPinState, - settings: renderContext.tabItemSettings + settings: renderContext.tabItemSettings, + onContextMenuAppear: onContextMenuAppear, + onContextMenuDisappear: onContextMenuDisappear ) .equatable() .id(tab.id) @@ -13508,6 +13554,12 @@ private struct TabItemView: View, Equatable { let allContextMenuWorkspacesHideTerminalScrollBar: Bool let contextMenuPinState: WorkspaceActionDispatcher.PinState? let settings: SidebarTabItemSettingsSnapshot + /// Called from this row's contextMenu.onAppear so the parent can freeze + /// `showsModifierShortcutHints` to the value it last passed in. Prevents + /// modifier-key transitions from flipping the badges on the row sitting + /// behind the open context menu. + let onContextMenuAppear: () -> Void + let onContextMenuDisappear: () -> Void @State private var workspaceSnapshotStorage: SidebarWorkspaceSnapshotBuilder.Snapshot? @StateObject private var contextMenuState = SidebarTabItemContextMenuState() @State private var rowInteractionState = SidebarWorkspaceRowInteractionState() @@ -14220,9 +14272,11 @@ private struct TabItemView: View, Equatable { rowInteractionState.contextMenuDidAppear() contextMenuState.hasDeferredWorkspaceObservationInvalidation = false contextMenuState.pendingWorkspaceSnapshot = nil + onContextMenuAppear() } .onDisappear { rowInteractionState.contextMenuDidDisappear() + onContextMenuDisappear() flushDeferredWorkspaceObservationInvalidation() } } diff --git a/cmuxTests/SidebarTabDropIndicatorPredicateTests.swift b/cmuxTests/SidebarTabDropIndicatorPredicateTests.swift index b6327059d6cf..b25ed1b75154 100644 --- a/cmuxTests/SidebarTabDropIndicatorPredicateTests.swift +++ b/cmuxTests/SidebarTabDropIndicatorPredicateTests.swift @@ -256,3 +256,65 @@ final class SidebarDragStateTests: XCTestCase { XCTAssertNil(state.dropIndicator) } } + +/// Covers the freeze policy that holds `showsModifierShortcutHints` stable +/// for the row whose context menu is open. Without it, pressing/releasing +/// the modifier key while a context menu is up would flip badges on the row +/// sitting behind the menu (visual regression flagged on the lazy-sidebar PR). +final class SidebarShortcutHintFreezePolicyTests: XCTestCase { + func testReturnsLiveWhenNoRowIsFrozen() { + let rowId = UUID() + XCTAssertTrue( + SidebarShortcutHintFreezePolicy.resolved( + live: true, + currentTabId: rowId, + frozenTabId: nil, + frozenValue: false + ) + ) + XCTAssertFalse( + SidebarShortcutHintFreezePolicy.resolved( + live: false, + currentTabId: rowId, + frozenTabId: nil, + frozenValue: true + ) + ) + } + + func testReturnsFrozenWhenCurrentTabMatchesFrozenTab() { + let rowId = UUID() + XCTAssertFalse( + SidebarShortcutHintFreezePolicy.resolved( + live: true, + currentTabId: rowId, + frozenTabId: rowId, + frozenValue: false + ), + "When this row is frozen, the modifier flipping live should not surface." + ) + XCTAssertTrue( + SidebarShortcutHintFreezePolicy.resolved( + live: false, + currentTabId: rowId, + frozenTabId: rowId, + frozenValue: true + ), + "Frozen-true must remain true even after the modifier is released." + ) + } + + func testReturnsLiveForRowsOtherThanTheFrozenOne() { + let frozenRow = UUID() + let otherRow = UUID() + XCTAssertTrue( + SidebarShortcutHintFreezePolicy.resolved( + live: true, + currentTabId: otherRow, + frozenTabId: frozenRow, + frozenValue: false + ), + "Freeze is per-row; only the row whose menu is open should be pinned." + ) + } +}