-
Notifications
You must be signed in to change notification settings - Fork 1
Google Auth - Integrate Auth into Navigation (Part 4) #4
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
Conversation
…ign out, minor change in empty HomeScreen
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.
Pull Request Overview
This PR integrates Google authentication into the app's navigation system, establishing the foundation for user sign-in functionality. The changes implement a complete authentication flow with proper navigation state management between authenticated and unauthenticated states.
- Added Firebase Google Sign-In dependencies and configuration
- Implemented onboarding navigation graph with sign-in screen
- Updated bottom navigation to use custom icons and added Messages tab
- Added navigation logic to handle authentication state transitions
Reviewed Changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| gradle/libs.versions.toml | Added Firebase, credential manager, and Google ID dependencies for authentication |
| app/src/main/res/values/strings.xml | Added Google OAuth client ID for authentication configuration |
| app/src/main/java/com/cornellappdev/hustle/ui/navigation/navgraphs/ProfileNavigation.kt | Added profile screen with sign-out functionality |
| app/src/main/java/com/cornellappdev/hustle/ui/navigation/navgraphs/OnboardingNavigation.kt | New navigation graph for authentication flow with sign-in screen |
| app/src/main/java/com/cornellappdev/hustle/ui/navigation/Routes.kt | Added onboarding routes and renamed Favorites to Messages |
| app/src/main/java/com/cornellappdev/hustle/ui/navigation/HustleNavigation.kt | Integrated authentication state management and conditional navigation |
| app/src/main/java/com/cornellappdev/hustle/ui/navigation/BottomNavigation.kt | Updated to use drawable resources instead of material icons and added Messages tab |
| app/build.gradle.kts | Added Firebase, authentication, and credential manager dependencies |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| LaunchedEffect(authUiState.isSignedIn) { | ||
| if (authUiState.isSignedIn) { | ||
| navController.navigate(HomeTab) { | ||
| popUpTo(0) { inclusive = true } | ||
| } | ||
| } else { | ||
| navController.navigate(Onboarding) { | ||
| popUpTo(0) { inclusive = true } | ||
| } | ||
| } | ||
| } |
Copilot
AI
Sep 26, 2025
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 LaunchedEffect will cause infinite navigation loops. When the user is already on HomeTab and signed in, or on Onboarding and signed out, this effect will still trigger navigation to the same destination, creating unnecessary navigation operations.
zachseidner1
left a comment
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.
Nice work, but we need to change the way we're injecting the VMs in this PR.
| fun NavGraphBuilder.onboardingNavGraph( | ||
| signInWithGoogle: () -> Unit, | ||
| isLoading: Boolean, | ||
| errorMessage: String?, | ||
| clearError: () -> Unit |
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.
The navGraph itself should not have any parameters if possible. We should only inject the view model in the screen where it is used. If AuthViewModel is the view model for sign in screen, we should inject it directly as a parameter in SignInScreen, with SignInScreen(viewModel: AuthViewModel = hiltViewModel()). Also, if AuthViewModel is for SignInScreen, it should be renamed to SignInViewModel for clarity.
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.
Ok that makes sense. My initial reasoning was because authviewmodel had some shared functions for multiple screens, but I think the functionality lives in the auth repository, so I could probably use it in separate viewmodels
| fun NavGraphBuilder.profileNavGraph( | ||
| user: User?, | ||
| onSignOut: () -> Unit, | ||
| isLoading: Boolean | ||
| ) { |
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.
yeah passing params like this that are specific to a screen in a navgraph doesn't make sense
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.
sounds good, will probably move them into profile vm then instead
app/src/main/java/com/cornellappdev/hustle/ui/navigation/HustleNavigation.kt
Outdated
Show resolved
Hide resolved
| val authUiState = authViewModel.collectUiStateValue() | ||
| val startDestination = if (authUiState.isSignedIn) HomeTab else Onboarding |
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.
oh I see why you have this. I think for authenticated apps, it's more typical to have an initial loading screen and then just navigate to the right screen from there, or just always start on the Onboarding screen but have a LaunchedEffect that navigates to the correct screen once the user is signed in. This way you don't need authViewModel in HustleNavigation. Lmk if that makes sense.
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.
I want to double check with you about this part. I think I agree that signinscreen could have its own view model with auth vm being renamed signin vm, but I'm trying to see if there is a better way to make sure that the user starts on the home screen if they logged in beforehand since I don't know if it is the best user experience if they always see the flicker from starting on the onboarding (or splash) into the home screen. I guess it's not the biggest deal lol, but figured it would be preferable to remove the flicker. I was wondering if it was possible if we had a root or main vm that deals with this sort of situation since authrepository can be shared among the vms or if there is a better practice we should follow?
| onboardingNavGraph( | ||
| signInWithGoogle = authViewModel::signInWithGoogle, | ||
| isLoading = authUiState.actionState == ActionState.Loading, | ||
| errorMessage = when (authUiState.actionState) { | ||
| is ActionState.Error -> authUiState.actionState.message | ||
| else -> null | ||
| }, | ||
| clearError = authViewModel::clearActionState | ||
|
|
||
| ) | ||
| homeNavGraph(navController = navController) | ||
| favoritesNavGraph(navController = navController) | ||
| profileNavGraph(navController = navController) | ||
| messagesNavGraph(navController = navController) | ||
| profileNavGraph( | ||
| user = authUiState.user, | ||
| onSignOut = authViewModel::signOut, | ||
| isLoading = authUiState.actionState == ActionState.Loading | ||
| ) |
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.
If you need to share actions across screens, those should probably be extracted to a repository. We should really only have 1 VM per screen, just to keep things simple and clear.
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.
I think based on the previous comments, this will be resolved once we make it so the sign in and profile screens have their own vms that both use the auth repository functions as needed
|
I made separate vms for the respective screens and removed parameters from the navgraphs except for navcontroller. For the auth viewmodel in navigation composable issue, I ultimately went with the approach of having a root vm in mainactivity that has loading and signedin fields, where the loading field is used to determine how long the splash screen should show, and the signedin field is passed down eventually into the navigation composable to determine the starting destination so we can have the splash screen --> home screen directly if the user signed in already instead of splash screen --> sign in screen (briefly) --> home screen. Let me know if you have any questions about the approach or if there are some things that might not follow best practice with this approach. Wasn't 100% sure if this was the most ideal approach. |
zachseidner1
left a comment
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 is a nice solution! I appreciate you figuring out something on your own to achieve the UX you wanted. This works well.
| import com.cornellappdev.hustle.ui.viewmodels.HustleViewModel | ||
| import kotlinx.coroutines.launch | ||
|
|
||
| fun <UiState> HustleViewModel<UiState>.executeAuthAction( |
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 function feels almost general enough that it could be its own executeActionStatefully, and just change the error message to something generic, like "An unknown error occurred". Then we could reuse it more later in the code. I recommend this name since it captures that we are modifying a state when executing the action.
Overview
Integrated auth changes into navigation code
Changes Made
Test Coverage
Next Steps
Related PRs or Issues
Screenshots
Screen.Recording.2025-09-26.at.3.32.38.PM.mov
Notes