fix(nuxt): force remount suspense when navigating after pending#33991
fix(nuxt): force remount suspense when navigating after pending#33991
Conversation
|
|
test/nuxt/composables.test.ts
Outdated
| } | ||
|
|
||
| afterEach(() => { | ||
| router.clearRoutes() |
There was a problem hiding this comment.
For some reason, this is not working as expected. The router is not fully reset after the test and this affects subsequent tests.
There was a problem hiding this comment.
cc @cernymatej (copied this from you code below)
There was a problem hiding this comment.
@OrbisK looking at it, it seems that while we do clear the routes, we don't reset the router fully... it stays on the /28425/p3 path for subsequent tests, for example:
and we also clear the initial route defined in test/runtime/app/router.options.ts
So maybe, it would be more appropriate to reset the router like this:
const initialRoutes = router.getRoutes()
afterEach(async () => {
await router.replace('/')
router.clearRoutes()
for (const route of initialRoutes) {
router.addRoute(route)
}
})But for some reason, this now causes expect(el.html()).toContain('<div> parent/suspense </div>') to fail 😕 and it requires waiting for page change twice to start working again 😅
await waitForPageChange()
await waitForPageChange()
expect(el.html()).toContain('<div> parent/suspense </div>') // ✅ worksThere was a problem hiding this comment.
Maybe we should implement some robust utilities to work with the router (resetting, awaiting page change with timeout, etc.)
|
thank you! ❤️ maybe use it.fails instead? |
WalkthroughAdds test scaffolding for navigateTo by creating a local Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 0
🧹 Nitpick comments (1)
test/nuxt/composables.test.ts (1)
481-503: Verify the test correctly reproduces the issue without intermediate waits.The test calls
navigateTomultiple times in rapid succession (lines 494-497) without awaiting between calls, then waits only once at the end (line 498). Whilst this may be intentional to reproduce the race condition described in #28425, please verify this accurately reflects the issue scenario.Additionally, consider making the test name more descriptive to explain what behaviour is being tested, e.g.,
'should handle multiple rapid navigateTo calls to dynamic routes with async setup'.Optional: More descriptive test name
- it.todo('#28425', async () => { + it.todo('#28425: should handle multiple rapid navigateTo calls to dynamic routes with async setup', async () => {
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/nuxt/composables.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Follow standard TypeScript conventions and best practices
Files:
test/nuxt/composables.test.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx,vue}: Use clear, descriptive variable and function names
Add comments only to explain complex logic or non-obvious implementations
Keep functions focused and manageable (generally under 50 lines), and extract complex logic into separate domain-specific files
Remove code that is not used or needed
Use error handling patterns consistently
Files:
test/nuxt/composables.test.ts
**/*.{test,spec}.{ts,tsx,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Write unit tests for core functionality using
vitest
Files:
test/nuxt/composables.test.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: nuxt/nuxt PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-25T11:42:16.132Z
Learning: Applies to **/e2e/**/*.{ts,tsx,js} : Write end-to-end tests using Playwright and `nuxt/test-utils`
📚 Learning: 2025-11-25T11:42:16.132Z
Learnt from: CR
Repo: nuxt/nuxt PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-25T11:42:16.132Z
Learning: Applies to **/e2e/**/*.{ts,tsx,js} : Write end-to-end tests using Playwright and `nuxt/test-utils`
Applied to files:
test/nuxt/composables.test.ts
📚 Learning: 2024-12-12T12:36:34.871Z
Learnt from: huang-julien
Repo: nuxt/nuxt PR: 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
📚 Learning: 2025-09-10T14:42:56.647Z
Learnt from: Tofandel
Repo: nuxt/nuxt PR: 33192
File: test/nuxt/use-async-data.test.ts:366-373
Timestamp: 2025-09-10T14:42:56.647Z
Learning: In the Nuxt useAsyncData test "should watch params deeply in a non synchronous way", the foo watcher intentionally updates both params.foo and params.locale using locale.value, simulating a scenario where one watcher consolidates multiple reactive values into a shared params object for testing debounced/non-synchronous behavior.
Applied to files:
test/nuxt/composables.test.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). (19)
- GitHub Check: test-fixtures (windows-latest, built, webpack, default, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, built, vite, default, manifest-off, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, dev, vite-env-api, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, dev, vite, default, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, dev, vite, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, webpack, default, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, dev, vite, default, manifest-off, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, dev, vite-env-api, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, webpack, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, vite, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, vite, default, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, dev, vite, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, vite, async, manifest-off, json, lts/-1)
- GitHub Check: release-pkg-pr-new
- GitHub Check: typecheck (ubuntu-latest, bundler)
- GitHub Check: typecheck (windows-latest, bundler)
- GitHub Check: test-size
- GitHub Check: test-benchmark
- GitHub Check: code
🔇 Additional comments (1)
test/nuxt/composables.test.ts (1)
443-453: LGTM! Test scaffolding follows established patterns.The setup with shared
nuxtAppandrouterinstances mirrors the existing pattern used in theuseRoutetests (lines 529-539), and thewaitForPageChangehelper is correctly implemented usinghookOnce. TheafterEachcleanup ensures routes are cleared between tests.
@nuxt/kit
@nuxt/nitro-server
nuxt
@nuxt/rspack-builder
@nuxt/schema
@nuxt/vite-builder
@nuxt/webpack-builder
commit: |
test/nuxt/composables.test.ts
Outdated
| nuxtApp._processingMiddleware = false | ||
| }) | ||
|
|
||
| it.todo('#28425', async () => { |
There was a problem hiding this comment.
We cant use the .fails here because the test results in an infinite loop that doesnt time out.
|
Merging this PR will improve performance by 11.62%
Performance Changes
Comparing 
Footnotes |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/nuxt/src/pages/runtime/page.ts (1)
180-199: Add missingonErrorcallback to reset state on Suspense failure.The
onPendingandonResolvecallbacks properly manage theisSuspensePendingflag and hook sequencing, withtry/finallyensuring cleanup occurs. However, there is noonErrorcallback to handle Suspense resolution failures, which would leaveisSuspensePending = trueif an error occurs. This violates the error handling consistency guideline and could break subsequent navigations.Add an
onErrorcallback:Example addition
onError: () => { isSuspensePending = false delete nuxtApp._runningTransition }Additionally, verify that the
await nextTick()addition at line 188 does not introduce timing regressions with route synchronisation during concurrent navigations.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/nuxt/src/pages/runtime/page.tstest/nuxt/composables.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/nuxt/composables.test.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Follow standard TypeScript conventions and best practices
Files:
packages/nuxt/src/pages/runtime/page.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx,vue}: Use clear, descriptive variable and function names
Add comments only to explain complex logic or non-obvious implementations
Keep functions focused and manageable (generally under 50 lines), and extract complex logic into separate domain-specific files
Remove code that is not used or needed
Use error handling patterns consistently
Files:
packages/nuxt/src/pages/runtime/page.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: nuxt/nuxt PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-25T11:42:16.132Z
Learning: Applies to **/e2e/**/*.{ts,tsx,js} : Write end-to-end tests using Playwright and `nuxt/test-utils`
Learnt from: huang-julien
Repo: nuxt/nuxt PR: 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.
⏰ 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). (19)
- GitHub Check: test-fixtures (ubuntu-latest, built, rspack, default, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, built, vite-env-api, default, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, vite, default, manifest-off, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, dev, vite, async, manifest-off, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, vite, async, manifest-off, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, dev, vite-env-api, default, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, vite-env-api, default, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, vite, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, rspack, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, vite, async, manifest-on, js, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, dev, vite, default, manifest-off, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, vite-env-api, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, dev, vite, async, manifest-on, js, lts/-1)
- GitHub Check: typecheck (windows-latest, bundler)
- GitHub Check: release-pkg-pr-new
- GitHub Check: typecheck (ubuntu-latest, bundler)
- GitHub Check: test-benchmark
- GitHub Check: test-size
- GitHub Check: code
🔇 Additional comments (2)
packages/nuxt/src/pages/runtime/page.ts (2)
71-72: LGTM! Clear variable declarations for suspense state tracking.The closure-scoped variables
isSuspensePendingandsuspenseKeyare well-named and appropriately initialised to track internal suspense state and enable force remounts.
178-178: Correct use of Suspense key prop.Adding the reactive
suspenseKeyas the Suspensekeyprop is the standard Vue pattern for forcing remounts when the key changes.
| // force suspense remount and restart async tracking | ||
| // if suspense is already pending and page key changed | ||
| if (isSuspensePending && previousPageKey !== key) { | ||
| suspenseKey++ | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "page.ts" -path "*/pages/runtime/*" | head -5Repository: nuxt/nuxt
Length of output: 94
🏁 Script executed:
wc -l packages/nuxt/src/pages/runtime/page.tsRepository: nuxt/nuxt
Length of output: 96
🏁 Script executed:
sed -n '140,170p' packages/nuxt/src/pages/runtime/page.tsRepository: nuxt/nuxt
Length of output: 1420
🏁 Script executed:
grep -n "isSuspensePending" packages/nuxt/src/pages/runtime/page.tsRepository: nuxt/nuxt
Length of output: 258
🏁 Script executed:
sed -n '1,100p' packages/nuxt/src/pages/runtime/page.ts | head -80Repository: nuxt/nuxt
Length of output: 3077
🏁 Script executed:
grep -n "Suspense\|onError\|error" packages/nuxt/src/pages/runtime/page.tsRepository: nuxt/nuxt
Length of output: 608
🏁 Script executed:
sed -n '170,195p' packages/nuxt/src/pages/runtime/page.tsRepository: nuxt/nuxt
Length of output: 1374
🏁 Script executed:
sed -n '100,120p' packages/nuxt/src/pages/runtime/page.tsRepository: nuxt/nuxt
Length of output: 1163
🌐 Web query:
Vue 3 Suspense component error handling onError callback
💡 Result:
Short answer: Suspense has no built‑in onError callback. Use errorCaptured (Options API) or onErrorCaptured (Composition API) in a parent/wrapper component to catch errors from suspended children, set an error state, show an error UI (or retry), and optionally return false to stop propagation. See Vue docs. [1][2]
Example (script setup):
<script setup> import { ref, onErrorCaptured } from 'vue' const error = ref(null) onErrorCaptured((err, instance, info) => { error.value = err // return false to stop propagation to higher handlers/app.config.errorHandler return false }) </script>Error: {{ error.message }}
RetryLoading…
Notes:
- The third arg (info) indicates source (e.g. "setup", "render", "async component loader"). In production it's a short code. [2]
- Don’t rely on Suspense for error boundaries; handle errors with errorCaptured/onErrorCaptured or app.config.errorHandler. [1][2]
Sources:
- Vue Suspense docs — error handling note. [1]
- Vue API docs — onErrorCaptured / errorCaptured details (args, return false behaviour, production codes). [2]
Add error handling to reset isSuspensePending flag on suspense failure.
The logic correctly forces remount when navigating during pending suspense, but the flag is never reset if the suspended component throws an error. Since Vue 3 Suspense provides only onPending and onResolve callbacks (no onError), if an async component fails, isSuspensePending remains true indefinitely. On subsequent navigations where the page key changes, this causes the condition to remain true and incorrectly increments suspenseKey even after the error has been resolved elsewhere.
Add error handling using onErrorCaptured in a wrapper component or use the app error hook to reset the flag when suspense-related errors occur, ensuring the flag reflects the actual suspense state.
|
@OrbisK Can you please tell, how you debug this and why |
🔗 Linked issue
resolves #28425
📚 Description
These tests should recreate the issue as runtime test.