refactor(types): canonicalize shared provider settings identifiers - #1109
refactor(types): canonicalize shared provider settings identifiers#1109WebMad wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 48 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change replaces hard-coded provider identifiers in provider settings and model mappings with ChangesProvider identifier consistency
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
4f9941b to
0bb9e56
Compare
0bb9e56 to
9819602
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
edelauna
left a comment
There was a problem hiding this comment.
Nice! Had some implementation comments to help clean up this section while we're touching it.
| @@ -428,40 +428,40 @@ const defaultSchema = z.object({ | |||
| }) | |||
|
|
|||
| export const providerSettingsSchemaDiscriminated = z.discriminatedUnion("apiProvider", [ | |||
There was a problem hiding this comment.
Since every discriminator here now comes from providerIdentifiers rather than an inline literal, a typo (e.g. picking the wrong registry key) would compile fine and only 32 of the 34 branches are unparsed by any test in the repo — only openaiCodex and kimi-code get a round-trip through providerSettingsSchemaDiscriminated. Worth adding a loop over Object.values(providerIdentifiers) that asserts safeParse succeeds for each?
| @@ -553,37 +553,37 @@ export const isTypicalProvider = (key: unknown): key is TypicalProvider => | |||
| isProviderName(key) && !isInternalProvider(key) && !isCustomProvider(key) && !isFauxProvider(key) | |||
|
|
|||
| export const modelIdKeysByProvider: Record<TypicalProvider, ModelIdKey> = { | |||
There was a problem hiding this comment.
The keys here are type-checked against TypicalProvider, but the values (ModelIdKey) aren't constrained to the right provider — e.g. nothing would catch anthropic pointing at "openRouterModelId". This map now drives form validation in webview-ui/src/utils/validate.ts:218. Is there a test that pins each provider to its expected key?
|
|
||
| export const MODELS_BY_PROVIDER: Record< | ||
| Exclude<ProviderName, "fake-ai" | "gemini-cli" | "openai">, | ||
| Exclude< |
There was a problem hiding this comment.
Could use a one-line comment on why these three are excluded (openai is the custom provider, geminiCli/fakeAi have no model list) — it's not obvious from the type alone and the surrounding blocks explain their groupings.
Summary
providerIdentifiersmodelIdKeysByProvideridvalues inMODELS_BY_PROVIDERtypeof providerIdentifiers.<provider>Implements item 1 from #944 (comment).
Test plan
pnpm exec vitest run src/__tests__/provider-settings.test.ts src/__tests__/provider-identifiers.test.tsfrompackages/typespnpm exec eslint src/provider-settings.ts src/__tests__/provider-identifiers.test.ts --max-warnings=0frompackages/typespnpm run check-typesfrompackages/typesSummary by CodeRabbit
Bug Fixes
Tests