[PM-39767] feat: Persist Premium upgrade pending state and resolve it on any sync - #3000
Open
KatherineInCode wants to merge 15 commits into
Open
[PM-39767] feat: Persist Premium upgrade pending state and resolve it on any sync#3000KatherineInCode wants to merge 15 commits into
KatherineInCode wants to merge 15 commits into
Conversation
github-actions
Bot
added
app:password-manager
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3000 +/- ##
==========================================
+ Coverage 79.54% 81.91% +2.37%
==========================================
Files 1169 1047 -122
Lines 75095 68131 -6964
==========================================
- Hits 59731 55812 -3919
+ Misses 15364 12319 -3045 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
KatherineInCode
marked this pull request as ready for review
August 28, 2026 18:35
KatherineInCode
requested review from
a team and
matt-livefront
as code owners
August 28, 2026 18:35
… on any sync Premium upgrade status was only tracked by an ephemeral, per-checkout-attempt Combine subscription, and any sync failure was silently swallowed and indistinguishable from "still waiting, no error." This meant a later, unrelated sync (e.g. Settings > Vault > Sync Now, or a sync triggered from the web vault) could never resolve a pending upgrade, and there was no way to know a sync attempt had actually failed versus just not finished yet — QA finding #3 ("Delayed Sync") on this ticket. BillingService now persists pending/failure state per-account via BillingStateService, and watches sync completions generically (not just the originating checkout attempt) so any successful sync can resolve a pending upgrade. Both the checkout-success path and the background sync watcher share a single resolvePendingUpgrade(userId:syncFailed:) — every read and write is scoped to an explicit userId (never "whichever account is active now"), so an account switch mid-flight can't corrupt another account's state, and the two callers double-processing the same sync is redundant but never incorrect. This is the foundation PR in a stack; the celebration sheet and "Sync Unsuccessful" dialog (QA findings #1 and #2) are deferred to later PRs, per the original scoping — #2's dialog design is still an open question raised separately on this ticket.
…dy pending premiumStatusChanged() left .pending sitting in the shared checkout-status subject indefinitely when a sync didn't confirm Premium (the ordinary "still processing" outcome, not a failure) — since it's a CurrentValueSubject, any later, unrelated subscriber (opening the upgrade screen for any reason) would immediately replay that stale value and get bounced into the pending dialog. Now resets to nil unconditionally, matching how .confirmed already did. Separately, none of the nine entry points into the upgrade flow (Settings > Plan, Send, item views, etc.) checked for an already-pending upgrade before starting a new one — only the Vault tab's action card did (PR #2936- equivalent). PremiumUpgradeHelper.startInAppPremiumUpgrade(), the single choke point all of them funnel through, now checks premiumUpgradePendingState() first and shows the pending alert directly instead of opening a second, redundant checkout.
Code review (Standard) caught the same sticky-.pending leak fixed in premiumStatusChanged() by an earlier commit, still present in its sibling reconcileCheckoutSuccess() — the more commonly-exercised path now, since PremiumUpgradeProcessor's checkout callback and the "Sync Now"/"Try again" retries all go through it. Left unresolved, a stuck .pending could either double-show the pending alert on the next startInAppPremiumUpgrade() call, or (across accounts, since the subject is app-global) dismiss a different account's freshly-opened upgrade screen with a pending alert for a checkout it never started. Also closes a narrower window in startInAppPremiumUpgrade(): navigatedToUpgradeScreen is now reset before subscribeToPremiumCheckoutStatus() attaches the new live subscription, not after the pending-state check resolves, so a status arriving in that gap can't be judged against the previous call's leftover value.
Code review caught two issues in the watcher/reconcile split added earlier in this branch: resolvePendingUpgrade(userId:syncFailed:) returned a hardcoded false on its early-exit branches (storage read failure, nothing pending), which reconcileCheckoutSuccess() read as an authoritative Premium answer. On the common happy path, the background watcher resolves the same sync first (SyncService persists the last-sync time partway through fetchSync, well before it returns), clears the pending flags, and leaves reconcileCheckoutSuccess()'s own call hitting the early exit — reporting .pending on a confirmed upgrade. Both early-exit branches now return the account's actual Premium status instead. Separately, premiumCheckoutStatusSubject was a CurrentValueSubject requiring a manual send(nil) reset on every exit path so it wouldn't replay a stale status to the next subscriber — a pattern that had already needed fixing once in premiumStatusChanged() and was still missing on reconcileCheckoutSuccess()'s account-switch guard. Switched it to a PassthroughSubject, which never retains a value to replay, and removed the now-unneeded resets and the compactMap(\.self) sentinel filter.
Local review found one more gap in the same class of bug already fixed twice in this branch: the .pending dismiss branch in subscribeToPremiumCheckoutStatus() never reset navigatedToUpgradeScreen after consuming it, so a second .pending (e.g. a "Sync Now" retry that also doesn't confirm) would dismiss a screen that was already closed — both coordinators resolve .dismiss as "dismiss whatever's presented," so this could take out something the user opened in the meantime. Reset the flag right before issuing the dismiss, mirroring the existing reset at the top of startInAppPremiumUpgrade(). Added a regression test. Also removed two tests (premiumStatusChanged_pending_resetsPublisherValue, reconcileCheckoutSuccess_pending_resetsPublisherValue) that asserted a late subscriber receives no replayed value — guaranteed by PassthroughSubject itself since e7bfdca, not by anything this code does, so they could never fail and only described the removed CurrentValueSubject/send(nil) design. Added a real regression test in their place for the bug e7bfdca actually fixed: reconcileCheckoutSuccess() simulating the background watcher resolving the pending flags first, confirming its own resolvePendingUpgrade call still reports the account's true Premium status instead of the old hardcoded false.
KatherineInCode
force-pushed
the
pm-39767/reconcile-pending-upgrade
branch
from
August 28, 2026 18:36
2b6b326 to
14f148a
Compare
Contributor
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Re-reviewed at Code Review DetailsStill open from a previous review:
Checked and deliberately not raised:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Give BillingServiceTests its own MockBillingStateService for the billingStateService: dependency instead of reusing MockStateService for both parameters. MockStateService's BillingStateService-inherited methods are now conformance-only stubs; a small PremiumUpgradeStateStore plus a nil-resolves-to-active-account helper back the generated mock's per-user-id state, since it can't otherwise represent two accounts at once.
StateService inherited BillingStateService only because VaultListProcessor reached billing-state methods through the general HasStateService dependency, with no narrower seam available. Add HasBillingStateService (mirroring the existing HasBillingRepository/ HasBillingService pattern) and point VaultListProcessor at it directly, so StateService no longer needs to carry BillingStateService's requirements at all. This lets MockStateService drop every BillingStateService method and backing property outright, rather than keeping conformance-only stubs around. BillingRepositoryTests and the VaultListProcessor test files move to MockBillingStateService for the billing-state pieces they need.
| await services.billingService.shouldShowUpgradedToPremiumActionCard() | ||
|
|
||
| let isBannerDismissed = await services.stateService.isPremiumUpgradeBannerDismissed() | ||
| let isBannerDismissed = await services.billingStateService.isPremiumUpgradeBannerDismissed() |
Contributor
There was a problem hiding this comment.
♻️ DEBT: The premium-upgrade-banner flag is now read through billingStateService but still written through stateService.
Details
This line moves the read to services.billingStateService.isPremiumUpgradeBannerDismissed(), but the write for the same persisted key stays on the other dependency at VaultListProcessor.swift:363:
try await services.stateService.setPremiumUpgradeBannerDismissed(true)(VaultGroupProcessor.swift:238 does the same.) isPremiumUpgradeBannerDismissed() lives on BillingStateService while setPremiumUpgradeBannerDismissed(_:userId:) is still declared on StateService (StateService.swift:587), so after this commit the same processor reaches the same premiumUpgradeBannerDismissed_<userId> key through two different injected protocols.
Production is unaffected — ServiceContainer supplies the same DefaultStateService for both. The cost is in tests: perform_dismissPremiumUpgradeActionCard asserts on stateService.premiumUpgradeBannerDismissedByUserId while the visibility tests configure billingStateService.isPremiumUpgradeBannerDismissedReturnValue, so a dismiss and a subsequent read can no longer contradict each other in a test the way they would in the app.
Moving setPremiumUpgradeBannerDismissed(_:userId:) onto BillingStateService alongside its getter would keep the pair together and finish the decoupling this commit started.
BillingServiceTests.swift had grown to 1023 lines with both type_body_length and file_length disabled outright. Split it into: - BillingServiceTests.swift: checkout, plan/subscription lookups, self-hosted detection, and premiumStatusChanged — none of which touch billingStateService. - BillingService+BillingStateTests.swift: action cards, the subscription attention card, and premium-upgrade-pending reconciliation — everything that does, taking the PremiumUpgradeStateStore/resolvedUserId mock-wiring helpers with it since they're now only used here. Follows the existing TypeName+ConcernTests.swift convention (separate type, duplicated setup) already used for VaultListProcessor+BillingTests.swift.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎟️ Tracking
PM-39767
📔 Objective
Replaces #2916. Persists Premium upgrade pending/failure state per account, and resolves it on any sync — not just the one that started the checkout. Fixes QA finding #3 ("Delayed Sync").
Checkout-success and the background sync watcher now share
resolvePendingUpgrade(userId:syncFailed:), scoped to an explicituserId. No lock needed (unlike #2916): an account switch mid-flight can't corrupt another account's state, and double-processing the same sync is harmless.Also fixes a gap left by #2873's CTA fix: none of the other eight upgrade entry points checked for an already-pending upgrade before starting a second one.
PremiumUpgradeHelper.startInAppPremiumUpgrade()— the shared choke point all nine go through — now checks once for everyone.Trade-offs:
lastAttemptFailedcan briefly be wrong after a specific compound sync failure. Left alone — nothing reads it yet.Celebration sheet and "Sync Unsuccessful" alert are later PRs in this stack; the latter's design is still open.