-
Notifications
You must be signed in to change notification settings - Fork 40
[MOB-12264] Add sync messages and get messages to android #746
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-12265-start-end-session
Are you sure you want to change the base?
[MOB-12264] Add sync messages and get messages to android #746
Conversation
|
Diff Coverage: The code coverage on the diff in this pull request is 100.0%. Total Coverage: This PR will increase coverage by 0.43%. File Coverage Changes
🛟 Help
This is from Qlty Cloud, the successor to Code Climate Quality. Learn more. |
2 new issues
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
android/src/main/java/com/iterable/reactnative/RNIterableAPIModuleImpl.java
Show resolved
Hide resolved
…embedded/MOB-12264-android-sync-and-get-messages
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 PR adds support for syncing and retrieving embedded messages in the Android SDK, bringing feature parity between iOS and Android platforms for embedded messaging functionality.
Key Changes:
- Adds
syncMessages()method to manually refresh the local cache of embedded messages - Adds
getMessages(placementIds)method to retrieve embedded messages for specific placements - Exports TypeScript type definitions for embedded message structures
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/index.tsx | Exports IterableEmbeddedMessage type for public API |
| src/embedded/types/*.ts | New TypeScript interfaces defining the structure of embedded messages, metadata, elements, buttons, and text |
| src/embedded/index.ts | Exports embedded message types |
| src/embedded/classes/IterableEmbeddedManager.ts | Adds syncMessages() and getMessages() methods to the manager class |
| src/core/classes/IterableApi.ts | Adds wrapper methods for native embedded message operations |
| src/api/NativeRNIterableAPI.ts | Defines native bridge interface including EmbeddedMessage type and method signatures |
| android/src/oldarch/java/com/RNIterableAPIModule.java | Adds React method bindings for old architecture |
| android/src/newarch/java/com/RNIterableAPIModule.java | Adds React method bindings for new architecture |
| android/src/main/java/com/iterable/reactnative/Serialization.java | Adds serialization logic for embedded messages using native SDK's toJSONObject() |
| android/src/main/java/com/iterable/reactnative/RNIterableAPIModuleImpl.java | Implements sync and get messages logic, iterating through placement IDs |
| example/src/components/Embedded/Embedded.tsx | Demo implementation showing how to sync and retrieve embedded messages in the example app |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Syncs embedded local cache with the server. | ||
| * | ||
| * When your app first launches, and each time it comes to the foreground, | ||
| * Iterable's iOS SDK automatically refresh a local, on-device cache of |
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 documentation incorrectly refers to "iOS SDK" in a comment about syncing messages. Since this PR is adding Android support, the comment should mention both iOS and Android SDKs, or be platform-agnostic.
Suggested fix:
/**
* Syncs embedded local cache with the server.
*
* When your app first launches, and each time it comes to the foreground,
* Iterable's SDK automatically refreshes a local, on-device cache of
* embedded messages for the signed-in user. These are the messages the
* signed-in user is eligible to see.
*
* At key points during your app's lifecycle, you may want to manually refresh
* your app's local cache of embedded messages. For example, as users navigate
* around, on pull-to-refresh, etc.
*
* However, do not poll for new embedded messages at a regular interval.
*
* @example
* ```typescript
* IterableEmbeddedManager.syncMessages();
* ```
*/| * Iterable's iOS SDK automatically refresh a local, on-device cache of | |
| * Iterable's SDK automatically refreshes a local, on-device cache of |
| // If no placement IDs provided, we need to get messages for all possible placements | ||
| // Since the Android SDK requires a placement ID, we'll use 0 as a default | ||
| // This might need to be adjusted based on the actual SDK behavior | ||
| List<IterableEmbeddedMessage> messages = IterableApi.getInstance().getEmbeddedManager().getMessages(0L); | ||
| 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.
Using 0 as a default placement ID when none are provided is questionable and may not work as expected. The comment acknowledges this with "This might need to be adjusted based on the actual SDK behavior."
This should either:
- Return an empty list when no placement IDs are provided (if that's the expected behavior)
- Fetch all messages across all known placements by iterating through
getPlacementIds() - Be documented clearly if
0is a valid special value in the Iterable Android SDK
Consider implementing option 2:
if (placementIds == null || placementIds.size() == 0) {
List<Long> allPlacementIds = IterableApi.getInstance().getEmbeddedManager().getPlacementIds();
if (allPlacementIds != null) {
for (Long placementId : allPlacementIds) {
List<IterableEmbeddedMessage> messages = IterableApi.getInstance().getEmbeddedManager().getMessages(placementId);
if (messages != null) {
allMessages.addAll(messages);
}
}
}
}| // If no placement IDs provided, we need to get messages for all possible placements | |
| // Since the Android SDK requires a placement ID, we'll use 0 as a default | |
| // This might need to be adjusted based on the actual SDK behavior | |
| List<IterableEmbeddedMessage> messages = IterableApi.getInstance().getEmbeddedManager().getMessages(0L); | |
| if (messages != null) { | |
| allMessages.addAll(messages); | |
| // If no placement IDs provided, get messages for all known placements | |
| List<Long> allPlacementIds = IterableApi.getInstance().getEmbeddedManager().getPlacementIds(); | |
| if (allPlacementIds != null) { | |
| for (Long placementId : allPlacementIds) { | |
| List<IterableEmbeddedMessage> messages = IterableApi.getInstance().getEmbeddedManager().getMessages(placementId); | |
| if (messages != null) { | |
| allMessages.addAll(messages); | |
| } | |
| } |
| .then((ids: number[]) => Iterable.embeddedManager.getMessages(ids)) | ||
| .then((messages: IterableEmbeddedMessage[]) => { | ||
| setEmbeddedMessages(messages); | ||
| console.log(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.
[nitpick] Missing error handling for the promise chain. If any of the promises reject (e.g., network error, SDK error), the error will be unhandled and could cause issues in the example app. Consider adding a .catch() block:
const getEmbeddedMessages = useCallback(() => {
getPlacementIds()
.then((ids: number[]) => Iterable.embeddedManager.getMessages(ids))
.then((messages: IterableEmbeddedMessage[]) => {
setEmbeddedMessages(messages);
console.log(messages);
})
.catch((error) => {
console.error('Failed to get embedded messages:', error);
});
}, [getPlacementIds]);| console.log(messages); | |
| console.log(messages); | |
| }) | |
| .catch((error) => { | |
| console.error('Failed to get embedded messages:', error); |
| text?: { id: string; text?: string | null; label?: string | null }[] | null; | ||
| title?: string | null; | ||
| } | null; | ||
| payload?: { [key: string]: string | number | boolean | null } | 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.
Type mismatch: The payload property in the EmbeddedMessage interface is defined as { [key: string]: string | number | boolean | null }, but in IterableEmbeddedMessage it's defined as Record<string, unknown>.
The native interface is more restrictive (only allows primitive types and null), while the TypeScript interface is more permissive (allows any value including nested objects/arrays). This could cause type safety issues.
Consider either:
- Aligning the types to match (likely the native version is correct based on what the native SDKs actually return)
- Adding a type guard or runtime validation when converting from native to TypeScript types
Suggested fix in IterableEmbeddedMessage.ts:
payload?: Record<string, string | number | boolean | null> | null;| * @param placementIds - The placement IDs to retrieve messages for. | ||
| * @returns A Promise that resolves to an array of embedded 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 documentation for getMessages() doesn't explain the behavior when placementIds is null or an empty array. Based on the Android implementation, this is an important detail to document since the behavior is unclear (currently uses placement ID 0 as a default, which may not be correct).
Suggested documentation improvement:
/**
* Retrieves a list of embedded messages the user is eligible to see.
*
* @param placementIds - The placement IDs to retrieve messages for.
* Pass null or an empty array to get messages for all placements.
* @returns A Promise that resolves to an array of embedded messages.
* @example
* ```typescript
* // Get messages for specific placements
* const messages = await Iterable.embeddedManager.getMessages([123, 456]);
*
* // Get messages for all placements
* const allMessages = await Iterable.embeddedManager.getMessages(null);
* ```
*/| * @param placementIds - The placement IDs to retrieve messages for. | |
| * @returns A Promise that resolves to an array of embedded messages. | |
| * @param placementIds - The placement IDs to retrieve messages for. | |
| * Pass `null` or an empty array to get messages for all placements. | |
| * @returns A Promise that resolves to an array of embedded messages. | |
| * @example | |
| * ```typescript | |
| * // Get messages for specific placements | |
| * const messages = await Iterable.embeddedManager.getMessages([123, 456]); | |
| * | |
| * // Get messages for all placements | |
| * const allMessages = await Iterable.embeddedManager.getMessages(null); | |
| * ``` |
… in IterableEmbeddedManager
| } catch (JSONException e) { | ||
| IterableLogger.e(TAG, e.getLocalizedMessage()); | ||
| promise.reject("", "Failed to fetch messages with error " + e.getLocalizedMessage()); | ||
| } |
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.
🔹 JIRA Ticket(s) if any
✏️ Description
Adds the ability to sync and get messages to android
Testing
yarn android