[PM-23713] premium badge interaction

[kdenney]

🎟️ Tracking

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

📔 Objective

This PR modifies the behavior of the premium badge in the following ways when the feature flag pm-23713-premium-badge-opens-new-premium-upgrade-dialog is enabled:

1. In Web, clicking on the badge now opens the same dialog used in the new navigation upgrade button, allowing the user to upgrade to premium or families immediately. After successfully upgrading, the vault is re-sync'd so that the badge no longer appears and the user can continue with what they were attempting to do.
2. In Browser and Desktop, clicking on the badge now opens a new dialog with a prompt to upgrade and a button that will redirect the user to the cloud web vault's subscription page.

To achieve this, several changes were made in locations where the premium badge was (or should have been) used, including:

1. Ensuring that code locations that included the premium badge but also would have the parent container redirect to premium now instead use the PremiumUpgradePromptService in every case.
2. This meant that I could also remove all usages of messaging service for redirecting to the premium page
3. This also includes using the prompt service instead of the messaging service in the has-premium.guard

The new dialog PremiumUpgradeDialogComponent was created in libs/angular so that it could be used in both Browser and Desktop. This required moving the SubscriptionPricingService to libs/common and creating an abstraction for it and moving to libs required removing the ToastService usage from the pricing service and moving that logic to the components that use it instead.

The new dialog uses the pricing card component from libs/pricing but needed to customize some minor styles about how the card component renders. Two new customization properties were added to the card component to achieve this: disableVerticalPadding and disableCardBorder.

A few other changes were made that require explanations and comments have been added to the files in this PR for those.

📸 Screenshots

Desktop dialog light mode:

Desktop dialog dark mode:

Browser dialog light mode:

Browser dialog dark mode:

Web vault reports page (did not use premium badge before, does now):

Video of the dialog being used in desktop:

desktop-video.mov

Video of the dialog being used in browser:

browser-video.mov

Video of the premium badge interaction in web vault:

web-video.mov

Video of the premium badge going away after upgrading in the web vault:

web-sync-after-upgrade.mov

⏰ Reminders before review

• Contributor guidelines followed
• All formatters and local linters executed and passed
• Written new unit and / or integration tests where applicable
• Protected functional changes with optionality (feature flags)
• Used internationalization (i18n) for all UI strings
• CI builds passed
• Communicated to DevOps any deployment requirements
• Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 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

[claude]

Claude finished @kdenney's task —— View job

---

Code Review Complete ✅

Comprehensive review of PR #16911: Premium badge interaction changes

---

Summary of Changes Since Last Review

This PR is a new feature implementing improved premium badge interactions across Web, Browser, and Desktop applications. The changes introduce a new premium upgrade dialog system with post-upgrade synchronization and refactor existing premium upgrade flows.

Key Changes:

• Created PremiumUpgradeDialogComponent in libs/angular for Browser/Desktop
• Moved SubscriptionPricingService to libs/common with proper abstraction
• Unified premium badge behavior using PremiumUpgradePromptService
• Added post-upgrade vault sync in Web to remove premium badges after successful upgrade
• Enhanced PricingCardComponent with customization options

---

Critical Issues ⚠️

1. Non-null assertion operator usage without safety checks

Location: libs/common/src/billing/services/subscription-pricing.service.ts:94

const familiesPlan = plans.data.find((plan) => plan.type === PlanType.FamiliesAnnually)!;

Issue: Using ! assumes the plan will always exist. If the API doesn't return this plan, the code will crash.

Impact: Runtime errors if API response structure changes or plan is unavailable.

Recommendation: Add explicit null checks:

const familiesPlan = plans.data.find((plan) => plan.type === PlanType.FamiliesAnnually);
if (!familiesPlan) {
  throw new Error("Families plan not found in API response");
}

Similar issues at: lines 94, 120, 148, 185 in subscription-pricing.service.ts

2. Potential race condition in premium check

Location: libs/angular/src/billing/directives/not-premium.directive.ts:24-42

async ngOnInit(): Promise<void> {
  const account = await firstValueFrom(this.accountService.activeAccount$);

  if (!account) {
    this.viewContainer.createEmbeddedView(this.templateRef);
    return;
  }

  this.billingAccountProfileStateService
    .hasPremiumFromAnySource$(account.id)
    .pipe(takeUntilDestroyed(this.destroyRef))
    .subscribe((premium) => {
      if (premium) {
        this.viewContainer.clear();
      } else {
        this.viewContainer.createEmbeddedView(this.templateRef);
      }
    });
}

Issue: The directive first creates the view if no account exists, then subscribes to premium status. If the account becomes available between these operations, the view might be created when it shouldn't be. Additionally, calling createEmbeddedView multiple times without checking if a view already exists could create duplicate views.

Recommendation:

async ngOnInit(): Promise<void> {
  const account = await firstValueFrom(this.accountService.activeAccount$);

  if (!account) {
    this.viewContainer.createEmbeddedView(this.templateRef);
    return;
  }

  this.billingAccountProfileStateService
    .hasPremiumFromAnySource$(account.id)
    .pipe(takeUntilDestroyed(this.destroyRef))
    .subscribe((premium) => {
      this.viewContainer.clear();
      if (!premium) {
        this.viewContainer.createEmbeddedView(this.templateRef);
      }
    });
}

3. Guard returns Observable but doesn't prevent navigation properly

Location: apps/web/src/app/billing/guards/has-premium.guard.ts:45-49

tap((userHasPremium: boolean) => {
  if (!userHasPremium && router.url === "/login") {
    return router.createUrlTree(["/"]);
  }
}),

Issue: The tap operator doesn't affect the stream value. Creating a UrlTree inside tap has no effect - it should return it from the main observable chain.

Impact: Users without premium on /login aren't redirected properly.

Recommendation:

map((userHasPremium: boolean) => {
  if (!userHasPremium && router.url === "/login") {
    return router.createUrlTree(["/"]);
  }
  return userHasPremium;
})

4. Missing error handling for sync failures

Location: apps/web/src/app/vault/services/web-premium-upgrade-prompt.service.ts:111-113

await this.apiService.refreshIdentityToken();
await this.syncService.fullSync(true);

Issue: If either operation fails, the user may still see the premium badge even after successful payment. No error handling or user notification.

Recommendation: Wrap in try-catch and show appropriate user feedback on failure.

5. Type safety issue in map function

Location: libs/angular/src/billing/components/premium-upgrade-dialog/premium-upgrade-dialog.component.ts:56

map((tier) => this.mapPremiumTierToCardDetails(tier!)),

Issue: Using non-null assertion tier! assumes find() always returns a value. If Premium tier is not in the array, this will crash.

Recommendation:

map((tier) => {
  if (!tier) {
    throw new Error("Premium tier not found");
  }
  return this.mapPremiumTierToCardDetails(tier);
}),

---

Significant Concerns ❌

6. Inconsistent async handling in premium prompt services

The three platform implementations handle the async promptForPremium differently:

• Web: Uses lastValueFrom for dialog result
• Browser/Desktop: Fire-and-forget dialog opening

This could lead to inconsistent behavior across platforms.

7. Missing validation in PricingCard component

Location: libs/pricing/src/components/pricing-card/pricing-card.component.ts

The component accepts disableVerticalPadding and disableCardBorder inputs but doesn't validate their usage. These are styling properties that could conflict with certain layouts.

8. Hardcoded pricing in SubscriptionPricingService

Location: libs/common/src/billing/services/subscription-pricing.service.ts:67-70

private premium$: Observable<PersonalSubscriptionPricingTier> = of({
  // premium plan is not configured server-side so for now, hardcode it
  basePrice: 10,
  additionalStoragePricePerGb: 4,
})

Issue: Hardcoded pricing values that should come from a configuration or API response. The comment acknowledges this is temporary, but there's no tracking issue or TODO.

Recommendation: Add a TODO with tracking issue number, or fetch from configuration service.

9. Missing i18n for error messages

Several error messages are hardcoded in English:

• subscription-pricing.service.ts: "Failed to load personal subscription pricing tiers"
• upgrade-payment.service.ts: Various validation error messages

These should use i18nService.t() for proper internationalization.

---

Suggested Improvements 🎨

10. Improve error handling in UpgradePaymentComponent

Location: apps/web/src/app/billing/individual/upgrade/upgrade-payment/upgrade-payment.component.ts:212-218

Current error handling is too generic. Consider:

• Distinguishing between payment failures, network errors, and validation errors
• Providing more specific user feedback
• Retry logic for transient failures

11. Extract magic numbers to constants

Location: Multiple files

// In upgrade-account and premium-upgrade-dialog
amount: tier.passwordManager.annualPrice / 12,

The 12 (months in a year) should be a named constant for clarity.

12. Consolidate pricing calculation logic

The monthly price calculation (annualPrice / 12) appears in multiple places:

• premium-upgrade-dialog.component.ts:98
• upgrade-account.component.ts:128

Consider adding a helper method to the pricing tier type or creating a utility function.

13. Improve type safety for PersonalSubscriptionPricingTierId

The type allows any string that's a key of the const object, but runtime checks just compare strings. Consider using branded types or a type guard function.

14. Add JSDoc comments to public methods

Several new public methods lack documentation:

• PremiumUpgradeDialogComponent.upgrade()
• DefaultSubscriptionPricingService helper methods
• UpgradePaymentService public methods

15. Test coverage gaps

Based on Codecov report (47.67% patch coverage), missing coverage for:

• premium-upgrade-dialog.component.stories.ts (19 lines)
• upgrade-payment.component.ts (15 lines)
• report-card.stories.ts (8 lines)
• Error paths in new services

Recommendation for improving test coverage

Focus on:

1. Error scenarios in SubscriptionPricingService
2. Edge cases in WebVaultPremiumUpgradePromptService (already has good coverage)
3. Payment validation failures in UpgradePaymentComponent
4. Premium status checks in various components

16. Consider caching strategy for pricing data

Location: libs/common/src/billing/services/subscription-pricing.service.ts:63-65

The service uses shareReplay({ bufferSize: 1, refCount: false }) which keeps the subscription alive forever. Consider if this is the intended behavior or if it should clean up after all subscribers disconnect.

17. Messaging service cleanup incomplete

While the PR removes messaging service usage for premium redirects, the MessagingService is still imported and injected in DesktopPremiumUpgradePromptService. This should be removed if no longer needed.

18. Consider extracting feature flag checks

The feature flag check FeatureFlag.PM23713_PremiumBadgeOpensNewPremiumUpgradeDialog is duplicated across three services. Consider:

• Creating a helper method
• Using a decorator
• Centralizing the logic

---

Security Considerations 🔒

19. Payment data handling looks secure ✅

• Payment method tokenization is handled correctly
• Sensitive data not logged
• Proper use of billing abstractions

20. No obvious XSS vulnerabilities ✅

• User input properly bound through Angular templates
• I18n usage is correct

21. API calls properly authenticated ✅

• Using existing ApiService and billing clients
• Token refresh handled appropriately

---

Performance Considerations ⚡

22. Observable subscription management

Good use of takeUntilDestroyed() throughout for cleanup.

23. API call optimization

The shareReplay operator in SubscriptionPricingService prevents duplicate API calls - good optimization.

24. Consider debouncing tax calculation

Location: apps/web/src/app/billing/individual/upgrade/upgrade-payment/upgrade-payment.component.ts:161-170

Already using debounceTime(1000) - good implementation.

---

Adherence to Repository Guidelines ✅

25. Angular patterns

• ✅ Uses Observable Data Services pattern correctly
• ✅ Proper use of takeUntilDestroyed() instead of manual subscriptions
• ✅ No new TypeScript enums (uses const objects as required)
• ✅ Component uses signals appropriately in UpgradeAccountComponent and UpgradePaymentComponent

26. Code organization

• ✅ Proper separation between libs and apps
• ✅ Abstractions created for cross-platform services
• ✅ Feature flag protection implemented

27. I18n usage

• ✅ All UI strings use i18nService.t()
• ⚠️ Some error messages missing i18n (see issue Imports #9)

28. Testing

• ✅ Good test coverage for service logic
• ⚠️ Missing coverage for some components and error paths

---

Good Practices Observed 👍

• Feature flag implementation for gradual rollout
• Proper abstraction of SubscriptionPricingService
• Post-upgrade sync to update UI state
• Comprehensive unit tests for core services
• Proper error handling in most places
• Use of RxJS patterns (catchError, shareReplay, etc.)
• Component reusability (PricingCardComponent enhancements)
• Clear separation of concerns (services, components, directives)

---

Action Items for Author

High Priority

1. ❗ Fix non-null assertions in subscription-pricing.service.ts (Issue i18n #1)
2. ❗ Fix guard navigation logic in has-premium.guard.ts (Issue Overlay Popup #3)
3. ❗ Add error handling for sync operations in web-premium-upgrade-prompt.service.ts (Issue New Site Notifications #4)
4. ❗ Add null check for premium tier in premium-upgrade-dialog.component.ts (Issue Generate Password Context Menu Option #5)

Medium Priority

5. Fix potential race condition in not-premium.directive.ts (Issue Site Search #2)
6. Improve error handling in UpgradePaymentComponent (Issue Where is the account being created? #10)
7. Add i18n for error messages (Issue Imports #9)
8. Increase test coverage for critical paths (Issue add LICENSE #15)

Low Priority (Nice to Have)

9. Extract magic numbers to constants (Issue Screenshots #11)
10. Add JSDoc to public methods (Issue Secure Notes #14)
11. Review caching strategy (Issue AES Nonce generation is done with paranoia level 0 #16)
12. Remove unused MessagingService import (Issue Safari Extension #17)
13. Consider extracting feature flag checks (Issue Incorrect displaying IP-addresses #18)

---

Overall Assessment

This is a well-structured PR that makes significant architectural improvements to the premium upgrade flow. The code generally follows Angular best practices and repository guidelines. The main concerns are around error handling, null safety, and test coverage.

The feature flag approach is excellent for gradual rollout. The refactoring to move shared code to libs/ is the right architectural decision.

Recommendation: Request changes to address the critical issues before merging, particularly issues #1, #3, #4, and #5.

---

| Branch: billing/pm-23713/premium-badge-interaction

[github-actions]

Checkmarx One – Scan Summary & Details – e3ab82a2-f79a-41e6-87b7-64f1febe31bc

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

[codecov]

Codecov Report

❌ Patch coverage is 52.79188% with 93 lines in your changes missing coverage. Please review.
✅ Project coverage is 39.22%. Comparing base (d65824e) to head (c3f487d).
⚠️ Report is 3 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...dialog/premium-upgrade-dialog.component.stories.ts	0.00%	19 Missing ⚠️
...grade/upgrade-payment/upgrade-payment.component.ts	11.76%	15 Missing ⚠️
.../reports/shared/report-card/report-card.stories.ts	0.00%	8 Missing ⚠️
...c/new-send-dropdown/new-send-dropdown.component.ts	0.00%	8 Missing ⚠️
...ps/web/src/app/billing/guards/has-premium.guard.ts	0.00%	6 Missing ⚠️
libs/angular/src/tools/send/add-edit.component.ts	0.00%	6 Missing ⚠️
...ar/src/billing/directives/not-premium.directive.ts	28.57%	5 Missing ⚠️
...ult/services/web-premium-upgrade-prompt.service.ts	86.66%	2 Missing and 2 partials ⚠️
...ling/individual/premium/premium-vnext.component.ts	0.00%	3 Missing ⚠️
...grade/upgrade-account/upgrade-account.component.ts	75.00%	3 Missing ⚠️
... and 9 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #16911      +/-   ##
==========================================
+ Coverage   39.20%   39.22%   +0.02%     
==========================================
  Files        3463     3466       +3     
  Lines       98126    98225      +99     
  Branches    14736    14737       +1     
==========================================
+ Hits        38469    38530      +61     
- Misses      57991    58030      +39     
+ Partials     1666     1665       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud