Repository navigation
fix(app-router): normalize trailing slash in implicit cache tags - #2390
james-elicx merged 3 commits into
Conversation
commit: |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: normalize trailing slash in implicit cache tags
The fix is correct and well-scoped. I traced the producer/consumer sides:
- Producer (
shims/cache.ts:135-136):revalidatePath()strips the trailing slash (stem = path.endsWith("/") ? path.slice(0,-1) : path) and builds_N_T_${stem || "/"}. - Consumers (the two builders this PR touches): now strip the trailing slash with
pathname.length > 1 && pathname.endsWith("/")before building_N_T_${stem}.
These line up for all cases I checked:
/en/legacy/->_N_T_/en/legacy(matchesrevalidatePath("/en/legacy")andrevalidatePath("/en/legacy/")).- Root
/-> the guardlength > 1keeps it/, producing_N_T_/, matchingrevalidatePathwherestem || "/"yields_N_T_/. Good — the guard is the key detail that prevents the root case from regressing to_N_T_.
The type: "page"/type: "layout" tags were already correct since they derive from segment splitting (empty trailing segment is skipped), so limiting the change to the no-type tag is right.
Dev/prod parity check (per AGENTS.md): I verified the Pages Router path (server/pages-page-response.ts:643) already normalizes via stem || "/", so no parallel gap is left behind. Both App Router builders (app-page-cache.ts and implicit-tags.ts) are used in distinct code paths, so updating both is correct.
Test coverage: The e2e spec correctly exercises the regression — before this fix the render-time tag was _N_T_/en/legacy/ which matched neither withSlash variant. The self-build/serve pattern matches the existing hash-rsc-requests.browser.spec.ts.
No blocking issues. A couple of minor nits below on the test fixtures.
|
Review posted successfully. Summary of my findings on PR #2390: Verdict: Correct, well-scoped fix — no blocking issuesThe fix is sound. I traced both sides of the cache-tag matching:
These line up for all cases including the tricky root Key verifications:
Two minor non-blocking nits posted inline: redundant nested |
Summary
revalidatePath()strips the trailing slash before constructing its_N_T_implicit cache tag. Pages served at trailing-slash URLs (e.g./en/) were building their own tags with the slash, so the tags never matched and revalidation silently did nothing.Fix: normalize the pathname (strip trailing
/) in both tag-building sites before generating the_N_T_tag, matching whatrevalidatePath()produces.Files touched
packages/vinext/src/server/app-page-cache.tsstembefore building_N_T_${stem}tag inbuildAppPageCacheTags()packages/vinext/src/server/implicit-tags.tsbuildPageCacheTags()—normalizedPathnameused for the implicit tagtests/fixtures/app-trailing-slash-isr/app/[lang]/legacy/page.tsx<span id="lang">+RevalidateButtontests/fixtures/app-trailing-slash-isr/app/[lang]/revalidate-button.tsx/api/revalidateand surfaces resulttests/fixtures/app-trailing-slash-isr/app/api/revalidate/route.tsrevalidatePath()with the path supplied by the buttontests/e2e/app-router/trailingslash-revalidate.browser.spec.tstests/app-trailing-slash-isr.test.tsTesting plan
PLAYWRIGHT_PROJECT=app-router-chrome-browser-specific pnpm playwright test trailingslash-revalidatewithSlash=trueandwithSlash=falsevariants passpnpm vitest run tests/app-trailing-slash-isr.test.ts/en/legacy/returns 200 with timestamp content/enredirects to/en/and renders correctlangparamReference
https://github.com/vercel/next.js/blob/canary/test/e2e/app-dir/trailingslash/trailingslash.test.ts
Fix #1832