Skip to content

Conversation

@ItzNotABug
Copy link
Member

@ItzNotABug ItzNotABug commented Oct 26, 2025

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

    • Updated email verification functionality to be dynamically configured at runtime rather than statically defined.
  • Chores

    • Updated console dependencies.
    • Removed hardcoded email verification settings from environment configuration and build workflows.

@ItzNotABug ItzNotABug self-assigned this Oct 26, 2025
@railway-app
Copy link

railway-app bot commented Oct 26, 2025

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.

@appwrite
Copy link

appwrite bot commented Oct 26, 2025

Console

Project ID: 688b7bf400350cbd60e9

Sites (1)
Site Status Logs Preview QR
 console-stage
688b7cf6003b1842c9dc
Ready Ready View Logs Preview URL QR Code

Tip

You can use Avatars API to generate QR code for any text or URLs.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 26, 2025

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

This pull request migrates email verification configuration from static build-time environment variables to runtime variables fetched from console configuration. The PUBLIC_CONSOLE_EMAIL_VERIFICATION environment variable is removed from .env.example, Dockerfile, build.js, and workflow files. The static EMAIL_VERIFICATION property is removed from src/lib/system.ts. New runtime logic introduces a consoleVariables fetch guarded by an isEmailVerificationRequired store, with updates across layout files and verification flows. Method signatures are updated from updateVerification to updateEmailVerification. A dependency in package.json is bumped to a newer commit.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • The transition from static build-time configuration to runtime variable fetching in src/routes/+layout.ts requires careful verification of error handling and null-coalescing logic
  • The new consoleVariables._APP_REQUIRE_CONSOLE_VERIFICATION gate in src/routes/verify-email/+page.ts needs validation that redirect logic behaves correctly when disabled
  • The package.json commit update from fe3277e to 2751 requires confirmation that this dependency version is compatible and intended
  • The new isEmailVerificationRequired store in src/routes/store.ts should be verified for proper initialization and state management across the application
  • Ensure the restructured parent() call in src/routes/(console)/+layout.ts correctly handles the renamed variables property in data flow

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The PR title "Update email verification logic" is clearly related to the changeset, as all modifications across multiple files center on refactoring email verification functionality. The title identifies the specific feature area being modified rather than being generic like "misc updates" or "stuff." However, the title lacks specificity about the nature of the update—it doesn't convey that this involves moving email verification from static environment variable-based configuration to dynamic runtime configuration via console variables. While the title adequately communicates the domain of change, it could be more descriptive to better convey the scope of the refactoring.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ItzNotABug ItzNotABug marked this pull request as draft October 26, 2025 12:46
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 consoleVariables is not provided by the parent, the code falls back to fetching via sdk.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 consoleVariables and immediately sets isEmailVerificationRequired at line 30. However, if multiple route loads occur concurrently (e.g., during navigation), there's a potential race condition where:

  1. Thread A checks get(isEmailVerificationRequired) === null → true
  2. Thread B checks get(isEmailVerificationRequired) === null → true
  3. 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

📥 Commits

Reviewing files that changed from the base of the PR and between bfcb207 and 449fa1f.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is 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 consoleVariables from parent() 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 isEmailVerificationRequired store enables tracking of email verification requirements fetched at runtime, replacing the static VARS.EMAIL_VERIFICATION environment variable approach.

The tri-state type boolean | null appropriately handles:

  • null: not yet loaded
  • true/false: verification requirement status from console variables
src/routes/(console)/account/updateMfa.svelte (1)

63-63: LGTM - API method rename for consistency.

The method name change from updateVerification to updateEmailVerification improves 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 updateVerification to updateEmailVerification is consistent with the broader SDK updates and improves naming clarity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants