feat: implement blocking email verification modal #2379
Conversation
ConsoleProject ID: Sites (2)
Note Cursor pagination performs better than offset pagination when loading further pages. |
|
Warning Rate limit exceeded@HarshMN2345 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 3 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
Walkthrough
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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: 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)
116-139: Scrub verification secrets from the URL after successful verification.Leaving
userIdandsecretin the address bar is a security/privacy risk (leaks via history, screenshots, support logs). Replace the URL withcleanUrlon success.Apply this diff inside the success branch:
try { await sdk.forConsole.account.updateVerification({ userId, secret }); addNotification({ message: 'Email verified successfully', type: 'success' }); await Promise.all([ invalidate(Dependencies.ACCOUNT), invalidate(Dependencies.FACTORS) ]); + // Remove sensitive query params from the URL without a navigation + if (browser) { + history.replaceState({}, '', cleanUrl); + } } catch (error) {
🧹 Nitpick comments (5)
src/lib/components/account/sendVerificationEmailModal.svelte (4)
195-209: Add a z-index to ensure the scrim truly blocks the console.Without an explicit z-index, fixed/sticky elements with higher stacking contexts can sit above the scrim.
.email-verification-scrim { position: fixed; top: 0; left: 0; width: 100%; height: 100%; background-color: hsl(240 5% 8% / 0.6); backdrop-filter: blur(4px); display: flex; align-items: center; justify-content: center; + z-index: 1000; /* ensure it overlays console chrome */ }
168-174: Prefer reactive$useroverget(user)in markup.Using
get()makes the email non-reactive and adds an unnecessary import.- style="display: inline;">{get(user)?.email}</Typography.Text> + style="display: inline;">{$user?.email}</Typography.Text>Also remove the unused import:
- import { get } from 'svelte/store';
60-60: Specify radix for parseInt.Be explicit with base-10 to avoid surprises.
- const timerEndTime = parseInt(savedTimerEnd); + const timerEndTime = parseInt(savedTimerEnd, 10);
176-177: Ensure “Switch account” is keyboard-accessible.If
Linkrequireshreffor focusability, switch to a button-styled control or providehrefand prevent default in the handler.Example:
-<Link variant="default" on:click={() => logout(false)}>Switch account</Link> +<Button variant="link" on:click={() => logout(false)}>Switch account</Button>src/routes/(console)/+layout.svelte (1)
367-368: Modal placement LGTM; consider gating at callsite (optional).The component self-gates on
isCloudand other conditions, but you could also wrap it here for tiny render savings.Example:
{#if isCloud} <SendVerificationEmailModal /> {/if}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/lib/components/account/sendVerificationEmailModal.svelte(2 hunks)src/lib/components/alerts/emailVerificationBanner.svelte(0 hunks)src/lib/components/index.ts(0 hunks)src/routes/(console)/+layout.svelte(2 hunks)
💤 Files with no reviewable changes (2)
- src/lib/components/alerts/emailVerificationBanner.svelte
- src/lib/components/index.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: e2e
- GitHub Check: build
🔇 Additional comments (3)
src/lib/components/account/sendVerificationEmailModal.svelte (2)
158-166: Double overlay check.The custom scrim plus Modal may result in two overlays/backdrops. If
Modalalready renders its own overlay, consider disabling it or reusing it (and applying the blur there) to avoid stacking/scroll issues.
40-51: Timer/persistence flow looks solid.Good use of a saved end time and a countdown with rehydration.
src/routes/(console)/+layout.svelte (1)
48-50: Imports look correct and consistent with the new flow.
| onDestroy(() => { | ||
| if (timerInterval) { | ||
| clearInterval(timerInterval); | ||
| } | ||
| // round up localstorage when component is destroyed | ||
| if (browser) { | ||
| localStorage.removeItem(TIMER_END_KEY); | ||
| localStorage.removeItem(EMAIL_SENT_KEY); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Don't clear localStorage on destroy; it defeats persistence of the resend timer.
Clearing TIMER_END_KEY/EMAIL_SENT_KEY in onDestroy breaks the “persist across reloads” goal and allows immediate resends after a refresh/navigation.
Apply this diff:
onDestroy(() => {
if (timerInterval) {
clearInterval(timerInterval);
}
- // round up localstorage when component is destroyed
- if (browser) {
- localStorage.removeItem(TIMER_END_KEY);
- localStorage.removeItem(EMAIL_SENT_KEY);
- }
});📝 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.
| onDestroy(() => { | |
| if (timerInterval) { | |
| clearInterval(timerInterval); | |
| } | |
| // round up localstorage when component is destroyed | |
| if (browser) { | |
| localStorage.removeItem(TIMER_END_KEY); | |
| localStorage.removeItem(EMAIL_SENT_KEY); | |
| } | |
| }); | |
| onDestroy(() => { | |
| if (timerInterval) { | |
| clearInterval(timerInterval); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In src/lib/components/account/sendVerificationEmailModal.svelte around lines 146
to 155, the onDestroy handler currently clears TIMER_END_KEY and EMAIL_SENT_KEY
from localStorage which breaks persistence across navigations; remove the
localStorage.removeItem calls from onDestroy so the resend timer state is
preserved across reloads and navigations, leaving only the
clearInterval(timerInterval) logic; ensure any clearing of those keys happens
only when the timer naturally expires or when an explicit cancel/reset action
occurs (not on component destroy).
| <Button on:click={sendVerificationEmail} disabled={creating || resendTimer > 0}> | ||
| {emailSent ? 'Resend email' : 'Send email'} | ||
| </Button> |
There was a problem hiding this comment.
why not button submit and use the onSubmit?
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/routes/(console)/onboarding/create-project/+page.ts (1)
63-64: Preserve query params on error redirectTo keep UX consistent with other redirects, append url.search to the fallback redirect.
Apply this diff:
- redirect(303, `${base}/create-organization`); + redirect(303, `${base}/create-organization${url.search ?? ''}`);Also update the load signature to receive url:
export const load: PageLoad = async ({ parent, url }) => { ... }Ensure this page never rethrows to itself via intermediary guards. If needed, I can provide a quick route-flow map.
src/routes/(console)/verify-email/+page.svelte (2)
44-46: Make modal visibility reactive to data updatesshowVerificationModal won’t update when data changes after invalidation.
Apply this diff:
- // Show verification modal only if email is not verified - let showVerificationModal = !data.user?.emailVerification; + // Show verification modal only if email is not verified + let showVerificationModal = false; + $: showVerificationModal = !data.user?.emailVerification;
193-204: Typo in comment and minor a11y note“efnsure” → “ensure”. Also confirm the modal traps focus and sets aria-modal to truly block interaction behind the scrim.
Would you like me to scan SendVerificationEmailModal for focus trap and aria-modal?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/lib/components/account/sendVerificationEmailModal.svelte(2 hunks)src/routes/(console)/+layout.svelte(3 hunks)src/routes/(console)/+layout.ts(1 hunks)src/routes/(console)/onboarding/create-project/+page.ts(1 hunks)src/routes/(console)/verify-email/+layout.svelte(1 hunks)src/routes/(console)/verify-email/+page.svelte(1 hunks)src/routes/(console)/verify-email/+page.ts(1 hunks)src/routes/+page.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/components/account/sendVerificationEmailModal.svelte
🧰 Additional context used
🧬 Code graph analysis (3)
src/routes/(console)/verify-email/+page.ts (1)
src/routes/(console)/+layout.ts (1)
load(11-63)
src/routes/+page.ts (1)
src/lib/system.ts (1)
isCloud(24-24)
src/routes/(console)/+layout.ts (3)
src/routes/(console)/verify-email/+page.ts (1)
load(3-10)src/routes/+page.ts (1)
load(31-53)src/lib/system.ts (1)
isCloud(24-24)
🔇 Additional comments (5)
src/routes/(console)/verify-email/+layout.svelte (1)
1-5: LGTMMinimal layout wrapper with slot is fine.
src/routes/(console)/verify-email/+page.ts (1)
3-10: LGTMData derivation from parent is correct for user and first organization.
src/routes/(console)/+layout.svelte (1)
341-347: Correctly hiding chrome on verify-email routesSide nav, header, and footer guards align with the blocking flow.
src/routes/(console)/+layout.ts (1)
14-16: No apply-credit route found — do not apply the exclusion
Search of src for "apply-credit" (and variants) returned no matches; src/routes/(console)/+layout.ts contains the email-verification redirect. Add the /apply-credit exclusion only if a console apply-credit route is actually present.src/routes/+page.ts (1)
36-38: Early verify-email gate looks good — confirm apply-credit routing/console guardRedirect condition and query-preservation are correct.
Found console-scoped route: src/routes/(console)/apply-credit/+page.svelte; src/routes/+layout.svelte also navigates to
${base}/apply-credit?….Root guard uses url.pathname.includes('apply-credit') so it will not redirect /console/apply-credit; the (console) layout guard can still block the flow — ensure a public /apply-credit exists or allow this flow in the (console) layout.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
src/routes/(console)/+layout.svelte (1)
341-347: Gating shell chrome on verify‑email route is correctHiding side‑nav/header/footer on /console/verify-email prevents layout clashes with the page’s own UI. Consider centralizing the pathname check to a single derived flag to avoid drift.
Apply this small refactor:
@@ export let data: LayoutData; @@ + $: isVerifyEmailRoute = page.url.pathname.includes('/console/verify-email'); @@ - showSideNavigation={page.url.pathname !== '/' && + showSideNavigation={page.url.pathname !== '/' && !page?.params.organization && !page.url.pathname.includes('/console/account') && !page.url.pathname.includes('/console/card') && - !page.url.pathname.includes('/console/onboarding') && - !page.url.pathname.includes('/console/verify-email')} - showHeader={!page.url.pathname.includes('/console/onboarding/create-project') && - !page.url.pathname.includes('/console/verify-email')} - showFooter={!page.url.pathname.includes('/console/onboarding/create-project') && - !page.url.pathname.includes('/console/verify-email')} + !page.url.pathname.includes('/console/onboarding') && + !isVerifyEmailRoute} + showHeader={!page.url.pathname.includes('/console/onboarding/create-project') && !isVerifyEmailRoute} + showFooter={!page.url.pathname.includes('/console/onboarding/create-project') && !isVerifyEmailRoute}src/routes/(console)/verify-email/+page.svelte (5)
44-46: MakeshowVerificationModalreactive todata.user.emailVerificationAs written,
showVerificationModalwon’t update after invalidation. Make it reactive to prevent any flicker or stale state.- // Show verification modal only if email is not verified - let showVerificationModal = !data.user?.emailVerification; + // Show verification modal only if email is not verified + $: showVerificationModal = !data.user?.emailVerification;
48-51: Close modal immediately when verified (pre‑navigation polish)Set
showVerificationModal = falsebefore redirecting to avoid a brief visible modal during the redirect.$: if (data.user?.emailVerification) { - // Email is verified, redirect immediately + // Email is verified, close modal and redirect + showVerificationModal = false; checkEmailVerification(); }
149-151: Bindingshowis redundant hereThe modal already renders based on its internal
shouldShowModal. You can simplify by dropping the boundshowprop and relying on the component’s gating.- <SendVerificationEmailModal bind:show={showVerificationModal} /> + <SendVerificationEmailModal />If you keep the binding, ensure it remains reactive as suggested above.
163-168: Duplicate.main-contentrulesConsolidate these duplicate style blocks to a single definition.
- .main-content { - flex: 1; - padding: 2rem; - margin-left: 190px; - min-height: 100vh; - } @@ - .main-content { - filter: blur(4px); - opacity: 0.6; - } + .main-content { + flex: 1; + padding: 2rem; + margin-left: 190px; + min-height: 100vh; + filter: blur(4px); + opacity: 0.6; + }Also applies to: 188-191
193-204: Minor typo in comment“efnsure” → “ensure”.
- /* efnsure modal is above everything and not blurred */ + /* ensure modal is above everything and not blurred */src/lib/components/account/sendVerificationEmailModal.svelte (3)
78-94: Defensive: clear an existing interval before starting a new onePrevents accidental double intervals if countdown is restarted (e.g., edge restore/race).
function startTimerCountdown(timerEndTime: number) { - timerInterval = setInterval(() => { + if (timerInterval) { + clearInterval(timerInterval); + timerInterval = null; + } + 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); }
40-47: Replace magic number with a named constantImproves readability and single‑source of truth for the resend window.
- function startResendTimer() { - const timerEndTime = Date.now() + 60 * 1000; - resendTimer = 60; + const RESEND_SECONDS = 60; + function startResendTimer() { + const timerEndTime = Date.now() + RESEND_SECONDS * 1000; + resendTimer = RESEND_SECONDS;Also update any other “60” occurrences tied to the timer.
104-108: Harden error messaging
error.messagecan be undefined. Fallback to a safe string to avoid empty toasts.- } catch (error) { - addNotification({ message: error.message, type: 'error' }); + } catch (error) { + const message = (error && (error as any).message) ? (error as any).message : String(error); + addNotification({ message, type: 'error' }); } @@ - } catch (error) { - addNotification({ - message: error.message, - type: 'error' - }); + } catch (error) { + const message = (error && (error as any).message) ? (error as any).message : String(error); + addNotification({ message, type: 'error' }); }Also applies to: 127-132
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/lib/components/account/sendVerificationEmailModal.svelte(2 hunks)src/routes/(console)/+layout.svelte(3 hunks)src/routes/(console)/verify-email/+page.svelte(1 hunks)
🔇 Additional comments (5)
src/routes/(console)/+layout.svelte (2)
48-49: LGTM on modal import/useSwitching from the banner to SendVerificationEmailModal here is correct and aligns with the new flow.
370-372: Fixed: avoid duplicate verification modal on /console/verify-emailThe conditional render prevents double scrims/timers on the verify‑email page. Thanks for addressing the earlier note.
src/routes/(console)/verify-email/+page.svelte (1)
118-121: Good fix: invalidate the correct dependency keyReplacing
invalidate('user')withinvalidate(Dependencies.ACCOUNT)ensures the parent load actually re‑runs.src/lib/components/account/sendVerificationEmailModal.svelte (2)
153-191: A11y/UX: ensure focus trap and keyboard flow are handled by ModalAssuming Modal already traps focus and disables background scroll when
dismissible={false}. If not, wire those in Modal to avoid focus leaking to blurred content.Would you like me to scan Modal’s implementation and open a follow‑up PR if the trap/scroll‑lock aren’t enforced?
141-150: Don’t clear localStorage on destroy — breaks resend timer persistenceClearing
TIMER_END_KEY/EMAIL_SENT_KEYon unmount defeats the “persist across reloads” requirement.onDestroy(() => { if (timerInterval) { clearInterval(timerInterval); } - // round up localstorage when component is destroyed - if (browser) { - localStorage.removeItem(TIMER_END_KEY); - localStorage.removeItem(EMAIL_SENT_KEY); - } });
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/lib/components/account/sendVerificationEmailModal.svelte (4)
78-94: Avoid overlapping intervals when restarting the countdown.Small guard to prevent duplicate intervals if this function is called while one is active.
function startTimerCountdown(timerEndTime: number) { - timerInterval = setInterval(() => { + // Avoid overlapping intervals + if (timerInterval) { + clearInterval(timerInterval); + timerInterval = null; + } + timerInterval = setInterval(() => {
60-60: Specify radix for parseInt.Be explicit with base 10.
- const timerEndTime = parseInt(savedTimerEnd); + const timerEndTime = parseInt(savedTimerEnd, 10);
165-169: Use reactive$userinstead ofget(user)in markup.Ensures the email updates if the store changes and avoids non‑reactive reads in the template.
- style="display: inline;">{get(user)?.email}</Typography.Text> + style="display: inline;">{$user?.email}</Typography.Text>
53-76: Optional: sync timer state across tabs/windows.Listen for the
storageevent to reflect sends or expiries from another tab.onMount(() => { updateEmailVerification(); restoreTimerState(); + if (browser) { + const onStorage = (e: StorageEvent) => { + if (!e.key) return; + if (e.key.startsWith('email-verification-timer-end') || e.key.startsWith('email-verification-sent')) { + restoreTimerState(); + } + }; + window.addEventListener('storage', onStorage); + // keep a reference to remove in onDestroy + (onMount as any)._onStorage = onStorage; + } });onDestroy(() => { if (timerInterval) { clearInterval(timerInterval); } + if (browser && (onMount as any)._onStorage) { + window.removeEventListener('storage', (onMount as any)._onStorage); + } });Also applies to: 136-140
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/components/account/sendVerificationEmailModal.svelte(2 hunks)
🔇 Additional comments (2)
src/lib/components/account/sendVerificationEmailModal.svelte (2)
96-109: Send gating and resend timer integration look solid.Blocking concurrent sends and tying the timer start to a successful API call is correct.
Please sanity‑check that
sdk.forConsole.account.createVerification({ url: cleanUrl })expects a URL without query params; if it requires placeholders or path adjustments, updatecleanUrlaccordingly.
141-150: Don’t clear localStorage on destroy; preserve timer persistence.Clearing
TIMER_END_KEY/EMAIL_SENT_KEYon destroy defeats the “persist across reloads” requirement and enables immediate resends after navigation/refresh. Remove these calls; only clear on natural expiry or explicit account change.Apply this diff:
onDestroy(() => { if (timerInterval) { clearInterval(timerInterval); } - // round up localstorage when component is destroyed - if (browser) { - localStorage.removeItem(TIMER_END_KEY); - localStorage.removeItem(EMAIL_SENT_KEY); - } });
| // Timer state key for localStorage | ||
| const TIMER_END_KEY = 'email-verification-timer-end'; | ||
| const EMAIL_SENT_KEY = 'email-verification-sent'; | ||
There was a problem hiding this comment.
Namespace timer keys per account to avoid cross‑account bleed.
Using global keys means logging out and switching users within 60s incorrectly throttles the next user. Namespace by user.$id (fallback to email) and update all usages.
-// Timer state key for localStorage
-const TIMER_END_KEY = 'email-verification-timer-end';
-const EMAIL_SENT_KEY = 'email-verification-sent';
+// Timer state keys, namespaced per account to avoid cross-account bleed
+const storageKey = (name: string) => {
+ const u = get(user);
+ const id = u?.$id ?? u?.email ?? 'anon';
+ return `${name}:${id}`;
+};
+const TIMER_END_KEY = () => storageKey('email-verification-timer-end');
+const EMAIL_SENT_KEY = () => storageKey('email-verification-sent');- localStorage.setItem(TIMER_END_KEY, timerEndTime.toString());
- localStorage.setItem(EMAIL_SENT_KEY, 'true');
+ localStorage.setItem(TIMER_END_KEY(), timerEndTime.toString());
+ localStorage.setItem(EMAIL_SENT_KEY(), 'true');- const savedTimerEnd = localStorage.getItem(TIMER_END_KEY);
- const savedEmailSent = localStorage.getItem(EMAIL_SENT_KEY);
+ const savedTimerEnd = localStorage.getItem(TIMER_END_KEY());
+ const savedEmailSent = localStorage.getItem(EMAIL_SENT_KEY());- localStorage.removeItem(TIMER_END_KEY);
- localStorage.removeItem(EMAIL_SENT_KEY);
+ localStorage.removeItem(TIMER_END_KEY());
+ localStorage.removeItem(EMAIL_SENT_KEY());- localStorage.removeItem(TIMER_END_KEY);
- localStorage.removeItem(EMAIL_SENT_KEY);
+ localStorage.removeItem(TIMER_END_KEY());
+ localStorage.removeItem(EMAIL_SENT_KEY());Also applies to: 46-48, 56-58, 70-71, 89-91
| <style> | ||
| .email-verification-scrim { | ||
| position: fixed; | ||
| top: 0; | ||
| left: 0; | ||
| width: 100%; | ||
| height: 100%; | ||
| background-color: hsl(240 5% 8% / 0.6); | ||
| backdrop-filter: blur(4px); | ||
| display: flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| } | ||
| </style> |
There was a problem hiding this comment.
Ensure the scrim overlays all console content.
Add a z-index and simplify positioning; without z-index the header/other fixed elements may sit above the scrim.
.email-verification-scrim {
position: fixed;
- top: 0;
- left: 0;
- width: 100%;
- height: 100%;
+ inset: 0;
background-color: hsl(240 5% 8% / 0.6);
backdrop-filter: blur(4px);
display: flex;
align-items: center;
justify-content: center;
+ z-index: var(--z-modal, 1000);
}📝 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.
| <style> | |
| .email-verification-scrim { | |
| position: fixed; | |
| top: 0; | |
| left: 0; | |
| width: 100%; | |
| height: 100%; | |
| background-color: hsl(240 5% 8% / 0.6); | |
| backdrop-filter: blur(4px); | |
| display: flex; | |
| align-items: center; | |
| justify-content: center; | |
| } | |
| </style> | |
| <style> | |
| .email-verification-scrim { | |
| position: fixed; | |
| inset: 0; | |
| background-color: hsl(240 5% 8% / 0.6); | |
| backdrop-filter: blur(4px); | |
| display: flex; | |
| align-items: center; | |
| justify-content: center; | |
| z-index: var(--z-modal, 1000); | |
| } | |
| </style> |
🤖 Prompt for AI Agents
In src/lib/components/account/sendVerificationEmailModal.svelte around lines 193
to 206, the scrim CSS can be overlaid by other fixed elements because it lacks a
z-index and uses verbose positioning; add a high z-index (e.g. 9999) to ensure
it sits above all console content and simplify positioning by replacing
top/left/width/height with inset: 0 while keeping position: fixed and centering
styles — update the .email-verification-scrim rule to include position: fixed;
inset: 0; z-index: 9999; display:flex; align-items:center;
justify-content:center; and keep the background and backdrop-filter as-is.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/routes/(console)/verify-email/+page.svelte (2)
193-199: Typo in comment (“efnsure”).Fix the spelling.
- /* efnsure modal is above everything and not blurred */ + /* ensure modal is above everything and not blurred */
47-51: Deduplicate navigation to avoid double goto.Reactive block and onMount can both trigger checkEmailVerification() causing redundant navigations. Add a guard.
+ let navigating = false; // Check if user gets verified and redirect accordingly async function checkEmailVerification() { - if (data.user?.emailVerification) { + if (data.user?.emailVerification && !navigating) { + navigating = true; if (!data.organization) { try { await createOrganizationForNewUser(); await goto(`${base}/onboarding/create-project`); } catch (e) { await goto(`${base}/create-organization`); } } else if (!(data.organization as any)?.onboardingCompleted) { await goto(`${base}/onboarding/create-project`); } else { await goto(`${base}`); } + // no finally needed; component will unmount post-navigation } }Also applies to: 112-121
src/lib/components/account/sendVerificationEmailModal.svelte (1)
105-109: Harden error handling for unknown error types.Avoid assuming error.message exists.
- } catch (error) { - addNotification({ message: error.message, type: 'error' }); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + addNotification({ message, type: 'error' }); } finally {
📜 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/routes/(console)/verify-email/+page.svelte (1)
118-121: Good fix: correct dependency invalidation key.Using invalidate(Dependencies.ACCOUNT) will correctly refresh parent data.
src/lib/components/account/sendVerificationEmailModal.svelte (5)
194-206: Ensure scrim overlays all content. Add z-index and simplify positioning.Without z-index, other fixed elements can appear above the scrim.
.email-verification-scrim { position: fixed; - top: 0; - left: 0; - width: 100%; - height: 100%; + inset: 0; background-color: hsl(240 5% 8% / 0.6); backdrop-filter: blur(4px); display: flex; align-items: center; justify-content: center; + z-index: var(--z-modal, 9999); }
97-105: Nice: concurrency guard on send + timer reset is correct.Early return prevents duplicate sends; starting the timer on success matches the UX spec.
154-162: Accessibility/focus-trap check.Confirm the Modal sets role="dialog", aria-modal="true", traps focus, and disables background from screen readers. If not, we should add inert/aria-hidden on the app root while the scrim is open.
Would you like me to draft a small helper to toggle inert/aria-hidden on the app shell while the modal is mounted?
142-151: Don’t clear timer keys on destroy; it breaks persistence across reloads.This defeats the “persist 60s resend timer” requirement.
onDestroy(() => { if (timerInterval) { clearInterval(timerInterval); } - // round up localstorage when component is destroyed - if (browser) { - localStorage.removeItem(TIMER_END_KEY); - localStorage.removeItem(EMAIL_SENT_KEY); - } });
25-27: Namespace timer keys per account to avoid cross‑account bleed.Switching users within 60s currently throttles the new user.
-// Timer state key for localStorage -const TIMER_END_KEY = 'email-verification-timer-end'; -const EMAIL_SENT_KEY = 'email-verification-sent'; +// Timer state keys, namespaced per account to avoid cross-account bleed +const storageKey = (name: string) => { + const u = get(user); + const id = u?.$id ?? u?.email ?? 'anon'; + return `${name}:${id}`; +}; +const TIMER_END_KEY = () => storageKey('email-verification-timer-end'); +const EMAIL_SENT_KEY = () => storageKey('email-verification-sent');- localStorage.setItem(TIMER_END_KEY, timerEndTime.toString()); - localStorage.setItem(EMAIL_SENT_KEY, 'true'); + localStorage.setItem(TIMER_END_KEY(), timerEndTime.toString()); + localStorage.setItem(EMAIL_SENT_KEY(), 'true');- const savedTimerEnd = localStorage.getItem(TIMER_END_KEY); - const savedEmailSent = localStorage.getItem(EMAIL_SENT_KEY); + const savedTimerEnd = localStorage.getItem(TIMER_END_KEY()); + const savedEmailSent = localStorage.getItem(EMAIL_SENT_KEY());- localStorage.removeItem(TIMER_END_KEY); - localStorage.removeItem(EMAIL_SENT_KEY); + localStorage.removeItem(TIMER_END_KEY()); + localStorage.removeItem(EMAIL_SENT_KEY()); resendTimer = 0; emailSent = false;- localStorage.removeItem(TIMER_END_KEY); - localStorage.removeItem(EMAIL_SENT_KEY); + localStorage.removeItem(TIMER_END_KEY()); + localStorage.removeItem(EMAIL_SENT_KEY());Also applies to: 46-49, 57-59, 71-75, 89-92
|
Closing this PR in favor of #2398 which includes the updated changes |
2 participants
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
UX Improvements
Removed
Navigation
Bug Fixes