[PM-13626] Remember last opened view for 5 minutes #4574

aj-rosado · 2025-01-16T11:57:19Z

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-13626

📔 Objective

After lock, for the next 5 minutes, the user should be redirected to the previous screen if they were:

Send
Verification Code
Generator
Search

To achieve this, a new manager was added to control the behaviour and saving the last screen on SettingsDiskSource (should this be on a different disk source?). The navigation is provided by deeplink using the existing SpecialCircumstance.

The current screen is always saved onResume and cleared onPause (to ensure we don't navigate to previous screens)
Before clearing, checking if the user is not locked, does the trick to not clear in case of lock when the app was on background

A different task will add more screens

⏰ Reminders before review

Contributor guidelines followed
All formatters and local linters executed and passed
Written new unit and / or integration tests where applicable
Used internationalization (i18n) for all UI strings
CI builds passed
Communicated to DevOps any deployment requirements
Updated any necessary documentation or informed the documentation team

🦮 Reviewer guidelines

👍 (:+1:) or similar for great changes
📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
❓ (:question:) for questions
🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed
issue and could potentially benefit from discussion
🎨 (:art:) for suggestions / improvements
❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
⛏ (:pick:) for minor or nitpick changes

… AppResume

…pResumeData object Signed-off-by: Andre Rosado <[email protected]>

# Conflicts: # app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/verificationcode/VerificationCodeScreen.kt

github-actions · 2025-01-16T12:02:19Z

Checkmarx One – Scan Summary & Details – 0661fedf-9ff8-4f93-a3e2-56efffd64e68

Great job, no security vulnerabilities found in this Pull Request

codecov · 2025-01-16T15:29:50Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.32%. Comparing base (31ccf49) to head (3548304).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4574      +/-   ##
==========================================
- Coverage   88.35%   88.32%   -0.04%     
==========================================
  Files         603      605       +2     
  Lines       40768    40887     +119     
  Branches     5738     5772      +34     
==========================================
+ Hits        36022    36112      +90     
- Misses       2742     2757      +15     
- Partials     2004     2018      +14

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

app/src/main/java/com/x8bit/bitwarden/data/auth/datasource/disk/AuthDiskSourceImpl.kt

app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/disk/SettingsDiskSourceImpl.kt

app/src/main/java/com/x8bit/bitwarden/data/platform/manager/AppResumeManagerImpl.kt

dseverns-livefront · 2025-01-20T20:11:51Z

app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/search/SearchViewModel.kt

 ) : BaseViewModel<SearchState, SearchEvent, SearchAction>(
    // We load the state from the savedStateHandle for testing purposes.
    initialState = savedStateHandle[KEY_STATE]
        ?: run {
            val searchType = SearchArgs(savedStateHandle).type
            val userState = requireNotNull(authRepo.userStateFlow.value)
            val specialCircumstance = specialCircumstanceManager.specialCircumstance
+            val searchTerm = if (specialCircumstance is SpecialCircumstance.SearchShortcut) {


this could also be:

(specialCircumstance as? SpecialCircumstance.SearchShortcut) ?.searchTerm ?.also { specialCircumstanceManager.specialCircumstance = null } .orEmpty()

dseverns-livefront · 2025-01-20T20:11:57Z

app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/search/SearchViewModel.kt

 ) : BaseViewModel<SearchState, SearchEvent, SearchAction>(
    // We load the state from the savedStateHandle for testing purposes.
    initialState = savedStateHandle[KEY_STATE]
        ?: run {
            val searchType = SearchArgs(savedStateHandle).type
            val userState = requireNotNull(authRepo.userStateFlow.value)
            val specialCircumstance = specialCircumstanceManager.specialCircumstance
+            val searchTerm = if (specialCircumstance is SpecialCircumstance.SearchShortcut) {


this could also be:

(specialCircumstance as? SpecialCircumstance.SearchShortcut) ?.searchTerm ?.also { specialCircumstanceManager.specialCircumstance = null } .orEmpty()

david-livefront · 2025-01-20T20:57:10Z

app/src/main/java/com/x8bit/bitwarden/data/auth/datasource/disk/AuthDiskSource.kt

+    /**
+     * Gets the last lock timestamp for the given [userId].
+     */
+    fun getLastLockTimestamp(userId: String): Long?


These should return the same types, either both longs or both Instants. I kinda prefer Instant but I'll leave it up to you

app/src/main/java/com/x8bit/bitwarden/data/auth/datasource/disk/AuthDiskSourceImpl.kt

david-livefront · 2025-01-20T20:59:10Z

app/src/main/java/com/x8bit/bitwarden/data/platform/manager/AppResumeManagerImpl.kt

+) : AppResumeManager {
+    private companion object {
+        // 5 minutes
+        private const val UNLOCK_NAVIGATION_TIME: Long = 5 * 60


Can we add _SECONDS to the end of this to indicate units.

Also, this can be a top level property of the file.

david-livefront · 2025-01-20T21:01:19Z

app/src/main/java/com/x8bit/bitwarden/data/platform/manager/AppResumeManagerImpl.kt

+            authDiskSource.getLastLockTimestamp(
+                userId = userId,
+            ) ?: Long.MIN_VALUE,
+        )


We don't need to handle a null timestamp at all:

val lastLockTimestamp = authDiskSource .getLastLockTimestamp(userId = userId) ?.let { Instant.ofEpochMilli(it) } ?: return null

app/src/main/java/com/x8bit/bitwarden/data/platform/manager/AppResumeManagerImpl.kt

david-livefront · 2025-01-20T21:04:14Z

app/src/main/java/com/x8bit/bitwarden/data/vault/manager/VaultLockManagerImpl.kt

@@ -268,6 +270,10 @@ class VaultLockManagerImpl(
        )
        if (!wasVaultLocked) {
            mutableVaultStateEventSharedFlow.tryEmit(VaultStateEvent.Locked(userId = userId))
+            authDiskSource.storeLastLockTimestamp(
+                userId = userId,
+                lastLockTimestamp = clock.instant(),


david-livefront · 2025-01-20T21:05:52Z

app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/search/SearchViewModel.kt

@@ -142,6 +153,8 @@ class SearchViewModel @Inject constructor(
            is SearchAction.SearchTermChange -> handleSearchTermChange(action)
            is SearchAction.VaultFilterSelect -> handleVaultFilterSelect(action)
            is SearchAction.OverflowOptionClick -> handleOverflowItemClick(action)
+            is SearchAction.LifecycleResume -> handleOnResumed()
+            is SearchAction.LifecycleStop -> handleOnPaused()


The function is still called handleOnPaused even though you are stopping. Can we rename this

app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/search/SearchViewModel.kt

david-livefront · 2025-01-20T21:07:31Z

app/src/main/java/com/x8bit/bitwarden/ui/tools/feature/generator/GeneratorScreen.kt

-
+            Lifecycle.Event.ON_STOP -> {
+                viewModel.trySendAction(GeneratorAction.LifecycleStop)
+            }


Wanna run the formatter here, there should be some newlines.

I would recommend adding a macro for this. There are instructions here.

Formatter is not making any changes on this file

Hmm, are you sure you have the correct formatter configured. Because this when I run the formatter it does this:

LivecycleEventEffect { _, event -> when (event) { Lifecycle.Event.ON_RESUME -> { viewModel.trySendAction(GeneratorAction.LifecycleResume) } Lifecycle.Event.ON_STOP -> { viewModel.trySendAction(GeneratorAction.LifecycleStop) } else -> Unit } }

david-livefront · 2025-01-20T21:08:05Z

app/src/main/java/com/x8bit/bitwarden/ui/tools/feature/generator/GeneratorViewModel.kt

@@ -2139,6 +2150,11 @@ sealed class GeneratorAction {
     */
    data object LifecycleResume : GeneratorAction()

+    /**
+     * Indicates the UI has been entered a resumed lifecycle state.


This refers to resume instead of stop

david-livefront · 2025-01-20T21:10:19Z

app/src/test/java/com/x8bit/bitwarden/data/auth/datasource/disk/util/FakeAuthDiskSource.kt

@@ -314,13 +317,21 @@ class FakeAuthDiskSource : AuthDiskSource {
        return storedNewDeviceNoticeState[userId] ?: NewDeviceNoticeState(
            displayStatus = NewDeviceNoticeDisplayStatus.HAS_NOT_SEEN,
            lastSeenDate = null,
-            )
+        )


david-livefront · 2025-01-20T21:20:52Z

app/src/test/java/com/x8bit/bitwarden/ui/platform/feature/rootnav/RootNavViewModelTest.kt

@@ -53,6 +53,31 @@ class RootNavViewModelTest : BaseViewModelTest() {
            dispatcherManager = FakeDispatcherManager(),
        )

+    private val mockVaultUnlockedUserState = UserState(


Can we just make this a constant at the bottom of the file.

dseverns-livefront · 2025-01-21T15:16:17Z

app/src/main/java/com/x8bit/bitwarden/data/platform/manager/AppResumeManagerImpl.kt

-import java.time.Instant
+
+// 5 minutes
+private const val UNLOCK_NAVIGATION_TIME_MS: Long = 5 * 60


⛏️ I appreciate the comment here, but the _MS immediately makes me think milliseconds, UNLOCK_NAVIGATION_TIME_MINUTES can never go wrong being UBER explicit.

I think that is my bad, I told him to add _MS because I thought it was milliseconds.

It should be UNLOCK_NAVIGATION_TIME_SECONDS since the value is in seconds

…reusable.

…opened-view

…Code

…and added test to validate it

# Conflicts: # app/src/main/java/com/x8bit/bitwarden/data/vault/manager/VaultLockManagerImpl.kt

aj-rosado added 11 commits January 8, 2025 11:31

Add AppResumeManager, SettingsDiskSource methods and unlock check for…

8007888

… AppResume

Add generator screen to AppResume

876df9a

Added SendShortcut and Send to AppResumeManager

0d1565a

Add App Resume logic to the Search

885e4b5

Verification Code app resume navigation

7937c22

Merge branch 'main' into PM-13626/remember-last-opened-view

cf91608

saving last lock timestamp and improved AppResume logic

fa0cdd9

improved app resume clarity

c934288

Refactor SettingsDiskSource appResume methods to receive/return an Ap…

b374725

…pResumeData object Signed-off-by: Andre Rosado <[email protected]>

Add tests for AppResume functionality

286ecd5

Merge branch 'main' into PM-13626/remember-last-opened-view

54826d6

# Conflicts: # app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/verificationcode/VerificationCodeScreen.kt

aj-rosado added 2 commits January 16, 2025 13:28

Fixed tests

f98e93c

Fixed SearchScreenTest

bbbd31f

aj-rosado marked this pull request as ready for review January 16, 2025 15:10

aj-rosado requested review from brian-livefront, david-livefront, dseverns-livefront, ahaisting-livefront, phil-livefront and a team as code owners January 16, 2025 15:10

Added tests for RootNavVM VaultUnlockNavBarVM and VaultVM

2bd72a4