feat(stloader): add terminal spinner package with shining text effect#189
feat(stloader): add terminal spinner package with shining text effect#189
Conversation
Add new stloader package for displaying loading spinners with optional animated text effects. Features include: - Braille dots spinner animation (⠋⠙⠹⠸⠼⠴⠦⠧⠇⠏) - Optional "shining" text effect with left-to-right color sweep - Configurable base color with 20% lighter highlight - Thread-safe Start/Stop/UpdateText methods - Standard library only (no third-party dependencies) - Uses ANSI 24-bit color escape codes for terminal colors 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary of ChangesHello @AnnatarHe, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new stloader package for terminal spinners, which is a great addition. The implementation is thread-safe and includes good test coverage. My review focuses on a few areas for improvement, including fixing a configuration bug, enhancing performance in the animation loop, and addressing an unused configuration option. Overall, this is a solid contribution.
| // HideCursor defaults to true (we check if explicitly set to false) | ||
| // Since bool zero value is false, we need a different approach | ||
| // For simplicity, we'll always hide cursor by default | ||
| cfg.HideCursor = true |
There was a problem hiding this comment.
The current implementation for HideCursor is buggy because it unconditionally sets the value to true, overriding any user-provided configuration. This prevents users from disabling the cursor-hiding feature.
To correctly handle a boolean option that should default to true, the idiomatic approach in Go is to use a pointer (*bool). This allows you to distinguish between the field being unset (nil) and being explicitly set to false.
Here's how you could implement this fix:
-
Change
LoaderConfig:
Modify theHideCursorfield to be a pointer.// In LoaderConfig struct HideCursor *bool
-
Update
NewLoader:
Set the default value if the field isnil.// In NewLoader function if cfg.HideCursor == nil { v := true cfg.HideCursor = &v }
-
Update usage sites:
Dereference the pointer when accessing the value.// e.g., in Start() and Stop() if *l.config.HideCursor { // ... }
| // BaseColor is the base text color (user-defined) | ||
| BaseColor RGB | ||
| // DarkTheme indicates if the terminal is in dark theme (highlighted char is 20% lighter) | ||
| DarkTheme bool |
There was a problem hiding this comment.
The DarkTheme field is declared in LoaderConfig but is not used anywhere in the package. This could be confusing for users of the API. It should be removed to keep the configuration clean and intentional.
| l.render() | ||
|
|
||
| // Update highlight index for shining effect | ||
| if l.config.EnableShining { | ||
| l.mu.Lock() | ||
| textLen := len([]rune(l.config.Text)) | ||
| if textLen > 0 { | ||
| l.highlightIndex = (l.highlightIndex + 1) % textLen | ||
| } | ||
| l.mu.Unlock() | ||
| } | ||
|
|
||
| // Update spinner symbol at appropriate interval | ||
| spinCounter++ | ||
| if spinCounter >= spinThreshold { | ||
| l.mu.Lock() | ||
| l.symbolIdx = (l.symbolIdx + 1) % len(l.config.Symbols) | ||
| l.mu.Unlock() | ||
| spinCounter = 0 | ||
| } |
There was a problem hiding this comment.
The animate function's main loop acquires and releases the mutex multiple times per tick: once in render(), and then up to twice more for updating highlightIndex and symbolIdx. This can be optimized by using a single lock for all shared state access within a single tick.
To implement this, you can modify render() to no longer acquire the lock itself, and then wrap the call to render() and the state updates in animate within a single lock-unlock block as suggested. This improves both readability and performance by reducing lock contention.
l.mu.Lock()
l.render()
// Update highlight index for shining effect
if l.config.EnableShining {
textLen := len([]rune(l.config.Text))
if textLen > 0 {
l.highlightIndex = (l.highlightIndex + 1) % textLen
}
}
// Update spinner symbol at appropriate interval
spinCounter++
if spinCounter >= spinThreshold {
l.symbolIdx = (l.symbolIdx + 1) % len(l.config.Symbols)
spinCounter = 0
}
l.mu.Unlock()| if i == highlightIdx { | ||
| result.WriteString(colorize(string(r), highlightColor)) | ||
| } else { | ||
| result.WriteString(colorize(string(r), baseColor)) | ||
| } |
There was a problem hiding this comment.
Inside the loop in renderShiningText, string(r) creates a new string from a rune in each iteration. This causes unnecessary memory allocations, which can be inefficient for long text strings. You can optimize this by avoiding the intermediate string conversion and writing the color codes and the character directly to the strings.Builder using fmt.Fprintf.
if i == highlightIdx {
fmt.Fprintf(&result, "\033[38;2;%d;%d;%dm%c", highlightColor.R, highlightColor.G, highlightColor.B, r)
} else {
fmt.Fprintf(&result, "\033[38;2;%d;%d;%dm%c", baseColor.R, baseColor.G, baseColor.B, r)
}Replace all usages of github.com/briandowns/spinner with the new stloader package. All commands now have consistent visual feedback with the shining text effect. Changes: - grep.go: Use stloader with "Searching commands..." text - auth.go: Use stloader with "Waiting for authentication..." text - query.go: Use stloader with "Querying AI..." text - Remove unused briandowns/spinner dependency 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Use *bool for HideCursor to distinguish unset (nil, defaults to true) from explicitly false - Remove unused DarkTheme field from LoaderConfig - Consolidate mutex locks in animate() to single lock-unlock per tick - Use fmt.Fprintf with %c in renderShiningText to avoid string(r) allocations 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 participant
Summary
stloaderpackage for displaying loading spinners with animated text effectsStart(),Stop(), andUpdateText()methodsFeatures
⠋⠙⠹⠸⠼⠴⠦⠧⠇⠏)Usage
Test plan
cmd/loadertestdemo🤖 Generated with Claude Code