refactor(core): introduce domain exceptions for ui error handling#3134
Open
mena-rizkalla wants to merge 2 commits intoopenMF:developmentfrom
Open
refactor(core): introduce domain exceptions for ui error handling#3134mena-rizkalla wants to merge 2 commits intoopenMF:developmentfrom
mena-rizkalla wants to merge 2 commits intoopenMF:developmentfrom
Conversation
- Introduce `MifosException` sealed class to categorize errors into `ServerError`, `ClientError`, `NetworkError`, and `GenericError`. - Add `ExceptionMapper` utility to convert `Throwable` and Ktor exceptions into `MifosException` types, including a suspend version for async error extraction. - Update `asDataStateFlow` extension to support custom exception mapping. - Refactor all repository implementations (`BeneficiaryRepositoryImp`, `LoanRepositoryImp`, `SavingsAccountRepositoryImp`, etc.) to use the new exception mapping logic. - Update ViewModels across all features to handle specific `MifosException` types instead of raw IO or Ktor exceptions for UI state management.
Contributor
📝 WalkthroughWalkthroughCentralizes exception handling by introducing a sealed MifosException hierarchy, adds BaseRepository with safeCall/asDataState helpers, updates DataState flow/suspend wrappers to map throwables to MifosException, and refactors repositories and many ViewModels to use the new centralized error mapping and dispatcher handling. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
feature/beneficiary/src/commonMain/kotlin/org/mifos/mobile/feature/beneficiary/beneficiaryList/BeneficiaryListViewModel.kt (1)
45-54:⚠️ Potential issue | 🟠 MajorDuplicate
initblocks causeobserveNetwork()to be called twice.There are two
initblocks that both callobserveNetwork(), resulting in duplicate flow collectors for the network status. This will cause theReceiveNetworkStatusaction to be dispatched twice for every network state change.Proposed fix: Remove the duplicate init block
/** * Initialize the view model. */ init { observeNetwork() } - /** - * Initialize the view model. - */ - init { - observeNetwork() - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/beneficiary/src/commonMain/kotlin/org/mifos/mobile/feature/beneficiary/beneficiaryList/BeneficiaryListViewModel.kt` around lines 45 - 54, The class contains two init blocks both calling observeNetwork(), which registers duplicate collectors and dispatches ReceiveNetworkStatus twice; remove the duplicate init block so observeNetwork() is only invoked once (keep a single init { observeNetwork() }), ensuring only one collector is registered and ReceiveNetworkStatus is emitted once per network change.feature/share-application/src/commonMain/kotlin/org/mifos/mobile/feature/share/application/shareApplication/ShareApplyViewModel.kt (1)
135-146:⚠️ Potential issue | 🔴 CriticalFix incorrect exception type check in catch block.
The condition
throwable.cause is MifosException.NetworkErroris incorrect. WhentoMifosException()wraps anIOException, it creates aMifosException.NetworkErrorinstance with theIOExceptionas its cause field. Therefore,throwableitself is already theMifosException.NetworkError—thecausefield holds the originalIOException, not anotherMifosException.Check
throwabledirectly instead:Proposed fix
.catch { throwable -> updateState { it.copy( - uiState = if (throwable.cause is MifosException.NetworkError) { + uiState = if (throwable is MifosException.NetworkError) { ShareApplicationUiState.Network } else { ShareApplicationUiState.Error(Res.string.feature_apply_share_error_server) }, ) } }Note:
SavingsApplyViewModelhas the same pattern error at line 194.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/share-application/src/commonMain/kotlin/org/mifos/mobile/feature/share/application/shareApplication/ShareApplyViewModel.kt` around lines 135 - 146, The catch block in ShareApplyViewModel incorrectly checks throwable.cause for MifosException.NetworkError; change the check to inspect throwable itself (i.e., whether throwable is MifosException.NetworkError) and update the uiState accordingly in the .catch { throwable -> ... } handler so Network errors map to ShareApplicationUiState.Network and other errors map to ShareApplicationUiState.Error; apply the same fix in SavingsApplyViewModel where the identical pattern occurs.feature/savings-application/src/commonMain/kotlin/org/mifos/mobile/feature/savings/application/savingsApplication/SavingsApplyViewModel.kt (1)
194-198:⚠️ Potential issue | 🟠 MajorNetwork error detection is checking the wrong object.
At Line 194, checking
throwable.causecan miss mapped domain errors. The throwable itself should be matched asMifosException.NetworkError.Proposed fix
- uiState = if (throwable.cause is MifosException.NetworkError) { + uiState = if (throwable is MifosException.NetworkError) { ScreenUiState.Network } else { ScreenUiState.Error(Res.string.feature_apply_savings_error_server) },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/savings-application/src/commonMain/kotlin/org/mifos/mobile/feature/savings/application/savingsApplication/SavingsApplyViewModel.kt` around lines 194 - 198, In SavingsApplyViewModel where uiState is set (the block that currently checks throwable.cause), change the error detection to match the throwable itself against MifosException.NetworkError rather than inspecting throwable.cause; i.e., replace the condition using throwable.cause with a direct type check for MifosException.NetworkError so you return ScreenUiState.Network, otherwise return ScreenUiState.Error(Res.string.feature_apply_savings_error_server).
🧹 Nitpick comments (3)
core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ClientChargeRepositoryImp.kt (1)
37-48: Consider usingasDataStateFlowhelper for consistency.Other repositories in this PR use
asDataStateFlow(Throwable::toMifosException)which handles the emit pattern correctly. Using the helper here would avoid the manual catch/emit pattern and ensure consistency across repositories.♻️ Proposed refactor using asDataStateFlow
+import org.mifos.mobile.core.common.asDataStateFlow override fun getCharges(clientId: Long): Flow<DataState<Page<Charge>>> { return dataManager.clientChargeApi.getClientChargeList(clientId) .map { response -> - DataState.Success( - response.toPageModel { dto -> - dto.toModel() - }, - ) + response.toPageModel { dto -> dto.toModel() } } - .catch { exception -> DataState.Error(exception.toMifosException(), null) } + .asDataStateFlow(Throwable::toMifosException) .flowOn(ioDispatcher) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ClientChargeRepositoryImp.kt` around lines 37 - 48, Replace the manual map/catch/flowOn chain in ClientChargeRepositoryImp.getCharges with the shared asDataStateFlow helper to match other repos: keep the mapping of response.toPageModel { dto -> dto.toModel() } on the Flow returned by dataManager.clientChargeApi.getClientChargeList(clientId), then call .asDataStateFlow(Throwable::toMifosException) to produce Flow<DataState<Page<Charge>>> and preserve .flowOn(ioDispatcher) if needed; remove the explicit .catch { ... } and DataState.Error construction so the helper handles error conversion consistently.core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/LoanRepositoryImp.kt (1)
48-50: Inconsistent error message extraction forClientRequestException.This uses
toMifosException()whilewithdrawLoanAccountat line 74 usestoMifosExceptionSuspend(). The suspend variant extracts a parsed user-friendly message from the HTTP response body viaextractErrorMessage(response), whereas the non-suspend variant just uses the rawmessage. Since this catch block is within aflow { }(suspend context), consider usingtoMifosExceptionSuspend()for consistent error messaging across all methods.♻️ Suggested fix
} catch (exception: Exception) { - emit(DataState.Error(exception.toMifosException())) + emit(DataState.Error(exception.toMifosExceptionSuspend())) }Also update the import:
import org.mifos.mobile.core.data.util.toMifosExceptionSuspend🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/LoanRepositoryImp.kt` around lines 48 - 50, The catch block in LoanRepositoryImp (inside the suspending flow) uses toMifosException() which yields inconsistent/raw messages compared to other suspend handlers like withdrawLoanAccount that use toMifosExceptionSuspend(); replace the call to exception.toMifosException() with exception.toMifosExceptionSuspend() and update imports to include org.mifos.mobile.core.data.util.toMifosExceptionSuspend (remove the old import if unused) so the parsed user-friendly HTTP error body is extracted consistently across methods.core/common/src/commonMain/kotlin/org/mifos/mobile/core/common/DataState.kt (1)
36-46: Consider documenting the trade-off between the two overloads.The parameterless
asDataStateFlow()wraps all non-MifosExceptionthrowables asGenericError, losing the distinction betweenNetworkError,ServerError, andClientError. This is fine for flows that don't throw Ktor-specific exceptions, but repositories dealing with network calls should use the overload withThrowable::toMifosExceptionto preserve error categorization.A brief KDoc comment could help prevent misuse:
📝 Suggested documentation
+/** + * Transforms this flow into a DataState flow. Non-MifosException throwables + * are wrapped as GenericError. For flows emitting Ktor exceptions, prefer + * the overload accepting an exceptionMapper (e.g., Throwable::toMifosException). + */ fun <T> Flow<T>.asDataStateFlow(): Flow<DataState<T>> =🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/common/src/commonMain/kotlin/org/mifos/mobile/core/common/DataState.kt` around lines 36 - 46, Add a brief KDoc above the parameterless asDataStateFlow() function explaining that it maps any non-MifosException throwable to MifosException.GenericError (thus losing NetworkError/ServerError/ClientError distinctions), and advise repositories that perform network/ktor calls to use the overload that accepts Throwable::toMifosException to preserve error categorization; reference the function name asDataStateFlow and the Throwable::toMifosException overload in the comment so callers know which variant to use.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/BeneficiaryRepositoryImp.kt`:
- Around line 54-56: The catch-all Exception handlers in
BeneficiaryRepositoryImp (in methods getBeneficiaries, updateBeneficiary,
deleteBeneficiary) are swallowing CancellationException by mapping every
Exception via toMifosExceptionSuspend(); change the error handling to re-throw
CancellationException before mapping (either by adding an explicit check in each
catch or, better, update Throwable.toMifosExceptionSuspend() in
ExceptionMapper.kt to re-throw when the Throwable is a CancellationException),
so coroutine cancellation is propagated correctly while other exceptions
continue to be mapped to MifosException.
In
`@core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ClientChargeRepositoryImp.kt`:
- Around line 88-90: The .catch block inside ClientChargeRepositoryImp (the flow
pipeline using .catch { exception ->
DataState.Error(exception.toMifosException(), null) }) is returning a value
instead of emitting it; change the catch handler to emit the error into the flow
(call emit(DataState.Error(exception.toMifosException(), null))) so the
DataState.Error is actually propagated, keeping the use of toMifosException()
and the surrounding flow unchanged.
- Line 46: In ClientChargeRepositoryImp (the flow pipeline using .catch), the
catch block constructs a DataState.Error but doesn't emit it, so errors are
swallowed; change the catch handler in that flow to call emit(...) with the
DataState.Error (e.g., .catch { e -> emit(DataState.Error(e.toMifosException(),
null)) }) so the error is propagated to downstream collectors.
- Line 57: The catch block in the Flow pipeline uses ".catch { exception ->
DataState.Error(exception.toMifosException(), null) }" but never emits the error
state; update the catch to emit the error by changing it to ".catch { exception
-> emit(DataState.Error(exception.toMifosException(), null)) }" so the Flow
actually propagates the DataState.Error (ensure this change is made in
ClientChargeRepositoryImp.kt where .catch, DataState.Error and
toMifosException() are used).
In
`@core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/GuarantorRepositoryImp.kt`:
- Around line 50-52: In GuarantorRepositoryImp's suspend methods
createGuarantor, updateGuarantor, and deleteGuarantor the generic catch (e:
Exception) swallows CancellationException; modify each method to rethrow
CancellationException instead of converting it to a DataState.Error (e.g., check
if the caught exception is a
kotlin.coroutines.cancellation.CancellationException and rethrow it, otherwise
call e.toMifosExceptionSuspend() and return DataState.Error). Ensure the
CancellationException check happens before the generic Exception handling so
coroutine cancellation propagates correctly.
- Around line 86-92: The getGuarantorList method currently returns hardcoded
demo data; replace the demo flow with the real API call by calling
dataManager.guarantorApi.getGuarantorList(loanId) and piping it through the same
asDataStateFlow and flowOn chain used elsewhere (preserve the
Throwable::toMifosException mapping and ioDispatcher usage) so
getGuarantorList(loanId) returns the actual API-backed
Flow<DataState<List<GuarantorPayload?>?>> instead of getDemoGuarantorPayloads().
In
`@core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ReviewLoanApplicationRepositoryImpl.kt`:
- Around line 48-49: The catch block in ReviewLoanApplicationRepositoryImpl
currently uses catch (e: Exception) which will swallow CancellationException and
prevent proper coroutine cancellation; modify the error handling in the method
where DataState.Error(e: toMifosExceptionSuspend()) is returned so that
CancellationException is not swallowed — either add an early check to rethrow if
e is CancellationException or split into two catches (catch (e:
CancellationException) { throw e } followed by catch (e: Exception) { return
DataState.Error(e.toMifosExceptionSuspend(), null) }), ensuring the coroutine
can cancel normally.
In
`@core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/SavingsAccountRepositoryImp.kt`:
- Around line 92-94: The catch blocks in submitSavingAccountApplication,
updateSavingsAccount, and submitWithdrawSavingsAccount currently catch Exception
and swallow CancellationException; update each catch to rethrow
CancellationException before mapping (e.g., in each catch do: if (e is
CancellationException) throw e; then return
DataState.Error(e.toMifosExceptionSuspend(), null)). Target the existing catch
sites in SavingsAccountRepositoryImp and keep using toMifosExceptionSuspend()
for non-cancellation errors.
In
`@core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ShareAccountRepositoryImp.kt`:
- Around line 67-69: The catch block in ShareAccountRepositoryImp (where you
convert exceptions to DataState.Error) currently catches Exception and will
swallow CancellationException; change the error handling to explicitly rethrow
CancellationException (or check e is CancellationException) before mapping other
exceptions to DataState.Error in the same method (the try/catch that returns
DataState.Error(e.toMifosExceptionSuspend(), ...)), ensuring structured
concurrency is preserved by not intercepting coroutine cancellations.
- Around line 48-49: The catch block in ShareAccountRepositoryImp.kt currently
constructs a DataState.Error but does not emit it, so exceptions are swallowed;
update the .catch { exception -> ... } handler to emit the error (e.g., .catch {
exception -> emit(DataState.Error(exception.toMifosException(), null)) }) so
consumers receive the error, keeping the existing DataState.Error and
toMifosException() usage and preserving the .flowOn(ioDispatcher) placement.
In
`@core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/UserDetailRepositoryImp.kt`:
- Around line 39-41: In UserDetailRepositoryImp (catch blocks around lines
handling exceptions), do not swallow CancellationException: in each catch (e:
Exception) before calling e.toMifosExceptionSuspend() re-throw if e is a
CancellationException (or use
kotlin.coroutines.cancellation.CancellationException), otherwise continue to map
and return DataState.Error; update both catch sites referenced in this file so
CancellationException propagates rather than being converted by
toMifosExceptionSuspend().
In
`@core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/util/ExceptionMapper.kt`:
- Line 14: In ExceptionMapper.kt replace the incorrect import of
kotlinx.io.IOException with okio.IOException so the ExceptionMapper (and any
functions referencing IOException) use the okio library provided transitively
via ktor-client-okhttp; update the import statement from kotlinx.io.IOException
to okio.IOException and ensure any usages of IOException in the file remain
unchanged and compile against okio.IOException.
- Around line 17-23: The toMifosException extension function incorrectly treats
CancellationException as a generic error; update Throwable.toMifosException to
explicitly detect kotlin.coroutines.cancellation.CancellationException (or
java.util.concurrent.CancellationException) and rethrow it instead of mapping it
to a MifosException so cancellation semantics are preserved (locate the when
branch in Throwable.toMifosException and add a case that throws the caught
CancellationException before the other mappings).
In
`@feature/beneficiary/src/commonMain/kotlin/org/mifos/mobile/feature/beneficiary/beneficiaryApplication/BeneficiaryApplicationViewModel.kt`:
- Around line 200-203: In the catch block inside BeneficiaryApplicationViewModel
where uiState is set, change the condition to check the caught exception itself
(error) for MifosException.NetworkError rather than inspecting error.cause;
update the conditional that currently reads error.cause is
MifosException.NetworkError to use error is MifosException.NetworkError so
network errors set ScreenUiState.Network and other MifosException cases fall
through to ScreenUiState.Error(Res.string.feature_generic_error_server).
In
`@feature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/loanAccountRepaymentSchedule/RepaymentScheduleViewModel.kt`:
- Around line 180-183: The catch block in RepaymentScheduleViewModel sets
uiState by checking error.cause instead of the thrown exception itself, which
can miss network errors; update the catch to directly check the caught exception
(e.g., error is MifosException.NetworkError) when setting uiState to
ScreenUiState.Network and otherwise use
ScreenUiState.Error(Res.string.feature_generic_error_server), ensuring you
reference the existing catch variable and MifosException.NetworkError type in
the uiState assignment.
---
Outside diff comments:
In
`@feature/beneficiary/src/commonMain/kotlin/org/mifos/mobile/feature/beneficiary/beneficiaryList/BeneficiaryListViewModel.kt`:
- Around line 45-54: The class contains two init blocks both calling
observeNetwork(), which registers duplicate collectors and dispatches
ReceiveNetworkStatus twice; remove the duplicate init block so observeNetwork()
is only invoked once (keep a single init { observeNetwork() }), ensuring only
one collector is registered and ReceiveNetworkStatus is emitted once per network
change.
In
`@feature/savings-application/src/commonMain/kotlin/org/mifos/mobile/feature/savings/application/savingsApplication/SavingsApplyViewModel.kt`:
- Around line 194-198: In SavingsApplyViewModel where uiState is set (the block
that currently checks throwable.cause), change the error detection to match the
throwable itself against MifosException.NetworkError rather than inspecting
throwable.cause; i.e., replace the condition using throwable.cause with a direct
type check for MifosException.NetworkError so you return ScreenUiState.Network,
otherwise return
ScreenUiState.Error(Res.string.feature_apply_savings_error_server).
In
`@feature/share-application/src/commonMain/kotlin/org/mifos/mobile/feature/share/application/shareApplication/ShareApplyViewModel.kt`:
- Around line 135-146: The catch block in ShareApplyViewModel incorrectly checks
throwable.cause for MifosException.NetworkError; change the check to inspect
throwable itself (i.e., whether throwable is MifosException.NetworkError) and
update the uiState accordingly in the .catch { throwable -> ... } handler so
Network errors map to ShareApplicationUiState.Network and other errors map to
ShareApplicationUiState.Error; apply the same fix in SavingsApplyViewModel where
the identical pattern occurs.
---
Nitpick comments:
In `@core/common/src/commonMain/kotlin/org/mifos/mobile/core/common/DataState.kt`:
- Around line 36-46: Add a brief KDoc above the parameterless asDataStateFlow()
function explaining that it maps any non-MifosException throwable to
MifosException.GenericError (thus losing NetworkError/ServerError/ClientError
distinctions), and advise repositories that perform network/ktor calls to use
the overload that accepts Throwable::toMifosException to preserve error
categorization; reference the function name asDataStateFlow and the
Throwable::toMifosException overload in the comment so callers know which
variant to use.
In
`@core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ClientChargeRepositoryImp.kt`:
- Around line 37-48: Replace the manual map/catch/flowOn chain in
ClientChargeRepositoryImp.getCharges with the shared asDataStateFlow helper to
match other repos: keep the mapping of response.toPageModel { dto ->
dto.toModel() } on the Flow returned by
dataManager.clientChargeApi.getClientChargeList(clientId), then call
.asDataStateFlow(Throwable::toMifosException) to produce
Flow<DataState<Page<Charge>>> and preserve .flowOn(ioDispatcher) if needed;
remove the explicit .catch { ... } and DataState.Error construction so the
helper handles error conversion consistently.
In
`@core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/LoanRepositoryImp.kt`:
- Around line 48-50: The catch block in LoanRepositoryImp (inside the suspending
flow) uses toMifosException() which yields inconsistent/raw messages compared to
other suspend handlers like withdrawLoanAccount that use
toMifosExceptionSuspend(); replace the call to exception.toMifosException() with
exception.toMifosExceptionSuspend() and update imports to include
org.mifos.mobile.core.data.util.toMifosExceptionSuspend (remove the old import
if unused) so the parsed user-friendly HTTP error body is extracted consistently
across methods.
🪄 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: c19a6526-45d0-49bb-8da0-9f0f30013fa4
📒 Files selected for processing (45)
core/common/src/commonMain/kotlin/org/mifos/mobile/core/common/DataState.ktcore/common/src/commonMain/kotlin/org/mifos/mobile/core/common/MifosException.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/AccountsRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/AuthenticationUserRepository.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/BeneficiaryRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ClientChargeRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ClientRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/GuarantorRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/HomeRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/LoanRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/RecentTransactionRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ReviewLoanApplicationRepositoryImpl.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/SavingsAccountRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ShareAccountRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ThirdPartyTransferRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/TransferRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/UserAuthRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/UserDetailRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/util/ExceptionMapper.ktfeature/accounts/src/commonMain/kotlin/org/mifos/mobile/feature/accounts/accountTransactions/TransactionViewModel.ktfeature/auth/src/commonMain/kotlin/org/mifos/mobile/feature/auth/login/LoginViewModel.ktfeature/auth/src/commonMain/kotlin/org/mifos/mobile/feature/auth/otpAuthentication/OtpAuthenticationViewModel.ktfeature/auth/src/commonMain/kotlin/org/mifos/mobile/feature/auth/registration/RegistrationViewModel.ktfeature/beneficiary/src/commonMain/kotlin/org/mifos/mobile/feature/beneficiary/beneficiaryApplication/BeneficiaryApplicationViewModel.ktfeature/beneficiary/src/commonMain/kotlin/org/mifos/mobile/feature/beneficiary/beneficiaryApplicationConfirmation/BeneficiaryApplicationConfirmationViewModel.ktfeature/beneficiary/src/commonMain/kotlin/org/mifos/mobile/feature/beneficiary/beneficiaryDetail/BeneficiaryDetailViewModel.ktfeature/beneficiary/src/commonMain/kotlin/org/mifos/mobile/feature/beneficiary/beneficiaryList/BeneficiaryListViewModel.ktfeature/client-charge/src/commonMain/kotlin/org/mifos/mobile/feature/charge/charges/ClientChargeViewModel.ktfeature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/loanAccount/LoanAccountViewModel.ktfeature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/loanAccountDetails/LoanAccountDetailsViewModel.ktfeature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/loanAccountRepaymentSchedule/RepaymentScheduleViewModel.ktfeature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/loanAccountSummary/AccountSummaryViewModel.ktfeature/loan-application/src/commonMain/kotlin/org/mifos/mobile/feature/loan/application/confirmDetails/ConfirmDetailsViewModel.ktfeature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccount/SavingsAccountViewmodel.ktfeature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsViewModel.ktfeature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountUpdate/AccountUpdateViewModel.ktfeature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountWithdraw/AccountWithdrawViewModel.ktfeature/savings-application/src/commonMain/kotlin/org/mifos/mobile/feature/savings/application/fillApplication/FillApplicationViewModel.ktfeature/savings-application/src/commonMain/kotlin/org/mifos/mobile/feature/savings/application/savingsApplication/SavingsApplyViewModel.ktfeature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/password/ChangePasswordViewModel.ktfeature/share-account/src/commonMain/kotlin/org/mifos/mobile/feature/shareaccount/shareAccount/ShareAccountViewModel.ktfeature/share-account/src/commonMain/kotlin/org/mifos/mobile/feature/shareaccount/shareAccountDetails/ShareAccountDetailsViewModel.ktfeature/share-application/src/commonMain/kotlin/org/mifos/mobile/feature/share/application/fillApplication/FillApplicationViewModel.ktfeature/share-application/src/commonMain/kotlin/org/mifos/mobile/feature/share/application/shareApplication/ShareApplyViewModel.ktfeature/transfer-process/src/commonMain/kotlin/org/mifos/mobile/feature/transfer/process/transferProcess/TransferProcessViewModel.kt
|
|
||
| import io.ktor.client.plugins.ClientRequestException | ||
| import io.ktor.client.plugins.ServerResponseException | ||
| import kotlinx.io.IOException |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for kotlinx-io dependency in gradle files
rg -n "kotlinx-io|kotlinx.io" --type=gradle -g "*.kts"Repository: openMF/mifos-mobile
Length of output: 45
🏁 Script executed:
find . -name "*.gradle.kts" -o -name "*.gradle" | head -20Repository: openMF/mifos-mobile
Length of output: 737
🏁 Script executed:
cat core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/util/ExceptionMapper.ktRepository: openMF/mifos-mobile
Length of output: 1519
🏁 Script executed:
rg "IOException" --type kotlin -A 2 -B 2 | head -50Repository: openMF/mifos-mobile
Length of output: 5967
🏁 Script executed:
cat core/data/build.gradle.ktsRepository: openMF/mifos-mobile
Length of output: 1353
🏁 Script executed:
rg "okio|kotlinx-io" --type=gradle -nRepository: openMF/mifos-mobile
Length of output: 45
🏁 Script executed:
cat core-base/network/build.gradle.ktsRepository: openMF/mifos-mobile
Length of output: 1388
🏁 Script executed:
rg "okio" --type=gradle -nRepository: openMF/mifos-mobile
Length of output: 45
🏁 Script executed:
cat build.gradle.kts | grep -A 50 "dependencies\|catalog"Repository: openMF/mifos-mobile
Length of output: 1702
🏁 Script executed:
find . -name "*.toml" -path "*/gradle/*" | head -5Repository: openMF/mifos-mobile
Length of output: 90
🏁 Script executed:
cat gradle/libs.versions.toml | head -150Repository: openMF/mifos-mobile
Length of output: 5629
🏁 Script executed:
cat gradle/libs.versions.toml | grep -E "ktor|okio|kotlinx-io" -A 2 -B 2Repository: openMF/mifos-mobile
Length of output: 3260
Replace kotlinx.io.IOException with okio.IOException.
The kotlinx-io dependency is not available in this project. The correct IOException to use is okio.IOException from the okio library (version 3.9.1), which is already a transitive dependency via ktor-client-okhttp. Other files in the codebase (e.g., LoanApplyViewModel.kt) correctly import from okio.
import okio.IOException
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/util/ExceptionMapper.kt`
at line 14, In ExceptionMapper.kt replace the incorrect import of
kotlinx.io.IOException with okio.IOException so the ExceptionMapper (and any
functions referencing IOException) use the okio library provided transitively
via ktor-client-okhttp; update the import statement from kotlinx.io.IOException
to okio.IOException and ensure any usages of IOException in the file remain
unchanged and compile against okio.IOException.
Comment on lines
+17
to
+23
| fun Throwable.toMifosException(): MifosException = when (this) { | ||
| is MifosException -> this | ||
| is ClientRequestException -> MifosException.ClientError(message, this) | ||
| is ServerResponseException -> MifosException.ServerError(message, this) | ||
| is IOException -> MifosException.NetworkError(message ?: "Network error", this) | ||
| else -> MifosException.GenericError(message ?: "Unknown error", this) | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if CancellationException is handled elsewhere in exception mappers or catch blocks
rg -n "CancellationException" --type=kotlinRepository: openMF/mifos-mobile
Length of output: 2242
🏁 Script executed:
# Find all usages of toMifosException
rg -n "toMifosException" --type=kotlin -A 3 -B 3Repository: openMF/mifos-mobile
Length of output: 50375
🏁 Script executed:
# Check the full ExceptionMapper.kt file to understand context
cat core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/util/ExceptionMapper.ktRepository: openMF/mifos-mobile
Length of output: 1519
Add explicit handling for CancellationException to prevent suppressing cancellation semantics.
The non-suspend version doesn't explicitly handle CancellationException, so it falls through to the else clause and gets wrapped as GenericError. If a coroutine is cancelled while in a catch block (e.g., during an API call), this suppresses the cancellation signal and breaks structured concurrency. Add an explicit case to rethrow it.
Suggested fix
+import kotlin.coroutines.cancellation.CancellationException
+
fun Throwable.toMifosException(): MifosException = when (this) {
is MifosException -> this
+ is CancellationException -> throw this
is ClientRequestException -> MifosException.ClientError(message, this)
is ServerResponseException -> MifosException.ServerError(message, this)
is IOException -> MifosException.NetworkError(message ?: "Network error", this)
else -> MifosException.GenericError(message ?: "Unknown error", this)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/util/ExceptionMapper.kt`
around lines 17 - 23, The toMifosException extension function incorrectly treats
CancellationException as a generic error; update Throwable.toMifosException to
explicitly detect kotlin.coroutines.cancellation.CancellationException (or
java.util.concurrent.CancellationException) and rethrow it instead of mapping it
to a MifosException so cancellation semantics are preserved (locate the when
branch in Throwable.toMifosException and add a case that throws the caught
CancellationException before the other mappings).
niyajali
requested changes
Apr 4, 2026
| .map { it.toModel() } | ||
| .asDataStateFlow().flowOn(ioDispatcher) | ||
| .asDataStateFlow(Throwable::toMifosException).flowOn(ioDispatcher) | ||
| } |
Collaborator
There was a problem hiding this comment.
We can map this directly in the toDataStateFlow extension function, so we won't need to update each repository method individually, alternatively, we could create an overload to keep it backward compatible.
We also need to make sure we're catching server-side errors properly and mapping them to the right classes. To do that, we'll need a full list of possible error responses from the backend, either check the web app or ask Victor if there's any documentation or reference for those.
In the meantime, check the NetworkResult class in the core-base/network module. it should give a good starting point for how errors are currently being handled and what patterns we can follow.
Contributor
There was a problem hiding this comment.
@mena-rizkalla You could take a look at web-app for Http Status Error codes at: https://github.com/openMF/web-app/blob/dev/src/app/core/http/error-handler.interceptor.ts, Lines: 118-169
| Status Code | Error Type | Message Source | Handling Behavior |
|---|---|---|---|
| 400 | Bad Request | response.error.errors[0].defaultUserMessage or response.error.errors[0].developerMessage |
Shows alert with "Invalid parameters were passed in the request!" fallback |
| 401 | Authentication Error | Fixed message | Shows "Invalid User Details. Please try again!" |
| 403 | Unauthorized | errorMessage or fixed message |
Shows "You are not authorized for this request!" (special handling for invalid one-time token) |
| 404 | Not Found | errorMessage or fixed message |
Shows "Resource does not exist!" (silently handles client image 404s) |
| 500 | Internal Server Error | Fixed message | Shows "Internal Server Error. Please try again later." |
| 501 | Not Implemented | Translation keys | Shows translated "Not Implemented Error" message |
Also API uses standard Http Status Codes, https://fineract.apache.org/docs/legacy/index.html
| Status Code | Meaning | Typical Use Case |
|---|---|---|
| 200 | OK | Success |
| 400 | Bad Request | Validation errors, invalid parameters |
| 401 | Authentication Error | Invalid credentials |
| 403 | Unauthorized | Insufficient permissions |
| 404 | Not Found | Resource doesn't exist |
| 409 | Conflict | Concurrent modification, locked resources |
| 500 | Internal Server Error | Unexpected server errors |
- Introduce `BaseRepository` to centralize safe API calling and `Flow` to `DataState` conversions using a shared `CoroutineDispatcher`. - Refactor all repository implementations to extend `BaseRepository`, simplifying data fetching and reducing boilerplate `try-catch` and `withContext` blocks. - Enhance `MifosException` with specific error types (e.g., `BadRequest`, `Unauthorized`, `NotFound`, `SerializationError`) and update the `ExceptionMapper` to support these types. - Update `DataState.Error` to provide a default "Unknown error" message when an exception message is null. - Implement a centralized `handleError` function in several ViewModels (`RecentTransaction`, `SavingsApply`, `Transaction`, `Home`, `ShareApply`, `RepaymentSchedule`, `FillApplication`, `LoanApply`, `TransactionDetails`, `Notification`, `BeneficiaryApplication`) to unify UI error state updates. - Remove redundant `.catch` operators in ViewModels, relying on the repository layer to emit `DataState.Error`. - Update `safeDataStateCall` and `asDataStateFlow` in `DataState.kt` to support custom or default exception mapping and properly handle `CancellationException`.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/RecentTransactionRepositoryImp.kt (1)
27-42:⚠️ Potential issue | 🟠 MajorNull
clientIdbypasses Flow exception handling and breaks the contract.The
clientId!!assertion on line 33 throws synchronously before the Flow is created, so exceptions bypass the.catch { }block inasDataStateFlow(). This violates the return type contract:Flow<DataState<...>>should wrap all errors, not throw raw exceptions.Either validate
clientIdbefore building the Flow (wrapping null asDataState.Error), or change the interface parameter to non-nullable if null is never intended.Example fix
+import kotlinx.coroutines.flow.flowOf +import org.mifos.mobile.core.common.MifosException + override fun recentTransactions( clientId: Long?, offset: Int?, limit: Int?, ): Flow<DataState<Page<Transaction>>> { + if (clientId == null) { + return flowOf(DataState.Error(MifosException.GenericError("Client ID is required"))) + } + return dataManager.recentTransactionsApi.getRecentTransactionsList( - clientId!!, + clientId, offset, limit, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/RecentTransactionRepositoryImp.kt` around lines 27 - 42, The recentTransactions implementation currently uses clientId!! which throws before the Flow is created; change it to handle null safely inside the Flow contract: validate clientId at the start of recentTransactions and if null return a Flow<DataState<Page<Transaction>>> that emits a DataState.Error (or DataState.InvalidInput) instead of throwing, otherwise call dataManager.recentTransactionsApi.getRecentTransactionsList(clientId, offset, limit).map { ... }.asDataState() so all failures are propagated as DataState within the Flow; alternatively, if null is never valid, update the function signature to make clientId non-nullable everywhere instead of using !!.core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/HomeRepositoryImp.kt (1)
43-57:⚠️ Potential issue | 🟠 MajorMove response body parsing before
.asDataState()to ensure exceptions are properly caught.In
clientImage(), callingbodyAsText()andsubstringAfter()inside the.mapblock downstream of.asDataState()allows parsing failures to escape the catch block and cancel the Flow, instead of being wrapped asDataState.Error. Additionally, the final.flowOn(ioDispatcher)is redundant sinceasDataState()already applies it.Proposed fix
override fun clientImage(clientId: Long): Flow<DataState<String>> { return dataManager.clientsApi.getClientImage(clientId) + .map { response -> + val encodedString = response.bodyAsText() + encodedString.substringAfter(',') + } .asDataState() - .map { response -> - when (response) { - is DataState.Success -> { - val encodedString = response.data.bodyAsText() - val pureBase64Encoded = encodedString.substringAfter(',') - DataState.Success(pureBase64Encoded) - } - is DataState.Error -> DataState.Error(response.exception) - DataState.Loading -> DataState.Loading - } - } - .flowOn(ioDispatcher) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/HomeRepositoryImp.kt` around lines 43 - 57, In clientImage(), move the response body parsing (calling bodyAsText() and substringAfter(',')) to immediately after dataManager.clientsApi.getClientImage(clientId) and before invoking asDataState(), so that any exceptions from parsing are thrown inside the asDataState() try/catch and will be wrapped as DataState.Error; update the pipeline to map the already-parsed String inside the downstream map (handling Success/Error/Loading) and remove the redundant .flowOn(ioDispatcher) since asDataState() already applies the IO dispatcher.
🧹 Nitpick comments (2)
feature/beneficiary/src/commonMain/kotlin/org/mifos/mobile/feature/beneficiary/beneficiaryApplication/BeneficiaryApplicationViewModel.kt (1)
225-231: Consider: onlyNetworkErroris treated specially — other transport-like errors fall into "server error".The
MifosExceptionhierarchy also includesRequestTimeout,TooManyRequests,Unauthorized,NotFound,SerializationError, etc. Currently everything exceptNetworkErroris collapsed intoScreenUiState.Error(feature_generic_error_server), which may hide actionable distinctions (e.g., timeouts/rate-limit arguably deserve a retry UX closer to the network state, and auth errors may warrant a re-login path). Not a blocker — the behavior is consistent with other ViewModels in this PR — but worth considering as the exception taxonomy matures. Can be deferred to a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/beneficiary/src/commonMain/kotlin/org/mifos/mobile/feature/beneficiary/beneficiaryApplication/BeneficiaryApplicationViewModel.kt` around lines 225 - 231, The error handling in BeneficiaryApplicationViewModel currently collapses all non-NetworkError MifosExceptions into ScreenUiState.Error(feature_generic_error_server); update the when branch that inspects beneficiaryList and beneficiaryTemplate (and the handleError call) to pattern-match on specific MifosException subclasses (e.g., NetworkError, RequestTimeout, TooManyRequests, Unauthorized, NotFound, SerializationError) and map them to more appropriate ScreenUiState values — for example treat RequestTimeout and TooManyRequests similarly to network-related transient states (retryable/loading), map Unauthorized to a distinct auth error state to trigger re-auth flow, and keep a default server error for other cases; ensure to reference the DataState.Error.exception instances and preserve existing calls to handleError or replace them with the new ScreenUiState updates accordingly.core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/SavingsAccountRepositoryImp.kt (1)
36-45: Use explicit null validation for nullable account IDs instead of force-unwraps.While NPE thrown by the
!!operator on lines 41, 91, 100, and 108 is technically caught and wrapped inDataState.Errorby.catch()(for Flow) andsafeCall(for suspend functions), relying on exception-based error handling for invalid input is less clear than explicit validation. The repository interface accepts nullableaccountId, but the implementation unconditionally requires it—explicit null checks followed by early returns would make this contract clearer and produce more meaningful domain errors instead of generic null failures.💡 Suggested pattern
override fun getSavingsWithAssociations( accountId: Long?, associationType: String?, ): Flow<DataState<SavingsWithAssociations>> { + if (accountId == null) { + return flowOf(DataState.Error( + MifosException.GenericError("Savings account ID is required"), + )) + } + return dataManager.savingAccountsListApi.getSavingsWithAssociations( - accountId!!, + accountId, associationType, ) .map { it.toModel() } .asDataState() }Apply the same pattern to
updateSavingsAccount(line 91),submitWithdrawSavingsAccount(line 100), andaccountTransferTemplate(line 108).Also applies to: 86-101, 104-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/SavingsAccountRepositoryImp.kt` around lines 36 - 45, The implementation uses force-unwraps of the nullable accountId (e.g., in getSavingsWithAssociations, updateSavingsAccount, submitWithdrawSavingsAccount, accountTransferTemplate) which hides invalid-input intent; replace each !! use with an explicit null check at the start of the method: if accountId is null return an early error result (for suspend functions return DataState.Error with a clear domain message; for Flow-returning methods emit/return a Flow containing DataState.Error) so callers get a meaningful validation error instead of an NPE wrapped later. Ensure the error message describes the missing accountId and keep the subsequent calls using the non-null accountId variable after the check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/LoanRepositoryImp.kt`:
- Around line 33-39: The getLoanWithAssociations implementation currently
force-unwraps loanId with !! which throws before the Flow is built; change it to
handle nullable loanId safely: if loanId is null, immediately return a Flow that
emits a typed DataState.Error indicating "missing loanId" (or change the
repository contract to make loanId non-nullable), otherwise call
dataManager.loanAccountsListApi.getLoanWithAssociations(loanId,
associationType).map { it.toModel() }.asDataState(); apply the same fix to the
second occurrence around the safeCall block (lines noted in the review) so
missing IDs produce a domain-typed DataState.Error instead of throwing or
producing a generic error.
In
`@feature/notification/src/commonMain/kotlin/org/mifos/mobile/feature/notification/NotificationViewModel.kt`:
- Around line 61-63: handle the case where a refresh fails by clearing the
refresh flag: when DataState.Error is received in refreshNotifications (and in
the other Error path around handleError/lines referenced), ensure you set
_isRefreshing (the pull-to-refresh state) back to false in addition to updating
_notificationUiState; update handleError or add a small helper (referencing
handleError, refreshNotifications, _isRefreshing, _notificationUiState, and
DataState.Error) so both error paths call it to clear the refresh indicator and
set the UI error state.
---
Outside diff comments:
In
`@core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/HomeRepositoryImp.kt`:
- Around line 43-57: In clientImage(), move the response body parsing (calling
bodyAsText() and substringAfter(',')) to immediately after
dataManager.clientsApi.getClientImage(clientId) and before invoking
asDataState(), so that any exceptions from parsing are thrown inside the
asDataState() try/catch and will be wrapped as DataState.Error; update the
pipeline to map the already-parsed String inside the downstream map (handling
Success/Error/Loading) and remove the redundant .flowOn(ioDispatcher) since
asDataState() already applies the IO dispatcher.
In
`@core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/RecentTransactionRepositoryImp.kt`:
- Around line 27-42: The recentTransactions implementation currently uses
clientId!! which throws before the Flow is created; change it to handle null
safely inside the Flow contract: validate clientId at the start of
recentTransactions and if null return a Flow<DataState<Page<Transaction>>> that
emits a DataState.Error (or DataState.InvalidInput) instead of throwing,
otherwise call
dataManager.recentTransactionsApi.getRecentTransactionsList(clientId, offset,
limit).map { ... }.asDataState() so all failures are propagated as DataState
within the Flow; alternatively, if null is never valid, update the function
signature to make clientId non-nullable everywhere instead of using !!.
---
Nitpick comments:
In
`@core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/SavingsAccountRepositoryImp.kt`:
- Around line 36-45: The implementation uses force-unwraps of the nullable
accountId (e.g., in getSavingsWithAssociations, updateSavingsAccount,
submitWithdrawSavingsAccount, accountTransferTemplate) which hides invalid-input
intent; replace each !! use with an explicit null check at the start of the
method: if accountId is null return an early error result (for suspend functions
return DataState.Error with a clear domain message; for Flow-returning methods
emit/return a Flow containing DataState.Error) so callers get a meaningful
validation error instead of an NPE wrapped later. Ensure the error message
describes the missing accountId and keep the subsequent calls using the non-null
accountId variable after the check.
In
`@feature/beneficiary/src/commonMain/kotlin/org/mifos/mobile/feature/beneficiary/beneficiaryApplication/BeneficiaryApplicationViewModel.kt`:
- Around line 225-231: The error handling in BeneficiaryApplicationViewModel
currently collapses all non-NetworkError MifosExceptions into
ScreenUiState.Error(feature_generic_error_server); update the when branch that
inspects beneficiaryList and beneficiaryTemplate (and the handleError call) to
pattern-match on specific MifosException subclasses (e.g., NetworkError,
RequestTimeout, TooManyRequests, Unauthorized, NotFound, SerializationError) and
map them to more appropriate ScreenUiState values — for example treat
RequestTimeout and TooManyRequests similarly to network-related transient states
(retryable/loading), map Unauthorized to a distinct auth error state to trigger
re-auth flow, and keep a default server error for other cases; ensure to
reference the DataState.Error.exception instances and preserve existing calls to
handleError or replace them with the new ScreenUiState updates accordingly.
🪄 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: 3384f320-f76c-4d80-9931-7a64eb7ac1c3
📒 Files selected for processing (33)
core/common/src/commonMain/kotlin/org/mifos/mobile/core/common/DataState.ktcore/common/src/commonMain/kotlin/org/mifos/mobile/core/common/MifosException.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/AccountsRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/AuthenticationUserRepository.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/BaseRepository.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/BeneficiaryRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ClientChargeRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ClientRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/GuarantorRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/HomeRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/LoanRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/NotificationRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/RecentTransactionRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ReviewLoanApplicationRepositoryImpl.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/SavingsAccountRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ShareAccountRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ThirdPartyTransferRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/TransferRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/UserAuthRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/UserDetailRepositoryImp.ktcore/data/src/commonMain/kotlin/org/mifos/mobile/core/data/util/ExceptionMapper.ktfeature/accounts/src/commonMain/kotlin/org/mifos/mobile/feature/accounts/accountTransactions/TransactionViewModel.ktfeature/accounts/src/commonMain/kotlin/org/mifos/mobile/feature/accounts/transactionDetail/TransactionDetailsViewModel.ktfeature/beneficiary/src/commonMain/kotlin/org/mifos/mobile/feature/beneficiary/beneficiaryApplication/BeneficiaryApplicationViewModel.ktfeature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeViewModel.ktfeature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/loanAccountRepaymentSchedule/RepaymentScheduleViewModel.ktfeature/loan-application/src/commonMain/kotlin/org/mifos/mobile/feature/loan/application/loanApplication/LoanApplyViewModel.ktfeature/notification/src/commonMain/kotlin/org/mifos/mobile/feature/notification/NotificationViewModel.ktfeature/recent-transaction/src/commonMain/kotlin/org/mifos/mobile/feature/recent/transaction/viewmodel/RecentTransactionViewModel.ktfeature/savings-application/src/commonMain/kotlin/org/mifos/mobile/feature/savings/application/savingsApplication/SavingsApplyViewModel.ktfeature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/settings/SettingsViewModel.ktfeature/share-application/src/commonMain/kotlin/org/mifos/mobile/feature/share/application/fillApplication/FillApplicationViewModel.ktfeature/share-application/src/commonMain/kotlin/org/mifos/mobile/feature/share/application/shareApplication/ShareApplyViewModel.kt
💤 Files with no reviewable changes (1)
- feature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/settings/SettingsViewModel.kt
🚧 Files skipped from review as they are similar to previous changes (16)
- core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ClientRepositoryImp.kt
- feature/accounts/src/commonMain/kotlin/org/mifos/mobile/feature/accounts/accountTransactions/TransactionViewModel.kt
- feature/share-application/src/commonMain/kotlin/org/mifos/mobile/feature/share/application/shareApplication/ShareApplyViewModel.kt
- core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/GuarantorRepositoryImp.kt
- core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/AccountsRepositoryImp.kt
- core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/UserAuthRepositoryImp.kt
- core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/AuthenticationUserRepository.kt
- core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ShareAccountRepositoryImp.kt
- core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ThirdPartyTransferRepositoryImp.kt
- core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/BeneficiaryRepositoryImp.kt
- core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/util/ExceptionMapper.kt
- core/common/src/commonMain/kotlin/org/mifos/mobile/core/common/DataState.kt
- core/common/src/commonMain/kotlin/org/mifos/mobile/core/common/MifosException.kt
- core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/ReviewLoanApplicationRepositoryImpl.kt
- core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/UserDetailRepositoryImp.kt
- core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/TransferRepositoryImp.kt
Comment on lines
33
to
+39
| override fun getLoanWithAssociations( | ||
| associationType: String?, | ||
| loanId: Long?, | ||
| ): Flow<DataState<LoanWithAssociations?>> = flow { | ||
| try { | ||
| dataManager.loanAccountsListApi.getLoanWithAssociations(loanId!!, associationType) | ||
| .collect { response -> | ||
| emit(DataState.Success(response.toModel())) | ||
| } | ||
| } catch (exception: Exception) { | ||
| emit(DataState.Error(exception)) | ||
| } | ||
| }.flowOn(ioDispatcher) | ||
| ): Flow<DataState<LoanWithAssociations?>> = | ||
| dataManager.loanAccountsListApi.getLoanWithAssociations(loanId!!, associationType) | ||
| .map { response -> response.toModel() } | ||
| .asDataState() |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect repository/interface/call-site contracts for nullable loan IDs.
rg -n -C3 --type=kt '\b(getLoanWithAssociations|withdrawLoanAccount)\s*\('Repository: openMF/mifos-mobile
Length of output: 89
🏁 Script executed:
#!/bin/bash
# Fix: Use -g pattern correctly for Kotlin files
rg -n -C3 -g '*.kt' '(getLoanWithAssociations|withdrawLoanAccount)' --max-count=20Repository: openMF/mifos-mobile
Length of output: 19981
🏁 Script executed:
#!/bin/bash
# First, locate the file mentioned in the review
find . -type f -name "LoanRepositoryImp.kt" | head -5Repository: openMF/mifos-mobile
Length of output: 159
🏁 Script executed:
#!/bin/bash
# Get the actual file to verify the code at lines 33-39 and 51-57
cat -n core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/LoanRepositoryImp.kt | sed -n '30,60p'Repository: openMF/mifos-mobile
Length of output: 1365
🏁 Script executed:
#!/bin/bash
# Find safeCall definition
rg -n 'fun safeCall' --max-count=5 -A 5Repository: openMF/mifos-mobile
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Check if any call site passes null explicitly or checks for null before calling
rg -n '\.getLoanWithAssociations\(|\.withdrawLoanAccount\(' --max-count=30 -B 2 -A 2Repository: openMF/mifos-mobile
Length of output: 12068
🏁 Script executed:
#!/bin/bash
# Look for the LoanRepository interface definition more fully
head -50 core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repository/LoanRepository.ktRepository: openMF/mifos-mobile
Length of output: 1400
🏁 Script executed:
#!/bin/bash
# Find BaseRepository and safeCall definition
find . -type f -name "BaseRepository.kt" | head -3Repository: openMF/mifos-mobile
Length of output: 156
🏁 Script executed:
#!/bin/bash
# Search for safeCall in repository files
rg -n 'safeCall' --max-count=10 -A 3 -B 1Repository: openMF/mifos-mobile
Length of output: 16423
🏁 Script executed:
#!/bin/bash
# Check if null is ever explicitly passed as loanId in any call site
rg -n 'getLoanWithAssociations\s*\(|withdrawLoanAccount\s*\(' -A 3 -B 1 | grep -E '(null|loanId|withdrawLoanAccount)' | head -30Repository: openMF/mifos-mobile
Length of output: 3071
🏁 Script executed:
#!/bin/bash
# Read BaseRepository to understand safeCall and error handling
cat -n core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/BaseRepository.kt | head -60Repository: openMF/mifos-mobile
Length of output: 1647
🏁 Script executed:
#!/bin/bash
# Find safeDataStateCall definition
rg -n 'fun safeDataStateCall' -A 10 --max-count=5Repository: openMF/mifos-mobile
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Check state.accountId type - see if it's nullable or always populated
rg -n 'val accountId|var accountId|accountId:' --max-count=20 -B 1Repository: openMF/mifos-mobile
Length of output: 24599
🏁 Script executed:
#!/bin/bash
# Find safeDataStateCall definition and understand error handling
rg -n 'fun safeDataStateCall' -A 15 --max-count=5Repository: openMF/mifos-mobile
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Check if there are any other implementations or usages showing how NPE in safeCall is handled
rg -n 'NullPointerException|NPE|throw.*null' --max-count=10Repository: openMF/mifos-mobile
Length of output: 362
🏁 Script executed:
#!/bin/bash
# Search for safeDataStateCall - it might be in core/common
find . -type f -name "*.kt" | xargs grep -l 'safeDataStateCall' | head -5Repository: openMF/mifos-mobile
Length of output: 234
🏁 Script executed:
#!/bin/bash
# Check the core/common module structure
find core/common -type f -name "*.kt" | grep -i 'data\|state' | head -10Repository: openMF/mifos-mobile
Length of output: 224
🏁 Script executed:
#!/bin/bash
# Read DataStateExtensions.kt to see safeDataStateCall implementation
cat core/common/src/commonMain/kotlin/org/mifos/mobile/core/common/DataStateExtensions.ktRepository: openMF/mifos-mobile
Length of output: 3766
🏁 Script executed:
#!/bin/bash
# Also check DataState.kt to understand the structure
cat core/common/src/commonMain/kotlin/org/mifos/mobile/core/common/DataState.ktRepository: openMF/mifos-mobile
Length of output: 2217
🏁 Script executed:
#!/bin/bash
# Search for explicit null being passed to these methods
rg -n 'getLoanWithAssociations\s*\(' -B 2 -A 2 | grep -E '(null|loanId.*=|accountId.*=)' | head -20Repository: openMF/mifos-mobile
Length of output: 225
Avoid force-unwrapping nullable loan IDs before DataState handling.
Line 37 throws synchronously before the Flow is even constructed, so collectors will never receive DataState.Error. Line 56 is inside safeCall and thus caught, but produces a generic error instead of a typed domain error. Either make the repository contract non-null or return a typed DataState.Error for missing IDs.
🐛 Proposed fix
+import kotlinx.coroutines.flow.flowOf
+import org.mifos.mobile.core.common.MifosException
+
override fun getLoanWithAssociations(
associationType: String?,
loanId: Long?,
- ): Flow<DataState<LoanWithAssociations?>> =
- dataManager.loanAccountsListApi.getLoanWithAssociations(loanId!!, associationType)
+ ): Flow<DataState<LoanWithAssociations?>> {
+ val id = loanId ?: return flowOf(
+ DataState.Error(MifosException.GenericError("Loan id is required")),
+ )
+
+ return dataManager.loanAccountsListApi.getLoanWithAssociations(id, associationType)
.map { response -> response.toModel() }
.asDataState()
+ }
override suspend fun withdrawLoanAccount(
loanId: Long?,
loanWithdraw: LoanWithdraw?,
- ): DataState<String> = safeCall {
+ ): DataState<String> {
+ val id = loanId ?: return DataState.Error(
+ MifosException.GenericError("Loan id is required"),
+ )
+
+ return safeCall {
dataManager.loanAccountsListApi
- .withdrawLoanAccount(loanId!!, loanWithdraw?.toDto())
+ .withdrawLoanAccount(id, loanWithdraw?.toDto())
.bodyAsText()
+ }
}Also applies to: 51-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@core/data/src/commonMain/kotlin/org/mifos/mobile/core/data/repositoryImpl/LoanRepositoryImp.kt`
around lines 33 - 39, The getLoanWithAssociations implementation currently
force-unwraps loanId with !! which throws before the Flow is built; change it to
handle nullable loanId safely: if loanId is null, immediately return a Flow that
emits a typed DataState.Error indicating "missing loanId" (or change the
repository contract to make loanId non-nullable), otherwise call
dataManager.loanAccountsListApi.getLoanWithAssociations(loanId,
associationType).map { it.toModel() }.asDataState(); apply the same fix to the
second occurrence around the safeCall block (lines noted in the review) so
missing IDs produce a domain-typed DataState.Error instead of throwing or
producing a generic error.
Comment on lines
+61
to
+63
| private fun handleError(exception: Throwable) { | ||
| _notificationUiState.value = NotificationUiState.Error(exception.message) | ||
| } |
Contributor
There was a problem hiding this comment.
Clear the refresh state on errors.
After refreshNotifications() sets _isRefreshing to true, the new DataState.Error path only updates the UI error state. A failed refresh will leave the pull-to-refresh indicator stuck.
Proposed fix
private fun handleError(exception: Throwable) {
+ _isRefreshing.value = false
- _notificationUiState.value = NotificationUiState.Error(exception.message)
+ _notificationUiState.value = NotificationUiState.Error(exception.message ?: "Unknown error")
}Also applies to: 75-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/notification/src/commonMain/kotlin/org/mifos/mobile/feature/notification/NotificationViewModel.kt`
around lines 61 - 63, handle the case where a refresh fails by clearing the
refresh flag: when DataState.Error is received in refreshNotifications (and in
the other Error path around handleError/lines referenced), ensure you set
_isRefreshing (the pull-to-refresh state) back to false in addition to updating
_notificationUiState; update handleError or add a small helper (referencing
handleError, refreshNotifications, _isRefreshing, _notificationUiState, and
DataState.Error) so both error paths call it to clear the refresh indicator and
set the UI error state.
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.
MifosExceptionsealed class to categorize errors intoServerError,ClientError,NetworkError, andGenericError.ExceptionMapperutility to convertThrowableand Ktor exceptions intoMifosExceptiontypes, including a suspend version for async error extraction.asDataStateFlowextension to support custom exception mapping.BeneficiaryRepositoryImp,LoanRepositoryImp,SavingsAccountRepositoryImp, etc.) to use the new exception mapping logic.MifosExceptiontypes instead of raw IO or Ktor exceptions for UI state management.Fixes - Jira-#577
Please Add Screenshots If there are any UI changes.
Please make sure these boxes are checked before submitting your pull request - thanks!
Run the static analysis check
./gradlew checkorci-prepush.shto make sure you didn't break anythingIf you have multiple commits please combine them into one commit by squashing them.
Summary by CodeRabbit