feat(loan): Implement undo approval screen#2665
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:
WalkthroughAdds an end-to-end "undo loan approval" feature: API endpoint, network datamanager, repository method and implementation, request model, DI/viewModel registration, navigation route, Compose screen and dialog, resizable text field component, string resource, and related build/podspec and network utilities. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as LoanUndoApprovalScreen
participant VM as LoanUndoApprovalViewModel
participant Repo as LoanAccountApprovalRepository
participant DML as DataManagerLoan
participant API as LoanService
User->>UI: Enter note and tap Submit
UI->>VM: dispatch OnSubmit
VM->>Repo: undoLoanApproval(loanId, LoanUndoApprovalRequest)
Repo->>DML: dataManagerLoan.undoLoanApproval(loanId, noteRequest)
DML->>API: POST /loans/{loanId}?command=undoApproval (body: note)
API-->>DML: HttpResponse (success/failure)
DML-->>Repo: return or throw on error
Repo-->>VM: DataState.Success or DataState.Error
VM-->>UI: update dialogState or emit NavigationBack
UI-->>User: show success (navigate back) or error dialog
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosOutlinedTextField.kt (1)
516-530: Applymodifierat the container level to keep handle and field aligned.On Line 517, the outer
Boxignores the incomingmodifier, while the text field uses it on Line 528. With caller padding/constraints, the drag handle and text field can drift out of alignment.♻️ Suggested adjustment
- Box( - modifier = Modifier - .fillMaxWidth(), - ) { + Box( + modifier = modifier.fillMaxWidth(), + ) { OutlinedTextField( value = value, onValueChange = { onValueChange(it) }, @@ - modifier = modifier + modifier = Modifier .fillMaxWidth() .height(fieldMinHeight),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosOutlinedTextField.kt` around lines 516 - 530, The Box container is not applying the incoming modifier while OutlinedTextField does, causing misalignment between the drag handle and the field when callers supply padding/constraints; move or apply the incoming modifier to the Box (instead of only to OutlinedTextField) so the same Modifier passed into MifosOutlinedTextField is used on the container (Box) and keep fieldMinHeight applied to the inner OutlinedTextField to preserve sizing—update the Box usage and remove the duplicate modifier usage on OutlinedTextField to ensure the handle and field remain aligned (refer to Box, OutlinedTextField, modifier, and fieldMinHeight).
🤖 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/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosOutlinedTextField.kt`:
- Around line 539-540: The resize handle in MifosOutlinedTextField uses
.size(DesignToken.sizes.iconAverage) with a pointerInput, which makes the touch
target too small; update the pointerInput hit area to meet accessibility
guidance by increasing the interactive area (e.g., use a larger size like
DesignToken.sizes.iconLarge or wrap the pointerInput with a minimum touch
target/padding modifier) while keeping the visual icon size unchanged if needed;
locate the call site with .size(DesignToken.sizes.iconAverage) and
pointerInput(Unit) and replace or augment it so the touch target is at least the
recommended 48.dp.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanUndoApproval/LoanUndoApprovalRoute.kt`:
- Around line 22-28: The navigateToLoanUndoApprovalScreen navigation helper is
defined but never called, leaving LoanUndoApprovalRoute unreachable; wire this
navigation into the UI by adding a user-triggered callback that calls
navigateToLoanUndoApprovalScreen(loanId) from the existing entry points such as
loanAccountSummary or loanProfileAccountDestination (where the route is
registered and the function is imported), e.g., expose an onUndoApprovalClick or
include the call inside the relevant button/option handler so the NavController
uses LoanUndoApprovalRoute(loanId) to navigate when the user initiates
undo-approval.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanUndoApproval/LoanUndoApprovalViewModel.kt`:
- Around line 31-37: The OnSubmit and OnRetry handlers call undoLoanApproval()
even when a request is already in flight, risking duplicate backend writes;
modify the handlers in LoanUndoApprovalViewModel (the branches handling
LoanUndoApprovalAction.OnSubmit and LoanUndoApprovalAction.OnRetry) to first
check the view state loading flag (e.g., isLoading/isUndoing) or an in-flight
guard and return early if a request is active, and ensure undoLoanApproval()
itself sets that guard (or state.isLoading) before starting the async work and
clears it on completion/error to prevent re-entrancy.
- Line 57: The dialog currently sets dialogState =
LoanUndoApprovalState.DialogState.Error(error.message.toString()), which can
render the literal "null" when error.message is null; update the dialogState
assignment in LoanUndoApprovalViewModel so it supplies a safe default (e.g., use
error.message ?: "Unknown error" or error.localizedMessage ?: "Unknown error")
instead of calling toString() on a nullable, so
LoanUndoApprovalState.DialogState.Error always receives a non-"null"
human-readable message.
---
Nitpick comments:
In
`@core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosOutlinedTextField.kt`:
- Around line 516-530: The Box container is not applying the incoming modifier
while OutlinedTextField does, causing misalignment between the drag handle and
the field when callers supply padding/constraints; move or apply the incoming
modifier to the Box (instead of only to OutlinedTextField) so the same Modifier
passed into MifosOutlinedTextField is used on the container (Box) and keep
fieldMinHeight applied to the inner OutlinedTextField to preserve sizing—update
the Box usage and remove the duplicate modifier usage on OutlinedTextField to
ensure the handle and field remain aligned (refer to Box, OutlinedTextField,
modifier, and fieldMinHeight).
🪄 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: 614b8863-b87e-4cb9-94d5-3b3fd9e40c9e
📒 Files selected for processing (12)
core/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanAccountApprovalRepository.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanAccountApprovalRepositoryImp.ktcore/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosOutlinedTextField.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/LoanUndoApprovalRequest.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.ktfeature/loan/src/commonMain/composeResources/values/strings.xmlfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/di/LoanModule.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanUndoApproval/LoanUndoApprovalRoute.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanUndoApproval/LoanUndoApprovalScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanUndoApproval/LoanUndoApprovalViewModel.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanUndoApproval/LoanUndoApprovalViewModel.kt (2)
55-55: Function should be private.
undoLoanApproval()is only called from withinhandleAction(). Making itpublicexposes an internal implementation detail unnecessarily.♻️ Suggested fix
- fun undoLoanApproval() { + private fun undoLoanApproval() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanUndoApproval/LoanUndoApprovalViewModel.kt` at line 55, The method undoLoanApproval() is public but only used internally from handleAction(), so change its visibility to private: locate the undoLoanApproval() function in LoanUndoApprovalViewModel and mark it private, and ensure handleAction() (or any internal callers) still reference it unchanged; run tests/compile to confirm no external usages rely on this method being public.
91-103: Consider usingdata objectfor singleton sealed class members.Kotlin 1.9+ introduces
data objectwhich provides bettertoString()output and consistentequals/hashCodebehavior for singleton sealed members. This is a minor stylistic improvement.♻️ Suggested changes
sealed interface DialogState { data class Error(val message: String) : DialogState - object Loading : DialogState + data object Loading : DialogState } } sealed interface LoanUndoApprovalEvent { - object NavigationBack : LoanUndoApprovalEvent + data object NavigationBack : LoanUndoApprovalEvent } sealed interface LoanUndoApprovalAction { data class OnNoteChange(val note: String) : LoanUndoApprovalAction - object OnSubmit : LoanUndoApprovalAction - object OnRetry : LoanUndoApprovalAction - object OnNavigateBack : LoanUndoApprovalAction + data object OnSubmit : LoanUndoApprovalAction + data object OnRetry : LoanUndoApprovalAction + data object OnNavigateBack : LoanUndoApprovalAction }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanUndoApproval/LoanUndoApprovalViewModel.kt` around lines 91 - 103, Replace plain singleton objects with Kotlin 1.9+ data objects for better toString/equals/hashCode: change object Loading (implementing DialogState), object NavigationBack in LoanUndoApprovalEvent, and the singleton members OnSubmit, OnRetry, OnNavigateBack of sealed interface LoanUndoApprovalAction to use `data object` syntax; update their declarations only (no behavior changes) so they remain singletons but gain data-object semantics.
🤖 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/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanUndoApproval/LoanUndoApprovalViewModel.kt`:
- Around line 63-80: The when branch in LoanUndoApprovalViewModel handling
DataState.Loading is unreachable because repository.undoLoanApproval (called in
repository.undoLoanApproval(route.loanId, LoanUndoApprovalRequest(...))) is a
suspend function returning a single DataState value, so remove the
DataState.Loading -> Unit arm from the when expression and leave only the
DataState.Error and DataState.Success cases (or replace with a sealed when
without Loading), ensuring the when remains exhaustive or compile-clean.
---
Nitpick comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanUndoApproval/LoanUndoApprovalViewModel.kt`:
- Line 55: The method undoLoanApproval() is public but only used internally from
handleAction(), so change its visibility to private: locate the
undoLoanApproval() function in LoanUndoApprovalViewModel and mark it private,
and ensure handleAction() (or any internal callers) still reference it
unchanged; run tests/compile to confirm no external usages rely on this method
being public.
- Around line 91-103: Replace plain singleton objects with Kotlin 1.9+ data
objects for better toString/equals/hashCode: change object Loading (implementing
DialogState), object NavigationBack in LoanUndoApprovalEvent, and the singleton
members OnSubmit, OnRetry, OnNavigateBack of sealed interface
LoanUndoApprovalAction to use `data object` syntax; update their declarations
only (no behavior changes) so they remain singletons but gain data-object
semantics.
🪄 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: 59df63c8-c4af-44af-80f3-de8a3716b541
📒 Files selected for processing (3)
core/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanAccountApprovalRepository.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanAccountApprovalRepositoryImp.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanUndoApproval/LoanUndoApprovalViewModel.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanAccountApprovalRepositoryImp.kt
- core/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanAccountApprovalRepository.kt
|
@niyajali please review it again |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanUndoApproval/LoanUndoApprovalViewModel.kt (3)
67-68:⚠️ Potential issue | 🟡 MinorAvoid surfacing
"null"as the error text.Line 67 uses
response.message, which can be the literal"null"when the throwable has no message.Suggested fix
is DataState.Error -> { + val message = response.exception.message?.takeIf { it.isNotBlank() } + ?: "Something went wrong" mutableStateFlow.update { it.copy( - dialogState = LoanUndoApprovalState.DialogState.Error(response.message), + dialogState = LoanUndoApprovalState.DialogState.Error(message), ) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanUndoApproval/LoanUndoApprovalViewModel.kt` around lines 67 - 68, The error state is being set with response.message which can be the literal string "null"; update the LoanUndoApprovalViewModel code that assigns LoanUndoApprovalState.DialogState.Error to sanitize the message (e.g., use response.message.takeIf { !it.isNullOrBlank() && it != "null" } ?: response.throwable?.localizedMessage ?: "Unknown error") so we never surface the string "null" — locate the assignment that constructs LoanUndoApprovalState.DialogState.Error and apply this fallback/sanitization logic.
79-79:⚠️ Potential issue | 🟡 MinorRemove unreachable
DataState.Loadingbranch.Line 79 is dead code here because
repository.undoLoanApproval(...)is a suspend call returning one finalDataStatevalue.Suggested fix
- DataState.Loading -> Unit }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanUndoApproval/LoanUndoApprovalViewModel.kt` at line 79, In LoanUndoApprovalViewModel.kt, remove the unreachable DataState.Loading -> Unit branch from the when that handles the result of repository.undoLoanApproval(...) (inside the undo approval coroutine in the ViewModel) since repository.undoLoanApproval is a suspend call that returns a single final DataState; update the when to handle only the actual returned cases (e.g., DataState.Success and DataState.Error) or use an exhaustive when/else if needed, and clean up any unused imports or dead variables related to the Loading branch.
55-63:⚠️ Potential issue | 🟠 MajorGuard against duplicate in-flight undo requests.
undoLoanApproval()can be re-entered from retry/submit while already loading, which risks duplicate backend writes.Suggested fix
- fun undoLoanApproval() { + private fun undoLoanApproval() { + if (state.dialogState is LoanUndoApprovalState.DialogState.Loading) return + mutableStateFlow.update { it.copy( dialogState = LoanUndoApprovalState.DialogState.Loading, ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanUndoApproval/LoanUndoApprovalViewModel.kt` around lines 55 - 63, The undoLoanApproval() function can be re-entered and send duplicate requests while a previous undo is in-flight; add an early guard that checks the current state (e.g., if state.dialogState == LoanUndoApprovalState.DialogState.Loading) and return immediately before updating mutableStateFlow or launching the coroutine. In practice, read the current state from mutableStateFlow or the state property at the top of undoLoanApproval(), return if already Loading, otherwise proceed to set dialogState to Loading and call repository.undoLoanApproval(...) inside viewModelScope.launch; keep all existing state updates and error handling but prevent re-entry to avoid duplicate backend writes.
🧹 Nitpick comments (1)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanUndoApproval/LoanUndoApprovalScreen.kt (1)
67-96: Limit helper composable visibility to module scope.
LoanUndoApprovalDialogandLoanUndoApprovalContentlook internal to this feature; consider marking theminternalto avoid expanding public API surface unintentionally.Suggested refactor
-@Composable -fun LoanUndoApprovalDialog( +@Composable +internal fun LoanUndoApprovalDialog( @@ -@Composable -fun LoanUndoApprovalContent( +@Composable +internal fun LoanUndoApprovalContent(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanUndoApproval/LoanUndoApprovalScreen.kt` around lines 67 - 96, Mark the feature-internal composables LoanUndoApprovalDialog and LoanUndoApprovalContent as internal to restrict their visibility to the module; update the function declarations for LoanUndoApprovalDialog(...) and LoanUndoApprovalContent(...) to use the internal modifier so they are not exposed as public API while leaving their parameters 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 `@cmp-shared/cmp_shared.podspec`:
- Line 53: The spec.resources entry currently uses Windows backslashes which
break CocoaPods resolution; update the spec.resources value (the literal
'build\compose\cocoapods\compose-resources') to use POSIX separators
('build/compose/cocoapods/compose-resources') so CocoaPods can find the
compose-resources directory.
In
`@core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanAccountApprovalRepositoryImp.kt`:
- Around line 67-68: In undoLoanApproval(), avoid swallowing coroutine
cancellations by adding an explicit catch for
kotlin.coroutines.cancellation.CancellationException (or
java.util.concurrent.CancellationException) that immediately rethrows, before
the existing catch (e: Exception) block that returns DataState.Error(e); ensure
the catch order is: catch (ce: CancellationException) { throw ce } followed by
the broad catch that converts other exceptions to DataState.Error, referencing
the undoLoanApproval function and the existing DataState.Error(e) return to
locate where to change.
---
Duplicate comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanUndoApproval/LoanUndoApprovalViewModel.kt`:
- Around line 67-68: The error state is being set with response.message which
can be the literal string "null"; update the LoanUndoApprovalViewModel code that
assigns LoanUndoApprovalState.DialogState.Error to sanitize the message (e.g.,
use response.message.takeIf { !it.isNullOrBlank() && it != "null" } ?:
response.throwable?.localizedMessage ?: "Unknown error") so we never surface the
string "null" — locate the assignment that constructs
LoanUndoApprovalState.DialogState.Error and apply this fallback/sanitization
logic.
- Line 79: In LoanUndoApprovalViewModel.kt, remove the unreachable
DataState.Loading -> Unit branch from the when that handles the result of
repository.undoLoanApproval(...) (inside the undo approval coroutine in the
ViewModel) since repository.undoLoanApproval is a suspend call that returns a
single final DataState; update the when to handle only the actual returned cases
(e.g., DataState.Success and DataState.Error) or use an exhaustive when/else if
needed, and clean up any unused imports or dead variables related to the Loading
branch.
- Around line 55-63: The undoLoanApproval() function can be re-entered and send
duplicate requests while a previous undo is in-flight; add an early guard that
checks the current state (e.g., if state.dialogState ==
LoanUndoApprovalState.DialogState.Loading) and return immediately before
updating mutableStateFlow or launching the coroutine. In practice, read the
current state from mutableStateFlow or the state property at the top of
undoLoanApproval(), return if already Loading, otherwise proceed to set
dialogState to Loading and call repository.undoLoanApproval(...) inside
viewModelScope.launch; keep all existing state updates and error handling but
prevent re-entry to avoid duplicate backend writes.
---
Nitpick comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanUndoApproval/LoanUndoApprovalScreen.kt`:
- Around line 67-96: Mark the feature-internal composables
LoanUndoApprovalDialog and LoanUndoApprovalContent as internal to restrict their
visibility to the module; update the function declarations for
LoanUndoApprovalDialog(...) and LoanUndoApprovalContent(...) to use the internal
modifier so they are not exposed as public API while leaving their parameters
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: 1ad7946f-4ada-4937-a4a3-d8f1de967b97
📒 Files selected for processing (8)
cmp-shared/cmp_shared.podspeccore/data/build.gradle.ktscore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanAccountApprovalRepositoryImp.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/util/NetworkMonitor.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/LoanUndoApprovalRequest.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanUndoApproval/LoanUndoApprovalRoute.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanUndoApproval/LoanUndoApprovalScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanUndoApproval/LoanUndoApprovalViewModel.kt
✅ Files skipped from review due to trivial changes (2)
- core/data/build.gradle.kts
- core/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/LoanUndoApprovalRequest.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanUndoApproval/LoanUndoApprovalRoute.kt
|
@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.
|
|
@Arinyadav1 Why are we permitting users to access the undo approval screen when the loan account has not yet been approved? |
|
@niyajali This functionality will be implement when action UI will build and this is only undo approval screen. In actions show undo approval screen action when user permitted to undo approval otherwise doesn't show undo approval action |
|
|
@Arinyadav1 Please resolve the merge conflicts. |
# Conflicts: # cmp-shared/cmp_shared.podspec # core/data/src/commonMain/kotlin/com/mifos/core/data/util/NetworkMonitor.kt
2387ab2 to
3f41f79
Compare
|



Fixes - Jira-#682
@coderabbitai make a summary description of this PR
Screen_recording_20260407_192954.webm
Summary by CodeRabbit
New Features
Chores