fix(settings): enforce "usable provider ⇒ concrete model" invariant (#580)#581
Merged
Conversation
…580) Layer 1 — resolve provider AND model atomically at every source: - add resolveSelectedModel(): like validateModel but no empty-input short-circuit, so it returns '' only when the provider has no models - fetchServerProviders: symmetric LLM/image/video provider recovery + resolveSelectedModel; remove the asymmetric #577 isServerConfigured-gated LLM tail and the separate first-load auto-select block (now subsumed, and covers client-API-key providers too) - setProviderConfig: entering an API key for the active/unset LLM provider resolves a concrete model in the same set() - setVideoProvider: resolve a model on switch (was id-only); setImageProvider routed through the shared resolver Layer 2 — remove states unreachable under the invariant: - add shared isLLMProviderConfigured / hasUsableLLMProvider; gate the landing-page generate button on a usable provider (state A vs B) and drop the modelNotConfigured toast + forced SettingsDialog - generation-toolbar: use the shared predicate; drop the dead "Select Model" label/tooltip fallback - delete dead components/settings/model-selector.tsx (zero refs) and the now-orphan settings.selectModel i18n key (6 locales) Note: generation-toolbar's `providerEntries.length === 0` branch is KEPT — it is reachable via search (search-no-results), not the dead no-provider state the issue assumed. modelNotConfigured i18n key kept (still used by the chat path in use-chat-sessions). Tests: resolveSelectedModel + hasUsableLLMProvider unit tests; server-sync invariant tests (client-key resolution, atomic API-key entry, provider switch, serverModels-preference + first-load regression guards). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…olution Code review Important #1: fold atomic model resolution into setImageProviderConfig / setVideoProviderConfig (design doc Layer 1 step 2 scoped these). Deleting the selected custom image/video model on the active provider now resolves to a valid model in the same set() instead of leaving it stale until the next server sync — restores LLM/image/video symmetry. Minor #2/#3: document the deliberate store-level (credential path) vs UX-level (incl. ≥1 model) "usable" split on isProviderUsable; clarify that setProviderConfig also adopts providerId on the first-load API-key path. Tests: +2 invariant tests (delete selected custom image/video model → falls back to a valid model). 89/89 store tests pass, tsc clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…able (#580) Code-review/e2e follow-up. ollama/lemonade ship requiresApiKey:false + a registry defaultBaseUrl, so the old isLLMProviderConfigured counted them as configured on a fresh install — but buildFallback/reconcile never auto-selects keyless providers, so hasUsableProvider could be true while modelId stayed '', re-enabling generation with no model (the exact #498/#580 class of bug, just via the keyless path). Narrow isLLMProviderConfigured to be consistent with isProviderUsable: keyless ⇒ usable only once the user sets an explicit baseUrl (registry defaultBaseUrl alone is not user intent); server-config and key-requiring paths unchanged. Now gate + toolbar + reconcile agree, and genuine State A (no usable provider) is reachable again. Add e2e/tests/model-invariant-580.spec.ts: State A (no provider → generate disabled + single "Set up model" affordance, no toast/forced dialog) and State B (server-configured → concrete model auto-resolved, generation enabled). +3 keyless unit tests. 92/92 store tests + 2 e2e pass, tsc clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
) Self-test found: deleting the selected provider in Settings left the model selector pointing at the deleted/invalid provider (or, via the component's hand-rolled fallback, at an unusable built-in like openai with no key). Root cause: setProvidersConfig (the bulk setter behind delete-provider / delete-model) did ZERO re-resolution — Layer 1 covered setProviderConfig (singular) + image/video setters + the reconcile, but not this path. The component's confirmDeleteProvider then picked Object.keys()[0] (always a built-in, ignoring usability), the exact per-call-site hand-rolled shape #580 condemns. Fix at the source: setProvidersConfig now re-resolves (providerId, modelId) via the canonical isLLMProviderConfigured + resolveSelectedModel — keep the current provider if still usable, else first usable, else State A. Removed the brittle confirmDeleteProvider override (kept only its local Settings-tab selection). Now every bulk config mutation (delete, import, reset) holds the invariant. Tests: +2 store tests (delete selected/only-usable provider → State A, not the deleted/invalid one; delete non-selected → keep usable selection). 94/94 store tests, full suite green (only pre-existing ssrf DNS), tsc/eslint clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…#580) Clarified repro: the realistic case isn't deleting a custom provider — it's clearing the SELECTED provider's API key so it becomes invalid. setProvider Config only re-resolved when the edit made a provider *usable*; the "became unusable" transition fell through (return base) → the now-invalid provider stayed selected. The previous commit fixed only the bulk path. Root-cause fix: extract one shared `resolveLLMSelection(config, providerId, modelId)` — keep current provider if still usable, else first usable, else State A; then resolve the model. Use it in BOTH setProviderConfig (single edit: key add/clear, model-list edit) and setProvidersConfig (bulk: delete, import, reset). The two paths can no longer diverge — exactly the per-call- site asymmetry #580 set out to eliminate. Drops the now-unused isProviderUsable import. Tests: +2 store tests (clear selected provider key → drops stale selection; clear non-selected key → keeps usable selection). 96/96 store tests, full suite green except pre-existing ssrf DNS; tsc + eslint clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
CI 'Lint, Typecheck & Unit Tests' ran prettier . --check and failed on the two added test specs (only tsc/eslint/vitest were run locally). Pure whitespace reformat, no logic change — 96/96 store tests still green. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
cosarah
approved these changes
May 18, 2026
Collaborator
cosarah
left a comment
cosarah
left a comment
There was a problem hiding this comment.
LGTM. Code: shared resolveLLMSelection symmetrically applied to single + bulk config edits + image/video setters; isLLMProviderConfigured consolidates the gate; resolveSelectedModel cleanly separates selection-resolution from validation. Comments explain the invariant well.
Locally tested on pr-581 worktree (port 3001):
- State A (no usable provider): generate button disabled, single "Set up model" CTA, no toast, no forced dialog.
- State B: toggling
requiresApiKeyon the active provider with no apiKey immediately switchesproviderIdto the next usable one (or to State A) — capsule updates without page refresh. Earlier mis-report on my end was caused by testing against an older dev server. - Clearing/adding apiKey, deleting the selected custom model, and bulk provider delete all re-resolve atomically as described.
- Keyless providers correctly require an explicit user
baseUrlbefore counting as configured.
Unit + store tests 364/365 (the one failure is the pre-existing ssrf-guard DNS test, unrelated). tsc clean.
This was referenced May 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #580.
Establishes the invariant
(usable provider) ⟺ (a concrete model of that provider is always selected)for LLM / image / video, resolving provider and model atomically at the source. The previously-representable(usable provider, empty modelId)state is eliminated.Layer 1 — code
resolveSelectedModel()— likevalidateModelbut no empty-input short-circuit, so it returns''only when the provider has no models.fetchServerProvidersreconcile: symmetric LLM/image/video provider-recovery +resolveSelectedModel; removed the asymmetric [codex] fix server LLM model auto-selection #577isServerConfigured-gated LLM tail and the separate first-load block (now also covers the client-API-key path [Bug]: 配置不成功 #498/[codex] fix server LLM model auto-selection #577 missed).resolveLLMSelection()used by bothsetProviderConfig(single edit: key add/clear, model-list edit) andsetProvidersConfig(bulk: delete provider, import, reset) — keep current provider if still usable, else first usable, else State A. Image/video config setters get the symmetric treatment.Layer 2 — UX (states now unreachable)
isLLMProviderConfigured/hasUsableLLMProvider; landing page gates generation on a usable provider and drops themodelNotConfiguredtoast + forced settings dialog. Toolbar uses the shared predicate; dead "Select Model" label/tooltip removed.components/settings/model-selector.tsx; pruned the orphansettings.selectModeli18n key (6 locales).Deviations from the issue's literal table (please confirm)
generation-toolbarproviderEntries.length === 0branch — it is reachable via search (search-no-results), not the dead no-provider state the issue assumed.settings.modelNotConfiguredi18n key — still used by the chat path (use-chat-sessions.ts), a separate reachable surface.baseUrlto count as "configured" (registrydefaultBaseUrlalone isn't user intent) — without this,hasUsableProviderwas true while the reconcile never selected them, re-enabling generation with no model.Testing
resolveSelectedModel/hasUsableLLMProviderunit tests; server-sync invariant tests (client-key resolution, atomic API-key entry/clear, provider switch, custom-model delete, bulk delete, serverModels-preference + first-load regression guards). 96/96 store tests, full suite green (the only failures are pre-existing environmentalssrf-guardDNS tests, untouched here).tsc+eslintclean. Added an e2e spec (State A / State B).Based on
main@2ed39da; will rebase before merge ifmainadvances.🤖 Generated with Claude Code