fix(i18n): localize invite, magic-link, and recovery emails - #1944
fix(i18n): localize invite, magic-link, and recovery emails#1944swissky wants to merge 4 commits into
Conversation
🦋 Changeset detectedLatest commit: bb84501 The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
Bot
added
review/needs-review
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-verification
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
The approach is sound: keep the auth package i18n-free by injecting final display strings, let the admin package own Lingui catalog resolution, and have EmDash core pick the locale from the site option or the requesting user's admin language. That separation matches the architecture and keeps the auth package backwards-compatible.
I checked the new/updated files, the export wiring between @emdash-cms/auth, @emdash-cms/admin, and emdash, the HTML-escaping changes in the email builders, the test files, and the changeset. The code is clean and the builders correctly fall back to English when no localized strings are passed.
One gap: the newly-added emdash core helper getEmailLocale implements the locale priority logic, but there is no unit or integration test in packages/core to exercise it. The existing tests cover the auth builders and the admin catalog resolution separately, but they do not prove that the site emdash:locale option wins over the request locale, or that the routes actually wire localized strings through. Per AGENTS.md's TDD convention, a bug fix should include a reproducing test for the new logic.
| export async function getEmailLocale(db: Kysely<Database>, request: Request): Promise<string> { | ||
| const options = new OptionsRepository(db); | ||
| const siteLocale = await options.get<string>("emdash:locale"); | ||
| // loadMessages falls back to English for unsupported codes, so a | ||
| // free-form option value degrades safely. | ||
| if (typeof siteLocale === "string" && siteLocale) return siteLocale; | ||
| return resolveLocale(request); | ||
| } |
There was a problem hiding this comment.
[needs fixing] This helper implements the locale-priority decision (site emdash:locale → request cookie/Accept-Language → English), but there is no test for it in packages/core/tests. The PR tests the auth builders and the admin catalog resolution in isolation, so the priority logic and route-level wiring are unverified. Add a unit test that stubs OptionsRepository and checks the fallback chain, or an integration test that sends an invite with a non-English site locale and asserts the email subject is localized.
| export async function getEmailLocale(db: Kysely<Database>, request: Request): Promise<string> { | |
| const options = new OptionsRepository(db); | |
| const siteLocale = await options.get<string>("emdash:locale"); | |
| // loadMessages falls back to English for unsupported codes, so a | |
| // free-form option value degrades safely. | |
| if (typeof siteLocale === "string" && siteLocale) return siteLocale; | |
| return resolveLocale(request); | |
| } | |
| export async function getEmailLocale(db: Kysely<Database>, request: Request): Promise<string> { | |
| const options = new OptionsRepository(db); | |
| const siteLocale = await options.get<string>("emdash:locale"); | |
| if (typeof siteLocale === "string" && siteLocale) return siteLocale; | |
| return resolveLocale(request); | |
| } |
There was a problem hiding this comment.
Added in 8f009d6 — tests/unit/api/email-locale.test.ts covers the full chain against a real test DB: site emdash:locale wins over cookie and Accept-Language, cookie wins when no site locale is set, Accept-Language matching (including region tags like es-ES), unsupported cookie values falling through, and the English default with no signals.
github-actions
Bot
added
review/awaiting-author
Review follow-up: unit-tests the site-locale -> cookie -> Accept-Language -> English fallback against a real test database.
github-actions
Bot
added
size/XL
review/needs-rereview
ascorbic
added
bot:review
There was a problem hiding this comment.
Re-review of #1944. The approach is still sound: keep @emdash-cms/auth i18n-free by injecting final display strings, resolve those strings in @emdash-cms/admin from the Lingui catalogs, and pick the locale in emdash core from the site option / request locale. My previous concern about missing test coverage for the new getEmailLocale helper is fixed by the new packages/core/tests/unit/api/email-locale.test.ts, which exercises site-option priority, cookie fallback, Accept-Language fallback, and the English default.
I re-read the new/updated files, the cross-package export wiring, the HTML-escaping changes in the email builders, the changeset, and the test files. The code is clean and backwards-compatible: the auth builders fall back to English when no localized strings are passed, and the new emailStrings fields are optional. No logic bugs or security issues found.
Two non-blocking suggestions remain:
-
packages/core/src/api/email-locale.tsimportsresolveLocalethrough the@emdash-cms/admin/localesbarrel, which pulls in the Lingui macro module (emails.ts) even thoughgetEmailLocaleonly needs locale resolution. Importing from@emdash-cms/admin/locales/config.jswould keep the dependency narrower and avoid evaluating the macro-heavy module for this code path. -
The HTML email templates now support Arabic/Farsi/etc. content, but the root
<html>element has nodirorlangattributes, so RTL text will render left-to-right. Adding direction/language metadata (e.g.,dir="auto"and a plumbedlang) would make localized emails actually direction-correct for RTL locales.
Neither blocks merge.
| * makes for its system mails. | ||
| */ | ||
|
|
||
| import { resolveLocale } from "@emdash-cms/admin/locales"; |
There was a problem hiding this comment.
[suggestion] getEmailLocale only needs resolveLocale, but this imports the @emdash-cms/admin/locales barrel. That transitively loads emails.ts, which contains Lingui msg macros that have to be compiled by the admin build / Astro Vite plugin. Importing the locale config directly avoids pulling the macro module into this code path.
| import { resolveLocale } from "@emdash-cms/admin/locales"; | |
| import { resolveLocale } from "@emdash-cms/admin/locales/config.js"; |
There was a problem hiding this comment.
Looked at this — importing ./locales/config.js directly doesn't actually avoid the issue: config.ts itself uses import.meta.env.DEV and the __EMDASH_PSEUDO_LOCALE__ global, so it needs the same Vite pipeline either way. Meanwhile admin.astro and the magic-link/invite routes already import the barrel (loadMessages, getInviteEmailStrings), so the barrel is on the runtime path regardless and tree-shaking keeps this import to resolveLocale + config.ts only. Moving resolveLocale to a Vite-free module would be a separate refactor of the locales package layout — happy to file it as a follow-up if you want it tracked.
| <body style="font-family: -apple-system, BlinkMacSystemFont, 'Segoe UI', Roboto, sans-serif; line-height: 1.5; color: #333; max-width: 600px; margin: 0 auto; padding: 20px;"> | ||
| <h1 style="font-size: 24px; margin-bottom: 20px;">You've been invited to ${safeName}</h1> | ||
| <p>Click the button below to create your account:</p> | ||
| <h1 style="font-size: 24px; margin-bottom: 20px;">${escapeHtml(s.subject)}</h1> |
There was a problem hiding this comment.
[suggestion] The localized strings may now be RTL (Arabic, Farsi, etc.), but the email root has no dir or lang attributes, so RTL content will render LTR. Consider threading the locale/direction through the builder and setting them on the root <html> element; the same applies to packages/auth/src/magic-link/index.ts.
There was a problem hiding this comment.
Done — the resolved locale is now threaded through to the invite/magic-link/recovery builders, which set lang + dir on the root <html> element. Direction is derived from the BCP 47 primary subtag via a small localeDir() helper in @emdash-cms/auth (keeps the auth package free of the admin dependency). Covered by new tests asserting <html lang="ar" dir="rtl">.
github-actions
Bot
added
review/awaiting-author
Localized invite/magic-link/recovery copy may be RTL (Arabic, Farsi, etc.), but the email HTML root had no lang/dir, so RTL text rendered LTR. Thread the resolved locale through to the builders and set lang + dir on the <html> element (localeDir derived from the BCP 47 primary subtag, keeping @emdash-cms/auth free of the admin dep).
github-actions
Bot
added
review/needs-rereview
There was a problem hiding this comment.
Re-review of #1944. The approach is still sound: keep @emdash-cms/auth i18n-free by injecting final display strings, resolve those strings in @emdash-cms/admin from the Lingui catalogs, and pick the locale in emDash core from the site option / request locale. This decouples the packages and keeps the auth API backwards-compatible.
The previous RTL/lang/dir concern is fixed: the email builders now thread locale through and emit <html lang="..." dir="rtl"> (or ltr) based on a primary-subtag RTL list. The new email-templates.test.ts covers this, and the new email-locale.test.ts exercises locale priority.
I re-read the changed files, the cross-package export wiring, the HTML-escaping changes, the changeset, and the tests. No logic bugs or security issues found. All findings are minor suggestions:
- Still open from the prior review:
packages/core/src/api/email-locale.tsimportsresolveLocalethrough the@emdash-cms/admin/localesbarrel, which evaluates the macro-heavyemails.tsmodule even thoughgetEmailLocaleonly needs locale resolution. Importing from@emdash-cms/admin/locales/config.jskeeps the dependency narrower. - New: the JSDoc for
getEmailLocalesays it loads the localized copy, but the function only resolves the locale string; callers load the copy. - New: when the site-wide
emdash:localeoption is set,getEmailLocalereturns it verbatim without canonicalizing the BCP 47 tag (unlikeresolveLocale, which usesIntl.Locale). A value like"pt-br"would miss the"pt-BR"catalog and silently fall back to English.
| * makes for its system mails. | ||
| */ | ||
|
|
||
| import { resolveLocale } from "@emdash-cms/admin/locales"; |
There was a problem hiding this comment.
[suggestion] Importing resolveLocale through the @emdash-cms/admin/locales barrel still evaluates the macro-heavy emails.ts module, even though this file only needs locale resolution. The barrel re-exports getInviteEmailStrings / getMagicLinkEmailStrings, which import @lingui/core/macro at module scope.
This was flagged in the previous review and remains unaddressed. Importing from the narrower subpath avoids loading the macro module for this code path:
| import { resolveLocale } from "@emdash-cms/admin/locales"; | |
| import { resolveLocale } from "@emdash-cms/admin/locales/config.js"; |
@emdash-cms/admin already exposes "./locales/*", so @emdash-cms/admin/locales/config.js resolves correctly.
| /** | ||
| * Resolve the locale for outbound system emails (invite, magic link, | ||
| * recovery), and load the matching localized copy from the admin | ||
| * catalogs (#915). |
There was a problem hiding this comment.
[suggestion] The JSDoc says this module "resolve[s] the locale ... and load the matching localized copy from the admin catalogs", but getEmailLocale only returns the locale string. The callers (invite/index.ts, magic-link/send.ts, send-recovery.ts) are the ones that call getInviteEmailStrings / getMagicLinkEmailStrings to load the copy. Update the doc to match the actual responsibility:
| /** | |
| * Resolve the locale for outbound system emails (invite, magic link, | |
| * recovery), and load the matching localized copy from the admin | |
| * catalogs (#915). | |
| /** | |
| * Resolve the locale for outbound system emails (invite, magic link, | |
| * recovery) (#915). | |
| * | |
| * Priority: the site-wide `emdash:locale` option (explicit site | |
| * language) -> the requesting user's admin locale (cookie / | |
| * Accept-Language, i.e. the language the inviter works in) -> English. | |
| * The recipient's language is unknowable server-side, so the site's | |
| * language is the best available signal. | |
| * | |
| * Callers load the matching localized copy from the admin catalogs. | |
| */ |
| // free-form option value degrades safely. | ||
| if (typeof siteLocale === "string" && siteLocale) return siteLocale; | ||
| return resolveLocale(request); | ||
| } |
There was a problem hiding this comment.
[suggestion] When emdash:locale is set, the value is returned verbatim. resolveLocale canonicalizes BCP 47 tags (e.g. "pt-br" -> "pt-BR") and only returns supported locales, but this code bypasses that path.
A lowercase or otherwise non-canonical site option (e.g. "pt-br") will miss the "./pt-BR/messages.mjs" catalog and silently fall back to English. Reusing the same canonicalization/normalization that resolveLocale applies would keep behavior consistent and avoid surprising locale mismatches.
| } | |
| const siteLocale = await options.get<string>("emdash:locale"); | |
| if (typeof siteLocale === "string" && siteLocale) { | |
| // Canonicalize the same way resolveLocale does so "pt-br" finds pt-BR. | |
| const canonical = new Intl.Locale(siteLocale.trim()).baseName; | |
| if (canonical) return canonical; | |
| } | |
| return resolveLocale(request); |
github-actions
Bot
added
review/awaiting-author
|
This PR has been inactive for 14 days. It will be closed automatically in 7 days if there is no further activity. If you're still working on this, please push an update or leave a comment. |
Overlapping PRsThis PR modifies files that are also changed by other open PRs: This may cause merge conflicts or duplicated work. A maintainer will coordinate. |
2 participants
What does this PR do?
System emails (user invites, magic-link sign-in, admin-initiated account recovery) were always sent in hardcoded English, bypassing the locale system entirely. On a site configured for German/Japanese/Arabic, invitees received English emails.
This PR routes the email copy through the admin's Lingui catalogs:
@emdash-cms/auth:buildInviteEmail()and the newbuildMagicLinkEmail()accept an optional strings object (InviteEmailStrings/MagicLinkEmailStrings) and fall back to the existing English copy — the auth package stays i18n-free and fully backwards compatible.@emdash-cms/admin: new server-side helpersgetInviteEmailStrings(locale, siteName)/getMagicLinkEmailStrings(locale, siteName)resolve the copy from the existing Lingui catalogs via module-scopemsgdescriptors (picked up bylocale:extracton merge).emdashcore: the invite, magic-link-send, and send-recovery routes resolve the email locale — site-wideemdash:localeoption first, then the requesting user's admin locale (cookie /Accept-Language), then English — and pass the localized strings through. Same trade-off WordPress makes: the recipient's language is unknowable, so the site's language is the best signal.Localized strings are HTML-escaped in the HTML email body like the site name already was.
Closes #915
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain.AI-generated code disclosure
Screenshots / test output
New tests:
packages/auth/src/email-templates.test.ts(builder defaults, injected copy, HTML escaping — 6 tests) andpackages/admin/tests/locales/emails.test.ts(catalog resolution, site-name interpolation, unknown-locale fallback — 4 tests). All pass locally alongside the existing invite/magic-link suites.