MW-346 Integrate the new passcode module, and replace with the old one #1987
Merged
therajanmaurya merged 40 commits intoopenMF:developmentfrom Mar 9, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces direct passcode navigation with a "Choose Auth Option" flow, adds passcode and biometrics feature modules, introduces AppLockOption and repositories for auth option/biometric data, wires DI and navigation, and updates app routing and settings to respect chosen auth option. No public API removals. Changes
Sequence DiagramsequenceDiagram
participant User
participant App as MifosPayApp
participant Repo as ChooseAuthOptionRepo
participant Choose as ChooseAuthOptionScreen
participant Pass as MifosPasscode
participant Bio as PlatformAuthenticator
User->>App: Login success
App->>Repo: getAuthOption()
Repo-->>App: AppLockOption
alt AppLockOption = MifosPasscode
App->>Choose: navigate to ChooseAuthOption
User->>Choose: select MifosPasscode
Choose->>Pass: navigateToRootMifosPasscodeScreen()
Pass->>Repo: saveAuthOption(MifosPasscode)
Pass->>App: onPasscodeConfirm
else AppLockOption = DeviceLock
App->>Choose: navigate to ChooseAuthOption
User->>Choose: select DeviceLock
Choose->>Bio: register/authenticate biometrics
Bio->>Repo: saveBiometricRegistrationData()
Bio->>Repo: saveAuthOption(DeviceLock)
Bio->>App: onAuthenticationSuccess
else AppLockOption = None
App->>Choose: show chooser
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
feature/auth/src/commonMain/composeResources/values/strings.xml (1)
3-3:⚠️ Potential issue | 🟡 MinorStale copyright year (
2024) — all other files in this PR use2026.🛠️ Proposed fix
- Copyright 2024 Mifos Initiative + Copyright 2026 Mifos Initiative🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/auth/src/commonMain/composeResources/values/strings.xml` at line 3, Update the stale copyright line "Copyright 2024 Mifos Initiative" to use 2026 instead (i.e., change the literal string "Copyright 2024 Mifos Initiative" to "Copyright 2026 Mifos Initiative") so it matches the year used in the rest of the PR.feature/auth/src/commonTest/kotlin/org/mifospay/feature/auth/login/LoginViewModelTest.kt (2)
199-199:⚠️ Potential issue | 🟡 MinorStale test method name and comment — still references "PasscodeScreen".
The method name
givenCorrectCredentials_whenLoginSucceeds_thenNavigateToPasscodeScreenEmittedand the comment on Line 231 still referencePasscodeScreen, but the assertion on Line 232 correctly checks forNavigateToChooseAuthOptionScreen. Update both to stay consistent with the new flow.Proposed fix
- fun givenCorrectCredentials_whenLoginSucceeds_thenNavigateToPasscodeScreenEmitted() = runTest { + fun givenCorrectCredentials_whenLoginSucceeds_thenNavigateToChooseAuthOptionScreenEmitted() = runTest {And at Line 231:
- // Expect NavigateToPasscodeScreen after successful login + // Expect NavigateToChooseAuthOptionScreen after successful login🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/auth/src/commonTest/kotlin/org/mifospay/feature/auth/login/LoginViewModelTest.kt` at line 199, Rename the stale test function givenCorrectCredentials_whenLoginSucceeds_thenNavigateToPasscodeScreenEmitted and update its inline comment that still mentions "PasscodeScreen" so they reflect the new flow (e.g., use a name like givenCorrectCredentials_whenLoginSucceeds_thenNavigateToChooseAuthOptionScreenEmitted and update the comment to "NavigateToChooseAuthOptionScreen"), leaving the test body and the existing assertion that checks NavigateToChooseAuthOptionScreen unchanged; ensure the updated function name and comment match the assertion and any references in the test class (e.g., LoginViewModelTest).
52-55:⚠️ Potential issue | 🔴 Critical
LoginViewModelconstructor requiresinstanceConfigManager, but the test doesn't provide it.The production
LoginViewModelconstructor takes three parameters:loginUseCase,instanceConfigManager, andsavedStateHandle. This test only passesloginUseCaseandsavedStateHandle. TheinstanceConfigManagerparameter is required and is actively used in the ViewModel's initialization, so this test would fail to compile.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/auth/src/commonTest/kotlin/org/mifospay/feature/auth/login/LoginViewModelTest.kt` around lines 52 - 55, The test instantiates LoginViewModel without the required instanceConfigManager argument; update LoginViewModelTest to provide a mocked or fake InstanceConfigManager when constructing LoginViewModel (e.g., create a mock InstanceConfigManager and pass it as the instanceConfigManager parameter to the LoginViewModel constructor alongside loginUseCase and SavedStateHandle) so the ViewModel initializes with the required dependency.
🟠 Major comments (17)
feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/components/SystemAuthenticatorButton.kt-60-98 (1)
60-98:⚠️ Potential issue | 🟠 MajorUnreachable
else if (Iris)branch — iris icon can never render.The outer condition on line 61 catches
PlatformAuthOptions.Irisunconditionally:if (platformAuthOptions.contains(PlatformAuthOptions.Iris) || ...)So whenever
Irisis present inplatformAuthOptions, execution always enters the firstifand showsAuthenticateButton("Use Biometrics"). Theelse if (platformAuthOptions.contains(PlatformAuthOptions.Iris))block at line 85–91 is therefore dead code — the eye scanner image can never be displayed.If the intent is to show the iris icon only when Iris is the sole option, the guard condition needs to be revised. For example, reserve the multi-option
AuthenticateButtonfor the case where more than one biometric type is available:🐛 Proposed fix
- if ( - platformAuthOptions.contains(PlatformAuthOptions.Iris) || - ( - platformAuthOptions.contains(PlatformAuthOptions.FaceId) && - platformAuthOptions.contains(PlatformAuthOptions.Fingerprint) - ) - ) { + if ( + ( + platformAuthOptions.contains(PlatformAuthOptions.FaceId) && + platformAuthOptions.contains(PlatformAuthOptions.Fingerprint) + ) || platformAuthOptions.count { + it == PlatformAuthOptions.Iris || + it == PlatformAuthOptions.FaceId || + it == PlatformAuthOptions.Fingerprint + } > 1 + ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/components/SystemAuthenticatorButton.kt` around lines 60 - 98, The Iris branch is unreachable because the first if treats PlatformAuthOptions.Iris the same as any multi-option case; change the first conditional (the one that decides to render AuthenticateButton) to only trigger when more than one biometric option is present (e.g., count of biometric options in platformAuthOptions > 1 or contains at least two of FaceId/Fingerprint/Iris), and keep the later else if checks (including PlatformAuthOptions.Iris) intact so the iris Image (painterResource(Res.drawable.eye_scanner)) is rendered when Iris is the sole biometric option; update the condition logic around AuthenticateButton and platformAuthOptions checks accordingly (referencing AuthenticateButton, platformAuthOptions, and PlatformAuthOptions.Iris/FaceId/Fingerprint).feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/components/SystemAuthenticatorButton.kt-74-74 (1)
74-74:⚠️ Potential issue | 🟠 MajorWrong
contentDescriptionreused across all biometric image types — accessibility defect.Every
Image— face scan, eye scanner, and keypad — usesRes.string.feature_authenticator_biometrics_fingerprint_iconas itscontentDescription. Screen readers will announce "fingerprint" for face and iris images, giving blind or low-vision users incorrect feedback about which biometric type is being offered.Each image should reference a semantically matching description string:
♿ Proposed fix (Android branch shown; apply the same pattern to the iOS branch)
Image( painter = painterResource(Res.drawable.face_scan), - contentDescription = stringResource(Res.string.feature_authenticator_biometrics_fingerprint_icon), + contentDescription = stringResource(Res.string.feature_authenticator_biometrics_face_icon), ... ) ... Image( painter = painterResource(Res.drawable.eye_scanner), - contentDescription = stringResource(Res.string.feature_authenticator_biometrics_fingerprint_icon), + contentDescription = stringResource(Res.string.feature_authenticator_biometrics_iris_icon), ... ) ... Image( painter = painterResource(Res.drawable.keypad), - contentDescription = stringResource(Res.string.feature_authenticator_biometrics_fingerprint_icon), + contentDescription = stringResource(Res.string.feature_authenticator_biometrics_credential_icon), ... )New string resources for
face_icon,iris_icon, andcredential_iconneed to be added alongside the existingfingerprint_iconstring.Also applies to: 81-81, 88-88, 95-95, 103-103, 115-115, 122-122
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/components/SystemAuthenticatorButton.kt` at line 74, The Image contentDescription in SystemAuthenticatorButton (function SystemAuthenticatorButton) incorrectly reuses Res.string.feature_authenticator_biometrics_fingerprint_icon for all biometric types; add new string resources (e.g. feature_authenticator_biometrics_face_icon, _iris_icon, _credential_icon) and update each Image's contentDescription call to reference the semantically matching string resource for that icon (replace fingerprint constant in the Image composables for face, iris, keypad/credential usages). Ensure you update both Android and iOS resource sets and keep the existing fingerprint string for fingerprint images.build-logic/convention/src/main/kotlin/CMPFeatureConventionPlugin.kt-37-38 (1)
37-38:⚠️ Potential issue | 🟠 MajorAdding auth libraries to the shared convention plugin forces them onto every feature module.
CMPFeatureConventionPluginis applied to all CMP feature modules (~20+). Placingmifos.authenticator.passcodeandmifos.authenticator.biometricshere means every unrelated feature (home, history, faq, finance, etc.) unconditionally pulls in both libraries, increasing binary size, incremental build cost, and coupling every feature to the auth library API surface.These dependencies should be declared only in the
build.gradle.ktsfiles of the specific modules that consume them (feature:passcode,feature:authenticator-biometrics, and wherever the composition provider is wired, e.g.cmp-shared).♻️ Proposed fix — remove from convention plugin
- add("commonMainImplementation", libs.findLibrary("mifos.authenticator.passcode").get()) - add("commonMainImplementation", libs.findLibrary("mifos.authenticator.biometrics").get())Then add them directly to
feature/passcode/build.gradle.kts,feature/authenticator-biometrics/build.gradle.kts, and any other module that explicitly depends on them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build-logic/convention/src/main/kotlin/CMPFeatureConventionPlugin.kt` around lines 37 - 38, Remove the unconditional addition of the auth libraries from CMPFeatureConventionPlugin: delete the two add("commonMainImplementation", libs.findLibrary("mifos.authenticator.passcode").get()) and add("commonMainImplementation", libs.findLibrary("mifos.authenticator.biometrics").get()) calls in CMPFeatureConventionPlugin, and instead declare those dependencies only in the specific feature module build files (e.g., feature/passcode/build.gradle.kts and feature/authenticator-biometrics/build.gradle.kts) and where the composition provider is wired (e.g., cmp-shared), so only modules that consume the APIs pull in mifos.authenticator.passcode and mifos.authenticator.biometrics.core/data/src/commonMain/kotlin/org/mifospay/core/data/repository/ChooseAuthOptionRepository.kt-14-22 (1)
14-22: 🛠️ Refactor suggestion | 🟠 Major
saveBiometricRegistrationDataviolates the Single Responsibility Principle.
ChooseAuthOptionRepositorymanages which auth lock option the user has selected. Persisting biometric registration credentials is a distinct concern already covered byPlatformAuthenticationDataRepository. Having this method here forces every implementation and test double to carry unrelated biometric storage logic.Move
saveBiometricRegistrationDatatoPlatformAuthenticationDataRepository(or its interface), and have call sites depend on the correct repository.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/commonMain/kotlin/org/mifospay/core/data/repository/ChooseAuthOptionRepository.kt` around lines 14 - 22, The method saveBiometricRegistrationData currently in ChooseAuthOptionRepository mixes responsibilities—remove saveBiometricRegistrationData from interface ChooseAuthOptionRepository and add it to PlatformAuthenticationDataRepository (or its interface) so biometric persistence lives with authentication data; update all call sites to depend on PlatformAuthenticationDataRepository.saveBiometricRegistrationData and adjust implementations and test doubles of ChooseAuthOptionRepository and PlatformAuthenticationDataRepository to reflect the move.cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayApp.kt-83-98 (1)
83-98:⚠️ Potential issue | 🟠 MajorExisting users without a stored
AppLockOptionwill be silently logged out after this update.
AppLockOption.None(the default when no value is stored) currently triggers an immediate logout. Existing users upgrading from the version before this PR have no stored auth option and will be force-logged-out on every app start until they re-authenticate and complete theChooseAuthOptionflow.Consider routing
AppLockOption.Noneto theChooseAuthOptionscreen instead of logging out, or provide a migration path that stores a sensible default for existing authenticated sessions.Would you like me to open a new issue to track the migration/upgrade path for existing users, or generate a proposed routing change?
🤖 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/MifosPayApp.kt` around lines 83 - 98, The current navDestination logic in MifosPayApp.kt treats AppLockOption.None by calling viewModel.logOut() and sending users to LOGIN_GRAPH, which forces existing users without a stored option to be logged out; change the handling of (viewModel.getAuthOption()) when it returns AppLockOption.None to route to a ChooseAuthOption screen (e.g., CHOOSE_AUTH_OPTION_ROUTE) instead of invoking viewModel.logOut(), or alternatively add a migration that seeds a sensible default AppLockOption for authenticated sessions before this when-block runs; update the when branch for AppLockOption.None to return CHOOSE_AUTH_OPTION_ROUTE (or perform the migration via a new viewModel.ensureAuthOptionForExistingUser() call) and remove the immediate viewModel.logOut() call so existing users are not silently logged out.cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayApp.kt-86-86 (1)
86-86:⚠️ Potential issue | 🟠 Major
viewModel.getAuthOption()is called on every recomposition without memoization.This synchronous repository read runs on the composition thread during every recomposition triggered by
uiStateor any parent. IfChooseAuthOptionRepositoryImplperforms any I/O or lock acquisition (e.g., reading from aSettingsstore), this can cause jank on the main thread. Wrap inremember(uiState)(as shown in the fix above) to limit the call to state changes only.🤖 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/MifosPayApp.kt` at line 86, Replace the direct call to viewModel.getAuthOption() inside the composable with a memoized call so it doesn't execute on every recomposition; specifically, wrap the call in a Compose remember keyed to uiState (e.g., use remember(uiState) { viewModel.getAuthOption() }) so ChooseAuthOptionRepositoryImpl reads are only performed when uiState changes rather than on every recomposition in MifosPayApp.kt.feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/components/AuthOptionCard.kt-74-79 (1)
74-79:⚠️ Potential issue | 🟠 MajorUse
RadioButtoncomposable instead of icon-based radio buttons for accessibility.
Icons.Outlined.RadioButtonChecked/Uncheckedrender correctly visually but carry no semantic role or state for accessibility services (TalkBack/Switch Access). A properRadioButton(selected = selected, onClick = null)composable correctly announces its role and selection state to screen readers.♿ Proposed fix
-import androidx.compose.material.icons.outlined.RadioButtonChecked -import androidx.compose.material.icons.outlined.RadioButtonUnchecked +import androidx.compose.material3.RadioButton ... - Icon( - imageVector = if (selected) Icons.Outlined.RadioButtonChecked else Icons.Outlined.RadioButtonUnchecked, - contentDescription = stringResource(Res.string.feature_auth_radiobutton), - modifier = Modifier.size(25.dp), - tint = if (selected) KptTheme.colorScheme.primary else KptTheme.colorScheme.onSurfaceVariant, - ) + RadioButton( + selected = selected, + onClick = null, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/components/AuthOptionCard.kt` around lines 74 - 79, The Icon used in AuthOptionCard to represent selection should be replaced with the RadioButton composable so screen readers get proper role and state; locate the Icon call inside AuthOptionCard (the imageVector using Icons.Outlined.RadioButtonChecked/Unchecked) and swap it to RadioButton(selected = selected, onClick = null) preserving the Modifier.size(25.dp) and applying equivalent colors (use RadioButtonDefaults.colors or map KptTheme colorScheme.primary and onSurfaceVariant) so visual appearance remains while enabling accessibility semantics.feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/AuthenticationScreen.kt-116-128 (1)
116-128:⚠️ Potential issue | 🟠 Major
confirmButtonText = ""renders an invisible but still-focusable confirm button in both dialogs.
MifosDialogBoxrenders aTextButtonunconditionally forconfirmButtonText(it only gates ondismissButtonText). An empty string produces an empty, zero-width interactive button that is still reachable via keyboard/accessibility traversal, and bothonConfirmandonDismissdispatchOnDismissDialog/OkayOnUserNotRegisteredErroridentically anyway. Either pass a non-empty label and keep distinct actions, or usenull/blank forconfirmButtonTextif the API supports suppressing it — but looking at theMifosDialogBoximplementation it does not. The simplest fix is to remove the confirm button by making the design single-action (dismiss only), or add suppression support toMifosDialogBox.🔧 Simplest fix — swap roles so dismiss shows the visible "OK" button
- MifosDialogBox( - title = stringResource(Res.string.feature_authenticator_biometrics_error), - showDialogState = true, - confirmButtonText = "", - dismissButtonText = stringResource(Res.string.feature_authenticator_biometrics_ok), - onConfirm = { onAction(AuthenticationScreenAction.OnDismissDialog) }, - onDismiss = { onAction(AuthenticationScreenAction.OnDismissDialog) }, - message = screenState.message, - ) + MifosDialogBox( + title = stringResource(Res.string.feature_authenticator_biometrics_error), + showDialogState = true, + confirmButtonText = stringResource(Res.string.feature_authenticator_biometrics_ok), + onConfirm = { onAction(AuthenticationScreenAction.OnDismissDialog) }, + onDismiss = { onAction(AuthenticationScreenAction.OnDismissDialog) }, + message = screenState.message, + )Apply the same pattern to the
UserNotRegistereddialog.Also applies to: 131-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/AuthenticationScreen.kt` around lines 116 - 128, The dialog is rendering an invisible but focusable confirm button because confirmButtonText = "" is always rendered; change to make the visible single action be the confirm button by swapping labels so confirmButtonText = stringResource(Res.string.feature_authenticator_biometrics_ok) and dismissButtonText = "" (or omitted per API), and wire onConfirm to AuthenticationScreenAction.OnDismissDialog (remove duplicate/no-op onDismiss or leave it as a no-op). Apply the same change to the other dialog instance (the UserNotRegistered dialog) so MifosDialogBox no longer creates an empty focusable confirm button.feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/AuthenticationScreen.kt-73-77 (1)
73-77:⚠️ Potential issue | 🟠 Major
LaunchedEffect(Unit)may missNOT_SETUPstatus arriving after initial composition.
authenticatorStatusis collected from aStateFlowthat may start as empty and populate asynchronously. Because the key isUnit, this effect fires exactly once on first composition. IfNOT_SETUParrives in a subsequent emission,AuthenticatorStatusNotSetupis never dispatched and the user is left on the screen with no way to proceed.🔧 Proposed fix — key on
authenticatorStatus- LaunchedEffect(Unit) { + LaunchedEffect(authenticatorStatus) { if (authenticatorStatus.contains(PlatformAuthenticatorStatus.NOT_SETUP)) { onAction(AuthenticationScreenAction.AuthenticatorStatusNotSetup) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/AuthenticationScreen.kt` around lines 73 - 77, The LaunchedEffect using Unit in AuthenticationScreen runs only once and can miss later updates to the StateFlow; replace the one-shot effect with a reactive effect keyed on authenticatorStatus (or collect changes via snapshotFlow/collectAsState) so that whenever authenticatorStatus contains PlatformAuthenticatorStatus.NOT_SETUP you call onAction(AuthenticationScreenAction.AuthenticatorStatusNotSetup); update the LaunchedEffect(Unit) usage to LaunchedEffect(authenticatorStatus) or equivalent to ensure subsequent emissions trigger the dispatch.feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/AuthenticationScreenViewModel.kt-99-101 (1)
99-101: 🛠️ Refactor suggestion | 🟠 Major
authenticatorStatusfield inAuthenticationScreenStateis never populated — dead code.
AuthenticationScreenState.authenticatorStatusis initialized toemptySet()and noupdateStatecall in the ViewModel ever modifies it. The UI reads the status directly fromplatformAuthenticationProvider.authenticatorStatusinAuthenticationContent, making this field unused. Remove it to avoid confusion, or wire it from the ViewModel if centralized state management was intended.🔧 Proposed fix
data class AuthenticationScreenState( - val authenticatorStatus: Set<PlatformAuthenticatorStatus> = emptySet(), val screenState: ScreenState? = null, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/AuthenticationScreenViewModel.kt` around lines 99 - 101, AuthenticationScreenState currently defines authenticatorStatus but it is never populated; either remove the dead field or wire it up from the ViewModel so state is authoritative. To fix, pick one: (A) remove the authenticatorStatus property from AuthenticationScreenState and update AuthenticationContent to read directly from platformAuthenticationProvider.authenticatorStatus; or (B) keep authenticatorStatus and in AuthenticationScreenViewModel (where updateState is used) add logic to read platformAuthenticationProvider.authenticatorStatus and include it when creating/updating AuthenticationScreenState, then update AuthenticationContent to read the status from state instead of platformAuthenticationProvider. Reference symbols: AuthenticationScreenState, authenticatorStatus, AuthenticationScreenViewModel, updateState, AuthenticationContent, platformAuthenticationProvider.feature/authenticator-biometrics/src/commonMain/composeResources/values/strings.xml-15-15 (1)
15-15:⚠️ Potential issue | 🟠 Major
feature_authenticator_biometrics_fingerprint_iconis reused ascontentDescriptionfor face-scan and iris-scan images — accessibility defect.In
feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/components/SystemAuthenticatorButton.kt(Lines 80, 86, 92), the face_scan and eye_scanner images usestringResource(Res.string.feature_authenticator_biometrics_fingerprint_icon)as theircontentDescription. Screen readers will announce "Fingerprint icon" when displaying a face or iris icon, which is misleading for visually impaired users.Add dedicated strings for each icon type:
🛡️ Proposed fix
+ <string name="feature_authenticator_biometrics_face_scan_icon">Face scan icon</string> + <string name="feature_authenticator_biometrics_iris_scan_icon">Iris scanner icon</string> + <string name="feature_authenticator_biometrics_keypad_icon">Keypad icon</string>Then update
SystemAuthenticatorButton.ktto use the appropriate string for each image.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/authenticator-biometrics/src/commonMain/composeResources/values/strings.xml` at line 15, Add separate accessible labels for face and iris icons in strings.xml (e.g., feature_authenticator_biometrics_face_icon and feature_authenticator_biometrics_iris_icon) instead of reusing feature_authenticator_biometrics_fingerprint_icon, then update SystemAuthenticatorButton.kt to replace the stringResource(Res.string.feature_authenticator_biometrics_fingerprint_icon) usage for the face_scan Image and the eye_scanner Image with stringResource(Res.string.feature_authenticator_biometrics_face_icon) and stringResource(Res.string.feature_authenticator_biometrics_iris_icon) respectively, leaving the fingerprint image using the original fingerprint string.cmp-android/dependencies/prodReleaseRuntimeClasspath.txt-433-436 (1)
433-436:⚠️ Potential issue | 🟠 Major
components-ui-tooling-previewat 1.9.3 is outdated and deprecated for Compose Multiplatform 1.10.1.
org.jetbrains.compose.components:components-ui-tooling-preview(line 435–436) is pinned at1.9.3while the rest of the Compose family is at1.10.1. This artifact is deprecated in Compose MP 1.10 and should be updated to1.10.1at minimum. Better: the newerorg.jetbrains.compose.ui:ui-tooling-preview(already present at1.10.1in your classpath) is the recommended replacement. Using the outdated legacy artifact can cause binary incompatibilities and version conflicts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmp-android/dependencies/prodReleaseRuntimeClasspath.txt` around lines 433 - 436, The dependency list includes the deprecated org.jetbrains.compose.components:components-ui-tooling-preview at 1.9.3 while other Compose artifacts are 1.10.1; update or remove it to avoid conflicts: either bump org.jetbrains.compose.components:components-ui-tooling-preview to 1.10.1 to match the rest, or preferably remove that legacy artifact and rely on the recommended org.jetbrains.compose.ui:ui-tooling-preview (already present at 1.10.1) to eliminate the deprecated dependency and prevent binary incompatibilities.core/data/src/commonMain/kotlin/org/mifospay/core/data/repositoryImpl/PlatformAuthenticationDataRepositoryImpl.kt-20-25 (1)
20-25:⚠️ Potential issue | 🟠 MajorEncrypt sensitive data stored in settings.
Biometric registration data at lines 21 and 25 is stored in plain text via
settings.putString()andsettings.getString(). This pattern extends to other sensitive data (authentication tokens, passcodes) throughout the codebase. All should use encrypted/keystore-backed storage instead of plain serialization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/commonMain/kotlin/org/mifospay/core/data/repositoryImpl/PlatformAuthenticationDataRepositoryImpl.kt` around lines 20 - 25, The biometric registration data is being persisted in plain text via settings.putString and read with settings.getString in saveBiometricRegistrationData/getBiometricRegistrationData (key BIOMETRIC_REGISTRATION_KEY); replace these calls with encrypted storage calls (e.g., use a keystore-backed encrypt/decrypt helper or an EncryptedSettings wrapper) so that saveBiometricRegistrationData encrypts registrationData before storing and getBiometricRegistrationData decrypts the retrieved value before returning; update or introduce an EncryptionManager (or use platform keystore APIs) and swap settings.putString/getString to the encrypted equivalents across these methods (and the same pattern for other sensitive methods/keys).cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/RootNavGraph.kt-65-67 (1)
65-67:⚠️ Potential issue | 🟠 MajorVerify
navigateToMainGraphback-stack clearing strategy—popUpTo(findStartDestination())may leave auth screens reachable.
navigateToMainGraph()does includepopUpTo(graph.findStartDestination().id)but this approach pops back to the start destination rather than removing auth/root routes. If the NavHost's startDestination is set to an auth screen, back-press could still return users to login/lock screens.Lines 65–67 also diverge from the pattern used at lines 54–55 and 73–74, which explicitly call
popBackStack()before navigating. For consistency and reliability, either:
- Change
popUpToto useMifosNavGraph.ROOT_GRAPHwithinclusive = trueto fully clear the auth stack, or- Add
popBackStack()calls at lines 65–67 to match the safe pattern used elsewhere.🤖 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/RootNavGraph.kt` around lines 65 - 67, The navigateToMainGraph usage on navHostController (used for onSkipButton, onPasscodeConfirm, onPasscodeCreation) may leave auth screens on the back stack; update the navigation to fully clear the auth/root graph by either calling navHostController.popBackStack() immediately before invoking navigateToMainGraph (to match the pattern at lines 54–55 and 73–74) or modify the navigateToMainGraph implementation to use popUpTo(MifosNavGraph.ROOT_GRAPH) with inclusive = true instead of popUpTo(graph.findStartDestination().id) so the root/auth routes are removed; apply the chosen approach consistently for these three call sites and ensure references are to navigateToMainGraph and navHostController.core/data/src/commonMain/kotlin/org/mifospay/core/data/repositoryImpl/MifosPasscodeAdapterImpl.kt-22-24 (1)
22-24:⚠️ Potential issue | 🟠 MajorUse encrypted storage for passcode persistence.
The passcode is stored in plaintext using the
russhwolf.settings.Settingslibrary, which maps to Android SharedPreferences and iOS UserDefaults without encryption. Replace with encrypted storage such as Android's EncryptedSharedPreferences or a secure keystore wrapper to prevent exposure if device storage is compromised.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/commonMain/kotlin/org/mifospay/core/data/repositoryImpl/MifosPasscodeAdapterImpl.kt` around lines 22 - 24, The savePasscode implementation in MifosPasscodeAdapterImpl is persisting passcodes in plaintext via settings.putString(MIFOS_PASSCODE, passcode); replace this with a secure encrypted storage mechanism: on Android use EncryptedSharedPreferences or a keystore-backed wrapper and on iOS use Keychain/secure storage, and update MifosPasscodeAdapterImpl.savePasscode and any corresponding load/delete methods to call the platform-specific secure API (instead of russhwolf.settings) while keeping the MIFOS_PASSCODE key constant for migration; ensure you add a platform-expected interface/expect-actual or DI for an EncryptedSettings/PasscodeStore and migrate existing plain values (decrypt/rehash or prompt re-entry) if present.feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/ChooseAuthOptionScreenViewmodel.kt-23-25 (1)
23-25:⚠️ Potential issue | 🟠 Major
DISPLAY_NAME = "XYZ"is a placeholder used in biometric credential registration.If
client?.displayNameis null,"XYZ"is silently passed toplatformAuthenticationProvider.registerUser(...)as the credential display name. This will produce a meaningless (and potentially confusing) passkey entry in the user's credential manager.
USER_EMAIL = "mifos@mifos.org"has the same problem ifclient?.emailAddressis null — a shared static email will be registered as the credential user handle.These constants should either be derived defensively from a non-null user attribute, or
registerUsershould be guarded to require valid client data before proceeding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/ChooseAuthOptionScreenViewmodel.kt` around lines 23 - 25, ChooseAuthOptionScreenViewmodel currently uses hardcoded USER_EMAIL and DISPLAY_NAME placeholders which can pass meaningless values into platformAuthenticationProvider.registerUser when client?.emailAddress or client?.displayName are null; remove or stop using the constants and instead either (A) derive non-null safe values from the client (e.g., use client.emailAddress and client.displayName with sensible fallbacks like throwing/returning an error if absent) or (B) add a defensive guard in the method that calls platformAuthenticationProvider.registerUser to require client != null && client.emailAddress != null && client.displayName != null and abort/return a user-facing error if any are missing; reference the DISPLAY_NAME and USER_EMAIL constants and the call to platformAuthenticationProvider.registerUser in ChooseAuthOptionScreenViewmodel to locate where to replace the constants with these checks.feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/ChooseAuthOptionScreen.kt-100-106 (1)
100-106:⚠️ Potential issue | 🟠 MajorIncorrect
Modifierordering clips content under system bars.The
padding(16.dp)is applied beforefillMaxSize()and the scaffold's inset paddingit. The scaffold's inset padding must be consumed first, otherwise the column may be clipped under the status/nav bars. The extra content padding should come last.🐛 Proposed fix
- modifier = Modifier.padding(16.dp) - .fillMaxSize() - .padding(it), + modifier = Modifier + .fillMaxSize() + .padding(it) + .padding(16.dp),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/ChooseAuthOptionScreen.kt` around lines 100 - 106, The Column's Modifier applies padding(16.dp) before fillMaxSize() and padding(it), which causes scaffold insets to be ignored and content clipped; update the Modifier chain in ChooseAuthOptionScreen's Column so the scaffold inset padding (padding(it)) is applied first and the extra content padding (padding(16.dp)) is applied last — e.g., call .padding(it) before .fillMaxSize() and keep .padding(16.dp) at the end so the Column consumes system insets properly.
🟡 Minor comments (13)
feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/components/SystemAuthenticatorButton.kt-130-142 (1)
130-142:⚠️ Potential issue | 🟡 Minor"Windows Hello" label is incorrect for non-Windows JVM targets.
The JVM platform covers Windows, macOS, and Linux desktops. Labelling the authenticate button "Authenticate with Windows Hello" will be misleading on macOS (Touch ID / passkeys) and Linux (PAM-based or no biometrics). Consider using a platform-neutral label (e.g., "Authenticate with device") or, if the library supports it, detecting the OS at runtime within the JVM branch.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/components/SystemAuthenticatorButton.kt` around lines 130 - 142, The JVM branch currently uses the Windows-specific label; update the JVM branch in SystemAuthenticatorButton.kt to use a platform-neutral label or pick the correct label at runtime: either replace stringResource(Res.string.feature_authenticator_biometrics_authenticate_windows_hello) with a neutral key (e.g., feature_authenticator_biometrics_authenticate_device) and update Res.string accordingly, or detect the OS via JVM APIs (e.g., System.getProperty("os.name")) inside the Platform.JVM case and choose the appropriate string key for Windows (Windows Hello), macOS (Touch ID), or a generic fallback; ensure the change is applied where AuthenticateButton is invoked and that PlatformAuthenticatorStatus checks remain unchanged.feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/components/SystemAuthenticatorButton.kt-158-165 (1)
158-165:⚠️ Potential issue | 🟡 MinorUse the
shapeparameter instead of.clip()for Material3 Button styling.
Modifier.clip(RoundedCornerShape(...))is a graphics-layer pixel mask that clips content at that point in the modifier chain. However, Material3Buttonhas a dedicatedshapeparameter that is semantic to the component—it controls the button's container, background, border, elevation/shadow, and ripple behavior. Usingclip()bypasses these Material-specific behaviors and can cause unintended side effects like clipped shadows.♻️ Proposed fix
Button( onClick = onClick, modifier = Modifier .height(50.dp) - .width(250.dp) - .clip(RoundedCornerShape(50)), + .width(250.dp), + shape = RoundedCornerShape(50), ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/components/SystemAuthenticatorButton.kt` around lines 158 - 165, The Button currently uses Modifier.clip(RoundedCornerShape(50)) which incorrectly clips the composable; instead remove the .clip(...) call from the Modifier and pass the shape to the Button via its shape parameter (e.g., shape = RoundedCornerShape(50)), keeping the existing Modifier.height(50.dp).width(250.dp) and onClick/Text usage in SystemAuthenticatorButton so shadows, ripple and Material3 semantics remain correct.feature/auth/src/commonMain/composeResources/values/strings.xml-96-96 (1)
96-96:⚠️ Potential issue | 🟡 Minor
feature_auth_radiobuttonis not a meaningful accessibility label.The value
"Radiobutton"only describes the widget type, not its purpose. Screen readers (TalkBack/VoiceOver) will announce this literally, giving users no context about what the radio button selects. Replace it with a descriptive content description that reflects the action or option being represented (e.g.,"Select authentication method"), or remove it if it is only used as a debug placeholder.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/auth/src/commonMain/composeResources/values/strings.xml` at line 96, The string resource feature_auth_radiobutton is currently a non-descriptive accessibility label ("Radiobutton"); update the value of feature_auth_radiobutton to a meaningful content description that explains the option or action (e.g., "Select authentication method", "Sign in with email", or another context-appropriate label) or remove the string if it was only a debug placeholder; ensure the new text matches the actual radio button purpose wherever feature_auth_radiobutton is referenced so screen readers (TalkBack/VoiceOver) convey clear intent.feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/components/AuthOptionCard.kt-82-93 (1)
82-93:⚠️ Potential issue | 🟡 Minor
Arrangement.SpaceEvenlyproduces awkward layout whensubtitleis empty.With
subtitle = ""(the default),SpaceEvenlydistributes equal space around the lone icon and the emptyText, creating a visually centered icon rather than a left-aligned one. UseArrangement.Startto keep the icon and text flush-left.♻️ Proposed fix
Row( - horizontalArrangement = Arrangement.SpaceEvenly, + horizontalArrangement = Arrangement.Start, verticalAlignment = Alignment.CenterVertically, ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/components/AuthOptionCard.kt` around lines 82 - 93, The Row in AuthOptionCard uses Arrangement.SpaceEvenly which centers the lone Icon when subtitle == "", causing an awkward layout; update the Row's horizontalArrangement from Arrangement.SpaceEvenly to Arrangement.Start in the AuthOptionCard composable so the Icon and Text are left-aligned (optionally keep the Spacer and Text as-is so empty subtitle still occupies no extra visible space).feature/passcode/src/commonMain/kotlin/org/mifos/feature/passcode/MifosPasscode.kt-26-26 (1)
26-26:⚠️ Potential issue | 🟡 MinorUnused import
blueTintshould be removed.
org.mifos.authenticator.passcode.theme.blueTintis imported but never referenced in this file. This generates a compiler warning.🔧 Proposed fix
-import org.mifos.authenticator.passcode.theme.blueTint🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/passcode/src/commonMain/kotlin/org/mifos/feature/passcode/MifosPasscode.kt` at line 26, Remove the unused import org.mifos.authenticator.passcode.theme.blueTint from MifosPasscode.kt (the import referencing blueTint is not used anywhere in the MifosPasscode class/file); simply delete that import line to eliminate the compiler warning and keep imports minimal.feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/AuthenticationScreenViewModel.kt-75-82 (1)
75-82:⚠️ Potential issue | 🟡 MinorHardcoded error message string — not externalized for i18n.
Line 78 embeds a raw English string
"The user has changed authentication settings, register again."directly in the ViewModel. This should be a string resource key, or at minimum passed as a resource-resolved string from a layer that has access toContext/stringResource.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/AuthenticationScreenViewModel.kt` around lines 75 - 82, The ViewModel currently uses a hardcoded English message in the AuthenticationResult.UserNotRegistered branch; update AuthenticationScreenViewModel so the message is not embedded in code: replace the literal in the updateState call that constructs AuthenticationScreenState.ScreenState.UserNotRegistered with a resource-backed string passed into the ViewModel (or a key/enum), e.g., accept a localizedMessage parameter or a provider interface on the ViewModel constructor and use that to supply the resolved string, keeping the call shape to updateState and still invoking clearUserRegistrationFromApp() afterward; ensure the change touches the AuthenticationResult.UserNotRegistered handling and preserves existing function names (updateState, AuthenticationScreenState.ScreenState.UserNotRegistered, clearUserRegistrationFromApp) so callers can provide the proper localized text.cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayViewModel.kt-46-48 (1)
46-48:⚠️ Potential issue | 🟡 Minor
getAuthOption()is synchronous and reads from preferences — acceptable but consider reactivity.The implementation reads via
settings.getString(), which uses the synchronous Multiplatform Settings API. While preference reads are typically lightweight and cached, if this method is called frequently from the UI layer, consider exposingauthOptionas aStateFlow/Flowto align with reactive patterns (matchinguiState) and avoid repeated synchronous reads. Currently there is no Flow variant in the repository, so this would require creating one if deemed necessary.🤖 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/MifosPayViewModel.kt` around lines 46 - 48, getAuthOption() currently performs a synchronous preferences read via chooseAuthOptionRepository.getAuthOption() (which uses settings.getString()); change the API to a reactive one by adding a Flow/StateFlow variant (e.g., add authOptionFlow or getAuthOptionFlow in ChooseAuthOptionRepository) and have MifosPayViewModel expose that Flow (or a ui-facing StateFlow) instead of the synchronous getAuthOption(), or alternatively provide both sync and flow APIs so the UI can subscribe to updates; implement the Flow by wrapping the settings read with a flow { emit(...) } or using a settings-backed flow helper so consumers avoid repeated synchronous reads.feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/AuthenticationScreenViewModel.kt-87-90 (1)
87-90:⚠️ Potential issue | 🟡 MinorConsider using
Dispatchers.IOor making these methods suspend if data durability is critical.Both
clearBiometricRegistrationData()andremoveAuthOption()usecom.russhwolf.settings.Settings, which performs synchronous preference storage operations. While the Multiplatform Settings library is designed for synchronous use and these operations are typically very fast, if data durability is important, consider wrapping them withDispatchers.IOor converting to suspend functions to ensure writes complete before the coroutine ends, reducing the risk of data loss if the process is terminated unexpectedly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/AuthenticationScreenViewModel.kt` around lines 87 - 90, The synchronous preference calls in clearUserRegistrationFromApp() (platformAuthenticationDataRepository.clearBiometricRegistrationData() and chooseAuthOptionRepository.removeAuthOption()) should be made safe for durability: either convert those repository methods to suspend functions and call them with suspend from clearUserRegistrationFromApp(), or wrap the current synchronous calls in a coroutine using Dispatchers.IO to ensure writes complete before returning. Update clearUserRegistrationFromApp() signature and its callers if you choose suspend, or launch/withContext(Dispatchers.IO) around the two repository calls to perform them on the IO dispatcher and await completion.cmp-android/dependencies/prodReleaseRuntimeClasspath.txt-459-463 (1)
459-463:⚠️ Potential issue | 🟡 MinorAlign
kotlin-stdlib-jdk7:1.9.0andkotlin-stdlib-jdk8:1.8.22withkotlin-stdlib:2.3.0or exclude them.Since Kotlin 1.8,
kotlin-stdlib-jdk7andkotlin-stdlib-jdk8were merged into the mainkotlin-stdlib. Having these older transitive versions (1.8.22 and 1.9.0) alongsidekotlin-stdlib:2.3.0causes duplicate-class warnings and potential resolution conflicts. Add explicit dependency constraints to align all three to2.3.0, or use the Kotlin BOM and exclude the legacy variants.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmp-android/dependencies/prodReleaseRuntimeClasspath.txt` around lines 459 - 463, The project has mixed Kotlin stdlib versions (kotlin-stdlib:2.3.0 vs kotlin-stdlib-jdk7:1.9.0 and kotlin-stdlib-jdk8:1.8.22) causing duplicate-class/conflict warnings; update dependency resolution so all stdlib artifacts use 2.3.0 or remove the legacy artifacts: add dependency constraints or use the Kotlin BOM to force kotlin-stdlib-jdk7 and kotlin-stdlib-jdk8 to 2.3.0 (or exclude them where they are transitively pulled), targeting the artifact coordinates kotlin-stdlib, kotlin-stdlib-jdk7, and kotlin-stdlib-jdk8 in your Gradle/Maven resolution rules.cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/MifosNavHost.kt-186-188 (1)
186-188:⚠️ Potential issue | 🟡 MinorSet verification result on passcode creation path as well.
Line 186 only pops back. If the caller reads
PASSCODE_VERIFICATION_KEY, creation currently returns no explicit result and may reuse stale state.💡 Proposed fix
onPasscodeCreation = { + navController.previousBackStackEntry + ?.savedStateHandle + ?.set(PASSCODE_VERIFICATION_KEY, 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 186 - 188, The onPasscodeCreation handler currently only calls navController.popBackStack() and never sets PASSCODE_VERIFICATION_KEY, which can leave a stale verification result; update the onPasscodeCreation lambda in MifosNavHost (where navController.popBackStack() is called) to set the verification result on the previousBackStackEntry's savedStateHandle (e.g., previousBackStackEntry?.savedStateHandle?.set(PASSCODE_VERIFICATION_KEY, true) or the appropriate result value) before popping the back stack so callers receive an explicit creation result.feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/ChooseAuthOptionScreen.kt-178-191 (1)
178-191:⚠️ Potential issue | 🟡 MinorEmpty
confirmButtonTextrenders a blank button in the error dialog.
confirmButtonText = ""on Line 182 produces a visible but empty button. Additionally, itsonConfirmhandler callsSetupPlatformAuthenticator, which is the action for the not-setup dialog, not the generic error dialog — this looks like a copy-paste mistake.If no confirm action is needed for errors, pass
null(or an empty lambda with the button hidden) and correct theonConfirmcallback accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/ChooseAuthOptionScreen.kt` around lines 178 - 191, The error dialog block in ChooseAuthOptionScreen uses MifosDialogBox with confirmButtonText = "" which renders a blank button and its onConfirm wrongly dispatches ChooseAuthOptionScreenAction.SetupPlatformAuthenticator (a copy-paste error); change confirmButtonText to null (or remove the confirm button via the dialog API) and update the onConfirm handler to either null/no-op or to dispatch ChooseAuthOptionScreenAction.DismissDialogBox (or the appropriate error-dismiss action) so the dialog shows only a proper dismiss button and does not trigger platform-setup logic; locate this change around the MifosDialogBox invocation in ChooseAuthOptionScreenUiState.ScreenState.Error within ChooseAuthOptionScreen.kt.feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/ChooseAuthOptionScreenViewmodel.kt-115-128 (1)
115-128:⚠️ Potential issue | 🟡 MinorHardcoded error strings should be in string resources.
"Option Not available"and"Platform authenticator not set."are user-facing strings embedded in the ViewModel. Move them to the feature's string resource file to support i18n and consistent copy management.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/ChooseAuthOptionScreenViewmodel.kt` around lines 115 - 128, The ViewModel is using hardcoded user-facing strings for RegistrationResult.PlatformAuthenticatorNotAvailable and RegistrationResult.PlatformAuthenticatorNotSet; add these messages to the feature's string resources and replace the literals in ChooseAuthOptionScreenViewmodel (the mutableStateFlow.update calls) with resource lookups (via your existing string/resource provider or an injected Strings/Resources interface) so the ViewModel reads e.g. strings.get(SOME_KEY) or resources.platformAuthenticatorNotAvailable and resources.platformAuthenticatorNotSet instead of the raw literals; also add the two new keys to the feature's string resource file and wire them into the common/shared strings access used by the feature.feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/ChooseAuthOptionScreen.kt-109-129 (1)
109-129:⚠️ Potential issue | 🟡 MinorSemantically incorrect icons for both auth options.
- Line 113:
Icons.Default.Dialpad(phone keypad) is used for Device Lock — should beIcons.Default.LockorIcons.Default.Fingerprint.- Line 125:
Icons.Default.People(contacts/group) is used for Mifos Passcode — should beIcons.Default.PinorIcons.Default.Password.🎨 Proposed fix
- icon = Icons.Default.Dialpad, + icon = Icons.Default.Lock,- icon = Icons.Default.People, + icon = Icons.Default.Pin,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/ChooseAuthOptionScreen.kt` around lines 109 - 129, Update the icons passed to AuthOptionCard in ChooseAuthOptionScreen to use semantically appropriate icons: for the card where selected check uses AppLockOption.DeviceLock (currently Icons.Default.Dialpad) swap to a lock-related icon such as Icons.Default.Lock or Icons.Default.Fingerprint; for the card where selected check uses AppLockOption.MifosPasscode (currently Icons.Default.People) swap to a passcode-related icon such as Icons.Default.Pin or Icons.Default.Password so the icon prop for each AuthOptionCard matches its authentication option.
...monMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/ChooseAuthOptionScreenViewmodel.kt
Outdated
Show resolved
Hide resolved
TheKalpeshPawar
force-pushed
the
integrate-passcode-module
branch
from
fd64d46 to
64ad8f9
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
feature/auth/src/commonTest/kotlin/org/mifospay/feature/auth/login/LoginViewModelTest.kt (1)
199-232:⚠️ Potential issue | 🟡 MinorUpdate the stale test name and inline comment to match the new event.
The test method is still named
...thenNavigateToPasscodeScreenEmittedand the inline comment at Line 231 still says// Expect NavigateToPasscodeScreen after successful login, both of which now contradict the assertion on Line 232.✏️ Proposed fix
- fun givenCorrectCredentials_whenLoginSucceeds_thenNavigateToPasscodeScreenEmitted() = runTest { + fun givenCorrectCredentials_whenLoginSucceeds_thenNavigateToChooseAuthOptionScreenEmitted() = runTest {- // Expect NavigateToPasscodeScreen after successful login + // Expect NavigateToChooseAuthOptionScreen after successful login assertTrue(awaitItem() is LoginEvent.NavigateToChooseAuthOptionScreen)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/auth/src/commonTest/kotlin/org/mifospay/feature/auth/login/LoginViewModelTest.kt` around lines 199 - 232, Rename the test method givenCorrectCredentials_whenLoginSucceeds_thenNavigateToPasscodeScreenEmitted to reflect the new expected event (e.g., givenCorrectCredentials_whenLoginSucceeds_thenNavigateToChooseAuthOptionScreenEmitted) and update the inline comment that currently reads "// Expect NavigateToPasscodeScreen after successful login" to reference the new event (e.g., "// Expect NavigateToChooseAuthOptionScreen after successful login"), ensuring the test name and comment match the asserted LoginEvent.NavigateToChooseAuthOptionScreen in the viewModel.eventFlow.test block.cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayApp.kt (1)
54-54:⚠️ Potential issue | 🟡 MinorUse
valinstead ofvarfor theMutableStatereference.
showErrorDialogholds aMutableStateobject whose reference never changes — only.valueis mutated. Usingvaris misleading. Either usevalwith.valueaccess, or switch to thebydelegate:- var showErrorDialog = remember { mutableStateOf<Boolean>(false) } + val showErrorDialog = remember { mutableStateOf(false) }Or idiomatically:
- var showErrorDialog = remember { mutableStateOf<Boolean>(false) } + var showErrorDialog by remember { mutableStateOf(false) }(and replace
.valueaccesses with direct reads/writes).🤖 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/MifosPayApp.kt` at line 54, Change the mutable state reference showErrorDialog from a var to a val because the MutableState object returned by remember { mutableStateOf(...) } is immutable as a reference; update the declaration to use val showErrorDialog = remember { mutableStateOf(false) } and then either access/update it via showErrorDialog.value or switch to the by delegate (val showErrorDialog by remember { mutableStateOf(false) }) and update all .value usages accordingly so only the contained value is mutated.
♻️ Duplicate comments (1)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayApp.kt (1)
83-98:⚠️ Potential issue | 🔴 Critical
viewModel.logOut()is a side effect called during composition.This calls
logOut()inside thenavDestinationcomputation, which runs on every recomposition. This was already flagged in a prior review — the fix is to move the logout into aLaunchedEffectkeyed on the auth option, keepingnavDestinationpurely declarative.🤖 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/MifosPayApp.kt` around lines 83 - 98, The computation for navDestination currently performs a side-effect by calling viewModel.logOut() inside the when branch for AppLockOption.None; move that logout call out of the navDestination declaration and into a LaunchedEffect keyed on the auth option (use viewModel.getAuthOption() or the derived authOption value) so navDestination remains purely declarative; keep the when branches returning routes only (ROOT_MIFOS_PASSCODE_ROUTE, PLATFORM_AUTHENTICATOR_ROUTE, LOGIN_GRAPH) and trigger viewModel.logOut() in a LaunchedEffect when the auth option equals AppLockOption.None.
🧹 Nitpick comments (12)
feature/auth/src/commonTest/kotlin/org/mifospay/feature/auth/login/LoginViewModelTest.kt (1)
232-232: PreferassertIs<T>overassertTrue(x is T)for a more informative failure message.The analogous assertion on Line 191 already uses
assertIs<LoginEvent.NavigateToSignup>(awaitItem()), which prints the actual runtime type on failure.assertTrueonly outputs "Expected true but was false".♻️ Proposed refactor
- assertTrue(awaitItem() is LoginEvent.NavigateToChooseAuthOptionScreen) + assertIs<LoginEvent.NavigateToChooseAuthOptionScreen>(awaitItem())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/auth/src/commonTest/kotlin/org/mifospay/feature/auth/login/LoginViewModelTest.kt` at line 232, Replace the weak type-check assertion in LoginViewModelTest by using the more informative assertion helper: change the assertion that uses assertTrue(awaitItem() is LoginEvent.NavigateToChooseAuthOptionScreen) to use assertIs<LoginEvent.NavigateToChooseAuthOptionScreen>(awaitItem()) so failures report the actual runtime type; update the line that references awaitItem() and the LoginEvent.NavigateToChooseAuthOptionScreen type accordingly to match the existing style used earlier in the test.feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/components/SystemAuthenticatorButton.kt (3)
57-149: Extract repeated icon-rendering pattern to reduce duplication.The "clickable icon with a specific size" pattern is repeated ~7 times across platform branches. A small private helper composable would reduce duplication and make future changes (like the
IconButtonmigration above or size adjustments) easier.Example helper
`@Composable` private fun AuthIconButton( onClick: () -> Unit, drawableRes: DrawableResource, contentDescription: String, size: Dp = 50.dp, ) { IconButton(onClick = onClick) { Image( painter = painterResource(drawableRes), contentDescription = contentDescription, modifier = Modifier.size(size), ) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/components/SystemAuthenticatorButton.kt` around lines 57 - 149, There’s repeated code rendering clickable icons in SystemAuthenticatorButton.kt; extract a small private composable (e.g., AuthIconButton) and replace each Image { painterResource(...), contentDescription = ..., modifier = Modifier.size(...).clickable { onClick() } } with calls to that helper; keep the helper signature like AuthIconButton(onClick: () -> Unit, drawableRes: DrawableResource, contentDescription: String, size: Dp = 50.dp) so Platform.ANDROID, Platform.IOS and DEVICE_CREDENTIAL/BIOMETRICS branches can call it, and leave AuthenticateButton usages unchanged.
72-77: PreferIconButtonoverImage+clickablefor interactive icons.Bare
Imagewith.clickablelacks the semantic button role and the standard minimum touch-target enforcement thatIconButtonprovides. This degrades accessibility (screen readers won't announce these as actionable) and may fail minimum touch-target size guidelines on some platforms.Switching to
IconButton(or at minimum addingsemantics { role = Role.Button }) would resolve both concerns.Example refactor for one occurrence (line 72–77)
- Image( - painter = painterResource(Res.drawable.fingerprint), - contentDescription = stringResource(Res.string.feature_authenticator_biometrics_fingerprint_icon), - modifier = Modifier.size(50.dp) - .clickable { onClick() }, - ) + IconButton(onClick = onClick) { + Image( + painter = painterResource(Res.drawable.fingerprint), + contentDescription = stringResource(Res.string.feature_authenticator_biometrics_fingerprint_icon), + modifier = Modifier.size(50.dp), + ) + }Also applies to: 92-98, 100-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/components/SystemAuthenticatorButton.kt` around lines 72 - 77, Replace the interactive Image usages inside SystemAuthenticatorButton (the Image(...) with modifier.clickable { onClick() }) with Compose's IconButton to provide proper semantics and touch-target enforcement; specifically, wrap the fingerprint/toggle images used in SystemAuthenticatorButton with IconButton(onClick = onClick) and move Modifier.size(50.dp) (or the appropriate sizing) onto the IconButton or its content, or alternatively add Modifier.semantics { role = Role.Button } and ensure minimum touch target if you must keep Image; apply the same change to the other occurrences referenced (the Image uses around the biometric toggle/secondary icons) so all interactive icons use IconButton (or explicit semantics + hit target) instead of bare Image.clickable.
158-165: UseButton'sshapeparameter instead ofModifier.clip.
Modifier.cliponly clips the layout/drawing bounds but does not affect theButton's internal container shape or its ripple/indication. The idiomatic approach is to passshapedirectly toButton.Proposed fix
Button( onClick = onClick, + shape = RoundedCornerShape(50), modifier = Modifier.height(50.dp) - .width(250.dp) - .clip(RoundedCornerShape(50)), + .width(250.dp), ) { Text(text) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/components/SystemAuthenticatorButton.kt` around lines 158 - 165, Replace the use of Modifier.clip in SystemAuthenticatorButton with the Button's shape parameter: remove .clip(RoundedCornerShape(50)) from the Modifier passed into Button (keep Modifier.height(50.dp).width(250.dp)) and add shape = RoundedCornerShape(50) to the Button call so the internal container shape, ripple and indication are applied correctly.core/ui/build.gradle.kts (2)
44-44: Consider whether navigation belongs incore/ui.
core/uiis a shared primitive/component library; pulling injb.composeNavigationcouples it to the routing framework. If the usages are limited to lightweight utilities (e.g.,LocalNavigator, back-press handlers),implementationscope is fine. If graph-level orNavHostconstructs are involved, a dedicated:core:navigationmodule would keep the dependency direction clean.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/ui/build.gradle.kts` at line 44, The build declares implementation(libs.jb.composeNavigation) in core/ui which couples the shared UI primitives to the navigation framework; decide whether to keep it as an implementation dependency for only lightweight utilities (e.g., LocalNavigator/back-press helpers) or move heavier NavHost/graph usages into a new module (e.g., :core:navigation). If usages in core/ui are only small helpers, keep the dependency but ensure no NavHost or graph-level types are exposed from public APIs; otherwise remove libs.jb.composeNavigation from core/ui and create a new module (reference symbols: jb.composeNavigation, LocalNavigator, NavHost) where NavHost/graph code lives and update callers to depend on :core:navigation.
44-44: Consider whether navigation belongs incore/ui.
core/uiis a shared primitive/component library. Pulling injb.composeNavigationcouples the core UI layer to the routing framework. If the usages are limited to lightweight utilities (e.g.,LocalNavigator, back-press handlers),implementationscope contains the leakage. If graph-level orNavHostconstructs are involved, a dedicated:core:navigationmodule would keep the dependency direction cleaner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/ui/build.gradle.kts` at line 44, The build pulls in libs.jb.composeNavigation into core/ui which couples the primitive UI module to the routing framework; either remove this dependency from core/ui and move navigation usages into a new :core:navigation module (or the feature module) that depends on libs.jb.composeNavigation, or if only lightweight APIs like LocalNavigator or back-press handlers are used, change the dependency scope and usages to keep implementation details out of public/core APIs—identify references to libs.jb.composeNavigation, NavHost, LocalNavigator, and any graph-level constructs in the core/ui code, relocate those NavHost/graph usages to :core:navigation (or the consuming module), and keep only minimal interfaces or abstractions in core/ui so core/ui no longer declares implementation(libs.jb.composeNavigation).cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/PasscodeNavGraph.kt (1)
16-40: Remove the obsolete commented navigation block.Keeping the removed graph implementation as comments makes navigation ownership unclear and increases maintenance noise.
🧹 Suggested cleanup
-// internal fun NavGraphBuilder.passcodeNavGraph(navController: NavController) { -// navigation( -// route = MifosNavGraph.PASSCODE_GRAPH, -// startDestination = PASSCODE_SCREEN, -// ) { -// passcodeRoute( -// onForgotButton = { -// navController.popBackStack() -// navController.navigateToMainGraph() -// }, -// onSkipButton = { -// navController.popBackStack() -// navController.navigateToMainGraph() -// }, -// onPasscodeConfirm = { -// navController.popBackStack() -// navController.navigateToMainGraph() -// }, -// onPasscodeRejected = { -// navController.popBackStack() -// navController.navigateToMainGraph() -// }, -// ) -// } -// }🤖 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/PasscodeNavGraph.kt` around lines 16 - 40, Remove the obsolete commented navigation block in PasscodeNavGraph by deleting the entire commented-out internal fun NavGraphBuilder.passcodeNavGraph(...) section (the block containing navigation(route = MifosNavGraph.PASSCODE_GRAPH, startDestination = PASSCODE_SCREEN) and the passcodeRoute callbacks like onForgotButton/onSkipButton/onPasscodeConfirm/onPasscodeRejected). This cleans up dead code and avoids confusion about navigation ownership; ensure no other references to NavGraphBuilder.passcodeNavGraph remain in the file after removing the comment.feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/components/AuthOptionCard.kt (1)
59-66: Prefer content-driven height to avoid clipping with large text/localization.The fixed
130.dpheight can truncate content at higher font scales or longer translations.📐 Suggested layout adjustment
modifier = Modifier - .height(130.dp) .fillMaxWidth() + .heightIn(min = 130.dp) .clickable { onSelect.invoke() }, @@ Column( modifier = Modifier - .padding(16.dp).fillMaxSize(), + .padding(16.dp) + .fillMaxWidth(), horizontalAlignment = Alignment.Start, verticalArrangement = Arrangement.SpaceAround, ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/components/AuthOptionCard.kt` around lines 59 - 66, The AuthOptionCard component currently hardcodes a height via .height(130.dp) which can cause text clipping with large fonts or translations; replace the fixed height with a content-driven constraint (e.g., use .heightIn(min = ...) or .wrapContentHeight()/Modifier.defaultMinSize(minHeight = ...) on the Card's modifier) and ensure the Column uses .fillMaxSize() or appropriate padding so the card expands with its contents; update the modifier chain in AuthOptionCard to remove .height(130.dp) and apply a minHeight or wrapContent approach so the layout adapts to localization and accessibility settings.core/data/src/commonMain/kotlin/org/mifospay/core/data/repository/PlatformAuthenticationDataRepository.kt (1)
16-16: Return nullable registration data to model “not set” explicitly.A non-null
Stringreturn makes absence ambiguous and pushes sentinel checks into callers.🔧 Suggested API tweak
- fun getBiometricRegistrationData(): String + fun getBiometricRegistrationData(): String?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/commonMain/kotlin/org/mifospay/core/data/repository/PlatformAuthenticationDataRepository.kt` at line 16, Change the API of getBiometricRegistrationData to return a nullable String (String?) so absence of registration is modeled explicitly; update the PlatformAuthenticationDataRepository interface signature for getBiometricRegistrationData to String?, then update all implementations of PlatformAuthenticationDataRepository and any callers to handle a null result (e.g., treat null as "not set" instead of using sentinel values or empty strings) and adjust unit tests accordingly to expect null when no registration exists.feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/AuthenticationScreen.kt (2)
92-96: Remove commented-out code before merging.This block is dead code. If it's needed later, it can be recovered from version control.
-// LaunchedEffect(Unit) { -// if(authenticatorStatus.contains(PlatformAuthenticatorStatus.BIOMETRICS_SET)){ -// onAction(AuthenticationScreenAction.OnClickAuthenticate(platformAuthenticationProvider)) -// } -// }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/AuthenticationScreen.kt` around lines 92 - 96, Delete the dead commented-out LaunchedEffect block in AuthenticationScreen.kt (the lines that reference LaunchedEffect(Unit), authenticatorStatus.contains(PlatformAuthenticatorStatus.BIOMETRICS_SET), and onAction(AuthenticationScreenAction.OnClickAuthenticate(platformAuthenticationProvider))); remove the entire commented section so only active code remains (it can be recovered from VCS if needed).
62-62: Unnecessary@OptIn(ExperimentalMaterial3Api::class).No experimental Material3 API appears to be used in
AuthenticationContent. Remove the opt-in to avoid misleading readers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/AuthenticationScreen.kt` at line 62, Remove the unnecessary `@OptIn`(ExperimentalMaterial3Api::class) annotation on the AuthenticationScreen/AuthenticationContent declaration: open AuthenticationScreen.kt, delete the `@OptIn`(ExperimentalMaterial3Api::class) line sitting above the AuthenticationContent (or surrounding composable) since no Experimental Material3 APIs are used; if a future experimental API is actually required, opt-in narrowly on the specific function or expression instead of the whole file/composable.feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/AuthenticationScreenNavigation.kt (1)
26-31: Consider using named arguments for theAuthenticationScreencall.Positional arguments for lambda callbacks can be error-prone if the parameter order changes. Named arguments improve clarity.
composable(route = PLATFORM_AUTHENTICATOR_ROUTE) { AuthenticationScreen( - onAuthenticationSuccess, - onForcedLogOut, + onAuthenticationSuccess = onAuthenticationSuccess, + onForcedLogOut = onForcedLogOut, ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/AuthenticationScreenNavigation.kt` around lines 26 - 31, The call to AuthenticationScreen inside the composable for PLATFORM_AUTHENTICATOR_ROUTE uses positional lambda arguments which can be error-prone; update the invocation of AuthenticationScreen to pass the callbacks as named parameters (e.g., onAuthenticationSuccess = ..., onForcedLogOut = ...) so the intent is explicit and resilient to parameter reordering in the AuthenticationScreen function signature.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/RootNavGraph.kt`:
- Around line 63-68: The mifosRootPasscodeScreen is leaving itself on the back
stack because it calls navHostController::navigateToMainGraph directly; change
its success handlers to first pop the passcode screen then navigate (mirror
platformAuthenticator: call navHostController.popBackStack() before
navHostController.navigateToMainGraph()). Also replace the silent
onPasscodeRejected = {} with a recovery action (e.g., call onForgotButton or
show a retry flow) so the user isn’t stuck; update the mifosRootPasscodeScreen
invocation to use a non-empty onPasscodeRejected handler that either pops to a
safe screen or triggers the forgot-passcode flow.
In
`@core/data/src/commonMain/kotlin/org/mifospay/core/data/repositoryImpl/MifosPasscodeAdapterImpl.kt`:
- Around line 22-29: The savePasscode/loadPasscode methods in
MifosPasscodeAdapterImpl currently persist MIFOS_PASSCODE into plain Settings;
replace this with an encrypted storage layer: update MifosPasscodeAdapterImpl to
encrypt the passcode before calling settings.putString(MIFOS_PASSCODE, ...) and
decrypt the value returned from settings.getString(MIFOS_PASSCODE, ...), or
better inject an EncryptedSettings abstraction via DI (instead of raw Settings
from PreferenceModule) that uses platform-specific secure storage (Android
Keystore / EncryptedSharedPreferences on Android, Keychain on iOS) to perform
encryption/decryption; ensure the DI binding for Settings/EncryptedSettings in
PreferenceModule is changed to provide the secure implementation so
savePasscode, loadPasscode and MIFOS_PASSCODE use encrypted persistence.
In `@feature/auth/src/commonMain/composeResources/values/strings.xml`:
- Around line 91-96: Update the user-facing string values for clarity and
accessibility: change the value of feature_auth_setup_authentication_options to
"Set up authentication options", change feature_auth_ok to "OK", and change
feature_auth_radiobutton to "Radio button"; verify these updated strings are
used for any accessibility labels (e.g., talkback/VoiceOver) and localized
resources so the new wording is consistent across the app.
In
`@feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/ChooseAuthOptionScreen.kt`:
- Around line 113-125: The icons for the auth options are semantically
mismatched; update the AuthOptionCard usages so the device-lock option uses a
lock-style icon and the passcode option uses a dialpad-style icon: in the block
that invokes onAction(ChooseAuthOptionScreenAction.OnSelectDeviceLock) replace
Icons.Default.Dialpad with Icons.Default.Lock (or Icons.Default.Fingerprint if
fingerprint semantics are intended), and in the AuthOptionCard for
AppLockOption.MifosPasscode replace Icons.Default.People with
Icons.Default.Dialpad to reflect numeric passcode input.
- Around line 179-187: The dialog currently sets confirmButtonText = "" in the
MifosDialogBox inside ChooseAuthOptionScreen, leaving the confirm button
unlabeled; replace the empty string with a meaningful localized label (e.g., use
stringResource(Res.string.feature_auth_retry) or an appropriate existing string)
and ensure the onConfirm handler
(ChooseAuthOptionScreenAction.SetupPlatformAuthenticator(platformAuthenticationProvider))
remains unchanged; if the chosen resource key doesn't exist, add a new
Res.string entry for the label and use stringResource to provide an accessible,
readable confirm button text.
In
`@feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/AuthenticationScreen.kt`:
- Around line 116-128: The MifosDialogBox in AuthenticationScreen currently
passes confirmButtonText = "" which renders an invisible but tappable confirm
button; change that to a meaningful label (e.g., confirmButtonText =
stringResource(Res.string.feature_authenticator_biometrics_ok)) and keep
onConfirm calling AuthenticationScreenAction.OnDismissDialog (or alternatively
remove/disable the confirm button via the MifosDialogBox API if available) so
only an explicit, visible button is presented instead of an empty tappable area.
- Around line 73-77: The LaunchedEffect in AuthenticationScreen is keyed to Unit
so it only runs once and can miss late updates to authenticatorStatus; change
the effect to key on the changing status (e.g.,
LaunchedEffect(authenticatorStatus) or the collectAsState() result) so the block
re-runs whenever authenticatorStatus updates and still calls
onAction(AuthenticationScreenAction.AuthenticatorStatusNotSetup) when
contains(PlatformAuthenticatorStatus.NOT_SETUP).
In
`@feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/components/SystemAuthenticatorButton.kt`:
- Around line 111-128: In the Platform.IOS branch of SystemAuthenticatorButton,
the code currently renders the face_scan icon whenever
PlatformAuthenticatorStatus.BIOMETRICS_SET; update it to inspect
platformAuthOptions (same approach as the Android branch) and select the correct
image and contentDescription based on available options (e.g., use face_scan,
fingerprint, iris resources matching the PlatformAuthenticator enum values),
falling back to a sensible default if none match, and keep the clickable
Modifier and sizing logic unchanged.
- Around line 78-98: Update the contentDescription for each non-fingerprint
Image in SystemAuthenticatorButton (the branches checking
platformAuthOptions.contains(PlatformAuthOptions.FaceId), .Iris, and
.UserCredential) to use distinct string resources instead of
feature_authenticator_biometrics_fingerprint_icon; add new strings (e.g.
feature_authenticator_biometrics_face_scan_icon, _iris_icon, _keypad_icon) to
the localization resources and reference them via stringResource(...) in the
corresponding Image calls so screen readers announce the correct icon for
FaceId, Iris and UserCredential.
- Around line 58-99: The Android branch in SystemAuthenticatorButton has a
compound condition that includes PlatformAuthOptions.Iris together with the
(FaceId && Fingerprint) check, making the later else-if for Iris unreachable;
edit the compound guard inside the Platform.ANDROID block (the if that currently
checks platformAuthOptions.contains(PlatformAuthOptions.Iris) ||
(platformAuthOptions.contains(PlatformAuthOptions.FaceId) &&
platformAuthOptions.contains(PlatformAuthOptions.Fingerprint))) to remove the
Iris check so it only checks the FaceId+Fingerprint combination, allowing the
later else-if branch that renders the eye_scanner Image (the Iris branch) to be
reached; keep the AuthenticateButton and other Image branches unchanged.
---
Outside diff comments:
In `@cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayApp.kt`:
- Line 54: Change the mutable state reference showErrorDialog from a var to a
val because the MutableState object returned by remember { mutableStateOf(...) }
is immutable as a reference; update the declaration to use val showErrorDialog =
remember { mutableStateOf(false) } and then either access/update it via
showErrorDialog.value or switch to the by delegate (val showErrorDialog by
remember { mutableStateOf(false) }) and update all .value usages accordingly so
only the contained value is mutated.
In
`@feature/auth/src/commonTest/kotlin/org/mifospay/feature/auth/login/LoginViewModelTest.kt`:
- Around line 199-232: Rename the test method
givenCorrectCredentials_whenLoginSucceeds_thenNavigateToPasscodeScreenEmitted to
reflect the new expected event (e.g.,
givenCorrectCredentials_whenLoginSucceeds_thenNavigateToChooseAuthOptionScreenEmitted)
and update the inline comment that currently reads "// Expect
NavigateToPasscodeScreen after successful login" to reference the new event
(e.g., "// Expect NavigateToChooseAuthOptionScreen after successful login"),
ensuring the test name and comment match the asserted
LoginEvent.NavigateToChooseAuthOptionScreen in the viewModel.eventFlow.test
block.
---
Duplicate comments:
In `@cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayApp.kt`:
- Around line 83-98: The computation for navDestination currently performs a
side-effect by calling viewModel.logOut() inside the when branch for
AppLockOption.None; move that logout call out of the navDestination declaration
and into a LaunchedEffect keyed on the auth option (use
viewModel.getAuthOption() or the derived authOption value) so navDestination
remains purely declarative; keep the when branches returning routes only
(ROOT_MIFOS_PASSCODE_ROUTE, PLATFORM_AUTHENTICATOR_ROUTE, LOGIN_GRAPH) and
trigger viewModel.logOut() in a LaunchedEffect when the auth option equals
AppLockOption.None.
---
Nitpick comments:
In
`@cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/PasscodeNavGraph.kt`:
- Around line 16-40: Remove the obsolete commented navigation block in
PasscodeNavGraph by deleting the entire commented-out internal fun
NavGraphBuilder.passcodeNavGraph(...) section (the block containing
navigation(route = MifosNavGraph.PASSCODE_GRAPH, startDestination =
PASSCODE_SCREEN) and the passcodeRoute callbacks like
onForgotButton/onSkipButton/onPasscodeConfirm/onPasscodeRejected). This cleans
up dead code and avoids confusion about navigation ownership; ensure no other
references to NavGraphBuilder.passcodeNavGraph remain in the file after removing
the comment.
In
`@core/data/src/commonMain/kotlin/org/mifospay/core/data/repository/PlatformAuthenticationDataRepository.kt`:
- Line 16: Change the API of getBiometricRegistrationData to return a nullable
String (String?) so absence of registration is modeled explicitly; update the
PlatformAuthenticationDataRepository interface signature for
getBiometricRegistrationData to String?, then update all implementations of
PlatformAuthenticationDataRepository and any callers to handle a null result
(e.g., treat null as "not set" instead of using sentinel values or empty
strings) and adjust unit tests accordingly to expect null when no registration
exists.
In `@core/ui/build.gradle.kts`:
- Line 44: The build declares implementation(libs.jb.composeNavigation) in
core/ui which couples the shared UI primitives to the navigation framework;
decide whether to keep it as an implementation dependency for only lightweight
utilities (e.g., LocalNavigator/back-press helpers) or move heavier
NavHost/graph usages into a new module (e.g., :core:navigation). If usages in
core/ui are only small helpers, keep the dependency but ensure no NavHost or
graph-level types are exposed from public APIs; otherwise remove
libs.jb.composeNavigation from core/ui and create a new module (reference
symbols: jb.composeNavigation, LocalNavigator, NavHost) where NavHost/graph code
lives and update callers to depend on :core:navigation.
- Line 44: The build pulls in libs.jb.composeNavigation into core/ui which
couples the primitive UI module to the routing framework; either remove this
dependency from core/ui and move navigation usages into a new :core:navigation
module (or the feature module) that depends on libs.jb.composeNavigation, or if
only lightweight APIs like LocalNavigator or back-press handlers are used,
change the dependency scope and usages to keep implementation details out of
public/core APIs—identify references to libs.jb.composeNavigation, NavHost,
LocalNavigator, and any graph-level constructs in the core/ui code, relocate
those NavHost/graph usages to :core:navigation (or the consuming module), and
keep only minimal interfaces or abstractions in core/ui so core/ui no longer
declares implementation(libs.jb.composeNavigation).
In
`@feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/components/AuthOptionCard.kt`:
- Around line 59-66: The AuthOptionCard component currently hardcodes a height
via .height(130.dp) which can cause text clipping with large fonts or
translations; replace the fixed height with a content-driven constraint (e.g.,
use .heightIn(min = ...) or
.wrapContentHeight()/Modifier.defaultMinSize(minHeight = ...) on the Card's
modifier) and ensure the Column uses .fillMaxSize() or appropriate padding so
the card expands with its contents; update the modifier chain in AuthOptionCard
to remove .height(130.dp) and apply a minHeight or wrapContent approach so the
layout adapts to localization and accessibility settings.
In
`@feature/auth/src/commonTest/kotlin/org/mifospay/feature/auth/login/LoginViewModelTest.kt`:
- Line 232: Replace the weak type-check assertion in LoginViewModelTest by using
the more informative assertion helper: change the assertion that uses
assertTrue(awaitItem() is LoginEvent.NavigateToChooseAuthOptionScreen) to use
assertIs<LoginEvent.NavigateToChooseAuthOptionScreen>(awaitItem()) so failures
report the actual runtime type; update the line that references awaitItem() and
the LoginEvent.NavigateToChooseAuthOptionScreen type accordingly to match the
existing style used earlier in the test.
In
`@feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/AuthenticationScreen.kt`:
- Around line 92-96: Delete the dead commented-out LaunchedEffect block in
AuthenticationScreen.kt (the lines that reference LaunchedEffect(Unit),
authenticatorStatus.contains(PlatformAuthenticatorStatus.BIOMETRICS_SET), and
onAction(AuthenticationScreenAction.OnClickAuthenticate(platformAuthenticationProvider)));
remove the entire commented section so only active code remains (it can be
recovered from VCS if needed).
- Line 62: Remove the unnecessary `@OptIn`(ExperimentalMaterial3Api::class)
annotation on the AuthenticationScreen/AuthenticationContent declaration: open
AuthenticationScreen.kt, delete the `@OptIn`(ExperimentalMaterial3Api::class) line
sitting above the AuthenticationContent (or surrounding composable) since no
Experimental Material3 APIs are used; if a future experimental API is actually
required, opt-in narrowly on the specific function or expression instead of the
whole file/composable.
In
`@feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/AuthenticationScreenNavigation.kt`:
- Around line 26-31: The call to AuthenticationScreen inside the composable for
PLATFORM_AUTHENTICATOR_ROUTE uses positional lambda arguments which can be
error-prone; update the invocation of AuthenticationScreen to pass the callbacks
as named parameters (e.g., onAuthenticationSuccess = ..., onForcedLogOut = ...)
so the intent is explicit and resilient to parameter reordering in the
AuthenticationScreen function signature.
In
`@feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/components/SystemAuthenticatorButton.kt`:
- Around line 57-149: There’s repeated code rendering clickable icons in
SystemAuthenticatorButton.kt; extract a small private composable (e.g.,
AuthIconButton) and replace each Image { painterResource(...),
contentDescription = ..., modifier = Modifier.size(...).clickable { onClick() }
} with calls to that helper; keep the helper signature like
AuthIconButton(onClick: () -> Unit, drawableRes: DrawableResource,
contentDescription: String, size: Dp = 50.dp) so Platform.ANDROID, Platform.IOS
and DEVICE_CREDENTIAL/BIOMETRICS branches can call it, and leave
AuthenticateButton usages unchanged.
- Around line 72-77: Replace the interactive Image usages inside
SystemAuthenticatorButton (the Image(...) with modifier.clickable { onClick() })
with Compose's IconButton to provide proper semantics and touch-target
enforcement; specifically, wrap the fingerprint/toggle images used in
SystemAuthenticatorButton with IconButton(onClick = onClick) and move
Modifier.size(50.dp) (or the appropriate sizing) onto the IconButton or its
content, or alternatively add Modifier.semantics { role = Role.Button } and
ensure minimum touch target if you must keep Image; apply the same change to the
other occurrences referenced (the Image uses around the biometric
toggle/secondary icons) so all interactive icons use IconButton (or explicit
semantics + hit target) instead of bare Image.clickable.
- Around line 158-165: Replace the use of Modifier.clip in
SystemAuthenticatorButton with the Button's shape parameter: remove
.clip(RoundedCornerShape(50)) from the Modifier passed into Button (keep
Modifier.height(50.dp).width(250.dp)) and add shape = RoundedCornerShape(50) to
the Button call so the internal container shape, ripple and indication are
applied correctly.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
feature/authenticator-biometrics/src/commonMain/composeResources/drawable/eye_scanner.jpgis excluded by!**/*.jpgfeature/authenticator-biometrics/src/commonMain/composeResources/drawable/face_scan.jpgis excluded by!**/*.jpgfeature/authenticator-biometrics/src/commonMain/composeResources/drawable/fingerprint.jpgis excluded by!**/*.jpgfeature/authenticator-biometrics/src/commonMain/composeResources/drawable/keypad.jpgis excluded by!**/*.jpg
📒 Files selected for processing (53)
build-logic/convention/src/main/kotlin/CMPFeatureConventionPlugin.ktcmp-android/dependencies/prodReleaseRuntimeClasspath.tree.txtcmp-android/dependencies/prodReleaseRuntimeClasspath.txtcmp-android/src/main/kotlin/org/mifospay/MainActivity.ktcmp-shared/build.gradle.ktscmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayApp.ktcmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayViewModel.ktcmp-shared/src/commonMain/kotlin/org/mifospay/shared/di/KoinModules.ktcmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/LoginNavGraph.ktcmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/MifosNavGraph.ktcmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/MifosNavHost.ktcmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/PasscodeNavGraph.ktcmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/RootNavGraph.ktcore/data/build.gradle.ktscore/data/src/commonMain/kotlin/org/mifospay/core/data/di/RepositoryModule.ktcore/data/src/commonMain/kotlin/org/mifospay/core/data/repository/AuthenticationRepository.ktcore/data/src/commonMain/kotlin/org/mifospay/core/data/repository/ChooseAuthOptionRepository.ktcore/data/src/commonMain/kotlin/org/mifospay/core/data/repository/ClientRepository.ktcore/data/src/commonMain/kotlin/org/mifospay/core/data/repository/PlatformAuthenticationDataRepository.ktcore/data/src/commonMain/kotlin/org/mifospay/core/data/repositoryImpl/ChooseAuthOptionRepositoryImpl.ktcore/data/src/commonMain/kotlin/org/mifospay/core/data/repositoryImpl/MifosPasscodeAdapterImpl.ktcore/data/src/commonMain/kotlin/org/mifospay/core/data/repositoryImpl/PlatformAuthenticationDataRepositoryImpl.ktcore/data/src/commonMain/kotlin/org/mifospay/core/data/util/AppLockOption.ktcore/data/src/commonMain/kotlin/org/mifospay/core/data/util/Helpers.ktcore/datastore/src/commonMain/kotlin/org/mifospay/core/datastore/UserPreferencesRepository.ktcore/ui/build.gradle.ktsfeature/auth/build.gradle.ktsfeature/auth/src/commonMain/composeResources/values/strings.xmlfeature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/ChooseAuthOptionScreen.ktfeature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/ChooseAuthOptionScreenNavigation.ktfeature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/ChooseAuthOptionScreenViewmodel.ktfeature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/components/AuthOptionCard.ktfeature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/di/AuthModule.ktfeature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/login/LoginScreen.ktfeature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/login/LoginViewModel.ktfeature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/navigation/LoginScreenNavigation.ktfeature/auth/src/commonTest/kotlin/org/mifospay/feature/auth/login/LoginViewModelTest.ktfeature/authenticator-biometrics/build.gradle.ktsfeature/authenticator-biometrics/src/commonMain/composeResources/values/strings.xmlfeature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/AuthenticationScreen.ktfeature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/AuthenticationScreenNavigation.ktfeature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/AuthenticationScreenViewModel.ktfeature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/AuthenticatorBiometricsModule.ktfeature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/components/SystemAuthenticatorButton.ktfeature/passcode/build.gradle.ktsfeature/passcode/src/commonMain/compseResources/values/strings.xmlfeature/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/SettingsScreen.ktfeature/settings/src/commonMain/kotlin/org/mifospay/feature/settings/SettingsViewModel.ktgradle/libs.versions.tomllibs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/PasscodeKeys.ktsettings.gradle.kts
💤 Files with no reviewable changes (1)
- cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/MifosNavGraph.kt
✅ Files skipped from review due to trivial changes (2)
- core/datastore/src/commonMain/kotlin/org/mifospay/core/datastore/UserPreferencesRepository.kt
- core/data/src/commonMain/kotlin/org/mifospay/core/data/repository/AuthenticationRepository.kt
🚧 Files skipped from review as they are similar to previous changes (20)
- settings.gradle.kts
- build-logic/convention/src/main/kotlin/CMPFeatureConventionPlugin.kt
- core/data/src/commonMain/kotlin/org/mifospay/core/data/repository/ChooseAuthOptionRepository.kt
- feature/passcode/src/commonMain/kotlin/org/mifos/feature/passcode/MifosPasscodeNavigation.kt
- core/data/src/commonMain/kotlin/org/mifospay/core/data/util/Helpers.kt
- feature/passcode/src/commonMain/kotlin/org/mifos/feature/passcode/MifosPasscode.kt
- feature/settings/src/commonMain/kotlin/org/mifospay/feature/settings/SettingsViewModel.kt
- feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/AuthenticatorBiometricsModule.kt
- gradle/libs.versions.toml
- feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/ChooseAuthOptionScreenNavigation.kt
- feature/auth/build.gradle.kts
- feature/passcode/src/commonMain/compseResources/values/strings.xml
- feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/AuthenticationScreenViewModel.kt
- feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/ChooseAuthOptionScreenViewmodel.kt
- cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayViewModel.kt
- core/data/src/commonMain/kotlin/org/mifospay/core/data/repositoryImpl/PlatformAuthenticationDataRepositoryImpl.kt
- core/data/build.gradle.kts
- feature/authenticator-biometrics/src/commonMain/composeResources/values/strings.xml
- cmp-shared/src/commonMain/kotlin/org/mifospay/shared/di/KoinModules.kt
- feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/login/LoginScreen.kt
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/RootNavGraph.kt
Outdated
Show resolved
Hide resolved
...data/src/commonMain/kotlin/org/mifospay/core/data/repositoryImpl/MifosPasscodeAdapterImpl.kt
Show resolved
Hide resolved
feature/auth/src/commonMain/composeResources/values/strings.xml
Outdated
Show resolved
Hide resolved
...h/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/ChooseAuthOptionScreen.kt
Outdated
Show resolved
Hide resolved
...h/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/ChooseAuthOptionScreen.kt
Outdated
Show resolved
Hide resolved
.../src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/AuthenticationScreen.kt
Outdated
Show resolved
Hide resolved
.../src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/AuthenticationScreen.kt
Outdated
Show resolved
Hide resolved
...kotlin/org/mifospay/feature/authenticator/biometrics/components/SystemAuthenticatorButton.kt
Outdated
Show resolved
Hide resolved
...kotlin/org/mifospay/feature/authenticator/biometrics/components/SystemAuthenticatorButton.kt
Outdated
Show resolved
Hide resolved
...kotlin/org/mifospay/feature/authenticator/biometrics/components/SystemAuthenticatorButton.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
core/data/build.gradle.kts (1)
33-33:⚠️ Potential issue | 🔴 CriticalRevert
core.modeltoapi(or explicitly wire all consumers) to prevent downstream compile breaks.Line 33 reintroduces the same transitive API break previously reported:
core:dataexposescore.modeltypes in its public surface, so downgrading toimplementationcan break dependents.Suggested fix
- implementation(projects.core.model) + api(projects.core.model)#!/bin/bash # Verify whether core:data exposes core.model in public API and # whether modules depending on core:data also declare core.model. echo "== core:data files importing core.model ==" rg -n --type=kt 'import\s+org\.mifospay\.core\.model\.' core/data/src/commonMain -C2 echo echo "== Modules depending on core:data but missing core.model ==" rg -n --glob '*.gradle.kts' 'projects\.core\.data' | cut -d: -f1 | sort -u | while read -r f; do if ! rg -q 'projects\.core\.model' "$f"; then echo "missing core.model in $f" fi done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/build.gradle.kts` at line 33, The build declaration implementation(projects.core.model) in core:data reintroduced a transitive API break because core:data exposes core.model types; change that dependency back to api(projects.core.model) in the core/data build script (or alternatively ensure every module that depends on core:data also directly declares projects.core.model) so downstream consumers keep access to core.model types; update the dependency line referencing implementation(projects.core.model) to api(projects.core.model) (or add explicit projects.core.model to the consumer modules' dependencies).feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/ChooseAuthOptionScreen.kt (1)
113-129: Icons are semantically mismatched —Dialpadfor Device Lock andPeoplefor Passcode.The device-lock card should use a lock/fingerprint icon and the passcode card should use the dialpad (numeric input). The past review flagged this exact issue and it remains unaddressed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/ChooseAuthOptionScreen.kt` around lines 113 - 129, The icons for the two AuthOptionCard entries are swapped: replace the icon used in the Device Lock card (where onSelect calls ChooseAuthOptionScreenAction.OnSelectDeviceLock) from Icons.Default.Dialpad to a lock/fingerprint icon (e.g., Icons.Default.Lock or Icons.Default.Fingerprint), and replace the icon used in the Passcode card (where selected checks AppLockOption.MifosPasscode and onSelect calls ChooseAuthOptionScreenAction.OnSelectPasscode) from Icons.Default.People to Icons.Default.Dialpad so device lock shows a lock/fingerprint and passcode shows a dialpad.
🧹 Nitpick comments (5)
feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/AuthenticationScreen.kt (1)
130-130: Prefernullover empty string for hidden dismiss button intent.On Line 130,
dismissButtonText = ""works, butnullis clearer and matches the component default behavior.♻️ Suggested small cleanup
- dismissButtonText = "", + dismissButtonText = null,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/AuthenticationScreen.kt` at line 130, The dismiss button is being hidden by setting dismissButtonText = "" in AuthenticationScreen (file: AuthenticationScreen.kt); change this to use null instead of an empty string so it matches the component's default/hiding behavior (replace dismissButtonText = "" with dismissButtonText = null in the AuthenticationScreen composable invocation).feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/ChooseAuthOptionScreen.kt (2)
197-204:navigationHelperuses twoifstatements — preferwhen.Two independent
ifstatements are harmless here (theNonecase is unreachable because the button is disabled), but awhenexpression is more idiomatic and explicitly exhaustive.♻️ Proposed refactor
private fun navigationHelper( option: AppLockOption, whenDeviceLockSelected: () -> Unit, whenPasscodeSelected: () -> Unit, ) { - if (option == AppLockOption.DeviceLock) whenDeviceLockSelected() - if (option == AppLockOption.MifosPasscode) whenPasscodeSelected() + when (option) { + AppLockOption.DeviceLock -> whenDeviceLockSelected() + AppLockOption.MifosPasscode -> whenPasscodeSelected() + else -> Unit + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/ChooseAuthOptionScreen.kt` around lines 197 - 204, Replace the two independent ifs in navigationHelper with an idiomatic when expression over the AppLockOption parameter: match AppLockOption.DeviceLock to invoke whenDeviceLockSelected(), AppLockOption.MifosPasscode to invoke whenPasscodeSelected(), and include an else branch (no-op) to make the expression exhaustive; update the navigationHelper function body accordingly.
100-106: Scaffold content padding is applied in an unusual order — consider reversing.
Modifier.padding(16.dp).fillMaxSize().padding(it)puts the fixed 16 dp padding beforefillMaxSize, which shrinks the layout's measured bounds, then applies the scaffold inset padding last (innermost). The conventional pattern isfillMaxSize().padding(it).padding(16.dp)so the scaffold insets are consumed first and the extra margin is added inside the safe area.♻️ Proposed fix
- modifier = Modifier.padding(16.dp) - .fillMaxSize() - .padding(it), + modifier = Modifier + .fillMaxSize() + .padding(it) + .padding(16.dp),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/ChooseAuthOptionScreen.kt` around lines 100 - 106, The Column's Modifier applies fixed padding before fillMaxSize and scaffold insets after, which causes the fixed 16.dp to reduce the measured bounds; update the Modifier chain used in ChooseAuthOptionScreen's Column so fillMaxSize() is called first and the scaffold content padding (the variable `it`) is applied before the fixed 16.dp (i.e., use fillMaxSize() then padding(it) then padding(16.dp)) so scaffold insets are consumed first and the extra margin is inside the safe area.feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/ChooseAuthOptionScreenViewmodel.kt (1)
100-135:registrationResultis set unconditionally but only cleared on the success path — error states leave stale data.Line 102 stores
registrationResultfor all outcomes. Lines 107–127 (error branches) updatescreenStatebut never callsetRegistrationResultNull(), so after any failure the oldRegistrationResultlingers inChooseAuthOptionScreenUiState. If the user retries and the new attempt is also an error, the state is consistent, but if they dismiss and return, the stale result may cause unexpected UI behaviour. Clear it in all branches, or better, eliminate the intermediate state write entirely.♻️ Proposed fix
- mutableStateFlow.update { - it.copy( - registrationResult = registrationResult, - ) - } - when (registrationResult) { is RegistrationResult.Error -> { mutableStateFlow.update { it.copy( + registrationResult = registrationResult, screenState = ChooseAuthOptionScreenUiState.ScreenState.Error(registrationResult.message), ) } } RegistrationResult.PlatformAuthenticatorNotAvailable -> { mutableStateFlow.update { it.copy( + registrationResult = registrationResult, screenState = ChooseAuthOptionScreenUiState.ScreenState.Error("Option not available"), ) } } RegistrationResult.PlatformAuthenticatorNotSet -> { mutableStateFlow.update { it.copy( + registrationResult = registrationResult, screenState = ChooseAuthOptionScreenUiState.ScreenState.AuthenticatorNotSetup, ) } } is RegistrationResult.Success -> { mutableStateFlow.update { - it.copy(screenState = null) + it.copy(screenState = null, registrationResult = null) } saveAppLockOption(AppLockOption.DeviceLock) saveRegistrationData(registrationResult.message) sendEvent(ChooseAuthOptionScreenEvents.BiometricRegistrationSuccess) - setRegistrationResultNull() } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/ChooseAuthOptionScreenViewmodel.kt` around lines 100 - 135, The code writes registrationResult into state unconditionally via mutableStateFlow.update then only clears it on success; fix by removing the initial unconditional write or ensuring setRegistrationResultNull() is called for all non-success branches: update the when(registrationResult) branches in ChooseAuthOptionScreenViewmodel (the mutableStateFlow.update block, the RegistrationResult.Error, PlatformAuthenticatorNotAvailable, and PlatformAuthenticatorNotSet branches) so that after updating screenState they also clear registrationResult (call setRegistrationResultNull()) or avoid setting registrationResult into the state until a success is confirmed (i.e., only call mutableStateFlow.update { it.copy(registrationResult = registrationResult) } on is RegistrationResult.Success).cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayApp.kt (1)
83-98: Remove the stale commented implementation block.Keeping the old
navDestinationlogic commented out makes this flow harder to maintain and can drift from the active implementation.♻️ Proposed cleanup
-// val navDestination = when (uiState) { -// is MainUiState.Loading -> LOGIN_GRAPH -// is Success -> if ((uiState as Success).userData.authenticated) { -// val authOption = viewModel.getAuthOption() -// when (authOption) { -// AppLockOption.MifosPasscode -> ROOT_MIFOS_PASSCODE_ROUTE -// AppLockOption.DeviceLock -> PLATFORM_AUTHENTICATOR_ROUTE -// AppLockOption.None -> { -// viewModel.logOut() -// LOGIN_GRAPH -// } -// } -// } else { -// LOGIN_GRAPH -// } -// }🤖 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/MifosPayApp.kt` around lines 83 - 98, Remove the stale commented navDestination implementation block (the commented when over navDestination referencing MainUiState.Loading, Success and the viewModel.getAuthOption/AppLockOption branches) to avoid drift and clutter; locate the commented section around the navDestination logic (mentions MainUiState.Loading, Success, viewModel.logOut, AppLockOption.MifosPasscode/DeviceLock/None and PLATFORM_AUTHENTICATOR_ROUTE/ROOT_MIFOS_PASSCODE_ROUTE/LOGIN_GRAPH) and delete the entire commented-out code so only the active navigation implementation remains.
🤖 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/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/ChooseAuthOptionScreenViewmodel.kt`:
- Around line 23-25: The DISPLAY_NAME constant is a placeholder ("XYZ") and will
be shown verbatim in biometric prompts; replace DISPLAY_NAME with a meaningful
fallback (e.g., the app name or a localized resource string) and remove the
hardcoded "XYZ". Update usages in ChooseAuthOptionScreenViewmodel (and any code
that reads DISPLAY_NAME) to use the new meaningful fallback, and add a warning
log entry when the fallback is actually used so developers can detect missing
user display names (log from the same class/method that chooses the display
name).
- Around line 67-69: Wrap the call to
action.platformAuthenticationProvider.setupPlatformAuthenticator() inside
viewModelScope.launch to match the other platform auth calls and avoid blocking
the main thread; replace the direct invocation in
ChooseAuthOptionScreenViewmodel (the is
ChooseAuthOptionScreenAction.SetupPlatformAuthenticator branch) with a coroutine
launch that calls setupPlatformAuthenticator(), consistent with the existing
patterns used for updateAuthenticatorStatus() and registerUser().
In
`@feature/passcode/src/commonMain/kotlin/org/mifos/feature/passcode/MifosPasscode.kt`:
- Around line 28-40: Add an explicit onPasscodeRejected callback parameter to
the MifosPasscode composable signature and forward it to PasscodeScreen instead
of passing a hardcoded empty lambda: update the function MifosPasscode to accept
onPasscodeRejected: () -> Unit = {} and call PasscodeScreen using named
arguments so the fifth parameter maps to onPasscodeRejected; also update
mifosRootPasscodeScreen in MifosPasscodeNavigation.kt to accept and forward
onPasscodeRejected into MifosPasscode so callers (and RootNavGraph) can provide
a handler for passcode rejection events.
---
Duplicate comments:
In `@core/data/build.gradle.kts`:
- Line 33: The build declaration implementation(projects.core.model) in
core:data reintroduced a transitive API break because core:data exposes
core.model types; change that dependency back to api(projects.core.model) in the
core/data build script (or alternatively ensure every module that depends on
core:data also directly declares projects.core.model) so downstream consumers
keep access to core.model types; update the dependency line referencing
implementation(projects.core.model) to api(projects.core.model) (or add explicit
projects.core.model to the consumer modules' dependencies).
In
`@feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/ChooseAuthOptionScreen.kt`:
- Around line 113-129: The icons for the two AuthOptionCard entries are swapped:
replace the icon used in the Device Lock card (where onSelect calls
ChooseAuthOptionScreenAction.OnSelectDeviceLock) from Icons.Default.Dialpad to a
lock/fingerprint icon (e.g., Icons.Default.Lock or Icons.Default.Fingerprint),
and replace the icon used in the Passcode card (where selected checks
AppLockOption.MifosPasscode and onSelect calls
ChooseAuthOptionScreenAction.OnSelectPasscode) from Icons.Default.People to
Icons.Default.Dialpad so device lock shows a lock/fingerprint and passcode shows
a dialpad.
---
Nitpick comments:
In `@cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayApp.kt`:
- Around line 83-98: Remove the stale commented navDestination implementation
block (the commented when over navDestination referencing MainUiState.Loading,
Success and the viewModel.getAuthOption/AppLockOption branches) to avoid drift
and clutter; locate the commented section around the navDestination logic
(mentions MainUiState.Loading, Success, viewModel.logOut,
AppLockOption.MifosPasscode/DeviceLock/None and
PLATFORM_AUTHENTICATOR_ROUTE/ROOT_MIFOS_PASSCODE_ROUTE/LOGIN_GRAPH) and delete
the entire commented-out code so only the active navigation implementation
remains.
In
`@feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/ChooseAuthOptionScreen.kt`:
- Around line 197-204: Replace the two independent ifs in navigationHelper with
an idiomatic when expression over the AppLockOption parameter: match
AppLockOption.DeviceLock to invoke whenDeviceLockSelected(),
AppLockOption.MifosPasscode to invoke whenPasscodeSelected(), and include an
else branch (no-op) to make the expression exhaustive; update the
navigationHelper function body accordingly.
- Around line 100-106: The Column's Modifier applies fixed padding before
fillMaxSize and scaffold insets after, which causes the fixed 16.dp to reduce
the measured bounds; update the Modifier chain used in ChooseAuthOptionScreen's
Column so fillMaxSize() is called first and the scaffold content padding (the
variable `it`) is applied before the fixed 16.dp (i.e., use fillMaxSize() then
padding(it) then padding(16.dp)) so scaffold insets are consumed first and the
extra margin is inside the safe area.
In
`@feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/ChooseAuthOptionScreenViewmodel.kt`:
- Around line 100-135: The code writes registrationResult into state
unconditionally via mutableStateFlow.update then only clears it on success; fix
by removing the initial unconditional write or ensuring
setRegistrationResultNull() is called for all non-success branches: update the
when(registrationResult) branches in ChooseAuthOptionScreenViewmodel (the
mutableStateFlow.update block, the RegistrationResult.Error,
PlatformAuthenticatorNotAvailable, and PlatformAuthenticatorNotSet branches) so
that after updating screenState they also clear registrationResult (call
setRegistrationResultNull()) or avoid setting registrationResult into the state
until a success is confirmed (i.e., only call mutableStateFlow.update {
it.copy(registrationResult = registrationResult) } on is
RegistrationResult.Success).
In
`@feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/AuthenticationScreen.kt`:
- Line 130: The dismiss button is being hidden by setting dismissButtonText = ""
in AuthenticationScreen (file: AuthenticationScreen.kt); change this to use null
instead of an empty string so it matches the component's default/hiding behavior
(replace dismissButtonText = "" with dismissButtonText = null in the
AuthenticationScreen composable invocation).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
cmp-android/dependencies/prodReleaseRuntimeClasspath.tree.txtcmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayApp.ktcore/data/build.gradle.ktsfeature/auth/src/commonMain/composeResources/values/strings.xmlfeature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/ChooseAuthOptionScreen.ktfeature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/ChooseAuthOptionScreenViewmodel.ktfeature/authenticator-biometrics/src/commonMain/composeResources/values/strings.xmlfeature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/AuthenticationScreen.ktfeature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/components/SystemAuthenticatorButton.ktfeature/passcode/src/commonMain/kotlin/org/mifos/feature/passcode/MifosPasscode.ktfeature/passcode/src/commonMain/kotlin/org/mifos/feature/passcode/MifosPasscodeNavigation.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- feature/auth/src/commonMain/composeResources/values/strings.xml
- feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/components/SystemAuthenticatorButton.kt
...monMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/ChooseAuthOptionScreenViewmodel.kt
Outdated
Show resolved
Hide resolved
...monMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/ChooseAuthOptionScreenViewmodel.kt
Outdated
Show resolved
Hide resolved
feature/passcode/src/commonMain/kotlin/org/mifos/feature/passcode/MifosPasscode.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/RootNavGraph.kt (1)
63-67:⚠️ Potential issue | 🟠 MajorBack-stack not popped on passcode success; rejection still a no-op.
The prior review flagged both of these — they remain unresolved:
Missing
popBackStack()— Lines 65–66 passnavHostController::navigateToMainGraphdirectly, without first callingpopBackStack(). The passcode destination stays on the back stack, so pressing Back from the main graph returns the user to the passcode screen. Compare withplatformAuthenticator(lines 70–73) andchooseAuthOptionScreen(lines 53–60), both of which correctly pop before navigating.Silent rejection —
onPasscodeRejectedwas removed as a call-site parameter in this commit, but it is now hardcoded as{}insidemifosRootPasscodeScreeninMifosPasscodeNavigation.kt(line 35:MifosPasscode(...) {}). The net effect is identical: a rejected passcode leaves the user stuck with no recovery path.🐛 Proposed fix for both issues
mifosRootPasscodeScreen( onForgotButton = onClickLogout, - onPasscodeConfirm = navHostController::navigateToMainGraph, - onPasscodeCreation = navHostController::navigateToMainGraph, + onPasscodeConfirm = { + navHostController.popBackStack() + navHostController.navigateToMainGraph() + }, + onPasscodeCreation = { + navHostController.popBackStack() + navHostController.navigateToMainGraph() + }, )For the rejection no-op,
mifosRootPasscodeScreeninMifosPasscodeNavigation.ktshould accept and forward anonPasscodeRejectedcallback instead of hardcoding{}, and the call-site here should passonClickLogout(or a dedicated retry route):// MifosPasscodeNavigation.kt fun NavGraphBuilder.mifosRootPasscodeScreen( onForgotButton: () -> Unit, onPasscodeConfirm: () -> Unit, onPasscodeCreation: () -> Unit, + onPasscodeRejected: () -> Unit, ) { composable(route = ROOT_MIFOS_PASSCODE_ROUTE) { MifosPasscode( onForgotButton, onPasscodeConfirm, onPasscodeCreation, - ) {} + onPasscodeRejected, ) } }// RootNavGraph.kt mifosRootPasscodeScreen( onForgotButton = onClickLogout, onPasscodeConfirm = { … }, onPasscodeCreation = { … }, + onPasscodeRejected = onClickLogout, )🤖 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/RootNavGraph.kt` around lines 63 - 67, The mifosRootPasscodeScreen call should pop the passcode destination before navigating and forward a rejection handler instead of relying on a hardcoded no-op: change the onPasscodeConfirm and onPasscodeCreation arguments from navHostController::navigateToMainGraph to lambdas that call navHostController.popBackStack() then navHostController.navigateToMainGraph, and update MifosPasscodeNavigation.kt so MifosPasscode/mifosRootPasscodeScreen accepts an onPasscodeRejected parameter (no longer hardcoded to {}) and forward that callback; at the call-site pass onClickLogout (or another appropriate retry/logout handler) for onPasscodeRejected so a rejected passcode has a recovery path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/RootNavGraph.kt`:
- Around line 63-67: The mifosRootPasscodeScreen call should pop the passcode
destination before navigating and forward a rejection handler instead of relying
on a hardcoded no-op: change the onPasscodeConfirm and onPasscodeCreation
arguments from navHostController::navigateToMainGraph to lambdas that call
navHostController.popBackStack() then navHostController.navigateToMainGraph, and
update MifosPasscodeNavigation.kt so MifosPasscode/mifosRootPasscodeScreen
accepts an onPasscodeRejected parameter (no longer hardcoded to {}) and forward
that callback; at the call-site pass onClickLogout (or another appropriate
retry/logout handler) for onPasscodeRejected so a rejected passcode has a
recovery path.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/RootNavGraph.ktfeature/passcode/src/commonMain/composeResources/values/strings.xmlfeature/passcode/src/commonMain/kotlin/org/mifos/feature/passcode/MifosPasscode.ktfeature/passcode/src/commonMain/kotlin/org/mifos/feature/passcode/MifosPasscodeNavigation.kt
✅ Files skipped from review due to trivial changes (1)
- feature/passcode/src/commonMain/composeResources/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- feature/passcode/src/commonMain/kotlin/org/mifos/feature/passcode/MifosPasscodeNavigation.kt
TheKalpeshPawar
force-pushed
the
integrate-passcode-module
branch
from
7a77109 to
e3248ea
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/auth/src/commonTest/kotlin/org/mifospay/feature/auth/login/LoginViewModelTest.kt (1)
52-55:⚠️ Potential issue | 🔴 Critical
LoginViewModeltest instantiation is missing a required constructor dependency.Line 52 constructs
LoginViewModelwithoutinstanceConfigManager, but the ViewModel constructor now requires it. This will fail test compilation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/auth/src/commonTest/kotlin/org/mifospay/feature/auth/login/LoginViewModelTest.kt` around lines 52 - 55, The test instantiates LoginViewModel without the newly required instanceConfigManager dependency; update the LoginViewModel creation in LoginViewModelTest to provide an instanceConfigManager (e.g., a mock or test double) when calling the LoginViewModel constructor alongside loginUseCase and SavedStateHandle; locate the LoginViewModel constructor to confirm the exact parameter type (InstanceConfigManager) and create/pass a matching mocked object so the test compiles.
♻️ Duplicate comments (4)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/di/KoinModules.kt (1)
105-109:MifosPasscodeModulePascalCase naming for aprivate val.Private
valproperties in Kotlin should follow camelCase convention (mifosPasscodeModule). The same applies to the pre-existingLibraryModuleat line 101.♻️ Proposed fix
- private val MifosPasscodeModule = module { + private val mifosPasscodeModule = module {- MifosPasscodeModule, + mifosPasscodeModule,🤖 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/di/KoinModules.kt` around lines 105 - 109, Private vals are using PascalCase; rename the private val identifiers MifosPasscodeModule -> mifosPasscodeModule (and similarly LibraryModule -> libraryModule) to follow Kotlin camelCase for properties, update any internal references/usages where those vals are referenced, and keep the initializer expression (single { PasscodeManager(get(), MainScope()).initialize() }) unchanged so behavior remains the same.feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/components/SystemAuthenticatorButton.kt (1)
105-122:⚠️ Potential issue | 🟡 MinoriOS branch still always renders
face_scanregardless ofplatformAuthOptions— Touch ID devices show the wrong icon.The iOS
BIOMETRICS_SETpath unconditionally usesRes.drawable.face_scan, ignoringplatformAuthOptionsentirely. An iPhone with Touch ID would display a face-scan icon. The Android branch already demonstrates the correct pattern (checkplatformAuthOptions.contains(PlatformAuthOptions.Fingerprint), etc.); the same differentiation should be applied here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/components/SystemAuthenticatorButton.kt` around lines 105 - 122, In the Platform.IOS branch of SystemAuthenticatorButton (the block that currently checks authenticatorStatus.contains(PlatformAuthenticatorStatus.BIOMETRICS_SET)), change the unconditional use of Res.drawable.face_scan to select the icon based on platformAuthOptions (e.g., if platformAuthOptions.contains(PlatformAuthOptions.FaceID) use Res.drawable.face_scan, if platformAuthOptions.contains(PlatformAuthOptions.Fingerprint) use the fingerprint drawable) so Touch ID devices show the fingerprint icon; keep the same Image component and onClick behavior but swap the painterResource selection logic to inspect platformAuthOptions rather than always using face_scan.core/data/build.gradle.kts (1)
33-33:⚠️ Potential issue | 🔴 Critical
core.modelvisibility drop can still break downstream compile paths.Line 33 switching from
apitoimplementationremains risky ifcore:datapublic declarations exposecore.modeltypes. This appears to be the same unresolved issue flagged previously.🐛 Proposed fix
- implementation(projects.core.model) + api(projects.core.model)#!/bin/bash set -euo pipefail echo "=== Imports from core.model inside core:data commonMain ===" rg -n --type=kt '^import\s+org\.mifospay\.core\.model\.' core/data/src/commonMain || true echo echo "=== Public declarations in core:data that may expose model types ===" rg -n --type=kt '^(public\s+)?(interface|class|data class|fun)\s+' core/data/src/commonMain || true echo echo "If public interfaces/classes/functions in core:data reference org.mifospay.core.model types," echo "keep projects.core.model as api to preserve transitive visibility."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/build.gradle.kts` at line 33, The dependency change to implementation(projects.core.model) in core/data can break downstream compilation if any public API in core:data exposes types from org.mifospay.core.model; inspect public declarations in core/data/src/commonMain for references to core.model types (e.g., public classes, interfaces, data classes, or public functions that import org.mifospay.core.model) and if any are found, change the dependency back to api(projects.core.model) so transitive visibility is preserved for consumers.feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/ChooseAuthOptionScreenViewmodel.kt (1)
67-69:⚠️ Potential issue | 🟠 MajorAvoid potentially blocking provider setup on the action handler thread.
Line 68 calls
setupPlatformAuthenticator()directly. If this API performs any blocking/platform work, this can stall UI handling.#!/bin/bash # Verify the provider contract and existing call patterns. rg -n --type kotlin -C3 'interface\s+PlatformAuthenticationProvider|fun\s+setupPlatformAuthenticator\s*\(|suspend\s+fun\s+setupPlatformAuthenticator' rg -n --type kotlin -C3 'SetupPlatformAuthenticator|setupPlatformAuthenticator\s*\('🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/ChooseAuthOptionScreenViewmodel.kt` around lines 67 - 69, The handler for ChooseAuthOptionScreenAction.SetupPlatformAuthenticator currently calls action.platformAuthenticationProvider.setupPlatformAuthenticator() directly which may block the action-handler thread; change this to invoke setupPlatformAuthenticator off the current thread (e.g., launch a coroutine or dispatch to a background dispatcher) so the UI/action loop isn't stalled—if setupPlatformAuthenticator is suspendable, call it from a coroutine scope (e.g., launch { ... } or withContext(Dispatchers.IO)), otherwise wrap the synchronous call in a background executor; ensure you update the ChooseAuthOptionScreenAction.SetupPlatformAuthenticator branch to perform the off-thread invocation and propagate errors/results back to the viewmodel appropriately.
🧹 Nitpick comments (10)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/PasscodeNavGraph.kt (2)
16-40: Remove the commented-out dead code instead of leaving it in.The entire
passcodeNavGraphbody is commented out rather than deleted. It also referencesMifosNavGraph.PASSCODE_GRAPH, which has already been removed in this PR — meaning it could not even compile if un-commented. Leaving it adds noise and could confuse future readers into thinking it is recoverable as-is.♻️ Proposed change — delete the dead block
-// internal fun NavGraphBuilder.passcodeNavGraph(navController: NavController) { -// navigation( -// route = MifosNavGraph.PASSCODE_GRAPH, -// startDestination = PASSCODE_SCREEN, -// ) { -// passcodeRoute( -// onForgotButton = { -// navController.popBackStack() -// navController.navigateToMainGraph() -// }, -// onSkipButton = { -// navController.popBackStack() -// navController.navigateToMainGraph() -// }, -// onPasscodeConfirm = { -// navController.popBackStack() -// navController.navigateToMainGraph() -// }, -// onPasscodeRejected = { -// navController.popBackStack() -// navController.navigateToMainGraph() -// }, -// ) -// } -// } - fun NavController.navigateToMainGraph() {🤖 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/PasscodeNavGraph.kt` around lines 16 - 40, The file contains a large commented-out dead block defining passcodeNavGraph that references removed symbols (MifosNavGraph.PASSCODE_GRAPH, PASSCODE_SCREEN); remove the entire commented-out passcodeNavGraph block so the file only contains active code and no stale references — locate the commented section around the passcodeNavGraph definition and delete it entirely.
42-57: Consider renaming the file to match its remaining content.With
passcodeNavGraphgone,PasscodeNavGraph.ktnow only hostsnavigateToMainGraph(). The filename is misleading;MainNavGraph.kt(or moving this function into an existing graph-helpers file) would better reflect what the file contains.🤖 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/PasscodeNavGraph.kt` around lines 42 - 57, The file PasscodeNavGraph.kt no longer contains passcodeNavGraph and only defines navigateToMainGraph(), so rename the file to reflect its content (e.g., MainNavGraph.kt) or move the navigateToMainGraph() function into an existing navigation helper file; update any imports/usages to reference the new file/module name and ensure the symbol navigateToMainGraph and MifosNavGraph.MAIN_GRAPH references remain unchanged.core/data/src/commonMain/kotlin/org/mifospay/core/data/util/Helpers.kt (1)
12-13: Consolidate serialization mapping into one source of truth.Current read/write mapping is duplicated and can drift when new
AppLockOptionvalues are added. Also, Line 21 uses an implicit empty-string sentinel forNone; a named persisted token is clearer.♻️ Proposed refactor (with backward compatibility)
const val MIFOS_PASSCODE_VALUE = "mifos_passcode" const val DEVICE_AUTHENTICATION_METHOD_VALUE = "device_authentication_method" +const val NO_AUTHENTICATION_METHOD_VALUE = "none" object Helpers { + private val optionToValue = mapOf( + AppLockOption.MifosPasscode to MIFOS_PASSCODE_VALUE, + AppLockOption.DeviceLock to DEVICE_AUTHENTICATION_METHOD_VALUE, + AppLockOption.None to NO_AUTHENTICATION_METHOD_VALUE, + ) + + private val valueToOption = optionToValue.entries.associate { (k, v) -> v to k } fun authOptionToStringMapperFunction(option: AppLockOption): String { - return when (option) { - AppLockOption.MifosPasscode -> MIFOS_PASSCODE_VALUE - AppLockOption.DeviceLock -> DEVICE_AUTHENTICATION_METHOD_VALUE - AppLockOption.None -> "" - } + return optionToValue.getValue(option) } fun stringToAuthOptionMapperFunction(option: String): AppLockOption { - return when (option) { - MIFOS_PASSCODE_VALUE -> AppLockOption.MifosPasscode - DEVICE_AUTHENTICATION_METHOD_VALUE -> AppLockOption.DeviceLock - else -> AppLockOption.None - } + return valueToOption[option] + ?: if (option.isEmpty()) AppLockOption.None // legacy stored value + else AppLockOption.None } }Also applies to: 17-30
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/commonMain/kotlin/org/mifospay/core/data/util/Helpers.kt` around lines 12 - 13, Create a single canonical mapping in Helpers.kt between AppLockOption enum values and their persisted strings (replace the duplicated read/write logic across lines ~12-30), introduce a named constant for the "none" sentinel (instead of using an implicit empty string), and update the serialization/deserialization code to use that one map; ensure backward compatibility by accepting the legacy empty-string value when reading but always write the new named token (update usages of MIFOS_PASSCODE_VALUE and DEVICE_AUTHENTICATION_METHOD_VALUE to read/write via the new map and reference AppLockOption constants).feature/authenticator-biometrics/src/commonMain/composeResources/values/strings.xml (1)
19-19: Move Windows Hello copy out ofcommonMaininto a Desktop-specific resource set.
"Authenticate using Windows Hello"contains platform-specific branding (Windows Hello) and belongs in adesktopMain(orwindowsMain) resource set rather thancommonMain. Keeping it here means Android and iOS build artefacts carry a string that is irrelevant to those platforms, and it muddies the boundary between shared and platform-specific UI copy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/authenticator-biometrics/src/commonMain/composeResources/values/strings.xml` at line 19, The string resource feature_authenticator_biometrics_authenticate_windows_hello in commonMain is platform-specific and should be moved to a desktop-specific resource set; remove this entry from feature/authenticator-biometrics/src/commonMain/composeResources/values/strings.xml and add an equivalent entry (same resource name or a desktop-specific name) into your desktopMain/windowsMain composeResources/values/strings.xml, then update any code references if you rename the key (or keep the same key so no code change is needed) and replace the commonMain copy with a neutral, cross-platform string (or remove it) so Android/iOS builds no longer include Windows-specific branding.feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/components/AuthOptionCard.kt (1)
59-60: Avoid fixed card height for large font sizes.Line 59 hard-codes
height(130.dp). With larger accessibility font scales or longer localized copy, content can clip.✅ Suggested fix
-import androidx.compose.foundation.layout.height +import androidx.compose.foundation.layout.heightIn ... - .height(130.dp) + .heightIn(min = 130.dp)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/components/AuthOptionCard.kt` around lines 59 - 60, AuthOptionCard currently uses a fixed Modifier.height(130.dp) which can clip content at large font scales or with longer localized text; replace the fixed height with a flexible constraint (for example use Modifier.heightIn(min = 130.dp) or Modifier.wrapContentHeight() with optional max via heightIn) on the same Modifier chain in AuthOptionCard so the card can expand as needed while preserving a minimum visual size.feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/components/SystemAuthenticatorButton.kt (1)
152-156: Use theshapeparameter ofButtoninstead ofModifier.clip.
Buttonaccepts ashape: Shape = ButtonDefaults.shapeparameter that controls the button's container shape through its internalSurface, allowing proper rendering of the background, border, shadow, and ripple effects with that shape.Using
Modifier.clip(RoundedCornerShape(50))is not the correct approach: while it does clip the button's final visual output (including background and ripple), it's better to pass the shape directly to theButtonparameter for proper Material3 styling:♻️ Proposed refactor
Button( onClick = onClick, + shape = RoundedCornerShape(50), modifier = Modifier.height(50.dp) .width(250.dp) - .clip(RoundedCornerShape(50)), ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/components/SystemAuthenticatorButton.kt` around lines 152 - 156, In SystemAuthenticatorButton, remove Modifier.clip(RoundedCornerShape(50)) from the Button's modifier and instead pass the shape via the Button shape parameter (e.g., shape = RoundedCornerShape(50)); keep the size modifiers (height(50.dp).width(250.dp)) on the Modifier and let Button apply the shape to its internal Surface so background, border, shadow and ripple render correctly; update the Button invocation in SystemAuthenticatorButton accordingly.cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayApp.kt (2)
83-98: Remove commented-out legacy navigation code.This large commented block is obsolete and makes the auth-routing path harder to read and maintain.
🧹 Proposed cleanup
-// val navDestination = when (uiState) { -// is MainUiState.Loading -> LOGIN_GRAPH -// is Success -> if ((uiState as Success).userData.authenticated) { -// val authOption = viewModel.getAuthOption() -// when (authOption) { -// AppLockOption.MifosPasscode -> ROOT_MIFOS_PASSCODE_ROUTE -// AppLockOption.DeviceLock -> PLATFORM_AUTHENTICATOR_ROUTE -// AppLockOption.None -> { -// viewModel.logOut() -// LOGIN_GRAPH -// } -// } -// } else { -// LOGIN_GRAPH -// } -// }🤖 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/MifosPayApp.kt` around lines 83 - 98, Remove the large commented-out legacy navigation block that computes navDestination (the when over MainUiState.Loading and Success, the cast to Success, calls to viewModel.getAuthOption(), AppLockOption branches including ROOT_MIFOS_PASSCODE_ROUTE, PLATFORM_AUTHENTICATOR_ROUTE, the viewModel.logOut() fallthrough to LOGIN_GRAPH, etc.); delete this obsolete commented code so the current auth-routing logic is clearer and not cluttered by the legacy navDestination snippet.
99-102: Move auth option retrieval to ViewModel state.
getAuthOption()synchronously callssettings.getString()during composition via theremember()block at line 101. The Settings library performs disk IO (SharedPreferences/DataStore backend), which can cause UI jank. Expose auth option as a StateFlow from the ViewModel and collect it reactively in the composable instead of calling it directly during composition.🤖 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/MifosPayApp.kt` around lines 99 - 102, The composable MifosPayApp currently calls viewModel.getAuthOption() synchronously inside remember (when uiState is Success), causing disk IO during composition; instead, move auth option retrieval into the ViewModel: create a StateFlow (e.g., authOptionFlow or authOption: StateFlow<AuthOption?>) that is updated from the ViewModel using a coroutine (viewModelScope) to call settings.getString()/IO off the main thread and emit the result (only when the user becomes authenticated), then in MifosPayApp collect that StateFlow with collectAsState() (or collectAsStateWithLifecycle) and remove the direct getAuthOption() call from the remember block so composition no longer performs IO.feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/AuthenticationScreenViewModel.kt (1)
57-60:authenticatorStatusis defined in state but never updated.Right now
AuthenticationScreenState.authenticatorStatuswill remainemptySet(). Either populate it afterupdateAuthenticatorStatus()or remove it from state to avoid misleading UI state.Also applies to: 99-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/AuthenticationScreenViewModel.kt` around lines 57 - 60, After calling platformAuthenticationProvider.updateAuthenticatorStatus(), read the provider's current status (e.g. platformAuthenticationProvider.authenticatorStatus or platformAuthenticationProvider.getAuthenticatorStatus()) and set AuthenticationScreenState.authenticatorStatus accordingly (for example via _state.value = _state.value.copy(authenticatorStatus = <provider status>)); do the same wherever updateAuthenticatorStatus() is called (including the block around onAuthenticatorClick) so the state reflects the updated status, or if you prefer remove AuthenticationScreenState.authenticatorStatus from the state model and related UI usage to avoid a stale/unused field.feature/settings/src/commonMain/kotlin/org/mifospay/feature/settings/SettingsViewModel.kt (1)
35-41:isChangePasscodeVisibleis snapshot-only; consider making it reactive.Line 40 computes visibility once during initialization. If auth option changes during the same Settings session, this flag won’t update.
Also applies to: 155-155
🤖 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 35 - 41, The isChangePasscodeVisible boolean is computed once at SettingsViewModel initialization and won't react to later auth option changes; update SettingsViewModel to derive that flag reactively from chooseAuthOptionRepository.getAuthOption() (or its Flow/StateFlow) instead of computing it once for SettingsState: subscribe/combine the auth option flow with the current SettingsState (or include authOption in SettingsState) and update SettingsState.isChangePasscodeVisible whenever chooseAuthOptionRepository emits a new AppLockOption so the UI reflects changes during the same session; look for SettingsViewModel, SettingsState, and chooseAuthOptionRepository.getAuthOption() to implement this reactive flow/combination.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmp-android/dependencies/prodReleaseRuntimeClasspath.txt`:
- Around line 459-463: The Kotlin stdlib artifacts on the runtime classpath
(org.jetbrains.kotlin:kotlin-stdlib-jdk7,
org.jetbrains.kotlin:kotlin-stdlib-jdk8, org.jetbrains.kotlin:kotlin-stdlib,
plus kotlin-reflect and kotlin-parcelize-runtime) are resolved to multiple
conflicting versions; find the transitive dependencies pulling 1.9.0, 1.8.22,
and 2.3.0 and force a single Kotlin version line (the catalog declares kotlin =
"2.2.21") by either updating the version in the version catalog to the desired
uniform version or adding a Gradle constraint/resolution rule (e.g.,
dependencyConstraints/platform or configurations.all {
resolutionStrategy.force(...) } or dependencySubstitution) to pin all
org.jetbrains.kotlin:* artifacts to the chosen version; search for usages of the
artifacts named above in dependency trees (./gradlew :dependencies) to locate
the transitive offenders and then apply the version alignment.
In `@cmp-android/src/main/kotlin/org/mifospay/MainActivity.kt`:
- Line 20: The activity currently imports and extends FragmentActivity which
breaks AppCompat theme attribute resolution; update the import to
androidx.appcompat.app.AppCompatActivity and change the MainActivity class
declaration to extend AppCompatActivity (keeping existing methods like
setContent and enableEdgeToEdge unchanged) so Theme.AppCompat.NoActionBar and
AppCompat styling are preserved.
In
`@feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/ChooseAuthOptionScreenViewmodel.kt`:
- Around line 92-138: Wrap the external provider calls inside the
viewModelScope.launch (the
platformAuthenticationProvider.updateAuthenticatorStatus() and
platformAuthenticationProvider.registerUser(...) calls) with a
runCatching/try-catch so any thrown exception is caught and the mutableStateFlow
is updated to an Error state (ChooseAuthOptionScreenUiState.ScreenState.Error)
with the exception message; ensure after catching you do not continue into the
success branch (skip saveAppLockOption, saveRegistrationData, sendEvent,
setRegistrationResultNull) unless registrationResult is a successful
RegistrationResult.Success, and make sure any exception path logs or surfaces
the error to the UI consistently.
In
`@feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/components/AuthOptionCard.kt`:
- Around line 48-49: AuthOptionCard currently defaults subtitle = "" but always
renders the subtitle row and trailing icon (the block that renders subtitle and
Icons.Outlined.ChevronRight), causing an empty/awkward second row when subtitle
is omitted; update the AuthOptionCard composable to conditionally render that
subtitle Row only when subtitle.isNotBlank(), leaving the primary title Row
unchanged and ensuring any spacing/padding or click behavior tied to that Row is
preserved when the subtitle is absent.
- Around line 58-61: The AuthOptionCard's Modifier currently uses .clickable {
onSelect.invoke() } which doesn't expose selection semantics to accessibility
services; replace that .clickable with .selectable(selected = /* use the card's
selected state */, onClick = { onSelect.invoke() }, role = Role.RadioButton)
(import androidx.compose.foundation.selection.selectable and
androidx.compose.ui.semantics.Role) so the container exposes the selected state
and radio role to TalkBack/VoiceOver; keep the rest of the Modifier chain
(.height, .fillMaxWidth) and ensure the selected boolean used matches the card's
selection state passed into AuthOptionCard.
In
`@feature/auth/src/commonTest/kotlin/org/mifospay/feature/auth/login/LoginViewModelTest.kt`:
- Around line 230-233: The test collects viewModel.eventFlow after the login
action, which can miss the emitted event; move the collection to start before
dispatching the login action so the Turbine subscription observes the emission.
Specifically, in LoginViewModelTest start the viewModel.eventFlow.test { ... }
(or create the Turbine collector) before calling the login/dispatch method on
viewModel (the method that triggers the login event), then assert awaitItem() is
LoginEvent.NavigateToChooseAuthOptionScreen and finally perform any necessary
cleanup.
In
`@feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/AuthenticationScreenViewModel.kt`:
- Around line 55-84: The authenticateUser method can run concurrently causing
duplicate prompts; add a concurrency guard (e.g., a private AtomicBoolean or a
Mutex) to the AuthenticationScreenViewModel that short-circuits authenticateUser
if an authentication is already in progress, set the flag/lock at start of the
operation and clear it in a finally block so it’s always reset on success,
error, or cancellation; keep existing behavior inside the coroutine
(platformAuthenticationProvider.updateAuthenticatorStatus,
platformAuthenticationProvider.onAuthenticatorClick, state updates, sendEvent
and clearUserRegistrationFromApp) but ensure they only run when the guard allows
the single active authentication.
In
`@feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/components/SystemAuthenticatorButton.kt`:
- Around line 72-99: The Image composables in SystemAuthenticatorButton.kt that
use Modifier.clickable { onClick() } need to include role = Role.Button so
screen readers announce them as interactive buttons; update every clickable
Image (the Fingerprint, FaceId, UserCredential branches and the
DEVICE_CREDENTIAL_SET branch) to call Modifier.size(50.dp).clickable(onClick = {
onClick() }, role = Role.Button) and add the necessary Role import
(androidx.compose.ui.semantics.Role) at the top of the file.
---
Outside diff comments:
In
`@feature/auth/src/commonTest/kotlin/org/mifospay/feature/auth/login/LoginViewModelTest.kt`:
- Around line 52-55: The test instantiates LoginViewModel without the newly
required instanceConfigManager dependency; update the LoginViewModel creation in
LoginViewModelTest to provide an instanceConfigManager (e.g., a mock or test
double) when calling the LoginViewModel constructor alongside loginUseCase and
SavedStateHandle; locate the LoginViewModel constructor to confirm the exact
parameter type (InstanceConfigManager) and create/pass a matching mocked object
so the test compiles.
---
Duplicate comments:
In `@cmp-shared/src/commonMain/kotlin/org/mifospay/shared/di/KoinModules.kt`:
- Around line 105-109: Private vals are using PascalCase; rename the private val
identifiers MifosPasscodeModule -> mifosPasscodeModule (and similarly
LibraryModule -> libraryModule) to follow Kotlin camelCase for properties,
update any internal references/usages where those vals are referenced, and keep
the initializer expression (single { PasscodeManager(get(),
MainScope()).initialize() }) unchanged so behavior remains the same.
In `@core/data/build.gradle.kts`:
- Line 33: The dependency change to implementation(projects.core.model) in
core/data can break downstream compilation if any public API in core:data
exposes types from org.mifospay.core.model; inspect public declarations in
core/data/src/commonMain for references to core.model types (e.g., public
classes, interfaces, data classes, or public functions that import
org.mifospay.core.model) and if any are found, change the dependency back to
api(projects.core.model) so transitive visibility is preserved for consumers.
In
`@feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/ChooseAuthOptionScreenViewmodel.kt`:
- Around line 67-69: The handler for
ChooseAuthOptionScreenAction.SetupPlatformAuthenticator currently calls
action.platformAuthenticationProvider.setupPlatformAuthenticator() directly
which may block the action-handler thread; change this to invoke
setupPlatformAuthenticator off the current thread (e.g., launch a coroutine or
dispatch to a background dispatcher) so the UI/action loop isn't stalled—if
setupPlatformAuthenticator is suspendable, call it from a coroutine scope (e.g.,
launch { ... } or withContext(Dispatchers.IO)), otherwise wrap the synchronous
call in a background executor; ensure you update the
ChooseAuthOptionScreenAction.SetupPlatformAuthenticator branch to perform the
off-thread invocation and propagate errors/results back to the viewmodel
appropriately.
In
`@feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/components/SystemAuthenticatorButton.kt`:
- Around line 105-122: In the Platform.IOS branch of SystemAuthenticatorButton
(the block that currently checks
authenticatorStatus.contains(PlatformAuthenticatorStatus.BIOMETRICS_SET)),
change the unconditional use of Res.drawable.face_scan to select the icon based
on platformAuthOptions (e.g., if
platformAuthOptions.contains(PlatformAuthOptions.FaceID) use
Res.drawable.face_scan, if
platformAuthOptions.contains(PlatformAuthOptions.Fingerprint) use the
fingerprint drawable) so Touch ID devices show the fingerprint icon; keep the
same Image component and onClick behavior but swap the painterResource selection
logic to inspect platformAuthOptions rather than always using face_scan.
---
Nitpick comments:
In `@cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayApp.kt`:
- Around line 83-98: Remove the large commented-out legacy navigation block that
computes navDestination (the when over MainUiState.Loading and Success, the cast
to Success, calls to viewModel.getAuthOption(), AppLockOption branches including
ROOT_MIFOS_PASSCODE_ROUTE, PLATFORM_AUTHENTICATOR_ROUTE, the viewModel.logOut()
fallthrough to LOGIN_GRAPH, etc.); delete this obsolete commented code so the
current auth-routing logic is clearer and not cluttered by the legacy
navDestination snippet.
- Around line 99-102: The composable MifosPayApp currently calls
viewModel.getAuthOption() synchronously inside remember (when uiState is
Success), causing disk IO during composition; instead, move auth option
retrieval into the ViewModel: create a StateFlow (e.g., authOptionFlow or
authOption: StateFlow<AuthOption?>) that is updated from the ViewModel using a
coroutine (viewModelScope) to call settings.getString()/IO off the main thread
and emit the result (only when the user becomes authenticated), then in
MifosPayApp collect that StateFlow with collectAsState() (or
collectAsStateWithLifecycle) and remove the direct getAuthOption() call from the
remember block so composition no longer performs IO.
In
`@cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/PasscodeNavGraph.kt`:
- Around line 16-40: The file contains a large commented-out dead block defining
passcodeNavGraph that references removed symbols (MifosNavGraph.PASSCODE_GRAPH,
PASSCODE_SCREEN); remove the entire commented-out passcodeNavGraph block so the
file only contains active code and no stale references — locate the commented
section around the passcodeNavGraph definition and delete it entirely.
- Around line 42-57: The file PasscodeNavGraph.kt no longer contains
passcodeNavGraph and only defines navigateToMainGraph(), so rename the file to
reflect its content (e.g., MainNavGraph.kt) or move the navigateToMainGraph()
function into an existing navigation helper file; update any imports/usages to
reference the new file/module name and ensure the symbol navigateToMainGraph and
MifosNavGraph.MAIN_GRAPH references remain unchanged.
In `@core/data/src/commonMain/kotlin/org/mifospay/core/data/util/Helpers.kt`:
- Around line 12-13: Create a single canonical mapping in Helpers.kt between
AppLockOption enum values and their persisted strings (replace the duplicated
read/write logic across lines ~12-30), introduce a named constant for the "none"
sentinel (instead of using an implicit empty string), and update the
serialization/deserialization code to use that one map; ensure backward
compatibility by accepting the legacy empty-string value when reading but always
write the new named token (update usages of MIFOS_PASSCODE_VALUE and
DEVICE_AUTHENTICATION_METHOD_VALUE to read/write via the new map and reference
AppLockOption constants).
In
`@feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/components/AuthOptionCard.kt`:
- Around line 59-60: AuthOptionCard currently uses a fixed
Modifier.height(130.dp) which can clip content at large font scales or with
longer localized text; replace the fixed height with a flexible constraint (for
example use Modifier.heightIn(min = 130.dp) or Modifier.wrapContentHeight() with
optional max via heightIn) on the same Modifier chain in AuthOptionCard so the
card can expand as needed while preserving a minimum visual size.
In
`@feature/authenticator-biometrics/src/commonMain/composeResources/values/strings.xml`:
- Line 19: The string resource
feature_authenticator_biometrics_authenticate_windows_hello in commonMain is
platform-specific and should be moved to a desktop-specific resource set; remove
this entry from
feature/authenticator-biometrics/src/commonMain/composeResources/values/strings.xml
and add an equivalent entry (same resource name or a desktop-specific name) into
your desktopMain/windowsMain composeResources/values/strings.xml, then update
any code references if you rename the key (or keep the same key so no code
change is needed) and replace the commonMain copy with a neutral, cross-platform
string (or remove it) so Android/iOS builds no longer include Windows-specific
branding.
In
`@feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/AuthenticationScreenViewModel.kt`:
- Around line 57-60: After calling
platformAuthenticationProvider.updateAuthenticatorStatus(), read the provider's
current status (e.g. platformAuthenticationProvider.authenticatorStatus or
platformAuthenticationProvider.getAuthenticatorStatus()) and set
AuthenticationScreenState.authenticatorStatus accordingly (for example via
_state.value = _state.value.copy(authenticatorStatus = <provider status>)); do
the same wherever updateAuthenticatorStatus() is called (including the block
around onAuthenticatorClick) so the state reflects the updated status, or if you
prefer remove AuthenticationScreenState.authenticatorStatus from the state model
and related UI usage to avoid a stale/unused field.
In
`@feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/components/SystemAuthenticatorButton.kt`:
- Around line 152-156: In SystemAuthenticatorButton, remove
Modifier.clip(RoundedCornerShape(50)) from the Button's modifier and instead
pass the shape via the Button shape parameter (e.g., shape =
RoundedCornerShape(50)); keep the size modifiers (height(50.dp).width(250.dp))
on the Modifier and let Button apply the shape to its internal Surface so
background, border, shadow and ripple render correctly; update the Button
invocation in SystemAuthenticatorButton accordingly.
In
`@feature/settings/src/commonMain/kotlin/org/mifospay/feature/settings/SettingsViewModel.kt`:
- Around line 35-41: The isChangePasscodeVisible boolean is computed once at
SettingsViewModel initialization and won't react to later auth option changes;
update SettingsViewModel to derive that flag reactively from
chooseAuthOptionRepository.getAuthOption() (or its Flow/StateFlow) instead of
computing it once for SettingsState: subscribe/combine the auth option flow with
the current SettingsState (or include authOption in SettingsState) and update
SettingsState.isChangePasscodeVisible whenever chooseAuthOptionRepository emits
a new AppLockOption so the UI reflects changes during the same session; look for
SettingsViewModel, SettingsState, and chooseAuthOptionRepository.getAuthOption()
to implement this reactive flow/combination.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
feature/authenticator-biometrics/src/commonMain/composeResources/drawable/eye_scanner.jpgis excluded by!**/*.jpgfeature/authenticator-biometrics/src/commonMain/composeResources/drawable/face_scan.jpgis excluded by!**/*.jpgfeature/authenticator-biometrics/src/commonMain/composeResources/drawable/fingerprint.jpgis excluded by!**/*.jpgfeature/authenticator-biometrics/src/commonMain/composeResources/drawable/keypad.jpgis excluded by!**/*.jpg
📒 Files selected for processing (53)
build-logic/convention/src/main/kotlin/CMPFeatureConventionPlugin.ktcmp-android/dependencies/prodReleaseRuntimeClasspath.tree.txtcmp-android/dependencies/prodReleaseRuntimeClasspath.txtcmp-android/src/main/kotlin/org/mifospay/MainActivity.ktcmp-shared/build.gradle.ktscmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayApp.ktcmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayViewModel.ktcmp-shared/src/commonMain/kotlin/org/mifospay/shared/di/KoinModules.ktcmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/LoginNavGraph.ktcmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/MifosNavGraph.ktcmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/MifosNavHost.ktcmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/PasscodeNavGraph.ktcmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/RootNavGraph.ktcore/data/build.gradle.ktscore/data/src/commonMain/kotlin/org/mifospay/core/data/di/RepositoryModule.ktcore/data/src/commonMain/kotlin/org/mifospay/core/data/repository/AuthenticationRepository.ktcore/data/src/commonMain/kotlin/org/mifospay/core/data/repository/ChooseAuthOptionRepository.ktcore/data/src/commonMain/kotlin/org/mifospay/core/data/repository/ClientRepository.ktcore/data/src/commonMain/kotlin/org/mifospay/core/data/repository/PlatformAuthenticationDataRepository.ktcore/data/src/commonMain/kotlin/org/mifospay/core/data/repositoryImpl/ChooseAuthOptionRepositoryImpl.ktcore/data/src/commonMain/kotlin/org/mifospay/core/data/repositoryImpl/MifosPasscodeAdapterImpl.ktcore/data/src/commonMain/kotlin/org/mifospay/core/data/repositoryImpl/PlatformAuthenticationDataRepositoryImpl.ktcore/data/src/commonMain/kotlin/org/mifospay/core/data/util/AppLockOption.ktcore/data/src/commonMain/kotlin/org/mifospay/core/data/util/Helpers.ktcore/datastore/src/commonMain/kotlin/org/mifospay/core/datastore/UserPreferencesRepository.ktcore/ui/build.gradle.ktsfeature/auth/build.gradle.ktsfeature/auth/src/commonMain/composeResources/values/strings.xmlfeature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/ChooseAuthOptionScreen.ktfeature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/ChooseAuthOptionScreenNavigation.ktfeature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/ChooseAuthOptionScreenViewmodel.ktfeature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/components/AuthOptionCard.ktfeature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/di/AuthModule.ktfeature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/login/LoginScreen.ktfeature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/login/LoginViewModel.ktfeature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/navigation/LoginScreenNavigation.ktfeature/auth/src/commonTest/kotlin/org/mifospay/feature/auth/login/LoginViewModelTest.ktfeature/authenticator-biometrics/build.gradle.ktsfeature/authenticator-biometrics/src/commonMain/composeResources/values/strings.xmlfeature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/AuthenticationScreen.ktfeature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/AuthenticationScreenNavigation.ktfeature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/AuthenticationScreenViewModel.ktfeature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/AuthenticatorBiometricsModule.ktfeature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/components/SystemAuthenticatorButton.ktfeature/passcode/build.gradle.ktsfeature/passcode/src/commonMain/composeResources/values/strings.xmlfeature/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/SettingsScreen.ktfeature/settings/src/commonMain/kotlin/org/mifospay/feature/settings/SettingsViewModel.ktgradle/libs.versions.tomllibs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/PasscodeKeys.ktsettings.gradle.kts
💤 Files with no reviewable changes (1)
- cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/MifosNavGraph.kt
✅ Files skipped from review due to trivial changes (1)
- core/data/src/commonMain/kotlin/org/mifospay/core/data/repository/AuthenticationRepository.kt
🚧 Files skipped from review as they are similar to previous changes (25)
- core/data/src/commonMain/kotlin/org/mifospay/core/data/util/AppLockOption.kt
- feature/passcode/build.gradle.kts
- core/data/src/commonMain/kotlin/org/mifospay/core/data/repository/PlatformAuthenticationDataRepository.kt
- build-logic/convention/src/main/kotlin/CMPFeatureConventionPlugin.kt
- gradle/libs.versions.toml
- core/ui/build.gradle.kts
- feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/AuthenticationScreen.kt
- cmp-shared/build.gradle.kts
- feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/ChooseAuthOptionScreen.kt
- core/data/src/commonMain/kotlin/org/mifospay/core/data/repositoryImpl/MifosPasscodeAdapterImpl.kt
- core/datastore/src/commonMain/kotlin/org/mifospay/core/datastore/UserPreferencesRepository.kt
- cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/RootNavGraph.kt
- feature/authenticator-biometrics/src/commonMain/kotlin/org/mifospay/feature/authenticator/biometrics/AuthenticatorBiometricsModule.kt
- core/data/src/commonMain/kotlin/org/mifospay/core/data/repositoryImpl/PlatformAuthenticationDataRepositoryImpl.kt
- feature/passcode/src/commonMain/kotlin/org/mifos/feature/passcode/MifosPasscodeNavigation.kt
- cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/MifosNavHost.kt
- core/data/src/commonMain/kotlin/org/mifospay/core/data/di/RepositoryModule.kt
- core/data/src/commonMain/kotlin/org/mifospay/core/data/repository/ChooseAuthOptionRepository.kt
- feature/settings/src/commonMain/kotlin/org/mifospay/feature/settings/SettingsScreen.kt
- feature/passcode/src/commonMain/kotlin/org/mifos/feature/passcode/MifosPasscode.kt
- feature/auth/build.gradle.kts
- libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/PasscodeKeys.kt
- core/data/src/commonMain/kotlin/org/mifospay/core/data/repositoryImpl/ChooseAuthOptionRepositoryImpl.kt
- core/data/src/commonMain/kotlin/org/mifospay/core/data/repository/ClientRepository.kt
- feature/auth/src/commonMain/composeResources/values/strings.xml
...monMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/ChooseAuthOptionScreenViewmodel.kt
Outdated
Show resolved
Hide resolved
...rc/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/components/AuthOptionCard.kt
Outdated
Show resolved
Hide resolved
...rc/commonMain/kotlin/org/mifospay/feature/auth/chooseAuthOption/components/AuthOptionCard.kt
Outdated
Show resolved
Hide resolved
feature/auth/src/commonTest/kotlin/org/mifospay/feature/auth/login/LoginViewModelTest.kt
Show resolved
Hide resolved
...onMain/kotlin/org/mifospay/feature/authenticator/biometrics/AuthenticationScreenViewModel.kt
Outdated
Show resolved
Hide resolved
...kotlin/org/mifospay/feature/authenticator/biometrics/components/SystemAuthenticatorButton.kt
Outdated
Show resolved
Hide resolved
TheKalpeshPawar
force-pushed
the
integrate-passcode-module
branch
from
68d097b to
d704966
Compare
TheKalpeshPawar
commented
Feb 28, 2026
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/PasscodeNavGraph.kt
Show resolved
Hide resolved
Collaborator
|
@TheKalpeshPawar No need to show an additional screen "Use Biometrics" instead show passcode screen and biometric prompt bottom sheet appear first if the user has enabled it, and upon closing the biometric prompt, user can log in using passcode, and resolve merge conflicts |
niyajali
requested changes
Mar 1, 2026
TheKalpeshPawar
force-pushed
the
integrate-passcode-module
branch
from
d704966 to
f999879
Compare
…gration - Introduce `ChooseAuthOption` feature to allow users to select between device lock and Mifos passcode. - Implement `ChooseAuthOptionRepository` and `ChooseAuthOptionScreenViewmodel` for managing authentication preferences. - Create UI components for authentication selection, including `AuthOptionCard` and `MessageDialogBox`. - Add `:feature:passcode` module for passcode-specific functionality. - Integrate `mifos-authenticator-passcode` and `mifos-authenticator-biometrics` (v2.0.2) across relevant modules. - Update `gradle/libs.versions.toml` and project configurations to support new dependencies.
…res (openMF#1969) - Added the Mozilla Public License v2.0 copyright header to multiple files in the `auth` and `passcode` features. - Applied code formatting and removed redundant whitespace in `ChooseAuthOptionScreenViewmodel.kt`, `ChooseAuthOptionScreen.kt`, and `MifosPasscode.kt`. - Updated indentation and trailing commas for better code consistency.
…code authenticator - Move `ChooseAuthOption` logic, repository, and utilities from `feature:auth` to `core:data` for centralized access. - Implement `ChooseAuthOptionRepository` and `MifosPasscodeAdapterImpl` to manage app lock preferences and passcode storage. - Introduce new navigation routes for authentication selection and root passcode verification. - Integrate `mifos-authenticator-passcode` and `mifos-authenticator-biometrics` libraries (v2.0.2). - Update `LoginViewModel` to navigate to the `ChooseAuthOption` screen upon successful login. - Refactor `MifosPasscode` component to use injected `PasscodeManager` and the new library-based `PasscodeScreen`. - Update project dependencies and Koin modules to support the refactored authentication architecture. - Apply theme-based styling to `AuthOptionCard` and other UI components.
- Added the Mozilla Public License v2.0 copyright header to `UserPreferencesRepository.kt`, `ClientRepository.kt`, `AuthenticationRepository.kt`, and `AppLockOption.kt`. - Registered `ChooseAuthOptionRepository` in `RepositoryModule.kt`. - Updated imports in `ChooseAuthOptionScreenViewmodel.kt` for better code organization. - Added `:core-base:datastore` and `:feature:passcode` to the runtime classpath for the Android build. - Reflected dependency tree changes in `prodReleaseRuntimeClasspath.tree.txt`, including the integration of `mifos-authenticator-passcode` and `mifos-authenticator-biometrics` (v2.0.2).
…ode management - Refactor `MifosPayApp` to navigate based on the selected `AppLockOption` (Passcode, Device Lock, or None). - Move `chooseAuthOptionScreen` from `LoginNavGraph` to `RootNavGraph` for better lifecycle management. - Update `ChooseAuthOptionScreenViewmodel` to handle passcode navigation and persist selection. - Integrate `PasscodeManager` into `SettingsScreen` to support the "Change Passcode" action. - Update `MifosPayViewModel` to handle secure logout by erasing passcode data via `PasscodeAction`. - Centralize `mifos.authenticator` dependencies in `CMPFeatureConventionPlugin` and remove redundant declarations in feature modules. - Update `mifos-authenticator` library versions to `2.0.3`. - Change `MainActivity` base class to `FragmentActivity` to support biometric authentication requirements. - Wrap the application content with `PlatformAuthenticatorLocalCompositionProvider`. - Improve passcode navigation by adding `onPasscodeCreation` callback and linking the change passcode setting.
…ature - Create `:feature:authenticator-biometrics` module to handle platform-specific biometric and device credential authentication. - Implement `AuthenticationScreen` and `AuthenticationScreenViewModel` for managing biometric authentication flows, including success, error, and re-registration states. - Add `SystemAuthenticatorButton` component that dynamically adapts its UI (Fingerprint, Face ID, Iris, or Keypad) based on the current platform and available authentication options. - Include necessary drawable resources for various authentication icons (fingerprint, face scan, eye scanner, keypad). - Update `ChooseAuthOptionScreenViewmodel` to correctly set the authentication preference when device lock is selected. - Refactor `ChooseAuthOptionScreen` to improve background styling consistency using the project theme. - Register the new `:feature:authenticator-biometrics` module in `settings.gradle.kts`.
…ation flow This commit refactors the biometric authentication data handling and improves the authentication screen's state management and user flow. - **Introduce `PlatformAuthenticationDataRepository`**: - Created a new repository and its implementation (`PlatformAuthenticationDataRepositoryImpl`) to specifically manage the storage of biometric registration data. - This separates the concern of storing biometric data from the `ChooseAuthOptionRepository`. - **Update Authentication Flow**: - Refactored `AuthenticationScreenViewModel` to use the new `PlatformAuthenticationDataRepository`. - Enhanced state management in `AuthenticationScreen` with a dedicated `ScreenState` sealed interface (`Loading`, `Error`, `UserNotRegistered`) for clearer UI representation. - Improved error handling for cases like `UserNotRegistered`, ensuring registration data is cleared and the user is logged out. - **Repository and Dependency Injection**: - `MifosPasscodeAdapterImpl` now correctly calls `chooseAuthOptionRepository.removeAuthOption()` upon passcode deletion. - Updated the Koin `RepositoryModule` to provide the `PlatformAuthenticationDataRepository` dependency. - **Navigation and UI**: - Added `AuthenticationScreenNavigation.kt` to define the navigation route and builder for the platform authenticator screen. - Replaced custom dialog logic in `AuthenticationScreen` with `MifosDialogBox` and `MifosProgressIndicatorOverlay` for a consistent UI.
…guration - Refactor `ChooseAuthOptionScreen` to use `MifosDialogBox` and a cleaner content/dialog separation. - Update `MifosPasscode` with comprehensive `PasscodeAppearanceConfig`, including theme integration for colors, typography, and shapes. - Implement `AuthenticatorBiometricsModule` for Koin dependency injection and update `cmp-shared` to use the local biometrics feature module. - Improve error handling in `ChooseAuthOptionScreenViewmodel` by introducing a specific `ScreenState` and explicit error messaging. - Add missing license headers to repository and navigation files. - Delete redundant `MessageDialogBox.kt` and consolidate UI components. - Standardize code formatting, trailing commas, and indentation across authentication-related modules.
- Add Mozilla Public License v2.0 header to `AuthenticatorBiometricsModule.kt`. - Remove unused imports in `MifosPasscode.kt` and `ChooseAuthOptionScreen.kt`. - Apply consistent code formatting, including trailing commas and indentation, across `MifosPasscode.kt`, `ChooseAuthOptionScreenViewmodel.kt`, and `ChooseAuthOptionScreen.kt`. - Clean up redundant whitespace and commented-out code in authentication UI components.
- Add `:feature:authenticator-biometrics` dependency to the Android runtime classpath. - Update `ChooseAuthOptionScreenNavigation` to use `ChooseAuthOptionScreen` instead of `ChooseAuthOptionContent`. - Clean up imports and formatting in `ChooseAuthOptionScreen.kt`, replacing `viewModel` with `koinViewModel`. - Ensure proper dependency tree resolution for Koin and Mifos authenticator libraries.
- Integrate `platformAuthenticator` into the root navigation graph and handle authentication routing based on app lock preferences. - Redesign `AuthenticateButton` (formerly `ClickableTextButton`) using Material3 `Button` with updated styling and simplified parameters. - Add `AuthenticatorBiometricsModule` to Koin dependency injection. - Update `SettingsViewModel` and `SettingsScreen` to conditionally show the "Change Passcode" option only when Mifos Passcode is the active authentication method. - Refactor `AuthenticationScreenViewModel` to remove redundant loading states and unused `DialogBoxType`. - Update navigation constants for consistency, renaming `PLATFORM_AUTHENTICATOR` to `PLATFORM_AUTHENTICATOR_ROUTE`. - Remove unused `MessageDialogBox` and cleanup background styling in `AuthenticationScreen`.
- Integrate `platformAuthenticator` into the root navigation graph and handle authentication routing based on app lock preferences. - Redesign `AuthenticateButton` (formerly `ClickableTextButton`) using Material3 `Button` with updated styling and simplified parameters. - Add `AuthenticatorBiometricsModule` to Koin dependency injection. - Update `SettingsViewModel` and `SettingsScreen` to conditionally show the "Change Passcode" option only when Mifos Passcode is the active authentication method. - Refactor `AuthenticationScreenViewModel` to remove redundant loading states and unused `DialogBoxType`. - Update navigation constants for consistency, renaming `PLATFORM_AUTHENTICATOR` to `PLATFORM_AUTHENTICATOR_ROUTE`. - Remove unused `MessageDialogBox` and cleanup background styling in `AuthenticationScreen`.
- Replace hardcoded strings with `stringResource` in `AuthOptionCard.kt` and `ChooseAuthOptionScreen.kt` for the `auth` feature. - Externalize UI strings in `SystemAuthenticatorButton.kt` and `AuthenticationScreen.kt` for the `authenticator-biometrics` feature. - Update `PasscodeKeys.kt` in the `mifos-passcode` library to use resource strings for accessibility descriptions. - Add new `strings.xml` resource files and entries across `feature/auth`, `feature/authenticator-biometrics`, and `feature/passcode` modules. - Ensure consistent string management across Android, iOS, and desktop platforms using Compose Resources.
- Update application name from "Mifos App" to "Mifos Pay" in the biometric authentication prompt. - Fix import ordering in `AuthenticationScreen.kt`.
… consistency - Update `switchTabColor` in `MifosPasscode.kt` to use `KptTheme.colorScheme.primary` instead of a hardcoded tint. - Apply `KptTheme.colorScheme.background` to the `AuthenticationScreen` column for consistent UI rendering. - Import `KptTheme` and `background` modifier in `AuthenticationScreen.kt`.
- Remove `onSkipButton` parameter and related logic from `MifosPasscode` and its navigation components. - Remove `toolbarConfig` and unused imports in `MifosPasscode.kt`. - Update `mifosAuthenticatorPasscode` version to `2.0.4` in `libs.versions.toml`. - Update `MifosPayViewModel` to clear authentication options and biometric registration data upon logout. - Inject `PlatformAuthenticationDataRepository` into `MifosPayViewModel` to handle biometric data clearing.
… flows and UI consistency - **feature/auth**: Refactor `ChooseAuthOptionScreenViewmodel` to use a sealed `ScreenState` for error handling instead of separate dialog types. Update `ChooseAuthOptionScreen` to simplify dialog interactions and fix string casing. - **feature/authenticator-biometrics**: Update `SystemAuthenticatorButton` to use specific icons for face scan and keypad authentication. Ensure `AuthenticationScreen` reacts to authenticator status changes via `LaunchedEffect`. - **feature/passcode**: Remove unused `onPasscodeRejected` callback from `MifosPasscode` and its navigation components. - **cmp-shared**: Improve app startup navigation by handling `AppLockOption.None` and ensuring proper logout and navigation to the login graph when no authentication is set. - **core/data**: Change `core.common` dependency to `api` to expose common utilities to dependent modules. - **Resources**: Add and update string resources for biometrics and authentication UI components across features.
- Reorganize dependency tree in `cmp-android`, moving `:core:common` and `:core:datastore` references. - Clean up redundant imports and whitespace in `SystemAuthenticatorButton.kt` and `MifosPayApp.kt`. - Updated various library versions including Kotlin, Coroutines, Coil, and Ktor within the dependency tree.
- Rename `compseResources` to `composeResources` in the `:feature:passcode` module to follow standard naming conventions.
- Update `MifosPasscode` to accept and handle an `onPasscodeRejected` callback. - Refactor `internalPasscodeVerificationScreen` to include `onPasscodeRejected` as a required parameter. - Clean up `RootNavGraph.kt` by removing redundant callback definitions. - Provide a default empty implementation for `MifosPasscode` in the main navigation entry point.
- Updated `USER_EMAIL` from `mifos@mifos.org` to `mifospay@mifos.org` in `ChooseAuthOptionScreenViewmodel.kt`. - Changed `DISPLAY_NAME` from `XYZ` to `Mifos Pay User` for better descriptive consistency.
- Update `mifosAuthenticatorPasscode` to 2.0.5 and `mifosAuthenticatorBiometrics` to 2.0.4 in `libs.versions.toml`. - Adjust Gradle JVM arguments in `gradle.properties`, reducing `Xmx` to 8g and `MaxMetaspaceSize` to 1g. - Add configuration to disable incremental compilation for Kotlin/JS to prevent compiler errors. - Refactor Kotlin/Native settings in `gradle.properties` to improve build stability and manage memory usage. - Minor formatting cleanup in `build.gradle.kts` and `gradle.properties`.
- Introduce a 15-second inactivity timeout that triggers re-authentication when the app returns to the foreground. - Add lifecycle observers in `MifosPayApp` to track background duration using `ON_STOP` and `ON_RESUME` events. - Implement re-authentication navigation routes for both Mifos Passcode (`RE_AUTH_MIFOS_PASSCODE_ROUTE`) and Biometric/Platform Authenticator (`RE_AUTH_PLATFORM_AUTHENTICATOR_ROUTE`). - Update `RootNavGraph` and feature navigation builders to support the new re-authentication screen variants. - Create `LifeCycleAwareAuthentication` class to handle lifecycle-based auth events. - Update `androidxLifecycle` dependency to version `2.9.4` and add `libs.androidx.lifecycle.process` to `cmp-shared`. - Refactor `MifosPayApp` to wrap content with `PlatformAuthenticatorLocalCompositionProvider` for better accessibility to biometric providers.
- Integrate `jb.navigationevent` dependency to handle back events in `MifosPasscode` and `AuthenticationScreen`. - Implement `NavigationBackHandler` in authentication screens to manage back navigation during re-authentication. - Enhance app lock logic in `MifosPayApp` to fetch the current `AppLockOption` dynamically on app resume. - Fix minor formatting, trailing commas, and remove unused imports in `MifosPayViewModel`, `MifosPayApp`, and navigation graphs. - Add license headers to `LifeCycleAwareAuthentication.kt`. - Update dependency configurations and `libs.versions.toml` to include `navigationevent` v1.0.1.
- Delete `LifeCycleAwareAuthentication.kt` from the `cmp-shared` module as it contained only an empty `onStop` implementation.
- Change the base class of `MainActivity` from `FragmentActivity` to `AppCompatActivity`. - Update the corresponding import from `androidx.fragment.app.FragmentActivity` to `androidx.appcompat.app.AppCompatActivity`.
- Reorder `PlatformAuthenticationDataRepository` and `OfficeRepository` in `RepositoryModule.kt`. - Adjust import order for `ExposedDropdownMenuAnchorType` in `AddEditBeneficiaryScreen.kt` and `AddEditSavingAccountScreen.kt` to maintain alphabetical sorting.
TheKalpeshPawar
force-pushed
the
integrate-passcode-module
branch
from
dbc2661 to
53f45b7
Compare
…ctor app lock logic - Consolidate biometric and passcode authentication by moving biometric features into the `feature:passcode` module and removing the standalone `feature:authenticator-biometrics` module. - Introduce `BiometricSetupScreen` to handle user biometric registration during the initial setup or from settings. - Integrate `BiometricsKey` into the `MifosPasscode` screen to allow toggling between passcode and biometric authentication. - Replace `ChooseAuthOptionRepository` with `AppLockRepository` to simplify application locking state management. - Update `MifosPasscodeAdapterImpl` to handle biometric registration data storage via `PasscodeStorageAdapter`. - Refactor `MifosPayApp` and `MifosPayViewModel` to use the new app lock logic and streamline re-authentication navigation. - Enhance `SettingsScreen` to allow users to enable or disable biometric authentication dynamically. - Update `MifosPasscode` and navigation graphs to support unified authentication success callbacks. - Upgrade `mifosAuthenticatorPasscode` dependency to version `2.0.6` and add `MifosAuthenticatorModule` for Koin dependency injection. - Add and reorder various UI strings and resources to support the unified authentication experience.
- Implement `isBiometricEnabled` state check in `BiometricsKey` to conditionally display the biometric button. - Add a dedicated `BIOMETRIC_REGISTERED_KEY` in `MifosPasscodeAdapterImpl` to explicitly track biometric registration status separate from the data itself. - Update `MifosPayApp` and `MifosPayViewModel` to verify passcode existence on app launch and force logout if an authenticated session exists without a created passcode. - Improve `MifosPasscode` lifecycle handling to trigger biometric prompt on resume only when in the "Enter Passcode" step. - Refactor `SettingsViewModel` to reactively update biometric registration status using `PasscodeManager` state. - Update `SettingsScreen` UI to use "Biometrics" terminology instead of "System Authentication". - Refactor `BiometricsSetupScreen` into a stateless content component with an added preview and updated button styling. - Update `mifos-authenticator-passcode` dependency to v2.0.6 and remove unused `:feature:authenticator-biometrics` project dependency from `cmp-android`.
- Update `LifecycleEventObserver` in `MifosPasscode` to only trigger `lockApp()` during `ON_START` if the current step is `PasscodeStep.Enter`. - Remove `AnimatedVisibility` wrapper from `MifosDialogBox` in `MifosPasscode.kt`, relying on internal state for visibility management. - Remove the unused `onPasscodeRejected` parameter from `mifosReAuthPasscodeScreen` navigation builder.
- Add missing space around `==` operator in `LifecycleEventObserver` for better code consistency.
- Replace `mifosRootPasscodeScreen`, `mifosReAuthPasscodeScreen`, and `internalPasscodeVerificationScreen` with a single, reusable `mifosPasscodeScreen` function in `MifosPasscodeNavigation.kt`. - Update `RootNavGraph.kt` to use the new `mifosPasscodeScreen` for both root and re-authentication routes. - Refactor `MifosNavHost.kt` to use `mifosPasscodeScreen` for internal passcode verification, maintaining existing callback logic for authentication success and rejection. - Update `mifosPasscodeScreen` parameters to support optional callbacks for passcode creation, rejection, and change events.
- Replace `mifosRootPasscodeScreen`, `mifosReAuthPasscodeScreen`, and `internalPasscodeVerificationScreen` with a single, reusable `mifosPasscodeScreen` function in `MifosPasscodeNavigation.kt`. - Update `RootNavGraph.kt` to use the new `mifosPasscodeScreen` for both root and re-authentication routes. - Refactor `MifosNavHost.kt` to use `mifosPasscodeScreen` for internal passcode verification, maintaining existing callback logic for authentication success and rejection. - Update `mifosPasscodeScreen` parameters to support optional callbacks for passcode creation, rejection, and change events.
- Replace `mifosRootPasscodeScreen`, `mifosReAuthPasscodeScreen`, and `internalPasscodeVerificationScreen` with a single, reusable `mifosPasscodeScreen` function in `MifosPasscodeNavigation.kt`. - Update `RootNavGraph.kt` to use the new `mifosPasscodeScreen` for both root and re-authentication routes. - Refactor `MifosNavHost.kt` to use `mifosPasscodeScreen` for internal passcode verification, maintaining existing callback logic for authentication success and rejection. - Update `mifosPasscodeScreen` parameters to support optional callbacks for passcode creation, rejection, and change events.
- Introduce `MifosPasscodeViewModel` to handle passcode and biometric authentication logic, state management, and lifecycle events. - Refactor `MifosPasscode.kt` to use the new ViewModel and observe state and events, reducing boilerplate and improving separation of concerns. - Simplify `BiometricsKey` by removing internal state and coroutine handling, delegating authentication clicks to the parent via a callback. - Update `RootNavGraph.kt` to ensure the backstack is correctly popped when navigating from authentication and setup screens. - Add `MifosPasscodeViewModel` to `MifosAuthenticatorModule` in `PasscodeModule.kt`. - Implement `PasscodeDialogState`, `MifosPasscodeAction`, and `MifosPasscodeEvent` for structured communication between the UI and ViewModel.
- Remove unnecessary whitespace in `onSkipBiometricSetup` lambda assignment within `RootNavGraph.kt`.
therajanmaurya
approved these changes
Mar 9, 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
3 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-346
New Impl
mifos-wallet-authenticator.mp4
This pull request introduces the new Mifos Passcode feature and updates the authentication flow to integrate passcode and biometric authentication into the app. It also upgrades several Compose and navigation-related dependencies to newer versions. The most significant changes are grouped below.
Feature Integration and Authentication Flow:
Integrated the new Mifos Authenticator Passcode and Biometrics libraries and feature modules into the project dependencies and DI setup, replacing the previous passcode implementation. (
CMPFeatureConventionPlugin.kt,prodReleaseRuntimeClasspath.txt,build.gradle.kts,KoinModules.kt,MifosPayApp.kt,MifosPayViewModel.kt) [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12]Updated the main authentication flow: users are now required to have a passcode set up after authentication, and the app will log out users if a passcode is missing. The app also locks after a period of inactivity and requires re-authentication. (
MifosPayApp.kt,MifosPayViewModel.kt) [1] [2] [3]Updated navigation logic to use the new passcode routes and navigation functions. (
LoginNavGraph.kt,MifosPayApp.kt) [1] [2]Dependency and Library Upgrades:
prodReleaseRuntimeClasspath.txt) [1] [2] [3] [4] [5]Other Technical Improvements:
MainActivityfromComponentActivitytoAppCompatActivityfor improved compatibility. (MainActivity.kt) [1] [2]These changes collectively modernize the authentication experience, improve security, and ensure the app is using the latest Compose and navigation libraries.