Repository navigation
Conversation
Drag start and end come from the divider's mouse-tracking lifecycle (mouseDown brackets AppKit's tracking loop), not from which event happens to be current inside a resize callback — the old inference missed any drag that paused before release. BonsplitController.isDividerDragActive and the dragDidBegin/End delegate calls expose the session to hosts. An imposed split re-applies its extent only on a fresh imposition, or when the divider moved while the split view's size did not — putting a drifted divider back where nothing else will. Re-asserting against a resized container fought AppKit from inside its own layout pass, recursively; a main-thread sample showed the whole thread in that fight. When a refused parent apply lands on a later retry, nested imposed splits re-apply against their settled containers.
📝 WalkthroughWalkthroughChangesDivider drag lifecycle and layout convergence
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ThemedSplitView
participant Coordinator
participant SplitViewController
participant BonsplitController
participant BonsplitDelegate
User->>ThemedSplitView: Start divider drag
ThemedSplitView->>Coordinator: dividerDragSessionChanged(true)
ThemedSplitView->>SplitViewController: noteDividerDragSession(true)
SplitViewController->>BonsplitController: onDividerDragSessionChange(true)
BonsplitController->>BonsplitDelegate: splitTabBarDividerDragDidBegin(controller)
ThemedSplitView->>ThemedSplitView: Track divider movement
ThemedSplitView->>Coordinator: dividerDragSessionChanged(false)
ThemedSplitView->>SplitViewController: noteDividerDragSession(false)
SplitViewController->>BonsplitController: onDividerDragSessionChange(false)
BonsplitController->>BonsplitDelegate: splitTabBarDividerDragDidEnd(controller)
🚥 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 |
Greptile SummaryThis PR makes divider drags explicit sessions and changes how imposed pane sizes settle after layout changes. The main changes are:
Confidence Score: 4/5Overlapping sessions, expanded AppKit hit regions, and container resizes can break the new drag and imposed-size contracts.
Sources/Bonsplit/Internal/Controllers/SplitViewController.swift and Sources/Bonsplit/Internal/Views/SplitContainerView.swift Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant User
participant Split as ThemedSplitView
participant State as SplitViewController
participant Host
User->>Split: mouseDown on divider
Split->>State: begin session
State->>Host: drag did begin
Split->>Split: AppKit tracking loop
User->>Split: mouseUp
Split->>State: end session
State->>Host: drag did end
Split->>Host: final geometry change
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant User
participant Split as ThemedSplitView
participant State as SplitViewController
participant Host
User->>Split: mouseDown on divider
Split->>State: begin session
State->>Host: drag did begin
Split->>Split: AppKit tracking loop
User->>Split: mouseUp
Split->>State: end session
State->>Host: drag did end
Split->>Host: final geometry change
Reviews (1): Last reviewed commit: "Divider drags are sessions; imposed spli..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Bonsplit/Internal/Views/SplitContainerView.swift (1)
1006-1039: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winNested imposed descendants aren't renudged when the ancestor converges on the first try.
renudgeImposedDescendantsis only invoked fromretryImposedIfStillShort's own eventual convergence (Lines 877-887). ThissyncPositionbranch callssetPositionSafely(target, in: splitView, layout: true)too, and when it converges immediately (imposedRetryBudget = 0, no retry scheduled), any descendant split with its own imposed extent — proportionally resized by this same layout pass — never gets nudged back. PersplitViewDidResizeSubviews(Lines 1289-1292), that descendant explicitly skips re-syncing on its own resize notification because it has an imposed extent, so nothing else corrects it: same failure mode the retry-path renudge fixes, just reached via the more common "first attempt succeeds" path instead of the retry path.🐛 Proposed fix: renudge descendants on immediate convergence too
} else if renudged || (moved && availUnchanged) { setPositionSafely(target, in: splitView, layout: true) lastImposedEpoch = splitState.imposedEpoch lastImposedAvail = available lastImposedOutcome = splitState.orientation == .horizontal ? splitView.arrangedSubviews[0].frame.width : splitView.arrangedSubviews[0].frame.height ... if abs((lastImposedOutcome ?? target) - target) > 0.01 { imposedRetryBudget = 5 imposedRetrySplitView = splitView scheduleImposedRetry() } else { imposedRetryBudget = 0 + renudgeImposedDescendants(of: splitState) } }🤖 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 1006 - 1039, Update the immediate-convergence branch in syncPosition, after setPositionSafely confirms the imposed position and imposedRetryBudget is set to zero, to invoke renudgeImposedDescendants for the affected split view. Preserve the existing retry scheduling behavior for non-converged outcomes and ensure descendants are renudged only when convergence occurs without a deferred retry.
🤖 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/Controllers/SplitViewController.swift`:
- Around line 65-76: Update noteDividerDragSession(_:) so
onDividerDragSessionChange is invoked only when activeDividerDragSessions
crosses between zero and positive: notify true on 0→1 and false on 1→0, while
preserving the clamped counter behavior for overlapping or unmatched sessions.
---
Outside diff comments:
In `@Sources/Bonsplit/Internal/Views/SplitContainerView.swift`:
- Around line 1006-1039: Update the immediate-convergence branch in
syncPosition, after setPositionSafely confirms the imposed position and
imposedRetryBudget is set to zero, to invoke renudgeImposedDescendants for the
affected split view. Preserve the existing retry scheduling behavior for
non-converged outcomes and ensure descendants are renudged only when convergence
occurs without a deferred retry.
🪄 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: e7543008-6cae-4614-aaa1-31da76d9f86f
📒 Files selected for processing (4)
Sources/Bonsplit/Internal/Controllers/SplitViewController.swiftSources/Bonsplit/Internal/Views/SplitContainerView.swiftSources/Bonsplit/Public/BonsplitController.swiftSources/Bonsplit/Public/BonsplitDelegate.swift
…any divider mouseDown With overlapping sessions (a host-bracketed custom drag alongside the built-in tracking), ending one must not announce the drag over while the other still owns the divider. And the session hit test cannot faithfully reconstruct the effective divider rect AppKit tracks (our expansion unioned with AppKit's own proposal), so any mouseDown that reaches the split view — content clicks never do — brackets a session; a session around a non-drag click is a harmless no-op pair. setImposedFirstExtent now documents that a stored extent is not re-asserted across container resizes: the host imposes fresh values for new sizes.
Author
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.
When a host imposes exact pane sizes (the
setImposedFirstExtentAPI from #176) while users can also drag dividers, two pieces of divider behavior turned out to be built on guesses, and both guesses failed in a host under real use.Drag detection guessed from event coincidence. The delegate decided "the user is dragging" by checking which event happened to be current inside a resize callback, and detected the release only when a resize callback happened to arrive while the mouseUp was still the current event. Drag a divider, hold it still, release: no resize coincides with the mouseUp, no drag end is ever reported, and a host that syncs the final position elsewhere (cmux sends it to a remote tmux) never hears about the drag. The reverse guess was worse: code treated "this split has an imposed extent" as proof it was not being dragged, so a host that re-imposed on its own schedule mid-drag silently discarded the user's gesture.
Dragging is now a session with a hard start and end taken from the mouse lifecycle itself: mouseDown on the divider's hit rect begins it, and the end fires when AppKit's own tracking loop returns at release — it cannot miss a drag-pause-release.
BonsplitController.isDividerDragActiveexposes the session so hosts can hold their own sizing while the user's hand is on the divider,splitTabBarDividerDragDidBegin/Endnotify the delegate, andnoteDividerDragSessionlets hosts with custom drag surfaces — and tests — bracket a session themselves.Imposed splits fought every resize they didn't initiate. When frames moved under an imposed split — a window resize, a parent divider landing — the resize callback re-asserted the stored extent synchronously, inside the very layout pass that had moved the frames. Each re-assert ran another layout pass, which moved more frames, recursively across every mounted split; a main-thread sample during the resulting beach-ball showed 100% of the time in that fight. The update path did the same whenever the divider sat off its last recorded outcome.
The rule now: an imposed split re-applies its extent only when the host imposes again (each call bumps an epoch), or when the divider moved while the split view itself did NOT change size — that is drift with nobody else coming to correct it, and one apply settles it, since applying cannot resize the split view. When the split view's size DID change, the stored extent is out of date and the host will impose a new one computed for the new size; re-applying the old one meanwhile just moves the divider back and forth against AppKit. One addition to #176's budgeted retry: when a refused parent apply finally lands a turn later, it resizes everything nested inside after those splits already applied their extents — the retry now asks each nested imposed split to apply again against its settled container.
The drag session also arms for a divider parked against a pane at its minimum size (the old hit test skipped panes under a point wide — exactly the divider a user grabs to restore a collapsed pane), and DEBUG logging follows the new shape: session begin/end, and each imposed apply with its target, outcome, and the container size it applied in.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Make divider drags explicit sessions bracketed by mouseDown/mouseUp, and stop imposed splits from re-applying during foreign resizes. Fixes missed drag-end notifications and removes recursive layout churn during window/parent resizes.
New Features
BonsplitController.isDividerDragActive,noteDividerDragSession(_:), and delegate hookssplitTabBarDividerDragDidBegin/End.Bug Fixes
setImposedFirstExtentnow documents that stored extents are not re-asserted across container resizes.Written for commit 0ef263b. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes