feat: import terminal color schemes from .itermcolors files#168
Merged
hanrw merged 2 commits intotddworks:mainfrom Apr 1, 2026
Merged
feat: import terminal color schemes from .itermcolors files#168hanrw merged 2 commits intotddworks:mainfrom
hanrw merged 2 commits intotddworks:mainfrom
Conversation
Add the ability to import .itermcolors files into ClaudeBar, automatically generating a full theme from 16 ANSI colors + background/foreground. Architecture: - TerminalColorScheme: Codable model with RGBColor, luminance, lighten/darken - ITermColorsParser: Parse .itermcolors XML plist (Foundation-native, zero deps) - TerminalThemeGenerator: Map ANSI colors to AppThemeProvider properties - ImportedTerminalTheme: Full AppThemeProvider backed by generated values - ImportedThemeStore: JSON persistence in ~/.claudebar/themes/ - ThemeImportView: File picker with import button in Settings Also adds customFontName + font() helper to AppThemeProvider protocol, and makes ThemeOptionButton dynamic (works with both built-in and imported themes). Compatible with 450+ pre-made schemes from mbadolato/iTerm2-Color-Schemes. Includes 21 tests across 3 test suites.
Contributor
📝 WalkthroughWalkthroughAdds import support for iTerm2 Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SettingsView
participant ThemeImportButton
participant ITermColorsParser
participant ThemeRegistry
participant ImportedThemeStore
participant TerminalThemeGenerator
User->>SettingsView: Open settings
SettingsView->>ThemeImportButton: Click "Import Theme"
ThemeImportButton->>User: Present file picker (.itermcolors)
User->>ThemeImportButton: Select file URL
ThemeImportButton->>ITermColorsParser: parse(from: URL)
ITermColorsParser-->>ThemeImportButton: TerminalColorScheme
ThemeImportButton->>ThemeRegistry: importItermcolors(from: URL)
ThemeRegistry->>TerminalThemeGenerator: generate(from: scheme)
TerminalThemeGenerator-->>ThemeRegistry: GeneratedThemeProperties
ThemeRegistry->>ImportedThemeStore: save(scheme)
ImportedThemeStore-->>ThemeRegistry: persisted
ThemeRegistry-->>ThemeImportButton: return ImportedTerminalTheme
ThemeImportButton-->>SettingsView: show success
SettingsView->>ThemeRegistry: allThemes (includes imported)
User->>SettingsView: select imported theme
SettingsView->>ThemeRegistry: apply selected theme id
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (10)
Sources/App/Views/SettingsView.swift (2)
1247-1251: Consider using consistent corner radius from current theme.The button uses
themeProvider.cardCornerRadius(the theme being represented) rather thantheme.cardCornerRadius(the currently active theme). This could cause visual inconsistency in the settings grid where different theme buttons might have different corner radii.♻️ Suggested fix for consistent styling
.background( - RoundedRectangle(cornerRadius: themeProvider.cardCornerRadius) + RoundedRectangle(cornerRadius: theme.cardCornerRadius) .fill(isSelected ? theme.accentPrimary.opacity(0.15) : (isHovering ? theme.hoverOverlay : Color.clear)) .overlay( - RoundedRectangle(cornerRadius: themeProvider.cardCornerRadius) + RoundedRectangle(cornerRadius: theme.cardCornerRadius) .stroke(isSelected ? theme.accentPrimary : theme.glassBorder.opacity(0.5), lineWidth: isSelected ? 2 : 1) ) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/App/Views/SettingsView.swift` around lines 1247 - 1251, The corner radius is using themeProvider.cardCornerRadius which can differ from the active theme; update the RoundedRectangle constructors and overlay stroke to use the active theme's radius (theme.cardCornerRadius) instead of themeProvider.cardCornerRadius so the button visuals (RoundedRectangle(...), overlay RoundedRectangle(...).stroke(...)) use a consistent corner radius for isSelected/isHovering states.
1227-1236: Removal button may trigger accidental deletion.The remove button is placed inline without a confirmation dialog. Users might accidentally delete imported themes. Consider adding a confirmation alert or using a swipe-to-delete pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/App/Views/SettingsView.swift` around lines 1227 - 1236, The inline remove Button (guarded by isImported) calls ThemeRegistry.shared.removeImportedTheme(id: themeProvider.id) with no confirmation; add a confirmation step before invoking removeImportedTheme: introduce a `@State` Bool like showConfirmDelete, change the Button action to set that flag, present an Alert (or confirmationDialog) tied to showConfirmDelete that, on confirmation, calls ThemeRegistry.shared.removeImportedTheme(id: themeProvider.id) and dismisses the alert; alternatively implement swipe-to-delete on the list that shows the same confirmation before calling removeImportedTheme. Ensure you reference the existing isImported check, the Button, themeProvider.id and the removeImportedTheme call so the confirmation wraps exactly that removal.Tests/InfrastructureTests/TerminalImport/TerminalColorSchemeTests.swift (1)
9-17: Consider using approximate comparison for floating-point values.While exact equality works here since values are round-tripped without arithmetic, JSON encoding/decoding of
Doublecan occasionally introduce representation differences. Consider using#expect(abs(decoded.red - 0.114) < 0.0001)or Swift Testing'sisApproximatelyEqualif available for more robust tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/InfrastructureTests/TerminalImport/TerminalColorSchemeTests.swift` around lines 9 - 17, The test `RGBColor encodes and decodes correctly` currently asserts exact equality on floating-point fields which can be brittle; update the assertions in that test to use approximate comparisons (e.g., check absolute difference < 1e-4 or use a provided isApproximatelyEqual helper) for decoded.red, decoded.green, decoded.blue, and decoded.alpha when comparing to 0.114, 0.145, 0.169, and 1.0 respectively so the test compares TerminalColorScheme.RGBColor floating values robustly after JSON round-trip.Sources/App/Theme/AppThemeProvider.swift (1)
182-194: Font name suffix convention may not work for all font families.The implementation assumes custom fonts follow the
FontName-Weightnaming convention (e.g., "IBMPlexMono-Bold"). While this works for many fonts, some use different conventions (e.g., "IBMPlexMono Bold" with a space, or "IBMPlexMonoBold").Consider adding a comment documenting this assumption, or making the suffix pattern configurable if broader font support is needed.
📝 Suggested documentation addition
public extension AppThemeProvider { - /// Returns the appropriate font for this theme — custom font if set, otherwise system font with fontDesign. + /// Returns the appropriate font for this theme — custom font if set, otherwise system font with fontDesign. + /// Note: Custom fonts must follow the `FontFamily-Weight` naming convention (e.g., "IBMPlexMono-Bold"). func font(size: CGFloat, weight: Font.Weight = .regular) -> Font {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/App/Theme/AppThemeProvider.swift` around lines 182 - 194, The font(size:weight:) helper currently builds custom font names by appending a hard-coded suffix pattern (variable suffix) to customFontName, which assumes the "FontName-Weight" convention; update the implementation to either (a) document this assumption above font(size:weight:) and customFontName, or (b) make the suffix generation configurable (for example expose a closure or enum strategy on AppThemeProvider to produce the final font name) so fonts with "FontName Weight" or "FontNameWeight" formats are supported; refer to the font(size:weight:), customFontName, and suffix symbols when applying the change.Sources/App/Theme/ThemeRegistry.swift (2)
105-114: Consider handling duplicate theme name conflicts.If a user imports a theme with the same name as an existing one, the current implementation will overwrite it via
register(). This might be the desired behavior, but consider:
- Checking for existing themes and prompting the user
- Appending a suffix to make names unique
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/App/Theme/ThemeRegistry.swift` around lines 105 - 114, The importItermcolors(from:) path currently creates and registers an ImportedTerminalTheme and calls importedThemeStore.save(scheme) which will overwrite an existing theme with the same name; change it to detect name collisions before saving/registration by checking the registry for an existing theme with scheme.name (use the existing registry lookup used by register(_:)), and if a collision exists either (a) append a unique numeric suffix to scheme.name (e.g., "Name (1)") and update the scheme and props accordingly before calling importedThemeStore.save and register, or (b) surface a user-facing conflict resolution (throw a specific error or return a status so the caller can prompt). Ensure the chosen path updates the ImportedTerminalTheme instance and persisted scheme to avoid accidental overwrites when calling importedThemeStore.save and register(theme).
117-123: Silent failure on delete could mask file system issues.Line 122 uses
try?which silently ignores errors when deleting the persisted theme file. Consider logging the error for debugging purposes.📝 Suggested improvement with error logging
public func removeImportedTheme(id: String) { guard let theme = themes[id], theme is ImportedTerminalTheme else { return } let displayName = theme.displayName themes.removeValue(forKey: id) themeOrder.removeAll { $0 == id } - try? importedThemeStore.delete(name: displayName) + do { + try importedThemeStore.delete(name: displayName) + } catch { + AppLog.ui.warning("Failed to delete theme file for '\(displayName)': \(error.localizedDescription)") + } }As per coding guidelines: Use
AppLogcategories for dual-output logging to OSLog and file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/App/Theme/ThemeRegistry.swift` around lines 117 - 123, The removeImportedTheme(id:) function currently swallows errors from importedThemeStore.delete(name:) with try?, so change that to a do-catch: call try importedThemeStore.delete(name: displayName) inside a do block and catch any thrown error, then log the failure using the AppLog category (e.g. AppLog.theme.error) including the displayName and error details to surface filesystem/persistence issues; keep the existing removal of themes[id] and themeOrder logic intact and only attempt the delete after confirming the theme is an ImportedTerminalTheme.Sources/Infrastructure/TerminalImport/TerminalColorScheme.swift (1)
38-54: ANSI accessors assume valid array length.These computed properties directly index into
ansiColorswithout bounds checking. This is acceptable givenisValidand the parser's enforcement of 16 colors, but the accessors will crash if used on an invalid scheme.This design is fine given the precondition in
TerminalThemeGenerator, but consider adding a doc comment noting theisValidrequirement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Infrastructure/TerminalImport/TerminalColorScheme.swift` around lines 38 - 54, The ANSI color accessor properties (black, red, ..., brightWhite) index into ansiColors without bounds checks and will crash if ansiColors is not length 16; add a doc comment on the computed properties (or on the TerminalColorScheme type) stating they require ansiColors to contain exactly 16 entries and that callers must validate isValid (used by TerminalThemeGenerator) before accessing these accessors, referencing the ansiColors storage and the isValid precondition used by TerminalThemeGenerator to make the contract explicit.Sources/App/Theme/ImportedThemeStore.swift (2)
49-53: Duplicate sanitization logic withTerminalThemeGenerator.The
sanitize(_:)function here duplicates the same logic inTerminalThemeGenerator.generate(from:)(lines 34-37). If either implementation drifts, filenames won't match theme IDs, breaking delete operations.♻️ Extract shared sanitization helper
Consider adding a shared utility in
Infrastructure:// In a shared location, e.g., TerminalColorScheme+Utils.swift public extension TerminalColorScheme { var sanitizedName: String { name.lowercased() .replacingOccurrences(of: " ", with: "-") .replacingOccurrences(of: "[^a-z0-9\\-]", with: "", options: .regularExpression) } }Then use
scheme.sanitizedNamein bothTerminalThemeGeneratorandImportedThemeStore.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/App/Theme/ImportedThemeStore.swift` around lines 49 - 53, The sanitize(_:) implementation in ImportedThemeStore duplicates the logic in TerminalThemeGenerator.generate(from:); extract the sanitization into a shared helper (e.g., add a public extension on TerminalColorScheme with a sanitizedName computed property) and replace calls to ImportedThemeStore.sanitize(_:) and the inline sanitization in TerminalThemeGenerator.generate(from:) to use scheme.sanitizedName instead so filenames and theme IDs remain consistent across both locations.
13-26: Consider logging decode failures for debugging.The
compactMapwithtry?silently skips corrupted or incompatible theme files. This makes it hard to diagnose why an imported theme doesn't appear after an app update that changes the schema.🔧 Add diagnostic logging
return files .filter { $0.pathExtension == "json" } .compactMap { url -> (TerminalColorScheme, Date)? in - guard let data = try? Data(contentsOf: url), - let entry = try? JSONDecoder().decode(StoredTheme.self, from: data) - else { return nil } + do { + let data = try Data(contentsOf: url) + let entry = try JSONDecoder().decode(StoredTheme.self, from: data) + return (entry.scheme, entry.importedAt) + } catch { + AppLog.ui.warning("Failed to load theme from \(url.lastPathComponent): \(error.localizedDescription)") + return nil + } - return (entry.scheme, entry.importedAt) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/App/Theme/ImportedThemeStore.swift` around lines 13 - 26, In loadAll(), currently the Data and JSON decoding failures for files in themesDirectory are silently ignored; update the compactMap closure (and/or replace try? with do/catch) to catch and log failures when reading Data or decoding StoredTheme (include the file URL/path, the error.localizedDescription or error object, and mention StoredTheme and TerminalColorScheme) so corrupted/incompatible JSON files are diagnosable while still skipping invalid files.Sources/Infrastructure/TerminalImport/ITermColorsParser.swift (1)
48-60: Silent defaults for missing color components may mask file corruption.When a color dictionary exists but is missing
Red Component,Green Component, orBlue Component, the parser defaults them to0.0. This could produce unexpected black colors from partially corrupted files rather than surfacing the issue.This is likely acceptable for compatibility with legacy formats (as tested), but worth documenting the behavior.
📝 Optional: Add a comment documenting the fallback behavior
private static func extractColor( from plist: [String: Any], key: String ) throws -> TerminalColorScheme.RGBColor { guard let colorDict = plist[key] as? [String: Any] else { throw ITermColorsParserError.missingColor(key) } + // Default missing components to 0.0 for legacy format compatibility let red = (colorDict["Red Component"] as? Double) ?? 0.0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Infrastructure/TerminalImport/ITermColorsParser.swift` around lines 48 - 60, The extractColor(from:key:) function currently substitutes missing "Red Component", "Green Component", and "Blue Component" with 0.0 (and "Alpha Component" with 1.0) which can silently produce black colors for partially corrupted plists; add a brief doc comment directly above the private static func extractColor(...) describing this fallback behavior (which components default to which values), why it is kept for legacy compatibility, and note that callers should expect possible black colors from incomplete data (mention ITermColorsParserError.missingColor as the error thrown when the entire color dictionary is absent).
🤖 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/Infrastructure/TerminalImport/TerminalThemeGenerator.swift`:
- Around line 34-40: The current sanitization of scheme.name to sanitizedId can
produce collisions (e.g., "My Theme!" vs "My-Theme!") which causes silent
overwrites when creating GeneratedThemeProperties; modify the creation path that
computes sanitizedId to detect or avoid collisions by appending a short
deterministic hash suffix derived from the original scheme identifier (for
example a 6–8 character hash of scheme.name or the full filename) to the id
(e.g., "imported-\(sanitizedId)-\(shortHash)"), ensuring the unique
GeneratedThemeProperties id and preventing silent replacement in ThemeRegistry;
implement the hash generation where sanitizedId is computed and use that new id
when constructing GeneratedThemeProperties so downstream registration no longer
collides.
- Around line 32-33: The generate(from:) function in TerminalThemeGenerator
currently calls precondition(scheme.isValid) which will crash on corrupted
persisted themes; change generate(from: TerminalColorScheme) to return an
optional GeneratedThemeProperties? (or throw) instead of asserting, returning
nil when scheme.isValid is false or when the ansiColors array is malformed, and
update callers such as ThemeRegistry.loadImportedThemes() (where
importedThemeStore.loadAll() is iterated) to guard-let the result, log a warning
(e.g. "Skipping invalid imported theme: \(scheme.name)"), and continue; ensure
ImportedTerminalTheme initialization only happens when props is non-nil.
---
Nitpick comments:
In `@Sources/App/Theme/AppThemeProvider.swift`:
- Around line 182-194: The font(size:weight:) helper currently builds custom
font names by appending a hard-coded suffix pattern (variable suffix) to
customFontName, which assumes the "FontName-Weight" convention; update the
implementation to either (a) document this assumption above font(size:weight:)
and customFontName, or (b) make the suffix generation configurable (for example
expose a closure or enum strategy on AppThemeProvider to produce the final font
name) so fonts with "FontName Weight" or "FontNameWeight" formats are supported;
refer to the font(size:weight:), customFontName, and suffix symbols when
applying the change.
In `@Sources/App/Theme/ImportedThemeStore.swift`:
- Around line 49-53: The sanitize(_:) implementation in ImportedThemeStore
duplicates the logic in TerminalThemeGenerator.generate(from:); extract the
sanitization into a shared helper (e.g., add a public extension on
TerminalColorScheme with a sanitizedName computed property) and replace calls to
ImportedThemeStore.sanitize(_:) and the inline sanitization in
TerminalThemeGenerator.generate(from:) to use scheme.sanitizedName instead so
filenames and theme IDs remain consistent across both locations.
- Around line 13-26: In loadAll(), currently the Data and JSON decoding failures
for files in themesDirectory are silently ignored; update the compactMap closure
(and/or replace try? with do/catch) to catch and log failures when reading Data
or decoding StoredTheme (include the file URL/path, the
error.localizedDescription or error object, and mention StoredTheme and
TerminalColorScheme) so corrupted/incompatible JSON files are diagnosable while
still skipping invalid files.
In `@Sources/App/Theme/ThemeRegistry.swift`:
- Around line 105-114: The importItermcolors(from:) path currently creates and
registers an ImportedTerminalTheme and calls importedThemeStore.save(scheme)
which will overwrite an existing theme with the same name; change it to detect
name collisions before saving/registration by checking the registry for an
existing theme with scheme.name (use the existing registry lookup used by
register(_:)), and if a collision exists either (a) append a unique numeric
suffix to scheme.name (e.g., "Name (1)") and update the scheme and props
accordingly before calling importedThemeStore.save and register, or (b) surface
a user-facing conflict resolution (throw a specific error or return a status so
the caller can prompt). Ensure the chosen path updates the ImportedTerminalTheme
instance and persisted scheme to avoid accidental overwrites when calling
importedThemeStore.save and register(theme).
- Around line 117-123: The removeImportedTheme(id:) function currently swallows
errors from importedThemeStore.delete(name:) with try?, so change that to a
do-catch: call try importedThemeStore.delete(name: displayName) inside a do
block and catch any thrown error, then log the failure using the AppLog category
(e.g. AppLog.theme.error) including the displayName and error details to surface
filesystem/persistence issues; keep the existing removal of themes[id] and
themeOrder logic intact and only attempt the delete after confirming the theme
is an ImportedTerminalTheme.
In `@Sources/App/Views/SettingsView.swift`:
- Around line 1247-1251: The corner radius is using
themeProvider.cardCornerRadius which can differ from the active theme; update
the RoundedRectangle constructors and overlay stroke to use the active theme's
radius (theme.cardCornerRadius) instead of themeProvider.cardCornerRadius so the
button visuals (RoundedRectangle(...), overlay
RoundedRectangle(...).stroke(...)) use a consistent corner radius for
isSelected/isHovering states.
- Around line 1227-1236: The inline remove Button (guarded by isImported) calls
ThemeRegistry.shared.removeImportedTheme(id: themeProvider.id) with no
confirmation; add a confirmation step before invoking removeImportedTheme:
introduce a `@State` Bool like showConfirmDelete, change the Button action to set
that flag, present an Alert (or confirmationDialog) tied to showConfirmDelete
that, on confirmation, calls ThemeRegistry.shared.removeImportedTheme(id:
themeProvider.id) and dismisses the alert; alternatively implement
swipe-to-delete on the list that shows the same confirmation before calling
removeImportedTheme. Ensure you reference the existing isImported check, the
Button, themeProvider.id and the removeImportedTheme call so the confirmation
wraps exactly that removal.
In `@Sources/Infrastructure/TerminalImport/ITermColorsParser.swift`:
- Around line 48-60: The extractColor(from:key:) function currently substitutes
missing "Red Component", "Green Component", and "Blue Component" with 0.0 (and
"Alpha Component" with 1.0) which can silently produce black colors for
partially corrupted plists; add a brief doc comment directly above the private
static func extractColor(...) describing this fallback behavior (which
components default to which values), why it is kept for legacy compatibility,
and note that callers should expect possible black colors from incomplete data
(mention ITermColorsParserError.missingColor as the error thrown when the entire
color dictionary is absent).
In `@Sources/Infrastructure/TerminalImport/TerminalColorScheme.swift`:
- Around line 38-54: The ANSI color accessor properties (black, red, ...,
brightWhite) index into ansiColors without bounds checks and will crash if
ansiColors is not length 16; add a doc comment on the computed properties (or on
the TerminalColorScheme type) stating they require ansiColors to contain exactly
16 entries and that callers must validate isValid (used by
TerminalThemeGenerator) before accessing these accessors, referencing the
ansiColors storage and the isValid precondition used by TerminalThemeGenerator
to make the contract explicit.
In `@Tests/InfrastructureTests/TerminalImport/TerminalColorSchemeTests.swift`:
- Around line 9-17: The test `RGBColor encodes and decodes correctly` currently
asserts exact equality on floating-point fields which can be brittle; update the
assertions in that test to use approximate comparisons (e.g., check absolute
difference < 1e-4 or use a provided isApproximatelyEqual helper) for
decoded.red, decoded.green, decoded.blue, and decoded.alpha when comparing to
0.114, 0.145, 0.169, and 1.0 respectively so the test compares
TerminalColorScheme.RGBColor floating values robustly after JSON round-trip.
🪄 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: 51b9c5c1-ae40-4de7-976c-e350bdf27520
📒 Files selected for processing (15)
README.mdSources/App/Settings/ThemeImportView.swiftSources/App/Theme/AppThemeProvider.swiftSources/App/Theme/ImportedTerminalTheme.swiftSources/App/Theme/ImportedThemeStore.swiftSources/App/Theme/ThemeEnvironment.swiftSources/App/Theme/ThemeRegistry.swiftSources/App/Views/SettingsView.swiftSources/Infrastructure/TerminalImport/ITermColorsParser.swiftSources/Infrastructure/TerminalImport/TerminalColorScheme.swiftSources/Infrastructure/TerminalImport/TerminalThemeGenerator.swiftTests/InfrastructureTests/TerminalImport/ITermColorsParserTests.swiftTests/InfrastructureTests/TerminalImport/TerminalColorSchemeTests.swiftTests/InfrastructureTests/TerminalImport/TerminalThemeGeneratorTests.swiftdocs/architecture/THEME_DESIGN.md
Comment on lines
+32
to
+33
| public static func generate(from scheme: TerminalColorScheme) -> GeneratedThemeProperties { | ||
| precondition(scheme.isValid, "TerminalColorScheme must have exactly 16 ANSI colors") |
Contributor
There was a problem hiding this comment.
precondition will crash the app if a persisted theme has corrupted data.
Per the relevant snippet from ThemeRegistry.swift:96-103, loadImportedThemes() calls generate(from:) without first checking scheme.isValid. If a user's persisted JSON file has an incomplete ansiColors array (e.g., due to file corruption or schema migration), the app will crash on launch.
🛡️ Proposed fix: Return nil or throw instead of precondition
-public static func generate(from scheme: TerminalColorScheme) -> GeneratedThemeProperties {
- precondition(scheme.isValid, "TerminalColorScheme must have exactly 16 ANSI colors")
+public static func generate(from scheme: TerminalColorScheme) -> GeneratedThemeProperties? {
+ guard scheme.isValid else {
+ return nil
+ }Then update callers to handle the optional:
// In ThemeRegistry.loadImportedThemes()
for (scheme, _) in importedThemeStore.loadAll() {
guard let props = TerminalThemeGenerator.generate(from: scheme) else {
AppLog.ui.warning("Skipping invalid imported theme: \(scheme.name)")
continue
}
let theme = ImportedTerminalTheme(props: props, scheme: scheme)
register(theme)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Infrastructure/TerminalImport/TerminalThemeGenerator.swift` around
lines 32 - 33, The generate(from:) function in TerminalThemeGenerator currently
calls precondition(scheme.isValid) which will crash on corrupted persisted
themes; change generate(from: TerminalColorScheme) to return an optional
GeneratedThemeProperties? (or throw) instead of asserting, returning nil when
scheme.isValid is false or when the ansiColors array is malformed, and update
callers such as ThemeRegistry.loadImportedThemes() (where
importedThemeStore.loadAll() is iterated) to guard-let the result, log a warning
(e.g. "Skipping invalid imported theme: \(scheme.name)"), and continue; ensure
ImportedTerminalTheme initialization only happens when props is non-nil.
Comment on lines
+34
to
+40
| let sanitizedId = scheme.name | ||
| .lowercased() | ||
| .replacingOccurrences(of: " ", with: "-") | ||
| .replacingOccurrences(of: "[^a-z0-9\\-]", with: "", options: .regularExpression) | ||
|
|
||
| return GeneratedThemeProperties( | ||
| id: "imported-\(sanitizedId)", |
Contributor
There was a problem hiding this comment.
Theme ID collision risk from name sanitization.
Two .itermcolors files with names like "My Theme!" and "My-Theme!" will both sanitize to imported-my-theme. Per the relevant snippet from ThemeRegistry.swift:50-58, the second import will silently replace the first without warning.
Consider either:
- Appending a hash suffix when collision is detected
- Warning the user before overwriting
- Using the full sanitized filename (which may already be unique due to filesystem constraints)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Infrastructure/TerminalImport/TerminalThemeGenerator.swift` around
lines 34 - 40, The current sanitization of scheme.name to sanitizedId can
produce collisions (e.g., "My Theme!" vs "My-Theme!") which causes silent
overwrites when creating GeneratedThemeProperties; modify the creation path that
computes sanitizedId to detect or avoid collisions by appending a short
deterministic hash suffix derived from the original scheme identifier (for
example a 6–8 character hash of scheme.name or the full filename) to the id
(e.g., "imported-\(sanitizedId)-\(shortHash)"), ensuring the unique
GeneratedThemeProperties id and preventing silent replacement in ThemeRegistry;
implement the hash generation where sanitizedId is computed and use that new id
when constructing GeneratedThemeProperties so downstream registration no longer
collides.
…o 'Import Theme' - Add comprehensive docstrings to TerminalColorScheme, RGBColor, ITermColorsParser, TerminalThemeGenerator, GeneratedThemeProperties, ImportedTerminalTheme, ImportedThemeStore, and ThemeRegistry import methods - Rename UI button from 'Import .itermcolors' to 'Import Theme' (more universal naming, consistent with Warp/Ghostty/VS Code conventions) - Target: pass CodeRabbit docstring coverage threshold (was 35%, needs 80%)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
Sources/App/Theme/ImportedThemeStore.swift (2)
27-31: Consider logging when theme files fail to load.Silently skipping corrupted JSON files makes debugging difficult. A log entry would help diagnose import issues.
📝 Proposed enhancement
.compactMap { url -> (TerminalColorScheme, Date)? in - guard let data = try? Data(contentsOf: url), - let entry = try? JSONDecoder().decode(StoredTheme.self, from: data) - else { return nil } + guard let data = try? Data(contentsOf: url) else { + AppLog.ui.warning("Failed to read theme file: \(url.lastPathComponent)") + return nil + } + guard let entry = try? JSONDecoder().decode(StoredTheme.self, from: data) else { + AppLog.ui.warning("Failed to decode theme file: \(url.lastPathComponent)") + return nil + } return (entry.scheme, entry.importedAt) }Based on coding guidelines: Use
AppLogcategories with.warning()for potential issues.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/App/Theme/ImportedThemeStore.swift` around lines 27 - 31, Wrap the Data and JSON decoding inside a do/catch inside the compactMap closure in ImportedThemeStore (the closure returning (TerminalColorScheme, Date)?), and on failure call AppLog.warning(...) with a clear message including the URL.path and the caught error; still return nil for corrupted files so behavior is unchanged but failures are logged. Ensure you reference StoredTheme when decoding and use AppLog's .warning() category so corrupted JSON files are no longer silently skipped.
58-62: Duplicated sanitization logic withTerminalThemeGenerator.The
sanitize()function here is identical to the sanitization inTerminalThemeGenerator.generate()(lines 49-52). Consider extracting to a shared utility to ensure they stay in sync.♻️ Suggested approach
Add a static method to
TerminalColorSchemeor create a shared helper:// In TerminalColorScheme.swift public static func sanitizeName(_ name: String) -> String { name.lowercased() .replacingOccurrences(of: " ", with: "-") .replacingOccurrences(of: "[^a-z0-9\\-]", with: "", options: .regularExpression) }Then both
ImportedThemeStoreandTerminalThemeGeneratorcan callTerminalColorScheme.sanitizeName().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/App/Theme/ImportedThemeStore.swift` around lines 58 - 62, The sanitize(_:) implementation in ImportedThemeStore is duplicated with TerminalThemeGenerator.generate(); extract this logic into a shared static helper (e.g., TerminalColorScheme.sanitizeName(_:)) and replace ImportedThemeStore.sanitize(_:) and the inline sanitization in TerminalThemeGenerator.generate() to call that new helper; update any call sites to use TerminalColorScheme.sanitizeName to keep behavior consistent and remove the duplicated function from ImportedThemeStore.
🤖 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/App/Theme/ImportedThemeStore.swift`:
- Around line 19-33: The docstring promises results "sorted by import date" but
loadAll() currently returns unsorted filesystem order; after you build the
[(TerminalColorScheme, Date)] array (from StoredTheme.importedAt in the
compactMap) sort it by the Date before returning — e.g. call sorted(by:) on the
array of tuples and order by the tuple's Date (importedAt) in the desired
direction (e.g. newest-first or oldest-first) so loadAll() returns entries
sorted by import date.
In `@Sources/App/Theme/ThemeRegistry.swift`:
- Around line 97-103: Validate each loaded scheme before calling
TerminalThemeGenerator.generate(): in loadImportedThemes() iterate
importedThemeStore.loadAll(), check scheme.isValid (or equivalent validation)
and only call TerminalThemeGenerator.generate(from:) for valid schemes; for
invalid/corrupted schemes skip registration and emit a warning via
AppLog.warning(...) including the scheme identifier (e.g., name or UUID) to aid
debugging; ensure you still create ImportedTerminalTheme and register only when
props were successfully generated to avoid the precondition crash.
In `@Sources/Infrastructure/TerminalImport/ITermColorsParser.swift`:
- Around line 62-74: The extractColor method (private static func extractColor)
currently silently defaults missing "Red Component"/"Green Component"/"Blue
Component" to 0.0; update it to validate that those three keys exist in
colorDict and if any are missing throw ITermColorsParserError.missingColor(key)
instead of defaulting to black, and add a unit test that constructs a color
dictionary missing one or more RGB keys to assert the throw; keep the existing
behavior for missing "Alpha Component" (default to 1.0) if desired.
---
Nitpick comments:
In `@Sources/App/Theme/ImportedThemeStore.swift`:
- Around line 27-31: Wrap the Data and JSON decoding inside a do/catch inside
the compactMap closure in ImportedThemeStore (the closure returning
(TerminalColorScheme, Date)?), and on failure call AppLog.warning(...) with a
clear message including the URL.path and the caught error; still return nil for
corrupted files so behavior is unchanged but failures are logged. Ensure you
reference StoredTheme when decoding and use AppLog's .warning() category so
corrupted JSON files are no longer silently skipped.
- Around line 58-62: The sanitize(_:) implementation in ImportedThemeStore is
duplicated with TerminalThemeGenerator.generate(); extract this logic into a
shared static helper (e.g., TerminalColorScheme.sanitizeName(_:)) and replace
ImportedThemeStore.sanitize(_:) and the inline sanitization in
TerminalThemeGenerator.generate() to call that new helper; update any call sites
to use TerminalColorScheme.sanitizeName to keep behavior consistent and remove
the duplicated function from ImportedThemeStore.
🪄 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: 00418dd1-7a8a-4838-86d1-2efdb8db544a
📒 Files selected for processing (7)
Sources/App/Settings/ThemeImportView.swiftSources/App/Theme/ImportedTerminalTheme.swiftSources/App/Theme/ImportedThemeStore.swiftSources/App/Theme/ThemeRegistry.swiftSources/Infrastructure/TerminalImport/ITermColorsParser.swiftSources/Infrastructure/TerminalImport/TerminalColorScheme.swiftSources/Infrastructure/TerminalImport/TerminalThemeGenerator.swift
🚧 Files skipped from review as they are similar to previous changes (3)
- Sources/App/Settings/ThemeImportView.swift
- Sources/App/Theme/ImportedTerminalTheme.swift
- Sources/Infrastructure/TerminalImport/TerminalColorScheme.swift
Comment on lines
+19
to
+33
| /// Load all imported color schemes from disk, sorted by import date. | ||
| public func loadAll() -> [(TerminalColorScheme, Date)] { | ||
| guard let files = try? FileManager.default.contentsOfDirectory( | ||
| at: themesDirectory, includingPropertiesForKeys: nil | ||
| ) else { return [] } | ||
|
|
||
| return files | ||
| .filter { $0.pathExtension == "json" } | ||
| .compactMap { url -> (TerminalColorScheme, Date)? in | ||
| guard let data = try? Data(contentsOf: url), | ||
| let entry = try? JSONDecoder().decode(StoredTheme.self, from: data) | ||
| else { return nil } | ||
| return (entry.scheme, entry.importedAt) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Docstring claims results are sorted by import date, but no sorting is applied.
The docstring says "sorted by import date" but loadAll() returns entries in arbitrary filesystem order (from contentsOfDirectory).
🔧 Proposed fix: Add sorting
return files
.filter { $0.pathExtension == "json" }
.compactMap { url -> (TerminalColorScheme, Date)? in
guard let data = try? Data(contentsOf: url),
let entry = try? JSONDecoder().decode(StoredTheme.self, from: data)
else { return nil }
return (entry.scheme, entry.importedAt)
}
+ .sorted { $0.1 < $1.1 }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/App/Theme/ImportedThemeStore.swift` around lines 19 - 33, The
docstring promises results "sorted by import date" but loadAll() currently
returns unsorted filesystem order; after you build the [(TerminalColorScheme,
Date)] array (from StoredTheme.importedAt in the compactMap) sort it by the Date
before returning — e.g. call sorted(by:) on the array of tuples and order by the
tuple's Date (importedAt) in the desired direction (e.g. newest-first or
oldest-first) so loadAll() returns entries sorted by import date.
Comment on lines
+97
to
+103
| private func loadImportedThemes() { | ||
| for (scheme, _) in importedThemeStore.loadAll() { | ||
| let props = TerminalThemeGenerator.generate(from: scheme) | ||
| let theme = ImportedTerminalTheme(props: props, scheme: scheme) | ||
| register(theme) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Add validation before calling generate() to prevent crash on corrupted data.
loadImportedThemes() directly passes each loaded scheme to TerminalThemeGenerator.generate(), which calls precondition(scheme.isValid). If a persisted JSON file has corrupted ansiColors (fewer than 16 entries), the app will crash on launch.
🛡️ Proposed fix: Guard against invalid schemes
private func loadImportedThemes() {
for (scheme, _) in importedThemeStore.loadAll() {
+ guard scheme.isValid else {
+ AppLog.ui.warning("Skipping invalid imported theme: \(scheme.name)")
+ continue
+ }
let props = TerminalThemeGenerator.generate(from: scheme)
let theme = ImportedTerminalTheme(props: props, scheme: scheme)
register(theme)
}
}Based on coding guidelines: Use AppLog categories for logging, specifically .warning() for potential issues.
📝 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
| private func loadImportedThemes() { | |
| for (scheme, _) in importedThemeStore.loadAll() { | |
| let props = TerminalThemeGenerator.generate(from: scheme) | |
| let theme = ImportedTerminalTheme(props: props, scheme: scheme) | |
| register(theme) | |
| } | |
| } | |
| private func loadImportedThemes() { | |
| for (scheme, _) in importedThemeStore.loadAll() { | |
| guard scheme.isValid else { | |
| AppLog.ui.warning("Skipping invalid imported theme: \(scheme.name)") | |
| continue | |
| } | |
| let props = TerminalThemeGenerator.generate(from: scheme) | |
| let theme = ImportedTerminalTheme(props: props, scheme: scheme) | |
| register(theme) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/App/Theme/ThemeRegistry.swift` around lines 97 - 103, Validate each
loaded scheme before calling TerminalThemeGenerator.generate(): in
loadImportedThemes() iterate importedThemeStore.loadAll(), check scheme.isValid
(or equivalent validation) and only call TerminalThemeGenerator.generate(from:)
for valid schemes; for invalid/corrupted schemes skip registration and emit a
warning via AppLog.warning(...) including the scheme identifier (e.g., name or
UUID) to aid debugging; ensure you still create ImportedTerminalTheme and
register only when props were successfully generated to avoid the precondition
crash.
Comment on lines
+62
to
+74
| private static func extractColor( | ||
| from plist: [String: Any], | ||
| key: String | ||
| ) throws -> TerminalColorScheme.RGBColor { | ||
| guard let colorDict = plist[key] as? [String: Any] else { | ||
| throw ITermColorsParserError.missingColor(key) | ||
| } | ||
| let red = (colorDict["Red Component"] as? Double) ?? 0.0 | ||
| let green = (colorDict["Green Component"] as? Double) ?? 0.0 | ||
| let blue = (colorDict["Blue Component"] as? Double) ?? 0.0 | ||
| let alpha = (colorDict["Alpha Component"] as? Double) ?? 1.0 | ||
| return TerminalColorScheme.RGBColor(red: red, green: green, blue: blue, alpha: alpha) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if any .itermcolors test fixtures have color dicts without component keys
# to understand if this edge case is covered in tests
fd -e swift -p 'Test' --exec grep -l 'ITermColorsParser' {} \; | head -5 | xargs -I{} cat {}Repository: tddworks/ClaudeBar
Length of output: 11472
Edge case of missing color component keys is untested and unvalidated.
The extractColor method silently defaults missing Red Component, Green Component, and Blue Component keys to 0.0 (black). While tests cover missing Alpha Component (correctly defaults to 1.0) and missing entire color dictionaries (correctly throws), they do not cover color dictionaries that exist but lack RGB component keys. This could silently produce black for corrupted or malformed .itermcolors files.
Consider either:
- Adding a test case for color dicts with missing RGB components to document intended behavior
- Validating that color dicts contain all required RGB keys and throwing
missingColorif incomplete
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Infrastructure/TerminalImport/ITermColorsParser.swift` around lines
62 - 74, The extractColor method (private static func extractColor) currently
silently defaults missing "Red Component"/"Green Component"/"Blue Component" to
0.0; update it to validate that those three keys exist in colorDict and if any
are missing throw ITermColorsParserError.missingColor(key) instead of defaulting
to black, and add a unit test that constructs a color dictionary missing one or
more RGB keys to assert the throw; keep the existing behavior for missing "Alpha
Component" (default to 1.0) if desired.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #168 +/- ##
==========================================
+ Coverage 80.37% 80.48% +0.10%
==========================================
Files 106 109 +3
Lines 7879 7994 +115
==========================================
+ Hits 6333 6434 +101
- Misses 1546 1560 +14
🚀 New features to boost your workflow:
|
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.
Add the ability to import .itermcolors files into ClaudeBar, automatically generating a full theme from 16 ANSI colors + background/foreground.
Architecture:
Also adds customFontName + font() helper to AppThemeProvider protocol, and makes ThemeOptionButton dynamic (works with both built-in and imported themes).
Compatible with 450+ pre-made schemes from mbadolato/iTerm2-Color-Schemes.
Includes 21 tests across 3 test suites.
Summary by CodeRabbit
New Features
Tests
Documentation