🌵 [SPIKE] Pass locale from portal to magic link endpoint, send localized magic links in multiple languages! #23380
Conversation
WalkthroughThis change introduces locale-awareness to the magic link email flow throughout the application. The Suggested labels
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
ghost/core/core/server/services/lib/magic-link/MagicLink.js (1)
123-129: Fix ESLint spacing issueThere's inconsistent spacing around the default parameter assignment.
-function defaultGetText(url, type, email, locale=null) { +function defaultGetText(url, type, email, locale = null) {-function defaultGetHTML(url, type, email, locale=null) { +function defaultGetHTML(url, type, email, locale = null) {-function defaultGetSubject(type, locale=null) { +function defaultGetSubject(type, locale = null) {Also applies to: 140-146, 155-160
🧰 Tools
🪛 ESLint
[error] 123-123: 'locale' is assigned a value but never used.
(no-unused-vars)
[error] 123-123: Operator '=' must be spaced.
(space-infix-ops)
ghost/core/core/server/services/members/api.js (2)
78-98: Factor-out locale calculation & simplify switch for better readability
sitewideLocale→effectiveLocaleis computed in three different methods; extracting a tiny helper (e.g.resolveLocale(requestedLocale)) will remove duplication and the risk of the branches diverging later.await i18n.withLocale(effectiveLocale, async (t) => { … })creates an extra async layer that isn’t needed – nothing inside the callback is awaited. Removingasync/awaithere shaves a micro-task and makes stack-traces cleaner.- The explicit
signincase followed bydefault:is redundant. A lookup map keeps the intent obvious and eliminates the “useless case clause” flagged by Biome.Example refactor (trimmed for brevity):
-async getSubject(type, locale) { - const siteTitle = settingsCache.get('title'); - const effectiveLocale = resolveLocale(locale); - return await i18n.withLocale(effectiveLocale, async (t) => { - switch (type) { - … - case 'signin': - default: - return … - } - }); +async getSubject(type, locale) { + const siteTitle = settingsCache.get('title'); + const effectiveLocale = resolveLocale(locale); + return i18n.withLocale(effectiveLocale, (t) => { + const subjects = { + subscribe: `📫 ${t('Confirm your subscription to {siteTitle}', {siteTitle})}`, + signup: `🙌 ${t('Complete your sign up to {siteTitle}!', {siteTitle})}`, + 'signup-paid': `🙌 ${t('Thank you for signing up to {siteTitle}!', {siteTitle})}`, + updateEmail: `📫 ${t('Confirm your email update for {siteTitle}!', {siteTitle})}`, + signin: `🔑 ${t('Secure sign in link for {siteTitle}', {siteTitle})}` + }; + return subjects[type] ?? subjects.signin; + }); }Small, isolated helper functions will also keep unit-tests focused.
🧰 Tools
🪛 Biome (1.9.4)
[error] 93-94: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
193-217: Parsing the site domain: use the WHATWG URL API & optional-chainingLines 196-199 build a RegExp at runtime and access the match array manually:
const siteUrl = urlUtils.urlFor('home', true); const domain = urlUtils.urlFor('home', true).match(new RegExp('^https?://([^/:?#]+)(?:[/:?#]|$)', 'i')); const siteDomain = (domain && domain[1]);Safer and shorter:
-const siteUrl = urlUtils.urlFor('home', true); -const domain = urlUtils.urlFor('home', true).match(new RegExp('^https?://([^/:?#]+)(?:[/:?#]|$)', 'i')); -const siteDomain = (domain && domain[1]); +const siteUrl = urlUtils.urlFor('home', true); +const siteDomain = new URL(siteUrl).hostname;Benefits:
• Eliminates a dynamic RegExp (static-analysis hint)
• Avoids re-callingurlFor
• Handles edge-cases like non-standard ports automatically
• Readability ↑Optional-chaining (
urlUtils?.urlFor) could also be considered if you want to guard against unexpectedundefinedduring early boot.🧰 Tools
🪛 Biome (1.9.4)
[error] 199-200: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 212-213: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
[error] 198-199: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.(lint/complexity/useRegexLiterals)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/portal/src/actions.js(3 hunks)apps/portal/src/utils/api.js(2 hunks)ghost/core/core/server/services/i18n.js(3 hunks)ghost/core/core/server/services/lib/magic-link/MagicLink.js(5 hunks)ghost/core/core/server/services/members/api.js(2 hunks)ghost/core/core/server/services/members/members-api/controllers/RouterController.js(5 hunks)ghost/core/core/server/services/members/members-api/members-api.js(2 hunks)ghost/core/test/unit/api/canary/session.test.js(1 hunks)ghost/core/test/unit/server/services/lib/magic-link/index.test.js(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
ghost/core/core/server/services/members/members-api/members-api.js (4)
ghost/core/core/server/services/i18n.js (2)
locale(42-42)settingsCache(39-39)ghost/core/core/server/services/members/api.js (1)
settingsCache(2-2)ghost/core/core/server/services/members/members-api/controllers/RouterController.js (2)
tokenData(652-658)tokenData(674-674)ghost/core/core/server/services/members/members-api/controllers/MemberController.js (1)
tokenData(60-60)
ghost/core/core/server/services/i18n.js (2)
ghost/core/core/server/services/members/api.js (1)
i18n(21-21)ghost/core/test/unit/api/canary/session.test.js (1)
i18n(5-5)
🪛 ESLint
ghost/core/test/unit/server/services/lib/magic-link/index.test.js
[error] 81-81: Expected indentation of 12 spaces but found 8.
(indent)
[error] 84-84: Expected indentation of 12 spaces but found 8.
(indent)
ghost/core/core/server/services/i18n.js
[error] 13-13: Direct calls to new Error() are not allowed. Please use @tryghost/errors.
(ghost/ghost-custom/no-native-error)
[error] 26-26: Direct calls to new Error() are not allowed. Please use @tryghost/errors.
(ghost/ghost-custom/no-native-error)
ghost/core/core/server/services/lib/magic-link/MagicLink.js
[error] 123-123: 'locale' is assigned a value but never used.
(no-unused-vars)
[error] 123-123: Operator '=' must be spaced.
(space-infix-ops)
[error] 140-140: 'locale' is assigned a value but never used.
(no-unused-vars)
[error] 140-140: Operator '=' must be spaced.
(space-infix-ops)
[error] 155-155: 'locale' is assigned a value but never used.
(no-unused-vars)
[error] 155-155: Operator '=' must be spaced.
(space-infix-ops)
🪛 Biome (1.9.4)
ghost/core/core/server/services/members/api.js
[error] 93-94: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
[error] 172-173: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
[error] 199-200: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 212-213: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
[error] 198-199: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.
(lint/complexity/useRegexLiterals)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Database tests (Node 22.13.1, mysql8)
- GitHub Check: Database tests (Node 20.11.1, mysql8)
- GitHub Check: Database tests (Node 20.11.1, sqlite3)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Regression tests (Node 20.11.1, sqlite3)
- GitHub Check: Regression tests (Node 20.11.1, mysql8)
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Unit tests (Node 22.13.1)
🔇 Additional comments (15)
ghost/core/test/unit/api/canary/session.test.js (1)
5-6: Good addition of i18n initializationAdding i18n initialization before tests ensures that the internationalization service is properly set up for locale-aware functionality being tested.
apps/portal/src/actions.js (3)
86-86: Consistently passing locale to magic link APIGood addition of locale to the signin magic link flow, which ensures users receive emails in their preferred language.
108-108: Consistently passing locale to magic link APIGood addition of locale to the signup magic link flow, maintaining consistency with the signin flow.
514-514: Consistently passing locale to magic link APIGood addition of locale to the oneClickSubscribe flow, completing the locale support across all magic link scenarios.
ghost/core/core/server/services/members/members-api/members-api.js (2)
210-210: Good function signature update for locale supportAdding the optional locale parameter with a null default value is a solid approach that maintains backward compatibility while enabling new functionality.
220-223: Well-implemented locale fallback logicThe fallback logic is correct - checking for the provided locale first, then falling back to the site's default locale from settings cache, and finally defaulting to 'en' if neither is available.
apps/portal/src/utils/api.js (1)
265-282: LGTM – Locale parameter properly added to sendMagicLinkThe addition of an optional
localeparameter to thesendMagicLinkmethod and passing it in the request body (defaulting tonullwhen not provided) successfully implements the locale-aware functionality required for magic links.ghost/core/core/server/services/lib/magic-link/MagicLink.js (2)
59-59: Documentation properly updated for new parameterThe JSDoc update correctly documents the new optional locale parameter.
78-80: Correctly passing locale to email generation methodsThese changes correctly propagate the locale parameter to the email content generation methods, allowing for localized email content.
ghost/core/core/server/services/members/members-api/controllers/RouterController.js (3)
561-562: Locale parameter correctly added to request destructuringThe locale parameter is properly extracted from the request body with a null default, which aligns with the client-side implementation.
629-661: Locale properly propagated through signup flowThe code correctly passes the locale parameter to the email sending service during the signup process.
663-676: Locale properly propagated through signin flowThe code correctly passes the locale parameter to the email sending service during the signin process.
ghost/core/core/server/services/i18n.js (2)
36-66: Well-designed initialization functionThe init function is well-structured and properly handles both the initial locale setting and subsequent updates through events.
68-78: Good use of getters for accessing i18n functionalityThe module exports a well-designed interface that makes the i18n functionality easily accessible while maintaining proper encapsulation.
ghost/core/core/server/services/members/api.js (1)
21-21: Import looks correct – thank you for wiring i18n in the service layer
No issues spotted with the import path; this matches the newservices/i18n.jslocation and keeps the dependency surface small.
ghost/core/test/unit/server/services/lib/magic-link/index.test.js
Outdated
Show resolved
Hide resolved
…b.com/cathysarisky/Ghost into spike-locales-for-transactional-email
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/portal/src/tests/SigninFlow.test.js(6 hunks)apps/portal/src/tests/SignupFlow.test.js(6 hunks)ghost/core/core/server/services/i18n.js(3 hunks)ghost/core/core/server/services/lib/magic-link/MagicLink.js(5 hunks)ghost/core/test/unit/server/services/lib/magic-link/index.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- ghost/core/core/server/services/lib/magic-link/MagicLink.js
- ghost/core/core/server/services/i18n.js
🧰 Additional context used
🪛 ESLint
ghost/core/test/unit/server/services/lib/magic-link/index.test.js
[error] 72-72: Unexpected trailing comma.
(comma-dangle)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Database tests (Node 22.13.1, mysql8)
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Database tests (Node 20.11.1, mysql8)
- GitHub Check: Unit tests (Node 22.13.1)
- GitHub Check: Regression tests (Node 20.11.1, sqlite3)
- GitHub Check: Database tests (Node 20.11.1, sqlite3)
- GitHub Check: Regression tests (Node 20.11.1, mysql8)
- GitHub Check: Lint
🔇 Additional comments (4)
apps/portal/src/tests/SigninFlow.test.js (1)
157-158: LGTM: Locale parameter properly included in tests.The addition of
locale: expect.any(String)in the test expectations appropriately validates that the locale parameter is now being passed to the magic link endpoint, which aligns with the PR's objective of supporting localized magic links.ghost/core/test/unit/server/services/lib/magic-link/index.test.js (2)
80-84: Good: Text and HTML generation tests now include locale parameter.The test assertions now correctly verify that the locale parameter is passed to the email content generation functions. This ensures proper testing of the locale-aware email generation.
86-87: Looks good: Subject generation test includes locale parameter.The test assertion now correctly verifies that the locale parameter is passed to the email subject generation function.
apps/portal/src/tests/SignupFlow.test.js (1)
238-239: LGTM: Locale parameter properly included in signup tests.The addition of
locale: expect.any(String)consistently across all test assertions correctly validates that the locale parameter is now being passed through the signup flow, which aligns with the PR's objective of supporting localized magic links.
ghost/core/test/unit/server/services/lib/magic-link/index.test.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
ghost/core/test/e2e-api/members/send-magic-link.test.js (4)
417-419: Fix padding in test suite declarationRemove the blank line after the opening brace of the describe block to adhere to code style guidelines.
describe('Magic links can be localized by passing a locale', function () { - it('Sends magic link email in French when passedlocale is fr', async function () {🧰 Tools
🪛 ESLint
[error] 417-419: Block must not be padded by blank lines.
(padded-blocks)
441-461: Maintain consistent string quote styleThis test correctly verifies English localization, but use single quotes for strings to maintain consistency with the codebase style.
- should(mail.text).containEql("Tap the link below to complete the signup process for Ghost"); - should(mail.html).containEql("Tap the link below to complete the signup process for Ghost"); + should(mail.text).containEql('Tap the link below to complete the signup process for Ghost'); + should(mail.html).containEql('Tap the link below to complete the signup process for Ghost');🧰 Tools
🪛 ESLint
[error] 459-459: Strings must use singlequote.
(quotes)
[error] 460-460: Strings must use singlequote.
(quotes)
512-536: Consider completing fallback test for unknown localesThe commented-out test for handling unknown locales is important for ensuring robust behavior. Consider completing this test once the fallback behavior is fixed.
I can help implement this test once the fallback behavior is properly determined (currently it's defaulting to English instead of the sitewide locale).
417-537: Add test for signin email type with localeThe current tests focus on the signup email type, but it would be beneficial to include a test for the signin email type with a specified locale to ensure complete coverage of the feature.
Consider adding a test like:
it('Sends signin magic link email with the specified locale', async function () { // Create a member first so signin works const email = '[email protected]'; await membersService.api.members.create({email, name: 'Signin Test'}); // Request a magic link with German locale await membersAgent.post('/api/send-magic-link') .body({ email, emailType: 'signin', locale: 'de' }) .expectEmptyBody() .expectStatus(201); // Check email is sent with German subject const mail = mockManager.assert.sentEmail({ to: email, subject: /Melden Sie sich bei Ghost an/ }); // Verify German content in email should(mail.text).match(/Klicken Sie auf den Link unten, um sich bei/); should(mail.html).match(/Klicken Sie auf den Link unten, um sich bei/); });🧰 Tools
🪛 ESLint
[error] 417-419: Block must not be padded by blank lines.
(padded-blocks)
[error] 459-459: Strings must use singlequote.
(quotes)
[error] 460-460: Strings must use singlequote.
(quotes)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/portal/src/tests/portal-links.test.js(1 hunks)ghost/core/test/e2e-api/members/send-magic-link.test.js(1 hunks)ghost/core/test/unit/server/services/lib/magic-link/index.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/test/unit/server/services/lib/magic-link/index.test.js
🧰 Additional context used
🧬 Code Graph Analysis (1)
ghost/core/test/e2e-api/members/send-magic-link.test.js (1)
ghost/core/test/e2e-api/admin/posts-legacy.test.js (1)
mockManager(13-13)
🪛 ESLint
ghost/core/test/e2e-api/members/send-magic-link.test.js
[error] 417-419: Block must not be padded by blank lines.
(padded-blocks)
[error] 459-459: Strings must use singlequote.
(quotes)
[error] 460-460: Strings must use singlequote.
(quotes)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Database tests (Node 20.11.1, mysql8)
- GitHub Check: Database tests (Node 22.13.1, mysql8)
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Database tests (Node 20.11.1, sqlite3)
- GitHub Check: Regression tests (Node 20.11.1, mysql8)
- GitHub Check: Unit tests (Node 22.13.1)
- GitHub Check: Regression tests (Node 20.11.1, sqlite3)
- GitHub Check: Ghost-CLI tests
🔇 Additional comments (4)
apps/portal/src/tests/portal-links.test.js (1)
235-236: LGTM: Properly testing locale parameter additionThis change correctly adds verification that the locale parameter is passed to the
sendMagicLinkAPI call, which aligns with the PR objective of supporting localized magic links.ghost/core/test/e2e-api/members/send-magic-link.test.js (3)
419-439: LGTM: Good test coverage for French localeThis test properly verifies that when passing
locale: 'fr'to the magic link endpoint, the email is sent with French content, checking both subject and body content.
463-485: LGTM: Good test for sitewide locale fallbackThis test properly verifies that the magic link system falls back to the sitewide locale (Spanish in this case) when no explicit locale is provided in the request.
487-511: LGTM: Good test for locale override behaviorThis test correctly verifies that a locale explicitly passed in the request overrides the sitewide locale setting, which is important for the feature to work properly.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ghost/core/core/server/services/members/api.js (1)
121-213: Large text templates still duplicate 80 % of their contentPrevious reviews already pointed out that having five near-identical blocks is hard to maintain.
Please consider extracting common boiler-plate and using a data-driven lookup instead of aswitch.🧰 Tools
🪛 Biome (1.9.4)
[error] 193-194: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
🧹 Nitpick comments (3)
ghost/core/core/server/services/members/api.js (3)
21-23: Avoid splitting i18n concerns across two packagesThe file imports the default
i18ninstance from../i18nand theSUPPORTED_LOCALESconstant from@tryghost/i18n.
In most of Ghost’s code-base the singleton instance and the constants live in the same package; mixing them can lead to version skew or duplicated bundles when the two packages drift.Consider importing both from the same source:
-const i18n = require('../i18n'); -const {SUPPORTED_LOCALES} = require('@tryghost/i18n'); +const {i18n, SUPPORTED_LOCALES} = require('../i18n'); // ← or `@tryghost/i18n` – choose oneThis keeps your dependency graph simpler and reduces risk of accidentally loading two different sets of translations.
101-120: Switch repetition – remove redundantsigninclause
case 'signin':is followed immediately bydefault:returning the exact same template string, so the explicit case is never reached.
Removing it cleans up the control flow and will silence the Biome “useless case clause” warning.- case 'signin': default: return `🔑 ${t(\`Secure sign in link for {siteTitle}\`, {siteTitle, interpolation: {escapeValue: false}})}`;(If you keep the named clause for readability, add a
// falls throughcomment so linters know it’s intentional.)🧰 Tools
🪛 Biome (1.9.4)
[error] 115-116: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
214-236: RegExp & optional-chaining clean-ups
- Creating a
RegExpobject on every call is unnecessary; a literal is faster and clearer.- The
domainarray access can be simplified using optional chaining.-const domain = urlUtils.urlFor('home', true).match(new RegExp('^https?://([^/:?#]+)(?:[/:?#]|$)', 'i')); -const siteDomain = (domain && domain[1]); +const domainMatch = siteUrl.match(/^https?:\/\/([^/:?#]+)(?:[/:?#]|$)/i); +const siteDomain = domainMatch?.[1];These are small, low-risk changes that improve readability and address the linter hints.
🧰 Tools
🪛 Biome (1.9.4)
[error] 219-220: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 232-233: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
[error] 218-219: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.(lint/complexity/useRegexLiterals)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ghost/core/core/server/services/members/api.js(3 hunks)ghost/core/test/e2e-api/members/send-magic-link.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/test/e2e-api/members/send-magic-link.test.js
🧰 Additional context used
🧬 Code Graph Analysis (1)
ghost/core/core/server/services/members/api.js (1)
ghost/core/core/server/services/i18n.js (4)
i18n(42-42)locale(47-47)settingsCache(44-44)t(36-36)
🪛 Biome (1.9.4)
ghost/core/core/server/services/members/api.js
[error] 115-116: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
[error] 193-194: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
[error] 219-220: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 232-233: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
[error] 218-219: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.
(lint/complexity/useRegexLiterals)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Regression tests (Node 20.11.1, mysql8)
- GitHub Check: Regression tests (Node 20.11.1, sqlite3)
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Unit tests (Node 22.13.1)
- GitHub Check: Database tests (Node 20.11.1, sqlite3)
- GitHub Check: Database tests (Node 20.11.1, mysql8)
- GitHub Check: Database tests (Node 22.13.1, mysql8)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
ghost/core/core/server/services/members/api.js (2)
31-47: 🛠️ Refactor suggestionCache locale validation to improve performance
The
validateLocalefunction is called multiple times for each email (ingetSubject,getText, andgetHTML), causing unnecessary async operations and potential I/O overhead. Consider caching validation results to avoid redundant locale loading.+// Cache promises so concurrent calls share the same load +const _localeLoadCache = new Map(); -async function validateLocale(locale) { - const sitewideLocale = settingsCache.get('locale'); +async function validateLocale(rawLocale) { + const sitewideLocale = (settingsCache.get('locale') || 'en').replace('_', '-').toLowerCase(); - // If no locale provided, use sitewide locale or fall back to English - if (!locale || !SUPPORTED_LOCALES.includes(locale)) { - return sitewideLocale || 'en'; + // Fast-path when no explicit locale is provided + if (!rawLocale) { + return sitewideLocale; } + // Canonicalise: `en_US` → `en-us` + const locale = rawLocale.replace('_', '-').toLowerCase(); + + if (!SUPPORTED_LOCALES.includes(locale)) { + return sitewideLocale; + } - try { - // Try to load the locale to validate it exists - await i18n.loadLocale(locale); - return locale; - } catch (e) { - // If locale loading fails, fall back to sitewide locale or English - return sitewideLocale || 'en'; + // De-duplicate parallel calls + if (!_localeLoadCache.has(locale)) { + _localeLoadCache.set(locale, i18n.loadLocale(locale).catch(() => null)); + } + + const loaded = await _localeLoadCache.get(locale); + return loaded ? locale : sitewideLocale; }
117-209: 🛠️ Refactor suggestionReduce duplication in email templates
The five email templates duplicate 80% of their content. This makes maintenance difficult and increases the risk of inconsistencies.
Consider refactoring to:
- Extract common elements (greeting, footer, expiration notice) into shared helper functions
- Keep only the unique content for each email type
- Use a data-driven approach instead of a switch statement
async getText(url, type, email, locale) { const siteTitle = settingsCache.get('title'); const effectiveLocale = await validateLocale(locale); return await i18n.withLocale(effectiveLocale, async (t) => { + // Common template parts + const greeting = t(`Hey there,`); + const expirationNotice = t('For your security, the link will expire in 24 hours time.'); + const separator = '---'; + const sentTo = t('Sent to {email}', {email}); + + // Type-specific content + const typeContent = { + 'subscribe': { + mainText: t('You\'re one tap away from subscribing to {siteTitle} — please confirm your email address with this link:', {siteTitle, interpolation: {escapeValue: false}}), + closingText: t('All the best!'), + disclaimerText: `${t('If you did not make this request, you can simply delete this message.')} ${t('You will not be subscribed.')}` + }, + 'signup': { + mainText: t('Tap the link below to complete the signup process for {siteTitle}, and be automatically signed in:', {siteTitle, interpolation: {escapeValue: false}}), + closingText: t('See you soon!'), + disclaimerText: `${t('If you did not make this request, you can simply delete this message.')} ${t('You will not be signed up, and no account will be created for you.')}` + }, + // Add other types here + }; + + // Default to signin if type not found + const content = typeContent[type] || typeContent['signin']; + + return trimLeadingWhitespace` + ${greeting} + + ${content.mainText} + + ${url} + + ${expirationNotice} + + ${content.closingText || ''} + + ${separator} + + ${sentTo} + ${content.disclaimerText} + `; - switch (type) { - case 'subscribe': - return trimLeadingWhitespace` - ${t(`Hey there,`)} - - ${t('You\'re one tap away from subscribing to {siteTitle} — please confirm your email address with this link:', {siteTitle, interpolation: {escapeValue: false}})} - - ${url} - - ${t('For your security, the link will expire in 24 hours time.')} - - ${t('All the best!')} - - --- - - ${t('Sent to {email}', {email})} - ${t('If you did not make this request, you can simply delete this message.')} ${t('You will not be subscribed.')} - `; - // Rest of cases... - } }); },🧰 Tools
🪛 Biome (1.9.4)
[error] 189-190: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
🧹 Nitpick comments (2)
ghost/core/core/server/services/members/api.js (2)
97-116: Consider using a data-driven approach instead of switch statementThe switch statement for translating email subjects could be replaced with an object map for better maintainability. Also, fix the "Useless case clause" flagged by static analysis.
async getSubject(type, locale) { const siteTitle = settingsCache.get('title'); const effectiveLocale = await validateLocale(locale); return await i18n.withLocale(effectiveLocale, async (t) => { - switch (type) { - case 'subscribe': - return `📫 ${t(`Confirm your subscription to {siteTitle}`, {siteTitle, interpolation: {escapeValue: false}})}`; - case 'signup': - return `🙌 ${t(`Complete your sign up to {siteTitle}!`, {siteTitle, interpolation: {escapeValue: false}})}`; - case 'signup-paid': - return `🙌 ${t(`Thank you for signing up to {siteTitle}!`, {siteTitle, interpolation: {escapeValue: false}})}`; - case 'updateEmail': - return `📫 ${t(`Confirm your email update for {siteTitle}!`, {siteTitle, interpolation: {escapeValue: false}})}`; - case 'signin': - default: - return `🔑 ${t(`Secure sign in link for {siteTitle}`, {siteTitle, interpolation: {escapeValue: false}})}`; - } + const subjectTemplates = { + 'subscribe': `📫 ${t(`Confirm your subscription to {siteTitle}`, {siteTitle, interpolation: {escapeValue: false}})}`, + 'signup': `🙌 ${t(`Complete your sign up to {siteTitle}!`, {siteTitle, interpolation: {escapeValue: false}})}`, + 'signup-paid': `🙌 ${t(`Thank you for signing up to {siteTitle}!`, {siteTitle, interpolation: {escapeValue: false}})}`, + 'updateEmail': `📫 ${t(`Confirm your email update for {siteTitle}!`, {siteTitle, interpolation: {escapeValue: false}})}`, + 'signin': `🔑 ${t(`Secure sign in link for {siteTitle}`, {siteTitle, interpolation: {escapeValue: false}})}` + }; + + return subjectTemplates[type] || subjectTemplates['signin']; }); },🧰 Tools
🪛 Biome (1.9.4)
[error] 111-112: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
214-215: Use regex literal and optional chaining for better codeThe static analysis tools flagged a few improvements that can be made here:
-const domain = urlUtils.urlFor('home', true).match(new RegExp('^https?://([^/:?#]+)(?:[/:?#]|$)', 'i')); -const siteDomain = (domain && domain[1]); +const domain = urlUtils.urlFor('home', true).match(/^https?:\/\/([^/:?#]+)(?:[/:?#]|$)/i); +const siteDomain = domain?.[1];🧰 Tools
🪛 Biome (1.9.4)
[error] 214-215: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.(lint/complexity/useRegexLiterals)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ghost/core/core/server/services/members/api.js(3 hunks)ghost/core/test/unit/server/services/lib/magic-link/index.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/test/unit/server/services/lib/magic-link/index.test.js
🧰 Additional context used
🪛 Biome (1.9.4)
ghost/core/core/server/services/members/api.js
[error] 111-112: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
[error] 189-190: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
[error] 215-216: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 228-229: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
[error] 214-215: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.
(lint/complexity/useRegexLiterals)
🔇 Additional comments (3)
ghost/core/core/server/services/members/api.js (3)
21-22: Import changes look goodThe code now imports the full i18n module and SUPPORTED_LOCALES constant, which are essential for the new locale-aware functionality.
210-233: Email HTML generation is now properly localizedThe HTML email generation now correctly passes the translation function to the email templates, enabling localized HTML emails.
Note: Consider refactoring to use a data-driven approach similar to the suggestion for
getSubjectto eliminate the "Useless case clause" warning.🧰 Tools
🪛 Biome (1.9.4)
[error] 215-216: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 228-229: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
[error] 214-215: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.(lint/complexity/useRegexLiterals)
31-215:✅ Verification successful
Verify the impact of asynchronous email functions
This change converts synchronous email functions to asynchronous ones. Ensure all calling code has been updated to handle these promises correctly.
🏁 Script executed:
#!/bin/bash # Search for direct calls to getSubject, getText, and getHTML that might not be awaiting the result echo "Searching for potentially non-awaited calls to getSubject, getText, and getHTML..." rg "getSubject\(|getText\(|getHTML\(" --type js | grep -v "await " | grep -v "async"Length of output: 1759
No missing awaits for members API email functions
The search for non-awaited calls to
getSubject,getText, andgetHTMLonly surfaced references in other services (SettingsBREADService,NewslettersService,EmailRenderer) and unit tests—not calls on themembersApiInstancereturned bycreateApiInstance. Since these async methods are invoked internally by the@tryghost/members-apilibrary and there are no direct calls in our code to await them, no changes are required.🧰 Tools
🪛 Biome (1.9.4)
[error] 111-112: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
[error] 189-190: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
[error] 214-215: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.(lint/complexity/useRegexLiterals)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
ghost/core/core/server/services/members/api.js (2)
127-218: 🛠️ Refactor suggestionLarge duplicate template blocks should be refactored
This locale-aware implementation works but contains significant code duplication that makes maintenance difficult.
Refactor the large switch statement with nearly identical templates to reduce duplication:
async getText(url, type, email, locale) { const siteTitle = settingsCache.get('title'); const effectiveLocale = await validateLocale(locale); return await i18n.withLocale(effectiveLocale, async (t) => { + // Common email sections + const greeting = t(`Hey there,`); + const securityNotice = t('For your security, the link will expire in 24 hours time.'); + const separator = '---'; + const sentTo = t('Sent to {email}', {email}); + + // Type-specific content + const typeContent = { + 'subscribe': { + mainContent: t('You\'re one tap away from subscribing to {siteTitle} — please confirm your email address with this link:', {siteTitle, interpolation: {escapeValue: false}}), + farewell: t('All the best!'), + disclaimer: `${t('If you did not make this request, you can simply delete this message.')} ${t('You will not be subscribed.')}` + }, + 'signup': { + mainContent: t('Tap the link below to complete the signup process for {siteTitle}, and be automatically signed in:', {siteTitle, interpolation: {escapeValue: false}}), + farewell: t('See you soon!'), + disclaimer: `${t('If you did not make this request, you can simply delete this message.')} ${t('You will not be signed up, and no account will be created for you.')}` + }, + 'signup-paid': { + mainContent: t('Thank you for subscribing to {siteTitle}. Tap the link below to be automatically signed in:', {siteTitle, interpolation: {escapeValue: false}}), + farewell: t('See you soon!'), + disclaimer: t('Thank you for subscribing to {siteTitle}!', {siteTitle, interpolation: {escapeValue: false}}) + }, + 'updateEmail': { + mainContent: t('Please confirm your email address with this link:'), + farewell: '', + disclaimer: `${t('If you did not make this request, you can simply delete this message.')} ${t('This email address will not be used.')}` + }, + 'signin': { + mainContent: t('Welcome back! Use this link to securely sign in to your {siteTitle} account:', {siteTitle, interpolation: {escapeValue: false}}), + farewell: t('See you soon!'), + disclaimer: t('If you did not make this request, you can safely ignore this email.') + } + }; + + // Default to signin if type not found + const content = typeContent[type] || typeContent['signin']; + + // Assemble email from components + return trimLeadingWhitespace` + ${greeting} + + ${content.mainContent} + + ${url} + + ${securityNotice} + + ${content.farewell ? content.farewell + '\n\n' : ''}${separator} + + ${sentTo} + ${content.disclaimer} + `; - switch (type) { - case 'subscribe': - return trimLeadingWhitespace` - ${t(`Hey there,`)} - - ${t('You\'re one tap away from subscribing to {siteTitle} — please confirm your email address with this link:', {siteTitle, interpolation: {escapeValue: false}})} - - ${url} - - ${t('For your security, the link will expire in 24 hours time.')} - - ${t('All the best!')} - - --- - - ${t('Sent to {email}', {email})} - ${t('If you did not make this request, you can simply delete this message.')} ${t('You will not be subscribed.')} - `; - case 'signup': - return trimLeadingWhitespace` - ${t(`Hey there,`)} - - ${t('Tap the link below to complete the signup process for {siteTitle}, and be automatically signed in:', {siteTitle, interpolation: {escapeValue: false}})} - - ${url} - - ${t('For your security, the link will expire in 24 hours time.')} - - ${t('See you soon!')} - - --- - - ${t('Sent to {email}', {email})} - ${t('If you did not make this request, you can simply delete this message.')} ${t('You will not be signed up, and no account will be created for you.')} - `; - case 'signup-paid': - return trimLeadingWhitespace` - ${t(`Hey there,`)} - - ${t('Thank you for subscribing to {siteTitle}. Tap the link below to be automatically signed in:', {siteTitle, interpolation: {escapeValue: false}})} - - ${url} - - ${t('For your security, the link will expire in 24 hours time.')} - - ${t('See you soon!')} - - --- - - ${t('Sent to {email}', {email})} - ${t('Thank you for subscribing to {siteTitle}!', {siteTitle, interpolation: {escapeValue: false}})} - `; - case 'updateEmail': - return trimLeadingWhitespace` - ${t(`Hey there,`)} - - ${t('Please confirm your email address with this link:')} - - ${url} - - ${t('For your security, the link will expire in 24 hours time.')} - - --- - - ${t('Sent to {email}', {email})} - ${t('If you did not make this request, you can simply delete this message.')} ${t('This email address will not be used.')} - `; - case 'signin': - default: - return trimLeadingWhitespace` - ${t(`Hey there,`)} - - ${t('Welcome back! Use this link to securely sign in to your {siteTitle} account:', {siteTitle, interpolation: {escapeValue: false}})} - - ${url} - - ${t('For your security, the link will expire in 24 hours time.')} - - ${t('See you soon!')} - - --- - - ${t('Sent to {email}', {email})} - ${t('If you did not make this request, you can safely ignore this email.')} - `; - } }); }🧰 Tools
🪛 Biome (1.9.4)
[error] 199-200: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
31-57: 🛠️ Refactor suggestionLocale cache implementation could be more robust
The locale validation and caching logic is a good improvement, but it could be enhanced to handle edge cases better.
Consider implementing these improvements:
- Normalize locale strings (convert to lowercase, replace underscores with hyphens)
- Cache promise results to prevent duplicate concurrent loads of the same locale
// Cache for successfully loaded locales -const _loadedLocalesCache = new Set(); +// Cache both successful loads and in-flight promises +const _localeLoadCache = new Map(); async function validateLocale(rawLocale) { const sitewideLocale = settingsCache.get('locale'); + // Normalize sitewideLocale for consistency + const normalizedSitewideLocale = (sitewideLocale || 'en').replace('_', '-').toLowerCase(); // If no locale provided, use sitewide locale or fall back to English - if (!locale || !SUPPORTED_LOCALES.includes(locale)) { - return sitewideLocale || 'en'; + if (!rawLocale) { + return normalizedSitewideLocale; } + // Normalize locale string (en_US → en-us) + const locale = rawLocale.replace('_', '-').toLowerCase(); + + if (!SUPPORTED_LOCALES.includes(locale)) { + return normalizedSitewideLocale; + } - // Return cached locale if it's already been loaded successfully - if (_loadedLocalesCache.has(locale)) { - return locale; - } + // De-duplicate parallel calls for the same locale + if (!_localeLoadCache.has(locale)) { + _localeLoadCache.set(locale, i18n.loadLocale(locale) + .then(() => locale) + .catch(() => normalizedSitewideLocale)); + } - try { - // Try to load the locale to validate it exists - await i18n.loadLocale(locale); - // Cache the successfully loaded locale - _loadedLocalesCache.add(locale); - return locale; - } catch (e) { - // If locale loading fails, fall back to sitewide locale or English - return sitewideLocale || 'en'; - } + return _localeLoadCache.get(locale); }
🧹 Nitpick comments (2)
ghost/core/core/server/services/members/api.js (2)
107-126: Good async locale-aware implementation but with a redundant caseThe implementation correctly handles localized email subjects but contains a minor redundancy.
Remove the redundant 'signin' case that's identical to the default:
return await i18n.withLocale(effectiveLocale, async (t) => { switch (type) { case 'subscribe': return `📫 ${t(`Confirm your subscription to {siteTitle}`, {siteTitle, interpolation: {escapeValue: false}})}`; case 'signup': return `🙌 ${t(`Complete your sign up to {siteTitle}!`, {siteTitle, interpolation: {escapeValue: false}})}`; case 'signup-paid': return `🙌 ${t(`Thank you for signing up to {siteTitle}!`, {siteTitle, interpolation: {escapeValue: false}})}`; case 'updateEmail': return `📫 ${t(`Confirm your email update for {siteTitle}!`, {siteTitle, interpolation: {escapeValue: false}})}`; - case 'signin': default: return `🔑 ${t(`Secure sign in link for {siteTitle}`, {siteTitle, interpolation: {escapeValue: false}})}`; } });🧰 Tools
🪛 Biome (1.9.4)
[error] 121-122: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
220-243: Good HTML generation implementation with minor improvement opportunitiesThe HTML email generation code correctly implements locale awareness and passes the translation function to the email templates.
Consider these small improvements:
async getHTML(url, type, email, locale) { const siteTitle = settingsCache.get('title'); const effectiveLocale = await validateLocale(locale); const siteUrl = urlUtils.urlFor('home', true); - const domain = urlUtils.urlFor('home', true).match(new RegExp('^https?://([^/:?#]+)(?:[/:?#]|$)', 'i')); - const siteDomain = (domain && domain[1]); + const domain = urlUtils.urlFor('home', true).match(/^https?:\/\/([^/:?#]+)(?:[/:?#]|$)/i); + const siteDomain = domain?.[1]; const accentColor = settingsCache.get('accent_color'); return await i18n.withLocale(effectiveLocale, async (t) => { switch (type) { case 'subscribe': return subscribeEmail({t, url, email, siteTitle, accentColor, siteDomain, siteUrl}); case 'signup': return signupEmail({t, url, email, siteTitle, accentColor, siteDomain, siteUrl}); case 'signup-paid': return signupPaidEmail({t, url, email, siteTitle, accentColor, siteDomain, siteUrl}); case 'updateEmail': return updateEmail({t, url, email, siteTitle, accentColor, siteDomain, siteUrl}); - case 'signin': default: return signinEmail({t, url, email, siteTitle, accentColor, siteDomain, siteUrl}); } }); }🧰 Tools
🪛 Biome (1.9.4)
[error] 225-226: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 238-239: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
[error] 224-225: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.(lint/complexity/useRegexLiterals)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ghost/core/core/server/services/members/api.js(3 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
ghost/core/core/server/services/members/api.js
[error] 121-122: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
[error] 199-200: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
[error] 225-226: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 238-239: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
[error] 224-225: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.
(lint/complexity/useRegexLiterals)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Database tests (Node 20.11.1, mysql8)
- GitHub Check: Unit tests (Node 22.13.1)
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Regression tests (Node 20.11.1, mysql8)
- GitHub Check: Database tests (Node 22.13.1, mysql8)
- GitHub Check: Database tests (Node 20.11.1, sqlite3)
- GitHub Check: Regression tests (Node 20.11.1, sqlite3)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Lint
🔇 Additional comments (1)
ghost/core/core/server/services/members/api.js (1)
21-22: Appropriate i18n dependencies for localization supportGood addition of the necessary imports for internationalization support. This correctly brings in the i18n module and the list of supported locales needed for the locale validation logic.
🌵 Welcome to my spike. :)
This PR adds being able to pass a locale to magic link requests, resulting in users receiving sign-in/subscribe/sign-up links in whatever language is passed.
Portal already accommodates multiple languages, accepting a data-locale attribute and using it to overwrite sitewide locale when passed. This PR causes Portal to also pass the locale to the magic link endpoint, and sets up the magic link endpoint to receive it and use it for sending localized text.
Where things are:
[X] Appears functional in manual testing
[X] More tests needed
[ ] Think about whether it should also be possible from data-attributes.