Is Safari intentionally excluded from the Service Worker installation?
-
@julian seems like safari is picky about replacing same notifs with
tag, the PR on the repo has some code to handle safari via mergeId.
> @baris said:
>
> @julian seems like safari is picky about replacing same notifs withtag, the PR on the repo has some code to handle safari via mergeId.
>
>
>Indeed, the last commit was written with the aim of solving this problem, but it should be noted that after testing it did not help.
I also do not have Safari to test, and I asked my friend to check with him (such a bad browser... it's a miracle I don't have one <img class="not-responsive emoji" src="https://community.nodebb.org/assets/plugins/nodebb-plugin-emoji/emoji/android/1f605.png?v=82067483ff1" title="
" /> )I did not even remember opening a PR with these commits.
I will now delete the last commit so that the effective code of the previous commits can be merged
-
I saw that there is an exception for the Safari browser that the Service Worker is not installed, is there a reason for this?
This makes it impossible to subscribe to push notifications in the Safari browser even when installing the page as a PWA application
-
I saw that there is an exception for the Safari browser that the Service Worker is not installed, is there a reason for this?
This makes it impossible to subscribe to push notifications in the Safari browser even when installing the page as a PWA application
Yeah without an iphone it's not possible to test it, the change the in the PR looked good to me so if that don't work, I am out of ideas.
-
Yeah without an iphone it's not possible to test it, the change the in the PR looked good to me so if that don't work, I am out of ideas.
@baris
Thanks, I won't trouble you...
As someone with an iPhone, I'll try to solve the problem myself...
However, as the platform manager, I'd appreciate some direction from you:
Do you have any idea how to test the issue?
What is it related to? The plugin itself or the core?
I'll try to investigate it myself, I just need a hint regarding what it might be connected to...
Thanks! -
I saw that there is an exception for the Safari browser that the Service Worker is not installed, is there a reason for this?
This makes it impossible to subscribe to push notifications in the Safari browser even when installing the page as a PWA application
I think all the functionality is in nodebb-plugin-web-push. The notification is shown here https://github.com/NodeBB/nodebb-plugin-web-push/blob/main/static/web-push.js#L16. You can check there if every chat message push comes with a different body or if they are identical.
-
I think all the functionality is in nodebb-plugin-web-push. The notification is shown here https://github.com/NodeBB/nodebb-plugin-web-push/blob/main/static/web-push.js#L16. You can check there if every chat message push comes with a different body or if they are identical.
@砖谞讬讗讜专-砖诪讞 my best guess is that the notification is received by web push and it does not correctly "rescind" the push notification when issuing a new one with an identical merge id
However if it works with Android then that suggests that it may be an apple specific issue with how they handle notification rescind. Maybe they require additional data the plugin doesn't send.
Hopefully they support notification rescind.
-
@julian
That's correct, but this change resolved the issue:>Title:
>> Fix for Chat Notifications Merging Logic in src/notifications.js
>>
>Body:
>> Hi NodeBB team,
>> I wanted to share a fix for an issue where chat notifications were not merging correctly, resulting in multiple separate alerts instead of a single grouped notification (e.g., "3 new messages from user").
>> The Issue:
>> The core logic responsible for finding related notifications was sometimes failing to identify previous chat alerts, preventing them from being merged into a single summary notification.
>> The Solution:
>> We modified the merging logic in src/notifications.js to ensure that chat notifications are correctly identified and merged based on their unique identifiers.
>> Here is the corrected code block for the Notifications.push logic:
>>javascript >> if (notification.mergeId) { >> const related = await Notifications.findRelated([notification.mergeId], `uid:${uid}:notifications:unread`); >> const merged = await Notifications.getMultiple(related).then(Notifications.merge); >> if (merged.length) { >> notification = merged.pop(); >> notification.pushed = false; // Ensure it re-triggers the push for the update >> } >> } >> >>
>> Why this works:
>> By forcing notification.pushed = false after merging, we ensure that the updated notification (containing the new count and content) is sent out again, correctly reflecting the merged state to the user.
>> I hope this helps anyone experiencing similar issues with notification grouping!
>> Best regards.The question is whether this is a professional and proper solution, or if it would be better to find a different one?
-
@julian
That's correct, but this change resolved the issue:>Title:
>> Fix for Chat Notifications Merging Logic in src/notifications.js
>>
>Body:
>> Hi NodeBB team,
>> I wanted to share a fix for an issue where chat notifications were not merging correctly, resulting in multiple separate alerts instead of a single grouped notification (e.g., "3 new messages from user").
>> The Issue:
>> The core logic responsible for finding related notifications was sometimes failing to identify previous chat alerts, preventing them from being merged into a single summary notification.
>> The Solution:
>> We modified the merging logic in src/notifications.js to ensure that chat notifications are correctly identified and merged based on their unique identifiers.
>> Here is the corrected code block for the Notifications.push logic:
>>javascript >> if (notification.mergeId) { >> const related = await Notifications.findRelated([notification.mergeId], `uid:${uid}:notifications:unread`); >> const merged = await Notifications.getMultiple(related).then(Notifications.merge); >> if (merged.length) { >> notification = merged.pop(); >> notification.pushed = false; // Ensure it re-triggers the push for the update >> } >> } >> >>
>> Why this works:
>> By forcing notification.pushed = false after merging, we ensure that the updated notification (containing the new count and content) is sent out again, correctly reflecting the merged state to the user.
>> I hope this helps anyone experiencing similar issues with notification grouping!
>> Best regards.The question is whether this is a professional and proper solution, or if it would be better to find a different one?
@砖谞讬讗讜专-砖诪讞 is that a pull request open to the plugin? I don't see it.
Hello! It looks like you're interested in this conversation, but you don't have an account yet.
Getting fed up of having to scroll through the same posts each visit? When you register for an account, you'll always come back to exactly where you were before, and choose to be notified of new replies (either via email, or push notification). You'll also be able to save bookmarks and upvote posts to show your appreciation to other community members.
With your input, this post could be even better 馃挆
Register Login