fix(app): surface cloud restore failures to the user and telemetry (SELF-3932) - #2273
fix(app): surface cloud restore failures to the user and telemetry (SELF-3932)#2273seshanthS wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (3)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aaa331e764
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * error with a fresh one, so every classifiable branch must throw outside it — | ||
| * whatever escapes the retry wrapper is unclassifiable by construction. | ||
| */ | ||
| | 'backup_read_failed'; |
There was a problem hiding this comment.
Wrap provider read failures in the declared error type
When Google Drive list/getText or iCloud isCloudAvailable/exists/readFile rejects, such as during a transient network or provider failure, the plain error bypasses every CloudBackupError throw site. The screen consequently records backup_download_failed and displays unexpected_error, while the newly declared backup_read_failed reason is never produced by production code, defeating the branch-specific copy and telemetry for storage failures. Catch these provider errors and rethrow CloudBackupError('backup_read_failed', ...).
Useful? React with 👍 / 👎.
| checkBiometricsAvailable() | ||
| .then(available => { | ||
| if (active) { | ||
| setBiometricsAvailable(available); | ||
| } |
There was a problem hiding this comment.
Preserve availability when the real capability check fails
When biometrics.isSensorAvailable() throws during a focus or foreground refresh, the production checkBiometricsAvailable implementation catches that exception and resolves false, so this .then overwrites a previously valid true value and the .catch intended to preserve it is never reached. A transient native error after returning to the app therefore disables cloud recovery until another successful refresh; make the provider expose an indeterminate/failure result and update the store only for an actual capability result. The added test currently mocks a rejection that the real provider never produces.
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
| trackEvent(BackupEvents.CLOUD_RESTORE_FAILED_UNKNOWN, { | ||
| reason: 'restore_failed', | ||
| }); | ||
| setError('restore_failed'); |
There was a problem hiding this comment.
Report keychain failures instead of blaming the cloud phrase
In the cloud path, download() has already parsed and validated the BIP39 mnemonic before this call, while restoreFromMnemonic catches failures from Keychain.setGenericPassword and returns false. This newly surfaced branch consequently shows copy telling the user to double-check a phrase they did not enter and that has already been validated, while telemetry records only restore_failed; on a keychain write or security-configuration failure the suggested remedy cannot help. Propagate and classify the secure-storage failure so the screen can present an actionable message.
Useful? React with 👍 / 👎.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
d3543b5 to
f762794
Compare
Closes SELF-3932.
Stacked on #2272 (SELF-3931). Base is
fix/self-3931-recovery-protocol-data; retarget todevonce that merges.Problem
AccountRecoveryChoiceScreencaught every cloud restore failure, logged to console, fired an event and cleared the spinner — and rendered nothing. The user saw a spinner, then an idle button, with nothing to report to support. 1787 of 1938Cloud Restore Failed: Unknown Errorevents in the last 30 days carried no properties, so the failing branch was unknowable, and there was no restore-attempted event to serve as a denominator.Two adjacent gating bugs on the same screen:
biometricsAvailablewas a device capability stored in persisted settings, so a stalefalsesurvived across launches and kept the cloud button permanently disabled; and it was only ever written by a fire-and-forget check at splash.Changes
app/src/services/cloud-backup/errors.tswithCloudBackupErrorcarrying areason(sign_in_cancelled,cloud_unavailable,no_backup_found,backup_corrupt,backup_read_failed). Every throw site inindex.ts/ios.tsis classified. Messages are byte-identical to before, so the existing prose assertions act as the regression guard.ios.ts download()callsCloudStorage.isCloudAvailable()beforeexists(). A signed-out device resolvesexistsasfalse, so without this guard it reported "no backup found" instead of "sign in to iCloud".RecoverWithPhraseScreen, rendered beneath the description. Built onCloudBackupErrorReason, so any new download failure is renderable by construction.CLOUD_RESTORE_STARTED, fired imperatively once per attempt; theCLOUD_BACKUP_STARTEDprop is removed from the recover button. Untyped download failures keepreason: 'backup_download_failed'so they stay separable fromunexpected_error.recoveryCopy.errorssuperset; the phrase screen's inlineERROR_MESSAGESmoved there verbatim. The Android↔iOS backup incompatibility is stated inno_backup_found, the one branch a platform-switcher actually hits.biometricsAvailable— moved toNonPersistedSettingsState, excluded frompartialize,SETTING_STORE_VERSIONbumped to 2 with a migration that deletes the stale key.partializealone only stops future writes; the already-persisted value would still rehydrate once and clobber the fresh check.useBiometricsAvailability— re-reads capability on focus and onAppStateactive. Both are needed: the OS-settings round trip never unfocuses the screen, and foreground alone misses in-app navigation onto it.isSensorAvailable()is a capability query, so no biometric prompt is shown.Notes for reviewers
CloudBackupScreenalso readsbiometricsAvailable. It now readsfalseuntil the splash check resolves, where it previously read a sticky persisted value. This widens an existing SplashScreen ordering race rather than creating one; the recovery screen is immunised by the new hook. Extending the hook toCloudBackupScreenis a follow-up.google.tsgoogleSignIn()swallows everyauthorize()failure, so a genuine Android auth or network failure reports assign_in_cancelled.react-native-app-authexposes no cancel sentinel in its JS surface, so splitting them means string-matching native errors. Follow-up, not scope.cloud_unavailableis iOS-only in practice — Google Drive always reports available — so there is no Android repro for testing instruction 2.biometric_sensor_checkedlog volume.CloudStorageError.codeis deliberately not mapped:withRetriesreplaces the original error, so every classifiable branch throws outside it and anything escaping the retry wrapper is unclassifiable by construction.Validation
New coverage: per-reason download classification (incl.
existsnever called when iCloud is unavailable), one test per rendered failure branch with its analyticsreason,CLOUD_RESTORE_STARTEDexactly once andCLOUD_BACKUP_STARTEDnever, error clearing on retry, the v2 migration, and the focus/foreground re-check with listener cleanup.Device QA still owed — these cannot be unit-tested: iOS signed out of iCloud (instruction 2), cancelling the real Google sign-in sheet (instruction 3), and toggling biometrics in OS settings and returning (instruction 5).
🤖 Generated with Claude Code