feat: implement blocking email verification modal #2379
Conversation
ConsoleProject ID: Sites (2)
Note Cursor pagination performs better than offset pagination when loading further pages. |
Walkthrough
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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: 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.
| import { BottomModalAlert } from '$lib/components'; | ||
| import SendVerificationEmailModal from '$lib/components/account/sendVerificationEmailModal.svelte'; |
There was a problem hiding this comment.
Duplicate verification modal renders on /console/verify-email
This layout always renders SendVerificationEmailModal, and the verify-email page also renders it. Results: double scrims, duplicate timers/listeners.
Apply this diff to suppress the layout modal on the verify-email route:
<BottomModalAlert />
-<SendVerificationEmailModal />
+{#if !page.url.pathname.includes('/console/verify-email')}
+ <SendVerificationEmailModal />
+{/if}Also applies to: 370-371
🤖 Prompt for AI Agents
In src/routes/(console)/+layout.svelte around lines 48-49 (and similarly at
lines 370-371), the layout unconditionally imports and renders
SendVerificationEmailModal causing duplicate modals on the /console/verify-email
page; change the layout to conditionally render that modal only when the current
route is not /console/verify-email by using the SvelteKit page store (or
$page.url.pathname) to check the pathname and wrap the modal import/render in
that condition so the modal is suppressed on the verify-email route.
| const interval = setInterval(async () => { | ||
| await invalidate('user'); | ||
| checkEmailVerification(); | ||
| }, 2000); | ||
|
|
||
| return () => clearInterval(interval); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
Polling invalidation likely no-ops; use the declared dependency key
invalidate('user') works only if a parent load called depends('user'). Prefer the canonical account dependency key.
Apply this diff (adjust the key name to match your constants):
- const interval = setInterval(async () => {
- await invalidate('user');
- checkEmailVerification();
- }, 2000);
+ const interval = setInterval(async () => {
+ await invalidate(Dependencies.ACCOUNT);
+ checkEmailVerification();
+ }, 2000);If Dependencies.ACCOUNT doesn’t exist, either add depends(Dependencies.ACCOUNT) in the parent providing account, or temporarily use invalidateAll().
Quick check for the correct key name:
🏁 Script executed:
#!/bin/bash
rg -nP 'enum\\s+Dependencies|const\\s+Dependencies' src/lib | sed -n 'p'
rg -nP 'Dependencies\\.[A-Z_]+' src -g '!**/dist/**' | rg -n 'ACCOUNT|USER|PROFILE|AUTH'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Verify presence of the referenced file and search for Dependencies / invalidate usage across the repo.
set -eu -o pipefail
echo "PWD: $(pwd)"
echo
echo "Top-level files:"
ls -la | sed -n '1,200p' || true
echo
echo "Target file snippet (if exists):"
if [ -f "src/routes/(console)/verify-email/+page.svelte" ]; then
sed -n '1,240p' "src/routes/(console)/verify-email/+page.svelte"
else
echo "File not found: src/routes/(console)/verify-email/+page.svelte"
fi
echo
echo "Searching for Dependencies enum/const definitions..."
rg -n --hidden --no-ignore -S 'enum\s+Dependencies|const\s+Dependencies|export\s+enum\s+Dependencies' || true
echo
echo "Searching for Dependencies.<NAME> usages..."
rg -n --hidden --no-ignore -S 'Dependencies\.[A-Z_]+' || true
echo
echo "Searching for invalidate('user')..."
rg -n --hidden --no-ignore -S "invalidate\(\s*['\"]user['\"]\s*\)" || true
echo
echo "Searching for any invalidate( calls (for context)..."
rg -n --hidden --no-ignore -S 'invalidate\(' -g '!**/dist/**' || true
echo
echo "Searching for depends( calls..."
rg -n --hidden --no-ignore -S 'depends\(' || true
echo
echo "Searching for ACCOUNT / USER tokens (uppercase)..."
rg -n --hidden --no-ignore -S '\bACCOUNT\b|\bUSER\b' || true
echo
echo "Searching for invalidateAll usage..."
rg -n --hidden --no-ignore -S 'invalidateAll' || true
echo
echo "Script finished."Length of output: 90587
Replace string 'user' invalidate with the declared dependency key (Dependencies.ACCOUNT)
invalidate('user') is a no-op here — the app layout depends on Dependencies.ACCOUNT, so invalidate(Dependencies.ACCOUNT) will trigger the parent load.
File: src/routes/(console)/verify-email/+page.svelte (onMount interval)
- const interval = setInterval(async () => {
- await invalidate('user');
- checkEmailVerification();
- }, 2000);
+ const interval = setInterval(async () => {
+ await invalidate(Dependencies.ACCOUNT);
+ checkEmailVerification();
+ }, 2000);Dependencies.ACCOUNT is defined in src/lib/constants.ts and src/routes/+layout.ts calls depends(Dependencies.ACCOUNT).
📝 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 interval = setInterval(async () => { | |
| await invalidate('user'); | |
| checkEmailVerification(); | |
| }, 2000); | |
| return () => clearInterval(interval); | |
| }); | |
| const interval = setInterval(async () => { | |
| await invalidate(Dependencies.ACCOUNT); | |
| checkEmailVerification(); | |
| }, 2000); | |
| return () => clearInterval(interval); | |
| }); |
🤖 Prompt for AI Agents
In src/routes/(console)/verify-email/+page.svelte around lines 118 to 124,
replace the no-op invalidate('user') with invalidate(Dependencies.ACCOUNT) so
the parent layout load is triggered; import the Dependencies constant from
src/lib/constants (or the project's $lib path) at the top of the file if not
already imported, then use invalidate(Dependencies.ACCOUNT) inside the interval
callback and keep the clearInterval return unchanged.
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