[PM-39767] feat: Persist Premium upgrade pending state and resolve it on any sync - #3000
[PM-39767] feat: Persist Premium upgrade pending state and resolve it on any sync#3000KatherineInCode wants to merge 7 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.85% +2.31%
==========================================
Files 1169 1047 -122
Lines 75095 68047 -7048
==========================================
- Hits 59731 55699 -4032
+ Misses 15364 12348 -3016 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
… 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.
2b6b326 to
14f148a
Compare
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the new per-account Premium upgrade pending state ( Code Review Details
Notes on things checked and deliberately not raised: the persistent-pending "no escape hatch" behavior and the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| func premiumUpgradePendingStatePublisher() -> AnyPublisher<PremiumUpgradePendingState, Never> { | ||
| // `premiumUpgradePendingStateSubject.send(_:)` is called from whichever background | ||
| // `Task` (`start()`'s account/sync subscribers) happens to be resolving it — never | ||
| // guaranteed to be the main thread. Pinned here so a consumer driving `@Published` UI | ||
| // state from this publisher doesn't need to hop to main itself. | ||
| premiumUpgradePendingStateSubject | ||
| .receive(on: DispatchQueue.main) | ||
| .eraseToAnyPublisher() | ||
| } |
There was a problem hiding this comment.
♻️ DEBT: premiumUpgradePendingStatePublisher() has no consumer and no test coverage.
Details
A repo-wide search finds premiumUpgradePendingStatePublisher only in this file (protocol declaration + implementation) — no production caller and no test. The supporting machinery is dead too: premiumUpgradePendingStateSubject and the four refreshPremiumUpgradePendingStateSubject() call sites in reconcileCheckoutSuccess(), start(), and reconcileOnEachNewSync(userId:) currently feed a subject nobody observes, and reconcileOnEachNewSync pays two storage reads per sync to do it.
Since it's untested, a mistake in it (e.g. the main-thread hop, or the fact that the refresh reads the active account while the watcher is scoped to an explicit userId) won't surface until the consuming PR lands. Consider landing this publisher with the celebration-sheet PR that consumes it, or adding coverage for it here.
1 participant
🎟️ 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.