From 646a5053bd58635316bf960f3d8e5d4a0996a24b Mon Sep 17 00:00:00 2001 From: Christian Rocha Date: Thu, 14 May 2026 12:48:46 -0400 Subject: [PATCH] fix(ui): keep tool spinners animating during long-running tasks --- 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(+) diff --git a/internal/ui/chat/agent.go b/internal/ui/chat/agent.go index abc65e7b47c684cd1a4d8a12b0a96efaa924329e..8342ebaf8345dae6caf16db6b44aad75b97e367c 100644 --- a/internal/ui/chat/agent.go +++ b/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 diff --git a/internal/ui/chat/tools.go b/internal/ui/chat/tools.go index cac2e227aeab5b12c9e99c5777e0e2f810462ee3..3774e217fa362dc80057a3e92707bdeeff9d07d3 100644 --- a/internal/ui/chat/tools.go +++ b/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) } diff --git a/internal/ui/chat/version_bump_test.go b/internal/ui/chat/version_bump_test.go index 847f0053bb38324e7019e35294c7e9a81a9ba7a4..47ce7c5ed8ee615fb57d73df15260b9cddab0482 100644 --- a/internal/ui/chat/version_bump_test.go +++ b/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()