From e18b901a8ddd69104bfbd277c238a822c3cf7683 Mon Sep 17 00:00:00 2001 From: can1357 Date: Tue, 9 Jun 2026 20:45:01 +0200 Subject: [PATCH] refactor(packages/coding-agent): reorganized canonical model selection - Replaced canonical-row resolution with getCanonicalModelSelections in model lists and selector flow. - Hydrated model selector state from registry on construction and kept cached selections during refresh. - Preserved highlighted and cached model selection when offline refresh completed or reordered models. - Added parity checks between getCanonicalModelSelections and resolveCanonicalModel via registry tests. --- packages/coding-agent/CHANGELOG.md | 1 + packages/coding-agent/src/cli/list-models.ts | 16 +-- .../coding-agent/src/config/model-registry.ts | 111 ++++++++++++++--- .../src/modes/components/model-selector.ts | 114 ++++++++++-------- ...ssue-970-custom-provider-discovery.test.ts | 3 +- .../keybindings-escape-components.test.ts | 3 +- .../coding-agent/test/model-registry.test.ts | 27 +++++ ...model-selector-role-badge-thinking.test.ts | 92 ++++++++++++-- 8 files changed, 272 insertions(+), 95 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 0c406399c..f7a3c0119 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -13,6 +13,7 @@ ### Fixed - Fixed the model selector dropping an immediate Enter when cached models were available but the selector's offline refresh was still pending. +- Removed transcript block freezing: blocks no longer replay a frozen render snapshot after crossing out of the live region, which could pin the visible window on stale or partial content (a tool stuck on its pending preview after a late result, an assistant message missing post-finalize updates like error pinning, collapsed/expanded toggles not reflected). Every block now renders its current content on every frame; the live-region seam still keeps still-mutating rows out of native scrollback, and history already committed to the terminal is never rewritten — the window simply always reflects the present state. - Fixed the edit tool's post-edit diff preview occasionally echoing a context line twice with out-of-order numbering. Block-boundary context injection classified space-prefixed diff rows as old-file-only, so an unchanged line sitting in a net-offset region (old N / new N+k) was missing from the new file's visibility window; `findBlockContextLines` then re-surfaced it under its post-edit number and the row was spliced in after the adjacent change run. New-file boundary lines are now translated back to pre-edit numbers (the compact-preview renumbering contract) and merged into a single old-numbered insertion pass — also fixing closers below a net-offset edit being dropped or renumbered incorrectly. ## [15.10.9] - 2026-06-09 diff --git a/packages/coding-agent/src/cli/list-models.ts b/packages/coding-agent/src/cli/list-models.ts index 70673350e..e9d40de34 100644 --- a/packages/coding-agent/src/cli/list-models.ts +++ b/packages/coding-agent/src/cli/list-models.ts @@ -64,22 +64,16 @@ export async function listModels(modelRegistry: ModelRegistry, searchPattern?: s } const filteredCanonical = modelRegistry - .getCanonicalModels({ availableOnly: true, candidates: filteredModels }) - .map(record => { - const selected = modelRegistry.resolveCanonicalModel(record.id, { - availableOnly: true, - candidates: filteredModels, - }); - if (!selected) return undefined; - return { + .getCanonicalModelSelections({ availableOnly: true, candidates: filteredModels }) + .map( + ({ record, model: selected }): CanonicalRow => ({ canonical: record.id, selected: `${selected.provider}/${selected.id}`, variants: String(record.variants.length), context: formatNumber(selected.contextWindow), maxOut: formatNumber(selected.maxTokens), - } satisfies CanonicalRow; - }) - .filter((row): row is CanonicalRow => row !== undefined) + }), + ) .sort((left, right) => left.canonical.localeCompare(right.canonical)); if (filteredModels.length === 0 && filteredCanonical.length === 0) { diff --git a/packages/coding-agent/src/config/model-registry.ts b/packages/coding-agent/src/config/model-registry.ts index 51b62be06..a1cfcb200 100644 --- a/packages/coding-agent/src/config/model-registry.ts +++ b/packages/coding-agent/src/config/model-registry.ts @@ -428,6 +428,12 @@ export interface CanonicalModelQueryOptions { candidates?: readonly Model[]; } +/** A canonical record (with query-filtered variants) plus the variant model selected for it. */ +export interface CanonicalModelSelection { + record: CanonicalModelRecord; + model: Model; +} + /** Result of loading custom models from models.json */ interface CustomModelsResult { models?: CustomModelOverlay[]; @@ -2217,48 +2223,81 @@ export class ModelRegistry { return this.#models; } - #isModelAvailable(model: Model): boolean { + /** + * Availability predicate with per-provider memoization. Auth lookups + * (`authStorage.hasAuth`) and the disabled-provider set are resolved once + * per provider instead of once per model, which matters when filtering the + * full bundled catalog (thousands of models, ~50 providers). + */ + #createAvailabilityCheck(): (model: Model) => boolean { const disabledProviders = getDisabledProviderIdsFromSettings(); - return ( - !disabledProviders.has(model.provider) && - (this.#keylessProviders.has(model.provider) || this.authStorage.hasAuth(model.provider)) - ); + const byProvider = new Map(); + return model => { + let available = byProvider.get(model.provider); + if (available === undefined) { + available = + !disabledProviders.has(model.provider) && + (this.#keylessProviders.has(model.provider) || this.authStorage.hasAuth(model.provider)); + byProvider.set(model.provider, available); + } + return available; + }; + } + + /** + * Build the shared per-query filter state for canonical model queries. + * Hoisted out of the per-record loop: building the candidate-selector set + * and availability memo once per query instead of once per record is what + * keeps `getCanonicalModelSelections` linear instead of O(records × candidates). + */ + #canonicalQueryFilters(options: CanonicalModelQueryOptions | undefined): { + candidateKeys: Set | undefined; + isAvailable: ((model: Model) => boolean) | undefined; + } { + return { + candidateKeys: options?.candidates + ? new Set(options.candidates.map(candidate => formatCanonicalVariantSelector(candidate))) + : undefined, + isAvailable: options?.availableOnly ? this.#createAvailabilityCheck() : undefined, + }; } #filterCanonicalVariants( record: CanonicalModelRecord, - options: CanonicalModelQueryOptions | undefined, + candidateKeys: ReadonlySet | undefined, + isAvailable: ((model: Model) => boolean) | undefined, ): CanonicalModelVariant[] { - const candidateKeys = options?.candidates - ? new Set(options.candidates.map(candidate => formatCanonicalVariantSelector(candidate))) - : undefined; return record.variants.filter(variant => { if (candidateKeys && !candidateKeys.has(variant.selector)) { return false; } - if (options?.availableOnly && !this.#isModelAvailable(variant.model)) { + if (isAvailable && !isAvailable(variant.model)) { return false; } return true; }); } + #buildModelOrder(candidates: readonly Model[]): Map { + const modelOrder = new Map(); + for (let index = 0; index < candidates.length; index += 1) { + modelOrder.set(formatCanonicalVariantSelector(candidates[index]!), index); + } + return modelOrder; + } + #providerRank(): Map { return buildModelProviderPriorityRank(getConfiguredProviderOrderFromSettings()); } #resolveCanonicalVariant( variants: readonly CanonicalModelVariant[], - allCandidates: readonly Model[], + modelOrder: ReadonlyMap, + providerRank: ReadonlyMap, ): CanonicalModelVariant | undefined { if (variants.length === 0) { return undefined; } - const providerRank = this.#providerRank(); - const modelOrder = new Map(); - for (let index = 0; index < allCandidates.length; index += 1) { - modelOrder.set(formatCanonicalVariantSelector(allCandidates[index]!), index); - } const sourceRank: Record = { override: 1, bundled: 1, @@ -2289,9 +2328,10 @@ export class ModelRegistry { } getCanonicalModels(options?: CanonicalModelQueryOptions): CanonicalModelRecord[] { + const { candidateKeys, isAvailable } = this.#canonicalQueryFilters(options); const records: CanonicalModelRecord[] = []; for (const record of this.#canonicalIndex.records) { - const variants = this.#filterCanonicalVariants(record, options); + const variants = this.#filterCanonicalVariants(record, candidateKeys, isAvailable); if (variants.length === 0) { continue; } @@ -2304,12 +2344,43 @@ export class ModelRegistry { return records; } + /** + * One-pass equivalent of `getCanonicalModels` + `resolveCanonicalModel` per + * record. The per-query state (candidate-selector set, availability memo, + * provider rank, candidate order) is built once, so the whole catalog + * resolves in O(records + candidates) instead of O(records × candidates). + * This is the path the model selector hydrates from synchronously on open. + */ + getCanonicalModelSelections(options?: CanonicalModelQueryOptions): CanonicalModelSelection[] { + const { candidateKeys, isAvailable } = this.#canonicalQueryFilters(options); + const candidates = options?.candidates ?? (options?.availableOnly ? this.getAvailable() : this.getAll()); + const modelOrder = this.#buildModelOrder(candidates); + const providerRank = this.#providerRank(); + const selections: CanonicalModelSelection[] = []; + for (const record of this.#canonicalIndex.records) { + const variants = this.#filterCanonicalVariants(record, candidateKeys, isAvailable); + if (variants.length === 0) { + continue; + } + const resolved = this.#resolveCanonicalVariant(variants, modelOrder, providerRank); + if (!resolved) { + continue; + } + selections.push({ + record: { id: record.id, name: record.name, variants }, + model: resolved.model, + }); + } + return selections; + } + getCanonicalVariants(canonicalId: string, options?: CanonicalModelQueryOptions): CanonicalModelVariant[] { const record = this.#canonicalIndex.byId.get(canonicalId.trim().toLowerCase()); if (!record) { return []; } - return this.#filterCanonicalVariants(record, options); + const { candidateKeys, isAvailable } = this.#canonicalQueryFilters(options); + return this.#filterCanonicalVariants(record, candidateKeys, isAvailable); } resolveCanonicalModel(canonicalId: string, options?: CanonicalModelQueryOptions): Model | undefined { @@ -2318,7 +2389,7 @@ export class ModelRegistry { return undefined; } const candidates = options?.candidates ?? (options?.availableOnly ? this.getAvailable() : this.getAll()); - return this.#resolveCanonicalVariant(variants, candidates)?.model; + return this.#resolveCanonicalVariant(variants, this.#buildModelOrder(candidates), this.#providerRank())?.model; } getCanonicalId(model: Model): string | undefined { @@ -2330,7 +2401,7 @@ export class ModelRegistry { * This is a fast check that doesn't refresh OAuth tokens. */ getAvailable(): Model[] { - return this.#models.filter(model => this.#isModelAvailable(model)); + return this.#models.filter(this.#createAvailabilityCheck()); } /** diff --git a/packages/coding-agent/src/modes/components/model-selector.ts b/packages/coding-agent/src/modes/components/model-selector.ts index f985727a2..5efd62877 100644 --- a/packages/coding-agent/src/modes/components/model-selector.ts +++ b/packages/coding-agent/src/modes/components/model-selector.ts @@ -263,21 +263,26 @@ export class ModelSelectorComponent extends Container { // Add bottom border this.addChild(new DynamicBorder()); - // Load models and do initial render - this.#loadModels().then(() => { - this.#buildProviderTabs(); - this.#updateTabBar(); - // Always apply the current search query — the user may have typed - // while models were loading asynchronously. - const currentQuery = this.#searchInput.getValue(); - if (currentQuery) { - this.#filterModels(currentQuery); - } else { - this.#updateList(); - } - // Request re-render after models are loaded - this.#tui.requestRender(); - }); + // Hydrate synchronously from the current registry snapshot so the first + // Enter after opening the selector acts on cached models instead of being + // dropped while the offline refresh promise is still pending. This stays + // on the open path, so it must remain cheap — heavy lifting lives in the + // registry's one-pass getCanonicalModelSelections. + this.#syncFromRegistryState(); + + // Reconcile with cached discovery state in the background. A --models + // scope is registry-independent, so the offline reload would only repeat + // the synchronous hydration above. + if (this.#scopedModels.length === 0) { + this.#modelRegistry + .refresh("offline") + .then(() => this.#syncFromRegistryState()) + .catch(error => { + this.#errorMessage = error instanceof Error ? error.message : String(error); + this.#updateList(); + }) + .finally(() => this.#tui.requestRender()); + } } #buildMenuRoleActions(): void { @@ -477,37 +482,30 @@ export class ModelSelectorComponent extends Container { const candidates = models.map(item => item.model); this.#loadRoleModels(candidates); - const canonicalRecords = this.#modelRegistry.getCanonicalModels({ + const canonicalSelections = this.#modelRegistry.getCanonicalModelSelections({ availableOnly: this.#scopedModels.length === 0, candidates, }); - const canonicalModels = canonicalRecords - .map(record => { - const selectedModel = this.#modelRegistry.resolveCanonicalModel(record.id, { - availableOnly: this.#scopedModels.length === 0, - candidates, - }); - if (!selectedModel) return undefined; - const searchText = [ - record.id, - record.name, - selectedModel.provider, - selectedModel.id, - selectedModel.name, - ...record.variants.flatMap(variant => [variant.selector, variant.model.name]), - ].join(" "); - return { - kind: "canonical" as const, - id: record.id, - model: selectedModel, - selector: record.id, - variantCount: record.variants.length, - searchText, - normalizedSearchText: normalizeSearchText(searchText), - compactSearchText: compactSearchText(searchText), - }; - }) - .filter((item): item is CanonicalModelItem => item !== undefined); + const canonicalModels = canonicalSelections.map(({ record, model: selectedModel }): CanonicalModelItem => { + const searchText = [ + record.id, + record.name, + selectedModel.provider, + selectedModel.id, + selectedModel.name, + ...record.variants.flatMap(variant => [variant.selector, variant.model.name]), + ].join(" "); + return { + kind: "canonical", + id: record.id, + model: selectedModel, + selector: record.id, + variantCount: record.variants.length, + searchText, + normalizedSearchText: normalizeSearchText(searchText), + compactSearchText: compactSearchText(searchText), + }; + }); this.#sortModels(models); this.#sortCanonicalModels(canonicalModels); @@ -523,12 +521,27 @@ export class ModelSelectorComponent extends Container { ); } - async #loadModels(): Promise { - if (this.#scopedModels.length === 0) { - // Reload config and cached discovery state without blocking on live provider refresh - await this.#modelRegistry.refresh("offline"); - } + /** + * Rebuild the visible model lists from the registry's in-memory state. + * Re-entrant: runs once synchronously at construction and again whenever a + * background refresh lands, so it re-applies the live search query and pins + * the highlighted item by selector — a refresh that reorders or inserts + * models must not yank the user's selection out from under a pending Enter. + */ + #syncFromRegistryState(): void { + const selectedKey = this.#getSelectedItem()?.selector; this.#loadModelsFromCurrentRegistryState(); + this.#buildProviderTabs(); + this.#updateTabBar(); + this.#applyTabFilter(); + if (selectedKey) { + const visibleItems = this.#getVisibleItems(); + const restoredIndex = visibleItems.findIndex(item => item.selector === selectedKey); + if (restoredIndex >= 0 && restoredIndex !== this.#selectedIndex) { + this.#selectedIndex = this.#coerceSelectedIndex(restoredIndex, visibleItems); + this.#updateList(); + } + } } #buildProviderTabs(): void { @@ -631,10 +644,7 @@ export class ModelSelectorComponent extends Container { // here must stay purely in-memory — do not call modelRegistry.refresh() // again or tab switches will pay an extra whole-registry reload after the // network round-trip completes. - this.#loadModelsFromCurrentRegistryState(); - this.#buildProviderTabs(); - this.#updateTabBar(); - this.#applyTabFilter(); + this.#syncFromRegistryState(); } catch (error) { this.#errorMessage = error instanceof Error ? error.message : String(error); this.#updateList(); diff --git a/packages/coding-agent/test/issue-970-custom-provider-discovery.test.ts b/packages/coding-agent/test/issue-970-custom-provider-discovery.test.ts index 4569869ae..05a54f1cd 100644 --- a/packages/coding-agent/test/issue-970-custom-provider-discovery.test.ts +++ b/packages/coding-agent/test/issue-970-custom-provider-discovery.test.ts @@ -33,8 +33,7 @@ async function createSelector(state: ProviderDiscoveryState): Promise [], getAll: () => [], getDiscoverableProviders: () => [state.provider], - getCanonicalModels: () => [], - resolveCanonicalModel: () => undefined, + getCanonicalModelSelections: () => [], getProviderDiscoveryState: () => state, } as unknown as ModelRegistry; const ui = { requestRender: vi.fn() } as unknown as TUI; diff --git a/packages/coding-agent/test/keybindings-escape-components.test.ts b/packages/coding-agent/test/keybindings-escape-components.test.ts index 679d30bb2..5f24a0027 100644 --- a/packages/coding-agent/test/keybindings-escape-components.test.ts +++ b/packages/coding-agent/test/keybindings-escape-components.test.ts @@ -78,8 +78,7 @@ describe("component escape bindings", () => { const modelRegistry = { getAll: () => [model], getDiscoverableProviders: () => [], - getCanonicalModels: () => [], - resolveCanonicalModel: () => undefined, + getCanonicalModelSelections: () => [], } as unknown as ModelRegistry; const ui = { requestRender: vi.fn(), diff --git a/packages/coding-agent/test/model-registry.test.ts b/packages/coding-agent/test/model-registry.test.ts index 1073cbd9e..9dbd31276 100644 --- a/packages/coding-agent/test/model-registry.test.ts +++ b/packages/coding-agent/test/model-registry.test.ts @@ -483,6 +483,33 @@ describe("ModelRegistry", () => { expect(resolved?.provider).toBe("demo"); expect(resolved?.id).toBe("anthropic/claude-sonnet-4.5"); }); + + test("getCanonicalModelSelections matches per-record resolveCanonicalModel over the bundled catalog", () => { + authStorage.setRuntimeApiKey("anthropic", "test-key"); + authStorage.setRuntimeApiKey("openrouter", "test-key"); + authStorage.setRuntimeApiKey("groq", "test-key"); + writeModelsJson({}); + + const registry = new ModelRegistry(authStorage, modelsJsonPath); + const candidates = registry.getAvailable(); + expect(candidates.length).toBeGreaterThan(0); + + const options = { availableOnly: true, candidates } as const; + const selections = registry.getCanonicalModelSelections(options); + const records = registry.getCanonicalModels(options); + expect(selections.length).toBe(records.length); + expect(selections.length).toBeGreaterThan(0); + + const mismatches = selections + .map(({ record, model }) => { + const resolved = registry.resolveCanonicalModel(record.id, options); + return resolved && resolved.provider === model.provider && resolved.id === model.id + ? undefined + : `${record.id}: batch=${model.provider}/${model.id} loop=${resolved?.provider}/${resolved?.id}`; + }) + .filter((entry): entry is string => entry !== undefined); + expect(mismatches).toEqual([]); + }); }); describe("OpenRouter routed suffix fallback", () => { diff --git a/packages/coding-agent/test/model-selector-role-badge-thinking.test.ts b/packages/coding-agent/test/model-selector-role-badge-thinking.test.ts index ef00b6df7..aa94c5858 100644 --- a/packages/coding-agent/test/model-selector-role-badge-thinking.test.ts +++ b/packages/coding-agent/test/model-selector-role-badge-thinking.test.ts @@ -15,8 +15,7 @@ function createSelector(model: Model, settings: Settings): ModelSelectorComponen const modelRegistry = { getAll: () => [model], getDiscoverableProviders: () => [], - getCanonicalModels: () => [], - resolveCanonicalModel: () => undefined, + getCanonicalModelSelections: () => [], } as unknown as ModelRegistry; const ui = { requestRender: vi.fn(), @@ -71,8 +70,7 @@ function createScopedSelector( const modelRegistry = { getAll: () => models, getDiscoverableProviders: () => [], - getCanonicalModels: () => [], - resolveCanonicalModel: () => undefined, + getCanonicalModelSelections: () => [], } as unknown as ModelRegistry; const ui = { requestRender: vi.fn(), @@ -196,6 +194,86 @@ describe("ModelSelector role badge thinking display", () => { expect(onSelect).not.toHaveBeenCalled(); }); + test("uses cached models for Enter while offline refresh is still pending", () => { + installTestTheme(); + const settings = Settings.isolated({}); + const cachedModel = createContextTestModel("cached-fast", 128_000); + const refreshGate = Promise.withResolvers(); + const onSelect = vi.fn(); + const modelRegistry = { + getAll: () => [cachedModel], + refresh: vi.fn(() => refreshGate.promise), + refreshProvider: vi.fn(async () => {}), + getError: () => undefined, + getAvailable: () => [cachedModel], + getDiscoverableProviders: () => [], + getCanonicalModelSelections: () => [], + } as unknown as ModelRegistry; + const ui = { + requestRender: vi.fn(), + } as unknown as TUI; + + const selector = new ModelSelectorComponent( + ui, + undefined, + settings, + modelRegistry, + [], + model => onSelect(model.id), + () => {}, + { temporaryOnly: true }, + ); + + selector.handleInput("\n"); + expect(onSelect).toHaveBeenCalledWith("cached-fast"); + expect(modelRegistry.refresh).toHaveBeenCalledTimes(1); + refreshGate.resolve(); + }); + + test("keeps the highlighted model when a background refresh reorders the list", async () => { + installTestTheme(); + const settings = Settings.isolated({}); + const modelBb = createContextTestModel("bb-model", 128_000); + const modelCc = createContextTestModel("cc-model", 128_000); + const modelAa = createContextTestModel("aa-model", 128_000); + let availableModels: Model[] = [modelBb, modelCc]; + const refreshGate = Promise.withResolvers(); + const onSelect = vi.fn(); + const modelRegistry = { + getAll: () => availableModels, + refresh: vi.fn(() => refreshGate.promise), + refreshProvider: vi.fn(async () => {}), + getError: () => undefined, + getAvailable: () => availableModels, + getDiscoverableProviders: () => [], + getCanonicalModelSelections: () => [], + } as unknown as ModelRegistry; + const ui = { + requestRender: vi.fn(), + } as unknown as TUI; + + const selector = new ModelSelectorComponent( + ui, + undefined, + settings, + modelRegistry, + [], + model => onSelect(model.id), + () => {}, + { temporaryOnly: true }, + ); + + // Highlight the second entry, then let the pending refresh land a model + // that sorts ahead of it and shifts every index. + selector.handleInput("\x1b[B"); + availableModels = [modelAa, modelBb, modelCc]; + refreshGate.resolve(); + await Bun.sleep(0); + + selector.handleInput("\n"); + expect(onSelect).toHaveBeenCalledWith("cc-model"); + }); + test("refreshes Ollama Cloud using provider id instead of tab label", async () => { installTestTheme(); const settings = Settings.isolated({}); @@ -213,8 +291,7 @@ describe("ModelSelector role badge thinking display", () => { getError: () => undefined, getAvailable: () => availableModels, getDiscoverableProviders: () => ["ollama-cloud"], - getCanonicalModels: () => [], - resolveCanonicalModel: () => undefined, + getCanonicalModelSelections: () => [], getProviderDiscoveryState: () => ({ provider: "ollama-cloud", status: "idle", @@ -276,8 +353,7 @@ describe("ModelSelector role badge thinking display", () => { getError: () => undefined, getAvailable: () => availableModels, getDiscoverableProviders: () => ["ollama-cloud"], - getCanonicalModels: () => [], - resolveCanonicalModel: () => undefined, + getCanonicalModelSelections: () => [], getProviderDiscoveryState: () => ({ provider: "ollama-cloud", status: "idle",