fix(domains): unblock delete when domain was removed from SES out-of-band#394
fix(domains): unblock delete when domain was removed from SES out-of-band#394Bluestart83 wants to merge 3 commits intousesend:mainfrom
Conversation
|
@ali-nld is attempting to deploy a commit to the kmkoushik's projects Team on Vercel. A member of the Team first needs to authorize it. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
WalkthroughThis change adds explicit error handling for domain deletion across the stack: the UI mutation now reports deletion failures via an Possibly related PRs
Suggested reviewers
🚥 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. 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.
🧹 Nitpick comments (1)
apps/web/src/app/(dashboard)/domains/domain-list.tsx (1)
12-12: Use the~/alias for the new import.This file lives under
apps/web/src, so the new relative path should follow the repo import convention instead of reaching across the directory tree.♻️ Proposed fix
-import { DeleteDomain } from "./[domainId]/delete-domain"; +import { DeleteDomain } from "~/app/(dashboard)/domains/[domainId]/delete-domain";As per coding guidelines,
apps/web/src/**/*.{ts,tsx}: Use the~/alias for imports from src in apps/web.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/`(dashboard)/domains/domain-list.tsx at line 12, Import uses a relative path that crosses the directory tree; update the import of DeleteDomain to use the apps/web src alias. Replace the existing import of DeleteDomain with the repo alias form (use the ~/ prefix) so the symbol DeleteDomain from "./[domainId]/delete-domain" is imported via "~/app/(dashboard)/domains/[domainId]/delete-domain" (adjust path under src as needed) to comply with apps/web src import conventions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/web/src/app/`(dashboard)/domains/domain-list.tsx:
- Line 12: Import uses a relative path that crosses the directory tree; update
the import of DeleteDomain to use the apps/web src alias. Replace the existing
import of DeleteDomain with the repo alias form (use the ~/ prefix) so the
symbol DeleteDomain from "./[domainId]/delete-domain" is imported via
"~/app/(dashboard)/domains/[domainId]/delete-domain" (adjust path under src as
needed) to comply with apps/web src import conventions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1c21feff-561c-40df-ade8-9b895a4e176b
📒 Files selected for processing (3)
apps/web/src/app/(dashboard)/domains/[domainId]/delete-domain.tsxapps/web/src/app/(dashboard)/domains/domain-list.tsxapps/web/src/server/aws/ses.ts
Bluestart83
changed the title
Reuse DeleteDomain component from detail page; same typed confirmation dialog. Adds delete affordance to /domains list rows.
- aws/ses.ts: catch NotFoundException on DeleteTenantResourceAssociation and DeleteEmailIdentity, log warn and continue (record can still be removed from DB when the domain was already deleted on SES). Other SES errors are now logged with full context before rethrow. - delete-domain.tsx: add onError toast so failures surface in the UI instead of silently leaving the dialog open.
Bluestart83
force-pushed
the
pr/delete-domain-list
branch
from
e2f5be6 to
3521883
Compare
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/server/aws/ses.ts`:
- Around line 187-193: The DeleteEmailIdentity flow currently returns false
silently when response.$metadata.httpStatusCode !== 200; update the code around
the DeleteEmailIdentityCommand handling (the block that calls
sesClient.send(command)) to log the full SES response and context (including the
domain/email identity and response metadata/body) before returning false so
downstream code has the SES response details; keep the existing catch/error
logging intact but ensure non-200 responses are explicitly logged (e.g., log
response and response.$metadata.httpStatusCode) prior to returning false.
🪄 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: 06da3fd9-d1e0-4cae-b981-f35736d17c3f
📒 Files selected for processing (3)
apps/web/src/app/(dashboard)/domains/[domainId]/delete-domain.tsxapps/web/src/app/(dashboard)/domains/domain-list.tsxapps/web/src/server/aws/ses.ts
✅ Files skipped from review due to trivial changes (1)
- apps/web/src/app/(dashboard)/domains/domain-list.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/app/(dashboard)/domains/[domainId]/delete-domain.tsx
Snapshot of UI change
|
Address CodeRabbit review: when DeleteEmailIdentity returns a non-200 status without throwing, the function silently returned false, leaving callers with no SES context. Now logs domain/region/response before returning false, consistent with the rest of this PR's no-silent-failures philosophy.
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/src/server/aws/ses.ts (1)
192-197: Prefer throwing on non-200DeleteEmailIdentityfor consistent failure semantics.This branch currently returns
falsewhile other SES error paths throw. Throwing here keeps error propagation consistent and better aligns with UIonErrorflows.Suggested patch
if (response.$metadata.httpStatusCode !== 200) { logger.error( { domain, region, response }, "[ses.deleteDomain] non-200 from DeleteEmailIdentity" ); - return false; + throw new Error("Failed to delete email identity"); } return true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/server/aws/ses.ts` around lines 192 - 197, Replace the inconsistent return false branch in the DeleteEmailIdentity handling with a thrown error so failures propagate like other SES error paths: when response.$metadata.httpStatusCode !== 200 (inside the DeleteEmailIdentity / [ses.deleteDomain] block), log the error as currently done (logger.error with { domain, region, response }) and then throw a new Error that includes a clear message (e.g., "DeleteEmailIdentity failed with non-200 status") and relevant response metadata so callers (and UI onError flows) receive an exception instead of a boolean.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/web/src/server/aws/ses.ts`:
- Around line 192-197: Replace the inconsistent return false branch in the
DeleteEmailIdentity handling with a thrown error so failures propagate like
other SES error paths: when response.$metadata.httpStatusCode !== 200 (inside
the DeleteEmailIdentity / [ses.deleteDomain] block), log the error as currently
done (logger.error with { domain, region, response }) and then throw a new Error
that includes a clear message (e.g., "DeleteEmailIdentity failed with non-200
status") and relevant response metadata so callers (and UI onError flows)
receive an exception instead of a boolean.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9274514d-e7d8-4857-8ac1-6fe684915985
📒 Files selected for processing (1)
apps/web/src/server/aws/ses.ts
1 participant
The bug
When a domain identity is deleted from AWS SES outside useSend (manual cleanup in the AWS console, an expired sandbox identity, an account migration, etc.), useSend gets stuck:
Domainrecord.deleteDomain()inapps/web/src/server/aws/ses.tsdoes not catchNotFoundExceptionfromDeleteEmailIdentityCommand/DeleteTenantResourceAssociationCommand. So the existing "Delete domain" button on/domains/[id]throws and aborts before deleting the DB row — the orphan is impossible to remove from the UI.isVerifying=truestate, the detail page itself becomes inaccessible:getDomain()callsrefreshDomainVerification(), which callsses.getDomainIdentity()unconditionally — that throws on a missing SES identity and the tRPC query errors out. So even reaching the Delete button can be blocked.DELETE FROM "Domain" WHERE id = ...), which is unacceptable for self-hosters and not really an option for cloud users at all.This is purely a useSend/SES integration issue — the SES side is consistent (identity gone), it's the app that doesn't handle a recoverable state.
The fix
Two complementary changes:
Make
deleteDomain()idempotent against an already-removed SES identity (the actual fix):DeleteTenantResourceAssociationandDeleteEmailIdentitycatchNotFoundException,logger.warn(with{ domain, region, sesTenantId }), and continue. The DB row gets removed in that case — exactly what an admin trying to recover wants.{ err, domain, region, sesTenantId }) before rethrow — previously the error was thrown bare with no log, so root cause was invisible.onErrortoast so failures actually surface instead of silently leaving the dialog open with no feedback.Add a "Delete domain" button on the
/domainslist page so the action is reachable even when the detail page itself is broken (case 3 above). Reuses the existingDeleteDomaincomponent (same typed-confirmation dialog, same mutation), no behavior change on the happy path.After this PR, an admin who lost the SES identity out-of-band can recover entirely from the UI: hit Delete on the row in
/domains, confirm, done. No DB surgery.Linked issues
None — drive-by fix while recovering from a manual SES cleanup. Happy to file an issue first if maintainers prefer.
Migration notes
None — no schema changes, no env changes, no infra changes.
Screenshot
/domainslist with the new "Delete domain" button (domain name redacted):Verification steps
/domains, type the domain name, confirm → row disappears, success toast (regular happy-path delete still works exactly like before).[ses.deleteDomain] identity already gone on SES, continuing.pnpm tsc --noEmitclean onapps/web.Summary by CodeRabbit
New Features
Bug Fixes