fix(i18n): localize invite, magic-link, and recovery emails - #1944
fix(i18n): localize invite, magic-link, and recovery emails#1944swissky wants to merge 5 commits into
Conversation
🦋 Changeset detectedLatest commit: e5019b9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 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-moderation
@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
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
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. |
bb84501 to
8a8cdd2
Compare
github-actions
Bot
added
review/needs-rereview
|
Rebased onto current main (the branch was ~500 commits behind; the
Also in this revision: the locale option is read in the same Full suites green locally (core 6447, auth 94, admin 2051). |
There was a problem hiding this comment.
This remains a sound fix for the right problem: system emails were hard-coded in English, and the PR keeps @emdash-cms/auth i18n-free while localizing copy in @emdash-cms/admin and choosing the locale in emdash core. The approach fits EmDash’s package split, the new public options are backwards-compatible, and the prior lang/dir and BCP 47 normalization concerns are addressed.
I re-read the diff, the full changed files, the cross-package export wiring, the tests, and the changeset. The only remaining item from the prior review is a minor import-coupling issue; everything else is either fixed or clean. No logic bugs, security issues, or AGENTS.md violations found. Two minor suggestions below — both non-blocking.
| * Accept-Language, i.e. the language the inviter works in) -> English. | ||
| */ | ||
|
|
||
| import { matchLocale, resolveLocale } from "@emdash-cms/admin/locales"; |
There was a problem hiding this comment.
[suggestion] This module only needs matchLocale and resolveLocale, but it imports them from the @emdash-cms/admin/locales barrel. That barrel also re-exports getInviteEmailStrings / getMagicLinkEmailStrings (which depend on the Lingui macro) and React-wrapped providers/hooks, so loading email-locale.ts pulls in unrelated runtime surface.
Because matchLocale and resolveLocale are already exported from ./locales/config.js, import from the narrower subpath instead. This was also flagged in the prior review and is still open.
| import { matchLocale, resolveLocale } from "@emdash-cms/admin/locales"; | |
| import { matchLocale, resolveLocale } from "@emdash-cms/admin/locales/config.js"; |
| /** | ||
| * Resolve the locale for outbound system emails (invite, magic link, | ||
| * recovery). Callers read the `emdash:locale` option (batched with | ||
| * their other options reads) and load the matching localized copy from | ||
| * the admin catalogs. | ||
| * | ||
| * 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. | ||
| */ |
There was a problem hiding this comment.
[suggestion] The JSDoc says callers read the option and load the matching localized copy, which reads as if this helper is responsible for both. It only resolves the locale string; callers load the catalog strings themselves. Clarify the doc so the boundary is obvious.
| /** | |
| * Resolve the locale for outbound system emails (invite, magic link, | |
| * recovery). Callers read the `emdash:locale` option (batched with | |
| * their other options reads) and load the matching localized copy from | |
| * the admin catalogs. | |
| * | |
| * 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. | |
| */ | |
| /** | |
| * Resolve the locale for outbound system emails (invite, magic link, | |
| * recovery). Callers use the returned locale to load localized copy from | |
| * the admin catalogs. | |
| * | |
| * 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 Smoke Tests failure on the previous push was real and is fixed in 6770bc5. Root cause, reproduced locally with the isolated-install cold-start test: the API routes' import of the Fix, following existing patterns in the repo:
The isolated-install suite now passes locally on the Cloudflare target (6/6). The remaining E2E 2/8 failure is the known |
|
Both suggestions are in: the narrower |
There was a problem hiding this comment.
This PR is a sound, carefully-scoped fix for the right problem: system emails (invite, magic-link, recovery) were hard-coded in English, and the change localizes them without making @emdash-cms/auth depend on i18n machinery.
I re-read the full diff and the files it touches, traced the cross-package wiring, and checked the tests and changeset against AGENTS.md.
- Approach: Keeping string resolution in
@emdash-cms/adminand passing final display strings into@emdash-cms/auth’s builders fits EmDash’s package split, preserves backwards compatibility, and keeps the auth package i18n-free. Resolving the locale fromemdash:locale→ requester locale → English matches the documented trade-off. - Correctness: The
resolveEmailLocalehelper canonicalizes BCP-47 tags and falls back cleanly; builders HTML-escape both the site name and localized strings; email HTML setslang/dirfor RTL; the invite accept page placeholder is now wrapped for translation. - Queries: The new
options.getMany(["emdash:site_title", "emdash:locale"])call batches the existing site-title read with the new locale read, so there is no extra database round-trip on the affected routes. - Subpath exports:
@emdash-cms/admin/locales/configand@emdash-cms/admin/locales/emailsare added as separate server-safe build entries, so core routes do not pull the React/Kumo graph into the server bundle. - Changeset: The new changeset is user-facing, names the affected capability and public API additions, and follows the .changeset/README.md guidance.
- Tests: Auth builder tests cover defaults, injected copy, escaping, and RTL lang/dir; admin tests cover catalog resolution and fallback; core locale tests cover priority/canonicalization; a route-level test covers the invite wiring.
The previously noted minor import-coupling concern (auth’s escapeHtml/localeDir shared from invite.ts into magic-link/index.ts) is still present, but it remains non-blocking for this bug fix and does not require refactoring here.
No new blocking issues, security problems, or AGENDS.md violations were found. This is clean work.
github-actions
Bot
added
review/approved
System emails were always sent in hardcoded English, bypassing the locale system. Route the copy through the admin Lingui catalogs: auth builders accept optional strings objects (English fallback), admin exposes getInviteEmailStrings/getMagicLinkEmailStrings, core resolves the email locale (site option > requesting user's admin locale > English) and threads lang/dir through for RTL.
Review follow-up: a non-canonical emdash:locale value (pt-br) now matches its catalog via the shared matchLocale helper; unsupported values fall through to the requester's locale. The locale option is read in the same getMany as the site title, so no route gains a query. resolveEmailLocale is a pure function; a route-level test covers the invite wiring (site locale -> email lang/dir). Changeset names the new public API surface of auth and admin.
The API routes' import of the admin locales barrel dragged the React/Kumo chunk into the Cloudflare dev runner, and @lingui/core was never pre-bundled for workerd, so the packed-install cold start died with 'exports is not defined'. Give locales/config and locales/emails their own server-safe build entries and subpath exports (mirroring ./slugify), import those from core, and pre-bundle @lingui/core like the other admin SSR deps.
d9488f6 to
ac7a845
Compare
|
Rebased onto current main (only conflict: the new |
github-actions
Bot
added
review/needs-rereview
|
The |
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.