Repository navigation
Conversation
…ads are in flight loadSuperProperties() assigns _superProperties[token] unconditionally once its AsyncStorage read resolves, so any register/unregister/clear issued while the read was in flight was silently overwritten — e.g. calling unregisterSuperProperty for several names in a row could leave all but the first removal discarded. Calls made before the first load resolved also threw inside the async method, since getSuperProperties() still returned undefined for delete. updateSuperProperties() now bumps a per-token version and loadSuperProperties() skips assigning a read issued before a mutation. The super property mutators await the pending/initial load via the same lazy-load path getSuperProperties() already uses before reading state. reset() clears through updateSuperProperties() so an in-flight read cannot resurrect properties after a reset.
…e first load Mutators that waited for the initial load used the object getSuperProperties() returned, so two register calls queued behind the same load each built on that snapshot and the second discarded the first. Read the current state after the wait instead.
 [Medium impact] The changes affect JavaScript-mode property updates, reads, event tracking, and reset. Fix identity ordering across queued resets before merging. Findings
Reviews (3) · Last reviewed commit: "test(analytics): cover the queue-tail cl..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…age loads Queue register, registerOnce, unregister, clear and reset per token while a load or an earlier change is pending, and make track wait for the queue. A clear or reset that follows a pending registration now wins, and the next event includes the registration. Add tests for the registerSuperPropertiesOnce wait and the reset clear.
… property changes
…changes getSuperProperties now waits for the per-token change queue like track does, so an awaited read after a register/unregister call reflects it; the queued mutators load through mixpanelPersistent directly so they do not wait on their own queue entry. optOutTracking routes its reset through reset(), and reset is queued even when properties are already loaded, so a change issued around a reset applies in call order instead of being undone by the in-flight wipe.
| const identityProps = { | ||
| distinct_id: this.mixpanelPersistent.getDistinctId(token), | ||
| $device_id: this.mixpanelPersistent.getDeviceId(token), | ||
| $user_id: this.mixpanelPersistent.getUserId(token), | ||
| }; | ||
| if (this._superPropertiesQueue[token]) { | ||
| await this._superPropertiesQueue[token]; |
There was a problem hiding this comment.
track() now copies identity before waiting for a queued reset(). With a signed-in user, calling registerSuperProperties(...); reset(); track("AfterLogout") during a pending storage read captures the old user's IDs. The event waits for the reset to finish but still sends those saved IDs, attributing activity after logout to the previous user.
Preserve identity ordering across queued resets while keeping events tracked before identify() on their original identity.
There was a problem hiding this comment.
This is the same on master. reset() replaces the in-memory IDs only after its storage removes finish, so reset(); track() without an await sends the old IDs there too. I checked it on both. This PR does not change when identity is read relative to a reset, so I am leaving it out of scope here.
Fixes #270. In JavaScript mode, super property changes made while an AsyncStorage load is pending are now applied in call order instead of being lost.
loadSuperProperties()assigned_superProperties[token]unconditionally when its read resolved, and the public wrappers are fire-and-forget. That caused two failures:unregisterSuperPropertycalls right afterinit(), while the constructor's load was pending, could leave only the first removal applied.getSuperProperties()asundefined(unregister threw), or merged onto an empty base and persisted over the stored properties.Changes:
javascript/mixpanel-persistent.js:updateSuperProperties()bumps a per-token version, andloadSuperProperties()skips its assignment if the version changed during the read. This is needed because the constructor,initialize()andreset()issue loads outside any ordered path.javascript/mixpanel-main.js: a per-token queue appliesregisterSuperProperties,registerSuperPropertiesOnce,unregisterSuperProperty,clearSuperPropertiesandresetin call order while a load or earlier change is pending. When state is already loaded the queue is bypassed (queueWhenLoaded = false), so mutations stay synchronous as existing callers and tests expect.resetpassestrueso its storage wipe orders against later changes. A failed queued change does not block later ones, and its caller still sees the rejection.mixpanelPersistent.loadSuperProperties()when state is absent, the same idiomgetSuperProperties()uses, so early calls merge onto stored properties.track()andgetSuperProperties()wait on the queue.track()captures event identity before waiting, so a concurrentidentifydoes not rewrite an earlier event.reset()clears throughupdateSuperProperties()so a pre-reset read cannot restore old values, andoptOutTracking()resets through the same queue.I chose the queue over a version guard alone, which covers only the reported unregister case, and over an unconditional
await this.getSuperProperties(token)in each mutator. That one is shorter but makes every mutation async, which breaks three existing tests that assert synchronous behavior when loaded, and it still lets aclearorresetrun out of order. Happy to change direction if you prefer one of those.__tests__/super-properties.test.jsadds 15 cases using mocked storage whose reads stay pending until released. Againstmaster, 11 fail, including the reported three-unregister sequence ({}expected,superProp2andsuperProp3remain). The other 4 guard ordering thatmasteralready gets right, and all 15 pass with the change.Accepting an array of names in
unregisterSuperProperty, as suggested in the issue, is left out because it is an API addition.