Conversation
WalkthroughThis pull request refactors the charges management system from client-specific to resource-generic architecture. Updates span multiple layers: repository interfaces consolidate into a unified Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Charges UI
participant VM as ChargesViewModel
participant Repo as ChargeRepository
participant DM as DataManagerCharge
participant API as ChargeService
UI->>VM: onAction: CreateCharge
VM->>VM: validate inputs
VM->>VM: show loading overlay
VM->>Repo: createCharges(resourceType, resourceId, payload)
Repo->>DM: createCharges(resourceId, resourceType, payload)
DM->>API: POST /{resourceType}/{resourceId}/charges
API-->>DM: HttpResponse
DM->>DM: validate status & parse response
DM-->>Repo: ChargeCreationResponse
Repo-->>VM: DataState.Success
VM->>VM: update state → ShowStatusDialog
UI->>UI: render success/failure dialog
UI->>VM: onAction: NavigateBack
VM-->>UI: emit NavigateBack event
sequenceDiagram
participant UI as Charges UI
participant VM as ChargesViewModel
participant Repo as ChargeRepository
participant Pager as Pager / PagingSource
participant DM as DataManagerCharge
participant API as ChargeService
UI->>VM: onAction: OnShowChargeBottomSheet
VM->>Repo: getListOfPagingCharges(resourceType, resourceId)
Repo->>Pager: create Pager(pageSize=10, pagingSourceFactory)
Pager->>Pager: load(LoadParams)
Pager->>DM: getListOfPagingCharges(resourceType, resourceId, offset, limit)
DM->>API: GET /{resourceType}/{resourceId}/charges?offset=X&limit=10
API-->>DM: Flow<Page<ChargesEntity>>
DM-->>Pager: Page
Pager-->>Repo: PagingData<ChargesEntity>
Repo-->>VM: Flow<PagingData<ChargesEntity>>
VM->>UI: update state with paging flow
UI->>UI: collectAsLazyPagingItems()
UI->>UI: render ShowChargeBottomSheet with items
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![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/androidMain/kotlin/com/mifos/feature/client/clientUpcomingCharges/ClientUpcomingChargesScreen.android.kt (1)
93-102: Fix action handlers to actually invoke the actions.The
onClickandonActionClickedlambdas create action objects but never invoke them via theonActioncallback. This means user clicks won't trigger any behavior.Apply this diff to fix both handlers:
- onClick = { ClientUpcomingChargesAction.CardClicked(index) }, + onClick = { onAction(ClientUpcomingChargesAction.CardClicked(index)) }, onActionClicked = { actions -> when (actions) { is Actions.PayOutstandingAmount -> { - ClientUpcomingChargesAction.PayOutstandingAmount + onAction(ClientUpcomingChargesAction.PayOutstandingAmount) } else -> {} } },core/database/src/commonMain/kotlin/com/mifos/room/helper/ChargeDaoHelper.kt (1)
32-63: Offline cache now conflates different resource types
saveClientChargesstores every payload underclientId = resourceId, butDataManagerCharge.getListOfPagingChargesnow calls this helper for any(resourceType, resourceId)pair. When offline,readClientCharges(resourceId)can return charges from another resource type that happens to share the same numeric id (e.g.,clients/42vssavingsaccounts/42). That breaks the very feature this PR is adding. Please persist the resource type alongside the id (either by adding a column toChargesEntityand filtering on it, or by keeping per-resource tables/helpers) so cached data stays isolated per resource.
🧹 Nitpick comments (5)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientProfile/ClientProfileNavigation.kt (1)
33-45: Pass the new callback through to the screen. The destination now requiresnavigateToAddCharge, but the lambda never reachesClientProfileScreen, leaving the new pathway inert while forcing callers to supply a value. Please either wire it through to the screen (and into whatever UI element should trigger it) or drop the parameter until it is needed.core/data/src/commonMain/kotlin/com/mifos/core/data/repository/ChargeRepository.kt (1)
39-50: Align parameter ordering across repository methods.Every other API here takes
(resourceType, resourceId)butdeleteCharge/updateChargeflip the order. That asymmetry is easy to misuse and will compile while calling the wrong backend path. Please reorder the parameters (and the implementation) to keep(resourceType, resourceId, …)everywhere.- suspend fun deleteCharge( - resourceId: Int, - resourceType: String, + suspend fun deleteCharge( + resourceType: String, + resourceId: Int, chargeId: Int, ): Flow<DataState<Unit>> - suspend fun updateCharge( - resourceId: Int, - resourceType: String, + suspend fun updateCharge( + resourceType: String, + resourceId: Int, chargeId: Int, payload: ChargesPayload, ): Flow<DataState<Unit>>core/domain/src/commonMain/kotlin/com/mifos/core/domain/useCases/GetChargesTemplateUseCase.kt (1)
23-24: Consider validating or constraining theresourceTypeparameter.The
resourceTypeparameter is aString, making it error-prone. Invalid values could lead to runtime errors or silent failures downstream. Consider using a sealed class, enum, or constant validation to ensure only valid resource types are accepted.Example approach using constants validation:
operator fun invoke(resourceType: String, resourceId: Int): Flow<DataState<ChargeTemplate>> = flow { require(resourceType in setOf(Constants.ENTITY_TYPE_CLIENTS, Constants.ENTITY_TYPE_SAVINGS, Constants.ENTITY_TYPE_LOANS)) { "Invalid resourceType: $resourceType" } emit(repository.getChargeTemplate(resourceType, resourceId)) }.asDataStateFlow()Or consider a sealed class approach in a shared location for stronger compile-time safety.
core/domain/src/commonMain/kotlin/com/mifos/core/domain/useCases/CreateChargesUseCase.kt (1)
23-29: Consider validating theresourceTypeparameter.Similar to
GetChargesTemplateUseCase, theresourceTypeparameter lacks validation. Invalid values could propagate through the system and cause issues at runtime.Consider adding validation or using a constrained type (enum or sealed class) to prevent invalid resource types from being passed.
feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ChargesViewModel.kt (1)
48-52: Validate route parameters in initialization.The
route.resourceTypeis extracted from navigation and used throughout the ViewModel without validation. Invalid resource types from navigation could cause runtime errors.Consider adding validation in the
initblock:val route = savedStateHandle.toRoute<ChargesRoute>() init { + require(route.resourceType in setOf( + Constants.ENTITY_TYPE_CLIENTS, + Constants.ENTITY_TYPE_SAVINGS, + Constants.ENTITY_TYPE_LOANS, + // Add other valid resource types + )) { + "Invalid resourceType: ${route.resourceType}" + } observeNetwork() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
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/services/ChargeService.kt(1 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(1 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/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/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/navigation/ClientNavigation.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)
💤 Files with no reviewable changes (10)
- feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesScreen.android.kt
- core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/ClientChargeRepositoryImp.kt
- feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesViewModel.kt
- feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargeUiState.kt
- feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientChargeDialog/ChargeDialogUiState.kt
- core/data/src/commonMain/kotlin/com/mifos/core/data/repository/ChargeDialogRepository.kt
- core/network/src/commonMain/kotlin/com/mifos/core/network/DataManager.kt
- feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientChargeDialog/ChargeDialogScreen.kt
- feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesScreen.kt
- core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/ChargeDialogRepositoryImp.kt
🧰 Additional context used
🧬 Code graph analysis (6)
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(94-119)
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-728)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)
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(50-76)core/network/src/commonMain/kotlin/com/mifos/core/network/services/ChargeService.kt (1)
getListOfPagingCharges(77-83)
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-728)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosTwoButtonRow.kt (1)
MifosTwoButtonRow(31-91)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ChargesViewModel.kt (2)
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)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ChargesRoute.kt (1)
chargesDestination(23-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR Checks / Static Analysis Check
🔇 Additional comments (2)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientUpcomingCharges/ClientUpcomingChargesRoute.kt (1)
35-40: All call sites and screen implementation have been correctly updated.Verification confirms the breaking change is properly implemented throughout the codebase:
- The route data class correctly defines
resourceIdandresourceTypeparameters- The navigation function properly accepts and passes both parameters
- The call site in ClientNavigation.kt line 238 correctly provides both arguments
- The viewModel properly extracts parameters from SavedStateHandle and uses them in the repository call at line 96
core/domain/src/commonMain/kotlin/com/mifos/core/domain/useCases/GetChargesTemplateUseCase.kt (1)
19-25: Verification complete: All callers have been updated to match the new signature.The only call site found in the codebase is in
ChargesViewModel.kt(line 337), which correctly uses the new signature:getChargeTemplateUseCase(route.resourceType, route.resourceId). No outdated single-parameter invocations remain.
| class ChargeRepositoryImp( | ||
| private val dataManagerCharge: DataManagerCharge, | ||
| ) : ChargeRepository { | ||
|
|
||
| override fun getListOfPagingCharges( | ||
| resourceType: String, | ||
| resourceId: Int, | ||
| ): Flow<PagingData<ChargesEntity>> { | ||
| return Pager( | ||
| config = PagingConfig( | ||
| pageSize = 10, | ||
| ), | ||
| pagingSourceFactory = { | ||
| ClientChargesPagingSource(resourceType, resourceId, dataManagerCharge) | ||
| }, | ||
| ).flow | ||
| } | ||
|
|
||
| override suspend fun getChargeTemplate(resourceType: String, resourceId: Int): ChargeTemplate { | ||
| return dataManagerCharge.getChargeTemplate(resourceType, resourceId) | ||
| } | ||
|
|
||
| override suspend fun createCharges( | ||
| resourceType: String, | ||
| resourceId: Int, | ||
| payload: ChargesPayload, | ||
| ): ChargeCreationResponse { | ||
| return dataManagerCharge.createCharges( | ||
| resourceId = resourceId, | ||
| resourceType = resourceType, | ||
| payload = payload, | ||
| ) | ||
| } | ||
|
|
||
| override suspend fun deleteCharge( | ||
| resourceId: Int, | ||
| resourceType: String, | ||
| chargeId: Int, | ||
| ): Flow<DataState<Unit>> { | ||
| return flow { | ||
| emit( | ||
| dataManagerCharge.deleteCharge( | ||
| resourceId = resourceId, | ||
| resourceType = resourceType, | ||
| chargeId = chargeId, | ||
| ), | ||
| ) | ||
| }.asDataStateFlow() | ||
| } | ||
|
|
||
| override suspend fun updateCharge( | ||
| resourceId: Int, | ||
| resourceType: String, | ||
| chargeId: Int, | ||
| payload: ChargesPayload, | ||
| ): Flow<DataState<Unit>> { | ||
| return flow { | ||
| emit( | ||
| dataManagerCharge.updateCharge( | ||
| resourceId = resourceId, | ||
| resourceType = resourceType, | ||
| chargeId = chargeId, | ||
| payload = payload, | ||
| ), | ||
| ) | ||
| }.asDataStateFlow() | ||
| } | ||
|
|
||
| override fun getListOfClientCharges( | ||
| resourceType: String, | ||
| resourceId: Int, | ||
| ): Flow<DataState<Page<ChargesEntity>>> { | ||
| return dataManagerCharge.getListOfClientCharges(resourceType, resourceId).asDataStateFlow() | ||
| } | ||
|
|
||
| override fun getListOfOtherAccountCharge( | ||
| resourceType: String, | ||
| resourceId: Int, | ||
| ): Flow<DataState<List<ChargesEntity>>> { | ||
| return dataManagerCharge.getListOfOtherAccountCharge(resourceType, resourceId) | ||
| .asDataStateFlow() | ||
| } | ||
|
|
||
| override fun getCharge( | ||
| resourceType: String, | ||
| resourceId: Int, | ||
| chargeId: Int, | ||
| ): Flow<DataState<ChargesEntity>> { | ||
| return dataManagerCharge.getCharge( | ||
| resourceId = resourceId, | ||
| resourceType = resourceType, | ||
| chargeId = chargeId, | ||
| ).asDataStateFlow() | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Inconsistent parameter ordering across methods.
The parameter order for resourceType and resourceId is inconsistent across methods in this class:
getListOfPagingCharges,getChargeTemplate,getListOfClientCharges,getListOfOtherAccountCharge:(resourceType, resourceId, ...)deleteCharge,updateCharge:(resourceId, resourceType, ...)getCharge:(resourceType, resourceId, ...)
This inconsistency is error-prone and violates the principle of least surprise. All methods should follow a consistent parameter order.
Apply this diff to standardize the parameter order to (resourceType, resourceId, ...):
override suspend fun deleteCharge(
- resourceId: Int,
resourceType: String,
+ resourceId: Int,
chargeId: Int,
): Flow<DataState<Unit>> {
return flow {
emit(
dataManagerCharge.deleteCharge(
- resourceId = resourceId,
resourceType = resourceType,
+ resourceId = resourceId,
chargeId = chargeId,
),
)
}.asDataStateFlow()
}
override suspend fun updateCharge(
- resourceId: Int,
resourceType: String,
+ resourceId: Int,
chargeId: Int,
payload: ChargesPayload,
): Flow<DataState<Unit>> {
return flow {
emit(
dataManagerCharge.updateCharge(
- resourceId = resourceId,
resourceType = resourceType,
+ resourceId = resourceId,
chargeId = chargeId,
payload = payload,
),
)
}.asDataStateFlow()
}Note: Ensure the corresponding interface definition in ChargeRepository.kt is also updated for consistency.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/ChargeRepositoryImp.kt
around lines 28 to 122, several methods use inconsistent parameter ordering for
resourceType and resourceId which is error‑prone; change the signatures of
deleteCharge and updateCharge to use (resourceType: String, resourceId: Int,
...) to match the others, update their internal calls and named-argument usage
accordingly, and update the ChargeRepository.kt interface to the same parameter
order (and any callers/tests) so all methods consistently use (resourceType,
resourceId, ...) throughout the codebase.
| 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.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Reconsider the naming and document the new spacing property.
The property name half is semantically misleading within this design system:
-
Naming inconsistency: All other spacing properties follow a semantic pattern describing relative size (
small,medium,large) or purpose. The namehalfreferences a mathematical relationship to another property rather than describing its intended use or relative magnitude. -
Missing documentation: The KDoc comment (lines 54-82) documents all other spacing properties but doesn't include this new
halfproperty. Please add a description of its purpose and intended use case. -
Verify the value: Please confirm that 500.dp is correct and not a typo for 50.dp. The value 50.dp would fit more naturally in the progression after
extraExtraLarge(48.dp), whereas 500.dp creates a significant jump and is an unusually large spacing value.
Consider renaming to something semantically meaningful (e.g., extraExtraExtraLarge, giant, maxOffset) based on its actual use case in the charge UI components.
Run the following script to examine how this spacing value is being used:
🏁 Script executed:
#!/bin/bash
# Description: Find usages of the new spacing.half property to understand the intended use case
# Search for usages of spacing.half or DesignToken.spacing.half
rg -n --type=kotlin 'spacing\.half|DesignToken\.spacing\.half' -C 3Length of output: 2561
Update spacing property naming for consistency with established design system patterns and add documentation.
The value 500.dp is confirmed as intentional—it's used deliberately as a maximum height constraint in the charges screens. However, two issues remain:
-
Naming inconsistency: The property name
halfbreaks the semantic pattern established by all other spacing properties (small,medium,large,extraLarge,extraExtraLarge). The name references a mathematical relationship to another property rather than describing its actual purpose or magnitude. In a design system, consistent naming is critical for developer experience. -
Missing documentation: Add the property to the KDoc comment (lines 54-82) with a description of its purpose and use case.
Suggested rename: Align with the size-based naming pattern—consider halfFull, extraExtraExtraLarge, or a more semantic name based on its actual layout purpose (e.g., halfScreenHeight, constrainedHeight).
🤖 Prompt for AI Agents
In
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/DesignToken.kt
around line 96, rename the spacing property `half` to a consistent, semantic
name (e.g., `constrainedHeight`) while leaving the value as 500.dp, update any
references/usages across the codebase to the new name, and add a KDoc entry for
this property within the existing KDoc block (lines 54-82) describing that it is
a 500.dp maximum height used for constraints on charges screens; ensure the new
property follows the same ordering and naming pattern as the other size-based
spacing properties.
core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerCharge.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 -> {} | ||
| } | ||
| }, | ||
| isExpandable = true, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Only first page of charges is rendered
chargesPagingList[0] grabs a single Page<ChargesEntity> and ignores every subsequent page emitted by the pager, so the user can never scroll past the first batch of results. Please iterate over all loaded pages (or flatten the paging stream to PagingData<ChargesEntity>) before feeding the list component.
- 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 -> {}
- }
- },
- isExpandable = true,
- )
- }
+ items(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 -> {}
+ }
+ },
+ isExpandable = true,
+ )
+ }
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| MifosTwoButtonRow( | ||
| firstBtnText = stringResource(Res.string.btn_back), | ||
| secondBtnText = stringResource(Res.string.action_add), | ||
| onFirstBtnClick = { | ||
| onAction(ChargesAction.DismissDialog) | ||
| }, | ||
| onSecondBtnClick = { | ||
| onAction(ChargesAction.DismissDialog) | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Add CTA never triggers the add-charge flow
Both buttons call ChargesAction.DismissDialog, so tapping “Add” just closes the sheet without launching the creation path promised by this PR. Please fire a dedicated add-charge action (e.g., ChargesAction.ShowCreateCharge or equivalent) so the view model can navigate into the add-charge workflow.
🤖 Prompt for AI Agents
In
feature/client/src/androidMain/kotlin/com/mifos/feature/client/charges/ShowClientCharge.android.kt
around lines 104 to 113, both buttons currently dispatch
ChargesAction.DismissDialog causing the “Add” CTA to only close the sheet;
change the onSecondBtnClick to dispatch the dedicated add-charge action (e.g.,
ChargesAction.ShowCreateCharge or the existing action used to start the
add-charge flow) instead of DismissDialog so the view model can navigate to the
create-charge workflow; update imports if necessary and leave the first button
dispatching ChargesAction.DismissDialog to preserve the back behavior.
feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ChargesViewModel.kt
Show resolved
Hide resolved
| private suspend fun updateCharge(payload: ChargesPayload, chargeId: Int?) { | ||
| /** | ||
| * It it check when Client is Update then it through error | ||
| * because client update API not have currently. | ||
| */ | ||
| if (route.resourceType == Constants.ENTITY_TYPE_CLIENTS) { | ||
| mutableStateFlow.update { | ||
| it.copy( | ||
| dialogState = ChargesState.DialogState.ShowStatusDialog( | ||
| ResultStatus.FAILURE, | ||
| message = getString(Res.string.charges_client_update_not_support_error_msg), | ||
| ), | ||
| ) | ||
| } | ||
| } else { | ||
| chargeId?.let { chargeId -> | ||
| repository.updateCharge( | ||
| resourceType = route.resourceType, | ||
| resourceId = route.resourceId, | ||
| payload = payload, | ||
| chargeId = chargeId, | ||
| ).collect { result -> | ||
| when (result) { | ||
| is DataState.Error -> { | ||
| mutableStateFlow.update { | ||
| it.copy( | ||
| dialogState = ChargesState.DialogState.ShowStatusDialog( | ||
| ResultStatus.FAILURE, | ||
| message = result.message, | ||
| ), | ||
| isOverlayLoading = false, | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| is DataState.Loading -> { | ||
| mutableStateFlow.update { | ||
| it.copy( | ||
| dialogState = ChargesState.DialogState.Loading, | ||
| isOverlayLoading = true, | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| is DataState.Success -> { | ||
| mutableStateFlow.update { | ||
| it.copy( | ||
| dialogState = ChargesState.DialogState.ShowStatusDialog( | ||
| ResultStatus.SUCCESS, | ||
| ), | ||
| isOverlayLoading = false, | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move business logic out of the ViewModel.
Lines 411-419 contain business logic that restricts client charge updates due to missing API support. This logic should be in the repository or use case layer, not the ViewModel, to maintain separation of concerns and improve testability.
Consider moving this check to ChargeRepository.updateCharge:
In ChargeRepository.kt or ChargeRepositoryImp.kt:
override suspend fun updateCharge(
resourceId: Int,
resourceType: String,
chargeId: Int,
payload: ChargesPayload,
): Flow<DataState<Unit>> {
return flow {
if (resourceType == Constants.ENTITY_TYPE_CLIENTS) {
emit(DataState.Error("Client charge updates are not currently supported"))
} else {
emit(
dataManagerCharge.updateCharge(
resourceId = resourceId,
resourceType = resourceType,
chargeId = chargeId,
payload = payload,
),
)
}
}.asDataStateFlow()
}Then simplify the ViewModel to just call the repository without conditional logic.
🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ChargesViewModel.kt
around lines 406-464 the ViewModel contains business logic (lines ~411-419) that
blocks client charge updates due to missing API support; move this conditional
into the repository/use-case layer so the ViewModel only reacts to DataState
results. Update ChargeRepository (ChargeRepository.kt or ChargeRepositoryImp.kt)
to perform the resourceType == Constants.ENTITY_TYPE_CLIENTS check and emit a
DataState.Error with the appropriate message when client updates are not
supported, otherwise call the dataManager/network layer to perform the update;
then simplify the ViewModel to always call repository.updateCharge(...) and
handle Loading/Success/Error states from the returned Flow without containing
the resource-type business rule itself.
| data class ClientUpcomingChargesRoute( | ||
| val clientId: Int = -1, | ||
| val resourceId: Int = -1, | ||
| val resourceType: String = "", | ||
| ) |
There was a problem hiding this comment.
Empty string default for resourceType may cause issues.
The empty string default for resourceType could lead to runtime errors or unexpected behavior if the route is constructed without explicit parameters. Consider either removing the default value to force explicit specification or validating that resourceType is non-empty.
Additionally, consider using an enum or sealed class for resourceType instead of String for better type safety and to prevent invalid resource type values:
enum class ResourceType {
CLIENT, LOAN, SAVINGS, GROUP;
override fun toString(): String = name.lowercase()
}
data class ClientUpcomingChargesRoute(
val resourceId: Int = -1,
val resourceType: ResourceType = ResourceType.CLIENT,
)This would make it impossible to pass invalid resource types and provide better IDE support.
🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientUpcomingCharges/ClientUpcomingChargesRoute.kt
around lines 18-21, the data class currently uses an empty string default for
resourceType which can lead to runtime errors; change the API to use a typed
enum or sealed class for resourceType (e.g., ResourceType with values CLIENT,
LOAN, SAVINGS, GROUP) and replace the String type with that enum (optionally
default to ResourceType.CLIENT) or remove the default to force callers to
provide an explicit value; then update all call sites to pass the enum value and
remove/adjust any string-based comparisons or serialization, or if you must keep
String instead add a non-empty validation in the constructor/factory to throw a
clear error when resourceType is empty.
| actual fun ShowClientCharge( | ||
| pagingFlow: Flow<PagingData<ChargesEntity>>, | ||
| onRetry: () -> Unit, | ||
| onAction: (ChargesAction) -> Unit, | ||
| ) { |
There was a problem hiding this comment.
Actual signature diverges from the expect contract
The common expect now exposes Flow<PagingData<Page<ChargesEntity>>>, but the desktop actual still uses PagingData<ChargesEntity>. This breaks the multiplatform linkage at compile time. Please align the signature (and import Page) with the new expect declaration.
+import com.mifos.core.model.objects.clients.Page
import androidx.paging.PagingData
import com.mifos.room.entities.client.ChargesEntity
import kotlinx.coroutines.flow.Flow
@Composable
actual fun ShowClientCharge(
- pagingFlow: Flow<PagingData<ChargesEntity>>,
+ pagingFlow: Flow<PagingData<Page<ChargesEntity>>>,
onAction: (ChargesAction) -> Unit,
)📝 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.
| actual fun ShowClientCharge( | |
| pagingFlow: Flow<PagingData<ChargesEntity>>, | |
| onRetry: () -> Unit, | |
| onAction: (ChargesAction) -> Unit, | |
| ) { | |
| actual fun ShowClientCharge( | |
| pagingFlow: Flow<PagingData<Page<ChargesEntity>>>, | |
| onAction: (ChargesAction) -> Unit, | |
| ) { |
🤖 Prompt for AI Agents
In
feature/client/src/desktopMain/kotlin/com/mifos/feature/client/charges/ShowClientCharge.desktop.kt
around lines 23 to 26, the actual function signature uses
Flow<PagingData<ChargesEntity>> but the expect contract now requires
Flow<PagingData<Page<ChargesEntity>>>; update the function parameter type to
Flow<PagingData<Page<ChargesEntity>>>, add the necessary import for Page, and
adapt any local usages/consumers inside this file to handle the Page wrapper
(e.g., unwrap or access the entity via page.content) so the desktop actual
matches the expect declaration.
| actual fun ShowClientCharge( | ||
| pagingFlow: Flow<PagingData<ChargesEntity>>, | ||
| onRetry: () -> Unit, | ||
| onAction: (ChargesAction) -> Unit, | ||
| ) { |
There was a problem hiding this comment.
Actual signature diverges from the expect contract
The native actual still takes Flow<PagingData<ChargesEntity>>, while the expect/Android actual use Flow<PagingData<Page<ChargesEntity>>>. This mismatch breaks compilation for the native target. Please update the signature (and add the Page import) to match the common declaration.
+import com.mifos.core.model.objects.clients.Page
import androidx.paging.PagingData
import com.mifos.room.entities.client.ChargesEntity
import kotlinx.coroutines.flow.Flow
@Composable
actual fun ShowClientCharge(
- pagingFlow: Flow<PagingData<ChargesEntity>>,
+ pagingFlow: Flow<PagingData<Page<ChargesEntity>>>,
onAction: (ChargesAction) -> Unit,
)📝 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.
| actual fun ShowClientCharge( | |
| pagingFlow: Flow<PagingData<ChargesEntity>>, | |
| onRetry: () -> Unit, | |
| onAction: (ChargesAction) -> Unit, | |
| ) { | |
| import com.mifos.core.model.objects.clients.Page | |
| import androidx.paging.PagingData | |
| import com.mifos.room.entities.client.ChargesEntity | |
| import kotlinx.coroutines.flow.Flow | |
| @Composable | |
| actual fun ShowClientCharge( | |
| pagingFlow: Flow<PagingData<Page<ChargesEntity>>>, | |
| onAction: (ChargesAction) -> Unit, | |
| ) { |
🤖 Prompt for AI Agents
In
feature/client/src/nativeMain/kotlin/com/mifos/feature/client/charges/ClientChargesScreen.native.kt
around lines 23 to 26, the actual function signature uses
Flow<PagingData<ChargesEntity>> but must match the expect contract and use
Flow<PagingData<Page<ChargesEntity>>>; change the parameter type accordingly and
add the import for Page (import androidx.paging.Page or the correct Page type
used in common code), then update any local references in the function to the
new Page-wrapped type so the native implementation compiles with the common
declaration.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ChargesScreen.kt (1)
289-309: Fix date picker initialization for edit flows (previously reported).Both date pickers are initialized with
Clock.System.now()regardless of existing state values, which causes two critical problems:
- Edit mode bug: When editing an existing charge, opening and confirming a date picker without changing the date overwrites the stored date with today's date.
- Past date restriction: The collected-on picker's
SelectableDatesconstraint prevents users from recording payments collected in the past (e.g., earlier this week).Apply this diff to fix the initialization:
+ // Helper to parse existing date or fall back to now + fun parseDateOrNow(dateString: String): Long { + return if (dateString.isBlank()) { + Clock.System.now().toEpochMilliseconds() + } else { + DateHelper.getLongFromString(dateString) ?: Clock.System.now().toEpochMilliseconds() + } + } + val dueDatePickerState = key(state.dueDate) { rememberDatePickerState( - initialSelectedDateMillis = Clock.System.now().toEpochMilliseconds(), + initialSelectedDateMillis = parseDateOrNow(state.dueDate), selectableDates = object : SelectableDates { override fun isSelectableDate(utcTimeMillis: Long): Boolean { return utcTimeMillis >= Clock.System.now().toEpochMilliseconds() } }, ) } val collectedOnDatePickerState = key(state.collectedOn) { rememberDatePickerState( - initialSelectedDateMillis = Clock.System.now().toEpochMilliseconds(), + initialSelectedDateMillis = parseDateOrNow(state.collectedOn), selectableDates = object : SelectableDates { override fun isSelectableDate(utcTimeMillis: Long): Boolean { - return utcTimeMillis >= Clock.System.now().toEpochMilliseconds() + return true // Allow past dates for collected-on } }, ) }
🧹 Nitpick comments (2)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ChargesScreen.kt (2)
151-152: Consider using string formatting for better i18n support.The text concatenation using
+operators can be problematic for internationalization. Consider using a formatted string resource instead.
311-375: Consider extracting date picker dialog to reduce duplication.The two date picker dialogs (lines 311-342 and 344-375) share nearly identical structure and only differ in the state variables and actions. Consider extracting a reusable
ChargeDatePickerDialogcomposable to improve maintainability.Example structure:
@Composable private fun ChargeDatePickerDialog( showPicker: Boolean, datePickerState: DatePickerState, onDismiss: () -> Unit, onConfirm: (String) -> Unit, ) { if (showPicker) { DatePickerDialog( onDismissRequest = onDismiss, confirmButton = { TextButton( onClick = { onDismiss() datePickerState.selectedDateMillis?.let { onConfirm(DateHelper.getDateAsStringFromLong(it)) } } ) { Text(stringResource(Res.string.feature_client_charge_select)) } }, dismissButton = { TextButton(onClick = onDismiss) { Text(stringResource(Res.string.feature_client_charge_cancel)) } }, ) { DatePicker(state = datePickerState) } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosActionsListingCardComponent.kt(4 hunks)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosAllUiComponentsPreview.kt(1 hunks)feature/client/src/androidMain/kotlin/com/mifos/feature/client/charges/ShowClientCharge.android.kt(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ChargesScreen.kt(1 hunks)feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountScreen.kt(4 hunks)feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountScreen.kt(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- feature/client/src/androidMain/kotlin/com/mifos/feature/client/charges/ShowClientCharge.android.kt
🧰 Additional context used
🧬 Code graph analysis (1)
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)
🔇 Additional comments (7)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ChargesScreen.kt (2)
97-122: LGTM!The screen composable follows proper patterns: state collection with lifecycle awareness, event handling for navigation, and appropriate delegation to content and dialog composables.
377-494: LGTM on the form structure!The charge add/edit form is well-structured with proper validation, conditional rendering, and clear user actions. The toggle between collapsed and expanded states provides good UX.
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosActionsListingCardComponent.kt (2)
617-684: LGTM! External state correctly wired throughout.The implementation consistently uses
isExpandedandonExpandTogglethroughout the component, properly removing all internal state management.
610-611: No migration issues found. API refactoring complete and consistent across the codebase.The parameter change from
isExpandable: Boolean = truetoisExpanded: BooleanandonExpandToggle: () -> Unithas been fully migrated. No remaining usages of the old parameter exist, and all call sites across feature modules (savings, note, loan, client, etc.) correctly use the new controlled component API.feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountScreen.kt (1)
303-343: LGTM! Correct adaptation to the new expansion API.The implementation correctly manages expansion state externally using
rememberSaveable, ensuring state persistence across configuration changes. The accordion pattern (single item expanded at a time) is properly implemented.core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosAllUiComponentsPreview.kt (1)
936-937: LGTM! Preview correctly updated for the new API.The preview appropriately demonstrates the component with the new external state management pattern.
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountScreen.kt (1)
451-511: LGTM! Correct adaptation to the new expansion API.The implementation properly manages expansion state using
rememberSaveable. The sharedexpandedIndexacross both charge lists (overdue and added) is acceptable since the dialog renders them mutually exclusively based on theisOverDueflag.
| chargeTitle = it.name.toString(), | ||
| type = it.chargeCalculationType?.value.toString(), | ||
| date = it.formattedDueDate, | ||
| collectedOn = it.formattedDueDate, |
There was a problem hiding this comment.
Fix incorrect property usage for collected-on date.
The collectedOn parameter is incorrectly displaying it.formattedDueDate. This should display the actual collected-on date property from the charge object.
Apply this diff to fix the property:
- collectedOn = it.formattedDueDate,
+ collectedOn = it.formattedCollectedOn,Note: Verify the correct property name from your charge data model.
📝 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.
| collectedOn = it.formattedDueDate, | |
| collectedOn = it.formattedCollectedOn, |
🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ChargesScreen.kt
around line 243, the collectedOn field is incorrectly set to
it.formattedDueDate; replace that with the charge object's actual collected-on
property (e.g., use the model's it.collectedOn or the correct collected-on
property name from the charge data model) so the screen shows the real
collected-on date.
| MifosTwoButtonRow( | ||
| firstBtnText = stringResource(Res.string.btn_back), | ||
| secondBtnText = stringResource(Res.string.action_add), | ||
| onFirstBtnClick = { | ||
| onAction(ChargesAction.DismissDialog) | ||
| }, | ||
| onSecondBtnClick = { | ||
| onAction(ChargesAction.DismissDialog) | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Fix the Add button action in the bottom sheet.
The second button labeled "Add" (line 268) only dismisses the dialog when clicked (line 273), but it should trigger the action to show the charge add fields. This prevents users from adding charges from the bottom sheet view.
Apply this diff to fix the button action:
onSecondBtnClick = {
- onAction(ChargesAction.DismissDialog)
+ onAction(ChargesAction.OnShowAddCharge)
},📝 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.
| MifosTwoButtonRow( | |
| firstBtnText = stringResource(Res.string.btn_back), | |
| secondBtnText = stringResource(Res.string.action_add), | |
| onFirstBtnClick = { | |
| onAction(ChargesAction.DismissDialog) | |
| }, | |
| onSecondBtnClick = { | |
| onAction(ChargesAction.DismissDialog) | |
| }, | |
| ) | |
| MifosTwoButtonRow( | |
| firstBtnText = stringResource(Res.string.btn_back), | |
| secondBtnText = stringResource(Res.string.action_add), | |
| onFirstBtnClick = { | |
| onAction(ChargesAction.DismissDialog) | |
| }, | |
| onSecondBtnClick = { | |
| onAction(ChargesAction.OnShowAddCharge) | |
| }, | |
| ) |
🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ChargesScreen.kt
around lines 266-275, the second "Add" button currently calls
ChargesAction.DismissDialog which only closes the sheet; replace that call so
the onSecondBtnClick dispatches the action that opens the add-charge fields
(e.g., ChargesAction.ShowAddChargeFields or the existing action that displays
the add form) instead of DismissDialog so clicking Add shows the charge add
fields.
# Conflicts: # feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDetailsProfile/ClientProfileDetailsNavigation.kt
# Conflicts: # feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountScreen.kt
# Conflicts: # feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountScreen.kt
Arinyadav1
force-pushed
the
Client-add-charge
branch
from
ffa3ca3 to
dc12fdb
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (13)
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/DesignToken.kt (1)
96-96: Naming inconsistency and missing documentation persist.The
halfproperty still breaks the established semantic naming pattern and lacks KDoc documentation, as previously flagged.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientUpcomingCharges/ClientUpcomingChargesRoute.kt (1)
18-21: Empty string default and type safety concerns persist.The empty string default for
resourceTypeand lack of type safety were previously flagged and remain unresolved.feature/client/src/nativeMain/kotlin/com/mifos/feature/client/charges/ClientChargesScreen.native.kt (1)
23-26: Signature mismatch breaks native compilation.The actual signature uses
Flow<PagingData<ChargesEntity>>but the expect requiresFlow<PagingData<Page<ChargesEntity>>>, as previously flagged.feature/client/src/desktopMain/kotlin/com/mifos/feature/client/charges/ShowClientCharge.desktop.kt (1)
23-26: Signature mismatch breaks desktop compilation.The actual signature uses
Flow<PagingData<ChargesEntity>>but the expect requiresFlow<PagingData<Page<ChargesEntity>>>, as previously flagged.feature/client/src/androidMain/kotlin/com/mifos/feature/client/charges/ShowClientCharge.android.kt (2)
65-111: Only first page of charges is rendered.Line 65 grabs a single page and ignores subsequent pages emitted by the pager, preventing users from scrolling past the first batch of results. Please iterate over all loaded pages or use the paging items directly.
113-122: "Add" button never triggers the add-charge flow.Both buttons dispatch
ChargesAction.DismissDialog, so tapping "Add" just closes the sheet without launching the creation path. Please fire a dedicated add-charge action instead ofDismissDialogon Line 120.core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerCharge.kt (1)
63-80: Cache collision: different resource types with same ID will overwrite each other's cached charges.Lines 69 and 73 persist and retrieve charges using only
resourceId, so a client and a loan with the same ID will share the same cache slot. When offline, the wrong charges will be returned. Please extend the DAO schema/API to includeresourceTypeas part of the cache key.core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/ChargeRepositoryImp.kt (1)
62-76: Inconsistent parameter ordering across methods.
deleteChargeandupdateChargeuse(resourceId, resourceType, ...)while all other methods use(resourceType, resourceId, ...). This inconsistency is error-prone and violates the principle of least surprise. Please standardize all methods to use(resourceType, resourceId, ...)order.Also applies to: 78-94
feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ChargesViewModel.kt (2)
406-464: Move business logic out of the ViewModel.Lines 411-419 contain business logic that restricts client charge updates due to missing API support. This logic should be in the repository or use case layer, not the ViewModel, to maintain separation of concerns and improve testability.
Consider moving this check to
ChargeRepository.updateCharge:In ChargeRepository.kt or ChargeRepositoryImp.kt:
override suspend fun updateCharge( resourceId: Int, resourceType: String, chargeId: Int, payload: ChargesPayload, ): Flow<DataState<Unit>> { return flow { if (resourceType == Constants.ENTITY_TYPE_CLIENTS) { emit(DataState.Error("Client charge updates are not currently supported")) } else { emit( dataManagerCharge.updateCharge( resourceId = resourceId, resourceType = resourceType, chargeId = chargeId, payload = payload, ), ) } }.asDataStateFlow() }Then simplify the ViewModel to just call the repository without conditional logic.
308-330: Fix potential IndexOutOfBoundsException when charge option is not found.Line 311 uses
indexOfFirstwhich returns-1if no matching element is found. This-1value is stored inchargeOptionIndex(line 317) and potentially used at lines 318-321 with.get(item), which would throw anIndexOutOfBoundsException.Apply this diff to handle the case when the charge option is not found:
is DataState.Success -> { mutableStateFlow.update { val index = state.chargeTemplate?.chargeOptions?.indexOfFirst { item -> item.name == dataState.data.name } + ?.takeIf { it >= 0 } it.copy( amount = dataState.data.amount.toString(), dueDate = DateHelper.getDateAsString( dataState.data.dueDate ?: emptyList(), ), chargeOptionIndex = index, chargeOptionId = index?.let { item -> state.chargeTemplate?.chargeOptions?.get( item, )?.id }, dialogState = null, showChargeAddFields = true, isOverlayLoading = false, isUpdate = true, chargeId = chargeId, ) } }feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ChargesScreen.kt (3)
239-239: Fix incorrect property usage for collected-on date.The
collectedOnparameter is incorrectly displayingit.formattedDueDate. This displays the due date instead of the actual collected-on date from the charge object.Apply this diff to fix the property:
- collectedOn = it.formattedDueDate, + collectedOn = it.formattedCollectedOn,Note: Verify the correct property name from your charge data model (e.g.,
formattedCollectedOn,collectedOn, or similar).
262-271: Fix the Add button action in the bottom sheet.The second button labeled "Add" (line 264) only dismisses the dialog when clicked (line 269), but it should trigger the action to show the charge add fields. This prevents users from adding charges from the bottom sheet view.
Apply this diff to fix the button action:
onSecondBtnClick = { - onAction(ChargesAction.DismissDialog) + onAction(ChargesAction.OnShowAddCharge) },
290-298: Fix date picker initialization to respect existing state.The date picker always seeds
rememberDatePickerStatewithClock.System.now().toEpochMilliseconds()(line 292), which overwrites any previously stored due date when opening the picker. For edit flows, this forces the date to "today" even if a different date was already selected.Derive
initialSelectedDateMillisfrom the currentstate.dueDateby parsing the existing string:val dueDatePickerState = key(state.dueDate) { rememberDatePickerState( - initialSelectedDateMillis = Clock.System.now().toEpochMilliseconds(), + initialSelectedDateMillis = runCatching { + DateHelper.getDateAsLongFromString(state.dueDate) + }.getOrElse { Clock.System.now().toEpochMilliseconds() }, selectableDates = object : SelectableDates { override fun isSelectableDate(utcTimeMillis: Long): Boolean { return utcTimeMillis >= Clock.System.now().toEpochMilliseconds() } }, ) }Note: Verify the correct DateHelper method name for parsing date strings to milliseconds.
🧹 Nitpick comments (2)
core/data/src/commonMain/kotlin/com/mifos/core/data/repository/ChargeRepository.kt (2)
39-50: Reconsider returning Flow for suspend single-shot operations.Both
deleteChargeandupdateChargeare markedsuspendand returnFlow<DataState<Unit>>. For single-shot operations, this creates redundancy—callers must bothsuspendand collect a Flow. Consider returningDataState<Unit>directly or removingsuspendif streaming is truly needed.Apply this pattern:
- suspend fun deleteCharge( + suspend fun deleteCharge( resourceId: Int, resourceType: String, chargeId: Int, - ): Flow<DataState<Unit>> + ): DataState<Unit> - suspend fun updateCharge( + suspend fun updateCharge( resourceId: Int, resourceType: String, chargeId: Int, payload: ChargesPayload, - ): Flow<DataState<Unit>> + ): DataState<Unit>
26-29: Consider using a type-safe enum for resourceType.All methods accept
resourceType: String, which lacks compile-time safety and can lead to runtime errors from invalid values. Consider defining aResourceTypeenum (e.g.,CLIENT,LOAN,SAVINGS,GROUP) for better type safety across the repository interface.Also applies to: 31-31, 33-37, 39-43, 45-50, 52-55, 57-60, 62-66
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (41)
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/services/ChargeService.kt(1 hunks)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosActionsListingCardComponent.kt(4 hunks)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosAllUiComponentsPreview.kt(1 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(1 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/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/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/navigation/ClientNavigation.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/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountScreen.kt(3 hunks)
💤 Files with no reviewable changes (10)
- feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesScreen.android.kt
- feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientChargeDialog/ChargeDialogScreen.kt
- core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/ClientChargeRepositoryImp.kt
- feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesScreen.kt
- core/data/src/commonMain/kotlin/com/mifos/core/data/repository/ChargeDialogRepository.kt
- core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/ChargeDialogRepositoryImp.kt
- core/network/src/commonMain/kotlin/com/mifos/core/network/DataManager.kt
- feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesViewModel.kt
- feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientChargeDialog/ChargeDialogUiState.kt
- feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargeUiState.kt
🚧 Files skipped from review as they are similar to previous changes (11)
- feature/client/src/commonMain/kotlin/com/mifos/feature/client/di/ClientModule.kt
- feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientUpcomingCharges/ClientUpcomingChargesScreen.android.kt
- core/domain/src/commonMain/kotlin/com/mifos/core/domain/di/UseCaseModule.kt
- core/database/src/commonMain/kotlin/com/mifos/room/helper/ChargeDaoHelper.kt
- feature/client/src/commonMain/composeResources/values/strings.xml
- core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosAllUiComponentsPreview.kt
- core/domain/src/commonMain/kotlin/com/mifos/core/domain/useCases/GetChargesTemplateUseCase.kt
- feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountScreen.kt
- feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDetailsProfile/ClientProfileDetailsScreen.kt
- core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosActionsListingCardComponent.kt
- feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ChargesRoute.kt
🧰 Additional context used
🧬 Code graph analysis (5)
core/data/src/commonMain/kotlin/com/mifos/core/data/pagingSource/ClientChargesPagingSource.kt (4)
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)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)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ChargesRoute.kt (1)
chargesDestination(23-33)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ChargesViewModel.kt (4)
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)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)
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/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)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR Checks / Build Android Application
🔇 Additional comments (11)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientUpcomingCharges/ClientUpcomingChargesViewmodel.kt (2)
18-18: LGTM - Repository refactor aligns with architecture.The migration from
ClientChargeRepositorytoChargeRepositoryis consistent with the PR's resource-based architecture refactor.Also applies to: 31-31
96-96: LGTM - Method call updated for resource-based routing.The call correctly uses
route.resourceTypeandroute.resourceId, aligning with the new resource-based API surface.core/data/src/commonMain/kotlin/com/mifos/core/data/repository/ChargeRepository.kt (1)
31-31: LGTM - Suspend functions for single-shot operations.These operations correctly use
suspendmodifiers for direct return values without unnecessary Flow wrapping.Also applies to: 33-37
feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ShowClientCharge.kt (1)
18-22: LGTM - Multiplatform expect function defined.The expect function establishes the contract for platform-specific implementations. Ensure actual implementations match this signature.
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDetailsProfile/ClientProfileDetailsNavigation.kt (1)
34-34: LGTM!The navigation callback is properly threaded through the destination to enable the add-charge flow.
Also applies to: 49-49
core/domain/src/commonMain/kotlin/com/mifos/core/domain/useCases/CreateChargesUseCase.kt (1)
20-29: LGTM!The refactoring from client-specific to resource-based charge creation is consistent and correctly implemented.
core/data/src/commonMain/kotlin/com/mifos/core/data/di/RepositoryModule.kt (1)
16-16: LGTM!The DI wiring correctly reflects the repository consolidation from client-specific to resource-based charge management.
Also applies to: 78-78, 153-153
feature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt (1)
20-21: LGTM!The navigation refactoring consistently migrates to resource-based routing using
navigateToChargesRoutewithENTITY_TYPE_CLIENTS.Also applies to: 130-132, 148-151
core/data/src/commonMain/kotlin/com/mifos/core/data/pagingSource/ClientChargesPagingSource.kt (1)
18-62: LGTM!The paging source correctly adapts to resource-based parameters while preserving the paging semantics.
core/network/src/commonMain/kotlin/com/mifos/core/network/services/ChargeService.kt (1)
28-191: Well-structured resource-generic API refactoring.The refactoring from client-specific to resource-generic endpoints is well-executed:
- Consistent
{resourceType}/{resourceId}pattern across all new endpoints- Comprehensive KDoc documentation for each method
- Legacy endpoints appropriately marked with TODO comments for future removal
- Proper use of Flow vs suspend for streaming vs single responses
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountScreen.kt (1)
44-44: LGTM: Imports correctly support the new expansion state.The added imports are necessary for implementing per-item expansion state with configuration change survival.
Also applies to: 46-47
| navigateToClientDetailsScreen: (Int) -> Unit, | ||
| viewAddress: (Int) -> Unit, | ||
| viewAssociatedAccounts: (Int) -> Unit, | ||
| navigateToAddCharge: (Int) -> Unit, |
There was a problem hiding this comment.
Unused parameter - not forwarded to ClientProfileScreen.
The navigateToAddCharge parameter is accepted but never passed to ClientProfileScreen (lines 36-45). Either forward it to the screen or remove it from this function signature.
🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientProfile/ClientProfileNavigation.kt
around line 33, the navigateToAddCharge parameter is declared but not forwarded
to ClientProfileScreen; update the call site to pass navigateToAddCharge through
to ClientProfileScreen (or remove the parameter from this function signature if
the screen doesn't need it). Ensure the function signature and the
ClientProfileScreen invocation remain consistent: either add navigateToAddCharge
as an argument to ClientProfileScreen where other navigation callbacks are
passed, or delete the unused parameter and any related unused imports/usages.
| state: NewLoanAccountState, | ||
| onAction: (NewLoanAccountAction) -> Unit, | ||
| ) { | ||
| var expandedIndex: Int? by rememberSaveable { mutableStateOf(-1) } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Fix type inconsistency in expandedIndex declaration.
The variable is declared as Int? but initialized with mutableStateOf(-1), which creates MutableState<Int>. When using -1 as a sentinel value, declare the variable as non-nullable Int. Alternatively, if you prefer nullable semantics, use null instead of -1.
Recommended approach (use non-nullable Int):
- var expandedIndex: Int? by rememberSaveable { mutableStateOf(-1) }
+ var expandedIndex: Int by rememberSaveable { mutableStateOf(-1) }Alternative approach (use nullable Int with null sentinel):
- var expandedIndex: Int? by rememberSaveable { mutableStateOf(-1) }
+ var expandedIndex: Int? by rememberSaveable { mutableStateOf(null) }If you choose the alternative, also update the toggle logic to use null instead of -1:
expandedIndex = if (expandedIndex == it.id) null else it.id🤖 Prompt for AI Agents
In
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountScreen.kt
around line 453, the variable expandedIndex is declared as nullable Int (Int?)
but initialized with mutableStateOf(-1), causing a type mismatch between Int?
and MutableState<Int>; change the declaration to a non-nullable Int (var
expandedIndex: Int by rememberSaveable { mutableStateOf(-1) }) so the sentinel
-1 matches the type, or if you prefer nullable semantics initialize with null
(var expandedIndex: Int? by rememberSaveable { mutableStateOf<Int?>(null) }) and
update any toggle logic to compare against null (e.g., set to null when
collapsing and to the item id when expanding).
| isExpanded = expandedIndex == it.id, | ||
| onExpandToggle = { | ||
| expandedIndex = if (expandedIndex == it.id) -1 else it.id | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that charge objects have a unique id field.
The expansion logic uses it.id to track which item is expanded. Ensure that both overdueCharges and addedCharges objects have a unique id field; otherwise, this will cause runtime errors or incorrect expansion behavior.
Run the following script to verify the charge object structure:
Also applies to: 508-511
🏁 Script executed:
#!/bin/bash
# Description: Verify that charge objects have an id field
# Search for the definition of addedCharges state property
echo "=== Searching for addedCharges type definition ==="
rg -n "addedCharges" --type=kt -A 2 -B 2
echo -e "\n=== Searching for overdueCharges in template ==="
rg -n "overdueCharges" --type=kt -A 2 -B 2
echo -e "\n=== Searching for charge data classes or models ==="
ast-grep --pattern 'data class $CHARGE {
$$$
id: $TYPE
$$$
}'Length of output: 207
🏁 Script executed:
#!/bin/bash
# Description: Verify charge objects have id field - corrected search
echo "=== Searching for addedCharges property definition ==="
rg -n "addedCharges" -g '*.kt' -A 3 -B 3
echo -e "\n=== Searching for overdueCharges property definition ==="
rg -n "overdueCharges" -g '*.kt' -A 3 -B 3
echo -e "\n=== Searching for charge-related data classes ==="
rg -n "data class.*Charge" -g '*.kt' -A 5
echo -e "\n=== Searching for id field in charge objects ==="
rg -n "id.*:" -g '*.kt' | grep -i charge -A 2 -B 2 || echo "No direct matches found"Length of output: 91591
🏁 Script executed:
#!/bin/bash
# Alternative: Search in common locations for model definitions
find . -name "*Charge*.kt" -o -name "*Model*.kt" -o -name "*State*.kt" 2>/dev/null | head -20Length of output: 1781
Use the list index instead of charge object id for expansion state tracking; all new charges default to id = -1, causing incorrect expansion behavior.
The expansion logic relies on it.id to track which charge is expanded, but CreatedCharges objects default to id = -1. When multiple newly-added charges share the same ID, toggling expansion on one unexpectedly affects all others.
Use the loop index instead:
- Replace
expandedIndex = if (expandedIndex == it.id) -1 else it.idwithexpandedIndex = if (expandedIndex == index) -1 else index - Update the condition
isExpanded = expandedIndex == it.idtoisExpanded = expandedIndex == index
Also applies to: 508-511 (addedCharges loop)
🤖 Prompt for AI Agents
In
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountScreen.kt
around lines 477-480 and 508-511, the expansion state uses charge.id which
defaults to -1 for new charges causing multiple items to share the same key;
replace uses of it.id with the loop index so expansion is tracked
per-list-position: change isExpanded comparisons from expandedIndex == it.id to
expandedIndex == index and change the toggle assignment from expandedIndex = if
(expandedIndex == it.id) -1 else it.id to expandedIndex = if (expandedIndex ==
index) -1 else index in both the shown loop and the addedCharges loop.
Co-authored-by: Shreyash16b <shreyashborde@gmail.com>
3 participants
Fixes - Jira-#566
IMG_4981.MOV
Summary by CodeRabbit
Release Notes
New Features
Refactor
Style