-
Notifications
You must be signed in to change notification settings - Fork 37
[WIP] Add recipe to show Nav2 to Nav3 migration plan #46
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
base: main
Are you sure you want to change the base?
Conversation
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.
Summary of Changes
Hello @dturner, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces a new recipe to guide developers through the migration process from Android's Navigation 2 to Navigation 3. It sets up a foundational Navigation 2 example and then demonstrates the initial step of integrating Navigation 3 components while retaining the existing Navigation 2 structure.
Highlights
- New Migration Recipe: Introduced a new 'recipe' to the project specifically designed to illustrate the migration path from Android's Navigation 2 to Navigation 3.
- Incremental Migration Steps: Added two distinct
MainActivity
implementations (start
andstep1
) to represent different stages of the migration. Thestart
activity provides a basic Navigation 2 setup, while thestep1
activity shows how to begin integrating Navigation 3'sNavDisplay
by bridging it with the existing Navigation 2NavHost
. - Dependency Management: Updated various Kotlin and serialization dependencies and explicitly added the Navigation 2 library to support the migration example.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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.
Code Review
This pull request introduces a new recipe to demonstrate a migration path from Navigation 2 to Navigation 3. It adds a 'start' state using Nav2 and a 'step1' state showing how to embed a Nav2 NavHost
within a Nav3 NavDisplay
.
The code is a good starting point, but I've identified a few areas for improvement:
- There is a critical memory leak due to improper
CoroutineScope
usage inNav3NavigatorSimple
. I've provided suggestions to fix this using a lifecycle-aware scope fromrememberCoroutineScope
. - I've also suggested replacing a
println
call with standard Android logging and improving a brittle check on a class name for better maintainability.
setEdgeToEdgeConfig() | ||
super.onCreate(savedInstanceState) | ||
setContent { | ||
val navController = rememberNavController() |
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.
To ensure proper lifecycle management and prevent memory leaks, a CoroutineScope
should be created using rememberCoroutineScope()
and passed to Nav3NavigatorSimple
. This scope is lifecycle-aware and will be cancelled when the composable leaves the composition. This declaration should precede the nav3Navigator
initialization. Don't forget to add import androidx.compose.runtime.rememberCoroutineScope
.
val navController = rememberNavController() | |
val coroutineScope = rememberCoroutineScope() |
super.onCreate(savedInstanceState) | ||
setContent { | ||
val navController = rememberNavController() | ||
val nav3Navigator = remember { Nav3NavigatorSimple(navController) } |
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 Nav3NavigatorSimple
should receive a lifecycle-aware CoroutineScope
from its constructor to prevent memory leaks. This ensures that any coroutines launched within Nav3NavigatorSimple
are properly cancelled when the MainActivity
composable is disposed. This change is paired with the declaration of coroutineScope
.
val nav3Navigator = remember { Nav3NavigatorSimple(navController) } | |
val nav3Navigator = remember { Nav3NavigatorSimple(navController, coroutineScope) } |
class Nav3NavigatorSimple(val navController: NavHostController){ | ||
|
||
val coroutineScope = CoroutineScope(Job()) |
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.
Using CoroutineScope(Job())
creates a scope that is not tied to any lifecycle, which causes memory leaks. The coroutine launched in init
will never be cancelled. This class should receive a lifecycle-aware CoroutineScope
from its creator (the MainActivity
composable in this case) via its constructor. This change is paired with another one in MainActivity
where the scope is created and passed.
class Nav3NavigatorSimple(val navController: NavHostController){ | |
val coroutineScope = CoroutineScope(Job()) | |
class Nav3NavigatorSimple( | |
val navController: NavHostController, | |
private val coroutineScope: CoroutineScope | |
) { |
NavDisplay( | ||
backStack = nav3Navigator.backStack, | ||
entryProvider = entryProvider(fallback = { key -> | ||
println("Key $key not handled by entryProvider, using fallback") |
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.
Using println
for logging is generally discouraged in Android applications. It's better to use the Log
class (e.g., Log.d(TAG, ...)
), which allows for filtering by tag and log level. While this might be acceptable for a simple recipe, using Log
is a better practice to demonstrate.
println("Key $key not handled by entryProvider, using fallback") | |
android.util.Log.d("Nav3Migration", "Key $key not handled by entryProvider, using fallback") |
if (entry.destination::class.qualifiedName == "androidx.navigation.compose.ComposeNavGraphNavigator.ComposeNavGraph"){ | ||
null | ||
} else { | ||
entry | ||
} |
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.
Checking the qualified name of the destination class is brittle and can break with library updates. A more robust way to identify and ignore the root navigation graph entry is to check if its parent
is null
.
if (entry.destination.parent == null) {
// Ignore nav graph root entries
null
} else {
entry
}
No description provided.