fixes #125, address remaining review issues from PR #124 #180
Open
tirth1356 wants to merge 1 commit into
Open
Conversation
…ch, and test console noise
Contributor
📝 WalkthroughWalkthroughThis PR standardizes error responses across mail-template endpoints, suppresses test-run logging noise across multiple services, and extends mail-template functionality to support multi-recipient emails and database-backed template lookup with project/global scoping. ChangesTest environment logging and mail template enhancements
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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}
Contributor
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
Comment on lines
+1598
to
+1601
| return res.status(500).json({ | ||
| error: "Internal server error", | ||
| details: process.env.NODE_ENV === "development" ? err.message : undefined, | ||
| }); |
Contributor
There was a problem hiding this comment.
Use the controller response contract for 500s and stop returning details to clients.
On Line 1598, Line 1636, Line 1702, Line 1770, Line 1859, and Line 1889, the new error payload shape ({ error, details }) breaks this controller contract and can leak backend error text in development responses.
Suggested fix pattern for all six catch blocks
- return res.status(500).json({
- error: "Internal server error",
- details: process.env.NODE_ENV === "development" ? err.message : undefined,
- });
+ return res.status(500).json({
+ success: false,
+ data: {},
+ message: "Internal server error",
+ });As per coding guidelines, **/src/controllers/**/*.{js,ts} must return { success: bool, data: {}, message: "" } and must “never expose MongoDB errors to client.”
Also applies to: 1636-1639, 1702-1705, 1770-1773, 1859-1862, 1889-1892
🤖 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/dashboard-api/src/controllers/project.controller.js` around lines 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.
Comment on lines
+176
to
+231
| MailTemplate.findOne.mockReturnValueOnce({ | ||
| lean: jest.fn().mockResolvedValue({ | ||
| _id: 'tpl_db_1', | ||
| name: 'welcome', | ||
| subject: 'Hello {{name}}', | ||
| text: 'Welcome to project!', | ||
| html: '<p>Welcome to project!</p>', | ||
| projectId: 'proj_1' | ||
| }) | ||
| }); | ||
|
|
||
| decrypt.mockReturnValue(null); | ||
| redis.eval.mockResolvedValue(1); | ||
|
|
||
| await mailController.sendMail(req, res); | ||
|
|
||
| expect(res.status).toHaveBeenCalledWith(200); | ||
| expect(res.json).toHaveBeenCalledWith(expect.objectContaining({ | ||
| success: true, | ||
| data: expect.objectContaining({ | ||
| templateUsed: expect.objectContaining({ name: 'welcome', id: 'tpl_db_1', scope: 'project' }), | ||
| }), | ||
| })); | ||
| }); | ||
|
|
||
| test('renders and sends a global mail template from DB when no project template exists', async () => { | ||
| const req = makeReq(); | ||
| req.body = { | ||
| to: 'user@example.com', | ||
| templateName: 'welcome', | ||
| variables: { name: 'Yash' }, | ||
| }; | ||
| const res = makeRes(); | ||
|
|
||
| mockProjectConfig({ | ||
| _id: 'proj_1', | ||
| resendApiKey: null, | ||
| }); | ||
|
|
||
| // First call: project scope (returns null) | ||
| MailTemplate.findOne.mockReturnValueOnce({ | ||
| lean: jest.fn().mockResolvedValue(null) | ||
| }); | ||
|
|
||
| // Second call: global scope | ||
| MailTemplate.findOne.mockReturnValueOnce({ | ||
| lean: jest.fn().mockResolvedValue({ | ||
| _id: 'tpl_global_1', | ||
| name: 'welcome', | ||
| subject: 'Global Hello {{name}}', | ||
| text: 'Global welcome!', | ||
| html: '<p>Global welcome!</p>', | ||
| projectId: null, | ||
| isSystem: true | ||
| }) | ||
| }); |
Contributor
There was a problem hiding this comment.
Assert MailTemplate.findOne query scope/order to avoid false-positive passes.
These tests currently verify output only. Since findOne is a loose mock, they can still pass if lookup order/filtering regresses (e.g., not actually querying project scope first, then global fallback).
🤖 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/__tests__/mail.controller.test.js` around lines 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).
Collaborator
|
Hello @tirth1356 |
Collaborator
|
@tirth1356 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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