fix(ui): keep tool spinners animating during long-running tasks

Christian Rocha created

Change summary

internal/ui/chat/agent.go             |  38 +++++++
internal/ui/chat/tools.go             |  12 ++
internal/ui/chat/version_bump_test.go | 140 +++++++++++++++++++++++++++++
3 files changed, 190 insertions(+)

Detailed changes

internal/ui/chat/agent.go ๐Ÿ”—

@@ -53,11 +53,21 @@ func NewAgentToolMessageItem(
 }
 
 // Animate progresses the message animation if it should be spinning.
+//
+// Bumps the parent's F6 list-cache version on both the parent-tick and
+// nested-tick branches. Nested tools are not list entries of their
+// own โ€” their IDs map to this parent's index in idInxMap
+// (internal/ui/model/chat.go:240-246) and their renders are embedded
+// inline in this parent's output โ€” so the list only checks the
+// parent's version. Without the bump, the list cache would serve the
+// previously rendered frame indefinitely and the spinner would appear
+// frozen.
 func (a *AgentToolMessageItem) Animate(msg anim.StepMsg) tea.Cmd {
 	if a.result != nil || a.Status() == ToolStatusCanceled {
 		return nil
 	}
 	if msg.ID == a.ID() {
+		a.Bump()
 		return a.anim.Animate(msg)
 	}
 	for _, nestedTool := range a.nestedTools {
@@ -65,6 +75,7 @@ func (a *AgentToolMessageItem) Animate(msg anim.StepMsg) tea.Cmd {
 			continue
 		}
 		if s, ok := nestedTool.(Animatable); ok {
+			a.Bump()
 			return s.Animate(msg)
 		}
 	}
@@ -211,6 +222,33 @@ func NewAgenticFetchToolMessageItem(
 	return t
 }
 
+// Animate progresses the message animation if it should be spinning.
+// See [AgentToolMessageItem.Animate] for the parent-bump rationale โ€”
+// without an override, the embedded base.Animate would (a) drop
+// StepMsgs whose ID matches a nested child instead of the parent
+// (anim.Animate's ID check at internal/ui/anim/anim.go:326-329
+// silently returns nil), and (b) never invalidate the parent's
+// list-cache entry on a parent tick.
+func (a *AgenticFetchToolMessageItem) Animate(msg anim.StepMsg) tea.Cmd {
+	if a.result != nil || a.Status() == ToolStatusCanceled {
+		return nil
+	}
+	if msg.ID == a.ID() {
+		a.Bump()
+		return a.anim.Animate(msg)
+	}
+	for _, nestedTool := range a.nestedTools {
+		if msg.ID != nestedTool.ID() {
+			continue
+		}
+		if s, ok := nestedTool.(Animatable); ok {
+			a.Bump()
+			return s.Animate(msg)
+		}
+	}
+	return nil
+}
+
 // NestedTools returns the nested tools.
 func (a *AgenticFetchToolMessageItem) NestedTools() []ToolMessageItem {
 	return a.nestedTools

internal/ui/chat/tools.go ๐Ÿ”—

@@ -296,10 +296,22 @@ func (t *baseToolMessageItem) StartAnimation() tea.Cmd {
 }
 
 // Animate progresses the assistant message animation if it should be spinning.
+//
+// Bumps the F6 list-cache version so the next draw re-renders this
+// item: a spinner tick mutates anim's internal frame counter, which
+// changes the rendered output but is invisible to the per-item
+// caches. Without the bump the list cache would serve the previously
+// rendered frame indefinitely and the spinner would appear frozen.
+// The ID gate keeps unrelated ticks (routed here by a future change
+// to chat.Animate's dispatch) from churning the cache.
 func (t *baseToolMessageItem) Animate(msg anim.StepMsg) tea.Cmd {
 	if !t.isSpinning() {
 		return nil
 	}
+	if msg.ID != t.toolCall.ID {
+		return nil
+	}
+	t.Bump()
 	return t.anim.Animate(msg)
 }
 

internal/ui/chat/version_bump_test.go ๐Ÿ”—

@@ -400,6 +400,146 @@ func TestAgenticFetchToolMessageItem_NestedChildInPlaceMutationBumpsParent(t *te
 		"parent SetNestedTools must bump even when child pointers are unchanged")
 }
 
+// requireNoBump asserts the supplied mutator leaves the item's
+// Version() unchanged. The mutator runs once; an unexpected bump
+// would force the F6 list memo to re-render an item whose output
+// did not change, churning the cache.
+func requireNoBump(t *testing.T, name string, item versionedItem, mutate func()) {
+	t.Helper()
+	before := item.Version()
+	mutate()
+	after := item.Version()
+	require.Equalf(t, before, after,
+		"%s must not bump Version() (before=%d, after=%d)", name, before, after)
+}
+
+// TestBaseToolMessageItem_AnimateBumpsVersion is the spinner
+// regression test for non-agent tools: while the tool is spinning,
+// every anim.StepMsg whose ID matches the tool must bump Version()
+// so the list-level cache invalidates and the next draw re-renders
+// the advanced spinner frame. Foreign IDs must not bump (they would
+// churn the cache on every frame), and a finished tool must not
+// bump on any ID (the entry is frozen and stays frozen).
+func TestBaseToolMessageItem_AnimateBumpsVersion(t *testing.T) {
+	t.Parallel()
+
+	sty := styles.CharmtonePantera()
+	tc := message.ToolCall{ID: "tc-spin", Name: "bash", Input: "{}", Finished: false}
+	item := NewToolMessageItem(&sty, "msg", tc, nil, false)
+	v := item.(versionedItem)
+	a, ok := item.(Animatable)
+	require.True(t, ok, "base tool message item must implement Animatable")
+
+	// Spinning + matching ID โ†’ bump.
+	requireBump(t, "Animate[spinning,own ID]", v, func() {
+		a.Animate(anim.StepMsg{ID: tc.ID})
+	})
+
+	// Spinning + foreign ID โ†’ no bump. Routing this StepMsg here at
+	// all would mean a future chat.Animate refactor; the item must
+	// be defensive against it so we don't churn the list cache.
+	requireNoBump(t, "Animate[spinning,foreign ID]", v, func() {
+		a.Animate(anim.StepMsg{ID: "some-other-tool"})
+	})
+
+	// Finished โ†’ no bump on any ID. The entry is frozen; a stray
+	// bump would needlessly invalidate frozen entries.
+	tcFinished := tc
+	tcFinished.Finished = true
+	item.SetToolCall(tcFinished)
+	item.SetResult(&message.ToolResult{ToolCallID: tc.ID, Content: "ok"})
+	require.True(t, item.Finished(), "tool must report Finished() once the result lands")
+
+	requireNoBump(t, "Animate[finished,own ID]", v, func() {
+		a.Animate(anim.StepMsg{ID: tc.ID})
+	})
+	requireNoBump(t, "Animate[finished,foreign ID]", v, func() {
+		a.Animate(anim.StepMsg{ID: "some-other-tool"})
+	})
+}
+
+// TestAgentToolMessageItem_AnimateBumpsVersion is the spinner
+// regression test for agent tools. The parent must bump on both
+// the parent-tick branch (msg.ID == parent.ID()) and the
+// nested-tick branch (msg.ID == nested.ID()) because the list
+// only checks the parent's version โ€” nested tools are not list
+// entries of their own. Unrelated IDs must not bump, and a parent
+// with a result must not bump on any ID.
+func TestAgentToolMessageItem_AnimateBumpsVersion(t *testing.T) {
+	t.Parallel()
+
+	sty := styles.CharmtonePantera()
+	parentTC := message.ToolCall{ID: "agent-parent", Name: "agent", Input: `{}`, Finished: false}
+	parent := NewAgentToolMessageItem(&sty, parentTC, nil, false)
+
+	childTC := message.ToolCall{ID: "agent-child", Name: "bash", Input: `{}`, Finished: false}
+	child := NewToolMessageItem(&sty, "msg", childTC, nil, false)
+	parent.AddNestedTool(child)
+
+	// Spinning + parent's own ID โ†’ parent bumps.
+	requireBump(t, "Animate[spinning,parent ID]", parent, func() {
+		parent.Animate(anim.StepMsg{ID: parentTC.ID})
+	})
+
+	// Spinning + nested child ID โ†’ parent bumps. The list only
+	// invalidates on the parent; without this the nested
+	// spinner's frame would never reach the screen even though
+	// the nested anim's step has advanced.
+	requireBump(t, "Animate[spinning,nested ID]", parent, func() {
+		parent.Animate(anim.StepMsg{ID: childTC.ID})
+	})
+
+	// Spinning + unrelated ID โ†’ no bump.
+	requireNoBump(t, "Animate[spinning,foreign ID]", parent, func() {
+		parent.Animate(anim.StepMsg{ID: "unrelated"})
+	})
+
+	// Once the parent has a result, neither branch bumps.
+	parent.SetResult(&message.ToolResult{ToolCallID: parentTC.ID, Content: "done"})
+	requireNoBump(t, "Animate[finished,parent ID]", parent, func() {
+		parent.Animate(anim.StepMsg{ID: parentTC.ID})
+	})
+	requireNoBump(t, "Animate[finished,nested ID]", parent, func() {
+		parent.Animate(anim.StepMsg{ID: childTC.ID})
+	})
+}
+
+// TestAgenticFetchToolMessageItem_AnimateBumpsVersion is the
+// agentic-fetch counterpart of the agent-tool Animate bump test.
+// Without an explicit override the embedded base Animate would
+// drop nested-child StepMsgs at anim.Animate's ID check and never
+// bump the parent on its own ticks; this test locks in the
+// override.
+func TestAgenticFetchToolMessageItem_AnimateBumpsVersion(t *testing.T) {
+	t.Parallel()
+
+	sty := styles.CharmtonePantera()
+	parentTC := message.ToolCall{ID: "fetch-parent", Name: "agentic_fetch", Input: `{}`, Finished: false}
+	parent := NewAgenticFetchToolMessageItem(&sty, parentTC, nil, false)
+
+	childTC := message.ToolCall{ID: "fetch-child", Name: "fetch", Input: `{}`, Finished: false}
+	child := NewToolMessageItem(&sty, "msg", childTC, nil, false)
+	parent.AddNestedTool(child)
+
+	requireBump(t, "Animate[spinning,parent ID]", parent, func() {
+		parent.Animate(anim.StepMsg{ID: parentTC.ID})
+	})
+	requireBump(t, "Animate[spinning,nested ID]", parent, func() {
+		parent.Animate(anim.StepMsg{ID: childTC.ID})
+	})
+	requireNoBump(t, "Animate[spinning,foreign ID]", parent, func() {
+		parent.Animate(anim.StepMsg{ID: "unrelated"})
+	})
+
+	parent.SetResult(&message.ToolResult{ToolCallID: parentTC.ID, Content: "done"})
+	requireNoBump(t, "Animate[finished,parent ID]", parent, func() {
+		parent.Animate(anim.StepMsg{ID: parentTC.ID})
+	})
+	requireNoBump(t, "Animate[finished,nested ID]", parent, func() {
+		parent.Animate(anim.StepMsg{ID: childTC.ID})
+	})
+}
+
 // TestBaseToolMessageItem_FinishedTransition covers ยง4.5.1 for
 // tools: a still-running tool reports Finished() == false; once the
 // tool call is marked finished and a result lands, Finished()