feat(push): capture each PostHog push open once across automatic and manual paths - #783
Merged
Merged
Conversation
Contributor
posthog-android Compliance ReportDate: 2026-09-14 20:03:46 UTC ✅ All Tests Passed!46/46 tests passed Capture Tests✅ 29/29 tests passed View Details
Feature_Flags Tests✅ 17/17 tests passed View Details
|
turnipdabeets
marked this pull request as ready for review
September 11, 2026 17:52
Contributor
Prompt To Fix All With AI### Issue 1
posthog/src/main/java/com/posthog/PostHog.kt:2127-2141
**Dropped events consume dedupe keys**
The push key is recorded before `capture()` runs its `beforeSend` hooks. If a hook drops the first automatic report—for example, because it lacks the title present on the manual report—the later manual report is still skipped as a duplicate, so no push-open event is delivered. Record the key only after the event passes the capture filters.
### Issue 2
posthog/src/main/java/com/posthog/PostHog.kt:2162-2163
**Live keys are evicted**
The 20-entry limit breaks the documented five-minute guarantee. After 21 distinct opens within the window, the first key is evicted and another report for it is captured again, producing a duplicate push-open event. Expire entries by timestamp or otherwise retain every key for the promised window.
### Issue 3
posthog/src/test/java/com/posthog/PostHogTest.kt:4738-4748
**Test resources remain open**
`getPushOpenSut` creates a `MockWebServer` but discards its handle, and the new tests call neither `sut.clear()` nor `http.shutdown()`. This violates the repository directive requiring both cleanup operations and leaks test resources. Expose the server through the fixture and perform both cleanup calls after each test; this requirement must be satisfied before merging.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix(push): time the push-open dedupe win..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
turnipdabeets
added a commit
to PostHog/posthog-js
that referenced
this pull request
Sep 11, 2026
posthog-android 3.64.0 (PostHog/posthog-android#783) skips a repeat of a PostHog-sent notification open (same posthog invocation_id + action_id) captured within 5 minutes, whichever path reported it first. Every path this plugin uses ends there: onNewIntent, the cold-start capture and the manual capturePushNotificationOpened method. A manual call from an old onNotificationOpenedApp handler is therefore counted once without any plugin-level dedupe. Also adds the migration sentence to the published 2.6.0 changelog entry. Claude-Session: https://claude.ai/code/session_01UJgnRvz58rzgFVCiUfjxkL
This was referenced Sep 11, 2026
Merged
turnipdabeets
added a commit
to PostHog/posthog-js
that referenced
this pull request
Sep 12, 2026
posthog-android 3.64.0 (PostHog/posthog-android#783) skips a repeat of a PostHog-sent notification open (same posthog invocation_id + action_id) captured within 5 minutes, whichever path reported it first. Every path this plugin uses ends there: onNewIntent, the cold-start capture and the manual capturePushNotificationOpened method. A manual call from an old onNotificationOpenedApp handler is therefore counted once without any plugin-level dedupe. Also adds the migration sentence to the published 2.6.0 changelog entry. Claude-Session: https://claude.ai/code/session_01UJgnRvz58rzgFVCiUfjxkL
…manual paths capturePushNotificationOpened now skips a repeat of a PostHog-sent notification (same posthog.invocation_id and action_id) captured in the last 5 minutes. Every automatic path and the manual API end in this method, so an app that still calls the manual API from Firebase's onNotificationOpenedApp / onMessageOpenedApp next to the plugins' automatic capture counts one open instead of two, whichever reports first. Payloads without a posthog.invocation_id are captured as before. Claude-Session: https://claude.ai/code/session_01UJgnRvz58rzgFVCiUfjxkL
nanoTime is uptime-based on Android and stops in deep sleep, so a looped workflow's next open on an idle phone could fall inside the 5-minute window and be dropped. The window now uses the date provider's currentTimeMillis, and a negative gap (wall clock moved back) captures the open instead of skipping it. Claude-Session: https://claude.ai/code/session_01UJgnRvz58rzgFVCiUfjxkL
The 20-entry eviction and the lower bound of the dedupe window had no test entering them: evicting the newest key instead of the oldest, or dropping the 0 lower bound, left the suite green.
A workflow that reruns or loops back to its push step sends a second notification with the same invocation_id/action_id pair. The dedupe key could not tell that from a second report of one tap, so the second open was dropped inside the 5-minute window. Remember the delivery id (google.message_id, present on the intent extras) next to each entry and use it only to recognise a new delivery: a report whose id disagrees with the stored one is captured, everything else stays deduped.
turnipdabeets
force-pushed
the
feat/push-open-dedupe
branch
from
September 12, 2026 18:20
893f1e1 to
b57cef8
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
isNewDelivery had one caller and a doc comment restating what the public KDoc and the dedupe-window constant already said, so the resend rule was written out three times. Inline it as a named `resent` condition and keep the explanation once, where the two ids are compared. Also drop the `posthogPayload == null` disjunct, which could never decide the guard that `invocationId.isNullOrEmpty()` already settles, and correct the comment on recentPushOpens, which still described a map of keys to timestamps from before the delivery id moved in beside them. Trim the changeset to one line of observable behaviour per the changelog style; the rationale belongs in the PR body. No behaviour change.
dustinbyrne
reviewed
Sep 14, 2026
dustinbyrne
left a comment
dustinbyrne
left a comment
Contributor
There was a problem hiding this comment.
Clock ordering
Consider sampling the clock inside the cache lock. An earlier caller can be overtaken by a later caller, then interpret that scheduling inversion as a backwards clock, clear the cache, and admit the same notification again. A concurrency test with reversed sampling/admission order would cover this.
Bounded guarantee
Consider qualifying the once-only guarantee with the five-minute window and twenty-distinct-key retention limit.
Two concurrent reports of one tap could be admitted in the opposite order to their clock samples; the later admission then read the inversion as a backwards wall clock, cleared the entry, and captured the open twice. Also document the 20-key retention bound on the public API.
Parks the first report between sampling the clock and admitting it, so the second samples later but is admitted first. Sampling outside the lock reads that inversion as a backwards clock and captures the open twice; the test fails on the pre-fix code and passes once the sample moves under the lock.
Contributor
Author
|
Thanks @dustinbyrne — both addressed. Clock ordering — real bug, fixed in bd305b8. The sample now happens inside A concurrency test — added in 5699d71. A fake Bounded guarantee — qualified in bd305b8. The posthog-ios#828 gets the same three changes, so the platforms stay identical. |
dustinbyrne
approved these changes
Sep 15, 2026
dustinbyrne
left a comment
dustinbyrne
left a comment
Contributor
There was a problem hiding this comment.
Source re-review at 5699d71385cb3f1cf9c50977a23b3da881f1224b: clock sampling is now inside the admission lock, and the retention bound is documented. No remaining findings in this change.
turnipdabeets
added a commit
to PostHog/posthog-ios
that referenced
this pull request
Sep 15, 2026
…manual paths (#828) * feat(push): capture each PostHog push open once across automatic and manual paths Every push-open path on iOS ends in the field-based `capturePushNotificationOpened`, so the skip lives there: a repeat of a PostHog-sent notification (same `posthog.invocation_id` and `action_id`) captured in the last 5 minutes is skipped and logged, whichever path reported it first. Payloads without a usable `invocation_id` behave exactly as before. This matches PostHog/posthog-android#783, so apps that call the manual API next to the SDK's automatic capture stop double counting on both platforms. Claude-Session: https://claude.ai/code/session_01UJgnRvz58rzgFVCiUfjxkL * test(push): cover the open-dedupe cap eviction and a backward wall clock The cap eviction and the negative-elapsed half of the dedupe guard had no test entering them: swapping removeFirst() for removeLast(), or deleting the elapsed >= 0 check, left the suite green. Also states that recentPushOpens is in insertion order and that eviction drops the front. * fix(push): count an open of a resent notification separately A workflow that reruns or loops back to its push step sends a second notification with the same invocation_id/action_id pair. The dedupe key could not tell that from a second report of one tap, so the second open was dropped inside the 5-minute window. Remember the delivery id (UNNotificationResponse's request identifier) next to each entry and use it only to recognise a new delivery: a report whose id disagrees with the stored one is captured, everything else stays deduped. The field-based overload carries no identifier, so a manual repeat of one tap still matches. Also point the config's local-notification guidance at the field-based overload: the response: overload reads content only from PostHog-sent notifications, so it captured a local one with no content at all. * refactor(push): fold the delivery-id check into recordPushOpen isNewDelivery had one caller and a doc comment restating what the public doc comments and the dedupe-window constant already said, so the resend rule was written out three times. Inline it as a named `resent` condition, bind the matched entry once instead of subscripting it twice, and keep the explanation where the two ids are compared. Trim the changeset to one line of observable behaviour per the changelog style; the rationale belongs in the PR body. No behaviour change. Keeps the implementation shape in step with posthog-android#783. * fix(push): sample the dedupe clock under the lock Two concurrent reports of one tap could be admitted in the opposite order to their clock samples; the later admission then read the inversion as a backwards wall clock, cleared the entry, and captured the open twice. The test parks the first report between sampling and admission so the second samples later but is admitted first: it captures twice before the fix and once after. Also document the 20-key retention bound on the public API.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
💡 Motivation and Context
The React Native and Flutter plugins now capture Android notification taps automatically (
@posthog/react-native-plugin2.6.0 via PostHog/posthog-js#4858,posthog_flutter5.40.0). The posthog.com docs used to tell those apps to also call the manual API from Firebase'sonNotificationOpenedApp/onMessageOpenedApp. Apps that still have that call now send two$push_notification_openedevents per tap. The server'spush-open-tracking.tsturns every one of those events into apush_openedapp metric with no dedupe, so Workflows open rates are inflated too. PostHog/posthog.com#20102 fixes the docs going forward. This PR covers the apps that already shipped the old snippet.PostHog/posthog-js#4919 and PostHog/posthog-flutter#578 fixed this inside each plugin. Both work, but each one copies the SDK's skip rules, and the two implementations already differ (consume-once vs. not). This PR moves the logic into posthog-android, where every path already ends up. Those plugin PRs will be repurposed afterwards.
Changes
One place. Every open path ends in
PostHog.capturePushNotificationOpened(title, body, payload, action)inposthogcore:onActivityCreated);PostHogAndroid.capturePushNotificationOpened(intent), which the Flutter plugin and the RN plugin'sonNewIntentcall, and which native apps call fromonNewIntent;The check lives there, so it doesn't depend on which path reports first. A manual call that arrives before the automatic one is handled the same way.
Match key:
invocation_id+action_idfrom the payload'sposthogentry, read as a JSON string or a map, the same way the event properties are read. The server stamps that entry on every push it sends (pushCorrelationDatainpush-notification.service.ts). Every step of one workflow run shares the run'sinvocation_id, so the key includesaction_id. A missingaction_idis allowed. With no usableinvocation_idthere's no key, and the call behaves exactly as today. That covers pushes from other senders and any malformed entry.Skip and record. A repeat of a key captured in the last 5 minutes is skipped and logged through
config.logger. Otherwise the key is recorded and the event is captured. The opted-out and disabled checks run first, so a call they drop records nothing. The existinggoogle.message_iddedupe layer above this one is unchanged.A resend is a different notification. Each entry also remembers the delivery id of the report that captured it:
payload["google.message_id"], which FCM leaves on the tapped intent. A report whose delivery id disagrees with the stored one is a second notification, not a second report of one tap, so it is captured and the entry is updated. The delivery id is never part of the key, because the manual API's caller has no delivery id to match with (RemoteMessage.getData()strips everygoogle.key), so keying on it would break the manual-repeat dedupe this PR exists for. A report with no delivery id, or one against an entry that stored none, stays deduped.Memory. Keys are kept in memory, per
PostHoginstance, in a synchronized insertion-ordered map capped at 20 entries.graph_validation.pytolerates them, and the executor tracks loop revisits throughactionStepCount), and a rerun reuses the run'sinvocation_id(rerun-paginator.service.ts). With no window, a "remind every day until they convert" loop would lose every open after the first for as long as the process stayed alive. The duplicate itself arrives much sooner: 14–33 ms after the automatic capture on RN (measured in fix(react-native): take the core push-open dedupe from posthog-android 3.65.0 and posthog-ios 3.75.0 posthog-js#4919), and 0.37 s warm / 0.66 s cold on Flutter debug builds (measured here). Five minutes leaves room for apps that callgetInitialMessage()late, for example after a splash screen.Scope.
posthog-serverhas its ownPostHogclass built onPostHogStateless, with no push-open API, so server behavior doesn't change.Public API. No new API;
apiCheckis clean. ThecapturePushNotificationOpenedKDoc inPostHogInterfacenow describes the skip. ThePostHogAndroidConfig.capturePushNotificationOpenedKDoc said the manual API is never deduped; it now says it dedupes only notifications sent by PostHog.Behavior change (minor)
The manual
capturePushNotificationOpenedchanges behavior for native apps too. A call is now skipped if its payload carries the sameposthog.invocation_idandaction_idas an open captured in the last 5 minutes, whether that open was captured automatically or by an earlier manual call. This affects:onMessageOpenedAppandgetInitialMessage(). These now count once, which is the intent of this PR.google.message_id, which is every FCM tray tap. Only a report that carries no delivery id, such as a foregroundmessage.datarelay, is dropped in that case.Nothing changes for payloads without a
posthog.invocation_id. The changeset bumpsposthogandposthog-androidasminor.posthog-androidis listed so it republishes with the new core (it re-exports it viaapi(project(":posthog"))). The next release should be posthog 6.36.0 / posthog-android 3.64.0, unless another changeset lands first. The plugins can then raise their floor to3.64.0.Cross-SDK
sdk-specscovers this in the open proposalopenspec/changes/add-push-notification-opens. PostHog/posthog-ios#828 lands the identical rule, including the resend case, where the delivery id isUNNotificationResponse.notification.request.identifier. The two SDKs also agree on the no-delivery-id case: a report without one never counts as a new delivery.The proposal's "Exactly one open per tap" requirement does not yet mention resends inside the window; it needs amending to match what both SDKs now do.
Rebase
Rebased onto
origin/main(was 5 commits behind). The branch predatedPostHogSessionReplayConfig.captureTouches(#780), whichposthog-flutter'smaincalls, so an artifact published from this branch alone could not compile the Flutter example. No CI job catches that.💚 How did you test it?
Unit tests.
./gradlew spotlessCheck :posthog:apiCheck :posthog-android:apiCheck :posthog:test :posthog-android:testReleaseUnitTestpassed: core 974, Android 534 (3 skipped), 0 failures. There are 11 new core tests:action_id, and another run;posthogentry;With the whole check removed, 5 of the 11 fail. Reverting only the delivery-id comparison (
isNewDeliveryforced tofalse) fails exactly 1: the resend test. Every other test, including all the manual-repeat ones, still passes both ways, which is the regression risk this change had to clear.Devices. A fresh Pixel 6 AVD (Android 17, API 37) on its own port. Every count below is the number of
$push_notification_openedevents in the SDK's/batchbodies, sent to a local mock server. App data was cleared before every row. Both reports carry the sameposthogentry,{"workflow_id":"wf-1","invocation_id":"inv-…","action_id":"step-1"}. The plugin rows use each plugin'sorigin/main, unchanged. "Before" is the posthog-android version thatorigin/mainresolves: 3.63.1 for Flutter's range, 3.62.0 for RN's pin. "After" is this branch, published to mavenLocal as3.63.99-dedupeand forced in with a temporary Gradle override.Native (posthog-android sample). Warm taps were delivered with
am startand tray-tap extras (google.message_id,posthog) into thesingleTopNormalActivity, which forwardsonNewIntenttoPostHogAndroid.capturePushNotificationOpened(intent). A temporary, uncommitted hook inonNewIntentthen callsPostHog.capturePushNotificationOpened(title = "manual", payload = mapOf("posthog" to …)).mainposthogentryposthogentryReal FCM tray taps (posthog-android sample +
firebase-messaging25.1.2). A freshAPI34_testAVD on its own port, rooted soam broadcast com.google.android.c2dm.intent.RECEIVEreaches Firebase's receiver. The Firebase SDK builds and posts the tray notification itself; taps are real taps in the shade. This covers the resend case end to end.invocation_id/action_id, both tappedm-r1,m-r2m-s1Flutter (
posthog_flutterexample,origin/main5.40.1). The harness is the one from PostHog/posthog-flutter#578:firebase_core+firebase_messaging16.6.0 with placeholder Firebase options, and the old docs snippet pasted in verbatim. FCM messages were injected with a rootam broadcast com.google.android.c2dm.intent.RECEIVE. Taps were real taps in the notification shade. For cold rows the process was killed witham killafter the notification was posted.getInitialMessage()handler, cold tapposthogentryIn every deduped row, the event that survives is the automatic one. The third row shows why a match doesn't consume the key: one automatic capture faces two manual calls.
React Native (
examples/example-rn-native-plugin,origin/main, plugin 2.7.0). This uses real@react-native-firebase/app+messaging23.8.8 (26.x needs RN ≥ 0.80; the example is on 0.79.6), with placeholder Firebase options. The old snippet ismessaging().onNotificationOpenedApp(m => capturePushNotificationOpened({ title, body, payload: m.data })). Injection and taps are the same as for Flutter.posthogentryam kill)The last row is a separate, existing RN plugin gap, and this PR doesn't change it. With the task kept, Android recreates
MainActivityfrom its original intent and delivers the tray intent throughonNewIntent. RN drops that intent (ReactHost.raiseSoftException(onNewIntent(...))in logcat) because React isn't ready yet, so neither the plugin nor RNFirebase sees the tap. It needs its own issue.Not verified:
posthogJSON shape the server sends.am startintents rather than shade taps.📝 Checklist
This is a behavior change for the manual API (see above), released as a
minorwith a changeset entry.If releasing new changes
pnpm changesetto generate a changeset fileRelated: PostHog/posthog-js#4919 and PostHog/posthog-flutter#578 (the per-plugin fixes this replaces), #753 (
PostHogAndroid.capturePushNotificationOpened(intent)), PostHog/posthog-js#4858 (RN automatic warm-tap capture), PostHog/posthog.com#20102 (docs).🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Built with Claude Code, driven by @turnipdabeets, to replace the two per-plugin fixes with one check in the SDK core.
PostHogAndroid.capturePushNotificationOpened(intent), the RN plugin's direct cold-start call, and the manual API from native code and both plugins) before putting the check inPostHog.capturePushNotificationOpened. That removes the plugins' need to mirror the SDK's gates or know which path captured first.invocation_id/action_id, so an unbounded set would drop real opens.Related PRs
One push-open capture effort across the mobile SDKs: count every notification tap exactly once, and stop losing taps the SDK starts too late to see.
firebase_messagingMerge order: posthog-android#783 and posthog-ios#828 first, then their releases. #4921, #4929 and #579 are independent and can go any time. #4919 and #578 go green once posthog-android 3.65.0 is published. Docs: #20114 can go now; #20102 last, after the releases.
Earlier work this builds on: #753, PostHog/posthog-ios#792, PostHog/posthog-js#4858, PostHog/posthog-flutter#556, PostHog/posthog-flutter#557, PostHog/posthog.com#19905.