On launch, go to the last visited account #1784

sm-sayedi · 2025-08-05T11:01:37Z

Fixes: #524

chrisbobbe

Thanks! Comments below.

chrisbobbe · 2025-08-08T21:18:09Z

lib/model/database.dart

 }

+@DataClassName('IntGlobalSettingRow')
+class IntGlobalSettings extends Table {


The commit 5cfd4ae, which added BoolGlobalSettings, included a lot of useful material that I think should be easy to propagate to this new thing:

(EDIT: and I see some of these have actually been addressed in the latest revision; I'll strike out those parts. 🙂)

The commit message points to why the new thing is helpful, in fact as soon as the summary line

An implementation comment in GlobalSettings saying "consider whether [etc.] can do the job instead", which helps us get the most usefulness from the thing. Let's change that comment so that it also mentions the int global settings

~~Dartdocs on BoolGlobalSettings and its fields.~~ (Let's also add bidirectional "See also:" notes in that class's dartdoc and in IntGlobalSettings's.)

Ditto the BoolGlobalSetting enum, and it also got this helpful-looking implementation comment:
// Former settings which might exist in the database, // whose names should therefore not be reused: // (this list is empty so far)
(No need to add a placeholderIgnore member, though; the pseudo-setting we're adding here isn't an "experimental feature" setting and we don't plan to remove it.)

~~Tests~~

chrisbobbe · 2025-08-08T21:21:10Z

lib/widgets/app.dart

+  @override
+  void didPush(Route<void> route, Route<void>? previousRoute) {
+    _changeLastVisitedAccountIfNecessary(route);
+  }
+
+  @override
+  void didPop(Route<void> route, Route<void>? previousRoute) {
+    _changeLastVisitedAccountIfNecessary(previousRoute);
+  }
+
+  @override
+  void didRemove(Route<void> route, Route<void>? previousRoute) {
+    _changeLastVisitedAccountIfNecessary(previousRoute);
+  }
+
+  @override
+  void didReplace({Route<void>? newRoute, Route<void>? oldRoute}) {
+    _changeLastVisitedAccountIfNecessary(newRoute);
+  }


Can we just use didChangeTop instead of all these, and inline _changeLastVisitedAccountIfNecessary?

Thanks, that's interesting. I think it can replace them!

chrisbobbe · 2025-08-08T21:39:54Z

lib/widgets/app.dart

-                  unawaited(logOutAccount(GlobalStoreWidget.of(context), accountId));
+                  unawaited(Future(() async {
+                    if (!context.mounted) return;
+                    await logOutAccount(GlobalStoreWidget.of(context), accountId);
+                    if (!context.mounted) return;
+                    await removeLastVisitedAccountIfNecessary(
+                      GlobalStoreWidget.of(context), accountId);
+                  }));


The if (!context.mounted) return;—really the fact that we clear lastVisitedAccountId in a separate database transaction from removing the Account in logOutAccount—means it's possible to enter a state where lastVisitedAccountId is an ID of an account that doesn't actually exist. That seems problematic when, later, we pass lastVisitedAccountId to HomePage.buildRoute, which is UI code to show a page for the account. From reading code, I think the behavior might not be worse than showing the page for a frame then immediately hiding it…but even that seems glitchy and worth avoiding.

I see two options:

Clear lastVisitedAccountId in the same DB transaction as the one in doRemoveAccount

Don't bother clearing lastVisitedAccountId on logout, but make its interface clear (in dartdoc) that it might point to an account that doesn't exist because it was logged out. Then, before passing the value to HomePage.buildRoute, check that it refers to an account that actually exists. If not, just do the same as we do when no account has ever been visited, i.e., open the choose-account page.

Curious for @gnprice's thoughts on this too :)

Went with the second option for the new revision until Greg shares his thoughts!

It looks like the new revision implements something that's not really either of those options. :) To do the second option completely, let's remove the removeLastVisitedAccountIfNecessary method and the "very-rare-edge-case" branding, and just say that lastVisitedAccountId will commonly refer to a nonexistent account, and handling that possibility is the responsibility of any code that reads it.

Ahh, thanks. I think I misunderstood that first clause of the second option: 😀

Don't bother clearing lastVisitedAccountId on logout,

I think the second option is fine.

The first, using a transaction, would be cleaner in principle — it would mean that when this value is present, it always actually points to a valid account and to the account that's intended. But I think arranging for these to share a transaction may be annoying given the way we currently have the code organized, with settings (including this "setting") in one place and the accounts in another. So we can defer solving that problem until we run into something that more strongly needs us to solve it, if we ever do.

The one thing that would potentially be glitchy about this in practice is if we ended up with an unrelated later account at the same account ID, with lastVisitedAccountId pointing to that one just because it had previously pointed to the previous/deleted account with that ID. (I'm not sure if it's actually possible for us to re-use an account ID; might not be.) But if we do add a new account, we'll immediately push a route for that account, which will update lastVisitedAccountId, so I think it'd be difficult for that to happen.

(I'm not sure if it's actually possible for us to re-use an account ID; might not be.)

With the fact that the account ID is auto-incremented in the database table, I think it will not be possible to have a previous ID with a new account. Or maybe I am missing something. 🙂

Yeah, I agree — that's the reason it might not be possible.

chrisbobbe · 2025-08-12T22:12:53Z

test/widgets/app_test.dart

+        final (actionButton, _) = await prepare(tester,
+          accounts: [eg.selfAccount], logoutAccount: eg.selfAccount);


How about using [eg.selfAccount] and eg.selfAccount by default?

Made them the defaults in the new revision. But wouldn't it be good to have them specified explicitly in each test case? That way, the reader can easily confirm the lines in these test cases where it looks for the accounts list (without looking inside prepare and seeing what the default is):

testWidgets('user confirms logging out', (tester) async { final (actionButton, _) = await prepare(tester); // ... check(testBinding.globalStore).accounts.isEmpty(); } testWidgets('user cancels logging out', (tester) async { final (_, cancelButton) = await prepare(tester); // ... check(testBinding.globalStore).accounts.deepEquals([eg.selfAccount]); }

I see; sure, that's reasonable, SGTM.

sm-sayedi · 2025-08-13T07:55:50Z

Thanks @chrisbobbe for the review. New changes pushed, PTAL.

sm-sayedi · 2025-08-13T18:37:28Z

Pushed a new revision, making a small change to the newly-merged share feature, with the content being shared to the last visited account if present, or otherwise to the first one.

chrisbobbe

Thanks! Comments below.

chrisbobbe · 2025-08-15T02:51:06Z

lib/widgets/app.dart

-                  unawaited(logOutAccount(GlobalStoreWidget.of(context), accountId));
+                  unawaited(Future(() async {
+                    if (!context.mounted) return;
+                    await logOutAccount(GlobalStoreWidget.of(context), accountId);
+                    if (!context.mounted) return;
+                    await removeLastVisitedAccountIfNecessary(
+                      GlobalStoreWidget.of(context), accountId);
+                  }));


It looks like the new revision implements something that's not really either of those options. :) To do the second option completely, let's remove the removeLastVisitedAccountIfNecessary method and the "very-rare-edge-case" branding, and just say that lastVisitedAccountId will commonly refer to a nonexistent account, and handling that possibility is the responsibility of any code that reads it.

chrisbobbe · 2025-08-15T02:53:37Z

test/widgets/app_test.dart

+        final (actionButton, _) = await prepare(tester,
+          accounts: [eg.selfAccount], logoutAccount: eg.selfAccount);


I see; sure, that's reasonable, SGTM.

chrisbobbe · 2025-08-15T02:59:47Z

test/notifications/open_test.dart

+      await testBinding.globalStore.settings
+        .setInt(IntGlobalSetting.lastVisitedAccountId, eg.selfAccount.id);


What's the purpose of this setup; what goes wrong if we don't include it? IIUC these tests are about what happens when you tap on a notification, and that shouldn't be affected by the last-visited account, right? (It should just open whichever account the notification is for.)

If that's omitted, the test will fail with the following output:

Test output

00:01 +0: NotificationOpenService stream message (variant: TargetPlatform.android) ══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════ The following TestFailure was thrown running a test: Expected: a Iterable<Route<void>> that: is deeply equal to [<A value that: is a MaterialAccountWidgetRoute<Object?> has accountId that: equals <1001> has page that: is a HomePage>] Actual: (MaterialWidgetRoute<Object?>(RouteSettings(none, null), animation: AnimationController#92b89(⏭ 1.000; paused; for MaterialWidgetRoute<Object?>(null)))) Which: has an element at [<0>] that: Actual: <MaterialWidgetRoute<Object?>(RouteSettings(none, null), animation: AnimationController#92b89(⏭ 1.000; paused; for MaterialWidgetRoute<Object?>(null)))> which Is a MaterialWidgetRoute<Object?> When the exception was thrown, this was the stack: #0 check.<anonymous closure> (package:checks/src/checks.dart:85:9) #1 _TestContext.expect (package:checks/src/checks.dart:708:12) #2 IterableChecks.deepEquals (package:checks/src/extensions/iterable.dart:244:12) #3 main.<anonymous closure>.takeStartingRoutes (file:///Users/sm-sayedi/Projects/open-source/zulip-flutter/test/notifications/open_test.dart:103:49) #4 main.<anonymous closure>.prepare (file:///Users/sm-sayedi/Projects/open-source/zulip-flutter/test/notifications/open_test.dart:121:7) <asynchronous suspension> #5 main.<anonymous closure>.<anonymous closure> (file:///Users/sm-sayedi/Projects/open-source/zulip-flutter/test/notifications/open_test.dart:197:7) <asynchronous suspension> #6 testWidgets.<anonymous closure>.<anonymous closure> (package:flutter_test/src/widget_tester.dart:192:15) <asynchronous suspension> #7 TestWidgetsFlutterBinding._runTestBody (package:flutter_test/src/binding.dart:1059:5) <asynchronous suspension> <asynchronous suspension> (elided one frame from package:stack_trace) The test description was: stream message (variant: TargetPlatform.android) ════════════════════════════════════════════════════════════════════════════════════════════════════ 00:01 +0 -1: NotificationOpenService stream message (variant: TargetPlatform.android) [E] Test failed. See exception logs above. The test description was: stream message (variant: TargetPlatform.android)

That's because the test first prepares the app (opens an account), then opens a notification. Previously, the first account was opened, but now the account corresponding to the lastVisitedAccountId is opened; otherwise, it will open the "Choose account" page if that's not present (specified).

Could we refactor to deduplicate this boilerplate? For example:

One or more prep commits to change the interface of prepare and init to accept an optional list of accounts for init to call testBinding.globalStore.add with, defaulting to [eg.selfAccount] if that list isn't passed

In the commit targeting On launch go to last account used, not just first in list #524, have init set IntGlobalSetting.lastVisitedAccountId to a reasonable value, which could be .firstOrNull of that list of accounts, or maybe an optional lastVisitedAccountId param if callers need to control that

takeStartingRoutes reads IntGlobalSetting.lastVisitedAccountId and decides which "starting routes" it should expect, based on that

chrisbobbe · 2025-08-15T03:00:40Z

test/widgets/app_test.dart

+        ]);
+      });
+
+      testWidgets('with last account visited, go to home page for last account', (tester) async {


nit: I would order the most common, "happy-path" test before the others

sm-sayedi · 2025-08-15T14:26:40Z

Thanks for the review. Pushed new changes.

chrisbobbe

Thanks! Small comments below.

chrisbobbe · 2025-08-18T18:20:27Z

lib/model/actions.dart

+Future<void> removeLastVisitedAccountIfNecessary(GlobalStore store, int loggedOutAccountId) async {
+  // If account is not logged out yet, do nothing.
+  if (store.getAccount(loggedOutAccountId) != null) return;
+
+  // If the logged-out account is different than the last visited one, do nothing.
+  if (loggedOutAccountId != store.settings.getInt(IntGlobalSetting.lastVisitedAccountId)) {
+    return;
+  }
+
+  await store.settings.setInt(IntGlobalSetting.lastVisitedAccountId, null);
+}


Bump on removing this ( #1784 (comment) )

chrisbobbe · 2025-08-18T18:29:02Z

lib/widgets/app.dart

+      if (lastAccountMissing)
        MaterialWidgetRoute(page: const ChooseAccountPage())


Suggested change

if (lastAccountMissing)

MaterialWidgetRoute(page: const ChooseAccountPage())

if (lastAccountMissing)

// No account has been visited, or the last-visited account was logged out.

MaterialWidgetRoute(page: const ChooseAccountPage())

chrisbobbe · 2025-08-18T18:59:04Z

test/notifications/open_test.dart

+      await testBinding.globalStore.settings
+        .setInt(IntGlobalSetting.lastVisitedAccountId, eg.selfAccount.id);


Could we refactor to deduplicate this boilerplate? For example:

One or more prep commits to change the interface of prepare and init to accept an optional list of accounts for init to call testBinding.globalStore.add with, defaulting to [eg.selfAccount] if that list isn't passed

In the commit targeting On launch go to last account used, not just first in list #524, have init set IntGlobalSetting.lastVisitedAccountId to a reasonable value, which could be .firstOrNull of that list of accounts, or maybe an optional lastVisitedAccountId param if callers need to control that

takeStartingRoutes reads IntGlobalSetting.lastVisitedAccountId and decides which "starting routes" it should expect, based on that

sm-sayedi · 2025-08-18T20:41:03Z

Thanks for the review. Changes pushed.

chrisbobbe · 2025-09-03T19:57:42Z

Thanks! LGTM; Greg in your review, could you please weigh in on #1784 (comment), the question of using the literal string 'lastVisitedAccountId' vs. IntGlobalSetting.lastVisitedAccountId.name?

gnprice

Thanks @sm-sayedi and @chrisbobbe for all the reviews and revisions while I was away!

Generally this all looks good now; just small comments below.

This review is based on reading all the changes since my previous review
#1784 (review) . (So I guess I should still read the whole set of test changes the PR makes.) I also replied to #1784 (comment) above.

gnprice · 2025-09-04T04:12:07Z

lib/model/database.dart

+          'name':
+                   // i.e., `IntGlobalSetting.lastVisitedAccountId.name` as of
+                   // writing this migration
+                   Variable('lastVisitedAccountId'),


nit: compact this formatting a bit

Suggested change

'name':

// i.e., `IntGlobalSetting.lastVisitedAccountId.name` as of

// writing this migration

Variable('lastVisitedAccountId'),

// i.e., `IntGlobalSetting.lastVisitedAccountId.name` as of

// writing this migration

'name': Variable('lastVisitedAccountId'),

gnprice · 2025-09-04T04:14:51Z

lib/model/database.dart

+      if (firstAccount == null) return;
+
+      final firstAccountId = firstAccount.read<int>('id');
+      await m.database.into(schema.intGlobalSettings).insert(


Actually, though, let's have a higher-level comment instead:

Suggested change

await m.database.into(schema.intGlobalSettings).insert(

// Like `globalStore.setLastVisitedAccount(firstAccountId)`,

// as of the schema at the time of this migration.

await m.database.into(schema.intGlobalSettings).insert(

Then I think the one below isn't needed.

gnprice · 2025-09-04T04:16:50Z

lib/model/database.dart

+      final firstAccount = await (m.database.select(schema.accounts)
+        ..limit(1)).getSingleOrNull();
+      if (firstAccount == null) return;
+
+      final firstAccountId = firstAccount.read<int>('id');


This version is OK, but ideally we'd adjust the SELECT statement so it fetches only the id field in the first place. (That's not critical here because this table isn't a lot of data, so it's not a problem to fetch the other columns even though we won't use them.) Can you look in the Drift docs and see what APIs are available for narrowing the query like that?

Nice idea. It can be achieved using selectOnly; used it in the new revision.

gnprice · 2025-09-04T04:19:00Z

lib/model/store.dart

+    if (id == null) return null; // No account has been visited yet.
+
+    // (Will be null if `id` refers to an account that has been logged out.)
+    return _accounts[id];


nit:

Suggested change

return _accounts[id];

return getAccount(id);

That way _accounts remains a bit more encapsulated among the methods that provide the list of accounts.

gnprice · 2025-09-04T04:29:13Z

test/model/database_test.dart

+      final after = v10.DatabaseAtV10(schema.newConnection());
+      final intGlobalSettings = await after.select(after.intGlobalSettings).getSingle();
+      check(intGlobalSettings.name).equals(IntGlobalSetting.lastVisitedAccountId.name);
+      check(intGlobalSettings.value).equals(1);


Instead of a hard-coded 1, it's better if this refers to the account ID of the intended one out of the two accounts inserted above.

Could do that either by selecting the account back out of the table, or using the return value of insert.

gnprice · 2025-09-04T04:39:16Z

test/widgets/app_test.dart

+      // testWidgets('when just upgraded to get the last-visited-account feature, '
+      //     'if there is at least one account, open that one, not the choose-account page', (tester) async {
+      //   // The database migration step from9To10 ensures that
+      //   // [IntGlobalSetting.lastVisitedAccountId], thus [GlobalStore.lastVisitedAccount],
+      //   // points to the first account in the list. That migration step has
+      //   // its own tests in test/model/database_test.dart.
+      // });


In general we don't check in commented-out code.

The comment within the body here would be fine, with a bit of editing to replace the context given by the (commented-out) test name. But I think it isn't necessary: from this code's perspective, there just either is or isn't a last-visited account, so the two test cases above and below here cover those cases.

(I'm guessing this test was originally written for a previous revision of this PR where this behavior was implemented in widgets/app.dart rather than in the DB migration. For readers of the PR thread, it could be useful to explain why this test isn't needed, because they saw the stage where it would have been needed. But readers of this code or this commit don't have that history, and as a result don't need that explanation.)

bump — can cut this whole comment

Oops, thanks. I wanted to remove the commented-out code at the end to preserve the line numbers to the version in the review, but forgot to do so.

gnprice · 2025-09-04T04:41:09Z

test/widgets/app_test.dart

+        final future = logOutAccount(testBinding.globalStore, eg.selfAccount.id);
+        await tester.pump(TestGlobalStore.removeAccountDuration);
+        await future;
+        check(testBinding.globalStore).lastVisitedAccount.isNull();


Maybe add something like:

Suggested change

check(testBinding.globalStore).lastVisitedAccount.isNull();

check(testBinding.globalStore).lastVisitedAccount.isNull();

check(testBinding.globalStore).accounts.isNotEmpty();

to make it clear how this setup differs from just not having any accounts in the first place.

gnprice · 2025-09-04T04:42:05Z

test/widgets/app_test.dart

+        await testBinding.globalStore.add(
+          eg.otherAccount, eg.initialSnapshot(realmUsers: [eg.otherUser]),
+          markLastVisited: false);
+        check(testBinding.globalStore).lastVisitedAccount.equals(eg.selfAccount);
+        final future = logOutAccount(testBinding.globalStore, eg.selfAccount.id);


Can simplify this a bit, right?

Suggested change

await testBinding.globalStore.add(

eg.otherAccount, eg.initialSnapshot(realmUsers: [eg.otherUser]),

markLastVisited: false);

check(testBinding.globalStore).lastVisitedAccount.equals(eg.selfAccount);

final future = logOutAccount(testBinding.globalStore, eg.selfAccount.id);

await testBinding.globalStore.add(

eg.otherAccount, eg.initialSnapshot(realmUsers: [eg.otherUser]));

check(testBinding.globalStore).lastVisitedAccount.equals(eg.otherAccount);

final future = logOutAccount(testBinding.globalStore, eg.otherAccount.id);

gnprice · 2025-09-04T04:43:05Z

test/model/test_store.dart

  /// account, in particular when a [PerAccountStoreWidget] is mounted.
-  Future<void> add(Account account, InitialSnapshot initialSnapshot) async {
+  ///
+  /// By default, the account is marked as [IntGlobalSetting.lastVisitedAccountId].


nit: refer to setLastVisitedAccount instead, since that's now the interface the bulk of the app consumes

gnprice · 2025-09-04T04:48:16Z

test/widgets/app_test.dart

+      await testBinding.globalStore.add(
+        eg.otherAccount, eg.initialSnapshot(realmUsers: [eg.otherUser]),
+        markLastVisited: false);


nit: can simplify to not use markLastVisited:

Suggested change

await testBinding.globalStore.add(

eg.otherAccount, eg.initialSnapshot(realmUsers: [eg.otherUser]),

markLastVisited: false);

await testBinding.globalStore.add(

eg.otherAccount, eg.initialSnapshot(realmUsers: [eg.otherUser]));

and just flip around which account is which in the later steps below.

(For existing test cases, it's good to use markLastVisited wherever that makes it possible to avoid other changes to the tests. But when adding a new test, it's just as easy to add the alternative version as to add this version.)

See discussion: zulip#1784 (comment)

sm-sayedi · 2025-09-04T17:03:02Z

Thanks for the review @gnprice. New revision pushed, PTAL.

gnprice

Thanks for the revision! Just a couple of comments on that.

gnprice · 2025-09-04T19:57:00Z

test/widgets/app_test.dart

+      // testWidgets('when just upgraded to get the last-visited-account feature, '
+      //     'if there is at least one account, open that one, not the choose-account page', (tester) async {
+      //   // The database migration step from9To10 ensures that
+      //   // [IntGlobalSetting.lastVisitedAccountId], thus [GlobalStore.lastVisitedAccount],
+      //   // points to the first account in the list. That migration step has
+      //   // its own tests in test/model/database_test.dart.
+      // });


bump — can cut this whole comment

gnprice · 2025-09-04T19:59:45Z

test/widgets/app_test.dart

+    testWidgets('choosing an account changes the last visited account', (tester) async {
+      addTearDown(testBinding.reset);
+      await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot());
+      await testBinding.globalStore.add(eg.otherAccount, eg.initialSnapshot(
+        realmUsers: [eg.otherUser]));


nit: this gets introduced in one commit, then rearranged in a later commit; should just get the desired version directly in the commit that introduces it

Looking at the test case again, if we make the change in the commit that introduces it, it will fail, unless we rearrange it in the later commit. That's because in the earlier commit, the app goes to the first account, instead of the last visited one. Maybe we would want to use the version with markLastVisited: false again.

Hmm, I see. Sure.

gnprice

OK, and I've now read through all the test changes too. Comments below.

gnprice · 2025-09-04T20:06:03Z

test/model/settings_test.dart

+      test('false to true -> does the update', () async {
+        final globalSettings = eg.globalStore(boolGlobalSettings: {
+          BoolGlobalSetting.placeholderIgnore: false,
+        }).settings;
+        globalSettings.addListener(() => notifiedCount++);
+
+        await globalSettings.setBool(BoolGlobalSetting.placeholderIgnore, true);
+        check(notifiedCount).equals(1);
+      });


There's quite a bit of duplication in this set of test cases. I think they can be made easier to read and understand by factoring out a helper like so:

Suggested change

test('false to true -> does the update', () async {

final globalSettings = eg.globalStore(boolGlobalSettings: {

BoolGlobalSetting.placeholderIgnore: false,

}).settings;

globalSettings.addListener(() => notifiedCount++);

await globalSettings.setBool(BoolGlobalSetting.placeholderIgnore, true);

check(notifiedCount).equals(1);

});

test('false to true -> does the update', () async {

await checkNotifiedCount(false, true, 1);

});

(Then similarly for the setInt cases when those are added later.)

gnprice · 2025-09-04T20:15:34Z

lib/model/settings.dart

+  /// A non-setting to ensure this enum has at least one value.
+  ///
+  /// (This is also handy to use in tests.)
+  placeholderIgnore,
+
+  /// A pseudo-setting recording the id of the account the user has visited most
+  /// recently, from the list of all the available accounts on the device.
+  ///
+  /// In some cases, this may point to an account that doesn't actually exist on
+  /// the device, for example, when the last visited account is logged out and
+  /// another account is not visited during the same running session. For cases
+  /// like these, it's the responsibility of the code that reads this value to
+  /// check for the availability of the account that corresponds to this id.
+  lastVisitedAccountId,


Now that you have a placeholderIgnore value in this enum, I think it should be easy to split this commit:
24f4e44 settings: Make general int global settings, so as to add without migrations

up into two commits:

one with the existing commit message, which adds all the general infrastructure but not lastVisitedAccountId specifically;

one that adds lastVisitedAccountId, along with the GlobalStore methods for it and the nav observer that keeps it updated.

I guess the cost of that is that then there'd be two migrations, and two added copies of the schema. Still, I think it'd make the commit a lot clearer to read. So let's go ahead and do that.

gnprice · 2025-09-04T20:22:55Z

test/model/test_store.dart

  }) : super(backend: _TestGlobalStoreBackend(),
         globalSettings: globalSettings ?? GlobalSettingsData(),
         boolGlobalSettings: boolGlobalSettings ?? {},
+         intGlobalSettings: intGlobalSettings ?? {}


nit: needs trailing comma

gnprice · 2025-09-04T20:27:41Z

test/notifications/open_test.dart

+      if (lastVisitedAccount != null) {
+        takeHomePageRouteForAccount(lastVisitedAccount.id);
+      } else {
+        takeChooseAccountPageRoute();


nit: this conditional gets introduced in one commit, and then the order is swapped around in a later commit; better to simplify the diff in the later commit by having it not change the order.

(I think either order is fine; either have the earlier commit go straight to this order, or have the later commit leave it in the other order.)

gnprice · 2025-09-04T20:30:36Z

test/notifications/open_test.dart

      await tester.pump();
      // The navigator first pushes the starting routes…
-      takeStartingRoutes();
+      takeChooseAccountPageRoute(); // (no lastVisitedAccountId in this test)


The last/main commit changes the behavior of this line:

// The navigator first pushes the starting routes… - takeHomePageRouteForAccount(eg.selfAccount.id); // because first in list + takeChooseAccountPageRoute(); // (no lastVisitedAccountId in this test)

Does that correspond to an intended change in how opening notifications works? (I.e., in how the code this test is testing works.)

I think it doesn't. So it'd be better to leave the test's behavior unchanged in this commit — otherwise, the test change looks like it's saying something changed in the app's real behavior.

I believe that can be accomplished by just not adding markLastVisited: false above.

Makes sense. I think we'd just want to change the comment from "because first in list" to "because last-visited"; the behavior stays the same but the for it reason changes.

gnprice · 2025-09-04T20:35:37Z

test/widgets/app_test.dart

-      // We'll need per-account data for the account that a page will be opened
-      // for, but not for the other account.


It's still the case that we don't need per-account data for the account that we're not opening a page for, right? So it'd be good to preserve this aspect, using insertAccount rather than add. That keeps the test a bit stronger: it would fail if ZulipApp accidentally started trying to load per-account data for all the accounts.

gnprice · 2025-09-04T20:36:55Z

test/widgets/app_test.dart

+      testWidgets('with account(s) visited, go to home page for the last visited account', (tester) async {
+        await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot());
+        await testBinding.globalStore.add(eg.otherAccount, eg.initialSnapshot(
+          realmUsers: [eg.otherUser]));
+        check(testBinding.globalStore).lastVisitedAccount.equals(eg.otherAccount);


This test would also be a bit stronger if the account to be opened was neither the first nor the last in the list of accounts. As is, it would pass if we accidentally started just opening the last account in the list.

See discussion: zulip#1784 (comment)

This will result in better organization when more tests are added for another type of settings (IntGlobalSettings) in the following commits.

See discussion: zulip#1784 (comment)

…ations

This `init` function has only one caller, `prepare` (it could probably be inlined?); simplify by removing this param and the code inside that was never being exercised.

See discussion: zulip#1784 (comment)

sm-sayedi · 2025-09-05T09:12:43Z

Thanks for the review. Pushed new revision.

Fixes: zulip#524 [chris: rebased atop some notification-test refactors in the last few commits] Co-authored-by: Chris Bobbe <[email protected]>

gnprice · 2025-09-05T22:52:21Z

Thanks again for all your work on this! Looks good; merging, with one commit-message tweak:

-    example_data: Add thirdAccount
+    test: Add eg.thirdAccount

sm-sayedi force-pushed the 524-launch-last-account branch 9 times, most recently from db70e92 to 52a8954 Compare August 9, 2025 19:37

sm-sayedi marked this pull request as ready for review August 9, 2025 19:41

sm-sayedi added the maintainer review PR ready for review by Zulip maintainers label Aug 9, 2025

sm-sayedi requested a review from chrisbobbe August 9, 2025 19:41

sm-sayedi assigned chrisbobbe Aug 9, 2025

sm-sayedi force-pushed the 524-launch-last-account branch 2 times, most recently from fa65075 to e68d2fb Compare August 10, 2025 04:25

chrisbobbe reviewed Aug 12, 2025

View reviewed changes

sm-sayedi force-pushed the 524-launch-last-account branch from e68d2fb to e29b7f0 Compare August 13, 2025 07:53

sm-sayedi requested a review from chrisbobbe August 13, 2025 07:56

sm-sayedi force-pushed the 524-launch-last-account branch from e29b7f0 to 1b742f3 Compare August 13, 2025 18:33

chrisbobbe reviewed Aug 15, 2025

View reviewed changes

sm-sayedi force-pushed the 524-launch-last-account branch from 1b742f3 to 52d0901 Compare August 15, 2025 14:25

sm-sayedi requested a review from chrisbobbe August 15, 2025 14:27

sm-sayedi force-pushed the 524-launch-last-account branch from 52d0901 to 3a6393d Compare August 15, 2025 17:09

chrisbobbe reviewed Aug 18, 2025

View reviewed changes

sm-sayedi force-pushed the 524-launch-last-account branch 2 times, most recently from 94ba26c to 39ab202 Compare August 18, 2025 20:40

chrisbobbe removed the maintainer review PR ready for review by Zulip maintainers label Sep 3, 2025

gnprice reviewed Sep 4, 2025

View reviewed changes

sm-sayedi pushed a commit to sm-sayedi/zulip-flutter that referenced this pull request Sep 4, 2025

notif test: Decompose takeStartingRoutes

f13e07b

See discussion: zulip#1784 (comment)

sm-sayedi force-pushed the 524-launch-last-account branch from 2abe893 to f3edee3 Compare September 4, 2025 17:01

gnprice reviewed Sep 4, 2025

View reviewed changes

sm-sayedi assigned gnprice and unassigned gnprice Sep 5, 2025

sm-sayedi pushed a commit to sm-sayedi/zulip-flutter that referenced this pull request Sep 5, 2025

notif test: Decompose takeStartingRoutes

6a41239

See discussion: zulip#1784 (comment)

sm-sayedi force-pushed the 524-launch-last-account branch from f3edee3 to c41a53f Compare September 5, 2025 07:52

sm-sayedi added 2 commits September 5, 2025 13:11

db test [nfc]: Factor out BoolGlobalSettings tests in its own group

44c0ce3

This will result in better organization when more tests are added for another type of settings (IntGlobalSettings) in the following commits.

settings: Avoid redundant updates in setBool

ed75f19

sm-sayedi pushed a commit to sm-sayedi/zulip-flutter that referenced this pull request Sep 5, 2025

notif test: Decompose takeStartingRoutes

a22bb7b

See discussion: zulip#1784 (comment)

sm-sayedi force-pushed the 524-launch-last-account branch from c41a53f to 89e2c92 Compare September 5, 2025 08:42

sm-sayedi and others added 4 commits September 5, 2025 13:14

settings: Make general int global settings, so as to add without migr…

331ed03

…ations

notif test [nfc]: Remove a param that's effectively never used

f1a238c

This `init` function has only one caller, `prepare` (it could probably be inlined?); simplify by removing this param and the code inside that was never being exercised.

notif test: Decompose takeStartingRoutes

f1badd4

See discussion: zulip#1784 (comment)

settings: Add IntGlobalSetting.lastVisitedAccountId, and keep updated

c569596

sm-sayedi force-pushed the 524-launch-last-account branch 2 times, most recently from af11330 to cdc234a Compare September 5, 2025 09:09

sm-sayedi requested a review from gnprice September 5, 2025 09:13

sm-sayedi and others added 2 commits September 5, 2025 15:51

test: Add eg.thirdAccount

a26a469

app: On launch, go to the last visited account

a9137f2

Fixes: zulip#524 [chris: rebased atop some notification-test refactors in the last few commits] Co-authored-by: Chris Bobbe <[email protected]>

gnprice force-pushed the 524-launch-last-account branch from cdc234a to a9137f2 Compare September 5, 2025 22:52

gnprice merged commit a9137f2 into zulip:main Sep 5, 2025

rajveermalviya mentioned this pull request Sep 9, 2025

Remember active account when app is closed #1845

Closed

		final (actionButton, _) = await prepare(tester,
		accounts: [eg.selfAccount], logoutAccount: eg.selfAccount);

		await testBinding.globalStore.settings
		.setInt(IntGlobalSetting.lastVisitedAccountId, eg.selfAccount.id);

		if (lastAccountMissing)
		MaterialWidgetRoute(page: const ChooseAccountPage())

	check(testBinding.globalStore).lastVisitedAccount.isNull();
	check(testBinding.globalStore).lastVisitedAccount.isNull();
	check(testBinding.globalStore).accounts.isNotEmpty();

		// We'll need per-account data for the account that a page will be opened
		// for, but not for the other account.

On launch, go to the last visited account #1784

On launch, go to the last visited account #1784

Uh oh!

Conversation

sm-sayedi commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Aug 8, 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!

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!

sm-sayedi commented Aug 13, 2025

Uh oh!

sm-sayedi commented Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

sm-sayedi commented Aug 15, 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!

sm-sayedi commented Aug 18, 2025

Uh oh!

chrisbobbe commented Sep 3, 2025

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!

sm-sayedi commented Aug 5, 2025 •

edited

Loading

chrisbobbe Aug 8, 2025 •

edited

Loading

sm-sayedi commented Aug 13, 2025 •

edited

Loading