Open
Conversation
gtsiolis
requested review from
anisaoshafi,
carole-lavillonniere and
silv-io
as code owners
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
📝 WalkthroughWalkthroughAdded a new Changes
Sequence DiagramsequenceDiagram
participant User as User
participant LSCmd as lstk aws
participant Config as Config/Container
participant AWSCli as AWS CLI
participant Spinner as Spinner
User->>LSCmd: lstk aws [args...]
LSCmd->>Config: resolveAWSPort()
Config-->>LSCmd: port
LSCmd->>LSCmd: Resolve LocalStack endpoint<br/>(host:port)
LSCmd->>LSCmd: BuildEnv() for AWS credentials
alt Terminal Detected
LSCmd->>Spinner: Start spinner
end
LSCmd->>AWSCli: Exec with<br/>--endpoint-url + args
AWSCli->>AWSCli: Execute command
alt Output Received
AWSCli-->>Spinner: Write to stdout/stderr
Spinner->>Spinner: Stop on first write
end
AWSCli-->>LSCmd: Exit status
LSCmd-->>User: Output + exit code
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (1)
cmd/aws.go (1)
15-25: Align user-facing help text with emulator terminology guidance.Please update the command help to reference emulator terminology/types instead of only “LocalStack” phrasing.
As per coding guidelines, CLI/help/docs in
cmd/**/*.goshould reference “emulator types (aws, snowflake, azure)” with only aws currently implemented.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/aws.go` around lines 15 - 25, Update the user-facing help text for the AWS CLI command (the cobra command variable like awsCmd / the Short and Long strings) to use the emulator terminology: refer to the emulator type "aws" (and note emulator types such as aws, snowflake, azure) instead of only "LocalStack"; keep existing example showing endpoint and env vars but change phrases like "Proxy AWS CLI commands to LocalStack" to "Proxy AWS CLI commands to the aws emulator (LocalStack)" and mention that only the aws emulator type is currently implemented. Ensure both Short and Long strings are updated accordingly to follow cmd/**/*.go guidance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/aws.go`:
- Around line 44-47: The loop over appCfg.Containers returning c.Port for the
AWS emulator can return an empty string; update the logic in the function that
iterates appCfg.Containers (checking c.Type == config.EmulatorAWS) to guard that
c.Port is not empty before returning it—i.e., only return c.Port when both the
container type equals config.EmulatorAWS and c.Port != ""; otherwise continue
searching and if no non-empty port is found, return the configured default port
value (use the existing default constant or literal used elsewhere in the file).
In `@internal/awscli/exec.go`:
- Around line 17-27: The spinner can be stopped twice (one per stdout/stderr
wrapper) and may never be stopped if the subprocess emits no output; fix by
making Stop idempotent and by sharing a single stop action between writers and
invoking it when the process exits. Update the spinner type to guard its Stop
method with an internal sync.Once (make spinner.Stop safe for repeated calls),
and change stopOnWriteWriter construction so both instances receive the same
stop function (or a shared sync.Once) instead of calling spinner.Stop directly
in Write; also ensure the code that waits for the subprocess explicitly calls
that shared stop function after process completion so the spinner is always
stopped even when there is no output (referencing stopOnWriteWriter, its Write
method, and spinner.Stop).
In `@test/integration/aws_cmd_test.go`:
- Around line 42-43: Tests in aws_cmd_test.go currently use runLstk (non-TTY)
and a fake aws binary that only echoes, so spinner lifecycle (TTY activation and
stopping on first streamed output) isn't exercised; add a new PTY-based
integration test that uses runLstkInPTY and a fake aws binary which streams
output (writes something after a short delay) to validate that the spinner
appears while waiting and is cleared once the first output is streamed. Create a
new test function (e.g., TestAwsCmd_SpinnerLifecyclePTY) that calls
runLstkInPTY(testContext(t), t.TempDir(), e, "aws", "s3", "ls") with the
streaming fake binary and assert PTY output contains spinner characters prior to
streamed content and that spinner is removed once output begins; leave existing
runLstk-based tests (argument injection and credential propagation) unchanged.
---
Nitpick comments:
In `@cmd/aws.go`:
- Around line 15-25: Update the user-facing help text for the AWS CLI command
(the cobra command variable like awsCmd / the Short and Long strings) to use the
emulator terminology: refer to the emulator type "aws" (and note emulator types
such as aws, snowflake, azure) instead of only "LocalStack"; keep existing
example showing endpoint and env vars but change phrases like "Proxy AWS CLI
commands to LocalStack" to "Proxy AWS CLI commands to the aws emulator
(LocalStack)" and mention that only the aws emulator type is currently
implemented. Ensure both Short and Long strings are updated accordingly to
follow cmd/**/*.go guidance.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 2e1d4c43-f552-4aa8-b33a-bb1ff4a6edc1
📒 Files selected for processing (9)
cmd/aws.gocmd/root.gointernal/awscli/exec.gointernal/awscli/exec_test.gointernal/awscli/spinner.gointernal/output/errors.gomain.gotest/integration/aws_cmd_test.gotest/integration/start_test.go
Comment on lines
+44
to
+47
| for _, c := range appCfg.Containers { | ||
| if c.Type == config.EmulatorAWS { | ||
| return c.Port | ||
| } |
There was a problem hiding this comment.
Guard against empty AWS port config before returning it.
If the AWS container exists but c.Port is empty, Line 46 returns "", which can yield an invalid endpoint URL. Fall back to default in that case.
Proposed fix
func resolveAWSPort() string {
const defaultPort = "4566"
@@
for _, c := range appCfg.Containers {
if c.Type == config.EmulatorAWS {
- return c.Port
+ if c.Port != "" {
+ return c.Port
+ }
+ return defaultPort
}
}
return defaultPort
}📝 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.
Suggested change
| for _, c := range appCfg.Containers { | |
| if c.Type == config.EmulatorAWS { | |
| return c.Port | |
| } | |
| func resolveAWSPort() string { | |
| const defaultPort = "4566" | |
| for _, c := range appCfg.Containers { | |
| if c.Type == config.EmulatorAWS { | |
| if c.Port != "" { | |
| return c.Port | |
| } | |
| return defaultPort | |
| } | |
| } | |
| return defaultPort | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/aws.go` around lines 44 - 47, The loop over appCfg.Containers returning
c.Port for the AWS emulator can return an empty string; update the logic in the
function that iterates appCfg.Containers (checking c.Type == config.EmulatorAWS)
to guard that c.Port is not empty before returning it—i.e., only return c.Port
when both the container type equals config.EmulatorAWS and c.Port != "";
otherwise continue searching and if no non-empty port is found, return the
configured default port value (use the existing default constant or literal used
elsewhere in the file).
Comment on lines
+17
to
+27
| type stopOnWriteWriter struct { | ||
| w io.Writer | ||
| spinner *spinner | ||
| once sync.Once | ||
| } | ||
|
|
||
| func (s *stopOnWriteWriter) Write(p []byte) (int, error) { | ||
| s.once.Do(func() { | ||
| s.spinner.Stop() | ||
| }) | ||
| return s.w.Write(p) |
There was a problem hiding this comment.
Fix spinner lifecycle: possible panic + leaked spinner when no output.
Current logic allows two independent stop paths (stdout/stderr wrappers), so Stop() can be invoked twice. Also, if the subprocess exits without writing output, the spinner is never stopped.
Proposed fix
type stopOnWriteWriter struct {
- w io.Writer
- spinner *spinner
- once sync.Once
+ w io.Writer
+ stop func()
}
func (s *stopOnWriteWriter) Write(p []byte) (int, error) {
- s.once.Do(func() {
- s.spinner.Stop()
- })
+ s.stop()
return s.w.Write(p)
}
@@
var s *spinner
if isTerminal(os.Stderr) {
s = newSpinner(os.Stderr, "Loading...")
s.Start()
+ var stopOnce sync.Once
+ stopSpinner := func() {
+ stopOnce.Do(func() { s.Stop() })
+ }
+ defer stopSpinner()
// Wrap stdout/stderr to stop spinner on first output
- stopWriter := &stopOnWriteWriter{w: os.Stdout, spinner: s}
- cmd.Stdout = stopWriter
- cmd.Stderr = &stopOnWriteWriter{w: os.Stderr, spinner: s}
+ cmd.Stdout = &stopOnWriteWriter{w: os.Stdout, stop: stopSpinner}
+ cmd.Stderr = &stopOnWriteWriter{w: os.Stderr, stop: stopSpinner}
} else {
cmd.Stdout = os.Stdout
cmd.Stderr = os.Stderr
}Also applies to: 44-56, 58-61
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/awscli/exec.go` around lines 17 - 27, The spinner can be stopped
twice (one per stdout/stderr wrapper) and may never be stopped if the subprocess
emits no output; fix by making Stop idempotent and by sharing a single stop
action between writers and invoking it when the process exits. Update the
spinner type to guard its Stop method with an internal sync.Once (make
spinner.Stop safe for repeated calls), and change stopOnWriteWriter construction
so both instances receive the same stop function (or a shared sync.Once) instead
of calling spinner.Stop directly in Write; also ensure the code that waits for
the subprocess explicitly calls that shared stop function after process
completion so the spinner is always stopped even when there is no output
(referencing stopOnWriteWriter, its Write method, and spinner.Stop).
Comment on lines
+42
to
+43
| stdout, stderr, err := runLstk(t, testContext(t), t.TempDir(), e, "aws", "s3", "ls") | ||
| require.NoError(t, err, "lstk aws failed: %s", stderr) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify whether aws integration coverage includes PTY/spinner assertions.
rg -nP 'runLstkInPTY\(|runLstk\(|Loading|spinner' test/integration/aws_cmd_test.go test/integration/main_test.goRepository: localstack/lstk
Length of output: 1085
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check runLstkInPTY implementation
echo "=== runLstkInPTY Implementation ==="
sed -n '236,280p' test/integration/main_test.go
# Check if any integration tests use runLstkInPTY
echo ""
echo "=== Other Tests Using runLstkInPTY ==="
rg -l 'runLstkInPTY' test/integration/ || echo "No other tests found using runLstkInPTY"
# Look for spinner-related code in cmd/
echo ""
echo "=== Spinner-Related Code in cmd/ ==="
rg -n 'spinner|Loading' cmd/ --type go | head -30
# Check the aws command implementation
echo ""
echo "=== AWS Command Implementation ==="
fd -e go 'aws.*\.go' cmd/ | xargs grep -l "spinner\|Loading" || echo "No spinner found in aws command files"Repository: localstack/lstk
Length of output: 1441
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all aws command related files
echo "=== AWS Command Files ==="
fd -e go 'aws' cmd/ -t f
# Check if spinner is part of output streaming in aws command
echo ""
echo "=== Check AWS S3 Command Implementation ==="
rg -n 'func.*aws.*s3\|func.*S3' cmd/ -A 20 | head -50Repository: localstack/lstk
Length of output: 140
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the aws command file completely
echo "=== aws.go Content ==="
wc -l cmd/aws.go
# If it's reasonable size, read it all
if [ $(wc -l < cmd/aws.go) -lt 200 ]; then
cat -n cmd/aws.go
else
echo "File is large, reading sections..."
fiRepository: localstack/lstk
Length of output: 1842
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for any streaming output or Bubble Tea related code
echo "=== Search for Bubble Tea/Tea imports ==="
rg -n 'tea\.|bubbletea|Bubble Tea' cmd/ --type go
# Search for output sink or output mode selection
echo ""
echo "=== Search for output sink selection ==="
rg -n 'PlainSink|TTY|interactive|output\.' cmd/aws.go -B 2 -A 2
# Check what output handling is done in the actual aws s3 ls command
echo ""
echo "=== Full aws.go to see output handling ==="
cat cmd/aws.goRepository: localstack/lstk
Length of output: 100
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find and check awscli package
echo "=== Find awscli files ==="
fd -e go 'awscli' internal/
# Search for spinner in internal/awscli
echo ""
echo "=== Search for spinner/Bubble Tea in awscli ==="
rg -n 'spinner|tea\.|bubbletea|Loading' internal/awscli/ --type go || echo "No spinner found in awscli"
# Check the awscli.Exec function
echo ""
echo "=== Check Exec function signature ==="
rg -n 'func Exec' internal/awscli/ -A 5Repository: localstack/lstk
Length of output: 1846
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the entire aws_cmd_test.go file to understand test structure
echo "=== aws_cmd_test.go Content ==="
cat -n test/integration/aws_cmd_test.goRepository: localstack/lstk
Length of output: 5217
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read exec.go to understand spinner activation condition
echo "=== internal/awscli/exec.go ==="
cat -n internal/awscli/exec.go
# Check if there are any existing spinner tests
echo ""
echo "=== Search for spinner tests ==="
rg -n 'spinner\|Loading' test/ --type goRepository: localstack/lstk
Length of output: 2706
🏁 Script executed:
#!/bin/bash
set -eulo pipefail
# Find isTerminal function definition
echo "=== Search for isTerminal function ==="
rg -n 'func isTerminal' internal/awscli/ -A 5
# Check if there are any other places where spinner is used or tested
echo ""
echo "=== Check spinner usage in entire codebase ==="
rg -n 'newSpinner\|spinner\.' --type go | grep -v test | grep -v '.go:.*// ' || echo "No additional spinner usage found"
# Verify that the review comment's claim about non-interactive spinner assertions
echo ""
echo "=== Verify stopOnWriteWriter implementation ==="
sed -n '16,28p' internal/awscli/exec.goRepository: localstack/lstk
Length of output: 942
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check what isTerminal does
fd -e go . internal/ | xargs grep -l 'isTerminal' | head -5
# Look for terminal detection helper
rg -n 'isTerminal\|IsTerminal' --type go | head -20Repository: localstack/lstk
Length of output: 109
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the full isTerminal implementation
echo "=== Full isTerminal implementation ==="
sed -n '74,90p' internal/awscli/spinner.go
# Check if there are any other tests that validate spinner behavior
echo ""
echo "=== Check for spinner-related assertions in tests ==="
rg -n 'Loading\|spinner\|Stop\|Start' test/integration/ --type go -B 2 -A 2
# Understand what the learnings ask - check if any cmd tests use runLstkInPTY
echo ""
echo "=== Check all cmd tests for PTY usage ==="
fd -e go 'cmd.*test.go' test/integration/ | xargs grep -l 'runLstkInPTY\|PTY' || echo "No cmd tests use PTY"Repository: localstack/lstk
Length of output: 346
Add a PTY-based integration test for spinner lifecycle in AWS command.
The spinner behavior (activate on TTY, stop on first streamed output) is not tested. All existing aws_cmd_test.go tests use runLstk(), which executes in non-interactive mode, and employ a fake aws binary that only echoes arguments—neither condition triggers or validates spinner behavior.
A dedicated regression test should use runLstkInPTY() with a fake aws binary that streams output to verify the spinner displays while waiting and clears when output begins. The existing tests (lines 42-43, 53-54, 69-70, 80-81, 101-102, 130-131) validating argument injection and credential propagation do not require conversion to PTY.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/integration/aws_cmd_test.go` around lines 42 - 43, Tests in
aws_cmd_test.go currently use runLstk (non-TTY) and a fake aws binary that only
echoes, so spinner lifecycle (TTY activation and stopping on first streamed
output) isn't exercised; add a new PTY-based integration test that uses
runLstkInPTY and a fake aws binary which streams output (writes something after
a short delay) to validate that the spinner appears while waiting and is cleared
once the first output is streamed. Create a new test function (e.g.,
TestAwsCmd_SpinnerLifecyclePTY) that calls runLstkInPTY(testContext(t),
t.TempDir(), e, "aws", "s3", "ls") with the streaming fake binary and assert PTY
output contains spinner characters prior to streamed content and that spinner is
removed once output begins; leave existing runLstk-based tests (argument
injection and credential propagation) unchanged.
anisaoshafi
reviewed
Mar 27, 2026
Contributor
There was a problem hiding this comment.
Thanks George for investing in this nice feature ⭐
- Issue: The spinner behaviour is weird, it gets stuck e.g:
- a) when you try
lstk aws s3 lswhen you have no bucket - b) create a bucket and then delete bucket. I suggest to revisit it to make it more robust 🙏🏼
- Suggestion: would be nice to add a more descriptive message when LocalStack is not running:
anisaoshafi
reviewed
Mar 27, 2026
Comment on lines
+7
to
+19
| "strings" | ||
| "sync" | ||
| "time" | ||
| ) | ||
|
|
||
| var dotFrames = []string{"⣾", "⣽", "⣻", "⢿", "⡿", "⣟", "⣯", "⣷"} | ||
|
|
||
| // ANSI color codes matching lstk's spinner style (color 69 blue) and secondary (color 241 gray) | ||
| const ( | ||
| spinnerColor = "\033[38;5;69m" | ||
| secondaryColor = "\033[38;5;241m" | ||
| resetColor = "\033[0m" | ||
| ) |
Contributor
There was a problem hiding this comment.
issue: didn't we have these properties defined somewhere for Nimbo? I think they're duplicated.
Also, In components package, we have spinner defined. Why are we redefining it again in this PR, can you try reuse them?
Btw 🎗️, we have this /review-pr claude skill: /review-pr #176 that the team added, and it can be run in local development to catch this architectural pattern issues 🙏🏼
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Updates the AWS CLI proxy command to show a loading spinner while waiting for a response, then automatically hide it as soon as output starts streaming.
See DES-190 and DRG-364 for more context.