feat(backup): recurring backup health reminders and Backup Settings redesign - #2453
feat(backup): recurring backup health reminders and Backup Settings redesign#2453BullishNode wants to merge 13 commits into
Conversation
|
@thibistaken experimental idea, but thouroughly reviewed by AI |
The engine asked one question in several voices and tracked state it did not need. It now answers "could you get your money back?" with one verb per posture. Delete the balance-tier machinery (BackupBalanceTier, the 1,000,000-sat tier, highestHandledBalanceTier) and the pending-action bookkeeping. The record is now the acknowledgement time plus a one-time milestone flag; keys written by the earlier shape are ignored rather than rejected, so no migration is needed for an unreleased feature. Make the cadence posture-dependent: 90 days for a vault-only wallet, 365 for one that already holds words. Anchor the schedule on the clock of the thing being urged rather than on the most recent backup of any kind — a both-backups wallet with a fresh vault and a two-year-old physical test was never reminded, which was the whole point of the reminder. Fire the 10,000,000-sat notice at most once per wallet, inclusive of the threshold, retired by either dismissing it or acting on it. Merge the both-backups posture into "test your backup" and drop the "review your backups" nag, which named no action. There is deliberately no vault or PIN freshness reminder: the fix for a single server-dependent recovery path is a physical backup, not a PIN rehearsal. The popup now asks one question and offers one action, with a low-emphasis dismissal that snoozes a full cycle and never records a test the app did not observe.
The screen listed every backup action at equal weight, so the one that mattered was the reader's problem to find. It now answers the same question the reminder does, in three parts: the status rows state the facts, one hero names the single most useful action, and the menu holds everything else. The hero is urgent when nothing is backed up (and replaces the Start Backup row), asks a vault-only wallet for a physical backup, asks for a test once the physical one is over a year old, and is absent when the backup is fresh — no empty card, no heading over nothing. It derives its posture from the same domain code the reminder uses, so the two surfaces cannot disagree, but it reads the real tested dates rather than the snooze-aware verdict: the screen keeps saying "last tested 14 months ago" while the popup is quiet. It stays silent until the first load resolves, because every wallet looks unprotected before then. Rename the Recoverbull menu row to "Encrypted vault settings" to match the status row's vocabulary. The key is deleted and re-added rather than renamed: all 26 non-EN values still said "Recoverbull", and falling back to English reads better than a translation of retired vocabulary. Compose the three sections as separate builders so the fork can add its metadata backup status row and menu row without touching hero logic.
The Backup Settings menu row now reads "Encrypted vault settings", but the page it opened was still titled "Recoverbull Settings". Bring the six strings that use Recoverbull as a feature name in line with the vocabulary the user already sees. Each key is deleted and re-added English-only. The identifiers keep their recoverbull* names — renaming them is churn with no user benefit — but the 26 non-EN entries are dropped so those locales fall back to English rather than rendering a translation of the retired wording. Left alone deliberately: the three "Visit recoverbull.com" strings and the four that name the RecoverBull server. That is the server's real name, and a superuser typing its URL needs to recognise it. recoverbullLearnMore reads "Learn more at recoverbull.com" rather than naming the vault: both of its usages open recoverbull.com, and one of them sits on the server-confirmation page where the subject is the server.
855593d to
b18dae2
Compare
The page asked the user to permit contacting the key server for the vault they had just chosen to create. It gated nothing: allowPermission wrote a local boolean whose only effect was whether the page appeared again. So the vault flows now go straight to their first real step. The navigator builds its page directly — settings to SettingsPage, everything else to ConnectingPage — with no permission fetch and no loading state while it resolves. Tor initialisation and the server check, previously dispatched only once permission had been granted, now always run. Deleting the page frees six strings that described the server rather than asking for anything. recoverbullContinue stays; password_input_page still uses it. The isPermissionGranted column stays. It is part of drift schemas v10 through v14 with a migration step behind it, so dropping it would mean a new schema version rather than abandoning a value; unread and still written as false on insert, it is inert. Users who already granted permission notice nothing: the flag is simply never read again.
Moving the Start Backup CTA into the hero made it disappear exactly when the hero does — so a user whose physical backup was fresh had no way to deliberately add an encrypted vault from this screen. Availability is not encouragement. We still never nag for a second backup once a physical one is tested, but the row is back in the menu, in every state, with its original label, icon and destination. In the zero-backup state it sits below the urgent hero, which offers the same action more prominently; that repetition is cheaper than a rule about when the row is allowed to exist. The label keeps its 26 existing translations rather than coming back English-only.
Removing the permission page took the custom-server warning with it. Put it back where it is actually useful: at the moment the user chooses a server, rather than on a screen they walk past afterwards. Saving a URL that is neither the default nor the server already in use now opens the shared warning sheet, and the URL is stored only if the user confirms. Dismissing stores nothing and drops back to showing the server in use, so nobody can end up pointed at someone else's key server without having read the warning. Returning to the default is the safe direction and passes straight through. Validation still runs first: a malformed URL is rejected before any warning, so the user is never asked to confirm something the app is about to refuse.
With nothing backed up, the hero already renders START BACKUP as its primary action, so the identical menu row a few pixels below it was pure noise. The row is suppressed in that one state and stays in every other one, where the hero offers a different action or none at all.
- start-backup becomes an outlined button between the hero and the settings list instead of a menu row: creating a backup is an action, not a setting, and it should read as one even when a backup exists. It keeps the translated label (27 locales) and is still suppressed in the zero-backup state, where the hero already offers it. - BackupOptionCard takes a list of tags so a card can carry more than one; the encrypted vault card now also shows 'Uses Tor', since reaching the key server over Tor is a property worth knowing before choosing that option.
It is the screen's main action whenever no hero is claiming that role, so it wears the app's primary fill rather than an outline.
settingsRecoverbullTitle, recoverbullRecoveryTitle and recoverbullVaultRecoveryTitle have no reader in lib, test or integration_test. Two of them had also come out of the vocabulary rename holding the same string, which is the duplicate-literal smell the repo warns about — resolved here by removing both rather than picking one.
|
@ethicnology I have removed forcing the user to "accept" the default recoverbull server but I've added a "uses Tor" label on the encrypted backup description card. Also, if user adds custom recoverbull server, there is still a little warning. |
3b54451 to
9393bef
Compare
BackupOptionCard grew from one optional tag to a list so the encrypted vault card could say both how easy it is and that it uses Tor, but nothing verified either the list or the Wrap that carries it. Covers all three cardinalities and both layouts: two tags sit side by side when the card is wide, and the second moves to its own row when it is not. That last case is the whole reason for the Wrap — with a Row it would overflow instead, and the test would fail.
BullishNode
changed the title
some screenshots |
Brings SatoshiPortal#2453 (13 commits) into the Get Paid stack. The screen is a merge, not a copy: the fork already wraps a second cubit and renders the inline "Bull backup" metadata card, so the upstream skeleton is rebuilt around both. The reminder engine is new to the fork. It answers one question — if this phone vanished right now, could you get your money back? — and each posture gets exactly one verb: - vault only, no physical: every 90 days, "add a physical backup" - physical done (± vault): every 365 days since the last test - a one-time popup the first time the device-key balance is observed at or above 10,000,000 sats - zero backup makes NO decision here; that state stays owned by the existing every-launch backup warning overlay Each cadence is anchored on the clock of the thing being urged, so a fresh vault write cannot silence a stale physical backup. Dismissing snoozes the popup for a full cycle and never touches last_tested, so the screen keeps telling the truth while the popup is quiet. There is deliberately no PIN or vault-freshness reminder and no both-verified "review" nag: neither has an action behind it. The screen becomes status rows -> posture hero -> Start Backup button -> menu rows, each section built by its own function so the fork's metadata insertions land without disturbing the others. Start Backup is a button rather than a settings row, and it is suppressed only in the zero-backup state where the hero already offers it. Recoverbull: the vault permission gate is gone (nothing else in the fork consumed FetchPermission/AllowPermission), a custom key server URL now warns before it is saved, and user-facing "Recoverbull" vocabulary is retired in favour of "encrypted vault" — while keeping the recoverbull.com domain strings and the four server-product strings. Localization follows the fork rule: retired wording is deleted from every locale and re-added EN-only so stale translations cannot resurface; unchanged wording keeps its translations. 9 keys deleted, 4 reworded, 21 added.
|
@claude please add your review |
|
Claude finished @i5hi's task in 5m 53s —— View job Review: recurring backup health reminders + Backup Settings redesign
Overall: this is a well-built PR and I'd be comfortable merging it after considering the findings below (none are blockers, though #1 and #4 are worth fixing before merge). The domain layer is genuinely nice: injected clocks everywhere, One change deserves explicit praise: moving the encrypted-backup timestamp out of Findings1. context.read<RecoverBullBloc>().add(const OnTorInitialization());
context.read<RecoverBullBloc>().add(const OnServerCheck());Every rebuild of this widget (theme/locale change, MediaQuery change, parent rebuild) re-fires Tor init and the server check. The old 2.
3. Vault creation reports failure after the backup has actually succeeded — In 4. Verify-mnemonic screen dead-ends after a persistence failure — Verification auto-fires when the last word is placed. If 5. Custom-server warning comparison is string-fragile —
Nits / notes
Design decisions I checked and agree with
|
Second review pass (Claude, multi-angle deep run — posted by @i5hi)This complements the automated review above rather than repeating it: everything below is new. Agreements with the earlier review are listed at the end. Same overall verdict — well-built PR, strong domain tests, honest PR description (every claim in it verified against the code) — but the popup lifecycle around the evaluate/acknowledge loop has real gaps. Findings (most severe first)1. "Not now" cannot silence a future-dated anchor — the blocking popup refires after every wallet sync. 2. The 10M-sats milestone popup flaps around the threshold. 3. Vault-only users get two contradictory blocking popups in one launch. 4. Upgrade path: a custom key server saved before this PR never re-confirms. 5. 6. Usecase calls usecase in onboarding. 7. The due-ness rule executes inside a widget 8. The blocking-overlay shell is now copy-pasted three times (nit). Agreements with the earlier reviewThe events-dispatched-in- Verified clean (so nobody re-checks)Schedule tests would genuinely fail on the pre-fix anchor code (checked against the PR's first commit); no drift schema change needed and none made; all 27 locale files parse, zero references to the 9 deleted keys anywhere, all 26 new keys have descriptions; |
Brings SatoshiPortal#2453 (13 commits) into the Get Paid stack. The screen is a merge, not a copy: the fork already wraps a second cubit and renders the inline "Bull backup" metadata card, so the upstream skeleton is rebuilt around both. The reminder engine is new to the fork. It answers one question — if this phone vanished right now, could you get your money back? — and each posture gets exactly one verb: - vault only, no physical: every 90 days, "add a physical backup" - physical done (± vault): every 365 days since the last test - a one-time popup the first time the device-key balance is observed at or above 10,000,000 sats - zero backup makes NO decision here; that state stays owned by the existing every-launch backup warning overlay Each cadence is anchored on the clock of the thing being urged, so a fresh vault write cannot silence a stale physical backup. Dismissing snoozes the popup for a full cycle and never touches last_tested, so the screen keeps telling the truth while the popup is quiet. There is deliberately no PIN or vault-freshness reminder and no both-verified "review" nag: neither has an action behind it. The screen becomes status rows -> posture hero -> Start Backup button -> menu rows, each section built by its own function so the fork's metadata insertions land without disturbing the others. Start Backup is a button rather than a settings row, and it is suppressed only in the zero-backup state where the hero already offers it. Recoverbull: the vault permission gate is gone (nothing else in the fork consumed FetchPermission/AllowPermission), a custom key server URL now warns before it is saved, and user-facing "Recoverbull" vocabulary is retired in favour of "encrypted vault" — while keeping the recoverbull.com domain strings and the four server-product strings. Localization follows the fork rule: retired wording is deleted from every locale and re-added EN-only so stale translations cannot resurface; unchanged wording keeps its translations. 9 keys deleted, 4 reworded, 21 added.
3 participants
Summary
Recurring backup-health reminders, plus the Backup Settings screen redesign they belong to. Rebased onto current
develop.The whole feature answers one question — "if this phone vanished right now, could you get your money back?" — and gives each state exactly one verb: nothing backed up ⇒ back up; encrypted vault only ⇒ add a physical backup; physical backup done ⇒ occasionally confirm it still works.
Reminder engine
State per wallet is three fields:
last_tested,last_acknowledged,crossed_ten_million_sats.last_testedresets only on completing a verification; creating a backup counts as day-zero tested (creation requires a test).Simplified relative to the first version of this PR
Deleted: balance tiers and their persisted state, tier-driven escalation, the pending-action bookkeeping, and the both-verified "review your backups" nag (it had no real action). There is deliberately no PIN / vault-freshness reminder: a vault-only user's real exposure is having a single recovery path that depends on someone else, and the fix for that is a physical backup — not testing a PIN.
Behaviour fix
The schedule anchor was
latest(vault, physical), so a user with a fresh vault and a two-year-old physical test was never reminded. The anchor now follows the thing being urged. Regression-tested.Backup Settings screen
Encrypted vault flow
recoverbull.com(a real domain) and "RecoverBull Server" (the server's actual product name, which a superuser configuring a URL needs) are kept.Validation
flutter analyze --fatal-warnings --fatal-infos: no issuesdart format --set-exit-if-changed: 0 changeddart fix --dry-run: nothing to fixbull_ui17 andbull_ui_catalogue1Notes for the reviewer
FEATURES.mdwhich the second removes; the final tree is clean, but squashing keeps that out of upstream history entirely.isPermissionGrantedis now dead weight: aNOT NULLcolumn baked into drift schemas v10–v14, written but never read. Dropping it needs a schema bump and migration on live data, so it is deliberately left for a future schema change rather than done here.