Conversation
gtsiolis
requested review from
carole-lavillonniere and
silv-io
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds Docker daemon health checks to the Runtime interface and DockerRuntime, introduces a SilentError wrapper to prevent re-printing already-displayed errors, updates error handling in command initialization to conditionally print errors, and generates GoMock mocks for testing. Changes
Sequence Diagram(s)sequenceDiagram
participant Cmd as Command
participant Container as Container Start
participant Runtime as Runtime/Docker
participant Output as Output Handler
Cmd->>Container: Start(ctx)
Container->>Runtime: Healthy(ctx)
alt Docker Daemon Available
Runtime->>Runtime: Ping Docker
Runtime-->>Container: nil
Container->>Container: selectContainersToStart()
else Docker Daemon Unavailable
Runtime->>Runtime: Ping fails
Runtime-->>Container: "cannot connect to Docker daemon" error
Container->>Output: EmitWarning(Docker guidance)
Output-->>Container:
Container->>Container: Wrap error as SilentError
Container-->>Cmd: SilentError returned
end
alt IsSilent(error) check
Cmd->>Cmd: Skip printing error
else Error not silent
Cmd->>Output: Print "Error: ..."
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 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: 2
🧹 Nitpick comments (3)
cmd/error_output.go (1)
12-21: Consider moving error styles tointernal/ui/styles.The error output styles are defined inline here. Based on coding guidelines, semantic styles are typically defined in
internal/ui/styles/styles.go. However, if these styles are intentionally scoped to CLI-only error output (separate from TUI), this is acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/error_output.go` around lines 12 - 21, The error style variables (errorTitleStyle, errorBodyStyle, errorNoteStyle, errorSummaryPrefix, errorActionArrowStyle, errorActionTextStyle, errorLinkStyle, errorK8sStyle) are declared inline in cmd/error_output.go and should be relocated to the shared semantic styles package to follow project conventions; move their definitions into internal/ui/styles (e.g., styles.go) as exported or package-level variables and import/use them from cmd/error_output.go (or if these styles are intentionally CLI-only, add a brief comment above the block explaining they are CLI-scoped to justify keeping them here).cmd/stop.go (1)
23-30: Pre-existing pattern:onProgresscallback passed to domain layer.The
onProgresscallback passed tocontainer.Stopmay conflict with the guideline to emit typed events throughinternal/outputrather than passing UI callbacks through domain layers. This is pre-existing code, so it's not blocking for this PR, but could be addressed in a future refactor for consistency with the output event pattern used elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/stop.go` around lines 23 - 30, The code passes a UI callback `onProgress` into the domain call `container.Stop`, which contradicts the project pattern of emitting typed events via the `internal/output` event system; refactor by replacing the `onProgress` callback use with emission of appropriate `internal/output` events (or an adapter that converts progress messages into those events) before calling `container.Stop`, and update `container.Stop` (and any related domain APIs) to accept no UI callbacks (or an event-emitter interface) so progress is reported through the `internal/output` event types instead of the `onProgress` function.internal/runtime/engine_check.go (1)
62-65: Consider expanding the Docker daemon unavailability heuristics.The current checks cover common error messages, but Docker can emit additional patterns depending on the environment (e.g.,
"connection refused","no such host","permission denied"for socket access). These would currently fall through to the generic error path on line 59, which is acceptable but produces a less user-friendly message.This is fine to defer if the current coverage meets your needs—the fallback still provides a clear error with the endpoint context.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/runtime/engine_check.go` around lines 62 - 65, Expand the Docker daemon unavailability heuristics in isDockerDaemonUnavailable by checking additional common error substrings (e.g., "connection refused", "no such host", "permission denied", "docker.sock", "dial unix") and make the match case-insensitive (use strings.ToLower(msg)) so these variants are recognized; update the function isDockerDaemonUnavailable to include these extra contains checks (or a small slice+loop of patterns) so more environment-specific daemon errors return true and trigger the friendly endpoint-specific message.
🤖 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/error_output.go`:
- Around line 50-57: The fmt.Fprintln call in printActionLine currently ignores
its returned (n, error); change it to explicitly discard the results or handle
the error—e.g., replace the plain fmt.Fprintln(...) with an assignment like "_,
_ = fmt.Fprintln(...)" inside the printActionLine function so the returned
values are not unchecked (or log/return the error if preferred).
- Around line 39-48: The linter flags an unchecked error from fmt.Fprintln
inside printIndentedStyled; update printIndentedStyled to handle the returned
(int, error) by capturing and explicitly handling or ignoring the error to
satisfy the linter—e.g., assign the returned values and, if desired, log or
discard the error via a named blank identifier or a short conditional that
ignores it; ensure the change is applied to the fmt.Fprintln call(s) inside the
printIndentedStyled function so the error return is no longer unchecked.
---
Nitpick comments:
In `@cmd/error_output.go`:
- Around line 12-21: The error style variables (errorTitleStyle, errorBodyStyle,
errorNoteStyle, errorSummaryPrefix, errorActionArrowStyle, errorActionTextStyle,
errorLinkStyle, errorK8sStyle) are declared inline in cmd/error_output.go and
should be relocated to the shared semantic styles package to follow project
conventions; move their definitions into internal/ui/styles (e.g., styles.go) as
exported or package-level variables and import/use them from cmd/error_output.go
(or if these styles are intentionally CLI-only, add a brief comment above the
block explaining they are CLI-scoped to justify keeping them here).
In `@cmd/stop.go`:
- Around line 23-30: The code passes a UI callback `onProgress` into the domain
call `container.Stop`, which contradicts the project pattern of emitting typed
events via the `internal/output` event system; refactor by replacing the
`onProgress` callback use with emission of appropriate `internal/output` events
(or an adapter that converts progress messages into those events) before calling
`container.Stop`, and update `container.Stop` (and any related domain APIs) to
accept no UI callbacks (or an event-emitter interface) so progress is reported
through the `internal/output` event types instead of the `onProgress` function.
In `@internal/runtime/engine_check.go`:
- Around line 62-65: Expand the Docker daemon unavailability heuristics in
isDockerDaemonUnavailable by checking additional common error substrings (e.g.,
"connection refused", "no such host", "permission denied", "docker.sock", "dial
unix") and make the match case-insensitive (use strings.ToLower(msg)) so these
variants are recognized; update the function isDockerDaemonUnavailable to
include these extra contains checks (or a small slice+loop of patterns) so more
environment-specific daemon errors return true and trigger the friendly
endpoint-specific message.
cmd/error_output.go
Outdated
Comment on lines
+39
to
+48
| func printIndentedStyled(out *os.File, text, indent string, style lipgloss.Style) { | ||
| lines := strings.Split(text, "\n") | ||
| for i, line := range lines { | ||
| lineIndent := indent | ||
| if i > 0 { | ||
| lineIndent += " " | ||
| } | ||
| fmt.Fprintln(out, lineIndent+style.Render(line)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Unchecked error return from fmt.Fprintln.
The linter flags that the return value of fmt.Fprintln is not checked. While the impact is low (this is error output right before exit), addressing the lint warning keeps the codebase clean.
Proposed fix
func printIndentedStyled(out *os.File, text, indent string, style lipgloss.Style) {
lines := strings.Split(text, "\n")
for i, line := range lines {
lineIndent := indent
if i > 0 {
lineIndent += " "
}
- fmt.Fprintln(out, lineIndent+style.Render(line))
+ _, _ = fmt.Fprintln(out, lineIndent+style.Render(line))
}
}📝 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
| func printIndentedStyled(out *os.File, text, indent string, style lipgloss.Style) { | |
| lines := strings.Split(text, "\n") | |
| for i, line := range lines { | |
| lineIndent := indent | |
| if i > 0 { | |
| lineIndent += " " | |
| } | |
| fmt.Fprintln(out, lineIndent+style.Render(line)) | |
| } | |
| } | |
| func printIndentedStyled(out *os.File, text, indent string, style lipgloss.Style) { | |
| lines := strings.Split(text, "\n") | |
| for i, line := range lines { | |
| lineIndent := indent | |
| if i > 0 { | |
| lineIndent += " " | |
| } | |
| _, _ = fmt.Fprintln(out, lineIndent+style.Render(line)) | |
| } | |
| } |
🧰 Tools
🪛 GitHub Check: Lint
[failure] 46-46:
Error return value of fmt.Fprintln is not checked (errcheck)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/error_output.go` around lines 39 - 48, The linter flags an unchecked
error from fmt.Fprintln inside printIndentedStyled; update printIndentedStyled
to handle the returned (int, error) by capturing and explicitly handling or
ignoring the error to satisfy the linter—e.g., assign the returned values and,
if desired, log or discard the error via a named blank identifier or a short
conditional that ignores it; ensure the change is applied to the fmt.Fprintln
call(s) inside the printIndentedStyled function so the error return is no longer
unchecked.
cmd/error_output.go
Outdated
Comment on lines
+50
to
+57
| func printActionLine(out *os.File, label, value string, labelStyle lipgloss.Style) { | ||
| fmt.Fprintln( | ||
| out, | ||
| errorActionArrowStyle.Render("⇒")+" "+ | ||
| labelStyle.Render(label)+" "+ | ||
| errorLinkStyle.Render(value), | ||
| ) | ||
| } |
There was a problem hiding this comment.
Unchecked error return from fmt.Fprintln.
Same lint issue here—consider using _, _ = to explicitly discard the return values.
Proposed fix
func printActionLine(out *os.File, label, value string, labelStyle lipgloss.Style) {
- fmt.Fprintln(
+ _, _ = fmt.Fprintln(
out,
errorActionArrowStyle.Render("⇒")+" "+
labelStyle.Render(label)+" "+
errorLinkStyle.Render(value),
)
}🧰 Tools
🪛 GitHub Check: Lint
[failure] 51-51:
Error return value of fmt.Fprintln is not checked (errcheck)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/error_output.go` around lines 50 - 57, The fmt.Fprintln call in
printActionLine currently ignores its returned (n, error); change it to
explicitly discard the results or handle the error—e.g., replace the plain
fmt.Fprintln(...) with an assignment like "_, _ = fmt.Fprintln(...)" inside the
printActionLine function so the returned values are not unchecked (or log/return
the error if preferred).
silv-io
requested changes
Feb 23, 2026
Member
silv-io
left a comment
silv-io
left a comment
There was a problem hiding this comment.
What's blocking is that we're defining too much logic in the cmd package. That one is just for surface level orchestration. Styling etc. should all happen in the UI package
cmd/error_output.go
Outdated
Comment on lines
+13
to
+20
| errorTitleStyle = lipgloss.NewStyle().Bold(true).Foreground(lipgloss.Color("#D04029")) | ||
| errorBodyStyle = lipgloss.NewStyle().Foreground(lipgloss.Color("15")) | ||
| errorNoteStyle = lipgloss.NewStyle().Foreground(lipgloss.Color("245")) | ||
| errorSummaryPrefix = lipgloss.NewStyle().Foreground(lipgloss.Color("245")) | ||
| errorActionArrowStyle = lipgloss.NewStyle().Foreground(lipgloss.Color("245")) | ||
| errorActionTextStyle = lipgloss.NewStyle().Foreground(lipgloss.Color("250")) | ||
| errorLinkStyle = lipgloss.NewStyle().Foreground(lipgloss.Color("245")).Bold(true) | ||
| errorK8sStyle = lipgloss.NewStyle().Foreground(lipgloss.Color("#A8C73A")).Bold(true) |
Member
There was a problem hiding this comment.
blocker: this is not reusing our existing styles
cmd/error_output.go
Outdated
Member
There was a problem hiding this comment.
blocker: the cmd package shouldn't have any styling logic. that's what the internal/ui package is for
Comment on lines
+15
to
+24
| func (f *fakeRuntime) PullImage(context.Context, string, chan<- PullProgress) error { return nil } | ||
| func (f *fakeRuntime) Start(context.Context, ContainerConfig) (string, error) { return "", nil } | ||
| func (f *fakeRuntime) Stop(context.Context, string) error { return nil } | ||
| func (f *fakeRuntime) Remove(context.Context, string) error { return nil } | ||
| func (f *fakeRuntime) IsRunning(context.Context, string) (bool, error) { return false, nil } | ||
| func (f *fakeRuntime) Logs(context.Context, string, int) (string, error) { return "", nil } | ||
| func (f *fakeRuntime) StreamLogs(context.Context, string, io.Writer) error { return nil } | ||
| func (f *fakeRuntime) GetImageVersion(context.Context, string) (string, error) { return "", nil } | ||
| func (f *fakeRuntime) CheckConnection(context.Context) error { return f.err } | ||
|
|
Member
There was a problem hiding this comment.
nit: it would be better to create a generated mock for this like we're doing in other tests
gtsiolis
force-pushed
the
george/des-114
branch
from
f993cf4 to
fe08870
Compare
Member
Author
|
Thanks for taking a look and sharing insights on how TUI works, @silv-io! Rebased and pushed an update. ⏳ |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmd/root.go (1)
26-35:rootCmd.RunduplicatesstartCmd.Runverbatim — extract a shared handler.Both
lstkandlstk startrun the same startup flow. Any future change to this logic (e.g., adding a flag or changing error handling) must be applied twice. Extracting a shared function eliminates the risk of the two commands drifting.♻️ Proposed refactor: extract shared handler
In
cmd/root.go(or a newcmd/start_handler.go):+func runStartCmd(cmd *cobra.Command, _ []string) { + rt, err := newRuntime(cmd.Context()) + if err != nil { + ui.ExitWithStartError(err) + } + if err := runStart(cmd.Context(), rt); err != nil { + ui.ExitWithStartError(err) + } +} + var rootCmd = &cobra.Command{ ... PreRunE: initConfig, - Run: func(cmd *cobra.Command, args []string) { - rt, err := newRuntime(cmd.Context()) - if err != nil { - ui.ExitWithStartError(err) - } - if err := runStart(cmd.Context(), rt); err != nil { - ui.ExitWithStartError(err) - } - }, + Run: runStartCmd, }In
cmd/start.go:var startCmd = &cobra.Command{ ... PreRunE: initConfig, - Run: func(cmd *cobra.Command, args []string) { - rt, err := newRuntime(cmd.Context()) - if err != nil { - ui.ExitWithStartError(err) - } - if err := runStart(cmd.Context(), rt); err != nil { - ui.ExitWithStartError(err) - } - }, + Run: runStartCmd, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/root.go` around lines 26 - 35, rootCmd.Run and startCmd.Run contain identical startup logic (calling newRuntime, handling errors with ui.ExitWithStartError, then runStart), so extract that sequence into a single shared function (e.g., startHandler(ctx context.Context) error or startHandler(ctx context.Context) that internally calls newRuntime and runStart and invokes ui.ExitWithStartError on failures) and have both rootCmd.Run and startCmd.Run delegate to that function; update calls to use the shared startHandler and keep references to newRuntime, runStart, and ui.ExitWithStartError so behavior remains identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/ui/error_output.go`:
- Around line 14-47: The fmt.Fprintln/Fprintf calls in ExitWithStartError,
printIndentedStyled and printActionLine ignore returned errors (errcheck
failure); update each call to capture the returned error and handle it (e.g. if
wErr := fmt.Fprintln(...); wErr != nil { log.Printf("failed writing error
output: %v", wErr) } ), and do the same inside the loop in printIndentedStyled
and for the multi-part print in printActionLine so every write error is checked
and logged (use log.Printf or similar non-panicking reporting).
In `@internal/ui/styles/styles.go`:
- Around line 31-50: Staticcheck warns that Style.Copy() is deprecated because
lipgloss.Style is a value type; remove the unnecessary .Copy() calls and use the
base styles directly when building the error styles. Replace expressions like
ErrorTitle = Title.Copy().Foreground(...) with ErrorTitle =
Title.Foreground(...), ErrorBody = Message.Foreground(...), ErrorActionText =
Version.Foreground(...), ErrorLink = Message.Bold(true) (or Message.Copy
removed), and ErrorK8s = NimboMid.Bold(true), keeping the same chained setters
and assignments for ErrorNote, ErrorSummaryPrefix, and ErrorActionArrow which
should remain as Message (no Copy).
---
Nitpick comments:
In `@cmd/root.go`:
- Around line 26-35: rootCmd.Run and startCmd.Run contain identical startup
logic (calling newRuntime, handling errors with ui.ExitWithStartError, then
runStart), so extract that sequence into a single shared function (e.g.,
startHandler(ctx context.Context) error or startHandler(ctx context.Context)
that internally calls newRuntime and runStart and invokes ui.ExitWithStartError
on failures) and have both rootCmd.Run and startCmd.Run delegate to that
function; update calls to use the shared startHandler and keep references to
newRuntime, runStart, and ui.ExitWithStartError so behavior remains identical.
ℹ️ Review info
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
cmd/logs.gocmd/root.gocmd/runtime.gocmd/start.gocmd/stop.gointernal/container/errors.gointernal/container/errors_test.gointernal/container/start.gointernal/runtime/docker.gointernal/runtime/engine_check.gointernal/runtime/engine_check_test.gointernal/runtime/mock_runtime_test.gointernal/runtime/runtime.gointernal/ui/error_output.gointernal/ui/error_output_test.gointernal/ui/styles/styles.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/runtime/docker.go
- cmd/stop.go
- internal/container/errors_test.go
- internal/runtime/engine_check_test.go
- cmd/logs.go
silv-io
force-pushed
the
george/des-114
branch
from
9a75849 to
9600976
Compare
Member
Looks like this now |
silv-io
force-pushed
the
george/des-114
branch
from
9600976 to
7d6cf7b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/ui/components/message.go (1)
8-19: Consider adding explicitSeverityInfocase for clarity.The function handles
SeveritySuccess,SeverityNote, andSeverityWarningexplicitly butSeverityInfofalls through to the default case. While the current behavior (rendering plain text) may be intentional, an explicit case would make the intent clearer and prevent accidental changes if new severities are added.♻️ Optional: Add explicit SeverityInfo case
func RenderMessage(e output.MessageEvent) string { prefix := styles.Secondary.Render("> ") switch e.Severity { + case output.SeverityInfo: + return styles.Message.Render(e.Text) case output.SeveritySuccess: return prefix + styles.Success.Render("Success:") + " " + styles.Message.Render(e.Text) case output.SeverityNote: return prefix + styles.Note.Render("Note:") + " " + styles.Message.Render(e.Text) case output.SeverityWarning: return prefix + styles.Warning.Render("Warning:") + " " + styles.Message.Render(e.Text) default: return styles.Message.Render(e.Text) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/components/message.go` around lines 8 - 19, RenderMessage currently falls through for output.SeverityInfo; add an explicit case for output.SeverityInfo in the RenderMessage function to make the intent clear—implement the case to return the same rendering as the default (use the existing prefix variable and styles.Message.Render(e.Text)) so behavior doesn't change but the severity is handled explicitly; reference output.SeverityInfo, RenderMessage, prefix, and styles.Message to locate where to add the case.internal/ui/app.go (1)
99-102: Avoid re-scheduling spinner ticks when already active.Line 101 always returns a new tick command for
Active: true. If multiple start events arrive, you can end up with overlapping tick chains.Refactor suggestion
case output.SpinnerEvent: - if msg.Active { - a.spinner = a.spinner.Start(msg.Text) - return a, a.spinner.Tick() + if msg.Active { + wasVisible := a.spinner.Visible() + a.spinner = a.spinner.Start(msg.Text) + if !wasVisible { + return a, a.spinner.Tick() + } + return a, nil } a.spinner = a.spinner.Stop() return a, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/app.go` around lines 99 - 102, The current branch always schedules a new tick when msg.Active is true which can create overlapping tick chains; change the handler so that when msg.Active is true you start or update a.spinner with msg.Text but only return a tick command (a.spinner.Tick()) if the spinner was previously inactive—otherwise update the spinner text/state without scheduling a new tick. Use the spinner's existing active/state flag (e.g., a.spinner.Active or equivalent) to detect prior activity and avoid re-scheduling Tick() on already-active spinners.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/ui/app.go`:
- Around line 99-102: The current branch always schedules a new tick when
msg.Active is true which can create overlapping tick chains; change the handler
so that when msg.Active is true you start or update a.spinner with msg.Text but
only return a tick command (a.spinner.Tick()) if the spinner was previously
inactive—otherwise update the spinner text/state without scheduling a new tick.
Use the spinner's existing active/state flag (e.g., a.spinner.Active or
equivalent) to detect prior activity and avoid re-scheduling Tick() on
already-active spinners.
In `@internal/ui/components/message.go`:
- Around line 8-19: RenderMessage currently falls through for
output.SeverityInfo; add an explicit case for output.SeverityInfo in the
RenderMessage function to make the intent clear—implement the case to return the
same rendering as the default (use the existing prefix variable and
styles.Message.Render(e.Text)) so behavior doesn't change but the severity is
handled explicitly; reference output.SeverityInfo, RenderMessage, prefix, and
styles.Message to locate where to add the case.
ℹ️ Review info
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (25)
cmd/root.gocmd/start.gogo.modinternal/auth/auth.gointernal/auth/auth_test.gointernal/auth/login.gointernal/container/start.gointernal/container/start_test.gointernal/output/errors.gointernal/output/events.gointernal/output/format.gointernal/output/format_test.gointernal/output/plain_sink_test.gointernal/output/tui_sink_test.gointernal/runtime/docker.gointernal/runtime/mock_runtime.gointernal/runtime/runtime.gointernal/ui/app.gointernal/ui/app_test.gointernal/ui/components/error_display.gointernal/ui/components/error_display_test.gointernal/ui/components/message.gointernal/ui/components/spinner.gointernal/ui/components/spinner_test.gointernal/ui/styles/styles.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/root.go
Member
Author
gtsiolis
commented
Feb 27, 2026
internal/container/start.go
Outdated
Comment on lines
+25
to
+31
| switch stdruntime.GOOS { | ||
| case "darwin": | ||
| actions = append([]output.ErrorAction{{Label: "Start Docker Desktop:", Value: "open -a Docker"}}, actions...) | ||
| case "linux": | ||
| actions = append([]output.ErrorAction{{Label: "Start Docker:", Value: "sudo systemctl start docker"}}, actions...) | ||
| case "windows": | ||
| actions = append([]output.ErrorAction{{Label: "Start Docker Desktop:", Value: "Start-Process 'Docker Desktop'"}}, actions...) |
Member
Author
There was a problem hiding this comment.
praise: Nice! I was gonna comment to hold back the macOS-specific command but this looks smooth. 💯
Member
There was a problem hiding this comment.
I feel thinking about these things can be a big differentiator and improvement for the new CLI. Good that doing this is so clean and easy to do in Go :)
silv-io
force-pushed
the
george/des-114
branch
from
7d6cf7b to
d45101f
Compare
Co-Authored-By: George Tsiolis <gtsiolis@users.noreply.github.com> # Conflicts: # internal/output/events.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/container/start.go`:
- Around line 22-37: The Docker-only remediation in the error event emitted by
output.EmitError (building an output.ErrorEvent with Title "Docker is not
available" and Summary err.Error()) must be extended to include Podman and
Kubernetes recovery actions and use user-facing term "emulator" instead of
"container"/"runtime"; update the actions slice (currently created and
conditionally prepended based on stdruntime.GOOS) to also append Podman actions
(e.g., install/start links/commands) and Kubernetes actions (e.g., links to
minikube/k3s and start commands) so users see actionable alternatives, and
change the Title/Summary/Action labels to reference "emulator" where visible to
users while keeping internal identifiers unchanged; locate this logic around the
actions := []output.ErrorAction{...}, the switch on stdruntime.GOOS, and the
output.EmitError call to make the edits.
ℹ️ Review info
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
cmd/root.gocmd/start.gointernal/container/start.gointernal/container/start_test.gointernal/output/errors.gointernal/output/events.gointernal/runtime/docker.gointernal/runtime/mock_runtime.gointernal/runtime/runtime.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/runtime/docker.go
- internal/container/start_test.go
- internal/output/errors.go
- internal/runtime/runtime.go
silv-io
approved these changes
Mar 2, 2026
Member
silv-io
left a comment
silv-io
left a comment
There was a problem hiding this comment.
looks good from my side now. Maybe @carole-lavillonniere can have a short look as well and then we can merge it.
carole-lavillonniere
approved these changes
Mar 2, 2026
Collaborator
carole-lavillonniere
left a comment
carole-lavillonniere
left a comment
There was a problem hiding this comment.
Looks good, just a small improvement suggestion
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
3 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.
Here's an attempt to resolve DES-114 and DRG-538. Early draft having fun with CLI UX, anything missing?