Repository navigation
feat(v3/cli): replace pterm with glyph + direct ANSI — full TUI redesign - #5411
taliesin-ai wants to merge 2 commits into
Conversation
Remove pterm from v3 entirely and replace with a lean, bespoke terminal UI layer in v3/internal/term: - Header: ◆ wails v3.x.x ─ Command Name - Sections: ── Section Title - Messages: ✓ success ✗ error ⚠ warning ↳ info - Tables: dim-keyed aligned two-column; HeaderTable with bold+separator - Spinner: glyph braille spinner (glyph.SpinnerBraille, cyan) on TTY; plain info line on CI/pipe - Confirm: simple y/N prompt without pterm dependency - Hyperlinks: OSC 8 terminal hyperlinks - Inline colour helpers: Cyan/Green/Red/Yellow/Blue/Magenta/Bold/Dim All callers updated (commands/, doctor/, templates/, generator/). config.Logger now routes through spinnerLogger in term instead of PtermLogger; DefaultPtermLogger and PtermLogger removed from config. Note: go.sum omitted from this patch; run `go mod tidy` in v3/ after applying to regenerate checksums for glyph and its transitive deps.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR removes the external pterm dependency and replaces it with an internal term package (ANSI + glyph spinners). The new term API provides formatted output, table renderers, prompts, and a glyph-based spinner; command files, doctor, templates, tests, and go.mod are updated to use term. ChangesMigrate from pterm to internal term library
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 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: 2
🧹 Nitpick comments (2)
v3/internal/term/term.go (1)
390-393: 💤 Low valueHyperlink embeds ANSI codes regardless of color settings.
The
Hyperlinkfunction directly embedsansiUnderandansiResetin the OSC 8 sequence. WhencolourEnabledis false, the hyperlink will still contain underline/reset codes, which may produce unexpected output in environments where ANSI codes should be stripped.Consider whether the underline styling should respect the
colourEnabledflag, similar to thecol()helper.♻️ Proposed fix to respect colourEnabled
func Hyperlink(url, text string) string { + if !colourEnabled { + return text + } return fmt.Sprintf("\x1b]8;;%s\x1b\\%s%s%s\x1b]8;;\x1b\\", url, ansiUnder, text, ansiReset) }🤖 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 `@v3/internal/term/term.go` around lines 390 - 393, The Hyperlink function always injects ansiUnder and ansiReset into the OSC 8 sequence even when colour output is disabled; change Hyperlink to respect the existing colourEnabled behavior (like the col() helper) by conditionally using the underline/reset sequences only when colourEnabled is true (or by calling col(ansiUnder) / col(ansiReset) or equivalent), so that when colourEnabled is false the hyperlink contains no ANSI underline/reset codes; update the function that constructs the OSC 8 string (Hyperlink) to reference ansiUnder, ansiReset and colourEnabled accordingly.v3/internal/doctor/doctor.go (1)
36-42: ⚡ Quick winConsider using
spinner.Success()for consistency.The current pattern uses
term.StopSpinner(spinner)both in the defer (line 40) and after successful completion (line 90). While functional, this differs from the pattern inupdate_cli.gowhich usesspinner.Success(message)andspinner.Fail(message)to clearly indicate the outcome.Using
spinner.Success("System scan complete")on line 90 would provide better user feedback and align with the spinner lifecycle pattern used elsewhere in the codebase.♻️ Refactor to use spinner.Success()
spinner := term.StartSpinner("Scanning system - Please wait (this may take a long time)…") defer func() { if err != nil { - term.StopSpinner(spinner) + spinner.Fail("System scan failed") } }() ... - term.StopSpinner(spinner) + spinner.Success("System scan complete")Also applies to: 62-63, 90-90
🤖 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 `@v3/internal/doctor/doctor.go` around lines 36 - 42, Replace the manual term.StopSpinner(spinner) calls with the spinner lifecycle helpers: after starting the spinner via term.StartSpinner (assigned to variable spinner), call spinner.Fail("message") in the error defer path and spinner.Success("System scan complete") on successful completion where term.StopSpinner(spinner) is currently used; update the defer to call spinner.Fail(...) when err != nil and change the success branch (currently calling term.StopSpinner(spinner)) to spinner.Success("System scan complete") so the doctor.go spinner usage matches update_cli.go's pattern.
🤖 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 `@v3/internal/commands/appimage.go`:
- Around line 31-32: The spinner is unconditionally stopped via defer
term.StopSpinner(spinner); instead, update the generateAppImage call site to
capture its error and call spinner.Fail("...") when generateAppImage returns an
error and spinner.Success("...") on success (using the same spinner variable
returned from term.StartSpinner), then stop the spinner appropriately (either
rely on spinner.Success/Fail to finalize or call term.StopSpinner(spinner) after
those calls); remove the unconditional defer and follow the pattern used in
update_cli.go so the final spinner state reflects success or failure.
In `@v3/internal/term/term.go`:
- Around line 21-25: The package-level booleans colourEnabled, outputEnabled,
and debugEnabled are unprotected and cause data races; introduce a sync.RWMutex
(e.g., stateMu) and use stateMu.RLock()/RUnlock() for all reads (col(), the
message functions that check output/debug, spinnerLogger.Debugf()) and
stateMu.Lock()/Unlock() for all writes (the toggle functions that set
colourEnabled/outputEnabled/debugEnabled); update all referenced
functions/locations (col, the various message printing helpers,
spinnerLogger.Debugf, and the toggle functions) to use the mutex to ensure safe
concurrent access.
---
Nitpick comments:
In `@v3/internal/doctor/doctor.go`:
- Around line 36-42: Replace the manual term.StopSpinner(spinner) calls with the
spinner lifecycle helpers: after starting the spinner via term.StartSpinner
(assigned to variable spinner), call spinner.Fail("message") in the error defer
path and spinner.Success("System scan complete") on successful completion where
term.StopSpinner(spinner) is currently used; update the defer to call
spinner.Fail(...) when err != nil and change the success branch (currently
calling term.StopSpinner(spinner)) to spinner.Success("System scan complete") so
the doctor.go spinner usage matches update_cli.go's pattern.
In `@v3/internal/term/term.go`:
- Around line 390-393: The Hyperlink function always injects ansiUnder and
ansiReset into the OSC 8 sequence even when colour output is disabled; change
Hyperlink to respect the existing colourEnabled behavior (like the col() helper)
by conditionally using the underline/reset sequences only when colourEnabled is
true (or by calling col(ansiUnder) / col(ansiReset) or equivalent), so that when
colourEnabled is false the hyperlink contains no ANSI underline/reset codes;
update the function that constructs the OSC 8 string (Hyperlink) to reference
ansiUnder, ansiReset and colourEnabled accordingly.
🪄 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: ee67795e-44bc-48fa-8344-2444cab64592
📒 Files selected for processing (14)
v3/cmd/wails3/main.gov3/go.modv3/internal/commands/appimage.gov3/internal/commands/capabilities.gov3/internal/commands/entitlements_setup.gov3/internal/commands/init.gov3/internal/commands/sign.gov3/internal/commands/signing_setup.gov3/internal/commands/update_cli.gov3/internal/doctor/doctor.gov3/internal/generator/analyse_test.gov3/internal/generator/config/log.gov3/internal/templates/templates.gov3/internal/term/term.go
| spinner := term.StartSpinner("Generating AppImage…") | ||
| defer term.StopSpinner(spinner) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use spinner.Success() and spinner.Fail() for proper status indication.
The defer unconditionally calls term.StopSpinner(spinner) regardless of success or failure. Based on the pattern in update_cli.go (lines 160-165), the spinner should signal its final state:
- Call
spinner.Fail(message)whengenerateAppImagereturns an error - Call
spinner.Success(message)when the operation completes successfully
This provides better UX by showing users whether the AppImage generation succeeded or failed.
♻️ Refactor to use spinner success/fail methods
- spinner := term.StartSpinner("Generating AppImage…")
- defer term.StopSpinner(spinner)
+ spinner := term.StartSpinner("Generating AppImage…")
if options.Binary == "" {
return fmt.Errorf("binary not provided")
}
...
term.Header("AppImage Generator")
- return generateAppImage(options)
+ err = generateAppImage(options)
+ if err != nil {
+ spinner.Fail("AppImage generation failed")
+ return err
+ }
+ spinner.Success("AppImage generated successfully")
+ return nil
}🤖 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 `@v3/internal/commands/appimage.go` around lines 31 - 32, The spinner is
unconditionally stopped via defer term.StopSpinner(spinner); instead, update the
generateAppImage call site to capture its error and call spinner.Fail("...")
when generateAppImage returns an error and spinner.Success("...") on success
(using the same spinner variable returned from term.StartSpinner), then stop the
spinner appropriately (either rely on spinner.Success/Fail to finalize or call
term.StopSpinner(spinner) after those calls); remove the unconditional defer and
follow the pattern used in update_cli.go so the final spinner state reflects
success or failure.
| var ( | ||
| colourEnabled = true | ||
| outputEnabled = true | ||
| debugEnabled = false | ||
| ) |
There was a problem hiding this comment.
Critical: Global state lacks synchronization for concurrent access.
The global variables colourEnabled, outputEnabled, and debugEnabled are accessed and modified without synchronization. These variables are:
- Read by
col()(line 43), all message functions (lines 70+, 98+, 122+), andspinnerLogger.Debugf()(line 379) - Modified by toggle functions (lines 58-62)
If any callers use the term package from multiple goroutines (e.g., concurrent spinners, logger from different contexts), this will cause data races and violate Go's memory model.
🔒 Proposed fix: Add mutex protection
var (
+ stateMu sync.RWMutex
colourEnabled = true
outputEnabled = true
debugEnabled = false
)Then wrap all reads with stateMu.RLock()/RUnlock() and all writes with stateMu.Lock()/Unlock(). For example:
func col(code, text string) string {
+ stateMu.RLock()
+ enabled := colourEnabled
+ stateMu.RUnlock()
- if !colourEnabled {
+ if !enabled {
return text
}
return code + text + ansiReset
}Apply similar patterns to all toggle functions and readers.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var ( | |
| colourEnabled = true | |
| outputEnabled = true | |
| debugEnabled = false | |
| ) | |
| var ( | |
| stateMu sync.RWMutex | |
| colourEnabled = true | |
| outputEnabled = true | |
| debugEnabled = false | |
| ) |
🤖 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 `@v3/internal/term/term.go` around lines 21 - 25, The package-level booleans
colourEnabled, outputEnabled, and debugEnabled are unprotected and cause data
races; introduce a sync.RWMutex (e.g., stateMu) and use
stateMu.RLock()/RUnlock() for all reads (col(), the message functions that check
output/debug, spinnerLogger.Debugf()) and stateMu.Lock()/Unlock() for all writes
(the toggle functions that set colourEnabled/outputEnabled/debugEnabled); update
all referenced functions/locations (col, the various message printing helpers,
spinnerLogger.Debugf, and the toggle functions) to use the mutex to ensure safe
concurrent access.
|
Superseded by #5412 which adds the 2-row glyph spinner with secondary info line routing. Taliesin is an AI agent. CC @leaanthony |
Remove
ptermfrom the v3 CLI entirely and replace with a lean, bespoke terminal UI layer inv3/internal/term.Design language
Spinner uses glyph's braille animation on TTY; degrades to a plain
↳info line on CI/pipes.What changed
v3/internal/term/term.go— complete rewrite:glyphfor animated spinners (SpinnerBraille, cyan)Header,Section,Table,HeaderTable,Confirm,HyperlinkSpinnertype withUpdateText,Success,Fail,Logger()StartSpinner(plain info line)v3/internal/generator/config/log.go— removedPtermLoggerandDefaultPtermLogger;Loggerinterface unchangedAll callers updated:
cmd/wails3/main.go,internal/commands/{appimage,capabilities,entitlements_setup,init,sign,signing_setup,update_cli}.go,internal/doctor/doctor.go,internal/templates/templates.go,internal/generator/analyse_test.gov3/go.mod: addedhub.lumenfield.work/kungfusheep/glyph v0.6.0, removedhub.lumenfield.work/pterm/ptermand its now-unused transitive deps (atomicgo.dev/{cursor,keyboard,schedule},github.com/containerd/console,github.com/gookit/color,github.com/lithammer/fuzzysearch)CC @leaanthony
Summary by CodeRabbit