feat(embedding): support configurable vector dimensions for OpenRouter provider#756
Open
Chewji9875 wants to merge 1 commit into
Open
feat(embedding): support configurable vector dimensions for OpenRouter provider#756Chewji9875 wants to merge 1 commit into
Chewji9875 wants to merge 1 commit into
Conversation
|
@Chewji9875 is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughOpenRouter embedding provider now reads OPENROUTER_EMBEDDING_DIMENSIONS, validates it as a positive integer (fallback 1536), stores it on the instance, and sends it as ChangesOpenRouter Embedding Dimensions Configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Chewji9875
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/embedding-provider.test.ts (1)
196-206: ⚡ Quick winAdd test case for zero dimensions to match OpenAI provider test coverage.
The
OpenAIEmbeddingProvidertest suite includes a test case that rejects"0"as an invalid dimension value (lines 165-168). TheOpenRouterEmbeddingProvidersuite should include the same test for consistency, since zero dimensions are also invalid for OpenRouter.➕ Add missing test case
process.env["OPENROUTER_EMBEDDING_DIMENSIONS"] = "-10"; expect(() => new OpenRouterEmbeddingProvider("test-key")).toThrow( /OPENROUTER_EMBEDDING_DIMENSIONS must be a positive integer/, ); + + process.env["OPENROUTER_EMBEDDING_DIMENSIONS"] = "0"; + expect(() => new OpenRouterEmbeddingProvider("test-key")).toThrow( + /OPENROUTER_EMBEDDING_DIMENSIONS must be a positive integer/, + ); });🤖 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 `@test/embedding-provider.test.ts` around lines 196 - 206, Add a test asserting that OPENROUTER_EMBEDDING_DIMENSIONS="0" is treated as invalid in the OpenRouterEmbeddingProvider tests: update the test block around the invalid dimensions (the one creating new OpenRouterEmbeddingProvider("test-key")) to include a case that sets process.env["OPENROUTER_EMBEDDING_DIMENSIONS"]="0" and expects new OpenRouterEmbeddingProvider("test-key") to throw the same /OPENROUTER_EMBEDDING_DIMENSIONS must be a positive integer/ error, mirroring the OpenAIEmbeddingProvider coverage.src/providers/embedding/openrouter.ts (1)
20-31: ⚡ Quick winConsider stricter integer validation for the environment variable.
The current validation uses
parseInt(dimStr, 10), which silently truncates decimals and ignores trailing non-numeric characters:
"768.5"→ parsed as768"768abc"→ parsed as768This may surprise users who mistype the value. For clearer error messages and to match the "must be a positive integer" expectation, validate that the trimmed string contains only digits before parsing.
🔒 Stricter validation example
const dimStr = getEnvVar("OPENROUTER_EMBEDDING_DIMENSIONS"); if (dimStr !== undefined && dimStr.trim().length > 0) { - const parsed = parseInt(dimStr, 10); - if (!Number.isFinite(parsed) || parsed <= 0) { + const trimmed = dimStr.trim(); + if (!/^\d+$/.test(trimmed)) { + throw new Error( + `OPENROUTER_EMBEDDING_DIMENSIONS must be a positive integer, got: ${dimStr}`, + ); + } + const parsed = parseInt(trimmed, 10); + if (parsed <= 0) { throw new Error( `OPENROUTER_EMBEDDING_DIMENSIONS must be a positive integer, got: ${dimStr}`, );Alternatively, use
Number()withNumber.isInteger():const dimStr = getEnvVar("OPENROUTER_EMBEDDING_DIMENSIONS"); if (dimStr !== undefined && dimStr.trim().length > 0) { - const parsed = parseInt(dimStr, 10); - if (!Number.isFinite(parsed) || parsed <= 0) { + const parsed = Number(dimStr.trim()); + if (!Number.isInteger(parsed) || parsed <= 0) { throw new Error( `OPENROUTER_EMBEDDING_DIMENSIONS must be a positive integer, got: ${dimStr}`, );🤖 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 `@src/providers/embedding/openrouter.ts` around lines 20 - 31, The validation for OPENROUTER_EMBEDDING_DIMENSIONS (dimStr) is too permissive—replace the parseInt-based check in the block that reads getEnvVar and sets this.dimensions with a stricter digit-only validation: trim dimStr, ensure it matches only digits (e.g., /^\d+$/) before parsing to an integer, then verify >0; if it fails, throw the existing error message; otherwise set this.dimensions to the parsed integer (falling back to 1536 when undefined). Use the existing identifiers dimStr, getEnvVar, parseInt (or Number) and this.dimensions to locate and update the code.
🤖 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 `@src/providers/embedding/openrouter.ts`:
- Around line 20-31: The validation for OPENROUTER_EMBEDDING_DIMENSIONS (dimStr)
is too permissive—replace the parseInt-based check in the block that reads
getEnvVar and sets this.dimensions with a stricter digit-only validation: trim
dimStr, ensure it matches only digits (e.g., /^\d+$/) before parsing to an
integer, then verify >0; if it fails, throw the existing error message;
otherwise set this.dimensions to the parsed integer (falling back to 1536 when
undefined). Use the existing identifiers dimStr, getEnvVar, parseInt (or Number)
and this.dimensions to locate and update the code.
In `@test/embedding-provider.test.ts`:
- Around line 196-206: Add a test asserting that
OPENROUTER_EMBEDDING_DIMENSIONS="0" is treated as invalid in the
OpenRouterEmbeddingProvider tests: update the test block around the invalid
dimensions (the one creating new OpenRouterEmbeddingProvider("test-key")) to
include a case that sets process.env["OPENROUTER_EMBEDDING_DIMENSIONS"]="0" and
expects new OpenRouterEmbeddingProvider("test-key") to throw the same
/OPENROUTER_EMBEDDING_DIMENSIONS must be a positive integer/ error, mirroring
the OpenAIEmbeddingProvider coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 097bc5b2-21e7-461f-a4f6-aa728e485c8d
📒 Files selected for processing (2)
src/providers/embedding/openrouter.tstest/embedding-provider.test.ts
Chewji9875
force-pushed
the
feat/openrouter-dimensions
branch
from
9456c9d to
08879c7
Compare
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.
WHAT
Adds dynamic configuration support for OpenRouter embedding vector dimensions via the
OPENROUTER_EMBEDDING_DIMENSIONSenvironment variable, validating that inputs are positive finite integers, and defaulting to 1536.WHERE
src/providers/embedding/openrouter.ts(Class properties and constructor parsing/validation).test/embedding-provider.test.ts(Unit tests for default, custom, and invalid values, plus anode:fsmock to isolate testing).WHEN
When initialized via environment variables or explicitly instantiated, especially when users target custom or newer models on OpenRouter (e.g., NVIDIA's 2048-dim models) that deviate from OpenAI's standard 1536 dimensions.
WHY
To prevent runtime dimension mismatches and crashes when developers use non-1536 dimension models through the OpenRouter embedding provider, enhancing flexibility and type-safe integration.
Summary by CodeRabbit
New Features
Tests