fix(auth): stop showing error dialog when Google sign-in is cancelled - #2427
fix(auth): stop showing error dialog when Google sign-in is cancelled#2427demolaf wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds handling for GetCredentialCancellationException when a user dismisses the Credential Manager sheet during Google Sign-In, updating the authentication state to AuthState.Cancelled without throwing an exception. It also changes the visibility of getRecoveryMessage, getRecoveryActionText, and isRecoverable in ErrorRecoveryDialog.kt from private to internal, enabling direct testing and the removal of redundant helper methods in ErrorRecoveryDialogLogicTest.kt. There are no review comments, and I have no feedback to provide.
russellwheatley
left a comment
There was a problem hiding this comment.
One concern on where sheet dismiss lands in the state machine.
| // not an error, so it goes to AuthState.Cancelled instead of AuthState.Error. | ||
| // Swallow (don't rethrow) so rememberGoogleSignInHandler's catch block doesn't | ||
| // overwrite this state with AuthState.Error. | ||
| updateAuthState(AuthState.Cancelled) |
There was a problem hiding this comment.
Routing sheet dismiss to AuthState.Cancelled fixes the Compose path (no dialog, onSignInCancelled fires), but FirebaseAuthActivity finishes with RESULT_CANCELED on that state. So Activity / AuthFlowController consumers would leave the whole auth UI when the user just dismisses the Google sheet, rather than staying on the method picker.
Is that intentional? If those consumers should stay on the picker, this probably wants Idle (or similar) instead of Cancelled, and onSignInCancelled would need a different trigger for provider-level dismiss.
There was a problem hiding this comment.
there's a gap in the implementation, the AuthFlowController.cancel triggers AuthState.Cancelled and terminates the entire activity which means AuthState.Cancelled is doing multiple things.
Using AuthState.Idle doesn't convey that an operation was cancelled, so that's ruled out.
We can leave AuthState.Cancelled for when an operation is cancelled and introduce a new state e.g. AuthState.Finished for when AuthFlowController.cancel is invoked?
This would be a breaking change.
There was a problem hiding this comment.
Yes, it would be a breaking change in behaviour, but Cancelled is doing two jobs. So I think we have to split it out. Here is what it should look like:
Cancelled cancels the current step within the flow (provider sheet dismissed, MFA challenge backed out of) and returns to the picker.
Aborted (new) aborts the entire flow, triggered by AuthFlowController.cancel(), and finishes the activity.
Went with Aborted over Finished, reads too close to Success in my opinion.
Another thing: Cancelled isn't just consumed in FirebaseAuthActivity, it's also handled in FirebaseAuthScreen, PhoneAuthScreen, and EmailAuthScreen. AuthState isn't sealed, so none of those will fail to compile if we forget to add the matching Aborted branch, they'll just silently no-op. Worth a test per call site rather than relying on the compiler to catch it.
Also, MFA-challenge-cancel currently routes through Cancelled too, so today it already finishes the whole activity via that same listener. After the split it'll correctly stay in the flow instead, that's a behavior change that should have its own test, not just a side effect.
d6ba147 to
0c191b4
Compare
|
Waiting on this PR #2440 after which we can merge this. |
0c191b4 to
e3deeb8
Compare
Fixes #2422.
Cancelling the Google sign-in Credential Manager sheet showed an "Authentication Error" dialog with the raw
GetCredentialCancellationExceptionmessage ("User cancelled the selector") as the button text, instead of returning silently viaAuthState.Cancelled.signInWithGoogleonly caughtkotlinx.coroutines.CancellationException, not the actual exception Credential Manager throws on dismissal, so it fell through to the generic error path.GoogleAuthProvider+FirebaseAuthUI.kt: catchesGetCredentialCancellationExceptionexplicitly and updatesAuthState.Cancelledwithout rethrowing, so the sign-in flow returns to the picker silently andonSignInCancelledfires.ErrorRecoveryDialog.kt:getRecoveryActionTextno longer uses the raw exception message as the button label forAuthCancelledException.Added a test covering the Credential Manager cancellation path, and fixed
ErrorRecoveryDialogLogicTestto exercise the real (now-internal) dialog logic functions instead of a stale duplicate that had drifted from the actual code.Preview
no-cancel-dialog.webm