[PM-28468] Updated validation and navigation for MigrateToMyItems #6279

aj-rosado · 2025-12-17T20:14:13Z

🎟️ Tracking

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

📔 Objective

Added an observable flow that will be updated after a sync and if the conditions are met will trigger the RootNavViewModel to display the MigrateToVaultItems flow.
Only after a sync in order to ensure we are not displaying the migration screen with stale data.

The conditions for the navigation are

VAULT_OWNERSHIP policy is active
User has vault items (no organizationId)
FeatureFlag is active
User hasInternetConnection

This is only in draft, missing all the unit tests and some manual tests

⏰ Reminders before review

Contributor guidelines followed
All formatters and local linters executed and passed
Written new unit and / or integration tests where applicable
Protected functional changes with optionality (feature flags)
Used internationalization (i18n) for all UI strings
CI builds passed
Communicated to DevOps any deployment requirements
Updated any necessary documentation (Confluence, contributing docs) 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

github-actions · 2025-12-17T20:20:10Z

Checkmarx One – Scan Summary & Details – 026087e0-a491-4e5f-8123-e2153562c6cb

Great job! No new security vulnerabilities introduced in this pull request

codecov · 2025-12-18T11:56:33Z

Codecov Report

❌ Patch coverage is 80.82192% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.49%. Comparing base (461e1e1) to head (5f55b6a).

Files with missing lines	Patch %	Lines
...arden/ui/platform/feature/rootnav/RootNavScreen.kt	0.00%	9 Missing and 1 partial ⚠️
...en/ui/platform/feature/rootnav/RootNavViewModel.kt	88.00%	2 Missing and 1 partial ⚠️
...twarden/data/vault/manager/VaultSyncManagerImpl.kt	87.50%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6279      +/-   ##
==========================================
- Coverage   85.50%   85.49%   -0.02%     
==========================================
  Files         763      764       +1     
  Lines       54623    54685      +62     
  Branches     7852     7866      +14     
==========================================
+ Hits        46708    46755      +47     
- Misses       5187     5199      +12     
- Partials     2728     2731       +3

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

claude · 2025-12-18T12:27:41Z

Claude finished @aj-rosado's task in 3m 18s —— View job

Overall Assessment: APPROVE

This PR adds vault migration validation and navigation logic with proper architecture adherence. The implementation follows established patterns with VaultMigrationManager handling state management, proper DI integration, and comprehensive test coverage. Migration checks occur after sync and after vault unlock to ensure up-to-date data, with navigation handled correctly through RootNavViewModel.

Code Review Details

No critical, important, or debt findings. The implementation is clean and follows Bitwarden architecture patterns.

Key Implementation Highlights:

VaultMigrationManager properly implements Manager pattern with single responsibility
Migration state verification occurs at two appropriate points: after sync (line 346) and after vault unlock (line 413) in VaultSyncManagerImpl
RootNavViewModel correctly prioritizes migration navigation after autofill special circumstances
Navigation loop protection exists via RootNavScreen's route equality check (lines 172-176)
Organization data is persisted before migration checks, avoiding staleness issues
Comprehensive test coverage including edge cases (null org ID, missing org name, no network, etc.)

app/src/main/kotlin/com/x8bit/bitwarden/data/vault/manager/VaultSyncManagerImpl.kt

SaintPatrck · 2025-12-29T14:39:34Z

...in/kotlin/com/x8bit/bitwarden/ui/vault/feature/migratetomyitems/MigrateToMyItemsViewModel.kt

+        val orgId = policyManager.getPersonalOwnershipPolicyOrganizationId()
+        val orgName = authRepository.userStateFlow.value
+            ?.activeAccount
+            ?.organizations
+            ?.firstOrNull { it.id == orgId }
+            ?.name


❓ Passing args makes for a simpler ViewModel; specifically in regards to handling scenarios when either orgId or orgName are null. Is there a reason why this approach is taken instead?

I have changed this quite a bit from original idea. RootNavViewModel only verifies if it should navigate from the shouldMigratePersonalVaultFlow, not having any info regarding the org that should do the migration.

This was changed because if the vault was updated on a different client, the migration flow would still be displayed as it would not check with the most synced data. Now we are syncing before displaying the screen to make sure we are not displaying the screen unless it is needed.

This being said, I agree that if we could pass the parameters would simplify the VM, not sure about a good way to achieve it, only that I can think about is the flow having the Organization object?

SaintPatrck · 2025-12-29T15:00:41Z

app/src/main/kotlin/com/x8bit/bitwarden/data/vault/manager/VaultSyncManagerImpl.kt

+        val shouldMigrate = policyManager
+            .getActivePolicies(PolicyTypeJson.PERSONAL_OWNERSHIP)
+            .any() &&
+            featureFlagManager.getFeatureFlag(FlagKey.MigrateMyVaultToMyItems) &&
+            connectionManager.isNetworkConnected &&
+            cipherList.any { it.organizationId == null }


⛏️ 🎨 Not a mandatory change request, but some of this duplication could be eliminated if userShouldMigrate() is changed to accept hasPersonalItems as a lambda.

private fun userShouldMigrateVault( hasPersonalItems: () -> Boolean, ) : Boolean { return connectionManager.isNetworkConnected && featureFlagManager.getFeatureFlag(...) && policyManager.getActivePolicies(...).any() && hasPersonalItems } private fun verifyAndUpdateIfUserShouldMigrateVaultToMyItems( cipherList: List<Cipher>, ) { mutableShouldMIgratePersonalVaultFlow.update { userShouldMigrateVault { cipherList.any { it.organizationId == null } } } }

This would also be a performance improvement since it would short-circuit before attempting to iterate over the entire cipher collection when any of the preceding conditions are false.

…arameter to MigrateToMyItems

…ation # Conflicts: # app/src/test/kotlin/com/x8bit/bitwarden/ui/vault/feature/migratetomyitems/MigrateToMyItemsViewModelTest.kt

…ation

SaintPatrck · 2026-01-05T16:29:09Z

app/src/main/kotlin/com/x8bit/bitwarden/data/vault/manager/VaultSyncManager.kt

+     * Flow that emits when conditions are met for the user to migrate their personal vault.
+     * Updated after each sync to reflect current policy and vault state.
+     */
+    val shouldMigratePersonalVaultFlow: StateFlow<VaultMigrationData>


⛏️ since this is no longer a simple Boolean value, can we rename it to something like vaultMigrationDataStateFlow? That would align with the naming of other flows defined above.

SaintPatrck · 2026-01-05T16:37:13Z

app/src/main/kotlin/com/x8bit/bitwarden/data/vault/manager/VaultSyncManagerImpl.kt

+                            // We need to be sure the data on device is updated
+                            // before sending the user to the migration screen
+                            if (userShouldMigrateVault {
+                                    result.successes.any { it.organizationId == null }


it is being shadowed by the outer map { } arg.

Suggested change

result.successes.any { it.organizationId == null }

result.successes.any { cipher -> cipher.organizationId == null }

david-livefront · 2026-01-05T16:39:08Z

app/src/main/kotlin/com/x8bit/bitwarden/data/vault/manager/VaultSyncManagerImpl.kt

+                                    result.successes.any { it.organizationId == null }
+                                }
+                            ) {
+                                sync(forced = true)


IDK how I feel about having side-affects in this flow 🤔

I do not feel totally comfortable with it as well. Any idea of a better approach?
Otherwise we will display the takeover to the user until a sync is forced even if the user does not need to do the migration anymore

david-livefront · 2026-01-05T16:40:26Z

app/src/main/kotlin/com/x8bit/bitwarden/data/vault/manager/VaultSyncManagerImpl.kt

+    }
+
+    private fun verifyAndUpdateIfUserShouldMigrateVaultToMyItems(cipherList: List<Cipher>) {
+        val userId = activeUserId ?: return


Can we just pass in the userId to avoid any conflicts

david-livefront · 2026-01-05T16:45:45Z

app/src/main/kotlin/com/x8bit/bitwarden/data/vault/manager/VaultSyncManagerImpl.kt

+                if (orgId != null && orgName != null) {
+                    VaultMigrationData.MigrationRequired(
+                        organizationId = orgId,
+                        organizationName = orgName,


The VaultSyncManager handling the migration feels a little odd to me. It make it responsible for more scope than it maybe should be responsible for.

What would you think of a VaultMigrationManager that consumed the cipher data to handle the migration?

SaintPatrck · 2026-01-05T16:57:27Z

app/src/main/kotlin/com/x8bit/bitwarden/data/vault/manager/VaultSyncManagerImpl.kt

+                                    result.successes.any { it.organizationId == null }
+                                }
+                            ) {
+                                sync(forced = true)


❓ Is there a specific scenario or issue you're encountering that requires this additional sync() call before navigating? Asking because this feels out of place. I would expect the disk source data to already be up-to-date here. If not, the next sync (manual or automatic) should trigger migration if it's needed.

If the concern is navigating to migration screens with stale data, could we perform sync or syncForResult in the MigateToMyItemsViewModel immediately before triggering the migration?

Yes the issues is displaying the Migrate screen when is not necessary.
So we navigate to the screen, sync and check again if the user still needs the migration and dismiss it if he does not? Looks good to me actually, this should be an edge case

…ce sync call when user should migrate on SyncManager and added a new Sync call on the MigrateToMyItemsVM

…ation

david-livefront · 2026-01-07T14:33:19Z

app/src/main/kotlin/com/x8bit/bitwarden/data/vault/manager/VaultSyncManagerImpl.kt

+                vaultMigrationManager.verifyAndUpdateMigrationState(
+                    userId = userId,
+                    cipherList = it,
+                )


This still feels like an inappropriate place to have this side-affect.

david-livefront · 2026-01-07T14:36:03Z

app/src/main/kotlin/com/x8bit/bitwarden/data/vault/manager/VaultMigrationManagerImpl.kt

+        }
+    }
+
+    override fun shouldMigrateVault(hasPersonalItems: () -> Boolean): Boolean {


Can this just be a private helper function. It is never used outside this class.

Updated validation and navigation for MigrateToMyItems

65a63aa

Updated RootNavViewModel and added tests

2734ac4

aj-rosado marked this pull request as ready for review December 18, 2025 12:26

aj-rosado requested review from a team, brian-livefront and david-livefront as code owners December 18, 2025 12:26

removed unnecessary hasPersonalVaultItems method

02c6dbd

SaintPatrck reviewed Dec 19, 2025

View reviewed changes

app/src/main/kotlin/com/x8bit/bitwarden/data/vault/manager/VaultSyncManagerImpl.kt Outdated Show resolved Hide resolved

Removed unnecessary comment

ff3693a

aj-rosado requested a review from SaintPatrck December 19, 2025 18:17

SaintPatrck reviewed Dec 29, 2025

View reviewed changes

Deduplicated method on VaultSyncManagerImple

cc9e336

aj-rosado requested a review from SaintPatrck December 30, 2025 14:33

aj-rosado added 2 commits December 30, 2025 18:15

Saving org info on shouldMigratePersonalVaultFlow and passing it as p…

66ced79

…arameter to MigrateToMyItems

Merge branch 'main' into PM-28468/validate-and-navigate-to-vault-migr…

9e911aa

…ation # Conflicts: # app/src/test/kotlin/com/x8bit/bitwarden/ui/vault/feature/migratetomyitems/MigrateToMyItemsViewModelTest.kt

github-actions bot added app:password-manager t:misc labels Dec 30, 2025

aj-rosado added 4 commits December 30, 2025 19:26

fixed tests

d49c1c1

fixed VaultSyncManagerTests

d60d149

added test coverage for more scenarios on VaultSyncManagerTest

a35d98a

Merge branch 'main' into PM-28468/validate-and-navigate-to-vault-migr…

3247cd1

…ation

SaintPatrck reviewed Jan 5, 2026

View reviewed changes

david-livefront reviewed Jan 5, 2026

View reviewed changes

SaintPatrck reviewed Jan 5, 2026

View reviewed changes

aj-rosado added 3 commits January 6, 2026 12:03

refactored personal vault migration logic into VaultMigrationManager

23737ef

Checking if migration should be called after unlock, removing the for…

cf711b6

…ce sync call when user should migrate on SyncManager and added a new Sync call on the MigrateToMyItemsVM

passing userId for the verifyAndUpdateMigrationState

a158233

aj-rosado requested review from SaintPatrck and david-livefront January 6, 2026 17:24

SaintPatrck added the ai-review Request a Claude code review label Jan 7, 2026

Merge branch 'main' into PM-28468/validate-and-navigate-to-vault-migr…

5f55b6a

…ation

github-actions bot removed the ai-review Request a Claude code review label Jan 7, 2026

david-livefront reviewed Jan 7, 2026

View reviewed changes

set shouldMigrateVault as private on VaultMigrationManager

ad94ce9

	result.successes.any { it.organizationId == null }
	result.successes.any { cipher -> cipher.organizationId == null }

[PM-28468] Updated validation and navigation for MigrateToMyItems #6279

Are you sure you want to change the base?

[PM-28468] Updated validation and navigation for MigrateToMyItems #6279

Conversation

aj-rosado commented Dec 17, 2025

🎟️ Tracking

📔 Objective

⏰ Reminders before review

🦮 Reviewer guidelines

Uh oh!

github-actions bot commented Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Great job! No new security vulnerabilities introduced in this pull request

Uh oh!

codecov bot commented Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

claude bot commented Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SaintPatrck Dec 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aj-rosado Jan 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions bot commented Dec 17, 2025 •

edited

Loading

codecov bot commented Dec 18, 2025 •

edited

Loading

claude bot commented Dec 18, 2025 •

edited

Loading

SaintPatrck Dec 29, 2025 •

edited

Loading

aj-rosado Jan 5, 2026 •

edited

Loading