From 17585b7f0b5de31d1d965260d5a28086613ebf41 Mon Sep 17 00:00:00 2001 From: Mario Zechner Date: Wed, 15 Apr 2026 17:13:06 +0200 Subject: [PATCH] fix(coding-agent): preserve scoped model order closes #3217 --- packages/coding-agent/CHANGELOG.md | 1 + .../interactive/components/model-selector.ts | 19 ++-- .../components/scoped-models-selector.ts | 35 +++---- .../src/modes/interactive/interactive-mode.ts | 63 +++--------- .../3217-scoped-model-order.test.ts | 98 +++++++++++++++++++ 5 files changed, 135 insertions(+), 81 deletions(-) create mode 100644 packages/coding-agent/test/suite/regressions/3217-scoped-model-order.test.ts diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index fedc9397..72562a0d 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -4,6 +4,7 @@ ### Fixed +- Fixed `/scoped-models` reordering to propagate into the `/model` scoped tab, preserving the user-defined scoped model order instead of re-sorting it ([#3217](https://github.com/badlogic/pi-mono/issues/3217)) - Fixed `session_shutdown` to fire on `SIGHUP` and `SIGTERM` in interactive, print, and RPC modes so extensions can run shutdown cleanup on those signal-driven exits ([#3212](https://github.com/badlogic/pi-mono/issues/3212)) - Fixed screenshot path parsing to handle lower case am/pm in macOS screenshot filenames ([#3194](https://github.com/badlogic/pi-mono/pull/3194) by [@jay-aye-see-kay](https://github.com/jay-aye-see-kay)) - Fixed interactive auto-retry status updates to show a live countdown during backoff instead of a static retry delay message ([#3187](https://github.com/badlogic/pi-mono/issues/3187)) diff --git a/packages/coding-agent/src/modes/interactive/components/model-selector.ts b/packages/coding-agent/src/modes/interactive/components/model-selector.ts index 9234a3d2..3acbfa86 100644 --- a/packages/coding-agent/src/modes/interactive/components/model-selector.ts +++ b/packages/coding-agent/src/modes/interactive/components/model-selector.ts @@ -168,16 +168,16 @@ export class ModelSelectorComponent extends Container implements Focusable { const refreshed = this.modelRegistry.find(scoped.model.provider, scoped.model.id); return refreshed ? { ...scoped, model: refreshed } : scoped; }); - this.scopedModelItems = this.sortModels( - this.scopedModels.map((scoped) => ({ - provider: scoped.model.provider, - id: scoped.model.id, - model: scoped.model, - })), - ); + this.scopedModelItems = this.scopedModels.map((scoped) => ({ + provider: scoped.model.provider, + id: scoped.model.id, + model: scoped.model, + })); this.activeModels = this.scope === "scoped" ? this.scopedModelItems : this.allModels; this.filteredModels = this.activeModels; - this.selectedIndex = Math.min(this.selectedIndex, Math.max(0, this.filteredModels.length - 1)); + const currentIndex = this.filteredModels.findIndex((item) => modelsAreEqual(this.currentModel, item.model)); + this.selectedIndex = + currentIndex >= 0 ? currentIndex : Math.min(this.selectedIndex, Math.max(0, this.filteredModels.length - 1)); } private sortModels(models: ModelItem[]): ModelItem[] { @@ -207,7 +207,8 @@ export class ModelSelectorComponent extends Container implements Focusable { if (this.scope === scope) return; this.scope = scope; this.activeModels = this.scope === "scoped" ? this.scopedModelItems : this.allModels; - this.selectedIndex = 0; + const currentIndex = this.activeModels.findIndex((item) => modelsAreEqual(this.currentModel, item.model)); + this.selectedIndex = currentIndex >= 0 ? currentIndex : 0; this.filterModels(this.searchInput.getValue()); if (this.scopeText) { this.scopeText.setText(this.getScopeText()); diff --git a/packages/coding-agent/src/modes/interactive/components/scoped-models-selector.ts b/packages/coding-agent/src/modes/interactive/components/scoped-models-selector.ts index 3f255e6e..18d39db8 100644 --- a/packages/coding-agent/src/modes/interactive/components/scoped-models-selector.ts +++ b/packages/coding-agent/src/modes/interactive/components/scoped-models-selector.ts @@ -70,22 +70,14 @@ interface ModelItem { export interface ModelsConfig { allModels: Model[]; - enabledModelIds: Set; - /** true if enabledModels setting is defined (empty = all enabled) */ - hasEnabledModelsFilter: boolean; + enabledModelIds: string[] | null; } export interface ModelsCallbacks { - /** Called when a model is toggled (session-only, no persist) */ - onModelToggle: (modelId: string, enabled: boolean) => void; + /** Called whenever the enabled model set or order changes (session-only, no persist) */ + onChange: (enabledModelIds: string[] | null) => void | Promise; /** Called when user wants to persist current selection to settings */ - onPersist: (enabledModelIds: string[]) => void; - /** Called when user enables all models. Returns list of all model IDs. */ - onEnableAll: (allModelIds: string[]) => void; - /** Called when user clears all models */ - onClearAll: () => void; - /** Called when user toggles all models for a provider. Returns affected model IDs. */ - onToggleProvider: (provider: string, modelIds: string[], enabled: boolean) => void; + onPersist: (enabledModelIds: string[] | null) => void | Promise; onCancel: () => void; } @@ -126,7 +118,7 @@ export class ScopedModelsSelectorComponent extends Container implements Focusabl this.allIds.push(fullId); } - this.enabledIds = config.hasEnabledModelsFilter ? [...config.enabledModelIds] : null; + this.enabledIds = config.enabledModelIds === null ? null : [...config.enabledModelIds]; this.filteredItems = this.buildItems(); // Header @@ -184,6 +176,10 @@ export class ScopedModelsSelectorComponent extends Container implements Focusabl this.footerText.setText(this.getFooterText()); } + private notifyChange(): void { + this.callbacks.onChange(this.enabledIds === null ? null : [...this.enabledIds]); + } + private updateList(): void { this.listContainer.clear(); @@ -254,6 +250,7 @@ export class ScopedModelsSelectorComponent extends Container implements Focusabl this.isDirty = true; this.selectedIndex += delta; this.refresh(); + this.notifyChange(); } } return; @@ -263,12 +260,10 @@ export class ScopedModelsSelectorComponent extends Container implements Focusabl if (matchesKey(data, Key.enter)) { const item = this.filteredItems[this.selectedIndex]; if (item) { - const wasAllEnabled = this.enabledIds === null; this.enabledIds = toggle(this.enabledIds, item.fullId); this.isDirty = true; - if (wasAllEnabled) this.callbacks.onClearAll(); - this.callbacks.onModelToggle(item.fullId, isEnabled(this.enabledIds, item.fullId)); this.refresh(); + this.notifyChange(); } return; } @@ -278,8 +273,8 @@ export class ScopedModelsSelectorComponent extends Container implements Focusabl const targetIds = this.searchInput.getValue() ? this.filteredItems.map((i) => i.fullId) : undefined; this.enabledIds = enableAll(this.enabledIds, this.allIds, targetIds); this.isDirty = true; - this.callbacks.onEnableAll(targetIds ?? this.allIds); this.refresh(); + this.notifyChange(); return; } @@ -288,8 +283,8 @@ export class ScopedModelsSelectorComponent extends Container implements Focusabl const targetIds = this.searchInput.getValue() ? this.filteredItems.map((i) => i.fullId) : undefined; this.enabledIds = clearAll(this.enabledIds, this.allIds, targetIds); this.isDirty = true; - this.callbacks.onClearAll(); this.refresh(); + this.notifyChange(); return; } @@ -304,15 +299,15 @@ export class ScopedModelsSelectorComponent extends Container implements Focusabl ? clearAll(this.enabledIds, this.allIds, providerIds) : enableAll(this.enabledIds, this.allIds, providerIds); this.isDirty = true; - this.callbacks.onToggleProvider(provider, providerIds, !allEnabled); this.refresh(); + this.notifyChange(); } return; } // Ctrl+S - Save/persist to settings if (matchesKey(data, Key.ctrl("s"))) { - this.callbacks.onPersist(this.enabledIds ?? [...this.allIds]); + this.callbacks.onPersist(this.enabledIds === null ? null : [...this.enabledIds]); this.isDirty = false; this.footerText.setText(this.getFooterText()); return; diff --git a/packages/coding-agent/src/modes/interactive/interactive-mode.ts b/packages/coding-agent/src/modes/interactive/interactive-mode.ts index d3da96a1..55a87f82 100644 --- a/packages/coding-agent/src/modes/interactive/interactive-mode.ts +++ b/packages/coding-agent/src/modes/interactive/interactive-mode.ts @@ -3673,35 +3673,25 @@ export class InteractiveMode { const hasSessionScope = sessionScopedModels.length > 0; // Build enabled model IDs from session state or settings - const enabledModelIds = new Set(); - let hasFilter = false; + let currentEnabledIds: string[] | null = null; if (hasSessionScope) { // Use current session's scoped models - for (const sm of sessionScopedModels) { - enabledModelIds.add(`${sm.model.provider}/${sm.model.id}`); - } - hasFilter = true; + currentEnabledIds = sessionScopedModels.map((scoped) => `${scoped.model.provider}/${scoped.model.id}`); } else { // Fall back to settings const patterns = this.settingsManager.getEnabledModels(); if (patterns !== undefined && patterns.length > 0) { - hasFilter = true; const scopedModels = await resolveModelScope(patterns, this.session.modelRegistry); - for (const sm of scopedModels) { - enabledModelIds.add(`${sm.model.provider}/${sm.model.id}`); - } + currentEnabledIds = scopedModels.map((scoped) => `${scoped.model.provider}/${scoped.model.id}`); } } - // Track current enabled state (session-only until persisted) - const currentEnabledIds = new Set(enabledModelIds); - let currentHasFilter = hasFilter; - // Helper to update session's scoped models (session-only, no persist) - const updateSessionModels = async (enabledIds: Set) => { - if (enabledIds.size > 0 && enabledIds.size < allModels.length) { - const newScopedModels = await resolveModelScope(Array.from(enabledIds), this.session.modelRegistry); + const updateSessionModels = async (enabledIds: string[] | null) => { + currentEnabledIds = enabledIds === null ? null : [...enabledIds]; + if (enabledIds && enabledIds.length > 0 && enabledIds.length < allModels.length) { + const newScopedModels = await resolveModelScope(enabledIds, this.session.modelRegistry); this.session.setScopedModels( newScopedModels.map((sm) => ({ model: sm.model, @@ -3721,49 +3711,18 @@ export class InteractiveMode { { allModels, enabledModelIds: currentEnabledIds, - hasEnabledModelsFilter: currentHasFilter, }, { - onModelToggle: async (modelId, enabled) => { - if (enabled) { - currentEnabledIds.add(modelId); - } else { - currentEnabledIds.delete(modelId); - } - currentHasFilter = true; - await updateSessionModels(currentEnabledIds); - }, - onEnableAll: async (allModelIds) => { - currentEnabledIds.clear(); - for (const id of allModelIds) { - currentEnabledIds.add(id); - } - currentHasFilter = false; - await updateSessionModels(currentEnabledIds); - }, - onClearAll: async () => { - currentEnabledIds.clear(); - currentHasFilter = true; - await updateSessionModels(currentEnabledIds); - }, - onToggleProvider: async (_provider, modelIds, enabled) => { - for (const id of modelIds) { - if (enabled) { - currentEnabledIds.add(id); - } else { - currentEnabledIds.delete(id); - } - } - currentHasFilter = true; - await updateSessionModels(currentEnabledIds); + onChange: async (enabledIds) => { + await updateSessionModels(enabledIds); }, onPersist: (enabledIds) => { // Persist to settings const newPatterns = - enabledIds.length === allModels.length + enabledIds === null || enabledIds.length === allModels.length ? undefined // All enabled = clear filter : enabledIds; - this.settingsManager.setEnabledModels(newPatterns); + this.settingsManager.setEnabledModels(newPatterns ? [...newPatterns] : undefined); this.showStatus("Model selection saved to settings"); }, onCancel: () => { diff --git a/packages/coding-agent/test/suite/regressions/3217-scoped-model-order.test.ts b/packages/coding-agent/test/suite/regressions/3217-scoped-model-order.test.ts new file mode 100644 index 00000000..b1c159e8 --- /dev/null +++ b/packages/coding-agent/test/suite/regressions/3217-scoped-model-order.test.ts @@ -0,0 +1,98 @@ +import type { TUI } from "@mariozechner/pi-tui"; +import stripAnsi from "strip-ansi"; +import { afterEach, beforeAll, describe, expect, it } from "vitest"; +import { ModelSelectorComponent } from "../../../src/modes/interactive/components/model-selector.js"; +import { ScopedModelsSelectorComponent } from "../../../src/modes/interactive/components/scoped-models-selector.js"; +import { initTheme } from "../../../src/modes/interactive/theme/theme.js"; +import { createHarness, type Harness } from "../harness.js"; + +function createFakeTui(): TUI { + return { + requestRender: () => {}, + } as unknown as TUI; +} + +async function waitForAsyncRender(): Promise { + await new Promise((resolve) => setTimeout(resolve, 0)); +} + +describe("issue #3217 scoped model ordering", () => { + const harnesses: Harness[] = []; + + beforeAll(() => { + initTheme("dark"); + }); + + afterEach(() => { + while (harnesses.length > 0) { + harnesses.pop()?.cleanup(); + } + }); + + it("propagates reordered scoped models back to the session state", async () => { + const harness = await createHarness({ + models: [ + { id: "faux-1", name: "One", reasoning: true }, + { id: "faux-2", name: "Two", reasoning: true }, + { id: "faux-3", name: "Three", reasoning: true }, + ], + }); + harnesses.push(harness); + + const orderedIds = harness.models.map((model) => `${model.provider}/${model.id}`); + const changes: Array = []; + const selector = new ScopedModelsSelectorComponent( + { + allModels: [...harness.models], + enabledModelIds: orderedIds, + }, + { + onChange: (enabledModelIds) => { + changes.push(enabledModelIds); + }, + onPersist: () => {}, + onCancel: () => {}, + }, + ); + + selector.handleInput("\x1b[1;3B"); + + expect(changes).toEqual([[orderedIds[1], orderedIds[0], orderedIds[2]]]); + }); + + it("preserves scoped model order in the /model scoped tab", async () => { + const harness = await createHarness({ + models: [ + { id: "faux-1", name: "One", reasoning: true }, + { id: "faux-2", name: "Two", reasoning: true }, + { id: "faux-3", name: "Three", reasoning: true }, + ], + }); + harnesses.push(harness); + + const modelOne = harness.getModel("faux-1")!; + const modelTwo = harness.getModel("faux-2")!; + const modelThree = harness.getModel("faux-3")!; + const selector = new ModelSelectorComponent( + createFakeTui(), + modelOne, + harness.settingsManager, + harness.session.modelRegistry, + [{ model: modelTwo }, { model: modelOne }, { model: modelThree }], + () => {}, + () => {}, + ); + + await waitForAsyncRender(); + + const renderedLines = stripAnsi(selector.render(120).join("\n")) + .split("\n") + .filter((line) => line.includes(`[${modelOne.provider}]`)); + const orderedIds = renderedLines.slice(0, 3).map((line) => { + const [modelId] = line.trim().replace(/^→\s*/, "").split(" ["); + return modelId?.trim() ?? ""; + }); + + expect(orderedIds).toEqual([modelTwo.id, modelOne.id, modelThree.id]); + }); +});