[Feat] [Integrate authentication system]#42
[Feat] [Integrate authentication system]#42shivamsharma-1996 wants to merge 31 commits intodevelopfrom
Conversation
- Added BaseAuthProvider interface and concrete implementations: - FirebaseGoogleAuthProvider - FirebaseAnonymousAuthProvider - Introduced AuthManager to handle unified login flow - Created AuthUiState and AuthViewModel to manage auth-related UI state - Defined AuthType, AuthResult, and User data models - Added FirestoreUserRepository for user persistence
WalkthroughThis change introduces a comprehensive user authentication system to the application, supporting both Google OAuth and anonymous sign-in via Firebase. It adds new authentication-related models, repositories, providers, dependency injection modules, UI components, and control flow for sign-in, sign-out, and user management. The UI now includes personalized greetings, sign-in screens, loading indicators, and logout confirmation dialogs, all integrated with the new authentication backend. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SignInActivity
participant AuthViewModel
participant AuthRepository
participant AuthManager
participant AuthProvider (Google/Anon)
participant UserRepository
participant Firebase
User->>SignInActivity: Launches app or chooses Sign In
SignInActivity->>AuthViewModel: signInWithGoogle() or signInAnonymously()
AuthViewModel->>AuthRepository: signInWithGoogle()/signInAnonymously()
AuthRepository->>AuthProvider (Google/Anon): signInWithGoogle()/signInAnonymously()
AuthProvider (Google/Anon)->>Firebase: Authenticate user
Firebase-->>AuthProvider (Google/Anon): Auth result
AuthProvider (Google/Anon)-->>AuthRepository: AuthResult
AuthRepository->>AuthManager: handleAuthSuccess(authProvider)
AuthManager->>AuthProvider (Google/Anon): getCurrentUser()
AuthProvider (Google/Anon)-->>AuthManager: FirebaseUser
AuthManager->>UserRepository: saveOrUpdateUser(FirebaseUser)
UserRepository-->>AuthManager: Success/Failure
AuthManager-->>AuthRepository: AuthResult
AuthRepository-->>AuthViewModel: AuthResult
AuthViewModel-->>SignInActivity: Update UI state
SignInActivity-->>User: Show main screen or error
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (11)
app/src/main/java/com/darshan/notificity/extensions/Extensions.kt (1)
87-88: Improve edge case handling in toTitleCase extension.The current implementation may not handle multiple consecutive spaces or leading/trailing spaces gracefully. Consider this more robust implementation:
-fun String.toTitleCase(): String = - lowercase().split(" ").joinToString(" ") { it.replaceFirstChar { char -> char.uppercaseChar() } } +fun String.toTitleCase(): String = + trim().lowercase().split("\\s+".toRegex()).joinToString(" ") { word -> + word.replaceFirstChar { char -> char.uppercaseChar() } + }This handles multiple spaces, leading/trailing whitespace, and other whitespace characters more robustly.
app/src/main/java/com/darshan/notificity/components/buttons/ActionButton.kt (1)
57-62: Consider adding content description for accessibility.The Icon component has
contentDescription = nullwhich may impact accessibility. Consider making this configurable for better screen reader support.@Composable fun PrimaryActionButton( text: String, iconPainter: Painter? = null, + iconContentDescription: String? = null, showLoader: Boolean = false, onClick: () -> Unit, // ... other parameters ) { // ... button implementation Icon( painter = iconPainter, - contentDescription = null, + contentDescription = iconContentDescription, modifier = Modifier.size(20.dp), tint = Color.Unspecified )app/src/main/AndroidManifest.xml (1)
37-37: Consider adding explicit exported attribute for clarity.While the activity is not exported by default, explicitly setting
android:exported="false"improves code clarity and follows current Android best practices.- <activity android:name=".ui.signin.SignInActivity"/> + <activity + android:name=".ui.signin.SignInActivity" + android:exported="false" />app/src/main/java/com/darshan/notificity/auth/repository/FirestoreUserRepository.kt (1)
55-63: Consider abstracting timestamp generation for testability.Using
System.currentTimeMillis()directly makes the method harder to test and could cause issues in different timezones or testing environments.Consider injecting a clock or time provider:
+import java.time.Clock + @Singleton class FirestoreUserRepository @Inject constructor( - firestore: FirebaseFirestore + firestore: FirebaseFirestore, + private val clock: Clock = Clock.systemDefaultZone() ) : UserRepository { override suspend fun updateLastLogin(userId: String): Result<Unit> { return try { - usersCollection.document(userId).update(FIELD_LAST_LOGIN, System.currentTimeMillis()) + usersCollection.document(userId).update(FIELD_LAST_LOGIN, clock.millis())app/src/main/java/com/darshan/notificity/components/HeadlineWithDescription.kt (1)
23-23: Consider using Material 3 color scheme instead of hardcoded Color.Gray.For better theme consistency and accessibility, consider using
MaterialTheme.colorScheme.onSurfaceVariantinstead ofColor.Grayfor the description color default.- descriptionColor: Color = Color.Gray + descriptionColor: Color = MaterialTheme.colorScheme.onSurfaceVariantapp/src/main/res/raw/lottie_signin_page.json (1)
1-1: Consider optimizing the Lottie animation file size.This animation file is quite large (~1080 lines) which could significantly impact your app bundle size and performance. Consider:
- Simplifying the animation if possible
- Using Lottie's optimization tools to reduce file size
- Verifying this level of animation complexity is necessary for the sign-in experience
#!/bin/bash # Check the actual file size of the Lottie animation du -h app/src/main/res/raw/lottie_signin_page.json # Check total raw resources size find app/src/main/res/raw -name "*.json" -exec du -ch {} + | tail -1app/src/main/java/com/darshan/notificity/auth/repository/AuthRepository.kt (3)
32-32: Usevalinstead ofvarfor immutable reference.The
googleAuthProvidervariable is never reassigned after initialization.- var googleAuthProvider = providers[AuthType.GOOGLE] as GoogleOAuthProvider + val googleAuthProvider = providers[AuthType.GOOGLE] as GoogleOAuthProvider
45-45: Usevalinstead ofvarfor immutable reference.The
anonymousAuthProvidervariable is never reassigned after initialization.- var anonymousAuthProvider = providers[AuthType.ANONYMOUS] as AnonymousAuthProvider + val anonymousAuthProvider = providers[AuthType.ANONYMOUS] as AnonymousAuthProvider
32-37: Consider safer casting with null handling.While the current casting approach works due to DI setup, consider using safe casting for more robust error handling.
- val googleAuthProvider = providers[AuthType.GOOGLE] as GoogleOAuthProvider - - return when (val result = googleAuthProvider.signInWithGoogle(activityContext)) { + val googleAuthProvider = providers[AuthType.GOOGLE] as? GoogleOAuthProvider + ?: return AuthResult.Error(Exception("Google auth provider not found")) + + return when (val result = googleAuthProvider.signInWithGoogle(activityContext)) {app/src/main/java/com/darshan/notificity/auth/manager/AuthManager.kt (1)
40-64: Consider simplifying user save/update logic.The current logic has nested conditionals that could be simplified for better readability.
private suspend fun saveOrUpdateUser( authProvider: BaseAuthProvider, firebaseUser: FirebaseUser ): AuthResult { return try { - // Check if user already exists - val existingUserResult = userRepository.getUser(firebaseUser.uid) - - if (existingUserResult.isSuccess) { - val existingUser = existingUserResult.getOrNull() - if (existingUser == null) { - // New user, save to FireStore - val newUser = authProvider.createUserFromFirebaseUser(firebaseUser) - userRepository.saveUser(newUser) - } else { - // Existing user, update last login - userRepository.updateLastLogin(firebaseUser.uid) - } - } else { - // Error getting user, try to save anyway + val existingUser = userRepository.getUser(firebaseUser.uid).getOrNull() + + if (existingUser != null) { + // Existing user, update last login + userRepository.updateLastLogin(firebaseUser.uid) + } else { + // New user or error getting user, save to Firestore val newUser = authProvider.createUserFromFirebaseUser(firebaseUser) userRepository.saveUser(newUser) } AuthResult.Success } catch (e: Exception) { AuthResult.Error(e) } }app/src/main/java/com/darshan/notificity/ui/signin/SigninActivity.kt (1)
113-121: Performance consideration for infinite animation.The continuous rotation animation runs indefinitely and may impact battery life and performance. Consider using a more subtle animation or limiting its duration.
val rotation by infiniteTransition.animateFloat( initialValue = -15f, targetValue = 15f, animationSpec = infiniteRepeatable( - animation = tween(durationMillis = 400, easing = LinearEasing), + animation = tween(durationMillis = 800, easing = LinearEasing), repeatMode = RepeatMode.Reverse ), label = "Notification Rotation" )Alternatively, consider using a finite animation that plays only a few times.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
app/src/main/res/drawable/ic_google.pngis excluded by!**/*.pngapp/src/main/res/drawable/ic_logout.pngis excluded by!**/*.pngapp/src/main/res/font/playfair_display.ttfis excluded by!**/*.ttf
📒 Files selected for processing (35)
app/build.gradle.kts(1 hunks)app/src/main/AndroidManifest.xml(1 hunks)app/src/main/java/com/darshan/notificity/analytics/AnalyticsConstants.kt(1 hunks)app/src/main/java/com/darshan/notificity/auth/AuthType.kt(1 hunks)app/src/main/java/com/darshan/notificity/auth/manager/AuthManager.kt(1 hunks)app/src/main/java/com/darshan/notificity/auth/models/AuthResult.kt(1 hunks)app/src/main/java/com/darshan/notificity/auth/models/AuthUiState.kt(1 hunks)app/src/main/java/com/darshan/notificity/auth/models/User.kt(1 hunks)app/src/main/java/com/darshan/notificity/auth/providers/base/AnonymousAuthProvider.kt(1 hunks)app/src/main/java/com/darshan/notificity/auth/providers/base/BaseAuthProvider.kt(1 hunks)app/src/main/java/com/darshan/notificity/auth/providers/base/GoogleOAuthProvider.kt(1 hunks)app/src/main/java/com/darshan/notificity/auth/providers/firebase/FirebaseAnonymousAuthProvider.kt(1 hunks)app/src/main/java/com/darshan/notificity/auth/providers/firebase/FirebaseGoogleAuthProvider.kt(1 hunks)app/src/main/java/com/darshan/notificity/auth/repository/AuthRepository.kt(1 hunks)app/src/main/java/com/darshan/notificity/auth/repository/FirestoreUserRepository.kt(1 hunks)app/src/main/java/com/darshan/notificity/auth/repository/UserRepository.kt(1 hunks)app/src/main/java/com/darshan/notificity/components/AppTitle.kt(1 hunks)app/src/main/java/com/darshan/notificity/components/HeadlineWithDescription.kt(1 hunks)app/src/main/java/com/darshan/notificity/components/LoadingScreen.kt(1 hunks)app/src/main/java/com/darshan/notificity/components/LottieCenteredAnimation.kt(1 hunks)app/src/main/java/com/darshan/notificity/components/NotificityAppBar.kt(2 hunks)app/src/main/java/com/darshan/notificity/components/buttons/ActionButton.kt(1 hunks)app/src/main/java/com/darshan/notificity/components/buttons/TextButton.kt(1 hunks)app/src/main/java/com/darshan/notificity/components/dialogs/LogoutConfirmationDialog.kt(1 hunks)app/src/main/java/com/darshan/notificity/constants/FirestoreConstants.kt(1 hunks)app/src/main/java/com/darshan/notificity/di/AuthModule.kt(1 hunks)app/src/main/java/com/darshan/notificity/extensions/Extensions.kt(1 hunks)app/src/main/java/com/darshan/notificity/extensions/FirebaseExtensions.kt(1 hunks)app/src/main/java/com/darshan/notificity/main/ui/MainActivity.kt(4 hunks)app/src/main/java/com/darshan/notificity/ui/settings/SettingsActivity.kt(9 hunks)app/src/main/java/com/darshan/notificity/ui/signin/AuthViewModel.kt(1 hunks)app/src/main/java/com/darshan/notificity/ui/signin/SigninActivity.kt(1 hunks)app/src/main/res/raw/lottie_signin_page.json(1 hunks)app/src/main/res/values/strings.xml(1 hunks)gradle/libs.versions.toml(2 hunks)
🔇 Additional comments (45)
app/src/main/java/com/darshan/notificity/auth/AuthType.kt (1)
1-12: LGTM! Clean enum design.The enum is well-structured with clear documentation and appropriate values for the authentication system. The naming convention and documentation follow Kotlin best practices.
app/src/main/java/com/darshan/notificity/components/LoadingScreen.kt (1)
12-22: LGTM! Well-designed loading screen component.The composable follows Compose best practices with proper theming, centering, and clean structure. It's appropriately simple and reusable.
app/src/main/java/com/darshan/notificity/constants/FirestoreConstants.kt (1)
6-11: LGTM! Good constant management practice.Centralizing Firestore collection and field names in constants follows best practices and improves maintainability. The naming convention is clear and consistent.
app/src/main/java/com/darshan/notificity/auth/providers/base/AnonymousAuthProvider.kt (1)
9-16: LGTM! Clean interface design.The interface follows good architectural patterns with single responsibility, proper use of suspend functions for async operations, and clear documentation. The extension of BaseAuthProvider promotes code reuse while maintaining focused functionality.
gradle/libs.versions.toml (3)
46-47: Firebase dependency declarations look correct.The Firebase Authentication and Firestore library declarations are properly structured and follow the expected format for version catalog.
56-60: AndroidX Credential Manager and Google Identity Services integration is well-structured.The library declarations for credential management and Google Identity services are correctly formatted and properly reference the version variables.
21-22: Verify dependency versions are current and secure.The added dependency versions should be verified to ensure they are the latest stable versions and free from known security vulnerabilities.
What are the latest versions of androidx.credentials and com.google.android.libraries.identity.googleid libraries?app/src/main/java/com/darshan/notificity/components/buttons/ActionButton.kt (2)
26-37: Well-designed composable with comprehensive parameters.The function signature provides good flexibility with sensible defaults. The parameter organization follows Compose best practices.
49-55: Loading indicator implementation is well-designed.The conditional loading state with CircularProgressIndicator provides good user feedback during authentication operations.
app/src/main/res/values/strings.xml (1)
4-4: Verify OAuth client ID security and configuration.The Google OAuth client ID is properly stored as a string resource, which is the standard approach for Android apps. However, ensure this client ID is:
- Configured correctly in the Google Cloud Console
- Restricted to your app's package name and signing certificate
- Not a development/testing client ID in production
What are the security best practices for storing Google OAuth client IDs in Android applications?app/src/main/java/com/darshan/notificity/analytics/AnalyticsConstants.kt (1)
38-38: Analytics constant addition follows established patterns.The new
SIGNINconstant is consistent with existing screen name constants and follows the established naming convention.app/build.gradle.kts (1)
80-86: LGTM! Appropriate dependencies for authentication system.The added dependencies correctly support the Firebase authentication and Google OAuth functionality. The Google Identity & Credentials dependencies align with the modern Credential Manager API approach for Google sign-in.
Verify that the dependency versions are current and secure:
#!/bin/bash # Description: Check for latest versions and security advisories for auth-related dependencies # Check Firebase BOM and auth versions curl -s "https://firebase.google.com/docs/android/setup" | grep -A 5 "firebase-bom" # Check Credential Manager versions curl -s "https://developer.android.com/jetpack/androidx/releases/credentials" | grep -A 3 "credentials"app/src/main/java/com/darshan/notificity/auth/models/AuthResult.kt (1)
6-17: LGTM! Clean and appropriate result wrapper pattern.The sealed class design follows best practices for representing authentication operation outcomes. Using an
objectforSuccessand adata classwithExceptionforErrorprovides a type-safe way to handle both success and failure cases throughout the authentication flow.app/src/main/java/com/darshan/notificity/auth/providers/base/GoogleOAuthProvider.kt (1)
10-18: Well-designed interface for Google OAuth functionality.The interface properly extends
BaseAuthProviderand defines a clear contract for Google sign-in. The use ofComponentActivityparameter is appropriate for Google's Credential Manager API, and the suspend function withAuthResultreturn type maintains consistency with the authentication system.Verify the
BaseAuthProviderinterface to ensure consistent method signatures:#!/bin/bash # Description: Check BaseAuthProvider interface definition for consistency ast-grep --pattern 'interface BaseAuthProvider { $$$ }'app/src/main/java/com/darshan/notificity/components/AppTitle.kt (1)
13-29: Well-designed composable with proper Material Design integration.The
AppTitlecomposable follows Compose best practices with sensible defaults and proper customization options. The typography styling usingMaterialTheme.typography.headlineLarge.copy()is the correct approach for extending Material Design styles.Verify that the Playfair Display font resource exists:
#!/bin/bash # Description: Check if the Playfair Display font resource is available fd -t f "playfair_display" app/src/main/res/font/app/src/main/java/com/darshan/notificity/auth/models/AuthUiState.kt (1)
3-10: Excellent documentation and structure.The KDoc comments are comprehensive and clearly explain each property's purpose. The nullable types are used appropriately for optional state properties.
app/src/main/java/com/darshan/notificity/components/LottieCenteredAnimation.kt (1)
19-39: Well-implemented Lottie animation component.The composable follows Compose best practices with:
- Proper use of
rememberLottieCompositionfor resource loading- Appropriate default parameters (280.dp size, center alignment)
- Clean layout structure with
Boxfor centering- Infinite iteration setting for continuous animation
The implementation is solid and reusable.
app/src/main/java/com/darshan/notificity/components/buttons/TextButton.kt (1)
13-36: Solid implementation of secondary text button.The composable demonstrates good practices:
- Consistent parameter ordering with
textfirst,onClickrequired, and optional styling parameters- Proper use of
MaterialTheme.typography.bodyMediumas base style- Reasonable default values (Gray color, 16.sp font size)
- Proper composition of Material3 components
The implementation is clean and follows Material Design guidelines.
app/src/main/java/com/darshan/notificity/auth/repository/FirestoreUserRepository.kt (1)
18-24: Excellent use of dependency injection and error handling.The class properly uses:
@Singletonannotation for proper lifecycle management@Injectconstructor for dependency injectionResulttype for comprehensive error handling- Constants for collection names
The architectural patterns are well-applied.
app/src/main/java/com/darshan/notificity/components/NotificityAppBar.kt (3)
31-32: Good parameter design with sensible defaults.The new
fontSizeandfontparameters enhance customization while providing reasonable defaults:
- 24.sp is appropriate for app bar titles
- Playfair Display font adds visual appeal
- Parameters are well-positioned in the function signature
56-63: Excellent text handling improvements.The Text composable enhancements are well-implemented:
weight(1f)ensures proper space utilizationmaxLines = 1prevents multi-line titlesTextOverflow.Ellipsishandles long titles gracefully- Font customization parameters are properly applied
These changes prevent UI layout issues and improve user experience.
54-54: Appropriate spacing adjustment.Reducing the spacing from 16.dp to 8.dp creates better visual balance when a navigation icon is present, providing adequate separation without excessive whitespace.
app/src/main/java/com/darshan/notificity/auth/repository/UserRepository.kt (1)
11-37: Well-designed repository interface with excellent error handling.The interface provides a clean abstraction for user data operations with proper async support using suspend functions and robust error handling with
Result<T>. The documentation is comprehensive and the method signatures are intuitive.app/src/main/java/com/darshan/notificity/components/HeadlineWithDescription.kt (1)
17-43: Clean and well-structured composable implementation.The component follows Compose best practices with proper parameter ordering, sensible defaults, and good use of Material 3 design tokens.
app/src/main/java/com/darshan/notificity/components/dialogs/LogoutConfirmationDialog.kt (1)
13-65: Excellent dialog implementation with proper UX considerations.The component correctly implements destructive action styling for the confirm button when
isDestructiveis true, which provides clear visual feedback for potentially harmful actions like logout. The Material 3 styling and theming are properly applied.app/src/main/java/com/darshan/notificity/auth/providers/base/BaseAuthProvider.kt (2)
36-36: Verify if createUserFromFirebaseUser needs to be suspend.The
createUserFromFirebaseUserfunction is marked as suspend, but typically converting a FirebaseUser to a User model would be a synchronous operation. Is this intentional due to additional async operations like fetching user data from Firestore?
11-37: Clean and well-designed authentication provider interface.The interface provides an excellent abstraction for different authentication providers with a clear separation of concerns. The method signatures are intuitive and properly handle both synchronous and asynchronous operations.
app/src/main/java/com/darshan/notificity/extensions/FirebaseExtensions.kt (1)
11-37: Well-implemented Firebase authentication extensions.The extension functions provide a clean, type-safe interface for determining Firebase user authentication types. The logic is correct, with proper handling of anonymous users and Google authentication providers.
app/src/main/java/com/darshan/notificity/auth/providers/firebase/FirebaseAnonymousAuthProvider.kt (1)
28-76: Solid anonymous authentication provider implementation.The implementation follows good practices with proper error handling, consistent use of extension functions, and clean separation of concerns. The suspend functions for async operations and synchronous methods for state checks provide a well-designed API.
app/src/main/java/com/darshan/notificity/ui/settings/SettingsActivity.kt (4)
73-73: Good integration of authentication into settings.The AuthViewModel is properly integrated to handle logout functionality.
89-94: Well-implemented logout flow.The logout sequence properly signs out the user, navigates to the sign-in screen, and finishes the current activity. This ensures a clean transition and prevents back navigation to authenticated screens.
207-218: Excellent user experience with confirmation dialog.The logout confirmation dialog prevents accidental sign-outs and follows good UX practices with clear messaging and destructive action styling.
311-374: Well-designed logout UI components.The LogoutSection and LogoutCard composables follow Material Design guidelines with proper error-themed styling, appropriate spacing, and consistent visual hierarchy.
app/src/main/java/com/darshan/notificity/auth/manager/AuthManager.kt (1)
26-97: Well-structured authentication manager with good separation of concerns.The AuthManager effectively centralizes authentication state management and provides a clean interface for user data operations. The methods are well-documented and follow consistent patterns.
app/src/main/java/com/darshan/notificity/ui/signin/SigninActivity.kt (1)
58-66: LGTM: Proper dependency injection setup.The activity is correctly annotated with
@AndroidEntryPointand uses proper ViewModel injection withby viewModels(). The screen name constant is appropriately set for analytics.app/src/main/java/com/darshan/notificity/main/ui/MainActivity.kt (3)
116-122: Good separation of concerns with loading state.The initial loading screen display while checking authentication is a good UX pattern. This prevents showing incomplete UI during auth state verification.
127-140: Potential race condition in auth state checking.The auth check flow uses
filterandfirst()which could potentially hang ifisAuthCheckednever becomes true due to an error incheckAuthState(). Consider adding a timeout or additional error handling.#!/bin/bash # Description: Check if AuthViewModel.checkAuthState() has proper error handling that ensures isAuthChecked is always set to true # Search for the checkAuthState implementation ast-grep --pattern $'fun checkAuthState() { $$$ }'
212-212: Verify toTitleCase extension function.The
toTitleCase()extension function is used for the user's name display. Ensure this function handles edge cases like null values, empty strings, and non-ASCII characters properly.#!/bin/bash # Description: Find and verify the toTitleCase extension function implementation # Search for toTitleCase function implementation ast-grep --pattern $'fun String.toTitleCase(): String { $$$ }' # Also search for nullable version rg -A 5 "toTitleCase"app/src/main/java/com/darshan/notificity/di/AuthModule.kt (2)
75-79: LGTM: Proper Firebase instance provision.Firebase instances are correctly provided as singletons using the standard
getInstance()methods. This ensures proper Firebase SDK initialization and resource management.
59-59: To confirm that none of theBaseAuthProviderimplementations depend onAuthManagerorAuthRepository, let’s list the interface and inspect each provider:#!/bin/bash # Locate BaseAuthProvider interface rg -n "interface BaseAuthProvider" -C3 app/src/main/java/com/darshan/notificity/auth # Find all provider implementations (exclude interface and module) and check for circular refs rg -l "BaseAuthProvider" app/src/main/java/com/darshan/notificity/auth \ | grep -v "BaseAuthProvider.kt" | grep -v "AuthModule.kt" \ | while read file; do echo "=== $file ===" rg -n -C3 "class " $file echo "→ References AuthManager:" rg -n "AuthManager" $file || echo " None" echo "→ References AuthRepository:" rg -n "AuthRepository" $file || echo " None" echo doneapp/src/main/java/com/darshan/notificity/ui/signin/AuthViewModel.kt (2)
101-116: Good abstraction with performAuthOperation.The
performAuthOperationfunction provides excellent code reuse and consistent error handling patterns for different authentication methods. This follows the DRY principle effectively.
29-62: Comprehensive auth state checking with proper error handling.The
checkAuthStatemethod properly manages loading states and handles both success and error scenarios. The try-catch block ensures the UI state is always updated even if exceptions occur.app/src/main/java/com/darshan/notificity/auth/providers/firebase/FirebaseGoogleAuthProvider.kt (3)
117-133: Good error handling with proper exception propagation.The
firebaseAuthWithGooglemethod correctly handles different exception types, preservesCancellationExceptionfor coroutine cancellation, and provides meaningful error messages.
94-110: Robust credential validation with proper type checking.The credential validation properly checks for Google ID token type and handles unexpected credential types gracefully with appropriate error messages.
71-71: Verify OAuth client ID configuration.The web client ID is retrieved from string resources. Ensure this is properly configured and not exposed in version control for security.
#!/bin/bash # Description: Check if web_client_id is properly configured and not hardcoded # Search for web_client_id in string resources fd -e xml . | xargs grep -l "web_client_id" # Check if it's defined in build config or external files rg "web_client_id" --type gradle
…ivity reference to prevent potential memory leaks
…der and pass it directly to methods
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
app/src/main/java/com/darshan/notificity/ui/signin/AuthViewModel.kt (2)
83-88: Potential data loss in sign-out success handling.When sign-out succeeds, the state is reset to a new
AuthUiState()instance, which could lose important state information. Consider preserving non-authentication-related state.is AuthResult.Success -> { _uiState.update { - AuthUiState().copy( + it.copy( + currentUser = null, isAuthChecked = true, - isAuthenticated = false + isAuthenticated = false, + isLoading = false, + error = null ) } }
142-146: Inconsistent state update pattern.The
handleAuthErrormethod uses direct assignment (_uiState.value = ...) instead of theupdatefunction used elsewhere. This could lead to race conditions or lost updates.private fun handleAuthError(errorMessage: String) { - _uiState.value = _uiState.value.copy( + _uiState.update { it.copy( isLoading = false, error = errorMessage - ) + ) } }
🧹 Nitpick comments (1)
app/src/main/java/com/darshan/notificity/auth/repository/AuthRepository.kt (1)
33-33: Usevalinstead ofvarfor immutable variables.Both
googleAuthProviderandanonymousAuthProviderare not reassigned after initialization.-var googleAuthProvider = providers[AuthType.GOOGLE] as GoogleOAuthProvider +val googleAuthProvider = providers[AuthType.GOOGLE] as GoogleOAuthProvider-var anonymousAuthProvider = providers[AuthType.ANONYMOUS] as AnonymousAuthProvider +val anonymousAuthProvider = providers[AuthType.ANONYMOUS] as AnonymousAuthProviderAlso applies to: 46-46
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/src/main/java/com/darshan/notificity/auth/providers/base/BaseAuthProvider.kt(1 hunks)app/src/main/java/com/darshan/notificity/auth/providers/firebase/FirebaseAnonymousAuthProvider.kt(1 hunks)app/src/main/java/com/darshan/notificity/auth/providers/firebase/FirebaseGoogleAuthProvider.kt(1 hunks)app/src/main/java/com/darshan/notificity/auth/repository/AuthRepository.kt(1 hunks)app/src/main/java/com/darshan/notificity/di/AuthModule.kt(1 hunks)app/src/main/java/com/darshan/notificity/ui/settings/SettingsActivity.kt(9 hunks)app/src/main/java/com/darshan/notificity/ui/signin/AuthViewModel.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- app/src/main/java/com/darshan/notificity/auth/providers/firebase/FirebaseAnonymousAuthProvider.kt
- app/src/main/java/com/darshan/notificity/auth/providers/base/BaseAuthProvider.kt
- app/src/main/java/com/darshan/notificity/ui/settings/SettingsActivity.kt
- app/src/main/java/com/darshan/notificity/di/AuthModule.kt
- app/src/main/java/com/darshan/notificity/auth/providers/firebase/FirebaseGoogleAuthProvider.kt
🔇 Additional comments (4)
app/src/main/java/com/darshan/notificity/auth/repository/AuthRepository.kt (1)
58-61: Good error handling pattern for sign-out operation.The null-safe handling of the current auth provider with appropriate error messaging follows good defensive programming practices.
app/src/main/java/com/darshan/notificity/ui/signin/AuthViewModel.kt (3)
30-63: Well-structured authentication state checking.The method properly handles loading states, error conditions, and updates the UI state appropriately with comprehensive error handling.
102-117: Excellent use of higher-order function for common auth operations.The
performAuthOperationhelper effectively reduces code duplication and provides consistent error handling across different authentication methods.
119-139: Good separation of success handling with proper error recovery.The method appropriately handles the additional step of fetching user data after authentication success and gracefully handles potential failures in user data retrieval.
…r reporting on auth providers.
- rename isAuthChecked to authCheckCompleted - add state change at missing place
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/main/java/com/darshan/notificity/auth/models/AuthUiState.kt(1 hunks)app/src/main/java/com/darshan/notificity/main/ui/MainActivity.kt(5 hunks)app/src/main/java/com/darshan/notificity/ui/signin/AuthViewModel.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/main/java/com/darshan/notificity/auth/models/AuthUiState.kt
- app/src/main/java/com/darshan/notificity/main/ui/MainActivity.kt
🔇 Additional comments (3)
app/src/main/java/com/darshan/notificity/ui/signin/AuthViewModel.kt (3)
1-28: LGTM! Well-structured authentication ViewModel setup.The class follows Android architecture best practices with proper dependency injection, reactive state management using StateFlow, and clear separation of concerns.
65-75: LGTM! Clean delegation pattern for sign-in operations.Both sign-in methods properly delegate to the shared
performAuthOperationhelper, maintaining consistency in error handling and state management.
144-154: LGTM! Clean error handling and utility methods.The error handling method and
clearError()utility follow consistent patterns and properly manage UI state.
| fun signOut(context: Context) { | ||
| viewModelScope.launch { | ||
| _uiState.update { it.copy(isLoading = true) } | ||
|
|
||
| when (val result = authRepository.signOut(context = context)) { | ||
| is AuthResult.Success -> { | ||
| _uiState.update { | ||
| it.copy( | ||
| currentUser = null, | ||
| authCheckCompleted = true, | ||
| isAuthenticated = false, | ||
| isLoading = false, | ||
| error = null | ||
| ) | ||
| } | ||
| } | ||
| is AuthResult.Error -> { | ||
| _uiState.update { | ||
| it.copy( | ||
| isLoading = false, | ||
| error = result.exception.message ?: "Sign out failed" | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent operation pattern for sign-out.
The signOut method doesn't use the performAuthOperation helper like other auth operations, creating inconsistency in the codebase and potentially different error handling behaviors.
Refactor to use the consistent pattern:
fun signOut(context: Context) {
- viewModelScope.launch {
- _uiState.update { it.copy(isLoading = true) }
-
- when (val result = authRepository.signOut(context = context)) {
- is AuthResult.Success -> {
- _uiState.update {
- it.copy(
- currentUser = null,
- authCheckCompleted = true,
- isAuthenticated = false,
- isLoading = false,
- error = null
- )
- }
- }
- is AuthResult.Error -> {
- _uiState.update {
- it.copy(
- isLoading = false,
- error = result.exception.message ?: "Sign out failed"
- )
- }
- }
- }
- }
+ performAuthOperation {
+ authRepository.signOut(context = context)
+ }
}Then handle the sign-out success case specifically in handleAuthSuccess():
private suspend fun handleAuthSuccess() {
try {
val userData = authRepository.getCurrentUserData()
+ val isSignedOut = userData == null
_uiState.update {
it.copy(
isLoading = false,
currentUser = userData,
- isAuthenticated = true,
+ isAuthenticated = !isSignedOut,
authCheckCompleted = true,
error = null
)
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/src/main/java/com/darshan/notificity/ui/signin/AuthViewModel.kt between
lines 77 and 103, refactor the signOut function to use the performAuthOperation
helper method for consistency with other auth operations. Replace the current
manual coroutine launch and state updates with a call to performAuthOperation
passing the signOut call. Then, override or extend handleAuthSuccess() to handle
the sign-out success case by updating the UI state accordingly, ensuring
consistent error handling and state management across all auth operations.
There was a problem hiding this comment.
@shivamsharma-1996 Understood! I'll ignore this refactor suggestion. The current implementation of the signOut method works as intended.
…e timestamp Replaced System.currentTimeMillis() with @ServerTimestamp and FieldValue.serverTimestamp() to ensure consistent, server-defined timestamps for createdAt and lastLoginAt.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/main/java/com/darshan/notificity/auth/models/User.kt(1 hunks)app/src/main/java/com/darshan/notificity/auth/repository/FirestoreUserRepository.kt(1 hunks)app/src/main/java/com/darshan/notificity/main/ui/MainActivity.kt(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/main/java/com/darshan/notificity/auth/repository/FirestoreUserRepository.kt
- app/src/main/java/com/darshan/notificity/main/ui/MainActivity.kt
🔇 Additional comments (1)
app/src/main/java/com/darshan/notificity/auth/models/User.kt (1)
15-18: Good improvement on timestamp handling!The use of
@ServerTimestampaddresses the synchronization concerns from the previous review by letting Firestore handle timestamp generation on the server side.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/src/main/java/com/darshan/notificity/ui/signin/AuthViewModel.kt (1)
77-103: Consider the sign-out state flag consistency.The
signOutmethod setsauthCheckCompleted = falseon success (line 86), which differs from other operations that typically set this totrue. While this might be intentional to trigger a fresh auth check after sign-out, consider documenting this behavior for clarity.is AuthResult.Success -> { _uiState.update { it.copy( currentUser = null, - authCheckCompleted = false, + authCheckCompleted = false, // Trigger fresh auth check after sign-out isAuthenticated = false, isLoading = false, error = null ) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/main/java/com/darshan/notificity/ui/signin/AuthViewModel.kt(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
app/src/main/java/com/darshan/notificity/ui/signin/AuthViewModel.kt (1)
Learnt from: shivamsharma-1996
PR: darshanpania/Notificity#42
File: app/src/main/java/com/darshan/notificity/ui/signin/AuthViewModel.kt:30-63
Timestamp: 2025-06-19T11:13:03.018Z
Learning: In AuthViewModel.kt checkAuthState() method, the error handling intentionally sets isAuthenticated = false when auth check fails. This is a security-conscious design decision to treat authentication verification failures as unauthenticated state rather than preserving potentially stale authentication status.
🔇 Additional comments (7)
app/src/main/java/com/darshan/notificity/ui/signin/AuthViewModel.kt (7)
18-25: LGTM: Clean ViewModel architecture with proper dependency injection.The class follows Android architecture best practices with Hilt dependency injection and clear separation of concerns.
27-28: LGTM: Proper StateFlow implementation for reactive UI state.The state management pattern correctly uses private MutableStateFlow with public StateFlow exposure, following recommended practices for UI state management.
30-63: LGTM: Security-conscious authentication state checking.Based on retrieved learnings, the error handling that sets
isAuthenticated = falseis intentionally designed for security, treating authentication verification failures as unauthenticated state.
65-75: LGTM: Consistent authentication operation pattern.Both sign-in methods properly delegate to the
performAuthOperationhelper, ensuring consistent error handling and state management across authentication operations.
105-120: LGTM: Well-structured helper method for authentication operations.The
performAuthOperationmethod provides consistent error handling and state management patterns across different authentication operations.
122-144: LGTM: Proper handling of authentication success with user data retrieval.The error handling correctly maintains
isAuthenticated = trueeven when user data retrieval fails, since authentication itself succeeded. This addresses the previous review concern about inconsistent state updates.
146-157: LGTM: Consistent state update patterns and utility functions.Both
handleAuthErrorandclearErrormethods properly use theupdatefunction for state management, maintaining consistency with the rest of the class.
…cessful login by adding a check on currentUser UI state
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/src/main/java/com/darshan/notificity/ui/signin/AuthViewModel.kt (1)
104-126: Consider adding loading state for sign-out operation.The
signOutmethod doesn't provide loading state feedback to users, unlike the sign-in methods. This could result in inconsistent UX where users don't receive visual feedback during sign-out operations.Consider adding loading state management:
fun signOut(context: Context) { viewModelScope.launch { + _uiState.update { it.copy(isLoading = true, error = null) } + when (val result = authRepository.signOut(context = context)) { is AuthResult.Success -> { _uiState.update { it.copy( currentUser = null, authCheckCompleted = false, isAuthenticated = false, + isLoading = false, error = null ) } } is AuthResult.Error -> { _uiState.update { it.copy( + isLoading = false, error = result.exception.message ?: "Sign out failed" ) } } } } }Note: You may need to add an
isLoadingproperty toAuthUiStateif it doesn't exist, or reuse one of the existing loading flags.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/src/main/java/com/darshan/notificity/auth/models/AuthUiState.kt(1 hunks)app/src/main/java/com/darshan/notificity/components/buttons/ActionButton.kt(1 hunks)app/src/main/java/com/darshan/notificity/ui/signin/AuthViewModel.kt(1 hunks)app/src/main/java/com/darshan/notificity/ui/signin/SigninActivity.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/main/java/com/darshan/notificity/components/buttons/ActionButton.kt
- app/src/main/java/com/darshan/notificity/ui/signin/SigninActivity.kt
🧰 Additional context used
🧠 Learnings (2)
app/src/main/java/com/darshan/notificity/auth/models/AuthUiState.kt (1)
Learnt from: shivamsharma-1996
PR: darshanpania/Notificity#42
File: app/src/main/java/com/darshan/notificity/auth/models/AuthUiState.kt:0-0
Timestamp: 2025-06-19T10:17:10.976Z
Learning: In AuthUiState data class, the `isAuthenticated` property is essential for MainActivity navigation logic. It works with the auth check completion flag to determine when auth check is completed and whether to navigate to SigninActivity. This property should not be removed even if it appears redundant with `currentUser != null` because it serves a specific purpose in the authentication flow state management.
app/src/main/java/com/darshan/notificity/ui/signin/AuthViewModel.kt (1)
Learnt from: shivamsharma-1996
PR: darshanpania/Notificity#42
File: app/src/main/java/com/darshan/notificity/ui/signin/AuthViewModel.kt:30-63
Timestamp: 2025-06-19T11:13:03.018Z
Learning: In AuthViewModel.kt checkAuthState() method, the error handling intentionally sets isAuthenticated = false when auth check fails. This is a security-conscious design decision to treat authentication verification failures as unauthenticated state rather than preserving potentially stale authentication status.
🔇 Additional comments (1)
app/src/main/java/com/darshan/notificity/auth/models/AuthUiState.kt (1)
1-26: Well-designed authentication state holder.The
AuthUiStatedata class is well-structured with comprehensive documentation and appropriate default values. The separation of loading states for different authentication methods (isGoogleLoading,isAnonymousLoading) provides granular UI control, and theauthCheckCompletedflag ensures proper lifecycle management for authentication flows.
…ocument in the User object Issue - Failure(java.lang.RuntimeException: Could not deserialize object. User does not define a no-argument constructor. If you are using ProGuard, make sure these constructors are not stripped) Issue has been fixed bt assigning a default null value to the user.id to let firebase deserialize fireStore document by creating User with empty constructor
2 participants
Closes Issue #10
Features Implemented:
Summary by CodeRabbit
New Features
Bug Fixes
Chores