Release/3.0.3 - #98
Conversation
…ose navigation modules
Develop/update gradle
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Code Review
This pull request updates the project version to 3.0.3, bumps the compile SDK to 37, and introduces Jetpack Compose support for Fragments and Navigation (including FrogoComposeFragment and FrogoComposeNavActivity) along with unit tests and sample implementations. The review feedback highlights several improvement opportunities: avoiding the use of a protected lateinit var for the NavController, removing an unused and directly instantiated ViewModel in SampleFragment, using LocalContext.current instead of the nullable Fragment context inside Composables, removing a redundant onCreateExt override, and centralizing the navigation-compose version in the Version Catalog.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| abstract class FrogoComposeNavActivity : FrogoComposeActivity() { | ||
|
|
||
| protected lateinit var navController: NavHostController | ||
|
|
||
| @Composable | ||
| override fun SetupCompose() { | ||
| navController = rememberNavController() | ||
| SetupNavigation(navController) | ||
| } |
There was a problem hiding this comment.
Storing navController as a protected lateinit var in the Activity class is risky and can lead to UninitializedPropertyAccessException if accessed before composition occurs (e.g., during onCreate or other lifecycle methods). Additionally, keeping a reference to NavController in the Activity can lead to state inconsistencies or memory leaks.
Since SetupNavigation already receives the navController as a parameter, we can completely eliminate the class property and use a local variable inside SetupCompose.
abstract class FrogoComposeNavActivity : FrogoComposeActivity() {
@Composable
override fun SetupCompose() {
val navController = rememberNavController()
SetupNavigation(navController)
}| private lateinit var sampleViewModel: SampleViewModel | ||
|
|
||
| override fun setupViewModel() { | ||
| super.setupViewModel() | ||
| sampleViewModel = SampleViewModel() | ||
| } |
There was a problem hiding this comment.
The sampleViewModel property is declared and directly instantiated via SampleViewModel(), which is an anti-pattern in Android (ViewModels should be managed by the lifecycle/ViewModelStore, not instantiated directly). Furthermore, this property is completely unused because setupCompose correctly retrieves the ViewModel using the viewModel() delegate. We should remove this redundant and incorrect initialization.
| @Composable | ||
| override fun setupCompose() { | ||
| val vm: SampleViewModel = viewModel() | ||
| val state by vm.uiState.collectAsState() | ||
|
|
||
| Column( | ||
| modifier = Modifier | ||
| .fillMaxSize() | ||
| .padding(16.dp), | ||
| verticalArrangement = Arrangement.Center, | ||
| horizontalAlignment = Alignment.CenterHorizontally | ||
| ) { | ||
| Text(text = "Sample Fragment Compose", style = MaterialTheme.typography.headlineMedium) | ||
| Spacer(modifier = Modifier.height(8.dp)) | ||
| Text(text = state.description, style = MaterialTheme.typography.bodyMedium) | ||
| Spacer(modifier = Modifier.height(16.dp)) | ||
| Text(text = "Counter: ${state.counter}", style = MaterialTheme.typography.titleLarge) | ||
| Spacer(modifier = Modifier.height(16.dp)) | ||
| Button(onClick = { | ||
| vm.incrementCounter() | ||
| Toast.makeText(context, "Clicked from Compose Fragment!", Toast.LENGTH_SHORT).show() | ||
| }) { | ||
| Text("Increment Counter") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
In Jetpack Compose, the idiomatic way to obtain the Android Context is by using LocalContext.current. Relying on the Fragment's context property directly can be unsafe because it is nullable (Context?) and can lead to crashes if the Fragment is detached when the click action is triggered.
@Composable
override fun setupCompose() {
val vm: SampleViewModel = viewModel()
val state by vm.uiState.collectAsState()
val context = androidx.compose.ui.platform.LocalContext.current
Column(
modifier = Modifier
.fillMaxSize()
.padding(16.dp),
verticalArrangement = Arrangement.Center,
horizontalAlignment = Alignment.CenterHorizontally
) {
Text(text = "Sample Fragment Compose", style = MaterialTheme.typography.headlineMedium)
Spacer(modifier = Modifier.height(8.dp))
Text(text = state.description, style = MaterialTheme.typography.bodyMedium)
Spacer(modifier = Modifier.height(16.dp))
Text(text = "Counter: ${state.counter}", style = MaterialTheme.typography.titleLarge)
Spacer(modifier = Modifier.height(16.dp))
Button(onClick = {
vm.incrementCounter()
Toast.makeText(context, "Clicked from Compose Fragment!", Toast.LENGTH_SHORT).show()
}) {
Text("Increment Counter")
}
}
}| @Composable | ||
| override fun setupCompose() { | ||
| Column( | ||
| modifier = Modifier | ||
| .fillMaxWidth() | ||
| .background( | ||
| color = MaterialTheme.colorScheme.surface, | ||
| shape = RoundedCornerShape(topStart = 16.dp, topEnd = 16.dp) | ||
| ) | ||
| .padding(24.dp), | ||
| horizontalAlignment = Alignment.CenterHorizontally | ||
| ) { | ||
| Text( | ||
| text = "Sample Bottom Sheet Fragment", | ||
| style = MaterialTheme.typography.titleLarge, | ||
| color = MaterialTheme.colorScheme.onSurface | ||
| ) | ||
| Spacer(modifier = Modifier.height(8.dp)) | ||
| Text( | ||
| text = "This bottom sheet UI is built entirely using Jetpack Compose hosted inside a BottomSheetDialogFragment.", | ||
| style = MaterialTheme.typography.bodyMedium, | ||
| color = MaterialTheme.colorScheme.onSurfaceVariant | ||
| ) | ||
| Spacer(modifier = Modifier.height(16.dp)) | ||
| Button(onClick = { | ||
| Toast.makeText(context, "Action from Bottom Sheet!", Toast.LENGTH_SHORT).show() | ||
| dismiss() | ||
| }) { | ||
| Text("Dismiss Sheet") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Using the Fragment's nullable context property directly inside a Composable is discouraged. Instead, use LocalContext.current to safely retrieve the non-null Context associated with the composition tree.
@Composable
override fun setupCompose() {
val context = androidx.compose.ui.platform.LocalContext.current
Column(
modifier = Modifier
.fillMaxWidth()
.background(
color = MaterialTheme.colorScheme.surface,
shape = RoundedCornerShape(topStart = 16.dp, topEnd = 16.dp)
)
.padding(24.dp),
horizontalAlignment = Alignment.CenterHorizontally
) {
Text(
text = "Sample Bottom Sheet Fragment",
style = MaterialTheme.typography.titleLarge,
color = MaterialTheme.colorScheme.onSurface
)
Spacer(modifier = Modifier.height(8.dp))
Text(
text = "This bottom sheet UI is built entirely using Jetpack Compose hosted inside a BottomSheetDialogFragment.",
style = MaterialTheme.typography.bodyMedium,
color = MaterialTheme.colorScheme.onSurfaceVariant
)
Spacer(modifier = Modifier.height(16.dp))
Button(onClick = {
Toast.makeText(context, "Action from Bottom Sheet!", Toast.LENGTH_SHORT).show()
dismiss()
}) {
Text("Dismiss Sheet")
}
}
}| class SampleNavComposeActivity : FrogoComposeNavActivity() { | ||
|
|
||
| override fun onCreateExt(savedInstanceState: Bundle?) { | ||
| super.onCreateExt(savedInstanceState) | ||
| // Additional configuration during onCreate if needed | ||
| } | ||
|
|
||
| @Composable | ||
| override fun SetupNavigation(navController: NavHostController) { |
There was a problem hiding this comment.
The onCreateExt method is overridden but only calls super.onCreateExt(savedInstanceState) without adding any custom logic. This redundant override can be safely removed to keep the codebase clean.
| class SampleNavComposeActivity : FrogoComposeNavActivity() { | |
| override fun onCreateExt(savedInstanceState: Bundle?) { | |
| super.onCreateExt(savedInstanceState) | |
| // Additional configuration during onCreate if needed | |
| } | |
| @Composable | |
| override fun SetupNavigation(navController: NavHostController) { | |
| class SampleNavComposeActivity : FrogoComposeNavActivity() { | |
| @Composable | |
| override fun SetupNavigation(navController: NavHostController) { |
| androidx-lifecycle-runtime-compose = { group = "androidx.lifecycle", name = "lifecycle-runtime-compose", version.ref = "liveCycle" } | ||
| androidx-compose-foundation = { group = "androidx.compose.foundation", name = "foundation" } | ||
| androidx-compose-material-icons-extended = { group = "androidx.compose.material", name = "material-icons-extended" } | ||
| androidx-navigation-compose = { group = "androidx.navigation", name = "navigation-compose", version = "2.8.5" } |
There was a problem hiding this comment.
It is best practice to define all dependency versions in the [versions] block of the Version Catalog (libs.versions.toml) rather than hardcoding them directly in the [libraries] block. This ensures centralized version management.
| androidx-navigation-compose = { group = "androidx.navigation", name = "navigation-compose", version = "2.8.5" } | |
| androidx-navigation-compose = { group = "androidx.navigation", name = "navigation-compose", version.ref = "navigation" } |
No description provided.