Repository navigation
fix(yuctl): migrate lipgloss to charm.land/lipgloss/v2 - #795
zackpollard wants to merge 1 commit into
Conversation
 |
![futo-kritika[bot]](https://avatars.githubusercontent.com/in/5211018?s=60&v=4){kind=small}
| Out: colorprofile.NewWriter(os.Stdout, os.Environ()), | ||
| Err: colorprofile.NewWriter(os.Stderr, os.Environ()), |
There was a problem hiding this comment.
[important · tests] Add regression coverage for output-profile conversion
The migration moves escape stripping and color degradation from rendering to the stream writers, but the existing UI test only checks Sparkline/Meter and the view test checks substrings. Neither exercises these writers, so the central compatibility guarantees in the description have no regression coverage. Tests need to assert final emitted bytes, including stdout and stderr with different profiles, rather than just Style.Render output.
Suggested fix
Add automated tests for piped, truecolor, ANSI256, and NO_COLOR output, including independent stdout/stderr profiles and preservation of the muted palette index.
Prompt for a coding agent
Add output-profile regression tests for packages/yuctl/ui/iostreams.go lines 26–27, using a testable stream-construction helper if necessary. Cover independently detected stdout/stderr destinations, piped output without escape sequences, ANSI256 downsampling that preserves muted's palette index, truecolor rendering, and NO_COLOR behavior. Assert the emitted bytes after the colorprofile writer rather than only testing the rendered strings.
| // first render, so the background has to be queried explicitly. Resolving the | ||
| // pairs here keeps ANSI palette indexes intact; compat.AdaptiveColor would | ||
| // flatten them to RGB. | ||
| var lightDark = lipgloss.LightDark(lipgloss.HasDarkBackground(os.Stdin, os.Stdout)) |
There was a problem hiding this comment.
[important · reliability] Defer terminal probing until styled output is needed
This executes the terminal query during package initialization, before Cobra parses arguments, so even yuctl --help and commands that never render a styled view now probe the terminal. In v2.0.6, BackgroundColor switches the terminal to raw mode and queryTerminal reads input with a two-second timeout; it tries both terminal files after a failed query. On a TTY that does not answer these queries, otherwise immediate commands incur those waits and typed input is consumed by the query reader. The previous renderer queried lazily when adaptive colors were rendered.
Suggested fix
Defer background detection until themed rendering is requested, cache it with sync.Once, and keep package initialization free of terminal I/O. Add a nonresponding-PTY test for --help.
Prompt for a coding agent
In packages/yuctl/ui/theme.go at line 13, remove terminal querying from package-level initialization. Initialize the palette with a non-querying default and introduce a sync.Once-backed theme initialization/accessor that calls lipgloss.HasDarkBackground only when themed output is actually needed; update the styled rendering call sites to use it. Ensure plain commands, --help, and argument validation do not probe or read the terminal. Add a subprocess/PTY regression test with no terminal response showing --help exits without a background query.
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The migration is complete and consistent, with only a minor documentation correction identified.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Migrates yuctl to Lip Gloss v2 while preserving terminal color behavior.
Changes:
- Migrates styles to the v2 module and explicit light/dark detection.
- Adds output-profile wrappers for downsampling and piped output.
- Refreshes Go dependencies and checksums.
| File | Description |
|---|---|
packages/yuctl/ui/theme.go |
Adapts colors to Lip Gloss v2. |
packages/yuctl/ui/iostreams.go |
Adds profile-aware output writers. |
packages/yuctl/go.mod |
Updates module dependencies. |
packages/yuctl/go.sum |
Refreshes dependency checksums. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| // System binds the real terminal. lipgloss v2 renders full-fidelity truecolor | ||
| // and leaves degradation to the output layer, so the streams are wrapped to | ||
| // downsample (and strip, when piped or under NO_COLOR) per destination. |
a3d98af to
99b3b50
Compare
| "charm.land/lipgloss/v2" | ||
| ) | ||
|
|
||
| // lipgloss v2 dropped the global renderer that probed the terminal on first | ||
| // render, so the background has to be queried explicitly — and the query puts | ||
| // the tty into raw mode, so it must stay behind a once: resolving at package | ||
| // scope uncooks the terminal on every invocation, `--help` included, where a | ||
| // Ctrl-C before the deferred restore leaves the shell needing `stty sane`. | ||
| // Hence styles are functions rather than vars. compat.AdaptiveColor would skip | ||
| // the query but flattens color.Color to RGBA, turning muted's palette indexes | ||
| // 244/241 into 38;2;…. | ||
| var lightDark = sync.OnceValue(func() lipgloss.LightDarkFunc { | ||
| return lipgloss.LightDark(lipgloss.HasDarkBackground(os.Stdin, os.Stdout)) | ||
| }) |
go.modand could not build: the v2 module's path ischarm.land/lipgloss/v2(notgithub.com/charmbracelet/lipgloss/v2), andAdaptiveColorno longer exists in the root package.ui/theme.go: colours resolve throughlipgloss.LightDarkoverlipgloss.HasDarkBackground(os.Stdin, os.Stdout), behind async.OnceValue, with the exported styles as functions (ui.Muted()rather thanui.Muted). v2's query doesterm.MakeRawon the tty, so resolving it at package scope probed on every invocation:yuctl --helpwent 0.010s → 4.010s under a pty that does not answer, the query bytes\x1b]11;?\x07\x1b[clanded ahead of command payload, and a Ctrl-C inside that window left the terminal needingstty sane. The once restores v1's semantics — probe on first styled render only, which a pty harness confirms: unstyled commands emit no query and leaveICANON=1 ECHO=1.compat.AdaptiveColorwas rejected because it flattenscolor.Colorto RGBA, turningmuted's palette indexes 244/241 into38;2;…. All ten styles render identically before and after the laziness rework (Muted().Render("x")→\x1b[38;5;241mx\x1b[m).ui/System()wraps stdout/stderr in acolorprofile.Writer: v2'sRenderalways emits truecolor and leaves degradation to the output layer, so without this piped output would gain escape codes it never had.\x1b[mreset and#F5C542→245;197;66(v2) vs245;197;65(v1 rounding).go mod tidyalso drops four indirect entries (josharian/intern,mailru/easyjson,mxk/go-flowrate,gopkg.in/yaml.v3) that were already stale onmain.main