Fix: TypeError in notification list #8935
Conversation
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
@AdityaJ2305 we would really try to merge the 1st PR for any issue, unless its super inactive for long. Note to Reviewers: See if we can savage #8686, if we are going ahead with #8935 close the other and let the author know the reason. |
WalkthroughThe changes in this pull request focus on enhancing the subscription management logic within the Changes
Assessment against linked issues
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: 0
🧹 Outside diff range and nitpick comments (2)
src/components/Notifications/NotificationsList.tsx (2)
264-267: Add user feedback for unsupported browsers.Good addition of the service worker availability check. However, users should be informed when notifications are not supported.
Consider adding user feedback:
if (!navigator.serviceWorker) { + Warn({ + msg: t("notifications_not_supported_in_browser"), + }); return; }
305-336: Consider modernizing with async/await pattern.The pushManager check effectively prevents the TypeError. However, the nested promise chain could be simplified.
Consider refactoring to use async/await for better readability:
- navigator.serviceWorker.ready - .then(function (reg) { - if (reg.pushManager) { - setIsSubscribing(true); - reg.pushManager - .getSubscription() - .then(function (subscription) { - subscription - ?.unsubscribe() - .then(async function (_successful) { - const data = { - pf_endpoint: "", - pf_p256dh: "", - pf_auth: "", - }; - await request(routes.updateUserPnconfig, { - pathParams: { username: username }, - body: data, - }); - setIsSubscribed("NotSubscribed"); - setIsSubscribing(false); - }) - .catch(function (_e) { - Error({ - msg: t("unsubscribe_failed"), - }); - }); - }) - .catch(function (_e) { - Error({ msg: t("subscription_error") }); - }); - } - }) - .catch(function (_e) { - Sentry.captureException(_e); - }); + try { + const reg = await navigator.serviceWorker.ready; + if (!reg.pushManager) return; + + setIsSubscribing(true); + const subscription = await reg.pushManager.getSubscription(); + + try { + await subscription?.unsubscribe(); + await request(routes.updateUserPnconfig, { + pathParams: { username: username }, + body: { + pf_endpoint: "", + pf_p256dh: "", + pf_auth: "", + }, + }); + setIsSubscribed("NotSubscribed"); + } catch (e) { + Error({ msg: t("unsubscribe_failed") }); + } finally { + setIsSubscribing(false); + } + } catch (e) { + Sentry.captureException(e); + Error({ msg: t("subscription_error") }); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Notifications/NotificationsList.tsx(2 hunks)
🔇 Additional comments (1)
src/components/Notifications/NotificationsList.tsx (1)
Line range hint 264-336: Verify the TypeError fix in different browser contexts.
The implemented checks should prevent the TypeError. However, we should verify the fix across different scenarios.
Let's check for any remaining potential issues:
✅ Verification successful
Service Worker and PushManager checks are properly implemented
The code changes effectively prevent the TypeError through comprehensive checks:
- All direct
serviceWorkeraccesses have proper existence checks - The
pushManageris verified before usage - Optional chaining is used appropriately in event listeners
- Error handling is implemented for subscription operations
Key protected locations:
NotificationsList.tsx: Guards withif (!navigator.serviceWorker)andif (reg.pushManager)useMessageListener.ts: Uses optional chaining for event listenersuseNotificationSubscriptionState.ts: Implements proper error handling for subscription states
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other potential unchecked service worker or pushManager usages
rg -A 3 'serviceWorker|pushManager' --type ts --type tsx
Length of output: 86
Script:
#!/bin/bash
# Search for other potential unchecked service worker or pushManager usages with correct file extensions
rg -A 3 'serviceWorker|pushManager' --type typescript --type tsx --type jsx
Length of output: 112
Script:
#!/bin/bash
# Search for service worker and pushManager usages without file type restrictions
rg -A 3 'serviceWorker|pushManager'
Length of output: 3113
…ttps://github.com/AdityaJ2305/care_fe into issue/8006/TypeError_in_NotificationsList_Component
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/components/Notifications/NotificationsList.tsx (2)
264-267: Add user feedback for unsupported browsers.Good addition of the service worker availability check. However, users should be informed when notifications aren't supported.
if (!navigator.serviceWorker) { + Warn({ msg: t("notifications_not_supported") }); return; }
302-339: Improve error handling and user feedback in unsubscribe function.The async/await refactor and error handling additions are good improvements. However, there are a few suggestions for enhancement:
- Add user feedback for unsupported pushManager:
- if (!reg.pushManager) return; + if (!reg.pushManager) { + Warn({ msg: t("push_notifications_not_supported") }); + setIsSubscribing(false); + return; + }
- Simplify the nested try-catch:
const subscription = await reg.pushManager.getSubscription(); if (subscription) { try { await subscription.unsubscribe(); - - const data = { - pf_endpoint: "", - pf_p256dh: "", - pf_auth: "", - }; - - await request(routes.updateUserPnconfig, { - pathParams: { username }, - body: data, - }); - - setIsSubscribed("NotSubscribed"); - } catch (e) { - Error({ msg: t("unsubscribe_failed") }); - } finally { - setIsSubscribing(false); - } -} else { - setIsSubscribing(false); -} + await request(routes.updateUserPnconfig, { + pathParams: { username }, + body: { + pf_endpoint: "", + pf_p256dh: "", + pf_auth: "", + }, + }); + setIsSubscribed("NotSubscribed"); + } catch (e) { + Error({ msg: t("unsubscribe_failed") }); + } +} +setIsSubscribing(false);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Notifications/NotificationsList.tsx(2 hunks)
🔇 Additional comments (1)
src/components/Notifications/NotificationsList.tsx (1)
Line range hint 264-339: Verify service worker and pushManager availability checks.
The implementation successfully addresses the TypeError by adding proper checks for service worker and pushManager availability. Let's verify the usage of these APIs across the codebase:
✅ Verification successful
Based on the search results, I can now provide a comprehensive verification. The code shows consistent patterns of service worker and pushManager checks across the codebase:
Service worker and pushManager checks are properly implemented across the codebase
- The main implementation in
NotificationsList.tsxhas proper guards - The shared hook
useNotificationSubscriptionState.tsfollows the same pattern of checking service worker availability - All pushManager usages are properly guarded with existence checks
- The error handling is consistent across all notification-related implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other instances of pushManager usage that might need similar guards
# Test 1: Search for direct pushManager usage
echo "Checking for pushManager usage..."
rg "pushManager"
# Test 2: Search for service worker registration patterns
echo "Checking for service worker registration patterns..."
rg "serviceWorker.ready"
# Test 3: Search for subscription-related code
echo "Checking for subscription-related code..."
rg "getSubscription|subscribe|unsubscribe"
Length of output: 7257
|
@coderabbitai full review |
|
@AdityaJ2305, I will perform a full review of the pull request. ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/components/Notifications/NotificationsList.tsx (2)
264-267: Add user feedback for unsupported browsers.The guard clause for
navigator.serviceWorkeravailability is good, but users should be informed why the subscription failed.Consider adding user feedback:
if (!navigator.serviceWorker) { + Error({ msg: t("browser_not_supported") }); return; }Also, you can simplify the code by using
isSubscribeddirectly:-const status = isSubscribed; -if (["NotSubscribed", "SubscribedOnAnotherDevice"].includes(status)) { +if (["NotSubscribed", "SubscribedOnAnotherDevice"].includes(isSubscribed)) {
302-339: Improve error handling specificity and consistency.The refactored unsubscribe function with proper error handling and state management looks good. The checks for
pushManageravailability and proper try-catch blocks effectively prevent the TypeError.Consider these minor improvements:
- Make error messages more specific:
- Error({ msg: t("unsubscribe_failed") }); + Error({ msg: t("unsubscribe_failed_subscription_removal") });
- Remove excessive empty lines for consistent spacing:
try { const reg = await navigator.serviceWorker.ready; - if (!reg.pushManager) { Error({ msg: t("unsubscribe_failed") }); return; } - setIsSubscribing(true); - const subscription = await reg.pushManager.getSubscription(); - if (subscription) {
|
👋 Hi, @AdityaJ2305, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
|
Hey @bodhish , This PR is labeled as “blocked.” I’ve made the requested changes requested by |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Note
I noticed there was an existing PR addressing this issue, but it seems inactive with unresolved conflicts. I’ve resolved the issue here to help move things forward.
Summary by CodeRabbit
Bug Fixes
Refactor