fix: apply before_send to identify and groupIdentify events #2550
Conversation
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
|
@MohamedH1998 is attempting to deploy a commit to the PostHog Team on Vercel. A member of the Team first needs to authorize it. |
MohamedH1998
commented
Nov 5, 2025
| $set_once: userPropsOnce, | ||
| }, | ||
| { disableGeoip } | ||
| this.addPendingPromise( |
Author
There was a problem hiding this comment.
this is consistent w/ capture method implementation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
packages/node/src/client.ts
Outdated
Comment on lines
1148
to
1173
| this.prepareEventMessage({ | ||
| distinctId: distinctId || `$${groupType}_${groupKey}`, | ||
| event: '$groupidentify', | ||
| properties: { | ||
| $group_type: groupType, | ||
| $group_key: groupKey, | ||
| $group_set: properties || {}, | ||
| }, | ||
| disableGeoip, | ||
| }) | ||
| .then(({ distinctId, properties, options }) => { | ||
| const { $group_type, $group_key, $group_set, ...eventProperties } = properties | ||
| return super.groupIdentifyStateless( | ||
| $group_type as string, | ||
| $group_key as string | number, | ||
| $group_set as PostHogEventProperties, | ||
| options, | ||
| distinctId, | ||
| eventProperties | ||
| ) | ||
| }) | ||
| .catch((err) => { | ||
| if (err) { | ||
| console.error(err) | ||
| } | ||
| }) |
Contributor
There was a problem hiding this comment.
logic: missing addPendingPromise() wrapper around promise chain
both identify() (line 437) and identifyImmediate() (line 483) wrap their promise chains with addPendingPromise(), but groupIdentify() doesn't. this means the promise won't be tracked in the promise queue, potentially causing events to be lost during flush() operations.
wrap the entire promise chain with this.addPendingPromise(...) like the other methods do.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/node/src/client.ts
Line: 1148:1173
Comment:
**logic:** missing `addPendingPromise()` wrapper around promise chain
both `identify()` (line 437) and `identifyImmediate()` (line 483) wrap their promise chains with `addPendingPromise()`, but `groupIdentify()` doesn't. this means the promise won't be tracked in the promise queue, potentially causing events to be lost during `flush()` operations.
wrap the entire promise chain with `this.addPendingPromise(...)` like the other methods do.
How can I resolve this? If you propose a fix, please make it concise.
MohamedH1998
commented
Nov 5, 2025
packages/node/src/client.ts
Outdated
| groupIdentify({ groupType, groupKey, properties, distinctId, disableGeoip }: GroupIdentifyMessage): void { | ||
| super.groupIdentifyStateless(groupType, groupKey, properties, { disableGeoip }, distinctId) | ||
| this.prepareEventMessage({ | ||
| distinctId: distinctId || `$${groupType}_${groupKey}`, |
Author
There was a problem hiding this comment.
this is how groupIdentifyStateless also sets distinctId when the incoming param is null
MohamedH1998
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Problem
An issue was raised where the
before_sendcallback was only invoked forcaptureevents, but not foridentifyorgroupIdentifyevents. This meant that users were unable to modify these event types viabefore_send.Changes
identify()andidentifyImmediate()methods to useprepareEventMessage()groupIdentify()to useprepareEventMessage()before_sendfunctionality for the updated methodsRelease info Sub-libraries affected
Libraries affected
Checklist
If releasing new changes
pnpm changesetto generate a changeset file