From fadf6f1859ecf8b3e1dc4d74a62c4c68995157bb Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 19 Oct 2026 13:15:10 -0700 Subject: [PATCH] Post the browser's push subscription from every page, and register it once Only room pages posted the subscription again, so a browser that changed its password and closed the profile page went without notifications until it next opened a room. A small controller in the layout does it now, still once per session, and the bell is back to how it was. Two tabs posting at once after a password change could both miss the row and insert two, so the endpoint is looked up again under the same write lock as the session check. From #371. (cherry picked from commit 3786ee52444935fbf6a4da9f1b45866b34884b31) --- .../users/push_subscriptions_controller.rb | 18 +++++++---- .../controllers/notifications_controller.js | 21 +----------- .../push_subscription_sync_controller.js | 32 +++++++++++++++++++ app/views/layouts/application.html.erb | 5 +++ app/views/rooms/involvements/_bell.html.erb | 3 +- .../push_subscriptions_controller_test.rb | 22 +++++++++++-- test/system/push_subscription_resync_test.rb | 7 ++-- 7 files changed, 73 insertions(+), 35 deletions(-) create mode 100644 app/javascript/controllers/push_subscription_sync_controller.js diff --git a/app/controllers/users/push_subscriptions_controller.rb b/app/controllers/users/push_subscriptions_controller.rb index e4e46524..a3517a83 100644 --- a/app/controllers/users/push_subscriptions_controller.rb +++ b/app/controllers/users/push_subscriptions_controller.rb @@ -10,7 +10,7 @@ class Users::PushSubscriptionsController < ApplicationController # Existing endpoints must pass current validations too if subscription.valid? - head save_while_signed_in(subscription) ? :ok : :unauthorized + head register_while_signed_in(subscription) ? :ok : :unauthorized else head :unprocessable_entity end @@ -27,12 +27,18 @@ class Users::PushSubscriptionsController < ApplicationController end # A password change ends every session and subscription. A request that authenticated just before it mustn't - # put one back, so the session is checked under the same write lock as the save. Validation, which looks up - # the endpoint's address, runs before taking the lock. - def save_while_signed_in(subscription) + # put one back, so the session is checked under the same write lock as the save, and the endpoint is looked up + # again there so two tabs posting at once register it once. Validation, which resolves the endpoint's address, + # runs before taking the lock. + def register_while_signed_in(subscription) Push::Subscription.transaction do - Session.exists?(Current.session.id) && - (subscription.new_record? ? subscription.save!(validate: false) : subscription.touch) + if Session.exists?(Current.session.id) + if existing = @push_subscriptions.find_by(push_subscription_params) + existing.touch + else + subscription.save!(validate: false) + end + end end end diff --git a/app/javascript/controllers/notifications_controller.js b/app/javascript/controllers/notifications_controller.js index b3698a31..3ce53a5c 100644 --- a/app/javascript/controllers/notifications_controller.js +++ b/app/javascript/controllers/notifications_controller.js @@ -4,10 +4,8 @@ import { pageIsTurboPreview } from "helpers/turbo_helpers" import { onNextEventLoopTick } from "helpers/timing_helpers" import { getCookie, setCookie } from "lib/cookie" -const PUSH_SUBSCRIPTION_SYNCED_KEY = "push-subscription-synced-session" - export default class extends Controller { - static values = { subscriptionsUrl: String, session: String } + static values = { subscriptionsUrl: String } static targets = [ "notAllowedNotice", "bell", "details" ] static classes = [ "attention" ] @@ -27,8 +25,6 @@ export default class extends Controller { this.#showBellAlert() } } - - this.#resyncPushSubscriptionOncePerSession() } } @@ -113,21 +109,6 @@ export default class extends Controller { if (!response.ok) subscription.unsubscribe() } - // A password change removes the account's subscriptions, the one this browser holds included. Post it again - // once per session, so a browser that's still signed in keeps getting notifications. A failure here leaves the - // browser's own subscription alone: the next session tries again. - async #resyncPushSubscriptionOncePerSession() { - if (this.#allowed && this.sessionValue && localStorage.getItem(PUSH_SUBSCRIPTION_SYNCED_KEY) !== this.sessionValue) { - const registration = await this.#serviceWorkerRegistration - const subscription = await registration?.pushManager?.getSubscription() - - if (Notification.permission === "granted" && subscription) { - await post(this.subscriptionsUrlValue, { body: this.#extractJsonPayloadAsString(subscription), responseKind: "turbo-stream" }) - localStorage.setItem(PUSH_SUBSCRIPTION_SYNCED_KEY, this.sessionValue) - } - } - } - async #requestPermissionAndSubscribe(registration) { const permission = await Notification.requestPermission() if (permission === "granted") this.#subscribe(registration) diff --git a/app/javascript/controllers/push_subscription_sync_controller.js b/app/javascript/controllers/push_subscription_sync_controller.js new file mode 100644 index 00000000..d2636523 --- /dev/null +++ b/app/javascript/controllers/push_subscription_sync_controller.js @@ -0,0 +1,32 @@ +import { Controller } from "@hotwired/stimulus" +import { post } from "@rails/request.js" + +const SYNCED_SESSION_KEY = "push-subscription-synced-session" + +// A password change removes the account's push subscriptions, the one this browser holds included. Post it again +// once per session, so a browser that's still signed in keeps getting notifications. A failure here leaves the +// browser's own subscription alone: the next session tries again. +export default class extends Controller { + static values = { url: String, session: String } + + async connect() { + if (this.#allowed && this.sessionValue && localStorage.getItem(SYNCED_SESSION_KEY) !== this.sessionValue) { + const registration = await navigator.serviceWorker.getRegistration(window.location.origin) + const subscription = await registration?.pushManager?.getSubscription() + + if (Notification.permission === "granted" && subscription) { + await post(this.urlValue, { body: this.#payload(subscription), responseKind: "turbo-stream" }) + localStorage.setItem(SYNCED_SESSION_KEY, this.sessionValue) + } + } + } + + get #allowed() { + return navigator.serviceWorker && window.Notification + } + + #payload(subscription) { + const { endpoint, keys: { p256dh, auth } } = subscription.toJSON() + return JSON.stringify({ push_subscription: { endpoint, p256dh_key: p256dh, auth_key: auth } }) + } +} diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index 23a66c5d..a3c90ec4 100644 --- a/app/views/layouts/application.html.erb +++ b/app/views/layouts/application.html.erb @@ -63,6 +63,11 @@ <%= render "layouts/lightbox" %> + <% if Current.session %> + <%= tag.div hidden: true, data: { controller: "push-subscription-sync", + push_subscription_sync_url_value: user_push_subscriptions_path, push_subscription_sync_session_value: push_subscription_session_marker } %> + <% end %> + diff --git a/app/views/rooms/involvements/_bell.html.erb b/app/views/rooms/involvements/_bell.html.erb index 266bdfb0..d9fa32c5 100644 --- a/app/views/rooms/involvements/_bell.html.erb +++ b/app/views/rooms/involvements/_bell.html.erb @@ -1,7 +1,6 @@ + data-controller="notifications" data-notifications-subscriptions-url-value="<%= user_push_subscriptions_path %>" data-notifications-attention-class="btn--pulsing"> <%= turbo_frame_tag dom_id(room, :involvement), data: { controller: "turbo-frame", action: "notifications:ready@window->turbo-frame#load", turbo_frame_url_param: room_involvement_path(room) } do %>