Skip to content

Fix local media ID normalization#1100

Merged
ascorbic merged 2 commits into
emdash-cms:mainfrom
jcheese1:codex/fix-local-media-id-normalization
May 19, 2026
Merged

Fix local media ID normalization#1100
ascorbic merged 2 commits into
emdash-cms:mainfrom
jcheese1:codex/fix-local-media-id-normalization

Conversation

@jcheese1
Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes media normalization so bare local media IDs are resolved through the local media provider before falling back to external URL behavior.

This matters for API/MCP-style writes that create media first, then use the returned media ID directly in an image field:

{
  "featured_image": "01KRZKN0BK219P9HBMPYYGYRHY"
}

Previously, EmDash stored that as an external image value:

{
  "provider": "external",
  "id": "",
  "src": "01KRZKN0BK219P9HBMPYYGYRHY"
}

That produced broken rendered image URLs. With this change, EmDash checks whether the bare string resolves as local media and stores it as a local media value. Unknown strings still keep the previous external fallback behavior.

Closes #

Type of change

  • Bug fix
  • Feature (requires maintainer-approved Discussion)
  • Refactor (no behavior change)
  • Translation
  • Documentation
  • Performance improvement
  • Tests
  • Chore (dependencies, CI, tooling)

Checklist

  • I have read CONTRIBUTING.md
  • pnpm typecheck passes
  • pnpm lint passes
  • pnpm test passes (or targeted tests for my change)
  • pnpm format has been run
  • I have added/updated tests for my changes (if applicable)
  • User-visible strings in the admin UI are wrapped for translation (if applicable). Do not include messages.po changes except in translation PRs — a workflow extracts catalogs on merge to main.
  • I have added a changeset (if this PR changes a published package)
  • New features link to an approved Discussion: https://github.com/emdash-cms/emdash/discussions/...

AI-generated code disclosure

  • This PR includes AI-generated code — model/tool: Codex / GPT-5

Screenshots / test output

Targeted test passed:

vitest run tests/unit/media/normalize.test.ts

✓ tests/unit/media/normalize.test.ts (23 tests) 5ms

Test Files  1 passed (1)
Tests       23 passed (23)

Note: the pnpm wrapper currently exits before running tests with:

ERR_PNPM_VERIFY_DEPS_BEFORE_RUN The value of the workspacePackagePatterns setting has changed
Run "pnpm install"

So I validated the touched test file directly with Vitest.

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 19, 2026

🦋 Changeset detected

Latest commit: ccda3a3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 13 packages
Name Type
emdash Patch
@emdash-cms/cloudflare Patch
@emdash-cms/fixture-perf-site Patch
@emdash-cms/perf-demo-site Patch
@emdash-cms/cache-demo-site Patch
@emdash-cms/admin Patch
@emdash-cms/auth Patch
@emdash-cms/blocks Patch
@emdash-cms/gutenberg-to-portable-text Patch
@emdash-cms/x402 Patch
create-emdash Patch
@emdash-cms/auth-atproto Patch
@emdash-cms/plugin-embeds Patch

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

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented May 19, 2026

Open in StackBlitz

@emdash-cms/admin

npm i https://pkg.pr.new/@emdash-cms/admin@1100

@emdash-cms/auth

npm i https://pkg.pr.new/@emdash-cms/auth@1100

@emdash-cms/blocks

npm i https://pkg.pr.new/@emdash-cms/blocks@1100

@emdash-cms/cloudflare

npm i https://pkg.pr.new/@emdash-cms/cloudflare@1100

emdash

npm i https://pkg.pr.new/emdash@1100

create-emdash

npm i https://pkg.pr.new/create-emdash@1100

@emdash-cms/gutenberg-to-portable-text

npm i https://pkg.pr.new/@emdash-cms/gutenberg-to-portable-text@1100

@emdash-cms/x402

npm i https://pkg.pr.new/@emdash-cms/x402@1100

@emdash-cms/plugin-ai-moderation

npm i https://pkg.pr.new/@emdash-cms/plugin-ai-moderation@1100

@emdash-cms/plugin-atproto

npm i https://pkg.pr.new/@emdash-cms/plugin-atproto@1100

@emdash-cms/plugin-audit-log

npm i https://pkg.pr.new/@emdash-cms/plugin-audit-log@1100

@emdash-cms/plugin-color

npm i https://pkg.pr.new/@emdash-cms/plugin-color@1100

@emdash-cms/plugin-embeds

npm i https://pkg.pr.new/@emdash-cms/plugin-embeds@1100

@emdash-cms/plugin-forms

npm i https://pkg.pr.new/@emdash-cms/plugin-forms@1100

@emdash-cms/plugin-webhook-notifier

npm i https://pkg.pr.new/@emdash-cms/plugin-webhook-notifier@1100

commit: ccda3a3

@ascorbic
Copy link
Copy Markdown
Collaborator

/review

Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Targeted, well-tested fix. The new resolveLocalId helper mirrors the existing resolveInternalUrl shape, and the fallback ordering (internal URL → http(s) external → local ID → external fallback) is sensible. A few minor observations below — none blocking.

id: "",
src: url,
});
};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: the existing test suite covers the happy path for a resolved local ID, but doesn't have a dedicated case for the new fallback — a bare non-URL string that the local provider doesn't recognize (or where no local provider is registered) still ending up as { provider: "external", id: "", src: "…" }. The behavior is correct, just untested. Worth adding an explicit case so a future refactor of resolveLocalId can't silently regress this.

};
}

async function resolveLocalId(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heads-up (not a blocker for this PR): resolveInternalUrl above passes the URL suffix (storage key) to localProvider.get(...), while MediaRepository.findById queries by id. So for any internal URL where the storage key differs from the row id (e.g. 01ABC.jpg vs row id 01ABC), the lookup will return null and fall back to external — the test at line 76 of normalize.test.ts only passes because the mock returns the provider item regardless of the argument. resolveLocalId does the right thing (querying by id), but the existing internal-URL path looks suspect. Probably worth a follow-up issue.

* - `null`/`undefined` → `null`
* - Bare URL string → `{ provider: "external", id: "", src: url }`
* - Bare internal media URL → resolved via local provider's `get()`
* - Bare local media ID → resolved via local provider's `get()`
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tiny nit: since the local-ID resolution only happens for strings that are neither an internal URL nor an http(s) URL, the JSDoc could be a hair more explicit about ordering — e.g. Bare local media ID (only when not a URL) → resolved via local provider's get(). Not blocking.

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented May 19, 2026

Review posted successfully. Summary:

  • Targeted, well-tested fix for normalizing bare local media IDs (the documented MCP/API use case).
  • Three minor, non-blocking observations:
    1. Missing test for the "unknown bare string falls back to external" case.
    2. Pre-existing suspect behavior in resolveInternalUrl (passes storage key to findById, which queries by id) — flagged as a follow-up.
    3. Tiny JSDoc clarity nit about resolution order.

github run

Copy link
Copy Markdown
Collaborator

@ascorbic ascorbic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@ascorbic ascorbic merged commit f753dba into emdash-cms:main May 19, 2026
40 checks passed
@emdashbot emdashbot Bot mentioned this pull request May 19, 2026
ascorbic added a commit that referenced this pull request May 19, 2026
Restores the two changesets removed in #1105 (which existed only to flip changesets/action onto the publish path for the already-versioned 0.13.0). With 0.13.0 now published, these re-enter the version path: the next Release run bumps 0.13.0 -> 0.13.1 and refreshes the changeset-release PR (#1103), whose merge publishes 0.13.1 with the #1100/#1101 fixes documented. Files are byte-identical to their pre-#1105 state.
ascorbic added a commit that referenced this pull request May 19, 2026
#1110 reintroduced these changesets, so changelog-github credited both to #1110/@ascorbic, dropping @jcheese1's credit for #1100. Add per-changeset pr/commit/author overrides (read by @changesets/changelog-github) so the regenerated release PR attributes #1100 to @jcheese1 and #1101 to @ascorbic with the original commits. Editing #1103 directly wouldn't survive the next force-push of changeset-release/main.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants