-
Notifications
You must be signed in to change notification settings - Fork 2
feat: error handler middleware #234
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 26 out of 26 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
packages/wallet-service/src/api/middlewares/errorHandler.ts:26
- Consider adding unit tests for the errorHandler middleware to verify that it correctly returns a generic error message in production and a detailed error message otherwise.
if (process.env.NODE_ENV === 'production') {
// Initialize response with default values if it hasn't been done yet. | ||
request.response = request.response ?? {...defaultResponse}; | ||
// Force the status code to 500 since this is an unhandled error | ||
request.response.statusCode = 500; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about using the STATUS_CODE_TABLE
from api/utils
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Above as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
@@ -1169,3 +1171,20 @@ describe('getWalletBalancesForTx', () => { | |||
}); | |||
}); | |||
}); | |||
|
|||
test('getTokenListFromInputsAndOutputs', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this test being added in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To increase coverage since the global coverage was failing
@@ -103,6 +103,7 @@ export const sendMessageToClient = async ( | |||
} | |||
}; | |||
|
|||
/* istanbul ignore next */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
This is apparently being used in wsAdminDisconnect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
…-lib-v2.1.1 * origin/master: feat: error handler middleware (#234)
Motivation
middy
does not have a default error handler, so unhandled errors would just return 200 with an empty body.Acceptance Criteria
Checklist
master
, confirm this code is production-ready and can be included in future releases as soon as it gets merged