[PM-41057] feat: Gate VFO-1 naming work behind vfo1-foundation feature flag - #7219
[PM-41057] feat: Gate VFO-1 naming work behind vfo1-foundation feature flag#7219andrebispo5 wants to merge 12 commits into
Conversation
Declares FlagKey.Vfo1Foundation (default false), adds its debug-menu entry, converts FeatureFlagsState to a real data class carrying the flag, and adds a vfo1Foundation(new, old) Composable helper for gating renamed strings/icons at call sites.
Injects FeatureFlagManager into MainViewModel, adds isVfo1FoundationEnabled to MainState (initialized synchronously and kept live via getFeatureFlagFlow), and exposes it through MainState.featureFlagsState so it reaches LocalFeatureFlagsState.
Re-adds the folders, collections, no_items_collection, no_collections_to_list, select_one_collection, and move_to_vault strings, plus the ic_collections drawable, all previously deleted as unused once the VFO1 rename shipped unconditionally. They're needed again now that the rename is gated: the flag-off path renders these.
Wraps the Vault, Settings, Folders, item listing, add/edit item, item detail, and move-to-organization screens' VFO1-renamed strings and icons with the vfo1Foundation() helper, so they read the old terminology when the flag is off.
Injects the flag's live value into VaultAddEditState and threads it
into CipherViewExtensions.toAvailableOwners, gating the "shared
folder" validation message and the personal-vault owner name ("My
vault" vs. the account email) between the new and old behavior.
Injects the live flag value into VaultMoveToOrganizationState, gating
its appBarText ("Move"/"Shared folders" vs. "Move to vault"/
"Collections") and its "select at least one" validation message.
Adds VaultTrailingIcon.COLLECTION_LEGACY and a matching VaultItemLocation.Collection icon parameter, and threads isVfo1FoundationEnabled through CipherListViewExtensions.toLabelIcons and the Vault/Search/ItemListing display-item construction functions, so the collection icon and any old/new terminology in these lists follow the flag.
github-actions
Bot
added
app:password-manager
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## readd-vfo1-naming-changes #7219 +/- ##
=============================================================
- Coverage 86.09% 85.97% -0.13%
=============================================================
Files 891 1020 +129
Lines 64869 67420 +2551
Branches 9700 9762 +62
=============================================================
+ Hits 55846 57961 +2115
- Misses 5552 5955 +403
- Partials 3471 3504 +33
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Re-reviewed the Code Review DetailsNo new findings this round. Previously reported items have been addressed:
Still-open items from earlier rounds (not re-posted, existing threads cover them): |
There was a problem hiding this comment.
Pull request overview
Adds a new vfo1-foundation feature flag and threads it through core + UI so the previously implemented VFO-1 terminology/icon changes can be toggled on/off at runtime (default off), providing a kill-switch for merging the work safely to main.
Changes:
- Introduces
FlagKey.Vfo1Foundationand exposes it viaMainState.featureFlagsState→LocalFeatureFlagsState. - Gates renamed terminology and icon variants behind the flag across Vault-related screens/view-model mapping.
- Restores legacy strings/icons needed to render the pre-VFO-1 terminology when the flag is disabled, and updates tests accordingly.
Reviewed changes
Copilot reviewed 45 out of 45 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/main/res/values/strings.xml | Restores legacy terminology strings (e.g., Collections/Folders variants) used when the flag is off. |
| ui/src/main/res/values/strings_non_localized.xml | Adds a debug-menu label for the new flag. |
| ui/src/main/res/drawable/ic_collections.xml | Adds legacy collections icon resource for the flag-off path. |
| ui/src/main/kotlin/com/bitwarden/ui/platform/components/debug/FeatureFlagListItems.kt | Exposes the new flag in the debug feature flags UI. |
| core/src/main/kotlin/com/bitwarden/core/data/manager/model/FlagKey.kt | Adds FlagKey.Vfo1Foundation (key name + default). |
| core/src/test/kotlin/com/bitwarden/core/data/manager/model/FlagKeyTest.kt | Adds/updates unit coverage for the new flag key. |
| app/src/main/kotlin/com/x8bit/bitwarden/ui/platform/model/FeatureFlagsState.kt | Converts UI feature flags from data object to data class with isVfo1FoundationEnabled. |
| app/src/main/kotlin/com/x8bit/bitwarden/ui/platform/composition/util/Vfo1FoundationExtensions.kt | Adds a Compose helper to switch resources/values based on vfo1-foundation. |
| app/src/main/kotlin/com/x8bit/bitwarden/MainViewModel.kt | Reads flag initially + subscribes to updates to publish into MainState.featureFlagsState. |
| app/src/test/kotlin/com/x8bit/bitwarden/MainViewModelTest.kt | Adds test ensuring MainState updates when the flag flow changes. |
| app/src/test/kotlin/com/x8bit/bitwarden/ui/platform/base/BitwardenComposeTest.kt | Updates default FeatureFlagsState construction for tests. |
| app/src/main/kotlin/com/x8bit/bitwarden/ui/vault/model/VaultTrailingIcon.kt | Adds a legacy trailing icon entry for the flag-off collection icon/label. |
| app/src/main/kotlin/com/x8bit/bitwarden/ui/vault/feature/util/CipherListViewExtensions.kt | Threads flag to icon selection for cipher “label icons”. |
| app/src/main/kotlin/com/x8bit/bitwarden/ui/vault/feature/vault/VaultViewModel.kt | Subscribes to the flag flow and threads it into Vault view state generation. |
| app/src/test/kotlin/com/x8bit/bitwarden/ui/vault/feature/vault/VaultViewModelTest.kt | Mocks the new flag flow for VaultViewModel tests. |
| app/src/main/kotlin/com/x8bit/bitwarden/ui/vault/feature/vault/VaultContent.kt | Gates Vault screen headers and collection icon resources via Compose helper. |
| app/src/main/kotlin/com/x8bit/bitwarden/ui/vault/feature/vault/util/VaultDataExtensions.kt | Threads flag through Vault data → view-state mapping (including trailing icons). |
| app/src/main/kotlin/com/x8bit/bitwarden/ui/vault/feature/itemlisting/VaultItemListingViewModel.kt | Threads flag into item-listing view-state mapping. |
| app/src/test/kotlin/com/x8bit/bitwarden/ui/vault/feature/itemlisting/VaultItemListingViewModelTest.kt | Mocks flag state for item listing view-model tests. |
| app/src/main/kotlin/com/x8bit/bitwarden/ui/vault/feature/itemlisting/VaultItemListingContent.kt | Gates listing headers + collection icon resources via Compose helper. |
| app/src/main/kotlin/com/x8bit/bitwarden/ui/vault/feature/itemlisting/util/VaultItemListingDataExtensions.kt | Gates empty-state messaging and trailing icons in item listing mapping. |
| app/src/main/kotlin/com/x8bit/bitwarden/ui/vault/feature/item/VaultItemViewModel.kt | Threads flag into item screen “locations” icon choice. |
| app/src/test/kotlin/com/x8bit/bitwarden/ui/vault/feature/item/VaultItemViewModelTest.kt | Adds FeatureFlagManager mocking for the new flag in tests. |
| app/src/main/kotlin/com/x8bit/bitwarden/ui/vault/feature/item/VaultItemScreen.kt | Gates overflow menu labels between terminology variants. |
| app/src/main/kotlin/com/x8bit/bitwarden/ui/vault/feature/item/model/VaultItemLocation.kt | Allows collection location icon to vary (parameterized) for gating. |
| app/src/main/kotlin/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditViewModel.kt | Reads + subscribes to flag updates and gates terminology in validation and mapping. |
| app/src/test/kotlin/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditViewModelTest.kt | Mocks flag flows and updates expected state snapshots. |
| app/src/main/kotlin/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditScreen.kt | Gates overflow/menu labels and bottom-sheet titles via Compose helper. |
| app/src/main/kotlin/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditItemContent.kt | Gates field labels (folder/vault) via Compose helper. |
| app/src/main/kotlin/com/x8bit/bitwarden/ui/vault/feature/addedit/util/CipherViewExtensions.kt | Threads flag into owner naming in available owners list. |
| app/src/test/kotlin/com/x8bit/bitwarden/ui/vault/feature/addedit/util/CipherViewExtensionsTest.kt | Updates mapping tests to include the new flag parameter. |
| app/src/main/kotlin/com/x8bit/bitwarden/ui/vault/components/CollectionItemSelector.kt | Gates collections/shared-folders section labels and empty text. |
| app/src/main/kotlin/com/x8bit/bitwarden/ui/vault/feature/movetoorganization/VaultMoveToOrganizationViewModel.kt | Injects FeatureFlagManager and updates UI state when the flag changes. |
| app/src/test/kotlin/com/x8bit/bitwarden/ui/vault/feature/movetoorganization/VaultMoveToOrganizationViewModelTest.kt | Adds flag manager mocking and new state parameter coverage. |
| app/src/main/kotlin/com/x8bit/bitwarden/ui/vault/feature/movetoorganization/VaultMoveToOrganizationContent.kt | Gates strings shown in the move-to-organization UI via Compose helper. |
| app/src/test/kotlin/com/x8bit/bitwarden/ui/vault/feature/movetoorganization/VaultMoveToOrganizationScreenTest.kt | Updates screen test state to include isVfo1FoundationEnabled. |
| app/src/main/kotlin/com/x8bit/bitwarden/ui/platform/feature/settings/vault/VaultSettingsScreen.kt | Gates “My folders/Folders” settings row label. |
| app/src/main/kotlin/com/x8bit/bitwarden/ui/platform/feature/settings/folders/FoldersScreen.kt | Gates folders screen title. |
| app/src/main/kotlin/com/x8bit/bitwarden/ui/platform/feature/search/SearchViewModel.kt | Injects FeatureFlagManager and threads flag into search mapping. |
| app/src/test/kotlin/com/x8bit/bitwarden/ui/platform/feature/search/SearchViewModelTest.kt | Mocks FeatureFlagManager for VFO-1 flag in search tests. |
| app/src/main/kotlin/com/x8bit/bitwarden/ui/platform/feature/search/util/SearchTypeDataExtensions.kt | Gates trailing icons in search results mapping via the flag. |
| app/src/test/kotlin/com/x8bit/bitwarden/ui/vault/feature/vault/VaultScreenTest.kt | Passes feature flags state into compose test content. |
| app/src/test/kotlin/com/x8bit/bitwarden/ui/vault/feature/itemlisting/VaultItemListingScreenTest.kt | Passes feature flags state into compose test content. |
| app/src/test/kotlin/com/x8bit/bitwarden/ui/vault/feature/item/VaultItemScreenTest.kt | Passes feature flags state into compose test content. |
| app/src/test/kotlin/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditScreenTest.kt | Passes feature flags state into compose test content and updates default states. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fun VaultData.toViewState( | ||
| isPremium: Boolean, | ||
| hasMasterPassword: Boolean, | ||
| isIconLoadingDisabled: Boolean, | ||
| baseIconUrl: String, | ||
| vaultFilterType: VaultFilterType, | ||
| restrictItemTypesPolicyOrgIds: List<String>, | ||
| validTotpIds: Set<String>, | ||
| isNewItemTypesEnabled: Boolean, | ||
| isVfo1FoundationEnabled: Boolean = true, | ||
| ): VaultState.ViewState { |
| @@ -131,6 +131,7 @@ fun VaultData.toViewState( | |||
| totpData: TotpData?, | |||
| isPremiumUser: Boolean, | |||
| restrictItemTypesPolicyOrgIds: List<String>, | |||
| isVfo1FoundationEnabled: Boolean = true, | |||
| ): VaultItemListingState.ViewState { | |||
| fun List<CipherListView>.toViewState( | ||
| searchTerm: String, | ||
| baseIconUrl: String, | ||
| hasMasterPassword: Boolean, | ||
| isIconLoadingDisabled: Boolean, | ||
| isAutofill: Boolean, | ||
| isPremiumUser: Boolean, | ||
| isVfo1FoundationEnabled: Boolean = true, | ||
| ): SearchState.ViewState = |
| BitwardenMultiSelectButton( | ||
| label = stringResource(id = BitwardenString.vault), | ||
| dialogTitle = stringResource(id = BitwardenString.filter_by_vault), | ||
| dialogTitle = vfo1Foundation( | ||
| stringResource(id = BitwardenString.filter_by_vault), | ||
| null, | ||
| ), |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| @@ -40,7 +41,10 @@ fun VaultMoveToOrganizationContent( | |||
| item { | |||
| BitwardenMultiSelectButton( | |||
| label = stringResource(id = BitwardenString.vault), | |||
There was a problem hiding this comment.
Details and fix
dialogTitle on this same widget is gated, but the rest of the Organization→Vault rename (PR 7192) on this screen is not. With vfo1-foundation disabled the screen renders a mix of old and new wording:
| Element | Flag-off renders | Pre-VFO-1 |
|---|---|---|
label (line 43) |
vault → "Vault" |
organization → "Organization" |
supportingText (line 60) |
move_to_vault_desc → "Choose a vault…" |
move_to_org_desc |
VaultMoveToOrganizationEmpty.kt:29 |
no_vaults_to_list → "No vaults to list." |
no_orgs_to_list |
VaultMoveToOrganizationViewModel.appBarText |
move_to_vault → "Move to vault" |
move_to_organization → "Move to Organization" |
…while the shared-folders header and the validation error on the same screen do fall back to "Collections" / "You must select at least one collection."
organization still exists in strings.xml, but move_to_org_desc, no_orgs_to_list, and move_to_organization were removed by PR 7192 and were not restored in this PR, so completing the gate needs those three strings added back alongside the ones already restored in d61ba4dd2.
| */ | ||
| @Composable | ||
| @ReadOnlyComposable | ||
| fun <T> vfo1Foundation(new: T, old: T): T = |
| isIconLoadingDisabled: Boolean, | ||
| isAutofill: Boolean, | ||
| isPremiumUser: Boolean, | ||
| isVfo1FoundationEnabled: Boolean = true, |
There was a problem hiding this comment.
Is this default value a bit risky?
| label = stringResource( | ||
| id = vfo1Foundation( | ||
| BitwardenString.shared_folders, | ||
| BitwardenString.collections, |
There was a problem hiding this comment.
Can we add names to these params?
| text = stringResource( | ||
| id = vfo1Foundation( | ||
| BitwardenString.there_are_no_shared_folders_to_list, | ||
| BitwardenString.no_collections_to_list, |
| id = BitwardenString.move, | ||
| id = vfo1Foundation( | ||
| BitwardenString.move, | ||
| BitwardenString.move_to_vault, |
This branch's flag-off fallbacks for the Organization->Vault rename were restored from main, which turned out to be incompletely reverted (fixed separately in PM-41100). Corrects the "old" argument in three existing vfo1Foundation() calls from move_to_vault to move_to_organization, and gates two previously-hardcoded sites (the Move-to-organization dropdown label/description and its empty-state text) that were always showing "Vault" text regardless of the flag. Adds the move_to_organization/move_to_org_desc/ no_orgs_to_list strings needed for the flag-off path.
| <string name="move_to_organization">Move to Organization</string> | ||
| <string name="no_orgs_to_list">No organizations to list.</string> | ||
| <string name="move_to_org_desc">Choose an organization that you wish to move this item to. Moving to an organization transfers ownership of the item to that organization. You will no longer be the direct owner of this item once it has been moved.</string> | ||
| <string name="move_to_vault">Move to vault</string> |
There was a problem hiding this comment.
♻️ DEBT: move_to_vault is added here but never referenced.
Details and fix
The flag-on path uses BitwardenString.move and the flag-off path uses BitwardenString.move_to_organization (matching main after #7220), so nothing reads move_to_vault. move_to_vault_desc and no_vaults_to_list are both still used by the flag-on path, but this one is dead.
Suggest dropping the line so the eventual flag cleanup doesn't have to reason about it.
| isVfo1FoundationEnabled = featureFlagManager | ||
| .getFeatureFlag(FlagKey.Vfo1Foundation), |
There was a problem hiding this comment.
♻️ DEBT: Reading the flag here (not observing it) means flipping the kill switch leaves this screen in a mixed state until the vault data re-emits.
Details and fix
VaultItemListingContent resolves its section headers and group icons through vfo1Foundation(...) / LocalFeatureFlagsState, which recomposes as soon as MainViewModel observes the flag change. The per-row extraIconList comes from this toViewState call, which only re-runs on a new vaultDataStateFlow emission. So after the flag is flipped off, the header reads "Collections" while the rows still carry the ic_shared_folder icon and "Shared folders" content description.
Same shape in:
SearchViewModel.kt:1094VaultItemViewModel.kt:197VaultAddEditViewModel—Vfo1FoundationFlagUpdateReceiveupdates the boolean but never re-runsappendFolderAndOwnerData, soselectedOwner.namestays on the old value ("My vault" vs. the account email).
VaultViewModel.handleVfo1FoundationFlagUpdateReceive already shows the fix: observe getFeatureFlagFlow(FlagKey.Vfo1Foundation), store it in state, and re-derive the view state on update. Since the whole point of this flag is an instant kill switch, it'd be worth applying that pattern consistently.
Default isVfo1FoundationEnabled to false (matching the flag's kill-switch default) instead of true, so any future call site that omits it fails safe. Name the new/old arguments at every multi-line vfo1Foundation(...) call site for readability, and drop the now-unreferenced move_to_vault string.
| isAutofill: Boolean, | ||
| isFido2Creation: Boolean, | ||
| isPremiumUser: Boolean, | ||
| isVfo1FoundationEnabled: Boolean = true, |
There was a problem hiding this comment.
♻️ DEBT: Private helpers still default isVfo1FoundationEnabled to true, inverting the kill-switch's safe default.
Details and fix
The public entry points were changed to = false in response to earlier review feedback, but the private helpers were not:
VaultItemListingDataExtensions.kt:462,:495,:537,:544SearchTypeDataExtensions.kt:213,:236
Every current call site passes the value explicitly, so these defaults are unreachable today — but if a new call site omits the argument it silently gets VFO-1 terminology while vfo1-foundation is off, which is the opposite of what the kill switch is for.
Suggested fix: drop the default entirely on the private helpers (they are all called with an explicit argument), or set it to false to match the flag default and the public functions.
The public toViewState/toDisplayItem entry points were already flipped to isVfo1FoundationEnabled = false, but their private per-item helpers in VaultItemListingDataExtensions.kt and SearchTypeDataExtensions.kt still defaulted to true, leaving an unreachable-today but unsafe default.
| ), | ||
| dialogState = null, | ||
| onlyShowCollections = false, | ||
| isVfo1FoundationEnabled = true, |
There was a problem hiding this comment.
♻️ DEBT: This test renders a flag combination that can't occur in production, and drops flag-on coverage for this screen.
Details and fix
setup() calls setContent { … } without featureFlagsState, so BitwardenComposeTest supplies FeatureFlagsState() → isVfo1FoundationEnabled = false. The state here sets it to true. The screen therefore renders half-old / half-new:
| Element | Source | Renders |
|---|---|---|
appBarText |
state.isVfo1FoundationEnabled = true |
"Move" / "Shared folders" |
label, supportingText, collections header, empty text |
vfo1Foundation(...) → LocalFeatureFlagsState = false |
"Organization", "Choose an organization that…", "Collections" |
Because of that, the assertions at lines 139/149, 156/164, 204 and 236 had to be changed from the VFO-1 wording back to the pre-VFO-1 wording — so the flag-on rendering of those elements (the state that actually ships when the flag is enabled) is no longer asserted anywhere for this screen.
The other four screen tests touched by this PR (VaultAddEditScreenTest:140, VaultItemScreenTest:94, VaultItemListingScreenTest:136, VaultScreenTest:112) all pass featureFlagsState = FeatureFlagsState(isVfo1FoundationEnabled = true). Suggest doing the same here and restoring the original VFO-1 assertions, keeping the new the app bar title should display the old text when the flag is off test (or adding a second setContent variant) for the flag-off path.
…ization screen test setup() left LocalFeatureFlagsState.isVfo1FoundationEnabled at its false default while the mocked ViewModel state defaulted isVfo1FoundationEnabled to true, so the screen rendered a half-old/half-new combination that can't happen in production and the assertions had drifted to pre-VFO1 wording, dropping flag-on coverage. Pass featureFlagsState explicitly to match the other four screen tests and restore the VFO-1 wording assertions.
| override val icon: Int | ||
| get() = BitwardenDrawable.ic_shared_folder | ||
| } | ||
| @DrawableRes override val icon: Int = BitwardenDrawable.ic_shared_folder, |
There was a problem hiding this comment.
♻️ DEBT: icon defaults to the VFO-1 value, the last remaining flag default that isn't fail-safe.
Details and fix
Every other isVfo1FoundationEnabled parameter in this PR was flipped to default to the pre-VFO-1 value so an omitted argument fails safe. This default goes the other way: any construction that omits icon gets ic_shared_folder even with the kill switch off.
VaultItemViewModel.kt:208 is the only production caller and it passes icon explicitly, so there's no user-visible impact today — but the ItemHeader.kt previews (lines 481–532) and VaultItemScreenTest/VaultItemViewModelTest all rely on the default and silently render new terminology.
@DrawableRes override val icon: Int = BitwardenDrawable.ic_collections,…FO1 value icon defaulted to ic_shared_folder (the VFO-1 icon), the last remaining non-fail-safe default in this gating work. The only production caller (VaultItemViewModel) already passes the flag-derived icon explicitly, so flip the default to ic_collections and pin the mockk stub matchers in VaultItemViewModelTest that relied on the old default.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-41057
📔 Objective
Adds a
vfo1-foundationfeature flag and gates the already-developed VFO-1 terminology rename work (Owner→Vault, Collection→Shared folder, Organization→Vault, Folders→My folders) behind it, so this branch can merge tomainwith an instant kill-switch instead of relying on a revert PR if issues surface.The flag is exposed via
MainState.featureFlagsState→LocalFeatureFlagsState, following the pattern already scaffolded in the codebase, and read directly viaFeatureFlagManagerin ViewModels/data-mapping functions that can't access a CompositionLocal. Renamed strings/icons render the old terminology when the flag is off, and the previously-deleted strings/icon needed for that fallback have been restored. Default isfalse.📸 Screenshots
Screen_recording_20260729_161938.mp4