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") +}