feat(styles,console,colorwriter): hoist palette color.Color vars and honor NO_COLOR on stdout#46197
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…colorprofile degradation for stdout Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Hoists shared palette colors and adds environment-aware ANSI degradation for stdout formatting.
Changes:
- Reuses package-level palette values across themes.
- Adds stdout and string degradation helpers.
- Applies color-profile handling to selected console output.
Show a summary per file
| File | Description |
|---|---|
pkg/styles/theme.go |
Defines shared palette colors. |
pkg/styles/huh_theme.go |
Reuses shared colors. |
pkg/console/console.go |
Adds stdout color degradation. |
pkg/colorwriter/colorprofile_writer.go |
Adds Stdout and Degrade. |
pkg/colorwriter/colorprofile_writer_wasm.go |
Adds wasm pass-through stubs. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 4
- Review effort level: Medium
| // applyStyleWithTTY conditionally applies styling based on a provided TTY check. | ||
| // Used for stderr-bound helpers where the rendered string is subsequently printed | ||
| // through stderrWriter() (which also applies colorprofile degradation). |
| if !isTTY() { | ||
| return text | ||
| } | ||
| return colorwriter.Degrade(style.Render(text), os.Environ()) |
| func Degrade(s string, environ []string) string { | ||
| var buf strings.Builder | ||
| w := colorprofile.NewWriter(&buf, environ) | ||
| fmt.Fprint(w, s) | ||
| return buf.String() |
| // Stdout returns a color-profile-aware writer for os.Stdout using the current | ||
| // process environment. | ||
| func Stdout() io.Writer { | ||
| return New(os.Stdout, os.Environ()) | ||
| } |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — requesting changes on test coverage and incomplete NO_COLOR propagation.
📋 Key Themes & Highlights
Key Themes
- Incomplete NO_COLOR coverage:
applyStyleis now degraded, butRenderTableand theapplyStyleWithTTY-backed stdout helpers are not — the fix is partial. See inline comment onconsole.go:39. - Missing tests for
Degrade: The newcolorwriter.Degradepath has no test coverage despite an existing test file. See inline comment oncolorprofile_writer.go:40. - Stale docs:
pkg/colorwriter/README.mdstill only documentsNewandStderr. See inline comment oncolorprofile_writer.go:30. applyStyleWithTTYscope: The comment claiming it is "stderr-bound" is inaccurate — several stdout-facing helpers also use it. See inline comment onconsole.go:44.
Positive Highlights
- ✅ Clean extraction of 22
color.Colorpackage-level vars intheme.go— eliminates repeated hex-to-color parsing and creates a real single source of truth. - ✅
huh_theme.gosimplification is mechanical and easy to verify. - ✅ Wasm stubs are symmetric and correctly pass-through.
- ✅ Good PR description with before/after snippet.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 13.3 AIC · ⌖ 4.39 AIC · ⊞ 6.7K
Comment /matt to run again
There was a problem hiding this comment.
Review: feat(styles,console,colorwriter): hoist palette color.Color vars and honor NO_COLOR on stdout
The palette-hoisting in pkg/styles is clean and correct — a straightforward refactor with no behavioral risk.
The NO_COLOR / color-profile fix in pkg/console and pkg/colorwriter is on the right track, but four non-blocking issues are worth addressing before considering this complete:
applyStyleWithTTYis mischaracterized as stderr-only — several stdout-bound helpers (FormatSuccessMessage,FormatInfoMessage, etc.) call it directly; the comment claiming it is stderr-bound is incorrect and the refactor is incomplete.RenderTablestill emits raw ANSI — its header/cell/border styles bypassDegradeentirely, soNO_COLOR=1with a TTY still produces colored table output.- No test coverage for
Degrade— the package already hasspec_test.go; a round-trip test withNO_COLOR=1and a non-trivial ANSI input would prevent regressions. - README is stale —
pkg/colorwriter/README.mddocuments onlyNewandStderrand should includeStdoutandDegrade.
These are correctness/completeness gaps rather than blockers. The palette hoist can land as-is; the color-profile path deserves the follow-ups above.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 12.3 AIC · ⌖ 4.38 AIC · ⊞ 5K
There was a problem hiding this comment.
Two non-blocking issues worth fixing
The changes are directionally correct — honoring NO_COLOR/COLORTERM/TERM on stdout is the right fix, and consolidating the palette into package-level vars eliminates redundant hex parsing. Two implementation concerns:
-
Performance:
os.Environ()is copied andcolorprofile.NewWriterre-parses the environment on everyapplyStylecall. In output-heavy paths this is O(N × env-size) unnecessary work. Cache the env snapshot withsync.Once(see inline comment). -
Correctness risk:
Degradecallsbuf.String()immediately afterfmt.Fprint(w, ...)with no flush or close on thecolorprofile.Writer. If the library buffers internally (or a future version does), the returned string could be silently truncated. Either check forio.Closer/io.Flusheror document the no-buffer contract (see inline comment).
🔎 Code quality review by PR Code Quality Reviewer · 39.1 AIC · ⌖ 5.27 AIC · ⊞ 5.6K
Comment /review to run again
| var buf strings.Builder | ||
| w := colorprofile.NewWriter(&buf, environ) | ||
| fmt.Fprint(w, s) | ||
| return buf.String() |
There was a problem hiding this comment.
Possible incomplete output if colorprofile.Writer buffers: buf.String() is called immediately after fmt.Fprint(w, s) with no flush or close, so if colorprofile.Writer buffers internally, the returned string may be silently truncated or empty.
💡 Suggested fix
colorprofile.Writer wraps an io.Writer and may implement io.Closer or have a Flush method to drain any internal buffer before the underlying writer holds the final bytes. Without seeing the contract guaranteed by colorprofile.NewWriter, the safe pattern is:
func Degrade(s string, environ []string) string {
var buf strings.Builder
w := colorprofile.NewWriter(&buf, environ)
fmt.Fprint(w, s)
if c, ok := w.(io.Closer); ok {
_ = c.Close()
}
return buf.String()
}Or, if the library guarantees synchronous pass-through writes (no buffering), add a comment asserting that contract so the next reader does not have to reverse-engineer it.
| return applyStyleWithTTY(style, text, isTTY) | ||
| if !isTTY() { | ||
| return text | ||
| } |
There was a problem hiding this comment.
os.Environ() copied and env re-parsed on every styled token: applyStyle calls colorwriter.Degrade(style.Render(text), os.Environ()) — copying the full process environment and re-parsing the color profile on each invocation, including in render loops.
💡 Suggested fix
Cache the environment snapshot and ideally the resolved profile at first use:
var (
stdoutEnvOnce sync.Once
stdoutEnv []string
)
func getStdoutEnv() []string {
stdoutEnvOnce.Do(func() { stdoutEnv = os.Environ() })
return stdoutEnv
}
func applyStyle(style lipgloss.Style, text string) string {
if !isTTY() {
return text
}
return colorwriter.Degrade(style.Render(text), getStdoutEnv())
}This brings the per-call cost from O(env-size) down to a single pointer load after the first call.
Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (106 new lines across Draft ADR committed:
What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
|
@copilot please run the Open review feedback to address:
Please cover the incomplete
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in Completed the remaining stdout Validation run locally:
|
|
🎉 This pull request is included in a new release. Release: |
Two low-priority cleanup items from the lipgloss v2 Go Fan review: eliminate redundant hex→color.Color parsing in
huh_theme.go, and fixNO_COLOR/COLORTERM/TERMnot being honored on styled stdout output.Hoist hex constants to package-level
color.Colorvars (pkg/styles)huh_theme.gowas callinglipgloss.Color(hexColorXxx)on everyHuhTheme(isDark)invocation — the same parsingtheme.goalready does once at package init.color.Colorvars (colorErrorLight,colorErrorDark, …) derived once from thehexColor*constants.theme.go'sadaptiveColorstructs andhuh_theme.go'slightDark()calls now both reference these shared vars.adaptiveColorvs per-renderLightDark) explicit.Honor color profile on stdout (
pkg/colorwriter,pkg/console)applyStylegated on a TTY check only —NO_COLOR=1with a TTY still emitted raw ANSI.colorwriter.Stdout()(symmetric with existingStderr()).colorwriter.Degrade(s, environ): routes a rendered string through acolorprofile.Writerbacked by astrings.Builder, downgrading/stripping ANSI per env without writing to stdout directly.applyStylenow callscolorwriter.Degrade()when stdout is a TTY, so all string-returning format helpers honorNO_COLOR,COLORTERM, andTERM.applyStyleWithTTY(stderr-bound helpers) is unchanged; those paths already get colorprofile degradation at print time viastderrWriter().colorwriterfunctions (pass-through).