Is Safari intentionally excluded from the Service Worker installation?
-
@שניאור-שמח 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.
