Handle hidden-titlebar Bonsplit chrome overlap#27
Handle hidden-titlebar Bonsplit chrome overlap#27lawrencecchen wants to merge 10 commits intomainfrom
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request introduces UI infrastructure for computing dynamic tab bar leading insets based on macOS window traffic lights, replaces legacy double-click monitoring with a new drag-region component, establishes bridging types for hosting SwiftUI content in AppKit, expands drop-zone handling with directional cases, and enhances tab bar interaction through new container views with improved accessibility and event handling. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a559813198
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| tabBarMinXInWindow: CGFloat, | ||
| trailingPadding: CGFloat = 14 | ||
| ) -> CGFloat { | ||
| max(0, trafficLightMaxX + trailingPadding - tabBarMinXInWindow) |
There was a problem hiding this comment.
tabBarLeadingTrafficLightInset computes clearance from only the X positions, so any pane whose tab bar starts near the left edge gets padded in hidden-titlebar mode even when its tab row is vertically far below the traffic lights (for example, lower panes in a vertical split). That introduces unnecessary leading whitespace and earlier tab truncation in multi-row layouts; this inset should be conditioned on actual traffic-light/tab-bar overlap, not just horizontal position.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
Sources/Bonsplit/Internal/Views/TabBarView.swift (2)
15-21: Avoid hardcoding the inset padding default.Line 18 duplicates a layout constant (
14). Using a shared metric avoids drift between inset math and tab bar padding rules.♻️ Suggested change
func tabBarLeadingTrafficLightInset( trafficLightMaxX: CGFloat, tabBarMinXInWindow: CGFloat, - trailingPadding: CGFloat = 14 + trailingPadding: CGFloat = TabBarMetrics.barPadding ) -> CGFloat { max(0, trafficLightMaxX + trailingPadding - tabBarMinXInWindow) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Bonsplit/Internal/Views/TabBarView.swift` around lines 15 - 21, The default trailingPadding in tabBarLeadingTrafficLightInset is hardcoded as 14; change it to use the shared layout metric used by the tab bar (e.g., TabBarMetrics.trafficLightInset or TabBarLayout.trailingPadding) instead of a literal so the inset math stays in sync with tab bar padding rules — update the function signature for tabBarLeadingTrafficLightInset to take trailingPadding: CGFloat = <sharedMetric> (or remove the default and reference the shared metric inside the function) and replace the literal 14 in the body with that shared constant.
100-119: Extract duplicatedonInsetChangewiring into one helper.The closure assignment is duplicated in
makeNSViewandupdateNSView; consolidating it will keep this bridge easier to maintain.♻️ Suggested cleanup
private struct TabBarLeadingInsetReader: NSViewRepresentable { `@Binding` var inset: CGFloat + + private func bindInsetHandler(to view: TabBarLeadingInsetPassthroughView) { + view.onInsetChange = { nextInset in + if abs(nextInset - inset) > 0.5 { + inset = nextInset + } + } + } func makeNSView(context: Context) -> NSView { let view = TabBarLeadingInsetPassthroughView() view.setFrameSize(.zero) - view.onInsetChange = { nextInset in - if abs(nextInset - inset) > 0.5 { - inset = nextInset - } - } + bindInsetHandler(to: view) return view } func updateNSView(_ nsView: NSView, context: Context) { guard let view = nsView as? TabBarLeadingInsetPassthroughView else { return } - view.onInsetChange = { nextInset in - if abs(nextInset - inset) > 0.5 { - inset = nextInset - } - } + bindInsetHandler(to: view) view.publishInsetIfNeeded() } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Bonsplit/Internal/Views/TabBarView.swift` around lines 100 - 119, Duplicate closure assignment for TabBarLeadingInsetPassthroughView.onInsetChange appears in makeNSView and updateNSView; extract that wiring into a single helper function (e.g., bindInsetHandler(to:)) and call it from both places. Implement a private method that accepts the view (TabBarLeadingInsetPassthroughView) and sets view.onInsetChange = { nextInset in if abs(nextInset - inset) > 0.5 { inset = nextInset } }, then replace the duplicated assignments in makeNSView and updateNSView with calls to this helper and keep the existing call to view.publishInsetIfNeeded() in updateNSView.Tests/BonsplitTests/BonsplitTests.swift (1)
7-35: Consider one test for non-defaulttrailingPadding.Since
tabBarLeadingTrafficLightInsetexposestrailingPadding, a single custom-padding assertion would lock that contract down too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/BonsplitTests/BonsplitTests.swift` around lines 7 - 35, Add a test that exercises the non-default trailingPadding parameter on tabBarLeadingTrafficLightInset: call tabBarLeadingTrafficLightInset(trafficLightMaxX: 64, tabBarMinXInWindow: 40, trailingPadding: 10) and assert the expected returned inset (34) to lock the trailingPadding contract; place this test alongside the existing tests in BonsplitTests.swift and name it something like testTabBarLeadingTrafficLightInsetRespectsCustomTrailingPadding so it’s easy to find.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/Bonsplit/Internal/Views/TabBarView.swift`:
- Around line 15-21: The default trailingPadding in
tabBarLeadingTrafficLightInset is hardcoded as 14; change it to use the shared
layout metric used by the tab bar (e.g., TabBarMetrics.trafficLightInset or
TabBarLayout.trailingPadding) instead of a literal so the inset math stays in
sync with tab bar padding rules — update the function signature for
tabBarLeadingTrafficLightInset to take trailingPadding: CGFloat = <sharedMetric>
(or remove the default and reference the shared metric inside the function) and
replace the literal 14 in the body with that shared constant.
- Around line 100-119: Duplicate closure assignment for
TabBarLeadingInsetPassthroughView.onInsetChange appears in makeNSView and
updateNSView; extract that wiring into a single helper function (e.g.,
bindInsetHandler(to:)) and call it from both places. Implement a private method
that accepts the view (TabBarLeadingInsetPassthroughView) and sets
view.onInsetChange = { nextInset in if abs(nextInset - inset) > 0.5 { inset =
nextInset } }, then replace the duplicated assignments in makeNSView and
updateNSView with calls to this helper and keep the existing call to
view.publishInsetIfNeeded() in updateNSView.
In `@Tests/BonsplitTests/BonsplitTests.swift`:
- Around line 7-35: Add a test that exercises the non-default trailingPadding
parameter on tabBarLeadingTrafficLightInset: call
tabBarLeadingTrafficLightInset(trafficLightMaxX: 64, tabBarMinXInWindow: 40,
trailingPadding: 10) and assert the expected returned inset (34) to lock the
trailingPadding contract; place this test alongside the existing tests in
BonsplitTests.swift and name it something like
testTabBarLeadingTrafficLightInsetRespectsCustomTrailingPadding so it’s easy to
find.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 57272985-4bb5-49a8-9a0c-4b84bacf5d9a
📒 Files selected for processing (3)
Sources/Bonsplit/Internal/Views/TabBarView.swiftSources/Bonsplit/Public/BonsplitController.swiftTests/BonsplitTests/BonsplitTests.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/Bonsplit/Internal/Views/TabBarView.swift">
<violation number="1" location="Sources/Bonsplit/Internal/Views/TabBarView.swift:20">
P2: Condition this inset on vertical overlap with the traffic-light region. Using only X coordinates adds unnecessary leading padding to panes that sit below the traffic lights, which reduces usable tab width and causes earlier truncation.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| tabBarMinXInWindow: CGFloat, | ||
| trailingPadding: CGFloat = 14 | ||
| ) -> CGFloat { | ||
| max(0, trafficLightMaxX + trailingPadding - tabBarMinXInWindow) |
There was a problem hiding this comment.
P2: Condition this inset on vertical overlap with the traffic-light region. Using only X coordinates adds unnecessary leading padding to panes that sit below the traffic lights, which reduces usable tab width and causes earlier truncation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Bonsplit/Internal/Views/TabBarView.swift, line 20:
<comment>Condition this inset on vertical overlap with the traffic-light region. Using only X coordinates adds unnecessary leading padding to panes that sit below the traffic lights, which reduces usable tab width and causes earlier truncation.</comment>
<file context>
@@ -12,6 +12,113 @@ private struct SelectedTabFramePreferenceKey: PreferenceKey {
+ tabBarMinXInWindow: CGFloat,
+ trailingPadding: CGFloat = 14
+) -> CGFloat {
+ max(0, trafficLightMaxX + trailingPadding - tabBarMinXInWindow)
+}
+
</file context>
There was a problem hiding this comment.
2 issues found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/Bonsplit/Internal/Views/TabBarWindowDragRegion.swift">
<violation number="1" location="Sources/Bonsplit/Internal/Views/TabBarWindowDragRegion.swift:35">
P2: `TabBarWindowDragRegion` is added but never mounted, so its drag/double-click behavior is dead code and cannot run.</violation>
</file>
<file name="Sources/Bonsplit/Internal/Views/PaneContainerView.swift">
<violation number="1" location="Sources/Bonsplit/Internal/Views/PaneContainerView.swift:15">
P1: When the view detaches from its window, you clear `monitoredWindow` before restoring `isMovable` and still install a new event monitor. This can leave the old window stuck with dragging disabled and creates a detached monitor.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| override func accessibilityRole() -> NSAccessibility.Role? { .group } | ||
|
|
||
| deinit { | ||
| removeEventMonitor() |
There was a problem hiding this comment.
P1: When the view detaches from its window, you clear monitoredWindow before restoring isMovable and still install a new event monitor. This can leave the old window stuck with dragging disabled and creates a detached monitor.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Bonsplit/Internal/Views/PaneContainerView.swift, line 15:
<comment>When the view detaches from its window, you clear `monitoredWindow` before restoring `isMovable` and still install a new event monitor. This can leave the old window stuck with dragging disabled and creates a detached monitor.</comment>
<file context>
@@ -2,6 +2,142 @@ import SwiftUI
+ override func accessibilityRole() -> NSAccessibility.Role? { .group }
+
+ deinit {
+ removeEventMonitor()
+ restoreWindowDraggingIfNeeded()
+ }
</file context>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/Bonsplit/Internal/Views/SplitNodeView.swift (1)
21-22: Stale comment referencesNSHostingController.The comment still says "Wrap in NSHostingController" but the implementation now uses
BonsplitHostingController.📝 Suggested fix
-/// Wrapper that uses NSHostingController for proper AppKit layout constraints +/// Wrapper that uses BonsplitHostingController for proper AppKit layout constraints🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Bonsplit/Internal/Views/SplitNodeView.swift` around lines 21 - 22, The comment above SinglePaneWrapper is stale—update it to reference BonsplitHostingController instead of NSHostingController; locate the comment near the SinglePaneWrapper instantiation in SplitNodeView (the lines that currently read "Wrap in NSHostingController for proper layout constraints") and change the text to accurately mention BonsplitHostingController and its purpose (e.g., "Wrap in BonsplitHostingController for proper layout constraints") so the comment matches the implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/Bonsplit/Internal/Views/SplitNodeView.swift`:
- Around line 21-22: The comment above SinglePaneWrapper is stale—update it to
reference BonsplitHostingController instead of NSHostingController; locate the
comment near the SinglePaneWrapper instantiation in SplitNodeView (the lines
that currently read "Wrap in NSHostingController for proper layout constraints")
and change the text to accurately mention BonsplitHostingController and its
purpose (e.g., "Wrap in BonsplitHostingController for proper layout
constraints") so the comment matches the implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 93561e57-e812-49a0-9948-ceebff85f248
📒 Files selected for processing (5)
Sources/Bonsplit/Internal/Views/BonsplitHostingView.swiftSources/Bonsplit/Internal/Views/PaneContainerView.swiftSources/Bonsplit/Internal/Views/SplitContainerView.swiftSources/Bonsplit/Internal/Views/SplitNodeView.swiftSources/Bonsplit/Internal/Views/TabBarWindowDragRegion.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 743de85dfb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if bounds.contains(point), !hitTestRoutesToWindowDragRegion(at: point) { | ||
| suppressWindowDraggingIfNeeded(window: window) |
There was a problem hiding this comment.
handleLocalMouseEvent now turns window.isMovable off for clicks inside the tab bar unless hitTestRoutesToWindowDragRegion finds a TabBarWindowDragRegionView, but this commit never mounts TabBarWindowDragRegion in the tab-bar view tree, so that predicate is effectively always false. In hidden-titlebar windows this removes the tab strip as a draggable surface and leaves users unable to drag the window from Bonsplit chrome. Please add the drag-region view to the hierarchy (or avoid suppressing movability when no drag region is present).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/Bonsplit/Internal/Views/TabBarView.swift">
<violation number="1" location="Sources/Bonsplit/Internal/Views/TabBarView.swift:216">
P2: `shouldShowSplitButtonsNow` no longer checks hidden-titlebar state, so split buttons can stay always visible in hidden-titlebar mode when fade mode is at its default disabled value.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/Bonsplit/Internal/Views/TabBarView.swift`:
- Around line 111-119: updateNSView is synchronously calling
TabBarLeadingInsetPassthroughView.publishInsetIfNeeded which triggers the
view.onInsetChange closure and mutates the inset Binding during the
representable update cycle; defer that work to the next runloop to avoid SwiftUI
re-entrancy warnings by wrapping the publishInsetIfNeeded call (and/or the
assignment inside the onInsetChange closure that updates inset) in a
DispatchQueue.main.async (or equivalent) so the bindable inset mutation happens
asynchronously after updateNSView returns, keeping the existing check using
abs(nextInset - inset) > 0.5 and the same view.onInsetChange closure logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a615d1b1-99fd-46ef-a789-be6268d51006
📒 Files selected for processing (1)
Sources/Bonsplit/Internal/Views/TabBarView.swift
| func updateNSView(_ nsView: NSView, context: Context) { | ||
| guard let view = nsView as? TabBarLeadingInsetPassthroughView else { return } | ||
| view.onInsetChange = { nextInset in | ||
| if abs(nextInset - inset) > 0.5 { | ||
| inset = nextInset | ||
| } | ||
| } | ||
| view.publishInsetIfNeeded() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /root && find . -name "TabBarView.swift" -type fRepository: manaflow-ai/bonsplit
Length of output: 119
🏁 Script executed:
find . -name "TabBarView.swift" -type fRepository: manaflow-ai/bonsplit
Length of output: 114
🏁 Script executed:
cat -n ./Sources/Bonsplit/Internal/Views/TabBarView.swiftRepository: manaflow-ai/bonsplit
Length of output: 50376
🏁 Script executed:
cat -n ./Sources/Bonsplit/Internal/Views/TabBarView.swift | sed -n '50,67p'Repository: manaflow-ai/bonsplit
Length of output: 942
🏁 Script executed:
cat -n ./Sources/Bonsplit/Internal/Views/TabBarView.swift | sed -n '97,120p'Repository: manaflow-ai/bonsplit
Length of output: 978
🏁 Script executed:
cat -n ./Sources/Bonsplit/Internal/Views/TabBarView.swift | sed -n '33,44p'Repository: manaflow-ai/bonsplit
Length of output: 445
🌐 Web query:
SwiftUI NSViewRepresentable binding mutation updateNSView re-entrancy
💡 Result:
In NSViewRepresentable, mutating a @Binding (or any SwiftUI state) from inside updateNSView is re-entrant: SwiftUI is already in the middle of a view update, and your mutation schedules/causes another update, which can produce the purple runtime warning (“Modifying state during view update…”) and/or an update loop / glitchy behavior. (chris.eidhof.nl)
A robust pattern is:
- Make
updateNSViewidempotent: only push SwiftUI → AppKit changes when the value is actually different (and setcontext.coordinator.parent = selfeach time). (chris.eidhof.nl) - Send AppKit → SwiftUI changes asynchronously from delegate/notifications (e.g.
DispatchQueue.main.async { binding = newValue }) to break re-entrancy. (chris.eidhof.nl) - If the AppKit control fires delegate callbacks for programmatic changes, add a guard flag (often stored in the coordinator/view model) so delegate-driven state writes don’t immediately trigger a compensating
updateNSViewwrite back (the classic “feedback loop” fix). (stackoverflow.com)
Citations:
- 1: https://chris.eidhof.nl/post/view-representable/
- 2: https://chris.eidhof.nl/post/view-representable/
- 3: https://chris.eidhof.nl/post/view-representable/
- 4: https://stackoverflow.com/questions/59981448/uiviewrepresentable-modifying-state-during-view-update-this-will-cause-undefi
Avoid synchronous Binding mutation during updateNSView.
Line 118 calls publishInsetIfNeeded(), which immediately invokes onInsetChange (line 66) and writes to the inset binding within the representable update cycle. This causes SwiftUI re-entrancy warnings and can lead to undefined update behavior.
Defer binding mutations and the inset publish call to the next runloop iteration:
Suggested fix
func updateNSView(_ nsView: NSView, context: Context) {
guard let view = nsView as? TabBarLeadingInsetPassthroughView else { return }
- view.onInsetChange = { nextInset in
- if abs(nextInset - inset) > 0.5 {
- inset = nextInset
- }
- }
- view.publishInsetIfNeeded()
+ view.onInsetChange = { nextInset in
+ guard abs(nextInset - inset) > 0.5 else { return }
+ DispatchQueue.main.async {
+ if abs(nextInset - inset) > 0.5 {
+ inset = nextInset
+ }
+ }
+ }
+ DispatchQueue.main.async {
+ view.publishInsetIfNeeded()
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Bonsplit/Internal/Views/TabBarView.swift` around lines 111 - 119,
updateNSView is synchronously calling
TabBarLeadingInsetPassthroughView.publishInsetIfNeeded which triggers the
view.onInsetChange closure and mutates the inset Binding during the
representable update cycle; defer that work to the next runloop to avoid SwiftUI
re-entrancy warnings by wrapping the publishInsetIfNeeded call (and/or the
assignment inside the onInsetChange closure that updates inset) in a
DispatchQueue.main.async (or equivalent) so the bindable inset mutation happens
asynchronously after updateNSView returns, keeping the existing check using
abs(nextInset - inset) > 0.5 and the same view.onInsetChange closure logic.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29a36a1281
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| monitoredWindow = window | ||
| installEventMonitor() | ||
| } | ||
|
|
||
| if window == nil { |
There was a problem hiding this comment.
viewDidMoveToWindow swaps monitoredWindow before calling restoreWindowDraggingIfNeeded, so when this view detaches while a tab-bar click is suppressing drag (previousWindowMovableState != nil), the restore path writes to nil/the new window and never restores the old one. In that sequence the original window can be left with isMovable = false, which breaks normal window dragging until something else resets it.
Useful? React with 👍 / 👎.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Sources/Bonsplit/Internal/Views/TabBarView.swift (1)
111-119:⚠️ Potential issue | 🟠 MajorDefer inset publication out of
updateNSView.This is still the same re-entrant state write from the earlier review:
publishInsetIfNeeded()can synchronously hitonInsetChangeand mutateinsetwhile SwiftUI is already updating the representable. Queue the publish/binding update onto the next main-runloop turn instead.Suggested fix
func updateNSView(_ nsView: NSView, context: Context) { guard let view = nsView as? TabBarLeadingInsetPassthroughView else { return } view.onInsetChange = { nextInset in - if abs(nextInset - inset) > 0.5 { - inset = nextInset - } + guard abs(nextInset - inset) > 0.5 else { return } + DispatchQueue.main.async { + if abs(nextInset - inset) > 0.5 { + inset = nextInset + } + } } - view.publishInsetIfNeeded() + DispatchQueue.main.async { + view.publishInsetIfNeeded() + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Bonsplit/Internal/Views/TabBarView.swift` around lines 111 - 119, The call to publishInsetIfNeeded() inside updateNSView can synchronously trigger onInsetChange and mutate the SwiftUI binding (inset) re-entrantly; change updateNSView (the TabBarView representable) so publishInsetIfNeeded() is deferred to the next runloop turn (e.g. DispatchQueue.main.async or equivalent) instead of calling it inline, leaving the onInsetChange closure that updates inset unchanged.
🧹 Nitpick comments (1)
Sources/Bonsplit/Internal/Views/TabBarWindowDragRegion.swift (1)
73-141: Extract the shared mouse-down path.
mouseDown(with:)andhandleLocalMouseDown(_:)now carry the same double-click/drag sequence. Pulling that into one helper will make future behavior fixes much safer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Bonsplit/Internal/Views/TabBarWindowDragRegion.swift` around lines 73 - 141, Extract the duplicated double-click/drag logic from mouseDown(with:) and handleLocalMouseDown(_:) into a single helper, e.g. handleMouseDownSequence(event: NSEvent, forWindow: NSWindow?) -> NSEvent?; move the shared behavior that checks clickCount, calls onDoubleClick?(), calls performTabBarStandardDoubleClick(window:), toggles window.isMovable around window.performDrag(with:), and returns nil when the event is consumed. Replace the duplicated blocks in mouseDown(with:) and handleLocalMouseDown(_:) to call this helper (in mouseDown, if it returns nil then return; if it returns the event and window is nil call super.mouseDown(with:)), keeping existing use of performTabBarStandardDoubleClick(window:), onDoubleClick, window.performDrag(with:), and previousMovableState logic intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/Bonsplit/Internal/Views/PaneContainerView.swift`:
- Around line 19-30: When swapping monitoredWindow in viewDidMoveToWindow(),
restore the old window's dragging state before overwriting monitoredWindow: if
window !== monitoredWindow, first call restoreWindowDraggingIfNeeded() (or a
dedicated restore-on(monitoredWindow) helper) while monitoredWindow is still the
old window, then call removeEventMonitor(), set monitoredWindow = window, and
installEventMonitor(); also keep the existing window == nil branch but ensure it
doesn't run after monitoredWindow was already cleared. This ensures
previousWindowMovableState is applied to the original window instead of nil or
the new window.
- Around line 128-150: PaneContainerView.makeNSView currently wraps `content`
directly in `BonsplitHostingView` (via
`TabBarHostingWrapper`/`BonsplitHostingView`) so `TabBarView` never receives
required environment values; update the factory so the `TabBarView` instance is
first modified with `.environment(bonsplitController)` and
`.environment(controller)` (the same modifiers used in tests) before passing it
into `BonsplitHostingView(rootView:)` (and ensure the same environment-updated
view is used in updateNSView via `context.coordinator.hostingView?.rootView`),
locating the change around `makeNSView`, `updateNSView`, `TabBarHostingWrapper`,
and where `hostingView.rootView` is assigned.
---
Duplicate comments:
In `@Sources/Bonsplit/Internal/Views/TabBarView.swift`:
- Around line 111-119: The call to publishInsetIfNeeded() inside updateNSView
can synchronously trigger onInsetChange and mutate the SwiftUI binding (inset)
re-entrantly; change updateNSView (the TabBarView representable) so
publishInsetIfNeeded() is deferred to the next runloop turn (e.g.
DispatchQueue.main.async or equivalent) instead of calling it inline, leaving
the onInsetChange closure that updates inset unchanged.
---
Nitpick comments:
In `@Sources/Bonsplit/Internal/Views/TabBarWindowDragRegion.swift`:
- Around line 73-141: Extract the duplicated double-click/drag logic from
mouseDown(with:) and handleLocalMouseDown(_:) into a single helper, e.g.
handleMouseDownSequence(event: NSEvent, forWindow: NSWindow?) -> NSEvent?; move
the shared behavior that checks clickCount, calls onDoubleClick?(), calls
performTabBarStandardDoubleClick(window:), toggles window.isMovable around
window.performDrag(with:), and returns nil when the event is consumed. Replace
the duplicated blocks in mouseDown(with:) and handleLocalMouseDown(_:) to call
this helper (in mouseDown, if it returns nil then return; if it returns the
event and window is nil call super.mouseDown(with:)), keeping existing use of
performTabBarStandardDoubleClick(window:), onDoubleClick,
window.performDrag(with:), and previousMovableState logic intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d02799ae-6371-435f-87ce-66e83a1214c2
📒 Files selected for processing (6)
Sources/Bonsplit/Internal/Views/PaneContainerView.swiftSources/Bonsplit/Internal/Views/SplitContainerView.swiftSources/Bonsplit/Internal/Views/SplitNodeView.swiftSources/Bonsplit/Internal/Views/TabBarView.swiftSources/Bonsplit/Internal/Views/TabBarWindowDragRegion.swiftTests/BonsplitTests/BonsplitTests.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- Sources/Bonsplit/Internal/Views/SplitNodeView.swift
- Sources/Bonsplit/Internal/Views/SplitContainerView.swift
| override func viewDidMoveToWindow() { | ||
| super.viewDidMoveToWindow() | ||
|
|
||
| if window !== monitoredWindow { | ||
| removeEventMonitor() | ||
| monitoredWindow = window | ||
| installEventMonitor() | ||
| } | ||
|
|
||
| if window == nil { | ||
| restoreWindowDraggingIfNeeded() | ||
| } |
There was a problem hiding this comment.
Restore the old window before swapping monitoredWindow.
If previousWindowMovableState is set and this view detaches mid-click, Line 24 overwrites monitoredWindow before any restore happens. The later restore then targets nil/the new window instead of the old one, so the original window can stay non-movable after a rehost.
Possible fix
override func viewDidMoveToWindow() {
super.viewDidMoveToWindow()
if window !== monitoredWindow {
+ let previousWindow = monitoredWindow
+ restoreWindowDraggingIfNeeded(on: previousWindow)
removeEventMonitor()
monitoredWindow = window
- installEventMonitor()
- }
-
- if window == nil {
- restoreWindowDraggingIfNeeded()
+ if window != nil {
+ installEventMonitor()
+ }
}
}
@@
- private func restoreWindowDraggingIfNeeded() {
+ private func restoreWindowDraggingIfNeeded(on window: NSWindow? = nil) {
guard let previousWindowMovableState else { return }
- monitoredWindow?.isMovable = previousWindowMovableState
+ (window ?? monitoredWindow)?.isMovable = previousWindowMovableState
self.previousWindowMovableState = nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Bonsplit/Internal/Views/PaneContainerView.swift` around lines 19 -
30, When swapping monitoredWindow in viewDidMoveToWindow(), restore the old
window's dragging state before overwriting monitoredWindow: if window !==
monitoredWindow, first call restoreWindowDraggingIfNeeded() (or a dedicated
restore-on(monitoredWindow) helper) while monitoredWindow is still the old
window, then call removeEventMonitor(), set monitoredWindow = window, and
installEventMonitor(); also keep the existing window == nil branch but ensure it
doesn't run after monitoredWindow was already cleared. This ensures
previousWindowMovableState is applied to the original window instead of nil or
the new window.
| func makeNSView(context: Context) -> NSView { | ||
| let containerView = TabBarInteractionContainerView() | ||
| containerView.setAccessibilityElement(true) | ||
| containerView.setAccessibilityIdentifier("paneTabBar") | ||
|
|
||
| let hostingView = BonsplitHostingView(rootView: content) | ||
| hostingView.translatesAutoresizingMaskIntoConstraints = false | ||
| hostingView.setAccessibilityElement(false) | ||
| containerView.addSubview(hostingView) | ||
|
|
||
| NSLayoutConstraint.activate([ | ||
| hostingView.topAnchor.constraint(equalTo: containerView.topAnchor), | ||
| hostingView.leadingAnchor.constraint(equalTo: containerView.leadingAnchor), | ||
| hostingView.trailingAnchor.constraint(equalTo: containerView.trailingAnchor), | ||
| hostingView.bottomAnchor.constraint(equalTo: containerView.bottomAnchor), | ||
| ]) | ||
|
|
||
| context.coordinator.hostingView = hostingView | ||
| return containerView | ||
| } | ||
|
|
||
| func updateNSView(_ nsView: NSView, context: Context) { | ||
| context.coordinator.hostingView?.rootView = content |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find BonsplitHostingView definition
fd -t f "\.swift$" | xargs rg -l "BonsplitHostingView" | head -20Repository: manaflow-ai/bonsplit
Length of output: 177
🏁 Script executed:
# Search for BonsplitHostingView implementation
rg -A 20 "class BonsplitHostingView|struct BonsplitHostingView" --type swiftRepository: manaflow-ai/bonsplit
Length of output: 2319
🏁 Script executed:
# Check TabBarView's environment requirements
rg "@Environment.*BonsplitController|@Environment.*SplitViewController" --type swift -B 2 -A 2Repository: manaflow-ai/bonsplit
Length of output: 2716
🏁 Script executed:
# Check how environments are provided at the view hierarchy level
rg "\.environment.*BonsplitController|\.environment.*SplitViewController" --type swift -B 2 -A 2Repository: manaflow-ai/bonsplit
Length of output: 46
🏁 Script executed:
# Get full PaneContainerView struct definition
cat -n Sources/Bonsplit/Internal/Views/PaneContainerView.swift | head -100Repository: manaflow-ai/bonsplit
Length of output: 4054
🏁 Script executed:
# Search for PaneContainerView instantiation
rg "PaneContainerView" --type swift -B 3 -A 3 | head -100Repository: manaflow-ai/bonsplit
Length of output: 3058
🏁 Script executed:
# Check where environments are set on views
rg "\.environment\(" --type swift | head -50Repository: manaflow-ai/bonsplit
Length of output: 894
🏁 Script executed:
# Get full PaneContainerView.swift file
wc -l Sources/Bonsplit/Internal/Views/PaneContainerView.swiftRepository: manaflow-ai/bonsplit
Length of output: 123
🏁 Script executed:
# Read the complete PaneContainerView.swift
cat -n Sources/Bonsplit/Internal/Views/PaneContainerView.swiftRepository: manaflow-ai/bonsplit
Length of output: 31387
🏁 Script executed:
# Check the test setup at lines 603-605 and surrounding context
sed -n '600,610p' Tests/BonsplitTests/BonsplitTests.swiftRepository: manaflow-ai/bonsplit
Length of output: 406
🏁 Script executed:
# Search for where BonsplitHostingView is used with environment modifiers
rg "BonsplitHostingView" --type swift -B 5 -A 10Repository: manaflow-ai/bonsplit
Length of output: 6504
🏁 Script executed:
# Check if there's any environment setup in NSViewRepresentable context handling
rg "Context.*environment|\.environment" Sources/Bonsplit/Internal/Views/PaneContainerView.swiftRepository: manaflow-ai/bonsplit
Length of output: 116
🏁 Script executed:
# Check BonsplitHostingController to see if it handles environments
fd -t f "BonsplitHostingController" --type swift | xargs cat 2>/dev/null || rg "BonsplitHostingController" --type swift -A 20Repository: manaflow-ai/bonsplit
Length of output: 236
🏁 Script executed:
# Get more context around the test setup
sed -n '590,620p' Tests/BonsplitTests/BonsplitTests.swiftRepository: manaflow-ai/bonsplit
Length of output: 1447
🏁 Script executed:
# Search for all usages of environment in BonsplitTests
rg "\.environment\(" Tests/BonsplitTests/BonsplitTests.swift -B 2 -A 2 | head -80Repository: manaflow-ai/bonsplit
Length of output: 914
🏁 Script executed:
# Check if TabBarView is ever wrapped with .environment before passing to TabBarHostingWrapper
rg "TabBarView" --type swift -B 3 -A 3Repository: manaflow-ai/bonsplit
Length of output: 3038
🏁 Script executed:
# Look for NSHostingView or environment propagation patterns in the codebase
rg "NSHostingView|NSViewRepresentable" --type swift -B 2 -A 8 | head -100Repository: manaflow-ai/bonsplit
Length of output: 7121
Apply environments to TabBarView before wrapping in NSHostingView.
TabBarView requires @Environment(BonsplitController.self) and @Environment(SplitViewController.self), but TabBarHostingWrapper.makeNSView() passes content directly to BonsplitHostingView(rootView:) without re-injecting them. The test manually applies both environments when instantiating TabBarView directly (test pattern: .environment(controller).environment(controller.internalController)), confirming these are mandatory for TabBarView to function. The production code must apply the same modifiers before passing to TabBarHostingWrapper:
Fix needed in PaneContainerView.swift, around line 326-331
TabBarHostingWrapper(
content: TabBarView(
pane: pane,
isFocused: isFocused,
showSplitButtons: showSplitButtons
)
.environment(bonsplitController)
.environment(controller)
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Bonsplit/Internal/Views/PaneContainerView.swift` around lines 128 -
150, PaneContainerView.makeNSView currently wraps `content` directly in
`BonsplitHostingView` (via `TabBarHostingWrapper`/`BonsplitHostingView`) so
`TabBarView` never receives required environment values; update the factory so
the `TabBarView` instance is first modified with
`.environment(bonsplitController)` and `.environment(controller)` (the same
modifiers used in tests) before passing it into `BonsplitHostingView(rootView:)`
(and ensure the same environment-updated view is used in updateNSView via
`context.coordinator.hostingView?.rootView`), locating the change around
`makeNSView`, `updateNSView`, `TabBarHostingWrapper`, and where
`hostingView.rootView` is assigned.
1 participant
Summary
Testing
swift test --filter BonsplitTests/testTabBarLeadingTrafficLightInsetKeepsOnlyRemainingOverlapClearanceSummary by cubic
Fixes tab bar overlap and restores reliable window drag with hidden title bars on macOS, while honoring macOS titlebar double‑click settings (preserved in minimal mode). Adds a live traffic‑light inset (minimal mode only), hover‑revealed tab actions in minimal mode, and exposes
onTabCloseRequestfor user‑initiated closes.Bug Fixes
New Features
onTabCloseRequest: (TabID, PaneID) -> Voidfor explicit close clicks; optional and non‑breaking.Written for commit 31c3810. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements
Tests