feat(shared): theme skins (dark/light/high-contrast) + --skin flag + :skin palette - #4
feat(shared): theme skins (dark/light/high-contrast) + --skin flag + :skin palette#4posquit0 wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a theme skin system (MW-10) allowing users to customize the terminal UI appearance via a new --skin CLI flag and a :skin <name> command palette option. It adds three built-in skins (dark, light, high-contrast) and supports loading custom YAML skins from the user's config directory, while refactoring various TUI components to use a centralized active-skin state. The review feedback highlights several important improvements: validating skin names to prevent directory traversal, validating custom YAML skin structures to avoid silent parsing failures, caching environment variables to avoid performance overhead in the rendering hot path, and improving test isolation by using t.Setenv instead of mutating the global environment.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| trimmed := strings.TrimSpace(name) | ||
| if trimmed == "" { | ||
| return SkinDef{}, fmt.Errorf("skin name is empty") | ||
| } |
There was a problem hiding this comment.
The name parameter is passed directly to userSkinPath and used in filepath.Join without validation. This allows directory traversal (e.g., ../../../../etc/passwd) to read arbitrary YAML files on the system. While this is a local CLI tool, it is best practice to restrict skin names to alphanumeric characters, hyphens, and underscores to prevent directory traversal.
trimmed := strings.TrimSpace(name)
if trimmed == "" {
return SkinDef{}, fmt.Errorf("skin name is empty")
}
if strings.Contains(trimmed, "..") || strings.ContainsAny(trimmed, "/\\") {
return SkinDef{}, fmt.Errorf("skin %q: invalid name", trimmed)
}| func parseSkinDef(data []byte, want string) (SkinDef, error) { | ||
| var def SkinDef | ||
| if err := yaml.Unmarshal(data, &def); err != nil { | ||
| return SkinDef{}, fmt.Errorf("skin %q: parse: %w", want, err) | ||
| } | ||
| if def.Name == "" { | ||
| def.Name = want | ||
| } | ||
| return def, nil | ||
| } |
There was a problem hiding this comment.
If a user-defined skin has a typo in the tones key (e.g., tonez:), or if the YAML file is empty, yaml.Unmarshal will silently succeed but leave def.Tones empty, resulting in an unstyled theme without any error feedback. Additionally, any typos in individual tone keys (e.g., row_curser) will be silently ignored. Adding validation for empty tones and unknown keys will greatly improve the user experience when creating custom skins.
var validToneKeys = map[string]bool{
"bg": true, "fg": true, "muted": true, "header": true, "accent": true,
"primary": true, "success": true, "warning": true, "danger": true, "info": true,
"magenta": true, "badge_sys": true, "badge_rule": true, "badge_large": true,
"badge_unmask": true, "row_cursor": true, "row_danger": true, "row_warning": true,
"row_muted": true, "row_changed": true,
}
func parseSkinDef(data []byte, want string) (SkinDef, error) {
var def SkinDef
if err := yaml.Unmarshal(data, &def); err != nil {
return SkinDef{}, fmt.Errorf("skin %q: parse: %w", want, err)
}
if len(def.Tones) == 0 {
return SkinDef{}, fmt.Errorf("skin %q: no tones defined", want)
}
for k := range def.Tones {
if !validToneKeys[k] {
return SkinDef{}, fmt.Errorf("skin %q: unknown tone key %q", want, k)
}
}
if def.Name == "" {
def.Name = want
}
return def, nil
}| if MonochromeEnabled() { | ||
| return Monochrome() | ||
| } |
There was a problem hiding this comment.
Calling os.Getenv on every single frame (via ActiveTokens -> MonochromeEnabled) introduces unnecessary overhead and lock contention on syscall.envMu in the rendering hot path. Consider caching this value in a package-level variable (e.g., var monochromeEnabled = os.Getenv("NO_COLOR") != "" in styles.go or skins.go) and referencing that instead. For tests, you can expose a helper or directly mutate the variable instead of using t.Setenv.
| ) | ||
|
|
||
| func Test_LoadSkin_AllBuiltinsResolve(t *testing.T) { | ||
| os.Unsetenv("NO_COLOR") |
There was a problem hiding this comment.
Using os.Unsetenv directly in tests mutates the global environment of the test runner and can pollute other tests running in the same process (especially if the user has NO_COLOR set in their environment). Use t.Setenv("NO_COLOR", "") instead, which automatically restores the original environment state after the test completes. This also applies to other tests in this file (lines 46, 67, 88, 112).
| os.Unsetenv("NO_COLOR") | |
| t.Setenv("NO_COLOR", "") |
| t.Cleanup(func() { | ||
| os.Unsetenv("NO_COLOR") | ||
| _, _ = shared.SetActiveSkin("dark") | ||
| }) |
There was a problem hiding this comment.
Since t.Setenv("NO_COLOR", "1") is called on line 137, Go's testing framework will automatically restore the original value of NO_COLOR when the test finishes. Manually calling os.Unsetenv("NO_COLOR") in t.Cleanup is redundant and can incorrectly unset NO_COLOR if it was originally set to a non-empty value before the test ran.
t.Cleanup(func() {
_, _ = shared.SetActiveSkin("dark")
})Address PR #4 review comments on the MW-10 theme system: - LoadTheme: reject empty, "..", "/", "\\" theme names before touching disk so `--theme=../../etc/passwd` can never escape ~/.config/ota/themes. - parseSkinDef: whitelist tone keys and reject empty tone bodies / empty tones map so silent YAML typos (e.g. `body_bg` for `bg`) fail loud instead of yielding an unstyled theme. - MonochromeEnabled: cache NO_COLOR at package init so ActiveTokens() doesn't syscall on every render. Add RefreshMonochromeEnabledForTest for tests that mutate NO_COLOR at runtime; wire it into testfx.PinTestEnvironment so golden tests keep observing the pinned value. - themes_test / styles_test: replace os.Unsetenv + t.Cleanup env-restore boilerplate with a small helper that calls t.Setenv("NO_COLOR", "") and refreshes the cache. Adds regression tests for the new validation paths (unsafe names, unknown tone keys, empty tone / tones map).
…:skin palette
Ship 3 built-in themes and let users drop custom YAML skins under
~/.config/ota/skins. Dark reproduces the current palette; light
inverts; high-contrast is attribute-only for NO_COLOR compat.
- internal/tui/shared/skins.go: SkinDef/SkinTone YAML types + LoadSkin
loader (embedded first, then ~/.config/ota/skins/<name>.yaml)
- internal/tui/shared/skins/{dark,light,high_contrast}.yaml embedded
- shared.ActiveTokens/SetActiveSkin under RWMutex for hot-swap
- Deps.Skin threaded through wire → app.New; --skin CLI flag
- :skin <name> palette command (aliased :theme) for runtime swap
- Every screen's activeTokens() delegates to shared.ActiveTokens()
- skins_test.go + app/skin_palette_test.go cover the flow
Unify terminology on 'theme'. skin was an intermediate name; every
type, file, flag, palette command, and yaml key now says theme.
- shared: SkinDef→ThemeDef, SkinTone→ThemeTone, LoadSkin→LoadTheme,
SetActiveSkin→SetActiveTheme, ActiveSkin→ActiveTheme
- files: internal/tui/shared/{skins.go, skins_test.go, skins/} →
{themes.go, themes_test.go, themes/}
- CLI: --skin → --theme
- palette: :skin dropped (was aliased already); :theme is the canonical
- Deps.Skin → Deps.Theme, wire.go + app.go threaded through
- app: paletteCmdSkin → paletteCmdTheme, internal/app/skin_palette_test.go
→ theme_palette_test.go
- overlay: palette hints + help entries updated; goldens regenerated
Address PR #4 review comments on the MW-10 theme system: - LoadTheme: reject empty, "..", "/", "\\" theme names before touching disk so `--theme=../../etc/passwd` can never escape ~/.config/ota/themes. - parseSkinDef: whitelist tone keys and reject empty tone bodies / empty tones map so silent YAML typos (e.g. `body_bg` for `bg`) fail loud instead of yielding an unstyled theme. - MonochromeEnabled: cache NO_COLOR at package init so ActiveTokens() doesn't syscall on every render. Add RefreshMonochromeEnabledForTest for tests that mutate NO_COLOR at runtime; wire it into testfx.PinTestEnvironment so golden tests keep observing the pinned value. - themes_test / styles_test: replace os.Unsetenv + t.Cleanup env-restore boilerplate with a small helper that calls t.Setenv("NO_COLOR", "") and refreshes the cache. Adds regression tests for the new validation paths (unsafe names, unknown tone keys, empty tone / tones map).
a69329d to
d065f4c
Compare
Summary
Files
Test plan