feat: bylines i18n support#1146
Conversation
🦋 Changeset detectedLatest commit: 662cb36 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,431 lines across 30 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. |
@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: |
|
/review |
There was a problem hiding this comment.
Nice work — the strict-per-locale credit hydration + chain-walk identity lookup distinction is well-reasoned and the test coverage (migration up/down, D1-like FK semantics, dedupe-after-resolve, copy-on-translation, no-fallback-when-explicit-exists) is thorough. The migration backfill, idempotency, and rollback guards are exactly the kind of belt-and-braces I'd want for a row-per-locale schema change on tables that already hold production data.
A few minor things noted inline — none are blockers:
- Extra per-entry query on every author-fallback path (
hydrateBylines): the newhasContentBylinesround-trip fires whenever the strict-localegetContentBylinesreturns empty. On monolingual installs (the majority case) this is a strict regression — one extra query per entry that has no bylines configured, for no functional benefit. Worth re-runningpnpm query-countsand either short-circuiting for monolingual sites or folding the "any junction exists" signal into the first query. - Inconsistent error response shape in the new translations route (raw
JSON.stringifyinstead ofapiError). Also a redundant source-not-found check that duplicates the handler's own check. BylineRepository.update()(unmodified in this PR but worth noting) doesn't validate slug or locale conflicts. An editor renaming the FR sibling ofjaneto a colliding(slug, locale)will fall through to the raw DB constraint and surface as a generic 500. The create path mirrors a careful CONFLICT response — would be nice to match in a follow-up.- Possible empty-state flash before the picker query loads (gated only on
bylines.length === 0, not on a loading flag).
Nothing structural. Once the query-count diff is reviewed, this looks ready.
| }, | ||
| }; | ||
| } | ||
| sourceGroup = source.translationGroup ?? source.id; |
There was a problem hiding this comment.
sourceGroup = source.translationGroup ?? source.id is correct given the migration backfills translation_group = id for every existing row, but it relies on that invariant holding indefinitely. The type translationGroup: string | null in BylineSummary and the comment in types.ts ("Nullable in storage for backwards compatibility") leave the door open.
If a row ever has translation_group = NULL, the findByTranslationGroup(source.id) lookup at the next line would only return the source row itself (real siblings would have a non-null group), and the (translation_group, locale) conflict check would silently miss.
Not something to act on now — but worth considering either (a) making the column NOT NULL in a follow-up migration once all installs are post-040, or (b) treating a null translationGroup as a data-integrity bug rather than a fallback case in code.
There was a problem hiding this comment.
Will skip this for now, should be addressed in a separate ticket/discussion
|
Review submitted successfully. The review was created with state "COMMENTED" and includes the summary body plus 4 inline comments anchored on:
Review URL: #1146 (review) |
|
@MohamedH1998 some valid review feedback there that's worth addressing |
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. |
@ascorbic i've addressed all the comments |
What does this PR do?
Adds first-class i18n support to bylines, mirroring the row-per-locale model already in place for content (migration 019), and menus + taxonomies (migration 036, PR #916). Each _emdash_bylines row carries locale and translation_group; siblings sharing a translation_group are the same byline identity in different languages. _emdash_content_bylines.byline_id is remapped to store the referenced byline's translation_group, so a single credit survives content translations and resolves against the active locale at runtime.
Highlights
Closes #
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