feat: i18n menus and taxonomies#916
Conversation
🦋 Changeset detectedLatest commit: 8c2f374 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 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 |
Scope checkThis PR changes 4,231 lines across 42 files. Large PRs are harder to review and more likely to be closed without review. If this scope is intentional, no action needed. A maintainer will review it. If not, please consider splitting this into smaller PRs. See CONTRIBUTING.md for contribution guidelines. |
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
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. |
|
/review |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/blocks
@emdash-cms/cloudflare
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@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-forms
@emdash-cms/plugin-webhook-notifier
commit: |
github-actions
Bot
added
the
query-count changed
Query-count snapshot changes4 routes changed, total Δ +8 queries. SQLite
D1
Comparing snapshot files between base and head. Updated automatically on each push. |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid migration design overall — the (name, locale) widening, translation_group remapping for content_taxonomies and _emdash_menu_items.reference_id, and the protective down() assertion on multi-locale installs are all good. Tests cover the migration paths well, the requestCached keys are properly scoped per-locale, and the new i18n/resolve.ts helper keeps fallback semantics consistent.
A few real issues, though, the most significant being:
The seed export/import claim doesn't actually work for menus and taxonomies
The PR description says:
pnpm export-seednow emits one entry per(name, locale)for menus, taxonomy defs and terms, with stable seed-local ids (menu:primary:en,tax:topics:en,term:topics:tech:en) andtranslationOfreferences between siblings
But SeedTaxonomy, SeedTaxonomyTerm, and SeedMenu in packages/core/src/seed/types.ts (not modified in this PR) have no id, locale, or translationOf fields — only SeedContentEntry does. As a consequence:
- Exporter (
packages/core/src/cli/commands/export-seed.ts:214-296) iterates every row from_emdash_taxonomy_defs/_emdash_menusand emits{ name, label, ... }with no locale tag. On a multi-locale site this produces multipleSeedTaxonomyentries with the samenameand no way to distinguish them. - Importer (
apply.ts:227-228and:478-482) checks existence bynameonly. The second per-locale row is either skipped (onConflict: skip) or overwrites the first (onConflict: update). The:estranslation never gets created. - Same problem for
SeedTaxonomyTerm(slug-onlyfindBySlugon import) andSeedMenuitems (no locale routing).
Net effect: exporting a multi-locale site and re-applying the seed silently loses every non-default-locale menu and taxonomy. There is no test exercising the multi-locale seed round-trip — tests/unit/seed/apply.test.ts doesn't reference locale or translationOf for these tables, only for content (which was wired up correctly in a prior PR).
This is what the open review comment from @MA2153 is asking about. Either:
- (a) Add
id/locale/translationOfto the seed types, update the exporter to emit them, and updateapply.tsto thread them through (the existingTaxonomyRepository.createandhandleMenuCreatepaths already accept those fields). Add a regression test. - (b) Drop the multi-locale seed claim from the PR description and changeset, and file a follow-up issue.
Other findings inline below.
| label_singular: taxonomy.labelSingular ?? null, | ||
| hierarchical: taxonomy.hierarchical ? 1 : 0, | ||
| collections: JSON.stringify(taxonomy.collections), | ||
| translation_group: defId, |
There was a problem hiding this comment.
Locale-unaware existence check above (apply.ts:227-228 — where("name", "=", taxonomy.name)). _emdash_taxonomy_defs is now UNIQUE(name, locale), so two seed entries with the same name but different locales (the export-seed claim from the PR description) would both match this single row and one would be skipped/overwritten. Should filter by locale once SeedTaxonomy has a locale field — see the broader summary comment.
| label: menu.label, | ||
| created_at: new Date().toISOString(), | ||
| updated_at: new Date().toISOString(), | ||
| translation_group: menuId, |
There was a problem hiding this comment.
Same locale-unaware existence check above (apply.ts:478-482) — _emdash_menus is now UNIQUE(name, locale) and the seed type lacks a locale field, so multi-locale menus can't round-trip through this path. Also note applyMenuItems (line 855) sets translation_group: itemId on every newly-inserted item with no concept of "this item is the ES translation of menu_item X", so even with locale support added on the menu level, items would diverge across translations.
| options: TaxonomyQueryOptions = {}, | ||
| ): Promise<TaxonomyDef | null> { | ||
| const chain = resolveLocaleChain(options.locale); | ||
| const allDefs = peekRequestCache<TaxonomyDef[]>("taxonomy-defs:all"); |
There was a problem hiding this comment.
Stale cache key — this peek will never hit. On main both this peekRequestCache call and the requestCached in getTaxonomyDefs used the literal string "taxonomy-defs:all". After this PR, getTaxonomyDefs writes under taxonomy-defs:${locale ?? "*"} (line 39), but this lookup still uses the old "taxonomy-defs:all" constant.
Result: the in-memory shortcut never fires, and every page that calls getTaxonomyDef on a hot site does an extra _emdash_taxonomy_defs query that the optimization was specifically there to avoid. Silent (correctness is unaffected), but it's a real per-request perf regression on every multi-call page.
| const allDefs = peekRequestCache<TaxonomyDef[]>("taxonomy-defs:all"); | |
| const chain = resolveLocaleChain(options.locale); | |
| const peekKey = `taxonomy-defs:${resolveLocale(options.locale) ?? "*"}`; | |
| const allDefs = peekRequestCache<TaxonomyDef[]>(peekKey); |
| * | ||
| * Returns null if the referenced content no longer exists (item should be skipped) | ||
| * Resolve a single menu item's URL. `reference_id` is a translation_group | ||
| * (migration 035 remapped all existing references); we join it against |
There was a problem hiding this comment.
Doc nit: comment says "migration 035" but the actual migration introduced in this PR is 036_i18n_menus_and_taxonomies (035_bounded_404_log is unrelated). Same off-by-one in the legacy-fallback comment further down.
| * (migration 035 remapped all existing references); we join it against | |
| * (migration 036 remapped all existing references); we join it against |
| } | ||
| if (!row) { | ||
| // Legacy rows whose reference_id still points at an id directly | ||
| // (defensive — migration 035 normalised these, but a row inserted |
There was a problem hiding this comment.
Same nit — should reference migration 036, not 035.
| // (defensive — migration 035 normalised these, but a row inserted | |
| // (defensive — migration 036 normalised these, but a row inserted |
| const name = params.name!; | ||
|
|
||
| const denied = requirePerm(user, "menus:read"); | ||
| if (denied) return denied; |
There was a problem hiding this comment.
Missing requireDb(emdash?.db) check — the term-translations route added in this PR has it (taxonomies/[name]/terms/[slug]/translations.ts:37-38), but this one goes straight from the permission check to using emdash.db. If the runtime isn't initialised the route will crash with an unhandled exception instead of returning a clean 500. Same issue in the POST handler below.
| if (denied) return denied; | |
| const dbErr = requireDb(emdash?.db); | |
| if (dbErr) return dbErr; | |
| const denied = requirePerm(user, "menus:read"); | |
| if (denied) return denied; |
| for (const table of tables) { | ||
| validateSystemIdent(table); | ||
| const result = await sql<{ count: number | string }>` | ||
| SELECT COUNT(*) AS count FROM ${sql.ref(table)} WHERE locale != 'en' |
There was a problem hiding this comment.
Hardcoding 'en' as the "safe to drop" locale doesn't match the rest of the system: the user's defaultLocale may be e.g. 'es' or 'de'. A fresh single-locale install in the user's chosen locale would have all rows backfilled to 'en' by up() — fine — but any rows inserted post-migration in the user's actual default locale would be flagged here as "non-default" and block rollback.
In practice this only bites users who reconfigured defaultLocale away from 'en' and have a single-locale install. Probably acceptable — the error message tells them what to do — but worth either documenting or comparing against the column DEFAULT introspectively rather than the literal 'en'.
There was a problem hiding this comment.
Me. I have a non-English default locale. This is kind of a big deal if true. The review seem to downplay it...
There was a problem hiding this comment.
Migration now resolves the locale via getI18nConfig()?.defaultLocale ?? "en" and threads it through every backfill, column DEFAULT, and the rollback guard. Added 3 regression tests with defaultLocale: "es".
| UPDATE _emdash_menu_items SET reference_id = ( | ||
| SELECT translation_group FROM ${ec} WHERE ${ec}.id = _emdash_menu_items.reference_id | ||
| ) | ||
| WHERE reference_collection = ${slug} AND reference_id IS NOT NULL |
There was a problem hiding this comment.
The remap only fires when reference_collection = ${slug}, but historically menu items of type='page' / type='post' may have reference_collection IS NULL (the runtime falls back to ${type}s in menus/index.ts:192). For those rows reference_id keeps pointing at the original row id rather than the translation_group, and runtime resolution then falls into the defensive legacy branch (menus/index.ts:284-289).
Not a correctness bug — the legacy lookup catches it — but on a multi-locale site those menu items will resolve to the row with that exact id regardless of active locale, instead of following the translation_group to the locale-specific copy. Worth either also matching reference_collection IS NULL AND type IN ('page','post') here, or leaving a comment explaining the runtime fallback is the intended escape hatch.
| target: item.target, | ||
| css_classes: item.css_classes, | ||
| ...(input.locale !== undefined ? { locale: input.locale } : {}), | ||
| translation_group: item.translation_group ?? item.id, |
There was a problem hiding this comment.
item.translation_group ?? item.id copies the source item's translation_group onto the new translation's items. That's actually a reasonable choice (cloned items in different locales then identify as "the same nav entry across translations") — but the comment two lines up says the opposite: "The items' own locale/translation_group get fresh values tied to the new menu."
One of the two needs updating. If source-shared groups are intended, the comment should say so. If fresh per-clone groups are intended, this should be translation_group: newId (or ulid()). Worth a test pinning down whichever you decide — at the moment a downstream caller can't tell from the docs which behaviour to rely on.
|
Review submitted successfully. The review covers:
|
|
|
||
| await sql` | ||
| INSERT INTO _emdash_menus_new (id, name, label, created_at, updated_at, locale, translation_group) | ||
| SELECT id, name, label, created_at, updated_at, 'en', id FROM _emdash_menus |
| .addColumn("label", "text", (c) => c.notNull()) | ||
| .addColumn("parent_id", "text") | ||
| .addColumn("data", "text") | ||
| .addColumn("locale", "text", (c) => c.notNull().defaultTo("en")) |
| .addColumn("hierarchical", "integer", (c) => c.defaultTo(0)) | ||
| .addColumn("collections", "text") | ||
| .addColumn("created_at", "text", (c) => c.defaultTo(currentTimestamp(db))) | ||
| .addColumn("locale", "text", (c) => c.notNull().defaultTo("en")) |
| await sql` | ||
| INSERT INTO _emdash_taxonomy_defs_new | ||
| (id, name, label, label_singular, hierarchical, collections, created_at, locale, translation_group) | ||
| SELECT id, name, label, label_singular, hierarchical, collections, created_at, 'en', id |
| ): Promise<void> { | ||
| validateSystemIdent(table); | ||
| const ref = sql.ref(table); | ||
| await sql`ALTER TABLE ${ref} ADD COLUMN IF NOT EXISTS locale TEXT NOT NULL DEFAULT 'en'`.execute( |
| await sql` | ||
| INSERT OR IGNORE INTO content_taxonomies_new (collection, entry_id, taxonomy_id) | ||
| SELECT ct.collection, ct.entry_id, COALESCE( | ||
| (SELECT t.id FROM taxonomies t WHERE t.translation_group = ct.taxonomy_id AND t.locale = 'en'), |
…egression tests for non-default locales
…e-aware references
…n for pending tasks
|
/review |
|
Pushed updates addressing the review. Tests pass locally.
|
There was a problem hiding this comment.
Took another pass after the i18n-config rework. The defaultLocale plumbing in the migration (and the new getDefaultLocale() helper threaded through column DEFAULTs and the rollback assertion) addresses the main concern from the previous reviews — nice job. The seed types now carry id / locale / translationOf for menus and taxonomies, the exporter emits per-locale entries with anchor-first ordering, and apply.ts resolves them through scoped maps. Cache key for taxonomy-defs is rekeyed correctly, both translations routes guard requireDb, comments reference migration 036, and the menu-items clone comment now matches the code.
One real bug left, plus two minor follow-ups inline.
| } | ||
|
|
||
| // Duplicate guard: same (name, locale). Default 'en' matches the column DEFAULT. | ||
| const effectiveLocale = input.locale ?? "en"; |
There was a problem hiding this comment.
Hardcoded "en" here doesn't match the rest of the system. The migration's column DEFAULT now uses getI18nConfig()?.defaultLocale ?? "en", so on an install with defaultLocale: "es" (the case the previous review thread specifically covered) an existing menu row inserted without locale lives at locale = 'es'. This duplicate guard searches locale = 'en', finds nothing, and falls through to the insert. The DB UNIQUE constraint then fires and gets caught by the generic catch at the bottom of the function — so the user sees MENU_CREATE_ERROR ("Failed to create menu") instead of the clean CONFLICT with the helpful message a few lines down.
Not catastrophic (data isn't corrupted), but it's exactly the 'en'-assumption pattern the rest of the PR moved away from. Suggest threading the same helper through:
| const effectiveLocale = input.locale ?? "en"; | |
| const effectiveLocale = input.locale ?? (getI18nConfig()?.defaultLocale ?? "en"); |
| css_classes: item.cssClasses ?? null, | ||
| created_at: new Date().toISOString(), | ||
| locale, | ||
| translation_group: itemId, |
There was a problem hiding this comment.
Heads-up / follow-up: every imported menu item gets a fresh translation_group = itemId, so when an exported multi-locale site is re-applied the per-locale menu items no longer share a translation_group across locales (each item identifies as its own group). Runtime resolution still works because reference_id stores the content's translation_group, but the cross-locale "this is the same nav entry" link is lost on the round-trip.
SeedMenuItem has no id / translationOf fields today, so there's no way to express item-level translation links in the seed format yet. Worth either calling out explicitly in the PR description ("menu items don't round-trip their translation_group, only the menu does") or filing a follow-up issue — the PR description currently implies items round-trip too.
| UPDATE _emdash_menu_items SET reference_id = ( | ||
| SELECT translation_group FROM ${ec} WHERE ${ec}.id = _emdash_menu_items.reference_id | ||
| ) | ||
| WHERE reference_collection = ${slug} AND reference_id IS NOT NULL |
There was a problem hiding this comment.
Minor robustness nit, not blocking: the remap predicate is WHERE reference_collection = ${slug}, but legacy _emdash_menu_items rows of type='page' / type='post' may have reference_collection IS NULL (the runtime falls back to ${type}s in menus/index.ts:198). For those rows reference_id keeps pointing at a row id rather than the translation_group, and resolveContentUrl then hits the defensive legacy branch (lines 291–299).
Functionally fine — the legacy branch catches it — but on a multi-locale site those items will lock to the row with that exact id regardless of active locale instead of following the translation_group. Worth either widening the predicate to also match (reference_collection IS NULL AND type IN ('page','post')) here, or adding a comment pointing to the runtime fallback as the intended escape hatch.
|
Review submitted. Summary of what I posted:
|
|
Pushed
Full CI is green, and I also ran the suite end-to-end in a Playwright Docker image locally: 3135 unit + 851 browser + 234/245 e2e. The two remaining e2e fails ( Thanks for the thorough reviews! |
3 participants
What does this PR do?
Adds i18n support to menus and taxonomies (definitions and terms), mirroring the per-locale row model already in place for content (migration 019). Each row carries
localeandtranslation_group; translations of the same menu/term/def share atranslation_group._emdash_menu_items.reference_idandcontent_taxonomies.taxonomy_idare remapped to store the referenced row'stranslation_group, so a single association survives content translations and is resolved against the active locale at runtime.Highlights:
036_i18n_menus_and_taxonomies(SQLite + Postgres paths). Idempotent column checks, validated identifiers, FK-aware data remap oncontent_taxonomies.down()refuses to run on multi-locale installs (would silently drop rows otherwise) and names the offending table in the error.getMenu,getTaxonomyTerms,getTerm,getEntryTerms,getAllTermsForEntries,getTaxonomyDef) accept an optional{ locale }and honour the i18n fallback chain via the new shared helperi18n/resolve.ts(resolveLocale/resolveLocaleChain). Single-locale sites keep their old query count — the chain walk is a 1-element loop when i18n is disabled.?locale=xx; POST acceptslocaleandtranslationOfin the body. Two new endpoints:GET/POST /_emdash/api/menus/:name/translationsGET/POST /_emdash/api/taxonomies/:name/terms/:slug/translationstaxonomy_list,taxonomy_list_terms,taxonomy_create_term,menu_list,menu_getacceptlocale. Two new tools:taxonomy_term_translations,menu_translations.TaxonomyManagerandMenuListsurface the existingLocaleSwitcherwhen multiple locales are configured. New shared<TranslationsPanel>component reused byContentEditor,MenuEditorandTaxonomyManagerso the affordance is visually consistent.MenuEditorreads?locale=from the route'svalidateSearch, so navigation between locales is bookmarkable.pnpm export-seednow emits one entry per(name, locale)for menus, taxonomy defs and terms, with stable seed-local ids (menu:primary:en,tax:topics:en,term:topics:tech:en) andtranslationOfreferences between siblings, anchors emitted first so the import can resolve them in order.seed/apply.tsconsumes those fields, threadinglocaleandtranslationOfto the existinghandleMenuCreate/TaxonomyRepository.createpaths (which already supported them after this PR's earlier commits).Type of change
Checklist
pnpm typecheckpassespnpm lintpasses (21 diagnostics, all pre-existing onmain; this PR introduces zero new ones)pnpm testpasses (193 test files, 3119 tests, all green; includes the new migration tests and the seed regression test)pnpm formathas been runmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain.AI-generated code disclosure
Mostly used for code analysis (data-flow tracing, bug hunting) and partial code generation, all reviewed before commit
Screenshots / test output
List of menus with the translation component
Menu details in two languages
List of categories with a translation option
Manual test (demo,
defaultLocale: "en",locales: ["en", "es"]):/menus/primary?locale=en→ click "Translate" on the ES row in the right-handTranslationsPanel./menus/primary?locale=es— items are cloned, can be renamed independently.TranslationsPanelnow shows "Edit" on the EN row (not "Translate"), confirming both rows share the sametranslation_group.Note: I reorganized the changes using a new fork