Repository navigation
Apply pane zoom by collapsing splits in place instead of swapping the rendered node - #175
Open
johnlindquist wants to merge 2 commits into
Open
johnlindquist wants to merge 2 commits into
johnlindquist wants to merge 2 commits into
Conversation
… rendered node Rendering `zoomedNode ?? rootNode` changed SwiftUI structural identity on every zoom toggle, tearing down and rebuilding the entire AppKit split hierarchy (every NSSplitView plus one NSHostingController per pane). With multiple panes open, un-zoom rebuilt all N panes and cost >1s in debug builds (8 panes: maximize 444ms / minimize 1303ms median). Zoom now keeps the tree mounted and collapses the side of each split that does not contain the zoomed pane (hidden arranged subviews are excluded from NSSplitView tiling). Split coordinators register a collapse applier with the controller so the AppKit collapse runs synchronously in the same runloop turn as the toggle; the SwiftUI pass re-applies the same state idempotently as a backstop, reading live controller state so stale update passes cannot flash the pre-toggle layout. Divider sync/persistence and entry-animation placement are suspended while a split is zoom-collapsed so the temporary full-size layout never overwrites the model ratio. New window-hosted benchmark (ZoomTogglePerformanceTests, 8 panes, 12 cycles): maximize 443.8ms -> ~170ms median, minimize 1303.4ms -> 3.3ms median, pane content NSView churn 1/8 -> 0/0 per toggle. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughChangesZoom-collapse flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant SplitViewController
participant SplitContainerView
participant NSSplitView
User->>SplitViewController: Toggle pane zoom
SplitViewController->>SplitContainerView: Notify registered applier
SplitContainerView->>NSSplitView: Hide or restore arranged subview
NSSplitView-->>SplitContainerView: Adjust subviews
SplitContainerView->>SplitViewController: Restore divider ratio when unzoomed
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/Bonsplit/Internal/Views/SplitContainerView.swift`:
- Around line 269-276: Use the captured local animationOrigin variable in the
zoomHiddenIndex early-return branch of SplitContainerView’s update logic instead
of checking the already-cleared splitState.animationOrigin; when it is non-nil,
clear it and reset context.coordinator.isAnimating to prevent the coordinator
from remaining permanently animating.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5b5f658e-2839-4913-ba11-69ee5ef1ae92
📒 Files selected for processing (5)
Sources/Bonsplit/Internal/Controllers/SplitViewController.swiftSources/Bonsplit/Internal/Views/SplitContainerView.swiftSources/Bonsplit/Internal/Views/SplitNodeView.swiftSources/Bonsplit/Internal/Views/SplitViewContainer.swiftTests/BonsplitTests/ZoomTogglePerformanceTests.swift
Comment on lines
+269
to
+276
| if context.coordinator.zoomHiddenIndex != nil { | ||
| context.coordinator.didApplyInitialDividerPosition = true | ||
| if splitState.animationOrigin != nil { | ||
| splitState.animationOrigin = nil | ||
| context.coordinator.isAnimating = false | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
isAnimating can get permanently stuck when a split is created during active zoom.
At line 249, splitState.animationOrigin is cleared (= nil). The zoom-collapsed early-return path at line 271 checks splitState.animationOrigin != nil, which will always be false. If shouldAnimate was true, isAnimating was set to true at line 256 but is never reset in this path. This permanently breaks syncPosition and splitViewDidResizeSubviews for the affected coordinator.
The fix is to use the captured local animationOrigin variable (line 227) instead of the already-cleared splitState.animationOrigin. The non-zoom-collapsed path at line 322 already correctly uses the local animationOrigin.
🐛 Proposed fix: use captured `animationOrigin` instead of already-cleared `splitState.animationOrigin`
if context.coordinator.zoomHiddenIndex != nil {
context.coordinator.didApplyInitialDividerPosition = true
- if splitState.animationOrigin != nil {
- splitState.animationOrigin = nil
+ if animationOrigin != nil {
context.coordinator.isAnimating = false
}
return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if context.coordinator.zoomHiddenIndex != nil { | |
| context.coordinator.didApplyInitialDividerPosition = true | |
| if splitState.animationOrigin != nil { | |
| splitState.animationOrigin = nil | |
| context.coordinator.isAnimating = false | |
| } | |
| return | |
| } | |
| if context.coordinator.zoomHiddenIndex != nil { | |
| context.coordinator.didApplyInitialDividerPosition = true | |
| if animationOrigin != nil { | |
| context.coordinator.isAnimating = false | |
| } | |
| return | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/Bonsplit/Internal/Views/SplitContainerView.swift` around lines 269 -
276, Use the captured local animationOrigin variable in the zoomHiddenIndex
early-return branch of SplitContainerView’s update logic instead of checking the
already-cleared splitState.animationOrigin; when it is non-nil, clear it and
reset context.coordinator.isAnimating to prevent the coordinator from remaining
permanently animating.
…invalidation or forced layout Under an external resize storm (socket resize-pane driving nested splits while 10 TUI panes render), two costs dominated the main thread: - syncPosition forced layoutSubtreeIfNeeded per applied divider change (~30% of main-thread samples): setPosition already updates arranged frames and marks layout dirty, so let AppKit coalesce the recursive pass into the display cycle instead of flushing per tick. - updateNSView read splitState.dividerPosition, so every model divider change invalidated the representable and cascaded a full pane-subtree diff (tab bars included) through the hosted rootView reassignment. Divider changes are now watched with Observation and applied straight to the NSSplitView by the coordinator; SwiftUI only handles structural updates. Arming is deferred a runloop turn so the tracked read is not captured by SwiftUI's own observation scope. Interactive divider drags are unaffected (view-driven writes still persist via splitViewDidResizeSubviews; the watcher's re-apply no-ops when view and model already agree). cmux end-to-end (10 live claude/codex TUIs, scripted resize storm, debug build): 93% -> ~50-60% app CPU; SplitContainerView sync work no longer appears in the top main-thread samples. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Author
|
Added a second commit (8062854): model divider changes are now applied to the NSSplitView directly via Observation — no SwiftUI representable invalidation and no forced layoutSubtreeIfNeeded per divider tick. Under a scripted resize storm with 10 live claude/codex TUI panes, app CPU went from 93% to ~50-60% and SplitContainerView sync work disappeared from the top main-thread samples. All 186 package tests pass. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Toggling pane zoom rendered
zoomedNode ?? rootNode, which changes SwiftUI structural identity — the entire AppKit split hierarchy (everyNSSplitViewplus oneNSHostingControllerper pane) was torn down on maximize and fully rebuilt on minimize. Cost scales with pane count: with 8 panes, maximize took 443.8 ms and minimize 1303.4 ms median (debug), and minimize recreated all 8 pane content views. In cmux this shows up as a very laggycmd+shift+enterwith multiple panes open.Fix
Zoom keeps the tree mounted and collapses the side of each split that does not contain the zoomed pane (hidden arranged subviews are excluded from NSSplitView tiling, so the zoom path expands to the full container).
SplitViewController; the AppKit collapse runs in the same runloop turn astogglePaneZoom()instead of waiting for SwiftUI to re-thread state through every nested hosting controller (display-paced, hundreds of ms on deep trees).updateNSViewre-applies the same state reading live controller state, so stale SwiftUI passes cannot flash the pre-toggle layout (restore-then-recollapse flap).Results
New window-hosted benchmark
Tests/BonsplitTests/ZoomTogglePerformanceTests.swift(8 panes, 12 cycles,swift test --filter ZoomTogglePerformanceTests):* residual is offscreen-window SwiftUI update scheduling in the headless harness; a probe confirms AppKit frames are final within the toggle call. In cmux (11 panes, real key events) end-to-end settle went from 315→115 ms (maximize) and 825→130 ms (minimize), with terminal-portal detach/attach per minimize going from 18–20 to 0.
Full benchmark report: https://polite-sandal-rgcv.here.now/
All 186 package tests pass.
Note for cmux integration: cmux's
WorkspaceContentViewre-keyed the Bonsplit subtree with.id(zoomState)(workaround for stale pane chrome above browser portals under the old node-swap rendering). That re-key must be removed together with this change, and pane content visibility made zoom-aware, since collapsed panes now stay mounted; the cmux-side commit is prepared to land with the submodule bump.🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Apply pane zoom by collapsing the non-zoomed side of each split in place, and move divider syncing out of SwiftUI. Keeps the AppKit split tree mounted, makes zoom toggles much faster, and cuts CPU during resize storms.
Refactors
SplitViewControllernotifies registered split coordinators onzoomedPaneIdchange in the same runloop turn.updateNSViewre-applies collapse using live controller state to avoid layout flashes.dividerPositionand apply changes directly toNSSplitView(noupdateNSViewdependency, nolayoutSubtreeIfNeededon ticks); reduces main-thread work under heavy resizes (cmux: app CPU ~93% → ~50–60%).SplitViewContainerpasseszoomedPaneIddown; appliers are registered inmakeNSViewand unregistered indismantleNSView.ZoomTogglePerformanceTests.swift(8 panes): minimize 1303.4 ms → 3.3 ms; maximize 443.8 ms → ~170 ms; pane content NSView churn → 0.Migration
.id(zoomState)fromWorkspaceContentViewand make pane content visibility zoom-aware, since collapsed panes now remain mounted.Written for commit 8062854. Summary will update on new commits.
Summary by CodeRabbit