fix: amount not being set correctly when using fiat value #600
Conversation
📝 WalkthroughWalkthroughThe useEffect handling currency conversion in PayButton.tsx was simplified. The fiat path now always computes converted values instead of conditionally, and the non-fiat path with randomSatoshis now follows a more direct computation path, reducing overall conditional branching while potentially altering recalculation timing. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
react/lib/components/PayButton/PayButton.tsx (1)
379-389: Missing dependency in useEffect array.The non-fiat branch at line 381 uses the state variable
addressType, butaddressTypeis not included in the dependency array at line 389. This means the effect won't re-run whenaddressTypestate changes, potentially causing stale conversions when the address type is updated (e.g., by the PaymentDialog component).🔎 Add missing dependency
- }, [price, currencyObj, amount, currency, randomSatoshis, to]); + }, [price, currencyObj, amount, currency, randomSatoshis, to, addressType]);Note: If you implement Option 2 from the previous comment (computing
addressTypefromtoin all branches), you can remove the state dependency and rely ontoinstead.
🧹 Nitpick comments (1)
react/lib/components/PayButton/PayButton.tsx (1)
370-370: Use more precise type annotation.
getCurrencyTypeFromAddressreturnsCryptoCurrency(per the relevant code snippet), but the variable is annotated as the broaderCurrencytype. While not incorrect, using the more specific type would improve clarity.🔎 More precise type
- const addressType: Currency = getCurrencyTypeFromAddress(to); + const addressType: CryptoCurrency = getCurrencyTypeFromAddress(to);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
react/lib/components/PayButton/PayButton.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
react/lib/components/PayButton/PayButton.tsx (4)
react/lib/util/types.ts (1)
Currency(140-140)react/lib/util/address.ts (1)
getCurrencyTypeFromAddress(26-34)react/lib/util/satoshis.ts (1)
getCurrencyObject(8-41)react/lib/util/currency.ts (1)
isFiat(4-6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run build
| const addressType: Currency = getCurrencyTypeFromAddress(to); | ||
| const convertedObj = getCurrencyObject( | ||
| currencyObj.float / price, | ||
| addressType, | ||
| randomSatoshis, | ||
| ); | ||
| setCryptoAmount(convertedObj.string); | ||
| setConvertedCurrencyObj(convertedObj); | ||
| } else if (!isFiat(currency) && randomSatoshis) { |
There was a problem hiding this comment.
Variable shadowing creates inconsistency with non-fiat branch.
Line 370 declares a local addressType variable that shadows the state variable (line 121), but this local variable is only used in the fiat branch. The non-fiat branch at lines 379-385 uses the state addressType variable. This inconsistency makes the code harder to reason about and could lead to subtle bugs if the state and derived values diverge.
Additionally, getCurrencyTypeFromAddress can throw an error for invalid addresses (per the relevant code snippet), but there's no error handling here.
🔎 Suggested improvements
Consider one of these approaches:
Option 1: Use a distinct name for the local variable
- const addressType: Currency = getCurrencyTypeFromAddress(to);
+ const derivedAddressType = getCurrencyTypeFromAddress(to);
const convertedObj = getCurrencyObject(
currencyObj.float / price,
- addressType,
+ derivedAddressType,
randomSatoshis,
);Option 2: Update the state variable and use it consistently
+ const derivedAddressType = getCurrencyTypeFromAddress(to);
+ setAddressType(derivedAddressType);
const convertedObj = getCurrencyObject(
currencyObj.float / price,
- addressType,
+ derivedAddressType,
randomSatoshis,
);Option 3: Add error handling
+ try {
const addressType: Currency = getCurrencyTypeFromAddress(to);
const convertedObj = getCurrencyObject(
currencyObj.float / price,
addressType,
randomSatoshis,
);
setCryptoAmount(convertedObj.string);
setConvertedCurrencyObj(convertedObj);
+ } catch (error) {
+ console.error('Invalid address type:', error);
+ // Handle error appropriately
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const addressType: Currency = getCurrencyTypeFromAddress(to); | |
| const convertedObj = getCurrencyObject( | |
| currencyObj.float / price, | |
| addressType, | |
| randomSatoshis, | |
| ); | |
| setCryptoAmount(convertedObj.string); | |
| setConvertedCurrencyObj(convertedObj); | |
| } else if (!isFiat(currency) && randomSatoshis) { | |
| const derivedAddressType = getCurrencyTypeFromAddress(to); | |
| const convertedObj = getCurrencyObject( | |
| currencyObj.float / price, | |
| derivedAddressType, | |
| randomSatoshis, | |
| ); | |
| setCryptoAmount(convertedObj.string); | |
| setConvertedCurrencyObj(convertedObj); | |
| } else if (!isFiat(currency) && randomSatoshis) { |
| const addressType: Currency = getCurrencyTypeFromAddress(to); | |
| const convertedObj = getCurrencyObject( | |
| currencyObj.float / price, | |
| addressType, | |
| randomSatoshis, | |
| ); | |
| setCryptoAmount(convertedObj.string); | |
| setConvertedCurrencyObj(convertedObj); | |
| } else if (!isFiat(currency) && randomSatoshis) { | |
| const derivedAddressType = getCurrencyTypeFromAddress(to); | |
| setAddressType(derivedAddressType); | |
| const convertedObj = getCurrencyObject( | |
| currencyObj.float / price, | |
| derivedAddressType, | |
| randomSatoshis, | |
| ); | |
| setCryptoAmount(convertedObj.string); | |
| setConvertedCurrencyObj(convertedObj); | |
| } else if (!isFiat(currency) && randomSatoshis) { |
| const addressType: Currency = getCurrencyTypeFromAddress(to); | |
| const convertedObj = getCurrencyObject( | |
| currencyObj.float / price, | |
| addressType, | |
| randomSatoshis, | |
| ); | |
| setCryptoAmount(convertedObj.string); | |
| setConvertedCurrencyObj(convertedObj); | |
| } else if (!isFiat(currency) && randomSatoshis) { | |
| try { | |
| const addressType: Currency = getCurrencyTypeFromAddress(to); | |
| const convertedObj = getCurrencyObject( | |
| currencyObj.float / price, | |
| addressType, | |
| randomSatoshis, | |
| ); | |
| setCryptoAmount(convertedObj.string); | |
| setConvertedCurrencyObj(convertedObj); | |
| } catch (error) { | |
| console.error('Invalid address type:', error); | |
| // Handle error appropriately | |
| } | |
| } else if (!isFiat(currency) && randomSatoshis) { |
Description
Amount wouldn't change even after closing the dialog and opening up a new one with a different fiat value; this fixes that.
Test plan
I was only able to reproduce the issue on coin.dance/support which has since been updated to use this branch... not sure how else to test the issue in master.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.