Extract inline lipgloss styles and harden ShowWelcomeBanner styling#39246
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR centralizes terminal styling by moving schedule heatmap (“schedule calendar”) intensity styles into pkg/styles and by styling the interactive welcome banner output, reducing ad-hoc lipgloss.NewStyle() usage outside the styles layer.
Changes:
- Added named schedule heatmap intensity styles to
pkg/styles/theme.go. - Updated the schedule heatmap renderer to use the new centralized style variables and avoid ANSI in non-TTY output.
- Styled
ShowWelcomeBanneroutput using the console/styles abstractions.
Show a summary per file
| File | Description |
|---|---|
| pkg/styles/theme.go | Adds named schedule calendar intensity styles to keep style definitions centralized. |
| pkg/cli/compile_schedule_calendar.go | Replaces inline lipgloss.NewStyle() calls with the new styles.ScheduleCalendar* variables and uses a zero-value style for non-TTY output. |
| pkg/console/terminal.go | Styles the welcome banner headline/description output to align with the console styling approach. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 1
| fmt.Fprintln(os.Stderr, "") | ||
| fmt.Fprintln(os.Stderr, "🚀 Welcome to GitHub Agentic Workflows!") | ||
| fmt.Fprintln(os.Stderr, applyStyle(styles.Header, "🚀 Welcome to GitHub Agentic Workflows!")) | ||
| fmt.Fprintln(os.Stderr, "") | ||
| fmt.Fprintln(os.Stderr, description) | ||
| fmt.Fprintln(os.Stderr, FormatInfoMessage(description)) |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. The PR only changes production files: pkg/cli/compile_schedule_calendar.go, pkg/console/terminal.go, and pkg/styles/theme.go. Test Quality Sentinel skipped. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed for PR #39246: the PR does not have the implementation label (has_implementation_label=false) and has only 32 new lines of code in business logic directories, which is at or below the 100-line threshold (requires_adr_by_default_volume=false). Neither Condition A nor Condition B is met. |
![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 /zoom-out and /improve-codebase-architecture — requesting changes on one correctness issue and two design concerns.
📋 Key Themes & Highlights
Key Issues
- stderr/stdout TTY mismatch (
terminal.goline 44):applyStylecheckstty.IsStdoutTerminal()but the output goes toos.Stderr. This can silently misfire — ANSI codes leaking into piped stderr, or missing styling when stdout is piped.ClearScreen/ClearLinein the same file usetty.IsStderrTerminal()as the correct precedent. - Semantic mismatch with
FormatInfoMessage(terminal.goline 46): the"i "icon prefix belongs to notification-style messages, not to a banner description. The change unintentionally redefines the welcome screen's visual vocabulary. - Count thresholds leaked into style doc comments (
theme.go): counts areintensityStyle's concern; keeping them out oftheme.godocs makes both files independently maintainable.
Positive Highlights
- ✅ Clean, mechanical 1:1 replacement of inline
lipgloss.NewStyle()calls inintensityStyle()— correct and risk-free. - ✅
lipgloss.Style{}zero-value substitution for the non-terminal path is semantically equivalent and idiomatic. - ✅ Named constants follow the established
pkg/stylesnaming convention perfectly. - ✅ Well-structured PR description with before/after snippets.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 202.8 AIC · ⌖ 13.5 AIC · ⊞ 29.4K
| ClearScreen() | ||
| fmt.Fprintln(os.Stderr, "") | ||
| fmt.Fprintln(os.Stderr, "🚀 Welcome to GitHub Agentic Workflows!") | ||
| fmt.Fprintln(os.Stderr, applyStyle(styles.Header, "🚀 Welcome to GitHub Agentic Workflows!")) |
There was a problem hiding this comment.
[/zoom-out] applyStyle uses isTTY() → tty.IsStdoutTerminal(), but this function writes to os.Stderr. ClearScreen and ClearLine in the same file correctly guard on tty.IsStderrTerminal(). If the user pipes stdout (e.g. gh aw run ... | tee log), isTTY() returns false and the header renders unstyled; conversely, if stderr is redirected to a file while stdout is a terminal, ANSI codes leak into the file.
💡 Suggested fix
Either wrap the call in an explicit stderr TTY check:
header := "🚀 Welcome to GitHub Agentic Workflows!"
if tty.IsStderrTerminal() {
header = styles.Header.Render(header)
}
fmt.Fprintln(os.Stderr, header)Or add a complementary applyStderrStyle helper alongside applyStyle in console.go that gates on tty.IsStderrTerminal() — keeping the abstraction consistent.
| fmt.Fprintln(os.Stderr, applyStyle(styles.Header, "🚀 Welcome to GitHub Agentic Workflows!")) | ||
| fmt.Fprintln(os.Stderr, "") | ||
| fmt.Fprintln(os.Stderr, description) | ||
| fmt.Fprintln(os.Stderr, FormatInfoMessage(description)) |
There was a problem hiding this comment.
[/improve-codebase-architecture] FormatInfoMessage prepends a styled "i " icon, turning the workflow description into a notification-style info message. A welcome banner description is not an info notification — it is the workflow's purpose statement. Using FormatInfoMessage changes the visual contract of the banner and could confuse users who see info-prefix styling in a greeting context.
💡 Consider a neutral approach
For a welcome-banner description, plain text or a subtle wrap is more appropriate:
// Option A: keep it plain (matches the original intent)
fmt.Fprintln(os.Stderr, description)
// Option B: apply a muted/normal style without the "i " icon
if tty.IsStderrTerminal() {
fmt.Fprintln(os.Stderr, styles.Muted.Render(description))
} else {
fmt.Fprintln(os.Stderr, description)
}Reserve FormatInfoMessage for actual informational notices, keeping the vocabulary consistent with the rest of the console package.
| var ScheduleCalendarEmpty = lipgloss.NewStyle(). | ||
| Foreground(ColorComment) | ||
|
|
||
| // ScheduleCalendarLow style for low-trigger slots (count == 1) - cyan |
There was a problem hiding this comment.
[/improve-codebase-architecture] The doc comments embed the count thresholds from intensityStyle ("count == 1", "count 2–3", "count 4–6", "count 7+"). This couples the style definitions to a single caller's threshold values. If the thresholds in intensityStyle are ever adjusted, these comments silently become stale — or worse, mislead a future caller who reuses these vars for a different heatmap.
💡 Suggested wording
Document semantic intent only — leave threshold specifics to intensityStyle:
// ScheduleCalendarLow style for low-intensity calendar slots - cyan
var ScheduleCalendarLow = lipgloss.NewStyle().
Foreground(ColorInfo)
// ScheduleCalendarMedium style for medium-intensity calendar slots - green
var ScheduleCalendarMedium = lipgloss.NewStyle().
Foreground(ColorSuccess)This keeps theme.go as a pure presentation layer and avoids leaking data-model details (count thresholds) into the style package.
There was a problem hiding this comment.
Two build-breaking issues must be fixed before this merges.
### Blocking issues
1. applyStyle undefined in wasm builds (already flagged in existing comment)
terminal.go has no build constraint so it compiles for every target, including js/wasm. applyStyle lives only in console.go (//go:build !js && !wasm), with no stub in console_wasm.go. Since pkg/parser and pkg/workflow import pkg/console, the wasm binary (make build-wasm) breaks immediately. The banner also gates styling on tty.IsStdoutTerminal() while writing to os.Stderr; the two should be co-fixed by guarding on tty.IsStderrTerminal() and using styles.Header.Render() directly.
2. Five new ScheduleCalendar* vars have no wasm stubs (see inline comment)
theme.go (!js && !wasm) adds ScheduleCalendarEmpty/Low/Medium/High/Critical. compile_schedule_calendar.go has no build constraint and references all five. theme_wasm.go has zero-value stubs for every other exported style var, but these five are missing — GOOS=js GOARCH=wasm go build ./... will fail.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · 460.1 AIC · ⌖ 13.2 AIC · ⊞ 17.3K
| // Schedule calendar intensity styles for the heatmap renderer | ||
|
|
||
| // ScheduleCalendarEmpty style for zero-trigger slots - muted | ||
| var ScheduleCalendarEmpty = lipgloss.NewStyle(). |
There was a problem hiding this comment.
Missing wasm stubs for 5 new ScheduleCalendar* vars will break go build ./... under GOOS=js GOARCH=wasm.
theme.go has //go:build !js && !wasm, so these vars are invisible to wasm. compile_schedule_calendar.go has no build constraint and references all five: styles.ScheduleCalendarEmpty, ScheduleCalendarLow, ScheduleCalendarMedium, ScheduleCalendarHigh, ScheduleCalendarCritical. Any wasm build covering ./... will fail with undefined: styles.ScheduleCalendarEmpty (and four siblings).
💡 Suggested fix
Add the five stubs to pkg/styles/theme_wasm.go, matching the pattern already there:
// Schedule calendar intensity styles (no-op in Wasm)
var (
ScheduleCalendarEmpty = WasmStyle{}
ScheduleCalendarLow = WasmStyle{}
ScheduleCalendarMedium = WasmStyle{}
ScheduleCalendarHigh = WasmStyle{}
ScheduleCalendarCritical = WasmStyle{}
)Every other style var in theme.go has a corresponding stub in theme_wasm.go; the five new ones are the only exceptions.
|
Hey One thing that would strengthen this before merge:
If you'd like a hand, you can assign this prompt to your coding agent:
|
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. I ran the PR finisher flow and pushed follow-up fixes in |
Two style fixes to stop bypassing the centralized
pkg/stylesabstraction: inlinelipgloss.NewStyle()calls in the schedule-calendar renderer and unstyled output inShowWelcomeBanner.pkg/styles/theme.goAdded five named vars for schedule-calendar intensity levels, keeping style definitions in the styles layer:
ScheduleCalendarEmpty·ScheduleCalendarLow·ScheduleCalendarMedium·ScheduleCalendarHigh·ScheduleCalendarCriticalAlso updated the schedule-calendar style comments to describe semantic intensity levels rather than embedding threshold logic.
pkg/styles/theme_wasm.goAdded wasm no-op stubs for the five new schedule-calendar styles so wasm builds have matching exported symbols:
ScheduleCalendarEmpty·ScheduleCalendarLow·ScheduleCalendarMedium·ScheduleCalendarHigh·ScheduleCalendarCriticalpkg/cli/compile_schedule_calendar.goReplaced all six inline
lipgloss.NewStyle()calls inintensityStyle()with the newstyles.*vars. Non-terminal (no-ANSI) case uses the zero valuelipgloss.Style{}.pkg/console/terminal.goStyled the banner headline directly with
styles.Header.Render(...)only whentty.IsStderrTerminal()is true (matching stderr output), and kept the description as plain text.