enhance new saving account flow #2529
Conversation
WalkthroughCentralized HTTP error extraction and unified UI error/dialog state; updated datamanagers to throw on non-success responses, changed several navigation onFinish callbacks to parameterless, integrated SnackbarHostState for success messages, and adjusted related strings and composable signatures. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as SavingsAccountScreen
participant VM as SavingsAccountViewModel
participant API as Network
rect rgb(220,240,200)
Note over UI,VM: Unified submit flow (DialogState -> Snackbar)
User->>UI: Submit form
UI->>VM: SavingsAccountEvent.Finish()
VM->>API: createSavingsAccount()
alt HTTP success (2xx)
API-->>VM: HttpResponse
VM->>VM: set DialogState.SuccessResponseStatus(true,msg)
VM-->>UI: state with DialogState.SuccessResponseStatus
UI->>UI: Show snackbar(msg)
Note over UI: delay(1s) then finish
UI->>UI: onFinish()
else HTTP non-success
API-->>VM: HttpResponse (non-2xx)
VM->>VM: extractErrorMessage -> throw IllegalStateException(msg)
VM-->>UI: ScreenState.Error(message)
UI->>UI: Display error UI
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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: 2
🧹 Nitpick comments (6)
feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/pages/DetailsPage.kt (2)
28-29: External ID should not use a Decimal keyboard.
IDs are commonly text/alphanumeric; Decimal restricts input and harms UX. Prefer Text (or Number only if strictly numeric).Apply:
- keyboardOptions = KeyboardOptions( - keyboardType = KeyboardType.Decimal, - ), + keyboardOptions = KeyboardOptions( + keyboardType = KeyboardType.Text, + ),Also applies to: 39-39, 160-163
172-172: Enable “Next” only when required selections are made.
Right now it’s enabled if a date exists (default non-empty). Consider also requiring product and officer to reduce error-cycles.- isSecondButtonEnabled = state.submissionDate.isNotEmpty(), + isSecondButtonEnabled = state.submissionDate.isNotEmpty() && + state.savingsProductSelected != -1 && + state.fieldOfficerIndex != -1,feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountScreen.kt (2)
175-186: Don’t detect success by comparing a localized message string.
This is fragile across locales and copy changes. Track success via state instead.Screen change:
- if (state.responseErrorMsg != null) { + if (state.responseErrorMsg != null) { LaunchedEffect(state.launchEffectKey) { snackbarHostState.showSnackbar( message = state.responseErrorMsg, ) - - if (state.responseErrorMsg == getString(Res.string.feature_savings_new_savings_account_created_successfully)) { - delay(1000) - onAction(SavingsAccountAction.Finish) - } + if (state.createSuccess == true) { + delay(1000) + onAction(SavingsAccountAction.Finish) + } } }Complementary ViewModel/state changes are proposed below in the ViewModel comment.
Also applies to: 16-16, 66-66
314-316: Avoid string concatenation for UI text — prefer a single formatted resource.
Concatenation impairs localization. Suggest adding a resource like “%1$s %2$s” or “%1$s %2$s”/“%1$s” with placeholders.feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountViewModel.kt (2)
175-207: Make success explicit instead of parsing/echoing messages.
Current flow relies on extractErrorMessage == MSG_NOT_FOUND and later compares a localized string in the UI. Replace with an explicit success flag and keep the message purely for display.Apply minimal changes:
@@ data class SavingsAccountState( - val responseErrorMsg: String? = null, - val launchEffectKey: Int? = null, + val responseErrorMsg: String? = null, + val createSuccess: Boolean? = null, + val launchEffectKey: Int? = null, @@ submitSavingsApplication() - is DataState.Success -> { - val error = extractErrorMessage(result.data) - // Successful create lona account if response error not found - if (error == Error.MSG_NOT_FOUND) { - mutableStateFlow.update { - it.copy( - responseErrorMsg = getString(Res.string.feature_savings_new_savings_account_created_successfully), - launchEffectKey = Random.nextInt(), - isOverLayLoadingActive = false, - ) - } - } else { - mutableStateFlow.update { - it.copy( - isOverLayLoadingActive = false, - responseErrorMsg = error, - launchEffectKey = Random.nextInt(), - ) - } - } - } + is DataState.Success -> { + val error = extractErrorMessage(result.data) + val (msg, success) = + if (error == Error.MSG_NOT_FOUND) + getString(Res.string.feature_savings_new_savings_account_created_successfully) to true + else error to false + mutableStateFlow.update { + it.copy( + responseErrorMsg = msg, + createSuccess = success, + launchEffectKey = Random.nextInt(), + isOverLayLoadingActive = false, + ) + } + }Then use createSuccess in the Screen as suggested earlier.
Also applies to: 188-207, 195-199
224-226: Optional: avoid getString in the ViewModel; let UI resolve resources.
Pass StringResource to UI and convert with stringResource/getString there. Reduces UI coupling inside the VM.Example shape:
- val responseErrorMsg: String? = null, + val responseMsgRes: StringResource? = null,And set responseMsgRes = Res.string.feature_savings_error_not_connected_internet, etc., resolving in the Screen.
Also applies to: 583-586, 599-601
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
core/data/src/commonMain/kotlin/com/mifos/core/data/repository/SavingsAccountRepository.kt(2 hunks)core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/SavingsAccountRepositoryImp.kt(2 hunks)core/domain/src/commonMain/kotlin/com/mifos/core/domain/useCases/CreateSavingsAccountUseCase.kt(2 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerSavings.kt(2 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt(1 hunks)feature/savings/src/commonMain/composeResources/values/feature_savings_strings.xml(1 hunks)feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/navigation/SavingsNavigation.kt(1 hunks)feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountRoute.kt(1 hunks)feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountScreen.kt(12 hunks)feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountViewModel.kt(18 hunks)feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/pages/ChargesPage.kt(0 hunks)feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/pages/DetailsPage.kt(6 hunks)feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/pages/PreviewPage.kt(0 hunks)feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/pages/TermsPage.kt(1 hunks)
💤 Files with no reviewable changes (2)
- feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/pages/PreviewPage.kt
- feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/pages/ChargesPage.kt
🧰 Additional context used
🧬 Code graph analysis (1)
feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountScreen.kt (2)
mifosng-android/src/main/java/com/mifos/mifosxdroid/AndroidClientViewModel.kt (1)
onFinish(124-126)core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosSweetError.kt (1)
MifosSweetError(40-83)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR Checks / Static Analysis Check
🔇 Additional comments (14)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt (1)
342-342: Navigation callback simplified topopBackStack.The change from a navigation callback with ID parameter to a simple
popBackStackcall is cleaner and aligns with the updatedonFinishsignature throughout the savings flow.feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/navigation/SavingsNavigation.kt (1)
31-31:onFinishcallback signature simplified.The callback no longer requires a savings account ID parameter, aligning with the navigation flow simplification across the savings feature.
feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountRoute.kt (1)
25-25: LGTM!The
onFinishcallback signature change is consistent with the navigation flow simplification.core/domain/src/commonMain/kotlin/com/mifos/core/domain/useCases/CreateSavingsAccountUseCase.kt (1)
26-27: Use case signature updated consistently.The return type change aligns with the repository interface update, maintaining consistency across the domain layer.
core/data/src/commonMain/kotlin/com/mifos/core/data/repository/SavingsAccountRepository.kt (1)
38-38: LGTM!The repository interface signature change is consistent with the architectural decision to return raw HTTP responses.
feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/pages/TermsPage.kt (1)
304-306: No issues found—validation is properly handled in ViewModel.The
handleOnTermSubmit()function (lines 231-280) validates both currency selection and decimal places constraints before allowing progression. Validation errors are properly stored in state (currencyError,decimalPlacesError) and displayed in the UI (TermsPage lines 89, 98-99). Invalid data is caught beforemoveToNextStep()is invoked, preventing network calls with incomplete or invalid input. The refactoring from page-level to ViewModel-level validation follows a sound architectural pattern.core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerSavings.kt (1)
287-289: Verify HTTP-status handling & error parsing for createSavingsAccountcreateSavingsAccount() currently emits HttpResponse through the flow and is wrapped into DataState.Success by asDataStateFlow without inspecting HTTP status; confirm that the consumer(s) either validate response.status (2xx vs 4xx/5xx) and parse error bodies (use extractErrorMessage) before treating the call as success.
- What I verified: DataManagerSavings.createSavingsAccount returns Flow (core/network/.../DataManagerSavings.kt:287–289); SavingsAccountRepositoryImp forwards it as DataState via .asDataStateFlow (core/data/.../SavingsAccountRepositoryImp.kt:54–56); asDataStateFlow simply emits Success(it) and only converts thrown exceptions to Error (core/common/.../DataState.kt:36–39). extractErrorMessage(response) exists for parsing error bodies (core/data/.../ErrorHandling.kt:20–26). Some repos already check status (e.g., ClientDetailsRepositoryImp checks res.status.value == 200).
- Required action: Either
- map HttpResponse → success/error at repository level (check status.isSuccess()/200–299 and call extractErrorMessage on non‑2xx) so callers receive a clean DataState.Success(domain) or DataState.Error(Exception), OR
- ensure the viewmodels that consume CreateSavingsAccountUseCase immediately validate response.status and parse error bodies before updating UI (review feature/savings/.../SavingAccountViewModel.kt — createSavingsAccount handler ~lines 119–131 — and feature/savings/.../savingsAccountv2/SavingsAccountViewModel.kt — createSavingsAccountUseCase collection ~lines 176–188).
- Recommendation: Prefer centralizing HTTP status → DataState mapping in repository to avoid duplicated checks across viewmodels.
feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/pages/DetailsPage.kt (1)
125-125: Dropdown field-level errors — LGTM.
Propagating errorMessage from state is clean and keeps validation local to the field.Also applies to: 150-151
feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountScreen.kt (4)
135-141: SnackbarHostState integration — LGTM.
Remember + scaffold host wiring looks correct and side-effect is properly keyed.
247-249: Safe indexing and null-safe mapping — LGTM.
Using getOrNull and elvis avoids crashes on invalid indices and missing names/times.Also applies to: 262-266
282-289: Date formatting on selection — LGTM.
Guard + DateHelper conversion is clear and safe.
75-75: All call sites for onFinish signature change are correctly updated.The verification confirms:
- The NEW SavingsAccountScreen in
savingsAccountv2/(line 72-75) includes theonFinish: () -> Unitparameter- Its only call site in
SavingsAccountRoute.kt(line 28-31) correctly passesonFinish- The OLD SavingsAccountScreen in
savingsAccount/has noonFinishparameter, and its call site inSavingsNavigation.kt(line 97-99) correctly omits itNo mismatches or incomplete updates exist.
feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountViewModel.kt (1)
231-254: Client-side validation — LGTM.
Currency/decimal bounds and details page validations are straightforward and clear.Also applies to: 256-280
feature/savings/src/commonMain/composeResources/values/feature_savings_strings.xml (1)
147-147: Verification complete — no issues found. Code changes are correct.All string resource changes have been verified:
- Old keys (
feature_savings_new_savings_account_submitted_success/_failed) are completely removed with no dangling references- New key
feature_savings_new_savings_account_created_successfullyis defined and actively used in SavingsAccountViewModel.kt and SavingsAccountScreen.kt- New key
field_empty_msgis defined and actively used in SavingsAccountViewModel.kt (validation logic)- Existing key
feature_savings_charges_active_countcontinues to be properly used
| override fun createSavingsAccount(savingsPayload: SavingsPayload?): Flow<DataState<HttpResponse>> { | ||
| return dataManagerSavings.createSavingsAccount(savingsPayload) | ||
| .asDataStateFlow() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify HttpResponse handling in the presentation layer.
The return type change from Savings to HttpResponse shifts parsing responsibility from the data layer to the presentation layer. Ensure that:
- HTTP status codes are properly checked
- Response body parsing handles all success/error cases
- Error messages are user-friendly
Run the following script to locate where the HttpResponse from createSavingsAccount is consumed and verify proper handling:
🏁 Script executed:
#!/bin/bash
# Description: Find usages of createSavingsAccount use case to verify HttpResponse handling
# Search for CreateSavingsAccountUseCase invocations
rg -n -C5 'CreateSavingsAccountUseCase|createSavingsAccount' --type=kt -g '!*Repository*' -g '!*DataManager*'Length of output: 26
I need to run a corrected search script. The kt file type isn't recognized by ripgrep; let me search using the file extension pattern instead.
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for createSavingsAccount usages across Kotlin files (presentation/UI layer)
# Exclude Repository and DataManager files to find where it's actually consumed
rg -n -C5 'createSavingsAccount' --glob='*.kt' --glob='!*Repository*' --glob='!*DataManager*'
Length of output: 20950
🏁 Script executed:
#!/bin/bash
# Also search for CreateSavingsAccountUseCase class/interface usage patterns
rg -n -C3 'CreateSavingsAccountUseCase' --glob='*.kt' --glob='!*Repository*' --glob='!*DataManager*'Length of output: 7506
🏁 Script executed:
#!/bin/bash
# Search for where HttpResponse is being parsed or handled after createSavingsAccount calls
rg -n -B2 -A8 'createSavingsAccount.*\(' --glob='*.kt' | head -100Length of output: 13397
🏁 Script executed:
#!/bin/bash
# Find ViewModel or Presenter classes that might use this use case
fd -e kt --type f | xargs rg -l 'ViewModel\|Presenter' | head -20Length of output: 0
I need to inspect the full error/success handling in the ViewModels and understand how asDataStateFlow() transforms the HttpResponse.
---
🏁 Script executed:
```shell
#!/bin/bash
# Get the complete createSavingsAccount method from SavingAccountViewModel.kt
rg -n -A20 'fun createSavingsAccount\(savingsPayload' --glob='*.kt' --type-list
Length of output: 389
🏁 Script executed:
#!/bin/bash
# Search for asDataStateFlow implementation to understand HttpResponse transformation
rg -n -B3 -A10 'fun.*asDataStateFlow|\.asDataStateFlow' --glob='*.kt' | head -60Length of output: 7552
🏁 Script executed:
#!/bin/bash
# Look for DataState.Success handling in savings ViewModels
rg -n -C3 'DataState\.Success' feature/savings --glob='*.kt'Length of output: 16633
🏁 Script executed:
#!/bin/bash
# Check if there's any HTTP status code validation (200, 201, etc.)
rg -n 'statusCode|status|code.*2[0-9]{2}|isSuccessful' feature/savings --glob='*.kt' | head -20Length of output: 3439
🏁 Script executed:
#!/bin/bash
# Find where HttpResponse body is being accessed/parsed
rg -n -B2 -A5 'HttpResponse|\.body|\.data' feature/savings --glob='*.kt' | head -40Length of output: 6211
I need to examine the utility functions that handle HttpResponse transformation and error extraction.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find asDataStateFlow implementation
fd -e kt --type f -path '*/core/common/*' | xargs rg -n 'fun.*asDataStateFlow|asDataStateFlow.*=' --max-count=1
Length of output: 302
🏁 Script executed:
#!/bin/bash
# Find extractErrorMessage implementation
fd -e kt --type f | xargs rg -n -B2 -A10 'fun extractErrorMessage' Length of output: 1552
🏁 Script executed:
#!/bin/bash
# Check what HttpResponse looks like
rg -n -B3 -A15 'class HttpResponse|data class HttpResponse' --glob='*.kt'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Verify if old SavingAccountViewModel or new v2 is the primary one
rg -n 'SavingAccountViewModel|SavingsAccountViewModel' core/domain --glob='*.kt' | grep -i 'inject\|provide'Length of output: 0
I need to examine the complete asDataStateFlow() implementation to see if it validates HTTP status codes.
---
🏁 Script executed:
```shell
#!/bin/bash
# Get complete asDataStateFlow implementation
rg -n -B5 -A20 'fun <T> Flow<T>.asDataStateFlow' core/common --glob='*.kt'
Length of output: 989
🏁 Script executed:
#!/bin/bash
# Check DataState class structure to understand Success/Error states
rg -n -B2 -A25 'sealed class DataState|class DataState' core/common --glob='*.kt' | head -80Length of output: 2619
🏁 Script executed:
#!/bin/bash
# Look for any status code validation in data layer or network utilities
rg -n 'status\.code|statusCode|isSuccess' core/data core/network --glob='*.kt' | head -30Length of output: 455
🏁 Script executed:
#!/bin/bash
# Verify which ViewModel is actually being used in dependency injection
fd -e kt | xargs rg -n 'SavingAccountViewModel|SavingsAccountViewModel' | grep -E 'bind|provide|factory' | head -10Length of output: 0
Critical HttpResponse handling issues confirmed in the codebase.
The review comment's concerns are valid. The problems identified:
-
No HTTP status code validation: The
asDataStateFlow()function (core/common/utils/DataState.kt:36-39) wraps all successful HTTP connections asDataState.Successwithout checking status codes. It only catches exceptions—responses with 4xx/5xx status codes are treated as success if the HTTP connection itself succeeds. -
Error detection relies solely on response body parsing:
extractErrorMessage()(core/data/util/ErrorHandling.kt:20-30) parses the response body as JSON without validating the HTTP status code. If the status is 4xx/5xx but the body doesn't parse as aMifosError, it returnsError.MSG_NOT_FOUND, creating ambiguity. -
Presentation layer workarounds:
SavingsAccountViewModel(v2, line 192) uses the patternif (error == Error.MSG_NOT_FOUND)as a proxy for detecting success—indicating the response is wrapped inDataState.Successeven for failed requests. -
Generic error messages: If
extractErrorMessage()fails to parse the response body, it returnsError.FAILED_TO_PARSE_ERROR_RESPONSErather than a user-friendly message. -
Type inconsistency: The older
SavingAccountViewModel.kt(line 135) passes aSavings()object to the success state, while the use case returnsHttpResponse—indicating incomplete refactoring or deprecated code still in use.
🤖 Prompt for AI Agents
In
core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/SavingsAccountRepositoryImp.kt
around lines 54-57, the repository currently returns raw HttpResponse wrapped as
success; update it to validate the HTTP status code and return DataState.Error
for non-2xx responses: call
dataManagerSavings.createSavingsAccount(savingsPayload), inspect response.status
(or response.status.value) and if in 200..299 parse and map the response body to
the domain Savings model and emit DataState.Success(mappedModel); otherwise call
extractErrorMessage(response) and emit DataState.Error with that message and the
status code. Also modify extractErrorMessage to first check the response status
and, for non-2xx responses, attempt structured error parsing but fall back to a
clear, user-friendly message that includes status and raw body when parsing
fails. Finally update any ViewModel usages to expect DataState.Error on non-2xx
responses (remove reliance on Error.MSG_NOT_FOUND) and ensure the success path
carries the domain Savings object rather than a raw HttpResponse.
.../src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountViewModel.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientIdentifiersAddUpdate/ClientIdentifiersAddUpdateViewModel.kt (1)
285-309: Reset the overlay loader when document creation fails.
createDocumentsetsisOverlayLoading = trueduring loading, but the error branch never clears it. Hitting a backend error leaves the overlay spinner permanently visible and the screen unusable. Please toggle it off on failure as well.is DataState.Error -> { mutableStateFlow.update { it.copy( dialogState = ClientIdentifiersAddUpdateState.DialogState.Error(state.message), + isOverlayLoading = false, ) } }core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerIdentifiers.kt (1)
84-108: Update outdated documentation.The KDoc comment at line 92 states the return type is
[GenericResponse], but the actual return type isFlow<HttpResponse>. Please update the documentation to reflect the current signature.The error handling implementation follows the same correct pattern as
DataManagerLoan.ktand properly validates response status before extracting error messages.Apply this diff to fix the documentation:
/** * Creates a new identifier for a given client. * * REST END POINT: * https://demo.openmf.org/fineract-provider/api/v1/clients/{clientId}/identifiers * * @param clientId The unique ID of the client. * @param identifierPayload The payload containing identifier details. - * @return [GenericResponse] indicating the result of the create operation. + * @return [Flow] emitting [HttpResponse] for the create operation. */
🧹 Nitpick comments (2)
feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountViewModel.kt (1)
38-38: Consider using a counter instead of Random for launchEffectKey.While collisions are unlikely, using
Random.nextInt()forlaunchEffectKey(lines 191, 204) is non-deterministic. A simple incrementing counter would be more predictable and eliminate any collision risk.Apply this diff:
-import kotlin.random.Random +And in the ViewModel class, add:
+ private var launchEffectCounter = 0Then replace
Random.nextInt()with++launchEffectCounter:- launchEffectKey = Random.nextInt(), + launchEffectKey = ++launchEffectCounter,feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountScreen.kt (1)
198-209: Consider reducing the success delay or making it configurable.The 1-second delay (line 205) before finishing allows users to read the success message, but it might feel slow in some contexts. Consider reducing it to 500-800ms, or making it configurable if different flows have different requirements.
- delay(1000) + delay(800) // Shorter delay for better perceived performance
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
core/common/src/commonMain/kotlin/com/mifos/core/common/utils/ErrorHandling.kt(2 hunks)core/data/src/commonMain/kotlin/com/mifos/core/data/repository/ClientIdentifiersRepository.kt(1 hunks)core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/ClientDetailsRepositoryImp.kt(1 hunks)core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/ClientIdentifiersRepositoryImp.kt(1 hunks)core/domain/src/commonMain/kotlin/com/mifos/core/domain/useCases/ActivateGroupUseCase.kt(1 hunks)core/domain/src/commonMain/kotlin/com/mifos/core/domain/useCases/CreateClientIdentifierUseCase.kt(1 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/apis/ClientIdentifierApi.kt(1 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerIdentifiers.kt(2 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.kt(5 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerSavings.kt(2 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientIdentifiersAddUpdate/ClientIdentifiersAddUpdateViewModel.kt(5 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/ShareAccountScreen.kt(0 hunks)feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountScreen.kt(4 hunks)feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt(2 hunks)feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountScreen.kt(14 hunks)feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountViewModel.kt(17 hunks)
💤 Files with no reviewable changes (1)
- feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/ShareAccountScreen.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerSavings.kt
🧰 Additional context used
🧬 Code graph analysis (4)
core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.kt (1)
core/common/src/commonMain/kotlin/com/mifos/core/common/utils/ErrorHandling.kt (1)
extractErrorMessage(20-31)
core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerIdentifiers.kt (1)
core/common/src/commonMain/kotlin/com/mifos/core/common/utils/ErrorHandling.kt (1)
extractErrorMessage(20-31)
feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountScreen.kt (2)
mifosng-android/src/main/java/com/mifos/mifosxdroid/AndroidClientViewModel.kt (1)
onFinish(124-126)core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosSweetError.kt (1)
MifosSweetError(40-83)
feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountViewModel.kt (1)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt (4)
handleChooseChargeIndexChange(324-328)handleDeleteCharge(366-373)handleEditChargeDialog(375-389)moveToNextStep(753-764)
🔇 Additional comments (14)
core/domain/src/commonMain/kotlin/com/mifos/core/domain/useCases/ActivateGroupUseCase.kt (1)
13-13: LGTM! Import path refactoring aligns with centralization effort.The import path change from
com.mifos.core.data.utiltocom.mifos.core.common.utilsis appropriate and aligns with the broader effort to centralize error handling utilities across the project.core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/ClientDetailsRepositoryImp.kt (1)
13-13: Import refactoring verified and approved.The
extractErrorMessagefunction has been successfully moved tocore/common/utils.ErrorHandling, with all imports updated consistently across the codebase. All call sites inClientDetailsRepositoryImp.ktare within suspend function contexts, making them compatible with the suspend function signature. The refactoring is complete with no duplicates or breaking changes.feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountViewModel.kt (4)
196-216: Clarify the IllegalStateException handling logic.The error handling distinguishes between
IllegalStateException(displayed via dialog) and other exceptions (displayed as screen errors). This suggests different severities or recovery paths, but the rationale isn't clear from the code.Could you clarify:
- Why does
IllegalStateExceptionreceive dialog treatment while other exceptions show a full screen error?- Is this exception type expected from the
createSavingsAccountUseCase, or is this defensive coding?If
IllegalStateExceptionindicates a specific, recoverable validation failure from the backend, consider adding a comment explaining this distinction.
254-278: LGTM! Clean validation logic.The validation pattern is consistent, clear, and provides good user feedback by setting specific error messages for each invalid field.
432-437: Excellent UX pattern for error clearing.Clearing field-specific errors when the user modifies the field provides immediate feedback and improves the user experience. This consistent pattern across all validated fields is well-implemented.
Also applies to: 506-511, 515-520, 549-554
229-252: <!-- [scratchpad_start]
[task verification 1/1 complete]
Verify decimal places range validation (0-6) at line 233[observations]
- Line 233:
isDecimalInvalid = decimalPlaces == null || decimalPlaces < 0 || decimalPlaces > 6- Hard-coded upper limit of 6 with no comments explaining rationale
- Source:
result.data.currency?.decimalPlaces?.toInt()from backend template (line 461)- SavingsProductTemplate loaded but contains no decimal places range constraints
- Other features (loan SchedulePage, client ChargeDialog) use decimal places dynamically without hard limits
- Mifos documentation describes decimal places as a product-level setting without specifying 0-6 range
- No config files, constants, or documentation found defining this specific range in the codebase
[analysis]
- Range appears isolated to savings feature only
- No backend constraints or business rules found to support 0-6 limit
- Loan feature directly uses
currency.decimalPlacesfrom backend with no validation ceiling- Charge dialog dynamically validates against provided decimal places value
- Mifos architecture treats decimal places as flexible product setting
- Hard-coded constraint lacks supporting documentation or centralized configuration
[pending]
- Cannot definitively determine if 0-6 is correct backend constraint
- Cannot confirm if this is intentional business rule or oversight
- Evidence is inconclusive but strongly suggests undocumented/potentially incorrect constraint
[actions]
Generate response flagging need for developer verification and documentation[done]
Thorough search of codebase, configuration, and documentation completed. No supporting evidence found for 0-6 range constraint.
[scratchpad_end]Document or verify the hard-coded 0-6 decimal places constraint.
The validation at line 233 enforces
decimalPlaces > 6without explanation. No supporting business rules, backend constraints, or configuration found in the codebase. Other features (loan, client charges) use decimal places dynamically without this limit. Confirm whether this range is intentional and documented, or if it should be removed/made configurable to match backend capabilities.feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountScreen.kt (3)
86-86: LGTM! Proper SnackbarHostState integration.The
SnackbarHostStateis correctly hoisted usingremember, and properly threaded through the composable hierarchy. This follows Compose best practices for state management.Also applies to: 91-91, 99-99, 109-109, 142-142, 183-183
250-251: Good null-safety improvements.The defensive null checks using
getOrNull()and elvis operators prevent potential crashes when accessing charge options, improving robustness.Also applies to: 264-268
73-73: Callback signature change is correctly implemented; no issues found.The
onFinishcallback signature change from(id: Int) -> Unitto() -> Unithas been consistently propagated throughout the entire call chain. The handler atClientNavigation.kt:342usesnavController::popBackStack, which takes no parameters and returns Unit, making the client ID unnecessary. All intermediate layers (SavingsNavigation,SavingsAccountRoute, andSavingsAccountScreen) correctly pass the callback without modification, and the screen implementation calls it without arguments.core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.kt (3)
62-68: LGTM! Flow wrapping is correct.The
flow { emit(...) }wrapper correctly converts the suspend function result into a Flow for reactive consumption.
90-91: LGTM! Formatting improvement.The multi-line assignment improves readability.
104-115: LGTM! Centralized error handling implemented correctly.The error handling pattern is well-structured:
- Maps over the Flow to validate HTTP response status
- Extracts user-friendly error messages for non-success responses
- Throws IllegalStateException to propagate errors through the Flow
- Response body remains intact for successful responses
This aligns with the centralized error-handling approach across the codebase.
core/data/src/commonMain/kotlin/com/mifos/core/data/repository/ClientIdentifiersRepository.kt (1)
33-36: LGTM! Interface updated for Flow-based pattern.The signature change from
suspend funreturningHttpResponsetofunreturningFlow<DataState<HttpResponse>>aligns with the project-wide migration to reactive Flow-based APIs with state management.core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/ClientIdentifiersRepositoryImp.kt (1)
46-51: LGTM! Repository implementation correctly updated.The implementation properly:
- Removes the
suspendmodifier to match the interface- Returns
Flow<DataState<HttpResponse>>by wrapping the data manager's Flow withasDataStateFlow()- Maintains consistency with other methods in the repository
...n/com/mifos/feature/client/clientIdentifiersAddUpdate/ClientIdentifiersAddUpdateViewModel.kt
Show resolved
Hide resolved
| val snackbarHostState = remember { SnackbarHostState() } | ||
|
|
||
| NewLoanAccountDialogs( | ||
| state = state, | ||
| onAction = remember(viewModel) { { viewModel.trySendAction(it) } }, | ||
| snackbarHostState = snackbarHostState, | ||
| ) | ||
|
|
||
| NewLoanAccountScaffold( | ||
| modifier = modifier, | ||
| state = state, | ||
| onAction = remember(viewModel) { { viewModel.trySendAction(it) } }, | ||
| navController = navController, | ||
| snackbarHostState = snackbarHostState, | ||
| ) |
There was a problem hiding this comment.
Remove blocking snackbar await before finishing
SnackbarHostState.showSnackbar suspends until the snackbar disappears, so the new flow now waits through the entire snackbar timeout (≈4–10 seconds depending on accessibility) before the one‑second delay even starts. That regresses the completion timing compared to the prior immediate finish and forces users to wait far longer than intended.(android.googlesource.com)
Trigger showSnackbar in a child coroutine so the finish path can proceed on its original schedule:
@@
-import androidx.compose.runtime.remember
+import androidx.compose.runtime.remember
+import kotlinx.coroutines.launch
@@
- is NewLoanAccountState.DialogState.SuccessResponseStatus -> {
- LaunchedEffect(state.launchEffectKey) {
- snackbarHostState.showSnackbar(
- message = state.dialogState.msg,
- )
-
- if (state.dialogState.successStatus) {
- delay(1000)
- onAction(NewLoanAccountAction.Finish)
- }
- }
- }
+ is NewLoanAccountState.DialogState.SuccessResponseStatus -> {
+ LaunchedEffect(state.launchEffectKey) {
+ launch {
+ snackbarHostState.showSnackbar(
+ message = state.dialogState.msg,
+ )
+ }
+ if (state.dialogState.successStatus) {
+ delay(1000)
+ onAction(NewLoanAccountAction.Finish)
+ }
+ }
+ }Also applies to: 235-246
🤖 Prompt for AI Agents
In
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountScreen.kt
around lines 98-112 (and also apply same change at 235-246), the call to
snackbarHostState.showSnackbar is invoked directly which suspends the current
coroutine and blocks the finish path until the snackbar duration elapses;
instead, launch showSnackbar in a child coroutine so the surrounding flow can
continue immediately (e.g., use a coroutineScope.launch /
LaunchedEffect/rememberCoroutineScope and call showSnackbar inside that launched
coroutine) and keep the original one-second finish delay unaffected.
| mutableStateFlow.update { | ||
| it.copy( | ||
| isOverLayLoadingActive = false, | ||
| launchEffectKey = Random.nextInt(), | ||
| dialogState = NewLoanAccountState.DialogState.SuccessResponseStatus( | ||
| successStatus = true, | ||
| msg = getString(Res.string.feature_loan_account_created_successfully), | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Don’t call suspend getString inside a non-suspending update lambda.
MutableStateFlow.update expects a synchronous lambda, so invoking the suspend getString inside it will not compile. Compute the string before calling update.
- mutableStateFlow.update {
+ val successMessage =
+ getString(Res.string.feature_loan_account_created_successfully)
+ mutableStateFlow.update {
it.copy(
isOverLayLoadingActive = false,
launchEffectKey = Random.nextInt(),
dialogState = NewLoanAccountState.DialogState.SuccessResponseStatus(
successStatus = true,
- msg = getString(Res.string.feature_loan_account_created_successfully),
+ msg = successMessage,
),
)
}Compose Multiplatform marks getString as a suspend function, so it has to run from a suspend context, not inside a plain lambda. (jetbrains.com)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| mutableStateFlow.update { | |
| it.copy( | |
| isOverLayLoadingActive = false, | |
| launchEffectKey = Random.nextInt(), | |
| dialogState = NewLoanAccountState.DialogState.SuccessResponseStatus( | |
| successStatus = true, | |
| msg = getString(Res.string.feature_loan_account_created_successfully), | |
| ), | |
| ) | |
| val successMessage = | |
| getString(Res.string.feature_loan_account_created_successfully) | |
| mutableStateFlow.update { | |
| it.copy( | |
| isOverLayLoadingActive = false, | |
| launchEffectKey = Random.nextInt(), | |
| dialogState = NewLoanAccountState.DialogState.SuccessResponseStatus( | |
| successStatus = true, | |
| msg = successMessage, | |
| ), | |
| ) | |
| } |
🤖 Prompt for AI Agents
In
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt
around lines 308 to 316, the update lambda calls the suspend function getString
which is illegal because MutableStateFlow.update expects a synchronous lambda;
fix by calling getString from the surrounding suspend context before invoking
update (e.g., compute val successMsg =
getString(Res.string.feature_loan_account_created_successfully) first) and then
call mutableStateFlow.update { it.copy(isOverLayLoadingActive = false,
launchEffectKey = Random.nextInt(), dialogState =
NewLoanAccountState.DialogState.SuccessResponseStatus(successStatus = true, msg
= successMsg)) }.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerIdentifiers.kt (1)
94-106: Fix the KDoc documentation mismatch and clarify the intentional pattern differences.The documentation at line 92 is incorrect: it states
@return [GenericResponse]but the actual return type isHttpResponse. This must be corrected.Regarding the error-handling approach: throwing
IllegalStateExceptionis an established pattern in this codebase—it's already used inDataManagerLoan.ktandDataManagerSavings.ktfor the same purpose. However, the inconsistency withupdateClientIdentifier(line 119) anddeleteClientIdentifier(line 80), which returnGenericResponsewithout error checking, reflects a fundamental difference in the underlying API signatures (the API itself definescreateClientIdentifierto returnHttpResponsewhile the others returnGenericResponse).If error checking should be applied consistently across all three operations, consider updating
updateClientIdentifieranddeleteClientIdentifierto follow the same pattern. Otherwise, document whycreateClientIdentifierdiffers.Fix the KDoc:
- * @return [GenericResponse] indicating the result of the create operation. + * @return [HttpResponse] indicating the result of the create operation.
🧹 Nitpick comments (1)
core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerIdentifiers.kt (1)
21-21: Remove unused import.The
mapimport is not used anywhere in this file.Apply this diff:
-import kotlinx.coroutines.flow.map
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerIdentifiers.kt(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerIdentifiers.kt (1)
core/common/src/commonMain/kotlin/com/mifos/core/common/utils/ErrorHandling.kt (1)
extractErrorMessage(20-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR Checks / Static Analysis Check
2 participants
Fixes - Jira-#609
Summary by CodeRabbit
Bug Fixes
New Features
Behavior Changes