fix(auth): report Google/Facebook/OAuth/Anonymous sign-in failures via onSignInFailure - #2433
fix(auth): report Google/Facebook/OAuth/Anonymous sign-in failures via onSignInFailure#2433demolaf wants to merge 1 commit into
Conversation
…a onSignInFailure
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an onSignInFailure callback to various authentication provider handlers (Anonymous, Facebook, Google, and OAuth) and propagates it through the UI and screens to report sign-in failures immediately. While the implementation and accompanying unit tests are solid, there are critical issues regarding Compose state management. Specifically, several remember and DisposableEffect blocks capture the new callback and other dependencies (like context and activity) without including them in their keys. This can lead to stale state captures, memory leaks (particularly with Activity references), and potential crashes. Updating these blocks to include all captured dependencies as keys is highly recommended.
| internal fun FirebaseAuthUI.rememberAnonymousSignInHandler( | ||
| config: AuthUIConfiguration, | ||
| onSignInFailure: (AuthException) -> Unit = {}, | ||
| ): () -> Unit { |
There was a problem hiding this comment.
The remember block on line 31 currently only keys on this (remember(this)). Since config, onSignInFailure, and context are captured inside the lambda but not included in the remember keys, any changes to these parameters (such as a new callback instance or updated configuration) will not be reflected in the returned launcher. This can lead to stale captures and memory leaks.\n\nPlease update the remember keys to include these dependencies:\nkotlin\nreturn remember(this, context, config, onSignInFailure) {\n
| context: Context, | ||
| config: AuthUIConfiguration, | ||
| provider: AuthProvider.Google, | ||
| onSignInFailure: (AuthException) -> Unit = {}, |
There was a problem hiding this comment.
The remember block on line 64 currently only keys on this, config. However, the lambda captures context, provider, and onSignInFailure. If any of these captured parameters change (for example, if the context changes due to configuration changes or a new onSignInFailure callback is provided), the returned launcher will continue to use the stale captured references.\n\nPlease update the remember keys to include all captured dependencies:\nkotlin\nreturn remember(this, context, config, provider, onSignInFailure) {\n
| activity: Activity?, | ||
| config: AuthUIConfiguration, | ||
| provider: AuthProvider.OAuth, | ||
| onSignInFailure: (AuthException) -> Unit = {}, |
There was a problem hiding this comment.
The remember block on line 66 keys on this, provider.providerId, config. However, the lambda captures context, activity, provider, and onSignInFailure.\n\nIn particular, capturing activity without keying on it is highly risky. If the Activity is recreated (e.g., due to screen rotation), the lambda will hold a stale reference to the destroyed activity, causing a memory leak and potentially crashing when attempting to start the OAuth flow.\n\nPlease update the remember keys to include all captured dependencies:\nkotlin\nreturn remember(this, context, activity, config, provider, onSignInFailure) {\n
| config: AuthUIConfiguration, | ||
| provider: AuthProvider.Facebook, | ||
| loginManagerProvider: AuthProvider.Facebook.LoginManagerProvider = AuthProvider.Facebook.DefaultLoginManagerProvider(), | ||
| onSignInFailure: (AuthException) -> Unit = {}, |
There was a problem hiding this comment.
The DisposableEffect on line 76 is currently keyed only on config. However, the registered FacebookCallback captures context, provider, and onSignInFailure. If any of these parameters change while config remains the same, the callback will use stale references.\n\nPlease update the DisposableEffect keys to include all captured dependencies:\nkotlin\nDisposableEffect(context, config, provider, onSignInFailure) {\n
1 participant
No description provided.