ios: Fix bug where tapping a notification didn't navigate to conversation #5676
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks! Glad you spotted this happening, and that we've learned all the things we've learned in this investigation. Reading the changes now. One process note up front:
Please file a quick issue for this in the tracker. Doesn't need more info than those two sentences, and the fact that you've done some investigation and have a draft fix. The benefit of having an issue filed is that there's then a crisp, stable identifier that you can mention in language like: as well as this PR description and the commit message where you fix the bug, so that it's easy to see they're all meant to refer to the same bug. |
gnprice
reviewed
Mar 10, 2023
Comment on lines
15
to
17
|
|
||
| @interface RCT_EXTERN_MODULE(ZLPNotificationsEvents, RCTEventEmitter) | ||
| @end |
There was a problem hiding this comment.
nit: put these in the same order as the classes appear in the Swift file
(I don't have a preference which order that is.)
Apple's documentation for this UIApplication method ( https://developer.apple.com/documentation/uikit/uiapplicationdelegate/1623013-application ) says at the top: > Tells the app that a remote notification arrived that indicates > there is data to be fetched. From experimentation, it turns out that the qualifier "that indicates…" is quite important! It seems to mean that the method won't be called except for "background update notifications". That's a special kind of notification that isn't mentioned by name in that doc, but is discussed in a section of a different doc, on creating remote notification payloads: https://developer.apple.com/library/archive/documentation/NetworkingInternet/Conceptual/RemoteNotificationsPG/CreatingtheNotificationPayload.html On my iPhone 13 Pro running iOS 16.1, using breakpoint debugging in Xcode, I don't see the method called when I receive a notification from a Zulip server. However, if I have my dev server send `content-available: 1` in the APNs payload (making it a "background update notification"; see doc linked just above), then I *do* see the method called! I don't know any other way to interpret the qualifier in the method's doc. My experiment was prompted by this Apple forum thread: https://developer.apple.com/forums/thread/128794 It seems like the doc should say more explicitly what kind of notifications will trigger the call. Anyway, we don't currently need to be told at all when a notification is received, whether it's this special kind or not. What we care about is the user's *response* to a notification: when they tap it, we want to bring the user to the relevant conversation in the app. From reading code in @react-native-community/push-notification-ios and from some experiments, this UIApplication method is the only thing that would ever cause our `listenIOS('notification', …)` callback to fire. So, remove that too. …And since we were expecting that JavaScript code to handle the user's notification *response* by navigating to a conversation, we note that that feature must've been broken for a while. Filed just now as issue zulip#5679. It doesn't help that the doc for @react-native-community/push-notification-ios incorrectly identifies 'notification' as the event carrying the user's response: https://github.com/react-native-push-notification/ios#how-to-determine-push-notification-user-click …Anyway, we have a planned fix for zulip#5679, coming next. Thankfully it involves using less of that library, not more. Not marking NFC because, especially with how unhelpful that Apple doc is, I guess it's possible that the system *was* calling our method override even without `content-available: 1` in the APNs payloads. If so, this will be a bugfix: in those cases, before this commit, the app would navigate to a notification's conversation without being prompted by the user tapping on the notification. Searching the tracker, the symptom in issue zulip#4763 matches that description ("[ios] Opening app from recents screen can reopen conversation from previous notification"), but in a quick test just now I wasn't able to reproduce that issue on current `main`, and it's time to move on to other work.
…tions We're about to add a new class to the same file.
This will be useful soon when we have listenIOS use a new NativeEventEmitter we've written, instead of relying on the emitter @react-native-community/push-notification-ios gives us, which is buggy. The `addListener` method of our new NativeEventEmitter will be typed such that the passed event name and handler are checked against each other. Now that listenIOS has a single param with both the name and the handler, it'll be easy to write it so it expects one or another kind of name/handler pair, by typing its param as a union of object types. Then the new caller can pass a value that flows through one branch of the union, to the new NativeEventEmitter, without changing any type constraints on the existing callers.
…tion For no apparent reason, the library we've been using for iOS notifications (@react-native-community/push-notification-ios) sends remote notification response events, including the simple response of tapping on a remote notification, with the name 'localNotification' instead of the name they document, which is 'notification': https://github.com/react-native-push-notification/ios/blob/v1.10.1/README.md#how-to-determine-push-notification-user-click (For what their 'notificaton' event actually means, see a recent commit where we removed our handler for it.) Rather than giving in and just listening for 'localNotification', set up our own native module to forward the event that iOS sends to the app. This took some time, but it makes the logic more transparent and removes some dependence on that library, which are nice outcomes. There was some custom `RCTConvert` and JavaScript munging to read through in the library's codepath, but thankfully the part that affects what we actually use is small: just a call to `RCTJSONClean`. Fixes: zulip#5679
chrisbobbe
force-pushed
the
pr-fix-ios-notif
branch
from
0f8c07e to
2ae6f79
Compare
|
Thanks for the review! Revision pushed, and I kicked the tires on Android and didn't get a crash or anything. |
|
Thanks! Looks good; merging. |
chrisbobbe
added a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Jun 1, 2023
…ation" And handle it ourselves, through our dedicated iOS native module ZLPNotificationsStatus. (The "initial notification" is a notification that, by being tapped, launched the app from cold. PR zulip#5676 was about notifications that are tapped when the app is already open, and that PR fixed a known bug.) This, like zulip#5676, makes our iOS notification handling more transparent. My hope is to smooth the path to custom iOS notification code in zulip-flutter (zulip-flutter#122, etc.) by demonstrating a working implementation that's simple and avoids depending on the details of @react-native-community/push-notification-ios. Cutting the library out of this codepath makes one small difference in the payload we pass to `fromAPNs` on the JS side: the `aps` property is now present. (So, remove a comment saying it might be absent). We've never looked at that property, so this is NFC. Unlike in 2ae6f79 / zulip#5676, there wasn't a call to the library's custom RCTConvert extensions to account for, as we did there with a cautious call to RCTJSONClean.
chrisbobbe
added a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Jun 1, 2023
…ation" And handle it ourselves, through our dedicated iOS native module ZLPNotificationsStatus. (The "initial notification" is a notification that, by being tapped, launched the app from cold. PR zulip#5676 was about notifications that are tapped when the app is already open, and that PR fixed a known bug.) This, like zulip#5676, makes our iOS notification handling more transparent. My hope is to smooth the path to custom iOS notification code in zulip-flutter (zulip-flutter#122, etc.) by demonstrating a working implementation that's simple and avoids depending on the details of @react-native-community/push-notification-ios. Cutting the library out of this codepath makes one small difference in the payload we pass to `fromAPNs` on the JS side: the `aps` property is now present. (So, remove a comment saying it might be absent). We've never looked at that property, so this is NFC. Unlike in 2ae6f79 / zulip#5676, there wasn't a call to the library's custom RCTConvert extensions to account for, as we did there with a cautious call to RCTJSONClean.
chrisbobbe
added a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Jun 1, 2023
…ation" And handle it ourselves, through our dedicated iOS native module ZLPNotificationsStatus. (The "initial notification" is a notification that, by being tapped, launched the app from cold. PR zulip#5676 was about notifications that are tapped when the app is already open, and that PR fixed a known bug.) This, like zulip#5676, makes our iOS notification handling more transparent. My hope is to smooth the path to custom iOS notification code in zulip-flutter (zulip-flutter#122, etc.) by demonstrating a working implementation that's simple and avoids depending on the details of @react-native-community/push-notification-ios. Cutting the library out of this codepath makes one small difference in the payload we pass to `fromAPNs` on the JS side: the `aps` property is now present. (So, remove a comment saying it might be absent). We've never looked at that property, so this is NFC. Unlike in 2ae6f79 / zulip#5676, there wasn't a call to the library's custom RCTConvert extensions to account for, as we did there with a cautious call to RCTJSONClean.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I've noticed a bug lately where I tap a notification on iOS and it doesn't take me to the relevant conversation. It's only active when the app is open (in the foreground or background).
TODO: Testing on Android that the JS changes don't break anything (I plan to do this before merging, but the code is reviewable in the meantime).Done!Fixes: #5679