Repository navigation
Separate pane background from tab chrome - #105
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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:
📝 WalkthroughWalkthroughReplaces hardcoded split-button behavior with configurable split-action buttons and backdrop styles; adds pane-specific color resolution and NSColor accessors for tab bar/chrome/backdrop; forwards custom button actions via BonsplitController/BonsplitDelegate; refactors TabBar trailing-reserve sizing, rendering, and tests. Changes
Sequence DiagramsequenceDiagram
participant User
participant TabBarView
participant BonsplitController
participant BonsplitDelegate
participant Host
User->>TabBarView: Click split action button
activate TabBarView
alt action is .custom(identifier)
TabBarView->>BonsplitController: requestCustomAction(identifier, pane)
activate BonsplitController
BonsplitController->>BonsplitDelegate: splitTabBar(... didRequestCustomAction: identifier, inPane: pane)
activate BonsplitDelegate
BonsplitDelegate->>Host: forward custom action
Host-->>BonsplitDelegate: handled
deactivate BonsplitDelegate
deactivate BonsplitController
else predefined action (newTerminal/newBrowser/splitRight/splitDown)
TabBarView->>TabBarView: performSplitActionButton(action, pane)
end
deactivate TabBarView
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 does two things: it adds Confidence Score: 4/5Safe to merge; all findings are P2 style/quality concerns with no functional regressions on the changed paths. No P0 or P1 issues found. The two cache cost-accounting gaps (invalid-image key memory and compressed-vs-uncompressed image size) can cause the cache to use more memory than intended under adversarial or heavy image loads, but neither causes data loss or incorrect behavior. The redundant guard and rawValue ambiguity are cosmetic. Comprehensive test coverage increases confidence. Sources/Bonsplit/Internal/Views/TabBarView.swift — SplitActionButtonImageCache cost accounting for both the valid and invalid image caches. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[TabBarView\ndouble-click / button tap] --> B{action}
B -->|newTerminal| C[requestNewTab\nkind: terminal]
B -->|newBrowser| D[requestNewTab\nkind: browser]
B -->|splitRight| E[splitPane\norientation: horizontal]
B -->|splitDown| F[splitPane\norientation: vertical]
B -->|custom identifier| G[requestCustomAction\nidentifier]
G --> H[BonsplitDelegate\ndidRequestCustomAction]
C --> I[BonsplitDelegate\ndidRequestNewTab]
D --> I
subgraph Appearance
J[ChromeColors\nbackgroundHex] --> K[tab chrome color]
L[ChromeColors\npaneBackgroundHex] -->|set| M[pane background color]
L -->|unset| K
N[splitButtons array] --> O[SplitActionButtonImageCache\nNSCache keyed by NSData]
O --> P[rendered NSImage\ntemplate or original]
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -322,7 +361,8 @@ struct TabBarView: View { | |||
| private var trailingTabContentInset: CGFloat { | |||
| TabBarStyling.trailingTabContentInset( | |||
| showSplitButtons: showSplitButtons, | |||
| isMinimalMode: isMinimalMode | |||
| isMinimalMode: isMinimalMode, | |||
There was a problem hiding this comment.
The cost stored for a successfully decoded NSImage is max(1, data.count) — the compressed source bytes. For PNG or JPEG inputs, the decompressed in-memory representation is typically several times larger; for SVG, an NSImage backed by a NSPDFImageRep or Core Graphics rasterisation can be much larger still. When the cache compares total cost against totalCostLimit = 8 MB, it will think it is well under budget while the actual resident memory is much higher. Consider using image.representations.compactMap { $0 as? NSBitmapImageRep }.reduce(0) { $0 + $1.bytesPerRow * $1.pixelsHigh } as a more accurate cost, with data.count as a reasonable fallback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/Styling/TabBarColors.swift`:
- Around line 20-27: paneBackgroundColor(for:) currently returns nil when
appearance.chromeColors.paneBackgroundHex exists but is malformed, causing
callers to fall back all the way to .textBackgroundColor; update
paneBackgroundColor(for:) to try constructing NSColor(bonsplitHex: value) and if
that returns nil (malformed hex) fall back to chromeBackgroundColor(for:
appearance) instead of returning nil. Locate the paneBackgroundColor(for:)
function and change the guard to attempt creating the color, returning the
created NSColor when valid and chromeBackgroundColor(for:) when invalid or
missing.
In `@Sources/Bonsplit/Internal/Views/TabBarView.swift`:
- Around line 1091-1111: The cache cost currently uses compressed data.count
which underestimates memory; in image(for data: Data) compute the decoded bitmap
byte size and use that as the cache cost instead of data.count: after creating
the NSImage, iterate its representations (or obtain an NSBitmapImageRep/CGImage)
to calculate bytes = sum(representation.bytesPerRow * representation.pixelsHigh)
or pixelsWide * pixelsHigh * (bitsPerPixel/8) for each bitmap rep, clamp to at
least 1, and pass that value to images.setObject(..., cost: decodedByteCount);
also use the same decodedByteCount when marking invalidImageData so the cache
accounting reflects actual decoded memory usage.
🪄 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: ab037dda-e91d-4e7c-8a0b-de703c99bdad
📒 Files selected for processing (6)
Sources/Bonsplit/Internal/Styling/TabBarColors.swiftSources/Bonsplit/Internal/Views/TabBarView.swiftSources/Bonsplit/Public/BonsplitConfiguration.swiftSources/Bonsplit/Public/BonsplitController.swiftSources/Bonsplit/Public/BonsplitDelegate.swiftTests/BonsplitTests/BonsplitTests.swift
| func image(for data: Data) -> NSImage? { | ||
| let key = data as NSData | ||
| if let image = images.object(forKey: key) { | ||
| return image | ||
| } | ||
| if invalidImageData.object(forKey: key) != nil { | ||
| return nil | ||
| } | ||
|
|
||
| guard let image = NSImage(data: data) else { | ||
| invalidImageData.setObject( | ||
| NSNumber(value: true), | ||
| forKey: key, | ||
| cost: max(1, min(data.count, 1024)) | ||
| ) | ||
| return nil | ||
| } | ||
| image.isTemplate = TabBarStyling.imageDataShouldRenderAsTemplate(data) | ||
|
|
||
| images.setObject(image, forKey: key, cost: max(1, data.count)) | ||
| return image |
There was a problem hiding this comment.
Use decoded image size for the cache cost.
The cache is capped, but the cost is based on data.count, which is only the compressed payload size. A small PNG can decode into a much larger bitmap, so the 8 MB limit will not actually bound memory for oversized split-button assets.
♻️ Proposed fix
func image(for data: Data) -> NSImage? {
let key = data as NSData
if let image = images.object(forKey: key) {
return image
}
@@
guard let image = NSImage(data: data) else {
invalidImageData.setObject(
NSNumber(value: true),
forKey: key,
cost: max(1, min(data.count, 1024))
)
return nil
}
image.isTemplate = TabBarStyling.imageDataShouldRenderAsTemplate(data)
-
- images.setObject(image, forKey: key, cost: max(1, data.count))
+ let decodedCost = image.representations
+ .compactMap { $0 as? NSBitmapImageRep }
+ .reduce(0) { $0 + ($1.bytesPerRow * $1.pixelsHigh) }
+
+ images.setObject(
+ image,
+ forKey: key,
+ cost: max(1, decodedCost > 0 ? decodedCost : data.count)
+ )
return image
}🤖 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 1091 - 1111,
The cache cost currently uses compressed data.count which underestimates memory;
in image(for data: Data) compute the decoded bitmap byte size and use that as
the cache cost instead of data.count: after creating the NSImage, iterate its
representations (or obtain an NSBitmapImageRep/CGImage) to calculate bytes =
sum(representation.bytesPerRow * representation.pixelsHigh) or pixelsWide *
pixelsHigh * (bitsPerPixel/8) for each bitmap rep, clamp to at least 1, and pass
that value to images.setObject(..., cost: decodedByteCount); also use the same
decodedByteCount when marking invalidImageData so the cache accounting reflects
actual decoded memory usage.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 6 files
You’re at about 99% of the monthly review limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
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/Styling/TabBarColors.swift">
<violation number="1" location="Sources/Bonsplit/Internal/Styling/TabBarColors.swift:26">
P2: Invalid `paneBackgroundHex` values skip fallback to `backgroundHex` and can produce an unexpected pane color.</violation>
</file>
<file name="Sources/Bonsplit/Internal/Views/TabBarView.swift">
<violation number="1" location="Sources/Bonsplit/Internal/Views/TabBarView.swift:933">
P2: Empty tab-bar new-tab interactions are incorrectly coupled to split-button visibility/config. If `.newTerminal` is not a visible split button, click/double-click in the empty tab-bar area stops creating a new tab.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| guard let value = appearance.chromeColors.paneBackgroundHex else { | ||
| return chromeBackgroundColor(for: appearance) | ||
| } | ||
| return NSColor(bonsplitHex: value) |
There was a problem hiding this comment.
P2: Invalid paneBackgroundHex values skip fallback to backgroundHex and can produce an unexpected pane color.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Bonsplit/Internal/Styling/TabBarColors.swift, line 26:
<comment>Invalid `paneBackgroundHex` values skip fallback to `backgroundHex` and can produce an unexpected pane color.</comment>
<file context>
@@ -17,6 +17,15 @@ enum TabBarColors {
+ guard let value = appearance.chromeColors.paneBackgroundHex else {
+ return chromeBackgroundColor(for: appearance)
+ }
+ return NSColor(bonsplitHex: value)
+ }
+
</file context>
| return NSColor(bonsplitHex: value) | |
| return NSColor(bonsplitHex: value) ?? chromeBackgroundColor(for: appearance) |
| guard let button = visibleSplitButtons.first(where: { $0.action == .newTerminal }) else { | ||
| return false | ||
| } | ||
| performSplitActionButton(button) | ||
| return true |
There was a problem hiding this comment.
P2: Empty tab-bar new-tab interactions are incorrectly coupled to split-button visibility/config. If .newTerminal is not a visible split button, click/double-click in the empty tab-bar area stops creating a new tab.
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 933:
<comment>Empty tab-bar new-tab interactions are incorrectly coupled to split-button visibility/config. If `.newTerminal` is not a visible split button, click/double-click in the empty tab-bar area stops creating a new tab.</comment>
<file context>
@@ -789,47 +829,112 @@ struct TabBarView: View {
+
+ private func performNewTerminalSplitButtonAction() -> Bool {
+ guard splitViewController.isInteractive else { return false }
+ guard let button = visibleSplitButtons.first(where: { $0.action == .newTerminal }) else {
+ return false
+ }
</file context>
| guard let button = visibleSplitButtons.first(where: { $0.action == .newTerminal }) else { | |
| return false | |
| } | |
| performSplitActionButton(button) | |
| return true | |
| controller.requestNewTab(kind: "terminal", inPane: pane.id) | |
| return true |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/Bonsplit/Internal/Views/TabBarView.swift (1)
836-850: PreferIdentifiableover indices for the split-buttonForEach.
SplitActionButtonalready conforms toIdentifiable(stableid: String), soForEach(buttons.indices, id: \.self)discards that identity and ties view state to position. If a host reorders or swapsappearance.splitButtonsat runtime, SwiftUI will reuse views by index instead of by button id, which can cause incorrect transitions, stale tooltips, and animation glitches. Using the buttons directly preserves identity.♻️ Proposed refactor
- HStack(spacing: TabBarStyling.splitButtonsSpacing) { - ForEach(buttons.indices, id: \.self) { index in - let button = buttons[index] - Button { - performSplitActionButton(button) - } label: { - splitActionButtonIcon(button.icon) - } - .buttonStyle(SplitActionButtonStyle(appearance: appearance)) - .safeHelp(splitActionButtonTooltip(button, tooltips: tooltips)) - } - } + HStack(spacing: TabBarStyling.splitButtonsSpacing) { + ForEach(buttons) { button in + Button { + performSplitActionButton(button) + } label: { + splitActionButtonIcon(button.icon) + } + .buttonStyle(SplitActionButtonStyle(appearance: appearance)) + .safeHelp(splitActionButtonTooltip(button, tooltips: tooltips)) + } + }🤖 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 836 - 850, Replace the index-based ForEach with identity-based iteration so SwiftUI tracks each SplitActionButton by its id instead of position: iterate ForEach over buttons (the appearance.splitButtons array) directly rather than buttons.indices, relying on SplitActionButton's Identifiable conformance; keep existing uses of performSplitActionButton(button), splitActionButtonIcon(button.icon), .buttonStyle(SplitActionButtonStyle(appearance: appearance)) and .safeHelp(splitActionButtonTooltip(button, tooltips: tooltips)) so the behavior and tooltips remain unchanged while preserving stable view identity.
🤖 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 836-850: Replace the index-based ForEach with identity-based
iteration so SwiftUI tracks each SplitActionButton by its id instead of
position: iterate ForEach over buttons (the appearance.splitButtons array)
directly rather than buttons.indices, relying on SplitActionButton's
Identifiable conformance; keep existing uses of
performSplitActionButton(button), splitActionButtonIcon(button.icon),
.buttonStyle(SplitActionButtonStyle(appearance: appearance)) and
.safeHelp(splitActionButtonTooltip(button, tooltips: tooltips)) so the behavior
and tooltips remain unchanged while preserving stable view identity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 796f97b4-26cb-4090-a1db-01776c700a17
📒 Files selected for processing (3)
Sources/Bonsplit/Internal/Styling/TabBarColors.swiftSources/Bonsplit/Internal/Views/TabBarView.swiftTests/BonsplitTests/BonsplitTests.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- Sources/Bonsplit/Internal/Styling/TabBarColors.swift
- Tests/BonsplitTests/BonsplitTests.swift
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Sources/Bonsplit/Internal/Views/TabBarView.swift (1)
186-194:String(decoding:as:)here is intentional — flag it for the linter.SwiftLint's
optional_data_string_conversionwarning at line 187 is a false positive:imageDataShouldRenderAsTemplatedeliberately tolerates invalid UTF-8 by relying on replacement substitution (covered bytestCurrentColorSVGImageDataRendersAsTemplateWithInvalidUTF8Suffix). Consider an inline// swiftlint:disable:next optional_data_string_conversionso the lint signal stays clean.♻️ Suggested annotation
static func imageDataShouldRenderAsTemplate(_ data: Data) -> Bool { + // swiftlint:disable:next optional_data_string_conversion + // SVG inspection must tolerate invalid UTF-8 trailers; replacement chars are fine here. let text = String(decoding: data.prefix(4096), as: UTF8.self) let lowercased = text.lowercased() return lowercased.contains("<svg") && lowercased.contains("currentcolor") }🤖 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 186 - 194, Add an inline SwiftLint disable for the false-positive `optional_data_string_conversion` on the `imageDataShouldRenderAsTemplate(_:)` function: specifically place a `// swiftlint:disable:next optional_data_string_conversion` immediately above the `String(decoding: data.prefix(4096), as: UTF8.self)` line so the linter ignores the intentional tolerant UTF-8 decode; keep the rest of the implementation (the lowercased checks for "<svg" and "currentcolor") and ensure existing test `testCurrentColorSVGImageDataRendersAsTemplateWithInvalidUTF8Suffix` still validates the behavior.Sources/Bonsplit/Public/BonsplitConfiguration.swift (1)
106-175: Optional: validateemojiscale at construction too, for round-trip symmetry.
emojiScale(in:)rejects non-finite/non-positive scales on decode, but the publicIcon.emoji(_, scale:)case constructor and theencode(to:)path don't. SoIcon.emoji("x", scale: -1)is constructible and encodable, but the resulting JSON will fail to decode. Runtime rendering is safe becauseemojiIconFontSizealready clamps, so this is just an API tidiness concern.♻️ One way to enforce symmetry
public func encode(to encoder: Encoder) throws { var container = encoder.container(keyedBy: CodingKeys.self) switch self { case .systemImage(let name): try container.encode("systemImage", forKey: .type) try container.encode(name, forKey: .name) case .emoji(let value, let scale): try container.encode("emoji", forKey: .type) try container.encode(value, forKey: .value) - if scale != 1 { + guard scale.isFinite, scale > 0 else { + throw EncodingError.invalidValue(scale, .init( + codingPath: container.codingPath + [CodingKeys.scale], + debugDescription: "Emoji icon scale must be a positive finite number" + )) + } + if scale != 1 { try container.encode(scale, forKey: .scale) }Or — depending on whether you'd rather fail loud — sanitize at construction with a static factory.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Bonsplit/Public/BonsplitConfiguration.swift` around lines 106 - 175, The emoji scale is validated on decode via emojiScale(in:) but not when someone constructs Icon.emoji(...) or when encoding, causing asymmetry; add the same finite-and-positive check in encode(to:) before writing the scale (throw DecodingError.dataCorruptedError or EncodingError.invalidValue as appropriate) and provide a public factory like static func emoji(_ value: String, scale: Double = 1) -> Icon that enforces/normalizes the scale (or documents use), reusing emojiScale logic for consistency; update references to Icon.emoji construction to prefer the factory for round-trip symmetry.
🤖 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 186-194: Add an inline SwiftLint disable for the false-positive
`optional_data_string_conversion` on the `imageDataShouldRenderAsTemplate(_:)`
function: specifically place a `// swiftlint:disable:next
optional_data_string_conversion` immediately above the `String(decoding:
data.prefix(4096), as: UTF8.self)` line so the linter ignores the intentional
tolerant UTF-8 decode; keep the rest of the implementation (the lowercased
checks for "<svg" and "currentcolor") and ensure existing test
`testCurrentColorSVGImageDataRendersAsTemplateWithInvalidUTF8Suffix` still
validates the behavior.
In `@Sources/Bonsplit/Public/BonsplitConfiguration.swift`:
- Around line 106-175: The emoji scale is validated on decode via
emojiScale(in:) but not when someone constructs Icon.emoji(...) or when
encoding, causing asymmetry; add the same finite-and-positive check in
encode(to:) before writing the scale (throw DecodingError.dataCorruptedError or
EncodingError.invalidValue as appropriate) and provide a public factory like
static func emoji(_ value: String, scale: Double = 1) -> Icon that
enforces/normalizes the scale (or documents use), reusing emojiScale logic for
consistency; update references to Icon.emoji construction to prefer the factory
for round-trip symmetry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a4dd5c33-d58a-4de0-8706-aba8dfae12ca
📒 Files selected for processing (4)
Sources/Bonsplit/Internal/Styling/TabBarColors.swiftSources/Bonsplit/Internal/Views/TabBarView.swiftSources/Bonsplit/Public/BonsplitConfiguration.swiftTests/BonsplitTests/BonsplitTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/Bonsplit/Internal/Styling/TabBarColors.swift
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Sources/Bonsplit/Public/BonsplitConfiguration.swift (2)
349-374: Consider conformingSplitButtonBackdropEffecttoEquatable.
SplitButtonTooltips(and similar appearance value types) isSendable, Equatable, butSplitButtonBackdropEffectis onlySendable. AddingEquatable(synthesizable since all stored properties areEquatable) lets hosts use it with SwiftUIonChange(of:),==, and test assertions without writing custom comparators. Same applies to making the newAppearanceproperties uniformly comparable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Bonsplit/Public/BonsplitConfiguration.swift` around lines 349 - 374, SplitButtonBackdropEffect is currently Sendable but not Equatable which prevents using it with SwiftUI onChange/== and tests; add Equatable conformance to the struct declaration (public struct SplitButtonBackdropEffect: Sendable, Equatable) so the compiler can synthesize == (all stored properties are already Equatable), and mirror this approach for related appearance value types (e.g., any new Appearance properties) so they are also Equatable and uniformly comparable.
274-284: Consider validatingIcon.emojiscale in the constructor too.
emojiScale(in:)enforcesscale.isFinite && scale > 0only on the decode path. A caller constructingSplitActionButton(id:..., icon: .emoji("🚀", scale: .nan), ...)(or0/negative) bypasses this check, thenemojiIconFontSize(scale:)inTabBarViewfalls back to1, but the value still participates inHashable/Equatable(NaN is pathological forHashable) and round-trips asymmetrically through Codable (the decoder would reject what the encoder just wrote).Either clamp/validate in
SplitActionButton.init(or in a factory for.emoji), or document that.emoji(_, scale:)requires a positive finite value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Bonsplit/Public/BonsplitConfiguration.swift` around lines 274 - 284, The SplitActionButton initializer currently accepts Icon.emoji values without validating the emoji scale, which can allow non-finite or non-positive scales (NaN/0/negative) to leak into Equatable/Hashable and into UI sizing; update SplitActionButton.init (or add a small factory/validator used by the initializer) to validate and/or clamp the Icon.emoji scale to a positive finite value (e.g., reject/throw or replace with a sane default >0) using the same rules as emojiScale(in:), so emojiIconFontSize(scale:) and TabBarView always receive a valid finite >0 scale and Codable/Hashable semantics remain consistent.
🤖 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 932-953: In splitActionButtonTooltip(_:tooltips:) the .custom(let
identifier) branch currently returns the raw identifier when button.tooltip is
missing; change this to avoid exposing internal identifiers by returning an
empty string (or a localized "help" placeholder) instead and optionally emit a
debug-only assertion/log mentioning BonsplitConfiguration.SplitActionButton and
the identifier; update the .custom case in splitActionButtonTooltip to return ""
(and add a debug assert/log) rather than returning identifier.
---
Nitpick comments:
In `@Sources/Bonsplit/Public/BonsplitConfiguration.swift`:
- Around line 349-374: SplitButtonBackdropEffect is currently Sendable but not
Equatable which prevents using it with SwiftUI onChange/== and tests; add
Equatable conformance to the struct declaration (public struct
SplitButtonBackdropEffect: Sendable, Equatable) so the compiler can synthesize
== (all stored properties are already Equatable), and mirror this approach for
related appearance value types (e.g., any new Appearance properties) so they are
also Equatable and uniformly comparable.
- Around line 274-284: The SplitActionButton initializer currently accepts
Icon.emoji values without validating the emoji scale, which can allow non-finite
or non-positive scales (NaN/0/negative) to leak into Equatable/Hashable and into
UI sizing; update SplitActionButton.init (or add a small factory/validator used
by the initializer) to validate and/or clamp the Icon.emoji scale to a positive
finite value (e.g., reject/throw or replace with a sane default >0) using the
same rules as emojiScale(in:), so emojiIconFontSize(scale:) and TabBarView
always receive a valid finite >0 scale and Codable/Hashable semantics remain
consistent.
🪄 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: 61c5511f-f970-47b1-b596-579988dedd85
📒 Files selected for processing (2)
Sources/Bonsplit/Internal/Views/TabBarView.swiftSources/Bonsplit/Public/BonsplitConfiguration.swift
| private func splitActionButtonTooltip( | ||
| _ button: BonsplitConfiguration.SplitActionButton, | ||
| tooltips: BonsplitConfiguration.SplitButtonTooltips | ||
| ) -> String { | ||
| if let tooltip = button.tooltip?.trimmingCharacters(in: .whitespacesAndNewlines), | ||
| !tooltip.isEmpty { | ||
| return tooltip | ||
| } | ||
|
|
||
| switch button.action { | ||
| case .newTerminal: | ||
| return tooltips.newTerminal | ||
| case .newBrowser: | ||
| return tooltips.newBrowser | ||
| case .splitRight: | ||
| return tooltips.splitRight | ||
| case .splitDown: | ||
| return tooltips.splitDown | ||
| case .custom(let identifier): | ||
| return identifier | ||
| } | ||
| } |
There was a problem hiding this comment.
Custom-action tooltip falls back to the raw identifier.
For .custom(let identifier), when no explicit tooltip is set the user-facing tooltip becomes the raw identifier string (Line 951). For embedder-defined identifiers like "open-settings" or "com.acme.action.foo", that surfaces in the UI as the tooltip text. Consider either requiring a non-empty tooltip for custom buttons (assert/log in DEBUG) or returning an empty string / .help(...) no-op when the tooltip is missing for .custom, so hosts don't accidentally ship internal identifiers to end users.
🤖 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 932 - 953, In
splitActionButtonTooltip(_:tooltips:) the .custom(let identifier) branch
currently returns the raw identifier when button.tooltip is missing; change this
to avoid exposing internal identifiers by returning an empty string (or a
localized "help" placeholder) instead and optionally emit a debug-only
assertion/log mentioning BonsplitConfiguration.SplitActionButton and the
identifier; update the .custom case in splitActionButtonTooltip to return ""
(and add a debug assert/log) rather than returning identifier.
There was a problem hiding this comment.
🧹 Nitpick comments (4)
Sources/Bonsplit/Public/BonsplitConfiguration.swift (2)
451-457: ConfirmdidSet-based dedup interacts cleanly with external mutation patterns.The
splitButtonssetter dedupes viadidSet { splitButtons = Self.uniqueSplitButtons(splitButtons) }. This is safe in Swift (re-assigning insidedidSetdoes not infinitely recurse, anduniqueSplitButtonsis idempotent), and the testtestAppearanceKeepsFirstSplitActionButtonForDuplicateIdsexercises both the initializer and post-init reassignment paths.One minor caveat: if a host uses
KeyPath-based bindings (e.g., a settings UI bound toappearance.splitButtons), the apparent "value committed" can differ from "value assigned" when duplicates are passed in. This is the documented behavior, but worth a one-line note in the doc-comment so embedder UIs don't get surprised.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Bonsplit/Public/BonsplitConfiguration.swift` around lines 451 - 457, The doc-comment for the public property splitButtons should mention that duplicates are removed in its didSet via Self.uniqueSplitButtons, so the value actually committed may differ from the value assigned (e.g., when using KeyPath/binding-based UIs); update the comment for splitButtons to add a single-line note stating that duplicate button ids are ignored and that post-assignment deduplication may alter the value observed by bindings, referencing splitButtons, SplitActionButton and uniqueSplitButtons to make the behavior clear to embedders.
349-383: Direct property mutation can bypass the init's clamping.
SplitButtonBackdropEffectclampsfadeRampStartFractionto[0, 0.95], opacities/contentOcclusionFractionto[0, 1], and widths to>= 0ininit, but the properties are exposed aspublic varso a host can later assigneffect.fadeRampStartFraction = 5.0(whichLinearGradientstops would reject),trailingOpacity = -1, etc.TabBarViewre-clamps when reading these, so this isn't currently a runtime defect — but it makes the invariant fragile against future call sites that read directly.Consider either making the stored properties
let(and exposing awith(...)builder for mutation) or moving the clamping into per-propertydidSetobservers so the contract holds across the entire lifetime of an instance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Bonsplit/Public/BonsplitConfiguration.swift` around lines 349 - 383, The struct SplitButtonBackdropEffect exposes public var properties that can bypass the clamping performed in init; change the API so the invariants cannot be violated at runtime — either convert the stored properties (style, fadeWidth, contentFadeWidth, solidWidth, fadeRampStartFraction, leadingOpacity, trailingOpacity, contentOcclusionFraction, masksTabContent) to let and add a fluent with(...) builder or factory method on SplitButtonBackdropEffect that returns a new clamped instance, or if mutability must remain, add per-property didSet clamps mirroring the init logic (use min/max clamps) to enforce ranges on assignment; update the public static default accordingly and adjust any callers (e.g., TabBarView) to use the new with(...) or rely on the immutable properties.Sources/Bonsplit/Internal/Views/TabBarView.swift (2)
866-877: Prefer identity-basedForEachsinceSplitActionButtonisIdentifiable.
ForEach(buttons.indices, id: \.self)keys rows by ordinal position, so reordering or removing a button reuses identities by index and can lead to stale closures or unwanted animation transitions. SinceSplitActionButtonalready conforms toIdentifiablewith aStringid, iterating over the array directly gives stable identity-based diffing.♻️ Proposed simplification
- HStack(spacing: TabBarStyling.splitButtonsSpacing) { - ForEach(buttons.indices, id: \.self) { index in - let button = buttons[index] - Button { - performSplitActionButton(button) - } label: { - splitActionButtonIcon(button.icon) - } - .buttonStyle(SplitActionButtonStyle(appearance: appearance)) - .safeHelp(splitActionButtonTooltip(button, tooltips: tooltips)) - } - } + HStack(spacing: TabBarStyling.splitButtonsSpacing) { + ForEach(buttons) { button in + Button { + performSplitActionButton(button) + } label: { + splitActionButtonIcon(button.icon) + } + .buttonStyle(SplitActionButtonStyle(appearance: appearance)) + .safeHelp(splitActionButtonTooltip(button, tooltips: tooltips)) + } + }🤖 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 866 - 877, The ForEach in TabBarView currently iterates over buttons.indices (ForEach(buttons.indices, id: \.self)) which keys by index; change it to iterate directly over the buttons array so SwiftUI uses each SplitActionButton's stable Identifiable id (e.g., ForEach(buttons) { button in ... }). Update the loop body to use the button value (not buttons[index]) and keep the existing calls to performSplitActionButton(button), splitActionButtonIcon(button.icon), and splitActionButtonTooltip(button, tooltips: tooltips), leaving the SplitActionButtonStyle and safeHelp unchanged.
186-194: Intentional use of non-failableString(decoding:as:)— SwiftLint hint is a false positive here.The SwiftLint warning (
optional_data_string_conversion) suggests the failableString(bytes:encoding:)initializer, buttestCurrentColorSVGImageDataRendersAsTemplateWithInvalidUTF8Suffixexplicitly verifies that template detection still works when the trailing bytes are invalid UTF-8. Switching to the failable initializer would returnnilon that input and break the test. The current implementation is correct — consider adding// swiftlint:disable:next optional_data_string_conversionso future contributors don't get nudged into a regression.🤖 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 186 - 194, The SwiftLint hint is a false positive for imageDataShouldRenderAsTemplate(_:): keep the non-failable String(decoding:as:) usage to preserve behavior for invalid UTF-8 suffixes; add a SwiftLint suppression comment immediately above the function (e.g., // swiftlint:disable:next optional_data_string_conversion) so the linter won't suggest switching to the failable initializer and risk breaking testCurrentColorSVGImageDataRendersAsTemplateWithInvalidUTF8Suffix.
🤖 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 866-877: The ForEach in TabBarView currently iterates over
buttons.indices (ForEach(buttons.indices, id: \.self)) which keys by index;
change it to iterate directly over the buttons array so SwiftUI uses each
SplitActionButton's stable Identifiable id (e.g., ForEach(buttons) { button in
... }). Update the loop body to use the button value (not buttons[index]) and
keep the existing calls to performSplitActionButton(button),
splitActionButtonIcon(button.icon), and splitActionButtonTooltip(button,
tooltips: tooltips), leaving the SplitActionButtonStyle and safeHelp unchanged.
- Around line 186-194: The SwiftLint hint is a false positive for
imageDataShouldRenderAsTemplate(_:): keep the non-failable String(decoding:as:)
usage to preserve behavior for invalid UTF-8 suffixes; add a SwiftLint
suppression comment immediately above the function (e.g., //
swiftlint:disable:next optional_data_string_conversion) so the linter won't
suggest switching to the failable initializer and risk breaking
testCurrentColorSVGImageDataRendersAsTemplateWithInvalidUTF8Suffix.
In `@Sources/Bonsplit/Public/BonsplitConfiguration.swift`:
- Around line 451-457: The doc-comment for the public property splitButtons
should mention that duplicates are removed in its didSet via
Self.uniqueSplitButtons, so the value actually committed may differ from the
value assigned (e.g., when using KeyPath/binding-based UIs); update the comment
for splitButtons to add a single-line note stating that duplicate button ids are
ignored and that post-assignment deduplication may alter the value observed by
bindings, referencing splitButtons, SplitActionButton and uniqueSplitButtons to
make the behavior clear to embedders.
- Around line 349-383: The struct SplitButtonBackdropEffect exposes public var
properties that can bypass the clamping performed in init; change the API so the
invariants cannot be violated at runtime — either convert the stored properties
(style, fadeWidth, contentFadeWidth, solidWidth, fadeRampStartFraction,
leadingOpacity, trailingOpacity, contentOcclusionFraction, masksTabContent) to
let and add a fluent with(...) builder or factory method on
SplitButtonBackdropEffect that returns a new clamped instance, or if mutability
must remain, add per-property didSet clamps mirroring the init logic (use
min/max clamps) to enforce ranges on assignment; update the public static
default accordingly and adjust any callers (e.g., TabBarView) to use the new
with(...) or rely on the immutable properties.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0992b3a1-5eb2-4228-b4c8-971ff115a901
📒 Files selected for processing (4)
Sources/Bonsplit/Internal/Styling/TabBarColors.swiftSources/Bonsplit/Internal/Views/TabBarView.swiftSources/Bonsplit/Public/BonsplitConfiguration.swiftTests/BonsplitTests/BonsplitTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/Bonsplit/Internal/Styling/TabBarColors.swift
1 participant
Adds an optional pane background color so embedders can keep tab chrome themed without repainting terminal panes underneath their own renderer.
Summary by cubic
Separates the pane background from the tab chrome and adds fully configurable split action buttons with host‑defined actions. Softens the split‑button backdrop with finer fade control and new content occlusion to keep contrast without heavy visuals.
New Features
appearance.chromeColors.paneBackgroundHex; falls back tobackgroundHexwhen unset.appearance.chromeColors.tabBarBackgroundHexandsplitButtonBackdropHexfor finer surface control.appearance.splitButtons; duplicate ids are ignored. Icons support SF Symbols, emoji (with scale), or image data; auto‑detects template SVGs and caches images.BonsplitController.requestCustomAction(_:inPane:)andBonsplitDelegate.splitTabBar(_:didRequestCustomAction:inPane:).appearance.splitButtonBackdropEffect(andsplitButtonBackdropStyle) to set fade width, content fade width, solid width, ramp start, leading/trailing opacity, content occlusion fraction, and whether tab content is masked. Backdrops paint when any split‑backdrop, tab‑bar, or chrome background is customized; defaults are softened for a subtler fade.newTerminalbutton if present; otherwise no‑op.BonsplitController.tabShortcutHintsEnabled.Bug Fixes
Written for commit 3ac045a. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements
Tests