From 6e3b5ed44b70f6d9f910ed8e0e8b8c17628823c7 Mon Sep 17 00:00:00 2001 From: Hampton Lintorn-Catlin Date: Mon, 11 May 2026 17:08:43 -0400 Subject: [PATCH] Add Web Push delivery service + per-device fan-out (CIRCLE-39) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Foundation in #107 stops at "subscriptions are stored, SW is wired up, Settings card works." This commit lights it up — the server actually sends a push to every device a user has enabled, anchored to the Notification model so reply / new_comment / mention / agent_response / status_change all flow through the same path. Engine - CoPlan::WebPush::Deliver — wraps web-push gem; signs with VAPID; returns :delivered or :expired (404/410); re-raises transient errors so SolidQueue can retry. - CoPlan::WebPush::PayloadForNotification — builds {title,body,url,tag} per Notification; reason-aware phrasing; mention chips rewritten to @username; markdown emphasis stripped; body truncated to 140 chars with an ellipsis; hyphens and # preserved (regression test included) so co-worker / URL#fragment survive intact; URL is engine-relative so the SW resolves it against self.location.origin. - CoPlan::WebPushDeliveryJob — per-(notification, subscription) so a single bad endpoint doesn't block the user's other devices; destroys the subscription on :expired; retry_on PushServiceError / TooManyRequests up to 5 times with polynomial backoff; defensive against either record being deleted before the job runs. - Notification#after_commit on :create fans out one job per active subscription belonging to the recipient. Quietly no-ops when web_push_configured? is false (host hasn't set VAPID). Specs (+20 examples) - Deliver: 2xx records delivery, 410/404 returns :expired, 503 re-raises, ConfigurationError when VAPID is unset. - PayloadForNotification: per-reason titles, mention/markdown stripping, hyphen/URL preservation, truncation with ellipsis, anonymous fallback. - WebPushDeliveryJob: calls Deliver with the right args, destroys on :expired, no-ops on missing records. - Notification: fan-out enqueues one job per subscription on create, zero when no subscriptions, zero on update, zero when web push isn't configured. Full suite: 834 examples, 0 failures. Generated with Amp Amp-Thread-ID: https://ampcode.com/threads/T-019df459-b110-726a-97e2-ff15e2903435 Co-authored-by: Amp --- .../app/jobs/coplan/web_push_delivery_job.rb | 30 ++++++ engine/app/models/coplan/notification.rb | 18 ++++ .../app/services/coplan/web_push/deliver.rb | 59 +++++++++++ .../web_push/payload_for_notification.rb | 77 +++++++++++++++ .../jobs/coplan/web_push_delivery_job_spec.rb | 79 +++++++++++++++ spec/models/coplan/notification_spec.rb | 74 ++++++++++++++ spec/services/coplan/web_push/deliver_spec.rb | 80 +++++++++++++++ .../web_push/payload_for_notification_spec.rb | 99 +++++++++++++++++++ 8 files changed, 516 insertions(+) create mode 100644 engine/app/jobs/coplan/web_push_delivery_job.rb create mode 100644 engine/app/services/coplan/web_push/deliver.rb create mode 100644 engine/app/services/coplan/web_push/payload_for_notification.rb create mode 100644 spec/jobs/coplan/web_push_delivery_job_spec.rb create mode 100644 spec/models/coplan/notification_spec.rb create mode 100644 spec/services/coplan/web_push/deliver_spec.rb create mode 100644 spec/services/coplan/web_push/payload_for_notification_spec.rb diff --git a/engine/app/jobs/coplan/web_push_delivery_job.rb b/engine/app/jobs/coplan/web_push_delivery_job.rb new file mode 100644 index 0000000..3c8dc73 --- /dev/null +++ b/engine/app/jobs/coplan/web_push_delivery_job.rb @@ -0,0 +1,30 @@ +require "web-push" + +module CoPlan + # Delivers one Notification's Web Push payload to one subscription. + # + # Per-subscription rather than per-notification so that a single bad + # endpoint (rate limited, briefly down, etc.) doesn't block delivery to + # the user's other devices, and so retries / backoff are scoped tightly. + class WebPushDeliveryJob < ApplicationJob + queue_as :default + + # SolidQueue retries everything else with backoff. We don't retry the + # known terminal cases below: ExpiredSubscription / InvalidSubscription + # are handled inside Deliver and surface as :expired. + retry_on ::WebPush::PushServiceError, wait: :polynomially_longer, attempts: 5 + retry_on ::WebPush::TooManyRequests, wait: :polynomially_longer, attempts: 5 + + def perform(notification_id:, subscription_id:) + notification = Notification.find_by(id: notification_id) + return unless notification + + subscription = WebPushSubscription.find_by(id: subscription_id) + return unless subscription + + payload = WebPush::PayloadForNotification.call(notification) + result = WebPush::Deliver.call(subscription: subscription, payload: payload) + subscription.destroy if result == :expired + end + end +end diff --git a/engine/app/models/coplan/notification.rb b/engine/app/models/coplan/notification.rb index cd50c46..2d36169 100644 --- a/engine/app/models/coplan/notification.rb +++ b/engine/app/models/coplan/notification.rb @@ -13,6 +13,8 @@ class Notification < ApplicationRecord scope :read, -> { where.not(read_at: nil) } scope :newest_first, -> { order(created_at: :desc) } + after_commit :enqueue_web_push_deliveries, on: :create + def read? read_at.present? end @@ -28,5 +30,21 @@ def self.ransackable_attributes(auth_object = nil) def self.ransackable_associations(auth_object = nil) %w[user plan comment_thread comment] end + + private + + # Fan-out one WebPushDeliveryJob per active subscription belonging to the + # recipient. Quietly skips when Web Push isn't configured (host hasn't + # set VAPID keys) or the recipient hasn't subscribed any browsers yet. + def enqueue_web_push_deliveries + return unless CoPlan.configuration.web_push_configured? + + user.web_push_subscriptions.find_each do |subscription| + WebPushDeliveryJob.perform_later( + notification_id: id, + subscription_id: subscription.id + ) + end + end end end diff --git a/engine/app/services/coplan/web_push/deliver.rb b/engine/app/services/coplan/web_push/deliver.rb new file mode 100644 index 0000000..68885d7 --- /dev/null +++ b/engine/app/services/coplan/web_push/deliver.rb @@ -0,0 +1,59 @@ +require "web-push" + +module CoPlan + module WebPush + # Sends a single push payload to a single browser subscription using the + # configured VAPID key pair. Returns one of: + # + # :delivered - push service accepted the message (2xx) + # :expired - subscription is gone (404 / 410); caller should destroy it + # + # Anything else (transient 5xx, rate limiting, network errors) raises so + # SolidQueue can retry with backoff. + class Deliver + def self.call(subscription:, payload:) + new(subscription: subscription, payload: payload).call + end + + def initialize(subscription:, payload:) + @subscription = subscription + @payload = payload + end + + def call + unless CoPlan.configuration.web_push_configured? + raise ConfigurationError, "Web Push VAPID keys are not configured" + end + + ::WebPush.payload_send( + endpoint: @subscription.endpoint, + p256dh: @subscription.p256dh_key, + auth: @subscription.auth_key, + message: @payload.to_json, + vapid: vapid_options, + ttl: 24 * 60 * 60, # 24h — push service drops the message after this + urgency: "normal" + ) + + @subscription.record_delivery! + :delivered + rescue ::WebPush::InvalidSubscription, ::WebPush::ExpiredSubscription + # Browser unsubscribed or endpoint was rotated. Tell the caller to + # destroy the row so we don't keep trying. + :expired + end + + class ConfigurationError < StandardError; end + + private + + def vapid_options + { + subject: CoPlan.configuration.vapid_subject, + public_key: CoPlan.configuration.vapid_public_key, + private_key: CoPlan.configuration.vapid_private_key + } + end + end + end +end diff --git a/engine/app/services/coplan/web_push/payload_for_notification.rb b/engine/app/services/coplan/web_push/payload_for_notification.rb new file mode 100644 index 0000000..9685197 --- /dev/null +++ b/engine/app/services/coplan/web_push/payload_for_notification.rb @@ -0,0 +1,77 @@ +module CoPlan + module WebPush + # Builds the { title, body, url, tag } hash the service worker shows for a + # given Notification. Title/body shape per reason; URL deep-links to the + # specific thread; tag groups updates for the same thread so successive + # replies replace each other rather than piling up. + class PayloadForNotification + BODY_TRUNCATE = 140 + + def self.call(notification) + new(notification).call + end + + def initialize(notification) + @notification = notification + @plan = notification.plan + @thread = notification.comment_thread + @comment = notification.comment || @thread.comments.order(:created_at).first + end + + def call + { + title: title, + body: body, + url: url, + tag: "comment-thread-#{@thread.id}" + } + end + + private + + def title + case @notification.reason + when "mention" + "#{actor_name} mentioned you on #{@plan.title}" + when "reply" + "#{actor_name} replied on #{@plan.title}" + when "new_comment" + "#{actor_name} commented on #{@plan.title}" + when "agent_response" + "Agent updated a thread on #{@plan.title}" + when "status_change" + "Thread updated on #{@plan.title}" + else + "Update on #{@plan.title}" + end + end + + def body + return "" unless @comment&.body_markdown + + # Strip mention chips and the markdown emphasis/quote/code characters + # that don't render usefully as plain text in an OS notification. + # Leave hyphens and `#` alone so co-worker / URL#fragment / -prefix + # text stays intact. + text = @comment.body_markdown + .gsub(/\[@(\w+)\]\(mention:[^)]+\)/, '@\1') + .gsub(/[*_`>]/, " ") + .gsub(/\s+/, " ") + .strip + text.truncate(BODY_TRUNCATE, omission: "…") + end + + def url + # Relative path is fine — the SW resolves against self.location.origin + # when opening / focusing the notification target tab. + CoPlan::Engine.routes.url_helpers.plan_path(@plan, thread: @thread.id) + end + + def actor_name + author = @comment&.author + return "Someone" unless author.respond_to?(:name) && author.name.present? + author.name + end + end + end +end diff --git a/spec/jobs/coplan/web_push_delivery_job_spec.rb b/spec/jobs/coplan/web_push_delivery_job_spec.rb new file mode 100644 index 0000000..190cfce --- /dev/null +++ b/spec/jobs/coplan/web_push_delivery_job_spec.rb @@ -0,0 +1,79 @@ +require "rails_helper" + +RSpec.describe CoPlan::WebPushDeliveryJob, type: :job do + let(:user) { create(:coplan_user) } + let(:subscription) { create(:coplan_web_push_subscription, user: user) } + let(:plan) { create(:plan, created_by_user: user) } + let(:thread) do + create(:comment_thread, + plan: plan, + plan_version: plan.current_plan_version, + created_by_user: user) + end + let!(:comment) do + thread.comments.create!( + author_type: "human", + author_id: user.id, + body_markdown: "Hi" + ) + end + let(:notification) do + create(:notification, + user: user, plan: plan, comment_thread: thread, comment: comment, reason: "reply") + end + + before do + CoPlan.configuration.vapid_public_key = "pub" + CoPlan.configuration.vapid_private_key = "priv" + CoPlan.configuration.vapid_subject = "mailto:test@example.com" + end + + after do + CoPlan.configuration.vapid_public_key = nil + CoPlan.configuration.vapid_private_key = nil + CoPlan.configuration.vapid_subject = nil + end + + describe "#perform" do + it "calls Deliver with the notification's payload and the subscription" do + allow(CoPlan::WebPush::Deliver).to receive(:call).and_return(:delivered) + + described_class.perform_now(notification_id: notification.id, subscription_id: subscription.id) + + expect(CoPlan::WebPush::Deliver).to have_received(:call).with( + subscription: subscription, + payload: hash_including(:title, :body, :url, :tag) + ) + end + + it "destroys the subscription when delivery reports :expired" do + # Eagerly create both rows so the change matcher only counts what the + # job itself does, not the let-driven setup. + subscription_id = subscription.id + notification_id = notification.id + allow(CoPlan::WebPush::Deliver).to receive(:call).and_return(:expired) + + expect { + described_class.perform_now(notification_id: notification_id, subscription_id: subscription_id) + }.to change(CoPlan::WebPushSubscription, :count).by(-1) + end + + it "no-ops if the notification was already deleted" do + notification_id = notification.id + notification.destroy! + + expect { + described_class.perform_now(notification_id: notification_id, subscription_id: subscription.id) + }.not_to raise_error + end + + it "no-ops if the subscription was already deleted" do + subscription_id = subscription.id + subscription.destroy! + + expect { + described_class.perform_now(notification_id: notification.id, subscription_id: subscription_id) + }.not_to raise_error + end + end +end diff --git a/spec/models/coplan/notification_spec.rb b/spec/models/coplan/notification_spec.rb new file mode 100644 index 0000000..e8cf059 --- /dev/null +++ b/spec/models/coplan/notification_spec.rb @@ -0,0 +1,74 @@ +require "rails_helper" + +RSpec.describe CoPlan::Notification do + let(:user) { create(:coplan_user) } + let(:plan) { create(:plan, created_by_user: user) } + let(:thread) do + create(:comment_thread, + plan: plan, + plan_version: plan.current_plan_version, + created_by_user: user) + end + let(:attrs) do + { user: user, plan: plan, comment_thread: thread, reason: "reply" } + end + + describe "after_create web push fan-out" do + context "when web push is configured" do + before do + CoPlan.configuration.vapid_public_key = "pub" + CoPlan.configuration.vapid_private_key = "priv" + CoPlan.configuration.vapid_subject = "mailto:test@example.com" + end + + after do + CoPlan.configuration.vapid_public_key = nil + CoPlan.configuration.vapid_private_key = nil + CoPlan.configuration.vapid_subject = nil + end + + it "enqueues one delivery job per subscription on create" do + sub_a = create(:coplan_web_push_subscription, user: user) + sub_b = create(:coplan_web_push_subscription, user: user) + + expect { + described_class.create!(attrs) + }.to have_enqueued_job(CoPlan::WebPushDeliveryJob).twice + + notification = described_class.last + [sub_a, sub_b].each do |sub| + expect(CoPlan::WebPushDeliveryJob).to have_been_enqueued.with( + notification_id: notification.id, + subscription_id: sub.id + ) + end + end + + it "enqueues nothing when the recipient has no subscriptions" do + expect { + described_class.create!(attrs) + }.not_to have_enqueued_job(CoPlan::WebPushDeliveryJob) + end + + it "does not enqueue on update" do + create(:coplan_web_push_subscription, user: user) + notification = described_class.create!(attrs) + + expect { + notification.mark_read! + }.not_to have_enqueued_job(CoPlan::WebPushDeliveryJob) + end + end + + context "when web push is not configured" do + it "does not enqueue any jobs even if subscriptions exist" do + # Sanity: in test env VAPID keys are nil by default. + create(:coplan_web_push_subscription, user: user) + + expect { + described_class.create!(attrs) + }.not_to have_enqueued_job(CoPlan::WebPushDeliveryJob) + end + end + end +end diff --git a/spec/services/coplan/web_push/deliver_spec.rb b/spec/services/coplan/web_push/deliver_spec.rb new file mode 100644 index 0000000..e66f368 --- /dev/null +++ b/spec/services/coplan/web_push/deliver_spec.rb @@ -0,0 +1,80 @@ +require "rails_helper" + +RSpec.describe CoPlan::WebPush::Deliver do + let(:subscription) { create(:coplan_web_push_subscription) } + let(:payload) { { title: "Hi", body: "Hello", url: "/plans/x", tag: "comment-thread-1" } } + + before do + CoPlan.configuration.vapid_public_key = "public" + CoPlan.configuration.vapid_private_key = "private" + CoPlan.configuration.vapid_subject = "mailto:test@example.com" + end + + after do + CoPlan.configuration.vapid_public_key = nil + CoPlan.configuration.vapid_private_key = nil + CoPlan.configuration.vapid_subject = nil + end + + describe ".call" do + it "POSTs the payload via the web-push gem and records delivery on success" do + allow(::WebPush).to receive(:payload_send) + + result = described_class.call(subscription: subscription, payload: payload) + + expect(::WebPush).to have_received(:payload_send).with( + endpoint: subscription.endpoint, + p256dh: subscription.p256dh_key, + auth: subscription.auth_key, + message: payload.to_json, + vapid: { + subject: "mailto:test@example.com", + public_key: "public", + private_key: "private" + }, + ttl: 24 * 60 * 60, + urgency: "normal" + ) + expect(result).to eq(:delivered) + expect(subscription.reload.notifications_delivered_count).to eq(1) + expect(subscription.last_delivered_at).to be_present + end + + it "returns :expired when the push service reports the subscription is gone (410)" do + response = instance_double(Net::HTTPResponse, code: "410", message: "Gone", body: "") + allow(::WebPush).to receive(:payload_send) + .and_raise(::WebPush::ExpiredSubscription.new(response, "fcm.googleapis.com")) + + result = described_class.call(subscription: subscription, payload: payload) + + expect(result).to eq(:expired) + expect(subscription.reload.notifications_delivered_count).to eq(0) + end + + it "returns :expired for invalid subscription (404)" do + response = instance_double(Net::HTTPResponse, code: "404", message: "Not Found", body: "") + allow(::WebPush).to receive(:payload_send) + .and_raise(::WebPush::InvalidSubscription.new(response, "fcm.googleapis.com")) + + expect(described_class.call(subscription: subscription, payload: payload)).to eq(:expired) + end + + it "re-raises transient errors so the caller can retry" do + response = instance_double(Net::HTTPResponse, code: "503", message: "Service Unavailable", body: "") + allow(::WebPush).to receive(:payload_send) + .and_raise(::WebPush::PushServiceError.new(response, "fcm.googleapis.com")) + + expect { + described_class.call(subscription: subscription, payload: payload) + }.to raise_error(::WebPush::PushServiceError) + end + + it "raises ConfigurationError when VAPID is not configured" do + CoPlan.configuration.vapid_private_key = nil + + expect { + described_class.call(subscription: subscription, payload: payload) + }.to raise_error(described_class::ConfigurationError) + end + end +end diff --git a/spec/services/coplan/web_push/payload_for_notification_spec.rb b/spec/services/coplan/web_push/payload_for_notification_spec.rb new file mode 100644 index 0000000..17bd63e --- /dev/null +++ b/spec/services/coplan/web_push/payload_for_notification_spec.rb @@ -0,0 +1,99 @@ +require "rails_helper" + +RSpec.describe CoPlan::WebPush::PayloadForNotification do + let(:author) { create(:coplan_user, name: "Alice") } + let(:recipient) { create(:coplan_user, name: "Bob") } + let(:plan) { create(:plan, title: "My Plan", created_by_user: recipient) } + let(:thread) do + create(:comment_thread, + plan: plan, + plan_version: plan.current_plan_version, + created_by_user: author) + end + let(:comment) do + thread.comments.create!( + author_type: "human", + author_id: author.id, + body_markdown: "Looks good — but **what about** [@bob](mention:bob) opinion on this?" + ) + end + + def notification_for(reason, comment_record: comment) + create(:notification, + user: recipient, + plan: plan, + comment_thread: thread, + comment: comment_record, + reason: reason) + end + + describe ".call" do + it "returns title/body/url/tag for a reply" do + payload = described_class.call(notification_for("reply")) + + expect(payload).to include( + title: "Alice replied on My Plan", + tag: "comment-thread-#{thread.id}" + ) + expect(payload[:url]).to include("/plans/#{plan.id}").and include("thread=#{thread.id}") + end + + it "uses 'mentioned you' phrasing for mention notifications" do + payload = described_class.call(notification_for("mention")) + + expect(payload[:title]).to eq("Alice mentioned you on My Plan") + end + + it "uses 'commented on' phrasing for new_comment notifications" do + payload = described_class.call(notification_for("new_comment")) + + expect(payload[:title]).to eq("Alice commented on My Plan") + end + + it "strips mention chips and markdown formatting from the body" do + payload = described_class.call(notification_for("reply")) + + # Markdown emphasis stripped, mention chip rewritten to @username, + # whitespace collapsed. Em dash and ordinary punctuation preserved. + expect(payload[:body]).to eq("Looks good — but what about @bob opinion on this?") + end + + it "preserves hyphens and # so words like co-worker and URLs survive" do + hyphen_comment = thread.comments.create!( + author_type: "human", + author_id: author.id, + body_markdown: "My co-worker linked https://example.com/foo-bar#baz" + ) + + payload = described_class.call(notification_for("reply", comment_record: hyphen_comment)) + + expect(payload[:body]).to eq("My co-worker linked https://example.com/foo-bar#baz") + end + + it "truncates long bodies with an ellipsis" do + long = "x" * 300 + long_comment = thread.comments.create!( + author_type: "human", + author_id: author.id, + body_markdown: long + ) + + payload = described_class.call(notification_for("reply", comment_record: long_comment)) + + expect(payload[:body].length).to eq(described_class::BODY_TRUNCATE) + expect(payload[:body]).to end_with("…") + end + + it "falls back to 'Someone' when the comment author has no name" do + anon_comment = thread.comments.create!( + author_type: "cloud_persona", + author_id: nil, + body_markdown: "agent reply" + ) + + payload = described_class.call(notification_for("agent_response", comment_record: anon_comment)) + + expect(payload[:title]).to eq("Agent updated a thread on My Plan") + end + end +end