Migrate sample to compose #173
Conversation
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully migrates the CredentialManager sample app from the traditional View system (Fragments and XML) to Jetpack Compose. The migration is comprehensive, replacing all UI components, navigation, and adapting the logic to a ViewModel-based architecture. The code is now more modern and follows many Compose best practices.
However, there are a few significant areas for improvement. The most critical is the downgrading of several key AndroidX and Kotlin libraries in build.gradle, which is a regression and should be reverted. Additionally, there are architectural concerns, primarily passing Activity and Context instances into ViewModels, which violates MVVM principles and can lead to memory leaks. I've also left comments on improving navigation logic, state handling, and making Compose Previews more robust.
Overall, this is a great step forward for the sample app. Addressing the feedback will make it an even better example of a modern Android application.
| Button( | ||
| onClick = { | ||
| if (activity != null) { | ||
| viewModel.signIn(activity, context) | ||
| } | ||
| }, | ||
| shape = RoundedCornerShape(4.dp), | ||
| enabled = !isLoading && activity != null, | ||
| modifier = Modifier.fillMaxWidth() | ||
| ) { |
There was a problem hiding this comment.
Passing the Activity or Context to the ViewModel (viewModel.signIn(activity, context)) is a violation of MVVM architecture. ViewModels should not hold references to UI-layer components, as this can lead to memory leaks and makes them difficult to test.
The CredentialManager API requires an Activity, so the call to credentialManager.getCredential should be initiated from the UI layer (e.g., inside this onClick lambda). The result can then be passed to the ViewModel for processing.
| private val _navigationEvent = MutableSharedFlow<NavigationEvent>() | ||
| val navigationEvent = _navigationEvent.asSharedFlow() | ||
|
|
||
| fun signIn(activity: Activity, context: Context) { |
There was a problem hiding this comment.
The signIn function accepts Activity and Context parameters. This tightly couples the ViewModel to the Android framework, making it difficult to unit test and potentially causing memory leaks. The responsibility of interacting with framework components like CredentialManager (which requires an Activity) should be handled in the UI layer (the Composable screen), and only the results (or data) should be passed to the ViewModel for processing.
CredentialManager/app/src/main/java/com/google/credentialmanager/sample/SignUpScreen.kt
Show resolved
Hide resolved
| Button( | ||
| onClick = { | ||
| if (activity != null) { | ||
| viewModel.signUpWithPasskey(activity, context) | ||
| } | ||
| }, | ||
| shape = RoundedCornerShape(4.dp), | ||
| enabled = !isLoading && activity != null, | ||
| modifier = Modifier.fillMaxWidth() | ||
| ) { | ||
| Text("Sign Up with passkey") | ||
| } |
There was a problem hiding this comment.
Passing the Activity and Context to the ViewModel (viewModel.signUpWithPasskey(activity, context)) is a violation of MVVM architecture. ViewModels should not hold references to UI-layer components, as this can lead to memory leaks and makes them difficult to test. The CredentialManager API call should be initiated from the UI layer (e.g., inside this onClick lambda), and the result can then be passed to the ViewModel for processing.
CredentialManager/app/src/main/java/com/google/credentialmanager/sample/HomeScreen.kt
Show resolved
Hide resolved
CredentialManager/app/src/main/java/com/google/credentialmanager/sample/HomeScreen.kt
Outdated
Show resolved
Hide resolved
CredentialManager/app/src/main/java/com/google/credentialmanager/sample/SignInScreen.kt
Show resolved
Hide resolved
CredentialManager/app/src/main/java/com/google/credentialmanager/sample/SignInScreen.kt
Outdated
Show resolved
Hide resolved
CredentialManager/app/src/main/java/com/google/credentialmanager/sample/SignUpScreen.kt
Outdated
Show resolved
Hide resolved
No description provided.