refactor(runtime-vapor): simplify slot fallback model#14899
Draft
edison1105 wants to merge 12 commits into
Draft
refactor(runtime-vapor): simplify slot fallback model#14899edison1105 wants to merge 12 commits into
edison1105 wants to merge 12 commits into
Conversation
Remove the carrier/fallback outlet model and make slot fallback switching rely on the slot boundary state directly. Gate slot dirty tracking behind compiler-emitted slot-root flags so only root v-if/v-for/slot/dynamic component branches register fallback rechecks.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Size ReportBundles
Usages
|
@vue/compiler-core
@vue/compiler-dom
@vue/compiler-sfc
@vue/compiler-ssr
@vue/compiler-vapor
@vue/reactivity
@vue/runtime-core
@vue/runtime-dom
@vue/runtime-vapor
@vue/server-renderer
@vue/shared
vue
@vue/compat
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/runtime-vapor/src/fragment.ts (1)
979-1051: 💤 Low valueComplex recheck logic with multiple branching paths.
The
recheckSlotFallbackfunction handles many state combinations. The logic appears correct, but the deep nesting and multiple conditional paths make it difficult to follow. Consider adding inline comments explaining each major branch for future maintainability.The current implementation correctly handles:
- Early return when rendering fallback
- Content validity transitions
- Fallback insertion/detachment based on validity changes
- Recursive recheck after pending updates
🤖 Prompt for 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. In `@packages/runtime-vapor/src/fragment.ts` around lines 979 - 1051, The recheckSlotFallback function contains multiple complex branches (referencing state.isRenderingFallback, state.activeFallback, isValidBlock, state.isContentValid, clearSlotFallback, insertActiveSlotFallback, renderSlotFallbackState, commitSlotFallback, state.pendingRecheck, state.syncNodes, and state.notifyFallbackValidityChange); add concise inline comments at each major decision point to explain intent and key transitions (early return when isRenderingFallback, the content-valid fast-path that syncs nodes, the contentValid vs fallback branches that detach or insert fallback blocks, the force/render path that calls renderSlotFallbackState and possibly recurses when pendingRecheck, and the final sync + notify step) so future readers can quickly understand why each branch exists and what state changes it performs.
🤖 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.
Nitpick comments:
In `@packages/runtime-vapor/src/fragment.ts`:
- Around line 979-1051: The recheckSlotFallback function contains multiple
complex branches (referencing state.isRenderingFallback, state.activeFallback,
isValidBlock, state.isContentValid, clearSlotFallback, insertActiveSlotFallback,
renderSlotFallbackState, commitSlotFallback, state.pendingRecheck,
state.syncNodes, and state.notifyFallbackValidityChange); add concise inline
comments at each major decision point to explain intent and key transitions
(early return when isRenderingFallback, the content-valid fast-path that syncs
nodes, the contentValid vs fallback branches that detach or insert fallback
blocks, the force/render path that calls renderSlotFallbackState and possibly
recurses when pendingRecheck, and the final sync + notify step) so future
readers can quickly understand why each branch exists and what state changes it
performs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 245f1ef3-0135-4911-b6f4-37adf66082a9
⛔ Files ignored due to path filters (11)
packages/compiler-vapor/__tests__/transforms/__snapshots__/TransformTransition.spec.ts.snapis excluded by!**/*.snappackages/compiler-vapor/__tests__/transforms/__snapshots__/logicalIndex.spec.ts.snapis excluded by!**/*.snappackages/compiler-vapor/__tests__/transforms/__snapshots__/transformElement.spec.ts.snapis excluded by!**/*.snappackages/compiler-vapor/__tests__/transforms/__snapshots__/transformKey.spec.ts.snapis excluded by!**/*.snappackages/compiler-vapor/__tests__/transforms/__snapshots__/transformSlotOutlet.spec.ts.snapis excluded by!**/*.snappackages/compiler-vapor/__tests__/transforms/__snapshots__/transformTemplateRef.spec.ts.snapis excluded by!**/*.snappackages/compiler-vapor/__tests__/transforms/__snapshots__/vFor.spec.ts.snapis excluded by!**/*.snappackages/compiler-vapor/__tests__/transforms/__snapshots__/vHtml.spec.ts.snapis excluded by!**/*.snappackages/compiler-vapor/__tests__/transforms/__snapshots__/vIf.spec.ts.snapis excluded by!**/*.snappackages/compiler-vapor/__tests__/transforms/__snapshots__/vSlot.spec.ts.snapis excluded by!**/*.snappackages/compiler-vapor/__tests__/transforms/__snapshots__/vText.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (25)
packages/compiler-vapor/__tests__/transforms/transformSlotOutlet.spec.tspackages/compiler-vapor/__tests__/transforms/vSlot.spec.tspackages/compiler-vapor/src/generators/block.tspackages/compiler-vapor/src/generators/component.tspackages/compiler-vapor/src/generators/for.tspackages/compiler-vapor/src/generators/if.tspackages/compiler-vapor/src/generators/slotOutlet.tspackages/compiler-vapor/src/ir/index.tspackages/runtime-core/src/apiCreateApp.tspackages/runtime-vapor/__tests__/apiCreateDynamicComponent.spec.tspackages/runtime-vapor/__tests__/componentAttrs.spec.tspackages/runtime-vapor/__tests__/componentSlots.spec.tspackages/runtime-vapor/__tests__/dom/prop.spec.tspackages/runtime-vapor/__tests__/hydration.spec.tspackages/runtime-vapor/__tests__/scopeId.spec.tspackages/runtime-vapor/__tests__/vdomInterop.spec.tspackages/runtime-vapor/src/apiCreateDynamicComponent.tspackages/runtime-vapor/src/apiCreateFor.tspackages/runtime-vapor/src/apiCreateIf.tspackages/runtime-vapor/src/apiDefineAsyncComponent.tspackages/runtime-vapor/src/block.tspackages/runtime-vapor/src/componentSlots.tspackages/runtime-vapor/src/fragment.tspackages/runtime-vapor/src/vdomInterop.tspackages/shared/src/vaporFlags.ts
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/runtime-vapor/src/vdomInterop.ts (1)
1490-1505:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't move the content branch when fallback is active.
On reinsert, this always moves
contentState.renderedand then insertsactiveFallback. IfrecheckSlotFallback()already detached the content branch,internals.m()/insert()will reattach that inactive branch during a move or KeepAlive activation.Suggested fix
} else { - if (isVNode(contentState.rendered)) { + if (fallbackState.activeFallback) { + insertActiveSlotFallback(fallbackState) + } else if (isVNode(contentState.rendered)) { // move vdom content internals.m( contentState.rendered, parentNode, anchor, @@ } else if (contentState.rendered) { // move vapor content insert(contentState.rendered, parentNode, anchor) } - - insertActiveSlotFallback(fallbackState) }🤖 Prompt for 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. In `@packages/runtime-vapor/src/vdomInterop.ts` around lines 1490 - 1505, The current reinsert always moves/attaches contentState.rendered via internals.m or insert then calls insertActiveSlotFallback; change it to skip moving/reattaching the content branch when the slot fallback is active or when recheckSlotFallback() indicates the content was detached: before calling internals.m(contentState.rendered, ...) or insert(contentState.rendered, ...), call or check recheckSlotFallback(fallbackState) / fallbackState active flag and only perform the move/insert when that check shows the content branch is still active; leave insertActiveSlotFallback(fallbackState) as-is so the fallback is inserted when appropriate.
🤖 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 `@packages/runtime-vapor/src/vdomInterop.ts`:
- Around line 1582-1590: The refreshSlotVNode callback only updates frag.nodes,
leaving fallbackState and frag validity stale; update refreshSlotVNode (the
function passed to trackSlotVNodeUpdatesWithRefresh for hydratedContent) to also
recompute and sync the content validity and block validity: call whatever
updates compute the slot's contentState.valid and fallbackState (so
fallbackState.isContentValid() reflects the new content), and update
frag.isBlockValid() (or recompute frag validity) after
resolveVNodeNodes(hydratedContent) runs; ensure this runs in the same callback
used for trackSlotVNodeUpdatesWithRefresh (and still invokes frag.onUpdated) so
a hydrated slot-root branch that flips valid/invalid on first client update
won't retain the old fallback state.
---
Outside diff comments:
In `@packages/runtime-vapor/src/vdomInterop.ts`:
- Around line 1490-1505: The current reinsert always moves/attaches
contentState.rendered via internals.m or insert then calls
insertActiveSlotFallback; change it to skip moving/reattaching the content
branch when the slot fallback is active or when recheckSlotFallback() indicates
the content was detached: before calling internals.m(contentState.rendered, ...)
or insert(contentState.rendered, ...), call or check
recheckSlotFallback(fallbackState) / fallbackState active flag and only perform
the move/insert when that check shows the content branch is still active; leave
insertActiveSlotFallback(fallbackState) as-is so the fallback is inserted when
appropriate.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 77997f1b-f3b5-4401-bdf6-4831e738e615
📒 Files selected for processing (8)
packages/runtime-vapor/__tests__/componentSlots.spec.tspackages/runtime-vapor/__tests__/vdomInterop.spec.tspackages/runtime-vapor/src/component.tspackages/runtime-vapor/src/componentSlots.tspackages/runtime-vapor/src/components/Transition.tspackages/runtime-vapor/src/components/TransitionGroup.tspackages/runtime-vapor/src/fragment.tspackages/runtime-vapor/src/vdomInterop.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/runtime-vapor/src/fragment.ts
edison1105
force-pushed
the
edison/slot-simplify-model
branch
from
bdc52d2 to
d739b9e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Remove the carrier/fallback outlet model and make slot fallback switching rely on
slot boundary state directly. Gate fallback rechecks behind compiler-emitted
slot-root flags, including forwarded Vapor slots, so inactive branches detach
cleanly while only root slot branches notify parent boundaries.
Summary by CodeRabbit
Release Notes