Feature Terms Steps for Recurring Deposit account Flow. 551 #2544
Conversation
WalkthroughRefactors Terms UI to a state-driven API: adds four new string resources, introduces RecurringAccountInterestChart state and actions in the ViewModel, updates screen wiring to pass state/onAction, and rewrites TermsPage to expose four dropdowns bound to state. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant TermsPage
participant RecurringAccountScreen
participant RecurringAccountViewModel
participant StateStore
Note over TermsPage,RecurringAccountViewModel: State-driven dropdown flow
User->>TermsPage: select dropdown option
TermsPage->>RecurringAccountScreen: onAction(RecurringAccountInterestChartAction.*)
RecurringAccountScreen->>RecurringAccountViewModel: handleAction(action)
RecurringAccountViewModel->>RecurringAccountViewModel: route -> private handler -> update state.copy()
RecurringAccountViewModel->>StateStore: emit updated RecurringAccountState
StateStore-->>RecurringAccountScreen: state updated
RecurringAccountScreen->>TermsPage: pass new state
TermsPage->>User: UI recomposed with updated selection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
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.
| <string name="feature_recurring_deposit_interest_compounding_period">Interest Compounding Period</string> | ||
| <string name="feature_recurring_deposit_interest_posting_period">Interest Posting Period</string> | ||
| <string name="feature_recurring_deposit_interest_calculation">Interest Calculated Using</string> | ||
| <string name="feature_recurring_deposit_Calculation_Days_In_Year">Days In Year </string> |
There was a problem hiding this comment.
Fix inconsistent naming convention and remove trailing space.
The string resource name uses an inconsistent naming pattern with a capital 'C' in Calculation_Days_In_Year, while all other resource names use lowercase. Additionally, the string value has a trailing space.
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>🤖 Prompt for AI Agents
In
feature/recurringDeposit/src/commonMain/composeResources/values/feature_recurring_deposit_string.xml
around line 55, the string resource name uses an inconsistent capital 'C'
("Calculation_Days_In_Year") and the value contains a trailing space; rename the
resource to use lowercase and match the project's snake_case style (e.g.,
"calculation_days_in_year") and remove the trailing space from the string value
so it becomes "Days In Year".
| MifosTextFieldDropdown( | ||
| value = if (state.recurringDepositAccountInterestChart.interestCompoundingPeriodType == null) { | ||
| " " | ||
| } else { | ||
| state.template.interestCalculationDaysInYearTypeOptions?.get(state.recurringDepositAccountInterestChart.interestCompoundingPeriodType)?.value | ||
| ?: "" | ||
| }, | ||
| onValueChanged = { }, | ||
| onOptionSelected = { index, value -> | ||
| onAction( | ||
| RecurringAccountAction.RecurringAccountInterestChartAction.OnInterestCompoundingPeriodType( | ||
| index, | ||
| ), | ||
| ) | ||
| }, | ||
| options = state.template.interestCompoundingPeriodTypeOptions?.map { | ||
| it.value ?: "" | ||
| } ?: emptyList(), | ||
| label = stringResource(Res.string.feature_recurring_deposit_interest_compounding_period), | ||
| ) |
There was a problem hiding this comment.
Critical: Wrong template options list used for dropdown value.
Line 39 accesses state.template.interestCalculationDaysInYearTypeOptions when it should use state.template.interestCompoundingPeriodTypeOptions to match the interestCompoundingPeriodType field.
Apply this diff:
MifosTextFieldDropdown(
value = if (state.recurringDepositAccountInterestChart.interestCompoundingPeriodType == null) {
" "
} else {
- state.template.interestCalculationDaysInYearTypeOptions?.get(state.recurringDepositAccountInterestChart.interestCompoundingPeriodType)?.value
+ state.template.interestCompoundingPeriodTypeOptions?.get(state.recurringDepositAccountInterestChart.interestCompoundingPeriodType)?.value
?: ""
},🤖 Prompt for AI Agents
In
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/InterestPage.kt
around lines 35 to 54, the dropdown value lookup uses
state.template.interestCalculationDaysInYearTypeOptions but must use
state.template.interestCompoundingPeriodTypeOptions to match the
interestCompoundingPeriodType field and the options list; update the value
expression to fetch the selected display string from
state.template.interestCompoundingPeriodTypeOptions (same null-safe access and
fallback logic) so it aligns with the options passed to the
MifosTextFieldDropdown.
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
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.
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.
| data class RecurringAccountInterestChartState( | ||
| val interestCalculationDaysInYearType: Int? = null, | ||
| val interestCalculationType: Int? = null, | ||
| val interestCompoundingPeriodType: Int? = null, | ||
| val interestPostingPeriodType: Int? = null, | ||
|
|
||
|
|
||
| ){ | ||
| val isTermsButtonEnabled = interestCalculationDaysInYearType != -1 && interestPostingPeriodType != -1 | ||
| && interestCompoundingPeriodType != -1 && interestCalculationType != -1 | ||
|
|
||
| } |
There was a problem hiding this comment.
Fix type inconsistency causing incorrect button state.
The fields are declared as Int? (nullable) with a default of null, but isTermsButtonEnabled checks them against -1. Since null != -1 evaluates to true, the button will always be enabled even when no selections have been made.
Based on the codebase convention (e.g., fieldOfficerIndex: Int = -1 on line 662), use non-nullable Int with -1 as the sentinel value.
Apply this diff:
data class RecurringAccountInterestChartState(
- val interestCalculationDaysInYearType: Int? = null,
- val interestCalculationType: Int? = null,
- val interestCompoundingPeriodType: Int? = null,
- val interestPostingPeriodType: Int? = null,
+ val interestCalculationDaysInYearType: Int = -1,
+ val interestCalculationType: Int = -1,
+ val interestCompoundingPeriodType: Int = -1,
+ val interestPostingPeriodType: Int = -1,
){
val isTermsButtonEnabled = interestCalculationDaysInYearType != -1 && interestPostingPeriodType != -1
&& interestCompoundingPeriodType != -1 && interestCalculationType != -1
}🤖 Prompt for AI Agents
In
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt
around lines 670-681, the four fields (interestCalculationDaysInYearType,
interestCalculationType, interestCompoundingPeriodType,
interestPostingPeriodType) are declared as nullable Int? = null but compared to
-1, causing isTermsButtonEnabled to be true when null; change each field to a
non-nullable Int with a sentinel default of -1 (e.g., Int = -1) so the existing
comparison logic works correctly, and keep the isTermsButtonEnabled expression
as-is.
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-21.at.19.31.01_3aa68443.mp4
this is video.