[PM-28468] Updated validation and navigation for MigrateToMyItems #6279
Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6279 +/- ##
==========================================
- Coverage 85.49% 85.49% -0.01%
==========================================
Files 763 764 +1
Lines 54640 54721 +81
Branches 7865 7881 +16
==========================================
+ Hits 46716 46782 +66
- Misses 5189 5201 +12
- Partials 2735 2738 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
aj-rosado
requested review from
a team,
brian-livefront and
david-livefront
as code owners
|
Claude finished @aj-rosado's task in 4m 30s —— View job Overall Assessment: REQUEST CHANGES Reviewed vault migration manager implementation with reactive flow observation, database optimizations, and navigation integration. The core reactive logic and database query optimizations are well-implemented and thoroughly tested. Found one critical issue with incomplete navigation implementation. Code Review Details
Review Notes:
|
app/src/main/kotlin/com/x8bit/bitwarden/data/vault/manager/VaultSyncManagerImpl.kt
Outdated
Show resolved
Hide resolved
| val orgId = policyManager.getPersonalOwnershipPolicyOrganizationId() | ||
| val orgName = authRepository.userStateFlow.value | ||
| ?.activeAccount | ||
| ?.organizations | ||
| ?.firstOrNull { it.id == orgId } | ||
| ?.name |
There was a problem hiding this comment.
❓ 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?
There was a problem hiding this comment.
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?
| val shouldMigrate = policyManager | ||
| .getActivePolicies(PolicyTypeJson.PERSONAL_OWNERSHIP) | ||
| .any() && | ||
| featureFlagManager.getFeatureFlag(FlagKey.MigrateMyVaultToMyItems) && | ||
| connectionManager.isNetworkConnected && | ||
| cipherList.any { it.organizationId == null } |
There was a problem hiding this comment.
⛏️ 🎨 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 }
}
}
}There was a problem hiding this comment.
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
app/src/main/kotlin/com/x8bit/bitwarden/data/vault/manager/VaultSyncManager.kt
Outdated
Show resolved
Hide resolved
app/src/main/kotlin/com/x8bit/bitwarden/data/vault/manager/VaultSyncManagerImpl.kt
Outdated
Show resolved
Hide resolved
| result.successes.any { it.organizationId == null } | ||
| } | ||
| ) { | ||
| sync(forced = true) |
There was a problem hiding this comment.
IDK how I feel about having side-affects in this flow 🤔
There was a problem hiding this comment.
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
app/src/main/kotlin/com/x8bit/bitwarden/data/vault/manager/VaultSyncManagerImpl.kt
Outdated
Show resolved
Hide resolved
| if (orgId != null && orgName != null) { | ||
| VaultMigrationData.MigrationRequired( | ||
| organizationId = orgId, | ||
| organizationName = orgName, |
There was a problem hiding this comment.
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?
The test 'vault unlock should decrypt cipher list from disk' was identical to the existing test 'ciphersStateFlow should emit decrypted list of ciphers when decryptCipherList succeeds'. This duplication arose from refactoring that removed migration verification logic, leaving only the cipher decryption behavior which was already tested.
**Objective:** Fix an issue where vault migration could be triggered incorrectly when the user is in a "special circumstance" (e.g., during specific onboarding flows or error states). **Behavioral Changes:** The application will now check if `specialCircumstance` is null before initiating the `MigrateToMyItems` navigation state. This ensures migration only occurs during standard vault operations. **Specific Changes:** - `RootNavViewModel.kt`: Added a check for `specialCircumstance == null` within the condition that triggers `RootNavState.MigrateToMyItems`.
Optimize VaultMigrationManagerImpl memory usage by replacing expensive getCiphersFlow() calls (loads all cipher JSON) with lightweight indexed hasPersonalCiphersFlow() boolean query. Database Schema Changes: - Add organizationId column to CipherEntity with composite index - Bump VaultDatabase version from 8 to 9 - Destructive migration triggers automatic vault sync - Column populated during sync operations Query Optimization: - Add hasPersonalCiphersFlow() to CiphersDao using EXISTS query - Query uses indexed organization_id column (no JSON parsing) - LIMIT 1 for short-circuit optimization Integration: - VaultMigrationManagerImpl uses optimized boolean flow - Replace List<Cipher> parameter with Boolean in verification methods - Update method signatures: verifyAndUpdateMigrationState, shouldMigrateVault Testing: - Update VaultMigrationManagerTest to use Boolean flow mocks - Add VaultDiskSourceTest cases for hasPersonalCiphersFlow - Update FakeCiphersDao test implementation Performance Impact: - Memory: Reduces from ~10-100MB cipher data to ~1KB boolean query - Query: O(n) JSON deserialization -> O(log n) indexed lookup
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
app/src/main/kotlin/com/x8bit/bitwarden/data/vault/manager/VaultMigrationManagerImpl.kt
Show resolved
Hide resolved
app/src/main/kotlin/com/x8bit/bitwarden/data/vault/manager/VaultMigrationManagerImpl.kt
Outdated
Show resolved
Hide resolved
app/src/main/kotlin/com/x8bit/bitwarden/data/vault/manager/VaultMigrationManagerImpl.kt
Outdated
Show resolved
Hide resolved
app/src/main/kotlin/com/x8bit/bitwarden/data/vault/datasource/disk/database/VaultDatabase.kt
Show resolved
Hide resolved
| val id: String, | ||
|
|
||
| @ColumnInfo(name = "user_id", index = true) | ||
| @ColumnInfo(name = "user_id") |
| tableName = "ciphers", | ||
| indices = [ | ||
| Index(value = ["user_id"]), | ||
| Index(value = ["user_id", "organization_id"]), |
There was a problem hiding this comment.
Is the basic user_id needed since you already added it to the ColumnInfo?
Do we need the compound Index? We do not query by a specific org id, so I'm unsure if that will gain us anything.
There was a problem hiding this comment.
I removed it from the ColumnInfo, so it is needed here.
We don't gain much from this particular query, but we'll be fetching all personal ciphers from the DB when we implement the actual migration API's. That's where the new index will really shine.
There was a problem hiding this comment.
Technically we don't need the individual user_id index and the composite index. At least I don't think we do. 🤔
@claude is the individual user_id column index needed if it's include in the composite index of user_id and organization_id?
There was a problem hiding this comment.
Claude finished @SaintPatrck's task in 30s —— View job
Index Redundancy Analysis
- Analyze the database index question
- Research SQLite/Room index behavior with composite indexes
- Provide answer with recommendation
Answer: You're correct - the individual user_id index is not needed when you have a composite index with user_id as the leftmost column.
How SQLite composite indexes work:
A composite index on (user_id, organization_id) can satisfy queries that filter by:
user_idalone ✅user_idANDorganization_id✅organization_idalone ❌ (would require a separate index)
This is because SQLite (and most relational databases) can use the "leftmost prefix" of a composite index. The B-tree structure allows efficient lookups starting from the leftmost column.
Current state in CipherEntity.kt:13-16:
indices = [
Index(value = ["user_id"]), // Redundant
Index(value = ["user_id", "organization_id"]),
],Recommended change:
indices = [
Index(value = ["user_id", "organization_id"]),
],Why remove the redundant index:
- Saves storage space (less disk usage)
- Faster write operations (fewer indexes to update on INSERT/UPDATE/DELETE)
- No impact on read performance for
user_id-only queries
The only scenario where you'd want both is if the single-column index provides significantly better performance for user_id-only queries (e.g., the composite index is very large). In practice, this is rarely a concern for mobile databases.
…e changes This commit refactors the `VaultMigrationManager` to ensure migration state is re-evaluated not only on vault unlock and data changes, but also in response to changes in network connectivity and sync completion status. Previously, the migration check was only performed at specific points, which could lead to stale state. For example, if a user unlocked their vault while offline, the migration prompt would not appear even after they came back online. Similarly, in multi-account scenarios where `lastSyncTime` might be cleared, the migration check would not re-run after a subsequent sync. This change makes the vault migration logic more robust and reactive by: - Combining the `hasPersonalCiphersFlow` with flows for network connection status (`isNetworkConnectedFlow`) and last sync time (`getLastSyncTimeFlow`). - Ensuring the migration state is automatically re-evaluated and updated whenever network connectivity changes (e.g., going from offline to online). - Triggering a migration state check upon the completion of a sync, which is critical for scenarios where the initial check occurred before the first sync. - Introducing a `FakeNetworkConnectionManager` in tests to reliably simulate network state changes and verify the reactive behavior.
…-navigate-to-vault-migration
...main/kotlin/com/x8bit/bitwarden/ui/platform/feature/vaultunlocked/VaultUnlockedNavigation.kt
Show resolved
Hide resolved
| expectNoEvents() | ||
|
|
||
| // Sync completes - should now trigger migration check | ||
| mutableLastSyncTimeFlow.value = Instant.now() |
There was a problem hiding this comment.
Can we use Instant.parse("2023-10-27T12:00:00Z") instead.
Or event Instant.MIN
| every { vaultUnlockDataStateFlow } returns mutableVaultUnlockDataStateFlow | ||
| } | ||
|
|
||
| private val mutableLastSyncTimeFlow = MutableStateFlow<Instant?>(Instant.now()) |
There was a problem hiding this comment.
Same thing here can we avoid the now()
|
Thank you @SaintPatrck and @david-livefront |
SaintPatrck
deleted the
PM-28468/validate-and-navigate-to-vault-migration
branch
|
Thank you @SaintPatrck and @david-livefront |
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-28468
📔 Objective
Implement navigation to the vault migration flow that prompts users with a Personal Ownership policy to migrate their personal vault items to an organization. The
VaultMigrationManagerreactively monitors vault state, sync completion, and network connectivity to determine when migration is required, displaying the prompt only when all conditions are met (policy active, feature flag enabled, personal ciphers exist, and network available).The implementation includes an optimized database query using an indexed
organizationIdcolumn to efficiently check for personal ciphers without loading full cipher JSON data, and ensures the migration prompt correctly appears or disappears as conditions change (e.g., going offline/online or after sync completes in multi-account scenarios).⏰ Reminders before review
🦮 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