cli/cmd/transfer/component-version/progress_test.go (1)

23-33: This test no longer verifies the non-terminal visualizer path.

fmt.Sprintf("%T", tracker) will contain Tracker no matter which visualizer selectVisualizer installs, so a regression from simple to bar still passes here. Assert observable output or expose the configured visualizer through a test helper.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/cmd/transfer/component-version/progress_test.go` around lines 23 - 33,
The test currently only checks the tracker type which doesn't verify which
visualizer selectVisualizer installed; update the test to assert the actual
visualizer used by either (a) asserting observable output written to the
captured writer by exercising tracker (e.g., start progress and emit updates,
then check out for expected simple vs bar output patterns) or (b) add a
test-only accessor in the progress package (e.g., Tracker.Visualizer() or a
helper that returns the installed visualizer) and assert its concrete type
returned by selectVisualizer; locate uses of selectVisualizer and
progress.NewTracker to implement the chosen approach and update the assertions
accordingly.

cli/internal/render/progress/bar/animation_test.go (1)

3-9: Fail fast if RunAnimation never ticks.

<-called can block the package until the global go test timeout if the callback is never invoked. Give this test its own timeout so the failure is local and fast.

⏱️ Suggested change

 import (
 	"bytes"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 )
@@
-	<-called // wait for at least one callback
+	select {
+	case <-called:
+	case <-time.After(time.Second):
+		t.Fatal("RunAnimation never invoked onSpin")
+	}
 	close(done)

Also applies to: 69-79

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/internal/render/progress/bar/animation_test.go` around lines 3 - 9, The
test currently waits indefinitely on the callback channel (called) from
RunAnimation and can hang the whole test suite if the callback never fires;
update the test(s) that read from the called channel (the RunAnimation callback
tests in this file) to use a bounded wait (e.g. select on <-called and
time.After with a short timeout) and call t.Fatalf/t.Error on timeout so the
failure is local and fast; apply the same pattern to the other similar test(s)
in this file that wait on called.

cli/internal/render/progress/tracker.go (2)

116-121: Consider logging buffered content before restoring logger.

When Stop() is called, any content remaining in t.logBuffer is silently discarded. If Stop() is called after a phase ends (normal flow), the buffer should be empty. However, if an unexpected early exit occurs, buffered logs are lost.

Consider draining the buffer to the restored logger before nil-ing previousLogger:

♻️ Optional: drain buffer before restoring

 func (t *Tracker) Stop() {
 	if t.previousLogger != nil {
+		// Drain any remaining buffered logs
+		if t.logBuffer != nil && t.logBuffer.Len() > 0 {
+			_, _ = io.Copy(t.out, t.logBuffer)
+		}
 		slog.SetDefault(t.previousLogger)
 		t.previousLogger = nil
 	}
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/internal/render/progress/tracker.go` around lines 116 - 121, When Stop()
is called on Tracker it currently restores t.previousLogger and discards any
entries left in t.logBuffer; modify Stop to drain/flush t.logBuffer to the
restored logger before clearing t.previousLogger so buffered messages are not
lost on unexpected exits. Specifically, after calling
slog.SetDefault(t.previousLogger) but before setting t.previousLogger = nil,
iterate over or otherwise flush t.logBuffer (using the same log formatting used
by Tracker) and emit each buffered entry to the new default logger; finally
clear the buffer and then nil out t.previousLogger. Ensure you reference
Tracker.Stop, t.logBuffer, t.previousLogger and slog.SetDefault in the change.

---

92-109: Global slog manipulation requires careful Stop() discipline.

NewTracker unconditionally redirects the global slog.Default() logger. If Stop() is never called (e.g., panic, early return), all subsequent logging in the process goes to the buffer. The context snippet shows production code uses defer tracker.Stop() correctly, but consider:

1. Defensive nil check: If opts contains a nil function, the loop will panic.
2. Documentation emphasis: The docstring mentions calling Stop, but a // IMPORTANT callout or returning a cleanup function could reduce misuse risk.

These are minor given the existing defer pattern in cmd.go, but worth noting for maintainability.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/internal/render/progress/tracker.go` around lines 92 - 109, NewTracker
currently unconditionally calls each TrackerOption and swaps the global
slog.Default, which can panic if an option is nil and risks leaving the global
logger redirected if Stop() is not called; update NewTracker to skip nil options
when iterating over opts (check each TrackerOption for nil before calling), and
to reduce misuse return a cleanup function alongside the *Tracker (e.g., return
(*Tracker, func()) that calls t.Stop()) or document an explicit "// IMPORTANT:
call Stop() or use the returned cleanup" comment near NewTracker; ensure the
implementation still sets t.previousLogger, t.logBuffer and calls
slog.SetDefault(slog.New(...)) and that any injected LogBufferAware visualizer
logic (SetLogBuffer) remains intact.

cli/internal/render/progress/bar/bar.go (1)

338-356: renderFinalHeader clears and redraws, but logs/bar rendered without checking total > 0.

In simple mode (total == 0), End() calls clearLines() and writes completion/failure directly (lines 141-154), never reaching renderFinalHeader. However, if someone were to call renderFinalHeader in simple mode (currently doesn't happen), it would call renderLogs() and renderBar() which assume tracked mode.

This is fine given current control flow, but adding a guard would make the code more defensive:

♻️ Optional defensive guard

 func (v *barVisualizer) renderFinalHeader(err error) {
+	if v.total == 0 {
+		return // Simple mode handled separately in End()
+	}
 	hasFailures := false

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/internal/render/progress/bar/bar.go` around lines 338 - 356,
renderFinalHeader should avoid calling tracked-mode-only methods when in simple
mode: check the bar's total (v.total) before calling v.renderLogs() and
v.renderBar(). Inside renderFinalHeader (around the clearLines() call), add a
guard like "if v.total > 0 { v.renderLogs(); v.renderBar() }" so renderLogs and
renderBar are only invoked in tracked mode; keep the existing
WriteFailedLine/WriteCompletedLine behavior for simple mode. Ensure you
reference the v.total field and the methods renderLogs and renderBar when making
the change.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

cli/cmd/transfer/component-version/cmd.go (1)

255-257: ⚠️ Potential issue | 🟠 Major

Wrap the graph error instead of discarding it.

Line 257 returns a fixed string, so callers lose the original failure reason after phase.Fail(err) renders it. This flattens cancellations and validation errors into the same generic CLI error.

🧾 Suggested fix

 	if err := graph.Process(ctx); err != nil {
 		phase.Fail(err)
-		return fmt.Errorf("graph execution failed")
+		return fmt.Errorf("graph execution failed: %w", err)
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/cmd/transfer/component-version/cmd.go` around lines 255 - 257, The code
currently calls graph.Process(ctx) and calls phase.Fail(err) but then returns a
generic error string, losing the original error; change the return to wrap the
original error so callers see the real failure (e.g., return an error that
includes or wraps err) while still calling phase.Fail(err). Update the return
that follows graph.Process(ctx) to wrap err (reference graph.Process and
phase.Fail in the same block) so the original error is preserved and propagated.

cli/cmd/transfer/component-version/cmd.go (1)

259-260: Redundant tracker.Stop() call.

tracker.Stop() is already deferred on line 177. While Stop() is idempotent (it nil-checks previousLogger), this explicit call is unnecessary and adds noise. Consider removing it or removing the defer if you prefer explicit control.

♻️ Remove redundant call

 	phase.Complete()
-	tracker.Stop()
 
 	slog.DebugContext(ctx, "transfer completed successfully")

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/cmd/transfer/component-version/cmd.go` around lines 259 - 260, The
explicit tracker.Stop() call after phase.Complete() is redundant because
tracker.Stop() is already deferred earlier via defer tracker.Stop(); remove the
explicit tracker.Stop() line to avoid duplicate calls (alternatively, if you
prefer explicit control, remove the defer and keep the explicit tracker.Stop(),
but do not keep both). Ensure you only have one stop invocation for
tracker.Stop() and leave phase.Complete() unchanged.

cli/cmd/transfer/component-version/progress.go (1)

44-46: Silently ignoring JSON marshal error.

If json.MarshalIndent fails (e.g., on cyclic structures or unsupported types), specJSON will be nil and ErrorDetail will contain only the info text without the spec dump. Consider logging the marshal error at debug level for troubleshooting.

♻️ Log marshal failures

-		specJSON, _ := json.MarshalIndent(t.Spec.Data, "", "  ")
+		specJSON, marshalErr := json.MarshalIndent(t.Spec.Data, "", "  ")
+		if marshalErr != nil {
+			slog.Debug("failed to marshal transformation spec", "error", marshalErr, "id", t.ID)
+		}
 		evt.ErrorDetail = bar.FramedText(info, string(specJSON), 4)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/cmd/transfer/component-version/progress.go` around lines 44 - 46, The
code silently ignores errors from json.MarshalIndent when building
evt.ErrorDetail from t.Spec.Data; change the snippet to capture the error (err
:= json.MarshalIndent(...)), and if err != nil log the error at debug level
(including the err and the related t.ID/t.Type.Name/t.Type.Version) and use a
clear fallback string (e.g., a short "<json marshal error: ...>" message) for
specJSON so bar.FramedText(info, specJSONString, 4) still contains useful info;
update references to specJSON, json.MarshalIndent, evt.ErrorDetail,
bar.FramedText, info and t.Spec.Data accordingly.

cli/internal/render/progress/tracked.go (2)

49-53: Document the Start-before-Complete/Fail contract.

Complete() and Fail() block on <-p.finished, which is nil until Start() is called. A receive on a nil channel blocks forever. While the intended usage flow is Start → Complete/Fail, consider adding a brief doc comment or a panic-guard to clarify the contract.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/internal/render/progress/tracked.go` around lines 49 - 53, The Start()
method initializes p.finished (a channel) but Complete() and Fail() block on
<-p.finished and will deadlock if they are called before Start(); add a short
doc comment to trackedPhase.Start/Complete/Fail stating the required call order
(Start → Complete/Fail) and add a panic-guard in Complete() and Fail() that
checks if p.finished == nil and panics with a clear message (referencing methods
Start, Complete, Fail and the finished channel) so callers get an immediate,
descriptive failure instead of blocking forever.

---

26-35: Unused ctx field in trackedPhase.

The ctx field is stored (line 28) but never used in the current implementation. If it's intended for future use (e.g., context cancellation in the event loop), consider adding it; otherwise, remove it to avoid confusion.

♻️ If unused, consider removing

 type trackedPhase[E any] struct {
 	vis            Visualizer
-	ctx            context.Context
 	name           string
 	events         <-chan E
 	mapper         func(E) Event
 	total          int
 	finished       chan struct{}
 	errorFormatter func(error) string
 }

And update the factory:

 	return func(t *Tracker, name string) Phase {
 		t.injectLogBuffer(vis)
 		return &trackedPhase[E]{
 			vis:            vis,
-			ctx:            t.ctx,
 			name:           name,

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/internal/render/progress/tracked.go` around lines 26 - 35, The stored ctx
field on trackedPhase is unused; remove the ctx field from the trackedPhase
struct and update the factory/constructor that creates trackedPhase (the
function that builds instances of trackedPhase, e.g., newTrackedPhase or any
factory you added) to stop accepting/passing a context parameter, and remove any
code that references trackedPhase.ctx. Ensure all call sites that constructed
trackedPhase are updated to the new signature and that no remaining references
to ctx remain; alternatively, if you intended cancellation support, instead wire
that ctx into the event loop inside the trackedPhase's processing function to
handle context.Done() – but do not leave the unused ctx field in the struct.

cli/internal/render/progress/tracker.go (1)

67-84: Global slog state manipulation may affect concurrent tests.

NewTracker replaces the global slog.Default() logger (line 82), which could cause issues if tests run in parallel or if multiple trackers are created. For CLI code with single execution this is fine, but consider documenting this limitation.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/internal/render/progress/tracker.go` around lines 67 - 84, NewTracker
currently mutates global slog state via slog.SetDefault (in NewTracker) which
can break parallel tests; change Tracker to avoid touching global state by
either creating and holding a dedicated logger/handler internally (use
newBufferedHandler with t.logBuffer and keep previousLogger for restoration
semantics) or accept an injected *slog.Logger in NewTracker signature so callers
supply a per-instance logger; remove the slog.SetDefault call from NewTracker
(and update any Stop logic to only affect the logger instance) and document the
new behavior on Tracker/NewTracker to preserve test isolation.

cli/internal/render/progress/tracked.go (1)

24-31: Unused ctx field in trackedPhase.

The ctx field is stored but never referenced in the current implementation. If it's intended for future use (e.g., respecting context cancellation during event processing), consider adding a TODO comment. Otherwise, remove it to avoid confusion.

♻️ Remove unused field if not needed

 type trackedPhase[E, T any] struct {
 	vis      EventVisualizer[T]
-	ctx      context.Context
 	name     string
 	events   <-chan E
 	mapper   func(E) Event[T]
 	finished chan struct{}
 }

And update the factory:

 	return &trackedPhase[E, T]{
 		vis:    vis,
-		ctx:    t.ctx,
 		name:   name,
 		events: events,
 		mapper: mapper,
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/internal/render/progress/tracked.go` around lines 24 - 31, The stored ctx
field on trackedPhase is unused; either remove it or mark it for future use.
Remove the ctx field from the trackedPhase struct declaration and update any
constructors/factories and call sites that build trackedPhase (e.g., functions
that instantiate trackedPhase or a NewTrackedPhase/trackPhase helper) to stop
passing a context, or alternatively add a clear TODO comment on the ctx field
explaining planned use (e.g., for cancellation during event processing) if you
intend to keep it.

cli/internal/render/progress/tracker.go (1)

55-62: PhaseFactory looks exported, but custom implementations are effectively closed off.

Because the function returns the unexported startable, callers outside this package can use helper-produced factories but cannot define their own. If that is intentional, I’d make PhaseFactory unexported; otherwise expose an exported creation contract.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/internal/render/progress/tracker.go` around lines 55 - 62, The exported
type PhaseFactory currently returns the unexported interface startable,
preventing callers outside the package from implementing custom factories;
either make PhaseFactory unexported or change its return type to an exported
contract—e.g., return Phase (or export the interface name like Startable) so
external packages can implement factories. Update the type alias PhaseFactory to
either be unexported (phaseFactory) if you intend to keep startable internal, or
change its signature to return the exported Phase (or rename startable to
Startable and export it) and adjust any code referencing PhaseFactory,
startable, or StartPhase accordingly.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

cli/internal/render/progress/tracker.go (1)

77-91: ⚠️ Potential issue | 🔴 Critical

Data race on shared *bytes.Buffer remains unaddressed.

The logBuffer is passed to slog.NewTextHandler (via newBufferedHandler) at line 90, which writes to it from any goroutine calling slog.Info() etc. Visualizers read and drain this buffer during rendering. Since bytes.Buffer is not thread-safe and accesses are unprotected, concurrent log writes and UI renders cause a data race.

Consider wrapping the buffer in a synchronized type:

🔒 Proposed fix using a synchronized buffer

+// syncBuffer wraps bytes.Buffer with mutex protection for concurrent access.
+type syncBuffer struct {
+	mu  sync.Mutex
+	buf bytes.Buffer
+}
+
+func (s *syncBuffer) Write(p []byte) (n int, err error) {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	return s.buf.Write(p)
+}
+
+func (s *syncBuffer) DrainString() string {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	str := s.buf.String()
+	s.buf.Reset()
+	return str
+}

Then update Tracker to use *syncBuffer instead of *bytes.Buffer, and have visualizers call DrainString() instead of direct buffer access.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/internal/render/progress/tracker.go` around lines 77 - 91, The
bytes.Buffer used by Tracker (fields logBuffer and set in NewTracker via
newBufferedHandler) is not concurrency-safe; implement a synchronized wrapper
type (e.g., syncBuffer) that embeds/contains a bytes.Buffer and guards
Read/Write/Reset operations with a mutex and exposes a DrainString() method;
change Tracker.logBuffer to *syncBuffer, update NewTracker to create and pass
the syncBuffer to newBufferedHandler (and keep previousLogger logic), and update
any visualizer/renderer code that currently reads/drains the buffer to call
DrainString() instead of accessing the raw bytes.Buffer so all concurrent writes
and drains are mutex-protected.

cli/internal/render/progress/tracker_test.go (1)

46-53: Consider asserting slog state after double-stop.

The test verifies Stop() is idempotent by not panicking, but doesn't assert that slog.Default() remains correctly restored after the second call.

💡 Suggested enhancement

 func TestTracker_StopIsIdempotent(t *testing.T) {
 	originalLogger := slog.Default()
 	defer slog.SetDefault(originalLogger)

 	tracker := NewTracker(t.Context(), os.Stdout)
 	tracker.Stop()
 	tracker.Stop()
+	// Verify logger is still correctly restored after double-stop
+	if IsTerminal(os.Stdout) {
+		assert.Equal(t, originalLogger, slog.Default())
+	}
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/internal/render/progress/tracker_test.go` around lines 46 - 53, The test
TestTracker_StopIsIdempotent should assert that slog.Default() is restored after
calling tracker.Stop() twice: keep the existing originalLogger :=
slog.Default(), call tracker.Stop() twice as now, then add an assertion that the
current slog.Default() equals originalLogger (e.g., using require.Same(t,
originalLogger, slog.Default()) or a t.Fatalf check) to ensure NewTracker /
tracker.Stop restore the global logger correctly; reference the
TestTracker_StopIsIdempotent test, NewTracker, and tracker.Stop to locate where
to add the assertion.

cli/internal/render/progress/bar/animation.go (1)

69-87: Consider prioritizing cancellation in the animation loop.

While renderLocked() includes a guard against rendering after cancellation, the current select statement can still choose a ticker case after done is closed, causing unnecessary callback invocations. Use a nested select with priority pattern to guarantee cancellation is checked first:

 func RunAnimation(done <-chan struct{}, onSpin func(spinFrame, dotFrame int)) {
 	go func() {
 		spinTicker := time.NewTicker(80 * time.Millisecond)
 		dotTicker := time.NewTicker(400 * time.Millisecond)
 		defer spinTicker.Stop()
 		defer dotTicker.Stop()

 		spinFrame, dotFrame := 1, 0
 		for {
+			// Check cancellation before handling any ready tick.
+			select {
+			case <-done:
+				return
+			default:
+			}
+
 			select {
 			case <-done:
 				return
 			case <-dotTicker.C:
 				dotFrame++
 			case <-spinTicker.C:
 				onSpin(spinFrame, dotFrame)
 				spinFrame++
 			}
 		}
 	}()
 }

This is the standard Go pattern for imposing priority in select statements. Per the Go spec, when multiple cases are ready, one is chosen uniformly at random; the nested select with default ensures cancellation wins.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/internal/render/progress/bar/animation.go` around lines 69 - 87, In
RunAnimation, the select can service ticker cases even after done is closed;
change the loop to use the "priority" nested-select pattern: in the for loop,
first select on <-done with a return and a default branch, and inside the
default branch run a second select that handles dotTicker.C and spinTicker.C and
calls onSpin(spinFrame, dotFrame); this guarantees the done channel is always
checked first and prevents invoking onSpin after cancellation while keeping
spinTicker and dotTicker logic and the spinFrame/dotFrame increments intact.

cli/cmd/transfer/component-version/progress.go (1)

66-69: ⚠️ Potential issue | 🔴 Critical

Derive the display label from Transformation.Spec.

mapEvent() now surfaces this string throughout the tracked UI, but formatTransformationName() still ignores component, version, resourceIdentity, and resource and always returns ID [Type]. That misses the human-readable naming goal and leaves the updated name-formatting coverage failing.

Suggested fix

 func formatTransformationName(t *graphPkg.Transformation) string {
-	return fmt.Sprintf("%s [%s]", t.ID, t.Type.Name)
+	name := t.ID
+
+	if t.Spec != nil {
+		component, _ := t.Spec.Data["component"].(string)
+		version, _ := t.Spec.Data["version"].(string)
+		resourceIdentity, _ := t.Spec.Data["resourceIdentity"].(string)
+		resource, _ := t.Spec.Data["resource"].(string)
+
+		switch {
+		case component != "" && version != "":
+			name = fmt.Sprintf("%s@%s", component, version)
+		case component != "":
+			name = component
+		case resourceIdentity != "":
+			name = resourceIdentity
+		case resource != "":
+			name = resource
+		}
+
+		switch {
+		case resourceIdentity != "" && name != resourceIdentity:
+			name = fmt.Sprintf("%s (%s)", name, resourceIdentity)
+		case resource != "" && name != resource:
+			name = fmt.Sprintf("%s (%s)", name, resource)
+		}
+	}
+
+	if t.Type.Name == "" {
+		return name
+	}
+	return fmt.Sprintf("%s [%s]", name, t.Type.Name)
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/cmd/transfer/component-version/progress.go` around lines 66 - 69,
formatTransformationName currently returns only "ID [Type]" and must instead
derive the human-readable label from the Transformation.Spec fields used by
mapEvent; update the function formatTransformationName to build the display
string from t.Spec.component, t.Spec.version, t.Spec.resourceIdentity and
t.Spec.resource (falling back to existing ID and Type.Name when those spec
fields are empty) so the UI shows the same human-friendly label as mapEvent.

cli/internal/render/progress/bar/bar.go (1)

111-129: ⚠️ Potential issue | 🟡 Minor

Print the phase-level error when no item reaches Failed.

End() still only emits details from failed item events. If the phase exits with err != nil before any item transitions to progress.Failed, the user only sees a failed header and no error body.

Suggested fix

 func (v *barVisualizer[T]) End(err error) {
 	v.mu.Lock()
 	defer v.mu.Unlock()
@@
 	for _, event := range v.events {
 		if event.State == progress.Failed {
 			v.renderFailureSummary()
 			return
 		}
 	}
+
+	if err != nil {
+		fmt.Fprint(v.out, "\nErrors:\n")
+		var zero T
+		if v.errorFormatter != nil {
+			if formatted := v.errorFormatter(zero, err); formatted != "" {
+				fmt.Fprintf(v.out, "%s\n", formatted)
+				return
+			}
+		}
+		fmt.Fprintf(v.out, "  %v\n", err)
+	}
 }

cli/internal/render/progress/bar/animation.go (1)

76-76: spinFrame starts at 1, skipping the first spinner frame on initial render.

The spinFrame counter is initialized to 1 at line 76, which means the first call to onSpin will use frame index 1, skipping SpinnerFrames[0]. If the intent is to start with the first frame, initialize to 0 instead.

📝 Suggested fix

-		spinFrame, dotFrame := 1, 0
+		spinFrame, dotFrame := 0, 0

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/internal/render/progress/bar/animation.go` at line 76, The spinner
animation starts on the second frame because spinFrame is initialized to 1;
change the initialization of spinFrame to 0 (leave dotFrame as-is) so the first
call to onSpin uses SpinnerFrames[0]; update the variable declaration where
spinFrame is set (refer to spinFrame, dotFrame, onSpin, and SpinnerFrames in
animation.go).

cli/internal/render/progress/tracked.go (1)

51-59: Complete() and Fail() block indefinitely if events channel is never closed.

Both methods wait on <-p.finished, which only completes when run() returns. Since run() uses for evt := range p.events, it will block forever if the events channel is never closed by the producer.

This is a design contract that callers must follow. Consider documenting this requirement on the Tracked function.

📝 Suggested documentation improvement

 // Tracked returns a PhaseFactory for a tracked phase that processes item-level events.
 // E is the raw event type from the source channel.
 // T is the domain type carried in Event[T] for typed error formatting.
+//
+// The events channel must be closed by the producer when all events have been sent;
+// otherwise Complete() and Fail() will block indefinitely.
 func Tracked[E, T any](vis EventVisualizer[T], events <-chan E, mapper func(E) Event[T]) PhaseFactory {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/internal/render/progress/tracked.go` around lines 51 - 59, Complete() and
Fail() on trackedPhase[E, T] block waiting on <-p.finished because run() loops
with for evt := range p.events and only returns when the events channel is
closed; update the package docs for the Tracked function (and/or its public
constructor) to explicitly state the contract that producers must close the
events channel when done so that run() can exit and p.finished be closed,
referencing trackedPhase.run, trackedPhase.events and the Complete/Fail methods
to make the requirement unambiguous.

cli/cmd/transfer/component-version/progress_test.go (1)

26-33: Consider adding assertion that phase completes without error.

The test starts a phase and calls Complete(), but doesn't verify any observable outcome. Consider capturing output or adding a test visualizer that records calls.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/cmd/transfer/component-version/progress_test.go` around lines 26 - 33,
The test TestSimplePhaseLifecycle currently starts a phase via
progress.NewTracker(...), StartPhase("Test phase",
progress.Simple(newSimpleVisualizer(out))) and calls phase.Complete() but makes
no assertions; modify the test to assert observable behavior by either (a)
inspecting the bytes.Buffer out for expected output produced by the simple
visualizer after phase.Complete() (e.g. contains "Test phase" or a completion
marker), or (b) replace newSimpleVisualizer(out) with a test visualizer that
records lifecycle events and assert that it received Start/Complete calls for
"Test phase"; update the test to fail when those expectations are not met and
keep using tracker.Stop() in defer.

cli/internal/render/progress/bar/bar_test.go (1)

317-335: Fragile test setup: manually closing and recreating the done channel.

The pattern of close(v.done) followed by v.done = make(chan struct{}) at lines 322-323 (and similar in other tests) works around the animation goroutine but is fragile. If the internal implementation changes how done is used, these tests will break or behave unexpectedly.

Consider adding a test helper or exposing a way to stop animations for testing purposes.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/internal/render/progress/bar/bar_test.go` around lines 317 - 335, The
test is fragile because it manually closes and recreates barVisualizer.done to
stop the animation goroutine; instead add a deterministic testing hook to
NewBarVisualizer so tests can disable animations (e.g. an option like
WithNoAnimation or WithAnimationDisabled) or expose a Stop/Close method on
barVisualizer that cleanly terminates the animation goroutine, then update
TestBarVisualizer_BeginEnd_Success to construct the visualizer with that option
(via NewBarVisualizer[string](..., WithHeaderIcon(), WithNoAnimation())) or call
the new Stop/Close before asserting, and remove direct manipulation of v.done;
update related tests to use the same hook.

cli/internal/render/progress/bar/format.go (1)

93-114: Documentation example inconsistency with implementation.

The doc comment example shows inconsistent formatting between title and content lines:

• Line 97: │ Title (with space after bar)
• Lines 98-99: │line one (no space after bar)

However, the implementation at lines 105 and 110 shows:

• Title: │ (bar + space + Reset + line)
• Content: │ (bar + Reset + line, no extra space)

The example should reflect this difference or the behavior should be consistent.

📝 Suggested documentation fix to match implementation

 // Example output:
 //
 //	│ Title
-//	│line one
-//	│line two
+//	│ line one
+//	│ line two

Or update the implementation to add a space for content lines too:

 for _, line := range strings.Split(content, "\n") {
-	fmt.Fprintf(&sb, "%s│%s%s\n", color, Reset, line)
+	fmt.Fprintf(&sb, "%s│ %s%s\n", color, Reset, line)
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/internal/render/progress/bar/format.go` around lines 93 - 114, The doc
example for SidebarText is inconsistent with the implementation: title lines are
rendered with a space after the bar while content lines are rendered without
one. Fix by making behavior consistent—either update the comment example to show
content lines without the extra space, or (preferably) change the content
formatting in SidebarText so the content loop uses the same format as the title
(i.e., include the space after the bar), updating the fmt.Fprintf call that
formats content (the loop iterating over strings.Split(content, "\n")) to match
the title format that uses "%s│ %s%s\n" with the same color and Reset usage.

cli/internal/render/progress/tracker_test.go (1)

46-53: Test behavior depends on terminal detection but doesn't skip or handle both cases.

TestTracker_StopIsIdempotent uses os.Stdout but doesn't check IsTerminal(os.Stdout). In a non-terminal environment (like CI), the tracker won't redirect slog, and the test becomes trivial. Consider either skipping in non-terminal mode or using a buffer to test the non-terminal path consistently.

📝 Suggested fix for consistent behavior

 func TestTracker_StopIsIdempotent(t *testing.T) {
+	// Test with non-terminal output for consistent behavior across environments
 	originalLogger := slog.Default()
 	defer slog.SetDefault(originalLogger)

-	tracker := NewTracker(t.Context(), os.Stdout)
+	tracker := NewTracker(t.Context(), &bytes.Buffer{})
 	tracker.Stop()
 	tracker.Stop()
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/internal/render/progress/tracker_test.go` around lines 46 - 53,
TestTracker_StopIsIdempotent relies on terminal detection and currently uses
os.Stdout, making the test trivial in non-terminal CI; modify the test
(TestTracker_StopIsIdempotent) to handle both cases by checking
IsTerminal(os.Stdout) and if false create the tracker with a bytes.Buffer (or
io.Discard) instead of os.Stdout so the non-terminal path is exercised,
otherwise keep using os.Stdout; ensure you still restore slog.Default() and call
tracker.Stop() twice to verify idempotence.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes