Improvements to Visual Embed for base.app Links #1571
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Jhonattan2121
changed the title
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new BaseAppEmbed component for base.app URLs that fetches and displays OpenGraph media with optional HLS video playback. Enhances ZoraEmbed with similar OG data fetching and video support. Refactors the OpenGraph API endpoint to use regex-based metadata extraction instead of JSDOM, adds video support, and improves URL handling. Extends Content Security Policy headers for Zora and Tenor media sources. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ 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: 5
🧹 Nitpick comments (1)
src/fidgets/farcaster/components/Embeds/BaseAppEmbed.tsx (1)
16-16: Replace Portuguese comment with English.For consistency and maintainability in an English codebase, comments should be in English.
Apply this diff:
- {/* Apenas o iframe do post, sem preview superior, altura maior */} + {/* Iframe container without top preview */}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/fidgets/farcaster/components/Embeds/BaseAppEmbed.tsx(1 hunks)src/fidgets/farcaster/components/Embeds/index.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: Agent
🔇 Additional comments (1)
src/fidgets/farcaster/components/Embeds/BaseAppEmbed.tsx (1)
25-25: The sandbox configuration is appropriate for this use case.
BaseAppEmbedis used exclusively forhttps://base.appURLs (as shown insrc/fidgets/farcaster/components/Embeds/index.tsx:33), which is an external, third-party domain. The security concern aboutallow-scriptscombined withallow-same-originapplies specifically to same-origin iframes, where the embedded document could programmatically remove sandbox protections and access the parent's origin data.In this case, since
base.appis a separate, cross-origin domain, theallow-same-originflag does not grant the iframe access to the parent page's cookies, storage, or DOM. The sandbox appropriately isolates the external content while allowing the embedded app to function (allow-scripts) and open popups (allow-popups).No changes required.
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for embedding base.app domain links within the Farcaster feed by introducing a dedicated BaseAppEmbed component and integrating it into the existing embed detection system.
Key Changes:
- Created a new
BaseAppEmbedcomponent that renders base.app content via iframe with responsive styling - Added URL detection logic in the embed system to identify and route base.app links to the new component
- Implemented dynamic component loading using React.lazy and Suspense for the BaseAppEmbed
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
src/fidgets/farcaster/components/Embeds/BaseAppEmbed.tsx |
New component that renders base.app links in an iframe with fixed height styling and sandbox security attributes |
src/fidgets/farcaster/components/Embeds/index.tsx |
Added conditional logic to detect base.app URLs and render them with the new BaseAppEmbed component using lazy loading |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Embed customizado para base.app | ||
| if (url.startsWith("https://base.app")) { | ||
| const BaseAppEmbed = React.lazy(() => import("./BaseAppEmbed")); |
There was a problem hiding this comment.
The React.lazy call is being executed on every render inside the conditional block. This creates a new component instance on each render, causing unnecessary re-mounts and breaking React's reconciliation.
The lazy component should be defined outside of the renderEmbedForUrl function at the module level, similar to other imports like ImageEmbed and VideoEmbed. Move this line to the top of the file:
const BaseAppEmbed = React.lazy(() => import("./BaseAppEmbed"));| } | ||
| if (!url) return null; | ||
|
|
||
| // Embed customizado para base.app |
There was a problem hiding this comment.
Comment contains non-English text. For consistency with the rest of the codebase (which uses English comments), this should be updated to English: "Custom embed for base.app"
| // Embed customizado para base.app | |
| // Custom embed for base.app |
| className="w-full h-full" | ||
| style={{ border: "none", minHeight: 700, width: '500px' }} |
There was a problem hiding this comment.
The responsive width classes conflict with the inline width: '500px' style on line 23. The container has w-full max-w-full sm:max-w-lg but the iframe has a fixed 500px width, which will break the responsive design. Either use Tailwind's responsive width classes consistently (e.g., w-full sm:w-[500px] on the iframe) or remove the conflicting inline style.
| className="w-full h-full" | |
| style={{ border: "none", minHeight: 700, width: '500px' }} | |
| className="w-full sm:w-[500px] h-full" | |
| style={{ border: "none", minHeight: 700 }} |
| className="w-full h-full" | ||
| style={{ border: "none", minHeight: 700, width: '500px' }} | ||
| allowFullScreen | ||
| sandbox="allow-scripts allow-same-origin allow-popups" |
There was a problem hiding this comment.
The sandbox attribute should include allow-forms if the embedded base.app content contains interactive forms that users need to submit. Without this, form submissions within the iframe will be blocked. Review the base.app content requirements and add allow-forms to the sandbox if needed: sandbox="allow-scripts allow-same-origin allow-popups allow-forms"
| sandbox="allow-scripts allow-same-origin allow-popups" | |
| sandbox="allow-scripts allow-same-origin allow-popups allow-forms" |
| if (!url) return null; | ||
|
|
||
| // Embed customizado para base.app | ||
| if (url.startsWith("https://base.app")) { |
There was a problem hiding this comment.
The URL detection logic using url.startsWith("https://base.app") will miss valid base.app URLs with different protocols or subdomains. Consider using a more robust pattern like url.includes("base.app") (consistent with how zora.co and paragraph.xyz are detected on lines 73-76) or checking for both http and https protocols to avoid missing valid URLs.
| if (url.startsWith("https://base.app")) { | |
| if (url.includes("base.app")) { |
| return ( | ||
| <div | ||
| key={key} | ||
| className="baseapp-embed-card w-full max-w-full sm:max-w-lg mx-auto rounded-2xl border border-gray-200 bg-white shadow-xl overflow-hidden flex flex-col items-center p-0" |
There was a problem hiding this comment.
The maximum width constraint sm:max-w-lg is inconsistent with other embed components in the codebase. Other embeds (ZoraEmbed, OpenGraphEmbed, SmartFrameEmbed, NounsBuildEmbed) use max-w-2xl for consistency. Consider changing sm:max-w-lg to max-w-2xl to maintain visual consistency across the application.
| className="baseapp-embed-card w-full max-w-full sm:max-w-lg mx-auto rounded-2xl border border-gray-200 bg-white shadow-xl overflow-hidden flex flex-col items-center p-0" | |
| className="baseapp-embed-card w-full max-w-full max-w-2xl mx-auto rounded-2xl border border-gray-200 bg-white shadow-xl overflow-hidden flex flex-col items-center p-0" |
| key?: string; | ||
| } | ||
|
|
||
| const BaseAppEmbed: React.FC<BaseAppEmbedProps> = ({ url, key }) => { | ||
| return ( | ||
| <div | ||
| key={key} |
There was a problem hiding this comment.
The key prop is being used incorrectly here. React's key prop is automatically handled when passed from the parent component and should not be manually assigned to DOM elements. Since key shouldn't be in the props interface (as noted in another comment), this line should be removed.
| key?: string; | |
| } | |
| const BaseAppEmbed: React.FC<BaseAppEmbedProps> = ({ url, key }) => { | |
| return ( | |
| <div | |
| key={key} | |
| } | |
| } | |
| const BaseAppEmbed: React.FC<BaseAppEmbedProps> = ({ url }) => { | |
| return ( | |
| <div |
| style={{ minHeight: 640 }} | ||
| > | ||
| {/* Apenas o iframe do post, sem preview superior, altura maior */} | ||
| <div className="w-full flex flex-col items-center px-0 py-8"> | ||
| <div className="w-full rounded-lg overflow-hidden border bg-gray-50" style={{ height: 700 }}> | ||
| <iframe | ||
| src={url} | ||
| title="Base App Preview" | ||
| className="w-full h-full" | ||
| style={{ border: "none", minHeight: 700, width: '500px' }} |
There was a problem hiding this comment.
Multiple conflicting height values are defined: minHeight: 640 on the container (line 14), height: 700 on the wrapper div (line 18), and minHeight: 700 on the iframe (line 23). This creates confusion and may cause layout issues. Consider consolidating to a single, well-defined height strategy, possibly using a constant like MAX_EMBED_HEIGHT as seen in other embed components (e.g., VideoEmbed.tsx).
| className="baseapp-embed-card w-full max-w-full sm:max-w-lg mx-auto rounded-2xl border border-gray-200 bg-white shadow-xl overflow-hidden flex flex-col items-center p-0" | ||
| style={{ minHeight: 640 }} | ||
| > | ||
| {/* Apenas o iframe do post, sem preview superior, altura maior */} |
There was a problem hiding this comment.
Comment contains non-English text. For consistency with the rest of the codebase (which uses English comments), this should be updated to English: "Only the iframe of the post, without top preview, larger height"
| {/* Apenas o iframe do post, sem preview superior, altura maior */} | |
| {/* Only the iframe of the post, without top preview, larger height */} |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/fidgets/farcaster/components/Embeds/index.tsx (2)
2-2: Consider grouping the lazy import with other component imports.While the module-level placement is correct, the lazy import is positioned between the React import and regular component imports. For better organization, consider moving it after the other import statements or grouping it with similar dynamic imports.
Apply this diff to improve import organization:
import React from "react"; -const BaseAppEmbed = React.lazy(() => import("./BaseAppEmbed")); import EmbededCast from "./EmbededCast"; import OnchainEmbed from "./OnchainEmbed"; import TweetEmbed from "./TweetEmbed"; import NounsBuildEmbed from "./NounsBuildEmbed"; import ParagraphXyzEmbed from "./ParagraphXyzEmbed"; import VideoEmbed from "./VideoEmbed"; import ImageEmbed from "./ImageEmbed"; import SmartFrameEmbed from "./SmartFrameEmbed"; import ZoraEmbed from "./ZoraEmbed"; import { isImageUrl, isVideoUrl } from "@/common/lib/utils/urls"; import CreateCastImage from "./createCastImage"; + +// Lazy-loaded components +const BaseAppEmbed = React.lazy(() => import("./BaseAppEmbed"));
34-34: Tighten the URL detection to match only base.app domain.The current check
url.includes("base.app")will match unintended URLs likehttps://notbase.app,https://example.com/base.app/path, orhttps://base.app.malicious.com. To match only the base.app domain and its subdomains, use a more precise pattern.Apply this diff:
// Custom embed for base.app - if (url.includes("base.app")) { + if (url.match(/^https?:\/\/([a-z0-9-]+\.)?base\.app\//i)) { return ( <React.Suspense fallback={null}> <BaseAppEmbed url={url} key={key} />Or, if you only want to support HTTPS:
// Custom embed for base.app - if (url.includes("base.app")) { + if (url.startsWith("https://base.app/") || url.match(/^https:\/\/[a-z0-9-]+\.base\.app\//i)) { return ( <React.Suspense fallback={null}> <BaseAppEmbed url={url} key={key} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/fidgets/farcaster/components/Embeds/BaseAppEmbed.tsx(1 hunks)src/fidgets/farcaster/components/Embeds/index.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/fidgets/farcaster/components/Embeds/BaseAppEmbed.tsx
|
@Jhonattan2121 leave the code comments in english and address those minor coderabbit comments |
|
+1 vlad's comment. also, please improve the design of the 'view on zora' component. Try a single line of grey text that says 'View on Zora' at the bottom of the cast, with no special container, blue CTA, or base:hex string |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/fidgets/farcaster/components/Embeds/ZoraEmbed.tsx (1)
18-18: Consider removing the redundant aria-label.The
aria-labelduplicates the visible link text. Since "View on Zora" already clearly describes the link's purpose, the aria-label provides no additional value and can be removed for cleaner code.Apply this diff to remove the redundant attribute:
<a href={tradeUrl} target="_blank" rel="noopener noreferrer" className="text-gray-500 text-sm" - aria-label="View on Zora" > View on Zora </a>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/fidgets/farcaster/components/Embeds/ZoraEmbed.tsx(1 hunks)src/fidgets/farcaster/components/Embeds/index.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/fidgets/farcaster/components/Embeds/index.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/fidgets/farcaster/components/Embeds/ZoraEmbed.tsx (1)
src/fidgets/farcaster/components/Embeds/zoraUtils.ts (1)
parseZoraUrl(1-75)
🔇 Additional comments (1)
src/fidgets/farcaster/components/Embeds/ZoraEmbed.tsx (1)
1-23: LGTM! Excellent simplification that aligns with design requirements.The refactor successfully transforms the complex embed card into a simple, minimal link as requested in the PR comments. The implementation correctly:
- Removes unused state management and OpenGraph fetching logic
- Uses secure link attributes (
rel="noopener noreferrer")- Applies grey text styling (
text-gray-500) matching the design spec- Handles parsing failures with a proper fallback to the original URL
|
@Jhonattan2121 the only difference I'm noticing between this and prod is that the zora/base link looks different (but not really any better). shouldn't there be an image or video displayed in the cast? also, the 'View on Zora' button should either have no container, or if you want to give it a container, should have padding so the text isn't touching the edges of the container. the button should also have a hover state. this branch: 
prod: 
|
…tyling - Add OpenGraph metadata fetching to display image/video previews - Add proper padding to buttons (px-4 py-2) so text doesn't touch edges - Add hover state to buttons (hover:bg-gray-500 with transition) - Display title, description, and site name from OpenGraph data - Improve visual consistency between BaseAppEmbed and ZoraEmbed components
…onal design - Add video playback support using HLS player for Zora and BaseApp embeds - Update OpenGraph API to fetch video metadata (og:video) and filter video URLs from image field - Add CSP rules for Zora domains and video CDNs (zora.co, media.tenor.com) - Redesign Zora embed with clickable card, badge indicator, and tooltip - Remove generic button, make entire Zora card clickable with hover effects - Add Zora badge in top-right corner (always visible) with hover animation - Implement tooltip on hover showing content preview - Add gradient preview fallback when OpenGraph data unavailable - Improve error handling and timeout management (8s timeout) - Fix video URL detection to prevent .m3u8 files being treated as images - Better visual hierarchy and professional styling - Apply same video/image validation to BaseAppEmbed for consistency
… compatibility - Replace JSDOM dependency with regex-based HTML parsing for better serverless compatibility - Improve regex patterns to handle various HTML formats (quotes, spacing, attribute order) - Add HTML entity decoding for proper content extraction - Add relative to absolute URL conversion for images and videos - Simplify ZoraEmbed error handling and remove unnecessary logs - Fix OpenGraph metadata extraction for Zora embeds on Vercel
I made some fixes for issues that were happening and improved the design to make it look better.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
src/fidgets/farcaster/components/Embeds/ZoraEmbed.tsx (1)
247-257: Video URL detection logic duplicated.This is the same logic as in
route.ts(lines 166-169) andBaseAppEmbed.tsx(lines 154-157). Extract to a shared utility function.src/fidgets/farcaster/components/Embeds/BaseAppEmbed.tsx (3)
172-175: Avoid direct DOM mutation for error handling.Same issue as in
ZoraEmbed.tsx: settingstyle.displaydirectly mutates the DOM. Use React state to conditionally render the image instead.
152-162: Video URL detection logic duplicated.This is the same logic duplicated in
route.tsandZoraEmbed.tsx. Consider extracting to a shared utility.
15-22:OpenGraphDatainterface duplicated from ZoraEmbed.Already flagged in ZoraEmbed review - extract to a shared types file.
🧹 Nitpick comments (6)
next.config.mjs (1)
15-15: Minor: Duplicate CSP entry for*.b-cdn.net.The
https://*.b-cdn.netsource appears twice in themedia-srcdirective. While harmless, consider removing the duplicate for cleaner configuration.src/app/api/opengraph/route.ts (2)
88-96: Consider extracting HTML entity decoding into a helper function.The same entity decoding logic is duplicated in
getMetaContent(lines 88-96) andgetTitleTag(lines 107-115). Extract to a shared helper for maintainability.🔎 Suggested refactor
+ const decodeHtmlEntities = (content: string): string => { + return content + .replace(/&/g, '&') + .replace(/</g, '<') + .replace(/>/g, '>') + .replace(/"/g, '"') + .replace(/'/g, "'") + .replace(/'/g, "'"); + }; + const getMetaContent = (property: string): string | null => { // ... pattern matching ... for (const pattern of patterns) { const match = html.match(pattern); if (match && match[1]) { - let content = match[1].trim(); - content = content.replace(/&/g, '&'); - // ... rest of replacements - return content; + return decodeHtmlEntities(match[1].trim()); } } return null; };Also applies to: 107-115
161-179: Video URL detection logic is duplicated across files.This video extension check duplicates similar logic in
ZoraEmbed.tsx(lines 248-252) andBaseAppEmbed.tsx(lines 154-157). Consider using the existingisVideoUrlutility fromsrc/common/lib/utils/urls.tsor creating a shared helper.src/fidgets/farcaster/components/Embeds/ZoraEmbed.tsx (2)
22-30: Extract sharedOpenGraphDatainterface.This interface is duplicated in
BaseAppEmbed.tsx(lines 15-22). Consider extracting it to a shared types file to maintain consistency and reduce duplication.🔎 Suggested location
Create a shared types file at
src/fidgets/farcaster/components/Embeds/types.ts:export interface OpenGraphData { title?: string; description?: string; image?: string; video?: string; siteName?: string; url?: string; error?: string; }
267-270: Avoid direct DOM mutation in React.Setting
style.displaydirectly mutates the DOM outside React's control. Consider using state to conditionally render the image instead.🔎 Alternative approach using state
+ const [imageError, setImageError] = useState(false); // In the render: - onError={(e) => { - e.currentTarget.style.display = 'none'; - }} + onError={() => setImageError(true)} // Wrap image in conditional: - <Image ... /> + {!imageError && <Image ... />}src/fidgets/farcaster/components/Embeds/BaseAppEmbed.tsx (1)
201-214: Remove redundanthrefattribute or simplify the click handler.The anchor tag has both
href={url}and anonClickthat callse.preventDefault(). Since the default action is always prevented, thehrefattribute serves no functional purpose. Either removehrefand use a<button>styled as a link, or simplify by removing theonClickhandler and relying on the native anchor behavior.🔎 Option 1: Use button instead
- <a - href={url} - target="_blank" - rel="noopener noreferrer" - onClick={(e) => { - e.preventDefault(); - e.stopPropagation(); - openWindow(url); - }} + <button + onClick={(e) => { + e.stopPropagation(); + openWindow(url); + }} className="inline-flex items-center rounded-sm bg-gray-600 px-4 py-2 text-sm font-semibold text-white shadow-sm hover:bg-gray-500 focus-visible:outline focus-visible:outline-2 focus-visible:outline-offset-2 focus-visible:outline-gray-600 transition-colors" > {ogData?.title ? `View ${ogData.title.length > 25 ? ogData.title.substring(0, 22) + '...' : ogData.title}` : "Open in Base"} <ArrowTopRightOnSquareIcon className="w-4 h-4 ml-2" /> - </a> + </button>
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
next.config.mjs(2 hunks)src/app/api/opengraph/route.ts(4 hunks)src/fidgets/farcaster/components/Embeds/BaseAppEmbed.tsx(1 hunks)src/fidgets/farcaster/components/Embeds/ZoraEmbed.tsx(2 hunks)src/fidgets/farcaster/components/Embeds/index.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/app/api/opengraph/route.ts (1)
src/common/lib/utils/urls.ts (1)
isVideoUrl(39-51)
src/fidgets/farcaster/components/Embeds/BaseAppEmbed.tsx (2)
src/common/lib/utils/urls.ts (1)
isVideoUrl(39-51)src/common/lib/utils/navigation.ts (1)
openWindow(1-5)
src/fidgets/farcaster/components/Embeds/ZoraEmbed.tsx (4)
src/fidgets/farcaster/components/Embeds/zoraUtils.ts (1)
parseZoraUrl(1-75)src/common/lib/utils/navigation.ts (1)
openWindow(1-5)src/common/components/atoms/tooltip.tsx (4)
TooltipProvider(28-28)Tooltip(28-28)TooltipTrigger(28-28)TooltipContent(28-28)src/common/lib/utils/urls.ts (1)
isVideoUrl(39-51)
🪛 ast-grep (0.40.0)
src/app/api/opengraph/route.ts
[warning] 79-79: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(<meta[^>]*(?:property|name)\\s*=\\s*["']${escapedProperty}["'][^>]*content\\s*=\\s*["']([^"']+)["'], 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 81-81: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(<meta[^>]*content\\s*=\\s*["']([^"']+)["'][^>]*(?:property|name)\\s*=\\s*["']${escapedProperty}["'], 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (6)
next.config.mjs (1)
58-61: CSP additions look correct for the new embed functionality.The added sources (
zora.co,*.zora.co,media.tenor.com,*.tenor.com) align with the new ZoraEmbed and BaseAppEmbed components that fetch media content from these domains.src/fidgets/farcaster/components/Embeds/index.tsx (2)
15-16: Good: Lazy component correctly defined at module level.Moving
React.lazy()to the module level prevents recreating the component on every render, which was flagged in past reviews.
36-43: URL pattern is robust and implementation is clean.The regex pattern
/^https?:\/\/([a-z0-9-]+\.)?base\.app\//icorrectly handles:
- Both
http://andhttps://protocols- Optional subdomains (e.g.,
app.base.app)- Case-insensitive matching
This addresses the past review comment about
url.startsWithbeing too restrictive.src/app/api/opengraph/route.ts (1)
199-211: Good error handling with graceful fallback.The catch block logs the error and returns minimal fallback data with an
errorfield, allowing consumers to handle failures gracefully without breaking the UI.src/fidgets/farcaster/components/Embeds/ZoraEmbed.tsx (1)
213-297: Well-structured embed with proper tooltip integration and responsive design.The component handles multiple states well (loading, no data, with data) and uses tooltips appropriately. The video-first rendering priority and aspect ratio handling are solid implementations.
src/fidgets/farcaster/components/Embeds/BaseAppEmbed.tsx (1)
26-128: Overall component structure is clean and well-organized.The component correctly:
- Handles loading state with a skeleton UI
- Uses dynamic import for HLS player to avoid SSR issues
- Implements video playback controls with mute-on-first-interaction pattern
- Calculates aspect ratio for responsive video sizing
- Addresses all past review comments (removed
keyprop, fixed styling conflicts)
| const getMetaContent = (property: string): string | null => { | ||
| const element = document.querySelector(`meta[property="${property}"], meta[name="${property}"]`); | ||
| return element?.getAttribute("content") || null; | ||
| // Escape special regex characters in property name | ||
| const escapedProperty = property.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); | ||
| // Match meta tags with property/name and content in any order, handling spaces and quotes | ||
| const patterns = [ | ||
| // property="..." content="..." or property='...' content='...' | ||
| new RegExp(`<meta[^>]*(?:property|name)\\s*=\\s*["']${escapedProperty}["'][^>]*content\\s*=\\s*["']([^"']+)["']`, 'i'), | ||
| // content="..." property="..." or content='...' property='...' | ||
| new RegExp(`<meta[^>]*content\\s*=\\s*["']([^"']+)["'][^>]*(?:property|name)\\s*=\\s*["']${escapedProperty}["']`, 'i'), | ||
| ]; | ||
|
|
||
| for (const pattern of patterns) { | ||
| const match = html.match(pattern); | ||
| if (match && match[1]) { | ||
| // Decode HTML entities | ||
| let content = match[1].trim(); | ||
| content = content.replace(/&/g, '&'); | ||
| content = content.replace(/</g, '<'); | ||
| content = content.replace(/>/g, '>'); | ||
| content = content.replace(/"/g, '"'); | ||
| content = content.replace(/'/g, "'"); | ||
| content = content.replace(/'/g, "'"); | ||
| return content; | ||
| } | ||
| } | ||
| return null; | ||
| }; |
There was a problem hiding this comment.
Add a length limit on HTML responses to mitigate potential ReDoS attacks.
While the regex patterns are used with hardcoded property names, the untrusted HTML content itself can be crafted to trigger catastrophic backtracking in patterns with nested quantifiers like [^>]*. In Node.js, you cannot directly timeout regex operations, so input length validation before regex execution is the primary defense. Set a reasonable size limit (e.g., 5–10 MB) on the HTML response before parsing to prevent performance degradation from malformed or adversarial content.
🧰 Tools
🪛 ast-grep (0.40.0)
[warning] 79-79: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(<meta[^>]*(?:property|name)\\s*=\\s*["']${escapedProperty}["'][^>]*content\\s*=\\s*["']([^"']+)["'], 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 81-81: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(<meta[^>]*content\\s*=\\s*["']([^"']+)["'][^>]*(?:property|name)\\s*=\\s*["']${escapedProperty}["'], 'i')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 Prompt for AI Agents
In src/app/api/opengraph/route.ts around lines 74 to 100, the getMetaContent
function runs regex against untrusted HTML which can trigger ReDoS via long
input; before performing any regex, check the html string length and bail out if
it exceeds a reasonable threshold (e.g., 5_000_000 to 10_000_000 characters) by
returning null or throwing a controlled error; implement this early length guard
so the regexes never run on oversized responses and include a concise comment
explaining the limit and reason.
| useEffect(() => { | ||
| const fetchOGData = async () => { | ||
| try { | ||
| setIsLoading(true); | ||
| const response = await fetch( | ||
| `/api/opengraph?url=${encodeURIComponent(url)}` | ||
| ); | ||
| if (response.ok) { | ||
| const data = await response.json(); | ||
| setOgData(data); | ||
| } | ||
| } catch (err) { | ||
| console.error("Error fetching OpenGraph data:", err); | ||
| } finally { | ||
| setIsLoading(false); | ||
| } | ||
| }; | ||
| fetchOGData(); | ||
| }, [url]); |
There was a problem hiding this comment.
Add timeout and cleanup for the fetch request.
Unlike ZoraEmbed.tsx which uses an AbortController with an 8-second timeout, this component has no timeout protection. A slow or hanging request will leave the loading state indefinitely. Add timeout handling for consistency and reliability.
🔎 Apply this diff to add timeout handling
useEffect(() => {
+ const controller = new AbortController();
+ const timeoutId = setTimeout(() => controller.abort(), 8000);
+
const fetchOGData = async () => {
try {
setIsLoading(true);
const response = await fetch(
- `/api/opengraph?url=${encodeURIComponent(url)}`
+ `/api/opengraph?url=${encodeURIComponent(url)}`,
+ { signal: controller.signal }
);
if (response.ok) {
const data = await response.json();
setOgData(data);
}
} catch (err) {
- console.error("Error fetching OpenGraph data:", err);
+ if (err instanceof Error && err.name !== 'AbortError') {
+ console.error("Error fetching OpenGraph data:", err);
+ }
} finally {
+ clearTimeout(timeoutId);
setIsLoading(false);
}
};
fetchOGData();
+
+ return () => {
+ controller.abort();
+ clearTimeout(timeoutId);
+ };
}, [url]);🤖 Prompt for AI Agents
In src/fidgets/farcaster/components/Embeds/BaseAppEmbed.tsx around lines 34 to
52, the fetch for OpenGraph data has no timeout or cleanup which can leave
isLoading true on slow/hanging requests; add an AbortController and a timer
(match ZoraEmbed's 8s timeout) and pass controller.signal to fetch, schedule
controller.abort() via setTimeout, and clear the timeout and call
controller.abort() in the useEffect cleanup so the request is cancelled when the
component unmounts or url changes; also catch abort errors gracefully (don’t
treat them as real errors) and ensure setIsLoading(false) still runs in finally.
| useEffect(() => { | ||
| let cancelled = false; | ||
| const fetchOG = async () => { | ||
| if (!parsed) { | ||
| setLoading(false); | ||
| return; | ||
| const fetchOGData = async () => { | ||
| try { | ||
| setIsLoading(true); | ||
| const controller = new AbortController(); | ||
| const timeoutId = setTimeout(() => controller.abort(), 8000); // 8 second timeout | ||
|
|
||
| try { | ||
| const response = await fetch( | ||
| `/api/opengraph?url=${encodeURIComponent(tradeUrl)}`, | ||
| { | ||
| signal: controller.signal, | ||
| } | ||
| ); | ||
|
|
||
| if (response.ok) { | ||
| const data = await response.json(); | ||
| // Accept data if we have any meaningful content | ||
| if (data.title || data.image || data.video || data.description) { | ||
| setOgData(data); | ||
| } | ||
| } | ||
| } finally { | ||
| clearTimeout(timeoutId); | ||
| } | ||
| } catch (err) { | ||
| // Silently fail - we'll show fallback UI | ||
| } finally { | ||
| setIsLoading(false); | ||
| } | ||
| }; | ||
| fetchOGData(); | ||
| }, [tradeUrl]); |
There was a problem hiding this comment.
Missing cleanup for AbortController on unmount.
The AbortController created in the effect will continue timing out even if the component unmounts. Add cleanup to abort the request on unmount to prevent state updates on unmounted components.
🔎 Apply this diff to add cleanup
useEffect(() => {
+ const controller = new AbortController();
+
const fetchOGData = async () => {
try {
setIsLoading(true);
- const controller = new AbortController();
- const timeoutId = setTimeout(() => controller.abort(), 8000); // 8 second timeout
+ const timeoutId = setTimeout(() => controller.abort(), 8000);
try {
const response = await fetch(
`/api/opengraph?url=${encodeURIComponent(tradeUrl)}`,
{
signal: controller.signal,
}
);
// ... rest of fetch logic
} finally {
clearTimeout(timeoutId);
}
} catch (err) {
- // Silently fail - we'll show fallback UI
+ if (err instanceof Error && err.name === 'AbortError') {
+ return; // Ignore abort errors
+ }
} finally {
setIsLoading(false);
}
};
fetchOGData();
+
+ return () => controller.abort();
}, [tradeUrl]);📝 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.
| useEffect(() => { | |
| let cancelled = false; | |
| const fetchOG = async () => { | |
| if (!parsed) { | |
| setLoading(false); | |
| return; | |
| const fetchOGData = async () => { | |
| try { | |
| setIsLoading(true); | |
| const controller = new AbortController(); | |
| const timeoutId = setTimeout(() => controller.abort(), 8000); // 8 second timeout | |
| try { | |
| const response = await fetch( | |
| `/api/opengraph?url=${encodeURIComponent(tradeUrl)}`, | |
| { | |
| signal: controller.signal, | |
| } | |
| ); | |
| if (response.ok) { | |
| const data = await response.json(); | |
| // Accept data if we have any meaningful content | |
| if (data.title || data.image || data.video || data.description) { | |
| setOgData(data); | |
| } | |
| } | |
| } finally { | |
| clearTimeout(timeoutId); | |
| } | |
| } catch (err) { | |
| // Silently fail - we'll show fallback UI | |
| } finally { | |
| setIsLoading(false); | |
| } | |
| }; | |
| fetchOGData(); | |
| }, [tradeUrl]); | |
| useEffect(() => { | |
| const controller = new AbortController(); | |
| const fetchOGData = async () => { | |
| try { | |
| setIsLoading(true); | |
| const timeoutId = setTimeout(() => controller.abort(), 8000); | |
| try { | |
| const response = await fetch( | |
| `/api/opengraph?url=${encodeURIComponent(tradeUrl)}`, | |
| { | |
| signal: controller.signal, | |
| } | |
| ); | |
| if (response.ok) { | |
| const data = await response.json(); | |
| // Accept data if we have any meaningful content | |
| if (data.title || data.image || data.video || data.description) { | |
| setOgData(data); | |
| } | |
| } | |
| } finally { | |
| clearTimeout(timeoutId); | |
| } | |
| } catch (err) { | |
| if (err instanceof Error && err.name === 'AbortError') { | |
| return; // Ignore abort errors | |
| } | |
| } finally { | |
| setIsLoading(false); | |
| } | |
| }; | |
| fetchOGData(); | |
| return () => controller.abort(); | |
| }, [tradeUrl]); |
🤖 Prompt for AI Agents
In src/fidgets/farcaster/components/Embeds/ZoraEmbed.tsx around lines 44 to 76,
the AbortController created inside the effect can continue after unmount; move
controller and timeoutId to the effect scope (outside fetchOGData) so you can
return a cleanup function that calls controller.abort() and clears the timeout.
Specifically, create the AbortController and timeoutId before calling
fetchOGData, pass controller.signal into fetch, and return () => {
controller.abort(); clearTimeout(timeoutId); } from the useEffect to ensure the
request is aborted and timers cleared on unmount.
|
looks great from testing perspective, nice work @Jhonattan2121! |
Changes in
BaseAppEmbed.tsxChanges in
index.tsxBaseAppEmbedcomponent only for these links, without affecting other embed types.React.lazyandSuspensefor dynamic component loading, optimizing performance.Summary by CodeRabbit
New Features
Performance
✏️ Tip: You can customize this high-level summary in your review settings.