Fix quick connect

[tonyd33]

Purpose

Quick connect encountered a race condition where the task to monitor fails before the secret was properly fetched. This meant that even after authorization, nothing was monitoring for the authorization status and you'd have to close the quick connect menu and reopen it to authorize.

Additionally on iOS, after authorization, QuickConnectView uninitializes (as it should), but then reinitializes and uninitializes immediately after for some reason. This causes a strange state in UserSignInViewModel where an extra monitor task is spawned and keeps repeating forever.

XCode gives a "Publishing changes from background threads is not allowed..." in NavigationCoordinatable:600 after authorization, so I'm guessing it's something to do with the router.dismissCoordinator() in QuickConnectView, but wrapping that with MainActor.run didn't fix it.

Bug showcase

(Poll rate was increased here)

Fix

• Add quickConnectState property to properly block and signal to checkAuthStatus when we're ready to poll
  • It now stores the quick connect secret & code since we wait for authorization if and only if we have the secret & code
  • Display these states
• Add quickConnectMonitorTaskID property to enforce that only one monitor task should be running at a time, and a for a single source of truth for the task status.
  • Otherwise, letting the task recurse onto itself over and over with no oversight gets out of hand really easily.
  • Cancelling the task would be nice, but I've had issues with that
• Add quickConnectMaxRetries property as a failsafe. In case UserSignInViewModel does enter a weird state, we don't want to silently poll for authentication during the entire session.
• Move quick connect stuff to its own section, add docs for the quick connect lifecycle

Testing

• Check that authorization works after first time entering code
• Checked that polling no longer happens after authorization
• Checked that the new loading & error screens look fine

iOS

tvOS

Notes

I did try just using Task.cancel before the taskID approach, but it's been extremely unreliable and unpredictable for me. It didn't stop checkAuthStatus from spawning more tasks unless it was cancelled extremely early. I can elaborate if needed.

[LePips]

The backend work looks fine to me, just feels a bit overengineered to try and avoid re-authorizing/polling on that weird second start which I did observe only a few times but works great! Only design changes.

Thanks for the comprehensive comments!

[tonyd33]

The backend work looks fine to me, just feels a bit overengineered to try and avoid re-authorizing/polling on that weird second start which I did observe only a few times but works great! Only design changes.

I agree it was overengineered. At the time, I wanted to preserve the existing interfaces as much as possible, but I didn't know them well, which lead to that solution. I addressed all your comments, but I ended up just rewriting the quick connect flow because I felt like there was an easier and simpler way to do it. Sorry for making you do a review from (mostly) scratch again.

Here's a quick summary of key changes, apart from your comments:

• Polling is done in a much more sane way now – just read pollForAuthSecret.
  • No more gimmicks with ID's, max retries, and cancelling the task works properly now
  • Polling can only be initiated when we have the initial secret
• No AsyncStreams anymore – from what I understand, this isn't a good use case of it. Just use async functions instead
• startQuickConnect was replaced with signInWithQuickConnect which gets the secrets and signs in automatically while updating state for views using quickConnectStatus

[LePips]

I'm liking the evolution of this so far. Apologies on my part, but I will ask that this is redone a little bit more. I have implemented a Stateful behavior for view models that I have liked so far and would have moved this entire behavior to it eventually, so let's do it now.

I think this would be fine to split out into a separate view model QuickConnectViewModel which the UserSignInViewModel holds onto, similar to how I have the SearchViewModel/PagingLibraryViewModel hold onto a FilterViewModel and how HomeViewModel holds onto a few. I am going against an old comment of mine saying it "can and should all be done in UserSignInViewModel but now that this is stateful it deserves its own thing. I also had the thought to provide a [default] Quick Connect mechanism in the API package so this would be a step towards that.

The views are fine and would just need to be updated to use the Stateful design. You can reference some views that I have migrated over to it.

[tonyd33]

The Stateful framework is a pretty natural extension of what I wrote, so I'm all for it.

[LePips]

Hey! Is there interest to keep working on this? If not, no worries. I'll be doing a refactor of the server/user handling/flows soon and would overlap with this work.

Since I've been doing some work in the SDK for the 10.9.0 release the thought of having this in the SDK has been on my mind. I would very much like a quick connect manager in the SDK and we use it in here. It may take a different form than what's implemented here for general client use.

I may take this over if this isn't worked on soon.

[tonyd33]

Yeah, I’m down to keep working on this. How can I help? Did you want me to move some of what’s implemented here into the SDK?

[LePips]

Actually, I may have been a little too hasty in thought since I've now taken a look at everything done here. Since this requires all the view work as well and I want to start on my work fairly soon, I'll accept this and will be able to work on top of it.

Not exactly the end of the world if quick connect isn't in the SDK and I think I would want it to have a different API over there which I may tinker with here.

[LePips]

I seem to have done a lot with Stateful since this work was last done and would have had probably more problems down the road, so I actually am glad that I got to this now.

Thank you for the contribution!