fix: bug fixes in backend and frontend integration #155
Conversation
WalkthroughUpdates adjust error handling and logging across backend and frontend: backend adds exception tracebacks and changes Weaviate client to create per-context clients (removing module singleton); queue manager enforces connectivity and serializes/persists messages; frontend silences several console.error calls and adjusts error handlers to reset UI state or redirect on 401. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Producer
participant QueueMgr
participant Broker
Producer->>QueueMgr: enqueue(message, priority)
alt no channel / closed
QueueMgr-->>Producer: raise ConnectivityError
else channel OK
QueueMgr->>QueueMgr: serialize priority as string
QueueMgr->>Broker: publish(Message persistent, content-type=application/json)
Broker-->>QueueMgr: ack
end
sequenceDiagram
autonumber
actor Worker
participant QueueMgr
participant Processor
Worker->>QueueMgr: _worker loop receives message
QueueMgr->>Processor: process(message)
alt processing error
Processor-->>QueueMgr: error
QueueMgr-->>QueueMgr: log error (exc_info=True)
else success
Processor-->>QueueMgr: success -> ack
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.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 (2)
backend/app/core/orchestration/queue_manager.py (1)
82-91: Blocker: Enum is not JSON‑serializable in queue_item.json.dumps(queue_item) will fail because QueuePriority is an Enum.
Apply this diff:
- queue_item = { + queue_item = { "id": message.get("id", f"msg_{datetime.now().timestamp()}"), - "priority": priority, + "priority": priority.value, "data": message }backend/app/database/weaviate/client.py (1)
8-16: Client lifetime: cached global is closed each use.get_client caches _client, but the context manager closes it in finally. Next use reuses a closed client. Either don’t cache or reset the cache after closing.
Option A (simplest): drop caching.
-_client = None - - -def get_client(): - """Get or create the global Weaviate client instance.""" - global _client - if _client is None: - _client = weaviate.use_async_with_local() - return _client +def get_client(): + """Create a new async Weaviate client instance per context use.""" + return weaviate.use_async_with_local()Option B: keep caching but reset after close (see next comment for diff).
🧹 Nitpick comments (13)
frontend/src/lib/api.ts (1)
164-171: Empty catch block hinders debugging and observability.The empty catch block makes it impossible to debug why health checks fail in production. While returning
falseon errors is appropriate, completely suppressing error details contradicts the PR's goal of "better debugging through full stack traces."Consider logging errors in development or using structured logging:
async healthCheck(): Promise<boolean> { try { const response = await this.client.get('/v1/health'); return response.status === 200; - } catch { + } catch (error) { + // Log in development for debugging + if (import.meta.env.DEV) { + console.error('Health check failed:', error); + } return false; } }Alternatively, integrate a proper observability service (e.g., Sentry) for production error tracking.
frontend/src/components/integration/BotIntegration.tsx (1)
49-53: Silent error handling removes user feedback and debugging visibility.While resetting state (lines 51-52) correctly ensures UI consistency, the completely silent error handling provides no feedback to users about why integration status couldn't be loaded. Users cannot distinguish between "integration not connected" and "failed to check status due to network/API error." The component already uses
toast(imported on line 3), but doesn't leverage it here.Consider providing user feedback while maintaining state reset:
- } catch { + } catch (error) { // Silently handle integration status loading errors setIsConnected(false); setIntegration(null); + // Optionally notify user of loading failure + if (import.meta.env.DEV) { + console.error('Failed to load integration status:', error); + } + // Consider: toast.error('Failed to load integration status'); }This improves observability while preserving the defensive state-reset behavior.
backend/app/core/orchestration/queue_manager.py (6)
44-45: Good: include traceback on connection failures.exc_info=True here is appropriate and keeps control flow unchanged. Consider standardizing this across other error logs in this module.
73-91: Guard against publishing while disconnected.enqueue assumes self.channel is ready; calling before start() or after stop() will raise.
Apply this diff:
async def enqueue(self, message: Dict[str, Any], priority: QueuePriority = QueuePriority.MEDIUM, delay: float = 0): """Add a message to the queue""" if delay > 0: await asyncio.sleep(delay) + if not self.channel or self.channel.is_closed: + raise RuntimeError("Queue channel is not connected. Call start() before enqueue().")
112-118: Add traceback to processing errors and avoid f-strings in logs.Include exc_info and defer string formatting to logger.
Apply this diff:
- except Exception as e: - logger.error(f"Error processing message: {e}") + except Exception as e: + logger.error("Error processing message: %s", e, exc_info=True) await message.nack(requeue=False)
122-124: Add traceback for worker loop errors.This helps diagnose queue.get/JSON errors.
Apply this diff:
- except Exception as e: - logger.error(f"Worker {worker_name} error: {e}") + except Exception as e: + logger.error("Worker %s error: %s", worker_name, e, exc_info=True)
141-142: Consider DLQ for unknown message types.Right now, unknown types are logged and then acked (since ack happens after _process_item). If you want post‑mortems, route unknowns to a dead‑letter exchange with a reason header instead of silently dropping.
88-91: Explicit delivery_mode required for message persistence in aio-pika 9.5.5.aio-pika 9.5.5 defaults to DeliveryMode.NOT_PERSISTENT, meaning messages will be lost if the RabbitMQ broker crashes before workers consume them—regardless of queue durability. Apply the suggested diff to add explicit persistence:
await self.channel.default_exchange.publish( aio_pika.Message( + body=json_message, + delivery_mode=aio_pika.DeliveryMode.PERSISTENT, + content_type="application/json", ), routing_key=self.queues[priority] )backend/app/database/weaviate/client.py (5)
26-27: Good: add traceback; also switch to parameterized logging.Use logger’s formatting instead of f-strings (addresses RUF010, avoids eager str()).
Apply this diff:
- logger.error(f"Weaviate client error: {str(e)}", exc_info=True) + logger.error("Weaviate client error: %s", e, exc_info=True)
31-32: Likewise here: parameterized logging.Keeps consistency and satisfies the linter.
Apply this diff:
- except Exception as e: - logger.warning(f"Error closing Weaviate client: {str(e)}", exc_info=True) + except Exception as e: + logger.warning("Error closing Weaviate client: %s", e, exc_info=True)
28-32: If you keep caching: reset the global after closing.Prevents reuse of a closed client.
Apply this diff:
finally: try: await client.close() except Exception as e: - logger.warning(f"Error closing Weaviate client: {str(e)}", exc_info=True) + logger.warning("Error closing Weaviate client: %s", e, exc_info=True) + finally: + # Ensure future calls don't reuse a closed client + global _client + _client = None
3-9: Tighten typing for the cached client.Add Optional type to make intent explicit.
Apply this diff:
-from typing import AsyncGenerator +from typing import AsyncGenerator, Optional @@ -_client = None +_client: Optional[weaviate.WeaviateClient] = None
19-27: Narrow exception handler from broadExceptionto specific Weaviate exceptions.Replace
except Exceptionwithexcept weaviate.exceptions.WeaviateBaseErrorto catch only expected client errors (e.g., WeaviateConnectionError, UnexpectedStatusCodeException, WeaviateStartUpError, WeaviateGRPCUnavailableError). All Weaviate-specific exceptions inherit from weaviate.exceptions.WeaviateBaseError, which will handle connection, auth, and startup failures while allowing unexpected Python errors to propagate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/app/core/orchestration/queue_manager.py(1 hunks)backend/app/database/weaviate/client.py(1 hunks)frontend/src/App.tsx(0 hunks)frontend/src/components/integration/BotIntegration.tsx(1 hunks)frontend/src/lib/api.ts(2 hunks)
💤 Files with no reviewable changes (1)
- frontend/src/App.tsx
🧰 Additional context used
🪛 Ruff (0.14.1)
backend/app/database/weaviate/client.py
26-26: Use explicit conversion flag
Replace with conversion flag
(RUF010)
31-31: Do not catch blind exception: Exception
(BLE001)
32-32: Use explicit conversion flag
Replace with conversion flag
(RUF010)
|
could you please review the coderabit suggested changes @Aditya30ag? |
|
Yaa sure |
|
@smokeyScraper Done |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
frontend/src/lib/api.ts (1)
174-177: Consider adding production error monitoring.While silencing health check errors in production reduces console noise (aligned with the PR objectives), completely muting them may hide connectivity issues. Consider sending these errors to a monitoring service (e.g., Sentry, DataDog) for production observability.
Example implementation:
} catch (error) { // Log in development for debugging if (import.meta.env.DEV) { console.error('Health check failed:', error); + } else { + // Send to monitoring service in production + // Example: Sentry.captureException(error); } return false; }backend/app/core/orchestration/queue_manager.py (2)
82-84: Good defensive check. Consider custom exception class.The connectivity guard prevents publishing to a closed/missing channel, which is excellent defensive coding. The static analysis hint about the long error message is valid but minor—consider extracting this to a custom exception class if you prefer stricter adherence to style guidelines.
Optional refactor to address the static analysis hint:
Create a custom exception at the top of the file:
class QueueNotConnectedError(RuntimeError): """Raised when attempting to enqueue without an active channel connection.""" def __init__(self): super().__init__("Queue channel is not connected. Call start() before enqueue().")Then use it:
- if not self.channel or self.channel.is_closed: - raise RuntimeError("Queue channel is not connected. Call start() before enqueue().") + if not self.channel or self.channel.is_closed: + raise QueueNotConnectedError()
151-152: Addexc_info=Truefor consistency.For consistency with the error logging improvements at lines 44, 125, and 131, consider adding
exc_info=Truehere as well to capture full stack traces when processing items fails.Apply this diff:
- logger.error(f"Error processing item {item.get('id', 'unknown')}: {str(e)}") + logger.error(f"Error processing item {item.get('id', 'unknown')}: {str(e)}", exc_info=True)backend/app/database/weaviate/client.py (1)
26-27: Consider narrowing the exception type in cleanup block.While catching broad
Exceptionin the finally block ensures cleanup doesn't crash, you could narrow it toweaviate.exceptions.WeaviateBaseErrorfor consistency with the main exception handler. The addition ofexc_info=Trueis good and aligns with the PR's debugging improvements.Apply this diff for more specific exception handling:
try: await client.close() - except Exception as e: + except weaviate.exceptions.WeaviateBaseError as e: logger.warning("Error closing Weaviate client: %s", e, exc_info=True)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/app/core/orchestration/queue_manager.py(3 hunks)backend/app/database/weaviate/client.py(2 hunks)frontend/src/components/integration/BotIntegration.tsx(1 hunks)frontend/src/lib/api.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/integration/BotIntegration.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/lib/api.ts (1)
frontend/src/lib/supabaseClient.ts (1)
supabase(12-12)
🪛 Ruff (0.14.2)
backend/app/database/weaviate/client.py
26-26: Do not catch blind exception: Exception
(BLE001)
backend/app/core/orchestration/queue_manager.py
84-84: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (7)
frontend/src/lib/api.ts (1)
86-94: Excellent fix for 401 handling!This implementation properly addresses the previous review comment about incomplete 401 error handling. The solution correctly:
- Signs out the user via Supabase
- Prevents infinite redirect loops by checking the current pathname
- Preserves the return URL for post-login navigation
- Still rejects the error for caller handling
The async/await pattern for
signOut()ensures the session is cleared before redirecting.backend/app/core/orchestration/queue_manager.py (4)
44-44: LGTM! Improved exception logging.Adding
exc_info=Truecaptures the full stack trace when RabbitMQ connection fails, which significantly aids debugging of connection issues.
88-88: LGTM! Explicit enum serialization.Using
priority.valuemakes the enum-to-string conversion explicit, ensuring consistent JSON serialization and improving code clarity.
93-97: Excellent improvement to message durability.Configuring messages with
PERSISTENTdelivery mode ensures they survive RabbitMQ restarts, and setting thecontent_typeheader improves message semantics. This pairs well with the durable queue declaration on line 41.
125-125: LGTM! Consistent exception logging improvements.Adding
exc_info=Trueto both message processing errors (line 125) and worker errors (line 131) provides full stack traces for debugging, consistent with the logging improvement at line 44.Also applies to: 131-131
backend/app/database/weaviate/client.py (2)
2-2: LGTM: Improved exception handling.The addition of
weaviate.exceptionsimport and the switch to catching specificWeaviateBaseErrorinstead of genericExceptionimproves error handling specificity. Theexc_info=Trueparameter successfully addresses the PR objective of capturing full stack traces for better debugging.Also applies to: 20-21
9-11: Based on my verification of Weaviate's official documentation, the per-context client pattern in the code is correct and recommended. Weaviate's documentation explicitly shows using the async context manager pattern withasync with weaviate.use_async_with_local()where the connection is automatically closed when the context is exited. The context manager is used to ensure that the client is connected to the server during data operations.The original review comment raised concerns about performance overhead and resource exhaustion from creating new clients per context. However, this pattern aligns with Weaviate's documented best practices for async clients. Weaviate recommends considering the asynchronous client API to improve performance in asynchronous environments.
While connection pooling is mentioned as a best practice to reduce overhead by allowing multiple requests to share a single connection, this is an optional optimization rather than a requirement. The current per-context implementation is a valid, documented pattern that ensures proper resource lifecycle management.
2 participants
Bugs Fixed in Devr.AI Project
This document lists the bugs that have been identified and fixed in the Devr.AI project.
✅ Fixed Bugs
1. Console.error Statements in Production Code
Severity: Medium
Location: Frontend
Files Affected:
frontend/src/App.tsxfrontend/src/lib/api.tsfrontend/src/components/integration/BotIntegration.tsxIssue:
Console.error statements were left in production code, which can expose sensitive error information in browser console and impact performance.
Fix Applied:
Impact: Improved security and performance by removing unnecessary console logging.
2. Missing exc_info in Backend Exception Logging
Severity: Medium
Location: Backend
Files Affected:
backend/app/database/weaviate/client.pybackend/app/core/orchestration/queue_manager.pyIssue:
Exception logging without
exc_info=Truemakes debugging difficult as stack traces are not captured.Fix Applied:
exc_info=Trueto all exception logging statements in Weaviate clientexc_info=Trueto RabbitMQ connection error loggingImpact: Better debugging capabilities with full stack traces in logs.
3. Improved Error Handling in BotIntegration
Severity: Low
Location: Frontend
Files Affected:
frontend/src/components/integration/BotIntegration.tsxIssue:
Error in
loadIntegrationStatuswas only logged but didn't reset component state, potentially leaving UI in inconsistent state.Fix Applied:
Impact: More robust error handling and consistent UI state.
Summary by CodeRabbit
Refactor
Bug Fixes / Behavior