Migrate stop command to output event system#71
Conversation
gtsiolis
requested review from
anisaoshafi,
carole-lavillonniere and
silv-io
as code owners
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaces callback-based progress handling with an output.Sink, updates Changes
Sequence DiagramsequenceDiagram
participant User as User/CLI
participant Cmd as cmd/stop
participant UI as ui.RunStop
participant Container as internal/container.Stop
participant Sink as output.Sink
User->>Cmd: invoke stop command
alt UI is interactive
Cmd->>UI: RunStop(ctx, rt)
UI->>UI: init Bubble Tea app
UI->>Container: Stop(ctx, rt, tuiSink) (background)
Container->>Sink: EmitSpinnerStart
Container->>Container: rt.IsRunning?
alt running
Container->>Container: stop container
Container->>Sink: EmitSpinnerStop
Container->>Sink: EmitSuccess
Container-->>UI: status updates
else not running / error
Container->>Sink: EmitSpinnerStop
Container-->>UI: error status
end
UI->>User: render interactive progress/results
else non-interactive
Cmd->>Container: Stop(ctx, rt, plainSink)
Container->>Sink: EmitSpinnerStart/Stop and messages (stdout)
Container-->>Cmd: return result
end
Cmd-->>User: exit
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 |
gtsiolis
force-pushed
the
george/compact-stop-command-output
branch
from
4e1eda3 to
5bbde72
Compare
![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
🤖 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/stop.go`:
- Around line 43-45: The init() function in cmd/stop.go attempts to call
rootCmd.AddCommand(newStopCmd()) but rootCmd is undefined in this package;
replace the incorrect registration by adding the stop command to the actual root
command symbol used in this module (or restore the previous registration path).
Locate the package's real root command (e.g., a symbol named RootCmd,
GetRootCmd(), or the central command initializer) and change init() to call that
symbol's AddCommand with newStopCmd(), or alternatively export newStopCmd() and
register it where the real root is constructed; ensure init() references the
correct identifier instead of rootCmd so the package compiles.
In `@internal/container/stop.go`:
- Around line 20-23: The current check `err != nil || !running` in the IsRunning
call masks real runtime errors by always returning "%s is not running"; change
it to handle the two outcomes separately: if `err != nil` return the underlying
error (wrapped for context, e.g. fmt.Errorf("checking %s running: %w", name,
err)), and only if `err == nil && !running` return fmt.Errorf("%s is not
running", name). Locate the call to `rt.IsRunning(ctx, name)` in stop.go and
update the conditional to preserve and wrap the original error instead of
collapsing both cases.
In `@internal/ui/run_stop.go`:
- Around line 15-16: The derived cancelable context created by
context.WithCancel(parentCtx) is never used; replace uses of parentCtx with the
derived ctx (the variable returned alongside cancel) so UI-triggered cancel will
propagate to container.Stop and any background waiter; specifically, pass the
derived ctx into container.Stop (instead of parentCtx) and into the goroutine or
wait logic that waits on the done channel (the code around the container.Stop
call and the later wait at the done/wait block), keep defer cancel() to ensure
cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 5a2e2128-38e6-48fa-99a3-bb10e9fbcc19
📒 Files selected for processing (3)
cmd/stop.gointernal/container/stop.gointernal/ui/run_stop.go
gtsiolis
force-pushed
the
george/compact-stop-command-output
branch
from
5bbde72 to
2be6f47
Compare
|
@coderabbitai left some review comments I appended in 2be6f47. @carole-lavillonniere Could you take another look when you're free? |
|
✅ Actions performedFull review triggered. |
|
The PR description images show a clear before/after comparison of the
✅ Actions performedFull review triggered. |
🧩 Analysis chain🏁 Script executed: Length of output: 25690 Let me check the current state of 🏁 Script executed: Length of output: 953 🏁 Script executed: Length of output: 1960
I'll trigger a full review on the updated changes now. ✅ Actions performedFull review triggered. |
gtsiolis
force-pushed
the
george/compact-stop-command-output
branch
from
2be6f47 to
52119a0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/container/stop.go (1)
18-26: Early return on first non-running container may leave other containers running.When iterating multiple containers, if one is not running, the function returns immediately without attempting to stop subsequent containers. This could leave the system in a partial state. Consider whether you want to:
- Continue stopping remaining containers and collect errors, or
- Change to a warning/skip pattern for containers that aren't running
If the current behavior is intentional (fail-fast), this is fine as-is.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/container/stop.go` around lines 18 - 26, The loop over cfg.Containers returns immediately if any container is not running, which leaves later containers unprocessed; modify the logic in the loop that calls rt.IsRunning(ctx, name) (and uses name := c.Name()) to either skip non-running containers with a warning (e.g., log via provided logger) or, preferably, continue processing all containers while collecting errors into a slice (or errors.Join) so you attempt to stop every container and then return a combined error at the end; update the early return paths that check running state to append to the error collection and continue, and after the loop return a single aggregated error (or nil) instead of failing fast.internal/ui/run_stop.go (1)
22-30: Consider usingdeferfor channel send consistency withrun.go.The error handling works correctly, but differs from the pattern in
internal/ui/run.gowhich usesdeferto ensure the channel send always happens. While not a bug, aligning with the existing pattern improves maintainability.♻️ Optional: Align with run.go pattern
go func() { + var err error + defer func() { runErrCh <- err }() err := container.Stop(ctx, rt, output.NewTUISink(programSender{p: p})) - runErrCh <- err if err != nil && !errors.Is(err, context.Canceled) { p.Send(runErrMsg{err: err}) return } p.Send(runDoneMsg{}) }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/run_stop.go` around lines 22 - 30, The goroutine currently sends runErrCh and p.Send conditionally; change it to follow the run.go pattern by using defer to always send the error to runErrCh: call err := container.Stop(ctx, rt, output.NewTUISink(programSender{p: p})), then immediately defer sending err into runErrCh (defer func(){ runErrCh <- err }()), after which check if err != nil && !errors.Is(err, context.Canceled) to p.Send(runErrMsg{err: err}) and return, otherwise p.Send(runDoneMsg{}); use the existing symbols container.Stop, programSender, runErrCh, runErrMsg, runDoneMsg, and p.Send to locate and update the code.
🤖 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/run_stop.go`:
- Around line 18-19: The stop UI currently constructs app := NewApp("", "", "",
cancel) which leaves blank header lines; mirror RunLogout by suppressing the
header or supply real version/emulator/endpoint values—specifically, change the
creation to call withoutHeader() on the app (e.g., NewApp("", "", "",
cancel).withoutHeader()) so the header is hidden consistently with RunLogout;
alternatively, populate the version, emulatorName, and endpoint arguments
instead of empty strings if you want the header shown.
---
Nitpick comments:
In `@internal/container/stop.go`:
- Around line 18-26: The loop over cfg.Containers returns immediately if any
container is not running, which leaves later containers unprocessed; modify the
logic in the loop that calls rt.IsRunning(ctx, name) (and uses name := c.Name())
to either skip non-running containers with a warning (e.g., log via provided
logger) or, preferably, continue processing all containers while collecting
errors into a slice (or errors.Join) so you attempt to stop every container and
then return a combined error at the end; update the early return paths that
check running state to append to the error collection and continue, and after
the loop return a single aggregated error (or nil) instead of failing fast.
In `@internal/ui/run_stop.go`:
- Around line 22-30: The goroutine currently sends runErrCh and p.Send
conditionally; change it to follow the run.go pattern by using defer to always
send the error to runErrCh: call err := container.Stop(ctx, rt,
output.NewTUISink(programSender{p: p})), then immediately defer sending err into
runErrCh (defer func(){ runErrCh <- err }()), after which check if err != nil &&
!errors.Is(err, context.Canceled) to p.Send(runErrMsg{err: err}) and return,
otherwise p.Send(runDoneMsg{}); use the existing symbols container.Stop,
programSender, runErrCh, runErrMsg, runDoneMsg, and p.Send to locate and update
the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 8926c43e-ada0-4e38-a780-b1955e0c94d3
📒 Files selected for processing (3)
cmd/stop.gointernal/container/stop.gointernal/ui/run_stop.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/stop.go
2 participants
See DES-152 for more context.
** header can be hidden once #69 is merged.