diff --git a/internal/tui/provider_wizard.go b/internal/tui/provider_wizard.go index 6c2789af7..8f4ee8bb3 100644 --- a/internal/tui/provider_wizard.go +++ b/internal/tui/provider_wizard.go @@ -1959,12 +1959,49 @@ func (wizard *providerWizardState) filteredModels() []providerWizardModel { return models } +// A model Description is only a good row label when it reads like a display +// NAME — "Grok 4.3" for x-ai/grok-4.3 — which is what the field is for, and why +// the row prefers it over an ugly raw ID. +// +// Providers do not all honour that. Aggregators commonly return a prose blurb +// instead ("Fast DeepSeek model for efficient chat, coding help, and agent +// loops"), which fails as a label twice over: it overflows the row, and because +// such blurbs are written per model FAMILY rather than per model, several +// DIFFERENT models collapse to byte-identical rows. A picker then offers two +// visually identical entries with the distinguishing ID visible only in the +// detail line of whichever one is highlighted. +// +// providerWizardGenericModelDescription cannot catch these: it rejects +// descriptions that are vague ("live model", "catalog default"), whereas these +// are specific — just shared and sentence-shaped. So the test is shape, not +// wording: a name is short and few-worded; anything longer is prose, and the ID +// (unique by construction) makes the better label. +const ( + providerWizardMaxModelNameChars = 28 + providerWizardMaxModelNameWords = 4 +) + +func providerWizardDescriptionIsDisplayName(description string) bool { + trimmed := strings.TrimSpace(description) + if trimmed == "" || providerWizardGenericModelDescription(trimmed) { + return false + } + if len(trimmed) > providerWizardMaxModelNameChars { + return false + } + return len(strings.Fields(trimmed)) <= providerWizardMaxModelNameWords +} + func (model providerWizardModel) displayLabel() string { - description := strings.TrimSpace(model.Description) - if description != "" && !providerWizardGenericModelDescription(description) { - return description + if providerWizardDescriptionIsDisplayName(model.Description) { + return strings.TrimSpace(model.Description) + } + if id := strings.TrimSpace(model.ID); id != "" { + return id } - return model.ID + // ID-less sentinel rows built in currentModel ("model name required", + // "no matching models") would otherwise render blank. + return strings.TrimSpace(model.Description) } func (model providerWizardModel) secondaryText() string { diff --git a/internal/tui/provider_wizard_test.go b/internal/tui/provider_wizard_test.go index bbb32fc17..4d095849e 100644 --- a/internal/tui/provider_wizard_test.go +++ b/internal/tui/provider_wizard_test.go @@ -1787,3 +1787,37 @@ func providerWizardIDs(descriptors []providercatalog.Descriptor) []string { } return ids } + +// A provider that returns prose blurbs instead of display names must not +// collapse distinct models into identical rows. Reproduces the reported picker +// where two "Open-weight GPT model..." and two "Kimi multimodal agent model..." +// rows were indistinguishable, with the ID visible only for the highlighted one. +func TestProviderWizardModelRowsStayDistinctWithProseDescriptions(t *testing.T) { + wizard := &providerWizardState{ + step: providerWizardStepModel, + modelSource: "models.dev", + models: []providerWizardModel{ + {ID: "gpt-oss-120b", Description: "Open-weight GPT model for self-hosted reasoning and instruction-following workloads"}, + {ID: "gpt-oss-20b", Description: "Open-weight GPT model for self-hosted reasoning and instruction-following workloads"}, + {ID: "kimi-k2-thinking", Description: "Kimi multimodal agent model for visual understanding, coding, and planning"}, + {ID: "kimi-k2-instruct", Description: "Kimi multimodal agent model for visual understanding, coding, and planning"}, + {ID: "x-ai/grok-4.3", Description: "Grok 4.3"}, + }, + } + view := plainRender(t, strings.Join(wizard.renderModelStep(96), "\n")) + + // every prose-described model is identified by its unique ID + for _, id := range []string{"gpt-oss-120b", "gpt-oss-20b", "kimi-k2-thinking", "kimi-k2-instruct"} { + if !strings.Contains(view, id) { + t.Errorf("model %q is not identifiable in the list:\n%s", id, view) + } + } + // a genuine display name is still preferred over the raw id + if !strings.Contains(view, "Grok 4.3") { + t.Errorf("friendly display name was lost:\n%s", view) + } + // the prose blurb must not be a row label + if strings.Contains(view, "❯ Open-weight GPT model") { + t.Errorf("prose blurb still used as a row label:\n%s", view) + } +} diff --git a/internal/tui/sidebar.go b/internal/tui/sidebar.go index 966debe35..13b87a14b 100644 --- a/internal/tui/sidebar.go +++ b/internal/tui/sidebar.go @@ -94,12 +94,26 @@ func (m model) sidebarAvailable() bool { if widthTier(m.width) < tierMedium { return false } - // Full-screen overlays (setup, wizards, pickers, the empty-state suggestion - // list) take over the chat column and render at full width; suppress the - // second column while any is active so their geometry and mouse hit-testing - // stay full-width as before. + // Full-screen overlays (setup, wizards, pickers) take over the chat column and + // render at full width; suppress the second column while any is active so + // their geometry and mouse hit-testing stay full-width as before. + // + // The `/` command palette is deliberately NOT in this list. It is not a + // full-screen overlay: suggestionOverlay draws it via centerRenderedBlock at + // minInt(width, suggestionPaletteMaxWidth) — a centred box capped at 76 cells + // — floating over the chat column rather than replacing it. Suppressing the + // sidebar for it collapsed the whole layout on every keystroke of `/`: the + // plan panel dropped out of the sidebar and re-rendered inline at the bottom + // and the transcript re-wrapped to full width, which is at its most jarring + // mid-run with a live plan on screen. Since the sidebar already requires the + // medium tier (>= 80 cells) the palette is always strictly narrower than the + // chat column here, so the two never contend for the same cells. + // + // Mouse hit-testing is unaffected: sidebarLineAtMouse carries its own + // suggestionsActive() guard, so clicks still go to the palette and not to the + // sidebar rows underneath it. if m.setup.visible || m.helpOverlay || m.leaderHelpOverlay || m.providerWizard != nil || m.mcpAddWizard != nil || - m.mcpManager != nil || m.picker != nil || m.suggestionsActive() { + m.mcpManager != nil || m.picker != nil { return false } // Home/welcome screen: stay single-column until there's real conversation, so diff --git a/internal/tui/sidebar_test.go b/internal/tui/sidebar_test.go index 45ffdac6a..2c0d15cc7 100644 --- a/internal/tui/sidebar_test.go +++ b/internal/tui/sidebar_test.go @@ -631,3 +631,58 @@ func TestTwoColumnTranscriptViewWidth(t *testing.T) { func stripSidebar(lines []string) string { return ansiPattern.ReplaceAllString(strings.Join(lines, "\n"), "") } + +// The `/` command palette must NOT collapse the sidebar. It is a centred box +// capped at suggestionPaletteMaxWidth floating over the chat column, not a +// full-screen overlay — suppressing the second column for it dropped the plan +// out of the sidebar and re-rendered it inline at the bottom on every `/`, +// which is at its most disruptive mid-run with a live plan on screen. The +// genuinely full-width overlays must still suppress it. +func TestSidebarSurvivesCommandPalette(t *testing.T) { + base := func() model { + m := runningPlanModel(t, 3) + m.altScreen = true + m.height = 40 + m.headerPrinted = true + m.transcript = append(m.transcript, transcriptRow{kind: rowToolCall, tool: "read_file", detail: "main.go"}) + return m + } + + m := base() + if !m.sidebarActive() { + t.Fatal("precondition: sidebar should be active for a wide alt-screen model with a plan") + } + + // `/` palette open: sidebar stays, so the plan keeps its home and the layout + // does not reflow. + m.suggestions = []commandSuggestion{{Name: "/model", Desc: "Pick a model."}, {Name: "/plan", Desc: "Show planning mode status."}} + if !m.suggestionsActive() { + t.Fatal("precondition: suggestions should be active") + } + if !m.sidebarActive() { + t.Error("command palette must not collapse the sidebar") + } + if got := m.renderPinnedPlanPanel(m.chatColumnWidth(), 10); got != "" { + t.Errorf("plan must stay in the sidebar, not fall back to the pinned panel:\n%s", got) + } + // The palette never contends for the sidebar's cells: the two-column path + // renders it at width = chatColumnWidth, so it is centred inside the chat + // column and cannot overlap the sidebar. + chatW := m.chatColumnWidth() + for _, line := range strings.Split(plainRender(t, m.suggestionOverlay(chatW)), "\n") { + if w := lipgloss.Width(line); w > chatW { + t.Errorf("palette line is %d wide, wider than the chat column %d — it would overlap the sidebar", w, chatW) + } + } + // Clicks still belong to the palette, not the sidebar rows beneath it. + if _, ok := m.sidebarLineAtMouse(tea.MouseClickMsg{Button: tea.MouseLeft}); ok { + t.Error("sidebar must not take mouse hits while the palette is open") + } + + // A genuinely full-width overlay still suppresses the sidebar. + full := base() + full.picker = &commandPicker{} + if full.sidebarActive() { + t.Error("a full-screen picker must still collapse the sidebar") + } +}