Implement Interest Charts Step for New Recurring Deposits Account Flow #2552
Conversation
WalkthroughThis PR introduces nullable handling for balance/message fields in core components, refactors the recurring deposit feature to support a rate chart dialog, restructures action types from InterestChartAction to TermAction, and converts InterestPage from callback-based to state-driven. String resources for recurring deposit are added. Changes
Sequence DiagramsequenceDiagram
participant User
participant RecurringAccountScreen
participant RecurringAccountViewModel
participant RecurringAccountDialog
participant InterestPage
participant RateChart
User->>InterestPage: View Interest Page
InterestPage->>User: Display rate chart list
User->>InterestPage: Click "View Rate Chart"
InterestPage->>RecurringAccountViewModel: onAction(OnShowRateChartDialog)
RecurringAccountViewModel->>RecurringAccountViewModel: Set dialogState = RateChartDialog
RecurringAccountViewModel-->>RecurringAccountScreen: Updated state with dialogState
RecurringAccountScreen->>RecurringAccountDialog: Render with RateChartDialog state
RecurringAccountDialog->>RateChart: Display rate chart
RateChart-->>User: Show chart slabs
User->>RateChart: Click back/next
RateChart->>RecurringAccountViewModel: onAction(OnDismissDialog or OnNextPress)
RecurringAccountViewModel->>RecurringAccountViewModel: Clear dialogState
RecurringAccountViewModel-->>RecurringAccountScreen: Updated state
RecurringAccountDialog->>RecurringAccountDialog: Dismiss dialog
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
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: 5
🧹 Nitpick comments (6)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosEmptyCard.kt (2)
40-45: Optional: usemsg?.let { … }for a slightly more idiomatic patternYou can make the null‑check + rendering a bit more idiomatic without changing behavior:
- if (msg != null) { - Spacer(Modifier.height((DesignToken.padding.medium))) - Text( - text = msg, - style = MifosTypography.bodySmall, - ) - } + msg?.let { + Spacer(Modifier.height(DesignToken.padding.medium)) + Text( + text = it, + style = MifosTypography.bodySmall, + ) + }Pure style; feel free to keep the existing version if you prefer.
32-36: Consider applyingmodifiertoMifosListingComponentOutlineinstead of the innerColumnRight now the
modifierparameter only affects the innerColumn, whileMifosListingComponentOutline’s ownmodifierparameter is left at its default. If you want callers’ modifiers (e.g., padding, click, test tags) to apply to the card container (with border/background), it may be clearer to wire it like this:-fun MifosEmptyCard( - msg: String? = stringResource(Res.string.core_ui_click_here_to_view_filled_state), - title: String = stringResource(Res.string.core_ui_no_item_found), - modifier: Modifier = Modifier, -) { - MifosListingComponentOutline { - Column(modifier.fillMaxWidth()) { +fun MifosEmptyCard( + msg: String? = stringResource(Res.string.core_ui_click_here_to_view_filled_state), + title: String = stringResource(Res.string.core_ui_no_item_found), + modifier: Modifier = Modifier, +) { + MifosListingComponentOutline(modifier = modifier) { + Column(Modifier.fillMaxWidth()) {This aligns the external
modifierwith the outline container’s Box (seeMifosListingComponentOutline), which is typically where consumers expect it to apply.feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountScreen.kt (1)
74-88: Consider markingRecurringAccountDialogasinternalorprivate.The composable is declared with default (public) visibility but appears to be an implementation detail of this screen. Consider adding
internalorprivatemodifier for consistency withRecurringAccountScaffold.@Composable -fun RecurringAccountDialog( +private fun RecurringAccountDialog( state: RecurringAccountState, onAction: (RecurringAccountAction) -> Unit, ) {feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/InterestPage.kt (3)
36-37: Remove unused imports.
getValueandsetValueare imported but not used in this file.import androidx.compose.runtime.Composable -import androidx.compose.runtime.getValue -import androidx.compose.runtime.setValue import androidx.compose.ui.Modifier
113-163:RateChartshould beinternalorprivate.
RateChartis exposed as a public composable but is only used within this module. Consider restricting visibility for better encapsulation.Also, at lines 144-146, the
MifosActionsChargeListingComponenthas empty callbacks foronActionClickedandonExpandTogglewithisExpanded = false. If the component is not intended to be interactive, consider documenting this or using a simpler listing component without action capabilities.@Composable -fun RateChart( +internal fun RateChart( state: RecurringAccountState, onAction: (RecurringAccountAction) -> Unit, ) {
132-148: Potential null safety concern withforEachIndexed.The
indexparameter fromforEachIndexedis declared but never used. Consider usingforEachinstead, or remove the unused parameter.Additionally, several properties accessed from
slabcould be null:
slab.currency?.codeandslab.currency?.decimalPlaces- handledslab.description- handled with?: ""slab.annualInterestRateandslab.fromPeriod- may need null handlingslab.periodType?.value- handled- state.template.accountChart?.chartSlabs?.forEachIndexed { index, slab -> - - /** here amountRangeTo implement later because currently API not support amountRangeTo*/ + state.template.accountChart?.chartSlabs?.forEach { slab -> + // TODO: Implement amountRangeTo when API supports it MifosActionsChargeListingComponent(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
core/common/src/androidMain/kotlin/com/mifos/core/common/utils/CurrencyFormatter.android.kt(1 hunks)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosActionsListingCardComponent.kt(2 hunks)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosEmptyCard.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/InterestPage.kt(1 hunks)feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/TermsPage.kt(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosEmptyCard.kt (1)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosListingComponent.kt (1)
MifosListingComponentOutline(79-101)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/InterestPage.kt (4)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosListingComponent.kt (1)
MifosDefaultListingComponentFromStringResources(244-267)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)core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosBottomSheet.kt (1)
MifosBottomSheet(47-89)
⏰ 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 (21)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosActionsListingCardComponent.kt (2)
671-677: Conditional rendering of amount field is correctly implemented.The amount Text is now displayed only when the parameter is non-null (lines 671–677). Ensure the visual layout and spacing of the collectedOn field remain appropriate when amount is omitted.
Verify that the visual alignment of the right-aligned column (collectedOn + optional amount) displays correctly when amount is null. Check if any spacing adjustments are needed for consistent visual hierarchy.
602-615: Verify the conditional rendering and all usages ofMifosActionsChargeListingComponent.The function signature shows
amount: String? = nullwhich is backward-compatible. However, the conditional rendering logic and impact on all callers cannot be verified without repository access. Please run the verification script to confirm that:
- All usages are compatible with the nullable
amountparameter- The conditional rendering at lines 671-677 properly handles null values
- No visual layout issues occur when
amountis nullfeature/recurringDeposit/src/commonMain/composeResources/values/feature_recurring_deposit_string.xml (1)
56-65: Well-structured string additions for the interest chart feature.The nine new strings align well with the recurring deposit interest chart feature and follow the established naming convention (feature_recurring_deposit_*). The additions support the PR objective to implement an interest charts step in the account creation flow.
Minor considerations: Ensure the "View" and "Rate Chart" labels align with their UI context, and verify translations are queued for all new strings to maintain i18n coverage.
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosEmptyCard.kt (1)
30-32: API change and conditional rendering look solidMaking
msgnullable with a sensible default and introducing a customizabletitleis backward‑compatible and gives callers more flexibility. The switch totext = titleand guarding the spacer + secondary text behindif (msg != null)correctly hides that block when no message is supplied.Also applies to: 37-45
core/common/src/androidMain/kotlin/com/mifos/core/common/utils/CurrencyFormatter.android.kt (1)
24-24: Verify: Defaulting null balance to 0 may mask data issues in financial applications.In financial systems,
nulltypically indicates "not available" or "unknown," while0is a valid balance. Silently convertingnullto0could hide missing data or upstream bugs where balance information is unavailable. This concern is valid in principle for financial software.Without access to the codebase, the actual implementation context, usage patterns, and documented business requirements cannot be verified. Consider:
- Whether
nullbalances can actually occur in the current architecture- How callers of this function handle and expect null values
- Whether a default of
0aligns with your financial data contract- If documentation or tests clarify the intended null-handling behavior
If
0is the correct default, this should be explicitly documented in the function's KDoc with a clear explanation of why null balances are treated as zero rather than propagated as missing data.feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountScreen.kt (3)
38-38: LGTM!Import added for the new
RateChartcomposable which is used inRecurringAccountDialog.
61-64: LGTM!Dialog orchestration is correctly placed before the scaffold, allowing it to overlay the content. The action callback is properly forwarded to the ViewModel.
117-119: LGTM!
InterestPageinvocation updated to use the state-driven API withstateandonActionparameters, consistent with the other page composables (DetailsPage,TermsPage,SettingPage).feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/InterestPage.kt (1)
52-56: LGTM!
InterestPagesignature updated to state-driven API. This aligns with the pattern used by other page composables in this module.feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/pages/TermsPage.kt (4)
62-64: LGTM!Action type correctly updated to
RecurringAccountTermAction.OnInterestCompoundingPeriodType. The rename fromInterestChartActiontoTermActionbetter reflects these are term-related settings.
82-84: LGTM!Action type correctly updated to
RecurringAccountTermAction.OnInterestPostingPeriodType.
102-104: LGTM!Action type correctly updated to
RecurringAccountTermAction.OnInterestCalculationType.
122-124: LGTM!Action type correctly updated to
RecurringAccountTermAction.OnInterestCalculationDaysInYearType.feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt (8)
165-174: LGTM!Handler signature correctly updated to accept
RecurringAccountTermAction.OnInterestCalculationDaysInYearType.
176-184: LGTM!Handler signature correctly updated to accept
RecurringAccountTermAction.OnInterestCalculationType.
186-194: LGTM!Handler signature correctly updated to accept
RecurringAccountTermAction.OnInterestCompoundingPeriodType.
196-204: LGTM!Handler signature correctly updated to accept
RecurringAccountTermAction.OnInterestPostingPeriodType.
673-687: LGTM!Action case patterns correctly updated to match the renamed
RecurringAccountTermActionhierarchy.
689-702: LGTM!Dialog state management implemented cleanly:
OnDismissDialogclears the dialog stateOnShowRateChartDialogsets the dialog toRateChartDialogThis follows the established pattern for state management in this ViewModel.
822-823: LGTM!New action objects for dialog management are properly defined at the top level of
RecurringAccountAction.
877-889: LGTM!Sealed class renamed from
RecurringAccountInterestChartActiontoRecurringAccountTermAction. This better reflects the semantic purpose as these actions relate to term settings, not interest chart operations. All inner data classes correctly extend the renamed parent class.
| ): String { | ||
| val balanceFormatter = NumberFormat.getCurrencyInstance() | ||
| balanceFormatter.maximumFractionDigits = maximumFractionDigits ?: 0 | ||
| balanceFormatter.currency = Currency.getInstance(currencyCode) |
There was a problem hiding this comment.
Critical: Currency.getInstance() will throw if currencyCode is null.
The parameter currencyCode is nullable (line 18), but Currency.getInstance() throws IllegalArgumentException when passed null. This will cause a runtime exception if a null currency code is provided.
Consider adding a null check or defaulting to a sensible currency:
- balanceFormatter.currency = Currency.getInstance(currencyCode)
+ balanceFormatter.currency = currencyCode?.let { Currency.getInstance(it) }
+ ?: Currency.getInstance("USD") // or throw IllegalArgumentException("currencyCode cannot be null")Alternatively, if currencyCode should never be null, change the parameter to non-nullable.
📝 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.
| balanceFormatter.currency = Currency.getInstance(currencyCode) | |
| balanceFormatter.currency = currencyCode?.let { Currency.getInstance(it) } | |
| ?: Currency.getInstance("USD") // or throw IllegalArgumentException("currencyCode cannot be null") |
🤖 Prompt for AI Agents
In
core/common/src/androidMain/kotlin/com/mifos/core/common/utils/CurrencyFormatter.android.kt
around line 23, the code calls Currency.getInstance(currencyCode) while
currencyCode is declared nullable on line 18; add a null-safety guard or default
value to avoid IllegalArgumentException: either change the function parameter to
non-nullable if null should be impossible, or if null is allowed provide a
fallback (e.g., use Locale.getDefault() currency or a fixed default like "USD")
and call Currency.getInstance(nonNullCurrencyCode); ensure you handle/ log
unexpected nulls and avoid passing null to getInstance.
| <string name="feature_recurring_deposit_calculation_days_in_year">Days In Year </string> | ||
| <string name="feature_recurring_deposit_valid_from_date">Valid from Date</string> | ||
| <string name="feature_recurring_deposit_grouping_by_amount">Grouping by Amount</string> | ||
| <string name="feature_recurring_deposit_interest_rate_chart">INTEREST RATE CHART</string> |
There was a problem hiding this comment.
Inconsistent capitalization: "INTEREST RATE CHART" uses all caps.
This string uses ALL CAPS while other UI labels use Title Case or sentence case (e.g., "Valid from Date", "No Interest Rate Chart available"). Ensure consistency with your UI design system—if this is intended as a section header, consider documenting it; otherwise, align it with the existing style.
Apply this diff to match the capitalization pattern of related strings:
- <string name="feature_recurring_deposit_interest_rate_chart">INTEREST RATE CHART</string>
+ <string name="feature_recurring_deposit_interest_rate_chart">Interest Rate Chart</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.
| <string name="feature_recurring_deposit_interest_rate_chart">INTEREST RATE CHART</string> | |
| <string name="feature_recurring_deposit_interest_rate_chart">Interest Rate Chart</string> |
🤖 Prompt for AI Agents
In
feature/recurringDeposit/src/commonMain/composeResources/values/feature_recurring_deposit_string.xml
around line 58, the string value "INTEREST RATE CHART" is in ALL CAPS and should
follow the project's UI capitalization (Title Case) like related labels; change
the value to "Interest Rate Chart" (and update any corresponding localized files
or documentation if needed) so it matches the existing style for section headers
and labels.
| <string name="feature_recurring_deposit_rate_chart">Rate Chart</string> | ||
| <string name="feature_recurring_deposit_yes">Yes</string> | ||
| <string name="feature_recurring_deposit_no">No</string> | ||
| <string name="feature_recurring_deposit_empty_date">Empty Date</string> |
There was a problem hiding this comment.
Clarify the intent of "Empty Date".
The string "Empty Date" is vague and could confuse developers or translators about its intended meaning. Provide context or a more descriptive label—for example, "No Date Selected", "Date Not Set", or "Date Required" depending on the UI scenario.
Consider revising to be more explicit:
- <string name="feature_recurring_deposit_empty_date">Empty Date</string>
+ <string name="feature_recurring_deposit_empty_date">No Date Selected</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.
| <string name="feature_recurring_deposit_empty_date">Empty Date</string> | |
| <string name="feature_recurring_deposit_empty_date">No Date Selected</string> |
🤖 Prompt for AI Agents
feature/recurringDeposit/src/commonMain/composeResources/values/feature_recurring_deposit_string.xml
around line 64: the string "Empty Date" is ambiguous; replace it with a clearer,
context-appropriate phrase (e.g., "No Date Selected" if the user hasn’t chosen a
date, "Date Not Set" if the value is unset programmatically, or "Date Required"
if the field is mandatory) and update the resource name to match the intent (for
example rename to feature_recurring_deposit_no_date_selected or
feature_recurring_deposit_date_required) so translators and developers
understand the UI scenario.
| text = if (state.isRateChartEmpty) { | ||
| stringResource(Res.string.feature_recurring_deposit_interest_rate_chart) | ||
| } else { | ||
| stringResource( | ||
| Res.string.feature_recurring_deposit_no_internet_connection, | ||
| ) | ||
| }, | ||
| btnEnabled = state.isRateChartEmpty, |
There was a problem hiding this comment.
Confusing property name isRateChartEmpty - the logic appears inverted.
The property isRateChartEmpty is true when chart slabs exist (i.e., when the chart is NOT empty). This is confusing because:
- When
isRateChartEmpty = true: displays "Interest Rate Chart" and enables the button - When
isRateChartEmpty = false: displays "No Internet Connection"
Consider renaming to hasRateChartData or isRateChartAvailable in the ViewModel for clarity.
In RecurringAccountViewModel.kt, line 731:
- val isRateChartEmpty = !template.accountChart?.chartSlabs.isNullOrEmpty()
+ val hasRateChartData = !template.accountChart?.chartSlabs.isNullOrEmpty()Then update InterestPage.kt:
- text = if (state.isRateChartEmpty) {
+ text = if (state.hasRateChartData) {
stringResource(Res.string.feature_recurring_deposit_interest_rate_chart)
} else {
stringResource(
Res.string.feature_recurring_deposit_no_internet_connection,
)
},
- btnEnabled = state.isRateChartEmpty,
+ btnEnabled = state.hasRateChartData,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/pages/InterestPage.kt
around lines 92-99, the boolean property isRateChartEmpty is inverted (true when
data exists) which is confusing; rename the property in
RecurringAccountViewModel.kt (around line 731) to hasRateChartData or
isRateChartAvailable and update InterestPage.kt to use the new name, flipping
the conditional logic so that when hasRateChartData/isRateChartAvailable is true
it shows the "Interest Rate Chart" string and enables the button, and when false
it shows the "No Internet Connection" string and disables the button; also
update all other references to the old property name across the module to the
new name to keep usage consistent.
| val dialogState: DialogState? = null, | ||
| ) { | ||
| sealed interface ScreenState { | ||
| data class Error(val message: String) : ScreenState | ||
| data object Loading : ScreenState | ||
| data object Success : ScreenState | ||
| } | ||
| sealed interface DialogState { | ||
| data object RateChartDialog : DialogState | ||
| } | ||
|
|
||
| val isRateChartEmpty = !template.accountChart?.chartSlabs.isNullOrEmpty() |
There was a problem hiding this comment.
Consider renaming isRateChartEmpty for clarity.
The property name isRateChartEmpty is misleading - it returns true when chart slabs exist (i.e., NOT empty). This inverted naming can cause confusion at call sites.
Recommend renaming to hasRateChartData or isRateChartAvailable.
- val isRateChartEmpty = !template.accountChart?.chartSlabs.isNullOrEmpty()
+ val hasRateChartData = !template.accountChart?.chartSlabs.isNullOrEmpty()📝 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 dialogState: DialogState? = null, | |
| ) { | |
| sealed interface ScreenState { | |
| data class Error(val message: String) : ScreenState | |
| data object Loading : ScreenState | |
| data object Success : ScreenState | |
| } | |
| sealed interface DialogState { | |
| data object RateChartDialog : DialogState | |
| } | |
| val isRateChartEmpty = !template.accountChart?.chartSlabs.isNullOrEmpty() | |
| val dialogState: DialogState? = null, | |
| ) { | |
| sealed interface ScreenState { | |
| data class Error(val message: String) : ScreenState | |
| data object Loading : ScreenState | |
| data object Success : ScreenState | |
| } | |
| sealed interface DialogState { | |
| data object RateChartDialog : DialogState | |
| } | |
| val hasRateChartData = !template.accountChart?.chartSlabs.isNullOrEmpty() |
🤖 Prompt for AI Agents
In
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/newRecurringDepositAccount/RecurringAccountViewModel.kt
around lines 720-731, the property isRateChartEmpty is named incorrectly (it
evaluates true when chart slabs exist). Rename it to hasRateChartData (or
isRateChartAvailable) and change its initializer to a non-nullable, clear
boolean expression such as template.accountChart?.chartSlabs?.isNullOrEmpty() ==
false (or use !template.accountChart?.chartSlabs.isNullOrEmpty() with safe-call
on chartSlabs), then update all call sites to use the new name.
2 participants
Fixes - Jira-#553
Summary by CodeRabbit
New Features
Improvements
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.