Skip to content

[PM-37992] fix: Show subscription attention card in empty vault and suppress conflicting CTAs#2867

Open
KatherineInCode wants to merge 10 commits into
mainfrom
pm-37992/cta-fixes
Open

[PM-37992] fix: Show subscription attention card in empty vault and suppress conflicting CTAs#2867
KatherineInCode wants to merge 10 commits into
mainfrom
pm-37992/cta-fixes

Conversation

@KatherineInCode

Copy link
Copy Markdown
Contributor

🎟️ Tracking

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

📔 Objective

QA follow-up on #2823. Two failures were found:

  1. Card missing for empty/near-empty vaultssubscriptionNeedsAttentionActionCard was only rendered inside vaultContents(with:), which only displays when the vault has items. Users with 0–4 vault items saw emptyVault instead, where the card was absent. Fixed by adding the card to emptyVault.

  2. Subscription attention card and archive onboarding card appear simultaneously — When a past-due user had an undismissed archive onboarding CTA, both cards showed at once. Fixed by suppressing archiveOnboardingActionCard (and importLoginsActionCard in the empty vault) when the subscription attention card is visible.

Also converts VaultListView previews from PreviewProvider to #Preview macros, and adds previews for the two failing states.

📸 Screenshots

…conflicting CTAs

- Add subscriptionNeedsAttentionActionCard to emptyVault so past-due users
  with 0-4 vault items see the card
- Suppress importLoginsActionCard and archiveOnboardingActionCard when
  subscription attention card is showing (priority suppression, not dismissal)
- Add ViewInspector tests for all new states
- Replace deleted _allPreviews snapshot catch-all with individual disabled
  snapshot tests for each uncovered state
@github-actions github-actions Bot added app:password-manager Bitwarden Password Manager app context t:bug Change Type - Bug labels Jul 8, 2026
@codecov

codecov Bot commented Jul 8, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 76.47059% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.65%. Comparing base (438d632) to head (86f8887).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...hared/UI/Vault/Vault/VaultList/VaultListView.swift 69.23% 12 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2867      +/-   ##
==========================================
+ Coverage   81.27%   81.65%   +0.38%     
==========================================
  Files        1028     1028              
  Lines       66164    65827     -337     
==========================================
- Hits        53773    53750      -23     
+ Misses      12391    12077     -314     

☔ View full report in Codecov by Harness.
📢 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.

… priority cascade

Add VaultListActionCard enum and activeActionCard computed property to
VaultListState, implementing an explicit priority ordering (upgradedToPremium
> upgradeNeeded > subscriptionNeedsAttention > introducingArchive > importItems).
Replace the five individual @ViewBuilder card properties with a single actionCard
view that switches on activeActionCard, so only one card is ever rendered at a time.
@KatherineInCode KatherineInCode marked this pull request as ready for review July 9, 2026 14:15
@KatherineInCode KatherineInCode requested review from a team and matt-livefront as code owners July 9, 2026 14:15
@KatherineInCode KatherineInCode added the ai-review Request a Claude code review label Jul 9, 2026
@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the QA follow-up to #2823, which consolidates the five separate action-card view builders into a single priority-driven actionCard backed by a new activeActionCard computed property and VaultListActionCard enum. This correctly fixes the two reported issues (subscription attention card missing in the empty/near-empty vault, and simultaneous display of subscription + archive cards) by rendering exactly one card by priority in both emptyVault and vaultContents. Test coverage is strong — the exhaustive 32-subset priority test and the migration of VaultListStateTests to Swift Testing are well done, and the ViewInspector tests verify the suppression behavior. One behavioral question is noted inline.

Code Review Details
  • ❓ : Import logins card can now appear in a populated vault (previously only shown in the empty vault); confirm this is intended.
    • BitwardenShared/UI/Vault/Vault/VaultList/VaultListState.swift:93

assertSnapshots(of: subject, as: [.defaultPortrait, .defaultPortraitDark])
}

// The following tests have no direct preview counterpart.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⛏️ Is this still needed?

Comment on lines +35 to +37
/// `activeActionCard` returns the highest-priority active card across all 32 flag combinations,
/// or `nil` when no flags are set.
@Test(arguments: allCardSubsets)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Nice!

if shouldShowPremiumUpgradeActionCard { return .upgradeNeeded }
if shouldShowSubscriptionAttentionCard { return .subscriptionNeedsAttention }
if shouldShowArchiveOnboardingActionCard { return .introducingArchive }
if shouldShowImportLoginsActionCard { return .importItems }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QUESTION: Import logins card can now appear in a populated vault, which wasn't possible before this refactor.

Details

Previously importLoginsActionCard was rendered only in emptyVault, while vaultContents rendered the premium/subscription/upgrade/archive cards. Both now share actionCard, which resolves through activeActionCard. Because importItems is in the priority chain, a user with importLoginsSetupProgress == .incomplete and a populated vault (and no higher-priority card active) will now see the import logins card in vaultContents — where it never appeared before.

importLoginsSetupProgress is driven by account setup progress (set to .incomplete during onboarding, cleared on dismiss/completion), independent of whether the vault has items, so this state combination is reachable.

Is showing the import logins card in a populated vault the intended behavior? If not, importItems should likely remain gated to the empty-vault path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review app:password-manager Bitwarden Password Manager app context t:bug Change Type - Bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants