fix: sync suppression list removal with AWS SES (closes #324) #331
Conversation
When removing an email from the suppression list, now also removes it from AWS SES account-level suppression list across all regions where the team has domains configured. - Add deleteFromSesSuppressionList helper to ses.ts - Update removeSuppression to query team domains for unique regions - Use best-effort pattern: AWS failures don't block local DB deletion - Handle NotFoundException gracefully (email not in SES list)
|
@ManojINaik is attempting to deploy a commit to the kmkoushik's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdded Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/src/server/aws/ses.tsapps/web/src/server/service/suppression-service.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{tsx,ts,jsx,js}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Include all required imports and ensure proper naming of key components in React/NextJS code
Files:
apps/web/src/server/aws/ses.tsapps/web/src/server/service/suppression-service.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use TypeScript-first approach with 2-space indent and semicolons enabled by Prettier in apps/web (Next.js), apps/marketing, apps/smtp-server, and all packages
Never use dynamic imports; always import on the top level
Run ESLint via @usesend/eslint-config and ensure no warnings remain before submitting PRs
Files:
apps/web/src/server/aws/ses.tsapps/web/src/server/service/suppression-service.ts
apps/web/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use alias
~/for src imports in apps/web (e.g.,import { x } from "~/utils/x")
Files:
apps/web/src/server/aws/ses.tsapps/web/src/server/service/suppression-service.ts
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/**/*.{ts,tsx}: Prefer to use TRPC for client-server communication unless explicitly asked otherwise in apps/web
Use Prisma for database access in apps/web
Files:
apps/web/src/server/aws/ses.tsapps/web/src/server/service/suppression-service.ts
**/*.{ts,tsx,md}
📄 CodeRabbit inference engine (AGENTS.md)
Run Prettier 3 for code formatting on TypeScript, TSX, and Markdown files
Files:
apps/web/src/server/aws/ses.tsapps/web/src/server/service/suppression-service.ts
🧬 Code graph analysis (2)
apps/web/src/server/aws/ses.ts (1)
apps/web/src/server/logger/log.ts (1)
logger(31-63)
apps/web/src/server/service/suppression-service.ts (3)
apps/web/src/server/db.ts (1)
db(20-20)apps/web/src/server/aws/ses.ts (1)
deleteFromSesSuppressionList(319-346)apps/web/src/server/logger/log.ts (1)
logger(31-63)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (2)
apps/web/src/server/aws/ses.ts (1)
314-346: LGTM! Well-structured best-effort deletion with appropriate error handling.The function correctly handles:
- Successful deletions
NotFoundException(gracefully returnstruesince email wasn't in the list)- Other errors (returns
falseand logs)This aligns well with the best-effort pattern described in the PR objectives.
apps/web/src/server/service/suppression-service.ts (1)
127-127: LGTM! Solid email normalization and resilient local DB deletion.Good implementation choices:
- Email normalization is consistent with
addSuppression(lines 43, 47)- Local DB deletion proceeds regardless of AWS cleanup outcome
- Graceful handling when suppression record doesn't exist
- Error handling preserved for actual DB failures
Also applies to: 170-218
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 2 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/web/src/server/aws/ses.ts">
<violation number="1" location="apps/web/src/server/aws/ses.ts:341">
P2: Logging only `error.message` loses the stack trace and other error details. The existing pattern in this file logs the full error object as `{ err: error }` for better debugging.</violation>
</file>
<file name="apps/web/src/server/service/suppression-service.ts">
<violation number="1" location="apps/web/src/server/service/suppression-service.ts:145">
P2: Failure detection logic is ineffective. Since `deleteFromSesSuppressionList` catches all errors internally and returns `false` on failure (never throws), `Promise.allSettled` will always produce `fulfilled` results. The filter for `rejected` status will always return an empty array, meaning AWS SES failures are silently ignored and never logged.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| return true; | ||
| } | ||
| logger.error( | ||
| { email, region, error: error.message }, |
There was a problem hiding this comment.
P2: Logging only error.message loses the stack trace and other error details. The existing pattern in this file logs the full error object as { err: error } for better debugging.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/server/aws/ses.ts, line 341:
<comment>Logging only `error.message` loses the stack trace and other error details. The existing pattern in this file logs the full error object as `{ err: error }` for better debugging.</comment>
<file context>
@@ -310,3 +311,36 @@ export async function addWebhookConfiguration(
+ return true;
+ }
+ logger.error(
+ { email, region, error: error.message },
+ "Failed to remove email from SES suppression list"
+ );
</file context>
| { email, region, error: error.message }, | |
| { email, region, err: error }, |
| ) | ||
| ); | ||
|
|
||
| const failures = results.filter((r) => r.status === "rejected"); |
There was a problem hiding this comment.
P2: Failure detection logic is ineffective. Since deleteFromSesSuppressionList catches all errors internally and returns false on failure (never throws), Promise.allSettled will always produce fulfilled results. The filter for rejected status will always return an empty array, meaning AWS SES failures are silently ignored and never logged.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/server/service/suppression-service.ts, line 145:
<comment>Failure detection logic is ineffective. Since `deleteFromSesSuppressionList` catches all errors internally and returns `false` on failure (never throws), `Promise.allSettled` will always produce `fulfilled` results. The filter for `rejected` status will always return an empty array, meaning AWS SES failures are silently ignored and never logged.</comment>
<file context>
@@ -120,22 +121,66 @@ export class SuppressionService {
+ )
+ );
+
+ const failures = results.filter((r) => r.status === "rejected");
+ if (failures.length > 0) {
+ logger.warn(
</file context>
| const failures = results.filter((r) => r.status === "rejected"); | |
| const failures = results.filter( | |
| (r) => r.status === "rejected" || (r.status === "fulfilled" && r.value === false) | |
| ); |
✅ Addressed in c63400f
deleteFromSesSuppressionList returns false on error (never throws), so check for fulfilled promises with value === false instead of rejected status.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/web/src/server/service/suppression-service.ts (1)
146-148: Optional: Consider defensive check for rejected promises.While the current implementation correctly handles the expected behavior (where
deleteFromSesSuppressionListreturnsfalseon error), consider adding a check for rejected promises as a defensive measure in case the function implementation changes in the future:const failures = results.filter( - (r) => r.status === "fulfilled" && r.value === false + (r) => r.status === "rejected" || (r.status === "fulfilled" && r.value === false) );This protects against unexpected throws while maintaining the existing logic.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/src/server/service/suppression-service.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{tsx,ts,jsx,js}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Include all required imports and ensure proper naming of key components in React/NextJS code
Files:
apps/web/src/server/service/suppression-service.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use TypeScript-first approach with 2-space indent and semicolons enabled by Prettier in apps/web (Next.js), apps/marketing, apps/smtp-server, and all packages
Never use dynamic imports; always import on the top level
Run ESLint via @usesend/eslint-config and ensure no warnings remain before submitting PRs
Files:
apps/web/src/server/service/suppression-service.ts
apps/web/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use alias
~/for src imports in apps/web (e.g.,import { x } from "~/utils/x")
Files:
apps/web/src/server/service/suppression-service.ts
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/**/*.{ts,tsx}: Prefer to use TRPC for client-server communication unless explicitly asked otherwise in apps/web
Use Prisma for database access in apps/web
Files:
apps/web/src/server/service/suppression-service.ts
**/*.{ts,tsx,md}
📄 CodeRabbit inference engine (AGENTS.md)
Run Prettier 3 for code formatting on TypeScript, TSX, and Markdown files
Files:
apps/web/src/server/service/suppression-service.ts
🔇 Additional comments (2)
apps/web/src/server/service/suppression-service.ts (2)
146-148: Confirmed: Critical bug fix applied correctly.The failure detection logic has been corrected to check for
status === "fulfilled" && value === false, which properly handles the fact thatdeleteFromSesSuppressionListreturnsfalseon error rather than throwing. This resolves the critical issue flagged in previous reviews.
127-171: Well-structured best-effort AWS SES cleanup.The implementation correctly follows the design pattern outlined in the PR objectives:
- Early email normalization ensures consistency across all operations
- Multi-region support via domain query handles teams with domains in different AWS regions
- Promise.allSettled enables parallel cleanup without blocking on individual region failures
- Try-catch wrapper ensures AWS failures don't prevent local DB deletion
- Comprehensive logging provides visibility into cleanup outcomes
The approach appropriately balances reliability (local DB always updated) with best-effort external synchronization.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/web/src/server/service/suppression-service.ts">
<violation number="1" location="apps/web/src/server/service/suppression-service.ts:146">
P2: This filter only catches fulfilled promises returning `false`, but ignores rejected promises. If `getSesClient(region)` throws before the try block (e.g., invalid config), the promise will be rejected and this failure won't be counted or logged. Consider checking for both cases.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const failures = results.filter( | ||
| (r) => r.status === "fulfilled" && r.value === false | ||
| ); |
There was a problem hiding this comment.
P2: This filter only catches fulfilled promises returning false, but ignores rejected promises. If getSesClient(region) throws before the try block (e.g., invalid config), the promise will be rejected and this failure won't be counted or logged. Consider checking for both cases.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/server/service/suppression-service.ts, line 146:
<comment>This filter only catches fulfilled promises returning `false`, but ignores rejected promises. If `getSesClient(region)` throws before the try block (e.g., invalid config), the promise will be rejected and this failure won't be counted or logged. Consider checking for both cases.</comment>
<file context>
@@ -142,7 +142,10 @@ export class SuppressionService {
- const failures = results.filter((r) => r.status === "rejected");
+ // Check for failures - deleteFromSesSuppressionList returns false on error
+ const failures = results.filter(
+ (r) => r.status === "fulfilled" && r.value === false
+ );
</file context>
| const failures = results.filter( | |
| (r) => r.status === "fulfilled" && r.value === false | |
| ); | |
| const failures = results.filter( | |
| (r) => r.status === "rejected" || (r.status === "fulfilled" && r.value === false) | |
| ); |
✅ Addressed in 698ee27
Updated the filter logic for Promise.allSettled to include 'rejected' status as well as 'fulfilled' with a 'false' value. This ensures that any errors occurring before the try block in deleteFromSesSuppressionList are correctly caught and logged.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/web/src/server/service/suppression-service.ts (1)
152-161: Optional: Consider logging which specific regions failed.The current warning logs the count of failed regions, which is helpful. For easier debugging, consider also logging which specific regions failed (e.g., their identifiers or a sample).
🔎 Proposed enhancement
const failures = results.filter( (r) => r.status === "rejected" || (r.status === "fulfilled" && r.value === false) ); if (failures.length > 0) { + const failedRegionsList = uniqueRegions.filter((region, index) => { + const result = results[index]; + return ( + result.status === "rejected" || + (result.status === "fulfilled" && result.value === false) + ); + }); logger.warn( { email: normalizedEmail, teamId, failedRegions: failures.length, totalRegions: uniqueRegions.length, + regions: failedRegionsList, }, "Some AWS SES regions failed during suppression removal" ); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/src/server/service/suppression-service.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{tsx,ts,jsx,js}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Include all required imports and ensure proper naming of key components in React/NextJS code
Files:
apps/web/src/server/service/suppression-service.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use TypeScript-first approach with 2-space indent and semicolons enabled by Prettier in apps/web (Next.js), apps/marketing, apps/smtp-server, and all packages
Never use dynamic imports; always import on the top level
Run ESLint via @usesend/eslint-config and ensure no warnings remain before submitting PRs
Files:
apps/web/src/server/service/suppression-service.ts
apps/web/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use alias
~/for src imports in apps/web (e.g.,import { x } from "~/utils/x")
Files:
apps/web/src/server/service/suppression-service.ts
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/**/*.{ts,tsx}: Prefer to use TRPC for client-server communication unless explicitly asked otherwise in apps/web
Use Prisma for database access in apps/web
Files:
apps/web/src/server/service/suppression-service.ts
**/*.{ts,tsx,md}
📄 CodeRabbit inference engine (AGENTS.md)
Run Prettier 3 for code formatting on TypeScript, TSX, and Markdown files
Files:
apps/web/src/server/service/suppression-service.ts
🧬 Code graph analysis (1)
apps/web/src/server/service/suppression-service.ts (2)
apps/web/src/server/db.ts (1)
db(20-20)apps/web/src/server/aws/ses.ts (1)
deleteFromSesSuppressionList(319-346)
🔇 Additional comments (6)
apps/web/src/server/service/suppression-service.ts (6)
5-5: LGTM: Import correctly added.The import for
deleteFromSesSuppressionListis properly placed and follows the project's import conventions.
127-135: LGTM: Email normalization and region query.The email normalization is consistent with other methods in the class, and the region query correctly extracts unique AWS regions from the team's domains for multi-region cleanup.
137-143: LGTM: Best-effort multi-region cleanup correctly implemented.The use of
Promise.allSettledis appropriate for the best-effort pattern, ensuring all region cleanups are attempted even if some fail. The guard condition correctly skips AWS cleanup when no domains exist.
145-161: Excellent fix: Failure detection now handles both rejected and false returns.The updated filter correctly addresses the previous bug by checking for both:
- Rejected promises (if
getSesClientthrows before the try block)- Fulfilled promises returning
false(normal error handling path)This ensures all AWS SES cleanup failures are properly detected and logged.
163-173: LGTM: Error handling correctly implements best-effort pattern.The catch block appropriately logs AWS SES cleanup failures while allowing the local database deletion to proceed, which aligns with the PR's design decision that AWS failures should not block suppression removal.
175-223: LGTM: Local database deletion with consistent normalization.The local deletion correctly uses
normalizedEmailthroughout, ensuring consistency with the AWS SES cleanup. Error handling appropriately distinguishes between non-existent records (graceful return) and actual database errors (throws exception).
1 participant
Summary
Fixes #324 - When removing an email from the suppression list, now also removes it from AWS SES account-level suppression list.
Changes
DeleteSuppressedDestinationCommandDesign Decisions
Testing
Summary by cubic
Removes emails from AWS SES account-level suppression when they are deleted from our suppression list. Cleans up all team regions and keeps local deletion reliable (addresses #324).
Written for commit 698ee27. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.