Conversation
WalkthroughThe changes focus on enhancing push notification handling in an iOS application using the Ably Flutter plugin. The modifications include updating the Changes
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (5)
✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
temporarily deployed
to
staging/pull/560/features
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
example/ios/Runner/AppDelegate.swift (2)
18-23: Consider using modern UNUserNotificationCenter API.While this example is intentionally simple, it's using a deprecated API. Consider adding a second example using the recommended
UNUserNotificationCenterAPI to demonstrate best practices.Here's an example implementation using the modern API:
func userNotificationCenter(_ center: UNUserNotificationCenter, didReceive response: UNNotificationResponse, withCompletionHandler completionHandler: @escaping () -> Void) { let userInfo = response.notification.request.content.userInfo NSLog("Notification received: \(userInfo)") completionHandler() }
18-23: Enhance the example push notification handling.While this provides a basic example, it uses a deprecated API and only logs the notification. Consider:
- Using the recommended
UNUserNotificationCenterDelegatemethods instead- Adding more comprehensive example handling
Here's an improved example using modern APIs:
- override func application(_ application: UIApplication, didReceiveRemoteNotification userInfo: [AnyHashable : Any], fetchCompletionHandler completionHandler: @escaping (UIBackgroundFetchResult) -> Void) { - NSLog("Notification received: \(userInfo)") - } + // MARK: - UNUserNotificationCenterDelegate + + func userNotificationCenter( + _ center: UNUserNotificationCenter, + willPresent notification: UNNotification, + withCompletionHandler completionHandler: @escaping (UNNotificationPresentationOptions) -> Void + ) { + let userInfo = notification.request.content.userInfo + NSLog("Notification received in foreground: \(userInfo)") + + // Example: Show banner and play sound for important notifications + if let aps = userInfo["aps"] as? [String: Any], + let category = aps["category"] as? String, + category == "IMPORTANT" { + completionHandler([.banner, .sound]) + } else { + completionHandler([]) + } + } + + func userNotificationCenter( + _ center: UNUserNotificationCenter, + didReceive response: UNNotificationResponse, + withCompletionHandler completionHandler: @escaping () -> Void + ) { + let userInfo = response.notification.request.content.userInfo + NSLog("Notification response received: \(userInfo)") + + // Example: Handle notification tap + switch response.actionIdentifier { + case UNNotificationDefaultActionIdentifier: + // Handle default tap action + break + case UNNotificationDismissActionIdentifier: + // Handle dismiss action + break + default: + // Handle custom actions + break + } + + completionHandler() + }example/ios/Runner/Info.plist (2)
5-6: Document the new configuration key.The new configuration key is correctly implemented. Consider adding documentation in the README or migration guide to explain:
- Purpose of the
AblyFlutterHandlePushNotificationskey- Default value and its implications
- Steps to opt out of default push handling
5-6: LGTM: Well-structured configuration key.The new configuration key follows platform conventions and maintains backward compatibility with its default value of
true.Consider adding a comment in the Info.plist to document this feature:
<key>AblyFlutterHandlePushNotifications</key> + <!-- Set to false to opt out of Ably's default push notification handling --> <true/>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
example/ios/Runner/AppDelegate.swift(1 hunks)example/ios/Runner/Info.plist(1 hunks)ios/Classes/AblyFlutter.m(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: android (29)
- GitHub Check: ios
- GitHub Check: android (24)
- GitHub Check: android
- GitHub Check: ios (iPhone 15)
🔇 Additional comments (6)
example/ios/Runner/AppDelegate.swift (2)
4-4: LGTM! Modern Swift syntax used.The update from
@UIApplicationMainto@mainfollows modern Swift conventions.
4-4: LGTM: Modern Swift syntax used.The change from
@UIApplicationMainto@mainfollows modern Swift conventions.ios/Classes/AblyFlutter.m (4)
698-702: LGTM! Proper handling of configuration with backward compatibility.The implementation correctly:
- Reads the configuration from Info.plist
- Provides a default value of YES for backward compatibility
- Safely handles the case when the key is not present
724-729: LGTM! Clean conditional initialization of push handlers.The push notification handlers are only initialized when handleAPNs is true, effectively implementing the opt-out feature.
698-702: LGTM: Clean implementation of opt-out feature.The implementation properly reads the configuration from Info.plist with null checking and maintains backward compatibility by defaulting to
YES.
724-729: LGTM: Safe initialization of push notification handlers.The code properly preserves the existing notification center delegate and only initializes the handlers when enabled.
github-actions
bot
temporarily deployed
to
staging/pull/560/features
|
Hey @maratal, I'm here in response to us being asked if we intend to merge this into
So, my questions:
|
Both FCM and Ably Flutter intercept incoming push. It wasn't causing crash in my experiments, but looks like for some users it does. FCM has similar option to opt-out, so I think it makes sense to give users this option in Ably Flutter too. It will help for some users, but turned out that at least one client doesn't have the control over his client's code, that's why I've made it to be opted out by default. |
|
If we make the handling opt-out, what guidance would we give to customers for this option? It would have to be something like "some users have experienced crashes that may be related to this SDK's default handling of push notifications; here's how to opt out". Is that the plan? If we were to make it opt-in, what would be the consequences for the existing users of the library? As for reproducing the crash, do we have any customer who has been able to reproduce it? |
maratal
force-pushed
the
fix/557-opt-out-push-handler
branch
from
814ed15 to
4399f09
Compare
ttypic
force-pushed
the
fix/557-opt-out-push-handler
branch
from
4399f09 to
9c0bc13
Compare
|
Hello! I'm still not sure how to opt out of using PN on Flutter iOS? Currently, I can't use Ably for the WebSocket services because I'm using Firebase messaging for PN. Edit: After checking the commit comments, it turns out this flag must be added in the Info.plist: |
4 participants
Closes #557
Some users use APNs via FCM, so there should be an option to turn-off default handling of push notifications in iOS Ably Flutter to give user an ability to handle it themselves or via FCM framework (similar opt-out exists in android's Ably Flutter via AndroidManifest).
Summary by CodeRabbit
New Features
Improvements