fix(api): send verification email upon public status page email subscription (#2571) - #2588
Rodrigoue9 wants to merge 9 commits into
Conversation
|
@Rodrigoue9 is attempting to deploy a commit to the OpenStatus Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/api/src/router/statusPage.ts">
<violation number="1" location="packages/api/src/router/statusPage.ts:1264">
P1: A visitor now receives two subscription confirmation emails. This PR sends the verification email directly in `statusPage.subscribe`, but the status-page frontend (`header.tsx`) still calls `emailRouter.sendPageSubscriptionVerification` in the subscribe mutation's `onSuccess`, and that procedure also resends the same verification email for the same subscriber/token. Since `sendPageSubscription` doesn't set a Resend idempotency key, both emails are delivered. Inline the server-side send here and drop the frontend follow-up call (or the reverse), so only one email is sent.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| const verifyUrl = `${baseUrl}/verify/${subscription.token}`; | ||
|
|
||
| try { | ||
| await sendEmailVerification( |
There was a problem hiding this comment.
P1: A visitor now receives two subscription confirmation emails. This PR sends the verification email directly in statusPage.subscribe, but the status-page frontend (header.tsx) still calls emailRouter.sendPageSubscriptionVerification in the subscribe mutation's onSuccess, and that procedure also resends the same verification email for the same subscriber/token. Since sendPageSubscription doesn't set a Resend idempotency key, both emails are delivered. Inline the server-side send here and drop the frontend follow-up call (or the reverse), so only one email is sent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/api/src/router/statusPage.ts, line 1264:
<comment>A visitor now receives two subscription confirmation emails. This PR sends the verification email directly in `statusPage.subscribe`, but the status-page frontend (`header.tsx`) still calls `emailRouter.sendPageSubscriptionVerification` in the subscribe mutation's `onSuccess`, and that procedure also resends the same verification email for the same subscriber/token. Since `sendPageSubscription` doesn't set a Resend idempotency key, both emails are delivered. Inline the server-side send here and drop the frontend follow-up call (or the reverse), so only one email is sent.</comment>
<file context>
@@ -1252,6 +1253,34 @@ export const statusPageRouter = createTRPCRouter({
+ const verifyUrl = `${baseUrl}/verify/${subscription.token}`;
+
+ try {
+ await sendEmailVerification(
+ {
+ id: subscription.id,
</file context>
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/api/src/router/public/subscribe.ts">
<violation number="1" location="packages/api/src/router/public/subscribe.ts:16">
P2: `shouldDispatchVerificationEmail` is dead code: nothing outside its own test file references it. The `subscribe` route in statusPage.ts already guards duplicate sends via `hasPendingSubscriber` and calls `sendEmailVerification` directly, so this export never affects the feature. Remove the unused function (and its test) or wire it into the dispatch path before merging.</violation>
<violation number="2" location="packages/api/src/router/public/subscribe.ts:23">
P3: At exactly 60 seconds after the previous send, this predicate still blocks dispatch even though the documented cooldown has elapsed. Use `>=` so retries are allowed at the cooldown boundary.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
894fc5e to
58c17d6
Compare
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
@claude review |
|
Claude finished @thibaultleouay's task in 3m 47s —— View job ReviewTodo list
SummaryThe core fix works: One issue from cubic's last review pass appears to still be open on current 🔴 Pending-subscriber row isn't cleaned up if the verification email send fails
await sendEmailVerification({ ... }, verifyUrl);This Nuance worth noting: I traced Suggest restoring explicit handling around the send (log-and-continue like before, or proactively expire/delete the pending row on failure) so a config or send error can't strand a visitor. Would also be worth a regression test for "subscribe when 🟡 Minor:
|
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Summary
Resolves #2571 by invoking
sendEmailVerificationwhen a visitor subscribes to a public status page via email.Problem
Previously, when a visitor submitted an email subscription on a public status page,
statusPage.subscribeupserted a pending subscriber record and returned a token, but never dispatched the verification email. The subscriber was left in a pending state without receiving the/verify/<token>link.Solution
sendEmailVerificationfrom@openstatus/subscriptionsinstatusPage.subscribe.${baseUrl}/verify/${token}using the page custom domain,NEXT_PUBLIC_APP_URL, or slug fallback.Closes #2571