-
-
Notifications
You must be signed in to change notification settings - Fork 204
feat: improve domain verification UX with enhanced feedback #202
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: improve domain verification UX with enhanced feedback #202
Conversation
- Add proper loading states with spinner for verify button - Show detailed verification status messages during process - Add toast notifications for success, error, and start events - Improve button text based on verification state - Add visual progress indicators and status section - Enhanced error handling in startVerification mutation - Track verification completion and show appropriate feedback
WalkthroughStricter delete confirmation (requires exact domain name), richer domain verification UX with toasts/spinners/status section and polling, and server-side hardening of startVerification to validate domain existence, set isVerifying/status, and return a structured { success, message } response. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant UI as Domains Page (UI)
participant API as startVerification (trpc)
participant DB as Database
U->>UI: Click "Verify domain"
UI->>UI: Show "Starting verification..." toast
UI->>API: startVerification(domainId)
API->>DB: findUnique(domainId)
alt domain found
API->>DB: update(isVerifying=true, status=PENDING)
API-->>UI: { success: true, message }
UI->>UI: start polling (refetchInterval)
UI-->>U: Show "Checking DNS records..." / spinner
else domain not found
API-->>UI: TRPCError NOT_FOUND
UI-->>U: Show error toast "Domain not found"
end
rect rgba(230,245,255,0.6)
loop Polling while isVerifying
UI->>DB: fetch domain status
DB-->>UI: status
alt status -> SUCCESS
UI-->>U: Show success toast "Verified"
UI->>UI: Button shows "Verified ✓ - Check again"
else status -> FAILED
UI-->>U: Show error toast "Verification failed"
UI->>UI: Show retry state and guidance
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
✨ Finishing Touches
🧪 Generate unit tests
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/src/app/(dashboard)/domains/[domainId]/page.tsx (2)
23-37
: Do not use React’s experimental use() in a client component; use useParams instead.Passing Promise props and calling use() on the client is unstable and will break on React <19. Next.js recommends useParams in client components.
Apply this diff:
-import React, { use, useEffect, useRef } from "react"; +import React, { useEffect, useRef } from "react"; +import { useParams } from "next/navigation"; @@ -export default function DomainItemPage({ - params, -}: { - params: Promise<{ domainId: string }>; -}) { - const { domainId } = use(params); +export default function DomainItemPage() { + const { domainId } = useParams<{ domainId: string }>();
202-224
: Fix MX row status binding
The MX row is wired tospfDetails
, which in your service maps to the AWS MailFromDomainStatus—not an MX-specific status. Since there is nomxStatus
(or similar) in the API, this will render the wrong badge for MX.• File:
apps/web/src/app/(dashboard)/domains/[domainId]/page.tsx
– Lines ~219–223: the<DnsVerificationStatus>
under the “MX” row usesdomainQuery.data?.spfDetails
.
• Service mapping: inapps/web/src/server/service/domain-service.ts
,spfDetails
is assigned fromMailFromAttributes.MailFromDomainStatus
, not MX.Suggested fixes:
- Remove the status badge from the MX row if no separate MX status exists.
- —OR— Rename
spfDetails
in your service and schema tomailFromDomainStatus
(or similar) and adjust the UI label to “Mail-From Domain Status” instead of “SPF” or “MX.”- <TableCell className=""> - <DnsVerificationStatus status={domainQuery.data?.spfDetails ?? "NOT_STARTED"} /> - </TableCell> + <!-- Remove or replace this block since `spfDetails` isn’t an MX-specific status -->
🧹 Nitpick comments (9)
apps/web/src/app/(dashboard)/domains/[domainId]/delete-domain.tsx (6)
120-123
: Harden the disable gate against whitespace/case mismatches.Users can accidentally include leading/trailing spaces or different casing. Normalize both sides to avoid unnecessary friction while still requiring an exact domain match.
Apply this diff:
- disabled={ - deleteDomainMutation.isPending || - domainForm.watch("domain") !== domain.name - } + disabled={ + deleteDomainMutation.isPending || + (domainForm.watch("domain") ?? "") + .trim() + .toLowerCase() !== domain.name.toLowerCase() + }
50-57
: Normalize comparison in submit handler as well.Mirror the same normalization in the server-side check to keep behavior consistent between UI gating and form validation.
- if (values.domain !== domain.name) { + const typed = (values.domain ?? "").trim().toLowerCase(); + if (typed !== domain.name.toLowerCase()) { domainForm.setError("domain", { message: "Domain name does not match", }); return; }
58-70
: Surface API errors to the user and keep cache consistent.Add an onError toast and optionally invalidate onSettled to ensure lists refresh even when the dialog remains open.
- deleteDomainMutation.mutate( + deleteDomainMutation.mutate( { id: domain.id, }, { onSuccess: () => { utils.domain.domains.invalidate(); setOpen(false); toast.success(`Domain ${domain.name} deleted`); router.replace("/domains"); }, + onError: (err) => { + toast.error(err.message || "Failed to delete domain"); + }, + onSettled: () => { + // Keep cache fresh regardless of outcome + utils.domain.domains.invalidate(); + }, } );
39-39
: Remove unused local state.domainName and setDomainName are never used; drop them to reduce noise.
- const [domainName, setDomainName] = useState("");
75-77
: Simplify onOpenChange.React will ignore no-op state sets; the conditional is unnecessary.
- onOpenChange={(_open) => (_open !== open ? setOpen(_open) : null)} + onOpenChange={setOpen}
117-126
: Optional: show a spinner while deleting for consistency with Verify button UX.Your Button in the verification flow uses isLoading/showSpinner. Mirror that here for consistent feedback.
- <Button + <Button type="submit" variant="destructive" disabled={ deleteDomainMutation.isPending || domainForm.watch("domain") !== domain.name } - > - {deleteDomainMutation.isPending ? "Deleting..." : "Delete"} + isLoading={deleteDomainMutation.isPending} + showSpinner + > + {deleteDomainMutation.isPending ? "Deleting..." : "Delete"}apps/web/src/app/(dashboard)/domains/[domainId]/page.tsx (3)
70-86
: Guard double-submissions and surface server message.Avoid starting verification while already verifying/pending, and display the message returned by the mutation for consistency with the backend.
- const handleVerify = () => { - toast.info("Starting domain verification..."); - verifyQuery.mutate( + const handleVerify = () => { + if (verifyQuery.isPending || domainQuery.data?.isVerifying) return; + toast.info("Starting domain verification..."); + verifyQuery.mutate( { id: Number(domainId) }, { - onSuccess: () => { - toast.success("Verification started successfully"); + onSuccess: (res) => { + toast.success(res?.message ?? "Verification started successfully"); }, onError: (error) => { toast.error(`Verification failed: ${error.message}`); }, onSettled: () => { domainQuery.refetch(); }, } ); };
125-141
: Disable Verify while verifying to prevent redundant requests.The button shows a spinner but remains clickable when isVerifying is true. Disable it in that state as well.
- <Button + <Button variant="outline" onClick={handleVerify} isLoading={verifyQuery.isPending || domainQuery.data?.isVerifying} showSpinner={true} - disabled={verifyQuery.isPending} + disabled={verifyQuery.isPending || domainQuery.data?.isVerifying} >
382-401
: Tighten prop typing for DnsVerificationStatus.This component expects domain verification states; constrain the prop to the known set to catch typos at compile time.
-const DnsVerificationStatus: React.FC<{ status: string }> = ({ status }) => { +type DomainStatusValue = "NOT_STARTED" | "PENDING" | "TEMPORARY_FAILURE" | "FAILED" | "SUCCESS"; +const DnsVerificationStatus: React.FC<{ status: DomainStatusValue }> = ({ status }) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/web/src/app/(dashboard)/domains/[domainId]/delete-domain.tsx
(2 hunks)apps/web/src/app/(dashboard)/domains/[domainId]/page.tsx
(4 hunks)apps/web/src/server/api/routers/domain.ts
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
{apps,packages}/**/*.{js,jsx,ts,tsx,css,scss,md,mdx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Prettier with the Tailwind plugin for code formatting
Files:
apps/web/src/app/(dashboard)/domains/[domainId]/delete-domain.tsx
apps/web/src/server/api/routers/domain.ts
apps/web/src/app/(dashboard)/domains/[domainId]/page.tsx
{apps,packages}/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
{apps,packages}/**/*.{js,jsx,ts,tsx}
: Group imports by source (internal/external) and alphabetize them
Use camelCase for variables and functions, PascalCase for components and classes
Use try/catch with specific error types for error handling
Files:
apps/web/src/app/(dashboard)/domains/[domainId]/delete-domain.tsx
apps/web/src/server/api/routers/domain.ts
apps/web/src/app/(dashboard)/domains/[domainId]/page.tsx
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
{apps,packages}/**/*.{ts,tsx}
: Use strong typing in TypeScript, avoidany
, and use Zod for validation
Follow Vercel style guides with strict TypeScript
Files:
apps/web/src/app/(dashboard)/domains/[domainId]/delete-domain.tsx
apps/web/src/server/api/routers/domain.ts
apps/web/src/app/(dashboard)/domains/[domainId]/page.tsx
{apps,packages}/**/*.{jsx,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
{apps,packages}/**/*.{jsx,tsx}
: Use functional React components with hooks and group related hooks together
In React components, structure code with props at the top, hooks next, helper functions, then JSX
Files:
apps/web/src/app/(dashboard)/domains/[domainId]/delete-domain.tsx
apps/web/src/app/(dashboard)/domains/[domainId]/page.tsx
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use tRPC for internal API endpoints
Files:
apps/web/src/app/(dashboard)/domains/[domainId]/delete-domain.tsx
apps/web/src/server/api/routers/domain.ts
apps/web/src/app/(dashboard)/domains/[domainId]/page.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Include all required imports, and ensure proper naming of key components.
Files:
apps/web/src/app/(dashboard)/domains/[domainId]/delete-domain.tsx
apps/web/src/server/api/routers/domain.ts
apps/web/src/app/(dashboard)/domains/[domainId]/page.tsx
🔇 Additional comments (5)
apps/web/src/app/(dashboard)/domains/[domainId]/delete-domain.tsx (1)
104-104
: Nice UX improvement: placeholder matches the actual domain.Showing the real domain as the placeholder reduces user error and clarifies the confirmation step. Looks good.
apps/web/src/server/api/routers/domain.ts (1)
36-45
: I've requested inspection of thedomainProcedure
and theteamProcedure
implementation to confirm whether team-based scoping and authorization are enforced. I’ll update my review comment once we have clarity.apps/web/src/app/(dashboard)/domains/[domainId]/page.tsx (3)
51-69
: LGTM: clear, minimal transition toasts based on prior status.The previousStatusRef pattern avoids initial-render toasts and keeps transitions crisp. Nicely done.
149-185
: LGTM: verification status panel adds great, actionable feedback.The progressive copy and iconography make state transitions obvious without being noisy.
56-64
: No action needed: this is a server componentThe
page.tsx
underapps/web/src/app/(dashboard)/domains/[domainId]/page.tsx
does not include a"use client"
directive, so it’s a Next.js Server Component by default. Imports from@prisma/client
here run entirely on the server and do not bloat the client bundle or affect edge runtimes. You can safely leave theDomainStatus
enum imports as-is.Likely an incorrect or invalid review comment.
- Replace generic error throws with TRPCError for better error management - Use DomainStatus enum for setting domain status - Improve error messages for domain not found and user email validation
@enemyrr is attempting to deploy a commit to the kmkoushik's projects Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (5)
apps/web/src/app/(dashboard)/domains/[domainId]/delete-domain.tsx (3)
120-123
: Trim input before comparison to prevent false negatives; consider useWatch.Users often add accidental spaces. Trim before equality check (keep case-sensitive per “exact” requirement). Optional: prefer useWatch over calling watch inside JSX.
Apply within these lines:
- disabled={ - deleteDomainMutation.isPending || - domainForm.watch("domain") !== domain.name - } + disabled={ + deleteDomainMutation.isPending || + (domainForm.watch("domain")?.trim() ?? "") !== domain.name + }And update submit validation accordingly (outside this hunk):
// in onDomainDelete const typed = values.domain.trim(); if (typed !== domain.name) { domainForm.setError("domain", { message: "Domain name does not match" }); return; }
39-39
: Remove unused state.domainName/setDomainName are unused; delete to reduce noise.
- const [domainName, setDomainName] = useState("");
1-1
: Rename file to PascalCase
Renameapps/web/src/app/(dashboard)/domains/[domainId]/delete-domain.tsx
toDeleteDomain.tsx
; no import references found that require updating.apps/web/src/server/api/routers/domain.ts (2)
39-56
: Add idempotency guard to avoid redundant writes and duplicate UX toasts.Early-return when already verifying or verified.
startVerification: domainProcedure.mutation(async ({ ctx, input }) => { try { const domain = await ctx.db.domain.findUnique({ where: { id: input.id }, }); if (!domain) { throw new TRPCError({ code: "NOT_FOUND", message: "Domain not found" }); } + if (domain.isVerifying || domain.status === DomainStatus.VERIFIED) { + return { + success: true, + message: domain.status === DomainStatus.VERIFIED + ? "Domain already verified" + : "Domain verification already in progress", + }; + } await ctx.db.domain.update({ where: { id: input.id }, data: { isVerifying: true, status: DomainStatus.PENDING, }, });
68-114
: Optional: standardize db access.This router mixes ctx.db and db. Pick one (prefer ctx.db) for consistency and easier mocking.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/src/app/(dashboard)/domains/[domainId]/delete-domain.tsx
(2 hunks)apps/web/src/app/(dashboard)/domains/[domainId]/page.tsx
(4 hunks)apps/web/src/server/api/routers/domain.ts
(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/app/(dashboard)/domains/[domainId]/page.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Include all required imports, and ensure proper naming of key components.
Files:
apps/web/src/server/api/routers/domain.ts
apps/web/src/app/(dashboard)/domains/[domainId]/delete-domain.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}
: Use 2-space indentation in TypeScript code (enforced by Prettier)
Use semicolons in TypeScript code (enforced by Prettier)
Do not use dynamic imports
Files:
apps/web/src/server/api/routers/domain.ts
apps/web/src/app/(dashboard)/domains/[domainId]/delete-domain.tsx
**/*.{ts,tsx,md}
📄 CodeRabbit inference engine (AGENTS.md)
Format code with Prettier 3 via pnpm format for TypeScript and Markdown files
Files:
apps/web/src/server/api/routers/domain.ts
apps/web/src/app/(dashboard)/domains/[domainId]/delete-domain.tsx
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
In apps/web, use the
/ alias for src imports (e.g., import { x } from "/utils/x")
Files:
apps/web/src/server/api/routers/domain.ts
apps/web/src/app/(dashboard)/domains/[domainId]/delete-domain.tsx
**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Name React component files in PascalCase (e.g., AppSideBar.tsx)
Files:
apps/web/src/app/(dashboard)/domains/[domainId]/delete-domain.tsx
🔇 Additional comments (4)
apps/web/src/app/(dashboard)/domains/[domainId]/delete-domain.tsx (1)
104-104
: LGTM: clearer confirmation hint.Using the exact domain as the placeholder improves clarity for destructive confirmation.
apps/web/src/server/api/routers/domain.ts (3)
2-3
: LGTM: typed errors and enum import.Brings stronger guarantees and consistent error semantics.
117-122
: LGTM: precise error codes for not-found and bad-request.Improves client handling and telemetry.
56-66
: LGTM: structured payload + robust TRPCError propagation. We couldn’t locate any client-sidestartVerification
calls—please manually verify that the UI invokes this RPC and handles the returned{ success, message }
payload instead of expecting void.
Summary
Changes Made
Test Plan
Summary by CodeRabbit
New Features
Improvements