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

rajveermalviya · 2025-02-26T16:36:09Z

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).

chrisbobbe · 2025-03-12T22:13:22Z

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 · 2025-03-13T22:14:53Z

(Rebased to main, Thanks!)

chrisbobbe

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.

chrisbobbe · 2025-03-18T21:09:38Z

lib/widgets/app.dart

-            GlobalLocalizations.zulipLocalizations = ZulipLocalizations.of(context);
-            return child!;
-          },
+    return DeferrredBuilderWidget(


nit: check spelling (here and in commit message)

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

chrisbobbe · 2025-03-18T21:22:34Z

lib/widgets/store.dart

-/// 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> {


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.

chrisbobbe · 2025-03-18T21:38:04Z

test/widgets/content_test.dart

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


We normally do this as testBinding.getGlobalStoreUniquely, right?

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

chrisbobbe · 2025-03-18T22:15:32Z

test/widgets/store_test.dart

+                store: store,
+                child: PerAccountStoreWidget(
+                  accountId: accountId,
+                  child: MyWidgetWithMixin(key: widgetWithMixinKey)));


Maybe we can use TestZulipApp for this test?

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

chrisbobbe · 2025-03-18T22:51:11Z

lib/notifications/open.dart

+        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;


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.

chrisbobbe · 2025-03-18T23:57:21Z

lib/widgets/app.dart

+  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.


chrisbobbe · 2025-03-19T00:09:59Z

test/notifications/open_test.dart

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


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?

chrisbobbe · 2025-03-19T00:25:50Z

lib/notifications/open.dart

+    final route = _routeForNotification(context, payload);
+    if (route == null) return; // TODO(log)
+
+    // TODO(nav): Better interact with existing nav stack on notif open


Maybe we should have an iOS counterpart to

android: When opening a notification, "back" should not leave the account's context #1210

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.

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.

chrisbobbe · 2025-03-19T00:26:07Z

pigeon/notifications.dart

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


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

chrisbobbe · 2025-03-19T00:28:01Z

test/notifications/open_test.dart

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


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

rajveermalviya · 2025-03-20T11:16:58Z

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

chrisbobbe

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.

chrisbobbe · 2025-03-24T23:59:47Z

ios/Runner/Notifications.g.swift

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


This needs an update for the Pigeon 25 upgrade e2aac35.

chrisbobbe · 2025-03-25T00:16:59Z

lib/widgets/app.dart

+  /// The widget to build when [future] completes, with it's result
+  /// passed as `result`.


chrisbobbe · 2025-03-25T20:28:55Z

android/app/src/main/kotlin/com/zulip/flutter/AndroidNotifications.g.kt

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


Commit-message nit:

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

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

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

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

chrisbobbe · 2025-03-25T23:31:26Z

pigeon/notifications.dart

+  /// 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


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.

chrisbobbe · 2025-03-26T21:33:48Z

test/model/binding.dart

+  FakeNotificationPigeonApi? _notificationPigeonApi;
+
+  @override
+  FakeNotificationPigeonApi get notificationPigeonApi {
+    return (_notificationPigeonApi ??= FakeNotificationPigeonApi());
+  }


nit: getter before private field: https://github.com/flutter/flutter/blob/master/docs/contributing/Style-guide-for-Flutter-repo.md#order-other-class-members-in-a-way-that-makes-sense

Also, can use fewer lines:

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

chrisbobbe · 2025-03-27T00:09:55Z

ios/Runner/AppDelegate.swift

+  func onNotificationTapEvent(data: NotificationPayloadForOpen) {
+    if let eventSink = eventSink {
+      eventSink.success(data)
+    }
+  }


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) } }

chrisbobbe · 2025-03-27T00:18:26Z

pigeon/notifications.dart

+@EventChannelApi()
+abstract class NotificationHostEvents {


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

chrisbobbe · 2025-03-27T00:28:53Z

lib/model/binding.dart

+// 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();


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.

chrisbobbe · 2025-03-27T00:36:42Z

lib/notifications/open.dart

+  /// 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 {


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.

chrisbobbe · 2025-03-27T00:41:57Z

test/notifications/open_test.dart

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


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

rajveermalviya · 2025-04-08T22:34:53Z

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

gnprice

Thanks for the revision!

Here's comments on just the first commit:
60577d6 app: Move initialization of GlobalStore from GlobalStoreWidget to ZulipApp

It's felt unsatisfying how that change concentrates more functionality on the omnibus widget ZulipApp, particularly as that causes it to get duplicated on TestZulipApp (which introduces the risk of divergence between test and live code). After the comment below about comments that refer to the store getting loaded, I noticed that it seemed like some of those comments were going to need to get more complicated, too, to explain the new situation.

So I went and tried implementing the solution I think I suggested when we looked at this task in a call a few weeks ago: have GlobalStoreWidget keep responsibility for loading the store, and add a parameter to have it await an additional future. I think that works out well. Just pushed back to the PR branch a revision with that change:
9426b82 store: Add "blocking future" option on GlobalStoreWidget

The new commit is nice and simple:

 lib/widgets/store.dart       | 11 ++++++++++
 test/widgets/store_test.dart | 54 ++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 63 insertions(+), 2 deletions(-)

vs.

 6 files changed, 158 insertions(+), 168 deletions(-)

The effect on the subsequent "Navigate when app launched from notification" commit is small:

    -@@ lib/widgets/app.dart: class _ZulipAppState extends State<ZulipApp> with WidgetsBindingObserver {
    -     WidgetsBinding.instance.addObserver(this);
    -     () async {
    -       final globalStore = await ZulipBinding.instance.getGlobalStoreUniquely();
    -+      final notifFuture = NotificationOpenManager.instance.initializationFuture;
    -+      if (notifFuture != null) await notifFuture;
    -       if (!mounted) return;
    -       setState(() {
    -         _globalStore = globalStore;
    +@@ lib/widgets/app.dart: class _ZulipAppState extends State<ZulipApp> with WidgetsBindingObserver {
    +   @override
    +   Widget build(BuildContext context) {
    +     return GlobalStoreWidget(
    ++      blockingFuture: NotificationOpenManager.instance.initializationFuture,
    +       child: Builder(builder: (context) {

gnprice · 2025-04-12T02:29:56Z

test/widgets/app_test.dart

+      globalStore = GlobalStoreWidget.of(navigator.context);
+    }));
+
+    // First, shows a loading page instead of child.


Suggested change

// First, shows a loading page instead of child.

// First, shows a loading page.

The "instead of child" no longer makes sense in the ZulipApp context.

gnprice · 2025-04-12T02:35:19Z

test/widgets/store_test.dart

-      Directionality(
-        textDirection: TextDirection.ltr,
-        child: GlobalStoreWidget(
-          // no PerAccountStoreWidget


This comment seems still relevant.

gnprice · 2025-04-12T02:50:29Z

lib/widgets/store.dart

@@ -15,15 +13,14 @@ import 'page.dart';
 ///  * [GlobalStoreWidget.of], to get access to the data.
 ///  * [PerAccountStoreWidget], for the user's data associated with a
 ///    particular Zulip account.
-class GlobalStoreWidget extends StatefulWidget {
-  const GlobalStoreWidget({
+class GlobalStoreWidget extends InheritedNotifier<GlobalStore> {


There are a few places where comments mention GlobalStoreWidget to refer to the work it does of loading the global store. See git grep -PC2 '\[GlobalStoreWidget\]', and in particular some of the matches in lib/model/binding.dart, test/model/binding.dart, test/widgets/test_app.dart. So those will need to be updated.

gnprice

And here's a re-review on the docs commit.

gnprice · 2025-04-12T03:36:51Z

docs/howto/push-notifications-ios-simulator.md

+The canned payloads were generated from Zulip Server 11.0-dev+git
+8fd04b0f, API Feature Level 377, in April 2025.


That's a version from this week, after you originally wrote this doc — did you confirm that it still produces the same output?

Yes, When I had tried this server version with the fresh dev environment, I had verified that it produced the same output, modulo the message content.

Great, thanks!

gnprice · 2025-04-12T03:39:38Z

docs/howto/push-notifications-ios-simulator.md

+these intermediate steps records how to get those payloads, which may
+be useful in future._
+
+Setup and run Zulip dev server following the [setup tutorial](https://zulip.readthedocs.io/en/latest/development/setup-recommended.html).


I think this isn't enough on its own — for getting one's phone to be able to talk to the dev server you'll want the instructions at https://github.com/zulip/zulip-mobile/blob/main/docs/howto/dev-server.md (per #1379 (comment)).

I had changed to that link because these instructions were specific to testing on iOS Simulator, and expectation was to do these intermediate (optional) steps on the Simulator too.

Changed back to this link, in case reader tries these steps on an actual device, or in your case where server is running on a different machine than the Mac.

Ah true, the doc does say it's about the simulator. And the key xcrun simctl push step at the end sure sounds like it'll only work on the simulator.

It's probably not real common to be running the dev server on a different machine from the Mac hosting the simulator. So it'd be reasonable to add a remark here saying that if you'll be running the dev server on the Mac that's also hosting the simulator, then those steps boil down to just the normal Zulip dev server instructions (and include a direct link to the latter).

gnprice · 2025-04-12T03:40:02Z

docs/howto/push-notifications-ios-simulator.md

+Apple's Push Notification service to show notifications on iOS
+Simulator.
+
+## 1.(Optional) Setup dev server


nit:

Suggested change

## 1.(Optional) Setup dev server

## 1. (Optional) Setup dev server

gnprice · 2025-04-12T03:45:18Z

docs/howto/push-notifications-ios-simulator.md

+[Follow the steps in this section to setup a development server, and
+register the development server to the production bouncer](https://github.com/zulip/zulip-mobile/blob/main/docs/howto/push-notifications.md#server).


Huh interesting. That is a bit puzzling, though — looking at the dev_settings.py in that commit, it seems like it'll end up trying to talk to the bouncer at http://push.zulipdev.com:9991 (exactly like the signup command was doing when I tried above), or at an even more unlikely URL like http://push.192.168.0.101:9991 when one sets EXTERNAL_HOST to enable one's phone to get to the server.

gnprice · 2025-04-12T03:47:27Z

docs/howto/push-notifications-ios-simulator.md

+
+## 3. (Optional) Login to the dev user on zulip-flutter.
+
+<!-- TODO(405) Guide to use the new devlogin page instead -->


Suggested change





That's the convention we use for TODO links to issues. See existing examples:

$ git grep -P 'TODO.\d+' $ git grep -P 'TODO.#\d+'

Keeping the convention consistent helps keep the references greppable.

gnprice · 2025-04-12T03:54:30Z

docs/howto/push-notifications-ios-simulator.md

+_If you skipped steps 2-5, you'll need pre-forged APNs payloads for
+existing messages in a default development server messages for the
+user `[email protected]`:_


bump — I'd like this information to not get lost for the next person.

Here's a version that could go in verbatim:

These canned payloads assume that EXTERNAL_HOST has its default value for the dev server. If you've set EXTERNAL_HOST to use an IP address in order to enable your device to connect to the dev server, you'll need to adjust the realm_url fields. You can do this by a find-and-replace for localhost; for example, perl -i -0pe s/localhost/10.0.2.2/g tmp/*.json after saving the canned payloads to files tmp/*.json.

rajveermalviya · 2025-04-14T17:15:42Z

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

gnprice · 2025-04-15T03:27:54Z

docs/howto/push-notifications-ios-simulator.md

+user `[email protected]`:_
+
+These canned payloads were generated from Zulip Server 11.0-dev+git
+8fd04b0f0, API Feature Level 377, in April 2025.


ah, thanks for adding the 9th digit here :-)

gnprice

OK, completed re-review of the things covered in the last pair of reviews. All looks good; one comment here above:
#1379 (comment)

Still on my plate in reviewing this are:

re-review the Swift code, revised since notif ios: Handle opening of conversation on tap; take 2 #1379 (review)
re-review the small early commits, revised since notif ios: Handle opening of conversation on tap; take 2 #1379 (review) (I believe little changed here and I didn't have much to say the first time, so should be quick)
retry the testing instructions on a fresh dev server, following notif ios: Handle opening of conversation on tap; take 2 #1379 (comment)
review the Dart changes in those two main commits (having covered the Swift changes earlier)

rajveermalviya · 2025-04-22T11:28:35Z

(Rebased to fix conflicts.)

…art` This makes it clear that these bindings are for Android only.

…orDialog And fix a typo.

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.

gnprice

OK, I finally came back and tested out these instructions after the debugging described in that chat thread (from #1379 (comment) ).

This version works great, with one correction below.

Then I also have one high-level comment on the structure (doesn't call for much new text, mostly just moving things around plus a bit of new section-intro text), and a couple of small comments.

My remaining review to-do list for this PR is the other three points in #1379 (review) .

gnprice · 2025-05-07T00:26:15Z

docs/howto/push-notifications-ios-simulator.md

+         user_profile, message, mentioned_user_group_id, mentioned_user_group_name, can_access_sender
+     )
+     logger.info("Sending push notifications to mobile clients for user %s", user_profile_id)
+    logger.info("APNS payload %s", orjson.dumps(apns_payload))


Suggested change

+ logger.info("APNS payload %s", orjson.dumps(apns_payload))

+ logger.info("APNS payload %s", orjson.dumps(apns_payload).decode())

That way it gets printed in the log as verbatim JSON. Otherwise the byte-string gets encoded the way it would be in Python source code, like b'…' — which means that if you try to copy-paste it to use directly, it can get mangled depending on the contents. For example if you send a message whose content is ", you get:
APNS payload: b'{"alert":{"title":"Desdemona","subtitle":"","body":"\\""},"sound":"default","badge":0,"custom":{"zulip":{"server":"192.168.0.113:9991","realm_id":2,"realm_uri":"http://192.168.0.113:9991","realm_url":"http://192.168.0.113:9991","realm_name":"Zulip Dev","user_id":11,"sender_id":9,"sender_email":"[email protected]","time":1746576414,"recipient_type":"private","message_ids":[104]}}}'

with "\\"" , which fails to parse as JSON.

gnprice · 2025-05-07T00:36:23Z

docs/howto/push-notifications-ios-simulator.md

+
+</details>
+
+To receive a notification on the iOS Simulator, we need to push


Now that I've gotten the full instructions working end to end for me and then scanned back through them, I think the structure of what the reader is intended to do here can be expressed more clearly with a bit of reordering:

First, a ## section consisting of the steps from here to the end.

This is the main thing the document is saying to do — everything else is optional, and/or only needs to be done occasionally before then doing this portion many times.

Then, a ## section with the three pre-canned payloads, i.e. the portion of Step 6 above this point.

Then, a third ## section describing how to produce new canned payloads. That consists of Steps 1–5, with each one adjusted to a ###-level section instead of ##-level so that it's a sub-section of this overall section.

Then the structure is: you do what's in the first section. To get input to use in that, you can use either the second section or the third section.

And then in particular the individual steps of the third section don't get labeled as optional; they're all required if you want to produce payloads. So this structure makes that relationship clearer, whereas the individual "optional" labels look as if they're saying one might pick and choose.

gnprice · 2025-05-07T00:36:38Z

docs/howto/push-notifications-ios-simulator.md

+Apple's Push Notification service to show notifications on iOS
+Simulator.
+
+## 1. (Optional) Setup dev server


Suggested change

## 1. (Optional) Setup dev server

## 1. (Optional) Set up dev server

"setup" is a noun, "set up" the verb

gnprice · 2025-05-07T00:39:32Z

docs/howto/push-notifications-ios-simulator.md

+And then follow the steps [here](https://zulip.com/help/mobile-notifications)
+to enable Mobile Notifications for "Channels".
+
+## 3. (Optional) Login to the dev user on zulip-flutter.


Suggested change

## 3. (Optional) Login to the dev user on zulip-flutter.

## 3. (Optional) Log in as the dev user on zulip-flutter.

Similarly "login" is a noun, "log in" the verb.

And as a smaller point: one logs in "to" a server, a realm, or even an account, but logs in "as" a user. The user isn't a space one is entering, but an identity one is taking on.

gnprice

Here's re-review on the Swift changes. The revisions look good; small comments below.

gnprice · 2025-05-07T00:49:59Z

ios/Runner/AppDelegate.swift

+      let listener = notificationTapEventListener!
+      let userInfo = response.notification.request.content.userInfo
+      listener.onNotificationTapEvent(payload: userInfo)


nit: bit simpler with listener inlined

gnprice · 2025-05-07T00:58:36Z

ios/Runner/AppDelegate.swift

+    // Setup handler for notification tap while the app is running.
+    UNUserNotificationCenter.current().delegate = self


nit: "setup" verb/noun

But actually I think this is a comment that can be omitted. In Xcode if you put your cursor on the delegate property that's getting set here, you get this bit of docs:
https://developer.apple.com/documentation/usernotifications/unusernotificationcenter/delegate

Use the delegate object to respond to user-selected actions and to process incoming notifications when your app is in the foreground.

and I think that makes the point just as well as the comment does.

gnprice · 2025-05-07T01:00:26Z

ios/Runner/AppDelegate.swift

+  }
+}
+
+class NotificationTapEventListener: NotificationTapEventsStreamHandler {


Let's include comments somewhere in here with links to the Flutter examples that you found helpful, from the thread #1379 (comment) , since those examples were nontrivial to find.

Here at the top of this class is probably a fine place.

gnprice

OK, and re-reviewed the small commits from early in the branch. Just a couple of small comments below.

With this and the reviews just above, the remaining items for me to review in this PR are, after #1379 (review), down to just:

review the Dart changes in those two main commits at the end of the branch (having covered the Swift changes above).

gnprice · 2025-05-07T01:12:31Z

lib/widgets/dialog.dart

+/// The context argument is used to look up [ZulipLocalizations],
+/// the [Navigator] and [Theme] for the dialog.


I'm not sure how to use this information, when writing or editing a call site of this function. What sort of context does this mean is acceptable or not acceptable?

I believe the main upshot is: this context should be a descendant of the app's main [Navigator]. (That's enough, because the [ZulipLocalizations] and [Theme] are provided by [MaterialApp] and go above the [Navigator].)

Does that cover it? Or are there situations where further guidance beyond that would be helpful?

Also the same information should probably go on showSuggestedActionDialog too, since that's so similar.

gnprice

OK, and started on those two main commits at the end.

I think in this round I've read the first of them:
37683db notif ios: Navigate when app launched from notification

except its tests.

gnprice · 2025-05-07T23:36:08Z

lib/notifications/open.dart

+  /// A [Future] that completes to signal that the initialization of
+  /// [NotificationOpenManager] has completed or errored.
+  Future<void>? get initializationFuture => _initializedSignal?.future;


What's the meaning when this is null?

gnprice · 2025-05-07T23:38:47Z

lib/widgets/app.dart

-          onGenerateInitialRoutes: _handleGenerateInitialRoutes);
+          // TODO migrate Android's handling to the new Pigeon API.
+          onGenerateInitialRoutes: defaultTargetPlatform == TargetPlatform.iOS
+            ? _handleGenerateInitialRoutesIos
+            : _handleGenerateInitialRoutes);


I think it would be somewhat cleaner to push this conditional down into the _handleGenerateInitialRoutes method. The method could consist of just checking this condition and calling one of two methods like _handleGenerateInitialRoutesAndroid and _handleGenerateInitialRoutesIos; pushing the conditional there would mean that this MaterialApp constructor call, which has lots of other things it's dealing with, doesn't get into that detail.

gnprice · 2025-05-07T23:43:30Z

lib/widgets/app.dart

@@ -202,6 +203,31 @@ class _ZulipAppState extends State<ZulipApp> with WidgetsBindingObserver {
    ];
  }

+  List<Route<dynamic>> _handleGenerateInitialRoutesIos(_) {


Re my comment just previous, it actually looks like the old and new methods would share most of their code — the start and end are the same, and only the middle differs. So I'd try making them one method with the conditional just in the middle.

The two sides of that conditional could still be in two separate methods if that seems cleanest — like:

final route = defaultTargetPlatform == TargetPlatform.iOS ? _initialRouteIos() : _initialRouteAndroid(initialRouteStr); if (route != null) { return [ HomePage.buildRoute(accountId: route.accountId), route, ]; }

gnprice · 2025-05-07T23:51:44Z

lib/notifications/open.dart

+/// Service for handling notification navigation.
+class NotificationOpenManager {


I think this doc comment is right — the term "service" fits for this, similar to NotificationService, and unlike NotificationDisplayManager. This is especially so with that listener for notification-tap events, which has this effectively standing ready to act.

So let's name this with "service" instead of "manager", and accordingly rename the init method to start.

gnprice · 2025-05-07T23:53:02Z

lib/notifications/open.dart

+  /// Returns null if app launch wasn't triggered by a notification, or if
+  /// an error occurs while determining the route for the notification, in
+  /// which case an error dialog is also shown.


Suggested change

/// Returns null if app launch wasn't triggered by a notification, or if

/// an error occurs while determining the route for the notification, in

/// which case an error dialog is also shown.

/// Returns null if app launch wasn't triggered by a notification, or if

/// an error occurs while determining the route for the notification.

/// In the latter case an error dialog is also shown.

Otherwise it's ambiguous whether the error dialog applies to both of these cases.

gnprice · 2025-05-07T23:54:24Z

lib/notifications/open.dart

+    unawaited(navigator.push(route));
+  }
+
+  NotificationNavigationData? _tryParsePayload(


nit: may as well have this as its own method when it's first introduced, instead of in the next commit

gnprice · 2025-05-07T23:56:29Z

lib/notifications/open.dart

+    required this.realmUrl,
+    required this.userId,
+    required this.narrow,
+  }) : assert(narrow is TopicNarrow || narrow is DmNarrow);


Perhaps make the type itself be SendableNarrow?

gnprice · 2025-05-07T23:58:09Z

lib/notifications/open.dart

+      final String realmUrl;
+      switch (zulipData) {
+        case {'realm_url': final String value}:
+          realmUrl = value;


This can be more compact as a switch-expression.

gnprice · 2025-05-08T00:03:27Z

lib/notifications/open.dart

+            allRecipientIds: pmUsers
+              .split(',')
+              .map((e) => int.parse(e, radix: 10))
+              .toList(growable: false)..sort(),


nit: the sort is a significant step, so best to keep it visible and not tuck it in:

Suggested change

.toList(growable: false)..sort(),

.toList(growable: false)

..sort(),

gnprice · 2025-05-08T00:13:16Z

lib/notifications/open.dart

+  /// Provides the route to open by parsing the notification payload.
+  ///
+  /// Returns null and shows an error dialog if the associated account is not
+  /// found in the global store.
+  AccountRoute<void>? _routeForNotification(BuildContext context, NotificationNavigationData data) {


It looks like the logic in this method all duplicates most of what's in NotificationDisplayManager.routeForNotification.

It'd be good to avoid duplicating that. Instead, can have a method with this logic (could live on either class), and have NotificationDisplayManager.routeForNotification call that.

This does take its input in a different form, as NotificationNavigationData instead of NotificationOpenPayload. The simplest way to share the logic, starting from this version, would be to have the method that has this logic just take the three fields as separate arguments — since those two classes have the exact same three fields.

It also feels like these two classes are really two ways of describing the same thing, though. How about making fromIosApnsPayload be a new constructor on the NotificationOpenPayload class? The doc comment on that class would change a bit:

/// The information contained in 'zulip://notification/…' internal /// Android intent data URL, used for notification-open flow. class NotificationOpenPayload {

but I think the real point remains the same: it's the data about a notification that describes what to do on opening it.

rajveermalviya force-pushed the dev-ios-notif-2 branch 5 times, most recently from 463b9ee to 8478226 Compare March 6, 2025 14:53

rajveermalviya marked this pull request as ready for review March 6, 2025 14:54

rajveermalviya force-pushed the dev-ios-notif-2 branch 2 times, most recently from 0142dbd to 89df63b Compare March 6, 2025 17:29

rajveermalviya mentioned this pull request Mar 6, 2025

notif ios: Handle opening of conversation on tap #1261

Closed

rajveermalviya force-pushed the dev-ios-notif-2 branch from 89df63b to 4f3224a Compare March 10, 2025 11:09

rajveermalviya added the maintainer review PR ready for review by Zulip maintainers label Mar 10, 2025

rajveermalviya assigned chrisbobbe Mar 10, 2025

rajveermalviya requested a review from chrisbobbe March 10, 2025 11:09

rajveermalviya force-pushed the dev-ios-notif-2 branch 2 times, most recently from 80a34eb to 9c07740 Compare March 10, 2025 18:37

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

chrisbobbe reviewed Mar 19, 2025

View reviewed changes

rajveermalviya force-pushed the dev-ios-notif-2 branch from 2178fe6 to 616defe Compare March 20, 2025 10:58

rajveermalviya requested a review from chrisbobbe March 20, 2025 11:17

gnprice mentioned this pull request Mar 25, 2025

notif: Ignore notifications for logged out accounts #1349

Merged

chrisbobbe reviewed Mar 27, 2025

View reviewed changes

rajveermalviya mentioned this pull request Mar 27, 2025

ios build: Take auto updates to Podfile.lock and an Xcode file #1444

Merged

gnprice mentioned this pull request Mar 27, 2025

Run iOS build in CI #773

Open

rajveermalviya force-pushed the dev-ios-notif-2 branch 2 times, most recently from 173f11f to 9813a4d Compare March 27, 2025 19:32

rajveermalviya force-pushed the dev-ios-notif-2 branch from 418b205 to b2bd272 Compare April 8, 2025 22:24

rajveermalviya requested a review from gnprice April 8, 2025 22:34

rajveermalviya force-pushed the dev-ios-notif-2 branch from b2bd272 to 994a641 Compare April 8, 2025 22:42

gnprice reviewed Apr 12, 2025

View reviewed changes

gnprice force-pushed the dev-ios-notif-2 branch from 994a641 to 31a44a6 Compare April 12, 2025 03:29

gnprice reviewed Apr 12, 2025

View reviewed changes

rajveermalviya force-pushed the dev-ios-notif-2 branch 2 times, most recently from d7e8e77 to 40864b1 Compare April 14, 2025 16:58

rajveermalviya requested a review from gnprice April 14, 2025 17:15

gnprice reviewed Apr 15, 2025

View reviewed changes

rajveermalviya force-pushed the dev-ios-notif-2 branch from 40864b1 to 07290de Compare April 15, 2025 11:27

rajveermalviya requested a review from gnprice April 15, 2025 11:28

rajveermalviya force-pushed the dev-ios-notif-2 branch from 07290de to c4cad8f Compare April 22, 2025 11:28

rajveermalviya and others added 8 commits April 22, 2025 17:26

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

e5d8f22

…art` This makes it clear that these bindings are for Android only.

dialog [nfc]: Document required ancestors for BuildContext in showErr…

b942a61

…orDialog And fix a typo.

notif: Fix error message when account not found in store

94813d5

binding test [nfc]: Reorder androidNotificationHost getter

c6f0423

docs: Document testing push notifications on iOS Simulator

aaf01e9

store: Add "blocking future" option on GlobalStoreWidget

9d3e500

notif ios: Navigate when app running but in background

7ba7cc4

rajveermalviya force-pushed the dev-ios-notif-2 branch from c4cad8f to 7ba7cc4 Compare April 22, 2025 11:59

gnprice reviewed May 7, 2025

View reviewed changes

gnprice reviewed May 8, 2025

View reviewed changes

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

		/// The widget to build when [future] completes, with it's result
		/// passed as `result`.

	// First, shows a loading page instead of child.
	// First, shows a loading page.

		The canned payloads were generated from Zulip Server 11.0-dev+git
		8fd04b0f, API Feature Level 377, in April 2025.

	## 1.(Optional) Setup dev server
	## 1. (Optional) Setup dev server

		[Follow the steps in this section to setup a development server, and
		register the development server to the production bouncer](https://github.com/zulip/zulip-mobile/blob/main/docs/howto/push-notifications.md#server).


		## 3. (Optional) Login to the dev user on zulip-flutter.

		<!-- TODO(405) Guide to use the new devlogin page instead -->

	<!-- TODO(405) Guide to use the new devlogin page instead -->
	<!-- TODO(#405) Guide to use the new devlogin page instead -->

	+ logger.info("APNS payload %s", orjson.dumps(apns_payload))
	+ logger.info("APNS payload %s", orjson.dumps(apns_payload).decode())


		</details>

		To receive a notification on the iOS Simulator, we need to push

		@EventChannelApi()
		abstract class NotificationHostEvents {

	## 1. (Optional) Setup dev server
	## 1. (Optional) Set up dev server

	## 3. (Optional) Login to the dev user on zulip-flutter.
	## 3. (Optional) Log in as the dev user on zulip-flutter.

		// Setup handler for notification tap while the app is running.
		UNUserNotificationCenter.current().delegate = self

		/// The context argument is used to look up [ZulipLocalizations],
		/// the [Navigator] and [Theme] for the dialog.

		/// Service for handling notification navigation.
		class NotificationOpenManager {

	.toList(growable: false)..sort(),
	.toList(growable: false)
	..sort(),

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

Are you sure you want to change the base?

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

Conversation

rajveermalviya commented Feb 26, 2025 • edited by gnprice Loading

chrisbobbe commented Mar 12, 2025

rajveermalviya commented Mar 13, 2025

chrisbobbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajveermalviya commented Mar 20, 2025

chrisbobbe left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Mar 27, 2025 • edited Loading

Choose a reason for hiding this comment

rajveermalviya commented Apr 8, 2025

gnprice left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajveermalviya commented Apr 14, 2025

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

rajveermalviya commented Apr 22, 2025

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajveermalviya commented Feb 26, 2025 •

edited by gnprice

Loading

chrisbobbe left a comment •

edited

Loading

chrisbobbe Mar 27, 2025 •

edited

Loading

gnprice left a comment •

edited

Loading