Conversation
github-actions
Bot
changed the title
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaces in-file chat utilities and the embedded completions schema with imports from new modules, and adds three utility modules (image conversion, token estimation, free-model check) plus a standalone completions schema file. Tests/imports updated accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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.
Pull request overview
This PR refactors the chat handler by extracting schemas and utility functions from the main chat.ts file into dedicated modules, improving code organization and maintainability.
Changes:
- Created a new schemas module (
completions.ts) with the completions request schema and type - Extracted three utility functions into separate modules:
estimateTokensFromContent,convertImagesToBase64, andisModelTrulyFree - Updated
chat.tsto import these new modules and maintained backward compatibility via re-export
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/gateway/src/chat/schemas/completions.ts | New schema module containing completions request schema and type definition |
| apps/gateway/src/chat/tools/estimate-tokens-from-content.ts | Token estimation utility extracted from chat.ts |
| apps/gateway/src/chat/tools/convert-images-to-base64.ts | Image URL to base64 conversion utility extracted from chat.ts |
| apps/gateway/src/chat/tools/is-model-truly-free.ts | Model free-tier check utility extracted from chat.ts |
| apps/gateway/src/chat/chat.ts | Main chat handler updated to import from new modules with backward-compatible re-export |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * Estimates tokens from content length using simple division | ||
| */ | ||
| export function estimateTokensFromContent(content: string): number { | ||
| return Math.max(1, Math.round(content.length / 4)); | ||
| } |
There was a problem hiding this comment.
This extracted utility function lacks dedicated test coverage. While it's tested indirectly through apps/gateway/src/lib/prompt-tokens.spec.ts, consider adding unit tests in the tools folder similar to heal-json-response.spec.ts to ensure the function works correctly in isolation.
| import { logger } from "@llmgateway/logger"; | ||
|
|
||
| import type { ImageObject } from "./types.js"; | ||
|
|
||
| /** | ||
| * Converts external image URLs to base64 data URLs | ||
| * Used for providers like Alibaba that return external URLs instead of base64 | ||
| */ | ||
| export async function convertImagesToBase64( | ||
| images: ImageObject[], | ||
| ): Promise<ImageObject[]> { | ||
| return await Promise.all( | ||
| images.map(async (image): Promise<ImageObject> => { | ||
| const url = image.image_url.url; | ||
| // Skip if already a data URL | ||
| if (url.startsWith("data:")) { | ||
| return image; | ||
| } | ||
|
|
||
| try { | ||
| const response = await fetch(url); | ||
| if (!response.ok) { | ||
| logger.warn("Failed to fetch image for base64 conversion", { | ||
| url, | ||
| status: response.status, | ||
| }); | ||
| return image; | ||
| } | ||
|
|
||
| const contentType = response.headers.get("content-type") || "image/png"; | ||
| const arrayBuffer = await response.arrayBuffer(); | ||
| const base64 = Buffer.from(arrayBuffer).toString("base64"); | ||
|
|
||
| return { | ||
| type: "image_url", | ||
| image_url: { | ||
| url: `data:${contentType};base64,${base64}`, | ||
| }, | ||
| }; | ||
| } catch (error) { | ||
| logger.warn("Error converting image to base64", { | ||
| url, | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }); | ||
| return image; | ||
| } | ||
| }), | ||
| ); | ||
| } |
There was a problem hiding this comment.
This extracted function lacks dedicated test coverage. Consider adding unit tests to verify error handling (failed fetch, network errors), data URL detection, and successful base64 conversion.
| import type { ModelDefinition } from "@llmgateway/models"; | ||
|
|
||
| /** | ||
| * Checks if a model is truly free (has free flag AND no per-request pricing) | ||
| */ | ||
| export function isModelTrulyFree(modelInfo: ModelDefinition): boolean { | ||
| if (!modelInfo.free) { | ||
| return false; | ||
| } | ||
| // Check if any provider has a per-request cost | ||
| return !modelInfo.providers.some((p) => p.requestPrice && p.requestPrice > 0); | ||
| } |
There was a problem hiding this comment.
This extracted function lacks dedicated test coverage. Consider adding unit tests to verify behavior when models have the free flag but have per-request pricing, when models don't have the free flag, and when models are truly free.
steebchen
changed the title
![terragon-labs[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
…del utils - Move completions request schema to separate completions.ts file for clarity - Extract convertImagesToBase64 utility to dedicated tool file - Extract estimateTokensFromContent to separate tool - Extract isModelTrulyFree utility for model pricing checks - Remove duplicates and redundant code from chat.ts to improve maintainability Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
…d update imports Removed the re-export of estimateTokensFromContent from the chat module to clean up legacy code. Updated tests to import estimateTokensFromContent directly from the tools directory for clarity and maintainability. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
c2291e9 to
f042781
Compare
|
superseded by #1537 |
Summary
Changes
Core modularization
apps/gateway/src/chat/schemas/completions.tscompletionsRequestSchemaandCompletionsRequesttypeapps/gateway/src/chat/tools/estimate-tokens-from-content.ts(token estimation based on content length)apps/gateway/src/chat/tools/convert-images-to-base64.ts(convert external image URLs to data URLs)apps/gateway/src/chat/tools/is-model-truly-free.ts(determine if a model is truly free)apps/gateway/src/chat/chat.tsupdated to import and use the new modules:completionsRequestSchemafrom./schemas/completions.jsconvertImagesToBase64from./tools/convert-images-to-base64.jsestimateTokensFromContentfrom./tools/estimate-tokens-from-content.jsisModelTrulyFreefrom./tools/is-model-truly-free.jsBackward compatibility
estimateTokensFromContentfrom./tools/estimate-tokens-from-content.jsto preserve existing importsCode cleanup
completionsRequestSchema(moved to new schemas module)estimateTokensFromContent(moved to new helpers)isModelTrulyFree(moved to new helpers)convert-images-to-base64.ts)Why
Tests / Validation
convert-images-to-base64utilityisModelTrulyFreefrom the new module when neededAdditional notes
🌿 Generated by Terry
ℹ️ Tag @terragon-labs to ask questions and address PR feedback
📎 Task: https://www.terragonlabs.com/task/d400513a-5888-4144-8eda-2ff9fc79ff08
Summary by CodeRabbit
Refactor
New Features
✏️ Tip: You can customize this high-level summary in your review settings.