Add configurable sidebar tint color with light/dark mode support#1465
Merged
lawrencecchen merged 1 commit intomanaflow-ai:mainfrom Mar 15, 2026
Merged
Add configurable sidebar tint color with light/dark mode support#1465lawrencecchen merged 1 commit intomanaflow-ai:mainfrom
lawrencecchen merged 1 commit intomanaflow-ai:mainfrom
Conversation
|
@rrva is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
📝 WalkthroughWalkthroughAdds per-appearance sidebar tint/background support: new AppStorage keys for light/dark tints and opacity, Settings UI color pickers and reset, GhosttyConfig parsing of Changes
Sequence DiagramsequenceDiagram
%% Components: User, Settings UI, AppStorage/UserDefaults, GhosttyConfig, Sidebar
participant User as "User"
participant Settings as "Settings UI"
participant Storage as "AppStorage / UserDefaults"
participant Config as "GhosttyConfig"
participant Sidebar as "Sidebar (ContentView)"
User->>Settings: choose Light/Dark tint & opacity
Settings->>Storage: write `sidebarTintHexLight` / `sidebarTintHexDark` / `sidebarTintOpacity`
Settings-->>User: show preview
Config->>Config: loadFromDisk() / parse `sidebar-background`
Config->>Config: resolveSidebarBackground(preferredColorScheme)
Config->>Storage: applySidebarAppearanceToUserDefaults()
Sidebar->>Storage: read tint keys + opacity
Sidebar->>Sidebar: pick resolvedHex (scheme-specific → fallback)
Sidebar->>Sidebar: compute NSColor (preserve alpha) and apply
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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 unit tests (beta)
📝 Coding Plan
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 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/GhosttyConfig.swift">
<violation number="1" location="Sources/GhosttyConfig.swift:169">
P2: Clear stale `sidebarTintHexLight`/`sidebarTintHexDark` values when no dual-mode color is resolved; otherwise old mode-specific values override new single-color config.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
Sources/cmuxApp.swift (1)
4037-4040: Avoid hardcoded sidebar tint defaults in reset action.Line 4039 and Line 4040 duplicate default values that already exist in sidebar preset/default logic. Centralizing avoids drift.
♻️ Suggested refactor
- sidebarTintHex = "#000000" - sidebarTintOpacity = 0.18 + let presetDefaults = SidebarPresetOption.nativeSidebar + sidebarTintHex = presetDefaults.tintHex + sidebarTintOpacity = presetDefaults.tintOpacity🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 4037 - 4040, The reset action is hardcoding sidebarTintHex and sidebarTintOpacity which duplicates central preset/default logic; instead, remove the explicit assignments to sidebarTintHex = "#000000" and sidebarTintOpacity = 0.18 and only clear per-variant overrides (set sidebarTintHexLight and sidebarTintHexDark to nil) so the existing centralized sidebar preset/default logic can supply the canonical defaults, or if there is an existing helper (e.g. applySidebarPresetDefaults() or similar), call that helper from the reset action rather than assigning literal values.
🤖 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/cmuxApp.swift`:
- Around line 3111-3114: The resetAllSettings() routine currently doesn't clear
the new sidebar tint AppStorage keys (sidebarTintHex, sidebarTintHexLight,
sidebarTintHexDark, sidebarTintOpacity); update resetAllSettings() to explicitly
reset these persisted settings by setting sidebarTintHex back to the default
("#000000"), sidebarTintHexLight and sidebarTintHexDark to nil (or their
defaults), and sidebarTintOpacity back to 0.18 (or its default), or remove those
keys from UserDefaults within that function so the Reset All Settings flow fully
restores the global defaults for the sidebar tint customization.
In `@Sources/ContentView.swift`:
- Around line 12577-12585: The per-theme override logic (resolvedHex) can return
an invalid hex and then NSColor(hex:) falls back to .black; change the resolver
so that when choosing sidebarTintHexLight or sidebarTintHexDark you first
validate that NSColor(hex: candidate) != nil (and non-empty) and only return the
candidate if valid, otherwise fall back to the shared sidebarTintHex; keep the
final tintColor construction using (NSColor(hex: resolvedHex) ??
.black).withAlphaComponent(sidebarTintOpacity) so opacity behavior is unchanged.
In `@Sources/GhosttyConfig.swift`:
- Around line 297-300: The case handling "sidebar-tint-opacity" currently
assigns any parsed Double to sidebarTintOpacity; clamp the parsed value to the
valid range 0.0...1.0 before assigning (or reject/ignore values outside that
range). Locate the switch case for "sidebar-tint-opacity" and replace the direct
assignment in that branch with a clamp operation (e.g., let clamped =
min(max(opacity, 0.0), 1.0); sidebarTintOpacity = clamped) so negative or >1
values cannot be persisted/used.
- Around line 167-184: When syncing sidebar appearance in
applySidebarAppearanceToUserDefaults(), ensure stale override keys are removed
to avoid old values winning: when setting a single sidebarBackground (the color)
call UserDefaults.standard.removeObject(forKey:) for "sidebarTintHexLight" and
"sidebarTintHexDark" so any previous light/dark overrides are cleared;
conversely, when setting sidebarBackgroundLight or sidebarBackgroundDark, remove
the single-key "sidebarTintHex" to prevent it from overriding the per-theme
values; keep the existing opacity writes as-is.
---
Nitpick comments:
In `@Sources/cmuxApp.swift`:
- Around line 4037-4040: The reset action is hardcoding sidebarTintHex and
sidebarTintOpacity which duplicates central preset/default logic; instead,
remove the explicit assignments to sidebarTintHex = "#000000" and
sidebarTintOpacity = 0.18 and only clear per-variant overrides (set
sidebarTintHexLight and sidebarTintHexDark to nil) so the existing centralized
sidebar preset/default logic can supply the canonical defaults, or if there is
an existing helper (e.g. applySidebarPresetDefaults() or similar), call that
helper from the reset action rather than assigning literal values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 450a024d-2305-4c91-b040-061fc5f87ee6
📒 Files selected for processing (3)
Sources/ContentView.swiftSources/GhosttyConfig.swiftSources/cmuxApp.swift
rrva
force-pushed
the
sidebar-background-config
branch
2 times, most recently
from
e89e307 to
5df5e35
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
Sources/cmuxApp.swift (1)
3122-3125:⚠️ Potential issue | 🟡 Minor
Reset All Settingsstill misses these new persisted keys.
resetAllSettings()around Line 4570 doesn't restoresidebarTintHex,sidebarTintHexLight,sidebarTintHexDark, orsidebarTintOpacity, so the global reset flow can leave sidebar tint customization behind.🛠️ Proposed fix (outside this hunk)
sidebarShowPorts = true sidebarShowLog = true sidebarShowProgress = true sidebarShowMetadata = true + sidebarTintHex = "#000000" + sidebarTintHexLight = nil + sidebarTintHexDark = nil + sidebarTintOpacity = 0.18 showOpenAccessConfirmation = false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 3122 - 3125, resetAllSettings() does not clear the new AppStorage keys for sidebar tinting, leaving user-customized sidebar colors after a global reset; update resetAllSettings() to explicitly restore sidebarTintHex, sidebarTintHexLight, sidebarTintHexDark, and sidebarTintOpacity to their default values (e.g., "#000000" for sidebarTintHex, nil for the Light/Dark variants, and 0.18 for sidebarTintOpacity) by setting those AppStorage-backed properties inside resetAllSettings() so the reset flow removes any custom sidebar tinting.Sources/GhosttyConfig.swift (1)
313-316:⚠️ Potential issue | 🟡 MinorClamp
sidebar-tint-opacitybefore storing it.Line 314 still accepts any
Double; negative or> 1values can leak into persisted alpha and downstream UI state.🛠️ Proposed fix
case "sidebar-tint-opacity": if let opacity = Double(value) { - sidebarTintOpacity = opacity + sidebarTintOpacity = min(1.0, max(0.0, opacity)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyConfig.swift` around lines 313 - 316, The "sidebar-tint-opacity" branch in GhosttyConfig.swift currently assigns any Double to sidebarTintOpacity; clamp the parsed opacity to the 0...1 range before storing to prevent negative or >1 values from persisting or affecting UI state—locate the case "sidebar-tint-opacity" and replace the direct assignment (where opacity is parsed) with a clamped value (e.g., using min/max or a range clamp) and then assign sidebarTintOpacity.
🤖 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/cmuxApp.swift`:
- Around line 4027-4037: The Slider in SettingsCardRow (Slider(value:
$sidebarTintOpacity, in: 0...0.7)) is capped at 0.7 while GhosttyConfig.parse
accepts higher values (e.g., 0.85), causing a mismatch; fix by making the ranges
consistent: either increase the Slider max to match the config parser's allowed
max (e.g., 0...1 or the specific max used by GhosttyConfig.parse) so
$sidebarTintOpacity can reach values parsed from config, or clamp/validate the
parsed value inside GhosttyConfig.parse to a 0.7 ceiling and update docs—adjust
the Slider range or the parsing/clamping logic accordingly and ensure both
SettingsCardRow/$sidebarTintOpacity and GhosttyConfig.parse use the same max.
---
Duplicate comments:
In `@Sources/cmuxApp.swift`:
- Around line 3122-3125: resetAllSettings() does not clear the new AppStorage
keys for sidebar tinting, leaving user-customized sidebar colors after a global
reset; update resetAllSettings() to explicitly restore sidebarTintHex,
sidebarTintHexLight, sidebarTintHexDark, and sidebarTintOpacity to their default
values (e.g., "#000000" for sidebarTintHex, nil for the Light/Dark variants, and
0.18 for sidebarTintOpacity) by setting those AppStorage-backed properties
inside resetAllSettings() so the reset flow removes any custom sidebar tinting.
In `@Sources/GhosttyConfig.swift`:
- Around line 313-316: The "sidebar-tint-opacity" branch in GhosttyConfig.swift
currently assigns any Double to sidebarTintOpacity; clamp the parsed opacity to
the 0...1 range before storing to prevent negative or >1 values from persisting
or affecting UI state—locate the case "sidebar-tint-opacity" and replace the
direct assignment (where opacity is parsed) with a clamped value (e.g., using
min/max or a range clamp) and then assign sidebarTintOpacity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9be67cf4-b6ca-4c80-80bb-86d997292144
📒 Files selected for processing (3)
Sources/ContentView.swiftSources/GhosttyConfig.swiftSources/cmuxApp.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/ContentView.swift
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Sources/cmuxApp.swift (1)
4032-4033:⚠️ Potential issue | 🟠 MajorIncrease the tint opacity slider max (or clamp parser/docs to 0.7).
At Line 4032, the UI caps at
0.7, but this feature accepts higher opacity values (including the PR’s own0.85example), so loaded config can’t be edited faithfully in Settings.🛠️ Suggested fix
- Slider(value: $sidebarTintOpacity, in: 0...0.7) + Slider(value: $sidebarTintOpacity, in: 0...1.0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 4032 - 4033, The Slider for sidebarTintOpacity in cmuxApp.swift is capped at 0.7 but configs (and the PR example 0.85) allow higher values; update the Slider range to allow the full accepted opacity (e.g., change the range from 0...0.7 to 0...1 or to a shared max constant like maxSidebarTintOpacity) so loaded values can be edited faithfully, and ensure any config parsing/clamping uses the same max constant (e.g., references to sidebarTintOpacity, Slider(value: $sidebarTintOpacity, in: ...), and any loader/clamp logic) so UI and parsing 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/cmuxApp.swift`:
- Around line 3981-4057: The referenced localization keys used in
SettingsSectionHeader and SettingsCardRow (settings.section.sidebarAppearance,
settings.sidebarAppearance.tintColorLight,
settings.sidebarAppearance.tintColorLight.subtitle,
settings.sidebarAppearance.tintColorLight.picker,
settings.sidebarAppearance.tintColorDark,
settings.sidebarAppearance.tintColorDark.subtitle,
settings.sidebarAppearance.tintColorDark.picker,
settings.sidebarAppearance.tintOpacity,
settings.sidebarAppearance.tintOpacity.subtitle,
settings.sidebarAppearance.reset, settings.sidebarAppearance.reset.subtitle,
settings.sidebarAppearance.reset.button) are missing from the strings catalog;
add each key to the Localizable.xcstrings resource with proper English and
Japanese translations, matching the defaultValue text used in the code as the
English entries and appropriate Japanese equivalents, then save and re-run your
localization validation/build to ensure SwiftUI picks up the new entries.
In `@Sources/GhosttyConfig.swift`:
- Around line 32-35: The file's top comment about sidebar behavior contradicts
the implemented logic: code only writes sidebar tint values when the
"sidebar-background" config key exists and always writes sidebarTintOpacity (it
does not clear tint hex keys when "sidebar-background" is absent). Update the
documentation/comments around the sidebar settings (the block describing
"sidebar-background" and the nearby comments around the code that writes
sidebarTintOpacity and tint hex keys) to state that writes occur only if
"sidebar-background" exists, that sidebarTintOpacity is always written when
present, and that tint hex keys are not cleared when "sidebar-background" is
absent (so the UI controls remain authoritative). Ensure mentions of
"sidebar-background", "sidebarTintOpacity" and the sidebar tint hex keys match
the runtime behavior.
---
Duplicate comments:
In `@Sources/cmuxApp.swift`:
- Around line 4032-4033: The Slider for sidebarTintOpacity in cmuxApp.swift is
capped at 0.7 but configs (and the PR example 0.85) allow higher values; update
the Slider range to allow the full accepted opacity (e.g., change the range from
0...0.7 to 0...1 or to a shared max constant like maxSidebarTintOpacity) so
loaded values can be edited faithfully, and ensure any config parsing/clamping
uses the same max constant (e.g., references to sidebarTintOpacity,
Slider(value: $sidebarTintOpacity, in: ...), and any loader/clamp logic) so UI
and parsing remain consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c8f532e4-8789-46b5-81b4-22e0cc6fba20
📒 Files selected for processing (3)
Sources/ContentView.swiftSources/GhosttyConfig.swiftSources/cmuxApp.swift
rrva
force-pushed
the
sidebar-background-config
branch
2 times, most recently
from
fe74d4e to
be4db6c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Sources/cmuxApp.swift (1)
4031-4034:⚠️ Potential issue | 🟠 MajorOpacity slider range is inconsistent with parser-accepted values.
Line 4031 caps UI input at
0...0.7, butGhosttyConfigaccepts/clampssidebar-tint-opacityto0...1. Values like0.85can load from config but cannot be edited faithfully in Settings.🛠️ Suggested fix
- Slider(value: $sidebarTintOpacity, in: 0...0.7) + Slider(value: $sidebarTintOpacity, in: 0...1.0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 4031 - 4034, The UI slider for sidebar opacity limits sidebarTintOpacity to 0...0.7 while GhosttyConfig accepts 0...1, causing loaded values like 0.85 to be uneditable; update the Slider range bound in the Settings UI (the Slider using the sidebarTintOpacity binding) to use 0...1 so the UI can represent and edit the full parser-accepted range for sidebar-tint-opacity and keep the existing percentage Text formatting unchanged.
🤖 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/GhosttyConfig.swift`:
- Around line 182-185: The current code in the sidebarBackground handling
unconditionally writes a default opacity (0.7) when sidebarTintOpacity is nil,
which unintentionally overwrites stored settings; change the logic in the block
that handles sidebarBackground (referencing sidebarBackground,
sidebarTintOpacity and the defaults.set calls for "sidebarTintHex" and
"sidebarTintOpacity") so that you always set the hex value, but only call
defaults.set(..., forKey: "sidebarTintOpacity") when sidebarTintOpacity is
non-nil (e.g., if let opacity = sidebarTintOpacity { defaults.set(opacity,
forKey: "sidebarTintOpacity") }) so the existing stored/AppStorage value is not
clobbered when the config omits sidebar-tint-opacity.
---
Duplicate comments:
In `@Sources/cmuxApp.swift`:
- Around line 4031-4034: The UI slider for sidebar opacity limits
sidebarTintOpacity to 0...0.7 while GhosttyConfig accepts 0...1, causing loaded
values like 0.85 to be uneditable; update the Slider range bound in the Settings
UI (the Slider using the sidebarTintOpacity binding) to use 0...1 so the UI can
represent and edit the full parser-accepted range for sidebar-tint-opacity and
keep the existing percentage Text formatting unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6954ca0b-e01c-4f6e-a6d8-f7366975621e
📒 Files selected for processing (3)
Sources/ContentView.swiftSources/GhosttyConfig.swiftSources/cmuxApp.swift
rrva
force-pushed
the
sidebar-background-config
branch
from
be4db6c to
93d584a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Sources/cmuxApp.swift (2)
3121-3124: Centralize sidebar tint defaults to prevent drift.
"#000000"and0.18are repeated across AppStorage initialization and reset flows. A shared default constant would prevent accidental mismatch (especially since config sync logic currently uses a different fallback opacity path inGhosttyConfig).♻️ Suggested refactor
+private enum SidebarTintDefaults { + static let hex = "#000000" + static let opacity = 0.18 +} ... - `@AppStorage`("sidebarTintHex") private var sidebarTintHex = "#000000" + `@AppStorage`("sidebarTintHex") private var sidebarTintHex = SidebarTintDefaults.hex ... - `@AppStorage`("sidebarTintOpacity") private var sidebarTintOpacity = 0.18 + `@AppStorage`("sidebarTintOpacity") private var sidebarTintOpacity = SidebarTintDefaults.opacity ... - sidebarTintHex = "#000000" - sidebarTintOpacity = 0.18 + sidebarTintHex = SidebarTintDefaults.hex + sidebarTintOpacity = SidebarTintDefaults.opacity ... - sidebarTintHex = "#000000" - sidebarTintOpacity = 0.18 + sidebarTintHex = SidebarTintDefaults.hex + sidebarTintOpacity = SidebarTintDefaults.opacityAlso applies to: 4050-4051, 4621-4624
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 3121 - 3124, The sidebar tint default values are duplicated; define shared constants (e.g., SIDEBAR_TINT_HEX_DEFAULT and SIDEBAR_TINT_OPACITY_DEFAULT) and replace the literal "#000000" and 0.18 in the AppStorage declarations (sidebarTintHex, sidebarTintHexLight, sidebarTintHexDark, sidebarTintOpacity) and in any reset/config-sync code paths (including GhosttyConfig fallback) to use these constants so defaults stay centralized and consistent across initialization and reset flows.
2427-2428: Prefer clearing@AppStoragebindings directly instead of rawUserDefaultscalls.Using the wrapped properties avoids key string duplication and keeps this path consistent with the rest of the view state writes.
♻️ Suggested refactor
- UserDefaults.standard.removeObject(forKey: "sidebarTintHexLight") - UserDefaults.standard.removeObject(forKey: "sidebarTintHexDark") + sidebarTintHexLight = nil + sidebarTintHexDark = nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 2427 - 2428, Replace the raw UserDefaults.removeObject calls with assignments to the corresponding `@AppStorage-wrapped` properties so key strings aren't duplicated; instead of UserDefaults.standard.removeObject(forKey: "sidebarTintHexLight") and ...("sidebarTintHexDark"), set the `@AppStorage` properties sidebarTintHexLight = nil and sidebarTintHexDark = nil (or to their empty/default values if non-optional) in the same function/block where those removes appear.
🤖 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/cmuxApp.swift`:
- Around line 3121-3124: The sidebar tint default values are duplicated; define
shared constants (e.g., SIDEBAR_TINT_HEX_DEFAULT and
SIDEBAR_TINT_OPACITY_DEFAULT) and replace the literal "#000000" and 0.18 in the
AppStorage declarations (sidebarTintHex, sidebarTintHexLight,
sidebarTintHexDark, sidebarTintOpacity) and in any reset/config-sync code paths
(including GhosttyConfig fallback) to use these constants so defaults stay
centralized and consistent across initialization and reset flows.
- Around line 2427-2428: Replace the raw UserDefaults.removeObject calls with
assignments to the corresponding `@AppStorage-wrapped` properties so key strings
aren't duplicated; instead of UserDefaults.standard.removeObject(forKey:
"sidebarTintHexLight") and ...("sidebarTintHexDark"), set the `@AppStorage`
properties sidebarTintHexLight = nil and sidebarTintHexDark = nil (or to their
empty/default values if non-optional) in the same function/block where those
removes appear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a9e76d77-e11f-4ce4-b105-52dbca645936
📒 Files selected for processing (4)
Resources/Localizable.xcstringsSources/ContentView.swiftSources/GhosttyConfig.swiftSources/cmuxApp.swift
✅ Files skipped from review due to trivial changes (1)
- Resources/Localizable.xcstrings
rrva
force-pushed
the
sidebar-background-config
branch
3 times, most recently
from
7307fd5 to
e5baef7
Compare
Contributor
Author
|
Re: review 3950173460 — both nitpicks addressed:
|
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Contributor
Author
Demo Video |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
cmuxTests/GhosttyConfigTests.swift (1)
1680-1736: Consider centralizing sidebar defaults keys in test-local constants.The same key literals are repeated across tests; extracting constants will reduce drift/typo risk.
♻️ Optional cleanup
+ private enum SidebarDefaultsKey { + static let hex = "sidebarTintHex" + static let hexLight = "sidebarTintHexLight" + static let hexDark = "sidebarTintHexDark" + static let opacity = "sidebarTintOpacity" + }- let testKey = "sidebarTintHex" + let testKey = SidebarDefaultsKey.hex- let keys = ["sidebarTintHex", "sidebarTintHexLight", "sidebarTintHexDark"] + let keys = [SidebarDefaultsKey.hex, SidebarDefaultsKey.hexLight, SidebarDefaultsKey.hexDark]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/GhosttyConfigTests.swift` around lines 1680 - 1736, Extract repeated UserDefaults key strings ("sidebarTintHex", "sidebarTintHexLight", "sidebarTintHexDark") into test-local constants (e.g., let kSidebarTint = "sidebarTintHex", kSidebarTintLight = "sidebarTintHexLight", kSidebarTintDark = "sidebarTintHexDark") and use those constants in the tests testApplyToUserDefaultsWritesHexWhenConfigSet, testApplyToUserDefaultsClearsStaleKeysOnSwitchFromDualToSingle and the earlier test that uses testKey; update the keys array, defaults.set/get calls, and restore loop to reference the constants so the literals are defined once and reduce duplication/typo risk.Sources/cmuxApp.swift (1)
3199-3221: Consider deduplicating the light/dark tint Binding implementations.These two bindings are structurally identical except for the target override key; a tiny helper would reduce repetition.
♻️ Proposed refactor
- private var settingsSidebarTintLightBinding: Binding<Color> { - Binding( - get: { - Color(nsColor: NSColor(hex: sidebarTintHexLight ?? sidebarTintHex) ?? .black) - }, - set: { newColor in - let nsColor = NSColor(newColor) - sidebarTintHexLight = nsColor.hexString() - } - ) - } - - private var settingsSidebarTintDarkBinding: Binding<Color> { - Binding( - get: { - Color(nsColor: NSColor(hex: sidebarTintHexDark ?? sidebarTintHex) ?? .black) - }, - set: { newColor in - let nsColor = NSColor(newColor) - sidebarTintHexDark = nsColor.hexString() - } - ) - } + private func settingsSidebarTintBinding(_ overrideHex: Binding<String?>) -> Binding<Color> { + Binding( + get: { + Color(nsColor: NSColor(hex: overrideHex.wrappedValue ?? sidebarTintHex) ?? .black) + }, + set: { newColor in + overrideHex.wrappedValue = NSColor(newColor).hexString() + } + ) + } + + private var settingsSidebarTintLightBinding: Binding<Color> { + settingsSidebarTintBinding($sidebarTintHexLight) + } + + private var settingsSidebarTintDarkBinding: Binding<Color> { + settingsSidebarTintBinding($sidebarTintHexDark) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 3199 - 3221, The two bindings settingsSidebarTintLightBinding and settingsSidebarTintDarkBinding are identical except for which stored-hex they read/write (sidebarTintHexLight vs sidebarTintHexDark), so extract a small helper that builds a Binding<Color> from two closures (one to get the optional override hex and one to set it). Implement e.g. a private func makeSidebarTintBinding(getOverride: `@escaping` () -> String?, setOverride: `@escaping` (String?) -> Void) -> Binding<Color> that performs the Color(nsColor: NSColor(hex: override ?? sidebarTintHex) ?? .black) get and converts Color -> NSColor -> hex string in the set; then replace settingsSidebarTintLightBinding and settingsSidebarTintDarkBinding to call makeSidebarTintBinding passing closures that get/set sidebarTintHexLight and sidebarTintHexDark respectively.Sources/GhosttyConfig.swift (1)
144-160: Reset resolved sidebar fields before recomputing.At Line 145, early return and conditional assignments can leave stale
sidebarBackground,sidebarBackgroundLight, andsidebarBackgroundDarkwhen this method is called more than once on the same instance.♻️ Proposed refactor
mutating func resolveSidebarBackground(preferredColorScheme: ColorSchemePreference) { - guard let raw = rawSidebarBackground else { return } + sidebarBackground = nil + sidebarBackgroundLight = nil + sidebarBackgroundDark = nil + + guard let raw = rawSidebarBackground, + !raw.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty else { return } let lightResolved = Self.resolveThemeName(from: raw, preferredColorScheme: .light) let darkResolved = Self.resolveThemeName(from: raw, preferredColorScheme: .dark) let hasDualMode = lightResolved != darkResolved🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyConfig.swift` around lines 144 - 160, In resolveSidebarBackground(preferredColorScheme:), clear/reset sidebarBackground, sidebarBackgroundLight, and sidebarBackgroundDark at the start (and when rawSidebarBackground is nil) so repeated calls don't leave stale values; then compute hasDualMode and only assign sidebarBackgroundLight/sidebarBackgroundDark when dual-mode, otherwise ensure those dual fields remain nil, and finally set sidebarBackground from the resolved color if available — refer to the function resolveSidebarBackground and the properties rawSidebarBackground, sidebarBackground, sidebarBackgroundLight, and sidebarBackgroundDark.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxTests/GhosttyConfigTests.swift`:
- Around line 1738-1756: The test
testApplyToUserDefaultsOnlyWritesOpacityWhenExplicit only snapshots
"sidebarTintHex" and "sidebarTintOpacity" but
applySidebarAppearanceToUserDefaults() can also clear "sidebarTintHexLight" and
"sidebarTintHexDark"; expand the keys array in that test to include
"sidebarTintHexLight" and "sidebarTintHexDark", capture their originals (like
the existing originals map) and ensure the defer restore loop resets those keys
via restoreDefaultsValue so state isn't leaked to other tests.
In `@Sources/GhosttyConfig.swift`:
- Around line 182-184: The current code only sets
defaults.set(color.hexString(), forKey: "sidebarTintHex") when sidebarBackground
is non-nil, leaving stale values when sidebarBackground is nil; update the
handling around sidebarBackground in Sources/GhosttyConfig.swift so that when
sidebarBackground is nil you clear the stored value by calling
defaults.removeObject(forKey: "sidebarTintHex") (or set nil/empty appropriately)
instead of leaving the old hex; locate the block that references
sidebarBackground and defaults to apply this change.
---
Nitpick comments:
In `@cmuxTests/GhosttyConfigTests.swift`:
- Around line 1680-1736: Extract repeated UserDefaults key strings
("sidebarTintHex", "sidebarTintHexLight", "sidebarTintHexDark") into test-local
constants (e.g., let kSidebarTint = "sidebarTintHex", kSidebarTintLight =
"sidebarTintHexLight", kSidebarTintDark = "sidebarTintHexDark") and use those
constants in the tests testApplyToUserDefaultsWritesHexWhenConfigSet,
testApplyToUserDefaultsClearsStaleKeysOnSwitchFromDualToSingle and the earlier
test that uses testKey; update the keys array, defaults.set/get calls, and
restore loop to reference the constants so the literals are defined once and
reduce duplication/typo risk.
In `@Sources/cmuxApp.swift`:
- Around line 3199-3221: The two bindings settingsSidebarTintLightBinding and
settingsSidebarTintDarkBinding are identical except for which stored-hex they
read/write (sidebarTintHexLight vs sidebarTintHexDark), so extract a small
helper that builds a Binding<Color> from two closures (one to get the optional
override hex and one to set it). Implement e.g. a private func
makeSidebarTintBinding(getOverride: `@escaping` () -> String?, setOverride:
`@escaping` (String?) -> Void) -> Binding<Color> that performs the Color(nsColor:
NSColor(hex: override ?? sidebarTintHex) ?? .black) get and converts Color ->
NSColor -> hex string in the set; then replace settingsSidebarTintLightBinding
and settingsSidebarTintDarkBinding to call makeSidebarTintBinding passing
closures that get/set sidebarTintHexLight and sidebarTintHexDark respectively.
In `@Sources/GhosttyConfig.swift`:
- Around line 144-160: In resolveSidebarBackground(preferredColorScheme:),
clear/reset sidebarBackground, sidebarBackgroundLight, and sidebarBackgroundDark
at the start (and when rawSidebarBackground is nil) so repeated calls don't
leave stale values; then compute hasDualMode and only assign
sidebarBackgroundLight/sidebarBackgroundDark when dual-mode, otherwise ensure
those dual fields remain nil, and finally set sidebarBackground from the
resolved color if available — refer to the function resolveSidebarBackground and
the properties rawSidebarBackground, sidebarBackground, sidebarBackgroundLight,
and sidebarBackgroundDark.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 69304419-033f-4932-984c-f6f55bc9c529
📒 Files selected for processing (6)
CHANGELOG.mdResources/Localizable.xcstringsSources/ContentView.swiftSources/GhosttyConfig.swiftSources/cmuxApp.swiftcmuxTests/GhosttyConfigTests.swift
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
✅ Actions performedReviews resumed. |
- Config: sidebar-background supports plain hex (#336699) or light/dark syntax (light:#fbf3db,dark:#103c48) - Config: sidebar-tint-opacity overrides tint opacity - Settings UI: per-scheme color pickers, opacity slider (0-70%), reset - SidebarBackdrop resolves light/dark hex based on @Environment colorScheme - applySidebarAppearanceToUserDefaults guards on rawSidebarBackground presence so UI picks survive appearance toggles when no config is set - Stale light/dark UserDefaults keys cleared when config switches from dual-mode to single or sidebar-background is removed - applyPreset() and Reset Tint clear per-scheme overrides - Debug snapshot (combinedPayload + copySidebarConfig) includes new keys - ColorPicker labels use String(localized:) per localization policy - Opacity slider capped at 0.7 to match debug view vibrancy constraint Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
rrva
force-pushed
the
sidebar-background-config
branch
from
e5baef7 to
adbe777
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/GhosttyConfig.swift (1)
162-190: Prefer shared constants for sidebar UserDefaults keys.These key literals are now spread across multiple writes/removals. Reusing a single key namespace (same source as
@AppStorage) would reduce drift risk during future renames.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyConfig.swift` around lines 162 - 190, Replace the scattered string literals in applySidebarAppearanceToUserDefaults with shared constants from the same key namespace used by your `@AppStorage` declarations (or add a single source-of-truth like SidebarKeys/sidebarUserDefaultsKeys), then use those constants for all get/set/remove calls for "sidebarTintOpacity", "sidebarTintHexLight", "sidebarTintHexDark", and "sidebarTintHex" to avoid drift; update applySidebarAppearanceToUserDefaults to reference the new constants (and remove the hardcoded strings) so all reads/writes use the unified keys.
🤖 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/GhosttyConfig.swift`:
- Around line 162-190: Replace the scattered string literals in
applySidebarAppearanceToUserDefaults with shared constants from the same key
namespace used by your `@AppStorage` declarations (or add a single source-of-truth
like SidebarKeys/sidebarUserDefaultsKeys), then use those constants for all
get/set/remove calls for "sidebarTintOpacity", "sidebarTintHexLight",
"sidebarTintHexDark", and "sidebarTintHex" to avoid drift; update
applySidebarAppearanceToUserDefaults to reference the new constants (and remove
the hardcoded strings) so all reads/writes use the unified keys.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5ee169dd-51db-4d02-86f5-f47771cbff77
📒 Files selected for processing (6)
CHANGELOG.mdResources/Localizable.xcstringsSources/ContentView.swiftSources/GhosttyConfig.swiftSources/cmuxApp.swiftcmuxTests/GhosttyConfigTests.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- cmuxTests/GhosttyConfigTests.swift
- CHANGELOG.md
Contributor
Author
|
Re: review 3950235040 — the existing sidebar keys ( |
5 tasks
bn-l
pushed a commit
to bn-l/cmux
that referenced
this pull request
Apr 3, 2026
…aflow-ai#1465) - Config: sidebar-background supports plain hex (#336699) or light/dark syntax (light:#fbf3db,dark:#103c48) - Config: sidebar-tint-opacity overrides tint opacity - Settings UI: per-scheme color pickers, opacity slider (0-70%), reset - SidebarBackdrop resolves light/dark hex based on @Environment colorScheme - applySidebarAppearanceToUserDefaults guards on rawSidebarBackground presence so UI picks survive appearance toggles when no config is set - Stale light/dark UserDefaults keys cleared when config switches from dual-mode to single or sidebar-background is removed - applyPreset() and Reset Tint clear per-scheme overrides - Debug snapshot (combinedPayload + copySidebarConfig) includes new keys - ColorPicker labels use String(localized:) per localization policy - Opacity slider capped at 0.7 to match debug view vibrancy constraint Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
2 participants
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
sidebar-backgroundandsidebar-tint-opacityconfig keys to the Ghostty config parserlight:/dark:prefix syntax (e.g.sidebar-background = light:#ece3cc,dark:#184956)SidebarBackdropautomatically switches tint color when macOS appearance changesCloses #1462
Config file usage
Settings go in the cmux-specific config (
~/Library/Application Support/com.cmuxterm.app/config):Or a single color for both modes:
Changes
Sources/GhosttyConfig.swiftsidebar-background(withlight:/dark:syntax) andsidebar-tint-opacity, sync to UserDefaultsSources/ContentView.swiftSidebarBackdropobservescolorSchemeand resolves light/dark-specific tint hexSources/cmuxApp.swiftSettingsViewwith light/dark color pickers, opacity slider, resetTesting
Demo Video
Screen.Recording.2026-03-15.at.15.42.19.mov
Checklist
Summary by CodeRabbit