Fix Notification Subscription Loading and Messaging Errors #9038
Conversation
WalkthroughThe changes in this pull request involve updates to the localization file Changes
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (2)
src/components/Notifications/NotificationsList.tsx (1)
308-311: Consider using Success notification for unsubscribe confirmationThe unsubscribe confirmation uses a warning notification (
Warn), but since unsubscribing is a successful action rather than a cautionary event, consider usingSuccessinstead to maintain consistent UX patterns.- Warn({ + Success({ msg: t("unsubscribed_successfully"), });public/locale/en.json (1)
891-891: Fix inconsistent message formatting.The notification messages should follow consistent formatting:
- "notification_cancelled": "Notification cancelled", + "notification_cancelled": "Notification Cancelled", "subscribed_successfully": "Subscribed Successfully", "subscription_failed": "Subscription Failed", - "unsubscribed_successfully": "Unsubscribed Successfully.", + "unsubscribed_successfully": "Unsubscribed Successfully"This ensures:
- Consistent title case capitalization across all words
- Consistent punctuation (no trailing periods)
Also applies to: 1161-1161, 1163-1163, 1206-1206
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
public/locale/en.json(3 hunks)src/components/Notifications/NotificationsList.tsx(3 hunks)
🔇 Additional comments (2)
src/components/Notifications/NotificationsList.tsx (1)
246-253: LGTM: Permission check prevents infinite loading state
The implementation correctly checks the notification permission status before attempting to subscribe, preventing the infinite loading state when permission is denied. The error message is properly localized using the translation system.
public/locale/en.json (1)
891-891: LGTM! The notification messages are clear and appropriate.
The new localization entries properly support the notification subscription feature improvements mentioned in the PR objectives.
Also applies to: 1161-1161, 1163-1163, 1206-1206
| const p256dh = btoa( | ||
| String.fromCharCode.apply( | ||
| null, | ||
| new Uint8Array(push.getKey("p256dh") as any) as any, | ||
| ), | ||
| ); | ||
| const auth = btoa( | ||
| String.fromCharCode.apply( | ||
| null, | ||
| new Uint8Array(push.getKey("auth") as any) as any, | ||
| ), | ||
| ); |
There was a problem hiding this comment.
Add error handling for key encoding
The btoa encoding of push notification keys could fail if the binary data contains invalid characters. Consider adding try-catch blocks around the encoding operations.
- const p256dh = btoa(
- String.fromCharCode.apply(
- null,
- new Uint8Array(push.getKey("p256dh") as any) as any,
- ),
- );
- const auth = btoa(
- String.fromCharCode.apply(
- null,
- new Uint8Array(push.getKey("auth") as any) as any,
- ),
- );
+ let p256dh, auth;
+ try {
+ p256dh = btoa(
+ String.fromCharCode(...new Uint8Array(push.getKey("p256dh") as ArrayBuffer))
+ );
+ auth = btoa(
+ String.fromCharCode(...new Uint8Array(push.getKey("auth") as ArrayBuffer))
+ );
+ } catch (error) {
+ throw new Error("Failed to encode push notification keys");
+ }📝 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 p256dh = btoa( | |
| String.fromCharCode.apply( | |
| null, | |
| new Uint8Array(push.getKey("p256dh") as any) as any, | |
| ), | |
| ); | |
| const auth = btoa( | |
| String.fromCharCode.apply( | |
| null, | |
| new Uint8Array(push.getKey("auth") as any) as any, | |
| ), | |
| ); | |
| let p256dh, auth; | |
| try { | |
| p256dh = btoa( | |
| String.fromCharCode(...new Uint8Array(push.getKey("p256dh") as ArrayBuffer)) | |
| ); | |
| auth = btoa( | |
| String.fromCharCode(...new Uint8Array(push.getKey("auth") as ArrayBuffer)) | |
| ); | |
| } catch (error) { | |
| throw new Error("Failed to encode push notification keys"); | |
| } |
| try { | ||
| const response = await request(routes.getPublicKey); | ||
| const public_key = response.data?.public_key; | ||
| const sw = await navigator.serviceWorker.ready; | ||
| const push = await sw.pushManager.subscribe({ | ||
| userVisibleOnly: true, | ||
| applicationServerKey: public_key, | ||
| }); | ||
| const p256dh = btoa( | ||
| String.fromCharCode.apply( | ||
| null, | ||
| new Uint8Array(push.getKey("p256dh") as any) as any, | ||
| ), | ||
| ); | ||
| const auth = btoa( | ||
| String.fromCharCode.apply( | ||
| null, | ||
| new Uint8Array(push.getKey("auth") as any) as any, | ||
| ), | ||
| ); | ||
|
|
||
| const data = { | ||
| pf_endpoint: push.endpoint, | ||
| pf_p256dh: p256dh, | ||
| pf_auth: auth, | ||
| }; | ||
| const data = { | ||
| pf_endpoint: push.endpoint, | ||
| pf_p256dh: p256dh, | ||
| pf_auth: auth, | ||
| }; | ||
|
|
||
| const { res } = await request(routes.updateUserPnconfig, { | ||
| pathParams: { username: username }, | ||
| body: data, | ||
| }); | ||
| const { res } = await request(routes.updateUserPnconfig, { | ||
| pathParams: { username: username }, | ||
| body: data, | ||
| }); | ||
|
|
||
| if (res?.ok) { | ||
| setIsSubscribed("SubscribedOnThisDevice"); | ||
| Success({ | ||
| msg: t("subscribed_successfully"), | ||
| }); | ||
| setIsSubscribing(false); | ||
| } else { | ||
| Error({ | ||
| msg: t("subscription_failed"), | ||
| }); | ||
| setIsSubscribing(false); | ||
| } | ||
| } catch (error) { | ||
| const permission = Notification.permission; | ||
|
|
||
| if (res?.ok) { | ||
| setIsSubscribed("SubscribedOnThisDevice"); | ||
| if (permission === "denied") { | ||
| Warn({ | ||
| msg: t("notification_permission_denied"), | ||
| }); | ||
| setIsSubscribing(false); | ||
| return; | ||
| } else if (permission === "default") { | ||
| Warn({ | ||
| msg: t("notification_cancelled"), | ||
| }); | ||
| setIsSubscribing(false); | ||
| return; | ||
| } | ||
| Error({ | ||
| msg: t("subscription_failed"), | ||
| }); | ||
| } finally { | ||
| setIsSubscribing(false); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor error handling and state management
While the error handling is comprehensive, there are a few improvements that could be made:
- The
setIsSubscribing(false)is duplicated across the function - The error handling in the catch block could be simplified
- The type casting could be more specific
Consider this refactoring:
async function subscribe() {
setIsSubscribing(true);
try {
const response = await request(routes.getPublicKey);
const public_key = response.data?.public_key;
const sw = await navigator.serviceWorker.ready;
const push = await sw.pushManager.subscribe({
userVisibleOnly: true,
applicationServerKey: public_key,
});
- const p256dh = btoa(
- String.fromCharCode.apply(
- null,
- new Uint8Array(push.getKey("p256dh") as any) as any,
- ),
- );
- const auth = btoa(
- String.fromCharCode.apply(
- null,
- new Uint8Array(push.getKey("auth") as any) as any,
- ),
- );
+ const p256dh = btoa(
+ String.fromCharCode(...new Uint8Array(push.getKey("p256dh") as ArrayBuffer))
+ );
+ const auth = btoa(
+ String.fromCharCode(...new Uint8Array(push.getKey("auth") as ArrayBuffer))
+ );
const data = {
pf_endpoint: push.endpoint,
pf_p256dh: p256dh,
pf_auth: auth,
};
const { res } = await request(routes.updateUserPnconfig, {
pathParams: { username: username },
body: data,
});
if (res?.ok) {
setIsSubscribed("SubscribedOnThisDevice");
Success({
msg: t("subscribed_successfully"),
});
- setIsSubscribing(false);
} else {
Error({
msg: t("subscription_failed"),
});
- setIsSubscribing(false);
}
} catch (error) {
const permission = Notification.permission;
- if (permission === "denied") {
- Warn({
- msg: t("notification_permission_denied"),
- });
- setIsSubscribing(false);
- return;
- } else if (permission === "default") {
- Warn({
- msg: t("notification_cancelled"),
- });
- setIsSubscribing(false);
- return;
- }
- Error({
- msg: t("subscription_failed"),
- });
+ const messages = {
+ denied: t("notification_permission_denied"),
+ default: t("notification_cancelled"),
+ };
+
+ if (permission in messages) {
+ Warn({ msg: messages[permission as keyof typeof messages] });
+ } else {
+ Error({ msg: t("subscription_failed") });
+ }
} finally {
setIsSubscribing(false);
}
}📝 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.
| try { | |
| const response = await request(routes.getPublicKey); | |
| const public_key = response.data?.public_key; | |
| const sw = await navigator.serviceWorker.ready; | |
| const push = await sw.pushManager.subscribe({ | |
| userVisibleOnly: true, | |
| applicationServerKey: public_key, | |
| }); | |
| const p256dh = btoa( | |
| String.fromCharCode.apply( | |
| null, | |
| new Uint8Array(push.getKey("p256dh") as any) as any, | |
| ), | |
| ); | |
| const auth = btoa( | |
| String.fromCharCode.apply( | |
| null, | |
| new Uint8Array(push.getKey("auth") as any) as any, | |
| ), | |
| ); | |
| const data = { | |
| pf_endpoint: push.endpoint, | |
| pf_p256dh: p256dh, | |
| pf_auth: auth, | |
| }; | |
| const data = { | |
| pf_endpoint: push.endpoint, | |
| pf_p256dh: p256dh, | |
| pf_auth: auth, | |
| }; | |
| const { res } = await request(routes.updateUserPnconfig, { | |
| pathParams: { username: username }, | |
| body: data, | |
| }); | |
| const { res } = await request(routes.updateUserPnconfig, { | |
| pathParams: { username: username }, | |
| body: data, | |
| }); | |
| if (res?.ok) { | |
| setIsSubscribed("SubscribedOnThisDevice"); | |
| Success({ | |
| msg: t("subscribed_successfully"), | |
| }); | |
| setIsSubscribing(false); | |
| } else { | |
| Error({ | |
| msg: t("subscription_failed"), | |
| }); | |
| setIsSubscribing(false); | |
| } | |
| } catch (error) { | |
| const permission = Notification.permission; | |
| if (res?.ok) { | |
| setIsSubscribed("SubscribedOnThisDevice"); | |
| if (permission === "denied") { | |
| Warn({ | |
| msg: t("notification_permission_denied"), | |
| }); | |
| setIsSubscribing(false); | |
| return; | |
| } else if (permission === "default") { | |
| Warn({ | |
| msg: t("notification_cancelled"), | |
| }); | |
| setIsSubscribing(false); | |
| return; | |
| } | |
| Error({ | |
| msg: t("subscription_failed"), | |
| }); | |
| } finally { | |
| setIsSubscribing(false); | |
| try { | |
| const response = await request(routes.getPublicKey); | |
| const public_key = response.data?.public_key; | |
| const sw = await navigator.serviceWorker.ready; | |
| const push = await sw.pushManager.subscribe({ | |
| userVisibleOnly: true, | |
| applicationServerKey: public_key, | |
| }); | |
| const p256dh = btoa( | |
| String.fromCharCode(...new Uint8Array(push.getKey("p256dh") as ArrayBuffer)) | |
| ); | |
| const auth = btoa( | |
| String.fromCharCode(...new Uint8Array(push.getKey("auth") as ArrayBuffer)) | |
| ); | |
| const data = { | |
| pf_endpoint: push.endpoint, | |
| pf_p256dh: p256dh, | |
| pf_auth: auth, | |
| }; | |
| const { res } = await request(routes.updateUserPnconfig, { | |
| pathParams: { username: username }, | |
| body: data, | |
| }); | |
| if (res?.ok) { | |
| setIsSubscribed("SubscribedOnThisDevice"); | |
| Success({ | |
| msg: t("subscribed_successfully"), | |
| }); | |
| } else { | |
| Error({ | |
| msg: t("subscription_failed"), | |
| }); | |
| } | |
| } catch (error) { | |
| const permission = Notification.permission; | |
| const messages = { | |
| denied: t("notification_permission_denied"), | |
| default: t("notification_cancelled"), | |
| }; | |
| if (permission in messages) { | |
| Warn({ msg: messages[permission as keyof typeof messages] }); | |
| } else { | |
| Error({ msg: t("subscription_failed") }); | |
| } | |
| } finally { | |
| setIsSubscribing(false); | |
| } |
Proposed Changes
Before :
Subscribe issue
Irrelevant Notification Message on clicking unsubscribe:
After:
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Bug Fixes
User Experience Enhancements