Skip to content

Conversation

alex-snezhko
Copy link
Contributor

@alex-snezhko alex-snezhko commented Sep 27, 2025

close #13881

Use hoisted Symbol() as the auto-generated v-if branch key in the event that any branches of the if block have custom user-defined keys to guarantee uniqueness across the branches with keys specified by the user vs branches with auto-generated keys.

Example use case fixed:

<script setup>
import { ref } from 'vue'

const toggle = ref(true)
</script>

<template>
  <div v-if="toggle" :key="1">1</div>
  <div v-else>2</div>

  <button @click="toggle = !toggle">Toggle</button>
</template>

Summary by CodeRabbit

  • Bug Fixes
    • Corrects key handling across v-if / v-else-if / v-else chains, respecting user-defined keys and avoiding conflicts.
    • Ensures reliable updates when switching branches, including cases with intervening text or whitespace.
  • Refactor
    • Cleans up code generation for conditional branches and prunes empty text nodes for more consistent output.
  • Tests
    • Expanded coverage and snapshots for v-if key scenarios to ensure stability across complex branch combinations.

Copy link

coderabbitai bot commented Sep 27, 2025

Walkthrough

Introduces cross-branch key awareness in v-if transforms by computing and storing anyBranchesHaveUserDefinedKey on IfNode, adjusting codegen to respect user-defined keys across v-if/v-else(-if) branches. Tests are reorganized and expanded to cover key handling scenarios, with helper updates and new snapshots.

Changes

Cohort / File(s) Summary of edits
Transform logic: v-if
packages/compiler-core/src/transforms/vIf.ts
Tracks user-defined keys across v-if/else-if/else branches; adds isEmptyTextNode helper; computes and assigns anyBranchesHaveUserDefinedKey; threads flag into codegen for branch key generation; cleans empty text nodes.
AST typing
packages/compiler-core/src/ast.ts
Adds optional boolean IfNode.anyBranchesHaveUserDefinedKey to carry cross-branch key metadata.
Tests: v-if
packages/compiler-core/__tests__/transforms/vIf.spec.ts
Imports new public export transformBind; augments parseWithIfTransform with expectAllChildrenIfNodes flag; reorganizes tests under “user-defined keys”; adds/updates assertions and snapshots for key/codegen/hoist behavior.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant C as Compiler
    participant T as v-if Transform
    participant B as Branch Analyzer
    participant G as Codegen Builder

    C->>T: processIf(node, siblings)
    T->>B: Collect branches (if/else-if/else)
    B->>B: Scan props for user-defined :key
    B->>B: Set anyBranchesHaveUserDefinedKey = true if any found
    B-->>T: Branch list + cross-branch key flag

    rect rgba(230,245,255,0.5)
    note right of T: New behavior
    T->>G: createCodegenNodeForBranch(branch, hasAnyUserDefinedKeys)
    G->>G: Generate keys<br/>- use user key if present<br/>- else generate non-conflicting key (index/hoisted Symbol)
    end

    G-->>C: If/Else codegen nodes with keys
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested labels

scope: compiler, :hammer: p3-minor-bug

Suggested reviewers

  • edison1105
  • baiwusanyu-c
  • sxzz

Poem

A hop through branches, keys in tow,
I sniff each if and else to know.
One key to mark, none clash today—
The codegen meadow’s clear of hay.
With tidy twigs and symbols bright,
I bound through tests—ears up, delight! 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title succinctly describes the core change in the compiler-core package by indicating that it fixes automatic key generation conflicts for v-if branches, directly reflecting the PR’s intention to avoid collisions between user-defined and compiler-generated keys. It is concise, specific, and clearly tied to the main functionality altered in the changeset.
Linked Issues Check ✅ Passed The pull request enhances the v-if transform by detecting user-defined keys across branches, exposing an anyBranchesHaveUserDefinedKey flag on the IfNode, and using a hoisted Symbol for auto-generated branch keys when any branch has a user key, thereby preventing collisions and ensuring correct mounting behavior for directives as described in issue #13881. The updated codegen logic and accompanying tests align directly with the linked issue’s requirement to avoid key reuse and trigger mounted hooks appropriately.
Out of Scope Changes Check ✅ Passed All modifications, including AST interface extension, v-if transform adjustments, and test reorganizations, directly support the goal of avoiding v-if branch key conflicts; no unrelated features or modules were altered beyond what is necessary to implement and verify the new key-uniqueness logic.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 102 kB 38.6 kB 34.8 kB
vue.global.prod.js 160 kB (+429 B) 58.9 kB (+155 B) 52.4 kB (+111 B)

Usages

Name Size Gzip Brotli
createApp (CAPI only) 46.7 kB 18.3 kB 16.7 kB
createApp 54.7 kB 21.3 kB 19.5 kB
createSSRApp 58.9 kB 23 kB 21 kB
defineCustomElement 60 kB 23 kB 21 kB
overall 68.8 kB 26.5 kB 24.2 kB

Copy link

pkg-pr-new bot commented Sep 27, 2025

Open in StackBlitz

@vue/compiler-core

npm i https://pkg.pr.new/@vue/compiler-core@13936

@vue/compiler-dom

npm i https://pkg.pr.new/@vue/compiler-dom@13936

@vue/compiler-sfc

npm i https://pkg.pr.new/@vue/compiler-sfc@13936

@vue/compiler-ssr

npm i https://pkg.pr.new/@vue/compiler-ssr@13936

@vue/reactivity

npm i https://pkg.pr.new/@vue/reactivity@13936

@vue/runtime-core

npm i https://pkg.pr.new/@vue/runtime-core@13936

@vue/runtime-dom

npm i https://pkg.pr.new/@vue/runtime-dom@13936

@vue/server-renderer

npm i https://pkg.pr.new/@vue/server-renderer@13936

@vue/shared

npm i https://pkg.pr.new/@vue/shared@13936

vue

npm i https://pkg.pr.new/vue@13936

@vue/compat

npm i https://pkg.pr.new/@vue/compat@13936

commit: 26bf932

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
packages/compiler-core/src/transforms/vIf.ts (1)

214-215: Keep anyBranchesHaveUserDefinedKey in sync when appending branches.

Once we push a new branch, we should fold its userKey back into sibling.anyBranchesHaveUserDefinedKey (e.g. sibling.anyBranchesHaveUserDefinedKey ||= !!branch.userKey;). Right now the flag relies entirely on the initial lookahead performed in the v-if arm; if a later transform ever injects an else(-if) branch with a user key after that scan, the metadata would go stale and downstream codegen would miss that fact. Updating it here is cheap and keeps the flag truthful regardless of transform ordering.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5a8aa0b and 26bf932.

⛔ Files ignored due to path filters (1)
  • packages/compiler-core/__tests__/transforms/__snapshots__/vIf.spec.ts.snap is excluded by !**/*.snap
📒 Files selected for processing (3)
  • packages/compiler-core/__tests__/transforms/vIf.spec.ts (4 hunks)
  • packages/compiler-core/src/ast.ts (1 hunks)
  • packages/compiler-core/src/transforms/vIf.ts (8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/compiler-core/__tests__/transforms/vIf.spec.ts (4)
packages/compiler-core/src/runtimeHelpers.ts (1)
  • MERGE_PROPS (47-47)
packages/compiler-core/__tests__/testUtils.ts (1)
  • createObjectMatcher (27-49)
packages/compiler-core/src/index.ts (2)
  • generate (25-25)
  • transformBind (47-47)
packages/compiler-core/src/transforms/vBind.ts (1)
  • transformBind (15-73)
⏰ 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). (3)
  • GitHub Check: Redirect rules
  • GitHub Check: Header rules
  • GitHub Check: Pages changed

@edison1105 edison1105 added scope: compiler 🍰 p2-nice-to-have Priority 2: this is not breaking anything but nice to have it addressed. labels Sep 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍰 p2-nice-to-have Priority 2: this is not breaking anything but nice to have it addressed. scope: compiler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Edge Case: Directives in v-if with specific key (1) will not trigger mounted hook in v-else case with a diffrent Directive.
2 participants