fix(runtime-vapor): preserve slot owner rendering context in resolveDynamicComponent#14475
Merged
edison1105 merged 1 commit intominorfrom Feb 26, 2026
Merged
fix(runtime-vapor): preserve slot owner rendering context in resolveDynamicComponent#14475edison1105 merged 1 commit intominorfrom
edison1105 merged 1 commit intominorfrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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: |
1bf0291 to
66f88c7
Compare
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 (2)
packages/runtime-vapor/src/apiCreateFor.ts (1)
177-405:⚠️ Potential issue | 🟠 MajorUse
try/finallyforsetCurrentRenderingInstancerestoration inrenderList.The current flow can leave
currentRenderingInstancestuck if an error happens during patch/mount work.🔧 Proposed hardening
- const prevOwner = setCurrentRenderingInstance(scopeOwner) - parent = parent || parentAnchor!.parentNode - if (!oldLength) { + const prevOwner = setCurrentRenderingInstance(scopeOwner) + try { + parent = parent || parentAnchor!.parentNode + if (!oldLength) { // ... - } - setCurrentRenderingInstance(prevOwner) + } + } finally { + setCurrentRenderingInstance(prevOwner) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/runtime-vapor/src/apiCreateFor.ts` around lines 177 - 405, The code sets prevOwner via setCurrentRenderingInstance(scopeOwner) but only restores it at the end with setCurrentRenderingInstance(prevOwner), which can be skipped if an exception occurs during the list patch/mount/unmount work; wrap the large body between the prevOwner assignment and the final setCurrentRenderingInstance call in a try/finally and move setCurrentRenderingInstance(prevOwner) into the finally block so that restoration always happens even if mount/update/unmount (e.g., mount, unmount, update, insert, moveLink) throws; keep the same scope and ensure any early returns (if present) still restore by returning inside the try while the finally restores prevOwner.packages/runtime-vapor/src/fragment.ts (1)
197-213:⚠️ Potential issue | 🟠 MajorProtect rendering-context restoration with
try/finally.If
render(or code after it) throws,currentRenderingInstance/KeepAlive context restoration is skipped, which can leak context across updates.🔧 Proposed hardening
- const prevOwner = setCurrentRenderingInstance(this.slotOwner) - // set currentKeepAliveCtx so nested DynamicFragments and components can capture it - const prevCtx = setCurrentKeepAliveCtx(keepAliveCtx) + const prevOwner = setCurrentRenderingInstance(this.slotOwner) + // set currentKeepAliveCtx so nested DynamicFragments and components can capture it + const prevCtx = setCurrentKeepAliveCtx(keepAliveCtx) + let prev: + | [GenericComponentInstance | null, EffectScope | undefined] + | undefined let prevBranchKey: any - if (keepAliveCtx && this.keyed) { - prevBranchKey = keepAliveCtx.setCurrentBranchKey(this.current) - } - // switch current instance to parent instance during update - // ensure that the parent instance is correct for nested components - const prev = parent && instance ? setCurrentInstance(instance) : undefined - this.nodes = this.scope.run(render) || [] - if (prev !== undefined) setCurrentInstance(...prev) - if (keepAliveCtx && this.keyed) { - keepAliveCtx.setCurrentBranchKey(prevBranchKey) - } - setCurrentKeepAliveCtx(prevCtx) - setCurrentRenderingInstance(prevOwner) + try { + if (keepAliveCtx && this.keyed) { + prevBranchKey = keepAliveCtx.setCurrentBranchKey(this.current) + } + // switch current instance to parent instance during update + // ensure that the parent instance is correct for nested components + prev = parent && instance ? setCurrentInstance(instance) : undefined + this.nodes = this.scope.run(render) || [] + } finally { + if (prev !== undefined) setCurrentInstance(...prev) + if (keepAliveCtx && this.keyed) { + keepAliveCtx.setCurrentBranchKey(prevBranchKey) + } + setCurrentKeepAliveCtx(prevCtx) + setCurrentRenderingInstance(prevOwner) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/runtime-vapor/src/fragment.ts` around lines 197 - 213, Wrap the call to this.scope.run(render) and the subsequent context-restoration in a try/finally: capture prevOwner (from setCurrentRenderingInstance), prevCtx (from setCurrentKeepAliveCtx), prevBranchKey (from keepAliveCtx.setCurrentBranchKey) and prev (from setCurrentInstance(instance)) before calling this.scope.run(render), then in a finally block always restore those contexts by calling setCurrentInstance(prev), keepAliveCtx.setCurrentBranchKey(prevBranchKey) (if used), setCurrentKeepAliveCtx(prevCtx), and setCurrentRenderingInstance(prevOwner) so context cannot leak if render throws.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/runtime-vapor/src/component.ts`:
- Around line 348-350: The code sets prevSlotOwner via
setCurrentRenderingInstance(null) but restores it only after potentially
throwing work; wrap the initialization/child rendering block in a try...finally
so setCurrentRenderingInstance(prevSlotOwner) is always called, and apply the
same try/finally pattern to the other restore site around lines handling
prevSlotOwner (the other setCurrentRenderingInstance call at the secondary
location). Ensure you keep the assignment const prevSlotOwner =
setCurrentRenderingInstance(null) and call
setCurrentRenderingInstance(prevSlotOwner) inside finally to guarantee
restoration of currentRenderingInstance even on exceptions.
---
Outside diff comments:
In `@packages/runtime-vapor/src/apiCreateFor.ts`:
- Around line 177-405: The code sets prevOwner via
setCurrentRenderingInstance(scopeOwner) but only restores it at the end with
setCurrentRenderingInstance(prevOwner), which can be skipped if an exception
occurs during the list patch/mount/unmount work; wrap the large body between the
prevOwner assignment and the final setCurrentRenderingInstance call in a
try/finally and move setCurrentRenderingInstance(prevOwner) into the finally
block so that restoration always happens even if mount/update/unmount (e.g.,
mount, unmount, update, insert, moveLink) throws; keep the same scope and ensure
any early returns (if present) still restore by returning inside the try while
the finally restores prevOwner.
In `@packages/runtime-vapor/src/fragment.ts`:
- Around line 197-213: Wrap the call to this.scope.run(render) and the
subsequent context-restoration in a try/finally: capture prevOwner (from
setCurrentRenderingInstance), prevCtx (from setCurrentKeepAliveCtx),
prevBranchKey (from keepAliveCtx.setCurrentBranchKey) and prev (from
setCurrentInstance(instance)) before calling this.scope.run(render), then in a
finally block always restore those contexts by calling setCurrentInstance(prev),
keepAliveCtx.setCurrentBranchKey(prevBranchKey) (if used),
setCurrentKeepAliveCtx(prevCtx), and setCurrentRenderingInstance(prevOwner) so
context cannot leak if render throws.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/runtime-core/src/compat/renderFn.tspackages/runtime-core/src/componentCurrentInstance.tspackages/runtime-core/src/componentRenderContext.tspackages/runtime-core/src/index.tspackages/runtime-core/src/vnode.tspackages/runtime-vapor/__tests__/apiCreateDynamicComponent.spec.tspackages/runtime-vapor/src/apiCreateFor.tspackages/runtime-vapor/src/component.tspackages/runtime-vapor/src/componentSlots.tspackages/runtime-vapor/src/fragment.ts
1e9a133 to
2c339a5
Compare
2c339a5 to
cd23d73
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.
close #14474
Summary by CodeRabbit
Release Notes
Refactor
Tests