Repository navigation
Fix translucent split wrapper layers - #29
Merged
Merged
Conversation
Allows the host application to distinguish user-initiated tab closes (clicking the X button) from internal/programmatic closes, so it can decide whether closing the last surface should also close the workspace. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![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.
📝 WalkthroughWalkthroughThis PR adds a public optional hook Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant TabBar as TabBarView
participant Controller as BonsplitController
User->>TabBar: Clicks tab close button
TabBar->>TabBar: Begin close transaction
TabBar->>Controller: onTabCloseRequest?(TabID, PaneID)
Note right of Controller: Optional host callback invoked
TabBar->>Controller: closeTab(...) %% proceed to close tab
TabBar->>TabBar: Commit transaction / update UI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?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
🧹 Nitpick comments (4)
Sources/Bonsplit/Internal/Views/SplitNodeView.swift (1)
75-78: Optional: extract repeated layer configuration into a helper.The same layer setup appears in two places; a small helper would reduce drift risk in future tweaks.
♻️ Suggested refactor
+ private func configureTransparentLayer(for view: NSView) { + view.wantsLayer = true + view.layer?.backgroundColor = NSColor.clear.cgColor + view.layer?.isOpaque = false + view.layer?.masksToBounds = true + } + func makeNSView(context: Context) -> NSView { @@ - containerView.wantsLayer = true - containerView.layer?.backgroundColor = NSColor.clear.cgColor - containerView.layer?.isOpaque = false - containerView.layer?.masksToBounds = true + configureTransparentLayer(for: containerView) @@ func updateNSView(_ nsView: NSView, context: Context) { @@ - nsView.wantsLayer = true - nsView.layer?.backgroundColor = NSColor.clear.cgColor - nsView.layer?.isOpaque = false - nsView.layer?.masksToBounds = true + configureTransparentLayer(for: nsView)Also applies to: 98-101
🤖 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 75 - 78, The repeated container layer setup (containerView.wantsLayer = true; containerView.layer?.backgroundColor = NSColor.clear.cgColor; containerView.layer?.isOpaque = false; containerView.layer?.masksToBounds = true) in SplitNodeView should be extracted into a single private helper (e.g. configureContainerLayer(_:)/setupContainerLayer()) and called from both locations (the two blocks around containerView). Create the helper as a private method on SplitNodeView that takes the view (or uses the instance property), moves the four layer lines into it, and replace both duplicated blocks with calls to that helper to ensure consistent configuration and easier future changes.Sources/Bonsplit/Internal/Views/SplitContainerView.swift (2)
327-332: Redundant layer configuration on existing containers.These containers (
arranged[0]andarranged[1]) are the sameNSViewinstances created inmakeNSView. Their layer properties are already configured and don't need to be re-set on every update cycle unless you expect external modification.This is low priority since the overhead is minimal and the pattern matches the split view handling above.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Bonsplit/Internal/Views/SplitContainerView.swift` around lines 327 - 332, The repeated layer setup for firstContainer and secondContainer in the update path is redundant because these are the same NSView instances created/configured in makeNSView (arranged[0]/arranged[1]); remove the duplicate calls (firstContainer.wantsLayer, layer?.backgroundColor, layer?.isOpaque and corresponding secondContainer lines) from the update routine and ensure the initial configuration remains only in makeNSView (or alternatively guard with an if layer == nil check) so the views’ layer properties aren’t reset on every update.
308-310: Consider consolidating layer configuration.These layer properties are already set in
makeNSView(lines 115-117). Re-applying them on everyupdateNSViewcall is defensive but adds redundant work. If the layer configuration is expected to be stable, you could remove this duplication.That said, if there's a concern about layer state being externally modified, keeping it is a valid defensive choice.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Bonsplit/Internal/Views/SplitContainerView.swift` around lines 308 - 310, The split view's layer properties are being set twice; remove the redundant reconfiguration from updateNSView by deleting the splitView.wantsLayer / splitView.layer?.backgroundColor / splitView.layer?.isOpaque lines so the layer is only configured in makeNSView (where it's already set), or if you intentionally want defensive reapplication, keep them but add a short comment in updateNSView explaining why the repetition is necessary; refer to SplitContainerView, makeNSView, updateNSView and the splitView variable to locate the changes.Tests/BonsplitTests/BonsplitTests.swift (1)
917-920: Use bounded polling instead of fixed sleep to reduce UI test flakiness.The fixed 50ms delay can be timing-sensitive on slower CI machines. Polling until a renderable pixel is available (with timeout) is more reliable.
Proposed refactor
- window.makeKeyAndOrderFront(nil) - contentView.layoutSubtreeIfNeeded() - RunLoop.current.run(until: Date().addingTimeInterval(0.05)) - contentView.layoutSubtreeIfNeeded() + window.makeKeyAndOrderFront(nil) + let deadline = Date().addingTimeInterval(0.5) + while Date() < deadline { + contentView.layoutSubtreeIfNeeded() + if renderedColor(in: hostingView, at: samplePoint) != nil { + break + } + RunLoop.current.run(mode: .default, before: Date().addingTimeInterval(0.01)) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/BonsplitTests/BonsplitTests.swift` around lines 917 - 920, Replace the fixed 50ms sleep (RunLoop.current.run(until: Date().addingTimeInterval(0.05))) with bounded polling: after calling window.makeKeyAndOrderFront(nil) and contentView.layoutSubtreeIfNeeded(), loop running the runloop in short increments and repeatedly check a render-ready condition on contentView (for example: a non-nil layer/presentation, a valid bitmapImageRepForCachingDisplay(_:), or any pixel-ready probe) until the condition is true or a timeout elapses; ensure the loop breaks on success and fails the test on timeout so window.makeKeyAndOrderFront, contentView.layoutSubtreeIfNeeded and the previous RunLoop usage are the only touched symbols.
🤖 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/SplitViewContainer.swift`:
- Line 20: Replace the unconditional
.background(TabBarColors.paneBackground(for: appearance)) so the split wrapper
remains clear for translucent chrome: introduce/use a splitWrapperBackground
policy that inspects the chrome/pane color returned by
TabBarColors.paneBackground(for: appearance) (or appearance.chrome background)
and returns Color.clear unless that color's alpha == 1.0, and then call
.background(splitWrapperBackground(for: appearance or chromeColor)) at the
SplitViewContainer call site; keep SplitContainerView and SplitNodeView behavior
unchanged and ensure the new helper only uses opaque colors when alpha is
exactly 1.0.
---
Nitpick comments:
In `@Sources/Bonsplit/Internal/Views/SplitContainerView.swift`:
- Around line 327-332: The repeated layer setup for firstContainer and
secondContainer in the update path is redundant because these are the same
NSView instances created/configured in makeNSView (arranged[0]/arranged[1]);
remove the duplicate calls (firstContainer.wantsLayer, layer?.backgroundColor,
layer?.isOpaque and corresponding secondContainer lines) from the update routine
and ensure the initial configuration remains only in makeNSView (or
alternatively guard with an if layer == nil check) so the views’ layer
properties aren’t reset on every update.
- Around line 308-310: The split view's layer properties are being set twice;
remove the redundant reconfiguration from updateNSView by deleting the
splitView.wantsLayer / splitView.layer?.backgroundColor /
splitView.layer?.isOpaque lines so the layer is only configured in makeNSView
(where it's already set), or if you intentionally want defensive reapplication,
keep them but add a short comment in updateNSView explaining why the repetition
is necessary; refer to SplitContainerView, makeNSView, updateNSView and the
splitView variable to locate the changes.
In `@Sources/Bonsplit/Internal/Views/SplitNodeView.swift`:
- Around line 75-78: The repeated container layer setup
(containerView.wantsLayer = true; containerView.layer?.backgroundColor =
NSColor.clear.cgColor; containerView.layer?.isOpaque = false;
containerView.layer?.masksToBounds = true) in SplitNodeView should be extracted
into a single private helper (e.g.
configureContainerLayer(_:)/setupContainerLayer()) and called from both
locations (the two blocks around containerView). Create the helper as a private
method on SplitNodeView that takes the view (or uses the instance property),
moves the four layer lines into it, and replace both duplicated blocks with
calls to that helper to ensure consistent configuration and easier future
changes.
In `@Tests/BonsplitTests/BonsplitTests.swift`:
- Around line 917-920: Replace the fixed 50ms sleep (RunLoop.current.run(until:
Date().addingTimeInterval(0.05))) with bounded polling: after calling
window.makeKeyAndOrderFront(nil) and contentView.layoutSubtreeIfNeeded(), loop
running the runloop in short increments and repeatedly check a render-ready
condition on contentView (for example: a non-nil layer/presentation, a valid
bitmapImageRepForCachingDisplay(_:), or any pixel-ready probe) until the
condition is true or a timeout elapses; ensure the loop breaks on success and
fails the test on timeout so window.makeKeyAndOrderFront,
contentView.layoutSubtreeIfNeeded and the previous RunLoop usage are the only
touched symbols.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5f871c2f-3b0a-46ca-b9c8-71fe637c4f0d
📒 Files selected for processing (5)
Sources/Bonsplit/Internal/Views/PaneContainerView.swiftSources/Bonsplit/Internal/Views/SplitContainerView.swiftSources/Bonsplit/Internal/Views/SplitNodeView.swiftSources/Bonsplit/Internal/Views/SplitViewContainer.swiftTests/BonsplitTests/BonsplitTests.swift
💤 Files with no reviewable changes (1)
- Sources/Bonsplit/Internal/Views/PaneContainerView.swift
3 tasks done
This was referenced Apr 28, 2026
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
2 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.
Summary
Testing
Issues
Summary by cubic
Fix translucent themes turning opaque after splitting by keeping split root and wrapper layers clear and moving background painting to the root. Adds a callback for user‑initiated tab closes.
Bug Fixes
SplitViewContainerto avoid alpha stacking and preserve translucency.New Features
onTabCloseRequesttoBonsplitControllerto signal explicit tab-close gestures.TabBarViewcalls this hook before closing, so hosts can handle last-surface/workspace logic.Written for commit 02fa188. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes / Visual
Tests