feature: Add wvlet.uni.cli package with Chalk, Spinner, and ProgressBar#297
feature: Add wvlet.uni.cli package with Chalk, Spinner, and ProgressBar#297
Conversation
Add CLI utilities inspired by TypeScript's Chalk and ora libraries: - Chalk: Fluent terminal text styling API with support for 16 ANSI colors, 256 colors, RGB/TrueColor, and text modifiers (bold, italic, underline, etc.) - Spinner: Terminal spinner/progress indicator with multiple animation styles and status symbols (success, fail, warn, info) - ProgressBar: Configurable progress bar with preset styles - Terminal: Cursor control and screen manipulation utilities - Cross-platform: JVM, Scala.js (Node.js), and Scala Native support Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary of ChangesHello @xerial, 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 significantly enhances the command-line interface capabilities by introducing a comprehensive set of utilities within the new Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
![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 comprehensive and well-designed wvlet.uni.cli package for building interactive command-line interfaces. The cross-platform support for JVM, Scala.js, and Scala Native is a significant feature. The APIs for Chalk, Spinner, and ProgressBar are fluent and intuitive. The code is generally of high quality with good test coverage.
My review focuses on improving thread-safety in the JVM and Native implementations and enhancing code style and maintainability in a few areas. Specifically, there are potential race conditions in the progress bar implementation that should be addressed. I've also suggested refactoring some methods to use more idiomatic functional patterns in Scala, which will improve readability and robustness.
| */ | ||
| private class JvmRunningProgressBar(config: ProgressBar) extends RunningProgressBar: | ||
| private val currentValue = new AtomicLong(0) | ||
| private var finished: Boolean = false |
There was a problem hiding this comment.
The finished flag is a mutable var and is accessed from multiple threads without synchronization (e.g., render can be called from any thread via update/increment, and finish/fail from another). This can lead to race conditions. To ensure thread safety, please use java.util.concurrent.atomic.AtomicBoolean. You will also need to update its usage from finished to finished.get() for reads and finished.set(true) for writes.
private val finished = new java.util.concurrent.atomic.AtomicBoolean(false)| */ | ||
| private class NativeRunningProgressBar(config: ProgressBar) extends RunningProgressBar: | ||
| private val currentValue = new AtomicLong(0) | ||
| private var finished: Boolean = false |
There was a problem hiding this comment.
The finished flag is a mutable var and is accessed from multiple threads without synchronization. Scala Native supports multi-threading, so this can lead to race conditions. To ensure thread safety, please use java.util.concurrent.atomic.AtomicBoolean. You will also need to update its usage from finished to finished.get() for reads and finished.set(true) for writes.
private val finished = new java.util.concurrent.atomic.AtomicBoolean(false)| override def colorLevel: ColorLevel = | ||
| if isBrowser then | ||
| // Browsers use CSS styling, not ANSI codes | ||
| ColorLevel.None | ||
| else if isNode then | ||
| // Check NO_COLOR | ||
| if env("NO_COLOR").isDefined then | ||
| return ColorLevel.None | ||
|
|
||
| // Check FORCE_COLOR | ||
| env("FORCE_COLOR") match | ||
| case Some("0") => | ||
| return ColorLevel.None | ||
| case Some("1") => | ||
| return ColorLevel.Basic | ||
| case Some("2") => | ||
| return ColorLevel.Ansi256 | ||
| case Some("3") => | ||
| return ColorLevel.TrueColor | ||
| case Some(_) => | ||
| return ColorLevel.Basic | ||
| case None => // continue | ||
|
|
||
| // Check COLORTERM | ||
| env("COLORTERM") match | ||
| case Some("truecolor") | Some("24bit") => | ||
| return ColorLevel.TrueColor | ||
| case _ => // continue | ||
|
|
||
| // Check TERM | ||
| val term = env("TERM").getOrElse("") | ||
| if term == "dumb" then | ||
| ColorLevel.None | ||
| else if term.endsWith("-256color") then | ||
| ColorLevel.Ansi256 | ||
| else if term.nonEmpty then | ||
| ColorLevel.Basic | ||
| else | ||
| ColorLevel.None | ||
| else | ||
| ColorLevel.None | ||
|
|
There was a problem hiding this comment.
The colorLevel method uses multiple return statements, which is not idiomatic in Scala and can make control flow harder to follow. Consider refactoring this method into a single expression, for example by nesting if/else expressions, to improve readability and adhere to functional programming principles.
| private def hideCursor(): Unit = | ||
| if js.typeOf(g.process) != "undefined" then | ||
| g.process.stderr.write("\u001b[?25l") |
There was a problem hiding this comment.
The check js.typeOf(g.process) != "undefined" is repeated across several methods (hideCursor, showCursor, write). To improve code clarity and avoid redundant checks, consider defining a private val isNode = js.typeOf(g.process) != "undefined" at the class level and using it in these methods.
| if currentText.nonEmpty then | ||
| println(currentText) | ||
|
|
||
| override def text: String = currentText | ||
| override def text_=(newText: String): Unit = currentText = newText | ||
|
|
||
| override def succeed(text: String): Unit = | ||
| val finalText = | ||
| if text.nonEmpty then | ||
| text | ||
| else | ||
| currentText | ||
| println(s"${Symbols.successColored} ${finalText}") | ||
| running = false | ||
|
|
||
| override def fail(text: String): Unit = | ||
| val finalText = | ||
| if text.nonEmpty then | ||
| text | ||
| else | ||
| currentText | ||
| println(s"${Symbols.errorColored} ${finalText}") | ||
| running = false | ||
|
|
||
| override def warn(text: String): Unit = | ||
| val finalText = | ||
| if text.nonEmpty then | ||
| text | ||
| else | ||
| currentText | ||
| println(s"${Symbols.warningColored} ${finalText}") | ||
| running = false | ||
|
|
||
| override def info(text: String): Unit = | ||
| val finalText = | ||
| if text.nonEmpty then | ||
| text | ||
| else | ||
| currentText | ||
| println(s"${Symbols.infoColored} ${finalText}") | ||
| running = false | ||
|
|
There was a problem hiding this comment.
DisabledSpinner uses println throughout, which writes to standard output (console.log in JS). UI elements like spinners should write to standard error to avoid mixing status updates with program output. For consistency with JsRunningSpinner and the JVM/Native implementations, please use config.stream.println instead of println in the constructor and in the succeed, fail, warn, and info methods to respect the configured stream (which defaults to System.err).
| private def hideCursor(): Unit = | ||
| if js.typeOf(g.process) != "undefined" then | ||
| g.process.stderr.write("\u001b[?25l") |
There was a problem hiding this comment.
The check js.typeOf(g.process) != "undefined" is repeated across several methods (hideCursor, showCursor, write). To improve code clarity and avoid redundant checks, consider defining a private val isNode = js.typeOf(g.process) != "undefined" at the class level and using it in these methods.
| override def colorLevel: ColorLevel = | ||
| // Check NO_COLOR first (https://no-color.org/) | ||
| if env("NO_COLOR").isDefined then | ||
| return ColorLevel.None | ||
|
|
||
| // Check FORCE_COLOR | ||
| env("FORCE_COLOR") match | ||
| case Some("0") => | ||
| return ColorLevel.None | ||
| case Some("1") => | ||
| return ColorLevel.Basic | ||
| case Some("2") => | ||
| return ColorLevel.Ansi256 | ||
| case Some("3") => | ||
| return ColorLevel.TrueColor | ||
| case Some(_) => | ||
| return ColorLevel.Basic | ||
| case None => // continue detection | ||
|
|
||
| // Check if not connected to a terminal | ||
| if !isInteractive then | ||
| return ColorLevel.None | ||
|
|
||
| // Check COLORTERM for true color support | ||
| env("COLORTERM") match | ||
| case Some("truecolor") | Some("24bit") => | ||
| return ColorLevel.TrueColor | ||
| case _ => // continue | ||
|
|
||
| // Check TERM | ||
| val term = env("TERM").getOrElse("") | ||
|
|
||
| if term == "dumb" then | ||
| return ColorLevel.None | ||
|
|
||
| // Check for 256 color support | ||
| if term.endsWith("-256color") || term.endsWith("256color") then | ||
| return ColorLevel.Ansi256 | ||
|
|
||
| // Check for true color support in terminal name | ||
| if term.contains("truecolor") || term.contains("24bit") then | ||
| return ColorLevel.TrueColor | ||
|
|
||
| // Windows 10+ has native ANSI support | ||
| if isWindows then | ||
| return ColorLevel.Basic | ||
|
|
||
| // Most modern terminals support at least basic colors | ||
| if term.nonEmpty then | ||
| ColorLevel.Basic | ||
| else | ||
| ColorLevel.None | ||
|
|
||
| end colorLevel |
There was a problem hiding this comment.
The colorLevel method uses multiple return statements, which is not idiomatic in Scala. This can make the control flow harder to follow. Refactoring this to a single expression without return would make the code more functional and readable.
| * Disabled spinner that just logs text without animation. | ||
| */ | ||
| private class DisabledSpinner(config: Spinner) extends RunningSpinner: | ||
| private var currentText: String = config.text |
There was a problem hiding this comment.
The currentText field is a var String, which is not thread-safe. Since text_= can be called from any thread, this could lead to visibility issues. Please use java.util.concurrent.atomic.AtomicReference[String] to ensure thread-safety. You will need to update all reads to currentText.get() and writes to currentText.set(newText).
private val currentText = new java.util.concurrent.atomic.AtomicReference[String](config.text)| override def colorLevel: ColorLevel = | ||
| // Check NO_COLOR | ||
| if env("NO_COLOR").isDefined then | ||
| return ColorLevel.None | ||
|
|
||
| // Check FORCE_COLOR | ||
| env("FORCE_COLOR") match | ||
| case Some("0") => | ||
| return ColorLevel.None | ||
| case Some("1") => | ||
| return ColorLevel.Basic | ||
| case Some("2") => | ||
| return ColorLevel.Ansi256 | ||
| case Some("3") => | ||
| return ColorLevel.TrueColor | ||
| case Some(_) => | ||
| return ColorLevel.Basic | ||
| case None => // continue | ||
|
|
||
| // Check COLORTERM | ||
| env("COLORTERM") match | ||
| case Some("truecolor") | Some("24bit") => | ||
| return ColorLevel.TrueColor | ||
| case _ => // continue | ||
|
|
||
| // Check TERM | ||
| val term = env("TERM").getOrElse("") | ||
| if term == "dumb" then | ||
| ColorLevel.None | ||
| else if term.endsWith("-256color") || term.endsWith("256color") then | ||
| ColorLevel.Ansi256 | ||
| else if term.contains("truecolor") || term.contains("24bit") then | ||
| ColorLevel.TrueColor | ||
| else if term.nonEmpty then | ||
| ColorLevel.Basic | ||
| else | ||
| ColorLevel.None | ||
|
|
||
| end colorLevel |
There was a problem hiding this comment.
The colorLevel method uses multiple return statements, which is not idiomatic in Scala. This can make the control flow harder to follow. Refactoring this to a single expression without return would make the code more functional and readable.
| * Disabled spinner that just logs text without animation. | ||
| */ | ||
| private class DisabledSpinner(config: Spinner) extends RunningSpinner: | ||
| private var currentText: String = config.text |
There was a problem hiding this comment.
The currentText field is a var String, which is not thread-safe. Since text_= can be called from any thread and Scala Native supports multi-threading, this could lead to visibility issues. Please use java.util.concurrent.atomic.AtomicReference[String] to ensure thread-safety. You will need to update all reads to currentText.get() and writes to currentText.set(newText).
private val currentText = new java.util.concurrent.atomic.AtomicReference[String](config.text)- Use AtomicBoolean for `finished` field in JVM/Native RunningProgressBar - Use AtomicReference for `currentText` in JVM/Native DisabledSpinner - Use config.stream.println instead of println in JS DisabledSpinner Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Thread-safety fixes addressedBased on Gemini's code review, I've fixed the following thread-safety issues: High Priority (Fixed)
Medium Priority (Fixed)
All changes have been pushed in commit 4d4726c. |
1 participant
Summary
Add CLI utilities package (
wvlet.uni.cli) inspired by TypeScript's Chalk and ora libraries:Chalk: Fluent terminal text styling API
Chalk.red.bold("Error!")"text".red(viaChalkOps)Spinner: Terminal spinner/progress indicator
succeed(),fail(),warn(),info()ProgressBar: Configurable progress bar
Terminal: Cursor control and screen manipulation
Cross-platform: JVM, Scala.js (Node.js), and Scala Native
Usage Examples
Test plan
🤖 Generated with Claude Code