feat(public-api): migrate controllers to AppError and update tests - #316
Merged
Merged
Conversation
Ayush4958
requested review from
Nitin-kumar-yadav1307 and
yash-pouranik
as code owners
June 17, 2026 06:35
Contributor
📝 WalkthroughWalkthroughAll exported controller handlers in the public API ( ChangesController Error Handling Standardization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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)
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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/public-api/src/__tests__/softDelete.test.js (1)
117-127:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPass
nextin the success-path recovery test to match handler signature.
recoverSingleDocis now a(req, res, next)controller, but this test still calls it withoutnext. If the code path ever errors, the test can fail withnext is not a functioninstead of asserting the intended AppError flow.🔧 Suggested fix
- await recoverSingleDoc(req, res); + await recoverSingleDoc(req, res, next); + expect(next).not.toHaveBeenCalled();🤖 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__/softDelete.test.js` around lines 117 - 127, The test for recoverSingleDoc is calling the function with only req and res parameters, but the controller now expects a (req, res, next) signature. Create a mock next function using jest.fn() before the test and pass it as the third argument when calling recoverSingleDoc(req, res, next) to match the updated handler signature and ensure proper error handling if the code path errors.apps/public-api/src/controllers/storage.controller.js (1)
303-309:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
confirmUploadquota check is inconsistent with the effective-limit model used byuploadFile/requestUpload.
requestUploadenforcesgetEffectiveStorageLimit(project, req)(including safety clamp behavior), butconfirmUploadcharges against persistedproject.storageLimitonly. This can let confirmation succeed under a different limit model than reservation, causing quota/accounting drift under concurrency or plan override scenarios.Proposed fix
// now it's safe to charge quota if (!external) { + const effectiveLimit = getEffectiveStorageLimit(project, req); + const quotaLimit = effectiveLimit === -1 ? SAFETY_MAX_BYTES : effectiveLimit; const result = await Project.updateOne( { _id: project._id, - $or: [ - { storageLimit: -1 }, - { $expr: { $lte: [{ $add: ["$storageUsed", actualSize] }, "$storageLimit"] } } - ] + $expr: { $lte: [{ $add: ["$storageUsed", actualSize] }, quotaLimit] } }, { $inc: { storageUsed: actualSize } } );Also applies to: 367-373, 379-379
🤖 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/storage.controller.js` around lines 303 - 309, The confirmUpload method uses project.storageLimit directly for quota enforcement, but requestUpload uses getEffectiveStorageLimit(project, req) which includes safety clamp behavior. This inconsistency can cause quota/accounting drift under concurrency or plan override scenarios. In the confirmUpload function (at the locations noted in lines 367-373 and 379), replace all instances where project.storageLimit is used for quota checking with getEffectiveStorageLimit(project, req), following the same pattern as requestUpload where the effective limit is used to determine the quotaLimit value for comparison against project.storageUsed + numericSize.apps/public-api/src/controllers/userAuth.controller.js (2)
754-757:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFinish migrating error paths to
next(new AppError(...))These branches still bypass centralized error middleware by sending direct JSON responses, so response shape/status behavior becomes inconsistent across handlers.
As per coding guidelines, “All API endpoints must return response format:
{ success: bool, data: {}, message: "" }” and “Use AppError class for errors. Never use raw throw statements or expose MongoDB errors to clients.”Also applies to: 780-783, 932-933, 1010-1011, 1420-1421
🤖 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/userAuth.controller.js` around lines 754 - 757, Replace all direct JSON response calls that bypass the centralized error middleware with the AppError class pattern. In the userAuth.controller.js file, locate all error response blocks (including the one at lines 754-757 returning status 422 with provider configuration error, and the additional instances at lines 780-783, 932-933, 1010-1011, and 1420-1421) and replace each `res.status(statusCode).json({...})` pattern with `next(new AppError(message, statusCode))`. This ensures all errors flow through centralized middleware for consistent response formatting and proper error handling as per the coding guidelines.Source: Coding guidelines
1076-1083:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSuccess payload contract is still inconsistent
Several success responses are still ad-hoc objects (
token,user, etc.) instead of the required{ success, data, message }envelope, which breaks the stated API contract consistency goal.As per coding guidelines, “All API endpoints must return response format:
{ success: bool, data: {}, message: "" }.”Also applies to: 1185-1190, 1229-1229, 1301-1304, 1755-1760
🤖 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/userAuth.controller.js` around lines 1076 - 1083, The response object in the user registration success handler does not follow the required API contract format of { success, data, message }. Refactor the res.status(201).json() response to wrap the current payload in the standard envelope: set success to true, move the accessToken, expiresIn, conditional refreshToken (from shouldExposeRefreshToken(req)), and userId into a data object, and keep the message as a top-level property. Apply this same refactoring pattern to all other affected endpoints mentioned (lines 1185-1190, 1229-1229, 1301-1304, and 1755-1760) to ensure consistent API contract compliance throughout the userAuth.controller.js file.Source: Coding guidelines
🧹 Nitpick comments (7)
apps/public-api/src/__tests__/health.route.test.js (3)
65-73: ⚡ Quick winError assertions should verify the
errorfield.The error response includes an
errorfield (set to "Health Check Failed" from the controller), but the test only checkssuccessandmessage. Verifying theerrorfield ensures the complete response contract is validated.📝 Add error field assertion
test('returns error when redis is not responsive', async () => { mockRedis.ping.mockRejectedValue(new Error('redis unavailable')); const res = await request(app).get('/api/health'); expect(res.status).toBe(503); expect(res.body.success).toBe(false); + expect(res.body.error).toBe('Health Check Failed'); expect(res.body.message).toBe('Service unavailable'); });🤖 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__/health.route.test.js` around lines 65 - 73, The test 'returns error when redis is not responsive' is missing an assertion to verify the error field in the response body. The controller returns an error field with the value "Health Check Failed" when the health check fails, but the test only validates the success and message fields. Add an additional expect assertion after the existing ones to verify that res.body.error equals "Health Check Failed" to ensure the complete error response contract is being validated.
55-63: ⚡ Quick winError assertions should verify the
errorfield.The error response includes an
errorfield (set to "Health Check Failed" from the controller), but the test only checkssuccessandmessage. Verifying theerrorfield ensures the complete response contract is validated.📝 Add error field assertion
test('returns error when mongodb is disconnected', async () => { mongoose.connection.readyState = 0; const res = await request(app).get('/api/health'); expect(res.status).toBe(503); expect(res.body.success).toBe(false); + expect(res.body.error).toBe('Health Check Failed'); expect(res.body.message).toBe('Service unavailable'); });🤖 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__/health.route.test.js` around lines 55 - 63, The test 'returns error when mongodb is disconnected' is missing an assertion for the error field in the response body. Add an expect statement to verify that res.body.error equals "Health Check Failed" alongside the existing assertions for res.status, res.body.success, and res.body.message to fully validate the error response contract.
14-18: ⚡ Quick winAppError mock doesn't fully match the production implementation.
The mock is missing the
statusandisOperationalfields, and uses simplified default logic for theerrorfield. While this works for the current test cases, it could mask issues if other code paths rely on these fields.♻️ Improve mock fidelity
jest.mock('`@urbackend/common`', () => ({ - AppError: class AppError extends Error { constructor(code, msg, errTitle) { super(msg); this.statusCode=code; this.error=errTitle||'Error'; } }, + AppError: class AppError extends Error { + constructor(code, msg, errTitle) { + super(msg); + this.statusCode = code; + this.status = `${code}`.startsWith('4') ? 'fail' : 'error'; + this.error = errTitle || (code >= 500 ? "Internal Server Error" : "Error"); + this.isOperational = true; + } + }, ApiResponse: class ApiResponse { constructor(d, m) { this.data=d; this.message=m; this.success=true; } send(res, code) { return res.status(code).json({ success: this.success, data: this.data, message: this.message }); } }, redis: mockRedis, }));This ensures the mock behavior matches production, particularly the conditional default for the
errorfield based on status code.🤖 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__/health.route.test.js` around lines 14 - 18, The AppError mock class in the jest.mock call for `@urbackend/common` is missing the `status` and `isOperational` fields that exist in the production implementation. Update the AppError constructor to add a `status` field that matches the `code` parameter, add an `isOperational` field (typically true for operational errors), and implement conditional logic for the `error` field that sets a default error message based on the status code rather than simply using the errTitle parameter directly. This will ensure the mock behavior aligns with the actual production AppError class.apps/public-api/src/controllers/health.controller.js (1)
31-35: ⚖️ Poor tradeoffConsider including dependency status in error responses for better diagnostics.
When the health check fails, the
payloadwith detailed dependency information (which MongoDB/Redis is down) is constructed but not returned to the client. The error response only includes "Service unavailable" without specifics.
[optional]🔍 Optional enhancement for operational diagnostics
If exposing internal dependency status is acceptable from a security perspective, consider including the payload in the error response:
if (status === 'ok') { return new ApiResponse(payload, "Health Check Passed").send(res, 200); } else { - return next(new AppError(503, "Service unavailable", "Health Check Failed")); + // Option 1: Include dependencies in message for logging/debugging + const detailMsg = `Service unavailable. MongoDB: ${payload.dependencies.mongodb}, Redis: ${payload.dependencies.redis}`; + return next(new AppError(503, detailMsg, "Health Check Failed")); + + // Option 2: If you want detailed response, use ApiResponse with 503 + // return new ApiResponse(payload, "Service unavailable").send(res, 503); }If the current approach is intentional to avoid exposing internal architecture details, you can disregard this suggestion.
🤖 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/health.controller.js` around lines 31 - 35, In the health check controller's failure branch (the else block), the `payload` variable containing detailed dependency status information is not being included in the error response. Modify the `AppError` call to include the `payload` data along with the error message so that clients receive specific information about which dependencies (MongoDB/Redis) are down, rather than just the generic "Service unavailable" message. Pass the payload as part of the error response to provide better diagnostic information.apps/public-api/src/__tests__/storage.controller.test.js (1)
39-46: ⚡ Quick winAlign the mocked
AppErrorshape with production (error, nottype) and assert the conflict error code.The mock currently stores the third argument as
type, while the sharedAppErrorcontract uses.error. This weakens coverage for theUPLOAD_NOT_READYbranch.Proposed test updates
- AppError: class AppError extends Error { - constructor(statusCode, message, type) { + AppError: class AppError extends Error { + constructor(statusCode, message, error = null) { super(message); this.statusCode = statusCode; - if (type) this.type = type; + this.error = error || (statusCode >= 500 ? 'Internal Server Error' : 'Error'); } },expect(next).toHaveBeenCalledWith(expect.any(AppError)); expect(next.mock.calls[0][0].statusCode).toBe(409); + expect(next.mock.calls[0][0].error).toBe('UPLOAD_NOT_READY');Also applies to: 537-550
🤖 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__/storage.controller.test.js` around lines 39 - 46, The mocked AppError class in the test file uses this.type for the third constructor argument, but the production AppError contract uses this.error instead. Update the AppError mock constructor to assign the third parameter to this.error (using the same conditional check) to align with the production implementation. Additionally, add assertions in the test cases that cover the UPLOAD_NOT_READY branch (around lines 537-550) to verify that the error code property matches the expected conflict error code value.apps/public-api/src/__tests__/userAuth.email.test.js (1)
88-89: ⚡ Quick winAlign mocked
AppErrorwith runtime shapeThe test mock only sets
statusCode; productionAppErroralso carrieserrorandisOperational. Adding those fields improves parity and prevents false confidence in error-contract tests.🤖 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__/userAuth.email.test.js` around lines 88 - 89, The mocked AppError class in the test is incomplete compared to the production version. In the AppError mock class constructor, add initialization of the `error` and `isOperational` properties alongside the existing `statusCode` property assignment. This ensures the mock accurately reflects the actual shape of the production AppError, improving test reliability and preventing false confidence in error-contract validation.apps/public-api/src/__tests__/userAuth.social.test.js (1)
644-645: ⚡ Quick winPass
nextconsistently in callback testsThese two tests still invoke
handleSocialAuthCallbackwithoutnext, unlike the rest of the suite. Passingnexthere keeps invocation parity with runtime and improves failure-path coverage consistency.Also applies to: 684-685
🤖 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__/userAuth.social.test.js` around lines 644 - 645, The calls to handleSocialAuthCallback in the test suite are inconsistent with the rest of the tests. Add the next parameter as the third argument to both handleSocialAuthCallback invocations (at the locations indicated in the comment) so that all tests pass req, res, and next consistently, which will maintain parity with the runtime behavior and improve failure-path coverage consistency.
🤖 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/public-api/src/app.js`:
- Around line 103-109: The operational error handler response format does not
align with the coding guidelines that mandate all API responses use the
structure { success: bool, data: {}, message: "" }. In the error handling block
that checks if err.isOperational and err.statusCode exist, modify the JSON
response to use a data field containing the error details instead of a separate
error field. This ensures consistency across all API endpoints and adheres to
the documented response contract.
In `@apps/public-api/src/controllers/data.controller.js`:
- Around line 102-105: Remove raw backend error details from AppError responses
in the data.controller.js file. At line 102, remove the err.message parameter
from the AppError constructor for the duplicate constraint violation (keeping
only the generic client-safe message). At line 105, replace the raw err.message
with a generic client-safe message like "An error occurred while processing your
request" instead of exposing the MongoDB error details. Apply the same pattern
to all other locations where AppError is instantiated with err.message (lines
398, 471, and 645), ensuring raw error details are never passed to clients but
kept only in server-side logs for debugging purposes.
In `@apps/public-api/src/controllers/schema.controller.js`:
- Around line 201-207: The catch block in the schema controller incorrectly
returns status 400 for all non-Zod errors, but server-side failures like
fullProject.save(), getConnection(), and createUniqueIndexes() should return 500
instead. Distinguish between validation/client errors and server errors by
checking the error type or instanceof checks before deciding the status code.
For server-side errors, return AppError with status 500 and a generic error
message instead of exposing internal err.message details to the client. Keep the
detailed error information in the console.error log for server-side debugging.
In `@apps/public-api/src/controllers/storage.controller.js`:
- Line 168: In storage.controller.js, modify all catch blocks (at lines 168,
230, 284, 322, 404) to preserve upstream AppError metadata instead of
force-converting every exception to a generic 500 error. Check if the caught
error is already an instance of AppError; if so, pass it through directly. If
it's not an AppError, create a new AppError with an appropriate status code.
Additionally, only expose err.message in development mode to avoid leaking
sensitive error details to clients in production. This ensures that operational
errors from dependencies maintain their original status codes and prevents raw
error messages from being exposed to end users.
- Line 228: The deleteFile endpoint in the storage.controller.js file violates
the standardized API response contract by passing null as the data parameter to
ApiResponse on line 228. To fix this, replace the null argument with an
appropriate response object (such as an empty object {} or a message object
containing relevant deletion details) to ensure the endpoint returns the
standardized format of { success: bool, data: {}, message: "" } as required by
the coding guidelines, allowing clients to receive properly structured responses
instead of null payloads.
In `@apps/public-api/src/controllers/userAuth.controller.js`:
- Around line 1378-1379: In the verification update query within the userAuth
controller, replace the `email` field with `normalizedEmail` in the filter
object (the first object parameter passed to the update operation). This ensures
that the query correctly matches the user record regardless of email case
variations, allowing OTP verification to work properly for mixed-case email
inputs.
- Around line 1680-1682: In the userAuth.controller.js file, the project ID
mismatch check (comparing req.project._id with session.projectId) returns an
error without clearing the refresh cookie first, leaving a stale cookie on the
client. Before returning the AppError with status 403 in this condition block,
clear the refresh cookie from the response object to ensure the client doesn't
retain an invalid token that would cause subsequent failed refresh/logout
attempts. Apply the same cookie cleanup to the related project-mismatch early
return mentioned at lines 1775-1777.
- Around line 1089-1090: The code is exposing raw internal error messages by
passing `err.message` directly to the AppError constructor for 500 responses.
Replace `err.message` with a generic client-safe message like "An error
occurred" in all AppError(500, ...) calls in the userAuth.controller.js file (at
lines 1089-1090, 1194-1195, 1265-1266, and 1849-1850). Keep the console.log(err)
or use an appropriate server-side logger to capture the actual error details for
debugging purposes without exposing them to the client.
- Around line 1031-1032: Remove the debug console.log statement that logs 'ABOUT
TO CALL NEXT FOR EXISTING USER' from the signup flow in the
userAuth.controller.js file. Additionally, identify and remove any unreachable
code that exists after the return next(new AppError(400, "User already exists
with this email.")) statement, as code following a return statement cannot be
executed and should not be in the codebase.
- Line 1193: The code uses the deprecated `err.errors` property from Zod which
no longer exists in Zod v4.1.13; instead, Zod v4 provides `err.issues`. In the
userAuth.controller.js file, locate all instances where `instanceof z.ZodError`
is checked and `err.errors` is passed to the AppError constructor, and replace
`err.errors` with `err.issues?.[0]?.message || "Validation failed"` to match the
pattern already established elsewhere in the file and ensure proper error
message extraction for Zod v4 compatibility.
---
Outside diff comments:
In `@apps/public-api/src/__tests__/softDelete.test.js`:
- Around line 117-127: The test for recoverSingleDoc is calling the function
with only req and res parameters, but the controller now expects a (req, res,
next) signature. Create a mock next function using jest.fn() before the test and
pass it as the third argument when calling recoverSingleDoc(req, res, next) to
match the updated handler signature and ensure proper error handling if the code
path errors.
In `@apps/public-api/src/controllers/storage.controller.js`:
- Around line 303-309: The confirmUpload method uses project.storageLimit
directly for quota enforcement, but requestUpload uses
getEffectiveStorageLimit(project, req) which includes safety clamp behavior.
This inconsistency can cause quota/accounting drift under concurrency or plan
override scenarios. In the confirmUpload function (at the locations noted in
lines 367-373 and 379), replace all instances where project.storageLimit is used
for quota checking with getEffectiveStorageLimit(project, req), following the
same pattern as requestUpload where the effective limit is used to determine the
quotaLimit value for comparison against project.storageUsed + numericSize.
In `@apps/public-api/src/controllers/userAuth.controller.js`:
- Around line 754-757: Replace all direct JSON response calls that bypass the
centralized error middleware with the AppError class pattern. In the
userAuth.controller.js file, locate all error response blocks (including the one
at lines 754-757 returning status 422 with provider configuration error, and the
additional instances at lines 780-783, 932-933, 1010-1011, and 1420-1421) and
replace each `res.status(statusCode).json({...})` pattern with `next(new
AppError(message, statusCode))`. This ensures all errors flow through
centralized middleware for consistent response formatting and proper error
handling as per the coding guidelines.
- Around line 1076-1083: The response object in the user registration success
handler does not follow the required API contract format of { success, data,
message }. Refactor the res.status(201).json() response to wrap the current
payload in the standard envelope: set success to true, move the accessToken,
expiresIn, conditional refreshToken (from shouldExposeRefreshToken(req)), and
userId into a data object, and keep the message as a top-level property. Apply
this same refactoring pattern to all other affected endpoints mentioned (lines
1185-1190, 1229-1229, 1301-1304, and 1755-1760) to ensure consistent API
contract compliance throughout the userAuth.controller.js file.
---
Nitpick comments:
In `@apps/public-api/src/__tests__/health.route.test.js`:
- Around line 65-73: The test 'returns error when redis is not responsive' is
missing an assertion to verify the error field in the response body. The
controller returns an error field with the value "Health Check Failed" when the
health check fails, but the test only validates the success and message fields.
Add an additional expect assertion after the existing ones to verify that
res.body.error equals "Health Check Failed" to ensure the complete error
response contract is being validated.
- Around line 55-63: The test 'returns error when mongodb is disconnected' is
missing an assertion for the error field in the response body. Add an expect
statement to verify that res.body.error equals "Health Check Failed" alongside
the existing assertions for res.status, res.body.success, and res.body.message
to fully validate the error response contract.
- Around line 14-18: The AppError mock class in the jest.mock call for
`@urbackend/common` is missing the `status` and `isOperational` fields that exist
in the production implementation. Update the AppError constructor to add a
`status` field that matches the `code` parameter, add an `isOperational` field
(typically true for operational errors), and implement conditional logic for the
`error` field that sets a default error message based on the status code rather
than simply using the errTitle parameter directly. This will ensure the mock
behavior aligns with the actual production AppError class.
In `@apps/public-api/src/__tests__/storage.controller.test.js`:
- Around line 39-46: The mocked AppError class in the test file uses this.type
for the third constructor argument, but the production AppError contract uses
this.error instead. Update the AppError mock constructor to assign the third
parameter to this.error (using the same conditional check) to align with the
production implementation. Additionally, add assertions in the test cases that
cover the UPLOAD_NOT_READY branch (around lines 537-550) to verify that the
error code property matches the expected conflict error code value.
In `@apps/public-api/src/__tests__/userAuth.email.test.js`:
- Around line 88-89: The mocked AppError class in the test is incomplete
compared to the production version. In the AppError mock class constructor, add
initialization of the `error` and `isOperational` properties alongside the
existing `statusCode` property assignment. This ensures the mock accurately
reflects the actual shape of the production AppError, improving test reliability
and preventing false confidence in error-contract validation.
In `@apps/public-api/src/__tests__/userAuth.social.test.js`:
- Around line 644-645: The calls to handleSocialAuthCallback in the test suite
are inconsistent with the rest of the tests. Add the next parameter as the third
argument to both handleSocialAuthCallback invocations (at the locations
indicated in the comment) so that all tests pass req, res, and next
consistently, which will maintain parity with the runtime behavior and improve
failure-path coverage consistency.
In `@apps/public-api/src/controllers/health.controller.js`:
- Around line 31-35: In the health check controller's failure branch (the else
block), the `payload` variable containing detailed dependency status information
is not being included in the error response. Modify the `AppError` call to
include the `payload` data along with the error message so that clients receive
specific information about which dependencies (MongoDB/Redis) are down, rather
than just the generic "Service unavailable" message. Pass the payload as part of
the error response to provide better diagnostic information.
🪄 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: 11a4da90-c200-4b4a-a46a-b9afab1acb90
📒 Files selected for processing (16)
apps/public-api/src/__tests__/aggregation.test.jsapps/public-api/src/__tests__/data.controller.read.test.jsapps/public-api/src/__tests__/health.route.test.jsapps/public-api/src/__tests__/mail.controller.test.jsapps/public-api/src/__tests__/softDelete.test.jsapps/public-api/src/__tests__/storage.controller.test.jsapps/public-api/src/__tests__/userAuth.email.test.jsapps/public-api/src/__tests__/userAuth.refresh.test.jsapps/public-api/src/__tests__/userAuth.social.test.jsapps/public-api/src/app.jsapps/public-api/src/controllers/data.controller.jsapps/public-api/src/controllers/health.controller.jsapps/public-api/src/controllers/mail.controller.jsapps/public-api/src/controllers/schema.controller.jsapps/public-api/src/controllers/storage.controller.jsapps/public-api/src/controllers/userAuth.controller.js
Contributor
There was a problem hiding this comment.
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/schema.controller.js (1)
49-52:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid returning internal exception text from
checkSchema.Line 51 forwards
err.messageto clients for unexpected 500s. Use a generic message and keep the detailed error inconsole.error.🛡️ Proposed fix
} catch (err) { console.error(err); - return next(new AppError(500, err.message)); + return next(new AppError(500, "An error occurred while checking the schema.")); }As per coding guidelines, “Use AppError class for errors. Never use raw throw statements or expose MongoDB errors to clients.”
🤖 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/schema.controller.js` around lines 49 - 52, The catch block in the schema.controller.js file is exposing internal error details to clients by passing err.message directly to the AppError constructor. Replace the err.message parameter in the AppError call with a generic, user-friendly message like "An error occurred while validating the schema" or similar, keeping the detailed error information only in the console.error call. This ensures sensitive error details remain server-side while clients receive appropriate but non-revealing error messages.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.
Outside diff comments:
In `@apps/public-api/src/controllers/schema.controller.js`:
- Around line 49-52: The catch block in the schema.controller.js file is
exposing internal error details to clients by passing err.message directly to
the AppError constructor. Replace the err.message parameter in the AppError call
with a generic, user-friendly message like "An error occurred while validating
the schema" or similar, keeping the detailed error information only in the
console.error call. This ensures sensitive error details remain server-side
while clients receive appropriate but non-revealing error messages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c1dedbc5-f640-4152-ba48-6f5f0aca9d1f
📒 Files selected for processing (6)
apps/public-api/src/__tests__/storage.controller.test.jsapps/public-api/src/app.jsapps/public-api/src/controllers/data.controller.jsapps/public-api/src/controllers/schema.controller.jsapps/public-api/src/controllers/storage.controller.jsapps/public-api/src/controllers/userAuth.controller.js
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/public-api/src/controllers/data.controller.js
- apps/public-api/src/controllers/storage.controller.js
- apps/public-api/src/tests/storage.controller.test.js
- apps/public-api/src/controllers/userAuth.controller.js
Contributor
Author
|
@yash-pouranik , |
Member
|
public api ka to hogya tha na pahle? |
Contributor
Author
Those were middlewares and this one have controller |
Member
|
ooh then okay. |
Member
|
does this needs any updation in the sdk or react sdk? |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Find SDK-related packages/directories
fd -t d -d 3 'sdk' --ignore-case
echo "---"
# Check package.json files for SDK-related packages
fd 'package.json' -d 3 --exec grep -l '"name"' {} \; | xargs -I{} sh -c 'echo "=== {} ==="; cat {}'
echo "---"
# Look for any client-side fetch/axios calls that parse error responses
rg -n --type=js --type=ts -C3 'success.*false|\.error|\.message' --iglob '*sdk*' -g '!node_modules' | head -80Length of output: 15802 🏁 Script executed: #!/bin/bash
# Check how the TypeScript SDK parses API responses
echo "=== TypeScript SDK source files ==="
fd -t f -e ts --search-path sdks/urbackend-sdk/src | head -30
echo ""
echo "=== TypeScript SDK - response parsing logic ==="
rg -n --type=ts -C4 '\.error|\.message|\.success|\.data' sdks/urbackend-sdk/src/ | head -100
echo ""
echo "=== TypeScript SDK - error handling ==="
rg -n --type=ts -C4 'throw|Error|status' sdks/urbackend-sdk/src/ | head -80
echo ""
echo "=== Python SDK - response parsing ==="
fd -t f -e py --search-path sdks/urbackend-python/ | head -10
rg -n -C4 '\.error|\.message|\.success|\["error"\]|\["message"\]|\["success"\]' sdks/urbackend-python/ | head -80Length of output: 20825 🏁 Script executed: #!/bin/bash
echo "=== Full TypeScript SDK client.ts ==="
cat sdks/urbackend-sdk/src/client.ts
echo ""
echo "=== Full TypeScript SDK errors.ts ==="
cat sdks/urbackend-sdk/src/errors.ts
echo ""
echo "=== Python SDK http.py ==="
cat sdks/urbackend-python/src/urbackend/http.pyLength of output: 16134
TypeScript SDK (
|
Member
|
and in python sdk?? |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
No changes needed in the Python SDK. The error parsing in message = (
data.get("error") # previously the primary field — now absent/None
or data.get("message") # fallback — always present in the new standard ✅
or message
)The Only thing worth a follow-up: there's a stale comment in that file: # standardizeApiResponse puts the message in "error" for 4xx/5xx;
# some controllers also use "message" directly.This described the old inconsistent behavior. With this PR, |
yash-pouranik
approved these changes
Jun 18, 2026
Merged
17 tasks
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 public 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
{ success: true, data: ... }for successful requests and{ success: false, message, data: ... }for failures.datawhen applicable).