fix(theme): validate and merge user theme files instead of blind override - #260
fix(theme): validate and merge user theme files instead of blind override#260HANCORE-linux wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds hex color validation, a Theme.Validate() method, and a merge(dst, src) that overlays only valid non-empty fields. loadUserDir now merges matching user themes onto built-ins or requires full validation for standalone themes. Unit and LoadAll tests are added/updated and use CLIAMP_CONFIG_DIR temp dirs. ChangesTheme validation and conditional merging
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
theme/load_test.go (1)
48-183: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winConvert the new/updated
LoadAllcases to a table-driven test.These added scenarios are currently separate single-case tests with duplicated setup/assert flow; repo policy for
*_test.gorequires table-driven patterns.Refactor sketch
+func TestLoadAllUserThemeScenarios(t *testing.T) { + cases := []struct { + name string + fileName string + fileBody string + assertFn func(t *testing.T, themes []Theme) + }{ + // partial merge onto builtin + // standalone partial rejected + // invalid hex ignored during merge + // full standalone theme accepted + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + userDir := filepath.Join(home, ".config", "cliamp", "themes") + if err := os.MkdirAll(userDir, 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + if tc.fileName != "" { + if err := os.WriteFile(filepath.Join(userDir, tc.fileName), []byte(tc.fileBody), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + } + tc.assertFn(t, LoadAll()) + }) + } +}As per coding guidelines,
**/*_test.go: “Tests must use table-driven test patterns.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@theme/load_test.go` around lines 48 - 183, Multiple near-duplicate tests exercise LoadAll with different user theme files; convert them into a single table-driven test that iterates test cases (e.g., name "partial merge", "user-only", "partial-no-builtin", "invalid-hex-merge") and performs the shared setup/teardown, file writes, call to LoadAll, and assertions per-case. Replace TestLoadAllPartialUserOverrideMergesOntoBuiltin, TestLoadAllAddsUserOnlyTheme, TestLoadAllSkipsPartialThemeWithoutBuiltinMatch, and TestLoadAllSkipsInvalidHexInMerge with one TestLoadAll_TableDriven that defines a slice of structs containing fields: case name, filename, file contents, expected presence (bool), and per-field expectations (Accent, FG, BrightFG, Red, etc.), then loop over cases, set HOME/temp dir, create userDir, write the file, call LoadAll(), find Theme by name and assert expectations accordingly; reuse Theme, LoadAll, and strings.EqualFold to locate themes and keep individual subtests using t.Run(case.name, func(t *testing.T) { ... }).Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@theme/theme_test.go`:
- Around line 152-232: Rewrite the four tests into table-driven subtests: create
a TestTheme table that contains cases for Validate (cases: valid, partial,
empty, badHex with expected error bool), ValidateErrorContainsFieldNames (a case
that runs Validate on Theme{"broken",...} and asserts the error string contains
the field names), and Merge scenarios (partial override and ignores-invalid-hex)
— each case should use t.Run(name, func(t *testing.T){...}) and perform the same
assertions currently in TestValidate, TestValidateErrorContainsFieldNames,
TestMerge and TestMergeIgnoresInvalidHex; reference and call the same symbols
(Theme, Validate, merge, BrightFG/FG/Accent/Green/Yellow/Red) and use table
fields for input Theme values and expected outcomes (expectedErr bool, expected
fields or expected substrings) to drive assertions.
---
Outside diff comments:
In `@theme/load_test.go`:
- Around line 48-183: Multiple near-duplicate tests exercise LoadAll with
different user theme files; convert them into a single table-driven test that
iterates test cases (e.g., name "partial merge", "user-only",
"partial-no-builtin", "invalid-hex-merge") and performs the shared
setup/teardown, file writes, call to LoadAll, and assertions per-case. Replace
TestLoadAllPartialUserOverrideMergesOntoBuiltin, TestLoadAllAddsUserOnlyTheme,
TestLoadAllSkipsPartialThemeWithoutBuiltinMatch, and
TestLoadAllSkipsInvalidHexInMerge with one TestLoadAll_TableDriven that defines
a slice of structs containing fields: case name, filename, file contents,
expected presence (bool), and per-field expectations (Accent, FG, BrightFG, Red,
etc.), then loop over cases, set HOME/temp dir, create userDir, write the file,
call LoadAll(), find Theme by name and assert expectations accordingly; reuse
Theme, LoadAll, and strings.EqualFold to locate themes and keep individual
subtests using t.Run(case.name, func(t *testing.T) { ... }).
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b1623432-60fc-452f-8aac-8d84def0eafe
📒 Files selected for processing (3)
theme/load_test.gotheme/theme.gotheme/theme_test.go
…ride
User theme files in ~/.config/cliamp/themes/ could silently replace built-in themes even when partial or corrupt, leaving empty hex fields that produced broken colours via lipgloss.Color("") in styles.go.
- validHex() validates #rgb/#rrggbb/#rrggbbaa format
- Theme.Validate() checks all six colour fields
- merge() applies only valid-hex fields onto the destination theme
- loadUserDir now uses two modes:
- MERGE: when the file name matches a built-in theme, partial fields are merged onto the built-in (rest survives)
- VALIDATE: themes without built-in match require all six hex fields or are rejected
- Invalid hex values in either mode are silently ignored
…solation - TestValidate and TestMerge now use table-driven subtests - TestLoadAllUserThemeScenarios combines 4 LoadAll scenarios with shared setup - All env-based tests use CLIAMP_CONFIG_DIR instead of HOME to avoid XDG_CONFIG_HOME override in appdir.Dir()
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@theme/load_test.go`:
- Line 83: Replace the inline single-line TOML string assigned to fileBody with
a multi-line raw string literal (backtick) to improve readability; locate the
test case where fileBody is set (the struct field named fileBody in
theme/load_test.go) and change the value from a single quoted string ("accent =
\"`#abcdef`\"...") to a backtick-wrapped here-doc style preserving newlines and
content exactly (e.g. `accent = "`#abcdef`"\nbright_fg = "`#ffffff`"\n...`) so the
TOML fields are each on their own line.
- Line 114: Replace the long single-line TOML in the test case's fileBody with a
Go raw multi-line string literal (backtick string) to improve readability;
locate the failing test in load_test.go where fileBody is set (the example with
"accent = \"`#ff0000`\"...red = \"not-a-color\"") and convert that quoted
single-line value into a backtick-delimited multi-line TOML block that preserves
identical content and line breaks, matching the style used in the "full
standalone theme accepted" case.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: be8d1969-b508-4e66-bc66-27ae789ca15c
📒 Files selected for processing (3)
theme/load_test.gotheme/theme.gotheme/theme_test.go
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Closing this — after testing on a real setup, the validate/merge approach doesn't fix the actual symptom ("wrong / too-bright colours, have to The themes that break are complete, valid user themes generated by the Omarchy theme-set hook. They pass The real bug is the hook's colour mapping, not cliamp:
Real fix (in the hook's repo): OldJobobo/theme-hook-plugin-manager#3. Optional cliamp-side hardening (separate, not required for this bug): when |
1 participant
Problem
loadUserDir()intheme/theme.gooverwrites built-in themes with user files from~/.config/cliamp/themes/*.tomlwithout any validation. A partial or corrupt file (e.g. onlyaccent = "#ff0000") replaces the entire built-in theme, leaving the other 5 colour fields empty.lipgloss.Color("")inui/styles.gothen produces broken colours.Workaround:
rm -rf ~/.config/cliamp/themes/removes the bad files and restores correct colours -- until the next corrupt file appears.Fix
Three new helpers in
theme/theme.go:validHex(s string) bool#rgb,#rrggbbor#rrggbbaaformatTheme.Validate() errormerge(dst *Theme, src Theme)srcontodstloadUserDirnow has two modes:dracula.toml->dracula)my-theme.toml)Invalid hex values are silently ignored in either mode -- the corresponding built-in (or zero) value survives. This prevents partial or corrupt files from breaking colours while still allowing single-field customisation.
Tests
22 tests, all PASS. New tests cover:
validHex-- valid:#fff#aabbcc#aabbccdd-- invalid: empty string#xyz2-char hexValidatewith valid, partial, empty, and invalid-hex themesFiles changed
theme/theme.go-- +validHex, +Validate, +merge, loadUserDir updatedtheme/theme_test.go-- +5 teststheme/load_test.go-- existing tests adapted to merge, +3 new testsSummary by CodeRabbit
New Features
Chores