Managed email domain deletion and Cloudflare DNS import UX#1442
Open
mantrakp04 wants to merge 2 commits into
Open
Managed email domain deletion and Cloudflare DNS import UX#1442mantrakp04 wants to merge 2 commits into
mantrakp04 wants to merge 2 commits into
Conversation
Let projects remove unused managed domains with upstream cleanup, and streamline Cloudflare NS record import during onboarding. Co-authored-by: Cursor <[email protected]>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughEnd-to-end managed email domain deletion: DB helpers, backend deletion service (Resend + DNSimple cleanup), admin API and app mappings, dashboard Cloudflare detection and deletion UI, plus E2E tests covering success, conflicts, auth, and not-found cases. ChangesManaged Email Domain Deletion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
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)
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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds managed email domain deletion support end-to-end (backend endpoint + SDK + dashboard UX), plus Cloudflare-specific DNS setup affordances and E2E coverage to validate deletion behavior and authorization.
Changes:
- Added an internal admin-only delete endpoint for managed email domains, including provider-side cleanup and “in-use” rejection.
- Exposed the delete operation through the admin SDK/template layer and surfaced it in the dashboard UI with confirmation.
- Improved DNS setup UX with Cloudflare detection, zone file download, and import instructions; added E2E tests for delete flows.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/template/src/lib/stack-app/apps/interfaces/admin-app.ts | Adds deleteManagedEmailDomain to the admin app interface. |
| packages/template/src/lib/stack-app/apps/implementations/admin-app-impl.ts | Implements the new admin SDK method and maps parameters to the interface request payload. |
| packages/stack-shared/src/interface/admin-interface.ts | Adds a StackAdminInterface method to call the internal delete endpoint. |
| apps/e2e/tests/backend/endpoints/api/v1/internal/managed-email-onboarding.test.ts | Adds E2E tests for auth, success, in-use rejection, post-switch deletion, and 404 deletion cases. |
| apps/dashboard/src/app/(main)/(protected)/projects/[projectId]/email-settings/domain-settings.tsx | Adds dashboard UI for deleting unused managed domains and Cloudflare DNS import UX. |
| apps/backend/src/lib/managed-email-onboarding.tsx | Implements managed domain deletion logic with Resend/DNSimple cleanup and in-use guard. |
| apps/backend/src/lib/managed-email-domains.tsx | Adds DB helpers to delete a managed domain row and count remaining references to a subdomain. |
| apps/backend/src/app/api/latest/internal/emails/managed-onboarding/delete/route.tsx | Introduces the internal admin delete endpoint route. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Greptile SummaryThis PR adds a managed email domain deletion feature end-to-end: an admin-only backend endpoint with Resend/DNSimple cleanup, an in-use guard, an SDK method, and a dashboard confirmation dialog — plus a Cloudflare detection UX with zone-file download and import instructions for the DNS setup step.
Confidence Score: 5/5Safe to merge; tenancy ownership is correctly enforced, the in-use guard prevents deleting active senders, and partial failures leave the system retryable. The delete flow is well-structured with proper auth checks, a clear tenancy boundary, and retryable external-cleanup ordering. Two minor observations (a tiny TOCTOU window in shared-zone cleanup and the dialog not reflecting the async operation's actual completion) do not affect correctness in normal use. The zone-cleanup section in Important Files Changed
Sequence DiagramsequenceDiagram
participant Admin as Admin (Dashboard)
participant API as DELETE /managed-onboarding/delete
participant DB as Database
participant Resend as Resend API
participant DNS as DNSimple API
Admin->>API: "POST { resend_domain_id }"
API->>DB: getManagedEmailDomainByResendDomainId()
DB-->>API: domain row (tenancyId, subdomain, senderLocalPart)
API->>API: tenancy ownership check
API->>API: isManagedEmailDomainInUseForTenancy()
alt domain is in use
API-->>Admin: 409 Cannot delete active domain
else not in use
API->>Resend: "DELETE /domains/{resendDomainId}"
Resend-->>API: 200 or 404 (both OK)
API->>DB: countManagedEmailDomainsBySubdomainExcludingId()
DB-->>API: remaining count
alt "remaining == 0"
API->>DNS: "DELETE /zones/{subdomain}"
DNS-->>API: deleted or not_found
end
API->>DB: deleteManagedEmailDomainById()
DB-->>API: deleted row
API-->>Admin: "200 { status: deleted }"
end
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
apps/backend/src/lib/managed-email-onboarding.tsx:711-720
**TOCTOU window in shared-zone cleanup**
Between `countManagedEmailDomainsBySubdomainExcludingId` returning 0 and `deleteDnsimpleZoneByName` completing, a concurrent `setupManagedEmailProvider` call for the same subdomain (from any tenancy) will reach `createOrReuseDnsimpleZone`, find the existing zone, and attach its new DB row to it. The deletion that follows then removes the zone from under the just-created row, leaving it with a valid DB record but a missing DNS zone. In practice two tenancies simultaneously racing a setup against a delete for the exact same subdomain is rare, but the broken state is silent — the new domain's status will never advance past `pending_dns`.
### Issue 2 of 2
apps/dashboard/src/app/(main)/(protected)/projects/[projectId]/email-settings/domain-settings.tsx:1186-1193
`runAsynchronouslyWithAlert` not awaited — dialog closes before the delete resolves.
`ActionDialog` does `await okButton.onClick?.()`. Because `onClick` fires `runAsynchronouslyWithAlert` without awaiting it, the outer promise resolves immediately and the dialog closes before the delete (or any error) has a chance to complete. Errors are still surfaced (good), but the "Remove" button shows no loading state, the dialog closes optimistically, and the domain remains in the list until `refreshDomains` eventually settles. Awaiting the call makes the dialog's built-in loading/disabled state cover the full operation.
```suggestion
onClick: async () => {
if (!confirmDelete) return;
await runAsynchronouslyWithAlert(async () => {
await stackAdminApp.deleteManagedEmailDomain({ resendDomainId: confirmDelete.domainId });
toast({ title: "Domain removed", description: `${confirmDelete.senderLocalPart}@${confirmDelete.subdomain} was removed.`, variant: "success" });
await refreshDomains();
});
},
```
Reviews (2): Last reviewed commit: "Refactor domain settings to improve subd..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: 3
🧹 Nitpick comments (2)
apps/e2e/tests/backend/endpoints/api/v1/internal/managed-email-onboarding.test.ts (1)
205-206: ⚡ Quick winReplace fixed sleeps with condition-based polling to reduce E2E flakiness.
Line 205 and Line 243 use
wait(1500)as a timing assumption for verification readiness. This can intermittently fail under slower CI timing; poll/checkuntil"verified"with a bounded timeout instead.♻️ Proposed fix
import { wait } from "`@stackframe/stack-shared/dist/utils/promises`"; import { describe } from "vitest"; import { it } from "../../../../../helpers"; import { Project, niceBackendFetch } from "../../../../backend-helpers"; +async function waitForManagedDomainVerified(domainId: string, subdomain: string, senderLocalPart: string) { + const timeoutAt = performance.now() + 10_000; + while (true) { + const checkResponse = await niceBackendFetch("/api/v1/internal/emails/managed-onboarding/check", { + method: "POST", + accessType: "admin", + body: { + domain_id: domainId, + subdomain, + sender_local_part: senderLocalPart, + }, + }); + if (checkResponse.body.status === "verified") return; + if (performance.now() > timeoutAt) { + throw new Error(`Timed out waiting for managed domain ${domainId} to become verified`); + } + await wait(250); + } +} + describe("managed email onboarding internal endpoints", () => { ... - await wait(1500); + await waitForManagedDomainVerified(setupResponse.body.domain_id, "mail.example.com", "noreply"); ... - await wait(1500); + await waitForManagedDomainVerified(setupResponse.body.domain_id, "mail.example.com", "noreply");Also applies to: 243-244
🤖 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 `@apps/e2e/tests/backend/endpoints/api/v1/internal/managed-email-onboarding.test.ts` around lines 205 - 206, Replace the fixed wait(1500) sleeps used before verification with a condition-based polling loop that repeatedly calls the /check endpoint until it returns a response indicating "verified" or a bounded timeout elapses; update the test that uses wait(1500) (and the other occurrence) to poll at short intervals (e.g., 200–500ms) for a maximum duration (e.g., 10s), assert success when the /check response contains "verified", and fail the test if the timeout is reached—use the existing wait helper only for the poll interval and keep the polling logic next to the verification steps so the test no longer depends on a fixed sleep.apps/dashboard/src/app/(main)/(protected)/projects/[projectId]/email-settings/domain-settings.tsx (1)
326-341: ⚡ Quick winKey the Cloudflare lookup off the subdomain, not the whole setup object.
Line 341 depends on
setupState, so everysetSetupState({ ...setupState, status })during verification re-runs the same DoH lookup even though the queried domain did not change. That adds an extra third-party request to each verification click for no benefit.♻️ Proposed fix
+ const setupSubdomain = setupState?.subdomain; + useEffect(() => { - if (!props.open || stage !== 2 || !setupState) { + if (!props.open || stage !== 2 || setupSubdomain == null) { setCloudflareApex(null); return; } let cancelled = false; runAsynchronously(async () => { - const zone = await findZoneApex(setupState.subdomain); + const zone = await findZoneApex(setupSubdomain); if (cancelled || !zone) return; const usesCloudflare = zone.nameServers.some((n) => /(^|\.)cloudflare\.com$/i.test(n)); if (usesCloudflare) setCloudflareApex(zone.apex); }); return () => { cancelled = true; }; - }, [props.open, stage, setupState]); + }, [props.open, stage, setupSubdomain]);🤖 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 `@apps/dashboard/src/app/`(main)/(protected)/projects/[projectId]/email-settings/domain-settings.tsx around lines 326 - 341, The effect that runs findZoneApex is being re-triggered on every change to the entire setupState object; change its dependency to the specific subdomain so the DoH lookup only runs when the queried domain changes: in the useEffect that calls findZoneApex (and updates setCloudflareApex), replace the dependency on setupState with the subdomain value (e.g., setupState?.subdomain) and guard the early-return checks against that subdomain variable instead of the whole setupState to avoid unnecessary lookups when setSetupState({...setupState, status}) is called.
🤖 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 `@apps/backend/src/lib/managed-email-domains.tsx`:
- Around line 244-249: The COUNT query in globalPrismaClient.$queryRaw against
"ManagedEmailDomain" counts archived/inactive rows, causing the deletion flow to
think a zone is still referenced; update the WHERE clause in that query (the one
using options.subdomain and options.excludeId) to exclude archived/inactive
records (for example add AND "archivedAt" IS NULL or AND "isArchived" = false
depending on the actual schema column used for soft-deletes) so only live
domains are counted; make sure to use the same archived/active column and
semantics used elsewhere in the codebase.
In `@apps/backend/src/lib/managed-email-onboarding.tsx`:
- Around line 720-729: The current "last reference" check uses
countManagedEmailDomainsBySubdomainExcludingId followed separately by
deleteDnsimpleZoneByName, which is racy; serialize cleanup for a given subdomain
by acquiring a per-subdomain critical section (e.g., a DB advisory lock or mutex
keyed by domain.subdomain) and perform the count-and-delete inside the same
transaction/locked section so the decision is atomic. Update the logic around
countManagedEmailDomainsBySubdomainExcludingId and deleteDnsimpleZoneByName (or
the caller createOrReuseDnsimpleZone flow) to: acquire lock for
domain.subdomain, re-check count (excluding the row being deleted) inside the
lock/transaction, call deleteDnsimpleZoneByName only if count === 0, then
release the lock; ensure lock acquisition/release errors are handled and do not
leave the zone orphaned.
In
`@apps/dashboard/src/app/`(main)/(protected)/projects/[projectId]/email-settings/domain-settings.tsx:
- Around line 1085-1093: The icon-only delete button (the button with
onClick={() => setConfirmDelete(d)} and the <Trash /> icon) lacks an accessible
name; add an explicit aria-label (for example aria-label={`Remove domain
${d.domain}` or a generic "Remove domain") on that button to provide a clear
label for screen readers while keeping the existing title; ensure the aria-label
text conveys the destructive action and identifies the domain when possible.
---
Nitpick comments:
In
`@apps/dashboard/src/app/`(main)/(protected)/projects/[projectId]/email-settings/domain-settings.tsx:
- Around line 326-341: The effect that runs findZoneApex is being re-triggered
on every change to the entire setupState object; change its dependency to the
specific subdomain so the DoH lookup only runs when the queried domain changes:
in the useEffect that calls findZoneApex (and updates setCloudflareApex),
replace the dependency on setupState with the subdomain value (e.g.,
setupState?.subdomain) and guard the early-return checks against that subdomain
variable instead of the whole setupState to avoid unnecessary lookups when
setSetupState({...setupState, status}) is called.
In
`@apps/e2e/tests/backend/endpoints/api/v1/internal/managed-email-onboarding.test.ts`:
- Around line 205-206: Replace the fixed wait(1500) sleeps used before
verification with a condition-based polling loop that repeatedly calls the
/check endpoint until it returns a response indicating "verified" or a bounded
timeout elapses; update the test that uses wait(1500) (and the other occurrence)
to poll at short intervals (e.g., 200–500ms) for a maximum duration (e.g., 10s),
assert success when the /check response contains "verified", and fail the test
if the timeout is reached—use the existing wait helper only for the poll
interval and keep the polling logic next to the verification steps so the test
no longer depends on a fixed sleep.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 535d9443-4e74-49ef-8313-e3dde8def862
⛔ Files ignored due to path filters (1)
apps/dashboard/public/assets/cloudflare-import-dns.pngis excluded by!**/*.png
📒 Files selected for processing (8)
apps/backend/src/app/api/latest/internal/emails/managed-onboarding/delete/route.tsxapps/backend/src/lib/managed-email-domains.tsxapps/backend/src/lib/managed-email-onboarding.tsxapps/dashboard/src/app/(main)/(protected)/projects/[projectId]/email-settings/domain-settings.tsxapps/e2e/tests/backend/endpoints/api/v1/internal/managed-email-onboarding.test.tspackages/stack-shared/src/interface/admin-interface.tspackages/template/src/lib/stack-app/apps/implementations/admin-app-impl.tspackages/template/src/lib/stack-app/apps/interfaces/admin-app.ts
…main removal feedback. - Updated the `ManagedDomainSetupDialog` to use `setupState?.subdomain` for better clarity and reliability in Cloudflare DNS setup. - Enhanced the domain removal button with an `aria-label` for improved accessibility. - Wrapped the domain deletion process in `runAsynchronouslyWithAlert` to provide user feedback during the operation.
N2D4
reviewed
May 21, 2026
| const responseBody = await response.text(); | ||
| throw new StatusError( | ||
| 502, | ||
| `DNSimple returned ${response.status} when deleting managed email zone ${zoneName}: ${responseBody.slice(0, 500)}`, |
Contributor
There was a problem hiding this comment.
we should never ever return messages with internal information
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
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.
Summary
Test plan
pnpm test run managed-email-onboardingMade with Cursor
Summary by CodeRabbit
New Features
Tests