Fix: wait for qr code to copy and pay #590
Conversation
WalkthroughVersion bumped to 5.1.2 across package.json files. Widget component props Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
949-955: AddqrLoadingto thehandleQrCodeClickdependency arrayThe callback reads
qrLoadingbut doesn't include it in the dependency array[disabled, to, url, setCopied, setRecentlyCopied]. This creates a stale closure where the callback retains an outdatedqrLoadingvalue from when it was memoized. While the component will eventually re-render and recreate the callback, the callback won't immediately respond toqrLoadingstate changes within that render cycle, potentially causing unexpected behavior when loading state flips to false.Add
qrLoadingto the dependencies:- const handleQrCodeClick = useCallback((): void => { - if (disabled || to === undefined || qrLoading) return - if (!url || !copyToClipboard(url)) return - setCopied(true) - setRecentlyCopied(true) - }, [disabled, to, url, setCopied, setRecentlyCopied]) + const handleQrCodeClick = useCallback((): void => { + if (disabled || to === undefined || qrLoading) return + if (!url || !copyToClipboard(url)) return + setCopied(true) + setRecentlyCopied(true) + }, [disabled, to, url, qrLoading, setCopied, setRecentlyCopied])Re-run tests to confirm QR behavior remains correct after this change.
🧹 Nitpick comments (2)
react/lib/components/Widget/Widget.tsx (2)
182-190: qrLoading integration generally solves the “too early click” issue for childrenIntroducing
isWaitingForPaymentIdand combining it withloadingintoqrLoadingto gate:
- QR visibility/animations,
- the hint text (“Loading...” vs “Not yet ready for payment”),
- and the main button’s
disabledstate (disabled || qrLoading)makes it much harder for users to click before the widget is actually ready. For child widgets that rely on an externally supplied
paymentId, this looks aligned with the PR objective.One thing to verify: non‑child flows still allow clicking while an internally generated
paymentIdis being created. If that was also part of the original bug, you may want to reuseisWaitingForPaymentIdor a similar flag on the non‑child path as well.Also applies to: 261-270, 351-383, 656-680, 1263-1273
731-749: Donation visibility and multi-output URL construction look coherent, but consider small clarificationsThe new helpers:
shouldShowDonationUI(amount, currencyType)shouldApplyDonation(amount, currencyType)and the updated donation URL logic in
resolveUrl:
- Only apply donation when
donationEnabledanduserDonationRate > 0.- Gate UI and URL behavior on
DEFAULT_MINIMUM_DONATION_AMOUNTthresholds.- Use a base amount for the main address and an additional
addr/amountpair for the donation output.are logically consistent with a multi‑output payment format (base
?amount=...for the primary address, followed by&addr=<donationAddress>&amount=<donationAmount>for the donation).Two small suggestions to improve clarity and reduce future mistakes:
Make the multi-output semantics explicit in code comments
The current code assumes consumers understand repeating
addr/amountis intentional. A brief comment where you append the donation params would help future maintainers.Avoid duplicating the
?amount=${amount}string literalBoth branches (
ifwith donation andelsewithout) build?amount=${amount}. Extracting the common part (or building a params array first) would make it less error‑prone if you ever tweak the parameter name or behavior.These are readability/maintainability concerns; behavior looks correct.
Also applies to: 761-833, 956-989, 1314-1405
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
package.json(1 hunks)paybutton/package.json(1 hunks)react/lib/components/Widget/Widget.tsx(3 hunks)react/package.json(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Run tests
react/lib/components/Widget/Widget.tsx
[error] 1-1: Command failed: yarn test (in react directory).
react/package.json
[error] 1-1: Command failed: yarn test (in react directory).
⏰ 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 (6)
paybutton/package.json (1)
3-3: Version bump metadata looks consistentPatch version updated to 5.1.2; this keeps the paybutton package in sync with the root/react packages and matches the bugfix nature of this PR.
package.json (1)
3-3: Root package version bump is appropriateRoot package.json is bumped to 5.1.2, matching sub-packages; good for a coordinated bugfix release.
react/package.json (1)
3-3: React package version bump is fine; fix failing tests before mergeThe 5.1.2 version bump here is consistent with the rest of the workspace. However, CI indicates
yarn testis currently failing in the react package, likely due to the Widget changes (types or behavior/snapshots). Please runcd react && yarn test, address any failures, and commit updated tests/snapshots before merging.react/lib/components/Widget/Widget.tsx (3)
57-75: Makingsuccess/disabledoptional in WidgetProps with sensible defaultsChanging
successanddisabledto optional inWidgetPropsand defaultingsuccesstofalsein the destructuring keeps the runtime behavior predictable while relaxing the public API. Internaldisabledstate still governs the widget, so omitting the prop is safe.Please double‑check downstream TS usage/tests for any assumptions that these props are required (e.g., component instantiations in tests or storybook configs) and update them if necessary.
Also applies to: 128-178
314-323: PaymentId initialization effect is robust and avoids redundant API callsThe new
useEffectthat derives aneffectiveAmount(from converted crypto, convertedCurrencyObj, or thisAmount), skips invalid/NaN values, memoizes the last effective amount vialastEffectiveAmountRef, and callscreatePaymentonly when needed is a solid improvement:
- It avoids hammering the API on every render/amount tweak.
- It correctly waits for fiat conversion (
isFiat(currency) && convertedCryptoAmount === undefined).- It no‑ops for children or when
disablePaymentId/setPaymentId/toconditions make payment IDs irrelevant.This logic looks correct and nicely contained.
Also applies to: 588-653
1188-1258: NumericFormat usage for editable amount looks soundThe
NumericFormatconfiguration for editing the amount:
- Tracks raw numeric
values.valueintodraftAmount.- Applies sensible constraints in
isAllowed(non‑negative, belowMAX_AMOUNT[...], and within decimal precision fromDECIMALS[...]).- Prevents redundant confirmation when the draft already matches the current amount (
isSameAmount).- Integrates with
applyDraftAmountto update the canonical amount and conversion state.This is a good use of
react-number-formatfor constrained numeric input.
|
We'll go with 5.2.0 but I can do that. |
Related to #
Description
Fixes issue where clicking the QR code or the button to pay too early would use the payment info without the paymentId
Test plan
Summary by CodeRabbit
Bug Fixes
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.
Greptile Overview
Greptile Summary
Prevented early QR code copying and button clicks before the
paymentIdloads, ensuring payment URLs always include the complete payment information. Madesuccessanddisabledprops optional for better API flexibility.Confidence Score: 5/5
qrLoadingstate (which includesisWaitingForPaymentId) before allowing user interactions. The changes are minimal, focused, and consistent with existing patterns in the codebase. The props made optional have sensible defaults already in place.Important Files Changed
File Analysis
successanddisabledprops optional, addedqrLoadingcheck to prevent QR copy and button clicks before paymentId loadsSequence Diagram
sequenceDiagram participant User participant Widget participant API participant QRCode participant Button Note over Widget: Component Mounts Widget->>Widget: Initialize (loading=true) alt isChild = false AND !disablePaymentId Widget->>API: createPayment(amount, address) Note over Widget: isWaitingForPaymentId = true Note over Widget: qrLoading = true API-->>Widget: paymentId Widget->>Widget: setPaymentId(paymentId) Note over Widget: isWaitingForPaymentId = false end Widget->>Widget: getAddressBalance() Widget->>Widget: setLoading(false) Note over Widget: qrLoading = false (if paymentId loaded) alt User clicks QR Code User->>QRCode: Click QRCode->>Widget: handleQrCodeClick() alt qrLoading = true Widget-->>User: No action (prevented) Note over Widget: FIX: Prevents copying incomplete URL else qrLoading = false Widget->>Widget: copyToClipboard(url with paymentId) Widget-->>User: "Payment copied!" end end alt User clicks Button User->>Button: Click Button->>Widget: handleButtonClick() alt disabled OR qrLoading = true Widget-->>User: Button disabled (no action) Note over Widget: FIX: Prevents navigation with incomplete URL else qrLoading = false Widget->>Widget: Navigate to url with paymentId end end