for #24665: Show recent synced placeholders early

[MatthewTighe]

for #24665

Separate commits since the first is technically unrelated, in case we want to file it for a different issue. I could also squash these if a single commit makes sense. During investigation I thought to maybe reuse fxaHasSyncedItems, but then I noticed that it was actually unused.

To download an APK when reviewing a PR:

1. click on Show All Checks,
2. click Details next to "Taskcluster (pull_request)" after it appears and then finishes with a green checkmark,
3. click on the "Fenix - assemble" task, then click "Run Artifacts".
4. the APK links should be on the left side of the screen, named for each CPU architecture

[gabrielluong]

Instead of having an isSyncing variable, could we check store.state.recentSyncedTabState to see if it's loading? Not saying this is necessarily the correct thing to do, but in prior reviews I had an inkling that maybe recentSyncedTabState should be tri-state with stopLoading implemented.

[MatthewTighe]

I'll update it to inspect the store's state, but would you mind clarifying about the tri-state? Maybe it's time to revisit that since we've already talked about it at least once.

Is the idea to introduce an error state? From the consumer's perspective, there isn't (currently) a desire to display error messages, which is why I've been defaulting to NONE on error.

[gabrielluong]

It might be something more like START( or NONE), LOADING, DONE. I don't know if this will necessarily be correct, but instead of only having NONE, we need to have a state that differentiates between whether or not loading was attempted.

[MatthewTighe]

There currently are three states that I think map pretty well to those: NONE, LOADING, and SUCCESS. Switching SUCCESS to a DONE state that had either successes or failures could potentially be an improvement, but I'm not sure what it benefits over the current implementation.

Honestly I think the underlying Controller implementation's usage of stopLoading is generally a bit confusing. The only case it's ever really useful IMO is the one where an account isn't authenticated, but refreshSyncedTabs shouldn't be called unless an account is authenticated anyway.

[MozillaNoah]

Using Settings/Preferences to monitor the authentication state seems a little sus. Were there other approaches you tried that didn't pan out?

[MatthewTighe]

The underlying issue is that auth is (relatively) slow, to the point where there will be noticeable pop-in of the recent synced tab if we wait for it to finish. To me that demonstrates that we need some kind of cache mechanism so that we can show the placeholders immediately on app startup. If you have any other suggestions, I'm definitely all ears. Is it specifically using Settings? Do we have another easy file abstraction that I've so far missed?

One alternative I can think of off the top of my head is to cache it in the FxaAccountManager itself, but that would require adding a storage layer there as well and would ultimately be the same type of implementation.

[MozillaNoah]

(for the purposes of this PR, this can still land)

I was just curious if you looked into having this live in AppState instead. That way it could be observed in other places via appState.observeAsComposableState in case other areas of the app can easily consume this in Compose, if necessary in the future.

[MatthewTighe]

I filed #24911 to move it into app state, but the more I think about it the more I wonder whether it's actually the right move. Here's some of my thoughts:

• Stores are generally meant to be an observable entity. Any component would need to observe changes to hasFxaAuthenticated can also observe changes to auth status directly from an FxaAccountManager.
• hasFxaAuthenticated is really meant as a one-time read value on startup
• Even if we move it into the store, it will still need to be provided from settings or some other kind of cache mechanism, so I think we'd just be creating a level of indirection

[MozillaNoah]

I think those concerns/points are fair

[MatthewTighe]

I closed it for now, then. Let me know if want to do something different in the future, happy to revisit this again.

[jonalmeida]

I would have preferred we didn't land this. Using settings isn't a great idea because we're duplicating and persisting the same state to multiple places.

We also have other account state holders, so we've added yet another mechanism to do the same/similar thing.

We should add the account status into the AppStore with a better abstraction that holds the Sync state (e.g. an account data class with an enum and maybe some other attributes) - this was one of the original applications for adding the AppStore. It removes the need to know how the account manager works for features that just want to know what is current state at this point in time, or observe it which idioms that we've already created with the Store model.

Stores are generally meant to be an observable entity.

This is true, but we can also "peek" at the current state in the store if it's a safe use-case for it.

Even if we move it into the store, it will still need to be provided from settings or some other kind of cache mechanism, so I think we'd just be creating a level of indirection

We're currently fetching the account state remotely, so another caching layer would be an in-memory solution which the AppStore would suffice? If you mean a secondary persistent caching, it has risks and benefits there which we can discuss separately.

[MatthewTighe]

@jonalmeida Sorry about that, I'll try to be more patient in the future when waiting for reviews. I'm happy to back out that particular part of the change in favor of something else.

Using settings isn't a great idea because we're duplicating and persisting the same state to multiple places.

Could you point me to where the auth state of an account specifically is persisted? That's the key requirement. The goal of this issue was to avoid pop-in on the home screen on app startup. Since there is a significant delay in account authentication, the only way to do that is with a disk cache.

[jonalmeida]

The goal of this issue was to avoid pop-in on the home screen on app startup. Since there is a significant delay in account authentication, the only way to do that is with a disk cache.

This is the same problem that we encounter on the home screen today with our other sources of data, so we could still use the same solution: the first cold start will be slow, we store it in the AppStore, when we return to the home screen again, we will already have it in memory. Why is the Synced tab a higher priority than the others?

We can also go the persistent cache route, so let's walk backwards to figure out how account manager persists it's state today and see what our options are. Writing out my learnings here as I walk through the code:

1. If we look at the FxaAccountManager.kt we need to find a reference to some kind of persistence.
2. There is one! accountOnDisk looks like what we want, and that leads to the StorageWrapper where the docs hint that we are in the right direction: "Knows how to read account from disk (or creating a new instance if there's no account), registering necessary watchers."
3. If we keep following along, the StatePersistenceCallback.persist(String) is a JSON object as a string.
4. The JSON object is created from an instance of FirefoxAccount which calls toJson that lives in... app services! This is interesting and good, which means Android-land isn't duplicating the account state persistence in a different way from other platforms (iOS/Desktop), so there is a good chance we have what we need for knowing the account is already an authenticated account (🤞 ).
5. So the inner class that holds the data is FirefoxAccount which is a generated class from app services' uniffi framework, which is what they use to create language bindings for each platform. So let's look at the fxa_client.udl that holds the definition of the class. From there, we can see that they persist the account name, email and avatar in a dictionary.
6. Coming back to what you want, is to know if the account was already authenticated sooner (even though there is a chance it may not always be that way when we check with the servers), I would ask the app services' team what they think is the best way to add the state needed to know if an account was recently authenticated to make start-up faster and accordingly ask for their support (or write some rust!) before proceeding to add it into the Android FxaAccountManager. What they might tell us is that we are fine with the drawbacks, we could write our own method to check for the existence of some other state in the JSON object which we can use to stay that an account was authenticated at some point.

Why go this route? If we start storing loosely maintained state in Fenix, we could start putting clients at risk of incorrectly saying they are (not) authenticated, which over time builds up a painful kind of tech debt. :)

[MatthewTighe]

Thanks for the detailed walkthrough! I'm still working on growing the skill of navigating our stack, so I appreciate the breakdown and reminder to think more about what belongs where. I posted some more thoughts below. Next week I'll keep taking a look into it, and figure out the best way to back out the change. This work is currently hidden (even from nightly) behind a flag, so it shouldn't have any real negative impact.

This is the same problem that we encounter on the home screen today with our other sources of data

I think the biggest difference is just source of the data. AFAIK, most of that other app store data is coming from local sources. In any case, the previous implementation of the synced tab was causing the loading placeholder to appear a full few seconds after the rest of the screen had populated.

Coming back to what you want, is to know if the account was already authenticated sooner (even though there is a chance it may not always be that way when we check with the servers)

Just to add some detail about expected UX as a result of that: we decided that it would be better for the placeholder to disappear in the case that it was shown before an auth failure. The thinking being that pop-in was more visually disruptive than the pop-out.

I'm initially wondering if exposing whether accountOnDisk is restored or not would work. I need to look more into whether it would cause harmful side-effects to inspect it on instantiation instead of during the normal processing of the state machine.

I'm not sure I'm following yet why it would be required to add additional state at the A-S layer. I would've thought that checking the existence of any state there would get us what we were looking for.

[MatthewTighe]

We should add the account status into the AppStore with a better abstraction that holds the Sync state

I'm wondering if we should possible take this a step further. I see additional benefits to introducing the sync state and data to BrowsterStore (perhaps even a SyncStore) at the A-C level. Any reductions in complexity we would see Fenix-side could be mirrored in existing features and usecases in A-C. In general, having a simpler abstraction around the FxA manager and various sync workers seems useful.

[jonalmeida]

Just to add some detail about expected UX as a result of that: we decided that it would be better for the placeholder to disappear in the case that it was shown before an auth failure. The thinking being that pop-in was more visually disruptive than the pop-out.

How often does the auth failure happen that we should prioritize the visual disruption? With the solution in this PR, you would still run into this case regardless so we are still not covered there; we need to wait for the Sync servers to get back and tell us that the auth token we're trying to use now is no longer valid. Our best effort might just be to live with the edge case of an auth failure from a user that hasn't used Fenix in a while so they see a brief visual error. See below how we can still solve your problem with the help of the A-S team's guidance.

I'm initially wondering if exposing whether accountOnDisk is restored or not would work. I need to look more into whether it would cause harmful side-effects to inspect it on instantiation instead of during the normal processing of the state machine.

This is a good question for the A-S team. They would be able to tell us exactly what is the right attribute to observe in order to consider the account authenticated at some point.

I'm not sure I'm following yet why it would be required to add additional state at the A-S layer. I would've thought that checking the existence of any state there would get us what we were looking for.

The accountOnDisk is a JSON object, so you could have that object exist without ever having an account authenticated within it. Or even a shell JSON object may exist without an account if the FxaAccountManager is only instantiated - from what I can read in the code here and the related bits, a piece of code could call accountManager.account() to test if an account is created and inadvertently create that shell JSON object on file.
I'm not really sure, so it would be good to ask the A-S folks what is the right thing to do and we can evaluate from there if we should consider the existence of the object on disk or a particular attribute within the object.

I'm wondering if we should possible take this a step further. I see additional benefits to introducing the sync state and data to BrowsterStore (perhaps even a SyncStore) at the A-C level. Any reductions in complexity we would see Fenix-side could be mirrored in existing features and usecases in A-C. In general, having a simpler abstraction around the FxA manager and various sync workers seems useful.

Oh yes, 100%. Moving the state to the AC level was an original ask. It doesn't necessarily mean that we need to replace the current API immediately. Rather, provide a nicer API for consumers to migrate towards in addition to the current API, and then we have more freedom to alter the internals of the manager (and re-design the observer pattern it uses or even remove it).

I don't have strong opinions towards whether it should be a separate store or within the BrowserStore, but I would lean towards a separate store mostly because, if you look at the internals of the account manager, it's basically a state machine and that's really the same thing we get with lib-state. So we could eventually replace the custom state machine in the future (historically, the account manager existed before lib-state, which is why it rolled it's own version) - this might be a good question to get @csadilek's opinion on as well before deciding to tackle it.

[csadilek]

I am leaning towards a separate store as well. This isn't state needed for core browsing functionality, and from the perspective of Focus, why would this empty state be in the browser state? I think it also aligns better with our module separation and composable design e.g., to only bring in state as needed by the application.

[MatthewTighe]

I filed A-C 12065 to track the work of adding a sync store. It's work I'm very interested in taking on, but may need to wait until more progress has been made on various items planned for H1. At the latest, I'll plan to push for it to be included in H2.

Additionally, it looks like my understanding of the issue this PR was intended to solve was just completely wrong. I've continued tinkering with it on the A-C and Fenix side, and it appears that auth state will be available as soon as other data from the homepage is. I'll have PRs up today to revert the change introduced here and still achieve desired behavior. Thanks for everyone's patience in engaging in the conversation, as it was very educational. That said, I don't think there will be any need to engage A-S for improvements and any required changes can be handled on our end.