Open
Conversation
gtsiolis
requested review from
carole-lavillonniere and
silv-io
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactors runtime initialization to a centralized context-aware helper, adds container engine pre-flight checks with a RuntimeUnavailableError, improves container name-conflict error formatting, and introduces structured startup error output and new UI styles. Changes
Sequence Diagram(s)sequenceDiagram
participant Cmd as CLI Command
participant Helper as newRuntime()
participant Runtime as DockerRuntime
participant Daemon as Docker Daemon
participant UI as UI Error Handler
Cmd->>Helper: newRuntime(ctx)
Helper->>Runtime: runtime.NewDockerRuntime()
Runtime-->>Helper: rt or error
alt creation error
Helper-->>Cmd: error
Cmd->>UI: ExitWithStartError(err)
UI-->>Cmd: Exit(1)
else created
Helper->>Runtime: CheckConnection(ctx) via CheckContainerEngine
Runtime->>Daemon: Ping()
alt ping fails
Daemon-->>Helper: connection error
Helper-->>Cmd: RuntimeUnavailableError
Cmd->>UI: ExitWithStartError(err)
UI-->>Cmd: Exit(1)
else ping ok
Daemon-->>Helper: success
Helper-->>Cmd: rt
Cmd->>Cmd: proceed with command logic
end
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
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.
Here's an attempt to resolve DES-114 and DRG-538. Early draft having fun with CLI UX, anything missing?