fix: many fixes and tests. #592
Conversation
WalkthroughReworks paymentId and amount/currency synchronization between PayButton and Widget (removing polling, using convertedCurrencyObj, coordinating createPayment), relaxes several WidgetContainer prop typings, adds a standalone demo HTML, expands data-driven tests, and adds a devDependency for canvas. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PayButton
participant Widget
participant API as createPayment
participant Dialog
User->>PayButton: click (open)
PayButton->>Widget: set dialogOpen, propagate amount & currencyObj
alt paymentId required
Widget->>API: createPayment(amount, currency, to)
API-->>Widget: paymentId
Widget-->>PayButton: notify paymentId available
else paymentId not required
Widget-->>PayButton: paymentId = undefined
end
Widget->>Dialog: render QR/confirm (paymentId/amount)
Dialog-->>PayButton: onOpen(paymentId?, amount)
User->>Dialog: confirm / complete payment
Dialog-->>PayButton: onSuccess / transaction callbacks
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
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)
155-158: Silent error handling may leave users stuck on "Loading".When payment creation fails, the error is only logged to console. The UI shows "Loading" indefinitely (as confirmed by tests). Consider setting an error state to inform users and allow retry.
+ const [paymentError, setPaymentError] = useState<string | undefined>(undefined); // In getPaymentId: } catch (err) { console.error('Error creating payment ID:', err) + setPaymentError('Failed to initialize payment. Please try again.') return undefined }
🧹 Nitpick comments (12)
react/lib/tests/components/Widget.test.tsx (1)
383-388: Fiat cases silently skip URL validation.For FIAT_CASES (USD, CAD),
currencyis neither 'XEC' nor 'BCH', so no assertion is made. Since fiat cases useTEST_ADDRESSES.ecash, the URL should containecash:. Consider checking the address prefix based on thetoaddress rather than thecurrencyprop:- if (currency === 'XEC') { - expect(copied).toContain('ecash:') - } else if (currency === 'BCH') { - expect(copied).toContain('bitcoincash:') - } + if (to.startsWith('ecash:') || currency === 'XEC') { + expect(copied).toContain('ecash:') + } else if (to.startsWith('bitcoincash:') || currency === 'BCH') { + expect(copied).toContain('bitcoincash:') + }react/lib/tests/components/PayButton.test.tsx (7)
40-51: Consider usingjest.spyOnfor cleaner console error suppression.While this approach works, using
jest.spyOn(console, 'error').mockImplementation(...)with conditional filtering is a more idiomatic Jest pattern and automatically restores the original inafterAll.-const realConsoleError = console.error -beforeAll(() => { - console.error = (...args: any[]) => { - if (args.some(a => typeof a === 'string' && a.includes('Error creating payment ID'))) { - return - } - realConsoleError(...args) - } -}) -afterAll(() => { - console.error = realConsoleError -}) +let consoleErrorSpy: jest.SpyInstance +beforeAll(() => { + consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation((...args: any[]) => { + if (args.some(a => typeof a === 'string' && a.includes('Error creating payment ID'))) { + return + } + }) +}) +afterAll(() => { + consoleErrorSpy.mockRestore() +})
102-113: Investigate the double-click workaround.The comment "for some reason first click here in the tests is not doing anything" suggests a potential race condition or timing issue in the component or test setup. This workaround may mask a real bug or cause flakiness. Additionally, Line 112 re-checks what Line 105 already verified.
Consider investigating why the first click doesn't work for fiat cases specifically—it might be related to async price fetching or state initialization.
157-161: Avoid direct DOM queries for Material-UI internals.Querying
.MuiBackdrop-rootdirectly couples tests to MUI's implementation details. Consider usingscreen.getByRole('presentation')or adding adata-testidto the dialog's backdrop for more resilient tests.- const backdrop = document.querySelector('.MuiBackdrop-root')!; - await user.click(backdrop); + // Consider adding data-testid="dialog-backdrop" to the PaymentDialog component + const backdrop = screen.getByTestId('dialog-backdrop'); + await user.click(backdrop);
194-195: Repeated double-click pattern across fiat tests.The double-click workaround appears in multiple fiat test cases (lines 194-195, 312-313, 436-437). This pattern should be investigated and fixed at the source rather than worked around in every fiat test. Consider creating a helper function if the workaround is truly necessary.
Also applies to: 312-313, 436-437
370-375: AvoidwaitForfor negative assertions.
waitFor(() => expect(onOpen).toHaveBeenCalledTimes(0))will always wait for the full timeout before passing since the condition is immediately true. Use a small delay and then assert directly, or restructure the test.await user.click(screen.getByRole('button', { name: /donate/i })) - await waitFor(() => expect(onOpen).toHaveBeenCalledTimes(0)) + // Wait for loading state to appear, then verify onOpen wasn't called + await expect(screen.findByText(/loading/i)).resolves.toBeDefined() + expect(onOpen).not.toHaveBeenCalled() - await expect(screen.findByText(/loading/i)).resolves.toBeDefined()
407-408: Inconsistent indentation.Lines 408 and 450 have extra indentation compared to surrounding code.
const backdrop = document.querySelector('.MuiBackdrop-root')! - await user.click(backdrop) + await user.click(backdrop)Also applies to: 449-450
461-461: UseTEST_ADDRESSES.XECconstant for consistency.The hardcoded address in the expected QR value should reference the
TEST_ADDRESSESconstant to maintain consistency with the rest of the test file.- expect(qr.getAttribute('data-value')).toBe('ecash:qz3wrtmwtuycud3k6w7afkmn3285vw2lfy36y43nvk?amount=17.89&op_return_raw=0450415900000010ffff2233445566778899aabbccddeeff'); + expect(qr.getAttribute('data-value')).toBe(`${TEST_ADDRESSES.XEC}?amount=17.89&op_return_raw=0450415900000010ffff2233445566778899aabbccddeeff`);react/lib/components/PayButton/PayButton.tsx (4)
167-199: Consider adding cleanup for the async payment creation.If the dialog closes or amount changes rapidly before
getPaymentIdcompletes, stale responses could set incorrectpaymentIdvalues. Consider using an abort flag orAbortControllerpattern.useEffect(() => { if (!dialogOpen || disablePaymentId || !to) return; + let cancelled = false; let effectiveAmount: number | null | undefined; // ... amount computation ... lastPaymentAmount.current = effectiveAmount; setPaymentId(undefined); - void getPaymentId(currency, to, effectiveAmount ?? undefined); + getPaymentId(currency, to, effectiveAmount ?? undefined).then((id) => { + if (!cancelled && id) { + setPaymentId(id); + } + }); + + return () => { + cancelled = true; + }; }, [ dialogOpen, currency, amount, convertedCurrencyObj, disablePaymentId, to, getPaymentId, ]);Note: This would require removing
setPaymentId(responsePaymentId)fromgetPaymentIdand having it just return the ID.
212-222: Replace magic string with a constant or boolean flag.Using
'__no_paymentid__'as a sentinel value is fragile and unclear. Consider using a dedicated boolean ref or a well-named constant to track whether onOpen was fired for the "no paymentId" case.+const NO_PAYMENT_ID_SENTINEL = Symbol('no_paymentid'); +// or at module level: const NO_PAYMENT_ID_FIRED = '__no_paymentid__' as const; if (disablePaymentId) { hasFiredOnOpenRef.current = true; - lastOnOpenPaymentId.current = '__no_paymentid__'; + lastOnOpenPaymentId.current = undefined; // Since hasFiredOnOpenRef tracks firing, this isn't needed if (isFiat(currency)) { onOpen(cryptoAmountRef.current, to, undefined); } else { onOpen(amount, to, undefined); } return; }
131-133: Dependency array includes bothcryptoAmountandconvertedCurrencyObj.The effect only uses
convertedCurrencyObj?.string, butcryptoAmountis also in the dependency array. SincecryptoAmountis set fromconvertedCurrencyObj.stringelsewhere, this may cause extra re-runs. Consider removingcryptoAmountif it's redundant.useEffect(() => { cryptoAmountRef.current = convertedCurrencyObj?.string; - }, [cryptoAmount, convertedCurrencyObj]); + }, [convertedCurrencyObj]);
136-161: Callback has dual responsibility: returns AND sets paymentId.
getPaymentIdboth returns the paymentId and callssetPaymentId. This makes the flow confusing since callers don't need the return value (it's ignored withvoidon line 190). Consider either:
- Having it only set state (remove return value), or
- Having it only return the value and let callers set state
This would also facilitate the abort pattern mentioned earlier.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
paybutton/dev/demo/single.html(1 hunks)react/lib/components/PayButton/PayButton.tsx(4 hunks)react/lib/components/Widget/Widget.tsx(12 hunks)react/lib/components/Widget/WidgetContainer.tsx(1 hunks)react/lib/tests/components/PayButton.test.tsx(1 hunks)react/lib/tests/components/Widget.test.tsx(1 hunks)react/lib/tests/util/constants.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
react/lib/tests/components/Widget.test.tsx (3)
react/lib/util/types.ts (1)
Currency(140-140)react/lib/tests/util/constants.ts (1)
TEST_ADDRESSES(1-4)react/lib/util/api-client.ts (1)
createPayment(93-113)
react/lib/components/Widget/Widget.tsx (4)
react/lib/themes/Theme.ts (1)
Theme(1-8)react/lib/util/currency.ts (1)
isFiat(4-6)react/.storybook/default-args.ts (1)
to(1-1)react/lib/util/format.ts (1)
isPropsTrue(66-75)
react/lib/components/PayButton/PayButton.tsx (3)
react/lib/util/currency.ts (1)
isFiat(4-6)react/lib/util/format.ts (1)
amount(5-12)react/lib/util/satoshis.ts (1)
getCurrencyObject(8-41)
🔇 Additional comments (15)
react/lib/tests/util/constants.ts (1)
1-5: LGTM!Clean test utility providing reusable address fixtures for the test suite.
react/lib/components/Widget/WidgetContainer.tsx (1)
37-51: LGTM!Making these props optional with internal fallback state allows the WidgetContainer to be used standalone without requiring parent components to manage all state. This aligns with the PR objective of fixing the standalone widget requirement for
setPaymentId.react/lib/components/Widget/Widget.tsx (5)
184-193: Good fix for loading state tracking.The introduction of
standalonePaymentPendingstate properly tracks when a payment creation is in-flight for standalone widgets. TheisWaitingForPaymentIdlogic correctly distinguishes between child components (which wait for parent-providedpaymentId) and standalone components (which track their own pending state).
602-605: Good fix for undefined amount edge case.Adding
&& thisAmount !== undefinedto the fiat conversion guard ensures that widgets with undefined amounts can still proceed to fetch apaymentIdwithout waiting for a conversion that will never happen.
634-646: Good error handling with finally block.The
try/finallypattern ensuresstandalonePaymentPendingis always reset, preventing the UI from being stuck in a loading state ifcreatePaymentfails. The error is logged for debugging purposes.
957-962: Good UX improvement for loading state.Adding the
qrLoadingguard tohandleQrCodeClickprevents users from copying an incomplete or invalid URL while the component is still loading or waiting for apaymentId.
1281-1281: Consistent loading state handling.Disabling the button while
qrLoadingis true prevents users from attempting to send a payment before the widget is fully initialized.react/lib/tests/components/Widget.test.tsx (3)
42-62: Good test coverage for core paymentId flow.The parameterized tests effectively cover both XEC and BCH currencies, validating that
createPaymentis called with the correct arguments on initial render.
392-455: Comprehensive loading state tests.Good coverage of loading behavior tests that validate:
- QR click interactions are blocked during loading
- Button is disabled during loading
- Both defined and undefined amounts are tested
This aligns well with the PR objective of showing "Loading" while the paymentId is being fetched.
12-24: Good mock structure preserving real implementations.Using
jest.requireActualwith targeted mocks forsetupChronikWebSocket,setupAltpaymentSocket,getAddressBalance,openCashtabPayment, andcreatePaymentallows tests to use real utility functions while controlling external dependencies.react/lib/tests/components/PayButton.test.tsx (2)
1-16: Well-structured mock setup.The mocks for
utilandqrcode.reactare cleanly organized. The QRCodeSVG mock exposingdata-valueis a good pattern for verifying QR content in tests.
22-37: Good use of data-driven test cases.The
TEST_ADDRESSES,CRYPTO_CASES,FIAT_CASES, andALL_CASESconstants provide clear, reusable test fixtures that improve test maintainability and readability.react/lib/components/PayButton/PayButton.tsx (3)
163-165: Good use of refs for tracking state across renders.The refs
lastPaymentAmount,lastOnOpenPaymentId, andhasFiredOnOpenRefappropriately track values that shouldn't trigger re-renders but need to persist across effect executions. This prevents duplicateonOpencalls and unnecessary payment ID regeneration.
247-249: Simplified click handler is cleaner.Wrapping
setDialogOpen(true)inuseCallbackwith an empty dependency array is appropriate since it has no dependencies.
340-350: Effect handles initial amount and currency setup correctly.The effect properly initializes
amountandcurrencyObjwheninitialAmountandcurrencychange. The null check on line 341 correctly handles undefined initial amounts.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
react/lib/tests/components/PayButton.test.tsx (1)
355-381: Rename/clarify “load forever” test title to match expected UX
This matches the previously raised concern: the title reads like a hang is desired. If the intended behavior is “stays in loading state on failure” (or “shows error”), align name + assertions accordingly.
🧹 Nitpick comments (12)
react/lib/tests/components/PayButton.test.tsx (7)
1-22: Avoid usingisFiatimported from the module you’re mocking inside the mock factory
Right nowgetFiatPrice’s mock usesisFiatfrom../../utilwhile../../utilis being mocked, which is easy to accidentally break with future mock edits. Prefer referencingjest.requireActual('../../util').isFiatfrom inside the factory.jest.mock('../../util', () => ({ - ...jest.requireActual('../../util'), + ...jest.requireActual('../../util'), getFiatPrice: jest.fn(async (currency: string, to: string) => { - if (isFiat(currency)) { + const { isFiat } = jest.requireActual('../../util') + if (isFiat(currency)) { return 100 } return null }), setupChronikWebSocket: jest.fn(() => Promise.resolve(undefined)), setupAltpaymentSocket: jest.fn(() => Promise.resolve(undefined)), getAddressBalance: jest.fn(async () => 0), createPayment: jest.fn(async () => '00112233445566778899aabbccddeeff'), }))
28-44: Prefer importingTEST_ADDRESSESfrom the shared test constants
This localTEST_ADDRESSESduplicates the fixture inreact/lib/tests/util/constants.ts(per repo context) and can drift.
46-57: Console error suppression: good idea, but keep the filter as specific as possible
Overridingconsole.errorglobally is fine here, but consider also matching on a known prefix / error type to avoid hiding unrelated regressions that happen to include that substring.
70-92:test.eachtitle interpolation is misleading with object cases
(%s)with{ currency, address }cases typically prints[object Object]. Prefer%oor jest’s$currencyplaceholder style.
163-167: Backdrop click via.MuiBackdrop-rootis brittle
This ties tests to MUI internals/classnames. Prefer closing via an explicit “Close”/“Cancel” button (role-based), or query the backdrop by role if available.Also applies to: 216-220, 413-417, 455-459
200-206: Fiat path assertions depend heavily on mock conversion behavior; consider asserting viagetFiatPricecalls too
Right now it asserts converted amounts (10 → 1) which is fine, but if conversion logic changes slightly (rounding/format), tests may become noisy. Adding an assertion thatgetFiatPricewas queried with expected currency can stabilize intent.Also applies to: 208-215
386-470: QRdata-valueassertions are strong and useful, but hardcode formatting
These assertions will break on benign changes (e.g., amount formatting/encoding). Consider parsing the URI and asserting key params (amount,op_return_raw,paymentId) rather than full-string equality.react/lib/tests/components/Widget.test.tsx (5)
13-31: Avoid usingisFiatimported from the mocked module inside the mock factory
Same maintainability risk as in PayButton tests—preferreal.isFiatfromjest.requireActual('../../util')within the factory.
44-47:cleanup()inbeforeEachis unusual; confirm you’re not double-cleaning
RTL typically auto-cleans after each test; callingcleanup()manually can be fine, but it can also mask lifecycle issues. If you need it, consider documenting why.
50-52:test.eachtitle interpolation:%swith object cases will be noisy
Prefer%oor$label-style placeholders so failures are readable.Also applies to: 70-72, 106-108
153-158: Remove “WIP wait for loading to go away” comments or make the wait explicit
Right now these tests only assertcreatePaymentis not called, but they don’t validate the UI state that explains why (e.g., still loading, disable flag honored). Either assert the expected “loading” UI or drop the WIP note.Also applies to: 174-179
346-384: QR copy test: add assertions for fiat destinations too
For fiat cases, you currently don’t assert the copied URI prefix even thoughtois an eCash address. Consider assertingcopiedcontainsecash:for fiat scenarios as well to prevent regressions in destination formatting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
react/lib/tests/components/PayButton.test.tsx(1 hunks)react/lib/tests/components/Widget.test.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
react/lib/tests/components/PayButton.test.tsx (1)
react/lib/util/api-client.ts (2)
createPayment(93-113)getFiatPrice(69-83)
react/lib/tests/components/Widget.test.tsx (5)
react/lib/util/currency.ts (1)
isFiat(4-6)react/lib/util/types.ts (1)
Currency(140-140)react/lib/tests/util/constants.ts (1)
TEST_ADDRESSES(1-4)react/lib/util/api-client.ts (1)
createPayment(93-113)react/lib/util/format.ts (1)
amount(5-12)
⏰ 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 (1)
react/lib/tests/components/PayButton.test.tsx (1)
236-256:disablePaymentIdcoverage looks good
Nice to see it parameterized across crypto+fiat to prevent regressions.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
react/lib/tests/components/Widget.test.tsx (5)
49-67: Brittle assertion with undefined third parameter.Same issue as flagged in previous reviews: asserting the third parameter is
undefinedcouples the test to the optional parameter passing style.
195-199: Brittle assertion with undefined third parameter.Same issue as flagged in previous reviews.
222-227: Brittle assertion with undefined third parameter.Same issue as flagged in previous reviews. Note that lines 266-268 demonstrate the preferred pattern of checking individual call parameters.
398-403: Critical: Real setTimeout risks slow tests and open handles.Same issue as flagged in previous reviews.
427-429: Critical: Real setTimeout risks slow tests and open handles.Same issue as flagged in previous reviews.
🧹 Nitpick comments (3)
react/lib/tests/components/Widget.test.tsx (3)
46-46: Optional:cleanup()is typically unnecessary in beforeEach.Modern
@testing-library/reactauto-cleans after each test. Explicitly callingcleanup()inbeforeEachis redundant unless you're encountering specific cleanup timing issues.
153-153: Remove or clarify WIP comments.These "WIP wait for loading to go away" comments appear in tests that verify
createPaymentis NOT called. ThewaitForalready handles async behavior appropriately, so these comments suggest either incomplete implementation or can be removed.Also applies to: 174-174
386-449: Tests don't verify eventual loading completion.Both loading tests verify that certain interactions are blocked during loading state, but neither test confirms that the component eventually exits loading state when
createPaymentresolves. This leaves loading behavior partially untested.Consider adding an assertion after the timer resolves (when using fake timers) to verify the loading state clears:
// After advancing fake timers or when promise resolves await waitFor(() => { expect(screen.queryByText(/loading/i)).toBeNull() })
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
react/lib/tests/components/Widget.test.tsx(1 hunks)
⏰ 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 (1)
react/lib/tests/components/Widget.test.tsx (1)
273-344: LGTM! Well-structured editable amount tests.The tests properly verify paymentId regeneration behavior with user input, using appropriate patterns for userEvent and call parameter assertions.
| if (currency === 'XEC') { | ||
| expect(copied).toContain('ecash:') | ||
| } else if (currency === 'BCH') { | ||
| expect(copied).toContain('bitcoincash:') | ||
| } | ||
| } |
There was a problem hiding this comment.
Incomplete assertion coverage for fiat currencies.
The test combines CRYPTO_CASES and FIAT_CASES but only validates URL prefixes for XEC and BCH. For fiat currencies (USD/CAD), no assertion is made about the copied URL content. Consider adding validation for the fiat cases or splitting into separate crypto/fiat tests.
🔎 Suggested fix to add fiat case validation
if (currency === 'XEC') {
expect(copied).toContain('ecash:')
} else if (currency === 'BCH') {
expect(copied).toContain('bitcoincash:')
+ } else {
+ // Fiat cases should still generate a valid crypto URL
+ expect(copied).toMatch(/^(ecash:|bitcoincash:)/)
}📝 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.
| if (currency === 'XEC') { | |
| expect(copied).toContain('ecash:') | |
| } else if (currency === 'BCH') { | |
| expect(copied).toContain('bitcoincash:') | |
| } | |
| } | |
| if (currency === 'XEC') { | |
| expect(copied).toContain('ecash:') | |
| } else if (currency === 'BCH') { | |
| expect(copied).toContain('bitcoincash:') | |
| } else { | |
| // Fiat cases should still generate a valid crypto URL | |
| expect(copied).toMatch(/^(ecash:|bitcoincash:)/) | |
| } | |
| } |
🤖 Prompt for AI Agents
In react/lib/tests/components/Widget.test.tsx around lines 377-382, the test
iterates combined CRYPTO_CASES and FIAT_CASES but only asserts URL prefixes for
XEC and BCH; fiat currencies (USD/CAD) lack assertions. Split the test into
separate crypto and fiat blocks (or branch inside the loop), and for fiat cases
add assertions that the copied string contains the fiat currency code (e.g.,
'USD' or 'CAD') and the expected amount pattern (a numeric value or query param
like 'amount='), or otherwise assert the exact expected fiat URL format used by
the component.
Klakurka
left a comment
Klakurka
left a comment
There was a problem hiding this comment.
Looks good.
I only did a light skim on the code so if you're good with it then we can merge.
Description
Fixes some bugs and adds many tests to hardcode the working behavior.
we also add tests for fixes introduced in other PRs (like #590)
and fixed some other problems, like the standalone widget in the react library need for the props
setPaymentId.The most important part are the tests, that now add more robustness to the overall codebase. From now on, as we fix bugs, we should always cover the fix with tests.
Test plan
Maybe try some of the aforementioned bugs in master, and try to reproduce here (should be fixed)
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Quality / Accessibility
Tests
✏️ Tip: You can customize this high-level summary in your review settings.