fix: bug fixes in backend and frontend integration #155
Conversation
📝 WalkthroughWalkthroughUpdates refine error logging and client lifecycles: backend adds exception tracebacks, enforces queue connectivity, serializes/persity messages, and converts Weaviate client to per-context usage; frontend silences several console.error calls, tightens 401 handling, and adds returnUrl redirect support 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: receive message (loop)
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 and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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.
|
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.
| const rawReturn = searchParams.get('returnUrl'); | ||
| try { | ||
| const decoded = rawReturn ? decodeURIComponent(rawReturn) : null; | ||
| // Protect against open redirects by only allowing internal paths | ||
| if (decoded && decoded.startsWith('/')) { | ||
| navigate(decoded, { replace: true }); | ||
| } else { | ||
| navigate('/', { replace: true }); | ||
| } |
There was a problem hiding this comment.
Open redirect vulnerability: startsWith('/') is insufficient.
The check decoded.startsWith('/') can be bypassed with protocol-relative URLs like //evil.com or backslash variants like /\evil.com, which browsers may interpret as external redirects.
Proposed fix
const rawReturn = searchParams.get('returnUrl');
try {
const decoded = rawReturn ? decodeURIComponent(rawReturn) : null;
// Protect against open redirects by only allowing internal paths
- if (decoded && decoded.startsWith('/')) {
+ if (decoded && decoded.startsWith('/') && !decoded.startsWith('//') && !decoded.startsWith('/\\')) {
navigate(decoded, { replace: true });
} else {
navigate('/', { replace: true });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const rawReturn = searchParams.get('returnUrl'); | |
| try { | |
| const decoded = rawReturn ? decodeURIComponent(rawReturn) : null; | |
| // Protect against open redirects by only allowing internal paths | |
| if (decoded && decoded.startsWith('/')) { | |
| navigate(decoded, { replace: true }); | |
| } else { | |
| navigate('/', { replace: true }); | |
| } | |
| const rawReturn = searchParams.get('returnUrl'); | |
| try { | |
| const decoded = rawReturn ? decodeURIComponent(rawReturn) : null; | |
| // Protect against open redirects by only allowing internal paths | |
| if (decoded && decoded.startsWith('/') && !decoded.startsWith('//') && !decoded.startsWith('/\\')) { | |
| navigate(decoded, { replace: true }); | |
| } else { | |
| navigate('/', { replace: true }); | |
| } |
🤖 Prompt for AI Agents
In frontend/src/components/pages/LoginPage.tsx around lines 69 to 77, the
current check decoded.startsWith('/') permits protocol-relative URLs (e.g.
//evil.com) and backslash variants; update validation to only allow internal
absolute-paths by ensuring the decoded path starts with a single forward slash
not followed by another slash or backslash (e.g. use a regex like ^\/(?![\/\\])
and also reject any input that contains backslashes or a colon), or
alternatively construct new URL(decoded, window.location.origin) and only accept
it if its origin equals window.location.origin and its pathname begins with '/';
then navigate to the validated pathname (or fall back to '/').
| // Prevents multiple simultaneous 401 handlers from racing and triggering | ||
| // sign-out/redirect more than once. | ||
| let isHandlingUnauthorized = false; |
There was a problem hiding this comment.
Race condition guard never resets — breaks subsequent 401 handling.
isHandlingUnauthorized is set to true on Line 110 but never reset to false. After the first 401 triggers a redirect, this flag stays true for the lifetime of the page. If the user logs back in (or if the page is cached/restored), any future 401 responses will be silently ignored, leaving users stuck with broken API calls.
Proposed fix
Since window.location.href navigates away and reloads the page (resetting module state), this works for the happy path. However, if navigation fails or is blocked, the flag stays set. Consider resetting in a finally block or after a timeout as a safety net:
if (!isHandlingUnauthorized) {
isHandlingUnauthorized = true;
try {
await supabase.auth.signOut();
} catch (e) {
// Log but continue to redirect; signing out isn't critical here
console.error(
'Error signing out on 401 handler',
e
);
}
const returnUrl = encodeURIComponent(
pathname + window.location.search
);
window.location.href = `/login?returnUrl=${returnUrl}`;
+ // Reset after a delay in case navigation is blocked (e.g., beforeunload handler)
+ setTimeout(() => {
+ isHandlingUnauthorized = false;
+ }, 2000);
}Also applies to: 109-125
🤖 Prompt for AI Agents
In frontend/src/lib/api.ts around lines 7-9 (and related handling at 109-125),
the module-level guard isHandlingUnauthorized is set true when a 401 is handled
but never reset, causing all subsequent 401s to be ignored; change the handler
so isHandlingUnauthorized is always reset (e.g., clear it in a finally block
after the redirect attempt and/or set a short fallback timeout to reset the flag
if navigation fails) so that the guard prevents concurrent handling but does not
permanently block future 401 handling.
|
Please have a look done the requested changes |
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
✏️ Tip: You can customize this high-level summary in your review settings.