feat(video-player): migrate from react player to media-chrome#799
feat(video-player): migrate from react player to media-chrome#799rogerantony-dev merged 24 commits intodevfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces the ReactPlayer-based implementation with a media-chrome-based VideoPlayer (native Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as "MediaController / ControlBar"
participant Provider as "MediaProvider"
participant Player as "youtube-video / video"
participant Handler as "ErrorHandler"
User->>UI: Play / Pause / Seek / Volume / PiP / Fullscreen
UI->>Provider: dispatch control action
Provider->>Player: apply command (play/pause/seek/volume)
Player->>Provider: emit media state events
Provider-->>UI: update control surface
Player->>Handler: native error event
Handler->>Handler: invoke onError callback
Handler-->>UI: surface error state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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🧪 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
src/components/video-player-icons.tsx (1)
65-81: Define a proper interface forSettingsIconprops.Per coding guidelines, component props should use a named TypeScript interface. The current inline optional parameter
(props?: { slotIcon?: boolean })works but is less discoverable.♻️ Suggested refactor
+interface SettingsIconProps { + slotIcon?: boolean; +} + -export function SettingsIcon(props?: { slotIcon?: boolean }) { +export function SettingsIcon(props?: SettingsIconProps) {As per coding guidelines, "Always define TypeScript interfaces for React component props."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-player-icons.tsx` around lines 65 - 81, The SettingsIcon component currently uses an inline type for props; create a named TypeScript interface (e.g., SettingsIconProps) that declares slotIcon?: boolean and update the SettingsIcon signature to accept that interface (use props: SettingsIconProps or props?: SettingsIconProps as appropriate), leaving the existing usage of props?.slotIcon and the Icon child unchanged so slot attribute handling stays the same.src/components/video-player-theme.ts (1)
3-14: Potential token duplication between JS styles and CSS theme file.
CONTROLLER_STYLEsets--base: 20pxand color tokens here, whilevideo-player-theme.cssindependently declares--video-base: 20pxand similar color values onmedia-controller. If one is updated without the other, they'll drift. Consider whether the CSS file alone can handle theming (via thestyleattribute on the element) or consolidate tokens in one place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-player-theme.ts` around lines 3 - 14, CONTROLLER_STYLE duplicates CSS tokens declared in video-player-theme.css (e.g., "--base" vs "--video-base" on media-controller), so consolidate to a single source of truth: either remove the duplicate tokens from CONTROLLER_STYLE and apply the CSS theme only (use the element's style attribute to override CSS variables when dynamic values are needed), or keep CONTROLLER_STYLE as the canonical token source and update video-player-theme.css to reference the same variable names (e.g., align "--base" and color variables) so both use identical variable names; update references in code that use CONTROLLER_STYLE and the CSS selector media-controller to the chosen variable names and remove the redundant definitions to prevent drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Line 121: Update the media-chrome dependency entry in package.json from
"media-chrome": "4.17.2" to "media-chrome": "4.18.0"; after changing the version
run your package manager to regenerate the lockfile (npm install or yarn
install) and run the test/build to verify there are no regressions, and commit
the updated package.json and lockfile changes.
In `@src/components/video-player-theme.css`:
- Line 227: Add the media-chrome attribute names that CSpell flags as unknown to
the project dictionary: add "breakpointmd" and "mediaisfullscreen" (and any
other flagged attribute tokens such as those shown in the review) to
.cspell/project-words.txt or run the repo helper command (pnpm fix:spelling) so
the CSS selectors like media-controller[breakpointmd] and
media-controller[mediaisfullscreen] are accepted; update the dictionary with
exact token strings to avoid future warnings.
- Around line 311-325: The CSS block for selector media-controller
.settings-menu-wrap media-playback-rate-menu has Stylelint
declaration-empty-line-before errors due to extra blank lines before
declarations and uses multiple !important overrides; remove the stray empty
lines so no blank line precedes declarations (e.g., before position, right,
left, transform, padding-block, border-radius, backdrop-filter) and either avoid
or minimize !important on position/right/left/transform by using a more specific
selector or reordering rules; if !important is truly required to override
media-chrome internals, add a short comment above the four !important
declarations explaining why they’re necessary (e.g., to override media-chrome
inline/author styles) so reviewers understand the intent.
- Around line 182-193: Remove the blank lines inside the CSS rule block that
contains the declarations starting with transform-origin and transform (the
block ending with the closing brace shown), so there are no empty lines before
subsequent declarations like width, height, backdrop-filter, transition, and the
`@apply` line; collapse those declarations into consecutive lines to satisfy
Stylelint's declaration-empty-line-before rule and allow pnpm fix:css to pass.
In `@src/components/VideoPlayer.tsx`:
- Line 91: Add the missing media-chrome attribute token "notooltip" to the
CSpell dictionary so the spellchecker stops flagging it; update
.cspell/project-words.txt by adding the word notooltip (this relates to the
MediaMuteButton usage in src/components/VideoPlayer.tsx where ref sets the
"notooltip" attribute).
- Around line 35-40: VideoPlayerProps includes isActive but the components never
use it, so media keeps playing when a slide becomes inactive; update VideoPlayer
and/or VideoPlayerInner to watch isActive (via useEffect) and control playback:
get the video element ref used in the component (the same ref passed to the
media element), and when isActive becomes true call el.play() (handle the
Promise) and when false call el.pause(); for embedded YouTube (isYouTube)
integrate with the player API or pause via postMessage/iframe API, and call
onError if play() rejects; ensure you guard null refs and do not block UI while
awaiting play().
- Around line 76-77: The youtube-video branch in VideoPlayer.tsx is missing
caption support; inspect whether the youtube-video-element supports <track>
children and, if it does, add a <track> element (kind="captions"/"subtitles",
srclang, src, default as appropriate) as a child of the <youtube-video ref={ref}
slot="media" src={src}> element to mirror the native <video> branch; if the
package does not support <track>, wire up captions via the YouTube player
API/params (enablecc=1 & track selection) or render an accessible fallback (a
visually hidden transcript/CC toggle and proper aria-label/aria-describedby) so
that VideoPlayer still provides captioning for audio/video content.
---
Nitpick comments:
In `@src/components/video-player-icons.tsx`:
- Around line 65-81: The SettingsIcon component currently uses an inline type
for props; create a named TypeScript interface (e.g., SettingsIconProps) that
declares slotIcon?: boolean and update the SettingsIcon signature to accept that
interface (use props: SettingsIconProps or props?: SettingsIconProps as
appropriate), leaving the existing usage of props?.slotIcon and the Icon child
unchanged so slot attribute handling stays the same.
In `@src/components/video-player-theme.ts`:
- Around line 3-14: CONTROLLER_STYLE duplicates CSS tokens declared in
video-player-theme.css (e.g., "--base" vs "--video-base" on media-controller),
so consolidate to a single source of truth: either remove the duplicate tokens
from CONTROLLER_STYLE and apply the CSS theme only (use the element's style
attribute to override CSS variables when dynamic values are needed), or keep
CONTROLLER_STYLE as the canonical token source and update video-player-theme.css
to reference the same variable names (e.g., align "--base" and color variables)
so both use identical variable names; update references in code that use
CONTROLLER_STYLE and the CSS selector media-controller to the chosen variable
names and remove the redundant definitions to prevent drift.
package.json
Outdated
| "jwt-decode": "4.0.0", | ||
| "lodash": "4.17.23", | ||
| "match-sorter": "8.2.0", | ||
| "media-chrome": "4.17.2", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check latest versions on npm
echo "=== media-chrome latest version ==="
curl -s https://registry.npmjs.org/media-chrome/latest | jq '.version'
echo ""
echo "=== youtube-video-element latest version ==="
curl -s https://registry.npmjs.org/youtube-video-element/latest | jq '.version'
echo ""
echo "=== media-chrome package info ==="
curl -s https://registry.npmjs.org/media-chrome/4.17.2 | jq '{version: .version, published: .time["4.17.2"]}'
echo ""
echo "=== youtube-video-element package info ==="
curl -s https://registry.npmjs.org/youtube-video-element/1.9.0 | jq '{version: .version, published: .time["1.9.0"]}'Repository: timelessco/recollect
Length of output: 334
🏁 Script executed:
#!/bin/bash
# Check for vulnerabilities using npm audit for the specific versions
echo "=== Checking media-chrome 4.17.2 ==="
npm view media-chrome@4.17.2 --json | jq '{version, homepage, license, _lastPublishedTime}'
echo ""
echo "=== Checking youtube-video-element 1.9.0 ==="
npm view youtube-video-element@1.9.0 --json | jq '{version, homepage, license, _lastPublishedTime}'
echo ""
echo "=== media-chrome changelog (4.17.2 to 4.18.0) ==="
npm view media-chrome versions --json | jq '.[-3:]'Repository: timelessco/recollect
Length of output: 1220
🌐 Web query:
media-chrome 4.18.0 release notes changelog
💡 Result:
media-chrome 4.18.0 — changelog (released 2026-02-19)
Features
- [a11y + i18n] Translate time phrases (PR #1232) [1]
Bug fixes
- i18n
aria-labelfixes for time range and seek buttons (PR #1253) [1] - Firefox accessibility test fix (PR #1258) [1]
- Patch memory leaks (PR #1261) [1]
- Thumbnail preview sizing fix (PR #1262), closes #1160 [1]
Upstream compare link: v4.17.2...v4.18.0 [1]
Update media-chrome to 4.18.0. The current version 4.17.2 is outdated. The latest stable release includes memory leak patches, accessibility improvements, and bug fixes. youtube-video-element@1.9.0 is current.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` at line 121, Update the media-chrome dependency entry in
package.json from "media-chrome": "4.17.2" to "media-chrome": "4.18.0"; after
changing the version run your package manager to regenerate the lockfile (npm
install or yarn install) and run the test/build to verify there are no
regressions, and commit the updated package.json and lockfile changes.
src/components/VideoPlayer.tsx
Outdated
| export interface VideoPlayerProps { | ||
| isActive: boolean; | ||
| src: string; | ||
| isYouTube?: boolean; | ||
| onError?: () => void; | ||
| }) => { | ||
| const playerRef = useRef<HTMLVideoElement | null>(null); | ||
| src: string; | ||
| } |
There was a problem hiding this comment.
isActive prop is accepted but never used — video won't pause when navigating away.
VideoPlayerProps declares isActive and callers pass it (e.g., LightboxRenderers.tsx passes isActive={isActive ?? false}), but it's never read in VideoPlayerInner or VideoPlayer. With the previous react-player implementation, isActive likely controlled playing state. Without it, video playback continues when the user navigates to a different lightbox slide.
You need to wire isActive to pause/play the media element — for example, via useEffect calling el.play()/el.pause() on the video ref, or using media-chrome's store API.
Also applies to: 138-144
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/VideoPlayer.tsx` around lines 35 - 40, VideoPlayerProps
includes isActive but the components never use it, so media keeps playing when a
slide becomes inactive; update VideoPlayer and/or VideoPlayerInner to watch
isActive (via useEffect) and control playback: get the video element ref used in
the component (the same ref passed to the media element), and when isActive
becomes true call el.play() (handle the Promise) and when false call el.pause();
for embedded YouTube (isYouTube) integrate with the player API or pause via
postMessage/iframe API, and call onError if play() rejects; ensure you guard
null refs and do not block UI while awaiting play().
src/components/VideoPlayer.tsx
Outdated
| {isYouTube ? ( | ||
| <youtube-video ref={ref} slot="media" src={src} /> |
There was a problem hiding this comment.
Missing <track> element on <youtube-video> for accessibility.
The native <video> branch (line 79–81) includes a <track> for captions, but the <youtube-video> branch does not. The coding guidelines require caption tracks for all audio and video elements.
Note that youtube-video-element may not support <track> children the same way native video does — verify whether captions can be provided via YouTube's built-in CC or if a <track> child is supported.
As per coding guidelines, "Always include caption tracks for audio and video elements."
Does youtube-video-element npm package support track element for captions?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/VideoPlayer.tsx` around lines 76 - 77, The youtube-video
branch in VideoPlayer.tsx is missing caption support; inspect whether the
youtube-video-element supports <track> children and, if it does, add a <track>
element (kind="captions"/"subtitles", srclang, src, default as appropriate) as a
child of the <youtube-video ref={ref} slot="media" src={src}> element to mirror
the native <video> branch; if the package does not support <track>, wire up
captions via the YouTube player API/params (enablecc=1 & track selection) or
render an accessible fallback (a visually hidden transcript/CC toggle and proper
aria-label/aria-describedby) so that VideoPlayer still provides captioning for
audio/video content.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
src/components/video-player-theme.css (3)
315-329:⚠️ Potential issue | 🟡 MinorStylelint
declaration-empty-line-beforeerrors on lines 325 and 328 — remove stray blank lines.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-player-theme.css` around lines 315 - 329, The CSS block for selector "media-controller .settings-menu-wrap media-playback-rate-menu" contains stray blank lines that trigger stylelint's declaration-empty-line-before rule; remove the empty lines between declarations (e.g., between "--media-control-padding: 0;" and the comment, and between "transform: translateX(-50%) !important;" and "padding-block: 0;") so all declarations are directly adjacent per stylelint expectations, then reformat to preserve spacing only where allowed by the rule.
186-197:⚠️ Potential issue | 🟡 MinorStylelint
declaration-empty-line-beforeerrors on lines 189, 192, 194 — remove stray blank lines.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-player-theme.css` around lines 186 - 197, Remove the stray empty lines inside the CSS declaration block containing transform-origin / transform / width / height / backdrop-filter / transition / `@apply` (the rule that sets transform-origin: center and transform: translateY(...)). Ensure there are no blank lines immediately before the declarations on lines with transform, width, and backdrop-filter so the declaration-empty-line-before lint rule is satisfied; collapse those blank lines so all property declarations are contiguous within that selector block.
231-231:⚠️ Potential issue | 🟡 MinorCSpell pipeline failures for
breakpointmdandmediaisfullscreen— add to project dictionary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-player-theme.css` at line 231, The CSpell pipeline is flagging custom attribute tokens like "breakpointmd" and "mediaisfullscreen" used in selectors such as media-controller[breakpointmd] and the mediaisfullscreen attribute; update the project's cspell dictionary (e.g., add these tokens to the words array in your cspell.json or dictionary file) to whitelist breakpointmd and mediaisfullscreen so the CSS selectors (media-controller[breakpointmd], media-time-display:not([showduration]), and any media-controller[mediaisfullscreen]) no longer trigger linting failures.src/components/VideoPlayer.tsx (3)
35-40:⚠️ Potential issue | 🔴 Critical
isActiveis declared inVideoPlayerPropsbut never consumed — video won't pause on slide change.
VideoPlayerInnerexplicitly dropsisActiveviaOmit<VideoPlayerProps, "isActive">, so playback is never paused when the containing lightbox slide becomes inactive.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/VideoPlayer.tsx` around lines 35 - 40, The prop isActive on VideoPlayerProps is declared but dropped by the Omit in VideoPlayerInner, so the component never pauses playback on slide change; restore and use isActive by including it in VideoPlayerInner's prop type (remove Omit<..., "isActive"> or add isActive to the inner props) and implement a useEffect inside VideoPlayerInner (or VideoPlayer) that watches isActive and calls the video element's pause() (via ref) when isActive becomes false and resumes or does nothing when true; reference VideoPlayerProps, VideoPlayerInner, isActive and the video element ref to locate where to add this logic.
91-91:⚠️ Potential issue | 🟡 MinorCSpell pipeline failure: add
notooltipto the project dictionary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/VideoPlayer.tsx` at line 91, The spellchecker flags the attribute value "notooltip" used on MediaMuteButton; add "notooltip" to the project's cspell dictionary (the words array in your cspell configuration) so the attribute is recognized, then commit the updated cspell config; reference the MediaMuteButton usage to locate the offending token.
76-82:⚠️ Potential issue | 🟡 MinorMissing
<track>on<youtube-video>— caption accessibility requirement not met for YouTube path.The native
<video>branch includes a<track>element, but the<youtube-video>branch does not. As per coding guidelines, caption tracks are required for all audio and video elements.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/VideoPlayer.tsx` around lines 76 - 82, The YouTube branch is missing a captions track: update the JSX in VideoPlayer (the isYouTube branch rendering <youtube-video ... />) to include a <track> element matching the native <video> branch (e.g., default kind="captions" label and srcLang attributes) so captions exist for both paths; keep existing props like ref, slot="media", src and crossOrigin on the <youtube-video> and add the <track> as a child to satisfy the caption accessibility requirement.
🧹 Nitpick comments (2)
src/components/video-player-theme.css (1)
203-213:.vol-wrap:focusis a dead selector —<div class="vol-wrap">is not focusable.Line 208 lists
media-controller .vol-wrap:focusas a condition to show the volume popup. A plain<div>withouttabIndexcannot receive focus directly, so:focusnever matches. Only:hoverand:focus-within(line 209) are ever active. The:focus-withinselector already covers keyboard navigation into theMediaVolumeRangeinside. Remove line 208 to avoid dead code.♻️ Proposed fix
media-controller .vol-wrap:hover, -media-controller .vol-wrap:focus, media-controller .vol-wrap:focus-within,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-player-theme.css` around lines 203 - 213, The CSS includes a dead selector "media-controller .vol-wrap:focus" because .vol-wrap is a non-focusable div; remove that selector from the rule set so only effective selectors remain (e.g., keep :hover and :focus-within). Update the rule block that currently lists "media-controller .vol-wrap:hover, media-controller .vol-wrap:focus, media-controller .vol-wrap:focus-within, ..." to drop the ".vol-wrap:focus" entry, ensuring the volume popup visibility is controlled by :hover and :focus-within (which covers keyboard focus into the MediaVolumeRange).src/components/video-player-theme.ts (1)
16-23:alignItemsandgapinCONTROL_BAR_STYLEduplicate the CSS@apply items-center gap-3.
video-player-theme.cssline 21 appliesitems-center gap-3(align-items: center; gap: 12px) tomedia-control-bar. Since the inlinestyleprop has higher specificity, the CSS@applyvalues are always overridden and become dead code for those two properties. Consider removing either thealignItems/gapfrom this object (keeping the CSS@apply) or removing the@apply items-center gap-3from the CSS (keeping the inline values).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-player-theme.ts` around lines 16 - 23, The CONTROL_BAR_STYLE object currently duplicates the CSS `@apply` items-center gap-3 (alignItems and gap) causing the CSS rules to be overridden; remove the alignItems and gap properties from the exported CONTROL_BAR_STYLE constant so the media-control-bar relies on the CSS `@apply` values instead, leaving paddingBottom/paddingInline/paddingTop intact (ensure the object still casts to CSSProperties and run checks to confirm layout remains correct).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/VideoPlayer.tsx`:
- Around line 53-57: The YouTube custom element is being rendered before its
definition loads; change the useEffect that imports "youtube-video-element"
(currently watching isYouTube) to set a local state flag (e.g., isYouTubeReady)
only after the import promise resolves, and then render the <youtube-video>
element conditionally based on that flag in the VideoPlayer component; this
ensures the element is only added to the DOM after the module registers the
custom element so MediaController can find a proper media element (keep the
import inside the existing useEffect, set state on success, handle failure by
logging or falling back to a placeholder).
---
Duplicate comments:
In `@src/components/video-player-theme.css`:
- Around line 315-329: The CSS block for selector "media-controller
.settings-menu-wrap media-playback-rate-menu" contains stray blank lines that
trigger stylelint's declaration-empty-line-before rule; remove the empty lines
between declarations (e.g., between "--media-control-padding: 0;" and the
comment, and between "transform: translateX(-50%) !important;" and
"padding-block: 0;") so all declarations are directly adjacent per stylelint
expectations, then reformat to preserve spacing only where allowed by the rule.
- Around line 186-197: Remove the stray empty lines inside the CSS declaration
block containing transform-origin / transform / width / height / backdrop-filter
/ transition / `@apply` (the rule that sets transform-origin: center and
transform: translateY(...)). Ensure there are no blank lines immediately before
the declarations on lines with transform, width, and backdrop-filter so the
declaration-empty-line-before lint rule is satisfied; collapse those blank lines
so all property declarations are contiguous within that selector block.
- Line 231: The CSpell pipeline is flagging custom attribute tokens like
"breakpointmd" and "mediaisfullscreen" used in selectors such as
media-controller[breakpointmd] and the mediaisfullscreen attribute; update the
project's cspell dictionary (e.g., add these tokens to the words array in your
cspell.json or dictionary file) to whitelist breakpointmd and mediaisfullscreen
so the CSS selectors (media-controller[breakpointmd],
media-time-display:not([showduration]), and any
media-controller[mediaisfullscreen]) no longer trigger linting failures.
In `@src/components/VideoPlayer.tsx`:
- Around line 35-40: The prop isActive on VideoPlayerProps is declared but
dropped by the Omit in VideoPlayerInner, so the component never pauses playback
on slide change; restore and use isActive by including it in VideoPlayerInner's
prop type (remove Omit<..., "isActive"> or add isActive to the inner props) and
implement a useEffect inside VideoPlayerInner (or VideoPlayer) that watches
isActive and calls the video element's pause() (via ref) when isActive becomes
false and resumes or does nothing when true; reference VideoPlayerProps,
VideoPlayerInner, isActive and the video element ref to locate where to add this
logic.
- Line 91: The spellchecker flags the attribute value "notooltip" used on
MediaMuteButton; add "notooltip" to the project's cspell dictionary (the words
array in your cspell configuration) so the attribute is recognized, then commit
the updated cspell config; reference the MediaMuteButton usage to locate the
offending token.
- Around line 76-82: The YouTube branch is missing a captions track: update the
JSX in VideoPlayer (the isYouTube branch rendering <youtube-video ... />) to
include a <track> element matching the native <video> branch (e.g., default
kind="captions" label and srcLang attributes) so captions exist for both paths;
keep existing props like ref, slot="media", src and crossOrigin on the
<youtube-video> and add the <track> as a child to satisfy the caption
accessibility requirement.
---
Nitpick comments:
In `@src/components/video-player-theme.css`:
- Around line 203-213: The CSS includes a dead selector "media-controller
.vol-wrap:focus" because .vol-wrap is a non-focusable div; remove that selector
from the rule set so only effective selectors remain (e.g., keep :hover and
:focus-within). Update the rule block that currently lists "media-controller
.vol-wrap:hover, media-controller .vol-wrap:focus, media-controller
.vol-wrap:focus-within, ..." to drop the ".vol-wrap:focus" entry, ensuring the
volume popup visibility is controlled by :hover and :focus-within (which covers
keyboard focus into the MediaVolumeRange).
In `@src/components/video-player-theme.ts`:
- Around line 16-23: The CONTROL_BAR_STYLE object currently duplicates the CSS
`@apply` items-center gap-3 (alignItems and gap) causing the CSS rules to be
overridden; remove the alignItems and gap properties from the exported
CONTROL_BAR_STYLE constant so the media-control-bar relies on the CSS `@apply`
values instead, leaving paddingBottom/paddingInline/paddingTop intact (ensure
the object still casts to CSSProperties and run checks to confirm layout remains
correct).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/VideoPlayer.tsxsrc/components/video-player-theme.csssrc/components/video-player-theme.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/components/VideoPlayer.tsx (2)
57-61:⚠️ Potential issue | 🟠 Major
<youtube-video>renders beforeyoutube-video-elementregisters its custom element definition.
useEffectfires post-commit, so the<youtube-video>element is already in the DOM as a plainHTMLElementwhile the dynamic import is in-flight.MediaControllerwon't find a valid media element until the upgrade completes, leaving controls non-functional during that window.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/VideoPlayer.tsx` around lines 57 - 61, The <youtube-video> element may be inserted before the dynamic import completes, so change the logic in VideoPlayer to await registration before rendering: in the useEffect that currently imports "youtube-video-element" (referencing useEffect and isYouTube), set a local state (e.g., youtubeRegistered) to true only after the import resolves, and render the <youtube-video> tag conditionally based on that state so MediaController sees an upgraded/custom element; alternatively, perform the dynamic import earlier (e.g., on mount) and block rendering until the module is loaded to ensure the custom element is registered before insertion.
39-44:⚠️ Potential issue | 🔴 Critical
isActiveprop remains unimplemented — video does not pause on lightbox navigation away.
isActiveis accepted inVideoPlayerPropsand spread intoVideoPlayerInner, butVideoPlayerInneromits it viaOmit<VideoPlayerProps, "isActive">and never reads it. The media element will keep playing when the user navigates to another lightbox slide.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/VideoPlayer.tsx` around lines 39 - 44, The isActive prop declared in VideoPlayerProps is never used because VideoPlayerInner is typed with Omit<VideoPlayerProps, "isActive"> and doesn't read it, causing media to continue playing when slides change; fix by accepting isActive in VideoPlayerInner (remove the Omit) or add an explicit isActive parameter to VideoPlayerInner, then use a ref to the HTMLMediaElement inside VideoPlayerInner (the same component that renders the <video> or <iframe>) and add an effect that when isActive becomes false pauses the media (call pause() and optionally set currentTime or mute) and when true resumes if desired, and ensure VideoPlayer forwards the isActive prop down to VideoPlayerInner so navigation away actually stops playback.
🧹 Nitpick comments (1)
src/components/video-player-theme.ts (1)
3-29: Consider migrating static inline-style objects tovideo-player-theme.cssto align with the "no CSS-in-JS" guideline.Every property in
BASE_CONTROLLER_STYLE/CONTROL_BAR_STYLEis static and can live in the CSS file. Two concrete problems with the current approach:
- Redundancy:
CONTROL_BAR_STYLEsetsalignItems: "center"andgap: "12px"(i.e.,gap-3), which are the same values already applied in the CSS via@apply items-center gap-3. Inline styles win on specificity, so the@applyline is effectively dead code.- Type unsafe cast: CSS custom properties (
--base,--_accent-color,--media-*) are not part ofCSSProperties, so theas CSSPropertiesassertion silently discards type checking. A proper extended type avoids this:type MediaControllerCSSProperties = React.CSSProperties & { [key: `--${string}`]: string | number; };For the YouTube conditional (
aspectRatio,width), a CSS class applied viaclassNameis idiomatic:♻️ Proposed refactor direction
In
video-player-theme.css:media-controller { --video-base: 20px; + --media-background-color: `#000`; + --media-primary-color: `#fff`; + --media-control-background: transparent; + --media-control-hover-background: rgb(255 255 255 / 10%); + border-radius: 16px; + max-height: 80vh; + max-width: 1200px; } +media-controller.youtube-player { + aspect-ratio: 16/9; + width: 100%; +} media-controller media-control-bar { - `@apply` items-center gap-3; /* removed — now explicit below */ + align-items: center; + gap: 12px; + background: linear-gradient(transparent, rgb(0 0 0 / 60%)); + padding-top: 60px; + padding-inline: 12px; + padding-bottom: 8px; touch-action: none; }In
VideoPlayer.tsx:<MediaController - style={isYouTube ? YOUTUBE_CONTROLLER_STYLE : CONTROLLER_STYLE} + className={isYouTube ? "youtube-player" : undefined} onPointerDownCapture={(event) => event.stopPropagation()} > ... - <MediaControlBar style={CONTROL_BAR_STYLE}> + <MediaControlBar>
video-player-theme.tscan then be removed or reduced to nothing.Based on learnings: "Applies to
**/*.{tsx,ts}: Avoid CSS-in-JS runtime styling solutions."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-player-theme.ts` around lines 3 - 29, Move all static inline style properties from BASE_CONTROLLER_STYLE, CONTROL_BAR_STYLE, and YOUTUBE_CONTROLLER_STYLE into video-player-theme.css as CSS classes (e.g., .media-controller, .control-bar, .youtube-controller) and replace usages to apply those classes via className; remove redundant properties that duplicate `@apply` rules (alignItems/gap) so CSS specificity isn't defeated. Replace the unsafe as CSSProperties cast by introducing a MediaControllerCSSProperties type (React.CSSProperties & { [key: `--${string}`]: string | number }) if you must keep any custom properties in TS, or remove them from the TS file entirely after migrating to CSS variables in video-player-theme.css. Ensure aspectRatio/width for YOUTUBE_CONTROLLER_STYLE become a .youtube-controller class and then delete or reduce video-player-theme.ts accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/VideoPlayer.tsx`:
- Around line 63-76: The callback ref returned from useCallback (the ref
assigned to the video element) currently returns a cleanup that only removes the
error listener, which in React 19 prevents mediaRef(null) from being called on
unmount and blocks MediaStore unregister; update the ref callback (the function
stored in ref) so its cleanup both removes the "error" listener (handleError)
and explicitly calls mediaRef(null) when cleaning up, and ensure the initial
branch that handles el being null still returns undefined; keep using
onErrorRef.current for the handler reference and preserve the existing listener
add/remove logic.
---
Duplicate comments:
In `@src/components/VideoPlayer.tsx`:
- Around line 57-61: The <youtube-video> element may be inserted before the
dynamic import completes, so change the logic in VideoPlayer to await
registration before rendering: in the useEffect that currently imports
"youtube-video-element" (referencing useEffect and isYouTube), set a local state
(e.g., youtubeRegistered) to true only after the import resolves, and render the
<youtube-video> tag conditionally based on that state so MediaController sees an
upgraded/custom element; alternatively, perform the dynamic import earlier
(e.g., on mount) and block rendering until the module is loaded to ensure the
custom element is registered before insertion.
- Around line 39-44: The isActive prop declared in VideoPlayerProps is never
used because VideoPlayerInner is typed with Omit<VideoPlayerProps, "isActive">
and doesn't read it, causing media to continue playing when slides change; fix
by accepting isActive in VideoPlayerInner (remove the Omit) or add an explicit
isActive parameter to VideoPlayerInner, then use a ref to the HTMLMediaElement
inside VideoPlayerInner (the same component that renders the <video> or
<iframe>) and add an effect that when isActive becomes false pauses the media
(call pause() and optionally set currentTime or mute) and when true resumes if
desired, and ensure VideoPlayer forwards the isActive prop down to
VideoPlayerInner so navigation away actually stops playback.
---
Nitpick comments:
In `@src/components/video-player-theme.ts`:
- Around line 3-29: Move all static inline style properties from
BASE_CONTROLLER_STYLE, CONTROL_BAR_STYLE, and YOUTUBE_CONTROLLER_STYLE into
video-player-theme.css as CSS classes (e.g., .media-controller, .control-bar,
.youtube-controller) and replace usages to apply those classes via className;
remove redundant properties that duplicate `@apply` rules (alignItems/gap) so CSS
specificity isn't defeated. Replace the unsafe as CSSProperties cast by
introducing a MediaControllerCSSProperties type (React.CSSProperties & { [key:
`--${string}`]: string | number }) if you must keep any custom properties in TS,
or remove them from the TS file entirely after migrating to CSS variables in
video-player-theme.css. Ensure aspectRatio/width for YOUTUBE_CONTROLLER_STYLE
become a .youtube-controller class and then delete or reduce
video-player-theme.ts accordingly.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/VideoPlayer.tsxsrc/components/video-player-theme.csssrc/components/video-player-theme.ts
src/components/VideoPlayer.tsx
Outdated
| const ref = useCallback( | ||
| (el: HTMLVideoElement | null) => { | ||
| mediaRef(el); | ||
|
|
||
| if (!el) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const handleError = () => onErrorRef.current?.(); | ||
| el.addEventListener("error", handleError); | ||
| return () => el.removeEventListener("error", handleError); | ||
| }, | ||
| [mediaRef], | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
React 19 ref callback cleanup function returns prevents null call unmount
💡 Result:
In React 19, a callback ref may return a cleanup function:
<div
ref={(node) => {
if (!node) return; // (optional) ignore detach
// setup using node
return () => {
// cleanup using the same node
};
}}
/>What changes vs older React:
- If your ref callback returns a cleanup function, React will call that cleanup when the ref is detached (including unmount), and it will skip calling your ref callback with
nullon unmount. This is the behavior you’re referring to (“return prevents the null call”). [1][2] - For backwards compatibility, if you don’t return a cleanup function, React still uses the old behavior and calls the ref callback with
nullwhen detaching—but React says thisnull-on-detach behavior will be removed in a future version. [2] - React also detaches/attaches when the ref callback identity changes (e.g., inline
(node) => {}on every render). If you want to avoid unnecessary detach/attach cycles, keep the ref callback stable (e.g.,useCallback). [2]
Sources: React 19 announcement + the ref callback docs on react.dev. [1][2]
🌐 Web query:
media-chrome useMediaRef documentation React cleanup null
💡 Result:
useMediaRef() is a React hook exported from media-chrome/react/media-store and is the intended way to “wire up” your <video>/<audio> (or compatible custom media element) to Media Chrome’s MediaStore (so useMediaSelector()/useMediaDispatch() work). You use it by passing it directly to the element’s ref prop. [1]
If you’re seeing cleanup errors like “cannot read … removeEventListener of null”, the common cause is that ref.current is mutable and may be set to null before your effect cleanup runs. The fix is to capture the element in a local variable inside the effect and use that for both add/remove. [2]
const mediaRef = useMediaRef();
useEffect(() => {
const el = mediaRef.current; // capture once
if (!el) return;
const onTimeUpdate = () => {};
el.addEventListener("timeupdate", onTimeUpdate);
return () => {
el.removeEventListener("timeupdate", onTimeUpdate);
};
}, []);Sources: Media Chrome “MediaStore React Hooks” docs (includes useMediaRef() usage) [1]; explanation of ref.current becoming null before cleanup + “capture in effect” pattern [2].
🏁 Script executed:
fd VideoPlayer.tsxRepository: timelessco/recollect
Length of output: 94
🏁 Script executed:
# Once we have the file path, examine it
if [ -f "src/components/VideoPlayer.tsx" ]; then
cat -n src/components/VideoPlayer.tsx | head -100
fiRepository: timelessco/recollect
Length of output: 3003
🏁 Script executed:
# Search for media-chrome imports and useMediaRef usage
rg "useMediaRef|media-chrome" -B2 -A2 --type ts --type tsxRepository: timelessco/recollect
Length of output: 91
🏁 Script executed:
rg "useMediaRef|from.*media-chrome" -t ts -t jsRepository: timelessco/recollect
Length of output: 360
🌐 Web query:
media-chrome MediaStore useMediaRef cleanup null unregister unmount
💡 Result:
In media-chrome/react/media-store, the intended “cleanup/unregister on unmount” behavior is usually driven by the ref callback receiving null (React calls a callback ref with null when the element unmounts or the ref moves).
So the key is: use the callback ref returned by useMediaRef() directly on your media element, and don’t “hide” it behind another ref pattern that prevents the null call from reaching MediaStore.
import { MediaProvider, useMediaRef } from "media-chrome/react/media-store";
function Player() {
const mediaRef = useMediaRef(); // callback ref
return (
<MediaProvider>
<video ref={mediaRef} />
</MediaProvider>
);
}Common gotchas that can break cleanup:
- Don’t convert it to an object ref (
useRef) and only copy.current; you’ll miss thenullcallback. - Don’t wrap it in a callback that ignores
null:const mediaRef = useMediaRef(); const bad = (el) => { if (el) mediaRef(el); }; // blocks cleanup
- If you need composition, forward
nulltoo:const mediaRef = useMediaRef(); const composed = (el) => { mediaRef(el); /* your logic */ };
If you’re chasing leaks specifically: Media Chrome has been actively patching “disconnect/unmount” cleanup (removing listeners, disconnecting observers, etc.) in recent work (merged Feb 19, 2026). If you’re on an older media-chrome version, upgrading may matter. (github.com)
References:
media-chromeexports the React media-store entrypoint (./react/media-store). (app.unpkg.com)- Example usage of
useMediaReffrommedia-chrome/react/media-storein the wild. (diceui.com)
Citations:
- 1: fix: Patch Memory Leaks muxinc/media-chrome#1261
- 2: https://app.unpkg.com/media-chrome%404.17.0/files/package.json?utm_source=openai
- 3: https://www.diceui.com/docs/components/media-player?utm_source=openai
🌐 Web query:
"media-chrome" "useMediaRef" source code implementation state cleanup
💡 Result:
In media-chrome, the React hook useMediaRef() is used as a ref callback you compose onto your <video>/<audio> element so the Media Store can be told when the backing media element changes (example usage shows const mediaRefCallback = useMediaRef(); ... ref={useComposedRefs(..., mediaRefCallback)}), which in turn triggers the store’s “media element changed” flow. [1]
On the Media Store side, the key “state cleanup” logic for custom state wiring lives in createMediaStore() → updateStateOwners() inside src/js/media-store/media-store.ts. Each state in the stateMediator can define stateOwnersUpdateHandlers (setup hooks that may return teardown functions). When any “state owner” changes (media element, textTracks, root node, etc.), the store:
- decides whether it must teardown and/or setup monitoring (
teardownSomething/setupSomething), - runs teardowns, then runs setups and stores the returned teardown(s). [2]
A bug existed where only the last teardown returned by stateOwnersUpdateHandlers was being kept/called, leaking earlier listeners. That was fixed and merged Nov 17, 2025 (“Ensure all stateOwnersUpdateHandlers are properly torn down”). The fix:
- stores all teardown functions (if multiple handlers return them),
- on subsequent updates, normalizes previous teardown(s) into an array and calls every function,
- filters out non-functions,
- stores the new teardown(s) as a single function or an array depending on count. [3][2]
Relevant code (current behavior):
- teardown all previous teardowns (array-or-single) when
teardownSomethingis true, then - setup new handlers and persist teardowns (single-or-array). [2]
Sources: Dice UI docs showing useMediaRef() ref-callback usage pattern [1]; Media Store implementation showing stateOwnersUpdateHandlers teardown/setup logic [2]; PR #1237 describing/fixing the multi-teardown leak and merged date [3].
[1] (diceui.com)
[2] (raw.githubusercontent.com)
[3] (github.com)
Citations:
- 1: https://www.diceui.com/docs/components/media-player?utm_source=openai
- 2: https://raw.githubusercontent.com/muxinc/media-chrome/main/src/js/media-store/media-store.ts
- 3: fix: Ensure all stateOwnersUpdateHandlers are properly torn down muxinc/media-chrome#1237
React 19 ref cleanup skips mediaRef(null) — MediaStore unregister is blocked on unmount.
In React 19, returning a cleanup function from a ref callback causes React to call that cleanup on unmount instead of invoking the callback with null. The returned cleanup at line 73 only removes the error listener — it never calls mediaRef(null). This breaks the contract that useMediaRef() from media-chrome/react/media-store relies on: the callback ref must receive null to trigger proper MediaStore cleanup and unregister the element.
The intended pattern for useMediaRef() is to pass it directly as the ref or forward it in a composed ref. When wrapped in a callback that blocks the null call, the MediaStore retains a stale reference to the detached element.
🛠️ Proposed fix
const ref = useCallback(
(el: HTMLVideoElement | null) => {
mediaRef(el);
if (!el) {
return undefined;
}
const handleError = () => onErrorRef.current?.();
el.addEventListener("error", handleError);
- return () => el.removeEventListener("error", handleError);
+ return () => {
+ el.removeEventListener("error", handleError);
+ mediaRef(null);
+ };
},
[mediaRef],
);📝 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.
| const ref = useCallback( | |
| (el: HTMLVideoElement | null) => { | |
| mediaRef(el); | |
| if (!el) { | |
| return undefined; | |
| } | |
| const handleError = () => onErrorRef.current?.(); | |
| el.addEventListener("error", handleError); | |
| return () => el.removeEventListener("error", handleError); | |
| }, | |
| [mediaRef], | |
| ); | |
| const ref = useCallback( | |
| (el: HTMLVideoElement | null) => { | |
| mediaRef(el); | |
| if (!el) { | |
| return undefined; | |
| } | |
| const handleError = () => onErrorRef.current?.(); | |
| el.addEventListener("error", handleError); | |
| return () => { | |
| el.removeEventListener("error", handleError); | |
| mediaRef(null); | |
| }; | |
| }, | |
| [mediaRef], | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/VideoPlayer.tsx` around lines 63 - 76, The callback ref
returned from useCallback (the ref assigned to the video element) currently
returns a cleanup that only removes the error listener, which in React 19
prevents mediaRef(null) from being called on unmount and blocks MediaStore
unregister; update the ref callback (the function stored in ref) so its cleanup
both removes the "error" listener (handleError) and explicitly calls
mediaRef(null) when cleaning up, and ensure the initial branch that handles el
being null still returns undefined; keep using onErrorRef.current for the
handler reference and preserve the existing listener add/remove logic.
…settings menu position
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/components/VideoPlayer.tsx (2)
65-78:mediaRef(null)is still not called in the React 19 ref cleanup —MediaStoreunregister remains blocked on unmount.In React 19, returning a cleanup function from a ref callback causes React to invoke that cleanup on unmount instead of re-invoking the callback with
null. The cleanup at line 75 only removes the error listener;mediaRef(null)is never called, so theMediaStoreretains a stale reference to the detached element. The proposed fix from the previous review (explicitly callingmediaRef(null)inside the cleanup) has not been applied.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/VideoPlayer.tsx` around lines 65 - 78, The ref callback returned by useCallback (the function assigned to ref) currently only removes the "error" listener in its cleanup, so in React 19 mediaRef(null) is never called on unmount; update the cleanup returned from the ref callback to both remove the error listener and explicitly call mediaRef(null) (i.e., ensure the cleanup calls mediaRef(null) before/after el.removeEventListener), keeping the existing handleError/onErrorRef.current logic and the useCallback dependency on mediaRef so MediaStore unregisters the element on unmount.
85-93:<youtube-video>branch is still missing a<track>element for captions.The native
<video>path (line 90–92) includes a<track kind="captions">, but the YouTube path renders<youtube-video>without any caption track. This was flagged in the previous review and remains unaddressed. Based on learnings, "Always include caption tracks for audio and video elements."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/VideoPlayer.tsx` around lines 85 - 93, The YouTube branch renders <youtube-video> without a captions track; update the JSX in VideoPlayer.tsx so that when isYouTube and youTubeReady render <youtube-video ref={ref} slot="media" src={src}>, you also include a <track kind="captions" default label="No captions" srcLang="en" /> (or the same captions source used for the native <video>) as a child of <youtube-video> to match the native <video> path; adjust props or children placement on the youtube-video element to ensure the track is present when youTubeReady is true.src/components/video-player-theme.css (1)
188-200: Stylelintdeclaration-empty-line-beforeerrors at lines 192, 195, 197 remain.Same block and same errors flagged in the previous review — the blank lines between
transform,width,backdrop-filter, andtransitiondeclarations still violate thedeclaration-empty-line-beforerule and will preventpnpm fix:cssfrom passing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-player-theme.css` around lines 188 - 200, The Stylelint errors come from extra blank lines before several declarations in the CSS block (the transform, width, backdrop-filter, and transition declarations); remove the empty lines so each declaration follows the previous one without a blank line (e.g., ensure transform-origin/transform, width/height, backdrop-filter, and transition appear consecutively) to satisfy the declaration-empty-line-before rule and allow pnpm fix:css to pass.src/components/lightbox/LightboxRenderers.tsx (1)
77-98: Video playback still not paused on slide navigation —isActivewas removed rather than wired.The previous review flagged that
isActivewas accepted byVideoPlayerbut never used to pause/resume playback. This PR resolves the symptom (removes the dead prop) but not the root behaviour:VideoPlayerhas no mechanism to stop playback when the lightbox navigates to a different slide, so the video keeps playing in the background.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/lightbox/LightboxRenderers.tsx` around lines 77 - 98, VideoSlide removed the isActive prop instead of wiring slide-active state through to VideoPlayer, so videos keep playing when the lightbox navigates; restore or accept an active flag in VideoSlide and pass it to VideoPlayer (or derive active from the lightbox context/parent) and update VideoPlayer to pause/resume playback when that prop changes (useEffect watching isActive and call the video element's pause()/play() via an internal ref or expose a pause API). Update VideoSlide to pass the active boolean (e.g., isActive) into the VideoPlayer call and ensure VideoPlayer implements the pause/resume behavior on prop change so playback stops when the slide is no longer active.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/video-player-theme.css`:
- Line 5: Stylelint is flagging Tailwind's `@reference` at-rule and blocking pnpm
fix:css; update the scss/at-rule-no-unknown rule in the Stylelint config to
ignore Tailwind-specific at-rules by adding "reference" to the ignoreAtRules
array for scss/at-rule-no-unknown so `@reference` is whitelisted (keep the rule
active for other at-rules). Locate the scss/at-rule-no-unknown rule in your
Stylelint configuration and append "reference" to its ignoreAtRules option.
In `@src/components/VideoPlayer.tsx`:
- Around line 59-63: The dynamic import of "youtube-video-element" inside the
async IIFE can reject and leaves the player stuck; wrap the import in a
try/catch inside the effect that uses isYouTube, and on error log the exception
and set a failure state so the UI can show a fallback instead of waiting
forever. Concretely, update the block that calls import("youtube-video-element")
/ setYouTubeReady to catch rejects, call setYouTubeReady(false) (and/or a new
setYouTubeError state), log the error (console.error or your app logger), and
ensure MediaController/VideoPlayer renders a graceful fallback or error message
when that error state is set.
---
Duplicate comments:
In `@src/components/lightbox/LightboxRenderers.tsx`:
- Around line 77-98: VideoSlide removed the isActive prop instead of wiring
slide-active state through to VideoPlayer, so videos keep playing when the
lightbox navigates; restore or accept an active flag in VideoSlide and pass it
to VideoPlayer (or derive active from the lightbox context/parent) and update
VideoPlayer to pause/resume playback when that prop changes (useEffect watching
isActive and call the video element's pause()/play() via an internal ref or
expose a pause API). Update VideoSlide to pass the active boolean (e.g.,
isActive) into the VideoPlayer call and ensure VideoPlayer implements the
pause/resume behavior on prop change so playback stops when the slide is no
longer active.
In `@src/components/video-player-theme.css`:
- Around line 188-200: The Stylelint errors come from extra blank lines before
several declarations in the CSS block (the transform, width, backdrop-filter,
and transition declarations); remove the empty lines so each declaration follows
the previous one without a blank line (e.g., ensure transform-origin/transform,
width/height, backdrop-filter, and transition appear consecutively) to satisfy
the declaration-empty-line-before rule and allow pnpm fix:css to pass.
In `@src/components/VideoPlayer.tsx`:
- Around line 65-78: The ref callback returned by useCallback (the function
assigned to ref) currently only removes the "error" listener in its cleanup, so
in React 19 mediaRef(null) is never called on unmount; update the cleanup
returned from the ref callback to both remove the error listener and explicitly
call mediaRef(null) (i.e., ensure the cleanup calls mediaRef(null) before/after
el.removeEventListener), keeping the existing handleError/onErrorRef.current
logic and the useCallback dependency on mediaRef so MediaStore unregisters the
element on unmount.
- Around line 85-93: The YouTube branch renders <youtube-video> without a
captions track; update the JSX in VideoPlayer.tsx so that when isYouTube and
youTubeReady render <youtube-video ref={ref} slot="media" src={src}>, you also
include a <track kind="captions" default label="No captions" srcLang="en" /> (or
the same captions source used for the native <video>) as a child of
<youtube-video> to match the native <video> path; adjust props or children
placement on the youtube-video element to ensure the track is present when
youTubeReady is true.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
package.jsonsrc/components/VideoPlayer.tsxsrc/components/lightbox/LightBox.tsxsrc/components/lightbox/LightboxRenderers.tsxsrc/components/video-player-theme.css
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
| * Video Player Theme (media-chrome) | ||
| * Scoped to media-controller. Uses project color variables. | ||
| */ | ||
| @reference "../styles/globals.css"; |
There was a problem hiding this comment.
@reference blocks pnpm fix:css — add it to the Stylelint ignoreAtRules allowlist.
The scss/at-rule-no-unknown rule (part of the stylelint-scss plugin) flags any at-rule it doesn't recognise as valid SCSS or CSS. @reference is a Tailwind CSS v4 directive that makes utilities available for @apply without emitting output; Stylelint has no knowledge of it. The most direct fix is to explicitly tell Stylelint to ignore Tailwind's at-rules — this preserves the rule for everything else while whitelisting Tailwind-specific ones.
Add "reference" to the scss/at-rule-no-unknown ignoreAtRules option in the project's Stylelint config:
🛠️ Proposed fix (Stylelint config)
"scss/at-rule-no-unknown": [true, {
"ignoreAtRules": [
+ "reference",
"apply",
"tailwind",
"layer",
"theme",
// …any other existing entries
]
}]🧰 Tools
🪛 Stylelint (17.3.0)
[error] 5-5: Unexpected unknown at-rule "@reference" (scss/at-rule-no-unknown)
(scss/at-rule-no-unknown)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/video-player-theme.css` at line 5, Stylelint is flagging
Tailwind's `@reference` at-rule and blocking pnpm fix:css; update the
scss/at-rule-no-unknown rule in the Stylelint config to ignore Tailwind-specific
at-rules by adding "reference" to the ignoreAtRules array for
scss/at-rule-no-unknown so `@reference` is whitelisted (keep the rule active for
other at-rules). Locate the scss/at-rule-no-unknown rule in your Stylelint
configuration and append "reference" to its ignoreAtRules option.
src/components/VideoPlayer.tsx
Outdated
| void (async () => { | ||
| await import("youtube-video-element"); | ||
| setYouTubeReady(true); | ||
| })(); | ||
| }, [isYouTube]); |
There was a problem hiding this comment.
Unhandled failure of youtube-video-element dynamic import — player silently becomes non-functional.
If the dynamic import rejects (network error, etc.), the async IIFE throws inside void (async () => {...})(), the rejection is swallowed, setYouTubeReady(true) is never called, and the user sees an empty/loading MediaController indefinitely with no error feedback or fallback.
🛠️ Proposed fix
- void (async () => {
- await import("youtube-video-element");
- setYouTubeReady(true);
- })();
+ import("youtube-video-element")
+ .then(() => {
+ setYouTubeReady(true);
+ })
+ .catch(() => {
+ onErrorRef.current?.();
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/VideoPlayer.tsx` around lines 59 - 63, The dynamic import of
"youtube-video-element" inside the async IIFE can reject and leaves the player
stuck; wrap the import in a try/catch inside the effect that uses isYouTube, and
on error log the exception and set a failure state so the UI can show a fallback
instead of waiting forever. Concretely, update the block that calls
import("youtube-video-element") / setYouTubeReady to catch rejects, call
setYouTubeReady(false) (and/or a new setYouTubeError state), log the error
(console.error or your app logger), and ensure MediaController/VideoPlayer
renders a graceful fallback or error message when that error state is set.
…etter interaction handling
Screen.Recording.2026-02-23.at.3.58.35.PM.movVideo Player TestingSteps1. Test native video (file or direct link)
2. Test YouTube video
YouTube embeds do not support PiP; that control is hidden for YouTube. please compare the player against: https://www.interfacecraft.dev/library/using-dial-kit it should match exactly |
…o, YouTube, and Spotify
1 participant
PR Checklist
Overview
Summary by CodeRabbit
New Features
Improvements