feat: standardize API error and success responses across all controllers - #46
Conversation
|
@Nitin-kumar-yadav1307 is attempting to deploy a commit to the Yash Pouranik's projects Team on Vercel. A member of the Team first needs to authorize it. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the API's consistency and maintainability by standardizing all success and error responses. By introducing dedicated utility functions, it ensures that all endpoints return predictable JSON structures, making client-side integration and error handling much more straightforward. This change also includes several minor bug fixes and cleanup, contributing to a more robust backend. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📝 WalkthroughWalkthroughIntroduces a new response utility module and integrates it across backend controllers (auth, userAuth, data, project, storage), replacing ad-hoc res.json/res.status calls with standardized success/error envelopes; adds a storage controller endpoint Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Important Merge conflicts detected (Beta)
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can customize the high-level summary generated by CodeRabbit.Configure the |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request does an excellent job of standardizing API responses across multiple controllers by introducing and using the errorResponse and successResponse utility functions. This greatly improves code consistency and maintainability. The changes also include several valuable fixes, such as adding a missing await for a database operation, removing a duplicate function call, and patching a security vulnerability in the analytics endpoint. The codebase is significantly cleaner as a result. I have one suggestion to improve logging consistency in one of the controllers.
| if (err instanceof z.ZodError) return res.status(400).json({ error: err.errors }); | ||
| res.status(500).json({ error: err.message }); | ||
| if (err instanceof z.ZodError) return errorResponse(res, 400, err.errors[0]?.message || "Validation failed"); | ||
| return errorResponse(res, 500, err.message); |
There was a problem hiding this comment.
For better observability and debugging, it's a good practice to log server-side errors. Other controllers updated in this PR consistently use console.error(err) in catch blocks before returning a 500 error. It would be beneficial to apply this pattern consistently here and in the other catch blocks throughout this file where it is currently missing.
console.error(err);
return errorResponse(res, 500, err.message);![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/controllers/storage.controller.js (1)
48-51:⚠️ Potential issue | 🔴 CriticalDo not call
.save()onreq.projecthere.
req.projectis sourced from API-key middleware that can provide a lean/cached object, so.save()is not reliable and can crash these paths for internal storage projects.💡 Suggested fix (use atomic Project updates)
- if (!external) { - project.storageUsed += file.size; - await project.save(); - } + if (!external) { + await Project.updateOne( + { _id: project._id }, + { $inc: { storageUsed: file.size } } + ); + } ... - if (!external && fileSize > 0) { - project.storageUsed = Math.max(0, project.storageUsed - fileSize); - await project.save(); - } + if (!external && fileSize > 0) { + await Project.updateOne( + { _id: project._id }, + { $inc: { storageUsed: -fileSize } } + ); + } ... - if (!isExternal(project)) { - project.storageUsed = 0; - await project.save(); - } + if (!isExternal(project)) { + await Project.updateOne( + { _id: project._id }, + { $set: { storageUsed: 0 } } + ); + }Also applies to: 109-112, 158-161
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/controllers/storage.controller.js` around lines 48 - 51, Don't call .save() on req.project because API-key middleware may supply a lean/cached object; instead perform an atomic update on the Project model to increment storageUsed. Replace usages where you mutate project.storageUsed and await project.save() (the blocks around project in storage.controller.js) with a Project.updateOne or Project.findByIdAndUpdate call that increments storageUsed by file.size (use $inc) and conditionally target the project by its _id; ensure the same change is applied for the three occurrences noted (around lines with project.storageUsed updates).backend/controllers/project.controller.js (1)
607-615:⚠️ Potential issue | 🟠 Major
updateProjectresponse may leak sensitive project secrets.Returning the full project document can expose
apiKeyhash andjwtSecret. Other handlers explicitly remove these fields; this one should align.🔒 Suggested response sanitization
- return successResponse(res, 200, { message: "Project updated successfully.", data: project }); + const projectObj = project.toObject(); + delete projectObj.apiKey; + delete projectObj.jwtSecret; + return successResponse(res, 200, { message: "Project updated successfully.", data: projectObj });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/controllers/project.controller.js` around lines 607 - 615, The updateProject handler currently returns the full Project document from Project.findOneAndUpdate which can leak sensitive fields (apiKey, jwtSecret); change the flow to exclude those secrets before returning by either adding a projection/exclusion to Project.findOneAndUpdate (e.g., exclude apiKey and jwtSecret) or converting the returned project to an object and deleting project.apiKey and project.jwtSecret prior to calling successResponse; update references to Project.findOneAndUpdate and the successResponse call in the updateProject handler accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/controllers/auth.controller.js`:
- Around line 45-49: The login flow currently returns distinct messages for
missing users and bad passwords (the checks around the dev variable and
bcrypt.compare in the auth controller), which leaks account existence; change
both branches that call errorResponse(res, 400, ...) — the "if (!dev)" branch
and the "if (!validPass)" branch — to return the same generic error message
(e.g., "Invalid email or password") so both missing-user and bad-password
failures are indistinguishable.
- Line 144: Normalize types before comparing OTPs: when checking existingOtp.otp
!== otp, convert both sides to the same type (e.g., String(existingOtp.otp) ===
String(otp) or Number(...) if you validate numeric) so valid but
differently-typed OTPs aren’t rejected; update the comparison in the auth
controller where existingOtp.otp and otp are compared and keep the same
errorResponse(res, 400, "Incorrect OTP") behavior if they don’t match.
- Line 33: Zod error handlers in auth.controller.js are still accessing the
removed err.errors property; update each Zod check to use
err.issues?.[0]?.message instead (e.g., replace occurrences in the errorResponse
calls inside the functions handling requests such as the auth controller methods
that currently do `if (err instanceof z.ZodError) return errorResponse(res, 400,
err.errors[0]?.message || "Validation failed")`). Ensure every similar branch
(all places that check `err instanceof z.ZodError`) swaps
`err.errors[0]?.message` for `err.issues?.[0]?.message` so validation messages
are correctly read from Zod v4.
In `@backend/controllers/data.controller.js`:
- Around line 68-69: Replace all direct exposures of err.message in the
errorResponse calls (the occurrences that call errorResponse(res, 500,
err.message) — e.g., the return sites shown and the similar calls around the
other noted ranges) with a generic client-facing message like "Internal server
error"; instead, log the full error object server-side using the existing logger
(or console.error) before returning the generic response. Identify each
occurrence by locating calls to errorResponse with a 500 status and the variable
err (e.g., the return statements shown and the similar calls at the other
referenced locations), add a server-side log of err, and change the response
payload to a non-leaking message.
- Around line 140-143: The updateSingleData path is missing Date type validation
causing bad payloads to surface as cast errors; in the updateSingleData handler
add the same Date check used in insertData: when fieldRule.type === 'Date'
validate the incoming value (e.g., ensure it's a valid Date or parsable ISO
string) and call errorResponse(res, 400, `Field '${key}' must be a Date.`) on
failure so Date inputs are rejected cleanly instead of causing DB cast errors.
In `@backend/controllers/project.controller.js`:
- Around line 49-50: Replace all uses of err.message passed directly into
errorResponse (e.g., errorResponse(res, 500, err.message)) with a generic 500
message like "Internal server error" and ensure the original error is logged on
the server (e.g., console.error(err) or use the existing logger). Locate
occurrences in project.controller.js (catch blocks where errorResponse is called
with err.message) and change the response payload while retaining existing error
handling flow and status code.
- Around line 48-49: The Zod error handler in project.controller.js currently
reads err.errors[0]?.message which is incompatible with Zod v4; update the Zod
branches (the instanceof z.ZodError checks that call errorResponse(res, ...)) to
read the first issue message from err.issues instead (e.g.
err.issues[0]?.message || "Validation failed"), and apply the same change to the
other occurrence later in the file (both places that use err.errors[0]). Keep
the instanceof z.ZodError check and existing errorResponse/res usage unchanged.
In `@backend/controllers/storage.controller.js`:
- Around line 124-127: The deleteAllFiles handler is currently protected only by
an API-key and must enforce owner/admin-level authorization; update
module.exports.deleteAllFiles to require an authenticated user and verify
ownership or an explicit admin scope before performing the destructive purge
(e.g., check that req.user exists and either req.user.id === project.ownerId or
req.user.isAdmin/hasScope('admin:files:delete')), otherwise return a 403
errorResponse; mirror the ownership-check pattern used in the project deletion
path in project.controller.js and do not proceed with file deletion unless that
check passes.
In `@backend/controllers/userAuth.controller.js`:
- Line 50: The controller is returning raw internal error strings by calling
errorResponse(res, 500, err.message); instead log the full error (e.g.,
console.error or your app logger) and return a generic client-facing message
such as "Internal server error" or "An unexpected error occurred" instead of
err.message; update each occurrence of errorResponse(res, 500, err.message) in
userAuth.controller.js (the three places shown) to log the error (keeping
err.stack/details) and call errorResponse(res, 500, "Internal server error") so
internal details are not leaked to clients.
- Line 48: In the Zod error handling branches that currently check "err
instanceof z.ZodError" and call errorResponse with err.errors[0]?.message (and
the similar branch later), replace access to the removed errors property with
err.issues?.[0]?.message to be Zod v4 compatible; also replace direct uses of
err.message in those same errorResponse calls with a generic validation/error
message (e.g., "Validation failed" or "Authentication failed") to avoid leaking
internal error details. Locate the two handlers that perform these checks (the
blocks that import/compare z.ZodError and call errorResponse) and update both
the Zod message access and the fallback err.message usage accordingly. Ensure
the rest of the errorResponse signature and status codes remain unchanged.
---
Outside diff comments:
In `@backend/controllers/project.controller.js`:
- Around line 607-615: The updateProject handler currently returns the full
Project document from Project.findOneAndUpdate which can leak sensitive fields
(apiKey, jwtSecret); change the flow to exclude those secrets before returning
by either adding a projection/exclusion to Project.findOneAndUpdate (e.g.,
exclude apiKey and jwtSecret) or converting the returned project to an object
and deleting project.apiKey and project.jwtSecret prior to calling
successResponse; update references to Project.findOneAndUpdate and the
successResponse call in the updateProject handler accordingly.
In `@backend/controllers/storage.controller.js`:
- Around line 48-51: Don't call .save() on req.project because API-key
middleware may supply a lean/cached object; instead perform an atomic update on
the Project model to increment storageUsed. Replace usages where you mutate
project.storageUsed and await project.save() (the blocks around project in
storage.controller.js) with a Project.updateOne or Project.findByIdAndUpdate
call that increments storageUsed by file.size (use $inc) and conditionally
target the project by its _id; ensure the same change is applied for the three
occurrences noted (around lines with project.storageUsed updates).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 829262fe-76bf-4e8b-a6a8-3b044d809a98
📒 Files selected for processing (6)
backend/controllers/auth.controller.jsbackend/controllers/data.controller.jsbackend/controllers/project.controller.jsbackend/controllers/storage.controller.jsbackend/controllers/userAuth.controller.jsbackend/utils/errorResponse.js
| return errorResponse(res, 500, err.message); | ||
| } |
There was a problem hiding this comment.
Prefer generic 500 messages over forwarding err.message.
This file repeatedly returns raw internal error strings. Standardize 500 responses to a generic message and log details server-side.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/controllers/project.controller.js` around lines 49 - 50, Replace all
uses of err.message passed directly into errorResponse (e.g., errorResponse(res,
500, err.message)) with a generic 500 message like "Internal server error" and
ensure the original error is logged on the server (e.g., console.error(err) or
use the existing logger). Locate occurrences in project.controller.js (catch
blocks where errorResponse is called with err.message) and change the response
payload while retaining existing error handling flow and status code.
| module.exports.deleteAllFiles = async (req, res) => { | ||
| try { | ||
| const project = req.project; // assuming middleware attaches project | ||
| if (!project) { | ||
| return res.status(404).json({ error: "Project not found" }); | ||
| } | ||
| const project = req.project; | ||
| if (!project) return errorResponse(res, 404, "Project not found."); |
There was a problem hiding this comment.
deleteAllFiles is under-authorized for a destructive operation.
This endpoint allows full project file purge using API-key middleware only. Given it is bulk destructive, require authenticated owner authorization (as done in backend/controllers/project.controller.js ownership-checked deletion path) or an explicitly scoped admin credential.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/controllers/storage.controller.js` around lines 124 - 127, The
deleteAllFiles handler is currently protected only by an API-key and must
enforce owner/admin-level authorization; update module.exports.deleteAllFiles to
require an authenticated user and verify ownership or an explicit admin scope
before performing the destructive purge (e.g., check that req.user exists and
either req.user.id === project.ownerId or
req.user.isAdmin/hasScope('admin:files:delete')), otherwise return a 403
errorResponse; mirror the ownership-check pattern used in the project deletion
path in project.controller.js and do not proceed with file deletion unless that
check passes.
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 (5)
backend/controllers/project.controller.js (4)
629-638:⚠️ Potential issue | 🟠 MajorInvalidate cached project lookups on delete.
This path removes the Mongo document but never clears the Redis entries that other mutators invalidate with
deleteProjectById()anddeleteProjectByApiKeyCache(). Until those keys expire, lookups by project ID or API key can still resolve a deleted project.🔧 Suggested fix
await Project.deleteOne({ _id: projectId }); + await deleteProjectById(projectId); + await deleteProjectByApiKeyCache(project.apiKey); storageRegistry.delete(projectId.toString());Also applies to: 671-674
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/controllers/project.controller.js` around lines 629 - 638, The delete path removes the Mongo Project but doesn't clear Redis cache keys, so lookups still return deleted projects; after successfully removing the Project document in the controller (the code around the Project.findOne and subsequent delete logic), call the existing cache invalidation helpers deleteProjectById(projectId) and deleteProjectByApiKeyCache(project.apiKey) (or the appropriate cached API key value) to evict Redis entries, and mirror the same calls in the other delete branch referenced (lines around 671-674) so both code paths invalidate caches consistently.
115-120:⚠️ Potential issue | 🔴 CriticalHarden the URI guard before dialing user-supplied MongoDB endpoints.
isSafeUri()only blocks a few loopback literals, but this handler stillcreateConnection()s to any other host. Private, link-local, and metadata targets can pass this check, which turns this endpoint into an internal network probe for authenticated users. Resolve the hostname and reject private/reserved ranges, or safer, allow-list approved MongoDB hosts before connecting.Also applies to: 135-142
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/controllers/project.controller.js` around lines 115 - 120, isSafeUri currently only rejects a few loopback hostnames (function isSafeUri) but still allows private, link-local and metadata service addresses to be dialed by createConnection; update isSafeUri to perform a DNS resolution of the provided hostname (use dns.lookup or dns.promises.lookup) and validate the resulting IP(s) against RFC1918/RFC6890 private, loopback, link-local and reserved ranges (and IPv6 equivalents) rejecting any match, or alternatively replace this logic with an explicit allow-list of approved MongoDB hosts used by the controller before calling createConnection; ensure the same strengthened check is applied to the other connection path referenced in the diff (the block around the second createConnection usage).
187-203:⚠️ Potential issue | 🟠 MajorAdd validation error handling to return 400 for malformed requests.
Both
deleteExternalDbConfiganddeleteExternalStorageConfigcallz.object(...).parse(req.body)but lack validation error handling. When zod validation fails, the ZodError is caught by the generic catch block and returned as 500, inconsistent with the validation error pattern used elsewhere in the file (e.g., line 180). Add a check forerr.errorsorerr.issuesbefore the generic catch to return 400 with the validation error message.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/controllers/project.controller.js` around lines 187 - 203, The Zod validation in deleteExternalDbConfig (and similarly deleteExternalStorageConfig) currently throws validation failures into the generic catch which returns 500; update each function to detect Zod validation errors after the try/catch by checking for err.errors or err.issues (or instanceof ZodError) and respond with errorResponse(res, 400, <validation message>) instead of 500, mirroring the validation handling pattern used elsewhere in this controller; ensure you keep the existing successResponse and Project.save logic intact and only change the error branch to return 400 for malformed req.body from z.object(...).parse(req.body).
576-589:⚠️ Potential issue | 🟠 MajorAbort the batch loop when storage deletion fails.
The result of
supabase.storage.from(bucket).remove(paths)is ignored. If that call fails,deletedis still incremented and the nextlist()will see the same files again, creating an infinite loop while reporting a false deletion count. The correct error-handling pattern is already used elsewhere in this file (line ~530).🔧 Suggested fix
} else { const paths = data.map(f => `${projectId}/${f.name}`); - await supabase.storage.from(bucket).remove(paths); + const { error: removeError } = await supabase.storage.from(bucket).remove(paths); + if (removeError) throw removeError; deleted += data.length; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/controllers/project.controller.js` around lines 576 - 589, The loop ignores the result of supabase.storage.from(bucket).remove(paths), so on failure deleted is still incremented and the loop can spin indefinitely; change the code to capture and check the remove result (e.g., const { data: removed, error: removeError } = await supabase.storage.from(bucket).remove(paths)), throw or return on removeError (mirroring the existing pattern used around line ~530), and only increment deleted after a successful removal (use removed.length or the original data.length from the list response). Ensure hasMore is only updated after successful deletions so the loop can abort on failure.backend/controllers/userAuth.controller.js (1)
88-105:⚠️ Potential issue | 🟠 MajorSeparate JWT verification from data lookup to avoid masking database errors.
Lines 88–105 wrap both
jwt.verify()and the Mongo collection lookup in the same try/catch block. Database failures (connection errors, ObjectId casting, etc.) are caught and returned as 401 "Invalid or Expired Token," hiding real outages. Extract JWT verification into its own try/catch that returns 401, then move the lookup outside so it either succeeds or propagates to the outer error handler (which correctly returns 500).🔧 Suggested shape
- try { - const decoded = jwt.verify(token, project.jwtSecret); - - const collectionName = `${project._id}_users`; - const collection = mongoose.connection.db.collection(collectionName); - - const user = await collection.findOne( - { _id: new mongoose.Types.ObjectId(decoded.userId) }, - { projection: { password: 0 } } - ); - - if (!user) return errorResponse(res, 404, "User not found."); - - return successResponse(res, 200, { data: user }); - } catch (err) { - return errorResponse(res, 401, "Invalid or Expired Token."); - } + let decoded; + try { + decoded = jwt.verify(token, project.jwtSecret); + } catch (err) { + return errorResponse(res, 401, "Invalid or Expired Token."); + } + + const collectionName = `${project._id}_users`; + const collection = mongoose.connection.db.collection(collectionName); + const user = await collection.findOne( + { _id: new mongoose.Types.ObjectId(decoded.userId) }, + { projection: { password: 0 } } + ); + + if (!user) return errorResponse(res, 404, "User not found."); + + return successResponse(res, 200, { data: user });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/controllers/userAuth.controller.js` around lines 88 - 105, Extract JWT verification into its own try/catch so token errors return 401 only; call jwt.verify(token, project.jwtSecret) inside a small try that on failure calls errorResponse(res, 401, "Invalid or Expired Token."); after successful decode, move the Mongo lookup (building collectionName, getting collection via mongoose.connection.db.collection, and awaiting collection.findOne with new mongoose.Types.ObjectId(decoded.userId)) outside that JWT try/catch so database errors can bubble to the outer handler and be returned as 500 rather than being masked as token errors; keep the existing successResponse(res, 200, { data: user }) and the not-found path errorResponse(res, 404, "User not found.") unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/controllers/project.controller.js`:
- Around line 614-618: The response envelope change wraps payloads under data
and breaks existing frontend callers; revert the envelope for the affected
handlers by returning the raw object instead of { data: ... }. Specifically, in
the updateProject handler (where you build projectObj and call successResponse)
and the analytics handler (the similar block around lines 705-713), change the
successResponse payload to return projectObj (or the analytics object) directly
so existing frontend consumers that expect res.data to be the object keep
working; update the successResponse invocation in those functions (not the
frontend) to preserve the original response shape.
- Around line 327-331: Replace the two different hard-coded fallback quotas with
a single shared fallback used by both write paths: define a constant (e.g.
DEFAULT_DB_QUOTA = 20 * 1024 * 1024) or a helper getDatabaseLimit(project) and
use project.databaseLimit || DEFAULT_DB_QUOTA everywhere you check quotas
(notably in the insertData path where you compute docSize and in the editRow
path and the other block noted around lines 407-413); update the quota checks to
reference that single fallback so inserts and edits enforce the same default
limit.
---
Outside diff comments:
In `@backend/controllers/project.controller.js`:
- Around line 629-638: The delete path removes the Mongo Project but doesn't
clear Redis cache keys, so lookups still return deleted projects; after
successfully removing the Project document in the controller (the code around
the Project.findOne and subsequent delete logic), call the existing cache
invalidation helpers deleteProjectById(projectId) and
deleteProjectByApiKeyCache(project.apiKey) (or the appropriate cached API key
value) to evict Redis entries, and mirror the same calls in the other delete
branch referenced (lines around 671-674) so both code paths invalidate caches
consistently.
- Around line 115-120: isSafeUri currently only rejects a few loopback hostnames
(function isSafeUri) but still allows private, link-local and metadata service
addresses to be dialed by createConnection; update isSafeUri to perform a DNS
resolution of the provided hostname (use dns.lookup or dns.promises.lookup) and
validate the resulting IP(s) against RFC1918/RFC6890 private, loopback,
link-local and reserved ranges (and IPv6 equivalents) rejecting any match, or
alternatively replace this logic with an explicit allow-list of approved MongoDB
hosts used by the controller before calling createConnection; ensure the same
strengthened check is applied to the other connection path referenced in the
diff (the block around the second createConnection usage).
- Around line 187-203: The Zod validation in deleteExternalDbConfig (and
similarly deleteExternalStorageConfig) currently throws validation failures into
the generic catch which returns 500; update each function to detect Zod
validation errors after the try/catch by checking for err.errors or err.issues
(or instanceof ZodError) and respond with errorResponse(res, 400, <validation
message>) instead of 500, mirroring the validation handling pattern used
elsewhere in this controller; ensure you keep the existing successResponse and
Project.save logic intact and only change the error branch to return 400 for
malformed req.body from z.object(...).parse(req.body).
- Around line 576-589: The loop ignores the result of
supabase.storage.from(bucket).remove(paths), so on failure deleted is still
incremented and the loop can spin indefinitely; change the code to capture and
check the remove result (e.g., const { data: removed, error: removeError } =
await supabase.storage.from(bucket).remove(paths)), throw or return on
removeError (mirroring the existing pattern used around line ~530), and only
increment deleted after a successful removal (use removed.length or the original
data.length from the list response). Ensure hasMore is only updated after
successful deletions so the loop can abort on failure.
In `@backend/controllers/userAuth.controller.js`:
- Around line 88-105: Extract JWT verification into its own try/catch so token
errors return 401 only; call jwt.verify(token, project.jwtSecret) inside a small
try that on failure calls errorResponse(res, 401, "Invalid or Expired Token.");
after successful decode, move the Mongo lookup (building collectionName, getting
collection via mongoose.connection.db.collection, and awaiting
collection.findOne with new mongoose.Types.ObjectId(decoded.userId)) outside
that JWT try/catch so database errors can bubble to the outer handler and be
returned as 500 rather than being masked as token errors; keep the existing
successResponse(res, 200, { data: user }) and the not-found path
errorResponse(res, 404, "User not found.") unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 199ad93b-0ba8-4bac-8fde-c887d4d4fcf1
📒 Files selected for processing (5)
backend/controllers/auth.controller.jsbackend/controllers/data.controller.jsbackend/controllers/project.controller.jsbackend/controllers/storage.controller.jsbackend/controllers/userAuth.controller.js
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/controllers/data.controller.js
- backend/controllers/storage.controller.js
- backend/controllers/auth.controller.js
| if (!project.resources.db.isExternal) { | ||
| docSize = Buffer.byteLength(JSON.stringify(incomingData)); | ||
|
|
||
| const limit = project.databaseLimit || 20 * 1024 * 1024; | ||
|
|
||
| if ((project.databaseUsed || 0) + docSize > limit) { | ||
| return res.status(403).json({ error: "Database limit exceeded. Delete some data." }); | ||
| return errorResponse(res, 403, "Database limit exceeded. Delete some data."); |
There was a problem hiding this comment.
Use one fallback database quota in both write paths.
insertData() falls back to 20 * 1024 * 1024, but editRow() falls back to 500 * 1024 * 1024. When project.databaseLimit is unset, edits can grow a project far beyond the cap enforced on inserts.
Also applies to: 407-413
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/controllers/project.controller.js` around lines 327 - 331, Replace
the two different hard-coded fallback quotas with a single shared fallback used
by both write paths: define a constant (e.g. DEFAULT_DB_QUOTA = 20 * 1024 *
1024) or a helper getDatabaseLimit(project) and use project.databaseLimit ||
DEFAULT_DB_QUOTA everywhere you check quotas (notably in the insertData path
where you compute docSize and in the editRow path and the other block noted
around lines 407-413); update the quota checks to reference that single fallback
so inserts and edits enforce the same default limit.
| const projectObj = project.toObject(); | ||
| delete projectObj.apiKey; | ||
| delete projectObj.jwtSecret; | ||
|
|
||
| return successResponse(res, 200, { message: "Project updated successfully.", data: projectObj }); |
There was a problem hiding this comment.
This response-envelope change breaks the current frontend callers.
updateProject and analytics now wrap their payloads under data, but the existing consumers in frontend/src/pages/ProjectSettings.jsx:1-50 and frontend/src/pages/Analytics.jsx:1-30 still store res.data directly. Those callers will now receive the envelope instead of the project/analytics object unless they are updated in the same PR.
Also applies to: 705-713
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/controllers/project.controller.js` around lines 614 - 618, The
response envelope change wraps payloads under data and breaks existing frontend
callers; revert the envelope for the affected handlers by returning the raw
object instead of { data: ... }. Specifically, in the updateProject handler
(where you build projectObj and call successResponse) and the analytics handler
(the similar block around lines 705-713), change the successResponse payload to
return projectObj (or the analytics object) directly so existing frontend
consumers that expect res.data to be the object keep working; update the
successResponse invocation in those functions (not the frontend) to preserve the
original response shape.
00d24c6 to
28e82da
Compare
1 participant
Summary
Closes #40
Standardizes all API responses across every controller to follow a consistent JSON structure, as requested by the maintainer.
Changes
New Utility:
backend/utils/errorResponse.jsTwo helper functions added:
errorResponse(res, status, message)— for all error responsessuccessResponse(res, status, { message, data })— for all success responsesError Response Format
{ "success": false, "error": "A clear message", "code": 400 }Success Response Format
{ "success": true, "message": "Done", "data": {}, "code": 200 }Controllers Updated
auth.controller.jsdata.controller.jsproject.controller.jsrelease.controller.jsschema.controller.jsstorage.controller.jsuserAuth.controller.jsAdditional Fixes
Minor bugs and cleanup found during review (duplicate function calls, missing
await, debug logs removed).Testing
Verified locally with curl:
POST /api/auth/register— returns correct success shapePOST /api/auth/registerwith duplicate email — returns correct error shapeSummary by CodeRabbit
New Features
Bug Fixes
Refactor