[Feature] Add PWA Push notifications and Shortcuts#29072
[Feature] Add PWA Push notifications and Shortcuts#29072webalexeu wants to merge 1 commit intoevcc-io:masterfrom
Conversation
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The
i18nLookuphelper re-reads and unmarshals the i18n JSON file on every manifest request and lookup; consider caching the parsed language maps (or at least the raw file bytes) to avoid repeated disk I/O and JSON decoding on each/meta/site.webmanifestcall. - In
sw.js, thepushsubscriptionchangehandler assumesevent.oldSubscription(and its.options) is always present and does not senduserAgentlike the initial subscription path; it may be worth hardening this flow (null checks, try/catch aroundsubscribeandfetch, and consistent payload structure) to avoid silent failures or inconsistent records.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `i18nLookup` helper re-reads and unmarshals the i18n JSON file on every manifest request and lookup; consider caching the parsed language maps (or at least the raw file bytes) to avoid repeated disk I/O and JSON decoding on each `/meta/site.webmanifest` call.
- In `sw.js`, the `pushsubscriptionchange` handler assumes `event.oldSubscription` (and its `.options`) is always present and does not send `userAgent` like the initial subscription path; it may be worth hardening this flow (null checks, try/catch around `subscribe` and `fetch`, and consistent payload structure) to avoid silent failures or inconsistent records.
## Individual Comments
### Comment 1
<location path="server/http.go" line_range="272" />
<code_context>
+
+ push.Methods(http.MethodGet).Path("/vapidkey").Handler(pushVapidKeyHandler())
+ push.Methods(http.MethodGet).Path("/check").Handler(pushCheckHandler())
+ push.Methods(http.MethodPost, http.MethodOptions).Path("/subscribe").Handler(pushSubscribeHandler())
+ }
+
</code_context>
<issue_to_address>
**issue:** OPTIONS preflight is routed to a handler that expects a JSON body, likely returning 400 on preflight
`pushSubscribeHandler` is used for both POST and OPTIONS, but it always tries to decode a JSON body and returns 400 on errors. For CORS preflight or other body-less OPTIONS requests, this will likely fail the subscription flow. Either rely on gorilla’s default OPTIONS handling by removing `http.MethodOptions` here, or short-circuit OPTIONS in `pushSubscribeHandler` (e.g., return 204 before attempting to decode the body).
</issue_to_address>
### Comment 2
<location path="assets/public/sw.js" line_range="24-26" />
<code_context>
+});
+
+// Handle browser-initiated subscription rotation (endpoint expiry).
+self.addEventListener("pushsubscriptionchange", (event) => {
+ event.waitUntil(
+ self.registration.pushManager
+ .subscribe(event.oldSubscription.options)
+ .then((subscription) => {
</code_context>
<issue_to_address>
**issue (bug_risk):** pushsubscriptionchange handler assumes oldSubscription/options are always present
`event.oldSubscription` (and its `options` property) isn’t guaranteed to be defined across browsers, so this can throw at runtime. Please guard access to `oldSubscription.options` and either fall back to a sane default (e.g. `{ userVisibleOnly: true, applicationServerKey: ... }`) or skip resubscribe if options are unavailable.
</issue_to_address>
### Comment 3
<location path="server/push/vapid.go" line_range="29-32" />
<code_context>
+ return "", "", err
+ }
+
+ settings.SetString(keyVAPIDPrivate, private)
+ settings.SetString(keyVAPIDPublic, public)
+
+ return private, public, nil
+}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Errors from persisting VAPID keys are ignored, which can hide configuration/storage issues
Both `settings.SetString` calls ignore their errors. If persistence fails (e.g. storage/DB issues), you still return the in-memory keys, and a restart may generate a new pair and break existing subscriptions. At minimum, log these errors or propagate them so callers can decide how to handle failure.
Suggested implementation:
```golang
if err := settings.SetString(keyVAPIDPrivate, private); err != nil {
return "", "", fmt.Errorf("persisting VAPID private key: %w", err)
}
if err := settings.SetString(keyVAPIDPublic, public); err != nil {
return "", "", fmt.Errorf("persisting VAPID public key: %w", err)
}
return private, public, nil
}
```
- Ensure `fmt` is imported at the top of `server/push/vapid.go` if it is not already:
- `import "fmt"`
- This change assumes `settings.SetString` returns an `error`. If its signature is different, adjust the error handling accordingly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| settings.SetString(keyVAPIDPrivate, private) | ||
| settings.SetString(keyVAPIDPublic, public) | ||
|
|
||
| return private, public, nil |
There was a problem hiding this comment.
suggestion (bug_risk): Errors from persisting VAPID keys are ignored, which can hide configuration/storage issues
Both settings.SetString calls ignore their errors. If persistence fails (e.g. storage/DB issues), you still return the in-memory keys, and a restart may generate a new pair and break existing subscriptions. At minimum, log these errors or propagate them so callers can decide how to handle failure.
Suggested implementation:
if err := settings.SetString(keyVAPIDPrivate, private); err != nil {
return "", "", fmt.Errorf("persisting VAPID private key: %w", err)
}
if err := settings.SetString(keyVAPIDPublic, public); err != nil {
return "", "", fmt.Errorf("persisting VAPID public key: %w", err)
}
return private, public, nil
}- Ensure
fmtis imported at the top ofserver/push/vapid.goif it is not already:import "fmt"
- This change assumes
settings.SetStringreturns anerror. If its signature is different, adjust the error handling accordingly.
b771371 to
915d898
Compare
| } | ||
|
|
||
| // Always add the Web Push (PWA) sender — it sends to browser-subscribed clients. | ||
| messageHub.Add(push.NewSender()) |
There was a problem hiding this comment.
I'm not convinced this is something we'd want. At least not without explicit opt-in.
There was a problem hiding this comment.
I'm not convinced this is something we'd want. At least not without explicit opt-in.
You can control the opt-in on app side by default on PWA
Not sure it is relevant to control it on evcc side, what do you think ?
There was a problem hiding this comment.
Dunno- would it interfere with browser users?
There was a problem hiding this comment.
Dunno- would it interfere with browser users?
Well, that's the same flag for browser or pwa if I'm not mistaken
There was a problem hiding this comment.
You prefer to enable this a notification service where user need to opt-in explicitely ?
I try to compare with other products and I think they all keep usually control within app
04185ac to
1219c4a
Compare
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- In
registerServiceWorker, the/api/push/checkcall is not wrapped in a try/catch; if the network request fails it will throw and abort the rest of the logic, so consider catching errors there to avoid silently skipping (or unnecessarily dropping) existing subscriptions in transient failure cases. - In
webmanifestHandler, a template parse error callslog.FATAL.Fatal, which will terminate the process; it would be safer to log the error and return a 5xx response so a malformed manifest template does not bring down the entire server. - The
Sender.Sendimplementation loads all push subscriptions into memory in one go; if the number of subscriptions grows large, consider iterating in batches or with a streaming approach to avoid potential memory and latency issues on large installations.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `registerServiceWorker`, the `/api/push/check` call is not wrapped in a try/catch; if the network request fails it will throw and abort the rest of the logic, so consider catching errors there to avoid silently skipping (or unnecessarily dropping) existing subscriptions in transient failure cases.
- In `webmanifestHandler`, a template parse error calls `log.FATAL.Fatal`, which will terminate the process; it would be safer to log the error and return a 5xx response so a malformed manifest template does not bring down the entire server.
- The `Sender.Send` implementation loads all push subscriptions into memory in one go; if the number of subscriptions grows large, consider iterating in batches or with a streaming approach to avoid potential memory and latency issues on large installations.
## Individual Comments
### Comment 1
<location path="assets/js/utils/push.ts" line_range="49" />
<code_context>
+ }
+
+ // Request notification permission (shows browser prompt if not yet decided).
+ const permission = await Notification.requestPermission();
+ if (permission !== "granted") return;
+
</code_context>
<issue_to_address>
**issue:** Guard `Notification.requestPermission` with a feature check to avoid runtime errors in environments without the Notification API.
In some environments (older browsers, embedded webviews) `Notification` can be undefined, causing a `ReferenceError` here. Since you already feature-check `serviceWorker` and `PushManager`, please also guard this with a check like `"Notification" in window` (or equivalent) and return early if notifications aren’t supported.
</issue_to_address>
### Comment 2
<location path="server/push/subscription.go" line_range="29-38" />
<code_context>
+// AllSubscriptions returns all stored push subscriptions.
+func AllSubscriptions() ([]Subscription, error) {
+ var subs []Subscription
+ if db.Instance == nil {
+ return nil, nil
+ }
+ return subs, db.Instance.Find(&subs).Error
+}
+
+// SubscriptionExists returns true if an endpoint is registered in the DB.
+func SubscriptionExists(endpoint string) (bool, error) {
+ if db.Instance == nil {
+ return false, nil
+ }
+ var count int64
</code_context>
<issue_to_address>
**issue (bug_risk):** `SubscriptionExists` returning false when DB is uninitialized can desync client subscriptions.
When `db.Instance` is nil, `SubscriptionExists` returns `false, nil`, causing `pushCheckHandler` to reply 404. The frontend treats this as “subscription not known” and will unsubscribe/resubscribe, even though the issue is backend state. Consider returning an error here (like `SaveSubscription`/`DeleteSubscription`) so the check endpoint can return 5xx instead of triggering unnecessary subscription churn.
</issue_to_address>
### Comment 3
<location path="server/push/vapid.go" line_range="29-30" />
<code_context>
+ return "", "", err
+ }
+
+ settings.SetString(keyVAPIDPrivate, private)
+ settings.SetString(keyVAPIDPublic, public)
+
+ return private, public, nil
</code_context>
<issue_to_address>
**issue (bug_risk):** Ignored errors when persisting VAPID keys can lead to inconsistent keys between restarts.
If a `SetString` call fails, the function still returns the new keys, but they won’t be persisted. On the next restart a new pair will be generated, breaking existing subscriptions. Please handle the `SetString` errors explicitly—either return an error so callers avoid using non-persisted keys, or at minimum log the failure and document that keys may change if persistence fails.
</issue_to_address>
### Comment 4
<location path="server/http_site_handler.go" line_range="67-76" />
<code_context>
+
+// i18nLookup resolves a dot-separated key path from the i18n map for the given
+// language. Falls back to English if the language or key is not found.
+func i18nLookup(lang string, path ...string) string {
+ lookup := func(m map[string]any) (string, bool) {
+ if m == nil {
+ return "", false
+ }
+ var cur any = m
+ for _, key := range path {
+ node, ok := cur.(map[string]any)
+ if !ok {
+ return "", false
+ }
+ cur = node[key]
+ }
+ val, ok := cur.(string)
+ return val, ok
+ }
+
+ if val, ok := lookup(i18nMap(lang)); ok {
+ return val
+ }
+ if val, ok := lookup(i18nMap("en")); ok {
+ return val
+ }
+ return path[len(path)-1]
+}
+
</code_context>
<issue_to_address>
**nitpick:** Defensive check for empty `path` in `i18nLookup` would avoid a potential panic.
This function assumes `path` is non-empty when doing `path[len(path)-1]`. While current callers always pass at least one segment, an empty `path` in future would cause an index-out-of-range panic. Please add a guard like `if len(path) == 0 { return "" }` to make this helper safe for all callers.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
205a4ce to
39d8039
Compare
- Add Web Push (VAPID) notification support: VAPID key generation/persistence, subscription storage (GORM), push sender integrated into message hub - Add service worker (sw.js) handling push events, subscription rotation, and notification clicks - Auto-register service worker and subscribe on page load (no UI toggle needed) - Add API endpoints: GET /api/push/vapidkey, GET /api/push/check, POST /api/push/subscribe - Auto-remove stale subscriptions on FCM 410/404 responses - Serve site.webmanifest as a template with i18n shortcut names (config, sessions, log) resolved from Accept-Language header; cache parsed maps - Add PWA shortcuts for Configuration, Charging Sessions, and Logs - Fix "purpose:" typo in webmanifest icon entries Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
39d8039 to
728f980
Compare
Improvement on the Progressive Web App by adding push notifications (Fixes #29071)
Summary
Accept-Languageheader.Changes
Backend
server/push/vapid.go— VAPID key pair generation, persisted in the settings DB so keys survive restartsserver/push/subscription.go— GORM model for browser push subscriptions (endpoint, auth, p256dh, user agent), with auto-migration viadb.Registerserver/push/sender.go—api.Messengerimplementation; sends push payloads to all subscriptions, auto-removes stale endpoints on FCM 410/404server/http_push_handler.go— three API endpoints:GET /api/push/vapidkey,GET /api/push/check,POST /api/push/subscribeserver/http.go— registers push API routes and/sw.js(no-cache, root scope); registers/meta/site.webmanifestbefore the generic file serverserver/http_site_handler.go—webmanifestHandlerserves the manifest as a Go template with i18n values injected;i18nLookuphelper with English fallbackcmd/setup.go— unconditionally registerspush.NewSender()in the message hub (zero overhead when no subscriptions)Frontend
assets/public/sw.js— service worker handlingpush,pushsubscriptionchange(endpoint rotation), andnotificationclickeventsassets/js/utils/push.ts—registerServiceWorker(): registers the SW, checks existing subscription against the backend, requests permission, fetches VAPID key, subscribes, and saves to backendassets/js/app.ts— callsregisterServiceWorker()on startupassets/index.html— addsmobile-web-app-capablemeta tagManifest
assets/public/meta/site.webmanifest— adds three PWA shortcuts (Configuration, Charging Sessions, Logs) with i18n names via[[.Title]]template placeholders; fixes pre-existing"purpose:"typo on SVG icon entriesZero-overhead design
The push sender returns immediately if no subscriptions are registered — no VAPID key load, no DB query for payloads beyond a single
COUNT. Adding it unconditionally to the hub requires no configuration flag.Dependencies
SherClockHolmes/webpush-go— VAPID key generation and Web Push deliveryThanks for this great product