Add TypeScript SDK client for listing provider models#1855
Open
Uday-sidagana wants to merge 1 commit into
Open
Add TypeScript SDK client for listing provider models#1855Uday-sidagana wants to merge 1 commit into
Uday-sidagana wants to merge 1 commit into
Conversation
Contributor
WalkthroughThis PR adds a new Models client to the TypeScript SDK. ModelsClient provides a ChangesModels Client Feature
Sequence DiagramsequenceDiagram
participant Consumer
participant OpenRAGClient
participant ModelsClient
participant APIEndpoint
Consumer->>OpenRAGClient: instantiate
OpenRAGClient->>ModelsClient: new ModelsClient(this)
Consumer->>ModelsClient: list("openai")
ModelsClient->>APIEndpoint: GET /api/v1/models/openai
APIEndpoint-->>ModelsClient: {language_models, embedding_models}
ModelsClient->>ModelsClient: normalize to arrays
ModelsClient-->>Consumer: ModelsResponse
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
sdks/typescript/tests/integration.test.ts (1)
130-135: ⚡ Quick winStrengthen the models contract assertion to validate element shape.
Current checks only assert array-ness. Add item-level checks (when arrays are non-empty) for
value,label, and optional booleandefaultto catch response normalization regressions.Suggested enhancement
describe("Models", () => { it("should list models for a provider", async () => { const models = await client.models.list("openai"); expect(Array.isArray(models.language_models)).toBe(true); expect(Array.isArray(models.embedding_models)).toBe(true); + + const firstLanguage = models.language_models[0]; + if (firstLanguage) { + expect(typeof firstLanguage.value).toBe("string"); + expect(typeof firstLanguage.label).toBe("string"); + if ("default" in firstLanguage) { + expect(typeof firstLanguage.default).toBe("boolean"); + } + } + + const firstEmbedding = models.embedding_models[0]; + if (firstEmbedding) { + expect(typeof firstEmbedding.value).toBe("string"); + expect(typeof firstEmbedding.label).toBe("string"); + if ("default" in firstEmbedding) { + expect(typeof firstEmbedding.default).toBe("boolean"); + } + } }); });🤖 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 `@sdks/typescript/tests/integration.test.ts` around lines 130 - 135, Update the test case in the "it('should list models for a provider', ...)" block that calls client.models.list("openai") to not only assert Array.isArray for models.language_models and models.embedding_models but, when each array is non-empty, iterate their items and assert each item has a string "value", a string "label", and if "default" is present it is a boolean; use the existing test identifiers (client.models.list, models.language_models, models.embedding_models) to locate the test and add these element-shape assertions to catch response normalization regressions.
🤖 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.
Inline comments:
In `@sdks/typescript/src/models.ts`:
- Around line 16-18: The list method is currently interpolating the raw provider
into the request path, which can produce invalid URLs; in async list(provider:
string): Promise<ModelsResponse> validate/normalize provider (e.g., trim and
ensure non-empty) and use encodeURIComponent(provider) when building the path
passed to this.client._request (replace `/api/v1/models/${provider}` with
`/api/v1/models/${encodeURIComponent(provider)}`), and throw or return an error
for invalid input to avoid sending malformed requests.
---
Nitpick comments:
In `@sdks/typescript/tests/integration.test.ts`:
- Around line 130-135: Update the test case in the "it('should list models for a
provider', ...)" block that calls client.models.list("openai") to not only
assert Array.isArray for models.language_models and models.embedding_models but,
when each array is non-empty, iterate their items and assert each item has a
string "value", a string "label", and if "default" is present it is a boolean;
use the existing test identifiers (client.models.list, models.language_models,
models.embedding_models) to locate the test and add these element-shape
assertions to catch response normalization regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 53124c11-e450-47f6-9b31-8abd51574131
📒 Files selected for processing (6)
sdks/typescript/README.mdsdks/typescript/src/client.tssdks/typescript/src/index.tssdks/typescript/src/models.tssdks/typescript/src/types.tssdks/typescript/tests/integration.test.ts
Comment on lines
+16
to
+18
| async list(provider: string): Promise<ModelsResponse> { | ||
| const response = await this.client._request("GET", `/api/v1/models/${provider}`); | ||
| const data = await response.json(); |
Contributor
There was a problem hiding this comment.
Encode and validate provider before building the request path.
Direct interpolation in Line 17 can produce invalid or unintended paths when provider contains /, spaces, or reserved characters. Add a basic guard and encodeURIComponent(provider) before calling _request.
Suggested fix
async list(provider: string): Promise<ModelsResponse> {
- const response = await this.client._request("GET", `/api/v1/models/${provider}`);
+ const normalizedProvider = provider.trim();
+ if (!normalizedProvider) {
+ throw new Error("provider is required");
+ }
+ const response = await this.client._request(
+ "GET",
+ `/api/v1/models/${encodeURIComponent(normalizedProvider)}`
+ );
const data = await response.json();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async list(provider: string): Promise<ModelsResponse> { | |
| const response = await this.client._request("GET", `/api/v1/models/${provider}`); | |
| const data = await response.json(); | |
| async list(provider: string): Promise<ModelsResponse> { | |
| const normalizedProvider = provider.trim(); | |
| if (!normalizedProvider) { | |
| throw new Error("provider is required"); | |
| } | |
| const response = await this.client._request( | |
| "GET", | |
| `/api/v1/models/${encodeURIComponent(normalizedProvider)}` | |
| ); | |
| const data = await response.json(); |
🤖 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 `@sdks/typescript/src/models.ts` around lines 16 - 18, The list method is
currently interpolating the raw provider into the request path, which can
produce invalid URLs; in async list(provider: string): Promise<ModelsResponse>
validate/normalize provider (e.g., trim and ensure non-empty) and use
encodeURIComponent(provider) when building the path passed to
this.client._request (replace `/api/v1/models/${provider}` with
`/api/v1/models/${encodeURIComponent(provider)}`), and throw or return an error
for invalid input to avoid sending malformed requests.
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
1 participant
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.
Adds TypeScript SDK support for listing provider models via the existing public
/v1/models/{provider}endpoint. This brings the TypeScript SDK to parity with the Python SDK, which already exposesclient.models.list(provider).What changed
client.models.list(provider).ModelsClient.ModelOptionandModelsResponsetypes.client.models.list("openai").Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests