git.iko.soy/crush
on main help sign in
fix(ui/chat): make keyboard expand work for assistant thinking blocks (#2791)
d7d25cfChristian Rocha
internal/ui/chat/assistant.go+14−9
compare with main
unifiedsplit
@@ -35,6 +35,8 @@ type AssistantMessageItem struct {
35 thinkingBoxHeight int // Tracks the rendered thinking box height for click detection.
35 thinkingBoxHeight int // Tracks the rendered thinking box height for click detection.
36}
36}
37
37
38+var _ Expandable = (*AssistantMessageItem)(nil)
39+
38// NewAssistantMessageItem creates a new AssistantMessageItem.
40// NewAssistantMessageItem creates a new AssistantMessageItem.
39func NewAssistantMessageItem(sty *styles.Styles, message *message.Message) MessageItem {
41func NewAssistantMessageItem(sty *styles.Styles, message *message.Message) MessageItem {
40 a := &AssistantMessageItem{
42 a := &AssistantMessageItem{
@@ -249,23 +251,26 @@ func (a *AssistantMessageItem) SetMessage(message *message.Message) tea.Cmd {
249 return nil
251 return nil
250}
252}
251
253
252−// ToggleExpanded toggles the expanded state of the thinking box.
254+// ToggleExpanded toggles the expanded state of the thinking box and returns
255+// whether the item is now expanded.
253−func (a *AssistantMessageItem) ToggleExpanded() {
256+func (a *AssistantMessageItem) ToggleExpanded() bool {
254 a.thinkingExpanded = !a.thinkingExpanded
257 a.thinkingExpanded = !a.thinkingExpanded
255 a.clearCache()
258 a.clearCache()
259+ return a.thinkingExpanded
256}
260}
257
261
258−// HandleMouseClick implements MouseClickable.
262+// HandleMouseClick implements MouseClickable. It signals (via a true return)
263+// that the click lies on the thinking box so the caller can invoke
264+// [AssistantMessageItem.ToggleExpanded] through the generic [Expandable]
265+// path. Toggling here directly would double-toggle because the caller always
266+// runs the generic path after a handled click.
259func (a *AssistantMessageItem) HandleMouseClick(btn ansi.MouseButton, x, y int) bool {
267func (a *AssistantMessageItem) HandleMouseClick(btn ansi.MouseButton, x, y int) bool {
260 if btn != ansi.MouseLeft {
268 if btn != ansi.MouseLeft {
261 return false
269 return false
262 }
270 }
263− // check if the click is within the thinking box
271+ // Only the thinking box is clickable; other regions of the assistant
272+ // message should not trigger expansion.
264− if a.thinkingBoxHeight > 0 && y < a.thinkingBoxHeight {
273+ return a.thinkingBoxHeight > 0 && y < a.thinkingBoxHeight
265− a.ToggleExpanded()
266− return true
267− }
268− return false
269}
274}
270
275
271// HandleKeyEvent implements KeyEventHandler.
276// HandleKeyEvent implements KeyEventHandler.
internal/ui/chat/assistant_test.gonew file+54
@@ -0,0 +1,54 @@
1+package chat
2+
3+import (
4+ "testing"
5+
6+ "github.com/charmbracelet/crush/internal/message"
7+ "github.com/charmbracelet/crush/internal/ui/styles"
8+ "github.com/charmbracelet/x/ansi"
9+ "github.com/stretchr/testify/require"
10+)
11+
12+// TestAssistantMessageItemExpandable guards the Expandable contract on
13+// AssistantMessageItem. The earlier implementation returned no value, which
14+// meant the type silently did not satisfy chat.Expandable and the
15+// keyboard-driven expand path in model/chat.go skipped thinking blocks.
16+func TestAssistantMessageItemExpandable(t *testing.T) {
17+ t.Parallel()
18+
19+ sty := styles.CharmtonePantera()
20+ msg := &message.Message{ID: "m1", Role: message.Assistant}
21+ item := NewAssistantMessageItem(&sty, msg)
22+
23+ exp, ok := item.(Expandable)
24+ require.True(t, ok, "AssistantMessageItem must satisfy Expandable")
25+
26+ require.True(t, exp.ToggleExpanded(), "first toggle should report expanded")
27+ require.False(t, exp.ToggleExpanded(), "second toggle should report collapsed")
28+}
29+
30+// TestAssistantMessageItemHandleMouseClick ensures HandleMouseClick does not
31+// toggle expansion on its own. The generic Expandable path in
32+// model/chat.go does the toggle; doing it here too would double-toggle and
33+// net to no change.
34+func TestAssistantMessageItemHandleMouseClick(t *testing.T) {
35+ t.Parallel()
36+
37+ sty := styles.CharmtonePantera()
38+ msg := &message.Message{ID: "m2", Role: message.Assistant}
39+ item := NewAssistantMessageItem(&sty, msg).(*AssistantMessageItem)
40+ item.thinkingBoxHeight = 5
41+
42+ // Click inside the thinking box signals handled but must not mutate
43+ // the expanded state.
44+ require.True(t, item.HandleMouseClick(ansi.MouseLeft, 0, 2))
45+ require.False(t, item.thinkingExpanded, "HandleMouseClick must not toggle expansion on its own")
46+
47+ // Click outside the thinking box is ignored entirely.
48+ require.False(t, item.HandleMouseClick(ansi.MouseLeft, 0, 10))
49+ require.False(t, item.thinkingExpanded)
50+
51+ // Non-left button is ignored.
52+ require.False(t, item.HandleMouseClick(ansi.MouseRight, 0, 2))
53+ require.False(t, item.thinkingExpanded)
54+}
internal/ui/model/chat.go+9−4
@@ -603,10 +603,15 @@ func (m *Chat) HandleDelayedClick(msg DelayedClickMsg) bool {
603 selectedItem := m.list.SelectedItem()
603 selectedItem := m.list.SelectedItem()
604 if clickable, ok := selectedItem.(list.MouseClickable); ok {
604 if clickable, ok := selectedItem.(list.MouseClickable); ok {
605 handled := clickable.HandleMouseClick(ansi.MouseButton1, msg.X, msg.Y)
605 handled := clickable.HandleMouseClick(ansi.MouseButton1, msg.X, msg.Y)
606− // Toggle expansion if applicable.
606+ // Toggle expansion only when the item signalled it handled the
607+ // click. Items like AssistantMessageItem only report handled when
608+ // the click is on their expandable region, so this avoids
609+ // toggling expansion for clicks outside the clickable area.
610+ if handled {
607− if expandable, ok := selectedItem.(chat.Expandable); ok {
611+ if expandable, ok := selectedItem.(chat.Expandable); ok {
608− if !expandable.ToggleExpanded() {
612+ if !expandable.ToggleExpanded() {
609− m.ScrollToIndex(m.list.Selected())
613+ m.ScrollToIndex(m.list.Selected())
614+ }
610 }
615 }
611 }
616 }
612 if m.AtBottom() {
617 if m.AtBottom() {
internal/ui/model/chat_expand_test.gonew file+49
@@ -0,0 +1,49 @@
1+package model
2+
3+import (
4+ "testing"
5+
6+ "github.com/charmbracelet/crush/internal/message"
7+ "github.com/charmbracelet/crush/internal/ui/chat"
8+ "github.com/stretchr/testify/require"
9+)
10+
11+// TestChatToggleExpandedSelectedItem_AssistantMessage is the regression test
12+// for §4.8.1: before the fix, AssistantMessageItem.ToggleExpanded returned no
13+// value so the type did not satisfy chat.Expandable, and the keyboard-driven
14+// ToggleExpandedSelectedItem path silently skipped thinking blocks. This
15+// wires a real Chat with an AssistantMessageItem, selects it, invokes
16+// ToggleExpandedSelectedItem, and asserts the thinking block actually
17+// flipped.
18+func TestChatToggleExpandedSelectedItem_AssistantMessage(t *testing.T) {
19+ t.Parallel()
20+
21+ u := newTestUI()
22+
23+ msg := &message.Message{ID: "m-assist", Role: message.Assistant}
24+ item := chat.NewAssistantMessageItem(u.com.Styles, msg)
25+
26+ // The keyboard expand path uses the generic Expandable interface;
27+ // verifying satisfaction at runtime guards the contract.
28+ exp, ok := item.(chat.Expandable)
29+ require.True(t, ok, "AssistantMessageItem must satisfy chat.Expandable")
30+
31+ u.chat.SetMessages(item)
32+ u.chat.SetSelected(0)
33+
34+ // First keyboard toggle should expand. Immediately follow with a
35+ // direct ToggleExpanded: it flips the now-expanded item back to
36+ // collapsed and returns false. If the keyboard path had silently
37+ // skipped the item (the bug), the item would still be collapsed and
38+ // the direct toggle would return true.
39+ u.chat.ToggleExpandedSelectedItem()
40+ require.False(t, exp.ToggleExpanded(),
41+ "keyboard toggle did not expand the assistant thinking block")
42+
43+ // Second keyboard toggle against the re-collapsed item should expand
44+ // it again. Direct ToggleExpanded then returns false because it
45+ // re-collapses.
46+ u.chat.ToggleExpandedSelectedItem()
47+ require.False(t, exp.ToggleExpanded(),
48+ "second keyboard toggle did not re-expand the assistant thinking block")
49+}