feat(loan): implement loan reject screen#2628
Open
gurnoorpannu wants to merge 52 commits into
Open
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 a complete loan-rejection feature: new data models, network API, data/repository and use-case wiring, a Compose screen and ViewModel with validation, date helpers, navigation integration, DI registrations, and localized strings. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client UI
participant VM as RejectLoanViewModel
participant UC as RejectLoanUseCase
participant Repo as LoanAccountRejectRepository
participant DM as DataManagerLoan
participant API as LoanService
UI->>VM: User fills date & note, taps Submit
VM->>VM: validate(state)
VM->>UC: invoke(loanId, RejectLoanPayload)
UC->>Repo: rejectLoan(loanId, payload)
Repo->>DM: rejectLoan(loanId, payload)
DM->>API: POST /loans/{loanId}?command=reject
API-->>DM: RejectLoanResponse
DM-->>Repo: RejectLoanResponse
Repo-->>UC: RejectLoanResponse
UC-->>VM: RejectLoanResponse
VM-->>UI: Emit RejectSuccess event / update state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
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 |
itsPronay
suggested changes
Mar 3, 2026
itsPronay
left a comment
itsPronay
left a comment
Member
There was a problem hiding this comment.
Also please use baseViewModel,
and its really great that you wrote tests; however, please confirm with @therajanmaurya in standup, if we should include tests in this PR
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/di/LoanModule.kt (1)
39-39: Consider usingviewModelOffor consistency with other ViewModel registrations.All other ViewModels in this module use
viewModelOf(::ClassName). Using the explicitviewModel { }block here breaks the pattern.♻️ Proposed fix
- viewModel { RejectLoanViewModel(rejectLoanUseCase = get(), savedStateHandle = get()) } + viewModelOf(::RejectLoanViewModel)🤖 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/di/LoanModule.kt` at line 39, Replace the explicit factory block for RejectLoanViewModel with the standard shorthand used across the module: change the registration that currently uses viewModel { RejectLoanViewModel(rejectLoanUseCase = get(), savedStateHandle = get()) } to use viewModelOf(::RejectLoanViewModel) so it matches the other ViewModel registrations and keeps consistency with the module's pattern.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt (1)
183-192: Reset the result key after consumption to prevent re-triggering.The current implementation correctly resets
LOAN_REJECT_SUCCESS_RESULT_KEYtofalseafter showing the snackbar. However, callingviewModel.trySendAction(LoanAccountSummaryAction.OnRetry)before the snackbar could cause a state update race. Consider reordering to reset the key first, then refresh, then show snackbar.Suggested reorder
LaunchedEffect(navController) { val savedStateHandle = navController.currentBackStackEntry?.savedStateHandle ?: return@LaunchedEffect savedStateHandle.getStateFlow(LOAN_REJECT_SUCCESS_RESULT_KEY, false).collect { isSuccess -> if (isSuccess) { - viewModel.trySendAction(LoanAccountSummaryAction.OnRetry) - snackbarHostState.showSnackbar(message = rejectLoanSuccessMessage) savedStateHandle[LOAN_REJECT_SUCCESS_RESULT_KEY] = false + viewModel.trySendAction(LoanAccountSummaryAction.OnRetry) + snackbarHostState.showSnackbar(message = rejectLoanSuccessMessage) } } }🤖 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/loanAccountSummary/LoanAccountSummaryScreen.kt` around lines 183 - 192, In the LaunchedEffect handling LOAN_REJECT_SUCCESS_RESULT_KEY, reorder operations to avoid a state race: inside the collect block first set savedStateHandle[LOAN_REJECT_SUCCESS_RESULT_KEY] = false to reset the result, then call viewModel.trySendAction(LoanAccountSummaryAction.OnRetry) to refresh state, and finally call snackbarHostState.showSnackbar(message = rejectLoanSuccessMessage); ensure these changes are applied within the same LaunchedEffect / collect scope that uses navController and savedStateHandle.feature/loan/src/commonTest/kotlin/com/mifos/feature/loan/loanReject/RejectLoanViewModelTest.kt (1)
90-114: Add a rapid double-submit regression test.Line 90–114 validates loading transitions, but it doesn’t assert that two quick
SubmitClickedintents result in a single repository call. A guard test here would prevent regressions for this non-idempotent action.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/loan/src/commonTest/kotlin/com/mifos/feature/loan/loanReject/RejectLoanViewModelTest.kt` around lines 90 - 114, Add a test in RejectLoanViewModelTest that ensures rapid double submission only triggers one repository call: configure repository.flowFactory to record/increment a counter or spy when invoked, call viewModel.processIntent(RejectLoanViewIntent.SubmitClicked) twice in quick succession (without awaiting), advanceTimeBy/advanceUntilIdle as needed, then assert the repository invocation count is exactly 1 and state transitions remain correct (e.g., isLoading/isSuccess). Use the existing repository.flowFactory and viewModel.processIntent helpers to locate where to insert this test.
🤖 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/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt`:
- Around line 106-118: The DisposableEffect is currently keyed to navController
but attaches an observer to navController.currentBackStackEntry?.lifecycle,
which can change while navController stays the same; change the effect to be
keyed to that lifecycle instead: compute val lifecycle =
navController.currentBackStackEntry?.lifecycle, use DisposableEffect(lifecycle)
so the effect reruns when the back stack entry changes, attach the
LifecycleEventObserver (which calls
viewModel.trySendAction(ClientLoanAccountsAction.Refresh) on ON_RESUME) to that
lifecycle, and keep the onDispose block to remove the observer from the same
lifecycle (handling null lifecycle safely).
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReject/RejectLoanScreen.kt`:
- Around line 145-147: The DatePicker conversions in RejectLoanScreen are using
the system default timezone causing date shifts; update all conversions that
call toEpochMillis()/Instant.fromEpochMilliseconds()/rememberDatePickerState
initialSelectedDateMillis and any LocalDate <-> millis conversions (e.g., the
usages around datePickerState and the handlers at ~lines 155–159 and 258–267) to
use TimeZone.UTC instead of TimeZone.currentSystemDefault() so the DatePicker
interprets selectedDateMillis as UTC (00:00:00Z) and prevents calendar day
shifts for non-UTC users.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReject/RejectLoanViewModel.kt`:
- Around line 79-80: The SubmitClicked branch calls submitLoanRejection() and
currently allows duplicate in-flight submissions; add an instance-level tracker
(e.g., a nullable Job or Boolean like submitJob or isSubmitting) on
RejectLoanViewModel and update submitLoanRejection() to return immediately if a
submit is already active, otherwise assign the launched coroutine Job to that
tracker and clear it when the job completes/fails; ensure both places that
invoke submitLoanRejection() (the SubmitClicked handler and any other callers in
the 115-167 region) use this same protection so concurrent taps cannot trigger
duplicate non-idempotent reject API calls.
---
Nitpick comments:
In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/di/LoanModule.kt`:
- Line 39: Replace the explicit factory block for RejectLoanViewModel with the
standard shorthand used across the module: change the registration that
currently uses viewModel { RejectLoanViewModel(rejectLoanUseCase = get(),
savedStateHandle = get()) } to use viewModelOf(::RejectLoanViewModel) so it
matches the other ViewModel registrations and keeps consistency with the
module's pattern.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt`:
- Around line 183-192: In the LaunchedEffect handling
LOAN_REJECT_SUCCESS_RESULT_KEY, reorder operations to avoid a state race: inside
the collect block first set savedStateHandle[LOAN_REJECT_SUCCESS_RESULT_KEY] =
false to reset the result, then call
viewModel.trySendAction(LoanAccountSummaryAction.OnRetry) to refresh state, and
finally call snackbarHostState.showSnackbar(message = rejectLoanSuccessMessage);
ensure these changes are applied within the same LaunchedEffect / collect scope
that uses navController and savedStateHandle.
In
`@feature/loan/src/commonTest/kotlin/com/mifos/feature/loan/loanReject/RejectLoanViewModelTest.kt`:
- Around line 90-114: Add a test in RejectLoanViewModelTest that ensures rapid
double submission only triggers one repository call: configure
repository.flowFactory to record/increment a counter or spy when invoked, call
viewModel.processIntent(RejectLoanViewIntent.SubmitClicked) twice in quick
succession (without awaiting), advanceTimeBy/advanceUntilIdle as needed, then
assert the repository invocation count is exactly 1 and state transitions remain
correct (e.g., isLoading/isSuccess). Use the existing repository.flowFactory and
viewModel.processIntent helpers to locate where to insert this test.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
core/data/src/commonMain/kotlin/com/mifos/core/data/di/RepositoryModule.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanAccountRejectRepository.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanAccountRejectRepositoryImp.ktcore/domain/src/commonMain/kotlin/com/mifos/core/domain/di/UseCaseModule.ktcore/domain/src/commonMain/kotlin/com/mifos/core/domain/useCases/RejectLoanUseCase.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/RejectLoanPayload.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/RejectLoanResponse.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.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/loanAccountSummary/LoanAccountSummaryScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreenRoute.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryViewModel.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReject/LoanRejectScreenRoute.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReject/RejectLoanScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReject/RejectLoanViewIntent.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReject/RejectLoanViewModel.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReject/RejectLoanViewState.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.ktfeature/loan/src/commonTest/kotlin/com/mifos/feature/loan/loanReject/RejectLoanUseCaseTest.ktfeature/loan/src/commonTest/kotlin/com/mifos/feature/loan/loanReject/RejectLoanViewModelTest.kt
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReject/RejectLoanViewModel.kt (1)
88-133:⚠️ Potential issue | 🔴 CriticalPrevent duplicate in-flight reject requests.
Line 89 only checks
isLoadingbefore launching; rapid repeatedSubmitClickedactions can still start multiple coroutines before Line 100 flips loading, causing duplicate reject API calls.Suggested fix
+import kotlinx.coroutines.Job ... ) : BaseViewModel<RejectLoanViewState, RejectLoanEvent, RejectLoanAction>( initialState = RejectLoanViewState(rejectedOnDate = today()), ) { + private var submitJob: Job? = null ... private fun submitLoanRejection() { - if (state.isLoading) return + if (submitJob?.isActive == true || state.isLoading) return - viewModelScope.launch { + submitJob = viewModelScope.launch { + try { val validatedState = validate(state) mutableStateFlow.value = validatedState ... } catch (e: CancellationException) { throw e } catch (e: Exception) { val errorMessage = e.message ?.takeIf { it.isNotBlank() } ?: getString(Res.string.feature_loan_unknown_error_occured) mutableStateFlow.update { it.copy( isLoading = false, submissionError = errorMessage, ) } + } finally { + submitJob = null } } }🤖 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/loanReject/RejectLoanViewModel.kt` around lines 88 - 133, The submitLoanRejection function can still start multiple coroutines because it only checks state.isLoading before launching; fix this by performing an atomic check-and-set on the ViewModel state so only one request can flip isLoading to true. For example, before viewModelScope.launch, attempt to atomically set isLoading=true (and submissionError=null) using mutableStateFlow.compareAndSet(currentState, currentState.copy(isLoading=true, submissionError=null)) and return if the compareAndSet fails; alternatively, protect the whole start sequence with a Mutex locked at the top of submitLoanRejection. Update the code around submitLoanRejection, state, and mutableStateFlow so the in-flight guard is applied atomically.
🤖 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/common/src/commonMain/kotlin/com/mifos/core/common/utils/DateHelper.kt`:
- Around line 459-463: today() currently uses TimeZone.currentSystemDefault()
while helpers like toEpochMillis()/toLocalDate() default to UTC; change today()
to accept a TimeZone parameter (e.g., fun today(tz: TimeZone = TimeZone.UTC)) so
its default matches the other helpers, update any internal calls to pass that
tz, and audit the related helpers (around the toEpochMillis/toLocalDate changes
at lines ~482-503) to ensure they share the same default; finally update the
reject-loan flow to pass a single explicit TimeZone instance to today(...),
toEpochMillis(...), and toLocalDate(...) to avoid cross-flow date drift.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/components/LoanAccountProfileActionItem.kt`:
- Around line 31-32: The import ordering in LoanAccountProfileActionItem.kt is
violating Spotless rules for import ordering; run the project's Spotless
formatter (or IDE import organizer) to normalize imports, ensuring the two
resource imports (feature_loan_profile_item_reject_loan_subtitle and
feature_loan_profile_item_reject_loan_title) are placed in the correct
sorted/grouped order and remove any unused imports so the file complies with the
project's Spotless configuration.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileScreen.kt`:
- Around line 121-126: Consume and clear the LOAN_REJECT_SUCCESS_RESULT_KEY flag
on the savedStateHandle before calling the suspending snackbar to avoid
re-trigger on cancellation: when you detect isSuccess from
savedStateHandle.getStateFlow(...), first set
savedStateHandle[LOAN_REJECT_SUCCESS_RESULT_KEY] = false (thus consuming the
event) and call viewModel.trySendAction(LoanAccountAction.OnRetry) immediately,
then call snackbarHostState.showSnackbar(message = rejectSuccessMessage); ensure
all references use the same savedStateHandle, LOAN_REJECT_SUCCESS_RESULT_KEY,
viewModel.trySendAction, snackbarHostState.showSnackbar and rejectSuccessMessage
identifiers.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileViewModel.kt`:
- Around line 143-149: The handler for LoanAccountAction.OnDetailItemClick
currently grabs loanId from mutableStateFlow.value.loanAccount?.id but only
returns on null; update it to validate that loanId is present and not a
default/invalid value before emitting navigation events. Specifically, in the
OnDetailItemClick branch (symbols: LoanAccountAction.OnDetailItemClick,
mutableStateFlow, LoanAccountProfileActionItem.RejectLoan, sendEvent,
LoanAccountEvent.NavigateToRejectLoan, LoanAccountEvent.NavigateToDetail), add a
guard that checks for invalid IDs (e.g., null/empty/blank strings or sentinel
numeric values such as 0 or negative) and early-returns or emits a safe
error/log event when invalid; only call sendEvent(NavigateToRejectLoan(...)) or
sendEvent(NavigateToDetail(...)) when the ID passes that validation.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReject/RejectLoanViewModel.kt`:
- Around line 147-148: The file RejectLoanViewModel.kt ends with an unnecessary
trailing blank line causing a Spotless violation; open the file (class/objects
such as RejectLoanViewModel) and remove the extra blank line at the end of the
file (or run ./gradlew spotlessApply) so the file ends immediately after the
final closing brace.
- Around line 36-43: RejectLoanViewModel currently calls today() twice (once in
the initialState's rejectedOnDate and once for initialDate) which can produce
differing values if midnight passes during init; compute today() once into a
single val (e.g., initialToday) near the top of the ViewModel and use that
single variable for both initialState = RejectLoanViewState(rejectedOnDate =
...) and for initialDate to ensure one source of truth and prevent spurious
isDirty changes.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt`:
- Around line 20-30: The import block in LoanNavigation.kt violates Spotless
ordering/duplication rules; run the project's Spotless formatter or manually
reorder and deduplicate the imports (e.g., ensure imports like
LoanAccountProfileActionItem, loanAccountSummary, LoanAccountApprovalScreen,
loanChargeScreen, navigateToLoanChargesScreen, loanDisbursementScreen,
navigateToLoanDisbursementScreen, LOAN_REJECT_SUCCESS_RESULT_KEY,
loanRejectScreen, navigateToLoanRejectScreen, and loanRepaymentScreen are
alphabetized and unique) and commit the resulting formatting so CI passes.
---
Duplicate comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReject/RejectLoanViewModel.kt`:
- Around line 88-133: The submitLoanRejection function can still start multiple
coroutines because it only checks state.isLoading before launching; fix this by
performing an atomic check-and-set on the ViewModel state so only one request
can flip isLoading to true. For example, before viewModelScope.launch, attempt
to atomically set isLoading=true (and submissionError=null) using
mutableStateFlow.compareAndSet(currentState, currentState.copy(isLoading=true,
submissionError=null)) and return if the compareAndSet fails; alternatively,
protect the whole start sequence with a Mutex locked at the top of
submitLoanRejection. Update the code around submitLoanRejection, state, and
mutableStateFlow so the in-flight guard is applied atomically.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 93fd9560-f725-4b2d-a33a-03e5293bea9c
📒 Files selected for processing (19)
core/common/src/commonMain/kotlin/com/mifos/core/common/utils/DateHelper.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanAccountRejectRepository.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanAccountRejectRepositoryImp.ktcore/domain/src/commonMain/kotlin/com/mifos/core/domain/useCases/RejectLoanUseCase.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.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/loanAccountProfile/LoanAccountProfileNavigation.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileViewModel.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/components/LoanAccountProfileActionItem.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReject/RejectLoanAction.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReject/RejectLoanEvent.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReject/RejectLoanScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReject/RejectLoanViewModel.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReject/RejectLoanViewState.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt
🚧 Files skipped from review as they are similar to previous changes (4)
- feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/di/LoanModule.kt
- core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.kt
- feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt
- feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReject/RejectLoanViewState.kt
There was a problem hiding this comment.
♻️ Duplicate comments (2)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReject/RejectLoanViewModel.kt (2)
36-43:⚠️ Potential issue | 🟡 MinorUse one captured date for both initial state and dirty checks.
Line 36 and Line 42 call
today()separately. If initialization crosses midnight, the untouched form can be marked dirty.💡 Proposed fix
) : BaseViewModel<RejectLoanViewState, RejectLoanEvent, RejectLoanAction>( initialState = RejectLoanViewState(rejectedOnDate = today()), ) { @@ - private val initialDate = today() + private val initialDate = state.rejectedOnDate🤖 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/loanReject/RejectLoanViewModel.kt` around lines 36 - 43, RejectLoanViewModel calls today() twice (in initialState and in initialDate) which can cross midnight and mark an untouched form dirty; fix by capturing a single date value (e.g., val capturedDate = today()) before initializing state and use that capturedDate both in initialState = RejectLoanViewState(rejectedOnDate = capturedDate) and for private val initialDate = capturedDate so both places reference the same timestamp.
88-133:⚠️ Potential issue | 🟠 MajorPrevent race window for duplicate reject submissions.
The Line 89
isLoadingguard can be bypassed by rapid taps before the launched coroutine sets loading at Lines 99-101, allowing concurrent non-idempotent reject calls.🛠️ Proposed fix (track active submit job)
import com.mifos.core.ui.util.BaseViewModel +import kotlinx.coroutines.Job import kotlinx.coroutines.flow.update import kotlinx.coroutines.launch @@ ) : BaseViewModel<RejectLoanViewState, RejectLoanEvent, RejectLoanAction>( initialState = RejectLoanViewState(rejectedOnDate = today()), ) { + private var submitJob: Job? = null @@ private fun submitLoanRejection() { - if (state.isLoading) return - - viewModelScope.launch { - val validatedState = validate(state) - mutableStateFlow.value = validatedState + if (submitJob?.isActive == true || state.isLoading) return + + submitJob = viewModelScope.launch { + try { + val validatedState = validate(state) + mutableStateFlow.value = validatedState - if (validatedState.rejectedOnDateError != null) { - return@launch - } + if (validatedState.rejectedOnDateError != null) { + return@launch + } @@ - try { rejectLoanUseCase(loanId, payload) mutableStateFlow.update { it.copy( @@ - } catch (e: CancellationException) { + } catch (e: CancellationException) { throw e } catch (e: Exception) { @@ mutableStateFlow.update { it.copy( isLoading = false, submissionError = errorMessage, ) } + } finally { + submitJob = null } } }#!/bin/bash # Verifies whether submit protection is only state-based (race-prone) or job-based (race-safe). FILE="$(fd RejectLoanViewModel.kt | head -n1)" echo "Inspecting: $FILE" rg -n 'submitLoanRejection\(|state\.isLoading|viewModelScope\.launch|Job|submitJob' "$FILE" # Expected for robust protection: presence of Job/submitJob guard around submit launch.🤖 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/loanReject/RejectLoanViewModel.kt` around lines 88 - 133, submitLoanRejection currently only checks state.isLoading before launching a coroutine, which allows a race where multiple rapid taps start concurrent rejectLoanUseCase calls; fix by tracking the active submission Job and guarding the launch: add a nullable Job property (e.g., submitJob) in RejectLoanViewModel, check submitJob?.isActive before starting work in submitLoanRejection, assign the Job returned by viewModelScope.launch to submitJob, and ensure submitJob is cleared on completion or failure while still updating mutableStateFlow (isLoading/submissionError) as you do now; reference submitLoanRejection, viewModelScope.launch, mutableStateFlow, and rejectLoanUseCase when making these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReject/RejectLoanViewModel.kt`:
- Around line 36-43: RejectLoanViewModel calls today() twice (in initialState
and in initialDate) which can cross midnight and mark an untouched form dirty;
fix by capturing a single date value (e.g., val capturedDate = today()) before
initializing state and use that capturedDate both in initialState =
RejectLoanViewState(rejectedOnDate = capturedDate) and for private val
initialDate = capturedDate so both places reference the same timestamp.
- Around line 88-133: submitLoanRejection currently only checks state.isLoading
before launching a coroutine, which allows a race where multiple rapid taps
start concurrent rejectLoanUseCase calls; fix by tracking the active submission
Job and guarding the launch: add a nullable Job property (e.g., submitJob) in
RejectLoanViewModel, check submitJob?.isActive before starting work in
submitLoanRejection, assign the Job returned by viewModelScope.launch to
submitJob, and ensure submitJob is cleared on completion or failure while still
updating mutableStateFlow (isLoading/submissionError) as you do now; reference
submitLoanRejection, viewModelScope.launch, mutableStateFlow, and
rejectLoanUseCase when making these changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2015e7a3-f459-47d7-8034-45b319df5a66
📒 Files selected for processing (4)
feature/loan/src/commonMain/composeResources/values/strings.xmlfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/components/LoanAccountProfileActionItem.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReject/RejectLoanViewModel.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/components/LoanAccountProfileActionItem.kt
Contributor
|
@gurnoorpannu View Account should navigate to Loan Profile Screen but it is still navigation the old Loan Account Summary Screen. Please review. 
It could be possible that the local branch hasn't been updated yet. If that's the case, then please do it and update the screen recording in the PR description with the new Loan Flow UI screens. |
biplab1
changed the title
Contributor
Author
@biplab1 Yeah! when I implemented this ticket , at that time the old LoanAccountSummaryScreen was in use , I'll update that |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/navigation/LoanNavigation.kt`:
- Around line 95-109: The onDetailItemClick lambda in LoanNavigation.kt
currently handles only RepaymentSchedule, Transactions, Charges and Documents
leaving other LoanAccountProfileActionItem values (General, Dashboard,
AccountDetails, Originators, Collateral, TermVariations, Reschedules, Notes,
StandingInstructions) as no-ops; update the when expression in the
onDetailItemClick handler to either add appropriate navigation calls (e.g.,
navController.navigateToLoanGeneralScreen, navigateToLoanDashboardScreen,
navigateToLoanAccountDetailsScreen, navigateToLoanOriginatorsScreen,
navigateToLoanCollateralScreen, navigateToLoanTermVariationsScreen,
navigateToLoanReschedulesScreen, navigateToLoanNotesScreen,
navigateToLoanStandingInstructionsScreen) for each enum member, or remove those
items from loanProfileActionItems where they’re constructed, or make the when
exhaustive by replacing else -> { } with an explicit fallback that logs and
throws (e.g., IllegalStateException("Unhandled LoanAccountProfileActionItem:
$actionItem")) so taps no longer silently do nothing; ensure any new
navController methods used match existing naming patterns and are implemented
where needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d80ee317-4f03-4202-aca8-42c7ca547a6a
📒 Files selected for processing (6)
core/common/src/commonMain/kotlin/com/mifos/core/common/utils/DateHelper.ktfeature/loan/src/commonMain/composeResources/values/strings.xmlfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileViewModel.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReject/RejectLoanViewModel.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- core/common/src/commonMain/kotlin/com/mifos/core/common/utils/DateHelper.kt
There was a problem hiding this comment.
♻️ Duplicate comments (1)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt (1)
105-118:⚠️ Potential issue | 🟠 MajorClickable profile items still fall through as no-ops.
These actions are still rendered but do nothing when tapped, and
LoanAccountProfileActionItem.AccountDetailsis in that group as well. That leaves the reported "View Account" path unresolved from this navigation layer. Please wire these items to real destinations or disable/remove them until the targets exist.🤖 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/navigation/LoanNavigation.kt` around lines 105 - 118, The listed profile actions (LoanAccountProfileActionItem.General, .Dashboard, .AccountDetails, .Originators, .Collateral, .TermVariations, .Reschedules, .Notes, .StandingInstructions) are currently falling through to no-ops in the when block in LoanNavigation.kt; update the handler for these items (the when that currently returns Unit) to either (a) call the appropriate navigation functions/screens for each item (e.g., navigateToAccountDetails, navigateToCollateralScreen, navigateToNotes, etc.) mapping each LoanAccountProfileActionItem to its real destination, or (b) if the destinations are not yet implemented, remove these items from the rendered list or mark them non-clickable by returning a no-op from the UI layer instead of exposing them; ensure LoanAccountProfileActionItem.AccountDetails is routed to the "View Account" navigation path when implemented.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt`:
- Around line 105-118: The listed profile actions
(LoanAccountProfileActionItem.General, .Dashboard, .AccountDetails,
.Originators, .Collateral, .TermVariations, .Reschedules, .Notes,
.StandingInstructions) are currently falling through to no-ops in the when block
in LoanNavigation.kt; update the handler for these items (the when that
currently returns Unit) to either (a) call the appropriate navigation
functions/screens for each item (e.g., navigateToAccountDetails,
navigateToCollateralScreen, navigateToNotes, etc.) mapping each
LoanAccountProfileActionItem to its real destination, or (b) if the destinations
are not yet implemented, remove these items from the rendered list or mark them
non-clickable by returning a no-op from the UI layer instead of exposing them;
ensure LoanAccountProfileActionItem.AccountDetails is routed to the "View
Account" navigation path when implemented.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 29c7b046-e694-458b-a2df-b57a79c802b6
📒 Files selected for processing (1)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt
Member
|
@gurnoorpannu the iOS build is currently failing because this KMP project isn’t integrated using CocoaPods. As a result, there is no cmp_shared.podspec, which is why the build shows errors like generateDummyFramework not found. Also, this project still has several existing issues. The last time I checked, there were database-related problems that prevented proper integration with iOS. It would be best to first make sure those issues are resolved and that the project is stable. Once everything is fixed and verified, we can proceed with integrating it properly with iOS and then address the build issue. |
biplab1
reviewed
Mar 10, 2026
| suspend fun rejectLoan( | ||
| @Path("loanId") loanId: Int, | ||
| @Body rejectLoanPayload: RejectLoanPayload, | ||
| ): RejectLoanResponse |
Contributor
There was a problem hiding this comment.
Please refer to the implementation in this PR: #2521 to see how the response is handled in the network layer using HttpResponse, and how it is subsequently mapped and passed using domain-specific models.
niyajali
requested changes
Apr 21, 2026
niyajali
left a comment
niyajali
left a comment
Collaborator
There was a problem hiding this comment.
Hello, it has been noted that despite undergoing review and discussion, the majority of the issues remain unresolved. Furthermore, the codebase does not adhere to established best practices and our internal guidelines. Please examine another project and implement the necessary updates accordingly.
| * Default implementation for [LoanAccountRejectRepository]. | ||
| */ | ||
| class LoanAccountRejectRepositoryImp( | ||
| private val dataManagerLoan: DataManagerLoan, |
Collaborator
There was a problem hiding this comment.
wrap with try-catch block and use io dispatcher and return as DataState
Contributor
Author
@niyajali do you want me to press the "Resolve conversation" button too , as what I think is I have updated the code with whatever you commented here , I was told that the reviewer who made the comment will only resolve it |
Contributor
Author
|
@niyajali if there is some particular practice that you think I missed can you be blunt and mention it here |
Collaborator
@gurnoorpannu I believe there are still outstanding issues that need addressing. We should use a proper dispatcher, incorporate try-catch blocks, and verify network connectivity before initiating requests. We have already discussed the methodologies for these tasks, and a module refactor adhering to established conventions and standards is forthcoming. It's crucial that we align with these new standards, meanwhile concurrently resolve other problems within the UI codebase and screens. |
biplab1
reviewed
Apr 23, 2026
biplab1
reviewed
Apr 23, 2026
Contributor
|
The PR shouldn't be having the changes related to |
gurnoorpannu
force-pushed
the
reject-screen
branch
from
d587061 to
b3cd263
Compare
Contributor
Author
|
@biplab1 sir I have made the changes , can you have a quick look and see if everything looks fine :) |
revanthkumarJ
previously approved these changes
Apr 29, 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.
- runAsDataState now rethrows CancellationException so structured cancellation propagates cleanly instead of being converted into a spurious DataState.Error. - AddEditNoteViewModel.OnRetry no longer waits on loadSingleNote() before retrying the edit. retrieveNote returns a long-lived withNetworkCheck flow that never completes, so the retry path was unreachable; OnRetry now retries the edit/add directly with the user's current text. - RejectLoanRequestDto tightens its contract: rejectedOnDate, locale, and dateFormat are required (non-nullable, no defaults); only note remains optional. - RejectLoanViewModel ignores cancel/back while a submit is in flight so the loading dialog state cannot be clobbered by PreventAccidentalBack mid-request. - RejectLoanViewModel's "today" computation switches from TimeZone.UTC to TimeZone.currentSystemDefault() for both the initial state and the future-date validation, fixing timezone-dependent off-by-one errors near midnight UTC.
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.
gurnoorpannu
force-pushed
the
reject-screen
branch
from
0604850 to
aea9181
Compare
|
|
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
9 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes - Jira-#703
Implementation:-
Screen_recording_20260331_105718.webm
Run the static analysis check
./gradlew checkorci-prepush.shto make sure you didn't break anythingIf you have multiple commits please combine them into one commit by squashing them.
Summary by CodeRabbit