Skip to content

Fix split action lane clipping before empty tab space #123

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
austinywang merged 3 commits into from
May 19, 2026
Merged

Fix split action lane clipping before empty tab space #123

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
207186b
test: cover action lane using trailing whitespace
austinywang May 13, 2026
ff55eda
fix: let action lane use empty tab bar space
austinywang May 13, 2026
e1d9954
test: keep overflow fixture crowded
austinywang May 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 23 additions & 5 deletions Sources/Bonsplit/Internal/Views/TabBarView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,7 @@ enum TabBarStyling {
struct TabBarLayout: Equatable {
let barHeight: CGFloat
let availableWidth: CGFloat
let tabContentWidthExcludingSplitButtonLane: CGFloat?
let splitButtonCount: Int
let splitButtonLaneVisible: Bool
let reservesSplitButtonLane: Bool
Expand All @@ -337,13 +338,15 @@ struct TabBarLayout: Equatable {
init(
tabBarHeight: CGFloat,
availableWidth: CGFloat = 0,
tabContentWidthExcludingSplitButtonLane: CGFloat? = nil,
splitButtonCount: Int,
splitButtonLaneVisible: Bool,
reservesSplitButtonLane: Bool,
measuredSplitButtonLaneWidth: CGFloat = 0
) {
self.barHeight = max(1, tabBarHeight)
self.availableWidth = max(0, availableWidth)
self.tabContentWidthExcludingSplitButtonLane = tabContentWidthExcludingSplitButtonLane.map { max(0, $0) }
self.splitButtonCount = max(0, splitButtonCount)
self.splitButtonLaneVisible = splitButtonLaneVisible
self.reservesSplitButtonLane = reservesSplitButtonLane
Expand All @@ -362,7 +365,16 @@ struct TabBarLayout: Equatable {

var maximumSplitButtonLaneWidth: CGFloat {
guard availableWidth > 0 else { return fullSplitButtonLaneWidth }
return availableWidth * TabBarStyling.maximumSplitButtonLaneWidthFraction
let fractionLimit = availableWidth * TabBarStyling.maximumSplitButtonLaneWidthFraction
return max(fractionLimit, trailingWhitespaceBeforeSplitButtonLane)
}

var trailingWhitespaceBeforeSplitButtonLane: CGFloat {
guard availableWidth > 0,
let tabContentWidthExcludingSplitButtonLane else {
return 0
}
return max(0, availableWidth - tabContentWidthExcludingSplitButtonLane)
}

var visibleSplitButtonLaneWidth: CGFloat {
Expand Down Expand Up @@ -761,6 +773,7 @@ struct TabBarView: View {
@State private var dropLifecycle: TabDropLifecycle = .idle
@State private var scrollOffset: CGFloat = 0
@State private var contentWidth: CGFloat = 0
@State private var tabContentWidthExcludingSplitButtonLane: CGFloat?
@State private var containerWidth: CGFloat = 0
@State private var selectedTabFrameInBar: CGRect?
@State private var tabFramesInBar: [UUID: CGRect] = [:]
Expand Down Expand Up @@ -803,6 +816,7 @@ struct TabBarView: View {
TabBarLayout(
tabBarHeight: appearance.tabBarHeight,
availableWidth: containerWidth,
tabContentWidthExcludingSplitButtonLane: tabContentWidthExcludingSplitButtonLane,
splitButtonCount: visibleSplitButtons.count,
splitButtonLaneVisible: shouldShowSplitButtons,
reservesSplitButtonLane: showSplitButtons && !isMinimalMode,
Expand Down Expand Up @@ -944,13 +958,11 @@ struct TabBarView: View {
GeometryReader { contentGeo in
Color.clear
.onChange(of: contentGeo.frame(in: .named("tabScroll"))) { _, newFrame in
scrollOffset = -newFrame.minX
contentWidth = newFrame.width
updateTabScrollContent(frame: newFrame)
}
.onAppear {
let frame = contentGeo.frame(in: .named("tabScroll"))
scrollOffset = -frame.minX
contentWidth = frame.width
updateTabScrollContent(frame: frame)
}
}
)
Expand Down Expand Up @@ -1536,6 +1548,12 @@ struct TabBarView: View {
splitButtonContentWidth = frame.width
}

private func updateTabScrollContent(frame: CGRect) {
scrollOffset = -frame.minX
contentWidth = frame.width
tabContentWidthExcludingSplitButtonLane = max(0, frame.width - tabBarLayout.trailingTabContentInset)
}
Comment on lines +1551 to +1555

@greptile-apps greptile-apps Bot May 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 The self-referential use of tabBarLayout.trailingTabContentInset here is not immediately obvious: tabBarLayout is recomputed from the current (pre-update) tabContentWidthExcludingSplitButtonLane, which matches the inset already baked into frame.width at render time, so the subtraction always yields the bare tab content width and the loop converges. A short comment prevents future maintainers from misreading this as accidental circular state.

Suggested change
private func updateTabScrollContent(frame: CGRect) {
scrollOffset = -frame.minX
contentWidth = frame.width
tabContentWidthExcludingSplitButtonLane = max(0, frame.width - tabBarLayout.trailingTabContentInset)
}
private func updateTabScrollContent(frame: CGRect) {
scrollOffset = -frame.minX
contentWidth = frame.width
// `tabBarLayout.trailingTabContentInset` is computed from the *current* (pre-update)
// `tabContentWidthExcludingSplitButtonLane`, which equals the trailing padding that
// was already baked into `frame.width`. Subtracting it therefore yields the bare tab
// content width. The layout loop converges within two render passes.
tabContentWidthExcludingSplitButtonLane = max(0, frame.width - tabBarLayout.trailingTabContentInset)
}


@ViewBuilder
private var splitButtonRow: some View {
let tooltips = controller.configuration.appearance.splitButtonTooltips
Expand Down
18 changes: 18 additions & 0 deletions Tests/BonsplitTests/BonsplitTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,6 +422,24 @@ final class BonsplitTests: XCTestCase {
XCTAssertTrue(layout.splitButtonLaneOverflowsViewport)
}

func testTabBarLayoutUsesTrailingWhitespaceBeforeClippingSplitButtons() {
let measuredWidth = TabBarStyling.splitButtonsBackdropWidth(buttonCount: 10)
let layout = TabBarLayout(
tabBarHeight: 28,
availableWidth: 930,
tabContentWidthExcludingSplitButtonLane: 300,
splitButtonCount: 10,
splitButtonLaneVisible: true,
reservesSplitButtonLane: true,
measuredSplitButtonLaneWidth: measuredWidth
)

XCTAssertEqual(layout.maximumSplitButtonLaneWidth, 630)
Comment on lines +425 to +437

@greptile-apps greptile-apps Bot May 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 The assertion maximumSplitButtonLaneWidth == 630 is only meaningful when fractionLimit < trailingWhitespace. With availableWidth = 930 and maximumSplitButtonLaneWidthFraction = 0.25, fractionLimit = 232.5, so the whitespace (630) wins. If the fraction constant were ever raised above ~0.677 the assertion would silently start testing the fraction path instead of the whitespace path. A guard assertion on fractionLimit would make the intent explicit.

Suggested change
func testTabBarLayoutUsesTrailingWhitespaceBeforeClippingSplitButtons() {
let measuredWidth = TabBarStyling.splitButtonsBackdropWidth(buttonCount: 10)
let layout = TabBarLayout(
tabBarHeight: 28,
availableWidth: 930,
tabContentWidthExcludingSplitButtonLane: 300,
splitButtonCount: 10,
splitButtonLaneVisible: true,
reservesSplitButtonLane: true,
measuredSplitButtonLaneWidth: measuredWidth
)
XCTAssertEqual(layout.maximumSplitButtonLaneWidth, 630)
func testTabBarLayoutUsesTrailingWhitespaceBeforeClippingSplitButtons() {
let measuredWidth = TabBarStyling.splitButtonsBackdropWidth(buttonCount: 10)
let layout = TabBarLayout(
tabBarHeight: 28,
availableWidth: 930,
tabContentWidthExcludingSplitButtonLane: 300,
splitButtonCount: 10,
splitButtonLaneVisible: true,
reservesSplitButtonLane: true,
measuredSplitButtonLaneWidth: measuredWidth
)
// fractionLimit = 930 * 0.25 = 232.5 < trailingWhitespace = 930 - 300 = 630,
// so the whitespace path governs maximumSplitButtonLaneWidth.
let fractionLimit = 930 * TabBarStyling.maximumSplitButtonLaneWidthFraction
XCTAssertLessThan(fractionLimit, 630, "Test precondition: whitespace must exceed fractionLimit")
XCTAssertEqual(layout.maximumSplitButtonLaneWidth, 630)

XCTAssertEqual(layout.visibleSplitButtonLaneWidth, measuredWidth)
XCTAssertEqual(layout.trailingTabContentInset, measuredWidth)
XCTAssertFalse(layout.splitButtonLaneOverflowsViewport)
}
Comment on lines +425 to +441

@coderabbitai coderabbitai Bot May 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Update companion rendering assertions for the new lane-width contract.

This new contract allows the action lane to exceed the quarter cap when trailing whitespace exists, but testOverflowingSplitButtonsClipToActionLane still samples using the old quarter-cap assumption and is now failing in CI (Line 1956). Please update that test/helper to derive its “outside lane” sample area from the same effective visibleSplitButtonLaneWidth inputs used at runtime.

🤖 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 `@Tests/BonsplitTests/BonsplitTests.swift` around lines 425 - 441, The failing
test uses an outdated quarter-cap assumption for the “outside lane” sample area;
update the test/helper used by testOverflowingSplitButtonsClipToActionLane to
compute its sampled outside-lane region from the same effective
visibleSplitButtonLaneWidth that TabBarLayout uses at runtime (e.g. use
layout.visibleSplitButtonLaneWidth or measuredSplitButtonLaneWidth inputs passed
into TabBarLayout) instead of a hard-coded quarter-cap; ensure any helper that
computes the sample rect references TabBarLayout.visibleSplitButtonLaneWidth so
the test reflects the new lane-width contract.


func testTabBarLayoutKeepsMeasuredLaneWhenItFitsQuarterOfAvailableWidth() {
let layout = TabBarLayout(
tabBarHeight: 28,
Expand Down
Loading