fix(head): only re-apply i18n params to the route they belong to - #4132
fix(head): only re-apply i18n params to the route they belong to#4132jansalwowski wants to merge 2 commits into
Conversation
_useSetI18nParams registers one watcher per instance which re-applies that instance's params to whatever route is current. Suspense keeps the outgoing page mounted while the incoming one resolves, so the page being left stamps its params onto the page being entered. A page that never calls the composable inherits them, and nothing removes them. Record the route the params were set for and re-apply only to that route. The per-navigation merged meta still needs restoring, so the existing behaviour is kept for same-route navigation and split into two tests.
WalkthroughThe routing logic now records the route name that owns stored i18n parameters. Navigation reapplication and delayed updates stop when the current route differs from the owning route. Tests cover same-route restoration, delayed updates after navigation, and cross-route parameter isolation. Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/routing/head.ts`:
- Around line 132-135: Update the _useSetI18nParams setter to capture the owning
route when created, then only assign __DYNAMIC_PARAMS_KEY__ and update
_i18nParamsRoute when router.currentRoute.value.name still matches that owner.
Ignore delayed writes from other routes without replacing the stored owner.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9ca74f26-c32c-4dd1-9850-df3199785caf
📒 Files selected for processing (2)
src/runtime/routing/head.tstest/routing-head.test.ts
Recording the route at set time left the leak reachable through the setter itself: a page whose data resolves after the user has navigated away wrote its params onto the destination route and re-pointed the owner at it, so the guard in the watcher no longer applied. Capture the owning route where the composable is created — in the page's own setup — and check it in both places. A late setter call is now a no-op rather than a write to a route the page no longer occupies.
|
@BobbieGoede any feedback? |
Fixes #4129.
The problem
_useSetI18nParamsregisters one watcher per instance which re-applies that instance's params to whatever route is current, with no binding between the params and the route they describe.<NuxtPage>'s Suspense keeps the outgoing page mounted while the incoming one resolves, so the outgoing watcher is still live when the route commits — and it stamps its params onto the incoming route.A page with its own setter overwrites them (its instance registers later, so its watcher runs last), which hides this for most routes. A page that never calls
useSetI18nParamshas nothing to overwrite them with and simply inherits the previous page's params.Measured in a minimal reproduction on 10.6.0 — the homepage, which never calls the composable, ends up owning an article's slugs after a client-side navigation.
The change
Capture the route the params belong to where the composable is created — in the owning page's
setup()— and check it in both the setter and the navigation watcher.An earlier revision of this PR recorded the route at set time. @coderabbitai pointed out that leaves the leak reachable through the setter itself: a page whose data resolves after the user has navigated away writes its params onto the destination route and re-points the owner at it, defeating the watcher guard. Since the documented pattern is to call the setter after awaiting data, losing that race is ordinary. Capturing at creation closes both paths.
On the test I changed
restores dynamic params after navigationasserted the reported behaviour directly: it set params on/products/big-chair, pushed/, and expected the product's params on the index route. So this PR does flip a covered behaviour, and that deserves an explicit look rather than being buried in the diff.What that test protects is real, though: every navigation builds a fresh merged
meta, so params are lost on navigation and something must re-apply them. Splitting it in two keeps that half and drops only the cross-route bleed:restores dynamic params when navigating within the same route— same route, freshmeta, params re-applied. Passes before and after.does not carry dynamic params onto a different route— fails onmain, passes here.ignores a setter that resolves after the user has navigated away— fails onmain, passes here.If the original intent was broader than I've read it, I'm happy to rework this.
One thing I could not determine
I could not construct a case where the watcher fires usefully on a different route. Instrumenting the composable across a direct load, a navigation to a page with a setter, a navigation to a page without one, and a same-record param change, the setter always ran with
router.currentRoutealready on its own route — so the direct write inset()was sufficient every time, and the only cross-route write the watcher made was the defect itself.That suggests the guard is safe, but it also means I don't know what the unguarded re-apply was originally for. The one case this changes is params set before their route commits:
_i18nParamsRoutewould hold the previous route and the re-apply would now be skipped. I could not make that happen on Nuxt 4.5.2, but I can't rule it out for older Nuxt,keepalive, or a custom<NuxtPage>key where the component is reused. If you know of such a case, comparing on something other than the route name would be the fix — happy to adjust.One more behavioural note
Capturing at creation is stricter than capturing at set: an instance created outside a page — in a plugin or layout — now owns whatever route was current at creation and no-ops elsewhere. I couldn't find such usage here or in the docs, and it was arguably already unsound, but it is wider than the reported bug and worth a deliberate yes/no.
Verification
pnpm test:unit— 467 passedpnpm lint— 0 errors (6 pre-existing jsdoc warnings, unchanged)tsc --noEmit— cleanmain, green hereSummary by CodeRabbit