-
Notifications
You must be signed in to change notification settings - Fork 139
Move sign-in error handling to SignInScreen #3473
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
The `PermissionDeniedDialog` is now managed within the `SignInScreen` based on the sign-in state, rather than being handled globally via `MainUiState` in `MainActivity`.
The `onUserSignInError` method and associated logic have been removed from `MainViewModel`, as sign-in error handling is now managed within the `SignInScreen`. The fallback behavior on error remains a sign-out state with a placeholder for future error dialog improvements.
Summary of ChangesHello @shobhitagarwal1612, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the application's sign-in error state management. By migrating error handling and the display of related UI components directly into the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
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 refactors the sign-in error handling by moving the logic from MainActivity and MainViewModel to the SignInScreen composable. This is a good architectural improvement, making the sign-in flow more self-contained. The changes include removing the error handling from MainViewModel, deleting MainUiState.OnPermissionDenied, and implementing a new SignInErrorUi composable to display different error states like permission denied, user cancellation, or unknown errors.
My main feedback is regarding a regression in error handling for issues that occur after a successful authentication but during the user session setup (e.g., in onUserSignedIn). These errors are now handled generically by signing the user out without any explanation, whereas previously specific errors like permission denied would show an informative dialog. Please see my detailed comment.
| } catch (e: Throwable) { | ||
| onUserSignInError(e) | ||
| Timber.e(e) | ||
| // TODO: Display some error dialog to the user with a helpful user-readable message. | ||
| onUserSignedOut() | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This catch block now handles all exceptions by signing the user out. This is a regression from the previous behavior where onUserSignInError would specifically handle PermissionDeniedException and show a dialog to the user. Now, if an error like permission denied occurs after sign-in (e.g., during userRepository.saveUserDetails), the user is returned to the sign-in screen without any explanation.
While the sign-in error handling has been moved to SignInScreen, that screen only handles SignInState.Error, which is for failures during the authentication process itself. Errors happening after successful authentication but during user session setup are now handled poorly.
Please consider restoring specific error handling here. One approach could be to check for isPermissionDeniedException within this catch block and then find a way to signal this specific error to the UI, so the user can be properly informed.
Tests have been added to verify: - Display of permission denied dialog on Firestore permission denied errors. - Application exit and sign-out actions within the permission denied dialog. - Suppression of error UI when sign-in is cancelled by the user. - Display of a snackbar for unknown sign-in errors.
The test case verifying that sign-in errors redirect to a signed-out state has been removed from `MainViewModelTest`.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3473 +/- ##
============================================
- Coverage 70.02% 69.77% -0.25%
+ Complexity 1604 1599 -5
============================================
Files 322 322
Lines 8646 8678 +32
Branches 949 950 +1
============================================
+ Hits 6054 6055 +1
- Misses 2017 2049 +32
+ Partials 575 574 -1
🚀 New features to boost your workflow:
|
Simplify method references.
Towards #1795
Migrates the logic related to handling sign-in errors from
MainViewModeltoSignInScreen.@andreia-ferreira PTAL?