Skip to content

Extract inline lipgloss styles and harden ShowWelcomeBanner styling #39246

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 3 commits into from
Jun 14, 2026
+41 −7
Merged

Extract inline lipgloss styles and harden ShowWelcomeBanner styling #39246

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ba1dee8
Initial plan
Copilot Jun 14, 2026
7c7374f
Extract inline lipgloss styles and style ShowWelcomeBanner
Copilot Jun 14, 2026
add907a
Fix stderr banner styling and wasm style stubs
Copilot Jun 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions pkg/cli/compile_schedule_calendar.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,20 +200,20 @@ func intensityChar(count int) string {
func intensityStyle(count int, isTerminal bool) lipgloss.Style {
if !isTerminal {
// Keep glyph rendering unchanged while preventing ANSI escapes in piped output.
return lipgloss.NewStyle()
return lipgloss.Style{}
}

switch {
case count == 0:
return lipgloss.NewStyle().Foreground(styles.ColorComment)
return styles.ScheduleCalendarEmpty
case count == 1:
return lipgloss.NewStyle().Foreground(styles.ColorInfo)
return styles.ScheduleCalendarLow
case count <= 3:
return lipgloss.NewStyle().Foreground(styles.ColorSuccess)
return styles.ScheduleCalendarMedium
case count <= 6:
return lipgloss.NewStyle().Foreground(styles.ColorWarning)
return styles.ScheduleCalendarHigh
default:
return lipgloss.NewStyle().Bold(true).Foreground(styles.ColorError)
return styles.ScheduleCalendarCritical
}
}

Expand Down
5 changes: 3 additions & 2 deletions pkg/console/terminal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
"fmt"
"os"

"github.com/github/gh-aw/pkg/styles"
"github.com/github/gh-aw/pkg/tty"
)

Expand Down Expand Up @@ -40,8 +41,8 @@
func ShowWelcomeBanner(description string) {
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!"))

Check failure on line 44 in pkg/console/terminal.go

View workflow job for this annotation

GitHub Actions / build 

undefined: applyStyle

Check failure on line 44 in pkg/console/terminal.go

View workflow job for this annotation

GitHub Actions / build-wasm 

undefined: applyStyle

@github-actions github-actions Bot Jun 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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, "")
fmt.Fprintln(os.Stderr, description)
fmt.Fprintln(os.Stderr, FormatInfoMessage(description))

@github-actions github-actions Bot Jun 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

fmt.Fprintln(os.Stderr, "")
}
23 changes: 23 additions & 0 deletions pkg/styles/theme.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,3 +347,26 @@ var TreeEnumerator = lipgloss.NewStyle().
// TreeNode style for tree node content
var TreeNode = lipgloss.NewStyle().
Foreground(ColorForeground)

// Schedule calendar intensity styles for the heatmap renderer

// ScheduleCalendarEmpty style for zero-trigger slots - muted
var ScheduleCalendarEmpty = lipgloss.NewStyle().

@github-actions github-actions Bot Jun 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Foreground(ColorComment)

// ScheduleCalendarLow style for low-trigger slots (count == 1) - cyan

@github-actions github-actions Bot Jun 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

var ScheduleCalendarLow = lipgloss.NewStyle().
Foreground(ColorInfo)

// ScheduleCalendarMedium style for medium-trigger slots (count 2–3) - green
var ScheduleCalendarMedium = lipgloss.NewStyle().
Foreground(ColorSuccess)

// ScheduleCalendarHigh style for high-trigger slots (count 4–6) - orange
var ScheduleCalendarHigh = lipgloss.NewStyle().
Foreground(ColorWarning)

// ScheduleCalendarCritical style for critical-trigger slots (count 7+) - bold red
var ScheduleCalendarCritical = lipgloss.NewStyle().
Bold(true).
Foreground(ColorError)
Loading