Standarized the API Response in Dashboard API-Controller#304
Open
Ayush4958 wants to merge 4 commits into
Open
Conversation
Ayush4958
requested review from
Nitin-kumar-yadav1307 and
yash-pouranik
as code owners
Contributor
|
Warning Review limit reached
More reviews will be available in 34 minutes and 48 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ 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 (6)
📝 WalkthroughWalkthroughControllers and tests in apps/dashboard-api were refactored to propagate errors via next(new AppError(...)), success responses use ApiResponse(...).send(res), and the global Express error handler was updated to recognize operational AppError instances. ChangesCentralized Error Handling Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
apps/dashboard-api/src/controllers/auth.controller.js (1)
237-249:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the success payloads on the standardized envelope.
This refactor standardized many failure paths, but these success responses still emit mixed shapes like
{ user: ... },{ message: ... }, and{ success: true, user: ... }instead of the required{ success, data, message }. That leaves the dashboard API contract inconsistent even thoughApiResponseis already available in this controller. As per coding guidelines,All API endpoints must return response format: { success: bool, data: {}, message: "" }.Also applies to: 311-311, 420-420, 440-440, 469-469, 509-514, 539-553, 584-584, 619-619
🤖 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/auth.controller.js` around lines 237 - 249, The success responses (e.g., sendTokenResponse) currently return inconsistent shapes; change them to the standardized envelope { success, data, message } by wrapping the existing payload under data (e.g., data: { user: { ... } }) and providing an appropriate message (or empty string) and the success boolean; use the existing ApiResponse helper if available to build the response. Update sendTokenResponse and the other listed success-return sites (lines referenced: 311, 420, 440, 469, 509-514, 539-553, 584, 619) to return res.status(statusCode).json({ success: true, data: <payload>, message: "<optional message>" }) instead of the current mixed shapes so all endpoints conform to the { success, data, message } contract.Source: Coding guidelines
apps/dashboard-api/src/controllers/userAuth.controller.js (3)
168-186:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope the
401remap tojwt.verifyonly.The user lookup and model resolution run inside the same inner
try, so a schema/DB failure is currently reported as “Invalid or Expired Token”. That masks operational errors and bypasses the centralized 5xx flow.🤖 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/userAuth.controller.js` around lines 168 - 186, The current try-catch wraps jwt.verify and the DB lookup (getConnection, getCompiledModel, Model.findOne), causing DB/schema failures to be remapped to a 401; separate the JWT verification from the DB work: call jwt.verify inside its own try/catch and only convert JsonWebTokenError/TokenExpiredError into next(new AppError(401, "Invalid or Expired Token")), then proceed to call getConnection, getCompiledModel and Model.findOne outside that JWT-specific catch so any errors from those (or other unexpected errors) bubble to the normal error path (i.e., call next(err) or let the outer catch produce a 5xx AppError) instead of returning 401.
360-360:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
getAuthCollectionis undefined in this module.There’s no import or local definition for
getAuthCollection, so both handlers will throwReferenceErrorbefore reaching MongoDB. Reuse the existinggetConnection(...)/getCompiledModel(...)path or import the helper explicitly.Also applies to: 399-399
🤖 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/userAuth.controller.js` at line 360, getAuthCollection is referenced but not defined or imported, causing a ReferenceError; update the handlers to either import the helper that exports getAuthCollection or replace its usage with the existing connection path (use getConnection(...) to obtain the MongoDB client/DB and then getCompiledModel(...) or directly access the collection) so the code uses defined symbols (getAuthCollection OR getConnection and getCompiledModel) and removes the undefined reference in the handler code paths.
112-116:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThese success paths still break the controller response contract.
They return raw users, tokens, sessions, or message-only payloads instead of
{ success, data, message }, so the migration only standardized failures. Clients still have to branch on endpoint-specific shapes. As per coding guidelines,All API endpoints must return response format: { success: bool, data: {}, message: "" }.Also applies to: 150-150, 183-183, 226-229, 269-269, 307-307, 329-337, 370-370, 406-406, 451-451, 477-477, 512-512, 539-539, 563-563
🤖 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/userAuth.controller.js` around lines 112 - 116, Update all success responses in userAuth.controller.js to conform to the unified response contract { success: bool, data: {}, message: "" }; for example replace the current res.status(201).json({ message: "...", token, userId }) with res.status(201).json({ success: true, data: { token, userId }, message: "User registered successfully. Please verify your email." }); apply the same pattern to every success path listed in the comment (lines ~112,150,183,226-229,269,307,329-337,370,406,451,477,512,539,563), preserving existing HTTP status codes and original message text, and for responses that previously returned only a message set data: {} while for those returning user/session/token objects wrap them inside data under descriptive keys (e.g., data: { user }, data: { session }) so clients always receive { success, data, message }.Source: Coding guidelines
apps/dashboard-api/src/controllers/webhook.controller.js (1)
59-70:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWebhook success responses are still ad hoc.
These handlers now standardize failures, but their success bodies still vary between
{ data },{ message }, and custom top-level fields. That violates the controller contract and keeps webhook clients on endpoint-specific parsing rules. As per coding guidelines,All API endpoints must return response format: { success: bool, data: {}, message: "" }.Also applies to: 110-110, 145-156, 211-223, 261-261, 307-315, 415-421
🤖 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/webhook.controller.js` around lines 59 - 70, The success responses in webhook.controller.js are nonstandard; update the res.status(...).json responses (e.g., the create response using webhook and other handlers that currently return `{ message }`, `{ data }`, or custom top-level fields) to the canonical format { success: true, data: { ... }, message: "..." } — for the create handler use { success: true, data: { _id: webhook._id, projectId: webhook.projectId, name: webhook.name, url: webhook.url, events: Object.fromEntries(webhook.events || new Map()), enabled: webhook.enabled, createdAt: webhook.createdAt }, message: "Webhook created" } and apply the same pattern to the corresponding update/get/list/delete/enable/disable responses that call res.status(...).json so every successful response from this controller follows { success, data, message }.Source: Coding guidelines
apps/dashboard-api/src/controllers/billing.controller.js (2)
202-244:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWebhook success branches still break the standardized response shape.
These returns omit
data, while the controller contract for this repo requires{ success, data, message }. Since this PR is standardizing controller responses, the webhook path should not keep the old shape.Suggested fix
- return res.json({ success: true, message: 'No subscription payload. Skipped.' }); + return res.json({ success: true, data: {}, message: 'No subscription payload. Skipped.' }); - return res.json({ success: true, message: 'No valid developer_id in notes. Skipped.' }); + return res.json({ success: true, data: {}, message: 'No valid developer_id in notes. Skipped.' }); - return res.json({ success: true, message: 'Developer not found. Skipped.' }); + return res.json({ success: true, data: {}, message: 'Developer not found. Skipped.' }); - res.json({ success: true, message: 'Webhook processed.' }); + res.json({ success: true, data: {}, message: 'Webhook processed.' }); - res.json({ success: true, message: 'Internal error. Logged.' }); + res.json({ success: true, data: {}, message: 'Internal error. Logged.' });As per coding guidelines,
{apps/public-api,apps/dashboard-api}/src/controllers/**/*.js: All API endpoints must return response format:{ success: bool, data: {}, message: "" }.🤖 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/billing.controller.js` around lines 202 - 244, The webhook handler in billing.controller.js returns non-standard responses (missing data) in several early-return and final res.json calls; update every res.json in this function (including the early returns after payload/developer checks, the success "Webhook processed." response, and the catch error response) to follow the contract { success: bool, data: {}, message: string } — e.g., keep the same success/message semantics but add data: {} (or contextual data when appropriate), ensuring all branches (payload missing, invalid developer_id, developer not found, processed, and error) return the standardized shape; locate these calls by searching for res.json within the Razorpay webhook handler that references payload, developerId, developer, and developer.save().Source: Coding guidelines
183-195:⚠️ Potential issue | 🟠 MajorGuard Razorpay webhook signature comparison and standardize webhook response shape
crypto.timingSafeEqual(Buffer.from(signature), Buffer.from(expectedSignature))throws when buffer lengths differ; thecatchhandler then returns200 { success: true, ... }, so malformedx-razorpay-signaturegets acknowledged instead of rejected with 401.- Webhook early-return/success/catch responses omit the required
data: {}field (e.g.,res.json({ success: true, message: ... })).Suggested fix
const signature = req.headers['x-razorpay-signature']; if (!signature) { return next(new AppError(401, 'Missing webhook signature.')); } const rawBody = req.rawBody || JSON.stringify(req.body); const expectedSignature = crypto .createHmac('sha256', webhookSecret) .update(rawBody) .digest('hex'); - if (!crypto.timingSafeEqual(Buffer.from(signature), Buffer.from(expectedSignature))) { + const normalizedSignature = typeof signature === 'string' ? signature.trim() : ''; + const isInvalidSignature = + normalizedSignature.length !== expectedSignature.length || + !/^[a-f0-9]+$/i.test(normalizedSignature) || + !crypto.timingSafeEqual( + Buffer.from(normalizedSignature, 'utf8'), + Buffer.from(expectedSignature, 'utf8'), + ); + + if (isInvalidSignature) { return next(new AppError(401, 'Invalid webhook signature.')); }🤖 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/billing.controller.js` around lines 183 - 195, The webhook handler currently uses crypto.timingSafeEqual on Buffer.from(signature) and Buffer.from(expectedSignature) which throws if lengths differ, causing malformed signatures to be acknowledged; update the comparison in the billing webhook code (the block using signature, rawBody, expectedSignature and crypto.timingSafeEqual) to first check Buffer.byteLength(signature) === Buffer.byteLength(expectedSignature) (or compare Buffer.from(signature).length === Buffer.from(expectedSignature).length) and only call timingSafeEqual when lengths match, otherwise return next(new AppError(401, 'Invalid webhook signature.')); also standardize all webhook JSON responses in this controller (success, error, and catch paths) to include the required data: {} field (e.g., res.json({ success: true, message: ..., data: {} }) or the error responses likewise) so every early-return and catch path returns the same shape.apps/dashboard-api/src/controllers/analytics.controller.js (1)
12-20:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReuse one owner matcher across these analytics queries.
getGlobalStatsexplicitly compensates for projects whoseowneris stored as either the raw JWT value or anObjectId, but the follow-upProject.find(...)/distinct(...)calls switch back to a single form. For accounts with legacy owner values,totalProjectscan include projects thattotalRequests,totalWebhooks,totalUsers, recent activity, and north-star metrics then miss.Suggested fix
const user_id = req.user._id; const userId = new mongoose.Types.ObjectId(user_id); + const ownerMatch = { + $or: [ + { owner: user_id }, + { owner: userId } + ] + }; const [stats, dev] = await Promise.all([ Project.aggregate([ - { - $match: { - $or: [ - { owner: user_id }, - { owner: userId } - ] - } - }, + { $match: ownerMatch }, { $group: { _id: null, @@ - const projects = await Project.find({ owner: user_id }).select("_id").lean(); + const projects = await Project.find(ownerMatch).select("_id").lean();Apply the same
ownerMatchto theProject.find(...).distinct("_id")call ingetRecentActivityand theProject.find(...)call ingetNorthStar.Also applies to: 42-46, 93-93, 275-276
🤖 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/analytics.controller.js` around lines 12 - 20, The aggregation in getGlobalStats defines an ownerMatch that covers both string and ObjectId JWT owner values, but subsequent queries (specifically the Project.find(...).distinct("_id") in getRecentActivity and the Project.find(...) in getNorthStar) revert to a single-form owner filter; update those calls to reuse the same ownerMatch object (the one used in getGlobalStats) instead of hardcoding owner/userId so all analytics use the identical matcher for owner values. Locate ownerMatch in getGlobalStats and pass or reference it in the Project.find(...).distinct("_id") call in getRecentActivity and the Project.find(...) call in getNorthStar (and the similar spots noted around lines 42-46, 93, 275-276) so legacy ObjectId/string owners are consistently handled.apps/dashboard-api/src/controllers/admin.metrics.controller.js (1)
115-120:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject impossible month values.
Line 115 only validates the
YYYY-MMshape, so inputs like2026-00and2026-13still pass andDate.UTCsilently rolls them into a different month. Reject anything outside01-12before building the cohort bounds.Suggested fix
const { month } = req.query; // e.g. "2026-05" if (!month || !/^\d{4}-\d{2}$/.test(month)) { return next(new AppError(400, 'Provide month as YYYY-MM (e.g. 2026-05)')); } const [year, mo] = month.split('-').map(Number); + if (mo < 1 || mo > 12) { + return next(new AppError(400, 'Provide month as YYYY-MM (e.g. 2026-05)')); + } const cohortStart = new Date(Date.UTC(year, mo - 1, 1)); const cohortEnd = new Date(Date.UTC(year, mo, 1));🤖 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/admin.metrics.controller.js` around lines 115 - 120, The regex only checks format but allows impossible months like "00" or "13"; before creating cohortStart use the parsed values from month.split('-').map(Number) (year and mo) and explicitly validate mo is an integer between 1 and 12 (inclusive); if not, call next(new AppError(400, 'Provide month as YYYY-MM (e.g. 2026-05)')) to reject it, then continue to construct cohortStart = new Date(Date.UTC(year, mo - 1, 1)).
🤖 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/app.js`:
- Around line 159-163: The 500 response currently includes err.message which can
leak internals; update the error handler around the res.status(500).json call to
return a generic message (e.g. "Internal server error" or "Something went
wrong") instead of err.message, and ensure the actual error is logged internally
(using your existing logger or console.error(err)) within the same
error-handling middleware so debugging info is retained server-side but not
exposed to clients.
In `@apps/dashboard-api/src/controllers/admin.metrics.controller.js`:
- Around line 50-52: In the catch block that currently calls next(err) in
admin.metrics.controller.js, import the AppError class and ensure all
non-AppError exceptions are wrapped before forwarding: replace next(err) with
logic like "if (!(err instanceof AppError)) err = new AppError('Internal server
error', 500);" then call next(err); this normalizes Mongoose/Mongo/runtime
errors to AppError so the global handler never exposes raw backend messages
while preserving existing AppError propagation.
In `@apps/dashboard-api/src/controllers/auth.controller.js`:
- Around line 409-412: Check for a missing Developer record before calling
bcrypt.compare: after calling
Developer.findById(req.user._id).select('+password') (the variable dev), if dev
is null return next(new AppError(404, "User not found")) (or an appropriate
AppError) instead of calling bcrypt.compare on dev.password; apply the same null
check and AppError return for the other occurrence noted around the 432-435
block so no raw errors or null dereferences reach the client.
- Around line 607-608: The catch block currently maps every error to a 403
AppError, hiding DB and other failures; change the catch to inspect the thrown
error and only convert JWT verification errors to a 403. Specifically, in the
catch surrounding Developer.findById(...) and sendTokenResponse(...), check
error.name (e.g., 'TokenExpiredError' or 'JsonWebTokenError') or other
JWT-specific indicators and call next(new AppError(403, "Invalid or expired
refresh token")) only for those cases, otherwise call next(err) to let
centralized 5xx handling handle DB/save or other operational errors.
In `@apps/dashboard-api/src/controllers/events.controller.js`:
- Around line 45-46: The catch block currently does next(err) which forwards raw
exceptions to the 500 handler; instead import/use the AppError class and wrap
unexpected errors before calling next: create a new AppError (e.g., new
AppError('Internal server error', 500)) or use AppError.withCause if available,
pass the original err as the cause or log it, then call next(wrappedAppError);
update the catch in the events controller (the catch handling err and next) to
perform this wrapping so Mongo/backend details are not sent to clients.
In `@apps/dashboard-api/src/controllers/release.controller.js`:
- Around line 69-73: The success responses in exports.getAllReleases (and the
other handlers returning raw JSON later in this file) must follow the standard
envelope; change the res.json(releases) success path to res.json({ success:
true, data: releases, message: "" }) and update the other success-returning
handlers (the ones around the later success return at lines 114-117) to the same
shape (success: boolean, data: <payload>, message: string) so all controller
endpoints in this file conform to the { success, data, message } contract.
- Around line 73-75: Replace direct forwarding of raw errors to next(err) in the
release controller catch blocks with normalization: if err is already an
instance of AppError call next(err), otherwise log the original error and
forward a new AppError (e.g., generic 500/Internal Server Error) so no
Mongo/driver messages are sent to clients; update both catch sites (the two
catch blocks in this controller) to perform this check and use AppError before
calling next.
In `@apps/dashboard-api/src/controllers/webhook.controller.js`:
- Around line 39-42: The validation error message extraction uses the outdated
Zod v3 property error.errors; update both validation branches to use Zod v4's
error.issues instead: replace validation.error.errors?.[0]?.message with
validation.error.issues?.[0]?.message in the createWebhook validation block
(where createWebhookSchema.safeParse is used) and likewise in the updateWebhook
validation branch (the other safeParse usage around lines ~183-186) so the
controller pulls the correct message from Zod's ZodError.
---
Outside diff comments:
In `@apps/dashboard-api/src/controllers/admin.metrics.controller.js`:
- Around line 115-120: The regex only checks format but allows impossible months
like "00" or "13"; before creating cohortStart use the parsed values from
month.split('-').map(Number) (year and mo) and explicitly validate mo is an
integer between 1 and 12 (inclusive); if not, call next(new AppError(400,
'Provide month as YYYY-MM (e.g. 2026-05)')) to reject it, then continue to
construct cohortStart = new Date(Date.UTC(year, mo - 1, 1)).
In `@apps/dashboard-api/src/controllers/analytics.controller.js`:
- Around line 12-20: The aggregation in getGlobalStats defines an ownerMatch
that covers both string and ObjectId JWT owner values, but subsequent queries
(specifically the Project.find(...).distinct("_id") in getRecentActivity and the
Project.find(...) in getNorthStar) revert to a single-form owner filter; update
those calls to reuse the same ownerMatch object (the one used in getGlobalStats)
instead of hardcoding owner/userId so all analytics use the identical matcher
for owner values. Locate ownerMatch in getGlobalStats and pass or reference it
in the Project.find(...).distinct("_id") call in getRecentActivity and the
Project.find(...) call in getNorthStar (and the similar spots noted around lines
42-46, 93, 275-276) so legacy ObjectId/string owners are consistently handled.
In `@apps/dashboard-api/src/controllers/auth.controller.js`:
- Around line 237-249: The success responses (e.g., sendTokenResponse) currently
return inconsistent shapes; change them to the standardized envelope { success,
data, message } by wrapping the existing payload under data (e.g., data: { user:
{ ... } }) and providing an appropriate message (or empty string) and the
success boolean; use the existing ApiResponse helper if available to build the
response. Update sendTokenResponse and the other listed success-return sites
(lines referenced: 311, 420, 440, 469, 509-514, 539-553, 584, 619) to return
res.status(statusCode).json({ success: true, data: <payload>, message:
"<optional message>" }) instead of the current mixed shapes so all endpoints
conform to the { success, data, message } contract.
In `@apps/dashboard-api/src/controllers/billing.controller.js`:
- Around line 202-244: The webhook handler in billing.controller.js returns
non-standard responses (missing data) in several early-return and final res.json
calls; update every res.json in this function (including the early returns after
payload/developer checks, the success "Webhook processed." response, and the
catch error response) to follow the contract { success: bool, data: {}, message:
string } — e.g., keep the same success/message semantics but add data: {} (or
contextual data when appropriate), ensuring all branches (payload missing,
invalid developer_id, developer not found, processed, and error) return the
standardized shape; locate these calls by searching for res.json within the
Razorpay webhook handler that references payload, developerId, developer, and
developer.save().
- Around line 183-195: The webhook handler currently uses crypto.timingSafeEqual
on Buffer.from(signature) and Buffer.from(expectedSignature) which throws if
lengths differ, causing malformed signatures to be acknowledged; update the
comparison in the billing webhook code (the block using signature, rawBody,
expectedSignature and crypto.timingSafeEqual) to first check
Buffer.byteLength(signature) === Buffer.byteLength(expectedSignature) (or
compare Buffer.from(signature).length === Buffer.from(expectedSignature).length)
and only call timingSafeEqual when lengths match, otherwise return next(new
AppError(401, 'Invalid webhook signature.')); also standardize all webhook JSON
responses in this controller (success, error, and catch paths) to include the
required data: {} field (e.g., res.json({ success: true, message: ..., data: {}
}) or the error responses likewise) so every early-return and catch path returns
the same shape.
In `@apps/dashboard-api/src/controllers/userAuth.controller.js`:
- Around line 168-186: The current try-catch wraps jwt.verify and the DB lookup
(getConnection, getCompiledModel, Model.findOne), causing DB/schema failures to
be remapped to a 401; separate the JWT verification from the DB work: call
jwt.verify inside its own try/catch and only convert
JsonWebTokenError/TokenExpiredError into next(new AppError(401, "Invalid or
Expired Token")), then proceed to call getConnection, getCompiledModel and
Model.findOne outside that JWT-specific catch so any errors from those (or other
unexpected errors) bubble to the normal error path (i.e., call next(err) or let
the outer catch produce a 5xx AppError) instead of returning 401.
- Line 360: getAuthCollection is referenced but not defined or imported, causing
a ReferenceError; update the handlers to either import the helper that exports
getAuthCollection or replace its usage with the existing connection path (use
getConnection(...) to obtain the MongoDB client/DB and then
getCompiledModel(...) or directly access the collection) so the code uses
defined symbols (getAuthCollection OR getConnection and getCompiledModel) and
removes the undefined reference in the handler code paths.
- Around line 112-116: Update all success responses in userAuth.controller.js to
conform to the unified response contract { success: bool, data: {}, message: ""
}; for example replace the current res.status(201).json({ message: "...", token,
userId }) with res.status(201).json({ success: true, data: { token, userId },
message: "User registered successfully. Please verify your email." }); apply the
same pattern to every success path listed in the comment (lines
~112,150,183,226-229,269,307,329-337,370,406,451,477,512,539,563), preserving
existing HTTP status codes and original message text, and for responses that
previously returned only a message set data: {} while for those returning
user/session/token objects wrap them inside data under descriptive keys (e.g.,
data: { user }, data: { session }) so clients always receive { success, data,
message }.
In `@apps/dashboard-api/src/controllers/webhook.controller.js`:
- Around line 59-70: The success responses in webhook.controller.js are
nonstandard; update the res.status(...).json responses (e.g., the create
response using webhook and other handlers that currently return `{ message }`,
`{ data }`, or custom top-level fields) to the canonical format { success: true,
data: { ... }, message: "..." } — for the create handler use { success: true,
data: { _id: webhook._id, projectId: webhook.projectId, name: webhook.name, url:
webhook.url, events: Object.fromEntries(webhook.events || new Map()), enabled:
webhook.enabled, createdAt: webhook.createdAt }, message: "Webhook created" }
and apply the same pattern to the corresponding
update/get/list/delete/enable/disable responses that call res.status(...).json
so every successful response from this controller follows { success, data,
message }.
🪄 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: 4f6b699d-2161-41c2-a51f-2324c7980fc8
📒 Files selected for processing (13)
apps/dashboard-api/src/__tests__/auth.controller.test.jsapps/dashboard-api/src/__tests__/billing.controller.test.jsapps/dashboard-api/src/__tests__/userAuth.controller.test.jsapps/dashboard-api/src/__tests__/webhook.controller.test.jsapps/dashboard-api/src/app.jsapps/dashboard-api/src/controllers/admin.metrics.controller.jsapps/dashboard-api/src/controllers/analytics.controller.jsapps/dashboard-api/src/controllers/auth.controller.jsapps/dashboard-api/src/controllers/billing.controller.jsapps/dashboard-api/src/controllers/events.controller.jsapps/dashboard-api/src/controllers/release.controller.jsapps/dashboard-api/src/controllers/userAuth.controller.jsapps/dashboard-api/src/controllers/webhook.controller.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/dashboard-api/src/controllers/release.controller.js (1)
72-72:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWrap list results in an object to keep the response contract stable.
Line 72 returns
data: releases(array), while the controller contract specifiesdataas an object-shaped envelope. Preferdata: { releases }for consistency across endpoints and clients.
As per coding guidelines,{apps/public-api,apps/dashboard-api}/src/controllers/**/*.js: All API endpoints must return response format:{ success: bool, data: {}, message: "" }.🤖 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/release.controller.js` at line 72, The response currently sends data: releases (an array) which violates the API contract expecting data to be an object envelope; change the res.json call in the release controller (the place that invokes res.json with success:true and data: releases) to wrap the list as an object, e.g. data: { releases }, so the response shape becomes { success: bool, data: {}, message: "" } and remains consistent with other endpoints.Source: Coding guidelines
🤖 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/events.controller.js`:
- Around line 46-48: The catch block currently reassigns the caught parameter
`err` (e.g., `if (!(err instanceof AppError)) err = new AppError(...)`);
instead, create a new constant like `forwardedError` (or similar) that is set to
`err` if it's an AppError or to a new `AppError(500, 'Internal server error')`
otherwise, and pass that `forwardedError` to `next(forwardedError)` without
mutating the original `err` parameter; update the error-normalization logic in
the controller (the catch block that references `err` and `AppError`)
accordingly.
---
Duplicate comments:
In `@apps/dashboard-api/src/controllers/release.controller.js`:
- Line 72: The response currently sends data: releases (an array) which violates
the API contract expecting data to be an object envelope; change the res.json
call in the release controller (the place that invokes res.json with
success:true and data: releases) to wrap the list as an object, e.g. data: {
releases }, so the response shape becomes { success: bool, data: {}, message: ""
} and remains consistent with other endpoints.
🪄 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: 29a4b37f-e4c5-4f11-8cb6-0529e738d362
📒 Files selected for processing (3)
apps/dashboard-api/src/controllers/events.controller.jsapps/dashboard-api/src/controllers/release.controller.jsapps/dashboard-api/src/controllers/webhook.controller.js
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/dashboard-api/src/controllers/webhook.controller.js
Contributor
Author
|
@yash-pouranik , |
Collaborator
|
yh lets see |
yash-pouranik
approved these changes
Jun 12, 2026
Collaborator
|
Everything seems good to me |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check the global error handler in app.js
cat apps/dashboard-api/src/app.js
# Check the AppError and ApiResponse utilities from common
fd -t f "AppError\|ApiResponse\|index" --full-path apps/dashboard-api/node_modules/@urbackend/common/src 2>/dev/null | head -20
fd -t f "index.js" --full-path apps/dashboard-api/node_modules/@urbackend/common 2>/dev/null | head -5
echo "=== Common package exports ==="
cat packages/common/src/index.js 2>/dev/null || cat packages/common/index.js 2>/dev/null || fd -t f "index" --full-path packages/common | head -5 | xargs cat 2>/dev/null
echo "=== AppError ==="
fd -t f "AppError" | head -5 | xargs cat 2>/dev/null
echo "=== ApiResponse ==="
fd -t f "ApiResponse" | head -5 | xargs cat 2>/dev/nullLength of output: 18530 🏁 Script executed: #!/bin/bash
echo "=== auth.controller.js ==="
cat apps/dashboard-api/src/controllers/auth.controller.js
echo "=== events.controller.js ==="
cat apps/dashboard-api/src/controllers/events.controller.js
echo "=== release.controller.js ==="
cat apps/dashboard-api/src/controllers/release.controller.jsLength of output: 27447 🏁 Script executed: #!/bin/bash
echo "=== webhook.controller.js ==="
cat apps/dashboard-api/src/controllers/webhook.controller.js
echo "=== billing.controller.js ==="
cat apps/dashboard-api/src/controllers/billing.controller.js
echo "=== admin.metrics.controller.js ==="
cat apps/dashboard-api/src/controllers/admin.metrics.controller.jsLength of output: 33247 🏁 Script executed: #!/bin/bash
echo "=== analytics.controller.js ==="
cat apps/dashboard-api/src/controllers/analytics.controller.js
echo "=== userAuth.controller.js ==="
cat apps/dashboard-api/src/controllers/userAuth.controller.jsLength of output: 30362 🏁 Script executed: #!/bin/bash
echo "=== Test: auth.controller.test.js ==="
cat apps/dashboard-api/src/__tests__/auth.controller.test.js
echo "=== Test: billing.controller.test.js ==="
cat apps/dashboard-api/src/__tests__/billing.controller.test.js
echo "=== Test: webhook.controller.test.js ==="
cat apps/dashboard-api/src/__tests__/webhook.controller.test.js
echo "=== Test: userAuth.controller.test.js ==="
cat apps/dashboard-api/src/__tests__/userAuth.controller.test.jsLength of output: 60765 🏁 Script executed: #!/bin/bash
# Check for any remaining raw res.status().json() patterns in controllers that should have been migrated
echo "=== Checking for remaining raw error responses in controllers ==="
rg -n 'res\.status\s*\(\s*[45]\d\d\s*\)\s*\.json' apps/dashboard-api/src/controllers/ -A1
echo ""
echo "=== Checking for unhandled console.error (should use next) ==="
rg -n 'console\.error' apps/dashboard-api/src/controllers/
echo ""
echo "=== Checking for controllers NOT accepting next ==="
rg -n 'module\.exports\.\w+ = async \(req, res\)' apps/dashboard-api/src/controllers/
echo ""
echo "=== Checking routes to ensure they still map correctly ==="
fd -t f "route\|router" --full-path apps/dashboard-api/src/routes 2>/dev/null | head -10 | xargs cat 2>/dev/null
fd -t f "\.js$" apps/dashboard-api/src/routes 2>/dev/null | xargs cat 2>/dev/nullLength of output: 47270
✅ What's Done WellThe core standardization goal is achieved across all targeted files:
The global error handler in
|
Collaborator
|
some last checks |
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.
🚀 Pull Request Description
I had added AppError and AppResponse Utility to entire backend's middleware of Dashboard controller which 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.✅ Checklist
Summary by CodeRabbit