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 6 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.
| ) { | ||
| // Get current error state | ||
| val currentErrorState = authState as? AuthState.Error | ||
| val currentErrorState = currentAuthState() as? AuthState.Error |
There was a problem hiding this comment.
There is a subtle race condition in the error dialog deduplication logic. Because authUI.updateAuthState(AuthState.Idle) is called immediately after showErrorDialog in the observing screens (e.g., EmailAuthScreen and FirebaseAuthScreen), the first observer to run will reset the auth state to Idle. When the second observer runs and calls showErrorDialog, currentAuthState() will return AuthState.Idle instead of AuthState.Error. This causes currentErrorState to be null, bypassing the deduplication check and allowing the second call to overwrite the dialog state (potentially losing custom onRetry or onRecover callbacks).
To fix this, consider adding an optional errorState: AuthState.Error? = null parameter to showErrorDialog and using it for deduplication:
fun showErrorDialog(
exception: AuthException,
errorState: AuthState.Error? = null,
onRetry: (AuthException) -> Unit = {},
onRecover: ((AuthException) -> Unit)? = null,
onDismiss: () -> Unit = {}
) {
val currentErrorState = errorState ?: (currentAuthState() as? AuthState.Error)
if (currentErrorState != null && currentErrorState in shownErrorStates) {
return
}
currentErrorState?.let { shownErrorStates.add(it) }
// ...
}| return remember { | ||
| TopLevelDialogController(stringProvider, authState) | ||
| } |
There was a problem hiding this comment.
In rememberTopLevelDialogController, stringProvider is passed as a parameter but is not used as a key in remember. If stringProvider changes (e.g., due to locale or configuration changes), the controller will hold a stale reference.
We should key the remember block on stringProvider so that the controller is recreated if the string provider changes.
| return remember { | |
| TopLevelDialogController(stringProvider, authState) | |
| } | |
| return remember(stringProvider) { | |
| TopLevelDialogController(stringProvider, authState) | |
| } |
|
|
||
| val authState by remember(authUI) { authUI.authStateFlow() }.collectAsState(AuthState.Idle) | ||
| val dialogController = rememberTopLevelDialogController(stringProvider, authState) | ||
| val dialogController = rememberTopLevelDialogController(stringProvider) { authState } |
There was a problem hiding this comment.
stringProvider is currently instantiated on every recomposition of FirebaseAuthScreen (line 138: val stringProvider = DefaultAuthUIStringProvider(context)). Because it is not remembered, any remember block keying on it (such as uiContext on line 275) will be invalidated and recreated on every single recomposition.
Please wrap stringProvider in a remember block on line 138:
val stringProvider = remember(context) { DefaultAuthUIStringProvider(context) }
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.