feat: Add terminal theme system with built-in presets

[kshivang]

Summary

• Add comprehensive theme system with 10 built-in presets (Dracula, Nord, Solarized, Gruvbox, One Dark, Monokai, Tokyo Night, Catppuccin Mocha)
• New Themes settings section with visual theme grid selector
• Full 16-color ANSI palette editor for custom themes
• Terminal color editors (foreground, background, cursor, selection, search, hyperlink)
• Custom theme support with persistence to ~/.bossterm/themes.json
• Dynamic color palette integration - theme colors apply to terminal rendering

New Files

File	Description
settings/theme/Theme.kt	Theme data class with 16 ANSI colors
settings/theme/BuiltinThemes.kt	10 preset themes
settings/theme/ThemeManager.kt	Theme management singleton
settings/sections/ThemeSettingsSection.kt	Theme settings UI

Modified Files

• ColorPaletteImpl1.kt - Added factory methods for dynamic palettes
• ColorUtils.kt - Now uses active theme's ANSI palette
• TerminalSettings.kt - Added activeThemeId field
• SettingsCategory.kt - Added THEMES category
• VisualSettingsSection.kt - Removed redundant color settings

Test plan

• Open Settings > Themes
• Verify theme grid shows all 10 built-in themes with previews
• Select different themes and verify terminal colors change
• Create a custom theme and verify it persists after restart
• Edit ANSI colors and verify they apply to terminal output
• Delete a custom theme

🤖 Generated with Claude Code

[claude]

PR Review: Terminal Theme System

Overview

This is an excellent feature addition that brings comprehensive theming capabilities to BossTerm. The implementation is well-structured with 10 built-in themes, custom theme support, and a polished UI. Overall quality is high, with some areas for improvement.

---

✅ Strengths

1. Solid Architecture

• Clean separation: Theme (data), ThemeManager (business logic), BuiltinThemes (presets)
• Proper use of Kotlin Flow for reactive state management
• Singleton pattern with thread-safe initialization in ThemeManager
• JSON serialization for persistence is straightforward and reliable

2. Excellent UI/UX Design

• Visual theme grid with live previews is intuitive
• ANSI palette editor with color swatches is user-friendly
• Terminal color editors for foreground/background/cursor/selection
• Custom theme creation with proper naming workflow

3. Integration with Existing Systems

• ColorUtils.kt properly integrates theme colors with ColorPalette
• Theme colors apply dynamically through convertTerminalColor()
• Settings persistence through TerminalSettings.activeThemeId

4. Code Quality

• Comprehensive documentation with clear comments
• Proper error handling (try-catch with fallbacks)
• Immutable data structures (data class Theme)
• UUID generation for custom theme IDs prevents conflicts

---

🔴 Critical Issues

1. Thread Safety Issue in ColorUtils.kt (Lines 21-28)

Problem: getCurrentPalette() accesses ThemeManager.instance.currentTheme.value on every color conversion call, which happens frequently during rendering (potentially 60+ times per second). StateFlow access itself is thread-safe, but the palette creation from the theme happens on every call without caching.

Impact:

• Performance overhead from repeated palette creation
• Potential for race conditions if theme changes mid-render
• GC pressure from allocating new ColorPalette instances

Recommendation:

object ColorUtils {
    private var cachedTheme: Theme? = null
    private var cachedPalette: ColorPalette = defaultPalette
    
    @Synchronized
    private fun getCurrentPalette(): ColorPalette {
        val currentTheme = try {
            ThemeManager.instance.currentTheme.value
        } catch (e: Exception) {
            return defaultPalette
        }
        
        // Only recreate palette if theme changed
        if (cachedTheme != currentTheme) {
            cachedTheme = currentTheme
            cachedPalette = createPaletteFromTheme(currentTheme)
        }
        
        return cachedPalette
    }
}

2. Memory Leak Risk: ThemeManager Holds SettingsManager Reference

Problem: ThemeManager is a singleton that holds a reference to SettingsManager (line 30). If SettingsManager is recreated (e.g., during testing or hot reload), the old instance remains referenced by ThemeManager.

Impact:

• Memory leak in long-running sessions
• Potential stale settings

Recommendation: Either:

• Make SettingsManager access lazy: private val settingsManager: SettingsManager get() = SettingsManager.instance
• Or ensure both singletons have proper lifecycle management

3. Unchecked Array Access in ColorPaletteImpl.kt (Line 17)

Problem: withColor() requires index 0-15 but uses require() which throws IllegalArgumentException. If this ever gets called with invalid input (e.g., from user-edited JSON), the app crashes.

Recommendation: Use clamping instead:

fun withColor(index: Int, color: Color): ColorPaletteImpl {
    val safeIndex = index.coerceIn(0, 15)
    val newColors = myColors.copyOf()
    newColors[safeIndex] = color
    return ColorPaletteImpl(newColors)
}

---

⚠️ Moderate Issues

4. No Validation for Theme Color Hex Strings

Problem: Theme.parseColor() (lines 149-161) silently returns Color.White on parse failure. If a user manually edits themes.json with invalid hex strings, they'll get white colors with no indication of the error.

Recommendation: Log parsing errors:

fun parseColor(hex: String): Color {
    return try {
        // ... existing code ...
    } catch (e: Exception) {
        System.err.println("Invalid color hex: $hex - ${e.message}")
        Color.White
    }
}

5. Missing Theme Import/Export UI

Observation: ThemeManager has exportTheme() and importTheme() methods, but ThemeSettingsSection.kt doesn't expose these to users. This is a great feature that should be accessible via the UI.

Recommendation: Add import/export buttons to the theme settings UI.

6. No Unit Tests

Problem: No test files found for the theme system. Given the complexity (JSON serialization, state management, color parsing), tests are essential.

Recommendation: Add tests for:

• Theme serialization/deserialization
• ThemeManager CRUD operations
• Color parsing edge cases (malformed hex, 6 vs 8 char formats)
• Theme switching with settings persistence

---

💡 Minor Issues & Suggestions

7. Hardcoded Color in ThemeSettingsSection.kt (Line 98)

backgroundColor = Color(0xFFE04040)  // Delete button color

Should use a theme constant (e.g., SettingsTheme.DangerColor) for consistency.

8. Magic Number in ColorUtils.kt (Line 47)

8 -> cleanHex.substring(2).toLong(16).toInt() // Skip alpha

Should use named constant: const val ALPHA_HEX_LENGTH = 8

9. Unused withColor() Method in ColorPaletteImpl

The new withColor() method (lines 14-22) is added but not used anywhere in the PR. Either use it or remove it.

10. Theme Preview Could Show More

The mini terminal preview in ThemePreviewCard (lines 312-332) only shows 3 lines. Consider showing a few more ANSI colors (red, green, yellow, blue) to better preview the theme.

---

🔒 Security Considerations

11. File Path Injection (Low Risk)

ThemeManager.themesFile uses user.home system property (line 52). While generally safe, consider validating the path doesn't escape .bossterm directory when reading/writing.

12. JSON Deserialization Safety

Using ignoreUnknownKeys = true (line 48) is good for forward compatibility. However, consider adding a version field to CustomThemesContainer for future migration support.

---

📊 Performance Considerations

13. LazyVerticalGrid Height Limit

modifier = Modifier.heightIn(max = 300.dp)

With 10+ themes, the grid might need scrolling. Consider making this dynamic based on available space or adding pagination.

14. Compose Recomposition Optimization

ThemeSettingsSection collects currentTheme and customThemes flows (lines 44-45). Ensure these don't trigger unnecessary recompositions when theme colors change during editing.

---

✅ Test Plan Validation

The test plan in the PR description is comprehensive and covers:

• ✅ Theme selection UI
• ✅ Theme persistence
• ✅ ANSI color editing
• ✅ Custom theme CRUD

Additional testing recommended:

• Test theme switching while terminal is actively rendering colored output
• Test with malformed themes.json file
• Test theme switching across multiple tabs (if applicable)
• Test with extremely long theme names (UI overflow?)

---

📝 Documentation

Missing:

• User-facing documentation on how to create/share themes
• Developer notes on adding new built-in themes
• Migration guide for users with custom color settings from previous versions

---

🎯 Recommendation

Status: ✅ Approve with changes

This is a high-quality feature that significantly enhances BossTerm's customization options. The core architecture is solid, and the UI is polished. However, the thread safety issue in ColorUtils (#1) should be addressed before merging to avoid performance problems.

Priority fixes before merge:

1. ✅ Cache palette in ColorUtils.getCurrentPalette() (Critical)
2. ✅ Fix ThemeManager memory leak risk (Critical)
3. ⚠️ Add basic unit tests for theme management (High)

Post-merge enhancements:

• Add import/export UI
• Improve theme preview
• Add documentation

---

Great work overall! The theme system is a valuable addition to BossTerm. 🎨

References to CLAUDE.md:

• ✅ Follows "avoid over-engineering" principle (focused on current requirements)
• ✅ Proper error handling with fallbacks
• ✅ Clean separation of concerns
• ⚠️ Could use more defensive programming for user-editable JSON
• ❌ Missing test coverage (contrary to project standards)

[claude]

Code Review: Terminal Theme System

This is an impressive feature addition that brings comprehensive theming capabilities to BossTerm. The implementation is well-structured and follows good architectural patterns.

---

Strengths

1. Excellent Architecture

• Clean separation of concerns: Theme (data), ThemeManager (logic), BuiltinThemes (presets)
• Singleton pattern with thread-safe lazy initialization in ThemeManager
• Proper use of StateFlow for reactive theme updates
• JSON persistence with graceful error handling

2. Code Quality

• Well-documented with clear KDoc comments
• Comprehensive feature set: 10 built-in themes plus custom theme support
• Export/import functionality for theme sharing
• Proper validation (e.g., require(index in 0..15))
• Good use of Kotlin idioms (@transient, computed properties, data classes)

3. UI/UX

• Visual theme grid with live previews - excellent discoverability
• Individual ANSI color editing with color pickers
• Terminal color customization (foreground, background, cursor, selection, etc.)
• Theme deletion with confirmation dialogs

4. Integration

• Seamless integration with existing ColorPaletteImpl via new factory methods
• ColorUtils properly updated to use theme-aware palettes
• Settings persistence through TerminalSettings.activeThemeId

---

Critical Issues

1. Dependency Version Upgrades (High Priority)

The PR includes significant dependency upgrades that should be in a separate PR:

• Compose: 1.7.1 to 1.9.3
• Coroutines: 1.9.0 to 1.10.2
• pty4j: 0.12.13 to 0.13.9
• Ktor: 2.3.7 to 3.3.2 (MAJOR VERSION BUMP!)

Why this is risky:

• Ktor 2.3.7 to 3.3.2 is a major version upgrade with breaking changes
• Compose 1.7.1 to 1.9.3 may introduce rendering behavior changes
• If CI fails or bugs appear, it's unclear whether the theme system or dependency upgrades are at fault
• Hard to bisect issues or revert changes

Recommendation: Create two separate PRs - one for dependency upgrades (test thoroughly in isolation), and one for the theme system feature.

2. Window Transparency Feature Scope Creep (Medium Priority)

This PR introduces undecorated windows, custom title bars, and transparency - features not mentioned in the PR title.

Issues:

• Custom title bar (TransparentWindowTitleBar.kt, 122 lines) is a separate feature
• Window transparency (WindowTransparency.kt, 83 lines) is a separate feature
• WindowTransparency.configureMacOSBlur() doesn't actually implement blur - the function name promises macOS blur but doesn't deliver it

Recommendation: Either split into separate PRs (theme system + window chrome + transparency) OR update PR title/description to reflect full scope. Also remove windowBlur setting or document that it's a placeholder for future work.

3. Missing Test Coverage (High Priority)

No automated tests for critical functionality:

• ThemeManager.createCustomTheme()
• ThemeManager.updateCustomTheme()
• ThemeManager.deleteCustomTheme()
• Theme.withAnsiColor() edge cases
• Theme.parseColor() malformed input
• ColorUtils.convertTerminalColor() with theme changes
• JSON serialization/deserialization
• Theme persistence across app restarts

Recommendation: Add unit tests for ThemeManager, Theme, and ColorUtils (minimum 80% coverage)

---

Other Issues

Security (Low Risk)

ThemeManager writes to ~/.bossterm/themes.json with no file size validation or atomic writes.

Recommendation:

• Add file size limit check (e.g., 10 MB max)
• Use atomic writes: write to .themes.json.tmp, then rename

Performance

applyTheme() writes to disk synchronously, which may lag on slow disks.

Recommendation: Make settings write async using Dispatchers.IO

Documentation

Missing user documentation for how to use themes, where they are stored, and how to export/import them.

---

Final Recommendation

Action Required Before Merge:

1. CRITICAL: Split dependency upgrades into a separate PR
2. CRITICAL: Add unit tests for ThemeManager and Theme
3. IMPORTANT: Clarify PR scope or split window chrome into separate PR
4. IMPORTANT: Remove windowBlur setting or document it's a placeholder
5. OPTIONAL: Add atomic file writes for themes.json
6. OPTIONAL: Document theme system in CLAUDE.md

Overall Assessment: Strong feature with excellent architecture, but needs scope clarification and test coverage before merge. The dependency upgrades are a blocker - they must be in a separate PR.

Estimated Risk: Medium (due to dependency upgrades + window chrome changes bundled together)

Estimated Time to Fix: 2-4 hours (split PRs + add basic tests)

---

Great work on this feature! The theme system itself is production-ready - just needs the process/packaging improvements above.

Generated with Claude Code
Co-Authored-By: Claude

[claude]

PR Review: Terminal Theme System

Overview

This is an excellent addition to BossTerm! The theme system is well-architected with clean separation of concerns, good Kotlin/Compose patterns, and thoughtful UX design. The implementation includes 10 beautiful built-in themes and a robust custom theme system with persistence.

✅ Strengths

Architecture & Code Quality

• Clean separation: Theme (data), ThemeManager (business logic), BuiltinThemes (constants), UI components well separated
• Singleton pattern: Proper thread-safe double-checked locking in ThemeManager
• Reactive state: Excellent use of StateFlow for theme changes with Compose integration
• Serialization: Proper use of Kotlinx serialization with @Transient for computed properties
• Error handling: Graceful fallbacks throughout (e.g., ColorUtils.getCurrentPalette())

Theme System Design

• Comprehensive color coverage: All terminal colors (fg, bg, cursor, selection, search, hyperlink) + full 16-color ANSI palette
• Excellent theme choices: Dracula, Nord, Solarized, Gruvbox, One Dark, Monokai, Tokyo Night, Catppuccin Mocha
• Flexible API: withAnsiColor(), getAnsiColor(), helper methods make theme manipulation elegant
• Import/Export: JSON import/export capability enables theme sharing

UI/UX

• Visual theme grid: Mini terminal previews make theme selection intuitive
• Live editing: Inline color editors with immediate visual feedback
• Custom theme support: Users can fork built-in themes and customize
• Protection: Built-in themes immutable, modifications auto-create custom copies

🔍 Issues & Concerns

1. CRITICAL: Breaking Main.kt Changes

The window transparency changes are out of scope and break functionality:

Problem: Lines 116-117 set undecorated = true, transparent = true unconditionally

Issues:

• Users lose native window controls (close/minimize/maximize buttons)
• Platform compatibility issues (macOS vs Linux vs Windows)
• Forced transparency even when backgroundOpacity = 1.0f
• Custom title bar incomplete (missing double-click, right-click menu)
• No feature flag to opt out

Recommendation: Remove transparency changes from this PR. They need separate PR with feature flags and platform testing.

2. Theme Color Application Incomplete

ThemeManager.applyTheme() only syncs 5 colors to settings. Missing: cursor, cursorText, selectionText

3. Performance: Theme Palette Recreation

ColorUtils.getCurrentPalette() recreates palette on every call (invoked per character at 60fps)

Fix: Cache palette in ThemeManager, invalidate on theme change

4. Transparency Settings Orphaned

backgroundOpacity and windowBlur in TerminalSettings are unused (tied to removed transparency feature)

🎯 Recommendations

Before Merge (Critical)

1. Remove window transparency: Main.kt lines 116-117, 315-391, and both window/ files
2. Fix palette caching: Add caching to ColorUtils
3. Sync cursor colors: Update applyTheme() to include cursor settings
4. Remove transparency settings: Or document as future use

Post-Merge Enhancements

• Add theme import/export UI
• Theme preview in real terminal
• Theme hotkey (Cmd+T?)

📝 Summary

High-quality feature with excellent architecture. Core theme system is production-ready.

Recommendation: Approve with changes - address transparency issues and performance concern, then merge.

---

Code Quality: 9/10
Architecture: 9/10
UX Design: 10/10
Testing: 6/10
Overall: 8.5/10

🤖 Generated with Claude Code

Co-Authored-By: Claude noreply@anthropic.com