backend: eliminate banner reminder false positives#3747
backend: eliminate banner reminder false positives#3747bznein merged 2 commits intoBitBoxSwiss:masterfrom
Conversation
bznein
force-pushed
the
banner-codex
branch
7 times, most recently
from
4b1588a to
9b75618
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Beerosagos
left a comment
Beerosagos
left a comment
There was a problem hiding this comment.
tACK with a few nits! 🙏
I left a few suggestions to improve readability and I think adding 1-2 comments in the right places could help too. Feel free to ignore the suggestions tho
There was a problem hiding this comment.
Not related to this PR, but while you're here: can you change to smth like: "Could not retrieve fiat balance for keystore"? 🙏
| } | ||
| } | ||
| if keystoreConfig, err := handlers.backend.Config().AccountsConfig().LookupKeystore(rootFingerprint); err == nil { | ||
| existingAllowed = keystoreConfig.BackupReminderAllowed |
There was a problem hiding this comment.
existingAllowed -> reminderConfig
| } | ||
|
|
||
| show := overThreshold | ||
| desiredAllowed := existingAllowed |
There was a problem hiding this comment.
desiredAllowed -> reminderAllowed
|
|
||
| show := overThreshold | ||
| desiredAllowed := existingAllowed | ||
| shouldUpdateAllowed := false |
There was a problem hiding this comment.
shouldUpdateAllowed -> shouldUpdateConfig
| show = false | ||
| } | ||
|
|
||
| if shouldUpdateAllowed && desiredAllowed != nil { |
There was a problem hiding this comment.
nit: you could move this section inside the existingAllowed == nil block and avoid shouldUpdateAllowed logic at all.
Instead of always showing the banner if the balance is above the threshold, we do not show it for accounts that were already over it the first time we check.
Remove the old banner test and replace with a more comprehensive approach that covers more cases.
2 participants
Instead of always showing the banner if the balance is above the threshold, we do not show it for accounts that were already over it the first time we check.
Before asking for reviews, here is a check list of the most common things you might need to consider: