Repository navigation
fix input amount - #519
fix input amount#519
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe change adds a state synchronization call in the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying wallet-bitcoin with
|
| Latest commit: |
1f87cb0
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://36804634.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://fix-fiat-value-on-send.wallet-bitcoin.pages.dev |
Deploying wallet-mutinynet with
|
| Latest commit: |
1f87cb0
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8d335036.arkade-wallet.pages.dev |
| Branch Preview URL: | https://fix-fiat-value-on-send.arkade-wallet.pages.dev |
ghost
left a comment
There was a problem hiding this comment.
✅ Approved — clean, correct fix
The bug: When scanning a BIP21 QR code with a prefilled amount (non-asset path), sendInfo.satoshis was being set via setState(), but the local amount state (passed as sats to <InputAmount>) was never updated. The InputAmount component needs the sats prop to drive the fiat conversion display — without it, the fiat value shows as empty.
The fix: Adding setAmount(satoshis) before setState(...) at Form.tsx:224 mirrors what every other code path already does:
- Lightning invoice path (line 241): ✅ calls
setAmount(satoshis) handleAmountChange(line 504): ✅ callssetAmount(sats)handleSendAll(line 588): ✅ callssetAmount(liquidBalance)- LNURL limits effect (line 330): ✅ calls
setAmount(...)
The BIP21 non-asset path was the only one missing it. Fix is correct and minimal.
Edge case check: satoshis from decodeBip21() is typed number | undefined. If the BIP21 URI has no amount, satoshis is undefined → setAmount(undefined) correctly clears the field. If it has an amount, it's a positive number (0 and negatives are rejected by decodeBip21 validation). No issue.
Not protocol-critical — this is purely a UI display bug (fiat amount not rendering). No VTXO, signing, or transaction logic is affected.
Minor note (not blocking): The asset BIP21 path (lines 213-222) also doesn't call setAmount(). If asset BIP21 URIs with prefilled amounts show a similar fiat display issue, that's a separate bug to track. But assets set satoshis: 0 so it may not apply the same way.
🤖 Reviewed by Arkana
closes #518
Summary by CodeRabbit