Skip to content

fix(core): inject favicon link with correct MIME type from site settings#847

Merged
ascorbic merged 2 commits intomainfrom
fix/favicon-injection
Apr 30, 2026
Merged

fix(core): inject favicon link with correct MIME type from site settings#847
ascorbic merged 2 commits intomainfrom
fix/favicon-injection

Conversation

@ascorbic
Copy link
Copy Markdown
Collaborator

What does this PR do?

User-configured site favicons were never injected into the public site's <head> by core. Per-template Base.astro layouts each rendered their own <link rel="icon">, but pre-#448 templates lacked the line entirely, and even current templates dropped the type attribute. SVG favicons therefore failed to render in Chromium browsers, which require type="image/svg+xml" when the URL has no .svg extension.

This PR centralizes favicon injection in EmDashHead and emits the correct MIME type from the stored media row.

Closes #831

Approach

A new renderSiteIdentity() helper in packages/core/src/page/site-identity.ts emits a <link rel="icon"> tag with the optional type attribute. It deliberately lives outside the plugin contribution pipeline (page/metadata.ts) because that pipeline's isSafeHref allowlist rejects same-origin paths like /_emdash/api/media/file/.... That restriction is correct for sandboxed plugin contributions but blocks our own first-party favicon URLs.

MediaReference now carries url, contentType, width, and height when resolved via resolveMediaReference, so callers can emit correct head tags without a second round-trip to the media table. The stored shape is unchanged: only the in-memory resolved value picks up the extra fields. Zod schemas at the REST/MCP boundary still define just { mediaId, alt? } and rely on default strip-mode parsing to discard the resolved fields if a client posts them back. A docstring on MediaReference calls this out so a future contributor doesn't accidentally relax the schema and start writing the resolved fields back to storage.

EmDashHead.astro now calls getSiteSettings() instead of getSiteSetting("seo") so it can pick up favicon alongside the SEO settings. Both helpers are request-cached and worker-cached; the standard Base.astro pattern in every shipped template already calls getSiteSettings() first, so this is a free read in practice.

Backwards compatibility

Templates that already render their own favicon link continue to work. Browsers tolerate the duplicate. A follow-up cleanup can drop the per-template line once this has shipped, but it does not need to be coordinated.

Query-count impact

Zero net delta from this change. getSiteSettings() is request-cached; Base.astro already calls it before EmDashHead renders. Verified by running pnpm query-counts with and without the change applied: identical numbers either way. The repository's snapshot is currently stale relative to actual behaviour on main (every public route is +1 to +6 queries above the recorded values), but that pre-existing drift is unrelated to this PR.

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 (0 diagnostics)
  • pnpm test passes (3053 / 3053 in core)
  • pnpm format has been run
  • I have added/updated tests for my changes (7 new unit tests in tests/unit/page/site-identity.test.ts)
  • 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/... (n/a, bug fix)

AI-generated code disclosure

  • This PR includes AI-generated code — model/tool: Claude Opus 4.7

Screenshots / test output

$ pnpm --filter emdash test --run tests/unit/page/site-identity
 ✓ tests/unit/page/site-identity.test.ts (7 tests) 2ms

 Test Files  1 passed (1)
      Tests  7 passed (7)

$ pnpm --filter emdash test --run
 Test Files  184 passed (184)
      Tests  3053 passed (3053)

The 7 new tests cover:

  • Empty / undefined input is a no-op
  • MediaReference without resolved url is a no-op (handles deleted media row gracefully)
  • Favicon with url and contentType renders the correct tag
  • SVG favicons get type="image/svg+xml" (the Favicon functionality fails against vectorized images #831 bug)
  • Missing contentType falls back to a typeless <link> tag (legacy stored media)
  • Hostile content in url and contentType is HTML-attribute-escaped via escapeHtmlAttr

Resolves #831. The user-configured site favicon was never emitted into
the public site's <head> by core. Per-template Base.astro layouts each
rendered their own <link rel="icon">, but pre-#448 templates lacked
the line entirely, and even current templates dropped the type
attribute. SVG favicons therefore failed to render in Chromium browsers,
which require type="image/svg+xml" when the URL has no .svg extension.

EmDashHead now emits a <link rel="icon"> with the correct type
attribute sourced from the stored media's MIME type. The work lives in
a new renderSiteIdentity helper rather than the plugin contribution
pipeline because that pipeline's isSafeHref allowlist rejects same-origin
paths (correct for sandboxed plugin contributions, but blocks our own
/_emdash/api/media/file/... favicon URLs).

MediaReference now carries url, contentType, width, and height when
resolved via resolveMediaReference, so callers can emit correct head
tags without a second round-trip to the media table.

Templates that already render their own favicon link continue to work;
browsers tolerate the duplicate, and a follow-up cleanup can drop the
per-template line.

Query-count neutral: getSiteSettings() is request-cached and the
standard Base.astro pattern already calls it before EmDashHead runs.
Copilot AI review requested due to automatic review settings April 30, 2026 12:07
@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented Apr 30, 2026

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
✅ Deployment successful!
View logs
emdash-perf-coordinator 2eb7071 Apr 30 2026, 01:02 PM

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Apr 30, 2026

🦋 Changeset detected

Latest commit: 2eb7071

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

@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented Apr 30, 2026

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
✅ Deployment successful!
View logs
emdash-demo-cache 2eb7071 Apr 30 2026, 01:03 PM

@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented Apr 30, 2026

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
✅ Deployment successful!
View logs
emdash-i18n 2eb7071 Apr 30 2026, 01:02 PM

@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented Apr 30, 2026

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
✅ Deployment successful!
View logs
docs 2eb7071 Apr 30 2026, 01:02 PM

@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented Apr 30, 2026

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
✅ Deployment successful!
View logs
emdash-playground 2eb7071 Apr 30 2026, 01:03 PM

@ascorbic
Copy link
Copy Markdown
Collaborator Author

/review

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented Apr 30, 2026

Open in StackBlitz

@emdash-cms/admin

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

@emdash-cms/auth

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

@emdash-cms/blocks

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

@emdash-cms/cloudflare

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

emdash

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

create-emdash

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

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

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

@emdash-cms/x402

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

@emdash-cms/plugin-ai-moderation

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

@emdash-cms/plugin-atproto

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

@emdash-cms/plugin-audit-log

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

@emdash-cms/plugin-color

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

@emdash-cms/plugin-embeds

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

@emdash-cms/plugin-forms

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

@emdash-cms/plugin-webhook-notifier

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

commit: 2eb7071

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Centralizes public-site favicon injection in core (EmDashHead) and ensures the emitted <link rel="icon"> includes the correct MIME type (notably image/svg+xml) based on the stored media row, fixing Chromium SVG favicon rendering for extension-less URLs.

Changes:

  • Add renderSiteIdentity() to render first-party site-identity <head> tags (currently favicon), with HTML-attribute escaping.
  • Extend resolved MediaReference values to include contentType, width, and height alongside url (no stored-shape change).
  • Update EmDashHead.astro to load full site settings and inject the favicon link; add unit tests + changeset.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
packages/core/src/components/EmDashHead.astro Switches to getSiteSettings() and injects renderSiteIdentity() output into <head>.
packages/core/src/page/site-identity.ts New helper to render favicon <link> with optional type and proper attribute escaping.
packages/core/src/settings/index.ts Enhances media reference resolution to include MIME type + intrinsic dimensions.
packages/core/src/settings/types.ts Documents and extends MediaReference with resolved-only fields (url, contentType, width, height).
packages/core/tests/unit/page/site-identity.test.ts Adds focused unit coverage for favicon rendering behavior and escaping.
.changeset/fix-favicon-injection.md Patch changeset documenting the bugfix and the resolved MediaReference additions.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented Apr 30, 2026

I'll honestly assess. The bug fix is correct, scoped, well-tested, and the latent risk is documented in code. I don't have a HIGH or MEDIUM bug. The "post LOW for defense-in-depth" is exactly the kind of "while I'm here" comment that the skill says NOT to manufacture: "If you found nothing, say 'No bugs found' and stop. Don't manufacture issues to seem thorough."

The PR description explicitly addresses everything I'd flag. Time to call it.

LGTM!

github run

Removes the redundant <link rel="icon"> from all 16 bundled
Base.astro layouts (templates, demos, fixtures, infra) and drops
siteFavicon from the matching site-identity helpers. Core's
EmDashHead now emits the favicon link via renderSiteIdentity()
with the correct type attribute, so per-template handling would
just produce a duplicate tag.

Existing user sites that still emit their own <link rel="icon">
continue to work because browsers tolerate the duplicate.

Updated blog-site-identity and starter-site-identity unit tests
to reflect the new helper return shape.
@ascorbic
Copy link
Copy Markdown
Collaborator Author

Addressing review feedback in 2eb7071:

  • Dropped <link rel="icon"> from all 16 bundled Base.astro files (templates, demos, fixtures, infra) and removed siteFavicon from the matching site-identity.ts helpers. Core now emits the link via renderSiteIdentity(), so the per-template line was producing a duplicate tag.
  • Pulled back from emitting apple-touch-icon and application-name automatically. They need their own configurable fields in SiteSettings: an SVG favicon would produce a broken iOS home-screen icon, and a small PNG favicon would scale up blurry. Tracked separately.
  • Re-verified query counts after rebuilding core: pnpm query-counts matches the snapshot. The earlier "drift" I'd noted was a stale build in my local fixture, not real.

@ascorbic ascorbic merged commit 1e2b024 into main Apr 30, 2026
36 checks passed
@ascorbic ascorbic deleted the fix/favicon-injection branch April 30, 2026 14:02
@emdashbot emdashbot Bot mentioned this pull request Apr 30, 2026
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.

Favicon functionality fails against vectorized images

2 participants