-
Notifications
You must be signed in to change notification settings - Fork 40
[MOB-12267] add pauseEmbeddedImpression method to manage embedded message impressions #785
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
base: loren/embedded/MOB-12264-android-sync-and-get-messages
Are you sure you want to change the base?
Conversation
…embedded/MOB-12267-android-add-start-impression-and-pause-impressio
1 new issue
This is from Qlty Cloud, the successor to Code Climate Quality. Learn more. |
android/src/main/java/com/iterable/reactnative/RNIterableAPIModuleImpl.java
Show resolved
Hide resolved
|
Diff Coverage: The code coverage on the diff in this pull request is 100.0%. Total Coverage: This PR will increase coverage by 0.42%. 🛟 Help
This is from Qlty Cloud, the successor to Code Climate Quality. Learn more. |
android/src/main/java/com/iterable/reactnative/RNIterableAPIModuleImpl.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request adds enhanced embedded messaging functionality to the React Native SDK, specifically adding the pauseEmbeddedImpression method and exposing embedded message types to enable developers to manage embedded message impressions more effectively. The changes introduce new TypeScript type definitions for embedded messages and their components, implement impression tracking methods across the TypeScript and Android layers, and update the example app to demonstrate the new functionality.
Key Changes
- Added
pauseEmbeddedImpressionandstartEmbeddedImpressionmethods to track when embedded messages are visible or hidden - Exposed
IterableEmbeddedMessageand related types to the public API for type-safe embedded message handling - Added
syncMessagesandgetMessagesmethods to theIterableEmbeddedManagerfor retrieving embedded messages
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/index.tsx | Exports IterableEmbeddedMessage type for external use |
| src/embedded/types/*.ts | Defines TypeScript interfaces for embedded messages, metadata, elements, buttons, and text |
| src/embedded/index.ts | Exports embedded types alongside classes |
| src/embedded/classes/IterableEmbeddedManager.ts | Adds syncMessages, getMessages, startImpression, and pauseImpression methods with documentation |
| src/core/classes/IterableApi.ts | Implements static methods that bridge to native layer for embedded messaging operations |
| src/api/NativeRNIterableAPI.ts | Defines TypeScript bridge interface and native module method signatures |
| android/src/oldarch/java/com/RNIterableAPIModule.java | Adds ReactMethod bindings for old architecture Android implementation |
| android/src/newarch/java/com/RNIterableAPIModule.java | Adds ReactMethod bindings for new architecture Android implementation |
| android/src/main/java/com/iterable/reactnative/Serialization.java | Adds serialization support for embedded messages |
| android/src/main/java/com/iterable/reactnative/RNIterableAPIModuleImpl.java | Implements core Android logic for embedded message operations including sync, retrieval, and impression tracking |
| example/src/components/Embedded/Embedded.tsx | Demonstrates new embedded messaging features with UI controls and message display |
| example/src/components/Embedded/Embedded.styles.ts | Adds styling for embedded message UI components |
| example/src/constants/styles/typography.ts | Adds link text style for interactive elements |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } else { | ||
| // Convert ReadableArray to individual placement IDs and get messages for each | ||
| for (int i = 0; i < placementIds.size(); i++) { | ||
| long placementId = placementIds.getInt(i); | ||
| List<IterableEmbeddedMessage> messages = IterableApi.getInstance().getEmbeddedManager().getMessages(placementId); | ||
| if (messages != null) { | ||
| allMessages.addAll(messages); | ||
| } |
Copilot
AI
Nov 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation for handling null/empty placementIds is unclear. Using 0 as a default placement ID may not be the correct behavior. Consider either:
- Getting messages from all registered placements by iterating through them
- Throwing an error if no placement IDs are provided
- Documenting that
0is a valid default placeholder in the comment
The current comment mentions "This might need to be adjusted based on the actual SDK behavior" which suggests this is uncertain logic that should be resolved.
| // TODO: check if this should be changed to a number, as per the type | ||
| Number(message.metadata.placementId) |
Copilot
AI
Nov 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TODO comment and Number() conversion are unnecessary. The placementId field is already typed as number in IterableEmbeddedMessageMetadata, so it can be passed directly to startImpression() which expects a number.
| // TODO: check if this should be changed to a number, as per the type | |
| Number(message.metadata.placementId) | |
| message.metadata.placementId |
| action: { type: string; data?: string } | null; | ||
| }[] | ||
| | null; | ||
| body?: string | null; | ||
| mediaUrl?: string | null; | ||
| mediaUrlCaption?: string | null; | ||
| defaultAction?: { type: string; data?: string } | null; |
Copilot
AI
Nov 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The action type in the EmbeddedMessage interface is missing the userInput field that exists in the IterableAction class. The action should be { type: string; data?: string; userInput?: string } to match the full IterableAction type definition.
| action: { type: string; data?: string } | null; | |
| }[] | |
| | null; | |
| body?: string | null; | |
| mediaUrl?: string | null; | |
| mediaUrlCaption?: string | null; | |
| defaultAction?: { type: string; data?: string } | null; | |
| action: { type: string; data?: string; userInput?: string } | null; | |
| }[] | |
| | null; | |
| body?: string | null; | |
| mediaUrl?: string | null; | |
| mediaUrlCaption?: string | null; | |
| defaultAction?: { type: string; data?: string; userInput?: string } | null; |
android/src/main/java/com/iterable/reactnative/RNIterableAPIModuleImpl.java
Show resolved
Hide resolved
android/src/main/java/com/iterable/reactnative/RNIterableAPIModuleImpl.java
Show resolved
Hide resolved
…RNIterableAPIModuleImpl
android/src/main/java/com/iterable/reactnative/RNIterableAPIModuleImpl.java
Show resolved
Hide resolved
… into loren/embedded/MOB-12267-android-add-start-impression-and-pause-impressio
… into loren/embedded/MOB-12267-android-add-start-impression-and-pause-impressio
🔹 JIRA Ticket(s) if any
✏️ Description
Adds the ability to start and pause an impression
Testing
yarn android