Enhances embed functionality for channels/playlists#3385
Conversation
📝 WalkthroughWalkthroughAdds embed-aware routing, UI, and metadata: new DYNAMIC_ROUTES_FIRST config to control static vs dynamic routing order; expanded embed wrapper with collections/markdown, PropTypes, playlist resolution, OG/Farcaster embed scripts; multiple components updated for embed mode and iframe/oEmbed handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant KoaServer as Server
participant Router as RouterMiddleware
participant Static as StaticMiddleware
Client->>Server: GET /some/path
Server->>Router: incoming request
alt config.DYNAMIC_ROUTES_FIRST == true
Router->>Router: attempt dynamic route (render HTML)
Router->>Static: call next() for /public/* or /sw.js
Static-->>Router: asset served or not
Router-->>Server: return HTML or 404
else DYNAMIC_ROUTES_FIRST == false
Router->>Static: try static asset first
Static-->>Router: served or not-found
alt static served
Router-->>Server: static asset response
else
Router->>Router: build HTML response
Router-->>Server: HTML
end
end
Server-->>Client: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
ui/redux/actions/arwallet.js (2)
289-296: Inconsistent error handling: dispatch vs. return value.The dispatch uses a hardcoded error message while the return statement uses
error?.message || error, which could pass a non-string error object to the caller. Consider ensuring consistency and that the returned error is always a string.🔎 Proposed fix for consistent error handling
} catch (error) { console.error(error); dispatch({ type: AR_TIP_STATUS_ERROR, data: { claimId: claimId, error: 'error: arweave transaction failed' }, }); - return { error: error?.message || error }; + const errorMsg = typeof error === 'string' ? error : error?.message || 'arweave transaction failed'; + return { error: errorMsg }; }
20-20: Update @permaweb/aoconnect to the latest version. Version 0.0.63 is outdated; the latest version is 0.0.93, released 8 days ago. The package demonstrates healthy version release cadence and project activity. Update to 0.0.93 to benefit from recent improvements and fixes.ui/component/socialShare/view.jsx (1)
364-382: PassuriAccessKeyto channel and collection embed components.The default embed case (lines 374-381) correctly passes
uriAccessKeyto generate signed embed URLs for unlisted content. However, channels and collections (lines 367-372) do not receive this parameter. SincedoFetchUriAccessKey()fetches signatures for any unlisted claim owned by the user—regardless of type—unlisted channels and collections should also passuriAccessKeyto theirEmbedTextAreacomponents to ensure valid embed URLs.ui/page/embedWrapper/index.js (1)
108-116: Remove duplicate try-catch block.The second try-catch block (lines 112-114) is identical to the first and serves no purpose. The existing comment on line 115 also questions this.
🔎 Proposed fix
if (isOldPermanentUriFormat) { try { return buildURI({ claimName, claimId }); } catch (error) {} - try { - return buildURI({ claimName, claimId }); - } catch (error) {} - // ^-------- why twice? }
🧹 Nitpick comments (15)
ui/component/viewers/videoViewer/internal/videojs.jsx (1)
373-378: Prefer the router'spathnameoverwindow.location.pathnamefor consistency.The
pathnameis already available from theuseHistoryhook (destructured at line 218). Using it directly would be more consistent with React Router patterns and avoid redundant window access.🔎 Proposed refactor
- // Reuse the saved player DOM only when appropriate. Reusing across embed routes - // has been observed to cause lockups; keep reuse enabled for popout. - const path = (typeof window !== 'undefined' && window.location && window.location.pathname) || ''; - const isPopoutRoute = path.startsWith('/$/popout'); - const isEmbedRoute = path.startsWith('/$/embed/'); + // Reuse the saved player DOM only when appropriate. Reusing across embed routes + // has been observed to cause lockups; keep reuse enabled for popout. + const isPopoutRoute = pathname.startsWith('/$/popout'); + const isEmbedRoute = pathname.startsWith('/$/embed/'); let canUseOldPlayer = window.oldSavedDiv && vjsParent && (!isEmbedRoute || isPopoutRoute);ui/component/viewers/videoViewer/view.jsx (1)
644-670: Nice feature addition!The reactions overlay for embedded videos is well-implemented with solid conditional logic. The overlay correctly appears when hovering during playback or whenever the video is paused, providing good UX.
The accessibility label and pointer-events handling are properly configured.
Optional: Consider extracting inline styles to a CSS class
The 20+ inline style properties (lines 646-665) could be extracted to a CSS class for improved maintainability and reusability. This would make the JSX cleaner and easier to read.
Example:
.video-reactions-overlay { position: absolute; right: 8px; bottom: 48px; z-index: 7; display: flex; gap: 8px; pointer-events: auto; background: rgba(0, 0, 0, 0.45); border-radius: 8px; padding: 6px 8px; align-items: center; backdrop-filter: blur(1.5px); --color-link: var(--color-text); --color-fire: var(--color-text); --color-slime: var(--color-text); }Then simplify the JSX:
- <div - style={{ - position: 'absolute', - ... - }} - aria-label={__('Reactions')} - > + <div className="video-reactions-overlay" aria-label={__('Reactions')}>This is purely a maintainability improvement and can be deferred.
ui/hocs/withStreamClaimRender/index.js (1)
38-69: LGTM! Well-crafted optimization for reducing re-renders.The custom equality function correctly handles the frequently-changing
playingUriobject by comparing specific fields (uri,sourceId,collection.collectionId) rather than relying on reference equality. The implementation is sound:
- Safe null handling with
|| {}pattern- Proper short-circuit evaluation for nested property access
- Key count check prevents false positives from different key sets
- Falls back to shallow comparison for other props
Optional: Consider adding unit tests
Given the complexity of this equality function, adding unit tests would help ensure correctness across edge cases:
- Both
playingUrinull/undefined- Nested
collectionobject null/undefined- Same values but different object references
- Different key sets with same count
- All fields identical (should return true)
ui/redux/actions/arwallet.js (1)
159-159: Consider applying the improved error handling pattern fromdoArConnect.For consistency with the error handling improvements in
doArConnect(lines 120-124), consider extracting the error message using the same safe pattern. This ensures consistent behavior when errors might be strings or objects.🔎 Suggested consistency improvement
- dispatch({ type: ARCONNECT_FAILURE, data: { error: e?.message || 'Error connecting to Arconnect.' } }); + const msg = typeof e === 'string' ? e : e && e.message ? e.message : undefined; + dispatch({ type: ARCONNECT_FAILURE, data: { error: msg || 'Error connecting to Arconnect.' } });Apply the same pattern to line 178 in
doArDisconnect.ui/hocs/withStreamClaimRender/view.jsx (1)
268-274:fileGetOptionsnot in dependency array may cause stale closure.The
fileGetOptionsobject (defined at line 142) includesuriAccessKeywhich can change based on URL params. If the signature params change while this effect is pending, the stale options may be used.Consider adding
fileGetOptionsto the deps or memoizing it, or extract only the needed values directly fromurlParamsinside the effect.web/index.js (1)
33-51: Verify actual route patterns don't intercept static file responses.Both conditional routing orders are technically sound since router.routes() falls back to serving static files when routes don't match. However, before merging, confirm:
- No dynamic routes match static file extensions (e.g.,
/**.js,/**.css,/**.html) whenDYNAMIC_ROUTES_FIRSTis enabled- Both modes preserve correct cache headers—verify the
maxage: 3600000is applied regardless of middleware order- Test that non-existent routes in both modes return 404, not partial responses
These are production safeguards, not blockers—the middleware fallthrough pattern is sound.
ui/component/header/view.jsx (1)
120-122: Minor: Add null check forpathnamefor consistency with other files.Other files in this PR (e.g.,
ui/page/claim/view.jsx,ui/page/collection/view.jsx) usepathname && pathname.startsWith('/$/embed')to guard against potential undefined values. Whilepathnamefromhistory.locationis typically always defined, adding the guard would maintain consistency across the codebase.🔎 Suggested change
- const isEmbedPath = pathname.startsWith('/$/embed'); + const isEmbedPath = pathname && pathname.startsWith('/$/embed');ui/page/claim/view.jsx (1)
74-78: Consider memoizingChannelPageWrapperLocalto avoid recreation on every render.Unlike
ClaimRenderWrapperwhich is memoized, this inline component is recreated on every render of the parent. While React is efficient at handling this, memoization would be more consistent with the pattern used forClaimRenderWrapperand could avoid unnecessary re-renders of children.🔎 Suggested change
+ const ChannelPageWrapperLocal = React.useMemo( + () => + ({ children }: { children: any }) => ( + <Page className="channelPage-wrapper" noFooter fullWidthPage noSideNavigation={isEmbedPath}> + {children} + </Page> + ), + [isEmbedPath] + ); if (isChannel) { const urlParams = new URLSearchParams(search); const editing = urlParams.get(CHANNEL_PAGE.QUERIES.VIEW) === CHANNEL_PAGE.VIEWS.EDIT; if (editing) { // ... editing wrapper } - const ChannelPageWrapperLocal = ({ children }: { children: any }) => ( - <Page className="channelPage-wrapper" noFooter fullWidthPage noSideNavigation={isEmbedPath}> - {children} - </Page> - ); - return ( <ClaimPageComponentui/page/embedWrapper/internal/embedClaimComponent/view.jsx (3)
92-107: Consider using CSS classes instead of inline styles for consistency.The inline
styleattributes work but deviate from the component's existing pattern of using CSS classes. This could make styling harder to maintain and override.🔎 Example using class-based styling
{channelClaimId && ( - <div style={{ marginTop: '16px' }}> - <h2 style={{ fontSize: '1.2rem', marginBottom: '12px', color: 'var(--color-text)' }}> + <div className="embed-channel-content"> + <h2 className="embed-channel-content__title"> {__('Recent Content')} </h2>Then add corresponding SCSS rules to the embed stylesheet.
225-226: Consider distinguishing between loading and error states more explicitly.Using
undefinedfor loading andnullfor error works but relies on subtle type differences. Consider using an explicit state object or separate state variables for clarity.🔎 Alternative approach with explicit states
const [state, setState] = React.useState({ loading: true, content: null, error: false }); // Then in render: if (state.loading) return <Spinner text={__('Loading post...')} />; if (state.error) return <div className="help--notice">{__("Sorry, we couldn't load this post.")}</div>;
248-263: PropTypes defined afterexport defaultis valid but unconventional.This placement works in JavaScript but is less common. Consider moving the PropTypes definition before the export for better readability, or keeping it as-is if this matches the codebase conventions.
ui/page/embedWrapper/index.js (1)
157-172: Empty catch block silently swallows errors.The empty catch block on line 170 will hide any parsing errors, making debugging difficult. Consider logging the error or at least adding a comment explaining why errors are intentionally ignored.
🔎 Proposed fix
} catch (e) {} + // Errors are intentionally ignored; fallback to empty params is safe return matchedPath;Or with debug logging:
- } catch (e) {} + } catch (e) { + console.debug('buildMatchFromPath: failed to parse pathname', pathname, e); + } return matchedPath;web/src/html.js (1)
167-172: Consider extracting the ready signal script to reduce duplication.The minified ready signal script here duplicates logic found in the expanded version (lines 397-444). Extracting this to a shared helper would improve maintainability and ensure both versions stay in sync.
ui/page/embedWrapper/view.jsx (2)
88-146: Consider extracting shared helpers to reduce duplication.
getUriFromMatchandbuildMatchWithHashare duplicated between this file andindex.js. While there are minor differences, extracting these to a shared utility (e.g.,util/embedUri.js) would reduce maintenance burden and ensure consistency.
79-84: Redundant PropTypes with Flow types.PropTypes are defined alongside Flow type annotations. While not harmful, this is redundant since Flow provides static type checking. If runtime validation is desired for embed contexts, this is acceptable; otherwise, PropTypes could be removed to reduce code.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
.gitignore(1 hunks)config.js(1 hunks)package.json(1 hunks)ui/component/app/view.jsx(1 hunks)ui/component/claimMenuList/view.jsx(1 hunks)ui/component/claimTilesDiscover/view.jsx(1 hunks)ui/component/embedTextArea/view.jsx(2 hunks)ui/component/fileActions/view.jsx(1 hunks)ui/component/header/view.jsx(4 hunks)ui/component/page/view.jsx(1 hunks)ui/component/router/view.jsx(4 hunks)ui/component/sideNavigation/view.jsx(1 hunks)ui/component/socialShare/view.jsx(2 hunks)ui/component/subscribeButton/view.jsx(2 hunks)ui/component/viewers/videoViewer/internal/videojs.jsx(1 hunks)ui/component/viewers/videoViewer/view.jsx(4 hunks)ui/hocs/withResolvedClaimRender/helper.jsx(1 hunks)ui/hocs/withStreamClaimRender/index.js(2 hunks)ui/hocs/withStreamClaimRender/view.jsx(3 hunks)ui/page/claim/internal/claimPageComponent/view.jsx(4 hunks)ui/page/claim/view.jsx(2 hunks)ui/page/collection/index.js(1 hunks)ui/page/collection/view.jsx(4 hunks)ui/page/embedWrapper/index.js(6 hunks)ui/page/embedWrapper/internal/embedClaimComponent/index.js(1 hunks)ui/page/embedWrapper/internal/embedClaimComponent/view.jsx(7 hunks)ui/page/embedWrapper/view.jsx(1 hunks)ui/redux/actions/arwallet.js(6 hunks)ui/scss/component/_claim-preview.scss(1 hunks)ui/scss/component/_embed-player.scss(2 hunks)ui/util/buildHomepage.js(3 hunks)web/index.js(1 hunks)web/src/farcaster.js(1 hunks)web/src/html.js(8 hunks)web/src/routes.js(2 hunks)web/src/tools/check-farcaster.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (21)
ui/component/subscribeButton/view.jsx (7)
ui/component/claimMenuList/view.jsx (1)
isEmbed(130-130)ui/component/sideNavigation/view.jsx (1)
isEmbed(37-37)ui/page/claim/internal/claimPageComponent/view.jsx (1)
isEmbed(69-69)web/src/tools/check-farcaster.js (2)
isEmbed(123-123)e(150-150)ui/component/app/view.jsx (1)
uri(204-204)ui/page/embedWrapper/index.js (1)
uri(20-20)ui/page/embedWrapper/view.jsx (2)
uri(37-37)uri(47-47)
ui/component/claimMenuList/view.jsx (1)
ui/component/claimMenuList/index.js (1)
lastUsedCollections(58-58)
ui/component/fileActions/view.jsx (4)
ui/component/claimMenuList/view.jsx (2)
isCollectionClaim(155-155)isChannel(137-137)ui/component/fileActions/index.js (1)
claim(31-31)ui/component/fileDownloadLink/index.js (1)
claim(16-16)ui/component/socialShare/view.jsx (1)
isChannel(128-128)
ui/component/viewers/videoViewer/internal/videojs.jsx (2)
ui/component/app/view.jsx (1)
path(187-187)ui/hocs/withResolvedClaimRender/helper.jsx (1)
path(34-34)
web/src/tools/check-farcaster.js (4)
ui/component/claimMenuList/view.jsx (1)
isEmbed(130-130)ui/component/sideNavigation/view.jsx (1)
isEmbed(37-37)ui/component/subscribeButton/view.jsx (1)
isEmbed(47-47)ui/page/claim/internal/claimPageComponent/view.jsx (1)
isEmbed(69-69)
ui/page/collection/view.jsx (5)
ui/component/page/view.jsx (1)
useHistory(62-64)ui/page/embedWrapper/view.jsx (1)
useHistory(31-34)ui/page/collection/internal/collectionPublishForm/view.jsx (1)
useHistory(77-80)ui/component/header/view.jsx (1)
isEmbedPath(120-120)ui/page/claim/view.jsx (1)
isEmbedPath(27-27)
ui/component/viewers/videoViewer/view.jsx (2)
ui/component/fileReactions/view.jsx (1)
FileReactions(28-77)ui/component/viewers/videoViewer/index.js (1)
uri(49-49)
ui/page/embedWrapper/internal/embedClaimComponent/index.js (3)
ui/page/embedWrapper/index.js (8)
select(14-72)state(15-15)state(17-17)claim(25-25)uri(20-20)channelClaimId(30-30)isCollection(53-53)collectionId(54-54)ui/hocs/withResolvedClaimRender/view.jsx (1)
withResolvedClaimRender(42-271)ui/page/embedWrapper/internal/embedClaimComponent/view.jsx (1)
EmbedClaimComponent(34-183)
web/src/routes.js (2)
config.js (1)
config(5-120)web/index.js (1)
config(1-1)
ui/hocs/withStreamClaimRender/view.jsx (4)
ui/component/header/view.jsx (1)
urlParams(106-106)ui/page/embedWrapper/internal/embedClaimComponent/view.jsx (1)
urlParams(55-55)ui/component/viewers/videoViewer/index.js (1)
urlParams(47-47)ui/util/url.js (1)
COLLECTIONS_CONSTS(9-9)
ui/component/header/view.jsx (2)
ui/page/claim/view.jsx (1)
isEmbedPath(27-27)ui/page/collection/view.jsx (1)
isEmbedPath(56-56)
ui/page/embedWrapper/index.js (3)
ui/page/embedWrapper/internal/embedClaimComponent/index.js (3)
select(16-33)channelClaimId(21-21)isCollection(22-22)ui/page/embedWrapper/view.jsx (3)
match(90-90)uri(37-37)uri(47-47)ui/redux/selectors/claims.js (17)
channelClaimId(964-964)channelClaimId(1217-1217)claim(128-128)claim(180-180)claim(208-208)claim(221-221)claim(227-227)claim(237-237)claim(446-446)uri(322-322)uri(363-363)uri(879-879)claimId(179-179)claimId(954-954)claimId(1050-1050)claimId(1225-1225)claimId(1236-1236)
ui/component/embedTextArea/view.jsx (2)
ui/component/fileActions/view.jsx (1)
isChannel(94-94)ui/page/claim/internal/claimPageComponent/view.jsx (1)
isCollection(68-68)
ui/component/app/view.jsx (4)
web/src/html.js (2)
embedPath(537-537)PAGES(26-26)ui/util/url.js (1)
PAGES(7-7)ui/component/nag/view.jsx (1)
Nag(24-89)web/src/lbryURI.js (1)
__(4-4)
ui/util/buildHomepage.js (4)
ui/page/discover/view.jsx (2)
channelIds(65-65)urlParams(51-51)ui/component/viewers/videoViewer/internal/videojs.jsx (1)
urlParams(234-234)ui/component/claimTilesDiscover/index.js (1)
urlParams(101-101)ui/util/url.js (3)
urlParams(106-106)urlParams(245-245)urlParams(269-269)
ui/page/embedWrapper/internal/embedClaimComponent/view.jsx (4)
ui/page/embedWrapper/view.jsx (3)
EmbedClaimComponent(14-16)uri(37-37)uri(47-47)ui/hocs/withStreamClaimRender/index.js (1)
props(72-72)ui/page/embedWrapper/internal/embedClaimComponent/index.js (1)
isCollection(22-22)ui/page/collection/index.js (1)
collectionId(25-25)
ui/component/socialShare/view.jsx (4)
ui/component/embedTextArea/view.jsx (3)
isChannel(26-26)EmbedTextArea(21-96)isCollection(27-27)ui/page/embedWrapper/index.js (2)
claim(25-25)isCollection(53-53)ui/page/embedWrapper/internal/embedClaimComponent/index.js (2)
claim(18-18)isCollection(22-22)ui/component/socialShare/index.js (1)
claim(20-20)
ui/page/collection/index.js (20)
ui/component/claimMenuList/view.jsx (1)
props(84-128)ui/page/collection/view.jsx (1)
props(36-50)ui/component/claimList/view.jsx (1)
props(81-130)ui/page/collection/internal/collectionActions/index.js (1)
props(16-16)ui/modal/modalRemoveCollection/view.jsx (1)
props(23-24)ui/page/collection/internal/collectionHeader/internal/collectionTitle/view.jsx (1)
props(27-27)ui/page/collection/internal/collectionPublishForm/index.js (1)
props(29-29)ui/hocs/withCollectionItems/index.js (1)
props(15-15)ui/component/collectionItemsList/index.js (1)
props(8-8)ui/modal/modalRemoveCollection/index.js (1)
props(13-13)ui/page/collection/internal/collectionHeader/internal/collectionTitle/index.js (1)
props(13-13)ui/component/fileActions/view.jsx (1)
collectionId(101-101)ui/page/embedWrapper/index.js (1)
collectionId(54-54)ui/component/claimMenuList/index.js (1)
collectionId(51-51)ui/redux/reducers/collections.js (4)
collectionId(78-78)collectionId(91-91)collectionId(182-182)collectionId(225-225)ui/component/playlistCard/index.js (1)
collectionId(27-27)ui/page/collection/internal/collectionHeader/index.js (1)
collectionId(10-10)ui/page/playlists/internal/collectionsListMine/internal/collectionPreview/index.js (1)
collectionId(28-28)ui/component/collectionMenuList/index.js (1)
collectionId(14-14)ui/component/claimListDiscover/view.jsx (1)
Claim(90-90)
web/index.js (2)
config.js (1)
config(5-120)web/src/routes.js (2)
config(2-2)router(18-18)
ui/page/claim/view.jsx (3)
ui/component/header/view.jsx (1)
isEmbedPath(120-120)ui/page/collection/view.jsx (1)
isEmbedPath(56-56)ui/page/claim/internal/claimPageComponent/view.jsx (1)
ClaimPageComponent(39-191)
ui/hocs/withResolvedClaimRender/helper.jsx (2)
ui/page/embedWrapper/index.js (1)
uri(20-20)ui/component/viewers/videoViewer/internal/videojs.jsx (1)
path(375-375)
🪛 Biome (2.1.2)
ui/page/claim/internal/claimPageComponent/view.jsx
[error] 166-166: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
ui/redux/actions/arwallet.js
[error] 74-74: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 74-74: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 370-370: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 370-370: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
ui/page/embedWrapper/view.jsx
[error] 19-25: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 27-27: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
ui/page/claim/view.jsx
[error] 31-31: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 61-61: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 74-74: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
ui/hocs/withResolvedClaimRender/helper.jsx
[error] 39-39: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 39-39: expected , but instead found ?
Remove ?
(parse)
[error] 39-39: expected , but instead found UriAccessKey
Remove UriAccessKey
(parse)
🔇 Additional comments (52)
.gitignore (1)
44-45: LGTM!Excluding
.claude/settings.local.jsonfrom version control is a reasonable best practice—local IDE/editor configurations should not be committed. This prevents unintended exposure of local development environment settings.ui/component/subscribeButton/view.jsx (2)
93-108: LGTM! Correctly disables subscription action in embed mode.The ternary pattern cleanly prevents subscription handling when
isEmbedis true, with thehrefon line 92 providing the appropriate redirect fallback for embedded contexts.
127-142: LGTM! Consistent with the first handler.The implementation correctly mirrors the pattern from lines 93-108, ensuring subscription actions are disabled in embed contexts while the
hrefon line 126 handles the redirect.ui/component/viewers/videoViewer/view.jsx (2)
26-26: LGTM!The FileReactions import is correctly added for the new embedded video reactions overlay feature.
538-538: LGTM!The hover state management is cleanly implemented using standard React patterns. The mouse event handlers correctly toggle the state on the parent container.
Also applies to: 553-554
ui/hocs/withStreamClaimRender/index.js (1)
117-118: LGTM! Correct integration of the custom equality function.The
areStatePropsEqualoption is properly passed toconnectas the fourth argument. This is the correct option name for comparing the result ofmapStateToProps(theselectfunction), which aligns with the optimization goal.ui/redux/actions/arwallet.js (2)
120-124: Improved error handling with safer message extraction.The updated logic safely extracts error messages and prevents potential TypeErrors when checking for cancellation. The fallback to a default message ensures the error payload is always meaningful.
362-365: Formatting improvements enhance readability.The multi-line formatting for dispatch calls and function definitions improves code readability without altering functionality.
Also applies to: 370-372, 469-482
ui/component/claimMenuList/view.jsx (1)
344-344: Good defensive guard for lastUsedCollections.The added check prevents runtime errors when
lastUsedCollectionsis null, undefined, or not an array before attempting to map over it. This aligns well with the Flow type annotation?Array<any>and ensures the component gracefully handles missing or malformed data.web/src/farcaster.js (1)
15-15: LGTM! Farcaster Mini App now points to embed homepage.This change aligns with the PR's objective to configure the homepage embed and point the Mini App action to the homepage embed path. The update ensures Farcaster frame interactions use the embed-optimized route.
ui/component/app/view.jsx (1)
228-242: Excellent UX improvement for upload notifications.This global nag effectively alerts users about active uploads when they navigate away from the upload page, reducing the risk of accidentally interrupting uploads. The implementation correctly:
- Excludes embed contexts where this warning isn't applicable
- Provides a direct CTA to return to the uploads page
- Complements the existing
beforeunloadhandler (lines 299-316)ui/page/collection/index.js (1)
23-25: Enhanced flexibility for collectionId resolution.This change allows the CollectionPage to receive
collectionIdeither as a direct prop or from route parameters, with a sensible fallback. This flexibility supports multiple usage patterns, including embed scenarios where the component might be instantiated with props directly rather than through routing.ui/component/embedTextArea/view.jsx (1)
26-28: Smart conditional rendering of autoplay toggle.Hiding the autoplay toggle for channels, collections, and newest-type embeds makes semantic sense since these content types represent lists or grids rather than single playable items. The implementation cleanly derives the conditions from the claim type and correctly applies them to the UI rendering.
Also applies to: 72-82
ui/util/buildHomepage.js (1)
46-49: Improved flexibility for homepage category ID handling.The changes enhance the homepage builder to:
- Accept IDs from either
cat.channelIdsorcat.ids, providing flexibility for different content types- Only process and set URL parameters when IDs actually exist, avoiding empty parameter pollution
- Allow
channelIdsto be undefined in the returned options, which is cleaner than forcing an empty arrayThis aligns well with the PR's goal of supporting diverse embed types (channels, playlists, etc.).
Also applies to: 75-78, 116-116
ui/component/fileActions/view.jsx (1)
93-99: Appropriately expanded web shareability criteria.The enhanced logic correctly recognizes that channels and collections are inherently web-shareable content types, as are text-based render modes (markdown, documents). The multi-condition approach maintains backward compatibility for cost-based shareability while extending support to content types that should always be shareable regardless of cost.
package.json (1)
30-30: Correct propagation of DYNAMIC_ROUTES_FIRST flag.The script now ensures the
DYNAMIC_ROUTES_FIRSTflag is available throughout the development lifecycle—both during the initial compilation and the concurrent watch/server processes. This is necessary for the routing configuration changes introduced in the PR to function consistently during development.ui/hocs/withResolvedClaimRender/helper.jsx (2)
30-36: Good defensive guards for embed routes.The early return for missing
uriand the embed path check prevent unnecessary URL mutations. However, the emptycatchblock silently swallows any exception. Consider at minimum logging in development mode or adding a comment explaining why errors are intentionally ignored.
38-53: Conditional URL update prevents unnecessary address bar churn.The logic correctly compares existing signature parameters before updating, which aligns with the PR objective to fix locking issues. The empty
.catch(() => {})is acceptable here since failures in fetching access keys shouldn't break the UI.ui/component/socialShare/view.jsx (1)
334-345: LGTM - Embed button now available for all shareable content types.The simplified
webShareablecondition properly allows the embed functionality for channels and collections, aligning with the PR objectives.ui/hocs/withStreamClaimRender/view.jsx (2)
132-135: LGTM - Collection ID fallback for alternate URL param.The fallback to
lidURL parameter provides flexibility for playlist-style embed URLs, which aligns with the enhanced embed functionality.
226-227: LGTM - Embed homepage detection extended.The
isHomecheck now correctly includes the embed homepage path, consistent with the routing changes in the PR.ui/page/embedWrapper/internal/embedClaimComponent/index.js (2)
16-32: Well-structured Redux selector expansion.The enhanced
selectfunction properly derives channel and collection information from the claim. The comment on line 20-21 explaining the channel ID resolution approach is helpful.One consideration:
collectionUrlson line 29 usescollectionIdfrom props, but ifcollectionIdis undefined/null for non-collection claims, the ternary correctly returnsnull.
35-40: LGTM - Clean Redux action binding.The
performobject properly binds the required action creators, and the component composition withwithResolvedClaimRenderfollows the established pattern in the codebase.ui/component/claimTilesDiscover/view.jsx (1)
119-122: LGTM - Pin injection extended to embed homepage.The condition correctly treats the embed homepage (
/$/embed/home) the same as the main homepage for pin URL injection, which is consistent with other embed-aware changes in this PR.Note: Direct
window.location.pathnameaccess in render works here since the component would remount/re-render on navigation anyway.ui/component/page/view.jsx (1)
104-104: LGTM! Clean prop forwarding for embed context.The
hideSidebarToggleprop correctly binds tonoSideNavigation, enabling the Header to hide the sidebar toggle in embed contexts. This aligns with the embed-aware navigation pattern introduced across the PR.ui/scss/component/_claim-preview.scss (1)
1541-1553: LGTM! More specific selector for embed context.The updated selector correctly scopes the tile styles to file-viewer contexts within embeds, preventing unintended style application to other embed scenarios.
ui/page/collection/view.jsx (2)
54-56: LGTM! Consistent embed path detection.The embed path detection follows the same pattern used in other components (e.g.,
ui/page/claim/view.jsx,ui/component/header/view.jsx), ensuring consistent behavior across the application.
101-101: LGTM! Comprehensive embed-aware layout.The
noSideNavigationprop is correctly applied to all Page component renders (geo-restricted, empty collection, and main view), ensuring consistent sidebar behavior in embed contexts.Also applies to: 124-124, 146-146
web/src/tools/check-farcaster.js (1)
123-124: LGTM! Robust URL pattern matching.The regex pattern correctly handles both encoded (
%24) and unencoded ($) dollar signs in embed URLs, making the validation more reliable for URLs that may have been encoded by browsers or external systems.ui/component/router/view.jsx (4)
165-165: LGTM! Type definition enhancement.Adding the
replacemethod to the history type definition is correct—it's a standard History API method.
261-261: LGTM! Proper dependency tracking for responsive routes.Adding
isSmallScreenandisMediumScreento the dependency array ensures category routes regenerate when screen size changes, which is correct sinceGetLinksDatauses these values.
360-363: LGTM! Cleaner URL decoding without extra history entries.Using
history.replaceinstead of<Redirect>is the correct approach here—it updates the URL without adding an extra entry to the browser history, providing a better user experience.
478-478: LGTM! Embed home route addition.The new embed home route correctly maps
/$/embed/hometo the HomePage component, supporting the embed functionality for the homepage.ui/scss/component/_embed-player.scss (2)
20-27: LGTM! Page-mode styling for embed wrapper.The new
.embed__wrapper--pageclass appropriately switches from the default full-viewport embed layout to a block layout suitable for page-like content, with proper background and text colors for readability.
47-52: LGTM! Improved text readability in embeds.Explicitly setting
color: var(--color-text)for documents and markdown ensures readable text contrast, particularly important when embedded content might inherit inverse colors from the embed wrapper's default dark background.config.js (1)
118-119: Add documentation for the DYNAMIC_ROUTES_FIRST flag's behavior and production defaults.The flag defaults to
false(static routes first), which is safe for production. However, this feature gate lacks deployment documentation. Add notes on:
- What the flag actually changes: middleware execution order (dynamic vs static route matching)
- When and why to enable it (currently only enabled in dev:web-server)
- Any caching or meta tag injection differences between modes, particularly for SEO-sensitive routes like embeds and homepages
ui/component/header/view.jsx (2)
250-262: LGTM!The sidebar toggle visibility is correctly conditioned on both
authHeaderand the newhideSidebarToggleprop. This provides clean control for embed contexts while preserving existing behavior.
264-276: LGTM!The home button navigation now correctly handles both the embed home path (
/$/embed/home) and the standard home path (/), triggering scroll-to-top and claim search clearing appropriately for both contexts.ui/component/sideNavigation/view.jsx (1)
36-53: LGTM!The
getHomeButtonfunction properly handles embed-aware navigation with appropriate SSR safety checks (typeof window !== 'undefined'). The dynamichomePathselection and conditional scroll behavior are well-implemented.web/src/routes.js (2)
105-110: LGTM for dynamic-first mode logic.The delegation to
next()for static assets (/public/and/sw.js) in dynamic-first mode correctly allows the static middleware to handle these requests.
102-123: The current implementation is intentional and correct. This was specifically addressed in commit a38cf06 ("Serve missing internal files as 404 instead of soft-404 page") to fix a claim collision issue unique to.jsfiles. Non-existent/public/*.jsrequests were being processed bygetHtml()and incorrectly resolved as claims, causing HTML responses and bad cache.Other static asset types (CSS, images, fonts) don't cause this collision because they don't follow claim URL patterns. The
.js-only detection was a deliberate, conservative approach—the commit message even questions whether the detection should be broader. The design correctly protects against the specific problem without over-detecting.Likely an incorrect or invalid review comment.
ui/page/claim/internal/claimPageComponent/view.jsx (2)
69-69: LGTM!The
isEmbedflag derivation is consistent with the pattern used across the codebase and correctly guards embed-specific behavior.
85-88: LGTM!The early return correctly prevents canonical URL rewriting when in embed mode, preserving the embed URL structure.
ui/page/claim/view.jsx (2)
26-27: LGTM!The
isEmbedPathderivation fromuseLocationis consistent with the pattern used across the PR.
33-42: LGTM!The
ClaimRenderWrappercorrectly addsnoSideNavigationonly when both in embed context and rendering a markdown post, appropriately memoized withisEmbedPathin the dependency array.ui/page/embedWrapper/internal/embedClaimComponent/view.jsx (3)
66-71: LGTM!The collection items fetch effect is correctly placed before conditional returns and has appropriate guards for
isCollection,collectionId, anddoFetchItemsInCollection.
206-223: LGTM!The markdown content fetching logic has proper cancellation handling to prevent state updates after unmount, good error handling, and correctly uses
credentials: 'omit'for cross-origin safety.
190-193: PropType definition forstreamClaimis correct.
streamClaimis typed asPropTypes.func, which is accurate. The HOCwithStreamClaimRendercreates this as aReact.useCallbackfunction and passes it to wrapped components. The Flow type invideoClaimRenderexplicitly declares it as() => void, confirming it's a callback function, not claim data.Likely an incorrect or invalid review comment.
ui/page/embedWrapper/index.js (1)
83-94: Good defensive checks added.The guard clauses for missing
claimNameand the special case forhomeroute prevent unnecessary resolution attempts. This improves robustness.web/src/html.js (1)
94-99: Clean implementation of fcActionUrl override.The fcActionUrl parameter is well-integrated into the existing metadata generation flow, with appropriate fallback to the computed
urlwhen not provided.ui/page/embedWrapper/view.jsx (2)
1-1: Static analysis hints are false positives.The Biome errors about "TypeScript only syntax" are incorrect—this file uses Flow (note the
// @flowannotation on line 1), which has similar type annotation syntax. Flow is properly configured for this codebase.
51-76: Clean implementation of lazy-loaded conditional rendering.The Suspense-based lazy loading with appropriate fallback spinner is well-implemented. The conditional rendering logic based on
collectionIdandisPageLikeis clear and the EmbedContext properly provides state to child components.
| // Detect collections from playlist-style URIs even if claim isn't resolved as a collection | ||
| let detectedCollectionId = null; | ||
| if (uri && typeof uri === 'string' && uri.toLowerCase().includes('lbry://playlist')) { | ||
| const match = uri.match(/[#:/]([0-9a-f]{40})/i); | ||
| if (match) detectedCollectionId = match[1]; | ||
| } |
There was a problem hiding this comment.
Variable shadowing: match shadows the outer scope variable.
Line 49 declares a new match variable that shadows the match from props (line 16). This is likely unintentional and could cause confusion. Consider renaming to collectionIdMatch or similar.
🔎 Proposed fix
// Detect collections from playlist-style URIs even if claim isn't resolved as a collection
let detectedCollectionId = null;
if (uri && typeof uri === 'string' && uri.toLowerCase().includes('lbry://playlist')) {
- const match = uri.match(/[#:/]([0-9a-f]{40})/i);
- if (match) detectedCollectionId = match[1];
+ const collectionIdMatch = uri.match(/[#:/]([0-9a-f]{40})/i);
+ if (collectionIdMatch) detectedCollectionId = collectionIdMatch[1];
}📝 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.
| // Detect collections from playlist-style URIs even if claim isn't resolved as a collection | |
| let detectedCollectionId = null; | |
| if (uri && typeof uri === 'string' && uri.toLowerCase().includes('lbry://playlist')) { | |
| const match = uri.match(/[#:/]([0-9a-f]{40})/i); | |
| if (match) detectedCollectionId = match[1]; | |
| } | |
| // Detect collections from playlist-style URIs even if claim isn't resolved as a collection | |
| let detectedCollectionId = null; | |
| if (uri && typeof uri === 'string' && uri.toLowerCase().includes('lbry://playlist')) { | |
| const collectionIdMatch = uri.match(/[#:/]([0-9a-f]{40})/i); | |
| if (collectionIdMatch) detectedCollectionId = collectionIdMatch[1]; | |
| } |
🤖 Prompt for AI Agents
In ui/page/embedWrapper/index.js around lines 46 to 51, the local variable match
declared for the regex result shadows an outer-scope prop named match (from line
16); rename the local variable (e.g., collectionIdMatch) and update its usages
so it no longer conflicts with the prop, keep it as a const, and preserve the
existing regex logic and assignment to detectedCollectionId.
tzarebczan
force-pushed
the
embed-improvements
branch
from
3b4f6d7 to
fc90109
Compare
There was a problem hiding this comment.
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)
ui/page/embedWrapper/index.js (1)
108-115: Remove duplicate try-catch block.The second try-catch block (lines 112-115) is identical to the first and serves no purpose.
🔎 Proposed fix
if (isOldPermanentUriFormat) { try { return buildURI({ claimName, claimId }); } catch (error) {} - try { - return buildURI({ claimName, claimId }); - } catch (error) {} - // ^-------- why twice? }
♻️ Duplicate comments (3)
ui/page/claim/internal/claimPageComponent/view.jsx (1)
146-177: Wrap the switch case declaration in a block to prevent scope leakage.The static analysis correctly identifies that
const newUrldeclared in theCOLLECTION/PLAYLISTcase could be erroneously accessed from other switch clauses. This is a duplicate of a previous review comment.🔎 Proposed fix
if (claim && isCollection && collectionFirstItemUri) { switch (collection?.type) { case COL_TYPES.COLLECTION: - case COL_TYPES.PLAYLIST: + case COL_TYPES.PLAYLIST: { urlParams.set(COLLECTIONS_CONSTS.COLLECTION_ID, claim.claim_id); const newUrl = formatLbryUrlForWeb(`${collectionFirstItemUri}?${urlParams.toString()}`); return <Redirect to={newUrl} />; + } case COL_TYPES.FEATURED_CHANNELS: return <Redirect to={`/$/${PAGES.PLAYLIST}/${claim.claim_id}`} />; default: // Do nothing break; } }web/src/html.js (1)
583-588: Unreachable code: this condition is already handled above.Lines 567-570 already check for
/$/embed/homeand return early, so this late special-case block will never execute. This is dead code that should be removed.🔎 Proposed fix
const googleVideoMetadata = await buildGoogleVideoMetadata(claimUri, claim); return insertToHead(html, ogMetadata.concat('\n', googleVideoMetadata)); } - // Late special-case: homepage embed when not enabled (maintain old order) - if (requestPath === '/$/embed/home' || requestPath === '/$/embed/home/') { - const ogMetadata = buildOgMetadata({ baseUrl: ctx.origin, fcActionUrl: `${URL}/$/embed/home` }); - return insertToHead(html, ogMetadata); - } return insertToHead(html);ui/page/embedWrapper/view.jsx (1)
105-112: Remove duplicate try-catch block (same issue as in index.js).The second try-catch block is identical to the first and serves no purpose.
🔎 Proposed fix
if (isOldPermanentUriFormat) { try { return buildURI({ claimName, claimId }); } catch (error) {} - try { - return buildURI({ claimName, claimId }); - } catch (error) {} }
🧹 Nitpick comments (7)
web/src/html.js (1)
166-172: Minified inline script is hard to audit; consider extracting or formatting.The ready signal script (line 170) is heavily minified inline JavaScript. While functional, this makes security audits difficult. Consider either:
- Extracting to a separate file served statically
- Adding a formatted/commented version in the codebase with the minified version generated
Also note: the
postMessagewith'*'origin could be tightened if the parent frame origin is known.ui/page/embedWrapper/internal/embedClaimComponent/view.jsx (2)
92-107: Prefer CSS classes over inline styles for consistency.The inline styles on lines 93-96 should use CSS classes to maintain consistency with the rest of the codebase and make styling more maintainable.
🔎 Suggested refactor
{channelClaimId && ( - <div style={{ marginTop: '16px' }}> - <h2 style={{ fontSize: '1.2rem', marginBottom: '12px', color: 'var(--color-text)' }}> + <div className="embed-channel-content"> + <h2 className="embed-channel-content__header"> {__('Recent Content')} </h2>Then add corresponding CSS in
_embed-player.scss.
248-263: PropTypes defined after export is non-standard; consider moving before export.While this works, the conventional pattern is to define
propTypesbefore theexport defaultstatement for better readability.🔎 Suggested refactor
+EmbedClaimComponent.propTypes = { + uri: PropTypes.string.isRequired, + latestClaimUrl: PropTypes.string, + collectionId: PropTypes.string, + renderMode: PropTypes.string, + isLivestreamClaim: PropTypes.bool, + showScheduledInfo: PropTypes.bool, + channelClaimId: PropTypes.string, + isCollection: PropTypes.bool, + streamingUrl: PropTypes.string, + doFileGetForUri: PropTypes.func, + collectionUrls: PropTypes.array, + doFetchItemsInCollection: PropTypes.func, +}; + export default EmbedClaimComponent; - -EmbedClaimComponent.propTypes = { - ... -};ui/page/claim/view.jsx (1)
72-82: Component recreated on every render; consider memoizing like other wrappers.
ChannelPageEditingWrapperLocalis defined inside the conditional block, causing it to be recreated on every render whenediting=true. This can cause unnecessary unmount/remount cycles. Consider memoizing it likeChannelPageWrapperabove.🔎 Suggested refactor
+ const ChannelPageEditingWrapper = React.useMemo( + () => + ({ children }: { children: any }) => + ( + <Page + className="channelPage-wrapper channelPage-edit-wrapper" + noFooter + fullWidthPage + noSideNavigation={isEmbedPath} + > + {children} + </Page> + ), + [isEmbedPath] + ); if (isChannel) { const urlParams = new URLSearchParams(search); const editing = urlParams.get(CHANNEL_PAGE.QUERIES.VIEW) === CHANNEL_PAGE.VIEWS.EDIT; if (editing) { - const ChannelPageEditingWrapperLocal = ({ children }: { children: any }) => ( - <Page - className="channelPage-wrapper channelPage-edit-wrapper" - noFooter - fullWidthPage - noSideNavigation={isEmbedPath} - > - {children} - </Page> - ); - return <ClaimPageComponent uri={uri} ClaimRenderWrapper={ChannelPageEditingWrapperLocal} Wrapper={Page} />; + return <ClaimPageComponent uri={uri} ClaimRenderWrapper={ChannelPageEditingWrapper} Wrapper={Page} />; }ui/page/embedWrapper/index.js (1)
157-172: Empty catch block silently swallows errors.The empty
catch (e) {}at line 170 silently swallows all errors during path parsing. Consider at minimum logging in development or returning a more explicit failure state.🔎 Suggested improvement
- } catch (e) {} + } catch (e) { + if (process.env.NODE_ENV === 'development') { + console.warn('buildMatchFromPath failed:', e); + } + }ui/page/embedWrapper/view.jsx (1)
88-120: Code duplication:getUriFromMatchandbuildMatchWithHashexist in both index.js and view.jsx.These helper functions are duplicated between
ui/page/embedWrapper/index.jsand this file. Consider extracting them to a shared utility module to maintain DRY principles and ensure consistent behavior.🔎 Suggested approach
Create a shared utility file:
// ui/page/embedWrapper/helpers.js export function getUriFromMatch(match) { ... } export function buildMatchWithHash(match, hash) { ... }Then import in both files:
import { getUriFromMatch, buildMatchWithHash } from './helpers';Also applies to: 122-146
web/index.js (1)
33-51: LGTM! The conditional routing logic correctly handles both development (dynamic-first for proper meta injection) and production (static-first for performance) scenarios. The configuration is cleanly separated.Consider adding
router.allowedMethods()afterrouter.routes()in both branches (lines 35 and 50) to properly handle HTTP 405 responses for unsupported HTTP methods.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
.gitignoreconfig.jspackage.jsonui/component/app/view.jsxui/component/claimMenuList/view.jsxui/component/claimTilesDiscover/view.jsxui/component/embedTextArea/view.jsxui/component/fileActions/view.jsxui/component/header/view.jsxui/component/page/view.jsxui/component/router/view.jsxui/component/sideNavigation/view.jsxui/component/socialShare/view.jsxui/component/subscribeButton/view.jsxui/component/viewers/videoViewer/internal/videojs.jsxui/component/viewers/videoViewer/view.jsxui/hocs/withResolvedClaimRender/helper.jsxui/hocs/withStreamClaimRender/index.jsui/hocs/withStreamClaimRender/view.jsxui/page/claim/internal/claimPageComponent/view.jsxui/page/claim/view.jsxui/page/collection/index.jsui/page/collection/view.jsxui/page/embedWrapper/index.jsui/page/embedWrapper/internal/embedClaimComponent/index.jsui/page/embedWrapper/internal/embedClaimComponent/view.jsxui/page/embedWrapper/view.jsxui/redux/actions/arwallet.jsui/scss/component/_claim-preview.scssui/scss/component/_embed-player.scssui/util/buildHomepage.jsweb/index.jsweb/src/farcaster.jsweb/src/html.jsweb/src/routes.jsweb/src/tools/check-farcaster.js
🚧 Files skipped from review as they are similar to previous changes (19)
- ui/component/app/view.jsx
- ui/page/collection/index.js
- ui/component/viewers/videoViewer/internal/videojs.jsx
- ui/component/socialShare/view.jsx
- ui/component/header/view.jsx
- ui/component/fileActions/view.jsx
- config.js
- ui/component/router/view.jsx
- ui/component/viewers/videoViewer/view.jsx
- ui/component/embedTextArea/view.jsx
- ui/util/buildHomepage.js
- .gitignore
- web/src/tools/check-farcaster.js
- ui/component/page/view.jsx
- ui/page/collection/view.jsx
- ui/scss/component/_claim-preview.scss
- ui/component/subscribeButton/view.jsx
- ui/component/sideNavigation/view.jsx
- ui/component/claimMenuList/view.jsx
🧰 Additional context used
🧬 Code graph analysis (7)
web/index.js (2)
config.js (1)
config(5-120)web/src/routes.js (1)
config(2-2)
web/src/html.js (3)
web/src/tools/check-farcaster.js (1)
url(28-28)ui/util/url.js (3)
url(89-89)url(163-163)url(240-240)web/src/routes.js (1)
ctx(21-21)
web/src/routes.js (1)
config.js (1)
config(5-120)
ui/page/embedWrapper/index.js (1)
ui/page/embedWrapper/internal/embedClaimComponent/index.js (3)
select(16-33)channelClaimId(21-21)isCollection(22-22)
ui/page/claim/view.jsx (1)
ui/component/header/view.jsx (1)
isEmbedPath(120-120)
ui/hocs/withResolvedClaimRender/helper.jsx (2)
ui/page/embedWrapper/index.js (1)
uri(20-20)ui/page/embedWrapper/view.jsx (2)
uri(37-37)uri(47-47)
ui/redux/actions/arwallet.js (1)
ui/redux/actions/notifications.js (1)
doToast(13-25)
🪛 Biome (2.1.2)
ui/page/claim/internal/claimPageComponent/view.jsx
[error] 166-166: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
ui/page/embedWrapper/view.jsx
[error] 19-25: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 27-27: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
ui/page/claim/view.jsx
[error] 31-31: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 58-58: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 72-72: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
ui/hocs/withResolvedClaimRender/helper.jsx
[error] 39-39: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 39-39: expected , but instead found ?
Remove ?
(parse)
[error] 39-39: expected , but instead found UriAccessKey
Remove UriAccessKey
(parse)
ui/hocs/withStreamClaimRender/view.jsx
[error] 19-19: type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
ui/redux/actions/arwallet.js
[error] 74-74: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 74-74: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 370-370: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 370-370: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
⏰ 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: lint
🔇 Additional comments (24)
ui/hocs/withStreamClaimRender/index.js (2)
117-118: LGTM!The
areStatePropsEqualoption is correctly integrated into theconnectcall. The HOC composition order (withRouter→connect→withStreamClaimRender) is preserved.
40-69: The equality check correctly covers all consumedplayingUrifields.The component accesses only
playingUri.uriandplayingUri.sourceId, and both are compared in the custom equality function. The comparison also includesplayingUri.collection.collectionId(which is not directly accessed), making the check conservative but safe—changes to unused fields won't trigger unnecessary re-renders, and all accessed fields are monitored.ui/component/claimTilesDiscover/view.jsx (1)
119-122: LGTM! Embed home path aligned with pin injection logic.The expansion to include
/$/embed/homealongside/correctly ensures that pinned content appears consistently on both the main homepage and the embed homepage.web/src/farcaster.js (1)
15-15: LGTM! Farcaster frame now points to embed home.The change correctly directs Farcaster Mini App navigation to the embed-optimized home route, aligning with the PR's objective to configure the homepage embed for the Mini App action.
ui/scss/component/_embed-player.scss (2)
20-27: LGTM! Page-mode styling supports full-page embed layouts.The new
.embed__wrapper--pageclass appropriately switches from flexbox channel layout to block layout with page-level background colors, enabling embed wrappers to render as full pages.
47-52: LGTM! Readability improvement for embedded content.The explicit
color: var(--color-text)ensures that markdown and document content remains readable in embed contexts, even when the wrapper uses different background/text color combinations.ui/page/claim/internal/claimPageComponent/view.jsx (2)
69-69: LGTM! Embed context correctly derived from pathname.The
isEmbedflag is appropriately set by checking if the pathname starts with/$/embed, providing a clean way to gate embed-specific behavior throughout the component.
86-87: LGTM! Canonical URL rewriting correctly skipped in embed mode.The early return when
isEmbedis true prevents the canonical URL rewrite effect from running on embed paths, preserving the embed URL structure. The addition ofisEmbedto the dependency array ensures the effect responds to changes in embed context.Also applies to: 123-123
ui/hocs/withResolvedClaimRender/helper.jsx (1)
30-57: LGTM! Guarded URL updates reduce unnecessary churn.The changes introduce several improvements:
- Early return for falsy
uriprevents unnecessary work- Embed-route guard avoids URL mutations on embed paths, reducing jank
- Difference check ensures
history.replaceStateonly runs when signature values actually change- Robust error handling with try/catch around pathname access
The removal of
doFetchUriAccessKeyfrom dependencies is intentional and correctly documented to avoid identity-based re-runs.ui/redux/actions/arwallet.js (1)
120-124: LGTM! Improved error message extraction.The enhanced error handling correctly extracts string messages from caught exceptions, checking for both string errors and errors with
.messageproperties. The specific check for 'User cancelled the AuthRequest' appropriately sets the disconnect flag, and the fallback error message improves user experience when error details are unavailable.ui/page/embedWrapper/internal/embedClaimComponent/index.js (1)
3-10: LGTM! Enhanced embed claim component with necessary data and actions.The additions correctly:
- Derive claim and channel information from state
- Determine collection status from claim value_type
- Provide streaming URL and collection URLs for rendering
- Wire up file fetching and collection item actions
The comment at line 20 helpfully clarifies the dual-path channel ID resolution for channel pages vs. streams.
Also applies to: 17-40
ui/hocs/withStreamClaimRender/view.jsx (4)
17-19: LGTM! Bounded Set prevents unbounded memory growth.The module-level
homeInitFlagsSet with a size limit of 100 addresses the previous review concern about unbounded accumulation. Clearing the Set when the limit is reached prevents memory leaks while maintaining the guard mechanism to prevent repeatedisHomeupdates.Based on learnings, the use of a bounded Set was recommended over the previous
window.__homeInitFlagsapproach.
137-137: LGTM! Fallback to 'lid' parameter improves compatibility.The addition of
urlParams.get('lid')as a fallback whenCOLLECTION_IDis absent enhances compatibility with alternative collection ID parameter formats.
230-241: LGTM! Homepage detection expanded and guarded to prevent loops.The changes correctly:
- Expand
isHometo include/$/embed/homealongside/- Use the bounded
homeInitFlagsSet to ensureupdateClaim('isHome')runs at most once per unique URI- Add
urito the effect's dependency array so the effect responds to URI changesThis prevents update loops on the homepage while supporting both main and embed home paths.
Also applies to: 269-269
271-277: LGTM! Non-video embeds now fetch their source correctly.The new effect ensures that markdown embeds fetch their source via
doFileGetForUriwhencanViewFileis true,renderModeis 'md', andstreamingUrlis not yet available. This complements the broader embed support for non-video content types.web/src/html.js (2)
209-211: LGTM!The
buildBasicOgMetadatafunction correctly forwardsoverrideOptionstobuildOgMetadata, maintaining consistency with the newfcActionUrlparameter support.
523-528: LGTM!Good use of
ctx.originto ensure the embed URL works correctly in development environments, avoiding hardcoded production URLs.ui/page/embedWrapper/internal/embedClaimComponent/view.jsx (2)
66-71: LGTM!The
useEffectcorrectly fetches collection items when conditions are met, with proper dependency array. Good placement before conditional returns per React hooks rules.
197-223: LGTM!The
EmbeddedMarkdowncomponent has well-structured async fetch logic with:
- Proper cancellation via
cancelledflag- Distinction between loading (
undefined), error (null), and loaded states- Appropriate cleanup on unmount
package.json (1)
30-30: LGTM!The
DYNAMIC_ROUTES_FIRST=trueenvironment variable is correctly propagated to both the initial compile and the concurrent watch/dev phases, ensuring consistent behavior during development.ui/page/claim/view.jsx (1)
26-27: LGTM!The
isEmbedPathderivation is consistent with patterns used elsewhere in the codebase (e.g.,ui/component/header/view.jsx:119).ui/page/embedWrapper/index.js (1)
46-51: LGTM!Variable shadowing issue from past review has been addressed - now uses
collectionIdMatchinstead ofmatch.ui/page/embedWrapper/view.jsx (1)
27-77: LGTM!The refactored component cleanly delegates rendering to specialized page components based on content type:
CollectionPagefor collectionsClaimPagefor channels/markdownEmbedClaimComponentfor simple embedsGood use of
React.Suspensewith a fallback spinner and theEmbedContext.Providerfor state sharing.web/src/routes.js (1)
102-119: Asymmetric handling is intentional and correct.The asymmetry between modes reflects the middleware order in
web/index.js:
- Dynamic-first mode: Routes execute before static middleware, so
/public/*must be passed vianext()to reach the static handler.- Static-first mode: Static middleware executes first and serves all
/public/*files (JS, CSS, images, etc.). The route handler only checks for missing/public/*.jsto return 404 (avoiding claim collision). Non-JS assets never reach the route handler because they're already served by the static middleware.Non-JS static assets in
/public/are handled correctly in both modes.
tzarebczan
force-pushed
the
embed-improvements
branch
from
fc90109 to
7b0d3f5
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI Agents
In @ui/page/claim/internal/claimPageComponent/view.jsx:
- Around line 161-176: The switch on collection?.type leaks the const newUrl
declared in the COL_TYPES.COLLECTION/COL_TYPES.PLAYLIST case; wrap that case
body in a block to scope the declaration (e.g., add { ... } around the lines
that set urlParams, compute newUrl via formatLbryUrlForWeb, and return <Redirect
/>) so newUrl cannot be accessed by other cases like
COL_TYPES.FEATURED_CHANNELS; keep the same logic and return statements but
ensure the block limits the const's scope.
In @ui/page/embedWrapper/internal/embedClaimComponent/view.jsx:
- Around line 190-193: The propTypes for EmbeddedVideoClaimComponent are wrong:
change streamClaim from PropTypes.func to the appropriate object shape used by
VideoRender (e.g., PropTypes.object or a more specific PropTypes.shape({...})
matching the claim fields VideoRender expects) and mark it .isRequired if the
component always receives it; update EmbeddedVideoClaimComponent.propTypes to
use that object/shape for streamClaim so the prop type matches actual usage.
In @web/src/html.js:
- Around line 566-570: The comment claims "only when enabled" but the code
unconditionally handles requestPath === '/$/embed/home'; either add a real
enablement check or correct the comment: implement a guard like
isHomepageEmbedEnabled() (or use an existing config flag) around the if
(requestPath === '/$/embed/home' ...) block so buildOgMetadata/insertToHead run
only when embedding is allowed (use ctx.origin, buildOgMetadata, insertToHead
as-is), or if no feature flag exists/desired, update the comment to remove "only
when enabled" and accurately state that the homepage embed path is always
handled here.
🧹 Nitpick comments (11)
ui/component/embedTextArea/view.jsx (1)
72-82: Remove dead code from thenameattribute.The conditional rendering logic is correct. However, line 75 contains unnecessary code: the ternary expression
(newestType ? ' ' + newestType : '')will always evaluate to an empty string becauseshowAutoplayToggleis onlytruewhennewestTypeis falsy (see line 28).🔎 Proposed simplification
<div className="margin-vertical-medium"> <FormField - name={'embed-autoplay' + (newestType ? ' ' + newestType : '')} + name="embed-autoplay" type="checkbox" label={__('Enable Autoplay')} checked={embedAutoplay} onChange={() => setEmbedAutoplay((prev) => !prev)} /> </div>web/src/html.js (1)
167-172: Consider externalizing or documenting the minified ready signal script.The inline minified script is difficult to read and maintain. While the logic appears sound (polling for SDK availability with timeout), consider one of these approaches:
- Keep the source code in a more readable format with comments
- Extract to a separate file and reference it
- Add inline comments explaining the minified code's behavior
This will make future debugging and modifications easier.
📝 Expanded readable version for reference
The minified script does the following:
- Defines a ready signal function that tries multiple SDK locations
- Polls every 100ms for up to 5 seconds (50 attempts)
- Sends
miniapp-readyvia postMessage when SDK is available- Sends
miniapp-ready-timeoutif SDK never becomes availableui/component/viewers/videoViewer/view.jsx (1)
644-670: The FileReactions overlay implementation looks solid, but consider externalizing styles.The conditional logic and positioning are appropriate for showing reactions in embedded authenticated contexts. The inline styles achieve the desired visual effect with proper z-index layering and backdrop blur.
However, the extensive inline styles (lines 646-665) could make maintenance more challenging. Consider moving these styles to a dedicated SCSS class in
_embed-player.scssor_video-js.scss.🔎 Optional refactor to externalize styles
In
ui/scss/component/_embed-player.scssor a suitable file, add:.file-viewer__reactions-overlay { position: absolute; right: 8px; bottom: 48px; z-index: 7; display: flex; gap: 8px; pointer-events: auto; background: rgba(0, 0, 0, 0.45); border-radius: 8px; padding: 6px 8px; align-items: center; backdrop-filter: blur(1.5px); // Override file-action hover colors for better contrast --color-link: var(--color-text); --color-fire: var(--color-text); --color-slime: var(--color-text); }Then simplify the JSX:
- <div - style={{ - position: 'absolute', - right: '8px', - bottom: '48px', - zIndex: 7, - display: 'flex', - gap: '8px', - pointerEvents: 'auto', - background: 'rgba(0, 0, 0, 0.45)', - borderRadius: '8px', - padding: '6px 8px', - alignItems: 'center', - backdropFilter: 'blur(1.5px)', - '--color-link': 'var(--color-text)', - '--color-fire': 'var(--color-text)', - '--color-slime': 'var(--color-text)', - }} - aria-label={__('Reactions')} - > + <div className="file-viewer__reactions-overlay" aria-label={__('Reactions')}> <FileReactions uri={uri} /> </div>ui/component/header/view.jsx (1)
250-250: Consider destructuringhideSidebarTogglefor consistency.While the current implementation works, accessing
props.hideSidebarToggledirectly is inconsistent with the rest of the component's pattern where props are destructured at the top (lines 62-88). Consider addinghideSidebarToggleto the destructured props list.🔎 Suggested refactor
At line 88, add
hideSidebarToggleto the destructured props:setSidebarOpen, signOut, + hideSidebarToggle, } = props;Then update line 250:
- {!authHeader && !props.hideSidebarToggle && ( + {!authHeader && !hideSidebarToggle && (ui/page/claim/view.jsx (2)
33-42: Consider simplifying boolean expression.The
noSideNavigationprop uses!!isMarkdownPostwhich is redundant in a boolean context. The double negation doesn't add value here since the&&operator already coerces to boolean.🔎 Suggested simplification
<Page className="file-page" filePage isMarkdown={!!isMarkdownPost} - noSideNavigation={isEmbedPath && !!isMarkdownPost} + noSideNavigation={isEmbedPath && isMarkdownPost} >Note: Keep
!!isMarkdownPostfor theisMarkdownprop if it requires a strict boolean, butnoSideNavigationdoesn't need the double negation.
72-82: Consider memoizingChannelPageEditingWrapperLocalfor consistency and performance.Unlike the other wrappers (
ClaimRenderWrapper,LivestreamPageWrapper,ChannelPageWrapper),ChannelPageEditingWrapperLocalis not memoized. This means it will be recreated on every render, potentially causing unnecessary re-renders ofClaimPageComponent. For consistency and to avoid potential performance issues, consider memoizing it.🔎 Suggested refactor
if (editing) { - const ChannelPageEditingWrapperLocal = ({ children }: { children: any }) => ( - <Page - className="channelPage-wrapper channelPage-edit-wrapper" - noFooter - fullWidthPage - noSideNavigation={isEmbedPath} - > - {children} - </Page> - ); + const ChannelPageEditingWrapperLocal = React.useMemo( + () => + ({ children }: { children: any }) => + ( + <Page + className="channelPage-wrapper channelPage-edit-wrapper" + noFooter + fullWidthPage + noSideNavigation={isEmbedPath} + > + {children} + </Page> + ), + [isEmbedPath] + ); return <ClaimPageComponent uri={uri} ClaimRenderWrapper={ChannelPageEditingWrapperLocal} Wrapper={Page} />; }ui/hocs/withStreamClaimRender/view.jsx (1)
233-241: Per-URI guard prevents repeatedupdateClaim('isHome')calls.The Set-based guard successfully prevents loops. When the Set reaches 100 entries, it clears completely and starts fresh.
For high-traffic scenarios with >100 unique URIs, consider an LRU eviction strategy to preserve recently-seen entries:
🔎 Optional LRU-based approach
- if (homeInitFlags.size >= HOME_INIT_FLAGS_MAX_SIZE) { - homeInitFlags.clear(); - } - homeInitFlags.add(uri); + if (homeInitFlags.size >= HOME_INIT_FLAGS_MAX_SIZE) { + // Remove oldest entry (first in Set) + const firstKey = homeInitFlags.values().next().value; + homeInitFlags.delete(firstKey); + } + homeInitFlags.add(uri);Note: This keeps the most recent 100 URIs rather than clearing everything. However, the current approach is simpler and sufficient for typical usage.
ui/page/embedWrapper/index.js (1)
119-119: Consider simplifying the String cast.The
String(claimName)cast is defensive, but sinceclaimNamecomes from route params (typically strings), you could optionally rely on string concatenation's implicit coercion or add an earlier type guard if needed.ui/page/embedWrapper/view.jsx (1)
20-25: Consider removing Flow type annotations.The file uses PropTypes for runtime validation (lines 79-84) but still has Flow-style
type Propsdefinition. Since Flow types are compile-time only and PropTypes provide runtime checks, consider removing the Flow annotations to avoid redundancy and satisfy the static analysis warnings.Note: The static analysis tool flagged this as TypeScript-only syntax in a
.jsxfile.ui/page/embedWrapper/internal/embedClaimComponent/view.jsx (2)
195-240: LGTM: Well-implemented markdown fetching with cleanup.The
EmbeddedMarkdowncomponent properly:
- Fetches file info when needed via
doFileGetForUri- Implements cancellation tokens to prevent state updates after unmount
- Handles loading, error, and success states clearly
Consider adding a fetch timeout for better UX if the markdown source is slow or unresponsive, though this is optional.
250-263: LGTM: Comprehensive PropTypes coverage.The PropTypes definitions cover all component props appropriately. If desired,
collectionUrlscould be refined toPropTypes.arrayOf(PropTypes.string)for stricter validation, but the currentPropTypes.arrayis acceptable.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
.gitignoreconfig.jspackage.jsonui/component/app/view.jsxui/component/claimTilesDiscover/view.jsxui/component/embedTextArea/view.jsxui/component/fileActions/view.jsxui/component/header/view.jsxui/component/page/view.jsxui/component/router/view.jsxui/component/sideNavigation/view.jsxui/component/socialShare/view.jsxui/component/subscribeButton/view.jsxui/component/viewers/videoViewer/internal/videojs.jsxui/component/viewers/videoViewer/view.jsxui/hocs/withResolvedClaimRender/helper.jsxui/hocs/withStreamClaimRender/index.jsui/hocs/withStreamClaimRender/view.jsxui/page/claim/internal/claimPageComponent/view.jsxui/page/claim/view.jsxui/page/collection/index.jsui/page/collection/view.jsxui/page/embedWrapper/index.jsui/page/embedWrapper/internal/embedClaimComponent/index.jsui/page/embedWrapper/internal/embedClaimComponent/view.jsxui/page/embedWrapper/view.jsxui/scss/component/_claim-preview.scssui/scss/component/_embed-player.scssui/util/buildHomepage.jsweb/index.jsweb/src/farcaster.jsweb/src/html.jsweb/src/routes.jsweb/src/tools/check-farcaster.js
🚧 Files skipped from review as they are similar to previous changes (14)
- ui/component/app/view.jsx
- web/src/farcaster.js
- ui/component/socialShare/view.jsx
- ui/component/viewers/videoViewer/internal/videojs.jsx
- ui/component/fileActions/view.jsx
- ui/component/subscribeButton/view.jsx
- ui/util/buildHomepage.js
- ui/page/collection/view.jsx
- ui/page/collection/index.js
- config.js
- .gitignore
- ui/component/sideNavigation/view.jsx
- ui/component/page/view.jsx
- package.json
🧰 Additional context used
🧬 Code graph analysis (8)
web/src/tools/check-farcaster.js (3)
ui/component/sideNavigation/view.jsx (1)
isEmbed(37-37)ui/component/subscribeButton/view.jsx (1)
isEmbed(47-47)ui/page/claim/internal/claimPageComponent/view.jsx (1)
isEmbed(69-69)
ui/component/embedTextArea/view.jsx (5)
ui/component/fileActions/view.jsx (1)
isChannel(94-94)ui/component/socialShare/view.jsx (2)
isChannel(128-128)isCollection(129-129)web/src/oEmbed.js (2)
claim(21-21)claim(32-32)ui/page/claim/internal/claimPageComponent/view.jsx (1)
isCollection(68-68)ui/component/common/form-components/form-field.jsx (1)
FormField(74-89)
ui/hocs/withResolvedClaimRender/helper.jsx (3)
ui/page/embedWrapper/index.js (1)
uri(20-20)ui/component/app/view.jsx (1)
uri(204-204)ui/page/embedWrapper/view.jsx (2)
uri(37-37)uri(47-47)
ui/page/claim/internal/claimPageComponent/view.jsx (12)
ui/component/sideNavigation/view.jsx (1)
isEmbed(37-37)web/src/frame.js (1)
search(9-15)ui/component/app/view.jsx (3)
isNewestPath(185-185)urlParams(166-166)uri(204-204)ui/component/header/view.jsx (1)
urlParams(106-106)ui/page/claim/view.jsx (1)
urlParams(68-68)ui/component/router/view.jsx (1)
urlParams(235-235)ui/page/collection/view.jsx (2)
urlParams(64-64)collection(62-62)ui/page/embedWrapper/internal/embedClaimComponent/view.jsx (1)
urlParams(55-55)ui/page/embedWrapper/view.jsx (3)
urlParams(40-40)uri(37-37)uri(47-47)ui/redux/selectors/collections.js (12)
urlParams(232-232)urlParams(399-399)collection(113-113)collection(118-118)collection(169-169)collection(230-230)collection(308-308)collection(315-315)collection(351-351)collection(375-375)collection(510-510)collection(515-515)ui/page/embedWrapper/internal/embedClaimComponent/index.js (1)
claim(18-18)ui/util/url.js (1)
COLLECTIONS_CONSTS(9-9)
web/index.js (2)
config.js (1)
config(5-120)web/src/routes.js (2)
config(2-2)router(18-18)
ui/hocs/withStreamClaimRender/index.js (1)
ui/component/viewers/videoViewer/index.js (1)
select(44-98)
ui/component/viewers/videoViewer/view.jsx (2)
ui/component/fileReactions/view.jsx (1)
FileReactions(28-77)ui/component/viewers/videoViewer/index.js (1)
uri(49-49)
ui/page/claim/view.jsx (3)
ui/component/header/view.jsx (2)
isEmbedPath(120-120)urlParams(106-106)ui/component/page/view.jsx (1)
urlParams(83-83)ui/page/embedWrapper/view.jsx (1)
urlParams(40-40)
🪛 Biome (2.1.2)
ui/hocs/withResolvedClaimRender/helper.jsx
[error] 39-39: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 39-39: expected , but instead found ?
Remove ?
(parse)
[error] 39-39: expected , but instead found UriAccessKey
Remove UriAccessKey
(parse)
ui/page/claim/internal/claimPageComponent/view.jsx
[error] 166-166: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
ui/hocs/withStreamClaimRender/view.jsx
[error] 19-19: type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
ui/page/embedWrapper/view.jsx
[error] 19-25: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 27-27: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
ui/page/claim/view.jsx
[error] 31-31: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 58-58: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 72-72: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
⏰ 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: lint
🔇 Additional comments (51)
ui/component/embedTextArea/view.jsx (1)
26-28: LGTM! Clean conditional logic for autoplay toggle visibility.The derived booleans correctly identify channels and collections, and the
showAutoplayTogglelogic appropriately hides the autoplay control for these content types and whennewestTypeis present. The pattern is consistent with similar checks elsewhere in the codebase.web/src/html.js (4)
95-95: LGTM: fcActionUrl parameter correctly integrated.The
fcActionUrlparameter is consistently used throughout the metadata generation with proper fallback tourl. This allows Farcaster MiniApp/Frame actions to target a different URL than the page itself, which aligns with the PR objective of pointing homepage embeds to the embed route.Also applies to: 137-137, 154-154, 165-165
209-210: LGTM: buildBasicOgMetadata correctly updated.The function signature and implementation are correctly updated to accept and forward
overrideOptionstobuildOgMetadata, maintaining consistency with the enhanced metadata generation.
523-527: Verify ctx.origin usage for fcActionUrl.The implementation correctly configures the homepage to point Farcaster MiniApp actions to the homepage embed. The use of
ctx.origininstead of theURLconstant means this will adapt to the current environment (dev/staging/prod).Confirm this is the intended behavior, as it means development environments will generate fcActionUrl pointing to their local origin rather than the production URL.
583-587: Unreachable code: this condition is already handled above.This has been previously identified. Lines 567-570 already check for
/$/embed/homeand return early, making this block unreachable. Please remove this dead code as suggested in the earlier review.Likely an incorrect or invalid review comment.
ui/component/claimTilesDiscover/view.jsx (1)
119-122: LGTM!The extension to include the embed homepage path is consistent with the broader embed routing changes in this PR. The logic correctly ensures that pin URLs are injected for both the main homepage and the embed homepage.
web/src/tools/check-farcaster.js (1)
123-124: LGTM!The regex pattern update correctly handles both URL-encoded (
%24) and unencoded ($) forms of the embed path, making the validation more flexible. This aligns with how embed paths are detected throughout the application (e.g.,/$/embedin ui/component/sideNavigation/view.jsx and ui/page/claim/internal/claimPageComponent/view.jsx).ui/component/viewers/videoViewer/view.jsx (3)
26-26: LGTM!The FileReactions import is correctly added to support the new overlay functionality for embedded views.
538-538: LGTM!The local
hoveredstate is correctly initialized to track hover interactions for the reactions overlay.
553-554: LGTM!The mouse event handlers correctly toggle the hover state to control the visibility of the FileReactions overlay.
ui/scss/component/_embed-player.scss (2)
20-27: LGTM!The
.embed__wrapper--pageclass correctly defines page-mode styling for embeds. The overrides appropriately change the layout from flex to block with full viewport height and standard page background/text colors.
47-52: LGTM!The readability rules ensure that embedded markdown and document content have appropriate text color for legibility. The comment clearly explains the purpose, and the selectors cover the necessary embedded content scenarios.
ui/scss/component/_claim-preview.scss (1)
1541-1553: LGTM!The selector update correctly scopes claim preview tiles to the file-viewer context within embeds, aligning with the embed wrapper changes introduced elsewhere in this PR. The layout properties (flex column, center alignment, 100% height) appropriately configure tiles for full-height embedded display.
ui/component/router/view.jsx (3)
165-165: LGTM! Type definition correctly extends history API.The addition of
replaceto the history type is consistent with React Router's history API and supports the new redirect behavior below.
360-362: LGTM! Improved redirect approach prevents extra history entries.Using
history.replace()instead of<Redirect>is the correct approach here to avoid accumulating unnecessary history entries during URL normalization.
484-484: LGTM! Embed home route correctly configured.The new embed home route is properly configured with
exactmatching and correctly positioned before the parameterized embed routes.ui/component/header/view.jsx (3)
58-58: LGTM! Optional prop correctly typed.The
hideSidebarToggleprop is appropriately defined as optional and used consistently below.
120-122: LGTM! Embed-aware home navigation correctly implemented.The embed path detection and conditional home button navigation ensure proper navigation context when users are viewing embedded content.
268-268: LGTM! Home button behavior correctly extended for embed home.The click handler now appropriately treats the embed home path the same as the regular home path for scrolling and search clearing.
ui/page/claim/internal/claimPageComponent/view.jsx (3)
69-69: LGTM! Embed detection correctly implemented.The
isEmbedflag is derived consistently with the pattern used in other files (e.g.,ui/component/header/view.jsxline 120).
86-87: LGTM! Embed guard correctly prevents canonical URL rewriting.The early return preserves embed URLs by preventing the canonical URL rewrite logic, and
isEmbedis correctly included in the dependency array.Also applies to: 123-123
147-160: LGTM! Redirect guards correctly preserve embed URLs.The embed guard successfully prevents unwanted redirects in embed mode while maintaining the existing redirect behavior for non-embed contexts.
ui/hocs/withResolvedClaimRender/helper.jsx (4)
30-30: LGTM! Defensive guard improves robustness.The early return when URI is missing prevents unnecessary processing and potential errors.
32-36: LGTM! Embed guard prevents unnecessary URL updates.The try/catch wrapper provides safety around window.location access, and the embed path check correctly prevents URL churn on embed routes.
38-53: LGTM! Optimized URL update reduces unnecessary churn.The conditional update that only modifies the URL when signature values actually differ is a good optimization that prevents unnecessary history manipulation and potential UI jank.
54-57: The omission ofdoFetchUriAccessKeyfrom the dependency array is appropriate.doFetchUriAccessKeyis a redux action creator passed via props that remains stable across renders, and its behavior depends solely on theuriparameter (which is in the dependency array) and internal state access viagetState(). Including it would cause unnecessary re-runs due to identity churn without changing the effect's behavior. No stale closure issues exist here.Likely an incorrect or invalid review comment.
ui/page/claim/view.jsx (2)
26-27: LGTM! Consistent embed path detection.The
isEmbedPathdetection follows the same pattern used in other files throughout this PR.
56-65: LGTM! Channel page wrapper correctly memoized with embed awareness.The
ChannelPageWrapperis properly memoized withisEmbedPathas a dependency, ensuring navigation visibility is correctly controlled in embed contexts.web/src/routes.js (2)
2-2: LGTM! Config import is necessary for the routing strategy.The import is correctly positioned and required for accessing
config.DYNAMIC_ROUTES_FIRSTin the route handler below.
102-119: Solid implementation coordinating with the middleware order.The route handler correctly adapts to both routing strategies:
- Dynamic-first: passes static assets to downstream middleware via
next()(lines 107-109)- Static-first: returns 404 for missing
/public/*.jsto prevent claim name collisions (lines 113-117)The logic properly coordinates with the middleware ordering in
web/index.js, where dynamic-first places routes before static serving and static-first places static serving before routes. TheCache-Control: no-storeheader is appropriate for 404 responses.ui/hocs/withStreamClaimRender/view.jsx (4)
17-19: Good improvement: Bounded Set replaces unbounded window global.The module-level
Setwith a max size of 100 addresses the previous concern about unbounded growth. This is a cleaner, more maintainable solution.Note: Biome flags the Flow type annotation on line 19 since this is a
.jsxfile. Since the file explicitly uses// @flowat the top, this is intentional and can be ignored if your project uses Flow for type checking.
230-230: LGTM! Embed-home path correctly added to homepage detection.Extending
isHometo include/$/embed/homealigns with the PR's embed functionality enhancements.
269-269: LGTM! Dependency array correctly includesuri.Adding
urito the effect dependencies is correct since the effect uses it (lines 234, 238). This ensures the effect runs when the URI changes.
271-277: LGTM! Effect correctly fetches source for markdown embeds.This effect ensures non-video embeds (markdown) fetch their streaming source when in embed mode. The conditions and dependencies are appropriate.
ui/hocs/withStreamClaimRender/index.js (2)
117-118: LGTM! Custom equality function correctly wired into Redux.The
areStatePropsEqualoption is properly passed as the fourth argument toconnect, enabling the optimization throughout the component tree.
38-69: Excellent Redux optimization: Custom equality prevents needless rerenders.The
areStatePropsEqualfunction correctly implements deep comparison for the frequently-changingplayingUriobject while falling back to shallow comparison for other props. This significantly reduces unnecessary component updates.The implementation is sound:
- Fast path for reference equality
- Deep comparison of
playingUri.uri,playingUri.sourceId, andplayingUri.collection.collectionId- Shallow comparison for remaining props
Properly integrated with react-redux 7.2.0 via the
connect()options parameter.ui/page/embedWrapper/internal/embedClaimComponent/index.js (4)
3-10: LGTM! New imports support expanded embed functionality.The additional selectors and actions enable the embed component to handle streaming URLs, collections, and channel data.
17-22: LGTM! Selector robustly derives claim metadata.The selector correctly:
- Retrieves the claim via
selectClaimForUri- Uses
getChannelIdFromClaimhelper to handle both channel pages and signing channels- Safely checks
claim?.value_typewith optional chainingThe comment on line 20 adds helpful context.
28-31: LGTM! New props correctly derived and conditionally computed.The additional props enhance embed rendering capabilities:
streamingUrl: Always derived for the URIcollectionUrls: Conditionally fetched only whencollectionIdis presentchannelClaimIdandisCollection: Metadata for embed context
35-40: LGTM! Actions properly wired for embed data fetching.The
performobject correctly exposes:
doFileGetForUri: Fetch file data for embedsdoFetchItemsInCollection: Fetch collection items for playlist embedsThe export properly connects both selectors and actions to the component.
web/index.js (1)
33-51: LGTM! Clean implementation of configurable routing strategy.The conditional middleware ordering is correctly implemented. The dynamic-first branch enables proper meta injection for embeds and homepage, while the default static-first branch maintains production-safe behavior. The
config.DYNAMIC_ROUTES_FIRSTdefaults tofalse(viaprocess.env.DYNAMIC_ROUTES_FIRST === 'true'), preserving the existing, proven static-first behavior in production.ui/page/embedWrapper/index.js (3)
84-94: LGTM: Robust URI derivation guards.The added guards properly handle edge cases: missing match/params, absent claimName, and the special 'home' route case. This prevents incorrect URI resolution and potential routing conflicts.
157-172: LGTM: Clean embed path parser.The
buildMatchFromPathhelper correctly parses embed-style URLs with proper URL decoding and error handling. The fallback to empty params on error prevents downstream issues.
46-51: No verification needed—this change is correct for the embed URL context.The collection detection logic correctly checks for
/playlistin the URI path rather thanlbry://playlist. Embed URLs do not include thelbry://protocol prefix; they're parsed from paths like/$/{PAGES.EMBED}/claimName/claimIdand built into URIs without protocol. The/playlistcheck appropriately matches when a playlist is part of the URI path. This is an intentional and necessary adaptation to the embed URL format, not a risky change.Likely an incorrect or invalid review comment.
ui/page/embedWrapper/view.jsx (4)
12-16: LGTM: Proper lazy loading setup.The lazy-loaded components with named chunks enable effective code splitting for the embed scenarios (claim, collection, and embed-specific components). This improves initial load performance.
46-49: LGTM: Clear render mode differentiation.The
isPageLikeflag logically distinguishes between full-page embeds (channels, collections, markdown) and minimal embeds (videos, posts), driving appropriate layout and component selection.
51-77: LGTM: Well-structured lazy rendering.The component properly combines
EmbedContext,React.Suspense, and conditional rendering based on content type. The fallback spinner ensures a good loading experience.
109-111: Remove duplicate try-catch block.The second
try-catchblock is identical to the first and serves no purpose. This appears to be a copy-paste error.🔎 Proposed fix
if (isOldPermanentUriFormat) { try { return buildURI({ claimName, claimId }); } catch (error) {} - try { - return buildURI({ claimName, claimId }); - } catch (error) {} }Likely an incorrect or invalid review comment.
ui/page/embedWrapper/internal/embedClaimComponent/view.jsx (3)
66-71: LGTM: Proper effect placement and dependencies.The collection fetch effect is correctly positioned before conditional returns, with appropriate dependencies and a guard for when
doFetchItemsInCollectionis available.
92-107: LGTM: Channel recent content feature.The "Recent Content" section for channel embeds provides a good user experience by showcasing latest uploads. The
ClaimListDiscovercomponent is properly configured with appropriate filters and layout.
118-144: LGTM: Comprehensive playlist embed states.The collection/playlist embed properly handles loading, populated, and empty states, providing clear feedback at each stage. The
playItemsOnClickprop enables seamless playlist playback.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @ui/page/embedWrapper/view.jsx:
- Around line 88-117: getUriFromMatch is duplicated; extract it (and related
helper buildMatchWithHash) into a single shared utility module, export
getUriFromMatch and buildMatchWithHash, keep the same logic around buildURI and
normalizeURI and the special-case for claimName === 'home', and replace the
duplicated implementations in both index.js and view.jsx with imports from the
new module so both files call the shared getUriFromMatch and buildMatchWithHash
functions.
- Around line 119-143: The buildMatchWithHash function is duplicated and fragile
when match is null because it creates an object without params; extract
buildMatchWithHash (and mirror getUriFromMatch) into a shared utility module,
update callers to import it, and inside the extracted function ensure you
initialize params explicitly (e.g., matchedPath.params = matchedPath.params ||
{}) before accessing/mutating claimName/claimId so the function is
self-contained and robust against null/undefined match inputs.
- Around line 20-29: The file mixes Flow types and PropTypes redundantly; remove
the Flow annotations and any Flow pragma to avoid Biome linting issues: delete
the "type Props = { ... }" declaration and the ": Props" annotation on the
EmbedWrapperPage declaration (and remove any "// @flow" at the top), leaving the
existing EmbedWrapperPage function and the PropTypes block (lines around the
existing PropTypes assignment) as the single source of runtime prop validation
so Biome no longer reports Flow-related parser errors.
🧹 Nitpick comments (1)
ui/page/embedWrapper/index.js (1)
84-120: Consider simplifying redundant param checks.Line 84 already ensures
match.paramsexists, making the|| {}fallback on line 85 unnecessary. While defensive programming can be valuable, this specific redundancy adds no safety.Additionally, the
typeof claimName === 'string'check (line 98) andString(claimName)coercion (line 115) are defensive but likely unnecessary since route parameters are already strings.🔎 Optional simplification
function getUriFromMatch(match) { if (match && match.params) { - const { claimName, claimId } = match.params || {}; + const { claimName, claimId } = match.params;For the string coercions, consider removing them if route params are guaranteed to be strings, or keep them if there's a possibility of non-string values from other code paths.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ui/component/app/view.jsxui/page/claim/internal/claimPageComponent/view.jsxui/page/embedWrapper/index.jsui/page/embedWrapper/view.jsxweb/src/html.js
🚧 Files skipped from review as they are similar to previous changes (3)
- ui/page/claim/internal/claimPageComponent/view.jsx
- ui/component/app/view.jsx
- web/src/html.js
🧰 Additional context used
🧬 Code graph analysis (2)
ui/page/embedWrapper/index.js (1)
ui/page/embedWrapper/internal/embedClaimComponent/index.js (3)
select(16-33)channelClaimId(21-21)isCollection(22-22)
ui/page/embedWrapper/view.jsx (3)
ui/page/collection/view.jsx (2)
CollectionPage(35-183)useHistory(52-55)ui/page/embedWrapper/internal/embedClaimComponent/view.jsx (4)
EmbedClaimComponent(34-183)useHistory(51-53)featureParam(56-56)parseURI(58-58)ui/util/lbryURI.js (2)
buildURI(212-276)normalizeURI(279-303)
🪛 Biome (2.1.2)
ui/page/embedWrapper/view.jsx
[error] 19-25: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 27-27: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
⏰ 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: lint
🔇 Additional comments (8)
ui/page/embedWrapper/index.js (4)
17-19: LGTM! Fallback logic for missing match prop.The addition of
buildMatchFromPathas a fallback whenmatchis absent provides resilience for embed URL parsing. The logic correctly extracts pathname with a safe fallback.
122-151: LGTM! Improved defensive param handling.The explicit
matchedPath.paramsexistence checks (lines 126-128, 132, 138) prevent potential runtime errors when processing hash-based canonical URIs. This is good defensive programming.
153-168: LGTM! Flexible embed path parsing.The
buildMatchFromPathhelper robustly parses embed URLs. The implementation is more flexible than the comment suggests—it finds theembedsegment at any position rather than requiring a specific structure like['$', 'embed', ...]. This flexibility is beneficial.The try-catch block appropriately handles potential
decodeURIComponenterrors.
46-56: No changes needed—regex pattern correctly handles playlist URI formats.The regex
/[#:/]([0-9a-f]{40})/iproperly matches claim IDs from LBRY URIs. It correctly captures the 40-character hexadecimal identifiers with any of the three valid separators (:,#, or/) used in the LBRY URI specification. The case-insensitive flag and prior lowercase conversion on the URI ensure edge cases are handled.ui/page/embedWrapper/view.jsx (4)
1-16: LGTM! Effective use of lazy loading for code splitting.The lazy imports for
ClaimPage,CollectionPage, andEmbedClaimComponentimprove initial bundle size and load performance. ThelazyImportutility with webpack chunk names enables efficient code splitting.
36-49: LGTM! Well-structured page-like rendering detection.The URI derivation with fallback (lines 36-38) and the
isPageLikedetermination (lines 46-49) correctly distinguish between full-page renders (channels, collections, markdown) and minimal embeds. The optional chaining onwindow?.location?.hashprovides appropriate defensive handling.
51-77: LGTM! Clean conditional rendering with Suspense.The refactored rendering logic appropriately handles different content types:
- Collections →
CollectionPage- Channels/markdown →
ClaimPage- Other content →
EmbedClaimComponentThe Suspense boundary with a Spinner fallback provides good user experience during lazy component loading. The CSS class conditionals properly reflect the rendering mode.
79-84: LGTM! Appropriate runtime prop validation.The PropTypes provide runtime validation for the component's props. All props are correctly marked as optional, matching their actual usage in the component.
ui/page/embedWrapper/view.jsx
Outdated
| type Props = { | ||
| uri: string, | ||
| claimId: string, | ||
| latestClaimId: ?string, | ||
| canonicalUrl: ?string, | ||
| channelUri: ?string, | ||
| channelClaimId: ?string, | ||
| isLivestreamClaim: boolean, | ||
| latestClaimUrl: ?string, | ||
| doFetchLatestClaimForChannel: (uri: string, isEmbed: boolean) => void, | ||
| doFetchChannelIsLiveForId: (string) => void, | ||
| doCommentSocketConnect: (string, string, string, ?string) => void, | ||
| doCommentSocketDisconnect: (string, string) => void, | ||
| contentUnlocked: boolean, | ||
| uri?: string, | ||
| collectionId?: string, | ||
| isCollection?: boolean, | ||
| renderMode?: string, | ||
| }; | ||
|
|
||
| const EmbedWrapperPage = (props: Props) => { | ||
| const { | ||
| uri, | ||
| claimId, | ||
| latestClaimId, | ||
| canonicalUrl, | ||
| channelUri, | ||
| channelClaimId, | ||
| isLivestreamClaim, | ||
| latestClaimUrl, | ||
| doFetchLatestClaimForChannel, | ||
| doFetchChannelIsLiveForId, | ||
| doCommentSocketConnect, | ||
| doCommentSocketDisconnect, | ||
| contentUnlocked, | ||
| } = props; | ||
|
|
||
| const fetchedLiveStatus = React.useRef(); | ||
| const [videoEnded, setVideoEnded] = React.useState(false); | ||
| const { uri: incomingUri, collectionId, isCollection, renderMode } = props; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check Flow configuration and project setup
echo "=== Looking for .flowconfig ==="
fd -t f "\.flowconfig" || echo "No .flowconfig found"
echo -e "\n=== Checking package.json for Flow ==="
if [ -f package.json ]; then
cat package.json | jq '.devDependencies["flow-bin"] // .dependencies["flow-bin"] // "not found"' 2>/dev/null || echo "Could not parse package.json"
fi
echo -e "\n=== Checking for Flow scripts in package.json ==="
if [ -f package.json ]; then
cat package.json | jq '.scripts | to_entries[] | select(.value | contains("flow")) | .key + ": " + .value' 2>/dev/null || echo "No flow scripts found"
fi
echo -e "\n=== Checking Biome configuration ==="
fd -t f "biome.json\|.biomerc\|biome.jsonc" || echo "No Biome config found"
if [ -f biome.json ]; then
echo "Biome config exists, checking content..."
cat biome.json | head -50
fi
echo -e "\n=== Examining target file ==="
if [ -f "ui/page/embedWrapper/view.jsx" ]; then
echo "File exists. First 35 lines:"
head -35 "ui/page/embedWrapper/view.jsx" | cat -n
else
echo "File not found at ui/page/embedWrapper/view.jsx"
fi
echo -e "\n=== Searching for // @flow annotations in JSX files ==="
rg -l "^// @flow" --type jsx | head -10 || echo "No @flow annotations found"Repository: OdyseeTeam/odysee-frontend
Length of output: 3480
🏁 Script executed:
#!/bin/bash
# Verify PropTypes definition in the file
echo "=== Checking lines 75-90 for PropTypes definition ==="
sed -n '75,90p' "ui/page/embedWrapper/view.jsx" | cat -n
echo -e "\n=== Checking if PropTypes is used anywhere in the file ==="
rg -n "propTypes|PropTypes" "ui/page/embedWrapper/view.jsx"Repository: OdyseeTeam/odysee-frontend
Length of output: 836
Remove Flow type annotations or configure Biome to support Flow.
The file uses Flow type annotations (lines 20-25, 27) alongside PropTypes (lines 79-83), creating redundancy. The project includes Flow in its dev dependencies and lint scripts, but Biome's configuration doesn't explicitly include Flow support, which may cause false positives.
Choose one approach:
- Keep Flow: Remove the Flow type annotations and rely on PropTypes for runtime validation.
- Keep PropTypes only: Remove the Flow type definitions and
// @flowannotation. - Configure Biome: Add Flow parser configuration if the team intends to continue using Flow alongside the linter.
Using both Flow and PropTypes simultaneously is redundant and increases maintenance burden.
🧰 Tools
🪛 Biome (2.1.2)
[error] 27-27: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
🤖 Prompt for AI Agents
In @ui/page/embedWrapper/view.jsx around lines 20 - 29, The file mixes Flow
types and PropTypes redundantly; remove the Flow annotations and any Flow pragma
to avoid Biome linting issues: delete the "type Props = { ... }" declaration and
the ": Props" annotation on the EmbedWrapperPage declaration (and remove any "//
@flow" at the top), leaving the existing EmbedWrapperPage function and the
PropTypes block (lines around the existing PropTypes assignment) as the single
source of runtime prop validation so Biome no longer reports Flow-related parser
errors.
| function getUriFromMatch(match) { | ||
| if (match) { | ||
| const { claimName, claimId } = match.params || {}; | ||
|
|
||
| // Special case: don't resolve "home" as a claim (it has its own route) | ||
| if (claimName === 'home' && !claimId) { | ||
| return ''; | ||
| } | ||
| }, [channelClaimId, doFetchChannelIsLiveForId, latestClaimUrl, liveContentPath]); | ||
|
|
||
| React.useEffect(() => { | ||
| if (!latestClaimUrl && latestContentPath && canonicalUrl) { | ||
| doFetchLatestClaimForChannel(canonicalUrl, true); | ||
| // https://{DOMAIN}/claimName/claimId | ||
| const isOldPermanentUriFormat = | ||
| claimName && !claimName.startsWith('@') && !claimName.includes(':') && !claimName.includes('#') && claimId; | ||
|
|
||
| // https://{DOMAIN}/channelName/claimName/ | ||
| // on match channelName = claimName / claimName = claimId | ||
| const isCanonicalUriFormat = !isOldPermanentUriFormat; | ||
|
|
||
| if (isOldPermanentUriFormat) { | ||
| try { | ||
| return buildURI({ claimName, claimId }); | ||
| } catch (error) {} | ||
| } | ||
| }, [canonicalUrl, doFetchLatestClaimForChannel, latestClaimUrl, latestContentPath]); | ||
|
|
||
| // Establish web socket connection for viewer count. | ||
| React.useEffect(() => { | ||
| if (!isLivestreamClaim || !socketClaimId || !channelUrl || !canonicalUrl) { | ||
| return; | ||
| if (isCanonicalUriFormat && claimName) { | ||
| return normalizeURI(claimName + '/' + (claimId || '')); | ||
| } | ||
| } | ||
|
|
||
| const channelName = formatLbryChannelName(channelUrl); | ||
| return ''; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract duplicated utility to shared module.
The getUriFromMatch function is duplicated between index.js (lines 83-120) and view.jsx (lines 88-117) with minor variations. This duplication violates DRY principles and makes maintenance harder.
🔎 Suggested refactor
Create a shared utility file ui/page/embedWrapper/utils.js:
export function getUriFromMatch(match) {
if (match && match.params) {
const { claimName, claimId } = match.params;
if (!claimName) return '';
if (claimName === 'home' && !claimId) return '';
const isOldPermanentUriFormat =
claimName &&
!claimName.startsWith('@') &&
!claimName.includes(':') &&
!claimName.includes('#') &&
claimId;
if (isOldPermanentUriFormat) {
try {
return buildURI({ claimName, claimId });
} catch (error) {}
}
if (!isOldPermanentUriFormat && claimName) {
return normalizeURI(claimName + '/' + (claimId || ''));
}
}
return '';
}
export function buildMatchWithHash(match, hash) {
// ... existing logic ...
}Then import and use in both files.
🤖 Prompt for AI Agents
In @ui/page/embedWrapper/view.jsx around lines 88 - 117, getUriFromMatch is
duplicated; extract it (and related helper buildMatchWithHash) into a single
shared utility module, export getUriFromMatch and buildMatchWithHash, keep the
same logic around buildURI and normalizeURI and the special-case for claimName
=== 'home', and replace the duplicated implementations in both index.js and
view.jsx with imports from the new module so both files call the shared
getUriFromMatch and buildMatchWithHash functions.
| function buildMatchWithHash(match, hash) { | ||
| const matchedPath = Object.assign({}, match || {}); | ||
|
|
||
| return () => { | ||
| if (socketClaimId) { | ||
| doCommentSocketDisconnect(socketClaimId, channelName); | ||
| } | ||
| }; | ||
| }, [ | ||
| canonicalUrl, | ||
| channelUrl, | ||
| contentUnlocked, | ||
| doCommentSocketConnect, | ||
| doCommentSocketDisconnect, | ||
| isLivestreamClaim, | ||
| socketClaimId, | ||
| ]); | ||
| // if a claim is using the hash canonical format ("lbry://@chanelName#channelClaimId/streamName#streamClaimId" | ||
| // instead of "lbry://@chanelName:channelClaimId/streamName:streamClaimId") | ||
| if (hash && hash.length > 0 && matchedPath.params) { | ||
| // the hash is on the channel part of the uri | ||
| if (hash.includes('/')) { | ||
| const newClaimNameParam = matchedPath.params.claimName; | ||
| const claimIdPart = hash.substring(0, hash.indexOf('/')); | ||
|
|
||
| return ( | ||
| <div | ||
| className={classnames('embed__wrapper', { | ||
| 'embed__wrapper--light-background': embedLightBackground, | ||
| 'embed__wrapper--channel': isChannel, | ||
| 'embed__wrapper--channel-notice': isChannel && latestClaimUrl === null, | ||
| })} | ||
| > | ||
| <EmbedContext.Provider value={{ setVideoEnded, videoEnded, fetchedLiveStatus: fetchedLiveStatus.current }}> | ||
| <EmbedClaimComponent uri={uri} latestClaimUrl={latestClaimUrl} /> | ||
| </EmbedContext.Provider> | ||
| </div> | ||
| ); | ||
| }; | ||
| if (newClaimNameParam && !newClaimNameParam.includes(claimIdPart)) { | ||
| matchedPath.params.claimName = newClaimNameParam + claimIdPart; | ||
| matchedPath.params.claimId = hash.substring(hash.indexOf('/') + 1); | ||
| } | ||
| } else { | ||
| // the hash is on the stream part of the uri, so it looks like | ||
| // "lbry://@chanelName:channelClaimId/streamName#streamClaimId" instead of | ||
| // "lbry://@chanelName:channelClaimId/streamName:streamClaimId" | ||
| matchedPath.params.claimId = (matchedPath.params.claimId || '') + hash; | ||
| } | ||
| } | ||
|
|
||
| export default EmbedWrapperPage; | ||
| return matchedPath; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Duplicated utility with fragile null handling.
Like getUriFromMatch, this buildMatchWithHash function is duplicated between files. Additionally, when match is null/undefined, line 120 creates an empty object without a params property, relying on downstream code (line 90's || {} fallback) to handle this gracefully. This makes the code fragile.
Consider:
- Extract to shared utility (same as previous comment)
- Initialize
paramsexplicitly whenmatchis null:
function buildMatchWithHash(match, hash) {
- const matchedPath = Object.assign({}, match || {});
+ const matchedPath = Object.assign({ params: {} }, match);This makes the function more robust and self-contained.
🤖 Prompt for AI Agents
In @ui/page/embedWrapper/view.jsx around lines 119 - 143, The buildMatchWithHash
function is duplicated and fragile when match is null because it creates an
object without params; extract buildMatchWithHash (and mirror getUriFromMatch)
into a shared utility module, update callers to import it, and inside the
extracted function ensure you initialize params explicitly (e.g.,
matchedPath.params = matchedPath.params || {}) before accessing/mutating
claimName/claimId so the function is self-contained and robust against
null/undefined match inputs.
Extends the embed component to support homepage, channels, playlists, and markdown content. For channel embeds, it now displays a grid of the latest uploads. Playlist embeds showcase the cover and a playable list. Adds support for rendering markdown content within embeds, fetching the content and displaying it. Also configures the homepage embed to point Mini App action to the homepage embed. Simplify unsaved settings warning logic remove comment Removed embed menu item to maintain focus on current changes. Improves embed and channel page handling Addresses issues related to embed path detection and homepage update loops. - Ensures correct embed path detection by checking for a valid pathname. - Prevents repeated 'isHome' updates on the homepage by using a bounded set, avoiding potential loops. - Extracts and memoizes the channel page wrapper component to prevent unnecessary re-renders. - Improves playlist ID detection for collection handling in embed wrappers. Improves embed and upload page handling Addresses several issues and improves the user experience. - Ensures the active uploads warning only appears when not on the upload page by checking the pathname more reliably. - Corrects collection redirection logic by adding a code block. - Removes duplicate URI building logic in embed wrappers. - Enhances Farcaster embed functionality by including the SDK and ready script only on embed pages to avoid unnecessary overhead on other pages. Improves Farcaster embed handling and OG image. Enhances Farcaster integration by adding an iframe check to the embed script, ensuring it only runs when not in a top-level browsing context. Updates the Open Graph image URL used for Farcaster frames to use the thumbnail card CDN URL if available, falling back to other defined URLs, which delivers a richer visual experience. Allows iframing for homepage Permits iframing of the homepage to support embedding in Farcaster miniapps, in addition to existing embed pages and API. This change enhances integration possibilities with external platforms. Adds debug logging for action URLs Adds console logs to output the generated fcActionUrl to aid in debugging mini app integrations on the homepage. This helps to verify the correct URL is being used, especially during development with different origins. Adds debug logging to build OG metadata Adds a console log statement to output the override options passed into the buildBasicOgMetadata function. This will aid in debugging issues related to Open Graph metadata generation during development. Improves embed player UI and URI handling Adds a dedicated CSS class for the reactions overlay in the embedded video player, improving styling and maintainability. Refactors URI handling in the embed wrapper to prioritize incoming URI for live/latest content, ensuring correct content resolution. Uses resolved URL in embed mode with live path In embed mode when viewing a live/latest stream, uses the resolved URL from lbry.tv instead of the channel URL. This ensures the correct stream is displayed when the user navigates from a live stream to its archived version. Enables playlist support in embed mode Adds support for playlist URLs in embed mode by redirecting to the first item in the playlist with a collection ID parameter. This allows users to view playlists in an embedded player, providing a better user experience. It also handles the initial loading state for playlist URLs. Additionally, the middleware is updated to allow iframing for playlist pages. Fixes channel page navigation in embed mode Ensures correct URL construction for channel pages when viewed in embed mode. It checks if the current path is within the embed context and prepends `/$/embed` to the base URL. This adjustment ensures proper navigation and link generation within the embedded player.
tzarebczan
force-pushed
the
embed-improvements
branch
from
a8d286a to
3ec5546
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @ui/page/embedWrapper/internal/embedClaimComponent/view.jsx:
- Around line 190-193: The PropTypes for EmbeddedVideoClaimComponent incorrectly
declare streamClaim as PropTypes.func; update the prop-type for streamClaim on
EmbeddedVideoClaimComponent.propTypes to PropTypes.object or a more specific
PropTypes.shape that matches the resolved stream claim shape provided by
withStreamClaimRender HOC (e.g., include expected keys like value, name, etc.),
ensuring the prop type reflects the actual object passed rather than a function.
In @ui/page/embedWrapper/view.jsx:
- Around line 21-29: Remove the Flow type declaration "type Props = { ... }"
from this .jsx file (the standalone type Props block) because the file is not
Flow/TS-enabled; keep the existing runtime PropTypes definitions for the
component (referenced as PropTypes in the file) intact so prop validation
continues to work, and ensure no remaining references to the removed "Props"
symbol remain in the component code.
In @web/src/oEmbed.js:
- Around line 135-174: When playlist resolution fails (the playlist handling
block that uses collectionClaimId, Lbry.claim_search, and returns
generateOEmbedData/generateXmlData on success), do not return a plain string;
instead set an appropriate HTTP status on ctx (e.g., ctx.status = 400 or 404)
and return a JSON error object (e.g., { error: 'The URL is invalid or the
playlist is empty.' }) so the failure response type matches the successful JSON
responses; ensure the XML branch still sets Content-Type when format==='xml' and
keep error details minimal for clients.
🧹 Nitpick comments (8)
ui/component/app/view.jsx (1)
228-244: Remove redundant variable shadowing.Line 230 declares a new
pathnamevariable that shadows the existing one from line 160 (const { pathname, hash, search } = location;). Sincelocationis always provided bywithRouter, the defensive check is unnecessary—just reuse the outerpathname.♻️ Suggested fix
// Active uploads warning (show globally so users know why the browser prompts on leave) if (uploadCount > 0 && !embedPath) { - const pathname = location && location.pathname; const onUploadPage = (pathname && pathname.startsWith(`/$/${PAGES.UPLOAD}`)) || (pathname && pathname.startsWith(`/$/${PAGES.UPLOADS}`)); if (!onUploadPage) {web/index.js (1)
33-51: LGTM! Routing order control is well-implemented.The conditional routing based on
DYNAMIC_ROUTES_FIRSTprovides flexibility for development (dynamic routes first for meta injection) vs production (static first for safety). The comments clearly explain the rationale.Consider extracting the
serveoptions to avoid duplication:♻️ Optional: Extract serve options to reduce duplication
const DIST_ROOT = path.resolve(__dirname, 'dist'); +const STATIC_SERVE_OPTIONS = { maxage: 3600000 }; // ... middleware ... if (config.DYNAMIC_ROUTES_FIRST) { // Route dynamic pages first so we can inject proper meta (/, embeds, etc) app.use(router.routes()); // Then fall through to static assets (e.g. /public/*) - app.use( - serve(DIST_ROOT, { - maxage: 3600000, - }) - ); + app.use(serve(DIST_ROOT, STATIC_SERVE_OPTIONS)); } else { // Default: serve static first (production-safe), then dynamic routes - app.use( - serve(DIST_ROOT, { - maxage: 3600000, - }) - ); + app.use(serve(DIST_ROOT, STATIC_SERVE_OPTIONS)); app.use(router.routes()); }ui/page/claim/view.jsx (1)
71-83: Consider memoizingChannelPageEditingWrapperLocal.Unlike
ChannelPageWrapper, this wrapper is defined inline within the render path, creating a new component identity on every render whenediting=true. While the editing flow may be infrequent enough that this doesn't matter in practice, memoizing it (similar toChannelPageWrapper) would be more consistent and avoid potential unnecessary child remounts.♻️ Suggested refactor
+ const ChannelPageEditingWrapper = React.useMemo( + () => + ({ children }: { children: any }) => + ( + <Page + className="channelPage-wrapper channelPage-edit-wrapper" + noFooter + fullWidthPage + noSideNavigation={isEmbedPath} + > + {children} + </Page> + ), + [isEmbedPath] + ); if (isChannel) { const urlParams = new URLSearchParams(search); const editing = urlParams.get(CHANNEL_PAGE.QUERIES.VIEW) === CHANNEL_PAGE.VIEWS.EDIT; if (editing) { - const ChannelPageEditingWrapperLocal = ({ children }: { children: any }) => ( - <Page - className="channelPage-wrapper channelPage-edit-wrapper" - noFooter - fullWidthPage - noSideNavigation={isEmbedPath} - > - {children} - </Page> - ); - return <ClaimPageComponent uri={uri} ClaimRenderWrapper={ChannelPageEditingWrapperLocal} Wrapper={Page} />; + return <ClaimPageComponent uri={uri} ClaimRenderWrapper={ChannelPageEditingWrapper} Wrapper={Page} />; }web/src/html.js (1)
558-599: Consider extracting shared playlist resolution logic.The playlist resolution logic here (lines 570-596) is similar to
web/src/oEmbed.jslines 136-173. While the contexts differ slightly (one for OG metadata, one for oEmbed responses), the core claim resolution pattern could potentially be extracted into a shared helper to reduce duplication.This is a low-priority observation given the different return value requirements.
ui/hocs/withStreamClaimRender/view.jsx (1)
17-19: Module-level singleton Set may cause unexpected re-triggers in long sessions.The
homeInitFlagsSet is defined at module scope, shared across all component instances throughout the browser session. While bounded at 100 URIs, the clear-on-full strategy (line 236) means after 100 unique URIs are processed, the entire Set clears and previously seen URIs can re-triggerupdateClaimcalls.For the current use case (homepage-only deduplication via
isHomecheck on line 230), this is likely acceptable since typical homepage sessions show fewer than 100 unique URIs. However, for longer sessions or if expanded beyond homepage, consider switching to a per-instanceuseRef-based approach to avoid potential re-triggers across the 100-URI boundary.ui/page/embedWrapper/index.js (1)
165-179: Consider logging errors in buildMatchFromPath for debugging.The empty catch block silently swallows all exceptions during path parsing, which could make debugging difficult in production.
Optional: Add minimal error logging
try { if (!pathname) return matchedPath; const parts = pathname.split('/').filter(Boolean); // Expect ['$', 'embed', <claimName>, <claimId>?] const embedIdx = parts.indexOf('embed'); if (embedIdx > -1) { const claimName = decodeURIComponent(parts[embedIdx + 1] || ''); const claimId = decodeURIComponent(parts[embedIdx + 2] || ''); matchedPath.params.claimName = claimName; if (claimId) matchedPath.params.claimId = claimId; } - } catch (e) {} + } catch (e) { + console.error('buildMatchFromPath: failed to parse pathname', pathname, e); + }ui/page/embedWrapper/internal/embedClaimComponent/view.jsx (1)
92-107: Consider extracting inline styles to CSS classes.The inline styles for the "Recent Content" section work but could be moved to CSS for consistency with the rest of the codebase.
ui/page/embedWrapper/view.jsx (1)
134-189:getUriFromMatchandbuildMatchWithHashare duplicated fromindex.js.These helper functions are nearly identical to those in
ui/page/embedWrapper/index.js(lines 95-163). This duplication can lead to maintenance issues if one copy is updated but not the other.Consider extracting these helpers to a shared utility file:
// e.g., ui/page/embedWrapper/helpers.js export function getUriFromMatch(match) { ... } export function buildMatchWithHash(match, hash) { ... }Then import in both
index.jsandview.jsx.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
.gitignoreconfig.jspackage.jsonui/component/app/view.jsxui/component/claimTilesDiscover/view.jsxui/component/embedTextArea/view.jsxui/component/fileActions/view.jsxui/component/header/view.jsxui/component/page/view.jsxui/component/router/view.jsxui/component/sideNavigation/view.jsxui/component/socialShare/view.jsxui/component/subscribeButton/view.jsxui/component/viewers/videoViewer/internal/videojs.jsxui/component/viewers/videoViewer/view.jsxui/hocs/withResolvedClaimRender/helper.jsxui/hocs/withStreamClaimRender/index.jsui/hocs/withStreamClaimRender/view.jsxui/page/claim/internal/claimPageComponent/internal/channelPage/view.jsxui/page/claim/internal/claimPageComponent/view.jsxui/page/claim/view.jsxui/page/collection/index.jsui/page/collection/view.jsxui/page/embedWrapper/index.jsui/page/embedWrapper/internal/embedClaimComponent/index.jsui/page/embedWrapper/internal/embedClaimComponent/view.jsxui/page/embedWrapper/view.jsxui/scss/component/_claim-preview.scssui/scss/component/_embed-player.scssui/util/buildHomepage.jsweb/index.jsweb/middleware/iframe-destroyer.jsweb/src/farcaster.jsweb/src/html.jsweb/src/oEmbed.jsweb/src/routes.jsweb/src/tools/check-farcaster.js
🚧 Files skipped from review as they are similar to previous changes (17)
- ui/component/header/view.jsx
- ui/component/embedTextArea/view.jsx
- ui/page/collection/view.jsx
- ui/component/socialShare/view.jsx
- ui/page/collection/index.js
- ui/component/router/view.jsx
- web/src/routes.js
- ui/component/fileActions/view.jsx
- ui/component/page/view.jsx
- package.json
- .gitignore
- ui/component/sideNavigation/view.jsx
- web/src/farcaster.js
- config.js
- web/src/tools/check-farcaster.js
- ui/scss/component/_claim-preview.scss
- ui/scss/component/_embed-player.scss
🧰 Additional context used
🧬 Code graph analysis (12)
ui/component/viewers/videoViewer/internal/videojs.jsx (2)
ui/component/app/view.jsx (1)
path(187-187)ui/hocs/withResolvedClaimRender/helper.jsx (1)
path(34-34)
web/middleware/iframe-destroyer.js (2)
web/src/html.js (1)
PAGES(26-26)web/src/oEmbed.js (1)
PAGES(2-2)
ui/page/embedWrapper/internal/embedClaimComponent/index.js (5)
ui/hocs/withStreamClaimRender/index.js (4)
select(71-106)props(72-72)claim(74-74)perform(108-115)ui/page/embedWrapper/index.js (9)
select(16-83)state(17-17)state(19-19)claim(34-34)uri(22-22)channelClaimId(39-39)isCollection(62-62)collectionId(63-63)perform(85-91)ui/component/claimTilesDiscover/view.jsx (1)
props(79-100)ui/component/embedTextArea/view.jsx (2)
props(22-23)isCollection(27-27)ui/page/embedWrapper/internal/embedClaimComponent/view.jsx (1)
EmbedClaimComponent(34-183)
ui/hocs/withStreamClaimRender/view.jsx (5)
ui/component/header/view.jsx (1)
urlParams(106-106)ui/component/viewers/videoViewer/view.jsx (1)
urlParams(181-181)ui/redux/actions/content.js (1)
urlParams(195-195)ui/redux/selectors/content.js (2)
urlParams(115-115)uri(94-94)ui/util/url.js (1)
COLLECTIONS_CONSTS(9-9)
ui/page/claim/view.jsx (2)
ui/component/header/view.jsx (2)
isEmbedPath(120-120)urlParams(106-106)ui/page/claim/internal/claimPageComponent/view.jsx (1)
urlParams(60-60)
ui/hocs/withResolvedClaimRender/helper.jsx (3)
ui/page/embedWrapper/index.js (1)
uri(22-22)ui/page/embedWrapper/view.jsx (2)
uri(57-57)uri(91-91)ui/component/viewers/videoViewer/internal/videojs.jsx (1)
path(398-398)
ui/component/viewers/videoViewer/view.jsx (2)
ui/component/fileReactions/view.jsx (1)
FileReactions(28-77)ui/component/viewers/videoViewer/index.js (1)
uri(49-49)
web/index.js (2)
config.js (1)
config(5-120)web/src/routes.js (2)
config(2-2)router(18-18)
ui/page/claim/internal/claimPageComponent/internal/channelPage/view.jsx (3)
ui/page/embedWrapper/view.jsx (1)
useHistory(42-45)ui/component/header/view.jsx (1)
isEmbedPath(120-120)ui/page/claim/view.jsx (1)
isEmbedPath(27-27)
ui/component/subscribeButton/view.jsx (4)
ui/component/sideNavigation/view.jsx (1)
isEmbed(37-37)ui/page/claim/internal/claimPageComponent/view.jsx (1)
isEmbed(69-69)ui/page/embedWrapper/index.js (1)
uri(22-22)ui/page/claim/internal/claimPageComponent/internal/channelPage/view.jsx (1)
permanentUrl(137-137)
web/src/html.js (2)
web/src/frame.js (6)
image(66-66)response(44-44)response(58-58)response(88-92)claim(45-45)claim(59-59)web/src/oEmbed.js (7)
collectionClaimId(138-138)collectionClaim(142-142)firstItemClaimId(143-143)response(24-24)firstItemClaim(147-147)claim(22-22)claim(33-33)
web/src/oEmbed.js (3)
web/middleware/iframe-destroyer.js (1)
PAGES(1-1)web/src/html.js (7)
PAGES(26-26)require(1-13)require(14-20)require(21-21)require(23-23)require(24-24)require(29-29)ui/component/fileActions/view.jsx (1)
collectionId(101-101)
🪛 Biome (2.1.2)
ui/hocs/withStreamClaimRender/view.jsx
[error] 19-19: type annotation are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
ui/page/claim/view.jsx
[error] 31-31: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 58-58: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 72-72: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
ui/hocs/withResolvedClaimRender/helper.jsx
[error] 39-39: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 39-39: expected , but instead found ?
Remove ?
(parse)
[error] 39-39: expected , but instead found UriAccessKey
Remove UriAccessKey
(parse)
ui/page/embedWrapper/view.jsx
[error] 21-29: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
[error] 31-31: Type annotations are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
⏰ 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: lint
🔇 Additional comments (41)
web/middleware/iframe-destroyer.js (1)
9-18: LGTM! Expanded iframe allowlist for embed functionality.The logic correctly consolidates iframe permission checks into a single
allowIframevariable. The security implications of allowing these specific paths to be iframed are intentional per the PR objectives (Farcaster miniapp, embed/playlist support).PAGES.PLAYLISTis properly defined inui/constants/pages.js.ui/component/claimTilesDiscover/view.jsx (1)
119-122: LGTM!The condition extension to include
/$/embed/homeis a sensible addition that ensures consistent pin injection behavior between the main homepage and the embed homepage. The clarifying comment explains the intent well.ui/component/viewers/videoViewer/view.jsx (2)
538-539: LGTM!The hover state management for the reactions overlay is clean and straightforward.
Also applies to: 553-554
643-648: Good UX for embedded reactions.The conditional logic ensures reactions are accessible (on hover or when paused) without being intrusive during playback. Showing only for authenticated users is correct since reactions require authentication.
ui/util/buildHomepage.js (2)
46-49: LGTM!The fallback to
cat.idswhencat.channelIdsis not available improves flexibility for categories that may use different ID field names. The truthy check on line 47 safely handles undefined or empty arrays.
75-78: LGTM!Using a local
channelIdsvariable improves readability and ensures the URL parameter is only set whenchannelIdsis defined. This defensive approach prevents empty or undefined values from being serialized into the URL.Also applies to: 116-116
ui/component/viewers/videoViewer/internal/videojs.jsx (1)
396-401: [Rewritten review comment]
[Exactly ONE classification tag]ui/component/subscribeButton/view.jsx (2)
93-108: LGTM - Embed-aware click handling.The ternary approach explicitly returns
undefinedwhen embedded, which is cleaner than the short-circuit AND pattern. Combined with thehreffallback on line 92, this ensures users are redirected to sign-in rather than triggering subscription actions directly in embed context.
127-142: LGTM - Consistent pattern with first button.Same embed-aware logic applied consistently to the second subscribe button variant.
ui/page/claim/internal/claimPageComponent/internal/channelPage/view.jsx (3)
112-114: LGTM - Embed path detection.Consistent with the embed detection pattern used across other components in this PR (e.g.,
ui/component/header/view.jsx,ui/page/claim/view.jsx).
265-268: LGTM - Tab navigation preserves embed context.The URL construction correctly prefixes with
/$/embedwhen in embed mode, ensuring tab changes don't break out of the embed context.
331-338: LGTM - Effect dependency includes isEmbedPath.The shorts redirect effect correctly includes
isEmbedPathin its dependency array and uses the same URL construction pattern.ui/hocs/withResolvedClaimRender/helper.jsx (3)
30-36: LGTM - Early returns and embed guard.The embed path guard prevents unnecessary URL manipulation in embed contexts. The try/catch is appropriately defensive since
window.locationaccess could theoretically throw in unusual environments.
38-53: Good optimization - diff-check before URL update.Comparing current URL params against new values before calling
history.replaceStateavoids unnecessary address bar churn. This is a sensible optimization.
54-57: Dependency omission is documented and intentional.Omitting
doFetchUriAccessKeyfrom deps is valid since it's a stable Redux action creator that won't change identity. The eslint-disable with explanation is appropriate.ui/page/claim/view.jsx (3)
26-27: LGTM - Embed path detection.Consistent pattern with other files in the PR.
33-42: LGTM - Embed-aware markdown rendering.The
noSideNavigationprop is correctly applied only when both in embed mode and rendering a markdown post. Dependencies are correctly specified.
56-65: LGTM - Memoized ChannelPageWrapper.Correctly memoized with
isEmbedPathas a dependency.web/src/oEmbed.js (1)
47-66: LGTM - Extended function signature with collection support.The
collectionIdparameter is optional (backward compatible), and the URL parameter construction correctly handles the?vs&separator logic.web/src/html.js (7)
91-126: LGTM - Farcaster embed SDK initialization.The polling approach (50 attempts × 100ms = 5s max) is reasonable for SDK initialization. The
requireIframeoption correctly gates script execution to iframe contexts when needed.
131-133: LGTM - Extended buildOgMetadata signature.The new
fcActionUrlandisEmbedparameters allow embed-aware metadata generation while maintaining backward compatibility via destructuring defaults.
163-210: LGTM - Farcaster meta tags use fcActionUrl.The
fcActionUrlis correctly threaded through miniapp, frame, and legacy frame meta tags, with appropriate fallback tourlwhen not provided.
247-250: LGTM - Helper function for basic OG metadata.Simple delegation to
buildOgMetadatawith proper header/footer wrapping.
433-436: LGTM - Conditional SDK script injection.SDK and ready script are only injected for actual embed pages (
isEmbed: true), avoiding unnecessary overhead on regular claim pages.
515-521: LGTM - Homepage embed action URL.Using
ctx.originensures the fcActionUrl works correctly in development environments without hardcoding production URLs.
618-653: LGTM - Playlist page metadata with fcActionUrl.The playlist page correctly resolves the first item and builds an fcActionUrl pointing to the first item's embed with the
lidparameter. Fallback to collection claim metadata when first item isn't available is sensible.ui/hocs/withStreamClaimRender/view.jsx (3)
136-139: LGTM - Support for 'lid' URL parameter.Adding
urlParams.get('lid')as a fallback aligns with the playlist embed URL structure used elsewhere in the PR (e.g.,web/src/html.jsline 583).
230-241: LGTM - Home detection and bounded guard.Expanding
isHometo include/$/embed/homeis consistent with the homepage embed feature. The bounded Set guard prevents repeatedupdateClaim('isHome')calls for the same URI.
269-277: LGTM - Markdown source fetching for embeds.This effect ensures markdown posts fetch their source content when embedded, enabling proper rendering. The guard conditions (
canViewFile,renderMode === 'md',!streamingUrl) are appropriate.ui/page/claim/internal/claimPageComponent/view.jsx (3)
69-73: Embed-aware URI resolution looks correct.The
isEmbeddetection andeffectiveUriderivation properly handle the edge case where embed mode with live/latest paths should use the resolvedlatestClaimUrlinstead of the channel URI. This ensures embedded content displays the actual stream rather than the channel page.
87-90: Early return prevents unwanted URL rewrites in embed mode.Correctly bypasses the
history.replaceStatelogic when embedded, preserving the/$/embed/...URL structure. The dependency array at line 125 properly includesisEmbed.
148-179: Redirect logic correctly gated for non-embed mode.Wrapping all redirect cases (latest content, reposts, collections) in
!isEmbedensures embed URLs remain stable and don't redirect users away from the embedded context.ui/hocs/withStreamClaimRender/index.js (1)
38-69: Good performance optimization for reducing unnecessary re-renders.The custom
areStatePropsEqualfunction properly handles the frequently-changingplayingUriobject by comparing only the fields actually used (uri,sourceId,collection.collectionId), preventing re-renders when unrelated nested properties change. The fallback to|| {}and|| undefinedhandles null/undefined cases correctly.ui/page/embedWrapper/internal/embedClaimComponent/index.js (2)
16-32: Redux wiring correctly expanded for embed functionality.The selector properly derives
channelClaimIdusinggetChannelIdFromClaim(handles both channel pages and signed streams),isCollectionfrom claim type, and conditionally fetchescollectionUrlsonly whencollectionIdis present. Clean and efficient.
35-40: Action wiring enables file and collection fetching for embeds.The
performobject correctly exposesdoFileGetForUri(for markdown content loading) anddoFetchItemsInCollection(for playlist rendering).ui/page/embedWrapper/index.js (2)
27-32: Playlist URL detection via regex is appropriate.The pattern
/\/\$\/(?:embed\/)?playlist\/([a-f0-9]{40})/icorrectly matches both/$/playlist/:idand/$/embed/playlist/:idformats, capturing the 40-character hex claim ID.
95-106: Good defensive guards for URI derivation.The early returns for missing
claimNameand the special-case handling for'home'prevent incorrect URI construction and avoid resolving the embed home route as a claim.ui/page/embedWrapper/internal/embedClaimComponent/view.jsx (2)
66-71: Effect placement before conditionals is correct.Following React's rules of hooks, the collection fetch effect is properly placed before any conditional returns.
195-240: EmbeddedMarkdown component has proper async handling.The component correctly:
- Triggers file fetch when
streamingUrlis missing- Uses a
cancelledflag to prevent state updates after unmount- Distinguishes loading (
undefined), error (null), and loaded (string) statesui/page/embedWrapper/view.jsx (2)
74-88: Playlist redirect and loading logic is well-structured.The redirect to first item with
COLLECTION_IDparameter for playlist paths, combined with the loading spinner while awaitingcollectionFirstItemUri, provides a good UX flow.
110-116: Rendering delegation to appropriate page components looks correct.The conditional rendering properly routes:
- Collections (non-playlist path) →
CollectionPage- Page-like content (channels, collections, markdown) →
ClaimPage- Simple embeds →
EmbedClaimComponent
|
|
||
| type Props = { | ||
| uri: string, | ||
| claimId: string, | ||
| latestClaimId: ?string, | ||
| canonicalUrl: ?string, | ||
| channelUri: ?string, | ||
| channelClaimId: ?string, | ||
| isLivestreamClaim: boolean, | ||
| latestClaimUrl: ?string, | ||
| doFetchLatestClaimForChannel: (uri: string, isEmbed: boolean) => void, | ||
| doFetchChannelIsLiveForId: (string) => void, | ||
| doCommentSocketConnect: (string, string, string, ?string) => void, | ||
| doCommentSocketDisconnect: (string, string) => void, | ||
| contentUnlocked: boolean, | ||
| uri?: string, | ||
| collectionId?: string, | ||
| collectionFirstItemUri?: string, | ||
| isCollection?: boolean, | ||
| renderMode?: string, | ||
| doFetchItemsInCollection?: ({ collectionId: string }) => void, | ||
| }; |
There was a problem hiding this comment.
Flow type syntax causes static analysis errors.
Biome reports parse errors for the type Props declaration since the file is .jsx, not .tsx or Flow-enabled. Since PropTypes are already defined at lines 123-130, consider removing the Flow type annotation to eliminate the static analysis errors.
Proposed fix: Remove Flow types, keep PropTypes
-type Props = {
- uri?: string,
- collectionId?: string,
- collectionFirstItemUri?: string,
- isCollection?: boolean,
- renderMode?: string,
- doFetchItemsInCollection?: ({ collectionId: string }) => void,
-};
-
-const EmbedWrapperPage = (props: Props) => {
+const EmbedWrapperPage = (props) => {🧰 Tools
🪛 Biome (2.1.2)
[error] 21-29: type alias are a TypeScript only feature. Convert your file to a TypeScript file or remove the syntax.
TypeScript only syntax
(parse)
🤖 Prompt for AI Agents
In @ui/page/embedWrapper/view.jsx around lines 21 - 29, Remove the Flow type
declaration "type Props = { ... }" from this .jsx file (the standalone type
Props block) because the file is not Flow/TS-enabled; keep the existing runtime
PropTypes definitions for the component (referenced as PropTypes in the file)
intact so prop validation continues to work, and ensure no remaining references
to the removed "Props" symbol remain in the component code.
Improves oEmbed error responses by returning a 400 status and a JSON object with an error message when the URL is invalid or the playlist is empty. Corrects the prop type definition for `streamClaim` in the `EmbeddedVideoClaimComponent` to accept an object instead of a function. This aligns with the expected data structure and resolves a potential type mismatch.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @web/src/oEmbed.js:
- Around line 140-171: Replace the empty catch at the end of the try block with
error logging and a proper 400 response: catch the error from the
Lbry.claim_search/generateOEmbedData flow (the try surrounding calls to
Lbry.claim_search, generateOEmbedData, and generateXmlData), log the error with
useful context (e.g., collectionClaimId, decodedQueryUri, requestUrl) via the
existing logger or console, and then return a 400 response to the client (e.g.,
set ctx.status = 400 and a minimal error body or use ctx.throw(400, ...)) so
failures are observable instead of silently swallowed.
🧹 Nitpick comments (5)
ui/page/embedWrapper/internal/embedClaimComponent/view.jsx (4)
92-107: Consider using CSS classes instead of inline styles.The inline styles for the "Recent Content" section work but deviate from the pattern used elsewhere in the codebase (e.g.,
className="help--notice"). For consistency and maintainability, consider moving these styles to a CSS class.
185-193: The ESLint disable comment is redundant.Since
PropTypesare defined immediately after the component (lines 190-193), the// eslint-disable-next-line react/prop-typescomment on line 185 is unnecessary. The linter should be satisfied by the PropTypes definition.♻️ Suggested fix
-// eslint-disable-next-line react/prop-types const EmbeddedVideoClaimComponent = ({ uri, streamClaim }) => ( <VideoRender uri={uri} embedded streamClaim={streamClaim} /> );
206-223: Consider adding a timeout for the fetch request.The fetch operation has no timeout configured. If the server is slow or unresponsive, the component will remain in the loading state indefinitely. Consider using
AbortControllerwith a timeout to improve UX.♻️ Suggested implementation with timeout
React.useEffect(() => { let cancelled = false; + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 15000); // 15s timeout + async function load() { try { if (!streamingUrl) return setContent(undefined); - const res = await fetch(streamingUrl, { credentials: 'omit' }); + const res = await fetch(streamingUrl, { credentials: 'omit', signal: controller.signal }); if (!res.ok) return setContent(null); const text = await res.text(); if (!cancelled) setContent(text); } catch (e) { if (!cancelled) setContent(null); } } load(); return () => { cancelled = true; + clearTimeout(timeoutId); + controller.abort(); }; }, [streamingUrl]);
195-197: Redundant ESLint disable comment.Same as
EmbeddedVideoClaimComponent, the// eslint-disable-next-line react/prop-typesis unnecessary since PropTypes are defined at lines 236-240.web/src/oEmbed.js (1)
161-168: Consider extracting format handling to reduce duplication.The format/content-type logic here duplicates lines 186-194. A helper function could reduce this repetition.
Example helper extraction
function sendOEmbedResponse(ctx, oEmbedData, requestUrl) { const formatQuery = getParameterByName('format', requestUrl); if (formatQuery === 'xml') { ctx.set('Content-Type', 'application/xml'); return generateXmlData(oEmbedData); } ctx.set('Content-Type', 'application/json'); return oEmbedData; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ui/page/embedWrapper/internal/embedClaimComponent/view.jsxweb/src/oEmbed.js
🧰 Additional context used
🧬 Code graph analysis (2)
web/src/oEmbed.js (3)
web/middleware/iframe-destroyer.js (1)
PAGES(1-1)web/src/html.js (7)
PAGES(26-26)require(1-13)require(14-20)require(21-21)require(23-23)require(24-24)require(29-29)ui/component/fileActions/view.jsx (1)
collectionId(101-101)
ui/page/embedWrapper/internal/embedClaimComponent/view.jsx (3)
ui/hocs/withStreamClaimRender/index.js (1)
props(72-72)ui/page/embedWrapper/internal/embedClaimComponent/index.js (2)
isCollection(22-22)channelClaimId(21-21)ui/page/embedWrapper/index.js (4)
isCollection(62-62)collectionId(63-63)channelClaimId(39-39)uri(22-22)
⏰ 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: lint
🔇 Additional comments (9)
ui/page/embedWrapper/internal/embedClaimComponent/view.jsx (6)
1-17: LGTM!The imports are well-organized, and the
@noflowdirective appropriately disables Flow since PropTypes are now used for runtime validation.
27-30: LGTM!The lazy-loaded imports for
ClaimListDiscoverandClaimListfollow the existing pattern and appropriately chunk these components for better code splitting.
66-71: LGTM!The
useEffectfor fetching collection items is correctly placed before conditional returns (respecting hooks rules) and has appropriate guards and dependencies.
118-144: LGTM!The playlist/collection embed correctly handles all three states: loading (
undefined), populated (array with items), and empty (empty array). The tri-state conditional rendering is well-structured.
242-246: LGTM!Clean implementation with PropTypes properly defined.
250-263: LGTM!PropTypes are comprehensive and cover all props used by the component. The placement after the export statement is unconventional but functionally correct.
web/src/oEmbed.js (3)
2-2: LGTM!Import is necessary for the playlist path detection logic below.
47-66: LGTM!The function signature extension is backwards-compatible and the URL construction logic correctly handles query parameter chaining. The
lidparameter doesn't need URL encoding since the source value is constrained to hex characters by the regex at line 138.
173-176: LGTM!Proper HTTP 400 response with JSON error body for invalid/empty playlist cases. The generic error message is appropriate for an external-facing API.
| try { | ||
| const collectionResponse = await Lbry.claim_search({ claim_ids: [collectionClaimId] }); | ||
| const collectionClaim = collectionResponse?.items?.at(0); | ||
| const firstItemClaimId = collectionClaim?.value?.claims?.[0]; | ||
|
|
||
| if (firstItemClaimId) { | ||
| const firstItemResponse = await Lbry.claim_search({ claim_ids: [firstItemClaimId] }); | ||
| const firstItemClaim = firstItemResponse?.items?.at(0); | ||
|
|
||
| if (firstItemClaim) { | ||
| const queryTimestampParam = getParameterByName('t', decodedQueryUri); | ||
| const queryReferralParam = getParameterByName('r', decodedQueryUri); | ||
| // Pass collectionClaimId as lid parameter for playlist context | ||
| const oEmbedData = generateOEmbedData( | ||
| firstItemClaim, | ||
| embedlyReferrer, | ||
| queryTimestampParam, | ||
| queryReferralParam, | ||
| collectionClaimId | ||
| ); | ||
|
|
||
| const formatQuery = getParameterByName('format', requestUrl); | ||
| if (formatQuery === 'xml') { | ||
| ctx.set('Content-Type', 'application/xml'); | ||
| return generateXmlData(oEmbedData); | ||
| } | ||
|
|
||
| ctx.set('Content-Type', 'application/json'); | ||
| return oEmbedData; | ||
| } | ||
| } | ||
| } catch {} |
There was a problem hiding this comment.
Silent catch block hides failures and complicates debugging.
The empty catch {} at line 171 swallows all errors (network failures, API timeouts, malformed responses) and treats them identically to an empty playlist. This makes it difficult to diagnose issues in production.
Consider logging the error for observability while still returning the 400 response to the client.
Proposed fix
- } catch {}
+ } catch (err) {
+ console.error('Playlist oEmbed resolution failed:', err);
+ }📝 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.
| try { | |
| const collectionResponse = await Lbry.claim_search({ claim_ids: [collectionClaimId] }); | |
| const collectionClaim = collectionResponse?.items?.at(0); | |
| const firstItemClaimId = collectionClaim?.value?.claims?.[0]; | |
| if (firstItemClaimId) { | |
| const firstItemResponse = await Lbry.claim_search({ claim_ids: [firstItemClaimId] }); | |
| const firstItemClaim = firstItemResponse?.items?.at(0); | |
| if (firstItemClaim) { | |
| const queryTimestampParam = getParameterByName('t', decodedQueryUri); | |
| const queryReferralParam = getParameterByName('r', decodedQueryUri); | |
| // Pass collectionClaimId as lid parameter for playlist context | |
| const oEmbedData = generateOEmbedData( | |
| firstItemClaim, | |
| embedlyReferrer, | |
| queryTimestampParam, | |
| queryReferralParam, | |
| collectionClaimId | |
| ); | |
| const formatQuery = getParameterByName('format', requestUrl); | |
| if (formatQuery === 'xml') { | |
| ctx.set('Content-Type', 'application/xml'); | |
| return generateXmlData(oEmbedData); | |
| } | |
| ctx.set('Content-Type', 'application/json'); | |
| return oEmbedData; | |
| } | |
| } | |
| } catch {} | |
| try { | |
| const collectionResponse = await Lbry.claim_search({ claim_ids: [collectionClaimId] }); | |
| const collectionClaim = collectionResponse?.items?.at(0); | |
| const firstItemClaimId = collectionClaim?.value?.claims?.[0]; | |
| if (firstItemClaimId) { | |
| const firstItemResponse = await Lbry.claim_search({ claim_ids: [firstItemClaimId] }); | |
| const firstItemClaim = firstItemResponse?.items?.at(0); | |
| if (firstItemClaim) { | |
| const queryTimestampParam = getParameterByName('t', decodedQueryUri); | |
| const queryReferralParam = getParameterByName('r', decodedQueryUri); | |
| // Pass collectionClaimId as lid parameter for playlist context | |
| const oEmbedData = generateOEmbedData( | |
| firstItemClaim, | |
| embedlyReferrer, | |
| queryTimestampParam, | |
| queryReferralParam, | |
| collectionClaimId | |
| ); | |
| const formatQuery = getParameterByName('format', requestUrl); | |
| if (formatQuery === 'xml') { | |
| ctx.set('Content-Type', 'application/xml'); | |
| return generateXmlData(oEmbedData); | |
| } | |
| ctx.set('Content-Type', 'application/json'); | |
| return oEmbedData; | |
| } | |
| } | |
| } catch (err) { | |
| console.error('Playlist oEmbed resolution failed:', err); | |
| } |
🤖 Prompt for AI Agents
In @web/src/oEmbed.js around lines 140 - 171, Replace the empty catch at the end
of the try block with error logging and a proper 400 response: catch the error
from the Lbry.claim_search/generateOEmbedData flow (the try surrounding calls to
Lbry.claim_search, generateOEmbedData, and generateXmlData), log the error with
useful context (e.g., collectionClaimId, decodedQueryUri, requestUrl) via the
existing logger or console, and then return a 400 response to the client (e.g.,
set ctx.status = 400 and a minimal error body or use ctx.throw(400, ...)) so
failures are observable instead of silently swallowed.
1 participant
Extends the embed component to support channels, playlists, and markdown content.
For channel embeds, it now displays a grid of the latest uploads. Playlist embeds showcase the cover and a playable list. Adds support for rendering markdown content within embeds, fetching the content and displaying it.
Also configures the homepage embed to point Mini App action to the homepage embed.
Fixes up a locking up bug too that plagues some users with lots of viewing history.
Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.