Repository navigation
Divider drags are sessions; imposed splits stop fighting foreign resizes - #179
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.
…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.
retryImposedFirstExtent re-asserts a stored extent on host recovery triggers (rebuilds, tab re-shows, reconnects). Fired mid-drag it would move the divider under the pointer, so it now returns false while any drag session is active for the tree; the host's drag-end sync imposes fresh values once the session closes. Also reconciles the drag-session line with the imposed-first-extent rework it now sits on: renudgeImposedDescendants uses the renamed syncDividerNow hook, and reapplyDividerForThicknessChange records the epoch/outcome/avail memo instead of the removed lastImposedTarget.
📝 WalkthroughWalkthroughDivider drag sessions now propagate deterministically through AppKit views, coordinators, controllers, and delegates. Imposed extent synchronization accounts for active drags, resize state, retry epochs, nested splits, and forced geometry notifications. Debug logging can also route entries to an external sink. ChangesDivider interaction and imposed synchronization
Debug log routing
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant ThemedSplitView
participant SplitContainerCoordinator
participant SplitViewController
participant BonsplitController
participant BonsplitDelegate
ThemedSplitView->>SplitContainerCoordinator: Begin divider drag session
SplitContainerCoordinator->>SplitViewController: Record active session
SplitViewController->>BonsplitController: Forward session state
BonsplitController->>BonsplitDelegate: Notify drag began
ThemedSplitView->>SplitContainerCoordinator: End divider drag session
SplitContainerCoordinator->>SplitViewController: Record inactive session
SplitViewController->>BonsplitController: Force final geometry notification
BonsplitController->>BonsplitDelegate: Notify drag ended
🚥 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-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
On session end the controller notified the delegate first, so drag-end arrived before the coordinator's final didChangeGeometry — inverting the documented contract that the settled geometry has already been reported when drag-end runs. A host that buffers geometry callbacks and commits at drag-end would commit pre-drag state. The coordinator now leads on both edges of the session; hosts that suppress geometry callbacks while the session is live lose nothing, since the model is final at release and drag-end is the signal to read it.
…tent A container resize proportionally rescales dividers, so an imposed split can drift off its extent. The host re-imposes after the resize, but the fresh plan often computes the same extent as before — per-pane ideals do not depend on the container — and setImposedFirstExtent deduped that call by value, bumping no epoch. syncPosition then saw an unchanged epoch with a changed container and, per the one-writer rule, refused to re-apply: the divider stayed wedged at the drifted position until some input happened to change the number. Live signature: a settled pane one row short of its tmux assignment, persisting through reconfirm. Every explicit non-nil call now bumps the epoch, matching syncPosition's documented 'bounded by explicit calls' contract. A repeat while the divider is already at target is memo-only (no layout pass), so the re-arm cannot churn. Regression: testReimposingSameExtentAfterContainerResizeRetargetsExactly imposes 120 in 400pt, shrinks to 300 (drift to 90), re-imposes the same 120 — red without the fix, green with it.
|
Pushed a7c9a81: the live fuzz run caught an imposed split settling one terminal row short of its assignment and staying there. Root cause: a container resize drifts the divider proportionally, the host re-imposes, but the freshly planned extent is often identical (per-pane ideals do not depend on the container) and setImposedFirstExtent deduped the call by value — no epoch bump, no re-apply, wedged until some input changed the number. Every explicit non-nil call now re-arms one bounded apply; a repeat with the divider already at target is memo-only, so no churn. Regression test drives impose → proportional drift → identical re-impose (red without the fix). Suite 192/192. |
The public retryImposedFirstExtent already refused during a drag session, but the internal apply paths did not: a mid-drag setImposedFirstExtent (or a drift renudge, a descendant renudge, or a deferred syncDividerNow apply) still ran its main-queue apply during AppKit's tracking loop and yanked the divider out from under the pointer, re-marking the split imposed against the drag's own clear. Gate the apply at its funnel instead of per API. Every imposed apply runs through the coordinator's syncPosition imposed branch or its deferred retry, so those two points now consult the tree's live drag session count and refuse outright: no divider write, no memo or epoch bookkeeping, no retry budget consumed. The pending extent stays armed. When the session count crosses back to zero, the internal controller gives every still-imposed split one deferred apply. There is no epoch bump, so a split already at its target only refreshes its memos, and an interrupted retry chain resumes from the same sync with the budget it had left. Drift renudges with no session active keep working as before.
The guaranteed final notification funneled through notifyGeometryChange, whose first line drops everything within ~50ms of a fromExternal update. A quick flick released inside that window lost the one notification the delegate contract promises has already been delivered when dragDidEnd runs. Deliver it from the session counter's zero crossing in the controller instead, with an explicit force that bypasses only the suppression window and only for this path; every other caller keeps the gate. The zero crossing also covers host-bracketed sessions via noteDividerDragSession, which never ran the coordinator's drag-end path at all, and it still fires before splitTabBarDividerDragDidEnd, so the ordering the contract promises is unchanged. The coordinator's session-end callback now only clears its drag flag.
|
Pushed two commits hardening the session contract. d0a6c15 moves the drag refusal to the apply layer: every imposed apply (setImposedFirstExtent including value-identical calls, drift renudges, descendant renudges, deferred applies, retryImposedFirstExtent) refuses while any drag session owns a divider — no divider write, no bookkeeping, no budget consumed — and the pending extent stays armed; the session's zero crossing gives each still-imposed split one deferred apply and resumes interrupted retry chains. 3273b80 guarantees the drag-end geometry report: it is delivered from the zero crossing with an explicit bypass of the external-update suppression window, which otherwise swallowed the final notification for a flick-drag released within ~50ms of a remote layout apply; all other callers keep the anti-echo gate. Both landed red-first (mid-session impose visibly yanked the divider 199.5→120 before the fix; the suppression test saw dragEnded with no geometry). Suite: 194 tests, 0 failures. |
…ider A window resize rescales an imposed split proportionally, moving the divider off its extent. Since the re-apply condition became `renudged || (moved && availUnchanged)`, that drift was left for the host to fix: the theory was that a resized container always gets a fresh imposition. But a host whose per-pane ideals are container-independent computes the SAME extent for any container size and only re-imposes when its own inputs change. If the epoch-bumped applies from the last imposition all expired against not-yet-final bounds (retry budget spent while a concurrent window resize was still committing), the park was permanent — a deterministic fuzz repro held the divider at the proportional position (plan 1199pt vs view 984pt) for 50+ seconds until an unrelated input. Give the parked divider one deferred apply of its own instead of waiting for the host. Both places that observe the size change — the didResize callback and syncPosition's imposed branch — record the new available size and arm a single retry against it. Recording the size immediately bounds this to one re-arm per size change, the apply runs a runloop turn later so it never fights AppKit from inside the resize's own layout pass, and applying cannot resize the split view, so it cannot re-trigger itself. A mid-drag retry still refuses without consuming its budget; the session-end renudge resumes the chain. The contract, now stated in the doc comments: an apply never terminates off-target without re-arming on the next size change. The new regression test imposes an extent, resizes the window with no further imposition and no drag session, and asserts the first pane returns to the extent; it fails without the fix (divider stays at 192pt instead of 120pt). The existing same-extent re-imposition test asserted the drift persisted until the host's call — updated to the new contract.
|
Pushed 6568da9, root-caused from a deterministic fuzz repro. When the window resized under an unchanged imposed extent, the divider was left at AppKit's proportional position on the theory the host would impose a fresh value for the new size — but a host that computes per-pane ideals independently of the container re-imposes the same number, and only when its own inputs change. If the retry budget from the last imposition had expired against not-yet-final bounds (SwiftUI still committing a concurrent window resize), nothing ever moved the divider again: plan 1199pt, view 984pt, for 50+ seconds until an unrelated input. The fix gives a parked divider one deferred apply of its own when the container's size changes — both observation points record the new available size (one re-arm per size change) and arm a single retry that runs a turn after the resize's layout pass. Mid-drag it still refuses and resumes at session end. New regression test resizes the window with no fresh imposition and asserts the pane returns to its extent (red without the fix); one prior test had pinned the parked behavior as correct and now asserts the self-heal. Suite 195/195. |
![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 `@Tests/BonsplitTests/BonsplitTests.swift`:
- Around line 3535-3539: Update the affected Bonsplit tests around the drift and
re-application scenarios to first assert that AppKit produces an observed
off-target divider state, then separately verify the explicit re-arm or deferred
apply restores the imposed extent. For the identical re-imposition case, move
verification before the self-healing loop or use the existing internal
epoch/apply-count hook so the test proves a new application was accepted rather
than merely observing the loop’s final correction.
🪄 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: ff5da075-b6d1-43ab-898a-42cb9363d6c2
📒 Files selected for processing (3)
Sources/Bonsplit/Internal/Views/SplitContainerView.swiftSources/Bonsplit/Public/BonsplitController.swiftTests/BonsplitTests/BonsplitTests.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- Sources/Bonsplit/Public/BonsplitController.swift
- Sources/Bonsplit/Internal/Views/SplitContainerView.swift
Both container-resize tests only checked the final divider width, so they would pass even if AppKit never moved the divider off the imposed extent. Capture the divider synchronously after setContentSize, before any runloop turn — the heal is deferred a turn, so the proportional drift (192pt for the 400 to 640 grow, under 116pt for the shrink) is observable there — and only then pump and assert the recovery. The same-extent re-imposition test also lost its teeth: the size-change re-arm already restores the extent before the re-impose call runs, so the final assertion could no longer tell whether the call was accepted or deduped by value. Isolate it by perturbing the divider at constant container size with a direct AppKit setPosition (the imposition stays stored and no avail changes, so the size-change re-arm cannot fire), pumping with no impose call and asserting the divider stays put — nothing heals it autonomously — then re-imposing the identical extent and asserting the divider returns. The recovery is attributable to the explicit call alone.
dlog appended to its log file by opening a FileHandle per line and doing seekToEnd()+write. When another writer in the same process appends to the same file, the two race: lines interleave, clobber each other, and land out of timestamp order. A host app can now install an external sink that receives every line, so all appends can go through one serialized writer. Without a sink, appends keep a single O_APPEND handle open instead of reopening per line, and dump() steps aside entirely when a sink owns the file.
|
Pushed 92093ae: hosts can take over debug log appends via setExternalSink. Motivation: bonsplit's dlog and the host's own debug logger were two independent per-line seek-and-write writers on one file, which interleaved fragments and reordered lines under load — during a live capture this dropped a session-begin line and sent an investigation down a false path. With a sink installed, all lines flow through the host's single sequenced writer; standalone bonsplit keeps one persistent O_APPEND handle instead of reopening per line. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Sources/Bonsplit/Public/DebugEventLog.swift (2)
101-104: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueOperational advice:
writedoes not throw on file rotation.The comment indicates an expectation that file rotation will cause
writeto throw. In POSIX file systems, rotating a file by moving or unlinking it (e.g., standardmv/rm) does not invalidate an existing open file descriptor.writewill continue succeeding (writing to the unlinked inode) and will not throw an error, meaningappendHandlewon't be automatically closed and reopened.If you ever need to support standard external log rotation, you will typically need a file-coordination mechanism, a signal handler (e.g.,
SIGHUP), or to usecopytruncatelog rotation. The currentcatchblock correctly and successfully recovers from actual IO errors (e.g., disk full, unmounted volumes) but won't detect standard log rotation.🤖 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/Public/DebugEventLog.swift` around lines 101 - 104, Update the comment in the write error handler near appendHandle to remove the incorrect claim that file rotation causes write to throw; describe this block as recovery from actual I/O errors while preserving the existing appendHandle reset behavior.
29-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClose
appendHandlewhen installing an external sink.When an external sink takes over, the built-in file append is bypassed. Consider closing and releasing the
appendHandleimmediately to avoid holding onto an unused open file descriptor.♻️ Proposed refactor
public static func setExternalSink(_ sink: (`@Sendable` (String) -> Void)?) { let log = shared log.queue.async { log.externalSink = sink + if sink != nil { + try? log.appendHandle?.close() + log.appendHandle = nil + } } }🤖 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/Public/DebugEventLog.swift` around lines 29 - 34, Update setExternalSink to close and release the existing appendHandle when installing a non-nil external sink, before assigning externalSink. Keep the handle available when removing the external sink so built-in file logging can continue as intended.
🤖 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.
Nitpick comments:
In `@Sources/Bonsplit/Public/DebugEventLog.swift`:
- Around line 101-104: Update the comment in the write error handler near
appendHandle to remove the incorrect claim that file rotation causes write to
throw; describe this block as recovery from actual I/O errors while preserving
the existing appendHandle reset behavior.
- Around line 29-34: Update setExternalSink to close and release the existing
appendHandle when installing a non-nil external sink, before assigning
externalSink. Keep the handle available when removing the external sink so
built-in file logging can continue as intended.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b030c13e-34be-4ede-9ae3-d4cc83f47ad6
📒 Files selected for processing (2)
Sources/Bonsplit/Public/DebugEventLog.swiftTests/BonsplitTests/BonsplitTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Tests/BonsplitTests/BonsplitTests.swift
2 participants
Status: a live fuzz marathon runs against a cmux build carrying this branch; the PR is open for review in parallel rather than held for it. Results land in a comment when the run finishes.
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: any mouseDown that reaches the split view begins it (content clicks never do — pane content swallows them), and the end fires when AppKit's own tracking loop returns at release — it cannot miss a drag-pause-release, and a session bracketing a non-drag click is a harmless no-op pair. The session deliberately does not re-derive the divider hit rect: AppKit unions our expanded rect with its own proposal, so any reconstruction would miss exactly the edge cases users grab, like a divider parked against a pane at its minimum size.
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. With overlapping sessions the delegate hears only the zero crossings: ending one session while another still owns the divider does not announce the drag over.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 old extent is not re-fought during the resize — but the split is not left parked either: each observed size change arms exactly one deferred apply that runs a turn after the resize's layout pass, because a host whose per-pane ideals are container-independent will re-impose the same number and only when its own inputs change, so nothing else would ever put a parked divider back. An apply never terminates off-target without a re-arm edge.
setImposedFirstExtentnow documents that contract. One addition to the 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.That rule has one subtle consequence: a container resize proportionally rescales dividers, so an imposed split drifts off its extent, and the host re-imposes afterward — but the fresh plan often computes the SAME extent, because per-pane ideals do not depend on the container. Deduplicating that call by value means nothing ever puts the divider back, and since settled inputs produce identical plans forever, the drift persists through every reconfirm; the visible symptom is a pane sitting one terminal row short of its assignment indefinitely. Every explicit call therefore re-arms one bounded apply, including a repeat of the stored extent; a repeat while the divider is already at target refreshes memos without a layout pass, so the re-arm cannot churn.
The same ownership rule covers every path that can move an imposed divider, enforced at the apply layer rather than per API: while any drag session owns a divider in the tree, an imposed apply — whether from
setImposedFirstExtent(identical value or not), a drift renudge, a descendant renudge, a deferred apply, orretryImposedFirstExtent— refuses outright, consuming no retry budget and recording no bookkeeping, so nothing can move the divider under the pointer. The pending extent stays armed: when the session count returns to zero, each still-imposed split gets one deferred apply, and an interrupted retry chain resumes with its remaining budget. Drift renudges with no session active are untouched.The drag-end geometry report is also guaranteed delivery: it is issued from the session counter's zero crossing with an explicit bypass of the external-update suppression window (that gate otherwise swallows a notification within ~50ms of a remote layout apply — a quick flick-drag released inside the window would end with
dragDidEndand no final geometry, breaking the documented order). All other callers keep the anti-echo gate, and host-bracketed sessions get the same delivery.DEBUG logging follows the new shape: session begin/end, and each imposed apply with its target, outcome, and the container size it applied in.
Supersedes #178 — same change rebased onto current main after the imposed-extent follow-up work landed there.
Summary by CodeRabbit