fix(nuxt): compare final matched routes when syncing route object
#32899
Conversation
|
|
@nuxt/kit
nuxt
@nuxt/rspack-builder
@nuxt/schema
@nuxt/vite-builder
@nuxt/webpack-builder
commit: |
Walkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/nuxt/src/pages/runtime/plugins/router.ts (2)
115-121: Minor readability refactor: avoid repeated indexingExtract leaf components into locals to reduce repetition and improve clarity.
- if (to.matched[to.matched.length - 1]?.components?.default === from.matched[from.matched.length - 1]?.components?.default) { - syncCurrentRoute() - } + const toLeaf = to.matched[to.matched.length - 1]?.components?.default + const fromLeaf = from.matched[from.matched.length - 1]?.components?.default + if (toLeaf === fromLeaf) { + syncCurrentRoute() + }
115-121: Document the nuance for future maintainersAdd a brief comment noting that we compare leaf components so that manual sync only occurs when the page component is reused (e.g., param/query change), and not during child-to-child transitions that should suspend.
Example:
// Compare leaf (last) matched components. If the page component is reused (e.g. param change), // no suspense is triggered, so we manually sync the route. For child→child transitions with a new // leaf component, suspense will resolve and update the route via `page:finish`.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/nuxt/src/pages/runtime/plugins/router.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Follow standard TypeScript conventions and best practices
Files:
packages/nuxt/src/pages/runtime/plugins/router.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: huang-julien
PR: nuxt/nuxt#29366
File: packages/nuxt/src/app/components/nuxt-root.vue:16-19
Timestamp: 2024-12-12T12:36:34.871Z
Learning: In `packages/nuxt/src/app/components/nuxt-root.vue`, when optimizing bundle size by conditionally importing components based on route metadata, prefer using inline conditional imports like:
```js
const IsolatedPage = route?.meta?.isolate ? defineAsyncComponent(() => import('#build/isolated-page.mjs')) : null
```
instead of wrapping the import in a computed property or importing the component unconditionally.
Learnt from: GalacticHypernova
PR: nuxt/nuxt#26468
File: packages/nuxt/src/components/plugins/loader.ts:24-24
Timestamp: 2024-11-05T15:22:54.759Z
Learning: In `packages/nuxt/src/components/plugins/loader.ts`, the references to `resolve` and `distDir` are legacy code from before Nuxt used the new unplugin VFS and will be removed.
📚 Learning: 2024-12-12T12:36:34.871Z
Learnt from: huang-julien
PR: nuxt/nuxt#29366
File: packages/nuxt/src/app/components/nuxt-root.vue:16-19
Timestamp: 2024-12-12T12:36:34.871Z
Learning: In `packages/nuxt/src/app/components/nuxt-root.vue`, when optimizing bundle size by conditionally importing components based on route metadata, prefer using inline conditional imports like:
```js
const IsolatedPage = route?.meta?.isolate ? defineAsyncComponent(() => import('#build/isolated-page.mjs')) : null
```
instead of wrapping the import in a computed property or importing the component unconditionally.
Applied to files:
packages/nuxt/src/pages/runtime/plugins/router.ts
📚 Learning: 2024-11-05T15:22:54.759Z
Learnt from: GalacticHypernova
PR: nuxt/nuxt#26468
File: packages/nuxt/src/components/plugins/loader.ts:24-24
Timestamp: 2024-11-05T15:22:54.759Z
Learning: In `packages/nuxt/src/components/plugins/loader.ts`, the references to `resolve` and `distDir` are legacy code from before Nuxt used the new unplugin VFS and will be removed.
Applied to files:
packages/nuxt/src/pages/runtime/plugins/router.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build
- GitHub Check: codeql (javascript-typescript)
- GitHub Check: code
🔇 Additional comments (2)
packages/nuxt/src/pages/runtime/plugins/router.ts (2)
118-120: Correct fix: compare leaf (last) matched components to avoid premature syncComparing the last matched records aligns with child-to-child navigation semantics and prevents early route sync before suspense resolves. This should address #32898 as intended.
115-121: No other directmatched[0]checks found
All occurrences of component-equality checks use either the last matched record (inrouter.afterEach) or anevery()check across all matches (inpackages/nuxt/src/app/components/utils.ts), so there’s no remaining logic relying solely onmatched[0].
|
do you think you could add a unit/runtime test? 🙏 |
CodSpeed Performance ReportMerging #32899 will not alter performanceComparing Summary
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
test/fixtures/basic/app/pages/parent/suspense.vue (1)
8-8: Optional: increase delay slightly to reduce flakinessA 1ms timeout can be racy on CI. Consider a small bump (e.g. 10ms) to make the suspense resolution timing more robust:
-await new Promise(resolve => setTimeout(resolve, 1)) +await new Promise(resolve => setTimeout(resolve, 10))test/nuxt/composables.test.ts (2)
475-477: Optional: pre-wire the page:finish hook to avoid any raceIt’s safer to set up the
page:finishpromise before callingnavigateTo, to eliminate any timing edge cases:-function waitForPageChange () { - return new Promise<void>(resolve => nuxtApp.hooks.hookOnce('page:finish', () => resolve())) -} +function nextPageFinish () { + return new Promise<void>(resolve => nuxtApp.hooks.hookOnce('page:finish', resolve)) +}And use:
const wait = nextPageFinish() await navigateTo('/parent') await waitIf you prefer to keep the current flow, please verify it still passes consistently on CI once the SFC/tsconfig issue is resolved.
497-533: Test logic LGTM; add wrapper unmount to prevent leaksThe assertions correctly validate that:
- Before suspense resolves, the parent route/component remains active.
- After
page:finish, the child suspense route is active androute.nameupdates.Add unmount to avoid lingering instances across tests:
-const el = await mountSuspended({ setup: () => () => h(NuxtPage) }) -const route = useRoute() +const el = await mountSuspended({ setup: () => () => h(NuxtPage) }) +const route = useRoute() +try { await navigateTo('/parent') await waitForPageChange() expect(el.html()).toContain('<div> parent/index </div>') expect(route.name).toBe('parent') await navigateTo('/parent/suspense') expect(el.html()).toContain('<div> parent/index </div>') expect(route.name).toBe('parent') await waitForPageChange() expect(el.html()).toContain('<div> parent/suspense </div>') expect(route.name).toBe('parent-suspense') +} finally { + el.unmount() +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/fixtures/basic/app/pages/parent/suspense.vue(1 hunks)test/nuxt/composables.test.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.vue
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Use
<script setup lang="ts">and the composition API when creating Vue components
Files:
test/fixtures/basic/app/pages/parent/suspense.vue
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Follow standard TypeScript conventions and best practices
Files:
test/nuxt/composables.test.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Write unit tests for core functionality using
vitest
Files:
test/nuxt/composables.test.ts
🧠 Learnings (6)
📓 Common learnings
Learnt from: huang-julien
PR: nuxt/nuxt#29366
File: packages/nuxt/src/app/components/nuxt-root.vue:16-19
Timestamp: 2024-12-12T12:36:34.871Z
Learning: In `packages/nuxt/src/app/components/nuxt-root.vue`, when optimizing bundle size by conditionally importing components based on route metadata, prefer using inline conditional imports like:
```js
const IsolatedPage = route?.meta?.isolate ? defineAsyncComponent(() => import('#build/isolated-page.mjs')) : null
```
instead of wrapping the import in a computed property or importing the component unconditionally.
Learnt from: Tofandel
PR: nuxt/nuxt#0
File: :0-0
Timestamp: 2024-11-11T12:34:22.648Z
Learning: Ensure that AI-generated summaries accurately reflect the key changes in the PR, focusing on notable changes such as the removal of unused imports and variables starting with underscores.
Learnt from: GalacticHypernova
PR: nuxt/nuxt#26468
File: packages/nuxt/src/components/plugins/loader.ts:24-24
Timestamp: 2024-11-05T15:22:54.759Z
Learning: In `packages/nuxt/src/components/plugins/loader.ts`, the references to `resolve` and `distDir` are legacy code from before Nuxt used the new unplugin VFS and will be removed.
Learnt from: CR
PR: nuxt/nuxt#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-18T16:46:07.446Z
Learning: Applies to **/e2e/**/*.{ts,js} : Write end-to-end tests using Playwright and `nuxt/test-utils`
📚 Learning: 2025-07-18T16:46:07.446Z
Learnt from: CR
PR: nuxt/nuxt#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-18T16:46:07.446Z
Learning: Applies to **/*.vue : Use `<script setup lang="ts">` and the composition API when creating Vue components
Applied to files:
test/fixtures/basic/app/pages/parent/suspense.vue
📚 Learning: 2024-12-12T12:36:34.871Z
Learnt from: huang-julien
PR: nuxt/nuxt#29366
File: packages/nuxt/src/app/components/nuxt-root.vue:16-19
Timestamp: 2024-12-12T12:36:34.871Z
Learning: In `packages/nuxt/src/app/components/nuxt-root.vue`, when optimizing bundle size by conditionally importing components based on route metadata, prefer using inline conditional imports like:
```js
const IsolatedPage = route?.meta?.isolate ? defineAsyncComponent(() => import('#build/isolated-page.mjs')) : null
```
instead of wrapping the import in a computed property or importing the component unconditionally.
Applied to files:
test/fixtures/basic/app/pages/parent/suspense.vuetest/nuxt/composables.test.ts
📚 Learning: 2025-07-18T16:46:07.446Z
Learnt from: CR
PR: nuxt/nuxt#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-18T16:46:07.446Z
Learning: Applies to **/e2e/**/*.{ts,js} : Write end-to-end tests using Playwright and `nuxt/test-utils`
Applied to files:
test/fixtures/basic/app/pages/parent/suspense.vuetest/nuxt/composables.test.ts
📚 Learning: 2025-07-18T16:46:07.446Z
Learnt from: CR
PR: nuxt/nuxt#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-18T16:46:07.446Z
Learning: Applies to **/*.{test,spec}.{ts,tsx,js,jsx} : Write unit tests for core functionality using `vitest`
Applied to files:
test/nuxt/composables.test.ts
📚 Learning: 2024-11-05T15:22:54.759Z
Learnt from: GalacticHypernova
PR: nuxt/nuxt#26468
File: packages/nuxt/src/components/plugins/loader.ts:24-24
Timestamp: 2024-11-05T15:22:54.759Z
Learning: In `packages/nuxt/src/components/plugins/loader.ts`, the references to `resolve` and `distDir` are legacy code from before Nuxt used the new unplugin VFS and will be removed.
Applied to files:
test/nuxt/composables.test.ts
🪛 GitHub Actions: autofix.ci
test/fixtures/basic/app/pages/parent/suspense.vue
[error] 1-1: Vite plugin 'vite:vue' error caused by missing tsconfig.server.json file during test execution.
test/nuxt/composables.test.ts
[error] 1-1: Test failure in routing utilities: useRoute > should sync route after child suspense resolves. Parsing failed for '/home/runner/work/nuxt/nuxt/test/fixtures/basic/.nuxt/tsconfig.server.json': ENOENT: no such file or directory.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
test/fixtures/basic/app/pages/parent/suspense.vue (1)
1-5: Template looks good for the fixtureThe minimal markup is fine and fits the test’s intent.
test/nuxt/composables.test.ts (2)
25-25: Importing NuxtPage is appropriateUsing
NuxtPagefrom#componentsis the right way to render current route in this test.
472-482: Good test hygiene clearing dynamic routes after eachDefining
waitForPageChange()and callingrouter.clearRoutes()inafterEachavoids test leakage. Looks good.
| <script setup lang="ts"> | ||
| await new Promise(resolve => setTimeout(resolve, 1)) | ||
| </script> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix CI: avoid dependency on missing .nuxt/tsconfig.server.json (remove TS) or commit minimal tsconfig
The autofix.ci pipeline fails because Vite’s vue plugin tries to read test/fixtures/basic/.nuxt/tsconfig.server.json when parsing a TS SFC, which isn’t present in CI. Since this fixture doesn’t use TypeScript features, the simplest fix is to drop lang="ts".
Apply this diff:
-<script setup lang="ts">
+<script setup>
await new Promise(resolve => setTimeout(resolve, 1))
</script>Alternative (keep TS): commit a minimal test/fixtures/basic/.nuxt/tsconfig.server.json to satisfy the plugin.
Example content:
{
"compilerOptions": {
"allowJs": true,
"skipLibCheck": true,
"types": ["node", "vite/client"]
}
}To confirm the root cause and the fix locally/CI:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Check presence of tsconfig.server.json in fixture:"
if [ -f test/fixtures/basic/.nuxt/tsconfig.server.json ]; then
echo "Found: test/fixtures/basic/.nuxt/tsconfig.server.json"
else
echo "Missing: test/fixtures/basic/.nuxt/tsconfig.server.json"
fi
echo
echo "Check if the fixture uses TypeScript in the SFC:"
rg -n 'script setup.*lang="ts"' test/fixtures/basic/app/pages/parent/suspense.vue || trueLength of output: 537
Fix CI failure in basic fixture: drop lang="ts"
Vite’s Vue plugin is attempting to read a missing test/fixtures/basic/.nuxt/tsconfig.server.json when parsing this TS SFC, causing the autofix.ci pipeline to fail. Since this component doesn’t actually use any TypeScript-specific features, removing the lang="ts" attribute resolves the issue.
Please apply the following change in test/fixtures/basic/app/pages/parent/suspense.vue:
--- a/test/fixtures/basic/app/pages/parent/suspense.vue
+++ b/test/fixtures/basic/app/pages/parent/suspense.vue
@@ Lines 7-9
-<script setup lang="ts">
+<script setup>
await new Promise(resolve => setTimeout(resolve, 1))
</script>📝 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.
| <script setup lang="ts"> | |
| await new Promise(resolve => setTimeout(resolve, 1)) | |
| </script> | |
| <script setup> | |
| await new Promise(resolve => setTimeout(resolve, 1)) | |
| </script> |
🤖 Prompt for AI Agents
In test/fixtures/basic/app/pages/parent/suspense.vue around lines 7 to 9, remove
the lang="ts" attribute from the <script setup> tag because the file does not
use TypeScript-specific features and having lang="ts" causes Vite’s Vue plugin
to look for a missing tsconfig.server.json, leading to CI failures. Change
<script setup lang="ts"> to just <script setup>.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
test/nuxt/composables.test.ts (3)
508-519: Optional: avoid runtime template compilation in testsRelying on
templatestrings indefineComponentcan depend on the runtime-compiler build. To keep tests robust across environments, prefer render functions and an asyncsetupto simulate suspense.You can replace both child components as follows:
- component: defineComponent({ - template: '<div> parent/index </div>', - }), + component: defineComponent({ + setup: () => () => h('div', ' parent/index ') + }), ... - component: defineComponent({ - template: '<div> parent/suspense </div>', - setup: () => new Promise(resolve => setTimeout(resolve, 1)), - }), + component: defineComponent({ + async setup () { + await new Promise(resolve => setTimeout(resolve, 1)) + return () => h('div', ' parent/suspense ') + }, + }),
542-544: Deduplicate route cleanupYou already call
router.clearRoutes()inafterEach. Removing the explicitremoveRouteavoids redundant cleanup.Apply:
- el.unmount() - router.removeRoute('parent-test') + el.unmount()
479-481: router.clearRoutes() fully clears all routes—ensure each test sets up its own baselineIt turns out
clearRoutes()doesn’t reset to a built-in default or “fixture” set of routes, but empties the router entirely. In this suite the first test relies on the default catch-all route before cleanup, and subsequent tests add their own routes manually, so it works today—but it’s brittle if tests are reordered or if new tests expect the default routes.Consider one of the following to make the behaviour explicit and robust:
• Move
router.clearRoutes()into a beforeEach, then explicitly add only the routes needed for that test (including the default catch-all if required).
• Or drop the blanketclearRoutes()and userouter.removeRoute(name)in each test’s teardown to remove only the routes that were added.
• If you need a true “reset to baseline,” implement or invoke a helper that re-adds the default routes after clearing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
test/nuxt/composables.test.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Follow standard TypeScript conventions and best practices
Files:
test/nuxt/composables.test.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Write unit tests for core functionality using
vitest
Files:
test/nuxt/composables.test.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: huang-julien
PR: nuxt/nuxt#29366
File: packages/nuxt/src/app/components/nuxt-root.vue:16-19
Timestamp: 2024-12-12T12:36:34.871Z
Learning: In `packages/nuxt/src/app/components/nuxt-root.vue`, when optimizing bundle size by conditionally importing components based on route metadata, prefer using inline conditional imports like:
```js
const IsolatedPage = route?.meta?.isolate ? defineAsyncComponent(() => import('#build/isolated-page.mjs')) : null
```
instead of wrapping the import in a computed property or importing the component unconditionally.
📚 Learning: 2024-12-12T12:36:34.871Z
Learnt from: huang-julien
PR: nuxt/nuxt#29366
File: packages/nuxt/src/app/components/nuxt-root.vue:16-19
Timestamp: 2024-12-12T12:36:34.871Z
Learning: In `packages/nuxt/src/app/components/nuxt-root.vue`, when optimizing bundle size by conditionally importing components based on route metadata, prefer using inline conditional imports like:
```js
const IsolatedPage = route?.meta?.isolate ? defineAsyncComponent(() => import('#build/isolated-page.mjs')) : null
```
instead of wrapping the import in a computed property or importing the component unconditionally.
Applied to files:
test/nuxt/composables.test.ts
🧬 Code Graph Analysis (1)
test/nuxt/composables.test.ts (4)
packages/nuxt/src/app/nuxt.ts (1)
useNuxtApp(544-557)packages/nuxt/src/app/composables/index.ts (3)
useRouter(15-15)useRoute(15-15)navigateTo(15-15)packages/nuxt/src/app/composables/router.ts (3)
useRouter(15-17)useRoute(20-28)navigateTo(119-224)packages/schema/src/types/hooks.ts (1)
NuxtPage(28-50)
🔇 Additional comments (5)
test/nuxt/composables.test.ts (5)
25-25: Importing NuxtPage from#componentsis appropriateThis is the right component to render nested route children in tests. No issues.
472-474: InitialisingnuxtAppandrouterat describe scope is fineConsistent with the rest of the suite and avoids repeated lookups.
475-477: Good choice of synchronisation point withpage:finishWaiting on
page:finishprecisely targets post-suspense completion, matching the bug’s intent and the router change comparing last matched records.
497-521: Test setup accurately reproduces the nested child + suspense scenarioThe custom parent route with two children (one async) mirrors the reported bug and exercises the fix. Nice use of
NuxtPageto render the active child.
523-541: Assertions correctly validate pre- and post-suspense route sync
- Before suspense resolves: DOM still shows the previous child;
route.nameremains 'parent'.- After
page:finish: DOM updates to the async child;route.nameupdates to 'parent-suspense'.This ensures the Nuxt
routeobject reflects the final matched routes only after suspense resolution, which aligns with the fix.
2 participants
🔗 Linked issue
fix #32898
📚 Description
This should fix an issue with syncing the
routeobject during navigation between 2 child routes.