Switch to Sonner for toast notifications #9405
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThe pull request includes significant modifications to the notification handling system of the application. It transitions from using the PNotify library to the Sonner library for toast notifications. This involves removing PNotify dependencies from Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
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 (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![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 (5)
src/components/ui/sonner.tsx (1)
9-28: Consider extracting theme-specific styles to a constants fileThe implementation looks good, but the long class names for different toast elements could be moved to a separate constants file for better maintainability.
Consider creating a
toasterStyles.ts:export const TOAST_STYLES = { toast: "group toast group-[.toaster]:bg-white ...", description: "group-[.toast]:text-gray-500 ...", // ... other styles };Then import and use it in the component:
- classNames: { - toast: "group toast group-[.toaster]:bg-white...", - description: "group-[.toast]:text-gray-500...", + classNames: TOAST_STYLES,src/Utils/Notifications.js (3)
41-43: Consider adding error message sanitization.While the error message handling is functional, consider sanitizing the error message before displaying it to prevent potential XSS attacks through error messages.
- toast.error(errorMsg, { duration: 5000 }); + const sanitizedMsg = DOMPurify.sanitize(errorMsg); + toast.error(sanitizedMsg, { duration: 5000 });
46-50: Consider adding a return value for closeAllNotifications.The function should return a boolean or Promise to indicate whether the operation was successful.
export const closeAllNotifications = () => { // Sonner doesn't require a close method, but you can manage this with custom logic if needed // Example: toast.dismiss() could be used to close all toasts if necessary. - toast.dismiss(); + toast.dismiss(); + return true; };
54-55: Standardize notification durations.Consider extracting duration values into constants to maintain consistency across different notification types.
+const NOTIFICATION_DURATIONS = { + SUCCESS: 3000, + ERROR: 5000, + INFO: 3000, +}; export const Success = ({ msg }) => { - toast.success(msg, { duration: 3000 }); + toast.success(msg, { duration: NOTIFICATION_DURATIONS.SUCCESS }); }; export const Error = ({ msg }) => { - toast.error(msg, { duration: 5000 }); + toast.error(msg, { duration: NOTIFICATION_DURATIONS.ERROR }); }; export const Warn = ({ msg }) => { - toast.info(msg, { duration: 3000 }); + toast.info(msg, { duration: NOTIFICATION_DURATIONS.INFO }); };Also applies to: 59-60, 64-65
src/components/Patient/DailyRounds.tsx (1)
341-343: Consider adding error recovery mechanism.While the error handling is functional, consider adding a retry mechanism for failed investigation updates.
+const MAX_RETRIES = 3; +const retryInvestigationUpdate = async (body, pathParams, retries = 0) => { + try { + const { error } = await request(routes.partialUpdateConsultation, { + body, + pathParams, + }); + return { error }; + } catch (err) { + if (retries < MAX_RETRIES) { + await new Promise(resolve => setTimeout(resolve, 1000 * (retries + 1))); + return retryInvestigationUpdate(body, pathParams, retries + 1); + } + return { error: err }; + } +}; if (state.form.rounds_type === "DOCTORS_LOG" && state.form.investigations_dirty) { - const { error: investigationError } = await request( - routes.partialUpdateConsultation, - { - body: { investigation: state.form.investigations }, - pathParams: { id: consultationId }, - }, - ); + const { error: investigationError } = await retryInvestigationUpdate( + { investigation: state.form.investigations }, + { id: consultationId } + ); if (investigationError) { Notification.Error({ msg: investigationError.message || investigationError, }); return; } }
📜 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 (10)
package.json(2 hunks)src/App.tsx(1 hunks)src/CAREUI/icons/Index.tsx(2 hunks)src/Utils/Notifications.js(2 hunks)src/components/Patient/DailyRounds.tsx(1 hunks)src/components/ui/sonner.tsx(1 hunks)src/components/ui/toast.tsx(0 hunks)src/components/ui/toaster.tsx(0 hunks)src/hooks/useToast.ts(0 hunks)src/style/index.css(0 hunks)
💤 Files with no reviewable changes (4)
- src/style/index.css
- src/components/ui/toaster.tsx
- src/hooks/useToast.ts
- src/components/ui/toast.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
src/Utils/Notifications.js
[error] 59-59: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🔇 Additional comments (4)
src/components/ui/sonner.tsx (1)
4-8: LGTM: Clean type-safe implementation
The component properly inherits and forwards all props from the Sonner Toaster component while adding theme support. The type safety is well implemented.
src/App.tsx (1)
5-5: LGTM: Clean import update
The import path update correctly reflects the migration to Sonner, and the Toaster component placement remains appropriate at the application root level.
src/Utils/Notifications.js (1)
1-1: LGTM: Clean import of Sonner.
The import statement correctly imports the toast function from the Sonner library.
package.json (1)
89-89: Verify Sonner version compatibility.
The added dependencies look correct, but let's verify the version compatibility and check for any known security issues.
Also applies to: 101-101
✅ Verification successful
Sonner dependency version is up-to-date and secure
The installed version (1.7.1) matches the latest version from npm registry, and there are no known security vulnerabilities reported for the package.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for latest versions and security advisories
echo "Checking latest versions..."
curl -s https://registry.npmjs.org/sonner/latest | jq '.version'
echo "Checking for security advisories..."
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NPM, package: "sonner") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 558
src/CAREUI/icons/Index.tsx
Outdated
| description: "Icon copied to clipboard successfully", | ||
| variant: "success", | ||
| }); | ||
| toast.success("Icon copied to clipboard successfully"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add i18n support for the toast message
The success message is hardcoded. Since the project uses i18next (as seen in the imports), this message should be internationalized.
- toast.success("Icon copied to clipboard successfully");
+ toast.success(t("icon.copy.success"));Committable suggestion skipped: line range outside the PR's diff.
src/CAREUI/icons/Index.tsx
Outdated
| const copyToClipboard = (text: string) => { | ||
| navigator.clipboard.writeText(text); | ||
|
|
||
| toast({ | ||
| description: "Icon copied to clipboard successfully", | ||
| variant: "success", | ||
| }); | ||
| toast.success("Icon copied to clipboard successfully"); | ||
| }; |
There was a problem hiding this comment.
Add error handling for clipboard operations
The clipboard operation could fail in certain scenarios (permissions, secure context, etc.). Consider adding error handling.
- const copyToClipboard = (text: string) => {
- navigator.clipboard.writeText(text);
- toast.success("Icon copied to clipboard successfully");
+ const copyToClipboard = async (text: string) => {
+ try {
+ await navigator.clipboard.writeText(text);
+ toast.success(t("icon.copy.success"));
+ } catch (error) {
+ toast.error(t("icon.copy.error"));
+ console.error("Failed to copy to clipboard:", 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.
| const copyToClipboard = (text: string) => { | |
| navigator.clipboard.writeText(text); | |
| toast({ | |
| description: "Icon copied to clipboard successfully", | |
| variant: "success", | |
| }); | |
| toast.success("Icon copied to clipboard successfully"); | |
| }; | |
| const copyToClipboard = async (text: string) => { | |
| try { | |
| await navigator.clipboard.writeText(text); | |
| toast.success(t("icon.copy.success")); | |
| } catch (error) { | |
| toast.error(t("icon.copy.error")); | |
| console.error("Failed to copy to clipboard:", error); | |
| } | |
| }; |
rithviknishad
changed the title
src/App.tsx
Outdated
There was a problem hiding this comment.
LGTM, just few things...
Ensure notifications are placed in top right of the screen and rich colors are used.
As already mentioned in the issue, use this:
Lines 38 to 45 in e48d37c
|
You'll also have to modify the cypress tests btw. |
|
👋 Hi, @nilay-v3rma, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
cypress/support/commands.ts (2)
Line range hint
213-219: Update or remove legacy closeNotification commandThe
closeNotificationcommand still uses the.pnotifyselector, which should be updated to use Sonner's implementation.Consider replacing it with:
-Cypress.Commands.add("closeNotification", () => { - cy.get(".pnotify") - .should("exist") - .each(($div) => { - cy.wrap($div).click(); - }); -}); +Cypress.Commands.add("closeNotification", () => { + cy.get("li[data-sonner-toast]") + .should("exist") + .each(($toast) => { + cy.wrap($toast).find('[data-close-button]').click(); + }); +});
Migration to Sonner is incomplete - PNotify reference found in Cypress commands
The migration from PNotify to Sonner is mostly complete with Sonner properly configured in App.tsx, but there's still a remaining PNotify reference in the Cypress test commands:
cypress/support/commands.ts: ThecloseNotificationcommand still uses the.pnotifyselectorThis Cypress command should be updated to use Sonner's toast elements instead of PNotify's for consistency with the new implementation.
🔗 Analysis chain
Line range hint
1-1: Verify complete removal of PNotifyLet's ensure the migration to Sonner is complete across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Comprehensive check for complete migration to Sonner # Check for any remaining PNotify dependencies echo "Checking package.json for PNotify..." jq '.dependencies + .devDependencies' package.json | grep -i pnotify # Check for PNotify imports echo "Checking for PNotify imports..." rg "import.*pnotify" --type ts --type tsx --type js --type jsx # Check for PNotify CSS classes echo "Checking for PNotify CSS classes..." rg "\.pnotify" --type css --type scss --type less # Verify Sonner is properly installed echo "Verifying Sonner installation..." jq '.dependencies["sonner"]' package.json # Check for Toaster component implementation echo "Checking for Toaster component..." rg "<Toaster" --type tsx --type jsxLength of output: 752
Script:
#!/bin/bash # Let's modify our search approach to handle file extensions correctly # Check for any PNotify-related code echo "Checking for any PNotify references..." rg -t js -t ts "PNotify|pnotify" # Check for Toaster component usage echo "Checking for Toaster component usage..." rg -t js -t ts "<Toaster" # List all files that might contain toast-related code echo "Listing potential toast-related files..." fd -e ts -e js -e tsx -e jsx | grep -i "toast"Length of output: 458
Script:
#!/bin/bash # Let's check the specific PNotify reference in commands.ts and look for toast-related code # Check the context of PNotify usage in commands.ts echo "Checking context of PNotify in commands.ts..." rg -B 5 -A 5 "\.pnotify" cypress/support/commands.ts # Look for toast/notification related functions echo "Checking toast-related functions..." rg -t js -t ts "toast\(" -B 2 -A 2 # Check App.tsx Toaster implementation echo "Checking Toaster implementation in App.tsx..." rg -B 5 -A 5 "<Toaster" src/App.tsxLength of output: 1077
🧹 Nitpick comments (1)
cypress/support/commands.ts (1)
116-122: Consider enhancing the verifyNotification command with timeout and error handlingThe implementation correctly uses Sonner's selectors and follows Cypress best practices. However, we can make it more robust.
Consider these enhancements:
//check sonner toast notification -Cypress.Commands.add("verifyNotification", (text) => { +Cypress.Commands.add("verifyNotification", (text, timeout = 10000) => { return cy .get("li[data-sonner-toast] div[data-title]", { timeout }) .should("exist") .and("be.visible") - .contains(text); + .contains(text) + .should((toast) => { + // Additional validation that toast is actually visible in viewport + const rect = toast[0].getBoundingClientRect(); + expect(rect.top).to.be.greaterThan(-rect.height); + expect(rect.top).to.be.lessThan(Cypress.config('viewportHeight')); + }); });This enhancement:
- Adds configurable timeout
- Ensures toast is actually visible in viewport
- Maintains chainability
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cypress/pageobject/Facility/FacilityManage.ts(1 hunks)cypress/support/commands.ts(1 hunks)src/App.tsx(2 hunks)src/CAREUI/icons/Index.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/CAREUI/icons/Index.tsx
- src/App.tsx
| // Selector for the Sonner toast content | ||
| const toastSelector = "li[data-sonner-toast] div[data-title]"; | ||
|
|
||
| if (isRegex) { | ||
| cy.get(".pnotify-text").should("be.visible").contains(text); | ||
| cy.get(toastSelector).should("be.visible").contains(text); | ||
| } else { | ||
| cy.get(".pnotify-text").should("be.visible").and("contain.text", text); | ||
| cy.get(toastSelector).should("be.visible").and("contain.text", text); |
There was a problem hiding this comment.
💡 Codebase verification
Migration to Sonner needs completion - PNotify selector still present in commands.ts
The verification revealed an inconsistency in the migration:
- The
closeNotificationcommand incypress/support/commands.tsstill uses the old.pnotifyselector - While
verifyNotificationcommand has been updated to use Sonner'sdata-sonner-toastselector - PNotify references in JSON files are just dependency declarations, not actual usage
The selector update in FacilityManage.ts is correct, but the migration is incomplete. The closeNotification command needs to be updated to use Sonner's selectors for consistency.
🔗 Analysis chain
LGTM! Selector update aligns with Sonner migration
The changes correctly update the toast notification selector to use Sonner's data attributes. The implementation maintains the existing functionality while properly adapting to Sonner's DOM structure.
Let's verify the consistency of Sonner selectors across the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining PNotify selectors and verify Sonner selector usage
# Look for any remaining PNotify selectors
echo "Checking for remaining PNotify selectors..."
rg -l "pnotify" --type ts --type tsx
# Verify consistent usage of Sonner selectors
echo "Verifying Sonner selector usage..."
rg -l "data-sonner-toast" --type ts --type tsx
Length of output: 315
Script:
#!/bin/bash
# Let's try without specifying file types and use more generic patterns
echo "Checking for remaining PNotify selectors..."
rg -l "pnotify"
echo -e "\nVerifying Sonner selector usage..."
rg -l "data-sonner-toast"
echo -e "\nChecking toast-related selectors in detail..."
rg "toast.*selector|selector.*toast" -A 2 -B 2
Length of output: 534
Script:
#!/bin/bash
# Let's check the content of files that still contain pnotify references
echo "Checking pnotify usage in commands.ts..."
rg "pnotify" cypress/support/commands.ts -A 2 -B 2
echo -e "\nChecking Sonner implementation in commands.ts..."
rg "data-sonner-toast" cypress/support/commands.ts -A 2 -B 2
# Check if pnotify in JSON files are just dependencies or actual usage
echo -e "\nChecking pnotify mentions in JSON files..."
rg "pnotify" src/components/Licenses/fe-sbom.json public/bom.json
Length of output: 3942
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cypress/support/commands.ts (1)
116-122: Enhance the verifyNotification command with better typing and documentation.The implementation correctly uses data attributes for testing and has proper visibility checks. Consider these improvements:
-//check sonner toast notification +/** + * Verifies that a Sonner toast notification with specific text is visible + * @param text - The expected text content of the notification + * @param timeout - Optional timeout in ms (default: Cypress default timeout) + */ -Cypress.Commands.add("verifyNotification", (text) => { +Cypress.Commands.add("verifyNotification", (text: string, timeout?: number) => { return cy .get("li[data-sonner-toast] div[data-title]") + .timeout(timeout || Cypress.config('defaultCommandTimeout')) .should("exist") .and("be.visible") .contains(text); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cypress/support/commands.ts(2 hunks)
🔇 Additional comments (1)
cypress/support/commands.ts (1)
116-122: Verify consistent notification selectors across all test files.
Let's ensure all test files have been updated to use the new Sonner selectors.
Also applies to: 223-223
✅ Verification successful
Migration to Sonner toast notifications is consistent across the codebase
The verification shows that:
- All toast notifications use the new Sonner selector
li[data-sonner-toast] div[data-title] - No legacy PNotify selectors were found
- The implementation is consistent between
commands.tsandFacilityManage.ts - All notification-related tests are using the new
verifyNotificationcommand or the equivalent Sonner selectors
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining PNotify selectors and verify consistent Sonner selector usage
echo "Checking for old PNotify selectors..."
rg -l "pnotify|PNotify" "cypress/"
echo "Verifying consistent Sonner selector usage..."
rg -l "data-sonner-toast" "cypress/"
echo "Checking for direct notification assertions that might need updating..."
rg "\.should\(.*contain.*notification" "cypress/"
Length of output: 532
Script:
#!/bin/bash
# Let's examine the notification-related assertions in the test files more thoroughly
echo "Checking all notification-related selectors and assertions..."
rg -A 2 "notification|toast" "cypress/" --type ts
echo "Examining FacilityManage.ts specifically..."
cat "cypress/pageobject/Facility/FacilityManage.ts"
echo "Looking for any cy.get patterns that might be notification-related..."
rg "cy\.get\(.*toast" "cypress/" --type ts
Length of output: 10140
|
LGTM, do resolve the conflicts. |
rithviknishad
added
needs testing
and removed
changes required
merge conflict
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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)
cypress/pageobject/Facility/FacilityManage.ts(1 hunks)cypress/support/commands.ts(2 hunks)package.json(3 hunks)src/App.tsx(2 hunks)src/components/Patient/DailyRounds.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- cypress/pageobject/Facility/FacilityManage.ts
- src/components/Patient/DailyRounds.tsx
- src/App.tsx
- package.json
🔇 Additional comments (2)
cypress/support/commands.ts (2)
Line range hint 229-233: Enhance closeNotification command with better error handling and verification.
The previous review comment about improving error handling and verification in the closeNotification command is still applicable.
The command should:
- Ensure notifications are visible and clickable
- Verify each notification is actually closed
- Handle potential animation states
Line range hint 116-233: Verify impact on existing Cypress tests.
Since we're switching from PNotify to Sonner, we should verify that all existing Cypress tests using these commands have been updated accordingly.
✅ Verification successful
No changes needed for existing Cypress tests
Based on the verification results, all notification-related commands in the codebase are already using the new Sonner implementation consistently. The commands verifyNotification and closeNotification are being used correctly across multiple test files, and there are no remaining references to PNotify in the codebase. The notification selectors in the commands (li[data-sonner-toast] div[data-title]) match Sonner's DOM structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing test files that might need updates
echo "Searching for tests using notification commands..."
rg -l "cy\.(verifyNotification|closeNotification)" "cypress/e2e"
# Look for any remaining PNotify references that might need updating
echo "Checking for remaining PNotify references..."
rg -i "pnotify" "cypress/e2e"
Length of output: 1267
Script:
#!/bin/bash
# Let's examine the actual usage of notification commands in these files
# to verify if they match our new Sonner implementation
echo "Checking notification usage patterns in test files..."
rg "cy\.(verifyNotification|closeNotification)" "cypress/e2e" -B 1 -A 1
# Let's also check the commands.ts file to see if there are any old notification-related commands
# that might need cleanup
echo "Checking for any old notification commands..."
rg "Notification" "cypress/support/commands.ts"
Length of output: 32377
|
tests failing... |
I will work on it |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Release Notes
New Features
Toastercomponent for improved toast notifications.Bug Fixes
DailyRoundscomponent for clearer error reporting.Chores
Refactor