Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds a devDependency, introduces a StatusError typedef and enhanced error-to-status resolution, updates tests to cover new mappings, adjusts Express callback to return a minimal fallback error payload when details are disabled, and applies a minor indentation change in the router. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor C as Client
participant E as Express App
participant CB as makeExpressCallback
participant Cn as Controller
participant L as Logger
C->>E: HTTP Request
E->>CB: Invoke callback(req,res)
CB->>Cn: controller(httpRequest)
alt Success
Cn-->>CB: { statusCode, body, headers }
CB->>E: res.status(statusCode).json(body)
else Error thrown
Cn-->>CB: throw error
CB->>CB: map status via error.status or code mapping
CB->>L: log error (level by status)
alt errorDetails = true
CB->>E: res.status(status).json(detailed error)
else errorDetails = false
CB->>E: res.status(status).json({ status, timestamp, message })
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
✨ Finishing touches🧪 Generate unit tests
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 |
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the error handling system to support custom error objects with a status property, providing more flexibility in returning appropriate HTTP status codes. The change allows errors to specify their own status codes via a status attribute, falling back to existing native error mappings when the status is not present.
Key changes:
- Modified error status resolution to check for error.status property first
- Updated parameter parsing to support mock mode behavior
- Added comprehensive test coverage for the new error status feature
Reviewed Changes
Copilot reviewed 3 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/error-status.js | Updated to check error.status property before falling back to type-based mapping |
| src/error-status.test.js | Added test case for ValidationError with status 400 |
| src/params.js | Enhanced parameter parsing with mock mode support and improved logic flow |
| src/params.test.js | Extended test coverage for mock mode scenarios |
| src/express-callback.js | Added performance timing and logging callbacks |
| src/api.js | Added ETag caching for API docs and removed secret parameter dependency |
| src/router.js | Refactored to support custom response validation and removed secret handling |
| src/server.js | Added body size limits and general code style improvements |
| Multiple test files | Updated to use consistent code style and remove semicolons |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 6 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/error-status.test.js (1)
23-26: Use string ctor for ValidationError to avoid signature drift.Safer across versions of @trojs/error; avoids relying on object-arg shape.
- error: new ValidationError({ message: 'test' }), + error: new ValidationError('test'),src/error-status.js (2)
21-23: Fix JSDoc phrasing to reflect behavior.- * Get a http status when you send an error. - * When it is a error, throw back the error. + * Resolve an HTTP status code for an error. + * Falls back to 500 when no match is found.
27-29: Prefer nullish and type-checked status to avoid truthiness pitfalls.Ensures numeric status is honored even if 0 (not used today) and prevents string leakage.
-export default (error) => - error.status - || errorCodesStatus.find((errorCode) => error instanceof errorCode.type)?.status - || 500 +export default (error) => + (typeof error.status === 'number' ? error.status : undefined) + ?? errorCodesStatus.find((errorCode) => error instanceof errorCode.type)?.status + ?? 500src/express-callback.js (2)
88-94: Trim or sanitize error payload in detailed mode.Serializing error.valueOf() is noisy and may expose unintended fields. Consider limiting to safe fields and optionally including cause.
errors: [ { message: error.message, - value: error.valueOf(), - type: error.constructor.name + type: error.constructor.name, + // optional: + // stack: error.stack, + // cause: error.cause } ],
35-75: Consider splitting handler to satisfy lint rule and improve readability.Static analysis flags 22 statements (>20). Extract helpers (e.g., buildFeedback, resolveParameters, logPost) to reduce complexity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
package.json(1 hunks)src/error-status.js(2 hunks)src/error-status.test.js(2 hunks)src/express-callback.js(2 hunks)src/router.js(1 hunks)
🧰 Additional context used
🪛 GitHub Check: build
src/express-callback.js
[warning] 35-35:
Async arrow function has too many statements (22). Maximum allowed is 20
🔇 Additional comments (4)
src/router.js (1)
93-97: LGTM: style-only change.The newline assignment style matches the rest of the module; no behavioral impact.
package.json (1)
36-42: Dev-only dependency addition looks correct.Adding @trojs/error under devDependencies aligns with tests-only usage; no runtime impact.
src/error-status.test.js (1)
28-30: Good coverage for fallback to 500.SyntaxError exercising the default path is helpful.
src/error-status.js (1)
1-3: Nice: explicit StatusError typedef.Documents the new contract clearly.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 6 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 6 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Fall back to http status 500 if error not found Write a test for the syntax error Test the edge case if it not based on a native error object
w3nl
force-pushed
the
feature/validation-error
branch
from
782fb2e to
786ecf3
Compare
See e.g. https://github.com/trojs/error/blob/main/src/validation-error.js that has the status 400.
If you throw this error, it will return a http status 400.
With this feature you are more flexible to work with any custom error with a status.
Not depending on something else like @trojs/error, the error only needs a status attribute, or else it falls back to the existing statuses for native error objects.
Summary by CodeRabbit