From d7d25cf89fe2613851aa083267ccb0e4389f05a1 Mon Sep 17 00:00:00 2001 From: Christian Rocha Date: Sat, 9 May 2026 20:15:44 -0400 Subject: [PATCH] fix(ui/chat): make keyboard expand work for assistant thinking blocks (#2791) The assistant message type's toggle-expand method returned no value, so it silently failed to satisfy the expandable interface and the keyboard expand path skipped assistant thinking blocks. Expansion only worked via the mouse, which called the concrete method directly. Fixing the signature exposed a second bug: the mouse path then toggled twice per click (once inside the item, once via the generic interface path), netting no change. The item now just reports whether the click hit the thinking box; the generic path does the toggle, gated on that flag so clicks elsewhere do nothing. Adds a runtime regression test plus a compile-time interface assertion so this can't silently come back. Co-authored-by: Crush --- internal/ui/chat/assistant.go | 23 +++++++----- internal/ui/chat/assistant_test.go | 54 +++++++++++++++++++++++++++ internal/ui/model/chat.go | 13 +++++-- internal/ui/model/chat_expand_test.go | 49 ++++++++++++++++++++++++ 4 files changed, 126 insertions(+), 13 deletions(-) create mode 100644 internal/ui/chat/assistant_test.go create mode 100644 internal/ui/model/chat_expand_test.go diff --git a/internal/ui/chat/assistant.go b/internal/ui/chat/assistant.go index ca95e8da9a40337f73a99a7df443abd9af2639d1..f49cd1d6ceca12b15f705640400da6941972342e 100644 --- a/internal/ui/chat/assistant.go +++ b/internal/ui/chat/assistant.go @@ -35,6 +35,8 @@ type AssistantMessageItem struct { thinkingBoxHeight int // Tracks the rendered thinking box height for click detection. } +var _ Expandable = (*AssistantMessageItem)(nil) + // NewAssistantMessageItem creates a new AssistantMessageItem. func NewAssistantMessageItem(sty *styles.Styles, message *message.Message) MessageItem { a := &AssistantMessageItem{ @@ -249,23 +251,26 @@ func (a *AssistantMessageItem) SetMessage(message *message.Message) tea.Cmd { return nil } -// ToggleExpanded toggles the expanded state of the thinking box. -func (a *AssistantMessageItem) ToggleExpanded() { +// ToggleExpanded toggles the expanded state of the thinking box and returns +// whether the item is now expanded. +func (a *AssistantMessageItem) ToggleExpanded() bool { a.thinkingExpanded = !a.thinkingExpanded a.clearCache() + return a.thinkingExpanded } -// HandleMouseClick implements MouseClickable. +// HandleMouseClick implements MouseClickable. It signals (via a true return) +// that the click lies on the thinking box so the caller can invoke +// [AssistantMessageItem.ToggleExpanded] through the generic [Expandable] +// path. Toggling here directly would double-toggle because the caller always +// runs the generic path after a handled click. func (a *AssistantMessageItem) HandleMouseClick(btn ansi.MouseButton, x, y int) bool { if btn != ansi.MouseLeft { return false } - // check if the click is within the thinking box - if a.thinkingBoxHeight > 0 && y < a.thinkingBoxHeight { - a.ToggleExpanded() - return true - } - return false + // Only the thinking box is clickable; other regions of the assistant + // message should not trigger expansion. + return a.thinkingBoxHeight > 0 && y < a.thinkingBoxHeight } // HandleKeyEvent implements KeyEventHandler. diff --git a/internal/ui/chat/assistant_test.go b/internal/ui/chat/assistant_test.go new file mode 100644 index 0000000000000000000000000000000000000000..8ccbb403c30a7aff2448c082d012512ee06cdc18 --- /dev/null +++ b/internal/ui/chat/assistant_test.go @@ -0,0 +1,54 @@ +package chat + +import ( + "testing" + + "github.com/charmbracelet/crush/internal/message" + "github.com/charmbracelet/crush/internal/ui/styles" + "github.com/charmbracelet/x/ansi" + "github.com/stretchr/testify/require" +) + +// TestAssistantMessageItemExpandable guards the Expandable contract on +// AssistantMessageItem. The earlier implementation returned no value, which +// meant the type silently did not satisfy chat.Expandable and the +// keyboard-driven expand path in model/chat.go skipped thinking blocks. +func TestAssistantMessageItemExpandable(t *testing.T) { + t.Parallel() + + sty := styles.CharmtonePantera() + msg := &message.Message{ID: "m1", Role: message.Assistant} + item := NewAssistantMessageItem(&sty, msg) + + exp, ok := item.(Expandable) + require.True(t, ok, "AssistantMessageItem must satisfy Expandable") + + require.True(t, exp.ToggleExpanded(), "first toggle should report expanded") + require.False(t, exp.ToggleExpanded(), "second toggle should report collapsed") +} + +// TestAssistantMessageItemHandleMouseClick ensures HandleMouseClick does not +// toggle expansion on its own. The generic Expandable path in +// model/chat.go does the toggle; doing it here too would double-toggle and +// net to no change. +func TestAssistantMessageItemHandleMouseClick(t *testing.T) { + t.Parallel() + + sty := styles.CharmtonePantera() + msg := &message.Message{ID: "m2", Role: message.Assistant} + item := NewAssistantMessageItem(&sty, msg).(*AssistantMessageItem) + item.thinkingBoxHeight = 5 + + // Click inside the thinking box signals handled but must not mutate + // the expanded state. + require.True(t, item.HandleMouseClick(ansi.MouseLeft, 0, 2)) + require.False(t, item.thinkingExpanded, "HandleMouseClick must not toggle expansion on its own") + + // Click outside the thinking box is ignored entirely. + require.False(t, item.HandleMouseClick(ansi.MouseLeft, 0, 10)) + require.False(t, item.thinkingExpanded) + + // Non-left button is ignored. + require.False(t, item.HandleMouseClick(ansi.MouseRight, 0, 2)) + require.False(t, item.thinkingExpanded) +} diff --git a/internal/ui/model/chat.go b/internal/ui/model/chat.go index 5cc620102febbe4af724ee1770f93f0f8de13212..177daf2570b1f2738ba596b43390df99c8550d7f 100644 --- a/internal/ui/model/chat.go +++ b/internal/ui/model/chat.go @@ -603,10 +603,15 @@ func (m *Chat) HandleDelayedClick(msg DelayedClickMsg) bool { selectedItem := m.list.SelectedItem() if clickable, ok := selectedItem.(list.MouseClickable); ok { handled := clickable.HandleMouseClick(ansi.MouseButton1, msg.X, msg.Y) - // Toggle expansion if applicable. - if expandable, ok := selectedItem.(chat.Expandable); ok { - if !expandable.ToggleExpanded() { - m.ScrollToIndex(m.list.Selected()) + // Toggle expansion only when the item signalled it handled the + // click. Items like AssistantMessageItem only report handled when + // the click is on their expandable region, so this avoids + // toggling expansion for clicks outside the clickable area. + if handled { + if expandable, ok := selectedItem.(chat.Expandable); ok { + if !expandable.ToggleExpanded() { + m.ScrollToIndex(m.list.Selected()) + } } } if m.AtBottom() { diff --git a/internal/ui/model/chat_expand_test.go b/internal/ui/model/chat_expand_test.go new file mode 100644 index 0000000000000000000000000000000000000000..f40e79b08512bd6f34b9f166ee661263e33e6d0a --- /dev/null +++ b/internal/ui/model/chat_expand_test.go @@ -0,0 +1,49 @@ +package model + +import ( + "testing" + + "github.com/charmbracelet/crush/internal/message" + "github.com/charmbracelet/crush/internal/ui/chat" + "github.com/stretchr/testify/require" +) + +// TestChatToggleExpandedSelectedItem_AssistantMessage is the regression test +// for ยง4.8.1: before the fix, AssistantMessageItem.ToggleExpanded returned no +// value so the type did not satisfy chat.Expandable, and the keyboard-driven +// ToggleExpandedSelectedItem path silently skipped thinking blocks. This +// wires a real Chat with an AssistantMessageItem, selects it, invokes +// ToggleExpandedSelectedItem, and asserts the thinking block actually +// flipped. +func TestChatToggleExpandedSelectedItem_AssistantMessage(t *testing.T) { + t.Parallel() + + u := newTestUI() + + msg := &message.Message{ID: "m-assist", Role: message.Assistant} + item := chat.NewAssistantMessageItem(u.com.Styles, msg) + + // The keyboard expand path uses the generic Expandable interface; + // verifying satisfaction at runtime guards the contract. + exp, ok := item.(chat.Expandable) + require.True(t, ok, "AssistantMessageItem must satisfy chat.Expandable") + + u.chat.SetMessages(item) + u.chat.SetSelected(0) + + // First keyboard toggle should expand. Immediately follow with a + // direct ToggleExpanded: it flips the now-expanded item back to + // collapsed and returns false. If the keyboard path had silently + // skipped the item (the bug), the item would still be collapsed and + // the direct toggle would return true. + u.chat.ToggleExpandedSelectedItem() + require.False(t, exp.ToggleExpanded(), + "keyboard toggle did not expand the assistant thinking block") + + // Second keyboard toggle against the re-collapsed item should expand + // it again. Direct ToggleExpanded then returns false because it + // re-collapses. + u.chat.ToggleExpandedSelectedItem() + require.False(t, exp.ToggleExpanded(), + "second keyboard toggle did not re-expand the assistant thinking block") +}