From 3fd111d594a014de2e0256c5221ea98b9a1687c0 Mon Sep 17 00:00:00 2001 From: KRATOS Date: Mon, 20 Jul 2026 19:35:26 +0530 Subject: [PATCH 1/2] fix(tui): label model rows by id when the description is prose The model picker (provider wizard AND onboarding, which share displayLabel) labelled each row with the provider's Description, falling back to the ID only when the description was "generic". That is right when Description holds a short 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: 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. Observed in a live picker with two identical "Open-weight GPT model for self-hosted reasoning..." rows and two identical "Kimi multimodal agent model..." rows -- the distinguishing ID was visible only in the detail line of whichever row happened to be highlighted, so the list could not be used to pick between them. providerWizardGenericModelDescription cannot catch these: it rejects vague descriptions ("live model", "catalog default"), whereas these are specific, just shared and sentence-shaped. So gate on shape instead -- a display name is short and few-worded; anything longer is prose and the ID (unique by construction) makes the better label. Friendly names are still preferred where providers supply them, so the existing behaviour is unchanged for well-behaved catalogs. --- internal/tui/provider_wizard.go | 45 +++++++++++++++++++++++++--- internal/tui/provider_wizard_test.go | 34 +++++++++++++++++++++ 2 files changed, 75 insertions(+), 4 deletions(-) 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) + } +} From 82d7a93027c26a83c124790b2eb794e634553ce1 Mon Sep 17 00:00:00 2001 From: KRATOS Date: Mon, 20 Jul 2026 20:05:14 +0530 Subject: [PATCH 2/2] fix(tui): keep the sidebar open under the / command palette sidebarAvailable grouped suggestionsActive() with the full-screen overlays (setup, wizards, pickers), whose stated rationale is that they "take over the chat column and render at full width". That is not true of the `/` palette: suggestionOverlay draws it through centerRenderedBlock at minInt(width, suggestionPaletteMaxWidth), a centred box capped at 76 cells floating over the chat column. Treating it as full-screen collapsed the whole layout on every `/`: the sidebar disappeared, the plan panel dropped out of it and re-rendered inline above the composer, and the transcript re-wrapped to full width -- a jarring reflow for a popup, and worst mid-run with a live plan on screen. It cannot overlap the sidebar either: twoColumnTranscriptView renders the overlay with width = chatColumnWidth, so the palette is capped at min(chatColumn, 76) and centred inside the chat column. Mouse hit-testing is unaffected because sidebarLineAtMouse carries its own suggestionsActive() guard, so clicks still reach the palette rather than the sidebar rows beneath it. The genuinely full-width overlays keep suppressing the sidebar, pinned by the new test alongside the palette case. --- internal/tui/sidebar.go | 24 ++++++++++++---- internal/tui/sidebar_test.go | 55 ++++++++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 5 deletions(-) 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") + } +}