close
Skip to content

[PM-33519] feat: Rewire upgrade CTAs to use conditional routing#6796

Draft
SaintPatrck wants to merge 2 commits intopremium-upgrade/PM-33518-upgrade-available-flowfrom
premium-upgrade/PM-33519-rewire-ctas
Draft

[PM-33519] feat: Rewire upgrade CTAs to use conditional routing#6796
SaintPatrck wants to merge 2 commits intopremium-upgrade/PM-33518-upgrade-available-flowfrom
premium-upgrade/PM-33519-rewire-ctas

Conversation

@SaintPatrck
Copy link
Copy Markdown
Contributor

🎟️ Tracking

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

📔 Objective

Rewire all existing premium upgrade CTAs across the app to conditionally route to either the in-app PlanScreen (modal) or the external web vault URL based on isInAppUpgradeAvailableFlow.

Stacked on PM-33518 PR (upgrade available flow)

Changes

  • Inject PremiumStateManager into ViewModels that have upgrade CTAs: VaultViewModel, VaultItemViewModel, VaultItemListingViewModel, VaultAddEditViewModel, AddEditSendViewModel, SearchViewModel
  • Add NavigateToPlanModal event and onNavigateToPlan callback to affected screens and navigation graphs
  • Conditionally route: if in-app upgrade available → navigate to PlanScreen modal; otherwise → launch web vault URL as before
  • Thread onNavigateToPlan through VaultUnlockedNavigation, VaultGraphNavigation, SendGraphNavigation, and all affected screen destinations
  • Full test coverage for conditional routing in all affected ViewModels and Screens

@SaintPatrck SaintPatrck added the ai-review-vnext Request a Claude code review using the vNext workflow label Apr 14, 2026
@github-actions github-actions Bot added app:password-manager Bitwarden Password Manager app context t:feature Change Type - Feature Development labels Apr 14, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 14, 2026

🤖 Bitwarden Claude Code Review

Overall Assessment: REQUEST CHANGES

This PR rewires premium upgrade CTAs across six ViewModels (Vault, VaultItem, VaultItemListing, VaultAddEdit, AddEditSend, Search) to conditionally route to the in-app PlanScreen modal when PremiumStateManager.isInAppUpgradeAvailable() returns true, falling back to the existing web vault URL otherwise. New NavigateToPlanModal events are added per-ViewModel (except VaultViewModel, which reuses the pre-existing NavigateToUpgradePremium), and an onNavigateToPlan callback is threaded through VaultUnlockedNavigation, VaultGraphNavigation, SendGraphNavigation, and all affected screen destinations. Test coverage is added for both branches of each ViewModel's upgrade handler and for most screens' event handling.

Code Review Details
  • ⚠️ : VaultItemListingScreen.onNavigateToPlan is the only screen callback with a default = {} and VaultItemListingScreenTest lacks a NavigateToPlanModal test, while the other five affected screens declare it as required and test the event (already noted in the existing unresolved thread on VaultItemListingScreen.kt:88 — no new comment posted to avoid duplication)

onNavigateToAddEditSendItem: (route: AddEditSendRoute) -> Unit,
onNavigateToViewSendItem: (route: ViewSendRoute) -> Unit,
onNavigateToSearch: (searchType: SearchType) -> Unit,
onNavigateToPlan: () -> Unit = {},
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.

⚠️ IMPORTANT: Default empty lambda on navigation callback silently swallows upgrade navigation

Details and fix

All other Screen composables in this PR define onNavigateToPlan as a required parameter (no default). This is the only one with = {}, which deviates from the codebase pattern where navigation callbacks are required parameters. If a future caller omits this, the premium upgrade button silently does nothing instead of producing a compile error.

This also masks a missing screen test: every other affected screen (SearchScreenTest, AddEditSendScreenTest, VaultAddEditScreenTest, VaultItemScreenTest) has a test for the NavigateToPlanModal event, but VaultItemListingScreenTest does not.

Suggested change
onNavigateToPlan: () -> Unit = {},
onNavigateToPlan: () -> Unit,

After removing the default, add a corresponding test in VaultItemListingScreenTest:

@Test
fun `NavigateToPlanModal event should invoke onNavigateToPlan`() {
    mutableEventFlow.tryEmit(VaultItemListingEvent.NavigateToPlanModal)
    assertTrue(onNavigateToPlanCalled)
}

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 14, 2026

Logo
Checkmarx One – Scan Summary & Details70e35b09-ca1c-4727-9c93-5e0b19ce4455

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

@SaintPatrck SaintPatrck force-pushed the premium-upgrade/PM-33518-upgrade-available-flow branch from 5d1322c to 65f09c8 Compare April 14, 2026 03:30
@SaintPatrck SaintPatrck force-pushed the premium-upgrade/PM-33519-rewire-ctas branch from e2c75cb to 6dc994e Compare April 14, 2026 03:30
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 14, 2026

Codecov Report

❌ Patch coverage is 90.90909% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.59%. Comparing base (b4dd755) to head (4d26c19).

Files with missing lines Patch % Lines
.../feature/itemlisting/VaultItemListingNavigation.kt 50.00% 2 Missing ⚠️
...den/ui/platform/feature/search/SearchNavigation.kt 0.00% 1 Missing ⚠️
...ools/feature/send/addedit/AddEditSendNavigation.kt 0.00% 1 Missing ⚠️
...ui/vault/feature/addedit/VaultAddEditNavigation.kt 0.00% 1 Missing ⚠️
...arden/ui/vault/feature/item/VaultItemNavigation.kt 0.00% 1 Missing ⚠️
...ault/feature/itemlisting/VaultItemListingScreen.kt 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@                                 Coverage Diff                                 @@
##           premium-upgrade/PM-33518-upgrade-available-flow    #6796      +/-   ##
===================================================================================
- Coverage                                            85.81%   85.59%   -0.23%     
===================================================================================
  Files                                                  859      830      -29     
  Lines                                                62519    61564     -955     
  Branches                                              8602     8602              
===================================================================================
- Hits                                                 53652    52693     -959     
- Misses                                                5898     5905       +7     
+ Partials                                              2969     2966       -3     
Flag Coverage Δ
app-data 17.34% <0.00%> (+0.32%) ⬆️
app-ui-auth-tools 20.31% <15.58%> (-0.02%) ⬇️
app-ui-platform 15.52% <20.77%> (-0.39%) ⬇️
app-ui-vault 25.91% <54.54%> (-0.46%) ⬇️
authenticator 6.66% <0.00%> (-0.01%) ⬇️
lib-core-network-bridge 4.20% <0.00%> (-0.01%) ⬇️
lib-data-ui 1.00% <0.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ 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.

@SaintPatrck SaintPatrck force-pushed the premium-upgrade/PM-33518-upgrade-available-flow branch from 65f09c8 to 698c16d Compare April 22, 2026 20:55
@SaintPatrck SaintPatrck force-pushed the premium-upgrade/PM-33519-rewire-ctas branch from 6dc994e to 9461811 Compare April 22, 2026 21:09
val baseUrl = environmentRepo.environment.environmentUrlData.baseWebVaultUrlOrDefault
val url = "$baseUrl/#/settings/subscription/premium?callToAction=upgradeToPremium"
sendEvent(SearchEvent.NavigateToUrl(url = url))
if (premiumStateManager.isInAppUpgradeAvailableFlow.value) {
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.

I am not seeing a scenario here we actually observer this flow. Will we need to do that in a future PR?

If not, maybe just make it a function?

@SaintPatrck SaintPatrck force-pushed the premium-upgrade/PM-33519-rewire-ctas branch from 9461811 to 4d26c19 Compare April 23, 2026 00:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review-vnext Request a Claude code review using the vNext workflow app:password-manager Bitwarden Password Manager app context t:feature Change Type - Feature Development

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants