[7418] VideoPicker control#4817
Draft
jvega190 wants to merge 8 commits into
Draft
Conversation
Contributor
WalkthroughAdds a new VideoPicker form control (lazy-registered) and a useVideoInfo hook that loads video dimensions/duration via a video element and fetches metadata (Content-Type, size) via HEAD requests. Changes
Sequence DiagramsequenceDiagram
participant User
participant VideoPicker
participant useVideoInfo
participant VideoElement
participant Server
User->>VideoPicker: Mount with video URL
VideoPicker->>useVideoInfo: request info(url)
par fetch dimensions
useVideoInfo->>VideoElement: create & set src
VideoElement-->>useVideoInfo: onloadedmetadata (width,height,duration)
and fetch metadata
useVideoInfo->>Server: HEAD request for headers
Server-->>useVideoInfo: Content-Type, Content-Length
end
useVideoInfo-->>VideoPicker: return videoInfo + loading/error states
VideoPicker->>User: render preview/controls
User->>VideoPicker: Click Replace
VideoPicker->>User: show datasource menu
User->>VideoPicker: Click Delete
VideoPicker->>VideoPicker: clear value, reset state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
ui/app/src/components/FormsEngine/controls/VideoPicker.tsx (3)
65-67:anchorElretains a stale DOM reference after the menu closes.
setAddMenuOpen(false)on line 138 hides the menu, butanchorElstill holds the previous button element. The standard MUI Menu pattern is to drive open state fromanchorElalone (i.e.open={Boolean(anchorEl)}), or at minimum clearanchorElon close. Also,addMenuButtonRef(line 66) is declared but only assigned — it's never read.Simplify by driving menu state from anchorEl only and drop unused ref
- const [anchorEl, setAnchorEl] = React.useState<null | HTMLElement>(null); - const addMenuButtonRef = useRef<HTMLButtonElement>(undefined); - const [addMenuOpen, setAddMenuOpen] = useState(false); + const [anchorEl, setAnchorEl] = useState<null | HTMLElement>(null); ... <Menu anchorEl={anchorEl} - open={addMenuOpen} - onClose={() => setAddMenuOpen(false)} + open={Boolean(anchorEl)} + onClose={() => setAnchorEl(null)} ... > ... <IconButton ... - ref={addMenuButtonRef} onClick={(event: React.MouseEvent<HTMLButtonElement>) => { setAnchorEl(event.currentTarget); - setAddMenuOpen(true); }} >Also applies to: 135-144
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/FormsEngine/controls/VideoPicker.tsx` around lines 65 - 67, The menu open state is kept separately and anchorEl is never cleared, leaving a stale DOM reference; remove the unused addMenuButtonRef, stop using addMenuOpen, and drive the MUI Menu open state from anchorEl (i.e. open={Boolean(anchorEl)}), setting anchorEl via setAnchorEl when opening and calling setAnchorEl(null) on close (replace uses of setAddMenuOpen(false) with setAnchorEl(null)); update any handlers that referenced addMenuOpen or addMenuButtonRef to use anchorEl instead.
73-127: Storing JSX elements (with event handlers) insideuseMemocan cause subtle issues.The
menuOptionsarray bakes JSX with a no-ophandleDataSourceOptionClickinto the memoized value. When the actual handler is implemented, it will likely need to reference state or callbacks that aren't in the dependency array, causing stale closures. Consider memoizing only the data (paths, types) and rendering the JSX in the return block instead. This also avoids thechildrenprop lint issue on the<Box>at line 227.Also,
allowedBrowsePathsandallowedUploadPathsare returned from the memo but never consumed outside of this memo — they can be dropped from the return value unless they're planned for future use.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/FormsEngine/controls/VideoPicker.tsx` around lines 73 - 127, The useMemo in VideoPicker currently returns JSX (menuOptions) that closes over a noop handleDataSourceOptionClick and may produce stale closures; change useMemo to only compute and return plain data: dataSourceIds, allowedBrowsePaths, allowedUploadPaths (based on field, contentType, contentTypes) and remove menuOptions from the memo return; create the actual handleDataSourceOptionClick and build the MenuItem JSX in the component render/return block (not inside useMemo) so the handler can reference current state/props and be included in its dependency list; also drop allowedBrowsePaths/allowedUploadPaths from the memo return if they are unused elsewhere, or keep them as plain data-only values used by the render logic.
107-107: Avoid passingchildrenas a prop (flagged by Biome).Lines 107, 117, and 227 use
children={...}as a prop attribute. This is flagged by Biome'snoChildrenProprule. Consider nesting the children instead.Proposed fix (example for lines 107 and 117)
- <ListItemText children={<FormattedMessage defaultMessage="Browse" />} /> + <ListItemText><FormattedMessage defaultMessage="Browse" /></ListItemText>- <ListItemText children={<FormattedMessage defaultMessage="Upload" />} /> + <ListItemText><FormattedMessage defaultMessage="Upload" /></ListItemText>For line 227, similarly nest
menuOptionsas a child of<Box>:- <Box - children={menuOptions} - sx={{...}} - /> + <Box sx={{...}}> + {menuOptions} + </Box>Also applies to: 117-117, 227-227
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/FormsEngine/controls/VideoPicker.tsx` at line 107, In VideoPicker.tsx replace any usage of the children prop (e.g., ListItemText children={<FormattedMessage .../>} at the ListItemText occurrences and Box children={menuOptions}) with nested JSX: put the <FormattedMessage .../> as the inner content of <ListItemText> and place the menuOptions value as the child inside <Box>, removing the children={...} prop; update all three flagged occurrences (ListItemText instances and the Box with menuOptions) so the components receive their children via nesting rather than the children prop.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/app/src/components/FormsEngine/controls/VideoPicker.tsx`:
- Line 168: The display suffix uses "Kb" which denotes kilobits; update the
string in VideoPicker.tsx where videoInfo?.size is rendered (the expression
using videoInfo.size) to use "KB" instead of "Kb" so it correctly shows
kilobytes; locate the rendering expression referencing videoInfo.size in the
VideoPicker component and replace the suffix accordingly.
- Around line 162-163: The error message in the VideoPicker control is a
copy-paste typo: change the FormattedMessage defaultMessage that currently reads
"Error loading image metadata" to "Error loading video metadata" in the
VideoPicker component (look for the Typography/FormattedMessage block inside
VideoPicker.tsx) so the copy reflects the control's media type.
In `@ui/app/src/hooks/useVideoInfo.ts`:
- Around line 53-95: The effect in useVideoInfo creates a video element and
starts a HEAD fetch but lacks cleanup and doesn't clear previous videoInfo when
url changes; fix by: when starting a new url, immediately reset merged state via
setVideoInfo(videoInfoInitialState) and
setIsFetchingDimensions/Metadata(true)/errors(null); create an AbortController
for the HEAD fetch and use it in fetch, and store both the video element and
controller in the effect scope so the cleanup function can abort the fetch,
remove/clear video event handlers (video.onloadedmetadata = null; video.onerror
= null), pause and remove the video (video.pause(); video.src = '';) to stop
loading, and ensure no stale callbacks call set* after cleanup; apply these
changes inside the effect that currently references video, fetch, setVideoInfo,
setIsFetchingDimensions, setIsFetchingMetadata, setErrorDimensions,
setErrorMetadata.
---
Nitpick comments:
In `@ui/app/src/components/FormsEngine/controls/VideoPicker.tsx`:
- Around line 65-67: The menu open state is kept separately and anchorEl is
never cleared, leaving a stale DOM reference; remove the unused
addMenuButtonRef, stop using addMenuOpen, and drive the MUI Menu open state from
anchorEl (i.e. open={Boolean(anchorEl)}), setting anchorEl via setAnchorEl when
opening and calling setAnchorEl(null) on close (replace uses of
setAddMenuOpen(false) with setAnchorEl(null)); update any handlers that
referenced addMenuOpen or addMenuButtonRef to use anchorEl instead.
- Around line 73-127: The useMemo in VideoPicker currently returns JSX
(menuOptions) that closes over a noop handleDataSourceOptionClick and may
produce stale closures; change useMemo to only compute and return plain data:
dataSourceIds, allowedBrowsePaths, allowedUploadPaths (based on field,
contentType, contentTypes) and remove menuOptions from the memo return; create
the actual handleDataSourceOptionClick and build the MenuItem JSX in the
component render/return block (not inside useMemo) so the handler can reference
current state/props and be included in its dependency list; also drop
allowedBrowsePaths/allowedUploadPaths from the memo return if they are unused
elsewhere, or keep them as plain data-only values used by the render logic.
- Line 107: In VideoPicker.tsx replace any usage of the children prop (e.g.,
ListItemText children={<FormattedMessage .../>} at the ListItemText occurrences
and Box children={menuOptions}) with nested JSX: put the <FormattedMessage .../>
as the inner content of <ListItemText> and place the menuOptions value as the
child inside <Box>, removing the children={...} prop; update all three flagged
occurrences (ListItemText instances and the Box with menuOptions) so the
components receive their children via nesting rather than the children prop.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ui/app/src/components/FormsEngine/controls/VideoPicker.tsxui/app/src/components/FormsEngine/lib/controlMap.tsui/app/src/hooks/useVideoInfo.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
ui/app/src/components/FormsEngine/controls/VideoPicker.tsx (1)
103-118: Avoid passing children via prop (Biome noChildrenProp).
This is flagged by the linter and is less idiomatic in React.♻️ Suggested refactor
- <ListItemText children={<FormattedMessage defaultMessage="Browse" />} /> + <ListItemText> + <FormattedMessage defaultMessage="Browse" /> + </ListItemText> ... - <ListItemText children={<FormattedMessage defaultMessage="Upload" />} /> + <ListItemText> + <FormattedMessage defaultMessage="Upload" /> + </ListItemText> ... - <Box - children={menuOptions} - sx={{ + <Box + sx={{ p: 1, gap: 1, py: 0.5, display: 'flex', flexDirection: 'row', flexWrap: 'wrap', color: 'primary.main', justifyContent: 'center', [`.${svgIconClasses.root}`]: { color: 'primary.main' }, [`.${menuItemClasses.root}`]: { flexDirection: 'column', justifyContent: 'center', borderRadius: 1 }, [`.${listItemIconClasses.root}`]: { justifyContent: 'center' } }} - /> + > + {menuOptions} + </Box>Also applies to: 225-249
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/FormsEngine/controls/VideoPicker.tsx` around lines 103 - 118, The linter flags passing children via the children prop on ListItemText in VideoPicker.tsx; change usages (e.g., the ListItemText inside the MenuItem blocks that call handleDataSourceOptionClick and the other occurrences around lines 225-249) to use normal JSX children instead of the children prop (move <FormattedMessage defaultMessage="Browse" /> and <FormattedMessage defaultMessage="Upload" /> between the ListItemText opening and closing tags); ensure all ListItemText components in this file are updated consistently so no component receives a children prop.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/app/src/components/FormsEngine/controls/VideoPicker.tsx`:
- Around line 73-95: The code in useMemo assumes contentTypes[contentType.id]
exists and will crash if it's undefined; update the useMemo callback (the block
that computes menuOptions, allowedBrowsePaths, allowedUploadPaths) to first
check whether contentTypes and contentTypes[contentType.id] are defined and
return default empty arrays/values when not ready, e.g., bail out early before
iterating over contentTypes[contentType.id].dataSources; adjust references to
contentTypes[contentType.id].dataSources inside that function (the same useMemo)
so they only run after the guard.
- Around line 98-114: The Browse/Upload menu items call
handleDataSourceOptionClick but that function is empty, leaving clickable
options that do nothing; implement handleDataSourceOptionClick to perform the
intended action (e.g., dispatch the data-source flow by invoking the existing
picker/open handler in this component such as openPicker, setPickerType, or
props.onOpenDataSource with the PickerType argument), or if that flow isn't
ready, remove the onClick and mark the MenuItem disabled or hide the options by
gating them off (use allowedBrowsePaths/allowedUploadPaths plus a
"datasourceReady" flag) so users can't click inert items; update the handler
referenced by handleDataSourceOptionClick and the MenuItem onClick usages
accordingly to restore expected behavior.
- Line 148: In VideoPicker, the preview URL is built by raw concatenation
(CardMedia component using image={`${guestBase}${value}`}) which can create
double slashes; update the preview construction to use the same normalization
helper as metadata (call ensureSingleSlash(guestBase, value) or equivalent) and
pass the normalized URL to CardMedia so both metadata and preview use the same
single-slash-normalized path.
In `@ui/app/src/hooks/useVideoInfo.ts`:
- Around line 83-94: In useVideoInfo, the HEAD fetch currently treats any HTTP
status as success; update the logic after awaiting fetch(url) to check
response.ok and handle non-OK responses by calling setErrorMetadata (or
throwing) and setting setIsFetchingMetadata(false) to avoid silent success;
additionally guard Content-Length parsing by verifying contentLength is a
numeric string before Number(...) and only compute sizeKb when valid, then call
setVideoInfo({ contentType, size: sizeKb }) for valid responses and ensure error
state is set for non-ok or invalid-length cases (refer to response, contentType,
contentLength, sizeKb, setVideoInfo, setErrorMetadata, setIsFetchingMetadata in
useVideoInfo).
---
Nitpick comments:
In `@ui/app/src/components/FormsEngine/controls/VideoPicker.tsx`:
- Around line 103-118: The linter flags passing children via the children prop
on ListItemText in VideoPicker.tsx; change usages (e.g., the ListItemText inside
the MenuItem blocks that call handleDataSourceOptionClick and the other
occurrences around lines 225-249) to use normal JSX children instead of the
children prop (move <FormattedMessage defaultMessage="Browse" /> and
<FormattedMessage defaultMessage="Upload" /> between the ListItemText opening
and closing tags); ensure all ListItemText components in this file are updated
consistently so no component receives a children prop.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ui/app/src/components/FormsEngine/controls/VideoPicker.tsxui/app/src/hooks/useVideoInfo.ts
Comment on lines
+98
to
+114
| const menuOptions = []; | ||
| // TODO: pending when datasources implementation is ready. | ||
| const handleDataSourceOptionClick = (event: ReactMouseEvent<HTMLLIElement, MouseEvent>, option: PickerType) => {}; | ||
| if (allowedBrowsePaths.length > 0) { | ||
| menuOptions.push( | ||
| <MenuItem key="search" onClick={(event) => handleDataSourceOptionClick(event, 'browse')}> | ||
| <ListItemIcon sx={{ mr: 0 }}> | ||
| <TravelExploreOutlined fontSize="small" /> | ||
| </ListItemIcon> | ||
| <ListItemText children={<FormattedMessage defaultMessage="Browse" />} /> | ||
| </MenuItem> | ||
| ); | ||
| } | ||
| if (allowedUploadPaths.length > 0) { | ||
| menuOptions.push( | ||
| <MenuItem key="upload" onClick={(event) => handleDataSourceOptionClick(event, 'upload')}> | ||
| <ListItemIcon sx={{ mr: 0 }}> |
Contributor
There was a problem hiding this comment.
Browse/Upload menu actions are no-ops.
handleDataSourceOptionClick is empty, so menu items appear clickable but do nothing. Either implement the handlers or temporarily disable/hide the options until datasource support is ready.
🧰 Tools
🪛 Biome (2.4.4)
[error] 107-107: Avoid passing children using a prop
(lint/correctness/noChildrenProp)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/app/src/components/FormsEngine/controls/VideoPicker.tsx` around lines 98 -
114, The Browse/Upload menu items call handleDataSourceOptionClick but that
function is empty, leaving clickable options that do nothing; implement
handleDataSourceOptionClick to perform the intended action (e.g., dispatch the
data-source flow by invoking the existing picker/open handler in this component
such as openPicker, setPickerType, or props.onOpenDataSource with the PickerType
argument), or if that flow isn't ready, remove the onClick and mark the MenuItem
disabled or hide the options by gating them off (use
allowedBrowsePaths/allowedUploadPaths plus a "datasourceReady" flag) so users
can't click inert items; update the handler referenced by
handleDataSourceOptionClick and the MenuItem onClick usages accordingly to
restore expected behavior.
60 tasks
…o feature/7418-video-picker # Conflicts: # ui/app/src/components/FormsEngine/lib/controlMap.ts
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
craftercms/craftercms#7418
Summary by CodeRabbit