-
Notifications
You must be signed in to change notification settings - Fork 1.1k
improvement: new custom domain flow #868
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
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis update introduces a major refactor of the custom domain management UI in the organization's dashboard. A new multi-step dialog for domain setup and verification is added, with supporting components and types. The previous inline domain management logic is removed and replaced by this dialog. Additional UI, dependency, and minor style adjustments are included. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CustomDomain
participant CustomDomainDialog
participant API
User->>CustomDomain: Click "Set up" or "Remove" button
CustomDomain->>CustomDomainDialog: Open dialog (if setup)
CustomDomainDialog->>User: Show stepper (domain input, verification, success)
User->>CustomDomainDialog: Enter domain & submit
CustomDomainDialog->>API: Update domain / Remove existing domain
API-->>CustomDomainDialog: Respond with result
CustomDomainDialog->>User: Show verification instructions
User->>CustomDomainDialog: Trigger verification check
CustomDomainDialog->>API: Poll/check verification status
API-->>CustomDomainDialog: Respond with verification status
CustomDomainDialog->>User: Show success step (confetti)
CustomDomainDialog->>CustomDomain: Close dialog and refresh status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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: 13
🔭 Outside diff range comments (2)
apps/web/actions/organization/update-domain.ts (2)
33-41: Normalize and validate domain server-side to ensure uniqueness and consistencyNormalize early to avoid case/whitespace duplicates and improve matching.
Proposed change:
-export async function updateDomain(domain: string, organizationId: string) { +export async function updateDomain(domain: string, organizationId: string) { const user = await getCurrentUser(); @@ - // Check if domain is already being used by another organization + // Normalize input and check if domain is already being used by another organization + const normalizedDomain = domain.trim().toLowerCase(); + const existingDomain = await db() .select() .from(organizations) - .where(eq(organizations.customDomain, domain)) + .where(eq(organizations.customDomain, normalizedDomain)) .limit(1); @@ - const addDomainResponse = await addDomain(domain); + const addDomainResponse = await addDomain(normalizedDomain); @@ .set({ - customDomain: domain, + customDomain: normalizedDomain, domainVerified: null, }) @@ - const status = await checkDomainStatus(domain); + const status = await checkDomainStatus(normalizedDomain);Follow-up (schema-level): enforce a unique index on organizations.customDomain (lowercased) to guarantee uniqueness at the DB layer.
Also applies to: 45-56, 59-66
73-77: Don’t swallow non-Error throwables; add a safe fallbackIf a non-Error is thrown, this path currently returns undefined without signaling failure. Add a fallback throw.
Proposed change:
} catch (error) { if (error instanceof Error) { throw new Error(error.message); } + // Fallback for non-Error throwables + throw new Error("Failed to update domain"); }
🧹 Nitpick comments (11)
apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/types.ts (2)
13-18: Tighten error map typing and intentUse Partial<Record<...>> to reflect optional presence of errors per step. This prevents forcing consumers to initialize empty strings and better models sparseness.
-export interface StepState { +export interface StepState { currentIndex: number; totalSteps: number; canNavigateBack: boolean; - errors: Record<string, string>; + // keys: step ids; values only exist when there's an error for that step + errors: Partial<Record<string, string>>; }
20-26: Consider action for dynamic step counts and error lifecycleIf steps can be added/removed based on feature flags/subscription, a SET_TOTAL_STEPS action (or derive from config) helps keep state consistent. Also a CLEAR_ALL_ERRORS action can be useful when re-entering the flow.
export type StepAction = | { type: "NEXT_STEP" } | { type: "PREV_STEP" } | { type: "GO_TO_STEP"; payload: number } | { type: "SET_ERROR"; payload: { stepId: string; error: string } } | { type: "CLEAR_ERROR"; payload: string } + | { type: "CLEAR_ALL_ERRORS" } + | { type: "SET_TOTAL_STEPS"; payload: number } | { type: "RESET" };apps/web/app/(org)/dashboard/settings/organization/components/OrganizationDetailsCard.tsx (1)
21-21: Prefer semantic separators and verify spacing consistencyUse a semantic hr (with aria-hidden if purely decorative) instead of empty divs. Also confirm whether both separators should have the same vertical spacing.
Proposed change:
- <div className="mt-2 w-full h-px border-t border-dashed border-gray-3" /> + <hr aria-hidden="true" className="mt-2 w-full h-px border-t border-dashed border-gray-3" /> <CustomDomain /> - <div className="w-full h-px border-t border-dashed border-gray-3" /> + <hr aria-hidden="true" className="w-full h-px border-t border-dashed border-gray-3" />Also applies to: 23-23
packages/ui/src/components/Dialog.tsx (1)
18-18: Verify Dialog z-index layering against existing overlaysA quick scan revealed several components using higher z-index values than the Dialog’s overlay (500) and content (501):
- Dropdown.tsx: z-[1000000000]
- CustomDomainDialog.tsx (Confetti): z-[600]
- Desktop recordings overlay (recordings-overlay.tsx): z-[999999]
- Editor KDialog in ui.tsx: z-50
- Multiple toasts, popovers, tooltips, mobile navs, etc., range from z-50 up to z-60
With the Dialog at 500/501, it will render beneath those higher overlays. Please confirm whether this stacking order is intentional; if not, bump the Dialog’s z-index above the highest existing overlay.
Consider centralizing all overlay layers into design tokens (e.g. z-dialog, z-toast, z-popover) to maintain consistent layering over time.
apps/web/actions/organization/update-domain.ts (1)
17-23: Broaden/align subscription gating and source of truthCurrent gating only allows "active". If your product treats "trialing" (or other states) as eligible, include them. Also confirm whether gating should be based on the organization’s subscription status rather than the owner’s user record.
Proposed change:
- //check user subscription to prevent abuse - const isSubscribed = user.stripeSubscriptionStatus === "active"; + // Check subscription to prevent abuse; adjust statuses per product policy + const allowedStatuses = new Set(["active", "trialing"]); + const isSubscribed = allowedStatuses.has(user.stripeSubscriptionStatus ?? "");Optional: If subscription is org-scoped, prefer reading org subscription status (or entitlement) server-side as the source of truth.
apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/SubscribeContent.tsx (1)
4-4: Make the sample domain customizable
capsDomainTextis hard-coded. Passing it as a prop (defaulting to this value) makes the component reusable and avoids future in-file edits.apps/web/app/(org)/dashboard/_components/Confetti.tsx (1)
50-53: Duplicateresizeoption
resize: trueis set both inglobalOptionsdefault and again inconfetti.create(...). One declaration is enough.apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/Stepper.tsx (1)
19-23: Add ARIA semantics for better accessibilityWrapping the stepper in
navwitharia-label="Progress"and addingaria-currenton the current step improves assistive-technology support.apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/CustomDomainDialog.tsx (3)
130-131: Make pollInterval type-safe across browser/Node and clear it reliably.
NodeJS.Timeoutcan be incompatible in the browser. PreferReturnType<typeof setInterval>and usenullsentinel.- const pollInterval = useRef<NodeJS.Timeout>(); + const pollInterval = useRef<ReturnType<typeof setInterval> | null>(null);- if (pollInterval.current) { - clearInterval(pollInterval.current); - pollInterval.current = undefined; - } + if (pollInterval.current !== null) { + clearInterval(pollInterval.current); + pollInterval.current = null; + }Also applies to: 278-281
254-258: Overly strict guard blocks manual verification after update.
checkOrganizationDomainneeds org id; requiringcustomDomaintoo can prevent verification immediately after update (before context refresh). Relax the guard.- if ( - !activeOrganization?.organization.id || - !activeOrganization?.organization.customDomain - ) - return; + if (!activeOrganization?.organization.id) return;
389-393: DRY: duplicate icon markup for spinner vs. idle.Toggle the
animate-spinclass instead of duplicating the node.- {verifying ? ( - <FontAwesomeIcon className="mr-1 opacity-70 animate-spin size-3.5" icon={faRefresh} /> - ) : ( - <FontAwesomeIcon className="mr-1 opacity-70 size-3.5" icon={faRefresh} /> - )} + <FontAwesomeIcon + className={`mr-1 opacity-70 size-3.5 ${verifying ? "animate-spin" : ""}`} + icon={faRefresh} + />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
apps/web/actions/organization/update-domain.ts(1 hunks)apps/web/app/(org)/dashboard/_components/Confetti.tsx(1 hunks)apps/web/app/(org)/dashboard/_components/Notifications/Skeleton.tsx(1 hunks)apps/web/app/(org)/dashboard/settings/organization/components/CapSettingsCard.tsx(1 hunks)apps/web/app/(org)/dashboard/settings/organization/components/CustomDomain.tsx(2 hunks)apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/CustomDomainDialog.tsx(1 hunks)apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/DomainStep.tsx(1 hunks)apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/Stepper.tsx(1 hunks)apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/SubscribeContent.tsx(1 hunks)apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/SuccessStep.tsx(1 hunks)apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/VerifyStep.tsx(1 hunks)apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/types.ts(1 hunks)apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainIconCard.tsx(0 hunks)apps/web/app/(org)/dashboard/settings/organization/components/OrganizationDetailsCard.tsx(1 hunks)apps/web/app/globals.css(0 hunks)apps/web/package.json(2 hunks)packages/ui/src/components/Dialog.tsx(3 hunks)packages/ui/src/components/Table.tsx(1 hunks)
💤 Files with no reviewable changes (2)
- apps/web/app/globals.css
- apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainIconCard.tsx
🧰 Additional context used
🧬 Code Graph Analysis (2)
apps/web/app/(org)/dashboard/settings/organization/components/OrganizationDetailsCard.tsx (1)
apps/web/app/(org)/dashboard/settings/organization/components/CustomDomain.tsx (1)
CustomDomain(14-119)
apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/DomainStep.tsx (1)
apps/web/app/(org)/dashboard/Contexts.tsx (1)
useDashboardContext(42-42)
🪛 Biome (2.1.2)
apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/CustomDomainDialog.tsx
[error] 65-65: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 66-66: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 84-84: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 303-303: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
⏰ 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). (3)
- GitHub Check: Clippy
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
🔇 Additional comments (7)
packages/ui/src/components/Table.tsx (1)
66-66: Style token update looks goodSwitching to border-gray-3 aligns with the other subtle border refinements in this PR.
apps/web/package.json (1)
60-60: canvas-confetti import is safely scoped to a client componentThe only use of
canvas-confettiis in
apps/web/app/(org)/dashboard/_components/Confetti.tsx
– which has a"use client"directive on line 1No server-side imports or unsafe SSR usage detected.
apps/web/app/(org)/dashboard/_components/Notifications/Skeleton.tsx (1)
63-63: Consistent border colorAdding border-gray-3 matches the table and settings card updates. Looks good.
apps/web/app/(org)/dashboard/settings/organization/components/CapSettingsCard.tsx (1)
51-51: Trailing comma nit acceptedTrailing comma reduces future diff noise and matches common formatting rules.
packages/ui/src/components/Dialog.tsx (1)
76-76: Icon container min-size adjustment looks goodThe added min-w/min-h constraints prevent layout shift and ensure consistent avatar/icon framing.
apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/SubscribeContent.tsx (1)
7-8: Absolute positioning may require a relative parentThe root div is
absolute, but the component assumes an ancestor withrelativepositioning. If that isn’t guaranteed, the overlay could escape its container. Consider documenting this requirement or wrapping with arelativediv inside the component.apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/VerifyStep.tsx (1)
76-90: Missing dependencies in polling effect
useEffectdepends oncheckVerification, but it’s omitted from the deps array. If the callback identity changes, polling may reference a stale function. Add it to the list (or wrap it inuseCallback).
apps/web/app/(org)/dashboard/settings/organization/components/CustomDomain.tsx
Show resolved
Hide resolved
| <CheckCircle className="text-green-200 size-2.5" /> | ||
| <p className="text-[11px] italic font-medium text-white">www.aashhab.design | ||
| <span className="ml-1 not-italic text-white/60">verified</span></p> | ||
| </div> |
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.
Verified banner shows a hard-coded domain
"www.aashhab.design" is hard-coded instead of orgCustomDomain, so verified users will always see the wrong domain. Replace with the actual value:
-<p className="text-[11px] italic font-medium text-white">www.aashhab.design
+<p className="text-[11px] italic font-medium text-white">{orgCustomDomain}📝 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.
| <div className="flex gap-2 items-center px-3 py-0.5 bg-green-900 rounded-full w-fit"> | |
| <CheckCircle className="text-green-200 size-2.5" /> | |
| <p className="text-[11px] italic font-medium text-white">www.aashhab.design | |
| <span className="ml-1 not-italic text-white/60">verified</span></p> | |
| <div className="flex gap-2 items-center px-3 py-0.5 bg-green-900 rounded-full w-fit"> | |
| <CheckCircle className="text-green-200 size-2.5" /> | |
| <p className="text-[11px] italic font-medium text-white">{orgCustomDomain} | |
| <span className="ml-1 not-italic text-white/60">verified</span></p> |
🤖 Prompt for AI Agents
In
apps/web/app/(org)/dashboard/settings/organization/components/CustomDomain.tsx
around lines 69 to 72, the verified banner displays a hard-coded domain
"www.aashhab.design" instead of using the dynamic orgCustomDomain value. Replace
the hard-coded string with the orgCustomDomain variable to correctly show the
verified domain for each user.
| import { checkOrganizationDomain } from "@/actions/organization/check-domain"; | ||
|
|
||
| import { SubscribeContent } from "./SubscribeContent"; | ||
| import { SuccesStep } from "./SuccessStep"; |
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.
Typo in component name: SuccesStep → SuccessStep.
The import and usage likely mismatch the exported name in SuccessStep.tsx. Standardize spelling to prevent compile/runtime errors.
-import { SuccesStep } from "./SuccessStep";
+import { SuccessStep } from "./SuccessStep";- <SuccesStep />
+ <SuccessStep />Also applies to: 371-373
🤖 Prompt for AI Agents
In
apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/CustomDomainDialog.tsx
at line 25 and lines 371-373, correct the typo in the imported component name
from "SuccesStep" to "SuccessStep" to match the actual exported component name.
Update all instances of "SuccesStep" to "SuccessStep" to ensure consistent
spelling and prevent import or runtime errors.
...p/(org)/dashboard/settings/organization/components/CustomDomainDialog/CustomDomainDialog.tsx
Show resolved
Hide resolved
apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/DomainStep.tsx
Outdated
Show resolved
Hide resolved
.../web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/SuccessStep.tsx
Outdated
Show resolved
Hide resolved
apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/types.ts
Show resolved
Hide resolved
apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/types.ts
Show resolved
Hide resolved
apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/VerifyStep.tsx
Show resolved
Hide resolved
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
♻️ Duplicate comments (5)
apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/CustomDomainDialog.tsx (5)
25-26: Typo:SuccesStepshould beSuccessStep.The misspelling remains in both the import and JSX usage and will break the build.
Also applies to: 380-381
64-72: WrapGO_TO_STEPcase in a block to avoid leakingtargetIndex&canNavigate.Scoped-declaration lint error persists; same fix as previously suggested.
83-89: WrapCLEAR_ERRORcase in a block to confinenewErrors.Same variable-leak issue flagged earlier is still present.
154-156: Remove early return before later hook calls (rule-of-hooks violation).
useEffectis declared after this conditional return, making hook order conditional.
184-186: Compare cleaned domain, not raw input, before deciding to skip update.Perform equality check after
cleanDomain()to avoid false mismatches (www., protocol, etc.).Also applies to: 188-217
🧹 Nitpick comments (1)
apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/CustomDomainDialog.tsx (1)
10-11: Combine icon imports to a single statement.Minor tidy-up – import
faGlobeandfaRefreshtogether to reduce duplicate statements.-import { faGlobe } from "@fortawesome/free-solid-svg-icons"; -... -import { faRefresh } from "@fortawesome/free-solid-svg-icons"; +import { faGlobe, faRefresh } from "@fortawesome/free-solid-svg-icons";Also applies to: 20-21
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/CustomDomainDialog.tsx(1 hunks)apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/VerifyStep.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/VerifyStep.tsx
🧰 Additional context used
🪛 Biome (2.1.2)
apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/CustomDomainDialog.tsx
[error] 65-65: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 66-66: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 84-84: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 310-310: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
⏰ 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). (3)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Clippy
This PR: introduces a all new custom domain flow
Note: the settings page will receive some further design improvements.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores
Refactor
Style