Standarized the API Response in Package-Middleware#270
Conversation
|
Warning Review limit reached
More reviews will be available in 48 minutes and 25 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR standardizes error handling and response schemas across the common package by introducing ChangesStandardized Error and Response Handling
Sequence DiagramsequenceDiagram
participant Middleware as checkAuthEnabled/<br/>loadProjectForAdmin/<br/>verifyEmail
participant ExpressNext as Express next()
participant ErrorHandler as Error Handler<br/>Middleware
participant standardize as standardizeApiResponse
participant Client
Middleware->>ExpressNext: next(new AppError(code, message, error))
ExpressNext->>ErrorHandler: pass AppError instance
ErrorHandler->>standardize: response with body={error, message}
standardize->>standardize: extract errorTitle, errorMessage from body
standardize->>Client: send {success: false, error, message}
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 2
🧹 Nitpick comments (1)
packages/common/src/queues/publicEmailQueue.js (1)
15-29: ⚡ Quick winConsider logging cleanup errors in non-test environments.
The function silently swallows all errors during cleanup, which could make debugging production shutdown issues difficult. While best-effort cleanup is appropriate here, logging errors would help diagnose rare but problematic worker shutdown failures.
📝 Suggested enhancement
} catch (_) { // Best-effort cleanup for test/runtime shutdown paths. + if (process.env.NODE_ENV !== 'test') { + console.error('[Queue] Worker cleanup error:', _); + } } finally {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/common/src/queues/publicEmailQueue.js` around lines 15 - 29, The resetPublicEmailWorker function currently swallows all errors during cleanup; change the catch to capture the thrown error (e.g., catch (err)) and log it in non-test environments so production shutdown failures are visible. Update resetPublicEmailWorker to check process.env.NODE_ENV !== 'test' (or similar env check) and emit the error via your project logger (prefer logger.error if available, otherwise console.error) including context like "resetPublicEmailWorker cleanup error" and the error object; still keep the best-effort behavior and ensure worker is nulled in finally.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/common/src/middleware/loadProjectForAdmin.js`:
- Line 18: In the loadProjectForAdmin middleware replace forwarding raw
err.message into the 500 AppError response: do not pass err.message into new
AppError(500, ...); instead log the full error server-side (e.g.,
logger.error(err) or console.error(err)) and call next(new AppError(500,
"Internal Server Error")) so the client only receives a generic message; update
the callsite where next(new AppError(500, err.message, "Internal Server Error"))
is used to reference AppError with the generic client message and preserve raw
err in server logs.
In `@packages/common/src/middleware/standardizeApiResponse.js`:
- Around line 75-77: The branch that sets errorMessage from body.error must
ensure a string value; update the logic around the body.error check (the branch
that currently sets errorTitle = 'Error' and errorMessage = body.error) to guard
and coerce body.error to a string: if body.error is already a string use it,
else try String(body.error) or JSON.stringify(body.error) with a safe fallback
(e.g., a generic message) so errorMessage always satisfies the response schema;
adjust the code in the same scope where errorTitle/errorMessage are assigned
(the standardize response branch handling body.error and body.message).
---
Nitpick comments:
In `@packages/common/src/queues/publicEmailQueue.js`:
- Around line 15-29: The resetPublicEmailWorker function currently swallows all
errors during cleanup; change the catch to capture the thrown error (e.g., catch
(err)) and log it in non-test environments so production shutdown failures are
visible. Update resetPublicEmailWorker to check process.env.NODE_ENV !== 'test'
(or similar env check) and emit the error via your project logger (prefer
logger.error if available, otherwise console.error) including context like
"resetPublicEmailWorker cleanup error" and the error object; still keep the
best-effort behavior and ensure worker is nulled in finally.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e48cc0d1-d579-4ad4-b414-c9944451fc86
📒 Files selected for processing (9)
packages/common/src/index.jspackages/common/src/middleware/checkAuthEnabled.jspackages/common/src/middleware/loadProjectForAdmin.jspackages/common/src/middleware/standardizeApiResponse.jspackages/common/src/middleware/verifyEmail.jspackages/common/src/queues/publicEmailQueue.jspackages/common/src/utils/ApiResponse.jspackages/common/src/utils/AppError.jspackages/common/src/utils/emailService.js
… standardizeApiResponse
|
@yash-pouranik , |
yash-pouranik
left a comment
yash-pouranik
left a comment
There was a problem hiding this comment.
Thank you for the PR
Everything look perfiect
|
please remember one thing that, UI is ok to handle those New responses. |
2 participants
🚀 Pull Request Description
I had added AppError and AppResponse Utility to entire backend's middleware of
pcakage/commonwhich provide a standard way to operate. Migrated raw JSON response with standard utility.Fixes #237
🛠️ Type of Change
🧪 Testing & Validation
Backend Verification:
npm testin thebackend/directory and all tests passed.Frontend Verification:
npm run lintin thefrontend/directory.📸 Screenshots / Recordings (Optional)
✅ Checklist
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements