Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add PWA manifest, service worker, and web push #751

Merged
merged 82 commits into from
Jun 24, 2023
Merged

Add PWA manifest, service worker, and web push #751

merged 82 commits into from
Jun 24, 2023

Conversation

nimbleghost
Copy link
Contributor

@nimbleghost nimbleghost commented May 24, 2023

Resolves #346
Resolves #199

Adds a PWA manifest, service worker, the required go server routes, and VAPID-based web push

I can't test Android, but this is what it looks like on macOS and iOS:

chrome_install_prompt chrome_installed
ios_add ios_opts iOS-subscribe
iOS-notif-1 ios_badge ios_opts

@codecov-commenter
Copy link

codecov-commenter commented May 24, 2023

Codecov Report

Patch coverage: 67.63% and project coverage change: +0.10 🎉

Comparison is base (4c7dc4c) 65.70% compared to head (141565d) 65.80%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #751      +/-   ##
==========================================
+ Coverage   65.70%   65.80%   +0.10%     
==========================================
  Files          50       53       +3     
  Lines        7342     7648     +306     
==========================================
+ Hits         4824     5033     +209     
- Misses       1728     1777      +49     
- Partials      790      838      +48     
Impacted Files Coverage Δ
server/errors.go 70.00% <ø> (ø)
server/log.go 78.84% <ø> (ø)
server/server_account.go 65.99% <33.33%> (-0.68%) ⬇️
server/webpush_store.go 59.13% <59.13%> (ø)
server/server_webpush.go 61.22% <61.22%> (ø)
server/server.go 63.99% <77.50%> (+0.48%) ⬆️
cmd/serve.go 60.07% <83.33%> (+1.06%) ⬆️
cmd/webpush.go 85.71% <85.71%> (ø)
server/config.go 100.00% <100.00%> (ø)
server/message_cache.go 66.38% <100.00%> (ø)
... and 3 more

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

server/server.go Outdated Show resolved Hide resolved
@binwiederhier
Copy link
Owner

This looks great. I have no idea what's involved to make the web push part work. I always assumed that ntfy will be a push provider, but my understanding is that that's not possible.

@nimbleghost
Copy link
Contributor Author

I always assumed that ntfy will be a push provider, but my understanding is that that's not possible.

It does work! I have it working with VAPID keys and no intermediate (e.g. Firebase) required. Pushing now - I will send you a message on Discord to discuss how to configure it though, that's what I'm looking at currently.

@nimbleghost nimbleghost changed the title Add PWA Add PWA manifest, service worker, and web push May 25, 2023
@nimbleghost nimbleghost marked this pull request as ready for review May 29, 2023 17:36
server/server.go Outdated Show resolved Hide resolved
docs/config.md Show resolved Hide resolved
docs/subscribe/desktop.md Outdated Show resolved Hide resolved
docs/subscribe/desktop.md Outdated Show resolved Hide resolved
docs/subscribe/web.md Outdated Show resolved Hide resolved
server/server.go Outdated Show resolved Hide resolved
server/server.yml Outdated Show resolved Hide resolved
server/web_push.go Outdated Show resolved Hide resolved
server/web_push.go Outdated Show resolved Hide resolved
web/public/config.js Show resolved Hide resolved
@nimbleghost
Copy link
Contributor Author

thanks for the review and good questions. I'll respond in detail when I have some free time or after work.

(I'm going to have a lot of nit-picky responses. Please do know that I am extremely impressed by what you have delivered here. But I am a little OCD about this stuff.)

much appreciated, I'm only vaguely familiar with go so I'm learning the patterns / idiomatic way of doing things as I go (haha).

@binwiederhier
Copy link
Owner

binwiederhier commented May 30, 2023

Did some work. Not even close to done.

  • 8f2120e changes some names
  • 0d2f0dd moves stuff to a new file
  • ad761f4 makes things more Go-y, and switches to user_id in the database
  • 35ac095 moves the config to config.js

Things I've noticed:

  • I love that the subscription drop down now has icons. Random change, but love it!
  • I do not for the life of me understand what "Sound only" means. I didn't look at the code, but from a user perspective, that's odd
  • I have that the "SubscribeDialog" now has 4(!) toggles. I don't think I want to even give people the option to opt-out of desktop notifications. If they want to, they can just mute the topic. That way, it'll be just 3 options (or we could even drop the web-push one and just make it available in the drop down, idk yet.

Lots of TODOs. Here are some major things:

  • The subscribe/unsubscribe endpoints should be POST + DELETE to the same endpoint v1/webpush/$topic or something
  • The unsubscribe endpoint does not work at all for me, the payload is empty

(I'll do more tonight, likely)

@nimbleghost
Copy link
Contributor Author

nimbleghost commented May 30, 2023

I love that the subscription drop down now has icons. Random change, but love it!

:D added the checkmarks for the type and then the rest of the list looked empty


I do not for the life of me understand what "Sound only" means. I didn't look at the code, but from a user perspective, that's odd

fair. the reasoning was that if a user denied browser notification access at all, I wanted there to be a way to represent that. or if the browser/context doesn't support it, e.g. http or opening a site in incognito. how would you represent that? It's distinct from muting the topic; I wouldn't want to force a topic muted state if the notification APIs were unavailable/denied. Maybe "No notifications"?


I have that the "SubscribeDialog" now has 4(!) toggles. I don't think I want to even give people the option to opt-out of desktop notifications. If they want to, they can just mute the topic. That way, it'll be just 3 options (or we could even drop the web-push one and just make it available in the drop down, idk yet.

Hmm, the desktop notification toggle was again browser notification opt-in UX driven. When you first visit the website and click subscribe, the toggle defaults to off:

CleanShot 2023-05-30 at 21 17 19@2x

You tap it, then you either click allow and it goes to enabled, or you see this flow if denied or unavailable (browsers will silently deny in incognito for example):

CleanShot 2023-05-30 at 21 18 17@2x

Maybe the solution is to have a little dialog that shows up when you subscribe for the first time, asking to enable notifications first. I do agree that there are too many toggles (esp. when logged in).


relatedly, I'm wondering if the "reserve topic" toggle could be replaced with just the dropdown with a default "public" setting.


The subscribe/unsubscribe endpoints should be POST + DELETE to the same endpoint v1/webpush/$topic or something

that's what I wanted initially, but you can't have a DELETE body, and we need to specify the endpoint we want to unsubscribe. since it's a URL it's inconvenient to embed in a URL, or it could be DELETE /topic/webpush/<subscription-id>..

The unsubscribe endpoint does not work at all for me, the payload is empty

which payload? it works for me:

$ curl https://<server>/test/web-push/unsubscribe \
  -d '{"endpoint":"https://fcm.googleapis.com/fcm/send/<xyz>"}' \
  -H "Authorization: Bearer xxx"

{"success":true}

@nimbleghost
Copy link
Contributor Author

Alright, addressed most of the review comments. What's remaining:

Do you see anything else?

@binwiederhier
Copy link
Owner

Additional action items:

  • "Bulk endpoints"?
  • Changing endpoints to v1/account/webpush
  • Subscription expiration (+ warning notification)

I'll try to actually get some time to try it tomorrow and on the weekend.

@skurfuerst
Copy link

Hey @nimbleghost and @binwiederhier ,

Just wanted to give a big THANK YOU to the both of you ❤️❤️! For my use cases, this makes ntfy even more helpful.

Thanks so much and keep up the great work 🥰
Sebastian

nimbleghost and others added 14 commits June 7, 2023 20:38
- Use new notification request/opt-in flow for push
- Implement unsubscribing
- Implement muting
- Implement emojis in title
- Add iOS specific PWA warning
- Don’t use websockets when web push is enabled
- Fix duplicate notifications
- Implement default web push setting
- Implement changing subscription type
- Implement web push subscription refresh
- Implement web push notification click
Rely directly on getting it from the browser
- Use a single endpoint
- Use a declarative web push sync hook. This thus handles all edge cases
  that had to be manually handled before: logout, login, account sync,
  etc.
- Simplify UX: browser notifications are always enabled (unless denied),
  web push toggle only shows up if permissions are already granted.
server/webpush_store.go Outdated Show resolved Hide resolved
server/webpush_store.go Show resolved Hide resolved
binwiederhier and others added 9 commits June 17, 2023 14:44
URL heuristic is the second check if there is no mime
There are sometimes edge cases on iOS which cause the app to crash,
it’s good to have a reload button as there’s no browser chrome (reload,
back, forward) in an iOS standalone PWA.
server/server_account_test.go Outdated Show resolved Hide resolved
server/server_webpush_test.go Show resolved Hide resolved
@binwiederhier binwiederhier merged commit 271056a into binwiederhier:main Jun 24, 2023
@nimbleghost nimbleghost deleted the pwa branch June 25, 2023 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Web Push + PWA, for background notifications Web Push support
5 participants