Standarized the API Response in Dashboard API-Middleware#299
Merged
yash-pouranik merged 4 commits intoJun 11, 2026
Conversation
Ayush4958
requested review from
Nitin-kumar-yadav1307 and
yash-pouranik
as code owners
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughFive dashboard-api middleware files and their tests were updated to import ChangesAppError Adoption Across Dashboard Middlewares
Sequence Diagram(s)sequenceDiagram
participant Client
participant authLimiter
participant authMiddleware
participant jwt
participant ExpressErrorHandler
Client->>authLimiter: incoming request (rate-limited)
authLimiter-->>ExpressErrorHandler: next(new AppError(429, "Too many requests")) (on breach)
Client->>authMiddleware: request with Authorization/cookie
authMiddleware->>jwt: jwt.verify(token)
jwt-->>authMiddleware: throws (expired/invalid) or returns payload
alt jwt throws
authMiddleware->>ExpressErrorHandler: next(new AppError(401, "Invalid Token"))
else valid token
authMiddleware->>authMiddleware: attach user -> next()
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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: 3
🧹 Nitpick comments (2)
apps/dashboard-api/src/__tests__/authMiddleware.test.js (1)
163-165: ⚡ Quick winUpdate expected statusCode when middleware is corrected.
These tests assert
statusCode400 for JWT verification failures, matching the current implementation. When the middleware is updated to use 401 for authentication failures (as suggested in the middleware review), update these assertions to expect 401.📝 Future update needed
- expect(next.mock.calls[0][0].statusCode).toBe(400); + expect(next.mock.calls[0][0].statusCode).toBe(401);Also applies to: 178-180
🤖 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/__tests__/authMiddleware.test.js` around lines 163 - 165, Update the test assertions that currently expect a 400 statusCode for JWT verification failures to expect 401 instead: change the assertions that inspect next.mock.calls[0][0].statusCode (and the identical assertions at the other block) so they assert 401 when the middleware returns an AppError for authentication failure; leave the AppError and message checks unchanged.apps/dashboard-api/src/__tests__/auth_limiter.test.js (1)
80-80: 💤 Low valueConsider asserting the full response shape.
The assertion now only checks
blocked.body.messageinstead of the full response object. While this is sufficient for verifying the rate-limit behavior, asserting the complete response shape (includingsuccess: falseanderrorfields) would provide stronger validation of the error handler's output format.🔍 Optional enhancement
+ expect(blocked.body).toEqual({ + success: false, + error: 'Error', + message: 'Too many attempts. Please try again in 15 minutes.' + }); - expect(blocked.body.message).toBe('Too many attempts. Please try again in 15 minutes.');🤖 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/__tests__/auth_limiter.test.js` at line 80, Update the assertion in the auth limiter test so it checks the entire response body shape returned when rate-limited (the `blocked` response), not just `blocked.body.message`; assert that `blocked.body` includes `success: false`, the `message: 'Too many attempts. Please try again in 15 minutes.'`, and an `error` field (or appropriate error details) to validate the error handler output format in this test (look for the `blocked` variable in apps/dashboard-api/src/__tests__/auth_limiter.test.js).
🤖 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/middlewares/authMiddleware.js`:
- Line 39: Replace the 400 status used for JWT verification failures with 401 in
the auth middleware: locate the return new AppError(...) call in authMiddleware
(the line that currently does return next(new AppError(400, 'Invalid Token'))),
and change the status code argument from 400 to 401 so token verification errors
produce HTTP 401 Unauthorized while keeping the existing error message and flow.
- Line 2: Locate the auth middleware where JWT verification uses the imported
AppError (symbol: AppError) and replace any instantiation that returns new
AppError(400, 'Invalid Token') with a 401 status (e.g., new AppError(401,
'Invalid Token') or new AppError(401, 'Unauthorized')) so JWT verification
failures return the correct HTTP Unauthorized status; ensure the same change is
applied for expired or otherwise invalid credential errors in the middleware
(look for functions handling token verification/verification callbacks).
In `@apps/dashboard-api/src/middlewares/loadProjectForAdmin.js`:
- Around line 5-20: Add unit tests for the loadProjectForAdmin middleware to
cover the three behaviors: when req.params.projectId is missing it should call
next with an AppError(400); when Project.findOne returns null it should call
next with an AppError(404); and when Project.findOne returns a project it should
set req.project and call next without an error. In tests stub/mock the Project
model's findOne to return null or a project, provide fake req/res objects and a
spy for next, and assert next was called with the expected AppError instance for
the error cases and that req.project equals the returned project for the success
case; also restore mocks between tests.
---
Nitpick comments:
In `@apps/dashboard-api/src/__tests__/auth_limiter.test.js`:
- Line 80: Update the assertion in the auth limiter test so it checks the entire
response body shape returned when rate-limited (the `blocked` response), not
just `blocked.body.message`; assert that `blocked.body` includes `success:
false`, the `message: 'Too many attempts. Please try again in 15 minutes.'`, and
an `error` field (or appropriate error details) to validate the error handler
output format in this test (look for the `blocked` variable in
apps/dashboard-api/src/__tests__/auth_limiter.test.js).
In `@apps/dashboard-api/src/__tests__/authMiddleware.test.js`:
- Around line 163-165: Update the test assertions that currently expect a 400
statusCode for JWT verification failures to expect 401 instead: change the
assertions that inspect next.mock.calls[0][0].statusCode (and the identical
assertions at the other block) so they assert 401 when the middleware returns an
AppError for authentication failure; leave the AppError and message checks
unchanged.
🪄 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: a43b248f-f47b-4297-a96d-1e30303a78d1
📒 Files selected for processing (5)
apps/dashboard-api/src/__tests__/authMiddleware.test.jsapps/dashboard-api/src/__tests__/auth_limiter.test.jsapps/dashboard-api/src/middlewares/authMiddleware.jsapps/dashboard-api/src/middlewares/auth_limiter.jsapps/dashboard-api/src/middlewares/loadProjectForAdmin.js
…n and authMiddleware
…initialization crash
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/dashboard-api/src/middlewares/authMiddleware.js (1)
34-39:⚠️ Potential issue | 🟠 MajorMap only JWT validation errors to
401 Invalid Token
The catch block currently converts everyjwt.verifyfailure into a 401, masking operational/config issues (e.g., missing/invalidJWT_SECRET, which typically won’t be one of jsonwebtoken’s token-validation error names). Restrict 401 tojsonwebtokenvalidation/expiration errors (JsonWebTokenError,TokenExpiredError,NotBeforeError) and forward everything else withnext(err).🔧 Proposed fix
} catch (err) { if (process.env.NODE_ENV !== 'test') { console.error(err); } - return next(new AppError(401, 'Invalid Token')); + if ( + err?.name === 'JsonWebTokenError' || + err?.name === 'TokenExpiredError' || + err?.name === 'NotBeforeError' + ) { + return next(new AppError(401, 'Invalid Token')); + } + return next(err); } };🤖 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/middlewares/authMiddleware.js` around lines 34 - 39, The catch block in your auth middleware is turning every error into a 401; update the catch in the function that calls jwt.verify (the auth middleware around jwt.verify) to only map jsonwebtoken validation errors to a new AppError(401, 'Invalid Token') by checking err.name against 'JsonWebTokenError', 'TokenExpiredError', and 'NotBeforeError'; for any other errors (e.g., missing/invalid JWT_SECRET or unexpected runtime errors) call next(err) to propagate the original error instead (you may keep the existing non-test console.error line for logging).
🧹 Nitpick comments (1)
apps/dashboard-api/src/__tests__/loadProjectForAdmin.test.js (1)
45-70: ⚡ Quick winAdd coverage for the
catch -> next(err)branch in the middleware.This suite validates three primary paths, but it does not assert behavior when
Project.findOnerejects. A small test here prevents regressions in error forwarding.➕ Suggested test case
it('should set req.project and call next without error if project is found', async () => { req.params.projectId = 'proj123'; const mockProject = { _id: 'proj123', name: 'Test Project' }; Project.findOne.mockResolvedValueOnce(mockProject); await loadProjectForAdmin(req, res, next); expect(Project.findOne).toHaveBeenCalledWith({ _id: 'proj123', owner: 'user123' }); expect(req.project).toEqual(mockProject); expect(next).toHaveBeenCalledTimes(1); expect(next).toHaveBeenCalledWith(); }); + + it('should forward unexpected errors via next(err)', async () => { + req.params.projectId = 'proj123'; + const dbError = new Error('db failure'); + Project.findOne.mockRejectedValueOnce(dbError); + + await loadProjectForAdmin(req, res, next); + + expect(next).toHaveBeenCalledTimes(1); + expect(next).toHaveBeenCalledWith(dbError); + }); });🤖 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/__tests__/loadProjectForAdmin.test.js` around lines 45 - 70, Add a test that covers the error branch by making Project.findOne reject (e.g., mockRejectedValueOnce(new Error('db'))), call loadProjectForAdmin(req, res, next), and assert that next was called once with the original error (expect(next).toHaveBeenCalledWith(expect.any(Error))) and that req.project is not set; reference the loadProjectForAdmin middleware, Project.findOne mock, and the next callback when adding this case.
🤖 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/__tests__/loadProjectForAdmin.test.js`:
- Around line 3-4: Move the jest.mock('`@urbackend/common`') call so it runs
before any require of '`@urbackend/common`' or the middleware; specifically, place
jest.mock('`@urbackend/common`') at the top of the test file before you require
Project or loadProjectForAdmin, then require Project and loadProjectForAdmin
after the mock is declared (optionally use jest.resetModules() in beforeEach if
tests need a clean module cache). This ensures the mock for '`@urbackend/common`'
is active when loadProjectForAdmin (which imports that module at module load
time) is required.
---
Outside diff comments:
In `@apps/dashboard-api/src/middlewares/authMiddleware.js`:
- Around line 34-39: The catch block in your auth middleware is turning every
error into a 401; update the catch in the function that calls jwt.verify (the
auth middleware around jwt.verify) to only map jsonwebtoken validation errors to
a new AppError(401, 'Invalid Token') by checking err.name against
'JsonWebTokenError', 'TokenExpiredError', and 'NotBeforeError'; for any other
errors (e.g., missing/invalid JWT_SECRET or unexpected runtime errors) call
next(err) to propagate the original error instead (you may keep the existing
non-test console.error line for logging).
---
Nitpick comments:
In `@apps/dashboard-api/src/__tests__/loadProjectForAdmin.test.js`:
- Around line 45-70: Add a test that covers the error branch by making
Project.findOne reject (e.g., mockRejectedValueOnce(new Error('db'))), call
loadProjectForAdmin(req, res, next), and assert that next was called once with
the original error (expect(next).toHaveBeenCalledWith(expect.any(Error))) and
that req.project is not set; reference the loadProjectForAdmin middleware,
Project.findOne mock, and the next callback when adding this case.
🪄 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: 540cb9d0-347c-45be-bca5-533236d7519d
📒 Files selected for processing (4)
apps/dashboard-api/src/__tests__/authMiddleware.test.jsapps/dashboard-api/src/__tests__/loadProjectForAdmin.test.jsapps/dashboard-api/src/middlewares/authMiddleware.jsapps/dashboard-api/src/middlewares/loadProjectForAdmin.js
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/dashboard-api/src/middlewares/loadProjectForAdmin.js
- apps/dashboard-api/src/tests/authMiddleware.test.js
Contributor
Author
|
@yash-pouranik , |
Collaborator
|
one coderabbitai |
Contributor
Author
|
@yash-pouranik , It's fixed but |
yash-pouranik
approved these changes
Jun 11, 2026
yash-pouranik
left a comment
yash-pouranik
left a comment
Collaborator
There was a problem hiding this comment.
cool now everything looks perfect.
Contributor
Author
|
@yash-pouranik |
Collaborator
|
yh |
yash-pouranik
added
GSSOC'26
quality:exceptional
type:refactor
gssoc:approved
level:advanced
enhancement
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 API 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.📸 Screenshots / Recordings (Optional)
✅ Checklist
Summary by CodeRabbit