Repository navigation
Unify tab bar button lane layout - #113
Conversation
…e-preview-file-drops
…e-preview-file-drops
…e-preview-file-drops
…e-preview-file-drops # Conflicts: # Sources/Bonsplit/Public/BonsplitController.swift
…e-preview-file-drops
|
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:
📝 WalkthroughWalkthroughIntroduces TabBarLayout and TabBarChromeSnapshot, rewrites split-button UI as a measured, scrollable overlay with new compositing/masking pipeline, extends split-button styling API, adds external file-drop handling, and adds extensive layout, rendering, and drop tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TabBarView
participant TabBarLayout
participant SplitScrollView
participant ChromeSnapshot
participant Renderer
User->>TabBarView: toggle split buttons / interact / scroll
TabBarView->>TabBarLayout: measure available width -> compute lane min/full/max & trailing inset
TabBarView->>SplitScrollView: mount split-button views with coordinateSpace
SplitScrollView->>TabBarView: report contentWidth, viewportWidth, scrollOffset
TabBarView->>ChromeSnapshot: request chrome params (masks, occlusion, indicator/separator frames)
ChromeSnapshot->>TabBarView: return chrome/backdrop/mask parameters
TabBarView->>Renderer: composite tabBarSurface, apply combinedMask, render splitButtonChrome overlay + fade mask
Renderer->>User: display updated tab bar
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 centralizes tab bar row geometry into a new Confidence Score: 4/5Safe to merge — clean refactor with a regression test; no logic errors found. All changes are P2 or better. The geometry centralization is correct, the clamping fix for the active indicator and separator gap is well-reasoned, and the pixel-level regression test matches the existing test infrastructure pattern. No files require special attention. Important Files Changed
Reviews (1): Last reviewed commit: "Unify tab bar button lane layout" | Re-trigger Greptile |
c369475 to
77a6f8c
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
6b78093 to
e625083
Compare
![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 the current code and only fix it if needed.
Inline comments:
In `@Tests/BonsplitTests/BonsplitTests.swift`:
- Around line 2433-2438: The saturation(of:) helper ignores alpha and returns
full saturation when RGB is unchanged despite fading; update saturation(of
color: NSColor) to account for alpha by using premultiplied RGB: let a =
color.alphaComponent; if a <= 0.01 { return 0 }; let r = color.redComponent * a,
g = color.greenComponent * a, b = color.blueComponent * a, then compute high =
max(r,g,b), low = min(r,g,b) and return (high - low) / high; keep the existing
guard threshold but base it on premultiplied values so
testSelectedTabIndicatorFadesIntoSplitButtonLane sees faded (low-alpha) pixels
as low saturation.
🪄 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: 73b52f51-f8cf-4c47-8f3a-8e3077392027
📒 Files selected for processing (2)
Sources/Bonsplit/Internal/Views/TabBarView.swiftTests/BonsplitTests/BonsplitTests.swift
![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: e625083879
ℹ️ 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".
| @@ -397,7 +501,7 @@ struct TabBarView: View { | |||
| } | |||
|
|
|||
| private var splitButtonsBackdropWidth: CGFloat { | |||
There was a problem hiding this comment.
When fullSplitButtonLaneWidth grows beyond the default reserved width (for example, custom split buttons with wider emoji/text icons), the lane now reserves and masks that larger width, but tabBarSurface still paints the backdrop using only fadeWidth + solidWidth. This creates a visible strip where buttons sit over the base tab-bar color instead of the split-button backdrop, which can reduce contrast and break the intended lane styling for wide button configurations. Consider deriving backdrop paint width from the same expanded lane width.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
|
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7aef42cd10
ℹ️ 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".
| .background(tabBarSurface) | ||
| .overlay(maskedTabBarChrome) | ||
| .background(TabBarDragAndHoverView( |
There was a problem hiding this comment.
Render order changed so maskedTabBarChrome is now applied as an outer .overlay, which places the selected-tab indicator and bottom separator above the split-button controls. When splitButtonBackdropEffect.masksTabContent is false (including debug styles and any custom appearance that disables masking), combinedMask does not clear the button lane, so the indicator/separator can visibly draw over button icons. Previously this chrome lived in the background layer and could not cover controls, so this is a regression for non-masking configurations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb53b34870
ℹ️ 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".
| activeDropZone = zone | ||
| #if DEBUG | ||
| dlog("pane.dropUpdated pane=\(pane.id.id.uuidString.prefix(5)) zone=\(zone)") | ||
| #endif | ||
| return DropProposal(operation: .move) | ||
| return DropProposal(operation: dropOperation(for: info)) |
There was a problem hiding this comment.
For hosts that only set the legacy onFileDrop callback, validateDrop accepts files only in the center zone, but dropUpdated still switches activeDropZone to edge split zones and keeps returning a .copy proposal. If the drag enters through center and then moves to an edge, the UI advertises a valid edge drop even though performFileDrop later rejects it (destination is .split and legacy handling only accepts .insert), so drops can fail unexpectedly at release time. Rechecking acceptsFileDrop in dropUpdated (or constraining file zones there) would keep feedback and actual drop handling consistent.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9d082c0 by making dropUpdated use the same file-drop zone acceptance as validateDrop.
— Claude Code
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
Tests/BonsplitTests/BonsplitTests.swift (2)
2969-2971:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPremultiply
brightness(of:)by alpha.A transparent or barely visible white pixel still returns
1.0here, so the new backdrop-coverage checks at Line 1696 and Line 1706 can pass even when the lane surface faded out instead of painting solidly. This is the same false-positive path previously flagged in this file.Possible fix
private func brightness(of color: NSColor) -> CGFloat { - max(color.redComponent, color.greenComponent, color.blueComponent) + guard let rgb = color.usingColorSpace(.sRGB) else { return 0 } + let alpha = max(0, min(1, rgb.alphaComponent)) + guard alpha > 0.01 else { return 0 } + return max( + rgb.redComponent * alpha, + rgb.greenComponent * alpha, + rgb.blueComponent * alpha + ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/BonsplitTests/BonsplitTests.swift` around lines 2969 - 2971, The brightness(of:) helper currently returns the max of red/green/blue and ignores alpha, causing fully transparent but bright pixels to report brightness 1.0; update brightness(of:) to premultiply the color channels by color.alphaComponent (i.e., compute max(red*alpha, green*alpha, blue*alpha)) so transparent pixels contribute appropriately when used by the backdrop-coverage checks referenced at lines ~1696/1706.
2418-2452:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the saturation sampler alpha-aware before relying on these new render assertions.
These helpers still read saturation through
maximumSaturation(in:sampleRect:), which computes chroma from raw RGB after only a coarse alpha cutoff. If the selected indicator fades mostly through alpha, the pixels in the split-button lane can still report high saturation and make these regressions fail or pass for the wrong reason. This is the same issue previously called out in this file.Possible fix in the shared sampler
private func maximumSaturation(in view: NSView, sampleRect: NSRect? = nil) -> CGFloat? { @@ for y in minY..<maxY { for x in minX..<maxX { guard let color = bitmap.colorAt(x: x, y: y), let rgb = color.usingColorSpace(.sRGB), rgb.alphaComponent > 0.05 else { continue } - let red = rgb.redComponent - let green = rgb.greenComponent - let blue = rgb.blueComponent + let alpha = max(0, min(1, rgb.alphaComponent)) + let red = rgb.redComponent * alpha + let green = rgb.greenComponent * alpha + let blue = rgb.blueComponent * alpha let high = max(red, green, blue) guard high > 0.01 else { continue } let low = min(red, green, blue) let saturation = (high - low) / high maximum = max(maximum, saturation)Also applies to: 2578-2618
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/BonsplitTests/BonsplitTests.swift` around lines 2418 - 2452, The saturation sampler used by renderedSplitButtonLaneTopSaturation relies on maximumSaturation(in:sampleRect:) which currently computes chroma from raw RGB with only a coarse alpha cutoff; update the sampler to be alpha-aware by ignoring low-alpha pixels or weighting color by alpha before computing saturation (e.g., skip pixels with alpha < 0.5 or compute saturation from premultiplied RGB = (r*α, g*α, b*α) and only consider pixels with α above a threshold), and apply the same change to the other helper usage at the block around lines 2578-2618 so the rendered tests don’t assert on colors that are only visible via transparency. Ensure you modify the implementation of maximumSaturation(in:sampleRect:) (or the shared sampler it calls) rather than each test helper so all callers benefit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Tests/BonsplitTests/BonsplitTests.swift`:
- Around line 2969-2971: The brightness(of:) helper currently returns the max of
red/green/blue and ignores alpha, causing fully transparent but bright pixels to
report brightness 1.0; update brightness(of:) to premultiply the color channels
by color.alphaComponent (i.e., compute max(red*alpha, green*alpha, blue*alpha))
so transparent pixels contribute appropriately when used by the
backdrop-coverage checks referenced at lines ~1696/1706.
- Around line 2418-2452: The saturation sampler used by
renderedSplitButtonLaneTopSaturation relies on maximumSaturation(in:sampleRect:)
which currently computes chroma from raw RGB with only a coarse alpha cutoff;
update the sampler to be alpha-aware by ignoring low-alpha pixels or weighting
color by alpha before computing saturation (e.g., skip pixels with alpha < 0.5
or compute saturation from premultiplied RGB = (r*α, g*α, b*α) and only consider
pixels with α above a threshold), and apply the same change to the other helper
usage at the block around lines 2578-2618 so the rendered tests don’t assert on
colors that are only visible via transparency. Ensure you modify the
implementation of maximumSaturation(in:sampleRect:) (or the shared sampler it
calls) rather than each test helper so all callers benefit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: db401c73-36ed-4340-befa-54531f72da9c
📒 Files selected for processing (2)
Sources/Bonsplit/Internal/Views/TabBarView.swiftTests/BonsplitTests/BonsplitTests.swift
✅ Files skipped from review due to trivial changes (1)
- Sources/Bonsplit/Internal/Views/TabBarView.swift
|
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 819688a433
ℹ️ 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".
| private func fileURLs(from info: DropInfo) -> [URL] { | ||
| guard info.hasItemsConforming(to: [.fileURL]) else { return [] } | ||
|
|
||
| // DropInfo exposes file URLs through asynchronous item providers, but | ||
| // DropDelegate.performDrop needs a synchronous result. AppKit publishes | ||
| // Finder/local file drags on the drag pasteboard for this callback. | ||
| return Self.fileURLs(from: NSPasteboard(name: .drag)) |
There was a problem hiding this comment.
performFileDrop calls fileURLs(from:), but that helper never reads DropInfo’s item providers and instead only inspects NSPasteboard(name: .drag). In drag sources that expose public.file-url asynchronously (or via file promises) without immediately materializing NSURL objects on the drag pasteboard, validateDrop can accept the drop while performDrop always sees an empty URL list and rejects it. This creates user-visible false negatives for external file drops even when handlers are configured.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing this in this PR because DropDelegate.performDrop and Bonsplit's file-drop handler are synchronous. Provider-only file promises need a separate async drop API instead of blocking the main thread; the supported synchronous path remains the drag pasteboard.
— Claude Code
There was a problem hiding this comment.
Handled in 90f4981. The synchronous DropDelegate path now only validates file drops when the drag pasteboard has readable file URLs, matching what performDrop can actually consume. Provider-only async file promises need a separate async drop API instead of accepting a drop that this delegate cannot fulfill synchronously.
|
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d082c0830
ℹ️ 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".
| guard let draggedTab = controller.activeDragTab ?? controller.draggingTab, | ||
| let sourcePaneId = controller.activeDragSourcePaneId ?? controller.dragSourcePaneId else { |
There was a problem hiding this comment.
Now that this drop target accepts .fileURL, performDrop still prioritizes the in-memory tab-drag state (activeDragTab/draggingTab) before checking the dropped item types, so any lingering local drag state causes a file drop to be handled as a tab move/split instead of a file drop. This is a behavioral regression introduced by adding file support: a stale tab-drag state can make Finder file drops unexpectedly rearrange tabs rather than invoking file handlers.
Useful? React with 👍 / 👎.
1 participant
Summary
Testing
Summary by cubic
Unifies the tab bar split-button lane layout and chrome masking, caps lane width, and makes fades/backdrops full-height to stop visual bleed. Adds external file drops routed through pane zones via
BonsplitController.onExternalFileDrop.Bug Fixes
Refactors
TabBarLayoutfor bar height, lane widths (min/measured, capped at 25%), insets, masks, and separator frames.TabBarViewand split‑action styling to share fade masks, snapshot action chrome, and use full‑height rectangular hits; added fade/separator tuning inBonsplitConfiguration.Written for commit 90f4981. Summary will update on new commits.
Summary by CodeRabbit
Refactor
Bug Fixes
New Features
Tests