feat: all fixes + self-hosted Google Fonts with Next.js parity#1
Open
feat: all fixes + self-hosted Google Fonts with Next.js parity#1
Conversation
📝 WalkthroughWalkthroughAdds a build-time font metrics module and integrates hashed self-hosted Google Fonts with fallback generation, updates font plugin and loader, adjusts navigation hydration snapshots, patches wrangler deploy config handling, broadens CI publish gate, and replaces the vinext README with full documentation. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
blitss
force-pushed
the
streamloop/all-fixes
branch
from
23b845c to
560752f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/vinext/src/entries/app-rsc-entry.ts (1)
1169-1176:⚠️ Potential issue | 🟠 MajorPopulate named slot entries in
segmentMap.This provider always receives
{ children: childSegs }, souseSelectedLayoutSegments(parallelRoutesKey)can never return anything for named slots. A layout asking for a key like"analytics"or"team"will still see an empty selection, which leaves the new parallel-route support only half wired.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vinext/src/entries/app-rsc-entry.ts` around lines 1169 - 1176, The LayoutSegmentProvider is always given only { children: childSegs }, so named parallel route slots are never populated and useSelectedLayoutSegments(parallelRoutesKey) returns nothing; fix by building a segmentMap that includes entries for each parallel route key: iterate the route.routeSegments (or relevant parallel route descriptors) and call __resolveChildSegments(route.routeSegments[key] || [], treePosForKey, params) per key, assigning the result to segmentMap[key] (with a fallback for "children"), then pass that full segmentMap into createElement(LayoutSegmentProvider, { segmentMap }, element) so named slots like "analytics" or "team" are available to useSelectedLayoutSegments.packages/vinext/src/plugins/fonts.ts (1)
364-393:⚠️ Potential issue | 🟠 MajorWarm-cache runs can emit broken
/fonts/...URLs.This publish step only runs on the cold-fetch path. If
style.cssalready exists, the function returns at Line 331 before any of these copies happen, so a cleanpublic/fontsdirectory can leave the rewritten CSS pointing at files that were never republished.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vinext/src/plugins/fonts.ts` around lines 364 - 393, The rewritten CSS font URL replacement and font file copy currently only run on the cold-fetch path, causing stale style.css to reference missing files on warm-cache runs; update the logic so that after reading or ensuring style.css exists (the early-return at the previous cache-check), you still iterate the urls list and perform the content-hash calculation and copy logic (the loop that references fontDir, publicFontsDir, filePath, hashedFilename and does fs.copyFileSync) and then replace occurrences in the CSS (css = css.split(fontUrl).join(...)) before finishing—i.e., move or extract the loop into a code path that always runs (or call a new helper like ensurePublicFontAndRewriteCss(urls, fontDir, publicFontsDir, css)) so warm-cache runs also produce the hashed files in public/fonts and update style.css URLs.
🧹 Nitpick comments (2)
package.json (1)
4-79: Avoid a second source of truth for pnpm workspace metadata.pnpm's docs put workspace package globs, catalogs, and overrides in
pnpm-workspace.yaml, and this repo already keeps those entries there. That suggests this duplicatedworkspaces/overridesblock inpackage.jsonis only advisory for other tooling, so it can drift from the configuration pnpm actually resolves from. (pnpm.io)Also applies to: 116-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 4 - 79, The package.json contains a duplicated pnpm workspace metadata block (workspaces -> packages and catalog/overrides) which creates a second source of truth; remove the redundant "workspaces" object (including "packages" and "catalog"/"overrides" entries) from package.json and ensure the canonical configuration remains only in pnpm-workspace.yaml so pnpm resolves workspace globs and overrides from the single source; update or run any tooling that relied on package.json advisory entries to read pnpm-workspace.yaml if needed.tests/prerender.test.ts (1)
832-856: Reduce duplicate parent-resolution cases to keep this suite maintainable.This block repeats scenarios already covered above (same two-level chain and expectations). Consider collapsing repeated setups into a table-driven test set.
♻️ Suggested consolidation pattern
+const parentResolutionCases = [ + { + name: "single parent segment", + child: "/shop/:category/:item", + map: { "/shop/:category": async () => [{ category: "electronics" }, { category: "clothing" }] }, + expected: [{ category: "electronics" }, { category: "clothing" }], + }, + { + name: "two parent levels", + child: "/a/:b/c/:d/:e", + map: { + "/a/:b": async () => [{ b: "1" }, { b: "2" }], + "/a/:b/c/:d": async ({ params }) => (params.b === "1" ? [{ d: "x" }] : [{ d: "y" }, { d: "z" }]), + }, + expected: [{ b: "1", d: "x" }, { b: "2", d: "y" }, { b: "2", d: "z" }], + }, +]; + +for (const c of parentResolutionCases) { + it(`resolves parent params: ${c.name}`, async () => { + const result = await resolveParentParams(mockRoute(c.child), c.map as StaticParamsMap); + expect(result).toEqual(c.expected); + }); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/prerender.test.ts` around lines 832 - 856, The two test cases duplicate existing parent-resolution scenarios; consolidate by replacing the repeated blocks with a single table-driven test that enumerates cases and expected outputs and calls resolveParentParams with mockRoute and StaticParamsMap per case (use Jest's test.each or a for-loop). Locate the duplicate tests referencing resolveParentParams, mockRoute, and StaticParamsMap and merge them into one parametrized test array of inputs/expected results to remove redundancy while preserving both the "layout-level generateStaticParams (no pagePath)" and "two levels of layout-only parent segments" scenarios.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/preview-release.yml:
- Around line 13-14: The current workflow condition only checks repository owner
and therefore still runs for forked PRs; update the job's if condition (the same
guarded block that references pkg-pr-new in the workflow) to additionally detect
and skip forked pull requests by requiring either that this is not a
pull_request event or that github.event.pull_request.head.repo.fork == false; in
practice replace the existing if with a combined expression that keeps the owner
check (github.repository_owner == 'cloudflare' || github.repository_owner ==
'streamloop') AND also requires (github.event_name != 'pull_request' ||
github.event.pull_request.head.repo.fork == false) so fork PRs are excluded.
In `@packages/vinext/README.md`:
- Around line 691-693: The README's local setup and CI guidance are still
specific to the upstream Cloudflare project (e.g., the git clone instruction and
the "Cloudflare employees" CI note); update the contributor guidance to point to
this fork's repository URL in the clone command and replace any
Cloudflare-specific preview/CI instructions with fork-appropriate steps and
terminology (update the clone command, local setup steps, and the
CI/preview-flow paragraphs referenced around the current git clone lines and the
"Cloudflare employees" wording). Ensure the rewritten text clearly instructs
contributors to clone this fork, run pnpm install, and follow the fork's
CI/preview flow (including any required repo names, branch naming, or PR preview
URLs) so the instructions are accurate for this repository.
In `@packages/vinext/src/cloudflare/tpr.ts`:
- Around line 877-883: The current code silently continues when BUILD_ID is
missing which reintroduces the dead-key issue; change the BUILD_ID read block
(the buildId variable set by fs.readFileSync(... "dist/server/BUILD_ID")) to
fail fast — if reading/buildId.trim() throws or is empty/undefined, immediately
abort the TPR seeding path (return or throw) instead of proceeding; ensure the
callers that perform the TPR upload/seed (the code that uses buildId to prefix
keys, and any function like the TPR seeding/upload routine) are not executed
when buildId is missing so no entries are uploaded with a mismatched key format.
In `@packages/vinext/src/entries/app-rsc-entry.ts`:
- Around line 1328-1335: The current loop over routes (the block using
r.isDynamic, r.pagePath, entries.push and emittedPatterns) deduplicates
generateStaticParams by raw r.pattern which causes a layout's
generateStaticParams to be dropped when a leaf page shares the same pattern;
change the emitted key to include the segment kind (e.g., distinguish page vs
layout or use r.kind/r.segmentKind) so that emittedPatterns tracks a composite
key like `${r.pattern}::${r.kind}` (and use the same composite key in the other
similar block that emits layout entries) and update the checks that add/check
emittedPatterns to use that composite key so page and layout entries are not
conflated.
In `@packages/vinext/src/plugins/fonts.ts`:
- Around line 616-618: The import regex importRe currently uses ([^;\n]+?) which
stops at newlines and misses multiline imports like import { Inter, } from
"next/font/google"; update importRe to allow newlines by replacing that capture
with a dot-all style pattern (e.g. ([\s\S]+?) or [^;]+?) so multiline import
specifiers are matched; keep the rest of the regex and the while ((importMatch =
importRe.exec(code)) !== null) loop unchanged so virtual-module rewrite and
self-hosted option injection run for those files as well.
In `@packages/vinext/src/shims/font-google.ts`:
- Around line 138-155: The style-axis builder currently emits both regular and
italic tuples and omits the ital axis when only italic is requested; update the
logic that builds the axis (the block using variables weights, styles, hasItalic
and mutating spec) so that: when styles includes only "italic" and
weights.length > 0 you generate only "1,{weight}" tuples (e.g.
ital,wght@1,400;1,700) instead of both "0,{w}" and "1,{w}"; and when styles
includes only "italic" and weights.length === 0 append the axis suffix ":ital@1"
to spec so the API explicitly requests italic-only variable axis. Make the same
changes in the mirrored builder in the plugins/fonts.ts file (the equivalent
spec/hasItalic/weights/styles logic).
In `@packages/vinext/src/shims/font-local.ts`:
- Around line 142-171: The code currently ignores options.preload in both dev
and self-hosted/build paths; update the logic so collectFontPreloads(...) is
only called when options.preload !== false and ensure the build/self-hosted
branch (within the block using options._selfHostedCSS/options._hashedFamily and
the return that sets className/style/variable) also conditions any preload
registration on options.preload !== false before calling collectFontPreloads or
emitting preload metadata; change calls around
injectSelfHostedCSS/injectVariableClassRule and any dev-path registration to be
guarded by options.preload !== false so setting preload: false prevents preload
links.
- Around line 175-177: The configKey currently hashes raw options and omits
normalized declarations and the normalized fallback array, which can make
distinct localFont() outputs collide; update the logic that builds configKey
(used by simpleHash) to use the normalized output that will actually be emitted
— include the normalized declarations and the normalized fallback (the same
array/string used to generate fontFamily/class names) and any other
normalization steps (e.g., computed srcKey) so simpleHash hashes the final
normalized representation rather than the raw options; adjust the code around
srcKey, configKey, and the call to simpleHash to reference those normalized
values.
In `@packages/vinext/src/shims/font-utils.ts`:
- Around line 134-145: The module-level sharedRootVariables Set and the
Symbol.for(...) singleton are keeping font SSR emission state across requests,
causing :root rules, ssrFontStyles and ssrFontPreloads to leak between renders;
change the design so FontSSRState is created per render (e.g., add a
createFontSSRState factory and stop using the module-level sharedRootVariables
and the Symbol.for(...) global singleton), pass that per-render FontSSRState
into functions that currently read
sharedRootVariables/ssrFontStyles/ssrFontPreloads, and remove or replace the
Symbol.for(...) lookup with the per-request instance so all
injectedFonts/injectedClassRules/injectedVariableRules/injectedSelfHosted/ssrFontStyles
are scoped to a single render.
In `@tests/prerender.test.ts`:
- Around line 875-876: The tests use functions in the StaticParamsMap that
sometimes return null but are currently cast to Promise<Record<string, string |
string[]>[]>, so update the StaticParamsMap function signature to explicitly
allow null by changing the return type to Promise<Record<string, string |
string[]>[] | null>; adjust any mapped entries like the "/a/:b" async function
(and other entries referenced) to conform to the new type so null pass-through
is typed safely.
---
Outside diff comments:
In `@packages/vinext/src/entries/app-rsc-entry.ts`:
- Around line 1169-1176: The LayoutSegmentProvider is always given only {
children: childSegs }, so named parallel route slots are never populated and
useSelectedLayoutSegments(parallelRoutesKey) returns nothing; fix by building a
segmentMap that includes entries for each parallel route key: iterate the
route.routeSegments (or relevant parallel route descriptors) and call
__resolveChildSegments(route.routeSegments[key] || [], treePosForKey, params)
per key, assigning the result to segmentMap[key] (with a fallback for
"children"), then pass that full segmentMap into
createElement(LayoutSegmentProvider, { segmentMap }, element) so named slots
like "analytics" or "team" are available to useSelectedLayoutSegments.
In `@packages/vinext/src/plugins/fonts.ts`:
- Around line 364-393: The rewritten CSS font URL replacement and font file copy
currently only run on the cold-fetch path, causing stale style.css to reference
missing files on warm-cache runs; update the logic so that after reading or
ensuring style.css exists (the early-return at the previous cache-check), you
still iterate the urls list and perform the content-hash calculation and copy
logic (the loop that references fontDir, publicFontsDir, filePath,
hashedFilename and does fs.copyFileSync) and then replace occurrences in the CSS
(css = css.split(fontUrl).join(...)) before finishing—i.e., move or extract the
loop into a code path that always runs (or call a new helper like
ensurePublicFontAndRewriteCss(urls, fontDir, publicFontsDir, css)) so warm-cache
runs also produce the hashed files in public/fonts and update style.css URLs.
---
Nitpick comments:
In `@package.json`:
- Around line 4-79: The package.json contains a duplicated pnpm workspace
metadata block (workspaces -> packages and catalog/overrides) which creates a
second source of truth; remove the redundant "workspaces" object (including
"packages" and "catalog"/"overrides" entries) from package.json and ensure the
canonical configuration remains only in pnpm-workspace.yaml so pnpm resolves
workspace globs and overrides from the single source; update or run any tooling
that relied on package.json advisory entries to read pnpm-workspace.yaml if
needed.
In `@tests/prerender.test.ts`:
- Around line 832-856: The two test cases duplicate existing parent-resolution
scenarios; consolidate by replacing the repeated blocks with a single
table-driven test that enumerates cases and expected outputs and calls
resolveParentParams with mockRoute and StaticParamsMap per case (use Jest's
test.each or a for-loop). Locate the duplicate tests referencing
resolveParentParams, mockRoute, and StaticParamsMap and merge them into one
parametrized test array of inputs/expected results to remove redundancy while
preserving both the "layout-level generateStaticParams (no pagePath)" and "two
levels of layout-only parent segments" scenarios.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3fa9d50-191e-49e6-ad57-012a74bc668d
⛔ Files ignored due to path filters (3)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamltests/__snapshots__/entry-templates.test.ts.snapis excluded by!**/*.snaptests/fixtures/fonts/inter-latin-400.woff2is excluded by!**/*.woff2
📒 Files selected for processing (36)
.agents/skills/migrate-to-vinext/SKILL.md.agents/skills/migrate-to-vinext/references/compatibility.md.github/workflows/preview-release.ymlREADME.mdpackage.jsonpackages/vinext/README.mdpackages/vinext/package.jsonpackages/vinext/src/build/prerender.tspackages/vinext/src/check.tspackages/vinext/src/cloudflare/tpr.tspackages/vinext/src/entries/app-rsc-entry.tspackages/vinext/src/font-metrics.tspackages/vinext/src/index.tspackages/vinext/src/plugins/fonts.tspackages/vinext/src/server/app-page-boundary-render.tspackages/vinext/src/server/app-page-boundary.tspackages/vinext/src/server/app-ssr-entry.tspackages/vinext/src/shims/font-google-base.tspackages/vinext/src/shims/font-google.tspackages/vinext/src/shims/font-local.tspackages/vinext/src/shims/font-utils.tspackages/vinext/src/shims/layout-segment-context.tsxpackages/vinext/src/shims/navigation.tspnpm-workspace.yamltests/app-page-boundary.test.tstests/app-router.test.tstests/check.test.tstests/fixtures/app-basic/app/dashboard/segment-display.tsxtests/fixtures/fonts/LICENSEtests/font-google.test.tstests/font-local-transform.test.tstests/font-metrics.test.tstests/font-utils.test.tstests/prerender.test.tstests/shims.test.tstests/tpr-kv-keys.test.ts
💤 Files with no reviewable changes (1)
- packages/vinext/src/shims/font-google-base.ts
Comment on lines
13
to
+14
| # Skip for forks — they don't have write access needed by pkg-pr-new. | ||
| if: github.repository_owner == 'cloudflare' | ||
| if: github.repository_owner == 'cloudflare' || github.repository_owner == 'streamloop' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "preview-release.yml" -type fRepository: streamloop/vinext
Length of output: 100
🏁 Script executed:
cat -n .github/workflows/preview-release.ymlRepository: streamloop/vinext
Length of output: 696
🌐 Web query:
GitHub Actions pull_request event fork head.repo.full_name documentation
💡 Result:
In GitHub Actions workflows triggered by the pull_request event, the full name of the fork repository (head repository) is available in the github context as github.event.pull_request.head.repo.full_name. This is a string in the format "owner/repo" for the repository containing the head branch of the pull request. This property is particularly useful for distinguishing pull requests from forks. For example, to skip a job if the PR originates from a fork: jobs: example: if: github.event.pull_request.head.repo.full_name == github.repository runs-on: ubuntu-latest steps: - ... The github.event context mirrors the webhook payload for the pull_request event, where head.repo is a repository object containing full_name, fork (boolean), name, etc. Official GitHub documentation confirms github.event is the full event webhook payload, and community/official action docs reference this path explicitly for fork detection in pull_request events. Note: This is only populated for pull_request and pull_request_target events. For forks, head.repo.full_name differs from github.repository (the base repo).
Citations:
- 1: https://raw.githubusercontent.com/peter-evans/create-pull-request/main/docs/concepts-guidelines.md
- 2: https://stackoverflow.com/questions/70775445/determining-the-origin-repo-of-pull-request-in-workflow-yaml
- 3: https://docs.github.com/en/actions/reference/contexts-reference
- 4: https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/contexts
Skip fork PRs explicitly, not just non-owned repos.
For pull_request events, GitHub sends fork PR events to the base repository. The condition github.repository_owner == 'cloudflare' || github.repository_owner == 'streamloop' will always evaluate to true for PRs to this repository regardless of fork origin, because github.repository_owner refers to the base repository owner, not the PR's source. This means fork PRs still enter the publish job, contradicting the Line 13 comment intent.
To properly skip fork PRs, guard the job with both an owner check and a fork detection:
Suggested fix
- if: github.repository_owner == 'cloudflare' || github.repository_owner == 'streamloop'
+ if: >
+ (github.repository_owner == 'cloudflare' || github.repository_owner == 'streamloop') &&
+ (
+ github.event_name != 'pull_request' ||
+ github.event.pull_request.head.repo.full_name == github.repository
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Skip for forks — they don't have write access needed by pkg-pr-new. | |
| if: github.repository_owner == 'cloudflare' | |
| if: github.repository_owner == 'cloudflare' || github.repository_owner == 'streamloop' | |
| # Skip for forks — they don't have write access needed by pkg-pr-new. | |
| if: > | |
| (github.repository_owner == 'cloudflare' || github.repository_owner == 'streamloop') && | |
| ( | |
| github.event_name != 'pull_request' || | |
| github.event.pull_request.head.repo.full_name == github.repository | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/preview-release.yml around lines 13 - 14, The current
workflow condition only checks repository owner and therefore still runs for
forked PRs; update the job's if condition (the same guarded block that
references pkg-pr-new in the workflow) to additionally detect and skip forked
pull requests by requiring either that this is not a pull_request event or that
github.event.pull_request.head.repo.fork == false; in practice replace the
existing if with a combined expression that keeps the owner check
(github.repository_owner == 'cloudflare' || github.repository_owner ==
'streamloop') AND also requires (github.event_name != 'pull_request' ||
github.event.pull_request.head.repo.fork == false) so fork PRs are excluded.
Comment on lines
+1328
to
+1335
| // 1. Page-level entries (leaf pages with generateStaticParams) | ||
| for (const r of routes) { | ||
| if (r.isDynamic && r.pagePath) { | ||
| entries.push( | ||
| ` ${JSON.stringify(r.pattern)}: ${getImportVar(r.pagePath)}?.generateStaticParams ?? null,`, | ||
| ); | ||
| emittedPatterns.add(r.pattern); | ||
| } |
There was a problem hiding this comment.
Don't dedupe page and layout generateStaticParams() by raw pattern.
A dynamic layout and its leaf page can legitimately share the same URL pattern, for example app/blog/[slug]/layout.tsx and app/blog/[slug]/page.tsx. Because page entries are emitted first and emittedPatterns drops the duplicate, descendant routes under that segment will resolve parent params from the page export instead of the layout export. This map needs to distinguish segment kind, not just path.
Also applies to: 1376-1382
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/vinext/src/entries/app-rsc-entry.ts` around lines 1328 - 1335, The
current loop over routes (the block using r.isDynamic, r.pagePath, entries.push
and emittedPatterns) deduplicates generateStaticParams by raw r.pattern which
causes a layout's generateStaticParams to be dropped when a leaf page shares the
same pattern; change the emitted key to include the segment kind (e.g.,
distinguish page vs layout or use r.kind/r.segmentKind) so that emittedPatterns
tracks a composite key like `${r.pattern}::${r.kind}` (and use the same
composite key in the other similar block that emits layout entries) and update
the checks that add/check emittedPatterns to use that composite key so page and
layout entries are not conflated.
Comment on lines
+616
to
618
| const importRe = /^[ \t]*import\s+([^;\n]+?)\s+from\s*(["'])next\/font\/google\2\s*;?/gm; | ||
| let importMatch; | ||
| while ((importMatch = importRe.exec(code)) !== null) { |
There was a problem hiding this comment.
Multiline next/font/google imports still bypass this transform.
([^;\n]+?) stops at the first newline, so the common formatted form import {\n Inter,\n} from "next/font/google" never matches. Those files won't get the virtual-module rewrite or the self-hosted option injection.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/vinext/src/plugins/fonts.ts` around lines 616 - 618, The import
regex importRe currently uses ([^;\n]+?) which stops at newlines and misses
multiline imports like import { Inter, } from "next/font/google"; update
importRe to allow newlines by replacing that capture with a dot-all style
pattern (e.g. ([\s\S]+?) or [^;]+?) so multiline import specifiers are matched;
keep the rest of the regex and the while ((importMatch = importRe.exec(code))
!== null) loop unchanged so virtual-module rewrite and self-hosted option
injection run for those files as well.
Comment on lines
+191
to
+197
| // Inject self-hosted @font-face CSS (includes hashed font-family names) | ||
| injectSelfHostedCSS(state, options._selfHostedCSS); | ||
|
|
||
| // Inject fallback @font-face CSS (with size-adjust metrics) | ||
| if (options._fallbackCSS) { | ||
| injectSelfHostedCSS(state, options._fallbackCSS); | ||
| } |
There was a problem hiding this comment.
Build mode ignores the subset-filtered preload list.
This path never reads options._preloadUrls or options.preload; it only hands the full stylesheet to injectSelfHostedCSS(). Based on the helper behavior covered in tests/font-utils.test.ts, that will collect every absolute .woff2 in the sheet, so primary-subset filtering is lost and preload: false still emits SSR preloads.
Comment on lines
+142
to
+171
| // --- Build mode: use plugin-injected metadata --- | ||
| if (options._selfHostedCSS && options._hashedFamily) { | ||
| const hashedFamily = options._hashedFamily; | ||
| const fallbackFamily = options._fallbackFamily; | ||
| const className = options._className ?? `__className_${simpleHash(options._selfHostedCSS)}`; | ||
| const variableClassName = | ||
| options._variableClassName ?? `__variable_${simpleHash(options._selfHostedCSS)}`; | ||
|
|
||
| const parts: string[] = [`'${escapeCSSString(hashedFamily)}'`]; | ||
| if (fallbackFamily) { | ||
| parts.push(`'${escapeCSSString(fallbackFamily)}'`); | ||
| } | ||
| parts.push(...fallback.map(sanitizeFallback)); | ||
| const fontFamily = parts.join(", "); | ||
|
|
||
| // Only set the CSS variable — do NOT set font-family. | ||
| // This matches Next.js behavior where .variable classes only define CSS variables. | ||
| let css = `.${variableClassName} { ${cssVarName}: ${fontFamily}; }\n`; | ||
| injectSelfHostedCSS(state, options._selfHostedCSS); | ||
| if (options._fallbackCSS) { | ||
| injectSelfHostedCSS(state, options._fallbackCSS); | ||
| } | ||
|
|
||
| // Also inject at :root so CSS variable inheritance works throughout the page. | ||
| // This ensures Tailwind utilities like `font-sans` that reference these | ||
| // variables via var(--font-geist-sans) work correctly. | ||
| if (!injectedRootVariables.has(cssVarName)) { | ||
| injectedRootVariables.add(cssVarName); | ||
| css += `:root { ${cssVarName}: ${fontFamily}; }\n`; | ||
| } | ||
| injectClassNameRule(state, className, fontFamily); | ||
| if (cssVarName) { | ||
| injectVariableClassRule(state, variableClassName, cssVarName, fontFamily); | ||
| } | ||
|
|
||
| // On server, store the CSS for SSR injection | ||
| if (typeof document === "undefined") { | ||
| ssrFontStyles.push(css); | ||
| return; | ||
| return { | ||
| className, | ||
| style: { fontFamily }, | ||
| ...(cssVarName ? { variable: variableClassName } : {}), | ||
| }; |
There was a problem hiding this comment.
Honor options.preload in both code paths.
The dev path always registers the sources with collectFontPreloads(...), so preload: false still emits preload links. The build/self-hosted path never consults the flag either, which means this public option has no effect here.
Suggested fix
export default function localFont(options: LocalFontOptions & BuildInjectedOptions): FontResult {
const fallback = options.fallback ?? ["sans-serif"];
const cssVarName = options.variable ? sanitizeCSSVarName(options.variable) : undefined;
+ const preloadPaths = normalizeSources(options).map((source) => source.path);
// --- Build mode: use plugin-injected metadata ---
if (options._selfHostedCSS && options._hashedFamily) {
+ if (options.preload !== false) {
+ collectFontPreloads(state, preloadPaths);
+ }
+
const hashedFamily = options._hashedFamily;
const fallbackFamily = options._fallbackFamily;
const className = options._className ?? `__className_${simpleHash(options._selfHostedCSS)}`;
const variableClassName =
options._variableClassName ?? `__variable_${simpleHash(options._selfHostedCSS)}`;
@@
- collectFontPreloads(
- state,
- normalizeSources(options).map((s) => s.path),
- );
+ if (options.preload !== false) {
+ collectFontPreloads(state, preloadPaths);
+ }Also applies to: 184-187
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/vinext/src/shims/font-local.ts` around lines 142 - 171, The code
currently ignores options.preload in both dev and self-hosted/build paths;
update the logic so collectFontPreloads(...) is only called when options.preload
!== false and ensure the build/self-hosted branch (within the block using
options._selfHostedCSS/options._hashedFamily and the return that sets
className/style/variable) also conditions any preload registration on
options.preload !== false before calling collectFontPreloads or emitting preload
metadata; change calls around injectSelfHostedCSS/injectVariableClassRule and
any dev-path registration to be guarded by options.preload !== false so setting
preload: false prevents preload links.
Comment on lines
+134
to
+145
| const sharedRootVariables = new Set<string>(); | ||
|
|
||
| /** | ||
| * SSR state container for font modules. Each font module (google/local) | ||
| * maintains its own instance to allow consumers to collect styles separately. | ||
| */ | ||
| export type FontSSRState = { | ||
| injectedFonts: Set<string>; | ||
| injectedClassRules: Set<string>; | ||
| injectedVariableRules: Set<string>; | ||
| injectedSelfHosted: Set<string>; | ||
| ssrFontStyles: string[]; |
There was a problem hiding this comment.
Keep font SSR emission state scoped to a render, not the process.
sharedRootVariables and the Symbol.for(...) singleton both outlive a single SSR render, but they decide which :root rules, styles, and preloads get emitted for the current response. After the first request, later renders can skip required :root declarations, and overlapping renders can also mix ssrFontStyles / ssrFontPreloads between requests.
Also applies to: 158-173, 285-288
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/vinext/src/shims/font-utils.ts` around lines 134 - 145, The
module-level sharedRootVariables Set and the Symbol.for(...) singleton are
keeping font SSR emission state across requests, causing :root rules,
ssrFontStyles and ssrFontPreloads to leak between renders; change the design so
FontSSRState is created per render (e.g., add a createFontSSRState factory and
stop using the module-level sharedRootVariables and the Symbol.for(...) global
singleton), pass that per-render FontSSRState into functions that currently read
sharedRootVariables/ssrFontStyles/ssrFontPreloads, and remove or replace the
Symbol.for(...) lookup with the per-request instance so all
injectedFonts/injectedClassRules/injectedVariableRules/injectedSelfHosted/ssrFontStyles
are scoped to a single render.
tests/prerender.test.ts
Outdated
Comment on lines
+875
to
+876
| "/a/:b": async () => null as unknown as Record<string, string | string[]>[], | ||
| "/a/:b/c/:d": async () => [{ d: "x" }], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== StaticParamsMap definition =="
rg -n "type StaticParamsMap|interface StaticParamsMap" packages tests
echo
echo "== null double-assertions in prerender tests =="
rg -n "null as unknown as Record<string, string \\| string\\[]>\\[\\]" tests/prerender.test.ts
echo
echo "== resolveParentParams signature and usages =="
rg -n "resolveParentParams\\s*\\(" packages/vinext/src/build/prerender.ts tests/prerender.test.tsRepository: streamloop/vinext
Length of output: 1784
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== StaticParamsMap definition =="
rg -n "type StaticParamsMap|interface StaticParamsMap" packages tests
echo
echo "== null double-assertions in prerender tests =="
rg -n "null as unknown as Record<string, string \\| string\\[]>\\[\\]" tests/prerender.test.ts
echo
echo "== Lines 875-876, 907-908, 917-919 in prerender.test.ts =="
sed -n '870,880p; 900,920p' tests/prerender.test.ts
echo
echo "== resolveParentParams signature =="
rg -n "function resolveParentParams|const resolveParentParams|resolveParentParams\s*\(" packages/vinext/src/build/prerender.ts -A 3Repository: streamloop/vinext
Length of output: 2773
🏁 Script executed:
sed -n '249,260p' packages/vinext/src/build/prerender.tsRepository: streamloop/vinext
Length of output: 391
Widen the StaticParamsMap function return type to explicitly include null.
The tests currently cast async () => null to satisfy Promise<Record<string, string | string[]>[]>, masking the fact that null-returning functions are legitimate (e.g., CF Workers Proxy scenarios where a route has no generateStaticParams). Update the function signature within StaticParamsMap to return Promise<Record<string, string | string[]>[] | null> so null pass-through behavior is explicit and type-safe.
Affected lines: 875, 907, 917–918.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/prerender.test.ts` around lines 875 - 876, The tests use functions in
the StaticParamsMap that sometimes return null but are currently cast to
Promise<Record<string, string | string[]>[]>, so update the StaticParamsMap
function signature to explicitly allow null by changing the return type to
Promise<Record<string, string | string[]>[] | null>; adjust any mapped entries
like the "/a/:b" async function (and other entries referenced) to conform to the
new type so null pass-through is typed safely.
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/vinext/src/shims/navigation.ts (1)
586-590:⚠️ Potential issue | 🔴 CriticalKeep
navigateClientSideexported untilLinkis migrated.
packages/vinext/src/shims/link.tsx:404-412still delegates tonavigateClientSide(...). Removing that export here breaks theLinkshim immediately.Minimal compatibility shim
+export function navigateClientSide( + href: string, + mode: "push" | "replace", + scroll: boolean, +): Promise<void> { + return navigateImpl(href, mode, scroll); +} + async function navigateImpl(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vinext/src/shims/navigation.ts` around lines 586 - 590, The export for navigateClientSide was removed but packages/vinext/src/shims/link.tsx (the Link shim) still calls navigateClientSide, so restore a compatible export to avoid breaking Link; re-export or reintroduce a navigateClientSide function that delegates to the existing navigateImpl (matching its signature: async function navigateImpl(href: string, mode: "push" | "replace", scroll: boolean): Promise<void>) and keep the exported name navigateClientSide until Link is fully migrated.README.md (1)
43-47:⚠️ Potential issue | 🟡 MinorThe manual command examples assume a global
vinextbinary.After
npm install vinext,vinext dev/build/deploywill not be on$PATHin a plain shell. These examples should usenpx vinext ...or point readers back to the npm scripts declared just above, otherwise the quick-start path fails immediately.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 43 - 47, The README's command examples assume a global vinext binary; change the examples shown (the three-line snippet under the quick-start) to either prefix commands with npx (e.g., "npx vinext dev/build/deploy") or add a short note pointing readers to use the npm scripts declared above (e.g., "npm run dev/build/deploy") so the commands work for users who installed vinext locally via npm.packages/vinext/src/check.ts (1)
141-155:⚠️ Potential issue | 🟡 MinorNamed type-only imports are still being counted as runtime usage.
The skip logic only matches
import type ..., soimport { type Metadata } from "next"still lands inimportUsage. That inflates the compatibility report for common Next.js typing-only imports even though they are erased at compile time.
♻️ Duplicate comments (3)
packages/vinext/src/shims/font-google.ts (2)
169-189:⚠️ Potential issue | 🟠 Major
preload: falsestill cannot work through the self-hosted path.This branch always feeds the full self-hosted stylesheet into
injectSelfHostedCSS()and never checksoptions.preload. That means SSR preloads are still inferred from everywoff2in the sheet, sopreload: falseand any subset-filtered preload list are lost here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vinext/src/shims/font-google.ts` around lines 169 - 189, The self-hosted branch always calls injectSelfHostedCSS with the full styles (using options._selfHostedCSS and options._fallbackCSS) regardless of options.preload, which causes SSR to infer preloads from all woff2 entries; update the logic around injectSelfHostedCSS so it honors options.preload: when options.preload === false, do not inject the full stylesheet (skip injectSelfHostedCSS) or instead pass a filtered CSS string that contains only the rules/resources you intend to preload (e.g., strip woff2 `@font-face` rules) before calling injectSelfHostedCSS; touch the block that references options._selfHostedCSS, options._fallbackCSS and the call to injectSelfHostedCSS to implement this conditional/filtering behavior.
126-150:⚠️ Potential issue | 🟠 MajorStyle-only Google Fonts requests still build the wrong CSS2 spec.
styleis only consulted inside theweights.length > 0branch. That meansstyle: ["italic"]with no explicit weight still emits justfamily=Foo, and italic-only + weights still produces both0,wand1,wtuples. In dev/CDN mode that fetches the wrong faces.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vinext/src/shims/font-google.ts` around lines 126 - 150, The code only checks styles when weights.length > 0, causing style-only requests to omit the ital axis; fix by moving the hasItalic check out of that branch and handling the case where styles includes "italic" but weights is empty: when weights.length === 0 and styles.includes("italic"), treat weights as the full range (100..900) and build the spec using the same ital,wght logic (use hasItalic, pairs, and spec += `:ital,wght@...`) so style-only requests produce the correct CSS2 spec; update the logic around weights, styles, hasItalic, and spec construction accordingly.packages/vinext/src/entries/app-rsc-entry.ts (1)
1324-1387:⚠️ Potential issue | 🟠 MajorA single
pattern -> fnmap still conflates page- and layout-levelgenerateStaticParams().
packages/vinext/src/build/prerender.tsreads the same map for bothstaticParamsMap[route.pattern]and parent-prefix lookups inresolveParentParams(). When a dynamic layout and its leaf page share/blog/:slug, this object cannot represent both exports: the currentemittedPatternsbranch hides the layout export behind the page/null entry, and simply relaxing that dedupe would make the later layout overwrite the page export. This needs separate page/layout storage, or a structured value plus matching lookup changes in prerender.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vinext/src/entries/app-rsc-entry.ts` around lines 1324 - 1387, The current emitted map conflates page- and layout-level generateStaticParams because entries builds a single pattern->fn string (using emittedPatterns, getImportVar, and entries) which causes layout exports to be hidden/overwritten by page entries; split storage so both can coexist: either emit two distinct maps (e.g., pageStaticParamsMap and layoutStaticParamsMap) when building entries (push page entries into page map and layout entries into layout map) or emit a structured value for each pattern like { page: <fn>|null, layout: <fn>|null }; then update packages/vinext/src/build/prerender.ts to read the new shapes (replace direct staticParamsMap[route.pattern] lookups and resolveParentParams parent-prefix lookups to check page vs layout slots or the two maps accordingly) and remove the emittedPatterns dedupe logic that hides the other export (adjust usages of emittedPatterns/emitted list as needed).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/vinext/src/check.ts`:
- Around line 311-321: The scan assumes middleware/proxy live only in repo root
and uses POSIX "/api/" substrings, causing misses on projects with src/
structure or on Windows; update the hasProxy/hasMiddleware checks to look for
both root and src locations (e.g., check path.join(root, "middleware.ts") and
path.join(root, "src", "middleware.ts") and same for proxy) and change the
pages/api split logic (the pagesDir/findSourceFiles filtering that builds pages
and apiRoutes) to use platform-safe path operations (e.g., normalize paths and
check for path separators or use path.relative and split on path.sep to detect
an "api" segment) so middleware/proxy are detected whether under root or src and
API routes are correctly identified on Windows and POSIX.
- Around line 38-61: IMPORT_SUPPORT is missing entries for several shipped
next/* shims so imports like next/form, next/legacy/image, next/constants, and
next/web-vitals are treated as unsupported; update the IMPORT_SUPPORT record to
include keys "next/form", "next/legacy/image", "next/constants", and
"next/web-vitals" with the appropriate status ("supported" or "stubbed") and
short detail strings matching the documented behavior in
.agents/skills/migrate-to-vinext/references/compatibility.md and README.md so
vinext check no longer false-flags these imports—modify the IMPORT_SUPPORT
constant in packages/vinext/src/check.ts to add those entries.
In `@packages/vinext/src/shims/font-utils.ts`:
- Around line 276-289: The docstring for injectFontStylesheet is misleading;
update it to state that the function marks the URL in state.injectedFonts for
deduplication (so callers can rely on that for SSR) and only injects a <link>
tag when running in a browser (when document is defined); reference
injectFontStylesheet and state.injectedFonts and note that callers in
font-google.ts should not need separate tracking after this change (or remove
their duplicate tracking if you standardize on this function).
In `@packages/vinext/src/shims/navigation.ts`:
- Around line 296-316: The issue is that prefetch() returns early when
getPrefetchedUrls().has(rscUrl) without checking expiry, so stale entries never
age out unless the cache is full; fix by, in the prefetch flow where you check
prefetched.has(rscUrl), also look up the corresponding entry in cache (using
rscUrl) and compare its timestamp against PREFETCH_CACHE_TTL and now, and if
expired remove it from both cache and getPrefetchedUrls() and continue with the
prefetch; keep existing sweep-and-FIFO logic for capacity but ensure the
early-return path verifies and evicts expired entries first (references:
prefetch(), rscUrl, cache, getPrefetchedUrls(), MAX_PREFETCH_CACHE_SIZE,
PREFETCH_CACHE_TTL).
- Around line 319-325: setClientParams stores params on globalThis but
_listeners is module-scoped, so in duplicated Vite modules updates only notify
listeners in that module; change listener storage to a single shared set on
globalThis (e.g., attach a unique symbol or property like
globalThis.__vinext_navigation_listeners) and have notifyListeners, add/remove
listener logic (used by useParams) read/modify that global set instead of the
local _listeners; update the other listener blocks referenced (lines ~373-390
and ~406-410) to use the same global listener container so all module instances
are notified when setClientParams runs.
- Around line 245-247: The browser entry still uses the removed RSC helpers
(CachedRscResponse, consumePrefetchResponse, restoreRscResponse,
snapshotRscResponse); update it to the new PrefetchCacheEntry shape ({ response,
timestamp }) by replacing imports/usages of those helpers with direct access to
the PrefetchCacheEntry.response and .timestamp, adapt any code that previously
called consumePrefetchResponse/restoreRscResponse to operate on the Response
object (e.g., read/clone the Response or rehydrate streaming body as needed),
and remove snapshotRscResponse-related logic in favor of storing/restoring the {
response, timestamp } object; search for the identifiers CachedRscResponse,
consumePrefetchResponse, restoreRscResponse, snapshotRscResponse in the browser
entry and refactor those code paths to use PrefetchCacheEntry and Response APIs
instead.
- Line 233: The browser entry is now failing because __basePath and the snapshot
helper exports (e.g., activateNavigationSnapshot, commitClientNavigationState
and related types/helpers) were removed or made internal from the navigation
shim; restore a compatible export surface by re-exporting __basePath and the
removed snapshot helpers/types from the navigation shim module so the browser
entry can import them unchanged. Concretely, reintroduce exported symbols named
__basePath, activateNavigationSnapshot, commitClientNavigationState (and any
other snapshot helpers/types referenced by the browser entry) in the navigation
shim and have them delegate to the internal implementations if needed so
existing imports in the browser entry continue to resolve. Ensure the exported
names and signatures match what the browser entry expects so builds no longer
error on missing exports.
- Around line 518-520: navigateImpl currently calls
window.history.pushState/replaceState and then notifyListeners(), but the file
also patches pushState/replaceState to call notifyListeners which causes double
notifications when an already-patched native is captured (e.g., multi-instance
Vite). Fix by installing a single global one-time patch of history.pushState and
history.replaceState and by capturing and using true native functions for
internal writes: store originals in a global symbol (use and set
_nativeReplaceState/_nativePushState from that global) when patching, have the
patched methods call the originals then notify once, and update navigateImpl and
saveScrollPosition to call the stored native functions
(_nativeReplaceState/_nativePushState) for internal writes so they do not
re-trigger the notifier; ensure notifyListeners is only invoked by the patched
methods, not again in navigateImpl.
In `@tests/font-google.test.ts`:
- Around line 23-37: Tests must initialize the plugin's closure state instead of
setting nonexistent properties; call the plugin's configResolved hook
(configResolved({... command: "build", root, cacheDir, ...})) via
getGoogleFontsPlugin() before running assertions to set the closure variables
isBuild, fontCache and cacheDir, and remove any assignments to
plugin._isBuild/_fontCache/_cacheDir; if tests need to pre-populate or inspect
the internal fontCache, either trigger the plugin code paths (e.g., call
transform with appropriate inputs) to populate the closure Map or add a small
test-only API in the plugin (e.g., export a setter/getter) that manipulates the
closure variables rather than relying on object properties.
---
Outside diff comments:
In `@packages/vinext/src/shims/navigation.ts`:
- Around line 586-590: The export for navigateClientSide was removed but
packages/vinext/src/shims/link.tsx (the Link shim) still calls
navigateClientSide, so restore a compatible export to avoid breaking Link;
re-export or reintroduce a navigateClientSide function that delegates to the
existing navigateImpl (matching its signature: async function navigateImpl(href:
string, mode: "push" | "replace", scroll: boolean): Promise<void>) and keep the
exported name navigateClientSide until Link is fully migrated.
In `@README.md`:
- Around line 43-47: The README's command examples assume a global vinext
binary; change the examples shown (the three-line snippet under the quick-start)
to either prefix commands with npx (e.g., "npx vinext dev/build/deploy") or add
a short note pointing readers to use the npm scripts declared above (e.g., "npm
run dev/build/deploy") so the commands work for users who installed vinext
locally via npm.
---
Duplicate comments:
In `@packages/vinext/src/entries/app-rsc-entry.ts`:
- Around line 1324-1387: The current emitted map conflates page- and
layout-level generateStaticParams because entries builds a single pattern->fn
string (using emittedPatterns, getImportVar, and entries) which causes layout
exports to be hidden/overwritten by page entries; split storage so both can
coexist: either emit two distinct maps (e.g., pageStaticParamsMap and
layoutStaticParamsMap) when building entries (push page entries into page map
and layout entries into layout map) or emit a structured value for each pattern
like { page: <fn>|null, layout: <fn>|null }; then update
packages/vinext/src/build/prerender.ts to read the new shapes (replace direct
staticParamsMap[route.pattern] lookups and resolveParentParams parent-prefix
lookups to check page vs layout slots or the two maps accordingly) and remove
the emittedPatterns dedupe logic that hides the other export (adjust usages of
emittedPatterns/emitted list as needed).
In `@packages/vinext/src/shims/font-google.ts`:
- Around line 169-189: The self-hosted branch always calls injectSelfHostedCSS
with the full styles (using options._selfHostedCSS and options._fallbackCSS)
regardless of options.preload, which causes SSR to infer preloads from all woff2
entries; update the logic around injectSelfHostedCSS so it honors
options.preload: when options.preload === false, do not inject the full
stylesheet (skip injectSelfHostedCSS) or instead pass a filtered CSS string that
contains only the rules/resources you intend to preload (e.g., strip woff2
`@font-face` rules) before calling injectSelfHostedCSS; touch the block that
references options._selfHostedCSS, options._fallbackCSS and the call to
injectSelfHostedCSS to implement this conditional/filtering behavior.
- Around line 126-150: The code only checks styles when weights.length > 0,
causing style-only requests to omit the ital axis; fix by moving the hasItalic
check out of that branch and handling the case where styles includes "italic"
but weights is empty: when weights.length === 0 and styles.includes("italic"),
treat weights as the full range (100..900) and build the spec using the same
ital,wght logic (use hasItalic, pairs, and spec += `:ital,wght@...`) so
style-only requests produce the correct CSS2 spec; update the logic around
weights, styles, hasItalic, and spec construction accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a7cf93e7-323a-4c29-b185-82dffd1a9a55
⛔ Files ignored due to path filters (3)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamltests/__snapshots__/entry-templates.test.ts.snapis excluded by!**/*.snaptests/fixtures/fonts/inter-latin-400.woff2is excluded by!**/*.woff2
📒 Files selected for processing (19)
.agents/skills/migrate-to-vinext/SKILL.md.agents/skills/migrate-to-vinext/references/compatibility.md.github/workflows/preview-release.ymlREADME.mdpackages/vinext/package.jsonpackages/vinext/src/check.tspackages/vinext/src/entries/app-rsc-entry.tspackages/vinext/src/font-metrics.tspackages/vinext/src/shims/font-google.tspackages/vinext/src/shims/font-local.tspackages/vinext/src/shims/font-utils.tspackages/vinext/src/shims/navigation.tstests/check.test.tstests/fixtures/fonts/LICENSEtests/font-google.test.tstests/font-local-transform.test.tstests/font-metrics.test.tstests/font-utils.test.tstests/shims.test.ts
✅ Files skipped from review due to trivial changes (3)
- tests/fixtures/fonts/LICENSE
- packages/vinext/src/shims/font-local.ts
- tests/font-utils.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/preview-release.yml
- packages/vinext/package.json
- .agents/skills/migrate-to-vinext/SKILL.md
- tests/check.test.ts
Comment on lines
38
to
61
| const IMPORT_SUPPORT: Record<string, { status: Status; detail?: string }> = { | ||
| next: { status: "supported", detail: "type-only exports (Metadata, NextPage, etc.)" }, | ||
| "next": { status: "supported", detail: "type-only exports (Metadata, NextPage, etc.)" }, | ||
| "next/link": { status: "supported" }, | ||
| "next/image": { status: "supported", detail: "uses @unpic/react (no local optimization yet)" }, | ||
| "next/legacy/image": { | ||
| status: "supported", | ||
| detail: "pre-Next.js 13 Image API with layout prop; translated to modern Image", | ||
| }, | ||
| "next/router": { status: "supported" }, | ||
| "next/compat/router": { | ||
| status: "supported", | ||
| detail: "useRouter() returns null in App Router, router object in Pages Router", | ||
| }, | ||
| "next/navigation": { status: "supported" }, | ||
| "next/headers": { status: "supported" }, | ||
| "next/server": { status: "supported", detail: "NextRequest/NextResponse shimmed" }, | ||
| "next/cache": { | ||
| status: "supported", | ||
| detail: "revalidateTag, revalidatePath, unstable_cache, cacheLife, cacheTag", | ||
| }, | ||
| "next/cache": { status: "supported", detail: "revalidateTag, revalidatePath, unstable_cache, cacheLife, cacheTag" }, | ||
| "next/dynamic": { status: "supported" }, | ||
| "next/head": { status: "supported" }, | ||
| "next/script": { status: "supported" }, | ||
| "next/font/google": { | ||
| status: "partial", | ||
| detail: "fonts loaded from CDN, not self-hosted at build time", | ||
| }, | ||
| "next/font/local": { | ||
| status: "supported", | ||
| detail: "className and variable modes both work; no build-time subsetting", | ||
| }, | ||
| "next/font/google": { status: "supported", detail: "self-hosted at build time with fallback metrics and hashed font-family names" }, | ||
| "next/font/local": { status: "supported", detail: "build-time @font-face extraction with fallback metrics and hashed font-family names" }, | ||
| "next/og": { status: "supported", detail: "ImageResponse via @vercel/og" }, | ||
| "next/config": { status: "supported" }, | ||
| "next/amp": { status: "unsupported", detail: "AMP is not supported" }, | ||
| "next/document": { status: "supported", detail: "custom _document.tsx" }, | ||
| "next/app": { status: "supported", detail: "custom _app.tsx" }, | ||
| "next/error": { status: "supported" }, | ||
| "next/form": { status: "supported", detail: "Form component with client-side navigation" }, | ||
| "next/web-vitals": { status: "supported", detail: "reportWebVitals helper" }, | ||
| "next/constants": { status: "supported", detail: "PHASE_* constants" }, | ||
| "next/third-parties/google": { | ||
| status: "unsupported", | ||
| detail: "third-party script optimization not implemented", | ||
| }, | ||
| "next/third-parties/google": { status: "unsupported", detail: "third-party script optimization not implemented" }, | ||
| "server-only": { status: "supported" }, | ||
| "client-only": { status: "supported" }, | ||
| // Internal next/dist/* paths used by libraries (testing utilities, older libs, etc.) | ||
| "next/dist/shared/lib/router-context.shared-runtime": { | ||
| status: "supported", | ||
| detail: "RouterContext for Pages Router; used by testing utilities and older libraries", | ||
| }, | ||
| "next/dist/shared/lib/app-router-context.shared-runtime": { | ||
| status: "supported", | ||
| detail: "AppRouterContext and layout contexts; used by testing utilities and UI libraries", | ||
| }, | ||
| "next/dist/shared/lib/app-router-context": { | ||
| status: "supported", | ||
| detail: "AppRouterContext and layout contexts; used by testing utilities and UI libraries", | ||
| }, | ||
| "next/dist/shared/lib/utils": { | ||
| status: "supported", | ||
| detail: "execOnce, getLocationOrigin and other shared utilities", | ||
| }, | ||
| "next/dist/server/api-utils": { | ||
| status: "supported", | ||
| detail: "NextApiRequestCookies and Pages Router API route utilities", | ||
| }, | ||
| "next/dist/server/web/spec-extension/cookies": { | ||
| status: "supported", | ||
| detail: "RequestCookies / ResponseCookies — shimmed via @edge-runtime/cookies", | ||
| }, | ||
| "next/dist/compiled/@edge-runtime/cookies": { | ||
| status: "supported", | ||
| detail: "RequestCookies / ResponseCookies — shimmed via @edge-runtime/cookies", | ||
| }, | ||
| "next/dist/server/app-render/work-unit-async-storage.external": { | ||
| status: "supported", | ||
| detail: "request-scoped AsyncLocalStorage for App Router server components", | ||
| }, | ||
| "next/dist/client/components/work-unit-async-storage.external": { | ||
| status: "supported", | ||
| detail: "request-scoped AsyncLocalStorage for App Router server components", | ||
| }, | ||
| "next/dist/client/components/request-async-storage.external": { | ||
| status: "supported", | ||
| detail: "request-scoped AsyncLocalStorage (legacy path alias)", | ||
| }, | ||
| "next/dist/client/components/request-async-storage": { | ||
| status: "supported", | ||
| detail: "request-scoped AsyncLocalStorage (legacy path alias)", | ||
| }, | ||
| "next/dist/client/components/navigation": { | ||
| status: "supported", | ||
| detail: "internal navigation module; re-exports next/navigation", | ||
| }, | ||
| "next/dist/server/config-shared": { | ||
| status: "supported", | ||
| detail: "shared config utilities; re-exports next/dist/shared/lib/utils", | ||
| }, | ||
| }; |
There was a problem hiding this comment.
The scanner now false-flags several shipped next/* shims as unsupported.
Anything missing from IMPORT_SUPPORT falls through to status: "unsupported", but the repo still documents next/form, next/legacy/image, next/constants, and next/web-vitals as working or stubbed in .agents/skills/migrate-to-vinext/references/compatibility.md and README.md. vinext check will now lower the score and report false blockers for apps using those imports.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/vinext/src/check.ts` around lines 38 - 61, IMPORT_SUPPORT is missing
entries for several shipped next/* shims so imports like next/form,
next/legacy/image, next/constants, and next/web-vitals are treated as
unsupported; update the IMPORT_SUPPORT record to include keys "next/form",
"next/legacy/image", "next/constants", and "next/web-vitals" with the
appropriate status ("supported" or "stubbed") and short detail strings matching
the documented behavior in
.agents/skills/migrate-to-vinext/references/compatibility.md and README.md so
vinext check no longer false-flags these imports—modify the IMPORT_SUPPORT
constant in packages/vinext/src/check.ts to add those entries.
packages/vinext/src/check.ts
Outdated
Comment on lines
+311
to
+321
| const hasProxy = fs.existsSync(path.join(root, "proxy.ts")) || fs.existsSync(path.join(root, "proxy.js")); | ||
| const hasMiddleware = fs.existsSync(path.join(root, "middleware.ts")) || fs.existsSync(path.join(root, "middleware.js")); | ||
|
|
||
| if (hasPages) { | ||
| const isSrc = pagesDir!.includes(path.join("src", "pages")); | ||
| items.push({ name: isSrc ? "Pages Router (src/pages/)" : "Pages Router (pages/)", status: "supported" }); | ||
|
|
||
| // Count pages | ||
| const pageFiles = findSourceFiles(pagesDir); | ||
| const pages = pageFiles.filter( | ||
| (f) => | ||
| !f.includes("/api/") && | ||
| !f.includes("_app") && | ||
| !f.includes("_document") && | ||
| !f.includes("_error"), | ||
| ); | ||
| const apiRoutes = pageFiles.filter((f) => f.includes("/api/")); | ||
| const pageFiles = findSourceFiles(pagesDir!); | ||
| const pages = pageFiles.filter(f => !f.includes("/api/") && !f.includes("_app") && !f.includes("_document") && !f.includes("_error")); | ||
| const apiRoutes = pageFiles.filter(f => f.includes("/api/")); |
There was a problem hiding this comment.
The conventions scan still assumes root-only middleware and POSIX separators.
hasProxy / hasMiddleware only check the repo root even though pages/ and app/ already fall back to src/, and the page/API split below relies on hard-coded "/api/" substrings. On src/middleware.ts / src/proxy.ts projects or on Windows paths, the report will miss middleware and overcount API routes as regular pages.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/vinext/src/check.ts` around lines 311 - 321, The scan assumes
middleware/proxy live only in repo root and uses POSIX "/api/" substrings,
causing misses on projects with src/ structure or on Windows; update the
hasProxy/hasMiddleware checks to look for both root and src locations (e.g.,
check path.join(root, "middleware.ts") and path.join(root, "src",
"middleware.ts") and same for proxy) and change the pages/api split logic (the
pagesDir/findSourceFiles filtering that builds pages and apiRoutes) to use
platform-safe path operations (e.g., normalize paths and check for path
separators or use path.relative and split on path.sep to detect an "api"
segment) so middleware/proxy are detected whether under root or src and API
routes are correctly identified on Windows and POSIX.
Comment on lines
+276
to
+289
| /** | ||
| * Inject a <link> stylesheet tag (client) or track URL for SSR (server). | ||
| */ | ||
| export function injectFontStylesheet(state: FontSSRState, url: string): void { | ||
| if (state.injectedFonts.has(url)) return; | ||
| state.injectedFonts.add(url); | ||
|
|
||
| if (typeof document !== "undefined") { | ||
| const link = document.createElement("link"); | ||
| link.rel = "stylesheet"; | ||
| link.href = url; | ||
| document.head.appendChild(link); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how injectFontStylesheet is used and whether SSR collects stylesheet URLs elsewhere
rg -n 'injectFontStylesheet' packages/vinext/src/
# Check if there's a separate SSR accessor for stylesheet URLs
rg -n 'ssrFontLinks|getSSRFontLinks' packages/vinext/src/Repository: streamloop/vinext
Length of output: 1985
🏁 Script executed:
#!/bin/bash
# Check FontSSRState definition
rg -n 'interface FontSSRState|type FontSSRState' packages/vinext/src/ -A 10
# Check getSSRFontLinks implementation in font-google.ts
sed -n '55,80p' packages/vinext/src/shims/font-google.ts
# Check how injectFontStylesheet is called at line 206
sed -n '200,210p' packages/vinext/src/shims/font-google.ts
# Check FontSSRState full definition
sed -n '1,50p' packages/vinext/src/shims/font-utils.ts | grep -A 20 'FontSSRState'Repository: streamloop/vinext
Length of output: 1912
🏁 Script executed:
#!/bin/bash
# Find where ssrFontUrlSet and ssrFontUrls are defined and used
rg -n 'ssrFontUrlSet|ssrFontUrls' packages/vinext/src/shims/font-google.ts -B 2 -A 2
# Check the context around line 206 to confirm the flow
sed -n '200,220p' packages/vinext/src/shims/font-google.tsRepository: streamloop/vinext
Length of output: 1278
🏁 Script executed:
#!/bin/bash
# Check all usages of injectFontStylesheet from font-utils.ts export
# to see if callers handle SSR tracking
sed -n '200,220p' packages/vinext/src/shims/font-google.ts
# Check if font-google-base.ts also handles SSR tracking for its injectFontStylesheet
sed -n '160,170p' packages/vinext/src/shims/font-google-base.ts
# Check context around font-google-base.ts line 392
sed -n '385,400p' packages/vinext/src/shims/font-google-base.tsRepository: streamloop/vinext
Length of output: 1594
🏁 Script executed:
#!/bin/bash
# Check the full injectFontStylesheet function in font-utils.ts (lines 276-289)
sed -n '276,289p' packages/vinext/src/shims/font-utils.ts
# Check all files that import injectFontStylesheet from font-utils
rg -l 'from.*font-utils|import.*injectFontStylesheet' packages/vinext/src/ | head -20Repository: streamloop/vinext
Length of output: 562
🏁 Script executed:
#!/bin/bash
# Check how font-local.ts uses injectFontStylesheet
rg -n 'injectFontStylesheet' packages/vinext/src/shims/font-local.ts -B 3 -A 3
# Check if font-local.ts has SSR tracking mechanism
rg -n 'ssrFontUrls|ssrFontStyles' packages/vinext/src/shims/font-local.tsRepository: streamloop/vinext
Length of output: 98
🏁 Script executed:
#!/bin/bash
# Search for injectFontStylesheet usage in font-local.ts with more context
rg -n 'injectFontStylesheet' packages/vinext/src/shims/font-local.ts -B 5 -A 5
# Get the full context around the usage
sed -n '1,100p' packages/vinext/src/shims/font-local.ts | tail -80Repository: streamloop/vinext
Length of output: 2546
🏁 Script executed:
#!/bin/bash
# Get the complete import list from font-local.ts to confirm no injectFontStylesheet
sed -n '1,20p' packages/vinext/src/shims/font-local.ts
# Check font-google-base.ts usage again - it had a different injectFontStylesheet
# (local function, not the exported one from font-utils)
sed -n '165,175p' packages/vinext/src/shims/font-google-base.tsRepository: streamloop/vinext
Length of output: 1149
Misleading docstring for injectFontStylesheet—function doesn't track URLs on server.
The docstring states "track URL for SSR (server)", but the function only injects the <link> tag on the client. On the server, when document is undefined, the function does nothing—it never adds the URL to state or any SSR tracking mechanism.
In font-google.ts, manual SSR tracking occurs separately after calling this function (lines 209–211), meaning callers must implement their own deduplication and tracking. This creates a fragile and error-prone API where the docstring doesn't match the actual behavior. Either the function should perform SSR tracking as its docstring claims, or the docstring should accurately reflect that client-side injection only is handled here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/vinext/src/shims/font-utils.ts` around lines 276 - 289, The
docstring for injectFontStylesheet is misleading; update it to state that the
function marks the URL in state.injectedFonts for deduplication (so callers can
rely on that for SSR) and only injects a <link> tag when running in a browser
(when document is defined); reference injectFontStylesheet and
state.injectedFonts and note that callers in font-google.ts should not need
separate tracking after this change (or remove their duplicate tracking if you
standardize on this function).
|
|
||
| /** basePath from next.config.js, injected by the plugin at build time */ | ||
| export const __basePath: string = process.env.__NEXT_ROUTER_BASEPATH ?? ""; | ||
| const __basePath: string = process.env.__NEXT_ROUTER_BASEPATH ?? ""; |
There was a problem hiding this comment.
Keep the browser entry in lockstep with this export-surface change.
packages/vinext/src/server/app-browser-entry.ts:23-42 still imports __basePath, activateNavigationSnapshot, commitClientNavigationState, and the rest of the old snapshot helpers/types from this module. With __basePath now internal and that API removed here, the browser entry hits missing-export build errors immediately.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/vinext/src/shims/navigation.ts` at line 233, The browser entry is
now failing because __basePath and the snapshot helper exports (e.g.,
activateNavigationSnapshot, commitClientNavigationState and related
types/helpers) were removed or made internal from the navigation shim; restore a
compatible export surface by re-exporting __basePath and the removed snapshot
helpers/types from the navigation shim module so the browser entry can import
them unchanged. Concretely, reintroduce exported symbols named __basePath,
activateNavigationSnapshot, commitClientNavigationState (and any other snapshot
helpers/types referenced by the browser entry) in the navigation shim and have
them delegate to the internal implementations if needed so existing imports in
the browser entry continue to resolve. Ensure the exported names and signatures
match what the browser entry expects so builds no longer error on missing
exports.
Comment on lines
245
to
247
| export type PrefetchCacheEntry = { | ||
| snapshot?: CachedRscResponse; | ||
| pending?: Promise<void>; | ||
| response: Response; | ||
| timestamp: number; |
There was a problem hiding this comment.
The browser entry still expects the old prefetch-response helpers.
This refactor drops CachedRscResponse, consumePrefetchResponse, restoreRscResponse, and snapshotRscResponse, but packages/vinext/src/server/app-browser-entry.ts:23-42,670-680 still imports/calls them. The prefetch path cannot compile or consume cached responses until that entry point is updated to the new { response, timestamp } contract.
Also applies to: 292-317
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/vinext/src/shims/navigation.ts` around lines 245 - 247, The browser
entry still uses the removed RSC helpers (CachedRscResponse,
consumePrefetchResponse, restoreRscResponse, snapshotRscResponse); update it to
the new PrefetchCacheEntry shape ({ response, timestamp }) by replacing
imports/usages of those helpers with direct access to the
PrefetchCacheEntry.response and .timestamp, adapt any code that previously
called consumePrefetchResponse/restoreRscResponse to operate on the Response
object (e.g., read/clone the Response or rehydrate streaming body as needed),
and remove snapshotRscResponse-related logic in favor of storing/restoring the {
response, timestamp } object; search for the identifiers CachedRscResponse,
consumePrefetchResponse, restoreRscResponse, snapshotRscResponse in the browser
entry and refactor those code paths to use PrefetchCacheEntry and Response APIs
instead.
Comment on lines
+296
to
+316
| // Sweep expired entries before resorting to FIFO eviction | ||
| if (cache.size >= MAX_PREFETCH_CACHE_SIZE) { | ||
| const prefetched = getPrefetchedUrls(); | ||
| for (const [key, entry] of cache) { | ||
| if (now - entry.timestamp >= PREFETCH_CACHE_TTL) { | ||
| cache.delete(key); | ||
| prefetched.delete(key); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| while (cache.size >= MAX_PREFETCH_CACHE_SIZE) { | ||
| // FIFO fallback if still at capacity after sweep | ||
| if (cache.size >= MAX_PREFETCH_CACHE_SIZE) { | ||
| const oldest = cache.keys().next().value; | ||
| if (oldest !== undefined) { | ||
| cache.delete(oldest); | ||
| prefetched.delete(oldest); | ||
| } else { | ||
| break; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Store a prefetched RSC response in the cache by snapshotting it to an | ||
| * ArrayBuffer. The snapshot completes asynchronously; during that window | ||
| * the entry is marked `pending` so consumePrefetchResponse() will skip it | ||
| * (the caller falls back to a fresh fetch, which is acceptable). | ||
| * | ||
| * Prefer prefetchRscResponse() for new call-sites — it handles the full | ||
| * prefetch lifecycle including dedup. storePrefetchResponse() is kept for | ||
| * backward compatibility and test helpers. | ||
| * | ||
| * NB: Caller is responsible for managing getPrefetchedUrls() — this | ||
| * function only stores the response in the prefetch cache. | ||
| */ | ||
| export function storePrefetchResponse(rscUrl: string, response: Response): void { | ||
| evictPrefetchCacheIfNeeded(); | ||
| const entry: PrefetchCacheEntry = { timestamp: Date.now() }; | ||
| entry.pending = snapshotRscResponse(response) | ||
| .then((snapshot) => { | ||
| entry.snapshot = snapshot; | ||
| }) | ||
| .catch(() => { | ||
| getPrefetchCache().delete(rscUrl); | ||
| }) | ||
| .finally(() => { | ||
| entry.pending = undefined; | ||
| }); | ||
| getPrefetchCache().set(rscUrl, entry); | ||
| } | ||
|
|
||
| /** | ||
| * Snapshot an RSC response to an ArrayBuffer for caching and replay. | ||
| * Consumes the response body and stores it with content-type and URL metadata. | ||
| */ | ||
| export async function snapshotRscResponse(response: Response): Promise<CachedRscResponse> { | ||
| const buffer = await response.arrayBuffer(); | ||
| return { | ||
| buffer, | ||
| contentType: response.headers.get("content-type") ?? "text/x-component", | ||
| paramsHeader: response.headers.get("X-Vinext-Params"), | ||
| url: response.url, | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Reconstruct a Response from a cached RSC snapshot. | ||
| * Creates a new Response with the original ArrayBuffer so createFromFetch | ||
| * can consume the stream from scratch. | ||
| * | ||
| * NOTE: The reconstructed Response always has `url === ""` — the Response | ||
| * constructor does not accept a `url` option, and `response.url` is read-only | ||
| * set by the fetch infrastructure. Callers that need the original URL should | ||
| * read it from `cached.url` directly rather than from the restored Response. | ||
| * | ||
| * @param copy - When true (default), copies the ArrayBuffer so the cached | ||
| * snapshot remains replayable (needed for the visited-response cache). | ||
| * Pass false for single-consumption paths (e.g. prefetch cache entries | ||
| * that are deleted after consumption) to avoid the extra allocation. | ||
| */ | ||
| export function restoreRscResponse(cached: CachedRscResponse, copy = true): Response { | ||
| const headers = new Headers({ "content-type": cached.contentType }); | ||
| if (cached.paramsHeader != null) { | ||
| headers.set("X-Vinext-Params", cached.paramsHeader); | ||
| } | ||
|
|
||
| return new Response(copy ? cached.buffer.slice(0) : cached.buffer, { | ||
| status: 200, | ||
| headers, | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Prefetch an RSC response and snapshot it for later consumption. | ||
| * Stores the in-flight promise so immediate clicks can await it instead | ||
| * of firing a duplicate fetch. | ||
| * Enforces a maximum cache size to prevent unbounded memory growth on | ||
| * link-heavy pages. | ||
| */ | ||
| export function prefetchRscResponse(rscUrl: string, fetchPromise: Promise<Response>): void { | ||
| const cache = getPrefetchCache(); | ||
| const prefetched = getPrefetchedUrls(); | ||
| const now = Date.now(); | ||
|
|
||
| const entry: PrefetchCacheEntry = { timestamp: now }; | ||
|
|
||
| entry.pending = fetchPromise | ||
| .then(async (response) => { | ||
| if (response.ok) { | ||
| entry.snapshot = await snapshotRscResponse(response); | ||
| } else { | ||
| prefetched.delete(rscUrl); | ||
| cache.delete(rscUrl); | ||
| } | ||
| }) | ||
| .catch(() => { | ||
| prefetched.delete(rscUrl); | ||
| cache.delete(rscUrl); | ||
| }) | ||
| .finally(() => { | ||
| entry.pending = undefined; | ||
| }); | ||
|
|
||
| // Insert the new entry before evicting. FIFO evicts from the front of the | ||
| // Map (oldest insertion order), so the just-appended entry is safe — only | ||
| // entries inserted before it are candidates for removal. | ||
| cache.set(rscUrl, entry); | ||
| evictPrefetchCacheIfNeeded(); | ||
| } | ||
|
|
||
| /** | ||
| * Consume a prefetched response for a given rscUrl. | ||
| * Only returns settled (non-pending) snapshots synchronously. | ||
| * Returns null if the entry is still in flight or doesn't exist. | ||
| */ | ||
| export function consumePrefetchResponse(rscUrl: string): CachedRscResponse | null { | ||
| const cache = getPrefetchCache(); | ||
| const entry = cache.get(rscUrl); | ||
| if (!entry) return null; | ||
|
|
||
| // Don't consume pending entries — let the navigation fetch independently. | ||
| if (entry.pending) return null; | ||
|
|
||
| cache.delete(rscUrl); | ||
| getPrefetchedUrls().delete(rscUrl); | ||
|
|
||
| if (entry.snapshot) { | ||
| if (Date.now() - entry.timestamp >= PREFETCH_CACHE_TTL) { | ||
| return null; | ||
| getPrefetchedUrls().delete(oldest); | ||
| } | ||
| return entry.snapshot; | ||
| } | ||
|
|
||
| return null; | ||
| cache.set(rscUrl, { response, timestamp: now }); |
There was a problem hiding this comment.
Expired prefetches never age out under the common case.
The 30s TTL only runs when cache.size >= MAX_PREFETCH_CACHE_SIZE, but prefetch() returns early on prefetched.has(rscUrl) before any expiry check. On tabs that stay under 50 entries, one stale prefetch permanently blocks re-prefetching that route.
Minimal fix
const fullHref = toBrowserNavigationHref(href, window.location.href, __basePath);
const rscUrl = toRscUrl(fullHref);
+ const cache = getPrefetchCache();
const prefetched = getPrefetchedUrls();
+ const cached = cache.get(rscUrl);
+ if (cached && Date.now() - cached.timestamp >= PREFETCH_CACHE_TTL) {
+ cache.delete(rscUrl);
+ prefetched.delete(rscUrl);
+ }
if (prefetched.has(rscUrl)) return;Also applies to: 693-700
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/vinext/src/shims/navigation.ts` around lines 296 - 316, The issue is
that prefetch() returns early when getPrefetchedUrls().has(rscUrl) without
checking expiry, so stale entries never age out unless the cache is full; fix
by, in the prefetch flow where you check prefetched.has(rscUrl), also look up
the corresponding entry in cache (using rscUrl) and compare its timestamp
against PREFETCH_CACHE_TTL and now, and if expired remove it from both cache and
getPrefetchedUrls() and continue with the prefetch; keep existing sweep-and-FIFO
logic for capacity but ensure the early-return path verifies and evicts expired
entries first (references: prefetch(), rscUrl, cache, getPrefetchedUrls(),
MAX_PREFETCH_CACHE_SIZE, PREFETCH_CACHE_TTL).
Comment on lines
319
to
325
| // Client navigation listeners | ||
| type NavigationListener = () => void; | ||
| const _CLIENT_NAV_STATE_KEY = Symbol.for("vinext.clientNavigationState"); | ||
|
|
||
| type ClientNavigationState = { | ||
| listeners: Set<NavigationListener>; | ||
| cachedSearch: string; | ||
| cachedReadonlySearchParams: ReadonlyURLSearchParams; | ||
| cachedPathname: string; | ||
| clientParams: Record<string, string | string[]>; | ||
| clientParamsJson: string; | ||
| pendingClientParams: Record<string, string | string[]> | null; | ||
| pendingClientParamsJson: string | null; | ||
| originalPushState: typeof window.history.pushState; | ||
| originalReplaceState: typeof window.history.replaceState; | ||
| patchInstalled: boolean; | ||
| hasPendingNavigationUpdate: boolean; | ||
| suppressUrlNotifyCount: number; | ||
| navigationSnapshotActiveCount: number; | ||
| }; | ||
|
|
||
| type ClientNavigationGlobal = typeof globalThis & { | ||
| [_CLIENT_NAV_STATE_KEY]?: ClientNavigationState; | ||
| }; | ||
|
|
||
| function getClientNavigationState(): ClientNavigationState | null { | ||
| if (isServer) return null; | ||
|
|
||
| const globalState = window as ClientNavigationGlobal; | ||
| globalState[_CLIENT_NAV_STATE_KEY] ??= { | ||
| listeners: new Set<NavigationListener>(), | ||
| cachedSearch: window.location.search, | ||
| cachedReadonlySearchParams: new ReadonlyURLSearchParams(window.location.search), | ||
| cachedPathname: stripBasePath(window.location.pathname, __basePath), | ||
| clientParams: {}, | ||
| clientParamsJson: "{}", | ||
| pendingClientParams: null, | ||
| pendingClientParamsJson: null, | ||
| // NB: These capture the currently installed history methods, not guaranteed | ||
| // native ones. If a third-party library (analytics, router) has already patched | ||
| // history methods before this module loads, we intentionally preserve that | ||
| // wrapper. With Symbol.for global state, the first module instance to load wins. | ||
| originalPushState: window.history.pushState.bind(window.history), | ||
| originalReplaceState: window.history.replaceState.bind(window.history), | ||
| patchInstalled: false, | ||
| hasPendingNavigationUpdate: false, | ||
| suppressUrlNotifyCount: 0, | ||
| navigationSnapshotActiveCount: 0, | ||
| }; | ||
| const _listeners: Set<NavigationListener> = new Set(); | ||
|
|
||
| return globalState[_CLIENT_NAV_STATE_KEY]!; | ||
| } | ||
|
|
||
| function notifyNavigationListeners(): void { | ||
| const state = getClientNavigationState(); | ||
| if (!state) return; | ||
| for (const fn of state.listeners) fn(); | ||
| function notifyListeners(): void { | ||
| for (const fn of _listeners) fn(); | ||
| } |
There was a problem hiding this comment.
setClientParams() still only wakes one module instance.
The params object lives on globalThis, but _listeners is still per-module. In the Vite duplication case called out in this file, packages/vinext/src/server/app-browser-entry.ts can call setClientParams() from one instance while useParams() subscribes in another, so params updates do not re-render until some unrelated URL event happens.
Also applies to: 373-390, 406-410
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/vinext/src/shims/navigation.ts` around lines 319 - 325,
setClientParams stores params on globalThis but _listeners is module-scoped, so
in duplicated Vite modules updates only notify listeners in that module; change
listener storage to a single shared set on globalThis (e.g., attach a unique
symbol or property like globalThis.__vinext_navigation_listeners) and have
notifyListeners, add/remove listener logic (used by useParams) read/modify that
global set instead of the local _listeners; update the other listener blocks
referenced (lines ~373-390 and ~406-410) to use the same global listener
container so all module instances are notified when setClientParams runs.
Comment on lines
+518
to
+520
| const _nativeReplaceState: typeof window.history.replaceState | null = !isServer | ||
| ? window.history.replaceState.bind(window.history) | ||
| : null; |
There was a problem hiding this comment.
Patch history once globally and avoid double-notifying internal navigations.
navigateImpl() calls notifyListeners() right after window.history.pushState/replaceState, but those methods are also patched below to notify already. In the multi-instance Vite case mentioned in this file, later instances also capture an already-patched replaceState as _nativeReplaceState, so saveScrollPosition() can re-enter the notifier during scroll-state writes. This needs a one-time global patch plus native-method calls for internal writes.
Also applies to: 621-625, 636-641, 929-968
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/vinext/src/shims/navigation.ts` around lines 518 - 520, navigateImpl
currently calls window.history.pushState/replaceState and then
notifyListeners(), but the file also patches pushState/replaceState to call
notifyListeners which causes double notifications when an already-patched native
is captured (e.g., multi-instance Vite). Fix by installing a single global
one-time patch of history.pushState and history.replaceState and by capturing
and using true native functions for internal writes: store originals in a global
symbol (use and set _nativeReplaceState/_nativePushState from that global) when
patching, have the patched methods call the originals then notify once, and
update navigateImpl and saveScrollPosition to call the stored native functions
(_nativeReplaceState/_nativePushState) for internal writes so they do not
re-trigger the notifier; ensure notifyListeners is only invoked by the patched
methods, not again in navigateImpl.
tests/font-google.test.ts
Outdated
Comment on lines
23
to
37
| /** Plugin with font-specific test fields exposed */ | ||
| interface GoogleFontsTestPlugin extends Plugin { | ||
| _isBuild: boolean; | ||
| _fontCache: Map<string, string>; | ||
| _cacheDir: string; | ||
| } | ||
|
|
||
| /** Extract the vinext:google-fonts plugin from the plugin array */ | ||
| function getGoogleFontsPlugin(): Plugin { | ||
| function getGoogleFontsPlugin(): GoogleFontsTestPlugin { | ||
| const plugins = vinext() as Plugin[]; | ||
| const plugin = plugins.find((p) => p.name === "vinext:google-fonts"); | ||
| if (!plugin) throw new Error("vinext:google-fonts plugin not found"); | ||
| return plugin; | ||
| // The plugin object has these fields at runtime (set in the plugin definition) | ||
| return plugin as unknown as GoogleFontsTestPlugin; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the plugin implementation uses closure variables, not exposed properties
ast-grep --pattern $'function createGoogleFontsPlugin($_) {
$$$
let isBuild = $_;
$$$
}'
# Check if plugin exposes any _isBuild, _fontCache, _cacheDir properties
rg -n '_isBuild|_fontCache|_cacheDir' packages/vinext/src/plugins/fonts.tsRepository: streamloop/vinext
Length of output: 43
🏁 Script executed:
# Find the fonts.ts plugin file
find . -name "fonts.ts" -type f | head -20Repository: streamloop/vinext
Length of output: 99
🏁 Script executed:
# Also check the test file mentioned
find . -name "font-google.test.ts" -type f | head -20Repository: streamloop/vinext
Length of output: 88
🏁 Script executed:
# List the vinext package structure
ls -la packages/vinext/src/plugins/ 2>/dev/null || echo "Path not found, searching for plugins directory"
find . -type d -name "plugins" | grep -E "(src|vinext)" | head -10Repository: streamloop/vinext
Length of output: 794
🏁 Script executed:
# Read the fonts.ts file to understand the implementation
wc -l packages/vinext/src/plugins/fonts.tsRepository: streamloop/vinext
Length of output: 101
🏁 Script executed:
# Read the beginning of fonts.ts to understand the plugin structure
head -100 packages/vinext/src/plugins/fonts.tsRepository: streamloop/vinext
Length of output: 3907
🏁 Script executed:
# Find the createGoogleFontsPlugin function definition
rg -n "createGoogleFontsPlugin|let isBuild|const fontCache|let cacheDir" packages/vinext/src/plugins/fonts.ts | head -30Repository: streamloop/vinext
Length of output: 347
🏁 Script executed:
# Read around where createGoogleFontsPlugin is defined
sed -n '200,350p' packages/vinext/src/plugins/fonts.tsRepository: streamloop/vinext
Length of output: 4886
🏁 Script executed:
# Continue reading the plugin definition to see if isBuild, fontCache, cacheDir are exposed
sed -n '527,650p' packages/vinext/src/plugins/fonts.tsRepository: streamloop/vinext
Length of output: 5268
🏁 Script executed:
# Read the test file to see what properties are being set/accessed
wc -l tests/font-google.test.tsRepository: streamloop/vinext
Length of output: 90
🏁 Script executed:
# Read the test file to see what it's doing with the plugin object
head -100 tests/font-google.test.tsRepository: streamloop/vinext
Length of output: 4648
🏁 Script executed:
# Search for where the test sets _isBuild, _fontCache, _cacheDir
rg -n "_isBuild|_fontCache|_cacheDir|plugin\._" tests/font-google.test.ts | head -30Repository: streamloop/vinext
Length of output: 1183
🏁 Script executed:
# Look at the test implementations to see if they actually use the _isBuild, _fontCache, _cacheDir properties
sed -n '275,330p' tests/font-google.test.tsRepository: streamloop/vinext
Length of output: 2459
🏁 Script executed:
# Check if the tests actually call the plugin hooks and whether those hooks read the closure variables
sed -n '275,450p' tests/font-google.test.tsRepository: streamloop/vinext
Length of output: 7082
🏁 Script executed:
# Read the transform hook implementation to verify it uses closure variables, not this._isBuild
sed -n '540,650p' packages/vinext/src/plugins/fonts.tsRepository: streamloop/vinext
Length of output: 4757
🏁 Script executed:
# Check if transform hook references isBuild directly or through this
rg -n "this\._isBuild|this\.isBuild|this\._fontCache|this\.fontCache" packages/vinext/src/plugins/fonts.tsRepository: streamloop/vinext
Length of output: 43
🏁 Script executed:
# Check if the transform hook reads from closure variables (isBuild, fontCache, cacheDir) or from this
sed -n '545,750p' packages/vinext/src/plugins/fonts.ts | head -100Repository: streamloop/vinext
Length of output: 4424
🏁 Script executed:
# Search for where the transform hook reads isBuild
rg -n "if.*isBuild|if.*fontCache|if.*cacheDir" packages/vinext/src/plugins/fonts.ts | head -20Repository: streamloop/vinext
Length of output: 87
🏁 Script executed:
# Read around line 735 to see how isBuild is used in the transform hook
sed -n '730,750p' packages/vinext/src/plugins/fonts.tsRepository: streamloop/vinext
Length of output: 1151
🏁 Script executed:
# Also search for references to fontCache and cacheDir in the handler
sed -n '545,750p' packages/vinext/src/plugins/fonts.ts | grep -n "fontCache\|cacheDir" | head -20Repository: streamloop/vinext
Length of output: 243
🏁 Script executed:
# Let me verify one more thing - check if getGoogleFontsPlugin() returns the actual plugin from vinext()
# and whether accessing _isBuild would create new properties
sed -n '1,50p' packages/vinext/src/index.tsRepository: streamloop/vinext
Length of output: 2057
🏁 Script executed:
# Also check how vinext() returns the plugins
rg -n "export.*function vinext|export default" packages/vinext/src/index.ts | head -10Repository: streamloop/vinext
Length of output: 205
🏁 Script executed:
# Read the getGoogleFontsPlugin function and how it gets and returns the plugin
sed -n '30,40p' tests/font-google.test.tsRepository: streamloop/vinext
Length of output: 666
🏁 Script executed:
# Check the vinext() export to see if it returns the plugin instances
sed -n '855,875p' packages/vinext/src/index.tsRepository: streamloop/vinext
Length of output: 730
🏁 Script executed:
# Let me verify: does the test actually call configResolved before calling transform?
sed -n '270,295p' tests/font-google.test.tsRepository: streamloop/vinext
Length of output: 1146
🏁 Script executed:
# Final verification - check if tests call configResolved to initialize closure variables
grep -n "configResolved" tests/font-google.test.tsRepository: streamloop/vinext
Length of output: 43
🏁 Script executed:
# Let's also check the entire test structure - does any test set up the plugin properly?
sed -n '265,280p' tests/font-google.test.tsRepository: streamloop/vinext
Length of output: 558
Test infrastructure assumes plugin exposes internal state as properties, but the plugin uses closure variables that tests never modify.
The plugin stores state in closure variables defined at lines 530-532 of packages/vinext/src/plugins/fonts.ts:
let isBuild = false;
const fontCache = new Map<string, string>();
let cacheDir = "";
The transform hook reads these closure variables directly (e.g., line 735: if (isBuild), lines 160-164: uses fontCache and cacheDir). However, the test interface assumes these exist as properties (_isBuild, _fontCache, _cacheDir) on the plugin object, and tests set these properties expecting them to affect hook behavior.
Setting plugin._isBuild = true creates a new property on the plugin object but does not affect the closure variable isBuild that the transform hook actually reads. Additionally, tests never call configResolved(), which is the only hook that initializes these closure variables. This means:
- The closure variable
isBuildremainsfalsethroughout all tests, even though tests setplugin._isBuild = true - Tests expecting build-mode behavior are actually testing dev-mode behavior
- The
fontCacheandcacheDirmodifications in tests are silently ignored
The tests are not testing the actual plugin behavior—they're testing against nonexistent state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/font-google.test.ts` around lines 23 - 37, Tests must initialize the
plugin's closure state instead of setting nonexistent properties; call the
plugin's configResolved hook (configResolved({... command: "build", root,
cacheDir, ...})) via getGoogleFontsPlugin() before running assertions to set the
closure variables isBuild, fontCache and cacheDir, and remove any assignments to
plugin._isBuild/_fontCache/_cacheDir; if tests need to pre-populate or inspect
the internal fontCache, either trigger the plugin code paths (e.g., call
transform with appropriate inputs) to populate the closure Map or add a small
test-only API in the plugin (e.g., export a setter/getter) that manipulates the
closure variables rather than relying on object properties.
…parity)
- Fix import regex: [^;\n] prevents cross-line matching
- Add destructure detection for default import pattern (const { Font } = googleFonts)
- Remove isBuild gate — self-hosting runs in dev and build
- Build manual Google Fonts URL to avoid URLSearchParams encoding issues
- Copy woff2 to public/fonts/ for web serving
- Generate hashed font-family names (__FontName_hash)
- Generate fallback @font-face with size-adjust metrics via @capsizecss/metrics
- Inject _hashedFamily, _fallbackCSS, _fallbackFamily into font loader calls
- Runtime: handle hashed families with fallback in font-google-base.ts
- Add font-metrics.ts from PR cloudflare#158 for capsize metric calculation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
blitss
force-pushed
the
streamloop/all-fixes
branch
from
560752f to
fa420b7
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
packages/vinext/README.md (1)
691-693:⚠️ Potential issue | 🟡 MinorPoint the contributor setup and CI docs at this fork.
Lines 691-693 still send contributors to
cloudflare/vinext, and Lines 724-726 still describe the upstream “Cloudflare employees” preview flow. In this fork, that points people at the wrong remote and the wrong preview instructions.Also applies to: 724-726
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vinext/README.md` around lines 691 - 693, Replace the upstream Cloudflare references in the README: change the git clone URL string "git clone https://github.com/cloudflare/vinext.git" to this fork's repository URL and update the "Cloudflare employees" preview flow text (the contributor/CI preview instructions block) so it describes the correct remote and preview steps for this fork; ensure both occurrences (the clone command and the preview flow paragraph) are updated consistently so contributors and CI guidance point to the fork and correct preview workflow.packages/vinext/src/plugins/fonts.ts (1)
575-575:⚠️ Potential issue | 🟠 MajorAllow multiline
next/font/googleimports through this rewrite.Line 575 still excludes
\n, so a formatted import likeimport {\n Inter,\n} from "next/font/google"bypasses the transform and never gets the virtual-module/self-hosted path.Suggested fix
- const importRe = /^[ \t]*import\s+([^;\n]+?)\s+from\s*(["'])next\/font\/google\2\s*;?/gm; + const importRe = + /^[ \t]*import\s+([\s\S]+?)\s+from\s*(["'])next\/font\/google\2\s*;?/gm;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vinext/src/plugins/fonts.ts` at line 575, The current importRe regex (constant importRe) only matches single-line imports because it excludes newline characters, so formatted multi-line imports from "next/font/google" are skipped; update importRe to allow newlines in the import specifier (e.g., replace the [^;\n]+? portion with a pattern that matches across lines such as [\s\S]+? or enable the dot-all behavior) so imports like `import {\n Inter,\n} from "next/font/google"` are correctly captured and rewritten to the virtual/self-hosted path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/vinext/README.md`:
- Line 440: The README’s Google Fonts docs still describe CDN-only loading for
the `next/font/google` entry and the limitations section; update the support
table row for `next/font/google` to reflect the new self-hosted path and
metric-based fallback CSS (adjust the status indicator and short description
accordingly) and revise the limitations/notes paragraph that mentions CDN-only
loading to describe self-hosting plus metric-based fallback behavior and any
changed caveats. Use the exact table key `next/font/google` and the
“limitations”/Google Fonts explanatory paragraph to locate and replace the
outdated text.
In `@packages/vinext/src/plugins/fonts.ts`:
- Around line 378-384: The rewrite currently hard-codes root-relative URLs
(`/fonts/<filename>`) causing 404s for apps served under a subpath; change the
replacement to build the font web path using the app's base/public path rather
than a leading slash. Locate the CSS rewrite that uses
css.split(fontUrl).join(`/fonts/${filename}`) and replace it with a URL built
from a configurable base (e.g., use an existing basePath/publicPath option or an
injected runtime/public prefix), joining segments with posix-safe logic
(path.posix.join) and falling back to 'fonts/<filename>' if no base is provided;
keep the same file copying logic for publicFontsDir/filename. Ensure you
reference the variables fontUrl, filename, publicFontsDir and css when making
the change.
In `@packages/vinext/src/shims/font-google-base.ts`:
- Around line 385-403: The self-hosted branch always calls
injectSelfHostedCSS(options._selfHostedCSS) which causes preload hints to be
collected regardless of the font's preload/subsets settings; update this branch
so injectSelfHostedCSS respects preload and subsets (either by passing the
preload/subsets flags into injectSelfHostedCSS or by conditionally calling it
only when preload is true and applying subsets filtering before calling).
Specifically, modify the block handling options._selfHostedCSS &&
options._hashedFamily to pass options.preload and options.subsets (or check
options.preload) to injectSelfHostedCSS, and ensure injectSelfHostedCSS uses
those values to suppress preload hints when preload is false and to limit
extracted URLs by subsets, keeping injectClassNameRule, injectVariableClassRule
and returned { className, style, variable } behavior unchanged.
---
Duplicate comments:
In `@packages/vinext/README.md`:
- Around line 691-693: Replace the upstream Cloudflare references in the README:
change the git clone URL string "git clone
https://github.com/cloudflare/vinext.git" to this fork's repository URL and
update the "Cloudflare employees" preview flow text (the contributor/CI preview
instructions block) so it describes the correct remote and preview steps for
this fork; ensure both occurrences (the clone command and the preview flow
paragraph) are updated consistently so contributors and CI guidance point to the
fork and correct preview workflow.
In `@packages/vinext/src/plugins/fonts.ts`:
- Line 575: The current importRe regex (constant importRe) only matches
single-line imports because it excludes newline characters, so formatted
multi-line imports from "next/font/google" are skipped; update importRe to allow
newlines in the import specifier (e.g., replace the [^;\n]+? portion with a
pattern that matches across lines such as [\s\S]+? or enable the dot-all
behavior) so imports like `import {\n Inter,\n} from "next/font/google"` are
correctly captured and rewritten to the virtual/self-hosted path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aae27b74-d8b3-43f8-a268-d2e9537b6234
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
.github/workflows/preview-release.ymlpackages/vinext/README.mdpackages/vinext/package.jsonpackages/vinext/src/font-metrics.tspackages/vinext/src/plugins/fonts.tspackages/vinext/src/shims/font-google-base.tspackages/vinext/src/shims/navigation.tspnpm-workspace.yaml
✅ Files skipped from review due to trivial changes (3)
- pnpm-workspace.yaml
- .github/workflows/preview-release.yml
- packages/vinext/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/vinext/src/shims/navigation.ts
| | `next/headers` | ✅ | Async `headers()`, `cookies()`, `draftMode()` | | ||
| | `next/dynamic` | ✅ | `ssr: true`, `ssr: false`, `loading` component | | ||
| | `next/script` | ✅ | All 4 strategies (`beforeInteractive`, `afterInteractive`, `lazyOnload`, `worker`) | | ||
| | `next/font/google` | 🟡 | Runtime CDN loading. No self-hosting, font subsetting, or fallback metrics | |
There was a problem hiding this comment.
Update the Google Fonts docs to match the new self-hosted path.
The Google Fonts portions of Line 440 and Line 560 still describe CDN-only loading with no fallback metrics. This PR adds self-hosting plus metric-based fallback CSS, so the support table and limitations section now understate the feature set.
Also applies to: 560-560
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/vinext/README.md` at line 440, The README’s Google Fonts docs still
describe CDN-only loading for the `next/font/google` entry and the limitations
section; update the support table row for `next/font/google` to reflect the new
self-hosted path and metric-based fallback CSS (adjust the status indicator and
short description accordingly) and revise the limitations/notes paragraph that
mentions CDN-only loading to describe self-hosting plus metric-based fallback
behavior and any changed caveats. Use the exact table key `next/font/google` and
the “limitations”/Google Fonts explanatory paragraph to locate and replace the
outdated text.
Comment on lines
+378
to
+384
| // Copy to public/fonts/ so they're served as static assets | ||
| const publicPath = path.join(publicFontsDir, filename); | ||
| if (fs.existsSync(filePath) && !fs.existsSync(publicPath)) { | ||
| fs.copyFileSync(filePath, publicPath); | ||
| } | ||
| // Rewrite CSS to use /fonts/<filename> web path | ||
| css = css.split(fontUrl).join(`/fonts/${filename}`); |
There was a problem hiding this comment.
Avoid hard-coding root-relative /fonts/* URLs here.
Lines 383-384 rewrite every font reference to /fonts/<filename>. Apps served from a subpath/basePath will request the font from the origin root instead of the app-prefixed URL, so these self-hosted assets 404 outside root deployments.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/vinext/src/plugins/fonts.ts` around lines 378 - 384, The rewrite
currently hard-codes root-relative URLs (`/fonts/<filename>`) causing 404s for
apps served under a subpath; change the replacement to build the font web path
using the app's base/public path rather than a leading slash. Locate the CSS
rewrite that uses css.split(fontUrl).join(`/fonts/${filename}`) and replace it
with a URL built from a configurable base (e.g., use an existing
basePath/publicPath option or an injected runtime/public prefix), joining
segments with posix-safe logic (path.posix.join) and falling back to
'fonts/<filename>' if no base is provided; keep the same file copying logic for
publicFontsDir/filename. Ensure you reference the variables fontUrl, filename,
publicFontsDir and css when making the change.
Comment on lines
+385
to
+403
| // Self-hosted mode with hashed family (Next.js parity) | ||
| if (options._selfHostedCSS && options._hashedFamily) { | ||
| const hashedFamily = options._hashedFamily; | ||
| const fallbackFamily = options._fallbackFamily; | ||
| const className = `__font_${family.toLowerCase().replace(/\s+/g, "_")}_${id}`; | ||
| const variableClassName = `__variable_${family.toLowerCase().replace(/\s+/g, "_")}_${id}`; | ||
|
|
||
| const parts: string[] = [`'${escapeCSSString(hashedFamily)}'`]; | ||
| if (fallbackFamily) parts.push(`'${escapeCSSString(fallbackFamily)}'`); | ||
| parts.push(...fallback.map(sanitizeFallback)); | ||
| const fontFamily = parts.join(", "); | ||
|
|
||
| injectSelfHostedCSS(options._selfHostedCSS); | ||
| if (options._fallbackCSS) injectSelfHostedCSS(options._fallbackCSS); | ||
| injectClassNameRule(className, fontFamily); | ||
| injectVariableClassRule(variableClassName, cssVarName, fontFamily); | ||
|
|
||
| return { className, style: { fontFamily }, variable: variableClassName }; | ||
| } |
There was a problem hiding this comment.
Honor preload in the self-hosted branch.
This branch always funnels through injectSelfHostedCSS(), and that helper unconditionally collects preload hints from the stylesheet. In self-hosted mode, preload: false therefore stops working, and subsets still cannot narrow which extracted URLs are hinted.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/vinext/src/shims/font-google-base.ts` around lines 385 - 403, The
self-hosted branch always calls injectSelfHostedCSS(options._selfHostedCSS)
which causes preload hints to be collected regardless of the font's
preload/subsets settings; update this branch so injectSelfHostedCSS respects
preload and subsets (either by passing the preload/subsets flags into
injectSelfHostedCSS or by conditionally calling it only when preload is true and
applying subsets filtering before calling). Specifically, modify the block
handling options._selfHostedCSS && options._hashedFamily to pass options.preload
and options.subsets (or check options.preload) to injectSelfHostedCSS, and
ensure injectSelfHostedCSS uses those values to suppress preload hints when
preload is false and to limit extracted URLs by subsets, keeping
injectClassNameRule, injectVariableClassRule and returned { className, style,
variable } behavior unchanged.
…er's wrangler.jsonc The @cloudflare/vite-plugin strips env-specific sections when generating the output wrangler.json, so `wrangler deploy --env production` couldn't resolve environment-specific vars/bindings. Now vinext deploy reads the user's original wrangler.jsonc, properly parses JSONC (preserving strings), and merges the env section into dist/server/wrangler.json before deploying. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/vinext/src/deploy.ts (2)
1421-1421: Consider preserving JSON formatting for debuggability.Writing minified JSON makes it harder to inspect the patched config when debugging deployment issues.
💅 Proposed fix
- fs.writeFileSync(distWranglerPath, JSON.stringify(distConfig)); + fs.writeFileSync(distWranglerPath, JSON.stringify(distConfig, null, 2) + "\n");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vinext/src/deploy.ts` at line 1421, The current write call uses fs.writeFileSync(distWranglerPath, JSON.stringify(distConfig)) which writes minified JSON; change the write to pretty-print the config for easier debugging by using JSON.stringify with indentation (e.g., JSON.stringify(distConfig, null, 2)) and include a trailing newline when calling fs.writeFileSync so the file is human-readable; update the call that writes distWranglerPath and references distConfig accordingly.
1378-1412: Consider usingjsonc-parserfor more robust JSONC handling.The custom character-by-character parser works for typical wrangler.jsonc files, but hand-rolled parsers can be fragile for edge cases (nested escapes, unicode, malformed input). The
jsonc-parserpackage (used by VS Code) is lightweight and battle-tested.If keeping the custom parser, it works correctly for common cases.
🔧 Optional: Use jsonc-parser library
+import * as jsonc from "jsonc-parser"; + // In the try block: - const userRaw = fs.readFileSync(userConfigPath, "utf-8"); - // Strip JSONC comments while preserving strings... - let userClean = ""; - // ... (remove entire custom parser) - userClean = userClean.replace(/,\s*([\]}])/g, "$1"); - const userConfig = JSON.parse(userClean); + const userRaw = fs.readFileSync(userConfigPath, "utf-8"); + const userConfig = jsonc.parse(userRaw);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vinext/src/deploy.ts` around lines 1378 - 1412, The manual character-by-character stripping of comments and trimming (variables userRaw, userClean, and the JSON.parse call) is fragile; replace that block with the jsonc-parser library: add the dependency, import the parse (or parseTree/stripComments) functions from "jsonc-parser", then call parse(userRaw, diagnostics, { allowTrailingComma: true }) to get userConfig (and handle diagnostics errors) instead of building userClean and calling JSON.parse; ensure any error handling for malformed input uses the diagnostics returned by jsonc-parser.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/vinext/src/deploy.ts`:
- Around line 1423-1425: The silent catch after the env-patching try block
should capture the error (change to catch (err)) and emit a warning with context
so users know env patching failed; e.g., call the existing logger
(processLogger.warn or logger.warn) with a message like "Failed to patch env for
deploy --env <name>" and include err (or err.message/stack) so the failure is
visible while still allowing deploy to proceed.
---
Nitpick comments:
In `@packages/vinext/src/deploy.ts`:
- Line 1421: The current write call uses fs.writeFileSync(distWranglerPath,
JSON.stringify(distConfig)) which writes minified JSON; change the write to
pretty-print the config for easier debugging by using JSON.stringify with
indentation (e.g., JSON.stringify(distConfig, null, 2)) and include a trailing
newline when calling fs.writeFileSync so the file is human-readable; update the
call that writes distWranglerPath and references distConfig accordingly.
- Around line 1378-1412: The manual character-by-character stripping of comments
and trimming (variables userRaw, userClean, and the JSON.parse call) is fragile;
replace that block with the jsonc-parser library: add the dependency, import the
parse (or parseTree/stripComments) functions from "jsonc-parser", then call
parse(userRaw, diagnostics, { allowTrailingComma: true }) to get userConfig (and
handle diagnostics errors) instead of building userClean and calling JSON.parse;
ensure any error handling for malformed input uses the diagnostics returned by
jsonc-parser.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a8e5d2f3-e361-4419-b09d-2b7c4258d640
📒 Files selected for processing (1)
packages/vinext/src/deploy.ts
Comment on lines
+1423
to
+1425
| } catch { | ||
| // Non-fatal — deploy will proceed with existing config | ||
| } |
There was a problem hiding this comment.
Add a warning log when env patching fails.
Silent error swallowing here could confuse users when wrangler deploy --env <name> subsequently fails due to missing env config. A warning helps diagnose issues.
🛠️ Proposed fix
- } catch {
- // Non-fatal — deploy will proceed with existing config
+ } catch (err) {
+ // Non-fatal — deploy will proceed with existing config
+ console.warn(` Warning: Could not patch wrangler.json with env config: ${err instanceof Error ? err.message : err}`);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } catch { | |
| // Non-fatal — deploy will proceed with existing config | |
| } | |
| } catch (err) { | |
| // Non-fatal — deploy will proceed with existing config | |
| console.warn(` Warning: Could not patch wrangler.json with env config: ${err instanceof Error ? err.message : err}`); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/vinext/src/deploy.ts` around lines 1423 - 1425, The silent catch
after the env-patching try block should capture the error (change to catch
(err)) and emit a warning with context so users know env patching failed; e.g.,
call the existing logger (processLogger.warn or logger.warn) with a message like
"Failed to patch env for deploy --env <name>" and include err (or
err.message/stack) so the failure is visible while still allowing deploy to
proceed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Merges upstream PRs and adds navigation + font fixes:
Upstream PRs merged
Navigation hydration fix
useSyncExternalStoreserver snapshot now uses client snapshot functions (window.location / globalThis state)_clientParamsstored on globalThis viaSymbol.forto fix Vite module duplicationGoogle Fonts self-hosting (Next.js parity)
.vinext/fonts/__Gabarito_a2795c)@font-facewithsize-adjustmetrics via@capsizecss/metricsfont-style: normalin className rules<link rel="stylesheet">, dev: inline<style>)font-google-base.tsand inlined plugin duplicates🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
New Features
Deployment
Documentation