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-05-20T14:38:04Z

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

gnprice

Thanks for splitting up that commit! That was very helpful for my reading those changes. Here's a full review of the resulting two commits:
80a89e3 notif [nfc]: Introduce NotificationNavigationServer
b8cfff7 notif ios: Navigate when app launched from notification

except the tests.

I'll take a look at those tests next. Then the remaining to-do list for review will be the final commit:
1c03950 notif ios: Navigate when app running but in background

gnprice · 2025-05-21T01:22:20Z

lib/notifications/navigate.dart

+  /// The context argument is used to look up the [Navigator], which is used
+  /// to show an error dialog if there is a failure.
+  AccountRoute<void>? routeForNotificationFromLaunch({required BuildContext context}) {


similar to #1379 (comment)

gnprice · 2025-05-21T01:36:48Z

lib/widgets/app.dart

+    } else {
+      // The account didn't match any existing accounts,
+      // fall through to show the default route below.


This comment doesn't seem accurate. This condition will indeed occur if the data for the initial route specified an account which didn't match any existing accounts… but it will also occur if the data for the initial route didn't specify any account at all.

In particular I believe this condition will occur every time the app is launched other than by opening a notification.

gnprice · 2025-05-21T01:37:27Z

lib/widgets/app.dart

          onGenerateRoute: (_) => null,
-
          onGenerateInitialRoutes: _handleGenerateInitialRoutes);


nit: stray diff hunk

gnprice · 2025-05-21T01:45:49Z

lib/notifications/navigate.dart

+    try {
+      return NotificationNavigationData.parseAndroidNotificationUrl(url);
+    } on FormatException catch (e, st) {
+      assert(debugLog('$e\n$st'));
+      final zulipLocalizations = ZulipLocalizations.of(context);
+      showErrorDialog(context: context,
+        title: zulipLocalizations.errorNotificationOpenTitle);


notif [nfc]: Introduce NotificationNavigationServer And move the notification navigation data parsing utilities to the new class.

This change doesn't look like it's NFC (see definition). If the URL doesn't parse, the old code would throw (because it called parseAndroidNotificationUrl directly); this version will instead pop up an error dialog.

The dialog seems like a fine change. That change should go in a different commit, though (either before or after). The great value of the NFC concept is that knowing a given commit is supposed to be NFC can help make it a lot simpler to read: all the logic in the red - parts of the diff will match up, not necessarily textually but semantically, with logic in the green + parts of the same diff. The reader can therefore mentally cross off all the bits that match up, aiming to get to zero.

gnprice · 2025-05-21T01:49:10Z

lib/widgets/app.dart

-    final route = _initialRouteAndroid(context, initialRoute);
+    final route = defaultTargetPlatform == TargetPlatform.iOS
+        ? _initialRouteIos(context)
+        : _initialRouteAndroid(context, initialRoute);


I like this diff hunk — it expresses pretty crisply the fact that before this change, we didn't respond to an initial route on iOS (only on Android), and after this change, we do.

gnprice · 2025-05-21T02:03:57Z

lib/notifications/navigate.dart

+  /// A [Future] that completes to signal that the initialization of
+  /// [NotificationNavigationService] has completed or errored.
+  ///
+  /// Returns null if [start] wasn't called yet.
+  Future<void>? get initializationFuture => _initializedSignal?.future;


In Future terminology, "complete" includes both success and failure. See e.g. [Future.whenComplete].

Suggested change

/// A [Future] that completes to signal that the initialization of

/// [NotificationNavigationService] has completed or errored.

///

/// Returns null if [start] wasn't called yet.

Future<void>? get initializationFuture => _initializedSignal?.future;

/// A [Future] that completes to signal that the initialization of

/// [NotificationNavigationService] has completed

/// (with either success or failure).

///

/// Null if [start] hasn't been called.

Future<void>? get initializationFuture => _initializedSignal?.future;

(also includes nit: omit "returns" for describing getter; a getter impersonates a field, so it just "is" its value; and "wasn't" doesn't sound right to me tense-wise)

For the name, how about just initialized? It's common for things of type Future to have names that describe the future's result, letting the type speak for the fact that it's a future.

gnprice · 2025-05-21T03:09:00Z

lib/notifications/navigate.dart

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


(Re NotificationNavigationData, we'll probably want that to end up covering the more general "open" concept too: in particular we'll need the message ID from the notification, and that will naturally become another field on that class. That more general concept is reflected in the name of the existing NotificationOpenPayload, and in the dartdoc this version has which I supplied at #1379 (comment) above.)

gnprice · 2025-05-21T03:11:00Z

lib/notifications/navigate.dart

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


Can this be static? Seems similar to tryParseAndroidNotificationUrl just below it.

gnprice · 2025-05-21T03:13:01Z

lib/notifications/navigate.dart

+    assert(context.mounted);
+    if (!context.mounted) return; // TODO(linter): this is impossible as there's no actual async gap, but the use_build_context_synchronously lint doesn't see that
+
+    final notifNavData = _tryParseIosApnsPayload(context, event.payload);


Looks like this _navigateForNotification method should get an assert that the platform is iOS, to clarify why this line makes sense.

gnprice · 2025-05-21T03:14:28Z

lib/notifications/navigate.dart

+  /// Navigates to the [MessageListPage] of the specific conversation
+  /// for the provided payload that was attached while creating the
+  /// notification.
+  Future<void> _navigateForNotification(NotificationTapEvent event) async {


Can this method be static? Doesn't look like it's using any instance data.

gnprice

OK, and here's a review on the tests in that next-to-last commit:
b8cfff7 notif ios: Navigate when app launched from notification

I think some of the changes requested here will help make those tests easier to read and understand, so for this round I haven't yet read every line.

gnprice · 2025-05-21T03:20:13Z

test/model/store_test.dart

-      addTearDown(NotificationService.debugReset);
      testBinding.packageInfoResult = eg.packageInfo(packageName: 'com.zulip.flutter');
+      addTearDown(NotificationService.debugReset);
+      addTearDown(NotificationNavigationService.debugReset);
      await NotificationService.instance.start();


Hmm yeah, good thought reordering these while you're here editing them.

gnprice · 2025-05-21T03:22:35Z

test/notifications/display_test.dart

    addTearDown(NotificationService.debugReset);
+    addTearDown(NotificationNavigationService.debugReset);


It'd be good if we can avoid going and adding another detail like this that tests have to know about.

How about having NotificationService.debugReset call the new service's debugReset? Seems appropriate for the main service to take responsibility for resetting the new one, given that the main service is also the one place responsible for starting up the new service.

gnprice · 2025-05-21T03:25:15Z

test/notifications/navigate_test.dart

+
+  Future<void> init() async {
+    addTearDown(testBinding.reset);
+    testBinding.firebaseMessagingInitialToken = '012abc';


Is this line needed? I'd think this data wouldn't be involved in any of these tests.

Yes, otherwise we hit this assert here:

zulip-flutter/test/model/binding.dart

Lines 492 to 495 in a4c564b

assert(_initialToken != null,

'Tests that call [NotificationService.start], or otherwise cause'

' a call to `ZulipBinding.instance.firebaseMessaging.getToken`,'

' must set `testBinding.firebaseMessagingInitialToken` first.');

Because ZulipBinding.firebaseMessaging.getToken ends up being called in NotificationService.start.

gnprice · 2025-05-21T03:28:15Z

test/notifications/display_test.dart

-  group('NotificationOpenPayload', () {
+  group('NotificationNavigationData', () {


Since the code this is testing is getting moved between files, the tests should move to remain in a corresponding file.

gnprice · 2025-05-21T03:38:33Z

lib/notifications/navigate.dart

+
+  /// Parses the iOS APNs payload and retrieves the information
+  /// required for navigation.
+  factory NotificationNavigationData.fromIosApnsPayload(Map<Object?, Object?> payload) {


Hmm, does this function have tests? I don't see them. (I do see a variety of test cases for the logic downstream of here, which is good.)

This logic is fairly densely packed with fiddly details, like parsing logic tends to be, so it's logic that's valuable to have good tests for. And, again typically for parsing logic, it has a pretty crisp interface for both input and output, which makes it nicely amenable to writing plenty of unit tests. So let's go ahead and do that.

For a point of comparison, see the existing tests of NotificationOpenPayload in display_test.dart.

gnprice · 2025-05-21T03:41:28Z

test/notifications/navigate_test.dart

+      matchesNavigation(check(pushedRoutes).single, eg.selfAccount, message);
+    }, variant: const TargetPlatformVariant({TargetPlatform.iOS}));
+
+    testWidgets('(iOS) uses associated account as initial account; if initial route', (tester) async {


I don't understand what a ";" means here.

gnprice · 2025-05-21T03:48:40Z

test/notifications/navigate_test.dart

+      // Set up a value for `PlatformDispatcher.defaultRouteName` to return,
+      // for determining the initial route.
+      final message = eg.streamMessage();
+      final payload = messageApnsPayload(message, account: eg.selfAccount);
+      testBinding.notificationPigeonApi.setNotificationDataFromLaunch(
+        NotificationDataFromLaunch(payload: payload));


Is this comment accurate? I don't see anything about PlatformDispatcher.defaultRouteName here.

It looks like this test case is copied from an existing one in display_test.dart. But that one says:

// Set up a value for `PlatformDispatcher.defaultRouteName` to return, // for determining the initial route. final account = eg.selfAccount; final message = eg.streamMessage(); final data = messageFcmMessage(message, account: account); final intentDataUrl = NotificationNavigationData( /* … */).buildAndroidNotificationUrl(); addTearDown(tester.binding.platformDispatcher.clearDefaultRouteNameTestValue); tester.binding.platformDispatcher.defaultRouteNameTestValue = intentDataUrl.toString();

so PlatformDispatcher.defaultRouteName really is what it's about.

gnprice · 2025-05-21T03:53:48Z

test/notifications/display_test.dart

      final account = eg.selfAccount;
      final message = eg.streamMessage();
      final data = messageFcmMessage(message, account: account);
-      final intentDataUrl = NotificationOpenPayload(
+      final intentDataUrl = NotificationNavigationData(


Speaking of this test, it looks like it (and several of its neighbors) should also move to navigate_test.dart / open_test.dart as part of the code-moving commit — the code they're testing is part of what's getting moved.

(In particular the way these tests exercise the code they're testing is via ZulipApp — and the relevant call sites in ZulipApp switch from NotificationDisplayManager methods to NotificationNavigationService methods.)

As a bonus, those moves may allow you to outright move some of the helpers from this test file, rather than copy them as the current version does.

gnprice · 2025-05-21T03:56:58Z

lib/notifications/display.dart

-    final route = routeForNotification(context: context, url: url);
+    assert(url.scheme == 'zulip' && url.host == 'notification');
+    final payload =
+      NotificationNavigationService.tryParseAndroidNotificationUrl(context, url);


This navigateForNotification method seems like it should move too — it's logically closely related to the other methods like routeForNotification which are moving.

It's also quite similar to the _navigateForNotification method which the last commit introduces on the other class. It might be fine to have some code duplication between these two methods — but if we do that, it's definitely better if they're right next to each other, so that the parallelism is more visible.

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

And fix a typo.

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 · 2025-05-28T18:57:29Z

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

gnprice

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

gnprice · 2025-05-29T18:59:25Z

test/notifications/open_test.dart

+      if (!dropStartingRoutes) {
+        check(pushedRoutes).isEmpty();
+        return;
+      }
+      await tester.pump();
+      takeStartingRoutes(account: account);
+      check(pushedRoutes).isEmpty();
+    }
+
+    Uri androidNotificationUrlForMessage(Account account, Message message) {


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.

gnprice · 2025-05-29T19:11:19Z

test/notifications/open_test.dart

+    });
+
+    group('parseIosApnsPayload', () {
+      test('smoke one-one DM', () {


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

gnprice · 2025-05-29T19:19:35Z

test/notifications/open_test.dart

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


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

gnprice · 2025-05-29T19:37:46Z

lib/notifications/open.dart

+        case TargetPlatform.iOS:
+          _notifDataFromLaunch = await _notifPigeonApi.getNotificationDataFromLaunch();
+          _notifPigeonApi.notificationTapEventsStream()
+            .listen(_navigateForNotification);


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.

[greg: cherry-picked from zulip#1379]

[greg: cherry-picked from #1379]

chrisbobbe · 2025-06-04T19:53:44Z

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

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

gnprice reviewed May 21, 2025

View reviewed changes

rajveermalviya force-pushed the dev-ios-notif-2 branch from 1c03950 to 2c3c35d Compare May 28, 2025 17:11

rajveermalviya and others added 8 commits May 28, 2025 23:03

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

50610d9

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

dialog [nfc]: Document required ancestors for BuildContext

b16d034

And fix a typo.

notif: Fix error message when account not found in store

1b11847

binding test [nfc]: Reorder androidNotificationHost getter

d966179

docs: Document testing push notifications on iOS Simulator

8288b10

docs: Clarify and expand a few spots in the iOS simulator notif doc

8f81702

Also make use of a handy shorthand within `jq`.

store: Add "blocking future" option on GlobalStoreWidget

a4252fe

notif [nfc]: Move NotificationOpenPayload to a separate file

6e51473

rajveermalviya force-pushed the dev-ios-notif-2 branch 2 times, most recently from 7d58286 to 2e40fb0 Compare May 28, 2025 18:03

rajveermalviya added 8 commits May 28, 2025 23:38

notif [nfc]: Introduce NotificationOpenService

cd147ec

And move the notification navigation data parsing utilities to the new class.

notif [nfc]: Rename NotificationOpenPayload methods

5292653

To make it clear that they are Android specific.

notif [nfc]: Refactor NotificationOpenService.routeForNotification

d130717

Update it to receive `NotificationOpenPayload` as an argument instead of the Android Notification URL. Also rename `navigateForNotification` to `navigateForAndroidNotificationUrl`, making it more explicit.

notif: Show a dialog if received malformed Android Notification URL

b2e0ed0

notif test: Refactor tests for NotificationOpenService

f12f911

notif ios: Add parser for iOS APNs payload

cead8bb

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.

notif ios: Navigate when app running but in background

02506ba

rajveermalviya force-pushed the dev-ios-notif-2 branch from 2e40fb0 to 02506ba Compare May 28, 2025 18:28

rajveermalviya requested a review from gnprice May 28, 2025 18:57

gnprice reviewed May 29, 2025

View reviewed changes

gnprice pushed a commit to gnprice/zulip-flutter that referenced this pull request May 30, 2025

notif: Fix error message when account not found in store

c263ae7

[greg: cherry-picked from zulip#1379]

chrisbobbe pushed a commit that referenced this pull request May 30, 2025

notif: Fix error message when account not found in store

ca3ef63

[greg: cherry-picked from #1379]

chrisbobbe mentioned this pull request Jun 4, 2025

settings: Upgrade Flutter; migrate to new RadioGroup API #1546

Open

		@@ -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`.

		onGenerateRoute: (_) => null,

		onGenerateInitialRoutes: _handleGenerateInitialRoutes);

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

		addTearDown(NotificationService.debugReset);
		addTearDown(NotificationNavigationService.debugReset);

	assert(_initialToken != null,
	'Tests that call [NotificationService.start], or otherwise cause'
	' a call to `ZulipBinding.instance.firebaseMessaging.getToken`,'
	' must set `testBinding.firebaseMessagingInitialToken` first.');

		group('NotificationOpenPayload', () {
		group('NotificationNavigationData', () {

		@EventChannelApi()
		abstract class NotificationHostEvents {

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

Uh oh!

Conversation

rajveermalviya commented Feb 26, 2025 • edited by gnprice Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisbobbe commented Mar 12, 2025

Uh oh!

rajveermalviya commented Mar 13, 2025

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajveermalviya commented Mar 20, 2025

Uh oh!

chrisbobbe left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajveermalviya commented May 20, 2025

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

rajveermalviya commented Feb 26, 2025 •

edited by gnprice

Loading

chrisbobbe left a comment •

edited

Loading

chrisbobbe Mar 27, 2025 •

edited

Loading