.#9
Merged
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the embedding service architecture to consolidate embedding operations in the LLM backend and improves error handling and null safety across the codebase. The minimal title "." does not adequately describe the changes made.
Changes:
- Refactored embedding service to call LLM backend's
/rag/embeddingsendpoint instead of directly calling Ollama from the backend - Removed duplicate exception handler in Python chat endpoint
- Improved null safety in Dart models with defensive parsing and default values
- Increased API timeout values and added sendTimeout configuration
- Removed unused
content_textdatabase column updates and cleaned up environment variables
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| backend/src/services/llm_backend.service.ts | Refactored embedTexts() to call LLM backend instead of Ollama directly, simplified retry logic |
| backend/src/services/document_rag.service.ts | Removed content_text column from database UPDATE queries |
| backend/src/config/env.ts | Removed OLLAMA_EMBEDDINGS_URL configuration (no longer needed) |
| scripts/dev_start.sh | Removed OLLAMA_EMBEDDINGS_URL and EMBEDDING_MODEL environment variable exports |
| llm_backend/app/api/chat.py | Removed duplicate exception handler block |
| lib/shared/services/api_service.dart | Increased timeout values (connect: 15→30s, receive: 15→60s, added send: 60s) |
| lib/shared/models/models.dart | Added null-safe parsing for Document fields (title, summary, uploaded_at) |
| lib/features/auth/data/api_auth_repository.dart | Added null-safe parsing for AuthUser fields |
| lib/shared/data/api_document_repository.dart | Added debug print statements for API responses and errors |
| lib/features/home/presentation/widgets/procedural_tree.dart | Removed unused constant variable branches |
Comments suppressed due to low confidence (2)
lib/shared/models/models.dart:164
- The handling of null
uploaded_atvalues could mask backend data issues. Since the database schema hasuploaded_atas NOT NULL with a default value, it should always be present. Consider logging a warning or throwing an error whenuploadedAtRawis not a valid string to make debugging easier. The current implementation silently falls back to DateTime.now() which could lead to incorrect timestamps if the backend has issues.
DateTime? uploadedAt;
final uploadedAtRaw = json['uploaded_at'];
if (uploadedAtRaw is String) {
uploadedAt = DateTime.tryParse(uploadedAtRaw);
}
return Document(
id: json['id'] as String?,
title: (json['title'] as String?) ?? 'Untitled',
summary: (json['summary'] as String?) ?? '',
status: status,
uploadedAt: uploadedAt,
backend/src/config/env.ts:36
- The EMBEDDING_MODEL configuration field is defined in the interface and exported but is not used anywhere in the backend service since embeddings are now handled by the LLM backend. Consider removing this unused configuration field from both the interface (line 11) and the config object (line 36) to keep the codebase clean and reduce confusion, or add a comment explaining why it's kept.
EMBEDDING_MODEL: string;
}
function validateEnv(): EnvConfig {
const nodeEnv = process.env.NODE_ENV || 'development';
const port = parseInt(process.env.PORT || '3000', 10);
// DATABASE_URL priority: use it if exists, otherwise construct from PG* vars
let databaseUrl = process.env.DATABASE_URL;
if (!databaseUrl) {
const host = process.env.PGHOST || 'localhost';
const pgPort = process.env.PGPORT || '5432';
const database = process.env.PGDATABASE || 'learning_coach_dev';
const user = process.env.PGUSER || 'postgres';
const password = process.env.PGPASSWORD || 'postgres';
databaseUrl = `postgres://${user}:${password}@${host}:${pgPort}/${database}?sslmode=disable`;
}
return {
NODE_ENV: nodeEnv,
PORT: port,
DATABASE_URL: databaseUrl,
LLM_BACKEND_URL: process.env.LLM_BACKEND_URL || 'http://localhost:8000',
EMBEDDING_MODEL: process.env.EMBEDDING_MODEL || 'nomic-embed-text',
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
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.
No description provided.