notif: On Android handle notification taps via Pigeon API by rajveermalviya · Pull Request #2043 · zulip/zulip-flutter

rajveermalviya · 2025-12-18T14:36:42Z

Instead of relying on Flutter's deeplinks implementation for routing the notification URL, handle the Android Intents generated by notification taps ourselves using Pigeon to pass those events over to the Dart layer from the Java layer.

The upstream Flutter's deeplinks implementation has a bug where if the deeplink is triggered after the app was killed by the OS when it was in background, the app will get launched again but the route/link will not reach the Flutter's navigation handlers. See: flutter/flutter#178305

In the failure case we seem to be receiving the Android Intent for the notification tap from the OS via MainActivity.onNewIntent without any problems. So, to workaround that upstream bug this commit changes the implementation to handle these Android Intents ourselves.

Fixes: #1567

chrisbobbe

Thanks! I'm not super familiar with this code. I think the main thing I'd like to understand is why so much in the ios/ directory is touched here, when the issue and PR description are pretty explicit that this is an Android bugfix. (There's even a name with "android" that appears in a file in ios/, which I've pointed out below; I'm confused about that.)

chrisbobbe · 2025-12-19T20:58:37Z

test/model/binding.dart

+  late final _notificationTapEventsStreamController =
+    StreamController<NotificationTapEvent>();


Is late final intended, instead of just final?

chrisbobbe · 2025-12-19T21:03:47Z

ios/Runner/Notifications.g.swift

+}
+
+/// Generated class from Pigeon that represents data sent in messages.
+struct AndroidNotificationTapEvent: NotificationTapEvent {


Why is the name AndroidNotificationTapEvent appearing in a file in ios/? That looks like a smell to me; can it be avoided?

This is because of a limitation in Pigeon, AndroidNotificationTapEvent is canonically defined in pigeon/notification.dart and every other class definitions with the same name in *.g.{dart,swift,kt} is generated by Pigeon. And there doesn't seem to be a convenient annotation to use to indicate a platform-specific type.

AIUI only way to conditionally generate these types would be to have separate files for Android an iOS in pigeon/, and @ConfigurePigeon(PigeonOptions(…)) in each of those would specify paths only for their corresponding platforms. Another complexity is that NotificationTapEvent is a sealed class, and I am not sure how well Pigeon's generator supports multi-file Dart library.

Do note that these types (AndroidNotificationTapEvent in Notification.g.swift, and IosNotificationTapEvent in Notifications.g.kt) remain unused, so they probably get tree-shaken out during the build.

Also I've pushed 4b1198b which separates the renaming of NotificationTapEvent -> IosNotificationTapEvent in a nfc commit, for easier review of the final commit.

rajveermalviya · 2025-12-29T18:44:42Z

Thanks for the review @chrisbobbe! Pushed an update, PTAL. Also see #2043 (comment) for the reason why there are changes being made in ios/ directory.

rajveermalviya · 2026-01-07T17:00:20Z

(Rebased to fix conflicts with #2059.)

chrisbobbe

Thanks! Glad to be fixing this bug; comments below.

chrisbobbe · 2026-01-15T21:48:18Z

lib/notifications/receive.dart

  ValueNotifier<String?> token = ValueNotifier(null);

  Future<void> start() async {
+    await NotificationOpenService.instance.start();


notif: Move NotificationOpenService init at the start of NotificationService init

In the commit-message summary line, do you mean s/at/to/?

It sounds like this commit is NFC. Can it be marked as such?

notif: Move NotificationOpenService init to the start of NotificationService init NotificationOpenService.instance.start already handles all the platforms itself, by doing nothing on all except iOS, so move it's initialization out of the platform-specific switch here.

chrisbobbe · 2026-01-15T21:55:43Z

lib/notifications/open.dart

-}
+}


nit: keep newline at end of file

chrisbobbe · 2026-01-15T21:57:36Z

pigeon/notifications.dart

+/// An event that is only emitted on iOS platform when a notification is
+/// tapped on.


nit: can make one line:

/// On iOS, an event emitted when a notification is tapped.

chrisbobbe · 2026-01-15T22:18:20Z

pigeon/notifications.dart

+/// tapped on.
+///
+/// See [notificationTapEvents].
+class IosNotificationTapEvent extends NotificationTapEvent {


notif [nfc]: Rename NotificationTapEvent to IosNotificationTapEvent Also make NotificationTapEvent a base sealed class, because we will soon introduce another variant for Android too.

This is really a refactor, not a rename: now there are two classes, where there was just one, and they have a certain relationship with each other. Could you look back through this commit to make sure all the code that refers to each class respects the meaning you've assigned to it?

For example, I see a dartdoc below that doesn't respect the new meaning of NotificationTapEvent—

@EventChannelApi() abstract class NotificationEventChannelApi { /// An event stream that emits a notification payload when the app /// encounters a notification tap, while the app is running. /// /// Emits an event when /// `userNotificationCenter(_:didReceive:withCompletionHandler:)` gets /// called, indicating that the user has tapped on a notification. The /// emitted payload will be the raw APNs data dictionary from the /// `UNNotificationResponse` passed to that method. NotificationTapEvent notificationTapEvents(); }

—because it assumes the context is iOS (by referring to iOS APIs) despite NotificationTapEvent now being explicitly not specific to iOS (because it has an Ios… subclass). Here, I think maybe the fix is to introduce the paragraph with "On iOS…", and have a parallel paragraph for Android saying that it's unimplemented / does nothing for now, but will soon.

chrisbobbe · 2026-01-15T22:25:20Z

pigeon/notifications.dart

  /// An event stream that emits a notification payload when the app
-  /// encounters a notification tap, while the app is running.
+  /// encounters a notification tap, on iOS and Android while the app is
+  /// running, or only on Android when apps was launched by tapping a
+  /// notification.


Four lines is quite long for a dartdoc heading. How about:

/// An event stream that emits a notification payload /// when a notification is tapped.

and factor the rest into the "On iOS" and "On Android" paragraphs.

chrisbobbe · 2026-01-15T23:51:53Z

android/app/src/main/kotlin/com/zulip/flutter/notifications/NotificationTapEventListener.kt

+   *
+   * Do not call if `intent.action` is not ACTION_VIEW.
+   */
+  fun maybeHandleViewNotif(intent: Intent): Boolean {


I don't love this method's name. An ACTION_VIEW intent is the thing this method will receive and decide whether to handle specially—I see that in the dartdoc and the assert—not a "view notif". In fact, "view notif" isn't really a coherent name for anything.

chrisbobbe · 2026-01-15T23:57:59Z

lib/notifications/open.dart

-        case TargetPlatform.android:
-          // Do nothing; we do notification routing differently on Android.
-          // TODO migrate Android to use the new Pigeon API.
-          break;


I don't love having a defaultTargetPlatform == TargetPlatform.iOS condition as the first thing that runs in a case TargetPlatform.android.

I think it would be appropriate here for the .listen call to appear twice in the code (once in the iOS case, after .getNotificationDataFromLaunch(), once in the Android case). From reading your implementation comment here, it sounds like the stream effectively has different meanings between the two platforms, right? It's probably helpful to give .listen a dartdoc that documents those different meanings.

Added d914ce1 and a797865, which should point to correct docs now.

chrisbobbe · 2026-01-16T00:03:00Z

lib/notifications/open.dart

+  /// the given [AndroidNotificationTapEvent] which carries
+  /// `zulip://notification/…` Android intent data URL.


Suggested change

/// the given [AndroidNotificationTapEvent] which carries

/// `zulip://notification/…` Android intent data URL.

/// the given [AndroidNotificationTapEvent] which carries a

/// `zulip://notification/…` Android intent data URL.

chrisbobbe · 2026-01-16T00:25:13Z

lib/notifications/open.dart

  /// The URL should have been generated with
  /// [NotificationOpenPayload.buildAndroidNotificationUrl]
  /// when creating the notification.


Can we delegate to AndroidNotificationTapEvent the job of saying what dataUrl is? (I think just by referring to the relevant methods of NotificationOpenPayload in the field's dartdoc?)

Then this method's dartdoc can be…deleted, actually:

the "Navigate appropriately" is clear from the context the method is called in, and from its name

the details of what the input means and looks like are all provided in the param's type, AndroidNotificationTapEvent.

chrisbobbe · 2026-01-16T00:25:35Z

pigeon/notifications.dart

+/// An event that is only emitted on Android platform when a notification is
+/// tapped on.


(Similarly, this can be shortened to one line.)

rajveermalviya · 2026-01-20T22:04:21Z

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

CI failure is unrelated, it is failing because of dart-lang/sdk@19b06b5 in newer than checked-in version of Flutter SDK and #2086 should fix it.

chrisbobbe · 2026-02-06T00:55:18Z

Thanks! LGTM; marking for Greg's review.

gnprice

Thanks @rajveermalviya for diagnosing and fixing this, and @chrisbobbe for the previous reviews! Comments below.

gnprice · 2026-02-09T21:51:34Z

lib/notifications/receive.dart

  ValueNotifier<String?> token = ValueNotifier(null);

  Future<void> start() async {
+    await NotificationOpenService.instance.start();


It sounds like this commit is NFC. Can it be marked as such?

notif: Move NotificationOpenService init to the start of NotificationService init NotificationOpenService.instance.start already handles all the platforms itself, by doing nothing on all except iOS, so move it's initialization out of the platform-specific switch here.

gnprice · 2026-02-09T21:53:50Z

pigeon/notifications.dart

 abstract class NotificationEventChannelApi {
-  /// An event stream that emits a notification payload when the app


nit:

pigeon [nfc]: Move event channel method dartdoc to it's class

s/it's/its/

The form "it's", with an apostrophe, is a contraction of "it is". So it only appears where you could have said "it is" and had the same meaning.

gnprice · 2026-02-09T22:03:16Z

android/app/src/main/kotlin/com/zulip/flutter/notifications/NotificationTapEventListener.kt

+  }
+
+  /**
+   * Recognize if the ACTION_VIEW intent came from tapping a notification; handle it if so


nit:

Suggested change

* Recognize if the ACTION_VIEW intent came from tapping a notification; handle it if so

* Recognize if the ACTION_VIEW intent came from tapping a notification; handle it if so.

gnprice · 2026-02-09T22:07:29Z

android/app/src/main/kotlin/com/zulip/flutter/notifications/NotificationTapEventListener.kt

+  private var eventSink: PigeonEventSink<NotificationTapEvent>? = null
+  private val buffer = mutableListOf<NotificationTapEvent>()
+
+  override fun onListen(p0: Any?, sink: PigeonEventSink<NotificationTapEvent>) {


Probably it'd be good to fully implement the protocol, even though we don't currently intend to use this feature: override onCancel too, and have it remove the sink.

That way there isn't a latent bug waiting for us in case we someday do start using that feature of the protocol.

gnprice · 2026-02-09T22:10:11Z

lib/notifications/open.dart

+          _notifPigeonApi.notificationTapEventsStream()
+            .listen(_navigateForNotification);


Hmm, does this risk being out of order? It looks like we end up calling this before NotificationDisplayManager.init().

AIUI, there isn't anything in NotificationOpenService that depends on NotificationDisplayManager being initialized first. So, the order shouldn't matter.

Mmm, I think I was confusing this _navigateForNotification with something that would cause a notification itself to be received and attempt to display it. But that's NotificationService / receive.dart, not this. So yeah, this is fine.

gnprice · 2026-02-09T22:59:08Z

lib/widgets/app.dart

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


It looks like this will mean that when launching the app on Android through a notification, we'll now first render a frame with the last visited account and its home page (as a loading spinner), then switch accounts if needed and then push the conversation page on top. Whereas previously, and on iOS still, we'd go straight to the intended conversation from the first frame.

I guess that's probably OK — certainly it's better than the bug this fixes — but it seems like it's probably a visible small glitch. What have you seen when manually testing this? Would you post a pair of before/after screen recordings showing how this behavior looks?

Here are the recordings on my device in release mode, the navigation is not really noticeable:

Before Before slomo After After slomo

before.mp4

before-slomo-export.mp4

after.mp4

after-slomo-export.mp4

Thanks! I agree, that seems acceptable.

In the "After" video, I completely missed the glitch the first time I watched. On watching a second time, I spotted it but it was pretty quick. In the "After slomo" video, the glitch is easy to see, but I think it's an acceptable glitch.

Let's leave a short comment in the relevant code here, though (maybe two or three lines) mentioning that there is this glitch. That will help us think about our different options if at some point in the future we're again revising how this works.

gnprice · 2026-02-11T21:40:55Z

Thanks for the revision!

Pushed a version that adds the comment I mentioned above (it went a bit longer than I expected):

+    } else {
+      // On Android, we ignore any notification at this step, and handle
+      // any initial notification by a navigation after the first frame.
+      // See [NotificationOpenService.start], and the buffering in
+      // NotificationTapEventListener.kt when onListen is not yet called.
+      //
+      // The navigation causes a small visible glitch where one loading spinner
+      // gets replaced by another; see recordings:
+      //   https://github.com/zulip/zulip-flutter/pull/2043#discussion_r2794138972
+      // TODO it'd be nice to avoid that glitch by controlling the initial route.
+      //   We accept this glitch as a workaround for an upstream issue:
+      //   https://github.com/flutter/flutter/issues/178305
     }

CI is failing. I think that's probably just the issue discussed at #2129, but I'm not sure, so I'll hold off on merging for now. (Hopefully that issue will be resolved within the next day or two.)

rajveermalviya · 2026-02-13T18:22:32Z

(Rebased to main to fix CI)

gnprice · 2026-02-13T23:26:29Z

Great, and CI has passed. Merging.

…cationService init NotificationOpenService.instance.start already handles all the platforms itself, by doing nothing on all except iOS, so move it's initialization out of the platform-specific switch here.

…cription Initialize StreamController early, this will allow scheduling mock notification tap events (via `addNotificationTapEvent`) even before `notificationTapEventsStream` is called.

The event channel API declaration in Pigeon are quite weird because it's declared as a method on an abstract class but the generated method in `*.g.dart` is a global function. AIUI, the declaration in the Pigeon file cannot be a global function, because then it will need an implementation and thus the abstract class "hack". But not sure why the generated code for it is a global function as opposed to a static method on a abstract class. Anyway, this change fixes the missing dartdoc on the generated files. My guess is that each event channel API declarations are supposed to be one abstract class + one method pair, but there doesn't seem to be any error when a new method is introduced on the same abstract class.

…cationTapEvent This change refactors NotificationTapEvent to be a base sealed class. This will be helpful because we will soon introduce another variant for Android.

Instead of relying on Flutter's deeplinks implementation for routing the notification URL, handle the Android Intents generated by notification taps ourselves using Pigeon to pass those events over to the Dart layer from the Java layer. The upstream Flutter's deeplinks implementation has a bug where if the deeplink is triggered after the app was killed by the OS when it was in background, the app will get launched again but the route/link will not reach the Flutter's navigation handlers. See: flutter/flutter#178305 In the failure case we seem to be receiving the Android Intent for the notification tap from the OS via `MainActivity.onNewIntent` without any problems. So, to workaround that upstream bug this commit changes the implementation to handle these Android Intents ourselves. Fixes: zulip#1567

rajveermalviya added the maintainer review PR ready for review by Zulip maintainers label Dec 18, 2025

rajveermalviya assigned chrisbobbe Dec 18, 2025

rajveermalviya requested a review from chrisbobbe December 18, 2025 14:37

rajveermalviya force-pushed the pr-android-notif-intents branch 2 times, most recently from 2e2b67b to 3521955 Compare December 18, 2025 18:00

chrisbobbe reviewed Dec 19, 2025

View reviewed changes

rajveermalviya force-pushed the pr-android-notif-intents branch from 3521955 to b900a61 Compare December 29, 2025 17:31

rajveermalviya requested a review from chrisbobbe December 29, 2025 18:44

chrisbobbe mentioned this pull request Jan 6, 2026

notif android: Migrate to cross-platform Pigeon API for navigation #1447

Closed

rajveermalviya mentioned this pull request Jan 6, 2026

Dedupe message-list pages on opening a notification #2059

Merged

rajveermalviya force-pushed the pr-android-notif-intents branch from b900a61 to c791db0 Compare January 7, 2026 16:43

chrisbobbe reviewed Jan 16, 2026

View reviewed changes

rajveermalviya force-pushed the pr-android-notif-intents branch from c791db0 to d8cf3ef Compare January 20, 2026 19:47

rajveermalviya requested a review from chrisbobbe January 20, 2026 22:04

gnprice mentioned this pull request Jan 23, 2026

Analyzer error: invalid @override on factory constructors in submessage.dart #2082

Closed

rajveermalviya force-pushed the pr-android-notif-intents branch from d8cf3ef to d02e89c Compare January 23, 2026 16:33

chrisbobbe requested a review from gnprice February 6, 2026 00:55

chrisbobbe assigned gnprice and unassigned chrisbobbe Feb 6, 2026

chrisbobbe added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Feb 6, 2026

gnprice reviewed Feb 9, 2026

View reviewed changes

rajveermalviya force-pushed the pr-android-notif-intents branch from d02e89c to d401311 Compare February 11, 2026 18:03

gnprice force-pushed the pr-android-notif-intents branch from d401311 to 7d8ea18 Compare February 11, 2026 21:40

rajveermalviya force-pushed the pr-android-notif-intents branch from 7d8ea18 to 06407a3 Compare February 13, 2026 12:42

rajveermalviya added 6 commits February 13, 2026 15:26

notif [nfc]: Move NotificationOpenService init to the start of Notifi…

86f3f98

…cationService init NotificationOpenService.instance.start already handles all the platforms itself, by doing nothing on all except iOS, so move it's initialization out of the platform-specific switch here.

notif test: Allow scheduling mock notification tap events before subs…

3b399c1

…cription Initialize StreamController early, this will allow scheduling mock notification tap events (via `addNotificationTapEvent`) even before `notificationTapEventsStream` is called.

notif [nfc]: Add a bit of dartdoc on notificationTapEventsStream wrapper

cb9d8ed

notif [nfc]: Introduce IosNotificationTapEvent as a variant of Notifi…

5a5db3c

…cationTapEvent This change refactors NotificationTapEvent to be a base sealed class. This will be helpful because we will soon introduce another variant for Android.

gnprice force-pushed the pr-android-notif-intents branch from 06407a3 to 44042f5 Compare February 13, 2026 23:26

gnprice merged commit 44042f5 into zulip:main Feb 13, 2026

		late final _notificationTapEventsStreamController =
		StreamController<NotificationTapEvent>();

		/// An event that is only emitted on iOS platform when a notification is
		/// tapped on.

		/// the given [AndroidNotificationTapEvent] which carries
		/// `zulip://notification/…` Android intent data URL.

		/// An event that is only emitted on Android platform when a notification is
		/// tapped on.

		abstract class NotificationEventChannelApi {
		/// An event stream that emits a notification payload when the app

	* Recognize if the ACTION_VIEW intent came from tapping a notification; handle it if so
	* Recognize if the ACTION_VIEW intent came from tapping a notification; handle it if so.

		_notifPigeonApi.notificationTapEventsStream()
		.listen(_navigateForNotification);

Conversation

rajveermalviya commented Dec 18, 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!

rajveermalviya Dec 29, 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!

Choose a reason for hiding this comment

Uh oh!

rajveermalviya commented Dec 29, 2025

Uh oh!

rajveermalviya commented Jan 7, 2026

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!

rajveermalviya commented Jan 20, 2026

Uh oh!

chrisbobbe commented Feb 6, 2026

Uh oh!

gnprice 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!

gnprice commented Feb 11, 2026

Uh oh!

rajveermalviya commented Feb 13, 2026

Uh oh!

gnprice commented Feb 13, 2026

Uh oh!

rajveermalviya Dec 29, 2025 •

edited

Loading