SER-330 Update flow for apex domains #2340
Conversation
ConsoleProject ID: Sites (2)
Note Appwrite has a Discord community with over 16 000 members. |
Walkthrough
Suggested reviewers
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing touches🧪 Generate unit tests
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
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/add-domain/verify-[domain]/+page.svelte (2)
58-61: Logic error: Verification always fails.The verify function will always throw an error for non-nameserver verification, even when verification succeeds. The error should only be thrown when verification actually fails.
Apply this fix:
const ruleData = await sdk .forProject(page.params.region, page.params.project) .proxy.updateRuleVerification({ ruleId }); verified = ruleData.status === 'verified'; -throw new Error( - 'Domain verification failed. Please check your domain settings or try again later' -); +if (!verified) { + throw new Error( + 'Domain verification failed. Please check your domain settings or try again later' + ); +}
67-70: Logic error: Verification always fails for new cloud domains.Similar to the issue above, the function throws an error even when verification succeeds for new cloud domains.
Apply this fix:
verified = domainData.nameservers.toLowerCase() === 'appwrite'; -throw new Error( - 'Domain verification failed. Please check your domain settings or try again later' -); +if (!verified) { + throw new Error( + 'Domain verification failed. Please check your domain settings or try again later' + ); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/lib/components/domains/cnameTable.svelte(2 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(3 hunks)src/routes/(console)/project-[region]-[project]/settings/domains/add-domain/verify-[domain]/+page.svelte(3 hunks)src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/add-domain/verify-[domain]/+page.svelte(3 hunks)src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/retryDomainModal.svelte(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e
- GitHub Check: build
🔇 Additional comments (11)
src/lib/components/domains/cnameTable.svelte (1)
33-34: LGTM! DNS propagation timeline update is consistent.The update to "48 hours" aligns with the industry-standard DNS propagation timeframe and is consistent across all files in this PR.
src/routes/(console)/organization-[organization]/domains/domain-[domain]/+page.svelte (1)
121-121: LGTM! DNS propagation timeline update is consistent.The update to "48 hours" aligns with the changes across the PR.
src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/retryDomainModal.svelte (1)
120-121: LGTM! Navigation callback properly integrated.The addition of the
onNavigateToNameserverscallback enables smooth navigation from CNAME to Nameservers tab when needed.src/routes/(console)/project-[region]-[project]/settings/domains/add-domain/verify-[domain]/+page.svelte (3)
34-36: LGTM! Simplified CNAME tab selection logic.Removing the subdomain check aligns with the PR objective of enabling CNAME for apex domains. The logic now correctly relies solely on the presence of
_APP_DOMAIN_TARGET_CNAME.
127-127: LGTM! Consistent CNAME tab visibility logic.The tab visibility condition correctly matches the selection logic, checking for CNAME target presence and excluding localhost.
165-169: LGTM! Navigation callback properly wired.The
onNavigateToNameserverscallback enables users to switch from CNAME to Nameservers tab, addressing the PR requirement for a CTA to use nameservers instead.src/routes/(console)/project-[region]-[project]/sites/site-[site]/domains/add-domain/verify-[domain]/+page.svelte (1)
166-167: LGTM! Navigation callback properly integrated.The
onNavigateToNameserverscallback is correctly wired to enable tab switching.src/routes/(console)/project-[region]-[project]/functions/function-[function]/domains/add-domain/verify-[domain]/+page.svelte (2)
34-35: LGTM! Simplified CNAME tab selection.The removal of subdomain checking aligns with the PR objective to enable CNAME for apex domains.
163-167: LGTM! Navigation callback properly integrated.The
onNavigateToNameserverscallback enables users to switch tabs as needed.src/lib/components/domains/recordTable.svelte (2)
14-28: LGTM! Well-structured props interface.The Props interface provides clear type definitions and the destructuring with defaults is properly implemented.
92-97: Well-implemented apex domain warning for CNAME.The alert clearly warns users about CNAME limitations for apex domains and provides a smooth navigation path to use nameservers instead. This addresses the PR requirement effectively.
| <Table.Row.Base {root}> | ||
| <Table.Cell {root}> | ||
| <Layout.Stack direction="row" alignItems="center" gap="xs"> | ||
| CAA | ||
| <Badge variant="secondary" size="s" content="Optional" /> | ||
| </Layout.Stack> | ||
| </Table.Cell> | ||
| <Table.Cell {root}>@</Table.Cell> | ||
| <Table.Cell {root}> | ||
| <InteractiveText variant="copy" isVisible text={`0 issue "certainly.com"`} /> | ||
| </Table.Cell> | ||
| </Table.Row.Base> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the CAA record value configurable.
The CAA record value is hardcoded to 0 issue "certainly.com". This should ideally be configurable through environment variables or settings, similar to how CNAME values are managed via $regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME.
Consider refactoring to make the CAA value configurable:
<Table.Row.Base {root}>
<Table.Cell {root}>
<Layout.Stack direction="row" alignItems="center" gap="xs">
CAA
<Badge variant="secondary" size="s" content="Optional" />
</Layout.Stack>
</Table.Cell>
<Table.Cell {root}>@</Table.Cell>
<Table.Cell {root}>
- <InteractiveText variant="copy" isVisible text={`0 issue "certainly.com"`} />
+ <InteractiveText variant="copy" isVisible text={$regionalConsoleVariables._APP_DOMAIN_CAA_RECORD || `0 issue "certainly.com"`} />
</Table.Cell>
</Table.Row.Base>📝 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.
| <Table.Row.Base {root}> | |
| <Table.Cell {root}> | |
| <Layout.Stack direction="row" alignItems="center" gap="xs"> | |
| CAA | |
| <Badge variant="secondary" size="s" content="Optional" /> | |
| </Layout.Stack> | |
| </Table.Cell> | |
| <Table.Cell {root}>@</Table.Cell> | |
| <Table.Cell {root}> | |
| <InteractiveText variant="copy" isVisible text={`0 issue "certainly.com"`} /> | |
| </Table.Cell> | |
| </Table.Row.Base> | |
| <Table.Row.Base {root}> | |
| <Table.Cell {root}> | |
| <Layout.Stack direction="row" alignItems="center" gap="xs"> | |
| CAA | |
| <Badge variant="secondary" size="s" content="Optional" /> | |
| </Layout.Stack> | |
| </Table.Cell> | |
| <Table.Cell {root}>@</Table.Cell> | |
| <Table.Cell {root}> | |
| <InteractiveText | |
| variant="copy" | |
| isVisible | |
| text={$regionalConsoleVariables._APP_DOMAIN_CAA_RECORD || `0 issue "certainly.com"`} | |
| /> | |
| </Table.Cell> | |
| </Table.Row.Base> |
🤖 Prompt for AI Agents
In src/lib/components/domains/cnameTable.svelte around lines 54 to 65 the CAA
record value is hardcoded to `0 issue "certainly.com"`; make it configurable by
reading the CAA value from the same config source used for CNAMEs (e.g. a
regionalConsoleVariables entry or an environment-derived prop), provide a
sensible default if unset, and pass that variable into the InteractiveText
component instead of the literal string; ensure the prop or store is
typed/validated and update any parent component or initialization to supply the
configurable CAA value.
| <Table.Row.Base {root}> | ||
| <Table.Cell {root}> | ||
| <Layout.Stack direction="row" alignItems="center" gap="xs"> | ||
| CAA | ||
| <Badge variant="secondary" size="s" content="Optional" /> | ||
| </Layout.Stack> | ||
| </Table.Cell> | ||
| <Table.Cell {root}>@</Table.Cell> | ||
| <Table.Cell {root}> | ||
| <InteractiveText variant="copy" isVisible text={`0 issue "certainly.com"`} /> | ||
| </Table.Cell> | ||
| </Table.Row.Base> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the CAA record value configurable.
Similar to the comment in cnameTable.svelte, the CAA record value should be configurable rather than hardcoded.
Consider using a configuration variable:
<Table.Cell {root}>
- <InteractiveText variant="copy" isVisible text={`0 issue "certainly.com"`} />
+ <InteractiveText variant="copy" isVisible text={$regionalConsoleVariables._APP_DOMAIN_CAA_RECORD || `0 issue "certainly.com"`} />
</Table.Cell>📝 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.
| <Table.Row.Base {root}> | |
| <Table.Cell {root}> | |
| <Layout.Stack direction="row" alignItems="center" gap="xs"> | |
| CAA | |
| <Badge variant="secondary" size="s" content="Optional" /> | |
| </Layout.Stack> | |
| </Table.Cell> | |
| <Table.Cell {root}>@</Table.Cell> | |
| <Table.Cell {root}> | |
| <InteractiveText variant="copy" isVisible text={`0 issue "certainly.com"`} /> | |
| </Table.Cell> | |
| </Table.Row.Base> | |
| <Table.Row.Base {root}> | |
| <Table.Cell {root}> | |
| <Layout.Stack direction="row" alignItems="center" gap="xs"> | |
| CAA | |
| <Badge variant="secondary" size="s" content="Optional" /> | |
| </Layout.Stack> | |
| </Table.Cell> | |
| <Table.Cell {root}>@</Table.Cell> | |
| <Table.Cell {root}> | |
| <InteractiveText | |
| variant="copy" | |
| isVisible | |
| text={$regionalConsoleVariables._APP_DOMAIN_CAA_RECORD || `0 issue "certainly.com"`} | |
| /> | |
| </Table.Cell> | |
| </Table.Row.Base> |
| </Table.Cell> | ||
| <Table.Cell {root}>@</Table.Cell> | ||
| <Table.Cell {root}> | ||
| <InteractiveText variant="copy" isVisible text={`0 issue "certainly.com"`} /> |
| @@ -51,6 +51,18 @@ | |||
| text={$regionalConsoleVariables._APP_DOMAIN_TARGET_CNAME} /> | |||
| </Table.Cell> | |||
| </Table.Row.Base> | |||
| <Table.Row.Base {root}> | |||
There was a problem hiding this comment.
Hide if we dont have caa in env var
What does this PR do?
Allow CNAME tab for apex domains when creating rules
Remove info box about CCA record, instead, add it to a table (all domains, in CNAME tab)
Add alert box when cname is used for apex domain, with copy telling not all providers support this
Consider adding CTA for "use nameserver instead" to switch tab
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
UI/UX