Feature Recurring Deposit Account Charges Step #2548
Conversation
WalkthroughThis PR adds charge management functionality to the recurring deposit feature by introducing state-driven UI dialogs, action handlers, new string resources, and validators for adding, editing, and deleting charges within the account creation workflow. Changes
Sequence DiagramsequenceDiagram participant User participant RecurringAccountScreen participant RecurringAccountViewModel participant DialogState User->>RecurringAccountScreen: Opens Charges section RecurringAccountScreen->>RecurringAccountViewModel: Route ShowAddChargeDialog action RecurringAccountViewModel->>DialogState: Update to AddNewCharge state DialogState-->>RecurringAccountScreen: Render AddNewChargeDialog User->>AddNewChargeDialog: Enter charge details (date, amount, type) AddNewChargeDialog->>RecurringAccountViewModel: Dispatch OnChargesAmountChange / OnChargesDateChange RecurringAccountViewModel->>RecurringAccountViewModel: Validate inputs alt Validation Success User->>AddNewChargeDialog: Confirm charge AddNewChargeDialog->>RecurringAccountViewModel: Dispatch AddChargeToList action RecurringAccountViewModel->>RecurringAccountViewModel: Append to addedCharges list RecurringAccountViewModel->>DialogState: Update to SuccessResponseStatus DialogState-->>RecurringAccountScreen: Show success snackbar + dismiss dialog else Validation Error RecurringAccountViewModel->>DialogState: Set chargeAmountError DialogState-->>AddNewChargeDialog: Display error feedback end User->>RecurringAccountScreen: Submit charges RecurringAccountScreen->>RecurringAccountViewModel: Dispatch Finish action RecurringAccountViewModel-->>User: Navigate forward Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 9
🧹 Nitpick comments (7)
core/ui/src/commonMain/composeResources/values/strings.xml (1)
121-125: Remove unnecessary trailing blank lines.These blank lines before the closing
</resources>tag add no value and should be removed to keep the file clean.<string name="client_share_accounts_approved_shares">Approved Shares</string> - - - - - </resources>feature/recurringDeposit/src/commonMain/composeResources/values/feature_recurring_deposit_string.xml (1)
52-58: Inconsistent naming convention for new string resources.The new strings use
recurring_step_charges_*prefix, while all existing strings in this file follow thefeature_recurring_deposit_*prefix pattern. Consider renaming for consistency:- <string name="recurring_step_charges_edit_charge">Edit Charge</string> - <string name="recurring_step_charges_add_new">Add New</string> - <string name="recurring_step_charges_add">Add</string> - <string name="recurring_step_charges_view">View</string> - <string name="recurring_step_charges_active">Active</string> - <string name="recurring_step_charges_submit">Submit</string> + <string name="feature_recurring_deposit_charges_edit">Edit Charge</string> + <string name="feature_recurring_deposit_charges_add_new">Add New</string> + <string name="feature_recurring_deposit_charges_add">Add</string> + <string name="feature_recurring_deposit_charges_view">View</string> + <string name="feature_recurring_deposit_charges_active">Active</string> + <string name="feature_recurring_deposit_charges_submit">Submit</string>feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/ChargesPage.kt (1)
101-103: String concatenation may cause i18n issues.Concatenating strings with spaces in composables doesn't translate well across languages with different word orders. Consider using a parameterized string resource instead.
Example string resource:
<string name="feature_recurring_deposit_active_charges">%1$d Active Charges</string>Then use:
text = stringResource(Res.string.feature_recurring_deposit_active_charges, state.addedCharges.size),feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountScreen.kt (3)
220-220: Remove duplicate@OptInannotation.
ExperimentalTime::classis listed twice.-@OptIn(ExperimentalTime::class, ExperimentalTime::class) +@OptIn(ExperimentalTime::class)
240-242: Consider extractingisSelectableDateto avoid duplication.This function is duplicated from
DetailsPage.kt(as noted in the relevant code snippets). Extract it to a shared utility to maintain DRY principle.// In a shared utility file, e.g., DateUtils.kt @OptIn(ExperimentalTime::class) fun isSelectableDate(utcTimeMillis: Long): Boolean { return utcTimeMillis >= Clock.System.now().toEpochMilliseconds().minus(86_400_000L) }
359-362: Expansion state logic usesit.idbut initializes to-1.
expandedIndexis initialized to-1and compared againstit.id. If charge IDs are auto-generated (e.g., starting from 0 or 1), this works, but if any charge hasid = -1, it would be expanded by default. Consider usingnullas the initial state for clarity.- var expandedIndex: Int? by rememberSaveable { mutableStateOf(-1) } + var expandedIndex: Int? by rememberSaveable { mutableStateOf(null) } ... - isExpanded = expandedIndex == it.id, + isExpanded = expandedIndex == it.id, onExpandToggle = { - expandedIndex = if (expandedIndex == it.id) -1 else it.id + expandedIndex = if (expandedIndex == it.id) null else it.id },feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt (1)
774-781: Review the placeholder RecurringAccountChargesState data class.This data class appears incomplete:
- It only contains one field (
chooseChargeIndex) which is already present inRecurringAccountState- The
@OptIn(ExperimentalTime::class)annotation is unnecessary as no time APIs are used- Multiple empty lines suggest this is a placeholder
Consider either completing this data class with the intended charge-related fields, or removing it if
RecurringAccountStatealready contains all necessary charge management fields.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
core/ui/src/commonMain/composeResources/values/strings.xml(1 hunks)core/ui/src/commonMain/kotlin/com/mifos/core/ui/util/TextFieldsValidator.kt(1 hunks)feature/recurringDeposit/src/commonMain/composeResources/values/feature_recurring_deposit_string.xml(1 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountScreen.kt(4 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt(8 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/ChargesPage.kt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/ChargesPage.kt (2)
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/MifosTwoButtonRow.kt (1)
MifosTwoButtonRow(31-91)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountScreen.kt (6)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/util/TextFieldsValidator.kt (1)
doubleNumberValidator(47-57)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/AddChargeDialog.kt (1)
AddChargeBottomSheet(47-169)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/DetailsPage.kt (1)
isSelectableDate(60-62)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(601-729)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosTwoButtonRow.kt (1)
MifosTwoButtonRow(31-91)
🔇 Additional comments (7)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/util/TextFieldsValidator.kt (1)
47-57: LGTM! Good refactor to normalize input before validation.Trimming whitespace before validation is a sensible improvement for handling user input. The logic correctly validates the trimmed value throughout.
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/ChargesPage.kt (1)
54-119: LGTM! State-driven refactor improves maintainability.The migration from a simple callback to a state-driven pattern with
RecurringAccountStateandonActionis well-structured. The UI properly reflects state and dispatches actions for user interactions.feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountScreen.kt (1)
185-218: LGTM! Dialog state machine is well-structured.The
NewRecurringAccountDialogproperly handles different dialog states using awhenexpression, delegating to appropriate dialog composables. The success state with delayed finish is a reasonable UX pattern.feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt (4)
21-21: LGTM!The new imports for charge management (DateHelper, StringResource, Clock, ExperimentalTime) are appropriate for the functionality being added.
Also applies to: 32-35
872-886: Verify the inheritance pattern is intentional.All action classes inside
RecurringAccountChargesActionextendRecurringAccountAction()directly rather thanRecurringAccountChargesAction(). While this works, it's an unusual pattern since the nested classes don't actually inherit from their enclosing sealed class.Verify this is intentional. If these actions should inherit from the parent sealed class, apply this pattern:
- sealed class RecurringAccountChargesAction : RecurringAccountAction(){ - object ShowAddChargeDialog : RecurringAccountAction() - object ShowCharge : RecurringAccountAction() - data class EditCharge (val index : Int ) : RecurringAccountAction() + sealed class RecurringAccountChargesAction : RecurringAccountAction(){ + object ShowAddChargeDialog : RecurringAccountChargesAction() + object ShowCharge : RecurringAccountChargesAction() + data class EditCharge (val index : Int ) : RecurringAccountChargesAction() // ... (apply to all nested action classes)Otherwise, if all actions must directly extend
RecurringAccountActionfor type system reasons, consider adding a comment explaining this design decision.
718-734: LGTM!The action handling for charges is well-structured, with each action properly delegated to its corresponding handler method.
931-938: LGTM!The
CreatedChargesdata class is well-defined with appropriate nullable types and sensible defaults.
| val snackbarHostState = remember { SnackbarHostState() } | ||
| NewRecurringAccountDialog( | ||
| state = state, | ||
| onAction = {viewModel.trySendAction(it)}, | ||
| snackbarHostState = snackbarHostState | ||
| | ||
| ) |
There was a problem hiding this comment.
snackbarHostState is not connected to a SnackbarHost - snackbars won't display.
The SnackbarHostState is created and passed to NewRecurringAccountDialog, but it's never passed to a MifosScaffold or SnackbarHost. The snackbar messages in SuccessResponseStatus (lines 206-208) will not be visible to users.
Either pass snackbarHostState to RecurringAccountScaffold to be used in MifosScaffold, or use a different mechanism for displaying success feedback.
🤖 Prompt for AI Agents
In feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountScreen.kt around lines 98-104, the created snackbarHostState is passed into NewRecurringAccountDialog but not wired to any Scaffold/SnackbarHost so snackbars will never appear; update the call site to pass snackbarHostState into RecurringAccountScaffold (or directly into the MifosScaffold) and ensure that scaffold uses it via SnackbarHost(hostState = snackbarHostState) (or the scaffold's snackbarHost parameter) so SuccessResponseStatus snackbars (lines ~206-208) are displayed to the user. | is RecurringAccountState.DialogState.showCharges -> ShowChargesDialog( | ||
| state = state, | ||
| onAction = onAction, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Class name showCharges violates Kotlin naming convention.
Sealed class/interface members should use PascalCase. Rename showCharges to ShowCharges for consistency with AddNewCharge and SuccessResponseStatus.
- is RecurringAccountState.DialogState.showCharges -> ShowChargesDialog( + is RecurringAccountState.DialogState.ShowCharges -> ShowChargesDialog(This also requires updating the sealed class definition in the state file.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountScreen.kt around lines 199–202 the sealed-class member name `showCharges` violates Kotlin PascalCase naming; rename the member to `ShowCharges` (both usages and constructor/calls) and update the sealed class definition in the corresponding state file to use `ShowCharges` as well so all references compile and match the existing PascalCase pattern like `AddNewCharge` and `SuccessResponseStatus`. | chargeType = if (state.chooseChargeIndex == -1) { | ||
| "" | ||
| } else { | ||
| state.template.chargeOptions?.get(state.chooseChargeIndex)?.chargeCalculationType?.value | ||
| ?: "" | ||
| }, |
There was a problem hiding this comment.
Potential IndexOutOfBoundsException - use getOrNull consistently.
Line 266 uses .get(state.chooseChargeIndex) which throws if the index is out of bounds, while lines 259 and 272 correctly use .getOrNull(). Use getOrNull consistently to avoid crashes.
chargeType = if (state.chooseChargeIndex == -1) { "" } else { - state.template.chargeOptions?.get(state.chooseChargeIndex)?.chargeCalculationType?.value + state.template.chargeOptions?.getOrNull(state.chooseChargeIndex)?.chargeCalculationType?.value ?: "" },📝 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.
| chargeType = if (state.chooseChargeIndex == -1) { | |
| "" | |
| } else { | |
| state.template.chargeOptions?.get(state.chooseChargeIndex)?.chargeCalculationType?.value | |
| ?: "" | |
| }, | |
| chargeType = if (state.chooseChargeIndex == -1) { | |
| "" | |
| } else { | |
| state.template.chargeOptions?.getOrNull(state.chooseChargeIndex)?.chargeCalculationType?.value | |
| ?: "" | |
| }, |
🤖 Prompt for AI Agents
In feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountScreen.kt around lines 263 to 268, the code uses .get(state.chooseChargeIndex) which can throw IndexOutOfBoundsException; change that call to .getOrNull(state.chooseChargeIndex) and keep the safe-chain access so the expression becomes .getOrNull(state.chooseChargeIndex)?.chargeCalculationType?.value ?: "" to match the surrounding usage and avoid crashes. | private fun handleEditChargeDialog(index : Int){ | ||
| val selectedEditCharge = state.addedCharges[index] | ||
| val chooseChargeIndex = state.template.chargeOptions?.indexOfFirst { | ||
| it.id == selectedEditCharge.id | ||
| }?: -1 | ||
| mutableStateFlow.update { | ||
| it.copy( | ||
| chargeAmount = selectedEditCharge.amount.toString(), | ||
| chargeDate = selectedEditCharge.date, | ||
| chooseChargeIndex = chooseChargeIndex, | ||
| dialogState = RecurringAccountState.DialogState.AddNewCharge(true,index) | ||
| | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Add bounds checking before accessing list element.
Line 266 accesses state.addedCharges[index] without verifying that the index is within bounds, which will cause an IndexOutOfBoundsException if an invalid index is passed.
Apply this diff to add bounds checking:
private fun handleEditChargeDialog(index : Int){ + if (index !in state.addedCharges.indices) return val selectedEditCharge = state.addedCharges[index]🤖 Prompt for AI Agents
In feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt around lines 265-279, the code directly reads state.addedCharges[index] which can throw IndexOutOfBoundsException; add a guard that verifies index is within 0 until state.addedCharges.size before accessing the list (if invalid, return early or no-op), and only then compute selectedEditCharge, chooseChargeIndex and call mutableStateFlow.update; keep the existing nullable handling for template.chargeOptions and ensure no further list access occurs when index is out of bounds. | private fun handleDeleteCharge(index : Int){ | ||
| val newCharges = state.addedCharges.toMutableList().apply { | ||
| removeAt(index) | ||
| } | ||
| mutableStateFlow.update{ | ||
| it.copy(addedCharges = newCharges) | ||
| } | ||
| } |
There was a problem hiding this comment.
Add bounds checking before removing list element.
Line 282 calls removeAt(index) without verifying that the index is within bounds, which will cause an IndexOutOfBoundsException if an invalid index is passed.
Apply this diff to add bounds checking:
private fun handleDeleteCharge(index : Int){ + if (index !in state.addedCharges.indices) return val newCharges = state.addedCharges.toMutableList().apply { removeAt(index) }📝 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.
| private fun handleDeleteCharge(index : Int){ | |
| val newCharges = state.addedCharges.toMutableList().apply { | |
| removeAt(index) | |
| } | |
| mutableStateFlow.update{ | |
| it.copy(addedCharges = newCharges) | |
| } | |
| } | |
| private fun handleDeleteCharge(index : Int){ | |
| if (index !in state.addedCharges.indices) return | |
| val newCharges = state.addedCharges.toMutableList().apply { | |
| removeAt(index) | |
| } | |
| mutableStateFlow.update{ | |
| it.copy(addedCharges = newCharges) | |
| } | |
| } |
🤖 Prompt for AI Agents
In feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt around lines 280 to 287, the code calls removeAt(index) without bounds checking which can throw IndexOutOfBoundsException; change the function to first validate that index is >= 0 and < state.addedCharges.size before calling removeAt (if invalid, either return early or log/ignore), then proceed to build the newCharges list and update mutableStateFlow only when a removal occurred. | private fun handleAddToChargeList(){ | ||
| val selectedIndex = state.chooseChargeIndex | ||
| val selectedCharges = state.template?.chargeOptions?.getOrNull(selectedIndex) | ||
| val amount = state.chargeAmount.toDoubleOrNull()?:selectedCharges?.amount?:0.0 | ||
| if (selectedCharges != null && state.chargeAmountError == null){ | ||
| val newCharge = CreatedCharges( | ||
| id = selectedCharges.id, | ||
| name = selectedCharges.name, | ||
| amount = amount, | ||
| date = state.chargeDate, | ||
| type = selectedCharges.chargeCalculationType?.value ?: "", | ||
| collectedOn = selectedCharges.chargeTimeType?.value ?: "", | ||
| ) | ||
| mutableStateFlow.update { | ||
| it.copy( | ||
| addedCharges = it.addedCharges + newCharge, | ||
| chooseChargeIndex = -1, | ||
| dialogState = null, | ||
| chargeAmount = "", | ||
| ) | ||
| } | ||
| } else { | ||
| mutableStateFlow.update { | ||
| it.copy( | ||
| chooseChargeIndex = -1, | ||
| dialogState = null, | ||
| chargeAmount = "", | ||
| ) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Review the else block behavior for better UX.
The else block (lines 344-352) dismisses the dialog and clears fields even when there's a validation error (chargeAmountError != null). Users typically expect the dialog to remain open when validation fails so they can correct the error.
Consider keeping the dialog open when validation errors exist:
- } else { + } else if (selectedCharges == null) { mutableStateFlow.update { it.copy( chooseChargeIndex = -1, dialogState = null, chargeAmount = "", ) } }📝 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.
| private fun handleAddToChargeList(){ | |
| val selectedIndex = state.chooseChargeIndex | |
| val selectedCharges = state.template?.chargeOptions?.getOrNull(selectedIndex) | |
| val amount = state.chargeAmount.toDoubleOrNull()?:selectedCharges?.amount?:0.0 | |
| if (selectedCharges != null && state.chargeAmountError == null){ | |
| val newCharge = CreatedCharges( | |
| id = selectedCharges.id, | |
| name = selectedCharges.name, | |
| amount = amount, | |
| date = state.chargeDate, | |
| type = selectedCharges.chargeCalculationType?.value ?: "", | |
| collectedOn = selectedCharges.chargeTimeType?.value ?: "", | |
| ) | |
| mutableStateFlow.update { | |
| it.copy( | |
| addedCharges = it.addedCharges + newCharge, | |
| chooseChargeIndex = -1, | |
| dialogState = null, | |
| chargeAmount = "", | |
| ) | |
| } | |
| } else { | |
| mutableStateFlow.update { | |
| it.copy( | |
| chooseChargeIndex = -1, | |
| dialogState = null, | |
| chargeAmount = "", | |
| ) | |
| } | |
| } | |
| } | |
| private fun handleAddToChargeList(){ | |
| val selectedIndex = state.chooseChargeIndex | |
| val selectedCharges = state.template?.chargeOptions?.getOrNull(selectedIndex) | |
| val amount = state.chargeAmount.toDoubleOrNull()?:selectedCharges?.amount?:0.0 | |
| if (selectedCharges != null && state.chargeAmountError == null){ | |
| val newCharge = CreatedCharges( | |
| id = selectedCharges.id, | |
| name = selectedCharges.name, | |
| amount = amount, | |
| date = state.chargeDate, | |
| type = selectedCharges.chargeCalculationType?.value ?: "", | |
| collectedOn = selectedCharges.chargeTimeType?.value ?: "", | |
| ) | |
| mutableStateFlow.update { | |
| it.copy( | |
| addedCharges = it.addedCharges + newCharge, | |
| chooseChargeIndex = -1, | |
| dialogState = null, | |
| chargeAmount = "", | |
| ) | |
| } | |
| } else if (selectedCharges == null) { | |
| mutableStateFlow.update { | |
| it.copy( | |
| chooseChargeIndex = -1, | |
| dialogState = null, | |
| chargeAmount = "", | |
| ) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt around lines 323–353, the else branch currently dismisses the dialog and clears the input even when chargeAmountError is present; update the logic so that only successful validation (inside the if where selectedCharges != null && chargeAmountError == null) clears and dismisses the dialog and resets chooseChargeIndex/chargeAmount, while the else (validation failed) should leave the dialogState, chooseChargeIndex and chargeAmount intact so the user can correct the error (i.e., remove or change the state.update in the else to either do nothing or only preserve existing fields rather than clearing/dismissing). | private fun handleEditCharge(index: Int) { | ||
| val selectedIndex = state.chooseChargeIndex | ||
| val selectedCharge = state.template.chargeOptions?.getOrNull(selectedIndex) | ||
| val amount = state.chargeAmount.toDoubleOrNull() ?: selectedCharge?.amount ?: 0.0 | ||
| if (selectedCharge != null && state.chargeAmountError == null) { | ||
| val newCharge = CreatedCharges( | ||
| id = selectedCharge.id, | ||
| name = selectedCharge.name, | ||
| amount = amount, | ||
| date = state.chargeDate, | ||
| type = selectedCharge.chargeCalculationType?.value ?: "", | ||
| collectedOn = selectedCharge.chargeTimeType?.value ?: "", | ||
| ) | ||
| val currentAddedCharges = state.addedCharges.toMutableList() | ||
| currentAddedCharges[index] = newCharge | ||
| mutableStateFlow.update { | ||
| it.copy( | ||
| addedCharges = currentAddedCharges, | ||
| chooseChargeIndex = -1, | ||
| dialogState = RecurringAccountState.DialogState.showCharges, | ||
| chargeAmount = "", | ||
| ) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Add bounds checking before updating list element.
Line 369 directly accesses currentAddedCharges[index] without verifying that the index is within bounds, which will cause an IndexOutOfBoundsException if an invalid index is passed.
Apply this diff to add bounds checking:
private fun handleEditCharge(index: Int) { + if (index !in state.addedCharges.indices) return val selectedIndex = state.chooseChargeIndex📝 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.
| private fun handleEditCharge(index: Int) { | |
| val selectedIndex = state.chooseChargeIndex | |
| val selectedCharge = state.template.chargeOptions?.getOrNull(selectedIndex) | |
| val amount = state.chargeAmount.toDoubleOrNull() ?: selectedCharge?.amount ?: 0.0 | |
| if (selectedCharge != null && state.chargeAmountError == null) { | |
| val newCharge = CreatedCharges( | |
| id = selectedCharge.id, | |
| name = selectedCharge.name, | |
| amount = amount, | |
| date = state.chargeDate, | |
| type = selectedCharge.chargeCalculationType?.value ?: "", | |
| collectedOn = selectedCharge.chargeTimeType?.value ?: "", | |
| ) | |
| val currentAddedCharges = state.addedCharges.toMutableList() | |
| currentAddedCharges[index] = newCharge | |
| mutableStateFlow.update { | |
| it.copy( | |
| addedCharges = currentAddedCharges, | |
| chooseChargeIndex = -1, | |
| dialogState = RecurringAccountState.DialogState.showCharges, | |
| chargeAmount = "", | |
| ) | |
| } | |
| } | |
| } | |
| private fun handleEditCharge(index: Int) { | |
| if (index !in state.addedCharges.indices) return | |
| val selectedIndex = state.chooseChargeIndex | |
| val selectedCharge = state.template.chargeOptions?.getOrNull(selectedIndex) | |
| val amount = state.chargeAmount.toDoubleOrNull() ?: selectedCharge?.amount ?: 0.0 | |
| if (selectedCharge != null && state.chargeAmountError == null) { | |
| val newCharge = CreatedCharges( | |
| id = selectedCharge.id, | |
| name = selectedCharge.name, | |
| amount = amount, | |
| date = state.chargeDate, | |
| type = selectedCharge.chargeCalculationType?.value ?: "", | |
| collectedOn = selectedCharge.chargeTimeType?.value ?: "", | |
| ) | |
| val currentAddedCharges = state.addedCharges.toMutableList() | |
| currentAddedCharges[index] = newCharge | |
| mutableStateFlow.update { | |
| it.copy( | |
| addedCharges = currentAddedCharges, | |
| chooseChargeIndex = -1, | |
| dialogState = RecurringAccountState.DialogState.showCharges, | |
| chargeAmount = "", | |
| ) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt around lines 355 to 379, the code updates currentAddedCharges[index] without verifying the index bounds, which can throw IndexOutOfBoundsException; check that index is >= 0 and < currentAddedCharges.size before assigning (or skip/return if invalid), and if invalid handle gracefully (e.g., log or set dialogState/chooseChargeIndex appropriately) so mutation only occurs when the index is valid. | val template: RecurringDepositAccountTemplate = RecurringDepositAccountTemplate(), | ||
| val recurringDepositAccountSettings: RecurringAccountSettingsState = RecurringAccountSettingsState(), | ||
| val chargeAmountError : StringResource? = null, | ||
| val chargeAmount : String = " ", |
There was a problem hiding this comment.
Initialize chargeAmount with empty string instead of space.
Line 749 initializes chargeAmount with " " (a space) instead of an empty string. This inconsistency could cause issues with string comparisons, validation logic, or trimming operations.
Apply this diff:
- val chargeAmount : String = " ", + val chargeAmount : String = "",📝 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 chargeAmount : String = " ", | |
| val chargeAmount : String = "", |
🤖 Prompt for AI Agents
In feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt around line 749, the property chargeAmount is initialized to a single space (" ") which can break comparisons/validation; change the initializer to an empty string ("") so chargeAmount = "" and ensure any callers relying on blankness still work (no extra trimming required). | sealed interface DialogState { | ||
| data object showCharges : DialogState | ||
| data class AddNewCharge (val edit : Boolean, val index : Int = -1) : DialogState | ||
| data class SuccessResponseStatus(val successStatus: Boolean, val msg: String = "") : | ||
| DialogState | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Fix inconsistent naming for sealed interface member.
Line 768 defines showCharges with a lowercase 's', which doesn't follow Kotlin naming conventions for sealed interface members (should be PascalCase). Additionally, line 374 references it as showCharges, creating inconsistency.
Apply this diff to follow naming conventions:
sealed interface DialogState { - data object showCharges : DialogState + data object ShowCharges : DialogState data class AddNewCharge (val edit : Boolean, val index : Int = -1) : DialogStateThen update the reference at line 374:
- dialogState = RecurringAccountState.DialogState.showCharges, + dialogState = RecurringAccountState.DialogState.ShowCharges,📝 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.
| sealed interface DialogState { | |
| data object showCharges : DialogState | |
| data class AddNewCharge (val edit : Boolean, val index : Int = -1) : DialogState | |
| data class SuccessResponseStatus(val successStatus: Boolean, val msg: String = "") : | |
| DialogState | |
| } | |
| sealed interface DialogState { | |
| data object ShowCharges : DialogState | |
| data class AddNewCharge (val edit : Boolean, val index : Int = -1) : DialogState | |
| data class SuccessResponseStatus(val successStatus: Boolean, val msg: String = "") : | |
| DialogState | |
| } |
🤖 Prompt for AI Agents
In feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt around lines 767-772, the sealed interface member is named `showCharges` (lowercase) which violates Kotlin naming conventions and is referenced elsewhere; rename the sealed member to PascalCase `ShowCharges` and update all usages including the reference at line 374 to use `DialogState.ShowCharges`; ensure imports/qualified references remain correct and run a quick compile to catch any other occurrences to rename. 1 participant
Fixes - Jira-#Issue_Number
Didn't create a Jira ticket, click here to create new.
Please Add Screenshots If there are any UI changes.
Please make sure these boxes are checked before submitting your pull request - thanks!
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
New Features
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.