Push Notifications setting lost again #411
Comments
|
Are you sure this was fixed at one point? I think it just happens randomly. And seems like the latest changes didn't work. It should resubscribe you automatically if your push subscription expired and you were subscribed before. |
|
No, I am not sure it was fixed. I thought it was reported as fixed, but perhaps that was just optimistic :) Let me know if there's any additional information I can provide to help debug further. |
Ah, I see, haha, yes, we were hoping in this release it would be fixed now with these changes.
Thanks for the offer, now that you ask, there might indeed be something: Can you confirm if anything is saved as
Other than that, I don't think there is any more information we can request at the moment. I also just noticed we don't log user ids during web push errors so we can't associate errors with users with specific subscriptions at the moment. Will push a fix soon. Opt-in client-side logging would be useful at this point for debugging now. I have only worked with Sentry so far and Sentry seems overkill for us at this stage. This website mentions some simpler alternatives including just rolling our own code. @huumn, what do you think about client-side logging? |
|
I'm for it. Two requirements:
|
On my desktop, yes. I don't know how to check this on my iOS device though, which is where I noticed the problem. |
|
Can confirm this isn't fixed on iOS at least |
|
Same, my setting gets lost every few days it seems. |
|
#463 worked and I think we found the culprit thanks to a HN post:
-- https://webkit.org/blog/10218/full-third-party-cookie-blocking-and-more/ So this means your Apple devices delete the existing subscription in IndexedDB and then, when the service worker checks if the subscription needs an update, it does not find a previous one. Therefore, it does not send the updated subscription to our server: sw/index.js async function handlePushSubscriptionChange (oldSubscription, newSubscription) {
// https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerGlobalScope/pushsubscriptionchange_event
// fallbacks since browser may not set oldSubscription and newSubscription
oldSubscription ??= await storage.getItem('subscription')
newSubscription ??= await self.registration.pushManager.getSubscription()
if (!newSubscription) {
// no subscription exists at the moment
return
}
if (oldSubscription?.endpoint === newSubscription.endpoint) {
// subscription did not change. no need to sync with server
return
}This updated subscription is also not stored in IndexedDB then since that only happens if you click the button or an updated subscription was sent to the server. What happens then is my assumption: At this point, there is still an active subscription on your device (so the checkbox in /notifications is still checked) but the server cannot send you any events anymore (since it doesn't have the updated subscription). I think this subscription expires at some point without an update since it was never used by the server. Then your checkbox is also unchecked.
I haven't fully confirmed this since there might have been some changes (the article is from 2020) but I think this is it. |
|
Mhh, but reading the resource thoroughly:
What counts as a "user interaction"? Anything? Then IndexedDB should not have been cleared.
Web apps added to home screen = PWAs? Also, actual use resets the timer? The timer does not seem to have been reset here. So either:
edit: 4) or of course, this is not the real reason For sake of completeness, here the related log lines: log 51 and 54 show that there was still a push subscription. I just checked the code and this line is printed if the service worker has no active subscription anymore: newSubscription ??= await self.registration.pushManager.getSubscription()
if (!newSubscription) {
// no subscription exists at the moment
messageChannelPort?.postMessage({ message: '[sw:handlePushSubscriptionChange] no existing subscription found' })
return
}So "existing" does not mean there is no subscription stored in IndexedDB at the moment. It means the service worker itself has no push subscription (=> checkbox unchecked). Therefore, the issue is because the service worker just lost the push subscription. However, the resource also mentions that registrations are deleted:
So this may still be the reason, I just missed that detail that it's not IndexedDB that we see to be suddenly empty, it's the actual registration of the service worker. And with that, push subscriptions are probably also gone. |
|
This might be a red herring but it looks like we initially think service workers aren't supported, then we register one. |
|
Yes, it looks weird in the logs but this happens for every device. IIRC this is because of render order / |
|
I've created a 500k bounty for this: https://stacker.news/items/277155 Also, we've learned this isn't strictly iOS. It also affects android devices |
This makes me wonder if we are replacing service worker registrations and that is causing the existing I verified that the subscription is still stored in IndexedDB, but this line indicates that there is no I also tried re-subscribing and verified that there is no expiration time set on the subscription, so that eliminates expiration as the source of this issue. I don't know enough about service workers, but these lines feel like we're replacing the service worker registration, which could lead to removal of the push subscription, though if that was the case, I'd expect this to be a more persistent issue. Footnotes |
Yes, that's also what I would except. This means it should happen on every deployment which doesn't seem to be the case. Unfortunately, it's hard for me to investigate this properly since I do not have this issue even though there was also someone on Android experiencing this issue. However, I have an idea for a possible workaround (without really fixing the underlying issue since we don't know what is causing this): We could delete the push subscription in IndexedDB if the user manually unchecks the push subscription. In that case, we know that if the service worker has no push subscription but there is a push subscription in IndexedDB during So this means that during stacker.news/components/serviceworker.js Lines 86 to 92 in e24e2a6 And then, during stacker.news/components/serviceworker.js Line 112 in e24e2a6 stacker.news/sw/eventListener.js Lines 108 to 117 in e24e2a6 |
|
I just realized that I've been using the wrong function signature for Line 74 in e24e2a6 stacker.news/sw/eventListener.js Lines 100 to 102 in e24e2a6 But I think that's unrelated to this bug since most browsers don't support this event anyway and it's the right function signature for the workaround code: stacker.news/sw/eventListener.js Lines 155 to 157 in e24e2a6 |
My only concern is if this is "OK" to do since it wouldn't be driven by an explicit user interaction? To register a new push subscription automatically under certain conditions. Just food for thought. I like the workaround in general, should improve the experience! |
Good point but I think it's okay since this only works if permissions were already granted. We're acting in the interest of our users here since they told us they want push notifications. The user can always opt-out via browser settings. The "game theory" of permissions is that applications should behave with the permissions they were granted else the user will remove permission via the browser which is a "nuclear option" since the application won't even be able to ask for permission again then:
-- https://web.dev/articles/push-notifications-permissions-ux#the_bad_ux |
|
Just lost mine again today ... after what felt like longer than normal at least! |
|
Mhh, I think this was the related log message:
Since it says Maybe the resubscribe code will work now for you |
|
Okay cool, will keep commenting here when they're lost |
|
lol, it seems it's getting worse since it seems I am also starting to lose my push subscription now, lol At least the checkbox is unchecked sometimes ...
btw, can you also comment if you ever receive a random "Stacker News notifications are active now"? :) |
|
I lose my notifications daily now and receive the "Stacker News notifications are active now" nearly every time I open the app, and it's a coinflip if my checkbox is checked. It's super super broken. I suspect we're misunderstanding something about how these work on phones. |
What does "daily now" mean? How many days already?
Yeah I wonder if I just never noticed that my checkbox might be unchecked ... i haven't tested if I might still receive push notifications even if it "seems" unchecked But weird, I think we haven't changed anything in the service worker for a while? 0b872cb is from Nov 9
I suspect the only way we will be able to ever know what is going wrong is to get really serious with logging. According to this, it's not really magic how this stuff is supposed to work:
But I might just have to read the RFC in full, anyway. Maybe there is some useful information in there. Or is the site we're building hitting some weird edge cases in the spec? Too many push notifications getting sent without some weird dance for the push service? Is our use case not the average use case for push notifications? Speculations, speculations, lol Another hope of mine is that if I implement push notifications for https://delphi.market/ (which uses Vue and Golang), I could use this implementation to send regular test push notifications to try to reproduce our issue on SN. If it's not reproduce-able there ... bad luck, but still some valuable information, I guess. |
|
It's been broken for awhile not just recently. |
|
I wonder if this might be related to storage getting cleared when a device is running out of space. I have an old iphone that's constantly evicting apps and is probably also evicting pwa storage. Perhaps we could use persistent storage? https://developer.mozilla.org/en-US/docs/Web/API/StorageManager/persist |
Ohh, my phone is also constantly running out of space. This might be indeed the reason that I also started to lose my push subscription recently.
Sounds like a good idea! Btw, this never happened on desktop, right? (or actually, not sure, it might have happened on desktop for me, too) 🤔 |
|
I don't have them enabled on desktop. I always have a tab open |
|
👀
-- https://web.dev/learn/pwa/offline-data#data_persistence Will create PR soon. If I understand this right, it's really just one simple API call. Btw, just checked my quota in my brave desktop session (just out of interest): |
|
Are we maybe accidentally doing silent pushes? https://dev.to/progressier/how-to-fix-ios-push-subscriptions-being-terminated-after-3-notifications-39a7 We're using |
I think returning a promise inside a promise is fine. As far as my JS knowledge goes, there is no difference for the calling context if you do But we might be doing silent pushes elsewhere. I vaguely remember that I was wondering if X could cause a silent push but I no longer know what X was 🤔 update: Yeah,
I even mentioned that this could cause silent pushes but again, I didn't consider vastly different or even completely breaking behavior on iOS:
Good catch! I think this is definitely an issue. |
I don't think it is. What happens here afaict is by returning a promise the |
Mhh, I see. I also see how my argument might have been too hand-wavy. I'll create a PR with I'll also think about |
|
Basically this function can only return a promise if that promise resolves, and only resolves, if |
|
For bookkeeping purposes, I'll write down here what we've just seen in our logs and discussed internally on Slack: We've noticed that this if block gets triggered in iOS: // fetch existing notifications with same tag
const notifications = await sw.registration.getNotifications({ tag })
// since we used a tag filter, there should only be zero or one notification
if (notifications.length > 1) {
const message = `[sw:push] more than one notification with tag ${tag} found`
messageChannelPort?.postMessage({ level: 'error', message })
console.error(message)
return null
}because we've found a lot of
in our logs. This makes sense since when we released push notifications, we realized pretty quick that This is another silent push because of which Safari might cancel push subscriptions. However, reading the last response from someone working at Apple or WebKit (?) got me confused:
Afaict, this doesn't add up with what we're seeing on iOS. According to the log message, tl;dr: Next attempt to fix this issue: Create PR which does not rely on
|
|
Regarding UX on Android since #719: It still works but I can definitely tell that it no longer uses the native So I think it makes sense to only run the code which does not depend on |
|
Agreed. We don't want to degrade android UX because iOS is behind. Also, I've maintained my notifications longer than usual but I did lose them once yesterday. It might've just been a fluke. I also spent sometime looking around for any clues about potential causes. It looks like many other devs have the same issue: https://developer.apple.com/forums/thread/728796 |
|
I have access to an iPhone with Safari on iOS 17.1 now. I was able to gather some information about what works and doesn't work. The columns of the log messages are:
"slow zaps": zapping the same post three times and waiting until notification shows up on device between zaps:Conclusions:
"fast zaps": multiple zaps in quick successionConclusions:
"slow" replies
Conclusions:
Or in other words: The data of the last notification has // this should reflect the amount of notifications that were already merged before
log(`[sw:push] ${Date.now()} ${nid} - notifications: ${JSON.stringify(currentNotifications.map(({ data }) => JSON.stringify(data)))}`)
const initialAmount = currentNotifications[0].data?.amount || 1
log(`[sw:push] ${Date.now()} ${nid} - initial amount: ${initialAmount}`)
const mergedPayload = currentNotifications.reduce((acc, { data }) => {
let newAmount, newSats
if (AMOUNT_TAGS.includes(compareTag)) {
newAmount = acc.amount + 1
}
if (SUM_SATS_TAGS.includes(compareTag)) {
newSats = acc.sats + data.sats
}
const newPayload = { ...data, amount: newAmount, sats: newSats }
return newPayload
}, { ...incomingData, amount: initialAmount })I think that we show the wrong amount of replies is fixable on our side by adding more iOS-specific logic. Will do this tomorrow. replying and closing notifications manually (swiping them away)Conclusion:
Will add a comment to the WebKit bug report tomorrow and ask for clarification about the behavior of
|
|
Just leaving a note that this seems to have been happening a lot more than usual lately (past few days). |
Description
It appears I have again lost the setting where I enabled push notifications. I think this was an issue in the past that was fixed at one point.
Steps to Reproduce
Open PWA and find Push Notifications disabled on the notifications page when it was enabled previously.
Expected behavior
Push notifications remains enabled if enabled.
Screenshots
N/A
Logs
N/A
Environment:
iOS PWA
Additional context
N/A
cc @ekzyis
The text was updated successfully, but these errors were encountered: