Adapt CLI colors to the terminal background (light/dark)#893
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (27)
✅ Files skipped from review due to trivial changes (10)
🚧 Files skipped from review as they are similar to previous changes (16)
📝 WalkthroughWalkthroughThis PR adds a shared ChangesRelated issues: None Sequence Diagram(s)sequenceDiagram
participant global.go setup
participant ClientConfig
participant theme package
participant Colors command
global.go setup->>ClientConfig: Theme()
ClientConfig-->>global.go setup: configured theme string
global.go setup->>theme package: Init(configuredTheme)
Colors command->>theme package: Current()
Colors command->>theme package: DetectedBackground()
Colors command->>theme package: Roles()
Poem: Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
cli/commands/color.go (1)
14-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
--format jsonsupport for the roles/diagnostic output.As per coding guidelines,
cli/commands/**/*.gocommands that output lists should support--format jsonvia theFormatOptionspattern.Colors()prints a list of semantic roles alongside resolved theme/background/profile without this option. Given the ANSI swatches are inherently visual, JSON support here has lower payoff than for typical tabular commands — worth a look if this command is expected to be scripted/parsed, otherwise low priority.🤖 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 `@cli/commands/color.go` around lines 14 - 37, `Colors()` in cli/commands/color.go currently prints a list of semantic roles and diagnostics as plain text only; add `FormatOptions` handling so it can honor `--format json` like other list-output commands. Update the `Colors` command signature and output path to branch on the requested format, preserving the existing human-readable ANSI swatches for default output and emitting structured JSON for parsed use cases. Use the existing `FormatOptions` pattern and keep `theme.Roles()`, `color.Background()`, and `profileName(lipgloss.ColorProfile())` as the key data sources.Source: Coding guidelines
cli/commands/app.go (1)
609-639: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInconsistent footer coloring between stack and horizontal modes.
The horizontal-mode footer (Line 636) now explicitly applies
theme.Muted, but the stack-mode footer just above (Lines 616-619) renders the "Updated:" text with plainlipgloss.NewStyle()and no color. This produces visually inconsistent output depending on which view mode is active. Also, sincefaint(Line 382) is alreadylipgloss.NewStyle().Foreground(theme.Muted), the horizontal-mode footer could reuse it instead of duplicatingForeground(theme.Muted).♻️ Proposed fix
footer = - lipgloss.NewStyle().Width(m.width).Align(lipgloss.Right).Render( + faint.Width(m.width).Align(lipgloss.Right).Render( fmt.Sprintf("Updated: %s", time.Now().Format(Stamp)), )footer = - lipgloss.NewStyle().Width(m.width - 3).Align(lipgloss.Right).Foreground(theme.Muted).Render( + faint.Width(m.width - 3).Align(lipgloss.Right).Render( fmt.Sprintf("Updated: %s", time.Now().Format(Stamp)), )🤖 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 `@cli/commands/app.go` around lines 609 - 639, The footer styling in the render path is inconsistent between the m.stack and horizontal branches in app.go. Update the stack-mode footer to use the same muted styling as the horizontal footer, and reuse the existing faint style instead of duplicating theme.Muted styling in the horizontal branch. Keep the Updated: render logic aligned across both branches so footer appearance is consistent regardless of layout.cli/commands/deploy.go (3)
665-669: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant local
faintStyleshadows the one already declared at Line 287.Inside the same
Deployfunction,faintStyleis already declared with the identicaltheme.Mutedforeground at Line 287. Re-declaring it here is unnecessary duplication.♻️ Reuse the existing style
if useExplainMode { if useOptimized && cachedFiles > 0 { - faintStyle := lipgloss.NewStyle().Foreground(theme.Muted) ctx.Printf(" %s\n", faintStyle.Render(fmt.Sprintf("Reused %d/%d files from previous deploy, uploading %d", cachedFiles, totalFiles, len(neededPaths))))🤖 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 `@cli/commands/deploy.go` around lines 665 - 669, The Deploy function redundantly re-declares faintStyle inside the useExplainMode/useOptimized block even though the same theme.Muted style already exists earlier in Deploy. Remove the local faintStyle declaration here and reuse the existing variable when rendering the cached-files message so the styling stays consistent and the code avoids duplication.
1373-1403: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBadge colors for distinct event kinds now collapse to the same theme role.
badgeFile/badgeDirboth render withtheme.Info, andbadgeConfig/badgeScriptboth render withtheme.Warning— the stale// blue/// cyanand// yellow/// orangecomments no longer describe distinct colors. Previously-distinguishable event kinds in the "Detection" output will now look visually identical. Consider usingBold/Italicor another differentiator to preserve visual distinction, or update/remove the now-inaccurate comments.Also applies to: 1406-1424
🤖 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 `@cli/commands/deploy.go` around lines 1373 - 1403, The badge styles in the Detection output are no longer visually distinct because `badgeFile` and `badgeDir` both use `theme.Info`, and `badgeConfig` and `badgeScript` both use `theme.Warning`. Update the style definitions near `analyzeTitleStyle`/`badgeFile`/`badgePackage`/`badgeFramework`/`badgeConfig`/`badgeDir`/`badgeScript` so each event kind remains distinguishable, either by assigning different theme roles or by adding another visual cue like `Bold`/`Italic`; also remove or correct the stale color comments that no longer match the actual styles.
1245-1250: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the warning theme role for markdown links
ui.RenderMarkdownLink(link, 208)still hardcodes the old warning ANSI index. Thread the warning role through instead so the link color follows the light/dark theme like the rest of the warning block.🤖 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 `@cli/commands/deploy.go` around lines 1245 - 1250, The warning block in the deploy output still hardcodes the old ANSI index when rendering the markdown link. Update the link rendering in the fields["link"] branch to thread the warning theme role through the existing ui.RenderMarkdownLink call instead of using the fixed 208 value, so it matches the rest of the warning styling. Use the surrounding detail/link printing logic in deploy and the ui.RenderMarkdownLink helper to locate the change.
🤖 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.
Nitpick comments:
In `@cli/commands/app.go`:
- Around line 609-639: The footer styling in the render path is inconsistent
between the m.stack and horizontal branches in app.go. Update the stack-mode
footer to use the same muted styling as the horizontal footer, and reuse the
existing faint style instead of duplicating theme.Muted styling in the
horizontal branch. Keep the Updated: render logic aligned across both branches
so footer appearance is consistent regardless of layout.
In `@cli/commands/color.go`:
- Around line 14-37: `Colors()` in cli/commands/color.go currently prints a list
of semantic roles and diagnostics as plain text only; add `FormatOptions`
handling so it can honor `--format json` like other list-output commands. Update
the `Colors` command signature and output path to branch on the requested
format, preserving the existing human-readable ANSI swatches for default output
and emitting structured JSON for parsed use cases. Use the existing
`FormatOptions` pattern and keep `theme.Roles()`, `color.Background()`, and
`profileName(lipgloss.ColorProfile())` as the key data sources.
In `@cli/commands/deploy.go`:
- Around line 665-669: The Deploy function redundantly re-declares faintStyle
inside the useExplainMode/useOptimized block even though the same theme.Muted
style already exists earlier in Deploy. Remove the local faintStyle declaration
here and reuse the existing variable when rendering the cached-files message so
the styling stays consistent and the code avoids duplication.
- Around line 1373-1403: The badge styles in the Detection output are no longer
visually distinct because `badgeFile` and `badgeDir` both use `theme.Info`, and
`badgeConfig` and `badgeScript` both use `theme.Warning`. Update the style
definitions near
`analyzeTitleStyle`/`badgeFile`/`badgePackage`/`badgeFramework`/`badgeConfig`/`badgeDir`/`badgeScript`
so each event kind remains distinguishable, either by assigning different theme
roles or by adding another visual cue like `Bold`/`Italic`; also remove or
correct the stale color comments that no longer match the actual styles.
- Around line 1245-1250: The warning block in the deploy output still hardcodes
the old ANSI index when rendering the markdown link. Update the link rendering
in the fields["link"] branch to thread the warning theme role through the
existing ui.RenderMarkdownLink call instead of using the fixed 208 value, so it
matches the rest of the warning styling. Use the surrounding detail/link
printing logic in deploy and the ui.RenderMarkdownLink helper to locate the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: da1e1346-c01e-4e95-8ec0-e75efa61a7bb
📒 Files selected for processing (26)
cli/commands/admin.gocli/commands/app.gocli/commands/app_history.gocli/commands/app_status.gocli/commands/cluster.gocli/commands/color.gocli/commands/config_bind_helpers.gocli/commands/deploy.gocli/commands/deploy_ui.gocli/commands/doctor.gocli/commands/env.gocli/commands/global.gocli/commands/help_render.gocli/commands/init.gocli/commands/tea_utils.goclientconfig/clientconfig.gopkg/progress/progressui/display.gopkg/theme/theme.gopkg/theme/theme_test.gopkg/ui/definition_list.gopkg/ui/hint.gopkg/ui/named_value.gopkg/ui/picker.gopkg/ui/prompt.gopkg/ui/status.gopkg/ui/table.go
phinze
force-pushed
the
phinze/mir-1312-cli-colors-are-hard-to-read-on-light-background-terminals
branch
from
e6a9769 to
0558ffa
Compare
The miren CLI's palette was tuned for dark terminals and hard-coded as bright 256-color ANSI indices scattered across ~20 files. On a light background the yellow headers washed out and the Faint(true) secondary text dropped below readable contrast. Introduce a central pkg/theme package that exposes semantic color roles (Header, Success, Warning, ...) as profile-aware CompleteAdaptiveColor values, resolved once at startup from the terminal background. Detection honors NO_COLOR / FORCE_COLOR / CLICOLOR, a MIREN_THEME env var, and a `theme` client-config field, falling back to an OSC 11 probe and finally to dark. The scattered color sites move onto the semantic roles, and Faint(true) becomes a real muted foreground. Each role carries per-profile values: truecolor terminals get jewel tones, 256-color terminals get xterm anchor indices so distinct roles don't collapse into one muddy bucket, and interactive terminals with a TERM termenv doesn't recognize floor to ANSI256 instead of going monochrome.
phinze
force-pushed
the
phinze/mir-1312-cli-colors-are-hard-to-read-on-light-background-terminals
branch
from
0558ffa to
d819d04
Compare
phinze
deleted the
phinze/mir-1312-cli-colors-are-hard-to-read-on-light-background-terminals
branch
2 participants
A community member on a light-background terminal reported the
mirenCLI was hard to read. The palette was tuned for dark terminals and hard-coded as bright 256-color ANSI indices spread across about twenty files, so on white the yellow section headers and table headings washed out and theFaint(true)secondary text dropped below readable contrast. There was no central theme, and nothing ever told lipgloss the background color, so even the loneAdaptiveColoralready in the tree silently always picked its dark variant.This adds a
pkg/themepackage that owns the palette. It exposes semantic roles (Header, Success, Warning, Error, Info, Muted, Highlight) instead of raw indices, and resolves the right variant once at startup. Detection follows the usual conventions:NO_COLOR/CLICOLOR=0turn color off,FORCE_COLOR/CLICOLOR_FORCEturn it on, aMIREN_THEMEenv var or athemefield inclientconfig.yamloverrides everything, and otherwise it auto-detects from the terminal background (aCOLORFGBGhint, then an OSC 11 query, then dark as the default). All the scattered call sites move onto the semantic roles, andFaint(true)becomes a real muted foreground, which is the biggest readability win on light backgrounds.The roles are
CompleteAdaptiveColor, so each carries explicit values per color profile: truecolor terminals get saturated jewel tones, and 256-color terminals get values pinned to xterm anchor indices so two distinct roles never quantize into the same bucket. Interactive terminals whoseTERMthis termenv build doesn't recognize (Ghostty over SSH, for one) floor to ANSI256 rather than rendering monochrome.There's also a
miren debug colorscommand that prints the resolved theme, detected background, active profile, and a swatch of every role, so a misdetection report comes with paste-able diagnostics.Closes MIR-1312