feat(core): implement automatic session lock after 5 minutes of inactivity#3074
feat(core): implement automatic session lock after 5 minutes of inactivity#3074mena-rizkalla wants to merge 17 commits intoopenMF:developmentfrom
Conversation
- Add SessionManager to track user inactivity - Add SessionHandler to detect touch events - Integrate logout trigger in ComposeApp - Stop session timer on Auth screens to prevent loops
📝 WalkthroughWalkthroughAdds a SessionManager with heartbeat-based timeout, integrates lifecycle and session-expiry handling into ComposeApp and navigation, wraps UI in a SessionHandler, updates passcode flow to callback-based unlocking, and wires SessionManager into auth/home viewmodels and DI. Changes
Sequence DiagramsequenceDiagram
participant User
participant ComposeApp as ComposeApp (Lifecycle)
participant SessionManager as SessionManager
participant SessionHandler as SessionHandler
participant RootNav as RootNavScreen
participant Passcode as PasscodeFlow
User->>ComposeApp: Launch / ON_START
ComposeApp->>SessionManager: collect isExpired
ComposeApp->>SessionManager: startSession (via Home or passcode)
Note over SessionManager: heartbeat checks every 30s\nupdate lastInteractionTime on userInteracted()
User->>SessionHandler: Touch input
SessionHandler->>SessionManager: userInteracted()
Note over SessionManager: inactivity >= TIMEOUT_SESSION_MS
SessionManager-->>ComposeApp: isExpired = true
ComposeApp->>SessionHandler: render expired state (consume input)
User->>SessionHandler: Choose to re-authenticate
SessionHandler->>RootNav: navigate to passcode
RootNav->>Passcode: present verification
Passcode->>Passcode: verify stored passcode
alt success
Passcode->>SessionManager: startSession()
Passcode->>RootNav: onPasscodeVerified -> navigate to authenticated
end
User->>ComposeApp: App background (ON_STOP)
ComposeApp->>SessionManager: stopSession() / dispatch LockApp
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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: 2
🤖 Fix all issues with AI agents
In
`@core/common/src/commonMain/kotlin/org/mifos/mobile/core/common/SessionManager.kt`:
- Around line 23-30: SessionManager uses shared mutable vars lastInteractionTime
and isMonitoring across Dispatchers.Default (heartbeat coroutine) and
Dispatchers.Main (userInteracted() and startSession()), causing race/visibility
issues; replace lastInteractionTime with an AtomicLong and isMonitoring with an
AtomicBoolean, update all reads to use .get()/getValue()/load() and writes to
use .set()/store() (or the Kotlin atomic equivalents you choose), and modify
userInteracted(), startSession(), and the heartbeat coroutine references to use
those atomic accessors so all mutations/reads are thread-safe.
In
`@core/ui/src/commonMain/kotlin/org/mifos/mobile/core/ui/utils/SessionHandler.kt`:
- Around line 35-46: The session timeout only tracks pointer events in the Box
using modifier.pointerInput and misses keyboard/IME/focus interactions; update
the composable that wraps Box (where modifier.pointerInput and
sessionManager.userInteracted are used) to also attach focusable() and add
Modifier.onKeyEvent and Modifier.onPreviewKeyEvent handlers that call
sessionManager.userInteracted(), listen for focus changes via onFocusChanged to
call userInteracted(), and ensure TextField/TextInput usages register callbacks
(e.g., KeyboardActions or text change listeners) that also call
sessionManager.userInteracted() so keyboard/IME and accessibility interactions
reset the inactivity timer.
🧹 Nitpick comments (4)
core/common/src/commonMain/kotlin/org/mifos/mobile/core/common/SessionManager.kt (1)
32-38: Potential for multiple concurrent heartbeats on rapid start/stop cycles.If
stopSession()is called andstartSession()is called again before the previous heartbeat loop exits (within its 30-second delay), two heartbeat coroutines will briefly run concurrently. Consider tracking and cancelling the heartbeatJob.♻️ Suggested fix to track and cancel the heartbeat job
class SessionManager { private val scope = CoroutineScope(Dispatchers.Default + SupervisorJob()) + private var heartbeatJob: Job? = null // ... other fields ... fun startSession() { if (isMonitoring) return isMonitoring = true lastInteractionTime = Clock.System.now().toEpochMilliseconds() - startHeartbeat() + heartbeatJob = startHeartbeat() } fun stopSession() { isMonitoring = false + heartbeatJob?.cancel() + heartbeatJob = null } - private fun startHeartbeat() { - scope.launch { + private fun startHeartbeat(): Job { + return scope.launch { // ... existing logic ... } } }Also applies to: 50-65
core/common/src/commonMain/kotlin/org/mifos/mobile/core/common/di/DispatchersModule.kt (1)
29-29: LGTM! SessionManager singleton registered correctly.The singleton scope is appropriate for app-wide session tracking. As per the coding guidelines, the DI binding is correctly placed in the
di/package.Optionally, consider creating a dedicated
SessionModule.ktin the future if session-related dependencies grow, to maintain separation of concerns from dispatcher bindings.feature/auth/src/commonMain/kotlin/org/mifos/mobile/feature/auth/registration/RegistrationViewModel.kt (1)
57-60: Minor: Consider adding a blank line for readability.There's no blank line between the
initblock and thevalidationJobproperty declaration.✏️ Suggested formatting
init { sessionManager.stopSession() } + private var validationJob: Job? = nullfeature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeViewModel.kt (1)
49-49: Consider simplifying sessionManager to a parameter-only dependency.The
sessionManageris stored as aprivate valbut is only used ininit. For consistency with other ViewModels in this PR (e.g.,UploadIdViewModel), consider making it a constructor parameter without storage.♻️ Suggested refactor
internal class HomeViewModel( private val homeRepositoryImpl: HomeRepository, private val networkMonitor: NetworkMonitor, - private val sessionManager: SessionManager, + sessionManager: SessionManager, userPreferencesRepositoryImpl: UserPreferencesRepository, ) : BaseViewModel<HomeState, HomeEvent, HomeAction>(Also applies to: 62-64
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
cmp-navigation/src/commonMain/kotlin/cmp/navigation/ComposeApp.ktcmp-navigation/src/commonMain/kotlin/cmp/navigation/ComposeAppViewModel.ktcore/common/src/commonMain/kotlin/org/mifos/mobile/core/common/Constants.ktcore/common/src/commonMain/kotlin/org/mifos/mobile/core/common/SessionManager.ktcore/common/src/commonMain/kotlin/org/mifos/mobile/core/common/di/DispatchersModule.ktcore/ui/build.gradle.ktscore/ui/src/commonMain/kotlin/org/mifos/mobile/core/ui/utils/SessionHandler.ktfeature/auth/src/commonMain/kotlin/org/mifos/mobile/feature/auth/login/LoginViewModel.ktfeature/auth/src/commonMain/kotlin/org/mifos/mobile/feature/auth/otpAuthentication/OtpAuthenticationViewModel.ktfeature/auth/src/commonMain/kotlin/org/mifos/mobile/feature/auth/recoverPassword/RecoverPasswordViewModel.ktfeature/auth/src/commonMain/kotlin/org/mifos/mobile/feature/auth/registration/RegistrationViewModel.ktfeature/auth/src/commonMain/kotlin/org/mifos/mobile/feature/auth/setNewPassword/SetPasswordViewModel.ktfeature/auth/src/commonMain/kotlin/org/mifos/mobile/feature/auth/uploadId/UploadIdViewmodel.ktfeature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeViewModel.kt
🧰 Additional context used
📓 Path-based instructions (1)
**/{src,}/**/di/**/*.kt
📄 CodeRabbit inference engine (CLAUDE.md)
Define Koin modules in the
di/package for each module
Files:
core/common/src/commonMain/kotlin/org/mifos/mobile/core/common/di/DispatchersModule.kt
🧠 Learnings (4)
📚 Learning: 2026-01-07T10:05:07.700Z
Learnt from: CR
Repo: openMF/mifos-mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-07T10:05:07.700Z
Learning: Use ViewModels with StateFlow/SharedFlow for state management in feature modules
Applied to files:
feature/auth/src/commonMain/kotlin/org/mifos/mobile/feature/auth/uploadId/UploadIdViewmodel.ktfeature/auth/src/commonMain/kotlin/org/mifos/mobile/feature/auth/login/LoginViewModel.ktfeature/auth/src/commonMain/kotlin/org/mifos/mobile/feature/auth/otpAuthentication/OtpAuthenticationViewModel.ktfeature/auth/src/commonMain/kotlin/org/mifos/mobile/feature/auth/setNewPassword/SetPasswordViewModel.ktfeature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeViewModel.ktcmp-navigation/src/commonMain/kotlin/cmp/navigation/ComposeApp.kt
📚 Learning: 2026-01-07T10:05:07.700Z
Learnt from: CR
Repo: openMF/mifos-mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-07T10:05:07.700Z
Learning: Use Koin for dependency injection across all platforms
Applied to files:
core/ui/build.gradle.ktscore/common/src/commonMain/kotlin/org/mifos/mobile/core/common/di/DispatchersModule.kt
📚 Learning: 2026-01-07T10:05:07.700Z
Learnt from: CR
Repo: openMF/mifos-mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-07T10:05:07.700Z
Learning: Applies to **/{src,}/**/di/**/*.kt : Define Koin modules in the `di/` package for each module
Applied to files:
core/common/src/commonMain/kotlin/org/mifos/mobile/core/common/di/DispatchersModule.kt
📚 Learning: 2026-01-07T10:05:07.700Z
Learnt from: CR
Repo: openMF/mifos-mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-07T10:05:07.700Z
Learning: Use Jetbrains Compose Navigation for cross-platform navigation with navigation graphs defined in `cmp-navigation/`
Applied to files:
cmp-navigation/src/commonMain/kotlin/cmp/navigation/ComposeApp.kt
🧬 Code graph analysis (1)
cmp-navigation/src/commonMain/kotlin/cmp/navigation/ComposeApp.kt (2)
core/ui/src/commonMain/kotlin/org/mifos/mobile/core/ui/utils/SessionHandler.kt (1)
SessionHandler(22-50)core/ui/src/commonMain/kotlin/org/mifos/mobile/core/ui/utils/NetworkBannerStatus.kt (1)
NetworkBanner(35-83)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR Checks KMP / Static Analysis Check
🔇 Additional comments (13)
core/ui/build.gradle.kts (1)
35-35: LGTM!The dependency on
core.commonis correctly added to supportSessionHandler's usage ofSessionManager.core/common/src/commonMain/kotlin/org/mifos/mobile/core/common/Constants.kt (1)
98-99: LGTM!The timeout constant correctly represents 5 minutes in milliseconds. Consider making this configurable via remote config or build flavors in the future if different timeout durations are needed per environment.
feature/auth/src/commonMain/kotlin/org/mifos/mobile/feature/auth/recoverPassword/RecoverPasswordViewModel.kt (1)
35-44: LGTM! Session timer correctly stopped during authentication flow.Calling
stopSession()in theinitblock ensures the inactivity timer doesn't trigger a logout while the user is on the password recovery screen. This aligns with the pattern used across other auth-related ViewModels. The Koin DI module properly injectsSessionManagerintoRecoverPasswordViewModelthrough the standard constructor injection mechanism.feature/auth/src/commonMain/kotlin/org/mifos/mobile/feature/auth/login/LoginViewModel.kt (1)
23-23: LGTM! Correctly stops session timer on login screen.The
sessionManager.stopSession()call ininitensures the inactivity timer doesn't run when the user is on the login screen (unauthenticated state). The parameter is appropriately not stored since it's only needed for this initialization side effect.Also applies to: 34-34, 43-43
feature/auth/src/commonMain/kotlin/org/mifos/mobile/feature/auth/registration/RegistrationViewModel.kt (1)
30-30: LGTM! Consistent session management pattern.The implementation correctly stops the session timer during registration, following the same pattern established in
LoginViewModel. This prevents the inactivity logout from triggering while a user is signing up.Also applies to: 52-52, 57-59
feature/auth/src/commonMain/kotlin/org/mifos/mobile/feature/auth/setNewPassword/SetPasswordViewModel.kt (1)
28-28: LGTM! Consistent with auth ViewModel pattern.The
SetPasswordViewModelcorrectly implements the session management pattern, stopping the session timer when users are on the set password screen. This is appropriate since users at this point are in the password recovery flow (unauthenticated).Also applies to: 38-46
cmp-navigation/src/commonMain/kotlin/cmp/navigation/ComposeAppViewModel.kt (2)
272-273: Logout action correctly placed in AppAction.The
Logoutaction is appropriately defined as a publicdata objectwithinAppAction, making it accessible from the UI layer (SessionHandler) while keeping internal actions properly encapsulated in theInternalsealed class.
149-158: LGTM! Logout action properly integrated with navigation.The
AppAction.Logouthandling correctly delegates touserPreferencesRepository.logOut()within a coroutine scope. The logout flow is properly connected to navigation through the authentication state system:
handleUserInactivityLogout()→userPreferencesRepository.logOut()→clearInfo()setsisAuthenticated = falseAuthenticationUserRepository.authState(StateFlow) automatically updates toAuthState.UnauthenticatedRootNavViewModelobservesauthStateand updates navigation state toRootNavState.Authwhen unauthenticatedThis aligns with the session timeout feature where
SessionHandlertriggers this action upon inactivity, and the user is automatically navigated back to the login screen.feature/auth/src/commonMain/kotlin/org/mifos/mobile/feature/auth/otpAuthentication/OtpAuthenticationViewModel.kt (1)
34-34: LGTM! Session management integrated into existing init block.The
stopSession()call is correctly added to the existinginitblock, ensuring the inactivity timer is stopped during OTP verification. This follows the consistent pattern established across all authentication-related ViewModels.Also applies to: 43-43, 48-49
feature/auth/src/commonMain/kotlin/org/mifos/mobile/feature/auth/uploadId/UploadIdViewmodel.kt (1)
40-49: LGTM - correct pattern for preventing logout during authentication.Calling
stopSession()on auth-related screens prevents the inactivity timer from triggering logout while the user is in the authentication flow. This aligns with the PR's loop prevention strategy.cmp-navigation/src/commonMain/kotlin/cmp/navigation/ComposeApp.kt (1)
66-93: LGTM - SessionHandler integration is correct.The
SessionHandlerproperly wraps the entire UI to capture touch events globally. The logout callback correctly dispatchesAppAction.Logoutto the ViewModel. The structure maintains proper layering with the network banner and navigation screen inside.core/ui/src/commonMain/kotlin/org/mifos/mobile/core/ui/utils/SessionHandler.kt (1)
29-33: LGTM - logout event collection is correctly implemented.Using
LaunchedEffect(Unit)withcollectLatestis appropriate for collecting the logout flow. The effect will survive recompositions and be cancelled when the composable leaves composition.feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeViewModel.kt (1)
62-65: LGTM - session lifecycle integration is correct.Starting the session when the user reaches the home screen (authenticated state) is appropriate. The
SessionManager.startSession()method safely handles multiple calls through an idempotency guard (if (isMonitoring) return), making it resilient to ViewModel recreation on configuration changes.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
core/common/src/commonMain/kotlin/org/mifos/mobile/core/common/SessionManager.kt
Outdated
Show resolved
Hide resolved
core/ui/src/commonMain/kotlin/org/mifos/mobile/core/ui/utils/SessionHandler.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
core/common/src/commonMain/kotlin/org/mifos/mobile/core/common/SessionManager.kt (2)
27-40: Consider addingextraBufferCapacityto the SharedFlow.
MutableSharedFlow<Unit>()defaults toextraBufferCapacity = 0, which meansemit()will suspend if there are no active collectors at emission time. If the collecting composable is temporarily inactive (e.g., app backgrounded, configuration change), the logout emission could hang until a collector resubscribes.♻️ Suggested fix
- private val _logoutEvent = MutableSharedFlow<Unit>() + private val _logoutEvent = MutableSharedFlow<Unit>(extraBufferCapacity = 1)Alternatively, use
tryEmit(Unit)instartHeartbeat()which returnsfalseinstead of suspending if the buffer is full.
64-79: Consider extracting the heartbeat interval to a constant.The 30-second polling interval is hardcoded while the timeout value comes from
Constants. For consistency and easier tuning, consider extracting it.♻️ Suggested change
In
Constants.kt:const val HEARTBEAT_INTERVAL_MS = 30_000LThen in
startHeartbeat():- delay(30_000) + delay(Constants.HEARTBEAT_INTERVAL_MS)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/common/src/commonMain/kotlin/org/mifos/mobile/core/common/SessionManager.kt
🔇 Additional comments (3)
core/common/src/commonMain/kotlin/org/mifos/mobile/core/common/SessionManager.kt (3)
1-26: License and imports look good.The experimental API opt-ins are handled appropriately at the usage sites rather than file-level, which is a reasonable pattern.
50-55: LGTM!Correct use of atomic operations: checks
isMonitoringbefore updatinglastInteractionTime. This efficiently handles high-frequency touch events without race conditions.
57-62: Implementation is correct; synchronization concern noted above.The
stopSession()logic properly stops monitoring and cancels the heartbeat job. The race condition withheartbeatJobwas addressed in the earlier comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
core/common/src/commonMain/kotlin/org/mifos/mobile/core/common/SessionManager.kt
Show resolved
Hide resolved
- Added `session_expired_title` and `session_expired_message` strings in English and Spanish. - Refactored `SessionManager` to use `isExpired` `StateFlow` instead of a logout event flow. - Added `MifosBasicDialog` in `ComposeApp` to notify users when their session has expired. - Updated `SessionHandler` to intercept and consume pointer and key events when the session is expired. - Simplified `SessionHandler` by removing the explicit `onLogout` callback in favor of managing logout via the expiration dialog.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@core/ui/src/commonMain/kotlin/org/mifos/mobile/core/ui/utils/SessionHandler.kt`:
- Around line 33-47: The pointerInput coroutine in the Box currently keys on
Unit, capturing a stale isExpired value; change the pointerInput key from Unit
to isExpired so the coroutine is restarted when isExpired changes, ensuring the
awaitPointerEventScope block inside pointerInput sees the current isExpired and
continues to call sessionManager.userInteracted() only when appropriate (and
still consumes events when isExpired is true).
♻️ Duplicate comments (2)
core/ui/src/commonMain/kotlin/org/mifos/mobile/core/ui/utils/SessionHandler.kt (1)
48-57: Key events may not be captured without focus + IME activity still untracked.
onPreviewKeyEventwon’t reliably fire unless the container participates in focus. Addfocusable()(or a focus target) so hardware‑keyboard users aren’t timed out while interacting. Consider extending activity tracking to IME/text input as well.🔧 Suggested change
-import androidx.compose.ui.input.key.onPreviewKeyEvent +import androidx.compose.foundation.focusable +import androidx.compose.ui.input.key.onPreviewKeyEvent ... - }.onPreviewKeyEvent { event -> + }.focusable().onPreviewKeyEvent { event -> if (event.type == KeyEventType.KeyUp) { if (isExpired) { return@onPreviewKeyEvent truecore/common/src/commonMain/kotlin/org/mifos/mobile/core/common/SessionManager.kt (1)
29-63: Potential race onheartbeatJoblifecycle (orphan job risk).
heartbeatJobis mutated from potentially different threads without synchronization, so a stop can interleave betweencompareAndSetand assignment, leaving a job untracked. Consider guarding start/stop with a Mutex or an atomic reference to keep lifecycle consistent.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmp-navigation/src/commonMain/composeResources/values-es/strings.xmlcmp-navigation/src/commonMain/composeResources/values/strings.xmlcmp-navigation/src/commonMain/kotlin/cmp/navigation/ComposeApp.ktcore/common/src/commonMain/kotlin/org/mifos/mobile/core/common/SessionManager.ktcore/ui/src/commonMain/kotlin/org/mifos/mobile/core/ui/utils/SessionHandler.kt
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-01-07T10:05:07.700Z
Learnt from: CR
Repo: openMF/mifos-mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-07T10:05:07.700Z
Learning: Use Jetbrains Compose Navigation for cross-platform navigation with navigation graphs defined in `cmp-navigation/`
Applied to files:
cmp-navigation/src/commonMain/kotlin/cmp/navigation/ComposeApp.kt
📚 Learning: 2026-01-07T10:05:07.700Z
Learnt from: CR
Repo: openMF/mifos-mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-07T10:05:07.700Z
Learning: Use ViewModels with StateFlow/SharedFlow for state management in feature modules
Applied to files:
cmp-navigation/src/commonMain/kotlin/cmp/navigation/ComposeApp.kt
🧬 Code graph analysis (1)
cmp-navigation/src/commonMain/kotlin/cmp/navigation/ComposeApp.kt (3)
core/ui/src/commonMain/kotlin/org/mifos/mobile/core/ui/utils/SessionHandler.kt (1)
SessionHandler(25-61)core/ui/src/commonMain/kotlin/org/mifos/mobile/core/ui/utils/NetworkBannerStatus.kt (1)
NetworkBanner(35-83)cmp-navigation/src/commonMain/kotlin/cmp/navigation/rootnav/RootNavScreen.kt (1)
RootNavScreen(49-131)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR Checks KMP / Static Analysis Check
🔇 Additional comments (3)
cmp-navigation/src/commonMain/composeResources/values/strings.xml (1)
17-18: LGTM — clear, user-facing messaging.cmp-navigation/src/commonMain/composeResources/values-es/strings.xml (1)
17-18: LGTM — localization aligns with the English source.cmp-navigation/src/commonMain/kotlin/cmp/navigation/ComposeApp.kt (1)
73-115: Nice integration of session handling and UI shell.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
core/ui/src/commonMain/kotlin/org/mifos/mobile/core/ui/utils/SessionHandler.kt
Outdated
Show resolved
Hide resolved
|
Could you please add a screen which says "Your Session has been timed out. Please login again." and add a "Log In" button which takes you to the login screen. This screen informs the user what just happened. Here's an example: 
The branding should be that of Mifos. |
cmp-navigation/src/commonMain/kotlin/cmp/navigation/ComposeApp.kt
Outdated
Show resolved
Hide resolved
It already added you can watch the video at second 20 |
|
Yes, I saw the video few days back so couldn't recall it if it was there or may be I missed it. I should have watched it again before commenting. Currently, a screen is visible in the background from where it logged out, wouldn't it be better to show the log in screen in the background so that no account related information (like account balance etc.) is visible? Or you could simply add an opaque screen which covers the entire screen area. |
… to `SessionHandler` when expired
|
Since the dialog is telling the user you have been logged out, shouldn't we show the login screen with the dialog overlay without any blur? Isn't the log out happening before the dialog pops up? |
I intentionally chose a 'Soft Timeout' pattern here rather than an immediate redirect for better UX. Also If we navigated to the Login screen first, we would need to pass arguments to the Login screen to trigger a dialog there, which complicates the navigation logic and risks the user missing the context of why they were logged out. The Flow: Timer ends: App freezes + Blur applies (Security layer) + Dialog appears. User clicks OK: Then the actual logout (clearing data & navigation) triggers, sending them to the Login screen. I also can replace the blur with the background color but honestly i prefer the blur |
- Added `SessionStorage` interface and `DatastoreSessionStorage` implementation to persist last interaction time. - Updated `SessionManager` to track session across app restarts (cold starts) using `SessionStorage`. - Introduced `shouldShowDialog` to `SessionManager` to control whether to show an expiration dialog or logout silently based on the session state. - Added a throttled disk-save mechanism for session time to minimize I/O overhead. - Enhanced `ComposeApp` with a lifecycle observer to trigger manual session expiration checks on app start. - Refactored `SessionHandler` to consistently consume pointer events when the session is expired. - Updated `UserPreferencesDataSource` and repository to support storing and retrieving the last session timestamp.
…n management - Refactored `ComposeAppViewModel` to replace `Logout` with `LockApp` and `SessionExpired` actions, updating `userPreferencesRepository` to manage unlock state instead of full logout. - Added background detection in `ComposeApp` to trigger `LockApp` when the app is backgrounded. - Updated `SessionManager` to simplify expiration logic, removing disk-based time tracking and cold start dialog suppression. - Enhanced `PasscodeViewModel` with a `Verify` mode to authenticate users using their stored passcode and update the application's lock status. - Introduced `passcodeNavGraph` to manage passcode verification flows and added success strings for the unlock process. - Modified `RootNavScreen` to handle `UserLocked` and `UserUnlocked` states, ensuring the app navigates to the passcode screen when locked.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmp-navigation/src/commonMain/kotlin/cmp/navigation/rootnav/RootNavViewModel.kt (1)
32-57: Prevent a brief “unlocked” state beforesetIsUnlocked(false)completes.Because
setIsUnlocked(false)is launched asynchronously, the initialsettingsState/authStateemissions can still reflect a previously-unlocked state, briefly navigating toUserUnlockedbefore being locked. That can expose sensitive UI for a moment. Consider awaiting the reset before starting the collectors.🔧 Suggested ordering fix
init { - viewModelScope.launch { - userPreferencesRepository.setIsUnlocked(false) - } - - viewModelScope.launch { - userDataRepository.authState - .collect { authState -> - if (authState is AuthState.Unauthenticated) { - if (mutableStateFlow.value !is RootNavState.Auth) { - mutableStateFlow.update { RootNavState.Auth } - } - } - } - } - combine( - userDataRepository.authState, - userDataRepository.settingsState, - ) { authState, settingsData -> - UserStateUpdateReceive( - authState = authState, - settingsData = settingsData, - ) - }.onEach(::handleAction) - .launchIn(viewModelScope) + viewModelScope.launch { + userPreferencesRepository.setIsUnlocked(false) + + launch { + userDataRepository.authState + .collect { authState -> + if (authState is AuthState.Unauthenticated) { + if (mutableStateFlow.value !is RootNavState.Auth) { + mutableStateFlow.update { RootNavState.Auth } + } + } + } + } + + combine( + userDataRepository.authState, + userDataRepository.settingsState, + ) { authState, settingsData -> + UserStateUpdateReceive( + authState = authState, + settingsData = settingsData, + ) + }.onEach(::handleAction) + .launchIn(this) + } }
🤖 Fix all issues with AI agents
In `@cmp-navigation/src/commonMain/kotlin/cmp/navigation/ComposeApp.kt`:
- Around line 64-81: The DisposableEffect currently calls
sessionManager.stopSession() in onDispose which disables session monitoring
across Activity/host recreation while the ViewModel (and authentication state)
is retained; to fix, stop removing/terminating the session on Compose host
dispose and instead either call sessionManager.stopSession() only on explicit
logout/auth state transition, or add logic to restart the session when the
authenticated UI/lifecycle is re-attached (e.g., re-invoke
HomeViewModel.startSession() or sessionManager.startSession() from the
DisposableEffect observer on lifecycle ON_START when the user is authenticated);
update the code around DisposableEffect, its onDispose, the
LifecycleEventObserver, and references to sessionManager.stopSession() and
HomeViewModel.startSession() accordingly.
In
`@cmp-navigation/src/commonMain/kotlin/cmp/navigation/rootnav/RootNavScreen.kt`:
- Around line 83-95: The onPasscodeVerified callback currently branches on
navController.previousBackStackEntry causing inconsistent behavior between entry
points; change the callback in RootNavScreen so it always calls
sessionManager.startSession() and then navigates to AuthenticatedGraphRoute
(using navController.navigate(AuthenticatedGraphRoute) with appropriate popUpTo
to clear the stack) instead of conditionally popping back to a previous entry,
and ensure entry-point callers (e.g., UserLocked path which already uses
popUpTo(navController.graph.id) { inclusive = true } and LoginScreen's
navigateToPasscodeScreen) handle backstack setup rather than relying on the
conditional in onPasscodeVerified.
♻️ Duplicate comments (1)
core/common/src/commonMain/kotlin/org/mifos/mobile/core/common/SessionManager.kt (1)
50-60: Potential race: asyncstartSessioncan resurrect monitoring afterstopSession.Because the initialization runs inside
scope.launch, a quickstopSession()can execute first, then the coroutine still setslastInteractionTimeand starts a heartbeat job. Consider doing the initialization synchronously or re-checkingisMonitoringunder a lock before starting the heartbeat.Kotlin coroutines launch scheduling and Job.cancel cooperative cancellation timing🛠️ Suggested fix (inline initialization)
- scope.launch { - val now = Clock.System.now().toEpochMilliseconds() - lastInteractionTime.store(now) - if (!_isExpired.value) { - heartbeatJob = startHeartbeat() - } - } + val now = Clock.System.now().toEpochMilliseconds() + lastInteractionTime.store(now) + if (!_isExpired.value) { + heartbeatJob = startHeartbeat() + }
🧹 Nitpick comments (2)
cmp-navigation/src/commonMain/kotlin/cmp/navigation/ComposeAppViewModel.kt (1)
156-165: Consider consolidating duplicate handler implementations.
handleLockApp()andhandleUserInactivityLogout()have identical implementations. If both actions should behave the same way, consider using a single private helper or having one action delegate to the other for clarity.♻️ Suggested refactor
- private fun handleLockApp() { - viewModelScope.launch { - userPreferencesRepository.setIsUnlocked(false) - } - } - - private fun handleUserInactivityLogout() { - viewModelScope.launch { - userPreferencesRepository.setIsUnlocked(false) - } + private fun lockApplication() { + viewModelScope.launch { + userPreferencesRepository.setIsUnlocked(false) + } }Then in
handleAction:- is AppAction.SessionExpired -> handleUserInactivityLogout() - - is AppAction.LockApp -> handleLockApp() + is AppAction.SessionExpired -> lockApplication() + + is AppAction.LockApp -> lockApplication()Alternatively, if you anticipate these actions diverging in the future (e.g.,
SessionExpiredmight also clear tokens or log analytics), keeping them separate is reasonable for extensibility.feature/passcode/src/commonMain/kotlin/org/mifos/mobile/feature/passcode/PasscodeViewModel.kt (1)
125-142: Consider using cachedstate.storedPasscodeinstead of re-fetching.Line 126 re-fetches the passcode from the repository, but the stored passcode was already loaded into
state.storedPasscodeduring initialization (line 45). Using the cached state value would be more consistent and avoid redundant data access.♻️ Suggested simplification
PasscodeMode.Verify -> viewModelScope.launch { - val storedPasscode = userPreferencesRepository.passcode.firstOrNull() val entered = passcodeBuilder.toString() - if (entered == storedPasscode) { + if (entered == state.storedPasscode) { userPreferencesRepository.setIsUnlocked(true)
cmp-navigation/src/commonMain/kotlin/cmp/navigation/rootnav/RootNavScreen.kt
Show resolved
Hide resolved
|
@mena-rizkalla Could you please mark it ready for review? |
|
@mena-rizkalla Have you addressed the above suggestions (relevant to your PR) by @coderabbitai yet? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI need to check what suggestions were made in the automated review to answer your question. Let me fetch those review comments first. 🏁 Script executed: Length of output: 32679 🏁 Script executed: Length of output: 39452
✅ Addressed (2 issues):
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll help check the status of the review suggestions. Let me gather the review comments from the full review. 🏁 Script executed: Length of output: 79065 Let me check the current state of the code to see which review suggestions have been addressed. 🏁 Script executed: Length of output: 8525
✅ Addressed Issues (2)
|
Ya am working on them |
…rage - Deleted `DatastoreSessionStorage` and `SessionStorage` interface, moving session management to in-memory state. - Removed `last_session_time` tracking from `UserPreferencesDataSource` and `UserPreferencesRepository`. - Updated `session_expired_message` in English and Spanish to reflect that users should verify their passcode instead of being logged out. - Adjusted `RootNavScreen` to always navigate to the authenticated graph upon passcode verification. - Removed explicit `sessionManager.stopSession()` call from `ComposeApp` lifecycle disposal. - Cleaned up `PreferenceModule` by removing unused `SessionStorage` binding.
mena-rizkalla
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@core/common/src/commonMain/kotlin/org/mifos/mobile/core/common/SessionManager.kt`:
- Around line 76-92: checkExpirationInternal currently sets _isExpired.value =
true but leaves isMonitoring true, which keeps the heartbeat running and
prevents startSession() from restarting; update checkExpirationInternal() so
when a session is marked expired (set _isExpired.value = true) you also clear
the monitoring flag (call isMonitoring.store(false)) so the monitor stops and
startSession() can successfully compareAndSet(false, true) later.
- Update `SessionManager` to stop monitoring once the session has expired. - Remove unused `THROTTLE_DISK_SAVE_MS` constant from `Constants.kt`. - Fix minor formatting in `Constants.kt`.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@core/common/src/commonMain/kotlin/org/mifos/mobile/core/common/SessionManager.kt`:
- Around line 76-92: checkExpirationInternal can race with stopSession because
stopSession clears _isExpired and lastInteractionTime without holding checkLock;
inside checkExpirationInternal, after entering checkLock add an immediate guard
checking isMonitoring (e.g., if (!isMonitoring.load()) return) before reading
lastInteractionTime so the method exits promptly when monitoring was stopped;
reference checkExpirationInternal, stopSession, checkLock, isMonitoring,
_isExpired and lastInteractionTime to locate and update the logic.
core/common/src/commonMain/kotlin/org/mifos/mobile/core/common/SessionManager.kt
Show resolved
Hide resolved
…omposeAppViewModel` - Remove the `MifosBasicDialog` for session expiration and instead trigger the `SessionExpired` action via `LaunchedEffect`. - Consolidate session expiration and app locking logic in `ComposeAppViewModel` by replacing `handleUserInactivityLogout` with `handleLockApp`.
- Removed the blur effect from `SessionHandler` when `isExpired` is true, keeping only the pointer input interception.
- Clean up unused `Modifier.blur` and `KptTheme` imports in `SessionHandler.kt`.
|
@mena-rizkalla @biplab1 We won't require this functionality at this time, as our application is still in its initial phase. We have numerous bugs and issues to address, and implementing this feature would create additional complications for developers and contributors. |
|
@niyajali I understand your concern and I had a similar thought that it might be a problem for contributors and developers. However, this ticket was discussed during standup as well on slack threads extensively. This is almost ready and only a final review was required which I was about to complete today. @mena-rizkalla has spent a lot of time and effort implementing this. Can we not implement a flag or something which can be toggled for production build only? Let me know your thoughts on this. cc: @therajanmaurya |
|
Ya I’ve put effort into this, but the most important thing for me is what’s best for the team and the project's current phase. I'm happy to either comment out the trigger of passcode screen or keep this PR open/as a draft until we decide when is the best time to merge it or if there are other options. I am completely fine with any option |
|
Convert this PR to draft and we'll merge it later.. |
3 participants
Fixes - MM-502
📝 Description
This PR introduces a security feature that implements a "Soft Lock" mechanism, automatically locking the session after 5 minutes of inactivity, improving both security and user experience.
SessionManagerthat tracks the last user interaction time and handle session expiry after the timeout (defined in Constants).SessionHandlerUI utility that wraps the application content. It uses aPointerInputspy to detect touch events globally without interfering with existing UI interactions.SessionHandlerinto the rootComposeAppto listen for the session expiry and trigger theApp Lockaction inComposeAppViewModel.stopSession()) in Auth-related ViewModels (Login, Sign Up, etc.) to ensure the timer does not run when the user is not authenticated.Please Add Screenshots If there are any UI changes.
document_17.mp4
document_18.mp4
Please make sure these boxes are checked before submitting your pull request - thanks!
Run the static analysis check
./gradlew checkorci-prepush.shto make sure you didn't break anythingIf you have multiple commits please combine them into one commit by squashing them.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.