Add accessible theme modes and custom palette support#30
Merged
Conversation
📝 WalkthroughWalkthroughAdds configurable display themes (default, no_color, neon, custom) with palette support, wiring through config parsing/validation, TUI style construction, settings overlay controls, localization, docs/specs, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User / TUI"
participant TUI as "Settings Overlay"
participant Model as "RootModel"
participant Config as "Config (file)"
participant Styles as "Styles Builder"
User->>TUI: open settings
TUI->>Model: set settingsThemeMode (←/→)
Model->>Styles: NewStyles(themeFromSettings(settingsThemeMode, themePalette))
Styles-->>Model: Styles (applied immediately)
User->>TUI: Save (Enter)
TUI->>Model: produce draft (draft.ThemeMode = settingsThemeMode)
Model->>Config: Save(draft)
Config-->>Model: persisted
Model->>Styles: NewStyles(themeFromSettings(persisted.ThemeMode, persisted.ThemePalette))
Styles-->>Model: Styles (persisted & applied)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 147 |
| Duplication | 24 |
TIP This summary will be updated as you push new changes. Give us feedback
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (6)
internal/app/model.go (1)
305-316: Minor: Redundant normalization insettingsThemeModeLabel.
m.settingsThemeModeis already normalized when the settings overlay is opened (line 241). The additionalconfig.NormalizeThemeMode()call on line 306 is defensive but unnecessary.This is a minor observation and doesn't affect correctness—the defensive call ensures safety if the field is ever modified without normalization elsewhere.
♻️ Optional: remove redundant normalization
func (m RootModel) settingsThemeModeLabel() string { - switch config.NormalizeThemeMode(m.settingsThemeMode) { + switch m.settingsThemeMode { case config.ThemeModeNoColor: return i18n.T(i18n.SettingsThemeNoColor) case config.ThemeModeNeon: return i18n.T(i18n.SettingsThemeNeon) case config.ThemeModeCustom: return i18n.T(i18n.SettingsThemeCustom) default: return i18n.T(i18n.SettingsThemeDefault) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/app/model.go` around lines 305 - 316, The settingsThemeModeLabel function redundantly calls config.NormalizeThemeMode on m.settingsThemeMode; remove that defensive normalization by switching directly on m.settingsThemeMode inside settingsThemeModeLabel (function name: settingsThemeModeLabel) so the switch becomes switch m.settingsThemeMode { ... } returning the same i18n.T branches for config.ThemeModeNoColor, config.ThemeModeNeon, config.ThemeModeCustom, and default; no behavior changes required beyond removing the extra NormalizeThemeMode invocation.internal/app/update_test.go (1)
674-676: Consider direct string comparison for lipgloss.Color.
lipgloss.Coloris a string type alias, soreflect.DeepEqualworks but a direct comparison is clearer and avoids the reflection overhead:if final.styles.Title.GetForeground() != lipgloss.Color("#A6FF00") {However, the current approach is functionally correct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/app/update_test.go` around lines 674 - 676, The test uses reflect.DeepEqual to compare lipgloss.Color values (final.styles.Title.GetForeground() and lipgloss.Color("#A6FF00")); replace the reflect.DeepEqual call with a direct string equality check (final.styles.Title.GetForeground() != lipgloss.Color("#A6FF00")) to be clearer and avoid reflection while keeping the same failure message via t.Fatalf.internal/tui/styles.go (2)
175-187: Parameter namehexis misleading for ANSI color codes.These helper functions accept both hex colors (
#A6FF00) and ANSI color codes (81,243). Consider renaming the parameter tocolororcolorSpecfor clarity.💡 Suggested rename
-func foregroundStyle(style lipgloss.Style, hex string) lipgloss.Style { - if hex == "" { +func foregroundStyle(style lipgloss.Style, color string) lipgloss.Style { + if color == "" { return style } - return style.Foreground(lipgloss.Color(hex)) + return style.Foreground(lipgloss.Color(color)) } -func borderStyle(style lipgloss.Style, hex string) lipgloss.Style { - if hex == "" { +func borderStyle(style lipgloss.Style, color string) lipgloss.Style { + if color == "" { return style } - return style.BorderForeground(lipgloss.Color(hex)) + return style.BorderForeground(lipgloss.Color(color)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tui/styles.go` around lines 175 - 187, The parameter name `hex` in helper functions foregroundStyle and borderStyle is misleading because they accept hex or ANSI color codes; rename the parameter to a clearer name like `color` (or `colorSpec`) and update the function signatures and their internal uses (e.g., lipgloss.Color(hex) -> lipgloss.Color(color)) as well as all callers to pass the renamed parameter to avoid compile errors.
61-70: Clarify:ResolvePalettereturnsdefaultPalette()forThemeNoColor.This may be intentional (e.g., for configuration display purposes), but it's worth documenting that
ResolvePalette(ThemeNoColor)does not return an empty palette—it returns the default palette values even thoughNewStyles(ThemeNoColor)doesn't use them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tui/styles.go` around lines 61 - 70, ResolvePalette currently returns defaultPalette() when given a Theme with Mode normalized to ThemeNoColor, which can be confusing because NewStyles(ThemeNoColor) ignores palettes; add a brief comment above ResolvePalette stating that ResolvePalette(ThemeNoColor) intentionally returns the default palette (not an empty palette) for purposes like configuration display, and note that NewStyles ignores the palette for ThemeNoColor so the palette is unused in rendering; reference the ResolvePalette function and the ThemeNoColor constant and NewStyles behavior in the comment to make the distinction clear.internal/tui/styles_test.go (1)
11-25: Consider adding test coverage forResolvePalettewithThemeNoColor.Tests cover
ThemeDefault,ThemeNeon, andThemeCustom, but there's no explicit test forResolvePalette(Theme{Mode: ThemeNoColor}). While the implementation likely falls through to the default case, an explicit test would document the expected behavior.💡 Suggested test addition
func TestResolvePaletteNoColor(t *testing.T) { t.Parallel() palette := ResolvePalette(Theme{Mode: ThemeNoColor}) // NoColor mode falls back to default palette (or returns empty - verify expected behavior) want := ThemePalette{ Accent: "81", Success: "42", Danger: "203", Muted: "245", Border: "", } if palette != want { t.Fatalf("palette = %+v, want %+v", palette, want) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tui/styles_test.go` around lines 11 - 25, Add a unit test named TestResolvePaletteNoColor that calls ResolvePalette(Theme{Mode: ThemeNoColor}) and asserts it returns the same ThemePalette as the default case; use the same expected ThemePalette values as TestResolvePaletteDefault (Accent "81", Success "42", Danger "203", Muted "245", Border ""), and fail the test with t.Fatalf if the returned palette differs; reference ResolvePalette, Theme, ThemeNoColor and ThemePalette to locate where to add the test.tasks/feature_spec.md (1)
437-437: Clarify:docs/specs/mentioned in scope vs. earlier "don't add" policy.Line 124 (from 2026-03-29 spec) states
docs/specs/は増やさず (don't add to docs/specs), but this spec includesdocs/specs/の恒久仕様 in scope. Please clarify if the team has revised the documentation policy, or if this should reference a different location (e.g., README or ADR).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tasks/feature_spec.md` at line 437, Resolve the contradiction between the mention of `docs/specs/` in scope and the earlier "don't add to docs/specs" policy by choosing one of two fixes: either remove `docs/specs/` from the scope list and replace it with the intended location (e.g., README or ADR), or amend the earlier policy text to explicitly allow permanent specs in `docs/specs/` and describe any conditions; update the spec text where `docs/specs/` is referenced and the earlier "don't add to docs/specs" statement so they consistently reference the same canonical storage location and rationale.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/app/model.go`:
- Around line 305-316: The settingsThemeModeLabel function redundantly calls
config.NormalizeThemeMode on m.settingsThemeMode; remove that defensive
normalization by switching directly on m.settingsThemeMode inside
settingsThemeModeLabel (function name: settingsThemeModeLabel) so the switch
becomes switch m.settingsThemeMode { ... } returning the same i18n.T branches
for config.ThemeModeNoColor, config.ThemeModeNeon, config.ThemeModeCustom, and
default; no behavior changes required beyond removing the extra
NormalizeThemeMode invocation.
In `@internal/app/update_test.go`:
- Around line 674-676: The test uses reflect.DeepEqual to compare lipgloss.Color
values (final.styles.Title.GetForeground() and lipgloss.Color("#A6FF00"));
replace the reflect.DeepEqual call with a direct string equality check
(final.styles.Title.GetForeground() != lipgloss.Color("#A6FF00")) to be clearer
and avoid reflection while keeping the same failure message via t.Fatalf.
In `@internal/tui/styles_test.go`:
- Around line 11-25: Add a unit test named TestResolvePaletteNoColor that calls
ResolvePalette(Theme{Mode: ThemeNoColor}) and asserts it returns the same
ThemePalette as the default case; use the same expected ThemePalette values as
TestResolvePaletteDefault (Accent "81", Success "42", Danger "203", Muted "245",
Border ""), and fail the test with t.Fatalf if the returned palette differs;
reference ResolvePalette, Theme, ThemeNoColor and ThemePalette to locate where
to add the test.
In `@internal/tui/styles.go`:
- Around line 175-187: The parameter name `hex` in helper functions
foregroundStyle and borderStyle is misleading because they accept hex or ANSI
color codes; rename the parameter to a clearer name like `color` (or
`colorSpec`) and update the function signatures and their internal uses (e.g.,
lipgloss.Color(hex) -> lipgloss.Color(color)) as well as all callers to pass the
renamed parameter to avoid compile errors.
- Around line 61-70: ResolvePalette currently returns defaultPalette() when
given a Theme with Mode normalized to ThemeNoColor, which can be confusing
because NewStyles(ThemeNoColor) ignores palettes; add a brief comment above
ResolvePalette stating that ResolvePalette(ThemeNoColor) intentionally returns
the default palette (not an empty palette) for purposes like configuration
display, and note that NewStyles ignores the palette for ThemeNoColor so the
palette is unused in rendering; reference the ResolvePalette function and the
ThemeNoColor constant and NewStyles behavior in the comment to make the
distinction clear.
In `@tasks/feature_spec.md`:
- Line 437: Resolve the contradiction between the mention of `docs/specs/` in
scope and the earlier "don't add to docs/specs" policy by choosing one of two
fixes: either remove `docs/specs/` from the scope list and replace it with the
intended location (e.g., README or ADR), or amend the earlier policy text to
explicitly allow permanent specs in `docs/specs/` and describe any conditions;
update the spec text where `docs/specs/` is referenced and the earlier "don't
add to docs/specs" statement so they consistently reference the same canonical
storage location and rationale.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ab5f451d-c028-43f7-be85-6ef6829f6a35
📒 Files selected for processing (19)
README.en.mdREADME.mdassets/locale/en.tomlassets/locale/ja.tomldocs/specs/theme-settings.mdinternal/app/model.gointernal/app/update.gointernal/app/update_test.gointernal/app/view.gointernal/app/view_test.gointernal/config/config.gointernal/config/config_test.gointernal/i18n/i18n_test.gointernal/i18n/keys.gointernal/tui/styles.gointernal/tui/styles_test.gotasks/feature_spec.mdtasks/lessons.mdtasks/todo.md
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/tui/styles.go (1)
184-195: Consider extracting shared normalization logic to reduce duplication.
NormalizeThemeModeduplicatesconfig.NormalizeThemeMode(both lower+trim and switch on the same values). While this keeps packages independent, you could extract the shared logic into a minimal shared package (e.g.,internal/theme/mode.go) if the duplication becomes a maintenance burden.For now this works correctly since both use identical string values.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tui/styles.go` around lines 184 - 195, The NormalizeThemeMode function duplicates logic already present in config.NormalizeThemeMode; extract the shared normalization (trim+lowercase and the Theme* switch mapping) into a small internal package (e.g., internal/theme/mode.go) that exposes a single NormalizeThemeMode and the Theme constants, then update internal/tui/styles.go to call that shared NormalizeThemeMode instead of duplicating the switch logic; ensure you keep the same returned constants (ThemeNoColor, ThemeNeon, ThemeCustom, ThemeDefault) and update imports accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/tui/styles.go`:
- Around line 184-195: The NormalizeThemeMode function duplicates logic already
present in config.NormalizeThemeMode; extract the shared normalization
(trim+lowercase and the Theme* switch mapping) into a small internal package
(e.g., internal/theme/mode.go) that exposes a single NormalizeThemeMode and the
Theme constants, then update internal/tui/styles.go to call that shared
NormalizeThemeMode instead of duplicating the switch logic; ensure you keep the
same returned constants (ThemeNoColor, ThemeNeon, ThemeCustom, ThemeDefault) and
update imports accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 31627952-f3ae-45d3-8808-43a09e25b642
📒 Files selected for processing (7)
internal/app/model.gointernal/app/update_test.gointernal/config/config.gointernal/config/config_test.gointernal/tui/styles.gointernal/tui/styles_test.gotasks/feature_spec.md
✅ Files skipped from review due to trivial changes (1)
- internal/app/update_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/config/config_test.go
- internal/tui/styles_test.go
- internal/config/config.go
- tasks/feature_spec.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
default,no_color,neon, andcustomtheme modes with config-backed palette supporttheme_paletteslots when saving configTesting
Closes #28
Summary by CodeRabbit
New Features
Improvements
Documentation