fix: handle subdomains & scroll for custom domain dialog #889
Conversation
WalkthroughAdds tldts-based subdomain detection and an internal isSubdomain helper to VerifyStep, tightens A-record gating, makes the VerifyStep content scrollable, and adds the tldts dependency. Other edits are formatting/template fixes with no public API changes. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant VerifyStep
participant TLDTS
User->>VerifyStep: Provide domain / open verification
VerifyStep->>TLDTS: parse(normalize(domain))
TLDTS-->>VerifyStep: parsed result (host, subdomain info)
alt parsed indicates subdomain
VerifyStep-->>User: Render verification UI WITHOUT A-record section
else parsed indicates apex domain
VerifyStep-->>User: Render verification UI WITH A-record section (if A-values present and not already configured)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/VerifyStep.tsx (1)
134-134: Scrolling container: avoid h-full to reduce layout coupling; prefer responsive max height
h-fullcan produce unexpected layouts depending on the parent’s height. You already cap withmax-h-[300px]; consider relying on max height alone and making it responsive.Apply this diff:
-<div className="overflow-y-auto h-full max-h-[300px] space-y-4"> +<div className="overflow-y-auto max-h-[60vh] space-y-4">If you prefer a fixed cap, drop
h-fulland keepmax-h-[300px]:-<div className="overflow-y-auto h-full max-h-[300px] space-y-4"> +<div className="overflow-y-auto max-h-[300px] space-y-4">
📜 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 (1)
apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/VerifyStep.tsx(3 hunks)
⏰ 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: Analyze (rust)
apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/VerifyStep.tsx
Outdated
Show resolved
Hide resolved
apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/VerifyStep.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/VerifyStep.tsx (1)
381-385: CNAME “Name” is computed incorrectly for multi-label TLDs and multi-label subdomainsSplitting by dots and taking the first label mislabels apex domains on public suffixes (e.g., example.co.uk becomes “example” instead of “@”) and drops nested labels (foo.bar.example.com should be “foo.bar”, not just “foo”). Use tldts to derive the subdomain safely.
Apply this targeted diff:
-<code className="px-2 py-1 text-xs rounded bg-gray-4"> - {domain.split(".").length > 2 - ? domain.split(".")[0] - : "@"} -</code> +<code className="px-2 py-1 text-xs rounded bg-gray-4"> + {(() => { + const input = domain.trim().replace(/^https?:\/\//i, "").split("/")[0] ?? ""; + const host = (input.replace(/\.$/, "").split(":")[0] || "").toLowerCase(); + const { subdomain } = parse(host); + return subdomain || "@"; + })()} +</code>Optionally, for clarity and reuse elsewhere, compute the label outside render:
// Place near other derived values const normalizeHost = (raw: string) => (raw.trim().replace(/^https?:\/\//i, "").split("/")[0] ?? "") .replace(/\.$/, "") .split(":")[0] .toLowerCase(); const { subdomain: domainSubdomain } = parse(normalizeHost(domain));Then in JSX:
<code className="px-2 py-1 text-xs rounded bg-gray-4"> {domainSubdomain || "@"} </code>
♻️ Duplicate comments (1)
apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/VerifyStep.tsx (1)
86-87: A-record visibility gating is now correct and subdomain-awareSwitching to
recommendedAValues.length > 0and excluding subdomains fixes the earlier gating bug and aligns with expected behavior.
🧹 Nitpick comments (4)
apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/VerifyStep.tsx (4)
58-76: Robust subdomain detection; minor nit on try/catchThe normalization and tldts-based parsing are solid. Note that
parse()doesn’t throw; thecatchis unlikely to run. If you want to keep a fallback, consider checkingparse(host).isIcann || parse(host).isPrivateand only falling back when parsing yields an invalid domain.Apply this minimal diff if you prefer to simplify:
- try { - // Prefer PSL-backed parsing for correctness (e.g., co.uk, com.au) - const { subdomain } = parse(host); - return Boolean(subdomain); - } catch { - // Fallback: conservative heuristic - const parts = host.split("."); - return parts.length > 2; - } + // Prefer PSL-backed parsing for correctness (e.g., co.uk, com.au) + const { subdomain } = parse(host); + return Boolean(subdomain);
147-147: Scrollable container: good UX, consider a responsive max-height
overflow-y-autowith a fixedmax-h-[300px]works, but on tall viewports it can feel cramped. Consider a viewport-relative cap (e.g., 50–60vh) and droph-fullto avoid conflicting constraints.Suggested tweak:
-<div className="overflow-y-auto h-full max-h-[300px] space-y-4"> +<div className="overflow-y-auto max-h-[60vh] space-y-4">
43-46: Avoid mutating arrays from props/state when sorting
recommendedIPv4.sort(...)mutates in place. Create a shallow copy before sorting to prevent side effects.Apply this diff:
- const sortedIPv4 = recommendedIPv4.sort((a, b) => a.rank - b.rank); + const sortedIPv4 = [...recommendedIPv4].sort((a, b) => a.rank - b.rank);
389-395: Don’t sortrecommendedCnamesin place during renderSorting in place can cause subtle bugs across renders. Sort a copy before mapping.
Apply this diff:
- {recommendedCnames - .sort((a, b) => a.rank - b.rank) - .map((cname, index) => { + {[...recommendedCnames] + .sort((a, b) => a.rank - b.rank) + .map((cname, index) => {
📜 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 ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/VerifyStep.tsx(4 hunks)apps/web/package.json(1 hunks)
⏰ 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: Analyze (rust)
🔇 Additional comments (2)
apps/web/package.json (1)
116-116: Add tldts dependency: LGTMBrings in a robust PSL-backed parser for domain handling. No concerns on this change.
apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/VerifyStep.tsx (1)
8-8: Using tldts.parse is the right callThis addresses public-suffix edge cases (e.g., co.uk, com.au) and is a solid foundation for subdomain logic.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
apps/web/app/api/video/delete/route.ts (1)
13-21: Declare all returned HttpApiError types (Unauthorized/InternalServerError) on the endpointThe handler maps domain errors to HttpApiError.Unauthorized and HttpApiError.InternalServerError (Lines 31-45), but the endpoint only declares Forbidden and NotFound. This mismatch can cause type/runtime inconsistencies (e.g., validation rejecting undeclared errors). Either declare these errors or map to an already-declared type.
Apply this diff to declare the missing errors:
class Api extends HttpApi.make("Api").add( HttpApiGroup.make("root").add( HttpApiEndpoint.del("deleteVideo")`/api/video/delete` .setUrlParams(Schema.Struct({ videoId: Video.VideoId })) .middleware(HttpAuthMiddleware) .addError(HttpApiError.Forbidden) - .addError(HttpApiError.NotFound), + .addError(HttpApiError.NotFound) + .addError(HttpApiError.Unauthorized) + .addError(HttpApiError.InternalServerError), ), ) {}
📜 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 (5)
apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/VerifyStep.tsx(4 hunks)apps/web/app/api/playlist/route.ts(2 hunks)apps/web/app/api/video/delete/route.ts(1 hunks)packages/web-backend/src/S3Buckets/index.ts(2 hunks)packages/web-backend/src/Videos/index.ts(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/web-backend/src/Videos/index.ts
- packages/web-backend/src/S3Buckets/index.ts
🚧 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
🧬 Code Graph Analysis (1)
apps/web/app/api/playlist/route.ts (1)
packages/env/server.ts (1)
serverEnv(80-84)
🔇 Additional comments (2)
apps/web/app/api/video/delete/route.ts (1)
21-21: LGTM on class closing brace formattingPurely stylistic change; consistent with repo formatting elsewhere.
apps/web/app/api/playlist/route.ts (1)
44-44: LGTM on class closing brace formattingCosmetic only; no behavioral impact.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/VerifyStep.tsx (2)
1-6: Add "use client" directive (component uses hooks but file isn't marked as client).This component uses useState/useEffect, so it must be a client component. Without the directive, Next.js will error at build/runtime.
Apply this diff at the very top of the file:
+"use client";
381-385: Derive CNAME “Name” using tldts to support nested subdomains (e.g., a.b.example.co.uk).Splitting by the first label yields only "a" for a.b.example…, which is misleading; many DNS providers expect the full subdomain part ("a.b").
Replace the naive split with PSL-backed parsing:
- <code className="px-2 py-1 text-xs rounded bg-gray-4"> - {domain.split(".").length > 2 - ? domain.split(".")[0] - : "@"} - </code> + <code className="px-2 py-1 text-xs rounded bg-gray-4"> + {parse(domain).subdomain || "@"} + </code>
🧹 Nitpick comments (4)
apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/VerifyStep.tsx (4)
58-76: Robust subdomain detection with PSL-backed parsing — looks solid.Host normalization + tldts.parse(host) covers multi-label TLDs (co.uk, com.au), ports, and trailing dots. The conservative fallback is acceptable if parse ever fails.
If you want to shave a few bytes and avoid re-creating the function each render, you can hoist isSubdomain to module scope since it has no component dependencies.
43-45: Avoid mutating the input array when sorting IPv4 recommendations.recommendedIPv4 comes from props; sorting it in-place can cause subtle side-effects across renders.
Use a non-mutating sort:
- const sortedIPv4 = recommendedIPv4.sort((a, b) => a.rank - b.rank); + const sortedIPv4 = [...recommendedIPv4].sort((a, b) => a.rank - b.rank);
389-396: Non-mutating sort for CNAME recommendations..sort mutates recommendedCnames (from props). Prefer copying before sorting.
Apply:
-{recommendedCnames - .sort((a, b) => a.rank - b.rank) +{[...recommendedCnames] + .sort((a, b) => a.rank - b.rank) .map((cname, index) => { const fieldId = `cname-${cname.rank}`; return ( <div key={cname.rank} className="grid grid-cols-[100px,1fr] items-center" >
102-116: Stabilize polling interval typing and cleanup; include checkVerification in deps.
- NodeJS.Timeout can be incorrect in the browser; use ReturnType.
- Avoid clearing an uninitialized interval.
- Add checkVerification to deps to prevent stale closure.
useEffect(() => { - let interval: NodeJS.Timeout; + let interval: ReturnType<typeof setInterval> | null = null; if (activeOrganization?.organization.customDomain && !isVerified) { checkVerification(false); - interval = setInterval(() => { + interval = setInterval(() => { checkVerification(false); }, POLL_INTERVAL); } return () => { - clearInterval(interval); + if (interval) clearInterval(interval); }; - }, [activeOrganization?.organization.customDomain, isVerified]); + }, [activeOrganization?.organization.customDomain, isVerified, checkVerification]);
📜 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 (1)
apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/VerifyStep.tsx(4 hunks)
⏰ 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: Analyze (rust)
🔇 Additional comments (3)
apps/web/app/(org)/dashboard/settings/organization/components/CustomDomainDialog/VerifyStep.tsx (3)
86-88: Correct A-record gating condition.Using recommendedAValues instead of a single requiredAValue covers IPv4 fallbacks and fixes the previously hidden section issue.
6-6: ✅ Dependency Confirmed & Approval
Good call using tldts for PSL-backed parsing. Static import keeps the bundle simple and enables reliable subdomain detection.
• Confirmed"tldts": "^7.0.11"is declared in apps/web/package.json.Approved!
147-147: No changes needed—custom-scrollalready enables vertical scrolling
The.custom-scrollclass inapps/web/app/globals.css(lines 563–565) includesoverflow-y: auto, so the container will scroll as intended.
Summary by CodeRabbit
Bug Fixes
Style
Chores