Repository navigation
[refactor] Move default provider ID functionality into provider definition - #2233
Conversation
Merge packages/types/src/provider-settings/*.ts into packages/types/src/providers/, appending each source file's code to the end of the same-named destination file and combining their imports. Files without a same-named counterpart (common, fake-ai, gemini-cli, litellm, openai-native, vscode-lm) move over unchanged. The providerDefinitionList registry from provider-settings/index.ts is appended to providers/index.ts, and all import sites now point at the new location.
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Default-model accessor contract packages/types/src/providers/common.ts |
Provider definitions now require a getDefaultModelId accessor. The shared module adds NO_DEFAULT_MODEL_ID for providers without a meaningful default. |
Provider module definitions packages/types/src/provider-settings/*, packages/types/src/providers/* |
Provider definitions and settings schemas move into provider modules, with model-ID accessors and default-model callbacks. The OpenAI extra-body parser and NanoGPT routing preferences are defined in their provider modules. |
Provider definition registry packages/types/src/provider-settings/index.ts, packages/types/src/providers/index.ts |
The providers barrel exports an ordered providerDefinitionList and the ProviderDefinition type. It adds exports for Fake AI, Gemini CLI, LiteLLM, OpenAI Native, and VS Code LM. |
Settings-aware default resolution packages/types/src/provider-settings.ts, packages/types/src/__tests__/*, webview-ui/src/components/settings/utils/providerModelConfig.ts, webview-ui/src/components/ui/hooks/useSelectedModel.ts |
getProviderDefaultModelId reads the provider definition’s default using the supplied settings and falls back to the Anthropic default. Webview callers pass the current configuration. Tests cover provider accessors and Z.ai defaults for configured and unset API lines. |
Priority:
Estimated code review effort: 4 (Complex) | ~45 minutes
Change: Refactor
Merge Risk: 
3b0d2
-
The Z.ai default selection behaves correctly. Its remaining test-coverage limitation is already documented and does not establish a runtime failure; no concrete blocker remains.
Pre-merge checks: 
-
Check name Status Explanation Resolution Description check The description explains the refactor and lists testing, but it does not provide the required approved GitHub Issue link. The test procedure is also too vague to reproduce. Add an approved issue reference in the Related GitHub Issue section, such as "Closes: #123". Provide specific unit and integration test commands or steps, including relevant environment details.Regression Evidence The refactor changes the default-model behavior for every provider through providerDefinitionListandgetProviderDefaultModelId(packages/types/src/providers/index.ts:82-120, `packages/types/src…Add a table-driven unit test at the getProviderDefaultModelIdlayer that covers every provider definition, including each static default constant, the empty defaults for OpenAI/Ollama/LM Studio, the Anthropic fallback providers, and all Z…-
Check name Status Explanation Title check The title clearly and concisely describes moving default provider ID functionality into provider definitions. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Security Boundaries No changed path meets the security failure conditions. createProviderDefinitionstill validates model settings with ZodsafeParsebefore model-ID access.getProviderDefaultModelIdonly dispatche…Persistence Integrity No changed persistence operation exists. The PR changes provider-definition schemas and default-model resolution only; ProviderSettingsManagerand its awaitedload/storepaths are unchanged. The…Lifecycle Resource Cleanup PASS. The reviewed changes move provider definitions and default-model resolution between TypeScript modules. The changed UI paths only pass provider settings to synchronous `getProviderDefaultModelId… Full details: Security Boundaries
-
Explanation
No changed path meets the security failure conditions.
createProviderDefinitionstill validates model settings with ZodsafeParsebefore model-ID access.getProviderDefaultModelIdonly dispatches to static provider callbacks; the only settings-dependent callback, Z.ai, validateszaiApiLinewithsafeParseand uses an allowlisted enum with an international fallback. The moved OpenAI extra-body parser retains rejection of reserved and prototype-pollution keys. No changed code leaks secrets or PII, executes unvalidated input, or bypasses an approval or allowlist control.
Full details: Persistence Integrity
-
Explanation
No changed persistence operation exists. The PR changes provider-definition schemas and default-model resolution only;
ProviderSettingsManagerand its awaitedload/storepaths are unchanged. The changed consumers pass persistedapiConfigurationintogetProviderDefaultModelId, including Z.AI API-line settings, so no persisted default propagation failure is present.
Full details: Lifecycle Resource Cleanup
-
Explanation
PASS. The reviewed changes move provider definitions and default-model resolution between TypeScript modules. The changed UI paths only pass provider settings to synchronous
getProviderDefaultModelIdcalls. The diff adds no listener, watcher, subscription, timer, task, worker, cancellation, disposal, or restart path. No changed lifecycle path can leak a resource or duplicate work.
-
Full details: Regression Evidence
-
Explanation
The refactor changes the default-model behavior for every provider through
providerDefinitionListandgetProviderDefaultModelId(packages/types/src/providers/index.ts:82-120,packages/types/src/provider-settings.ts:363-369). Focused assertions cover only a subset, plus Z.ai branches and a few added providers. The new all-provider test (packages/types/src/__tests__/provider-settings.test.ts:21-29) checks only that the accessor exists and returns any string. It would pass if a provider returned the wrong model ID. For example, the new DeepSeek callback (packages/types/src/providers/deepseek.ts:88-92) has no assertion that the resolver returnsdeepSeekDefaultModelId. The same gap applies to many moved defaults.Resolution
Add a table-driven unit test at the
getProviderDefaultModelIdlayer that covers every provider definition, including each static default constant, the empty defaults for OpenAI/Ollama/LM Studio, the Anthropic fallback providers, and all Z.ai API-line and unset/invalid cases. Keep the existing structural registration test, but do not use its string-type assertion as semantic coverage.
-
Finishing Touches: 💡 1
-
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
Review statusThanks for contributing. This comment tracks the review sequence and the next action. Current step: Awaiting fresh human maintainer or CODEOWNER approval. Automated review is complete for the latest commit but does not replace human approval. Review-state labels are managed by this workflow; do not edit them manually. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@coderabbitai full review |
✅ Action performed
|
github-actions
Bot
added
coderabbit-review-active
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
github-actions
Bot
added
awaiting-maintainer
edelauna
left a comment
edelauna
left a comment
There was a problem hiding this comment.
Nice! One comment to boost the test coverage for z.ai - otherwise this looks good.
github-actions
Bot
added
awaiting-author
…itions Each provider definition now declares its own `getDefaultModelId`, mirroring `getModelId`, so per-provider default-model data lives next to the rest of that provider's configuration instead of in a central switch. - Add `getDefaultModelId` to `ProviderDefinition`/`createProviderDefinition` and a shared `NO_DEFAULT_MODEL_ID` constant. - Move every provider's default into its definition; Z.ai keeps its region-dependent logic by reading `zaiApiLine` from settings. - Remove the `getProviderDefaultModelId` switch from `providers/index.ts`. - Re-add `getProviderDefaultModelId(provider, settings)` in `provider-settings.ts`, resolving the definition and falling back to the Anthropic default. - Update webview callers to pass `apiConfiguration` and drop the duplicated Z.ai region logic in `providerModelConfig.ts`. - Extend tests to cover the new accessor and the Z.ai API-line defaults.
e7181c0 to
3b0d281
Compare
github-actions
Bot
removed
the
awaiting-author
github-actions
Bot
added
coderabbit-review-active
github-actions
Bot
added
awaiting-maintainer
…ove) Re-anchors the prompt cache TTL knob onto Zoo-Code-Org#2233's new provider-definition layout: the knob stays in baseProviderSettingsShape, which now lives in packages/types/src/providers/common.ts. Updates the TTL spec imports to the new module paths.
Re-merges the Bedrock cache-TTL gate onto the updated parent Zoo-Code-Org#2264, which now carries Zoo-Code-Org#2233's provider-settings -> providers move. No conflicts: this branch never edited packages/types/src/providers/common.ts, so the parent's re-anchored TTL knob lands unmodified and the gate's six files apply clean. Verified: origin/main is an ancestor; 29 co-touched specs pass by name; no stale provider-settings/ imports remain.
Brings the leaf's settings/UI prompt-cache-TTL surface onto the parent's re-anchored provider layout (Zoo-Code-Org#2233 moved provider-settings/ to providers/ and added the TTL knob to baseProviderSettingsShape). No textual conflict: the two sides touch disjoint paths. The leaf's delta is webview/settings/UI only (Anthropic.tsx control, i18n, SettingsView and ClineProvider specs); the parent's delta is packages/types + provider runtime. packages/types/src/providers/common.ts resolves byte-identical to the parent, which already carries both Zoo-Code-Org#2233's structure and the TTL knob. Verified: 32 co-touched specs run by name (1178 tests), check-types green for packages/types, src and webview-ui, eslint --max-warnings=0 on all 97 touched files, no stale provider-settings/ imports, eslint-suppressions.json unchanged.
Round-3 re-merge onto main@33b56507188f. Conflict set (2 paths): - packages/types/src/provider-settings/mimo.ts (modify/delete): main's Zoo-Code-Org#2233 moved every provider definition into packages/types/src/providers/. The deletion wins; this branch's added comment on the mimoBaseUrl union moved with the definition into providers/mimo.ts. - webview-ui/src/components/ui/hooks/useSelectedModel.ts (content): kept this branch's static-catalog fallback branch and applied main's new two-argument getProviderDefaultModelId signature to the final fallback. Also retargeted two comments that named the deleted path (src/api/providers/fetchers/mimo.ts and its spec).
…ition (#2233) * refactor(types): move provider-settings into providers Merge packages/types/src/provider-settings/*.ts into packages/types/src/providers/, appending each source file's code to the end of the same-named destination file and combining their imports. Files without a same-named counterpart (common, fake-ai, gemini-cli, litellm, openai-native, vscode-lm) move over unchanged. The providerDefinitionList registry from provider-settings/index.ts is appended to providers/index.ts, and all import sites now point at the new location. * refactor(types): move provider default model data into provider definitions Each provider definition now declares its own `getDefaultModelId`, mirroring `getModelId`, so per-provider default-model data lives next to the rest of that provider's configuration instead of in a central switch. - Add `getDefaultModelId` to `ProviderDefinition`/`createProviderDefinition` and a shared `NO_DEFAULT_MODEL_ID` constant. - Move every provider's default into its definition; Z.ai keeps its region-dependent logic by reading `zaiApiLine` from settings. - Remove the `getProviderDefaultModelId` switch from `providers/index.ts`. - Re-add `getProviderDefaultModelId(provider, settings)` in `provider-settings.ts`, resolving the definition and falling back to the Anthropic default. - Update webview callers to pass `apiConfiguration` and drop the duplicated Z.ai region logic in `providerModelConfig.ts`. - Extend tests to cover the new accessor and the Z.ai API-line defaults.
Related GitHub Issue
Simple refactor.
Description
Move default provider ID functionality into provider definition. This replaces yet another switch statement with declarative style. Provider definitions were moved into files containing provider model data to reduce the number of unnecessary imports.
Test Procedure
unit, integration tests.
Pre-Submission Checklist
*.visual.tsxsnapshot inwebview-ui/. Seewebview-ui/AGENTS.md→ "When a UI change needs a snapshot".Visual Snapshots
n/a
Videos (interaction / animation only)
n/a
Documentation Updates
n/a