fix(nuxt): keep vnode when leaving deeper nested route #33778
Conversation
|
|
WalkthroughThe vnode-rendering condition in the page runtime was changed: during route transitions the code now returns the previous vnode when either the number of children is the same or an existing vnode is present (hasSameChildren || vnode). If that condition is not met, the existing fallback that returns a null vnode when leaving a route with a null child remains unchanged. Possibly related PRs
Pre-merge checks and finishing touchesโ Passed checks (5 passed)
โจ Finishing touches
๐งช Generate unit tests (beta)
Tip ๐ Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests โ including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Tip โจ Issue Enrichment is now available for GitHub issues!CodeRabbit can now help you manage issues more effectively:
Disable automatic issue enrichmentTo disable automatic issue enrichment, add the following to your issue_enrichment:
auto_enrich:
enabled: falseThanks 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 |
643a47e to
ac62ad4
Compare
@nuxt/kit
@nuxt/nitro-server
nuxt
@nuxt/rspack-builder
@nuxt/schema
@nuxt/vite-builder
@nuxt/webpack-builder
commit: |
There was a problem hiding this comment.
Actionable comments posted: 0
๐งน Nitpick comments (1)
packages/nuxt/src/pages/runtime/page.ts (1)
132-135: Consider updating the comment for precision.The comment states "if leaving a route with an existing child route", but the condition now also returns the old vnode when any previous vnode exists, regardless of whether child routes match. Consider updating to reflect both conditions:
- // if leaving a route with an existing child route, render the old vnode + // if leaving a route with same number of children or any existing vnode, render the old vnode if (hasSameChildren || vnode) { return vnode }
๐ Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
๐ Files selected for processing (1)
packages/nuxt/src/pages/runtime/page.ts(1 hunks)
๐งฐ 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)
๐ 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:
packages/nuxt/src/pages/runtime/page.ts
๐ Additional comments (1)
packages/nuxt/src/pages/runtime/page.ts (1)
133-133: I apologize for the technical difficulty. The repository clone has failed, which prevents me from directly examining the codebase to verify the review comment.Let me assess the review comment based on the information provided:
The review comment contains a well-structured analysis with:
- Detailed scratchpad tracking of the fix logic
- Clear explanation of how the change (
|| vnode) prevents visual flash- Identification of guards and fallback behavior
- A specific verification request for nested route navigation test coverage
However, I cannot complete the verification because:
- Cannot access the actual file to confirm line numbers, context, and code structure
- Cannot search for or verify the existence of nested route navigation tests
- Cannot independently validate the claims about vnode initialization and guard conditions
Given this limitation, the review comment cannot be definitively verified or rewritten with confidence. The comment appears technically sound based on the logic described, but without access to the codebase, I cannot confirm:
- Whether the analysis of line 133's context is accurate
- Whether the guards at lines 113 and 131 function as described
- Whether test coverage for nested routes actually exists or is needed
Would you be able to:
- Provide direct access to view the file content, or
- Confirm whether you'd like me to proceed with the review comment as-is given the verification constraints?
CodSpeed Performance ReportMerging #33778 will not alter performanceComparing Summary
|
๐ Linked issue
Resolves: #33766
๐ Description
Fixes a visual flash when navigating between nested routes with different depths (e.g., 3 levels โ 2 levels).
The condition at line 133 only checked
hasSameChildren(same route depth) before returning the existing vnode. When navigating from a deeper to a shallower route, it would return null even though a valid vnode existed - causing a brief flash.Fix:
if (hasSameChildren) โ if (hasSameChildren || vnode)Now null is only returned when there was no previous content (as the comment always intended).