Feature Terms Steps for Recurring Deposit account Flow. 551 #2544
Conversation
WalkthroughRefactors Terms UI to a state-driven API: adds four recurring-deposit string resources, introduces RecurringAccountInterestChart state and actions in the ViewModel, updates screen wiring to pass state/onAction, and rewrites TermsPage to use dropdowns bound to state and dispatch actions. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant TermsPage as "TermsPage (UI)"
participant Screen as "RecurringAccountScreen"
participant VM as "RecurringAccountViewModel"
participant Store as "StateStore"
Note over TermsPage,VM `#f0f4ff`: State-driven dropdown flow
User->>TermsPage: select dropdown option
TermsPage->>Screen: onAction(RecurringAccountInterestChartAction.*)
Screen->>VM: handleAction(action)
VM->>VM: route -> private handler -> update state
VM->>Store: emit updated RecurringAccountState
Store-->>Screen: state updated
Screen->>TermsPage: pass new state
TermsPage->>User: UI recomposed with updated selection
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt (2)
113-116: Critical: Interest chart values not used in payload.The interest chart fields are hardcoded to
nullin the payload despite being collected in the UI throughrecurringDepositAccountInterestChart. This means user selections from the Interest page are ignored when creating the account.Apply this diff:
- interestCalculationDaysInYearType = null, - interestCalculationType = null, - interestCompoundingPeriodType = null, - interestPostingPeriodType = null, + interestCalculationDaysInYearType = s.recurringDepositAccountInterestChart.interestCalculationDaysInYearType, + interestCalculationType = s.recurringDepositAccountInterestChart.interestCalculationType, + interestCompoundingPeriodType = s.recurringDepositAccountInterestChart.interestCompoundingPeriodType, + interestPostingPeriodType = s.recurringDepositAccountInterestChart.interestPostingPeriodType,
211-226: Include interest chart state in reset.The
resetForRetrymethod resets all state fields exceptrecurringDepositAccountInterestChart, which should also be cleared to ensure a clean retry state.Apply this diff:
private fun resetForRetry() { mutableStateFlow.update { it.copy( isOnline = false, clientId = -1, currentStep = 0, screenState = ScreenState.Loading, recurringDepositAccountDetail = RecurringAccountDetailsState(), template = RecurringDepositAccountTemplate(), recurringDepositAccountSettings = RecurringAccountSettingsState(), + recurringDepositAccountInterestChart = RecurringAccountInterestChartState(), currencyIndex = -1, currencyError = null, ) } loadRecurringAccountTemplate() }
🧹 Nitpick comments (1)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/InterestPage.kt (1)
36-41: Consider using empty string instead of space for null values.All four dropdowns use a single space
" "when the value is null. Using an empty string""would be more conventional and avoid potential display issues.Example for first dropdown:
value = if (state.recurringDepositAccountInterestChart.interestCompoundingPeriodType == null) { - " " + "" } else {Apply the same change to the other three dropdowns.
Also applies to: 57-62, 78-83, 99-104
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
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(1 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt(5 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/InterestPage.kt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/InterestPage.kt (1)
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosTextFieldDropdown.kt (1)
MifosTextFieldDropdown(39-112)
⏰ 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 (5)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt (4)
173-209: LGTM!The handler methods correctly update the
recurringDepositAccountInterestChartstate using immutable copy operations.
619-630: LGTM!Action routing to the interest chart handlers is correctly implemented.
670-675: LGTM!The
RecurringAccountInterestChartStatedata class is well-structured with appropriate nullable Int fields for the four interest parameters.
778-783: LGTM!The
RecurringAccountInterestChartActionsealed class properly defines actions for all four interest chart parameters.feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountScreen.kt (1)
94-97: LGTM!The InterestPage invocation correctly passes both
stateandonAction, aligning with the refactored stateful API.
...recurringDeposit/src/commonMain/composeResources/values/feature_recurring_deposit_string.xml
Outdated
Show resolved
Hide resolved
...n/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/InterestPage.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/TermsPage.kt (3)
38-38: Use empty string instead of space for null values.All four dropdowns use a space
" "when the state value is null. Consider using an empty string""instead for consistency and to avoid potential spacing issues in the UI.Apply this diff to all four dropdowns:
- value = if (state.recurringDepositAccountInterestChart.interestCompoundingPeriodType == null) { - " " - } else { + value = if (state.recurringDepositAccountInterestChart.interestCompoundingPeriodType == null) { + "" + } else {Also applies to: 59-59, 80-80, 101-101
36-118: Consider extracting a helper function to reduce duplication.All four dropdown components follow an identical pattern with only minor variations. Extracting a helper function would improve maintainability and reduce the risk of copy-paste errors (like the one on line 40).
Example approach:
@Composable private fun InterestChartDropdown( value: Int?, options: List<ValueOption>?, label: String, onOptionSelected: (Int) -> Unit ) { MifosTextFieldDropdown( value = if (value == null) { "" } else { options?.get(value)?.value ?: "" }, onValueChanged = { }, onOptionSelected = { index, _ -> onOptionSelected(index) }, options = options?.map { it.value ?: "" } ?: emptyList(), label = label, ) }
13-13: String resource name has inconsistent casing.The resource name
feature_recurring_deposit_Calculation_Days_In_Yearhas a capital 'C' in the middle, breaking the snake_case naming convention. This should befeature_recurring_deposit_calculation_days_in_year.Note: This change would need to be made in the string resource file
feature_recurring_deposit_string.xmland updated here accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountScreen.kt(1 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/InterestPage.kt(2 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/TermsPage.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountScreen.kt
- feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/InterestPage.kt
🧰 Additional context used
🧬 Code graph analysis (1)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/TermsPage.kt (1)
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosTextFieldDropdown.kt (1)
MifosTextFieldDropdown(39-112)
⏰ 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 (1)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/TermsPage.kt (1)
32-33: State structure and template field definitions verified—all required fields are properly defined.Verification confirms:
RecurringAccountStatehasrecurringDepositAccountInterestChart: RecurringAccountInterestChartStateRecurringAccountStatehastemplate: RecurringDepositAccountTemplateRecurringAccountInterestChartStatehas all four properties (interestCalculationDaysInYearType, interestCalculationType, interestCompoundingPeriodType, interestPostingPeriodType)RecurringDepositAccountTemplatehas all four *Options listsRecurringAccountAction.RecurringAccountInterestChartActionhas all four required action types
...Main/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/TermsPage.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
feature/recurringDeposit/src/commonMain/composeResources/values/feature_recurring_deposit_string.xml (1)
55-55: Remove trailing space from string value.The string value contains a trailing space after "Year".
Apply this diff:
- <string name="feature_recurring_deposit_calculation_days_in_year">Days In Year </string> + <string name="feature_recurring_deposit_calculation_days_in_year">Days In Year</string>
🧹 Nitpick comments (1)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/TermsPage.kt (1)
35-117: Consider extracting common dropdown pattern.All four dropdowns follow an identical structure with only the state field, action, options list, and label differing. A helper composable function could reduce duplication and improve maintainability.
Example approach:
@Composable private fun InterestChartDropdown( selectedIndex: Int?, options: List<String>, label: String, onSelect: (Int) -> Unit ) { MifosTextFieldDropdown( value = selectedIndex?.let { options.getOrNull(it) } ?: " ", onValueChanged = { }, onOptionSelected = { index, _ -> onSelect(index) }, options = options, label = label, ) }Then use it for each dropdown:
InterestChartDropdown( selectedIndex = state.recurringDepositAccountInterestChart.interestCompoundingPeriodType, options = state.template.interestCompoundingPeriodTypeOptions?.mapNotNull { it.value } ?: emptyList(), label = stringResource(Res.string.feature_recurring_deposit_interest_compounding_period), onSelect = { index -> onAction(RecurringAccountAction.RecurringAccountInterestChartAction.OnInterestCompoundingPeriodType(index)) } )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
feature/recurringDeposit/src/commonMain/composeResources/values/feature_recurring_deposit_string.xml(1 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/InterestPage.kt(2 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/TermsPage.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/InterestPage.kt
🧰 Additional context used
🧬 Code graph analysis (1)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/TermsPage.kt (1)
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosTextFieldDropdown.kt (1)
MifosTextFieldDropdown(39-112)
⏰ 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 (1)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/TermsPage.kt (1)
30-33: Well-designed API surface change.The refactoring from a simple callback to a state-driven pattern with actions improves testability and aligns with modern Compose architecture.
...Main/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/TermsPage.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/TermsPage.kt (2)
40-122: Dropdowns are correctly bound; consider extracting a shared helper to reduce duplicationAll four dropdowns correctly:
- Derive
valuefrom the corresponding*Typeindex usinggetOrNull(avoiding out‑of‑bounds),- Map template option
values toList<String>,- Dispatch the appropriate
RecurringAccountInterestChartAction.Given the very similar structure, you might want to extract a small private composable to avoid repetition, e.g. something along the lines of:
@Composable private fun InterestDropdown( label: String, selectedIndex: Int?, options: List<String>, onSelected: (Int) -> Unit, ) { MifosTextFieldDropdown( value = selectedIndex?.let { options.getOrNull(it) } ?: " ", onValueChanged = { }, onOptionSelected = { index, _ -> onSelected(index) }, options = options, label = label, ) }Then each block becomes a one‑liner with its own label,
selectedIndex, andoptions. This keeps behavior identical while making future changes less error‑prone.
48-54: Use_for unusedvalueparameter inonOptionSelectedlambdasIn all four dropdowns, the
valueparameter ofonOptionSelected = { index, value -> ... }is unused. To avoid warnings and make intent clearer, you can ignore it explicitly:- onOptionSelected = { index, value -> + onOptionSelected = { index, _ ->Apply this change to each
onOptionSelectedblock.Also applies to: 69-75, 90-96, 111-117
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/TermsPage.kt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/TermsPage.kt (2)
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosTextFieldDropdown.kt (1)
MifosTextFieldDropdown(39-112)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosTwoButtonRow.kt (1)
MifosTwoButtonRow(31-91)
⏰ 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/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/TermsPage.kt (2)
13-18: Imports for resources, design system, and state/actions look consistentThe new imports for generated string resources, design system components, and
RecurringAccountState/RecurringAccountActionalign with how the rest of the feature appears to be structured; nothing problematic here.Also applies to: 22-30
34-37: State + action‑based TermsPage API and navigation wiring look correct
TermsPagenow depends onRecurringAccountStateand dispatchesRecurringAccountActionthroughonAction, and the Back/Next buttons correctly emitOnBackPress/OnNextPress. This is a clean, unidirectional data‑flow setup and should integrate well with the ViewModel.Also applies to: 124-128
|
@ankitkumarrain video is not uploaded check once |
revanthkumarJ
left a comment
revanthkumarJ
left a comment
There was a problem hiding this comment.
and also in terms page video show the options clicking and next button should be enabled only after every option selected
| onFirstBtnClick = { onAction(RecurringAccountAction.OnBackPress) }, | ||
| onSecondBtnClick = { onAction(RecurringAccountAction.OnNextPress) }, | ||
| isButtonIconVisible = true, | ||
|
|
There was a problem hiding this comment.
i think you can use isButtonEnabled some thing like that in MifosTwoButtonRow to make sure next button is enabled only after everything is filled
There was a problem hiding this comment.
enabled next button
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt (2)
113-116: Populate interest chart fields from state.The payload hardcodes these fields to
null, but the PR introducesrecurringDepositAccountInterestChartstate to capture user selections. These fields should be populated fromstate.recurringDepositAccountInterestChartinstead of being set tonull.Apply this diff:
fieldOfficerId = s.recurringDepositAccountDetail.fieldOfficer?.id, - interestCalculationDaysInYearType = null, - interestCalculationType = null, - interestCompoundingPeriodType = null, - interestPostingPeriodType = null, + interestCalculationDaysInYearType = s.recurringDepositAccountInterestChart.interestCalculationDaysInYearType, + interestCalculationType = s.recurringDepositAccountInterestChart.interestCalculationType, + interestCompoundingPeriodType = s.recurringDepositAccountInterestChart.interestCompoundingPeriodType, + interestPostingPeriodType = s.recurringDepositAccountInterestChart.interestPostingPeriodType, isCalendarInherited = null,
211-226: Reset the interest chart state during retry.The
resetForRetrymethod resets other account state fields but does not resetrecurringDepositAccountInterestChart. This could leave stale data in the state after a retry.Apply this diff:
private fun resetForRetry() { mutableStateFlow.update { it.copy( isOnline = false, clientId = -1, currentStep = 0, screenState = ScreenState.Loading, recurringDepositAccountDetail = RecurringAccountDetailsState(), template = RecurringDepositAccountTemplate(), recurringDepositAccountSettings = RecurringAccountSettingsState(), + recurringDepositAccountInterestChart = RecurringAccountInterestChartState(), currencyIndex = -1, currencyError = null, ) } loadRecurringAccountTemplate() }
🧹 Nitpick comments (4)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/TermsPage.kt (3)
48-54: Consider removing unusedvalueparameter.All four
onOptionSelectedcallbacks receive bothindexandvalueparameters but only useindex. While this matches theMifosTextFieldDropdownsignature, you could use_for the unused parameter to make the intent clearer:-onOptionSelected = { index, value -> +onOptionSelected = { index, _ ->Also applies to: 69-74, 90-96, 111-117
41-46: Consider using empty string instead of space for null values.All four dropdowns use
" "(a single space) as the default value when the state field is null. Using an empty string""would be more conventional and avoid potential issues if other code checks for empty values via.isEmpty().-value = if (state.recurringDepositAccountInterestChart.interestCompoundingPeriodType == null) { - " " -} else { +value = state.recurringDepositAccountInterestChart.interestCompoundingPeriodType?.let { index -> - state.template.interestCompoundingPeriodTypeOptions?.getOrNull(state.recurringDepositAccountInterestChart.interestCompoundingPeriodType)?.value - ?: "" -}, + state.template.interestCompoundingPeriodTypeOptions?.getOrNull(index)?.value +} ?: "",This pattern can be applied to all four dropdowns for cleaner null handling.
Also applies to: 62-67, 83-88, 104-109
39-39: Consider adding consistent padding.The
ColumnandMifosTwoButtonRowlack explicit padding modifiers, which might cause the content to appear cramped or touch screen edges. Consider adding:-Column(horizontalAlignment = Alignment.CenterHorizontally) { +Column( + horizontalAlignment = Alignment.CenterHorizontally, + modifier = Modifier.padding(DesignToken.padding.medium) +) {And adding spacing before the button row:
if (state.recurringDepositAccountDetail.isMiniLoaderActive) { MifosProgressIndicatorMini() } + Spacer(modifier = Modifier.height(DesignToken.padding.large)) MifosTwoButtonRow(Also, remove the unnecessary blank line at line 134:
isSecondButtonEnabled = state.recurringDepositAccountInterestChart.isTermsButtonEnabled, isButtonIconVisible = true, - )Also applies to: 127-136
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt (1)
173-209: Remove the extra blank line for consistency.Line 179 contains an unnecessary blank line inside the
copy()call, which is inconsistent with the other three handler methods that follow the same pattern.Apply this diff:
private fun handleInterestCalculationDaysInYearType(action: RecurringAccountAction.RecurringAccountInterestChartAction.OnInterestCalculationDaysInYearType) { mutableStateFlow.update { it.copy( recurringDepositAccountInterestChart = it.recurringDepositAccountInterestChart.copy( interestCalculationDaysInYearType = action.interestCalculationTypeDaysInYear, ), - ) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
feature/client/src/commonMain/composeResources/values/strings.xml(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/DetailsPage.kt(2 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt(7 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/TermsPage.kt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/TermsPage.kt (3)
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosTextFieldDropdown.kt (1)
MifosTextFieldDropdown(39-112)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosProgressIndicator.kt (1)
MifosProgressIndicatorMini(122-146)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosTwoButtonRow.kt (1)
MifosTwoButtonRow(31-91)
⏰ 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 (9)
feature/client/src/commonMain/composeResources/values/strings.xml (1)
539-545: String key rename looks consistentRenaming the resource key to
field_officerwith the same user-visible text is fine and aligns with lowercase/underscore naming. Just ensure any localized resource files and all call sites have been updated to use the new key so nothing breaks at compile time.feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/pages/DetailsPage.kt (1)
18-18: Field officer label correctly wired to renamed string keyUpdating the import and dropdown label to use
Res.string.field_officermatches the new resource name and keeps the UI text unchanged. This keeps the Details page in sync with the global string rename.Also applies to: 133-133
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/TermsPage.kt (4)
12-32: LGTM! Imports align well with the state-driven refactor.The new imports for string resources, UI components, and state management types are appropriate for the refactored implementation.
35-38: LGTM! State-driven pattern improves maintainability.The refactored signature using
RecurringAccountStateandRecurringAccountActionis a solid architectural choice that improves testability and state management.
123-125: LGTM! Progress indicator placement is appropriate.The conditional rendering of
MifosProgressIndicatorMinibased onisMiniLoaderActiveprovides good user feedback during async operations.
132-132: LGTM! Button state management addresses previous feedback.The
isSecondButtonEnabledis correctly tied tostate.recurringDepositAccountInterestChart.isTermsButtonEnabled, addressing the previous review comment about ensuring the next button is only enabled when all required fields are filled.feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt (3)
619-630: LGTM!The action routing correctly dispatches the four interest chart actions to their respective handlers and follows the established pattern in the file.
644-644: LGTM!The state field addition follows the established pattern and is correctly initialized with a default value.
787-792: LGTM!The sealed class structure for interest chart actions is well-defined and follows the established pattern for other action groups in the file.
...n/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/TermsPage.kt (1)
40-59: Use_for unused lambda parameters.The
valueparameter in all fouronOptionSelectedlambdas is unused since only theindexis dispatched to the action. Replace with_to follow Kotlin conventions and suppress IDE warnings.Example fix for the first dropdown (apply similarly to all four):
onOptionSelected = { index, value -> + onOptionSelected = { index, _ -> onAction( RecurringAccountAction.RecurringAccountInterestChartAction.OnInterestCompoundingPeriodType( index, ), ) },Also applies to: 61-80, 82-101, 103-122
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt(6 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/TermsPage.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt
🧰 Additional context used
🧬 Code graph analysis (1)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/TermsPage.kt (3)
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosTextFieldDropdown.kt (1)
MifosTextFieldDropdown(39-112)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosProgressIndicator.kt (1)
MifosProgressIndicatorMini(122-146)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosTwoButtonRow.kt (1)
MifosTwoButtonRow(31-91)
🔇 Additional comments (3)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/TermsPage.kt (3)
34-38: LGTM on the state-driven signature refactor.The function signature correctly follows the unidirectional data flow pattern with state and action callback. This aligns well with the broader refactoring mentioned in the AI summary.
127-135: Good implementation of conditional button enablement.The
isSecondButtonEnabledis correctly wired tostate.recurringDepositAccountInterestChart.isTermsButtonEnabled, which addresses the past review suggestion to enable the next button only after all fields are filled. This ensures proper form validation before navigation.
123-125: State pathisMiniLoaderActiveis correct—no changes needed.Verification confirms that
isMiniLoaderActiveis indeed a property ofRecurringAccountDetailsState(line 665 in RecurringAccountViewModel.kt), and the access pathstate.recurringDepositAccountDetail.isMiniLoaderActiveis valid and intentional. This pattern is consistently used elsewhere in the codebase (DetailsPage.kt line 156).
...n/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt (1)
113-116: Critical: Interest chart state values are never used.The four interest chart fields are hardcoded to
nullin the payload, but the PR adds an entire state object (recurringDepositAccountInterestChart) to track these values from user input. This means any selections made on the Terms page will be discarded when creating the account.Apply this diff to use the tracked state values:
- interestCalculationDaysInYearType = null, - interestCalculationType = null, - interestCompoundingPeriodType = null, - interestPostingPeriodType = null, + interestCalculationDaysInYearType = s.recurringDepositAccountInterestChart.interestCalculationDaysInYearType, + interestCalculationType = s.recurringDepositAccountInterestChart.interestCalculationType, + interestCompoundingPeriodType = s.recurringDepositAccountInterestChart.interestCompoundingPeriodType, + interestPostingPeriodType = s.recurringDepositAccountInterestChart.interestPostingPeriodType,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt(6 hunks)
⏰ 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 (4)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt (4)
173-209: Handlers are structurally correct.The four private handler methods follow the established pattern in this ViewModel for updating state immutably via
mutableStateFlow.updateand nestedcopyoperations.
619-630: Action routing is correct.The four new interest chart actions are properly wired into the
handleActiondispatch, each routing to its corresponding private handler method.
644-644: State field addition is appropriate.The
recurringDepositAccountInterestChartfield is correctly added toRecurringAccountStatewith proper type and default value.
783-788: Action definitions follow correct pattern.The
RecurringAccountInterestChartActionsealed class and its four data class cases are properly structured and follow the established action pattern in this codebase.
...n/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt
Show resolved
Hide resolved
revanthkumarJ
left a comment
revanthkumarJ
left a comment
There was a problem hiding this comment.
Let me know once you done those i will approve
...n/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt
Outdated
Show resolved
Hide resolved
| Spacer(modifier = Modifier.height(DesignToken.padding.large)) | ||
| MifosTextFieldDropdown( | ||
| value = if (state.recurringDepositAccountInterestChart.interestPostingPeriodType == null) { | ||
| "" |
There was a problem hiding this comment.
i think here
value = if (state.recurringDepositAccountInterestChart.interestPostingPeriodType == null)
it won't become null at any case as we initializing with -1 and later changing it to a index value but not null.
change it to ==-1
...Main/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/TermsPage.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/TermsPage.kt (1)
40-59: PotentialIndexOutOfBoundsExceptionwith.get()access.While the
== -1sentinel check is correct for the non-nullable pattern, using.get(index)on the options list can still throw if the stored index exceeds the list size (e.g., stale state after template reload). Consider using.getOrNull()for defensive access.I note there was conflicting advice in past reviews on this. However, the index being non-nullable doesn't guarantee it's within bounds of the options list.
- state.template.interestCompoundingPeriodTypeOptions?.get(state.recurringDepositAccountInterestChart.interestCompoundingPeriodType)?.value + state.template.interestCompoundingPeriodTypeOptions?.getOrNull(state.recurringDepositAccountInterestChart.interestCompoundingPeriodType)?.valueSame pattern applies to the other three dropdowns at lines 65, 86, and 107.
🧹 Nitpick comments (1)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt (1)
783-788: Minor: Inconsistent parameter naming in action class.The parameter name
interestCalculationTypeDaysInYeardiffers from the naming pattern used in other actions (e.g.,interestCompoundingPeriodType,interestPostingPeriodType). Consider renaming for consistency.sealed class RecurringAccountInterestChartAction : RecurringAccountAction() { - data class OnInterestCalculationDaysInYearType(val interestCalculationTypeDaysInYear: Int) : RecurringAccountInterestChartAction() + data class OnInterestCalculationDaysInYearType(val interestCalculationDaysInYearType: Int) : RecurringAccountInterestChartAction() data class OnInterestCompoundingPeriodType(val interestCompoundingPeriodType: Int) : RecurringAccountInterestChartAction() data class OnInterestCalculationType(val interestCalculationType: Int) : RecurringAccountInterestChartAction() data class OnInterestPostingPeriodType(val interestPostingPeriodType: Int) : RecurringAccountInterestChartAction() }If applied, update the handler at line 177 accordingly:
interestCalculationDaysInYearType = action.interestCalculationDaysInYearType,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt(6 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/TermsPage.kt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/TermsPage.kt (3)
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosTextFieldDropdown.kt (1)
MifosTextFieldDropdown(39-112)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosProgressIndicator.kt (1)
MifosProgressIndicatorMini(122-146)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosTwoButtonRow.kt (1)
MifosTwoButtonRow(31-91)
🔇 Additional comments (6)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt (3)
173-209: LGTM!The handler functions follow the established pattern in this file and correctly update the nested
recurringDepositAccountInterestChartstate using immutable copy operations.
670-679: Type inconsistency resolved.The fields now correctly use non-nullable
Int = -1sentinel values, matching the convention used elsewhere in this file (e.g.,fieldOfficerIndexat line 662). TheisTermsButtonEnabledlogic properly validates all four fields.
618-630: LGTM!The action routing correctly dispatches to the appropriate handlers for all four interest chart actions.
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/TermsPage.kt (3)
35-38: Good refactor to state-driven API.The updated function signature properly accepts
RecurringAccountStateand anonActioncallback, enabling unidirectional data flow and better testability.
127-135: LGTM!The button row correctly wires back/next actions through
onActionand usesisTermsButtonEnabledfrom state to control the next button, ensuring all dropdowns are filled before proceeding.
123-125: I'll help you verify this review comment. Let me start by investigating whereisMiniLoaderActiveis set and how it's used across the codebase.
<function_calls>
#!/bin/bashFind where isMiniLoaderActive is set to true
rg -n "isMiniLoaderActive\s*=" --type=kotlin -A 2 -B 2
</function_calls>
#!/bin/bashFind all usages of isMiniLoaderActive to see where it's checked
rg -n "isMiniLoaderActive" --type=kotlin -B 2 -A 2
</function_calls>
<function_calls>
#!/bin/bashLook at the TermsPage file to understand the context better
wc -l "feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/TermsPage.kt"
</function_calls>
<function_calls>
#!/bin/bashFind the state/ViewModel definitions
fd -t f -e ".kt$" | xargs rg -l "recurringDepositAccountDetail" | head -20
</function_calls>
2 participants
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.
WhatsApp.Video.2025-11-25.at.22.03.45_587f1ab4.mp4
Uploading WhatsApp Video 2025-11-25 at 22.03.45_587f1ab4.mp4…
this is video.