Reply: Migrate from Navigation 2 to Navigation 3#1645
Reply: Migrate from Navigation 2 to Navigation 3#1645stevenelliottjr wants to merge 2 commits intoandroid:mainfrom
Conversation
Replace NavHost/NavController with NavDisplay and a developer-owned SnapshotStateList backstack. NavDestination-based route matching is replaced with direct Route equality checks on type-safe NavKey types. Fixes android#1632
stevenelliottjr
requested review from
dturner,
riggaroo and
simona-anomis
as code owners
Summary of ChangesHello @stevenelliottjr, 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 significantly refactors the application's navigation architecture by transitioning to the Navigation3 library. The core change involves replacing the existing Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe 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 by creating a comment using either
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 Limitations & Feedback Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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
|
![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 app from Jetpack Navigation 2 to Navigation 3, simplifying the navigation logic by replacing NavController and NavHost with a developer-owned SnapshotStateList for the backstack and a NavDisplay. The code is cleaner and the removal of ReplyNavigationActions is a good simplification. However, I've identified a critical issue in the new implementation within ReplyApp.kt where the navigation state (backStack) is not correctly hoisted. This breaks the existing mechanism for navigating to detail screens, as the callbacks for navigation are unable to modify the backstack.
| val navController = rememberNavController() | ||
| val navigationActions = remember(navController) { | ||
| ReplyNavigationActions(navController) | ||
| val backStack = remember { mutableStateListOf<Route>(Route.Inbox) } |
There was a problem hiding this comment.
The backStack is created and managed locally within ReplyApp, but the functions for navigating to and from detail screens (navigateToDetail, closeDetailScreen) are passed in from the caller. This prevents the caller from modifying the backStack to add or remove detail screen routes, which appears to be a regression from the previous navigation implementation.
To fix this, the backStack state should be hoisted. ReplyApp should receive the backStack as a parameter, allowing the caller to create, own, and modify it. This will re-enable navigation to detail screens.
There was a problem hiding this comment.
The detail screen navigation in Reply is entirely ViewModel-driven — `navigateToDetail` calls `viewModel.setOpenedEmail()` and `closeDetailScreen` calls `viewModel.closeDetailScreen()`, both of which mutate `ReplyHomeUIState` state. The detail view is rendered inline within `ReplyInboxScreen` based on `replyHomeUIState.isDetailOnlyOpen`, not as a separate navigation destination. This was also the case with Nav2 — the `NavHost` only had four top-level `composable<Route.*>` entries and no detail route.
So there's no regression here — the callers never modified the navigation backstack for detail screens, they only modify ViewModel state.
|
|
||
| val navigateToTopLevelDestination: (ReplyTopLevelDestination) -> Unit = { destination -> | ||
| if (backStack.lastOrNull() != destination.route) { | ||
| while (backStack.size > 1) backStack.removeLast() |
There was a problem hiding this comment.
Using a while loop to remove elements from the list is a bit imperative and can be less performant than bulk operations for large lists. A more declarative and efficient approach is to use subList().clear() to remove a range of elements.
This also makes the intent of clearing the stack down to the root more explicit.
| while (backStack.size > 1) backStack.removeLast() | |
| if (backStack.size > 1) backStack.subList(1, backStack.size).clear() |
There was a problem hiding this comment.
Applied — using subList(1, size).clear() now. Thanks.
Replace while loop with subList(1, size).clear() for cleaner intent and better performance on larger stacks.
1 participant
Summary
NavHost/NavControllerwith Nav3'sNavDisplayand a developer-ownedSnapshotStateListbackstackNavDestination-based route matching (hasRoute) with directRouteequality checks on type-safeNavKeytypesReplyNavigationActionsclass — navigation is now simple list operations on the backstackChanges
libs.versions.tomlnavigation3-runtime,navigation3-ui,lifecycle-viewmodel-navigation3build.gradle.ktsnavigation-composefor Nav3 depsReplyNavigationActions.ktRoutenow extendsNavKey, deletedReplyNavigationActionsclassReplyNavigationComponents.ktNavDestination?→Route?,hasRoute()→==comparisonReplyApp.ktNavHost→NavDisplaywithentryProviderlambda,rememberNavController()→mutableStateListOf<Route>()Test plan
./gradlew :Reply:app:assembleDebugbuilds successfullyFixes #1632