-
Notifications
You must be signed in to change notification settings - Fork 514
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
Replace notification with toast #9833
base: develop
Are you sure you want to change the base?
Replace notification with toast #9833
Conversation
WalkthroughThis pull request introduces a comprehensive migration from a custom notification utility to the Sonner toast notification library across multiple components in the application. The changes involve replacing existing notification methods with Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Finishing Touches
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 (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
👋 Hi, @Mahendar0701, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
@Mahendar0701 I'd recommend getting this resolved and merged asap since its changing a lot of files and would easily catch merge conflicts |
Hey @rithviknishad how to deal with error msg when recieved as object, we cant pass it to toast. Is there any existing implementation which i can use. Edit : error msg when recieved as array |
What's the context for this? If it's for error handler for useQuery/useMutate, that's being changed (handlePydanticErrors) in #9708 |
@Mahendar0701 There seems to be only 3 places thats using Feel free to get the rest of the things migrated and merged, and we can handle the removal of BadRequest as a follow up PR once that PR is merged too. Edit: We might need to dig deeper on handling structured responses properly in a different approach. In some places errors from backend needs to be silenced if it's a field error and set those errors in the form's state, and if there are other errors we need to rethrow for the global handler to do it's thing. This might take time as we still need to figure out a clean solution for it. |
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.
Actionable comments posted: 9
🔭 Outside diff range comments (3)
src/Utils/request/handleResponse.ts (1)
Line range hint
23-26
: Replace deprecated BadRequest with notifyErrorThe code is still using the deprecated
BadRequest
function. This should be updated to usenotifyError
for consistency.- notify?.BadRequest({ errs: error }); + notify?.notifyError(error);src/Utils/request/uploadFile.ts (1)
Line range hint
27-33
: Replace deprecated BadRequest with toast.errorFor consistency with the network error handling, replace
BadRequest
with direct usage oftoast.error
.- Notification.BadRequest({ errs: error.errors }); + toast.error(error.errors);src/components/Users/UserSoftwareUpdate.tsx (1)
Line range hint
19-32
: Add error handling for update checkThe
checkUpdates
function should handle potential errors fromcheckForUpdate
.const checkUpdates = async () => { setUpdateStatus({ ...updateStatus, isChecking: true }); await new Promise((resolve) => setTimeout(resolve, 500)); - if ((await checkForUpdate()) != null) { - setUpdateStatus({ - isUpdateAvailable: true, - isChecking: false, - }); - } else { - setUpdateStatus({ - isUpdateAvailable: false, - isChecking: false, - }); - toast.success("No update available"); + try { + const update = await checkForUpdate(); + setUpdateStatus({ + isUpdateAvailable: update != null, + isChecking: false, + }); + if (!update) { + toast.success("You're running the latest version"); + } + } catch (error) { + setUpdateStatus({ + isUpdateAvailable: false, + isChecking: false, + }); + toast.error("Failed to check for updates. Please try again later."); } };
🧹 Nitpick comments (24)
src/Utils/Notifications.ts (2)
22-22
: Consider using a more specific type for error parameterThe
any
type for the error parameter reduces type safety. Consider defining a specific error interface.-export const notifyError = (error: any) => { +interface ErrorResponse { + detail?: string; + [key: string]: unknown; +} +export const notifyError = (error: ErrorResponse | string | null) => {
Line range hint
22-41
: Improve error message formatting for toast notificationsThe current implementation concatenates multiple error messages with newlines (
\n
). Toast notifications typically render better with HTML formatting.} else { + const errors: string[] = []; for (const [key, value] of Object.entries(error)) { const keyName = formatKey(key); if (Array.isArray(value)) { const uniques = [...new Set(value)]; - errorMsg += `${keyName} - ${uniques.splice(0, 5).join(", ")}`; + errors.push(`${keyName} - ${uniques.splice(0, 5).join(", ")}`); } else if (typeof value === "string") { - errorMsg += `${keyName} - ${value}`; + errors.push(`${keyName} - ${value}`); } else { - errorMsg += `${keyName} - Bad Request`; + errors.push(`${keyName} - Bad Request`); } - errorMsg += "\n"; } + errorMsg = errors.join("<br>"); }src/Utils/request/handleResponse.ts (1)
18-18
: Standardize error message formatThe error messages have inconsistent formats:
- 404 case:
{ msg: "Not Found" }
- Other errors:
{ msg: error?.detail || "Something went wrong...!" }
Consider standardizing to:
- notify?.notifyError({ msg: "Not Found" }); + notify?.notifyError("Not Found"); - notify?.notifyError({ msg: error?.detail || "Something went wrong...!" }); + notify?.notifyError(error?.detail || "Something went wrong...!");Also applies to: 42-42
src/Utils/request/uploadFile.ts (1)
48-50
: Consider using a more descriptive error messageThe current network error message could be more helpful to users.
- toast.error("Network Failure. Please check your internet connectivity."); + toast.error("Unable to upload file. Please check your internet connection and try again.");src/components/Users/UserSoftwareUpdate.tsx (1)
31-31
: Enhance success message with version informationThe current success message could be more informative by including the current version.
- toast.success("No update available"); + toast.success("You're running the latest version");src/Utils/request/errorHandler.ts (1)
43-45
: Consider migrating to toast.error for consistency.While the current implementation works, consider migrating this to
toast.error
for consistency with other error notifications in the file.- Notifications.notifyError({ - msg: cause?.detail || "Something went wrong...!", - }); + toast.error(cause?.detail || "Something went wrong...!");src/components/Resource/ResourceCommentSection.tsx (1)
34-34
: Consider adding more context to the success message.The success message could be more descriptive to provide better feedback.
- toast.success("Comment added successfully"); + toast.success("Your comment has been posted successfully");src/components/Users/UserAvatar.tsx (1)
77-77
: Use i18n translation for consistency.The success message is hardcoded. Consider using the translation system for consistency with other messages in the application.
- toast.success("Profile picture deleted"); + toast.success(t("profile_picture_deleted"));src/Utils/useSegmentedRecorder.ts (1)
Line range hint
102-107
: Consider using toast for error handling in startRecording.The function throws an error but doesn't notify the user. Consider using toast.error for consistency with other error handling in the hook.
} catch (error) { setMicrophoneAccess(false); + toast.error(t("audio__permission_message")); throw new Error("Microphone access denied"); }
src/components/Users/UserHome.tsx (1)
55-57
: Improve error message handling.The current implementation might display "Error while fetching user details: undefined". Consider using nullish coalescing for a better fallback message.
- toast.error( - "Error while fetching user details: " + (error?.message || ""), - ); + toast.error( + `Error while fetching user details: ${error?.message ?? "Unknown error"}`, + );src/pages/Facility/FacilityDetailsPage.tsx (1)
4-4
: Enhance error message with more contextThe generic error message "Error while fetching users data" could be more helpful by including specific details about what went wrong. Consider including the facility name or ID in the message.
-toast.error("Error while fetching users data"); +toast.error(`Error while fetching users data for facility ${id}`);Also applies to: 56-56
src/components/Auth/ResetPassword.tsx (1)
4-4
: LGTM! Consider clearing form data after successThe implementation correctly handles the success notification and token cleanup. For additional security, consider clearing the form data before navigation.
if (res?.ok) { localStorage.removeItem(LocalStorageKeys.accessToken); + setForm(initForm); // Clear sensitive data toast.success(t("password_reset_success")); navigate("/login"); }
Also applies to: 85-85
src/components/Users/UserSummary.tsx (1)
57-62
: Consider structured error handling for better user experienceThe error message concatenation could be improved to handle structured error objects more gracefully.
- toast.error(t("user_delete_error") + ": " + (error || "")); + toast.error( + typeof error === "object" + ? `${t("user_delete_error")}: ${error.message || JSON.stringify(error)}` + : `${t("user_delete_error")}: ${error || ""}` + );src/components/Files/CameraCaptureDialog.tsx (1)
1-1
: Consider standardizing error handling patterns across componentsWhile the migration to toast notifications is generally well-implemented, there's an opportunity to standardize error handling patterns across components. Consider creating a shared utility function that handles common error scenarios (e.g., permission denials, API errors) consistently.
Example utility:
// src/Utils/toast.ts export const notifyError = (error: unknown, defaultMessage: string) => { if (typeof error === 'object' && error !== null) { toast.error((error as Error).message || defaultMessage); } else { toast.error(String(error) || defaultMessage); } };src/pages/Appoinments/PatientSelect.tsx (1)
83-83
: Consider using optional chaining for error handling.The success notification is well implemented. For the error case, consider using optional chaining for safer error message access.
-toast.error(error?.message || t("failed_to_create_appointment")); +toast.error(error?.message ?? t("failed_to_create_appointment"));Also applies to: 95-95
src/components/Resource/ResourceDetailsUpdate.tsx (1)
181-181
: Consider using translation key for success message.The success notification is implemented correctly, but the message is hardcoded. Consider using a translation key for consistency with the rest of the application.
-toast.success("Request updated successfully"); +toast.success(t("request_updated_successfully"));src/hooks/useFileUpload.tsx (1)
190-197
: File upload notifications look good, but callback can be improved.The success and error notifications are well implemented with appropriate error messages and translations. However, the onUpload callback can be improved using optional chaining.
-onUpload && onUpload(data); +onUpload?.(data);Also applies to: 204-204
🧰 Tools
🪛 Biome (1.9.4)
[error] 192-192: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/components/Facility/FacilityHome.tsx (1)
148-148
: Use translation keys for toast messages.For consistency, use translation keys for the cover image operation messages instead of hard-coded strings.
-toast.success("Cover image updated."); +toast.success(t("cover_image_updated")); -toast.success("Cover image deleted"); +toast.success(t("cover_image_deleted"));Also applies to: 166-166
src/components/Common/AvatarEditModal.tsx (1)
110-110
: Use translation key for the warning message.For consistency, use a translation key for the file type warning message instead of a hard-coded string.
-toast.warning("Please upload an image file!"); +toast.warning(t("please_upload_image_file"));src/components/Facility/CreateFacilityForm.tsx (1)
146-146
: Improve geolocation error message handling.Consider using translation keys for geolocation error messages and avoid string concatenation.
-toast.error("Unable to get location: " + error.message); +toast.error(t("unable_to_get_location", { error: error.message })); -toast.error("Geolocation is not supported by this browser"); +toast.error(t("geolocation_not_supported"));Also applies to: 151-151
src/hooks/useFileManager.tsx (1)
Line range hint
491-512
: Improve download error handling.The current error handling for file downloads is too generic. Consider:
- Adding specific error cases (network error, file not found, etc.)
- Logging errors for debugging
- Providing more informative error messages to users
try { if (!file.id) return; - toast.success("Downloading file..."); + toast.success(t("file_download_started")); const fileData = await retrieveUpload(file, associating_id); const response = await fetch(fileData?.read_signed_url || ""); - if (!response.ok) throw new Error("Network response was not ok."); + if (!response.ok) { + throw new Error(`HTTP error! status: ${response.status}`); + } // ... download logic ... + toast.success(t("file_download_completed")); } catch (err) { + console.error("Download failed:", err); + if (err instanceof TypeError && err.message === "Failed to fetch") { + toast.error(t("network_error")); + } else if (err instanceof Error && err.message.includes("HTTP error")) { + toast.error(t("file_not_found")); + } else { + toast.error(t("file_download_failed", { error: err.message })); + } - toast.error("Failed to download file"); }src/pages/Appoinments/PatientRegistration.tsx (2)
Line range hint
153-168
: Improve error handling in appointment creation.The error handling could be enhanced by providing more specific error messages to users.
- toast.error(error?.message || t("failed_to_create_appointment")); + const errorMessage = error?.cause?.detail || error?.message || t("failed_to_create_appointment"); + toast.error(errorMessage);
Line range hint
181-195
: Enhance error handling for array of errors.The current implementation only shows the first error message when multiple errors exist. Consider showing all error messages.
- toast.error(firstError.msg); + const errorMessages = errors.map(error => error.msg).join(", "); + toast.error(errorMessages);src/components/Patient/PatientRegistration.tsx (1)
151-155
: Consider adding specific error messages.The error notification could be more informative by including the specific error details.
- toast.error(t("patient_update_error")); + const errorMessage = error?.cause?.detail || t("patient_update_error"); + toast.error(errorMessage);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
public/locale/en.json
(1 hunks)src/Utils/Notifications.ts
(1 hunks)src/Utils/request/errorHandler.ts
(2 hunks)src/Utils/request/handleResponse.ts
(2 hunks)src/Utils/request/uploadFile.ts
(2 hunks)src/Utils/request/utils.ts
(2 hunks)src/Utils/useSegmentedRecorder.ts
(2 hunks)src/Utils/useVoiceRecorder.ts
(2 hunks)src/Utils/utils.ts
(2 hunks)src/components/Auth/Login.tsx
(1 hunks)src/components/Auth/ResetPassword.tsx
(2 hunks)src/components/Common/AvatarEditModal.tsx
(3 hunks)src/components/Common/DateInputV2.tsx
(4 hunks)src/components/Common/ExcelFIleDragAndDrop.tsx
(2 hunks)src/components/Facility/CreateFacilityForm.tsx
(4 hunks)src/components/Facility/FacilityCreate.tsx
(2 hunks)src/components/Facility/FacilityHome.tsx
(4 hunks)src/components/Files/AudioCaptureDialog.tsx
(2 hunks)src/components/Files/CameraCaptureDialog.tsx
(3 hunks)src/components/Form/Form.tsx
(2 hunks)src/components/Patient/PatientDetailsTab/Demography.tsx
(2 hunks)src/components/Patient/PatientRegistration.tsx
(5 hunks)src/components/Resource/ResourceCommentSection.tsx
(2 hunks)src/components/Resource/ResourceCreate.tsx
(2 hunks)src/components/Resource/ResourceDetailsUpdate.tsx
(2 hunks)src/components/Users/CreateUserForm.tsx
(2 hunks)src/components/Users/UserAvatar.tsx
(3 hunks)src/components/Users/UserFilter.tsx
(2 hunks)src/components/Users/UserHome.tsx
(2 hunks)src/components/Users/UserResetPassword.tsx
(2 hunks)src/components/Users/UserSoftwareUpdate.tsx
(2 hunks)src/components/Users/UserSummary.tsx
(2 hunks)src/hooks/useFileManager.tsx
(5 hunks)src/hooks/useFileUpload.tsx
(2 hunks)src/pages/Appoinments/PatientRegistration.tsx
(5 hunks)src/pages/Appoinments/PatientSelect.tsx
(4 hunks)src/pages/Appoinments/Schedule.tsx
(5 hunks)src/pages/Appoinments/Success.tsx
(2 hunks)src/pages/Appoinments/auth/PatientLogin.tsx
(3 hunks)src/pages/Facility/FacilityDetailsPage.tsx
(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/hooks/useFileUpload.tsx
[error] 192-192: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (25)
src/Utils/Notifications.ts (1)
Line range hint
45-54
: Plan for deprecation of BadRequest functionThe
BadRequest
function is marked as deprecated but is still being used in the codebase. We should plan to remove it.Let's check where it's still being used:
Consider creating a new issue to track the replacement of
BadRequest
with direct usage ofnotifyError
across the codebase.src/Utils/request/errorHandler.ts (2)
13-13
: LGTM! Good error fallback message.The migration to toast.error includes a proper fallback message when the error doesn't have a specific message.
Line range hint
35-40
: Verify the BadRequest error handling approach.The current implementation maintains
Notifications.BadRequest
for object-type errors. Based on the PR comments, this is intentional and will be addressed in a follow-up PR.✅ Verification successful
Current BadRequest error handling approach is consistent and contained
The current implementation using
Notifications.BadRequest
for object-type errors is isolated toerrorHandler.ts
and follows a consistent pattern alongsideNotifications.notifyError
. The plan to address this in a follow-up PR is reasonable as the current implementation is well-contained and doesn't affect other parts of the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find all instances of BadRequest usage to verify the scope rg "Notifications\.BadRequest" -A 2Length of output: 195
Script:
#!/bin/bash # Search for other Notifications methods and error handling patterns rg "Notifications\." -A 2 # Look for error handling related functions ast-grep --pattern 'function handle$_Error' # Find the Notifications class/object definition ast-grep --pattern 'class Notifications'Length of output: 469
src/Utils/request/utils.ts (1)
53-54
: LGTM! Good error handling pattern.The migration to toast.error maintains the proper error handling pattern by both notifying the user and throwing the error for proper error propagation.
src/components/Resource/ResourceCommentSection.tsx (1)
26-28
: LGTM! Clear validation message.The error message clearly indicates the validation requirement for non-empty comments.
src/pages/Appoinments/Success.tsx (1)
38-38
: LGTM! Properly maintains internationalization.The migration to toast.error correctly maintains the translation support for the error message.
src/Utils/useVoiceRecorder.ts (1)
32-32
: LGTM!The error handling is well-implemented, properly handling both Error instances and unknown error types.
src/Utils/useSegmentedRecorder.ts (1)
34-37
: LGTM!The implementation properly handles microphone access state and user feedback.
src/components/Form/Form.tsx (1)
2-2
: LGTM! Clean migration to toast notificationsThe implementation correctly handles form-wide errors using toast notifications while maintaining the existing validation logic.
Also applies to: 77-77
src/components/Users/UserFilter.tsx (1)
1-2
: LGTM! Clear warning messageThe warning message clearly communicates the requirement for selecting a district when a state is selected.
Also applies to: 73-73
src/components/Files/AudioCaptureDialog.tsx (1)
52-54
: LGTM! Clean migration to toast notificationsThe error handling for microphone permissions is well implemented with proper state management and user feedback.
src/components/Files/CameraCaptureDialog.tsx (1)
Line range hint
46-72
: LGTM! Well-structured camera error handlingThe implementation properly handles both camera permission denial and device availability with appropriate warning messages and cleanup.
src/pages/Appoinments/PatientSelect.tsx (1)
48-54
: LGTM! Error handling looks good.The error notifications for missing staff, phone number, and selected slot are properly implemented using toast.error with appropriate navigation fallbacks.
src/pages/Appoinments/auth/PatientLogin.tsx (1)
106-106
: Error handling implementation looks good.The error notifications for OTP operations are well implemented:
- OTP sending error uses a simple translated message
- OTP verification error properly extracts the specific error message from the response with a fallback
Also applies to: 154-154
src/components/Patient/PatientDetailsTab/Demography.tsx (1)
97-97
: LGTM!The implementation correctly uses i18n for the toast message and follows best practices.
src/pages/Appoinments/Schedule.tsx (1)
106-108
: LGTM!The implementation correctly uses i18n for the toast messages and follows best practices.
src/components/Facility/FacilityHome.tsx (1)
125-127
: LGTM! Toast migration implemented correctly.The migration from Notification to toast is implemented correctly with appropriate success messages.
Also applies to: 148-148, 166-166
src/components/Common/AvatarEditModal.tsx (1)
110-110
: LGTM! Toast warnings implemented correctly.The migration to toast warnings is implemented correctly for both file upload validation and camera permission handling.
Also applies to: 342-342
src/components/Facility/CreateFacilityForm.tsx (1)
104-104
: LGTM! Toast notifications implemented comprehensively.The migration to toast notifications is implemented correctly with appropriate success and error handling.
Also applies to: 113-113, 116-116, 142-142, 146-146, 151-151
src/Utils/utils.ts (1)
647-649
: LGTM! Clear and concise error handling.The toast notifications provide appropriate feedback for both successful and failed clipboard operations.
src/components/Patient/PatientRegistration.tsx (2)
Line range hint
128-142
: LGTM! Clear success and error handling.The notifications provide appropriate feedback for patient creation operations.
262-262
: LGTM! Appropriate validation error handling.The error notification is clear and accompanied by visual feedback (scrolling to the error field).
src/components/Common/DateInputV2.tsx (1)
4-4
: LGTM! Notification system migration looks good.The changes correctly replace the old notification system with Sonner toast notifications while maintaining the same error messages and user feedback.
Also applies to: 152-154, 268-270, 294-294
src/components/Auth/Login.tsx (1)
172-172
: LGTM! Error notification migration looks good.The change correctly replaces the old notification system with Sonner toast notification while maintaining the same error message.
public/locale/en.json (1)
1152-1152
: LGTM! New translation key added correctly.The new localization entry for successful location updates follows the established pattern in the file.
toast.success(data?.message as string); | ||
} else { | ||
Notification.Error({ | ||
Notification.notifyError({ | ||
msg: error?.message ?? t("password_update_error"), | ||
}); |
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.
🛠️ Refactor suggestion
Standardize notification pattern
The component mixes two different notification patterns: toast for success and Notification.notifyError for errors. Let's standardize to use toast for both cases.
- Notification.notifyError({
- msg: error?.message ?? t("password_update_error"),
- });
+ toast.error(error?.message ?? t("password_update_error"));
📝 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.
toast.success(data?.message as string); | |
} else { | |
Notification.Error({ | |
Notification.notifyError({ | |
msg: error?.message ?? t("password_update_error"), | |
}); | |
toast.success(data?.message as string); | |
} else { | |
toast.error(error?.message ?? t("password_update_error")); |
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.
@rithviknishad earlier i was trying to tell you about this.
When i done as prescribed above then typescript showing error as
Argument of type '{}' is not assignable to parameter of type 'string | number | boolean | (() => ReactNode) | ReactElement<any, string | JSXElementConstructor<any>> | Iterable<ReactNode> | ReactPortal | null | undefined'.ts(2345)
Now i have replaced with
toast.error((error?.message as string) ?? t("password_update_error"));
is this correct?? because sometimes we recieve errors as arrays right?
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.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
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.
most of coderabbit's suggestions seems to be valid, get those done.
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.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/pages/Appoinments/Schedule.tsx (1)
50-54
: 🛠️ Refactor suggestionAdd delay before navigation to ensure toast visibility.
The immediate navigation might prevent users from reading the toast message.
- toast.error(t("staff_username_not_found")); - navigate(`/facility/${facilityId}/`); + toast.error(t("staff_username_not_found")); + setTimeout(() => navigate(`/facility/${facilityId}/`), 2000); - toast.error(t("phone_number_not_found")); - navigate(`/facility/${facilityId}/appointments/${staffId}/otp/send`); + toast.error(t("phone_number_not_found")); + setTimeout(() => navigate(`/facility/${facilityId}/appointments/${staffId}/otp/send`), 2000);src/components/Users/UserResetPassword.tsx (1)
94-99
: 🛠️ Refactor suggestionStandardize to toast notifications
The component mixes two different notification patterns: toast for success and Notification.notifyError for errors. Let's standardize to use toast for both cases.
toast.success(data?.message as string); } else { - Notification.notifyError({ - msg: error?.message ?? t("password_update_error"), - }); - // toast.error(error?.message ?? t("password_update_error")); + toast.error(error?.message ?? t("password_update_error")); }
🧹 Nitpick comments (1)
src/hooks/useFileManager.tsx (1)
Line range hint
492-511
: Enhance download error handling.While the success notifications are well implemented, the error handling could be improved:
- Include specific error details in the error message
- Handle different types of errors separately
- Ensure cleanup in error scenarios
const downloadFile = async ( file: FileUploadModel, associating_id: string, ) => { try { if (!file.id) return; toast.success(t("file_download_started")); const fileData = await retrieveUpload(file, associating_id); const response = await fetch(fileData?.read_signed_url || ""); if (!response.ok) throw new Error("Network response was not ok."); const data = await response.blob(); const blobUrl = window.URL.createObjectURL(data); const a = document.createElement("a"); a.href = blobUrl; a.download = file.name || "file"; document.body.appendChild(a); a.click(); // Clean up window.URL.revokeObjectURL(blobUrl); document.body.removeChild(a); toast.success(t("file_download_completed")); } catch (err) { - toast.error(t("file_download_failed")); + if (err instanceof Error) { + toast.error(t("file_download_failed_with_reason", { error: err.message })); + } else { + toast.error(t("file_download_failed")); + } + // Ensure cleanup in error scenarios + const link = document.querySelector('a[download]'); + if (link) document.body.removeChild(link); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
public/locale/en.json
(9 hunks)src/Utils/Notifications.ts
(2 hunks)src/Utils/request/handleResponse.ts
(3 hunks)src/components/Facility/FacilityCreate.tsx
(2 hunks)src/components/Resource/ResourceCreate.tsx
(2 hunks)src/components/Users/UserResetPassword.tsx
(3 hunks)src/hooks/useFileManager.tsx
(5 hunks)src/pages/Appoinments/Schedule.tsx
(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/components/Facility/FacilityCreate.tsx
- src/components/Resource/ResourceCreate.tsx
- src/Utils/request/handleResponse.ts
- public/locale/en.json
- src/Utils/Notifications.ts
🔇 Additional comments (7)
src/hooks/useFileManager.tsx (3)
4-4
: LGTM: Import statement is correctly placed.The
toast
import from 'sonner' is properly scoped and well-organized with other imports.
149-149
: LGTM: Archive success notification is well implemented.The success notification is properly placed after the archive operation and uses translations correctly.
218-218
: LGTM: File rename success notification is well implemented.The success notification is properly placed and uses translations correctly, with appropriate cleanup following the operation.
src/pages/Appoinments/Schedule.tsx (4)
6-6
: LGTM: Import change aligns with toast migration.The replacement of the Notifications import with the sonner toast import is consistent with the PR objectives.
69-69
: LGTM: Proper error handling with translation.The facility error handling is correctly implemented with proper internationalization.
81-81
: LGTM: Proper error handling with translation.The user error handling is correctly implemented with proper internationalization.
106-108
: LGTM: Well-structured error handling with specific messages.The implementation properly handles structured error responses by:
- Distinguishing between specific error cases
- Using appropriate translations for each case
- Providing different user feedback based on the error type
This aligns well with the discussion in PR objectives about handling error objects.
@coderabbitai can you look for files if i have missed doing i18n translations in this pull request |
Tip For best results, initiate chat on the files or code changes. @Mahendar0701 I found several hardcoded strings that need to be translated using the
// src/pages/Organization/components/LinkUserSheet.tsx
toast.success("User added to organization successfully");
toast.error("Please select both user and role");
// src/pages/Organization/components/EditUserRoleSheet.tsx
toast.success("User role updated successfully");
toast.success("User removed from organization successfully");
toast.error("Please select a different role");
// src/pages/FacilityOrganization/components/CreateFacilityOrganizationSheet.tsx
toast.success("Organization created successfully");
toast.error("Please enter an organization name");
// src/pages/FacilityOrganization/components/EditFacilityUserRoleSheet.tsx
toast.success("User role updated successfully");
toast.success("User removed from organization successfully");
toast.error("Please select a different role");
// src/pages/FacilityOrganization/components/LinkFacilityUserSheet.tsx
toast.success("User added to organization successfully");
toast.error("Please select both user and role");
// src/components/Schedule/ScheduleTemplateForm.tsx
toast.success("Schedule template created successfully");
// src/components/Schedule/ScheduleExceptionForm.tsx
toast.success("Exception created successfully");
// src/components/Schedule/Appointments/AppointmentCreatePage.tsx
toast.error("Please select a practitioner");
toast.error("Please select a slot");
toast.success("Appointment created successfully");
toast.error("Failed to create appointment");
// src/components/Schedule/Appointments/AppointmentDetailsPage.tsx
toast.success("Appointment card has been saved!");
// src/components/Patient/PatientIndex.tsx
toast.error("Please enter a valid year of birth (YYYY)");
// src/components/Patient/PatientDetailsTab/PatientUsers.tsx
toast.success("User added to patient successfully");
toast.error("Please select both user and role");
toast.success("User removed successfully");
// src/components/Questionnaire/QuestionnaireEditor.tsx
toast.success("Questionnaire updated successfully");
toast.error("Failed to update questionnaire");
// src/components/Questionnaire/QuestionnaireForm.tsx
toast.error("Failed to submit questionnaire");
toast.success("Questionnaire submitted successfully");
// src/components/Questionnaire/ManageQuestionnaireOrganizationsSheet.tsx
toast.success("Organizations updated successfully");
// src/components/Encounter/CreateEncounterForm.tsx
toast.success("Encounter created successfully");
// src/components/Users/UserAvailabilityTab.tsx
toast.error("User needs to be linked to a home facility");
// src/CAREUI/icons/Index.tsx
toast.success("Icon copied to clipboard successfully"); Please update these messages to use the translation function // Before
toast.success("User added successfully");
// After
toast.success(t("user_added_successfully")); Don't forget to add corresponding translation keys in the 🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script:
Length of output: 23081 |
@coderabbitai what about the files which i changed in this PR? Did i missed any i18n translation in my files changed |
Tip For best results, initiate chat on the files or code changes. @Mahendar0701 Yes, I found a few hardcoded strings in the files you modified that need to be translated. Here are the ones that need to be updated:
// Change this:
toast.error("Please grant microphone permission to record audio.");
// To:
toast.error(t("microphone_permission_required"));
// Change this:
toast.success("Prescription created successfully");
// To:
toast.success(t("prescription_created_successfully"));
// Change this:
toast.success("Icon copied to clipboard successfully");
// To:
toast.success(t("icon_copied_successfully"));
// Change this:
toast.success("Schedule template created successfully");
// To:
toast.success(t("schedule_template_created_successfully"));
// Change this:
toast.success("Exception created successfully");
// To:
toast.success(t("exception_created_successfully")); Don't forget to add these new translation keys to your {
"microphone_permission_required": "Please grant microphone permission to record audio",
"prescription_created_successfully": "Prescription created successfully",
"icon_copied_successfully": "Icon copied to clipboard successfully",
"schedule_template_created_successfully": "Schedule template created successfully",
"exception_created_successfully": "Exception created successfully"
} 🔗 Analysis chain@Mahendar0701 Yes, I found a few hardcoded strings in the files you modified that need to be translated. Here are the ones that need to be updated:
// Change this:
toast.success("Profile picture deleted");
// To:
toast.success(t("profile_picture_deleted"));
// Change this:
toast.success("Password updated successfully");
// To:
toast.success(t("password_updated_successfully"));
// Change this:
toast.success("Resource created successfully");
// To:
toast.success(t("resource_created_successfully"));
// Change this:
toast.success("Resource updated successfully");
// To:
toast.success(t("resource_updated_successfully"));
// Change this:
toast.success("Patient created successfully");
// To:
toast.success(t("patient_created_successfully")); Don't forget to add these new translation keys to your {
"profile_picture_deleted": "Profile picture deleted",
"password_updated_successfully": "Password updated successfully",
"resource_created_successfully": "Resource created successfully",
"resource_updated_successfully": "Resource updated successfully",
"patient_created_successfully": "Patient created successfully"
} 🏁 Scripts executedThe following scripts were executed for the analysis: Script:
Length of output: 1554 Script:
Length of output: 187 Script:
Length of output: 33733 |
Tip For best results, initiate chat on the files or code changes. @Mahendar0701 I've checked all the files you modified in this PR, and I'm happy to report that you haven't missed any i18n translations! All toast messages in your modified files are properly using the The files I listed in my previous message are from other parts of the codebase that weren't modified in this PR, so you don't need to worry about those for now. 🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script:
Length of output: 15149 ✏️ Learnings added
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/Patient/PatientRegistration.tsx (2)
140-140
: Consider enhancing error feedback.The error notifications could be more informative by including the specific error message from the mutation error object.
- toast.error(t("patient_registration_error")); + toast.error(error?.message || t("patient_registration_error"));- toast.error(t("patient_update_error")); + toast.error(error?.message || t("patient_update_error"));Also applies to: 153-153
260-262
: Consider showing specific validation errors in the toast.While the form shows detailed validation errors inline, the toast could be more helpful by including the first validation error message.
- toast.error(t("please_fix_errors")); + const firstError = Object.values(validate).find(errors => errors.length > 0)?.[0]; + toast.error(firstError || t("please_fix_errors"));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/locale/en.json
(21 hunks)src/components/Patient/PatientRegistration.tsx
(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- public/locale/en.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: OSSAR-Scan
🔇 Additional comments (3)
src/components/Patient/PatientRegistration.tsx (3)
6-6
: LGTM! Import changes align with the PR objective.The addition of the sonner toast import correctly supports the migration from the Notification system.
126-126
: LGTM! Consistent migration to toast notifications.The notification changes are consistently applied across both create and update mutation handlers.
Also applies to: 140-140, 149-149, 153-153
Line range hint
1-824
: Verify complete migration from Notification to toast.Let's ensure no Notification usages were missed in this file.
✅ Verification successful
Migration to toast is complete
All notifications in this file have been properly migrated to use the toast component. No remnants of the old Notification component were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining Notification usages in the file # Expected: No matches as all should be migrated to toast rg -i "notification" "src/components/Patient/PatientRegistration.tsx"Length of output: 67
Script:
#!/bin/bash # Check for any Notification-related imports or component usage ast-grep --pattern 'import { $$$Notification$$$ } from "$_"' ast-grep --pattern '<Notification$$$>'Length of output: 103
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.
Other than the comment mentioned below, lgtm.
…Mahendar0701/care_fe into replace-notification-with-toast
👋 Hi, @Mahendar0701, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
👋 Hi, @Mahendar0701, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
@Jacobjeevan can you update the review. I am getting merge conflicts as this pr changing many files 😅 |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Here are the updated release notes for this update:
Localization
Notifications
sonner
library.Error Handling
Code Cleanup
The update focuses on enhancing the user notification experience by implementing a more consistent and modern toast notification system.