fix: apply before_send to identify and groupIdentify events #2550

MohamedH1998 · 2025-11-05T23:20:37Z

Problem

An issue was raised where the before_send callback was only invoked for capture events, but not for identify or groupIdentify events. This meant that users were unable to modify these event types via before_send.

Changes

Refactor identify() and identifyImmediate() methods to use prepareEventMessage()
Refactor groupIdentify() to use prepareEventMessage()
Add tests for before_send functionality for the updated methods

Release info Sub-libraries affected

Libraries affected

Checklist

Tests for new code
Accounted for the impact of any changes across different platforms
Accounted for backwards compatibility of any changes (no breaking changes!)
Took care not to unnecessarily increase the bundle size

If releasing new changes

Ran pnpm changeset to generate a changeset file
Added the "release" label to the PR to indicate we're publishing new versions for the affected packages

vercel · 2025-11-05T23:20:42Z

@MohamedH1998 is attempting to deploy a commit to the PostHog Team on Vercel.

A member of the Team first needs to authorize it.

MohamedH1998 · 2025-11-05T23:22:03Z

packages/node/src/client.ts

-        $set_once: userPropsOnce,
-      },
-      { disableGeoip }
+    this.addPendingPromise(


this is consistent w/ capture method implementation

greptile-apps

_{2 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2025-11-05T23:25:51Z

packages/node/src/client.ts

+    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)
+        }
+      })


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 · 2025-11-05T23:26:21Z

packages/node/src/client.ts

  groupIdentify({ groupType, groupKey, properties, distinctId, disableGeoip }: GroupIdentifyMessage): void {
-    super.groupIdentifyStateless(groupType, groupKey, properties, { disableGeoip }, distinctId)
+    this.prepareEventMessage({
+      distinctId: distinctId || `$${groupType}_${groupKey}`,


this is how groupIdentifyStateless also sets distinctId when the incoming param is null

posthog-bot · 2025-11-13T09:35:43Z

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

pauldambra · 2025-11-13T09:39:35Z

hey @MohamedH1998 i'm not familiar with the node SDK 🙈 i'll find out who is best to review - thanks for the PR!

can you expand on the things you want to edit in the identify event?
(we're slightly more careful with some events than others and i'm not sure why it was excluded in the first implementation)

MohamedH1998 · 2025-11-13T10:36:10Z

the main aim of this change is to give the identify methods access to the before_send hook, so users can redact data before events are sent

the changes to the identify methods (identify, identifyImmediately, and groupIdentify) are:

makes a call to prepareEventMessage which subsequently invokes the before_send hook, this also transforms the data (capture() works this way)
wraps the promise chain in addPendingPromise() to add the promises to the queue (capture() also works this way)

cc: @pauldambra

pauldambra · 2025-11-13T10:39:18Z

thanks @MohamedH1998, makes sense

in posthog-js (the browser SDK) we do let folk amend any event

we have seen a few instances where they redact themselves into a bad state, but that's i guess a consequence of this kind of power_user escape hatch 😊

MohamedH1998 · 2025-11-13T11:37:39Z

yeahh exactly, appreciate the comments @pauldambra

pauldambra · 2025-11-13T11:41:36Z

packages/node/src/__tests__/posthog-node.spec.ts

-      jest.runOnlyPendingTimers()
-      await waitForPromises()
+
+      await waitForFlushTimer()


i'd be interested to see what refactor an AI tool would make of this if we asked it to parameterise the tests

there's a lot of repetition (normally i wouldn't ask on an external PR, but since I'm assuming something like Claude would do the work for you 🙈 )

yep, let's try an AI pass to tighten up this test file and then i think we're good to go

(you're welcome to do that by hand obvs but 🤖 is a good calculator for words)

done, shout out claude code 🫡

any chance of a second glance at this @pauldambra

hpouillot

LGTM

MohamedH1998 added 2 commits November 5, 2025 23:03

fix: apply before send to identify and groupIdentify

b7fdc15

chore: changeset

fad88ce

MohamedH1998 commented Nov 5, 2025

View reviewed changes

greptile-apps bot reviewed Nov 5, 2025

View reviewed changes

MohamedH1998 commented Nov 5, 2025

View reviewed changes

fix: ensure groupIdentify promises are tracked during flush

2a7db01

MohamedH1998 changed the title ~~fix: apply before_send to identify and groupIdentify ends~~ fix: apply before_send to identify and groupIdentify events Nov 5, 2025

posthog-bot added the stale label Nov 13, 2025

pauldambra reviewed Nov 13, 2025

View reviewed changes

hpouillot approved these changes Nov 13, 2025

View reviewed changes

chore: parameterise tests

6274121

posthog-bot removed the stale label Nov 14, 2025

fix: apply before_send to identify and groupIdentify events #2550

Are you sure you want to change the base?

fix: apply before_send to identify and groupIdentify events #2550

Conversation

MohamedH1998 commented Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Changes

Release info Sub-libraries affected

Libraries affected

Checklist

If releasing new changes

Uh oh!

vercel bot commented Nov 5, 2025

Uh oh!

MohamedH1998 Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

MohamedH1998 Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

posthog-bot commented Nov 13, 2025

Uh oh!

pauldambra commented Nov 13, 2025

Uh oh!

MohamedH1998 commented Nov 13, 2025

Uh oh!

pauldambra commented Nov 13, 2025

Uh oh!

MohamedH1998 commented Nov 13, 2025

Uh oh!

pauldambra Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

pauldambra Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

MohamedH1998 Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

MohamedH1998 Nov 18, 2025

Choose a reason for hiding this comment

Uh oh!

hpouillot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MohamedH1998 commented Nov 5, 2025 •

edited

Loading