Client Add Charges #2518
Client Add Charges #2518
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughRefactors client charges: removes legacy ChargeDialog and platform ClientChargeContent implementations, centralizes state and actions into a BaseViewModel-backed ClientChargesViewModel/ClientChargesState, introduces route-based navigation for client charges, adds string resources, updates repository API for total counts, and bumps several Compose/paging dependencies. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Screen as ClientChargesScreen
participant VM as ClientChargesViewModel
participant Repo as ClientChargeRepository
participant State as ClientChargesState
User->>Screen: NavigateToClientCharges(clientId)
Screen->>VM: init(clientId) / subscribe state
VM->>Repo: loadCharges(clientId) -> returns PagingData flow
Repo-->>VM: PagingData flow
VM->>State: update chargesFlow
User->>Screen: Open Add Charge
Screen->>VM: Action(LoadChargeTemplate)
VM->>Repo: getChargeTemplate()
Repo-->>VM: ChargeTemplate
VM->>State: DialogState = AllChargesV2
User->>Screen: Submit Charge(payload)
Screen->>VM: Action(CreateCharge(payload))
VM->>Repo: createChargesUseCase(payload)
Repo-->>VM: ResultStatus (Success/Error)
alt Success
VM->>State: DialogState = ShowStatusDialog(Success)
VM->>Repo: refreshCharges()
VM->>State: close dialog / update chargesFlow
else Error
VM->>State: DialogState = ShowStatusDialog(Error)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–75 minutes Potential hotspots to review:
Pre-merge checks and finishing touches✅ Passed checks (2 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: 7
🧹 Nitpick comments (15)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesViewModel.kt (3)
288-296: Avoid Any? for chargesFlow; introduce a common typealias or UI model.Using Any? erodes type safety and invites runtime casts. Define a platform‑agnostic type for the paging flow in commonMain and keep the state strongly typed.
Example approach (sketch):
// commonMain expect class ChargeItem typealias ChargesPagingFlow = Flow<PagingData<ChargeItem>> data class ClientChargesState( /* ... */ - val chargesFlow: Any? = null, + val chargesFlow: ChargesPagingFlow? = null, /* ... */ )Then provide actuals:
- Android/desktop: actual typealias ChargeItem = com.mifos.room.entities.client.ChargesEntity
- Or map to a shared ChargeUiModel.
Also applies to: 190-231
295-296: Default dialog state as Loading may show a spinner on first render; add an Idle state.Unless UI gates on showAddCharges, users might see a loading indicator prematurely. Prefer an explicit Idle to represent “no dialog”.
- sealed class ChargeDialogState { + sealed class ChargeDialogState { + data object Idle : ChargeDialogState() data object Loading : ChargeDialogState() data class Error(val message: StringResource) : ChargeDialogState() data class AllChargesV2( val chargeTemplate: ChargeTemplate, val selectedChargeName: String, val selectedChargeId: Int, ) : ChargeDialogState() }And in state default:
- val chargeDialogState: ClientChargesViewModel.ChargeDialogState = ClientChargesViewModel.ChargeDialogState.Loading, + val chargeDialogState: ClientChargesViewModel.ChargeDialogState = ClientChargesViewModel.ChargeDialogState.Idle,Also applies to: 276-285
338-352: Action set overlaps (AddCharge vs OnShowAddCharge). Consolidate to one.Two actions trigger the same UI state (showAddCharges true, showCharges false). This increases surface without benefit.
- Drop OnShowAddCharge and use AddCharge everywhere.
- Or keep one and mark the other internal/private to the UI layer.
Also applies to: 82-121
feature/client/src/desktopMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesScreen.desktop.kt (1)
22-35: Unused onAction parameter on desktop; suppress to keep builds clean.The actual implementation doesn’t use onAction. Suppress the warning locally.
@Composable actual fun ClientChargeContent( pagingFlow: Flow<PagingData<ChargesEntity>>, - onAction: (ClientChargesAction) -> Unit, + onAction: (ClientChargesAction) -> Unit, ) { + @Suppress("UNUSED_PARAMETER") Box( modifier = Modifier.fillMaxSize(), contentAlignment = Alignment.Center, ) { Text( text = "Not available in desktop yet", ) } }feature/client/src/nativeMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesScreen.native.kt (1)
25-26: UnusedonActionparameter in placeholder implementationSuppress or use an underscore name to avoid warnings until native is implemented.
Apply one of:
-actual fun ClientChargeContent( +@Suppress("UNUSED_PARAMETER") +actual fun ClientChargeContent( pagingFlow: Flow<PagingData<ChargesEntity>>, onAction: (ClientChargesAction) -> Unit, )or
- onAction: (ClientChargesAction) -> Unit, + _onAction: (ClientChargesAction) -> Unit,feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesRoute.kt (2)
17-21: Avoid sentinel default for critical route arg
clientIddefaulting to -1 hides mistakes. Prefer enforcing a valid id.-@Serializable -data class ClientChargesRoute( - val clientId: Int = -1, -) +@Serializable +data class ClientChargesRoute( + val clientId: Int, +)
34-42: Guard navigation with argument validationFail fast if an invalid id is passed.
fun NavController.navigateToClientChargesRoute( clientId: Int, ) { - this.navigate( + require(clientId > 0) { "clientId must be > 0" } + navigate( ClientChargesRoute( clientId = clientId, ), ) }Verified: ClientChargesViewModel reads clientId from savedStateHandle.toRoute().clientId.
feature/client/src/commonMain/composeResources/values/strings.xml (1)
99-102: Copy polish: clearer success/failure messagingTighten phrasing and keep UI voice consistent.
- <string name="feature_client_created_charge_success_title">Charge Added Successfully</string> - <string name="feature_client_created_charge_failure_title">Charge Creation Failed</string> - <string name="feature_client_created_charge_success_message">Client Charge created successfully. Please continue to go back to Profile Details</string> + <string name="feature_client_created_charge_success_title">Charge added successfully</string> + <string name="feature_client_created_charge_failure_title">Charge creation failed</string> + <string name="feature_client_created_charge_success_message">Client charge created. Tap Continue to return to Profile Details.</string>feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesScreen.android.kt (2)
102-106: Use generic action strings, not identifier-specific onesReplace identifier-specific strings with generic actions.
-Text( - text = stringResource(Res.string.client_identifier_btn_view), +Text( + text = stringResource(Res.string.action_view),
96-117: Don’t materialize the entire paging list; render with LazyColumn inside the sheetCreating
List(itemCount) { chargesPagingList[index] }defeats paging and can cause jank.- val chargesList = List(chargesPagingList.itemCount){index -> - chargesPagingList[index] - }.filterNotNull() - MifosBottomSheet(onDismiss = { + MifosBottomSheet(onDismiss = { onAction(ClientChargesAction.CloseShowChargesDialog) }, content = { Column( modifier = Modifier .fillMaxWidth() .padding(DesignToken.padding.large), verticalArrangement = Arrangement.spacedBy(DesignToken.padding.largeIncreased), ) { Text( - text = stringResource(Res.string.action_view), + text = stringResource(Res.string.action_view), style = MifosTypography.titleMediumEmphasized, ) - chargesList.forEach { it -> - MifosActionsChargeListingComponent( - chargeTitle = it.name.toString(), - type = it.chargeCalculationType?.value.toString(), - date = it.formattedDueDate, - collectedOn = it.formattedCollectedOnDate ?: it.formattedDueDate, - amount = it.amount.toString(), - onActionClicked = {}, - isExpandable = true - ) - } + LazyColumn(verticalArrangement = Arrangement.spacedBy(DesignToken.padding.medium)) { + items( + count = chargesPagingList.itemCount, + key = { index -> chargesPagingList[index]?.id ?: "index_$index" }, + ) { index -> + chargesPagingList[index]?.let { charge -> + MifosActionsChargeListingComponent( + chargeTitle = charge.name.orEmpty(), + type = charge.chargeCalculationType?.value.orEmpty(), + date = charge.formattedDueDate, + collectedOn = charge.formattedCollectedOnDate ?: charge.formattedDueDate, + amount = charge.amount.toString(), + onActionClicked = {}, + isExpandable = true + ) + } + } + when (chargesPagingList.loadState.append) { + is LoadState.Loading -> item { MifosPagingAppendProgress() } + is LoadState.Error -> item { + MifosSweetError( + message = stringResource(Res.string.feature_client_failed_to_load_more_charges) + ) { onAction(ClientChargesAction.OnRetry) } + } + else -> Unit + } + }feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesScreen.kt (4)
479-499: Amount field validation UX polishShow the trailing error icon only when invalid; you already compute
isAmountValid.- trailingIcon = { - if (!isAmountValid && state.amountTouched) { - Icon(imageVector = MifosIcons.Error, contentDescription = null) - } - }, + trailingIcon = { + if (!isAmountValid && state.amountTouched) { + Icon(imageVector = MifosIcons.Error, contentDescription = null) + } + },And prefer
state.amount.orEmpty()to avoid “null”:- value = state.amount ?: "", + value = state.amount.orEmpty(),
198-206: Count string not localizedCompose the count using resources (or plurals) instead of concatenation.
Add a string like
feature_client_charges_count= "%1$d Charges" and use:- text = state.addedCharges.size.toString() + " " + stringResource(Res.string.feature_client_charges), + text = stringResource(Res.string.feature_client_charges_count, state.addedCharges.size),
164-167: Avoid!!on error messageRender safely to prevent crashes if
erroris null.- MifosSweetError( - message = state.error!!, - ) + state.error?.let { MifosSweetError(message = it) }
219-231: Type-safety forchargesFlow— fix the unsafe cast
ClientChargesState.chargesFlowis currently typed asAny?, causing the unsafe cast inClientChargesScreen.kt. The review's suggestion is valid.
- Change
chargesFlow: Any? = nulltochargesFlow: Flow<PagingData<ChargesEntity>>? = nullinClientChargesState(ClientChargesViewModel.kt:290)- Simplify the
whenpattern inClientChargesScreen.ktto use a null-safe check insteadThis aligns with the proper typing already used in
ClientUpcomingChargesViewmodel.kt:130and eliminates the runtime type-checking and casting risks.feature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt (1)
478-481: Deprecate old navigation API during type-safe migrationThe old
navigateClientChargesScreenfunction is unused internally while the new type-safenavigateToClientChargesRouteis actively used elsewhere (3 call sites). The suggested deprecation with replacement is appropriate for this migration.-fun NavController.navigateClientChargesScreen(clientId: Int) { - navigate(ClientScreens.ClientChargesScreen.argument(clientId)) -} +@Deprecated( + message = "Use typed route", + replaceWith = ReplaceWith("navigateToClientChargesRoute(clientId)"), +) +fun NavController.navigateClientChargesScreen(clientId: Int) = + navigateToClientChargesRoute(clientId)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesScreen.android.kt(2 hunks)feature/client/src/commonMain/composeResources/values/strings.xml(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/ClientChargesRoute.kt(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesScreen.kt(2 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesViewModel.kt(2 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/navigation/ClientNavigation.kt(6 hunks)feature/client/src/desktopMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesScreen.desktop.kt(1 hunks)feature/client/src/nativeMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesScreen.native.kt(1 hunks)
💤 Files with no reviewable changes (2)
- feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientChargeDialog/ChargeDialogUiState.kt
- feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientChargeDialog/ChargeDialogScreen.kt
🧰 Additional context used
🪛 GitHub Actions: PR Checks
feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesScreen.android.kt
[error] 1-1: Spotless Kotlin formatting violations detected. The build failed during :feature:client:spotlessKotlinCheck due to multiple formatting issues in ClientChargesScreen.android.kt (and other client charges-related files). Run './gradlew :feature:client:spotlessApply' to auto-fix formatting, or fix the violations manually. See log for specific import formatting and whitespace issues.
🔇 Additional comments (6)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDetailsProfile/ClientProfileDetailsNavigation.kt (1)
22-35: No changes needed. The only call site already passes the parameter.The review assumes existing call sites may not pass
navigateToAddCharge, but verification shows the only invocation site in the codebase (ClientNavigation.kt:248) already passes all parameters includingnavigateToAddCharge = navController::navigateToClientChargesRoute(line 260). Therefore, the concern about downstream compile breaks does not apply.Likely an incorrect or invalid review comment.
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientProfile/ClientProfileNavigation.kt (1)
33-45: The review comment's premise is incorrect; the breaking change concern is not supported.The single call site at
ClientNavigation.kt:175already suppliesnavigateToAddCharge = navController::navigateToClientChargesRoute. No source-breaking change exists, and no call sites are missing the argument.However, a separate concern remains:
navigateToAddChargeis accepted byclientProfileDestinationbut cannot be forwarded toClientProfileScreen(which does not have this parameter in its signature atClientProfileScreen.kt:51). The parameter is unused in the current flow.To resolve this properly, either:
- Add
navigateToAddChargetoClientProfileScreen's parameters and forward it, or- Remove
navigateToAddChargefromclientProfileDestinationif it is not needed.Likely an incorrect or invalid review comment.
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDetailsProfile/ClientProfileDetailsScreen.kt (1)
89-104: Navigation wiring verified: navigateToAddCharge is correctly supplied to the screen.The nav graph properly passes
navigateToAddChargeas a parameter toClientProfileDetailsScreenand the callback is invoked correctly with a safe fallback (state.client?.id?:-1). Code changes are good to proceed.Optionally, consider adding a guard to skip navigation when
id == -1to prevent dead routes, though this is not blocking.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesViewModel.kt (1)
31-40: No changes needed - androidx.navigation.toRoute is correctly configured for multiplatform.The codebase is already using the correct multiplatform dependency:
org.jetbrains.androidx.navigation:navigation-compose(defined asjb-composeNavigationin the version catalog). This is provided through thecore.uimodule tofeature/clientincommonMain, ensuringtoRoute<>works across all targets (Android, iOS, Desktop, Web).feature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt (1)
130-131: Good move to typed navigation for Client ChargesUsing
navigateToClientChargesRouteandclientChargesDestinationimproves type-safety.Also applies to: 146-149
feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesScreen.android.kt (1)
12-26: Verify Spotless violations by running checks locallyThe review comment lacks specific details about formatting violations. The code shown appears properly formatted: copyright header is present, imports are organized logically (androidclient resources → androidx → com.mifos), and follows the project's ktlint (v1.0.1) rules configured in
.editorconfig.Manually verify this by running:
./gradlew :feature:client:spotlessCheckIf violations exist, apply fixes with:
./gradlew :feature:client:spotlessApplyThe sandbox environment prevents automated verification, so you must confirm the actual Spotless status locally before merging.
| MifosActionsChargeListingComponent( | ||
| chargeTitle = it.name.toString(), | ||
| type = it.chargeCalculationType?.value.toString(), | ||
| date = it.formattedDueDate, | ||
| collectedOn = it.formattedDueDate, | ||
| amount = it.amount.toString(), | ||
| // menuList = , | ||
| onActionClicked = {}, |
There was a problem hiding this comment.
🧩 Analysis chain
Wrong “collectedOn” value
Using formattedDueDate for collectedOn is misleading.
If ChargesEntity exposes a collected-on field, bind it; otherwise omit or clearly label as due date.
MifosActionsChargeListingComponent(
chargeTitle = it.name.toString(),
type = it.chargeCalculationType?.value.toString(),
date = it.formattedDueDate,
- collectedOn = it.formattedDueDate,
+ collectedOn = it.formattedCollectedOnDate ?: it.formattedDueDate,
amount = it.amount.toString(),Check available fields on ChargesEntity:
🏁 Script executed:
#!/bin/bash
rg -nP -C2 'data\s+class\s+ChargesEntity\b' && rg -nP -C2 'formattedCollectedOn|collectedOn'Length of output: 19343
Let me retrieve the complete ChargesEntity class definition directly.
---
🏁 Script executed:
```shell
#!/bin/bash
cat core/database/src/commonMain/kotlin/com/mifos/room/entities/client/ChargesEntity.kt
Length of output: 1803
Now let me verify the ChargeTimeTypeEntity structure to confirm the correct property path.
---
🏁 Script executed:
```shell
#!/bin/bash
rg -nP 'data\s+class\s+ChargeTimeTypeEntity|sealed\s+class\s+ChargeTimeTypeEntity' -A5 --type=kotlin
Length of output: 690
---
🏁 Script executed:
```shell
#!/bin/bash
cat core/database/src/commonMain/kotlin/com/mifos/room/entities/client/ChargeTimeTypeEntity.kt
Length of output: 936
Use chargeTimeType?.value for collectedOn, not formattedDueDate
The issue is valid: formattedDueDate is a due date, not a collected-on timestamp. However, the suggested fix referencing formattedCollectedOnDate is incorrect—this field doesn't exist in ChargesEntity.
The correct field to use is chargeTimeType?.value, which represents the charge collection time type and matches the pattern used in other screens (NewLoanAccountScreen.kt:467).
MifosActionsChargeListingComponent(
chargeTitle = it.name.toString(),
type = it.chargeCalculationType?.value.toString(),
date = it.formattedDueDate,
- collectedOn = it.formattedDueDate,
+ collectedOn = it.chargeTimeType?.value.toString(),
amount = it.amount.toString(),📝 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.
| MifosActionsChargeListingComponent( | |
| chargeTitle = it.name.toString(), | |
| type = it.chargeCalculationType?.value.toString(), | |
| date = it.formattedDueDate, | |
| collectedOn = it.formattedDueDate, | |
| amount = it.amount.toString(), | |
| // menuList = , | |
| onActionClicked = {}, | |
| MifosActionsChargeListingComponent( | |
| chargeTitle = it.name.toString(), | |
| type = it.chargeCalculationType?.value.toString(), | |
| date = it.formattedDueDate, | |
| collectedOn = it.chargeTimeType?.value.toString(), | |
| amount = it.amount.toString(), | |
| // menuList = , | |
| onActionClicked = {}, |
🤖 Prompt for AI Agents
In
feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesScreen.android.kt
around lines 107 to 114, the collectedOn prop is incorrectly set to
formattedDueDate; change it to use the charge collection type by passing
chargeTimeType?.value (matching the pattern used elsewhere) — ensure you convert
it to a safe string (e.g., using toString() or a null-safe fallback) so
collectedOn reflects the chargeTimeType value rather than the due date.
| firstBtnText = stringResource(Res.string.btn_back), | ||
| secondBtnText = stringResource(Res.string.client_identifier_btn_add), | ||
| onFirstBtnClick = { | ||
|
|
||
| is LoadState.Loading -> { | ||
| item { | ||
| MifosPagingAppendProgress() | ||
| } | ||
| } | ||
|
|
||
| is LoadState.NotLoading -> Unit | ||
| } | ||
|
|
||
| if ( | ||
| chargesPagingList.loadState.append is LoadState.NotLoading && | ||
| chargesPagingList.loadState.append.endOfPaginationReached && | ||
| chargesPagingList.itemCount > 0 | ||
| ) { | ||
| item { | ||
| Text( | ||
| modifier = Modifier | ||
| .fillMaxWidth() | ||
| .padding(6.dp), | ||
| text = stringResource(Res.string.feature_client_no_more_charges_available), | ||
| style = MaterialTheme.typography.bodyMedium, | ||
| textAlign = TextAlign.Center, | ||
| }, | ||
| onSecondBtnClick = { | ||
| // onAction(NewLoanAccountAction.ShowAddChargeDialog) | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Wire buttons: Back should close sheet; Add should open Add Charge
Currently both callbacks are no-ops.
MifosTwoButtonRow(
- firstBtnText = stringResource(Res.string.btn_back),
- secondBtnText = stringResource(Res.string.client_identifier_btn_add),
- onFirstBtnClick = {
-
- },
- onSecondBtnClick = {
-// onAction(NewLoanAccountAction.ShowAddChargeDialog)
- },
+ firstBtnText = stringResource(Res.string.btn_back),
+ secondBtnText = stringResource(Res.string.action_add),
+ onFirstBtnClick = { onAction(ClientChargesAction.CloseShowChargesDialog) },
+ onSecondBtnClick = { onAction(ClientChargesAction.OnShowAddCharge) },
)Also remove the unused NewLoanAccountAction import.
| <string name="feature_client_choose_charge">Choose Charge</string> | ||
| <string name="feature_client_charge_title">Charge Title</string> | ||
| <string name="feature_client_charge_name">Charge Name</string> | ||
| <string name="feature_client_charge_amount">Amount</string> | ||
| <string name="feature_client_due_date">Due Date</string> | ||
| <string name="feature_client_collected_on_date">Collected On</string> | ||
| <string name="feature_client_charge_invalid_amount_format">Invalid amount format</string> |
There was a problem hiding this comment.
🧩 Analysis chain
Terminology consistency: “Collected on” vs labels
Ensure the label used in UI matches this key (“Collected On”). Cross-check usages where the collected-on field uses the due-date label.
🏁 Script executed:
#!/bin/bash
rg -nP 'feature_client_collected_on_date|feature_client_due_date' --type=kt -C2Length of output: 26
I need to fix the search command since --type=kt is not recognized. Let me search across the codebase without the type restriction to find usages of these string keys.
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for usages of the two string resource keys across the codebase
rg -n 'feature_client_collected_on_date|feature_client_due_date' -C3
Length of output: 6893
Fix terminology inconsistency: collected-on field incorrectly uses due-date label
Line 505 in ClientChargesScreen.kt has a critical bug. The collected-on date picker field's label uses feature_client_due_date ("Due Date") instead of feature_client_collected_on_date ("Collected On"):
label = if (state.collectedOn == null) "" else stringResource(Res.string.feature_client_due_date), // Wrong!Should be:
label = if (state.collectedOn == null) "" else stringResource(Res.string.feature_client_collected_on_date),🤖 Prompt for AI Agents
In feature/client/src/commonMain/composeResources/values/strings.xml (lines
63–69) and ClientChargesScreen.kt (around line 505), the collected-on date
picker uses the wrong label resource (feature_client_due_date) causing the field
to display "Due Date" instead of "Collected On"; change the label reference in
ClientChargesScreen.kt at line ~505 to use
Res.string.feature_client_collected_on_date when state.collectedOn is non-null
so the picker shows the correct "Collected On" label.
| value = state.chargeName.toString(), | ||
| onValueChanged = { onAction(ClientChargesAction.OnChargeNameChange(it)) }, | ||
| label = stringResource(Res.string.feature_client_choose_charge), | ||
| readOnly = true, | ||
| onOptionSelected = { index, value -> | ||
| onAction( | ||
| ClientChargesAction.OnChargeIdChange( | ||
| chargeTemplate.chargeOptions[index].id ?: -1 | ||
| ) | ||
| ) | ||
| onAction(ClientChargesAction.OnChargeNameChange(value)) | ||
| onAction(ClientChargesAction.OnChargeNameTouched) | ||
| onAction(ClientChargesAction.OnShowAddCharge) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid toString() on nullable state for text fields
toString() renders “null”. Prefer orEmpty() and explicit null handling.
- value = state.chargeName.toString(),
+ value = state.chargeName.orEmpty(),
@@
- onAction(ClientChargesAction.OnChargeNameChange(value))
+ onAction(ClientChargesAction.OnChargeNameChange(value ?: ""))Also update validation:
- val isChargeNameValid = validateChargeName(state.chargeName.toString())
+ val isChargeNameValid = validateChargeName(state.chargeName.orEmpty())Committable suggestion skipped: line range outside the PR's diff.
...e/client/src/commonMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesScreen.kt
Outdated
Show resolved
Hide resolved
...e/client/src/commonMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesScreen.kt
Outdated
Show resolved
Hide resolved
...lient/src/commonMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesViewModel.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesViewModel.kt (1)
167-173: Refresh flag clears before data finishes loading.
loadCharges()launches its own coroutine (line 141), sorefreshChargesList()setsisRefreshing = falseimmediately without awaiting completion. ConvertloadChargesto a suspend function and await it.Apply this diff:
- private fun loadCharges() { - viewModelScope.launch { + private suspend fun loadCharges() { mutableStateFlow.update { it.copy(isLoading = true, error = null) } try { val response = mutableStateFlow.value.clientId?.let { repository.getClientCharges(it) } - val count = mutableStateFlow.value.clientId?.let { repository.getClientChargesTotal(it) }!! + val count = mutableStateFlow.value.clientId?.let { repository.getClientChargesTotal(it) } ?: 0 mutableStateFlow.update { it.copy( isLoading = false, chargesFlow = response, totalClientCharges = count, error = null, ) } } catch (e: Exception) { mutableStateFlow.update { it.copy( isLoading = false, error = e.message ?: "Failed to load charges", chargesFlow = null, ) } } - } } - private fun refreshChargesList() { - viewModelScope.launch { + private fun refreshChargesList() = viewModelScope.launch { mutableStateFlow.update { it.copy(isRefreshing = true) } loadCharges() mutableStateFlow.update { it.copy(isRefreshing = false) } - } }Also update callers:
init { - loadCharges() + viewModelScope.launch { loadCharges() } } override fun handleAction(action: ClientChargesAction) { when (action) { ClientChargesAction.NavigateBack -> sendEvent(ClientChargesEvent.NavigateBack) ClientChargesAction.Refresh -> refreshChargesList() ClientChargesAction.LoadChargeTemplate -> loadChargeTemplate() - ClientChargesAction.OnRetry -> loadCharges() + ClientChargesAction.OnRetry -> viewModelScope.launch { loadCharges() }
🧹 Nitpick comments (4)
feature/client/build.gradle.kts (1)
44-44: Potential duplicate dependency declaration.The
androidx.paging.composedependency is declared in bothcommonMain(line 44) andandroidMain(line 58). SinceandroidMaininherits dependencies fromcommonMainin Kotlin Multiplatform projects, this duplication may be unnecessary.Consider removing the declaration from
androidMainunless you specifically need different versions or configurations for Android:androidMain.dependencies { implementation(libs.maps.compose) implementation(libs.accompanist.permission) implementation(libs.ktor.client.android) - - implementation(libs.androidx.paging.compose) implementation(libs.coil.network.okhttp) implementation(libs.play.services.location) implementation(libs.kotlinx.coroutines.play.services) }Also applies to: 58-58
feature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt (2)
30-31: LGTM! Clean migration to destination-based navigation.The refactor from direct
ClientChargesScreenusage to the destination pattern (clientChargesDestinationandnavigateToClientChargesRoute) is consistent and well-integrated throughout the navigation graph.Consider removing the commented-out legacy navigation code (lines 404-415) in a follow-up cleanup to reduce clutter.
Also applies to: 130-130, 146-149, 186-186, 260-260
404-415: Consider removing commented legacy code.The old
clientChargesRouteimplementation is now fully replaced by the destination-based approach. Remove this commented block in a follow-up cleanup.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesScreen.kt (1)
259-259: Use.orEmpty()instead of.toString()on nullable strings.Using
.toString()on nullable strings renders"null"as a literal string. While validation currently handles this, it's clearer to use.orEmpty().Apply this diff:
- val amountValidation = validateAmount(state.amount.toString(), currencyDecimalPlaces) + val amountValidation = validateAmount(state.amount.orEmpty(), currencyDecimalPlaces) - val isChargeNameValid = validateChargeName(state.chargeName.toString()) + val isChargeNameValid = validateChargeName(state.chargeName.orEmpty()) MifosTextFieldDropdown( - value = state.chargeName.toString(), + value = state.chargeName.orEmpty(),Also applies to: 300-300, 318-318
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
cmp-android/dependencies/demoDebugRuntimeClasspath.txt(5 hunks)cmp-android/dependencies/demoReleaseRuntimeClasspath.txt(5 hunks)cmp-android/dependencies/prodDebugRuntimeClasspath.txt(5 hunks)cmp-android/dependencies/prodReleaseRuntimeClasspath.txt(5 hunks)core/data/src/commonMain/kotlin/com/mifos/core/data/repository/ClientChargeRepository.kt(1 hunks)core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/ClientChargeRepositoryImp.kt(2 hunks)feature/client/build.gradle.kts(1 hunks)feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesScreen.android.kt(0 hunks)feature/client/src/commonMain/composeResources/values/strings.xml(2 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesRoute.kt(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesScreen.kt(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesViewModel.kt(2 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/navigation/ClientNavigation.kt(6 hunks)feature/client/src/desktopMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesScreen.desktop.kt(0 hunks)feature/client/src/nativeMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesScreen.native.kt(0 hunks)gradle/libs.versions.toml(1 hunks)
💤 Files with no reviewable changes (3)
- feature/client/src/desktopMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesScreen.desktop.kt
- feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesScreen.android.kt
- feature/client/src/nativeMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesScreen.native.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDetailsProfile/ClientProfileDetailsScreen.kt
- feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesRoute.kt
🧰 Additional context used
🧬 Code graph analysis (2)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesRoute.kt (1)
clientChargesDestination(22-32)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesScreen.kt (14)
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/MifosProgressIndicator.kt (1)
MifosProgressIndicator(41-68)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/MifosErrorComponent.kt (1)
MifosErrorComponent(41-60)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/MifosStatusDialog.kt (1)
MifosStatusDialog(36-103)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/MifosTwoButtonRow.kt (1)
MifosTwoButtonRow(31-91)core/common/src/commonMain/kotlin/com/mifos/core/common/utils/FormatDate.kt (1)
formatDate(17-21)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/MifosEmptyUi.kt (1)
MifosEmptyUi(32-66)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosActionsListingCardComponent.kt (1)
MifosActionsChargeListingComponent(598-728)
🔇 Additional comments (8)
gradle/libs.versions.toml (1)
79-79: Verify the need for alpha version in production.The paging library has been updated to
3.4.0-alpha04, which is a pre-release alpha version. Alpha releases may contain bugs, breaking changes, or unstable APIs.Are there specific features in this alpha release that are required for this PR? If not, consider using the latest stable version (3.3.x series) to avoid potential stability issues in production.
cmp-android/dependencies/demoReleaseRuntimeClasspath.txt (1)
15-60: Multiple alpha/pre-release versions in runtime classpath.This runtime classpath includes several alpha and pre-release versions:
- Compose 1.10.0-alpha03 (animation, foundation, runtime, ui modules)
- Paging 3.4.0-alpha04
- Core 1.16.0
While alpha versions provide access to new features, they may introduce stability issues or breaking changes in production.
Ensure that the team has tested these alpha versions thoroughly and that any stability risks are acceptable for this release. Consider whether the features from these alpha versions are critical, or if stable alternatives are sufficient.
Also applies to: 63-66, 77-77, 85-106, 108-108, 117-120, 129-133, 152-152
core/data/src/commonMain/kotlin/com/mifos/core/data/repository/ClientChargeRepository.kt (1)
23-23: Clean API extension.The new
getClientChargesTotalfunction is a well-designed addition to the repository interface. The function signature is clear, follows Kotlin coroutine conventions, and provides a useful capability for retrieving charge totals.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDetailsProfile/ClientProfileDetailsNavigation.kt (1)
34-34: LGTM! Navigation callback properly integrated.The
navigateToAddChargecallback follows the established pattern for client profile navigation and is correctly forwarded toClientProfileDetailsScreen.Also applies to: 49-49
feature/client/src/commonMain/composeResources/values/strings.xml (1)
63-69: LGTM! String resources properly added.The new charge-related string resources support the expanded charges UI flow. Note: The past review comment regarding
collected_on_datelabel inconsistency appears to have been resolved, as the current implementation uses onlydueDatein the state.Also applies to: 99-103
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesViewModel.kt (1)
33-42: LGTM! Clean BaseViewModel architecture.The refactor to
BaseViewModel<ClientChargesState, ClientChargesEvent, ClientChargesAction>with proper state management, sealed interfaces for events/actions, and route-based initialization is well-structured and aligns with modern Compose/KMP patterns.Also applies to: 48-138, 271-327
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesScreen.kt (2)
535-560: LGTM! Robust validation logic.The validation functions properly handle:
- Blank/empty checks
- Null date validation
- Amount format validation with configurable decimal places
- Regex patterns that prevent leading zeros and enforce decimal precision
The
AmountValidationResultenum provides clear validation states for UI feedback.
109-136: LGTM! Well-structured UI composition.The screen refactor introduces solid patterns:
- Event-driven navigation with
EventsEffect- Pull-to-refresh integration
- State-driven dialogs and bottom sheets
- Date picker with future-only date selection
- Paging integration with proper
LoadStatehandling- Form validation tied to submit button state
The multi-layered composition (Screen → Scaffold → Content → Dialogs) provides good separation of concerns.
Also applies to: 139-218, 251-459, 462-533
| override suspend fun getClientChargesTotal(clientId: Int): Int { | ||
| return dataManagerCharge.getClientCharges( | ||
| clientId = clientId, | ||
| offset = 0, | ||
| limit = 1, | ||
| ).first().totalFilteredRecords | ||
| } |
There was a problem hiding this comment.
Handle potential empty flow to avoid exceptions.
The implementation uses .first() which will throw NoSuchElementException if the flow emits no values. This could occur if the API call fails or returns an empty response.
Consider adding error handling:
override suspend fun getClientChargesTotal(clientId: Int): Int {
- return dataManagerCharge.getClientCharges(
+ return dataManagerCharge.getClientCharges(
clientId = clientId,
offset = 0,
limit = 1,
- ).first().totalFilteredRecords
+ ).firstOrNull()?.totalFilteredRecords ?: 0
}Alternatively, if the API should always return a value, document this assumption and ensure proper error handling upstream.
📝 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.
| override suspend fun getClientChargesTotal(clientId: Int): Int { | |
| return dataManagerCharge.getClientCharges( | |
| clientId = clientId, | |
| offset = 0, | |
| limit = 1, | |
| ).first().totalFilteredRecords | |
| } | |
| override suspend fun getClientChargesTotal(clientId: Int): Int { | |
| return dataManagerCharge.getClientCharges( | |
| clientId = clientId, | |
| offset = 0, | |
| limit = 1, | |
| ).firstOrNull()?.totalFilteredRecords ?: 0 | |
| } |
🤖 Prompt for AI Agents
In
core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/ClientChargeRepositoryImp.kt
around lines 42 to 48, the code calls .first() on a Flow which can throw
NoSuchElementException if the flow emits no values; change the implementation to
safely handle empty flows by using firstOrNull() and then either return a safe
default (e.g. 0) or throw a clear, documented exception, or wrap the call in a
try/catch that maps NoSuchElementException to a domain-specific error; ensure
you log or propagate meaningful context (clientId) so callers can handle the
missing data; update KDoc if you decide the API always returns a value to
document that invariant.
|
|
||
| try { | ||
| val response = mutableStateFlow.value.clientId?.let { repository.getClientCharges(it) } | ||
| val count = mutableStateFlow.value.clientId?.let { repository.getClientChargesTotal(it) }!! |
There was a problem hiding this comment.
Remove dangerous !! operator to prevent NPE.
Line 146 uses !! after a let call, which will throw a NullPointerException if clientId is null or if getClientChargesTotal returns null. Use safe handling instead.
Apply this diff:
- val count = mutableStateFlow.value.clientId?.let { repository.getClientChargesTotal(it) }!!
+ val count = mutableStateFlow.value.clientId?.let { repository.getClientChargesTotal(it) } ?: 0📝 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 count = mutableStateFlow.value.clientId?.let { repository.getClientChargesTotal(it) }!! | |
| val count = mutableStateFlow.value.clientId?.let { repository.getClientChargesTotal(it) } ?: 0 |
🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientCharges/ClientChargesViewModel.kt
around line 146, the expression uses the unsafe !! operator on the result of a
let call which can throw an NPE if clientId or getClientChargesTotal(...) is
null; replace this with safe handling — either use a safe-call chain and a
default (e.g. val count = mutableStateFlow.value.clientId?.let {
repository.getClientChargesTotal(it) } ?: 0) or perform an early
return/null-check (e.g. val clientId = mutableStateFlow.value.clientId ?:
return; val count = repository.getClientChargesTotal(clientId) ?: 0) so you
avoid !! and handle nulls deterministically.
|
@Shreyash16b It is better to add a comment while closing a PR as shown below: Closed in reference to #2533 |
3 participants
Fixes - Client Add Charge
MIFOSAC-566.3.mp4
ClientChargesViewModelto inherit fromBaseViewModelinstead of the plainViewModelto align with the rest of the architecture.MifosBottomSheetto view chargesRemaining tasks:
Charge functions like edit/delete
Run the static analysis check
./gradlew checkorci-prepush.shto make sure you didn't break anythingIf you have multiple commits please combine them into one commit by squashing them.
Summary by CodeRabbit