fix: bug fixes in backend and frontend integration #155
Conversation
📝 WalkthroughWalkthroughBackend: tightened queue connectivity checks, message serialization to JSON with persistent delivery, and richer exc_info logging; migrated Weaviate client from a module singleton to per-context creation and narrowed exception handling. Frontend: silenced some console.error calls, added deduplicated 401 handling, and implemented validated returnUrl redirects on login. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Producer
participant QueueMgr
participant Broker
Producer->>QueueMgr: enqueue(message, priority)
alt no channel / channel closed
QueueMgr-->>Producer: raise ConnectivityError
else channel available
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: poll/receive message
QueueMgr->>Processor: process(message)
alt processing error
Processor-->>QueueMgr: error
QueueMgr-->>QueueMgr: log error (exc_info=True)
QueueMgr->>Worker: nack / handle failure
else success
Processor-->>QueueMgr: success
QueueMgr->>Worker: ack
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
|
hey @Aditya30ag, Issues to Address
|
| this.client.interceptors.response.use( | ||
| (response) => response, | ||
| (error) => { | ||
| async (error) => { | ||
| if (error.response?.status === 401) { | ||
| // Handle unauthorized - could redirect to login | ||
| console.error('Unauthorized request'); | ||
| // Clear session and redirect to login | ||
| // Avoid infinite redirect loop if already on login page | ||
| if (!window.location.pathname.includes('/login')) { | ||
| await supabase.auth.signOut(); | ||
| const returnUrl = encodeURIComponent(window.location.pathname + window.location.search); | ||
| window.location.href = `/login?returnUrl=${returnUrl}`; | ||
| } |
There was a problem hiding this comment.
returnUrl is added but never consumed by LoginPage.tsx
There was a problem hiding this comment.
solve this issue also
frontend/src/lib/api.ts
Outdated
| console.error('Unauthorized request'); | ||
| // Clear session and redirect to login | ||
| // Avoid infinite redirect loop if already on login page | ||
| if (!window.location.pathname.includes('/login')) { |
There was a problem hiding this comment.
Misses /forgot-password, /reset-password, /signup; users on these pages will be incorrectly redirected.
There was a problem hiding this comment.
const PUBLIC_PATHS = [
'/login',
'/signup',
'/forgot-password',
'/reset-password',
];
added this
|
Could you please make these changes @Aditya30ag ? Rest, the PR seems ready to be merged. |
|
@smokeyScraper yeah sure i will do the changes |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
frontend/src/components/pages/LoginPage.tsx (1)
78-81: Unused exception variable.The caught exception
eis not logged or used. Consider removing it or prefixing with underscore to indicate intentional discard.Proposed fix
- } catch (e) { + } catch { // If decoding fails, fallback to home navigate('/', { replace: true }); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/pages/LoginPage.tsxfrontend/src/lib/api.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: smokeyScraper
Repo: AOSSIE-Org/Devr.AI PR: 85
File: tests/test_supabase.py:1-3
Timestamp: 2025-06-28T14:45:55.244Z
Learning: In the Devr.AI project, smokeyScraper prefers to defer comprehensive test refactoring to separate PRs/efforts when doing major backend restructuring, rather than expanding the scope of the current refactoring PR to include test updates.
Learnt from: smokeyScraper
Repo: AOSSIE-Org/Devr.AI PR: 87
File: tests/test_supabase.py:1-3
Timestamp: 2025-06-28T23:15:13.374Z
Learning: In the Devr.AI project, smokeyScraper prefers to defer test updates and fixes (like missing imports after module reorganization) to separate PRs rather than expanding the scope of module update/chore PRs to include comprehensive test refactoring.
🧬 Code graph analysis (1)
frontend/src/lib/api.ts (1)
frontend/src/lib/supabaseClient.ts (1)
supabase(12-12)
🔇 Additional comments (3)
frontend/src/lib/api.ts (3)
11-17: LGTM!The
PUBLIC_PATHSarray correctly includes all auth-related routes referenced in the codebase, and thestartsWithcheck properly handles nested paths. This addresses the previously raised concern about missing public routes.
121-124: Ensure consistent open-redirect protection with LoginPage.The
returnUrlencoding here is correct. However, sincepathnamecomes fromwindow.location.pathname, it's trusted. The concern is on the consumption side (LoginPage.tsx) where thestartsWith('/')check should be strengthened to prevent//evil.comstyle attacks — see the related comment on that file.
200-211: LGTM!Limiting health check error logging to development mode aligns with the PR objective to reduce console noise in production.
|
Please have a look done the requested changes |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/pages/LoginPage.tsx (1)
90-93:⚠️ Potential issue | 🟡 MinorMinor: typo in error message.
Line 92:
"An Unknown error occured!"— "occured" should be "occurred".Proposed fix
- toast.error(error?.message ||"An Unknown error occured!"); + toast.error(error?.message || "An unknown error occurred!");
🧹 Nitpick comments (2)
frontend/src/lib/api.ts (1)
96-135: Potential information leakage throughreturnUrlwhen query strings contain sensitive data.Line 122 captures
pathname + window.location.searchinto thereturnUrl. If any protected page passes sensitive tokens or identifiers as query parameters, those will be encoded into the login URL and may appear in browser history, server logs, or referrer headers.Consider stripping query parameters and only preserving the pathname in the
returnUrl:Proposed fix
- const returnUrl = encodeURIComponent( - pathname + window.location.search - ); + const returnUrl = encodeURIComponent(pathname);If preserving query params is intentional (e.g., to restore filters/state), at minimum document that assumption so future developers are aware.
frontend/src/components/pages/LoginPage.tsx (1)
68-88:returnUrlvalidation is solid but the regex character whitelist may reject legitimate internal paths.The open-redirect mitigations look good — blocking
//,\\,..,%, and applying a character whitelist is a strong defense-in-depth approach.However, the regex on line 80 (
/^[\/a-zA-Z0-9\-_?=&]+$/) excludes several characters that commonly appear in legitimate URLs:
.(dots in path segments, e.g./api/v1.0/resource)#(hash fragments)+and other characters often found in query valuesThis means some valid
returnUrlvalues will silently fall back to/. This is safe but may surprise users who lose their intended destination. Consider expanding the whitelist to include.:Proposed fix
- /^[\/a-zA-Z0-9\-_?=&]+$/.test(decoded)) { + /^[\/a-zA-Z0-9\-_.?=&#+]+$/.test(decoded)) {
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
New Feature