Repository navigation
Conversation
Implements the contentMapping proposal from discussion emdash-cms#1672: an optional per-form setting that creates a draft entry in a target collection from each successful submission. Mappings are validated against the target collection at form-save time (including required field coverage), and the inbox submission is preserved even when content creation fails. Adds getCollection and REST-parity slug generation to the plugin content API in core to support this. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 75835fb The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
All contributors have signed the CLA ✍️ ✅ |
Scope checkThis PR changes 1,061 lines across 14 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. |
|
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
This PR implements the approved discussion well: it adds an optional contentMapping form setting, keeps submissions in the inbox, creates draft-only entries, validates mappings at form-save time, and preserves submissions when content creation fails. The core additions (ctx.content.getCollection and slug generation in ctx.content.create) are additive and mirror the REST path. I read the diff, the changed files, ContentRepository, the REST handleContentCreate, and the forms validation code, and checked against AGENTS.md.
The implementation is generally solid and well-tested, but I found a few gaps worth fixing before merge:
- AGENTS.md violation: module-scope mutable singleton.
content-mapping.tskeeps alet keyCounter = 0counter and mutates it fromgenerateKey().AGENTS.mdrequires module-scope singletons to live onglobalThisbecause Vite can duplicate modules across SSR chunks. Use aSymbol.forkey onglobalThis. - Metadata constant validation is incomplete.
validateContentMappingonly checks that a metadata key is a real collection field. Anull/undefinedconstant for a required target field will pass validation but fail every submit-time insert against theNOT NULLcolumn. The validator should reject required fields covered by nullish metadata. - Optional form fields can satisfy required collection fields. A required collection field mapped from an optional form field (or a transform that can return
undefined) may produce an entry with a default/empty value, or, when combined with a failing transform, a logged create failure. Requiring the source form field to berequiredwhen the target collection field is required would close the loophole. - Form duplication skips validation.
formsDuplicateHandlerspreads the existing definition (includingcontentMapping) into a new form without re-runningvalidateContentMapping. If the target collection or fields changed since the original was saved, the duplicate’s mapping is invalid from the start. - Plugin slug generation ignores site locale.
ctx.content.createcallsgenerateUniqueSlug(...)without alocale, while the REST create path scopes uniqueness to the effective locale and creates entries in that locale. Plugin-created entries therefore default toenand use global slug uniqueness, which will behave differently on non-English default sites.
None of these are merge blockers on their own, but (1) is a convention violation and (2)/(3) weaken the save-time validation that is a headline feature of this PR.
Findings
-
[needs fixing]
packages/plugins/forms/src/content-mapping.ts:183-189Module-scope mutable state (
let keyCounter = 0) is used for Portable Text_keygeneration.AGENTS.mdsays module-scope singletons must live onglobalThisbecause Vite can duplicate modules across SSR chunks, so two copies of this module could share/reset the counter and produce duplicate keys.const KEY_COUNTER = Symbol.for("emdash-forms.content-mapping.key-counter"); function nextKeyCounter(): number { const current = ((globalThis as Record<symbol, number>)[KEY_COUNTER] ?? 0) + 1; (globalThis as Record<symbol, number>)[KEY_COUNTER] = current; return current; } /** Generate a Portable Text `_key`, unique within this process */ function generateKey(): string { return `form-${nextKeyCounter().toString(36)}-${Math.random().toString(36).slice(2, 7)}`; } -
[needs fixing]
packages/plugins/forms/src/content-mapping.ts:69-76The metadata key check verifies the key exists as a collection field, but it does not verify the constant value. A
nullorundefinedmetadata constant for a required target field will pass validation and then fail every submit-time insert on theNOT NULLcolumn. Reject required fields covered by nullish metadata constants at save time.for (const key of metadataKeys) { const collectionField = collectionFields.get(key); if (!collectionField) { throw PluginRouteError.badRequest( `Content mapping metadata targets unknown field "${key}" in collection "${mapping.collection}"`, ); } const value = mapping.metadata?.[key]; if (collectionField.required && (value === undefined || value === null)) { throw PluginRouteError.badRequest( `Content mapping metadata for required field "${key}" in collection "${mapping.collection}" must not be null or undefined`, ); } } -
[suggestion]
packages/plugins/forms/src/content-mapping.ts:46-82The required-field coverage check guarantees that a required collection field is referenced by a mapping or metadata, but not that a field mapping will actually supply a value. If an optional form field is mapped to a required collection field and the submitter leaves it empty, the entry will receive the column default (empty string/
0/etc.) or a transform will skip it and a logged create failure occurs. Consider requiring the source form field to berequiredwhen it backs a required target field that isn’t covered by metadata.const requiredFormFields = new Set( pages.flatMap((page) => page.fields.filter((field) => field.required).map((field) => field.name)), ); const mappedTargets = new Set<string>(); const targetsFromOptionalField = new Set<string>(); for (const [formField, target] of Object.entries(mapping.fieldMappings)) { if (!formFieldNames.has(formField)) { throw PluginRouteError.badRequest( `Content mapping references unknown form field "${formField}"`, ); } const targetField = typeof target === "string" ? target : target.field; const collectionField = collectionFields.get(targetField); if (!collectionField) { throw PluginRouteError.badRequest( `Content mapping targets unknown field "${targetField}" in collection "${mapping.collection}"`, ); } if (collectionField.required && !requiredFormFields.has(formField)) { targetsFromOptionalField.add(targetField); } mappedTargets.add(targetField); } // ... for (const field of collection.fields) { if (field.required && !mappedTargets.has(field.slug) && !metadataKeys.has(field.slug)) { throw PluginRouteError.badRequest( `Content mapping does not map required field "${field.slug}" in collection "${mapping.collection}"`, ); } if (field.required && targetsFromOptionalField.has(field.slug) && !metadataKeys.has(field.slug)) { throw PluginRouteError.badRequest( `Content mapping maps required field "${field.slug}" in collection "${mapping.collection}" from an optional form field; mark the source form field as required or cover it with metadata`, ); } } -
[suggestion]
packages/plugins/forms/src/handlers/forms.ts:203-235formsDuplicateHandlercreates a brand-new form by spreading the existing definition, includingcontentMapping, but never re-runsvalidateContentMapping. If the target collection or its fields changed since the original form was saved, the duplicated form will contain an invalid mapping from the moment it is written. Add a validation call beforeforms(ctx).put(id, duplicate)so duplication behaves like the other save paths. -
[suggestion]
packages/core/src/plugins/context.ts:441Plugin
ctx.content.createderives a slug without passing alocale, whilehandleContentCreatescopes uniqueness to the effective locale and inserts the entry in that locale. This means plugin-created entries default toenand use global slug uniqueness, which diverges from the REST/admin path on non-English sites. Consider threadingctx.site.locale(or adding locale support toContentWriteInput) so the plugin API matches the native create behavior.
- Move the Portable Text _key counter to a globalThis singleton - Reject nullish metadata constants for required collection fields - Require required collection fields to be mapped from required form fields (or covered by metadata) - Re-validate the mapping when duplicating a form - Thread the site default locale through plugin content.create so the entry locale and slug uniqueness match the REST create path Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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. |
There was a problem hiding this comment.
This re-review confirms the PR is in good shape. The author has addressed all five findings from the previous review:
- Module-scope key counter is now stored on
globalThisviaSymbol.for(packages/plugins/forms/src/content-mapping.ts). - Metadata constants for required fields now reject
null/undefinedat save time (packages/plugins/forms/src/content-mapping.ts). - Required collection fields mapped from optional form fields are rejected (
targetsWithRequiredSourcecheck inpackages/plugins/forms/src/content-mapping.ts). - Form duplication now re-runs
validateContentMappingbefore saving the duplicate (packages/plugins/forms/src/handlers/forms.ts). - Plugin slug generation now uses the site's configured default locale and locale-scoped uniqueness, matching the REST create path (
packages/core/src/plugins/context.ts).
The feature continues to follow the approved discussion: mappings are validated on form save, entries are always created as drafts, the inbox submission is preserved even if content creation fails, and the forms plugin correctly declares content:write. The core additions (ctx.content.getCollection and slug generation in ctx.content.create) are additive and mirror the REST path. Tests cover validation, duplicate revalidation, submit-time failures, transforms, and locale-scoped slug behavior. There are no admin UI changes, so Lingui/RTL concerns do not apply; changesets are present for both affected packages.
No remaining blockers or regressions were found.
|
locally smoke-tested and ready, pending GitHub checks |
masonjames
left a comment
There was a problem hiding this comment.
Thanks for adapting this to the current plugin APIs. I reviewed 4b389c2 and reran the promised checks in an isolated checkout: the root build, package typecheck, quick/type-aware lint (0 diagnostics), and formatting of the eight changed files pass. All 77 forms tests and 115 existing core capability/locale/deferred-work tests pass. The prior synchronous-create and comment findings are addressed.
I reproduced two additional save-time validation gaps, detailed inline. Three temporary regression checks use real SQLite and EmDashRuntime.handleContentCreate: each mapping is accepted on save, but its resulting entry is rejected with VALIDATION_ERROR. The inbox remains protected; the missing draft is the problem. These would be useful regression tests alongside the metadata/field collision already identified by the latest bot review.
Hosted validation is still pending: the CI run for this commit is action_required with zero jobs. A maintainer needs to approve the workflows before merge; ci:rerun cannot clear that approval gate. Local results do not replace those checks.
I removed my old manually applied review/ready-for-maintainer label from July while these items are outstanding. The workflow-managed review, area, size, overlap, and CLA labels can remain.
Review assisted by Codex; the reproductions and checks above were run against the stated commit.
| if (source.required) { | ||
| targetsWithRequiredSource.add(targetField); |
There was a problem hiding this comment.
[P2] Required source fields can still be absent from the mapped data. This counts every source.required field as guaranteed coverage, but validateSubmission skips a required field when its condition is false and excludes file fields from result.data even when an upload satisfies their required check. buildContentEntry(mapping, result.data) consequently omits the target.
For example, mapping a conditionally displayed required text field to required collection title saves successfully; submitting the branch where that field is hidden is a valid submission, but the deferred content create fails with VALIDATION_ERROR for the missing title. File sources have the same omission. I reproduced both through real SQLite/core content validation.
Count only sources that are guaranteed to appear in the mapping input toward required-target coverage. A conditional source needs an unconditional source or valid metadata fallback; file sources should be rejected until their uploaded values are explicitly supported. Please add regression coverage for both cases.
There was a problem hiding this comment.
Fixed in 17ee6ba. Only required form fields without a condition count toward required-field coverage, and file fields cannot be mapped at all. Both have regression tests.
| `Content mapping metadata targets unknown field "${key}" in collection "${mapping.collection}"`, | ||
| ); | ||
| } | ||
| if (collectionField.required && (metadata[key] === undefined || metadata[key] === null)) { |
There was a problem hiding this comment.
[P2] Empty metadata can satisfy a required field without supplying a valid value. Only null/undefined are rejected here. With a required text title, { fieldMappings: { details: "body" }, metadata: { title: "" } } passes save-time validation, and the later required-field loop treats title as covered. Every submission then builds an empty title and the real runtime rejects the draft with VALIDATION_ERROR.
I reproduced this against SQLite and EmDashRuntime.handleContentCreate. Reject empty string constants for required string fields, consistent with core content validation, and add a save-time regression test. This is separate from the metadata/field collision flagged by the bot.
There was a problem hiding this comment.
Fixed in 17ee6ba. Empty metadata for a required field is rejected on save. This also rejects whitespace-only strings, which core accepts, since a blank title is never useful.
There was a problem hiding this comment.
This is still a sensible, well-scoped implementation of the approved Discussion. I verified the two earlier review items are resolved: the content-entry create now runs in after() so it does not block the anonymous submission response, and the new step comment is unnumbered. The contentMapping schema, save-time validation, merge logic, and submit-time deferred create all fit the plugin patterns.
However, the one remaining issue from the previous review is still outstanding: validateContentMapping does not reject a metadata key that collides with a fieldMappings target. Because buildContentEntry spreads metadata first and then overwrites colliding keys with any non-empty mapped form value, the changeset's claim that metadata sets "fixed values on every entry" is not reliable in practice.
Please add the collision check to validateContentMapping and a regression test that asserts the collision is rejected. Once that is fixed, this PR should be clean to merge.
| for (const key of metadataKeys) { | ||
| const collectionField = collectionFields.get(key); | ||
| if (!collectionField) { | ||
| throw PluginRouteError.badRequest( | ||
| `Content mapping metadata targets unknown field "${key}" in collection "${mapping.collection}"`, | ||
| ); | ||
| } | ||
| if (collectionField.required && (metadata[key] === undefined || metadata[key] === null)) { | ||
| throw PluginRouteError.badRequest( | ||
| `Content mapping metadata for required field "${key}" in collection "${mapping.collection}" must not be null`, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
[needs fixing] The metadata key loop validates that each key exists in the target collection, but it never checks whether the same key is also a fieldMappings target. buildContentEntry spreads mapping.metadata first and then overwrites any colliding key with a transformed form value whenever the mapped field is non-empty, so metadata is a fallback rather than the "fixed value on every entry" documented in the PR description and changeset.
Reject the collision at save time so the stored mapping is unambiguous and the changeset stays accurate:
| for (const key of metadataKeys) { | |
| const collectionField = collectionFields.get(key); | |
| if (!collectionField) { | |
| throw PluginRouteError.badRequest( | |
| `Content mapping metadata targets unknown field "${key}" in collection "${mapping.collection}"`, | |
| ); | |
| } | |
| if (collectionField.required && (metadata[key] === undefined || metadata[key] === null)) { | |
| throw PluginRouteError.badRequest( | |
| `Content mapping metadata for required field "${key}" in collection "${mapping.collection}" must not be null`, | |
| ); | |
| } | |
| } | |
| for (const key of metadataKeys) { | |
| if (mappedTargets.has(key)) { | |
| throw PluginRouteError.badRequest( | |
| `Content mapping metadata field "${key}" in collection "${mapping.collection}" is also mapped from a form field`, | |
| ); | |
| } | |
| const collectionField = collectionFields.get(key); | |
| if (!collectionField) { | |
| throw PluginRouteError.badRequest( | |
| `Content mapping metadata targets unknown field "${key}" in collection "${mapping.collection}"`, | |
| ); | |
| } | |
| if (collectionField.required && (metadata[key] === undefined || metadata[key] === null)) { | |
| throw PluginRouteError.badRequest( | |
| `Content mapping metadata for required field "${key}" in collection "${mapping.collection}" must not be null`, | |
| ); | |
| } | |
| } |
Add a matching regression test in packages/plugins/forms/tests/content-mapping.test.ts that asserts this collision is rejected by the save handlers.
A mapping is now rejected when a metadata key is also a mapping target, when a required field's metadata value is empty, when a required field is covered only by a conditional form field, when a file field is mapped, or when a form field, transform or metadata value cannot produce a value the target field accepts. Each of these saved before but failed at submit time with VALIDATION_ERROR. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
@masonjames thanks for the reproductions.
Each case has a regression test; 91 forms tests pass, with the root build, typecheck and lint clean. Two gaps remain that I could not close from the form side: a number form field onto an integer collection field passes but a decimal entry fails at submit, since forms have no integer field type, and a metadata string for a url field is not URL-validated. Happy to tighten either if you prefer. The workflow approval for the CI run is on your side. |
|
This PR is waiting on its author because it has merge conflicts with |
masonjames
left a comment
There was a problem hiding this comment.
Reviewed the feature against main 0eefcfebc and published the reviewed update branch at 39c9d4832, preserving the contributor's commits. The branch includes main 0eefcfebc (freshly fetched before publication), resolves both handler conflicts, preserves main's digest and notification fixes, and closes mapping/input validation gaps. Required blank checkbox selections are rejected, optional blank selections no longer become numeric zero, and the minimum supported core version is corrected to 0.42.0 for compatible URL validation. Documentation now describes the existing admin editor limitation for multi-page or titled forms.
Verification on the prepared revision: root build and package typecheck pass; formatting completed; quick and type-aware lint report zero diagnostics; 190 forms tests and 124 core/runtime/capability/deferred-work tests pass. Eleven new real SQLite runtime tests cover unpublished drafts, default locale, unique slugs, typed mappings, rejected metadata, required selections, and inbox preservation on failure. A controlled pending-create test proves confirmation does not wait for content creation. Claude Opus 5.5 performed a complete tool-disabled adversarial review plus follow-ups and approved the final fixes.
An existing main race remains outside this diff: writing submission counters from a whole-form snapshot can restore concurrently changed settings, including a cleared mapping or paused status. Opus accepted a separate follow-up because a reliable fix requires atomic updates or separate counter storage. Expanded-year dates also remain a documented nonblocking runtime limitation.
This verification applies to the prepared update, not GitHub's existing 75835fb39 head. Once someone with write access applies the update, hosted CI and human maintainer review are still required.
~ 🤖 Codex (GPT-6)
What does this PR do?
Adds an optional
contentMappingsetting to the forms plugin. When a form has one, each successful submission also creates a draft entry in a chosen collection. The submission is still stored in the forms inbox as it is today, so the mapping only adds to what a submission does.Feature Discussion: #1672
Drafts only. Entries are always created as drafts. @masonjames suggested defaulting to drafts, or only allowing them. This PR only allows drafts, since anonymous visitors should not be able to publish content. A
statusoption can be added later if there is a need for one.Checked on save. Creating, updating or duplicating a form checks its mapping against the target collection. The collection and every mapped field must exist, and
metadatakeys must be fields in the collection. Each form field, with its transform, must produce a value the target field accepts, so a text field cannot feed a number field without thenumbertransform, and a select field's options must all be options of the target. A field cannot be set by both a mapping andmetadata, and file fields cannot be mapped. As @MA2153 asked, every required field in the collection must be covered by a non-emptymetadatavalue or a required form field that is always shown. An edit to a form's pages that would break its mapping is rejected as well.Submissions are never lost. The entry is created after the submission is stored, in
after()like the webhook, so the visitor gets their confirmation without waiting for it. If creating the entry fails, the error is logged.Slugs. Entries get a unique slug from their
titleornamefield, the same way entries created in the admin do. An earlier version of this PR added aslugFromoption and changes to the plugin content API in core. Those are gone:main's plugin create path already generates slugs, so this PR no longer changesemdashcore.Capabilities. The plugin now declares
content:writeandschema:read. It uses them only for forms that have acontentMapping, to look up the target collection and to create the entry.Not in this PR. There is no admin UI for the mapping yet. It is set through the
forms/createandforms/updateplugin routes. I plan to follow up with a mapping editor in the admin once this shape lands.Type of change
Checklist
pnpm typecheckpassespnpm lintpasses (lint:jsonreports 0 diagnostics)pnpm testpasses (or targeted tests for my change)pnpm formathas been runmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain. (Not applicable: no admin UI changes.)@emdash-cms/plugin-forms: minor)AI-generated code disclosure
Screenshots / test output
Not applicable for screenshots, since there are no UI changes.
The tests cover the cases from the discussion: a form with no mapping behaves as before, a mapped submission creates an entry and keeps the inbox submission, an invalid mapping is rejected when the form is saved, and a failed create does not lose the submission. Each transform also has its own tests.
pnpm build,pnpm typecheckandpnpm lint:json(0 diagnostics) are clean on the merged branch.🤖 Generated with Claude Code