notif: Use associated account as initial account; if opened from background #1240

rajveermalviya · 2024-12-30T20:35:10Z

Previously, when two accounts (Account-1 and Account-2) were logged in, the app always defaulted to showing the home page of Account-1 on launch. If the app was closed and the user opened a notification from Account-2, the navigation stack would be:
HomePage(Account-1) -> MessageListPage(Account-2)

This commit fixes that behaviour, now when a notification is opened while the app is closed, the home page will correspond to the account associated with the notification's conversation.

This addresses #1210 for background notifications.

chrisbobbe

Here's a quick review :) in particular I haven't yet tested this manually on the office Android device.

chrisbobbe · 2024-12-30T20:49:26Z

lib/widgets/app.dart

+      final navigator = await ZulipApp.navigator;
+      final context = navigator.context;
+      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


Interesting. For a moment, seeing the await, I thought this comment must be wrong. But in fact I see that context isn't _handleGenerateInitialRoutes's context param, it's the final context that shadows that param. How about giving this one a different name, like navigatorContext?

Also nit: the TODO(linter) comment is too long; let's put it above this line, and it probably needs to be split onto multiple lines too

chrisbobbe · 2024-12-30T20:50:38Z

lib/widgets/app.dart

+    // TODO(#524) choose initial account as last one used
+    final initialAccountId = globalStore.accounts.firstOrNull?.id;
+
+    return (String intialRoute) {


nit: spelling of "initial"

chrisbobbe · 2024-12-30T20:52:58Z

test/notifications/display_test.dart

@@ -1096,6 +1096,44 @@ void main() {
      takeStartingRoutes();
      matchesNavigation(check(pushedRoutes).single, account, message);
    });
+
+    testWidgets('uses associated account as intial account; if intial route', (tester) async {


nit: spelling of "initial"

chrisbobbe · 2024-12-30T20:56:00Z

test/notifications/display_test.dart

+        narrow: switch (data.recipient) {
+        FcmMessageChannelRecipient(:var streamId, :var topic) =>
+          TopicNarrow(streamId, topic),
+        FcmMessageDmRecipient(:var allRecipientIds) =>
+          DmNarrow(allRecipientIds: allRecipientIds, selfUserId: data.userId),
+      }).buildUrl();


nit: Indentation looks wrong here (code inside switch should be indented)

chrisbobbe · 2024-12-30T20:57:07Z

test/notifications/display_test.dart

+        FcmMessageDmRecipient(:var allRecipientIds) =>
+          DmNarrow(allRecipientIds: allRecipientIds, selfUserId: data.userId),
+      }).buildUrl();
+      tester.binding.platformDispatcher.defaultRouteNameTestValue = intentDataUrl.toString();


This looks like it needs a corresponding teardown, with clearDefaultRouteNameTestValue. (Same with the existing place we set this, actually)

rajveermalviya · 2024-12-30T21:45:21Z

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

chrisbobbe

Thanks! Small comments below, and all looks good in a quick manual test on an Android emulator.

Does this PR change behavior on iOS? We haven't implemented navigation on tapping a notification on iOS yet; that's #1147. I haven't tested on iOS because Apple makes it complicated to test client-side notification changes: https://github.com/zulip/zulip-mobile/blob/main/docs/howto/push-notifications.md#testing-client-side-changes-on-ios

chrisbobbe · 2024-12-30T21:51:28Z

test/notifications/display_test.dart

@@ -1070,6 +1070,7 @@ void main() {

    testWidgets('at app launch', (tester) async {
      addTearDown(testBinding.reset);
+      addTearDown(tester.binding.platformDispatcher.clearDefaultRouteNameTestValue);


This would be best in its own commit, since it's about an unrelated test.

Moved this and the formating change in this test to a separate commit.

chrisbobbe · 2024-12-30T22:03:19Z

lib/widgets/app.dart

+
+    final globalStore = GlobalStoreWidget.of(context);
+    // TODO(#524) choose initial account as last one used
+    final initialAccountId = globalStore.accounts.firstOrNull?.id;


How about doing this (checking the contents of globalStore.accounts) inside the returned InitialRouteListFactory callback? That's what we do in the open-from-notification case.

Then in particular we don't have to think about what happens if the firstOrNull account is logged out between a call to _ZulipAppState.build and the time when Flutter decides to call this InitialRouteListFactory callback.

—ah, I see: its position outside the callback is the same in main. How about a prep commit that just puts the query on globalStore.accounts into the callback, then?

Seems also nice if it's below the open-from-notification code in that callback, so it's easier to see that it's not part of that logic.

rajveermalviya · 2025-01-06T11:19:05Z

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

chrisbobbe

Thanks! Small comments below.

chrisbobbe · 2025-01-24T23:49:43Z

lib/widgets/app.dart

@@ -152,6 +152,21 @@ class _ZulipAppState extends State<ZulipApp> with WidgetsBindingObserver {
    return super.didPushRouteInformation(routeInformation);
  }

+  InitialRouteListFactory _handleGenerateInitialRoutes(BuildContext context) {


app [nfc]: Pull out _handleGenerateInitialRoutes

This commit message is misleading 🙂; the change doesn't follow the common pattern of just moving some code out to a helper function for better organization. Putting the globalStore.accounts query inside the callback, instead of outside, is significant and worth mentioning; in fact it might not be NFC. The reason I gave in #1240 (comment) was:

Then in particular we don't have to think about what happens if the firstOrNull account is logged out between a call to _ZulipAppState.build and the time when Flutter decides to call this InitialRouteListFactory callback.

Does that seem right, or am I missing something?

It would be even clearer if it were split into two commits: one that changes when globalStore.accounts is queried, and one for making the helper function (the pure refactor).

chrisbobbe · 2025-01-25T00:02:43Z

lib/notifications/display.dart

+    final payload = NotificationOpenPayload.parseUrl(url);
+
+    final account = globalStore.accounts.firstWhereOrNull((account) =>
+      account.realmUrl == payload.realmUrl && account.userId == payload.userId);


How likely do we think it is that these realm URLs could differ in boring ways that shouldn't matter? Like https://chat.zulip.org/ vs. ``https://chat.zulip.org`. Do we need to just compare .origin instead, like we do for most (all?) other realm-URL checks?

Right, I've added a commit that changes this query to compare only using .origin.

rajveermalviya · 2025-01-28T15:01:36Z

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

chrisbobbe · 2025-01-29T01:45:38Z

Thanks, LGTM! Marking for Greg's review.

gnprice

Thanks both! This approach looks good. Some comments below.

gnprice · 2025-01-30T22:19:47Z

test/notifications/display_test.dart

@@ -1103,6 +1118,7 @@ void main() {

    testWidgets('at app launch', (tester) async {
      addTearDown(testBinding.reset);
+      addTearDown(tester.binding.platformDispatcher.clearDefaultRouteNameTestValue);


nit:

notif test [nfc]: Cleanup `TestPlatformDispatcher.defaultRouteNameTestValue`

"cleanup" is a noun; "clean up" is the verb, or verb phrase. More at: #958 (comment)

Separately: this isn't NFC, rather it's fixing a (latent) bug in this test: it would leak some modified state, which could cause a later test to fail in a puzzling way.

(It's fine that the same commit also includes the NFC formatting fix in the nearby code, though.)

gnprice · 2025-01-30T22:22:28Z

test/notifications/display_test.dart

+            TopicNarrow(streamId, topic),
+          FcmMessageDmRecipient(:var allRecipientIds) =>
+            DmNarrow(allRecipientIds: allRecipientIds, selfUserId: data.userId),
+        }).buildUrl();
      tester.binding.platformDispatcher.defaultRouteNameTestValue = intentDataUrl.toString();


nit: put the addTearDown just before this line, instead of up at the top of the function — that pattern helps draw the connection, and helps us more consistently remember to include the tear-down

gnprice · 2025-01-30T22:29:00Z

lib/widgets/app.dart

@@ -177,9 +215,6 @@ class _ZulipAppState extends State<ZulipApp> with WidgetsBindingObserver {
    final themeData = zulipThemeData(context);
    return GlobalStoreWidget(
      child: Builder(builder: (context) {
-        final globalStore = GlobalStoreWidget.of(context);
-        // TODO(#524) choose initial account as last one used
-        final initialAccountId = globalStore.accounts.firstOrNull?.id;


app: Query initial-account-id while handling initial routes This avoids a potential race if the queried account is logged out between the invocation of `_ZulipAppState.build` and `MaterialApp.onGenerateInitialRoutes`.

Can that actually happen? I suspect it can't.

Or rather: the actual scenario where this would make a non-NFC difference is if the account gets logged out between this builder callback (underneath GlobalStoreWidget) getting called, and onGenerateInitialRoutes getting called. I'd guess that those happen synchronously in the same frame.

Not worth spending time investigating that, if you don't already know of a way it can happen after all — Navigator is real complicated. Just add a few words like:

This avoids a potential race if the queried account is logged out between the invocation of this Builder callback and `MaterialApp.onGenerateInitialRoutes` (if such a race is possible).

if that's an accurate description of what you know.

[…] I'd guess that those happen synchronously in the same frame.

Not worth spending time investigating that, if you don't already know of a way it can happen after all — Navigator is real complicated.

Correct, I'm not aware of a way it can happen after all.

gnprice · 2025-01-30T22:45:17Z

lib/widgets/app.dart

-    final initialRouteUrl = Uri.parse(WidgetsBinding.instance.platformDispatcher.defaultRouteName);
-    if (initialRouteUrl case Uri(scheme: 'zulip', host: 'notification')) {
-      await NotificationDisplayManager.navigateForNotification(initialRouteUrl);
+  InitialRouteListFactory _handleGenerateInitialRoutes(BuildContext context) {


The "handle foo" name for this doesn't quite fit, because this isn't a callback — this widget's code calls this function eagerly for itself, rather than passing it to some other code to call later. Rather this is a helper function that returns a callback.

It could be renamed, but I think actually the cleanest solution is to make it a callback, erasing the distinction between the first half of this method body which gets called immediately and the second half which gets called later. The GlobalStoreWidget.of call can move inside the callback just as well as the globalStore.accounts.firstOrNull?.id lookup did.

gnprice · 2025-01-30T22:47:01Z

lib/widgets/app.dart

+        else
+          HomePage.buildRoute(accountId: initialAccountId),
+      ];
+    };
  }

  @override
  void initState() {


nit: keep these lifecycle methods up at the top (and next to the didPushRouteInformation method, which they drive); _handleGenerateInitialRoutes can go after

(perhaps in fact move them to the very top, so didPushRouteInformation can be closer to _handleGenerateInitialRoutes — it's true those are related)

gnprice · 2025-01-30T23:04:31Z

lib/widgets/app.dart

+          return [
+            HomePage.buildRoute(accountId: notificationResult.accountId),
+            notificationResult.route,
+          ];


It's a bit unsatisfying to have routeForNotification have to return two values like this, when the route already includes the account ID.

I think we can simplify to make it look like this:

Suggested change

return [

HomePage.buildRoute(accountId: notificationResult.accountId),

notificationResult.route,

];

return [

HomePage.buildRoute(accountId: route.accountId),

route,

];

with a small refactor to make the account ID more transparent on the route.

I'll push a small NFC onto your PR branch that makes that refactor. Then you can move it to the front of the branch and make use of it.

gnprice · 2025-01-30T23:14:14Z

lib/widgets/app.dart

+      showErrorDialog(context: navigatorContext,
+        title: zulipLocalizations.errorNotificationOpenTitle,
+        message: zulipLocalizations.errorNotificationOpenAccountMissing);


This is exactly the same error dialog as navigateForNotification shows, right?

Which is probably a good thing — they're basically the same situation, differing only in whether the app was already running (including in the background).

So maybe routeForNotification should go ahead and show the error. That'd keep the details of this error in one place, over in the notifications code which is good because it's an error about the notification.

gnprice · 2025-01-30T23:27:00Z

test/notifications/display_test.dart

+      check(pushedRoutes).deepEquals(<Condition<Object?>>[
+        (it) => it.isA<MaterialAccountWidgetRoute>()
+          ..accountId.equals(accountB.id)
+          ..page.isA<HomePage>(),
+        (it) => it.isA<MaterialAccountWidgetRoute>()
+          ..accountId.equals(accountB.id)
+          ..page.isA<MessageListPage>()
+            .initNarrow.equals(SendableNarrow.ofMessage(message,
+              selfUserId: accountB.userId))
+      ]);
+      pushedRoutes.clear();


I think we can tighten this to make it compare more directly with the neighboring tests, by extending takeStartingRoutes a bit:

Suggested change

check(pushedRoutes).deepEquals(<Condition<Object?>>[

(it) => it.isA<MaterialAccountWidgetRoute>()

..accountId.equals(accountB.id)

..page.isA<HomePage>(),

(it) => it.isA<MaterialAccountWidgetRoute>()

..accountId.equals(accountB.id)

..page.isA<MessageListPage>()

.initNarrow.equals(SendableNarrow.ofMessage(message,

selfUserId: accountB.userId))

]);

pushedRoutes.clear();

takeStartingRoutes(account: accountB);

matchesNavigation(check(pushedRoutes).single, accountB, message);

This avoids a potential race if the queried account is logged out between the invocation of this Builder callback and `MaterialApp.onGenerateInitialRoutes` (if such a race is possible).

…ground Previously, when two accounts (Account-1 and Account-2) were logged in, the app always defaulted to showing the home page of Account-1 on launch. If the app was closed and the user opened a notification from Account-2, the navigation stack would be: HomePage(Account-1) -> MessageListPage(Account-2) This commit fixes that behaviour, now when a notification is opened while the app is closed, the home page will correspond to the account associated with the notification's conversation. This addresses zulip#1210 for background notifications.

This fixes a potential bug, in case the server returned `realm_url` contains a trailing `/`.

rajveermalviya · 2025-02-07T21:29:56Z

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

gnprice

Thanks for the revision!

This looks good except one further simplification I think can be made to the logic that interacts with the navigator. Also a couple of nits.

gnprice · 2025-02-08T00:11:50Z

lib/widgets/app.dart

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


nit:

Suggested change

// The account didn't match any existing accounts,

// fallthrough to show default the route below.

}

// The account didn't match any existing accounts,

// fallthrough to show the default route below.

}

?

ah and another nit: "fall through" is the verb, while "fallthrough" is a noun

(similar to "clean up" / "cleanup" and so on, as in #1240 (comment) )

gnprice · 2025-02-08T00:16:47Z

lib/notifications/display.dart

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


Instead of having to go look up a context via ZulipApp statics like this, let's have routeForNotification take a BuildContext argument. Its callers both have such an argument already.

Then it can also use that to look up the global store. That's how both of its callers are getting the global store themselves. (As a bonus, one caller will no longer need to do that lookup at all.)

gnprice · 2025-02-08T00:26:45Z

lib/notifications/display.dart

+    // A helper to avoid making this function an async function,
+    // allowing it to be called from non-async contexts and where
+    // BuildContext hasn't been initialized with localizations yet.
+    void showNotificationErrorDialog() async {


Ah, I guess this does add a bit of complication to my picture from the previous comment. But:

One caller, navigateForNotification, already does that wait before it calls this function. So it can just pass the context it gets from that.

In the other caller, _handleGenerateInitialRoutes is called from MaterialApp.onGenerateInitialRoutes. It's true that that callback doesn't get passed a BuildContext that's under the navigator (and so can be used by showErrorDialog).

But: by the time onGenerateInitialRoutes gets called, the navigator has been mounted. In fact here's its call site, in _WidgetsAppState.build:

child: Navigator( // … onGenerateInitialRoutes: widget.onGenerateInitialRoutes == null ? Navigator.defaultGenerateInitialRoutes : (NavigatorState navigator, String initialRouteName) { return widget.onGenerateInitialRoutes!(initialRouteName); },

(That calls WidgetsApp.onGenerateInitialRoute, which MaterialApp passed directly from MaterialApp.onGenerateInitialRoute.)

So by the time it's called, there's a NavigatorState object there.

Sadly that NavigatorState doesn't get passed on to our callback. But I believe that still means ZulipApp.navigator should work at that point, to get our hands on that NavigatorState after all.

rajveermalviya added the maintainer review PR ready for review by Zulip maintainers label Dec 30, 2024

rajveermalviya requested a review from chrisbobbe December 30, 2024 20:37

chrisbobbe reviewed Dec 30, 2024

View reviewed changes

rajveermalviya force-pushed the pr-notif-route-account branch 2 times, most recently from 1b7b732 to 54adf40 Compare December 30, 2024 21:42

rajveermalviya requested a review from chrisbobbe December 30, 2024 21:45

chrisbobbe reviewed Dec 30, 2024

View reviewed changes

rajveermalviya force-pushed the pr-notif-route-account branch from 54adf40 to ae30cd3 Compare January 2, 2025 16:14

rajveermalviya requested a review from chrisbobbe January 6, 2025 11:19

PIG208 assigned chrisbobbe Jan 8, 2025

chrisbobbe reviewed Jan 25, 2025

View reviewed changes

rajveermalviya force-pushed the pr-notif-route-account branch 4 times, most recently from ac26a01 to 856a702 Compare January 28, 2025 14:57

rajveermalviya requested a review from chrisbobbe January 28, 2025 15:01

chrisbobbe requested a review from gnprice January 29, 2025 01:45

chrisbobbe assigned gnprice and unassigned chrisbobbe Jan 29, 2025

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 Jan 29, 2025

gnprice reviewed Jan 30, 2025

View reviewed changes

rajveermalviya force-pushed the pr-notif-route-account branch 4 times, most recently from d11b519 to acafa4d Compare February 7, 2025 21:12

rajveermalviya and others added 7 commits February 8, 2025 02:46

notif test: Clean up TestPlatformDispatcher.defaultRouteNameTestValue

f551aec

app: Query initial-account-id while handling initial routes

ea5c881

This avoids a potential race if the queried account is logged out between the invocation of this Builder callback and `MaterialApp.onGenerateInitialRoutes` (if such a race is possible).

app [nfc]: Reorder _ZulipAppState methods

58bc2fa

app [nfc]: Pull out _handleGenerateInitialRoutes

f6fbc99

page [nfc]: Add interface to get account ID for most of our routes

7648d9c

notif: Query account by realm URL origin, not full URL

33bfa91

This fixes a potential bug, in case the server returned `realm_url` contains a trailing `/`.

rajveermalviya force-pushed the pr-notif-route-account branch from acafa4d to 33bfa91 Compare February 7, 2025 21:16

rajveermalviya requested a review from gnprice February 7, 2025 21:30

gnprice reviewed Feb 8, 2025

View reviewed changes

notif: Use associated account as initial account; if opened from background #1240

Are you sure you want to change the base?

notif: Use associated account as initial account; if opened from background #1240

Conversation

rajveermalviya commented Dec 30, 2024

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

rajveermalviya commented Dec 30, 2024

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

chrisbobbe Dec 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajveermalviya commented Jan 6, 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

rajveermalviya commented Jan 28, 2025

chrisbobbe commented Jan 29, 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

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 Feb 7, 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

chrisbobbe Dec 30, 2024 •

edited

Loading