Show lstk aws spinner only for long-running operations#238
Conversation
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@gtsiolis can we get your opinion on this change? |
There was a problem hiding this comment.
Thanks, @peter-smith-phd! Left some comments below. Once we revert the copy and update the delay duration, we can merge this unless there are any engineering concerns from @carole-lavillonniere.
| stdout, stderr := io.Writer(os.Stdout), io.Writer(os.Stderr) | ||
| if terminal.IsTerminal(os.Stderr) { | ||
| s := terminal.NewSpinner(os.Stderr, "Loading...") | ||
| s := terminal.NewSpinner(os.Stderr, "Service loading...", 4*time.Second) |
There was a problem hiding this comment.
issue: On the copy change, I'd revert Service loading... back to Loading.....
- The first one is more ambiguous and confusing. For example, which service is loading?
lstk,docker,localstack, something else? - The verb-based pattern would actually be informative (
Creating bucket…,Listing buckets...etc) but isn't possible without overengineering becauseawscommand is a pure proxy.
There was a problem hiding this comment.
I was intentionally trying to be more specific, and less vague. The problem is that when I see Loading..., I immediately ask myself "What is loading?". When I use the aws command, I don't expect anything to be loading. I don't expect lstk to be loading, nor do I expect docker or localstack because both of them have been loaded already. It was actually quite confusing to see the Loading... message at all, because I wasn't expecting anything to be loading.
By adding Service loading... we're specifically explaining that the delay is (most likely) due to the emulated service loading. Use of aws is usually very quick and responsive, because that's how AWS designed the APIs. The fact that we're using lazy service loading is what makes it sometimes slow. As an expert user of aws, that extra Service word was important to help me understand why.
There was a problem hiding this comment.
Fair point. Could you rename it to Loading service... so it becomes a verb-based label?
| stdout, stderr := io.Writer(os.Stdout), io.Writer(os.Stderr) | ||
| if terminal.IsTerminal(os.Stderr) { | ||
| s := terminal.NewSpinner(os.Stderr, "Loading...") | ||
| s := terminal.NewSpinner(os.Stderr, "Service loading...", 4*time.Second) |
There was a problem hiding this comment.
thought: Adding a delay before showing a spinner is a common pattern in many UI design systems. In contrast, most CLI and TUI design guidelines or frameworks either recommend printing something as fast as possible to the user or don't expose any delay option at all, with one or two exceptions that also default to zero delay. The deferred spinner sounds ok to add here given the AWS CLI itself does not surface any loading feedback.
| stdout, stderr := io.Writer(os.Stdout), io.Writer(os.Stderr) | ||
| if terminal.IsTerminal(os.Stderr) { | ||
| s := terminal.NewSpinner(os.Stderr, "Loading...") | ||
| s := terminal.NewSpinner(os.Stderr, "Service loading...", 4*time.Second) |
There was a problem hiding this comment.
suggestion: In CLI design being responsive is more important than being fast[1]. The 4 second window seems unreasonably high. A slow invocation is indistinguishable from a hung CLI. If we're going to add a delay parameter I'd...
- Add a smaller delay like 1 second, so the spinner doesn't appear on every AWS command invocation but the silent window is short enough to stay responsive.
- Add a minimum visible duration of ~500 ms once the spinner starts, so commands that finish just past the 1 second threshold don't flash a single frame.
- Skip [2] if slow AWS invocations always overshoot 1 scond by a meaningful margin which is true from what I've seen.
There was a problem hiding this comment.
I did think about 3 seconds initially, but when I did some practical experiments it felt like 4 seconds was the time at which I start to think "this is taking a while". Normally, the aws command doesn't show any spinners, nor would customers expect aws commands to take more than a few seconds.
I feel that 1 second is far too short, as the user will see the spinner even on "normal" AWS calls - that is, when there is no lazy-loading actually happening.
There was a problem hiding this comment.
Following up from #238 (comment), we can also keep the 4 seconds for now and adjust later.
| stdout, stderr := io.Writer(os.Stdout), io.Writer(os.Stderr) | ||
| if terminal.IsTerminal(os.Stderr) { | ||
| s := terminal.NewSpinner(os.Stderr, "Loading...") | ||
| s := terminal.NewSpinner(os.Stderr, "Service loading...", 4*time.Second) |
There was a problem hiding this comment.
suggestion: Just to confirm, this adds the delay only for aws commands, right?
There was a problem hiding this comment.
The delay is only added to the non-TTY (aka non-TUI/non-bubbletea) spinner which is only used for the aws command at the moment. However it would be applied to any other future use case.
Taking this back. The delay can be configured so whether the spinner should appear after a delay or not can be decided on a command basis 👏
There was a problem hiding this comment.
Yes, this needs to be configurable for each command. The proposed message Service loading... and the delay of 4 seconds only makes sense in the context of aws. For other commands there would be different messages and different delays.
carole-lavillonniere
left a comment
There was a problem hiding this comment.
Good to go from a code perspective once @gtsiolis is happy, well done @peter-smith-phd
gtsiolis
left a comment
There was a problem hiding this comment.
Looks shippable. ![]()
Updated PR description to use the new copy.
Thanks for contributing, @peter-smith-phd! 🏀
The
lstk awsspinner was rendering for every invocation, which looks odd for the common case where the command completes in well under a second. The spinner now waits 4 s before rendering its first frame, so it only appears for operations that are actually slow (e.g. lazy service init). The label is also clarified from "Loading..." to "Loading service..." so the wait is self-explanatory.Fixed DRG-829