Repository navigation
Fix qrcode copy button - #617
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe PR updates the receive screen's copy button to automatically copy the unified BIP21 URI to the clipboard instead of only opening a sheet. A new ChangesBIP21 Copy Button Feature
Regtest Dependency Update
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
🔍 Code Review — #617
🐛 Bug: !copied guard prevents re-copy after first use
src/screens/Wallet/Receive/QrCode.tsx — handleCopyButton
const handleCopyButton = async () => {
if (!prefersReducedMotion) hapticSubtle()
setShowCopySheet(true)
if (qrCodeValue && !copied) { // ← bug here
await copyToClipboard(qrCodeValue)
toast("Copied to clipboard")
setCopied(qrCodeValue)
}
}copied is a string (line 85: useState("")). After the first successful copy, copied becomes a non-empty string. !copied is then always false, so the auto-copy never fires again — even when qrCodeValue has changed (e.g., user edited the amount, or the QR was swapped to a different address type from the sheet).
This partially defeats the purpose of the fix. Scenario:
- User clicks Copy → copies BIP21 URI ✅
- User opens copy sheet, selects "Ark address" →
copied= ark address - User clicks Copy button again →
!copiedisfalse→ nothing copied ❌
Fix: Replace !copied with copied !== qrCodeValue:
if (qrCodeValue && copied !== qrCodeValue) {This correctly skips the copy only when the current value is already on the clipboard, and re-copies when the value has changed.
⚠️ Nit: Unrelated submodule bump
The regtest submodule pointer changed (6333e4b8 → dc23da2c). This is unrelated to the copy button fix and adds noise to the diff. Consider reverting unless intentional.
✅ What looks good
- The new
handleCopyButtoncorrectly combines copy-to-clipboard with opening the sheet, matching issue #573 spec - Haptic feedback is consistent with the existing
handleCopypattern - Test covers the happy path (Copy button → clipboard gets BIP21 URI)
- No protocol-critical code touched
📝 Suggestion for test coverage
Consider adding a test for the re-copy scenario: copy once, change qrCodeValue (via rerender), copy again → assert copyToClipboard was called a second time with the new value. The existing QR-tap test at line 357 does this for the QR button; the Copy button should have parity.
Verdict: Request changes — the !copied guard bug means the fix only works on the first click per session. One-line fix (!copied → copied !== qrCodeValue).
🤖 Reviewed by Arkana
![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
🧹 Nitpick comments (1)
src/test/screens/wallet/receive-qrcode.test.tsx (1)
343-355: ⚡ Quick winConsider adding test cases for edge scenarios.
The current test verifies the happy path (first click), but additional test cases would improve coverage and catch potential regressions:
- Repeated clicks: Verify that clicking the Copy button multiple times behaves correctly (e.g., doesn't copy unnecessarily, or does re-copy if that's the intent)
- Address switching: Verify that after selecting a specific address (which changes
qrCodeValue), clicking the Copy button copies the new value- State management: Verify that the
copiedstate is properly maintained across different user interactionsThese test cases would have caught the conditional logic issue in
handleCopyButtonwhere!copiedprevents re-copying when the QR code value changes.📝 Example test cases
it('clicking Copy button after selecting a specific address copies the new value', async () => { const { rerender } = render(buildTree(tapFixture())) const copyButton = await screen.findByRole('button', { name: 'Copy' }) // First click - copies unified BIP21 await act(async () => { fireEvent.click(copyButton) }) const firstCopy = copyToClipboardMock.mock.calls[0][0] expect(firstCopy).toMatch(/^bitcoin:/) // Simulate selecting a specific address (changes qrCodeValue) await act(async () => { rerender(buildTree(tapFixture({ off: 'ark1different', bd: 'bc1different' }))) }) // Second click - should copy the new value await act(async () => { fireEvent.click(copyButton) }) expect(copyToClipboardMock).toHaveBeenCalledTimes(2) const secondCopy = copyToClipboardMock.mock.calls[1][0] expect(secondCopy).toContain('ark1different') expect(secondCopy).not.toBe(firstCopy) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/screens/wallet/receive-qrcode.test.tsx` around lines 343 - 355, The test only covers the first copy action; add tests that simulate repeated clicks and address switching so the Copy button re-copies when qrCodeValue changes and copied state is preserved correctly; specifically, extend receive-qrcode.test.tsx to (1) click the Copy button multiple times and assert copyToClipboardMock call count increments as expected, and (2) rerender or dispatch the action that changes qrCodeValue (using tapFixture with a different address) then click Copy again and assert the new URI (containing the new ark1 address) was copied and not equal to the previous value; if component logic prevents re-copying, update handleCopyButton (and the copied state logic) to reset or compare the current qrCodeValue so subsequent copies of a changed value are allowed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/screens/Wallet/Receive/QrCode.tsx`:
- Around line 332-340: The copy handler handleCopyButton uses a truthy check on
copied which prevents re-copying when qrCodeValue changes; update the condition
so it copies whenever qrCodeValue exists and is different from copied (e.g.,
compare qrCodeValue !== copied) — keep existing behaviors (hapticSubtle when
!prefersReducedMotion, setShowCopySheet(true)), call await
copyToClipboard(qrCodeValue), call toast('Copied to clipboard'), and then call
setCopied(qrCodeValue) so subsequent clicks with the same value do nothing but a
changed qrCodeValue will re-copy.
---
Nitpick comments:
In `@src/test/screens/wallet/receive-qrcode.test.tsx`:
- Around line 343-355: The test only covers the first copy action; add tests
that simulate repeated clicks and address switching so the Copy button re-copies
when qrCodeValue changes and copied state is preserved correctly; specifically,
extend receive-qrcode.test.tsx to (1) click the Copy button multiple times and
assert copyToClipboardMock call count increments as expected, and (2) rerender
or dispatch the action that changes qrCodeValue (using tapFixture with a
different address) then click Copy again and assert the new URI (containing the
new ark1 address) was copied and not equal to the previous value; if component
logic prevents re-copying, update handleCopyButton (and the copied state logic)
to reset or compare the current qrCodeValue so subsequent copies of a changed
value are allowed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 73b57c1f-8c74-4bf3-83b0-c1d2235549f7
📒 Files selected for processing (3)
regtestsrc/screens/Wallet/Receive/QrCode.tsxsrc/test/screens/wallet/receive-qrcode.test.tsx
Deploying tmp-boltz-upstream-mainnet-arkade-wallet with
|
| Latest commit: |
c5f1b6f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://12da65c1.tmp-boltz-upstream-mainnet-arkade-wallet.pages.dev |
| Branch Preview URL: | https://fix-qrcode-copy-button.tmp-boltz-upstream-mainnet-arkade-wallet.pages.dev |
Deploying wallet-bitcoin with
|
| Latest commit: |
c5f1b6f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d0ba995c.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://fix-qrcode-copy-button.wallet-bitcoin.pages.dev |
Deploying wallet-mutinynet with
|
| Latest commit: |
c5f1b6f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://026b9ad9.arkade-wallet.pages.dev |
| Branch Preview URL: | https://fix-qrcode-copy-button.arkade-wallet.pages.dev |
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
✅ Approved — Fix verified
The !copied → copied !== qrCodeValue fix from my previous review has been correctly applied. The copy button now properly re-fires when qrCodeValue changes (amount edit, address type switch from sheet).
No protocol-critical code touched. LGTM.
🤖 Reviewed by Arkana
Fixes #573
Supersede #579
Summary by CodeRabbit
Bug Fixes
Tests