feat(costs): update image token calculation based on size - #1210
Conversation
WalkthroughThis PR adds image size awareness to cost calculations by introducing an optional Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 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.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/gateway/src/chat/chat.ts (1)
3202-3216: Add enum validation forimage_sizeand normalize case-sensitivity in cost calculations.The concern is confirmed. The schema defines
image_sizeas a plain string with no validation (chat.ts:246), and the cost calculation inapps/gateway/src/lib/costs.ts:263only recognizes uppercase "4K", defaulting all other values (including "4k") to 1120 tokens instead of 2000:const TOKENS_PER_IMAGE = imageSize === "4K" ? 2000 : 1120;This creates a cost calculation bug if users pass lowercase variants or invalid values.
Fixes required:
- chat.ts:246 — Change
image_size: z.string().optional()toimage_size: z.enum(["1K", "2K", "4K"]).optional()- apps/gateway/src/lib/costs.ts:263 — Normalize case-sensitivity:
imageSize?.toUpperCase() === "4K"
🧹 Nitpick comments (2)
apps/gateway/src/lib/costs.ts (2)
68-88: Update JSDoc to document the new parameter.The function signature now includes
imageSize, but the JSDoc comment (lines 68-72) doesn't document this parameter.Apply this diff to update the documentation:
/** * Calculate costs based on model, provider, and token counts * If promptTokens or completionTokens are not available, it will try to calculate them * from the fullOutput parameter if provided + * @param imageSize - Optional image size (e.g., "4K") to determine per-image token count */ export function calculateCosts( model: Model,
260-264: Consider case-insensitive comparison and explicit value handling.The current implementation has a few potential issues:
Case sensitivity: The comparison
imageSize === "4K"is case-sensitive. If users pass "4k" (lowercase), it will default to 1120 tokens instead of 2000.Implicit defaults: The comment mentions "1K/2K images" but these values aren't explicitly handled—they rely on the fallback to 1120. This could be confusing.
Magic string: The "4K" string literal could be a constant or enum for better maintainability.
Consider one of these approaches:
Option 1: Case-insensitive with explicit handling
-const TOKENS_PER_IMAGE = imageSize === "4K" ? 2000 : 1120; +const normalizedSize = imageSize?.toUpperCase(); +const TOKENS_PER_IMAGE = + normalizedSize === "4K" ? 2000 : + normalizedSize === "2K" || normalizedSize === "1K" ? 1120 : + 1120; // defaultOption 2: Use constants
+const IMAGE_TOKEN_COUNTS = { + "4K": 2000, + "2K": 1120, + "1K": 1120, +} as const; +const DEFAULT_IMAGE_TOKENS = 1120; + -const TOKENS_PER_IMAGE = imageSize === "4K" ? 2000 : 1120; +const normalizedSize = imageSize?.toUpperCase() as keyof typeof IMAGE_TOKEN_COUNTS; +const TOKENS_PER_IMAGE = IMAGE_TOKEN_COUNTS[normalizedSize] ?? DEFAULT_IMAGE_TOKENS;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.gitignore(1 hunks)apps/gateway/src/chat/chat.ts(2 hunks)apps/gateway/src/lib/costs.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: lint / run
- GitHub Check: build / run
- GitHub Check: test / run
- GitHub Check: autofix
- GitHub Check: e2e-shards (1)
- GitHub Check: e2e-shards (2)
- GitHub Check: e2e-shards (4)
- GitHub Check: e2e-shards (3)
- GitHub Check: e2e-shards (5)
🔇 Additional comments (2)
.gitignore (1)
21-21: LGTM!Standard addition to ignore a temporary output file.
apps/gateway/src/chat/chat.ts (1)
3758-3772: LGTM - Consistent implementation.The
image_sizeparameter is correctly passed in the non-streaming path, maintaining consistency with the streaming implementation.
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.