Skip to content

Release v0.8.0#105

Merged
yimsk merged 5 commits intomainfrom
develop
Jan 5, 2026
Merged

Release v0.8.0#105
yimsk merged 5 commits intomainfrom
develop

Conversation

@yimsk
Copy link
Copy Markdown
Contributor

@yimsk yimsk commented Jan 5, 2026

Release v0.8.0

New Features

  • Theming Support - 6 preset themes: dark, light, nord, dracula, gruvbox, catppuccin
  • :theme command - Switch themes at runtime (persisted when autosave enabled)
  • --theme CLI flag - Set theme for session
  • :autosave command - Toggle config persistence on/off
  • --autosave/--no-autosave CLI flags - Control config persistence

Improvements

  • Migrate to lipgloss/v2/table for proper cell theming
  • YAML config patching preserves comments and unknown keys
  • Thread-safe theme access with RWMutex
  • Multi-profile support in config (startup.profiles array)

Breaking Changes ⚠️

Before After Migration
persistence.enabled autosave.enabled Auto-read old key, new saves use autosave
--persist / --no-persist --autosave / --no-autosave Update scripts/aliases
startup.profile (string) startup.profiles (array) Old format still readable

Closes


Note: develop → main PRs use merge commit (not squash)

yimsk added 5 commits January 5, 2026 07:57
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
- 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
Style helpers now call Current() which acquires RLock instead of
directly accessing the module-level current variable.
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.
feat(ui): theming support with config autosave
@claude
Copy link
Copy Markdown

claude Bot commented Jan 5, 2026

PR Review: Release v0.8.0 - Theming Support

Overview

This is a substantial and well-implemented feature PR that adds theming support with 6 preset themes, runtime theme switching, and config autosave functionality. The code quality is high with excellent test coverage and proper concurrency handling.

✅ Strengths

Code Quality & Architecture

  • Excellent separation of concerns: Theme logic cleanly separated in internal/ui/theme.go
  • Strong concurrency safety: Proper use of RWMutex for theme access and config saves
  • YAML patching preserves comments: Smart implementation that reads, patches, and writes YAML while preserving user comments and unknown keys
  • Backward compatibility: Old persistence config key still readable, old startup.profile (string) format supported
  • Atomic file writes: Uses temp file + rename pattern to prevent corruption

Testing

  • Comprehensive test coverage in config/file_test.go and ui/theme_test.go
  • Concurrent access tests with race detector support
  • Edge case handling: Empty configs, invalid colors, negative values, etc.
  • Tests for comment preservation, backward compatibility, and YAML patching

Thread Safety

  • All style helpers call Current() which properly acquires RLock
  • Config saves hold mutex during entire operation (both memory update + file I/O)
  • Renamed patchConfigpatchConfigLocked to indicate caller must hold lock
  • Good use of defensive copying (e.g., append([]string(nil), s.Profiles...))

User Experience

  • 6 well-designed preset themes with proper color palettes
  • CLI flag override support (--theme, --autosave)
  • Runtime commands (:theme, :autosave on/off) with visual feedback
  • Flash messages for save errors when autosave enabled

🔍 Issues & Concerns

1. Potential Race Condition in SaveRegions (Medium Priority)

Location: internal/config/file.go:334-349

The early return when regions is empty bypasses the lock. While this specific case is likely safe (caller owns the slice), it is inconsistent with the locking pattern.

Recommendation: Consider moving the check inside the lock or document why it is safe.

2. SaveProfiles Does Not Handle Empty Profiles (Low Priority)

Location: internal/config/file.go:351-364

SaveProfiles has no early-return check for empty profiles, unlike SaveRegions. This inconsistency could be confusing.

Recommendation: Add the same empty check or document the difference in behavior.

3. Missing Validation for Theme Names (Low Priority)

Location: internal/app/app.go:226-245 and internal/view/command_input.go:296-303

Error handling is good in app.go, but the command input accepts any string without immediate feedback. The theme command handler does not validate the theme name before creating the message.

Recommendation: Add tab completion for theme names or validation in the command input layer.

4. Inconsistent Error Handling in Config Save Paths (Low Priority)

Location: Throughout internal/app/app.go

Some places show user warnings on save failure, others silently log.

Recommendation: Standardize whether config save failures should show user-visible warnings.

5. ParseColor Accepts #RGB but Documentation Unclear (Low Priority)

Location: internal/ui/theme.go:23-54

The ParseColor function expands 3-digit hex colors (#RGB → #RRGGBB), which is great, but this is not documented in the user-facing config documentation.

Recommendation: Update README.md or add examples showing #RGB support.

🎯 Performance Considerations

✅ Good

  • Config saves use atomic writes (temp + rename) - fast and safe
  • YAML patching only parses/writes when saving, not on reads
  • RWMutex allows concurrent reads of theme data
  • Style helpers are lightweight

⚠️ Minor Concerns

  • Multiple saves in quick succession: If a user rapidly changes themes, each save does full YAML parse + write. Consider debouncing if this becomes an issue.
  • Lock held during file I/O: patchConfigLocked holds the write lock during file operations. For slow filesystems, this could block readers. However, this is necessary to prevent concurrent save races, so it is the right tradeoff.

🔒 Security Assessment

✅ No Major Concerns

  • File permissions: Config dir created with 0755, file written with system defaults
  • Path traversal: Uses filepath.Join and validated paths
  • Input sanitization: Theme names validated against preset list
  • YAML injection: Uses proper YAML encoding, not string concatenation

💡 Recommendations

  • Consider setting explicit file permissions (0644) on config file for clarity
  • The color parsing accepts ANSI codes 0-255 and hex - both safe

📊 Test Coverage Assessment

✅ Excellent Coverage

  • config/file_test.go: 682 lines covering backward compatibility, comment preservation, concurrent saves, empty/invalid inputs, partial configs, and YAML patching
  • ui/theme_test.go: 668 lines covering all 6 preset themes, color parsing, invalid color handling, concurrent theme access, config overrides, and CLI overrides

Minor Gaps

  • No integration tests for the :theme and :autosave commands end-to-end
  • No tests for theme switching while views are active (manual testing likely done)

📝 Documentation

✅ Good

  • README updated with theme documentation and examples
  • Breaking changes clearly documented in PR description
  • Migration path provided for renamed config keys

Could Improve

  • Add examples of theme customization (preset + overrides)
  • Document the #RGB shorthand support

🎨 Code Style & Conventions

  • Consistent with existing codebase style
  • Good use of comments for complex logic (YAML patching)
  • Clear function names (patchConfigLocked, GetProfiles)
  • Proper error wrapping with fmt.Errorf

🚀 Recommendations

High Priority

  1. Review the empty-check pattern in SaveRegions for consistency

Medium Priority

  1. Consider adding theme name validation in command input
  2. Standardize user feedback for config save errors

Low Priority

  1. Add theme name tab completion
  2. Document #RGB color support in README
  3. Consider debouncing rapid theme changes (if needed)

📊 Summary

Category Rating Notes
Code Quality ⭐⭐⭐⭐⭐ Excellent architecture, clean separation
Testing ⭐⭐⭐⭐⭐ Comprehensive coverage, edge cases handled
Security ⭐⭐⭐⭐⭐ No concerns, proper validation
Performance ⭐⭐⭐⭐☆ Good, minor lock contention possible
Documentation ⭐⭐⭐⭐☆ Good, could add more examples
Thread Safety ⭐⭐⭐⭐⭐ Excellent concurrency handling

✅ Verdict: APPROVED (with minor suggestions)

This is a high-quality PR that adds significant value with minimal risk. The few issues noted are minor and do not block merging. The code demonstrates excellent engineering practices with proper concurrency, testing, and backward compatibility.

Great work! 🎉

@yimsk yimsk merged commit 0074396 into main Jan 5, 2026
9 checks passed
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