[PM-36251] feat: Navigate archive premium upsell through in-app upgrade flow#2607
Conversation
github-actions
Bot
added
app:password-manager
andrebispo5
added
the
ai-review-vnext
There was a problem hiding this comment.
Pull request overview
Routes the “Archive unavailable” upsell CTA through the same premium in-app upgrade flow as the vault list banner (with a web vault URL fallback when in-app upgrade isn’t available), and refactors premium-upgrade banner eligibility vs. dismissal into distinct concerns.
Changes:
- Split premium upgrade logic into eligibility (
isPremiumUpgradeEligible) vs. banner dismissal (isPremiumUpgradeBannerDismissed). - Centralized in-app upgrade availability checks and navigation fallback logic inside
VaultListProcessor. - Updated/added tests to cover banner dismissal behavior and archive CTA routing (in-app vs web fallback).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| BitwardenShared/UI/Vault/Vault/VaultList/VaultListProcessor.swift | Refactors premium upgrade banner visibility logic and routes archive upsell to in-app upgrade when available, otherwise web URL fallback. |
| BitwardenShared/UI/Vault/Vault/VaultList/VaultListProcessorTests.swift | Updates banner tests for new eligibility/dismissal split; adds archive CTA routing tests. |
| BitwardenShared/Core/Platform/Services/StateService.swift | Updates StateService API by replacing shouldShowPremiumUpgradeBanner() with eligibility + dismissal methods and implements them in DefaultStateService. |
| BitwardenShared/Core/Platform/Services/StateServiceTests.swift | Renames/adjusts tests for the new StateService API and adds dismissal-specific tests. |
| BitwardenShared/Core/Platform/Services/TestHelpers/MockStateService.swift | Updates the mock to match the new StateService API used by tests/processors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let alert = coordinator.alertShown.last | ||
| try? await alert?.tapAction(title: Localizations.upgradeToPremium) | ||
| try await waitForAsync { self.coordinator.routes.last == .premiumUpgrade } |
There was a problem hiding this comment.
Avoid try? + optional chaining here; if the alert/action isn't present, the test will silently continue and could pass incorrectly. Unwrap the alert and use try await so failures are surfaced.
| let alert = coordinator.alertShown.last | ||
| XCTAssertEqual(alert?.title, Localizations.archiveUnavailable) | ||
| XCTAssertEqual(alert?.message, Localizations.archivingItemsIsAPremiumFeatureDescriptionLong) | ||
|
|
||
| try? await alert?.tapAction(title: Localizations.upgradeToPremium) |
There was a problem hiding this comment.
Avoid try? + optional chaining here; if the alert/action isn't present, the test will silently continue and could pass incorrectly. Unwrap the alert and use try await so failures are surfaced.
| let alert = coordinator.alertShown.last | |
| XCTAssertEqual(alert?.title, Localizations.archiveUnavailable) | |
| XCTAssertEqual(alert?.message, Localizations.archivingItemsIsAPremiumFeatureDescriptionLong) | |
| try? await alert?.tapAction(title: Localizations.upgradeToPremium) | |
| let alert = try XCTUnwrap(coordinator.alertShown.last) | |
| XCTAssertEqual(alert.title, Localizations.archiveUnavailable) | |
| XCTAssertEqual(alert.message, Localizations.archivingItemsIsAPremiumFeatureDescriptionLong) | |
| try await alert.tapAction(title: Localizations.upgradeToPremium) |
| @@ -2672,12 +2672,12 @@ class StateServiceTests: BitwardenTestCase { // swiftlint:disable:this type_body | |||
| ))) | |||
| appSettingsStore.premiumUpgradeBannerDismissedByUserId["1"] = false | |||
|
|
|||
| let shouldShow = await subject.shouldShowPremiumUpgradeBanner() | |||
| let shouldShow = await subject.isPremiumUpgradeEligible() | |||
| XCTAssertTrue(shouldShow) | |||
There was a problem hiding this comment.
The doc comment for test_isPremiumUpgradeEligible_true still says eligibility requires the banner to not be dismissed, but isPremiumUpgradeEligible() no longer checks dismissal (that’s covered by isPremiumUpgradeBannerDismissed()). Please update the comment (and consider renaming shouldShow to isEligible for clarity).
| /// `receive(_:)` with `.itemPressed` navigates to the premium upgrade screen even when the | ||
| /// banner has been dismissed, since the archive entry point bypasses the dismissal check. | ||
| @MainActor | ||
| func test_receive_itemPressed_archiveGroup_noPremium_noItems_actionTapped_bannerDismissed() async throws { | ||
| configService.featureFlagsBool[.premiumUpgradePath] = true | ||
| stateService.isPremiumUpgradeEligibleResult = true | ||
| vaultRepository.hasMinimumCipherCountResult = .success(true) | ||
| storefrontService.isUSStorefrontReturnValue = true | ||
| let statusSubject = PassthroughSubject<PremiumCheckoutStatus, Never>() | ||
| billingService.premiumCheckoutStatusPublisherReturnValue = statusSubject.eraseToAnyPublisher() |
There was a problem hiding this comment.
This test claims the archive upgrade flow should work even when the banner is dismissed, but it never sets stateService.isPremiumUpgradeBannerDismissedResult = true. As written it doesn't actually verify the dismissal-bypass behavior.
| @@ -2142,13 +2164,61 @@ class VaultListProcessorTests: BitwardenTestCase { // swiftlint:disable:this typ | |||
| XCTAssertEqual(alert?.title, Localizations.archiveUnavailable) | |||
| XCTAssertEqual(alert?.message, Localizations.archivingItemsIsAPremiumFeatureDescriptionLong) | |||
|
|
|||
| XCTAssertTrue(vaultRepository.archiveCipher.isEmpty) | |||
| try? await alert?.tapAction(title: Localizations.upgradeToPremium) | |||
| try await waitForAsync { self.coordinator.routes.last == .premiumUpgrade } | |||
There was a problem hiding this comment.
Avoid try? + optional chaining here; if the alert/action isn't present, the test will silently continue and could pass incorrectly. Unwrap the alert and use try await so failures are surfaced.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2607 +/- ##
==========================================
- Coverage 87.28% 86.27% -1.01%
==========================================
Files 1908 2137 +229
Lines 169827 184896 +15069
==========================================
+ Hits 148226 159512 +11286
- Misses 21601 25384 +3783 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| func isPremiumUpgradeBannerDismissed() async -> Bool | ||
|
|
||
| /// Returns whether the user meets the eligibility criteria for the premium upgrade | ||
| /// (free account and 7+ days old). |
There was a problem hiding this comment.
⛏️ I wouldn't say which are the requirements here in the protocol as if they change you need to remember updating this comment as well. Also the caller doesn't need to know which are the requirements just what the function does and returns.
So for easier maintainability I'd just remove this line
| /// (free account and 7+ days old). |
| } | ||
|
|
||
| func isPremiumUpgradeBannerDismissed() async -> Bool { | ||
| await ((try? getPremiumUpgradeBannerDismissed()) ?? false) |
There was a problem hiding this comment.
⛏️ I think you can drop the outer parenthesis here:
| await ((try? getPremiumUpgradeBannerDismissed()) ?? false) | |
| await (try? getPremiumUpgradeBannerDismissed()) ?? false |
🤔 Also, I think you should log the error if getPremiumUpgradeBannerDismissed throws and then return false instead of of using try? here. Or is it fine to ignore the errors on this function?
| guard timeProvider.timeSince(creationDate) >= Constants.premiumUpgradeBannerAccountAge else { | ||
| return false | ||
| } | ||
|
|
||
| return true |
There was a problem hiding this comment.
🎨 Can't you just return the operation result here?
| guard timeProvider.timeSince(creationDate) >= Constants.premiumUpgradeBannerAccountAge else { | |
| return false | |
| } | |
| return true | |
| return timeProvider.timeSince(creationDate) >= Constants.premiumUpgradeBannerAccountAge |
| guard (try? await services.vaultRepository | ||
| .hasMinimumCipherCount(Constants.minimumPremiumUpgradeBannerCipherCount)) ?? false |
There was a problem hiding this comment.
🤔 Shouldn't this be logging the error when hasMinimumCipherCount throws instead of ignoring it?
| guard (try? await services.vaultRepository | ||
| .hasMinimumCipherCount(Constants.minimumPremiumUpgradeBannerCipherCount)) ?? false | ||
| else { return false } | ||
| guard await services.storefrontService.isUSStorefront() else { return false } |
There was a problem hiding this comment.
🎨 IMO this services.storefrontService.isUSStorefront should be checked before hasMinimumCipherCount or even services.stateService.isPremiumUpgradeEligible as it should be faster and consume less resources than the others, as it's just a cached value.
| if await isInAppUpgradeAvailable() { | ||
| subscribeToPremiumCheckoutStatus() | ||
| coordinator.navigate(to: .premiumUpgrade) | ||
| } else { | ||
| state.url = services.environmentService.upgradeToPremiumURL | ||
| } |
There was a problem hiding this comment.
⛏️ I'd use a guard here to avoid the nesting blocks and be more "Swifty":
| if await isInAppUpgradeAvailable() { | |
| subscribeToPremiumCheckoutStatus() | |
| coordinator.navigate(to: .premiumUpgrade) | |
| } else { | |
| state.url = services.environmentService.upgradeToPremiumURL | |
| } | |
| guard await isInAppUpgradeAvailable() else { | |
| state.url = services.environmentService.upgradeToPremiumURL | |
| return | |
| } | |
| subscribeToPremiumCheckoutStatus() | |
| coordinator.navigate(to: .premiumUpgrade) |
| let isBannerDismissed = await services.stateService.isPremiumUpgradeBannerDismissed() | ||
| let isUpgradeAvailable = await isInAppUpgradeAvailable() | ||
| state.shouldShowPremiumUpgradeActionCard = !isBannerDismissed && isUpgradeAvailable |
There was a problem hiding this comment.
🎨 If the banner has already been dismissed then it's wasteful to check for isInAppUpgradeAvailable as that value doesn't really matter later. So I would rewrite this with short-circuit as:
let isBannerDismissed = await services.stateService.isPremiumUpgradeBannerDismissed()
guard !isBannerDismissed else {
state.shouldShowPremiumUpgradeActionCard = false
return
}
state.shouldShowPremiumUpgradeActionCard = await isInAppUpgradeAvailable()or like this:
let isBannerDismissed = await services.stateService.isPremiumUpgradeBannerDismissed()
state.shouldShowPremiumUpgradeActionCard = !isBannerDismissed && (await isInAppUpgradeAvailable())
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE This PR wires the archive unavailable upsell CTA through the in-app premium upgrade flow used by the vault list banner, introduces a Code Review DetailsNo new findings beyond those already captured in existing open review threads on this PR. The substantive concerns raised earlier — including the archive-upsell subscription path, the duplication of Minor observations not worth their own inline:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| coordinator.showAlert( | ||
| Alert.archiveUnavailable(action: { [weak self] in | ||
| guard let self else { return } | ||
| state.url = services.environmentService.upgradeToPremiumURL | ||
| Task { [weak self] in | ||
| await self?.navigateToPremiumUpgrade() | ||
| } |
There was a problem hiding this comment.
❓ QUESTION: The same Alert.archiveUnavailable is also shown from VaultItemMoreOptionsHelper.archive() (when a non-premium user taps Archive on an individual cipher), but that path still goes straight to services.environmentService.upgradeToPremiumURL and bypasses the new in-app upgrade flow.
Details
BitwardenShared/UI/Vault/Helpers/VaultItemMoreOptionsHelper.swift:131-134:
Alert.archiveUnavailable(action: { [weak self] in
guard let self else { return }
handleOpenURL(services.environmentService.upgradeToPremiumURL)
}),Was the more-options entry point intentionally left out of scope, or should it be migrated to the same navigateToPremiumUpgrade() path so that the identical alert routes consistently from both entry points?
| coordinator.showAlert( | ||
| Alert.archiveUnavailable(action: { [weak self] in | ||
| guard let self else { return } | ||
| handleOpenURL(services.environmentService.upgradeToPremiumURL) | ||
| Task { [weak self] in | ||
| guard let self else { return } | ||
| if await self.services.billingRepository.isInAppUpgradeAvailable() { | ||
| self.coordinator.navigate(to: .premiumUpgrade) | ||
| } else { | ||
| handleOpenURL(self.services.environmentService.upgradeToPremiumURL) | ||
| } | ||
| } | ||
| }), | ||
| ) |
There was a problem hiding this comment.
.premiumUpgrade without subscribing to premiumCheckoutStatusPublisher, so a successful checkout never dismisses the upgrade screen.
Details and fix
PremiumUpgradeProcessor.subscribeToPremiumCheckoutStatus only handles .canceled itself; for .confirmed, .pending, and .syncing it relies on the parent owning the dismiss (see the explicit comment in PremiumUpgradeProcessor.swift:120-122: "VaultListProcessor owns the dismiss and all post-dismiss actions via DismissAction for each of these states.").
VaultListProcessor.navigateToPremiumUpgrade() calls subscribeToPremiumCheckoutStatus() before navigating, but the new code path in this helper does not. Concrete consequences when in-app upgrade is available and the user completes checkout from the archive more-options entry:
- The premium upgrade modal does not dismiss after
.confirmed(noDismissAction). - The vault is not refreshed and
state.hasPremiumis not updated, so the archive group still appears unavailable. - The "upgrade pending" alert is not shown for
.pending. - The confirming-loading overlay is not shown for
.syncing.
VaultItemMoreOptionsHelper is a @MainActor helper, not a StateProcessor, so it cannot reasonably own the dismiss/refresh logic. The cleanest fix is to keep the upsell routing centralized in VaultListProcessor — either by surfacing this through the existing .upgradeToPremium action, or by exposing a coordinator/delegate hook so the processor can subscribe before navigation.
Reference: BitwardenShared/UI/Billing/PremiumUpgrade/PremiumUpgradeProcessor.swift:117-124, BitwardenShared/UI/Vault/Vault/VaultList/VaultListProcessor.swift:562-569.
github-actions
Bot
added
the
app:authenticator
| @@ -0,0 +1,11 @@ | |||
| @testable import BitwardenShared | |||
|
|
|||
| class MockBillingRepository: BillingRepository { | |||
There was a problem hiding this comment.
🤔 BillingRepository is already marked as AutoMockable; we shouldn't need a manual mock, right?
| /// A protocol for a `BillingRepository` which manages access to billing-related data | ||
| /// needed by the UI layer. | ||
| /// | ||
| protocol BillingRepository: AnyObject { // sourcery: AutoMockable |
There was a problem hiding this comment.
🤔 Why is it necessary for this to conform to AnyObject?
| @testable import BitwardenShared | ||
| @testable import BitwardenSharedMocks | ||
|
|
||
| class BillingRepositoryTests: BitwardenTestCase { |
There was a problem hiding this comment.
🎨 Ideally new tests are Swift Testing, instead of XCTest. The skill in the repo should be able to convert it for you fairly easily
| /// | ||
| /// - Returns: `true` if the user has dismissed the banner. | ||
| /// | ||
| func isPremiumUpgradeBannerDismissed() async -> Bool |
There was a problem hiding this comment.
🤔 I have to wonder if it would make sense to actually put these in a BillingStateService for some sort of interface segregation, like we have with the ConfigStateService, BiometricsStateService, UserSessionStateService, etc.
| /// Checks (in order): | ||
| /// 1. The `.premiumUpgradePath` feature flag is enabled. | ||
| /// 2. The App Store storefront is in the United States. | ||
| /// 3. The user is eligible for a premium upgrade (not already premium and account is 7+ days old). | ||
| /// 4. The vault contains at least the minimum cipher count. | ||
| /// |
There was a problem hiding this comment.
⛏️ Remove what this checks inside. Unless it's strictly necessary this shouldn't be added to protocol docs as the caller should only care in this case if the in-app premium upgrade path is available and not how billing internally checks that.
👍 The callout for the banner dismissal is correct as it's something that callers may need to know for their logic.
| /// Checks (in order): | |
| /// 1. The `.premiumUpgradePath` feature flag is enabled. | |
| /// 2. The App Store storefront is in the United States. | |
| /// 3. The user is eligible for a premium upgrade (not already premium and account is 7+ days old). | |
| /// 4. The vault contains at least the minimum cipher count. | |
| /// |
| guard await configService.getFeatureFlag(.premiumUpgradePath) else { return false } | ||
| guard await storefrontService.isUSStorefront() else { return false } | ||
| guard await stateService.isPremiumUpgradeEligible() else { return false } |
There was a problem hiding this comment.
⛏️ You can use comma separated conditions here with one guard:
| guard await configService.getFeatureFlag(.premiumUpgradePath) else { return false } | |
| guard await storefrontService.isUSStorefront() else { return false } | |
| guard await stateService.isPremiumUpgradeEligible() else { return false } | |
| guard await configService.getFeatureFlag(.premiumUpgradePath), | |
| storefrontService.isUSStorefront(), | |
| stateService.isPremiumUpgradeEligible() | |
| else { | |
| return false | |
| } |
| /// A default implementation of `StateService`. | ||
| /// | ||
| actor DefaultStateService: StateService, ActiveAccountStateProvider, ConfigStateService, FlightRecorderStateService, LanguageStateService { // swiftlint:disable:this type_body_length line_length | ||
| actor DefaultStateService: StateService, ActiveAccountStateProvider, BillingStateService, ConfigStateService, FlightRecorderStateService, LanguageStateService { // swiftlint:disable:this type_body_length line_length |
There was a problem hiding this comment.
🎨 This file is gigantic, could you move this to BillingStateService as an extension so the implementation is there? You can check ServerCommunicationConfigStateService as an example.
Also move its tests.
| coordinator.showAlert( | ||
| Alert.archiveUnavailable(action: { [weak self] in | ||
| guard let self else { return } | ||
| handleOpenURL(services.environmentService.upgradeToPremiumURL) | ||
| Task { [weak self] in | ||
| guard let self else { return } | ||
| if await self.services.billingRepository.isInAppUpgradeAvailable() { | ||
| self.coordinator.navigate(to: .premiumUpgrade) | ||
| } else { | ||
| handleOpenURL(self.services.environmentService.upgradeToPremiumURL) | ||
| } | ||
| } | ||
| }), | ||
| ) |
…ingStateService extension
…ectionProcessor to check isInAppUpgradeAvailable
…ot of presented navigator
…nd add premium upgrade coverage
| private func subscribeToPremiumCheckoutStatus() { | ||
| premiumStatusChangedCancellable = services.billingService | ||
| .premiumCheckoutStatusPublisher() | ||
| .receive(on: DispatchQueue.main) | ||
| .sink { [weak self] status in | ||
| guard let self else { return } | ||
| switch status { | ||
| case .canceled: | ||
| break | ||
| case .confirmed: | ||
| premiumStatusChangedCancellable = nil | ||
| coordinator.navigate( | ||
| to: .dismiss(DismissAction { [weak self] in | ||
| guard let self else { return } | ||
| coordinator.hideLoadingOverlay() | ||
| Task { await self.refreshVaultGroup() } | ||
| }), | ||
| ) | ||
| case .pending: | ||
| coordinator.navigate( | ||
| to: .dismiss(DismissAction { [weak self] in | ||
| guard let self else { return } | ||
| coordinator.hideLoadingOverlay() | ||
| coordinator.showAlert(.upgradePending { | ||
| await self.services.billingService.premiumStatusChanged() | ||
| }) | ||
| }), | ||
| ) | ||
| case .syncing: | ||
| coordinator.navigate( | ||
| to: .dismiss(DismissAction { [weak self] in | ||
| guard let self else { return } | ||
| coordinator.showLoadingOverlay( | ||
| LoadingOverlayState(title: Localizations.confirmingYourUpgrade), | ||
| ) | ||
| }), | ||
| ) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
♻️ DEBT: New subscribeToPremiumCheckoutStatus() lacks coverage for the .confirmed, .pending, and .syncing branches.
Details
The existing VaultGroupProcessorTests.test_perform_morePressed_navigateToPremiumUpgrade_inAppUpgradeAvailable only asserts that premiumCheckoutStatusPublisher() was called. None of the dismiss / loading-overlay / pending-alert / refreshVaultGroup paths inside this sink are exercised.
VaultListProcessorTests already provides this coverage (test_subscribeToPremiumCheckoutStatus_confirmed/pending/syncing/canceled). Mirroring that pattern here would prevent regressions in the new in-app upgrade flow for vault groups.
The same gap exists in VaultItemSelectionProcessor.swift:166-204 — its tests also only assert the publisher was called, not that .confirmed/.pending/.syncing produce the expected dismiss/alert/overlay behavior.
There was a problem hiding this comment.
This navigation will change in a future PR, the amount of work to refactor it is not worth it for now.
… to VaultGroup and VaultItemSelection processors
| private func subscribeToPremiumCheckoutStatus() { | ||
| premiumStatusChangedCancellable = services.billingService | ||
| .premiumCheckoutStatusPublisher() | ||
| .receive(on: DispatchQueue.main) | ||
| .sink { [weak self] status in | ||
| guard let self else { return } | ||
| switch status { | ||
| case .canceled: | ||
| break | ||
| case .confirmed: | ||
| premiumStatusChangedCancellable = nil | ||
| coordinator.navigate( | ||
| to: .dismiss(DismissAction { [weak self] in | ||
| guard let self else { return } | ||
| coordinator.hideLoadingOverlay() | ||
| Task { await self.refreshVaultGroup() } | ||
| }), | ||
| ) | ||
| case .pending: | ||
| coordinator.navigate( | ||
| to: .dismiss(DismissAction { [weak self] in | ||
| guard let self else { return } | ||
| coordinator.hideLoadingOverlay() | ||
| coordinator.showAlert(.upgradePending { | ||
| await self.services.billingService.premiumStatusChanged() | ||
| }) | ||
| }), | ||
| ) | ||
| case .syncing: | ||
| coordinator.navigate( | ||
| to: .dismiss(DismissAction { [weak self] in | ||
| guard let self else { return } | ||
| coordinator.showLoadingOverlay( | ||
| LoadingOverlayState(title: Localizations.confirmingYourUpgrade), | ||
| ) | ||
| }), | ||
| ) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
♻️ DEBT: subscribeToPremiumCheckoutStatus() is duplicated across three processors with only the .confirmed branch differing.
Details and fix
This PR introduces near-identical copies of navigateToPremiumUpgrade() + subscribeToPremiumCheckoutStatus() in VaultListProcessor, VaultGroupProcessor, and VaultItemSelectionProcessor. The .canceled, .pending, and .syncing branches are byte-for-byte the same; only the post-.confirmed action varies (refresh vault list vs. refresh vault group vs. nothing).
This couples three processors to identical orchestration logic. A bug fix or behavior change to the pending/syncing branch must be applied in three places, and the duplication is likely to grow as more entry points are added.
Suggested approaches:
- Extract a
PremiumCheckoutFlowHandler(or similar) helper holding theAnyCancellableand accepting anonConfirmed: () async -> Voidclosure for the variant behavior. - Or expose the orchestration on the existing
BillingService/BillingRepositoryso processors only inject a confirmation callback.
Worth doing as a follow-up since the pattern is now anchored in three places.
There was a problem hiding this comment.
This navigation will change in a future PR, the amount of work to refactor it is not worth it for now.
# Conflicts: # BitwardenShared/UI/Billing/BillingCoordinatorTests.swift
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-36251
📔 Objective
Wires the archive unavailable upsell CTA to the same in-app premium upgrade flow used by the vault list banner, instead of always opening the web vault URL.
shouldShowPremiumUpgradeBanner→isPremiumUpgradeEligibleto better reflect that it checks user eligibility, not banner visibility.isPremiumUpgradeBannerDismissedas a separateStateServicemethod so banner dismissal is a distinct concern.isInAppUpgradeAvailable()andnavigateToPremiumUpgrade()toVaultListProcessor, encapsulating the routing decision (in-app upgrade flow vs. web vault fallback) based on the feature flag and US storefront.