feat: Donation opt in/edit button #575
Conversation
WalkthroughAdds localStorage-backed donation persistence and UI to the Widget: initializes internal state from storage, persists changes, gates donation application on Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Widget as Widget Component
participant Storage as localStorage
participant UI as Donation UI
Note over Widget: Mount
Widget->>Storage: getItem(DONATION_RATE_STORAGE_KEY)
Storage-->>Widget: storedRate or null
Widget->>Widget: init userDonationRate, donationEnabled, previousDonationRate
Note over User,UI: Interactions
User->>UI: toggle or edit % input
UI->>Widget: toggleDonation() / changeDonationRate(newRate)
Widget->>Widget: clamp/validate, set donationEnabled/previousDonationRate
Widget->>Storage: setItem(DONATION_RATE_STORAGE_KEY, userDonationRate)
Widget->>Widget: recalc amounts & URL if shouldApplyDonation()
Widget->>UI: re-render footer and amount displays
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
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 |
There was a problem hiding this comment.
- "Send ___ XEC" text doesn't change when the donation % changes
- Just do whole numbers (no decimals) to make it simpler and take up less horizontal space
- How about putting it on the same line as the "Powered by PayButton.org" so the dialog/widget doesn't need to be any taller?
- Have it off by default if no local storage value is set
- When off, don't display the input box + % label
|
Cool added those and made it more subtle |
|
For sure, widened it just a tad and set the limit to 99 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
| isChild, | ||
| donationAddress = config.donationAddress, | ||
| donationRate = DEFAULT_DONATE_RATE | ||
| donationRate: _donationRate = DEFAULT_DONATE_RATE // Unused - we default to 0 (off) if no localStorage value |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
react/lib/components/Widget/Widget.tsx (3)
168-169: Document the deprecation of thedonationRateprop.The
donationRateprop is now being ignored in favor of localStorage-backed user preferences, but it's still part of theWidgetPropsinterface. This could confuse users who pass this prop expecting it to work.Consider:
- Adding a deprecation notice in the interface documentation
- Adding a console warning if a user passes this prop
- Or removing it from the interface in the next major version
616-638: Consider extracting duplicate donation calculation logic.The donation calculation logic appears twice in this useEffect (lines 616-638 and 654-667) with very similar structure. Both calculate the donation amount as a percentage, add it to the original amount, create a currency object, and update the display text.
This duplication makes maintenance harder and increases the risk of bugs if only one location is updated.
Consider extracting into a helper function:
const calculateAmountWithDonation = ( baseAmount: number, currency: string, donationRate: number, randomSats: boolean = false ) => { const donationValue = baseAmount * (donationRate / 100) const totalAmount = baseAmount + donationValue const amountObj = getCurrencyObject(totalAmount, currency, randomSats) return { amountObj, donationValue } }Then use it in both places:
if (donationEnabled && userDonationRate > 0) { const { amountObj, donationValue } = calculateAmountWithDonation( thisCurrencyObject.float, currency, userDonationRate, false ) amountToDisplay = amountObj.string // ... rest of logic setDonationAmount(donationValue) }Also applies to: 654-667
806-830: Simplify redundant condition checks.Line 806 checks
userDonationRate && Number(userDonationRate), which is redundant. IfdonationEnabledis true and properly managed,userDonationRateshould already be a valid number > 0.Consider simplifying to:
- if (donationAddress && donationEnabled && userDonationRate && Number(userDonationRate)) { + if (donationAddress && donationEnabled && userDonationRate > 0) {This is clearer and avoids the double check.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
paybutton/dev/demo/index.html(1 hunks)react/lib/components/Widget/Widget.tsx(13 hunks)
🔇 Additional comments (9)
react/lib/components/Widget/Widget.tsx (8)
7-8: LGTM!The new imports for
IconButtonandTooltipare appropriate for the donation control UI being added.
120-120: LGTM!Using a constant for the localStorage key is good practice and improves maintainability.
304-304: LGTM!The new
fade-slide-upanimation adds a nice visual effect for the footer appearance.
372-382: LGTM!The footer layout changes to flexbox with centered alignment work well for the new donation controls UI.
700-709: LGTM!The localStorage persistence effect is well-structured with appropriate error handling. Saving on every
userDonationRatechange ensures the user's preference is preserved.
714-741: LGTM with smart auto-enable/disable behavior.The donation handlers correctly manage the toggle state and rate changes. The auto-enable when entering a value > 0 (line 733-735) and auto-disable when entering 0 (line 738-739) provide good UX by keeping the toggle state in sync with user intent.
Note: This relies on fixing the max value inconsistency mentioned in the earlier comment (lines 250-273).
830-830: LGTM!The useCallback dependencies correctly include all values used within the
resolveUrlfunction.
1076-1158: Well-implemented donation UI with good UX.The donation control UI is thoughtfully designed:
- Conditional rendering only for XEC with an amount present
- Tooltip provides context
- Heart icon visual state (filled/outlined) clearly indicates enabled/disabled
- Input only appears when enabled, reducing clutter
- Proper ARIA labels for accessibility
The implementation looks solid.
To ensure the donation feature works correctly across edge cases, please verify:
- Toggling donation on/off preserves and restores the percentage
- Changing the percentage updates the URL and QR code correctly
- Setting percentage to 0 disables the donation
- The donation amount is correctly calculated and added to multi-output BIP21 URIs
- localStorage persistence works across page reloads
paybutton/dev/demo/index.html (1)
30-31: LGTM - Test configuration for donation feature.Adding an explicit amount to this demo button enables testing of the new donation feature, as the donation UI only appears when an amount is present and the currency is XEC.
chedieck
left a comment
chedieck
left a comment
There was a problem hiding this comment.
Not working well. For a button set like this:
... we would expect to send 100,000 XEC to the original payout address and an extra 4,000 to our address. But copying the QR code actually shows:
... which is clearly not the desired behavior.
paybutton/dev/demo/index.html
Outdated
| display-currency="BCH" size="sm"></div> | ||
|
|
||
| <div class="paybutton" to="ecash:qp2v7kemclu7mv5y3h9qprwp0mrevkqt9gprvmm7yl" | ||
| <div class="paybutton" to="ecash:qp2v7kemclu7mv5y3h9qprwp0mrevkqt9gprvmm7yl" amount="100000" |
There was a problem hiding this comment.
This is a significant amount, I don't think is good to leave this in a test page.
| copied: boolean | ||
| } | ||
|
|
||
| const DONATION_RATE_STORAGE_KEY = 'paybutton_donation_rate' |
There was a problem hiding this comment.
Move this to react/lib/util/constants.ts, otherwise is still just a hardcoded string in a component file.
| const [goalPercent, setGoalPercent] = useState(0) | ||
| const [altpaymentEditable, setAltpaymentEditable] = useState<boolean>(false) | ||
|
|
||
| // Load donation rate from localStorage on mount, defaulting to 0 (off) if not set |
There was a problem hiding this comment.
"...defaulting to 0 (off) if not set"
then on line 262: "...default to off"
then on line 266 "Default to 0 (donation off)..."
Too much redundancy. Comments should add value, otherwise they just pollute and we end up skiping then when reading the code. Some of the comments here in this PR are indeed useful, but some are simply redundant.
|
Working better now, I think we need only a way to display that some amount is too small to be donated. For example, here: ... the original amount was 100, with the 9% donation, it shows a total of 109 XEC. However, this extra 9 XEC is smaller than our defined minimum (10 XEC), so the QR code (rightfully so) won't add this to the outputs: ...leading to a mismatch between the UI and the actual functionality |
…/paybutton into donation-opt-in-button
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
react/lib/components/Widget/Widget.tsx (1)
720-733: Fix maximum donation rate inconsistency.Line 721 clamps to
100, but the localStorage validation (line 257) and input field (line 1116) both use99as the maximum. This inconsistency was flagged in a previous review but remains unresolved.Apply this diff to align with the established maximum:
const handleDonationRateChange = (value: number) => { - const clampedValue = Math.max(0, Math.min(100, value)) + const clampedValue = Math.max(0, Math.min(99, value)) setUserDonationRate(clampedValue)
🧹 Nitpick comments (1)
react/lib/components/Widget/Widget.tsx (1)
168-169: Mark the unused prop to clarify intent.The
donationRateprop is destructured but superseded byuserDonationRate(loaded from localStorage). To clarify that this is intentional, prefix it with an underscore or add a comment.Apply this diff:
donationAddress = config.donationAddress, - donationRate = DEFAULT_DONATION_RATE, + donationRate: _donationRate = DEFAULT_DONATION_RATE, // Unused; donation rate is now persisted in localStorage } = props
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
react/lib/components/Widget/Widget.tsx(13 hunks)react/lib/util/constants.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
react/lib/components/Widget/Widget.tsx (2)
react/lib/util/constants.ts (6)
DEFAULT_DONATION_RATE(21-21)DONATION_RATE_STORAGE_KEY(25-25)DONATION_RATE_STORAGE_KEY(34-34)CURRENCY_PREFIXES_MAP(14-17)DECIMALS(5-9)DEFAULT_MINIMUM_DONATION_AMOUNT(29-32)react/lib/util/satoshis.ts (1)
getCurrencyObject(8-41)
🪛 Biome (2.1.2)
react/lib/util/constants.ts
[error] 34-34: Shouldn't redeclare 'DONATION_RATE_STORAGE_KEY'. Consider to delete it or rename it.
'DONATION_RATE_STORAGE_KEY' is defined here:
(lint/suspicious/noRedeclare)
🔇 Additional comments (6)
react/lib/components/Widget/Widget.tsx (6)
7-8: LGTM!The new imports support the donation UI controls (IconButton, Tooltip) and localStorage persistence (DONATION_RATE_STORAGE_KEY).
Also applies to: 18-18
249-274: LGTM!The localStorage initialization logic properly validates the stored donation rate (0-99), handles errors gracefully, and falls back to the prop value when initializing
previousDonationRate.
692-701: LGTM!The donation rate is properly persisted to localStorage whenever
userDonationRatechanges, with appropriate error handling.
789-825: LGTM!The URL resolution correctly calculates the donation amount from the base amount and only includes it in the URL when it meets the minimum threshold. Dependencies are properly tracked.
614-670: LGTM!The display logic correctly shows donation-adjusted amounts in the UI text while passing the base amount to
resolveUrl, which properly handles the donation as a separate output. The effect dependencies are appropriate.
1069-1156: LGTM!The donation UI is well-implemented with proper accessibility (aria-label), smooth animations, and conditional rendering. The threshold check at line 1072 (minimum * 100) correctly prevents showing the controls for amounts where even a small donation percentage would fall below the minimum donation amount, addressing the edge case discussed in the PR comments.
There was a problem hiding this comment.
Actionable comments posted: 0
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/Widget/Widget.tsx (1)
616-661: Donation threshold inconsistency confirmed and affects default user behavior.The inconsistency identified in the original review is confirmed:
- Line 616 (fiat): Requires
userDonationRate >= DONATION_RATE_FIAT_THRESHOLD(5%)- Line 650 (crypto XEC/BCH): Requires
userDonationRate > 0(any positive)- DEFAULT_DONATION_RATE is 2%, which falls below the fiat threshold
This means users with the default 2% donation rate will see their donation applied in crypto transactions but not in fiat conversions—a confusing inconsistency with no documentation explaining the behavior.
The threshold constant itself has no comments explaining the business rationale, and the disparity between the default rate and threshold suggests this may be unintentional.
Required actions:
- Either apply
DONATION_RATE_FIAT_THRESHOLDconsistently across both paths, OR- Document the intentional threshold difference with clear comments in
react/lib/util/constants.tsand inline code explaining why fiat conversions require a higher threshold than crypto- Consider adjusting
DEFAULT_DONATION_RATEif the 5% threshold is intentional
♻️ Duplicate comments (1)
react/lib/components/Widget/Widget.tsx (1)
720-733: Fix maximum donation rate inconsistency (still present from previous review).Line 721 clamps to
Math.min(100, value), but:
- Line 1116: Input field restricts to
max: 99- Line 257: localStorage validation checks
<= 99This inconsistency was flagged in a previous review but remains unaddressed. While unlikely in normal use, it's possible for the rate to briefly reach 100 through direct state manipulation or edge cases, creating an invalid state that conflicts with validation elsewhere.
Apply this diff to align all validations:
const handleDonationRateChange = (value: number) => { - const clampedValue = Math.max(0, Math.min(100, value)) + const clampedValue = Math.max(0, Math.min(99, value)) setUserDonationRate(clampedValue)
🧹 Nitpick comments (2)
react/lib/components/Widget/Widget.tsx (2)
703-704: Consider documenting the prop behavior change more prominently.These comments explain that
donationRateprop is no longer synced with internal state, with localStorage taking precedence. However, this is a significant deviation from typical React patterns where props are the source of truth. This could surprise developers integrating the widget.Consider adding a JSDoc comment at the
WidgetPropsinterface (around line 111) to clearly document thatdonationRateis only used as a fallback value and user preferences in localStorage will override it.Add documentation like this to the interface:
/** * Initial donation rate as a percentage (0-99). * Note: This is only used as a fallback. User preferences stored in localStorage take precedence. * @default 0 (no donation) */ donationRate?: number
1072-1156: Consider extracting the complex visibility condition to a helper function.Line 1072 contains a very complex conditional that determines whether to show the donation UI. While the logic is correct (checking currency type, positive amount, and minimum threshold), the inline expression is difficult to read and maintain.
Extract to a helper function for better readability:
const shouldShowDonationUI = (): boolean => { if (thisAddressType !== 'XEC' && thisAddressType !== 'BCH') return false if (!thisCurrencyObject?.float || thisCurrencyObject.float <= 0) return false const minimumAmount = DEFAULT_MINIMUM_DONATION_AMOUNT[thisAddressType.toUpperCase()] || 0 const threshold = minimumAmount * 100 // Hide for small amounts to avoid mismatches return thisCurrencyObject.float >= threshold }Then use it in the JSX:
-{((thisAddressType === 'XEC' || thisAddressType === 'BCH') && thisCurrencyObject?.float && thisCurrencyObject.float > 0 && thisCurrencyObject.float >= (DEFAULT_MINIMUM_DONATION_AMOUNT[thisAddressType.toUpperCase()] || 0) * 100) ? ( +{shouldShowDonationUI() ? (This makes the code more maintainable and adds documentation through the function name and inline comment about why the threshold exists.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
react/lib/components/Widget/Widget.tsx(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
react/lib/components/Widget/Widget.tsx (2)
react/lib/util/constants.ts (6)
DONATION_RATE_STORAGE_KEY(25-25)DONATION_RATE_STORAGE_KEY(34-34)DONATION_RATE_FIAT_THRESHOLD(26-26)CURRENCY_PREFIXES_MAP(14-17)DECIMALS(5-9)DEFAULT_MINIMUM_DONATION_AMOUNT(29-32)react/lib/util/satoshis.ts (1)
getCurrencyObject(8-41)
🔇 Additional comments (7)
react/lib/components/Widget/Widget.tsx (7)
7-8: LGTM! Imports are correctly added.The new imports for
IconButtonandTooltipfrom MUI, along withDONATION_RATE_STORAGE_KEYfrom constants, are properly used throughout the component.Also applies to: 18-18
250-274: LGTM! localStorage initialization is well-implemented.The initialization logic correctly:
- Loads and validates donation rate from localStorage with proper bounds checking (0-99)
- Handles errors gracefully with console warnings
- Memoizes the initial value to avoid recalculation
- Sets up state with sensible defaults
692-701: LGTM! Persistence effect is correctly implemented.The effect properly saves the donation rate to localStorage whenever it changes, with appropriate error handling.
706-718: LGTM! Toggle handler provides good UX.The handler correctly preserves the user's donation rate when toggling, allowing them to easily turn donations on/off without losing their preferred percentage.
798-824: LGTM! URL generation correctly handles the minimum donation edge case.The logic properly checks if the calculated donation amount meets the minimum threshold before appending donation parameters (line 809), which prevents the UI/output mismatch mentioned in the PR objectives.
670-670: LGTM! Effect dependencies are correctly configured.The dependency array properly includes
userDonationRateanddonationEnabledto ensure the effect runs when donation settings change. The previous infinite loop issue withdonationAmounthas been correctly addressed.
304-304: LGTM! Animation and styling are well-implemented.The new
fade-slide-upkeyframe animation and updated footer styles with flexbox layout provide a smooth, polished user experience for the donation UI.Also applies to: 367-382
|
sorry for the delay, had some merge conflicts to contend with and then github was down most the day for me. Think this is working as intended now. Its a bit confusing with the min rate for fiat values, but I have it so
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
react/lib/util/constants.ts (1)
23-23: Good addition, but check semicolon consistency.The new storage key constant is well-placed and follows naming conventions. However, line 23 is missing a semicolon while line 24 has one, creating a minor inconsistency.
Consider adding a semicolon for consistency:
-export const DONATION_RATE_STORAGE_KEY = 'paybutton_donation_rate' +export const DONATION_RATE_STORAGE_KEY = 'paybutton_donation_rate';react/lib/components/Widget/Widget.tsx (6)
642-660: Consider usingminDonationRateinstead of hardcoded constant.Line 642 checks
userDonationRate >= DONATION_RATE_FIAT_THRESHOLD, hardcoding the 5% minimum. While functionally correct, using the computedminDonationRatewould be more consistent with the context-aware minimum logic established in lines 261-263.Consider updating the condition:
- if ( donationEnabled && userDonationRate && userDonationRate >= DONATION_RATE_FIAT_THRESHOLD){ + if (donationEnabled && userDonationRate && userDonationRate >= minDonationRate) {This would maintain consistency if the minimum calculation logic ever changes.
678-691: Optional: Make minimum rate check explicit.Line 678 checks
userDonationRate > 0, which is safe because the UI and initialization logic prevent rates below the minimum. However, for clarity and consistency with the fiat path, consider checking againstminDonationRate:- if (donationEnabled && userDonationRate && userDonationRate > 0 && (cur === 'XEC' || cur === 'BCH')) { + if (donationEnabled && userDonationRate && userDonationRate >= minDonationRate && (cur === 'XEC' || cur === 'BCH')) {This makes the minimum requirement explicit and aligns with the context-aware minimum logic.
734-765: Handlers are functionally correct; consider UX feedback for clamping.The toggle and rate change handlers properly manage donation state with context-aware minimums. The rate handler aggressively clamps user input (line 753), which ensures valid state but might surprise users who type a value below the minimum.
Consider adding a tooltip or helper text to the input field explaining the context-specific minimum (5% for fiat, 1% for crypto) to improve user understanding when their input is clamped.
830-856: URL generation logic is correct; minor style issue.The multi-output URL format properly separates base and donation payments using BIP21 format. The minimum donation amount check (line 841) ensures only valid donations are included.
Fix minor style issue on line 841:
- if(thisDonationAmount >= minimumDonationAmount){ + if (thisDonationAmount >= minimumDonationAmount) {
1104-1115: Document or extract the magic number multiplier.Line 1110 multiplies
DEFAULT_MINIMUM_DONATION_AMOUNTby 100 to determine when to show the donation UI. This ensures the base amount is large enough for a meaningful donation (e.g., 1000 XEC needed so a 1% donation yields 10 XEC minimum).Add a comment explaining the calculation:
+ // Show UI only if base amount is 100x the minimum donation (e.g., 1000 XEC base for 10 XEC minimum donation at 1%) const minDonationAmount = (DEFAULT_MINIMUM_DONATION_AMOUNT[thisAddressType.toUpperCase()] || 0) * 100Or extract to a constant:
const MIN_BASE_AMOUNT_MULTIPLIER = 100; // Base must be 100x donation minimum for UI visibility const minDonationAmount = (DEFAULT_MINIMUM_DONATION_AMOUNT[thisAddressType.toUpperCase()] || 0) * MIN_BASE_AMOUNT_MULTIPLIER;
1148-1194: Input field width may be tight for 2-digit values.The donation rate TextField has comprehensive styling, but the 34px width (line 1166) might be cramped for displaying "99" with the current font size and padding. Consider testing across different browsers and zoom levels to ensure readability.
Consider increasing the width slightly if testing reveals cramping:
- width: '34px', + width: '38px',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
react/lib/components/Widget/Widget.tsx(12 hunks)react/lib/util/constants.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
react/lib/components/Widget/Widget.tsx (4)
react/lib/util/currency.ts (1)
isFiat(4-6)react/lib/util/constants.ts (5)
DONATION_RATE_FIAT_THRESHOLD(24-24)DONATION_RATE_STORAGE_KEY(23-23)CURRENCY_PREFIXES_MAP(14-17)DECIMALS(5-9)DEFAULT_MINIMUM_DONATION_AMOUNT(27-30)react/lib/util/types.ts (1)
CurrencyObject(4-8)react/lib/util/satoshis.ts (1)
getCurrencyObject(8-41)
⏰ 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
🔇 Additional comments (7)
react/lib/components/Widget/Widget.tsx (7)
7-8: LGTM - New imports are properly used.The IconButton and Tooltip imports support the donation UI footer, and DONATION_RATE_STORAGE_KEY enables localStorage persistence.
Also applies to: 18-18
253-307: Well-designed context-aware initialization with localStorage.The donation state initialization correctly:
- Computes context-aware minimums (5% for fiat conversions, 1% for crypto)
- Loads from localStorage with validation against current context minimum
- Disables donation if stored rate is below current minimum
- Preserves previous rate for toggle functionality
The logic handles edge cases like context switching and missing localStorage gracefully.
328-328: LGTM - Animation enhances UX.The
fade-slide-upkeyframe provides smooth entry for the donation footer UI, consistent with existing widget animations.
396-406: LGTM - Footer styling supports new donation UI.The flex layout and fade-slide-up animation provide smooth visual integration. Animation timing (0.7s delay) coordinates well with other widget elements.
698-698: LGTM - Effect dependencies are comprehensive and correct.The dependency array properly includes all donation-related state, ensuring the URL and display text update when donation settings change. The previous infinite loop issue (donationAmount in deps) has been successfully addressed.
721-729: LGTM - Clean localStorage persistence implementation.The effect correctly saves donation rate changes to localStorage with proper error handling and environment checks.
1118-1147: LGTM - Well-implemented toggle button with good accessibility.The heart icon button provides clear visual feedback (filled red when enabled, outlined when disabled) and includes proper ARIA labels for accessibility. The tooltip enhances discoverability.
| }, [hasPrice, currency]) | ||
|
|
||
| // Calculate minimum donation rate based on context | ||
| // Fiat requires 5% minimum, crypto allows 1% minimum |
There was a problem hiding this comment.
There was a bit of confusion on the %s:
Let's just have default min on both to be the same (2%). We can ignore the special fiat logic for "below 5%, don't change the displayed fiat price" that we specified in the task.
We're making it a deliberate donation now, rather than trying to hide it from them.
There was a problem hiding this comment.
Also both can do 1% (no different in mins).
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/Widget/Widget.tsx (1)
614-673: Critical: Mismatch between displayed amount and actual payment when donation is below minimum.The donation amount is added to the display text unconditionally (lines 655-662), but
resolveUrlonly includes the donation in the payment URL if it meets the network minimum (line 813 inresolveUrl). This creates a discrepancy where the UI shows a donation that won't actually be sent.Example scenario:
- Amount: 1000 XEC (just at visibility threshold)
- User manually types: 0.5% donation
- Display text: "Send 1,005.00 XEC" (1000 + 5 XEC donation)
- Actual QR/URL:
ecash:...?amount=1000(donation omitted because 5 < 10 minimum)Impact: Users are misled about the payment amount, seeing one value in the UI but sending a different (lower) amount.
Solution: Apply the same minimum donation check when calculating display amounts. Extract the check logic to avoid duplication:
+// Helper to check if donation meets minimum +const isDonationAboveMinimum = (donationAmount: number, currency: string): boolean => { + const minDonation = DEFAULT_MINIMUM_DONATION_AMOUNT[currency.toUpperCase()] || 0 + return donationAmount >= minDonation +} + // In the display calculation section (around line 654): -if (donationEnabled && userDonationRate && userDonationRate > 0 && (cur === 'XEC' || cur === 'BCH')) { +if (donationEnabled && userDonationRate && userDonationRate > 0 && (cur === 'XEC' || cur === 'BCH')) { const donationAmountValue = thisCurrencyObject.float * (userDonationRate / 100) - const amountWithDonation = thisCurrencyObject.float + donationAmountValue - const amountWithDonationObj = getCurrencyObject( - amountWithDonation, - cur, - false, - ) - amountToDisplay = amountWithDonationObj.string + // Only add donation to display if it meets the minimum + if (isDonationAboveMinimum(donationAmountValue, cur)) { + const amountWithDonation = thisCurrencyObject.float + donationAmountValue + const amountWithDonationObj = getCurrencyObject( + amountWithDonation, + cur, + false, + ) + amountToDisplay = amountWithDonationObj.string + } }Apply similar logic to the fiat conversion case (lines 618-635).
🧹 Nitpick comments (1)
react/lib/components/Widget/Widget.tsx (1)
793-829: Approve with optional defensive improvement suggestion.The
resolveUrlfunction correctly implements the donation logic with proper minimum threshold checking (line 813), ensuring donations below the network minimum are not added to the payment URL.Optional defensive improvement: Add a fallback for
decimalsto prevent potential precision loss ifnetworklookup fails unexpectedly:const decimals = network ? DECIMALS[network.toUpperCase()] : undefined; const donationPercent = userDonationRate / 100 const thisDonationAmount = amount * donationPercent const minimumDonationAmount = network ? DEFAULT_MINIMUM_DONATION_AMOUNT[network.toUpperCase()] : 0 thisUrl += `?amount=${amount}` if(thisDonationAmount >= minimumDonationAmount){ - thisUrl += `&addr=${donationAddress}&amount=${thisDonationAmount.toFixed(decimals)}`; + const safeDecimals = decimals ?? (network?.toUpperCase() === 'BCH' ? 8 : 2); + thisUrl += `&addr=${donationAddress}&amount=${thisDonationAmount.toFixed(safeDecimals)}`; }Though
networkshould always be valid given upstream validation, this prevents silent precision loss if the lookup unexpectedly fails.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
react/lib/components/Widget/Widget.tsx(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
react/lib/components/Widget/Widget.tsx (4)
react/lib/util/constants.ts (4)
DONATION_RATE_STORAGE_KEY(23-23)CURRENCY_PREFIXES_MAP(14-17)DECIMALS(5-9)DEFAULT_MINIMUM_DONATION_AMOUNT(27-30)react/lib/util/types.ts (1)
CurrencyObject(4-8)react/lib/util/currency.ts (1)
isFiat(4-6)react/lib/util/satoshis.ts (1)
getCurrencyObject(8-41)
⏰ 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
🔇 Additional comments (3)
react/lib/components/Widget/Widget.tsx (3)
252-283: LGTM! Clean localStorage initialization with proper validation.The localStorage loading logic correctly validates the stored value (0-99 range), handles SSR concerns with the window check, includes error handling, and initializes all related state variables consistently.
696-737: LGTM! Handler functions implement intuitive donation toggle behavior.The localStorage persistence and handler logic correctly:
- Saves changes to localStorage with proper error handling
- Preserves the previous donation rate when toggling off/on
- Auto-enables donation when user enters a positive value
- Auto-disables when user explicitly enters 0
- Clamps values consistently to 0-99 range
1076-1087: Visibility threshold is correctly calculated.The donation UI is appropriately hidden for amounts below the threshold, where even a 1% donation would fail to meet the network minimum. The logic correctly handles both fiat conversion (using
convertedCryptoAmount) and direct crypto scenarios.
chedieck
left a comment
chedieck
left a comment
There was a problem hiding this comment.
In the video below, is possible to observe three different situations:
- I set the button amount to 1000 XEC and the donation rate to 2%, which works well since it creates a QR-code with the info to transfer 1000 XEC to the original amount and 20 to the donation amount.
- I then set the amount in the button to 900 XEC. Since the user has defined the donation rate to 2% previously (notice this could be done not by changing the amount on that very same button, but by having opening another button before, since the
localStorageinformation is button agnostic), this supposedly would be a 18 XEC donation. However, the donation component is not rendered: my guess is this happens bc 1% of 900 is 9, which is less than the donation minimum of 10 XEC. Nonetheless, the amount appears as 918 and the QR-code indeed contains the information to donate 18 XEC, even tho this is not editable anymore. - Finally, I set the amount to 400 XEC, this still displays the total amount including that 2% donation rate (thus showing 408 XEC), but this time, since the 2% donation is 8 XEC which is less than the 10 XEC minimum, this doesn't go to the QR-code.
2025-11-20_12-06-26.mp4
Proposed Solution
The simplest way to solve the incongruent behavior IMO is to:
If 1% of the XEC amount is <10 XEC, not only hide the component but also ignore the donation rate altogether. Meaning, in case 2 and 3, the amount should appear as 900 & 400 XEC (resp.) and the QR code should contain no donation information, even if the saved donation rate was e.g. 5% and this 5% would be above minimum.
- only show the ui and apply the donation amount to the qr code if 1% of the button amount is >= the min donation amount
|
good catch. Added some better checks so if 1% of the amount is below the min donation amount the ui is hidden and nothing gets added to the qr code, even if the saved rate with the amount meets the threshold |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
react/lib/components/Widget/Widget.tsx (1)
304-305: Footer animation and donation UI UX are generally solid (minor nits only)The new
fade-slide-upkeyframe andfooterflex styling integrate cleanly and give the footer a subtle entrance animation. The donation UI itself—heart toggle, tooltip, and compact percentage field constrained to 1–99—fits the “subtle, opt‑in” goal and uses appropriate a11y labels.Two very minor UX nits (optional):
- The donation
TextFieldusesplaceholder="0"while the enforced minimum is1; changing the placeholder to"1"or""would better reflect the allowed range.- You might eventually want to coerce
userDonationRateto an integer (e.g.Math.round) insidehandleDonationRateChangeto align with the integerstep: 1, but allowing decimals is not functionally wrong given the current logic.Neither is blocking; the current implementation is fine to ship.
Also applies to: 367-382, 1091-1187
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
react/lib/components/Widget/Widget.tsx(13 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
react/lib/components/Widget/Widget.tsx (4)
react/lib/util/constants.ts (3)
DONATION_RATE_STORAGE_KEY(23-23)DEFAULT_MINIMUM_DONATION_AMOUNT(27-30)DECIMALS(5-9)react/lib/util/types.ts (1)
CurrencyObject(4-8)react/lib/util/currency.ts (1)
isFiat(4-6)react/lib/util/satoshis.ts (1)
getCurrencyObject(8-41)
⏰ 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
🔇 Additional comments (3)
react/lib/components/Widget/Widget.tsx (3)
7-18: Imports and constant usage look consistentAdding
IconButton,Tooltip, and importingDONATION_RATE_STORAGE_KEYfromutil/constantsis consistent with the rest of the file and centralizes the storage key as requested in earlier reviews. No issues here.
283-283: Donation visibility and minimum-threshold gating align with requirements
shouldShowDonationUIandshouldApplyDonationcorrectly:
- Restrict donations to
XEC/BCH.- Use
DEFAULT_MINIMUM_DONATION_AMOUNTso that 1% of the base (or converted) amount must meet the network minimum before the UI shows or a donation is applied.- Reuse the same helper both when building display text and when generating URLs, avoiding UI/QR mismatches.
- For fiat,
convertedCryptoAmountis stored once and reused in the footer visibility check, so the threshold is applied in crypto terms.This matches the “hide the heart when base amount is below the minimum-donation threshold” behavior described in the PR discussion.
Also applies to: 599-618, 635-642, 651-652, 667-691, 1093-1103
821-833: Donation URL generation is consistent with gating, but consider a small guardThe
resolveUrlchanges:
- Respect
disabled/toguards andCURRENCY_PREFIXES_MAPas before.- Only attach donation outputs when
donationAddress && shouldApplyDonation(amount, currencyType)is true, so QR contents stay in sync with the text logic that also usesshouldApplyDonation.- Use
DECIMALS[currencyType](falling back to XEC) to format the donation amount.One small robustness improvement: the footer donation UI (Lines 1093–1103) doesn’t check
donationAddressbefore rendering the heart/input, whileresolveUrlsilently skips the donation ifdonationAddressis falsy. To avoid an apparent “working” toggle that never affects the QR, you could optionally gate the UI on a truthydonationAddress:- return (thisAddressType === 'XEC' || thisAddressType === 'BCH') && - amountToCheck !== undefined && - amountToCheck > 0 && - shouldShowDonationUI(amountToCheck, thisAddressType) + return !!donationAddress && + (thisAddressType === 'XEC' || thisAddressType === 'BCH') && + amountToCheck !== undefined && + amountToCheck > 0 && + shouldShowDonationUI(amountToCheck, thisAddressType)Not critical if
donationAddressis always configured, but it makes behavior safer if it’s ever omitted.
[ suggest_recommended_refactor ]Also applies to: 845-845
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
react/lib/components/Widget/Widget.tsx (1)
743-761: Verify fallback to DEFAULT_DONATION_RATE when enabling with no valid rate.When enabling donation via toggle, if both
previousDonationRateandclampedDonationRatePropare0, the code setsdonationEnabled = truebutuserDonationRate = 0. AlthoughshouldApplyDonationcorrectly short-circuits onuserDonationRate <= 0, the UI displays an enabled heart icon with a0%input, which contradicts the intended 1–99% range and creates user confusion.A past review comment suggested falling back to
DEFAULT_DONATION_RATEin this scenario. Consider:const handleDonationToggle = () => { if (donationEnabled) { // Turning off - save current rate (already clamped) and set to 0 setPreviousDonationRate(userDonationRate) setUserDonationRate(0) setDonationEnabled(false) } else { // Turning on - restore previous rate or use clamped prop/default // Use same clamping logic as handleDonationRateChange to ensure 1-99 range - const rateToRestore = previousDonationRate > 0 ? previousDonationRate : clampedDonationRateProp + const rateToRestore = + previousDonationRate > 0 + ? previousDonationRate + : clampedDonationRateProp > 0 + ? clampedDonationRateProp + : DEFAULT_DONATION_RATE const clampedRate = clampDonationRate(rateToRestore) setUserDonationRate(clampedRate) setDonationEnabled(true) // Update previousDonationRate to the clamped value if (clampedRate > 0) { setPreviousDonationRate(clampedRate) } } }
🧹 Nitpick comments (2)
react/lib/components/Widget/Widget.tsx (2)
763-773: Consider auto-disabling donation when rate is set to 0.When
clampedValueis0(e.g., from manual localStorage edit or edge-case input), the handler leavesdonationEnabledin its current state. If enabled, the UI shows a filled heart with0%, which is confusing since no donation is applied (correctly gated byshouldApplyDonation).For consistency with the auto-enable behavior (line 769), consider auto-disabling when the rate becomes 0:
const handleDonationRateChange = (value: number) => { const clampedValue = clampDonationRate(value) setUserDonationRate(clampedValue) if (clampedValue >= 1) { // Auto-enable donation if user enters a value >= 1 if (!donationEnabled) { setDonationEnabled(true) } setPreviousDonationRate(clampedValue) + } else if (clampedValue === 0) { + // Auto-disable donation if user enters 0 + setDonationEnabled(false) } }Note: The input constraints (
min: 1) prevent this via normal UI interaction, so this primarily handles edge cases.
829-862: Consider using toFixed for base amount consistency.The donation amount is formatted with
toFixed(decimals)(line 848), but the base amount (line 847) is not. WhilegetCurrencyObjectalready rounds values, usingtoFixedon the base amount would guard against floating-point precision edge cases in URLs and maintain formatting consistency between both outputs.if (amount) { // Check if donation should be applied (1% of amount >= minimum) const currencyType = currency.toUpperCase() if (donationAddress && shouldApplyDonation(amount, currencyType)) { const decimals = DECIMALS[currencyType] || DECIMALS.XEC; const donationPercent = userDonationRate / 100 // Calculate donation amount from base amount const thisDonationAmount = amount * donationPercent - thisUrl += `?amount=${amount}` + thisUrl += `?amount=${amount.toFixed(decimals)}` thisUrl += `&addr=${donationAddress}&amount=${thisDonationAmount.toFixed(decimals)}`; } else { thisUrl += `?amount=${amount}` } }Otherwise, LGTM! The multi-output BIP21 URI format is correct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
react/lib/components/Widget/Widget.tsx(13 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
react/lib/components/Widget/Widget.tsx (4)
react/lib/util/constants.ts (3)
DONATION_RATE_STORAGE_KEY(23-23)DEFAULT_MINIMUM_DONATION_AMOUNT(27-30)DECIMALS(5-9)react/lib/util/types.ts (1)
CurrencyObject(4-8)react/lib/util/currency.ts (1)
isFiat(4-6)react/lib/util/satoshis.ts (1)
getCurrencyObject(8-41)
⏰ 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
🔇 Additional comments (4)
react/lib/components/Widget/Widget.tsx (4)
609-628: LGTM! Clean donation eligibility logic.The two-tier approach (visibility vs. application) correctly implements the requirement that 1% of the amount must meet the minimum donation threshold. The currency normalization and defensive fallback are good practices.
630-707: LGTM! Correct donation application in both fiat and crypto paths.The effect properly:
- Applies donation percentage consistently to both fiat display and converted crypto amounts in the fiat conversion path
- Maintains exchange rate correctness (total fiat / price = total crypto)
- Passes base amounts (without donation) to
resolveUrl, which handles donation logic internally- Clears
convertedCryptoAmountwhen not in fiat mode to avoid stale stateThe dual gating (lines 651, 687) ensures donations are only applied when eligible.
729-738: LGTM! Straightforward localStorage persistence.The effect correctly persists user donation rate changes with appropriate error handling. The single dependency ensures it saves on every rate change.
1105-1204: LGTM! Donation UI correctly implements visibility and interaction logic.The conditional rendering (lines 1109-1120) properly gates the donation controls based on currency type, amount presence, and minimum threshold via
shouldShowDonationUI. The heart toggle and percentage input provide clear, accessible UI elements. The input constraints (min: 1,max: 99) align with validation logic, and the controlled input pattern withparseFloatfallback handles edge cases adequately.The fade-slide-up animation and compact styling integrate well with the footer.
chedieck
left a comment
chedieck
left a comment
There was a problem hiding this comment.
Looks good. Looks like there is a bug in master where fiat buttons always show "send any amount of X" instead of converting to fiat, so couldn't test that.
|
Sweet. I think if you have paybutton server running while testing the fiat amount should work |
Related to #574
Description
Adding the user controlled button and input to edit the donation amount
Test plan
Test a button with an amount and try changing the value, turning it on and off. check the proper donation is being applied
Summary by CodeRabbit
New Features
Style
✏️ Tip: You can customize this high-level summary in your review settings.