Repository navigation
Publish the commits cmux main already pins (fork menu availability + divider drag sessions) - #188
Merged
Merged
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.
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.
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.
…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.
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.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughDivider drag lifecycle callbacks now coordinate imposed divider synchronization across nested splits, resize retries, and final geometry notifications. Debug logging can target an external sink while retaining file-based fallback behavior. ChangesDivider synchronization
Debug log sink
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ThemedSplitView
participant SplitViewController
participant SplitContainerView
participant BonsplitController
User->>ThemedSplitView: drag divider
ThemedSplitView->>SplitViewController: begin/end drag session
SplitViewController->>SplitContainerView: update tree drag state
SplitContainerView->>SplitContainerView: defer imposed sync and schedule retries
SplitViewController->>BonsplitController: notify drag lifecycle
BonsplitController->>BonsplitController: force final geometry notification
Possibly related PRs
Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 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 publishes the bonsplit vendor state that
Confidence Score: 4/5Safe to merge; the drag-session and imposed-extent changes are well-tested and the new public API is additive with backward-compatible default delegate implementations. The ordering of
Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant AppKit as AppKit (NSSplitView)
participant TSV as ThemedSplitView
participant Coord as Coordinator
participant SVC as SplitViewController
participant BC as BonsplitController
participant Delegate as Host Delegate
Note over AppKit,Delegate: Drag Begin
AppKit->>TSV: mouseDown(event)
TSV->>Coord: dividerDragSessionChanged(true)
TSV->>SVC: noteDividerDragSession(true)
SVC->>BC: onDividerDragSessionChange(true)
BC->>Delegate: splitTabBarDividerDragDidBegin
Note over AppKit,Delegate: During Drag
AppKit->>Coord: splitViewDidResizeSubviews
Note over Coord: imposed applies refused (isTreeDragSessionActive)
Note over AppKit,Delegate: Drag End
AppKit-->>TSV: super.mouseDown returns (defer)
TSV->>Coord: dividerDragSessionChanged(false)
TSV->>SVC: noteDividerDragSession(false)
Note over SVC: zero crossing
SVC->>BC: onDividerDragSessionChange(false)
BC->>BC: notifyGeometryChange(force: true)
BC->>Delegate: didChangeGeometry
BC->>Delegate: splitTabBarDividerDragDidEnd
Note over SVC: syncDividerNow for imposed splits (AFTER dragDidEnd)
SVC->>Coord: syncDividerNow
Coord->>AppKit: setPositionSafely
%%{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 AppKit as AppKit (NSSplitView)
participant TSV as ThemedSplitView
participant Coord as Coordinator
participant SVC as SplitViewController
participant BC as BonsplitController
participant Delegate as Host Delegate
Note over AppKit,Delegate: Drag Begin
AppKit->>TSV: mouseDown(event)
TSV->>Coord: dividerDragSessionChanged(true)
TSV->>SVC: noteDividerDragSession(true)
SVC->>BC: onDividerDragSessionChange(true)
BC->>Delegate: splitTabBarDividerDragDidBegin
Note over AppKit,Delegate: During Drag
AppKit->>Coord: splitViewDidResizeSubviews
Note over Coord: imposed applies refused (isTreeDragSessionActive)
Note over AppKit,Delegate: Drag End
AppKit-->>TSV: super.mouseDown returns (defer)
TSV->>Coord: dividerDragSessionChanged(false)
TSV->>SVC: noteDividerDragSession(false)
Note over SVC: zero crossing
SVC->>BC: onDividerDragSessionChange(false)
BC->>BC: notifyGeometryChange(force: true)
BC->>Delegate: didChangeGeometry
BC->>Delegate: splitTabBarDividerDragDidEnd
Note over SVC: syncDividerNow for imposed splits (AFTER dragDidEnd)
SVC->>Coord: syncDividerNow
Coord->>AppKit: setPositionSafely
Reviews (1): Last reviewed commit: "Merge fork menu state with divider drag ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+513
to
+518
| // Re-install alongside the identity stamps above so a reused | ||
| // coordinator keeps answering for the tree it currently renders. | ||
| let internalController = controller | ||
| context.coordinator.isTreeDragSessionActive = { [weak internalController] in | ||
| (internalController?.activeDividerDragSessions ?? 0) > 0 | ||
| } |
There was a problem hiding this comment.
isTreeDragSessionActive is re-installed in updateNSView so a reused coordinator always references the live controller, but splitView.onDividerDragSession (which calls both coordinator?.dividerDragSessionChanged and internalController?.noteDividerDragSession) is only wired in makeNSView. If SwiftUI ever re-uses the split view for a different controller instance — the same scenario the defensive comment here acknowledges as a concern — drag-session begin/end would still forward to the old internalController, while isTreeDragSessionActive now answers for the new one. The counter and the gate would be mismatched: a drag could be invisible to isTreeDragSessionActive even though the coordinator correctly sets isDragging.
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
3 participants
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.
cmux main has pinned vendor/bonsplit at 10563e2 since manaflow-ai/cmux#8140 merged, but that commit was never pushed here — so every clean cmux checkout fails to build the Mac app (
type 'Bool' has no member 'hidden'in Workspace.swift). This publishes the exact stack the pointer references: the tab context-menu fork-availability API (TabContextForkConversationAvailability) and the divider drag-session work. Fast-forward from current main, no rewrites.🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Publishes the vendor
bonsplitstate pinned bycmux mainso clean checkouts build, and adds divider drag-session support with safer imposed-extent behavior to stop divider fights and hangs.New Features
BonsplitController.isDividerDragActiveandnoteDividerDragSession(_:).splitTabBarDividerDragDidBeginandsplitTabBarDividerDragDidEnd. Final geometry is delivered before drag-end and bypasses the external-update suppression window.DebugEventLog.setExternalSink(_:)to route lines to a host writer; fallback keeps a single O_APPEND handle open.Bug Fixes
Written for commit 10563e2. Summary will update on new commits.
Summary by CodeRabbit