feat(plugins): support translation-aware content creation - #3182
Conversation
🦋 Changeset detectedLatest commit: 50c8141 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 1,461 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. |
github-actions
Bot
added
review/needs-review
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
docs | 50c8141 | Sep 19 2026, 10:31 AM |
ascorbic
added
bot:review
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the expected next slice of the maintainer-provided sandboxed capability expansion (stacked on #3171), so the Discussion requirement is reasonably satisfied. The approach is sound and idiomatic for EmDash: a runtime-owned contentCreate callback wires the sandbox transports back to EmDashRuntime.handleContentCreate; the translation insert reads the source atomically in the same SQL statement; a partial unique index on (translation_group, lower(locale)) enforces one active row per locale; and hook re-entry is avoided with a combination of AsyncLocalStorage and the sandbox originHook marker.
I read the full diff, traced the runtime, Cloudflare, and workerd paths, checked the migration against dialect-helpers, and reviewed the tests, docs, and changeset. The implementation aligns with the PR description and the docs/changeset accurately reflect the behavior. Tests cover concurrency conflicts, missing sources, locale casing, custom configured locales, source mutation during hooks, save-hook re-entry, and cross-transport error codes.
The only concrete issue is a migration portability bug in the down path: up() correctly truncates the Postgres index name to 63 bytes before checking existence, but down() drops the untruncated name, so it silently misses the index on tables with long collection slugs and will fail tests or rollbacks that exercise it. There is no SQL-injection risk, no missing locale filter, no logged-out query-count regression, and no material doc/changeset problem.
github-actions
Bot
added
review/awaiting-author
|
Checked this before changing the migration. PostgreSQL truncates identifiers written in commands to I also drafted the suggested truncated-name helper and a long-slug rollback assertion. Independent adversarial review found that the assertion passes against the existing implementation too, so it does not reproduce a regression. I discarded that non-fix and left the current green head unchanged. PostgreSQL reference: https://www.postgresql.org/docs/current/sql-syntax-lexical.html#SQL-SYNTAX-IDENTIFIERS ~ 🤖 Codex |
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. |
@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-test
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-loader
@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: |
There was a problem hiding this comment.
This re-review confirms the prior migration-down issue is fixed and finds no new blockers. The PR is the expected next slice of the maintainer-provided sandboxed capability expansion, so the approved-Discussion requirement remains reasonably satisfied.
What changed since the last review:
packages/core/src/database/migrations/080_content_translation_locale_unique.tsnow truncates the Postgres index name to 63 bytes indown()before dropping it, matching theup()path. The newcontent-translation-index-name.test.tsassertions also cover this.
What I re-checked:
- Runtime translation creation (
EmDashRuntime.handleContentCreate) atomically resolves the source, enforces the one-active-locale-per-group rule with a pre-flight sibling check plus the unique index as a race guard, copies non-translatable fields afterbeforeSavehooks, and excludes the creating plugin’s owncontent:afterSave. ContentRepository.createrewrites the translation create as anINSERT ... SELECTagainst the source row, so shared-field values are read at insert time and a source that is trashed during hooks turns into aNOT_FOUND.- Cloudflare Worker Loader and Node/workerd transports both wire the runtime-owned
contentCreatecallback, forward theoriginHookmarker, and mapCONFLICT/NOT_FOUND/VALIDATION_ERROR/SAVE_REJECTEDback to plugin-visible errors. - The hook re-entry guard combines
AsyncLocalStoragefor native plugins with the sandboxoriginHookmarker for isolated hooks, while normal route-created content still runs save hooks. - The partial unique index on
(translation_group, lower(locale))is added both in migration 080 and inSchemaRegistry.createContentTable. - Docs (
capabilities.mdx,testing.mdx, SKILL files) and the changeset accurately describe the behavior.
No logic bugs, regressions, SQL-injection risks, missing locale filters, logged-out query-count regressions, or material documentation inaccuracies remain. Approve.
github-actions
Bot
added
review/approved
|
Review of Blocker: on Postgres the restore conflict mapping never matches, so 409 becomes 500function isTranslationLocaleConflict(error: unknown): boolean {
if (!(error instanceof Error)) return false;
const message = error.message.toLowerCase();
return (
message.includes("active_tg_locale") ||
(message.includes("unique constraint failed") &&
message.includes("translation_group") &&
message.includes("locale"))
);
}
Postgres raises That name gets truncated. The migration itself already accounts for this: const indexName = `uidx_${tableName}_active_tg_locale`;
const storedIndexName = isPostgres(db) ? indexName.slice(0, 63) : indexName;
Failure scenario: Postgres site, collection slug ≥ 39 chars. Trash the The branch's own Fix: match the name the migration actually computes instead of a suffix. const expected = `uidx_ec_${collection}_active_tg_locale`.slice(0, 63).toLowerCase();
// ... message.includes(expected) || (existing SQLite clause)Checked and clean: One non-blocking note, since this changes a shared host path: ~ 🤖 Codex |
Cross-PR blocker: three open PRs all add migration
|
| PR | migration file | sorts |
|---|---|---|
| #3182 | 080_content_translation_locale_unique.ts |
1st |
| #3184 | 080_redirect_write_guards.ts |
2nd |
| #3169 | 080_taxonomy_translation_locale_unique.ts |
3rd |
Individually each is fine. Together they can hard-break upgrades, because the runner builds a plain new Migrator({ provider: new StaticMigrationProvider() }) (packages/core/src/database/migrations/runner.ts:227-229) with no allowUnorderedMigrations, so Kysely enforces strict alphabetical ordering:
throw new Error(`corrupted migrations: expected previously executed migration ${executedMigrations[i]} to be at index ${i} but ${migrations[i].name} was found in its place. New migrations must always have a name that comes alphabetically after the last executed migration.`);kysely@0.29.2/dist/migration/migrator.js:503, #ensureMigrationsInOrder
Failure scenario: #3184 merges and ships, and a site upgrades and runs 080_redirect_write_guards. #3182 then merges and ships 080_content_translation_locale_unique, which sorts before the already-executed 080. On the next upgrade migrateToLatest() throws — and because migrations run from runtime init, that site can't boot into a migrated state at all. Recovery means hand-editing the migration table on production.
This only bites when the 080s land in different releases. If they all ship in one release a fresh run executes them in sorted order and nothing notices, which is exactly why this will slip through CI — each PR is green on its own, and main never sees the conflict until the second one merges.
Fix: keep 080 for whichever merges first and renumber the others to 081/082 (file name, the static import and the getMigrations() key in runner.ts) before they merge. If the intent is instead to allow out-of-order migrations, that's a deliberate migrator-level decision and needs its own change, not three coincidental 080s.
For this PR: 080_content_translation_locale_unique sorts first of the three, so it's the only one that is safe to merge at 080 without renumbering — but only if it merges before #3184 and #3169 ship. If either of those lands first, this one must be renumbered.
~ 🤖 Codex
|
Addressed both review points at
~ 🤖 Codex (gpt-5.6-sol) |
github-actions
Bot
added
review/needs-rereview
77d7ab5 to
a6d6a7b
Compare
a6d6a7b to
50c8141
Compare
There was a problem hiding this comment.
This is the expected next slice of the sandboxed capability expansion and the approach is sound: translation creates are routed through the runtime, source fields/attribution are resolved atomically by the repository, hook re-entry is guarded for both native and sandbox transports, and the unique partial index plus migration repair keep the invariant forward-compatible.
I re-read the full diff and checked the core runtime path, ContentRepository.create, migration 080 (including the fixed down() truncation), SchemaRegistry index creation, Cloudflare and workerd bridge/wrapper error envelopes and originHook wiring, AsyncLocalStorage hook-context plumbing, locale resolution changes, and the new test coverage.
No logic bugs, SQL-injection risks, missing locale filters, logged-out query-count regressions, or material documentation inaccuracies remain. The migration-down truncation fix is in place and covered by the updated index-name test.
I did find three small quality issues: a missing test for the documented "creating plugin's own afterSave is not re-entered" behavior, a dead __emdashOriginHook option in two test fixtures, and a new tautological string-in-template assertion in the plugin-cli tests. None block correctness, so the review is a comment rather than a request for changes.
| await expect( | ||
| runtimeHost.inspect.content.terms("posts", translated.id, "tags", "en"), | ||
| ).resolves.toEqual([expect.objectContaining({ slug: "news" })]); | ||
| await expect( | ||
| runtimeHost.transport.invokeRoute("content-translation-error", { |
There was a problem hiding this comment.
[needs fixing] The runtime-translation test covers shared fields, copied bylines, inherited taxonomy terms, and error codes, but it does not verify the documented behavior that the creating plugin's own content:afterSave hook is skipped for translation creates. The fixture's content:afterSave writes a storage event for every saved entry, so add an assertion that no events row exists for translated.id after any deferred hooks have had a chance to run.
| await expect( | |
| runtimeHost.inspect.content.terms("posts", translated.id, "tags", "en"), | |
| ).resolves.toEqual([expect.objectContaining({ slug: "news" })]); | |
| await expect( | |
| runtimeHost.transport.invokeRoute("content-translation-error", { | |
| ).resolves.toEqual([expect.objectContaining({ slug: "news" })]); | |
| await vi.waitFor(async () => { | |
| await expect( | |
| runtimeHost!.inspect.storage.get("events", translated.id), | |
| ).resolves.toBeNull(); | |
| }); | |
| await expect( | |
| runtimeHost.transport.invokeRoute("content-translation-error", { |
| const options = { | ||
| locale: route.input.locale, | ||
| translationOf: route.input.translationOf, | ||
| __emdashOriginHook: "content:beforeSave", |
There was a problem hiding this comment.
[suggestion] __emdashOriginHook inside the ctx.content.create options is dead code. The sandbox wrappers and bridge handlers only read a dedicated originHook field supplied by the wrapper itself from the hook name; nothing consumes an extra key inside options. Leaving it here suggests a hook-skip mechanism that does not exist and will confuse future maintainers.
| __emdashOriginHook: "content:beforeSave", | |
| const options = { | |
| locale: route.input.locale, | |
| translationOf: route.input.translationOf, | |
| }; |
| { | ||
| locale: route.input.locale, | ||
| translationOf: route.input.translationOf, | ||
| __emdashOriginHook: "content:beforeSave" |
There was a problem hiding this comment.
[suggestion] Same as in the plugin-test fixture: __emdashOriginHook inside ctx.content.create options is never read by the workerd wrapper or bridge. It should be removed to avoid implying a non-existent API.
| __emdashOriginHook: "content:beforeSave" | |
| { | |
| locale: route.input.locale, | |
| translationOf: route.input.translationOf, | |
| } |
| expect(skill).toContain("Node/workerd parity opt-in"); | ||
| expect(skill).toContain("schema:read"); | ||
| expect(skill).toContain("content:revisions:read"); | ||
| expect(skill).toContain("{ locale, translationOf }"); |
There was a problem hiding this comment.
[suggestion] This assertion only checks that the generated skill text contains the literal substring { locale, translationOf }. It cannot fail on a real functional regression — it only fails if the prose is intentionally removed. Per the repo's testing conventions, this is a config-pin / assert-the-diff test. Remove it and rely on the runtime tests that exercise ctx.content.create(..., { locale, translationOf }) instead.
| expect(skill).toContain("{ locale, translationOf }"); | |
| expect(skill).toContain("createPluginRuntimeTestHost()"); | |
| expect(skill).toContain("Node/workerd parity opt-in"); | |
| expect(skill).toContain("schema:read"); | |
| expect(skill).toContain("content:revisions:read"); | |
| expect(skill).toContain("@<publisher-handle>/<slug>"); |
github-actions
Bot
added
review/approved
1 participant
What does this PR do?
Adds translation-aware content creation for sandboxed plugins as the next slice stacked on #3171.
Plugins can pass
{ locale, translationOf }toctx.content.create(). EmDash validates the active source in the same collection, atomically joins its translation group, preserves non-translatable fields, byline credits, and taxonomy assignments, and runs the normal content validation and save-hook path across native, Cloudflare Worker Loader, and Node.js workerd execution.The database permits one active row per case-insensitive locale in a translation group. The forward-only migration repairs historical duplicates before adding the unique index, and stable
CONFLICT,NOT_FOUND,VALIDATION_ERROR, andSAVE_REJECTEDcodes cross both sandbox transports. Save-hook-originated creates do not re-enter save hooks.Closes: n/a
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
Not applicable. This PR has no admin UI changes.
Validated with:
pnpm buildpnpm typecheckpnpm format:checkpnpm lint:json(zero diagnostics)pnpm --dir docs build@emdash-cms/plugin-testruntime-host suite (15/15)One full workerd run encountered a transient local port collision; the affected wall-time test passed immediately in isolation, and the complete bridge suite plus the translation/restart real-workerd journey passed.