Skip to content

fix: Forward impression events and products instead of null events#355

Open
mmustafa-tse wants to merge 1 commit into
mParticle:mainfrom
mmustafa-tse:fix/impressionEvent
Open

fix: Forward impression events and products instead of null events#355
mmustafa-tse wants to merge 1 commit into
mParticle:mainfrom
mmustafa-tse:fix/impressionEvent

Conversation

@mmustafa-tse

Copy link
Copy Markdown
Contributor

Summary

A customer reported that Impression events sent from React Native SDK are being received as null in the UI, after further investigation it seems the iOS react native bridge was missing the logic to handle impressions in logCommerceEvents.

Testing Plan

Tested E2E by changing the code locally:
Screenshot 2026-07-06 at 2 49 15 PM

Screenshot 2026-07-06 at 2 06 18 PM Screenshot 2026-07-06 at 2 08 48 PM

Master Issue

Closes https://rokt.atlassian.net/browse/SDKE-1210

@mmustafa-tse
mmustafa-tse requested a review from a team as a code owner July 6, 2026 21:53
@cursor

cursor Bot commented Jul 6, 2026

Copy link
Copy Markdown

PR Summary

Low Risk
Localized bridge mapping for analytics commerce impressions; no auth, persistence, or API contract changes beyond fixing dropped impression data on the New Arch iOS path.

Overview
Fixes impression commerce events from React Native on iOS showing as null in the dashboard by mapping commerceEvent.impressions() in the New Architecture logCommerceEvent bridge.

When impressions are present, each list’s impressionListName and nested products are converted with the same field mapping as regular products (createMPProductFromDict), then attached via addImpression:product listName: before the event is logged. Impression lists without a list name are skipped.

This aligns the Turbo Module path with the existing old architecture RCTConvert (MPCommerceEvent) impression handling, which was already implemented but not wired in the struct-based New Arch method.

Reviewed by Cursor Bugbot for commit 557df1f. Bugbot is set up for automated code reviews on this repo. Configure here.

NSMutableDictionary *productDict = [[NSMutableDictionary alloc] init];
if (productStruct.name()) productDict[@"name"] = productStruct.name();
if (productStruct.sku()) productDict[@"sku"] = productStruct.sku();
productDict[@"price"] = @(productStruct.price());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Any reason price doesn't have a conditional whereas all the others do?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@rmi22186 - I pretty much copied the same logic on for commerceEvent.products().has_value() in line 513, I can add the if statement for both to be on the safer side

@rmi22186 rmi22186 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Note this is new architecture that doesn't support it, not the old arch, the old arch supports, so the PR description is slightly off.

@rmi22186

Copy link
Copy Markdown
Member

Also the iOS test app is not building. Take a look at that.

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