-
Notifications
You must be signed in to change notification settings - Fork 16
Better cashtab-connect error handling #573
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughopenCashtabPayment in Changes
Sequence DiagramsequenceDiagram
participant Caller
participant openCashtabPayment
participant CashtabExt
participant WebFallback
Caller->>openCashtabPayment: call(bip21Url, fallbackUrl)
Note over openCashtabPayment: compute webUrl = fallbackUrl || default
alt extension available
openCashtabPayment->>CashtabExt: await sendBip21(bip21Url)
CashtabExt-->>openCashtabPayment: success
else address denied
Note over openCashtabPayment: user denied -> return
else extension unavailable or timeout
openCashtabPayment->>WebFallback: open(webUrl)
openCashtabPayment-->>Caller: return
else unknown error
openCashtabPayment->>WebFallback: open(webUrl)
openCashtabPayment-->>Caller: return
end
openCashtabPayment-->>Caller: done
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
react/lib/util/cashtab.ts (1)
87-94: Approve with optional consistency suggestion.The await on
sendBip21is correct and enables proper error handling. The logic flow is sound.For consistency with the error handlers below (which use early returns), consider adding an explicit return after line 93:
} else { window.open(webUrl, '_blank'); + return; }This makes the control flow more explicit and consistent, though the current code is functionally correct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
react/lib/util/cashtab.ts(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 (3)
react/lib/util/cashtab.ts (3)
1-6: LGTM! Imports are now properly utilized.All imported error types are now used in the
openCashtabPaymentfunction's error handling logic and re-exported for consumers. This resolves the build warnings about unused imports mentioned in the PR objectives.
84-86: Excellent fix: webUrl computed upfront ensures reliable fallback.Computing the web URL outside the try/catch block is the key improvement that fixes the fallback issue. This ensures the fallback URL is always available in all error paths, regardless of when or where errors occur.
95-110: Excellent error handling improvements!The error handling hierarchy is well-structured and comprehensive:
- User rejection → Respects user choice by doing nothing
- Extension unavailable or timeout → Falls back to web (fixes the core issue)
- Unknown errors → Falls back to web (defensive, ensures users can complete transactions)
The explicit instanceof checks for each error type are type-safe, and the early returns prevent fall-through. This implementation directly addresses the PR objective of fixing the fallback behavior when Brave extensions interfere.
Description
I was running into issues w/ my Brave extensions and noticed that the fallback to using cashtab.com wasn't working. I've introduced some more robust error handling. During build, it will no longer show the warnings that we're importing but not using some of the cashtab-connect error handling.
Test plan
Make sure cashtab extension + .com open as appropriate when clicking "Send with Cashtab".
Summary by CodeRabbit