Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Autocapture native app lifecycle events #398

Merged

Conversation

rafaeelaudibert
Copy link
Member

@rafaeelaudibert rafaeelaudibert commented Feb 19, 2025

Problem

Right now, we have two configurations for autotracking lifecycle events, and we suggest customers to only use one or another. The problem is that they're not equivalent.

The options.captureNativeAppLifecycleEvents option is capturing both app lifecycle events ("Application Opened", "Application Backgrounded", "Application Became Active") but also native events ("Application Installed" and "Application Updated").

autocapture.captureLifecycleEvents on the other hand does not capture the latter.

Changes

Let's make them act in the same way by using the option from inside autocapture - the one we suggest in our docs - and copying it over to the options config hash. They'll now behave in the same way, there isn't even a problem setting them both at the same time - we might need to update docs.

See https://posthoghelp.zendesk.com/agent/tickets/25578

Release info Sub-libraries affected

Bump level

  • Major
  • Minor
  • Patch

Libraries affected

  • All of them
  • posthog-web
  • posthog-node
  • posthog-ai
  • posthog-react-native

Changelog notes

fix: Autocapture native app lifecycle events

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

This PR unifies and fixes the behavior of lifecycle event tracking in the React Native SDK, ensuring consistent behavior between different configuration methods.

  • Modified useLifecycleTracker.ts to capture initial URL with Application Opened event
  • Consolidated lifecycle tracking configuration in PostHogProvider.tsx by using autocapture.captureLifecycleEvents as source of truth
  • Updated captureNativeAppLifecycleEvents option to match behavior with autocapture.captureLifecycleEvents
  • Added proper cleanup for AppState subscription in lifecycle tracker

3 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile

@rafaeelaudibert rafaeelaudibert force-pushed the fix-native-lifecycle-events-not-being-tracked-on-autocapture branch from 3eadc32 to 57c4910 Compare February 19, 2025 21:24
Copy link

github-actions bot commented Feb 19, 2025

Size Change: -392 B (-0.3%)

Total Size: 131 kB

Filename Size Change
posthog-react-native/lib/posthog-react-native/index.js 454 B -31 B (-6.39%)
posthog-react-native/lib/posthog-react-native/src/frameworks/wix-navigation.js 505 B -146 B (-22.43%) 🎉
posthog-react-native/lib/posthog-react-native/src/hooks/useLifecycleTracker.js 0 B -416 B (removed) 🏆
posthog-react-native/lib/posthog-react-native/src/posthog-rn.js 4.46 kB +32 B (+0.72%)
posthog-react-native/lib/posthog-react-native/src/PostHogProvider.js 1.74 kB +169 B (+10.74%) ⚠️
ℹ️ View Unchanged
Filename Size
posthog-node/lib/index.cjs.js 31 kB
posthog-node/lib/index.esm.js 31 kB
posthog-react-native/lib/posthog-core/src/eventemitter.js 1.08 kB
posthog-react-native/lib/posthog-core/src/index.js 10.3 kB
posthog-react-native/lib/posthog-core/src/lz-string.js 1.42 kB
posthog-react-native/lib/posthog-core/src/types.js 420 B
posthog-react-native/lib/posthog-core/src/utils.js 870 B
posthog-react-native/lib/posthog-core/src/vendor/uuidv7.js 2.04 kB
posthog-react-native/lib/posthog-react-native/src/autocapture.js 1.8 kB
posthog-react-native/lib/posthog-react-native/src/hooks/useFeatureFlag.js 437 B
posthog-react-native/lib/posthog-react-native/src/hooks/useFeatureFlags.js 362 B
posthog-react-native/lib/posthog-react-native/src/hooks/useNavigationTracker.js 628 B
posthog-react-native/lib/posthog-react-native/src/hooks/usePostHog.js 249 B
posthog-react-native/lib/posthog-react-native/src/legacy.js 810 B
posthog-react-native/lib/posthog-react-native/src/native-deps.js 1.31 kB
posthog-react-native/lib/posthog-react-native/src/optional/OptionalAsyncStorage.js 183 B
posthog-react-native/lib/posthog-react-native/src/optional/OptionalExpoApplication.js 215 B
posthog-react-native/lib/posthog-react-native/src/optional/OptionalExpoDevice.js 211 B
posthog-react-native/lib/posthog-react-native/src/optional/OptionalExpoFileSystem.js 224 B
posthog-react-native/lib/posthog-react-native/src/optional/OptionalExpoLocalization.js 216 B
posthog-react-native/lib/posthog-react-native/src/optional/OptionalReactNativeDeviceInfo.js 220 B
posthog-react-native/lib/posthog-react-native/src/optional/OptionalReactNativeLocalize.js 169 B
posthog-react-native/lib/posthog-react-native/src/optional/OptionalReactNativeNavigation.js 218 B
posthog-react-native/lib/posthog-react-native/src/optional/OptionalReactNativeNavigationWix.js 222 B
posthog-react-native/lib/posthog-react-native/src/optional/OptionalSessionReplay.js 231 B
posthog-react-native/lib/posthog-react-native/src/PostHogContext.js 210 B
posthog-react-native/lib/posthog-react-native/src/storage.js 1.09 kB
posthog-react-native/lib/posthog-react-native/src/types.js 90 B
posthog-react-native/lib/posthog-react-native/src/version.js 123 B
posthog-web/lib/index.cjs.js 18.3 kB
posthog-web/lib/index.esm.js 18.2 kB

compressed-size-action

Right now, we have two configurations for autotracking lifecycle events, and we suggest customers to only use one or another. The problem is that they're not equivalent.

The `options.captureNativeAppLifecycleEvents` option is capturing both app lifecycle events ("Application Opened", "Application Backgrounded", "Application Became Active") but also native events ("Application Installed" and "Application Updated").

`autocapture.captureLifecycleEvents` on the other hand does not capture the latter.

Let's make them act in the same way by using the option from inside `autocapture` - the one we suggest in our docs - and copying it over to the `options` config hash. They'll now behave in the same way, there isn't even a problem setting them both at the same time - we might need to update docs.

See https://posthoghelp.zendesk.com/agent/tickets/25578
@rafaeelaudibert rafaeelaudibert force-pushed the fix-native-lifecycle-events-not-being-tracked-on-autocapture branch from 57c4910 to 735e93d Compare February 19, 2025 21:29
@marandaneto
Copy link
Member

One issue I see is that after this PR, you cannot opt out of the app install and app update events.
That is the difference between captureNativeAppLifecycleEvents and captureLifecycleEvents I think, ps: they were added past me, so I agree that it is confusing but I don't have context on why it was done this way

@rafaeelaudibert
Copy link
Member Author

One issue I see is that after this PR, you cannot opt out of the app install and app update events. That is the difference between captureNativeAppLifecycleEvents and captureLifecycleEvents I think, ps: they were added past me, so I agree that it is confusing but I don't have context on why it was done this way

Yeah, 100% true. In my mind, the best solution is to break the existing API compatibility with a major and then split this setting up in two at the config level rather than having some of it running at the provider level like we have now. One problem we have now is that you can't opt in to native lifecycle events without opting in to lifecycle events, so it's already kinds restrictive anyway.

So, we have two options: get a new major ready now, and we might wanna include more things in it, or just keep this as is and I can tell the user to move from the autocapture setting to the options setting - and then update the docs to mention how quirky this is

@marandaneto
Copy link
Member

One issue I see is that after this PR, you cannot opt out of the app install and app update events. That is the difference between captureNativeAppLifecycleEvents and captureLifecycleEvents I think, ps: they were added past me, so I agree that it is confusing but I don't have context on why it was done this way

Yeah, 100% true. In my mind, the best solution is to break the existing API compatibility with a major and then split this setting up in two at the config level rather than having some of it running at the provider level like we have now. One problem we have now is that you can't opt in to native lifecycle events without opting in to lifecycle events, so it's already kinds restrictive anyway.

So, we have two options: get a new major ready now, and we might wanna include more things in it, or just keep this as is and I can tell the user to move from the autocapture setting to the options setting - and then update the docs to mention how quirky this is

tbh I think this was just an oversight, the wording 'native' lifecycles does not even reflect anything 'native' just because it captures 2 more possible events.
Added to #212
let me think about it for a sec.

@marandaneto
Copy link
Member

@rafaeelaudibert #401
an idea is to just merge them, document better as I did in this PR, and on the next major, really have a single option.

I think the reason why 'native' and not 'native' is because 'native' might have the storage set up and can compare the versions (if its a new install or an update) and not native there's no storage at all (which is not true since you can set up your own storage function/its just not automatic), so it's confusing but I guess we'd be fine with a minor after merging my PR.
it'd be a minor because we'd add a feature on top of the current one.

if one does not want the new approach with app install and app update events, they can simply set up their own AppState.addEventListener which is just a couple of lines instead having a more granular config for each app lifecycle event.

WDYT?

@rafaeelaudibert
Copy link
Member Author

@rafaeelaudibert #401 an idea is to just merge them, document better as I did in this PR, and on the next major, really have a single option.

I think the reason why 'native' and not 'native' is because 'native' might have the storage set up and can compare the versions (if its a new install or an update) and not native there's no storage at all (which is not true since you can set up your own storage function/its just not automatic), so it's confusing but I guess we'd be fine with a minor after merging my PR. it'd be a minor because we'd add a feature on top of the current one.

if one does not want the new approach with app install and app update events, they can simply set up their own AppState.addEventListener which is just a couple of lines instead having a more granular config for each app lifecycle event.

WDYT?

Ok, I like that, especially the fact that they can just roll their own if they want a more customizable experience. Then the next major can add specific configs for each individual kind of event - or add something akin to posthog-js's before_send where they can filter these events out.

Just to confirm: Are you proposing we just merge your PR on this one, and then both go live as a minor? I'm happy to do that


On another note, I don't know how releases work on this repo, and I couldn't find any info in the README - it's too barebones IMO.

@marandaneto
Copy link
Member

@rafaeelaudibert

Just to confirm: Are you proposing we just merge your PR on this one, and then both go live as a minor? I'm happy to do that

yes, the release is simple, all info is here

@rafaeelaudibert
Copy link
Member Author

@marandaneto Just merged your branch here, will get the release ready - by EOD, I'm working on some other stuff already

@marandaneto marandaneto enabled auto-merge (squash) February 21, 2025 10:34
@marandaneto marandaneto merged commit 1c45a81 into main Feb 21, 2025
4 checks passed
@marandaneto marandaneto deleted the fix-native-lifecycle-events-not-being-tracked-on-autocapture branch February 21, 2025 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants