Skip to content

fix: emit served URLs for next/font/google self-hosted assets#819

Merged
james-elicx merged 3 commits intocloudflare:mainfrom
ShorebirdManagement:fix/preload-link-header-uses-served-url
Apr 12, 2026
Merged

fix: emit served URLs for next/font/google self-hosted assets#819
james-elicx merged 3 commits intocloudflare:mainfrom
ShorebirdManagement:fix/preload-link-header-uses-served-url

Conversation

@Shorebirdmgmt
Copy link
Copy Markdown
Contributor

Summary

fetchAndCacheFont() in packages/vinext/src/plugins/fonts.ts downloads Google Fonts .woff2 files into <root>/.vinext/fonts/<family>-<urlHash>/ and rewrites the cached @font-face CSS with css.split(fontUrl).join(path.join(fontDir, filename)) — an absolute dev-machine filesystem path. That CSS is then embedded verbatim as _selfHostedCSS in the server bundle, and every downstream consumer reads from the same leaked string: the injected <style data-vinext-fonts> block's @font-face { src: url(...) }, the HTML body's <link rel="preload"> tags, and the HTTP Link: response header.

Production requests on workerd produce header and body preload entries like </home/<user>/<project>/.vinext/fonts/geist-<hash>/geist-<hash>.woff2> — the browser follows the absolute href to <origin>/home/<user>/..., workerd returns 404 (the cached files are never copied into dist/client/ either), and the console fills with downloadable font: download failed and preloaded with link preload was not used warnings on every page view. Because preload is high-priority for fonts, the broken request contends with real critical-path traffic and Cloudflare's 103 Early Hints path would emit these broken entries before the HTML even starts streaming.

Any app using stock next/font/google self-hosted mode is affected — no user-facing config can work around it (#472 tracks assetPrefix support but that's orthogonal, and the leak happens regardless of whether assetPrefix is set).

Root cause

Three downstream font-preload emitters all read from the same in-memory array populated by collectFontPreloadsFromCSS() in shims/font-google-base.ts, which extracts url(...) references from _selfHostedCSS via regex:

  1. The injected <style data-vinext-fonts> block's @font-face { src: url(...) } (via ssrFontStyles in shims/font-google-base.ts).
  2. The HTML body's <link rel="preload"> tags emitted from server/app-ssr-entry.ts:renderFontHtml() via fontData.preloads[*].href.
  3. The HTTP Link: response header, built by buildAppPageFontLinkHeader() in server/app-page-execution.ts and set on the response in server/app-page-response.ts:242.

All three read from the same source of truth, so a single fix at the CSS level propagates to every emitter.

The upstream source — the cached CSS — is written by fetchAndCacheFont():

// packages/vinext/src/plugins/fonts.ts
for (const [fontUrl, filename] of urls) {
  const filePath = path.join(fontDir, filename);
  // ... download + write to disk ...
  // Rewrite CSS to use absolute path (Vite will resolve /@fs/ for dev, or asset for build)
  css = css.split(fontUrl).join(filePath);
}

The "Vite will resolve /@fs/ for dev, or asset for build" comment describes the intended behaviour but is not what actually happens: the CSS is embedded as a JavaScript string literal in the bundle, and Vite's asset pipeline operates on CSS files and import/new URL(...) references — not on string literals inside JS. The filesystem path gets baked into _selfHostedCSS verbatim, and nothing downstream rewrites it.

Separately, fetchAndCacheFont leaves the downloaded .woff2 files in <root>/.vinext/fonts/ and nothing ever copies them into dist/client/ — so even a correctly-rewritten URL would 404 in production without a companion copy step.

Fix

Two changes in packages/vinext/src/plugins/fonts.ts, both inside the existing vinext:google-fonts plugin:

  1. _rewriteCachedFontCssToServedUrls() — a small helper (3 lines of real logic plus regression-detection JSDoc) that replaces the absolute cacheDir prefix in a cached CSS string with the served URL namespace /assets/_vinext_fonts. Called from injectSelfHostedCss() right before the CSS string is JSON.stringify'd into the bundle, so every downstream consumer sees the rewritten URL.

  2. writeBundle hook (client environment only) — recursively copies every .woff2/.woff/.ttf/.otf/.eot file out of <root>/.vinext/fonts/ into <clientOutDir>/assets/_vinext_fonts/, preserving the <family>-<hash>/ subdirectory structure. The existing _headers rule for /assets/* already covers the new namespace, and StaticFileCache picks the copied files up via its recursive walk of dist/client/, so Content-Type: font/woff2, Cache-Control: public, max-age=31536000, immutable, and an automatic content-hashed ETag all flow through without any server-side changes.

No config surface is added — this does not implement #472 (assetPrefix / basePath support), it just stops the absolute filesystem path from leaking into the preload emitters regardless of whether assetPrefix is set.

Tests

Two new regression tests, both verified to fail on main and pass with the fix:

  • Unittests/font-google.test.ts: five cases in a new _rewriteCachedFontCssToServedUrls describe block drive the helper directly with (a) a realistic cached CSS containing multiple url(...) references, (b) multi-occurrence replacement where the same path appears multiple times in a single block, (c) a cache directory containing regex metacharacters (/tmp/build (1)/...) to prove split/join is safer than a constructed regex, (d) CSS that never references the cache directory (no-op), and (e) an empty cacheDir defensive guard so a split on "" doesn't insert the URL namespace between every character.
  • Integrationtests/app-router.test.ts, in a new App Router Production server self-hosted next/font/google headers describe block. Builds the existing tests/fixtures/font-google-multiple fixture (stock Geist + Geist_Mono via next/font/google) with a mocked fetch that stands in for the Google Fonts CDN. The mock returns CSS with real https://fonts.gstatic.com/... URLs so fetchAndCacheFont's regex actually exercises the path-rewriting code that was the bug source — returning CSS with already-relative URLs would sidestep the failure mode. Starts startProdServer() on a random port and asserts on the raw HTTP response: (a) Link: header contains the served URL and neither the absolute fixture path nor .vinext/fonts appears anywhere in it, (b) body <link rel="preload"> tags match /assets/_vinext_fonts/..., (c) the injected <style data-vinext-fonts> block's @font-face src: url() also uses the served URL, and (d) the copied font file serves 200 with content-type: font/woff2 and an immutable cache-control.

A separate fixture is used instead of extending app-basic because app-basic is shared by every integration test in tests/app-router.test.ts — adding next/font/google to its root layout would force a real Google Fonts network fetch into every test run in the file. The mocked-fetch approach keeps the test hermetic.

Verified

With the fix applied:

Check Result
pnpm test:unit 2853 / 2853 pass
pnpm test:integration 1262 / 1262 pass (2 pre-existing skips)
pnpm run check (lint + format + types) 0 warnings, 0 errors
pnpm test:e2eapp-router project 322 / 322 pass (3 flaky on retry, 9 pre-existing skips)
pnpm test:e2ecloudflare-workers project (Miniflare) 38 / 38 pass (1 flaky on retry)

Both new tests were also re-run with the fix reverted on a clean tree to confirm they deterministically catch the regression — the integration suite printed the leaked path verbatim in the <style data-vinext-fonts> assertion, e.g.

src: url(/home/<user>/.../.vinext/fonts/geist-4db05770f54f/geist-2fba11a4.woff2) format('woff2');

and the font-file 200 OK assertion failed because the cached .woff2 files were never copied into dist/client/.

End-to-end runtime verification on workerd (Miniflare, via wrangler dev) against examples/app-router-cloudflare with a next/font/google layout import:

Before

$ curl -sI http://localhost:4999/
HTTP/1.1 200 OK
Content-Type: text/html; charset=utf-8
Link: </home/<user>/<project>/.vinext/fonts/geist-4db05770f54f/geist-8e42e564.woff2>; rel=preload; as=font; type=font/woff2; crossorigin, </home/<user>/<project>/.vinext/fonts/geist-4db05770f54f/geist-bd9fc9d8.woff2>; rel=preload; as=font; type=font/woff2; crossorigin, </home/<user>/<project>/.vinext/fonts/geist-4db05770f54f/geist-79e7a99c.woff2>; rel=preload; as=font; type=font/woff2; crossorigin
$ curl -s http://localhost:4999/ | grep -oE '<link[^>]*preload[^>]*woff2[^>]*>'
<link rel="preload" href="/home/<user>/<project>/.vinext/fonts/geist-4db05770f54f/geist-8e42e564.woff2" as="font" type="font/woff2" crossorigin />
<link rel="preload" href="/home/<user>/<project>/.vinext/fonts/geist-4db05770f54f/geist-bd9fc9d8.woff2" as="font" type="font/woff2" crossorigin />
<link rel="preload" href="/home/<user>/<project>/.vinext/fonts/geist-4db05770f54f/geist-79e7a99c.woff2" as="font" type="font/woff2" crossorigin />
$ curl -sI 'http://localhost:4999/home/<user>/<project>/.vinext/fonts/geist-4db05770f54f/geist-8e42e564.woff2'
HTTP/1.1 404 Not Found

After

$ curl -sI http://localhost:4999/
HTTP/1.1 200 OK
Content-Type: text/html; charset=utf-8
Link: </assets/_vinext_fonts/geist-4db05770f54f/geist-8e42e564.woff2>; rel=preload; as=font; type=font/woff2; crossorigin, </assets/_vinext_fonts/geist-4db05770f54f/geist-bd9fc9d8.woff2>; rel=preload; as=font; type=font/woff2; crossorigin, </assets/_vinext_fonts/geist-4db05770f54f/geist-79e7a99c.woff2>; rel=preload; as=font; type=font/woff2; crossorigin
$ curl -s http://localhost:4999/ | grep -oE '<link[^>]*preload[^>]*woff2[^>]*>'
<link rel="preload" href="/assets/_vinext_fonts/geist-4db05770f54f/geist-8e42e564.woff2" as="font" type="font/woff2" crossorigin />
<link rel="preload" href="/assets/_vinext_fonts/geist-4db05770f54f/geist-bd9fc9d8.woff2" as="font" type="font/woff2" crossorigin />
<link rel="preload" href="/assets/_vinext_fonts/geist-4db05770f54f/geist-79e7a99c.woff2" as="font" type="font/woff2" crossorigin />
$ curl -sI http://localhost:4999/assets/_vinext_fonts/geist-4db05770f54f/geist-8e42e564.woff2
HTTP/1.1 200 OK
Content-Type: font/woff2
Cache-Control: public, max-age=31536000, immutable
ETag: "99db0b97ae3a4012f8b03153490d9518"
CF-Cache-Status: HIT

The writeBundle copy hook landed the cached files in dist/client/assets/_vinext_fonts/geist-4db05770f54f/ and the existing _headers rule for /assets/* applies an immutable cache-control header at the edge — workerd returned CF-Cache-Status: HIT on the second request, identical to every other hashed asset.

References

`fetchAndCacheFont()` in `packages/vinext/src/plugins/fonts.ts` downloads
Google Fonts `.woff2` files into `<root>/.vinext/fonts/<family>-<urlHash>/`
and rewrites the cached `@font-face` CSS with
`css.split(fontUrl).join(path.join(fontDir, filename))` — an absolute
dev-machine filesystem path. That CSS is then embedded verbatim as
`_selfHostedCSS` in the server bundle, and every downstream consumer reads
from the same leaked string:

  1. The injected `<style data-vinext-fonts>` block's
     `@font-face { src: url(...) }` (via `ssrFontStyles` in
     `shims/font-google-base.ts`).
  2. The HTML body's `<link rel="preload">` tags emitted from
     `server/app-ssr-entry.ts:renderFontHtml()` via
     `collectFontPreloadsFromCSS()`'s url-extraction regex.
  3. The HTTP `Link:` response header emitted from
     `buildAppPageFontLinkHeader()` in `server/app-page-execution.ts` and
     set on the response in `server/app-page-response.ts:242`.

Production requests on workerd produce header and body preload entries
like `</home/<user>/<project>/.vinext/fonts/geist-<hash>/geist-<hash>.woff2>`
— the browser requests `<origin>/home/<user>/...`, workerd returns 404
(the cached files are never copied into `dist/client/` either), and the
console fills with `downloadable font: download failed` and
`preloaded with link preload was not used` warnings on every page view.
Because preload is high-priority for fonts, the broken request contends
with real critical-path traffic.

The fix adds `_rewriteCachedFontCssToServedUrls()` which replaces the
`cacheDir` prefix with the served URL namespace `/assets/_vinext_fonts`
right before `_selfHostedCSS` is embedded in the bundle. A matching
`writeBundle` hook on the `vinext:google-fonts` plugin (client environment
only) recursively copies every `.woff2`/`.woff`/`.ttf`/`.otf`/`.eot` file
out of `<root>/.vinext/fonts/` into
`<clientOutDir>/assets/_vinext_fonts/` preserving the subdirectory
structure, so the rewritten URLs resolve against the origin instead of
404ing. No config surface is added — this does not implement `cloudflare#472`
(`assetPrefix` support), it just stops leaking the filesystem path
regardless of `assetPrefix`.

The existing `_headers` rule for `/assets/*` covers the new namespace,
and `StaticFileCache` picks the copied files up via its recursive walk of
`dist/client/`, so `Content-Type: font/woff2` and
`Cache-Control: public, max-age=31536000, immutable` and an automatic
content-hashed ETag all flow through without any server-side changes. On
Cloudflare the edge returns `CF-Cache-Status: HIT` on the second request,
unchanged from other assets.

## Tests

Two new regression tests, both verified to fail on `main` (before this
commit) and pass with the fix:

- **Unit** — `tests/font-google.test.ts`: five cases in a new
  `_rewriteCachedFontCssToServedUrls` describe block that drive the
  helper directly with crafted cache directories, multi-occurrence
  replacements, regex-metacharacter paths, empty cacheDir, and
  no-op CSS. Failure on `main`: the symbol does not exist, then
  asserts would detect the leaked `/home/` prefix even if it did.

- **Integration** — `tests/app-router.test.ts`, in a new
  `App Router Production server self-hosted next/font/google headers`
  describe block. Builds the existing `tests/fixtures/font-google-multiple`
  fixture (`Geist` + `Geist_Mono` via `next/font/google`) with a mocked
  fetch that stands in for the Google Fonts CDN — the mock returns CSS
  with real `https://fonts.gstatic.com/...` URLs so
  `fetchAndCacheFont`'s regex actually exercises the path-rewriting code
  that was the bug source. Starts `startProdServer()` on a random port
  and asserts on the raw HTTP response: (a) `Link:` header contains the
  served URL and neither the absolute fixture path nor `.vinext/fonts`
  appears anywhere in it, (b) body `<link rel="preload">` tags match
  `/assets/_vinext_fonts/...`, (c) the injected `<style data-vinext-fonts>`
  block's `@font-face src: url()` also uses the served URL, and (d) the
  copied font file serves 200 with `content-type: font/woff2` and an
  `immutable` cache-control. Failure on `main`: asserts produce the
  leaked path verbatim, e.g.
  `src: url(/home/<user>/.../.vinext/fonts/geist-<hash>/geist-<hash>.woff2)`.

A separate fixture is used instead of extending `app-basic` because
`app-basic` is shared by every integration test in the file — adding
`next/font/google` to its root layout would force a real Google Fonts
network fetch into every test run.

## Verified

With the fix applied:

| Check | Result |
|---|---|
| `pnpm test:unit` | **2853 / 2853** pass |
| `pnpm test:integration` | **1262 / 1262** pass (2 pre-existing skips) |
| `pnpm run check` (lint + format + types) | **0** warnings, **0** errors |
| `pnpm test:e2e` — `app-router` project | **322 / 322** pass (3 flaky on retry, 9 pre-existing skips) |
| `pnpm test:e2e` — `cloudflare-workers` project (Miniflare) | **38 / 38** pass (1 flaky on retry) |

Both new tests were also re-run with the fix reverted on a clean tree to
confirm they deterministically catch the regression — the integration
suite printed the leaked path verbatim and the unit suite failed at the
export site.

End-to-end runtime verification on `workerd` (Miniflare, via
`wrangler dev`) against `examples/app-router-cloudflare` with a
`next/font/google` layout import:

- **Before** — `curl -sI /` returns
  `Link: </home/<user>/<project>/.vinext/fonts/geist-<hash>/geist-<hash>.woff2>; rel=preload; as=font; ...`,
  the body `<link rel="preload">` tags contain the same path, and
  requesting the leaked path returns `HTTP/1.1 404 Not Found`.
- **After** — the same `curl -sI /` returns
  `Link: </assets/_vinext_fonts/geist-<hash>/geist-<hash>.woff2>; rel=preload; as=font; ...`,
  the body `<link>` tags match, and `curl -sI` on the served URL returns
  `HTTP/1.1 200 OK`, `Content-Type: font/woff2`,
  `Cache-Control: public, max-age=31536000, immutable`,
  `CF-Cache-Status: HIT` on the second request.

## References

- cloudflare#472 — `assetPrefix` / `basePath` support in `next.config` (orthogonal:
  this PR does not implement `assetPrefix`, it just stops the absolute
  filesystem path from leaking into the preload emitters regardless of
  whether `assetPrefix` is set)
- cloudflare#812 — parallel `applyMiddlewareRequestHeaders` sealed-snapshot
  invalidation fix that followed the same report-and-fixture pattern

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new bot commented Apr 10, 2026

Open in StackBlitz

npm i https://pkg.pr.new/vinext@819

commit: a5c2b72

@southpolesteve
Copy link
Copy Markdown
Collaborator

/bigbonk review this PR

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.

Review

This is a well-diagnosed and well-fixed bug. The PR description is thorough, the root cause analysis is accurate, and the fix is minimal and correct for the default configuration. The tests are solid — they cover the rewrite helper exhaustively and the integration test exercises the full production pipeline end-to-end.

One concern about build.assetsDir parity between the URL rewriter and the writeBundle copier, and a few minor notes below.

Summary

  • The core approach (rewrite CSS paths before embedding, copy font files in writeBundle) is sound and well-targeted.
  • Test coverage is excellent: unit tests cover edge cases (regex metacharacters, empty cacheDir, multi-occurrence), and the integration test uses a mocked fetch that exercises the actual fetchAndCacheFont rewrite codepath.
  • The fixture design decision (separate fixture vs. extending app-basic) is well-reasoned.
  • One real concern: hardcoded assets in _rewriteCachedFontCssToServedUrls will diverge from the writeBundle hook's config.build.assetsDir when a user customizes it.

Comment thread packages/vinext/src/plugins/fonts.ts Outdated
*/
export function _rewriteCachedFontCssToServedUrls(css: string, cacheDir: string): string {
if (!cacheDir || !css.includes(cacheDir)) return css;
return css.split(cacheDir).join(`/${DEFAULT_ASSETS_DIR}/${VINEXT_FONT_URL_NAMESPACE}`);
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.

Concern: build.assetsDir mismatch when customized.

_rewriteCachedFontCssToServedUrls hardcodes DEFAULT_ASSETS_DIR ("assets") into the served URL, but the writeBundle hook on line 887 reads the real assetsDir from this.environment.config.build.assetsDir. If a user sets build: { assetsDir: 'static' } in their Vite config, the CSS will contain /assets/_vinext_fonts/... while the files get copied to <outDir>/static/_vinext_fonts/... — producing 404s.

The JSDoc on DEFAULT_ASSETS_DIR (line 93-98) acknowledges this indirectly but frames it as "the matching writeBundle hook ... will fall back to this value when it is unset", which undersells the divergence risk.

I understand this helper is exported and used in tests without plugin state, which makes threading the real assetsDir through slightly awkward. A couple of options:

  1. Add an optional third parameter assetsDir = DEFAULT_ASSETS_DIR to _rewriteCachedFontCssToServedUrls and pass the resolved assetsDir at the call site in injectSelfHostedCss (where you have access to plugin state). The test API stays the same since the parameter would default.
  2. Store the resolved assetsDir in a closure variable (like cacheDir), populated in configResolved, and read it at the call site.

This is a low-probability issue (few apps customize assetsDir) but it would be a confusing 404 to debug and the fix is trivial, so worth addressing in this PR.

handler(outputOptions: { dir?: string }) {
// Only copy on the client build — the server/SSR environments
// don't serve static assets.
if (this.environment?.name !== "client") return;
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.

Nit: this.environment?.name and this.environment?.config?.build?.assetsDir — is the optional chaining on this.environment necessary here? In Vite's writeBundle hook, this.environment should always be available. If it's a defensive guard for Vite 7 compat (where this.environment might not exist), a comment would help. Otherwise the optional chaining on a property that's always present can mask bugs where the hook runs in an unexpected context.

Comment thread tests/app-router.test.ts Outdated
// of the bug. Returning CSS with already-relative URLs would sidestep
// the failure mode.
const originalFetch = globalThis.fetch;
globalThis.fetch = async (input: unknown) => {
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.

Good: the mock types input as unknown and coerces to string. Worth noting that globalThis.fetch signature expects RequestInfo | URL — the input as RequestInfo cast in the fallback is fine since it's only reached for non-Google URLs, but if Vite or any test helper passes a Request object with a Google Fonts URL, String(input) would produce [object Request] and the mock would miss it.

This is unlikely to matter in practice (the build plugin always passes string URLs to fetch), just flagging for awareness.

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk bot commented Apr 11, 2026

Review posted. Here's a summary of the feedback:

Overall: This is a well-diagnosed and well-fixed bug. The root cause analysis is accurate, the fix is minimal and correctly targeted, and the test coverage is excellent.

One real concern: _rewriteCachedFontCssToServedUrls hardcodes "assets" as the URL prefix, but the writeBundle hook reads the actual config.build.assetsDir from Vite when deciding where to copy files. If a user customizes build.assetsDir to something other than "assets", the embedded CSS URLs and the actual file locations would diverge, producing 404s. The fix is straightforward — add an optional assetsDir parameter to the rewrite function and pass the resolved value at the call site.

Two minor notes:

  1. The optional chaining on this.environment in the writeBundle hook — if it's defensive for Vite 7 compat, a comment would help; otherwise it could mask bugs.
  2. The fetch mock in the integration test coerces input via String(input), which would produce [object Request] if a Request object were passed — unlikely to matter since the build plugin passes string URLs, but worth awareness.

github run

Three follow-ups from `ask-bonk`'s review of df2710c:

## 1. Thread the real `build.assetsDir` through (real concern)

`_rewriteCachedFontCssToServedUrls` previously hardcoded the default
`assets` directory into the served URL while the `writeBundle` hook
read the real `envConfig.build.assetsDir` from Vite. A user who
customized `build.assetsDir` — for example to `"static"` — would get a
silent divergence: the embedded CSS would contain
`/assets/_vinext_fonts/...` while the font files were copied to
`<clientOutDir>/static/_vinext_fonts/...`, and every preload would 404
in production.

The fix adds an optional `assetsDir` parameter to the exported helper
(default `DEFAULT_ASSETS_DIR` so the existing unit tests still drive it
without plugin state) and threads the resolved value from the call
site in `injectSelfHostedCss`. Because `injectSelfHostedCss` is a
nested function declaration with no `this` binding, the value is read
once at the top of the outer `transform` handler (where `this.environment`
is bound by Rollup to the plugin context) and closed over by the inner
helper. Both the transform-time rewrite and the `writeBundle` copy hook
now read `this.environment?.config?.build?.assetsDir ?? DEFAULT_ASSETS_DIR`
from the same source, so they cannot diverge by construction.

The follow-up was verified end-to-end on workerd via Miniflare with a
fixture that sets `build.assetsDir: "static"` in its Vite config:

- `dist/client/static/_vinext_fonts/geist-<hash>/geist-<hash>.woff2`
  — files landed under the custom directory
- `_selfHostedCSS` in `dist/server/index.js` contains
  `src: url(/static/_vinext_fonts/...)`
- `curl -sI /` returns
  `Link: </static/_vinext_fonts/...>; rel=preload; as=font; ...`
- `curl -sI /static/_vinext_fonts/geist-<hash>/geist-<hash>.woff2`
  returns `200 OK`, `Content-Type: font/woff2`,
  `Cache-Control: public, max-age=31536000, immutable`,
  `CF-Cache-Status: HIT`

Two new unit test cases in `tests/font-google.test.ts` exercise
`rewriteCachedFontCssToServedUrls(..., "static")` and assert the URL
prefix tracks the argument, plus a defensive-guard case where an empty
string falls back to the default (so the URL never has a `//` segment).

Note: the Node.js production server's request handler hardcodes
`pathname.startsWith("/assets/")` as its static-file fast path (see
`server/prod-server.ts:995, 1286, 1551`) — this is a pre-existing
limitation affecting _all_ custom `build.assetsDir` users, not just
fonts, and is explicitly out of scope for this PR. On Cloudflare
Workers (the primary deployment target) the asset binding serves the
entire `dist/client/` tree recursively, so the custom-`assetsDir` case
works end-to-end today.

## 2. Document the `this.environment?` optional chaining (nit)

The reviewer asked whether the optional chaining on `this.environment`
in the `writeBundle` hook is defensive or masking bugs. It matches the
existing convention in `vinext:precompress` and `vinext:cloudflare-build`
(both in `src/index.ts`) but a short explainer keeps future readers
from wondering. Added a comment noting that under normal Vite builds
`this.environment` is always populated, the guard is there so the hook
safely no-ops if invoked outside a bound environment (e.g. from a
thin unit-test harness), and the early-return path is unreachable
during a real build.

The same comment clarifies why `this.environment.config` is read
inside the hook rather than via a closure variable from
`configResolved` — Vite's `configResolved` receives the top-level
resolved config and the per-environment `build.assetsDir` may differ,
so reading from `this.environment.config` at hook time is the only way
to get the value Vite actually used for the current environment.

## 3. Normalize `fetch()` input shapes in the test mock (nit)

The integration test's mock previously used `String(input)` to coerce
to a URL string, which would produce `[object Request]` for a `Request`
instance — not matching any of the mock's substring checks and
silently falling through to a real network fetch for Google Fonts.
The build plugin always passes string URLs today so this was
theoretical, but the fix is a four-line `resolveFetchUrl` helper that
handles `string`, `URL`, and `Request` explicitly before falling back
to `String()`. Prevents a future-vinext or test-helper regression
where the mock silently stops covering the build's real network calls.

## Verified

With the follow-up fixes applied:

| Check | Result |
|---|---|
| `pnpm test:unit` | **2855 / 2855** pass (+2 new `assetsDir` cases) |
| `pnpm test:integration` | **1262 / 1262** pass (2 pre-existing skips) |
| `pnpm run check` | **0** warnings, **0** errors |
| `pnpm test:e2e` — `app-router` project | **322 / 322** pass (2 flaky on retry, 9 pre-existing skips) |
| `pnpm test:e2e` — `cloudflare-workers` project (Miniflare) | **38 / 38** pass |

Custom `build.assetsDir: "static"` additionally verified end-to-end on
Miniflare against `examples/app-router-cloudflare` with a
`next/font/google` import — `Link:` header, body `<link rel="preload">`
tags, and the font file `200 OK` response all use `/static/...`
consistently.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@Shorebirdmgmt
Copy link
Copy Markdown
Contributor Author

Thanks for the review. Pushed 93af737 addressing all three points.

1. build.assetsDir divergence — fixed

Threaded the resolved assetsDir through from plugin state. _rewriteCachedFontCssToServedUrls now takes an optional third parameter (defaulting to DEFAULT_ASSETS_DIR so the exported helper still drives cleanly from unit tests without a plugin context). At the call site in injectSelfHostedCss, I read this.environment?.config?.build?.assetsDir ?? DEFAULT_ASSETS_DIR once at the top of the outer transform handler and close over it — doing this in the inner injectSelfHostedCss declaration directly triggered TS2683: 'this' implicitly has type 'any' because function declarations shadow this. The writeBundle hook already read the real value, so both call sites now consume the same source and cannot diverge by construction.

End-to-end verification on Miniflare with examples/app-router-cloudflare and build: { assetsDir: "static" }:

$ find dist/client -name '*.woff2'
dist/client/static/_vinext_fonts/geist-4db05770f54f/geist-8e42e564.woff2
dist/client/static/_vinext_fonts/geist-4db05770f54f/geist-bd9fc9d8.woff2
dist/client/static/_vinext_fonts/geist-4db05770f54f/geist-79e7a99c.woff2

$ curl -sI http://localhost:4997/ | grep -i '^link:'
Link: </static/_vinext_fonts/geist-4db05770f54f/geist-8e42e564.woff2>; rel=preload; as=font; type=font/woff2; crossorigin, </static/_vinext_fonts/geist-4db05770f54f/geist-bd9fc9d8.woff2>; rel=preload; as=font; type=font/woff2; crossorigin, </static/_vinext_fonts/geist-4db05770f54f/geist-79e7a99c.woff2>; rel=preload; as=font; type=font/woff2; crossorigin

$ curl -sI http://localhost:4997/static/_vinext_fonts/geist-4db05770f54f/geist-8e42e564.woff2
HTTP/1.1 200 OK
Content-Type: font/woff2
Cache-Control: public, max-age=31536000, immutable
ETag: "60f7c478fdf66f978f14ec8a5d2abfff"
CF-Cache-Status: HIT

Two new unit test cases cover the custom assetsDir path and the empty-string fallback (so the URL can never acquire a // segment if a misconfigured environment passes "").

One caveat worth flagging separately: while investigating I noticed server/prod-server.ts hardcodes pathname.startsWith("/assets/") at lines 995, 1286, and 1551 as the static-file fast path. That's a pre-existing limitation affecting every custom build.assetsDir user, not just fonts — on the Node.js production server a custom assetsDir currently 404s for all hashed assets, not just for the rewritten font URLs. Fixing that is out of scope for this PR since it predates the font leak and touches an unrelated codepath, but happy to file a follow-up issue if useful. On Cloudflare Workers (the primary deployment target) the asset binding serves the entire dist/client/ tree recursively, so the custom-assetsDir case works end-to-end today, as the Miniflare capture above shows.

2. Optional chaining on this.environment — documented

Left the optional chaining in place and added a comment explaining: it matches the existing convention in vinext:precompress and vinext:cloudflare-build (both in src/index.ts), this.environment is always populated during a real Vite build, the guard is there so the hook safely no-ops if invoked outside a bound environment (e.g. from a thin unit-test harness), and the early-return path is unreachable during a normal build. The same comment clarifies why the writeBundle hook reads this.environment.config.build.assetsDir at hook time rather than via a closure from configResolved — Vite's top-level configResolved receives the global config, but per-environment build.assetsDir may differ, so reading from this.environment.config at hook time is the only way to get the value Vite actually used for the current environment.

3. Fetch mock String(input) — hardened

Replaced with a small resolveFetchUrl helper that handles string, URL, and Request inputs explicitly before falling back to String(). As you noted, the build plugin always passes string URLs today so this is theoretical, but it prevents a future vinext internals change or test helper from silently making the mock fall through to a real network fetch for Google Fonts.

Verification matrix (re-run with follow-up)

Check Result
pnpm test:unit 2855 / 2855 pass (+2 new assetsDir cases)
pnpm test:integration 1262 / 1262 pass (2 pre-existing skips)
pnpm run check 0 warnings, 0 errors
pnpm test:e2eapp-router 322 / 322 pass (2 flaky on retry, 9 skipped)
pnpm test:e2ecloudflare-workers (Miniflare) 38 / 38 pass

@james-elicx
Copy link
Copy Markdown
Collaborator

/bigbonk 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.

Review

This is a well-executed fix for a real production bug. The root cause analysis is accurate, the fix is minimal and correctly targeted at the right layer (rewriting CSS paths before bundle embedding so all three downstream consumers get fixed at once), and the test coverage is thorough.

What I verified

  • Rewrite correctness: _rewriteCachedFontCssToServedUrls uses split/join which handles both the regex-metacharacter edge case and the multi-occurrence case correctly. The cacheDir prefix from path.join(config.root, ".vinext", "fonts") never has a trailing slash, so the split is exact.
  • Cache hit path: fetchAndCacheFont returns CSS with filesystem paths whether fresh-fetched (line 445 rewrite) or read from cache (line 401 style.css read). The rewrite function handles both since the CSS contains cacheDir as a prefix either way.
  • assetsDir threading: After the first review's feedback, both the transform-time rewrite (line 635) and the writeBundle copy (line 939) read assetsDir from this.environment.config.build.assetsDir, so they cannot diverge. The default fallback only exists for the exported test API.
  • Dev mode is unaffected: The entire injectSelfHostedCss flow is gated behind isBuild (line 834), so dev mode continues using CDN mode.
  • writeBundle scoping: The hook correctly filters to client environment only and skips non-font files.
  • _headers coverage: The existing _headers file generation (line 3687-3695) uses the resolved assetsDir, so the immutable cache-control directive correctly covers /<assetsDir>/_vinext_fonts/.
  • Test design: The separate fixture (font-google-multiple) and mocked fetch are the right call. The mock returns CSS with real https://fonts.gstatic.com/... URLs that trigger fetchAndCacheFont's path-rewrite codepath.

Minor nits (non-blocking)

  1. Stale comment in fetchAndCacheFont (line 444, not in this diff): The existing comment says "Vite will resolve /@fs/ for dev, or asset for build" — this was the incorrect assumption that caused the bug. Now that _rewriteCachedFontCssToServedUrls handles the rewrite explicitly, the comment is misleading. Worth updating to mention the rewrite in a follow-up.

  2. Noted caveat: The author flagged that prod-server.ts hardcodes pathname.startsWith("/assets/") at lines 995/1286/1551, meaning a custom build.assetsDir would 404 for all hashed assets on the Node.js prod server (not just fonts). That's pre-existing and orthogonal to this PR — worth a follow-up issue but not a blocker here. On Cloudflare Workers (the primary target), the asset binding serves the entire dist/client/ tree.

Comment thread tests/app-router.test.ts Outdated
if (typeof Request !== "undefined" && input instanceof Request) return input.url;
return String(input);
};
globalThis.fetch = async (input: unknown) => {
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.

Very minor: the async (input: unknown) signature loses the second init parameter of globalThis.fetch, so the fallback originalFetch(input as RequestInfo) on line 2397 never forwards request options. This doesn't matter for the current test since all intercepted calls are simple GETs and the fallback path is only for non-Google URLs during the build, but if a future test case relies on headers or method in the fallback path, it would silently drop them.

Not blocking — just noting for awareness since the resolveFetchUrl hardening was specifically about future-proofing.

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk bot commented Apr 11, 2026

Review posted successfully. Here's a summary of my review:

Approved the PR. This is a well-executed fix for a real production bug where next/font/google self-hosted font URLs leaked the dev-machine absolute filesystem path into the HTTP Link: header, <link rel="preload"> tags, and <style data-vinext-fonts> block, causing 404s on every production request.

Key points I verified:

  • The split/join rewrite in _rewriteCachedFontCssToServedUrls is correct and handles both fresh-fetch and cache-hit paths
  • The assetsDir is properly threaded through both the transform-time rewrite and the writeBundle copy hook, so they can't diverge
  • Dev mode is unaffected (gated behind isBuild)
  • The writeBundle hook correctly scopes to client environment only
  • The existing _headers immutable cache-control rule covers the new font URL namespace
  • Test coverage is thorough with good design decisions (separate fixture, mocked fetch that exercises the actual bug codepath)

Two non-blocking nits:

  1. The stale comment on line 444 in fetchAndCacheFont ("Vite will resolve /@fs/ for dev, or asset for build") is the incorrect assumption that caused the bug and should be updated
  2. The fetch mock in the integration test drops the init parameter, which could silently lose headers/method if the fallback path is exercised in a future test

github run

…view

Both noted as non-blocking by `ask-bonk`'s approval review; landing
them keeps the review trail tidy and closes the documentation loop so
the next maintainer does not rebuild the incorrect mental model that
caused the original bug.

## 1. Stale comment in `fetchAndCacheFont`

The old comment at the `css.split(fontUrl).join(filePath)` line read

    // Rewrite CSS to use absolute path
    // (Vite will resolve /@fs/ for dev, or asset for build)

which was never true: the CSS ends up as a JavaScript string literal
inside the server bundle, and Vite's asset pipeline only scans real
CSS files and `import`/`new URL(...)` references — not string
literals. That incorrect assumption is exactly what seeded the leak
this PR fixes. Replaced the comment with an explicit description of
the handoff: `fetchAndCacheFont` writes absolute filesystem paths as
the on-disk intermediate form, `_rewriteCachedFontCssToServedUrls()`
rewrites the `cacheDir` prefix to the served URL namespace at embed
time, and the filesystem path must never reach the bundle, the
`<style data-vinext-fonts>` block, the body `<link rel="preload">`
tags, or the HTTP `Link:` header. The new comment ends with a "do not
resurrect that assumption" warning aimed at future maintainers.

## 2. Preserve `fetch(input, init)` signature in the test mock

`globalThis.fetch` accepts `(input, init)` but the integration test's
mock signature was `async (input: unknown) =>`, dropping `init` on
the floor. The build plugin only issues plain GETs for Google Fonts
so the current test does not need the `init` argument in any mock
branch — but the fallback `originalFetch(input as RequestInfo)` at
the bottom silently stripped headers/method/body from any unrelated
request that happened to run during the test and fall through.
Forwarding `init` through costs one extra parameter in the signature
and one extra argument at the fallback call site; it also keeps the
mock consistent with the `resolveFetchUrl` hardening landed in the
previous review round, which was justified as "future-proofing the
mock against a vinext internals change or test helper".

Neither change affects runtime behavior or test outcomes — the first
is a comment-only edit, the second is a 2-line signature-preserving
fix that never fires during the current test. Targeted runs after
the change:

- `pnpm run check` — 0 warnings, 0 errors, all 919 files formatted.
- `pnpm test:unit -t "_rewriteCachedFontCssToServedUrls"` — 7/7 pass
  (unchanged).
- `pnpm test:integration -t "self-hosted next/font/google"` — 4/4
  pass (unchanged).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@Shorebirdmgmt
Copy link
Copy Markdown
Contributor Author

Both non-blocking nits addressed in a5c2b72 to close the review trail cleanly.

1. Stale fetchAndCacheFont comment

Replaced the old Vite will resolve /@fs/ for dev, or asset for build comment — that was the incorrect assumption that seeded this bug in the first place — with an explicit description of the handoff: fetchAndCacheFont writes absolute paths as the on-disk intermediate form, _rewriteCachedFontCssToServedUrls() rewrites the prefix at embed time, and the filesystem path must never reach the bundle, the injected <style data-vinext-fonts> block, the body <link rel="preload"> tags, or the HTTP Link: header. New comment ends with an explicit "do not resurrect that assumption" warning for future maintainers.

2. fetch(input, init) signature preserved in the test mock

The mock signature is now async (input: unknown, init?: RequestInit) => and the fallback forwards init to originalFetch(input as RequestInfo, init). Consistent with the resolveFetchUrl hardening from the previous round — future-proofing the mock against a vinext internals change or test helper that relies on headers/method in the fallback path.

Both changes are surgical (comment-only + 2-line signature fix). Verified:

  • pnpm run check — 0 warnings, 0 errors, all 919 files formatted
  • pnpm test:unit -t "_rewriteCachedFontCssToServedUrls" — 7/7 pass (unchanged)
  • pnpm test:integration -t "self-hosted next/font/google" — 4/4 pass (unchanged)

Thanks for the thorough review — approval is unchanged by these nits, landing them just to keep the codebase tidy and the intent explicit for the next person who touches this pipeline.

@james-elicx
Copy link
Copy Markdown
Collaborator

/bigbonk 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.

Review

This is a clean, well-targeted fix for a real production bug. The root cause analysis is accurate, the fix is minimal and correctly placed at the right layer (CSS rewrite before bundle embedding), and the test coverage is thorough.

What I verified

  • Single source of truth: The rewrite in _rewriteCachedFontCssToServedUrls runs once before JSON.stringify embeds the CSS into the bundle. All three downstream consumers (the <style data-vinext-fonts> block, the <link rel="preload"> tags, and the HTTP Link: header) read from the same rewritten string, so the fix propagates everywhere automatically.
  • assetsDir parity: Both the transform-time rewrite (line 647, passed to the call on line 819-822) and the writeBundle copy (line 951) read assetsDir from this.environment.config.build.assetsDir, so they cannot diverge.
  • Dev mode unaffected: The entire injectSelfHostedCss flow is gated behind isBuild (line 846), and the writeBundle hook only fires during builds.
  • writeBundle scoping: Correctly filters to client environment only (line 942) and skips non-font files via the extension regex (line 967).
  • _headers coverage: The _headers file generation at src/index.ts:3687-3691 uses /${assetsDir}/* which is a wildcard covering the new _vinext_fonts subdirectory.
  • split/join safety: Correct choice over regex — avoids metacharacter issues with filesystem paths (tested explicitly with the build (1) case).
  • Defensive guards: Empty cacheDir guard prevents split-on-empty-string corruption. Empty assetsDir fallback prevents // in URLs.
  • Test design: Separate fixture avoids polluting app-basic. Mocked fetch returns CSS with real https://fonts.gstatic.com/... URLs that exercise the actual bug codepath. The resolveFetchUrl helper and init forwarding are good future-proofing.

Pre-existing note (not blocking)

The author already flagged this: prod-server.ts:505 and static-file-cache.ts:120 both hardcode startsWith("assets/") for the immutable cache-control heuristic. With a custom build.assetsDir, font files (and all other hashed assets) would still serve 200 but with max-age=3600 instead of immutable on the Node.js prod server. On Cloudflare Workers the _headers file handles it correctly. This is orthogonal to this PR — a follow-up issue would be appropriate.

Clean fix, good tests, no concerns. Approving.

stack.push(src);
continue;
}
if (!/\.(woff2?|ttf|otf|eot)$/i.test(entry.name)) continue;
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.

Nit: The extension allowlist here (woff2?|ttf|otf|eot) is correct and covers all standard web font formats. Worth noting that if a future Google Fonts response ever included .svg font references (extremely unlikely — SVG fonts are deprecated), they'd be silently skipped. Not worth changing, just documenting the intent.

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk bot commented Apr 12, 2026

Review posted and approved. Here's a summary of my findings:

Approved. This is a well-executed fix for a production bug where next/font/google self-hosted font URLs leaked the dev-machine absolute filesystem path into the HTTP Link: header, <link rel="preload"> tags, and <style data-vinext-fonts> block, causing 404s on every production request.

Key things I verified:

  • The rewrite runs once before bundle embedding, fixing all three downstream consumers from a single source of truth
  • assetsDir is properly threaded through both the transform-time rewrite and the writeBundle copy hook so they can't diverge
  • Dev mode is unaffected (gated behind isBuild)
  • The writeBundle hook correctly scopes to the client environment only
  • The existing _headers wildcard rule (/${assetsDir}/*) covers the new _vinext_fonts subdirectory
  • split/join over regex is the right choice for filesystem paths with potential metacharacters
  • Test coverage is thorough with good design decisions (separate fixture, mocked fetch exercising the actual bug codepath)

I noted one pre-existing concern (already flagged by the author): prod-server.ts:505 and static-file-cache.ts:120 hardcode startsWith("assets/") for immutable cache-control, which would affect any custom assetsDir user on the Node.js prod server — but that's orthogonal to this PR and works correctly on Cloudflare Workers.

github run

@james-elicx james-elicx merged commit 4f61aed into cloudflare:main Apr 12, 2026
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants