refactor(auth): scope phone verification loading state to the composition that owns it - #2454
Conversation
There was a problem hiding this comment.
Code Review
This pull request simplifies state management in FirebaseAuthUI by removing the AtomicLong revision tracking and instead handling the retraction of the Loading state directly within the UI layer using Compose's lifecycle. Specifically, a DisposableEffect is introduced in PhoneAuthScreen to reset the authentication state to Idle upon disposal if it was left in a Loading state. The review feedback points out a potential bug where rememberUpdatedState(authState) combined with DisposableEffect(authUI) can cause a mismatched state capture when authUI changes, as well as a race condition with cancellation. A code suggestion is provided to track the state per authUI instance using remember(authUI) { mutableStateOf(authState) }.
485c2d9 to
3edb410
Compare
russellwheatley
left a comment
There was a problem hiding this comment.
LGTM. The core simplification is sound, traced through all the cancellation call sites and the DisposableEffect(authUI) cleanup correctly fixes the per-instance state capture bug flagged in review.
One thing worth a follow-up look: submitVerificationCode and signInWithPhoneAuthCredential still write AuthState.Error on CancellationException, so the same stale-state-after-disposal issue this PR fixes for verifyPhoneNumber might still apply if the screen disposes mid-flight on those. Pre-existing and out of scope here, not blocking.
3edb410 to
98d196c
Compare
FirebaseAuthUIcarried a revision counter plusclearLoadingStateandcurrentAuthStateRevisionpurely soverifyPhoneNumbercould retract its ownAuthState.Loadingon cancellation without clobbering whatever superseded it.verifyPhoneNumbernow writes no state on cancellation and just rethrows, andPhoneAuthScreenretracts its ownLoadinginstead — inonChangeNumberClick, and from aDisposableEffectwhen the composition goes away mid-verification. Cancelling an attempt was already the screen's own bookkeeping, so the state it leaves behind belongs to the screen too.Dropped three tests covering the removed API and the old callee-retracts contract, and added one to
PhoneAuthScreenVerificationLifecycleTestfor the change-number case — verified it fails without the change.