Replace all spinner.New() calls with TTY-aware SpinWhile

AGENT.md

@@ -16,3 +16,4 @@ This project is the Buildkite CLI (`bk`)
 
 ## Notes
 - CI: https://buildkite.com/buildkite/buildkite-cli
+- Always format after changing code

internal/io/spinner_test.go

@@ -1,7 +1,10 @@
 package io
 
 import (
+	"os"
 	"testing"
+
+	"github.com/mattn/go-isatty"
 )
 
 func TestSpinWhileWithoutTTY(t *testing.T) {
@@ -19,3 +22,58 @@ func TestSpinWhileWithoutTTY(t *testing.T) {
 		t.Error("Action should have been called")
 	}
 }
+
+func TestSpinWhileActionIsExecuted(t *testing.T) {
+	// Test that the action is always executed regardless of TTY status
+	counter := 0
+	err := SpinWhile("Test action", func() {
+		counter++
+	})
+
+	if err != nil {
+		t.Errorf("SpinWhile should not return error: %v", err)
+	}
+
+	if counter != 1 {
+		t.Errorf("Action should have been called exactly once, got %d", counter)
+	}
+}
+
+func TestSpinWhileWithError(t *testing.T) {
+	// Test SpinWhile when action panics or has issues
+	actionCalled := false
+	err := SpinWhile("Test action with panic recovery", func() {
+		actionCalled = true
+		// Don't actually panic in test, just test normal flow
+	})
+
+	if err != nil {
+		t.Errorf("SpinWhile should not return error for normal action: %v", err)
+	}
+
+	if !actionCalled {
+		t.Error("Action should have been called")
+	}
+}
+
+func TestSpinWhileTTYDetection(t *testing.T) {
+	// Test that TTY detection works as expected
+	// This test documents the behavior rather than forcing specific outcomes
+	isTTY := isatty.IsTerminal(os.Stdout.Fd())
+
+	actionCalled := false
+	err := SpinWhile("TTY detection test", func() {
+		actionCalled = true
+	})
+
+	if err != nil {
+		t.Errorf("SpinWhile should not return error: %v", err)
+	}
+
+	if !actionCalled {
+		t.Error("Action should have been called regardless of TTY status")
+	}
+
+	// Document the current TTY status for debugging
+	t.Logf("Current TTY status: %v", isTTY)
+}

internal/pipeline/resolver/repository.go

@@ -4,10 +4,10 @@ import (
 	"context"
 	"strings"
 
+	bk_io "github.com/buildkite/cli/v3/internal/io"
 	"github.com/buildkite/cli/v3/internal/pipeline"
 	"github.com/buildkite/cli/v3/pkg/cmd/factory"
 	buildkite "github.com/buildkite/go-buildkite/v4"
-	"github.com/charmbracelet/huh/spinner"
 	git "github.com/go-git/go-git/v5"
 )
 
@@ -19,12 +19,9 @@ func ResolveFromRepository(f *factory.Factory, picker PipelinePicker) PipelineRe
 	return func(ctx context.Context) (*pipeline.Pipeline, error) {
 		var err error
 		var pipelines []pipeline.Pipeline
-		spinErr := spinner.New().
-			Title("Resolving pipeline").
-			Action(func() {
-				pipelines, err = resolveFromRepository(ctx, f)
-			}).
-			Run()
+		spinErr := bk_io.SpinWhile("Resolving pipeline", func() {
+			pipelines, err = resolveFromRepository(ctx, f)
+		})
 		if spinErr != nil {
 			return nil, spinErr
 		}

pkg/cmd/agent/view.go

@@ -5,9 +5,9 @@ import (
 
 	"github.com/MakeNowJust/heredoc"
 	"github.com/buildkite/cli/v3/internal/agent"
+	bk_io "github.com/buildkite/cli/v3/internal/io"
 	"github.com/buildkite/cli/v3/pkg/cmd/factory"
 	buildkite "github.com/buildkite/go-buildkite/v4"
-	"github.com/charmbracelet/huh/spinner"
 	"github.com/pkg/browser"
 	"github.com/spf13/cobra"
 )
@@ -37,12 +37,9 @@ func NewCmdAgentView(f *factory.Factory) *cobra.Command {
 
 			var err error
 			var agentData buildkite.Agent
-			spinErr := spinner.New().
-				Title("Loading agent").
-				Action(func() {
-					agentData, _, err = f.RestAPIClient.Agents.Get(cmd.Context(), org, id)
-				}).
-				Run()
+			spinErr := bk_io.SpinWhile("Loading agent", func() {
+				agentData, _, err = f.RestAPIClient.Agents.Get(cmd.Context(), org, id)
+			})
 			if spinErr != nil {
 				return spinErr
 			}

pkg/cmd/artifacts/download.go

@@ -10,8 +10,8 @@ import (
 
 	"github.com/MakeNowJust/heredoc"
 	"github.com/buildkite/cli/v3/internal/graphql"
+	bk_io "github.com/buildkite/cli/v3/internal/io"
 	"github.com/buildkite/cli/v3/pkg/cmd/factory"
-	"github.com/charmbracelet/huh/spinner"
 	"github.com/spf13/cobra"
 )
 
@@ -32,12 +32,9 @@ func NewCmdArtifactsDownload(f *factory.Factory) *cobra.Command {
 
 			var err error
 			var downloadDir string
-			spinErr := spinner.New().
-				Title("Downloading artifact").
-				Action(func() {
-					downloadDir, err = download(cmd.Context(), f, artifactId)
-				}).
-				Run()
+			spinErr := bk_io.SpinWhile("Downloading artifact", func() {
+				downloadDir, err = download(cmd.Context(), f, artifactId)
+			})
 			if spinErr != nil {
 				return spinErr
 			}

pkg/cmd/artifacts/list.go

@@ -8,10 +8,10 @@ import (
 	"github.com/buildkite/cli/v3/internal/artifact"
 	buildResolver "github.com/buildkite/cli/v3/internal/build/resolver"
 	"github.com/buildkite/cli/v3/internal/build/resolver/options"
+	bk_io "github.com/buildkite/cli/v3/internal/io"
 	pipelineResolver "github.com/buildkite/cli/v3/internal/pipeline/resolver"
 	"github.com/buildkite/cli/v3/pkg/cmd/factory"
 	buildkite "github.com/buildkite/go-buildkite/v4"
-	"github.com/charmbracelet/huh/spinner"
 	"github.com/charmbracelet/lipgloss"
 	"github.com/spf13/cobra"
 )
@@ -74,30 +74,27 @@ func NewCmdArtifactsList(f *factory.Factory) *cobra.Command {
 			var wg sync.WaitGroup
 			var mu sync.Mutex
 
-			spinErr := spinner.New().
-				Title("Loading artifacts information").
-				Action(func() {
-					wg.Add(1)
-
-					go func() {
-						defer wg.Done()
-						var apiErr error
-
-						if job != "" { // list artifacts for a specific job
-							buildArtifacts, _, apiErr = f.RestAPIClient.Artifacts.ListByJob(cmd.Context(), bld.Organization, bld.Pipeline, fmt.Sprint(bld.BuildNumber), job, nil)
-						} else {
-							buildArtifacts, _, apiErr = f.RestAPIClient.Artifacts.ListByBuild(cmd.Context(), bld.Organization, bld.Pipeline, fmt.Sprint(bld.BuildNumber), nil)
-						}
-						if apiErr != nil {
-							mu.Lock()
-							err = apiErr
-							mu.Unlock()
-						}
-					}()
-
-					wg.Wait()
-				}).
-				Run()
+			spinErr := bk_io.SpinWhile("Loading artifacts information", func() {
+				wg.Add(1)
+
+				go func() {
+					defer wg.Done()
+					var apiErr error
+
+					if job != "" { // list artifacts for a specific job
+						buildArtifacts, _, apiErr = f.RestAPIClient.Artifacts.ListByJob(cmd.Context(), bld.Organization, bld.Pipeline, fmt.Sprint(bld.BuildNumber), job, nil)
+					} else {
+						buildArtifacts, _, apiErr = f.RestAPIClient.Artifacts.ListByBuild(cmd.Context(), bld.Organization, bld.Pipeline, fmt.Sprint(bld.BuildNumber), nil)
+					}
+					if apiErr != nil {
+						mu.Lock()
+						err = apiErr
+						mu.Unlock()
+					}
+				}()
+
+				wg.Wait()
+			})
 			if spinErr != nil {
 				return spinErr
 			}

pkg/cmd/build/cancel.go

@@ -6,13 +6,12 @@ import (
 
 	"github.com/MakeNowJust/heredoc"
 	buildResolver "github.com/buildkite/cli/v3/internal/build/resolver"
-	"github.com/buildkite/cli/v3/internal/io"
+	bk_io "github.com/buildkite/cli/v3/internal/io"
 	pipelineResolver "github.com/buildkite/cli/v3/internal/pipeline/resolver"
 	"github.com/buildkite/cli/v3/internal/util"
 	"github.com/buildkite/cli/v3/internal/validation/scopes"
 	"github.com/buildkite/cli/v3/pkg/cmd/factory"
 	buildkite "github.com/buildkite/go-buildkite/v4"
-	"github.com/charmbracelet/huh/spinner"
 	"github.com/spf13/cobra"
 )
 
@@ -62,7 +61,7 @@ func NewCmdBuildCancel(f *factory.Factory) *cobra.Command {
 				return err
 			}
 
-			err = io.Confirm(&confirmed, fmt.Sprintf("Cancel build #%d on %s", bld.BuildNumber, bld.Pipeline))
+			err = bk_io.Confirm(&confirmed, fmt.Sprintf("Cancel build #%d on %s", bld.BuildNumber, bld.Pipeline))
 			if err != nil {
 				return err
 			}
@@ -92,12 +91,9 @@ func NewCmdBuildCancel(f *factory.Factory) *cobra.Command {
 func cancelBuild(ctx context.Context, org string, pipeline string, buildId string, web bool, f *factory.Factory) error {
 	var err error
 	var build buildkite.Build
-	spinErr := spinner.New().
-		Title(fmt.Sprintf("Cancelling build #%s from pipeline %s", buildId, pipeline)).
-		Action(func() {
-			build, err = f.RestAPIClient.Builds.Cancel(ctx, org, pipeline, buildId)
-		}).
-		Run()
+	spinErr := bk_io.SpinWhile(fmt.Sprintf("Cancelling build #%s from pipeline %s", buildId, pipeline), func() {
+		build, err = f.RestAPIClient.Builds.Cancel(ctx, org, pipeline, buildId)
+	})
 	if spinErr != nil {
 		return spinErr
 	}

pkg/cmd/build/download.go

@@ -10,10 +10,10 @@ import (
 	"github.com/buildkite/cli/v3/internal/build"
 	buildResolver "github.com/buildkite/cli/v3/internal/build/resolver"
 	"github.com/buildkite/cli/v3/internal/build/resolver/options"
+	bk_io "github.com/buildkite/cli/v3/internal/io"
 	pipelineResolver "github.com/buildkite/cli/v3/internal/pipeline/resolver"
 	"github.com/buildkite/cli/v3/internal/validation/scopes"
 	"github.com/buildkite/cli/v3/pkg/cmd/factory"
-	"github.com/charmbracelet/huh/spinner"
 	"github.com/spf13/cobra"
 )
 
@@ -85,12 +85,9 @@ func NewCmdBuildDownload(f *factory.Factory) *cobra.Command {
 			}
 
 			var dir string
-			spinErr := spinner.New().
-				Title("Downloading build resources").
-				Action(func() {
-					dir, err = download(cmd.Context(), bld, f)
-				}).
-				Run()
+			spinErr := bk_io.SpinWhile("Downloading build resources", func() {
+				dir, err = download(cmd.Context(), bld, f)
+			})
 			if spinErr != nil {
 				return spinErr
 			}

pkg/cmd/build/new.go

@@ -9,13 +9,12 @@ import (
 
 	"github.com/MakeNowJust/heredoc"
 	bkErrors "github.com/buildkite/cli/v3/internal/errors"
-	"github.com/buildkite/cli/v3/internal/io"
+	bk_io "github.com/buildkite/cli/v3/internal/io"
 	"github.com/buildkite/cli/v3/internal/pipeline/resolver"
 	"github.com/buildkite/cli/v3/internal/util"
 	"github.com/buildkite/cli/v3/internal/validation/scopes"
 	"github.com/buildkite/cli/v3/pkg/cmd/factory"
 	buildkite "github.com/buildkite/go-buildkite/v4"
-	"github.com/charmbracelet/huh/spinner"
 	"github.com/spf13/cobra"
 	"github.com/spf13/pflag"
 )
@@ -99,7 +98,7 @@ func NewCmdBuildNew(f *factory.Factory) *cobra.Command {
 				)
 			}
 
-			err = io.Confirm(&confirmed, fmt.Sprintf("Create new build on %s?", resolvedPipeline.Name))
+			err = bk_io.Confirm(&confirmed, fmt.Sprintf("Create new build on %s?", resolvedPipeline.Name))
 			if err != nil {
 				return bkErrors.NewUserAbortedError(err, "confirmation canceled")
 			}
@@ -179,47 +178,44 @@ func NewCmdBuildNew(f *factory.Factory) *cobra.Command {
 func newBuild(ctx context.Context, org string, pipeline string, f *factory.Factory, message string, commit string, branch string, web bool, env map[string]string, metaData map[string]string, ignoreBranchFilters bool) error {
 	var actionErr error
 	var build buildkite.Build
-	spinErr := spinner.New().
-		Title(fmt.Sprintf("Starting new build for %s", pipeline)).
-		Action(func() {
-			branch = strings.TrimSpace(branch)
-			if len(branch) == 0 {
-				p, _, err := f.RestAPIClient.Pipelines.Get(ctx, org, pipeline)
-				if err != nil {
-					actionErr = bkErrors.WrapAPIError(err, "fetching pipeline information")
-					return
-				}
-
-				// Check if the pipeline has a default branch set
-				if p.DefaultBranch == "" {
-					actionErr = bkErrors.NewValidationError(
-						nil,
-						fmt.Sprintf("No default branch set for pipeline %s", pipeline),
-						"Please specify a branch using --branch (-b)",
-						"Set a default branch in your pipeline settings on Buildkite",
-					)
-					return
-				}
-				branch = p.DefaultBranch
-			}
-
-			newBuild := buildkite.CreateBuild{
-				Message:                     message,
-				Commit:                      commit,
-				Branch:                      branch,
-				Env:                         env,
-				MetaData:                    metaData,
-				IgnorePipelineBranchFilters: ignoreBranchFilters,
+	spinErr := bk_io.SpinWhile(fmt.Sprintf("Starting new build for %s", pipeline), func() {
+		branch = strings.TrimSpace(branch)
+		if len(branch) == 0 {
+			p, _, err := f.RestAPIClient.Pipelines.Get(ctx, org, pipeline)
+			if err != nil {
+				actionErr = bkErrors.WrapAPIError(err, "fetching pipeline information")
+				return
 			}
 
-			var err error
-			build, _, err = f.RestAPIClient.Builds.Create(ctx, org, pipeline, newBuild)
-			if err != nil {
-				actionErr = bkErrors.WrapAPIError(err, "creating build")
+			// Check if the pipeline has a default branch set
+			if p.DefaultBranch == "" {
+				actionErr = bkErrors.NewValidationError(
+					nil,
+					fmt.Sprintf("No default branch set for pipeline %s", pipeline),
+					"Please specify a branch using --branch (-b)",
+					"Set a default branch in your pipeline settings on Buildkite",
+				)
 				return
 			}
-		}).
-		Run()
+			branch = p.DefaultBranch
+		}
+
+		newBuild := buildkite.CreateBuild{
+			Message:                     message,
+			Commit:                      commit,
+			Branch:                      branch,
+			Env:                         env,
+			MetaData:                    metaData,
+			IgnorePipelineBranchFilters: ignoreBranchFilters,
+		}
+
+		var err error
+		build, _, err = f.RestAPIClient.Builds.Create(ctx, org, pipeline, newBuild)
+		if err != nil {
+			actionErr = bkErrors.WrapAPIError(err, "creating build")
+			return
+		}
+	})
 	if spinErr != nil {
 		return bkErrors.NewInternalError(spinErr, "error in spinner UI")
 	}