-
Notifications
You must be signed in to change notification settings - Fork 196
Update email verification logic. #2511
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
|
This PR was not deployed automatically as @ItzNotABug does not have access to the Railway project. In order to get automatic PR deploys, please add @ItzNotABug to your workspace on Railway. |
ConsoleProject ID: Tip You can use Avatars API to generate QR code for any text or URLs. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request migrates email verification configuration from static build-time environment variables to runtime variables fetched from console configuration. The Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
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: 4
🧹 Nitpick comments (3)
src/routes/(console)/+layout.ts (2)
17-24: Consider error handling for the SDK fallback call.When
consoleVariablesis not provided by the parent, the code falls back to fetching viasdk.forConsole.console.variables()at line 23. However, this Promise.all has no error handling, which could cause the entire layout to fail if the console variables endpoint is unavailable.Consider adding error handling similar to the approach used in
src/routes/+layout.ts(lines 28-33), where console variables errors are caught and ignored:- const [preferences, plansArray, versionData, variables] = await Promise.all([ + const [preferences, plansArray, versionData, variablesResult] = await Promise.all([ sdk.forConsole.account.getPrefs(), isCloud ? sdk.forConsole.billing.getPlansInfo() : null, fetch(`${endpoint}/health/version`, { headers: { 'X-Appwrite-Project': project } }).then((response) => response.json() as { version?: string }), - consoleVariables ? consoleVariables : sdk.forConsole.console.variables() + consoleVariables + ? Promise.resolve(consoleVariables) + : sdk.forConsole.console.variables().catch(() => null) ]); + + const variables = variablesResult ?? consoleVariables;
53-53: Variable naming could be clearer.The flow
consoleVariables(from parent) →variables(Promise result) →consoleVariables(return value) creates cognitive overhead when tracing the data flow.Consider renaming for clarity:
- const { organizations, consoleVariables } = await parent(); + const { organizations, consoleVariables: parentConsoleVariables } = await parent(); // ... - const [preferences, plansArray, versionData, variables] = await Promise.all([ + const [preferences, plansArray, versionData, consoleVariables] = await Promise.all([ // ... - consoleVariables ? consoleVariables : sdk.forConsole.console.variables() + parentConsoleVariables ? parentConsoleVariables : sdk.forConsole.console.variables() ]); // ... return { // ... - consoleVariables: variables, + consoleVariables, };src/routes/+layout.ts (1)
29-30: Consider handling race conditions with store updates.The code fetches
consoleVariablesand immediately setsisEmailVerificationRequiredat line 30. However, if multiple route loads occur concurrently (e.g., during navigation), there's a potential race condition where:
- Thread A checks
get(isEmailVerificationRequired) === null→ true- Thread B checks
get(isEmailVerificationRequired) === null→ true- Both threads fetch console variables simultaneously
While SvelteKit's load functions typically run sequentially per navigation, you could add defensive guards:
if (get(isEmailVerificationRequired) === null) { try { consoleVariables = await sdk.forConsole.console.variables(); - isEmailVerificationRequired.set(consoleVariables._APP_REQUIRE_CONSOLE_VERIFICATION); + // Only set if still null (defensive against race conditions) + if (get(isEmailVerificationRequired) === null) { + isEmailVerificationRequired.set(consoleVariables._APP_REQUIRE_CONSOLE_VERIFICATION); + } } catch (error) { // ignore } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
.env.example(0 hunks).github/workflows/publish.yml(0 hunks)Dockerfile(0 hunks)build.js(0 hunks)package.json(1 hunks)src/lib/system.ts(0 hunks)src/routes/(console)/+layout.ts(2 hunks)src/routes/(console)/account/updateMfa.svelte(1 hunks)src/routes/(console)/verify-email/+page.ts(2 hunks)src/routes/+layout.ts(3 hunks)src/routes/store.ts(1 hunks)
💤 Files with no reviewable changes (5)
- build.js
- src/lib/system.ts
- .env.example
- .github/workflows/publish.yml
- Dockerfile
🧰 Additional context used
🧬 Code graph analysis (3)
src/routes/(console)/+layout.ts (3)
src/lib/stores/sdk.ts (1)
sdk(153-176)src/lib/system.ts (1)
isCloud(25-25)src/routes/(console)/store.ts (1)
consoleVariables(9-12)
src/routes/(console)/verify-email/+page.ts (2)
src/routes/(console)/store.ts (1)
consoleVariables(9-12)src/lib/stores/sdk.ts (1)
sdk(153-176)
src/routes/+layout.ts (3)
src/routes/(console)/store.ts (1)
consoleVariables(9-12)src/routes/store.ts (2)
isEmailVerificationRequired(8-8)redirectTo(5-5)src/lib/system.ts (1)
isCloud(25-25)
⏰ 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/routes/(console)/+layout.ts (1)
10-10: LGTM - Optimized data flow.The destructuring of
consoleVariablesfromparent()enables reuse of data fetched upstream, reducing redundant API calls.src/routes/store.ts (1)
8-8: LGTM - New store for runtime email verification gating.The
isEmailVerificationRequiredstore enables tracking of email verification requirements fetched at runtime, replacing the staticVARS.EMAIL_VERIFICATIONenvironment variable approach.The tri-state type
boolean | nullappropriately handles:
null: not yet loadedtrue/false: verification requirement status from console variablessrc/routes/(console)/account/updateMfa.svelte (1)
63-63: LGTM - API method rename for consistency.The method name change from
updateVerificationtoupdateEmailVerificationimproves clarity and aligns with the SDK updates across the codebase.src/routes/(console)/verify-email/+page.ts (1)
24-27: LGTM - API method rename for consistency.The method name change from
updateVerificationtoupdateEmailVerificationis consistent with the broader SDK updates and improves naming clarity.

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
Bug Fixes
Chores