Update template to use platform injection#10974
Conversation
📝 WalkthroughWalkthroughThis PR introduces a platform context ( Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25–35 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ 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). (3)
🔇 Additional comments (2)
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 |
| 'X-SDK-Language', | ||
| 'X-SDK-Platform', | ||
| 'X-SDK-GraphQL', | ||
| 'X-SDK-Profile', |
There was a problem hiding this comment.
this is specifically for Console SDK so we can identify the Console profile and build proper platforms on Cloud's injection.
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/config/locale/templates/email-base-styled.tpl (1)
183-265: Template variable renames break other branded emailsYou’ve renamed the base template placeholders to
{{twitter}},{{discord}},{{github}},{{terms}},{{privacy}}and{{platform}}, but several flows still set the old keys (twitterUrl,discordUrl,githubUrl,termsUrl,privacyUrl) and never provideplatform:
createEmailToken(email OTP) branded branch still mergestwitterUrl,discordUrl,githubUrl,termsUrl,privacyUrl.createEmailVerificationbranded branch does the same.- Those flows also don’t provide a
platformvariable, so the logo alt and footer will show a raw{{platform}}or stay empty.With
smtpBaseTemplate === APP_BRANDED_EMAIL_BASE_TEMPLATE, OTP/verification emails will now have missing/broken social & policy links and footer branding.Consider either:
- Reverting the template placeholders back to
*Urlandproject(and updatingsendSessionAlertto match), or- Updating all branded-email flows to:
- Depend on the injected
$platformarray, and- Populate the new keys (
discord,github,terms,privacy,platform) instead of*Url.app/controllers/api/account.php (2)
210-345: $createSession signature change breaks existing callersYou added
array $platformto$createSession:$createSession = function (string $userId, string $secret, Request $request, Response $response, User $user, Database $dbForProject, Document $project, array $platform, Locale $locale, Reader $geodb, Event $queueForEvents, Mail $queueForMails, Store $store, ProofsToken $proofForToken, ProofsCode $proofForCode) { ... }
/v1/account/sessions(createSessiontoken route) correctly injects and passesplatform, but other callers are now misaligned:
/v1/account/sessions/magic-urlupdate (Line 2635) calls$createSession($userId, $secret, $request, $response, $user, $dbForProject, $project, $locale, ...)and never passes$platform./v1/account/sessions/phoneupdate (Line 2680) uses->action($createSession)without injectingplatform.At runtime this will shift arguments so that
$platformreceives aLocaleor other object, causing aTypeError(arrayexpected) and breaking those endpoints.You should:
- Inject
platforminto both routes, respecting the param order:App::put('/v1/account/sessions/magic-url') @@ - ->inject('project') - ->inject('locale') + ->inject('project') + ->inject('platform') + ->inject('locale') @@ - ->action(function ($userId, $secret, $request, $response, $user, $dbForProject, $project, $locale, $geodb, $queueForEvents, $queueForMails, $store, $proofForCode) use ($createSession) { + ->action(function ($userId, $secret, $request, $response, $user, $dbForProject, $project, array $platform, $locale, $geodb, $queueForEvents, $queueForMails, $store, $proofForCode) use ($createSession) { @@ - $createSession($userId, $secret, $request, $response, $user, $dbForProject, $project, $locale, $geodb, $queueForEvents, $queueForMails, $store, $proofForToken, $proofForCode); + $createSession($userId, $secret, $request, $response, $user, $dbForProject, $project, $platform, $locale, $geodb, $queueForEvents, $queueForMails, $store, $proofForToken, $proofForCode); });App::put('/v1/account/sessions/phone') @@ - ->inject('project') - ->inject('locale') + ->inject('project') + ->inject('platform') + ->inject('locale') @@ - ->action($createSession); + ->action($createSession);(For the phone route, adding the
platforminjection in the correct position is enough; Utopia will pass it into$createSessiontransparently.)
2558-2568: Branded OTP/verification emails still use old social/policy variable namesIn both email OTP and email verification flows, the branded block still populates:
'accentColor' => APP_EMAIL_ACCENT_COLOR, 'logoUrl' => APP_EMAIL_LOGO_URL, 'twitterUrl' => APP_SOCIAL_TWITTER, 'discordUrl' => APP_SOCIAL_DISCORD, 'githubUrl' => APP_SOCIAL_GITHUB_APPWRITE, 'termsUrl' => APP_EMAIL_TERMS_URL, 'privacyUrl' => APP_EMAIL_PRIVACY_URL,But
email-base-styled.tplnow expectsdiscord,github,terms,privacy, andplatform(no*Urlkeys). As a result, branded OTP and verification emails will:
- Render empty/broken social and policy links.
- Not have a value for
{{platform}}(logo alt/footer branding).To align with the new platform-driven design and the updated template:
- Inject
platforminto these routes, and- Change the branded merge to:
$emailVariables = array_merge($emailVariables, [ 'accentColor' => $platform['accentColor'], 'logoUrl' => $platform['logoUrl'], 'twitter' => $platform['twitterUrl'], 'discord' => $platform['discordUrl'], 'github' => $platform['githubUrl'], 'terms' => $platform['termsUrl'], 'privacy' => $platform['privacyUrl'], 'platform' => $platform['platformName'], ]);This keeps all branded emails consistent with
sendSessionAlertand the new base template.Also applies to: 3919-3929
🧹 Nitpick comments (1)
app/controllers/api/account.php (1)
77-207: sendSessionAlert platform integration is mostly soundThe new
$platformparameter, console-specificprojectNameoverride, and platform-based variables (accentColor,logoUrl, social/policy URLs,platform,emailSenderName) are consistent with the new config and base template and look correct.One nuance: when
smtpBaseTemplate === APP_BRANDED_EMAIL_BASE_TEMPLATEyou always override the sender display name withplatform['emailSenderName'], even if a custom template defined its own sender name. If custom per-project sender names are expected to win over platform defaults, consider only applying the override when no custom senderName was set.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/config/locale/templates/email-base-styled.tpl(4 hunks)app/config/platform.php(1 hunks)app/controllers/api/account.php(8 hunks)app/init/resources.php(1 hunks)
🔇 Additional comments (7)
app/init/resources.php (1)
255-285: CORS header addition looks consistentAdding
X-SDK-ProfiletoallowedHeadersis consistent with the otherX-SDK-*headers and should work fine with existing CORS behavior. No further changes needed here.app/config/platform.php (1)
8-26: Platform config extension is straightforwardAdding
emailSenderNamealongside existing platform branding keys is consistent and matches usage insendSessionAlert. No issues here.app/controllers/api/account.php (5)
969-1084: Email/password session: platform param is threaded correctlyFor
/v1/account/sessions/emailyou now injectplatformand pass it intosendSessionAlert. That keeps session alert branding consistent with the new platform-aware template and matches the updatedsendSessionAlertsignature.No issues here.
1260-1275: Token-based session: platform injection matches $createSessionThe
createSessionroute now injectsplatformand delegates directly to$createSession, whose signature expects the array between$projectand$locale. The injection order matches that signature, so this call path is safe and platform-aware.
2073-2193: Magic URL token: default URL now respects platform.consoleHostnameInjecting
platformintocreateMagicURLTokenand usingplatform['consoleHostname']to build the default/console/auth/magic-urlwhenurlis empty brings magic-link flows in line with the new platform-driven routing and keeps behavior centralized.Logic and ordering look good.
1307-1369: OAuth2 session: console redirect now uses platform.consoleHostnameUsing
$platform['consoleHostname']to construct default success/failure redirect URLs aligns OAuth2 flows with the configured console host instead of the API host. The redirect base construction is correct.
1969-2033: OAuth2 token route uses platform-based redirect host for consistencyThe OAuth2 token route correctly injects
platformand usesplatform['consoleHostname']to construct the defaultsuccess/failureredirect URLs, mirroring the session route behavior. This keeps redirect handling consistent across both endpoints.Caveat: This assumes
consoleHostnameis reliably set in platform config.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenges/Create.php (2)
121-121: Consider stronger typing for the platform parameter.While
array $platformworks, consider using a more specific type (e.g., a DTO or typed array documentation) to improve IDE support and prevent incorrect usage. This would make the expected structure explicit.
312-323: Use null coalescing operators for consistency with email template handling elsewhere in the codebase.While the platform array keys are guaranteed by configuration, other parts of the codebase handling similar email template variables (Certificates.php, Migrations.php, Webhooks.php) defensively apply null coalescing with fallback constants. Adopt the same pattern here for consistency:
if ($smtpBaseTemplate === APP_BRANDED_EMAIL_BASE_TEMPLATE) { $emailVariables = array_merge($emailVariables, [ - 'accentColor' => $platform['accentColor'], - 'logoUrl' => $platform['logoUrl'], - 'twitter' => $platform['twitterUrl'], - 'discord' => $platform['discordUrl'], - 'github' => $platform['githubUrl'], - 'terms' => $platform['termsUrl'], - 'privacy' => $platform['privacyUrl'], - 'platform' => $platform['platformName'], + 'accentColor' => $platform['accentColor'] ?? APP_EMAIL_ACCENT_COLOR, + 'logoUrl' => $platform['logoUrl'] ?? APP_EMAIL_LOGO_URL, + 'twitter' => $platform['twitterUrl'] ?? APP_SOCIAL_TWITTER, + 'discord' => $platform['discordUrl'] ?? APP_SOCIAL_DISCORD, + 'github' => $platform['githubUrl'] ?? APP_SOCIAL_GITHUB, + 'terms' => $platform['termsUrl'] ?? APP_EMAIL_TERMS_URL, + 'privacy' => $platform['privacyUrl'] ?? APP_EMAIL_PRIVACY_URL, + 'platform' => $platform['platformName'] ?? APP_EMAIL_PLATFORM_NAME, ]); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenges/Create.php(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenges/Create.php (1)
src/Appwrite/Event/Mail.php (2)
setRecipient(60-65)setSenderName(434-438)
⏰ 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). (7)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: E2E General Test
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: Unit Test
- GitHub Check: Benchmark
🔇 Additional comments (1)
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenges/Create.php (1)
101-101: LGTM: Platform injection follows standard DI pattern.The platform dependency injection is correctly configured and consistent with other injected dependencies.
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenges/Create.php
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/controllers/api/account.php (1)
1342-1348: Add null check for consoleHostname platform key.
$platform['consoleHostname']is accessed without verifying the key exists. This will cause an undefined array key error if the platform config doesn't include this field, breaking OAuth2 redirect URL construction.Apply this diff to both occurrences (lines 1342 and 2004):
-$host = $platform['consoleHostname'] ?? ''; +$host = $platform['consoleHostname'] ?? $request->getHostname();This ensures a fallback to the current request hostname if
consoleHostnameis missing.Also applies to: 2004-2012
♻️ Duplicate comments (1)
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenges/Create.php (1)
338-341: Add null check for emailSenderName platform key (duplicate concern).As flagged in a previous review,
$platform['emailSenderName']is accessed without verifying the key exists. This could cause an undefined array key error if the platform provider doesn't guarantee this field.Apply the fix suggested in the earlier review:
// since this is console project, set email sender name! if ($smtpBaseTemplate === APP_BRANDED_EMAIL_BASE_TEMPLATE) { - $queueForMails->setSenderName($platform['emailSenderName']); + $queueForMails->setSenderName($platform['emailSenderName'] ?? $senderName); }This ensures a fallback to the already-configured
$senderNameifemailSenderNameis missing.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/controllers/api/account.php(8 hunks)src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenges/Create.php(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenges/Create.php (1)
src/Appwrite/Event/Mail.php (2)
setRecipient(60-65)setSenderName(434-438)
⏰ 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). (14)
- GitHub Check: E2E Service Test (VCS)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (Tokens)
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (Databases/Legacy)
- GitHub Check: E2E Service Test (Users)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: Unit Test
- GitHub Check: E2E General Test
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: Benchmark
🔇 Additional comments (5)
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenges/Create.php (1)
101-101: LGTM!Platform dependency injection is correctly added to the constructor and action signature, enabling platform-aware email rendering.
Also applies to: 121-121
app/controllers/api/account.php (4)
77-77: LGTM!Function signature correctly updated to accept platform context for platform-aware session alert emails.
209-209: LGTM!Closure signature correctly updated to accept platform context for session creation flows.
968-968: LGTM!Platform injection is correctly added across multiple account/session endpoints, enabling consistent platform-aware rendering and behavior.
Also applies to: 1266-1266, 1306-1306, 1968-1968, 2072-2072
310-310: LGTM!Both
sendSessionAlertcalls correctly pass the platform context parameter to support platform-aware email rendering.Also applies to: 1078-1078
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenges/Create.php
Show resolved
Hide resolved
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenges/Create.php
Show resolved
Hide resolved
✨ Benchmark results
⚡ Benchmark Comparison
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/controllers/api/account.php (3)
1342-1348: Critical: Add null check for consoleHostname.Direct access to
$platform['consoleHostname']on line 1342 will cause an undefined array key error if the platform config doesn't include this field.Apply this diff to add a null coalescing fallback:
- $host = $platform['consoleHostname'] ?? ''; + $host = $platform['consoleHostname'] ?? $request->getHostname(); $redirectBase = $protocol . '://' . $host;This ensures a fallback to the current hostname if
consoleHostnameis missing from the platform config.
2004-2012: Critical: Add null check for consoleHostname.Direct access to
$platform['consoleHostname']on line 2004 will cause an undefined array key error if the platform config doesn't include this field.Apply this diff to add a null coalescing fallback:
- $host = $platform['consoleHostname'] ?? ''; + $host = $platform['consoleHostname'] ?? $request->getHostname(); $protocol = System::getEnv('_APP_OPTIONS_FORCE_HTTPS') == 'disabled' ? 'http' : 'https';This ensures a fallback to the current hostname if
consoleHostnameis missing.
2179-2188: Critical: Add null check for consoleHostname.Direct access to
$platform['consoleHostname']on line 2179 will cause an undefined array key error if the platform config doesn't include this field.Apply this diff to add a null coalescing fallback:
$protocol = System::getEnv('_APP_OPTIONS_FORCE_HTTPS') === 'disabled' ? 'http' : 'https'; - $host = $platform['consoleHostname'] ?? ''; + $host = $platform['consoleHostname'] ?? $request->getHostname(); $port = $request->getPort();This ensures a fallback to the current hostname if
consoleHostnameis missing.
♻️ Duplicate comments (6)
app/controllers/api/account.php (6)
160-163: Critical: Add null check for platformName.Direct access to
$platform['platformName']will cause an undefined array key error if the platform config doesn't include this field.Apply this diff to add a null coalescing fallback:
$projectName = $project->getAttribute('name'); if ($project->getId() === 'console') { - $projectName = $platform['platformName']; + $projectName = $platform['platformName'] ?? $project->getAttribute('name', 'Console'); }Based on past review comments, this issue was previously identified but not yet resolved.
179-187: Critical: Add null checks for all platform array keys.All platform keys are accessed without verifying their existence. This will cause undefined array key errors if the platform provider doesn't guarantee all fields.
Apply this diff to add null coalescing operators:
if ($smtpBaseTemplate === APP_BRANDED_EMAIL_BASE_TEMPLATE) { $emailVariables = array_merge($emailVariables, [ - 'accentColor' => $platform['accentColor'], - 'logoUrl' => $platform['logoUrl'], - 'twitter' => $platform['twitterUrl'], - 'discord' => $platform['discordUrl'], - 'github' => $platform['githubUrl'], - 'terms' => $platform['termsUrl'], - 'privacy' => $platform['privacyUrl'], - 'platform' => $platform['platformName'], + 'accentColor' => $platform['accentColor'] ?? '', + 'logoUrl' => $platform['logoUrl'] ?? '', + 'twitter' => $platform['twitterUrl'] ?? '', + 'discord' => $platform['discordUrl'] ?? '', + 'github' => $platform['githubUrl'] ?? '', + 'terms' => $platform['termsUrl'] ?? '', + 'privacy' => $platform['privacyUrl'] ?? '', + 'platform' => $platform['platformName'] ?? 'Appwrite', ]); }Based on past review comments, this issue was previously identified but not yet resolved.
200-203: Critical: Add null check for emailSenderName platform key.
$platform['emailSenderName']is accessed without verifying the key exists. This could cause an undefined array key error if the platform provider doesn't guarantee this field.Apply this diff to add a null coalescing fallback:
// since this is console project, set email sender name! if ($smtpBaseTemplate === APP_BRANDED_EMAIL_BASE_TEMPLATE) { - $queueForMails->setSenderName($platform['emailSenderName']); + $queueForMails->setSenderName($platform['emailSenderName'] ?? $senderName); }This ensures a fallback to the already-configured
$senderNameifemailSenderNameis missing.Based on past review comments, this issue was previously identified but not yet resolved.
3869-3875: Critical: Add null check for platformName.Direct access to
$platform['platformName']will cause an undefined array key error if the platform config doesn't include this field.Apply this diff to add a null coalescing fallback:
$projectName = $project->isEmpty() ? 'Console' : $project->getAttribute('name', '[APP-NAME]'); if ($project->getId() === 'console') { - $projectName = $platform['platformName']; + $projectName = $platform['platformName'] ?? 'Console'; }
2563-2574: Critical: Add null checks for all platform array keys.All platform keys are accessed without verifying their existence. This will cause undefined array key errors if the platform provider doesn't guarantee all fields.
Apply this diff to add null coalescing operators:
if ($smtpBaseTemplate === APP_BRANDED_EMAIL_BASE_TEMPLATE) { $emailVariables = array_merge($emailVariables, [ - 'accentColor' => $platform['accentColor'], - 'logoUrl' => $platform['logoUrl'], - 'twitter' => $platform['twitterUrl'], - 'discord' => $platform['discordUrl'], - 'github' => $platform['githubUrl'], - 'terms' => $platform['termsUrl'], - 'privacy' => $platform['privacyUrl'], - 'platform' => $platform['platformName'], + 'accentColor' => $platform['accentColor'] ?? '', + 'logoUrl' => $platform['logoUrl'] ?? '', + 'twitter' => $platform['twitterUrl'] ?? '', + 'discord' => $platform['discordUrl'] ?? '', + 'github' => $platform['githubUrl'] ?? '', + 'terms' => $platform['termsUrl'] ?? '', + 'privacy' => $platform['privacyUrl'] ?? '', + 'platform' => $platform['platformName'] ?? 'Appwrite', ]); }
2584-2587: Critical: Add null check for emailSenderName platform key.
$platform['emailSenderName']is accessed without verifying the key exists. This could cause an undefined array key error if the platform provider doesn't guarantee this field.Apply this diff to add a null coalescing fallback:
// since this is console project, set email sender name! if ($smtpBaseTemplate === APP_BRANDED_EMAIL_BASE_TEMPLATE) { - $queueForMails->setSenderName($platform['emailSenderName']); + $queueForMails->setSenderName($platform['emailSenderName'] ?? $senderName); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/controllers/api/account.php(22 hunks)tests/e2e/General/HTTPTest.php(1 hunks)
🔇 Additional comments (7)
tests/e2e/General/HTTPTest.php (1)
34-34: LGTM!The test correctly validates that
X-SDK-Profileis now included in the allowed CORS headers, reflecting the infrastructure change to support platform context in API requests.app/controllers/api/account.php (6)
77-77: LGTM - Function signature updated correctly.The addition of the
array $platformparameter enables platform-specific branding in session alert emails.
209-209: LGTM - Function signature updated correctly.The addition of
array $platformparameter enables platform context propagation through session creation flows.
310-310: LGTM - Platform correctly propagated.The platform array is correctly passed to
sendSessionAlert, enabling platform-specific email branding.
968-977: LGTM - Platform injection configured correctly.Platform dependency injection is properly configured, and the action function signature correctly accepts the platform array for use in session alert emails.
1078-1078: LGTM - Platform correctly propagated.Platform array is properly passed to
sendSessionAlertfor branded email delivery.
1266-1274: LGTM - Platform injection configured correctly.Platform dependency is properly injected and will be passed to the
$createSessionclosure.
What does this PR do?
Use
platforminjection for emails on Account route.Test Plan
Manual.
Related PRs and Issues
N/A.
Checklist