Conversation
WalkthroughAdds embeddings support: new EmbeddingService, new POST /api/ai/embeddings route with validation and error handling (route appears duplicated in-file), Zod/OpenAPI schemas and TS types, a manual integration test script, and reduced AI usage log verbosity. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@backend/src/api/routes/ai/index.routes.ts`:
- Around line 477-513: Add a rate limiter middleware to the embeddings route:
create an express-rate-limit instance (e.g., embeddingsRateLimiter using
rateLimit with sensible windowMs/max settings) and insert it into the middleware
chain for the router.post('/embeddings') route (for example:
router.post('/embeddings', embeddingsRateLimiter, verifyUser, ...)). Ensure you
import rateLimit from 'express-rate-limit', configure the limiter, and apply it
before the handler that calls EmbeddingService.getInstance().createEmbeddings to
enforce limits on this cost-heavy endpoint.
In `@backend/src/services/ai/ai-usage.service.ts`:
- Around line 93-121: In trackEmbeddingUsage, stop computing outputTokens when
either totalTokens or inputTokens is undefined and ensure outputTokens is
clamped to >=0 so zeros are preserved; specifically, change the logic in
trackEmbeddingUsage to: only compute outputTokens when both inputTokens and
totalTokens are numbers, set outputTokens = Math.max(0, totalTokens -
inputTokens), and pass null for input_tokens/output_tokens in the INSERT only
when the corresponding value is undefined (not when it's 0), updating the values
array and logger usage accordingly.
In `@backend/tests/manual/test-ai-embeddings.sh`:
- Line 219: The script always prints "print_success" and exits 0 even when
track_test_failure sets TEST_FAILED; update the end of the test (around the
print_success "🎉 AI Embeddings test completed!" call) to check the TEST_FAILED
variable and if set exit with a non-zero status (e.g., call print_failure or
echo an error and exit 1), otherwise print_success and exit 0; reference the
track_test_failure function and TEST_FAILED variable to locate where to add this
conditional exit.
In `@shared-schemas/src/ai-api.schema.ts`:
- Around line 159-189: The embeddingObjectSchema currently forces embedding:
z.array(z.number()) which breaks for encoding_format="base64"; change
embeddingObjectSchema to use a union for embedding:
z.union([z.array(z.number()), z.string()]) so embeddings can be either number[]
or base64 string, and update any code that maps/normalizes responses (the
service mapping that currently does an unsafe "as number[]" cast) to stop
casting—preserve the embedding field as number[] | string and add explicit
handling: if embedding is a string decode base64 to numbers in a dedicated
helper, otherwise pass through the number[]; update signatures/types accordingly
so embedding is typed as number[] | string (symbols: embeddingObjectSchema,
embeddingsResponseSchema and the service mapping that performs the unsafe cast).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@backend/src/services/ai/embedding.service.ts`:
- Line 4: The import for AIConfigService in embedding.service.ts is missing the
.js extension which breaks ESM resolution; update the relative import statement
that references AIConfigService (symbol: AIConfigService) to include the .js
extension (e.g., change './ai-config.service' to './ai-config.service.js') so it
matches other imports like './ai-usage.service.js' and '@/utils/logger.js' and
ensures consistent ESM module resolution at runtime.
♻️ Duplicate comments (1)
backend/src/services/ai/embedding.service.ts (1)
48-67: Duplicate: guard and preserve token counts for embeddings usage.This hits the same concern already raised about embeddings usage tracking—token counts should be guarded and zero values preserved so usage data isn’t skewed. Please apply the earlier fix in AIUsageService to this path as well.
…ESM module resolution
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@backend/src/services/ai/embedding.service.ts`:
- Around line 56-68: The embedding service is incorrectly calling
this.aiUsageService.trackChatUsage(...) and using chat-oriented token math;
replace that with a dedicated embedding tracking call: add a new method on
AiUsageService (e.g., trackEmbeddingUsage(aiConfigId, inputTokens, model,
metadata?)) and call it from embedding.service.ts instead of trackChatUsage,
passing tokenUsage.promptTokens as inputTokens and a fixed 0 for outputTokens
(or omit output tokens), and ensure the new method persists a
usage_type='embedding' (or adds a usage_type column in the ai_usage schema) so
embedding usage can be distinguished in analytics/billing; update any
callers/tests accordingly to use trackEmbeddingUsage and options.model for the
model id.
🧹 Nitpick comments (2)
backend/src/services/ai/embedding.service.ts (2)
31-31: Consider logging when AI config is not found for the model.If
findByModelIdreturns null, the code proceeds silently without usage tracking. Adding a debug/warning log here would help identify misconfigured models during development.♻️ Suggested improvement
const aiConfig = await this.aiConfigService.findByModelId(options.model); + if (!aiConfig) { + logger.debug('No AI config found for model, usage tracking will be skipped', { + model: options.model, + }); + } const response = await this.openRouterProvider.sendRequest((client) =>
83-91: Consider usinglogger.errorfor exceptions.Using
warnlevel for errors that cause exceptions to be thrown may make production issues harder to detect in alerting systems that filter by log level.♻️ Suggested change
} catch (error) { - logger.warn('Embedding error', { + logger.error('Embedding generation failed', { error: error instanceof Error ? error.message : String(error), model: options.model, });
Fermionic-Lyu
left a comment
There was a problem hiding this comment.
Code looks good 2 me
Summary
add new endpoint to support embedding:
POST /api/ai/embeddingsHow did you test this change?
run test-ai-embedding.sh manually.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.