Connect saving account item list to saving account detail Page #2536
Conversation
WalkthroughThe expanded/collapse state for MifosActionsSavingsListingComponent was moved from internal component state to external props ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Screen
participant Component as MifosActionsSavingsListingComponent
participant ViewModel
participant Navigation
rect rgb(220,245,220)
Note over User,Screen: Expand/collapse (per-item)
User->>Screen: Click header
Screen->>Screen: Update expandedIndex (rememberSaveable)
Screen->>Component: render with isExpanded, onExpandToggle
Component->>Component: AnimatedVisibility driven by isExpanded
end
rect rgb(245,245,220)
Note over User,ViewModel: Approve flow
User->>Component: Tap Approve
Component->>Screen: emit Approve action (accountId)
Screen->>ViewModel: handle ApproveAccount(accountId)
ViewModel->>Navigation: navigateToApproveAccount(accountId)
end
rect rgb(245,220,245)
Note over User,ViewModel: View flow with type
User->>Component: Tap View
Component->>Screen: emit View action (accountId, depositType)
Screen->>ViewModel: handle ViewAccount(accountId, depositType)
ViewModel->>Navigation: navigateToViewAccount(accountId, depositType)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
|
@Arinyadav1 resolve the conflicts and let me know i will approve |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsViewModel.kt (1)
70-74: Critical: ViewAccount action missing account identifier.The
ViewAccountaction is defined as adata object(line 161) and carries no parameters, yet this handler needs to know which loan account the user selected. Currently, onlystate.clientIdis passed to the event, but there's no way to specify which loan account from the list should be viewed.This prevents navigation to a specific account's detail page.
Apply this diff to fix the action and handler:
- is ClientLoanAccountsAction.ViewAccount -> sendEvent( + is ClientLoanAccountsAction.ViewAccount -> sendEvent( ClientLoanAccountsEvent.ViewAccount( - state.clientId, + action.accountId, ), )And update the action definition at line 161:
- data object ViewAccount : ClientLoanAccountsAction + data class ViewAccount(val accountId: Int) : ClientLoanAccountsActionYou may also need to verify that the
ClientLoanAccountsEvent.ViewAccountevent carries the correct parameters for navigation (e.g., bothclientIdandaccountIdif both are required by the detail screen).feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientProfile/ClientProfileNavigation.kt (1)
24-46: Wire or remove the unused parameter.The
navigateToAddChargeparameter is accepted but never passed toClientProfileScreen, creating dead code. Either wire it through to the screen or remove it from the function signature if it's not yet needed.If the parameter should be wired, apply this diff:
fun NavGraphBuilder.clientProfileDestination( onNavigateBack: () -> Unit, navController: NavController, notes: (Int) -> Unit, documents: (Int) -> Unit, identifiers: (Int) -> Unit, navigateToClientDetailsScreen: (Int) -> Unit, viewAddress: (Int) -> Unit, viewAssociatedAccounts: (Int) -> Unit, navigateToAddCharge: (Int) -> Unit, ) { composable<ClientProfileRoute> { ClientProfileScreen( notes = notes, documents = documents, identifiers = identifiers, onNavigateBack = onNavigateBack, navigateToClientDetailsScreen = navigateToClientDetailsScreen, viewAddress = viewAddress, viewAssociatedAccounts = viewAssociatedAccounts, navController = navController, + navigateToAddCharge = navigateToAddCharge, ) } }Otherwise, remove the parameter:
fun NavGraphBuilder.clientProfileDestination( onNavigateBack: () -> Unit, navController: NavController, notes: (Int) -> Unit, documents: (Int) -> Unit, identifiers: (Int) -> Unit, navigateToClientDetailsScreen: (Int) -> Unit, viewAddress: (Int) -> Unit, viewAssociatedAccounts: (Int) -> Unit, - navigateToAddCharge: (Int) -> Unit, ) {
🧹 Nitpick comments (8)
feature/client/src/nativeMain/kotlin/com/mifos/feature/client/charges/ClientChargesScreen.native.kt (1)
27-34: LGTM: Appropriate stub implementation.The placeholder implementation clearly communicates that this feature is not yet available on the native platform. The unused parameters are expected in a stub.
If you plan to implement native platform support in the future, consider adding a TODO comment with a tracking issue reference for visibility.
feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientUpcomingCharges/ClientUpcomingChargesScreen.android.kt (1)
68-70: Consider conditional rendering to avoid composing both empty card and empty LazyColumn.The empty state logic correctly identifies when there are no items. However, both
MifosEmptyCardand theLazyColumn(lines 72-145) will be composed when the list is empty. While this works functionally, you might want to conditionally render theLazyColumnonly when there are items for cleaner composition.For example:
if (chargesPagingList.loadState.refresh is LoadState.NotLoading && chargesPagingList.itemCount == 0) { MifosEmptyCard() -} - -LazyColumn { +} else { + LazyColumn { + // ... existing content + } +}Alternatively, if you prefer to keep the current structure, that's also acceptable since the empty
LazyColumndoesn't consume visual space.core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosProgressIndicator.kt (1)
156-162: Rename preview function for clarity.The preview function has the same name as the composable (
MifosProgressIndicatorMini), which can cause confusion. Consider using a more descriptive name following the established pattern in this file.Apply this diff to align with the naming pattern:
@Preview @Composable -private fun MifosProgressIndicatorMini() { +private fun MifosProgressIndicatorMini_Preview() { MifosTheme { MifosProgressIndicatorMini() } }feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountScreen.kt (1)
453-453: Use non-nullableInttype instead ofInt?.The
expandedIndexis initialized to-1and never assignednull, so the nullable typeInt?is unnecessary. Using-1as a sentinel for "no item expanded" is appropriate, but the type should reflect that it's always non-null.Apply this diff:
- var expandedIndex: Int? by rememberSaveable { mutableStateOf(-1) } + var expandedIndex: Int by rememberSaveable { mutableStateOf(-1) }feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDetailsProfile/ClientProfileDetailsScreen.kt (1)
101-103: Consider validating client ID before navigation.The code falls back to
-1whenstate.client?.idis null. If the client ID is unavailable, navigating to add a charge with an invalid ID could cause downstream errors. Consider either:
- Disabling the "Add Charge" action when client data is unavailable, or
- Showing an error message instead of navigating with
-1.Example validation approach:
ClientProfileDetailsActionItem.AddCharge -> { - navigateToAddCharge(state.client?.id ?: -1) + state.client?.id?.let { clientId -> + navigateToAddCharge(clientId) + } ?: run { + // Show error or log warning + } }core/data/src/commonMain/kotlin/com/mifos/core/data/repository/ChargeRepository.kt (1)
39-50: Standardize parameter ordering for consistency.There's an inconsistency in parameter ordering across the interface:
- Read operations (
getListOfPagingCharges,getChargeTemplate,createCharges,getListOfClientCharges,getListOfOtherAccountCharge,getCharge) useresourceTypefirst, thenresourceId- Mutation operations (
deleteCharge,updateCharge) useresourceIdfirst, thenresourceTypeThis inconsistency can lead to confusion and parameter-swapping errors.
Consider standardizing all methods to use
resourceTypefirst, thenresourceId:suspend fun deleteCharge( - resourceId: Int, resourceType: String, + resourceId: Int, chargeId: Int, ): Flow<DataState<Unit>> suspend fun updateCharge( - resourceId: Int, resourceType: String, + resourceId: Int, chargeId: Int, payload: ChargesPayload, ): Flow<DataState<Unit>>core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerCharge.kt (2)
86-101: Preserve exception context in error handling.The error handling throws a new
IllegalStateExceptionwithout preserving the original exception context. This can make debugging more difficult in production.Apply this diff to preserve the exception chain:
suspend fun createCharges( resourceId: Int, resourceType: String, payload: ChargesPayload, ): ChargeCreationResponse { val response = mBaseApiManager.chargeService.createCharges(resourceType, resourceId, payload) if (!response.status.isSuccess()) { val errorMsg = extractErrorMessage(response) - throw IllegalStateException(errorMsg) + throw IllegalStateException(errorMsg, response.status.description?.let { Exception(it) }) } return Json.decodeFromString<ChargeCreationResponse>(response.bodyAsText()) }Alternatively, consider defining a custom exception type that better represents API errors rather than using
IllegalStateException.
103-127: Note: Parameter ordering inconsistency.Similar to the comment on
ChargeRepository.kt, these methods haveresourceIdbeforeresourceType, while other methods in this class useresourceTypefirst. Consider standardizing the parameter order across all methods for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (62)
cmp-navigation/build.gradle.kts(1 hunks)cmp-navigation/src/commonMain/kotlin/cmp/navigation/di/KoinModules.kt(2 hunks)core/data/src/commonMain/kotlin/com/mifos/core/data/di/RepositoryModule.kt(3 hunks)core/data/src/commonMain/kotlin/com/mifos/core/data/pagingSource/ClientChargesPagingSource.kt(3 hunks)core/data/src/commonMain/kotlin/com/mifos/core/data/repository/ChargeDialogRepository.kt(0 hunks)core/data/src/commonMain/kotlin/com/mifos/core/data/repository/ChargeRepository.kt(1 hunks)core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/ChargeDialogRepositoryImp.kt(0 hunks)core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/ChargeRepositoryImp.kt(1 hunks)core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/ClientChargeRepositoryImp.kt(0 hunks)core/database/src/commonMain/kotlin/com/mifos/room/helper/ChargeDaoHelper.kt(2 hunks)core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/DesignToken.kt(1 hunks)core/domain/src/commonMain/kotlin/com/mifos/core/domain/di/UseCaseModule.kt(2 hunks)core/domain/src/commonMain/kotlin/com/mifos/core/domain/useCases/CreateChargesUseCase.kt(1 hunks)core/domain/src/commonMain/kotlin/com/mifos/core/domain/useCases/GetChargesTemplateUseCase.kt(1 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/DataManager.kt(0 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerCharge.kt(2 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerRecurringAccount.kt(0 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/di/DataMangerModule.kt(2 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/services/ChargeService.kt(1 hunks)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosActionsListingCardComponent.kt(7 hunks)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosAllUiComponentsPreview.kt(2 hunks)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosProgressIndicator.kt(2 hunks)feature/client/src/androidMain/kotlin/com/mifos/feature/client/charges/ShowClientCharge.android.kt(1 hunks)feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesScreen.android.kt(0 hunks)feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientUpcomingCharges/ClientUpcomingChargesScreen.android.kt(3 hunks)feature/client/src/commonMain/composeResources/values/strings.xml(3 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ChargesRoute.kt(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ChargesScreen.kt(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ChargesViewModel.kt(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ShowClientCharge.kt(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientApplyNewApplications/ClientApplyNewApplicationRoute.kt(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientApplyNewApplications/ClientApplyNewApplicationsScreen.kt(2 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientChargeDialog/ChargeDialogScreen.kt(0 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientChargeDialog/ChargeDialogUiState.kt(0 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargeUiState.kt(0 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesScreen.kt(0 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesViewModel.kt(0 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDetailsProfile/ClientProfileDetailsNavigation.kt(2 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDetailsProfile/ClientProfileDetailsScreen.kt(2 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsViewModel.kt(4 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientProfile/ClientProfileNavigation.kt(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientUpcomingCharges/ClientUpcomingChargesRoute.kt(2 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientUpcomingCharges/ClientUpcomingChargesViewmodel.kt(3 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/di/ClientModule.kt(2 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/FixedDepositAccountScreen.kt(5 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/FixedDepositAccountViewModel.kt(3 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt(10 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/recurringDepositAccount/RecurringDepositAccountScreen.kt(5 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/recurringDepositAccount/RecurringDepositAccountViewModel.kt(3 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/savingsAccounts/SavingsAccounts.kt(8 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/savingsAccounts/SavingsAccountsViewModel.kt(7 hunks)feature/client/src/desktopMain/kotlin/com/mifos/feature/client/charges/ShowClientCharge.desktop.kt(2 hunks)feature/client/src/nativeMain/kotlin/com/mifos/feature/client/charges/ClientChargesScreen.native.kt(2 hunks)feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountScreen.kt(4 hunks)feature/recurringDeposit/src/commonMain/composeResources/values/string.xml(1 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountRoute.kt(1 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountScreen.kt(4 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt(3 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/di/RecurringDepositModule.kt(1 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/DetailsPage.kt(1 hunks)feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountScreen.kt(3 hunks)
💤 Files with no reviewable changes (11)
- core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/ChargeDialogRepositoryImp.kt
- feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientChargeDialog/ChargeDialogUiState.kt
- core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerRecurringAccount.kt
- core/network/src/commonMain/kotlin/com/mifos/core/network/DataManager.kt
- feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargeUiState.kt
- feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesViewModel.kt
- core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/ClientChargeRepositoryImp.kt
- core/data/src/commonMain/kotlin/com/mifos/core/data/repository/ChargeDialogRepository.kt
- feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientChargeDialog/ChargeDialogScreen.kt
- feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesScreen.android.kt
- feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesScreen.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- feature/client/src/commonMain/kotlin/com/mifos/feature/client/savingsAccounts/SavingsAccountsViewModel.kt
🧰 Additional context used
🧬 Code graph analysis (14)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/recurringDepositAccount/RecurringDepositAccountScreen.kt (1)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosActionsListingCardComponent.kt (1)
MifosActionsSavingsListingComponent(728-819)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt (4)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosProgressIndicator.kt (1)
MifosProgressIndicator(41-68)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosSearchBar.kt (1)
MifosSearchBar(40-104)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosEmptyCard.kt (1)
MifosEmptyCard(28-46)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosActionsListingCardComponent.kt (1)
MifosActionsLoanListingComponent(401-502)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountRoute.kt (1)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountScreen.kt (1)
RecurringAccountScreen(43-66)
feature/client/src/androidMain/kotlin/com/mifos/feature/client/charges/ShowClientCharge.android.kt (5)
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosSweetError.kt (1)
MifosSweetError(40-83)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosProgressIndicator.kt (1)
MifosProgressIndicator(41-68)core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosBottomSheet.kt (1)
MifosBottomSheet(47-89)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosActionsListingCardComponent.kt (1)
MifosActionsChargeListingComponent(598-726)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosTwoButtonRow.kt (1)
MifosTwoButtonRow(31-91)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/DetailsPage.kt (4)
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosTextFieldDropdown.kt (1)
MifosTextFieldDropdown(39-112)core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosOutlinedTextField.kt (1)
MifosDatePickerTextField(363-402)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosProgressIndicator.kt (1)
MifosProgressIndicatorMini(122-146)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosTwoButtonRow.kt (1)
MifosTwoButtonRow(31-91)
core/data/src/commonMain/kotlin/com/mifos/core/data/pagingSource/ClientChargesPagingSource.kt (4)
core/data/src/commonMain/kotlin/com/mifos/core/data/repository/ChargeRepository.kt (2)
getListOfPagingCharges(24-67)getListOfPagingCharges(26-29)core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/ChargeRepositoryImp.kt (1)
getListOfPagingCharges(32-44)core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerCharge.kt (1)
getListOfPagingCharges(54-80)core/network/src/commonMain/kotlin/com/mifos/core/network/services/ChargeService.kt (1)
getListOfPagingCharges(77-83)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ChargesRoute.kt (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ChargesScreen.kt (1)
ChargesScreen(96-121)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosProgressIndicator.kt (1)
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/Theme.kt (1)
MifosTheme(250-275)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ChargesScreen.kt (11)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/util/EventsEffect.kt (1)
EventsEffect(28-43)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosBreadCrumb.kt (1)
MifosBreadcrumbNavBar(36-107)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosRowWithTextAndButton.kt (1)
MifosRowWithTextAndButton(35-95)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosProgressIndicator.kt (2)
MifosProgressIndicatorOverlay(70-102)MifosProgressIndicator(41-68)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosErrorComponent.kt (1)
MifosErrorComponent(41-60)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosStatusDialog.kt (1)
MifosStatusDialog(36-103)core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosBottomSheet.kt (1)
MifosBottomSheet(47-89)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosActionsListingCardComponent.kt (1)
MifosActionsChargeListingComponent(598-726)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosTwoButtonRow.kt (1)
MifosTwoButtonRow(31-91)core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosTextFieldDropdown.kt (1)
MifosTextFieldDropdown(39-112)core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosOutlinedTextField.kt (1)
MifosDatePickerTextField(363-402)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt (2)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ChargesRoute.kt (1)
chargesDestination(23-33)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountRoute.kt (1)
recurringAccountDestination(22-32)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/savingsAccounts/SavingsAccounts.kt (1)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/util/EventsEffect.kt (1)
EventsEffect(28-43)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ChargesViewModel.kt (4)
core/data/src/commonMain/kotlin/com/mifos/core/data/repository/ChargeRepository.kt (3)
updateCharge(45-50)deleteCharge(39-43)getCharge(62-66)core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/ChargeRepositoryImp.kt (3)
updateCharge(78-94)deleteCharge(62-76)getCharge(111-121)core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerCharge.kt (3)
updateCharge(115-127)deleteCharge(103-113)getCharge(149-159)core/network/src/commonMain/kotlin/com/mifos/core/network/services/ChargeService.kt (3)
updateCharge(174-180)deleteCharge(158-163)getCharge(125-130)
feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientUpcomingCharges/ClientUpcomingChargesScreen.android.kt (1)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosEmptyCard.kt (1)
MifosEmptyCard(28-46)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountScreen.kt (5)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/DetailsPage.kt (1)
DetailsPage(51-168)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosBreadCrumb.kt (1)
MifosBreadcrumbNavBar(36-107)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosErrorComponent.kt (1)
MifosErrorComponent(41-60)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosProgressIndicator.kt (1)
MifosProgressIndicator(41-68)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosStepper.kt (1)
MifosStepper(47-139)
🔇 Additional comments (34)
feature/client/src/nativeMain/kotlin/com/mifos/feature/client/charges/ClientChargesScreen.native.kt (2)
10-10: LGTM: Package reorganization.The package rename from
clientChargestochargesremoves redundancy and aligns with the feature module structure.
23-25: LGTM: API alignment with multiplatform architecture.The function rename and signature change align with the broader refactor to use action-based callbacks. The
onAction: (ChargesAction) -> Unitparameter provides more flexibility than the previousonRetrycallback.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsViewModel.kt (2)
100-131: LGTM: Loading state management is correct.The loading state is properly managed—set to
trueat the start of the fetch operation and explicitly cleared tofalsein both success and error paths. This ensures the UI will always exit the loading state regardless of outcome.
134-146: LGTM: State definition with isLoading is well-structured.The addition of the
isLoadingflag with a sensible default and the simplifiedDialogState(containing onlyError) properly separates loading state from error state. This refactor improves clarity and aligns with the broader pattern applied across the PR.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientApplyNewApplications/ClientApplyNewApplicationsScreen.kt (2)
82-84: LGTM!The call site correctly passes
state.clientIdto the recurring account navigation callback, matching the pattern used for loan, savings, and share accounts.
65-65: All upstream callers are correctly updated.Verification confirms the signature change is properly implemented across the codebase:
- Route definition expects
onNavigateApplyRecurringAccount: (Int) -> Unit- Navigation function
navigateToRecurringAccountRoute(clientId: Int)matches this signature- Call site passes the correct method reference with matching signature
No further changes needed.
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientApplyNewApplications/ClientApplyNewApplicationRoute.kt (1)
28-28: LGTM!The signature change is consistent with the screen implementation and follows the established navigation pattern. The callback is correctly propagated to
ClientApplyNewApplicationsScreenat line 38.core/network/src/commonMain/kotlin/com/mifos/core/network/di/DataMangerModule.kt (1)
27-27: LGTM!The import is correctly placed in alphabetical order.
feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientUpcomingCharges/ClientUpcomingChargesScreen.android.kt (1)
36-36: LGTM!The import is properly added to support the empty state rendering below.
feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountScreen.kt (2)
37-40: LGTM! Necessary imports for state management.The added imports support the new per-item expansion state introduced in
ShowChargesDialog.
352-356: Verification confirms ID assumptions are valid.The expansion toggle implementation is safe:
- Charge IDs are sourced from API data (selectedCharge.id), not user-assigned
- The
-1sentinel is never assigned as a valid charge ID- No collision risk exists
The code can be approved as-is. Note: The type design using
Int?with-1default is semantically inconsistent but functionally correct and matches the existing codebase pattern.core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosProgressIndicator.kt (1)
122-146: Verify animation iterations behavior.The
animateLottieCompositionAsStatecall at Line 131 doesn't specify aniterationsparameter, so the animation will play once by default. Other progress indicators useiterations = Int.MAX_VALUEfor continuous animation. Confirm whether the single-play behavior is intentional for this mini indicator or if continuous animation is needed.If continuous animation is desired, apply this diff:
- val progress by animateLottieCompositionAsState(composition) + val progress by animateLottieCompositionAsState( + composition, + iterations = Int.MAX_VALUE, + )feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountScreen.kt (2)
44-47: LGTM!The new imports support the externalized expansion state management pattern introduced in this file.
477-480: LGTM!The expansion toggle logic correctly implements single-item expansion using charge IDs as keys. The pattern is consistent across both overdue and regular charges.
Note: The implementation assumes that
it.idvalues are unique across all charges. If duplicate IDs exist, multiple items may appear expanded/collapsed together.Also applies to: 508-511
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosAllUiComponentsPreview.kt (2)
170-171: LGTM!The preview correctly supplies the new
isExpandedandonExpandToggleparameters, showing the component in its expanded state.
938-939: LGTM!The preview parameters are updated to match the new component API, replacing
isExpandablewith externally controlledisExpandedandonExpandToggle.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDetailsProfile/ClientProfileDetailsScreen.kt (1)
89-89: LGTM!The new
navigateToAddChargecallback follows the established pattern for navigation functions in this screen.feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/di/RecurringDepositModule.kt (1)
1-18: LGTM!The new DI module follows the established pattern for feature modules and correctly exposes the
RecurringAccountViewModel. The module is properly integrated into the DI graph viaKoinModules.kt.cmp-navigation/src/commonMain/kotlin/cmp/navigation/di/KoinModules.kt (1)
34-34: LGTM!The
RecurringDepositModuleis properly imported and registered in the DI graph, following the established pattern for feature modules.Also applies to: 87-87
feature/client/src/desktopMain/kotlin/com/mifos/feature/client/charges/ShowClientCharge.desktop.kt (1)
10-10: LGTM!The API surface updates align with the broader refactoring to use action-based callbacks. Since this is a desktop stub showing "Not available in desktop yet", the
onActionparameter is appropriately unused.Also applies to: 23-25
core/domain/src/commonMain/kotlin/com/mifos/core/domain/di/UseCaseModule.kt (1)
42-42: All references toGetAllChargesV2UseCasehave been successfully updated.Verification confirms:
- The old use case file no longer exists
- No remaining references to
GetAllChargesV2UseCasein the codebaseGetChargesTemplateUseCaseis properly imported at line 42- The new use case is correctly registered in the DI module at line 120
ChargesViewModelis actively using the new use casefeature/client/src/commonMain/kotlin/com/mifos/feature/client/di/ClientModule.kt (1)
14-14: All references toClientChargesViewModelhave been successfully updated.The verification confirms that the migration from
ClientChargesViewModeltoChargesViewModelis complete. The DI module correctly imports and bindsChargesViewModelat lines 14 and 53, with no remaining references to the old ViewModel name found anywhere in the codebase.core/domain/src/commonMain/kotlin/com/mifos/core/domain/useCases/GetChargesTemplateUseCase.kt (1)
23-25: Flow wrapping aligns with repository contract.Resource-scoped fetch with
asDataStateFlow()keeps the API neat; no issues spotted.feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/FixedDepositAccountScreen.kt (1)
190-238: Expansion wiring and action dispatch look good.Indexed expansion plus the routed approve callback keep behavior intact.
core/database/src/commonMain/kotlin/com/mifos/room/helper/ChargeDaoHelper.kt (1)
32-61: Rename stays consistent.Resource-scoped save mirrors prior flow; no issues spotted.
core/domain/src/commonMain/kotlin/com/mifos/core/domain/useCases/CreateChargesUseCase.kt (1)
24-29: Resource-based call update looks correct.Constructor wiring and the invoke delegation now match the new
ChargeRepository.createCharges(resourceType, resourceId, payload)contract—no issues spotted here.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDetailsProfile/ClientProfileDetailsNavigation.kt (1)
34-50: New add-charge callback is wired through.The extra lambda is threaded cleanly from the destination builder into
ClientProfileDetailsScreen, keeping the nav surface consistent.feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ShowClientCharge.kt (1)
18-22: Shared ShowClientCharge entry point looks solid.The expect composable takes the paging stream and action callback we need for platform actuals; no concerns.
feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/FixedDepositAccountViewModel.kt (1)
133-158: Loading flag handling reads well.The DataState branches now toggle
isLoadingwhile clearing dialog state, which keeps UI feedback straightforward.feature/client/src/commonMain/kotlin/com/mifos/feature/client/savingsAccounts/SavingsAccounts.kt (1)
64-189: Navigation handling now carries account context.Forwarding accountId + depositType for view and exposing the approve route keeps the screen aligned with the updated event model—looks good.
core/data/src/commonMain/kotlin/com/mifos/core/data/pagingSource/ClientChargesPagingSource.kt (1)
18-22: LGTM: Clean refactor to resource-based pattern.The generalization from
clientIdtoresourceType/resourceIdmakes this paging source reusable across different resource types (clients, savings accounts, loans, etc.), aligning well with the PR's objective to connect multiple account types.feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountScreen.kt (2)
45-49: LGTM: Proper DI integration.The migration from
viewModel()tokoinViewModel()aligns with the Koin dependency injection pattern used across the codebase, and the addition ofnavControllerenables breadcrumb navigation.
109-141: LGTM: Well-structured state-driven UI.The state-based rendering pattern (Loading → Error → Success) with breadcrumb navigation provides proper user feedback and improves navigation UX. The state handling is clean and follows best practices.
core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/ChargeRepositoryImp.kt (1)
28-122: LGTM: Clean repository implementation.The implementation correctly delegates to
DataManagerChargeand properly wraps results inFlow<DataState<...>>for consistent error handling across the data layer. The paging integration is well-structured.
| val extraLarge: Dp = 28.dp, | ||
| val extraLargeIncreased: Dp = 32.dp, | ||
| val extraExtraLarge: Dp = 48.dp, | ||
| val half: Dp = 500.dp, |
There was a problem hiding this comment.
🧩 Analysis chain
Update documentation and verify usage of the new spacing value.
The new half spacing field is missing from the KDoc comment above (lines 72-82), which documents all other spacing values. Additionally, the name "half" breaks the established semantic naming pattern (extraSmall, small, medium, large, etc.) and may cause confusion.
Please add documentation for this field. Apply this diff:
* @param extraExtraLarge Significant spacing for major layout divisions or empty states (48.dp).
+ * @param half Half of maximum spacing, used for substantial offsets or specific layout needs (500.dp).
* @param full Maximum spacing, typically used for centering or deep offset layouts (1000.dp).Consider renaming to better match the semantic pattern, such as halfFull, substantial, or extraExtraExtraLarge.
Verify that this spacing value is used correctly across the codebase:
🏁 Script executed:
#!/bin/bash
# Description: Find usages of the new 'half' spacing value to confirm it's needed and applied correctly
# Search for references to DesignToken.spacing.half or spacing.half
rg -n --type=kotlin 'spacing\.half' -C 3
# Also check for any hardcoded 500.dp values that might be replaced by this token
rg -n --type=kotlin '\b500\.dp\b' -C 2Length of output: 3973
Add documentation and replace hardcoded 500.dp values with the new token.
The new half spacing field is missing from the KDoc comment (lines 72-82). Additionally, two hardcoded 500.dp values in the codebase should be replaced with DesignToken.spacing.half for consistency and centralized design management.
Add documentation to DesignToken.kt with this diff:
* @param extraExtraLarge Significant spacing for major layout divisions or empty states (48.dp).
+ * @param half Half of maximum spacing, used for constraining scrollable content heights (500.dp).
* @param full Maximum spacing, typically used for centering or deep offset layouts (1000.dp).Replace hardcoded values:
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosRadioButtonAlertDialog.ktline 56:.heightIn(max = 500.dp)→.heightIn(max = DesignToken.spacing.half)core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosAlertDialog.ktline 130:.heightIn(max = 500.dp)→.heightIn(max = DesignToken.spacing.half)
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/DesignToken.kt
around line 96, add the missing KDoc entry for the new spacing field `half`
within the existing KDoc block (lines ~72-82) describing its purpose (represents
half spacing, maps to 500.dp) and ensure the property declaration remains `val
half: Dp = 500.dp`; then replace hardcoded 500.dp usages with the new token: in
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosRadioButtonAlertDialog.kt
at line ~56 change `.heightIn(max = 500.dp)` to `.heightIn(max =
DesignToken.spacing.half)` and in
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosAlertDialog.kt
at line ~130 change `.heightIn(max = 500.dp)` to `.heightIn(max =
DesignToken.spacing.half)`.
core/network/src/commonMain/kotlin/com/mifos/core/network/di/DataMangerModule.kt
Show resolved
Hide resolved
| val chargesList = chargesPagingList[0]?.pageItems ?: emptyList() | ||
|
|
||
| MifosBottomSheet( | ||
| onDismiss = { | ||
| onAction(ChargesAction.DismissDialog) | ||
| }, | ||
| content = { | ||
| Column( | ||
| modifier = Modifier.padding(DesignToken.padding.large) | ||
| .heightIn(max = DesignToken.spacing.half), | ||
| verticalArrangement = Arrangement.spacedBy(DesignToken.padding.largeIncreased), | ||
| ) { | ||
| Text( | ||
| text = stringResource(Res.string.charges_view_charges), | ||
| style = MifosTypography.titleMediumEmphasized, | ||
| ) | ||
|
|
||
| LazyColumn( | ||
| verticalArrangement = Arrangement.spacedBy(DesignToken.padding.medium), | ||
| ) { | ||
| items(chargesList) { | ||
| MifosActionsChargeListingComponent( | ||
| chargeTitle = it.name.toString(), | ||
| type = it.chargeCalculationType?.value.toString(), | ||
| date = it.formattedDueDate, | ||
| collectedOn = it.formattedDueDate, | ||
| amount = it.amount.toString(), | ||
| onActionClicked = { action -> | ||
| when (action) { | ||
| is Actions.Delete -> { | ||
| onAction(ChargesAction.DeleteCharge(it.id)) | ||
| } | ||
|
|
||
| is Actions.Edit -> { | ||
| onAction(ChargesAction.FetchEditChargeData(it.id)) | ||
| } | ||
|
|
||
| else -> {} | ||
| } | ||
| }, | ||
| isExpanded = expandedIndex == it.id, | ||
| onExpandToggle = { | ||
| expandedIndex = if (expandedIndex == it.id) -1 else it.id | ||
| }, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Paging pipeline only renders the first page.
chargesPagingList[0]?.pageItems drops every subsequent page, so accounts with >1 page of charges will never see later entries. Please iterate over all loaded pages instead of hard-coding index 0.
Apply this diff to materialize every page item:
- val chargesList = chargesPagingList[0]?.pageItems ?: emptyList()
-
MifosBottomSheet(
onDismiss = {
onAction(ChargesAction.DismissDialog)
},
content = {
Column(
modifier = Modifier.padding(DesignToken.padding.large)
.heightIn(max = DesignToken.spacing.half),
verticalArrangement = Arrangement.spacedBy(DesignToken.padding.largeIncreased),
) {
Text(
text = stringResource(Res.string.charges_view_charges),
style = MifosTypography.titleMediumEmphasized,
)
LazyColumn(
verticalArrangement = Arrangement.spacedBy(DesignToken.padding.medium),
) {
- items(chargesList) {
- MifosActionsChargeListingComponent(
- chargeTitle = it.name.toString(),
- type = it.chargeCalculationType?.value.toString(),
- date = it.formattedDueDate,
- collectedOn = it.formattedDueDate,
- amount = it.amount.toString(),
- onActionClicked = { action ->
- when (action) {
- is Actions.Delete -> {
- onAction(ChargesAction.DeleteCharge(it.id))
- }
-
- is Actions.Edit -> {
- onAction(ChargesAction.FetchEditChargeData(it.id))
- }
-
- else -> {}
- }
- },
- isExpanded = expandedIndex == it.id,
- onExpandToggle = {
- expandedIndex = if (expandedIndex == it.id) -1 else it.id
- },
- )
- }
+ items(count = chargesPagingList.itemCount) { pageIndex ->
+ chargesPagingList[pageIndex]?.pageItems.orEmpty().forEach { charge ->
+ MifosActionsChargeListingComponent(
+ chargeTitle = charge.name.toString(),
+ type = charge.chargeCalculationType?.value.toString(),
+ date = charge.formattedDueDate,
+ collectedOn = charge.formattedDueDate,
+ amount = charge.amount.toString(),
+ onActionClicked = { action ->
+ when (action) {
+ is Actions.Delete -> {
+ onAction(ChargesAction.DeleteCharge(charge.id))
+ }
+
+ is Actions.Edit -> {
+ onAction(ChargesAction.FetchEditChargeData(charge.id))
+ }
+
+ else -> {}
+ }
+ },
+ isExpanded = expandedIndex == charge.id,
+ onExpandToggle = {
+ expandedIndex = if (expandedIndex == charge.id) -1 else charge.id
+ },
+ )
+ }
+ }
}
MifosTwoButtonRow(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val chargesList = chargesPagingList[0]?.pageItems ?: emptyList() | |
| MifosBottomSheet( | |
| onDismiss = { | |
| onAction(ChargesAction.DismissDialog) | |
| }, | |
| content = { | |
| Column( | |
| modifier = Modifier.padding(DesignToken.padding.large) | |
| .heightIn(max = DesignToken.spacing.half), | |
| verticalArrangement = Arrangement.spacedBy(DesignToken.padding.largeIncreased), | |
| ) { | |
| Text( | |
| text = stringResource(Res.string.charges_view_charges), | |
| style = MifosTypography.titleMediumEmphasized, | |
| ) | |
| LazyColumn( | |
| verticalArrangement = Arrangement.spacedBy(DesignToken.padding.medium), | |
| ) { | |
| items(chargesList) { | |
| MifosActionsChargeListingComponent( | |
| chargeTitle = it.name.toString(), | |
| type = it.chargeCalculationType?.value.toString(), | |
| date = it.formattedDueDate, | |
| collectedOn = it.formattedDueDate, | |
| amount = it.amount.toString(), | |
| onActionClicked = { action -> | |
| when (action) { | |
| is Actions.Delete -> { | |
| onAction(ChargesAction.DeleteCharge(it.id)) | |
| } | |
| is Actions.Edit -> { | |
| onAction(ChargesAction.FetchEditChargeData(it.id)) | |
| } | |
| else -> {} | |
| } | |
| }, | |
| isExpanded = expandedIndex == it.id, | |
| onExpandToggle = { | |
| expandedIndex = if (expandedIndex == it.id) -1 else it.id | |
| }, | |
| ) | |
| } | |
| } | |
| MifosBottomSheet( | |
| onDismiss = { | |
| onAction(ChargesAction.DismissDialog) | |
| }, | |
| content = { | |
| Column( | |
| modifier = Modifier.padding(DesignToken.padding.large) | |
| .heightIn(max = DesignToken.spacing.half), | |
| verticalArrangement = Arrangement.spacedBy(DesignToken.padding.largeIncreased), | |
| ) { | |
| Text( | |
| text = stringResource(Res.string.charges_view_charges), | |
| style = MifosTypography.titleMediumEmphasized, | |
| ) | |
| LazyColumn( | |
| verticalArrangement = Arrangement.spacedBy(DesignToken.padding.medium), | |
| ) { | |
| items(count = chargesPagingList.itemCount) { pageIndex -> | |
| chargesPagingList[pageIndex]?.pageItems.orEmpty().forEach { charge -> | |
| MifosActionsChargeListingComponent( | |
| chargeTitle = charge.name.toString(), | |
| type = charge.chargeCalculationType?.value.toString(), | |
| date = charge.formattedDueDate, | |
| collectedOn = charge.formattedDueDate, | |
| amount = charge.amount.toString(), | |
| onActionClicked = { action -> | |
| when (action) { | |
| is Actions.Delete -> { | |
| onAction(ChargesAction.DeleteCharge(charge.id)) | |
| } | |
| is Actions.Edit -> { | |
| onAction(ChargesAction.FetchEditChargeData(charge.id)) | |
| } | |
| else -> {} | |
| } | |
| }, | |
| isExpanded = expandedIndex == charge.id, | |
| onExpandToggle = { | |
| expandedIndex = if (expandedIndex == charge.id) -1 else charge.id | |
| }, | |
| ) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In
feature/client/src/androidMain/kotlin/com/mifos/feature/client/charges/ShowClientCharge.android.kt
around lines 65 to 111, the code currently uses chargesPagingList[0]?.pageItems
which only takes the first page and drops subsequent pages; replace that with a
safe aggregation across all loaded pages (e.g., iterate over chargesPagingList
and collect/flatten each pageItems into a single list, handling null pages and
null pageItems by using empty lists) and then use that flattened list for the
LazyColumn items so every loaded page item is rendered.
| import androidx.compose.foundation.layout.fillMaxWidth | ||
| import androidx.compose.foundation.layout.padding | ||
| import androidx.compose.foundation.lazy.LazyColumn | ||
| import androidx.compose.foundation.lazy.items |
There was a problem hiding this comment.
🧩 Analysis chain
Verify if this import is necessary.
The items import appears to be for the List/Array-based extension function, but the code uses items(count = ...) at line 73, which is a different overload from LazyListScope. Please confirm whether this import is actually needed.
Run the following script to check if the List-based items function is used elsewhere in this file:
🏁 Script executed:
#!/bin/bash
# Check if the items function is used with a List or collection in this file
rg -n 'items\s*\(' feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientUpcomingCharges/ClientUpcomingChargesScreen.android.kt -A 2Length of output: 142
---
🏁 Script executed:
```shell
#!/bin/bash
# View the import section and LazyColumn context to confirm
sed -n '1,30p' feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientUpcomingCharges/ClientUpcomingChargesScreen.android.kt && echo "---" && sed -n '70,80p' feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientUpcomingCharges/ClientUpcomingChargesScreen.android.kt
Length of output: 166
---
🏁 Script executed:
```shell
#!/bin/bash
# View the import section and LazyColumn context to confirm
sed -n '1,30p' feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientUpcomingCharges/ClientUpcomingChargesScreen.android.kt && echo "---" && sed -n '70,80p' feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientUpcomingCharges/ClientUpcomingChargesScreen.android.kt
Length of output: 1828
Remove the unused items import at line 20.
The import androidx.compose.foundation.lazy.items is for List/Array-based extension functions, but the code uses items(count = ...) at line 73, which is the count-based overload built into LazyListScope. This import is not needed.
🤖 Prompt for AI Agents
In
feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientUpcomingCharges/ClientUpcomingChargesScreen.android.kt
around line 20, the import androidx.compose.foundation.lazy.items is unused
because the file uses the count-based items(count = ...) overload; remove that
import line to eliminate the unused import.
| @Serializable | ||
| data class ChargesRoute( | ||
| val resourceId: Int = -1, | ||
| val resourceType: String = "", | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify sentinel default values are validated before use.
The default values resourceId = -1 and resourceType = "" serve as sentinel values. Ensure that ChargesScreen or its ViewModel validates these before making API calls, as invalid defaults could lead to runtime errors or unexpected API behavior.
Run this script to verify validation logic in ChargesScreen and ChargesViewModel:
🏁 Script executed:
#!/bin/bash
# Description: Check if ChargesScreen/ChargesViewModel validates resourceId and resourceType
# Search for validation of resourceId and resourceType in ChargesScreen and ChargesViewModel
rg -nP -A5 -B5 'resourceId.*(-1|validation|check|require)' --type=kt -g '**/charges/**'
rg -nP -A5 -B5 'resourceType.*(isEmpty|isBlank|validation|check|require)' --type=kt -g '**/charges/**'Length of output: 53
🏁 Script executed:
#!/bin/bash
# Find relevant Kotlin files in charges directory
echo "=== Finding Kotlin files in charges directory ==="
fd -e kt -p '.*charges.*' | head -20
echo -e "\n=== Searching for ChargesScreen ==="
fd -e kt 'ChargesScreen'
echo -e "\n=== Searching for ChargesViewModel ==="
fd -e kt 'ChargesViewModel'Length of output: 2624
🏁 Script executed:
#!/bin/bash
# Search for validation patterns around resourceId and resourceType
echo "=== Checking for resourceId validation ==="
rg -n 'resourceId' -g '**charges**' -A3 -B3 | head -50
echo -e "\n=== Checking for resourceType validation ==="
rg -n 'resourceType' -g '**charges**' -A3 -B3 | head -50Length of output: 88
🏁 Script executed:
#!/bin/bash
# Search for patterns indicating validation: require, check, isEmpty, isBlank, etc
echo "=== Checking for defensive checks before API calls ==="
rg -n '(require|check|isEmpty|isBlank|validates|guard)' -g '**charges**' | head -30Length of output: 54
🏁 Script executed:
#!/bin/bash
# Read the ChargesScreen and ChargesViewModel files directly
echo "=== ChargesScreen.kt ==="
cat feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ChargesScreen.kt
echo -e "\n\n=== ChargesViewModel.kt ==="
cat feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ChargesViewModel.ktLength of output: 39987
Validate sentinel route parameters before making API calls.
The default values resourceId = -1 and resourceType = "" are sentinel values that lack validation before use. In ChargesViewModel.observeNetwork(), these values are passed directly to repository.getListOfClientCharges(), repository.getListOfOtherAccountCharge(), and getChargeTemplateUseCase() without guard clauses. Add validation (e.g., require(route.resourceId > 0) and require(route.resourceType.isNotEmpty())) in observeNetwork() or in the init block to prevent invalid API calls.
🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ChargesRoute.kt
around lines 17 to 21, the data class uses sentinel defaults (resourceId = -1,
resourceType = ""), so update the consumer (ChargesViewModel.observeNetwork() or
the ChargesRoute init) to validate these before making API calls: add
require(route.resourceId > 0) and require(route.resourceType.isNotEmpty()) (or
equivalent guard clauses) and bail out or return early if validation fails so
repository.getListOfClientCharges(), repository.getListOfOtherAccountCharge(),
and getChargeTemplateUseCase() are never called with invalid parameters.
| is Actions.ApproveAccount -> { | ||
| RecurringDepositAccountAction.ApproveAccount( | ||
| recurringDeposit.accountNo ?: "", | ||
| ) | ||
| } |
There was a problem hiding this comment.
Approve action never dispatches.
Line 234 instantiates RecurringDepositAccountAction.ApproveAccount but fails to call onAction, so tapping “Approve” now does nothing. Please forward the action just like the view branch.
Apply this diff:
- is Actions.ApproveAccount -> {
- RecurringDepositAccountAction.ApproveAccount(
- recurringDeposit.accountNo ?: "",
- )
- }
+ is Actions.ApproveAccount -> {
+ onAction(
+ RecurringDepositAccountAction.ApproveAccount(
+ recurringDeposit.accountNo ?: "",
+ ),
+ )
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| is Actions.ApproveAccount -> { | |
| RecurringDepositAccountAction.ApproveAccount( | |
| recurringDeposit.accountNo ?: "", | |
| ) | |
| } | |
| is Actions.ApproveAccount -> { | |
| onAction( | |
| RecurringDepositAccountAction.ApproveAccount( | |
| recurringDeposit.accountNo ?: "", | |
| ), | |
| ) | |
| } |
🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/recurringDepositAccount/RecurringDepositAccountScreen.kt
around lines 233 to 237, the Approve branch constructs
RecurringDepositAccountAction.ApproveAccount but never dispatches it; change the
code to call onAction with that action (i.e., replace the bare instantiation
with
onAction(RecurringDepositAccountAction.ApproveAccount(recurringDeposit.accountNo
?: ""))) so the tap triggers the action, mirroring the view branch behavior.
| import org.jetbrains.compose.resources.stringResource | ||
| import kotlin.time.Clock | ||
| import kotlin.time.ExperimentalTime | ||
|
|
||
| @OptIn(ExperimentalTime::class, ExperimentalMaterial3Api::class) | ||
| @Composable | ||
| fun DetailsPage(onNext: () -> Unit) { | ||
| fun DetailsPage( |
There was a problem hiding this comment.
Fix the Clock import to restore compilation.
Clock.System lives in kotlinx.datetime.Clock; importing kotlin.time.Clock leaves System unresolved and the file won’t compile. Drop the ExperimentalTime opt-in and use the datetime Clock instead.
-import kotlin.time.Clock
-import kotlin.time.ExperimentalTime
-
-@OptIn(ExperimentalTime::class, ExperimentalMaterial3Api::class)
+import kotlinx.datetime.Clock
+
+@OptIn(ExperimentalMaterial3Api::class)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import org.jetbrains.compose.resources.stringResource | |
| import kotlin.time.Clock | |
| import kotlin.time.ExperimentalTime | |
| @OptIn(ExperimentalTime::class, ExperimentalMaterial3Api::class) | |
| @Composable | |
| fun DetailsPage(onNext: () -> Unit) { | |
| fun DetailsPage( | |
| import org.jetbrains.compose.resources.stringResource | |
| import kotlinx.datetime.Clock | |
| @OptIn(ExperimentalMaterial3Api::class) | |
| @Composable | |
| fun DetailsPage( |
🤖 Prompt for AI Agents
In
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/DetailsPage.kt
around lines 47-53, the file incorrectly imports kotlin.time.Clock and opts into
ExperimentalTime which makes Clock.System unresolved; remove the import of
kotlin.time.Clock, remove the ExperimentalTime opt-in, add an import for
kotlinx.datetime.Clock (so you can use Clock.System), and keep the existing
ExperimentalMaterial3Api opt-in only.
| if (!state.template?.fieldOfficerOptions.isNullOrEmpty()) { | ||
| MifosDatePickerTextField( | ||
| value = state.submissionDate, | ||
| label = stringResource(Res.string.submitted_on), | ||
| openDatePicker = { | ||
| onAction(RecurringAccountAction.OnSubmissionDatePick(true)) | ||
| }, | ||
| ) | ||
|
|
||
| Spacer(Modifier.height(DesignToken.padding.large)) | ||
| MifosTextFieldDropdown( | ||
| value = if (state.fieldOfficerIndex == -1) { | ||
| "" | ||
| } else { | ||
| state.fieldOfficerOptions?.get(state.fieldOfficerIndex)?.displayName ?: "" | ||
| }, | ||
| onValueChanged = {}, | ||
| onOptionSelected = { index, value -> | ||
| onAction(RecurringAccountAction.OnFieldOfficerChange(index)) | ||
| }, | ||
| options = state.fieldOfficerOptions?.mapNotNull { | ||
| it.displayName | ||
| } ?: emptyList(), | ||
| label = stringResource(Res.string.field_officer), | ||
| ) | ||
|
|
||
| MifosOutlinedTextField( | ||
| value = state.externalId, | ||
| onValueChange = { | ||
| onAction(RecurringAccountAction.OnExternalIdChange(it)) | ||
| }, | ||
| label = stringResource(Res.string.external_id), | ||
| config = MifosTextFieldConfig( | ||
| keyboardOptions = KeyboardOptions( | ||
| keyboardType = KeyboardType.Text, | ||
| ), | ||
| ), | ||
| ) | ||
| Spacer(Modifier.height(DesignToken.padding.large)) |
There was a problem hiding this comment.
Do not hide submission date/external ID behind field-officer options.
By wrapping the submission date picker and external ID field inside if (!state.template?.fieldOfficerOptions.isNullOrEmpty()), those inputs disappear whenever the template has no field officer choices—users then lose mandatory form fields. Please move the conditional so only the field-officer dropdown is gated.
- if (!state.template?.fieldOfficerOptions.isNullOrEmpty()) {
- MifosDatePickerTextField(...)
- ...
- MifosTextFieldDropdown( // field officer
- ...
- )
-
- MifosOutlinedTextField(...)
- Spacer(...)
- }
+ MifosDatePickerTextField(...)
+
+ Spacer(Modifier.height(DesignToken.padding.large))
+
+ if (!state.template?.fieldOfficerOptions.isNullOrEmpty()) {
+ MifosTextFieldDropdown(
+ ...
+ )
+ Spacer(Modifier.height(DesignToken.padding.large))
+ }
+
+ MifosOutlinedTextField(...)
+ Spacer(Modifier.height(DesignToken.padding.large))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!state.template?.fieldOfficerOptions.isNullOrEmpty()) { | |
| MifosDatePickerTextField( | |
| value = state.submissionDate, | |
| label = stringResource(Res.string.submitted_on), | |
| openDatePicker = { | |
| onAction(RecurringAccountAction.OnSubmissionDatePick(true)) | |
| }, | |
| ) | |
| Spacer(Modifier.height(DesignToken.padding.large)) | |
| MifosTextFieldDropdown( | |
| value = if (state.fieldOfficerIndex == -1) { | |
| "" | |
| } else { | |
| state.fieldOfficerOptions?.get(state.fieldOfficerIndex)?.displayName ?: "" | |
| }, | |
| onValueChanged = {}, | |
| onOptionSelected = { index, value -> | |
| onAction(RecurringAccountAction.OnFieldOfficerChange(index)) | |
| }, | |
| options = state.fieldOfficerOptions?.mapNotNull { | |
| it.displayName | |
| } ?: emptyList(), | |
| label = stringResource(Res.string.field_officer), | |
| ) | |
| MifosOutlinedTextField( | |
| value = state.externalId, | |
| onValueChange = { | |
| onAction(RecurringAccountAction.OnExternalIdChange(it)) | |
| }, | |
| label = stringResource(Res.string.external_id), | |
| config = MifosTextFieldConfig( | |
| keyboardOptions = KeyboardOptions( | |
| keyboardType = KeyboardType.Text, | |
| ), | |
| ), | |
| ) | |
| Spacer(Modifier.height(DesignToken.padding.large)) | |
| MifosDatePickerTextField( | |
| value = state.submissionDate, | |
| label = stringResource(Res.string.submitted_on), | |
| openDatePicker = { | |
| onAction(RecurringAccountAction.OnSubmissionDatePick(true)) | |
| }, | |
| ) | |
| Spacer(Modifier.height(DesignToken.padding.large)) | |
| if (!state.template?.fieldOfficerOptions.isNullOrEmpty()) { | |
| MifosTextFieldDropdown( | |
| value = if (state.fieldOfficerIndex == -1) { | |
| "" | |
| } else { | |
| state.fieldOfficerOptions?.get(state.fieldOfficerIndex)?.displayName ?: "" | |
| }, | |
| onValueChanged = {}, | |
| onOptionSelected = { index, value -> | |
| onAction(RecurringAccountAction.OnFieldOfficerChange(index)) | |
| }, | |
| options = state.fieldOfficerOptions?.mapNotNull { | |
| it.displayName | |
| } ?: emptyList(), | |
| label = stringResource(Res.string.field_officer), | |
| ) | |
| Spacer(Modifier.height(DesignToken.padding.large)) | |
| } | |
| MifosOutlinedTextField( | |
| value = state.externalId, | |
| onValueChange = { | |
| onAction(RecurringAccountAction.OnExternalIdChange(it)) | |
| }, | |
| label = stringResource(Res.string.external_id), | |
| config = MifosTextFieldConfig( | |
| keyboardOptions = KeyboardOptions( | |
| keyboardType = KeyboardType.Text, | |
| ), | |
| ), | |
| ) | |
| Spacer(Modifier.height(DesignToken.padding.large)) |
🤖 Prompt for AI Agents
In
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/DetailsPage.kt
around lines 114 to 152, the submission date picker and external ID field are
incorrectly placed inside the conditional that checks fieldOfficerOptions,
causing them to be hidden when no field-officer options exist; refactor by
moving the if (!state.template?.fieldOfficerOptions.isNullOrEmpty()) so it wraps
only the MifosTextFieldDropdown block (leave MifosDatePickerTextField and
MifosOutlinedTextField outside and always rendered), and ensure the dropdown
still uses the options mapping and index handling as before.
| onNavigateBack = {}, | ||
| onFinish = {}, | ||
| navController = navController, | ||
| ) |
There was a problem hiding this comment.
Restore navigation callbacks in RecurringAccountScreen wiring
Line 27 currently passes onNavigateBack = {} and onFinish = {}. RecurringAccountScreen raises these callbacks when the user taps back or after completing account creation, so turning them into no-ops leaves the user stuck on this screen. Please hook them back to real navigation (e.g., by calling the appropriate navController operations) instead of empty lambdas.
RecurringAccountScreen(
- onNavigateBack = {},
- onFinish = {},
+ onNavigateBack = { navController.popBackStack() },
+ onFinish = { navController.popBackStack() },
navController = navController,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onNavigateBack = {}, | |
| onFinish = {}, | |
| navController = navController, | |
| ) | |
| RecurringAccountScreen( | |
| onNavigateBack = { navController.popBackStack() }, | |
| onFinish = { navController.popBackStack() }, | |
| navController = navController, | |
| ) |
🤖 Prompt for AI Agents
In
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountRoute.kt
around lines 27–30, the call sites currently pass onNavigateBack = {} and
onFinish = {}, which are no-ops and prevent navigation; replace those lambdas
with real navController operations: set onNavigateBack to call
navController.popBackStack() (or navController.navigateUp()) and set onFinish to
navigate to the post-creation destination (e.g.,
navController.navigate("accounts") or the specific account details route),
making sure to use appropriate nav options (popUpTo or clearBackStack) if needed
to prevent returning to the form.
| var expandedIndex: Int? by rememberSaveable { mutableStateOf(-1) } | ||
|
|
There was a problem hiding this comment.
Fix type inconsistency and clarify variable naming.
Two issues with the state declaration:
- Type mismatch: Declaring
Int?(nullable) but initializing with non-null-1is inconsistent. - Misleading name:
expandedIndexsuggests it stores an index, but Line 352 shows it actually stores anidvalue fromit.id.
Apply this diff to use idiomatic nullable handling and clearer naming:
- var expandedIndex: Int? by rememberSaveable { mutableStateOf(-1) }
+ var expandedItemId: Int? by rememberSaveable { mutableStateOf(null) }Then update the toggle logic accordingly:
- isExpanded = expandedIndex == it.id,
+ isExpanded = expandedItemId == it.id,
onExpandToggle = {
- expandedIndex = if (expandedIndex == it.id) -1 else it.id
+ expandedItemId = if (expandedItemId == it.id) null else it.id
},Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountScreen.kt
around lines 309–310, the state is declared as `var expandedIndex: Int? by
rememberSaveable { mutableStateOf(-1) }` which mixes a nullable type with a
non-null sentinel and the name implies an index while you actually store an id;
change the variable to a nullable id name (e.g., `expandedId: Int?`) and
initialize it with `null` via rememberSaveable { mutableStateOf<Int?>(null) };
then update the toggle logic to compare against `it.id` and set `expandedId = if
(expandedId == it.id) null else it.id`, and update any other usages/readers to
use the new name and nullable semantics.
# Conflicts: # feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/FixedDepositAccountScreen.kt # feature/client/src/commonMain/kotlin/com/mifos/feature/client/recurringDepositAccount/RecurringDepositAccountScreen.kt # feature/client/src/commonMain/kotlin/com/mifos/feature/client/savingsAccounts/SavingsAccounts.kt
Arinyadav1
force-pushed
the
connect-saving-item-to-saving-detail-page
branch
from
81567fb to
27811bc
Compare
|
@revanthkumarJ resolve merge conflicts |
3 participants
Fixes - Jira-#611
IMG_4970.1.MOV
Summary by CodeRabbit
New Features
Refactor