fix(auth): stale one-off AuthState no longer leaks across screen instances - #2415
fix(auth): stale one-off AuthState no longer leaks across screen instances#2415demolaf wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors TopLevelDialogController to accept a lambda for retrieving the current AuthState, preventing the controller from being recreated on every state change. It also updates the auth screens to reset the auth state to Idle after consumption and introduces local state variables in EmailAuthScreen to manage UI states. Additionally, new unit tests are added to verify state consumption and controller survival. The review feedback highlights a potential race condition in the error dialog deduplication logic due to immediate state resetting, suggests keying the controller's remember block on stringProvider to prevent stale references, and recommends wrapping stringProvider in a remember block in FirebaseAuthScreen to avoid unnecessary recompositions.
fb5b700 to
267731e
Compare
267731e to
e035a60
Compare
russellwheatley
left a comment
There was a problem hiding this comment.
LGTM overall, nice fix for the stale auth state leak. Just a couple of concerns before merging, left inline.
…cation instead of a hardcoded list
A one-off AuthState (Error, Cancelled, SMSAutoVerified, PasswordResetLinkSent, EmailSignInLinkSent) written to FirebaseAuthUI's singleton state flow was never reset back to Idle after being consumed, so it got redelivered to a completely new screen/Activity that had nothing to do with the original event — reproduced by triggering a phone-auth error, backing out of the Activity, and re-entering: a stale "Authentication Error" dialog appeared immediately, before touching anything.
FirebaseAuthScreen, EmailAuthScreen, and PhoneAuthScreen now reset auth state back to Idle immediately after consuming Error/Cancelled/SMSAutoVerified in the same effect that reacts to them. TopLevelDialogController no longer keys its remember on the live auth state (or on an unmemoized stringProvider), since either caused the controller — and the dialog it was showing — to be torn down on the very next recomposition.
EmailAuthScreen.kt:resetLinkSent/emailSignInLinkSentare now latched into local state instead of derived directly fromauthState, since their confirmation dialogs latch on the boolean's transition and would otherwise close immediately when the reset above fires.Added regression coverage in
FirebaseAuthUIAuthStateTestand a newTopLevelDialogControllerTestproving stale state doesn't leak to a fresh collector and the dialog survives auth state changes.Behavior
bug-demo.webm
fix-dialog.webm