Skip to content

fix(sidebar): order panels without reading a single tab title - #12023

Closed
STRML wants to merge 1 commit into
manaflow-ai:mainfrom
STRML:fix/sidebar-order-without-tab-titles-v2
Closed

STRML wants to merge 1 commit into
manaflow-ai:mainfrom
STRML:fix/sidebar-order-without-tab-titles-v2

Conversation

@STRML

@STRML STRML commented Sep 5, 2026 •

Copy link
Copy Markdown
Contributor

Status: updated onto main (78f4c82504) as merge commit 25a2e642a5. Still a draft: it calls bonsplitController.tabIds(inPane:), which lands in manaflow-ai/bonsplit#233 and is not in the pinned submodule yet. Nothing else in the diff needs that PR.

Reviewer's guide

  • Read Sources/Workspace.swift (sidebarOrderedPanelIds) and the new PaneSpatialOrder enum; the tree method delegates to it, so there is one implementation.
  • Tests: Packages/macOS/CmuxPanes/Tests/CmuxPanesTests/SpatialOrderTests.swift.
  • Skip the pbxproj line.

Draft: blocked on manaflow-ai/bonsplit#233. This calls bonsplitController.tabIds(inPane:), which does not exist in the bonsplit commit currently pinned here. It will not compile until that PR merges and the submodule pointer moves. Opening it now so the cmux-side change is reviewable alongside it. The pointer bump is deliberately not in this diff.

Summary

sidebarOrderedPanelIds() reads every tab's title to produce a list of ids.

It asks bonsplit for tabs(inPane:) and builds a treeSnapshot(). 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 invalidates them for an ordering that never looks at a title.

allPaneIds already walks the tree first-then-second, exactly as ExternalTreeNode.orderedPaneIds does, so the snapshot was not buying the pane order either.

Change

The call site asks for tabIds(inPane:) and never mentions a tab. The ordering moves out of the ExternalTreeNode extension into PaneSpatialOrder, which takes pane ids directly; the tree method delegates to it, so there is still one implementation and one set of tests.

Measurement

From a 20.7s sample of a running cmux, main thread:

Frame Time
sidebarOrderedPanelIds path 142.4 ms
VerticalTabsSidebar body 1058.6 ms (21.4%)

After, in a 16013-sample run with tab titles updating, sidebarOrderedPanelIds costs 8 samples (0.05%) and treeSnapshot does not appear on the main thread at all.

Two caveats on those numbers, since they are not a clean A/B. The before and after traces are from different sessions under different load, so treat the 142.4 ms against 8 samples as a direction rather than a ratio. The VerticalTabsSidebar figure is the whole body, which this change only partly accounts for; the rest is #5832.

Related


https://claude.ai/code/session_01Bj791kgph6Xk9CJf9c41Db

@vercel

vercel Bot commented Sep 5, 2026

Copy link
Copy Markdown

@STRML is attempting to deploy a commit to the Manaflow Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitai

coderabbitai Bot commented Sep 5, 2026

Copy link
Copy Markdown

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@STRML

STRML commented Sep 16, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto main (78f4c82), no conflicts. Still a draft on purpose: it calls tabIds(inPane:), which lands in manaflow-ai/bonsplit#233 and is not in the pinned submodule yet, so it cannot compile until that merges and the pointer moves. Everything else in the diff is reviewable now; the description opens with a reviewer's guide. @lawrencecchen if bonsplit#233 looks good, this is the cmux-side half.

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.
@STRML
STRML force-pushed the fix/sidebar-order-without-tab-titles-v2 branch from 25a2e64 to 500f9c5 Compare September 24, 2026 13:39
@STRML

STRML commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by #13931 (ac0ceae), which landed this exact mechanism on main 2026-09-24: SpatialPanelOrder in Packages/macOS/CmuxPanes/Sources/CmuxPanes/Geometry/SpatialPanelOrder.swift orders from bonsplitController.allPaneIds with no tree snapshot, and sidebarOrderedPanelIds() delegates to it. Upstream's paneTabs[paneId] ?? [] loop already exhibits this PR's one behavior #13931 had no test for (a pane id missing from paneTabs is skipped, not fatal), so the only thing left here is a duplicate.\n\nThanks to maintainers for merging the same fix; nothing left to carry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants