Let embedders register built-in themes via RegisterBuiltinThemes#3182
Conversation
fd2fc6f to
6c7b2f9
Compare
Add styles.RegisterBuiltinThemes(fs.FS), which adds an embedder-provided theme source (e.g. an embed.FS of themes/*.yaml) to the built-in theme set. Registered themes flow through the exact list/load/merge-onto-default pipeline cagent already uses for its bundled themes, so they are first-class: they appear in ListThemeRefs and the /theme picker, resolve via LoadTheme/ApplyThemeRef, and persist as the user's selection — with no changes to the picker or handlers. Registered sources take precedence over the bundled themes, so an embedder can override a built-in and, in particular, mask "default" with their own (still merged onto cagent's pristine DefaultTheme() base). This lets a downstream CLI/TUI ship its own default and theme set without forking, while cagent core stays domain-agnostic.
6c7b2f9 to
9a14205
Compare
- lint: use errors.New for the static nil-fs message (perfsprint) and require.Error for the error assertions (testifylint) - docs: the godoc stated the opposite of the implemented behavior. Registered sources take precedence over bundled themes and can mask "default", which is what the PR description and the tests already assert. Rewrite the godoc to match. - bug: RegisterBuiltinThemes invalidated only the refs-list cache, not the parsed-theme cache. Built-in cache entries are treated as permanently valid, so an override of a built-in (or "default") that was loaded before registration was silently ignored. Invalidate the theme cache on registration and add a regression test.
|
Pushed a few fixes (
|
|
Thanks for the collaboration @Sayt-0! Here's a screenshot of a test app built from this branch (incl. your commit), showing an overridden default theme ("Embedded Default") plus an additional theme ("Embedded Extra").
|
When more than one registered source provides the same theme ref, the most recently registered source now wins (last-wins), matching the conventional Register(...) override semantics rather than first-wins. Registered sources still take precedence over cagent's bundled themes. The flip is a single reversed loop in readRegisteredThemeData; the name list is unaffected since a collision yields the same ref regardless of which source backs its data. Also renames the end-to-end test to _Integration to set it apart from the narrower behavior tests.
Satisfies the modernize linter (slicesbackward) flagged in CI.
Sayt-0
left a comment
There was a problem hiding this comment.
Reviewed the full register/list/load/merge path plus a multi-lens pass (concurrency, consistency, edge cases, tests). The design is clean: generalizing the built-in source from a single embed.FS to a list lets registered themes flow through the exact existing pipeline with no picker/handler changes, and the partial-override-onto-pristine-default semantics are preserved. Build, vet, test -race, and golangci-lint all pass locally; the 7 new tests pass.
Approving. The notes below are minor, non-blocking polish (hardening plus test coverage on documented behaviors); none affect the core feature.
Two candidate concerns were investigated and dismissed:
registeredThemeFSes()returning the live slice is not a data race: the slice is append-only under lock and readers only range[0,len), so an in-place append writes a disjoint index. A-racestress test (8 writers / 16 readers) is clean. The word "snapshot" in its godoc is slightly loose, nothing more;slices.Clonewould be harmless future-proofing.- Masking
defaultis honored on every real path (LoadTheme, the picker, startup, cancel-preview); the empty/nil fallback divergence is not reachable because the cancel ref always comes fromCurrentTheme().Ref, which is never empty.
One additional note on a file not in this diff: ApplyThemeRef (pkg/tui/styles/hooks.go:39-46) falls back to DefaultTheme() (the pristine base) on a LoadTheme error, whereas LoadTheme("default") and the picker resolve an embedder-masked default. The mainline ApplyThemeRef("default") path is correct; only an unresolvable ref reaches the divergence. Optional: fall back through LoadTheme(DefaultThemeRef) so the fallback agrees with the picker.
| } else if before, ok := strings.CutSuffix(name, ".yml"); ok { | ||
| refs = append(refs, before) | ||
| for _, r := range extraRefs { | ||
| if r == DefaultThemeRef || seen[r] { |
There was a problem hiding this comment.
Registered refs are aggregated here skipping only default and duplicates, but they are not validated. A registered file named themes/user:x.yaml yields ref user:x, which is listed as a built-in, yet IsBuiltinTheme("user:x") returns false and LoadTheme routes it to the user-theme dir (not found). A name containing .. is listed and classified built-in, but LoadTheme rejects it via validateThemeRef. Not a security issue (fs.ReadFile stays rooted), but the listing layer disagrees with the classify/load layer, leaving an unselectable picker entry.
Optional hardening, mirroring the existing default skip: also skip refs where strings.HasPrefix(r, UserThemePrefix) or validateThemeRef(r) != nil, so every listed built-in ref is guaranteed loadable and IsBuiltinTheme agrees with LoadTheme.
|
|
||
| // TestRegisterBuiltinThemes covers core registration: a registered theme is | ||
| // listed, classified as built-in, and loads merged onto the default theme. | ||
| func TestRegisterBuiltinThemes(t *testing.T) { |
There was a problem hiding this comment.
readThemeData and readThemeRefsFromFS try .yaml then .yml for registered sources, and .yml is documented as supported on RegisterBuiltinThemes. The existing .yml coverage only exercises the os-based user-dir path (loadThemeFrom / listThemeRefsFrom), not the fs.FS path, so the registered .yml branch is untested. Consider one MapFS case with only themes/foo.yml, asserted via ListThemeRefs and LoadTheme("foo").
| } | ||
|
|
||
| // TestRegisterBuiltinThemes_Errors covers eager validation of the source. | ||
| func TestRegisterBuiltinThemes_Errors(t *testing.T) { |
There was a problem hiding this comment.
The error path for a registered theme with invalid YAML is uncovered: loadBuiltinTheme wraps the unmarshal error and ApplyThemeRef then silently falls back to the default. Consider registering a themes/bad.yaml with malformed YAML and asserting LoadTheme("bad") errors (and is not cached), plus ApplyThemeRef("bad") falls back to the default theme.
|
|
||
| // TestRegisterBuiltinThemes_OverridesBuiltin verifies a registered source takes | ||
| // precedence over a bundled theme of the same name. | ||
| func TestRegisterBuiltinThemes_OverridesBuiltin(t *testing.T) { |
There was a problem hiding this comment.
These override/mask tests assert LoadTheme behavior but not the listing invariants the PR documents ("each name listed once", "default listed first"). assert.Contains cannot catch a duplicate. Consider asserting refs[0] == DefaultThemeRef and that default and nord each appear exactly once after registering a masking default and an override, so a regression in the dedup (seen[r]) or ordering is caught.

What
Adds
styles.RegisterBuiltinThemes(fsys fs.FS) error, which lets an embeddercontribute additional built-in themes from a filesystem (typically an
embed.FSofthemes/*.yaml).Why
Themes are already partial overrides on top of
DefaultTheme()—loadBuiltinThemeand
loadThemeFrombothmergeTheme(DefaultTheme(), &override), which is why abundled theme YAML only declares the colors it changes. But the built-in set is a
single compile-time
//go:embed themes/*.yaml, which a downstream consumer can'textend.
A downstream CLI/TUI built on cagent wants its own theme(s) to be first-class
without forking.
RegisterBuiltinThemesgeneralizes the bundled-theme sourcefrom one hardcoded
embed.FSto a list, so registered themes flow through theexact list/load/merge pipeline cagent already uses. They are treated like
cagent's own built-ins with no changes to the picker or handlers:
ListThemeRefs(and the/themepicker),LoadTheme/ApplyThemeRef,IsBuiltinTheme),Registered sources take precedence over the bundled themes, so an embedder can
override a built-in and, in particular, mask
defaultwith their own. Amasked default is still a partial override merged onto cagent's pristine
DefaultTheme()base, so it only needs to specify the fields it changes.DefaultTheme()itself remains the canonical merge base.Embedder usage
themes/brand.yamlis a partial override; unset fields fall back to the defaulttheme:
To make the brand theme the launch default, ship it as
themes/default.yamlinstead (or in addition) — it masks cagent's default while inheriting everything
it doesn't set.
Notes / semantics
themes/<ref>.yaml(or.yml). An embedder thatembeds at
themes/*.yamlpasses itsembed.FSdirectly; otherwisefs.Subtore-root at the themes dir.
name is still listed once.
RegisterBuiltinThemesvalidates the source eagerly (nil / missingthemesdir) so problems surface at registration, not at picker time.
Tests
Tests live in
theme_test.go(package styles), consistent with the existingtheme tests, and exercise registration through the public functions an embedder
uses, with a small unexported reset for isolation:
TestRegisterBuiltinThemes_EmbedderFlow— registers from a realembed.FSandruns the full register → list →
IsBuiltinTheme→ApplyThemeRefloop.TestRegisterBuiltinThemes— listed, classified built-in, loads merged ontothe default (overridden field changes, unset fields inherited).
TestRegisterBuiltinThemes_MultipleSources— aggregates across more than onesource; first-registered wins a collision between two registered sources.
TestRegisterBuiltinThemes_OverridesBuiltin— a registered name overrides thebundled theme.
TestRegisterBuiltinThemes_MasksDefault— masksdefault, merged onto thepristine
DefaultTheme()base.TestRegisterBuiltinThemes_Errors— nil / missing-themes-dir rejected eagerly.Full
pkg/tui/stylessuite passes under-race;go vetandgofmtclean.