Is Safari intentionally excluded from the Service Worker installation?
-
I think that might be just the notification merging that happens in core, not something related to service worker.
-
-
@שניאור-שמח can you share a screenshot of the issue you are seeing, is it the notification in nodebb or the email notification?
-
-
These are push notifications so they are likely handled in the web-push plugin, I see a PR there https://github.com/NodeBB/nodebb-plugin-web-push/pull/64 that might fix this issue @julian
-
@baris That's right, that's what I have installed, but it only helped the notifications appear on the iPhone in the Safari browser, but it still didn't help with the notification merging problem.
So I ask you to try to sort this out.
thanks. -
@שניאור-שמח 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
