Skip to content

fix(auth): report Google/Facebook/OAuth/Anonymous sign-in failures via onSignInFailure - #2433

Merged
russellwheatley merged 4 commits into
version-10.0.0-beta04from
fix/auth-signin-failure-reporting
Aug 12, 2026
Merged

fix(auth): report Google/Facebook/OAuth/Anonymous sign-in failures via onSignInFailure#2433
russellwheatley merged 4 commits into
version-10.0.0-beta04from
fix/auth-signin-failure-reporting

Conversation

@demolaf

@demolaf demolaf commented Jul 30, 2026

Copy link
Copy Markdown
Member

FirebaseAuthScreen's onSignInFailure callback was only ever invoked from EmailAuthScreen and PhoneAuthScreen's own effects. Google, Facebook, OAuth (Apple/Github/Microsoft/Yahoo/Twitter/generic), and Anonymous sign-in failures only updated the shared AuthState.Error - which the top-level dialog controller shows - but never reached onSignInFailure, so a consuming app's own error logging/analytics wired through that callback silently never fired for those providers.

Threads an onSignInFailure callback into all four remember*SignInHandler functions, calling it alongside each existing updateAuthState(AuthState.Error(...)) so each provider reports its own failure exactly once, at the source - symmetric with how Email/Phone already report theirs.

  • GoogleAuthProvider+FirebaseAuthUI.kt, FacebookAuthProvider+FirebaseAuthUI.kt, OAuthProvider+FirebaseAuthUI.kt, AnonymousAuthProvider+FirebaseAuthUI.kt: added an onSignInFailure parameter to each remember*Handler, called in their catch blocks.
  • FirebaseAuthScreen.kt: threads onSignInFailure through rememberOnProviderSelected and its two call sites - the main flow passes the real callback; the reauth sheet keeps its existing no-op default, matching how it already no-ops Email/Phone's onError there.

Added direct tests forcing a failure (testCredentialManagerProvider for Google, a failing signInAnonymously() task for Anonymous) confirming onSignInFailure fires immediately without navigating anywhere - verified both are load-bearing by temporarily reverting the fix and confirming they fail.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@demolaf
demolaf marked this pull request as ready for review August 5, 2026 12:27
@demolaf
demolaf force-pushed the fix/auth-signin-failure-reporting branch from 5c54629 to c3c78bb Compare August 5, 2026 12:37

@russellwheatley russellwheatley left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cancel filtering looks incomplete for OAuth web cancels, plus a couple of smaller consistency/coverage notes

@demolaf
demolaf force-pushed the fix/auth-signin-failure-reporting branch from 08edd5a to 6ce6495 Compare August 11, 2026 19:59
@russellwheatley
russellwheatley merged commit 730ea98 into version-10.0.0-beta04 Aug 12, 2026
9 of 10 checks passed
@russellwheatley
russellwheatley deleted the fix/auth-signin-failure-reporting branch August 12, 2026 11:23
@github-project-automation github-project-automation Bot moved this from Backlog to Done in studio-2394994192-60a69 Aug 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants