-
Notifications
You must be signed in to change notification settings - Fork 20
Fix gallery fidget link handling #1618
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: canary
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughA property in Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~4 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
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)
src/fidgets/ui/gallery.tsx (1)
344-397: Consider consistent error display between branches.The error display differs between the linked and non-linked rendering:
- Line 371 (linked): Uses inline
<div style={errorStyle}>- Line 395 (non-linked): Uses
<ErrorWrapper>componentThis creates inconsistent UX. Additionally, when errors occur (indicating no valid image), wrapping the error message in a clickable link could be confusing.
Apply this diff to use consistent error handling:
{settings.selectMediaSource?.name === MediaSourceTypes.WALLET ? ( <div className="absolute bottom-2 right-2 flex h-fit w-fit"> <TooltipProvider> <Tooltip> <TooltipTrigger> <VerifiedNft color={badgeColor} /> </TooltipTrigger> <TooltipContent side="left">Verified Owner</TooltipContent> </Tooltip> </TooltipProvider> </div> ) : null} - {error && <div style={errorStyle}>{error}</div>} + {error && <ErrorWrapper icon="⚠️" message={error} />} </div> </a>Optional: Add URL validation
Consider validating that
redirectUrlis a valid URL to prevent broken links or potential XSS with protocols likejavascript:. This is a nice-to-have improvement.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/fidgets/ui/gallery.tsx(2 hunks)
🔇 Additional comments (3)
src/fidgets/ui/gallery.tsx (3)
176-189: LGTM!The field configuration is clear and well-structured. The display name "Links To" and hint "URL to open when image is clicked" provide good UX guidance.
342-343: LGTM!The trim() logic correctly handles whitespace and makes the link conditional on having a non-empty value. The optional chaining properly handles undefined/null cases.
33-33: No backward compatibility concerns - this is a new file.This commit adds a new
gallery.tsxfile rather than modifying an existing one. TheLinkproperty is part of the freshGalleryFidgetSettingstype definition with no priorRedirectionURLproperty to migrate from. No breaking changes or data migration issues apply here.
Summary
Testing
pagesorappdirectory fornext lint)Codex Task
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.