refactor(types): use canonical identifiers for default models#991
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesProvider default resolution
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/types/src/__tests__/provider-default-model.test.ts (1)
33-35: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the international Zai branch.
The test only exercises
{ isChina: true }; add an assertion for{ isChina: false }or omitted options againstinternationalZAiDefaultModelIdso regressions in the other branch are detected.Proposed test addition
import { anthropicDefaultModelId, getProviderDefaultModelId, + internationalZAiDefaultModelId, mainlandZAiDefaultModelId, openRouterDefaultModelId, vscodeLlmDefaultModelId, } from "../providers/index.js" it("preserves region-dependent defaults", () => { expect(getProviderDefaultModelId(providerIdentifiers.zai, { isChina: true })).toBe(mainlandZAiDefaultModelId) + expect(getProviderDefaultModelId(providerIdentifiers.zai)).toBe(internationalZAiDefaultModelId) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/types/src/__tests__/provider-default-model.test.ts` around lines 33 - 35, Extend the “preserves region-dependent defaults” test for providerIdentifiers.zai with an assertion using { isChina: false } or omitted options, and verify it returns internationalZAiDefaultModelId while retaining the existing mainland assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/types/src/__tests__/provider-default-model.test.ts`:
- Around line 33-35: Extend the “preserves region-dependent defaults” test for
providerIdentifiers.zai with an assertion using { isChina: false } or omitted
options, and verify it returns internationalZAiDefaultModelId while retaining
the existing mainland assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4bd59281-3da6-4484-abbb-6352934a72e6
📒 Files selected for processing (2)
packages/types/src/__tests__/provider-default-model.test.tspackages/types/src/providers/index.ts
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
b999600 to
64a0508
Compare
edelauna
left a comment
There was a problem hiding this comment.
Thanks - noticed a small quality of life change we can make while updating this file.
Summary
getProviderDefaultModelId()with the canonicalproviderIdentifiersregistryCloses #954
TDD
The new test first overrides the canonical OpenRouter identifier. Before the implementation change, the literal-based switch fails that case and returns the Anthropic fallback. Migrating the switch to
providerIdentifiersmakes it pass. Additional cases triangulate the behavior across static/internal, region-dependent, custom/local, and fallback providers.Verification
cd packages/types && npx vitest run src/__tests__/provider-default-model.test.ts src/__tests__/provider-identifiers.test.ts— 17 passedcd packages/types && npm test— 253 passedcd packages/types && npm run check-types— passedcd packages/types && npm run lint— passedNote
The repository-wide pre-push type check currently reports unrelated pre-existing errors involving MiniMax M3, Moonshot router typing, LiteLLM exports, and
abandonSubtaskWithId. The affected@roo-code/typespackage type check passes.Summary by CodeRabbit