feat(ui): theming support with config autosave#104
Conversation
Add preset themes (dark, light, nord, dracula, gruvbox, catppuccin) with runtime switching via :theme command and --theme CLI flag. Theme System: - 6 preset themes with full color customization - Config support: preset name or object with overrides - :theme command for persistent change (when autosave enabled) - --theme CLI flag for transient theme selection - ThemeChangedMsg for runtime style reload across all views Config Autosave: - :autosave on/off command with flash feedback - --autosave/--no-autosave CLI flags - Multi-profile support with startup.profiles array format - YAML patching to preserve comments and unknown keys Table Improvements: - Migrate to lipgloss/v2/table for proper cell theming - Extract TableCursor and NewTableStyleFunc for code reuse - Consistent table styling across resource browser and tag search BREAKING CHANGE: Config key renamed from 'persistence' to 'autosave' BREAKING CHANGE: CLI flags renamed from --persist to --autosave BREAKING CHANGE: Config 'startup.profile' (string) replaced by 'startup.profiles' (array) - Old format still readable for backward compatibility - New saves use profiles array format
This comment was marked as resolved.
This comment was marked as resolved.
- Add RWMutex to Current()/SetTheme() for thread safety - Show warning flash when theme save fails with autosave enabled - Add concurrent access test with race detector
This comment was marked as resolved.
This comment was marked as resolved.
Style helpers now call Current() which acquires RLock instead of directly accessing the module-level current variable.
This comment was marked as resolved.
This comment was marked as resolved.
Save methods now hold mutex during both in-memory update and file I/O to prevent concurrent saves from overwriting each other's changes. Renamed patchConfig to patchConfigLocked to indicate caller holds lock.
PR Review: Theming Support with Config AutosaveGreat work on this comprehensive feature addition! The implementation is well-structured with good test coverage. Here are my findings: ✅ Strengths
Code Quality ObservationsStrong Points:
Minor Issues:
|
Potential Issues
SecurityNo major security concerns identified. Good practices observed:
Minor consideration: Config files may contain profile names which could leak info if shared. Performance
|
Test CoverageExcellent coverage overall:
Missing test scenarios:
DocumentationStrong documentation:
Suggestions:
Specific Code Suggestionsinternal/config/file.go:366-375 - Consider adding theme validation before saving to prevent invalid theme names from being persisted. internal/app/app.go:219 - Consider logging failed theme persistence at debug level instead of warn, as this is not critical. |
Migration Guide ValidationThe breaking changes section is excellent. Verified:
SummaryThis is a high-quality PR with solid architecture, good testing, and thoughtful backward compatibility. The concerns raised are mostly minor and do not block merging. Recommendation: APPROVE with optional improvements The race condition in config saves is the only notable issue, but given this is a single-user CLI tool, it is acceptable to document as a known limitation rather than block the PR. Priority fixes optional:
Nice-to-have improvements:
Great work overall! |
Summary
:themecommand and--themeCLI flag for theme switching:autosave on/offcommand and--autosaveCLI flagsBreaking Changes⚠️
persistence.enabledautosave.enabled--persist/--no-persist--autosave/--no-autosavestartup.profile: namestartup.profiles: [name]New Commands
:theme <name>- Switch theme (persisted if autosave enabled):autosave on/off- Toggle config autosaveNew CLI Flags
-t, --theme <name>- Set theme for session (transient)--autosave/--no-autosave- Enable/disable config persistenceTesting
task testpassestask lintpassestask buildpassesCloses #96