feat(v2/cli): replace pterm/ctc/gommon with unified glyph TUI + spinners#5403
feat(v2/cli): replace pterm/ctc/gommon with unified glyph TUI + spinners#5403taliesin-ai wants to merge 1 commit into
Conversation
Consolidates the CLI's five overlapping TUI libraries (pterm, wzshiming/ctc, labstack/gommon/color, gookit/color, charmbracelet/glamour) down to a single coherent system: a new internal/tui package for all styled output plus glyph for animated spinners on interactive terminals. - Add v2/internal/tui: ANSI-direct color helpers, NO_COLOR/non-TTY detection, and WithSpinner() that wraps glyph's NewInlineApp() for build/init/update/ generate operations - Rewrite v2/internal/colour/colour.go to delegate to tui (keeps API compat) - Remove pterm, wzshiming/ctc, labstack/gommon/color imports from all CLI cmds - Add github.com/kungfusheep/glyph v0.6.0 as direct dependency - glamour kept for markdown rendering (release notes) Co-authored-by: multica-agent <github@multica.ai>
WalkthroughThis PR comprehensively refactors the Wails CLI from pterm-based rendering to an internal tui package, adding spinner animations via the glyph library and standardizing color/styling handling with NO_COLOR support. The old internal/colour package becomes a backward-compatibility wrapper. ChangesCLI Terminal UI Migration to TUI Package
Sequence Diagram(s)sequenceDiagram
participant User as User (CLI)
participant Cmd as Command Handler
participant TUI as tui Package
participant Op as Operation
User->>Cmd: Run build/generate/update
Cmd->>TUI: WithSpinner("message", fn)
TUI->>TUI: Initialize spinner animation
TUI->>Op: Execute fn()
Op-->>TUI: Success or Error
alt Operation Succeeds
TUI->>TUI: Show checkmark
TUI-->>Cmd: Return nil
Cmd->>User: Continue/Done
else Operation Fails
TUI->>TUI: Show ✗ with error
TUI-->>Cmd: Return error
Cmd->>TUI: tui.Error(message)
Cmd->>User: Exit with error
end
sequenceDiagram
participant Doc as Doctor Command
participant TUI as tui Package
participant Shell as Shell/System
Doc->>TUI: Section("Wails Doctor")
Doc->>TUI: WithSpinner("Scanning system", fn)
TUI->>Shell: Gather system info
Shell-->>TUI: Info collected
Doc->>TUI: Section("Wails Info")
Doc->>TUI: Table(build info rows)
Doc->>TUI: Section("Dependencies")
Doc->>TUI: HeaderTable(dep rows)
Doc->>TUI: Section("Diagnosis")
alt All required deps present
Doc->>TUI: Success("Ready to go!")
else Missing deps
Doc->>TUI: Warning("Missing dependencies")
end
Doc->>TUI: Println("Installation instructions")
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.1)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
v2/go.mod (1)
23-23:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
go mod tidymust be run — direct dependencies do not match imports, and manifest is out of order.The repository still imports
ptermin v2/pkg/commands/build/base.go and v2/pkg/commands/build/build.go, yet the PR description claims this dependency was replaced. Either the claimed migration was not completed, or the description is inaccurate.Additionally,
labstack/gommonremains in go.mod (line 23) but has no corresponding imports in v2/, indicating the file is out of sync with actual usage.The out-of-order placement of
github.com/kungfusheep/glyph v0.6.0(line 41, which should appear alphabetically aftergithub.com/goreleaser/goreleaser) is a clear symptom thatgo mod tidywas not run after editing the manifest.Run
go mod tidyto resolve the manifest, then clarify whether the PR description accurately reflects the intended dependency changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/go.mod` at line 23, The go.mod is out of sync with imports (unused labstack/gommon entry, pterm still imported in v2/pkg/commands/build/base.go and v2/pkg/commands/build/build.go) and entries are not ordered; run "go mod tidy" in the v2 module to regenerate the manifest and sort dependencies, verify and remove any unused entries (e.g., github.com/labstack/gommon) and ensure pterm was actually removed or update the PR description if pterm is still used; after tidying, run `go list -m all` or `grep` those packages in v2/ to confirm the dependency graph matches the code.v2/cmd/wails/dev.go (1)
12-33:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winQuiet mode may leak output via
logutilscalls that bypass the muted logger.Previously,
pterm.DisableOutput()silenced all output globally. Now--quietonly mutes thecliloggerand skips the banner. Code indev.Applicationthat useslogutils.LogRed(),logutils.LogDarkYellow(), etc., callsprintln(tui.Red(...))directly, which bypasses the muted logger and respects only theColourEnabledflag (controlled by--nocolor, not--quiet). If--quietis expected to fully silence dev server bootstrap output, these call sites should either route through the (muted)loggeror check a separate quiet flag intui/logutils.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/cmd/wails/dev.go` around lines 12 - 33, Quiet mode currently only mutes clilogger in devApplication but dev.Application still emits output via logutils/tui (e.g., logutils.LogRed(), logutils.LogDarkYellow(), println(tui.Red(...))) which bypasses the muted logger; update the bootstrap so dev.Application's startup messages respect --quiet by either routing those messages through the created logger (use logger.Info/Error/Warn instead of direct logutils/tui printlns) or by adding and checking a quiet flag in tui/logutils (e.g., tui.IsQuiet or logutils.Muted) before printing; change calls in dev.Application (and any helper that calls tui.Red/println) to use the logger or check the new quiet flag so --quiet fully silences output.v2/cmd/wails/doctor.go (1)
197-210:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
externalPackagesis collected but unused — either render or remove the collection.
externalPackagesis populated at line 198 for dependencies markedExternal(packages the system package manager cannot install, such as npm or NSIS), but the slice is never used and is explicitly discarded at line 210 with_ = externalPackages. This creates dead code that accumulates unused data.Either render an
External Packagessection in the Dependencies output (similar to how optional dependencies are flagged with*), or remove the collection logic entirely if external packages are not meant to be surfaced to the user.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/cmd/wails/doctor.go` around lines 197 - 210, externalPackages is populated when dependency.External is true but never used; either drop the collection or surface it to the user. To fix, either remove the externalPackages slice and the block that appends to it (and delete the `_ = externalPackages` line) to eliminate dead code, or render an "External Packages" section after tui.HeaderTable by checking len(externalPackages)>0 and printing a brief header and each external package's name and version (use the same dependency fields as used for dependenciesTableData) so the information collected by externalPackages is displayed to users.
🧹 Nitpick comments (4)
v2/cmd/wails/generate.go (1)
26-29: ⚡ Quick winDead
loggersetup left over from the migration — drop it (and the_ = loggerdiscard).In
generateModule(Line 26-28),loggeris constructed and muted but never written to. IngenerateTemplate(Line 67-69), the same pattern appears, followed by an explicit_ = loggerto silence the compiler — a strong tell that the migration removed the only call sites and the setup wasn't cleaned up. Either remove the dead code or actually route progress messages through the (muted) logger so--quietcontinues to suppress them.🧹 Suggested cleanup
func generateModule(f *flags.GenerateModule) error { if f.NoColour { tui.SetNoColour() } - quiet := f.Verbosity == flags.Quiet - logger := clilogger.New(os.Stdout) - logger.Mute(quiet) - buildTags, err := buildtags.Parse(f.Tags)func generateTemplate(f *flags.GenerateTemplate) error { if f.NoColour { tui.SetNoColour() } - quiet := f.Quiet - logger := clilogger.New(os.Stdout) - logger.Mute(quiet) - _ = logger - if f.Name == "" {If you'd rather respect
--quiethere, keep the logger and replace thetui.Section/tui.BulletPoint/tui.Infocalls withlogger.Println(...)so that mute takes effect.Also applies to: 66-69
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/cmd/wails/generate.go` around lines 26 - 29, The logger created in generateModule and generateTemplate (logger := clilogger.New(os.Stdout); logger.Mute(quiet)) is dead code: remove the unused logger variable and the `_ = logger` discard if you don't intend to route output through it, or alternatively replace the tui.Section / tui.BulletPoint / tui.Info calls in those functions with logger.Println/logger.Printf so --quiet (f.Verbosity == flags.Quiet) actually mutes progress output; update generateModule and generateTemplate accordingly and delete the unused logger setup to eliminate the compiler discard.v2/internal/tui/tui.go (1)
108-177: ⚡ Quick winColumn-width math uses byte length; will misalign with ANSI / wide runes.
Both
Table(Line 114, 127, 131) andHeaderTable(Line 146, 157) measure width withlen(cell). Two failure modes:
- Callers passing colored cells (
tui.Green("OK")etc.) will have ANSI escapes counted in the width, producing visibly skewed columns.- East-Asian / emoji / combining characters render at a different terminal width than their byte length.
go-runewidthis already an indirect dep (v0.0.19), so this is a cheap fix if/when the alignment becomes user-visible. Today, all in-repo callers appear to pass plain ASCII strings, so this is preventive rather than user-visible — flagging only because the helpers are now public API for the whole CLI.🔧 Sketch
import "github.com/mattn/go-runewidth" // helper: visible width, ignoring ANSI SGR escapes. func visualWidth(s string) int { // strip CSI sequences first if cells may contain ANSI... return runewidth.StringWidth(stripansi.Strip(s)) }Then replace
len(cell)/len(key)withvisualWidth(...)and adjust thestrings.Repeat(" ", …)calculations accordingly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/internal/tui/tui.go` around lines 108 - 177, Replace byte-length measurements with a visual-width helper and strip ANSI before measuring: add imports "github.com/mattn/go-runewidth" and "github.com/acarl005/stripansi" and implement visualWidth(s string) int { return runewidth.StringWidth(stripansi.Strip(s)) }. In Table, replace len(key)/len(row[0]) and len(key) uses with visualWidth(key) and use strings.Repeat(" ", maxKey-visualWidth(key)) when padding (don't rely on fmt's %-*s). In HeaderTable, replace len(cell) and widths computation with visualWidth(cell), compute padded := cell + strings.Repeat(" ", w-visualWidth(cell)), and build the separator using runewidth-derived widths. Update all occurrences in functions Table and HeaderTable to use visualWidth so ANSI escapes and wide runes are handled correctly.v2/internal/colour/colour.go (2)
13-32: ⚡ Quick win
ColourEnabledis no longer honored by the per-color helpers, only byRainbow.The compatibility shim documents
ColourEnabledas the legacy toggle, butYellow/Red/Green/.../DarkXall unconditionally delegate totui, so callers that still docolour.ColourEnabled = false(without also touchingtui) will keep getting ANSI sequences. OnlyRainbow(lines 38-48) checks the flag, which is an inconsistent contract for a "backward-compatible wrapper."Either gate every helper on
ColourEnabled, or makeColourEnableda write-through that callstui.SetNoColour()so the legacy and new toggles stay in sync.♻️ Option A: gate every helper on the local flag
-func Yellow(text string) string { return tui.Yellow(text) } -func Red(text string) string { return tui.Red(text) } -func Blue(text string) string { return tui.Blue(text) } -func Green(text string) string { return tui.Green(text) } -func Cyan(text string) string { return tui.Cyan(text) } -func Magenta(text string) string { return tui.Magenta(text) } -func White(text string) string { return tui.White(text) } -func Black(text string) string { return tui.Black(text) } +func apply(fn func(string) string, text string) string { + if !ColourEnabled { + return text + } + return fn(text) +} + +func Yellow(text string) string { return apply(tui.Yellow, text) } +func Red(text string) string { return apply(tui.Red, text) } +func Blue(text string) string { return apply(tui.Blue, text) } +func Green(text string) string { return apply(tui.Green, text) } +func Cyan(text string) string { return apply(tui.Cyan, text) } +func Magenta(text string) string { return apply(tui.Magenta, text) } +func White(text string) string { return apply(tui.White, text) } +func Black(text string) string { return apply(tui.Black, text) }(apply the same pattern to the
Dark*helpers)♻️ Option B: make `ColourEnabled` a setter that mirrors `tui`
Replace the
var ColourEnabled = truewith a getter/setter pair (or a smallSetColourEnabled(bool)helper) that callstui.SetNoColour()so legacy callers that toggle the package-level flag stay in sync withtui.ColourEnabled.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/internal/colour/colour.go` around lines 13 - 32, The per-color helpers (Yellow, Red, Green, Blue, Cyan, Magenta, White, Black and the Dark* variants like DarkGreen) currently ignore the legacy ColourEnabled flag while Rainbow checks it; either make every helper respect ColourEnabled by gating their return on that flag, or convert the package-level var into a write-through setter (e.g., replace var ColourEnabled with a SetColourEnabled(bool) or a getter/setter pair) that calls tui.SetNoColour(!enabled) so toggling ColourEnabled stays in sync with the tui backend; update all helper functions referenced above (and Rainbow if needed) to use the chosen approach.
15-15: ⚡ Quick win
Colis now a no-op that returns text unmodified; clarify intent and consistency.The current implementation of
Colsimply returns text without any color transformation, differing from other color functions. Since this is an internal package with no callers found in the codebase, the breaking change concern does not have practical impact.However, the inconsistency with
ColourEnabledremains: all color functions in this package (includingCol) delegate to thetuipackage unconditionally and ignore the localColourEnabledvariable set for backward compatibility. IfColis intended to be a retained public function, either implement it with color support that respectsColourEnabled, or clearly document that it is a deprecated no-op.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/internal/colour/colour.go` at line 15, The Col function is currently a no-op and inconsistent with other color helpers and the ColourEnabled flag; update Col to behave like the other color functions by delegating to the tui color helper while observing the local ColourEnabled boolean, or explicitly mark it deprecated in its comment/signature. Concretely, change the Col implementation to check ColourEnabled and return text unchanged when false, otherwise call the equivalent tui color function (same pattern as other helpers that reference tui) and add/update a short docstring stating its deprecation or intended behavior; ensure you reference the Col function and the ColourEnabled variable so the change mirrors existing helper implementations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@v2/cmd/wails/build.go`:
- Around line 223-248: The spinner is racing with build output because
tui.WithSpinner redraws while build.Build writes to buildOptions.Logger
(os.Stdout) — fix by not running the spinner when build output is unmuted: in
the DryRun/real build block, check verbosity (or whether buildOptions.Logger is
os.Stdout) and if not Quiet, replace the WithSpinner call with a simple status
emit (e.g., tui.Section or tui.Info stating "Compiling PLATFORM/ARCH...") and
then call build.Build directly; alternatively, add a boolean flag to
tui.WithSpinner (e.g., pauseRedraw bool) and set it to true when
buildOptions.Logger is not Quiet so the spinner stops redrawing while the
closure runs; update references in this block: WithSpinner, build.Build,
buildOptions.Logger, and the status emit used.
In `@v2/cmd/wails/init.go`:
- Around line 131-139: The inline spinner races with direct subprocess
stdout/stderr; instead of assigning cmd.Stdout/cmd.Stderr to os.Stdout/os.Stderr
inside the tui.WithSpinner closure (where you call
exec.Command("go","mod","tidy") with cmd.Dir = options.TargetDir and using the
quiet flag), capture both outputs into bytes.Buffer(s) (e.g., var out, errBuf
bytes.Buffer), set cmd.Stdout/ cmd.Stderr to those buffers, run cmd.Run(), then
after WithSpinner returns write the buffered output to os.Stdout/os.Stderr
(respecting the quiet flag) so the spinner animation won't interleave with
command output.
In `@v2/internal/tui/tui.go`:
- Around line 193-253: WithSpinner has races on frame and runErr and lacks
panic-safe cleanup; protect accesses to frame and runErr with the existing mu
(lock around frame++ and any reads glyph may perform, and lock around assigning
runErr and reading it after RunNonInteractive), and make the worker goroutine
panic-safe by deferring a cleanup that sets done=true under mu and calls
app.Stop() (and re-panics if recover() returns a panic). Update the spinner
ticker goroutine to lock mu when incrementing frame and when reading done, and
update the worker goroutine that calls fn() to assign runErr while holding mu
(or lock around assignment) and use defer to guarantee done/app.Stop/recover
behavior.
---
Outside diff comments:
In `@v2/cmd/wails/dev.go`:
- Around line 12-33: Quiet mode currently only mutes clilogger in devApplication
but dev.Application still emits output via logutils/tui (e.g.,
logutils.LogRed(), logutils.LogDarkYellow(), println(tui.Red(...))) which
bypasses the muted logger; update the bootstrap so dev.Application's startup
messages respect --quiet by either routing those messages through the created
logger (use logger.Info/Error/Warn instead of direct logutils/tui printlns) or
by adding and checking a quiet flag in tui/logutils (e.g., tui.IsQuiet or
logutils.Muted) before printing; change calls in dev.Application (and any helper
that calls tui.Red/println) to use the logger or check the new quiet flag so
--quiet fully silences output.
In `@v2/cmd/wails/doctor.go`:
- Around line 197-210: externalPackages is populated when dependency.External is
true but never used; either drop the collection or surface it to the user. To
fix, either remove the externalPackages slice and the block that appends to it
(and delete the `_ = externalPackages` line) to eliminate dead code, or render
an "External Packages" section after tui.HeaderTable by checking
len(externalPackages)>0 and printing a brief header and each external package's
name and version (use the same dependency fields as used for
dependenciesTableData) so the information collected by externalPackages is
displayed to users.
In `@v2/go.mod`:
- Line 23: The go.mod is out of sync with imports (unused labstack/gommon entry,
pterm still imported in v2/pkg/commands/build/base.go and
v2/pkg/commands/build/build.go) and entries are not ordered; run "go mod tidy"
in the v2 module to regenerate the manifest and sort dependencies, verify and
remove any unused entries (e.g., github.com/labstack/gommon) and ensure pterm
was actually removed or update the PR description if pterm is still used; after
tidying, run `go list -m all` or `grep` those packages in v2/ to confirm the
dependency graph matches the code.
---
Nitpick comments:
In `@v2/cmd/wails/generate.go`:
- Around line 26-29: The logger created in generateModule and generateTemplate
(logger := clilogger.New(os.Stdout); logger.Mute(quiet)) is dead code: remove
the unused logger variable and the `_ = logger` discard if you don't intend to
route output through it, or alternatively replace the tui.Section /
tui.BulletPoint / tui.Info calls in those functions with
logger.Println/logger.Printf so --quiet (f.Verbosity == flags.Quiet) actually
mutes progress output; update generateModule and generateTemplate accordingly
and delete the unused logger setup to eliminate the compiler discard.
In `@v2/internal/colour/colour.go`:
- Around line 13-32: The per-color helpers (Yellow, Red, Green, Blue, Cyan,
Magenta, White, Black and the Dark* variants like DarkGreen) currently ignore
the legacy ColourEnabled flag while Rainbow checks it; either make every helper
respect ColourEnabled by gating their return on that flag, or convert the
package-level var into a write-through setter (e.g., replace var ColourEnabled
with a SetColourEnabled(bool) or a getter/setter pair) that calls
tui.SetNoColour(!enabled) so toggling ColourEnabled stays in sync with the tui
backend; update all helper functions referenced above (and Rainbow if needed) to
use the chosen approach.
- Line 15: The Col function is currently a no-op and inconsistent with other
color helpers and the ColourEnabled flag; update Col to behave like the other
color functions by delegating to the tui color helper while observing the local
ColourEnabled boolean, or explicitly mark it deprecated in its
comment/signature. Concretely, change the Col implementation to check
ColourEnabled and return text unchanged when false, otherwise call the
equivalent tui color function (same pattern as other helpers that reference tui)
and add/update a short docstring stating its deprecation or intended behavior;
ensure you reference the Col function and the ColourEnabled variable so the
change mirrors existing helper implementations.
In `@v2/internal/tui/tui.go`:
- Around line 108-177: Replace byte-length measurements with a visual-width
helper and strip ANSI before measuring: add imports
"github.com/mattn/go-runewidth" and "github.com/acarl005/stripansi" and
implement visualWidth(s string) int { return
runewidth.StringWidth(stripansi.Strip(s)) }. In Table, replace
len(key)/len(row[0]) and len(key) uses with visualWidth(key) and use
strings.Repeat(" ", maxKey-visualWidth(key)) when padding (don't rely on fmt's
%-*s). In HeaderTable, replace len(cell) and widths computation with
visualWidth(cell), compute padded := cell + strings.Repeat(" ",
w-visualWidth(cell)), and build the separator using runewidth-derived widths.
Update all occurrences in functions Table and HeaderTable to use visualWidth so
ANSI escapes and wide runes are handled correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 07962f33-02ee-4bf3-8be7-409e05d54277
⛔ Files ignored due to path filters (1)
v2/go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
v2/cmd/wails/build.gov2/cmd/wails/dev.gov2/cmd/wails/doctor.gov2/cmd/wails/generate.gov2/cmd/wails/init.gov2/cmd/wails/internal/logutils/color-logs.gov2/cmd/wails/main.gov2/cmd/wails/show.gov2/cmd/wails/update.gov2/go.modv2/internal/colour/colour.gov2/internal/tui/tui.gov2/pkg/clilogger/clilogger.go
| if !f.DryRun { | ||
| // Start Time | ||
| start := time.Now() | ||
|
|
||
| compiledBinary, err := build.Build(buildOptions) | ||
| var compiledBinary string | ||
| err := tui.WithSpinner( | ||
| fmt.Sprintf("Compiling %s/%s", buildOptions.Platform, buildOptions.Arch), | ||
| func() error { | ||
| var buildErr error | ||
| compiledBinary, buildErr = build.Build(buildOptions) | ||
| return buildErr | ||
| }, | ||
| ) | ||
| if err != nil { | ||
| pterm.Error.Println(err.Error()) | ||
| tui.Error(err.Error()) | ||
| targetErr = err | ||
| return | ||
| } | ||
|
|
||
| buildOptions.IgnoreFrontend = true | ||
| buildOptions.CleanBinDirectory = false | ||
|
|
||
| // Output stats | ||
| buildOptions.Logger.Println("%s", fmt.Sprintf("Built '%s' in %s.\n", compiledBinary, time.Since(start).Round(time.Millisecond).String())) | ||
|
|
||
| outputBinaries[buildOptions.Platform+"/"+buildOptions.Arch] = compiledBinary | ||
| } else { | ||
| pterm.Info.Println("Dry run: skipped build.") | ||
| tui.Info("Dry run: skipped build.") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f -e go . v2/internal/tui | xargs -r rg -nP -C5 '\bWithSpinner\b|NewInlineApp|\bRun\(|Pause|Suspend|Stop'Repository: wailsapp/wails
Length of output: 1092
🏁 Script executed:
#!/bin/bash
# Find and read the complete WithSpinner function
fd -t f -e go . v2/internal/tui | head -5 | while read f; do
if rg -q "func WithSpinner" "$f"; then
echo "=== Found in: $f ==="
# Get line number and read surrounding context
line=$(rg -n "func WithSpinner" "$f" | cut -d: -f1)
start=$((line - 5))
[ $start -lt 1 ] && start=1
end=$((line + 80))
sed -n "${start},${end}p" "$f"
fi
doneRepository: wailsapp/wails
Length of output: 2113
🏁 Script executed:
#!/bin/bash
# Look for glyph imports and understand what RunNonInteractive does
rg -A 20 "RunNonInteractive" v2/internal/tui/tui.go | head -30Repository: wailsapp/wails
Length of output: 703
Spinner redraws race with build.Build's logger output on non-quiet verbosity.
The spinner animation loop in WithSpinner continuously redraws every 80ms via app.RequestRender() while fn() executes concurrently. Since buildOptions.Logger (initialized with os.Stdout) is unmuted when verbosity is not Quiet, the build pipeline's output and the spinner's terminal state management write to os.Stdout without synchronization. This causes visible artifacts: torn spinner frames, duplicated output, and misaligned text interleaved in interactive terminals.
Two reasonable options:
- Only show the spinner when
quietis true (the only case where stdout is silent during the closure), and otherwise print atui.Sectionor status line. - Have
tui.WithSpinnerpause spinner redraws while the closure runs, relying on the build's own progress output.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@v2/cmd/wails/build.go` around lines 223 - 248, The spinner is racing with
build output because tui.WithSpinner redraws while build.Build writes to
buildOptions.Logger (os.Stdout) — fix by not running the spinner when build
output is unmuted: in the DryRun/real build block, check verbosity (or whether
buildOptions.Logger is os.Stdout) and if not Quiet, replace the WithSpinner call
with a simple status emit (e.g., tui.Section or tui.Info stating "Compiling
PLATFORM/ARCH...") and then call build.Build directly; alternatively, add a
boolean flag to tui.WithSpinner (e.g., pauseRedraw bool) and set it to true when
buildOptions.Logger is not Quiet so the spinner stops redrawing while the
closure runs; update references in this block: WithSpinner, build.Build,
buildOptions.Logger, and the status emit used.
| err = tui.WithSpinner("Running go mod tidy", func() error { | ||
| cmd := exec.Command("go", "mod", "tidy") | ||
| cmd.Dir = options.TargetDir | ||
| cmd.Stderr = os.Stderr | ||
| if !quiet { | ||
| cmd.Stdout = os.Stdout | ||
| } | ||
| return cmd.Run() | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'spinner|tui' v2/internal/tui | xargs -r rg -n -C5 'WithSpinner|InlineApp|os\.Stdout|Pause|Suspend' 2>/dev/null
ast-grep --pattern $'func WithSpinner($_, $_) $_ {
$$$
}'Repository: wailsapp/wails
Length of output: 3807
🏁 Script executed:
# Check how WithSpinner is used elsewhere and whether output is typically expected
rg -n 'WithSpinner' v2/cmd/wails/ -A 5 -B 2Repository: wailsapp/wails
Length of output: 2860
🏁 Script executed:
# Look for any stderr/stdout handling patterns in the tui module
fd -t f . v2/internal/tui | xargs rg -n 'Stdout|Stderr|Run|Command' -B 2 -A 2Repository: wailsapp/wails
Length of output: 509
🏁 Script executed:
# Check if there are any comments or issues about spinner output corruption
rg -n 'spinner|output|corrupt|interleave' v2/ -i --max-count=20Repository: wailsapp/wails
Length of output: 29095
🏁 Script executed:
# Check shell.RunCommand implementation
fd -t f 'shell.go' v2/ | xargs rg -n 'func RunCommand' -A 15Repository: wailsapp/wails
Length of output: 1445
🏁 Script executed:
# Search for go mod tidy behavior and whether output is expected
rg -n 'go mod tidy' v2/ -B 3 -A 3Repository: wailsapp/wails
Length of output: 1549
🏁 Script executed:
# Check if there are TODOs or comments about spinner and output
rg -n 'TODO|FIXME|BUG|XXX' v2/internal/tui/tui.goRepository: wailsapp/wails
Length of output: 40
🏁 Script executed:
# Get full context of update.go WithSpinner usage
sed -n '120,135p' v2/cmd/wails/update.goRepository: wailsapp/wails
Length of output: 497
🏁 Script executed:
# Also check the full context of init.go around lines 131-139
sed -n '128,145p' v2/cmd/wails/init.goRepository: wailsapp/wails
Length of output: 450
Buffer go mod tidy output to prevent TTY corruption during inline spinner animation.
The closure attaches cmd.Stdout = os.Stdout and cmd.Stderr = os.Stderr while WithSpinner actively animates using glyph.NewInlineApp() (inline rendering with carriage returns). The concurrent spinner redraw goroutine and the subprocess output will race for TTY writes, interleaving partial spinner glyphs with command output.
The cmd.Stderr is unconditionally attached (line 134), so even quiet mode is affected when go mod tidy downloads modules. Note that update.go (line 122) uses shell.RunCommand() to capture output into buffers and displays it after the spinner stops—this is the correct pattern for use with WithSpinner.
Recommended fix: capture output into bytes.Buffer inside the closure and write after the spinner completes.
🛠️ Suggested fix: buffer stdio, flush after spinner
if !f.CIMode {
- err = tui.WithSpinner("Running go mod tidy", func() error {
- cmd := exec.Command("go", "mod", "tidy")
- cmd.Dir = options.TargetDir
- cmd.Stderr = os.Stderr
- if !quiet {
- cmd.Stdout = os.Stdout
- }
- return cmd.Run()
- })
+ var tidyOut, tidyErr bytes.Buffer
+ err = tui.WithSpinner("Running go mod tidy", func() error {
+ cmd := exec.Command("go", "mod", "tidy")
+ cmd.Dir = options.TargetDir
+ cmd.Stderr = &tidyErr
+ cmd.Stdout = &tidyOut
+ return cmd.Run()
+ })
+ if !quiet && tidyOut.Len() > 0 {
+ os.Stdout.Write(tidyOut.Bytes())
+ }
+ if tidyErr.Len() > 0 {
+ os.Stderr.Write(tidyErr.Bytes())
+ }
if err != nil {
return err
}(remember to add the bytes import)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@v2/cmd/wails/init.go` around lines 131 - 139, The inline spinner races with
direct subprocess stdout/stderr; instead of assigning cmd.Stdout/cmd.Stderr to
os.Stdout/os.Stderr inside the tui.WithSpinner closure (where you call
exec.Command("go","mod","tidy") with cmd.Dir = options.TargetDir and using the
quiet flag), capture both outputs into bytes.Buffer(s) (e.g., var out, errBuf
bytes.Buffer), set cmd.Stdout/ cmd.Stderr to those buffers, run cmd.Run(), then
after WithSpinner returns write the buffered output to os.Stdout/os.Stderr
(respecting the quiet flag) so the spinner animation won't interleave with
command output.
| func WithSpinner(message string, fn func() error) error { | ||
| if !ColourEnabled { | ||
| fmt.Println(message + "...") | ||
| err := fn() | ||
| if err != nil { | ||
| fmt.Println(" Failed: " + err.Error()) | ||
| } else { | ||
| fmt.Println(" Done.") | ||
| } | ||
| return err | ||
| } | ||
|
|
||
| var ( | ||
| done bool | ||
| runErr error | ||
| mu sync.Mutex | ||
| frame int | ||
| ) | ||
|
|
||
| displayMsg := message | ||
| app := glyph.NewInlineApp() | ||
| app.Height(1) | ||
| app.SetView( | ||
| glyph.HBox( | ||
| glyph.Spinner(&frame).Frames(glyph.SpinnerBraille).FG(glyph.Cyan), | ||
| glyph.SpaceW(1), | ||
| glyph.Text(&displayMsg), | ||
| ), | ||
| ) | ||
|
|
||
| go func() { | ||
| for { | ||
| mu.Lock() | ||
| d := done | ||
| mu.Unlock() | ||
| if d { | ||
| return | ||
| } | ||
| frame++ | ||
| app.RequestRender() | ||
| time.Sleep(80 * time.Millisecond) | ||
| } | ||
| }() | ||
|
|
||
| go func() { | ||
| runErr = fn() | ||
| mu.Lock() | ||
| done = true | ||
| mu.Unlock() | ||
| app.Stop() | ||
| }() | ||
|
|
||
| _ = app.RunNonInteractive() | ||
|
|
||
| if runErr != nil { | ||
| fmt.Println(ansi("91", " ✗") + " " + message) | ||
| } else { | ||
| fmt.Println(ansi("92", " ✓") + " " + message) | ||
| } | ||
| return runErr | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does github.com/kungfusheep/glyph v0.6.0 support passing *atomic.Int32 (or similar) to Spinner(...), or does it strictly require *int? Is there a recommended pattern for driving a glyph Spinner's frame from a separate goroutine without races?
💡 Result:
No, github.com/kungfusheep/glyph v0.6.0 does not explicitly document support for passing *atomic.Int32 (or similar atomic types) directly to Spinner(...). The Spinner component expects a frame pointer of type *int, as indicated by the opSpinner struct in the template compiler which defines framePtr *int [1]. Glyph's general binding pattern is to pass pointers to data (*int, *float64, etc.), which glyph reads atomically during render passes [2]. Atomic types like *atomic.Int32 are not directly compatible since they don't expose a plain int pointer; their Load method must be called to get the value.
Citations:
- 1: https://github.com/kungfusheep/glyph/blob/master/template.go
- 2: https://github.com/themastersheep/glyph
🏁 Script executed:
# First, let's locate and examine the actual tui.go file
find . -name "tui.go" -type f | head -20Repository: wailsapp/wails
Length of output: 83
🏁 Script executed:
# Read the WithSpinner function from the file to confirm it matches the snippet
cat -n v2/internal/tui/tui.go | sed -n '193,253p'Repository: wailsapp/wails
Length of output: 1458
🏁 Script executed:
# Check the imports at the top of the file to understand what's already imported
head -30 v2/internal/tui/tui.goRepository: wailsapp/wails
Length of output: 822
WithSpinner has three critical concurrency bugs.
-
Data race on
frame(lines 217, 231). The glyph library reads the pointer-passedframevalue asynchronously during every render call. The ticker goroutine incrementsframe++(line 231) without holding the mutex, creating a race between the goroutine's write and glyph's reads. -
Data race on
runErr(lines 238, 247).runErr = fn()is written in the worker goroutine without synchronization, and the main goroutine reads it afterRunNonInteractive()returns without acquiring the mutex. The mutex protects onlydone; there is no happens-before guarantee forrunErr. -
No panic safety (line 238). If
fn()panics, the goroutine crashes immediately without executingdone = trueorapp.Stop(). The spinner is left in an inconsistent state with no recovery path. Usedeferto ensure cleanup runs even on panic.
All three will be detected by go test -race.
Fix approach: Extend the mutex to protect frame reads and writes, and wrap the worker goroutine with defer to guarantee cleanup:
func WithSpinner(message string, fn func() error) error {
// ... [no change to the non-TTY branch] ...
var (
done bool
runErr error
mu sync.Mutex
frame int
)
// ... [setup code unchanged] ...
go func() {
for {
mu.Lock()
d := done
mu.Unlock()
if d {
return
}
mu.Lock()
frame++
mu.Unlock()
app.RequestRender()
time.Sleep(80 * time.Millisecond)
}
}()
go func() {
defer func() {
mu.Lock()
done = true
mu.Unlock()
app.Stop()
if r := recover(); r != nil {
panic(r)
}
}()
runErr = fn()
}()
// ... [rest unchanged] ...
}(Note: The suggested fix in the original review uses atomic.Int32, but glyph.Spinner requires *int and does not support atomic types, so a mutex-based approach is necessary.)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@v2/internal/tui/tui.go` around lines 193 - 253, WithSpinner has races on
frame and runErr and lacks panic-safe cleanup; protect accesses to frame and
runErr with the existing mu (lock around frame++ and any reads glyph may
perform, and lock around assigning runErr and reading it after
RunNonInteractive), and make the worker goroutine panic-safe by deferring a
cleanup that sets done=true under mu and calls app.Stop() (and re-panics if
recover() returns a panic). Update the spinner ticker goroutine to lock mu when
incrementing frame and when reading done, and update the worker goroutine that
calls fn() to assign runErr while holding mu (or lock around assignment) and use
defer to guarantee done/app.Stop/recover behavior.
1 participant
Closes #3374
Summary
Replaces the Wails v2 CLI's five overlapping TUI libraries — pterm, wzshiming/ctc, labstack/gommon/color, gookit/color (indirect) — with a single coherent system:
v2/internal/tuipackage: direct ANSI escape codes for styled text + glyph'sNewInlineApp()for animated spinners in interactive terminals. Auto-detects non-TTY (CI, pipes) viaos.Stdout.Stat()and falls back to plain text. RespectsNO_COLOR,TERM=dumb, and the existing-nocolorflag.build,update,init(template install +go mod tidy), andgeneratecommands viatui.WithSpinner().tui.Green(),tui.Red(),tui.Section(),tui.Table(), etc. — replacing the scattered per-command colour calls.v2/internal/coloursimplified: now delegates totuifor full backward compatibility with callers outsidecmd/wails.Testing
go build ./cmd/wails— exits 0 on Linux (Ubuntu 24.04, Go 1.23.4 / 1.26.2)go vet ./cmd/wails— exits 0CC @leaanthony
Summary by CodeRabbit
Refactor
Chores