notif ios: Buffer notification tap events; same as on Android#2229
Open
rajveermalviya wants to merge 2 commits into
Open
notif ios: Buffer notification tap events; same as on Android#2229rajveermalviya wants to merge 2 commits into
rajveermalviya wants to merge 2 commits into
Conversation
rajveermalviya
force-pushed
the
pr-ios-notif-fix
branch
from
5c3f7b3 to
3996923
Compare
rajveermalviya
added
the
maintainer review
rajveermalviya
force-pushed
the
pr-ios-notif-fix
branch
from
3996923 to
2615cfb
Compare
chrisbobbe
reviewed
Mar 26, 2026
Collaborator
chrisbobbe
left a comment
chrisbobbe
left a comment
There was a problem hiding this comment.
This looks like a helpful simplification! I haven't tested manually yet, and see one comment below. I'll take another look (and do that manual testing) when this is un-marked as a draft.
| // https://github.com/zulip/zulip-flutter/pull/2043#discussion_r2794138972 | ||
| // TODO it'd be nice to avoid that glitch by controlling the initial route. | ||
| // We accept this glitch as a workaround for an upstream issue: | ||
| // https://github.com/flutter/flutter/issues/178305 |
Collaborator
There was a problem hiding this comment.
The linked Flutter issue appears to be Android-specific, so this reasoning doesn't hold in the iOS case.
chrisbobbe
reviewed
Mar 30, 2026
| } | ||
|
|
||
| @HostApi() | ||
| abstract class NotificationHostApi { |
Collaborator
There was a problem hiding this comment.
Can we keep NotificationHostApi (without any methods), as a home for future functionality e.g. two methods I propose in #2257?
Member
Author
There was a problem hiding this comment.
I tried this, but when we have an empty NotificationHostApi then Pigeon generates a _extractReplyValueOrThrow which ends up being unused. And because of that analyzer ends up complaining about it.
rajveermalviya
force-pushed
the
pr-ios-notif-fix
branch
2 times, most recently
from
32008cd to
43cc2ad
Compare
Member
Author
|
This is ready for review now. |
It turns out that `userNotificationCenter(_:didReceive:withCompletionHandler:)` does execute when app is terminated and later opened by tapping on a notification, so there is no need to check `launchOptions` in `application(_:didFinishLaunchingWithOptions:)` to handle this case. But because we currently handle both cases, there is a potential bug where if both are handled, it could cause double navigations for the same notification. This is not observed in practice, probably because we do not buffer events from `userNotificationCenter(_:didReceive:withCompletionHandler:)` currently and those events may be missed until Flutter init is complete and the handler (in NotificationOpenService.init) is setup. So, remove the unnecessary handlers for `launchOptions`. This also unifies our notification opening implementation between Android and iOS, where we handle all types of notification tap events via the same Pigeon EventChannel API.
rajveermalviya
force-pushed
the
pr-ios-notif-fix
branch
from
43cc2ad to
bd0306b
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
It turns out that
userNotificationCenter(_:didReceive:withCompletionHandler:)does execute when app is terminated and later opened by tapping on a notification, so there is no need to checklaunchOptionsinapplication(_:didFinishLaunchingWithOptions:)to handle this case.But because we currently handle both cases, there is a potential bug where if both are handled, it could cause double navigations for the same notification. This is not observed in practice, probably because we do not buffer events from
userNotificationCenter(_:didReceive:withCompletionHandler:)currently and those events may be missed until Flutter init is complete and the handler (in NotificationOpenService.init) is setup.So, remove the unnecessary handlers for
launchOptions. This also unifies our notification opening implementation between Android and iOS, where we handle all types of notification tap events via the same Pigeon EventChannel API.