Merge pull request #1669 from rumpl/fix-tick-leak

Fix tick leak

Author: dgageot

pkg/tui/components/spinner/spinner.go

@@ -19,8 +19,13 @@ const (
 	ModeSpinnerOnly
 )
 
-type Spinner struct {
-	animSub             animation.Subscription // manages animation tick subscription
+type Spinner interface {
+	layout.Model
+	Reset() Spinner
+	Stop()
+}
+type spinner struct {
+	animSub             *animation.Subscription // manages animation tick subscription
 	dotsStyle           lipgloss.Style
 	styledSpinnerFrames []string // pre-rendered spinner frames
 	mode                Mode
@@ -69,7 +74,9 @@ func New(mode Mode, dotsStyle lipgloss.Style) Spinner {
 		styledFrames[i] = dotsStyle.Render(char)
 	}
 
-	return Spinner{
+	sub := &animation.Subscription{}
+	return &spinner{
+		animSub:             sub,
 		dotsStyle:           dotsStyle,
 		styledSpinnerFrames: styledFrames,
 		mode:                mode,
@@ -79,11 +86,11 @@ func New(mode Mode, dotsStyle lipgloss.Style) Spinner {
 	}
 }
 
-func (s Spinner) Reset() Spinner {
+func (s *spinner) Reset() Spinner {
 	return New(s.mode, s.dotsStyle)
 }
 
-func (s Spinner) Update(message tea.Msg) (layout.Model, tea.Cmd) {
+func (s *spinner) Update(message tea.Msg) (layout.Model, tea.Cmd) {
 	if msg, ok := message.(animation.TickMsg); ok {
 		// Respond to global animation tick (all spinners advance together)
 		s.frame = msg.Frame
@@ -107,25 +114,25 @@ func (s Spinner) Update(message tea.Msg) (layout.Model, tea.Cmd) {
 	return s, nil
 }
 
-func (s Spinner) View() string {
+func (s *spinner) View() string {
 	spinner := s.styledSpinnerFrames[s.frame%len(s.styledSpinnerFrames)]
 	if s.mode == ModeSpinnerOnly {
 		return spinner
 	}
 	return spinner + " " + s.renderMessage()
 }
 
-func (s Spinner) SetSize(_, _ int) tea.Cmd { return nil }
+func (s *spinner) SetSize(_, _ int) tea.Cmd { return nil }
 
 // Init registers the spinner with the animation coordinator.
 // If this is the first active animation, it starts the global tick.
-func (s Spinner) Init() tea.Cmd {
+func (s *spinner) Init() tea.Cmd {
 	return s.animSub.Start()
 }
 
 // Stop unregisters the spinner from the animation coordinator.
 // Call this when the spinner is no longer active/visible.
-func (s Spinner) Stop() {
+func (s *spinner) Stop() {
 	s.animSub.Stop()
 }
 
@@ -139,7 +146,7 @@ var lightStyles = []lipgloss.Style{
 	styles.SpinnerTextDimmestStyle,
 }
 
-func (s Spinner) renderMessage() string {
+func (s *spinner) renderMessage() string {
 	var out strings.Builder
 	for i, char := range s.currentMessage {
 		dist := min(max(i-s.lightPosition, s.lightPosition-i), len(lightStyles)-1)

pkg/tui/components/spinner/spinner_test.go

@@ -4,8 +4,23 @@ import (
 	"testing"
 
 	"charm.land/lipgloss/v2"
+	"github.com/stretchr/testify/require"
+
+	"github.com/docker/cagent/pkg/tui/animation"
 )
 
+func TestSpinnerCopyDoesNotLeakAnimationSubscription(t *testing.T) {
+	s1 := New(ModeSpinnerOnly, lipgloss.NewStyle())
+	cmd := s1.Init()
+	require.NotNil(t, cmd)
+	require.True(t, animation.HasActive())
+
+	// Copy the spinner value and stop via the copy; should still stop the shared subscription.
+	s2 := s1
+	s2.Stop()
+	require.False(t, animation.HasActive())
+}
+
 func BenchmarkSpinner_ModeSpinnerOnly(b *testing.B) {
 	s := New(ModeSpinnerOnly, lipgloss.NewStyle())
 	for b.Loop() {