fix(reactivity): handle Set with initial reactive values edge case#12393
Merged
edison1105 merged 4 commits intovuejs:mainfrom Mar 9, 2026
Merged
fix(reactivity): handle Set with initial reactive values edge case#12393edison1105 merged 4 commits intovuejs:mainfrom
Set with initial reactive values edge case#12393edison1105 merged 4 commits intovuejs:mainfrom
Conversation
edison1105
reviewed
Nov 15, 2024
edison1105
reviewed
Nov 15, 2024
edison1105
added
scope: reactivity
🍰 p2-nice-to-have
edison1105
reviewed
Nov 15, 2024
Size ReportBundles
Usages
|
@vue/compiler-core
@vue/compiler-dom
@vue/compiler-sfc
@vue/compiler-ssr
@vue/reactivity
@vue/runtime-core
@vue/runtime-dom
@vue/server-renderer
@vue/shared
vue
@vue/compat
commit: |
edison1105
changed the title
Set with initial reactive values edge case
edison1105
added
ready to merge
📝 WalkthroughWalkthroughA bug fix that corrects Set behavior when initialized with reactive objects. Previously, adding a reactive object that already existed in the Set would incorrectly modify the Set despite Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/reactivity/src/collectionHandlers.ts (1)
172-181:⚠️ Potential issue | 🟠 Major
add()still diverges fromhas()for readonly/shallow proxies.Line 176 only checks
valueToAddplus the originalvalue. Whenvalueis readonly or shallow,valueToAdd === value, so the raw identity is never consulted. That meansreactive(new Set([raw])).has(readonly(raw))returnstrue, butadd(readonly(raw))still mutates the Set. The duplicate check needs to includetoRaw(value)as well to keepadd()aligned withhas().Possible fix
const target = toRaw(this) const proto = getProto(target) + const rawValue = toRaw(value) const valueToAdd = !shallow && !isShallow(value) && !isReadonly(value) - ? toRaw(value) + ? rawValue : value const hadKey = proto.has.call(target, valueToAdd) || - (value !== valueToAdd && proto.has.call(target, value)) + (hasChanged(value, valueToAdd) && proto.has.call(target, value)) || + (hasChanged(rawValue, valueToAdd) && + proto.has.call(target, rawValue)) if (!hadKey) { target.add(valueToAdd) trigger(target, TriggerOpTypes.ADD, valueToAdd, valueToAdd) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/reactivity/src/collectionHandlers.ts` around lines 172 - 181, The add() branch uses valueToAdd and the original value to detect duplicates, but when value is readonly/shallow (so valueToAdd === value) it never checks the raw identity; update the duplicate detection in the hadKey calculation (the logic around valueToAdd, proto.has, and value) to also test proto.has.call(target, toRaw(value)) so that toRaw(value) is consulted like has() does; keep existing checks for valueToAdd and value and then include toRaw(value) to ensure add() aligns with has() for readonly/shallow proxies (referencing symbols: valueToAdd, toRaw, proto.has, target.add, trigger, TriggerOpTypes.ADD).
🧹 Nitpick comments (1)
packages/reactivity/__tests__/reactive.spec.ts (1)
116-125: Assert that duplicateadd()does not trigger effects.This only proves cardinality stays at
1. It won't catch a regression whereadd()still emitsTriggerOpTypes.ADDand re-runs dependents. A smalleffect(() => observedSet.size)counter would lock down the reactive contract this PR is fixing.Proposed test strengthening
test('observing Set with reactive initial value', () => { const observed = reactive({}) const observedSet = reactive(new Set([observed])) + let runs = 0 + + effect(() => { + runs++ + observedSet.size + }) expect(observedSet.has(observed)).toBe(true) expect(observedSet.size).toBe(1) + expect(runs).toBe(1) // expect nothing happens observedSet.add(observed) expect(observedSet.size).toBe(1) + expect(runs).toBe(1) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/reactivity/__tests__/reactive.spec.ts` around lines 116 - 125, The test currently only checks Set cardinality but not whether duplicate add() emits a reactive trigger; add an effect that depends on observedSet.size (e.g. let runs = 0; effect(() => { void observedSet.size; runs++ })) before calling observedSet.add(observed), then call observedSet.add(observed) and assert runs did not increase (stays the same) to ensure no TriggerOpTypes.ADD side-effect; locate this around the existing test using identifiers observed, observedSet, reactive, and effect.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/reactivity/src/collectionHandlers.ts`:
- Around line 172-181: The add() branch uses valueToAdd and the original value
to detect duplicates, but when value is readonly/shallow (so valueToAdd ===
value) it never checks the raw identity; update the duplicate detection in the
hadKey calculation (the logic around valueToAdd, proto.has, and value) to also
test proto.has.call(target, toRaw(value)) so that toRaw(value) is consulted like
has() does; keep existing checks for valueToAdd and value and then include
toRaw(value) to ensure add() aligns with has() for readonly/shallow proxies
(referencing symbols: valueToAdd, toRaw, proto.has, target.add, trigger,
TriggerOpTypes.ADD).
---
Nitpick comments:
In `@packages/reactivity/__tests__/reactive.spec.ts`:
- Around line 116-125: The test currently only checks Set cardinality but not
whether duplicate add() emits a reactive trigger; add an effect that depends on
observedSet.size (e.g. let runs = 0; effect(() => { void observedSet.size;
runs++ })) before calling observedSet.add(observed), then call
observedSet.add(observed) and assert runs did not increase (stays the same) to
ensure no TriggerOpTypes.ADD side-effect; locate this around the existing test
using identifiers observed, observedSet, reactive, and effect.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 70c73362-eed7-404e-b512-6f16417d49d1
📒 Files selected for processing (2)
packages/reactivity/__tests__/reactive.spec.tspackages/reactivity/src/collectionHandlers.ts
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
2 participants
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.
fix #8647
Summary by CodeRabbit
Bug Fixes
Tests