[PM-39767] fix: Show Sync Unsuccessful alert when a pending upgrade's sync attempt fails - #2955
Draft
KatherineInCode wants to merge 3 commits into
Draft
[PM-39767] fix: Show Sync Unsuccessful alert when a pending upgrade's sync attempt fails#2955KatherineInCode wants to merge 3 commits into
KatherineInCode wants to merge 3 commits into
Conversation
… sync attempt fails A user with a pending Premium upgrade never got any feedback if the sync that would confirm it actually failed outright — only a generic "pending" state, indistinguishable from just still waiting. VaultListProcessor now shows a "Sync unsuccessful" alert the first time a sync attempt tied to the pending upgrade fails, with "Not now" and "Try again" (which retries via the same premiumStatusChanged() call the pending dialog's own "Sync Now" uses). Also fixes a staleness bug found while wiring this up: BillingService's reconciliation logic only cleared the failure flag when the account became Premium, so a later, unrelated sync that succeeded without granting Premium yet would leave the flag stuck at "failed" even though the most recent attempt had actually worked.
…up test reconcilePendingUpgradeIfNeeded() was reachable from the last-sync-time publisher's initial replay on subscribe (a CurrentValueSubject cached value, not evidence of a new sync), which could clear a genuine stale lastAttemptFailed flag before any real sync happened. Now compares each emission against a directly-snapshotted baseline instead of relying on positional/first-emission semantics, which also naturally ignores re-emissions caused by a different account's sync updating the shared last-sync-time store. Also rewrote the VaultListProcessor de-dup test, whose wait condition was trivially satisfied before the emission it claimed to verify was even processed.
…nt retry failure Task.yield() twice didn't reliably settle start()'s subscription before the test's next send() — DefaultBillingService isn't @mainactor, so the yields on the test's actor had no ordering relationship with the service's cooperative-pool tasks. Replaced with a wait on a new getLastSyncTimeCallCount, which fires exactly once at the point start() takes its baseline snapshot. Also fixed streamPremiumUpgradePendingState(): tapping "Try again" on the Sync Unsuccessful alert never reset the transition tracker, so a retry that also failed produced no alert and no other feedback, silently doing nothing from the user's perspective. MockStateService.setLastSyncTime(_:userId:) now also updates lastSyncTimeSubject, matching DefaultStateService's write-side symmetry and closing a latent trap where clearing the dict entry could leave a stale subject value behind.
github-actions
Bot
added
app:password-manager
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## pm-39767/cta-fixes #2955 +/- ##
=====================================================
Coverage ? 81.12%
=====================================================
Files ? 1038
Lines ? 67459
Branches ? 0
=====================================================
Hits ? 54724
Misses ? 12735
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
https://bitwarden.atlassian.net/browse/PM-39767
📔 Objective
Third of four stacked PRs addressing QA follow-up findings on PM-39767 (base:
pm-39767/cta-fixes, PR #2936).When a pending Premium upgrade's sync attempt fails, the user previously got no feedback at all — no error, no retry affordance, nothing distinguishing "still processing" from "actually failed."
Adds a "Sync unsuccessful" system alert (
Alert.syncUnsuccessful), shown via a live subscription toBillingService.premiumUpgradePendingStatePublisher()(added in PR #2916) on a false-to-true transition oflastAttemptFailed, so it surfaces once per failure without repeating for unrelated state changes. "Try again" re-triggerspremiumStatusChanged()to retry the sync; "Not now" just dismisses — no new suppression state, matching the existing persisted-flag model (the alert can reappear the next time the user's on the Vault tab if the failure is still unresolved).Two rounds of local code review found and fixed four real issues along the way:
reconcilePendingUpgradeIfNeeded()could be triggered by the last-sync-time publisher's initial replay-on-subscribe (a cached value, not an actual new sync completing), which could prematurely clear a genuine stale failure flag. Now compares each emission against a directly-snapshotted baseline instead of relying on positional/first-emission semantics.waitForcondition that was trivially satisfied before the emission it claimed to verify was even processed, andTask.yield()-based test synchronization that turned out to be genuinely flaky under load (confirmed via reproduction) — replaced with a deterministic wait on a call count.Each fix was verified by mutation-testing the guard it protects (temporarily breaking the production logic, confirming the test fails, then restoring it), plus repeated full-suite runs to rule out flakiness.
📸 Screenshots
Not applicable — a standard system alert matching existing patterns (e.g.
secureCheckoutDidntLoad).