MM-520 the transfer feature is implemented through two different navigation paths with inconsistent UI#3075
Conversation
📝 WalkthroughWalkthroughThis PR removes the Third Party Transfer (TPT) feature and its DI module, deletes TPT UI, ViewModel, and navigation helpers; updates authenticated/navbar navigation to stop wiring the TPT navigator; and redirects transfer-related navigation to the Make Transfer flow with minor MakeTransferViewModel and navigation adjustments. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…gation paths with inconsistent UI.
…gation paths with inconsistent UI.
Naman-kr404
force-pushed
the
MM-520-the-transfer-feature-is-implemented-through-two-different-navigation-paths-with-inconsistent-ui
branch
from
d7ab78f to
9524baf
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@cmp-navigation/src/commonMain/kotlin/cmp/navigation/authenticatednavbar/AuthenticatedNavBarTabItem.kt`:
- Line 25: Rename the typo'd property noNaviggationRoute to noNavigationRoute in
AuthenticatedNavBarTabItem and update all references (e.g., usages currently
referencing noNaviggationRoute at the two places noted) to the new identifier;
ensure any related imports/exports, string value stays the same
("__no_navigation__"), and run a quick build to catch any remaining references
to noNaviggationRoute so you replace them with noNavigationRoute.
🧹 Nitpick comments (6)
cmp-navigation/src/commonMain/kotlin/cmp/navigation/authenticatednavbar/AuthenticatedNavbarNavigation.kt (1)
20-20: Remove commented-out code instead of leaving it in place.Since the TPT feature is being fully removed from the codebase, the commented-out import, parameter, and argument should be deleted rather than left as comments. Commented-out code creates noise and confusion for future maintainers. Version control preserves history if this code is ever needed again.
♻️ Suggested cleanup
import org.mifos.mobile.core.ui.composableWithStayTransitions import org.mifos.mobile.feature.home.navigation.HomeNavigator -// import org.mifos.mobile.feature.third.party.transfer.navigation.TptNavigator `@Serializable` data object AuthenticatedNavbarRouteinternal fun NavGraphBuilder.authenticatedNavbarGraph( homeNavigator: HomeNavigator, -// tptNavigator: TptNavigator, ) { composableWithStayTransitions<AuthenticatedNavbarRoute> { AuthenticatedNavbarNavigationScreen( homeNavigator = homeNavigator, -// tptNavigator = tptNavigator, - ) } }Also applies to: 29-39
feature/transfer-process/src/commonMain/kotlin/org/mifos/mobile/feature/transfer/process/makeTransfer/MakeTransferViewModel.kt (3)
66-67: Remove commented-out debug code before merging.Lines 66-67 and 83-86 contain commented-out code with debug markers (e.g.,
🔥 FORCE initial load). This appears to be development/debugging artifacts that should be cleaned up.♻️ Proposed cleanup
-// accountId = route.accountId ?: -1L, accountId = route.accountId,init { observeNetworkStatus() - // 🔥 FORCE initial load -// viewModelScope.launch { -// fetchAccountOptions() -// } }Also applies to: 83-86
390-412: Remove large commented-out code block.This entire block appears to be an alternative implementation that was commented out during development. Dead code like this should be removed to maintain code clarity. If needed later, it can be retrieved from version control history.
♻️ Remove the commented block
-// private fun fetchAccountOptions() { -// //chatgpt -// if (state.accountId <= 0L) { -// fetchActiveAccount() -// return -// } -// updateState { -// it.copy(uiState = MakeTransferState.MakeTransferScreenState.Loading) -// } -// -// viewModelScope.launch { -// savingsAccountRepositoryImpl -// .accountTransferTemplate( -// accountId = state.accountId.takeIf { it != -1L } ?: 0L, -// accountType = 2L -// ) -// .collect { result -> -// sendAction( -// MakeTransferAction.Internal.ReceiveAccountOptionsTemplateResult(result) -// ) -// } -// } -// }
638-640: Usevalinstead ofvarfor data class properties.
accountOptionsTemplate,fromAccountOptions, andtoAccountOptionsare declared asvar, which breaks immutability of the state data class. State classes should be immutable; mutations should occur throughcopy().♻️ Proposed fix
- var accountOptionsTemplate: AccountOptionsTemplate = AccountOptionsTemplate(), - var fromAccountOptions: List<AccountOption> = emptyList(), - var toAccountOptions: List<AccountOption> = emptyList(), + val accountOptionsTemplate: AccountOptionsTemplate = AccountOptionsTemplate(), + val fromAccountOptions: List<AccountOption> = emptyList(), + val toAccountOptions: List<AccountOption> = emptyList(),cmp-navigation/src/commonMain/kotlin/cmp/navigation/authenticatednavbar/AuthenticatedNavbarNavigationScreen.kt (1)
185-196: Clarify therepeat(2)back-stack manipulation logic.The
repeat(2) { navController.popScreens() }is a magic number pattern that's unclear in intent. What if the back stack has fewer than 2 entries? This could lead to unexpected behavior or crashes.Consider:
- Adding a comment explaining why exactly 2 pops are needed.
- Using a more robust approach that handles edge cases gracefully.
#!/bin/bash # Find the popScreens implementation to verify safety ast-grep --pattern $'fun NavController.popScreens($$$) { $$$ }' # Alternative search rg -n "fun.*popScreens" --type=kt -A10cmp-navigation/src/commonMain/kotlin/cmp/navigation/di/KoinModules.kt (1)
74-74: Remove the commented-out module instead of leaving it.Since the Third Party Transfer feature is being removed, the commented-out line should be deleted entirely rather than left as a comment. Dead code should be removed to keep the codebase clean.
♻️ Proposed fix
TransferProcessModule, SettingsModule, -// ThirdPartyTransferModule, BeneficiaryModule,
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmp-navigation/src/commonMain/kotlin/cmp/navigation/authenticatednavbar/AuthenticatedNavBarTabItem.kt (1)
45-60: TransferTab will not display as selected and navigation bar visibility may fail due to sentinel route design.
TransferTabreturns"__no_navigation__"for bothgraphRouteandstartDestinationRoute, but the tab selection logic (line 122) usesisCurrentRoute(it.graphRoute)to determine which tab to highlight. Since"__no_navigation__"never matches any actual navigation destination in the hierarchy, the TransferTab will never appear as selected, even when actively viewing the MakeTransfer screen.Additionally, the navigation bar visibility check (line 141) uses
startDestinationRoute, which will also fail to match. Consider mappingTransferTabto an actual route that corresponds to the MakeTransfer screen destination, or add special handling inisCurrentRoute()and the visibility check for the sentinel route.
🧹 Nitpick comments (1)
cmp-navigation/src/commonMain/kotlin/cmp/navigation/authenticatednavbar/AuthenticatedNavBarTabItem.kt (1)
24-25: Consider moving constant to companion object.
noNavigationRouteis a constant string used as a sentinel value. Defining it as an instance property on the sealed class is unconventional; placing it in acompanion objectis more idiomatic for constants in Kotlin.♻️ Suggested refactor
sealed class AuthenticatedNavBarTabItem : NavigationItem { - val noNavigationRoute = "__no_navigation__" + companion object { + const val NO_NAVIGATION_ROUTE = "__no_navigation__" + } data object HomeTab : AuthenticatedNavBarTabItem() {Then update references in
TransferTab:override val graphRoute: String - get() = noNavigationRoute + get() = NO_NAVIGATION_ROUTE override val startDestinationRoute: String - get() = noNavigationRoute + get() = NO_NAVIGATION_ROUTE
|
Can you please update the pr description and put the images beside each other for easily comparing |
2 participants
Fixes - Jira-#520
Current Flow
Users can make transfers either via the Transfer tab in the bottom navigation or by navigating to Savings Account, selecting an account, and tapping Transfer.
Issue
Both flows perform the same transfer functionality but display different UI, causing duplication.
------------------------------------|----------------------------------------|
| | |
After (Via Transfer tab and saving account respectively, both are same) |
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.