feat: implement email verification flow with route guard and modal #2398
Conversation
ConsoleProject ID: Sites (2)
Note Cursor pagination performs better than offset pagination when loading further pages. |
WalkthroughAdds an 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/routes/(console)/verify-email/+page.svelte (1)
64-70: Consider pausing polling once verified to reduce churn.You already redirect on verification; the cleanup runs, so this is OK. Optional: guard the interval by
if (!data.user?.emailVerification)before starting it.src/lib/components/account/sendVerificationEmailModal.svelte (4)
36-38: Namespace resend-timer keys per account to avoid cross-account collisions.A single key can leak state across users or tabs.
Apply this diff:
- const TIMER_END_KEY = 'email_verification_timer_end'; - const EMAIL_SENT_KEY = 'email_verification_sent'; + const keySuffix = (email || get(user)?.$id || 'anon'); + const TIMER_END_KEY = `email_verification_timer_end:${keySuffix}`; + const EMAIL_SENT_KEY = `email_verification_sent:${keySuffix}`;
20-20: Use portable timer type.Avoid Node‐only typings in the browser.
Apply this diff:
-let timerInterval: NodeJS.Timeout | null = null; +let timerInterval: ReturnType<typeof setInterval> | null = null;
72-86: Prevent duplicate intervals when restarting the countdown.Clear any existing interval before creating a new one.
Apply this diff:
function startTimerCountdown(timerEndTime: number) { - timerInterval = setInterval(() => { + if (timerInterval) clearInterval(timerInterval); + timerInterval = setInterval(() => {
127-135: Don’t clear localStorage on destroy if you want the timer to persist across navigations.This contradicts the PR goal (“persists state in localStorage”).
Apply this diff:
onDestroy(() => { if (timerInterval) { clearInterval(timerInterval); } - if (browser) { - localStorage.removeItem(TIMER_END_KEY); - localStorage.removeItem(EMAIL_SENT_KEY); - } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/lib/components/account/sendVerificationEmailModal.svelte(2 hunks)src/routes/(console)/+layout.svelte(1 hunks)src/routes/(console)/verify-email/+page.svelte(1 hunks)src/routes/(console)/verify-email/+page.ts(1 hunks)src/routes/+layout.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/routes/(console)/verify-email/+page.ts (1)
src/routes/+layout.ts (1)
load(17-70)
src/routes/+layout.ts (1)
src/lib/system.ts (1)
isCloud(24-24)
🪛 GitHub Actions: Tests
src/lib/components/account/sendVerificationEmailModal.svelte
[error] 16-16: Error: 'show' is declared but its value is never read. (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). (1)
- GitHub Check: e2e
🔇 Additional comments (3)
src/routes/(console)/+layout.svelte (1)
48-48: Import looks correct and matches usage.
BottomModalAlertis imported and rendered at Line 365. No issues.src/routes/(console)/verify-email/+page.ts (1)
3-9: Load function is correct and minimal.Re-exposes
accountasuserfor the page. All good.src/routes/+layout.ts (1)
32-40: Tighten route-guard: block unverified users from(authenticated)routes and coerce checksRemove the
(authenticated)exemption so unverified cloud users are redirected to /verify-email, and coerceroute.idchecks to Booleans to avoidundefined.- if (isCloud && !account.emailVerification) { - const isPublicRoute = route.id?.startsWith('/(public)'); - const isAuthRoute = route.id?.startsWith('/(authenticated)'); - const isVerifyEmailPage = url.pathname === `${base}/verify-email`; - - if (!isPublicRoute && !isAuthRoute && !isVerifyEmailPage) { - redirect(303, `${base}/verify-email`); - } - } + if (isCloud && !account.emailVerification) { + const isPublicRoute = Boolean(route.id?.startsWith('/(public)')); + const isVerifyEmailPage = url.pathname === `${base}/verify-email`; + if (!isPublicRoute && !isVerifyEmailPage) { + redirect(303, `${base}/verify-email`); + } + }
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/lib/components/account/sendVerificationEmailModal.svelte (5)
36-39: Namespace timer keys per account to avoid cross-account collisions.Current keys are global; switching accounts on the same device can incorrectly throttle resends.
-// Manage resend timer in localStorage -const TIMER_END_KEY = 'email_verification_timer_end'; -const EMAIL_SENT_KEY = 'email_verification_sent'; +// Manage resend timer in localStorage (namespace by user) +const KEY_PREFIX = (get(user)?.$id || email || 'anon'); +const TIMER_END_KEY = `verify_timer_end:${KEY_PREFIX}`; +const EMAIL_SENT_KEY = `verify_sent:${KEY_PREFIX}`;
20-20: Timer type: useReturnType<typeof setInterval>for browser compatibility.-let timerInterval: NodeJS.Timeout | null = null; +let timerInterval: ReturnType<typeof setInterval> | null = null;
56-56: Specify radix inparseInt.-const timerEndTime = parseInt(savedTimerEnd); +const timerEndTime = parseInt(savedTimerEnd, 10);
94-96: Remove redundantemailSent = true(set bystartResendTimer()).- await sdk.forConsole.account.createVerification({ url: cleanUrl }); - emailSent = true; - startResendTimer(); + await sdk.forConsole.account.createVerification({ url: cleanUrl }); + startResendTimer();
160-164: Accessibility nit: announce countdown updates.Consider
aria-live="polite"so screen readers are informed of the changing timer.-<Typography.Text color="neutral-secondary"> +<Typography.Text color="neutral-secondary" aria-live="polite"> Didn't get the email? Try again in {resendTimer}s </Typography.Text>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lib/components/account/sendVerificationEmailModal.svelte(2 hunks)src/lib/components/alerts/emailVerificationBanner.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 (4)
src/lib/components/alerts/emailVerificationBanner.svelte (1)
35-35: Pass-through email prop looks correct.
email={$user?.email}matches the modal’s public API and avoids undefined crashes with optional chaining.src/lib/components/account/sendVerificationEmailModal.svelte (3)
22-32: Always invalidate and redirect on logout; simplify handler.Avoid a non‑redirecting path that can leave stale account state. Also bind the handler directly.
-async function logout(redirect = true) { +async function logout() { try { await sdk.forConsole.account.deleteSession({ sessionId: 'current' }); - if (redirect) { - await invalidate(Dependencies.ACCOUNT); - goto(`${base}/login`); - } + await invalidate(Dependencies.ACCOUNT); + goto(`${base}/login`); } catch (error) { console.error('Logout error:', error); } } @@ -<Link variant="default" on:click={() => logout()}>Switch account</Link> +<Link variant="default" on:click={logout}>Switch account</Link>Also applies to: 157-157
139-175: Scrim is always rendered → gate it behindshow.The full-page scrim overlays the app even when the modal is hidden. Render the scrim conditionally. This is the same issue previously flagged.
-{@html ''}<!-- marker --> -<div class="email-verification-scrim"> - <Modal - bind:show +{#if show} +<div class="email-verification-scrim"> + <Modal + bind:show title="Verify your email address" {onSubmit} dismissible={false} autoClose={false}> @@ - </Modal> -</div> + </Modal> +</div> +{/if}
1-1: Sanity-check SvelteKit 'invalidate' usageConfirm the project targets SvelteKit v2+ (which exports invalidate from $app/navigation). If not, replace uses of invalidate with invalidateAll or standardize imports to $app/navigation to avoid build errors.
Location: src/lib/components/account/sendVerificationEmailModal.svelte (script block).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/lib/components/account/sendVerificationEmailModal.svelte (5)
36-39: Namespace timer keys per account/email to prevent cross‑account lockouts.If a user logs out and another logs in, the new user inherits the previous timer. Scope keys by email or user ID.
-// Manage resend timer in localStorage -const TIMER_END_KEY = 'email_verification_timer_end'; -const EMAIL_SENT_KEY = 'email_verification_sent'; +// Manage resend timer in localStorage (scoped per account/email) +const keySuffix = $derived(email || $user?.$id || 'anon'); +const TIMER_END_KEY = $derived(`email_verification_timer_end:${keySuffix}`); +const EMAIL_SENT_KEY = $derived(`email_verification_sent:${keySuffix}`);
40-49: Prevent duplicate intervals when restarting the timer.Clear any existing interval before starting a new countdown.
function startResendTimer() { const timerEndTime = Date.now() + 60 * 1000; resendTimer = 60; emailSent = true; if (browser) { localStorage.setItem(TIMER_END_KEY, timerEndTime.toString()); localStorage.setItem(EMAIL_SENT_KEY, 'true'); } + if (timerInterval) { + clearInterval(timerInterval); + timerInterval = null; + } startTimerCountdown(timerEndTime); } @@ function startTimerCountdown(timerEndTime: number) { - timerInterval = setInterval(() => { + if (timerInterval) { + clearInterval(timerInterval); + } + timerInterval = setInterval(() => { const now = Date.now(); const remainingTime = Math.max(0, Math.ceil((timerEndTime - now) / 1000)); resendTimer = remainingTime; if (remainingTime <= 0) { clearInterval(timerInterval); timerInterval = null; if (browser) { localStorage.removeItem(TIMER_END_KEY); localStorage.removeItem(EMAIL_SENT_KEY); } } }, 1000); }Also applies to: 73-87
151-154: Make the displayed email reactive (avoid one‑timeget(user)).Use
$userso updates propagate; drop the unusedgetimport.-import { get } from 'svelte/store'; @@ - style="display: inline;">{email || get(user)?.email}</Typography.Text> + style="display: inline;">{email || $user?.email}</Typography.Text>Also applies to: 7-7
94-96: Remove redundant state write.
startResendTimer()already setsemailSent = true.- emailSent = true; - startResendTimer(); + startResendTimer();
41-41: Parse int safely.Be explicit with radix or use
Number(...)to avoid surprises.- const timerEndTime = Date.now() + 60 * 1000; + const timerEndTime = Date.now() + 60 * 1000; @@ - const timerEndTime = parseInt(savedTimerEnd); + const timerEndTime = Number(savedTimerEnd); - const remainingTime = Math.max(0, Math.ceil((timerEndTime - now) / 1000)); + const remainingTime = Math.max(0, Math.ceil((timerEndTime - now) / 1000));Also applies to: 58-59
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lib/components/account/sendVerificationEmailModal.svelte(2 hunks)src/routes/(console)/verify-email/+page.svelte(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/routes/(console)/verify-email/+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 (4)
src/lib/components/account/sendVerificationEmailModal.svelte (4)
22-32: Logout should always invalidate + redirect; also clear timer keys to avoid cross‑account bleed.No caller passes
falsenow; simplify API and prevent timer leakage when switching accounts.[Suggest_recommended_refactor]
-async function logout(redirect = true) { +async function logout() { try { await sdk.forConsole.account.deleteSession({ sessionId: 'current' }); - if (redirect) { - await invalidate(Dependencies.ACCOUNT); - goto(`${base}/login`); - } + await invalidate(Dependencies.ACCOUNT); + if (browser) { + localStorage.removeItem(TIMER_END_KEY); + localStorage.removeItem(EMAIL_SENT_KEY); + } + goto(`${base}/login`); } catch (error) { console.error('Logout error:', error); } } @@ -<Link variant="default" on:click={() => logout()}>Switch account</Link> +<Link variant="default" on:click={logout}>Switch account</Link>Run to confirm no other call sites rely on
logout(false):#!/bin/bash rg -nC2 -g '!**/dist/**' -P '\blogout\s*\('Also applies to: 156-158
8-9: Use public $app/stores and the reactive $page store.
$app/stateis private. Switch to$app/storesand reference$pagefor reactivity. Also preferconstfor the derived URL.-import { page } from '$app/state'; +import { page } from '$app/stores'; @@ -let cleanUrl = $derived(page.url.origin + page.url.pathname); +const cleanUrl = $derived($page.url.origin + $page.url.pathname); @@ - const searchParams = page.url.searchParams; + const searchParams = $page.url.searchParams;Also applies to: 34-35, 103-107
139-174: Scrim is always rendered → gate byshowto avoid permanent blur.The wrapper div renders regardless of modal state, blocking the app even when
show === false.-{#key Math.random()} -<div class="email-verification-scrim"> +{#if show} +<div class="email-verification-scrim"> <Modal - bind:show + bind:show title="Verify your email address" {onSubmit} dismissible={false} autoClose={false}> ... </Modal> </div> +{/if}
128-136: Don’t clear localStorage in onDestroy — breaks the “persist timer across navigation/refresh” requirement.Only clear on timer expiry (already handled) or on explicit logout.
onDestroy(() => { if (timerInterval) { clearInterval(timerInterval); } - if (browser) { - localStorage.removeItem(TIMER_END_KEY); - localStorage.removeItem(EMAIL_SENT_KEY); - } });
| let sideBarIsOpen = writable(false); | ||
| let showAccountMenu = writable(false); | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any |
There was a problem hiding this comment.
we don't need this if no any is used.
There was a problem hiding this comment.
I feel using any here might be more practical than defining all the properties inline, since otherwise we’d need to add all the props explicitly. What do you think?
let project: any = { $id: 'verify-email-project', region: 'us-east-1', name: 'Verify Email Project' };
There was a problem hiding this comment.
only added the dummy ones and casted to Models.Project
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lib/components/account/sendVerificationEmailModal.svelte(2 hunks)src/routes/(console)/verify-email/+page.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 (6)
src/lib/components/account/sendVerificationEmailModal.svelte (2)
147-150: Keep the resend lock persisted across navigationClearing the localStorage keys in
onDestroywipes the resend lock any time the modal unmounts (e.g. navigation or refresh), letting users immediately resend and defeating the throttling requirement we added. Please drop this cleanup so the timer truly persists. This was already flagged earlier.- if (browser) { - localStorage.removeItem(TIMER_END_KEY); - localStorage.removeItem(EMAIL_SENT_KEY); - }
154-190: Scrim must disappear whenshowis falseBecause the scrim wrapper is always rendered, the full-screen overlay and blur stay active even when
showisfalse, effectively blocking the entire console. Wrap the scrim + modal in a{#if show}so the overlay only mounts when the modal is open. This mirrors the earlier feedback on the same point.-<div class="email-verification-scrim"> - <Modal - bind:show +{#if show} +<div class="email-verification-scrim"> + <Modal + bind:show @@ - </Modal> -</div> + </Modal> +</div> +{/if}src/routes/(console)/verify-email/+page.svelte (4)
16-20: Consider using a proper TypeScript type instead ofany.While the inline comment acknowledges the use of
any, it's better to define a proper interface or type for the project object to maintain type safety.Define a minimal interface for the mock project:
- // eslint-disable-next-line @typescript-eslint/no-explicit-any - let project: any = { + interface MockProject { + $id: string; + region: string; + name: string; + } + let project: MockProject = { $id: 'verify-email-project', region: 'us-east-1', name: 'Verify Email Project' };
106-111: Remove unused CSS class.The
.main-contentclass is defined but not used anywhere in the template.Remove the unused CSS:
- .main-content { - flex: 1; - padding: 2rem; - margin-left: 190px; - min-height: 100vh; - }
112-141: Consider avoiding global selectors for better maintainability.Using
:global()selectors can lead to unintended side effects and makes styles harder to maintain. Consider using CSS custom properties or class-based styling instead.Consider using a more maintainable approach with CSS custom properties or data attributes:
- :global(.verify-email-page .sidebar) { + .verify-email-page :global(.sidebar) { position: fixed !important; left: 0 !important; top: 0 !important; height: 100vh !important; z-index: 1000 !important; filter: blur(4px); opacity: 0.6; }This scopes the global selector within the component's context, reducing the risk of unintended side effects.
51-69: Potential memory leak with interval not being cleared on early returns.The interval cleanup function is only returned if execution reaches line 68, but there are early returns at lines 54 and 60 that would prevent the cleanup function from being registered.
Store the interval reference and ensure cleanup in all paths:
onMount(() => { + let interval: NodeJS.Timeout | undefined; + if (!page.data.account) { goto(`${base}/login`); return; } // If email is already verified, redirect immediately if (page.data.account?.emailVerification) { checkEmailVerification(); return; } - const interval = setInterval(async () => { + interval = setInterval(async () => { await invalidate(Dependencies.ACCOUNT); checkEmailVerification(); }, 2000); return () => clearInterval(interval); });Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/components/account/sendVerificationEmailModal.svelte (1)
114-116: Use$pagefor URL params.Avoid reading the store object itself.
-const searchParams = page.url.searchParams; +const searchParams = $page.url.searchParams;
🧹 Nitpick comments (3)
src/routes/(console)/verify-email/+page.svelte (2)
15-21: Avoidany; provide a minimal type or narrow the shape.Use a light type to satisfy Navbar/Sidebar props.
-// eslint-disable-next-line @typescript-eslint/no-explicit-any -let project: any = { +type ProjectShape = { $id: string; region: string; name: string }; +let project: ProjectShape = { $id: 'verify-email-project', region: 'us-east-1', name: 'Verify Email Project' };Also applies to: 27-35
63-69: Optional: await check after invalidation.Ensures navigation happens after the account state refresh completes.
- const interval = setInterval(async () => { - await invalidate(Dependencies.ACCOUNT); - checkEmailVerification(); - }, 2000); + const interval = setInterval(async () => { + await invalidate(Dependencies.ACCOUNT); + await checkEmailVerification(); + }, 2000);src/lib/components/account/sendVerificationEmailModal.svelte (1)
23-30: Logout flow: always invalidate and redirect.Simplify API; remove the
redirectflag to avoid stale state.-async function logout(redirect = true) { +async function logout() { try { await sdk.forConsole.account.deleteSession({ sessionId: 'current' }); - if (redirect) { - await invalidate(Dependencies.ACCOUNT); - goto(`${base}/login`); - } + await invalidate(Dependencies.ACCOUNT); + goto(`${base}/login`); } catch (error) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lib/components/account/sendVerificationEmailModal.svelte(2 hunks)src/routes/(console)/verify-email/+page.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: build
- GitHub Check: e2e
🔇 Additional comments (5)
src/lib/components/account/sendVerificationEmailModal.svelte (5)
94-111: Good: send flow guards, persistence kick-off, and user feedback.The createVerification guard, timer start, and error notification look correct.
Consider verifying that the
urlpoints to the same route hosting this modal (e.g., base/verify-email) in environments with non-rootbasepaths.
8-8: Use the public page store.Switch from
$app/stateto$app/storesto avoid private API usage.-import { page } from '$app/state'; +import { page } from '$app/stores';
143-151: Don’t clear localStorage on destroy — defeats persistence.Keep keys until timer expiry or explicit reset.
onDestroy(() => { if (timerInterval) { clearInterval(timerInterval); } - if (browser) { - localStorage.removeItem(TIMER_END_KEY); - localStorage.removeItem(EMAIL_SENT_KEY); - } });
154-190: Scrim always renders → gate byshow.Wrap the scrim and pass
showto Modal to prevent a permanent overlay.-<div class="email-verification-scrim"> - <Modal - bind:show +{#if show} +<div class="email-verification-scrim"> + <Modal + bind:show title="Verify your email address" {onSubmit} dismissible={false} autoClose={false}> @@ - </Modal> -</div> + </Modal> +</div> +{/if}
39-39: Fix$derivedsource: use$page.Current expression won’t react/update.
-const cleanUrl = $derived(page.url.origin + page.url.pathname); +const cleanUrl = $derived($page.url.origin + $page.url.pathname);
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 (1)
src/lib/components/account/sendVerificationEmailModal.svelte (1)
111-134: Ensure base route is allowed post-verificationThe guard in
+layout.tsonly exempts public, auth, and/verify-emailpages. Root (${base}/) is neither a public nor auth route, so users will be redirected back to/verify-email, causing a loop. Update the guard to also allow the base path:- const isVerifyEmailPage = url.pathname === `${base}/verify-email`; + const isVerifyEmailPage = url.pathname === `${base}/verify-email`; + const isBasePage = url.pathname === `${base}/` || url.pathname === `${base}`; - if (!isPublicRoute && !isAuthRoute && !isVerifyEmailPage) { - redirect(303, `${base}/verify-email`); + if (!isPublicRoute && !isAuthRoute && !isVerifyEmailPage && !isBasePage) { + redirect(303, `${base}/verify-email`); }
♻️ Duplicate comments (4)
src/routes/(console)/verify-email/+page.svelte (3)
11-11: Use the public$app/storesAPI and reactive$pagestore.Line 11 imports
pagefrom the private$app/statemodule. This import is not part of SvelteKit's public API and won't provide reactive updates. All references topage.datathroughout this file (lines 38, 41, 47, 53, 59, 102) will read stale values.Apply this diff:
-import { page } from '$app/state'; +import { page } from '$app/stores';Then update all usages to the reactive
$pagestore:-let showVerificationModal = $state(!page.data.account?.emailVerification); +let showVerificationModal = $state(!$page.data.account?.emailVerification); $effect(() => { - if (page.data.account?.emailVerification) { + if ($page.data.account?.emailVerification) { checkEmailVerification(); } }); async function checkEmailVerification() { - if (page.data.account?.emailVerification) { + if ($page.data.account?.emailVerification) { await goto(`${base}/`); } } onMount(() => { - if (!page.data.account) { + if (!$page.data.account) { goto(`${base}/login`); return; } - if (page.data.account?.emailVerification) { + if ($page.data.account?.emailVerification) { checkEmailVerification(); return; } // ... });And in the template:
<SendVerificationEmailModal bind:show={showVerificationModal} - email={page.data.account?.email} /> + email={$page.data.account?.email} />
14-15: Replace writable stores with plain boolean variables for two‑way binding.Lines 14-15 create writable stores, but Svelte's
bind:directive cannot bind to$storevalues (lines 88-89, 92-93). This will cause runtime errors.Apply this diff:
-import { writable } from 'svelte/store'; +// Removed writable - using plain booleans for bind: ... -let sideBarIsOpen = writable(false); -let showAccountMenu = writable(false); +let sideBarIsOpen = false; +let showAccountMenu = false;<Navbar {...navbarProps} - bind:sideBarIsOpen={$sideBarIsOpen} - bind:showAccountMenu={$showAccountMenu} /> + bind:sideBarIsOpen + bind:showAccountMenu /> <Sidebar - bind:sideBarIsOpen={$sideBarIsOpen} - bind:showAccountMenu={$showAccountMenu} + bind:sideBarIsOpen + bind:showAccountMenu {project} {avatar} {progressCard} state="open" />
38-50: Remove redundant$effectblock.Lines 40-44 duplicate the same verification check already performed in
onMount(lines 59-61). ThecheckEmailVerificationfunction also redundantly checksemailVerificationbefore redirecting. This creates multiple reactive paths for the same logic.Apply this diff:
let showVerificationModal = $state(!$page.data.account?.emailVerification); -$effect(() => { - if ($page.data.account?.emailVerification) { - checkEmailVerification(); - } -}); - async function checkEmailVerification() { - if ($page.data.account?.emailVerification) { - await goto(`${base}/`); - } + await goto(`${base}/`); }The
onMountlogic already handles the initial check, and the realtime subscription + interval will callcheckEmailVerificationwhen the account updates.src/lib/components/account/sendVerificationEmailModal.svelte (1)
8-8: Use the public$app/storesAPI for reactive page access.Line 8 imports
pagefrom$app/state, which is a private SvelteKit module. Although a past review comment indicates this was "addressed in commit e84df2d", the import is still using the private API in the current code.Apply this diff:
-import { page } from '$app/state'; +import { page } from '$app/stores';Then update the derived and function that reference
page:-const cleanUrl = $derived(page.url.origin + page.url.pathname); +const cleanUrl = $derived($page.url.origin + $page.url.pathname); async function updateEmailVerification() { - const searchParams = page.url.searchParams; + const searchParams = $page.url.searchParams; const userId = searchParams.get('userId'); const secret = searchParams.get('secret'); // ... }
🧹 Nitpick comments (3)
src/routes/(console)/verify-email/+page.svelte (3)
16-21: Removeanytype and define a minimal project interface.The
anytype with an eslint-disable comment defeats type safety. Since this is mock data for the verification page, define a minimal type.Apply this diff:
- // eslint-disable-next-line @typescript-eslint/no-explicit-any - let project: any = { + let project = { $id: 'verify-email-project', region: 'us-east-1', name: 'Verify Email Project' };The type will be inferred from the object literal. If Navbar/Sidebar require specific types, import and use those instead.
69-72: Polling interval is too aggressive; rely on realtime.The 10-second polling interval (line 72) is excessive given that realtime subscriptions are already in place. This creates unnecessary API load and could cause rate limiting.
Based on the past review comments, a reviewer suggested relying on realtime and using polling only as a backup. Apply this diff:
const interval = setInterval(async () => { await invalidate(Dependencies.ACCOUNT); checkEmailVerification(); - }, 10000); + }, 30000); // 30s fallback in case realtime failsOr consider removing the interval entirely if realtime is reliable in your environment.
105-145: Review global style overrides—may affect other pages.Lines 114-143 use
:globalselectors with!importantto blur navbar and sidebar while keeping the modal sharp. If this component is ever rendered alongside other modals or overlays, these global styles could interfere.Consider scoping the blur effects more tightly:
:global { - .sidebar { + .verify-email-page .sidebar { position: fixed !important; left: 0 !important; top: 0 !important; height: 100vh !important; z-index: 1000 !important; filter: blur(4px); opacity: 0.6; } - .navbar, - [data-pink-navbar], - header { + .verify-email-page .navbar, + .verify-email-page [data-pink-navbar], + .verify-email-page header { filter: blur(2px); opacity: 0.4; z-index: 1; } - .email-verification-scrim { + .verify-email-page .email-verification-scrim { z-index: 9999 !important; filter: none !important; opacity: 1 !important; * { filter: none !important; opacity: 1 !important; } } }This ensures the styles only apply within
.verify-email-page.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lib/components/account/sendVerificationEmailModal.svelte(2 hunks)src/routes/(console)/verify-email/+page.svelte(1 hunks)
🔇 Additional comments (6)
src/lib/components/account/sendVerificationEmailModal.svelte (6)
23-35: LGTM! Logout correctly invalidates and redirects.The logout function properly deletes the session, invalidates account dependencies, navigates to login, and handles errors with user notifications.
43-90: Timer persistence logic is well-structured.The localStorage-based timer persistence across page loads and navigation is correctly implemented:
startResendTimerstores the end time and starts countdownrestoreTimerStateresumes the timer on mount if still validstartTimerCountdowncleans up expired timers
92-109: Email sending flow correctly enforces timer lock.The
onSubmitfunction properly checksresendTimer > 0to prevent premature resends, starts the timer on success, and notifies users on error.
136-149: Consider whether localStorage should persist across sessions.Lines 141-149 clear the timer state from localStorage on component destroy. A past review comment suggested removing this cleanup to preserve the timer across navigation, but the reviewer (ItzNotABug) disagreed, stating "we should keep for later page load as well."
The current implementation clears localStorage on every unmount, which means:
- ✅ Timer persists across page refreshes (onMount restores state before onDestroy runs)
- ❌ Timer is cleared when navigating away from the verify-email page
If users navigate to another tab or close the modal and return later, the timer resets. Is this the intended behavior? If the timer should persist across sessions (even after closing the page), remove the localStorage cleanup from
onDestroy:onDestroy(() => { if (timerInterval) { clearInterval(timerInterval); } - if (browser) { - localStorage.removeItem(TIMER_END_KEY); - localStorage.removeItem(EMAIL_SENT_KEY); - } });This way, the timer expires naturally after 60 seconds regardless of navigation.
152-187: Modal is now properly bound and conditionally rendered.Lines 152-187 correctly bind the
showprop to the Modal component and wrap it in a scrim. The previous issue whereshow={true}forced the modal to always be visible has been resolved.
170-170: Simplify logout call—redirectparameter is always true.Line 170 calls
logout()with no arguments. The function signature no longer includes aredirectparameter (removed in response to past review comments), so this is correct. However, a past review comment noted thatlogout(false)was never used, confirming the parameter was redundant.The logout flow is now clean and straightforward.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (8)
src/routes/(console)/verify-email/+page.svelte (5)
2-2: Use the publicpagestore from$app/stores.
$app/stateis a private/internal API. Use the public store from$app/storesfor compatibility and reactivity.Apply this diff:
- import { page } from '$app/state'; + import { page } from '$app/stores';
7-9: Replace writable stores with plain variables forbind:compatibility.You cannot use
bind:with store values (e.g.,bind:sideBarIsOpen={$sideBarIsOpen}). Svelte'sbind:directive requires plain variables, not reactive store subscriptions.Apply this diff:
- import { writable } from 'svelte/store'; - - let sideBarIsOpen = writable(false); - let showAccountMenu = writable(false); + let sideBarIsOpen = false; + let showAccountMenu = false; let showVerificationModal = $state(!page.data.account?.emailVerification);
9-9: Use$page.datafor reactive access.Accessing
page.datawithout the$prefix won't be reactive. Once you switch to$app/stores, use$pageto ensure reactivity.Apply this diff:
- let showVerificationModal = $state(!page.data.account?.emailVerification); + let showVerificationModal = $state(!$page.data.account?.emailVerification);
34-45: Update bindings to use plain variables.After converting the stores to plain variables, update the bindings accordingly.
Apply this diff:
<Navbar {...navbarProps} - bind:sideBarIsOpen={$sideBarIsOpen} - bind:showAccountMenu={$showAccountMenu} /> + bind:sideBarIsOpen + bind:showAccountMenu /> <Sidebar - bind:sideBarIsOpen={$sideBarIsOpen} - bind:showAccountMenu={$showAccountMenu} + bind:sideBarIsOpen + bind:showAccountMenu {project}
48-50: Pass email from$pagefor reactivity.Use
$page.datato ensure the email value updates when the page store changes.Apply this diff:
<SendVerificationEmailModal bind:show={showVerificationModal} - email={page.data.account?.email} /> + email={$page.data.account?.email} />src/lib/components/account/sendVerificationEmailModal.svelte (3)
8-8: Use the publicpagestore from$app/stores.Although a past review comment suggests this was addressed, the current code still imports from the private
$app/stateAPI. Use$app/storesfor the public, stable API.Apply this diff:
- import { page } from '$app/state'; + import { page } from '$app/stores';
119-128: Remove localStorage clearing fromonDestroyto preserve timer state.Clearing
localStorageon component unmount defeats the persistence feature. The timer should persist across navigation and page reloads. Past review comments and maintainer feedback confirm this should be removed.Based on learnings
Apply this diff:
onDestroy(() => { if (timerInterval) { clearInterval(timerInterval); } - - if (browser) { - localStorage.removeItem(TIMER_END_KEY); - localStorage.removeItem(EMAIL_SENT_KEY); - } });
42-42: Use$page.urlfor reactive access.After switching to
$app/stores, use the$pagesubscription to ensure the URL value is reactive.Apply this diff:
- const cleanUrl = $derived(page.url.origin + page.url.pathname); + const cleanUrl = $derived($page.url.origin + $page.url.pathname);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/lib/components/account/sendVerificationEmailModal.svelte(1 hunks)src/lib/components/alerts/emailVerificationBanner.svelte(0 hunks)src/lib/components/index.ts(1 hunks)src/routes/(console)/verify-email/+page.svelte(1 hunks)src/routes/(console)/verify-email/+page.ts(1 hunks)src/routes/+layout.ts(2 hunks)
💤 Files with no reviewable changes (1)
- src/lib/components/alerts/emailVerificationBanner.svelte
🧰 Additional context used
🧬 Code graph analysis (2)
src/routes/(console)/verify-email/+page.ts (4)
src/routes/+layout.ts (1)
load(17-69)src/lib/system.ts (1)
VARS(9-16)src/lib/stores/sdk.ts (1)
sdk(147-170)src/lib/stores/notifications.ts (1)
addNotification(46-67)
src/routes/+layout.ts (1)
src/lib/system.ts (2)
isCloud(26-26)VARS(9-16)
⏰ 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/index.ts (1)
89-89: LGTM!The export correctly replaces the removed
EmailVerificationBannerwith the newSendVerificationEmailModalcomponent, maintaining consistency with the file's export pattern.src/routes/+layout.ts (3)
9-9: LGTM!The import of
resolvefrom$app/pathsis correctly added to support the verification redirect logic.
12-12: LGTM!The import correctly adds
VARSto access theEMAIL_VERIFICATIONfeature flag.
32-39: LGTM!The email verification guard correctly prevents unverified cloud users from accessing console routes while avoiding redirect loops by checking the current pathname.
src/routes/(console)/verify-email/+page.ts (3)
10-12: LGTM!The feature flag check correctly guards the verification flow and redirects when disabled.
14-15: LGTM!Correctly retrieves account data from parent and marks the dependency for cache invalidation.
17-44: LGTM!The verification flow correctly handles all cases:
- Redirects already-verified users
- Processes verification tokens with appropriate notifications
- Falls back to redirect when no tokens are present
The 10-second success notification timeout is reasonable given the reload time mentioned in the comment.
src/lib/components/account/sendVerificationEmailModal.svelte (4)
17-23: LGTM!The props definition correctly uses
$bindablefor two-way binding of theshowstate and accepts the
31-40: LGTM!The logout function correctly deletes the session, invalidates the account dependency, and navigates to the login page with proper error handling.
44-100: LGTM!The timer persistence logic is well-implemented:
- Stores timer state in
localStorage- Restores state on mount if still valid
- Clears expired timers appropriately
- Uses proper interval management
102-115: LGTM!The email sending logic correctly prevents duplicate sends during the timer countdown and handles errors appropriately.
| let sideBarIsOpen = writable(false); | ||
| let showAccountMenu = writable(false); | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any |
There was a problem hiding this comment.
only added the dummy ones and casted to Models.Project
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/lib/components/account/sendVerificationEmailModal.svelte (1)
119-128: Timer persistence is still defeated by clearing localStorage on destroy.Lines 124-127 remove the timer keys from localStorage when the component unmounts, which prevents the timer from persisting across navigation or page refresh. The
restoreTimerStatefunction expects these keys to exist on subsequent mounts.Based on the past review discussion, remove the localStorage cleanup from
onDestroyso the timer persists:onDestroy(() => { if (timerInterval) { clearInterval(timerInterval); } - - if (browser) { - localStorage.removeItem(TIMER_END_KEY); - localStorage.removeItem(EMAIL_SENT_KEY); - } });The localStorage cleanup is already handled correctly when the timer expires (lines 94-97).
🧹 Nitpick comments (1)
src/lib/components/account/sendVerificationEmailModal.svelte (1)
25-25: Specify the error type asstring | null.The error state is initialized as
nullbut assigned string values (lines 38, 111). For type safety, declare it explicitly.Apply this diff:
- let error = $state(null); + let error = $state<string | null>(null);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/components/account/sendVerificationEmailModal.svelte(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-05T09:41:40.421Z
Learnt from: ItzNotABug
PR: appwrite/console#2398
File: src/routes/(console)/verify-email/+page.svelte:48-51
Timestamp: 2025-10-05T09:41:40.421Z
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.
Applied to files:
src/lib/components/account/sendVerificationEmailModal.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 (3)
src/lib/components/account/sendVerificationEmailModal.svelte (3)
31-40: LGTM!The logout flow correctly invalidates account dependencies before redirecting to login, and includes proper error handling.
48-115: Timer and submission logic look solid.The implementation correctly:
- Persists timer state to localStorage with proper browser checks
- Restores timer state on mount (line 117 correctly passes the function directly)
- Prevents concurrent submissions and resends during active countdown
- Handles errors with internal state management
131-173: Modal implementation addresses previous concerns.The template now:
- Properly uses
bind:show(fixing the always-visible modal issue)- Binds the internal error state
- Shows email from prop or user store
- Displays countdown feedback
- Disables actions during creation or active timer
What does this PR do?
Test Plan
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?
yes
Summary by CodeRabbit
New Features
Removed