fix error display modal #632
Conversation
WalkthroughThe pull request introduces changes to the error handling logic within the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (2)
client/components/open/forms/components/form-components/FormErrorModal.vue (2)
12-12: Consider adding a default prop value for validationErrorResponseThe error display logic looks good, but could be more robust with a default value for the validationErrorResponse prop.
Add a default value to prevent potential undefined errors:
props: { show: { type: Boolean, required: true }, - validationErrorResponse: { type: Object, required: false }, + validationErrorResponse: { + type: Object, + required: false, + default: () => ({ message: '', errors: {} }) + }, },Also applies to: 16-17, 21-21
Line range hint
1-52: Architectural feedback on error handling approachThe changes improve error handling by making it more explicit and focused. However, consider these additional improvements for robust error handling:
- Add error type differentiation (validation vs server errors)
- Include error logging for non-validation errors
- Consider adding retry capability for transient server errors
Would you like me to provide implementation examples for any of these suggestions?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
client/components/open/forms/components/FormEditor.vue(2 hunks)client/components/open/forms/components/form-components/FormErrorModal.vue(2 hunks)
🔇 Additional comments (3)
client/components/open/forms/components/form-components/FormErrorModal.vue (2)
7-7: LGTM! Improved error message visibility
The darker red shade (text-red-600) provides better contrast and readability for the error message header.
44-44: Verify the prop name change across the codebase
The component changes look good. The explicit component name and more descriptive prop name improve code clarity.
Let's verify all usages of this component have been updated:
Also applies to: 47-47
✅ Verification successful
All usages of FormErrorModal have been updated correctly
The verification shows that the only usage of FormErrorModal component is in FormEditor.vue, and it's already using the new prop name validation-error-response instead of the old form prop. No further changes are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any instances where FormErrorModal is used with the old 'form' prop
rg -l 'FormErrorModal.*form=' client/
# Search for all FormErrorModal usage to ensure we haven't missed any
rg -A 5 '<FormErrorModal' client/
Length of output: 575
client/components/open/forms/components/FormEditor.vue (1)
Line range hint 262-315: Verify error handling changes across the application
The change from error.response.data to error.data might affect other components that expect the old error structure.
Let's verify the error handling pattern across the codebase:
| @@ -259,7 +259,7 @@ export default { | |||
| }) | |||
| .catch((error) => { | |||
| if (error?.response?.status === 422) { | |||
| this.validationErrorResponse = error.response.data | |||
| this.validationErrorResponse = error.data | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add defensive error handling
The direct access to error.data might cause issues if the error object structure is different than expected.
Consider adding defensive checks:
- this.validationErrorResponse = error.data
+ this.validationErrorResponse = error?.data || { message: 'Validation failed', errors: {} }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.validationErrorResponse = error.data | |
| this.validationErrorResponse = error?.data || { message: 'Validation failed', errors: {} } |
| @@ -306,7 +306,7 @@ | |||
| }) | |||
| .catch((error) => { | |||
| if (error?.response?.status === 422) { | |||
| this.validationErrorResponse = error.response | |||
| this.validationErrorResponse = error.data | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
DRY: Extract duplicated error handling logic
The error handling logic is duplicated between saveFormEdit and saveFormCreate methods.
Consider extracting the common error handling logic into a separate method:
+ handleFormError(error) {
+ if (error?.response?.status === 422) {
+ this.validationErrorResponse = error?.data || { message: 'Validation failed', errors: {} }
+ this.showValidationErrors()
+ } else {
+ useAlert().error(
+ "An error occurred while saving the form, please try again.",
+ )
+ captureException(error)
+ }
+ }
// Then in both methods:
- if (error?.response?.status === 422) {
- this.validationErrorResponse = error.data
- this.showValidationErrors()
- } else {
- useAlert().error(
- "An error occurred while saving the form, please try again.",
- )
- captureException(error)
- }
+ this.handleFormError(error)Committable suggestion skipped: line range outside the PR's diff.
Fixed #629
Summary by CodeRabbit
Bug Fixes
Style
Refactor
formtovalidationErrorResponsein the FormErrorModal component for more accurate error reporting.