Skip to content

fix: payment QR popup despite attached wallet #2215

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

m0wer
Copy link
Contributor

@m0wer m0wer commented Jun 8, 2025

Description

Fixed payment QR popup appearing even when wallets are properly configured. The issue was caused by stale closures in React hooks - callbacks were capturing outdated wallet state and incorrectly reporting hasSendWallet: false to the server.

Closes #1656

Key fixes:

  • Changed dependency from wallets.length to wallets in onSubmit callback
  • Added proper useCallback wrappers in useAct hook functions
  • Ensures callbacks always reference current wallet state instead of stale values

Screenshots

N/A - bug fix only

Additional Context

The original bug is intermittent and hard to reproduce consistently, so testing was mainly focused on ensuring the changes don't break existing functionality. The stale closure pattern was clear from code review though - callbacks weren't tracking wallet state changes properly.

Checklist

Are your changes backwards compatible? Please answer below:

Yes, no API or interface changes - just internal callback dependency fixes.

On a scale of 1-10 how well and how have you QA'd this change and any features it might affect? Please answer below:

7/10 - Can't reproduce the original intermittent bug to verify it's completely fixed, but confident about the code changes. Tested zapping with QR (still works) and zapping with an attached wallet (LNbits). Happy path and errors (QR when not enpough balance) still work.

For frontend changes: Tested on mobile, light and dark mode? Please answer below:

No changes to layout or theme.

Did you introduce any new environment variables? If so, call them out explicitly here:

No new environment variables.

@m0wer m0wer requested a review from ekzyis June 8, 2025 21:21
@m0wer m0wer self-assigned this Jun 9, 2025
@m0wer m0wer marked this pull request as ready for review June 9, 2025 16:38
@m0wer m0wer force-pushed the payment_qr_popup branch from e365245 to 014ee0a Compare June 9, 2025 16:40
@huumn
Copy link
Member

huumn commented Jun 10, 2025

What's the reasoning behind the new useCallback calls?

The way I view useCallback is that it's strictly a performance enhancement - like useMemo it prevents re-initializing functions/variables during re-renders (except when relevant dependencies change). These changes make it so that rather than re-initializing those functions every render, it only does it sometimes. I guess it's possible that some future re-render has empty wallets or something, but then that's the source of the bug and this is a hack to cover it up.

Further, those useCallback functions relate to clientside cache updates - which happen after a payment is made/attempted and should not affect a QR popping up or not.

afaict the root of this bug is caused either by:

  1. useSendWallets returning an empty array sometimes or
  2. the result of useSendWallets being interpreted as a an empty array somewhere that it shouldn't be

@m0wer
Copy link
Contributor Author

m0wer commented Jun 10, 2025

Yes - the useCallback additions are just performance optimizations.

The actual fix is changing the dependency from wallets.length to wallets in the onSubmit callback, as ekzyis suspected. This ensures the callback updates when wallet contents change, not just when the count changes.

The root issue is likely useSendWallets() sometimes returning stale/empty arrays when wallets should be available but this fix makes it better. I will look into what could trigger the problem with useSendWallets().

@m0wer
Copy link
Contributor Author

m0wer commented Jun 10, 2025

Looks like useSendWallets() involves a bunch of async loading and can be evaluated before the loading and decrypting is done, probably causing the issue. If the solution presented is not enough, we could refactor the useSendWallets() logic and state management to ensure that it's not used until fully ready.

Do you have any other ideas? Do you like the simple fix more now? xD

@ekzyis
Copy link
Member

ekzyis commented Jun 10, 2025

What I don't understand about the fix here, my own comment and fix is that if wallets.length does change, it should update the callback and that's all the callback cares about since we don't actually use the values in wallets, we only check for the length. Right?

So I now think the issue must be somewhere else. 🤔

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.

payment QR popup despite wallets being attached and enabled
3 participants