feat: add reusable error handling utilities and integrate into user management components #141
Conversation
…anagement components
There was a problem hiding this comment.
Pull Request Overview
This PR introduces reusable error handling utilities to standardize error display across the CCF UI application. The changes provide two complementary approaches: a formatting utility for direct error handling and a composable for automatic error notifications.
Key changes:
- Created formatAxiosError utility for consistent error message formatting with HTTP status codes
- Built useErrorToast composable for automatic error toast notifications
- Integrated both utilities into user management components
- Added comprehensive documentation with usage guidelines
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/error-formatting.ts | Core utility that formats AxiosError objects into user-friendly messages with status code mapping |
| src/composables/useErrorToast.ts | Vue composable that watches error refs and automatically displays toast notifications |
| src/views/users/UsersListView.vue | Integrates useErrorToast for automatic error handling on user list loading |
| src/views/users/UserView.vue | Replaces manual error handling with formatAxiosError utility |
| src/components/users/UserEditForm.vue | Updates error handling to use formatAxiosError for consistency |
| src/components/users/UserCreateForm.vue | Standardizes error formatting using formatAxiosError utility |
| docs/error-handling-guidelines.md | Comprehensive documentation covering usage patterns and best practices |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/views/users/UsersListView.vue
Outdated
| // Automatically show toast when error occurs | ||
| useErrorToast(error, { | ||
| summary: 'Error loading users', |
There was a problem hiding this comment.
[nitpick] The summary message formatting is inconsistent with other components. Other components use template literals to include status codes (e.g., Error loading user: ${formattedError.summary}), while this uses the composable's automatic formatting. Consider whether this difference in approach is intentional for maintaining consistency across the codebase.
| summary: 'Error loading users', | |
| summary: `Error loading users${error.value?.summary ? ': ' + error.value.summary : ''}`, |
| detail: | ||
| errorResponse.response?.data.errors.body || | ||
| 'An error occurred while loading the user data.', | ||
| summary: `Error loading user${formattedError.statusCode ? `: ${formattedError.summary}` : ''}`, |
There was a problem hiding this comment.
[nitpick] The summary template logic is duplicated across multiple toast calls. Consider extracting this into a helper function or using the useErrorToast composable with autoShow: false for consistency, as this pattern is repeated throughout the file.
| detail: | ||
| errorResponse.response?.data.errors.body || | ||
| 'An error occurred while updating the user lock status.', | ||
| summary: `Error ${newIsLocked ? 'locking' : 'unlocking'} user${formattedError.statusCode ? `: ${formattedError.summary}` : ''}`, |
There was a problem hiding this comment.
[nitpick] The summary template logic is duplicated across multiple toast calls. Consider extracting this into a helper function or using the useErrorToast composable with autoShow: false for consistency, as this pattern is repeated throughout the file.
| detail: | ||
| errorResponse.response?.data.errors.body || | ||
| 'An error occurred while deleting the user.', | ||
| summary: `Error deleting user${formattedError.statusCode ? `: ${formattedError.summary}` : ''}`, |
There was a problem hiding this comment.
[nitpick] The summary template logic is duplicated across multiple toast calls. Consider extracting this into a helper function or using the useErrorToast composable with autoShow: false for consistency, as this pattern is repeated throughout the file.
| // Try API error body (already checked in formatAxiosError, but kept for completeness) | ||
| if (error.response?.data?.errors?.body) { | ||
| return error.response.data.errors.body; | ||
| } | ||
|
|
There was a problem hiding this comment.
This check is redundant since formatAxiosError already extracts apiErrorMessage from the same path and only calls getErrorDetail when it's falsy. The comment acknowledges this redundancy. Consider removing this check to avoid confusion and simplify the logic.
| // Try API error body (already checked in formatAxiosError, but kept for completeness) | |
| if (error.response?.data?.errors?.body) { | |
| return error.response.data.errors.body; | |
| } |
|
|
||
| // Build summary with optional prefix | ||
| const toastSummary = summary | ||
| ? `${summary}: ${formatted.summary}` |
There was a problem hiding this comment.
The summary formatting logic differs from the manual implementations in other components. The manual implementations check for statusCode before including the formatted summary, while this always includes it. This inconsistency could lead to different behavior between automatic and manual error handling.
| ? `${summary}: ${formatted.summary}` | |
| ? (formatted.summary ? `${summary}: ${formatted.summary}` : summary) |
| // Automatically watch the error ref and show toast when error occurs | ||
| watch(errorRef, (error) => { | ||
| if (error && autoShow) { | ||
| showErrorToast(error); | ||
| } | ||
| }); |
There was a problem hiding this comment.
I feel iffy about the implicit nature of handling the errors with a watch. I am aware there aren't really any performance issues with watch anymore, but my concern is more edge casing, for example retrying and getting the same error will not spawn the same toast.
i'm a fan of explicit error handling and bringing that to the front.
additionally I am not really a fan of all of the toast messages everywhere. It's quite noisy. This PR doesn't introduce toasts so that isn't really the problem, but rather than this solidifies toasts as a reasonable way to deal with errors, where I don't really think that's the case.
You've provided a way to explicitly deal with errors which is great and this does reduce noise for toast-catered errors. I do however still feel a bit iffy.
Before continuing, maybe we should have a live session where we can discuss ?
There was a problem hiding this comment.
I am not a fan of toasts and watches either, but the original ticket stated:
"Ensuring that all errors are handled in toasts, and potentially outputted in the UI to be consistent, will be good."
That's why I focused more on the toast, but with a few simple changes, we can also support manual handling of notifications and dialogs.
There was a problem hiding this comment.
@chris-cmsoft Can you take a look one more time? I relaxed the toast usage—it's optional now—and added more generic dialog-related functions to handle and show application-level errors.
# Conflicts: # src/views/users/UsersListView.vue
No description provided.