Is Safari intentionally excluded from the Service Worker installation?
-
@שניאור-שמח are chats the only type of notification failing at merging?
@baris web push is a direct reflection of regular notifications so if they share merge id, the old push notif is removed and replaced
-
@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.
-
-
-
The code in that pull request isn't merged yet, so it is not active in the latest version of nodebb. Once it's merged and published you can install and test it. Or you can replace the plugin with your own version for testing.
-
@baris This is what I did, I installed the plugin with the changes and it still didn't help..
What I'm trying to explain is that the change my friend made for message merging didn't help the problem..
So I would appreciate it if you could see what the problem is and fix it..
Thank you -
@שניאור-שמח unfortunately I don't have an iphone so I can't test the changes.
-
@baris I have an iPhone, I'll check what's needed, but you guys do what's needed to fix the bug and I'll check if it worked or not..
I just don't understand the system very well, and I wasn't able to fix the problem on my own.. -
> @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
)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
-
@baris
What's the end? Will you be able to fix this? Even though you don't have an iPhone? -
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 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
-
@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.
