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

We are pretty certain that the upstream commit flutter/flutter@df114fbe9 was the exact one that caused the increased RAM usage for the Android build. The upstream change causes the engine artifacts to contain debug symbols, which is intentional (as it enables putting debug symbols in the app's bundle artifact) but makes the artifacts about 8x larger. That causes known regressions in CPU and memory use at build time: flutter/flutter#162675 Since the regression was expected, there is no action to be taken upstream. However, we haven't found an effective way to rewrite the build script in a way that this is mitigated without needing to raise the limit. For the investigation details, see CZO discussion: https://chat.zulip.org/#narrow/channel/243-mobile-team/topic/Gradle.20out.20of.20memory Since a previous bump to 3 GiB, the issue has been mitigated, but it still happens some of the time: at least twice in the past day or so. Add another 1 GiB to see if that addresses the flakes.

This avoids a potential race if the queried account is logged out between the invocation of `_ZulipAppState.build` and `MaterialApp.onGenerateInitialRoutes`.

…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 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 from 897ed88 to 4e505a7 Compare February 7, 2025 10:03

rajveermalviya force-pushed the pr-notif-route-account branch from 4e505a7 to 70073f6 Compare February 7, 2025 10:14

notif test: Clean up TestPlatformDispatcher.defaultRouteNameTestValue

bbe796b

rajveermalviya and others added 5 commits February 7, 2025 15:53

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

dff757d

This avoids a potential race if the queried account is logged out between the invocation of `_ZulipAppState.build` and `MaterialApp.onGenerateInitialRoutes`.

app [nfc]: Pull out _handleGenerateInitialRoutes

9345e52

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

e263d1d

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

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

d11b519

rajveermalviya force-pushed the pr-notif-route-account branch from 70073f6 to d11b519 Compare February 7, 2025 10:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rajveermalviya commented Dec 30, 2024

chrisbobbe left a comment

chrisbobbe Dec 30, 2024

chrisbobbe Dec 30, 2024

chrisbobbe Dec 30, 2024

chrisbobbe Dec 30, 2024

chrisbobbe Dec 30, 2024

chrisbobbe Dec 30, 2024

rajveermalviya commented Dec 30, 2024

chrisbobbe left a comment

chrisbobbe Dec 30, 2024

rajveermalviya Jan 6, 2025

chrisbobbe Dec 30, 2024 •

edited

Loading

chrisbobbe Dec 30, 2024

rajveermalviya commented Jan 6, 2025

chrisbobbe left a comment

chrisbobbe Jan 24, 2025

chrisbobbe Jan 25, 2025

chrisbobbe Jan 25, 2025

rajveermalviya Jan 28, 2025

rajveermalviya commented Jan 28, 2025

chrisbobbe commented Jan 29, 2025

gnprice left a comment

gnprice Jan 30, 2025

gnprice Jan 30, 2025

gnprice Jan 30, 2025

gnprice Jan 30, 2025

chrisbobbe Feb 6, 2025

gnprice Jan 30, 2025

gnprice Jan 30, 2025

gnprice Jan 30, 2025

gnprice Jan 30, 2025

gnprice Jan 30, 2025

gnprice Jan 30, 2025

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

chrisbobbe Dec 30, 2024 •

edited

Loading