feat(passcode): Implement change passcode flow#1949
feat(passcode): Implement change passcode flow#1949sahilshivekar wants to merge 1 commit intoopenMF:developmentfrom
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:
📝 WalkthroughWalkthroughNavigation and passcode flows were refactored to use an Intention-driven passcode route and arg, consolidating passcode callbacks into a single completion callback, adding a Verify/Create/Confirm step model, tracking skipped-setup state, and threading a root NavHostController through app navigation. Changes
Sequence Diagram(s)sequenceDiagram
participant App as MifosPayApp
participant VM as MifosPayViewModel
participant PM as PasscodeManager
participant Root as RootNavController
participant PassNav as PasscodeNavGraph
participant PVM as PasscodeViewModel
participant Screen as PasscodeScreen
App->>VM: request uiState
VM->>PM: read hasPasscode & hasSkippedPasscodeSetup
PM-->>VM: return flags
VM-->>App: MainUiState.Success(userData, hasPasscode, hasSkippedPasscodeSetup)
alt not authenticated
App->>Root: navigate to LOGIN_GRAPH
else hasPasscode
App->>Root: navigate to passcodeGraphRoute(Intention.LOGIN_WITH_PASSCODE)
else skipped setup
App->>Root: navigate to MAIN_GRAPH
else create flow
App->>Root: navigate to passcodeGraphRoute(Intention.CREATE_PASSCODE)
end
Root->>PassNav: start passcode graph (INTENTION arg)
PassNav->>PVM: initialize with intention
PVM->>Screen: render passcode UI
Screen->>PVM: user input (Verify/Create/Confirm)
PVM->>PM: validate/save as needed
PVM-->>Screen: emit completion
Screen->>PassNav: onPasscodeFlowComplete()
PassNav->>Root: navigateToMainGraph()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayViewModel.kt (1)
27-35: Fix reactivity issue: passcode state changes won't trigger UI updates.Reading
passcodeManager.hasPasscode.valueandhasSkippedPasscodeSetup.valueinside themapblock captures only the current snapshot values. When passcode state changes (e.g., user creates/changes/clears passcode),uiStatewon't update unlessuserInfoalso changes, causing the UI and navigation logic to use stale passcode data.🔎 Proposed fix using combine to merge all flows
- val uiState: StateFlow<MainUiState> = userDataRepository.userInfo.map { - val hasPasscode = passcodeManager.hasPasscode.value - val hasSkippedPasscodeSetup = passcodeManager.hasSkippedPasscodeSetup.value - MainUiState.Success(it, hasPasscode, hasSkippedPasscodeSetup) - }.stateIn( + val uiState: StateFlow<MainUiState> = combine( + userDataRepository.userInfo, + passcodeManager.hasPasscode, + passcodeManager.hasSkippedPasscodeSetup, + ) { userInfo, hasPasscode, hasSkippedPasscodeSetup -> + MainUiState.Success(userInfo, hasPasscode, hasSkippedPasscodeSetup) + }.stateIn( scope = viewModelScope, initialValue = MainUiState.Loading, started = SharingStarted.WhileSubscribed(5_000), )You'll also need to add the import:
import kotlinx.coroutines.flow.combinelibs/mifos-passcode/src/commonMain/kotlin/proto/org/mifos/library/passcode/data/PasscodePreferencesDataSource.kt (1)
68-72:clearInfo()does not resetpasscodeSettingsStateFlow.After removing the key from settings, the
passcodeSettingsMutableStateFlow still holds the old value. This could cause inconsistent state ifclearInfo()is called and then the flows are observed before a new value is written.🔎 Proposed fix
suspend fun clearInfo() { withContext(dispatcher) { settings.remove(PASSCODE_INFO_KEY) + passcodeSettings.value = PasscodePreferencesProto.DEFAULT } }
🧹 Nitpick comments (9)
feature/settings/src/commonMain/kotlin/org/mifospay/feature/settings/SettingsScreen.kt (2)
142-151: Dynamic passcode action label works correctly.The conditional logic cleanly toggles between "Change Passcode" and "Create Passcode" based on state. Note that the action
SettingsAction.ChangePasscodeis used in both cases—consider whether this action name still accurately describes the intent when the user has no passcode (it triggers creation, not change).Optional: Rename action for semantic clarity
If desired, rename the action to something more generic like
ManagePasscodeto better reflect that it handles both creation and change scenarios:- onAction(SettingsAction.ChangePasscode) + onAction(SettingsAction.ManagePasscode)This is purely a naming consideration—the current implementation works correctly.
246-252: Consider adding a preview for thehasPasscode = truestate.The preview only covers the
hasPasscode = falsecase. Adding another preview would help visualize both UI states during development.Add preview for hasPasscode = true
@Preview @Composable private fun SettingsScreenWithPasscodePreview() { SettingsScreenContent( onAction = {}, hasPasscode = true, ) }cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayApp.kt (2)
19-19: Potentially unused import.The
viewModelimport fromandroidx.lifecycle.viewmodel.composeappears unused—the code useskoinViewModelon line 49. If not needed elsewhere, consider removing it.-import androidx.lifecycle.viewmodel.compose.viewModel
83-109: Excellent documentation of the navigation decision tree.The KDoc comment clearly explains each branch of the navigation logic, making the code self-documenting and easier to maintain.
However, the repeated casting of
uiStatetoSuccesson lines 97-100 is verbose. Consider using Kotlin's smart cast with awhensubject:Refactor to avoid repeated casts
val navDestination = when { - uiState is Success -> { - val userData = (uiState as Success).userData - val hasPasscode = (uiState as Success).hasPasscode - val hasSkippedPasscodeSetup = (uiState as Success).hasSkippedPasscodeSetup + uiState is Success -> with(uiState) { when { - !userData.authenticated -> LOGIN_GRAPH + !this.userData.authenticated -> LOGIN_GRAPH hasPasscode -> passcodeGraphRoute(Intention.LOGIN_WITH_PASSCODE) hasSkippedPasscodeSetup -> MAIN_GRAPH else -> passcodeGraphRoute(Intention.CREATE_PASSCODE) } } else -> LOGIN_GRAPH }Or extract to a local variable after the
is Successcheck since smart cast applies.feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/login/LoginScreen.kt (1)
240-243: Hardcoded string should use a string resource for i18n.The text "Don't have an account yet? " is hardcoded. For consistency with the rest of the file (which uses
stringResource), consider moving this to a string resource.libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/ChangePasscodeSuccessDialog.kt (1)
29-47: Hardcoded colors break dark theme support.Using
Color.WhiteandColor.Blackdirectly bypasses Material theming. This will cause poor contrast or visibility issues in dark mode.🔎 Proposed fix using theme colors
AlertDialog( shape = MaterialTheme.shapes.large, - containerColor = Color.White, + containerColor = MaterialTheme.colorScheme.surface, title = { Text( text = stringResource(Res.string.library_mifos_passcode_passcode_changed_message), - color = Color.Black, + color = MaterialTheme.colorScheme.onSurface, ) }, confirmButton = { TextButton(onClick = onDismiss) { Text( text = stringResource(Res.string.library_mifos_passcode_done), - color = Color.Black, + color = MaterialTheme.colorScheme.primary, ) } }, onDismissRequest = onDismiss, )libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/PasscodeHeader.kt (1)
88-89: Themodifierparameter is not applied to the Box.The function accepts a
modifierparameter but usesModifier.fillMaxWidth()directly instead. This prevents callers from customizing the header's layout.🔎 Proposed fix
Box( - modifier = Modifier.fillMaxWidth(), + modifier = modifier.fillMaxWidth(), contentAlignment = Alignment.Center, ) {libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/PassCodeScreen.kt (1)
118-121: Consider repositioning the dialog for semantic clarity.The
ChangePasscodeSuccessDialogis placed within the content layout betweenMifosIconand the passcode inputColumn. While functionally correct (dialogs render as overlays), placing it at the end of theScaffoldcontent or outside theColumnwould be semantically cleaner.libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/viewmodels/PasscodeViewModel.kt (1)
112-117: Consider sending the event after the coroutine completes for consistency.
sendEvent(PasscodeEvent.PasscodeSetupSkipped)is called before the coroutine persisting the skip state completes. If navigation happens immediately, there's a small window where the preference might not be saved.🔎 Proposed fix: emit event inside the coroutine
private fun onSkipPasscodeSetup() { viewModelScope.launch { passcodeRepository.setSkippedPasscodeSetup(true) + sendEvent(PasscodeEvent.PasscodeSetupSkipped) } - sendEvent(PasscodeEvent.PasscodeSetupSkipped) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
cmp-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/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.ktcmp-shared/src/commonMain/kotlin/org/mifospay/shared/ui/MifosApp.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/settings/build.gradle.ktsfeature/settings/src/commonMain/composeResources/values/strings.xmlfeature/settings/src/commonMain/kotlin/org/mifospay/feature/settings/SettingsScreen.ktfeature/settings/src/commonMain/kotlin/org/mifospay/feature/settings/SettingsViewModel.ktlibs/mifos-passcode/src/commonMain/composeResources/values/strings.xmllibs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/PassCodeScreen.ktlibs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/PasscodeNavigation.ktlibs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/ChangePasscodeSuccessDialog.ktlibs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/PasscodeButton.ktlibs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/PasscodeHeader.ktlibs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/PasscodeStepIndicator.ktlibs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/utility/Step.ktlibs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/viewmodels/PasscodeViewModel.ktlibs/mifos-passcode/src/commonMain/kotlin/proto/org/mifos/library/passcode/data/PasscodeManager.ktlibs/mifos-passcode/src/commonMain/kotlin/proto/org/mifos/library/passcode/data/PasscodePreferencesDataSource.ktlibs/mifos-passcode/src/commonMain/kotlin/proto/org/mifos/library/passcode/di/PreferenceModule.ktlibs/mifos-passcode/src/commonMain/kotlin/proto/org/mifos/library/passcode/model/PasscodePreferencesProto.kt
💤 Files with no reviewable changes (1)
- cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/LoginNavGraph.kt
🧰 Additional context used
🧬 Code graph analysis (4)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayApp.kt (1)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/MifosNavGraph.kt (1)
passcodeGraphRoute(31-33)
libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/PassCodeScreen.kt (1)
libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/ChangePasscodeSuccessDialog.kt (1)
ChangePasscodeSuccessDialog(23-49)
libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/viewmodels/PasscodeViewModel.kt (1)
core-base/ui/src/commonMain/kotlin/template/core/base/ui/BaseViewModel.kt (1)
sendEvent(97-99)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/PasscodeNavGraph.kt (1)
libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/PasscodeNavigation.kt (1)
passcodeRoute(47-67)
🔇 Additional comments (33)
feature/settings/build.gradle.kts (1)
26-26: Dependency addition is appropriate.Adding
mifosPasscodeenables the Settings feature to access passcode state (hasPasscode) and navigation utilities, which aligns with the dynamic "Create Passcode" / "Change Passcode" UI requirement.feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/login/LoginViewModel.kt (1)
139-149: Good documentation of the architectural change.The comment clearly explains why
NavigateToPasscodeScreenwas removed and where that responsibility now lives. This decoupling allows app-level navigation to make routing decisions based on both authentication state and passcode status, which is cleaner than having the login screen handle passcode navigation directly.feature/settings/src/commonMain/composeResources/values/strings.xml (1)
18-18: String resource follows conventions.The new
feature_settings_create_passcodestring is appropriately named and placed alongside the existingfeature_settings_change_passcode, supporting the dynamic UI that toggles between these based on passcode state.libs/mifos-passcode/src/commonMain/composeResources/values/strings.xml (1)
21-24: String resources support the change passcode flow.The new strings appropriately support the success dialog and the "Create new Passcode" step in the change flow. They follow the
library_mifos_passcode_naming convention used by other recent strings in this file.feature/settings/src/commonMain/kotlin/org/mifospay/feature/settings/SettingsScreen.kt (1)
67-68: Proper lifecycle-aware state observation.Using
collectAsStateWithLifecycle()for the passcode state ensures the UI updates reactively when the passcode status changes, which is especially important when returning from the passcode creation flow.libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/PasscodeNavigation.kt (3)
32-42: Intention enum is well-designed with clear documentation.The KDoc clearly explains each intention's purpose and flow. The
fromValuehelper with a default fallback is practical for handling unexpected route arguments.One consideration: the default fallback to
CREATE_PASSCODEon unknown values means an attacker or bug causing an invalid intention would route users to passcode creation. Verify this is the safest default—LOGIN_WITH_PASSCODEmight be more conservative since it requires verification of an existing passcode.
47-67: Route definition correctly uses optional query parameter.The route pattern
$PASSCODE_SCREEN?$INTENTION={$INTENTION}with a default value ensures backward compatibility. The singleonPasscodeFlowCompletecallback simplifies the API compared to separate success/failure callbacks—this works because the ViewModel now handles rejection internally with retry logic.
69-72: Navigation helper correctly constructs the parameterized route.The function properly encodes the intention value into the route string. The optional
NavOptionsparameter provides flexibility for callers.cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/MifosNavHost.kt (2)
94-94: Correct use of separate NavController for passcode navigation.Passing
rootNavControlleras a separate parameter enables navigation to the passcode screen which exists outside the main navigation graph. This is the right architectural approach.
187-198: Correct the inverted logic in the comment.The comment states the ViewModel changes intention "if there is already a passcode set," but the actual code (PasscodeViewModel.kt, line 70) checks
!state.hasPasscode, meaning it activates when NO passcode exists. Additionally, the ViewModel changes theactiveStep, not the intention itself. Update the comment to reflect: the ViewModel starts at the Create step directly (skipping Verify) when the user navigates to "Change Passcode" but never set up a passcode initially.feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/navigation/LoginScreenNavigation.kt (1)
21-28: LGTM! Navigation refactoring is well-documented.The removal of
onNavigateToPasscodeScreenand the explanatory comment clearly communicate that passcode navigation is now handled at the app level, aligning with the PR's intention-based navigation approach.libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/PasscodeButton.kt (2)
29-58: LGTM! Intent-based rendering improves clarity.The shift from boolean-based to intention-based gating makes the button visibility logic more explicit and maintainable. The documentation clearly explains when the skip button appears.
60-89: LGTM! Consistent with skip button refactoring.The forgot button correctly appears only during login, with clear documentation explaining the behavior.
feature/settings/src/commonMain/kotlin/org/mifospay/feature/settings/SettingsViewModel.kt (1)
44-53: LGTM! Reactive passcode state exposure.Exposing
hasPasscodeas aStateFlowenables the Settings UI to reactively display "Create Passcode" or "Change Passcode" based on the current passcode state. The comprehensive documentation clearly explains the purpose.libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/PasscodeStepIndicator.kt (2)
29-72: LGTM! Step indicator logic correctly implements single-step highlighting.The changes appropriately:
- Hide the indicator during the Verify step (when no creation progress needs to be shown)
- Highlight only the current step in the 2-step creation flow (Create → Confirm)
- Provide clear documentation explaining the behavior
The active step calculation
step + 1 == activeStep.indexcorrectly highlights step 0 during Create(1) and step 1 during Confirm(2).
74-78: LGTM! Preview function aids development.libs/mifos-passcode/src/commonMain/kotlin/proto/org/mifos/library/passcode/model/PasscodePreferencesProto.kt (1)
14-31: LGTM! Skip tracking prevents repeated prompts.The
hasSkippedPasscodeSetupfield appropriately distinguishes between users who never encountered passcode setup and those who explicitly declined, preventing unwanted re-prompting. The documentation clearly explains this distinction.cmp-shared/src/commonMain/kotlin/org/mifospay/shared/ui/MifosApp.kt (1)
69-74: LGTM! Root controller enables app-level navigation.Adding the
rootNavControllerparameter allowsMifosAppto coordinate navigation at the application level, which is essential for the intention-based passcode routing introduced in this PR.libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/utility/Step.kt (1)
12-19: LGTM! Three-step flow supports verification.Adding the
Verifystep appropriately supports both login verification and the "verify old passcode" step in the change flow. The index reordering is correct.feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/login/LoginScreen.kt (1)
57-70: LGTM on the navigation refactor!The docblock clearly explains the architectural decision to separate passcode navigation from the login flow. This is a good separation of concerns that prevents duplicate navigation calls.
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/RootNavGraph.kt (1)
40-46: LGTM!Passing
rootNavControllertoMifosAppenables proper navigation for the change passcode flow from within the main graph back to the passcode graph.libs/mifos-passcode/src/commonMain/kotlin/proto/org/mifos/library/passcode/data/PasscodeManager.kt (1)
43-61: LGTM on the skip-passcode tracking!The implementation correctly:
- Exposes
hasSkippedPasscodeSetupas a reactive StateFlow- Resets the skip flag when a passcode is saved (line 54)
- Provides
setSkippedPasscodeSetupfor explicit updatesThis ensures consistent state management across the passcode flows.
libs/mifos-passcode/src/commonMain/kotlin/proto/org/mifos/library/passcode/data/PasscodePreferencesDataSource.kt (1)
46-66: LGTM on the skip-passcode tracking implementation!The
hasSkippedPasscodeSetupflow andupdateSkippedPasscodeSetupfunction are correctly implemented with proper dispatcher context switching for thread safety.cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/MifosNavGraph.kt (1)
19-33: LGTM on the intention-based routing!The implementation correctly:
- Uses query parameter syntax for the intention argument
- Provides a clean helper function to build routes with specific intentions
- Includes clear documentation explaining the purpose
libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/PasscodeHeader.kt (1)
92-127: LGTM on the intention-based header logic!The
whenexpression cleanly handles all three steps with appropriate text based on the intention. The distinction between "Create Passcode" (initial) and "Create New Passcode" (change flow) provides clear UX differentiation.libs/mifos-passcode/src/commonMain/kotlin/proto/org/mifos/library/passcode/di/PreferenceModule.kt (1)
30-33: LGTM on the singleton scope change!Switching from
factorytosingleis the correct fix. Withfactory, each injection site would get a new instance with its own StateFlows, breaking reactivity across the app. The IO dispatcher is appropriate for preferences I/O operations.cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/PasscodeNavGraph.kt (2)
22-37: LGTM! Clear documentation and correct route construction.The documentation block clearly explains the three intention modes, and the start destination with the intention placeholder correctly inherits the argument from the graph route when navigating to the passcode graph.
39-52: LGTM! Clean callback consolidation.The unified
onPasscodeFlowCompletecallback simplifies the API surface. The navigation logic foronForgotButton(to login) andonSkipButton/onPasscodeFlowComplete(to main graph) correctly reflects the intended user flows.libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/PassCodeScreen.kt (2)
79-93: LGTM! Clean event-to-callback mapping.The
EventsEffectcorrectly maps the three event types to appropriate callbacks.PasscodeSetupSkippedtriggeringonSkipButton()maintains consistency with the skip flow.
109-114: LGTM! Skip button correctly dispatches to ViewModel.Delegating the skip action to the ViewModel via
PasscodeAction.SkipPasscodeSetupallows proper persistence of the skip state before navigation occurs.libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/viewmodels/PasscodeViewModel.kt (3)
119-142: LGTM! Clean step-based buffer selection.The refactored
enterKeycorrectly selects the appropriate buffer based onactiveStepand triggers processing when the passcode is complete.
203-219: LGTM! Well-structured flow routing.The
processCompletedPasscodefunction clearly handles all intention/step combinations with appropriate transitions. The documentation accurately describes the flow logic.
330-339: LGTM! State class is well-documented.The
PasscodeStatedata class with the newintentionandisChangePasscodeSuccessfulproperties supports all flow variations cleanly.
sahilshivekar
force-pushed
the
feature/mw-324-change-passcode
branch
from
1c37870 to
468aa86
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayViewModel.kt (1)
27-35: Non-reactive flow composition breaks real-time passcode updates.The
uiStateflow only reacts to changes inuserInfobecause it usesmapon that single flow. Lines 28-29 read passcode state via.value, which captures a snapshot but doesn't create a reactive subscription. When passcode state changes (e.g., user creates/changes passcode), the UI won't update unlessuserInfoalso happens to change.This violates the PR requirement: "The UI observes passcode state in real time for reactive updates."
🔎 Proposed fix using combine() for reactive composition
- val uiState: StateFlow<MainUiState> = userDataRepository.userInfo.map { - val hasPasscode = passcodeManager.hasPasscode.value - val hasSkippedPasscodeSetup = passcodeManager.hasSkippedPasscodeSetup.value - MainUiState.Success(it, hasPasscode, hasSkippedPasscodeSetup) + val uiState: StateFlow<MainUiState> = combine( + userDataRepository.userInfo, + passcodeManager.hasPasscode, + passcodeManager.hasSkippedPasscodeSetup + ) { userInfo, hasPasscode, hasSkippedPasscodeSetup -> + MainUiState.Success(userInfo, hasPasscode, hasSkippedPasscodeSetup) }.stateIn( scope = viewModelScope, initialValue = MainUiState.Loading,Don't forget to import
combine:+import kotlinx.coroutines.flow.combine
♻️ Duplicate comments (3)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayApp.kt (1)
96-109: Address the reactivestartDestinationissue flagged in the previous review.This code still uses a reactive
startDestinationwhich was flagged as problematic in the previous review. The NavHost'sstartDestinationshould be fixed (e.g.,LOGIN_GRAPH), and navigation decisions should be handled viaLaunchedEffectwithnavController.navigate(). This prevents NavHost graph re-creation and back-stack issues.Based on the previous review, the correct pattern is:
- Set
startDestination = LOGIN_GRAPH(fixed value)- Add a
LaunchedEffect(uiState)that watches state changes- Programmatically navigate using
navController.navigate(target)with properpopUpTohandling- Follow the authorization handling pattern already used in lines 57-61
libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/viewmodels/PasscodeViewModel.kt (2)
40-78: Race condition:hasPasscodeuninitialized when checking.The
getIntention()method runs synchronously ininitand checksstate.hasPasscodeon line 70. However,observePasscodeRepository()launches an asynchronous coroutine to collect thehasPasscodeflow. When line 70 executes,state.hasPasscodewill still befalse(the default) because the flow hasn't emitted yet.Impact: For
CHANGE_PASSCODEintention, the condition!state.hasPasscodewill incorrectly evaluate totrueeven when a passcode exists, causing the Verify step to be skipped and allowing passcode changes without verification.🔎 Proposed fix: await hasPasscode before checking
init { - observePasscodeRepository() - getIntention() + viewModelScope.launch { + // Wait for hasPasscode to be loaded before determining flow + val hasPasscode = passcodeRepository.hasPasscode.first() + mutableStateFlow.update { it.copy(hasPasscode = hasPasscode) } + getIntention() + } + observePasscodeRepository() }
225-228: Bug: Should setisChangePasscodeSuccessfultofalsewhen dismissing.Line 226 sets
isChangePasscodeSuccessful = true, but this is redundant—the flag is alreadytrue(which is why the dialog is visible). The dismiss action should reset it tofalseto clear the success state.🔎 Proposed fix
private fun onDismissChangePasscodeSuccessDialog() { - mutableStateFlow.update { it.copy(isChangePasscodeSuccessful = true) } + mutableStateFlow.update { it.copy(isChangePasscodeSuccessful = false) } sendEvent(PasscodeEvent.PasscodeConfirmed) }
🧹 Nitpick comments (3)
libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/ChangePasscodeSuccessDialog.kt (1)
29-47: Consider using MaterialTheme.colorScheme instead of hardcoded colors.The dialog uses hardcoded
Color.WhiteandColor.Blackwhich won't respect theming or dark mode preferences. Consider using theme-aware colors.🔎 Proposed refactor to use theme colors
AlertDialog( shape = MaterialTheme.shapes.large, - containerColor = Color.White, + containerColor = MaterialTheme.colorScheme.surface, title = { Text( text = stringResource(Res.string.library_mifos_passcode_passcode_changed_message), - color = Color.Black, + color = MaterialTheme.colorScheme.onSurface, ) }, confirmButton = { TextButton(onClick = onDismiss) { Text( text = stringResource(Res.string.library_mifos_passcode_done), - color = Color.Black, + color = MaterialTheme.colorScheme.primary, ) } }, onDismissRequest = onDismiss, )libs/mifos-passcode/src/commonMain/kotlin/proto/org/mifos/library/passcode/data/PasscodeManager.kt (1)
43-47: Consider usingSharingStarted.WhileSubscribed()instead ofEagerly.The
hasSkippedPasscodeSetupStateFlow usesSharingStarted.Eagerly, which keeps the flow active even when there are no collectors. Since this is a UI-related state,SharingStarted.WhileSubscribed(5000)would be more appropriate—it stops collection after 5 seconds of no subscribers, reducing unnecessary work.🔎 Proposed refactor
val hasSkippedPasscodeSetup = source.hasSkippedPasscodeSetup.stateIn( scope = coroutineScope, - started = SharingStarted.Eagerly, + started = SharingStarted.WhileSubscribed(5000), initialValue = false, )libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/utility/Step.kt (1)
12-14: Consider documenting all enum values for consistency.The KDoc currently only documents the
Verifystep. While the explanation forVerifyis helpful, adding brief descriptions forCreateandConfirmwould improve documentation completeness and maintainability.📝 Suggested documentation improvement
/** + * Represents the steps in the passcode flow. + * * @property Verify Verifying existing passcode (used in LOGIN and CHANGE intentions) + * @property Create Creating a new passcode (used in CREATE and CHANGE intentions) + * @property Confirm Confirming the newly created passcode */ enum class Step(var index: Int) {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
cmp-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/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.ktcmp-shared/src/commonMain/kotlin/org/mifospay/shared/ui/MifosApp.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/settings/build.gradle.ktsfeature/settings/src/commonMain/composeResources/values/strings.xmlfeature/settings/src/commonMain/kotlin/org/mifospay/feature/settings/SettingsScreen.ktfeature/settings/src/commonMain/kotlin/org/mifospay/feature/settings/SettingsViewModel.ktlibs/mifos-passcode/src/commonMain/composeResources/values/strings.xmllibs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/PassCodeScreen.ktlibs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/PasscodeNavigation.ktlibs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/ChangePasscodeSuccessDialog.ktlibs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/PasscodeButton.ktlibs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/PasscodeHeader.ktlibs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/PasscodeStepIndicator.ktlibs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/utility/Step.ktlibs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/viewmodels/PasscodeViewModel.ktlibs/mifos-passcode/src/commonMain/kotlin/proto/org/mifos/library/passcode/data/PasscodeManager.ktlibs/mifos-passcode/src/commonMain/kotlin/proto/org/mifos/library/passcode/data/PasscodePreferencesDataSource.ktlibs/mifos-passcode/src/commonMain/kotlin/proto/org/mifos/library/passcode/di/PreferenceModule.ktlibs/mifos-passcode/src/commonMain/kotlin/proto/org/mifos/library/passcode/model/PasscodePreferencesProto.kt
💤 Files with no reviewable changes (1)
- cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/LoginNavGraph.kt
🚧 Files skipped from review as they are similar to previous changes (9)
- libs/mifos-passcode/src/commonMain/kotlin/proto/org/mifos/library/passcode/data/PasscodePreferencesDataSource.kt
- libs/mifos-passcode/src/commonMain/kotlin/proto/org/mifos/library/passcode/model/PasscodePreferencesProto.kt
- cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/RootNavGraph.kt
- libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/PasscodeStepIndicator.kt
- feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/navigation/LoginScreenNavigation.kt
- libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/PasscodeHeader.kt
- libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/PasscodeNavigation.kt
- cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/MifosNavHost.kt
- feature/settings/src/commonMain/composeResources/values/strings.xml
🧰 Additional context used
🧬 Code graph analysis (4)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayApp.kt (1)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/MifosNavGraph.kt (1)
passcodeGraphRoute(31-33)
libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/viewmodels/PasscodeViewModel.kt (1)
core-base/ui/src/commonMain/kotlin/template/core/base/ui/BaseViewModel.kt (1)
sendEvent(97-99)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/PasscodeNavGraph.kt (1)
libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/PasscodeNavigation.kt (1)
passcodeRoute(47-67)
libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/PassCodeScreen.kt (1)
libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/ChangePasscodeSuccessDialog.kt (1)
ChangePasscodeSuccessDialog(23-49)
🔇 Additional comments (29)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayViewModel.kt (2)
37-42: LGTM!The logout logic correctly clears both user data and passcode state in the appropriate scope.
47-51: LGTM!The expanded
Successstate structure cleanly carries user and passcode metadata, aligning with the PR's intention-driven passcode flow requirements.feature/settings/build.gradle.kts (1)
26-26: LGTM! The addition ofmifosPasscodedependency is correct and enables the settings module to interact with passcode components for the "Create/Change Passcode" functionality.The library exists with proper structure, has no circular dependencies, and is correctly referenced in the commonMain scope for cross-platform usage.
libs/mifos-passcode/src/commonMain/composeResources/values/strings.xml (1)
21-24: LGTM!The new string resources appropriately support the change passcode flow and success dialog.
libs/mifos-passcode/src/commonMain/kotlin/proto/org/mifos/library/passcode/di/PreferenceModule.kt (1)
19-34: LGTM!The change from
factorytosingleis essential for maintaining reactive flows across multiple screens. The documentation clearly explains the rationale—usingfactorywould create new instances with separate flows, breaking reactivity. The switch to IO dispatcher is also more appropriate for preference persistence operations.cmp-shared/src/commonMain/kotlin/org/mifospay/shared/ui/MifosApp.kt (2)
73-73: LGTM!The addition of the
rootNavControllerparameter enables proper navigation wiring at the app level, supporting the intention-based passcode navigation flow.
163-163: LGTM!Correctly forwards the
rootNavControllertoMifosNavHostfor navigation handling.feature/settings/src/commonMain/kotlin/org/mifospay/feature/settings/SettingsViewModel.kt (1)
44-53: LGTM!The
hasPasscodeproperty exposure enables reactive Settings UI updates (showing "Change Passcode" vs "Create Passcode"). The documentation clearly explains the use case, and it correctly leverages the singletonPasscodeManagerto maintain consistent state.libs/mifos-passcode/src/commonMain/kotlin/proto/org/mifos/library/passcode/data/PasscodeManager.kt (2)
49-57: LGTM!Correctly resets
hasSkippedPasscodeSetuptofalsewhen saving a passcode, ensuring the skip flag is cleared when the user actively sets a passcode.
59-61: LGTM!The
setSkippedPasscodeSetupfunction provides a clean interface for updating the skip flag.cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/MifosNavGraph.kt (1)
19-33: LGTM!The refactoring from a fixed
PASSCODE_GRAPHconstant to an intention-driven routing system is well-designed. The templated route and builder function enable multiple passcode flows (LOGIN_WITH_PASSCODE, CREATE_PASSCODE, CHANGE_PASSCODE) while maintaining clean navigation semantics.libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/PasscodeButton.kt (2)
29-58: LGTM! Clean refactor to intention-based visibility.The shift from boolean flags to
Intentionenum provides clearer semantics and better maintainability. The KDoc accurately describes when the Skip button appears.
60-89: LGTM! Correct intention-based gating.The Forgot button correctly appears only during login flows. The KDoc clearly communicates this behavior.
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/PasscodeNavGraph.kt (3)
22-37: LGTM! Intention-based routing properly configured.The start destination correctly includes the intention placeholder, allowing the graph route to pass intention through to the passcode screen. This aligns with the
passcodeRoutedefinition in PasscodeNavigation.kt.
40-47: LGTM! Correct navigation for forgot passcode.The change to navigate to login screen (instead of main graph) when user forgets their passcode makes sense - it provides a fallback authentication path.
48-51: LGTM! Unified callback simplifies API.Consolidating
onPasscodeConfirmandonPasscodeRejectedinto a singleonPasscodeFlowCompletecallback simplifies the navigation surface and aligns with the intention-driven flow where the screen internally handles different outcomes.libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/PassCodeScreen.kt (4)
69-82: LGTM! Unified callback correctly wired.The event handling correctly maps
PasscodeConfirmedto the unifiedonPasscodeFlowCompletecallback, simplifying the public API.
92-92: LGTM! Skip event properly handled.The
PasscodeSetupSkippedevent correctly triggers theonSkipButtoncallback, completing the skip flow.
109-114: LGTM! Skip button correctly integrated.The button now uses intention-based gating and triggers the ViewModel action, properly encapsulating the skip logic within the ViewModel.
118-121: LGTM! Success dialog properly integrated.The
ChangePasscodeSuccessDialogis correctly wired to state and triggers the dismiss action through the ViewModel.libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/viewmodels/PasscodeViewModel.kt (3)
122-142: LGTM! Per-step buffer selection correctly implemented.The
enterKeymethod correctly selects the appropriate buffer (verifyPasscode,createPasscode, orconfirmPasscode) based onactiveStep, ensuring inputs are kept separate across the multi-step flow.
203-218: LGTM! Intention-driven flow logic is sound.The
processCompletedPasscodemethod correctly orchestrates the passcode flow based on intention and step, handling all three flows (CREATE, LOGIN, CHANGE) appropriately.
240-253: LGTM! Dual-purpose validation correctly implemented.The validation logic correctly uses the
verifyPasscodebuffer and branches appropriately: for CHANGE_PASSCODE it advances to Create step, while for LOGIN_WITH_PASSCODE it completes the flow.feature/settings/src/commonMain/kotlin/org/mifospay/feature/settings/SettingsScreen.kt (4)
34-34: Required resource import added correctly.The import for
feature_settings_create_passcodeis necessary for the dynamic button title feature.
142-151: Excellent UX: Dynamic button title based on passcode state.The conditional title ("Change Passcode" vs "Create Passcode") provides clear guidance to users based on their current passcode configuration. The comment effectively explains the feature's purpose.
251-251: Preview correctly updated with new parameter.The preview properly supplies the
hasPasscodeparameter with a sensible default value.
67-68: Good reactive state observation for dynamic UI updates.Collecting
hasPasscodeas state enables the UI to reactively update when passcode status changes. The SettingsViewModel properly exposeshasPasscodeas a StateFlow, and the implementation correctly usescollectAsStateWithLifecycle()for lifecycle-aware state observation.feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/login/LoginScreen.kt (1)
57-70: Excellent documentation of the architectural refactor and bug fix.The documentation clearly explains:
- The bug that was fixed (double navigation to passcode graph)
- The root cause (redundant navigation calls)
- The new responsibility separation
This is verified by the app-level navigation logic in
MifosPayApp.kt(lines 57-95), which centrally determines the post-login destination based on authentication and passcode state. Only a singlenavigateToPasscodeScreencall remains in the codebase (in settings for theCHANGE_PASSCODEflow), confirming the double-navigation issue is fully resolved and LoginScreen no longer handles passcode navigation.feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/login/LoginViewModel.kt (1)
139-149: Documentation clearly explains the architectural change.The app-level navigation properly handles all passcode routing scenarios. Authentication state changes are observed in MifosPayViewModel, which determines the navigation destination based on authentication status, existing passcode, and skip preferences. Post-login passcode verification (LOGIN_WITH_PASSCODE), first-time setup (CREATE_PASSCODE), and change flows (CHANGE_PASSCODE) are all correctly routed through the PasscodeNavGraph.
sahilshivekar
force-pushed
the
feature/mw-324-change-passcode
branch
from
a13601d to
c74d216
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayViewModel.kt (1)
27-35: Fix reactivity issue: passcode state changes won't update UI.Using
.valueon StateFlows inside themapblock captures a snapshot at the timeuserInfoemits, but won't react to independent changes in passcode state. If a user creates or changes their passcode via Settings, the UI won't reflect this untiluserInfoemits again for an unrelated reason.Use
combine()to properly observe all three flows together for reactive updates.🔎 Proposed fix using combine for reactive state
- val uiState: StateFlow<MainUiState> = userDataRepository.userInfo.map { - val hasPasscode = passcodeManager.hasPasscode.value - val hasSkippedPasscodeSetup = passcodeManager.hasSkippedPasscodeSetup.value - MainUiState.Success(it, hasPasscode, hasSkippedPasscodeSetup) - }.stateIn( + val uiState: StateFlow<MainUiState> = combine( + userDataRepository.userInfo, + passcodeManager.hasPasscode, + passcodeManager.hasSkippedPasscodeSetup + ) { userInfo, hasPasscode, hasSkippedPasscodeSetup -> + MainUiState.Success(userInfo, hasPasscode, hasSkippedPasscodeSetup) + }.stateIn( scope = viewModelScope, initialValue = MainUiState.Loading, started = SharingStarted.WhileSubscribed(5_000),Don't forget to add the import:
import kotlinx.coroutines.flow.combine
♻️ Duplicate comments (3)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayApp.kt (1)
96-109: Critical: ReactivestartDestinationcauses NavHost re-creation and back-stack issues.This is the same issue previously flagged. Computing
navDestinationreactively and using it asstartDestination(line 116) causes Compose Navigation to re-create the NavHost graph wheneveruiStatechanges, leading to back-stack anomalies and navigation glitches.Solution: Use a fixed
startDestination(e.g.,LOGIN_GRAPH) and add aLaunchedEffectthat watchesuiStateto programmatically navigate usingnavController.navigate()with properpopUpTohandling. The unauthorized handling pattern (lines 57-61) demonstrates the correct approach.🔎 Recommended fix pattern
- /** - * Determines the initial navigation destination based on authentication and passcode state. - * - * Navigation decision tree: - * 1. If still loading -> LOGIN_GRAPH (temporary until we know auth state) - * 2. If not authenticated -> LOGIN_GRAPH - * 3. If authenticated AND has passcode -> PASSCODE_GRAPH with LOGIN_WITH_PASSCODE intention - * (user needs to verify their passcode to access the app) - * 4. If authenticated AND skipped passcode setup -> MAIN_GRAPH - * (user previously chose to skip, respect that choice) - * 5. If authenticated but no passcode and not skipped -> PASSCODE_GRAPH with CREATE_PASSCODE intention - * (first-time setup, user can create or skip) - */ - val navDestination = when { - uiState is Success -> { - val userData = (uiState as Success).userData - val hasPasscode = (uiState as Success).hasPasscode - val hasSkippedPasscodeSetup = (uiState as Success).hasSkippedPasscodeSetup - when { - !userData.authenticated -> LOGIN_GRAPH - hasPasscode -> passcodeGraphRoute(Intention.LOGIN_WITH_PASSCODE) - hasSkippedPasscodeSetup -> MAIN_GRAPH - else -> passcodeGraphRoute(Intention.CREATE_PASSCODE) - } - } - else -> LOGIN_GRAPH - } + LaunchedEffect(uiState) { + if (uiState is Success) { + val successState = uiState as Success + val userData = successState.userData + val hasPasscode = successState.hasPasscode + val hasSkippedPasscodeSetup = successState.hasSkippedPasscodeSetup + + val targetDestination = when { + !userData.authenticated -> LOGIN_GRAPH + hasPasscode -> passcodeGraphRoute(Intention.LOGIN_WITH_PASSCODE) + hasSkippedPasscodeSetup -> MAIN_GRAPH + else -> passcodeGraphRoute(Intention.CREATE_PASSCODE) + } + + navController.navigate(targetDestination) { + popUpTo(navController.graph.id) { + inclusive = true + } + launchSingleTop = true + } + } + } MifosTheme { RootNavGraph( networkMonitor = networkMonitor, timeZoneMonitor = timeZoneMonitor, navHostController = navController, - startDestination = navDestination, + startDestination = LOGIN_GRAPH, modifier = modifier,Based on learnings from past review and web search results on Compose Navigation best practices.
Also applies to: 116-116
libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/viewmodels/PasscodeViewModel.kt (2)
40-43: Race condition:hasPasscodemay not be initialized whengetIntention()checks it.This issue was flagged in a previous review and remains unaddressed.
observePasscodeRepository()launches an async coroutine to collecthasPasscode, butgetIntention()runs synchronously immediately after. When line 70 checks!state.hasPasscode, the value will still befalse(the default) because the flow hasn't emitted yet.Impact: For
CHANGE_PASSCODEintention, the condition!state.hasPasscodewill betrueeven when the user has a passcode, causing the Verify step to be incorrectly skipped—allowing passcode change without verification.🔎 Proposed fix: await hasPasscode before determining flow
init { observePasscodeRepository() - getIntention() + viewModelScope.launch { + // Wait for hasPasscode to be loaded before determining flow + val hasPasscode = passcodeRepository.hasPasscode.first() + mutableStateFlow.update { it.copy(hasPasscode = hasPasscode) } + getIntention() + } }
225-228: Bug: Should setisChangePasscodeSuccessfultofalsewhen dismissing.This issue was flagged in a previous review and remains unaddressed. Setting
isChangePasscodeSuccessful = trueon line 226 is incorrect—it's alreadytrue(which is why the dialog was visible). This should be set tofalseto properly reset the state.🔎 Proposed fix
private fun onDismissChangePasscodeSuccessDialog() { - mutableStateFlow.update { it.copy(isChangePasscodeSuccessful = true) } + mutableStateFlow.update { it.copy(isChangePasscodeSuccessful = false) } sendEvent(PasscodeEvent.PasscodeConfirmed) }
🧹 Nitpick comments (5)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayApp.kt (1)
98-100: Consider extracting the Success cast to reduce repetition.The code casts
uiStatetoSuccessthree times (lines 98-100). Extracting this to a local variable would improve readability and reduce redundancy.🔎 Suggested refactor
val navDestination = when { uiState is Success -> { - val userData = (uiState as Success).userData - val hasPasscode = (uiState as Success).hasPasscode - val hasSkippedPasscodeSetup = (uiState as Success).hasSkippedPasscodeSetup + val successState = uiState as Success + val userData = successState.userData + val hasPasscode = successState.hasPasscode + val hasSkippedPasscodeSetup = successState.hasSkippedPasscodeSetup when { !userData.authenticated -> LOGIN_GRAPHNote: This refactor would be unnecessary if the reactive
startDestinationis replaced with theLaunchedEffectpattern as recommended above.libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/PasscodeStepIndicator.kt (1)
74-78: Preview function should be private.The
PasscodeStepIndicatorPreviewfunction is markedpublic(default visibility), but the component it previews isinternal. Preview functions for internal components should also be private to avoid polluting the public API surface.🔎 Proposed fix
@Preview @Composable -fun PasscodeStepIndicatorPreview() { +private fun PasscodeStepIndicatorPreview() { PasscodeStepIndicator(activeStep = Step.Confirm) }libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/PasscodeNavigation.kt (1)
38-41: Inconsistent default intention betweenfromValueand route argument.
Intention.fromValue()defaults toCREATE_PASSCODE(line 40), but thenavArgumentdefault isLOGIN_WITH_PASSCODE(line 56). This inconsistency could cause confusion iffromValueis called with an unknown string—it would returnCREATE_PASSCODEinstead of the expectedLOGIN_WITH_PASSCODE.Consider aligning the defaults:
🔎 Proposed fix
companion object { fun fromValue(value: String): Intention = - Intention.entries.firstOrNull { it.value == value } ?: CREATE_PASSCODE + Intention.entries.firstOrNull { it.value == value } ?: LOGIN_WITH_PASSCODE }libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/PassCodeScreen.kt (1)
118-121: Dialog placement within Column may cause layout shifts.
ChangePasscodeSuccessDialogis placed inside the mainColumnlayout. WhileAlertDialogtypically renders as a popup overlay, placing it inline within the column structure is unconventional and could cause unexpected behavior on some platforms.Consider moving the dialog outside the
Column, either after theScaffoldcontent or as a sibling to theScaffold:🔎 Suggested placement
Scaffold(...) { paddingValues -> Column(...) { // ... column content without dialog } } ChangePasscodeSuccessDialog( visible = state.isChangePasscodeSuccessful, onDismiss = { viewModel.trySendAction(PasscodeAction.DismissChangePasscodeSuccessDialog) }, )feature/settings/src/commonMain/kotlin/org/mifospay/feature/settings/SettingsScreen.kt (1)
248-253: Consider adding a preview for the alternative state.The current preview tests
hasPasscode = false(showing "Create Passcode"). Adding a second preview withhasPasscode = truewould improve visual testing coverage for both UI states.💡 Suggested additional preview
@Preview @Composable private fun SettingsScreenPreviewWithPasscode() { SettingsScreenContent( onAction = {}, hasPasscode = true, ) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
cmp-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/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.ktcmp-shared/src/commonMain/kotlin/org/mifospay/shared/ui/MifosApp.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/settings/build.gradle.ktsfeature/settings/src/commonMain/composeResources/values/strings.xmlfeature/settings/src/commonMain/kotlin/org/mifospay/feature/settings/SettingsScreen.ktfeature/settings/src/commonMain/kotlin/org/mifospay/feature/settings/SettingsViewModel.ktlibs/mifos-passcode/src/commonMain/composeResources/values/strings.xmllibs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/PassCodeScreen.ktlibs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/PasscodeNavigation.ktlibs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/ChangePasscodeSuccessDialog.ktlibs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/PasscodeButton.ktlibs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/PasscodeHeader.ktlibs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/PasscodeStepIndicator.ktlibs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/utility/Step.ktlibs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/viewmodels/PasscodeViewModel.ktlibs/mifos-passcode/src/commonMain/kotlin/proto/org/mifos/library/passcode/data/PasscodeManager.ktlibs/mifos-passcode/src/commonMain/kotlin/proto/org/mifos/library/passcode/data/PasscodePreferencesDataSource.ktlibs/mifos-passcode/src/commonMain/kotlin/proto/org/mifos/library/passcode/di/PreferenceModule.ktlibs/mifos-passcode/src/commonMain/kotlin/proto/org/mifos/library/passcode/model/PasscodePreferencesProto.kt
💤 Files with no reviewable changes (1)
- cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/LoginNavGraph.kt
🚧 Files skipped from review as they are similar to previous changes (13)
- feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/navigation/LoginScreenNavigation.kt
- libs/mifos-passcode/src/commonMain/kotlin/proto/org/mifos/library/passcode/di/PreferenceModule.kt
- cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/MifosNavHost.kt
- libs/mifos-passcode/src/commonMain/kotlin/proto/org/mifos/library/passcode/model/PasscodePreferencesProto.kt
- cmp-shared/src/commonMain/kotlin/org/mifospay/shared/ui/MifosApp.kt
- feature/settings/src/commonMain/kotlin/org/mifospay/feature/settings/SettingsViewModel.kt
- libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/utility/Step.kt
- feature/settings/build.gradle.kts
- cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/PasscodeNavGraph.kt
- libs/mifos-passcode/src/commonMain/kotlin/proto/org/mifos/library/passcode/data/PasscodePreferencesDataSource.kt
- libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/PasscodeButton.kt
- libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/ChangePasscodeSuccessDialog.kt
- cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/MifosNavGraph.kt
🧰 Additional context used
🧬 Code graph analysis (3)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayApp.kt (1)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/MifosNavGraph.kt (1)
passcodeGraphRoute(31-33)
libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/PasscodeNavigation.kt (1)
libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/PassCodeScreen.kt (1)
PasscodeScreen(65-171)
libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/viewmodels/PasscodeViewModel.kt (1)
core-base/ui/src/commonMain/kotlin/template/core/base/ui/BaseViewModel.kt (1)
sendEvent(97-99)
🔇 Additional comments (14)
feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/login/LoginViewModel.kt (1)
139-144: LGTM! Clear documentation of architectural change.The documentation effectively explains why
NavigateToPasscodeScreenwas removed and clarifies that passcode navigation is now centralized at the app level. This helps future maintainers understand the design decision.cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayViewModel.kt (1)
47-51: Data class signature updated appropriately.The addition of
hasPasscodeandhasSkippedPasscodeSetupfields toMainUiState.Successcorrectly exposes the passcode state needed for the dynamic Settings UI feature described in the PR objectives.feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/login/LoginScreen.kt (1)
57-70: Excellent documentation of navigation refactoring.The documentation thoroughly explains the double-navigation bug that was fixed and clearly describes the new separation of concerns where login handles authentication while app-level logic handles passcode routing. This provides valuable context for future maintainers.
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/RootNavGraph.kt (1)
38-46: LGTM!The
passcodeNavGraphregistration androotNavControllerpropagation toMifosAppcorrectly enable intention-based passcode navigation from the main graph. This cleanly separates passcode routing from the login flow.libs/mifos-passcode/src/commonMain/kotlin/proto/org/mifos/library/passcode/data/PasscodeManager.kt (1)
43-61: LGTM!The
hasSkippedPasscodeSetupflow andsetSkippedPasscodeSetupfunction provide a clean API for tracking skip state. The automatic reset of the skip flag when saving a passcode (line 54) ensures consistent state—users who eventually create a passcode are no longer considered "skipped."libs/mifos-passcode/src/commonMain/composeResources/values/strings.xml (1)
21-24: LGTM!The new string resources appropriately support the change-passcode flow with clear, user-friendly messaging. The naming follows the existing
library_mifos_passcode_prefix pattern used for newer strings in this file.libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/PassCodeScreen.kt (1)
65-93: LGTM on the unified callback and event handling.The consolidation of
onPasscodeConfirm/onPasscodeRejectedintoonPasscodeFlowCompletesimplifies the API. TheEventsEffectproperly handles the three event types (PasscodeConfirmed,PasscodeRejected,PasscodeSetupSkipped) with appropriate responses.libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/viewmodels/PasscodeViewModel.kt (1)
119-142: LGTM on step-based passcode buffer handling.The switch to per-step
StringBuilderbuffers (verifyPasscode,createPasscode,confirmPasscode) based onactiveStepcleanly separates input across the multi-step flow and prevents cross-contamination between steps.feature/settings/src/commonMain/composeResources/values/strings.xml (1)
18-18: LGTM! String resource addition is correct.The new string resource follows the existing naming convention and integrates well with the dynamic passcode UI flow.
feature/settings/src/commonMain/kotlin/org/mifospay/feature/settings/SettingsScreen.kt (5)
34-34: LGTM! Import correctly added.The import of the new string resource is properly placed and follows the existing pattern.
67-68: LGTM! State collection follows Compose best practices.The lifecycle-aware state collection ensures the UI updates reactively when the passcode state changes, with proper lifecycle handling.
89-94: LGTM! State propagation is correct.The
hasPasscodestate is correctly passed to the content composable following unidirectional data flow.
98-102: LGTM! Function signature correctly extended.The
hasPasscodeparameter addition is correct, and requiring it without a default ensures explicit state provision from callers.
142-151: LGTM! Dynamic passcode button implementation is correct.The conditional label switching between "Change Passcode" and "Create Passcode" based on
hasPasscodestate provides clear user guidance. The unifiedSettingsAction.ChangePasscodeaction is the correct design pattern here. The navigation layer routes to the passcode screen withIntention.CHANGE_PASSCODE, and thePasscodeScreenViewModeladapts the intention based on whether a passcode already exists, avoiding the need for the navigation layer to check passcode state.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
@libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/viewmodels/PasscodeViewModel.kt:
- Around line 226-232: The ViewModel sets isChangePasscodeSuccessful = true in
validateAndSavePasscode() but never clears it, so when dismissing the success
dialog in onDismissChangePasscodeSuccessDialog() you should reset
isChangePasscodeSuccessful back to false before sending
PasscodeEvent.PasscodeConfirmed; locate the boolean property
isChangePasscodeSuccessful in the PasscodeViewModel, update
onDismissChangePasscodeSuccessDialog() to clear that flag (and any related UI
state) and then call sendEvent(PasscodeEvent.PasscodeConfirmed) to ensure the
success state does not persist across navigations or reused ViewModel instances.
- Around line 244-257: In validateExistingPasscode(), when a passcode is
rejected you clear verifyPasscode and reset currentPasscodeInput but do not
reset filledDots, leaving the UI showing filled dots; update the
mutableStateFlow.update call (used to set currentPasscodeInput = "") to also
reset filledDots (e.g., filledDots = 0 or empty as appropriate) so the state
reflects the cleared input—apply this change in the reject branch (and consider
doing it after verifyPasscode.clear in the success path as well) to ensure the
UI is consistent.
🧹 Nitpick comments (3)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayApp.kt (1)
95-108: Avoid repeated smart casting for better readability.Lines 97-99 cast
uiState as Successthree times. Extract the cast once to improve code clarity and efficiency.♻️ Refactor to eliminate repeated casts
val navDestination = when { uiState is Success -> { - val userData = (uiState as Success).userData - val hasPasscode = (uiState as Success).hasPasscode - val hasSkippedPasscodeSetup = (uiState as Success).hasSkippedPasscodeSetup + val successState = uiState as Success + val userData = successState.userData + val hasPasscode = successState.hasPasscode + val hasSkippedPasscodeSetup = successState.hasSkippedPasscodeSetup when { !userData.authenticated -> LOGIN_GRAPH hasPasscode -> passcodeGraphRoute(Intention.LOGIN_WITH_PASSCODE) hasSkippedPasscodeSetup -> MAIN_GRAPH else -> passcodeGraphRoute(Intention.CREATE_PASSCODE) } } else -> LOGIN_GRAPH }libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/viewmodels/PasscodeViewModel.kt (2)
124-179: Consider extracting duplicated buffer selection logic.The
when (state.activeStep)pattern for selecting the current passcode buffer is repeated acrossenterKey,deleteKey, anddeleteAllKeys. Consider extracting to a computed property for DRY.♻️ Suggested refactor
+ private val currentPasscodeBuffer: StringBuilder + get() = when (state.activeStep) { + Step.Verify -> verifyPasscode + Step.Create -> createPasscode + Step.Confirm -> confirmPasscode + } + private fun enterKey(key: String) { if (state.filledDots >= PASSCODE_LENGTH) return - - val currentPasscode = when (state.activeStep) { - Step.Verify -> verifyPasscode - Step.Create -> createPasscode - Step.Confirm -> confirmPasscode - } - - currentPasscode.append(key) + currentPasscodeBuffer.append(key) // ... rest of function uses currentPasscodeBuffer
208-224: LGTM with minor suggestion.The flow logic is correct for all documented intention/step combinations. Consider adding an else branch with logging for defensive debugging in case of unexpected state combinations.
♻️ Optional: add defensive else branch
state.intention == Intention.CHANGE_PASSCODE && state.activeStep == Step.Confirm -> validateAndSavePasscode() + else -> { + // Log unexpected state for debugging + // timber.w("Unexpected passcode state: intention=${state.intention}, step=${state.activeStep}") + } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayApp.ktlibs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/viewmodels/PasscodeViewModel.kt
🧰 Additional context used
🧬 Code graph analysis (2)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayApp.kt (1)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/MifosNavGraph.kt (1)
passcodeGraphRoute(31-33)
libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/viewmodels/PasscodeViewModel.kt (1)
core-base/ui/src/commonMain/kotlin/template/core/base/ui/BaseViewModel.kt (1)
sendEvent(97-99)
🔇 Additional comments (10)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayApp.kt (3)
22-22: LGTM! Clean imports for the new navigation flow.The new imports support the intention-based navigation routing introduced in this PR.
Also applies to: 30-31
82-94: Excellent documentation of the navigation decision tree.The multi-line comment clearly explains the routing logic and helps future maintainers understand the complex state-driven navigation flow.
100-105: The logic correctly ensures mutual exclusivity. When a passcode is saved viasavePasscode(),hasSkippedPasscodeSetupis explicitly reset to false, which is properly documented in thePasscodeManagerclass. The flags cannot both be true simultaneously because creating or changing a passcode always clears the skip flag. No changes needed.libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/viewmodels/PasscodeViewModel.kt (7)
28-38: LGTM!Good separation of concerns with per-step passcode buffers. Using separate
StringBuilderinstances prevents cross-contamination between Verify/Create/Confirm steps.
40-83: Well-structured intention-based initialization.The flow correctly handles all three intentions and the edge case where a user who skipped initial passcode setup can still create one via "Change Passcode". The KDoc comment explains the reasoning clearly.
97-122: LGTM!Clean action dispatch. The skip tracking via
setSkippedPasscodeSetup(true)is important for UX—prevents repeatedly prompting users who deliberately chose not to use a passcode.
259-281: LGTM!Clean step transitions with proper UI state reset for each new step.
283-308: LGTM!Good UX differentiation—showing a success dialog for passcode change provides clearer feedback, while initial setup remains streamlined. The KDoc explains the rationale well.
310-320: LGTM!Correct partial reset—clears input buffers while preserving flow state (intention, activeStep). The
isChangePasscodeSuccessfulreset issue was addressed in a previous comment.
323-366: LGTM!Well-structured state, events, and actions for the intention-driven passcode flow. The
Parcelizeannotation ensures state can survive configuration changes.
sahilshivekar
force-pushed
the
feature/mw-324-change-passcode
branch
from
04b02ac to
90060f2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/PasscodeStepIndicator.kt (2)
24-24: Nice addition of preview support!The preview import and preview function enable design-time visualization of the component, which improves developer experience.
💡 Optional: Consider adding preview variants for different states
To better showcase the component's behavior across different steps, consider adding preview variants:
@Preview @Composable fun PasscodeStepIndicatorPreview() { Column(verticalArrangement = Arrangement.spacedBy(16.dp)) { // Create step PasscodeStepIndicator(activeStep = Step.Create) // Confirm step PasscodeStepIndicator(activeStep = Step.Confirm) } }This would show how the indicator progresses through the flow.
Also applies to: 74-78
56-56: Consider adding a descriptive animation label.The animation label is currently an empty string. While functional, a descriptive label like
"stepColor"or"stepIndicatorColor"would improve debugging and help with animation inspection tooling.♻️ Suggested improvement
val stepColor = - animateColorAsState(if (isActiveStep) blueTint else Color.Gray, label = "") + animateColorAsState(if (isActiveStep) blueTint else Color.Gray, label = "stepColor")cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/MifosNavHost.kt (1)
187-198: Consider streamlining inline comments.The inline comments are helpful but quite verbose (7 lines) and reference internal VM behavior details. This couples the navigation layer to implementation details that might change.
♻️ Proposed streamlined version
- // When user clicks "Change Passcode" in settings, navigate to passcode screen - // with CHANGE_PASSCODE intention to start the 3-step change flow: - // 1. Verify old passcode - // 2. Create new passcode - // 3. Confirm new passcode - // The PasscodeScreenViewModel will change the intention from CHANGE_PASSCODE to CREATE_PASSCODE if there is already a passcode set - // (this is done to avoid the navigation layer needing to check if a passcode exists) + // Navigate to passcode screen to start change flow (Verify → Create → Confirm) onChangePasscode = { rootNavController.navigateToPasscodeScreen( intention = Intention.CHANGE_PASSCODE, ) },cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayViewModel.kt (1)
27-35: Consider combining flows for reactive passcode state.The current implementation reads
passcodeManagerstate as snapshots (.value) within theuserInfoflow mapping. If passcode state changes independently, the UI won't update untiluserInfoemits again, potentially showing stale passcode status.♻️ Proposed reactive flow combination
- val uiState: StateFlow<MainUiState> = userDataRepository.userInfo.map { - val hasPasscode = passcodeManager.hasPasscode.value - val hasSkippedPasscodeSetup = passcodeManager.hasSkippedPasscodeSetup.value - MainUiState.Success(it, hasPasscode, hasSkippedPasscodeSetup) - }.stateIn( + val uiState: StateFlow<MainUiState> = combine( + userDataRepository.userInfo, + passcodeManager.hasPasscode, + passcodeManager.hasSkippedPasscodeSetup, + ) { userInfo, hasPasscode, hasSkippedPasscodeSetup -> + MainUiState.Success(userInfo, hasPasscode, hasSkippedPasscodeSetup) + }.stateIn( scope = viewModelScope, initialValue = MainUiState.Loading, started = SharingStarted.WhileSubscribed(5_000), )Add import at the top:
import kotlinx.coroutines.flow.combinecmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayApp.kt (2)
53-53: RedundantvarwithmutableStateOf.Using
varalongsideremember { mutableStateOf(...) }is redundant. TheMutableStateholder already provides mutability via.value. This pattern can cause confusion.♻️ Suggested fix
- var showErrorDialog = remember { mutableStateOf<Boolean>(false) } + val showErrorDialog = remember { mutableStateOf(false) }Or use delegate syntax for cleaner access:
var showErrorDialog by remember { mutableStateOf(false) }
95-108: Simplify repeated casting with smart cast.The
uiStateis cast toSuccessthree times within the same block. Use Kotlin's smart casting or destructuring to avoid repetition.♻️ Suggested refactor
val navDestination = when { uiState is Success -> { - val userData = (uiState as Success).userData - val hasPasscode = (uiState as Success).hasPasscode - val hasSkippedPasscodeSetup = (uiState as Success).hasSkippedPasscodeSetup + val (userData, hasPasscode, hasSkippedPasscodeSetup) = uiState when { !userData.authenticated -> LOGIN_GRAPH hasPasscode -> passcodeGraphRoute(Intention.LOGIN_WITH_PASSCODE) hasSkippedPasscodeSetup -> MAIN_GRAPH else -> passcodeGraphRoute(Intention.CREATE_PASSCODE) } } else -> LOGIN_GRAPH }This requires adding a
componentN()destructuring operator toSuccess, or simply rely on smart casting since theis Successcheck already narrows the type.libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/PassCodeScreen.kt (1)
118-121: Consider moving dialog outside the layout Column.
ChangePasscodeSuccessDialog(which renders anAlertDialog) is placed inside theColumnlayout. WhileAlertDialoguses a popup window and may render correctly, placing dialogs outside layout containers is the conventional pattern in Compose for clarity and to avoid potential measurement issues.♻️ Suggested placement
Move the dialog outside the
Scaffoldcontent, similar to howPasscodeMismatchedDialogis handled inPasscodeView:Scaffold( modifier = modifier .fillMaxSize(), ) { paddingValues -> + ChangePasscodeSuccessDialog( + visible = state.isChangePasscodeSuccessful, + onDismiss = { viewModel.trySendAction(PasscodeAction.DismissChangePasscodeSuccessDialog) }, + ) + Column( ... ) { ... - ChangePasscodeSuccessDialog( - visible = state.isChangePasscodeSuccessful, - onDismiss = { viewModel.trySendAction(PasscodeAction.DismissChangePasscodeSuccessDialog) }, - )libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/viewmodels/PasscodeViewModel.kt (1)
61-83: State access after async update could be clearer.After
mutableStateFlow.update { ... }completes on line 71, you accessstate.intentionon line 73. While this works becauseupdateis synchronous within the coroutine andstateis a getter forstateFlow.value, the pattern could be clearer by capturing the intention value locally.♻️ Suggested refactor for clarity
private fun getIntention() { viewModelScope.launch { val hasPasscode = passcodeRepository.hasPasscode.first() + val intention = savedStateHandle.get<String>(INTENTION) + ?.let(Intention::fromValue) + ?: Intention.LOGIN_WITH_PASSCODE + mutableStateFlow.update { it.copy( - intention = savedStateHandle.get<String>(INTENTION) - ?.let(Intention::fromValue) - ?: Intention.LOGIN_WITH_PASSCODE, + intention = intention, hasPasscode = hasPasscode, ) } if ( - state.intention == Intention.CREATE_PASSCODE || - (state.intention == Intention.CHANGE_PASSCODE && !state.hasPasscode) + intention == Intention.CREATE_PASSCODE || + (intention == Intention.CHANGE_PASSCODE && !hasPasscode) ) { mutableStateFlow.update { it.copy( activeStep = Step.Create, ) } } } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
cmp-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/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.ktcmp-shared/src/commonMain/kotlin/org/mifospay/shared/ui/MifosApp.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/settings/build.gradle.ktsfeature/settings/src/commonMain/composeResources/values/strings.xmlfeature/settings/src/commonMain/kotlin/org/mifospay/feature/settings/SettingsScreen.ktfeature/settings/src/commonMain/kotlin/org/mifospay/feature/settings/SettingsViewModel.ktlibs/mifos-passcode/src/commonMain/composeResources/values/strings.xmllibs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/PassCodeScreen.ktlibs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/PasscodeNavigation.ktlibs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/ChangePasscodeSuccessDialog.ktlibs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/PasscodeButton.ktlibs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/PasscodeHeader.ktlibs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/PasscodeStepIndicator.ktlibs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/utility/Step.ktlibs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/viewmodels/PasscodeViewModel.ktlibs/mifos-passcode/src/commonMain/kotlin/proto/org/mifos/library/passcode/data/PasscodeManager.ktlibs/mifos-passcode/src/commonMain/kotlin/proto/org/mifos/library/passcode/data/PasscodePreferencesDataSource.ktlibs/mifos-passcode/src/commonMain/kotlin/proto/org/mifos/library/passcode/di/PreferenceModule.ktlibs/mifos-passcode/src/commonMain/kotlin/proto/org/mifos/library/passcode/model/PasscodePreferencesProto.kt
💤 Files with no reviewable changes (1)
- cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/LoginNavGraph.kt
🚧 Files skipped from review as they are similar to previous changes (11)
- cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/RootNavGraph.kt
- feature/settings/src/commonMain/kotlin/org/mifospay/feature/settings/SettingsScreen.kt
- libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/PasscodeHeader.kt
- cmp-shared/src/commonMain/kotlin/org/mifospay/shared/ui/MifosApp.kt
- feature/settings/src/commonMain/composeResources/values/strings.xml
- libs/mifos-passcode/src/commonMain/kotlin/proto/org/mifos/library/passcode/di/PreferenceModule.kt
- feature/settings/src/commonMain/kotlin/org/mifospay/feature/settings/SettingsViewModel.kt
- libs/mifos-passcode/src/commonMain/kotlin/proto/org/mifos/library/passcode/model/PasscodePreferencesProto.kt
- cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/PasscodeNavGraph.kt
- libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/ChangePasscodeSuccessDialog.kt
- libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/PasscodeNavigation.kt
🧰 Additional context used
🧬 Code graph analysis (3)
libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/PassCodeScreen.kt (1)
libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/ChangePasscodeSuccessDialog.kt (1)
ChangePasscodeSuccessDialog(23-49)
libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/viewmodels/PasscodeViewModel.kt (1)
core-base/ui/src/commonMain/kotlin/template/core/base/ui/BaseViewModel.kt (1)
sendEvent(97-99)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayApp.kt (1)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/MifosNavGraph.kt (1)
passcodeGraphRoute(31-33)
🔇 Additional comments (26)
feature/settings/build.gradle.kts (1)
26-26: LGTM! Dependency addition aligns with the PR objectives.The addition of
mifosPasscodedependency enables the Settings feature to observe passcode state and display dynamic UI labels ("Change Passcode" or "Create Passcode"). The dependency reference is syntactically correct, appropriately scoped tocommonMain, and introduces no circular dependencies—mifosPasscodehas no reverse dependency onfeature.settings.feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/login/LoginViewModel.kt (1)
139-149: LGTM! Clean removal of passcode navigation from login flow.The documentation clearly explains that passcode navigation is now handled at the app level after authentication state changes. The
LoginEventsealed class is properly updated with no references to passcode navigation, and thehandleLoginResultmethod correctly handles success without triggering any navigation events.feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/login/LoginScreen.kt (2)
57-70: Excellent documentation explaining the bug fix and architectural improvement.The documentation clearly explains both the technical issue (duplicate navigation calls causing two PasscodeScreenViewModel instances) and the solution (app-level navigation handling). This provides valuable context for future maintainers.
72-93: App-level navigation correctly handles all post-login passcode scenarios.The app-level navigation in
MifosPayApp.kt(lines 95-108) properly implements all three post-login scenarios:
- User with existing passcode → navigates to passcode verification (
LOGIN_WITH_PASSCODE)- User who skipped passcode → navigates directly to main app
- User needing passcode setup → navigates to passcode creation (
CREATE_PASSCODE)The implementation extracts
hasPasscodeandhasSkippedPasscodeSetupfrom the UI state and routes accordingly. The separation of concerns is clean—LoginScreen focuses on authentication while app-level navigation handles all post-login passcode flows.feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/navigation/LoginScreenNavigation.kt (1)
21-43: LGTM! Navigation builder properly updated.The removal of
onNavigateToPasscodeScreencallback is clean and consistent with the app-level navigation refactor. The function signature and LoginScreen invocation are properly updated, with clear documentation explaining the change.libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/PasscodeStepIndicator.kt (3)
29-38: Excellent documentation!The KDoc clearly explains the component's behavior, including why the indicator is hidden during Verify and how the step highlighting works. This will help future maintainers understand the design decisions.
44-71: Good UX decision to conditionally show the indicator.Hiding the step indicator during the Verify phase is appropriate since users don't need progress indication during login or old passcode verification. The indicator appropriately shows only for the Create and Confirm steps where progress tracking is meaningful.
54-54: The code is correct.STEPS_COUNT = 2matches the highlighting logic: the indicator displays 2 boxes forStep.Create(index 1) andStep.Confirm(index 2), withstep + 1 == activeStep.indexproperly highlighting the active step. TheVerifystep (index 0) is not included in the visible indicator.libs/mifos-passcode/src/commonMain/composeResources/values/strings.xml (2)
21-21: LGTM! String resources are clear and consistent.The new string resources follow the established
library_mifos_passcode_*naming pattern and are appropriately worded for their use cases in the passcode flows.Also applies to: 24-24
22-22: No action required. The message "Your passcode has been updated" is correctly scoped to the change-passcode flow only. Initial passcode creation (CREATE_PASSCODE intention) follows a different code path and does not display this success dialog; instead, it emits PasscodeEvent.PasscodeConfirmed directly without showing a dialog.cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/MifosNavHost.kt (1)
94-94: LGTM! Root navigation controller enables cross-graph passcode flow.The addition of
rootNavControllerparameter correctly enables navigation to the passcode screen from the main navigation graph, supporting the intention-based passcode flow described in the PR.libs/mifos-passcode/src/commonMain/kotlin/proto/org/mifos/library/passcode/data/PasscodeManager.kt (3)
18-24: LGTM! Clear documentation of skip-state behavior.The KDoc clearly explains the auto-reset behavior when a passcode is saved, which is important context for consumers of this API.
43-47: LGTM! Consistent flow setup.The
hasSkippedPasscodeSetupflow follows the same pattern as existing flows in this class with appropriate initial value and sharing strategy.
49-61: LGTM! Logical auto-reset behavior.Setting
hasSkippedPasscodeSetup = falsewhen saving a passcode is correct—once a user actively sets a passcode, they are no longer in a "skipped" state. The newsetSkippedPasscodeSetupmethod is a clean delegation to the data source.libs/mifos-passcode/src/commonMain/kotlin/proto/org/mifos/library/passcode/data/PasscodePreferencesDataSource.kt (2)
46-47: LGTM! Consistent flow mapping.The
hasSkippedPasscodeSetupflow follows the established pattern for exposing settings properties as reactive streams.
56-66: LGTM! Correct selective update implementation.The method correctly uses
copy()to update only the skip flag while preserving other preferences, and properly updates both persistent storage and in-memory state.libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/utility/Step.kt (1)
15-18: The Step enum reordering is safe—no code in the codebase relies on enum ordinals. All usages are by explicit name matching (when expressions, equality checks), not ordinal values. The explicitindexproperty is also unused.Likely an incorrect or invalid review comment.
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayViewModel.kt (1)
47-51: Breaking API change: Success data class signature expanded, but all consumers already updated.The
Successdata class now requires three parameters (userData,hasPasscode,hasSkippedPasscodeSetup) instead of one. The single constructor call inMifosPayViewModel.kt:30already uses the new signature, and the detailed consumer inMifosPayApp.ktcorrectly accesses all three new properties. No further updates needed.cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/MifosNavGraph.kt (1)
19-33: LGTM!The intention-based route building is well-structured. Private constants for implementation details with a public templated route and a helper function follows good encapsulation practices. The KDoc documentation is helpful.
libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/PasscodeButton.kt (2)
35-58: LGTM!The refactor from boolean
hasPassCodetoIntentionenum makes the visibility logic more explicit and self-documenting. The KDoc accurately describes the button's context.
66-89: LGTM!Consistent with
PasscodeSkipButton, usingIntentionfor visibility gating is cleaner than the previous boolean approach.libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/PassCodeScreen.kt (1)
65-93: LGTM on the event handling and callback consolidation.Consolidating
onPasscodeConfirmandonPasscodeRejectedintoonPasscodeFlowCompletesimplifies the API surface. The event-to-action mapping is clear and correctly handles all three event types.libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/viewmodels/PasscodeViewModel.kt (4)
124-147: LGTM on step-based buffer management.The switch to step-specific buffers (
verifyPasscode,createPasscode,confirmPasscode) prevents cross-contamination between flow steps. Thewhenexpression correctly selects the appropriate buffer based onactiveStep.
208-224: LGTM on intention-based flow routing.The
processCompletedPasscodefunction correctly handles all combinations of intention and step. The branching logic is well-documented and covers:
- CREATE_PASSCODE: Create → Confirm → Save
- LOGIN_WITH_PASSCODE: Verify only
- CHANGE_PASSCODE: Verify → Create → Confirm → Save
244-257: Verify buffer cleared only on success path.On line 252,
verifyPasscode.clear()is called only when validation succeeds. On the rejection path (line 254), the buffer is not cleared here—it's handled later whenresetState()is called via the UI flow (dialog dismiss → restart). This is correct but relies on the UI calling restart.Consider whether clearing the buffer immediately on rejection would be more defensive:
} else { + verifyPasscode.clear() sendEvent(PasscodeEvent.PasscodeRejected) }
334-343: LGTM on state model updates.The
PasscodeStatedata class is well-structured with:
intentionfor flow-type trackingisChangePasscodeSuccessfulfor dialog visibility- Default
activeStep = Step.Verifyaligns with the most common entry pointThe KDoc documentation clearly explains each property's purpose.
biplab1
left a comment
biplab1
left a comment
There was a problem hiding this comment.
Some preliminary review comments have been added. Let me know after you done the changes, I will review again.
sahilshivekar
force-pushed
the
feature/mw-324-change-passcode
branch
from
5285924 to
b925603
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/viewmodels/PasscodeViewModel.kt (1)
293-308: On passcode mismatch at Confirm step, user remains stuck unable to proceed.When
createPasscodeandconfirmPasscodedon't match,resetState()clears both buffers but leavesactiveStepat Confirm. SincecreatePasscodeis now empty, any subsequent passcode entry at Confirm cannot match it, blocking the user from completing the flow.The user must return to Create step to re-enter a new passcode that can be confirmed.
🐛 Proposed fix
private suspend fun validateAndSavePasscode() { if (createPasscode.toString() == confirmPasscode.toString()) { passcodeRepository.savePasscode(confirmPasscode.toString()) if (state.intention == Intention.CHANGE_PASSCODE) { mutableStateFlow.update { it.copy(isChangePasscodeSuccessful = true) } } else { sendEvent(PasscodeEvent.PasscodeConfirmed) } resetState() } else { sendEvent(PasscodeEvent.PasscodeRejected) - resetState() + resetStateToCreateStep() } } + private fun resetStateToCreateStep() { + mutableStateFlow.update { + it.copy( + activeStep = Step.Create, + currentPasscodeInput = "", + filledDots = 0, + ) + } + verifyPasscode.clear() + createPasscode.clear() + confirmPasscode.clear() + }
🤖 Fix all issues with AI agents
In
`@libs/mifos-passcode/src/commonMain/kotlin/proto/org/mifos/library/passcode/data/PasscodePreferencesDataSource.kt`:
- Around line 55-65: The read-modify-write in updateSkippedPasscodeSetup races
with updatePasscodeSettings because both run on MifosDispatchers.IO; introduce a
shared Mutex (e.g., private val passcodeMutex = Mutex()) and wrap the critical
sections of both updateSkippedPasscodeSetup and updatePasscodeSettings with
passcodeMutex.withLock { ... } so the sequence (read passcodeSettings.value,
settings.putPasscodePreference(updated), passcodeSettings.value = updated) is
atomic; ensure the same Mutex guards calls that modify passcodeSettings and
settings.putPasscodePreference to prevent lost updates.
♻️ Duplicate comments (1)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayApp.kt (1)
82-103: Avoid state‑drivenstartDestination; navigate viaLaunchedEffectinstead.Deriving
startDestinationfromuiStatecan recreate the NavHost and reset the back stack when state changes (e.g., loading → success). Use a fixed start destination and perform state-driven navigation in aLaunchedEffectwithpopUpTo.✅ Suggested pattern
- val navDestination = when { + val navDestination = when { uiState is Success -> { val userData = (uiState as Success).userData val hasPasscode = (uiState as Success).hasPasscode val hasSkippedPasscodeSetup = (uiState as Success).hasSkippedPasscodeSetup when { !userData.authenticated -> LOGIN_GRAPH hasPasscode -> passcodeGraphRoute(Intention.LOGIN_WITH_PASSCODE) hasSkippedPasscodeSetup -> MAIN_GRAPH else -> passcodeGraphRoute(Intention.CREATE_PASSCODE) } } else -> LOGIN_GRAPH } + + LaunchedEffect(navDestination) { + if (navController.currentBackStackEntry?.destination?.route != navDestination) { + navController.navigate(navDestination) { + popUpTo(navController.graph.id) { inclusive = true } + launchSingleTop = true + } + } + } @@ - startDestination = navDestination, + startDestination = LOGIN_GRAPH,Please verify with a cold start + state transition that the back stack remains stable.
🧹 Nitpick comments (1)
libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/viewmodels/PasscodeViewModel.kt (1)
34-36: Consider moving inline comments to KDoc or removing them.Based on learnings from past review comments, the project prefers avoiding inline comments. The explanation about the
verifyPasscodebuffer purpose could be moved to a KDoc on the class or removed if self-explanatory.
sahilshivekar
force-pushed
the
feature/mw-324-change-passcode
branch
from
b925603 to
e432d33
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/viewmodels/PasscodeViewModel.kt (1)
293-320: Mismatch path can trap users on the Confirm stepAfter rejection, Line 305 calls
resetState()which clearscreatePasscode/confirmPasscode(Lines 318–319) but leavesactiveStepat Confirm, so the next attempt can never match. Reset to Create (or Verify for CHANGE_PASSCODE if you want re‑verification) before allowing retry.🐛 Suggested fix
@@ } else { sendEvent(PasscodeEvent.PasscodeRejected) - resetState() + resetState() + moveToCreateStep() }cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/PasscodeNavGraph.kt (1)
22-41: startDestination base route mismatch with navigation routeThe
startDestinationuses"$PASSCODE_SCREEN?..."(resolves to"passcode_screen?...") while the navigation route usesMifosNavGraph.PASSCODE_GRAPH_ROUTE(resolves to"passcode_graph?..."). In Compose Navigation, the startDestination must reference a valid destination within the graph. The base routes must match.Fix the mismatch by using
"$PASSCODE_GRAPH_BASE?$INTENTION={$INTENTION}"for thestartDestinationto align with the route definition.
🤖 Fix all issues with AI agents
In `@cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayViewModel.kt`:
- Around line 27-31: uiState currently maps only userDataRepository.userInfo and
samples passcodeManager.hasPasscode.value and hasSkippedPasscodeSetup.value, so
passcode changes won't trigger emissions; replace the mapping with a combined
flow that observes userDataRepository.userInfo, passcodeManager.hasPasscode, and
passcodeManager.hasSkippedPasscodeSetup (e.g., use combine(userInfo,
hasPasscode, hasSkipped) and map to MainUiState.Success(...)) and then call
stateIn(...) so uiState updates whenever any of those flows change.
In
`@feature/auth/src/commonMain/kotlin/org/mifospay/feature/auth/login/LoginScreen.kt`:
- Around line 57-61: KDoc in LoginScreen.kt contains a duplicated word "the the"
in the comment block above the function/class; update the comment text to remove
the duplicate so it reads "As the app-level navigation logic (MifosPayApp.kt) is
already doing it..." (edit the KDoc near the LoginScreen declaration or the top
comment block) ensuring the rest of the wording remains unchanged.
In
`@libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/PasscodeNavigation.kt`:
- Around line 38-41: The fromValue companion method currently returns
CREATE_PASSCODE when the provided value is invalid, which can bypass
verification; update PasscodeNavigation.Companion.fromValue to default to the
safer LOGIN_WITH_PASSCODE (or throw) instead of CREATE_PASSCODE by changing the
fallback from CREATE_PASSCODE to LOGIN_WITH_PASSCODE (or replacing the fallback
with an explicit error/exception) so Intention.entries.firstOrNull { it.value ==
value } uses LOGIN_WITH_PASSCODE as the safe default.
In
`@libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/PassCodeScreen.kt`:
- Around line 118-121: The success dialog dismissal currently only sends
PasscodeAction.DismissChangePasscodeSuccessDialog and leaves the user on the
passcode screen; update the dialog's onDismiss to both dismiss and complete the
flow by calling viewModel.onPasscodeFlowComplete() (i.e.,
ChangeChangePasscodeSuccessDialog onDismiss should call
viewModel.trySendAction(PasscodeAction.DismissChangePasscodeSuccessDialog) and
then viewModel.onPasscodeFlowComplete()), or alternatively update the
ViewModel's handler for PasscodeAction.DismissChangePasscodeSuccessDialog to
invoke onPasscodeFlowComplete() so navigation back to settings occurs; look for
ChangePasscodeSuccessDialog, state.isChangePasscodeSuccessful,
PasscodeAction.DismissChangePasscodeSuccessDialog, and
viewModel.onPasscodeFlowComplete to implement the fix.
In
`@libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/utility/Step.kt`:
- Around line 12-18: The Step enum's index property is declared mutable but
never changed; change the property declaration in enum class Step from var
index: Int to val index: Int to make it immutable, updating the enum definition
(Step.Verify, Step.Create, Step.Confirm) accordingly; verify usages such as in
PasscodeStepIndicator.kt (where activeStep.index is read) still compile and
adjust any tests or callers if they attempted to assign to Step.index (they
shouldn't).
In
`@libs/mifos-passcode/src/commonMain/kotlin/proto/org/mifos/library/passcode/di/PreferenceModule.kt`:
- Around line 19-34: The KDoc incorrectly states dispatcher usage: update the
comment or the wiring so they match; either change the KDoc to explain that
PasscodePreferencesDataSource uses MifosDispatchers.IO while PasscodeManager is
intentionally constructed with MifosDispatchers.Unconfined, or change the DI
binding in PasscodePreferenceModule so PasscodeManager receives
get(named(MifosDispatchers.IO.name)) instead of Unconfined; reference the
PasscodePreferenceModule, PasscodeManager, PasscodePreferencesDataSource, and
MifosDispatchers.IO/Unconfined symbols when making the change.
♻️ Duplicate comments (3)
libs/mifos-passcode/src/commonMain/kotlin/proto/org/mifos/library/passcode/data/PasscodePreferencesDataSource.kt (1)
48-64: Concurrent updates can lose state (race).
updatePasscodeSettingsandupdateSkippedPasscodeSetupcan interleave, overwriting each other’s writes. Guard these mutations with a shared lock or consolidate into a single atomic update path.🔒 Suggested fix with a shared Mutex
+import kotlinx.coroutines.sync.Mutex +import kotlinx.coroutines.sync.withLock @@ class PasscodePreferencesDataSource( private val settings: Settings, private val dispatcher: CoroutineDispatcher, ) { + private val updateMutex = Mutex() @@ suspend fun updatePasscodeSettings(passcodePreferences: PasscodePreferencesProto) { withContext(dispatcher) { - settings.putPasscodePreference(passcodePreferences) - passcodeSettings.value = passcodePreferences + updateMutex.withLock { + settings.putPasscodePreference(passcodePreferences) + passcodeSettings.value = passcodePreferences + } } } @@ suspend fun updateSkippedPasscodeSetup(skipped: Boolean) { withContext(dispatcher) { - val updated = passcodeSettings.value.copy(hasSkippedPasscodeSetup = skipped) - settings.putPasscodePreference(updated) - passcodeSettings.value = updated + updateMutex.withLock { + val updated = passcodeSettings.value.copy(hasSkippedPasscodeSetup = skipped) + settings.putPasscodePreference(updated) + passcodeSettings.value = updated + } } }cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayApp.kt (1)
82-103: Avoid dynamicstartDestinationto prevent NavHost resets.
startDestinationis derived from state at Line 82 and passed intoRootNavGraphat Line 102. This can recreate the NavHost graph on recomposition, leading to back‑stack anomalies. Prefer a fixedstartDestination(or a router/splash) and navigate in aLaunchedEffectwhenuiStatechanges (similar to the unauthorized handling).✅ Suggested adjustment (fixed start + state-driven navigate)
- val navDestination = when { - uiState is Success -> { - val userData = (uiState as Success).userData - val hasPasscode = (uiState as Success).hasPasscode - val hasSkippedPasscodeSetup = (uiState as Success).hasSkippedPasscodeSetup - when { - !userData.authenticated -> LOGIN_GRAPH - hasPasscode -> passcodeGraphRoute(Intention.LOGIN_WITH_PASSCODE) - hasSkippedPasscodeSetup -> MAIN_GRAPH - else -> passcodeGraphRoute(Intention.CREATE_PASSCODE) - } - } - else -> LOGIN_GRAPH - } + LaunchedEffect(uiState) { + val navDestination = when { + uiState is Success -> { + val userData = (uiState as Success).userData + val hasPasscode = (uiState as Success).hasPasscode + val hasSkippedPasscodeSetup = (uiState as Success).hasSkippedPasscodeSetup + when { + !userData.authenticated -> LOGIN_GRAPH + hasPasscode -> passcodeGraphRoute(Intention.LOGIN_WITH_PASSCODE) + hasSkippedPasscodeSetup -> MAIN_GRAPH + else -> passcodeGraphRoute(Intention.CREATE_PASSCODE) + } + } + else -> LOGIN_GRAPH + } + navController.navigate(navDestination) { + popUpTo(navController.graph.id) { inclusive = true } + launchSingleTop = true + } + } ... - startDestination = navDestination, + startDestination = LOGIN_GRAPH,Compose Navigation NavHost startDestination dynamic changes best practiceslibs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/PasscodeHeader.kt (1)
88-91: Themodifierparameter is still ignored.This issue was previously flagged and discussed. The
modifierparameter is declared but not used—Line 89 usesModifier.fillMaxWidth()instead of chaining with the incomingmodifier. This breaks the composable's customization contract.🔧 Proposed fix
Box( - modifier = Modifier.fillMaxWidth(), + modifier = modifier.fillMaxWidth(), contentAlignment = Alignment.Center, ) {
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
`@libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/PasscodeStepIndicator.kt`:
- Around line 64-71: There is a leftover merge conflict in
PasscodeStepIndicator.kt around the .background call; remove the conflict
markers (<<<<<<<, =======, >>>>>>>) and pick one shape source (either
KptTheme.shapes.medium or MaterialTheme.shapes.medium) so the shape argument is
unambiguous; if you choose MaterialTheme, add the appropriate import for
androidx.compose.material.MaterialTheme, and if you choose KptTheme ensure its
import remains and any unused import is removed.
In
`@libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/PasscodeNavigation.kt`:
- Around line 21-34: Remove the leftover Git conflict markers surrounding the
KDoc in PasscodeNavigation (the lines containing <<<<<<<, =======, >>>>>>>) and
restore the intended KDoc comment for the enum; ensure the KDoc immediately
precedes the PasscodeNavigation enum declaration and that there are no stray
markers or duplicated blocks so the file compiles cleanly.
In
`@libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/viewmodels/PasscodeViewModel.kt`:
- Around line 34-38: The file PasscodeViewModel.kt contains unresolved Git merge
conflict markers (<<<<<<< HEAD, =======, >>>>>>>) scattered across the file
(e.g., near the comment about a separate StringBuilder for passcode verification
and other blocks); remove all conflict markers and reconcile the two conflicting
variants so the intended Kotlin code/comments remain valid (for example keep the
explanatory comment about the verification StringBuilder), ensuring methods and
properties in the PasscodeViewModel class compile cleanly; search for and delete
all <<<<<<<, =======, and >>>>>>> markers, resolve each block by merging or
choosing the correct lines, and run a build to confirm no remaining conflict
artifacts.
In
`@libs/mifos-passcode/src/commonMain/kotlin/proto/org/mifos/library/passcode/data/PasscodePreferencesDataSource.kt`:
- Around line 55-61: Remove the leftover git merge conflict markers (<<<<<<<,
=======, >>>>>>>) in PasscodePreferencesDataSource.kt and restore the intended
comment/block around the hasSkippedPasscodeSetup update; specifically edit the
PasscodePreferencesDataSource file to delete the conflict markers and keep the
documented comment starting with "Updates only the hasSkippedPasscodeSetup
flag." so the method that updates hasSkippedPasscodeSetup (the function handling
the "Skip" action) compiles normally and retains the explanatory KDoc.
♻️ Duplicate comments (2)
libs/mifos-passcode/src/commonMain/kotlin/proto/org/mifos/library/passcode/data/PasscodePreferencesDataSource.kt (1)
62-67: Guard the read‑modify‑write with a mutex to avoid lost updates.
passcodeSettings.value.copy(...)can race with other updates on a multi-threaded dispatcher.🔒 Suggested fix (shared mutex for both updates)
+import kotlinx.coroutines.sync.Mutex +import kotlinx.coroutines.sync.withLock ... class PasscodePreferencesDataSource( private val settings: Settings, private val dispatcher: CoroutineDispatcher, ) { + private val passcodeMutex = Mutex() private val passcodeSettings = MutableStateFlow( ... ) ... suspend fun updatePasscodeSettings(passcodePreferences: PasscodePreferencesProto) { withContext(dispatcher) { - settings.putPasscodePreference(passcodePreferences) - passcodeSettings.value = passcodePreferences + passcodeMutex.withLock { + settings.putPasscodePreference(passcodePreferences) + passcodeSettings.value = passcodePreferences + } } } ... suspend fun updateSkippedPasscodeSetup(skipped: Boolean) { withContext(dispatcher) { - val updated = passcodeSettings.value.copy(hasSkippedPasscodeSetup = skipped) - settings.putPasscodePreference(updated) - passcodeSettings.value = updated + passcodeMutex.withLock { + val updated = passcodeSettings.value.copy(hasSkippedPasscodeSetup = skipped) + settings.putPasscodePreference(updated) + passcodeSettings.value = updated + } } }libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/PasscodeNavigation.kt (1)
41-44: Safer fallback for invalid intention still needed.Line 42 still defaults to
CREATE_PASSCODEwhen the value is invalid, which can bypass verification.
🧹 Nitpick comments (1)
libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/PasscodeStepIndicator.kt (1)
78-82: Limit preview composable visibility.
PasscodeStepIndicatorPreviewis public in a commonMain library; consider making itinternal(orprivate) so it doesn’t leak into the public API.♻️ Proposed tweak
`@Preview` `@Composable` -fun PasscodeStepIndicatorPreview() { +internal fun PasscodeStepIndicatorPreview() { PasscodeStepIndicator(activeStep = Step.Confirm) }
sahilshivekar
force-pushed
the
feature/mw-324-change-passcode
branch
4 times, most recently
from
6fe0d5e to
398ffe7
Compare
sahilshivekar
force-pushed
the
feature/mw-324-change-passcode
branch
from
398ffe7 to
8717dea
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/navigation/PasscodeNavGraph.kt (1)
22-41:⚠️ Potential issue | 🟠 MajorRemove
popBackStack()beforenavigateToLogin()and add proper back stack management, or ensurenavigateToLogin()includespopUpTo.When the passcode graph is the root (user's first navigation destination on app startup with
hasPasscode = true),popBackStack()cannot remove the root destination and returnsfalse. This leaves the passcode graph in the back stack.The issue is specifically in
onForgotButton(line 28–29): afterpopBackStack()fails,navigateToLogin()executes without back stack cleanup. Users can then press back from the login screen and return to the passcode screen—they're trapped in a loop instead of progressing to authentication.The
onSkipButtonandonPasscodeFlowCompletecallbacks avoid this becausenavigateToMainGraph()usespopUpTo(graph.findStartDestination().id)to clear the stack, making thepopBackStack()call redundant but harmless there.Solution: Either add
popUpToto thenavigateToLogin()extension function (similar tonavigateToMainGraph()), or remove the redundantpopBackStack()calls ifnavigateToLogin()is updated to handle back stack properly. Alternatively, passnavOptionswithpopUpTodirectly in the callbacks.libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/viewmodels/PasscodeViewModel.kt (1)
222-237:⚠️ Potential issue | 🔴 CriticalBug: Passcode mismatch leaves user stuck on Confirm step with empty Create buffer.
When
createPasscode != confirmPasscode,resetState()clears the threeStringBuilderbuffers but does not resetactiveStep. The user remains onStep.Confirmwith an emptycreatePasscodebuffer. Any subsequent confirm entry will be compared against"", causing an infinite mismatch loop.The user must be moved back to
Step.Createon mismatch so they can re-enter both passcodes.🐛 Proposed fix
Replace
resetState()withmoveToCreateStep()and explicit buffer clears on mismatch:} else { sendEvent(PasscodeEvent.PasscodeRejected) - resetState() + moveToCreateStep() + createPasscode.clear() + confirmPasscode.clear() + verifyPasscode.clear() }This is preferred over modifying
resetState()to resetactiveStepbecauseresetState()is also called on success paths (line 232) where changing the step would interfere with the state prior to sending the success event.
🤖 Fix all issues with AI agents
In
`@libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/viewmodels/PasscodeViewModel.kt`:
- Around line 239-249: When handling a passcode mismatch in
validateAndSavePasscode, explicitly reset activeStep back to the initial step
instead of changing resetState; update the mutableStateFlow in the mismatch
branch of validateAndSavePasscode to set activeStep = Step.Create (or the
appropriate initial Step enum) before/when you clear inputs and call resetState
so the UI leaves Step.Confirm and returns to the create step; do not modify
resetState itself since it's used on success paths where activeStep must remain
unchanged.
🧹 Nitpick comments (1)
libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/viewmodels/PasscodeViewModel.kt (1)
165-181:processCompletedPasscodecovers all valid intention+step combinations but has no fallback.The
whenblock has noelsebranch. If an unexpected combination occurs (e.g., due to a future code change introducing a new step or intention), the passcode input will silently be ignored after the user types 4 digits. Consider adding a loggingelsebranch for debuggability.
sahilshivekar
force-pushed
the
feature/mw-324-change-passcode
branch
from
8717dea to
97eb4b9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/viewmodels/PasscodeViewModel.kt (1)
222-249:⚠️ Potential issue | 🔴 CriticalFix: Reset
activeSteptoStep.Createwhen passcode mismatch occurs.When
createPasscodeandconfirmPasscodedon't match (line 233),resetState()clears the input buffers and dots but does not resetactiveStepback toStep.Create. The user remains stuck onStep.Confirmwith empty input. Dismissing the mismatch dialog viarestart()→resetState()still leaves the step unchanged, making it impossible for the user to re-enter a new passcode.This affects both
CREATE_PASSCODEandCHANGE_PASSCODEflows. AddactiveStep = Step.Createto the mismatch branch:Proposed fix
} else { sendEvent(PasscodeEvent.PasscodeRejected) + mutableStateFlow.update { it.copy(activeStep = Step.Create) } resetState() }
🧹 Nitpick comments (3)
feature/settings/src/commonMain/kotlin/org/mifospay/feature/settings/SettingsScreen.kt (1)
141-147: Consider extracting the string resource selection for readability.The inline ternary on Line 142 is quite long. Minor nit — could extract to a local
valfor readability, but not blocking.Optional refactor
+ val passcodeTitle = if (hasPasscode) { + stringResource(Res.string.feature_settings_change_passcode) + } else { + stringResource(Res.string.feature_settings_create_passcode) + } + SettingsCardItem( - title = if (hasPasscode) stringResource(Res.string.feature_settings_change_passcode) else stringResource(Res.string.feature_settings_create_passcode), + title = passcodeTitle, icon = vectorResource(Res.drawable.outline_pin),cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayApp.kt (1)
82-95: Avoid repeated explicit casts; usewhenwith a subject variable.
uiStateis cast toSuccessthree times on lines 84–86. Using Kotlin'swhenwith a captured subject eliminates the repetition and is more idiomatic.♻️ Suggested refactor
- val navDestination = when { - uiState is Success -> { - val userData = (uiState as Success).userData - val hasPasscode = (uiState as Success).hasPasscode - val hasSkippedPasscodeSetup = (uiState as Success).hasSkippedPasscodeSetup - when { - !userData.authenticated -> LOGIN_GRAPH - hasPasscode -> passcodeGraphRoute(Intention.LOGIN_WITH_PASSCODE) - hasSkippedPasscodeSetup -> MAIN_GRAPH - else -> passcodeGraphRoute(Intention.CREATE_PASSCODE) - } + val navDestination = when (val state = uiState) { + is Success -> when { + !state.userData.authenticated -> LOGIN_GRAPH + state.hasPasscode -> passcodeGraphRoute(Intention.LOGIN_WITH_PASSCODE) + state.hasSkippedPasscodeSetup -> MAIN_GRAPH + else -> passcodeGraphRoute(Intention.CREATE_PASSCODE) } - else -> LOGIN_GRAPH - } + else -> LOGIN_GRAPH + }libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/PasscodeStepIndicator.kt (1)
64-68: Preview function visibility should match the component'sinternalvisibility.
PasscodeStepIndicatorisinternal, butPasscodeStepIndicatorPreviewispublic. In a library module this unnecessarily expands the public API surface. Consider marking itinternal(orprivate) to keep it consistent.♻️ Suggested fix
`@Preview` `@Composable` -fun PasscodeStepIndicatorPreview() { +internal fun PasscodeStepIndicatorPreview() { PasscodeStepIndicator(activeStep = Step.Confirm) }
sahilshivekar
force-pushed
the
feature/mw-324-change-passcode
branch
2 times, most recently
from
120257f to
817511b
Compare
sahilshivekar
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayApp.kt (1)
53-53:⚠️ Potential issue | 🟡 MinorUse
valinstead ofvarforshowErrorDialog.The
MutableStateobject is never reassigned—only its.valueis mutated. Usingvaris misleading and could allow accidental reassignment, which would silently break theremembercontract.Proposed fix
- var showErrorDialog = remember { mutableStateOf<Boolean>(false) } + val showErrorDialog = remember { mutableStateOf(false) }🤖 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 53, The variable showErrorDialog is declared with var but its MutableState returned from remember is never reassigned—only its .value is mutated; change the declaration from var showErrorDialog = remember { mutableStateOf<Boolean>(false) } to use val (val showErrorDialog = remember { mutableStateOf<Boolean>(false) }) to prevent accidental reassignment and preserve the remember contract in MifosPayApp.
🧹 Nitpick comments (6)
feature/settings/build.gradle.kts (1)
26-26: Consider grouping project dependencies separately from external library dependencies.
projects.libs.mifosPasscodeis currently interleaved betweencompose.*andlibs.koin.*entries. Grouping allprojects.*dependencies together improves readability and aligns with common Gradle convention in multi-module KMP builds.♻️ Suggested dependency ordering
commonMain.dependencies { implementation(compose.ui) implementation(compose.foundation) implementation(compose.material3) implementation(compose.components.resources) implementation(compose.components.uiToolingPreview) - implementation(projects.libs.mifosPasscode) implementation(libs.koin.compose.viewmodel) implementation(libs.koin.compose) + implementation(projects.libs.mifosPasscode) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/settings/build.gradle.kts` at line 26, The dependency implementation(projects.libs.mifosPasscode) is interleaved with external libraries (compose.* and libs.koin.*); move this implementation line so all project module dependencies (projects.*) are grouped together in the dependencies block, placing implementation(projects.libs.mifosPasscode) with other projects entries and keeping compose.* and libs.koin.* together to improve readability and follow Gradle conventions.libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/viewmodels/PasscodeViewModel.kt (1)
165-181:processCompletedPasscode: unhandledwhenbranches silently no-op.
LOGIN_WITH_PASSCODEpaired withStep.CreateorStep.Confirmmatches no branch, so passcode input is silently swallowed. While these states should be unreachable in the current navigation design, an explicitelseguard would surface logic errors immediately during future refactors.♻️ Proposed defensive guard
state.intention == Intention.CHANGE_PASSCODE && state.activeStep == Step.Confirm -> validateAndSavePasscode() + + else -> { + // This combination is not a valid state; log or assert in debug builds. + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/viewmodels/PasscodeViewModel.kt` around lines 165 - 181, processCompletedPasscode currently uses a non-exhaustive when over state.intention/state.activeStep so some combinations (e.g., Intention.LOGIN_WITH_PASSCODE with Step.Create or Step.Confirm) silently no-op; add a defensive else branch to the when in processCompletedPasscode that surface unexpected combinations by logging an error and/or throwing an IllegalStateException (include the current state values) so future navigation regressions fail-fast; reference the processCompletedPasscode method and the state properties (state.intention, state.activeStep) and the enum symbols Intention.LOGIN_WITH_PASSCODE, Step.Create, Step.Confirm when implementing the guard.libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/ChangePasscodeSuccessDialog.kt (1)
29-47: Mixed theming:MaterialTheme.shapesused alongsideKptTheme.colorScheme.Line 30 uses
MaterialTheme.shapes.largewhile all colors come fromKptTheme.colorScheme. IfKptThemeexposes its own shape system, useKptTheme.shapes.largefor consistency and to avoid divergence if the design token changes.♻️ Proposed fix
- shape = MaterialTheme.shapes.large, + shape = KptTheme.shapes.large,If
KptThemedoesn't expose shapes, remove the unusedimport androidx.compose.material3.MaterialTheme.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/ChangePasscodeSuccessDialog.kt` around lines 29 - 47, The AlertDialog mixes MaterialTheme.shapes.large with KptTheme.colorScheme; update the shape usage to the KptTheme shape system by replacing MaterialTheme.shapes.large with KptTheme.shapes.large in the ChangePasscodeSuccessDialog AlertDialog call (or, if KptTheme does not expose shapes, remove the unused import of androidx.compose.material3.MaterialTheme and keep shapes omitted/defaulted) so theming is consistent across shape and color tokens.cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayApp.kt (1)
83-86: Avoid repeateduiState as Successcasts.The success state is cast three separate times. Bind it once and destructure.
Suggested refactor
uiState is Success -> { - val userData = (uiState as Success).userData - val hasPasscode = (uiState as Success).hasPasscode - val hasSkippedPasscodeSetup = (uiState as Success).hasSkippedPasscodeSetup + val successState = uiState as Success + val userData = successState.userData + val hasPasscode = successState.hasPasscode + val hasSkippedPasscodeSetup = successState.hasSkippedPasscodeSetup🤖 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 - 86, In the Success branch of the uiState when-check, avoid repeating the cast by binding the cast result once: create a local val (e.g., success = uiState as Success) and then read success.userData, success.hasPasscode, and success.hasSkippedPasscodeSetup (or destructure those properties) instead of casting uiState three times; update the pattern in the Success branch where uiState is referenced to use that single bound variable.libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/PassCodeScreen.kt (1)
107-107: Use named argument for clarity.The second positional argument to
PasscodeToolbaris not labeled, reducing readability.Suggested fix
- PasscodeToolbar(activeStep = state.activeStep, state.hasPasscode) + PasscodeToolbar(activeStep = state.activeStep, hasPasscode = state.hasPasscode)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/PassCodeScreen.kt` at line 107, PasscodeToolbar is called with an unlabeled second positional argument which reduces readability; change the call to pass the second parameter using a named argument (e.g., PasscodeToolbar(activeStep = state.activeStep, hasPasscode = state.hasPasscode)) so the meaning of state.hasPasscode is explicit; update the invocation of PasscodeToolbar in PassCodeScreen to use the named parameter.libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/PasscodeNavigation.kt (1)
58-61: Reorder parameters: place requiredintentionbefore optionaloptions.The call site uses named arguments (
intention = Intention.CHANGE_PASSCODE), so reordering is safe. This aligns with Kotlin conventions where required parameters precede optional ones, improving consistency and readability.Suggested change
-fun NavController.navigateToPasscodeScreen(options: NavOptions? = null, intention: Intention) { +fun NavController.navigateToPasscodeScreen(intention: Intention, options: NavOptions? = null) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/PasscodeNavigation.kt` around lines 58 - 61, The function signature NavController.navigateToPasscodeScreen currently declares the optional NavOptions? parameter before the required Intention parameter; reorder the parameters so the required intention: Intention comes first and the optional options: NavOptions? comes after (i.e., fun NavController.navigateToPasscodeScreen(intention: Intention, options: NavOptions? = null)), then update the function body to build the route using the intention parameter as before and verify/adjust all call sites (they already use named arguments like intention = Intention.CHANGE_PASSCODE) to match the new parameter order.
🤖 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/PasscodeNavGraph.kt`:
- Around line 36-39: The current onPasscodeFlowComplete handler always calls
navController.popBackStack() then navController.navigateToMainGraph(), which
causes a CHANGE_PASSCODE flow started from Settings to return to main home
instead of back to Settings; modify the handler (onPasscodeFlowComplete /
onChangePasscodeFlowComplete) to accept or inspect the originating flow type
(e.g., CHANGE_PASSCODE) and conditionally skip the navigateToMainGraph() call
when the flow was started from Settings (or alternatively expose an intent
parameter to the callback so the caller can decide); update the call sites so
the passcode flow provides that intent/flow enum and ensure popBackStack() is
still called but navigateToMainGraph() is only invoked for flows that should go
to the main graph.
---
Outside diff comments:
In `@cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayApp.kt`:
- Line 53: The variable showErrorDialog is declared with var but its
MutableState returned from remember is never reassigned—only its .value is
mutated; change the declaration from var showErrorDialog = remember {
mutableStateOf<Boolean>(false) } to use val (val showErrorDialog = remember {
mutableStateOf<Boolean>(false) }) to prevent accidental reassignment and
preserve the remember contract in MifosPayApp.
---
Duplicate comments:
In `@cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayApp.kt`:
- Around line 82-95: The current reactive computation of navDestination from
uiState causes NavHost graph re-creation; replace this by using a fixed
startDestination (e.g., LOGIN_GRAPH) in NavHost and remove the navDestination
variable, then drive navigation imperatively inside a LaunchedEffect keyed on
uiState: inspect uiState (Success and its fields userData, hasPasscode,
hasSkippedPasscodeSetup) and call navController.navigate(...) to the appropriate
routes (passcodeGraphRoute(Intention.LOGIN_WITH_PASSCODE),
passcodeGraphRoute(Intention.CREATE_PASSCODE), MAIN_GRAPH, or LOGIN_GRAPH) with
proper popUpTo behavior to clear the back stack as needed (same semantics as the
existing unauthorized handling). Ensure you reference and update the same
symbols used now: uiState, navController, passcodeGraphRoute, Intention,
MAIN_GRAPH, and LOGIN_GRAPH, and remove the reactive startDestination usage so
NavHost is stable.
In `@cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayViewModel.kt`:
- Around line 27-31: Replace the current mapping that reads
passcodeManager.hasPasscode.value and .hasSkippedPasscodeSetup.value inside
userDataRepository.userInfo.map with a reactive combine so uiState updates when
any source changes: use combine(userDataRepository.userInfo,
passcodeManager.hasPasscode, passcodeManager.hasSkippedPasscodeSetup) and map
the combined triple into MainUiState.Success(...) before calling stateIn; ensure
you keep the same uiState declaration and stateIn usage so all three flows
(userInfo, hasPasscode, hasSkippedPasscodeSetup) drive updates.
In
`@libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/PasscodeNavigation.kt`:
- Around line 27-30: The fromValue function currently falls back to
CREATE_PASSCODE when the input is invalid, which can mistakenly route users into
the create flow; change the fallback to LOGIN_WITH_PASSCODE so malformed or
unknown values default to the safer verification path by returning
LOGIN_WITH_PASSCODE instead of CREATE_PASSCODE in the companion object fromValue
implementation that uses Intention.entries.firstOrNull { it.value == value }.
In
`@libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/PassCodeScreen.kt`:
- Around line 118-121: The success dialog dismissal currently only dispatches
PasscodeAction.DismissChangePasscodeSuccessDialog from
ChangePasscodeSuccessDialog which hides the dialog but never finishes the flow;
update the dismissal handler to also trigger the passcode flow completion by
invoking the existing onPasscodeFlowComplete path (either dispatch a new action
that the ViewModel maps to calling onPasscodeFlowComplete() or call the
ViewModel method directly) so that when ChangePasscodeSuccessDialog’s onDismiss
runs it both hides the dialog and completes the passcode flow (refer to
ChangePasscodeSuccessDialog, PasscodeAction.DismissChangePasscodeSuccessDialog,
viewModel.trySendAction and onPasscodeFlowComplete).
In
`@libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/utility/Step.kt`:
- Around line 12-16: Change the Step enum to make the index immutable and remove
the redundant constructor parameter: update "enum class Step(var index: Int)" to
use a val or remove the index parameter entirely since the enum's ordinal
already provides the same values; then in PasscodeStepIndicator (where you
reference activeStep.index, e.g., at the use in PasscodeStepIndicator.kt)
replace activeStep.index with activeStep.ordinal and remove any
constructor/property that stored the duplicate index so the enum is simplified
and immutable.
In
`@libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/viewmodels/PasscodeViewModel.kt`:
- Around line 233-235: The mismatch branch in validateAndSavePasscode() sends
PasscodeEvent.PasscodeRejected and calls resetState() but leaves activeStep at
Step.Confirm, locking the UI; modify that branch so after sending
PasscodeEvent.PasscodeRejected you explicitly set activeStep = Step.Create
(before or after calling resetState()) so the user is returned to the
create-passcode step; keep resetState() unchanged because it’s also used on the
success path.
---
Nitpick comments:
In `@cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayApp.kt`:
- Around line 83-86: In the Success branch of the uiState when-check, avoid
repeating the cast by binding the cast result once: create a local val (e.g.,
success = uiState as Success) and then read success.userData,
success.hasPasscode, and success.hasSkippedPasscodeSetup (or destructure those
properties) instead of casting uiState three times; update the pattern in the
Success branch where uiState is referenced to use that single bound variable.
In `@feature/settings/build.gradle.kts`:
- Line 26: The dependency implementation(projects.libs.mifosPasscode) is
interleaved with external libraries (compose.* and libs.koin.*); move this
implementation line so all project module dependencies (projects.*) are grouped
together in the dependencies block, placing
implementation(projects.libs.mifosPasscode) with other projects entries and
keeping compose.* and libs.koin.* together to improve readability and follow
Gradle conventions.
In
`@libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/ChangePasscodeSuccessDialog.kt`:
- Around line 29-47: The AlertDialog mixes MaterialTheme.shapes.large with
KptTheme.colorScheme; update the shape usage to the KptTheme shape system by
replacing MaterialTheme.shapes.large with KptTheme.shapes.large in the
ChangePasscodeSuccessDialog AlertDialog call (or, if KptTheme does not expose
shapes, remove the unused import of androidx.compose.material3.MaterialTheme and
keep shapes omitted/defaulted) so theming is consistent across shape and color
tokens.
In
`@libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/PasscodeNavigation.kt`:
- Around line 58-61: The function signature
NavController.navigateToPasscodeScreen currently declares the optional
NavOptions? parameter before the required Intention parameter; reorder the
parameters so the required intention: Intention comes first and the optional
options: NavOptions? comes after (i.e., fun
NavController.navigateToPasscodeScreen(intention: Intention, options:
NavOptions? = null)), then update the function body to build the route using the
intention parameter as before and verify/adjust all call sites (they already use
named arguments like intention = Intention.CHANGE_PASSCODE) to match the new
parameter order.
In
`@libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/PassCodeScreen.kt`:
- Line 107: PasscodeToolbar is called with an unlabeled second positional
argument which reduces readability; change the call to pass the second parameter
using a named argument (e.g., PasscodeToolbar(activeStep = state.activeStep,
hasPasscode = state.hasPasscode)) so the meaning of state.hasPasscode is
explicit; update the invocation of PasscodeToolbar in PassCodeScreen to use the
named parameter.
In
`@libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/viewmodels/PasscodeViewModel.kt`:
- Around line 165-181: processCompletedPasscode currently uses a non-exhaustive
when over state.intention/state.activeStep so some combinations (e.g.,
Intention.LOGIN_WITH_PASSCODE with Step.Create or Step.Confirm) silently no-op;
add a defensive else branch to the when in processCompletedPasscode that surface
unexpected combinations by logging an error and/or throwing an
IllegalStateException (include the current state values) so future navigation
regressions fail-fast; reference the processCompletedPasscode method and the
state properties (state.intention, state.activeStep) and the enum symbols
Intention.LOGIN_WITH_PASSCODE, Step.Create, Step.Confirm when implementing the
guard.
sahilshivekar
force-pushed
the
feature/mw-324-change-passcode
branch
from
817511b to
705a743
Compare
2 participants
Issue Fix
Jira Task: 324
Screen recordings
Before
the passcode change flow does not proceed
video_1280.mp4
After
Recording 1: User with existing passcode clicks "Change Passcode" and goes through the complete flow:
mw-324-change-passcode-from-settings.mp4
Recording 2: User logs in, skips passcode setup, then navigates to settings showing "Create Passcode" button and goes through the complete flow Create passcode → Confirm passcode → Success dialog
mw-324-create-passcode-from-settings.mp4
Description
Key Features
1. Change Passcode Flow
2. Dynamic Settings UI
3. Intention-Based Navigation
Intentionenum with three modes:LOGIN_WITH_PASSCODE: Verify existing passcode during app loginCREATE_PASSCODE: Initial passcode setup (with skip option)CHANGE_PASSCODE: Change existing passcode (3-step flow)4. Skip Passcode Tracking
hasSkippedPasscodeSetuppreference to track user choiceSummary by CodeRabbit
New Features
Refactor
Chores