refactor(cli): unify CLI presentation and redesign images prune#76
refactor(cli): unify CLI presentation and redesign images prune#76
Conversation
Route root, backup, and images command output through shared presentation helpers and add UI adoption guardrails to prevent regressions in styling usage.
Detect forbidden fmt Fprint writes to stdout paths, switch matrix coverage to AST-based function detection, and deduplicate empty-state rendering helper.
Use the shared table component with fixed column widths and grapheme-safe truncation so long repositories, tags, and image IDs stay readable without breaking row shape.
Make 'gordon images prune' run both dangling-runtime and registry cleanup by default, keeping latest + 3 previous release tags per repository. Add --dangling and --registry scope flags to restrict which subsystems run, --keep-releases to control tag retention, --no-confirm to skip the interactive confirmation prompt, and reuse the existing RunConfirm component for the destructive-operation gate. The options contract flows CLI -> remote client -> admin handler -> usecase service via domain.ImagePruneOptions, with the handler and scheduled job both defaulting to both-scopes-enabled.
📝 WalkthroughWalkthroughReplaces scalar image prune parameter with ImagePruneOptions (KeepLast, PruneDangling, PruneRegistry); updates CLI to build/send new request shape, adds CLI presentation helpers and table truncation, adapts HTTP client/handler, domain, usecase, boundaries, and extensive tests to the options-based pruning API. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as CLI
participant HTTP as HTTP Admin
participant Service as Image Service
participant Runtime as Runtime Pruner
participant Registry as Registry Pruner
User->>CLI: gordon images prune --dangling --keep-releases=3
CLI->>CLI: build ImagePruneRequest / ImagePruneOptions
CLI->>HTTP: POST /admin/images/prune (request)
HTTP->>Service: Prune(ctx, opts)
alt PruneDangling enabled
Service->>Runtime: prune runtime images
Runtime-->>Service: runtime report
else
Note over Service: skip runtime pruning
end
alt PruneRegistry enabled
Service->>Registry: prune registry tags (keepLast)
Registry-->>Service: registry report
else
Note over Service: skip registry pruning
end
Service-->>HTTP: ImagePruneReport
HTTP-->>CLI: response
CLI->>User: render results (title, table, summary)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Comment |
There was a problem hiding this comment.
Pull request overview
This PR refactors the CLI to use a shared presentation layer (Lipgloss-styled helpers), redesigns gordon images list to render a fixed-width, grapheme-safe table, and changes gordon images prune to support scoped pruning (dangling/runtime vs registry) with new defaults and flags. It also threads new prune options through the DTO → remote client → HTTP admin handler → domain/usecase layers and adds AST-based guardrail tests to prevent UI regression.
Changes:
- Introduces
presentation.gohelpers and migrates multiple commands away from rawfmt.Print*to consistent styled output. - Reworks
images listoutput using a table component with fixed column widths + grapheme-safe truncation. - Redesigns
images pruneoptions/flags (default both scopes,--keep-releases,--dangling/--registry, confirmation flow) and adds broad test coverage + UI adoption guardrails.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/usecase/images/service.go | Updates prune entrypoint to accept domain.ImagePruneOptions and conditionally run runtime/registry prunes. |
| internal/usecase/images/service_test.go | Extends prune tests to cover dangling-only and registry-only scope behavior. |
| internal/domain/images.go | Introduces ImagePruneOptions + defaults helper. |
| internal/boundaries/in/images.go | Updates ImageService port signature to use prune options object. |
| internal/app/run.go | Updates scheduled prune to call service with explicit prune options. |
| internal/adapters/in/http/admin/images.go | Parses new DTO scope fields, applies defaults, rejects both-scopes-disabled, forwards options to service. |
| internal/adapters/in/http/admin/handler_test.go | Updates stubs and adds tests for scope defaulting/validation. |
| internal/adapters/dto/admin_images.go | Extends prune DTO with optional *bool scope fields. |
| internal/adapters/in/cli/presentation.go | Adds shared CLI write/render helpers (testable seams). |
| internal/adapters/in/cli/images.go | Implements new images list table rendering and redesigned images prune flag/flow. |
| internal/adapters/in/cli/images_test.go | Adds coverage for scope resolution, confirmation flow, retention semantics, and table truncation behaviors. |
| internal/adapters/in/cli/remote/client.go | Updates remote client prune API to accept ImagePruneRequest. |
| internal/adapters/in/cli/remote/client_test.go | Updates remote client tests to validate new request fields. |
| internal/adapters/in/cli/ui/components/table.go | Adds fixed-width rendering + truncation (grapheme-safe) to the shared table component. |
| internal/adapters/in/cli/ui/components/table_test.go | Adds unit tests for truncation and width behavior. |
| internal/adapters/in/cli/root.go | Migrates version/log output to presentation helpers. |
| internal/adapters/in/cli/serve.go | Migrates deprecated start warning to presentation helpers. |
| internal/adapters/in/cli/backup.go | Adds consistent titles/empty-states via presentation helpers. |
| internal/adapters/in/cli/ui_adoption_test.go | Adds AST guardrails + seam tests ensuring commands adopt presentation helpers. |
| internal/adapters/in/cli/parity_matrix_test.go | Adds expectation matrix + coverage test for UI adoption guardrails. |
| docs/config/images.md | Documents CLI defaults aligning with scheduled prune defaults. |
| docs/cli/images.md | Updates CLI docs for new prune flags, defaults, scope resolution, and retention semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| cmd.Flags().IntVar(&opts.KeepReleases, "keep-releases", domain.DefaultImagePruneKeepLast, | ||
| "Number of previous non-latest tags to keep per repository (latest is always kept)") | ||
| cmd.Flags().BoolVar(&opts.Dangling, "dangling", false, "Prune dangling runtime images only") | ||
| cmd.Flags().BoolVar(&opts.Registry, "registry", false, "Prune old registry tags only") | ||
| cmd.Flags().BoolVar(&opts.NoConfirm, "no-confirm", false, "Skip confirmation prompt") |
There was a problem hiding this comment.
The --dangling/--registry flag help strings say "only", but both flags can be combined (and combining them means both scopes run). Consider rewording these descriptions to reflect that they restrict scope when used alone (or that they enable each scope) to avoid confusing CLI users.
internal/adapters/in/cli/images.go
Outdated
| func runImagesPruneDryRun(ctx context.Context, client imagesClient, opts imagesPruneOptions, pruneDangling, pruneRegistry bool, out io.Writer) error { | ||
| images, err := client.ListImages(ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to prune images: %w", err) | ||
| return fmt.Errorf("failed to list images: %w", err) | ||
| } |
There was a problem hiding this comment.
In dry-run mode, runImagesPruneDryRun always calls client.ListImages even when runtime pruning is disabled (registry-only scope). For remote mode this is an unnecessary network call and can be expensive on large registries. Consider only listing images when pruneDangling is true (or when you actually need the dangling count).
| if keepLast < 0 { | ||
| func (c *Client) PruneImages(ctx context.Context, req dto.ImagePruneRequest) (*dto.ImagePruneResponse, error) { | ||
| if req.KeepLast != nil && *req.KeepLast < 0 { | ||
| return nil, fmt.Errorf("keepLast must be >= 0") |
There was a problem hiding this comment.
The client-side validation error message uses "keepLast" (camelCase) rather than the API field name (keep_last) or CLI flag name (--keep-releases). Since this can surface to end users, consider aligning the message with the public contract (e.g., keep_last must be >= 0 or --keep-releases must be >= 0).
| return nil, fmt.Errorf("keepLast must be >= 0") | |
| return nil, fmt.Errorf("--keep-releases must be >= 0") |
| applyWidth := func(s lipgloss.Style) lipgloss.Style { | ||
| if width > 0 { | ||
| return s.Width(width).MaxWidth(width) | ||
| } | ||
| return s | ||
| } |
There was a problem hiding this comment.
In TableModel.Render, truncateCell uses the raw column width, but applyWidth later sets a Lipgloss style width that may also include the default left/right padding (NewTable sets Padding(0, 1)). This can cause cells that were truncated to col.Width (ending with ...) to be clipped again at render time, potentially cutting off the ellipsis and breaking the intended fixed-width behavior. Consider truncating to the available content width (column width minus horizontal padding) or adjusting the style width to account for padding so the rendered cell width matches the truncation width.
| func truncateCell(value string, maxWidth int) string { | ||
| if strings.Contains(value, "\x1b[") { | ||
| return value | ||
| } |
There was a problem hiding this comment.
truncateCell currently returns the original value unchanged whenever it contains an ANSI escape sequence ("\x1b["). This avoids breaking ANSI codes, but it also means long styled cell values can exceed the configured column width and defeat the fixed-width table layout. Consider using an ANSI-aware truncation approach (measure/trim printable width while preserving escape sequences) so styled cells remain bounded.
| if opts.PruneDangling { | ||
| runtimeReport, err := s.PruneRuntime(ctx) | ||
| if err != nil { | ||
| log.Warn().Err(err).Msg("runtime prune failed; continuing with registry prune") |
There was a problem hiding this comment.
The warning log message says "continuing with registry prune" even when opts.PruneRegistry is false (dangling-only mode). This makes logs misleading for scoped prunes. Consider changing the message to be scope-agnostic or conditional on opts.PruneRegistry.
| log.Warn().Err(err).Msg("runtime prune failed; continuing with registry prune") | |
| msg := "runtime prune failed" | |
| if opts.PruneRegistry { | |
| msg += "; continuing with registry prune" | |
| } else { | |
| msg += "; continuing" | |
| } | |
| log.Warn().Err(err).Msg(msg) |
- Reword --dangling/--registry help to clarify they restrict scope, not exclusive - Skip ListImages call in dry-run when pruneDangling is false (registry-only) - Use API field name keep_last in remote client validation error - Make service log message conditional on PruneRegistry when runtime prune fails
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/usecase/images/service.go`:
- Around line 334-366: In Prune, when calling s.PruneRuntime(ctx) inside the
Prune method, do not swallow the error if opts.PruneRegistry is false: if
PruneRuntime returns an error and opts.PruneRegistry is false, return that error
(along with the partially built report) instead of only logging it; if
opts.PruneRegistry is true keep the current behavior of logging the runtime
error and continuing to call s.PruneRegistry(ctx, opts.KeepLast). Ensure
references to the symbols Prune, PruneRuntime, PruneRegistry, opts, and report
are used to locate and adjust the control flow accordingly.
When PruneRegistry is false, return the PruneRuntime error to the caller instead of swallowing it into a warning log. Both-scopes mode keeps the existing behavior of logging and continuing to registry cleanup.
1 participant
Summary
presentation.gowith styled helpers (cliWriteLine,cliRenderTitle,cliRenderMuted, etc.) and migrateroot,backup,serve, andimagescommands from rawfmt.Print*to consistent Lipgloss-styled output.images listwith fixed-width columns and grapheme-safe truncation so long repositories, tags, and IDs stay readable.images prunewith no flags now prunes both dangling runtime images and registry tags (default retention:latest+ 3 previous non-latest tags). New flags:--dangling/--registryto scope,--keep-releasesfor retention control,--no-confirmto skip the interactive confirmation prompt. Reuses the existingRunConfirmBubble Tea component.ui_adoption_test.go,parity_matrix_test.go) enforce that CLI commands use presentation helpers, preventing regressions.Options contract
The prune options flow through all layers:
The DTO uses
*booloptional fields so the handler distinguishes "not provided" (both default true) from explicit false. The handler rejects both-false with HTTP 400.Breaking changes
images prune --keepis replaced by--keep-releases(default 3, aligned with scheduled job / admin API defaults).images prunewith no args now actually prunes registry tags (previously it only ran dangling cleanup because--keepdefaulted to 0).Test coverage
Files changed
22 files, +1400 / -151 lines across CLI, remote client, admin handler, domain, usecase, and docs.
Summary by CodeRabbit
New Features
gordon images pruneadds flags: --keep-releases, --dangling, --registry, --no-confirm; latest tag always preserved; default keeps latest + 3 releases and enables both runtime and registry cleanup.Behavior
UI/UX
Documentation