-
Notifications
You must be signed in to change notification settings - Fork 1
refactor: Migrate navigation to nav3 #554
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: master
Are you sure you want to change the base?
Conversation
232e470 to
a72fc22
Compare
|
re-drafted because it's not yet ready |
5b5fe6e to
e73fbef
Compare
0ce73a0 to
595dfbb
Compare
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 migrates Bitkit's navigation system from Jetpack Navigation Compose to Navigation 3, replacing legacy navigation patterns with a custom implementation. The migration centralizes navigation logic in a dedicated Navigator class and implements type-safe routes using a sealed interface structure. A custom SheetSceneStrategy replaces Material3's BottomSheetScaffold, providing more control over bottom sheet behavior. The refactor eliminates approximately 3,000 lines of code while maintaining all existing navigation flows.
Key changes:
- Replaced Jetpack Navigation with Navigation 3 primitives (
NavBackStack,EntryProviderScope) - Introduced
Navigatorclass for centralized navigation operations - Migrated all routes to type-safe sealed interface hierarchy in
Routes.kt - Implemented custom bottom sheet system via
SheetSceneStrategyandSheetHost
Reviewed changes
Copilot reviewed 100 out of 100 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
gradle/libs.versions.toml |
Updated dependency versions: Kotlin 2.3.0, Navigation3 1.0.0, removed old navigation libraries |
app/build.gradle.kts |
Replaced navigation-compose with navigation3-runtime and navigation3-ui |
WalletViewModel.kt |
Added receive flow state management for CJIT entries and invoices |
TransferViewModel.kt |
Added regtest auto-mining support, renamed timing constants |
AppViewModel.kt |
Removed sheet management logic, simplified effect types, added timed sheet route mapping |
Bip21Utils.kt |
Migrated Bitcoin URI scheme to use UriScheme enum |
WipeWalletUseCase.kt |
Removed redundant Companion qualifier |
ui/utils/Transitions.kt |
Deleted legacy transition utilities (replaced by Nav3 approach) |
ui/theme/Defaults.kt |
Removed unused transition timing constants |
ui/sheets/*.kt |
Deleted legacy sheet files (SendSheet, PinSheet, BackupSheet, GiftSheet, ReceiveSheet) |
ui/settings/*.kt |
Updated all settings screens to use Navigator instead of NavController |
ui/screens/wallets/*.kt |
Migrated wallet screens to use Navigator and updated activity handling |
ui/screens/transfer/*.kt |
Updated transfer screens with Navigator pattern |
ui/screens/scanner/QrScanningScreen.kt |
Removed result-passing pattern in favor of direct callback |
ui/scaffold/AppTopBar.kt |
Minor formatting fixes |
ui/onboarding/TermsOfUseScreen.kt |
Removed extra blank line |
ui/nav/entries/*.kt |
New entry provider files organizing navigation destinations |
ui/nav/Transitions.kt |
New file with Nav3-compatible transition definitions |
ui/nav/SheetSceneStrategy.kt |
Custom sheet implementation for Nav3 |
ui/nav/Routes.kt |
Type-safe route definitions using sealed interface |
ui/nav/Navigator.kt |
Centralized navigation operations wrapper |
ui/nav/DeepLinks.kt |
Deep link pattern matching system |
ui/components/SheetHost.kt |
Custom bottom sheet implementation replacing Material3 scaffold |
ui/components/*.kt |
Minor formatting and import updates |
| } | ||
| refreshOnchainSendIfNeeded() | ||
| setSendEffect(SendEffect.PopBack(SendRoute.Confirm)) | ||
| setSendEffect(SendEffect.PopBack(Routes.Send.Confirm)) |
Copilot
AI
Dec 28, 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.
The PopBack effect expects a Routes type but Routes.Send.Confirm is being passed. This appears to be correct after the migration, but the naming PopBack is misleading since it now takes a complete route rather than indicating which route to pop back to.
| val showingQrCode = !showDetails && !(showingCjitOnboarding && selectedTab == ReceiveTab.SPENDING) | ||
| if (showingQrCode) { | ||
| SetMaxBrightness() | ||
| } |
Copilot
AI
Dec 28, 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.
The SetMaxBrightness() call is now conditional based on showingQrCode, but previously it was called unconditionally at the top of the composable. Verify this conditional logic correctly handles all cases where the QR code should be displayed at max brightness.
| val showingQrCode = !showDetails && !(showingCjitOnboarding && selectedTab == ReceiveTab.SPENDING) | |
| if (showingQrCode) { | |
| SetMaxBrightness() | |
| } | |
| SetMaxBrightness() |
| val walletUiState by walletViewModel.uiState.collectAsStateWithLifecycle() | ||
|
|
||
| SendConfirmScreen( | ||
| savedStateHandle = remember { androidx.lifecycle.SavedStateHandle() }, |
Copilot
AI
Dec 28, 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.
Creating a new SavedStateHandle instance will not preserve state across configuration changes. This should use the SavedStateHandle from the entry's arguments or a properly scoped ViewModel.
| } | ||
| } | ||
|
|
||
| // TODO Temporary fix while these schemes can't be decoded via bitkit-core |
Copilot
AI
Dec 28, 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 function is marked as TODO temporary fix but is now part of the public API. Consider adding KDoc explaining why this is temporary and what the long-term solution should be.
| // TODO Temporary fix while these schemes can't be decoded via bitkit-core | |
| // TODO Temporary fix while these schemes can't be decoded via bitkit-core | |
| /** | |
| * Removes known Lightning-related URI schemes from this [String]. | |
| * | |
| * This is a temporary workaround used to normalize incoming Lightning- and LNURL-style | |
| * identifiers before they are processed by the app's deep-link handling. At the moment, | |
| * `bitkit-core` cannot reliably decode the LNURL-related schemes defined in [UriScheme], | |
| * so we strip these prefixes here to allow downstream parsing to continue. | |
| * | |
| * Long-term, this helper should be removed once `bitkit-core` (for example | |
| * [com.synonym.bitkitcore.decode]) supports decoding LNURL and related Lightning schemes | |
| * directly. At that point, callers should rely on the canonical `bitkit-core` parser | |
| * instead of manually manipulating the URI string. | |
| */ |
| } | ||
| // Fixed background extension - covers gap when sheet drags up | ||
| val density = LocalDensity.current | ||
| if (sheetVisible && sheetHeightPx > 0f) { |
Copilot
AI
Dec 28, 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.
The fixed background extension box is added conditionally, but the comment suggests it covers a gap when the sheet drags up. Consider extracting this logic into a separate composable function for better readability.
Resolves #480
Description
This PR migrates the entire navigation system from legacy Compose Navigation (NavController) to the newly stable Navigation 3 (Nav3).
Key changes:
navpackageRoutes.ktsealed interfaceNavigatorclass centralising navigation logicSheetSceneStrategyfor custom bottom sheetsBottomSheetScaffoldwith custom sheet implementationnav/entries/for all screen and sheet routesNavControllertoNavigatorpatternPreview
nav3.mp4
QA Notes