-
Notifications
You must be signed in to change notification settings - Fork 186
feat: added modal for email verfication #2354
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
ConsoleProject ID: Sites (2)
Note Cursor pagination performs better than offset pagination when loading further pages. |
WalkthroughAdds a new Svelte component src/lib/components/account/sendVerificationEmailModal.svelte that displays a modal to send an email verification via sdk.forConsole.account.createVerification using a cleanUrl (origin + pathname). It exposes a public show prop, prevents concurrent submissions with an internal creating flag, and pushes success/error notifications. Updates src/lib/components/alerts/emailVerificationBanner.svelte to simplify visibility logic, remove dismiss props/logic, and open the new modal via local state. Adjusts src/routes/(console)/+layout.svelte to use without props, reflecting the banner’s updated public API. Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal). Please share your feedback with us on this Discord post. ✨ 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/lib/components/alerts/emailVerificationBanner.svelte (2)
11-12
: Avoid hard-coded '/console' in route checkUse base from $app/paths to be robust to deployments behind a path prefix.
@@ -const notOnOnboarding = $derived(!page.url?.pathname?.includes('/console/onboarding')); +import { base } from '$app/paths'; +const notOnOnboarding = $derived(!page.url?.pathname?.includes(`${base}/onboarding`));
17-31
: UX OK; consider Cloud-only visibility gateIf email verification is enforced only on Cloud, gate the banner with isCloud to avoid noise on self-hosted.
+ import { isCloud } from '$lib/system'; - const shouldShowEmailBanner = $derived(hasUser && needsEmailVerification && notOnOnboarding); + const shouldShowEmailBanner = $derived(isCloud && hasUser && needsEmailVerification && notOnOnboarding);src/lib/components/account/sendVerificationEmailModal.svelte (3)
41-44
: Add Cancel button and a11y busy stateLet users back out and expose loading state to AT.
@@ - <svelte:fragment slot="footer"> - <Button submit disabled={creating}>Send email</Button> - </svelte:fragment> + <svelte:fragment slot="footer"> + <Button secondary on:click={() => (show = false)} disabled={creating}>Cancel</Button> + <Button submit disabled={creating} aria-busy={creating}>Send email</Button> + </svelte:fragment>
22-24
: Harden error messagingSDK errors aren’t always guaranteed to have .message. Normalize before display to avoid undefined.
- } catch (error) { - addNotification({ message: error.message, type: 'error' }); + } catch (error) { + const message = typeof error === 'object' && error && 'message' in error + ? String((error as any).message) + : 'Failed to send verification email'; + addNotification({ message, type: 'error' });
13-21
: Use a canonical verification return URL (not the current page path)Change src/lib/components/account/sendVerificationEmailModal.svelte to stop sending page.url.pathname as the return URL and instead point the verification link to the route that finalizes verification (src/routes/(console)/account/updateMfa.svelte — it calls updateVerification with userId/secret). This ensures the emailed link always lands on a handler that processes the userId/secret (e.g., use
${page.url.origin}${base}/account/updateMfa
or similar).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/lib/components/account/sendVerificationEmailModal.svelte
(1 hunks)src/lib/components/alerts/emailVerificationBanner.svelte
(1 hunks)src/routes/(console)/+layout.svelte
(1 hunks)
🔇 Additional comments (2)
src/routes/(console)/+layout.svelte (1)
349-349
: Verify placement: banner rendered outside ShellRendering after may bypass header layout constraints. If HeaderAlert is expected to live within Shell’s layout region, consider moving it inside Shell (e.g., right after ) or confirm CSS still positions it correctly across routes.
src/lib/components/alerts/emailVerificationBanner.svelte (1)
11-15
: Svelte 5 present — confirm runes are enabled before using $state/$derivedpackage.json shows "svelte": "^5.25.3" but svelte.config.* contains no
runes: true
. If this repo uses SvelteKit (SvelteKit automatically enables Svelte 5 runes) the new$state
/$derived
are fine; otherwise enable runes in svelte.config or revert to classic reactivity. Fallback (classic reactivity) diff:@@ -const hasUser = $derived(!!$user); -const needsEmailVerification = $derived(hasUser && !$user.emailVerification); -const notOnOnboarding = $derived(!page.url?.pathname?.includes('/console/onboarding')); -const shouldShowEmailBanner = $derived(hasUser && needsEmailVerification && notOnOnboarding); - -let showSendVerification = $state(false); +$: hasUser = !!$user; +$: needsEmailVerification = hasUser && !$user.emailVerification; +$: notOnOnboarding = !page.url?.pathname?.includes('/console/onboarding'); +$: shouldShowEmailBanner = hasUser && needsEmailVerification && notOnOnboarding; + +let showSendVerification = false;
What does this PR do?
(Provide a description of what this PR does.)
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
Changes