-
Notifications
You must be signed in to change notification settings - Fork 0
Add support for tracking push notification opens [sc-184466] #31
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: main
Are you sure you want to change the base?
Conversation
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 support for tracking push notification opens by introducing a new handlePushNotificationOpenedWithContent method across the React Native bridge. The PR updates both iOS and Android native implementations, along with the example app to demonstrate the feature integration using the react-native-notifications library.
Changes:
- Added
handlePushNotificationOpenedWithContentmethod to the TypeScript interface and native implementations (iOS Swift/Objective-C and Android Kotlin) - Upgraded MovableInk SDK versions (iOS to 2.3.0, Android to 2.3.0-SNAPSHOT)
- Integrated
react-native-notificationslibrary in the example app with complete push notification setup for both platforms - Updated build configurations including Kotlin version (2.2.0), Java compatibility (Java 17), and Firebase dependencies
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/index.tsx | Added TypeScript interface method for handling push notification opens |
| ios/RNMovableInk.swift | Implemented Swift method to handle push notification content with Sendable type safety |
| ios/RNMovableInk.m | Exported the new method to React Native bridge |
| react-native-movable-ink.podspec | Updated MovableInk dependency configuration for testing |
| android/src/main/java/com/rnmovableink/RNMovableInkModule.kt | Added Kotlin method to handle push notifications with type conversion |
| android/build.gradle | Updated SDK version, Java/Kotlin compatibility, and Maven repository |
| android/gradle.properties | Upgraded Kotlin version to 2.2.0 |
| example/src/App.tsx | Integrated notification handling with complete lifecycle management |
| example/package.json | Added react-native-notifications dependency |
| example/yarn.lock | Lock file update for new dependency |
| example/ios/MovableInkExample/AppDelegate.mm | Added iOS notification delegate methods |
| example/ios/MovableInkExample/MovableInkExample.entitlements | Added APS environment for push notifications |
| example/android/settings.gradle | Included react-native-notifications module |
| example/android/build.gradle | Added Firebase and Kotlin dependencies |
| example/android/app/build.gradle | Added Firebase BOM and messaging dependencies |
| example/android/app/src/main/java/com/movableink/MainApplication.kt | Added unused RNNotificationsPackage import |
| example/android/app/src/main/AndroidManifest.xml | Changed to relative package references |
| example/android/app/google-services.json | Added Firebase configuration with production credentials |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| mavenCentral() | ||
| maven { | ||
| url 'https://s01.oss.sonatype.org/content/repositories/snapshots/' | ||
| url 'https://central.sonatype.com/repository/maven-snapshots/' |
Copilot
AI
Jan 30, 2026
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 Maven repository URL has been changed from 's01.oss.sonatype.org/content/repositories/snapshots/' to 'central.sonatype.com/repository/maven-snapshots/'. However, 'central.sonatype.com' is not a valid Maven repository URL for fetching artifacts. The correct URL for Sonatype Central snapshots should be 'https://s01.oss.sonatype.org/content/repositories/snapshots/' or 'https://oss.sonatype.org/content/repositories/snapshots/'. The current URL will likely fail when attempting to resolve dependencies.
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.
So this is interesting, when it was https://s01.oss.sonatype.org/content/repositories/snapshots/, the snapshot couldn't be found, but when I changed it, it could... @Graydyn Any ideas about this?
| val map: Map<String, String> = properties.toHashMap().mapValues { it.value.toString() } | ||
| MIClient.handlePushNotificationOpened(map) | ||
| } | ||
|
|
Copilot
AI
Jan 30, 2026
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 type conversion using toString() may not preserve the original type information from the notification payload. This converts all values to strings, which could cause issues if the underlying MIClient.handlePushNotificationOpened expects other types like booleans, numbers, or nested objects. Consider preserving the original types or documenting that all notification data must be string-based.
| val map: Map<String, String> = properties.toHashMap().mapValues { it.value.toString() } | |
| MIClient.handlePushNotificationOpened(map) | |
| } | |
| MIClient.handlePushNotificationOpened(properties.toHashMap()) | |
| } |
| compileSdkVersion rootProject.ext.compileSdkVersion | ||
|
|
||
| namespace "com.movableink.app" | ||
| namespace "com.movableink" |
Copilot
AI
Jan 30, 2026
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 namespace has been changed from "com.movableink.app" to "com.movableink", but the applicationId remains "com.movableink.app". While this is valid (namespace and applicationId can differ), it could be confusing and may lead to issues. Ensure this discrepancy is intentional, as the namespace affects how Android resolves package references while applicationId is the unique identifier on Google Play.
| namespace "com.movableink" | |
| namespace "com.movableink.app" |
| s.dependency "MovableInk" | ||
|
|
||
| # Disable when testing local pod | ||
| s.dependency "MovableInk", "2.2.0" | ||
| # s.dependency "MovableInk", "2.3.0" |
Copilot
AI
Jan 30, 2026
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 MovableInk dependency is configured for local testing but should be pinned to a specific version for production use. The commented version "2.3.0" appears to be the intended version, but the active dependency doesn't specify a version, which could lead to unpredictable builds. Consider uncommenting the version-specific dependency and commenting out the versionless one.
| import com.facebook.react.flipper.ReactNativeFlipper | ||
| import com.facebook.soloader.SoLoader | ||
| import com.rnmovableink.BuildConfig | ||
| import com.wix.reactnativenotifications.RNNotificationsPackage; |
Copilot
AI
Jan 30, 2026
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 RNNotificationsPackage import is unused. Since React Native's autolinking automatically includes the react-native-notifications package, this manual import is not necessary and can be removed.
| import com.wix.reactnativenotifications.RNNotificationsPackage; |
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.
Their docs explicitly say to add this:
https://wix.github.io/react-native-notifications/docs/installation-android
|
|
||
| <application | ||
| android:name="com.movableink.MainApplication" | ||
| android:name=".MainApplication" |
Copilot
AI
Jan 30, 2026
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 android:name is changed to use relative package resolution ('.MainApplication' instead of 'com.movableink.MainApplication'). While this works with the namespace change in build.gradle (line 106), ensure this is intentional. The relative reference depends on the namespace being correctly set, which could cause issues if the namespace is ever misconfigured.
| android:name=".MainApplication" | |
| android:name="com.movableink.MainApplication" |
260c9ec to
2499552
Compare
No description provided.