-
Notifications
You must be signed in to change notification settings - Fork 92
feat: add clickable hyperlinks in transaction memos #2024
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
base: master
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughAdds URL linkification: a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 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
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: 1
🤖 Fix all issues with AI agents
In `@frontend/src/lib/linkify.tsx`:
- Line 21: The protocol check is case-sensitive (url.startsWith("http")) while
the validation regex is case-insensitive, so URLs like "HTTP://example.com"
become "https://HTTP://example.com"; update the logic that computes href (use
the same case-insensitive test as the regex) by either normalizing the input
(e.g., url.toLowerCase().startsWith("http")) or replacing the check with a
case-insensitive regex test (e.g., /^https?:/i.test(url)); apply this change
where href is assigned (the code referencing url and href in linkify.tsx) so
uppercase protocols are detected correctly.
🧹 Nitpick comments (1)
frontend/src/lib/linkify.tsx (1)
3-4: URL regex may produce false positives and has limited validation.The regex matches any word containing a dot followed by 2+ letters (e.g.,
test.co,file.txt), which could linkify unintended text. Consider:
- Requiring
www.prefix for non-http URLs, or- Validating against a known TLD list, or
- At minimum, requiring a longer TLD or path component
5910ebb to
c3634e9
Compare
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/TransactionItem.tsx (1)
167-188: Avoid nested interactive elements insideDialogTrigger.
DialogTriggerrenders a<button>by default, and linkified<a>tags inside it create invalid nested interactive elements (can break keyboard/mouse behavior). ConsiderasChildwith a non‑interactive wrapper and explicit keyboard handling, or move the linkified text outside the trigger.🛠️ Suggested structure
- <DialogTrigger className="p-3 mb-4 hover:bg-muted/50 data-[state=selected]:bg-muted cursor-pointer rounded-md slashed-zero transaction sensitive"> - <div - className={cn( - "flex gap-3", - tx.state === "pending" && "animate-pulse" - )} - > + <DialogTrigger asChild> + <div + className="p-3 mb-4 hover:bg-muted/50 data-[state=selected]:bg-muted cursor-pointer rounded-md slashed-zero transaction sensitive" + role="button" + tabIndex={0} + onKeyDown={(e) => { + if (e.key === "Enter" || e.key === " ") { + e.preventDefault(); + e.currentTarget.click(); + } + }} + > + <div + className={cn( + "flex gap-3", + tx.state === "pending" && "animate-pulse" + )} + > {typeStateIcon} ... - </div> - </DialogTrigger> + </div> + </div> + </DialogTrigger>
|
Hi, thanks for the PR. Is there a more standard way to do it then adding this custom linkify code? |
Fixes: #1515
Makes URLs in transaction memos clickable. Links open in a new tab.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.