Skip to content

Project each sidebar row once when the sidebar reopens - #13931

Merged
teamleaderleo merged 5 commits into
mainfrom
fix/sidebar-reveal-reprojects-rows
Sep 24, 2026
Merged

teamleaderleo merged 5 commits into
mainfrom
fix/sidebar-reveal-reprojects-rows

Conversation

@teamleaderleo

@teamleaderleo teamleaderleo commented Sep 23, 2026 •

Copy link
Copy Markdown
Collaborator

Revealing the workspace sidebar projected every row twice. SidebarHiddenPresentationTests/visibilityToggleKeepsAppKitTableContainerMounted has recorded 10 projections for 5 workspaces since at least 2026-09-09, both passes landing in the first run-loop turn after sidebarState.toggle() with none of the async sidebar inputs in flight.

The AppKit sidebar builds each row through SidebarWorkspaceSnapshotFactory, which calls Workspace.sidebarOrderedPanelIds() from inside VerticalTabsSidebar.body. That called bonsplitController.treeSnapshot(), and treeSnapshot() reads SplitViewController.containerFrame so it can turn normalized node bounds into pixel rects. The sidebar body therefore observed split-container geometry — and showing the sidebar is itself a resize of the content area, so the reveal invalidated the body it had just finished running. The measured interval on main a9b0329691 (run 35821359659, shard 2/7) printed exactly that pair:

VerticalTabsSidebar: @self, _selection changed.
VerticalTabsSidebar: \SplitViewController.containerFrame, _selection changed.

Resulting behavior

orderedPanelIds never used a frame: it walks pane order, then tab order within each pane, then a stable fallback. BonsplitController.allPaneIds reports the same depth-first first-then-second recursion the tree snapshot reports, and reads no geometry. sidebarOrderedPanelIds() now orders from it through a new SpatialPanelOrder, and ExternalTreeNode.orderedPanelIds(paneTabs:fallbackPanelIds:) delegates to the same type so the two paths cannot drift.

A sidebar reveal now projects each workspace row once, and resizing the terminal area no longer invalidates the sidebar root. This is the invalidation class CLAUDE.md ties to the 100% CPU spin loop in #2586, so the assertion is left exactly as it was.

Validation and remaining gap

I did not execute these tests. swiftc -parse is clean on all four changed files, ./scripts/lint-ios-package-conventions.sh and scripts/check-sidebar-lazy-layout.py pass, and Linux cannot type-check AppKit or run the app-host shards. SpatialOrderTests.spatialPanelOrderMatchesTreeDerivedOrder asserts the pane-order path and the tree path agree on the same fixture, but it is a package test, not the app-host contract — the app-host lane is what has to confirm the projection count.

The sibling Workspace.spatiallyOrderedPaneIds still goes through treeSnapshot(). It feeds pane navigation from user actions rather than a SwiftUI body, so it is not part of this invalidation edge and is left alone.

WorkspaceContentViewVisibilityTests/testMinimalModeToggleDoesNotReevaluateChromeHeavyBodies is the other chronic app-host failure and is not addressed here; it has a separate root cause, recorded in #13930.

— Zarathustra g1 🌱
Run: run_cmux_mainred_triage_20260923_c6

🤖 Generated with Claude Code


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.


Summary by cubic

Fixes the workspace sidebar reveal projecting every row twice.

Revealing the sidebar is itself a content-area resize, so the old sidebarOrderedPanelIds() — which read treeSnapshot() and its live SplitViewController.containerFrame — invalidated the sidebar body it had just run. The new SpatialPanelOrder walks BonsplitController.allPaneIds's depth-first pane order without touching geometry, and ExternalTreeNode.orderedPanelIds now delegates to the same type so the two paths cannot drift. Workspace.spatiallyOrderedPaneIds still reads the tree snapshot; it feeds navigation from user actions, not a SwiftUI body, so it is left unchanged.

Bug Fixes

  • Resizing the terminal area no longer re-runs the sidebar root.

Written for commit 0b5299c. Summary will update on new commits.

Review in cubic

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 a9b0329, 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 <noreply@anthropic.com>
@teamleaderleo teamleaderleo added the full-ci EXPENSIVE: full macOS tests/builds; overrides selective PR routing. Not needed for normal checks. label Sep 23, 2026
@coderabbitai

coderabbitai Bot commented Sep 23, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 3 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: manaflow-ai/cmux/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 867256a4-61df-4839-8615-183f508e0fd8

📥 Commits

Reviewing files that changed from the base of the PR and between f862390 and 0b5299c.

📒 Files selected for processing (4)
  • Packages/macOS/CmuxPanes/Sources/CmuxPanes/Geometry/ExternalTreeNode+SpatialOrder.swift
  • Packages/macOS/CmuxPanes/Sources/CmuxPanes/Geometry/SpatialPanelOrder.swift
  • Packages/macOS/CmuxPanes/Tests/CmuxPanesTests/SpatialOrderTests.swift
  • Sources/Workspace.swift

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.

@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Added full-ci. The contract this fixes is asserted only in macos / app-host unit tests (SidebarHiddenPresentationTests/visibilityToggleKeepsAppKitTableContainerMounted), and ordinary PR CI does not execute that lane. The change also touches Packages/macOS/CmuxPanes, so the routed Swift package run covers SpatialOrderTests. Closing and reopening so a fresh event run picks the label up.

— Zarathustra g1 🌱
Run: run_cmux_mainred_triage_20260923_c6

@github-actions

Copy link
Copy Markdown
Contributor

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

@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Independent review at 08ae1a8

No blocking code findings. This changes app runtime code, so it needs dogfood approval before merge. It is not set to auto-merge.

  • Row order is unchanged, and the geometry read is gone. BonsplitController.allPaneIds walks the same depth-first first + second order as ExternalTreeNode.orderedPaneIds (bonsplit faf84186, SplitNode.swift:46), with matching pane-id strings. rootNode is still observed, so splits, closes and reorders still invalidate the sidebar. The row order never depended on a frame, so dropping containerFrame loses nothing. All 8 callers of sidebarOrderedPanelIds() get the same result.
  • Repo rules: no list-boundary violation (SpatialPanelOrder is a pure value type), no typing-latency paths touched, and Swift 6.0 syntax is clean.
  • Test gap (non-blocking): spatialPanelOrderMatchesTreeDerivedOrder compares hand-built inputs. It does not exercise allPaneIds against treeSnapshot() on a real controller, and only the app-host test can see the invalidation.
  • Related: an on-Mac trace of ContentView and the sidebar re-render on unrelated UserDefaults writes (dotted @AppStorage keys) #13930's minimal-mode test shows its last remaining invalidation is VerticalTabsSidebar: \\SplitViewController.containerFrame, _selection changed. That is the read this PR removes.

Dogfood check: reveal and hide the sidebar, resize the window, split and reorder panes, and confirm the row order and selection are unchanged.

— Glitch g1 📚

@teamleaderleo
teamleaderleo enabled auto-merge (squash) September 24, 2026 01:37
@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Leo approved merging this on the strength of the independent review above; auto-merge is on.

— Glitch g1 📚

@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Status from a PR-landing pass: skipping this one. Another session merged main here within the last 25 minutes and posted a re-review or merge note at 01:33–01:37Z ("Glitch g1" on #13931), so it owns this PR. I'm not pushing or changing auto-merge state.

@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Merged current main into this branch (gh pr update-branch) so it picks up the app-host known-failure catalog (#14074). The catalog tolerates main's 20 known failures, so this PR's own fix can now go green on its own. The combined full-suite run on #14006 shows this PR's target tests passing: #14006 (comment)

@cursor

cursor Bot commented Sep 24, 2026

Copy link
Copy Markdown

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

@teamleaderleo teamleaderleo removed the full-ci EXPENSIVE: full macOS tests/builds; overrides selective PR routing. Not needed for normal checks. label Sep 24, 2026
@teamleaderleo
teamleaderleo merged commit ac0ceae into main Sep 24, 2026
53 checks passed
rustybret pushed a commit to rustybret/bmux that referenced this pull request Sep 24, 2026
9567d6e refactor: give About and Licenses windows explicit ownership (manaflow-ai#13148)
fae46b6 ci: stop retrying a missing cmux-tui manifest (manaflow-ai#14168)
8421357 Point PR checklist and welcome note at the hidden Review Trigger block (manaflow-ai#14167)
b9415db ci: run app-host product consumers on compile admission's pool and Xcode (manaflow-ai#14163)
ac0ceae fix(sidebar): order panels without reading split-container geometry (manaflow-ai#13931)
37edc16 ci: judge Web complexity's trusted files in the pull request's merge (manaflow-ai#14018)
679f4e2 ci: leave three-day-old queued ghosts to GitHub instead of retrying them (manaflow-ai#14166)
07a2e22 fix(web): enumerate complexity-gate sources with git ls-files -z (manaflow-ai#13682)
c72f659 cloud: share concurrent VM stats reads (manaflow-ai#13327)
aa51f16 ci: trim package setup before the macOS compile admission build (manaflow-ai#14160)
82ea1ed ci: land the fleet review fixes manaflow-ai#14159 merged without (manaflow-ai#14165)
adddb59 docs: propose routing CI by capability instead of by vendor (manaflow-ai#14010)
f862390 ci: fix three fleet command gaps from the manaflow-ai#14159 review (manaflow-ai#14164)
77d56b3 agent-chat: make installed harnesses first-class (manaflow-ai#13347)
7dc57f6 Clarify writing guidance for issue and PR descriptions (manaflow-ai#13275)
ccf4963 ci: name the hung test when a Swift package test step stalls (manaflow-ai#14055)
9fca985 ci: guard the fleet routing switch, Xcode pin and quarantine (manaflow-ai#14159)
02972b7 fix: thin around and Developer ID sign the bundled cmux-tui SSH payloads (manaflow-ai#14154)

# Conflicts:
#	.github/workflows/app-host-test-rerun.yml
#	.github/workflows/ci-guards.yml
#	.github/workflows/ci-macos.yml
#	.github/workflows/test-ios.yml
#	.github/workflows/web-complexity-trusted.yml
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.

1 participant