feat(client): add transfer fund option for overpaid loan accounts - #2675
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds a "Transfer Fund" action and icon, wires it through client loan listings and ViewModel events to a simplified transfer route that accepts only a loan/account ID, updates AmountTransferViewModel to fetch loan details on init, and makes AccountTransferRequest fields nullable. Changes (summary)Transfer Fund Feature Integration
Sequence Diagram — high-level transfer flowsequenceDiagram
participant UI as ClientLoanAccountsScreen
participant VM as ClientLoanAccountsViewModel
participant Nav as NavController
participant TransferVM as AmountTransferViewModel
participant LoanRepo as LoanAccountSummaryRepository
UI->>VM: Actions.TransferFund(loanId)
VM-->>UI: ClientLoanAccountsEvent.TransferFund(loanId)
UI->>Nav: navigateToTransferFund(loanId)
Nav->>TransferVM: navigate with AmountTransferScreenRoute(fromAccountId)
TransferVM->>LoanRepo: getLoanById(fromAccountId)
LoanRepo-->>TransferVM: loan details
TransferVM->>TransferVM: update state, fetch templates, submit transfer
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 warning, 1 inconclusive)
✅ 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 |
biplab1
reviewed
May 6, 2026
biplab1
reviewed
May 6, 2026
biplab1
reviewed
May 6, 2026
biplab1
reviewed
May 6, 2026
biplab1
reviewed
May 6, 2026
biplab1
reviewed
May 6, 2026
biplab1
reviewed
May 6, 2026
biplab1
reviewed
May 7, 2026
…ViewModel to handle nullability
Member
Author
|
@biplab1 review this one I have done the changes |
![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 (3)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferViewModel.kt (3)
184-207:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
fetchClientDetails(null)silently nulls out previously-populated source-account state.When
clientIdis null (e.g., the loan API returned a payload withoutclientId, orOnRetryFetchingis dispatched before the loan details loaded — see Line 141),clientId?.let { clientRepo.getClient(it) }short-circuits toclient = null, but themutableStateFlow.updateblock still runs and writesfromOfficeId = null,fromClientName = null,fromOfficeName = null. This destroys any state set byfetchLoanAccountDetailsand leaves the UI showing a "loaded" screen with empty source fields, then proceeds to callfetchInitialTemplatewith insufficient state.Guard the update so a null
clientIdresults in either an explicit error dialog or simply skips the overwrite + template fetch.🛡️ Proposed fix
private fun fetchClientDetails(clientId: Int?) { + if (clientId == null) { + mutableStateFlow.update { + it.copy( + dialogState = AmountTransferUiState.DialogState.FetchingFailed( + "Missing client information for source account", + ), + ) + } + return + } mutableStateFlow.update { it.copy(dialogState = AmountTransferUiState.DialogState.Loading) } viewModelScope.launch { try { - val client = clientId?.let { clientRepo.getClient(it) } + val client = clientRepo.getClient(clientId) mutableStateFlow.update { it.copy( - fromOfficeId = client?.officeId, - fromClientName = client?.displayName, - fromOfficeName = client?.officeName, + fromOfficeId = client.officeId, + fromClientName = client.displayName, + fromOfficeName = client.officeName, ) }Based on learnings: when a Kotlin function parameter is nullable and downstream calls require a non-null value, add null-safety handling and ensure consistent null handling across all callers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferViewModel.kt` around lines 184 - 207, fetchClientDetails currently overwrites source-account state with null when clientId is null; change fetchClientDetails to early-handle a null clientId: if clientId is null either update mutableStateFlow to set dialogState to a FetchingFailed (or appropriate error) and return, or simply return without calling mutableStateFlow.update or fetchInitialTemplate so existing fromOfficeId/fromClientName/fromOfficeName are preserved; locate the logic inside fetchClientDetails (the clientId?.let { clientRepo.getClient(it) } call, the following mutableStateFlow.update block, and the fetchInitialTemplate invocation) and add the null-guard before calling clientRepo.getClient or updating state so downstream code only runs with a non-null client and consistent behavior with fetchLoanAccountDetails/OnRetryFetching.
209-249:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStuck
Loadingdialog whenstate.fromAccountTypeis null.
fetchClientDetailssetsdialogState = Loading(Line 185), then invokesfetchInitialTemplate. Here, ifstate.fromAccountTypeis null, the chainstate.fromAccountType?.let { ... }?.collect { ... }short-circuits tonulland thewhenblock is never executed, so thedialogStateis never cleared and the UI is stuck on the loading spinner forever. This is reachable any time the loan-account payload lacksloanType.id.Same risk exists in
fetchTemplateWithDependencies(Lines 258–273): the loading state is set first, but the?.collectis conditional on bothfromClientIdandfromAccountTypebeing non-null.Either reset the dialog state in the null branch, or restructure with an early return.
🛡️ Proposed fix (fetchInitialTemplate)
private fun fetchInitialTemplate() { viewModelScope.launch { - state.fromAccountType?.let { fromAccountType -> - repository.getAccountTransferTemplate( - fromClientId = state.fromClientId ?: 0, - fromAccountType = fromAccountType, - fromAccountId = route.fromAccountId, - fromOfficeId = state.fromOfficeId, - ) - }?.collect { dataState -> + val fromAccountType = state.fromAccountType + val fromClientId = state.fromClientId + if (fromAccountType == null || fromClientId == null) { + mutableStateFlow.update { it.copy(dialogState = null) } + return@launch + } + repository.getAccountTransferTemplate( + fromClientId = fromClientId, + fromAccountType = fromAccountType, + fromAccountId = route.fromAccountId, + fromOfficeId = state.fromOfficeId, + ).collect { dataState -> ... } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferViewModel.kt` around lines 209 - 249, fetchInitialTemplate (and likewise fetchTemplateWithDependencies) sets dialogState = Loading then uses a nullable flow chain (state.fromAccountType?.let { ... }?.collect { ... }) so when fromAccountType or fromClientId is null the collect never runs and the Loading dialog is never cleared; fix by checking required fields before setting Loading (early return) or explicitly handling the null branch to reset dialogState via mutableStateFlow.update(... copy(dialogState = null)) — update fetchInitialTemplate and fetchTemplateWithDependencies to validate state.fromAccountType and state.fromClientId up front and either return before launching the Loading state or clear dialogState when the nullable let returns null so the UI is not stuck.
251-313:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSame stuck-loading risk and inconsistent state capture in
fetchTemplateWithDependencies.Two issues here:
- Stuck Loading: Lines 258–260 set
dialogState = Loading, but Lines 262–273 wrap the call instate.fromClientId?.let { state.fromAccountType?.let { ... } }?.collect. If either is null (very plausible when the user changes office while loan details are still in flight), the Loading state never clears.- Inconsistent state snapshotting: Line 252 captures
currentState, but Lines 262–268 then read the livestate.fromClientId,state.fromAccountType, andstate.fromOfficeIdwhile reading the snapshottedcurrentState.selectedOfficeId/selectedClientId/accountTypeId. Mixing the two during async work makes the eventual request payload non-deterministic if the user changes the form mid-flight. Pick one (preferablycurrentState).🛡️ Sketch
private fun fetchTemplateWithDependencies() { val currentState = state - if (currentState.selectedOfficeId == null) return + val fromClientId = currentState.fromClientId + val fromAccountType = currentState.fromAccountType + if (currentState.selectedOfficeId == null || fromClientId == null || fromAccountType == null) return viewModelScope.launch { mutableStateFlow.update { it.copy(dialogState = AmountTransferUiState.DialogState.Loading) } - state.fromClientId?.let { fromClientId -> - state.fromAccountType?.let { fromAccountType -> - repository.getAccountTransferTemplate( - fromClientId = fromClientId, - fromAccountType = fromAccountType, - fromAccountId = route.fromAccountId, - fromOfficeId = state.fromOfficeId, - toOfficeId = currentState.selectedOfficeId, - toClientId = currentState.selectedClientId, - toAccountType = currentState.accountTypeId, - ) - } - }?.collect { dataState -> + repository.getAccountTransferTemplate( + fromClientId = fromClientId, + fromAccountType = fromAccountType, + fromAccountId = route.fromAccountId, + fromOfficeId = currentState.fromOfficeId, + toOfficeId = currentState.selectedOfficeId, + toClientId = currentState.selectedClientId, + toAccountType = currentState.accountTypeId, + ).collect { dataState -> ... } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferViewModel.kt` around lines 251 - 313, fetchTemplateWithDependencies sets dialogState = Loading before checking nullable inputs and mixes a snapshot (currentState.selectedOfficeId) with live state fields, causing stuck-loading and racey requests; fix by snapshotting all needed fields from state into local vals (e.g., capture selectedOfficeId, selectedClientId, accountTypeId, fromClientId, fromAccountType, fromOfficeId) before launching, check those captured vals for nulls and only set dialogState = Loading when all required inputs are present, then call repository.getAccountTransferTemplate with the captured values and collect; if required inputs are missing, update mutableStateFlow to clear Loading / set an appropriate FetchingFailed instead of leaving Loading indefinitely.
♻️ Duplicate comments (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt (1)
255-258:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid dispatching transfer with invalid fallback IDs.
At Line [257],
loan.id ?: -1can emitTransferFundwith-1, which may navigate into a broken transfer flow for records missing IDs. Only dispatch whenloan.idis non-null (and preferably positive).Suggested fix
menuList = buildList { add( Actions.ViewAccount( vectorResource(Res.drawable.wallet), ), ) when { loan.status?.active == true -> add( Actions.MakeRepayment( vectorResource(Res.drawable.cash_bundel), ), ) - loan.status?.overpaid == true -> add( + loan.status?.overpaid == true && loan.id != null -> add( Actions.TransferFund(), ) } }, onActionClicked = { actions -> when (actions) { @@ - is Actions.TransferFund -> onAction( - ClientLoanAccountsAction.TransferFund( - loanId = loan.id ?: -1, - ), - ) + is Actions.TransferFund -> { + loan.id?.let { validLoanId -> + onAction( + ClientLoanAccountsAction.TransferFund( + loanId = validLoanId, + ), + ) + } + } else -> null } },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt` around lines 255 - 258, The code dispatches ClientLoanAccountsAction.TransferFund using a fallback id of -1 (loan.id ?: -1) which can start a broken flow; update the onAction invocation in ClientLoanAccountsScreen so it only dispatches ClientLoanAccountsAction.TransferFund when loan.id is non-null and positive (e.g., check loan.id != null && loan.id > 0) and otherwise skip dispatch or handle the missing-id case (show error/disable action) so Actions.TransferFund is never created with an invalid id.
🧹 Nitpick comments (2)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileScreen.kt (1)
88-109: 💤 Low valueRedundant
accountre-declaration inside theTransferbranch.
accountis already safely unwrapped at line 99; re-declaring it at line 105 shadows the outer binding unnecessarily and is inconsistent with howApproveandRepaymentbranches consume the same variable.♻️ Proposed cleanup
LoanProfileAction.Transfer -> { - val account = state.loanAccount ?: return@EventsEffect navigateToTransferScreen( account.id, ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileScreen.kt` around lines 88 - 109, Redundant local `account` is re-declared inside the LoanProfileAction.Transfer branch of LoanAccountProfileScreen's EventsEffect; remove the inner declaration and use the already-unwrapped `account` from before the when so Transfer calls navigateToTransferScreen(account.id) directly, keeping the existing safe-return (return@EventsEffect) behavior if state.loanAccount is null.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferViewModel.kt (1)
141-141: ⚡ Quick win
OnRetryFetchingdoesn't retry the operation that actually failed.
FetchingFailedcan be produced by eitherfetchLoanAccountDetails(Lines 151-159) orfetchClientDetails(Lines 200-204), but the retry handler only re-invokesfetchClientDetails(state.fromClientId). If the loan fetch was the failing one,state.fromClientIdisnull, so the retry becomes a no-op (and, with the bug noted on Lines 184-207, still nulls out source fields) instead of re-fetching the loan account.Consider routing the retry through
fetchLoanAccountDetails(route.fromAccountId), which is the canonical entry point and will naturally chain forward to client and template fetches.♻️ Proposed change
- AmountTransferAction.OnRetryFetching -> fetchClientDetails(state.fromClientId) + AmountTransferAction.OnRetryFetching -> fetchLoanAccountDetails(route.fromAccountId)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferViewModel.kt` at line 141, The retry handler currently maps AmountTransferAction.OnRetryFetching to fetchClientDetails(state.fromClientId) which doesn't cover failures from fetchLoanAccountDetails and can be a no-op when state.fromClientId is null; change the retry to invoke the canonical entrypoint fetchLoanAccountDetails(route.fromAccountId) (or conditionally call fetchClientDetails if only client fetch failed) so retries always re-run the correct failed operation—update the handler for AmountTransferAction.OnRetryFetching to call fetchLoanAccountDetails with route.fromAccountId (or fallback to fetchClientDetails(state.fromClientId) when appropriate) and ensure this new flow preserves source fields instead of nulling them out.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferViewModel.kt`:
- Around line 211-217: The code currently passes a sentinel 0 for fromClientId
to repository.getAccountTransferTemplate; instead, guard on state.fromClientId
like you do for fromAccountType and skip calling
repository.getAccountTransferTemplate when fromClientId is null—e.g., check
state.fromClientId (or use state.fromClientId?.let) alongside
state.fromAccountType?.let and only call AmountTransferViewModel's
repository.getAccountTransferTemplate with the non-null fromClientId to avoid
sending an invalid client id.
- Around line 165-178: Remove the incorrect assignment of fromOfficeId from the
loan success branch (the loanOfficerId is a staff ID, not an office ID) so
mutableStateFlow.update in the DataState.Success branch does not set
fromOfficeId; rely on fetchClientDetails to populate fromOfficeId from the
client.officeId. Also stop converting Currency.code to an empty string on
null—when updating currency in that same mutableStateFlow.update use the raw
nullable currency code (or preserve the existing state value) instead of calling
.orEmpty(), so downstream UI can detect a missing currency. Ensure
fetchInitialTemplate() / submitTransfer() use the client-sourced office ID once
fetchClientDetails completes.
---
Outside diff comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferViewModel.kt`:
- Around line 184-207: fetchClientDetails currently overwrites source-account
state with null when clientId is null; change fetchClientDetails to early-handle
a null clientId: if clientId is null either update mutableStateFlow to set
dialogState to a FetchingFailed (or appropriate error) and return, or simply
return without calling mutableStateFlow.update or fetchInitialTemplate so
existing fromOfficeId/fromClientName/fromOfficeName are preserved; locate the
logic inside fetchClientDetails (the clientId?.let { clientRepo.getClient(it) }
call, the following mutableStateFlow.update block, and the fetchInitialTemplate
invocation) and add the null-guard before calling clientRepo.getClient or
updating state so downstream code only runs with a non-null client and
consistent behavior with fetchLoanAccountDetails/OnRetryFetching.
- Around line 209-249: fetchInitialTemplate (and likewise
fetchTemplateWithDependencies) sets dialogState = Loading then uses a nullable
flow chain (state.fromAccountType?.let { ... }?.collect { ... }) so when
fromAccountType or fromClientId is null the collect never runs and the Loading
dialog is never cleared; fix by checking required fields before setting Loading
(early return) or explicitly handling the null branch to reset dialogState via
mutableStateFlow.update(... copy(dialogState = null)) — update
fetchInitialTemplate and fetchTemplateWithDependencies to validate
state.fromAccountType and state.fromClientId up front and either return before
launching the Loading state or clear dialogState when the nullable let returns
null so the UI is not stuck.
- Around line 251-313: fetchTemplateWithDependencies sets dialogState = Loading
before checking nullable inputs and mixes a snapshot
(currentState.selectedOfficeId) with live state fields, causing stuck-loading
and racey requests; fix by snapshotting all needed fields from state into local
vals (e.g., capture selectedOfficeId, selectedClientId, accountTypeId,
fromClientId, fromAccountType, fromOfficeId) before launching, check those
captured vals for nulls and only set dialogState = Loading when all required
inputs are present, then call repository.getAccountTransferTemplate with the
captured values and collect; if required inputs are missing, update
mutableStateFlow to clear Loading / set an appropriate FetchingFailed instead of
leaving Loading indefinitely.
---
Duplicate comments:
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt`:
- Around line 255-258: The code dispatches ClientLoanAccountsAction.TransferFund
using a fallback id of -1 (loan.id ?: -1) which can start a broken flow; update
the onAction invocation in ClientLoanAccountsScreen so it only dispatches
ClientLoanAccountsAction.TransferFund when loan.id is non-null and positive
(e.g., check loan.id != null && loan.id > 0) and otherwise skip dispatch or
handle the missing-id case (show error/disable action) so Actions.TransferFund
is never created with an invalid id.
---
Nitpick comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferViewModel.kt`:
- Line 141: The retry handler currently maps
AmountTransferAction.OnRetryFetching to fetchClientDetails(state.fromClientId)
which doesn't cover failures from fetchLoanAccountDetails and can be a no-op
when state.fromClientId is null; change the retry to invoke the canonical
entrypoint fetchLoanAccountDetails(route.fromAccountId) (or conditionally call
fetchClientDetails if only client fetch failed) so retries always re-run the
correct failed operation—update the handler for
AmountTransferAction.OnRetryFetching to call fetchLoanAccountDetails with
route.fromAccountId (or fallback to fetchClientDetails(state.fromClientId) when
appropriate) and ensure this new flow preserves source fields instead of nulling
them out.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileScreen.kt`:
- Around line 88-109: Redundant local `account` is re-declared inside the
LoanProfileAction.Transfer branch of LoanAccountProfileScreen's EventsEffect;
remove the inner declaration and use the already-unwrapped `account` from before
the when so Transfer calls navigateToTransferScreen(account.id) directly,
keeping the existing safe-return (return@EventsEffect) behavior if
state.loanAccount is null.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 89eef9e4-6436-4c37-9baf-ac65d2622377
📒 Files selected for processing (12)
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/icon/MifosIcons.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/transfer/AccountTransferRequest.ktcore/ui/src/commonMain/composeResources/values/strings.xmlcore/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosActionsListingCardComponent.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsRoute.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsViewModel.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferScreenRoute.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferViewModel.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileNavigation.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileScreen.kt
💤 Files with no reviewable changes (1)
- feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferScreenRoute.kt
biplab1
reviewed
May 7, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferViewModel.kt (2)
170-170:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid collapsing null currency codes to an empty string.
Using
.orEmpty()here hides missing currency metadata and propagates""as if it were a valid code. Keep null semantics explicit (or apply a meaningful fallback constant at one boundary).Based on learnings: nullable currency values should be handled safely and consistently instead of passing ambiguous values downstream.
Also applies to: 469-469
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferViewModel.kt` at line 170, The assignment `currency = dataState.data?.currency?.code.orEmpty()` in AmountTransferViewModel collapses a missing currency into an empty string; instead preserve null semantics by assigning the nullable code directly (currency = dataState.data?.currency?.code) or, if a default is required, use a named fallback constant (e.g., DEFAULT_CURRENCY_CODE) at the view-model boundary; update the same pattern found later in the file (the second occurrence) so downstream code can handle nulls explicitly or rely on a clear constant rather than an empty string.
165-173:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the client office ID as the source office; don’t map
loanOfficerIdtofromOfficeId.Line 169 assigns
loanOfficerIdintofromOfficeId. That value represents a staff reference, not an office reference, and can leak wrong source-office data beforefetchClientDetailscorrects it.💡 Suggested fix
is DataState.Success -> { mutableStateFlow.update { it.copy( dialogState = null, - fromOfficeId = dataState.data?.loanOfficerId, currency = dataState.data?.currency?.code.orEmpty(), fromClientId = dataState.data?.clientId, fromAccountNumber = dataState.data?.accountNo, fromAccountType = dataState.data?.loanType?.id, ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferViewModel.kt` around lines 165 - 173, The code is incorrectly assigning loanOfficerId to fromOfficeId in the DataState.Success branch; instead, update the mutableStateFlow.update to set fromOfficeId from the client's office id (e.g., dataState.data?.clientOfficeId or dataState.data?.officeId if that’s the actual property) and remove the mapping from dataState.data?.loanOfficerId to fromOfficeId so the source office reflects the client’s office until fetchClientDetails runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferViewModel.kt`:
- Line 141: The retry path can set dialogState = Loading even when
state.fromClientId is null causing an indefinite spinner; in the handler for
AmountTransferAction.OnRetryFetching (and inside fetchClientDetails) guard the
null: only set dialogState = Loading after confirming state.fromClientId is
non-null (or immediately return / set dialogState to an error/idle state if it
is null) and keep the template-fetching logic inside the same non-null branch
(i.e., move the Loading assignment into the let { ... } block or add an early
null check at the start of fetchClientDetails to bail out and set a non-loading
dialogState when fromClientId == null).
---
Duplicate comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferViewModel.kt`:
- Line 170: The assignment `currency = dataState.data?.currency?.code.orEmpty()`
in AmountTransferViewModel collapses a missing currency into an empty string;
instead preserve null semantics by assigning the nullable code directly
(currency = dataState.data?.currency?.code) or, if a default is required, use a
named fallback constant (e.g., DEFAULT_CURRENCY_CODE) at the view-model
boundary; update the same pattern found later in the file (the second
occurrence) so downstream code can handle nulls explicitly or rely on a clear
constant rather than an empty string.
- Around line 165-173: The code is incorrectly assigning loanOfficerId to
fromOfficeId in the DataState.Success branch; instead, update the
mutableStateFlow.update to set fromOfficeId from the client's office id (e.g.,
dataState.data?.clientOfficeId or dataState.data?.officeId if that’s the actual
property) and remove the mapping from dataState.data?.loanOfficerId to
fromOfficeId so the source office reflects the client’s office until
fetchClientDetails runs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: be895e7c-31a8-4c67-8da9-716277312d16
📒 Files selected for processing (1)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferViewModel.kt
…vent dispatching logic
Member
Author
|
@biplab1 please again review |
Contributor
|
@Arinyadav1 Have you tested the screens to ensure everything works fine after the recent updates? |
Member
Author
|
Yes working fine
…On Thu, 7 May 2026 at 8:36 PM, Biplab Dutta ***@***.***> wrote:
*biplab1* left a comment (openMF/mifos-x-field-officer-app#2675)
<#2675 (comment)>
@Arinyadav1 <https://github.com/Arinyadav1> Have you tested the screens
to ensure everything works fine after the recent updates?
—
Reply to this email directly, view it on GitHub
<#2675 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/A3UKE4GF2C3LSGLZDJNROK34ZSRBHAVCNFSM6AAAAACYTP5NJGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DGOJYGI3TKMRXHA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
niyajali
requested changes
May 7, 2026
niyajali
left a comment
niyajali
left a comment
Collaborator
There was a problem hiding this comment.
@Arinyadav1 do these changes on other ViewModel too if needed
There was a problem hiding this comment.
🧹 Nitpick comments (2)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsViewModel.kt (1)
138-141: 💤 Low valueSearch filter is case-sensitive and does not handle null
searchTextwhitespace edge cases gracefully.
it.accountNo?.contains(state.searchText.trim()) == trueis case-sensitive, so searching"abc"will not matchaccountNo = "ABC123". This is pre-existing behavior reflected in the// Todo modify search accordinglycomment, but worth noting if you're updating this surface area.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsViewModel.kt` around lines 138 - 141, The search currently uses a case-sensitive contains on accountNo and can fail when state.searchText is null/blank; update the filter in ClientLoanAccountsViewModel where loanAccounts is computed (repository.getClientAccounts(...).loanAccounts) to normalize both sides and guard empty input: trim and coalesce state.searchText to an empty string, return all accounts when that trimmed string is empty, and perform a case-insensitive comparison (e.g., compare lowercase accountNo and lowercase searchText) so searches like "abc" match "ABC123".feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferViewModel.kt (1)
226-232: 💤 Low valueAvoid catching
CancellationExceptionvia the broadExceptioncatch.
catch (e: Exception)in a coroutine swallowsCancellationException, which can break structured concurrency (e.g., the dialog stays inFetchingFailedif the scope is cancelled). Either rethrowCancellationExceptionor use a more specific catch.♻️ Proposed refactor
- } catch (e: Exception) { + } catch (e: kotlinx.coroutines.CancellationException) { + throw e + } catch (e: Exception) { mutableStateFlow.update { it.copy( dialogState = AmountTransferUiState.DialogState.FetchingFailed(e.message.toString()), ) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferViewModel.kt` around lines 226 - 232, The current catch in AmountTransferViewModel swallows CancellationException; update the error handling around the mutableStateFlow.update block so CancellationException is rethrown instead of being handled as a fetch failure—either add an explicit catch (e: CancellationException) { throw e } before the general Exception catch, or change the catch to catch (e: Throwable) { if (e is CancellationException) throw e; /* set dialogState to FetchingFailed with e.message */ } so the dialogState assignment (the mutableStateFlow.update that sets DialogState.FetchingFailed) only runs for non-cancellation errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsViewModel.kt`:
- Around line 138-141: The search currently uses a case-sensitive contains on
accountNo and can fail when state.searchText is null/blank; update the filter in
ClientLoanAccountsViewModel where loanAccounts is computed
(repository.getClientAccounts(...).loanAccounts) to normalize both sides and
guard empty input: trim and coalesce state.searchText to an empty string, return
all accounts when that trimmed string is empty, and perform a case-insensitive
comparison (e.g., compare lowercase accountNo and lowercase searchText) so
searches like "abc" match "ABC123".
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferViewModel.kt`:
- Around line 226-232: The current catch in AmountTransferViewModel swallows
CancellationException; update the error handling around the
mutableStateFlow.update block so CancellationException is rethrown instead of
being handled as a fetch failure—either add an explicit catch (e:
CancellationException) { throw e } before the general Exception catch, or change
the catch to catch (e: Throwable) { if (e is CancellationException) throw e; /*
set dialogState to FetchingFailed with e.message */ } so the dialogState
assignment (the mutableStateFlow.update that sets DialogState.FetchingFailed)
only runs for non-cancellation errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d745bb99-ccd1-43b6-991d-f73aeed4eff5
📒 Files selected for processing (3)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsViewModel.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/amountTransfer/AmountTransferViewModel.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Member
Author
|
@biplab1 please do the final review |
Contributor
|
@Arinyadav1 Please address the relevant CodeRabbitAI suggestions. You may skip the data class related suggestion. |
Member
Author
|
@biplab1 the data class is directly use in service class so if I remove Serializable then it throw error so I thought these all architecture issue resolve in separate pr because this PR only focus on transfer fund option |
niyajali
requested changes
May 8, 2026
niyajali
left a comment
niyajali
left a comment
Collaborator
There was a problem hiding this comment.
do these changes on other files too where required
…tShowCard component
niyajali
requested changes
May 8, 2026
…ve state handling
Member
Author
niyajali
reviewed
May 12, 2026
niyajali
left a comment
niyajali
left a comment
Collaborator
There was a problem hiding this comment.
@Arinyadav1 resolve merge conflicts
biplab1
reviewed
May 13, 2026
biplab1
reviewed
May 13, 2026
Member
Author
|
@biplab1 I have change the transfer logo please review it |
Contributor
|
@Arinyadav1 Please place the icon inside |
…AccountsScreen` references
Member
Author
|
@biplab1 it has done review and merge it |
Member
Author
|
@biplab1 please review and merge this one |
Member
Author
|
@biplab1 please do the final review and make it mergable |
niyajali
approved these changes
May 22, 2026
niyajali
enabled auto-merge (squash)
May 22, 2026 03:43
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes - Jira-#659
Screen_recording_20260507_000801.mp4