Skip to content

notif ios: Handle opening of conversation on tap; take 2 #1379

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

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

rajveermalviya
Copy link
Member

@rajveermalviya rajveermalviya commented Feb 26, 2025

Fixes #1147.

2nd attempt, first attempt was #1261. This one uses pigeon to move most of the notification payload handling to dart side (and doesn't use/rely on zulip://notification URL).

@rajveermalviya rajveermalviya force-pushed the dev-ios-notif-2 branch 5 times, most recently from 463b9ee to 8478226 Compare March 6, 2025 14:53
@rajveermalviya rajveermalviya marked this pull request as ready for review March 6, 2025 14:54
@rajveermalviya rajveermalviya force-pushed the dev-ios-notif-2 branch 2 times, most recently from 0142dbd to 89df63b Compare March 6, 2025 17:29
@rajveermalviya rajveermalviya added the maintainer review PR ready for review by Zulip maintainers label Mar 10, 2025
@rajveermalviya rajveermalviya force-pushed the dev-ios-notif-2 branch 2 times, most recently from 80a34eb to 9c07740 Compare March 10, 2025 18:37
@chrisbobbe
Copy link
Collaborator

Ah this has gathered a conflict in lib/widgets/app.dart; could you resolve it please? (I see you did a few days ago, but looks like it's happened again; thanks. 🙂)

@rajveermalviya rajveermalviya force-pushed the dev-ios-notif-2 branch 3 times, most recently from 3b50218 to 2178fe6 Compare March 13, 2025 22:07
@rajveermalviya
Copy link
Member Author

(Rebased to main, Thanks!)

Copy link
Collaborator

@chrisbobbe chrisbobbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! There's a lot here, and I haven't gotten around to it all today. But here are some comments from an initial review.

GlobalLocalizations.zulipLocalizations = ZulipLocalizations.of(context);
return child!;
},
return DeferrredBuilderWidget(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: check spelling (here and in commit message)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(also spelling of _ZulipAppState.initState in the commit message)

Comment on lines 8 to 16
/// Provides access to the app's data.
///
/// There should be one of this widget, near the root of the tree.
///
/// See also:
/// * [GlobalStoreWidget.of], to get access to the data.
/// * [PerAccountStoreWidget], for the user's data associated with a
/// particular Zulip account.
class GlobalStoreWidget extends StatefulWidget {
// This is separate from [GlobalStoreWidget] only because we need
// a [StatefulWidget] to get hold of the store, and an [InheritedWidget] to
// provide it to descendants, and one widget can't be both of those.
class GlobalStoreWidget extends InheritedNotifier<GlobalStore> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to move this implementation comment here and delete the dartdoc? The implementation comment doesn't make sense in this context—saying GlobalStoreWidget is "separate from" GlobalStoreWidget.

child: PerAccountStoreWidget(accountId: eg.selfAccount.id,
child: RealmContentNetworkImage(src))));
await tester.pumpWidget(DeferrredBuilderWidget(
future: ZulipBinding.instance.getGlobalStoreUniquely(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We normally do this as testBinding.getGlobalStoreUniquely, right?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, for these tests that aren't specifically about GlobalStoreWidget, it would be simpler to use TestZulipApp instead, I think.

store: store,
child: PerAccountStoreWidget(
accountId: accountId,
child: MyWidgetWithMixin(key: widgetWithMixinKey)));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can use TestZulipApp for this test?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used TestZulipApp in tests where possible, but kept this unchanged because I was getting the same extraneous dep changes mentioned in the above TODO.

Comment on lines 41 to 65
case TargetPlatform.android:
case TargetPlatform.fuchsia:
case TargetPlatform.linux:
case TargetPlatform.macOS:
case TargetPlatform.windows:
// Do nothing; we don't offer notifications on these platforms.
break;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment is wrong about Android; we do offer notifications on Android.

A lot of the code added in this commit is implemented just for iOS, but named/documented as though it's cross-platform, because it doesn't say it's just for iOS.

I don't know if we plan to align the implementation with the names/docs or vice versa. The answer might be in the later commits (I haven't read them yet), but it would be helpful to comment on this in the commit message, I think.

List<Route<dynamic>> _handleGenerateInitialRoutesIos(_) {
// The `_ZulipAppState.context` lacks the required ancestors. Instead
// we use the Navigator which should be available when this callback is
// called and it's context should have the required ancestors.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: "its"

await tester.pump();
takeStartingRoutes();
matchesNavigation(check(pushedRoutes).single, account, message);
debugDefaultTargetPlatformOverride = null;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, awkward to have this teardown line so far away from its corresponding setup line.

How about instead passing variant: const TargetPlatformVariant({TargetPlatform.iOS})) to testWidgets?

final route = _routeForNotification(context, payload);
if (route == null) return; // TODO(log)

// TODO(nav): Better interact with existing nav stack on notif open
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should have an iOS counterpart to

for this? Or make that a both-platforms issue? That issue says:

(The iOS counterpart is covered by #1147, for navigating at all when a notification is tapped.)

but it seems reasonable to postpone this part of it; we'd just want to keep track of it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this PR is merged the potential fix for that issue would work on both platforms, because this PR consolidates the notification routing implementation on both iOS and Android.

@EventChannelApi()
abstract class NotificationHostEvents {
/// An event stream that emits a notification payload when
/// app encounters a notification tap, while the app is runnning.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: "the app encounters a notification tap, while the app is running."

await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot());
await prepare(tester);
await checkOpenNotification(tester, eg.selfAccount, eg.streamMessage());
debugDefaultTargetPlatformOverride = null;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(same comment about maybe using variant: const TargetPlatformVariant({TargetPlatform.iOS}))

@rajveermalviya
Copy link
Member Author

Thanks for the review @chrisbobbe! Pushed a new revision, PTAL.

Copy link
Collaborator

@chrisbobbe chrisbobbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Here's a more thorough review of the first four commits:

d1f648c app: Use DeferredBuilderWidget while loading GlobalStore
a5a0fff pigeon [nfc]: Rename pigeon file to notification -> android_notifications
ab3ff84 notif ios: Navigate when app launched from notification
4b2ade0 notif ios: Navigate when app running but in background

That leaves the last two commits:

28ea77f notif android: Migrate to cross-platform Pigeon API for navigation
616defe docs: Document testing push notifications on iOS Simulator

Actually for your next revision, could you send a new PR with everything except the "Migrate to cross-platform" commit? That one's pretty large, so makes sense to review separately.

@@ -0,0 +1,167 @@
// Autogenerated from Pigeon (v24.2.1), do not edit directly.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs an update for the Pigeon 25 upgrade e2aac35.

Comment on lines 279 to 315
/// The widget to build when [future] completes, with it's result
/// passed as `result`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: "its"

@@ -381,7 +381,7 @@ data class StoredNotificationSound (
)
}
}
private open class NotificationsPigeonCodec : StandardMessageCodec() {
private open class AndroidNotificationsPigeonCodec : StandardMessageCodec() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit-message nit:

pigeon [nfc]: Rename pigeon file to `notification` -> `android_notifications`

I think the "to" should be deleted? Or moved to replace the "->"?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pigeon [nfc]: Rename pigeon file `notifications.dart` to `android_notifications.dart`

commit-message nit: limit summary line length to 76 (this is 85).

Comment on lines 21 to 26
/// On iOS, this checks and returns value for the `remoteNotification` key
/// in the `launchOptions` map. The value could be either the raw APNs data
/// dictionary, if the launch of the app was triggered by a notification tap,
/// otherwise it will be null.
///
/// See: https://developer.apple.com/documentation/uikit/uiapplication/launchoptionskey/remotenotification
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be a bit more concise I think:

  /// Returns `launchOptions.remoteNotification`,
  /// which is the raw APNs data dictionary
  /// if the app launch was opened by a notification tap,
  /// else null. See Apple doc:
  ///   https://developer.apple.com/documentation/uikit/uiapplication/launchoptionskey/remotenotification

And this is only used on iOS at this commit, right; the "On iOS" can be added in the later commit where it also starts being used on Android.

Comment on lines 293 to 298
FakeNotificationPigeonApi? _notificationPigeonApi;

@override
FakeNotificationPigeonApi get notificationPigeonApi {
return (_notificationPigeonApi ??= FakeNotificationPigeonApi());
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, can use fewer lines:

  @override
  FakeNotificationPigeonApi get notificationPigeonApi =>
    (_notificationPigeonApi ??= FakeNotificationPigeonApi());

Comment on lines 64 to 68
func onNotificationTapEvent(data: NotificationPayloadForOpen) {
if let eventSink = eventSink {
eventSink.success(data)
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of the one class NotificationPayloadForOpen, could we have two separate classes NotificationDataFromLaunch and NotificationTapEvent? Perhaps with a Payload typedef helper:

typedef Payload = Map<Object?, Object?>;

class NotificationDataFromLaunch {
  const NotificationDataFromLaunch({required this.payload});

  /// The raw payload that is attached to the notification,
  /// holding the information required to carry out the navigation.
  final Payload payload;
}

class NotificationTapEvent {
  const NotificationTapEvent({required this.payload});

  /// The raw payload that is attached to the notification,
  /// holding the information required to carry out the navigation.
  final Payload payload;
}

I think the current naming makes the event-channel code harder to read than it needs to be. When reading the Pigeon example code, I see "event" used pretty consistently in the names of things. Here, we have both "data" (onNotificationTapEvent's param) and "payload", and "payload" is used ambiguously for a payload (NotificationPayloadForOpen.payload) and something that contains a payload (NotificationPayloadForOpen itself).

That would mean, for this method:

  func onNotificationTapEvent(event: NotificationTapEvent) {
    if let eventSink = eventSink {
      eventSink.success(event)
    }
  }

Comment on lines 30 to 31
@EventChannelApi()
abstract class NotificationHostEvents {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would NotificationEventChannelApi be a better name for this, to make it clearer what kind of thing it is?

// in global scope of the generated file. This is a helper class to
// namespace the notification related Pigeon API under a single class.
class NotificationPigeonApi {
final _notifInteractionHost = notif_pigeon.NotificationHostApi();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is _notifInteractionHost the best name for this? How about _hostApi? We might add more to NotificationHostApi that's not about interacting with notifications (such as a method to query the current notification permissions). Also, NotificationHostApi isn't the only code that's about interacting with notifications; notif_pigeon.notificationTapEvents is too.

Comment on lines 102 to 105
/// Navigates to the [MessageListPage] of the specific conversation
/// for the provided payload that was attached while creating the
/// notification.
Future<void> _navigateForNotification(NotificationPayloadForOpen payload) async {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another reason the NotificationPayloadForOpen rename would be helpful: currently, it's pretty unclear that this private method is only about notifications that come while the app is open. It'll be helpful for debugging if it's easier to see what code is about the launch notification vs. not.


testWidgets('(iOS) stream message', (tester) async {
addTearDown(testBinding.reset);
await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot());
Copy link
Collaborator

@chrisbobbe chrisbobbe Mar 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This add-self-account line looks boring; can we put it in prepare?

@rajveermalviya rajveermalviya force-pushed the dev-ios-notif-2 branch 3 times, most recently from 2e40fb0 to 02506ba Compare May 28, 2025 18:28
@rajveermalviya
Copy link
Member Author

Thanks for the review @gnprice! Pushed an update, PTAL.

@rajveermalviya rajveermalviya requested a review from gnprice May 28, 2025 18:57
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks for splitting these out as separate commits — that's very helpful for reading them. They all look good.
5292653 notif [nfc]: Rename NotificationOpenPayload methods
d130717 notif [nfc]: Refactor NotificationOpenService.routeForNotification
b2e0ed0 notif: Show a dialog if received malformed Android Notification URL

I've now finished reading the whole thing, and generally it all looks good; just a handful of comments below. A couple of them are for follow-ups we can pursue after launch.

There's one commit making a bunch of test changes which I'd like split up to make clearer — as is, I don't yet fully understand all the things it's doing, which means it's hard to be sure whether it's accidentally undermining some of the tests. (Which in turn would mean there could be a bug that the tests are no longer able to catch.)

Comment on lines 119 to 124
if (!dropStartingRoutes) {
check(pushedRoutes).isEmpty();
return;
}
await tester.pump();
takeStartingRoutes(account: account);
check(pushedRoutes).isEmpty();
}

Uri androidNotificationUrlForMessage(Account account, Message message) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commit f12f911 does several different things:

    notif test: Refactor tests for NotificationOpenService
---
 test/notifications/open_test.dart | 103 +++++++++++++++++++++++------------------------------
 1 file changed, 45 insertions(+), 58 deletions(-)
  • make init not handle adding the account to the store, moving that to its callers
  • OTOH make prepare add the account to the store by default, which it previously never did, moving that from its callers
  • add an account option to prepare, for using something other than eg.selfAccount
  • rename early to dropStartingRoutes and flip the direction of it
  • split out this androidNotificationUrlForMessage method from openNotification
  • a couple of other things

I understand some of the changes it's making. But it's hard to track all of them, and as a reviewer to check that they all line up and identify any changes in behavior they might make, because there are so many things combined in one.

It's also hard to tell what the motivation is for many of them, partly because it seems like there are probably two or three different motivations for different changes.

So these would be helpful to split into several commits:

  • Those first three points, about managing whether to involve an account and which one, would be at least one commit of their own. Probably can be further clarified by splitting into two or three commits.
  • The early/dropStartingRoutes point should be an easy small commit of its own.
  • The androidNotificationUrlForMessage split makes another.
  • Then at least one other commit for the remaining changes (like setupNotificationDataForLaunch); not sure how many because I'm not sure what all the remaining changes are.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed the unnecessary refactors in the latest revision but kept the changes for androidNotificationUrlForMessage, openNotification and setupNotificationDataForLaunch in the test refactor commit (075bae8) as they were essential for simpler diff for these tests in the later commits.

});

group('parseIosApnsPayload', () {
test('smoke one-one DM', () {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, these tests are a good start.

Because of the nature of this function as parsing code (echoing #1379 (comment)), and because notifications are important, I'd like to have more thorough testing for it than only smoke tests.

But let's make that a follow-up issue we can pursue after launch. Even if there are edge cases this code doesn't do anything on, that won't be a regression from what's in main. (And if there are edge cases where it does something but it's the wrong thing, going to the wrong narrow is probably not much worse than doing nothing.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed: #1563

Comment on lines +142 to +145
switch (defaultTargetPlatform) {
case TargetPlatform.android:
final intentDataUrl = androidNotificationUrlForMessage(account, message);
unawaited(
WidgetsBinding.instance.handlePushRoute(intentDataUrl.toString()));
await tester.idle(); // let navigateForNotification find navigator

case TargetPlatform.iOS:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, I like this mechanism for reusing the same test code for both platforms.

Comment on lines +50 to +53
case TargetPlatform.iOS:
_notifDataFromLaunch = await _notifPigeonApi.getNotificationDataFromLaunch();
_notifPigeonApi.notificationTapEventsStream()
.listen(_navigateForNotification);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if the user opens the app from the home screen, and then immediately taps a notification, while the app is still just beginning to start up and so before it reaches this listen call?

In that case the notification won't get passed at startup to application(_:didFinishLaunchingWithOptions:). Instead it'll go to userNotificationCenter(_:didReceive:withCompletionHandler:). Will it make it to the app's Dart code so that the right conversation gets opened, or will it get dropped because there isn't a listener yet?

Looking at the Swift code, I think it will get dropped:

  override func onListen(withArguments arguments: Any?, sink: PigeonEventSink<NotificationTapEvent>) {
    eventSink = sink
  }

  func onNotificationTapEvent(payload: [AnyHashable : Any]) {
    eventSink?.success(NotificationTapEvent(payload: payload))
  }

I think the ideal behavior would be that instead it gets buffered, stored in the stream, and then when the app reaches this point it consumes any notification that's been buffered there.

If there isn't an obvious tweak to make that would provide that behavior, we can cheerfully leave it as a followup. I think this isn't a super common case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried the buffering implementation but weirdly I was observing some repeated navigations when tapped on a notification. So, I think I will leave this as a follow up, to investigate more about it later.

gnprice pushed a commit to gnprice/zulip-flutter that referenced this pull request May 30, 2025
chrisbobbe pushed a commit that referenced this pull request May 30, 2025
@chrisbobbe
Copy link
Collaborator

There's a breaking change coming from upstream that I think might affect this work: #1546 (comment)

rajveermalviya and others added 15 commits June 11, 2025 15:06
…art`

This makes it clear that these bindings are for Android only.
Also make use of a handy shorthand within `jq`.
And move the notification navigation data parsing utilities to
the new class.
To make it clear that they are Android specific.
Update it to receive `NotificationOpenPayload` as an argument
instead of the Android Notification URL.

Also rename `navigateForNotification` to
`navigateForAndroidNotificationUrl`, making it more explicit.
Introduces NotificationOpenPayload.parseIosApnsPayload which
can parse the payload that Apple push notification service
delivers to the app for displaying a notification. It retrieves
the navigation data for the specific message notification.
Introduces a new Pigeon API file, and adds the corresponding
bindings in Swift. Unlike the `pigeon/android_notifications.dart`
API this doesn't use the ZulipPlugin hack, as that is
only needed when we want the Pigeon functions to be available
inside a background isolate (see doc in `zulip_plugin/pubspec.yaml`).

Since the notification tap will trigger an app launch first
(if not running already) anyway, we can be sure that these new
functions won't be running on a Dart background isolate, thus not
needing the ZulipPlugin hack.
@rajveermalviya
Copy link
Member Author

Thanks for the review @gnprice! Pushed an update, PTAL.

@rajveermalviya rajveermalviya requested a review from gnprice June 11, 2025 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration review Added by maintainers when PR may be ready for integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ios notifs: Support tapping a notification to open the conversation
3 participants