-
Notifications
You must be signed in to change notification settings - Fork 40
[MOB-12261] Add IterableEmbeddedManager class #742
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-12260-create-embedded-tab-in-example-app
Are you sure you want to change the base?
[MOB-12261] Add IterableEmbeddedManager class #742
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.45%. File Coverage Changes
🛟 Help
This is from Qlty Cloud, the successor to Code Climate Quality. Learn more. |
…-app' into loren/embedded/MOB-12261-add-embedded-manager-class
…-app' into loren/embedded/MOB-12261-add-embedded-manager-class
…-app' into loren/embedded/MOB-12261-add-embedded-manager-class
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 introduces the IterableEmbeddedManager class as the foundation for embedded messaging functionality in the Iterable React Native SDK. The implementation adds the manager class with basic enabled/disabled state management and integrates it with the SDK's configuration system.
Key changes:
- Adds
IterableEmbeddedManagerclass withisEnabledproperty andsetEnabled()method - Introduces
embeddedMessagingEnabledconfiguration option inIterableConfig - Integrates the embedded manager as a static property on the
Iterableclass, initialized during SDK setup
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/embedded/classes/IterableEmbeddedManager.ts |
New manager class with basic enabled state management |
src/embedded/classes/index.ts |
Export file for embedded classes |
src/embedded/index.ts |
Main export file for embedded module |
src/core/classes/IterableConfig.ts |
Adds embeddedMessagingEnabled configuration property and includes it in serialization |
src/core/classes/Iterable.ts |
Adds embeddedManager static property and initializes it based on config during SDK setup |
src/index.tsx |
Exports IterableEmbeddedManager from the main SDK entry point |
example/src/components/Embedded/Embedded.tsx |
Example component demonstrating how to check manager existence and enabled state |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| isEnabled = false; | ||
|
|
||
| /** | ||
| * Sets whether the embedded manager is enabled. | ||
| * | ||
| * @param enabled - Whether the embedded manager is enabled. | ||
| */ | ||
| setEnabled(enabled: boolean) { | ||
| this.isEnabled = enabled; |
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 isEnabled property should be readonly or private to prevent direct mutation from outside the class. Currently, consumers can bypass setEnabled() and directly modify the property (e.g., Iterable.embeddedManager.isEnabled = true), which could lead to inconsistent state management.
Consider making this property readonly and providing a getter method, or making it private with a public getter:
private _isEnabled = false;
get isEnabled(): boolean {
return this._isEnabled;
}Then update setEnabled() to use this._isEnabled = enabled;
| isEnabled = false; | |
| /** | |
| * Sets whether the embedded manager is enabled. | |
| * | |
| * @param enabled - Whether the embedded manager is enabled. | |
| */ | |
| setEnabled(enabled: boolean) { | |
| this.isEnabled = enabled; | |
| private _isEnabled = false; | |
| /** | |
| * Gets whether the embedded manager is enabled. | |
| */ | |
| get isEnabled(): boolean { | |
| return this._isEnabled; | |
| } | |
| /** | |
| * Sets whether the embedded manager is enabled. | |
| * | |
| * @param enabled - Whether the embedded manager is enabled. | |
| */ | |
| setEnabled(enabled: boolean) { | |
| this._isEnabled = enabled; |
| * Iterable.embeddedManager.getMessages().then(messages => { | ||
| * console.log('Messages:', 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 example shows calling getMessages() method, but the IterableEmbeddedManager class does not implement this method. The current implementation only has isEnabled property and setEnabled() method.
Either update the example to reflect the current API:
// Check if embedded manager is enabled
if (Iterable.embeddedManager.isEnabled) {
console.log('Embedded messaging is enabled');
}Or remove the example until getMessages() is implemented.
| * Iterable.embeddedManager.getMessages().then(messages => { | |
| * console.log('Messages:', messages); | |
| * }); | |
| * // Check if embedded manager is enabled | |
| * if (Iterable.embeddedManager.isEnabled) { | |
| * console.log('Embedded messaging is enabled'); | |
| * } |
| Iterable.embeddedManager.setEnabled( | ||
| config.embeddedMessagingEnabled === true | ||
| ); |
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 new embeddedMessagingEnabled configuration option and embeddedManager integration lack test coverage. Similar configuration options like logReactNativeSdkCalls have comprehensive test coverage in Iterable.test.ts.
Consider adding tests for:
- Default value of
embeddedMessagingEnabled(should befalse) embeddedMessagingEnabledbeing included in thetoDict()outputembeddedManager.isEnabledbeing set correctly duringIterable.initialize()based on the config valueembeddedManagerbeing accessible as a static property
Example test structure:
describe('embeddedManager', () => {
it('should be disabled by default', () => {
const config = new IterableConfig();
expect(config.embeddedMessagingEnabled).toBe(false);
expect(Iterable.embeddedManager.isEnabled).toBe(false);
});
it('should enable embeddedManager when config is set', async () => {
const config = new IterableConfig();
config.embeddedMessagingEnabled = true;
await Iterable.initialize('test-key', config);
expect(Iterable.embeddedManager.isEnabled).toBe(true);
});
});| * - [Android Embedded Messaging](https://support.iterable.com/hc/en-us/articles/23061877893652-Embedded-Messages-with-Iterable-s-Android-SDK) | ||
| * - [iOS Embedded Messaging](https://support.iterable.com/hc/en-us/articles/23061840746900-Embedded-Messages-with-Iterable-s-iOS-SDK) | ||
| */ | ||
| embeddedMessagingEnabled = false; |
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 test that validates default configuration values in Iterable.test.ts does not include verification for the new embeddedMessagingEnabled property. The test should be updated to include:
expect(config.embeddedMessagingEnabled).toBe(false);
expect(configDict.embeddedMessagingEnabled).toBe(false);This ensures the new configuration option has proper test coverage for its default value and serialization.
src/core/classes/Iterable.ts
Outdated
| IterableLogger.setLogLevel(config.logLevel); | ||
|
|
||
| Iterable.embeddedManager.setEnabled( | ||
| config.embeddedMessagingEnabled === true |
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] The explicit === true comparison is redundant since config.embeddedMessagingEnabled is already typed as a boolean and defaults to false. This is inconsistent with the pattern used on line 195 for config.logReactNativeSdkCalls.
Consider simplifying to match the existing pattern:
Iterable.embeddedManager.setEnabled(
config.embeddedMessagingEnabled ?? false
);Or if you prefer simpler syntax since the default is already false:
Iterable.embeddedManager.setEnabled(config.embeddedMessagingEnabled);| config.embeddedMessagingEnabled === true | |
| config.embeddedMessagingEnabled ?? false |
…-app' into loren/embedded/MOB-12261-add-embedded-manager-class
1 new issue
This is from Qlty Cloud, the successor to Code Climate Quality. Learn more. |
🔹 JIRA Ticket(s) if any
✏️ Description
Adds the embedded manager class
Testing
yarn install watchman watch-del-all yarn start --reset-cache ```bashyarn ios) or android (yarn android)Does embedded class exist? Yes
Is embedded manager enabled? Yes
example/src/hooks/useIterableApp.tsxconfig.enableEmbeddedMessaging = true;toconfig.enableEmbeddedMessaging = false;yarn start), and hit the keyrDoes embedded class exist? Yes
Is embedded manager enabled? No