fix(teleport): remove stale target anchors after invalid target updates#14600
fix(teleport): remove stale target anchors after invalid target updates#14600edison1105 merged 4 commits 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 ⚙️ 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:
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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: |
bdd995d to
e1c2e96
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/runtime-vapor/src/components/Teleport.ts (1)
309-314: Non-null assertion relies on paired creation of anchors.The
removemethod at line 312 uses a non-null assertion (this.targetAnchor!) within theif (this.targetStart)block, assuming that iftargetStartexists,targetAnchormust also exist.While this is currently safe (both anchors are always created together in
mountToTargetandmountChildren), consider guardingtargetAnchorremoval with its own existence check for defensive coding:if (this.targetStart) { remove(this.targetStart, parentNode(this.targetStart)!) this.targetStart = undefined +} +if (this.targetAnchor) { remove(this.targetAnchor!, parentNode(this.targetAnchor!)!) this.targetAnchor = undefined }This matches the pattern used in the runtime-core implementation at
packages/runtime-core/src/components/Teleport.ts:323-328.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/runtime-vapor/src/components/Teleport.ts` around lines 309 - 314, The removal block in Teleport.remove currently uses a non-null assertion on this.targetAnchor inside the if (this.targetStart) branch; change it to defensively check for this.targetAnchor before calling remove to avoid assuming paired creation (referencing methods/fields: Teleport class, targetStart, targetAnchor, remove, mountToTarget, mountChildren). Update the code so that after unsetting this.targetStart you only call remove(this.targetAnchor, parentNode(this.targetAnchor)!) if this.targetAnchor is truthy, then set this.targetAnchor = undefined, mirroring the defensive pattern used in runtime-core.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/runtime-vapor/src/components/Teleport.ts`:
- Around line 309-314: The removal block in Teleport.remove currently uses a
non-null assertion on this.targetAnchor inside the if (this.targetStart) branch;
change it to defensively check for this.targetAnchor before calling remove to
avoid assuming paired creation (referencing methods/fields: Teleport class,
targetStart, targetAnchor, remove, mountToTarget, mountChildren). Update the
code so that after unsetting this.targetStart you only call
remove(this.targetAnchor, parentNode(this.targetAnchor)!) if this.targetAnchor
is truthy, then set this.targetAnchor = undefined, mirroring the defensive
pattern used in runtime-core.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 635017b2-55b2-4c77-a2fc-ef53e2432703
📒 Files selected for processing (4)
packages/runtime-core/__tests__/components/Teleport.spec.tspackages/runtime-core/src/components/Teleport.tspackages/runtime-vapor/__tests__/components/Teleport.spec.tspackages/runtime-vapor/src/components/Teleport.ts
Summary by CodeRabbit
Bug Fixes
Tests