MW-390 Pass-code/Bio-metric authentication for intra-bank transactions#1999
Merged
niyajali merged 12 commits intoopenMF:developmentfrom Mar 20, 2026
Merged
Conversation
…n logic - Migrate passcode navigation to use type-safe routes (`RootPasscodeRoute`, `ReAuthPasscodeRoute`, and `InternalPasscodeRoute`) instead of raw strings. - Split the generic `mifosPasscodeScreen` into specialized `rootMifosPasscodeScreen`, `reAuthMifosPasscodeScreen`, and `internalMifosPasscodeScreen` functions. - Implement a user verification flow in `TransferConfirmViewModel` that requires authentication before initiating an intra-bank transfer. - Update `MifosNavHost` and `TransferConfirmNavigation` to support passing and observing unique verification keys via `SavedStateHandle`. - Refactor `TransferConfirmState` to remove the `isProcessing` flag, utilizing `DialogState` to manage loading and error transitions. - Rename `onPasscodeRejected` to `onAuthenticationFailed` within the passcode feature to better reflect its role in both biometric and passcode failures. - Decentralize authentication keys by moving them from `AuthenticationUtils` to their specific feature view models (e.g., `SettingsViewModel` and `TransferConfirmViewModel`). - Add a dependency on the passcode feature in `feature/transfer-interbank`.
- Update `TransferConfirmScreen` to observe authentication results from `NavBackStackEntry.savedStateHandle` and bridge them to the ViewModel. - Refactor `TransferConfirmViewModel` verification logic to use `onSuccess` and `onFailed` callbacks, improving separation between verification and transfer execution. - Rename the core transfer logic to `handleTransfer` and introduce a new `initiateTransfer` method to coordinate the verification flow. - Update `TransferConfirmNavigation` to pass the `backStackEntry.savedStateHandle` to the screen, allowing it to receive results from navigation-based verification.
- Replace the direct `authenticationResult` property in `TransferConfirmViewModel` with a structured `TransferConfirmAction.UpdateUserVerificationResult`. - Update `TransferConfirmScreen` to dispatch the verification result via the new action instead of modifying ViewModel state directly. - Incorporate `userVerificationResult` into `TransferConfirmState` to centralize state management. - Refactor `handleUserVerification` in `TransferConfirmViewModel` to observe verification results through the state flow. - Clean up unused imports and improve formatting in `TransferConfirmScreen.kt`, `TransferConfirmViewModel.kt`, and `IntraBankHubViewModel.kt`. - Add `SavedStateHandle` and `INTER_BANK_TRANSFER_VERIFICATION_KEY` to `InterbankTransferViewModel` in preparation for interbank verification. - Update the project dependency tree to reflect reorganization of the `:feature:passcode` module.
…Model - Remove unused imports for `viewModelScope`, `MutableStateFlow`, and `launch` in `InterbankTransferViewModel.kt`. - Remove redundant whitespace for better code clarity.
- Introduce `isProcessing` state to `TransferConfirmState` to track active transfer operations and prevent duplicate submissions. - Update `TransferConfirmScreen` to display a `CircularProgressIndicator` within the action button and show `MifosProgressIndicatorOverlay` while processing. - Refactor `TransferConfirmViewModel` to manage the `isProcessing` flag during the transfer lifecycle and API calls. - Convert `handleUserVerification` into a suspend function to improve structured concurrency within the ViewModel. - Refine the default value for `description` in `TransferConfirmState` and remove outdated comments in `TransferConfirmScreen`.
…mViewModel - Update the `onFailed` callback in `TransferConfirmViewModel` to explicitly set `isProcessing` to `false` and update `dialogState` with a validation error. - Replace the call to `updateValidationError` with a direct `mutableStateFlow` update to ensure the UI state accurately reflects that processing has stopped.
- Add trailing comma to `ValidationError` assignment in `TransferConfirmViewModel.kt`.
niyajali
requested changes
Mar 14, 2026
Collaborator
niyajali
left a comment
niyajali
left a comment
There was a problem hiding this comment.
use this navigation transition accordingly and see which is looks better and update it accordingly
| onPasscodeChanged: () -> Unit = {}, | ||
| onDisableBiometrics: () -> Unit = {}, | ||
| ) { | ||
| composable<ReAuthPasscodeRoute> { |
Collaborator
There was a problem hiding this comment.
animate the screen transition using composableWithSlideTransitions inline method which could be found on NavGraphBuilderExtensions.kt file to animate from up and down
| onDisableBiometrics: () -> Unit = {}, | ||
| ) { | ||
| composable(route = route) { | ||
| composable<RootPasscodeRoute> { |
Collaborator
There was a problem hiding this comment.
this should be the root stay transition
| onPasscodeChanged: () -> Unit = {}, | ||
| onDisableBiometrics: () -> Unit = {}, | ||
| ) { | ||
| composable<InternalPasscodeRoute> { backStackEntry -> |
Collaborator
There was a problem hiding this comment.
this might be the same as the above "composableWithSlideTransitions"
niyajali
requested changes
Mar 14, 2026
| onTransferSuccess: (TransferResult, String) -> Unit, | ||
| navigateForPasscodeVerification: (String) -> Unit, | ||
| ) { | ||
| composable<TransferConfirmRoute> { backStackEntry -> |
Collaborator
There was a problem hiding this comment.
use proper transition in this navigation
…ansfer screens - Add `coreBase.ui` dependency to `feature:passcode` and `feature:transfer-intrabank` modules. - Update `MifosPasscodeNavigation.kt` to use `composableWithStayTransitions` for the root route and `composableWithSlideTransitions` for re-authentication and internal verification routes. - Replace the standard `composable` call with `composableWithSlideTransitions` in `TransferConfirmNavigation.kt` for the transfer confirmation screen.
Contributor
|
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:
📝 WalkthroughWalkthroughRefactors passcode navigation into root/re-auth/internal routes, adds time-limited UserVerificationRepository (record/consume), wires SavedStateHandle verification keys across flows, integrates passcode verification into intra-/interbank transfers, renames authentication callbacks/keys, and adds related module dependencies. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TransferConfirm as TransferConfirmScreen
participant ViewModel as TransferConfirmViewModel
participant NavHost as NavigationHost
participant Passcode as InternalPasscodeScreen
participant StateHandle as SavedStateHandle
User->>TransferConfirm: Tap confirm
TransferConfirm->>ViewModel: Emit NavigateForPasscodeVerification
ViewModel->>NavHost: navigateForPasscodeVerification(verificationKey)
NavHost->>Passcode: Navigate InternalPasscodeRoute(verificationKey)
User->>Passcode: Enter passcode
alt success
Passcode->>StateHandle: set verificationKey = true
StateHandle->>TransferConfirm: stateFlow emits true
TransferConfirm->>ViewModel: UpdateUserVerificationResult(true)
ViewModel->>ViewModel: proceed with transfer
else failure
Passcode->>StateHandle: set verificationKey = false
StateHandle->>TransferConfirm: stateFlow emits false
TransferConfirm->>ViewModel: UpdateUserVerificationResult(false)
ViewModel->>ViewModel: show verification failed
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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)
📝 Coding Plan
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 |
- Update `prodReleaseRuntimeClasspath.tree.txt` to include the `:core-base:ui` project dependency in the production release runtime classpath.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
feature/transfer-interbank/src/commonMain/kotlin/org/mifospay/feature/transfer/interbank/InterbankTransferViewModel.kt (1)
42-47:⚠️ Potential issue | 🟠 MajorLine 46 adds
SavedStateHandledependency but the ViewModel doesn’t use it yet.This can introduce runtime DI fragility (constructor resolution) without behavior gain. Either wire it now for verification state handling or defer/remove until used.
#!/bin/bash set -euo pipefail # Check module wiring for this ViewModel. rg -n -C3 'interbankTransferModule|viewModelOf\(::InterbankTransferViewModel\)|InterbankTransferViewModel\(' # Find other ViewModels that inject SavedStateHandle and compare registration pattern. rg -n -C3 'class .*ViewModel\([^)]*SavedStateHandle|viewModelOf\(::.*ViewModel\)' # Identify Koin/ViewModel dependency versions/configuration. fd 'build.gradle.kts|build.gradle|libs.versions.toml|settings.gradle.kts|gradle.properties' rg -n -C2 'koin|androidx.lifecycle|savedstate|viewmodel'Expected result: confirmed, consistent pattern proving
SavedStateHandleinjection works for this module and platform setup.feature/settings/src/commonMain/kotlin/org/mifospay/feature/settings/SettingsViewModel.kt (1)
153-177:⚠️ Potential issue | 🟠 MajorUse one-shot collection for verification to avoid accumulating collectors.
handlePasscodeVerification()creates a new long-lived collector each time it is called. SinceauthenticationSuccessis aMutableStateFlowthat persists across configuration changes, repeated toggles will stack collectors and execute side effects (e.g., delete/update operations) multiple times on the same flow emission.💡 Suggested fix
+import kotlinx.coroutines.flow.filterNotNull +import kotlinx.coroutines.flow.first + private fun handlePasscodeVerification() { viewModelScope.launch { - authenticationSuccess.collect { result -> - when (result) { - true -> { - passcodeManager.trySendAction(PasscodeAction.DeleteBiometricRegistration) - mutableStateFlow.update { - it.copy( - isBiometricsRegistered = false, - ) - } - savedStateHandle.remove<Boolean?>(DISABLE_BIOMETRICS_VERIFICATION_KEY) - authenticationSuccess.value = null - } - false -> { - mutableStateFlow.update { - it.copy(isBiometricsRegistered = true) - } - savedStateHandle.remove<Boolean?>(DISABLE_BIOMETRICS_VERIFICATION_KEY) - authenticationSuccess.value = null - } - null -> {} - } - } + when (authenticationSuccess.filterNotNull().first()) { + true -> { + passcodeManager.trySendAction(PasscodeAction.DeleteBiometricRegistration) + mutableStateFlow.update { it.copy(isBiometricsRegistered = false) } + } + false -> { + mutableStateFlow.update { it.copy(isBiometricsRegistered = true) } + } + } + savedStateHandle.remove<Boolean?>(DISABLE_BIOMETRICS_VERIFICATION_KEY) + authenticationSuccess.value = null } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/settings/src/commonMain/kotlin/org/mifospay/feature/settings/SettingsViewModel.kt` around lines 153 - 177, handlePasscodeVerification currently launches a long-lived collector on authenticationSuccess each call; replace the repeated collect with a one-shot read (e.g., suspend until a non-null emission via authenticationSuccess.filter { it != null }.first() or authenticationSuccess.first { it != null }) so the coroutine handles the single result and then proceeds to call passcodeManager.trySendAction(PasscodeAction.DeleteBiometricRegistration), update mutableStateFlow (isBiometricsRegistered true/false), remove DISABLE_BIOMETRICS_VERIFICATION_KEY from savedStateHandle, and set authenticationSuccess.value = null; keep those side-effect calls but ensure they run only once per verification by using the single-value retrieval instead of collect.
🧹 Nitpick comments (2)
feature/transfer-intrabank/src/commonMain/kotlin/org/mifospay/feature/transfer/intrabank/confirm/TransferConfirmViewModel.kt (1)
44-45: Minor: Consider using underscore instead of hyphen in the verification key.The key
"intra-banking_transfer_verification_key"mixes hyphens and underscores. While functional, using consistent separators (intra_bank_transfer_verification_key) would be more idiomatic.♻️ Optional fix for naming consistency
-const val INTRA_BANK_TRANSFER_VERIFICATION_KEY = "intra-banking_transfer_verification_key" +const val INTRA_BANK_TRANSFER_VERIFICATION_KEY = "intra_bank_transfer_verification_key"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/transfer-intrabank/src/commonMain/kotlin/org/mifospay/feature/transfer/intrabank/confirm/TransferConfirmViewModel.kt` around lines 44 - 45, The constant INTRA_BANK_TRANSFER_VERIFICATION_KEY currently holds the string "intra-banking_transfer_verification_key" which mixes a hyphen and underscores; update its value to use consistent underscores ("intra_bank_transfer_verification_key") to match idiomatic naming, and then run a quick search for any string literals or persisted keys matching the old value to update them or ensure backward compatibility; keep the constant name unchanged so callers using INTRA_BANK_TRANSFER_VERIFICATION_KEY continue to work.feature/settings/src/commonMain/kotlin/org/mifospay/feature/settings/SettingsViewModel.kt (1)
37-37: Consider making the key literal context-specific to this flow.
DISABLE_BIOMETRICS_VERIFICATION_KEYnow names a specific flow, but the literal is still generic (...authentication.verification.key). A more specific literal reduces accidental key reuse.✏️ Suggested refinement
-const val DISABLE_BIOMETRICS_VERIFICATION_KEY = "org.mifospay.mifos.authentication.verification.key" +const val DISABLE_BIOMETRICS_VERIFICATION_KEY = + "org.mifospay.feature.settings.disable_biometrics.verification.key"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/settings/src/commonMain/kotlin/org/mifospay/feature/settings/SettingsViewModel.kt` at line 37, The constant DISABLE_BIOMETRICS_VERIFICATION_KEY uses a generic literal which risks accidental reuse; update the string literal value for DISABLE_BIOMETRICS_VERIFICATION_KEY to a context-specific key (e.g., include "settings" and "disable_biometrics" in the literal) so it clearly belongs to this settings/biometrics flow, then update all usages that read/write this key to the new literal and add a small migration/read-fallback that checks the old generic key and migrates its value to the new key to preserve existing user state.
🤖 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/transfer-interbank/src/commonMain/kotlin/org/mifospay/feature/transfer/interbank/InterbankTransferViewModel.kt`:
- Line 36: The verification key constants are inconsistent and
INTER_BANK_TRANSFER_VERIFICATION_KEY is unused; update constants to a single
package-style naming (e.g., match DISABLE_BIOMETRICS_VERIFICATION_KEY pattern)
and then either wire the interbank flow to use that key or remove the unused
pieces: in InterbankTransferViewModel use the injected savedStateHandle to set
the verification result under INTER_BANK_TRANSFER_VERIFICATION_KEY when
authentication completes, and update TransferConfirmScreen (or the caller) to
read that same package-style key (or conversely remove
INTER_BANK_TRANSFER_VERIFICATION_KEY and the savedStateHandle injection if
interbank does not need state propagation). Ensure you reference and reconcile
DISABLE_BIOMETRICS_VERIFICATION_KEY, INTRA_BANK_TRANSFER_VERIFICATION_KEY,
INTER_BANK_TRANSFER_VERIFICATION_KEY, InterbankTransferViewModel,
savedStateHandle, and TransferConfirmScreen so naming and usage are consistent.
In
`@feature/transfer-intrabank/src/commonMain/kotlin/org/mifospay/feature/transfer/intrabank/confirm/TransferConfirmViewModel.kt`:
- Around line 301-319: handleUserVerification can suspend forever waiting for
userVerificationResult; change it to await a result with a timeout (use
kotlinx.coroutines.withTimeoutOrNull) after sending
NavigateForPasscodeVerification so the coroutine resumes if the user backs out.
Specifically, wrap the stateFlow.map/filter/.first() wait in
withTimeoutOrNull(...) and treat a null timeout result as a failure path (call
onFailed(), reset mutableStateFlow/isProcessing as needed) so the UI and button
state are restored even when verification is cancelled.
---
Outside diff comments:
In
`@feature/settings/src/commonMain/kotlin/org/mifospay/feature/settings/SettingsViewModel.kt`:
- Around line 153-177: handlePasscodeVerification currently launches a
long-lived collector on authenticationSuccess each call; replace the repeated
collect with a one-shot read (e.g., suspend until a non-null emission via
authenticationSuccess.filter { it != null }.first() or
authenticationSuccess.first { it != null }) so the coroutine handles the single
result and then proceeds to call
passcodeManager.trySendAction(PasscodeAction.DeleteBiometricRegistration),
update mutableStateFlow (isBiometricsRegistered true/false), remove
DISABLE_BIOMETRICS_VERIFICATION_KEY from savedStateHandle, and set
authenticationSuccess.value = null; keep those side-effect calls but ensure they
run only once per verification by using the single-value retrieval instead of
collect.
---
Nitpick comments:
In
`@feature/settings/src/commonMain/kotlin/org/mifospay/feature/settings/SettingsViewModel.kt`:
- Line 37: The constant DISABLE_BIOMETRICS_VERIFICATION_KEY uses a generic
literal which risks accidental reuse; update the string literal value for
DISABLE_BIOMETRICS_VERIFICATION_KEY to a context-specific key (e.g., include
"settings" and "disable_biometrics" in the literal) so it clearly belongs to
this settings/biometrics flow, then update all usages that read/write this key
to the new literal and add a small migration/read-fallback that checks the old
generic key and migrates its value to the new key to preserve existing user
state.
In
`@feature/transfer-intrabank/src/commonMain/kotlin/org/mifospay/feature/transfer/intrabank/confirm/TransferConfirmViewModel.kt`:
- Around line 44-45: The constant INTRA_BANK_TRANSFER_VERIFICATION_KEY currently
holds the string "intra-banking_transfer_verification_key" which mixes a hyphen
and underscores; update its value to use consistent underscores
("intra_bank_transfer_verification_key") to match idiomatic naming, and then run
a quick search for any string literals or persisted keys matching the old value
to update them or ensure backward compatibility; keep the constant name
unchanged so callers using INTRA_BANK_TRANSFER_VERIFICATION_KEY continue to
work.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 335f2668-ade7-4095-ba92-7e55e6eca1a9
📒 Files selected for processing (15)
cmp-android/dependencies/prodReleaseRuntimeClasspath.tree.txtcmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/MifosNavHost.ktcmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/RootNavGraph.ktcore/ui/src/commonMain/kotlin/org/mifospay/core/ui/utils/AuthenticationUtils.ktfeature/passcode/build.gradle.ktsfeature/passcode/src/commonMain/kotlin/org/mifos/feature/passcode/MifosPasscode.ktfeature/passcode/src/commonMain/kotlin/org/mifos/feature/passcode/MifosPasscodeNavigation.ktfeature/settings/src/commonMain/kotlin/org/mifospay/feature/settings/SettingsViewModel.ktfeature/transfer-interbank/build.gradle.ktsfeature/transfer-interbank/src/commonMain/kotlin/org/mifospay/feature/transfer/interbank/InterbankTransferViewModel.ktfeature/transfer-intrabank/build.gradle.ktsfeature/transfer-intrabank/src/commonMain/composeResources/values/strings.xmlfeature/transfer-intrabank/src/commonMain/kotlin/org/mifospay/feature/transfer/intrabank/confirm/TransferConfirmScreen.ktfeature/transfer-intrabank/src/commonMain/kotlin/org/mifospay/feature/transfer/intrabank/confirm/TransferConfirmViewModel.ktfeature/transfer-intrabank/src/commonMain/kotlin/org/mifospay/feature/transfer/intrabank/navigation/TransferConfirmNavigation.kt
💤 Files with no reviewable changes (1)
- core/ui/src/commonMain/kotlin/org/mifospay/core/ui/utils/AuthenticationUtils.kt
| import kotlin.uuid.ExperimentalUuidApi | ||
| import kotlin.uuid.Uuid | ||
|
|
||
| const val INTER_BANK_TRANSFER_VERIFICATION_KEY = "inter-banking_transfer_verification_key" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify all verification-key declarations/usages and compare exact literals.
rg -n -C3 'INTER_BANK_TRANSFER_VERIFICATION_KEY|INTRA_BANK_TRANSFER_VERIFICATION_KEY|DISABLE_BIOMETRICS_VERIFICATION_KEY|verification_key'
# Verify SavedStateHandle read/write sites that use verification keys.
rg -n -C3 'SavedStateHandle|savedStateHandle|previousBackStackEntry|currentBackStackEntry|getStateFlow|set\('Repository: openMF/mifos-pay
Length of output: 1780
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify all verification-key declarations/usages and compare exact literals
rg -n -C3 'INTER_BANK_TRANSFER_VERIFICATION_KEY|INTRA_BANK_TRANSFER_VERIFICATION_KEY|DISABLE_BIOMETRICS_VERIFICATION_KEY|verification_key'
# Verify SavedStateHandle read/write sites that use verification keys
rg -n -C3 'SavedStateHandle|savedStateHandle|previousBackStackEntry|currentBackStackEntry|getStateFlow|set\('Repository: openMF/mifos-pay
Length of output: 50373
Verify key naming convention consistency and SavedStateHandle usage in interbank transfer flow.
Found naming inconsistency: DISABLE_BIOMETRICS_VERIFICATION_KEY uses package-style naming ("org.mifospay.mifos.authentication.verification.key"), while INTRA_BANK_TRANSFER_VERIFICATION_KEY and INTER_BANK_TRANSFER_VERIFICATION_KEY use hyphenated literal patterns ("intra-banking_transfer_verification_key" and "inter-banking_transfer_verification_key"). This mismatch risks silent failures when passing verification state across modules.
More critically, INTER_BANK_TRANSFER_VERIFICATION_KEY is declared on Line 36 but does not appear to be used anywhere in the interbank auth flow (unlike INTRA_BANK_TRANSFER_VERIFICATION_KEY, which is actively used in TransferConfirmScreen for state synchronization). The injected savedStateHandle in InterbankTransferViewModel similarly shows no usage of any verification key, suggesting the interbank verification result propagation is incomplete.
Ensure:
- All verification-key constants follow a single naming convention (recommend package-style for consistency with security context)
INTER_BANK_TRANSFER_VERIFICATION_KEYis actually used to propagate auth results back to the confirmation screen, or remove the unused constant andsavedStateHandleinjection if not required
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/transfer-interbank/src/commonMain/kotlin/org/mifospay/feature/transfer/interbank/InterbankTransferViewModel.kt`
at line 36, The verification key constants are inconsistent and
INTER_BANK_TRANSFER_VERIFICATION_KEY is unused; update constants to a single
package-style naming (e.g., match DISABLE_BIOMETRICS_VERIFICATION_KEY pattern)
and then either wire the interbank flow to use that key or remove the unused
pieces: in InterbankTransferViewModel use the injected savedStateHandle to set
the verification result under INTER_BANK_TRANSFER_VERIFICATION_KEY when
authentication completes, and update TransferConfirmScreen (or the caller) to
read that same package-style key (or conversely remove
INTER_BANK_TRANSFER_VERIFICATION_KEY and the savedStateHandle injection if
interbank does not need state propagation). Ensure you reference and reconcile
DISABLE_BIOMETRICS_VERIFICATION_KEY, INTRA_BANK_TRANSFER_VERIFICATION_KEY,
INTER_BANK_TRANSFER_VERIFICATION_KEY, InterbankTransferViewModel,
savedStateHandle, and TransferConfirmScreen so naming and usage are consistent.
Comment on lines
+301
to
+319
| private suspend fun handleUserVerification( | ||
| onSuccess: suspend () -> Unit, | ||
| onFailed: suspend () -> Unit, | ||
| ) { | ||
| mutableStateFlow.update { it.copy(userVerificationResult = null) } | ||
|
|
||
| sendEvent(TransferConfirmEvent.NavigateForPasscodeVerification) | ||
|
|
||
| val result = stateFlow | ||
| .map { it.userVerificationResult } | ||
| .filter { it != null } | ||
| .first() | ||
|
|
||
| when (result) { | ||
| true -> onSuccess() | ||
| false -> onFailed() | ||
| null -> {} | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Potential indefinite suspension if user backs out of verification.
The handleUserVerification function suspends indefinitely waiting for userVerificationResult to become non-null:
val result = stateFlow
.map { it.userVerificationResult }
.filter { it != null }
.first()If the user navigates back from the passcode screen without completing verification:
isProcessingis alreadytrue(set at line 115)- The coroutine remains suspended waiting for a result that never arrives
- The UI may become unresponsive or the button remains disabled
Consider adding a timeout or handling the back-navigation case by treating it as a failure:
🛠️ Suggested approach with timeout
private suspend fun handleUserVerification(
onSuccess: suspend () -> Unit,
onFailed: suspend () -> Unit,
) {
mutableStateFlow.update { it.copy(userVerificationResult = null) }
sendEvent(TransferConfirmEvent.NavigateForPasscodeVerification)
- val result = stateFlow
- .map { it.userVerificationResult }
- .filter { it != null }
- .first()
+ val result = withTimeoutOrNull(60_000L) { // 60 second timeout
+ stateFlow
+ .map { it.userVerificationResult }
+ .filter { it != null }
+ .first()
+ }
when (result) {
true -> onSuccess()
- false -> onFailed()
- null -> {}
+ false, null -> onFailed() // Treat timeout/cancellation as failure
}
}This requires adding import kotlinx.coroutines.withTimeoutOrNull.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/transfer-intrabank/src/commonMain/kotlin/org/mifospay/feature/transfer/intrabank/confirm/TransferConfirmViewModel.kt`
around lines 301 - 319, handleUserVerification can suspend forever waiting for
userVerificationResult; change it to await a result with a timeout (use
kotlinx.coroutines.withTimeoutOrNull) after sending
NavigateForPasscodeVerification so the coroutine resumes if the user backs out.
Specifically, wrap the stateFlow.map/filter/.first() wait in
withTimeoutOrNull(...) and treat a null timeout result as a failure path (call
onFailed(), reset mutableStateFlow/isProcessing as needed) so the UI and button
state are restored even when verification is cancelled.
Collaborator
There was a problem hiding this comment.
@TheKalpeshPawar This is correct finding though I don't think the suggested changes will work properly and while navigating to another screen the ViewModel will recreate and i don't know how it's working now and you're collecting data inside of suspend function..and if we're delaying the internal navigation event then in production the app might crush with ANR: Dispatch timed out error. So find the proper production level solution and change the codebase accordingly
Contributor
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Contributor
Author
There was a problem hiding this comment.
@coderabbitai. What is your suggestion for a production level solution for this?
Contributor
There was a problem hiding this comment.
@TheKalpeshPawar Great question. The reviewer is right — the "suspend and wait for UI result" pattern is fundamentally at odds with how ViewModels should work. Here's the production-level solution using a reactive state-machine approach, which eliminates the suspension entirely.
The Core Problem
The current approach:
- Sets
isProcessing = true - Navigates to passcode screen
- Suspends a coroutine waiting for
userVerificationResultto become non-null
If the user presses back without completing verification, nothing ever sets the result, the coroutine hangs forever, and the UI stays stuck with isProcessing = true.
Production-Level Fix: Reactive State Machine
Instead of suspending and waiting, emit the navigation event and react when UpdateUserVerificationResult arrives. No coroutine is left dangling.
Step 1 — Add a "pending verification" flag to state:
`@Serializable`
internal data class TransferConfirmState(
// ... existing fields ...
val isProcessing: Boolean = false,
val isPendingVerification: Boolean = false, // <-- ADD THIS
val userVerificationResult: Boolean? = null,
)Step 2 — Replace initiateTransfer and remove handleUserVerification entirely:
private fun initiateTransfer() {
// Reset any stale result, mark that we are waiting for verification
mutableStateFlow.update {
it.copy(
userVerificationResult = null,
isPendingVerification = true,
)
}
sendEvent(TransferConfirmEvent.NavigateForPasscodeVerification)
// No suspension. We return here. The result will arrive via UpdateUserVerificationResult.
}Step 3 — React to the result in handleAction:
is TransferConfirmAction.UpdateUserVerificationResult -> {
// Guard: only act if we were actually waiting for verification
if (!state.isPendingVerification) return
mutableStateFlow.update {
it.copy(
userVerificationResult = action.result,
isPendingVerification = false,
)
}
if (action.result) {
viewModelScope.launch { handleTransfer() }
} else {
mutableStateFlow.update {
it.copy(
isProcessing = false,
dialogState = TransferConfirmState.DialogState
.Error.ValidationError(
Res.string.feature_make_transfer_user_verification_failed,
),
)
}
}
}Step 4 — Handle the back-navigation case (user dismisses passcode screen without result):
The TransferConfirmScreen already observes entryStateHandle for the verification key. When the user backs out, no value is ever written to SavedStateHandle, so UpdateUserVerificationResult is never sent. You need to handle this in the screen's DisposableEffect or via a lifecycle observer:
// In TransferConfirmScreen.kt
DisposableEffect(Unit) {
onDispose { }
}
// Better: observe when the passcode destination leaves the back stack.
// In MifosNavHost / internalMifosPasscodeScreen, on back-navigation without
// a result, explicitly write `false` to the savedStateHandle:
// backStackEntry.savedStateHandle[verificationKey] = falseThis ensures a false result is always emitted even on back-press, which triggers UpdateUserVerificationResult(false) in the ViewModel and resets isProcessing = false.
Why This Is Production-Safe
| Concern | Old approach | New approach |
|---|---|---|
| User backs out | Coroutine hangs forever | false written to SavedStateHandle → onFailed runs |
| ViewModel recreation | Suspended coroutine lost | State machine is pure state; isPendingVerification survives |
| ANR risk | None (coroutine), but UI stuck | No suspension at all |
| Collecting in suspend | Yes (anti-pattern) | No; purely event-driven |
| Testability | Hard to test timing | Simple unit test of handleAction |
The key insight is: never suspend a ViewModel coroutine waiting for a UI interaction result. Instead, emit an event, return immediately, and handle the incoming action when it arrives.
Contributor
Author
|
@niyajali @therajanmaurya I am going to implement the timeout feature, for payments. Bases on the coderabbit's suggestion. |
niyajali
requested changes
Mar 16, 2026
| ) { | ||
| val state by viewModel.stateFlow.collectAsStateWithLifecycle() | ||
|
|
||
| val authResult by entryStateHandle |
Collaborator
There was a problem hiding this comment.
I don't think this the best way..find some alternative approach
Contributor
Author
There was a problem hiding this comment.
The back option is disabled on the passcode screen. So, the result cannot be null.
What other option I should? Navigation argument?
Contributor
Author
There was a problem hiding this comment.
@niyajali
A similar approach is used here:
https://developer.android.com/guide/navigation/use-graph/conditional
- Introduce `UserVerificationRepository` and its implementation to manage a 30-second verification window stored in platform settings. - Record a verification timestamp in `MifosNavHost` upon successful authentication via the internal passcode screen. - Update `TransferConfirmViewModel` to validate and consume the verification state before processing transfers, showing an error if expired or missing. - Update `SettingsViewModel` to ensure a valid verification exists before allowing the removal of biometric registration. - Register `UserVerificationRepository` in `RepositoryModule` for dependency injection.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/settings/src/commonMain/kotlin/org/mifospay/feature/settings/SettingsViewModel.kt (1)
155-181:⚠️ Potential issue | 🟠 MajorAvoid creating unbounded collectors in
handlePasscodeVerification.Each call to this function starts a new
authenticationSuccess.collectthat never completes. With repeated toggling of biometrics, collectors accumulate and all will process the same emission, causing duplicated side effects (multipleconsumeVerification()calls, redundant state updates, repeated key removals).Replace with a one-shot pattern using
.filterNotNull().first()paired with job cancellation to ensure only the latest invocation processes the result:🛠️ Suggested fix (single active job + one-shot wait)
+import kotlinx.coroutines.Job +import kotlinx.coroutines.flow.filterNotNull +import kotlinx.coroutines.flow.first ... + private var passcodeVerificationJob: Job? = null ... private fun handlePasscodeVerification() { - viewModelScope.launch { - authenticationSuccess.collect { result -> - when (result) { + passcodeVerificationJob?.cancel() + passcodeVerificationJob = viewModelScope.launch { + when (val result = authenticationSuccess.filterNotNull().first()) { true -> { if (userVerificationRepository.consumeVerification()) { passcodeManager.trySendAction(PasscodeAction.DeleteBiometricRegistration) mutableStateFlow.update { it.copy( isBiometricsRegistered = false, ) } } - savedStateHandle.remove<Boolean?>(DISABLE_BIOMETRICS_VERIFICATION_KEY) - authenticationSuccess.value = null } false -> { mutableStateFlow.update { it.copy(isBiometricsRegistered = true) } - savedStateHandle.remove<Boolean?>(DISABLE_BIOMETRICS_VERIFICATION_KEY) - authenticationSuccess.value = null } - null -> {} - } + } + savedStateHandle.remove<Boolean?>(DISABLE_BIOMETRICS_VERIFICATION_KEY) + authenticationSuccess.value = null } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/settings/src/commonMain/kotlin/org/mifospay/feature/settings/SettingsViewModel.kt` around lines 155 - 181, handlePasscodeVerification currently launches a new unbounded collector on authenticationSuccess each call, causing duplicate side effects; change it to keep a single cancellable job (e.g., a private var passcodeVerificationJob: Job?) that you cancel before starting a new viewModelScope.launch, and inside that coroutine await a one-shot emission using authenticationSuccess.filterNotNull().first() (or first { it != null }) instead of collect; then perform the existing logic (call userVerificationRepository.consumeVerification(), passcodeManager.trySendAction(PasscodeAction.DeleteBiometricRegistration), mutableStateFlow.update(...), savedStateHandle.remove(DISABLE_BIOMETRICS_VERIFICATION_KEY), and reset authenticationSuccess.value) so only the latest invocation processes the result once.
♻️ Duplicate comments (1)
feature/transfer-intrabank/src/commonMain/kotlin/org/mifospay/feature/transfer/intrabank/confirm/TransferConfirmViewModel.kt (1)
321-330:⚠️ Potential issue | 🟠 MajorVerification wait can still hang indefinitely when no result is posted.
The
first()wait on non-nulluserVerificationResultcan block forever if the passcode flow exits without writing a value, leaving the transfer in processing state.🛠️ Suggested bounded wait fallback
+import kotlinx.coroutines.withTimeoutOrNull ... - val result = stateFlow - .map { it.userVerificationResult } - .filter { it != null } - .first() + val result = withTimeoutOrNull(30_000L) { + stateFlow + .map { it.userVerificationResult } + .filter { it != null } + .first() + } when (result) { true -> onSuccess() false -> onFailed() - null -> {} + null -> onFailed() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/transfer-intrabank/src/commonMain/kotlin/org/mifospay/feature/transfer/intrabank/confirm/TransferConfirmViewModel.kt` around lines 321 - 330, The current wait uses stateFlow.map { it.userVerificationResult }.filter { it != null }.first(), which can hang forever if no result is posted; change this to a bounded wait using a coroutine timeout (e.g., kotlinx.coroutines.withTimeoutOrNull) around collecting the mapped value from stateFlow (referencing stateFlow and userVerificationResult in TransferConfirmViewModel) and treat a timed-out/null return as the fallback path (call onFailed() or other cleanup) while keeping the true -> onSuccess() and false -> onFailed() branches intact.
🧹 Nitpick comments (1)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/MifosNavHost.kt (1)
180-186: Limit verification recording to keyed authentication flows.
recordVerification()currently runs even whenverificationKeyis null. That can create a valid verification window for passcode flows that did not request a verifiable result.♻️ Suggested tightening
- onAuthenticationSuccess = { verificationKey -> - userVerificationRepository.recordVerification() - verificationKey?.let { + onAuthenticationSuccess = { verificationKey -> + verificationKey?.let { + userVerificationRepository.recordVerification() navController.previousBackStackEntry ?.savedStateHandle ?.set(it, true) } navController.popBackStack() },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/MifosNavHost.kt` around lines 180 - 186, Move the call to userVerificationRepository.recordVerification() so it only runs for keyed authentication flows by invoking it inside the verificationKey non-null branch; i.e., inside the onAuthenticationSuccess lambda, call verificationKey?.let { userVerificationRepository.recordVerification(); navController.previousBackStackEntry?.savedStateHandle?.set(it, true) } so recordVerification() is not executed when verificationKey is null.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@feature/settings/src/commonMain/kotlin/org/mifospay/feature/settings/SettingsViewModel.kt`:
- Around line 155-181: handlePasscodeVerification currently launches a new
unbounded collector on authenticationSuccess each call, causing duplicate side
effects; change it to keep a single cancellable job (e.g., a private var
passcodeVerificationJob: Job?) that you cancel before starting a new
viewModelScope.launch, and inside that coroutine await a one-shot emission using
authenticationSuccess.filterNotNull().first() (or first { it != null }) instead
of collect; then perform the existing logic (call
userVerificationRepository.consumeVerification(),
passcodeManager.trySendAction(PasscodeAction.DeleteBiometricRegistration),
mutableStateFlow.update(...),
savedStateHandle.remove(DISABLE_BIOMETRICS_VERIFICATION_KEY), and reset
authenticationSuccess.value) so only the latest invocation processes the result
once.
---
Duplicate comments:
In
`@feature/transfer-intrabank/src/commonMain/kotlin/org/mifospay/feature/transfer/intrabank/confirm/TransferConfirmViewModel.kt`:
- Around line 321-330: The current wait uses stateFlow.map {
it.userVerificationResult }.filter { it != null }.first(), which can hang
forever if no result is posted; change this to a bounded wait using a coroutine
timeout (e.g., kotlinx.coroutines.withTimeoutOrNull) around collecting the
mapped value from stateFlow (referencing stateFlow and userVerificationResult in
TransferConfirmViewModel) and treat a timed-out/null return as the fallback path
(call onFailed() or other cleanup) while keeping the true -> onSuccess() and
false -> onFailed() branches intact.
---
Nitpick comments:
In
`@cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/MifosNavHost.kt`:
- Around line 180-186: Move the call to
userVerificationRepository.recordVerification() so it only runs for keyed
authentication flows by invoking it inside the verificationKey non-null branch;
i.e., inside the onAuthenticationSuccess lambda, call verificationKey?.let {
userVerificationRepository.recordVerification();
navController.previousBackStackEntry?.savedStateHandle?.set(it, true) } so
recordVerification() is not executed when verificationKey is null.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6a8ac974-45f5-4c0c-9efa-cc30a798a30a
📒 Files selected for processing (6)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/MifosNavHost.ktcore/data/src/commonMain/kotlin/org/mifospay/core/data/di/RepositoryModule.ktcore/data/src/commonMain/kotlin/org/mifospay/core/data/repository/UserVerificationRepository.ktcore/data/src/commonMain/kotlin/org/mifospay/core/data/repositoryImpl/UserVerificationRepositoryImpl.ktfeature/settings/src/commonMain/kotlin/org/mifospay/feature/settings/SettingsViewModel.ktfeature/transfer-intrabank/src/commonMain/kotlin/org/mifospay/feature/transfer/intrabank/confirm/TransferConfirmViewModel.kt
…rmScreen - Add a `LaunchedEffect` using `repeatOnLifecycle(Lifecycle.State.RESUMED)` to monitor lifecycle changes. - Reset the user verification result by dispatching `UpdateUserVerificationResult(false)` if the screen is resumed while in a processing state without an active authentication result.
Contributor
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/transfer-intrabank/src/commonMain/kotlin/org/mifospay/feature/transfer/intrabank/confirm/TransferConfirmScreen.kt`:
- Around line 123-132: The lifecycle handler in TransferConfirmScreen
(LocalLifecycleOwner.current + LaunchedEffect +
lifecycleOwner.repeatOnLifecycle) can erroneously cancel transfers because it
uses state.isProcessing && authResult == null; add a new boolean flag
isAwaitingPasscodeVerification to TransferConfirmState, set it true immediately
before dispatching NavigateForPasscodeVerification and set it false when
handling TransferConfirmAction.UpdateUserVerificationResult, then change the
lifecycle check to only send UpdateUserVerificationResult(false) when
state.isProcessing && authResult == null && state.isAwaitingPasscodeVerification
so background/resume events don’t cancel in-flight API calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0dbe0f32-cb7b-414e-a199-477944c431eb
📒 Files selected for processing (1)
feature/transfer-intrabank/src/commonMain/kotlin/org/mifospay/feature/transfer/intrabank/confirm/TransferConfirmScreen.kt
Comment on lines
+123
to
+132
| val lifecycleOwner = LocalLifecycleOwner.current | ||
| LaunchedEffect(lifecycleOwner) { | ||
| lifecycleOwner.repeatOnLifecycle(Lifecycle.State.RESUMED) { | ||
| if (state.isProcessing && authResult == null) { | ||
| viewModel.trySendAction( | ||
| TransferConfirmAction.UpdateUserVerificationResult(false), | ||
| ) | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how isProcessing is used in the ViewModel to understand if it's set
# only for passcode verification or also for other operations (API calls, etc.)
echo "=== Searching for isProcessing usage in TransferConfirmViewModel ==="
fd -t f "TransferConfirmViewModel" --exec cat {}
echo ""
echo "=== Searching for UpdateUserVerificationResult action handling ==="
ast-grep --pattern 'UpdateUserVerificationResult($$$)'Repository: openMF/mifos-pay
Length of output: 17760
🏁 Script executed:
cat feature/transfer-intrabank/src/commonMain/kotlin/org/mifospay/feature/transfer/intrabank/confirm/TransferConfirmScreen.kt | head -150Repository: openMF/mifos-pay
Length of output: 7587
Lifecycle handler may incorrectly cancel in-progress operations.
The repeatOnLifecycle(RESUMED) block fires on every lifecycle resume, not just when returning from the passcode screen. The condition state.isProcessing && authResult == null will evaluate to true in two problematic scenarios:
- While awaiting passcode: If the user switches apps or their phone enters/exits lock screen before completing passcode verification, the handler incorrectly sends
false. - During transfer API execution: After passcode returns and is consumed,
authResultresets to null whileisProcessingremains true throughout the network call. Any app background/resume interrupts this, incorrectly canceling the in-flight transfer.
Introduce a dedicated isAwaitingPasscodeVerification flag set to true only when navigating to the passcode screen and reset after verification completes. This prevents cancellation during general processing states like active API calls.
Suggested approach
LaunchedEffect(lifecycleOwner) {
lifecycleOwner.repeatOnLifecycle(Lifecycle.State.RESUMED) {
- if (state.isProcessing && authResult == null) {
+ if (state.isAwaitingPasscodeVerification && authResult == null) {
viewModel.trySendAction(
TransferConfirmAction.UpdateUserVerificationResult(false),
)
}
}
}Add isAwaitingPasscodeVerification: Boolean = false to TransferConfirmState, set it to true before sending NavigateForPasscodeVerification, and reset it when UpdateUserVerificationResult is received.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/transfer-intrabank/src/commonMain/kotlin/org/mifospay/feature/transfer/intrabank/confirm/TransferConfirmScreen.kt`
around lines 123 - 132, The lifecycle handler in TransferConfirmScreen
(LocalLifecycleOwner.current + LaunchedEffect +
lifecycleOwner.repeatOnLifecycle) can erroneously cancel transfers because it
uses state.isProcessing && authResult == null; add a new boolean flag
isAwaitingPasscodeVerification to TransferConfirmState, set it true immediately
before dispatching NavigateForPasscodeVerification and set it false when
handling TransferConfirmAction.UpdateUserVerificationResult, then change the
lifecycle check to only send UpdateUserVerificationResult(false) when
state.isProcessing && authResult == null && state.isAwaitingPasscodeVerification
so background/resume events don’t cancel in-flight API calls.
Collaborator
There was a problem hiding this comment.
@TheKalpeshPawar Fix this issue and look for other coderabbit suggestions
Contributor
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Collaborator
There was a problem hiding this comment.
do this change and lemme know
- Introduce `isAwaitingPasscodeVerification` to `TransferConfirmState` to specifically track when a user is prompted for authentication. - Update `TransferConfirmViewModel` to toggle `isAwaitingPasscodeVerification` when initiating and completing user verification. - Modify `TransferConfirmScreen` to use the new state flag instead of `isProcessing` when determining if the verification result should be reset on lifecycle resume.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
feature/transfer-intrabank/src/commonMain/kotlin/org/mifospay/feature/transfer/intrabank/confirm/TransferConfirmViewModel.kt (1)
329-337:⚠️ Potential issue | 🟠 MajorThis
nullbranch never executes.
filter { it != null }.first()only resumes after a non-null emission, sonull -> {}does not cover the "verification ended without a result" path. That case still leaveshandleUserVerification()suspended andisProcessingstuck.Either guarantee every exit path publishes
false, or move the success/failure transition intoUpdateUserVerificationResultinstead of blocking here.With Kotlin Flow, does `flow.filter { it != null }.first()` ever return `null`, or does it suspend until a non-null value is emitted?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/transfer-intrabank/src/commonMain/kotlin/org/mifospay/feature/transfer/intrabank/confirm/TransferConfirmViewModel.kt` around lines 329 - 337, The current use of stateFlow.map { it.userVerificationResult }.filter { it != null }.first() means the null branch never occurs and handleUserVerification() can remain suspended if no result is ever emitted; fix by either (A) ensuring every exit path emits a non-null Boolean (emit false on cancellations/timeouts) so keep the current when (result) { true -> onSuccess(); false -> onFailed() } logic, or (B) change the await logic to val result = stateFlow.map { it.userVerificationResult }.firstOrNull() and handle the null case by setting isProcessing = false and invoking onFailed() (or handling timeout) in handleUserVerification(); locate code using stateFlow, handleUserVerification(), isProcessing, and the UpdateUserVerificationResult event to implement the chosen fix.
🤖 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/transfer-intrabank/src/commonMain/kotlin/org/mifospay/feature/transfer/intrabank/confirm/TransferConfirmViewModel.kt`:
- Around line 329-337: The current use of stateFlow.map {
it.userVerificationResult }.filter { it != null }.first() means the null branch
never occurs and handleUserVerification() can remain suspended if no result is
ever emitted; fix by either (A) ensuring every exit path emits a non-null
Boolean (emit false on cancellations/timeouts) so keep the current when (result)
{ true -> onSuccess(); false -> onFailed() } logic, or (B) change the await
logic to val result = stateFlow.map { it.userVerificationResult }.firstOrNull()
and handle the null case by setting isProcessing = false and invoking onFailed()
(or handling timeout) in handleUserVerification(); locate code using stateFlow,
handleUserVerification(), isProcessing, and the UpdateUserVerificationResult
event to implement the chosen fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8608b24e-aafa-4bcf-8f23-81a12796e847
📒 Files selected for processing (2)
feature/transfer-intrabank/src/commonMain/kotlin/org/mifospay/feature/transfer/intrabank/confirm/TransferConfirmScreen.ktfeature/transfer-intrabank/src/commonMain/kotlin/org/mifospay/feature/transfer/intrabank/confirm/TransferConfirmViewModel.kt
niyajali
approved these changes
Mar 20, 2026
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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Jira Task: MW-390
Screen Recording
intra-bank-transfer.mp4
addedNavigationTransitionAndAnimation.mp4
Desciption
This pull request refactors the navigation and authentication flow for the passcode feature, improving type safety, flexibility, and code clarity. The main changes include replacing string-based navigation routes with type-safe serializable route objects, updating navigation functions and composable screens to use these new routes, and generalizing the passcode authentication callbacks to support more use cases. Additionally, a verification key is now passed through navigation for internal authentication flows, and constants have been relocated and renamed for better clarity.
Key changes:
Navigation and Routing Improvements:
RootPasscodeRoute,ReAuthPasscodeRoute,InternalPasscodeRoute) and updated all navigation and composable functions to use these types, improving type safety and maintainability (feature/passcode/src/commonMain/kotlin/org/mifos/feature/passcode/MifosPasscodeNavigation.kt).navigateToInternalMifosPasscodeScreennow accept averificationKeyparameter, allowing information to be passed through navigation for authentication flows (feature/passcode/src/commonMain/kotlin/org/mifos/feature/passcode/MifosPasscodeNavigation.kt,cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/MifosNavHost.kt). [1] [2]Passcode Screen Refactoring:
mifosPasscodeScreeninto three specialized composable functions:rootMifosPasscodeScreen,reAuthMifosPasscodeScreen, andinternalMifosPasscodeScreen, each tailored to their respective navigation routes and callback needs (feature/passcode/src/commonMain/kotlin/org/mifos/feature/passcode/MifosPasscodeNavigation.kt).onPasscodeRejectedwith a more generalonAuthenticationFailed, and ensured that verification keys are passed to and from the appropriate callbacks (feature/passcode/src/commonMain/kotlin/org/mifos/feature/passcode/MifosPasscode.kt,cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/MifosNavHost.kt). [1] [2]Constants and Dependency Updates:
DISABLE_BIOMETRICS_VERIFICATION_KEYinSettingsViewModelfor clarity, and removed the old constant fromAuthenticationUtils(feature/settings/src/commonMain/kotlin/org/mifospay/feature/settings/SettingsViewModel.kt,core/ui/src/commonMain/kotlin/org/mifospay/core/ui/utils/AuthenticationUtils.kt). [1] [2]:feature:passcodemodule and its transitive dependencies in the Android build configuration (cmp-android/dependencies/prodReleaseRuntimeClasspath.tree.txt). [1] [2]These changes collectively modernize the navigation and authentication logic, making the codebase more robust and easier to extend for future authentication-related features.
Summary by CodeRabbit
New Features
Chores