fix(runtime-dom): re-sync select when v-model is overridden in change handler (fix #10505)#14960
fix(runtime-dom): re-sync select when v-model is overridden in change handler (fix #10505)#14960zzlw wants to merge 1 commit into
Conversation
… handler (fix vuejs#10505) When a `<select v-model>` change handler reassigns the model to a value different from the user's selection (e.g. resetting it to null), the `_assigning` flag added in 2ffb956 for perf caused the `updated` hook to skip `setSelected`, leaving the DOM out of sync with the model. Remember the value produced by the user interaction and only skip the redundant DOM sync while it still matches the model; otherwise re-sync.
📝 WalkthroughWalkthroughAdds an ChangesvModelSelect override fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/runtime-dom/__tests__/directives/vModel.spec.ts (1)
1012-1056: ⚡ Quick winConsider adding test coverage for additional scenarios.
The current test covers the core regression (override to a different non-null value). Consider adding tests for:
- Override to
null(specifically mentioned in issue#10505)- No override (verify optimization still works - no redundant DOM updates when model matches user selection)
These would provide more comprehensive coverage of the fix and ensure the performance optimization is preserved.
🤖 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-dom/__tests__/directives/vModel.spec.ts` around lines 1012 - 1056, Add two additional test cases to complement the existing test in the vModel.spec.ts file. First, create a test case similar to the existing "should sync DOM when the model is overridden in the change handler" test but configure the onUpdate:modelValue handler to override the model value to null instead of 'foo', then verify the DOM correctly reflects null as the selected value. Second, add another test case that verifies the optimization is preserved when there is no override—set up a select element where the onUpdate:modelValue handler allows the model to update to the value the user selected (without override), then verify the DOM updates correctly and that there are no redundant updates performed when the model already matches the user selection.
🤖 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-dom/__tests__/directives/vModel.spec.ts`:
- Around line 1012-1056: Add two additional test cases to complement the
existing test in the vModel.spec.ts file. First, create a test case similar to
the existing "should sync DOM when the model is overridden in the change
handler" test but configure the onUpdate:modelValue handler to override the
model value to null instead of 'foo', then verify the DOM correctly reflects
null as the selected value. Second, add another test case that verifies the
optimization is preserved when there is no override—set up a select element
where the onUpdate:modelValue handler allows the model to update to the value
the user selected (without override), then verify the DOM updates correctly and
that there are no redundant updates performed when the model already matches the
user selection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: da3a30c6-6dbc-42ba-9ffd-1f0763f7b32d
📒 Files selected for processing (2)
packages/runtime-dom/__tests__/directives/vModel.spec.tspackages/runtime-dom/src/directives/vModel.ts
Summary
Fixes #10505.
When a
<select v-model>has a change handler (or watcher) that reassignsthe model to a value different from the option the user just selected
(e.g. resetting it to
null), the select's DOM state is left out of syncwith the model.
Reproduction
From the issue: https://play.vuejs.org/#eNp9Uk1P4zAQ/SuzvtBKkO5q91SllXYRh+VAEXACc4iSSWtwbMsfoVLIf2ds00IRkEMSz3sz8+Z5BvbXmKIPyOas9NgZWXlccgVQOpRYe+hPOt2gXHCWA0IrziJh9/QnWs3rTaXWSCT6NhJX6jQFOEu1qJo2MRP6SoZI+0nILVpdzjLwBe0X0VYKD1jlLAvJB0P9W22jPBAKMoQN5Q0DOBjHcmaIWc7eDUdHV1thPEgSSanecUapPkSq6Iy2Hgaw2MIIrdUdHJFDRzGx1sr51y5R6CKySuetUGt4BhWkXE7ie3rIxeaAene/nNzdE4erNqhc6tA5mEzn0GvRwBDnFC1M9l2LZA/8WCxSw2mmwL5VxgsT3OZjErUEGOPrY7VciysCyeFkz5IdkzU0RivWxYPTipYkteKs1p0REu0q3QvZN9+J4KySUj+dp5i3AY938XqD9eMn8Qe3jTHOLi06tD1tzR7zlV2jz/DZ9QVu6X8P0mIGSexvwCt0Woa8tJH2L0ST7TteUvs/3TldzI0723pUbjdUFJosS3zOaA9Ovxn9Te7v4s+r1SMbXwCvMCS3
A select whose value is cleared/overridden inside the update handler keeps
showing the option the user picked instead of reflecting the new model value.
Root cause
2ffb956(perf: optimize v-model multiple select w/ large lists) introducedan
_assigningflag so theupdatedhook can skip the redundantsetSelectedcall right after a user-initiated change. However, the flag is checked
unconditionally:
If the model is changed to a value other than the one the user selected during
the same flush,
_assigningis stilltrue, sosetSelectedis skipped andthe DOM never gets re-synced.
Fix
Remember the value produced by the user interaction (
_assignedValue) and onlyskip the DOM sync while the model still matches it. When the model has been
overridden, fall back to
setSelected, keeping the original perf optimizationfor the common case (the
looseEqualcheck short-circuits on referenceequality, which is the case for unchanged large lists).
Tests
Added a regression test under
vModel.spec.tsthat fails without the fix(
selectedIndexstays-1instead of pointing to the overridden option) andpasses with it. All existing
runtime-domunit tests continue to pass.Summary by CodeRabbit