SER-287 Fix domain flows on console #2364
Conversation
ConsoleProject ID: Sites (2)
Note Appwrite has a Discord community with over 16 000 members. |
WalkthroughAdds a short relative-time helper Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 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 |
vermakhushboo
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/table.svelte (1)
41-41: Fix type: selectedProxyRule should allow null.Prevents a TS error with the initial null assignment.
-let selectedProxyRule: Models.ProxyRule = $state(null); +let selectedProxyRule: Models.ProxyRule | null = $state(null);
🧹 Nitpick comments (4)
src/lib/helpers/date.ts (1)
209-227: Make unit-shortening locale-agnostic and handle future times; dedupe validation.Current string replaces miss “in x minutes/seconds/hours” and depend on English phrasing; also duplicates validation from timeFromNow. Replace with regex-based unit swaps and reuse timeFromNow.
-export function timeFromNowShort(datetime: string): string { - if (!datetime) { - return 'unknown time'; - } - if (!isValidDate(datetime)) { - return 'invalid date'; - } - - const timeStr = dayjs().to(dayjs(datetime)); - return timeStr - .replace(' seconds ago', ' secs ago') - .replace(' second ago', ' sec ago') - .replace(' minutes ago', ' mins ago') - .replace(' minute ago', ' min ago') - .replace(' hours ago', ' hrs ago') - .replace(' hour ago', ' hr ago') - .replace(' days ago', ' days ago') - .replace(' day ago', ' day ago'); -} +export function timeFromNowShort(datetime: string): string { + const base = timeFromNow(datetime); + if (base === 'unknown time' || base === 'invalid date') return base; + return base + .replace(/\bseconds\b/g, 'secs') + .replace(/\bsecond\b/g, 'sec') + .replace(/\bminutes\b/g, 'mins') + .replace(/\bminute\b/g, 'min') + .replace(/\bhours\b/g, 'hrs') + .replace(/\bhour\b/g, 'hr'); +}Note: if the app ever localizes dayjs relativeTime, these swaps should be disabled or made locale-aware.
src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/table.svelte (3)
93-108: Gate “View logs” by rule.logs to avoid empty modal.Row UI always shows “View logs”, while the action menu hides it when logs are absent. Make them consistent.
- <Link - size="s" - on:click={(e) => { - e.preventDefault(); - selectedProxyRule = rule; - showLogs = true; - }}> - View logs - </Link> + {#if rule.logs} + <Link + size="s" + on:click={(e) => { + e.preventDefault(); + selectedProxyRule = rule; + showLogs = true; + }}> + View logs + </Link> + {/if}
110-125: Apply the same logs gating for the unverified branch.Mirror the condition used in the action menu.
- <Link - size="s" - on:click={(e) => { - e.preventDefault(); - selectedProxyRule = rule; - showLogs = true; - }}> - View logs - </Link> + {#if rule.logs} + <Link + size="s" + on:click={(e) => { + e.preventDefault(); + selectedProxyRule = rule; + showLogs = true; + }}> + View logs + </Link> + {/if}
130-145: Avoid inline styles; use a class for the updated-time text.Keeps styling consistent and easier to theme.
- <Typography.Text - variant="m-400" - color="--fgcolor-neutral-tertiary" - style="font-size: 0.875rem;"> + <Typography.Text + variant="m-400" + color="--fgcolor-neutral-tertiary" + class="updated-cell">Add to the file’s <style>:
.updated-cell { font-size: 0.875rem; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/lib/helpers/date.ts(1 hunks)src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/store.ts(1 hunks)src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/table.svelte(2 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). (2)
- GitHub Check: build
- GitHub Check: e2e
🔇 Additional comments (3)
src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/store.ts (1)
20-25: Add a visible, accessible header: "Updated".Blank headers harm accessibility and sorting; set the title and confirm whether this column should be sortable. Apply:
- title: '', + title: 'Updated',Also confirm whether this column should be sortable; if not, set the appropriate flag on Column to prevent confusing sort UI (depends on your Column type).
src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/table.svelte (2)
28-28: LGTM: import of timeFromNowShort.
76-92: Confirm status mapping: created → “Verification failed”.“created” typically reads as an initial state; showing “Verification failed” might be misleading. Verify backend status semantics before shipping the copy.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/add-domain/+page.svelte (1)
33-35: Type mismatch: initializing string state withnull.
let redirect: string = $state(null);(andbranch) violates the declared type.-let redirect: string = $state(null); -let branch: string = $state(null); +let redirect = $state(''); // or: let redirect: string | null = $state(null); +let branch = $state(''); // or: let branch: string | null = $state(null);src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/table.svelte (1)
41-41: Type: allow null in reactive state.
$state(null)conflicts withModels.ProxyRuletype. Use a union to avoid TS errors under strict mode.-let selectedProxyRule: Models.ProxyRule = $state(null); +let selectedProxyRule: Models.ProxyRule | null = $state(null);
🧹 Nitpick comments (17)
src/routes/(console)/project-[region]-[project]/settings/domains/add-domain/+page.svelte (2)
55-58: Use locale‑independent lowercasing for domains.
toLocaleLowerCase()can miscase in some locales (e.g., Turkish). UsetoLowerCase()for DNS labels.- .proxy.createAPIRule({ domain: domainName.toLocaleLowerCase() }); + .proxy.createAPIRule({ domain: domainName.toLowerCase() });
62-70: URL‑encode dynamic path/query parts in redirect.Safer when domains include wildcards or unusual chars; also encode
rule.$id.- let redirect = `${routeBase}/add-domain/verify-${domainName}?rule=${rule.$id}`; + const encodedDomain = encodeURIComponent(domainName); + let redirect = `${routeBase}/add-domain/verify-${encodedDomain}?rule=${encodeURIComponent(rule.$id)}`;src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/add-domain/+page.svelte (2)
104-111: Encode redirect URL and avoid shadowingredirectstate.The local
redirectstring shadows the boundredirectfield and the domain/rule aren’t encoded. Rename and encode.- let redirect = `${routeBase}/add-domain/verify-${domainName}?rule=${rule.$id}`; + const encodedDomain = encodeURIComponent(domainName); + let verificationUrl = `${routeBase}/add-domain/verify-${encodedDomain}?rule=${encodeURIComponent(rule.$id)}`; - if (isCloud && domain?.$id) { - redirect += `&domain=${domain.$id}`; - } + if (isCloud && domain?.$id) { + verificationUrl += `&domain=${domain.$id}`; + } - await goto(redirect); + await goto(verificationUrl);
78-99: Normalize domain before rule creation.For consistency and to avoid case‑sensitive mismatches, lower‑case the domain in all create calls.
- domain: domainName, + domain: domainName.toLowerCase(), @@ - domain: domainName, + domain: domainName.toLowerCase(), @@ - domain: domainName, + domain: domainName.toLowerCase(),src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/store.ts (1)
20-25: Add an accessible header for the Updated column.Empty titles can hurt a11y. Prefer a visible or visually‑hidden label.
- title: '', + title: 'Updated',src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/table.svelte (3)
93-107: Gate “View logs” links on log availability (matches action menu logic).Action menu checks
proxyRule.logs, but inline links don’t. Avoid opening an empty modal.- {:else if proxyRule.status === 'verifying'} + {:else if proxyRule.status === 'verifying' && proxyRule.logs} @@ - {:else if proxyRule.status === 'unverified'} + {:else if proxyRule.status === 'unverified' && proxyRule.logs}Also applies to: 109-124
83-91: A11y: interactive “Link” without href.Provide
href="#"(with preventDefault) or switch to a button‑styled component to maintain semantics.- <Link + <Link href="#" size="s" on:click={(e) => { e.preventDefault(); selectedProxyRule = proxyRule; showRetry = true; }}> @@ - <Link + <Link href="#" size="s" on:click={(e) => { e.preventDefault(); selectedProxyRule = proxyRule; showLogs = true; }}> @@ - <Link + <Link href="#" size="s" on:click={(e) => { e.preventDefault(); selectedProxyRule = proxyRule; showLogs = true; }}>Also applies to: 100-107, 116-124
27-29: Prefer importing DnsRecordsAction from the barrel for consistency.Minor consistency tweak with
ViewLogsModalimport.-import DnsRecordsAction from '$lib/components/domains/dnsRecordsAction.svelte'; +import { DnsRecordsAction } from '$lib/components';src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/table.svelte (3)
85-91: Instrument “Retry” and “View logs” clicks for analytics.Deletion is tracked; these new actions aren’t. Add tracking for observability.
Example:
selectedProxyRule = rule; showRetry = true; +trackEvent(Click.DomainRetryClick, { source: 'sites_domain_overview' });selectedProxyRule = rule; showLogs = true; +trackEvent(Click.DomainViewLogsClick, { source: 'sites_domain_overview' });Also applies to: 101-107, 117-124
153-165: Icon‑only button needs an accessible name.Add
aria-label(and optionallytitle) to the kebab menu trigger for screen readers.-<Button +<Button text icon + aria-label="More actions" + title="More actions" on:click={(e) => {
130-146: Guard rule.$updatedAt (handle nullish dates); 'updated' column confirmed
- Confirmed: columns store includes { id: 'updated' } — src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/store.ts.
- Action: avoid "Invalid date" by rendering only when rule.$updatedAt is present. Suggested tweak:
-{#if rule.status !== 'verified'} +{#if rule.status !== 'verified' && rule.$updatedAt} ... - {#if rule.status === 'created'} + {#if rule.status === 'created'} Checked {timeFromNowShort(rule.$updatedAt)}src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/table.svelte — apply change around the existing updated column render.
src/routes/(console)/project-[region]-[project]/settings/domains/table.svelte (6)
48-55: Columns: width and updated column—OK; add sortability if desired.If users sort by "updated", consider wiring sort metadata.
134-149: Updated column: add nullish guard.Prevent rendering when
domain.$updatedAtis falsy to avoid bad output.-{#if domain.status !== 'verified'} +{#if domain.status !== 'verified' && domain.$updatedAt}
169-178: Action menu logs gating: good; also add analytics.Track "View logs" selections for parity with delete.
selectedDomain = domain; showLogs = true; toggle(e); +trackEvent(Click.DomainViewLogsClick, { source: 'settings_domain_overview' });
180-190: Retry action: add analytics.selectedDomain = domain; showRetry = true; toggle(e); +trackEvent(Click.DomainRetryClick, { source: 'settings_domain_overview' });
231-235: CSS nit: align divider styles across pages.Sites table also adds vertical padding; consider keeping styles consistent.
.action-menu-divider { - margin-inline: -1rem; + margin-inline: -1rem; + padding-block-start: 0.25rem; + padding-block-end: 0.25rem; }
156-165: Add accessible name to kebab button.Icon‑only button needs an
aria-label(andtitle) for SR users.-<Button +<Button text icon + aria-label="More actions" + title="More actions" on:click={(e) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/lib/components/domains/index.ts(1 hunks)src/lib/components/index.ts(1 hunks)src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/add-domain/+page.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/store.ts(1 hunks)src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/table.svelte(7 hunks)src/routes/(console)/project-[region]-[project]/settings/domains/add-domain/+page.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/settings/domains/table.svelte(8 hunks)src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/store.ts(1 hunks)src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/table.svelte(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/store.ts
⏰ 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). (2)
- GitHub Check: build
- GitHub Check: e2e
🔇 Additional comments (14)
src/routes/(console)/project-[region]-[project]/settings/domains/add-domain/+page.svelte (1)
64-70: Good guard on appending domain param only when present.The added
isCloud && domain?.$idcheck prevents bogus&domain=query strings. LGTM.src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/store.ts (1)
10-11: Wider domain column looks right.The increased min width improves readability for long hostnames.
src/lib/components/index.ts (1)
90-90: Barrel re‑export added.Makes domain components available via
$lib/components. Ensure no name collisions with existing exports.src/lib/components/domains/index.ts (1)
1-5: Barrel for domain components looks good.Straightforward re‑exports; helps consumers consolidate imports.
src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/table.svelte (3)
6-16: Imports: OK.Added
IconTerminal,Divider,ViewLogsModal, andtimeFromNowShortare appropriate for the new UI.
75-125: Confirm backend→UI status mapping.You’re rendering:
- created → “Verification failed” + Retry
- verifying → “Generating certificate” + View logs
- unverified → “Certificate generation failed” + View logs
Please confirm these labels align with server semantics so we don’t show “failed” on initial create.
129-146: Updated column rendering looks solid.Clear, concise relative times via
timeFromNowShort. Good right‑alignment for visual scanning.src/routes/(console)/project-[region]-[project]/settings/domains/table.svelte (5)
6-11: LGTM on icon imports.
39-39: State for logs modal: OK.No issues spotted.
27-27: timeFromNowShort import/use looks good. Returns 'unknown time' for falsy input and 'invalid date' when isValidDate(datetime) fails — no changes required.
24-24: Verified: ViewLogsModal is exported as a named export.Found in src/lib/components/domains/index.ts:
export { default as ViewLogsModal } from './viewLogsModal.svelte';
226-229: Prop name & type confirmed — no change required.src/lib/components/domains/viewLogsModal.svelte exports selectedProxyRule: Models.ProxyRule and uses selectedProxyRule.logs; passing selectedProxyRule={selectedDomain} is correct.
src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/table.svelte (2)
24-24: ViewLogsModal correctly re-exported.
Confirmed in src/lib/components/domains/index.ts:export { default as ViewLogsModal } from './viewLogsModal.svelte';
76-126: Align status copy, gate "View logs", and prefer Button for actionsFile: src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/table.svelte Lines: 76-126
- 'created' currently renders "Verification failed" — if
createdmeans "not verified yet"/pending, change the copy:- content="Verification failed" + content="Verification pending"
- Gate inline "View logs" the same way the ActionMenu does (only show when
rule.logs):- <Link + {#if rule.logs} + <Link size="s" on:click={(e) => { e.preventDefault(); selectedProxyRule = rule; showLogs = true; }}> - View logs - </Link> + View logs + </Link> + {/if}
- Actions look like buttons. If
Linkrenders an anchor, prefer a Button (text) for a11y or add appropriate role/aria attributes:-<Link size="s" on:click={...}>Retry</Link> +<Button text size="s" on:click={...} aria-label="Retry verification">Retry</Button>
src/routes/(console)/project-[region]-[project]/settings/domains/table.svelte
Show resolved
Hide resolved
| {#if domain.status === 'created'} | ||
| <Layout.Stack direction="row" gap="xs" alignItems="center"> | ||
| <Badge | ||
| variant="secondary" | ||
| type="error" | ||
| content="Verification failed" | ||
| size="xs" /> | ||
| <Link | ||
| size="s" | ||
| on:click={(e) => { | ||
| e.preventDefault(); | ||
| selectedDomain = domain; | ||
| showRetry = true; | ||
| }}> | ||
| Retry | ||
| </Link> | ||
| </Layout.Stack> | ||
| {:else if domain.status === 'verifying'} | ||
| <Layout.Stack direction="row" gap="xs" alignItems="center"> | ||
| <Badge | ||
| variant="secondary" | ||
| content="Generating certificate" | ||
| size="xs" /> | ||
| <Link | ||
| size="s" | ||
| on:click={(e) => { | ||
| e.preventDefault(); | ||
| selectedDomain = domain; | ||
| showLogs = true; | ||
| }}> | ||
| View logs | ||
| </Link> | ||
| </Layout.Stack> | ||
| {:else if domain.status === 'unverified'} | ||
| <Layout.Stack direction="row" gap="xs" alignItems="center"> | ||
| <Badge | ||
| variant="secondary" | ||
| type="error" | ||
| content="Certificate generation failed" | ||
| size="xs" /> | ||
| <Link | ||
| size="s" | ||
| on:click={(e) => { | ||
| e.preventDefault(); | ||
| selectedDomain = domain; | ||
| showLogs = true; | ||
| }}> | ||
| View logs | ||
| </Link> | ||
| </Layout.Stack> | ||
| {/if} | ||
| </Layout.Stack> |
There was a problem hiding this comment.
Status mapping, logs gating, and a11y: mirror fixes from sites table.
- "Verification failed" for
domain.status === 'created'is likely incorrect—please confirm and fix copy. - Gate inline "View logs" on
domain.logs(or make modal handle empty state). - Prefer Button (text) for action triggers, or add appropriate aria attributes to Link if it renders as anchor without href.
Apply analogous diffs as suggested in the sites table for these blocks.
| import RetryDomainModal from './retryDomainModal.svelte'; | ||
| import { ViewLogsModal } from '$lib/components'; | ||
| import { regionalProtocol } from '../../store'; | ||
| import DnsRecordsAction from '$lib/components/domains/dnsRecordsAction.svelte'; |
There was a problem hiding this comment.
doesn't components/domains have a barrel file now? index.ts which is also exported via parent index.ts?
src/routes/(console)/project-[region]-[project]/settings/domains/table.svelte
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/components/domains/recordTable.svelte (1)
76-85: Replace hard‑coded CA vendor with Let's Encrypt (or neutral copy)File: src/lib/components/domains/recordTable.svelte (lines 76–85) — replace the Alert copy so it references Let's Encrypt (letsencrypt.org) for Appwrite‑managed TLS, or use vendor‑neutral text ("the certificate authority used by Appwrite"). Keep the existing Link to the custom‑domains docs.
Search for other occurrences before committing:
rg -n -C2 -i 'certainly\.com|caa|letsencrypt|sectigo|digicert' src || truesrc/routes/(console)/project-[region]-[project]/settings/domains/add-domain/verify-[domain]/+page.svelte (1)
128-131:isSubmittingis not bound; submit button won’t reflect submission state.Use two‑way binding like other pages to prevent double submits and align UX.
- <Form onSubmit={verify} {isSubmitting}> + <Form onSubmit={verify} bind:isSubmitting>
🧹 Nitpick comments (38)
src/routes/(console)/project-[region]-[project]/settings/domains/add-domain/verify-[domain]/+page.ts (3)
10-13: Return a 400 using SvelteKit’s error helper instead of throwing a generic ErrorImproves status codes and error handling paths.
Apply:
- if (!ruleId) { - throw new Error('Rule ID is required'); - } + if (!ruleId) { + throw error(400, 'Rule ID is required'); + }Add import (outside this hunk):
import { error } from '@sveltejs/kit';
6-6: Type the loader with PageLoad for stricter inference and safer data typingLocks the return shape into route $types and catches regressions.
Apply:
-export const load = async ({ params, parent, depends, url }) => { +export const load: PageLoad = async ({ params, parent, depends, url }) => {Add import (outside this hunk):
import type { PageLoad } from './$types';
16-22: Optional: fetch in parallel to shave latency
domains.listandproxy.getRuleare independent; run concurrently.One way:
const proxyRulePromise = sdk.forProject(params.region, params.project).proxy.getRule({ ruleId }); const domainsListPromise: Promise<Models.DomainsList | undefined> = isCloud ? sdk.forConsole.domains.list({ queries: [Query.equal('teamId', organization.$id)] }) : Promise.resolve(undefined); const [proxyRule, domainsList] = await Promise.all([proxyRulePromise, domainsListPromise]);src/lib/components/domains/cnameTable.svelte (3)
15-16: NarrowruleStatustype and include explicit'verified'branch.Use a string‑literal union to prevent typos and handle the
'verified'state directly (today it depends onverified === trueonly).Apply:
-export let verified: boolean | undefined = undefined; -export let ruleStatus: string | undefined = undefined; +type RuleStatus = 'created' | 'verifying' | 'verified' | 'unverified'; +export let verified: boolean | undefined = undefined; +export let ruleStatus: RuleStatus | undefined = undefined;
27-39: Status-to-badge mapping likely inverted; add a pending state and treat'unverified'as verification failure.Current copy shows “Verification failed” for
'created'. Suggest:
'verified'→ “Verified”'verifying'→ “Generating certificate”'unverified'→ “Verification failed”'created'→ “Awaiting verification” (or similar)Apply:
-{#if ruleStatus === 'created'} - <Badge variant="secondary" type="error" size="xs" content="Verification failed" /> -{:else if ruleStatus === 'verifying'} - <Badge variant="secondary" size="xs" content="Generating certificate" /> -{:else if ruleStatus === 'unverified'} - <Badge - variant="secondary" - type="error" - size="xs" - content="Certificate generation failed" /> -{:else if verified === true} +{#if ruleStatus === 'verified' || verified === true} <Badge variant="secondary" type="success" size="xs" content="Verified" /> +{:else if ruleStatus === 'verifying'} + <Badge variant="secondary" size="xs" content="Generating certificate" /> +{:else if ruleStatus === 'unverified'} + <Badge variant="secondary" type="error" size="xs" content="Verification failed" /> +{:else if ruleStatus === 'created'} + <Badge variant="secondary" size="xs" content="Awaiting verification" /> {/if}
55-56: Show “@” for apex domains.
subdomainis empty for apex records; render “@” for clarity (as done in recordTable).Apply:
-<Table.Cell {root}>{subdomain}</Table.Cell> +<Table.Cell {root}>{subdomain || '@'}</Table.Cell>src/lib/components/domains/nameserverTable.svelte (2)
7-8: ConstrainruleStatusto known values.Prevents accidental strings and aligns with other components.
Apply:
-export let verified: boolean | undefined = undefined; -export let ruleStatus: string | undefined = undefined; +type RuleStatus = 'created' | 'verifying' | 'verified' | 'unverified'; +export let verified: boolean | undefined = undefined; +export let ruleStatus: RuleStatus | undefined = undefined;
20-31: Align badge copy with lifecycle and include'verified'.Same mapping suggestion as cnameTable.
Apply:
-{#if ruleStatus === 'created'} - <Badge variant="secondary" type="error" size="xs" content="Verification failed" /> -{:else if ruleStatus === 'verifying'} - <Badge variant="secondary" size="xs" content="Generating certificate" /> -{:else if ruleStatus === 'unverified'} - <Badge - variant="secondary" - type="error" - size="xs" - content="Certificate generation failed" /> -{:else if verified === true} +{#if ruleStatus === 'verified' || verified === true} <Badge variant="secondary" type="success" size="xs" content="Verified" /> +{:else if ruleStatus === 'verifying'} + <Badge variant="secondary" size="xs" content="Generating certificate" /> +{:else if ruleStatus === 'unverified'} + <Badge variant="secondary" type="error" size="xs" content="Verification failed" /> +{:else if ruleStatus === 'created'} + <Badge variant="secondary" size="xs" content="Awaiting verification" /> {/if}src/lib/components/domains/recordTable.svelte (2)
14-18: Type safety forruleStatus.Use a union and keep
variantconsistent.Apply:
-export let verified: boolean | undefined = undefined; +export let verified: boolean | undefined = undefined; export let variant: 'cname' | 'a' | 'aaaa'; export let service: 'sites' | 'general' = 'general'; -export let ruleStatus: string | undefined = undefined; +type RuleStatus = 'created' | 'verifying' | 'verified' | 'unverified'; +export let ruleStatus: RuleStatus | undefined = undefined;
41-53: Fix status mapping and include'verified'.Mirror the same lifecycle mapping used across components.
Apply:
-{#if ruleStatus === 'created'} - <Badge variant="secondary" type="error" size="xs" content="Verification failed" /> -{:else if ruleStatus === 'verifying'} - <Badge variant="secondary" size="xs" content="Generating certificate" /> -{:else if ruleStatus === 'unverified'} - <Badge - variant="secondary" - type="error" - size="xs" - content="Certificate generation failed" /> -{:else if verified === true} +{#if ruleStatus === 'verified' || verified === true} <Badge variant="secondary" type="success" size="xs" content="Verified" /> +{:else if ruleStatus === 'verifying'} + <Badge variant="secondary" size="xs" content="Generating certificate" /> +{:else if ruleStatus === 'unverified'} + <Badge variant="secondary" type="error" size="xs" content="Verification failed" /> +{:else if ruleStatus === 'created'} + <Badge variant="secondary" size="xs" content="Awaiting verification" /> {/if}src/routes/(console)/project-[region]-[project]/settings/domains/add-domain/+page.svelte (1)
45-45: PrefertoLowerCase()for domain normalization.
toLocaleLowerCase()can be locale‑sensitive; domains should be ASCII‑lowercased.Apply:
- .proxy.createAPIRule({ domain: domainName.toLocaleLowerCase() }); + .proxy.createAPIRule({ domain: domainName.toLowerCase() });src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/retryDomainModal.svelte (2)
30-41: Avoid resetting the user’s selected tab on reactive changes.The
$effectwill overwrite manual tab picks if dependencies change. Consider setting default only on open.Apply:
-$effect(() => { - if ($regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME && isSubDomain) { + $effect(() => { + if (!show) return; // only when opening + if ($regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME && isSubDomain) { selectedTab = 'cname'; } else if (!isCloud && $regionalConsoleVariables._APP_DOMAIN_TARGET_A) { selectedTab = 'a'; } else if (!isCloud && $regionalConsoleVariables._APP_DOMAIN_TARGET_AAAA) { selectedTab = 'aaaa'; } else { selectedTab = 'nameserver'; } });
113-125: Consistent “verified” rendering across components.Once components accept
'verified'inruleStatus, we can rely onselectedProxyRule.statusand drop the separateverifiedprop.After updating the components, simplify to:
<NameserverTable domain={selectedProxyRule.domain} ruleStatus={selectedProxyRule.status} /> <!-- and --> <RecordTable service="general" variant={selectedTab} domain={selectedProxyRule.domain} ruleStatus={selectedProxyRule.status} />src/routes/(console)/project-[region]-[project]/settings/domains/retryDomainModal.svelte (3)
47-54: Success toast can be misleading when status !== 'verified'.Toast always says “has been verified” even if the rule remains verifying/failed.
- show = false; - verified = domain.status === 'verified'; + show = false; + verified = domain.status === 'verified'; await invalidate(Dependencies.DOMAINS); addNotification({ type: 'success', - message: `${selectedDomain.domain} has been verified` + message: + domain.status === 'verified' + ? `${selectedDomain.domain} verified` + : `Verification retried for ${selectedDomain.domain}. We’ll keep checking.` });
61-64: Type‑narrow the caught error.
eisunknownin TS; accessinge.messagecan error.- } catch (e) { - error = - e.message ?? - 'Domain verification failed. Please check your domain settings or try again later'; - trackError(e, Submit.DomainUpdateVerification); + } catch (e) { + const message = + e instanceof Error && e.message + ? e.message + : 'Domain verification failed. Please check your domain settings or try again later'; + error = message; + trackError(e, Submit.DomainUpdateVerification); }
76-112: De‑duplicate tab logic by deriving an allowed‑tabs list.Keeps selection/render rules in one place and avoids future drift.
Example:
const availableTabs = $derived(() => { const tabs: Array<'cname'|'nameserver'|'a'|'aaaa'> = []; const hasCname = isSubDomain && !!$regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME && $regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME !== 'localhost'; const hasA = !isCloud && !!$regionalConsoleVariables._APP_DOMAIN_TARGET_A && $regionalConsoleVariables._APP_DOMAIN_TARGET_A !== '127.0.0.1'; const hasAaaa = !isCloud && !!$regionalConsoleVariables._APP_DOMAIN_TARGET_AAAA && $regionalConsoleVariables._APP_DOMAIN_TARGET_AAAA !== '::1'; if (hasCname) tabs.push('cname'); if (isCloud) tabs.push('nameserver'); else tabs.push('nameserver'); // always include if (hasA) tabs.push('a'); if (hasAaaa) tabs.push('aaaa'); return tabs; }); $effect(() => { selectedTab = availableTabs[0] ?? 'nameserver'; });src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/add-domain/verify-[domain]/+page.ts (3)
10-13: Use SvelteKit’s error helper for bad input.Return a 400 instead of throwing a generic Error.
+import { error } from '@sveltejs/kit'; ... - if (!ruleId) { - throw new Error('Rule ID is required'); - } + if (!ruleId) { + error(400, 'Rule ID is required'); + }
15-21: domainsList may be undefined on self‑hosted.Type and initialize accordingly to avoid implicit undefined.
-let domainsList: Models.DomainsList; +let domainsList: Models.DomainsList | undefined;
17-19: Optional: region‑aware console SDK.If console endpoints are region‑scoped, prefer
sdk.forConsoleIn(params.region).src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/add-domain/verify-[domain]/+page.ts (3)
10-13: Use SvelteKit’s error helper for bad input.Return a 400 instead of throwing.
+import { error } from '@sveltejs/kit'; ... - if (!ruleId) { - throw new Error('Rule ID is required'); - } + if (!ruleId) { + error(400, 'Rule ID is required'); + }
15-21: domainsList can be undefined outside cloud.Adjust type to avoid uninitialized variable.
-let domainsList: Models.DomainsList; +let domainsList: Models.DomainsList | undefined;
17-19: Optional: region‑aware console SDK.Prefer
sdk.forConsoleIn(params.region)if applicable.src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/add-domain/+page.svelte (2)
99-101: Optional: perform nameserver update before navigation.Running it pre-
gotoreduces the chance of cancellation on route change.
103-107: Type‑narrow error in notification.
errorisunknown; guard before accessing.message.- } catch (error) { - addNotification({ - type: 'error', - message: error.message - }); + } catch (error) { + addNotification({ + type: 'error', + message: error instanceof Error ? error.message : 'Failed to add domain' + }); }src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/add-domain/+page.svelte (2)
42-44: TS nullability mismatch forredirectandbranch.Declared as
stringbut initialized withnull. Make the types explicit to satisfy strict TS and avoid runtime surprises in bound inputs.- let redirect: string = $state(null); - let branch: string = $state(null); + let redirect = $state<string | null>(null); + let branch = $state<string | null>(null);
58-71: Follow-up: domain ID discovery for retries (optional).If create returns domain_already_exists, consider resolving the existing domain ID (e.g., list + filter) so the later nameserver update can still run. This will improve success rate after “verified” short-circuit.
Do you want me to wire a fallback lookup via
sdk.forConsole.domains.list({ search: apexDomain, teamId: $project.teamId })when create says “already exists”?src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/add-domain/verify-[domain]/+page.svelte (3)
69-69: Emptycatchblocks hide actionable failures and break CI.Replace silent catches with targeted handling for
domain_already_existsand user-facing notifications for unexpected errors.- } catch (error) {} + } catch (error: any) { + if (error?.type !== 'domain_already_exists') { + addNotification({ type: 'warning', message: error?.message ?? 'Failed to create apex domain' }); + } + } ... - } catch (error) {} + } catch (error: any) { + addNotification({ type: 'warning', message: error?.message ?? 'Failed to update nameservers' }); + }Also applies to: 86-86
72-75: Possible undefineddata.domainsList.
data.domainsListmay be absent outside cloud or on load failures. Use optional chaining to avoid crashes.- const domain = data.domainsList.domains.find( + const domain = data.domainsList?.domains?.find( (d: Models.Domain) => d.domain === apexDomain );
90-95: Guard missingruleId.
ruleIdcan be null if the page is hit directly. Add a guard to prevent a 400 and guide the user.- const ruleData = await sdk + if (!ruleId) { + throw new Error('Missing rule ID. Please restart domain verification.'); + } + const ruleData = await sdk .forProject(page.params.region, page.params.project) .proxy.updateRuleVerification({ ruleId });src/routes/(console)/project-[region]-[project]/settings/domains/add-domain/verify-[domain]/+page.svelte (5)
50-51: AlignisSubmittingstore shape with other pages.Use the same
$state(writable(false))pattern for consistency with bindings.- const isSubmitting = writable(false); + let isSubmitting = $state(writable(false));
56-56: Remove stray console.log.Leftover debug log will leak to users’ consoles.
- console.log('apexDomain', apexDomain); + // no-op
69-69: Emptycatchblocks.Same issue as other verify pages; replace with minimal handling to satisfy CI and improve debuggability.
- } catch (error) {} + } catch (error: any) { + if (error?.type !== 'domain_already_exists') { + addNotification({ type: 'warning', message: error?.message ?? 'Failed to create apex domain' }); + } + } ... - } catch (error) {} + } catch (error: any) { + addNotification({ type: 'warning', message: error?.message ?? 'Failed to update nameservers' }); + }Also applies to: 86-86
72-75: Optional chaining on domains list.Prevent crashes if
domainsListis missing.- const domain = data.domainsList.domains.find( + const domain = data.domainsList?.domains?.find( (d: Models.Domain) => d.domain === apexDomain );
90-98: GuardruleIdbefore verification.Avoid calling the API with
nulland show a clear error.- const ruleData = await sdk + if (!ruleId) { + throw new Error('Missing rule ID. Please restart domain verification.'); + } + const ruleData = await sdk .forProject(page.params.region, page.params.project) .proxy.updateRuleVerification({ ruleId });src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/add-domain/verify-[domain]/+page.svelte (4)
46-47: Inconsistentverifiedtyping.Other pages use
boolean. Keep consistent unless components truly need tri‑state.- let verified: boolean | undefined = $state(undefined); + let verified = $state(false);
67-67: Emptycatchblocks.Mirror the handling used in other verify pages to avoid CI failure and improve UX.
- } catch (error) {} + } catch (error: any) { + if (error?.type !== 'domain_already_exists') { + addNotification({ type: 'warning', message: error?.message ?? 'Failed to create apex domain' }); + } + } ... - } catch (error) {} + } catch (error: any) { + addNotification({ type: 'warning', message: error?.message ?? 'Failed to update nameservers' }); + }Also applies to: 84-84
70-73: Optional chaining ondomainsList.Prevent runtime errors when the list is unavailable.
- const domain = data.domainsList.domains.find( + const domain = data.domainsList?.domains?.find( (d: Models.Domain) => d.domain === apexDomain );
88-95: Guard missingruleId.Avoid API call with
nulland provide clear remediation.- const ruleData = await sdk + if (!ruleId) { + throw new Error('Missing rule ID. Please restart domain verification.'); + } + const ruleData = await sdk .forProject(page.params.region, page.params.project) .proxy.updateRuleVerification({ ruleId });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/lib/components/domains/cnameTable.svelte(2 hunks)src/lib/components/domains/nameserverTable.svelte(2 hunks)src/lib/components/domains/recordTable.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/add-domain/+page.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/add-domain/verify-[domain]/+page.svelte(6 hunks)src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/add-domain/verify-[domain]/+page.ts(2 hunks)src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/retryDomainModal.svelte(3 hunks)src/routes/(console)/project-[region]-[project]/settings/domains/add-domain/+page.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/settings/domains/add-domain/verify-[domain]/+page.svelte(6 hunks)src/routes/(console)/project-[region]-[project]/settings/domains/add-domain/verify-[domain]/+page.ts(1 hunks)src/routes/(console)/project-[region]-[project]/settings/domains/retryDomainModal.svelte(3 hunks)src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/add-domain/+page.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/add-domain/verify-[domain]/+page.svelte(6 hunks)src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/add-domain/verify-[domain]/+page.ts(1 hunks)src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/retryDomainModal.svelte(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/add-domain/verify-[domain]/+page.ts (3)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/add-domain/verify-[domain]/+page.ts (1)
load(6-29)src/routes/(console)/project-[region]-[project]/settings/domains/add-domain/verify-[domain]/+page.ts (1)
load(6-29)src/lib/stores/sdk.ts (1)
sdk(142-165)
src/routes/(console)/project-[region]-[project]/settings/domains/add-domain/verify-[domain]/+page.ts (4)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/add-domain/verify-[domain]/+page.ts (1)
load(6-29)src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/add-domain/verify-[domain]/+page.ts (1)
load(6-29)src/lib/system.ts (1)
isCloud(24-24)src/lib/stores/sdk.ts (1)
sdk(142-165)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/add-domain/verify-[domain]/+page.ts (5)
src/routes/(console)/project-[region]-[project]/settings/domains/add-domain/verify-[domain]/+page.ts (1)
load(6-29)src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/add-domain/verify-[domain]/+page.ts (1)
load(6-29)src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/+page.ts (1)
load(10-50)src/routes/(console)/project-[region]-[project]/functions/function-[function]/store.ts (1)
func(7-7)src/lib/stores/sdk.ts (1)
sdk(142-165)
🪛 GitHub Check: build
src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/add-domain/+page.svelte
[failure] 59-59:
Empty block statement
[failure] 96-96:
Empty block statement
src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/add-domain/verify-[domain]/+page.svelte
[failure] 84-84:
Empty block statement
[failure] 67-67:
Empty block statement
src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/add-domain/+page.svelte
[failure] 69-69:
Empty block statement
[failure] 106-106:
Empty block statement
src/routes/(console)/project-[region]-[project]/settings/domains/add-domain/verify-[domain]/+page.svelte
[failure] 86-86:
Empty block statement
[failure] 69-69:
Empty block statement
src/routes/(console)/project-[region]-[project]/settings/domains/add-domain/+page.svelte
[failure] 39-39:
Empty block statement
[failure] 52-52:
Empty block statement
🪛 GitHub Actions: Tests
src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/add-domain/+page.svelte
[error] 59-59: ESLint: Empty block statement (no-empty). Step: pnpm run lint
⏰ 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). (1)
- GitHub Check: e2e
🔇 Additional comments (9)
src/routes/(console)/project-[region]-[project]/settings/domains/add-domain/verify-[domain]/+page.ts (3)
22-22: LGTM: per‑project proxy rule fetch aligns with new ruleStatus plumbing
8-8: Dependencies.DOMAINS is correct for this project-level verify page — no change needed. Matches other project domain routes (src/routes/(console)/project-[region]-[project]/settings/domains/+page.ts and add-domain/+page.ts) and the verify Svelte invalidates the same key.
15-20: domainsList may be undefined when isCloud is false — make the type optional or initialize itdomainsList is declared but only assigned inside the
if (isCloud)branch, so it can be unassigned (undefined) for self‑hosted runs; change the type or provide a default/guard before use.File: src/routes/(console)/project-[region]-[project]/settings/domains/add-domain/verify-[domain]/+page.ts — lines 15–20 (also applies to 24–27)
Suggested change:
-let domainsList: Models.DomainsList; +let domainsList: Models.DomainsList | undefined;If callers expect a non‑null value, either initialize to an empty list/object or guard in the loader/UI before using domainsList.
src/routes/(console)/project-[region]-[project]/settings/domains/add-domain/+page.svelte (1)
54-56: Route usesdomainNameas typed; ensure consistent normalization.If normalization is required (lowercase), apply the same when building the verify URL.
Do you want this to be
verify-${domainName.toLowerCase()}to match the create call?src/routes/(console)/project-[region]-[project]/settings/domains/retryDomainModal.svelte (1)
114-125: Props handoff looks correct.Passing
domain,verified, andruleStatusinto NameserverTable/RecordTable aligns with the new domains UI.src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/retryDomainModal.svelte (1)
114-124: LGTM on new prop wiring.
ruleStatus={selectedProxyRule.status}is consistent across both tables.If not already covered by types, confirm both components export
export let ruleStatus: string | undefined;.src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/add-domain/verify-[domain]/+page.ts (1)
26-27: Return shape clarity.Returning
domainsListas possibly undefined is fine if consumers handle it; otherwise default tonullfor explicitness.Search consumers of
data.domainsListon this route to ensure undefined is handled.src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/add-domain/verify-[domain]/+page.ts (1)
24-28: Return payload LGTM.Naming
function: funcavoids reserved word collisions; inclusion ofproxyRulealigns with the UI.src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/add-domain/+page.svelte (1)
65-68: Team/Org ID source inconsistent — confirm equivalenceSearch shows create calls use different ID sources: some use $project.teamId while verify/org flows use $organization.$id or page.params.organization. Confirm $project.teamId === $organization.$id in all console contexts or unify to a single source to avoid accidental cross-org writes.
Uses $project.teamId:
- src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/add-domain/+page.svelte (lines ~65–67)
- src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/add-domain/+page.svelte (lines ~55–57)
- src/routes/(console)/project-[region]-[project]/settings/domains/add-domain/+page.svelte (lines ~35–37)
Uses $organization.$id or page.params.organization:
- src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/add-domain/verify-[domain]/+page.svelte (teamId: $organization.$id)
- src/routes/(console)/project-[region]-[project]/settings/domains/add-domain/verify-[domain]/+page.svelte (teamId: $organization.$id)
- src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/add-domain/verify-[domain]/+page.svelte (teamId: $organization.$id)
- src/routes/(console)/organization-[organization]/domains/add-domain/+page.svelte (teamId: page.params.organization)
If these IDs are not guaranteed equal, switch to a single canonical org-id source (or explicitly map project.teamId → organization id) before calling sdk.forConsole.domains.create.
| let domain: Models.Domain; | ||
| if (apexDomain && !domain && isCloud) { | ||
| if (isCloud && apexDomain) { | ||
| try { | ||
| domain = await sdk.forConsole.domains.create({ | ||
| teamId: $project.teamId, | ||
| domain: apexDomain | ||
| }); | ||
| } catch (error) { | ||
| // apex might already be added on organization level, skip. | ||
| const alreadyAdded = error?.type === 'domain_already_exists'; | ||
| if (!alreadyAdded) { | ||
| addNotification({ | ||
| type: 'error', | ||
| message: error.message | ||
| }); | ||
| return; | ||
| } | ||
| } | ||
| } catch (error) {} | ||
| } |
There was a problem hiding this comment.
Empty catch triggers lint failure and hides real errors; also domain may remain uninitialized.
Replace the try/catch with a safe promise .catch and make domain nullable.
- let domain: Models.Domain;
-
- if (isCloud && apexDomain) {
- try {
- domain = await sdk.forConsole.domains.create({
- teamId: $project.teamId,
- domain: apexDomain
- });
- } catch (error) {}
- }
+ let domain: Models.Domain | null = null;
+ if (isCloud && apexDomain) {
+ domain = await sdk.forConsole.domains
+ .create({ teamId: $project.teamId, domain: apexDomain })
+ .catch(() => null); // Domain may already exist; safe to ignore
+ }📝 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.
| let domain: Models.Domain; | |
| if (apexDomain && !domain && isCloud) { | |
| if (isCloud && apexDomain) { | |
| try { | |
| domain = await sdk.forConsole.domains.create({ | |
| teamId: $project.teamId, | |
| domain: apexDomain | |
| }); | |
| } catch (error) { | |
| // apex might already be added on organization level, skip. | |
| const alreadyAdded = error?.type === 'domain_already_exists'; | |
| if (!alreadyAdded) { | |
| addNotification({ | |
| type: 'error', | |
| message: error.message | |
| }); | |
| return; | |
| } | |
| } | |
| } catch (error) {} | |
| } | |
| let domain: Models.Domain | null = null; | |
| if (isCloud && apexDomain) { | |
| domain = await sdk.forConsole.domains | |
| .create({ teamId: $project.teamId, domain: apexDomain }) | |
| .catch(() => null); // Domain may already exist; safe to ignore | |
| } |
🧰 Tools
🪛 GitHub Check: build
[failure] 59-59:
Empty block statement
🪛 GitHub Actions: Tests
[error] 59-59: ESLint: Empty block statement (no-empty). Step: pnpm run lint
🤖 Prompt for AI Agents
In
src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/add-domain/+page.svelte
around lines 51 to 60, the empty catch both silences errors and can leave
`domain` uninitialized; change `domain` to be nullable (e.g., Models.Domain |
null) and replace the try/catch with the promise form using `.catch(...)` so any
error is handled explicitly (log or surface it) and set `domain = null` on
failure; additionally, adjust any downstream code to handle the nullable
`domain` case safely.
| return; | ||
| } | ||
| } | ||
| } catch (error) {} |
There was a problem hiding this comment.
🧩 Analysis chain
Fix pipeline: remove empty catch blocks.
These exact lines are failing ESLint (no-empty). The diffs above resolve both occurrences.
Run to confirm no empty catch blocks remain:
Also applies to: 96-96
🏁 Script executed:
#!/bin/bash
rg -nP "catch\s*\([^)]*\)\s*\{\s*\}" -S || trueLength of output: 1764
Fix pipeline: remove empty catch blocks.
ESLint no-empty failures found at:
- src/routes/(console)/project-[region]-[project]/settings/domains/add-domain/+page.svelte:39
- src/routes/(console)/project-[region]-[project]/settings/domains/add-domain/+page.svelte:52
- src/routes/(console)/project-[region]-[project]/settings/domains/add-domain/verify-[domain]/+page.svelte:69
- src/routes/(console)/project-[region]-[project]/settings/domains/add-domain/verify-[domain]/+page.svelte:86
- src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/add-domain/+page.svelte:69
- src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/add-domain/+page.svelte:106
- src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/add-domain/verify-[domain]/+page.svelte:69
- src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/add-domain/verify-[domain]/+page.svelte:86
- src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/add-domain/+page.svelte:59
- src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/add-domain/+page.svelte:96
- src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/add-domain/verify-[domain]/+page.svelte:67
- src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/add-domain/verify-[domain]/+page.svelte:84
Replace empty catch bodies with proper handling (log, rethrow, or an explicit justified eslint-disable comment).
🧰 Tools
🪛 GitHub Check: build
[failure] 59-59:
Empty block statement
🪛 GitHub Actions: Tests
[error] 59-59: ESLint: Empty block statement (no-empty). Step: pnpm run lint
🤖 Prompt for AI Agents
In
src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/add-domain/+page.svelte
around line 59, there is an empty catch block; replace it with proper error
handling by capturing the caught error (e.g., catch (error)) and either log it
(console.error/processLogger.error) and surface a user-friendly message/state
update, rethrow the error if it must propagate, or if suppression is intentional
add a single-line eslint-disable comment with a justification; ensure any added
logging includes the error object for debugging and preserve original control
flow.
| if (isCloud) { | ||
| try { | ||
| await sdk.forConsole.domains.updateNameservers({ domainId: domain.$id }); | ||
| } catch (error) {} | ||
| } |
There was a problem hiding this comment.
Guard updateNameservers and avoid empty catch.
Prevent null deref on domain and remove empty catch.
- if (isCloud) {
- try {
- await sdk.forConsole.domains.updateNameservers({ domainId: domain.$id });
- } catch (error) {}
- }
+ if (isCloud && domain?.$id) {
+ await sdk.forConsole.domains
+ .updateNameservers({ domainId: domain.$id })
+ .catch(() => {}); // Non-blocking best-effort
+ }📝 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.
| if (isCloud) { | |
| try { | |
| await sdk.forConsole.domains.updateNameservers({ domainId: domain.$id }); | |
| } catch (error) {} | |
| } | |
| if (isCloud && domain?.$id) { | |
| await sdk.forConsole.domains | |
| .updateNameservers({ domainId: domain.$id }) | |
| .catch(() => void 0); // Non-blocking best-effort | |
| } |
🧰 Tools
🪛 GitHub Check: build
[failure] 96-96:
Empty block statement
🤖 Prompt for AI Agents
In
src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/add-domain/+page.svelte
around lines 93-97, avoid potential null dereference of domain and remove the
empty catch: ensure you only call updateNameservers when domain and domain.$id
are present and when the method exists (use a guard/optional chaining on
sdk.forConsole.domains.updateNameservers), and replace the empty catch with
proper error handling (e.g., log the error and/or surface a user-facing message
or rethrow) so failures are not silently ignored.
...le)/project-[region]-[project]/functions/function-[function]/domains/retryDomainModal.svelte
Show resolved
Hide resolved
...le)/project-[region]-[project]/functions/function-[function]/domains/retryDomainModal.svelte
Outdated
Show resolved
Hide resolved
| return; | ||
| } | ||
| } | ||
| } catch (error) {} |
There was a problem hiding this comment.
Fix build failure: empty block statements.
Fill the catch blocks with logging/notifications as suggested above to resolve CI.
Also applies to: 52-52
🧰 Tools
🪛 GitHub Check: build
[failure] 39-39:
Empty block statement
🤖 Prompt for AI Agents
In
src/routes/(console)/project-[region]-[project]/settings/domains/add-domain/+page.svelte
around lines 39 and 52, there are empty catch blocks causing build failures;
replace each empty catch with proper error handling: catch the error as unknown
(e.g., catch (error: unknown)), log it (console.error or processLogger) and
surface a user-facing notification or set an error state so the UI shows the
failure (use the existing toast/notification helper in the project or import
one), and optionally rethrow or return after handling; ensure imports for any
notification utilities are added.
| try { | ||
| await sdk.forConsole.domains.updateNameservers({ domainId: domain.$id }); | ||
| } catch (error) {} | ||
| } |
There was a problem hiding this comment.
Avoid null dereference on domain.$id and surface errors.
domain can be null if creation failed/skipped. Also, the empty catch eats real issues.
Apply:
- if (isCloud) {
- try {
- await sdk.forConsole.domains.updateNameservers({ domainId: domain.$id });
- } catch (error) {}
- }
+ if (isCloud && apexDomain && domain?.$id) {
+ try {
+ await sdk.forConsole.domains.updateNameservers({ domainId: domain.$id });
+ } catch (error) {
+ console.warn('updateNameservers failed for', apexDomain, error);
+ addNotification({
+ type: 'warning',
+ message: `Nameserver update skipped: ${apexDomain}`
+ });
+ }
+ }📝 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.
| try { | |
| await sdk.forConsole.domains.updateNameservers({ domainId: domain.$id }); | |
| } catch (error) {} | |
| } | |
| if (isCloud && apexDomain && domain?.$id) { | |
| try { | |
| await sdk.forConsole.domains.updateNameservers({ domainId: domain.$id }); | |
| } catch (error) { | |
| console.warn('updateNameservers failed for', apexDomain, error); | |
| addNotification({ | |
| type: 'warning', | |
| message: `Nameserver update skipped: ${apexDomain}` | |
| }); | |
| } | |
| } |
🧰 Tools
🪛 GitHub Check: build
[failure] 52-52:
Empty block statement
🤖 Prompt for AI Agents
In
src/routes/(console)/project-[region]-[project]/settings/domains/add-domain/+page.svelte
around lines 50–53, avoid dereferencing domain.$id when domain may be null and
stop swallowing errors in the catch block: first guard that domain and
domain.$id exist (e.g., if (!domain?.$id) return or display an appropriate
error/validation message to the user) before calling
sdk.forConsole.domains.updateNameservers, and replace the empty catch with error
handling that surfaces the problem (log to console, show a user-facing
toast/alert, or rethrow) so real failures are visible to developers and users.
| $effect(() => { | ||
| if ($regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME && isSubDomain) { | ||
| selectedTab = 'cname'; | ||
| } else if (!isCloud && $regionalConsoleVariables._APP_DOMAIN_TARGET_A) { | ||
| selectedTab = 'a'; | ||
| } else if (!isCloud && $regionalConsoleVariables._APP_DOMAIN_TARGET_AAAA) { | ||
| selectedTab = 'aaaa'; | ||
| } else { | ||
| selectedTab = 'nameserver'; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Initial tab selection can target a non-rendered tab (mismatch with UI conditions).
The $effect doesn’t mirror the render-time exclusions for localhost/loopback targets. It can auto-select a/aaaa/cname even when those tabs are hidden, leaving content out of sync with the visible tabs.
Apply this diff to align selection with render conditions:
$effect(() => {
- if ($regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME && isSubDomain) {
- selectedTab = 'cname';
- } else if (!isCloud && $regionalConsoleVariables._APP_DOMAIN_TARGET_A) {
- selectedTab = 'a';
- } else if (!isCloud && $regionalConsoleVariables._APP_DOMAIN_TARGET_AAAA) {
- selectedTab = 'aaaa';
- } else {
- selectedTab = 'nameserver';
- }
+ const hasCname =
+ isSubDomain &&
+ !!$regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME &&
+ $regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME !== 'localhost';
+ const hasA =
+ !isCloud &&
+ !!$regionalConsoleVariables._APP_DOMAIN_TARGET_A &&
+ $regionalConsoleVariables._APP_DOMAIN_TARGET_A !== '127.0.0.1';
+ const hasAaaa =
+ !isCloud &&
+ !!$regionalConsoleVariables._APP_DOMAIN_TARGET_AAAA &&
+ $regionalConsoleVariables._APP_DOMAIN_TARGET_AAAA !== '::1';
+
+ if (hasCname) selectedTab = 'cname';
+ else if (hasA) selectedTab = 'a';
+ else if (hasAaaa) selectedTab = 'aaaa';
+ else selectedTab = 'nameserver';
});📝 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.
| $effect(() => { | |
| if ($regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME && isSubDomain) { | |
| selectedTab = 'cname'; | |
| } else if (!isCloud && $regionalConsoleVariables._APP_DOMAIN_TARGET_A) { | |
| selectedTab = 'a'; | |
| } else if (!isCloud && $regionalConsoleVariables._APP_DOMAIN_TARGET_AAAA) { | |
| selectedTab = 'aaaa'; | |
| } else { | |
| selectedTab = 'nameserver'; | |
| } | |
| }); | |
| $effect(() => { | |
| const hasCname = | |
| isSubDomain && | |
| !!$regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME && | |
| $regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME !== 'localhost'; | |
| const hasA = | |
| !isCloud && | |
| !!$regionalConsoleVariables._APP_DOMAIN_TARGET_A && | |
| $regionalConsoleVariables._APP_DOMAIN_TARGET_A !== '127.0.0.1'; | |
| const hasAaaa = | |
| !isCloud && | |
| !!$regionalConsoleVariables._APP_DOMAIN_TARGET_AAAA && | |
| $regionalConsoleVariables._APP_DOMAIN_TARGET_AAAA !== '::1'; | |
| if (hasCname) selectedTab = 'cname'; | |
| else if (hasA) selectedTab = 'a'; | |
| else if (hasAaaa) selectedTab = 'aaaa'; | |
| else selectedTab = 'nameserver'; | |
| }); |
| if (isCloud && apexDomain && !isSiteDomain) { | ||
| try { | ||
| domain = await sdk.forConsole.domains.create({ | ||
| teamId: $project.teamId, | ||
| domain: apexDomain | ||
| }); | ||
| } catch (error) { | ||
| // apex might already be added on organization level, skip. | ||
| const alreadyAdded = error?.type === 'domain_already_exists'; | ||
| if (!alreadyAdded) { | ||
| addNotification({ | ||
| type: 'error', | ||
| message: error.message | ||
| }); | ||
| return; | ||
| } | ||
| } | ||
| } catch (error) {} | ||
| } |
There was a problem hiding this comment.
Over-eager apex domain creation and swallowed errors.
- Removing the
!domainguard triggers a create attempt on every add (cloud + apex), causing noisydomain_already_existserrors. - The empty
catchhides real failures and trips CI (“Empty block statement”).
Handle the known duplicate gracefully and report unexpected errors.
Apply this diff:
- if (isCloud && apexDomain && !isSiteDomain) {
- try {
- domain = await sdk.forConsole.domains.create({
- teamId: $project.teamId,
- domain: apexDomain
- });
- } catch (error) {}
- }
+ if (isCloud && apexDomain && !isSiteDomain) {
+ try {
+ domain = await sdk.forConsole.domains.create({
+ teamId: $project.teamId,
+ domain: apexDomain
+ });
+ } catch (error: any) {
+ // Ignore "already exists", surface anything else.
+ if (error?.type !== 'domain_already_exists') {
+ addNotification({
+ type: 'error',
+ message: error?.message ?? 'Failed to create apex domain'
+ });
+ return;
+ }
+ }
+ }📝 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.
| if (isCloud && apexDomain && !isSiteDomain) { | |
| try { | |
| domain = await sdk.forConsole.domains.create({ | |
| teamId: $project.teamId, | |
| domain: apexDomain | |
| }); | |
| } catch (error) { | |
| // apex might already be added on organization level, skip. | |
| const alreadyAdded = error?.type === 'domain_already_exists'; | |
| if (!alreadyAdded) { | |
| addNotification({ | |
| type: 'error', | |
| message: error.message | |
| }); | |
| return; | |
| } | |
| } | |
| } catch (error) {} | |
| } | |
| if (isCloud && apexDomain && !isSiteDomain) { | |
| try { | |
| domain = await sdk.forConsole.domains.create({ | |
| teamId: $project.teamId, | |
| domain: apexDomain | |
| }); | |
| } catch (error: any) { | |
| // Ignore "already exists", surface anything else. | |
| if (error?.type !== 'domain_already_exists') { | |
| addNotification({ | |
| type: 'error', | |
| message: error?.message ?? 'Failed to create apex domain' | |
| }); | |
| return; | |
| } | |
| } | |
| } |
🧰 Tools
🪛 GitHub Check: build
[failure] 69-69:
Empty block statement
🤖 Prompt for AI Agents
In
src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/add-domain/+page.svelte
around lines 63–70, reintroduce the guard so we only attempt creating the apex
domain when no domain object exists (e.g., keep/check !domain) and avoid
creating on every add; wrap the create call in a try/catch that does not swallow
errors—detect the known duplicate case (error code/message like
"domain_already_exists") and ignore or handle it silently, but for any other
error log/report it to the UI or rethrow so CI/monitoring sees it (do not leave
an empty catch block).
...outes/(console)/project-[region]-[project]/sites/site-[site]/domains/add-domain/+page.svelte
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/retryDomainModal.svelte (1)
52-56: Success toast fires even when verification didn’t succeed.You always notify success after
updateRuleVerification, regardless ofdomain.status. Gate the toast on success; otherwise show an info/error.- show = false; - verified = domain.status === 'verified'; - await invalidate(Dependencies.SITES_DOMAINS); - - addNotification({ - type: 'success', - message: `${selectedProxyRule.domain} has been verified` - }); + show = false; + verified = domain.status === 'verified'; + await invalidate(Dependencies.SITES_DOMAINS); + if (verified) { + addNotification({ + type: 'success', + message: `${selectedProxyRule.domain} has been verified` + }); + } else { + addNotification({ + type: 'info', + message: `Verification retried. Current status: ${domain.status}` + }); + }
♻️ Duplicate comments (3)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/retryDomainModal.svelte (2)
40-41: Initialize verified from the selected rule.Prevents incorrect “unverified” badges before retry completes.
-let verified = $state(false); +let verified = $state(selectedProxyRule?.status === 'verified');
52-55: Toast claims “verified” even when status isn’t verified.Use the returned status in the message.
addNotification({ type: 'success', - message: `${selectedProxyRule.domain} has been verified` + message: + domain.status === 'verified' + ? `${selectedProxyRule.domain} has been verified` + : `Verification retried for ${selectedProxyRule.domain} (status: ${domain.status})` });src/routes/(console)/project-[region]-[project]/settings/domains/retryDomainModal.svelte (1)
27-37: Initial tab can target a hidden tab. Mirror render conditions.Prevents selecting CNAME/A/AAAA when they’re filtered (localhost/loopback) and avoiding Nameservers on self‑hosted.
$effect(() => { - if ($regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME) { - selectedTab = 'cname'; - } else if (!isCloud && $regionalConsoleVariables._APP_DOMAIN_TARGET_A) { - selectedTab = 'a'; - } else if (!isCloud && $regionalConsoleVariables._APP_DOMAIN_TARGET_AAAA) { - selectedTab = 'aaaa'; - } else { - selectedTab = 'nameserver'; - } + const hasCname = + !!$regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME && + $regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME !== 'localhost'; + const hasNameserver = isCloud; + const hasA = + !isCloud && + !!$regionalConsoleVariables._APP_DOMAIN_TARGET_A && + $regionalConsoleVariables._APP_DOMAIN_TARGET_A !== '127.0.0.1'; + const hasAaaa = + !isCloud && + !!$regionalConsoleVariables._APP_DOMAIN_TARGET_AAAA && + $regionalConsoleVariables._APP_DOMAIN_TARGET_AAAA !== '::1'; + + if (hasCname) selectedTab = 'cname'; + else if (hasNameserver) selectedTab = 'nameserver'; + else if (hasA) selectedTab = 'a'; + else if (hasAaaa) selectedTab = 'aaaa'; + else selectedTab = 'cname'; });
🧹 Nitpick comments (23)
src/lib/components/domains/cnameTable.svelte (2)
27-39: Double‑check status→badge mapping ("created" => "Verification failed" looks off).Confirm backend enum→UI copy. Today:
- created → "Verification failed"
- verifying → "Generating certificate"
- unverified → "Certificate generation failed"
If "created" is initial/new, showing "failed" is misleading. Consider a small mapper + default branch to avoid incorrect badges for unknown states.
Apply a mapper inline:
- {#if ruleStatus === 'created'} - <Badge variant="secondary" type="error" size="xs" content="Verification failed" /> - {:else if ruleStatus === 'verifying'} - <Badge variant="secondary" size="xs" content="Generating certificate" /> - {:else if ruleStatus === 'unverified'} - <Badge - variant="secondary" - type="error" - size="xs" - content="Certificate generation failed" /> + {#if ruleStatus} + {#if ruleStatus === 'verifying'} + <Badge variant="secondary" size="xs" content="Generating certificate" /> + {:else if ruleStatus === 'unverified' || ruleStatus === 'failed'} + <Badge variant="secondary" type="error" size="xs" content="Certificate generation failed" /> + {:else if ruleStatus === 'created'} + <Badge variant="secondary" size="xs" content="Verification pending" /> + {/if} {:else if verified === true} <Badge variant="secondary" type="success" size="xs" content="Verified" /> {/if}
63-81: CAA row UX: clarify value format for common cases.Nice addition. Minor polish: add a short hint that multiple CAA tags (issue, issuewild) may be needed, and that the provided value is a recommended baseline.
src/lib/components/domains/recordTable.svelte (4)
54-66: Same status→badge mapping concerns as cnameTable.Align copy and handle unknown states via a mapper to avoid showing a "failed" badge for "created".
Apply the same diff pattern used in cnameTable here.
25-33: Make A/AAAA tab visibility reactive (or explicitly constant).These are computed once. If regionalConsoleVariables can change (region switch), this won’t update. Either:
- mark as intentional (constant per session), or
- make them reactive (
$:) so UI updates when the store changes.
108-130: Apex detection uses a naive subdomain check; use getApexDomain for correctness.
!subdomainassumes two‑label apex (fails for e.g., example.co.uk). Use TLD helper for apex detection and subdomain derivation.Apply this diff locally for the condition:
- {#if variant === 'cname' && !subdomain} + {#if variant === 'cname' && domain === getApexDomain(domain)}And add at top:
+ import { getApexDomain } from '$lib/helpers/tlds';Outside this hunk, update subdomain derivation to be TLD‑aware (example):
// replace current subdomain logic const apex = getApexDomain(domain); const subdomain = domain && apex && domain.endsWith(`.${apex}`) ? domain.slice(0, -(apex.length + 1)) : '';
87-105: CAA row: same minor UX hint as cnameTable.Consider a brief caption or help icon noting when to use issue vs issuewild and that multiple CAA records can coexist.
src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/add-domain/verify-[domain]/+page.svelte (4)
35-45: Default tab gating inconsistent with tab visibility.You gate the CNAME tab button on
_APP_DOMAIN_TARGET_CNAME !== 'localhost', but the default‑selection effect doesn’t. Result: default selects a hidden tab. Align both.Apply this diff:
- if ($regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME) { + if ($regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME && $regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME !== 'localhost') { selectedTab = 'cname';Also applies to: 151-159
52-87: Apex‑domain pre‑verification: silent catches hide actionable failures.Swallowing errors twice makes troubleshooting hard. At minimum, log or track them; ideally, surface a non‑blocking info notification.
Add lightweight logging:
- } catch (error) {} + } catch (error) { + console.warn('Apex domain create/verify skipped:', error); + } ... - } catch (error) {} + } catch (error) { + console.warn('Apex domain nameserver update skipped:', error); + }
107-114: Type‑safe error handling in catch.
errorisunknownin TS. Accessingerror.messagecan break type‑checks. Normalize safely.Apply this diff:
- } catch (error) { + } catch (error: unknown) { verified = false; isSubmitting.set(false); addNotification({ type: 'error', - message: error.message + message: error instanceof Error ? error.message : String(error) }); }
123-124: URL‑encode the domain query param.Avoid issues with IDNs/punycode or special chars.
Apply this diff:
- await goto(`${routeBase}/add-domain?domain=${data.proxyRule.domain}`); + await goto(`${routeBase}/add-domain?domain=${encodeURIComponent(data.proxyRule.domain)}`);src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/add-domain/verify-[domain]/+page.svelte (4)
35-44: Default tab gating inconsistent with tab visibility (same as Sites).Add the
!== 'localhost'guard to the default‑selection effect.- if ($regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME) { + if ($regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME && $regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME !== 'localhost') { selectedTab = 'cname';Also applies to: 149-157
52-86: Apex‑domain pre‑verification: avoid empty catches.Mirror Sites page advice: at least log warnings so support can trace issues.
- } catch (error) {} + } catch (error) { + console.warn('Apex domain create/verify skipped:', error); + } ... - } catch (error) {} + } catch (error) { + console.warn('Apex domain nameserver update skipped:', error); + }
105-112: Type‑safe error handling in catch.Normalize
unknownerrors before reading.message.- } catch (error) { + } catch (error: unknown) { verified = false; isSubmitting.set(false); addNotification({ type: 'error', - message: error.message + message: error instanceof Error ? error.message : String(error) }); }
121-122: URL‑encode the domain query param on back.- await goto(`${routeBase}/add-domain?domain=${data.proxyRule.domain}`); + await goto(`${routeBase}/add-domain?domain=${encodeURIComponent(data.proxyRule.domain)}`);src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/retryDomainModal.svelte (1)
28-36: Default tab gating inconsistent with visibility (localhost case).Align the effect with the tab button guard to avoid defaulting to a hidden CNAME tab.
- if ($regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME) { + if ($regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME && $regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME !== 'localhost') { selectedTab = 'cname';Also applies to: 75-83
src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/retryDomainModal.svelte (3)
27-37: Align default tab selection with what’s actually rendered.Effect can select CNAME/A/AAAA even when hidden (localhost/loopback), or fall back to Nameservers on self‑hosted. Mirror render conditions.
$effect(() => { - if ($regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME) { - selectedTab = 'cname'; - } else if (!isCloud && $regionalConsoleVariables._APP_DOMAIN_TARGET_A) { - selectedTab = 'a'; - } else if (!isCloud && $regionalConsoleVariables._APP_DOMAIN_TARGET_AAAA) { - selectedTab = 'aaaa'; - } else { - selectedTab = 'nameserver'; - } + const hasCname = + !!$regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME && + $regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME !== 'localhost'; + const hasNameserver = isCloud; + const hasA = + !isCloud && + !!$regionalConsoleVariables._APP_DOMAIN_TARGET_A && + $regionalConsoleVariables._APP_DOMAIN_TARGET_A !== '127.0.0.1'; + const hasAaaa = + !isCloud && + !!$regionalConsoleVariables._APP_DOMAIN_TARGET_AAAA && + $regionalConsoleVariables._APP_DOMAIN_TARGET_AAAA !== '::1'; + + if (hasCname) selectedTab = 'cname'; + else if (hasNameserver) selectedTab = 'nameserver'; + else if (hasA) selectedTab = 'a'; + else if (hasAaaa) selectedTab = 'aaaa'; + else selectedTab = 'cname'; });
58-61: Handle unknown error type safely.Avoid assuming
e.message.- error = - e.message ?? - 'Domain verification failed. Please check your domain settings or try again later'; + const msg = + e instanceof Error + ? e.message + : (typeof e === 'string' ? e : null); + error = + msg ?? + 'Domain verification failed. Please check your domain settings or try again later';
52-55: Optional: notification type by status.Success for verified; info otherwise.
-addNotification({ - type: 'success', - message: - domain.status === 'verified' - ? `${selectedProxyRule.domain} has been verified` - : `Verification retried for ${selectedProxyRule.domain} (status: ${domain.status})` -}); +addNotification({ + type: domain.status === 'verified' ? 'success' : 'info', + message: + domain.status === 'verified' + ? `${selectedProxyRule.domain} has been verified` + : `Verification retried for ${selectedProxyRule.domain} (status: ${domain.status})` +});src/routes/(console)/project-[region]-[project]/settings/domains/retryDomainModal.svelte (5)
18-23: Default bindable show to false for consistency.Prevents an accidental undefined initial state.
- let { - show = $bindable(), + let { + show = $bindable(false), selectedDomain
40-41: Initialize verified from the selected domain status.Ensures correct badges before retry.
-let verified = $state(false); +let verified = $state(selectedDomain?.status === 'verified');
52-55: Toast claims “verified” regardless of result.Reflect actual
domain.status.addNotification({ type: 'success', - message: `${selectedDomain.domain} has been verified` + message: + domain.status === 'verified' + ? `${selectedDomain.domain} has been verified` + : `Verification retried for ${selectedDomain.domain} (status: ${domain.status})` });
58-61: Harden error handling for non-Error throws.Avoids
any/unknown pitfalls.- error = - e.message ?? - 'Domain verification failed. Please check your domain settings or try again later'; + const msg = + e instanceof Error + ? e.message + : (typeof e === 'string' ? e : null); + error = + msg ?? + 'Domain verification failed. Please check your domain settings or try again later';
52-55: Optional: notification type by status.Use success for verified; info otherwise.
-addNotification({ - type: 'success', - message: - domain.status === 'verified' - ? `${selectedDomain.domain} has been verified` - : `Verification retried for ${selectedDomain.domain} (status: ${domain.status})` -}); +addNotification({ + type: domain.status === 'verified' ? 'success' : 'info', + message: + domain.status === 'verified' + ? `${selectedDomain.domain} has been verified` + : `Verification retried for ${selectedDomain.domain} (status: ${domain.status})` +});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/lib/components/domains/cnameTable.svelte(3 hunks)src/lib/components/domains/recordTable.svelte(3 hunks)src/routes/(console)/organization-[organization]/domains/domain-[domain]/+page.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/add-domain/verify-[domain]/+page.svelte(7 hunks)src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/retryDomainModal.svelte(3 hunks)src/routes/(console)/project-[region]-[project]/settings/domains/add-domain/verify-[domain]/+page.svelte(7 hunks)src/routes/(console)/project-[region]-[project]/settings/domains/retryDomainModal.svelte(3 hunks)src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/add-domain/verify-[domain]/+page.svelte(7 hunks)src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/retryDomainModal.svelte(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/routes/(console)/organization-[organization]/domains/domain-[domain]/+page.svelte
🚧 Files skipped from review as they are similar to previous changes (1)
- src/routes/(console)/project-[region]-[project]/settings/domains/add-domain/verify-[domain]/+page.svelte
⏰ 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). (2)
- GitHub Check: e2e
- GitHub Check: build
🔇 Additional comments (1)
src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/retryDomainModal.svelte (1)
111-115: Props passthrough LGTM.Passing ruleStatus to child tables is consistent with the new badge logic; navigation callbacks are wired correctly.
Please sanity‑check that NameserverTable uses ruleStatus the same way as the Record/CNAME tables do.
Also applies to: 120-124
...ion]-[project]/functions/function-[function]/domains/add-domain/verify-[domain]/+page.svelte
Show resolved
Hide resolved
...project-[region]-[project]/sites/site-[site]/domains/add-domain/verify-[domain]/+page.svelte
Show resolved
Hide resolved
vermakhushboo
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/add-domain/+page.svelte (1)
60-60: Type should reflect optional state (domaincan be undefined).
domainis not guaranteed to be set; widen the type to avoid misleading types and follow-on runtime assumptions.-let domain: Models.Domain; +let domain: Models.Domain | undefined;src/routes/(console)/project-[region]-[project]/settings/domains/add-domain/verify-[domain]/+page.svelte (3)
55-55: Useconsole.debuginstead ofconsole.logfor debug statementsProduction code should use appropriate logging levels.
-console.log('apexDomain', apexDomain); +console.debug('apexDomain', apexDomain);
62-67: Silent verification failures from apex domain creation are confusingThe apex domain creation sets
verified = truebased on nameservers, but then throws an error if not verified. This error is caught silently in lines 68-70, leaving users unaware of the actual verification state until line 97.Consider removing the intermediate verification check since the final verification happens at line 97 anyway:
-verified = domainData.nameservers.toLowerCase() === 'appwrite'; -if (!verified) { - throw new Error( - 'Domain verification failed. Please check your domain settings or try again later' - ); -} +// Apex domain creation successful, verification will be checked later
68-70: Empty catch blocks without context lose important error detailsSilently swallowing errors makes debugging difficult. Consider at least logging them at debug level.
-} catch (error) { - // Empty as domain creation error needs to be silent -} +} catch (error) { + console.debug('Apex domain creation skipped:', error.message); +}And similarly for the second catch block:
-} catch (error) { - // Empty as domain update error needs to be silent -} +} catch (error) { + console.debug('Apex domain nameserver update skipped:', error.message); +}Also applies to: 87-89
src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/add-domain/verify-[domain]/+page.svelte (3)
62-67: Silent verification failures from apex domain creation are confusingSame issue as in the settings domains file - the apex domain creation sets
verified = truebased on nameservers but throws an error that gets silently caught.Consider removing the intermediate verification check:
-verified = domainData.nameservers.toLowerCase() === 'appwrite'; -if (!verified) { - throw new Error( - 'Domain verification failed. Please check your domain settings or try again later' - ); -} +// Apex domain creation successful, verification will be checked later
68-70: Empty catch blocks lose debugging contextConsider adding debug logging for troubleshooting:
-} catch (error) { - // Empty as domain creation error needs to be silent -} +} catch (error) { + console.debug('Apex domain creation skipped:', error.message); +}And for the second catch:
-} catch (error) { - // Empty as domain update error needs to be silent -} +} catch (error) { + console.debug('Apex domain nameserver update skipped:', error.message); +}Also applies to: 87-89
52-119: Extract apex-domain verification into a shared helper (src/lib/helpers/domains.ts)Identical apex-domain extraction + create/update verification logic appears in multiple places — consolidate into a single helper (e.g., verifyApexDomain) and call it from each location:
- src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/add-domain/verify-[domain]/+page.svelte
- src/routes/(console)/project-[region]-[project]/settings/domains/add-domain/verify-[domain]/+page.svelte
- src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/add-domain/verify-[domain]/+page.svelte
- src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/add-domain/+page.svelte
- src/routes/(console)/project-[region]-[project]/settings/domains/add-domain/+page.svelte
- src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/add-domain/+page.svelte
- (helper candidate) src/lib/helpers/domains.ts — add exported verification utility here and replace the in-file implementations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/add-domain/+page.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/add-domain/verify-[domain]/+page.svelte(7 hunks)src/routes/(console)/project-[region]-[project]/settings/domains/add-domain/+page.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/settings/domains/add-domain/verify-[domain]/+page.svelte(7 hunks)src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/add-domain/+page.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/add-domain/verify-[domain]/+page.svelte(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/routes/(console)/project-[region]-[project]/settings/domains/add-domain/+page.svelte
⏰ 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). (2)
- GitHub Check: build
- GitHub Check: e2e
🔇 Additional comments (9)
src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/add-domain/+page.svelte (2)
63-71: Stop swallowing apex create errors; handle duplicate explicitly and surface real failures.Empty catch trips CI and hides genuine faults. Treat
domain_already_existsas benign; report others and abort to avoid inconsistent follow-up.- if (isCloud && apexDomain && !isSiteDomain) { - try { - domain = await sdk.forConsole.domains.create({ - teamId: $project.teamId, - domain: apexDomain - }); - } catch (error) { - // Empty as domain creation error needs to be silent - } - } + if (isCloud && apexDomain && !isSiteDomain) { + try { + domain = await sdk.forConsole.domains.create({ + teamId: $project.teamId, + domain: apexDomain + }); + } catch (error: any) { + // Ignore "already exists", surface anything else. + if (error?.type !== 'domain_already_exists') { + addNotification({ + type: 'error', + message: error?.message ?? 'Failed to create apex domain' + }); + return; + } + } + }Optional: If you have the domains list in
data, short‑circuit the create when the apex already exists to avoid noisy duplicates.
105-111: GuardupdateNameserversand don’t swallow failures.
domainmay be undefined; callingdomain.$idcan crash. Also avoid empty catch; warn the user so they can retry via “View logs.”- if (isCloud) { - try { - await sdk.forConsole.domains.updateNameservers({ domainId: domain.$id }); - } catch (error) { - // Empty as domain update error needs to be silent - } - } + if (isCloud && domain?.$id) { + try { + await sdk.forConsole.domains.updateNameservers({ domainId: domain.$id }); + } catch (error: any) { + addNotification({ + type: 'warning', + message: + 'Failed to update nameservers for apex domain. You can retry from Domains → View logs.' + }); + } + }src/routes/(console)/project-[region]-[project]/settings/domains/add-domain/verify-[domain]/+page.svelte (1)
93-96: Guard missing ruleId before verifying
ruleIdcan be null (line 31); passing it into the SDK will likely throw. This should fail fast with a clear message.+if (!ruleId) { + throw new Error('Missing rule id. Please restart the verification flow.'); +} const ruleData = await sdk .forProject(page.params.region, page.params.project) .proxy.updateRuleVerification({ ruleId });src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/add-domain/verify-[domain]/+page.svelte (1)
93-95: Missing ruleId guard before SDK call
ruleIdfrom line 31 can be null. The SDK call needs protection against null values.+if (!ruleId) { + throw new Error('Missing rule id. Please restart the verification flow.'); +} const ruleData = await sdk .forProject(page.params.region, page.params.project) .proxy.updateRuleVerification({ ruleId });src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/add-domain/+page.svelte (2)
59-61: Fix empty catch block that causes lint failure.ESLint ignores block statements which contain a comment to indicate intentional behavior. The comments in lines 60 and 99 resolve the lint issue, but the approach violates error handling best practices.
Apply this diff to handle domain creation errors properly:
- let domain: Models.Domain; - - if (isCloud && apexDomain) { - try { - domain = await sdk.forConsole.domains.create({ - teamId: $project.teamId, - domain: apexDomain - }); - } catch (error) { - // Empty as domain creation error needs to be silent - } - } + let domain: Models.Domain | null = null; + if (isCloud && apexDomain) { + domain = await sdk.forConsole.domains + .create({ teamId: $project.teamId, domain: apexDomain }) + .catch(() => null); // Domain may already exist; safe to ignore + }
95-101: Guard against null reference and fix empty catch block.ESLint ignores block statements which contain a comment, but this approach can mask real errors and cause null reference issues if
domainis undefined.Apply this diff to safely handle the nameserver update:
- if (isCloud) { - try { - await sdk.forConsole.domains.updateNameservers({ domainId: domain.$id }); - } catch (error) { - // Empty as domain update error needs to be silent - } - } + if (isCloud && domain?.$id) { + await sdk.forConsole.domains + .updateNameservers({ domainId: domain.$id }) + .catch(() => {}); // Non-blocking best-effort + }src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/add-domain/verify-[domain]/+page.svelte (3)
91-95: Guard against missing ruleId before SDK call.The code calls
updateRuleVerificationwithruleIdwithout checking if it's null or undefined, which could cause SDK errors.+ if (!ruleId) { + throw new Error('Missing rule ID. Please restart the verification flow.'); + } + const ruleData = await sdk .forProject(page.params.region, page.params.project) .proxy.updateRuleVerification({ ruleId });
189-202: LGTM! Clean component integration with proper status propagation.The component integration correctly passes
ruleStatusfromdata.proxyRule.statusto bothNameserverTableandRecordTablecomponents, along with the navigation callbacks for tab switching. This provides a consistent UI experience across the verification flow.
52-89: Complex apex domain verification with multiple silent error handling issues.The verification flow has several problematic patterns:
- Multiple empty catch blocks that silence all errors without proper handling
- Inconsistent verification state management
- Potential null reference issues with domain lookups
Apply this comprehensive refactor:
async function verify() { try { + let apexVerified = true; if (isCloud) { const apexDomain = getApexDomain(data.proxyRule.domain); if (apexDomain) { - try { - const domainData = await sdk.forConsole.domains.create({ - teamId: $organization.$id, - domain: apexDomain - }); - verified = domainData.nameservers.toLowerCase() === 'appwrite'; - if (!verified) { - throw new Error( - 'Domain verification failed. Please check your domain settings or try again later' - ); - } - } catch (error) { - // Empty as domain creation error needs to be silent - } - - try { + // Create apex domain if needed (ignore if already exists) + const domainData = await sdk.forConsole.domains.create({ + teamId: $organization.$id, + domain: apexDomain + }).catch(() => null); + + if (domainData) { + apexVerified = domainData.nameservers.toLowerCase() === 'appwrite'; + } + + // Update nameservers for existing domain + if (!apexVerified) { const domain = data.domainsList.domains.find( (d: Models.Domain) => d.domain === apexDomain ); if (domain) { - const output = await sdk.forConsole.domains.updateNameservers({ + const output = await sdk.forConsole.domains + .updateNameservers({ - domainId: domain.$id - }); - verified = output.nameservers.toLowerCase() === 'appwrite'; - if (!verified) { - throw new Error( - 'Domain verification failed. Please check your domain settings or try again later' - ); - } + domainId: domain.$id + }) + .catch(() => ({ nameservers: '' })); + apexVerified = output.nameservers.toLowerCase() === 'appwrite'; } - } catch (error) { - // Empty as domain update error needs to be silent } + + if (!apexVerified) { + throw new Error( + 'Apex domain verification failed. Please check your domain settings.' + ); + } } } + if (!ruleId) { + throw new Error('Missing rule ID. Please restart the verification flow.'); + } + const ruleData = await sdk .forProject(page.params.region, page.params.project) .proxy.updateRuleVerification({ ruleId }); verified = ruleData.status === 'verified'; if (!verified) { throw new Error( 'Domain verification failed. Please check your domain settings or try again later' ); }
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/table.svelte (1)
41-41: Fix type: selectedProxyRule can be null.Allow null to satisfy TS and Svelte runes.
-let selectedProxyRule: Models.ProxyRule = $state(null); +let selectedProxyRule: Models.ProxyRule | null = $state(null);
🧹 Nitpick comments (2)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/table.svelte (2)
226-230: Unify ActionMenu divider styles with Sites table.Match padding used in Sites variant for consistent spacing.
.action-menu-divider { margin-inline: -1rem; + padding-block-start: 0.25rem; + padding-block-end: 0.25rem; }
186-186: Nit: prefer consistent prop style for DnsRecordsAction.Either use shorthand
{rule}or explicitrule={proxyRule}across files for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/table.svelte(7 hunks)src/routes/(console)/project-[region]-[project]/settings/domains/table.svelte(8 hunks)src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/table.svelte(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/routes/(console)/project-[region]-[project]/settings/domains/table.svelte
⏰ 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). (6)
- GitHub Check: publish-cloud-no-regions
- GitHub Check: publish-self-hosted
- GitHub Check: publish-cloud-stage
- GitHub Check: publish-cloud
- GitHub Check: build
- GitHub Check: e2e
🔇 Additional comments (3)
src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/table.svelte (2)
130-146: LGTM: updated column with short relative time.Good conditional rendering and concise timestamp via
timeFromNowShort.
76-92: Confirm backend/UX semantics for domain status → badge copyFrontend consistently maps: created → "Verification failed"; verifying → "Generating certificate"; unverified → "Certificate generation failed". Verify that the backend status meanings and intended product/UX copy match this mapping.
Locations to check:
- src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/table.svelte (≈ lines 76–116)
- src/routes/(console)/project-[region]-[project]/settings/domains/table.svelte (≈ lines 82–121)
- src/lib/components/domains/cnameTable.svelte (≈ lines 27–36), nameserverTable.svelte (≈ lines 20–29), recordTable.svelte (≈ lines 54–63)
- src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/table.svelte (≈ lines 75–114)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/table.svelte (1)
130-146: LGTM: updated column withtimeFromNowShort.
...utes/(console)/project-[region]-[project]/functions/function-[function]/domains/table.svelte
Show resolved
Hide resolved
...utes/(console)/project-[region]-[project]/functions/function-[function]/domains/table.svelte
Outdated
Show resolved
Hide resolved
src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/table.svelte
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/lib/helpers/tlds.ts (1)
7-8: Small robustness/DRY tweak: normalize input and reuse parse options.
- Trim and strip a trailing dot before parsing to handle inputs like
example.com..- Hoist parse options to avoid repeating literals.
Apply this diff and add the helper just below imports:
-import { parse } from 'tldts'; +import { parse } from 'tldts'; +const parseOpts = { allowPrivateDomains: true } as const; + +function normalizeDomain(input: string | null | undefined): string { + return (input ?? '').trim().replace(/\.$/, ''); +}Then update calls:
-export function getApexDomain(domain: string): string | null { - return parse(domain, { allowPrivateDomains: true }).domain; +export function getApexDomain(domain: string): string | null { + return parse(normalizeDomain(domain), parseOpts).domain; } @@ - const { domain: apex, subdomain } = parse(domain, { allowPrivateDomains: true }); + const { domain: apex, subdomain } = parse(normalizeDomain(domain), parseOpts); @@ -export function getSubdomain(domain: string): string { - if (!domain) return ''; - - return parse(domain, { allowPrivateDomains: true }).subdomain || ''; +export function getSubdomain(domain: string): string { + if (!domain) return ''; + return parse(normalizeDomain(domain), parseOpts).subdomain || ''; }Also applies to: 16-20, 25-29
src/lib/components/domains/cnameTable.svelte (2)
19-20: Make subdomain reactive to domain changes.As written,
subdomainwon’t update ifdomainprop changes after mount.Apply this diff:
- let subdomain = getSubdomain(domain); + let subdomain: string; + $: subdomain = getSubdomain(domain);
64-82: Guard CAA string handling (minor).
includes(' ')assumes a string. It’s wrapped in an{#if _APP_DOMAIN_TARGET_CAA}, but if the store ever returns a non-string (e.g., boolean), this will throw. Coerce to string before using.Apply this diff:
- text={$regionalConsoleVariables._APP_DOMAIN_TARGET_CAA.includes(' ') - ? $regionalConsoleVariables._APP_DOMAIN_TARGET_CAA - : `0 issue "${$regionalConsoleVariables._APP_DOMAIN_TARGET_CAA}"`} /> + text={(String($regionalConsoleVariables._APP_DOMAIN_TARGET_CAA)).includes(' ') + ? String($regionalConsoleVariables._APP_DOMAIN_TARGET_CAA) + : `0 issue "${String($regionalConsoleVariables._APP_DOMAIN_TARGET_CAA)}"`} />src/lib/components/domains/recordTable.svelte (3)
23-24: Make subdomain reactive to domain changes.Same issue as in CNAME table; ensure updates on prop change.
Apply this diff:
- let subdomain = getSubdomain(domain); + let subdomain: string; + $: subdomain = getSubdomain(domain);
87-105: Guard CAA string handling (minor).Mirror the CNAME table fix to avoid potential runtime errors if the store value isn’t a string.
Apply this diff:
- <InteractiveText + <InteractiveText variant="copy" isVisible - text={$regionalConsoleVariables._APP_DOMAIN_TARGET_CAA.includes(' ') - ? $regionalConsoleVariables._APP_DOMAIN_TARGET_CAA - : `0 issue "${$regionalConsoleVariables._APP_DOMAIN_TARGET_CAA}"`} /> + text={(String($regionalConsoleVariables._APP_DOMAIN_TARGET_CAA)).includes(' ') + ? String($regionalConsoleVariables._APP_DOMAIN_TARGET_CAA) + : `0 issue "${String($regionalConsoleVariables._APP_DOMAIN_TARGET_CAA)}"`} />
108-130: Nice UX for apex guidance; consider extracting shared badge/status view.This banner logic is solid. Given similar status/badge blocks across domain components, consider a tiny helper to centralize status→badge text/type and re-use it to avoid drift.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/lib/components/domains/cnameTable.svelte(4 hunks)src/lib/components/domains/recordTable.svelte(3 hunks)src/lib/helpers/tlds.ts(2 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). (2)
- GitHub Check: e2e
- GitHub Check: build
🔇 Additional comments (3)
src/lib/helpers/tlds.ts (1)
7-8: Private-domain awareness looks correct (LGTM).Using
allowPrivateDomains: truealigns apex/subdomain detection with PSL private suffixes and matches the new UI flows.Also applies to: 16-20
src/lib/components/domains/cnameTable.svelte (1)
28-41: Confirm status mapping for ruleStatus.Mapping
created → "Verification failed"andunverified → "Certificate generation failed"is non‑obvious. Verify these align with backend rule statuses to avoid misleading badges.src/lib/components/domains/recordTable.svelte (1)
54-66: Confirm status mapping for ruleStatus.Same mapping concern as CNAME table; ensure wording matches actual backend states.
| {#if ruleStatus === 'created'} | ||
| <Badge variant="secondary" type="error" size="xs" content="Verification failed" /> | ||
| {:else if ruleStatus === 'verifying'} | ||
| <Badge variant="secondary" size="xs" content="Generating certificate" /> |
There was a problem hiding this comment.
do we not use a warning [orange] variant in similar places for different states? 🤔
There was a problem hiding this comment.
Not as per new design, only in org > domains table
| </Typography.Text> | ||
| {#if verified === true} | ||
| {#if ruleStatus === 'created'} | ||
| <Badge variant="secondary" type="error" size="xs" content="Verification failed" /> |
There was a problem hiding this comment.
is this correct? does ruleStatus === 'created mean it failed verification? is there no other status on rule?
There was a problem hiding this comment.
As per backend, created = verification failed, verifying = 'generating certificate', unverified = 'certificate generation failed', verified = verified
| {#if ruleStatus === 'created'} | ||
| <Badge variant="secondary" type="error" size="xs" content="Verification failed" /> | ||
| {:else if ruleStatus === 'verifying'} | ||
| <Badge variant="secondary" size="xs" content="Generating certificate" /> |
...console)/project-[region]-[project]/settings/domains/add-domain/verify-[domain]/+page.svelte
Show resolved
Hide resolved
| const domainData = await sdk.forConsole.domains.create({ | ||
| teamId: $organization.$id, | ||
| domain: apexDomain | ||
| }); |
There was a problem hiding this comment.
can we group this and the below call into Promise.all if they both are independent? would save some time to go sequentially.
There was a problem hiding this comment.
This has been rewritten to not use await and happen in background
src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/table.svelte
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/add-domain/verify-[domain]/+page.svelte (3)
35-44: Align default tab selection with tab visibility gatesDefault selection can point to a tab that’s hidden (e.g., localhost/127.0.0.1/::1, or self‑host where Nameservers tab is hidden). Make selection logic consistent with render conditions and avoid selecting Nameservers on self‑host.
- if ($regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME) { + if ( + $regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME && + $regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME !== 'localhost' + ) { selectedTab = 'cname'; - } else if (!isCloud && $regionalConsoleVariables._APP_DOMAIN_TARGET_A) { + } else if ( + !isCloud && + $regionalConsoleVariables._APP_DOMAIN_TARGET_A && + $regionalConsoleVariables._APP_DOMAIN_TARGET_A !== '127.0.0.1' + ) { selectedTab = 'a'; - } else if (!isCloud && $regionalConsoleVariables._APP_DOMAIN_TARGET_AAAA) { + } else if ( + !isCloud && + $regionalConsoleVariables._APP_DOMAIN_TARGET_AAAA && + $regionalConsoleVariables._APP_DOMAIN_TARGET_AAAA !== '::1' + ) { selectedTab = 'aaaa'; } else { - selectedTab = 'nameserver'; + selectedTab = isCloud ? 'nameserver' : 'a'; }
52-68: Optional: clarify “fire‑and‑forget” intentThese promises are intentionally not awaited; consider prefixing with void for linter clarity and to signal intent. Also guard nameservers on potential null.
- sdk.forConsole.domains + void sdk.forConsole.domains .create({ teamId: $organization.$id, domain: apexDomain }) .then((domainData) => { - if (domainData.nameservers.toLowerCase() === 'appwrite') { + if (domainData.nameservers?.toLowerCase() === 'appwrite') { verified = true; } })
123-124: URL‑encode domain in navigationAvoid malformed URLs for domains with special characters.
- await goto(`${routeBase}/add-domain?domain=${data.proxyRule.domain}`); + await goto(`${routeBase}/add-domain?domain=${encodeURIComponent(data.proxyRule.domain)}`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/add-domain/+page.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/add-domain/verify-[domain]/+page.svelte(7 hunks)src/routes/(console)/project-[region]-[project]/settings/domains/add-domain/+page.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/settings/domains/add-domain/verify-[domain]/+page.svelte(7 hunks)src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/add-domain/+page.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/add-domain/verify-[domain]/+page.svelte(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/add-domain/+page.svelte
🔇 Additional comments (13)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/add-domain/verify-[domain]/+page.svelte (6)
15-15: LGTM: Type import for ModelsType import is appropriate and used below for Models.Domain.
24-24: LGTM: Import getApexDomainImport aligns with usage in verify().
140-141: LGTM: Use proxyRule.domain for displayConsistent with the rest of the flow.
151-166: LGTM: Tab visibility guardsConditions correctly hide tabs for localhost/127.0.0.1/::1 and self‑host/cloud.
Double‑check that the default selection logic (Lines 35‑44) is updated per earlier suggestion to avoid selecting a hidden tab.
Also applies to: 167-182
187-191: LGTM: Propagate ruleStatus and domain to tablesMatches the updated component interfaces in this PR.
Ensure nameserverTable.svelte and recordTable.svelte accept ruleStatus as a prop and handle undefined verified.
Also applies to: 192-200
89-91: Guard missing ruleId before calling updateRuleVerification- const ruleData = await sdk + if (!ruleId) { + throw new Error('Missing rule id. Please restart the verification flow.'); + } + const ruleData = await sdk .forProject(page.params.region, page.params.project) .proxy.updateRuleVerification({ ruleId }); verified = ruleData.status === 'verified'; if (!verified) { throw new Error( 'Domain verification failed. Please check your domain settings or try again later' ); }Also applies to: 93-97
src/routes/(console)/project-[region]-[project]/settings/domains/add-domain/verify-[domain]/+page.svelte (2)
55-55: Remove debug log.Leftover
console.logshould be dropped.Apply:
- console.log('apexDomain', apexDomain);
91-94: GuardruleIdbefore verification.Prevents SDK call with null/undefined.
Apply:
- const ruleData = await sdk + if (!ruleId) { + throw new Error('Missing rule id. Please restart the verification flow.'); + } + const ruleData = await sdk .forProject(page.params.region, page.params.project) .proxy.updateRuleVerification({ ruleId });src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/add-domain/+page.svelte (2)
60-75: Avoid empty catch; await domain creation and handle duplicates explicitly.The non-awaited create with a comment-only catch both hides failures and can trip the linter (“empty block”). Also, without awaiting,
domainmay still be undefined when you later try to use it, causing the nameserver update to be skipped due to a race.Apply:
- let domain: Models.Domain; + let domain: Models.Domain | undefined; @@ - if (isCloud && apexDomain && !isSiteDomain) { - sdk.forConsole.domains - .create({ - teamId: $project.teamId, - domain: apexDomain - }) - .then((createdDomain) => { - domain = createdDomain; - }) - .catch(() => { - // Empty as domain creation error needs to be silent - }); - } + if (isCloud && apexDomain && !isSiteDomain) { + try { + domain = await sdk.forConsole.domains.create({ + teamId: $project.teamId, + domain: apexDomain + }); + } catch (error: any) { + // Ignore known duplicate, surface anything else. + if (error?.type !== 'domain_already_exists') { + addNotification({ + type: 'error', + message: error?.message ?? 'Failed to create apex domain' + }); + return; + } + } + }
108-112: Guarddomain.$idand don’t swallow update errors.Avoid comment-only catch; also ensure
$idexists.Apply:
- if (isCloud && domain) { - sdk.forConsole.domains.updateNameservers({ domainId: domain.$id }).catch(() => { - // Empty as domain update error needs to be silent - }); - } + if (isCloud && domain?.$id) { + sdk.forConsole.domains + .updateNameservers({ domainId: domain.$id }) + .catch((error: any) => { + addNotification({ + type: 'warning', + message: + 'Failed to update nameservers for apex domain. You can retry from Domains → View logs.' + }); + }); + }src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/add-domain/verify-[domain]/+page.svelte (1)
91-96: Guard missingruleIdbefore callingupdateRuleVerification.Fail fast with a clear message to avoid SDK errors.
Apply:
- const ruleData = await sdk + if (!ruleId) { + throw new Error('Missing rule id. Please restart the verification flow.'); + } + const ruleData = await sdk .forProject(page.params.region, page.params.project) .proxy.updateRuleVerification({ ruleId });src/routes/(console)/project-[region]-[project]/settings/domains/add-domain/+page.svelte (2)
54-57: Guarddomain?.$idand surface failures in catch.Apply:
- if (isCloud && domain) { - sdk.forConsole.domains.updateNameservers({ domainId: domain.$id }).catch(() => { - // Empty as domain update error needs to be silent - }); - } + if (isCloud && domain?.$id) { + sdk.forConsole.domains + .updateNameservers({ domainId: domain.$id }) + .catch((error: any) => { + addNotification({ + type: 'warning', + message: + 'Failed to update nameservers for apex domain. You can retry from Domains → View logs.' + }); + }); + }
31-45: Same issue: empty catch and race ondomain; await and handle errors.The comment-only catch will fail lint; also without awaiting,
domainmay not be set.Apply:
- let domain: Models.Domain; + let domain: Models.Domain | undefined; @@ - if (isCloud && apexDomain) { - sdk.forConsole.domains - .create({ - teamId: $project.teamId, - domain: apexDomain - }) - .then((createdDomain) => { - domain = createdDomain; - }) - .catch(() => { - // Empty as domain creation error needs to be silent - }); - } + if (isCloud && apexDomain) { + try { + domain = await sdk.forConsole.domains.create({ + teamId: $project.teamId, + domain: apexDomain + }); + } catch (error: any) { + if (error?.type !== 'domain_already_exists') { + addNotification({ + type: 'error', + message: error?.message ?? 'Failed to create apex domain' + }); + return; + } + } + }
| const domain = data.domainsList.domains.find( | ||
| (d: Models.Domain) => d.domain === apexDomain | ||
| ); | ||
| if (domain) { | ||
| sdk.forConsole.domains | ||
| .updateNameservers({ | ||
| domainId: domain.$id | ||
| }) | ||
| .then((output) => { | ||
| if (output.nameservers.toLowerCase() === 'appwrite') { | ||
| verified = true; | ||
| } | ||
| }) | ||
| .catch(() => { | ||
| // Empty as domain update error needs to be silent | ||
| }); | ||
| } |
There was a problem hiding this comment.
Make domains list access null‑safe
data.domainsList may be absent or fail to load; direct access can throw at runtime.
- const domain = data.domainsList.domains.find(
- (d: Models.Domain) => d.domain === apexDomain
- );
+ const domain = data.domainsList?.domains?.find(
+ (d: Models.Domain) => d.domain === apexDomain
+ );
if (domain) {
sdk.forConsole.domains
.updateNameservers({
domainId: domain.$id
})
.then((output) => {
- if (output.nameservers.toLowerCase() === 'appwrite') {
+ if (output.nameservers?.toLowerCase() === 'appwrite') {
verified = true;
}
})📝 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.
| const domain = data.domainsList.domains.find( | |
| (d: Models.Domain) => d.domain === apexDomain | |
| ); | |
| if (domain) { | |
| sdk.forConsole.domains | |
| .updateNameservers({ | |
| domainId: domain.$id | |
| }) | |
| .then((output) => { | |
| if (output.nameservers.toLowerCase() === 'appwrite') { | |
| verified = true; | |
| } | |
| }) | |
| .catch(() => { | |
| // Empty as domain update error needs to be silent | |
| }); | |
| } | |
| const domain = data.domainsList?.domains?.find( | |
| (d: Models.Domain) => d.domain === apexDomain | |
| ); | |
| if (domain) { | |
| sdk.forConsole.domains | |
| .updateNameservers({ | |
| domainId: domain.$id | |
| }) | |
| .then((output) => { | |
| if (output.nameservers?.toLowerCase() === 'appwrite') { | |
| verified = true; | |
| } | |
| }) | |
| .catch(() => { | |
| // Empty as domain update error needs to be silent | |
| }); | |
| } |
🤖 Prompt for AI Agents
In
src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/add-domain/verify-[domain]/+page.svelte
around lines 69 to 85, the code directly accesses data.domainsList.domains which
can be undefined and cause a runtime crash; make this null-safe by first
guarding that data and data.domainsList exist and using optional chaining or a
default empty array when calling .find (e.g. const domains =
data?.domainsList?.domains || []; const domain = domains.find(...)) and if no
domain is found simply skip the updateNameservers call (or return early) so
updateNameservers is only invoked when domain is present.
...console)/project-[region]-[project]/settings/domains/add-domain/verify-[domain]/+page.svelte
Show resolved
Hide resolved
...project-[region]-[project]/sites/site-[site]/domains/add-domain/verify-[domain]/+page.svelte
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (7)
src/lib/components/domains/nameserverTable.svelte (2)
6-14: Narrow ruleStatus type to prevent typos and enable exhaustiveness.Use a string union for known statuses instead of generic string.
- }: { - domain: string; - verified?: boolean | undefined; - ruleStatus?: string | undefined; - } = $props(); + }: { + domain: string; + verified?: boolean; + ruleStatus?: 'created' | 'verifying' | 'unverified' | 'verified'; + } = $props();
16-19: Make nameserverList reactive and guard store access.If regionalConsoleVariables resolves later, current const can miss updates and can throw if undefined. Derive reactively with optional chaining.
- const nameserverList = $regionalConsoleVariables?._APP_DOMAINS_NAMESERVERS - ? $regionalConsoleVariables?._APP_DOMAINS_NAMESERVERS?.split(',') - : ['ns1.appwrite.io', 'ns2.appwrite.io']; + const nameserverList = $derived( + $regionalConsoleVariables?._APP_DOMAINS_NAMESERVERS + ? $regionalConsoleVariables._APP_DOMAINS_NAMESERVERS.split(',') + : ['ns1.appwrite.io', 'ns2.appwrite.io'] + );src/lib/components/domains/cnameTable.svelte (1)
15-23: Narrow ruleStatus type.Constrain to known values for safer branching.
- }: { - domain: string; - verified?: boolean | undefined; - ruleStatus?: string | undefined; - } = $props(); + }: { + domain: string; + verified?: boolean; + ruleStatus?: 'created' | 'verifying' | 'unverified' | 'verified'; + } = $props();src/lib/components/domains/recordTable.svelte (4)
15-33: Narrow ruleStatus type and keep handler props optional.Constrain to known statuses and simplify optional boolean typing.
- }: { - domain: string; - verified?: boolean | undefined; - variant: 'cname' | 'a' | 'aaaa'; - service?: 'sites' | 'general'; - ruleStatus?: string | undefined; - onNavigateToNameservers?: () => void; - onNavigateToA?: () => void; - onNavigateToAAAA?: () => void; - } = $props(); + }: { + domain: string; + verified?: boolean; + variant: 'cname' | 'a' | 'aaaa'; + service?: 'sites' | 'general'; + ruleStatus?: 'created' | 'verifying' | 'unverified' | 'verified'; + onNavigateToNameservers?: () => void; + onNavigateToA?: () => void; + onNavigateToAAAA?: () => void; + } = $props();
48-53: Guard CAA text derivation.Mirror the safety applied in cnameTable to avoid undefined access.
- const caaText = $derived( - $regionalConsoleVariables._APP_DOMAIN_TARGET_CAA?.includes(' ') - ? $regionalConsoleVariables._APP_DOMAIN_TARGET_CAA - : `0 issue "${$regionalConsoleVariables._APP_DOMAIN_TARGET_CAA}"` - ); + const caaText = $derived( + $regionalConsoleVariables?._APP_DOMAIN_TARGET_CAA?.includes(' ') + ? $regionalConsoleVariables?._APP_DOMAIN_TARGET_CAA ?? '' + : $regionalConsoleVariables?._APP_DOMAIN_TARGET_CAA + ? `0 issue "${$regionalConsoleVariables._APP_DOMAIN_TARGET_CAA}"` + : '' + );
107-120: Guard CAA conditional in markup.Use optional chaining for store value presence.
-{#if $regionalConsoleVariables._APP_DOMAIN_TARGET_CAA} +{#if $regionalConsoleVariables?._APP_DOMAIN_TARGET_CAA}
54-65: Minor: setTarget default case for completeness.Add a default return to avoid implicit undefined and help future extensions.
function setTarget() { switch (variant) { case 'cname': return service === 'general' ? $regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME : $regionalConsoleVariables._APP_DOMAIN_SITES; case 'a': return $regionalConsoleVariables._APP_DOMAIN_TARGET_A; case 'aaaa': return $regionalConsoleVariables._APP_DOMAIN_TARGET_AAAA; + default: + return ''; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/lib/components/domains/cnameTable.svelte(4 hunks)src/lib/components/domains/nameserverTable.svelte(2 hunks)src/lib/components/domains/recordTable.svelte(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-31T05:32:42.285Z
Learnt from: ItzNotABug
PR: appwrite/console#2284
File: src/lib/components/searchQuery.svelte:26-27
Timestamp: 2025-08-31T05:32:42.285Z
Learning: SvelteKit automatically enables Svelte 5 runes without requiring explicit "use runes" directive or runes: true configuration when using Svelte 5.
Applied to files:
src/lib/components/domains/nameserverTable.sveltesrc/lib/components/domains/cnameTable.svelte
📚 Learning: 2025-09-25T04:31:05.213Z
Learnt from: ItzNotABug
PR: appwrite/console#2373
File: src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/options.svelte:33-42
Timestamp: 2025-09-25T04:31:05.213Z
Learning: Svelte 5 uses `onclick` instead of `on:click` for event handlers. Event handlers are now properties like any other rather than directives that require the `on:` prefix.
Applied to files:
src/lib/components/domains/nameserverTable.svelte
📚 Learning: 2025-09-25T04:31:05.213Z
Learnt from: ItzNotABug
PR: appwrite/console#2373
File: src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/options.svelte:33-42
Timestamp: 2025-09-25T04:31:05.213Z
Learning: In Svelte 5, the `on:` directive syntax (like `on:click`) has been replaced with direct property syntax (like `onclick`). This is part of the migration from Svelte 4 to Svelte 5 where event handlers are treated as regular properties.
Applied to files:
src/lib/components/domains/cnameTable.svelte
⏰ 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). (2)
- GitHub Check: e2e
- GitHub Check: build
🔇 Additional comments (1)
src/lib/components/domains/cnameTable.svelte (1)
68-69: Good: apex handling for CNAME “Name” cell.Showing “@” for apex domains improves DNS clarity.
| const subdomain = $derived(getSubdomain(domain)); | ||
| const caaText = $derived( | ||
| $regionalConsoleVariables._APP_DOMAIN_TARGET_CAA?.includes(' ') | ||
| ? $regionalConsoleVariables._APP_DOMAIN_TARGET_CAA | ||
| : `0 issue "${$regionalConsoleVariables._APP_DOMAIN_TARGET_CAA}"` | ||
| ); |
There was a problem hiding this comment.
Prevent runtime errors if regionalConsoleVariables is undefined.
Accessing properties on an undefined store value will throw. Guard with optional chaining and provide a safe fallback.
- const caaText = $derived(
- $regionalConsoleVariables._APP_DOMAIN_TARGET_CAA?.includes(' ')
- ? $regionalConsoleVariables._APP_DOMAIN_TARGET_CAA
- : `0 issue "${$regionalConsoleVariables._APP_DOMAIN_TARGET_CAA}"`
- );
+ const caaText = $derived(
+ $regionalConsoleVariables?._APP_DOMAIN_TARGET_CAA?.includes(' ')
+ ? $regionalConsoleVariables?._APP_DOMAIN_TARGET_CAA ?? ''
+ : $regionalConsoleVariables?._APP_DOMAIN_TARGET_CAA
+ ? `0 issue "${$regionalConsoleVariables._APP_DOMAIN_TARGET_CAA}"`
+ : ''
+ );📝 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.
| const subdomain = $derived(getSubdomain(domain)); | |
| const caaText = $derived( | |
| $regionalConsoleVariables._APP_DOMAIN_TARGET_CAA?.includes(' ') | |
| ? $regionalConsoleVariables._APP_DOMAIN_TARGET_CAA | |
| : `0 issue "${$regionalConsoleVariables._APP_DOMAIN_TARGET_CAA}"` | |
| ); | |
| const subdomain = $derived(getSubdomain(domain)); | |
| const caaText = $derived( | |
| $regionalConsoleVariables?._APP_DOMAIN_TARGET_CAA?.includes(' ') | |
| ? $regionalConsoleVariables?._APP_DOMAIN_TARGET_CAA ?? '' | |
| : $regionalConsoleVariables?._APP_DOMAIN_TARGET_CAA | |
| ? `0 issue "${$regionalConsoleVariables._APP_DOMAIN_TARGET_CAA}"` | |
| : '' | |
| ); |
🤖 Prompt for AI Agents
In src/lib/components/domains/cnameTable.svelte around lines 25 to 31, the code
reads properties from $regionalConsoleVariables directly which can be undefined
and cause runtime errors; update the expression to use optional chaining and a
safe fallback (e.g., treat undefined as an empty string) when calling includes
and when interpolating the value so the derived store always receives a string;
ensure the conditional becomes something like checking
($regionalConsoleVariables?._APP_DOMAIN_TARGET_CAA ?? '') and fallback to a
default CAA string when empty so no property access occurs on undefined.
| {#if ruleStatus === 'created'} | ||
| <Badge variant="secondary" type="error" size="xs" content="Verification failed" /> | ||
| {:else if ruleStatus === 'verifying'} | ||
| <Badge variant="secondary" size="xs" content="Generating certificate" /> | ||
| {:else if ruleStatus === 'unverified'} | ||
| <Badge | ||
| variant="secondary" | ||
| type="error" | ||
| size="xs" | ||
| content="Certificate generation failed" /> | ||
| {:else if verified === true} | ||
| <Badge variant="secondary" type="success" size="xs" content="Verified" /> |
There was a problem hiding this comment.
Handle ruleStatus === 'verified' explicitly.
Show the success badge when status is verified even if verified flag isn’t set.
- {:else if ruleStatus === 'unverified'}
+ {:else if ruleStatus === 'unverified'}
<Badge
variant="secondary"
type="error"
size="xs"
content="Certificate generation failed" />
- {:else if verified === true}
+ {:else if ruleStatus === 'verified' || verified === true}
<Badge variant="secondary" type="success" size="xs" content="Verified" />📝 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.
| {#if ruleStatus === 'created'} | |
| <Badge variant="secondary" type="error" size="xs" content="Verification failed" /> | |
| {:else if ruleStatus === 'verifying'} | |
| <Badge variant="secondary" size="xs" content="Generating certificate" /> | |
| {:else if ruleStatus === 'unverified'} | |
| <Badge | |
| variant="secondary" | |
| type="error" | |
| size="xs" | |
| content="Certificate generation failed" /> | |
| {:else if verified === true} | |
| <Badge variant="secondary" type="success" size="xs" content="Verified" /> | |
| {#if ruleStatus === 'created'} | |
| <Badge variant="secondary" type="error" size="xs" content="Verification failed" /> | |
| {:else if ruleStatus === 'verifying'} | |
| <Badge variant="secondary" size="xs" content="Generating certificate" /> | |
| {:else if ruleStatus === 'unverified'} | |
| <Badge | |
| variant="secondary" | |
| type="error" | |
| size="xs" | |
| content="Certificate generation failed" /> | |
| {:else if ruleStatus === 'verified' || verified === true} | |
| <Badge variant="secondary" type="success" size="xs" content="Verified" /> |
🤖 Prompt for AI Agents
In src/lib/components/domains/cnameTable.svelte around lines 40 to 51, the
current conditional omits handling for ruleStatus === 'verified' and only shows
the success badge when verified === true; update the conditional flow to
explicitly check for ruleStatus === 'verified' (render the success Badge) before
the final verified === true fallback so that a 'verified' status always shows
the success badge regardless of the verified flag, keeping the existing other
branches intact.
| {#if $regionalConsoleVariables._APP_DOMAIN_TARGET_CAA} | ||
| <Table.Row.Base {root}> | ||
| <Table.Cell {root}> | ||
| <Layout.Stack gap="s" direction="row" alignItems="center"> | ||
| <span>CAA</span> | ||
| <Badge variant="secondary" size="xs" content="Recommended" /> | ||
| </Layout.Stack> | ||
| </Table.Cell> | ||
| <Table.Cell {root}>@</Table.Cell> | ||
| <Table.Cell {root}> | ||
| <InteractiveText variant="copy" isVisible text={caaText} /> | ||
| </Table.Cell> | ||
| </Table.Row.Base> | ||
| {/if} |
There was a problem hiding this comment.
Guard CAA conditional and keep value consistent with the guard.
Use optional chaining in the {#if} and rely on the already-guarded caaText.
- {#if $regionalConsoleVariables._APP_DOMAIN_TARGET_CAA}
+ {#if $regionalConsoleVariables?._APP_DOMAIN_TARGET_CAA}
<Table.Row.Base {root}>
<Table.Cell {root}>
<Layout.Stack gap="s" direction="row" alignItems="center">
<span>CAA</span>
<Badge variant="secondary" size="xs" content="Recommended" />
</Layout.Stack>
</Table.Cell>
<Table.Cell {root}>@</Table.Cell>
<Table.Cell {root}>
<InteractiveText variant="copy" isVisible text={caaText} />
</Table.Cell>
</Table.Row.Base>
{/if}🤖 Prompt for AI Agents
In src/lib/components/domains/cnameTable.svelte around lines 76-89, the CAA
conditional should use optional chaining and the rendered value should rely on
the already-guarded caaText; update the {#if} to check
$regionalConsoleVariables?._APP_DOMAIN_TARGET_CAA (optional chaining) and ensure
the Table.Cell passes caaText (not re-reading the store) to InteractiveText so
the displayed value matches the guard.
| {#if ruleStatus === 'created'} | ||
| <Badge variant="secondary" type="error" size="xs" content="Verification failed" /> | ||
| {:else if ruleStatus === 'verifying'} | ||
| <Badge variant="secondary" size="xs" content="Generating certificate" /> | ||
| {:else if ruleStatus === 'unverified'} | ||
| <Badge | ||
| variant="secondary" | ||
| type="error" | ||
| size="xs" | ||
| content="Certificate generation failed" /> | ||
| {:else if verified === true} | ||
| <Badge variant="secondary" type="success" size="xs" content="Verified" /> |
There was a problem hiding this comment.
Handle ruleStatus === 'verified' explicitly.
If callers pass only ruleStatus without toggling verified=true, the "Verified" badge won’t render. Add a branch for ruleStatus === 'verified'.
- {:else if ruleStatus === 'unverified'}
+ {:else if ruleStatus === 'unverified'}
<Badge
variant="secondary"
type="error"
size="xs"
content="Certificate generation failed" />
- {:else if verified === true}
+ {:else if ruleStatus === 'verified' || verified === true}
<Badge variant="secondary" type="success" size="xs" content="Verified" />📝 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.
| {#if ruleStatus === 'created'} | |
| <Badge variant="secondary" type="error" size="xs" content="Verification failed" /> | |
| {:else if ruleStatus === 'verifying'} | |
| <Badge variant="secondary" size="xs" content="Generating certificate" /> | |
| {:else if ruleStatus === 'unverified'} | |
| <Badge | |
| variant="secondary" | |
| type="error" | |
| size="xs" | |
| content="Certificate generation failed" /> | |
| {:else if verified === true} | |
| <Badge variant="secondary" type="success" size="xs" content="Verified" /> | |
| {#if ruleStatus === 'created'} | |
| <Badge variant="secondary" type="error" size="xs" content="Verification failed" /> | |
| {:else if ruleStatus === 'verifying'} | |
| <Badge variant="secondary" size="xs" content="Generating certificate" /> | |
| {:else if ruleStatus === 'unverified'} | |
| <Badge | |
| variant="secondary" | |
| type="error" | |
| size="xs" | |
| content="Certificate generation failed" /> | |
| {:else if ruleStatus === 'verified' || verified === true} | |
| <Badge variant="secondary" type="success" size="xs" content="Verified" /> | |
| {/if} |
🤖 Prompt for AI Agents
In src/lib/components/domains/nameserverTable.svelte around lines 26 to 37, the
template lacks an explicit branch for ruleStatus === 'verified' so if callers
set ruleStatus to 'verified' but don't set verified=true the "Verified" badge
never renders; add an {:else if ruleStatus === 'verified'} branch (before the
final {:else if verified === true} or replace the final check) that renders the
same Badge as the verified=true case (variant="secondary" type="success"
size="xs" content="Verified"), ensuring the verified state is handled
consistently by ruleStatus alone.
| const subdomain = $derived(getSubdomain(domain)); | ||
| const aTabVisible = $derived( | ||
| !isCloud && | ||
| Boolean($regionalConsoleVariables._APP_DOMAIN_TARGET_A) && | ||
| $regionalConsoleVariables._APP_DOMAIN_TARGET_A !== '127.0.0.1' | ||
| ); | ||
| const aaaaTabVisible = $derived( | ||
| !isCloud && | ||
| Boolean($regionalConsoleVariables._APP_DOMAIN_TARGET_AAAA) && | ||
| $regionalConsoleVariables._APP_DOMAIN_TARGET_AAAA !== '::1' | ||
| ); |
There was a problem hiding this comment.
Guard store access with optional chaining.
Avoid potential runtime errors if regionalConsoleVariables is not yet available.
- const subdomain = $derived(getSubdomain(domain));
+ const subdomain = $derived(getSubdomain(domain));
- const aTabVisible = $derived(
- !isCloud &&
- Boolean($regionalConsoleVariables._APP_DOMAIN_TARGET_A) &&
- $regionalConsoleVariables._APP_DOMAIN_TARGET_A !== '127.0.0.1'
- );
- const aaaaTabVisible = $derived(
- !isCloud &&
- Boolean($regionalConsoleVariables._APP_DOMAIN_TARGET_AAAA) &&
- $regionalConsoleVariables._APP_DOMAIN_TARGET_AAAA !== '::1'
- );
+ const aTabVisible = $derived(
+ !isCloud &&
+ Boolean($regionalConsoleVariables?._APP_DOMAIN_TARGET_A) &&
+ $regionalConsoleVariables?._APP_DOMAIN_TARGET_A !== '127.0.0.1'
+ );
+ const aaaaTabVisible = $derived(
+ !isCloud &&
+ Boolean($regionalConsoleVariables?._APP_DOMAIN_TARGET_AAAA) &&
+ $regionalConsoleVariables?._APP_DOMAIN_TARGET_AAAA !== '::1'
+ );📝 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.
| const subdomain = $derived(getSubdomain(domain)); | |
| const aTabVisible = $derived( | |
| !isCloud && | |
| Boolean($regionalConsoleVariables._APP_DOMAIN_TARGET_A) && | |
| $regionalConsoleVariables._APP_DOMAIN_TARGET_A !== '127.0.0.1' | |
| ); | |
| const aaaaTabVisible = $derived( | |
| !isCloud && | |
| Boolean($regionalConsoleVariables._APP_DOMAIN_TARGET_AAAA) && | |
| $regionalConsoleVariables._APP_DOMAIN_TARGET_AAAA !== '::1' | |
| ); | |
| const subdomain = $derived(getSubdomain(domain)); | |
| const aTabVisible = $derived( | |
| !isCloud && | |
| Boolean($regionalConsoleVariables?._APP_DOMAIN_TARGET_A) && | |
| $regionalConsoleVariables?._APP_DOMAIN_TARGET_A !== '127.0.0.1' | |
| ); | |
| const aaaaTabVisible = $derived( | |
| !isCloud && | |
| Boolean($regionalConsoleVariables?._APP_DOMAIN_TARGET_AAAA) && | |
| $regionalConsoleVariables?._APP_DOMAIN_TARGET_AAAA !== '::1' | |
| ); |
🤖 Prompt for AI Agents
In src/lib/components/domains/recordTable.svelte around lines 35 to 46, accesses
to regionalConsoleVariables are unguarded and may throw if the store value is
undefined; update the expressions to use optional chaining (e.g.,
$regionalConsoleVariables?._APP_DOMAIN_TARGET_A and
$regionalConsoleVariables?._APP_DOMAIN_TARGET_AAAA) and ensure Boolean checks
handle undefined safely so the derived values compute only when
regionalConsoleVariables is present.
| {#if ruleStatus === 'created'} | ||
| <Badge variant="secondary" type="error" size="xs" content="Verification failed" /> | ||
| {:else if ruleStatus === 'verifying'} | ||
| <Badge variant="secondary" size="xs" content="Generating certificate" /> | ||
| {:else if ruleStatus === 'unverified'} | ||
| <Badge | ||
| variant="secondary" | ||
| type="error" | ||
| size="xs" | ||
| content="Certificate generation failed" /> | ||
| {:else if verified === true} | ||
| <Badge variant="secondary" type="success" size="xs" content="Verified" /> | ||
| {:else if verified === false} | ||
| <Badge variant="secondary" type="warning" size="xs" content="Verification failed" /> | ||
| {/if} |
There was a problem hiding this comment.
Handle ruleStatus === 'verified'.
Render success badge when status is verified even if verified flag isn’t true.
- {:else if ruleStatus === 'unverified'}
+ {:else if ruleStatus === 'unverified'}
<Badge
variant="secondary"
type="error"
size="xs"
content="Certificate generation failed" />
- {:else if verified === true}
+ {:else if ruleStatus === 'verified' || verified === true}
<Badge variant="secondary" type="success" size="xs" content="Verified" />📝 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.
| {#if ruleStatus === 'created'} | |
| <Badge variant="secondary" type="error" size="xs" content="Verification failed" /> | |
| {:else if ruleStatus === 'verifying'} | |
| <Badge variant="secondary" size="xs" content="Generating certificate" /> | |
| {:else if ruleStatus === 'unverified'} | |
| <Badge | |
| variant="secondary" | |
| type="error" | |
| size="xs" | |
| content="Certificate generation failed" /> | |
| {:else if verified === true} | |
| <Badge variant="secondary" type="success" size="xs" content="Verified" /> | |
| {:else if verified === false} | |
| <Badge variant="secondary" type="warning" size="xs" content="Verification failed" /> | |
| {/if} | |
| {#if ruleStatus === 'created'} | |
| <Badge variant="secondary" type="error" size="xs" content="Verification failed" /> | |
| {:else if ruleStatus === 'verifying'} | |
| <Badge variant="secondary" size="xs" content="Generating certificate" /> | |
| {:else if ruleStatus === 'unverified'} | |
| <Badge | |
| variant="secondary" | |
| type="error" | |
| size="xs" | |
| content="Certificate generation failed" /> | |
| {:else if ruleStatus === 'verified' || verified === true} | |
| <Badge variant="secondary" type="success" size="xs" content="Verified" /> | |
| {/if} |
🤖 Prompt for AI Agents
In src/lib/components/domains/recordTable.svelte around lines 74 to 86, the
template currently only renders the "Verified" success badge when the verified
flag is true, but the review requests handling ruleStatus === 'verified' too;
update the conditional so the success badge is shown when ruleStatus ===
'verified' OR verified === true (either by adding an else if branch for
ruleStatus === 'verified' before the final check, or by changing the final
condition to check ruleStatus === 'verified' || verified === true), ensuring the
"Verified" Badge renders for the 'verified' status even if the verified boolean
is false.
| {#if variant === 'cname' && !subdomain} | ||
| {#if isCloud} | ||
| <Alert.Inline> | ||
| Since <Badge variant="secondary" size="s" content={domain} /> is an apex domain, | ||
| CNAME record is only supported by certain providers. If yours doesn't, please verify | ||
| using | ||
| <Link variant="muted" on:click={onNavigateToNameservers}>nameservers</Link> instead. | ||
| </Alert.Inline> | ||
| {:else if aTabVisible || aaaaTabVisible} | ||
| <Alert.Inline> | ||
| Since <Badge variant="secondary" size="s" content={domain} /> is an apex domain, | ||
| CNAME record is only supported by certain providers. If yours doesn't, please verify | ||
| using | ||
| {#if aTabVisible} | ||
| <Link variant="muted" on:click={onNavigateToA}>A record</Link> | ||
| {#if aaaaTabVisible} | ||
| or <Link variant="muted" on:click={onNavigateToAAAA}>AAAA record</Link | ||
| >{/if} | ||
| {:else if aaaaTabVisible} | ||
| <Link variant="muted" on:click={onNavigateToAAAA}>AAAA record</Link> | ||
| {/if} instead. | ||
| </Alert.Inline> | ||
| {/if} |
There was a problem hiding this comment.
Migrate event handlers to Svelte 5 syntax.
Use onclick instead of on:click. This aligns with project-wide Svelte 5 migration. Based on learnings
- <Link variant="muted" on:click={onNavigateToNameservers}>nameservers</Link> instead.
+ <Link variant="muted" onclick={onNavigateToNameservers}>nameservers</Link> instead.
...
- <Link variant="muted" on:click={onNavigateToA}>A record</Link>
+ <Link variant="muted" onclick={onNavigateToA}>A record</Link>
- or <Link variant="muted" on:click={onNavigateToAAAA}>AAAA record</Link
+ or <Link variant="muted" onclick={onNavigateToAAAA}>AAAA record</Link
>{/if}
- {:else if aaaaTabVisible}
- <Link variant="muted" on:click={onNavigateToAAAA}>AAAA record</Link>
+ {:else if aaaaTabVisible}
+ <Link variant="muted" onclick={onNavigateToAAAA}>AAAA record</Link>📝 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.
| {#if variant === 'cname' && !subdomain} | |
| {#if isCloud} | |
| <Alert.Inline> | |
| Since <Badge variant="secondary" size="s" content={domain} /> is an apex domain, | |
| CNAME record is only supported by certain providers. If yours doesn't, please verify | |
| using | |
| <Link variant="muted" on:click={onNavigateToNameservers}>nameservers</Link> instead. | |
| </Alert.Inline> | |
| {:else if aTabVisible || aaaaTabVisible} | |
| <Alert.Inline> | |
| Since <Badge variant="secondary" size="s" content={domain} /> is an apex domain, | |
| CNAME record is only supported by certain providers. If yours doesn't, please verify | |
| using | |
| {#if aTabVisible} | |
| <Link variant="muted" on:click={onNavigateToA}>A record</Link> | |
| {#if aaaaTabVisible} | |
| or <Link variant="muted" on:click={onNavigateToAAAA}>AAAA record</Link | |
| >{/if} | |
| {:else if aaaaTabVisible} | |
| <Link variant="muted" on:click={onNavigateToAAAA}>AAAA record</Link> | |
| {/if} instead. | |
| </Alert.Inline> | |
| {/if} | |
| {#if variant === 'cname' && !subdomain} | |
| {#if isCloud} | |
| <Alert.Inline> | |
| Since <Badge variant="secondary" size="s" content={domain} /> is an apex domain, | |
| CNAME record is only supported by certain providers. If yours doesn't, please verify | |
| using | |
| <Link variant="muted" onclick={onNavigateToNameservers}>nameservers</Link> instead. | |
| </Alert.Inline> | |
| {:else if aTabVisible || aaaaTabVisible} | |
| <Alert.Inline> | |
| Since <Badge variant="secondary" size="s" content={domain} /> is an apex domain, | |
| CNAME record is only supported by certain providers. If yours doesn't, please verify | |
| using | |
| {#if aTabVisible} | |
| <Link variant="muted" onclick={onNavigateToA}>A record</Link> | |
| {#if aaaaTabVisible} | |
| or <Link variant="muted" onclick={onNavigateToAAAA}>AAAA record</Link | |
| >{/if} | |
| {:else if aaaaTabVisible} | |
| <Link variant="muted" onclick={onNavigateToAAAA}>AAAA record</Link> | |
| {/if} instead. | |
| </Alert.Inline> | |
| {/if} |
🤖 Prompt for AI Agents
In src/lib/components/domains/recordTable.svelte around lines 123 to 145, the
event handlers use the Svelte 3 "on:click" syntax; migrate them to Svelte 5 by
replacing on:click={...} with the onclick={...} prop on the Link components
(apply to onNavigateToNameservers, onNavigateToA, and onNavigateToAAAA),
ensuring no other behavior changes and preserving existing casing and props.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/routes/(console)/project-[region]-[project]/settings/domains/table.svelte (2)
141-156: Updated column UX/a11yGood relative timestamps. Consider giving the header a non-empty accessible name (e.g., “Updated”) or aria-label so screen readers don’t encounter a blank header.
239-242: Unify divider spacing with sites table (optional)Consider matching the padding used in the sites table for consistency.
src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/table.svelte (3)
76-133: Status copy for 'created' and a11y for action Links
- Please confirm “Verification failed” for status 'created' is the intended copy/state mapping.
- Use Button or add role to Link used as a button for keyboard/a11y.
Add role/labels to Links:
- <Link + <Link size="s" - variant="muted" + variant="muted" + role="button" + aria-label="Retry verification" on:click={(e) => { e.preventDefault(); selectedProxyRule = rule; showRetry = true; }}> Retry </Link>- <Link + <Link size="s" - variant="muted" + variant="muted" + role="button" + aria-label="View logs" on:click={(e) => { e.preventDefault(); selectedProxyRule = rule; showLogs = true; }}> View logs </Link>- <Link + <Link size="s" - variant="muted" + variant="muted" + role="button" + aria-label="View logs" on:click={(e) => { e.preventDefault(); selectedProxyRule = rule; showLogs = true; }}> View logs </Link>
137-153: Updated column UX/a11yRelative text is good. Ensure the columns store provides an accessible header/aria-label for the “Updated” column (it shouldn’t be blank for screen readers).
76-133: Reduce duplication across tables (optional)The status badge + action snippets are duplicated between sites and settings tables; consider a small shared StatusMeta component or a mapping helper to centralize copy and actions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/table.svelte(7 hunks)src/routes/(console)/project-[region]-[project]/settings/domains/table.svelte(8 hunks)src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/table.svelte(2 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). (6)
- GitHub Check: publish-cloud
- GitHub Check: publish-cloud-no-regions
- GitHub Check: publish-self-hosted
- GitHub Check: publish-cloud-stage
- GitHub Check: build
- GitHub Check: e2e
🔇 Additional comments (9)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/table.svelte (1)
41-41: AllowselectedProxyRuleto be nullable
$state(null)initializes withnull, but the annotation disallows it and fails TypeScript checks. Loosen the type to includenull(and handle the nullable path where consumed).-let selectedProxyRule: Models.ProxyRule = $state(null); +let selectedProxyRule: Models.ProxyRule | null = $state(null);src/routes/(console)/project-[region]-[project]/settings/domains/table.svelte (5)
24-27: Barrel imports + helper usage look goodSwitch to public barrels and date helper is consistent with PR direction.
82-140: Status copy for 'created' and a11y for action Links
- Please confirm “Verification failed” for status 'created' is correct per backend states; it reads like an error state rather than “not verified yet”.
- Use Button or add role to Link used as a button for keyboard/a11y.
Add role/labels to Links:
- <Link + <Link size="s" - variant="muted" + variant="muted" + role="button" + aria-label="Retry verification" on:click={(e) => { e.preventDefault(); selectedDomain = domain; showRetry = true; }}> Retry </Link>- <Link + <Link size="s" - variant="muted" + variant="muted" + role="button" + aria-label="View logs" on:click={(e) => { e.preventDefault(); selectedDomain = domain; showLogs = true; }}> View logs </Link>- <Link + <Link size="s" - variant="muted" + variant="muted" + role="button" + aria-label="View logs" on:click={(e) => { e.preventDefault(); selectedDomain = domain; showLogs = true; }}> View logs </Link>
176-187: Logs gating matches flowGating “View logs” in the menu by both status and presence of logs is correct and consistent with the inline actions.
Also applies to: 199-203
234-236: Logs modal wiring looks correctProp name matches ViewLogsModal API; state is set before opening.
40-40: Type allows null for reactive stateselectedDomain is initialized with null but typed non-nullable. Make it a union.
Apply this diff:
-let selectedDomain: Models.ProxyRule = $state(null); +let selectedDomain: Models.ProxyRule | null = $state(null);Based on past review comments
src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/table.svelte (3)
24-28: Barrel imports + helper usage look goodAligns with new exports surface; reduces deep imports.
172-182: Logs gating in ActionMenu is correctCondition matches inline behavior and prevents empty modal.
Also applies to: 195-199
230-232: Logs modal wiring looks correctProp and toggle are consistent with selection flow.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/lib/components/domains/recordTable.svelte (3)
37-121: Guard regionalConsoleVariables access before it initializes.
$regionalConsoleVariablescan beundefinedwhile the store loads, so direct property reads (A/AAAA derived flags, CAA text, template interpolations) will throw. Please optional-chain every access (both in the derived values and the markup) so the component stays defensive until the store resolves.- const aTabVisible = $derived( - !isCloud && - Boolean($regionalConsoleVariables._APP_DOMAIN_TARGET_A) && - $regionalConsoleVariables._APP_DOMAIN_TARGET_A !== '127.0.0.1' - ); - const aaaaTabVisible = $derived( - !isCloud && - Boolean($regionalConsoleVariables._APP_DOMAIN_TARGET_AAAA) && - $regionalConsoleVariables._APP_DOMAIN_TARGET_AAAA !== '::1' - ); + const aTabVisible = $derived( + !isCloud && + Boolean($regionalConsoleVariables?._APP_DOMAIN_TARGET_A) && + $regionalConsoleVariables?._APP_DOMAIN_TARGET_A !== '127.0.0.1' + ); + const aaaaTabVisible = $derived( + !isCloud && + Boolean($regionalConsoleVariables?._APP_DOMAIN_TARGET_AAAA) && + $regionalConsoleVariables?._APP_DOMAIN_TARGET_AAAA !== '::1' + ); @@ - const caaText = $derived( - $regionalConsoleVariables._APP_DOMAIN_TARGET_CAA?.includes(' ') - ? $regionalConsoleVariables._APP_DOMAIN_TARGET_CAA - : `0 issue "${$regionalConsoleVariables._APP_DOMAIN_TARGET_CAA}"` - ); + const caaText = $derived( + $regionalConsoleVariables?._APP_DOMAIN_TARGET_CAA?.includes(' ') + ? $regionalConsoleVariables?._APP_DOMAIN_TARGET_CAA + : `0 issue "${$regionalConsoleVariables?._APP_DOMAIN_TARGET_CAA ?? ''}"` + ); @@ - <Typography.Text variant="m-400"> - Add the following {$regionalConsoleVariables._APP_DOMAIN_TARGET_CAA - ? 'records' - : 'record'} on your DNS provider. + <Typography.Text variant="m-400"> + Add the following {$regionalConsoleVariables?._APP_DOMAIN_TARGET_CAA + ? 'records' + : 'record'} on your DNS provider. @@ - {#if $regionalConsoleVariables._APP_DOMAIN_TARGET_CAA} + {#if $regionalConsoleVariables?._APP_DOMAIN_TARGET_CAA} @@ - <InteractiveText variant="copy" isVisible text={caaText} /> + <InteractiveText variant="copy" isVisible text={caaText} />
74-86: Show the success badge when ruleStatus is verified.When
ruleStatus === 'verified'butverifiedis false, the UI never shows the success badge. TreatruleStatus === 'verified'as verified so users see the correct state.- {:else if ruleStatus === 'unverified'} + {:else if ruleStatus === 'unverified'} <Badge variant="secondary" type="error" size="xs" content="Certificate generation failed" /> - {:else if verified === true} + {:else if ruleStatus === 'verified' || verified === true} <Badge variant="secondary" type="success" size="xs" content="Verified" />
131-145: Migrate event handlers to Svelte 5 syntax.Svelte 5 no longer uses
on:click; handlers must be assigned via theonclickprop. Update all threeLinkcomponents here accordingly.- <Link variant="muted" on:click={onNavigateToNameservers}>nameservers</Link> instead. + <Link variant="muted" onclick={onNavigateToNameservers}>nameservers</Link> instead. @@ - <Link variant="muted" on:click={onNavigateToA}>A record</Link> + <Link variant="muted" onclick={onNavigateToA}>A record</Link> @@ - or <Link variant="muted" on:click={onNavigateToAAAA}>AAAA record</Link + or <Link variant="muted" onclick={onNavigateToAAAA}>AAAA record</Link @@ - <Link variant="muted" on:click={onNavigateToAAAA}>AAAA record</Link> + <Link variant="muted" onclick={onNavigateToAAAA}>AAAA record</Link>Based on learnings
src/lib/components/domains/cnameTable.svelte (1)
25-31: Provide safe fallback when_APP_DOMAIN_TARGET_CAAis undefined.If
_APP_DOMAIN_TARGET_CAAis undefined, the current logic will produce"0 issue \"undefined\""because the false branch of the ternary doesn't guard against undefined interpolation.Apply this diff to add a safe fallback:
const caaText = $derived( - $regionalConsoleVariables._APP_DOMAIN_TARGET_CAA?.includes(' ') - ? $regionalConsoleVariables._APP_DOMAIN_TARGET_CAA - : `0 issue "${$regionalConsoleVariables._APP_DOMAIN_TARGET_CAA}"` + $regionalConsoleVariables?._APP_DOMAIN_TARGET_CAA?.includes(' ') + ? $regionalConsoleVariables._APP_DOMAIN_TARGET_CAA + : $regionalConsoleVariables?._APP_DOMAIN_TARGET_CAA + ? `0 issue "${$regionalConsoleVariables._APP_DOMAIN_TARGET_CAA}"` + : '' );
🧹 Nitpick comments (2)
src/lib/components/domains/cnameTable.svelte (2)
55-59: Use optional chaining for consistency.While the current store access works, using optional chaining aligns with defensive programming practices used elsewhere in the component.
-Add the following {$regionalConsoleVariables._APP_DOMAIN_TARGET_CAA +Add the following {$regionalConsoleVariables?._APP_DOMAIN_TARGET_CAA ? 'records' : 'record'} on your DNS provider. Note that DNS changes may take up to 48 hours to propagate fully.
78-91: Use optional chaining in conditional for consistency.The CAA row conditional should use optional chaining to align with defensive programming practices, especially since the condition guards the rendering of optional DNS records.
-{#if $regionalConsoleVariables._APP_DOMAIN_TARGET_CAA} +{#if $regionalConsoleVariables?._APP_DOMAIN_TARGET_CAA} <Table.Row.Base {root}> <Table.Cell {root}> <Layout.Stack gap="s" direction="row" alignItems="center"> <span>CAA</span> <Badge variant="secondary" size="xs" content="Recommended" /> </Layout.Stack> </Table.Cell> <Table.Cell {root}>@</Table.Cell> <Table.Cell {root}> <InteractiveText variant="copy" isVisible text={caaText} /> </Table.Cell> </Table.Row.Base> {/if}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lib/components/domains/cnameTable.svelte(4 hunks)src/lib/components/domains/recordTable.svelte(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-25T04:31:05.213Z
Learnt from: ItzNotABug
PR: appwrite/console#2373
File: src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/options.svelte:33-42
Timestamp: 2025-09-25T04:31:05.213Z
Learning: Svelte 5 uses `onclick` instead of `on:click` for event handlers. Event handlers are now properties like any other rather than directives that require the `on:` prefix.
Applied to files:
src/lib/components/domains/recordTable.svelte
📚 Learning: 2025-09-25T04:31:05.213Z
Learnt from: ItzNotABug
PR: appwrite/console#2373
File: src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/options.svelte:33-42
Timestamp: 2025-09-25T04:31:05.213Z
Learning: In Svelte 5, the `on:` directive syntax (like `on:click`) has been replaced with direct property syntax (like `onclick`). This is part of the migration from Svelte 4 to Svelte 5 where event handlers are treated as regular properties.
Applied to files:
src/lib/components/domains/recordTable.svelte
📚 Learning: 2025-08-31T05:32:42.285Z
Learnt from: ItzNotABug
PR: appwrite/console#2284
File: src/lib/components/searchQuery.svelte:26-27
Timestamp: 2025-08-31T05:32:42.285Z
Learning: SvelteKit automatically enables Svelte 5 runes without requiring explicit "use runes" directive or runes: true configuration when using Svelte 5.
Applied to files:
src/lib/components/domains/cnameTable.svelte
🔇 Additional comments (4)
src/lib/components/domains/cnameTable.svelte (4)
4-4: LGTM!The import of
getSubdomainis appropriate and aligns with the centralized subdomain parsing approach introduced in this PR.
15-23: LGTM!The migration to Svelte 5 runes syntax using
$props()is correctly implemented with proper TypeScript typing for optional properties. This addresses the past review comment requesting migration to runes syntax.Based on learnings
70-70: LGTM!The subdomain display with
'@'fallback correctly represents the root domain when subdomain is empty.
40-52: Add explicitruleStatus === 'verifiedbranch
Confirm that the backend can return'verified'as aruleStatusvalue; otherwise standardize on a single status source (either drop theverifiedboolean fallback or align both).
There was a problem hiding this comment.
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)
src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/retryDomainModal.svelte (2)
27-37: Align tab selection logic with tab visibility conditions.The effect sets
selectedTabbased on environment variables but doesn't match the visibility conditions used in the template (lines 80-111). For example, the effect selects'cname'if_APP_DOMAIN_TARGET_CNAMEis truthy, but the CNAME tab only renders when it's also not'localhost'. This mismatch could select a tab that won't be rendered, leaving no visible content.Apply this diff to align the conditions:
$effect(() => { - if ($regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME) { + if ( + !!$regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME && + $regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME !== 'localhost' + ) { selectedTab = 'cname'; - } else if (!isCloud && $regionalConsoleVariables._APP_DOMAIN_TARGET_A) { + } else if ( + !isCloud && + !!$regionalConsoleVariables._APP_DOMAIN_TARGET_A && + $regionalConsoleVariables._APP_DOMAIN_TARGET_A !== '127.0.0.1' + ) { selectedTab = 'a'; - } else if (!isCloud && $regionalConsoleVariables._APP_DOMAIN_TARGET_AAAA) { + } else if ( + !isCloud && + !!$regionalConsoleVariables._APP_DOMAIN_TARGET_AAAA && + $regionalConsoleVariables._APP_DOMAIN_TARGET_AAAA !== '::1' + ) { selectedTab = 'aaaa'; } else { selectedTab = 'nameserver'; } });
40-40: InitializeverifiedfromselectedProxyRule.statusto reflect actual state.The
verifiedstate is initialized tofalse, causing the UI to show an unverified state initially even when the rule is already verified. Initialize it from the selected rule's status.Apply this diff:
-let verified = $state(false); +let verified = $state(selectedProxyRule?.status === 'verified');
♻️ Duplicate comments (2)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/retryDomainModal.svelte (1)
40-40: InitializeverifiedfromselectedProxyRule.statusto reflect actual state.The
verifiedstate is initialized tofalse, causing the UI to show an unverified state initially even when the rule is already verified. Initialize it from the selected rule's status.Apply this diff:
-let verified = $state(false); +let verified = $state(selectedProxyRule?.status === 'verified');src/routes/(console)/project-[region]-[project]/settings/domains/retryDomainModal.svelte (1)
27-37: Align initial tab selection with rendered tabs (exclude loopbacks; avoid selecting hidden tabs).Effect can pick CNAME/A/AAAA when those tabs are hidden (localhost/127.0.0.1/::1), or select Nameservers on self-hosted where it isn’t rendered. Mirror the header logic.
Apply:
$effect(() => { - if ($regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME) { - selectedTab = 'cname'; - } else if (!isCloud && $regionalConsoleVariables._APP_DOMAIN_TARGET_A) { - selectedTab = 'a'; - } else if (!isCloud && $regionalConsoleVariables._APP_DOMAIN_TARGET_AAAA) { - selectedTab = 'aaaa'; - } else { - selectedTab = 'nameserver'; - } + const hasCname = + !!$regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME && + $regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME !== 'localhost'; + const hasA = + !isCloud && + !!$regionalConsoleVariables._APP_DOMAIN_TARGET_A && + $regionalConsoleVariables._APP_DOMAIN_TARGET_A !== '127.0.0.1'; + const hasAaaa = + !isCloud && + !!$regionalConsoleVariables._APP_DOMAIN_TARGET_AAAA && + $regionalConsoleVariables._APP_DOMAIN_TARGET_AAAA !== '::1'; + + if (hasCname) selectedTab = 'cname'; + else if (hasA) selectedTab = 'a'; + else if (hasAaaa) selectedTab = 'aaaa'; + else selectedTab = isCloud ? 'nameserver' : 'a'; });
🧹 Nitpick comments (3)
src/routes/(console)/project-[region]-[project]/settings/domains/retryDomainModal.svelte (3)
51-61: Differentiate “verifying” from “failed” to match flow states.Treat all non-verified states as “failed”. Show a clearer message for in-progress verification.
Apply:
- if (verified) { + if (verified) { show = false; addNotification({ type: 'success', message: `${selectedDomain.domain} has been verified` }); } else { - error = - 'Domain verification failed. Please check your domain settings or try again later'; + error = + domain.status === 'verifying' + ? 'Verification started. This may take a few minutes to complete.' + : 'Domain verification failed. Please check your DNS records and try again.'; }
63-66: Make catch block type-safe (unknown catch) and preserve message.Accessing e.message without narrowing can break TS configs with useUnknownInCatchVariables.
Apply:
- } catch (e) { - error = - e.message ?? - 'Domain verification failed. Please check your domain settings or try again later'; + } catch (e: unknown) { + const msg = + e instanceof Error && e.message + ? e.message + : 'Domain verification failed. Please check your domain settings or try again later'; + error = msg; trackError(e, Submit.DomainUpdateVerification); }
121-129: Guard navigation to Nameservers on self-hosted.Avoid switching to a non-rendered tab in non‑cloud environments.
Apply:
- onNavigateToNameservers={() => (selectedTab = 'nameserver')} + onNavigateToNameservers={() => isCloud && (selectedTab = 'nameserver')}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/retryDomainModal.svelte(3 hunks)src/routes/(console)/project-[region]-[project]/settings/domains/retryDomainModal.svelte(3 hunks)src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/retryDomainModal.svelte(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). (2)
- GitHub Check: build
- GitHub Check: e2e
🔇 Additional comments (4)
src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/retryDomainModal.svelte (2)
51-60: Improved verification feedback logic.The conditional handling now correctly shows a success notification only when verification succeeds, and sets an error message when it fails. This prevents misleading "verified" toasts when the verification is still pending.
78-130: Well-structured tabbed UI with status propagation.The tabbed interface cleanly separates different DNS record types, and the conditional rendering ensures only valid tabs are shown. The
ruleStatusprop is correctly passed to bothNameserverTableandRecordTable, aligning with the PR's objective to surface rule status in the UI.src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/retryDomainModal.svelte (2)
51-60: Improved verification feedback logic.The conditional handling now correctly shows a success notification only when verification succeeds, and sets an error message when it fails. This addresses the concern from the previous review about always showing a "verified" success toast.
78-130: Well-structured tabbed UI with status propagation.The tabbed interface cleanly separates different DNS record types, and the conditional rendering ensures only valid tabs are shown. The
ruleStatusprop is correctly passed to bothNameserverTableandRecordTable, aligning with the PR's objective to surface rule status in the UI.
| $effect(() => { | ||
| if ($regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME) { | ||
| selectedTab = 'cname'; | ||
| } else if (!isCloud && $regionalConsoleVariables._APP_DOMAIN_TARGET_A) { | ||
| selectedTab = 'a'; | ||
| } else if (!isCloud && $regionalConsoleVariables._APP_DOMAIN_TARGET_AAAA) { | ||
| selectedTab = 'aaaa'; | ||
| } else { | ||
| selectedTab = 'nameserver'; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Align tab selection logic with tab visibility conditions.
The effect sets selectedTab based on environment variables but doesn't match the visibility conditions used in the template (lines 80-111). For example, the effect selects 'cname' if _APP_DOMAIN_TARGET_CNAME is truthy, but the CNAME tab only renders when it's also not 'localhost'. This mismatch could select a tab that won't be rendered, leaving no visible content.
Apply this diff to align the conditions:
$effect(() => {
- if ($regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME) {
+ if (
+ !!$regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME &&
+ $regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME !== 'localhost'
+ ) {
selectedTab = 'cname';
- } else if (!isCloud && $regionalConsoleVariables._APP_DOMAIN_TARGET_A) {
+ } else if (
+ !isCloud &&
+ !!$regionalConsoleVariables._APP_DOMAIN_TARGET_A &&
+ $regionalConsoleVariables._APP_DOMAIN_TARGET_A !== '127.0.0.1'
+ ) {
selectedTab = 'a';
- } else if (!isCloud && $regionalConsoleVariables._APP_DOMAIN_TARGET_AAAA) {
+ } else if (
+ !isCloud &&
+ !!$regionalConsoleVariables._APP_DOMAIN_TARGET_AAAA &&
+ $regionalConsoleVariables._APP_DOMAIN_TARGET_AAAA !== '::1'
+ ) {
selectedTab = 'aaaa';
} else {
selectedTab = 'nameserver';
}
});📝 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.
| $effect(() => { | |
| if ($regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME) { | |
| selectedTab = 'cname'; | |
| } else if (!isCloud && $regionalConsoleVariables._APP_DOMAIN_TARGET_A) { | |
| selectedTab = 'a'; | |
| } else if (!isCloud && $regionalConsoleVariables._APP_DOMAIN_TARGET_AAAA) { | |
| selectedTab = 'aaaa'; | |
| } else { | |
| selectedTab = 'nameserver'; | |
| } | |
| }); | |
| $effect(() => { | |
| if ( | |
| !!$regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME && | |
| $regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME !== 'localhost' | |
| ) { | |
| selectedTab = 'cname'; | |
| } else if ( | |
| !isCloud && | |
| !!$regionalConsoleVariables._APP_DOMAIN_TARGET_A && | |
| $regionalConsoleVariables._APP_DOMAIN_TARGET_A !== '127.0.0.1' | |
| ) { | |
| selectedTab = 'a'; | |
| } else if ( | |
| !isCloud && | |
| !!$regionalConsoleVariables._APP_DOMAIN_TARGET_AAAA && | |
| $regionalConsoleVariables._APP_DOMAIN_TARGET_AAAA !== '::1' | |
| ) { | |
| selectedTab = 'aaaa'; | |
| } else { | |
| selectedTab = 'nameserver'; | |
| } | |
| }); |
🤖 Prompt for AI Agents
In
src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/retryDomainModal.svelte
around lines 27 to 37, the effect that sets selectedTab picks tabs based only on
truthiness of environment variables but does not mirror the template's
visibility checks (e.g. CNAME/A/AAAA tabs also require the target not be
'localhost' and the A/AAAA tabs require !isCloud), which can pick a tab that
won't render; update the effect to use the same visibility conditions as the
template — for CNAME require _APP_DOMAIN_TARGET_CNAME &&
_APP_DOMAIN_TARGET_CNAME !== 'localhost', for A require !isCloud &&
_APP_DOMAIN_TARGET_A && _APP_DOMAIN_TARGET_A !== 'localhost', for AAAA require
!isCloud && _APP_DOMAIN_TARGET_AAAA && _APP_DOMAIN_TARGET_AAAA !== 'localhost' —
and otherwise set selectedTab to 'nameserver' so the selected tab is always one
that will render.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/routes/(console)/project-[region]-[project]/settings/domains/table.svelte (1)
39-41: Fix null typing forselectedDomainstate.
$state(null)initializes withnull, but the declaration forbids it, so the build fails. Allownull(and make the modals tolerate it) before opening this component.-let selectedDomain: Models.ProxyRule = $state(null); +let selectedDomain: Models.ProxyRule | null = $state(null);
🧹 Nitpick comments (1)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/table.svelte (1)
75-141: Consider extracting shared domain status rendering logic.The status rendering and updated column logic (lines 75-141) is nearly identical to the sites table (src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/table.svelte lines 76-142). This duplication makes maintenance harder and increases the risk of divergence.
Consider extracting the domain status cell and updated cell into shared components:
Create a new component
src/lib/components/domains/domainStatusCell.svelte:<script lang="ts"> import { Link } from '$lib/elements'; import { Badge, Layout } from '@appwrite.io/pink-svelte'; import type { Models } from '@appwrite.io/console'; let { rule, onRetry, onViewLogs }: { rule: Models.ProxyRule; onRetry: () => void; onViewLogs: () => void; } = $props(); </script> {#if rule.status === 'created'} <Layout.Stack direction="row" gap="s" alignItems="center"> <Badge variant="secondary" type="error" content="Verification failed" size="xs" /> <Link size="s" variant="muted" on:click={(e) => { e.preventDefault(); onRetry(); }}> Retry </Link> </Layout.Stack> {:else if rule.status === 'verifying'} <Layout.Stack direction="row" gap="s" alignItems="center"> <Badge variant="secondary" content="Generating certificate" size="xs" /> </Layout.Stack> {:else if rule.status === 'unverified'} <Layout.Stack direction="row" gap="s" alignItems="center"> <Badge variant="secondary" type="error" content="Certificate generation failed" size="xs" /> </Layout.Stack> {/if} {#if rule.logs && rule.logs.length > 0} <Link size="s" variant="muted" on:click={(e) => { e.preventDefault(); onViewLogs(); }}> View logs </Link> {/if}Then use it in both tables to reduce duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/+layout.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/table.svelte(7 hunks)src/routes/(console)/project-[region]-[project]/settings/domains/+layout.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/settings/domains/table.svelte(8 hunks)src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/+layout.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/table.svelte(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). (6)
- GitHub Check: publish-self-hosted
- GitHub Check: publish-cloud
- GitHub Check: publish-cloud-no-regions
- GitHub Check: publish-cloud-stage
- GitHub Check: e2e
- GitHub Check: build
🔇 Additional comments (10)
src/routes/(console)/project-[region]-[project]/settings/domains/+layout.svelte (1)
1-17: LGTM!The realtime subscription pattern is correctly implemented. The layout listens for proxy update events and invalidates the DOMAINS cache, ensuring the UI stays in sync with backend changes.
src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/+layout.svelte (1)
1-17: LGTM!The realtime subscription is correctly configured for site-specific domains, using SITES_DOMAINS dependency for cache invalidation.
src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/table.svelte (6)
24-28: LGTM!The imports correctly use the public barrel exports for shared components and the new time formatting helper.
76-122: LGTM!The multi-branch status rendering correctly handles all domain verification states with appropriate badges and actions. The conditional "View logs" link properly guards against empty logs.
126-142: LGTM!The updated column effectively displays relative timestamps with status-appropriate prefixes. The conditional rendering (hidden for verified) keeps the UI clean.
161-171: LGTM!The "View logs" action menu item is properly gated and follows the established UI pattern.
184-188: LGTM!The conditional divider properly separates log-related actions from destructive actions when logs exist.
219-221: LGTM!The ViewLogsModal is correctly wired with proper state binding.
src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/table.svelte (1)
222-226: Verify CSS consistency with sites table.The
.action-menu-dividerstyling here differs from the sites table (src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/table.svelte lines 223-229), which includes additional padding rules:padding-block-start: 0.25rem; padding-block-end: 0.25rem;Confirm whether this difference is intentional or if the padding should be applied here as well for visual consistency.
src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/+layout.svelte (1)
3-3: Use$app/storesfor the page store in SvelteKit 2.x
Update line 3 in+layout.svelteto:import { page } from '$app/stores';SvelteKit 2.x exports
pagefrom$app/stores, not$app/state.⛔ Skipped due to learnings
Learnt from: ItzNotABug PR: appwrite/console#2413 File: src/routes/(console)/project-[region]-[project]/databases/table.svelte:33-39 Timestamp: 2025-10-13T05:13:54.531Z Learning: In Svelte 5, `import { page } from '$app/state'` provides a reactive state proxy that can be accessed directly (e.g., `page.params`), unlike the older `import { page } from '$app/stores'` which returns a readable store requiring the `$page` syntax for auto-subscription in components.Learnt from: ItzNotABug PR: appwrite/console#2398 File: src/routes/(console)/verify-email/+page.svelte:48-51 Timestamp: 2025-10-05T09:41:40.439Z Learning: In SvelteKit 5, `page` imported from `$app/state` is a reactive state object (using runes), not a store. It should be accessed as `page.data` without the `$` prefix, unlike the store-based `$page` from `$app/stores` in earlier versions.
| onMount(() => { | ||
| return realtime | ||
| .forProject(page.params.region, page.params.project) | ||
| .subscribe('console', (response) => { | ||
| if (response.events.includes('projects.*.proxy.*.update')) { | ||
| invalidate(Dependencies.FUNCTION_DOMAINS); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify subscription cleanup and consider adding error handling.
Two potential concerns:
-
Cleanup verification: The subscription is returned directly from
onMount. Svelte's cleanup mechanism expects a function to be returned. Verify thatrealtime.forProject().subscribe()returns a cleanup function (not an object with anunsubscribemethod), otherwise the subscription may not be properly torn down on component destroy. -
Error handling: The real-time subscription lacks error handling. If the subscription fails or the connection drops, there's no fallback or retry mechanism.
Consider verifying the subscription API and adding error handling:
If the subscription returns an object rather than a function, refactor to:
onMount(() => {
- return realtime
+ const subscription = realtime
.forProject(page.params.region, page.params.project)
.subscribe('console', (response) => {
if (response.events.includes('projects.*.proxy.*.update')) {
invalidate(Dependencies.FUNCTION_DOMAINS);
}
});
+
+ return () => subscription.unsubscribe();
});🏁 Script executed:
#!/bin/bash
# Search for other real-time subscription patterns in the codebase
rg -n -C3 'realtime\.forProject.*\.subscribe' --type=svelteLength of output: 30
🏁 Script executed:
#!/bin/bash
rg -n -C3 'realtime\.forProject.*\.subscribe' -g '*.svelte'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Locate where `realtime` is imported in Svelte components
rg -n "import.*realtime" -g "*.svelte"
# Find the implementation of `forProject` (and its subscribe) in JS/TS sources
rg -n "forProject" -g "*.ts" -g "*.js"Length of output: 23905
🏁 Script executed:
#!/bin/bash
# Extract the `realtime` export definition
rg -n -C5 "export .*realtime" -g "src/lib/stores/sdk.ts"
# Extract any `.subscribe(` occurrences and nearby code in the SDK file
rg -n -C5 "\.subscribe" -g "src/lib/stores/sdk.ts"Length of output: 679
🏁 Script executed:
#!/bin/bash
# Show full forProject implementation
sed -n '137,200p' src/lib/stores/sdk.ts
# Find `clientRealtime` import/definition
rg -n "clientRealtime" -C3 src/lib/stores/sdk.tsLength of output: 2302
Return a cleanup function in onMount to unsubscribe and add error handling
realtime.forProject(...).subscribe()returns a subscription object (with.unsubscribe()), so returning it directly won’t clean up. Wrap it:
onMount(() => {
- return realtime
- .forProject(page.params.region, page.params.project)
- .subscribe('console', handler);
+ const sub = realtime
+ .forProject(page.params.region, page.params.project)
+ .subscribe('console', handler);
+ return () => sub.unsubscribe();
});- Add error handling (e.g.
.on('error', …)or retry logic) to surface and recover from connection failures.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/+layout.svelte
around lines 8 to 16, the onMount currently returns the subscription object
directly which won't be used as a cleanup function and it lacks error handling;
change it to capture the subscription, return a function that calls
subscription.unsubscribe() for cleanup, and attach error handling to the
realtime subscription (e.g., subscription.on('error', handler) or wrap subscribe
with try/catch and implement a retry/backoff or logging handler) so connection
failures are surfaced and can be retried or logged appropriately.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/table.svelte (1)
41-41: Critical: Address the unresolved type safety issue from previous review.The type of
selectedProxyRuledoes not match its initial value. This was flagged in the previous review but remains unaddressed.Apply this diff to fix the type mismatch:
-let selectedProxyRule: Models.ProxyRule = $state(null); +let selectedProxyRule: Models.ProxyRule | null = $state(null);src/routes/(console)/project-[region]-[project]/settings/domains/table.svelte (1)
40-40: Type: allow null in reactive state.Same issue flagged in previous review:
selectedDomainis typed asModels.ProxyRulebut initialized tonull. Update to union type.-let selectedDomain: Models.ProxyRule = $state(null); +let selectedDomain: Models.ProxyRule | null = $state(null);
🧹 Nitpick comments (4)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/table.svelte (2)
194-198: Consider consistent divider placement.The divider before the Delete action only appears when logs exist, creating inconsistent visual separation. For better UX consistency, consider either always showing the divider before the destructive Delete action, or placing it immediately after the View logs action to group log-related items.
If you prefer to always separate the Delete action:
- {#if proxyRule.logs && proxyRule.logs.length > 0} - <div class="action-menu-divider"> - <Divider /> - </div> - {/if} + <div class="action-menu-divider"> + <Divider /> + </div>
99-110: Optional: Simplify repeated null checks with optional chaining.The pattern
proxyRule.logs && proxyRule.logs.length > 0appears multiple times and can be simplified using optional chaining for better readability.Consider this refactor:
-{#if proxyRule.logs && proxyRule.logs.length > 0} +{#if proxyRule.logs?.length}This applies to lines 99, 119, and 171.
Also applies to: 119-130, 171-181
src/routes/(console)/project-[region]-[project]/settings/domains/table.svelte (2)
176-186: Consider extracting the logs existence check.The condition
domain.logs && domain.logs.length > 0appears multiple times (lines 106, 126, 176, 199). Consider a derived value or helper function for maintainability.Add a derived value near line 40:
+let hasLogs = $derived(selectedDomain?.logs && selectedDomain.logs.length > 0);Or add a helper in the loop context:
{#each domains.rules as domain} {@const hasLogs = domain.logs && domain.logs.length > 0} <Table.Row.Base {root}> ... {#if hasLogs} ... {/if}Also applies to: 199-203
89-98: Use Button for non-navigational actions instead of Link
Replace the inline<Link>(withpreventDefault) for “Retry” and “View logs” with<Button text size="s">…</Button>to restore proper semantics, keyboard support, and ARIA roles.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/table.svelte(7 hunks)src/routes/(console)/project-[region]-[project]/settings/domains/table.svelte(8 hunks)src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/table.svelte(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). (2)
- GitHub Check: build
- GitHub Check: e2e
🔇 Additional comments (5)
src/routes/(console)/project-[region]-[project]/settings/domains/table.svelte (5)
6-27: LGTM!The import updates correctly support the new features:
IconTerminalfor logs UI,Dividerfor menu separation, barrel exports forViewLogsModalandDnsRecordsAction, andtimeFromNowShortfor the updated column.
42-56: LGTM!The column updates appropriately accommodate the new UI: wider domain column for badges and actions, and a right-aligned updated column for timestamps.
141-157: LGTM!The updated column effectively communicates status-based timestamps. The prefix variations ("Checked", "Updated", "Failed") provide helpful context for each state.
234-236: LGTM!The
ViewLogsModalintegration follows the same conditional rendering pattern as the other modals.
238-242: LGTM!The negative margin on
.action-menu-dividercorrectly extends the divider to the menu edges.
| {:else if column.id === 'updated'} | ||
| <Layout.Stack direction="row" justifyContent="flex-end"> | ||
| {#if rule.status !== 'verified'} | ||
| <Typography.Text | ||
| variant="m-400" | ||
| color="--fgcolor-neutral-tertiary" | ||
| style="font-size: 0.875rem;"> | ||
| {#if rule.status === 'created'} | ||
| Checked {timeFromNowShort(rule.$updatedAt)} | ||
| {:else if rule.status === 'verifying'} | ||
| Updated {timeFromNowShort(rule.$updatedAt)} | ||
| {:else if rule.status === 'unverified'} | ||
| Failed {timeFromNowShort(rule.$updatedAt)} | ||
| {/if} | ||
| </Typography.Text> | ||
| {/if} | ||
| </Layout.Stack> |
There was a problem hiding this comment.
Show updated timestamp for verified domains
Column header promises “Updated,” but verified rows render an empty cell because the Typography block is skipped. Most domains end up verified, so the new column appears blank, degrading the UX. Render a timestamp for verified rules as well (e.g., Verified {timeFromNowShort(...)}) to keep the column meaningful.
🤖 Prompt for AI Agents
In
src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/table.svelte
around lines 137 to 153, the "Updated" column skips rendering for rule.status
=== 'verified', leaving cells empty; update the conditional branch so the
Typography.Text block is rendered for verified rows as well and show a timestamp
like "Verified {timeFromNowShort(rule.$updatedAt)}" (add an {:else if
rule.status === 'verified'} branch or refactor the conditions to include
verified) so the column always displays an appropriate status label with time.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/+layout.svelte (1)
8-22: Subscription cleanup and error handling still need attention.The concerns raised in the previous review remain valid:
- Cleanup:
realtime.forProject().subscribe()returns a subscription object (not a cleanup function), so returning it directly fromonMountwon't properly unsubscribe on component destroy.- Error handling: The subscription lacks error handling for connection failures or subscription errors.
Apply this diff to fix cleanup:
onMount(() => { - return realtime - .forProject(page.params.region, page.params.project) - .subscribe('console', (response) => { + const subscription = realtime + .forProject(page.params.region, page.params.project) + .subscribe('console', (response) => { if ( response.events.includes('proxy.rules.*.update') || response.events.includes('proxy.rules.*.create') || response.events.includes('proxy.rules.*.delete') ) { if ((response.payload as any)?.deploymentResourceId === page.params.function) { invalidate(Dependencies.FUNCTION_DOMAINS); } } }); + + return () => subscription.unsubscribe(); });Additionally, consider adding error handling to the subscription (e.g.,
.on('error', handler)or retry logic) to surface and recover from connection failures.src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/table.svelte (1)
41-41: Type mismatch:selectedProxyRuleinitialized tonullbut typed as non-nullable.The variable is declared as
Models.ProxyRulebut initialized withnull, causing a type error. The type should allownull.Apply this diff:
-let selectedProxyRule: Models.ProxyRule = $state(null); +let selectedProxyRule: Models.ProxyRule | null = $state(null);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/+layout.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/+page.ts(1 hunks)src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/table.svelte(7 hunks)
🧰 Additional context used
🪛 ESLint
src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/+layout.svelte
[error] 17-17: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
🪛 GitHub Check: build
src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/+layout.svelte
[failure] 17-17:
Unexpected any. Specify a different type
⏰ 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). (5)
- GitHub Check: publish-cloud-stage
- GitHub Check: publish-self-hosted
- GitHub Check: publish-cloud-no-regions
- GitHub Check: publish-cloud
- GitHub Check: e2e
🔇 Additional comments (4)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/+page.ts (1)
31-31: LGTM! Sort order fixed.The change from an empty string to
'$updatedAt'fixes the invalid sort parameter and aligns with the PR's addition of an "updated" column to domain tables.src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/table.svelte (3)
75-132: LGTM! Status rendering logic is clear and well-structured.The status-based badge rendering correctly handles the three non-verified states (
created,verifying,unverified) with appropriate labels and actions. The "View logs" links are properly gated by log presence, addressing previous review concerns.
136-152: LGTM! Updated column rendering is appropriate.The relative time display using
timeFromNowShortprovides a clear, user-friendly indication of when the domain status was last updated. The status-specific prefixes ("Checked", "Updated", "Failed") add helpful context.
171-198: LGTM! Action menu items and divider are well-implemented.The "View logs" menu item and divider are both properly gated by log presence, ensuring they only appear when relevant. The divider styling appropriately extends to the menu edges.
| response.events.includes('proxy.rules.*.create') || | ||
| response.events.includes('proxy.rules.*.delete') | ||
| ) { | ||
| if ((response.payload as any)?.deploymentResourceId === page.params.function) { |
There was a problem hiding this comment.
Replace any with a specific type.
The explicit any cast bypasses type safety. Define an interface or use the appropriate type from the SDK.
Apply this diff to add type safety:
+ interface ProxyRulePayload {
+ deploymentResourceId?: string;
+ [key: string]: unknown;
+ }
+
onMount(() => {
return realtime
.forProject(page.params.region, page.params.project)
.subscribe('console', (response) => {
if (
response.events.includes('proxy.rules.*.update') ||
response.events.includes('proxy.rules.*.create') ||
response.events.includes('proxy.rules.*.delete')
) {
- if ((response.payload as any)?.deploymentResourceId === page.params.function) {
+ if ((response.payload as ProxyRulePayload)?.deploymentResourceId === page.params.function) {
invalidate(Dependencies.FUNCTION_DOMAINS);
}
}
});
});As per static analysis.
📝 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.
| if ((response.payload as any)?.deploymentResourceId === page.params.function) { | |
| <script lang="ts"> | |
| import { onMount } from 'svelte'; | |
| import { realtime, Dependencies } from '$lib/stores/sdk'; | |
| interface ProxyRulePayload { | |
| deploymentResourceId?: string; | |
| [key: string]: unknown; | |
| } | |
| onMount(() => { | |
| return realtime | |
| .forProject(page.params.region, page.params.project) | |
| .subscribe('console', (response) => { | |
| if ( | |
| response.events.includes('proxy.rules.*.update') || | |
| response.events.includes('proxy.rules.*.create') || | |
| response.events.includes('proxy.rules.*.delete') | |
| ) { | |
| if ((response.payload as ProxyRulePayload)?.deploymentResourceId === page.params.function) { | |
| invalidate(Dependencies.FUNCTION_DOMAINS); | |
| } | |
| } | |
| }); | |
| }); | |
| </script> |
🧰 Tools
🪛 ESLint
[error] 17-17: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
🪛 GitHub Check: build
[failure] 17-17:
Unexpected any. Specify a different type
🤖 Prompt for AI Agents
In
src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/+layout.svelte
around line 17, the code casts response.payload to any when checking
deploymentResourceId; replace the any with a concrete type by importing or
declaring an interface that includes deploymentResourceId (and any other used
fields) or use the SDK's payload type if available, then cast response.payload
to that type or narrow it with a type guard before the comparison so the check
becomes typesafe and avoids using any.
3 participants
What does this PR do?
a. Fix add-domain and verify domain flows
verification failed->generating certificate->certificate generation failedview logs,retrylinks nexts to badgesview logsin functions + projects domains dropdownlistDomainsfor cloudupdatedcolumn in each domains tableb. Update verification flows for sites/ functions/ projects tables
c. Update verification flow for org > domains table
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work.)
Related PRs and Issues
(If this PR is related to any other PR or resolves any issue or related to any issue link all related PR and issues here.)
Have you read the Contributing Guidelines on issues?
(Write your answer here.)
Summary by CodeRabbit
New Features
Style
Documentation