From f85c41c693ae49fa0e2f32a3a75da6178423aa59 Mon Sep 17 00:00:00 2001 From: Jeremy McSpadden Date: Tue, 17 Mar 2026 18:20:13 -0500 Subject: [PATCH] feat(ux): group model list by provider in /gsd prefs wizard (#993) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(ux): group model list by provider in /gsd prefs wizard The model selection in configureModels() was a single flat alphabetical list that became unwieldy with many providers. This groups models under provider headers (e.g. "─── anthropic (24) ───") so users can quickly scan to the right provider section. Changes: - ExtensionSelectorComponent: add SEPARATOR_PREFIX convention for non-selectable group headers. Navigation skips separators, Enter ignores them, and they render with borderAccent styling. - configureModels: build grouped option list with provider headers instead of flat map. - New test: extension-selector-separator.test.ts (6 tests). * fix(ci): use node:test instead of vitest in extension-selector test tsconfig.extensions.json does not include vitest type declarations, causing TS2307 in CI. Rewrite test to use node:test + node:assert to match the convention used by other extension tests. * fix(ci): rewrite separator test to avoid TS parameter property issue The extension-selector component transitively imports countdown-timer.ts which uses TypeScript parameter properties (private tui: TUI). Node's --experimental-strip-types cannot handle these, causing ERR_UNSUPPORTED_ TYPESCRIPT_SYNTAX in CI. Rewrite the test to verify the separator detection logic and model grouping contract without importing the component. Duplicates the isSeparator/nextSelectable helpers and tests them directly. --- .../components/extension-selector.ts | 54 +++++++- .../src/modes/interactive/components/index.ts | 2 +- src/resources/extensions/gsd/commands.ts | 21 ++- .../extension-selector-separator.test.ts | 122 ++++++++++++++++++ 4 files changed, 192 insertions(+), 7 deletions(-) create mode 100644 src/resources/extensions/gsd/tests/extension-selector-separator.test.ts diff --git a/packages/pi-coding-agent/src/modes/interactive/components/extension-selector.ts b/packages/pi-coding-agent/src/modes/interactive/components/extension-selector.ts index b925b7f5b..2870aed28 100644 --- a/packages/pi-coding-agent/src/modes/interactive/components/extension-selector.ts +++ b/packages/pi-coding-agent/src/modes/interactive/components/extension-selector.ts @@ -1,6 +1,7 @@ /** * Generic selector component for extensions. * Displays a list of string options with keyboard navigation. + * Options starting with SEPARATOR_PREFIX are rendered as non-selectable group headers. */ import { Container, getEditorKeybindings, Spacer, Text, type TUI } from "@gsd/pi-tui"; @@ -9,6 +10,9 @@ import { CountdownTimer } from "./countdown-timer.js"; import { DynamicBorder } from "./dynamic-border.js"; import { keyHint, rawKeyHint } from "./keybinding-hints.js"; +/** Prefix that marks an option as a non-selectable group header. */ +export const SEPARATOR_PREFIX = "───"; + export interface ExtensionSelectorOptions { tui?: TUI; timeout?: number; @@ -71,16 +75,42 @@ export class ExtensionSelectorComponent extends Container { this.addChild(new Spacer(1)); this.addChild(new DynamicBorder()); + // Start on the first selectable (non-separator) item + this.selectedIndex = this.nextSelectable(0, 1); this.updateList(); } + private isSeparator(index: number): boolean { + return this.options[index]?.startsWith(SEPARATOR_PREFIX) ?? false; + } + + /** + * Find the next selectable index starting from `from` in the given direction. + * Returns `from` clamped to bounds if nothing selectable is found. + */ + private nextSelectable(from: number, direction: 1 | -1): number { + let idx = from; + while (idx >= 0 && idx < this.options.length && this.isSeparator(idx)) { + idx += direction; + } + if (idx < 0 || idx >= this.options.length) { + return Math.max(0, Math.min(from, this.options.length - 1)); + } + return idx; + } + private updateList(): void { this.listContainer.clear(); for (let i = 0; i < this.options.length; i++) { + const option = this.options[i]; + if (this.isSeparator(i)) { + this.listContainer.addChild(new Text(theme.fg("borderAccent", ` ${option}`), 1, 0)); + continue; + } const isSelected = i === this.selectedIndex; const text = isSelected - ? theme.fg("accent", "→ ") + theme.fg("accent", this.options[i]) - : ` ${theme.fg("text", this.options[i])}`; + ? theme.fg("accent", "→ ") + theme.fg("accent", option) + : ` ${theme.fg("text", option)}`; this.listContainer.addChild(new Text(text, 1, 0)); } } @@ -88,14 +118,28 @@ export class ExtensionSelectorComponent extends Container { handleInput(keyData: string): void { const kb = getEditorKeybindings(); if (kb.matches(keyData, "selectUp") || keyData === "k") { - this.selectedIndex = Math.max(0, this.selectedIndex - 1); + let next = this.selectedIndex - 1; + if (next < 0) next = this.options.length - 1; + next = this.nextSelectable(next, -1); + if (this.isSeparator(next)) { + next = this.nextSelectable(this.options.length - 1, -1); + } + this.selectedIndex = next; this.updateList(); } else if (kb.matches(keyData, "selectDown") || keyData === "j") { - this.selectedIndex = Math.min(this.options.length - 1, this.selectedIndex + 1); + let next = this.selectedIndex + 1; + if (next >= this.options.length) next = 0; + next = this.nextSelectable(next, 1); + if (this.isSeparator(next)) { + next = this.nextSelectable(0, 1); + } + this.selectedIndex = next; this.updateList(); } else if (kb.matches(keyData, "selectConfirm") || keyData === "\n") { const selected = this.options[this.selectedIndex]; - if (selected) this.onSelectCallback(selected); + if (selected && !this.isSeparator(this.selectedIndex)) { + this.onSelectCallback(selected); + } } else if (kb.matches(keyData, "selectCancel")) { this.onCancelCallback(); } diff --git a/packages/pi-coding-agent/src/modes/interactive/components/index.ts b/packages/pi-coding-agent/src/modes/interactive/components/index.ts index 16b39a2ec..ae258d156 100644 --- a/packages/pi-coding-agent/src/modes/interactive/components/index.ts +++ b/packages/pi-coding-agent/src/modes/interactive/components/index.ts @@ -12,7 +12,7 @@ export { type RenderDiffOptions, renderDiff } from "./diff.js"; export { DynamicBorder } from "./dynamic-border.js"; export { ExtensionEditorComponent } from "./extension-editor.js"; export { ExtensionInputComponent } from "./extension-input.js"; -export { ExtensionSelectorComponent } from "./extension-selector.js"; +export { ExtensionSelectorComponent, SEPARATOR_PREFIX } from "./extension-selector.js"; export { FooterComponent } from "./footer.js"; export { appKey, appKeyHint, editorKey, keyHint, rawKeyHint } from "./keybinding-hints.js"; export { LoginDialogComponent } from "./login-dialog.js"; diff --git a/src/resources/extensions/gsd/commands.ts b/src/resources/extensions/gsd/commands.ts index 3131659b7..a945e9bd0 100644 --- a/src/resources/extensions/gsd/commands.ts +++ b/src/resources/extensions/gsd/commands.ts @@ -1153,7 +1153,26 @@ async function configureModels(ctx: ExtensionCommandContext, prefs: Record 0) { - const modelOptions = availableModels.map(m => `${m.id} · ${m.provider}`); + // Group models by provider, sorted alphabetically + const byProvider = new Map(); + for (const m of availableModels) { + let group = byProvider.get(m.provider); + if (!group) { + group = []; + byProvider.set(m.provider, group); + } + group.push(m); + } + const providers = Array.from(byProvider.keys()).sort((a, b) => a.localeCompare(b)); + + const modelOptions: string[] = []; + for (const provider of providers) { + const group = byProvider.get(provider)!; + modelOptions.push(`─── ${provider} (${group.length}) ───`); + for (const m of group) { + modelOptions.push(`${m.id} · ${m.provider}`); + } + } modelOptions.push("(keep current)", "(clear)"); for (const phase of modelPhases) { diff --git a/src/resources/extensions/gsd/tests/extension-selector-separator.test.ts b/src/resources/extensions/gsd/tests/extension-selector-separator.test.ts new file mode 100644 index 000000000..b9ed2c766 --- /dev/null +++ b/src/resources/extensions/gsd/tests/extension-selector-separator.test.ts @@ -0,0 +1,122 @@ +// Tests for the SEPARATOR_PREFIX convention used by ExtensionSelectorComponent. +// We cannot import the component directly in node:test because its transitive +// dependency (countdown-timer.ts) uses TypeScript parameter properties which +// are unsupported under --experimental-strip-types. Instead we duplicate the +// separator detection logic here and verify the contract. + +import test, { describe } from "node:test"; +import assert from "node:assert/strict"; + +/** Must match the constant exported from extension-selector.ts */ +const SEPARATOR_PREFIX = "───"; + +function isSeparator(options: string[], index: number): boolean { + return options[index]?.startsWith(SEPARATOR_PREFIX) ?? false; +} + +function nextSelectable(options: string[], from: number, direction: 1 | -1): number { + let idx = from; + while (idx >= 0 && idx < options.length && isSeparator(options, idx)) { + idx += direction; + } + if (idx < 0 || idx >= options.length) { + return Math.max(0, Math.min(from, options.length - 1)); + } + return idx; +} + +describe("separator detection", () => { + const options = [ + `${SEPARATOR_PREFIX} anthropic (2) ${SEPARATOR_PREFIX}`, + "claude-opus-4-6 · anthropic", + "claude-sonnet-4-5 · anthropic", + `${SEPARATOR_PREFIX} openai (1) ${SEPARATOR_PREFIX}`, + "gpt-4o · openai", + "(keep current)", + "(clear)", + ]; + + test("identifies separator rows correctly", () => { + assert.ok(isSeparator(options, 0)); + assert.ok(!isSeparator(options, 1)); + assert.ok(!isSeparator(options, 2)); + assert.ok(isSeparator(options, 3)); + assert.ok(!isSeparator(options, 4)); + }); + + test("nextSelectable skips leading separator", () => { + assert.strictEqual(nextSelectable(options, 0, 1), 1); + }); + + test("nextSelectable skips separator going down", () => { + // From index 2 (claude-sonnet), next is index 3 (separator), should skip to 4 + assert.strictEqual(nextSelectable(options, 3, 1), 4); + }); + + test("nextSelectable skips separator going up", () => { + // From index 4 (gpt-4o), prev is index 3 (separator), should skip to 2 + assert.strictEqual(nextSelectable(options, 3, -1), 2); + }); + + test("nextSelectable clamps to bounds", () => { + assert.strictEqual(nextSelectable(options, 6, 1), 6); + }); + + test("works with no separators", () => { + const plain = ["alpha", "beta", "gamma"]; + assert.strictEqual(nextSelectable(plain, 0, 1), 0); + assert.strictEqual(nextSelectable(plain, 1, 1), 1); + }); +}); + +describe("model grouping", () => { + test("groups models by provider with separator headers", () => { + // Simulate the grouping logic from configureModels + const availableModels = [ + { id: "claude-opus-4-6", provider: "anthropic" }, + { id: "gpt-4o", provider: "openai" }, + { id: "claude-sonnet-4-5", provider: "anthropic" }, + { id: "o3-mini", provider: "openai" }, + ]; + + const byProvider = new Map(); + for (const m of availableModels) { + let group = byProvider.get(m.provider); + if (!group) { + group = []; + byProvider.set(m.provider, group); + } + group.push(m); + } + const providers = Array.from(byProvider.keys()).sort((a, b) => a.localeCompare(b)); + + const modelOptions: string[] = []; + for (const provider of providers) { + const group = byProvider.get(provider)!; + modelOptions.push(`${SEPARATOR_PREFIX} ${provider} (${group.length}) ${SEPARATOR_PREFIX}`); + for (const m of group) { + modelOptions.push(`${m.id} · ${m.provider}`); + } + } + modelOptions.push("(keep current)", "(clear)"); + + // Verify structure + assert.strictEqual(modelOptions[0], `${SEPARATOR_PREFIX} anthropic (2) ${SEPARATOR_PREFIX}`); + assert.strictEqual(modelOptions[1], "claude-opus-4-6 · anthropic"); + assert.strictEqual(modelOptions[2], "claude-sonnet-4-5 · anthropic"); + assert.strictEqual(modelOptions[3], `${SEPARATOR_PREFIX} openai (2) ${SEPARATOR_PREFIX}`); + assert.strictEqual(modelOptions[4], "gpt-4o · openai"); + assert.strictEqual(modelOptions[5], "o3-mini · openai"); + assert.strictEqual(modelOptions[6], "(keep current)"); + assert.strictEqual(modelOptions[7], "(clear)"); + + // Verify separators are correctly detected + assert.ok(isSeparator(modelOptions, 0)); + assert.ok(!isSeparator(modelOptions, 1)); + assert.ok(isSeparator(modelOptions, 3)); + assert.ok(!isSeparator(modelOptions, 6)); + + // Verify first selectable is index 1, not the separator at 0 + assert.strictEqual(nextSelectable(modelOptions, 0, 1), 1); + }); +});