diff --git a/auth/src/main/java/com/firebase/ui/auth/FirebaseAuthUI.kt b/auth/src/main/java/com/firebase/ui/auth/FirebaseAuthUI.kt index 972b1786b..432d25c82 100644 --- a/auth/src/main/java/com/firebase/ui/auth/FirebaseAuthUI.kt +++ b/auth/src/main/java/com/firebase/ui/auth/FirebaseAuthUI.kt @@ -41,7 +41,6 @@ import kotlinx.coroutines.flow.combine import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.tasks.await import java.util.concurrent.ConcurrentHashMap -import java.util.concurrent.atomic.AtomicLong /** * The central class that coordinates all authentication operations for Firebase Auth UI Compose. @@ -80,7 +79,6 @@ class FirebaseAuthUI private constructor( ) { private val _authStateFlow = MutableStateFlow(AuthState.Idle) - private val authStateRevision = AtomicLong(0) @RestrictTo(RestrictTo.Scope.LIBRARY_GROUP) var testCredentialManagerProvider: AuthProvider.Google.CredentialManagerProvider? = null @@ -365,29 +363,9 @@ class FirebaseAuthUI private constructor( */ @MainThread fun updateAuthState(state: AuthState) { - authStateRevision.incrementAndGet() _authStateFlow.value = state } - /** - * Retracts a pending [AuthState.Loading] by resetting to [AuthState.Idle], but only while - * [revision] is still the most recent write. Any state emitted since is left untouched. - * - * The revision is what makes this precise: [AuthState.Loading] compares equal whenever the - * message matches, and [MutableStateFlow] drops a write equal to the current value without - * replacing the stored reference - so neither equality nor identity can tell a concurrent - * operation's Loading apart from the caller's. - * - * @param revision The value [currentAuthStateRevision] returned right after the caller emitted - * the [AuthState.Loading] it now wants to retract - */ - internal fun clearLoadingState(revision: Long) { - if (authStateRevision.get() == revision) updateAuthState(AuthState.Idle) - } - - /** Identifies the most recent [updateAuthState] write. See [clearLoadingState]. */ - internal fun currentAuthStateRevision(): Long = authStateRevision.get() - internal fun updateAuthStateWithResult(result: AuthResult?, defaultIsNewUser: Boolean = false) { val user = result?.user if (user != null) { diff --git a/auth/src/main/java/com/firebase/ui/auth/configuration/auth_provider/PhoneAuthProvider+FirebaseAuthUI.kt b/auth/src/main/java/com/firebase/ui/auth/configuration/auth_provider/PhoneAuthProvider+FirebaseAuthUI.kt index 1ee3dc72f..8cf9fa7c2 100644 --- a/auth/src/main/java/com/firebase/ui/auth/configuration/auth_provider/PhoneAuthProvider+FirebaseAuthUI.kt +++ b/auth/src/main/java/com/firebase/ui/auth/configuration/auth_provider/PhoneAuthProvider+FirebaseAuthUI.kt @@ -119,11 +119,8 @@ internal suspend fun FirebaseAuthUI.verifyPhoneNumber( forceResendingToken: PhoneAuthProvider.ForceResendingToken? = null, verifier: AuthProvider.Phone.Verifier = AuthProvider.Phone.DefaultVerifier(), ) { - // -1 never matches a real revision, so a cancellation before the Loading lands clears nothing. - var loadingRevision = -1L try { updateAuthState(AuthState.Loading(config.stringProvider.loadingVerifyingPhoneNumber)) - loadingRevision = currentAuthStateRevision() provider.verifyPhoneNumberFlow( auth = auth, activity = activity, @@ -148,9 +145,8 @@ internal suspend fun FirebaseAuthUI.verifyPhoneNumber( } } } catch (e: CancellationException) { - // Cancellation here is the screen's own bookkeeping, not a failure: retract only the - // Loading this call emitted, then rethrow so no spurious Error reaches authStateFlow. - clearLoadingState(loadingRevision) + // Writes nothing: the caller cancelling this attempt owns whatever state replaces it, and + // a retraction from here would race the replacement's own Loading. throw e } catch (e: AuthException) { updateAuthState(AuthState.Error(e)) diff --git a/auth/src/main/java/com/firebase/ui/auth/ui/screens/phone/PhoneAuthScreen.kt b/auth/src/main/java/com/firebase/ui/auth/ui/screens/phone/PhoneAuthScreen.kt index 2406da779..6d966f027 100644 --- a/auth/src/main/java/com/firebase/ui/auth/ui/screens/phone/PhoneAuthScreen.kt +++ b/auth/src/main/java/com/firebase/ui/auth/ui/screens/phone/PhoneAuthScreen.kt @@ -18,6 +18,7 @@ import android.content.Context import android.util.Log import androidx.activity.compose.LocalActivity import androidx.compose.runtime.Composable +import androidx.compose.runtime.DisposableEffect import androidx.compose.runtime.LaunchedEffect import androidx.compose.runtime.collectAsState import androidx.compose.runtime.getValue @@ -178,8 +179,21 @@ fun PhoneAuthScreen( } } - val authState by remember(authUI) { authUI.authStateFlow() }.collectAsState(AuthState.Idle) + val currentAuthState = remember(authUI) { authUI.authStateFlow() }.collectAsState(AuthState.Idle) + val authState by currentAuthState val isLoading = authState is AuthState.Loading + + // A cancelled attempt leaves its Loading behind, and that state outlives this composition on + // the process-scoped FirebaseAuthUI, so a freshly composed screen would inherit the spinner. + // Read through currentAuthState, which is re-remembered per authUI: onDispose must see the + // state of the instance it was showing, not one it was swapped for. + DisposableEffect(authUI) { + onDispose { + if (currentAuthState.value is AuthState.Loading) { + authUI.updateAuthState(AuthState.Idle) + } + } + } val errorMessage = if (authState is AuthState.Error) (authState as AuthState.Error).exception.message else null @@ -399,6 +413,8 @@ fun PhoneAuthScreen( resendTimer = resendTimerSeconds.intValue, onChangeNumberClick = { cancelVerification("changing phone number") + // Nothing replaces the cancelled attempt here, so this handler retracts its Loading. + authUI.updateAuthState(AuthState.Idle) verificationJob.value = null isSubmittingCode.value = false step.value = PhoneAuthStep.EnterPhoneNumber diff --git a/auth/src/test/java/com/firebase/ui/auth/configuration/auth_provider/PhoneAuthProviderFirebaseAuthUITest.kt b/auth/src/test/java/com/firebase/ui/auth/configuration/auth_provider/PhoneAuthProviderFirebaseAuthUITest.kt index 18b7a7c22..df78205cb 100644 --- a/auth/src/test/java/com/firebase/ui/auth/configuration/auth_provider/PhoneAuthProviderFirebaseAuthUITest.kt +++ b/auth/src/test/java/com/firebase/ui/auth/configuration/auth_provider/PhoneAuthProviderFirebaseAuthUITest.kt @@ -411,23 +411,6 @@ class PhoneAuthProviderFirebaseAuthUITest { .isNotInstanceOf(AuthState.Error::class.java) } - @Test - fun `verifyPhoneNumber - cancellation clears the pending Loading state`() = runTest { - val instance = FirebaseAuthUI.create(firebaseApp, mockFirebaseAuth) - val deferred = startNeverResolvingVerifyPhoneNumber(instance) - - deferred.cancel() - try { - deferred.await() - } catch (_: CancellationException) { - // Expected - } - - val state = instance.authStateFlow().first() - assertThat(state).isNotInstanceOf(AuthState.Loading::class.java) - assertThat(state).isInstanceOf(AuthState.Idle::class.java) - } - @Test fun `verifyPhoneNumber - cancellation does not clobber a newer unrelated state`() = runTest { val instance = FirebaseAuthUI.create(firebaseApp, mockFirebaseAuth) @@ -470,32 +453,6 @@ class PhoneAuthProviderFirebaseAuthUITest { resend.cancel() } - @Test - fun `clearLoadingState - equal but distinct Loading instances do not clear each other`() = - runTest { - val instance = FirebaseAuthUI.create(firebaseApp, mockFirebaseAuth) - val first = AuthState.Loading("verifying") - val second = AuthState.Loading("verifying") - assertThat(first).isEqualTo(second) - - instance.updateAuthState(first) - val firstRevision = instance.currentAuthStateRevision() - instance.updateAuthState(second) - instance.clearLoadingState(firstRevision) - - assertThat(instance.authStateFlow().first()).isInstanceOf(AuthState.Loading::class.java) - } - - @Test - fun `clearLoadingState - clears when its Loading is still the latest state`() = runTest { - val instance = FirebaseAuthUI.create(firebaseApp, mockFirebaseAuth) - instance.updateAuthState(AuthState.Loading("verifying")) - - instance.clearLoadingState(instance.currentAuthStateRevision()) - - assertThat(instance.authStateFlow().first()).isInstanceOf(AuthState.Idle::class.java) - } - // Starts verifyPhoneNumber against a flow that never emits, UNDISPATCHED so the call reaches // its suspension point (past the Loading emission) before the caller can cancel it. private fun CoroutineScope.startNeverResolvingVerifyPhoneNumber( diff --git a/auth/src/test/java/com/firebase/ui/auth/ui/screens/phone/PhoneAuthScreenVerificationLifecycleTest.kt b/auth/src/test/java/com/firebase/ui/auth/ui/screens/phone/PhoneAuthScreenVerificationLifecycleTest.kt index 1eb925f8f..30147f5f7 100644 --- a/auth/src/test/java/com/firebase/ui/auth/ui/screens/phone/PhoneAuthScreenVerificationLifecycleTest.kt +++ b/auth/src/test/java/com/firebase/ui/auth/ui/screens/phone/PhoneAuthScreenVerificationLifecycleTest.kt @@ -509,6 +509,20 @@ class PhoneAuthScreenVerificationLifecycleTest { } } + @Test + fun `change-number clears the loading state left behind by the cancelled attempt`() { + mockStatic(PhoneAuthProvider::class.java).use { statics -> + setScreenContent() + sendCode(statics) + assertThat(capturedState!!.isLoading).isTrue() + + onUi { it.onChangeNumberClick() } + settle() + + assertThat(capturedState!!.isLoading).isFalse() + } + } + @Test fun `a failed sign-in reports exactly one error`() { val credential = mock(PhoneAuthCredential::class.java)