Fix rich push notifications for single-instance situations #2296
Conversation
|
@tbroyer Thanks for opening this PR! Could you provide some more detail on what bug this fixes exactly? I don't use push notifications myself, so I'm not super familiar with this part of the code. Thanks! |
|
Ah I see, I missed #1663. I will study to see what is going on here. |
|
I'm confused about what's going on here and what this fixes. Maybe my brain is dead because it's Sunday morning, but I just don't get #1663 at all. #1663 (comment) |
| const knownInstances = await getKnownInstances() | ||
| if (knownInstances.length !== 1) { | ||
| // TODO: find a way to determine the pushing instance from the PushEvent | ||
| await showSimpleNotification(data) |
There was a problem hiding this comment.
It looks like we are eagerly showing a simple notification rather than a rich one, whereas the original logic was to show a rich one and fallback to a simple one. Why would we prefer to always show a simple notification rather than a rich one, when there is only one instance in the known instances list?
There was a problem hiding this comment.
More details in #1663 (comment)
The problem is that there's no way to compute the origin from the event alone.
There was a problem hiding this comment.
So since event.target.origin is undefined, that means that showRichNotification is going to fail 100% of the time. Why even keep this code around?
pinafore/src/service-worker.js
Lines 175 to 179 in 380d2a0
There was a problem hiding this comment.
The idea here is that:
- if we only know about a single instance, the notification should come from it so we use it (line 181) and show a rich notification
- otherwise, we have no way to determine the
origin, so fallback to a simple notification.
Pending deeper changes on the way enabling notifications is handled (e.g. only allow enabling notifications on a single instance) or changes on Mastodon's side (including the URL to the notification, or at least the instance, in the PushEvent), this PR only fixes things in the case where the user logged in to a single instance. Not ideal but that's the simplest change to at least get rich notifications for some users (most of them? a good proportion of them? I'm certainly one of them).
Partial fix/workaround for #1663
| @@ -169,8 +171,14 @@ self.addEventListener('fetch', event => { | |||
| self.addEventListener('push', event => { | |||
| event.waitUntil((async () => { | |||
| const data = event.data.json() | |||
| const { origin } = event.target | |||
There was a problem hiding this comment.
OK so this code was literally doing nothing because event.target.origin is undefined.
|
|
||
| const origin = basename(knownInstances[0]) |
There was a problem hiding this comment.
This is an unfortunate solution because a user might log into 2 instances and enable notifications on the second one.
It seems like a better system would be for us to log somewhere which instance they've enabled notifications for. I.e. we can enforce the "you can only have notifications for one instance at once time" system ourselves.
There was a problem hiding this comment.
Indeed it's not ideal. For the case where a user logged into 2 instances, they'd have "simple" notifications even if they did enable notifications only on one of them.
Note that this line gets the single element from knownInstances, as the case where there are more than one (more accurately, not exactly one) is handled above; so it's not a matter of "enabling notifications on the second one".
…nlawson#2296) Partially addresses nolanlawson#1663. Co-authored-by: Nolan Lawson <nolan@nolanlawson.com>
No description provided.