Web Push delivery service + per-device fan-out (CIRCLE-39)#109
Conversation
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 <amp@ampcode.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e3b5ed44b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| subscription = WebPushSubscription.find_by(id: subscription_id) | ||
| return unless subscription | ||
|
|
||
| payload = WebPush::PayloadForNotification.call(notification) | ||
| result = WebPush::Deliver.call(subscription: subscription, payload: payload) |
There was a problem hiding this comment.
WebPushDeliveryJob#perform fetches a subscription by ID and sends immediately, but never checks that it is still owned by the notification’s user. This can leak notifications across accounts because WebPushSubscription.upsert_for reassigns an existing endpoint_digest row to whatever user most recently subscribes on that browser/device; if that reassignment happens before the queued job runs, the payload is delivered to the wrong user’s device. Guard on subscription.user_id == notification.user_id (or scope the lookup by both IDs) before calling deliver.
Useful? React with 👍 / 👎.
| # Leave hyphens and `#` alone so co-worker / URL#fragment / -prefix | ||
| # text stays intact. | ||
| text = @comment.body_markdown | ||
| .gsub(/\[@(\w+)\]\(mention:[^)]+\)/, '@\1') |
There was a problem hiding this comment.
The mention-chip cleanup regex only matches \w+, so push bodies fail to normalize valid usernames containing . or - (for example [@jane.doe](mention:jane.doe)), leaving raw markdown syntax in notifications. This is inconsistent with the app’s username and mention parsing rules, which allow . and -; use the same pattern ([\w.-]+ with a backreference) to preserve readable @username text.
Useful? React with 👍 / 👎.
1 participant
Stacked on top of #108. Foundation in #107 shipped subscriptions, the service worker, and the Settings UI. This PR is the part that actually sends notifications: when a
Notificationis created (reply, mention, new comment, agent reply, status change), we fan out one job per active browser subscription belonging to the recipient and POST a VAPID-signed payload to each.Linear: CIRCLE-39
What's here
CoPlan::WebPush::Deliver— wraps theweb-pushgem, signs with the configured VAPID keypair, returns::delivered(2xx) → bumpsnotifications_delivered_countandlast_delivered_at:expired(404 / 410) → caller destroys the rowPushServiceError/TooManyRequestsfor transient failures so SolidQueue can retryCoPlan::WebPush::PayloadForNotification—{title, body, url, tag}perNotification:[@bob](mention:bob)→@bob) and markdown emphasis, then truncates to 140 chars with…. Hyphens and#are intentionally preserved (regression test) soco-workerandhttps://example.com/foo-bar#bazsurvive.self.location.originand the existingnotificationclickhandler focuses an existing tab on that origin.tag: "comment-thread-#{thread.id}"so a follow-up reply replaces the previous notification rather than stacking.CoPlan::WebPushDeliveryJob— per-(notification, subscription):retry_on PushServiceError, TooManyRequests— polynomial backoff, 5 attempts.:expired.find_byreturns silently if either record was deleted between enqueue and execution.Notification#after_commit on: :create— fans out one job per row inuser.web_push_subscriptions. Quietly no-ops whenCoPlan.configuration.web_push_configured?is false, so deployments without VAPID keys (including the test env) stay silent.Verification
bundle exec rspec→ 834 examples, 0 failures (was 814 before — +20 new specs across delivery, payload, job, and the model hook).code_reviewtool) addressed before commit: tightened the body-strip regex (was corrupting hyphens), switched toString#truncate, removed a misleading test comment.Out of scope (deliberately)
:mentionnotification event for the mention path — already wired here viaProcessMentionscreatingNotificationrows withreason: "mention", but CIRCLE-42 covers any cross-system event work that may still be needed.Generated with Amp