-
Notifications
You must be signed in to change notification settings - Fork 694
[link] Web auth fallback #11571
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[link] Web auth fallback #11571
Conversation
|
Diffuse output: APKDEX |
| @Parcelize | ||
| @RestrictTo(RestrictTo.Scope.LIBRARY_GROUP) | ||
| @Serializable | ||
| data class DisplayablePaymentDetails( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀 Moved to its own file
|
|
||
| @Suppress("CyclomaticComplexMethod", "ComplexCondition") | ||
| private suspend fun updateScreenState(withAnimationDelay: Boolean) { | ||
| private suspend fun updateScreenState( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀 I gave this code a significant makeover in order to improve readability. It might still be hard to follow because of the inherent complexity, but hopefully it's better. Most of the existing behavior shouldn't have changed, which the tests cover.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is amazing, very needed.
| } | ||
|
|
||
| @VisibleForTesting | ||
| internal fun getScreenStateForAuthorizationAfterRefresh( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add docs for this function? just to clarify that it can either return a next screen, or just return null and dismiss internally.
| // Launch web auth flow if web auth URL is available. | ||
| // Next steps will happen in `handleWebAuthActivityResult`. | ||
| if (accountStatus.webviewOpenUrl != null) { | ||
| launchWebAuthFlow?.invoke(accountStatus.webviewOpenUrl) | ||
| return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a couple minor comments! doing some testing now.
| consumerPublishableKey = linkAccount.consumerPublishableKey | ||
| ) | ||
| .onFailure { error -> | ||
| linkEventsReporter.onAccountLookupFailure(error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[no action] This is not a lookup call right? we might want to have a specific event for this (can be a follow-up)
| } | ||
|
|
||
| @VisibleForTesting | ||
| internal fun getScreenStateForAuthorizationAfterRefresh( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add docs for this function? just to clarify that it can either return a next screen, or just return null and dismiss internally.
5579982 to
1939cea
Compare
|
I updated it so web auth is mostly handled in Verification instead. This has the benefits of:
|
1939cea to
5228e09
Compare
|
Still in draft because it's blocked on changes to the |
|
Backend changes were deployed. Pushed some minor-ish changes and updated the PR description with videos from manual testing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great work here - code looks good, doing some testing now.
| private val dismissWithResult: (LinkActivityResult) -> Unit, | ||
| ) : ViewModel() { | ||
|
|
||
| private val isProcessingWebAuth = linkAccount.webviewOpenUrl != null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - should we keep this only on the state to avoid confusion?
|
|
||
| private fun startWebVerification() { | ||
| viewModelScope.launch { | ||
| // If the web auth URL has already been consumed, perform lookup again to get a fresh one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can maybe mention that auth web urls cannot be reused, for context
|
|
||
| @Suppress("CyclomaticComplexMethod", "ComplexCondition") | ||
| private suspend fun updateScreenState(withAnimationDelay: Boolean) { | ||
| private suspend fun updateScreenState( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is amazing, very needed.
Summary
Fall back to web auth when required. High-level summary of how it works:
link-popup://...URI that the SDK will interceptMotivation
It won't be SMS only forever.
Testing
Screenshots
Recordings of various flows below
Screen.Recording.2025-09-17.at.12.51.33.PM-compressed.mov
Screen.Recording.2025-09-17.at.12.28.09.PM-compressed.mov
Screen.Recording.2025-09-17.at.12.23.21.PM-compressed.mov
Screen.Recording.2025-09-17.at.12.22.16.PM-compressed.mov