refactor(note): add DataState, optimize Flow, add DTOs, entities, domain models - #2666
Merged
therajanmaurya merged 25 commits intoMay 5, 2026
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:
📝 WalkthroughWalkthroughAdded a new Changes
Sequence DiagramsequenceDiagram
participant Caller
participant RunAsDataState_Network as runAsDataState\n(network-aware)
participant NetworkMonitor
participant RunAsDataState_Basic as runAsDataState\n(basic)
participant Dispatcher as CoroutineDispatcher
participant DataState
Caller->>RunAsDataState_Network: invoke(networkMonitor, context?, block)
RunAsDataState_Network->>NetworkMonitor: isOnline.first()
alt Offline
NetworkMonitor-->>RunAsDataState_Network: false
RunAsDataState_Network->>DataState: Error(NetworkUnavailableException())
DataState-->>Caller: DataState.Error
else Online
NetworkMonitor-->>RunAsDataState_Network: true
RunAsDataState_Network->>RunAsDataState_Basic: invoke(context?, block)
rect rgba(100, 150, 200, 0.5)
RunAsDataState_Basic->>Dispatcher: withContext(context) [optional]
RunAsDataState_Basic->>RunAsDataState_Basic: execute block()
end
alt Success
RunAsDataState_Basic->>DataState: Success(result)
else Exception caught
RunAsDataState_Basic->>DataState: Error(exception)
end
DataState-->>Caller: DataState<T>
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
core/data/src/commonMain/kotlin/com/mifos/core/data/util/NetworkUnavailableException.kt (1)
15-17: Change toExceptionas the base type for this domain error.Inheriting from
Throwableis overly broad and bypasses the exception handlers that catchExceptionthroughout the codebase (over 80 instances across feature, core, and library modules). For app-level failures like offline state,Exceptionis the appropriate choice.♻️ Proposed change
class NetworkUnavailableException( message: String = "Network is unavailable. Please check your connection and try again.", -) : Throwable(message) +) : Exception(message)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/commonMain/kotlin/com/mifos/core/data/util/NetworkUnavailableException.kt` around lines 15 - 17, Update the domain error class NetworkUnavailableException to extend Exception instead of Throwable so it is caught by existing exception handlers; locate the class declaration for NetworkUnavailableException and change its base type from Throwable to Exception, keeping the message parameter and behavior unchanged.
🤖 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/com/mifos/core/data/util/AsDataStateExtension.kt`:
- Around line 47-48: The catch block in the asDataState extension currently
swallows all Throwables and returns DataState.Error(e), which breaks structured
concurrency; update the catch (e: Throwable) in AsDataStateExtension (the
asDataState extension function) to rethrow
kotlinx.coroutines.CancellationException (i.e., if e is CancellationException
then throw e) and only convert other exceptions into DataState.Error(e).
---
Nitpick comments:
In
`@core/data/src/commonMain/kotlin/com/mifos/core/data/util/NetworkUnavailableException.kt`:
- Around line 15-17: Update the domain error class NetworkUnavailableException
to extend Exception instead of Throwable so it is caught by existing exception
handlers; locate the class declaration for NetworkUnavailableException and
change its base type from Throwable to Exception, keeping the message parameter
and behavior unchanged.
🪄 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: a3f81870-2fc8-4b40-9df4-f54e6f3b6ef6
📒 Files selected for processing (2)
core/data/src/commonMain/kotlin/com/mifos/core/data/util/AsDataStateExtension.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/util/NetworkUnavailableException.kt
Contributor
|
@coderabbitai Improve PR title based on github.com/openMF/mobile-wallet/wiki/Commit-style-guide. Give us just the title. |
|
Tip For best results, initiate chat on the files or code changes.
|
biplab1
changed the title
There was a problem hiding this comment.
♻️ Duplicate comments (1)
core/data/src/commonMain/kotlin/com/mifos/core/data/util/DataStateUtils.kt (1)
47-48:⚠️ Potential issue | 🟠 MajorRe-throw coroutine cancellation instead of wrapping it into
DataState.Error.Line 47–48 currently catches
Throwable; this swallowsCancellationExceptionand can break cancellation/timeout propagation.🛠️ Proposed fix
import com.mifos.core.common.utils.DataState +import kotlinx.coroutines.CancellationException import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.flow.first import kotlinx.coroutines.withContext @@ - } catch (e: Throwable) { + } catch (e: CancellationException) { + throw e + } catch (e: Exception) { DataState.Error(e) }#!/bin/bash # Verify cancellation handling in DataState utilities. rg -nP --type=kotlin 'catch\s*\(\s*\w+\s*:\s*Throwable\s*\)' core/data/src/commonMain/kotlin/com/mifos/core/data/util -C3 rg -nP --type=kotlin 'CancellationException' core/data/src/commonMain/kotlin/com/mifos/core/data/util -C3🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/commonMain/kotlin/com/mifos/core/data/util/DataStateUtils.kt` around lines 47 - 48, The catch block currently swallows CancellationException by catching Throwable and returning DataState.Error(e); change it to preserve coroutine cancellation by detecting CancellationException and rethrowing it (or only catch Exception instead of Throwable) before wrapping other failures into DataState.Error so cancellation/timeouts propagate correctly; update the catch in DataStateUtils.kt where DataState.Error(e) is returned to rethrow CancellationException and only wrap non-cancellation errors.
🧹 Nitpick comments (1)
core/data/src/commonMain/kotlin/com/mifos/core/data/util/DataStateUtils.kt (1)
81-81: Consider usingStateFlow<Boolean>inNetworkMonitorfor explicit initial-value guarantees.The current implementation emits an initial value before async setup (Android sends synchronously at line 64; other platforms use
flowOf(true)), sofirst()won't suspend. However, usingStateFlowwould make this guarantee explicit in the type system and prevent future regression if the initial emission is accidentally removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/commonMain/kotlin/com/mifos/core/data/util/DataStateUtils.kt` at line 81, The check using networkMonitor.isOnline.first() relies on an initial emission but that contract isn't explicit; change NetworkMonitor.isOnline from a Flow<Boolean> to a StateFlow<Boolean> (or expose a StateFlow wrapper) so callers like the code in DataStateUtils.kt can rely on an immediate initial value. Update the NetworkMonitor interface/type to return StateFlow<Boolean>, ensure all platform implementations initialize a concrete MutableStateFlow with the correct initial value (e.g., true/false as appropriate), and adjust usages (e.g., the call in DataStateUtils.kt) to use the new StateFlow<Boolean> without changing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@core/data/src/commonMain/kotlin/com/mifos/core/data/util/DataStateUtils.kt`:
- Around line 47-48: The catch block currently swallows CancellationException by
catching Throwable and returning DataState.Error(e); change it to preserve
coroutine cancellation by detecting CancellationException and rethrowing it (or
only catch Exception instead of Throwable) before wrapping other failures into
DataState.Error so cancellation/timeouts propagate correctly; update the catch
in DataStateUtils.kt where DataState.Error(e) is returned to rethrow
CancellationException and only wrap non-cancellation errors.
---
Nitpick comments:
In `@core/data/src/commonMain/kotlin/com/mifos/core/data/util/DataStateUtils.kt`:
- Line 81: The check using networkMonitor.isOnline.first() relies on an initial
emission but that contract isn't explicit; change NetworkMonitor.isOnline from a
Flow<Boolean> to a StateFlow<Boolean> (or expose a StateFlow wrapper) so callers
like the code in DataStateUtils.kt can rely on an immediate initial value.
Update the NetworkMonitor interface/type to return StateFlow<Boolean>, ensure
all platform implementations initialize a concrete MutableStateFlow with the
correct initial value (e.g., true/false as appropriate), and adjust usages
(e.g., the call in DataStateUtils.kt) to use the new StateFlow<Boolean> without
changing behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 28697f6e-f26c-4db4-911e-ad1146d54b04
📒 Files selected for processing (1)
core/data/src/commonMain/kotlin/com/mifos/core/data/util/DataStateUtils.kt
biplab1
reviewed
Apr 13, 2026
biplab1
reviewed
Apr 13, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
feature/offline/src/commonMain/kotlin/com/mifos/feature/offline/syncGroupPayloads/SyncGroupPayloadsViewModel.kt (1)
122-127: Remove nullable parameter and non-null assertion here.
groupPayloadis nullable but force-unwrapped at Line 127. Make it non-null at the function boundary to prevent accidental NPE regressions.Based on learnings: when downstream requires non-null, avoid nullable parameters with force-unwrapping and add explicit null-safety.Proposed refactor
- private fun syncGroupPayload(groupPayload: GroupPayloadEntity?) { + private fun syncGroupPayload(groupPayload: GroupPayloadEntity) { viewModelScope.launch { _syncGroupPayloadsUiState.value = SyncGroupPayloadsUiState.Loading - when (val result = repository.createGroup(groupPayload!!)) { + when (val result = repository.createGroup(groupPayload)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/offline/src/commonMain/kotlin/com/mifos/feature/offline/syncGroupPayloads/SyncGroupPayloadsViewModel.kt` around lines 122 - 127, Change the syncGroupPayload function signature to accept a non-null GroupPayloadEntity (remove the nullable type) and delete the force-unwrap (!!) inside the function; update any callers to ensure they pass a non-null GroupPayloadEntity or perform null-checks before calling syncGroupPayload so repository.createGroup(groupPayload) is always invoked with a non-null value (refer to syncGroupPayload and repository.createGroup for locating the code).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@feature/offline/src/commonMain/kotlin/com/mifos/feature/offline/syncGroupPayloads/SyncGroupPayloadsViewModel.kt`:
- Around line 127-137: The error branch currently calls updateGroupPayload()
regardless of exception type, which advances sync even on
NetworkUnavailableException; change the logic in the DataState.Error branch
handling repository.createGroup(groupPayload!!) so that after setting
_syncGroupPayloadsUiState.value = SyncGroupPayloadsUiState.Error(errorMessage)
you only call updateGroupPayload() when result.exception is NOT a
NetworkUnavailableException (i.e., skip the updateGroupPayload() call for
NetworkUnavailableException) to stop progression while offline.
---
Nitpick comments:
In
`@feature/offline/src/commonMain/kotlin/com/mifos/feature/offline/syncGroupPayloads/SyncGroupPayloadsViewModel.kt`:
- Around line 122-127: Change the syncGroupPayload function signature to accept
a non-null GroupPayloadEntity (remove the nullable type) and delete the
force-unwrap (!!) inside the function; update any callers to ensure they pass a
non-null GroupPayloadEntity or perform null-checks before calling
syncGroupPayload so repository.createGroup(groupPayload) is always invoked with
a non-null value (refer to syncGroupPayload and repository.createGroup for
locating the code).
🪄 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: cdd8ab05-ebf7-4fb5-93a1-0e27e17f7e75
📒 Files selected for processing (4)
core/data/src/commonMain/kotlin/com/mifos/core/data/repository/SyncGroupPayloadsRepository.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/SyncGroupPayloadsRepositoryImp.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/util/DataStateUtils.ktfeature/offline/src/commonMain/kotlin/com/mifos/feature/offline/syncGroupPayloads/SyncGroupPayloadsViewModel.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- core/data/src/commonMain/kotlin/com/mifos/core/data/util/DataStateUtils.kt
itsPronay
force-pushed
the
datastate-extension
branch
from
April 13, 2026 18:35
57cd2f8 to
35166d7
Compare
biplab1
changed the title
Contributor
|
@itsPronay Please upload screen recording of the feature screen - with and without network. |
biplab1
changed the title
biplab1
changed the title
Contributor
|
@Arinyadav1 @itsPronay Could you please address the @coderabbitai suggestions relevant to this PR? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 60164
🟠 Major (Actionable) Issues1. Re-throw The import com.mifos.core.common.utils.DataState
+import kotlinx.coroutines.CancellationException
import kotlinx.coroutines.CoroutineDispatcher
import kotlinx.coroutines.flow.first
import kotlinx.coroutines.withContext
- } catch (e: Throwable) {
+ } catch (e: CancellationException) {
+ throw e
+ } catch (e: Exception) {
DataState.Error(e)
}2. Stop sync progression on offline error in
- is DataState.Error -> {
- val errorMessage = if (result.exception is NetworkUnavailableException) {
- Res.string.feature_offline_error_not_connected_internet
- } else {
- Res.string.feature_offline_error_group_sync_failed
- }
- _syncGroupPayloadsUiState.value = SyncGroupPayloadsUiState.Error(errorMessage)
- updateGroupPayload()
- }
+ is DataState.Error -> {
+ if (result.exception is NetworkUnavailableException) {
+ _syncGroupPayloadsUiState.value = SyncGroupPayloadsUiState.Error(
+ Res.string.feature_offline_error_not_connected_internet,
+ )
+ return@launch
+ }
+ _syncGroupPayloadsUiState.value = SyncGroupPayloadsUiState.Error(
+ Res.string.feature_offline_error_group_sync_failed,
+ )
+ updateGroupPayload()
+ }🧹 Nitpick Suggestions3. class NetworkUnavailableException(
message: String = "Network is unavailable. Please check your connection and try again.",
-) : Throwable(message)
+) : Exception(message)4. - private fun syncGroupPayload(groupPayload: GroupPayloadEntity?) {
+ private fun syncGroupPayload(groupPayload: GroupPayloadEntity) {
viewModelScope.launch {
_syncGroupPayloadsUiState.value = SyncGroupPayloadsUiState.Loading
- when (val result = repository.createGroup(groupPayload!!)) {
+ when (val result = repository.createGroup(groupPayload)) {5. ❌ Pre-merge Check FailureDocstring Coverage: Currently at 18.18%, below the required 80% threshold. Please add KDoc comments to the new public functions ( |
biplab1
reviewed
Apr 27, 2026
Contributor
Shouldn't the screen stay where it is when clicking the submit button, after typing the note, since there is a "no internet" which is shown below instead of showing the try again page? Showing a try again page indicates the note being typed was lost, however, it is not. When the network is restored it has the note, which was just added, in the background. The UX could be improved a bit. This could be handled in a separate PR, I think. |
biplab1
reviewed
Apr 27, 2026
Contributor
|
Could you please take a look at this issue: #2666 (comment) |
biplab1
reviewed
May 5, 2026
Member
|
@biplab1 do the final review |
biplab1
reviewed
May 5, 2026
biplab1
reviewed
May 5, 2026
biplab1
reviewed
May 5, 2026
biplab1
reviewed
May 5, 2026
|
Contributor
|
@Arinyadav1 Have you tested the screens if they are working fine after the recent updates? Please upload a new screen recording. |
biplab1
approved these changes
May 5, 2026
biplab1
left a comment
biplab1
left a comment
Contributor
There was a problem hiding this comment.
Looks good to me. This can be merged.
biplab1
changed the title
biplab1
changed the title
biplab1
changed the title
sahilshivekar
pushed a commit
to sahilshivekar/mifos-x-field-officer-app
that referenced
this pull request
May 7, 2026
…dels (openMF#2666) Co-authored-by: Arin Yadav <115909232+Arinyadav1@users.noreply.github.com> Co-authored-by: Arin Yadav <arinyadav98@gmail.com>
sahilshivekar
pushed a commit
to sahilshivekar/mifos-x-field-officer-app
that referenced
this pull request
May 7, 2026
…dels (openMF#2666) Co-authored-by: Arin Yadav <115909232+Arinyadav1@users.noreply.github.com> Co-authored-by: Arin Yadav <arinyadav98@gmail.com>
PRAteek-singHWY
added a commit
to PRAteek-singHWY/mifos-x-field-officer-app
that referenced
this pull request
May 8, 2026
Apply the DataState + NetworkMonitor + DispatcherManager pattern established in PR openMF#2666 (note module refactor) to the close-loan flow: - LoanService.closeLoanAccount: change Flow<GenericResponse> to suspend GenericResponse to match PR openMF#2666's service-layer style. - DataManagerLoan.closeLoanAccount: mirror as suspend GenericResponse. - CloseLoanRepository: action methods now return DataState<Unit>; observation flow unchanged (Flow<DataState<LoanTransactionTemplate?>>). - CloseLoanRepositoryImp: inject NetworkMonitor and DispatcherManager; use runAsDataState() for actions and withNetworkCheck()/flowOn(io) for observation, matching NoteRepositoryImp. - CloseLoanUseCase: simplify by delegating to the DataState-returning repository; sync remains best-effort.
gurnoorpannu
added a commit
to gurnoorpannu/mifos-x-field-officer-app
that referenced
this pull request
May 9, 2026
- Split RejectLoanPayload into RejectLoanInput (domain) + RejectLoanRequestDto (network); drop RejectLoanResponse domain class in favor of typed RejectLoanResponseDto consumed by Ktorfit. - LoanService.rejectLoan returns the typed DTO directly; DataManagerLoan becomes a thin pass-through (no manual HttpResponse / bodyAsText / Json.decodeFromString). - LoanAccountRejectRepository returns DataState<Unit> via runAsDataState(networkMonitor, dispatcher.io); connectivity check moves out of the ViewModel into the repository. - Add RejectLoanUseCase as a suspend pass-through; register in UseCaseModule. - ViewModel injects the use case (not the repo), drops NetworkMonitor / networkConnection / isLoading state, surfaces results through Action.Internal.ReceiveRejectResult(DataState<Unit>) per the new pattern, and renames the discard guard to PreventAccidentalBack. - Screen drives loading from DialogState.Loading (single source of truth) and uses the renamed dialog state. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
gurnoorpannu
added a commit
to gurnoorpannu/mifos-x-field-officer-app
that referenced
this pull request
May 9, 2026
The post-openMF#2666 typed-DTO pass-through hid backend validation messages. Without expectSuccess on the Ktor client, 4xx/5xx responses were either silently decoded into an empty RejectLoanResponseDto (with ignoreUnknownKeys) — surfacing as a fake success — or thrown as a SerializationException with a developer-facing message. Either way, Fineract's defaultUserMessage (e.g. "Loan cannot be rejected as it is not in submitted state") never reached the UI. Mirror the convention already used by submitLoanReschedule, submitAccountTransfer, and createLoansAccount in this same file: LoanService.rejectLoan returns the raw HttpResponse, and DataManagerLoan.rejectLoan checks status, runs extractErrorMessage on failure, and decodes RejectLoanResponseDto on success. The repository contract (DataState<Unit>) and all upstream layers are unchanged. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
gurnoorpannu
added a commit
to gurnoorpannu/mifos-x-field-officer-app
that referenced
this pull request
May 9, 2026
- Split RejectLoanPayload into RejectLoanInput (domain) + RejectLoanRequestDto (network); drop RejectLoanResponse domain class in favor of typed RejectLoanResponseDto consumed by Ktorfit. - LoanService.rejectLoan returns the typed DTO directly; DataManagerLoan becomes a thin pass-through (no manual HttpResponse / bodyAsText / Json.decodeFromString). - LoanAccountRejectRepository returns DataState<Unit> via runAsDataState(networkMonitor, dispatcher.io); connectivity check moves out of the ViewModel into the repository. - Add RejectLoanUseCase as a suspend pass-through; register in UseCaseModule. - ViewModel injects the use case (not the repo), drops NetworkMonitor / networkConnection / isLoading state, surfaces results through Action.Internal.ReceiveRejectResult(DataState<Unit>) per the new pattern, and renames the discard guard to PreventAccidentalBack. - Screen drives loading from DialogState.Loading (single source of truth) and uses the renamed dialog state.
gurnoorpannu
added a commit
to gurnoorpannu/mifos-x-field-officer-app
that referenced
this pull request
May 9, 2026
The post-openMF#2666 typed-DTO pass-through hid backend validation messages. Without expectSuccess on the Ktor client, 4xx/5xx responses were either silently decoded into an empty RejectLoanResponseDto (with ignoreUnknownKeys) — surfacing as a fake success — or thrown as a SerializationException with a developer-facing message. Either way, Fineract's defaultUserMessage (e.g. "Loan cannot be rejected as it is not in submitted state") never reached the UI. Mirror the convention already used by submitLoanReschedule, submitAccountTransfer, and createLoansAccount in this same file: LoanService.rejectLoan returns the raw HttpResponse, and DataManagerLoan.rejectLoan checks status, runs extractErrorMessage on failure, and decodes RejectLoanResponseDto on success. The repository contract (DataState<Unit>) and all upstream layers are unchanged.
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
5 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.
JIRA - https://mifosforge.jira.com/browse/MIFOSAC-769
Screen Recording uploaded to Slack by @Arinyadav1:
datastate-flow-dto-entity-domain-models.mp4
Summary by CodeRabbit