From 89a8eb4226205cb82912770350027dbc9ed14d6c Mon Sep 17 00:00:00 2001 From: Leo Li Date: Tue, 22 Sep 2026 23:55:36 -0700 Subject: [PATCH] fix(sidebar): order panels without reading split-container geometry Revealing the sidebar projected every workspace row twice. The AppKit sidebar builds each row from `SidebarWorkspaceSnapshotFactory`, which calls `Workspace.sidebarOrderedPanelIds()` from inside `VerticalTabsSidebar.body`. That read `bonsplitController.treeSnapshot()`, and `treeSnapshot()` reads `SplitViewController.containerFrame` to convert normalized node bounds into pixel rects. The sidebar body therefore observed split-container geometry, and showing the sidebar is itself a content-area resize, so the reveal invalidated the body it had just run: `SidebarHiddenPresentationTests` recorded 10 row projections for 5 workspaces, both passes inside the first run-loop turn, with no notification in flight. Confirmed from the app-host shard on main a9b0329691, where the measured interval printed `VerticalTabsSidebar: @self, _selection changed` followed by `VerticalTabsSidebar: \SplitViewController.containerFrame, _selection changed`. `orderedPanelIds` never used a frame; it walks pane order and tab order only. `BonsplitController.allPaneIds` reports the same depth-first first/second recursion the tree snapshot does, without touching the container frame, so `sidebarOrderedPanelIds()` now orders from it through the new `SpatialPanelOrder`, and the tree-based entry point delegates to the same type. Reveal re-projects each row once, and a resize of the terminal area no longer invalidates the sidebar root. Co-Authored-By: Claude Opus 5 --- .../ExternalTreeNode+SpatialOrder.swift | 22 ++----- .../Geometry/SpatialPanelOrder.swift | 61 +++++++++++++++++++ .../CmuxPanesTests/SpatialOrderTests.swift | 28 +++++++++ Sources/Workspace.swift | 22 +++++-- 4 files changed, 109 insertions(+), 24 deletions(-) create mode 100644 Packages/macOS/CmuxPanes/Sources/CmuxPanes/Geometry/SpatialPanelOrder.swift diff --git a/Packages/macOS/CmuxPanes/Sources/CmuxPanes/Geometry/ExternalTreeNode+SpatialOrder.swift b/Packages/macOS/CmuxPanes/Sources/CmuxPanes/Geometry/ExternalTreeNode+SpatialOrder.swift index 40d9e8f424b1..1338823f9e8e 100644 --- a/Packages/macOS/CmuxPanes/Sources/CmuxPanes/Geometry/ExternalTreeNode+SpatialOrder.swift +++ b/Packages/macOS/CmuxPanes/Sources/CmuxPanes/Geometry/ExternalTreeNode+SpatialOrder.swift @@ -19,27 +19,13 @@ extension ExternalTreeNode { /// order, tabs within each pane in tab order, then any panels missing /// from the tree in the caller-provided stable fallback order. Formerly /// `SidebarBranchOrdering.orderedPanelIds(tree:paneTabs:fallbackPanelIds:)`. + /// Building the ``ExternalTreeNode`` this reads costs a live container-frame + /// read, so a SwiftUI `body` should use ``SpatialPanelOrder`` directly. public func orderedPanelIds( paneTabs: [String: [UUID]], fallbackPanelIds: [UUID] ) -> [UUID] { - var ordered: [UUID] = [] - var seen: Set = [] - - for paneId in orderedPaneIds { - for panelId in paneTabs[paneId] ?? [] { - if seen.insert(panelId).inserted { - ordered.append(panelId) - } - } - } - - for panelId in fallbackPanelIds { - if seen.insert(panelId).inserted { - ordered.append(panelId) - } - } - - return ordered + SpatialPanelOrder(orderedPaneIds: orderedPaneIds) + .panelIds(paneTabs: paneTabs, fallbackPanelIds: fallbackPanelIds) } } diff --git a/Packages/macOS/CmuxPanes/Sources/CmuxPanes/Geometry/SpatialPanelOrder.swift b/Packages/macOS/CmuxPanes/Sources/CmuxPanes/Geometry/SpatialPanelOrder.swift new file mode 100644 index 000000000000..26045d564474 --- /dev/null +++ b/Packages/macOS/CmuxPanes/Sources/CmuxPanes/Geometry/SpatialPanelOrder.swift @@ -0,0 +1,61 @@ +public import Foundation + +/// Orders a workspace's panel ids from pane order alone, without reading pane geometry. +/// +/// ``ExternalTreeNode/orderedPanelIds(paneTabs:fallbackPanelIds:)`` needs a +/// `Bonsplit` tree snapshot, and `BonsplitController.treeSnapshot()` reads the +/// live split container frame to turn normalized bounds into pixel rects. A +/// SwiftUI `body` that builds one therefore observes split-container geometry +/// and re-runs whenever the content area resizes. +/// +/// Callers on a render path pass `BonsplitController.allPaneIds` here instead. +/// It walks the same depth-first, first-then-second recursion the tree snapshot +/// reports, so the resulting order is identical, but it touches no frames. +/// +/// ```swift +/// SpatialPanelOrder(orderedPaneIds: bonsplitController.allPaneIds.map(\.id.uuidString)) +/// .panelIds(paneTabs: paneTabs, fallbackPanelIds: fallbackPanelIds) +/// ``` +public struct SpatialPanelOrder { + private let orderedPaneIds: [String] + + /// Creates an order over panes already listed in on-screen order. + /// - Parameter orderedPaneIds: Pane identifier strings, first/top pane first. + public init(orderedPaneIds: [String]) { + self.orderedPaneIds = orderedPaneIds + } + + /// Panel ids in on-screen spatial order. + /// + /// Panes appear in the order given to ``init(orderedPaneIds:)``, tabs within + /// each pane in tab order, then any panels missing from the tree in the + /// caller-provided stable fallback order. Repeated panel ids are dropped + /// after their first appearance. + /// - Parameters: + /// - paneTabs: Panel ids per pane identifier, in that pane's tab order. + /// - fallbackPanelIds: Stable order for panels that no pane lists. + /// - Returns: The deduplicated panel ids in display order. + public func panelIds( + paneTabs: [String: [UUID]], + fallbackPanelIds: [UUID] + ) -> [UUID] { + var ordered: [UUID] = [] + var seen: Set = [] + + for paneId in orderedPaneIds { + for panelId in paneTabs[paneId] ?? [] { + if seen.insert(panelId).inserted { + ordered.append(panelId) + } + } + } + + for panelId in fallbackPanelIds { + if seen.insert(panelId).inserted { + ordered.append(panelId) + } + } + + return ordered + } +} diff --git a/Packages/macOS/CmuxPanes/Tests/CmuxPanesTests/SpatialOrderTests.swift b/Packages/macOS/CmuxPanes/Tests/CmuxPanesTests/SpatialOrderTests.swift index 7d521a5f4e34..60ed7787cd9b 100644 --- a/Packages/macOS/CmuxPanes/Tests/CmuxPanesTests/SpatialOrderTests.swift +++ b/Packages/macOS/CmuxPanes/Tests/CmuxPanesTests/SpatialOrderTests.swift @@ -39,6 +39,34 @@ import Bonsplit #expect(result == [p1, p2, p3, orphan]) } + /// A render path passes `BonsplitController.allPaneIds` instead of building a + /// tree snapshot, because the snapshot reads the live container frame. Both + /// walks are depth-first first/second, so the panel order must not differ. + @Test func spatialPanelOrderMatchesTreeDerivedOrder() { + 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: [String: [UUID]] = ["a": [p1, p2], "b": [p3, p1], "c": []] + let fallbackPanelIds = [orphan, p2] + + let fromPaneOrder = SpatialPanelOrder(orderedPaneIds: ["a", "b", "c"]) + .panelIds(paneTabs: paneTabs, fallbackPanelIds: fallbackPanelIds) + + #expect(fromPaneOrder == [p1, p2, p3, orphan]) + #expect( + fromPaneOrder == tree.orderedPanelIds( + paneTabs: paneTabs, + fallbackPanelIds: fallbackPanelIds + ) + ) + } + @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 f1a12d07c773..59bf1d366ea1 100644 --- a/Sources/Workspace.swift +++ b/Sources/Workspace.swift @@ -6589,9 +6589,19 @@ final class Workspace: Identifiable, ObservableObject, FilePreviewTabMetadataHos recomputeListeningPorts() } + /// Panel ids in on-screen order, read from pane order only. + /// + /// The sidebar row projection calls this from a SwiftUI `body`, so it must + /// not touch split geometry: `treeSnapshot()` reads the Bonsplit container + /// frame to build pixel rects, which subscribed the sidebar body to + /// `SplitViewController.containerFrame`. Revealing the sidebar resizes that + /// container, so every workspace row was projected a second time in the same + /// run-loop turn. `allPaneIds` walks the same depth-first first/second + /// recursion the tree snapshot reports, without reading a frame. 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) } @@ -6600,11 +6610,11 @@ final class Workspace: Identifiable, ObservableObject, FilePreviewTabMetadataHos ) let fallbackPanelIds = panels.keys.sorted { $0.uuidString < $1.uuidString } - let tree = bonsplitController.treeSnapshot() - return tree.orderedPanelIds( - paneTabs: paneTabs, - fallbackPanelIds: fallbackPanelIds - ) + return SpatialPanelOrder(orderedPaneIds: orderedPaneIds.map { $0.id.uuidString }) + .panelIds( + paneTabs: paneTabs, + fallbackPanelIds: fallbackPanelIds + ) } func sidebarFinderDirectory() -> String? {