You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
I noticed this minor issue:
Opening and closing the menu once after app start triggered a full node refetch, with a visible loading indicator, although nothing had been changed. A second open/close was clean. Only reproducible with a lock configured (PIN, password or biometrics).
Tracing it turned up two further problems in the same mechanism, fixed in the follow-up commits.
resetAuthenticationAttempts() was fire-and-forget. updateSettings() persists to storage before it sets triggerSettingsRefresh, so on a successful login the flag landed after proceed() had already returned to the Wallet screen and its handleFocus() had cleared it. The flag then survived until the next focus event (first menu close) and forced the refetch there.
-> Now awaited at all four call sites.
updateSettings() armed triggerSettingsRefresh unconditionally, so writes re-persisting an identical value cost a full refetch too. Two happen on every normal app start: supportedBiometryType and authenticationAttempts: 0. Now guarded by an isEqual() check against the settings read at the start of the call.
Mostly theoretical, because probably not reproducible in practice, but fixed anyway since it is a simple fix: handleFocus() also cleared the flags in the branch where the _navigating guard skipped the call. A settings change arriving while a refresh was in flight was therefore consumed without ever being applied. Clearing now happens only in the branch that actually starts getSettingsAndNavigate().
Testing
Verified on Android with PIN and with biometrics: no refetch on the first menu open/close. Settings changes that do require a reconnect still trigger one.
Please test on iOS.
This pull request is categorized as a:
New feature
Bug fix
Code refactor
Configuration change
Locales update
Quality assurance
Other
Checklist
I’ve run yarn run tsc and made sure my code compiles correctly
I’ve run yarn run lint and made sure my code didn’t contain any problematic patterns
I’ve run yarn run prettier and made sure my code is formatted correctly
I’ve run yarn run test and made sure all of the tests pass
Testing
If you modified or added a utility file, did you add new unit tests?
No, I’m a fool
Yes
N/A
I have tested this PR on the following platforms (please specify OS version and phone model/VM):
Android
iOS
I have tested this PR with the following types of nodes (please specify node version and API version where appropriate):
On-device
LDK Node
Embedded LND
Remote
LND (REST)
LND (Lightning Node Connect)
Core Lightning (CLNRest)
Nostr Wallet Connect
LndHub
Locales
I’ve added new locale text that requires translations
I’m aware that new translations should be made on the ZEUS Transfix page and not directly to this repo
Third Party Dependencies and Packages
Contributors will need to run yarn after this PR is merged in
3rd party dependencies have been modified:
verify that package.json and yarn.lock have been properly updated
verify that dependencies are installed for both iOS and Android platforms
Other:
Changes were made that require an update to the README
Changes were made that require an update to onboarding
Traced all three mechanisms against master and the diagnosis checks out: updateSettings() does persist before arming triggerSettingsRefresh, so the fire-and-forget reset's flag can land after Wallet's handleFocus() already cleared it; the legit post-unlock refetch (proceed() arming the flag before pop()) then absorbs the now-awaited write. All four new await sites are in async functions. Nice writeup, the three-commit split made this easy to verify.
Needs rebase. The branch is conflicting: master's applySettingsUpdate gained a persisted early-return guard (data-wipe write latch) that rewrites the exact lines this PR touches. Mechanical fix: the isEqual guard belongs after if (!persisted) return this.settings;. Semantics compose cleanly since blocked writes already skip arming the flag.
One real gap in the isEqual guard: devices without biometrics.getSupportedBiometryType() returns undefined when no sensor is available, so Wallet's componentDidMount writes { supportedBiometryType: undefined } on every start. JSON.stringify drops the key when persisting, but the merged in-memory object keeps it, and lodash isEqual({ a: 1 }, { a: 1, b: undefined }) is false. So on no-biometry devices this every-start write still arms the flag, which defeats half of commit 2's stated purpose. Today it happens to be absorbed by the initialLoad refetch, but combined with commit 3's leave-armed behavior it can resurface as a deferred refetch. Suggest stripping undefined-valued keys from resolvedSetting before the merge/compare (or comparing against the JSON round-trip of newSettings), which also matches what actually gets persisted.
Non-blocking observations:
Unlocks preceded by a failed attempt still trigger a refetch: the increment and the reset back to 0 are both real changes. Not a regression, but it suggests bookkeeping keys (authenticationAttempts, supportedBiometryType) arguably should never arm the refresh at all; an exclusion list would be the more complete fix. Same one-time arm the first time authenticationAttempts: 0 is written to a blob that never had the key.
posWasEnabled has the same disease: applySettingsUpdate still sets it unconditionally for any pos write with posEnabled !== Disabled, even a no-op re-persist. Follow-up candidate.
Commit 3's trade-off: a settings change racing an in-flight refresh is now deferred to the next focus event, which can be the next menu close, i.e. the same visible symptom in a rarer path. Optional improvement: re-check the flags in the .finally() and re-trigger immediately.
On the tests checkbox: stores/SettingsStore.test.ts already has the exact harness needed (seedSettings/StorageMock/persistedSettings, with existing triggerSettingsRefresh assertions in the blocked-write test). Two cheap cases would lock in commit 2: a no-op write leaves the flag false, a real change sets it. A third could pin down the undefined-key behavior.
Can run it through iOS once the rebase lands.
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
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.
Description
I noticed this minor issue:
Opening and closing the menu once after app start triggered a full node refetch, with a visible loading indicator, although nothing had been changed. A second open/close was clean. Only reproducible with a lock configured (PIN, password or biometrics).
Tracing it turned up two further problems in the same mechanism, fixed in the follow-up commits.
resetAuthenticationAttempts()was fire-and-forget.updateSettings()persists to storage before it setstriggerSettingsRefresh, so on a successful login the flag landed afterproceed()had already returned to the Wallet screen and itshandleFocus()had cleared it. The flag then survived until the next focus event (first menu close) and forced the refetch there.-> Now awaited at all four call sites.
updateSettings()armedtriggerSettingsRefreshunconditionally, so writes re-persisting an identical value cost a full refetch too. Two happen on every normal app start:supportedBiometryTypeandauthenticationAttempts: 0. Now guarded by anisEqual()check against the settings read at the start of the call.Mostly theoretical, because probably not reproducible in practice, but fixed anyway since it is a simple fix:
handleFocus()also cleared the flags in the branch where the_navigatingguard skipped the call. A settings change arriving while a refresh was in flight was therefore consumed without ever being applied. Clearing now happens only in the branch that actually startsgetSettingsAndNavigate().Testing
Verified on Android with PIN and with biometrics: no refetch on the first menu open/close. Settings changes that do require a reconnect still trigger one.
Please test on iOS.
This pull request is categorized as a:
Checklist
yarn run tscand made sure my code compiles correctlyyarn run lintand made sure my code didn’t contain any problematic patternsyarn run prettierand made sure my code is formatted correctlyyarn run testand made sure all of the tests passTesting
If you modified or added a utility file, did you add new unit tests?
I have tested this PR on the following platforms (please specify OS version and phone model/VM):
I have tested this PR with the following types of nodes (please specify node version and API version where appropriate):
On-device
Remote
Locales
Third Party Dependencies and Packages
yarnafter this PR is merged inpackage.jsonandyarn.lockhave been properly updatedOther: