fix: language switcher crash on minority languages in#9244
Open
tanflem wants to merge 3 commits into
Open
Conversation
… to prevent crash on minority languages When language switcher navigates to a language with no images or imageAlt data, direct array index access [0].prop threw TypeError. Add optional chaining so empty arrays render gracefully instead of crashing the Error Boundary. Fixes WAT-204
…leming/wat-204-fix-language-switcher-crash-on-minority-languages-in
Contributor
WalkthroughThree video control components (VideoHero, VideoBlock, and main VideoControls) now safely guard against disposed or unavailable Video.js players using new helper functions. Image alt props are updated to fall back to empty strings. Tests verify that disposed players do not cause render errors. ChangesPlayer Safety Guards and Image Fallbacks
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
View your CI Pipeline Execution ↗ for commit d9d2d77
☁️ Nx Cloud last updated this comment at |
Contributor
|
The latest updates on your projects.
|
Contributor
|
The latest updates on your projects.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
apps/resources/src/components/NewVideoContentPage/VideoCarousel/VideoCard/VideoCard.tsx (1)
57-69:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftNext.js
Imagefallback should not usesrc=""
Inapps/resources/src/components/NewVideoContentPage/VideoCarousel/VideoCard/VideoCard.tsxthenext/imagesrcfalls back to''(src={video.images[0]?.mobileCinematicHigh ?? ''}). Next.js 16 treats a falsy/emptysrcas missing and throws during rendering/optimization, so this can replace the original runtime issue with a Next.jssrcerror.✅ Suggested change (render Image only when src is available)
- <Image - fill - src={video.images[0]?.mobileCinematicHigh ?? ''} - alt={video.imageAlt[0]?.value ?? ''} - style={{ - width: '100%', - objectFit: 'cover', - maskImage: - 'linear-gradient(to bottom, rgba(0,0,0,1) 50%, transparent 100%)', - maskSize: 'cover', - pointerEvents: 'none' - }} - /> + {video.images[0]?.mobileCinematicHigh != null ? ( + <Image + fill + src={video.images[0].mobileCinematicHigh} + alt={video.imageAlt[0]?.value ?? ''} + style={{ + width: '100%', + objectFit: 'cover', + maskImage: + 'linear-gradient(to bottom, rgba(0,0,0,1) 50%, transparent 100%)', + maskSize: 'cover', + pointerEvents: 'none' + }} + /> + ) : ( + <div className="h-full w-full bg-gray-800" /> + )}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/resources/src/components/NewVideoContentPage/VideoCarousel/VideoCard/VideoCard.tsx` around lines 57 - 69, The Image usage in VideoCard (the JSX that renders <Image ... src={video.images[0]?.mobileCinematicHigh ?? ''} ... />) must not pass an empty string to next/image; change the component to only render the Image when a valid src exists (e.g. check video.images[0]?.mobileCinematicHigh truthiness) and otherwise render a safe fallback (placeholder div/skeleton or nothing) so next/image never receives '' or a falsy src; update the VideoCard component's JSX accordingly.apps/resources/src/components/VideoContentPage/VideoHero/VideoPlayer/VideoControls/VideoControls.tsx (2)
418-423:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDo not set fullscreen state to
truewhen mobile fullscreen request is skipped.If
playeris not ready, the request is not executed, but state is still forced to fullscreen. This desynchronizes UI state from actual fullscreen state.Minimal fix
if (isMobile()) { - if (isPlayerReady(player)) void player.requestFullscreen() + if (!isPlayerReady(player)) return + void player.requestFullscreen() dispatchPlayer({ type: 'SetFullscreen', fullscreen: true }) } else {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/resources/src/components/VideoContentPage/VideoHero/VideoPlayer/VideoControls/VideoControls.tsx` around lines 418 - 423, The code sets fullscreen state to true even when the mobile fullscreen request is skipped; change the logic so you only dispatchPlayer({ type: 'SetFullscreen', fullscreen: true }) when the player request actually runs: check isPlayerReady(player) first, call void player.requestFullscreen() and then dispatch the SetFullscreen action; if the player isn't ready, do not set fullscreen to true. Optionally attach a .catch to player.requestFullscreen() and only dispatch on success to avoid desynchronization.
228-400:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMissing cleanup for player/fscreen subscriptions can duplicate analytics and state updates.
This effect re-runs (notably when
loadingchanges) and adds new anonymous listeners each time, but never unregisters them. Over time, events will fan out and dispatch multiple times.Suggested direction
useEffect(() => { - player?.on('play', () => { + const handlePlayEvent = () => { // ... - }) + } + // define all other handlers similarly... + const handleFullscreenEvent = () => { + // ... + } - player?.on('pause', () => { ... }) + player?.on('play', handlePlayEvent) + // register other handlers with stable refs... + fscreen.addEventListener('fullscreenchange', handleFullscreenEvent) + + return () => { + player?.off('play', handlePlayEvent) + // unregister all other handlers... + fscreen.removeEventListener('fullscreenchange', handleFullscreenEvent) + } -}, [id, player, dispatchPlayer, loading, title, variant]) +}, [id, player, dispatchPlayer, title, variant])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/resources/src/components/VideoContentPage/VideoHero/VideoPlayer/VideoControls/VideoControls.tsx` around lines 228 - 400, The effect attaches many anonymous listeners to player and fscreen without removing them, causing duplicated analytics/state updates on re-runs; fix by extracting each listener into named handler functions (for play, pause, timeupdate, volumechange, fullscreenchange, useractive, userinactive, waiting, playing, ended, canplay, canplaythrough and the fscreen fullscreenchange handler) and register them with player.on(...) and fscreen.addEventListener(...), then return a cleanup function that calls player.off(handler) for each registered event and fscreen.removeEventListener(...) for the fullscreen handler; ensure you also clear any state side-effects if needed (e.g., avoid stale closures for dispatchPlayer, eventToDataLayer, setInitialLoadComplete) by referencing the same handlers in the cleanup.apps/watch/src/components/VideoControls/VideoControls.tsx (2)
239-411:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftEffect subscribes repeatedly without cleanup, causing event/listener accumulation.
This hook attaches fresh anonymous handlers on each re-run and never unregisters them. Because the dependency list includes state like
loading, duplicated analytics and repeated dispatches are likely.Suggested direction
useEffect(() => { - player?.on('play', () => { ... }) + const handlePlayEvent = () => { ... } + // define remaining handlers + const handleFullscreenEvent = () => { ... } + player?.on('play', handlePlayEvent) + // register other handlers + fscreen.addEventListener('fullscreenchange', handleFullscreenEvent) + + return () => { + player?.off('play', handlePlayEvent) + // unregister other handlers + fscreen.removeEventListener('fullscreenchange', handleFullscreenEvent) + } -}, [id, player, dispatchPlayer, loading, title, variant]) +}, [id, player, dispatchPlayer, title, variant])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/watch/src/components/VideoControls/VideoControls.tsx` around lines 239 - 411, The effect in useEffect is adding anonymous event handlers (player?.on(...) and fscreen.addEventListener(...)) on every rerun (e.g. due to loading) without removing them, causing listener accumulation; refactor by extracting stable handler functions (for play, pause, timeupdate, volumechange, fullscreenchange, useractive, userinactive, waiting, playing, ended, canplay, canplaythrough and the fscreen fullscreenchange handler), register them once inside the useEffect, and return a cleanup function that removes each listener via player.off(...) and fscreen.removeEventListener(...) so listeners are not duplicated; keep dispatchPlayer, setInitialLoadComplete, eventToDataLayer and helper calls (getPlayerCurrentTime, getPlayerDuration, getPlayerVolume, isPlayerReady, secondsToTimeFormat) inside those handlers and ensure dependencies only include truly changing refs (id, title, variant, player) or use refs for stable values to avoid re-subscribing.
429-434:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPrevent fullscreen UI state drift on mobile when player is unavailable.
fullscreenis set totrueeven when no fullscreen request is made because player readiness failed.Minimal fix
if (isMobile()) { - if (isPlayerReady(player)) void player.requestFullscreen() + if (!isPlayerReady(player)) return + void player.requestFullscreen() dispatchPlayer({ type: 'SetFullscreen', fullscreen: true }) } else {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/watch/src/components/VideoControls/VideoControls.tsx` around lines 429 - 434, The code sets fullscreen state unconditionally on mobile even when the player isn't ready; update the block so you only dispatch the 'SetFullscreen' action when the player is ready and the fullscreen request actually succeeds: call isPlayerReady(player) and attempt player.requestFullscreen(), and only after that succeeds (or inside the same conditional that proved player readiness) call dispatchPlayer({ type: 'SetFullscreen', fullscreen: true }); do not set fullscreen=true when player is unavailable or the request fails (handle the promise rejection or wrap in try/catch around player.requestFullscreen()).apps/watch/src/components/VideoBlock/VideoBlockPlayer/VideoControls/VideoControls.tsx (1)
655-660:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard fullscreen state update behind successful mobile request path.
When
playeris not ready, fullscreen request is skipped but state is still set totrue.Minimal fix
if (isMobile()) { - if (isPlayerReady(player)) void player.requestFullscreen() + if (!isPlayerReady(player)) return + void player.requestFullscreen() dispatchPlayer({ type: 'SetFullscreen', fullscreen: true }) } else {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/watch/src/components/VideoBlock/VideoBlockPlayer/VideoControls/VideoControls.tsx` around lines 655 - 660, When handling the mobile fullscreen path in VideoControls.tsx, avoid setting fullscreen state unless the player's requestFullscreen actually ran successfully: wrap the dispatchPlayer({ type: 'SetFullscreen', fullscreen: true }) behind the same isPlayerReady(player) check and only call it after awaiting and/or confirming player.requestFullscreen() succeeded (catch errors and skip dispatch on failure). Update the block that currently calls isMobile(), isPlayerReady(player), player.requestFullscreen and dispatchPlayer so that dispatching the 'SetFullscreen' action occurs only after a successful requestFullscreen call (or when isPlayerReady is true for non-mobile paths), referencing isMobile, isPlayerReady, player.requestFullscreen, and dispatchPlayer/'SetFullscreen'.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@apps/resources/src/components/NewVideoContentPage/VideoCarousel/VideoCard/VideoCard.tsx`:
- Around line 57-69: The Image usage in VideoCard (the JSX that renders <Image
... src={video.images[0]?.mobileCinematicHigh ?? ''} ... />) must not pass an
empty string to next/image; change the component to only render the Image when a
valid src exists (e.g. check video.images[0]?.mobileCinematicHigh truthiness)
and otherwise render a safe fallback (placeholder div/skeleton or nothing) so
next/image never receives '' or a falsy src; update the VideoCard component's
JSX accordingly.
In
`@apps/resources/src/components/VideoContentPage/VideoHero/VideoPlayer/VideoControls/VideoControls.tsx`:
- Around line 418-423: The code sets fullscreen state to true even when the
mobile fullscreen request is skipped; change the logic so you only
dispatchPlayer({ type: 'SetFullscreen', fullscreen: true }) when the player
request actually runs: check isPlayerReady(player) first, call void
player.requestFullscreen() and then dispatch the SetFullscreen action; if the
player isn't ready, do not set fullscreen to true. Optionally attach a .catch to
player.requestFullscreen() and only dispatch on success to avoid
desynchronization.
- Around line 228-400: The effect attaches many anonymous listeners to player
and fscreen without removing them, causing duplicated analytics/state updates on
re-runs; fix by extracting each listener into named handler functions (for play,
pause, timeupdate, volumechange, fullscreenchange, useractive, userinactive,
waiting, playing, ended, canplay, canplaythrough and the fscreen
fullscreenchange handler) and register them with player.on(...) and
fscreen.addEventListener(...), then return a cleanup function that calls
player.off(handler) for each registered event and
fscreen.removeEventListener(...) for the fullscreen handler; ensure you also
clear any state side-effects if needed (e.g., avoid stale closures for
dispatchPlayer, eventToDataLayer, setInitialLoadComplete) by referencing the
same handlers in the cleanup.
In
`@apps/watch/src/components/VideoBlock/VideoBlockPlayer/VideoControls/VideoControls.tsx`:
- Around line 655-660: When handling the mobile fullscreen path in
VideoControls.tsx, avoid setting fullscreen state unless the player's
requestFullscreen actually ran successfully: wrap the dispatchPlayer({ type:
'SetFullscreen', fullscreen: true }) behind the same isPlayerReady(player) check
and only call it after awaiting and/or confirming player.requestFullscreen()
succeeded (catch errors and skip dispatch on failure). Update the block that
currently calls isMobile(), isPlayerReady(player), player.requestFullscreen and
dispatchPlayer so that dispatching the 'SetFullscreen' action occurs only after
a successful requestFullscreen call (or when isPlayerReady is true for
non-mobile paths), referencing isMobile, isPlayerReady,
player.requestFullscreen, and dispatchPlayer/'SetFullscreen'.
In `@apps/watch/src/components/VideoControls/VideoControls.tsx`:
- Around line 239-411: The effect in useEffect is adding anonymous event
handlers (player?.on(...) and fscreen.addEventListener(...)) on every rerun
(e.g. due to loading) without removing them, causing listener accumulation;
refactor by extracting stable handler functions (for play, pause, timeupdate,
volumechange, fullscreenchange, useractive, userinactive, waiting, playing,
ended, canplay, canplaythrough and the fscreen fullscreenchange handler),
register them once inside the useEffect, and return a cleanup function that
removes each listener via player.off(...) and fscreen.removeEventListener(...)
so listeners are not duplicated; keep dispatchPlayer, setInitialLoadComplete,
eventToDataLayer and helper calls (getPlayerCurrentTime, getPlayerDuration,
getPlayerVolume, isPlayerReady, secondsToTimeFormat) inside those handlers and
ensure dependencies only include truly changing refs (id, title, variant,
player) or use refs for stable values to avoid re-subscribing.
- Around line 429-434: The code sets fullscreen state unconditionally on mobile
even when the player isn't ready; update the block so you only dispatch the
'SetFullscreen' action when the player is ready and the fullscreen request
actually succeeds: call isPlayerReady(player) and attempt
player.requestFullscreen(), and only after that succeeds (or inside the same
conditional that proved player readiness) call dispatchPlayer({ type:
'SetFullscreen', fullscreen: true }); do not set fullscreen=true when player is
unavailable or the request fails (handle the promise rejection or wrap in
try/catch around player.requestFullscreen()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e860d431-3a25-4051-9a91-e7730c3f6324
📒 Files selected for processing (6)
apps/resources/src/components/NewVideoContentPage/VideoCarousel/VideoCard/VideoCard.tsxapps/resources/src/components/VideoContentPage/VideoHero/VideoPlayer/VideoControls/VideoControls.spec.tsxapps/resources/src/components/VideoContentPage/VideoHero/VideoPlayer/VideoControls/VideoControls.tsxapps/watch/src/components/VideoBlock/VideoBlockPlayer/VideoControls/VideoControls.spec.tsxapps/watch/src/components/VideoBlock/VideoBlockPlayer/VideoControls/VideoControls.tsxapps/watch/src/components/VideoControls/VideoControls.tsx
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.
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests