feat(passcode): implement change passcode flow (MW-324) #1949
Conversation
📝 WalkthroughWalkthroughRoutes and passcode flows were reworked to an intention-driven multi-step model (Verify/Create/Confirm). Passcode routing moved from login to app-startup decision logic; skip-state persisted; view models, navigation APIs, UI components, DI scopes, and resources updated to support the new flows. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App Startup
participant VM as MifosPayViewModel
participant Decider as MifosPayApp (Decision)
participant NavHost as MifosNavHost
participant Passcode as PasscodeModule
participant Main as Main Graph
App->>VM: request auth + passcode state
VM-->>App: MainUiState.Success(user, hasPasscode, hasSkippedPasscodeSetup)
App->>Decider: evaluate state
alt Not authenticated
Decider->>NavHost: navigate(LOGIN_GRAPH)
else Authenticated & hasPasscode
Decider->>NavHost: navigate(passcodeGraphRoute(LOGIN_WITH_PASSCODE))
NavHost->>Passcode: start Verify flow
else Authenticated & no passcode & not skipped
Decider->>NavHost: navigate(passcodeGraphRoute(CREATE_PASSCODE))
NavHost->>Passcode: start Create → Confirm flow
else Authenticated & skipped
Decider->>NavHost: navigate(MAIN_GRAPH)
NavHost->>Main: show main app
end
Note over Passcode,NavHost: onPasscodeFlowComplete -> NavHost navigates to MAIN_GRAPH
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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
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.
...os-passcode/src/commonMain/kotlin/org/mifos/library/passcode/viewmodels/PasscodeViewModel.kt
Show resolved
Hide resolved
...os-passcode/src/commonMain/kotlin/org/mifos/library/passcode/viewmodels/PasscodeViewModel.kt
Show resolved
Hide resolved
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.
libs/mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/utility/Step.kt
Show resolved
Hide resolved
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.
cmp-shared/src/commonMain/kotlin/org/mifospay/shared/MifosPayApp.kt
Outdated
Show resolved
Hide resolved
.../mifos-passcode/src/commonMain/kotlin/org/mifos/library/passcode/component/PasscodeHeader.kt
Show resolved
Hide resolved
...os-passcode/src/commonMain/kotlin/org/mifos/library/passcode/viewmodels/PasscodeViewModel.kt
Show resolved
Hide resolved
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.
...os-passcode/src/commonMain/kotlin/org/mifos/library/passcode/viewmodels/PasscodeViewModel.kt
Show resolved
Hide resolved
...os-passcode/src/commonMain/kotlin/org/mifos/library/passcode/viewmodels/PasscodeViewModel.kt
Show resolved
Hide resolved
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.
1 participant
Issue Fix
Jira Task: 324
Screen recordings
Recording 1: User with existing passcode clicks "Change Passcode" and goes through the complete flow:
change_passcode_correctly_working.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
create_passcode_instead_of_change_passcode_if_no_passcode_set.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
Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.