Merged
Conversation
- Add pruneWithSpinner that shows progress during image cleanup - Test for idempotent prune: running prune twice should not delete additional items or error - Spinner message adapts based on which scopes are enabled (runtime, registry, or both) - Handles missing child manifests gracefully (log warning, continue) Fixes #74
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds an interactive spinner-backed CLI prune flow that runs image pruning in a background goroutine, extracts registry tag-selection/counting into helper functions for prune estimation, and adds a test asserting prune idempotency (the test was inserted twice). Changes
Sequence DiagramsequenceDiagram
participant User as "User / CLI"
participant CLI as "pruneWithSpinner()"
participant Spinner as "Spinner UI (interactive)"
participant Worker as "Background Goroutine"
participant Client as "Client.PruneImages()"
User->>CLI: invoke pruneWithSpinner(ctx, client, req, ...)
alt interactive terminal
CLI->>Spinner: start spinner with scope-aware message
CLI->>Worker: spawn goroutine to run PruneImages(req)
Worker->>Client: call PruneImages(ctx, req)
Client-->>Worker: return ImagePruneResponse / error
Worker-->>CLI: send result on channel
loop spinner render loop
Spinner->>Spinner: animate frames
end
CLI->>Spinner: render final success frame and stop
else non-interactive terminal
CLI->>Client: call PruneImages(ctx, req) directly
Client-->>CLI: return ImagePruneResponse / error
end
CLI-->>User: return prune result / error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
…tic complexity Break down complex function into smaller helper functions: - collectTagsByRepository: Group tags by repo - countTagsToRemove: Count tags that would be removed - toSortedTagInfos: Sort tags by creation time - selectKeptTags: Apply retention policy Fixes golangci-lint cyclomatic complexity > 15 warning.
![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
🤖 Fix all issues with AI agents
In `@internal/adapters/in/cli/images.go`:
- Around line 136-187: The spinner is being driven incorrectly by calling
model.Init() and printing its returned tea.Cmd; instead call the model's Update
and View methods each tick so the spinner advances and renders properly. In
pruneWithSpinner replace uses of model.Init() and printing that value with a
loop that on each 100ms tick invokes model.Update(spinner.TickMsg{}) (importing
"github.com/charmbracelet/bubbles/spinner") to get the updated model and any
cmd, then call model.View() and print that string; on success set the message
via model.SetMessage(cliRenderSuccess("Pruning complete")), perform a final
Update/View cycle to render the success frame, and only then return the
response. Ensure you still consume the done channel and handle errors as before.
In `@internal/usecase/images/service_test.go`:
- Around line 461-490: The test's idempotency check ignores runtime pruning
because the fake runtime (rt of type fakeRuntime) always returns a non‑zero
pkgruntime.PruneReport; update the test so runtime doesn't mask registry
idempotency: either construct the Service without the runtime (pass nil or a
disabled runtime) when calling NewService, or reset rt.pruneReport to an empty
pkgruntime.PruneReport before the second svc.Prune call and assert
report2.Runtime (or report2.Runtime.DeletedIDs/SpaceReclaimed) is zero to ensure
the second prune truly removed nothing from the runtime.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/adapters/in/cli/images.go (1)
380-393:⚠️ Potential issue | 🟠 MajorBug:
keepReleases == 0case returns incorrect estimate.When
keepReleasesis 0, the user wants to keep only the "latest" tag and prune everything else. However, this early return causes the preview to report 0 tags to prune, which is incorrect.The condition should be
keepReleases < 0(unreachable due to CLI validation) or removed entirely. The helper functions already handlekeepReleases=0correctly —selectKeptTagswill keep only "latest" whenkeepReleases=0.🐛 Proposed fix
func estimateRegistryTagsToPrune(images []dto.Image, keepReleases int) int { - if keepReleases <= 0 { - return 0 - } - tagsByRepo := collectTagsByRepository(images) removed := 0
- Fix estimateRegistryTagsToPrune: remove incorrect early return for keepReleases=0
When keepReleases is 0, user wants to keep only 'latest' tag, but early
return caused preview to report 0 tags to prune incorrectly.
- Fix pruneWithSpinner: properly drive spinner animation
Use model.Update(spinner.TickMsg{}) and View() methods instead of
calling Init() repeatedly.
- Fix test idempotency: reset runtime pruneReport between prunes
Ensures second prune truly tests idempotency by asserting zero
runtime results after clearing fake runtime's state.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/adapters/in/cli/images.go`:
- Around line 168-191: The spinner never advances because the returned model
from model.Update(spinner.TickMsg{}) isn't reassigned and each frame prints a
new line; fix by reassigning the updated model (e.g., model = updatedModel)
after calling model.Update(spinner.TickMsg{}) everywhere in the loop (both
timeout case and success branch where spinner.TickMsg{} is used), and change
fmt.Print(m.View()) to print with a carriage return (e.g., prepend or append
"\r") so frames overwrite the same line; ensure the same reassignment/CR
behavior is applied in the success path where cliRenderSuccess("Pruning
complete") is set before calling Update.
Comment on lines
168
to
191
| for { | ||
| select { | ||
| case result := <-done: | ||
| if result.err != nil { | ||
| return nil, result.err | ||
| } | ||
| if result.resp == nil { | ||
| return nil, fmt.Errorf("prune operation completed but no response received") | ||
| } | ||
| // Success - set final message and render | ||
| model.SetMessage(cliRenderSuccess("Pruning complete")) | ||
| updatedModel, _ := model.Update(spinner.TickMsg{}) | ||
| if m, ok := updatedModel.(components.SpinnerModel); ok { | ||
| fmt.Print(m.View()) | ||
| } | ||
| return result.resp, nil | ||
| case <-time.After(100 * time.Millisecond): | ||
| updatedModel, _ := model.Update(spinner.TickMsg{}) | ||
| if m, ok := updatedModel.(components.SpinnerModel); ok { | ||
| fmt.Print(m.View()) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Spinner model state is not preserved between iterations, breaking animation.
The Update method returns a new model with updated spinner state, but it's never reassigned to model. Each iteration calls Update on the same initial state, so the spinner frame never advances. Additionally, without a carriage return (\r), each frame prints on a new line instead of overwriting.
🔧 Proposed fix
for {
select {
case result := <-done:
if result.err != nil {
return nil, result.err
}
if result.resp == nil {
return nil, fmt.Errorf("prune operation completed but no response received")
}
// Success - set final message and render
model.SetMessage(cliRenderSuccess("Pruning complete"))
updatedModel, _ := model.Update(spinner.TickMsg{})
if m, ok := updatedModel.(components.SpinnerModel); ok {
- fmt.Print(m.View())
+ fmt.Print("\r" + m.View() + "\n")
}
return result.resp, nil
case <-time.After(100 * time.Millisecond):
updatedModel, _ := model.Update(spinner.TickMsg{})
if m, ok := updatedModel.(components.SpinnerModel); ok {
- fmt.Print(m.View())
+ model = m
+ fmt.Print("\r" + m.View())
}
}
}📝 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 { | |
| select { | |
| case result := <-done: | |
| if result.err != nil { | |
| return nil, result.err | |
| } | |
| if result.resp == nil { | |
| return nil, fmt.Errorf("prune operation completed but no response received") | |
| } | |
| // Success - set final message and render | |
| model.SetMessage(cliRenderSuccess("Pruning complete")) | |
| updatedModel, _ := model.Update(spinner.TickMsg{}) | |
| if m, ok := updatedModel.(components.SpinnerModel); ok { | |
| fmt.Print(m.View()) | |
| } | |
| return result.resp, nil | |
| case <-time.After(100 * time.Millisecond): | |
| updatedModel, _ := model.Update(spinner.TickMsg{}) | |
| if m, ok := updatedModel.(components.SpinnerModel); ok { | |
| fmt.Print(m.View()) | |
| } | |
| } | |
| } | |
| } | |
| for { | |
| select { | |
| case result := <-done: | |
| if result.err != nil { | |
| return nil, result.err | |
| } | |
| if result.resp == nil { | |
| return nil, fmt.Errorf("prune operation completed but no response received") | |
| } | |
| // Success - set final message and render | |
| model.SetMessage(cliRenderSuccess("Pruning complete")) | |
| updatedModel, _ := model.Update(spinner.TickMsg{}) | |
| if m, ok := updatedModel.(components.SpinnerModel); ok { | |
| fmt.Print("\r" + m.View() + "\n") | |
| } | |
| return result.resp, nil | |
| case <-time.After(100 * time.Millisecond): | |
| updatedModel, _ := model.Update(spinner.TickMsg{}) | |
| if m, ok := updatedModel.(components.SpinnerModel); ok { | |
| model = m | |
| fmt.Print("\r" + m.View()) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@internal/adapters/in/cli/images.go` around lines 168 - 191, The spinner never
advances because the returned model from model.Update(spinner.TickMsg{}) isn't
reassigned and each frame prints a new line; fix by reassigning the updated
model (e.g., model = updatedModel) after calling model.Update(spinner.TickMsg{})
everywhere in the loop (both timeout case and success branch where
spinner.TickMsg{} is used), and change fmt.Print(m.View()) to print with a
carriage return (e.g., prepend or append "\r") so frames overwrite the same
line; ensure the same reassignment/CR behavior is applied in the success path
where cliRenderSuccess("Pruning complete") is set before calling Update.
- Reassign model after Update() to preserve spinner state between iterations - Add \r carriage return so frames overwrite instead of printing new lines - Add \n after final success message for clean line ending
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
1 participant
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.
Summary
Changes
pruneWithSpinner: Runs PruneImages in goroutine with Bubble Tea spinnerTestService_Prune_IdempotentMultiplePrunesDoNotAccumulate: Runs prune twice, verifies second is no-opTest Coverage
All existing Prune tests pass, plus new idempotent test.
Summary by CodeRabbit
New Features
Tests