fix: push tracking and handling issues by brainbicycle · Pull Request #13068 · artsy/eigen

brainbicycle · 2025-12-04T19:56:07Z

This PR resolves DIAM-214

cc @artsy/diamond-devs

Description

Fixes a few issues with push and tracking related to push notifications:

moves the notification_tapped event outside if checks to be closer to the old behavior
fixes an issue where the bridge being invalidated can cause pushes not to be handled
fixes an issue where nav remounting causes pushes from logged out state to not navigate to screens

Scenario	Before	After
Logged Out Push	before-login-low.mov	after-login-low.mov
Invalidated Bridge Push	before-event-emitter-low.mov	after-eventemitter-low.mov

Follow-ups

Upstream pr for eventEmitter issue

PR Checklist

I have tested my changes on the following platforms:
- Android.
- iOS.
I hid my changes behind a feature flag, or they don't need one.
I have included screenshots or videos at least on Android, or I have not changed the UI.
I have added tests, or my changes don't require any.
I added an app state migration, or my changes do not require one.
I have documented any follow-up work that this PR will require, or it does not require any.
I have added a changelog entry below, or my changes do not require one.

To the reviewers 👀

I would like at least one of the reviewers to run this PR on the simulator or device.

Changelog updates

Cross-platform user-facing changes

fix push tracking and behavior bugs - brian

iOS user-facing changes

Android user-facing changes

Dev changes

Need help with something? Have a look at our docs, or get in touch with us.

Restores previous native behavior where notification tap tracking fired immediately when notifications arrived, not gated behind login/navigation state. This is important as the event is used as a session start event. Split the useEffect into two separate hooks: - First hook tracks notification taps immediately when pushNotification changes - Second hook handles navigation, still gated behind login/ready checks This prevents undertracking that was introduced during the notifee migration. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>

…ference

brainbicycle · 2025-12-04T21:43:40Z

        ...pushNotification.data,
      })
+    }
+  }, [pushNotification])


the tracking event from the native side was sent every time the app opened from a push, and this event is important it is one of the special events that ladders up to sessions, this could account for some of the discrepancies with looker session tracking

brainbicycle · 2025-12-04T21:44:41Z

+
+      // Use requestAnimationFrame to ensure navigation happens after AuthenticatedRoutes
+      // has mounted and initialized (important when navigating after login)
+      requestAnimationFrame(() => {


when users log in we mount the AuthenticatedRoutes nav stack which results in navigation to the home tab at same time we try to handle the push navigation, home wins and push fails

[not for this PR]: I know we like requestAnimationFrame, but what's the result of using InteractionManager.runAfterInteractions instead?

can take a look!

brainbicycle · 2025-12-04T21:45:30Z

+    // Reset listener count to allow proper re-initialization on bridge reload
+    // This is necessary for modules that persist across bridge reloads (singletons, dev reload, Expo Updates)
+    // Without this reset, startObserving will never be called again because _listenerCount > 1
+    _listenerCount = 0;


see the explanation in HACKS.md, not clear how often this happens in prod but if it does happen it could explain poor push performance because the native events to handle pushes will just silently fail with no tracking or navigation

ArtsyOpenSource · 2025-12-04T23:24:09Z

This PR contains the following changes:

Cross-platform user-facing changes (fix push tracking and behavior bugs - brian - brainbicycle)

Generated by 🚫 dangerJS against 2eda1b7

araujobarret · 2025-12-05T13:59:48Z

+
+      // Use requestAnimationFrame to ensure navigation happens after AuthenticatedRoutes
+      // has mounted and initialized (important when navigating after login)
+      requestAnimationFrame(() => {


[not for this PR]: I know we like requestAnimationFrame, but what's the result of using InteractionManager.runAfterInteractions instead?

MounirDhahri

Thanks for fixing this.
Sorry forgot to submit my review

* remove now unused analytics constant * fix: track push notification taps regardless of login state Restores previous native behavior where notification tap tracking fired immediately when notifications arrived, not gated behind login/navigation state. This is important as the event is used as a session start event. Split the useEffect into two separate hooks: - First hook tracks notification taps immediately when pushNotification changes - Second hook handles navigation, still gated behind login/ready checks This prevents undertracking that was introduced during the notifee migration. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * requestAnimationFrame when handling push to prevent initial nav interference * workaround for listenerCount being reset preventing observing starting * move event emitter fix to patch * update tests --------- Co-authored-by: Claude <noreply@anthropic.com>

Summary: If a subclass of RCTEventEmitter outlives the bridge and the bridge is invalidated, the invalidate method is called on RCTEventEmitter which calls stopObserving but does not change the listenerCount. Once the bridge is recreated and a listener added to the RCTEventEmitter if the listenerCount is > 1 startListening is never called: https://github.com/facebook/react-native/blob/a9a02689533f9cf30fc20cfc7be1ade4f4a624da/packages/react-native/React/Modules/RCTEventEmitter.m#L109 This just resets the listenerCount to prevent this. Probably not a good idea to have modules that outlive bridge in first place but this seems like unwanted behavior in any case :). ## Changelog: [iOS] [Fixed] - fix listenerCount not resetting for RCTEventEmitter Pull Request resolved: #54809 Test Plan: I tested this code via a patch in the artsy/eigen app repo: artsy/eigen#13068 The app was using a native module that posts events over the bridge to handle push notifications. Behavior before: 1. Start app listeners added to ARNotificationManager module, startListener called listenerCount = 2 2. Force a dev reload invalidating the bridge 3. Invalidate called on bridge and the native module causing stopObserving to be called but listenerCount remains 2 4. Bridge refreshes listeners added again but startObserving never called Behavior after: 1. Start app listeners added to ARNotificationManager module, startListener called listenerCount = 2 2. Force a dev reload invalidating the bridge 3. Invalidate called on bridge and the native module causing stopObserving to be called but listenerCount reset to 0 4. Bridge refreshes listeners added again listeners added again, startObserving called after first Reviewed By: cipolleschi Differential Revision: D88647586 Pulled By: javache fbshipit-source-id: 8a8b8b8cf4c7e683f02a08e40fc9d95c8ba4e4ac

brainbicycle and others added 4 commits December 3, 2025 13:02

remove now unused analytics constant

6a1a362

requestAnimationFrame when handling push to prevent initial nav inter…

5c2d068

…ference

workaround for listenerCount being reset preventing observing starting

dc5fc21

brainbicycle self-assigned this Dec 4, 2025

brainbicycle marked this pull request as draft December 4, 2025 19:56

move event emitter fix to patch

30b2728

artsy-peril Bot added the Jira Synced label Dec 4, 2025

brainbicycle requested review from MounirDhahri and gkartalis December 4, 2025 21:40

brainbicycle marked this pull request as ready for review December 4, 2025 21:41

brainbicycle commented Dec 4, 2025

View reviewed changes

update tests

2eda1b7

artsy deleted a comment from ArtsyOpenSource Dec 4, 2025

araujobarret approved these changes Dec 5, 2025

View reviewed changes

MounirDhahri approved these changes Dec 5, 2025

View reviewed changes

gkartalis approved these changes Dec 5, 2025

View reviewed changes

brainbicycle merged commit a235337 into main Dec 5, 2025
12 checks passed

brainbicycle deleted the brian/braze-tracking-issue branch December 5, 2025 14:12

brainbicycle mentioned this pull request Dec 5, 2025

fix: reset listenerCount when invalidating RCTEventEmitter react/react-native#54809

Closed

Navigation Menu

Repository navigation

fix: push tracking and handling issues - #13068

fix: push tracking and handling issues#13068
brainbicycle merged 6 commits into
mainfrom
brian/braze-tracking-issue

brainbicycle commented Dec 4, 2025 •

edited

Loading

Uh oh!

brainbicycle Dec 4, 2025

Uh oh!

brainbicycle Dec 4, 2025

Uh oh!

araujobarret Dec 5, 2025

Uh oh!

brainbicycle Dec 5, 2025

Uh oh!

brainbicycle Dec 4, 2025

Uh oh!

ArtsyOpenSource commented Dec 4, 2025

Uh oh!

araujobarret Dec 5, 2025

Uh oh!

MounirDhahri left a comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

brainbicycle commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Follow-ups

PR Checklist

To the reviewers 👀

Changelog updates

Cross-platform user-facing changes

iOS user-facing changes

Android user-facing changes

Dev changes

Uh oh!

brainbicycle Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

brainbicycle Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

araujobarret Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

brainbicycle Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

brainbicycle Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

ArtsyOpenSource commented Dec 4, 2025

This PR contains the following changes:

Uh oh!

araujobarret Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

MounirDhahri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

brainbicycle commented Dec 4, 2025 •

edited

Loading