From 7cf4161852a6f4d5b1ed31e54527e2ee36fbad2f Mon Sep 17 00:00:00 2001 From: Eli Mallon Date: Wed, 22 Jul 2026 15:17:50 -0700 Subject: [PATCH] fix: address Greptile review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four issues flagged by Greptile on #1211: 1. Expired subscriptions never pruned from blast path. The ExpiredSubscriptionError type existed but no caller unwrapped it to delete dead rows. WebPushNotifier.Blast now collects expired tokens into a BlastsError; ExpiredTokens(err) walks the MultiNotifier → errors.Join → BlastsError tree to extract them. Both blast call sites (queue_processor, sync.go) now prune dead subscriptions after each blast so they don't accumulate and burn time on every notification. 2. HandleVapidPublicKey manually concatenated JSON. Replaced with json.NewEncoder so serialization is always correct. 3. Notifications settings screen forced a re-render every second to refresh Notification.permission. Replaced with the Permissions API onchange listener, which fires only on actual state transitions. 4. disableWebNotifications cleared notificationToken in a finally block even when the server DELETE failed, leaving the user unable to retry while the server kept pushing. Now only clears after a confirmed successful DELETE; on failure the token stays set so the toggle remains "on" and the user can retry. Co-Authored-By: Claude Opus 4.8 --- .../notifications-category-settings.tsx | 20 +++-- js/app/store/slices/platformSlice.ts | 16 +++- pkg/api/api.go | 5 +- pkg/atproto/sync.go | 10 ++- pkg/notifications/multi.go | 12 +-- pkg/notifications/webpush.go | 82 ++++++++++++++++++- pkg/notifications/webpush_test.go | 52 ++++++++++++ pkg/statedb/queue_processor.go | 9 ++ 8 files changed, 179 insertions(+), 27 deletions(-) diff --git a/js/app/components/settings/notifications-category-settings.tsx b/js/app/components/settings/notifications-category-settings.tsx index 9ef4f3034..402c570cd 100644 --- a/js/app/components/settings/notifications-category-settings.tsx +++ b/js/app/components/settings/notifications-category-settings.tsx @@ -46,16 +46,20 @@ export function NotificationsCategorySettings() { ? permission === "granted" && !!notificationToken : permission === "granted"; - // Re-check permission when the screen gains focus (the user may have - // changed it in browser settings). - const [refreshKey, setRefreshKey] = useState(0); + // Re-check permission when the browser's permission state changes (e.g. the + // user toggled it in browser settings while the app is open). + const [, forceRender] = useState(0); useEffect(() => { - if (!isWeb) return; - const interval = setInterval(() => setRefreshKey((k) => k + 1), 1000); - return () => clearInterval(interval); + if (!isWeb || typeof navigator.permissions === "undefined") return; + let status: PermissionStatus | undefined; + navigator.permissions.query({ name: "notifications" }).then((s) => { + status = s; + s.onchange = () => forceRender((k) => k + 1); + }); + return () => { + if (status) status.onchange = null; + }; }, [isWeb]); - // touch refreshKey so the linter doesn't complain and the re-render happens - void refreshKey; const handleToggle = async (value: boolean) => { if (busy) return; diff --git a/js/app/store/slices/platformSlice.ts b/js/app/store/slices/platformSlice.ts index 75e4b2841..426457b16 100644 --- a/js/app/store/slices/platformSlice.ts +++ b/js/app/store/slices/platformSlice.ts @@ -145,19 +145,27 @@ export const createPlatformSlice: StateCreator< if (existing && existing.endpoint === sub.endpoint) { await existing.unsubscribe(); } - // Tell the server to drop the row. + // Tell the server to drop the row. Only clear the local token after + // the DELETE succeeds — otherwise the toggle shows "off" while the + // server keeps pushing to a subscription the user thought they + // disabled, with no way to retry. if (url) { - await fetch(`${url}/api/notification`, { + const res = await fetch(`${url}/api/notification`, { method: "DELETE", headers: { "content-type": "application/json" }, body: JSON.stringify({ token: notificationToken }), }); + if (!res.ok) { + throw new Error(`server delete failed: ${res.status}`); + } } + set({ notificationToken: null }); } } catch (e) { console.error("disableWebNotifications error", e); - } finally { - set({ notificationToken: null }); + // Leave notificationToken set so the toggle stays "on" and the user + // can retry. The browser-side unsubscribe may have already succeeded, + // but the server row is what matters for stopping future pushes. } }, webNotificationPermission: () => { diff --git a/pkg/api/api.go b/pkg/api/api.go index 331ab77c7..bc8a96883 100644 --- a/pkg/api/api.go +++ b/pkg/api/api.go @@ -645,7 +645,10 @@ func (a *StreamplaceAPI) HandleVapidPublicKey(ctx context.Context) http.HandlerF return } w.Header().Set("Content-Type", "application/json") - if _, err := w.Write([]byte(`{"publicKey":"` + keys.PublicKey + `"}`)); err != nil { + resp := struct { + PublicKey string `json:"publicKey"` + }{PublicKey: keys.PublicKey} + if err := json.NewEncoder(w).Encode(resp); err != nil { log.Error(ctx, "error writing vapid public key", "error", err) } } diff --git a/pkg/atproto/sync.go b/pkg/atproto/sync.go index 401a6b3fb..03aa1370a 100644 --- a/pkg/atproto/sync.go +++ b/pkg/atproto/sync.go @@ -981,9 +981,15 @@ func (atsync *ATProtoSynchronizer) notifyBetaInvite(ctx context.Context, rec *pl } if err := atsync.Noter.Blast(ctx, targets, blast); err != nil { log.Error(ctx, "beta invite notification: blast failed", "did", rec.Did, "feature", rec.Feature, "err", err) - return + } else { + log.Log(ctx, "sent beta invite notification", "did", rec.Did, "feature", rec.Feature, "tokens", len(notifications)) + } + // Prune dead web push subscriptions so they don't accumulate. + for _, token := range notificationpkg.ExpiredTokens(err) { + if delErr := atsync.StatefulDB.DeleteNotification(token); delErr != nil { + log.Error(ctx, "beta invite notification: failed to prune expired", "token", token, "err", delErr) + } } - log.Log(ctx, "sent beta invite notification", "did", rec.Did, "feature", rec.Feature, "tokens", len(notifications)) } // betaInviteBlast builds the push payload for a newly-granted beta feature. diff --git a/pkg/notifications/multi.go b/pkg/notifications/multi.go index ae008dee2..a4ec9c5ed 100644 --- a/pkg/notifications/multi.go +++ b/pkg/notifications/multi.go @@ -2,7 +2,7 @@ package notifications import ( "context" - "fmt" + "errors" ) // MultiNotifier fans a single blast out to every transport it wraps. It @@ -42,11 +42,7 @@ func (m *MultiNotifier) Blast(ctx context.Context, targets []NotificationTarget, errs = append(errs, err) } } - if len(errs) == 1 { - return errs[0] - } - if len(errs) > 1 { - return fmt.Errorf("multi-notifier: %d transports failed: %v", len(errs), errs) - } - return nil + // errors.Join preserves the tree so callers can errors.As into the + // individual transport errors (e.g. to extract expired web tokens). + return errors.Join(errs...) } diff --git a/pkg/notifications/webpush.go b/pkg/notifications/webpush.go index ba5adbaa5..8c8dbf9d6 100644 --- a/pkg/notifications/webpush.go +++ b/pkg/notifications/webpush.go @@ -3,6 +3,7 @@ package notifications import ( "context" "encoding/json" + "errors" "fmt" "sync" @@ -43,6 +44,11 @@ func NewWebPushNotifier(keys VAPIDKeys, subscriber string) *WebPushNotifier { // Blast implements Notifier. It fans a push out to every web target in // parallel (each subscription is an independent HTTP POST to the browser // push service). Firebase targets are ignored. +// +// Expired subscriptions (410 Gone / 404) are collected and returned as a +// *BlastError whose Expired field holds the raw subscription tokens that +// should be pruned from the DB. Callers can extract them with +// ExpiredTokens(err). func (w *WebPushNotifier) Blast(ctx context.Context, targets []NotificationTarget, blast *NotificationBlast) error { webTargets := make([]NotificationTarget, 0, len(targets)) for _, t := range targets { @@ -65,6 +71,7 @@ func (w *WebPushNotifier) Blast(ctx context.Context, targets []NotificationTarge success int failed int errs []error + expired []string ) for _, t := range webTargets { @@ -77,6 +84,10 @@ func (w *WebPushNotifier) Blast(ctx context.Context, targets []NotificationTarge if err != nil { failed++ errs = append(errs, err) + var expiredErr *ExpiredSubscriptionError + if errors.As(err, &expiredErr) { + expired = append(expired, token) + } log.Error(ctx, "web push failed", "err", err) } else { success++ @@ -85,14 +96,77 @@ func (w *WebPushNotifier) Blast(ctx context.Context, targets []NotificationTarge } wg.Wait() - log.Log(ctx, "web push blast complete", "success", success, "failed", failed, "total", len(webTargets)) + log.Log(ctx, "web push blast complete", "success", success, "failed", failed, "total", len(webTargets), "expired", len(expired)) if len(errs) == 0 { return nil } - if len(errs) == 1 { - return errs[0] + return &BlastsError{ + Errs: errs, + Expired: expired, } - return fmt.Errorf("web push blast: %d of %d failed: %v", len(errs), len(webTargets), errs) +} + +// BlastsError wraps the per-target errors from a WebPushNotifier.Blast and +// exposes the subscription tokens that should be pruned because their +// endpoints returned 410 Gone / 404. +type BlastsError struct { + // Errs are all per-target errors, including ExpiredSubscriptionErrors. + Errs []error + // Expired holds the raw subscription tokens (DB primary keys) whose + // endpoints are dead and should be deleted. + Expired []string +} + +func (e *BlastsError) Error() string { + if len(e.Errs) == 1 { + return e.Errs[0].Error() + } + return fmt.Sprintf("web push blast: %d targets failed", len(e.Errs)) +} + +// Unwrap returns the wrapped errors so errors.Is / errors.As can traverse them. +func (e *BlastsError) Unwrap() []error { return e.Errs } + +// ExpiredTokens walks an error tree (handling errors.Join, MultiNotifier, and +// BlastsError wrapping) and returns the raw subscription tokens whose push +// endpoints returned 410 Gone / 404. Callers should delete these rows from +// the notifications table so dead subscriptions don't accumulate. +func ExpiredTokens(err error) []string { + if err == nil { + return nil + } + var expired []string + // If this node is a BlastsError, take its Expired slice directly and + // stop — recursing into its Unwrap() would only re-encounter the same + // ExpiredSubscriptionErrors and double-count. + var be *BlastsError + if errors.As(err, &be) { + return be.Expired + } + // Otherwise traverse children (errors.Join from MultiNotifier, or a + // single Unwrap chain) looking for nested BlastsErrors. + for _, inner := range errorsUnwrap(err) { + expired = append(expired, ExpiredTokens(inner)...) + } + return expired +} + +// errorsUnwrap returns the direct children of err for tree-walking. Supports +// errors.Join (Unwrap() []error) and single-wrap (Unwrap() error). +func errorsUnwrap(err error) []error { + // errors.Join / BlastsError expose Unwrap() []error + type multiUnwrapper interface{ Unwrap() []error } + if mu, ok := err.(multiUnwrapper); ok { + return mu.Unwrap() + } + // standard single-wrap + type unwrapper interface{ Unwrap() error } + if u, ok := err.(unwrapper); ok { + if inner := u.Unwrap(); inner != nil { + return []error{inner} + } + } + return nil } // sendOne decrypts the stored subscription JSON and POSTs the encrypted diff --git a/pkg/notifications/webpush_test.go b/pkg/notifications/webpush_test.go index 7a57b3117..bd63554a3 100644 --- a/pkg/notifications/webpush_test.go +++ b/pkg/notifications/webpush_test.go @@ -110,6 +110,58 @@ func TestMultiNotifierFanout(t *testing.T) { require.Equal(t, []string{"web-1"}, web.seen, "web notifier should only see web targets") } +// TestExpiredTokensExtraction confirms that ExpiredTokens walks the +// MultiNotifier → BlastsError tree and returns the dead subscription tokens +// so callers can prune them from the DB. +func TestExpiredTokensExtraction(t *testing.T) { + priv, pub, err := webpush.GenerateVAPIDKeys() + require.NoError(t, err) + notifier := NewWebPushNotifier(VAPIDKeys{PublicKey: pub, PrivateKey: priv}, "mailto:test@example.com") + + // Two web targets: one live, one that returns 410 Gone. + var liveSub, deadSub webpush.Subscription + _, livePub, err := webpush.GenerateVAPIDKeys() + require.NoError(t, err) + liveSub.Keys.P256dh = livePub + _, deadPub, err := webpush.GenerateVAPIDKeys() + require.NoError(t, err) + deadSub.Keys.P256dh = deadPub + + mux := http.NewServeMux() + liveHits := 0 + deadHits := 0 + mux.HandleFunc("/live", func(w http.ResponseWriter, r *http.Request) { + liveHits++ + w.WriteHeader(201) + }) + mux.HandleFunc("/dead", func(w http.ResponseWriter, r *http.Request) { + deadHits++ + w.WriteHeader(410) + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + liveSub.Endpoint = srv.URL + "/live" + deadSub.Endpoint = srv.URL + "/dead" + liveJSON, _ := json.Marshal(liveSub) + deadJSON, _ := json.Marshal(deadSub) + + targets := []NotificationTarget{ + {Token: string(liveJSON), Type: NotificationTypeWeb}, + {Token: string(deadJSON), Type: NotificationTypeWeb}, + } + // Wrap in a MultiNotifier to test the full error tree the callers see. + multi := NewMultiNotifier(notifier) + err = multi.Blast(context.Background(), targets, &NotificationBlast{Title: "t", Body: "b"}) + require.Error(t, err) + + expired := ExpiredTokens(err) + require.Len(t, expired, 1, "only the dead subscription should be extracted") + require.Equal(t, string(deadJSON), expired[0]) + require.Equal(t, 1, liveHits) + require.Equal(t, 1, deadHits) +} + // recordingNotifier is a test double that records the tokens it was asked to // blast, filtered to a single type. type recordingNotifier struct { diff --git a/pkg/statedb/queue_processor.go b/pkg/statedb/queue_processor.go index cc765433f..1756f1d49 100644 --- a/pkg/statedb/queue_processor.go +++ b/pkg/statedb/queue_processor.go @@ -497,6 +497,15 @@ func (state *StatefulDB) processNotificationTask(ctx context.Context, task *AppT } else { log.Log(ctx, "sent notifications", "user", userDID, "count", len(notifications), "content", nb) } + // Prune web push subscriptions whose endpoints returned 410 Gone / + // 404 — they're dead and would just fail again on every future blast. + for _, token := range notificationpkg.ExpiredTokens(err) { + if delErr := state.DeleteNotification(token); delErr != nil { + log.Error(ctx, "failed to prune expired notification", "token", token, "err", delErr) + } else { + log.Log(ctx, "pruned expired notification", "token", token) + } + } } else { log.Log(ctx, "no notifier configured, skipping notifications", "user", userDID, "count", len(notifications)) } -- 2.51.2