cleanup vertex api.ts #1350
cleanup vertex api.ts #1350
Conversation
DescriptionSummary By MatterAI
🔄 What Changed
🔍 Impact of the Change
📁 Total Files Changed
🧪 Test Added
🔒Security Vulnerabilities
MotivationN/A Type of Change
How Has This Been Tested?
Screenshots (if applicable)N/A Checklist
Related IssuesN/A Tanka Poem ♫
Sequence DiagramN/A
|
![matter-code-review[bot]](https://avatars.githubusercontent.com/in/474549?s=60&v=4){kind=small}
| constructor( | ||
| message: string, | ||
| public status: number = 500, | ||
| public cause?: Error | ||
| ) { | ||
| super(message); | ||
| this.name = 'GatewayError'; | ||
| this.status = status; | ||
| } |
There was a problem hiding this comment.
🐛 Bug Fix
Issue: Redundant assignment of status property in constructor
Fix: Remove redundant assignment since it's already initialized in parameter
Impact: Cleaner code, no functional change
| constructor( | |
| message: string, | |
| public status: number = 500, | |
| public cause?: Error | |
| ) { | |
| super(message); | |
| this.name = 'GatewayError'; | |
| this.status = status; | |
| } | |
| constructor( | |
| message: string, | |
| public status: number = 500, | |
| public cause?: Error | |
| ) { | |
| super(message); | |
| this.name = 'GatewayError'; | |
| } |
| }), | ||
| { | ||
| status: 500, | ||
| status: error instanceof GatewayError ? error.status : 500, |
There was a problem hiding this comment.
🛠️ Code Refactor
Issue: Hardcoded error status code
Fix: Use status from GatewayError if available, otherwise default to 500
Impact: More accurate error status propagation
| status: error instanceof GatewayError ? error.status : 500, | |
| status: error instanceof GatewayError ? error.status : 500, |
| import { getModelAndProvider, getAccessToken, getBucketAndFile } from './utils'; | ||
|
|
||
| const getApiVersion = (provider: string, inputModel: string) => { | ||
| const getApiVersion = (provider: string) => { |
There was a problem hiding this comment.
🛠️ Code Refactor
Issue: Unused parameter inputModel in getApiVersion function
Fix: Remove unused parameter
Impact: Cleaner function signature
| const getApiVersion = (provider: string) => { | |
| const getApiVersion = (provider: string) => { |
| } = providerOptions; | ||
| let projectId = inputProjectId; | ||
| if (vertexServiceAccountJson && vertexServiceAccountJson.project_id) { | ||
| if (vertexServiceAccountJson?.project_id) { |
There was a problem hiding this comment.
🛠️ Code Refactor
Issue: Unnecessary explicit check for nested property
Fix: Use optional chaining for cleaner code
Impact: More concise and readable code
| if (vertexServiceAccountJson?.project_id) { | |
| if (vertexServiceAccountJson?.project_id) { |
There was a problem hiding this comment.
✅ Resolved: Previous suggestion was for optional chaining which is not needed as the code already handles null case correctly
|
|
||
| const { provider } = getModelAndProvider(inputModel as string); | ||
| let routeVersion = getApiVersion(provider, inputModel as string); | ||
| const routeVersion = getApiVersion(provider); |
There was a problem hiding this comment.
🛠️ Code Refactor
Issue: Unused parameter in function call
Fix: Remove unused inputModel parameter
Impact: Cleaner function call
| const routeVersion = getApiVersion(provider); | |
| const routeVersion = getApiVersion(provider); |
|
|
||
| const url = new URL(gatewayRequestURL); | ||
| const searchParams = url.searchParams; | ||
| const searchParams = new URLSearchParams(url.search); |
There was a problem hiding this comment.
🛠️ Code Refactor
Issue: Inconsistent URL search params initialization
Fix: Use new URLSearchParams(url.search) for consistency
Impact: More consistent code pattern
| const searchParams = new URLSearchParams(url.search); | |
| const searchParams = new URLSearchParams(url.search); |
| default: | ||
| return ''; |
There was a problem hiding this comment.
🛠️ Code Refactor
Issue: Missing default case in switch statement
Fix: Add default case returning empty string
Impact: Prevents potential undefined returns
| default: | |
| return ''; | |
| default: | |
| return ''; |
| if (!inputModel) { | ||
| throw new GatewayError('Model is required', 400); | ||
| } |
There was a problem hiding this comment.
🔒 Security Issue Fix
Issue: Missing model validation could lead to unexpected behavior
Fix: Add explicit model validation with GatewayError
Impact: Prevents processing requests without required model parameter
| if (!inputModel) { | |
| throw new GatewayError('Model is required', 400); | |
| } | |
| if (!inputModel) { | |
| throw new GatewayError('Model is required', 400); | |
| } |
| } else if (mappedFn === 'messagesCountTokens') { | ||
| return `${projectRoute}/publishers/${provider}/models/count-tokens:rawPredict`; | ||
| } | ||
| return `${projectRoute}/publishers/${provider}/models/${model}:rawPredict`; |
There was a problem hiding this comment.
🛠️ Code Refactor
Issue: Missing default return in switch statement
Fix: Add default return for rawPredict endpoint
Impact: Ensures consistent return value
| return `${projectRoute}/publishers/${provider}/models/${model}:rawPredict`; | |
| return `${projectRoute}/publishers/${provider}/models/${model}:rawPredict`; |
|
Code cleanup and image edit endpoint support added across multiple providers |
|
Minor cleanup in Vertex AI API header construction |
No description provided.