fixes #125, address remaining review issues from PR #124 - #180
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughSuppresses console error noise during tests, updates dashboard mail-template controllers to forward 500s via Express ChangesTest environment logging and mail template enhancements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/public-api/src/controllers/data.controller.js (1)
87-99:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop leaking raw internal errors from controller catch paths
Several touched catch blocks still send raw
err.message/detailsto clients (e.g., Line 94, Line 98, Line 370, Line 444, Line 509, Line 513, Line 575), which leaks internal DB/runtime details and breaks the controller response contract. Return a sanitized message and keep detailed error info server-side only.Suggested pattern
- return res.status(500).json({ error: err.message }); + return next(new AppError("Internal server error", 500));- return res.status(409).json({ - error: "Duplicate value violates unique constraint.", - details: err.message, - }); + return next(new AppError("Duplicate value violates unique constraint.", 409));- return res.status(500).json({ - success: false, - data: {}, - message: err.message || "Failed to execute aggregation.", - }); + return next(new AppError("Failed to execute aggregation.", 500));As per coding guidelines, "
**/src/controllers/**/*.{js,ts}: All API endpoints return:{ success: bool, data: {}, message: "" }. Use AppError class for errors — never raw throw, never expose MongoDB errors to client."Also applies to: 178-189, 367-371, 429-446, 502-514, 572-576
🤖 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 `@apps/public-api/src/controllers/data.controller.js` around lines 87 - 99, The controller currently returns raw internal error messages (e.g., using err.message and details in the catch path around isDuplicateKeyError) which leaks DB/runtime internals and violates the response contract; update the catch blocks in apps/public-api/src/controllers/data.controller.js to stop sending err.message or err details to clients, instead log the full error server-side (console.error or logger) and return a sanitized response shaped as { success: false, data: {}, message: "<user-friendly message>" } using the AppError pattern; for duplicate-key errors detected by isDuplicateKeyError(err) return a 409 with a generic message like "Conflict: duplicate value" (no DB details), for other errors return a 500 with a generic message like "Internal server error", and ensure any thrown errors in this controller use the AppError class rather than raw error objects.
🤖 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 `@apps/dashboard-api/src/controllers/project.controller.js`:
- Around line 1598-1601: Several catch blocks in project.controller.js currently
return res.status(500).json({ error, details: ... }) and leak backend error
text; update each of these catch blocks (the ones that call
res.status(500).json(...) for 500 responses) to follow the controller response
contract by returning res.status(500).json({ success: false, data: {}, message:
"Internal server error" }) and remove any use of err.message or a details field
so no MongoDB/backend error text is exposed to clients; apply the same change to
all six occurrences that currently return the { error, details } payload.
In `@apps/public-api/src/__tests__/mail.controller.test.js`:
- Around line 176-231: The test currently only asserts output while mocking
MailTemplate.findOne loosely, which can hide regressions in lookup order/scope;
after invoking mailController.sendMail add assertions on MailTemplate.findOne
calls to ensure the first call queried the project-scoped template and the
second called the global fallback (e.g.,
expect(MailTemplate.findOne).toHaveBeenNthCalledWith(1,
expect.objectContaining({ name: 'welcome', projectId: 'proj_1' })) and
expect(MailTemplate.findOne).toHaveBeenNthCalledWith(2,
expect.objectContaining({ name: 'welcome', projectId: null })) so the test fails
if the controller no longer queries project-first then global).
---
Outside diff comments:
In `@apps/public-api/src/controllers/data.controller.js`:
- Around line 87-99: The controller currently returns raw internal error
messages (e.g., using err.message and details in the catch path around
isDuplicateKeyError) which leaks DB/runtime internals and violates the response
contract; update the catch blocks in
apps/public-api/src/controllers/data.controller.js to stop sending err.message
or err details to clients, instead log the full error server-side (console.error
or logger) and return a sanitized response shaped as { success: false, data: {},
message: "<user-friendly message>" } using the AppError pattern; for
duplicate-key errors detected by isDuplicateKeyError(err) return a 409 with a
generic message like "Conflict: duplicate value" (no DB details), for other
errors return a 500 with a generic message like "Internal server error", and
ensure any thrown errors in this controller use the AppError class rather than
raw error objects.
🪄 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: 0682ec95-9843-4f9a-b91e-96cdf8e88a6d
📒 Files selected for processing (6)
apps/dashboard-api/src/controllers/project.controller.jsapps/dashboard-api/src/middlewares/authMiddleware.jsapps/public-api/src/__tests__/mail.controller.test.jsapps/public-api/src/controllers/data.controller.jspackages/common/src/queues/publicEmailQueue.jspackages/common/src/utils/input.validation.js
|
Hello @tirth1356 |
|
@tirth1356 |
|
yes im there, sorry for late reply i have done the changes and will push it soon @yash-pouranik |
b81c8ca to
3b78063
Compare
|
done @yash-pouranik, all conflicts have been resolved and the above given review is also considered |
|
@coderabbitai |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 145306
✅ No merge conflict markers foundZero residual ✅ No code loss detected — all changes are intact
🟡 One minor observation (non-blocking)
Overall: the PR looks good from a code-integrity standpoint and is safe to merge after your manual review. 🐇 |
2 participants
fixed #125
This PR addresses the follow-up tasks from PR #124, improving error sanitization, increasing test coverage, resolving SDK type mismatches, and cleaning up test execution noise.
Changes Summary:
apps/dashboard-api/src/controllers/project.controller.jswith a generic"Internal server error"message, only exposing details indevelopmentmode.apps/public-api/src/__tests__/mail.controller.test.jsto verify:sendMailSchemaZod definition for thetofield inpackages/common/src/utils/input.validation.jsto natively accept both a single email string and an array of email strings (z.union([z.string().email(), z.array(z.string().email())])), matching the SDK's definitions.apps/dashboard-api/src/middlewares/authMiddleware.jsapps/public-api/src/controllers/data.controller.jspackages/common/src/queues/publicEmailQueue.js🛠️ Type of Change
🧪 Testing & Validation
Backend Verification:
npm testor the local Jest runner inside the monorepo directories and all tests passed cleanly.✅ Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests