home: Stop assuming account existence from loading page

[PIG208]

We could pass realmUrl when initializing the _LoadingPlaceholderPage, but that will still require a check for the existence of the account.

The loading page will be blank when the account does not exist. The user can't easily reach this page because they can only logout from ChooseAccountPage, until we start invalidating API keys. Even then, they will only see the blank page briefly before getting navigated, so we should not show any text at all.

Screenshot

Fixes: #1219

[PIG208]

Not sure if we still show the loading indicator there, though. This page can be visible for a frame only after #737, but if we don't have the realmUrl, there will potentially be a layout shift as the circular indicator's position is affected by the hidden text.

Because realmUrl probably stays the same for the same accountId, an alternative fix would be caching the realm URL when we first learn about the account, and pass that down. This requires us to know for sure that the account exists at some point, unless we allow the realmUrl to be null for the loading page.

[gnprice]

Not sure if we still show the loading indicator there, though. This page can be visible for a frame only after #737, but if we don't have the realmUrl, there will potentially be a layout shift as the circular indicator's position is affected by the hidden text.

Yeah, that's a fine reason to leave out the loading indicator. It's probably less jarring to have it disappear while the page is animating out than to have it suddenly shift while the page is animating out.

Note it's more than a frame — the animation duration for leaving a page is more like 200ms.

[chrisbobbe]

LGTM, thanks! Marking for Greg's review.

[gnprice]

Thanks! The fix looks good. A couple of comments below on the test.

[gnprice]

What's the role of loadPerAccountDuration here? It doesn't look like anything else in the test refers to it.

[PIG208]

Yeah it is not needed.

[gnprice]

This test feels like it could easily stop having any effect if something otherwise unrelated changes about the timing of the steps here. It could end up not getting to the loading placeholder at all — or going past it, to the normal contents of HomePage.

How about having it check that it can find a widget with text containing, say, "taking a while"? That should be pretty identifying of the loading page.

[PIG208]

We can't check for "taking a while" because the text cannot be rendered without the realmUrl. The fix replaced the page with SizedBox.shrink() which doesn't really stand out as a widget to look for. I ended up adding CircularProgressIndicator back instead. It might result in a layout shift as mentioned earlier.

[gnprice]

Making the test easier to write is definitely not a good reason to change the actual UX in a way we previously decided we didn't like. 🙂

The SizedBox.shrink() only comes after logging out, right? The step I'm asking for above is to check that we got to the loading placeholder page in the first place, right before logging the user out.

[PIG208]

Hmm, I see. Previously the account would finish loading before getting logged out. This setup relies on the behavior that after logging out, the app is briefly brought back to the loading page after removing the PerAccountStore and Account, and stays there until the route is popped from the navigator.

By removing the extra pump, checking it before logging out works.

Looking for another way to check for the alternative loading placeholder page after logging out, I found that we might do something like this without modifying the UX:

      // Missing AppBar title currently only applies to the loading pages,
      // but the check is still somewhat brittle.
      check(tester.widget<AppBar>(find.byType(AppBar))).title.isNull();

      check(tester.widget<SizedBox>(find.byType(SizedBox)))
        ..width.equals(0)
        ..height.equals(0);

For now, I'm pushing a revision that checks for the loading placeholder page before logging out, with the UX restored.

[gnprice]

I see. Rereading #1219, it's really about what happens after the normal contents of HomePage have already loaded, and the account subsequently gets logged out. The involvement of _LoadingPlaceholderPage is really just a detail of how the bug happened — there's no reason there needs to be a "loading placeholder" involved in the process of removing an account.

OK, so in order to function as a regression test for that issue, the test should exercise that situation: the data fully loads, and then the account gets logged out. I think that's what an earlier revision of this PR (before last week) did.

But because the issue is about HomePage, not _LoadingPlaceholderPage, there's no need to have it check that a loading page is involved at that point… and it should go outside of the _LoadingPlaceholderPage test group. I think the test group is what confused me earlier — it's a way of saying the test is about _LoadingPlaceholderPage, so I noticed the test didn't really ensure _LoadingPlaceholderPage was involved in what it tested and that seemed like a problem.

In particular, consider what should happen if in the future we change the behavior of HomePage (perhaps via MaterialAccountWidgetRoute) so that when the account gets logged out, the loadingPlaceholderPage doesn't get built — instead it's just an empty page or something. This test will still be doing a useful job by checking that when the user gets logged out from HomePage, there's no crash — effectively checking that that new arrangement doesn't reintroduce this bug. But there's no need for it to complain about the fact that _LoadingPlaceholderPage is no longer present.

This new version of the test, where the loading page is still in place when the user gets logged out, is potentially useful too — so we might as well keep it, in addition to restoring the test that's a more direct regression test for #1219.

[PIG208]

Thanks for the review! Updated the PR by restoring the old version of this test, with a minor tweak that checks that HomePage is fully loaded, and removed the check for loading page after logging out. The new version of the test is kept too.