Repository navigation
fix(core): save a blank image or file field value as null - #3129
Conversation
🦋 Changeset detectedLatest commit: 721f201 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 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 |
@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
upgrade-emdash
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-ai-search
@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.
The fix’s intent is right: normalizeMediaValue should not turn "" into a fake { provider: "external", id: "", src: "" } object. However, returning null is only half the story. The runtime then relies on if (normalized) result[field.slug] = normalized to leave the value untouched, so an empty string stays "" in processedData. validateContentData validates image/file fields against a generated Zod schema that expects a MediaValue object (or null/undefined for optional fields) and does not accept "". Because validation runs right after normalizeMediaFields, saving an entry whose media field is currently "" will now fail with VALIDATION_ERROR rather than silently persisting.
This is a concrete regression for the exact data the PR describes — optional imported image/file fields containing "" — so I don’t think it should merge as-is.
The two reasonable fixes are:
- Canonicalize blank strings to
nullin the save pipeline. This is the smallest correct change:nullis already the canonical "no media" shape and the schema accepts it. It means existing""values migrate tonullon the next save, but after that they are stable and no longer churn. - Make
validateContentData/generateZodSchemaaccept""for image/file fields. This preserves the raw""byte-for-byte, but it widens the field type and may require other consumers to tolerate the string.
I lean toward option 1 because it keeps the field’s canonical type honest and avoids a one-off string value lingering in image/file columns.
I checked the diff, the changed unit test, the runtime save/validation flow, the generated content schema, the WordPress import rewrite path, and the changeset. The unit test correctly covers normalizeMediaValue, but there is no save/validation round-trip test proving that a blank media field can actually be persisted after this change. Whichever fix is chosen, an integration test should exercise that path.
There was a problem hiding this comment.
The approach is the right one: null is already the canonical empty value for image/file fields, and the save pipeline should canonicalize blank strings to null so existing "" values migrate and stop churning on every save.
The re-review confirms the prior requested changes are addressed:
normalizeMediaValue()now returnsnullfor empty/whitespace-only strings before any provider lookup.EmDashRuntime.normalizeMediaFields()now writesnullfor blank top-level image/file values and for blank images inside repeater items, so the validator no longer sees a raw"".- A new integration test (
media-empty-string.test.ts) exercises the create/update round-trip throughhandleContentCreate/handleContentUpdateand asserts the stored values becomenull.
I checked the diff, normalizeMediaFields, the validation/schema pipeline, the content repository serialization, the dark-variant normalizer, the WordPress rewrite path, and the changeset. The only remaining issue is a new inline comment that justifies the decision instead of explaining a non-obvious invariant.
|
Dropped in d66aaf8. The No logic change; |
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V5zZFLGnKweBXoAobayQmE
13cbc16 to
c86d275
Compare
|
Rebased onto current Blank media inside a block. The block branch of Changeset. Narrowed to what the code does: "A blank image or file field, or a blank image inside a repeater, is now saved as Checked on the rebased branch: |
Fixes saving empty image or file fields to return null instead of an object. Updates validation for required fields to prevent saving empty values.
danielmlr
left a comment
There was a problem hiding this comment.
Thank you for this fix!
I added one sentence to the changeset. It says that a required image or file field now returns a validation error for "", where it used to save an empty image.
There was a problem hiding this comment.
Canonicalizing blank image/file strings to null at save time is the right approach for the export/sync churn. The top-level image/file and repeater-image changes address the original failure, and the changeset now matches the narrower repeater scope and mentions the required-field change.
The reviewers disagree about the block-field handling. One reviewer judged the new block fallback fixed and approved, noting only that no test covers it. The other found that block validation (normalizeBlocksData() → normalizeBlockArray() → blockSchema(...).safeParse) runs before normalizeFieldValues(), so blank block media is rejected with VALIDATION_ERROR before the new fallback can run. That would make the fix ineffective for blocks. I kept this as needs_fixing, since an untested block path with a possible upstream rejection is the riskiest part of the change.
The earlier request for repeater file sub-fields was withdrawn by the reviewer who made it, because RepeaterSubField does not support files. I did not carry it forward. Nothing was run; both reviews were static.
Reviewed by anthropic/claude-opus-5-5 and openai/gpt-6.1-sol.
Addressed in later commits.
What does this PR do?
Saving an entry whose image or file field holds an empty string (
"") rewrites the field to{ "provider": "external", "id": "", "src": "" }. Nothing visible changes, since both render as no image, but every save of such an entry changes its stored data, which shows up as churn in exports, diffs and sync jobs. On our site that was most of ~9,700 directory listings, whose importedlogofield is""whenever a business has no logo.No existing issue.
Cause
normalizeMediaValue()(src/media/normalize.ts) sends any string tonormalizeStringUrl(). An empty string is not an internal media URL, not anhttp(s)URL, and not a local media id, so it reaches the "unrecognized string, treat as external" fallback and comes back as an external media object with an emptysrc.Fix
normalizeMediaValue()returnsnullfor an empty or whitespace-only string, before any provider lookup. The same path coversdarkVariant, where a blank string now drops the variant instead of attaching an empty external object.EmDashRuntime.normalizeMediaFields()writesnullfor a blank top-level image or file value, and for a blank image inside a repeater item. It used to assign only non-null results, which would have left""in place, andvalidateContentDatarejects""for these fields (the review on this PR caught that).nullis the canonical empty value and the field schema accepts it, so an existing""becomesnullon its next save and then stays stable.Every other string input is unchanged.
Type of change
Checklist
pnpm typecheckpasses (packages/core, afterpnpm buildat the root)pnpm lintpasses (pnpm lint:json: 0 diagnostics)pnpm testpasses (or targeted tests for my change):tests/unit/media,tests/unit/fields,tests/integration/content/repeater-media-normalize.test.tsand the newtests/integration/content/media-empty-string.test.ts, 21 files, 213 testspnpm formathas been runemdash: patch)AI-generated code disclosure
Screenshots / test output
Not applicable for UI.
Unit test in
tests/unit/media/normalize.test.ts: for""and" "the result isnulland the local provider is never asked.Integration test in
tests/integration/content/media-empty-string.test.ts. It creates an entry with a blank image, a whitespace-only file and a blank repeater image, and it updates an existing entry with a blank image. Without the runtime change:With it: