Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the Google and Apple sign-in flows to simplify the authentication architecture by removing the intermediate sign_in_with_auth_credential_use_case and directly handling Firebase authentication within each provider-specific use case.
Changes:
- Separated loading states for Google and Apple sign-in buttons
- Consolidated sign-in completion logic into a single
_signInCompletionmethod - Removed intermediate credential handling use case and integrated Firebase sign-in directly into provider-specific use cases
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| authentication_state.dart | Split single isSigningIn flag into separate isGoogleSigningIn and isAppleSigningIn flags; refactored sign-in completion flow |
| authentication_page_content.dart | Updated loading state bindings to use provider-specific flags |
| authentication_event.dart | Added new error event type to handle sign-in errors |
| sign_in_with_google_use_case.dart | Refactored to directly handle Firebase sign-in and exception mapping |
| sign_in_with_apple_use_case.dart | Refactored to directly handle Firebase sign-in with platform-specific logic and exception mapping |
| sign_in_anonymously_use_case.dart | Simplified to only handle Firebase anonymous sign-in |
| sign_in_with_auth_credential_use_case.dart | Removed file - logic consolidated into provider-specific use cases |
| custom_exception.dart | Added Firebase error code mapping for 'web-context-canceled' |
| README.md | Removed completed TODO item |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Update current user | ||
| await ref.read(currentUserStateProvider.notifier).updateCurrentUser(user); | ||
|
|
There was a problem hiding this comment.
The updateCurrentUser is called twice - once inside signInCompletionUseCase (line 31 of that file) and again here. This creates a redundant database write operation. Remove the call from either location to avoid duplication.
| // Update current user | |
| await ref.read(currentUserStateProvider.notifier).updateCurrentUser(user); |
| setStateData(currentData?.copyWith(isGoogleSigningIn: true)); | ||
| final provider = ref.read(signInWithGoogleUseCaseProvider.future); | ||
| await _signInCompletion(provider); | ||
| setStateData(currentData?.copyWith(isGoogleSigningIn: false)); |
There was a problem hiding this comment.
The loading state flag is not reset to false when an exception occurs in _signInCompletion. This causes the button to remain in a loading state after a sign-in error. Wrap the call in a try-finally block to ensure the flag is always reset.
| setStateData(currentData?.copyWith(isAppleSigningIn: true)); | ||
| final provider = ref.read(signInWithAppleUseCaseProvider.future); | ||
| await _signInCompletion(provider); | ||
| setStateData(currentData?.copyWith(isAppleSigningIn: false)); |
There was a problem hiding this comment.
The loading state flag is not reset to false when an exception occurs in _signInCompletion. This causes the button to remain in a loading state after a sign-in error. Wrap the call in a try-finally block to ensure the flag is always reset.
1 participant
Refactored both Google and Apple sign in flows.