Repository navigation
Fix split action lane clipping before empty tab space - #123
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughTabBarLayout now accepts an optional ChangesSplit-button lane width constraint refinement
Possibly related PRs
Poem
🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Tests/BonsplitTests/BonsplitTests.swift`:
- Around line 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.
🪄 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: 58987a5b-015c-45e3-bf07-5923cbb85283
📒 Files selected for processing (2)
Sources/Bonsplit/Internal/Views/TabBarView.swiftTests/BonsplitTests/BonsplitTests.swift
| 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) | ||
| XCTAssertEqual(layout.visibleSplitButtonLaneWidth, measuredWidth) | ||
| XCTAssertEqual(layout.trailingTabContentInset, measuredWidth) | ||
| XCTAssertFalse(layout.splitButtonLaneOverflowsViewport) | ||
| } |
There was a problem hiding this comment.
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.
Greptile SummaryThis PR introduces a measurement-based expansion of the split action button lane: instead of always capping it at
Confidence Score: 4/5Safe to merge; the trailing-whitespace expansion logic is correct and self-stabilizing with no risk of infinite layout loops. The core layout change is small and well-reasoned. The measurement loop in
Important Files Changed
Sequence DiagramsequenceDiagram
participant SV as ScrollView (contentGeo)
participant TV as TabBarView
participant TBL as TabBarLayout
Note over TV: Initial state: tabContentWidthExcludingSplitButtonLane = nil
SV->>TV: onAppear / onChange(frame)
TV->>TBL: "trailingTabContentInset (nil state -> fractionLimit-based T_old)"
TV->>TV: "tabContentWidthExcludingSplitButtonLane = frame.width - T_old"
Note over TV: State update triggers re-render
TV->>TBL: recompute with new tabContentWidthExcludingSplitButtonLane
TBL-->>TV: "trailingWhitespaceBeforeSplitButtonLane = availableWidth - tabs"
TBL-->>TV: "maximumSplitButtonLaneWidth = max(fractionLimit, whitespace)"
TBL-->>TV: "visibleSplitButtonLaneWidth = min(full, maximum)"
TBL-->>TV: "trailingTabContentInset = T_new"
Note over SV: Padding changes -> frame changes
SV->>TV: "onChange(newFrame) frame.width = tabs + T_new"
TV->>TV: "tabContentWidthExcludingSplitButtonLane = tabs + T_new - T_new = tabs"
Note over TV: State unchanged -> converged in 2 passes
Reviews (1): Last reviewed commit: "test: keep overflow fixture crowded" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private func updateTabScrollContent(frame: CGRect) { | ||
| scrollOffset = -frame.minX | ||
| contentWidth = frame.width | ||
| tabContentWidthExcludingSplitButtonLane = max(0, frame.width - tabBarLayout.trailingTabContentInset) | ||
| } |
There was a problem hiding this comment.
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.
| 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) | |
| } |
| 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) |
There was a problem hiding this comment.
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.
| 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) |
1 participant
Summary
Testing
Summary by cubic
Allow the split action lane to use trailing empty tab bar space before the fractional cap to prevent early clipping of split buttons. Addresses Linear issue 4110.
maximumSplitButtonLaneWidthwhen it exceeds the fraction cap.tabContentWidthExcludingSplitButtonLanefrom tab scroll geometry into layout.Written for commit e1d9954. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests