Skip to content

Conversation

skirtles-code
Copy link
Contributor

@skirtles-code skirtles-code commented Oct 7, 2025

The PRs #8639 and #13637 both attempted similar fixes that would have introduced regressions. But the existing tests didn't catch those regressions.

This PR introduces an extra test for that case.

Specifically, when an array or set is used for the value of an <input type="checkbox">, mutating the array or set should lead to the checked state being updated accordingly. It isn't enough to use an === check to compare the old and new values, as a mutated array/set won't pass that check.

This PR could also be seen as a test case for 2937530, though that wasn't my initial intention.

Also note that the fix in 2937530 only works when vModelCheckbox or vModelSelect are used directly, not when they're wrapped with vModelDynamic. The tests in vModel.spec.ts do generally use vModelDynamic, but as it's missing the deep: true it won't trigger rendering updates. I haven't attempted to fix that here (it isn't immediately clear to me whether it's safe to add deep: true to vModelDynamic), but I've used vModelCheckbox directly in my test case to dodge that problem. In practice, using vModelCheckbox more accurately reflects real usage anyway.

Summary by CodeRabbit

  • Tests
    • Added comprehensive tests to ensure checkbox bindings stay in sync when the bound value is an array or set, covering operations like additions, index replacements, and set mutations.
    • Expanded coverage for directive-driven checkbox models to validate state synchronization across common mutation scenarios, improving reliability and reducing regression risk in reactive form behavior.

Copy link

coderabbitai bot commented Oct 7, 2025

Walkthrough

Adds a new unit test in runtime-dom directive tests to verify vModelCheckbox correctly handles Array and Set bindings, ensuring checkbox state remains in sync after mutations (push, index replacement, and Set add/delete). Updates imports to include vModelCheckbox for directive-based checkbox binding.

Changes

Cohort / File(s) Summary
Runtime DOM directive tests
packages/runtime-dom/__tests__/directives/vModel.spec.ts
Added a test "should support mutating an array or set value for a checkbox" validating checkbox state sync with Array/Set mutations; extended imports to include vModelCheckbox from @vue/runtime-dom.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant U as User
  participant C as Checkbox (DOM)
  participant D as vModelCheckbox (Directive)
  participant M as Model (Array/Set)
  participant R as Renderer

  Note over C,D: Initial bind
  D->>M: Read current value (Array/Set)
  D->>C: Set checked based on membership

  U->>C: Toggle click
  C->>D: change event
  D->>M: Add/Remove value in Array/Set
  M-->>D: Updated collection
  D->>C: Reflect checked state

  Note over M,R: External mutation
  U->>M: push/replace / add/delete
  M-->>R: Trigger reactive update
  R->>D: Patch with new value
  D->>C: Recompute checked from membership
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

I twitch my whiskers, tests aligned,
Arrays and Sets now stay in kind.
Check, uncheck—no state astray,
The model hops the DOM’s ballet.
With one small tick, we verify—
A checkbox true, by and by. 🐇✅

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title clearly captures the core change of adding a v-model checkbox test that mutates array or Set values, making the primary update immediately apparent without extraneous detail. It succinctly specifies the test scope and intent, aligning directly with the changeset. Its concise phrasing ensures readability and clarity for anyone reviewing pull request history.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c16f8a9 and 6354019.

📒 Files selected for processing (1)
  • packages/runtime-dom/__tests__/directives/vModel.spec.ts (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/runtime-dom/__tests__/directives/vModel.spec.ts (1)
packages/runtime-dom/src/directives/vModel.ts (1)
  • vModelCheckbox (120-159)
⏰ 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
🔇 Additional comments (2)
packages/runtime-dom/__tests__/directives/vModel.spec.ts (2)

8-8: LGTM!

The import of vModelCheckbox is correctly added to support the new test case that uses the directive directly.


1450-1495: Excellent test coverage for array/Set mutations!

The test correctly uses vModelCheckbox directly (instead of vModelDynamic) to leverage the deep: true configuration, ensuring mutations to the same array/Set instance trigger checkbox state updates. The test comprehensively covers:

  • Array mutations (push, index replacement)
  • Set operations (creation, add, delete)

All assertions properly verify the checkbox's checked state after each mutation.


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

github-actions bot commented Oct 7, 2025

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 58.7 kB 52.2 kB

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 Oct 7, 2025

Open in StackBlitz

@vue/compiler-core

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

@vue/compiler-dom

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

@vue/compiler-sfc

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

@vue/compiler-ssr

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

@vue/reactivity

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

@vue/runtime-core

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

@vue/runtime-dom

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

@vue/server-renderer

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

@vue/shared

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

vue

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

@vue/compat

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

commit: 6354019

@edison1105 edison1105 merged commit 079010a into vuejs:main Oct 9, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants