Skip to content

feat: auto-match sidebar background to Ghostty terminal theme#2232

Closed
christi4nity wants to merge 1 commit intomanaflow-ai:mainfrom
christi4nity:feat/sidebar-theme-match
Closed

feat: auto-match sidebar background to Ghostty terminal theme#2232
christi4nity wants to merge 1 commit intomanaflow-ai:mainfrom
christi4nity:feat/sidebar-theme-match

Conversation

@christi4nity
Copy link
Copy Markdown

@christi4nity christi4nity commented Mar 27, 2026

Summary

  • When no explicit sidebar-background is configured, the sidebar now automatically uses the terminal's Ghostty theme background color and opacity
  • Bypasses the glass/blur material (sets material to none) so the sidebar color matches the terminal exactly — no visual distortion from NSVisualEffectView
  • Keeps sidebar in sync on runtime theme changes via the existing refreshGhosttyAppearanceConfig notification flow
  • Respects manual Settings UI overrides: if the user has chosen a non-default material (e.g. HUD Window), auto-sync is skipped

Changes

File Change
Sources/ContentView.swift Fix SidebarBackdrop to render solid Color when material is none
Sources/GhosttyConfig.swift Fall back to terminal background + settings protection guard
Sources/WorkspaceContentView.swift Runtime sidebar sync after theme changes
cmuxTests/GhosttyConfigTests.swift Updated + 2 new tests

Test plan

  • Verify sidebar matches terminal background with no sidebar-background in Ghostty config
  • Verify sidebar stays in sync when changing Ghostty theme at runtime
  • Verify explicit sidebar-background in config still works
  • Verify manual Settings UI material choice is not overwritten
  • Verify unit tests pass

Summary by cubic

Auto-match the sidebar background to the active Ghostty terminal theme when no sidebar color is set. Uses a solid color (material set to none) for an exact match, stays in sync on theme changes, and respects manual material choices.

  • New Features
    • Falls back to terminal background color and opacity; removes glass/blur by setting material to none.
    • Syncs on runtime theme changes; skips auto-sync if the user selected a non-default material in Settings.
    • SidebarBackdrop now renders a solid color when material is none; tests cover fallback and override cases.

Written for commit 472f08f. Summary will update on new commits.

Summary by CodeRabbit

Release Notes

  • Bug Fixes

    • Fixed sidebar appearing empty when no custom material is configured; now displays a solid tint color based on the terminal theme.
  • Improvements

    • Sidebar appearance now automatically synchronizes with the active terminal theme when no custom background is set, while preserving user-chosen custom material settings.

When no explicit `sidebar-background` is configured, the sidebar now
automatically uses the terminal's background color and opacity with
material set to `none`, bypassing the glass/blur material that distorts
the theme color. Runtime theme changes keep the sidebar in sync via
the existing `refreshGhosttyAppearanceConfig` notification flow.
Respects manual Settings UI overrides by skipping auto-sync when a
non-default material has been explicitly chosen.
@vercel
Copy link
Copy Markdown

vercel bot commented Mar 27, 2026

@christi4nity is attempting to deploy a commit to the Manaflow Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 27, 2026

📝 Walkthrough

Walkthrough

Updated sidebar appearance logic to auto-sync from terminal theme when no custom background is configured. When rawSidebarBackground is absent, the application now seeds sidebar tint and material settings from the active terminal's background color via UserDefaults, while preserving previously chosen material settings.

Changes

Cohort / File(s) Summary
Sidebar Rendering
Sources/ContentView.swift
Modified SidebarBackdrop to always render a solid tint when no sidebar material is configured, instead of rendering nothing.
Configuration Sync
Sources/GhosttyConfig.swift, Sources/WorkspaceContentView.swift
applySidebarAppearanceToUserDefaults() now auto-syncs sidebar tint/material from terminal theme when rawSidebarBackground is absent, respecting user-chosen non-default materials. refreshGhosttyAppearanceConfig() triggers this sync when needed.
Tests
cmuxTests/GhosttyConfigTests.swift
Expanded test coverage with three new tests validating auto-sync behavior: fallback to terminal background, skipping sync on custom materials, and syncing when using default material.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Poem

A sidebar once hidden in void, now shines so bright,
With theme-tinted colors, perfectly aligned just right,
The rabbit hops gleefully—no more config despair,
UserDefaults synced with background care,
And custom choices always respected fair! 🐰✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main feature change: auto-matching sidebar background to Ghostty terminal theme.
Description check ✅ Passed The description covers the required sections (Summary, Testing, Checklist) with sufficient detail about what changed and why, though the Testing section uses a checkbox format rather than narrative answers.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🤖 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 2484-2515: The test
testApplyToUserDefaultsFallsBackToTerminalBackground asserts the sidebar tint
hex as "#1e1e2e" but the code stores backgroundColor.hexString() in UserDefaults
which returns an uppercase hex; update the expectation to the uppercase form
(e.g. "#1E1E2E") to match the value written by
GhosttyConfig.applySidebarAppearanceToUserDefaults (and the NSColor.hexString
implementation).

In `@Sources/GhosttyConfig.swift`:
- Line 181: The saved sidebar opacity currently uses "sidebarTintOpacity ??
backgroundOpacity" which can write an out-of-range backgroundOpacity; update
GhosttyConfig.swift so the value persisted via defaults.set(...) is clamped to
the valid range (e.g., 0.0...1.0). Specifically, when calling defaults.set(...)
for sidebarTintOpacity, compute the fallback as a clampedBackgroundOpacity
(apply the same clamping logic used for sidebarTintOpacity to backgroundOpacity)
and use that clamped value instead of raw backgroundOpacity; reference the
variables sidebarTintOpacity, backgroundOpacity and the defaults.set(...) call
to locate and fix the code.
- Around line 169-171: The branch is incorrectly treating the user-selected
SidebarMaterialOption.none as auto-syncable; instead of overloading
SidebarMaterialOption.none, add and use an explicit auto-sync marker (e.g., a
new enum case or a separate Bool in settings) and change the condition around
currentMaterial in GhosttyConfig (the if that checks currentMaterial against
SidebarMaterialOption.none and SidebarMaterialOption.sidebar) to guard on that
new auto-sync flag (or enum value) rather than the .none rawValue; apply the
same change to the similar check at the other occurrence (the block around lines
178-182) so explicitly user-selected “None” is never overwritten during theme
refresh.
🪄 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: d25fcce2-4525-4455-a6ce-f5e84a44515f

📥 Commits

Reviewing files that changed from the base of the PR and between 6e5903e and 472f08f.

📒 Files selected for processing (4)
  • Sources/ContentView.swift
  • Sources/GhosttyConfig.swift
  • Sources/WorkspaceContentView.swift
  • cmuxTests/GhosttyConfigTests.swift

Comment on lines +2484 to +2515
func testApplyToUserDefaultsFallsBackToTerminalBackground() {
let defaults = UserDefaults.standard
let testKey = "sidebarTintHex"
let original = defaults.string(forKey: testKey)
defer { restoreDefaultsValue(original, key: testKey, defaults: defaults) }
let keys = ["sidebarTintHex", "sidebarTintHexLight", "sidebarTintHexDark",
"sidebarTintOpacity", "sidebarMaterial"]
let originals = keys.map { defaults.object(forKey: $0) }
defer {
for (key, original) in zip(keys, originals) {
restoreDefaultsValue(original, key: key, defaults: defaults)
}
}

defaults.set("#AAAAAA", forKey: "sidebarTintHex")
defaults.set("#BBBBBB", forKey: "sidebarTintHexLight")
defaults.set("#CCCCCC", forKey: "sidebarTintHexDark")

var config = GhosttyConfig()
config.backgroundColor = NSColor(hex: "#1e1e2e")!
config.backgroundOpacity = 0.9
config.applySidebarAppearanceToUserDefaults()

XCTAssertEqual(defaults.string(forKey: "sidebarTintHex"), "#1e1e2e",
"Should fall back to terminal background when rawSidebarBackground is nil")
XCTAssertNil(defaults.string(forKey: "sidebarTintHexLight"),
"Should clear stale light key")
XCTAssertNil(defaults.string(forKey: "sidebarTintHexDark"),
"Should clear stale dark key")
XCTAssertEqual(defaults.double(forKey: "sidebarTintOpacity"), 0.9, accuracy: 0.0001,
"Should use backgroundOpacity when sidebarTintOpacity is nil")
XCTAssertEqual(defaults.string(forKey: "sidebarMaterial"),
SidebarMaterialOption.none.rawValue,
"Should set material to none for solid color rendering")
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify hexString() output format by checking its implementation and existing test assertions

echo "=== Checking NSColor hexString() implementation ==="
ast-grep --pattern 'func hexString() -> String {
  $$$
}'

echo ""
echo "=== Checking existing hexString() test assertions for case pattern ==="
rg -n 'hexString\(\).*#[A-Fa-f0-9]' --type swift | head -20

Repository: manaflow-ai/cmux

Length of output: 516


🏁 Script executed:

# Find applySidebarAppearanceToUserDefaults() implementation
rg -n "func applySidebarAppearanceToUserDefaults" --type swift -A 30

Repository: manaflow-ai/cmux

Length of output: 2559


🏁 Script executed:

# Read the complete test methods (2484-2564) to see all hex assertions
sed -n '2484,2564p' cmuxTests/GhosttyConfigTests.swift

Repository: manaflow-ai/cmux

Length of output: 3978


Fix hex case mismatch in test assertion.

Line 2504 expects lowercase "#1e1e2e", but hexString() returns uppercase hex per the implementation (line 178: defaults.set(backgroundColor.hexString(), ...)). The assertion will fail.

🐛 Fix: use uppercase hex
-        XCTAssertEqual(defaults.string(forKey: "sidebarTintHex"), "#1e1e2e",
+        XCTAssertEqual(defaults.string(forKey: "sidebarTintHex"), "#1E1E2E",
                        "Should fall back to terminal background when rawSidebarBackground is nil")
📝 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
func testApplyToUserDefaultsFallsBackToTerminalBackground() {
let defaults = UserDefaults.standard
let testKey = "sidebarTintHex"
let original = defaults.string(forKey: testKey)
defer { restoreDefaultsValue(original, key: testKey, defaults: defaults) }
let keys = ["sidebarTintHex", "sidebarTintHexLight", "sidebarTintHexDark",
"sidebarTintOpacity", "sidebarMaterial"]
let originals = keys.map { defaults.object(forKey: $0) }
defer {
for (key, original) in zip(keys, originals) {
restoreDefaultsValue(original, key: key, defaults: defaults)
}
}
defaults.set("#AAAAAA", forKey: "sidebarTintHex")
defaults.set("#BBBBBB", forKey: "sidebarTintHexLight")
defaults.set("#CCCCCC", forKey: "sidebarTintHexDark")
var config = GhosttyConfig()
config.backgroundColor = NSColor(hex: "#1e1e2e")!
config.backgroundOpacity = 0.9
config.applySidebarAppearanceToUserDefaults()
XCTAssertEqual(defaults.string(forKey: "sidebarTintHex"), "#1e1e2e",
"Should fall back to terminal background when rawSidebarBackground is nil")
XCTAssertNil(defaults.string(forKey: "sidebarTintHexLight"),
"Should clear stale light key")
XCTAssertNil(defaults.string(forKey: "sidebarTintHexDark"),
"Should clear stale dark key")
XCTAssertEqual(defaults.double(forKey: "sidebarTintOpacity"), 0.9, accuracy: 0.0001,
"Should use backgroundOpacity when sidebarTintOpacity is nil")
XCTAssertEqual(defaults.string(forKey: "sidebarMaterial"),
SidebarMaterialOption.none.rawValue,
"Should set material to none for solid color rendering")
}
func testApplyToUserDefaultsFallsBackToTerminalBackground() {
let defaults = UserDefaults.standard
let keys = ["sidebarTintHex", "sidebarTintHexLight", "sidebarTintHexDark",
"sidebarTintOpacity", "sidebarMaterial"]
let originals = keys.map { defaults.object(forKey: $0) }
defer {
for (key, original) in zip(keys, originals) {
restoreDefaultsValue(original, key: key, defaults: defaults)
}
}
defaults.set("#AAAAAA", forKey: "sidebarTintHex")
defaults.set("#BBBBBB", forKey: "sidebarTintHexLight")
defaults.set("#CCCCCC", forKey: "sidebarTintHexDark")
var config = GhosttyConfig()
config.backgroundColor = NSColor(hex: "#1e1e2e")!
config.backgroundOpacity = 0.9
config.applySidebarAppearanceToUserDefaults()
XCTAssertEqual(defaults.string(forKey: "sidebarTintHex"), "#1E1E2E",
"Should fall back to terminal background when rawSidebarBackground is nil")
XCTAssertNil(defaults.string(forKey: "sidebarTintHexLight"),
"Should clear stale light key")
XCTAssertNil(defaults.string(forKey: "sidebarTintHexDark"),
"Should clear stale dark key")
XCTAssertEqual(defaults.double(forKey: "sidebarTintOpacity"), 0.9, accuracy: 0.0001,
"Should use backgroundOpacity when sidebarTintOpacity is nil")
XCTAssertEqual(defaults.string(forKey: "sidebarMaterial"),
SidebarMaterialOption.none.rawValue,
"Should set material to none for solid color rendering")
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cmuxTests/GhosttyConfigTests.swift` around lines 2484 - 2515, The test
testApplyToUserDefaultsFallsBackToTerminalBackground asserts the sidebar tint
hex as "#1e1e2e" but the code stores backgroundColor.hexString() in UserDefaults
which returns an uppercase hex; update the expectation to the uppercase form
(e.g. "#1E1E2E") to match the value written by
GhosttyConfig.applySidebarAppearanceToUserDefaults (and the NSColor.hexString
implementation).

Comment on lines +169 to +171
if let currentMaterial,
currentMaterial != SidebarMaterialOption.none.rawValue,
currentMaterial != SidebarMaterialOption.sidebar.rawValue {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Manual None is not actually protected here.

SidebarMaterialOption.none is a real Settings choice in Sources/ContentView.swift:13790-13823, but this branch treats .none as auto-sync-eligible. If a user explicitly picks None and customizes tint/opacity, the next theme refresh will overwrite those values. This needs a separate “auto-synced vs user-selected” marker instead of overloading the .none raw value.

Also applies to: 178-182

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Sources/GhosttyConfig.swift` around lines 169 - 171, The branch is
incorrectly treating the user-selected SidebarMaterialOption.none as
auto-syncable; instead of overloading SidebarMaterialOption.none, add and use an
explicit auto-sync marker (e.g., a new enum case or a separate Bool in settings)
and change the condition around currentMaterial in GhosttyConfig (the if that
checks currentMaterial against SidebarMaterialOption.none and
SidebarMaterialOption.sidebar) to guard on that new auto-sync flag (or enum
value) rather than the .none rawValue; apply the same change to the similar
check at the other occurrence (the block around lines 178-182) so explicitly
user-selected “None” is never overwritten during theme refresh.

defaults.set(backgroundColor.hexString(), forKey: "sidebarTintHex")
defaults.removeObject(forKey: "sidebarTintHexLight")
defaults.removeObject(forKey: "sidebarTintHexDark")
defaults.set(sidebarTintOpacity ?? backgroundOpacity, forKey: "sidebarTintOpacity")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Clamp the fallback opacity before persisting it.

sidebarTintOpacity is clamped on parse, but backgroundOpacity is not. With this fallback, an out-of-range background-opacity now gets written straight into the sidebar setting.

Suggested fix
-            defaults.set(sidebarTintOpacity ?? backgroundOpacity, forKey: "sidebarTintOpacity")
+            let resolvedOpacity = min(max(sidebarTintOpacity ?? backgroundOpacity, 0), 1)
+            defaults.set(resolvedOpacity, forKey: "sidebarTintOpacity")
📝 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
defaults.set(sidebarTintOpacity ?? backgroundOpacity, forKey: "sidebarTintOpacity")
let resolvedOpacity = min(max(sidebarTintOpacity ?? backgroundOpacity, 0), 1)
defaults.set(resolvedOpacity, forKey: "sidebarTintOpacity")
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Sources/GhosttyConfig.swift` at line 181, The saved sidebar opacity currently
uses "sidebarTintOpacity ?? backgroundOpacity" which can write an out-of-range
backgroundOpacity; update GhosttyConfig.swift so the value persisted via
defaults.set(...) is clamped to the valid range (e.g., 0.0...1.0). Specifically,
when calling defaults.set(...) for sidebarTintOpacity, compute the fallback as a
clampedBackgroundOpacity (apply the same clamping logic used for
sidebarTintOpacity to backgroundOpacity) and use that clamped value instead of
raw backgroundOpacity; reference the variables sidebarTintOpacity,
backgroundOpacity and the defaults.set(...) call to locate and fix the code.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 27, 2026

Greptile Summary

This PR adds automatic sidebar-background synchronization with the active Ghostty terminal theme. When no explicit sidebar-background is configured, applySidebarAppearanceToUserDefaults now writes the terminal's backgroundColor (and its opacity) to the relevant UserDefaults keys and forces sidebarMaterial to "none" so SidebarBackdrop renders a solid colour without NSVisualEffectView distortion. Runtime sync is wired into the existing refreshGhosttyAppearanceConfig notification flow in WorkspaceContentView. The feature is well-structured and the three new tests cover the main branches.\n\nKey changes:\n- GhosttyConfig.applySidebarAppearanceToUserDefaults auto-syncs terminal background → sidebar colour when rawSidebarBackground is nil, with a guard that skips sync when the user has manually chosen a non-default material.\n- SidebarBackdrop now renders a solid Color in the else branch (when materialOption?.material is nil), enabling the material = \"none\" path to display correctly.\n- WorkspaceContentView.refreshGhosttyAppearanceConfig calls applySidebarAppearanceToUserDefaults at runtime for configs with no explicit sidebar background.\n\nIssues found:\n- The guard distinguishing "user chose None" from "auto-sync set None" is ambiguous: both states leave sidebarMaterial = \"none\", so a user who explicitly picked the None material from Settings UI will have their custom tint hex silently overwritten on the next theme change.\n- sidebarTintOpacity is now unconditionally written (falling back to backgroundOpacity which defaults to 1.0), which resets any opacity the user may have configured in the Settings UI on every subsequent theme change.

Confidence Score: 3/5

Merging will silently overwrite user-configured sidebar opacity on every runtime theme change and makes manual "None" material indistinguishable from the auto-sync state; needs one of the two P1s resolved before shipping.

The feature logic and rendering changes are correct, and the three new unit tests cover the happy path well. However two P1 issues affect existing users: (1) any user who manually tuned the sidebar opacity via Settings UI will have it reset to the terminal background opacity on each theme change because the write is now unconditional; (2) the "None" material sentinel is shared between "auto-synced" and "user explicitly chose None", so the custom colour protection only works for non-None materials. Both issues cause silent data-loss of user preferences in normal usage.

Sources/GhosttyConfig.swift — guard condition and unconditional opacity write (lines 169–182)

Important Files Changed

Filename Overview
Sources/GhosttyConfig.swift Core logic change: rewrites applySidebarAppearanceToUserDefaults to auto-sync sidebar color from terminal background when no explicit sidebar-background is configured; includes guard to respect user material overrides (anything other than "none" or "sidebar").
Sources/ContentView.swift Adds else branch to SidebarBackdrop so material="none" renders a solid Color(nsColor: tintColor) instead of nothing; correctly handles the new "match terminal background" use case.
Sources/WorkspaceContentView.swift Calls applySidebarAppearanceToUserDefaults at runtime theme changes, gated behind a rawSidebarBackground == nil check to only apply auto-sync (not re-write explicit sidebar backgrounds at runtime).
cmuxTests/GhosttyConfigTests.swift Replaces one old test and adds two new unit tests covering: fallback to terminal background, skip when user chose a custom material, and proceed when material is the default "sidebar".

Sequence Diagram

sequenceDiagram
    participant G as GhosttyConfig
    participant WCV as WorkspaceContentView
    participant UD as UserDefaults
    participant SB as SidebarBackdrop

    Note over G: App launch / loadFromDisk
    G->>G: resolveSidebarBackground()
    G->>UD: applySidebarAppearanceToUserDefaults()
    alt rawSidebarBackground == nil
        G->>UD: check sidebarMaterial
        alt material is nil / none / sidebar
            UD-->>G: proceed with auto-sync
            G->>UD: set sidebarTintHex = backgroundColor.hexString()
            G->>UD: remove sidebarTintHexLight/Dark
            G->>UD: set sidebarTintOpacity = sidebarTintOpacity ?? backgroundOpacity
            G->>UD: set sidebarMaterial = none
        else material is user override (e.g. hudWindow)
            G-->>G: skip (respect user choice)
        end
    else rawSidebarBackground != nil
        G->>UD: write explicit sidebar colors
    end

    Note over WCV: Runtime theme change
    WCV->>G: refreshGhosttyAppearanceConfig(next)
    WCV->>WCV: config = next
    WCV->>WCV: applyGhosttyChrome(from: next)
    alt next.rawSidebarBackground == nil
        WCV->>UD: next.applySidebarAppearanceToUserDefaults()
    end

    Note over SB: SwiftUI re-render
    UD-->>SB: @AppStorage sidebarMaterial = none
    SB->>SB: materialOption?.material == nil
    SB->>SB: else branch -> Color(nsColor: tintColor)
Loading

Reviews (1): Last reviewed commit: "feat: auto-match sidebar background to G..." | Re-trigger Greptile

Comment on lines +169 to +172
if let currentMaterial,
currentMaterial != SidebarMaterialOption.none.rawValue,
currentMaterial != SidebarMaterialOption.sidebar.rawValue {
return
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 "None" material indistinguishable from auto-sync state

The guard allows auto-sync to proceed whenever currentMaterial is nil, "none", or "sidebar". Because auto-sync itself writes "none" to sidebarMaterial, every subsequent theme-change invocation will also pass through the guard and overwrite both sidebarTintHex and sidebarTintOpacity.

The problem surfaces for any user who manually chose None from the Settings UI material picker with a custom tint color: after a theme change, their custom hex (and opacity) are silently replaced by the terminal background. There's no way to distinguish "auto-sync wrote none" from "user explicitly chose none."

A light mitigation would be to introduce a separate sentinel key (e.g. sidebarAutoSyncEnabled) written by applySidebarAppearanceToUserDefaults and checked by the guard, so a deliberate user "none" choice can be respected.

defaults.set(backgroundColor.hexString(), forKey: "sidebarTintHex")
defaults.removeObject(forKey: "sidebarTintHexLight")
defaults.removeObject(forKey: "sidebarTintHexDark")
defaults.set(sidebarTintOpacity ?? backgroundOpacity, forKey: "sidebarTintOpacity")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Auto-sync unconditionally overwrites user-configured opacity

sidebarTintOpacity is now always written on every auto-sync call, falling back to backgroundOpacity (which defaults to 1.0). The previous implementation only wrote this key when sidebarTintOpacity was explicitly present in the Ghostty config.

A user who adjusts the opacity slider in the Settings UI will have their preference reset on the next runtime theme change, because the material remains "none" after the first auto-sync (allowing the guard to pass through on every subsequent call). Users who had dialled down the opacity will find it silently reset to fully-opaque.

Consider only overwriting the opacity when the Ghostty config explicitly specifies one, and leaving the existing UserDefaults value untouched otherwise — matching the original guarded behaviour.

Comment on lines +675 to +677
if next.rawSidebarBackground == nil {
next.applySidebarAppearanceToUserDefaults()
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Redundant outer nil-guard

applySidebarAppearanceToUserDefaults() already returns early in its first if rawSidebarBackground == nil block when the condition is false. The outer check here is not wrong, but it adds noise and could mislead future readers into thinking the method doesn't internally handle the non-nil case.

A brief inline comment explaining why the guard is intentional (i.e., "we deliberately skip re-writing explicit sidebar colours on every theme event") would make the intent clearer without requiring a change to the method itself.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

3 issues found across 4 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="cmuxTests/GhosttyConfigTests.swift">

<violation number="1" location="cmuxTests/GhosttyConfigTests.swift:2504">
P2: Test expectation uses lowercase hex while app serialization outputs uppercase, causing false-negative failures.</violation>

<violation number="2" location="cmuxTests/GhosttyConfigTests.swift:2519">
P2: Test cleanup is incomplete: it restores only two UserDefaults keys, but the exercised auto-sync path mutates additional sidebar keys, risking cross-test state leakage/flakiness.</violation>
</file>

<file name="Sources/GhosttyConfig.swift">

<violation number="1" location="Sources/GhosttyConfig.swift:170">
P1: Treating `.none` as auto-sync-eligible causes manual “None” material selections to be overwritten during theme refresh. Add a distinct auto-sync marker (or otherwise differentiate user-selected `.none`) so custom tint/opacity values are preserved.</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-ai with guidance or docs links (including llms.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.

// Settings, respect their choice and skip auto-sync.
let currentMaterial = defaults.string(forKey: "sidebarMaterial")
if let currentMaterial,
currentMaterial != SidebarMaterialOption.none.rawValue,
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai bot Mar 27, 2026

Choose a reason for hiding this comment

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

P1: Treating .none as auto-sync-eligible causes manual “None” material selections to be overwritten during theme refresh. Add a distinct auto-sync marker (or otherwise differentiate user-selected .none) so custom tint/opacity values are preserved.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/GhosttyConfig.swift, line 170:

<comment>Treating `.none` as auto-sync-eligible causes manual “None” material selections to be overwritten during theme refresh. Add a distinct auto-sync marker (or otherwise differentiate user-selected `.none`) so custom tint/opacity values are preserved.</comment>

<file context>
@@ -160,10 +160,26 @@ struct GhosttyConfig {
+            // Settings, respect their choice and skip auto-sync.
+            let currentMaterial = defaults.string(forKey: "sidebarMaterial")
+            if let currentMaterial,
+               currentMaterial != SidebarMaterialOption.none.rawValue,
+               currentMaterial != SidebarMaterialOption.sidebar.rawValue {
+                return
</file context>
Fix with Cubic


func testApplyToUserDefaultsSkipsAutoSyncWhenUserChoseCustomMaterial() {
let defaults = UserDefaults.standard
let keys = ["sidebarTintHex", "sidebarMaterial"]
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai bot Mar 27, 2026

Choose a reason for hiding this comment

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

P2: Test cleanup is incomplete: it restores only two UserDefaults keys, but the exercised auto-sync path mutates additional sidebar keys, risking cross-test state leakage/flakiness.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmuxTests/GhosttyConfigTests.swift, line 2519:

<comment>Test cleanup is incomplete: it restores only two UserDefaults keys, but the exercised auto-sync path mutates additional sidebar keys, risking cross-test state leakage/flakiness.</comment>

<file context>
@@ -2481,19 +2481,86 @@ final class SidebarBackgroundConfigTests: XCTestCase {
+
+    func testApplyToUserDefaultsSkipsAutoSyncWhenUserChoseCustomMaterial() {
+        let defaults = UserDefaults.standard
+        let keys = ["sidebarTintHex", "sidebarMaterial"]
+        let originals = keys.map { defaults.object(forKey: $0) }
+        defer {
</file context>
Fix with Cubic

config.backgroundOpacity = 0.9
config.applySidebarAppearanceToUserDefaults()

XCTAssertEqual(defaults.string(forKey: "sidebarTintHex"), "#1e1e2e",
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai bot Mar 27, 2026

Choose a reason for hiding this comment

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

P2: Test expectation uses lowercase hex while app serialization outputs uppercase, causing false-negative failures.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmuxTests/GhosttyConfigTests.swift, line 2504:

<comment>Test expectation uses lowercase hex while app serialization outputs uppercase, causing false-negative failures.</comment>

<file context>
@@ -2481,19 +2481,86 @@ final class SidebarBackgroundConfigTests: XCTestCase {
+        config.backgroundOpacity = 0.9
+        config.applySidebarAppearanceToUserDefaults()
+
+        XCTAssertEqual(defaults.string(forKey: "sidebarTintHex"), "#1e1e2e",
+                       "Should fall back to terminal background when rawSidebarBackground is nil")
+        XCTAssertNil(defaults.string(forKey: "sidebarTintHexLight"),
</file context>
Suggested change
XCTAssertEqual(defaults.string(forKey: "sidebarTintHex"), "#1e1e2e",
XCTAssertEqual(defaults.string(forKey: "sidebarTintHex"), "#1E1E2E",
Fix with Cubic

@christi4nity christi4nity deleted the feat/sidebar-theme-match branch March 27, 2026 04:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant