feat(tui): add adaptive theme support#9
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughIntroduces a complete light/dark/auto theme system. Core types ( ChangesDynamic Light/Dark Theme System
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI flags<br/>--theme light
participant Root as cli/root.go
participant App as app.go
participant Config as config.go<br/>+ Viper
participant SystemTheme as SystemThemeReader<br/>DBus Portal
participant Prompt as StartupPrompt
participant Model as tui.Model
participant Theme as theme.ApplyAppearance
CLI->>Root: parse --theme light --config path
Root->>Viper: BindPFlag(theme, light)
Root->>Viper: BindPFlag(config, ./ero.toml)
App->>Config: loadRuntimeConfig(cfg)
App->>Config: watchThemeConfigChanges(cfg) → themeModeChanges
alt --theme not set or auto
App->>SystemTheme: CurrentPreference()
SystemTheme-->>App: SystemThemePreference
end
App->>App: compute initialThemeAppearance
App->>Prompt: WithAppearance(initialThemeAppearance)
Prompt-->>App: themed StartupPrompt
App->>Model: NewModelWithActiveProviderContextConfig(ModelConfig)
Model->>Model: Init() → watchThemeModeChanges cmd
Config-->>Model: themeConfigChangedMsg (file change)
Model->>Theme: ApplyAppearance(ResolveThemeAppearance(...))
Model->>Model: MarkdownRenderer.Clear()
Note over Model: tea.BackgroundColorMsg (auto mode)
Model->>Theme: ApplyAppearance(from terminal background)
Model->>Model: scheduleThemeDetectionTick (generation++)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 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)
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: 4
🤖 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 `@internal/adapters/in/tui/model.go`:
- Around line 265-280: The issue is in the applyThemeMode function where
themeDetectionGeneration is incremented whenever the mode is auto (checking
previousMode == core.ThemeModeAuto || m.themeMode == core.ThemeModeAuto on line
270), but a new theme detection tick is only scheduled when transitioning from
non-auto to auto (checking previousMode != core.ThemeModeAuto on line 278). When
the mode stays as auto, this increments the generation counter and invalidates
the queued tick without scheduling a replacement, breaking the periodic
detection loop. Change the condition on line 270 to only increment
themeDetectionGeneration when transitioning to auto mode, meaning it should
check that previousMode is not auto AND the new themeMode is auto, ensuring the
generation bump only happens alongside the new tick scheduling logic.
In `@internal/adapters/in/tui/render/line_test.go`:
- Around line 74-75: The test cleanup function hardcodes theme.ApplyAppearance
to core.ThemeAppearanceDark, which can leak global state across tests if the
theme had a different appearance before. Save the current theme appearance
before calling theme.ApplyAppearance(core.ThemeAppearanceLight), then restore
that saved original appearance in the t.Cleanup callback instead of hardcoding
the dark appearance.
In `@internal/adapters/in/tui/theme/styles_test.go`:
- Around line 11-13: The test TestApplyAppearanceKeepsDarkPaletteAsDefault
assumes dark mode is already active before asserting that applying dark again
results in no change, which causes flakiness with global state and shuffled test
execution. Add an initial call to ApplyAppearance(core.ThemeAppearanceDark)
before line 12 to establish dark mode as the baseline state, ensuring the
subsequent ApplyAppearance call on line 12 is testing the deterministic no-op
behavior of applying dark when dark is already active.
In `@internal/app/app.go`:
- Around line 35-38: The `themedStartupPrompt` interface declared at line 35 is
not being used anywhere, causing a lint failure. At line 104, the same interface
contract is being redefined inline in a type assertion. To fix this, replace the
inline interface definition at line 104 with a reference to the already-declared
`themedStartupPrompt` interface. This will eliminate the unused declaration and
resolve the CI blocker.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7a525086-6ef7-40bd-890f-abcc1161f70e
📒 Files selected for processing (21)
go.modinternal/adapters/in/cli/root.gointernal/adapters/in/cli/root_test.gointernal/adapters/in/tui/comment_editor.gointernal/adapters/in/tui/component/statusbar.gointernal/adapters/in/tui/markdown_renderer.gointernal/adapters/in/tui/markdown_renderer_test.gointernal/adapters/in/tui/model.gointernal/adapters/in/tui/model_theme_test.gointernal/adapters/in/tui/pr_sheet.gointernal/adapters/in/tui/render/line.gointernal/adapters/in/tui/render/line_test.gointernal/adapters/in/tui/render/review_row.gointernal/adapters/in/tui/startup_prompt.gointernal/adapters/in/tui/theme/styles.gointernal/adapters/in/tui/theme/styles_test.gointernal/app/app.gointernal/app/app_test.gointernal/app/config.gointernal/core/theme.gointernal/core/theme_test.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/adapters/in/tui/theme/styles.go (1)
362-362:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
MutedStyleusesFileRuleFginstead ofColorMutedText, causing poor contrast in light mode.In the light palette,
FileRuleFgis#d0d7dewhileColorMutedTextis#57606a. Using#d0d7defor muted text on a white background results in a contrast ratio of approximately 1.3:1, well below WCAG minimum (4.5:1). This would render help text and subtitles nearly invisible in light mode.Proposed fix
- MutedStyle: lipgloss.NewStyle().Foreground(lipgloss.Color(p.FileRuleFg)), + MutedStyle: lipgloss.NewStyle().Foreground(lipgloss.Color(p.ColorMutedText)),🤖 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 `@internal/adapters/in/tui/theme/styles.go` at line 362, The MutedStyle definition in the styles.go file is using FileRuleFg color which results in poor contrast for text elements in light mode. Change the MutedStyle assignment to use ColorMutedText instead of FileRuleFg as the foreground color. This will ensure that muted text elements like help text and subtitles maintain proper contrast ratios and remain visible in light mode.
🤖 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.
Outside diff comments:
In `@internal/adapters/in/tui/theme/styles.go`:
- Line 362: The MutedStyle definition in the styles.go file is using FileRuleFg
color which results in poor contrast for text elements in light mode. Change the
MutedStyle assignment to use ColorMutedText instead of FileRuleFg as the
foreground color. This will ensure that muted text elements like help text and
subtitles maintain proper contrast ratios and remain visible in light mode.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 41378ae4-54cf-4ce2-81ff-940350464588
📒 Files selected for processing (7)
internal/adapters/in/tui/model.gointernal/adapters/in/tui/model_theme_test.gointernal/adapters/in/tui/review_pane.gointernal/adapters/in/tui/review_pane_test.gointernal/adapters/in/tui/theme/styles.gointernal/adapters/in/tui/theme/styles_test.gointernal/app/app.go
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/adapters/in/tui/model_theme_test.go (1)
106-107: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueHard-coded color sequences are somewhat brittle.
Checking for the absence of specific ANSI color codes (
"48;5;236","48;2;31;42;68") will pass even if the dark palette changes to use different codes in the future. A more robust approach would assert the presence of expected light background colors rather than the absence of known dark codes.🤖 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 `@internal/adapters/in/tui/model_theme_test.go` around lines 106 - 107, The test assertions in model_theme_test.go are checking for the absence of dark palette color codes using NotContains, which is fragile because changes to the dark palette that use different codes would still pass the test. Replace the two NotContains assertions (checking for "48;5;236" and "48;2;31;42;68") with positive assertions that verify the presence of expected light background color codes instead, ensuring the test will catch any unintended palette changes in the future.
🤖 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 `@internal/adapters/in/tui/model_theme_test.go`:
- Around line 16-34: Add a cleanup handler to the
TestModelAutoUsesInitialSystemThemePreference test function for consistency with
other tests in the file. Insert a t.Cleanup() call at the beginning of the
subtest within the t.Run closure to ensure proper state cleanup after each test
iteration, even though the test doesn't appear to mutate global theme state.
In `@internal/core/theme.go`:
- Around line 57-69: The ParseSystemThemePreference function uses casts of iota
constants (SystemThemePreferDark and SystemThemePreferLight) in the switch
cases, creating implicit coupling between the constant ordering and the XDG
Desktop Portal protocol values. Replace the switch cases that cast these
constants with explicit literal values corresponding to the portal spec: use
case 1 for prefer-dark, case 2 for prefer-light, and ensure the default case
handles case 0 for no preference. This makes the wire-protocol contract explicit
and decouples the function from future reordering of the iota constants.
---
Outside diff comments:
In `@internal/adapters/in/tui/model_theme_test.go`:
- Around line 106-107: The test assertions in model_theme_test.go are checking
for the absence of dark palette color codes using NotContains, which is fragile
because changes to the dark palette that use different codes would still pass
the test. Replace the two NotContains assertions (checking for "48;5;236" and
"48;2;31;42;68") with positive assertions that verify the presence of expected
light background color codes instead, ensuring the test will catch any
unintended palette changes in the future.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3d952056-e034-428c-bed2-f1faea3b11ae
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
go.modinternal/adapters/in/systemtheme/portal.gointernal/adapters/in/tui/model.gointernal/adapters/in/tui/model_theme_test.gointernal/adapters/in/tui/render/line_cache.gointernal/adapters/in/tui/render/line_cache_test.gointernal/app/app.gointernal/app/app_test.gointernal/core/theme.gointernal/core/theme_test.gointernal/ports/theme.go
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
1 participant
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
Test Plan
Summary by CodeRabbit
Release Notes
--themeand--configCLI options to control theme mode and configuration loading.