Repository navigation
Add tab icon and color customization - #196
Open
lawrencecchen wants to merge 10 commits into
Open
lawrencecchen wants to merge 10 commits into
lawrencecchen wants to merge 10 commits into
Conversation
|
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:
📝 WalkthroughWalkthroughThe PR adds per-tab background, icon, and accent metadata. It persists and propagates these values through tab creation, updates, transfers, and snapshots. It adds context-menu customization, color-aware rendering, icon selection, localization, documentation, tests, and an example debug command. ChangesPer-tab appearance customization
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant TabContextMenuPresenter
participant TabItemView
participant TabColorPickerView
participant TabBarView
participant BonsplitController
TabContextMenuPresenter->>TabItemView: open tab color controls
TabItemView->>TabColorPickerView: present custom color picker
TabColorPickerView->>TabBarView: return selected hex color
TabBarView->>BonsplitController: updateTab(colorHexOverride)
BonsplitController->>TabItemView: update tab metadata and rendering
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![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 `@Sources/Bonsplit/Internal/Views/TabItemView.swift`:
- Around line 604-605: Update the control color resolution in TabItemView: at
Sources/Bonsplit/Internal/Views/TabItemView.swift lines 604-605, use the
custom-aware audio hover foreground/surface pair; apply the same pair at lines
637-638 for zoom hover and lines 1072-1073 for close hover; at line 788, resolve
the pinned audio badge glyph against its custom-aware circle fill. Preserve the
existing control behavior while ensuring all glyphs and surfaces derive from the
tab’s custom background.
🪄 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 Plus
Run ID: 40679b1a-3596-44d1-abd2-7f5a2921122d
📒 Files selected for processing (11)
CHANGELOG.mdExample/BonsplitExample/AppState.swiftExample/BonsplitExample/BonsplitExampleApp.swiftREADME.mdSources/Bonsplit/Internal/Models/TabItem.swiftSources/Bonsplit/Internal/Styling/TabBarColors.swiftSources/Bonsplit/Internal/Views/TabDragPreview.swiftSources/Bonsplit/Internal/Views/TabItemView.swiftSources/Bonsplit/Public/BonsplitController.swiftSources/Bonsplit/Public/Types/Tab.swiftTests/BonsplitTests/BonsplitTests.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 11 files
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/Views/TabItemView.swift">
<violation number="1" location="Sources/Bonsplit/Internal/Views/TabItemView.swift:579">
P3: The remote indicator (and audio glyph) renders `currentTabText.opacity(0.78)`. For custom-background tabs, `currentTabText` is already the contrast-resolved white/black selected for that background, so the extra 0.78 alpha can push an already-marginal contrast below readable. Consider lowering opacity only for non-custom backgrounds, or raising the base contrast threshold for custom backgrounds so the dimmed state stays legible.</violation>
<violation number="2" location="Sources/Bonsplit/Internal/Views/TabItemView.swift:604">
P2: Hovering the audio, zoom, or close affordances can also produce low-contrast glyphs on custom tabs: these branches use custom-background text while their hover circles remain appearance-level colors. The hover text and fill should be resolved from the same surface, or the hover fill should become tab-aware.</violation>
</file>
<file name="Sources/Bonsplit/Internal/Styling/TabBarColors.swift">
<violation number="1" location="Sources/Bonsplit/Internal/Styling/TabBarColors.swift:259">
P3: For a tab with a custom background, hovering lightens/darkens the background in `tabBackground` but the text color resolved by `effectiveTabTextColor` is computed once from the unadjusted base color. On hover, the background shifts brightness (e.g., dark colors lighten by 0.07) while the chosen black/white text stays fixed, so contrast can drop below the level the code intended to guarantee. Consider resolving the hover-adjusted color first and feeding that into the contrast decision (or applying the adjustment to both text and background together) so readability stays consistent across hover/selected states.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 11 files
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:116">
P3: A valid 8-digit per-tab color with alpha `00` is treated as if `backgroundHex` were nil. Preserving the parsed clear color here would keep transparent colors distinct from the explicit `.some(nil)` reset path and avoid applying the standard selected-tab fill unexpectedly.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
lawrencecchen
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Sources/Bonsplit/Internal/Styling/TabAppearanceColor.swift (1)
40-63: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winVerify swatch legibility for near-transparent custom colors.
Added rail color conversion that resolves colors in sRGB and adjusts brightness and saturation before returning an
NSColor. The alpha component is passed through unchanged from the input hex.railNSColorfeedscolorSwatchImageinTabItemView.swift,TabColorPickerView, andTabDragPreview. If a user picks a highly transparent custom background (a scenario the PR explicitly supports), the resulting swatch/rail dot stays nearly invisible even though brightness and saturation are boosted, because alpha is not floored.Confirm this matches the intended design (showing true transparency in the swatch) rather than an oversight. If swatches need to stay legible regardless of the underlying color's opacity, floor the alpha for swatch rendering, for example:
💡 Possible fix: floor alpha for rail/swatch rendering
let boostedBrightness = min(1, max(brightness, 0.62) + ((1 - brightness) * 0.28)) let boostedSaturation = saturation <= 0.08 ? saturation : min(1, saturation + ((1 - saturation) * 0.12)) + let boostedAlpha = max(alpha, 0.55) return NSColor( hue: hue, saturation: boostedSaturation, brightness: boostedBrightness, - alpha: alpha + alpha: boostedAlpha )🤖 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 `@Sources/Bonsplit/Internal/Styling/TabAppearanceColor.swift` around lines 40 - 63, Update railNSColor so the alpha used for rail and swatch rendering has an appropriate minimum floor, while preserving the existing sRGB conversion and brightness/saturation adjustments. Ensure highly transparent custom colors remain legible in colorSwatchImage consumers without changing the original color’s stored transparency.
🤖 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 `@Sources/Bonsplit/Internal/Views/TabColorPickerView.swift`:
- Around line 42-46: Update the ColorPicker in TabColorPickerView to set
supportsOpacity to true, enabling users to select and preserve the alpha
component supported by TabAppearanceColor’s `#RRGGBBAA` parsing and serialization.
In `@Sources/Bonsplit/Public/Types/TabColorPaletteEntry.swift`:
- Around line 3-6: Add a separate stable, nonlocalized palette entry key to
TabColorPaletteEntry and use it for Identifiable identity and Hashable semantics
instead of the user-facing name; localize the displayed label and update the
context-menu rendering to use that localized label rather than entry.name,
preserving distinct entries that share a translated name but have different hex
values.
---
Nitpick comments:
In `@Sources/Bonsplit/Internal/Styling/TabAppearanceColor.swift`:
- Around line 40-63: Update railNSColor so the alpha used for rail and swatch
rendering has an appropriate minimum floor, while preserving the existing sRGB
conversion and brightness/saturation adjustments. Ensure highly transparent
custom colors remain legible in colorSwatchImage consumers without changing the
original color’s stored transparency.
🪄 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 Plus
Run ID: 0aa67a90-acfe-4366-8210-e028877cdcde
📒 Files selected for processing (16)
README.mdSources/Bonsplit/Internal/Models/TabItem.swiftSources/Bonsplit/Internal/Styling/TabAppearanceColor.swiftSources/Bonsplit/Internal/Views/TabBarView.swiftSources/Bonsplit/Internal/Views/TabColorPickerView.swiftSources/Bonsplit/Internal/Views/TabContextMenuPresenter.swiftSources/Bonsplit/Internal/Views/TabDragPreview.swiftSources/Bonsplit/Internal/Views/TabIconCatalog.swiftSources/Bonsplit/Internal/Views/TabIconCustomizerView.swiftSources/Bonsplit/Internal/Views/TabItemView.swiftSources/Bonsplit/Public/BonsplitController.swiftSources/Bonsplit/Public/Types/Tab.swiftSources/Bonsplit/Public/Types/TabColorPaletteEntry.swiftSources/Bonsplit/Resources/en.lproj/Localizable.stringsSources/Bonsplit/Resources/ja.lproj/Localizable.stringsTests/BonsplitTests/BonsplitTests.swift
🚧 Files skipped from review as they are similar to previous changes (6)
- Sources/Bonsplit/Resources/ja.lproj/Localizable.strings
- Sources/Bonsplit/Resources/en.lproj/Localizable.strings
- Sources/Bonsplit/Public/Types/Tab.swift
- Sources/Bonsplit/Internal/Views/TabDragPreview.swift
- Sources/Bonsplit/Internal/Models/TabItem.swift
- Tests/BonsplitTests/BonsplitTests.swift
Comment on lines
+42
to
+46
| ColorPicker( | ||
| localized("tabColor.color", defaultValue: "Color"), | ||
| selection: $color, | ||
| supportsOpacity: false | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 2 'supportsOpacity|`#RRGGBBAA`|func hex' \
Sources/Bonsplit/Internal/Views/TabColorPickerView.swift \
Sources/Bonsplit/Internal/Styling/TabAppearanceColor.swift \
Tests/BonsplitTests/BonsplitTests.swiftRepository: manaflow-ai/bonsplit
Length of output: 1518
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- TabAppearanceColor.swift ---\n'
sed -n '1,140p' Sources/Bonsplit/Internal/Styling/TabAppearanceColor.swift | cat -n
printf '\n--- TabColorPickerView.swift ---\n'
sed -n '1,90p' Sources/Bonsplit/Internal/Views/TabColorPickerView.swift | cat -n
printf '\n--- Relevant tests references ---\n'
rg -n -C 3 'TabAppearanceColor|tabColor|ColorPicker|alpha|opacity|`#RRGGBBAA`|`#RRGGBBAA`|supportsOpacity' Tests Sources | sed -n '1,220p'Repository: manaflow-ai/bonsplit
Length of output: 249
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- TabAppearanceColor.swift ---'
sed -n '1,140p' Sources/Bonsplit/Internal/Styling/TabAppearanceColor.swift | cat -n
echo
echo '--- TabColorPickerView.swift ---'
sed -n '1,90p' Sources/Bonsplit/Internal/Views/TabColorPickerView.swift | cat -n
echo
echo '--- Relevant references ---'
rg -n -C 3 'TabAppearanceColor|tabColor|ColorPicker|alpha|opacity|`#RRGGBBAA`|`#RRGGBBAA`|supportsOpacity' Tests Sources | sed -n '1,260p'Repository: manaflow-ai/bonsplit
Length of output: 27501
Enable alpha for custom tab colors.
supportsOpacity: false removes the opacity control, but TabAppearanceColor parses and serializes #RRGGBBAA values. Set supportsOpacity to true so users can apply an explicit alpha via #RRGGBBAA.
Proposed fix
ColorPicker(
localized("tabColor.color", defaultValue: "Color"),
selection: $color,
- supportsOpacity: false
+ supportsOpacity: true
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ColorPicker( | |
| localized("tabColor.color", defaultValue: "Color"), | |
| selection: $color, | |
| supportsOpacity: false | |
| ) | |
| ColorPicker( | |
| localized("tabColor.color", defaultValue: "Color"), | |
| selection: $color, | |
| supportsOpacity: true | |
| ) |
🤖 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 `@Sources/Bonsplit/Internal/Views/TabColorPickerView.swift` around lines 42 -
46, Update the ColorPicker in TabColorPickerView to set supportsOpacity to true,
enabling users to select and preserve the alpha component supported by
TabAppearanceColor’s `#RRGGBBAA` parsing and serialization.
Comment on lines
+3
to
+6
| /// A named color shown in the tab color context-menu palette. | ||
| public struct TabColorPaletteEntry: Identifiable, Hashable, Sendable { | ||
| /// The user-facing color name. | ||
| public let name: String |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 5 \
'\bTabColorPaletteEntry\b|\bdefaultPalette\b|\bentry\.name\b|\bForEach\b' \
Sources/Bonsplit/Internal/Views Sources/Bonsplit/Resources TestsRepository: manaflow-ai/bonsplit
Length of output: 10320
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## TabColorPaletteEntry.swift"
cat -n Sources/Bonsplit/Public/Types/TabColorPaletteEntry.swift
echo
echo "## TabItemView context menu section"
sed -n '1720,1765p' Sources/Bonsplit/Internal/Views/TabItemView.swift | cat -n
echo
echo "## Localization/defaultPalette references"
rg -n -C 3 'Color Palette|palette|Palette|Color|defaultPalette|TAB_COLOR|tab.color|tabColor|colorPaletteProvider|entry\.name|entry\.hex|Hashable|Identifiable' Sources Tests | head -n 240Repository: manaflow-ai/bonsplit
Length of output: 24448
Use a separate stable entry key for identity and localization.
id is derived from name, while Name is user-facing and displayed as entry.name in the context menu. Identical labels can be supplied with different hex values, so hash equality and identifiable identity no longer match. Add a nonlocalized palette entry key for id/hashing, localize the displayed label, and render the menu title from the localized label instead of from entry.name.
🤖 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 `@Sources/Bonsplit/Public/Types/TabColorPaletteEntry.swift` around lines 3 - 6,
Add a separate stable, nonlocalized palette entry key to TabColorPaletteEntry
and use it for Identifiable identity and Hashable semantics instead of the
user-facing name; localize the displayed label and update the context-menu
rendering to use that localized label rather than entry.name, preserving
distinct entries that share a translated name but have different hex values.
This was referenced Aug 3, 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
1 participant
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
Tests
swift test(218 tests, 0 failures)swift buildSummary by CodeRabbit