Conversation
But to avoid leaking unread ANSI sequences to the terminal, don't try to switch back to cooked mode before launching TUI. Instead, stay in raw mode until exit. Also, don't use a partially-detected palette. It should be all or nothing.
There was a problem hiding this comment.
Pull request overview
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
This PR restores a simplified terminal color-detection flow for the UI, keeping the terminal in raw mode through the TUI lifetime and enforcing “all-or-nothing” palette detection to avoid partially-applied themes and stray escape sequences.
Changes:
- Introduces
WithRawTerminaland wraps the full TUI run in raw terminal mode. - Refactors palette initialization/detection to be “complete detection or fallback,” with ANSI-safe behavior when true color isn’t enabled.
- Reworks OSC/DA1 parsing and expands tests to validate complete detection, COLORTERM overrides, and ANSI fallbacks.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/ui/terminal.go | Adds a helper to switch /dev/tty into raw mode for the duration of the TUI run. |
| internal/ui/app.go | Runs palette detection + Bubble Tea program inside WithRawTerminal, and toggles truecolor profile based on the palette. |
| internal/ui/palette.go | Replaces exported constructors with a default palette + Detect() that upgrades synthesized colors only when true color is enabled. |
| internal/ui/palette_detect.go | Rewrites terminal color detection to parse OSC/CSI synchronously and require complete (18/18) results. |
| internal/ui/styles.go | Initializes global Colors from the new default palette function. |
| internal/ui/dashboard_panel.go | Avoids rendering transition corners when BackgroundTint is unavailable. |
| internal/ui/*_test.go | Updates and expands tests to cover new detection semantics and ANSI fallback behavior. |
Comments suppressed due to low confidence (1)
internal/ui/dashboard_panel.go:221
- BackgroundTint can now be nil when true color isn’t enabled, and these transition helpers already guard for that. However,
DashboardPanel.Viewstill unconditionally callsbodyStyle.Background(Colors.BackgroundTint)andWithBackground(Colors.BackgroundTint, ...)whenselected, which may behave incorrectly or panic with a nil color. Consider updating the selected-body rendering to also skip background rendering whenColors.BackgroundTint == nil(consistent with these guards).
func (p DashboardPanel) renderTopTransition(selected bool, width int) string {
if !selected || Colors.BackgroundTint == nil {
return strings.Repeat(" ", width)
}
indicatorChar := lipgloss.NewStyle().Foreground(Colors.Focused).Render("▗")
bodyChars := lipgloss.NewStyle().Foreground(Colors.BackgroundTint).Render(strings.Repeat("▄", width-1))
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| time.AfterFunc(timeout, func() { tty.Close() }) | ||
| return detectFrom(tty) |
There was a problem hiding this comment.
DetectTerminalColors opens /dev/tty but never closes it on the success path; it relies on time.AfterFunc to close later, and the timer is never stopped. This can leak a file descriptor for up to timeout (or indefinitely if timeout is very large) and may close the fd after the function has already returned. Consider defer tty.Close() plus a timer you Stop() when detection finishes (or defer timer.Stop() if you keep AfterFunc).
| time.AfterFunc(timeout, func() { tty.Close() }) | |
| return detectFrom(tty) | |
| timer := time.AfterFunc(timeout, func() { tty.Close() }) | |
| colors, ok := detectFrom(tty) | |
| if !timer.Stop() { | |
| // Timer already fired and closed tty (or is in the process of doing so). | |
| return colors, ok | |
| } | |
| _ = tty.Close() | |
| return colors, ok |
| // Palette holds all colors used by the UI. ANSI color fields always contain | ||
| // BasicColor values so the terminal applies its own theme. Synthesized | ||
| // colors (FocusOrange, BackgroundTint, LightText) are true-color RGB. | ||
| // BasicColor values so the terminal applies its own theme. The synthesized | ||
| // colors (FocusOrange, BackgroundTint, LightText) are true-color RGB when | ||
| // the terminal supports it, or ANSI fallbacks otherwise. |
There was a problem hiding this comment.
The doc comment says synthesized colors (including BackgroundTint) use “ANSI fallbacks otherwise”, but BackgroundTint is intentionally left nil when true color isn’t enabled. Consider updating the comment to reflect that BackgroundTint may be nil (and callers should guard), rather than implying an ANSI fallback exists.
| // synthesized colors are computed from the detected RGB values. | ||
| // If COLORTERM indicates true-color support, synthesized colors | ||
| // are computed from fallback samples. Otherwise the ANSI defaults | ||
| // from defaultPalette are kept. |
There was a problem hiding this comment.
Detect calls DetectTerminalColors, which requires the terminal to already be in raw mode (per the comment in palette_detect.go). The Detect docstring doesn’t mention this precondition, so it’s easy to call it elsewhere and get unreliable results/timeouts. Consider documenting the raw-mode requirement here (or moving the raw-mode handling inside Detect).
| // from defaultPalette are kept. | |
| // from defaultPalette are kept. | |
| // | |
| // DetectTerminalColors requires the terminal to already be in raw mode; | |
| // callers of Detect are responsible for putting the terminal into raw mode | |
| // before calling this method and restoring the previous state afterwards. | |
| // Calling Detect without raw mode can lead to unreliable results or timeouts. |
But to avoid leaking unread ANSI sequences to the terminal, don't try to switch back to cooked mode before launching TUI. Instead, stay in raw mode until exit.
Also, don't use a partially-detected palette. It should be all or nothing.