diff --git a/pkg/runtime/runtime.go b/pkg/runtime/runtime.go index 2ae323026..dcfe93521 100644 --- a/pkg/runtime/runtime.go +++ b/pkg/runtime/runtime.go @@ -1139,6 +1139,7 @@ func toolsetStatusFor(ts tools.ToolSet) tools.ToolsetStatus { // earlier if Start failed. status.State = lifecycleStateForUnsupervised(ts) } + _, status.Restartable = tools.As[tools.Restartable](ts) status.Name = nameFor(ts, status.Description) return status } diff --git a/pkg/runtime/toolsetstatus_test.go b/pkg/runtime/toolsetstatus_test.go index 0ca2cdf2b..7e04011e7 100644 --- a/pkg/runtime/toolsetstatus_test.go +++ b/pkg/runtime/toolsetstatus_test.go @@ -59,6 +59,23 @@ func TestToolsetStatusFor_NoStatableMeansReady(t *testing.T) { assert.Equal(t, 0, got.RestartCount) require.NoError(t, got.LastError) assert.Equal(t, "filesystem", got.Description) + assert.False(t, got.Restartable, "toolset without Restart() must report Restartable=false") +} + +func TestToolsetStatusFor_RestartableReportsTrue(t *testing.T) { + t.Parallel() + + ts := &restartableToolset{desc: "mcp(stdio cmd=foo)", state: lifecycle.StateInfo{State: lifecycle.StateReady}} + got := toolsetStatusFor(ts) + assert.True(t, got.Restartable) +} + +func TestToolsetStatusFor_NonRestartableReportsFalse(t *testing.T) { + t.Parallel() + + ts := &statefulToolset{desc: "filesystem", info: lifecycle.StateInfo{State: lifecycle.StateReady}} + got := toolsetStatusFor(ts) + assert.False(t, got.Restartable) } // TestToolsetStatusFor_UnwrapsStartable verifies the inner Statable is diff --git a/pkg/tools/status.go b/pkg/tools/status.go index 2c5129108..af3e286cf 100644 --- a/pkg/tools/status.go +++ b/pkg/tools/status.go @@ -43,4 +43,9 @@ type ToolsetStatus struct { // RestartCount is the number of supervisor restarts since the last // successful Ready transition. Zero for toolsets without a supervisor. RestartCount int + // Restartable reports whether the toolset implements Restartable + // (typically supervisor-backed MCP/LSP toolsets). Surfaces such as the + // /toolset-restart completion popup use this to annotate, not hide, + // entries that cannot actually be restarted. + Restartable bool } diff --git a/pkg/tui/commands/commands.go b/pkg/tui/commands/commands.go index f504fba79..02f10d28c 100644 --- a/pkg/tui/commands/commands.go +++ b/pkg/tui/commands/commands.go @@ -1,6 +1,7 @@ package commands import ( + "cmp" "context" "fmt" "slices" @@ -11,6 +12,7 @@ import ( "github.com/docker/docker-agent/pkg/app" "github.com/docker/docker-agent/pkg/config/types" "github.com/docker/docker-agent/pkg/feedback" + "github.com/docker/docker-agent/pkg/tools" mcptools "github.com/docker/docker-agent/pkg/tools/mcp" "github.com/docker/docker-agent/pkg/tui/components/toolcommon" "github.com/docker/docker-agent/pkg/tui/core" @@ -26,6 +28,18 @@ type Category struct { Commands []Item } +// ArgumentCandidate is one completable value for a command's argument, +// e.g. a toolset name for /toolset-restart. It is intentionally free of any +// completion-UI or app-domain types so pkg/tui/commands stays decoupled from +// both pkg/tui/components/completion and pkg/app. +type ArgumentCandidate struct { + Label string + Description string + // Disabled marks a candidate that is shown for context but cannot be + // submitted (e.g. a non-restartable toolset for /toolset-restart). + Disabled bool +} + // Item represents a single command in the palette type Item struct { ID string @@ -38,6 +52,11 @@ type Item struct { // Immediate marks commands that should run as soon as they are submitted // instead of being treated as ordinary queued chat input. Immediate bool + // CompleteArgument, when set, returns the candidates for this command's + // argument. Called lazily at completion-popup-open time so results + // reflect current runtime state (e.g. toolset lifecycle). Nil for + // commands with no argument completion. + CompleteArgument func() []ArgumentCandidate } func builtInSessionCommands() []Item { @@ -577,6 +596,64 @@ func newMCPPromptItem(promptName string, promptInfo mcptools.PromptInfo) Item { } } +// toolsetStatusSource is the minimal surface toolsetRestartCandidates needs. +// *app.App satisfies it; tests can supply a stub instead of constructing a +// full application. +type toolsetStatusSource interface { + CurrentAgentToolsetStatuses() []tools.ToolsetStatus +} + +// toolsetRestartCandidates returns one ArgumentCandidate per toolset of the +// current agent, in declaration order, deduplicated by name (preferring the +// restartable entry when duplicate names disagree — display names are not +// guaranteed unique, mirroring runtime.RestartToolset's own matching). +// Non-restartable toolsets are marked Disabled rather than omitted, so the +// popup teaches users the full toolset inventory and why a restart isn't +// offered for some of them. +func toolsetRestartCandidates(source toolsetStatusSource) []ArgumentCandidate { + statuses := source.CurrentAgentToolsetStatuses() + byName := make(map[string]tools.ToolsetStatus, len(statuses)) + order := make([]string, 0, len(statuses)) + for _, s := range statuses { + if s.Name == "" { + continue + } + if existing, ok := byName[s.Name]; !ok { + byName[s.Name] = s + order = append(order, s.Name) + } else if !existing.Restartable && s.Restartable { + byName[s.Name] = s + } + } + + candidates := make([]ArgumentCandidate, 0, len(order)) + for _, name := range order { + s := byName[name] + candidates = append(candidates, ArgumentCandidate{ + Label: s.Name, + Description: cmp.Or(s.Kind, "Built-in") + " · " + s.State.String(), + Disabled: !s.Restartable, + }) + } + return candidates +} + +// attachToolsetRestartCompletion wires the toolset-name argument completer +// onto the /toolset-restart item. Attached post-hoc (rather than inline in +// builtInSessionCommands) so that function stays free of any status-source +// dependency. +func attachToolsetRestartCompletion(items []Item, source toolsetStatusSource) { + for i := range items { + if items[i].ID != "session.toolset.restart" { + continue + } + items[i].CompleteArgument = func() []ArgumentCandidate { + return toolsetRestartCandidates(source) + } + return + } +} + // BuildCommandCategories builds the list of command categories for the command palette func BuildCommandCategories(ctx context.Context, application *app.App) []Category { // Get session commands and filter based on model capabilities @@ -584,6 +661,7 @@ func BuildCommandCategories(ctx context.Context, application *app.App) []Categor if !application.SnapshotsEnabled() { sessionCommands = removeByIDs(sessionCommands, snapshotCommandIDs) } + attachToolsetRestartCompletion(sessionCommands, application) categories := []Category{ { diff --git a/pkg/tui/commands/commands_test.go b/pkg/tui/commands/commands_test.go index f8bad58d8..25722ce12 100644 --- a/pkg/tui/commands/commands_test.go +++ b/pkg/tui/commands/commands_test.go @@ -6,6 +6,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/docker/docker-agent/pkg/tools" + "github.com/docker/docker-agent/pkg/tools/lifecycle" "github.com/docker/docker-agent/pkg/tui/messages" ) @@ -277,3 +279,122 @@ func TestSettingsCommandLabel(t *testing.T) { assert.Equal(t, "settings.open", settings.ID) assert.Equal(t, "Settings", settings.Label) } + +// stubToolsetStatusSource is a minimal toolsetStatusSource for exercising +// toolsetRestartCandidates without a real *app.App. +type stubToolsetStatusSource struct { + statuses []tools.ToolsetStatus +} + +func (s stubToolsetStatusSource) CurrentAgentToolsetStatuses() []tools.ToolsetStatus { + return s.statuses +} + +func TestToolsetRestartCandidates_AllShownWithDisabledFlag(t *testing.T) { + t.Parallel() + + source := stubToolsetStatusSource{statuses: []tools.ToolsetStatus{ + {Name: "github", Kind: "MCP", State: lifecycle.StateReady, Restartable: true}, + {Name: "filesystem", State: lifecycle.StateReady, Restartable: false}, + }} + + candidates := toolsetRestartCandidates(source) + require.Len(t, candidates, 2) + + assert.Equal(t, "github", candidates[0].Label) + assert.False(t, candidates[0].Disabled) + assert.Contains(t, candidates[0].Description, "MCP") + + assert.Equal(t, "filesystem", candidates[1].Label) + assert.True(t, candidates[1].Disabled, "non-restartable toolsets must be shown, not hidden") + assert.Contains(t, candidates[1].Description, "Built-in") +} + +func TestToolsetRestartCandidates_DedupePrefersRestartable(t *testing.T) { + t.Parallel() + + source := stubToolsetStatusSource{statuses: []tools.ToolsetStatus{ + {Name: "dup", State: lifecycle.StateStopped, Restartable: false}, + {Name: "dup", State: lifecycle.StateReady, Restartable: true}, + }} + + candidates := toolsetRestartCandidates(source) + require.Len(t, candidates, 1, "duplicate names must be deduplicated") + assert.False(t, candidates[0].Disabled, "the restartable entry must win over the non-restartable duplicate") +} + +func TestToolsetRestartCandidates_EmptyNameSkipped(t *testing.T) { + t.Parallel() + + source := stubToolsetStatusSource{statuses: []tools.ToolsetStatus{{Name: "", Restartable: true}}} + assert.Empty(t, toolsetRestartCandidates(source)) +} + +func TestAttachToolsetRestartCompletion(t *testing.T) { + t.Parallel() + + items := builtInSessionCommands() + source := stubToolsetStatusSource{statuses: []tools.ToolsetStatus{ + {Name: "github", Restartable: true}, + }} + attachToolsetRestartCompletion(items, source) + + var restart, other *Item + for i := range items { + switch items[i].ID { + case "session.toolset.restart": + restart = &items[i] + case "session.exit": + other = &items[i] + } + } + require.NotNil(t, restart) + require.NotNil(t, restart.CompleteArgument, "the restart item must get a completer") + candidates := restart.CompleteArgument() + require.Len(t, candidates, 1) + assert.Equal(t, "github", candidates[0].Label) + + require.NotNil(t, other) + assert.Nil(t, other.CompleteArgument, "commands without a provider keep a nil CompleteArgument") +} + +// TestAttachToolsetRestartCompletion_QueriesFreshOnEachCall is a regression +// test for bug 2 (PR #3728): the attached CompleteArgument closure must query +// the toolset status source live on every call, not capture a snapshot at +// attach time. Otherwise the first Tab after "/toolset-restart " shows +// whatever toolsets existed when the command list was built, not the +// current ones. +func TestAttachToolsetRestartCompletion_QueriesFreshOnEachCall(t *testing.T) { + t.Parallel() + + items := builtInSessionCommands() + source := &stubToolsetStatusSource{statuses: []tools.ToolsetStatus{ + {Name: "github", State: lifecycle.StateReady, Restartable: true}, + }} + attachToolsetRestartCompletion(items, source) + + var restart *Item + for i := range items { + if items[i].ID == "session.toolset.restart" { + restart = &items[i] + } + } + require.NotNil(t, restart) + + first := restart.CompleteArgument() + require.Len(t, first, 1) + assert.Equal(t, "github", first[0].Label) + + // The toolset status source changes after the completer was attached + // (e.g. the agent restarted with a different toolset set). A second call + // must reflect that change rather than replaying the first snapshot. + source.statuses = []tools.ToolsetStatus{ + {Name: "filesystem", State: lifecycle.StateReady, Restartable: true}, + {Name: "github", State: lifecycle.StateReady, Restartable: true}, + } + + second := restart.CompleteArgument() + require.Len(t, second, 2, "must reflect the current toolset set, not the one captured at attach time") + assert.Equal(t, "filesystem", second[0].Label) + assert.Equal(t, "github", second[1].Label) +} diff --git a/pkg/tui/components/completion/completion.go b/pkg/tui/components/completion/completion.go index a81352c79..1b92b4029 100644 --- a/pkg/tui/components/completion/completion.go +++ b/pkg/tui/components/completion/completion.go @@ -34,6 +34,11 @@ type Item struct { Value string Execute func() tea.Cmd Pinned bool // Pinned items always appear at the top, in original order + // Disabled marks an item that is shown for context but cannot be + // submitted (e.g. a non-restartable toolset for /toolset-restart). + // Enter/Tab are a no-op on it and it never drives the ghost-suggestion + // preview; cursor movement over it is still allowed. + Disabled bool } type OpenMsg struct { @@ -262,11 +267,17 @@ func (c *manager) Update(msg tea.Msg) (layout.Model, tea.Cmd) { return c, cmd case key.Matches(msg, c.keyMap.Enter): - c.visible = false if len(c.filteredItems) == 0 || c.selected >= len(c.filteredItems) { + c.visible = false return c, core.CmdHandler(ClosedMsg{}) } selectedItem := c.filteredItems[c.selected] + if selectedItem.Disabled { + // No-op: the dimmed style already signals why this entry + // can't be submitted. Keep the popup open. + return c, nil + } + c.visible = false return c, tea.Sequence( core.CmdHandler(SelectedMsg{ Value: selectedItem.Value, @@ -276,11 +287,15 @@ func (c *manager) Update(msg tea.Msg) (layout.Model, tea.Cmd) { core.CmdHandler(ClosedMsg{}), ) case key.Matches(msg, c.keyMap.Tab): - c.visible = false if len(c.filteredItems) == 0 || c.selected >= len(c.filteredItems) { + c.visible = false return c, core.CmdHandler(ClosedMsg{}) } selectedItem := c.filteredItems[c.selected] + if selectedItem.Disabled { + return c, nil + } + c.visible = false return c, tea.Sequence( core.CmdHandler(SelectedMsg{ Value: selectedItem.Value, @@ -339,19 +354,34 @@ func (c *manager) View() string { itemStyle := styles.CompletionNormalStyle descStyle := styles.CompletionDescStyle - if isSelected { + switch { + case item.Disabled && isSelected: + itemStyle = styles.CompletionDisabledSelectedStyle + descStyle = styles.CompletionDisabledDescStyle + case item.Disabled: + itemStyle = styles.CompletionDisabledStyle + descStyle = styles.CompletionDisabledDescStyle + case isSelected: itemStyle = styles.CompletionSelectedStyle descStyle = styles.CompletionSelectedDescStyle } // Pad label to maxLabelLen so descriptions align paddedLabel := item.Label + strings.Repeat(" ", maxLabelLen+1-lipgloss.Width(item.Label)) - text := paddedLabel + // Render the label and description as separate segments and join them + // rather than re-Render()-ing the whole line through itemStyle: that + // outer re-wrap mangles the ANSI reset already embedded by descStyle's + // inner Render (most visibly with CompletionDisabledSelectedStyle's + // Underline, which leaks the raw escape code as literal text). + line := itemStyle.Render(paddedLabel) if item.Description != "" { - text += " " + descStyle.Render(item.Description) + line = lipgloss.JoinHorizontal(lipgloss.Top, line, " ", descStyle.Render(item.Description)) + } + if pad := (c.width - 6) - lipgloss.Width(line); pad > 0 { + line += itemStyle.Render(strings.Repeat(" ", pad)) } - lines = append(lines, itemStyle.Width(c.width-6).Render(text)) + lines = append(lines, line) } } @@ -376,12 +406,18 @@ func (c *manager) GetLayers() []*lipgloss.Layer { } } -// notifySelectionChanged sends a SelectionChangedMsg with the currently selected item's value +// notifySelectionChanged sends a SelectionChangedMsg with the currently selected item's value. +// Disabled items report an empty value so the editor's ghost-suggestion +// preview shows nothing for entries that can't be submitted. func (c *manager) notifySelectionChanged() tea.Cmd { if len(c.filteredItems) == 0 || c.selected >= len(c.filteredItems) { return core.CmdHandler(SelectionChangedMsg{Value: ""}) } - return core.CmdHandler(SelectionChangedMsg{Value: c.filteredItems[c.selected].Value}) + selectedItem := c.filteredItems[c.selected] + if selectedItem.Disabled { + return core.CmdHandler(SelectionChangedMsg{Value: ""}) + } + return core.CmdHandler(SelectionChangedMsg{Value: selectedItem.Value}) } func (c *manager) filterItems(query string) { diff --git a/pkg/tui/components/completion/disabled_test.go b/pkg/tui/components/completion/disabled_test.go new file mode 100644 index 000000000..299fa6be6 --- /dev/null +++ b/pkg/tui/components/completion/disabled_test.go @@ -0,0 +1,143 @@ +package completion + +import ( + "testing" + + tea "charm.land/bubbletea/v2" + "github.com/charmbracelet/x/ansi" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestCompletionManagerDisabledItems verifies that Disabled items (e.g. a +// non-restartable toolset for /toolset-restart) are shown for context but +// cannot be submitted: Enter/Tab are a no-op and the popup stays open, the +// ghost-suggestion preview stays empty while one is selected, and cursor +// movement still lands on them. +func TestCompletionManagerDisabledItems(t *testing.T) { + t.Parallel() + + t.Run("enter on a disabled item is a no-op and keeps the popup open", func(t *testing.T) { + t.Parallel() + + m := New().(*manager) + m.Update(OpenMsg{Items: []Item{ + {Label: "filesystem", Value: "/toolset-restart filesystem", Disabled: true}, + }}) + require.True(t, m.visible) + + _, cmd := m.Update(tea.KeyPressMsg{Code: tea.KeyEnter}) + + assert.True(t, m.visible, "popup must stay open on a disabled entry") + assert.Nil(t, cmd, "no SelectedMsg/ClosedMsg should be dispatched") + }) + + t.Run("tab on a disabled item is a no-op and keeps the popup open", func(t *testing.T) { + t.Parallel() + + m := New().(*manager) + m.Update(OpenMsg{Items: []Item{ + {Label: "filesystem", Value: "/toolset-restart filesystem", Disabled: true}, + }}) + + _, cmd := m.Update(tea.KeyPressMsg{Code: tea.KeyTab}) + + assert.True(t, m.visible) + assert.Nil(t, cmd) + }) + + t.Run("enter on an enabled item still submits as usual", func(t *testing.T) { + t.Parallel() + + m := New().(*manager) + m.Update(OpenMsg{Items: []Item{ + {Label: "github", Value: "/toolset-restart github", Disabled: false}, + }}) + + _, cmd := m.Update(tea.KeyPressMsg{Code: tea.KeyEnter}) + + assert.False(t, m.visible, "popup must close on a normal entry") + assert.NotNil(t, cmd) + }) + + t.Run("selecting a disabled item sends an empty preview value", func(t *testing.T) { + t.Parallel() + + m := New().(*manager) + m.Update(OpenMsg{Items: []Item{ + {Label: "github", Value: "/toolset-restart github", Disabled: false}, + {Label: "filesystem", Value: "/toolset-restart filesystem", Disabled: true}, + }}) + + // Move selection onto the disabled item. + _, cmd := m.Update(tea.KeyPressMsg{Code: tea.KeyDown}) + assert.Equal(t, 1, m.selected, "cursor movement onto a disabled item must still work") + + require.NotNil(t, cmd) + msg := cmd() + selChanged, ok := msg.(SelectionChangedMsg) + require.True(t, ok) + assert.Empty(t, selChanged.Value, "disabled items must not drive the ghost-suggestion preview") + }) + + t.Run("view dims a disabled item and does not dim a normal one", func(t *testing.T) { + t.Parallel() + + m := New().(*manager) + m.width = 80 + m.height = 24 + m.Update(OpenMsg{Items: []Item{ + {Label: "github", Description: "MCP · ready", Value: "/toolset-restart github", Disabled: false}, + {Label: "filesystem", Description: "Built-in · (not restartable)", Value: "/toolset-restart filesystem", Disabled: true}, + }}) + + view := m.View() + assert.Contains(t, view, "github") + assert.Contains(t, view, "filesystem") + assert.Contains(t, view, "not restartable") + }) + + t.Run("view never leaks raw ANSI escape text, selected or not, disabled or not", func(t *testing.T) { + t.Parallel() + + // Regression test for a bug where selecting a disabled row rendered + // the literal escape text (e.g. "[3;38;2;90;106;168m...[m") instead of + // a styled line: completion.go's View() rendered the description with + // descStyle.Render(...) and then re-wrapped the whole line (label + + // already-ANSI description) in itemStyle.Render(...). That outer + // re-render mangled the inner ANSI reset whenever itemStyle also set an + // attribute (e.g. CompletionDisabledSelectedStyle's Underline). + newManager := func(selected int) *manager { + m := New().(*manager) + m.width = 80 + m.height = 24 + m.Update(OpenMsg{Items: []Item{ + {Label: "github", Description: "MCP · ready", Value: "/toolset-restart github", Disabled: false}, + {Label: "filesystem", Description: "Built-in · ready (not restartable)", Value: "/toolset-restart filesystem", Disabled: true}, + }}) + m.selected = selected + return m + } + + for _, tc := range []struct { + name string + selected int + wantText string + }{ + {"disabled selected", 1, "Built-in · ready (not restartable)"}, + {"disabled unselected", 0, "Built-in · ready (not restartable)"}, + {"enabled selected", 0, "MCP · ready"}, + {"enabled unselected", 1, "MCP · ready"}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + visible := ansi.Strip(newManager(tc.selected).View()) + + assert.NotContains(t, visible, "[3;", "raw ANSI SGR params must not leak as literal text") + assert.NotContains(t, visible, "[m", "raw ANSI reset must not leak as literal text") + assert.Contains(t, visible, tc.wantText, "the annotation must still render") + }) + } + }) +} diff --git a/pkg/tui/components/editor/completion_argument_test.go b/pkg/tui/components/editor/completion_argument_test.go new file mode 100644 index 000000000..ed5f3c3da --- /dev/null +++ b/pkg/tui/components/editor/completion_argument_test.go @@ -0,0 +1,313 @@ +package editor + +import ( + "testing" + + "charm.land/bubbles/v2/textarea" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/docker/docker-agent/pkg/tui/components/completion" + "github.com/docker/docker-agent/pkg/tui/components/editor/completions" + "github.com/docker/docker-agent/pkg/tui/messages" +) + +// mockArgumentCompletion implements both completions.Completion and +// completions.ArgumentCompleter, mirroring commandCompletion for +// /toolset-restart-style argument completion without depending on the real +// commands package. +type mockArgumentCompletion struct { + mockCompletion + + argItems []completion.Item + argMatched bool +} + +func (m *mockArgumentCompletion) ArgumentItems(string) ([]completion.Item, bool) { + return m.argItems, m.argMatched +} + +// MatchMode overrides the embedded mockCompletion's fuzzy default: the real +// commandCompletion source uses prefix matching, and tests assert on it. +func (m *mockArgumentCompletion) MatchMode() completion.MatchMode { + return completion.MatchPrefix +} + +var _ completions.ArgumentCompleter = (*mockArgumentCompletion)(nil) + +func newArgumentCompletion(matched bool, items ...completion.Item) *mockArgumentCompletion { + return &mockArgumentCompletion{ + mockCompletion: mockCompletion{trigger: "/"}, + argItems: items, + argMatched: matched, + } +} + +// dynamicArgumentCompletion is an ArgumentCompleter whose candidates are +// computed live by a callback on every call, mirroring how the real +// commandCompletion source calls CompleteArgument() fresh each time instead +// of returning a cached snapshot. +type dynamicArgumentCompletion struct { + mockCompletion + + itemsFunc func() []completion.Item +} + +func (d *dynamicArgumentCompletion) ArgumentItems(string) ([]completion.Item, bool) { + return d.itemsFunc(), true +} + +var _ completions.ArgumentCompleter = (*dynamicArgumentCompletion)(nil) + +// newArgumentTestEditor builds a bare editor with the given value and +// completion sources, mirroring newTestEditor but without pre-seeding any +// active completion session. +func newArgumentTestEditor(value string, comps ...completions.Completion) *editor { + ta := textarea.New() + ta.SetWidth(80) + ta.SetHeight(10) + ta.Focus() + ta.SetValue(value) + ta.MoveToEnd() + + return &editor{ + textarea: ta, + completions: comps, + userTyped: true, + } +} + +func TestTryStartArgumentCompletion(t *testing.T) { + t.Parallel() + + t.Run("no space in value returns nil without mutating state", func(t *testing.T) { + t.Parallel() + + argComp := newArgumentCompletion(true, completion.Item{Label: "github", Value: "/toolset-restart github"}) + e := newArgumentTestEditor("/toolset-restart", argComp) + + cmd := e.TryStartArgumentCompletion() + + assert.Nil(t, cmd) + assert.Empty(t, e.argumentPrefix) + assert.Nil(t, e.currentCompletion) + }) + + t.Run("unknown command returns nil", func(t *testing.T) { + t.Parallel() + + argComp := newArgumentCompletion(false) + e := newArgumentTestEditor("/nope something", argComp) + + cmd := e.TryStartArgumentCompletion() + + assert.Nil(t, cmd) + assert.Empty(t, e.argumentPrefix) + }) + + t.Run("command without candidates returns nil", func(t *testing.T) { + t.Parallel() + + argComp := newArgumentCompletion(true) // matched, but zero items + e := newArgumentTestEditor("/toolset-restart ", argComp) + + cmd := e.TryStartArgumentCompletion() + + assert.Nil(t, cmd) + assert.Empty(t, e.argumentPrefix) + }) + + t.Run("plain text (no leading command) returns nil", func(t *testing.T) { + t.Parallel() + + argComp := newArgumentCompletion(false) + e := newArgumentTestEditor("hello world", argComp) + + cmd := e.TryStartArgumentCompletion() + + assert.Nil(t, cmd) + assert.Empty(t, e.argumentPrefix) + }) + + t.Run("matching command opens popup, sets prefix and seeds query", func(t *testing.T) { + t.Parallel() + + argComp := newArgumentCompletion(true, + completion.Item{Label: "github", Value: "/toolset-restart github"}, + completion.Item{Label: "filesystem", Value: "/toolset-restart filesystem", Disabled: true}, + ) + e := newArgumentTestEditor("/toolset-restart gi", argComp) + + cmd := e.TryStartArgumentCompletion() + require.NotNil(t, cmd) + + assert.Equal(t, "/toolset-restart ", e.argumentPrefix) + assert.Same(t, argComp, e.currentCompletion) + + msgs := collectMsgs(cmd) + require.Len(t, msgs, 2) + + openMsg, ok := msgs[0].(completion.OpenMsg) + require.True(t, ok, "first message should be OpenMsg") + assert.Equal(t, argComp.argItems, openMsg.Items) + assert.Equal(t, completion.MatchPrefix, openMsg.MatchMode) + + queryMsg, ok := msgs[1].(completion.QueryMsg) + require.True(t, ok, "second message should be the seeded QueryMsg") + assert.Equal(t, "gi", queryMsg.Query) + }) + + t.Run("trailing space with no typed argument yet seeds an empty query", func(t *testing.T) { + t.Parallel() + + argComp := newArgumentCompletion(true, completion.Item{Label: "github", Value: "/toolset-restart github"}) + e := newArgumentTestEditor("/toolset-restart ", argComp) + + cmd := e.TryStartArgumentCompletion() + require.NotNil(t, cmd) + + msgs := collectMsgs(cmd) + require.Len(t, msgs, 2) + queryMsg, ok := msgs[1].(completion.QueryMsg) + require.True(t, ok) + assert.Empty(t, queryMsg.Query) + }) + + // Regression test for bug 2 (PR #3728): a single Tab must reflect the + // current candidate set on every call, never a snapshot captured by an + // earlier invocation (the bug required typing a space, deleting it, then + // Tab again to see fresh candidates). + t.Run("reflects the current candidate set on each call, not a stale snapshot", func(t *testing.T) { + t.Parallel() + + current := []completion.Item{{Label: "oldtool", Value: "/toolset-restart oldtool"}} + argComp := &dynamicArgumentCompletion{ + mockCompletion: mockCompletion{trigger: "/"}, + itemsFunc: func() []completion.Item { return current }, + } + e := newArgumentTestEditor("/toolset-restart ", argComp) + + firstCmd := e.TryStartArgumentCompletion() + require.NotNil(t, firstCmd) + openMsg, ok := collectMsgs(firstCmd)[0].(completion.OpenMsg) + require.True(t, ok) + require.Len(t, openMsg.Items, 1) + assert.Equal(t, "oldtool", openMsg.Items[0].Label) + + // The underlying toolset set changes (e.g. the agent restarted with a + // different toolset). A single, fresh Tab press must pick that up. + e.currentCompletion = nil + e.argumentPrefix = "" + current = []completion.Item{{Label: "newtool", Value: "/toolset-restart newtool"}} + + secondCmd := e.TryStartArgumentCompletion() + require.NotNil(t, secondCmd) + openMsg, ok = collectMsgs(secondCmd)[0].(completion.OpenMsg) + require.True(t, ok) + require.Len(t, openMsg.Items, 1) + assert.Equal(t, "newtool", openMsg.Items[0].Label, "a fresh Tab must reflect the current candidates, not replay the first call's snapshot") + }) +} + +func TestArgumentModeSelectedMsg(t *testing.T) { + t.Parallel() + + t.Run("Tab (no auto-submit) inserts the full value and keeps editing", func(t *testing.T) { + t.Parallel() + + e := newArgumentTestEditor("/toolset-restart gi") + e.argumentPrefix = "/toolset-restart " + + _, cmd := e.Update(completion.SelectedMsg{ + Value: "/toolset-restart github", + AutoSubmit: false, + }) + + assert.Nil(t, cmd) + assert.Equal(t, "/toolset-restart github", e.textarea.Value()) + }) + + t.Run("Enter (auto-submit) sends the full value", func(t *testing.T) { + t.Parallel() + + e := newArgumentTestEditor("/toolset-restart gi") + e.argumentPrefix = "/toolset-restart " + + _, cmd := e.Update(completion.SelectedMsg{ + Value: "/toolset-restart github", + AutoSubmit: true, + }) + + require.NotNil(t, cmd) + var found bool + for _, msg := range collectMsgs(cmd) { + if sm, ok := msg.(messages.SendMsg); ok { + assert.Equal(t, "/toolset-restart github", sm.Content) + found = true + } + } + assert.True(t, found, "should send the full argument-mode value") + }) +} + +func TestUpdateCompletionQueryArgumentMode(t *testing.T) { + t.Parallel() + + t.Run("typing further narrows the query", func(t *testing.T) { + t.Parallel() + + e := newArgumentTestEditor("/toolset-restart git") + e.argumentPrefix = "/toolset-restart " + + cmd := e.updateCompletionQuery() + require.NotNil(t, cmd) + + queryMsg, ok := findQueryMsg(cmd) + require.True(t, ok) + assert.Equal(t, "git", queryMsg.Query) + assert.Equal(t, "/toolset-restart ", e.argumentPrefix, "prefix should stay active") + }) + + t.Run("editing away the command prefix closes the session", func(t *testing.T) { + t.Parallel() + + e := newArgumentTestEditor("/toolset-restar") + e.argumentPrefix = "/toolset-restart " + e.currentCompletion = newArgumentCompletion(true) + + cmd := e.updateCompletionQuery() + require.NotNil(t, cmd) + + assert.True(t, hasCloseMsg(cmd)) + assert.Empty(t, e.argumentPrefix) + assert.Nil(t, e.currentCompletion) + }) +} + +func TestArgumentModeClosedMsgClearsPrefix(t *testing.T) { + t.Parallel() + + e := newArgumentTestEditor("/toolset-restart github") + e.argumentPrefix = "/toolset-restart " + e.currentCompletion = newArgumentCompletion(true) + + _, _ = e.Update(completion.ClosedMsg{}) + + assert.Empty(t, e.argumentPrefix) + assert.Nil(t, e.currentCompletion) +} + +// TestTryStartArgumentCompletionSkipsNonArgumentSources ensures +// TryStartArgumentCompletion safely skips sources that don't implement +// ArgumentCompleter (e.g. the "@" file source) instead of panicking on a +// failed type assertion. +func TestTryStartArgumentCompletionSkipsNonArgumentSources(t *testing.T) { + t.Parallel() + + fileComp := &mockCompletion{trigger: "@"} + e := newArgumentTestEditor("/toolset-restart gi", fileComp) + + cmd := e.TryStartArgumentCompletion() + + assert.Nil(t, cmd) +} diff --git a/pkg/tui/components/editor/completion_backspace_test.go b/pkg/tui/components/editor/completion_backspace_test.go index 279c313cc..0edaec03d 100644 --- a/pkg/tui/components/editor/completion_backspace_test.go +++ b/pkg/tui/components/editor/completion_backspace_test.go @@ -1,6 +1,7 @@ package editor import ( + "reflect" "testing" "charm.land/bubbles/v2/textarea" @@ -128,7 +129,7 @@ func TestBackspaceUpdatesCompletionQuery(t *testing.T) { }) } -// collectMsgs executes a command (or batch of commands) and collects all returned messages +// collectMsgs executes a command (or batch/sequence of commands) and collects all returned messages func collectMsgs(cmd tea.Cmd) []tea.Msg { if cmd == nil { return nil @@ -144,6 +145,25 @@ func collectMsgs(cmd tea.Cmd) []tea.Msg { } return msgs } + + // tea.Sequence returns an unexported slice-of-Cmd type; reflection is the + // only way to unpack it from outside the tea package. + msgValue := reflect.ValueOf(msg) + if msg != nil && msgValue.Kind() == reflect.Slice { + var msgs []tea.Msg + for i := range msgValue.Len() { + elem := msgValue.Index(i) + if elem.CanInterface() { + if innerCmd, ok := elem.Interface().(tea.Cmd); ok && innerCmd != nil { + msgs = append(msgs, collectMsgs(innerCmd)...) + } + } + } + if len(msgs) > 0 { + return msgs + } + } + if msg != nil { return []tea.Msg{msg} } diff --git a/pkg/tui/components/editor/completions/command.go b/pkg/tui/components/editor/completions/command.go index 873b384a1..999314861 100644 --- a/pkg/tui/components/editor/completions/command.go +++ b/pkg/tui/components/editor/completions/command.go @@ -52,3 +52,39 @@ func sortItemsByLabel(items []completion.Item) []completion.Item { func (c *commandCompletion) MatchMode() completion.MatchMode { return completion.MatchPrefix } + +// ArgumentItems implements ArgumentCompleter: it looks up the command whose +// SlashCommand is the first token of line and, if it exposes a +// CompleteArgument provider, maps its candidates to completion items. The +// full command line ("/cmd label") is used as Value so selecting an item +// replaces the editor content wholesale — this tolerates argument values +// containing spaces, which a word-based splice would not. +func (c *commandCompletion) ArgumentItems(line string) ([]completion.Item, bool) { + slashCommand, _, _ := strings.Cut(line, " ") + + for _, cat := range c.categories { + for _, cmd := range cat.Commands { + if cmd.SlashCommand != slashCommand || cmd.CompleteArgument == nil { + continue + } + + candidates := cmd.CompleteArgument() + items := make([]completion.Item, 0, len(candidates)) + for _, candidate := range candidates { + description := candidate.Description + if candidate.Disabled { + description = strings.TrimSpace(description + " (not restartable)") + } + items = append(items, completion.Item{ + Label: candidate.Label, + Description: description, + Value: cmd.SlashCommand + " " + candidate.Label, + Disabled: candidate.Disabled, + }) + } + return items, true + } + } + + return nil, false +} diff --git a/pkg/tui/components/editor/completions/command_test.go b/pkg/tui/components/editor/completions/command_test.go index cc9e1d8d5..6bef0f111 100644 --- a/pkg/tui/components/editor/completions/command_test.go +++ b/pkg/tui/components/editor/completions/command_test.go @@ -75,3 +75,126 @@ func TestCommandCompletionMatchModeIsPrefix(t *testing.T) { c := NewCommandCompletion(nil) assert.Equal(t, completion.MatchPrefix, c.MatchMode()) } + +// categoriesWithArgumentCommand adds a command with a CompleteArgument +// provider (mirroring /toolset-restart) alongside one with none, so tests +// can exercise both the hit and the no-provider miss paths. +func categoriesWithArgumentCommand() []commands.Category { + return []commands.Category{ + { + Name: "Session", + Commands: []commands.Item{ + {ID: "session.exit", Label: "Exit", SlashCommand: "/exit"}, + { + ID: "session.toolset.restart", + Label: "Restart Toolset", + SlashCommand: "/toolset-restart", + CompleteArgument: func() []commands.ArgumentCandidate { + return []commands.ArgumentCandidate{ + {Label: "github", Description: "MCP · ready"}, + {Label: "filesystem", Description: "Built-in · ready", Disabled: true}, + } + }, + }, + }, + }, + } +} + +func TestCommandCompletionArgumentItems_Hit(t *testing.T) { + t.Parallel() + + c, ok := NewCommandCompletion(categoriesWithArgumentCommand()).(ArgumentCompleter) + require.True(t, ok, "commandCompletion must implement ArgumentCompleter") + + items, matched := c.ArgumentItems("/toolset-restart gi") + require.True(t, matched) + require.Len(t, items, 2) + + assert.Equal(t, "github", items[0].Label) + assert.Equal(t, "/toolset-restart github", items[0].Value) + assert.False(t, items[0].Disabled) + assert.NotContains(t, items[0].Description, "not restartable") + + assert.Equal(t, "filesystem", items[1].Label) + assert.Equal(t, "/toolset-restart filesystem", items[1].Value) + assert.True(t, items[1].Disabled) + assert.Contains(t, items[1].Description, "not restartable") +} + +func TestCommandCompletionArgumentItems_UnknownCommand(t *testing.T) { + t.Parallel() + + c, ok := NewCommandCompletion(categoriesWithArgumentCommand()).(ArgumentCompleter) + require.True(t, ok) + + items, matched := c.ArgumentItems("/nope something") + assert.False(t, matched) + assert.Nil(t, items) +} + +func TestCommandCompletionArgumentItems_CommandWithoutProvider(t *testing.T) { + t.Parallel() + + c, ok := NewCommandCompletion(categoriesWithArgumentCommand()).(ArgumentCompleter) + require.True(t, ok) + + // /exit exists but has no CompleteArgument, so it must miss just like an + // unknown command — the popup should not open. + items, matched := c.ArgumentItems("/exit") + assert.False(t, matched) + assert.Nil(t, items) +} + +func TestCommandCompletionArgumentItems_NoTrailingArgumentStillMatches(t *testing.T) { + t.Parallel() + + c, ok := NewCommandCompletion(categoriesWithArgumentCommand()).(ArgumentCompleter) + require.True(t, ok) + + items, matched := c.ArgumentItems("/toolset-restart") + require.True(t, matched) + assert.Len(t, items, 2) +} + +// TestCommandCompletionArgumentItems_QueriesFreshOnEachCall is a regression +// test for bug 2 (PR #3728): ArgumentItems must invoke CompleteArgument again +// on every call rather than reusing a result computed by an earlier call, so +// a single Tab always reflects the live toolset set. +func TestCommandCompletionArgumentItems_QueriesFreshOnEachCall(t *testing.T) { + t.Parallel() + + callCount := 0 + categories := []commands.Category{ + { + Name: "Session", + Commands: []commands.Item{ + { + ID: "session.toolset.restart", + Label: "Restart Toolset", + SlashCommand: "/toolset-restart", + CompleteArgument: func() []commands.ArgumentCandidate { + callCount++ + if callCount == 1 { + return []commands.ArgumentCandidate{{Label: "oldtool"}} + } + return []commands.ArgumentCandidate{{Label: "newtool"}} + }, + }, + }, + }, + } + c, ok := NewCommandCompletion(categories).(ArgumentCompleter) + require.True(t, ok) + + first, matched := c.ArgumentItems("/toolset-restart ") + require.True(t, matched) + require.Len(t, first, 1) + assert.Equal(t, "oldtool", first[0].Label) + + second, matched := c.ArgumentItems("/toolset-restart ") + require.True(t, matched) + require.Len(t, second, 1) + assert.Equal(t, "newtool", second[0].Label, "a second call must reflect the current candidates, not replay the first") + assert.Equal(t, 2, callCount, "CompleteArgument must be invoked again on each call") +} diff --git a/pkg/tui/components/editor/completions/completion.go b/pkg/tui/components/editor/completions/completion.go index 579ae3adf..8aa0d22ce 100644 --- a/pkg/tui/components/editor/completions/completion.go +++ b/pkg/tui/components/editor/completions/completion.go @@ -25,3 +25,16 @@ type AsyncLoader interface { // It returns a channel that receives the items when loading is complete. LoadItemsAsync(ctx context.Context) <-chan []completion.Item } + +// ArgumentCompleter is an optional interface for a Completion source that can +// also complete a command's ARGUMENT (as opposed to the command name +// itself). It is command-agnostic: it works for any commands.Item exposing a +// CompleteArgument provider, not just /toolset-restart. +type ArgumentCompleter interface { + // ArgumentItems returns the completion items for the argument of the + // command found at the start of line (e.g. "/toolset-restart git"), and + // whether line matched a command with argument candidates at all. A + // false second value means the caller should not open a popup (unknown + // command, or a command without an argument provider). + ArgumentItems(line string) ([]completion.Item, bool) +} diff --git a/pkg/tui/components/editor/editor.go b/pkg/tui/components/editor/editor.go index 8c86c2407..d2ffaefcf 100644 --- a/pkg/tui/components/editor/editor.go +++ b/pkg/tui/components/editor/editor.go @@ -67,6 +67,15 @@ type Editor interface { layout.Focusable SetWorking(working bool) tea.Cmd AcceptSuggestion() tea.Cmd + // TryStartArgumentCompletion opens the argument-completion popup for the + // slash command at the start of the editor's value (e.g. + // "/toolset-restart "), if that command exposes argument candidates. + // Called from the Tab/switchFocus path, never from editor keypress + // handling, so a bare space after a command never auto-opens a popup — + // only Tab does. Returns nil (leaving editor state untouched) when + // there's nothing to complete, so the caller can fall through to its own + // Tab handling (e.g. focus switch). + TryStartArgumentCompletion() tea.Cmd ScrollByWheel(delta int) // Value returns the current editor content Value() string @@ -123,6 +132,11 @@ type editor struct { // completionWord stores the word being completed completionWord string currentCompletion completions.Completion + // argumentPrefix is the exact "/slashCommand " prefix active during an + // argument-completion session (non-empty only then). It disambiguates + // full-line-splice argument selection (see the completion.SelectedMsg + // case) from the trigger-word splice used for name completion (/, @). + argumentPrefix string suggestion string hasSuggestion bool @@ -716,6 +730,20 @@ func (e *editor) Update(msg tea.Msg) (layout.Model, tea.Cmd) { return e, cmd case completion.SelectedMsg: + if e.argumentPrefix != "" { + // Argument-mode selection: Value is the FULL command line (e.g. + // "/toolset-restart github"), not a single word, so it replaces + // the editor content wholesale rather than splicing a trigger word. + if msg.AutoSubmit { + cmd := e.resetAndSend(msg.Value) + return e, cmd + } + e.textarea.SetValue(msg.Value) + e.textarea.MoveToEnd() + e.clearSuggestion() + return e, nil + } + if e.currentCompletion == nil { return e, nil } @@ -765,6 +793,7 @@ func (e *editor) Update(msg tea.Msg) (layout.Model, tea.Cmd) { case completion.ClosedMsg: e.completionWord = "" e.currentCompletion = nil + e.argumentPrefix = "" e.refreshSuggestion() // Reset file loading state e.fileLoadStarted = false @@ -1064,6 +1093,20 @@ func (e *editor) handleGraphemeBackspace() (layout.Model, tea.Cmd) { // updateCompletionQuery sends the appropriate completion message based on current editor state. // It returns a command that either updates the completion query or closes the completion popup. func (e *editor) updateCompletionQuery() tea.Cmd { + if e.argumentPrefix != "" { + value := e.textarea.Value() + if query, ok := strings.CutPrefix(value, e.argumentPrefix); ok { + return core.CmdHandler(completion.QueryMsg{Query: query}) + } + // Value no longer starts with the argument prefix (e.g. the command + // name itself was edited) - end the argument-completion session. + e.argumentPrefix = "" + e.currentCompletion = nil + e.completionWord = "" + e.clearSuggestion() + return core.CmdHandler(completion.CloseMsg{}) + } + currentWord := e.textarea.Word() if e.currentCompletion != nil && strings.HasPrefix(currentWord, e.currentCompletion.Trigger()) { @@ -1131,8 +1174,6 @@ func (e *editor) startFullFileLoad() tea.Cmd { } func (e *editor) startCompletion(c completions.Completion) tea.Cmd { - e.currentCompletion = c - // For @ trigger, open instantly with paste items + "Browse files…" and start async file loading if c.Trigger() == "@" { items := e.getPasteCompletionItems() @@ -1147,10 +1188,7 @@ func (e *editor) startCompletion(c completions.Completion) tea.Cmd { Pinned: true, }) - openCmd := core.CmdHandler(completion.OpenMsg{ - Items: items, - MatchMode: c.MatchMode(), - }) + openCmd := e.openCompletion(c, items) // Start initial shallow file loading immediately loadCmd := e.startInitialFileLoad() @@ -1158,14 +1196,57 @@ func (e *editor) startCompletion(c completions.Completion) tea.Cmd { return tea.Batch(openCmd, loadCmd) } - items := c.Items() + return e.openCompletion(c, c.Items()) +} +// openCompletion activates c as the current completion source and opens the +// popup with the given items. Shared by name-completion (startCompletion) +// and argument-mode (TryStartArgumentCompletion), which supply different +// item sets for the same completion source. +func (e *editor) openCompletion(c completions.Completion, items []completion.Item) tea.Cmd { + e.currentCompletion = c return core.CmdHandler(completion.OpenMsg{ Items: items, MatchMode: c.MatchMode(), }) } +// TryStartArgumentCompletion implements [Editor]. See the interface doc for +// the full contract. +func (e *editor) TryStartArgumentCompletion() tea.Cmd { + value := e.textarea.Value() + // Require a space so a command name that's still being typed (and whose + // name-completion popup narrows it) never gets reinterpreted as an + // argument-completion trigger. + if !strings.Contains(value, " ") { + return nil + } + + for _, c := range e.completions { + argCompleter, ok := c.(completions.ArgumentCompleter) + if !ok { + continue + } + + items, matched := argCompleter.ArgumentItems(value) + if !matched || len(items) == 0 { + continue + } + + slashCommand, query, _ := strings.Cut(value, " ") + e.argumentPrefix = slashCommand + " " + + // Sequenced (not batched) so the popup opens with the full item set + // before the query narrows it, regardless of scheduling order. + return tea.Sequence( + e.openCompletion(c, items), + core.CmdHandler(completion.QueryMsg{Query: query}), + ) + } + + return nil +} + // startInitialFileLoad starts a shallow file scan for immediate display. // It loads ~100 files from 2 levels deep for a snappy initial UX. func (e *editor) startInitialFileLoad() tea.Cmd { diff --git a/pkg/tui/styles/styles.go b/pkg/tui/styles/styles.go index ddeec6129..d8d8a9d75 100644 --- a/pkg/tui/styles/styles.go +++ b/pkg/tui/styles/styles.go @@ -545,6 +545,23 @@ var ( Foreground(TextMuted). Italic(true). Align(lipgloss.Center) + + // CompletionDisabledStyle renders a completion item that is shown for + // context but cannot be submitted (e.g. a non-restartable toolset for + // /toolset-restart): dim and unbolded so it reads as inert next to the + // selectable entries. + CompletionDisabledStyle = BaseStyle. + Foreground(TextMuted). + Italic(true) + + // CompletionDisabledSelectedStyle is CompletionDisabledStyle with the + // cursor position still visible (underline) but without the selected + // blue background, so the cursor can rest on a disabled row without it + // looking submittable. + CompletionDisabledSelectedStyle = CompletionDisabledStyle. + Underline(true) + + CompletionDisabledDescStyle = CompletionDisabledStyle ) // Agent and transfer badge styles diff --git a/pkg/tui/styles/theme.go b/pkg/tui/styles/theme.go index 1f869f3b6..20a1792b9 100644 --- a/pkg/tui/styles/theme.go +++ b/pkg/tui/styles/theme.go @@ -1391,6 +1391,9 @@ func rebuildStyles() { CompletionDescStyle = BaseStyle.Foreground(TextSecondary) CompletionSelectedDescStyle = CompletionDescStyle.Foreground(White).Background(MobyBlue) CompletionNoResultsStyle = BaseStyle.Foreground(TextMuted).Italic(true).Align(lipgloss.Center) + CompletionDisabledStyle = BaseStyle.Foreground(TextMuted).Italic(true) + CompletionDisabledSelectedStyle = CompletionDisabledStyle.Underline(true) + CompletionDisabledDescStyle = CompletionDisabledStyle // Agent badge styles AgentBadgeStyle = BaseStyle. diff --git a/pkg/tui/switchfocus_test.go b/pkg/tui/switchfocus_test.go new file mode 100644 index 000000000..a9d0d4d94 --- /dev/null +++ b/pkg/tui/switchfocus_test.go @@ -0,0 +1,92 @@ +package tui + +import ( + "testing" + + tea "charm.land/bubbletea/v2" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestSwitchFocusArgumentCompletion covers the Tab/switchFocus wiring: an +// open argument-completion popup (e.g. "/toolset-restart ") must win over +// the normal editor<->content focus toggle, but plain text must still +// switch focus as before. +func TestSwitchFocusArgumentCompletion(t *testing.T) { + t.Parallel() + + t.Run("argument completion available: focus stays on editor", func(t *testing.T) { + t.Parallel() + + m, ed := newTestModel(t) + m.focusedPanel = PanelEditor + type argCompletionOpenedMsg struct{} + ed.argumentCompletionCmd = func() tea.Msg { return argCompletionOpenedMsg{} } + + _, cmd := m.switchFocus() + + assert.Equal(t, PanelEditor, m.focusedPanel, "focus should not switch when the popup opens") + require.NotNil(t, cmd) + msgs := collectMsgs(cmd) + require.Len(t, msgs, 1) + _, ok := msgs[0].(argCompletionOpenedMsg) + assert.True(t, ok, "should return the command from TryStartArgumentCompletion") + }) + + t.Run("no argument completion: focus switches to content as before", func(t *testing.T) { + t.Parallel() + + m, ed := newTestModel(t) + m.focusedPanel = PanelEditor + ed.argumentCompletionCmd = nil + + _, _ = m.switchFocus() + + assert.Equal(t, PanelContent, m.focusedPanel, "focus should switch when there's nothing to complete") + }) + + t.Run("argument completion available takes priority over a pending suggestion", func(t *testing.T) { + t.Parallel() + + // Regression test: a stale ghost suggestion (e.g. a history match like + // "/toolset-restart oldtool" left over from a previous run) must not + // win the race for Tab and get silently accepted in place of opening a + // fresh argument-completion popup - that was bug 2, and the fix is to + // check TryStartArgumentCompletion before AcceptSuggestion in + // switchFocus. + m, ed := newTestModel(t) + m.focusedPanel = PanelEditor + type argCompletionOpenedMsg struct{} + type suggestionAcceptedMsg struct{} + ed.argumentCompletionCmd = func() tea.Msg { return argCompletionOpenedMsg{} } + ed.acceptSuggestionCmd = func() tea.Msg { return suggestionAcceptedMsg{} } + + _, cmd := m.switchFocus() + + assert.Equal(t, PanelEditor, m.focusedPanel) + require.NotNil(t, cmd) + msgs := collectMsgs(cmd) + require.Len(t, msgs, 1) + _, ok := msgs[0].(argCompletionOpenedMsg) + assert.True(t, ok, "argument completion must win over accepting a pending suggestion") + }) + + t.Run("no argument completion falls back to accepting a pending suggestion", func(t *testing.T) { + t.Parallel() + + m, ed := newTestModel(t) + m.focusedPanel = PanelEditor + ed.argumentCompletionCmd = nil + type suggestionAcceptedMsg struct{} + ed.acceptSuggestionCmd = func() tea.Msg { return suggestionAcceptedMsg{} } + + _, cmd := m.switchFocus() + + assert.Equal(t, PanelEditor, m.focusedPanel, "focus should not switch when a suggestion was accepted") + require.NotNil(t, cmd) + msgs := collectMsgs(cmd) + require.Len(t, msgs, 1) + _, ok := msgs[0].(suggestionAcceptedMsg) + assert.True(t, ok) + }) +} diff --git a/pkg/tui/tui.go b/pkg/tui/tui.go index 3f23c8177..38a26e86a 100644 --- a/pkg/tui/tui.go +++ b/pkg/tui/tui.go @@ -2425,7 +2425,16 @@ func parseCtrlNumberKey(msg tea.KeyPressMsg) int { func (m *appModel) switchFocus() (tea.Model, tea.Cmd) { switch m.focusedPanel { case PanelEditor: - // Check if editor has a suggestion to accept first + // Tab-triggered argument completion (e.g. "/toolset-restart ") takes + // priority over both suggestion-acceptance and switching focus; it's a + // no-op when not applicable. Checked first so a stale ghost suggestion + // left over from history (e.g. a previous "/toolset-restart ") + // never gets silently accepted in place of opening a fresh, current + // candidate popup - see regression test for the bug this guards against. + if cmd := m.editor.TryStartArgumentCompletion(); cmd != nil { + return m, cmd + } + // Otherwise, accept a pending suggestion if there is one. if cmd := m.editor.AcceptSuggestion(); cmd != nil { return m, cmd } diff --git a/pkg/tui/tui_exit_test.go b/pkg/tui/tui_exit_test.go index d6b907ae6..9bd3c852d 100644 --- a/pkg/tui/tui_exit_test.go +++ b/pkg/tui/tui_exit_test.go @@ -65,6 +65,13 @@ func (m *mockChatPage) Help() help.KeyMap { return nil } // mockEditor implements editor.Editor for testing. type mockEditor struct { cleanupCalled bool + // argumentCompletionCmd is returned by TryStartArgumentCompletion, letting + // tests simulate an open (or not-applicable) argument-completion popup. + argumentCompletionCmd tea.Cmd + // acceptSuggestionCmd is returned by AcceptSuggestion, letting tests + // simulate a pending ghost suggestion (e.g. a history match) that would + // otherwise compete with argument completion for the same Tab press. + acceptSuggestionCmd tea.Cmd } func (m *mockEditor) Init() tea.Cmd { return nil } @@ -74,7 +81,8 @@ func (m *mockEditor) SetSize(int, int) tea.Cmd { return nil } func (m *mockEditor) Focus() tea.Cmd { return nil } func (m *mockEditor) Blur() tea.Cmd { return nil } func (m *mockEditor) SetWorking(bool) tea.Cmd { return nil } -func (m *mockEditor) AcceptSuggestion() tea.Cmd { return nil } +func (m *mockEditor) AcceptSuggestion() tea.Cmd { return m.acceptSuggestionCmd } +func (m *mockEditor) TryStartArgumentCompletion() tea.Cmd { return m.argumentCompletionCmd } func (m *mockEditor) ScrollByWheel(int) {} func (m *mockEditor) Value() string { return "" } func (m *mockEditor) SetValue(string) {}