Show Basic Options for All Savings Accounts #2983
Conversation
|
@WizCoderr I am not getting what are you trying to do. The options which you are showing on the drop-down list are already present on the screen under |
|
Now i get it, Will be ready tonightr |
WalkthroughAdds a @serializable SavingStatus enum with parser and allowed-actions mapping; updates savings-account ViewModel and Screen to use SavingStatus, adds savingStatus to state, changes actions flow and navigation events, removes SavingsAccountDialogs, and applies a minor strings.xml formatting change. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Screen as SavingsAccountDetailsScreen
participant VM as SavingsAccountDetailsViewModel
participant Enum as SavingStatus
User->>Screen: Open account details
Screen->>VM: loadAccountDetails()
VM->>Enum: SavingStatus.fromStatus(statusString)
Enum-->>VM: SavingStatus
VM->>VM: compute allowedActions = savingStatus.allowedActions
VM-->>Screen: emit state (savingStatus, allowedActions, transactions)
Screen->>Screen: visibleActions = state.savingStatus?.allowedActions
Screen->>Screen: render SavingsAccountActions(visibleActions)
User->>Screen: Tap action (Transactions / Charges / QR)
Screen->>VM: send event ShowTransactions | ShowCharges | ShowQrCode
VM-->>Screen: navigate to corresponding destination
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
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 |
Screen_recording_20251024_211911.webm |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsViewModel.kt (2)
21-21: Use Ktor’s multiplatform IOException, not kotlinx.io.The network error check won’t match Ktor exceptions otherwise.
- import kotlinx.io.IOException + import io.ktor.utils.io.errors.IOException
275-289: Use a null-safe mapper; don’t crash on status parsing.fromStatus throws; prefer a tolerant mapping and handle null.
- savingStatus = SavingStatus.fromStatus(savings.status?.value ?: ""), + savingStatus = SavingStatus.fromStatusOrNull(savings.status?.value),Also guard uses with
?.as already done (?.allowedActions).
🧹 Nitpick comments (7)
core/model/src/commonMain/kotlin/org/mifos/mobile/core/model/SavingStatus.kt (1)
4-9: License link likely points to wrong repo path.URL shows “mobile-mobile”; project appears to be “mifos-mobile”. Please align.
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsScreen.kt (4)
105-115: Avoid event duplication; use one navigation path.ShowTransactions/ShowCharges/ShowQrCode are handled here but never emitted; OnNavigateToAction already navigates. Remove these branches or switch UI to dispatch the Show* events consistently.
182-188: Externalize “Last Transactions” label.Hard-coded string blocks i18n. Add a resource and use it here.
Example:
- AccountDetailsGrid( - label = "Last Transactions", + AccountDetailsGrid( + label = stringResource(Res.string.feature_savings_last_transactions_label), details = state.transactionList, isActive = state.isActive, )Add to strings.xml:
+ <string name="feature_savings_last_transactions_label">Last Transactions</string>
190-197: Memoize visibleActions to avoid churn; consider stable ordering.Compute once per status and (optionally) switch to List for deterministic order.
- val visibleActions = state.savingStatus?.allowedActions ?: emptySet() + val visibleActions = remember(state.savingStatus) { + state.savingStatus?.allowedActions ?: emptySet() + }Optional: change types to List if order matters in UI.
295-313: Use string resource for heading and remove println.Replace literal “Actions” with the new title string and drop debug output.
internal fun SavingsAccountActions( visibleActions: Set<SavingsActionItems>, onActionClick: (String) -> Unit, ) { @@ - Text( - text = "Actions", + Text( + text = stringResource(Res.string.feature_savings_account_options_title), style = MifosTypography.labelLargeEmphasized, color = MaterialTheme.colorScheme.onSurface, ) @@ - println("Visible Actions: $visibleActions") - visibleActions.forEach { item -> + visibleActions.forEach { item -> MifosActionCard(feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsViewModel.kt (2)
331-351: Consider List for deterministic UI order; verify CLOSED actions.Sets don’t guarantee order conceptually; a List communicates order and avoids surprises. Also, confirm that Transfer is allowed for CLOSED accounts.
Example:
-val SavingStatus.allowedActions: Set<SavingsActionItems> - get() = when (this) { +val SavingStatus.allowedActions: List<SavingsActionItems> + get() = when (this) { - SavingStatus.ACTIVE -> setOf( + SavingStatus.ACTIVE -> listOf( SavingsActionItems.Transactions, SavingsActionItems.Charges, SavingsActionItems.QrCode, SavingsActionItems.Transfer, ) - SavingStatus.INACTIVE -> setOf( + SavingStatus.INACTIVE -> listOf( SavingsActionItems.Transfer, SavingsActionItems.QrCode, ) - SavingStatus.CLOSED -> setOf( + SavingStatus.CLOSED -> listOf( SavingsActionItems.QrCode, - SavingsActionItems.Transfer, - SavingsActionItems.Transactions, + // Verify these two: + // SavingsActionItems.Transfer, + // SavingsActionItems.Transactions, ) - SavingStatus.SUBMIT_AND_PENDING_APPROVAL -> setOf( + SavingStatus.SUBMIT_AND_PENDING_APPROVAL -> listOf( SavingsActionItems.QrCode, ) }
366-371: Show events are defined but not used; remove or adopt consistently.*Currently navigation uses NavigateToAction(route). Either dispatch and handle Show* events end-to-end or drop them.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/model/src/commonMain/kotlin/org/mifos/mobile/core/model/SavingStatus.kt(1 hunks)feature/savings-account/src/commonMain/composeResources/values/strings.xml(1 hunks)feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsScreen.kt(4 hunks)feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsViewModel.kt(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsScreen.kt (2)
feature/client-charge/src/commonMain/kotlin/org/mifos/mobile/feature/charge/charges/ClientChargeRoute.kt (1)
navigateToClientChargeScreen(38-43)feature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/loanAccountDetails/LoanAccountDetailsScreen.kt (2)
AccountDetailsGrid(197-239)SavingsAccountActions(241-268)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsViewModel.kt (1)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccount/SavingsAccountViewmodel.kt (1)
updateState(107-109)
core/model/src/commonMain/kotlin/org/mifos/mobile/core/model/SavingStatus.kt
Show resolved
Hide resolved
core/model/src/commonMain/kotlin/org/mifos/mobile/core/model/SavingStatus.kt
Show resolved
Hide resolved
feature/savings-account/src/commonMain/composeResources/values/strings.xml
Outdated
Show resolved
Hide resolved
.../mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsViewModel.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsViewModel.kt (4)
16-16: Remove stray import.io.ktor.client.utils.EmptyContent.status is unused and misleading. Drop it.
-import io.ktor.client.utils.EmptyContent.status
262-273: Format transaction amounts with CurrencyFormatter.Keep display consistent with account figures.
- LabelValueItem( - Res.string.feature_savings_transaction_amount_label, - txn.amount.toString(), - ), + LabelValueItem( + Res.string.feature_savings_transaction_amount_label, + CurrencyFormatter.format(txn.amount, currencyCode, decimalPlaces), + ),
212-228: Avoid "null" literals in QR payload.Use orEmpty() for nullable fields.
- clientName = state.clientName.toString(), - accountNumber = state.accountNumber.toString(), + clientName = state.clientName.orEmpty(), + accountNumber = state.accountNumber.orEmpty(),
331-351: Validate allowed actions per status.
- CLOSED/INACTIVE permitting Transfer seems questionable; confirm product rules.
- ACTIVE omits Deposit/Withdraw; intended?
If order matters, prefer List over Set or sort before rendering.
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsScreen.kt (3)
105-116: Remove unreachable Show event handlers.*Navigation is already covered by OnNavigateToAction; these branches are dead.
- SavingsAccountDetailsEvent.ShowTransactions -> { - navigateToSavingsAccountTransactionScreen(uiState.accountId) - } - - SavingsAccountDetailsEvent.ShowCharges -> { - navigateToClientChargeScreen(ChargeType.SAVINGS.name, uiState.accountId) - } - - SavingsAccountDetailsEvent.ShowQrCode -> { - navigateToQrCodeScreen(viewModel.getQrString()) - }
182-189: Localize section title; avoid hardcoded text.Use a string resource (e.g., feature_transaction_info_title) instead of "Last Transactions".
- AccountDetailsGrid( - label = "Last Transactions", + AccountDetailsGrid( + label = stringResource(Res.string.feature_transaction_info_title),
297-324: Use string resource for title; drop debug println.
- Replace "Actions" with feature_savings_account_options_title.
- Remove println to keep UI pure.
- Text( - text = "Actions", + Text( + text = stringResource(Res.string.feature_savings_account_options_title), @@ - println("Visible Actions: $visibleActions") - visibleActions.forEach { item -> + visibleActions.forEach { item ->core/model/src/commonMain/kotlin/org/mifos/mobile/core/model/SavingStatus.kt (1)
14-29: Expand enum coverage and add a safe lookup.Enum lacks likely backend statuses (e.g., Approved, Matured). fromStatus currently throws, causing crashes upstream.
Add entries and provide fromStatusOrNull:
@Serializable enum class SavingStatus(val status: String) { ACTIVE("Active"), INACTIVE("Inactive"), CLOSED("Closed"), SUBMIT_AND_PENDING_APPROVAL("Submitted and pending approval"), + APPROVED("Approved"), + MATURED("Matured"), + SUBMITTED("Submitted"), ; companion object { - fun fromStatus(status: String): SavingStatus { + fun fromStatus(status: String): SavingStatus { return entries.find { it.status.equals(status, ignoreCase = true) } ?: throw IllegalArgumentException("Invalid status: $status") } + fun fromStatusOrNull(status: String?): SavingStatus? = + status?.let { s -> entries.find { it.status.equals(s, ignoreCase = true) } } } }Follow-up: update ViewModel to use fromStatusOrNull as proposed in its diff.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/model/src/commonMain/kotlin/org/mifos/mobile/core/model/SavingStatus.kt(1 hunks)feature/savings-account/src/commonMain/composeResources/values/strings.xml(1 hunks)feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsScreen.kt(4 hunks)feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsViewModel.kt(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsScreen.kt (2)
feature/client-charge/src/commonMain/kotlin/org/mifos/mobile/feature/charge/charges/ClientChargeRoute.kt (1)
navigateToClientChargeScreen(38-43)feature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/loanAccountDetails/LoanAccountDetailsScreen.kt (2)
AccountDetailsGrid(197-239)SavingsAccountActions(241-268)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsViewModel.kt (1)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccount/SavingsAccountViewmodel.kt (1)
updateState(107-109)
⏰ 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 KMP / Static Analysis Check
🔇 Additional comments (1)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsScreen.kt (1)
190-197: Depends on safe status parsing.visibleActions is fine if savingStatus parsing can’t throw (see ViewModel fix).
feature/savings-account/src/commonMain/composeResources/values/strings.xml
Outdated
Show resolved
Hide resolved
.../mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsViewModel.kt
Show resolved
Hide resolved
...org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsScreen.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsScreen.kt (1)
1-60: Remove debug println statement at line 305.The string resource and component imports (StringResource, SavingsActionItems, savingsAccountActions) are correctly imported and actively used. However, the debug
println("Visible Actions: $visibleActions")statement at line 305 should be removed before merging.
🧹 Nitpick comments (7)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsScreen.kt (7)
84-95: Simplify route dispatch for readability.Use when(event.route) instead of chained boolean checks.
- is SavingsAccountDetailsEvent.NavigateToAction -> { - when { - event.route == Constants.CHARGES -> { + is SavingsAccountDetailsEvent.NavigateToAction -> when (event.route) { + Constants.CHARGES -> { navigateToClientChargeScreen(ChargeType.SAVINGS.name, uiState.accountId) - } - - event.route == Constants.TRANSFER -> { + } + Constants.TRANSFER -> { navigateToTransferScreen(uiState.accountId) - } - - event.route == Constants.TRANSACTIONS -> { + } + Constants.TRANSACTIONS -> { navigateToSavingsAccountTransactionScreen(uiState.accountId) - } - - event.route == Constants.QR_CODE -> { + } + Constants.QR_CODE -> { navigateToQrCodeScreen(viewModel.getQrString()) - } - } - } + } + else -> Unit + }
107-109: Remove no‑op else branch.The empty else adds noise and can hide new events. Drop it.
- else -> { - - } + // no else — keep when over events explicit
176-182: Localize “Last Transactions” and avoid passing unused isActive.Use a string resource; isActive doesn’t affect transaction chips.
- if (state.transactionList.isNotEmpty()) { - AccountDetailsGrid( - label = "Last Transactions", - details = state.transactionList, - isActive = state.isActive, - ) - } + if (state.transactionList.isNotEmpty()) { + AccountDetailsGrid( + label = stringResource(Res.string.feature_account_last_transactions_label), + details = state.transactionList, + ) + }Note: add import for Res.string.feature_account_last_transactions_label.
184-191: Derive and pass deterministic action order.Sets don’t guarantee UI order. Either sort or filter a canonical list to preserve stable ordering.
- val visibleActions = state.savingStatus?.allowedActions ?: emptySet() + val visibleActions = state.savingStatus?.allowedActions ?: emptySet() + // Preserve a stable ordering based on the canonical list + val orderedActions = remember(visibleActions) { + savingsAccountActions.filter { it in visibleActions }.toSet() + } ... - SavingsAccountActions( - visibleActions = visibleActions, + SavingsAccountActions( + visibleActions = orderedActions, onActionClick = { onAction(SavingsAccountDetailsAction.OnNavigateToAction(it)) }, )
291-307: i18n + remove debug print + deterministic iteration.
- Localize “Actions”.
- Don’t println in a composable.
- Iterate in a stable order.
@Composable internal fun SavingsAccountActions( visibleActions: Set<SavingsActionItems>, onActionClick: (String) -> Unit, ) { Column( verticalArrangement = Arrangement.spacedBy(DesignToken.spacing.large), ) { Text( - text = "Actions", + text = stringResource(Res.string.feature_account_details_action), style = MifosTypography.labelLargeEmphasized, color = MaterialTheme.colorScheme.onSurface, ) FlowRow( modifier = Modifier.fillMaxWidth(), ) { - println("Visible Actions: $visibleActions") - visibleActions.forEach { item -> + val ordered = remember(visibleActions) { + savingsAccountActions.filter { it in visibleActions } + } + ordered.forEach { item -> MifosActionCard( title = item.title, subTitle = item.subTitle, icon = item.icon, onClick = { onActionClick(item.route) }, ) } } } }Note: add import for Res.string.feature_account_details_action if missing.
325-337: Preview currently renders no actions.visibleActions is derived from savingStatus.allowedActions; the preview state doesn’t set it, so the list is empty. Provide a sample savingStatus (e.g., ACTIVE) or a stub allowedActions to visualize cards.
Would you like a follow‑up patch to wire a preview‑only state with allowedActions populated?
291-293: API naming nit.“SavingsAccountActions.visibleActions” is fine, but consider “actions” to avoid leaking visibility concerns into the UI layer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsScreen.kt(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsScreen.kt (1)
feature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/loanAccountDetails/LoanAccountDetailsScreen.kt (2)
AccountDetailsGrid(197-239)SavingsAccountActions(241-268)
🔇 Additional comments (2)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsScreen.kt (2)
78-106: Functional sanity check.Navigation routes look correct (Charges/Transfer/Transactions/QR) and error/loading/network states remain intact.
If the dropdown keeps the same items shown in “Actions”, ensure one of them is removed to avoid redundancy in UX.
Also applies to: 140-158
240-287: Status highlight condition OK.Coloring only the “Status” label when isActive is true matches intent; no issues.
.../mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsViewModel.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsViewModel.kt (2)
16-16: Remove invalid import (breaks compile).This import does not exist and will cause compilation failure. Simply delete this line.
Apply this diff:
-import io.ktor.client.utils.EmptyContent.status
231-232: Crash risk: strict fromStatus() without null-safe handling.
SavingStatus.fromStatus(...)likely throws on unknown or empty status values. The backend may return statuses not covered by the enum (e.g., "Approved", "Matured"), causing a crash.Additionally, derive
isActiveandisUpdatefrom the parsed enum rather than comparing raw strings.Create a
fromStatusOrNullhelper in SavingStatus:fun fromStatusOrNull(status: String?): SavingStatus? = try { if (status.isNullOrEmpty()) null else fromStatus(status) } catch (e: Exception) { null }Then apply this diff:
- val isActive = savings.status?.value == SavingStatus.ACTIVE.status - val isUpdate = savings.status?.value == SavingStatus.SUBMIT_AND_PENDING_APPROVAL.status + val parsedStatus = savings.status?.value?.let(SavingStatus::fromStatusOrNull) + val isActive = parsedStatus == SavingStatus.ACTIVE + val isUpdate = parsedStatus == SavingStatus.SUBMIT_AND_PENDING_APPROVAL @@ - savingStatus = SavingStatus.fromStatus(savings.status?.value ?: ""), + savingStatus = parsedStatus,Also applies to: 277-277
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsScreen.kt(4 hunks)feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsViewModel.kt(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsViewModel.kt (1)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccount/SavingsAccountViewmodel.kt (1)
updateState(107-109)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsScreen.kt (1)
feature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/loanAccountDetails/LoanAccountDetailsScreen.kt (2)
AccountDetailsGrid(197-239)SavingsAccountActions(241-268)
🔇 Additional comments (4)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsViewModel.kt (2)
130-132: LGTM: Clean state update helper.The
updateStatehelper provides good encapsulation for state mutations, consistent with patterns used elsewhere in the codebase.
331-351: The Transactions action on CLOSED accounts is actually appropriate—reconsider the other allowed actions instead.Viewing and downloading transaction history for closed accounts is explicitly recommended for closed savings accounts. The
Transactionsaction in the codebase routes to a transaction history view, making it the correct action to include.However, the
allowedActionsmapping for CLOSED accounts includesTransferandQrCode, which warrant closer review:
- Transfer typically enables fund movements that may risk inadvertent account reopening
- QrCode typically enables deposit reception, which is inappropriate for closed accounts
Consider verifying whether these two actions should be restricted for CLOSED status accounts per your business requirements.
Likely an incorrect or invalid review comment.
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsScreen.kt (2)
173-179: Good improvement: Show transactions based on data availability.Changing from
state.isActivetostate.transactionList.isNotEmpty()is more user-friendly. Users can now view transaction history regardless of account status, which provides better transparency.
181-188: Status-driven actions look good, but handle null savingStatus.The new visibleActions flow correctly implements status-based action filtering. However, if
state.savingStatusisnull(e.g., due to parsing failure in the ViewModel), no actions will be displayed.Ensure the ViewModel's
fromStatuscrash risk is addressed (see ViewModel review comments) to prevent a silent no-actions state.
...org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsScreen.kt
Outdated
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 (2)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsScreen.kt (2)
294-295: Use string resource for "Actions" label.The text "Actions" is hardcoded and should use a string resource for proper internationalization support.
Looking at the imports, there's a similar resource
feature_account_details_action(line 37). Verify if this or a similar resource should be used here:Text( - text = "Actions", + text = stringResource(Res.string.feature_account_details_action), style = MifosTypography.labelLargeEmphasized, color = MaterialTheme.colorScheme.onSurface, )
326-332: Update preview to match current state structure.The preview function references
itemsanddialogStateproperties inSavingsAccountDetailsState, but based on the AI summary,SavingsAccountDialogswas removed and the state structure has changed to usesavingStatusand visibility-driven actions.Update the preview to use the current state structure, or verify that these properties still exist. Based on the changes, the preview likely needs to be updated to properly demonstrate the new functionality.
🧹 Nitpick comments (1)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsScreen.kt (1)
287-288: Consider action display order consistency.The parameter type changed from
ImmutableList<SavingsActionItems>toSet<SavingsActionItems>. While this aligns with the pattern used inLoanAccountDetailsScreen.kt(lines 240-267 in the relevant snippets), sets don't guarantee iteration order, which could make the action cards appear in unpredictable order across renders or different accounts.If action display order matters for UX consistency, consider either:
- Keeping the Set but converting it to a sorted list before iteration
- Reverting to List type to maintain deterministic ordering
For example, to sort before rendering:
internal fun SavingsAccountActions( visibleActions: Set<SavingsActionItems>, onActionClick: (String) -> Unit, ) { Column( verticalArrangement = Arrangement.spacedBy(DesignToken.spacing.large), ) { Text( text = "Actions", style = MifosTypography.labelLargeEmphasized, color = MaterialTheme.colorScheme.onSurface, ) FlowRow( modifier = Modifier.fillMaxWidth(), ) { - visibleActions.forEach { item -> + visibleActions.sortedBy { it.title }.forEach { item -> MifosActionCard( title = item.title, subTitle = item.subTitle, icon = item.icon, onClick = { onActionClick(item.route) }, ) } } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsScreen.kt(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsScreen.kt (1)
feature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/loanAccountDetails/LoanAccountDetailsScreen.kt (2)
AccountDetailsGrid(197-239)SavingsAccountActions(241-268)
🔇 Additional comments (1)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsScreen.kt (1)
173-179: Unable to verify the claimed behavior change—original code condition not accessible.The review comment assumes the transaction display condition was previously
state.isActive, but this cannot be verified in the current codebase. Codebase inspection shows only the current state (state.transactionList.isNotEmpty()), with no evidence of the prior condition. TheisActivefield is currently used only for styling inside theAccountDetailsGridcomposable, not for visibility gating.To properly verify this concern, manual review of the git diff or PR history is required to confirm what condition was actually replaced. The current implementation is semantically reasonable (display transactions when they exist), but the behavior change itself cannot be confirmed.
...org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsScreen.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsScreen.kt (1)
294-295: Use string resource for "Actions" text.The hardcoded "Actions" string should be replaced with a string resource for proper internationalization support. Based on the relevant code snippet from
LoanAccountDetailsScreen.kt, the resourceRes.string.feature_account_details_actionis already available.Apply this diff:
Text( - text = "Actions", + text = stringResource(Res.string.feature_account_details_action), style = MifosTypography.labelLargeEmphasized, color = MaterialTheme.colorScheme.onSurface, )
♻️ Duplicate comments (1)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsScreen.kt (1)
181-188: Unaddressed issue: Empty Actions section can still render.The past review comment on lines 181-188 identified that when
visibleActionsis empty, theSavingsAccountActionscomposable will still render the "Actions" header with no action cards beneath it, creating a confusing empty section. This issue was not addressed.Apply this diff to conditionally render the Actions section only when actions exist:
val visibleActions = state.savingStatus?.allowedActions ?: emptySet() - - SavingsAccountActions( - visibleActions = visibleActions, - onActionClick = { - onAction(SavingsAccountDetailsAction.OnNavigateToAction(it)) - }, - ) + + if (visibleActions.isNotEmpty()) { + SavingsAccountActions( + visibleActions = visibleActions, + onActionClick = { + onAction(SavingsAccountDetailsAction.OnNavigateToAction(it)) + }, + ) + }
🧹 Nitpick comments (1)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsScreen.kt (1)
83-91: Remove unnecessary blank lines.The blank lines at 83, 87, and 91 between the
whenbranches are inconsistent with the formatting of the CHARGES branch above and add no value.Apply this diff:
event.route == Constants.CHARGES -> { navigateToClientChargeScreen(ChargeType.SAVINGS.name, uiState.accountId) } - event.route == Constants.TRANSFER -> { navigateToTransferScreen(uiState.accountId) } - event.route == Constants.TRANSACTIONS -> { navigateToSavingsAccountTransactionScreen(uiState.accountId) } - event.route == Constants.QR_CODE -> { navigateToQrCodeScreen(viewModel.getQrString()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsScreen.kt(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsScreen.kt (1)
feature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/loanAccountDetails/LoanAccountDetailsScreen.kt (2)
AccountDetailsGrid(197-239)SavingsAccountActions(241-268)
🔇 Additional comments (1)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsScreen.kt (1)
173-179: Good improvement to transaction visibility logic.Conditionally rendering the "Last Transactions" section based on
transactionList.isNotEmpty()rather thanisActiveis more accurate and provides a better user experience.
3 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.
WhatsApp.Video.2025-10-19.at.4.18.24.PM.mp4
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.
WhatsApp.Video.2025-10-19.at.4.18.24.PM.mp4
Summary by CodeRabbit
New Features
Refactor