Show container start guidance on more commands#168
Conversation
gtsiolis
requested review from
anisaoshafi,
carole-lavillonniere and
silv-io
as code owners
📝 WalkthroughWalkthroughThe PR adds early runtime health checks to the logs, stop, and status commands to fail immediately if Docker is unavailable, and refactors error output formatting to use styled icons (✗ marker) and blockquote-style summaries instead of plain text prefixes. Changes
Sequence DiagramsequenceDiagram
participant Cmd as Command (logs/stop)
participant RT as Docker Runtime
participant HC as Health Checker
participant Out as Output Sink
participant Err as Error Handler
Cmd->>RT: NewDockerRuntime(cfg)
RT-->>Cmd: runtime instance
Cmd->>HC: checkRuntimeHealth(ctx, rt, cfg)
HC->>RT: IsHealthy(ctx)
alt Runtime is Unhealthy
RT-->>HC: error
HC->>RT: EmitUnhealthyError(error)
RT->>Out: emit styled error event
Out-->>Out: render with ✗ icon & blockquote
HC-->>Cmd: SilentError(wrapped)
Cmd->>Err: abort early
else Runtime is Healthy
RT-->>HC: nil
HC-->>Cmd: nil
Cmd->>Cmd: proceed to load config
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 docstrings
🧪 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.gh.lumenfield.work/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmd/root.go (1)
188-197: Comment is misleading andcfgparameter is unused.The comment states errors are emitted "if not in interactive mode," but the implementation always emits to
PlainSink(os.Stdout)regardless of mode—which aligns with the PR's goal of showing errors consistently. However, this makes thecfgparameter unused and the comment inaccurate.Consider either:
- Removing
cfgfrom the signature and updating the comment to reflect actual behavior, or- If future differentiation is planned, add a TODO comment.
✏️ Suggested comment fix (if cfg removal is desired)
-// checkRuntimeHealth checks if the runtime is healthy and emits an error -// through the sink if not in interactive mode. Returns a SilentError to -// suppress duplicate error printing. -func checkRuntimeHealth(ctx context.Context, rt runtime.Runtime, cfg *env.Env) error { +// checkRuntimeHealth checks if the runtime is healthy and emits an error +// through a plain sink. Returns a SilentError to suppress duplicate error printing. +func checkRuntimeHealth(ctx context.Context, rt runtime.Runtime) error { if err := rt.IsHealthy(ctx); err != nil { rt.EmitUnhealthyError(output.NewPlainSink(os.Stdout), err) return output.NewSilentError(err) } return nil }This would also require updating call sites in
logs.go,stop.go, andstatus.go.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/root.go` around lines 188 - 197, The function checkRuntimeHealth has an unused cfg parameter and an inaccurate comment; remove the cfg *env.Env* parameter from checkRuntimeHealth's signature, update its comment to state it always emits the runtime unhealthy error to a PlainSink(os.Stdout) and returns a SilentError, and then update every call site of checkRuntimeHealth to stop passing cfg (i.e., adjust callers to call checkRuntimeHealth(ctx, rt)); run tests/build to ensure no remaining references.
🤖 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/status.go`:
- Around line 26-40: The NewDockerRuntime failure handling in status.go is
inconsistent with logs.go and stop.go and omits the OS-specific action provided
by EmitUnhealthyError; extract a shared helper (e.g., initAndCheckRuntime or
EnsureHealthyRuntime) that encapsulates runtime.NewDockerRuntime(cfg.DockerHost)
plus the health-check via checkRuntimeHealth and when NewDockerRuntime returns
an error calls the same styled EmitUnhealthyError/EmitError path used elsewhere
(mirroring EmitUnhealthyError's actions and message), then replace the
NewDockerRuntime + checkRuntimeHealth logic in status.go, logs.go, and stop.go
with calls to this helper so all commands present identical, OS-aware error
output.
In `@internal/output/plain_format.go`:
- Around line 153-164: The test suite lacks assertions verifying visual styling
differences between the first and subsequent actions; update
TestErrorDisplay_MultiActionRenders to assert that the rendered output contains
styling markers (or ANSI sequences) produced by errorActionStyle and
errorValueStyle for the first action and that subsequent actions include the
errorMutedStyle output (e.g., by checking for the specific ANSI color/bold
sequences or unique style markers applied by errorActionStyle/errorValueStyle vs
errorMutedStyle in the output string derived from rendering e.Actions); locate
rendering logic around e.Actions in plain_format.go (errorActionStyle,
errorValueStyle, errorMutedStyle) and enhance the test to explicitly assert the
first action uses the action+value styles while later actions use the muted
style.
---
Nitpick comments:
In `@cmd/root.go`:
- Around line 188-197: The function checkRuntimeHealth has an unused cfg
parameter and an inaccurate comment; remove the cfg *env.Env* parameter from
checkRuntimeHealth's signature, update its comment to state it always emits the
runtime unhealthy error to a PlainSink(os.Stdout) and returns a SilentError, and
then update every call site of checkRuntimeHealth to stop passing cfg (i.e.,
adjust callers to call checkRuntimeHealth(ctx, rt)); run tests/build to ensure
no remaining references.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 04e4a18d-8ab3-4aaa-bfd5-9c04aa757e7b
📒 Files selected for processing (7)
cmd/logs.gocmd/root.gocmd/status.gocmd/stop.gointernal/output/plain_format.gointernal/output/plain_format_test.gointernal/output/plain_sink_test.go
| rt, err := runtime.NewDockerRuntime(cfg.DockerHost) | ||
| if err != nil { | ||
| output.EmitError(output.NewPlainSink(os.Stdout), output.ErrorEvent{ | ||
| Title: "Docker is not available", | ||
| Summary: err.Error(), | ||
| Actions: []output.ErrorAction{ | ||
| {Label: "See help:", Value: "lstk -h"}, | ||
| {Label: "Install Docker:", Value: "https://docs.docker.com/get-docker/"}, | ||
| }, | ||
| }) | ||
| return output.NewSilentError(err) | ||
| } | ||
| if err := checkRuntimeHealth(cmd.Context(), rt, cfg); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Inconsistent error handling between commands and duplicate logic.
status.go now handles NewDockerRuntime failures with a styled error, but logs.go and stop.go return the raw error for the same failure case. This creates inconsistent UX—users get a styled error from lstk status but a plain error from lstk logs when Docker client initialization fails.
Additionally, the error emitted here (lines 28-35) differs from EmitUnhealthyError (used by checkRuntimeHealth): this version omits the OS-specific "Start Docker Desktop" action that EmitUnhealthyError provides.
Consider:
- Applying the same
NewDockerRuntimeerror handling tologs.goandstop.go, or - Extracting a shared helper that handles both runtime creation and health check consistently.
🔧 One possible approach: reuse EmitUnhealthyError pattern
rt, err := runtime.NewDockerRuntime(cfg.DockerHost)
if err != nil {
- output.EmitError(output.NewPlainSink(os.Stdout), output.ErrorEvent{
- Title: "Docker is not available",
- Summary: err.Error(),
- Actions: []output.ErrorAction{
- {Label: "See help:", Value: "lstk -h"},
- {Label: "Install Docker:", Value: "https://docs.docker.com/get-docker/"},
- },
- })
- return output.NewSilentError(err)
+ // Create a minimal runtime just for error emission, or inline the error event
+ sink := output.NewPlainSink(os.Stdout)
+ output.EmitError(sink, output.ErrorEvent{
+ Title: "Docker is not available",
+ Summary: fmt.Sprintf("cannot initialize Docker client: %v", err),
+ Actions: dockerInstallActions(), // shared helper with OS-specific actions
+ })
+ return output.NewSilentError(err)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/status.go` around lines 26 - 40, The NewDockerRuntime failure handling in
status.go is inconsistent with logs.go and stop.go and omits the OS-specific
action provided by EmitUnhealthyError; extract a shared helper (e.g.,
initAndCheckRuntime or EnsureHealthyRuntime) that encapsulates
runtime.NewDockerRuntime(cfg.DockerHost) plus the health-check via
checkRuntimeHealth and when NewDockerRuntime returns an error calls the same
styled EmitUnhealthyError/EmitError path used elsewhere (mirroring
EmitUnhealthyError's actions and message), then replace the NewDockerRuntime +
checkRuntimeHealth logic in status.go, logs.go, and stop.go with calls to this
helper so all commands present identical, OS-aware error output.
| if len(e.Actions) > 0 { | ||
| sb.WriteString("\n") | ||
| for i, action := range e.Actions { | ||
| sb.WriteString("\n") | ||
| if i > 0 { | ||
| sb.WriteString(errorMutedStyle.Render(ErrorActionPrefix + action.Label + " " + action.Value)) | ||
| } else { | ||
| sb.WriteString(errorActionStyle.Render(ErrorActionPrefix+action.Label+" ")) | ||
| sb.WriteString(errorValueStyle.Render(action.Value)) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there are any tests covering multiple ErrorActions
rg -n 'Actions:.*ErrorAction.*ErrorAction' --type go
# Also check for tests with Actions array having more than one element
rg -n 'Actions: \[\]output\.ErrorAction\{' -A5 --type go | head -50Repository: localstack/lstk
Length of output: 1684
🏁 Script executed:
cat -n internal/ui/components/error_display_test.go | head -100Repository: localstack/lstk
Length of output: 3811
🏁 Script executed:
cat -n internal/output/plain_format.go | sed -n '153,164p'Repository: localstack/lstk
Length of output: 524
🏁 Script executed:
rg -n 'errorActionStyle\|errorValueStyle\|errorMutedStyle' --type go -B2 -A2Repository: localstack/lstk
Length of output: 41
🏁 Script executed:
rg -n 'errorMutedStyle|errorActionStyle|errorValueStyle' -B3 -A3Repository: localstack/lstk
Length of output: 1930
🏁 Script executed:
fd 'error_display.go|plain_format.go' --type fRepository: localstack/lstk
Length of output: 130
🏁 Script executed:
cat -n internal/output/plain_format.go | sed -n '1,50p'Repository: localstack/lstk
Length of output: 1700
Add test coverage to validate the visual styling differentiation for multiple actions.
The code implements distinct styling: the first action renders with errorActionStyle (color) and errorValueStyle (bold), while subsequent actions use errorMutedStyle (muted grey). The existing test TestErrorDisplay_MultiActionRenders covers multiple actions but only validates content presence via strings.Contains(), not the styling output.
Add a test assertion that validates the rendered output distinguishes the first action visually from subsequent ones to document this intentional design.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/output/plain_format.go` around lines 153 - 164, The test suite lacks
assertions verifying visual styling differences between the first and subsequent
actions; update TestErrorDisplay_MultiActionRenders to assert that the rendered
output contains styling markers (or ANSI sequences) produced by errorActionStyle
and errorValueStyle for the first action and that subsequent actions include the
errorMutedStyle output (e.g., by checking for the specific ANSI color/bold
sequences or unique style markers applied by errorActionStyle/errorValueStyle vs
errorMutedStyle in the output string derived from rendering e.Actions); locate
rendering logic around e.Actions in plain_format.go (errorActionStyle,
errorValueStyle, errorMutedStyle) and enhance the test to explicitly assert the
first action uses the action+value styles while later actions use the muted
style.
| errorSecondaryStyle = lipgloss.NewStyle().Foreground(lipgloss.Color("241")) | ||
| errorActionStyle = lipgloss.NewStyle().Foreground(lipgloss.Color("69")) | ||
| errorValueStyle = lipgloss.NewStyle().Bold(true) | ||
| errorMutedStyle = lipgloss.NewStyle().Foreground(lipgloss.Color("245")) |
There was a problem hiding this comment.
issue: do we really mean to redeclare all these, since they already exist in internal/ui/styles/styles.go?
| sb.WriteString(errorActionStyle.Render(ErrorActionPrefix+action.Label+" ")) | ||
| sb.WriteString(errorValueStyle.Render(action.Value)) | ||
| } | ||
| } |
There was a problem hiding this comment.
question: I would like to hear what @silv-io says here. We were previously not doing any styling in non-interactive mode.
There was a problem hiding this comment.
The plain output should remain styling free IMO. If we want a non-interactive styled output I think that should be a separate sink
There was a problem hiding this comment.
Why do you think so?
Right now it's binary: interactive TTY gets Bubble Tea, everything else gets plain. A third styled-but-non-interactive sink would need a trigger. What would it be? CI is a good example: no TTY, so non-interactive, but GitHub Actions and most CI systems do render ANSI colors fine.
There was a problem hiding this comment.
the trigger word there is "most" for me. I don't want to force color onto systems where it might not be rendered well. If we add color styling to the plain output, we should at the least check that it will handle NO_COLOR correctly and add a test for that.
Also, then I think the styling should live outside of the UI package and in its own "style" package to avoid potential circular dependencies in the future.
There was a problem hiding this comment.
I think for now we could just remove colors/styling from plain sink and revisit later if we need a third kind of sink.
Fine for you @gtsiolis?
I'm wondering if this was intentional in the first place?
3 participants
Show actionable Docker errors consistently across all commands
When Docker isn't available, status, stop, and logs now all show a clear, styled error message with actionable next steps — regardless of whether the terminal is interactive or not.
Previously the error guidance was only shown in non-interactive mode (piped output), so running lstk status in a terminal with Docker stopped produced no visible output. The plain formatter also rendered errors as unstyled text even when colors were available.