fix(core): normalize media in repeater image sub-fields - #2251
fix(core): normalize media in repeater image sub-fields#2251dchaudhari7177 wants to merge 3 commits into
Conversation
🦋 Changeset detectedLatest commit: 1d95270 The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 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 |
github-actions
Bot
added
area/core
size/M
review/awaiting-author
@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-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@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: |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR fixes the right bug in the right place: normalizeMediaFields only normalized top-level image/file fields, so a bare media ID posted into a repeater image sub-field was stored verbatim. The change walks repeater items and runs the same normalizeMediaValue helper on image sub-fields, with per-sub-field failures swallowed just like top-level failures. That matches the existing media-usage extraction code, which already knows how to find media IDs inside repeater image sub-fields.
I checked the changed runtime code and the new integration test, traced call-sites for normalizeMediaFields (used on both create and update paths), confirmed RepeaterSubField only supports image media sub-fields (so filtering sub.type === "image" is consistent and not an accidental omission of file), and verified there are no SQL, auth, locale-table, or logged-out hot-path changes.
No logic bugs or regressions. Two comments are PR-style narrative rather than evergreen code explanation; I'd tighten/delete them before merge.
| // (a bare media id posted inside a repeater item would otherwise be stored verbatim and | ||
| // render as "Image not found" in the admin). | ||
| const repeaterFields = collectionInfo.fields.filter( |
There was a problem hiding this comment.
[suggestion] This three-line comment is PR narrative/justification: it explains the bug symptom and why the code exists, rather than a non-obvious invariant a future reader would get wrong. Per AGENTS.md, comments should be evergreen and not summarize the change or reference admin-only symptoms.
| // (a bare media id posted inside a repeater item would otherwise be stored verbatim and | |
| // render as "Image not found" in the admin). | |
| const repeaterFields = collectionInfo.fields.filter( | |
| // Repeater sub-fields are stored inside JSON items, so image sub-fields need | |
| // the same MediaValue normalization as top-level image/file fields. |
Alternatively, delete the comment entirely — the loop body is clear enough on its own.
| // Before the fix the bare id string was stored verbatim inside the repeater item, so the | ||
| // admin rendered "Image not found". It must now be normalized exactly like the top-level | ||
| // `hero` field (a MediaValue object, not the raw id string). |
There was a problem hiding this comment.
[suggestion] This comment is change narrative (Before the fix..., It must now be normalized...), which belongs in the PR description/commit message, not in a test file. Per AGENTS.md, comments should not reference bugs, fixes, or PR context. The assertions below already make the expected behavior clear.
| // Before the fix the bare id string was stored verbatim inside the repeater item, so the | |
| // admin rendered "Image not found". It must now be normalized exactly like the top-level | |
| // `hero` field (a MediaValue object, not the raw id string). | |
| expect(typeof subImage).toBe("object"); |
(i.e., remove the three-line comment and keep the assertions.)
github-actions
Bot
added
review/needs-rereview
khoinguyenpham04
added
the
bot:review
There was a problem hiding this comment.
This is still the right fix in the right place: normalizeMediaFields now walks repeater image sub-fields and applies the same normalizeMediaValue treatment as top-level image/file fields, which matches how media-usage extraction already discovers nested repeater media IDs. The create/update call sites are unchanged, and the new integration test compares the repeater sub-field output to the top-level field output so it doesn't overfit on a concrete object shape.
Since the prior review, nothing in the runtime code or test appears to have changed. The only priorfeedback still open is the two PR-style narrative comments flagged last time:
packages/core/src/emdash-runtime.tsstill has a three-line comment that narrates the pre-fix bug ("a bare media id posted inside a repeater item would otherwise be stored verbatim and render as 'Image not found'"). The first line is legitimate intent; the rest is PR context.- The new test still has a comment that explains the pre-fix behavior rather than trusting its name and assertions.
No logic bugs, security issues, or AGENTS.md violations beyond comment discipline. I'd still tighten/delete those two comments before merge.
| // Repeater fields can contain image sub-fields, whose values need the same normalization | ||
| // (a bare media id posted inside a repeater item would otherwise be stored verbatim and | ||
| // render as "Image not found" in the admin). |
There was a problem hiding this comment.
[suggestion] This comment explains the non-obvious intent on line 3666, but then narrates the PR bug history ("would otherwise be stored verbatim and render as 'Image not found' in the admin"). Per AGENTS.md commentary discipline, comments should be evergreen and addressed to future readers, not describe the pre-fix behavior.
| // Repeater fields can contain image sub-fields, whose values need the same normalization | |
| // (a bare media id posted inside a repeater item would otherwise be stored verbatim and | |
| // render as "Image not found" in the admin). | |
| // Walk repeater fields so their image sub-fields get the same normalization as top-level fields. |
| // Before the fix the bare id string was stored verbatim inside the repeater item, so the | ||
| // admin rendered "Image not found". It must now be normalized exactly like the top-level | ||
| // `hero` field (a MediaValue object, not the raw id string). |
There was a problem hiding this comment.
[suggestion] This comment restates pre-fix behavior and the PR's motivation ("Before the fix...", "Image not found"). The test name and assertions already describe the observable behavior; the comment is PR narrative, not evergreen test documentation.
| // Before the fix the bare id string was stored verbatim inside the repeater item, so the | |
| // admin rendered "Image not found". It must now be normalized exactly like the top-level | |
| // `hero` field (a MediaValue object, not the raw id string). | |
| expect(typeof subImage).toBe("object"); | |
| expect(subImage).not.toBe(mediaId); | |
| expect(subImage).toEqual(hero); |
github-actions
Bot
added
review/awaiting-author
What does this PR do?
normalizeMediaFieldsonly visited fields whose top-level type isimage/file, so a bare media id posted into a repeater image sub-field was stored verbatim and the admin rendered "Image not found". This walks the image sub-fields of repeater fields and normalizes their values the same way (vianormalizeMediaValue), so a repeater sub-field gets the same treatment as a top-level image field. Top-level fields and non-repeater collections are unaffected; failures for a single sub-field are swallowed (as for top-level fields) so a save is never blocked.Closes #2231
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runAI-generated code disclosure
Screenshots / test output
Added
tests/integration/content/repeater-media-normalize.test.ts: it posts the same bare media id into a top-levelimagefield and a repeaterimagesub-field and asserts the repeater value normalizes identically to the top-level field (so it's harness-independent). It fails onmain(the sub-field stays a raw id string) and passes here. Thecontentintegration suite +media-usage-content-fieldsstay green (55 tests);oxlintis clean on the changed files.I ran targeted tests and
oxlint/prettieron the changed files rather than the fullpnpm typecheck/pnpm lint/pnpm test(which build the whole monorepo); happy to run those if you'd like.