From 500f9c5b87974674164b9454ecf9152ecd735063 Mon Sep 17 00:00:00 2001 From: Samuel Reed Date: Wed, 26 Aug 2026 08:35:03 -0400 Subject: [PATCH] fix(sidebar): order panels without reading a single tab title sidebarOrderedPanelIds() built a Tab per tab and a full treeSnapshot() to produce a list of ids. Both read titles: Tab.init(from:) copies all fourteen TabItem properties, and the snapshot puts a title in every ExternalTab. Sidebar views call this from inside their bodies, so an animating tab title invalidated them at spinner rate for an ordering that never looks at a title. allPaneIds walks the tree first-then-second exactly as ExternalTreeNode.orderedPaneIds does, so the snapshot was not buying the pane order either. Lift the ordering out of the ExternalTreeNode extension into PaneSpatialOrder, which takes pane ids directly; the tree method now delegates to it, so there is still one implementation and one set of tests. The call site asks bonsplit for tabIds(inPane:) and never mentions a tab. --- .../ExternalTreeNode+SpatialOrder.swift | 25 +++++++++++++ .../CmuxPanesTests/SpatialOrderTests.swift | 37 +++++++++++++++++++ Sources/Workspace.swift | 26 +++++++++---- 3 files changed, 81 insertions(+), 7 deletions(-) diff --git a/Packages/macOS/CmuxPanes/Sources/CmuxPanes/Geometry/ExternalTreeNode+SpatialOrder.swift b/Packages/macOS/CmuxPanes/Sources/CmuxPanes/Geometry/ExternalTreeNode+SpatialOrder.swift index 40d9e8f424b1..25dd405d84a8 100644 --- a/Packages/macOS/CmuxPanes/Sources/CmuxPanes/Geometry/ExternalTreeNode+SpatialOrder.swift +++ b/Packages/macOS/CmuxPanes/Sources/CmuxPanes/Geometry/ExternalTreeNode+SpatialOrder.swift @@ -22,6 +22,31 @@ extension ExternalTreeNode { public func orderedPanelIds( paneTabs: [String: [UUID]], fallbackPanelIds: [UUID] + ) -> [UUID] { + PaneSpatialOrder.orderedPanelIds( + orderedPaneIds: orderedPaneIds, + paneTabs: paneTabs, + fallbackPanelIds: fallbackPanelIds + ) + } +} + +/// Ordering that needs pane order but not the rest of a tree snapshot. +/// +/// `BonsplitController.allPaneIds` walks first-then-second the same way +/// `ExternalTreeNode.orderedPaneIds` does, so a caller holding pane ids +/// already has the order and does not need to build a snapshot to get it. +/// That matters because a snapshot reads every tab's title, and a caller +/// running inside a SwiftUI update is then invalidated by every title change +/// in the window. +public enum PaneSpatialOrder { + /// Panel ids in on-screen spatial order: panes in the given order, tabs + /// within each pane in tab order, then any panels missing from the panes + /// in the caller-provided stable fallback order. + public static func orderedPanelIds( + orderedPaneIds: [String], + paneTabs: [String: [UUID]], + fallbackPanelIds: [UUID] ) -> [UUID] { var ordered: [UUID] = [] var seen: Set = [] diff --git a/Packages/macOS/CmuxPanes/Tests/CmuxPanesTests/SpatialOrderTests.swift b/Packages/macOS/CmuxPanes/Tests/CmuxPanesTests/SpatialOrderTests.swift index 7d521a5f4e34..d3f692beda67 100644 --- a/Packages/macOS/CmuxPanes/Tests/CmuxPanesTests/SpatialOrderTests.swift +++ b/Packages/macOS/CmuxPanes/Tests/CmuxPanesTests/SpatialOrderTests.swift @@ -39,6 +39,43 @@ import Bonsplit #expect(result == [p1, p2, p3, orphan]) } + /// The tree method is the pane-id method with the ids read off the tree. + /// Callers that already hold pane ids skip the snapshot, so the two have + /// to agree or skipping it changes the sidebar order. + @Test func orderedPanelIdsIsTheSameThroughEitherEntryPoint() { + let p1 = UUID(), p2 = UUID(), p3 = UUID(), orphan = UUID() + let tree = ExternalTreeNode.split(ExternalSplitNode( + id: "s1", orientation: "horizontal", dividerPosition: 0.5, + first: pane("a"), + second: .split(ExternalSplitNode( + id: "s2", orientation: "vertical", dividerPosition: 0.5, + first: pane("b"), second: pane("c") + )) + )) + let paneTabs = ["a": [p1, p2], "b": [p3, p1], "c": []] + let fallback = [orphan, p2] + + #expect( + PaneSpatialOrder.orderedPanelIds( + orderedPaneIds: tree.orderedPaneIds, + paneTabs: paneTabs, + fallbackPanelIds: fallback + ) == tree.orderedPanelIds(paneTabs: paneTabs, fallbackPanelIds: fallback) + ) + } + + /// A pane id with no entry in `paneTabs` contributes nothing rather than + /// stopping the walk, which is what an empty pane looks like. + @Test func panesMissingFromPaneTabsAreSkipped() { + let p1 = UUID(), p2 = UUID() + let result = PaneSpatialOrder.orderedPanelIds( + orderedPaneIds: ["a", "missing", "b"], + paneTabs: ["a": [p1], "b": [p2]], + fallbackPanelIds: [] + ) + #expect(result == [p1, p2]) + } + @Test func paneCycleNavigatorWrapsForwardAndBackward() { let panes = [paneId(1), paneId(2), paneId(3)] let navigator = PaneCycleNavigator() diff --git a/Sources/Workspace.swift b/Sources/Workspace.swift index 2ac7a825f0d2..3e099577f17f 100644 --- a/Sources/Workspace.swift +++ b/Sources/Workspace.swift @@ -1,6 +1,7 @@ import CmuxAppKitSupportUI import CMUXMobileCore import CmuxFoundation +import CmuxPanes import Foundation import CmuxCore import CmuxRemoteDaemon @@ -6554,21 +6555,32 @@ final class Workspace: Identifiable, ObservableObject, FilePreviewTabMetadataHos recomputeListeningPorts() } + /// Panel ids in on-screen order. + /// + /// Ids only, all the way down. The version this replaced asked for `Tab` + /// values and a `treeSnapshot()`, and both read every tab's title: + /// `Tab.init(from:)` copies all fourteen `TabItem` properties, and the + /// snapshot puts a title in every `ExternalTab`. Sidebar views call this + /// from inside their bodies, so an animating tab title invalidated them + /// about twenty times a second for an ordering that never looked at a + /// title. `allPaneIds` walks the tree first-then-second exactly as + /// `ExternalTreeNode.orderedPaneIds` does, so the snapshot was not buying + /// the order either. func sidebarOrderedPanelIds() -> [UUID] { + let orderedPaneIds = bonsplitController.allPaneIds let paneTabs: [String: [UUID]] = Dictionary( - uniqueKeysWithValues: bonsplitController.allPaneIds.map { paneId in + uniqueKeysWithValues: orderedPaneIds.map { paneId in let panelIds = bonsplitController - .tabs(inPane: paneId) - .compactMap { panelIdFromSurfaceId($0.id) } + .tabIds(inPane: paneId) + .compactMap { panelIdFromSurfaceId($0) } return (paneId.id.uuidString, panelIds) } ) - let fallbackPanelIds = panels.keys.sorted { $0.uuidString < $1.uuidString } - let tree = bonsplitController.treeSnapshot() - return tree.orderedPanelIds( + return PaneSpatialOrder.orderedPanelIds( + orderedPaneIds: orderedPaneIds.map { $0.id.uuidString }, paneTabs: paneTabs, - fallbackPanelIds: fallbackPanelIds + fallbackPanelIds: panels.keys.sorted { $0.uuidString < $1.uuidString } ) }