Fix soft bounce suppression flow#1394
Open
buildwithnidhi wants to merge 1 commit into
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
WalkthroughThe PR introduces an internal bounce classification and suppression system for email delivery. It adds database models for bounce tracking ( ChangesEmail Bounce Tracking & Suppression System
Sequence Diagram(s)sequenceDiagram
participant Resend as Resend Webhook
participant Handler as Webhook Handler
participant Classify as Bounce Classifier
participant DB as Prisma Transaction
participant BounceLog as emailBounceLog
participant BlockedEmail as blockedEmail
Resend->>Handler: email event + fingerprint
Handler->>Handler: compute webhookKey from email_id
Handler->>DB: start transaction
alt email.bounced or email.delivery_delayed
Handler->>Classify: parse fingerprint, classify bounce type
Classify-->>Handler: bounce category + diagnostic code
Handler->>DB: query emailBounceLog for email
DB-->>Handler: current bounce record
Handler->>BounceLog: upsert with consecutiveBounces++, lastBounceAt
Handler->>Handler: check suppression rules
alt should suppress
Handler->>BounceLog: set suppressedAt
Handler->>BlockedEmail: upsert reason (soft/hard bounce)
end
else email.delivered
Handler->>BounceLog: reset consecutiveBounces, suppressedAt
Handler->>Handler: check if soft-bounce reason
alt was soft-bounce
Handler->>BlockedEmail: delete record
end
else email.complained
Handler->>BlockedEmail: delete email settings in transaction
end
DB->>DB: commit transaction
alt constraint violation P2002
Handler-->>Resend: 200 (prevent reprocessing)
else success
Handler-->>Resend: 200
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/features/emails/utils/queueEmail.ts (1)
48-48: 💤 Low valueAdd explicit return type annotation.
As per coding guidelines, "Declare return types for top-level module functions in TypeScript".
-async function isBlockedRecipient(userId?: string) { +async function isBlockedRecipient(userId?: string): Promise<boolean> {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/emails/utils/queueEmail.ts` at line 48, The top-level function isBlockedRecipient lacks an explicit return type; update its signature to declare the return type as Promise<boolean> (i.e., async function isBlockedRecipient(userId?: string): Promise<boolean>) so the module follows the TypeScript guideline to annotate top-level function return types; ensure any returned values within isBlockedRecipient conform to boolean and adjust any internal early returns to return Promise-resolved booleans accordingly.src/pages/api/email/webhook/index.ts (1)
58-81: 💤 Low valueConsider log level for email addresses.
Email addresses are logged at
infolevel (lines 63, 76, 79). While this aids debugging bounce issues, consider whetherdebuglevel would be more appropriate for PII, or ensure your log retention/access policies account for this.Also applies to: 283-311
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/api/email/webhook/index.ts` around lines 58 - 81, Log entries that include recipient email are currently at info level in deleteEmailSettings; change those logger.info calls that interpolate recipientEmail to logger.debug (keep non-PII entries like user.id at info if desired) so PII isn’t emitted at info level. Update the logger call inside deleteEmailSettings (and any other functions in this same file that log recipientEmail or other PII, e.g., the webhook handler that logs bounce/recipient details) to use debug or redact the email, and ensure logging behavior conforms to your retention/access policy.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/features/emails/utils/queueEmail.ts`:
- Around line 72-74: The log in queueEmail.ts currently includes a PII email
(normalizedEmail) in the logger.info call; update the logger invocation (the
logger.info that mentions `Skipping queued email for blocked recipient`) to
remove the email and only include `userId` and context like
`blockedEmail.reason` (or redact the email variable if you must keep a
placeholder). Locate the `logger.info` call that references `normalizedEmail`,
drop or redact `normalizedEmail`, and keep `userId` and `blockedEmail.reason` so
logs no longer contain PII.
In `@src/pages/api/email/webhook/index.ts`:
- Around line 372-375: The catch block currently logs the full error and returns
the raw error to the client (logger.error(error); return
res.status(400).send(error);) which can leak internals; keep the detailed
logger.error(error) but replace the response body with a generic message (e.g.,
res.status(400).json({ error: 'Invalid request' }) or similar) and avoid sending
the error/stack to the client; update the error handling in the webhook handler
where logger and res are used to return a non-sensitive, user-facing error
message.
---
Nitpick comments:
In `@src/features/emails/utils/queueEmail.ts`:
- Line 48: The top-level function isBlockedRecipient lacks an explicit return
type; update its signature to declare the return type as Promise<boolean> (i.e.,
async function isBlockedRecipient(userId?: string): Promise<boolean>) so the
module follows the TypeScript guideline to annotate top-level function return
types; ensure any returned values within isBlockedRecipient conform to boolean
and adjust any internal early returns to return Promise-resolved booleans
accordingly.
In `@src/pages/api/email/webhook/index.ts`:
- Around line 58-81: Log entries that include recipient email are currently at
info level in deleteEmailSettings; change those logger.info calls that
interpolate recipientEmail to logger.debug (keep non-PII entries like user.id at
info if desired) so PII isn’t emitted at info level. Update the logger call
inside deleteEmailSettings (and any other functions in this same file that log
recipientEmail or other PII, e.g., the webhook handler that logs
bounce/recipient details) to use debug or redact the email, and ensure logging
behavior conforms to your retention/access policy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e1074e60-68d4-4297-860f-a931204e6af1
📒 Files selected for processing (4)
prisma/schema.prismasrc/features/emails/utils/queueEmail.tssrc/pages/api/email/webhook/index.tssrc/pages/api/sponsor-dashboard/submission/add-payment.ts
Comment on lines
+72
to
+74
| logger.info( | ||
| `Skipping queued email for blocked recipient: userId=${userId}, email=${normalizedEmail}, reason=${blockedEmail.reason ?? 'unspecified'}`, | ||
| ); |
Contributor
There was a problem hiding this comment.
Avoid logging PII (email address).
The log message includes the user's email address, which is personally identifiable information. This creates compliance risk (GDPR/CCPA) and should be avoided in production logs. Use userId alone to identify the blocked recipient.
logger.info(
- `Skipping queued email for blocked recipient: userId=${userId}, email=${normalizedEmail}, reason=${blockedEmail.reason ?? 'unspecified'}`,
+ `Skipping queued email for blocked recipient: userId=${userId}, reason=${blockedEmail.reason ?? 'unspecified'}`,
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| logger.info( | |
| `Skipping queued email for blocked recipient: userId=${userId}, email=${normalizedEmail}, reason=${blockedEmail.reason ?? 'unspecified'}`, | |
| ); | |
| logger.info( | |
| `Skipping queued email for blocked recipient: userId=${userId}, reason=${blockedEmail.reason ?? 'unspecified'}`, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/features/emails/utils/queueEmail.ts` around lines 72 - 74, The log in
queueEmail.ts currently includes a PII email (normalizedEmail) in the
logger.info call; update the logger invocation (the logger.info that mentions
`Skipping queued email for blocked recipient`) to remove the email and only
include `userId` and context like `blockedEmail.reason` (or redact the email
variable if you must keep a placeholder). Locate the `logger.info` call that
references `normalizedEmail`, drop or redact `normalizedEmail`, and keep
`userId` and `blockedEmail.reason` so logs no longer contain PII.
Comment on lines
372
to
375
| } catch (error) { | ||
| logger.error(error); | ||
| return res.status(400).send(error); | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid leaking internal error details to clients.
Sending the raw error object to the client could expose stack traces or internal implementation details. Return a generic error message instead.
Proposed fix
} catch (error) {
logger.error(error);
- return res.status(400).send(error);
+ return res.status(400).json({ error: 'Webhook processing failed' });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } catch (error) { | |
| logger.error(error); | |
| return res.status(400).send(error); | |
| } | |
| } catch (error) { | |
| logger.error(error); | |
| return res.status(400).json({ error: 'Webhook processing failed' }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/api/email/webhook/index.ts` around lines 372 - 375, The catch block
currently logs the full error and returns the raw error to the client
(logger.error(error); return res.status(400).send(error);) which can leak
internals; keep the detailed logger.error(error) but replace the response body
with a generic message (e.g., res.status(400).json({ error: 'Invalid request' })
or similar) and avoid sending the error/stack to the client; update the error
handling in the webhook handler where logger and res are used to return a
non-sensitive, user-facing error message.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this does
This adds proper soft-bounce handling on our side instead of just letting Resend keep retrying and then forgetting about the address.
Right now the issue is basically:
This change fixes that flow in the app.
What changed
EmailBounceLogtable to track bounce state per emailemail.bounced,email.delivery_delayed,email.delivered, andemail.complainedin a more deterministic wayblockedEmailno mx/no such domain/domain does not existare handled more aggressivelyaddPaymentnow passesuserIdso that targeted path is also covered by the queue-time checkBehavior now
Why this was done this way
This keeps the blast radius small.
We did not change the whole email system or wipe user preferences as the main fix.
We kept
blockedEmailas the actual enforcement gate and made the webhook + queue layer smarter so repeated bad recipients stop getting targeted.This also avoids overreacting to ambiguous DNS/provider noise. Only explicit dead-domain signals are treated as dead-domain cases.
Not in this PR
Those can come later after this is live and we confirm webhook traffic + suppression behavior in prod.
After merge / rollout checklist
email.bouncedemail.delivery_delayedemail.deliveredemail.complainedSummary by CodeRabbit
Bug Fixes
New Features