feat(tui): add /provider command, custom registry import, and tabbed model selector#264
feat(tui): add /provider command, custom registry import, and tabbed model selector#264sailist wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: ccb1060 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2d7290cd4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| baseUrl, | ||
| apiKey, | ||
| models, | ||
| selectedModelId: '', // no default yet; user picks in the model selector |
There was a problem hiding this comment.
When adding a known provider, this passes an empty selectedModelId into applyCatalogProvider, but that helper always sets config.defaultModel to ${providerId}/${selectedModelId}. If the user presses Esc in the following selector (which the comment says should leave the provider without a default selection), the config has already been saved with an invalid default such as anthropic/, so later config refreshes/startup cannot resolve the default model. Persist the provider/models without setting defaultModel, or clear it before setConfig when no model was selected.
Useful? React with 👍 / 👎.
| applyCustomRegistryProvider( | ||
| config as Parameters<typeof applyCustomRegistryProvider>[0], | ||
| entry as Parameters<typeof applyCustomRegistryProvider>[1], | ||
| source, | ||
| ); |
There was a problem hiding this comment.
For an already-imported custom registry provider, applyCustomRegistryProvider deletes stale aliases only from this in-memory config, but host.harness.setConfig deep-merges patches, so aliases omitted from config['models'] are not removed from the existing TOML. Re-importing an api.json after a model is removed therefore leaves the old alias selectable, unlike the catalog path above which calls removeProvider before applying. Delete each existing provider id before applying its refreshed registry entry, or use a write path that can remove missing model keys.
Useful? React with 👍 / 👎.
| type: entry.type, | ||
| baseUrl: entry.api, | ||
| apiKey: source.apiKey, | ||
| source, |
There was a problem hiding this comment.
Storing source here does not survive the actual setConfig path: provider patches are parsed by ProviderConfigSchema, which only keeps type, apiKey, baseUrl, defaultModel, oauth, env, and customHeaders. After import/reload, ProviderManagerComponent can no longer see cfg.source.kind === 'apiJson', so multiple providers from the same api.json are shown and deleted as unrelated standalone providers instead of one registry source. Add source to the managed provider schema/TOML handling or store the registry metadata in a supported field.
Useful? React with 👍 / 👎.
| // only `adaptive_thinking = true`. Without this it would render as "unsupported" | ||
| // and switching to it would force thinking off. | ||
| if (caps.includes('thinking') || model.adaptiveThinking === true) return 'toggle'; | ||
| if (caps.includes('thinking')) return 'toggle'; |
There was a problem hiding this comment.
For aliases configured with adaptiveThinking: true but no explicit thinking capability, this now reports thinking as unsupported and effectiveThinking forces the selection to false. That regresses custom Anthropic endpoints that rely on adaptiveThinking to force adaptive thinking (the provider still forwards that alias field), so selecting the model from /model silently disables thinking instead of preserving/toggling it. Keep model.adaptiveThinking === true in the toggleable case.
Useful? React with 👍 / 👎.
| this.opts = opts; | ||
| this.tabs = buildTabs(opts); | ||
|
|
||
| const preferredProvider = opts.initialTabId ?? opts.models[opts.currentValue]?.provider; |
There was a problem hiding this comment.
When /model <alias> names a model from a different provider than the current one, showModelPicker passes that alias as selectedValue, but the tabbed selector still opens the current provider's tab. That tab does not contain the requested alias, so pressing Enter confirms the current model instead of the alias the user typed; prefer opts.models[opts.selectedValue]?.provider before falling back to the current model provider.
Useful? React with 👍 / 👎.
| if (tokenValue.length === 0) { | ||
| this.hint = 'token-empty'; | ||
| this.activeField = 'token'; | ||
| return; |
There was a problem hiding this comment.
This dialog refuses to submit when the Bearer token field is empty, but fetchCustomRegistry explicitly supports an empty apiKey by omitting the Authorization header. As a result, users cannot import public/tokenless api.json registries through /provider even though the underlying importer can fetch them. Let an empty token submit as apiKey: '' instead of treating it as a validation error.
Useful? React with 👍 / 👎.
| name: 'provider', | ||
| aliases: ['providers'], |
There was a problem hiding this comment.
Replacing the /connect registry entry with /provider leaves no compatibility alias for existing users and scripts; after this change /connect ... is no longer recognized as a slash command and falls through as a normal message to the model. The repo rules treat removed commands as breaking changes, so either keep /connect as an alias/forwarder to the provider catalog flow or ship this with an explicit major-version change.
Useful? React with 👍 / 👎.
sailist
force-pushed
the
haozhe/provider-tui
branch
from
b2d7290 to
79938e5
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79938e511e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| aliases: ['providers'], | ||
| description: 'Manage AI providers (add / delete / refresh)', | ||
| priority: 95, | ||
| availability: 'always', |
There was a problem hiding this comment.
When the assistant is streaming or compacting, availability: 'always' lets /provider open and perform add/delete actions even though those callbacks mutate provider config and can call refreshConfigAfterLogin() or clearActiveSessionAfterLogout() without a busy-state guard. In that state, adding a provider may call session.setModel() through the refresh path, and deleting the active provider can close the active session mid-turn; the old provider-mutating /connect command was idle-only by default. Keep /provider idle-only or add an internal streaming/compacting guard before allowing mutations.
Useful? React with 👍 / 👎.
| if (typeof output === 'number' && Number.isInteger(output) && output > 0) { | ||
| return output; |
There was a problem hiding this comment.
When an imported registry model has limit.output but no limit.context, this uses the output-token cap as maxContextSize; for example an 8192 output limit makes the model look like it only has an 8k context window. The parsed output limit is also never written as maxOutputSize on the alias, so runtime provider setup cannot forward it as the model's max-token budget. Leave the context at the default when only output is present and store output in maxOutputSize instead.
Useful? React with 👍 / 👎.
| initialTabId: providerId, | ||
| onSelect: ({ alias, thinking }) => { | ||
| host.restoreEditor(); | ||
| void setDefaultModel(host, alias, thinking); |
There was a problem hiding this comment.
After adding a catalog provider, this callback fires setDefaultModel() without any catch, but that helper writes config and calls refreshConfigAfterLogin(), which can reject if the config write fails or if activating the newly selected provider fails (for example because the entered API key is invalid). Since this selection happens after the slash-command handler has already returned, the dispatcher’s try/catch does not see the rejection, leaving the user with no error message and potentially an unhandled promise rejection; mirror performModelSwitch and surface the failure through host.showError.
Useful? React with 👍 / 👎.
…model selector - add "/provider" slash command for managing AI providers with CRUD UI\n- add custom registry import via api.json URL and Bearer token\n- introduce tabbed model selector grouped by provider\n- add fetchCustomRegistry and applyCustomRegistryProvider in oauth package\n- replace deprecated "/connect" command with unified "/provider" flow\n- update provider and slash-command documentation
sailist
force-pushed
the
haozhe/provider-tui
branch
from
79938e5 to
ccb1060
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ccb10605c0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| aliases: [], | ||
| description: 'Switch LLM model', | ||
| priority: 100, | ||
| availability: 'always', |
There was a problem hiding this comment.
When the app is compacting (isCompacting true while streamingPhase can still be idle), marking /model as always lets the picker open and then performModelSwitch proceeds because its only busy guard checks streamingPhase !== 'idle'; selecting a model can therefore call session.setModel/setThinking and persist defaults during compaction. Keep this command idle-only or add an isCompacting guard before applying the selection.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Replace stale /connect guidance after removing the command
This empty-model notice still tells users to run /connect, but the same change removes /connect from the command registry and dispatch path, so in this exact no-model state the suggested command is no longer executable and will be treated as normal input. Point users at /provider instead so they can actually add a provider from the TUI.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let config = await host.harness.getConfig(); | ||
| for (const entry of Object.values(entries)) { | ||
| if (config.providers[entry.id] !== undefined) { | ||
| config = await host.harness.removeProvider(entry.id); |
There was a problem hiding this comment.
When re-importing a custom registry that contains multiple providers already present in the config, this call reloads the config from disk on each iteration. Any provider applied earlier in the loop has not been persisted yet, so a later removeProvider() overwrites config with a snapshot that does not include that earlier in-memory addition; after the final setConfig, only the providers applied after the last removal survive. Remove all conflicting providers before applying entries, or delete stale providers/models from the in-memory config instead of replacing config mid-loop.
Useful? React with 👍 / 👎.
| description: 'Manage AI providers (add / delete / refresh)', | ||
| priority: 95, | ||
| availability: 'always', | ||
| }, |
There was a problem hiding this comment.
Marking /provider as always lets users open it while a response is streaming or compaction is running, but the add/delete flows mutate the active runtime: picking a default after adding a provider goes through setDefaultModel() → refreshConfigAfterLogin() → activateModelAfterLogin(), which calls session.setModel() without the non-idle guard used by /model, and deleting the active provider can close the session. This can change or tear down the current session while a turn is still in flight, so provider mutations should stay idle-only or guard these actions explicitly.
Useful? React with 👍 / 👎.
| aliases: [], | ||
| description: 'Switch LLM model', | ||
| priority: 100, | ||
| availability: 'always', |
There was a problem hiding this comment.
With this availability change, /model is no longer blocked during streaming, so the user can spend time in the selector, but selecting a model still immediately hits performModelSwitch()'s streamingPhase !== 'idle' check and only shows Cannot switch models while streaming. Either keep the command idle-only or remove the later guard so the advertised always-available workflow can actually complete.
Useful? React with 👍 / 👎.
| models: config.models, | ||
| }); | ||
|
|
||
| await host.authFlow.refreshConfigAfterLogin(); |
There was a problem hiding this comment.
At this point the provider has only been added and the user has not picked the new default yet, but refreshConfigAfterLogin() also calls activateModelAfterLogin() for whatever config.defaultModel currently says. If the current session is using a runtime-selected model that differs from the saved default, simply adding a provider can silently switch the session back before the selector appears (and canceling the selector leaves it there); use a model/provider snapshot refresh here and only activate after setDefaultModel().
Useful? React with 👍 / 👎.
| const spinner = host.showLoginProgressSpinner(`Fetching catalog from ${DEFAULT_CATALOG_URL}`); | ||
| let catalog: Catalog | undefined; | ||
| try { | ||
| catalog = await fetchCatalog(DEFAULT_CATALOG_URL, controller.signal); |
There was a problem hiding this comment.
The new known-provider path always fetches DEFAULT_CATALOG_URL and returns on failure, while the removed /connect path first used the bundled catalog and could fall back to it when the network was unavailable. Since /connect is no longer reachable, users behind a proxy or offline can no longer add catalog providers even though the built-in catalog is still shipped; load BUILT_IN_CATALOG_JSON as the default/fallback here or keep an equivalent command path.
Useful? React with 👍 / 👎.
1 participant
Summary
This PR introduces a comprehensive provider/model management system for the TUI, including a new
/providerslash command, a tabbed model selector, custom registry import support, and supporting UI dialogs. It also adds corresponding documentation updates and unit tests.1. Provider & Model Management in TUI
Problem: Users previously had limited ability to manage LLM providers and select models directly from the TUI. Switching providers or importing custom registries required manual configuration file edits.
What was done:
/providerTUI slash command for managing providers interactively.ProviderManagerDialogto list, add, edit, and remove providers.TabbedModelSelectorto let users browse and select models across providers in a tabbed interface.2. Custom Registry Import
Problem: There was no built-in way to import external provider registries (e.g., custom OpenAI-compatible endpoints) without manually editing configuration files.
What was done:
CustomRegistryImporterUI dialog for importing registries via URL or pasted JSON.packages/oauth/src/custom-registry.tsmodule to parse, validate, and merge custom registry definitions.packages/oauth/test/custom-registry.test.ts.3. Documentation & Test Updates
Problem: New TUI commands and registry features lacked user-facing documentation, and existing tests needed updates for the changed command surface.
What was done:
choice-picker.test.ts,kimi-tui-message-flow.test.ts) to reflect new command additions.Checklist
make gen-changelogto update the changelog.make gen-docsto update the user documentation.