store: Handle invalid API key on register-queue #1183

PIG208 · 2024-12-19T22:04:29Z

This is near term fix for a user-reported issue:
https://chat.zulip.org/#narrow/channel/48-mobile/topic/0.2E0.2E19.20Flutter.20.3A.20Cant.20connect.20to.20self.20hosted.20instance/near/2004042

It is not intended to be the full fix. With a better UX, we would bring the user back to the choose-account page without them manually doing so. That's covered by #737 but out-of-scope for this commit.

This cherry-picks #1235, and partially addresses #890, by handling INVALID_API_KEY errors from the PerAccountStoreWidget.

Fixes:

Handle invalid API key on register-queue #737

gnprice · 2024-12-20T00:44:48Z

Thanks!

Would you post a screenshot of what the error looks like? Then we can also wordsmith the error message.

PIG208 · 2024-12-20T00:53:45Z

Sure!

gnprice · 2024-12-20T01:09:56Z

Cool, thanks.

I think "Invalid API key" is probably too technical for this context. @alya do we have an appropriate message handy from elsewhere in the product?

PIG208 · 2024-12-21T00:30:47Z

Perhaps we can rename and reuse the title string we have for login failure:

  "errorLoginCouldNotConnectTitle": "Could not connect",

~~This sort of feels like an instance of #741 the way we currently handle it, but this message will not be needed after #737.~~

screenshot

PIG208 · 2025-01-02T13:32:40Z

Updated the PR to implement Chris' suggestion here. We now handle invalid API key by calling logOutAccount, which removes the routes associated with the affected account, pushing a choose-account page route onto the stack if it becomes empty, and showing an error dialog.

chrisbobbe

Thanks! Here's a review of the first five commits:

06c5543 l10n [nfc]: Use a generalize name for errorCouldNotConnectTitle
d27b46c log [nfc]: Rename ReportErrorCallback to ReportErrorCancellablyCallback
c5fc2d9 example_data: Start generating account id
652c332 home: Stop assuming account existence from loading page
ab91480 log: Add reportErrorModally

which leaves the last three that I haven't read yet:

58fd4b1 store: Handle invalid API key on register-queue
7a632b2 display test [nfc]: Make openNotification public
2e0bcb9 actions test: Use a realistic example

For the last commit, it's the one with the Fixes: line but it doesn't actually change any app code, it only touches test code. Is that intentional? I'd like to understand if/how these test changes are related to your app-code changes for #737.

chrisbobbe · 2025-01-02T22:05:23Z

assets/l10n/app_en.arb

-  "errorLoginCouldNotConnectTitle": "Could not connect",
-  "@errorLoginCouldNotConnectTitle": {
+  "errorCouldNotConnectTitle": "Could not connect",
+  "@errorCouldNotConnectTitle": {


l10n [nfc]: Use a generalize name for errorCouldNotConnectTitle

commit-message nit: "generalized"

chrisbobbe · 2025-01-02T22:08:07Z

lib/widgets/home.dart

+    if (account == null) {
+      // We should only reach this state very briefly.
+      // See [_LoadingPlaceholderPage.accountId].
+      return Scaffold(
+        appBar: AppBar(),
+        body: const SizedBox.shrink());
+    }


This feels like it duplicates more details than it needs to, like the appBar param and the whole set of Scaffold params that aren't passed. How about applying the condition more precisely to the things that are supposed to be different?

chrisbobbe · 2025-01-02T22:16:39Z

lib/log.dart

+// This gets set in [ZulipApp].  We need this indirection to keep `lib/log.dart`
+// from importing widget code, because the file is a dependency for the rest of
+// the app.
+ReportErrorCallback reportErrorToUserModally = defaultReportErrorToUserBriefly;


Setting reportErrorToUserModally to defaultReportErrorToUserBriefly looks odd. How about renaming defaultReportErrorToUserBriefly to something that seems like an appropriate value for both reportErrorToUserBriefly and reportErrorToUserModally?

Maybe dumpErrorToConsole? That name makes it sound like a potential problem if it actually gets called (errors should be reported, not dropped on the floor 🙂)—but that's actually appropriate in this case.

Good idea! Renaming to reportErrorToConsole because we don't usually dump the error object, which I feel is what "dump" is for.

chrisbobbe · 2025-01-02T22:18:28Z

test/widgets/app_test.dart

+      reportErrorToUserModally(message, details: details);
+      check(ZulipApp.ready).value.isFalse();
+      await tester.pump();
+      check(find.byType(AlertDialog)).findsNothing();


I have a commit

test [nfc]: Factor out checkNoErrorDialog helper

in my revision for #1239; how about cherry-picking that as a prep commit and using it here? (See reasoning in the commit)

PIG208 · 2025-01-03T02:17:52Z

The last commit "fixes" the issue because it resolves a TODO comment for the issue. With #737 we can set up the test in a scenario where the API key is invalidated.

chrisbobbe

Thanks! Comments below.

chrisbobbe · 2025-01-03T16:53:20Z

assets/l10n/app_en.arb

@@ -458,6 +458,13 @@
  "@topicValidationErrorMandatoryButEmpty": {
    "description": "Topic validation error when topic is required but was empty."
  },
+  "errorInvalidApiKeyMessage": "Your account at {url} cannot be authenticated. Please try again or use another account.",


I think my comment in CZO still applies:

[…] the message looks like it can't be literally true. If an action really "cannot" be done, then trying the same action again won't work.

Or, from another angle: the server hasn't said that the account can't be authenticated, so we shouldn't tell the user that. That would be a pretty pathological case: I guess in theory it would be accurate if the database were corrupted in a way that made it impossible to know if any API key was valid.

What we know from the INVALID_API_KEY error is that the current attempt to authenticate failed, because we used the wrong API key. Here's a proposal:

Your account at {url} could not be authenticated. Please try logging in again or use another account.

cc @alya for thoughts on this message.

chrisbobbe · 2025-01-03T17:22:18Z

lib/widgets/app.dart

+  /// Navigate to [ChooseAccountPage], ensuring that its route is at the root level.
+  static void navigate(BuildContext context) {
+    final navigator = Navigator.of(context);
+    navigator.popUntil((route) => route.isFirst);
+    unawaited(navigator.pushReplacement(
+      MaterialWidgetRoute(page: const ChooseAccountPage())));
+  }


It looks like this isn't used anywhere.

chrisbobbe · 2025-01-03T21:20:55Z

lib/widgets/store.dart

+            if (!navigator.canPop()) {
+              // This ensures that the navigator stack is non-empty after the
+              // removal of the route.
+              unawaited(navigator.push(
+                MaterialWidgetRoute(page: const ChooseAccountPage())));
+            }


In my manual testing, I'm getting an extra ChooseAccountPage in the nav stack. Do you reproduce? Possibly it's only happening when I have more than one PerAccountStoreWidget mounted for the logged-out account. I think probably what's happening is you expected NavigatorState.canPop() to return false only when the nav stack is empty, but it's returning false when the nav stack consists of one route.

Yeah, a check for both isFirst and isCurrent would be more appropriate. I think this also means that the current test does not cover this case correctly, will fix that too.

It turned out that

Yeah, a check for both isFirst and isCurrent would be more appropriate.

is not accurate.

There can be routes that are not removed on logout but are not helpful either for the user to navigate to a choose-account page. DialogRoute is an example of that. So checks based on "if the navigator stack is empty" will likely not work. We need a solution that guarantees a choose-account page route at the root level, and this is probably not the right place to do that.

We now implement a NavigatorObserver based solution. It implements didPop and didRemove to push a choose-account page route proactively if it observes that the navigator stack is becoming empty.

chrisbobbe · 2025-01-03T21:58:21Z

lib/widgets/store.dart

+              // The API key is invalid and the store can never be loaded
+              // unless the user retries manually.
+              if (!mounted) return;
+              final zulipLocalizations = ZulipLocalizations.of(context);
+              reportErrorToUserModally(
+                zulipLocalizations.errorCouldNotConnectTitle,
+                details: zulipLocalizations.errorInvalidApiKeyMessage(
+                  globalStore.getAccount(widget.accountId)!.realmUrl.toString()));
+              unawaited(logOutAccount(context, widget.accountId));
+              return;


All of this gets skipped if there's been a previously successful /register, because in that case the store != null path is taken (some lines above this).

So in this revision, in the refresh-expired-event-queue case (GlobalStore._reloadPerAccount), I think the uncaught error means the user gets stuck with a stale PerAccountStore with no active event queue, and they can't replace that store (because we take the store != null path) except by closing/opening the app, or I guess logging out and back in. We should handle the refresh-expired-event-queue case, with test coverage.

Can we put this reportErrorToUserModally(), the logOutAccount(), and the reset-navigation-state action (previous comment) somewhere that clearly gets invoked once (just once) on every /register / INVALID_API_KEY, including the one in _reloadPerAccount?

We also need tests to check that the account is in fact logged out / removed, and that some subtle related things are dealt with, like _perAccountStores and _perAccountStoresLoading, calling PerAccountStore.dispose, and notifying listeners. How about using TestGlobalStore.takeDoRemoveAccountCalls for this, because GlobalStore.removeAccount already has tests for those things.

For example (just for the first-/register case):

test('GlobalStore.perAccount on INVALID_API_KEY', () => awaitFakeAsync((async) async { addTearDown(testBinding.reset); await testBinding.globalStore.insertAccount(eg.selfAccount.toCompanion(false)); testBinding.globalStore.loadPerAccountException = ZulipApiException( routeName: '/register', code: 'INVALID_API_KEY', httpStatus: 400, data: {}, message: ''); await check(testBinding.globalStore.perAccount(eg.selfAccount.id)) .throws<ZulipApiException>(); // fails check(testBinding.globalStore.takeDoRemoveAccountCalls()) .single.equals(eg.selfAccount.id); }));

(to go near the other 'GlobalStore.perAccount' tests)

I think the right place for calling logOutAccount and reportErrorModally should be loadPerAccount. We throw an AccountNotFoundException after processing the error, and make sure that all call-sites handles that correctly. _PerAccountStoreWidgetState already expects the exception.

For the other call-site, we need to handle it at least for one of _reloadPerAccount, UpdateMachine._handlePollError, or UpdateMachine.poll.

It doesn't seem right to catch it from _reloadPerAccount without rethrowing or at least indicating that the reload was a failure; _handlePollError's caller assume that the store is disposed after calling it (because of a reloaded PerAccountStore), so we should still either rethrow the error or indicate the failure reloading the store. So we probably should just catch it from poll, within another catch block.

Because logOutAccount is no longer purely a user action, we might want to revisit #1010 (comment) and figure out a new home to this. While GlobalStore is logically a good fit, I agree that lib/model/store.dart has become too crowded for us to add more to it. Maybe using mixins for store.dart would be a helpful prep/follow-up change.

chrisbobbe · 2025-01-03T22:13:13Z

test/widgets/store_test.dart

+    await tester.tap(find.text('Try another account'));
+    await tester.pump(); // tap the button
+    await tester.pump(const Duration(milliseconds: 250)); // wait for animation
+    check(find.byType(CircularProgressIndicator)).findsOne();


I don't yet understand this CircularProgressIndicator check.

Does the test expect it to be on a specific page? (meaning the check stops being as useful if another page adds a CircularProgressIndicator)

If we're expecting it on a non-active/foregrounded page, do we need to use skipOffstage: false?

It's looking for the route that is not on the top of the stack. I'm switching to a rewrite of this using NavigatorObserver's, so that we can check the navigator stack more directly.

chrisbobbe · 2025-01-03T22:43:04Z

test/widgets/actions_test.dart

-      // TODO(#737): switch to a realistic setup:
-      //   https://github.com/zulip/zulip-flutter/pull/1076#discussion_r1874124363


Reading that linked discussion and links from there, I'm not finding an explanation for why the TODO involves #737 at all. What Greg meant by not "realistic setup" is just that "These pages don't exist in real life", meaning two MaterialAccountWidgetRoutes with page InboxPageBody. Routes like that don't exist in the app because, since the bottom-tabs update, the Inbox page is never alone on its own route, it's always part of the route returned by HomePage.buildRoute.

The direct fix is to use realistic routes for account1Route and account2Route, e.g. by using MessageListPage.buildRoute instead, making follow-on changes to findAccount{1,2}PageContent, and renaming the makeUnreadTopicInInbox helper. Let's have a commit that just does that and removes the TODO, explaining in the commit message that it's not related to 737 after all.

If you'd like to go further in a follow-up commit (which I think is not a high priority), I'd have some feedback on that direction:

Take care to include skipOffstage: false where necessary when checking for something on a route that's not the top of the stack.

I intentionally put account1Route (the one to be removed) under account2Route in the stack, to guard against buggy implementations that might use something like NavigatorState.popUntil (which starts from the top and stops at the first route that shouldn't be removed, even if there are ones that should be removed under it). This would be a nice feature to keep, but I guess not essential.

Thanks for the detailed explanation! This has become #1257, but probably the refactor is not going to be a blocker for #737 so we can hold off reviewing that for now.

This rewrite ended being a bit more substantial than just switching both accounts initial routes to the result of MessageListPage.buildRoute(). We do not use popUtil because the navigator stack normally should not become empty, so the HomePage route for account1 stays. Additionally, because we are pushing a different page route, we no longer need to set up different messages as the discriminator, further simplifying the test. See also: zulip#1183 (comment) Signed-off-by: Zixuan James Li <[email protected]>

PIG208 · 2025-01-09T08:00:32Z

The PR has been updated to use a different approach maintaining the navigator stack. We are reusing a code path that throws AccountNotFoundException as a result of logging out the account. To ensure that the removal of a route does not end up with an empty navigator stack, we add a navigator observer to ZulipApp that pushes a choose-account page route when it becomes empty.

gnprice · 2025-01-16T05:30:18Z

lib/model/store.dart

@@ -19,6 +19,7 @@ import '../api/backoff.dart';
 import '../api/route/realm.dart';
 import '../log.dart';
 import '../notifications/receive.dart';
+import '../widgets/actions.dart';


Prompted to look at this by #1257 (comment) . As I said there just now, let's avoid importing widgets/ from models/.

As the lib/widgets/actions.dart file's dartdoc says:

/// Methods that act through the Zulip API and show feedback in the UI. /// /// The methods in this file can be thought of as higher-level wrappers for /// some of the Zulip API endpoint binding methods in `lib/api/route/`. /// But they don't belong in `lib/api/`, because they also interact with widgets /// in order to present success or error feedback to the user through the UI.

the point of that file being in widgets/ is that its functions use widgets to give the user feedback. Since logOutAccount and its callee unregisterToken don't do that, they can move elsewhere.

An easy solution is to move those two functions to a file lib/model/actions.dart, i.e. under model/.

I think ultimately the right organization is probably that these belong as methods on GlobalStore, after all the more local-storage-focused methods that also act on accounts.

(Then in the future, as a matter of code organization, all those accounts-oriented methods' implementations will live on an AccountStore or AccountsStore, but the way one calls them will still be via GlobalStore — much like the existing ChannelStore and MessageStore etc. help keep PerAccountStore organized.)

Echoing the last paragraph of #1183 (comment), I assume the reason that we don't move it to GlobalStore right away is to prevent cluttering the class before we carry out the code reorganization you mentioned. The plan of moving it to lib/model/actions.dart sounds good to me (along with the BuildContext parameter to GlobalStore refactor).

This rewrite ended being a bit more substantial than just switching both accounts initial routes to the result of MessageListPage.buildRoute(). We do not use popUtil because the navigator stack normally should not become empty, so the HomePage route for account1 stays. Additionally, because we are pushing a different page route, we no longer need to set up different messages as the discriminator, further simplifying the test. See also: zulip#1183 (comment) Signed-off-by: Zixuan James Li <[email protected]>

PIG208 · 2025-01-16T23:22:21Z

Updated the PR to resolve conflicts and move logOutAccount and unregisterToken to lib/model/actions. The relevant tests weren't moved. We can if we want. Those that can be moved require a bit more care to be refactored from testWidgets tests to test tests, but it might not be worth it at the moment.

chrisbobbe

Thanks! Comments below, including one where I got something unexpected in manual testing.

chrisbobbe · 2025-01-31T01:34:05Z

lib/log.dart

-void defaultReportErrorToUserBriefly(String? message, {String? details}) {
+/// Show the user a dismissable error message in a modal popup.
+///
+/// Typically this shows a [AlertDialog] containing the message.


nit: "an [AlertDialog]"

chrisbobbe · 2025-02-07T00:33:26Z

lib/model/store.dart

+          // The API key is invalid and the store can never be loaded
+          // unless the user retries manually.
+          final zulipLocalizations = GlobalLocalizations.zulipLocalizations;
+          final account = getAccount(accountId)!;


This will throw if there is no account, which can legitimately happen if the account is logged out during await doLoadPerAccount(accountId).

How about: get the realm URL before the await doLoadPerAccount(accountId), after the assert(_accounts.containsKey(accountId)), and store it in a variable?

Then also either skip the logOutAccount if getAccount(accountId) is null, or else investigate logOutAccount's TODO(log) for removal. (We won't want noise in Sentry from things that are expected to happen.)

chrisbobbe · 2025-02-07T00:56:35Z

lib/model/store.dart

    if (!_accounts.containsKey(accountId)) {
-      // [removeAccount] was called during [doLoadPerAccount].
-      store.dispose();
+      // [removeAccount] was called during or after [doLoadPerAccount].
+      store?.dispose();
      throw AccountNotFoundException();
    }
-    return store;
+    return store!;


This code gets harder to understand, since now it has to be read for both the success case and the INVALID_API_KEY case. How about leaving this code untouched and keep the INVALID_API_KEY case self-contained in its case in the catch? For that, I think we'd just need to throw AccountNotFoundException() at the end of that case; there's no store that we have to clean up.

chrisbobbe · 2025-02-07T01:09:02Z

lib/model/store.dart

+      } on AccountNotFoundException {
+        // Cannot recover by replacing the store because the account
+        // was logged out.
+        return;
+      }


Can we assert(_disposed) here too?

Also curious whether this is better to do here or in _handlePollError.

~~Cannot assert(_disposed) here because the reload might not have happened if there was an error (likely during _reloadPerAccount) that prevents the store from being disposed.~~

I chose to not handle it in _handlePollError because of the assert(_disposed) that follows the _handlePollError call. This way we also maintain that _handlePollError has the store replaced. An alternative would be returning a bool from _handlePollError, indicating if the store gets disposed.

It turned out that my claims aren't accurate. The store does get disposed: the account is logged out when loadPerAccount handles the error, and the store subsequently gets disposed. We just need to be clear that _handlePollError doesn't replace the store in this case.

Looking at _handlePollError, I think can indeed handle the error inside, by just having the try block contain the _reloadPerAccount call, since that's where the AccountNotFoundException might come from (the underlying endpoint is /register).

Having tested this further, there are two cases _handlePollError should be aware of:

The _reloadPerAccount call (POST /register) can fail due to an invalid API key. Handling this is necessary because otherwise it is a runtime error.

The Object error parameter, i.e. the error from polling (GET /events) can be a ZulipApiException with HTTP status code 401, indicating the invalid API key error.
Without handling it, it is marked as unexpected in the default branch under the switch, but we are still able to recover as long as we have error handling for the _reloadPerAccount call.

To handle the second case, we just need to acknowledge that such an error is expected and will be dealt with later when we try to replace the event queue.

Another way is to log out the account before trying to replace the event queue. Not sure which way is better.

Let's defer the handling of GET /events failure to #1054.

chrisbobbe · 2025-02-07T01:20:43Z

test/model/store_test.dart

@@ -843,6 +858,25 @@ void main() {
      check(store.debugMessageListViews).isEmpty();
    }));

+    test('log out if fail to reload on unexpected errors', () => awaitFakeAsync((async) async {


I think this test description should mention INVALID_API_KEY; how about:

unexpected poll error, but reload fails with INVALID_API_KEY; log out

chrisbobbe · 2025-02-07T02:08:37Z

lib/model/store.dart

        assert(debugLog('Backing off, then will retry…'));
-        // TODO tell user if initial-fetch errors persist, or look non-transient
+        // TODO(#890): tell user if initial-fetch errors persist, or look non-transient
+        switch (e) {
+          case ZulipApiException(code: 'INVALID_API_KEY'):
+            // We cannot recover from this error through retrying.
+            // Leave it to [GlobalStore.loadPerAccount].
+            rethrow;
+        }


The log line 'Backing off, then will retry…' becomes inaccurate in some cases; can you adjust so that doesn't happen?

chrisbobbe · 2025-02-07T02:14:17Z

lib/model/store.dart

-        // TODO tell user if initial-fetch errors persist, or look non-transient
+        // TODO(#890): tell user if initial-fetch errors persist, or look non-transient
+        switch (e) {
+          case ZulipApiException(code: 'INVALID_API_KEY'):


In manual testing on a dev server, where I changed an account's API key, the code I got was actually 'UNAUTHORIZED', not 'INVALID_API_KEY'. Have you seen 'INVALID_API_KEY' in manual testing? If you haven't done manual testing of the new functionality, please do that. 🙂

Oh, I see! Didn't realize that my prior testing was limited to throwing ZulipApiException from certain places, but not with a dev server. Will do that. Thanks for pointing it out! 🥲

PIG208 · 2025-02-11T03:22:50Z

The PR has been updated. Thanks for the review!

I haven't yet tested the reload-on-dead-queue case manually yet; did you do that too?

Do you mean the case when the event queue expires but the auth keys aren't valid? It works for me when I leave the app in the background after invalidating the API key.

chrisbobbe · 2025-02-11T20:45:08Z

Do you mean the case when the event queue expires but the auth keys aren't valid?

Yeah; (1) in your commit message:

The method loadPerAccount has two call sites, i.e. places
where we send register-queue requests:

1. _reloadPerAccount through [UpdateMachine._handlePollError]
2. perAccount through [PerAccountStoreWidget] (the common case)

Which I guess, thinking more about it, doesn't actually require the event queue expiring, right, because polling could fail for other reasons—including an invalid-API-key error :)

It works for me when I leave the app in the background after invalidating the API key.

Great!

PIG208 · 2025-02-11T20:53:11Z

Yeah, a GET /events invalid-API-key error is typically a precursor to the invalid-API-key from the _reloadPerAccount code path, among other non-transient errors.

chrisbobbe · 2025-02-11T20:59:35Z

Yeah, a GET /events invalid-API-key error is typically a precursor to the invalid-API-key from the _reloadPerAccount code path, among other non-transient errors.

Yeah, that makes sense.

If GET /events has an invalid-API-key error, am I reading correctly that this code in _handlePollError is run?:

      default:
        assert(debugLog('BUG: Unexpected error in event polling: $error\n' // TODO(log)
          'Replacing event queue…'));
        _reportToUserErrorConnectingToServer(error);
        // Similar story to the _EventHandlingException case;
        // separate only so that that other case can print more context.
        // The bug here could be in the server if it's an ApiRequestException,
        // but our remedy is the same either way.
        isUnexpected = true;

If so, that seems wrong; this is something we expect to happen, though rarely, and the _reportToUserErrorConnectingToServer looks like redundant UI feedback, because we'll give similar feedback when the replace-event-queue fails with the same error.

PIG208 · 2025-02-11T21:08:42Z

am I reading correctly that this code in _handlePollError is run

Yes. I was thinking of deferring that to #1054 (#1183 (comment)). For now, there is a TODO comment for this: 7334cca.

We can also handle this by assuming that this is expected, and leave to _reloadPerAccount to trigger that error again, but ideally we shouldn't need to call _reloadPerAccount if that ever happens, and we can leave this ideal version to #1054 instead.

This allows us to call it from model code when GlobalStore is available. Signed-off-by: Zixuan James Li <[email protected]>

This is more about testing the implementation of PerAccountStoreWidget to handle routeToRemoveOnLogout, instead of logOutAccount. Signed-off-by: Zixuan James Li <[email protected]>

plus moving and refactoring the tests to match Signed-off-by: Zixuan James Li <[email protected]>

chrisbobbe · 2025-02-11T21:52:47Z

Ah I see—thanks for reminding me of where you discussed this. :) I think deferring it to #1054 is fine.

chrisbobbe · 2025-02-11T21:52:59Z

So, LGTM; marking for Greg's review.

`PerAccountStore` shouldn't be an owner of the `MessageListView` objects. Its relationship to `MessageListView` is similar to that of `AutocompleteViewManager` to `MentionAutocompleteView` (zulip#645). With two owners, `MessageListView` can be disposed twice when `removeAccount` is called (when the user logs out, for example): 1. Before the frame is rendered, after removing the `PerAccountStore` from `GlobalStore`, `removeAccount` disposes the `PerAccountStore` , which disposes the `MessageListView` (via `MessageStoreImpl`). `removeAccount` also notifies the listeners of `GlobalStore`. At this point `_MessageListState` is not yet disposed. 2. Being dependent on `GlobalStore`, `PerAccountStoreWidget` is rebuilt. This time, the StatefulElement corresponding to the `MessageList` widget, is no longer in the element tree because `PerAccountStoreWidget` cannot find the account and builds a placeholder instead. 3. During finalization, `_MessageListState` tries to dispose the `MessageListView`, and fails to do so. We couldn't've kept `MessageStoreImpl.dispose` with an assertion `_messageListView.isEmpty`, because `PerAccountStore` is disposed before `_MessageListState.dispose` (and similarily `_MessageListState.onNewStore`) is called. Fixing that will be a future follow-up to this, as noted in the TODO comment. See discussion: https://chat.zulip.org/#narrow/channel/243-mobile-team/topic/MessageListView.20lifecycle/near/2083074 Signed-off-by: Zixuan James Li <[email protected]>

Signed-off-by: Zixuan James Li <[email protected]>

This highlights the API choice that the callback signature allows the caller to clear/cancel the reported errors, by passing `null` for the `message` parameter, drawing distinction from a later added variant that does not allow this. Signed-off-by: Zixuan James Li <[email protected]>

Signed-off-by: Zixuan James Li <[email protected]>

Coming up with a realistic test case doesn't actually require invalidating API key. Because the goal is to use routes that exist in the app (`InboxPageBody` has become a part of `HomePage` and doesn't exist on its own), we can set up HomePage and MessageListPage instead. Signed-off-by: Zixuan James Li <[email protected]>

Signed-off-by: Zixuan James Li <[email protected]>

The method loadPerAccount has two call sites, i.e. places where we send register-queue requests: 1. _reloadPerAccount through [UpdateMachine._handlePollError] 2. perAccount through [PerAccountStoreWidget] (the common case) We utilize the existing [AccountNotFoundException] because invalidating invalid auth keys effectively logs out the account, that it can no longer be found in the store. [PerAccountStoreWidget] already expects this error by ignoring it and assuming that the route will be removed: ```dart try { // If this succeeds, globalStore will notify listeners, and // [didChangeDependencies] will run again, this time in the // `store != null` case above. await globalStore.perAccount(widget.accountId); } on AccountNotFoundException { // The account was logged out while its store was loading. // This widget will be showing [placeholder] perpetually, // but that's OK as long as other code will be removing it from the UI // (usually by using [routeToRemoveOnLogout]). } ``` (included because unchanged code is not in the diff) To handle 1, we apply the same expectation that the account gets logged out when the exception happens. For `_handlePollError`, while the event queue was not replaced at all, the end result of having the old store disposed is still expected. This partly addresses zulip#890 by handling authentication errors for register-queue. Fixes: zulip#737 Signed-off-by: Zixuan James Li <[email protected]>

PIG208 requested a review from chrisbobbe December 19, 2024 22:04

PIG208 assigned chrisbobbe Dec 19, 2024

PIG208 added the maintainer review PR ready for review by Zulip maintainers label Dec 19, 2024

PIG208 force-pushed the pr-register branch 2 times, most recently from a43e320 to 4df9d15 Compare December 21, 2024 00:45

PIG208 force-pushed the pr-register branch from 4df9d15 to 2e0bcb9 Compare January 2, 2025 12:31

chrisbobbe reviewed Jan 2, 2025

View reviewed changes

PIG208 force-pushed the pr-register branch from 2e0bcb9 to 3b2d9ad Compare January 3, 2025 02:25

chrisbobbe requested changes Jan 3, 2025

View reviewed changes

PIG208 mentioned this pull request Jan 7, 2025

Rewrite logOutAccount actions test to use realistic routes #1257

Open

PIG208 force-pushed the pr-register branch from 3b2d9ad to 501aaa6 Compare January 9, 2025 07:57

gnprice reviewed Jan 16, 2025

View reviewed changes

PIG208 force-pushed the pr-register branch from 501aaa6 to acd5d3f Compare January 16, 2025 23:11

PIG208 requested a review from chrisbobbe January 21, 2025 16:23

PIG208 force-pushed the pr-register branch from acd5d3f to 7949a71 Compare January 27, 2025 20:59

chrisbobbe reviewed Feb 7, 2025

View reviewed changes

PIG208 force-pushed the pr-register branch 3 times, most recently from ee10619 to 2859759 Compare February 8, 2025 00:58

PIG208 mentioned this pull request Feb 8, 2025

store: Ensure sole ownership of MessageListView #1340

Open

PIG208 force-pushed the pr-register branch from 2859759 to 3f3a460 Compare February 11, 2025 03:21

PIG208 force-pushed the pr-register branch from 3f3a460 to 00eb740 Compare February 11, 2025 20:53

PIG208 added 3 commits February 11, 2025 16:13

login [nfc]: Pass GlobalStore to logOutAccount

a1c50e6

This allows us to call it from model code when GlobalStore is available. Signed-off-by: Zixuan James Li <[email protected]>

actions test [nfc]: Move a store-related test to a better home

2347aee

This is more about testing the implementation of PerAccountStoreWidget to handle routeToRemoveOnLogout, instead of logOutAccount. Signed-off-by: Zixuan James Li <[email protected]>

actions [nfc]: Move logOutAccount and unregisterToken under lib/model

2db40f4

plus moving and refactoring the tests to match Signed-off-by: Zixuan James Li <[email protected]>

chrisbobbe requested a review from gnprice February 11, 2025 21:53

chrisbobbe assigned gnprice and unassigned chrisbobbe Feb 11, 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 Feb 11, 2025

PIG208 force-pushed the pr-register branch 2 times, most recently from f72a133 to 6bd13ee Compare February 12, 2025 00:27

PIG208 added 10 commits February 11, 2025 20:03

l10n [nfc]: Use a generalized name for errorCouldNotConnectTitle

ea05ef1

Signed-off-by: Zixuan James Li <[email protected]>

log [nfc]: Rename default report error helper to reportErrorToConsole

3051b86

Signed-off-by: Zixuan James Li <[email protected]>

log: Add reportErrorModally

7415448

Signed-off-by: Zixuan James Li <[email protected]>

store [nfc]: Add TODO for handling auth errors when polling

b1153c1

Signed-off-by: Zixuan James Li <[email protected]>

store [nfc]: Mention issue number for register-queue feedback

92347e3

Signed-off-by: Zixuan James Li <[email protected]>

app: Maintain that the navigator stack is never empty

0329015

Signed-off-by: Zixuan James Li <[email protected]>

PIG208 force-pushed the pr-register branch from 6bd13ee to 19044a0 Compare February 12, 2025 01:05

		// TODO(#737): switch to a realistic setup:
		// https://github.com/zulip/zulip-flutter/pull/1076#discussion_r1874124363

store: Handle invalid API key on register-queue #1183

Are you sure you want to change the base?

store: Handle invalid API key on register-queue #1183

Conversation

PIG208 commented Dec 19, 2024 • edited Loading

gnprice commented Dec 20, 2024

PIG208 commented Dec 20, 2024 • edited by gnprice Loading

gnprice commented Dec 20, 2024

PIG208 commented Dec 21, 2024 • edited Loading

PIG208 commented Jan 2, 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

PIG208 Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 commented Jan 3, 2025 • edited Loading

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

PIG208 Jan 6, 2025 • edited Loading

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 Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

PIG208 Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 commented Jan 9, 2025

Choose a reason for hiding this comment

PIG208 Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

PIG208 commented Jan 16, 2025 • edited Loading

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

PIG208 Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

PIG208 Feb 7, 2025 • edited Loading

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

PIG208 commented Feb 11, 2025

chrisbobbe commented Feb 11, 2025

PIG208 commented Feb 11, 2025

chrisbobbe commented Feb 11, 2025 • edited Loading

PIG208 commented Feb 11, 2025 • edited Loading

chrisbobbe commented Feb 11, 2025

chrisbobbe commented Feb 11, 2025

PIG208 commented Dec 19, 2024 •

edited

Loading

PIG208 commented Dec 20, 2024 •

edited by gnprice

Loading

PIG208 commented Dec 21, 2024 •

edited

Loading

PIG208 Jan 3, 2025 •

edited

Loading

PIG208 commented Jan 3, 2025 •

edited

Loading

PIG208 Jan 6, 2025 •

edited

Loading

chrisbobbe Jan 3, 2025 •

edited

Loading

PIG208 Jan 6, 2025 •

edited

Loading

PIG208 Jan 9, 2025 •

edited

Loading

PIG208 Jan 16, 2025 •

edited

Loading

PIG208 commented Jan 16, 2025 •

edited

Loading

PIG208 Feb 7, 2025 •

edited

Loading

PIG208 Feb 7, 2025 •

edited

Loading

chrisbobbe commented Feb 11, 2025 •

edited

Loading

PIG208 commented Feb 11, 2025 •

edited

Loading