Skip to content

fix: preserve LnurlPay comment when navigating back from AmountKeypad#3965

Open
myxmaster wants to merge 1 commit intoZeusLN:masterfrom
myxmaster:fix/lnurlpay-preserve-comment-on-back-navigation
Open

fix: preserve LnurlPay comment when navigating back from AmountKeypad#3965
myxmaster wants to merge 1 commit intoZeusLN:masterfrom
myxmaster:fix/lnurlpay-preserve-comment-on-back-navigation

Conversation

@myxmaster
Copy link
Copy Markdown
Collaborator

Description

Found a minor issue, where the comment field in the Lightning Address payment screen (LnurlPay) was cleared when you tapped the amount field and navigated back.

Root cause: The amount field opens the dedicated AmountKeypad screen. When you return from it, React Navigation fires a focus event on LnurlPay. The registered focus listener called resetState(), which did a full state rebuild via stateFromProps(), including hardcoding comment: ''. Since comment is not part of the route params, it was lost.

Fix: the focus listener now only recalculates the displayed amount in the current unit (the one thing that can legitimately change when coming back from the keypad). All other state, including comment, is left untouched. Also renamed resetState -> recalculateDisplayAmount to reflect the narrower scope.

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

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the LnurlPay component by replacing the resetState method with recalculateDisplayAmount to handle unit changes more efficiently when navigating back to the screen. The logic was simplified to only update the amount state when a non-zero satAmount exists. A review comment suggests adding a conditional check before calling setState to avoid redundant re-renders if the displayed amount remains unchanged.

});
} else {
this.setState(fresh);
this.setState({ amount: displayAmount });
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To avoid unnecessary re-renders when navigating back to this screen, consider checking if the newly calculated displayAmount is different from the current amount in the state before calling setState. This is especially useful if the user didn't change the unit or amount in the keypad, as the focus event will still trigger this recalculation.

if (displayAmount !== this.state.amount) {
    this.setState({ amount: displayAmount });
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant