fix($subscribe): prevent duplicate watcher when same callback subscribed twice - #3144
Merged
posva merged 2 commits intoJun 30, 2026
Merged
Conversation
…bed twice When a callback function is passed to `$subscribe` more than once, the subscriptions Set correctly stores it only once (Set.add is idempotent), but the previous code unconditionally created a new Vue watcher on every call. This meant that N subscriptions of the same callback reference produced N active watchers, each independently calling the callback on direct state mutations (store.prop = value), while $patch correctly called it only once via triggerSubscriptions (which iterates the Set). The inconsistency was introduced when subscriptions were changed from an Array to a Set in vuejs#2887 to prevent duplicates: the Set-level dedup worked for $patch but the watcher creation was never guarded. Fix: check whether the callback is already in the Set before calling scope.run(() => watch(...)). For a duplicate $subscribe call the guard is a no-op (the Set entry already exists), so the returned cleanup function still removes the callback from the Set when invoked, and the initialised-to-noop stopWatcher makes the cleanup closure safe to call. Add a regression test (both Options Store and Setup Store variants via the describe.each runner) that subscribes the same spy twice and asserts it is invoked exactly once per direct mutation and once per $patch, and that unsubscribing stops further calls.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthrough
Changes$subscribe duplicate watcher fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@packages/pinia/src/store.ts`:
- Around line 452-478: The $subscribe cleanup in store.ts is still tied to each
addSubscription call, so duplicate unsubscribe order can leave the watcher from
watch() alive after the shared subscription entry is removed. Update the
subscription handling around addSubscription, stopWatcher, and the watch() setup
so the cleanup for a given callback is shared across all $subscribe() calls and
only tears down the watcher when the last subscription is removed. Ensure the
watcher created in the alreadySubscribed block and the cleanup returned by
removeSubscription stay coordinated for duplicate unsubscribes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6ba21316-2f16-444f-a006-d71acbfa2e94
📒 Files selected for processing (2)
packages/pinia/__tests__/subscriptions.spec.tspackages/pinia/src/store.ts
posva
approved these changes
Jun 30, 2026
posva
left a comment
posva
left a comment
Member
There was a problem hiding this comment.
Thanks! Simplified the implementation
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
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.
Closes #3143
Bug
When the same callback reference is passed to
$subscribemore than once the subscriptionsSetcorrectly stores it only once — so$patch(which iterates the Set viatriggerSubscriptions) calls it only once. But$subscribeunconditionally created a new Vuewatch()on every call, causing each watcher to fire the callback independently on direct state mutations (store.prop = value). N duplicate calls therefore produced N calls per mutation instead of one.This inconsistency was introduced in #2887 when subscriptions were changed from an
Arrayto aSet.Fix
Before calling
scope.run(() => watch(...)), check whether the callback is already in the subscriptions Set. If it is, skip creating a new watcher. ThestopWatchervariable is initialised tonoopso the cleanup closure remains safe to call in the duplicate case.Verification
RUN v4.0.18 /tmp/oss-pr-loop3-20260616/pinia
Coverage enabled with v8
✓ |pinia| tests/subscriptions.spec.ts (34 tests) 60ms
Test Files 1 passed (1)
Tests 34 passed (34)
Type Errors no errors
Start at 12:19:16
Duration 1.07s (transform 197ms, setup 284ms, import 37ms, tests 60ms, environment 288ms)
% Coverage report from v8
-------------------|---------|----------|---------|---------|-------------------
File | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
-------------------|---------|----------|---------|---------|-------------------
All files | 38.75 | 37.02 | 28.3 | 38.11 |
pinia/src | 45.63 | 44.25 | 32.96 | 44.76 |
createPinia.ts | 55.55 | 36.36 | 50 | 56 | 32,39-44,59,73-78
diagnostics.ts | 20 | 100 | 0 | 20 | 18-40
env.ts | 100 | 100 | 100 | 100 |
global.d.ts | 0 | 0 | 0 | 0 |
...Extensions.ts | 0 | 0 | 0 | 0 |
mapHelpers.ts | 5.71 | 0 | 0 | 5.71 | 76-286,397-550
rootStore.ts | 44.44 | 6.66 | 33.33 | 37.5 | 49-57
store.ts | 49 | 48.26 | 42.55 | 48.36 | ...73,898,925-934
storeToRefs.ts | 0 | 0 | 0 | 0 | 90-115
subscriptions.ts | 100 | 100 | 80 | 100 |
types.ts | 100 | 66.66 | 100 | 100 | 24
testing/src | 0 | 0 | 0 | 0 |
diagnostics.ts | 0 | 100 | 100 | 0 | 9
...oreGetters.ts | 0 | 100 | 0 | 0 | 15
testing.ts | 0 | 0 | 0 | 0 | 113-282
-------------------|---------|----------|---------|---------|-------------------
The new tests (one per store flavour via the existing
describe.each) subscribe the same spy twice and assert:$patchfires spy onceSummary by CodeRabbit
store.$subscribe()registrations so the same callback no longer fires more than once per mutation.