Conversation
…o-improve-audio-and-subtitle
WalkthroughRefactors the watch app’s language surface to be ID-centric: introduces an SWR-based useLanguages hook and tests, omits nameless languages from /api/languages, replaces slug/LanguageContext and slug-based GraphQL hooks with ID-based queries/actions, refactors language UI to prop-driven MUI Autocomplete, and updates many pages, context types, middleware, tests and stories. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Audio/Subtitles UI
participant Hook as useLanguages (SWR)
participant API as /api/languages
participant Actions as useLanguageActions
User->>UI: Open language picker
UI->>Hook: call useLanguages()
Hook->>API: GET /api/languages
API-->>Hook: rows [[id:slug:native], "id:name", ...]
Hook-->>UI: languages[], isLoading=false
UI->>UI: order by videoAudioLanguageIds, group Available/Other
User->>UI: Select language
UI->>Actions: updateAudioLanguage({id, slug}) / updateSubtitleLanguage({id})
alt reload=true
Actions->>Router: router.push(asPath with slug)
else
Actions->>WatchContext: dispatch SetLanguagePreferences
end
sequenceDiagram
autonumber
participant Watch as WatchContext
participant Hook as useSubtitleUpdate
participant Player as Video.js Player
Note over Watch: subtitleLanguageId, subtitleOn
Watch->>Hook: subtitleUpdate({player, subtitleLanguageId, subtitleOn})
alt subtitleOn true && subtitleLanguageId set
Hook->>GQL: GET_SUBTITLES(videoId)
GQL-->>Hook: variant.subtitle[]
Hook->>Player: addRemoteTextTrack({id: subtitleLanguageId, src, label, srclang})
Hook->>Player: enable matching track, disable others
else
Hook->>Player: disable all subtitle tracks
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
View your CI Pipeline Execution ↗ for commit 66818bb
☁️ Nx Cloud last updated this comment at |
|
View your CI Pipeline Execution ↗ for commit 612bd32
☁️ Nx Cloud last updated this comment at |
tataihono
changed the title
tataihono
changed the title
github-actions
bot
temporarily deployed
to
Preview - journeys-admin
|
The latest updates on your projects.
|
|
The latest updates on your projects.
|
|
The latest updates on your projects.
|
|
The latest updates on your projects.
|
|
The latest updates on your projects.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (14)
apps/watch/src/components/NewVideoContentPage/NewVideoContentHeader/NewVideoContentHeader.tsx (1)
65-74: Fix interactive-in-interactive: button inside Link + remove no-op passHref
NextLinkrenders an anchor; nesting a<button>inside it is invalid and harms accessibility. Also,passHrefis unnecessary here. Render the anchor styled as a button (or use MUI Button with NextLink integration per repo guidelines).- <NextLink - href={`/watch/${container.variant?.slug as string}`} - locale={false} - passHref - > - <button className="border border-[#bbbcbc] rounded-md px-2 py-1 text-sm text-[#bbbcbc] hidden xl:block cursor-pointer font-bold"> - {container.label === VideoLabel.featureFilm - ? 'Watch Full Film' - : 'See All'} - </button> - </NextLink> + {container.variant?.slug && ( + <NextLink + href={`/watch/${container.variant.slug}`} + locale={false} + className="border border-[#bbbcbc] rounded-md px-2 py-1 text-sm text-[#bbbcbc] hidden xl:block cursor-pointer font-bold" + > + {container.label === VideoLabel.featureFilm + ? t('Watch Full Film') + : t('See All')} + </NextLink> + )}Optional (preferred per apps/* guideline): replace with MUI Button.
// outside diff, example // import Button from '@mui/material/Button'; // <Button // component={NextLink} // href={`/watch/${container.variant?.slug ?? ''}`} // locale={false} // variant="outlined" // size="small" // sx={{ display: { xs: 'none', xl: 'inline-flex' }, fontWeight: 'bold', color: '#bbbcbc', borderColor: '#bbbcbc' }} // > // {container.label === VideoLabel.featureFilm ? t('Watch Full Film') : t('See All')} // </Button>apps/watch/src/components/VideoContentPage/VideoHero/VideoPlayer/VideoControls/VideoControls.tsx (2)
170-185: Listener cleanup bug: off() receives a different function than on().You add anonymous wrappers on on(), but call off() with updateDuration. This leaks listeners and retries.
- const events = ['durationchange', 'loadedmetadata', 'canplay'] - events.forEach((event) => { - player.on(event, () => updateDuration(event)) - }) + const events = ['durationchange', 'loadedmetadata', 'canplay'] as const + const listeners = events.map((event) => { + const fn = () => updateDuration(event) + player.on(event, fn) + return { event, fn } + }) ... - events.forEach((event) => { - player.off(event, updateDuration) - }) + listeners.forEach(({ event, fn }) => { + player.off(event, fn) + })
353-386: Add cleanup for global fullscreenchange listener.Without removeEventListener, this accumulates across re-renders.
- fscreen.addEventListener('fullscreenchange', () => { + const onFullscreenChange = () => { if (fscreen.fullscreenElement != null) { eventToDataLayer( 'video_enter_full_screen', id, variant?.language.id, title[0].value, variant?.language?.name.find(({ primary }) => !primary)?.value ?? variant?.language?.name[0]?.value, Math.round(player?.currentTime() ?? 0), Math.round( ((player?.currentTime() ?? 0) / (player.duration() ?? 1)) * 100 ) ) } else { eventToDataLayer( 'video_exit_full_screen', id, variant?.language.id, title[0].value, variant?.language?.name.find(({ primary }) => !primary)?.value ?? variant?.language?.name[0]?.value, Math.round(player?.currentTime() ?? 0), Math.round( ((player?.currentTime() ?? 0) / (player.duration() ?? 1)) * 100 ) ) } dispatchPlayer({ type: 'SetFullscreen', fullscreen: fscreen.fullscreenElement != null }) - }) + } + fscreen.addEventListener('fullscreenchange', onFullscreenChange) + return () => { + fscreen.removeEventListener('fullscreenchange', onFullscreenChange) + }apps/watch/src/components/VideoContentPage/VideoHero/VideoPlayer/VideoPlayer.tsx (1)
33-64: Dispose video.js player to prevent leaks and duplicate instancesAdd a cleanup that calls player.dispose() when the component unmounts or re-initializes.
useEffect(() => { if (videoRef.current != null) { // Create Mux metadata for video analytics const muxMetadata: MuxMetadata = { env_key: process.env.NEXT_PUBLIC_MUX_DEFAULT_REPORTING_KEY || '', player_name: 'watch', video_title: last(title)?.value ?? '', video_id: variant?.id ?? '' } - setPlayer( - videojs(videoRef.current, { + const instance = videojs(videoRef.current, { ...defaultVideoJsOptions, autoplay: true, controls: false, controlBar: false, bigPlayButton: false, userActions: { hotkeys: true, doubleClick: true }, responsive: true, plugins: { mux: { debug: false, data: muxMetadata } } - }) - ) + }) + setPlayer(instance) } - }, [variant, videoRef, title]) + return () => { + setPlayer(undefined) + instance?.dispose() + } + }, [variant, title])apps/watch/pages/watch/index.tsx (1)
28-32: Type mismatch:flagsreturned by getStaticProps isn’t in HomePagePropsAdd a
flagsfield to HomePageProps with the correct type to satisfy Next.js typing.interface HomePageProps { initialApolloState?: NormalizedCacheObject serverState?: InstantSearchServerState localLanguageId?: string + // ensure compatibility with getStaticProps return + flags?: Awaited<ReturnType<typeof getFlags>> }Also applies to: 83-95
apps/watch/src/libs/watchContext/useSubtitleUpdate/useSubtitleUpdate.tsx (2)
43-55: Do not construct TextTrackList; it has no public constructor.
new TextTrackList()will throw at runtime in browsers. Guard for absence instead.- const tracks = player.textTracks?.() ?? new TextTrackList() + const tracks = player.textTracks?.() + if (tracks == null) return
81-92: Same issue: avoidnew TextTrackList()when updating modes.- const updatedTracks = player.textTracks?.() ?? new TextTrackList() + const updatedTracks = player.textTracks?.() + if (updatedTracks == null) returnapps/watch/src/components/VideoContentPage/VideoHero/VideoPlayer/VideoControls/VideoControls.spec.tsx (4)
46-75: Dispose the video.js player and remove the appendedWe’re appending a video element and creating a player per test without disposing, which can leak timers/listeners and cause flakiness across tests.
- let player + let player + let videoEl: HTMLVideoElement + let originalUA: string beforeEach(() => { jest.clearAllMocks() - const video = document.createElement('video') - document.body.appendChild(video) - player = videojs(video, { + originalUA = window.navigator.userAgent + videoEl = document.createElement('video') + document.body.appendChild(videoEl) + player = videojs(videoEl, { ...defaultVideoJsOptions, autoplay: false, controls: true, userActions: { hotkeys: true, doubleClick: true }, controlBar: { playToggle: true, remainingTimeDisplay: true, progressControl: { seekBar: true }, fullscreenToggle: true, volumePanel: { inline: false } }, responsive: true }) act(() => { player.duration(250) }) useLanguagesMock.mockReturnValue({ languages: [], isLoading: false }) }) afterEach(() => { - cleanup() + try { + player?.dispose?.() + } catch {} + try { + videoEl?.remove?.() + } catch {} + // restore UA if changed + try { + Object.defineProperty(window.navigator, 'userAgent', { + value: originalUA, + configurable: true + }) + } catch {} + cleanup() })Also applies to: 81-83
201-201: Don’t assign navigator.userAgent directly (read-only in JSDOM).Direct assignment may throw. Use defineProperty and restore after.
-;(global.navigator.userAgent as unknown as string) = 'iPhone' +Object.defineProperty(window.navigator, 'userAgent', { + value: 'iPhone', + configurable: true +})-;(global.navigator.userAgent as unknown as string) = 'Mac' +Object.defineProperty(window.navigator, 'userAgent', { + value: 'Mac', + configurable: true +})Also applies to: 228-228
285-312: Avoid re-spying on the same method.Calling jest.spyOn twice on the same method in one test can throw. Reuse the existing spy and change its return value.
- jest.spyOn(player, 'currentTime').mockReturnValue(0) + const currentTimeSpy = jest.spyOn(player, 'currentTime').mockReturnValue(0) render(… act(() => { - jest.spyOn(player, 'currentTime').mockReturnValue(50) + currentTimeSpy.mockReturnValue(50) // event needs to be triggered manually because of jsdom limitations player.trigger('timeupdate') })
318-347: Same issue for volume spy.Reuse the spy instead of re-spying.
- jest.spyOn(player, 'volume').mockReturnValue(0) + const volumeSpy = jest.spyOn(player, 'volume').mockReturnValue(0) … act(() => { - jest.spyOn(player, 'volume').mockReturnValue(0.5) + volumeSpy.mockReturnValue(0.5) // event needs to be triggered manually because of jsdom limitations player.trigger('volumechange') })apps/watch/src/components/LanguageSwitchDialog/SubtitlesSelect/SubtitlesSelect.tsx (1)
81-86: Associate label with inputhtmlFor="subtitles-select" doesn’t match any input id. Pass id to TextField.
- <TextField + <TextField {...params} + id="subtitles-select" hiddenLabel variant="filled" helperText={helperText} />Also applies to: 138-145
apps/watch/src/libs/watchContext/WatchContext.tsx (2)
133-138: Type usage: import ReactNode and avoid React namespace types.You reference React.ReactNode without importing React. Import ReactNode as a type and use it directly to avoid TS errors in stricter configs.
Apply this diff:
-import { - Dispatch, - createContext, - useContext, - useEffect, - useReducer -} from 'react' +import type { ReactNode } from 'react' +import { Dispatch, createContext, useContext, useEffect, useReducer } from 'react'interface WatchProviderProps { /** Child components that will have access to the watch context */ - children: React.ReactNode + children: ReactNode /** Initial state for the watch context */ initialState?: WatchState }
188-201: Docs example uses old API names.The JSDoc still shows audioLanguage, not audioLanguageId, and reads state.audioLanguage. Update to the new ID-based API.
Apply this doc fix:
- * const handleLanguageChange = (language: string) => { + * const handleLanguageChange = (languageId: string) => { ... - * audioLanguage: language + * audioLanguageId: languageId ... - * return <div>Current language: {state.audioLanguage}</div> + * return <div>Current audio language: {state.audioLanguageId}</div>
♻️ Duplicate comments (2)
apps/watch/pages/watch/easter.html/french.html/index.tsx (1)
51-51: Consistent with the refactor; good to go.Same consideration as the Spanish page regarding lingering LanguageContext references.
apps/watch/src/components/LanguageSwitchDialog/SubtitlesSelect/SubtitlesSelect.spec.tsx (1)
187-205: Avoid asserting action mocks (see earlier comment)This test asserts
updateSubtitleLanguagedirectly; prefer verifying the UI reflects the selection instead. If you keep it, make the option query less brittle:- await userEvent.click( - screen.getByRole('option', { name: 'French Français' }) - ) + await userEvent.click( + await screen.findByRole('option', { name: /French/i }) + )
🧹 Nitpick comments (66)
apps/watch/src/libs/videoContext/VideoContext.tsx (2)
19-21: Fix hook name in error message.The hook is
useVideo, but the error saysuseVideos.- throw new Error('useVideos must be used within a VideoProvider') + throw new Error('useVideo must be used within a VideoProvider')
36-38: Avoid leaking __typename into the context value.Strip __typename before providing to match the Context type and reduce accidental downstream coupling.
export function VideoProvider({ children, value }: VideoProviderProps): ReactElement { - return ( + // Omit __typename from the provided content + const { __typename: _t, ...content } = value.content + return ( <VideoContext.Provider - value={{ ...value.content, container: value.container }} + value={{ ...content, container: value.container }} > {children} </VideoContext.Provider> ) }apps/watch/src/components/NewVideoContentPage/NewVideoContentPage.spec.tsx (1)
1-1: Remove MockedProvider wrapper and import from NewVideoContentPage.spec.tsxThe component tree has no Apollo queries (no useQuery/useMutation), so the MockedProvider import and its JSX wrapper in the spec file add unnecessary overhead and can be removed.
apps/watch/src/components/VideoContentPage/VideoHero/VideoPlayer/VideoControls/VideoControls.tsx (2)
128-129: Use browser-safe timeout types.Prefer ReturnType over NodeJS.Timeout in browser code.
- let retryTimeout: NodeJS.Timeout | undefined + let retryTimeout: ReturnType<typeof setTimeout> | undefined- let timeoutID: NodeJS.Timeout | undefined + let timeoutID: ReturnType<typeof setTimeout> | undefinedAlso applies to: 454-456
661-672: Prefer MUI components over p/span inside Typography.Aligns with apps/* guideline and improves consistency.
- <span> - <p className="font-sans">{currentTime ?? '0:00'}</p> - </span> + <Box component="span"> + <Typography component="span" className="font-sans"> + {currentTime ?? '0:00'} + </Typography> + </Box> ... - <span> - <p className="font-sans">{duration}</p> - </span> + <Box component="span"> + <Typography component="span" className="font-sans"> + {duration} + </Typography> + </Box>apps/watch/src/components/VideoContainerPage/AudioLanguageSelect/AudioLanguageSelectContent/AudioLanguageSelectContent.tsx (2)
15-18: Guard empty ID list to skip work.Early return avoids filter when videoAudioLanguageIds is empty/undefined.
- const filteredLanguages = useMemo( - () => languages.filter(({ id }) => videoAudioLanguageIds?.includes(id)), - [languages, videoAudioLanguageIds] - ) + const filteredLanguages = useMemo(() => { + if (!videoAudioLanguageIds?.length) return [] + return languages.filter(({ id }) => videoAudioLanguageIds.includes(id)) + }, [languages, videoAudioLanguageIds])
23-59: Use Next.js Link + MUI Link instead of raw .Meets apps/* MUI guideline, enables prefetching, and keeps a11y.
- <a + <Link + passHref key={option.id} - href={`/watch${ + href={`/watch${ variant?.slug != null ? `/${variant.slug.split('/')[0]}.html/` : '/' }${option.slug}.html`} - className={` + > + <MuiLink + className={` block hover:bg-gray-100 focus:bg-gray-100 data-[highlighted]:bg-gray-100 data-[state=checked]:bg-blue-50 data-[state=checked]:text-blue-900 cursor-pointer p-2 rounded `} role="option" aria-label={option.displayName} > <div className="flex items-center gap-1"> <span className="text-sm text-black font-sans" data-testid="AudioLanguageSelectDisplayLanguageName" > {option.displayName} </span> {option.nativeName?.value && ( <span className="text-xs text-gray-600 font-sans" data-testid="AudioLanguageSelectNativeLanguageName" > ({option.nativeName?.value}) </span> )} </div> - </a> + </MuiLink> + </Link>Add imports at top:
+import Link from 'next/link' +import MuiLink from '@mui/material/Link'apps/watch/src/components/Header/LocalAppBar/LocalAppBar.spec.tsx (1)
91-93: Good: scopes WatchProvider only where neededLimiting the provider to the dialog test keeps other tests lean. Consider userEvent.click for more realistic interaction, but not required.
apps/watch/src/components/NewVideoContentPage/VideoContentHero/VideoContentHero.spec.tsx (1)
36-65: Reduce duplication with a small render helperThis spec repeats the same provider scaffolding across tests. Consider a tiny factory to render with providers to cut noise and ease future changes.
Example:
function renderWithProviders(ui: React.ReactElement, watchInit = { audioLanguageId: '529', subtitleLanguageId: '529', subtitleOn: false }) { return render( <MockedProvider> <VideoProvider value={{ content: videos[0] }}> <WatchProvider initialState={watchInit}>{ui}</WatchProvider> </VideoProvider> </MockedProvider> ) }apps/watch/src/components/VideoContentPage/VideoHero/VideoPlayer/VideoPlayer.tsx (1)
66-72: Guard player.src() when HLS URL is missingAvoid setting an empty source to the player; early-return if hls is falsy.
useEffect(() => { - player?.src({ - src: variant?.hls ?? '', - type: 'application/x-mpegURL' - }) + if (!player || !variant?.hls) return + player.src({ + src: variant.hls, + type: 'application/x-mpegURL' + }) }, [player, variant?.hls])apps/watch/src/libs/watchContext/useSubtitleUpdate/useSubtitleUpdate.spec.tsx (3)
74-79: Strengthen assertion: verify the chosen track is “showing” and others are “disabled”Adds confidence that the hook flips modes correctly.
await result.current.subtitleUpdate({ player: mockPlayer, subtitleLanguageId: '529', subtitleOn: true }) - expect(result.current.subtitleUpdate).toBeDefined() - expect(result.current.subtitlesLoading).toBeDefined() + const tracks = mockPlayer.textTracks?.() + if (tracks) { + for (let i = 0; i < tracks.length; i++) { + const t = tracks[i] + if (t.kind === 'subtitles') { + expect(t.mode).toBe(t.id === '529' ? 'showing' : 'disabled') + } + } + } await waitFor(() => { expect(getSubtitlesMockResults).toHaveBeenCalled() })
98-102: Also assert we don’t fetch subtitles when subtitles are offThe hook should bail early; ensure no GraphQL call is made.
-const { result } = renderHook(() => useSubtitleUpdate(), { +const getSubtitlesMockResults = jest.fn().mockReturnValue({ ...getSubtitlesMock.result }) +const { result } = renderHook(() => useSubtitleUpdate(), { wrapper: ({ children }) => ( - <MockedProvider mocks={[getSubtitlesMock]} addTypename={false}> + <MockedProvider + mocks={[{ ...getSubtitlesMock, result: getSubtitlesMockResults }]} + addTypename={false} + > <VideoProvider value={{ content: mockVideoContent }}> {children} </VideoProvider> </MockedProvider> ) }) await result.current.subtitleUpdate({ player: mockPlayer, subtitleLanguageId: '529', subtitleOn: false }) +expect(getSubtitlesMockResults).not.toHaveBeenCalled()
126-130: Likewise, avoid fetch when subtitleLanguageId is null and ensure tracks disabledMirror the “off” case to prove early-return behavior.
-const { result } = renderHook(() => useSubtitleUpdate(), { +const getSubtitlesMockResults = jest.fn().mockReturnValue({ ...getSubtitlesMock.result }) +const { result } = renderHook(() => useSubtitleUpdate(), { wrapper: ({ children }) => ( - <MockedProvider mocks={[getSubtitlesMock]} addTypename={false}> + <MockedProvider + mocks={[{ ...getSubtitlesMock, result: getSubtitlesMockResults }]} + addTypename={false} + > <VideoProvider value={{ content: mockVideoContent }}> {children} </VideoProvider> </MockedProvider> ) }) await result.current.subtitleUpdate({ player: mockPlayer, subtitleLanguageId: null, subtitleOn: true }) +expect(getSubtitlesMockResults).not.toHaveBeenCalled()apps/watch/pages/watch/index.tsx (1)
45-49: Prefer locale-based fallback over hard-coded '529'Use the computed localLanguageId when cookies are absent to better match user locale.
- const initialWatchState: WatchState = { - audioLanguageId: getCookie('AUDIO_LANGUAGE') ?? '529', - subtitleLanguageId: getCookie('SUBTITLE_LANGUAGE') ?? '529', - subtitleOn: getCookie('SUBTITLES_ON') === 'true' - } + const initialWatchState: WatchState = { + audioLanguageId: getCookie('AUDIO_LANGUAGE') ?? localLanguageId ?? '529', + subtitleLanguageId: getCookie('SUBTITLE_LANGUAGE') ?? localLanguageId ?? '529', + subtitleOn: getCookie('SUBTITLES_ON') === 'true' + }apps/watch/src/libs/watchContext/TestWatchState.tsx (1)
13-21: Stabilize display values for easier test assertionsCoalesce subtitleOn to false for consistent output.
- <div>subtitleOn: {state.subtitleOn?.toString()}</div> + <div>subtitleOn: {(state.subtitleOn ?? false).toString()}</div>apps/watch/src/components/LanguageSwitchDialog/SubtitlesSelect/SubtitlesSelect.spec.tsx (5)
10-19: Prefer interaction-focused tests over asserting mocked callsTeam learning notes indicate avoiding direct assertions on action mocks in these tests; instead verify user interactions and resulting UI state. Consider dropping the
useLanguageActionsmock and asserting observable effects (e.g., helper text changes, selected value) to align with the existing strategy.
31-36: Fix englishName.id in test fixtures to match contract (id '529' for English)
englishName.idshould be '529' (English) for all languages. Using the language’s own id is inconsistent withuseLanguages’s data contract and may hide bugs.Apply:
@@ const french = { id: '496', slug: 'french', displayName: 'French', name: { id: '529', value: 'French', primary: false }, - englishName: { id: '496', value: 'French', primary: false }, + englishName: { id: '529', value: 'French', primary: false }, nativeName: { id: '496', value: 'Français', primary: true } } @@ { id: '21028', slug: 'spanish', displayName: 'Spanish', name: { id: '21028', value: 'Spanish', primary: false }, - englishName: { id: '21028', value: 'Spanish', primary: false }, + englishName: { id: '529', value: 'Spanish', primary: false }, nativeName: { id: '21028', value: 'Español', primary: true } }Also applies to: 52-58
168-185: Use role='listbox' and within() for robust Autocomplete assertionsMUI Autocomplete uses listbox/option roles; querying 'list' is fragile. Also prefer scoping with within().
Apply:
@@ - await waitFor(() => { - expect(screen.getByText('Available Languages')).toBeInTheDocument() - expect(screen.getByText('Other Languages')).toBeInTheDocument() - }) - // available languages - expect(screen.getAllByRole('list')[0].children[0]).toHaveTextContent( - 'English' - ) - expect(screen.getAllByRole('list')[0].children[1]).toHaveTextContent( - 'FrenchFrançais' - ) - // other languages - expect(screen.getAllByRole('list')[1].children[0]).toHaveTextContent( - 'SpanishEspañol' - ) + await waitFor(() => { + expect(screen.getByText('Available Languages')).toBeInTheDocument() + expect(screen.getByText('Other Languages')).toBeInTheDocument() + }) + const lists = screen.getAllByRole('listbox') + expect(lists).toHaveLength(2) + // available languages + expect(within(lists[0]).getByText('English')).toBeInTheDocument() + expect(within(lists[0]).getByText('French')).toBeInTheDocument() + expect(within(lists[0]).getByText('Français')).toBeInTheDocument() + // other languages + expect(within(lists[1]).getByText('Spanish')).toBeInTheDocument() + expect(within(lists[1]).getByText('Español')).toBeInTheDocument()Also add the missing import:
-import { render, screen, waitFor } from '@testing-library/react' +import { render, screen, waitFor, within } from '@testing-library/react'
208-223: Rename test to match function nameUse “updateSubtitlesOn” (plural) in the test description for clarity.
-it('should call updateSubtitleOn when checkbox is changed', async () => { +it('should call updateSubtitlesOn when checkbox is changed', async () => {
1-1: Remove unnecessary MockedProvider wrappersThese tests don’t use Apollo; the MockedProvider adds noise. Render with just WatchProvider.
Example (apply similarly to other occurrences):
-import { MockedProvider } from '@apollo/client/testing' @@ - render( - <MockedProvider mocks={[]} addTypename={false}> - <WatchProvider> - <SubtitlesSelect subtitleLanguageId="496" /> - </WatchProvider> - </MockedProvider> - ) + render( + <WatchProvider> + <SubtitlesSelect subtitleLanguageId="496" /> + </WatchProvider> + )Also applies to: 65-71, 82-88, 101-107, 115-124, 134-143, 157-166, 188-194, 209-215
apps/watch/src/components/NewVideoContentPage/VideoContentHero/HeroVideo/HeroVideo.tsx (1)
114-124: Guard subtitleUpdate on player readiness; tighten depsAvoid running updates before the player/text tracks are ready and after disposal. Gate on
playerReadyand removevariantfrom deps to prevent extra runs tied to object identity.- useEffect(() => { + useEffect(() => { const player = playerRef.current - if (player == null) return + if (player == null || !playerReady) return @@ - }) - }, [playerRef, subtitleLanguageId, subtitleOn, variant, mute]) + }) + }, [playerRef, playerReady, subtitleLanguageId, subtitleOn, mute])Can you confirm
useSubtitleUpdaterequires a ready player (text tracks available)? If so, the guard above prevents race conditions when the player is recreated.apps/watch/pages/watch/[part1].tsx (1)
50-54: Optional: memoize initialWatchStateAvoid recreating the object on re-renders (even if it currently has no effect on reducer init).
-import { renderToString } from 'react-dom/server' +import { renderToString } from 'react-dom/server' +import { useMemo } from 'react' @@ - const initialWatchState: WatchState = { - audioLanguageId: getCookie('AUDIO_LANGUAGE') ?? languageId, - subtitleLanguageId: getCookie('SUBTITLE_LANGUAGE') ?? languageId, - subtitleOn: getCookie('SUBTITLES_ON') === 'true' - } + const initialWatchState: WatchState = useMemo( + () => ({ + audioLanguageId: getCookie('AUDIO_LANGUAGE') ?? languageId, + subtitleLanguageId: getCookie('SUBTITLE_LANGUAGE') ?? languageId, + subtitleOn: getCookie('SUBTITLES_ON') === 'true' + }), + [languageId] + )apps/watch/src/libs/useLanguages/useLanguages.spec.tsx (2)
13-23: Wrapper children type should be ReactNode, not ReactElement.Testing-library wrappers accept ReactNode; using ReactElement narrows valid children and can cause TS friction.
- return ({ children }: { children: ReactElement }) => { + return ({ children }: { children: React.ReactNode }) => {
25-25: Reset MSW handlers between tests to avoid leakage.Ensure per-test isolation.
describe('useLanguages', () => { + afterEach(() => { + server.resetHandlers() + })If this already exists in a global test setup, ignore.
apps/watch/src/components/VideoContainerPage/AudioLanguageSelect/AudioLanguageSelect.tsx (4)
23-31: Add a named handler and fallback label for robustness and readability.Avoid inline handlers per repo style and show a sane label while languages load.
const { state: { audioLanguageId, videoAudioLanguageIds } } = useWatch() const [open, setOpen] = useState<boolean | null>(null) const { languages } = useLanguages() const language = useMemo( () => languages.find(({ id }) => id === audioLanguageId), [languages, audioLanguageId] ) + const handleOpenChange = (nextOpen: boolean): void => { + setOpen(nextOpen) + }
33-40: Use the named handler.- onOpenChange={(open) => { - setOpen(open) - }} + onOpenChange={handleOpenChange}
66-79: Provide a display fallback when the language hasn’t resolved.Prevents an empty label when data isn’t ready.
- {language?.displayName} + {language?.displayName ?? audioLanguageId ?? ''}
41-101: Consider MUI primitives for markup.Replace div/span + classes with MUI Box/Typography for consistency with apps/watch guideline.
apps/watch/src/libs/watchContext/useSubtitleUpdate/useSubtitleUpdate.tsx (2)
34-36: Add generics to useLazyQuery for type safety.-import { graphql } from '@core/shared/gql' +import { graphql, ResultOf, VariablesOf } from '@core/shared/gql' ... -export const GET_SUBTITLES = graphql(` +export const GET_SUBTITLES = graphql(` query GetSubtitles($id: ID!) { ... -`) +`) +type GetSubtitles = ResultOf<typeof GET_SUBTITLES> +type GetSubtitlesVariables = VariablesOf<typeof GET_SUBTITLES> ... - const [getSubtitleLanguages, { loading: subtitlesLoading }] = - useLazyQuery(GET_SUBTITLES) + const [getSubtitleLanguages, { loading: subtitlesLoading }] = + useLazyQuery<GetSubtitles, GetSubtitlesVariables>(GET_SUBTITLES)
68-79: Avoid duplicate remote tracks for the same language.Consider removing an existing track with the same id before adding a new one.
+ // Remove any existing track for this language id to prevent duplicates + for (let i = 0; i < tracks.length; i++) { + const track = tracks[i] + if (track.kind === 'subtitles' && track.id === subtitleLanguageId) { + // @ts-expect-error video.js exposes removeRemoteTextTrack at runtime + player.removeRemoteTextTrack?.(track) + } + }apps/watch/src/components/LanguageSwitchDialog/LanguageSwitchDialog.spec.tsx (3)
18-25: Fix englishName.id for French.English name should carry id '529' (English), not '496'.
- englishName: { id: '496', value: 'French', primary: false }, + englishName: { id: '529', value: 'French', primary: false },
120-137: Assertions index into lists; prefer role/text scoping to reduce brittleness.Target the “Available Languages”/“Other Languages” groups or headings if present, rather than list indices.
121-124: Combobox value assertion may be implementation-specific.Custom Selects often don’t expose “value” as the visible label. Consider asserting visible text instead.
- expect(screen.getAllByRole('combobox')[0]).toHaveValue('English') + expect(screen.getAllByRole('combobox')[0]).toHaveTextContent('English')If your Select already maps value to label, ignore.
apps/watch/src/components/VideoContainerPage/AudioLanguageSelect/AudioLanguageSelect.spec.tsx (3)
25-65: Deduplicate shadcn Select DOM/polyfill scaffolding into a shared test util.This block is duplicated across specs and adds noise. Extract to a reusable helper (e.g.,
apps/watch/test/setupShadcnSelect.ts) and import it in both specs.Example helper (new file, outside current ranges):
// apps/watch/test/setupShadcnSelect.ts export function installShadcnSelectTestPolyfills() { const originalPointerEvent = (window as any).PointerEvent const originalHTMLElementMethods = { scrollIntoView: window.HTMLElement.prototype.scrollIntoView, releasePointerCapture: window.HTMLElement.prototype.releasePointerCapture, hasPointerCapture: window.HTMLElement.prototype.hasPointerCapture } function createMockPointerEvent(type: string, props: PointerEventInit = {}) { const event = new Event(type, props) as PointerEvent Object.assign(event, { button: props.button ?? 0, ctrlKey: props.ctrlKey ?? false, pointerType: props.pointerType ?? 'mouse' }) return event } Object.defineProperty(window, 'PointerEvent', { writable: true, value: createMockPointerEvent as any }) Object.assign(window.HTMLElement.prototype, { scrollIntoView: jest.fn(), releasePointerCapture: jest.fn(), hasPointerCapture: jest.fn() }) return () => { Object.assign(window.HTMLElement.prototype, originalHTMLElementMethods) Object.defineProperty(window, 'PointerEvent', { writable: true, value: originalPointerEvent }) } }Then in each spec:
const uninstall = installShadcnSelectTestPolyfills() afterEach(uninstall)
1-1: Remove unused Apollo MockedProvider wrapper to simplify tests.These tests no longer fetch via Apollo; the extra provider adds setup cost without value.
Apply:
- import { MockedProvider } from '@apollo/client/testing' + // Apollo MockedProvider not needed @@ - render( - <MockedProvider mocks={[]}> - <VideoProvider value={defaultProps}> - <WatchProvider - initialState={{ - audioLanguageId: '529', - videoAudioLanguageIds: ['529', '496'] - }} - > - <AudioLanguageSelect /> - </WatchProvider> - </VideoProvider> - </MockedProvider> - ) + render( + <VideoProvider value={defaultProps}> + <WatchProvider + initialState={{ + audioLanguageId: '529', + videoAudioLanguageIds: ['529', '496'] + }} + > + <AudioLanguageSelect /> + </WatchProvider> + </VideoProvider> + )Also applies to: 95-108
110-118: Tighten assertions to avoid unnecessary async.
Englishis rendered immediately;waitForisn’t needed. Prefer synchronousgetByTextfor stability.- await userEvent.hover(screen.getByTestId('AudioLanguageSelectTrigger')) - await waitFor(() => { - expect(screen.getByText('English')).toBeInTheDocument() - }) + await userEvent.hover(screen.getByTestId('AudioLanguageSelectTrigger')) // keep if needed for opening behavior elsewhere + expect(screen.getByText('English')).toBeInTheDocument()apps/watch/src/components/VideoContainerPage/AudioLanguageSelect/AudioLanguageSelectContent/AudioLanguageSelectContent.spec.tsx (3)
27-66: Extract shadcn Select polyfills to a shared helper.Same suggestion as the sibling spec to reduce duplication and boilerplate.
98-112: Drop Apollo MockedProvider here as well.No GraphQL usage in this test path.
- render( - <MockedProvider mocks={[]}> - <VideoProvider value={defaultProps}> - <WatchProvider - initialState={{ videoAudioLanguageIds: ['529', '496', '21028'] }} - > - <Select> - <SelectTrigger data-testid="TestSelectTrigger"> - {'Test Select Trigger'} - </SelectTrigger> - <AudioLanguageSelectContent /> - </Select> - </WatchProvider> - </VideoProvider> - </MockedProvider> - ) + render( + <VideoProvider value={defaultProps}> + <WatchProvider initialState={{ videoAudioLanguageIds: ['529', '496', '21028'] }}> + <Select> + <SelectTrigger data-testid="TestSelectTrigger"> + {'Test Select Trigger'} + </SelectTrigger> + <AudioLanguageSelectContent /> + </Select> + </WatchProvider> + </VideoProvider> + )
120-128: Avoid order-dependent assertions; select options by accessible name.Relying on array order can be brittle if sorting changes. Query each option by its
aria-label.- await waitFor(() => { - const [english, french, spanish] = screen.getAllByRole('option') - expect(english).toHaveAttribute('href', '/watch/jesus.html/english.html') - expect(english).toHaveTextContent('English') - expect(french).toHaveAttribute('href', '/watch/jesus.html/french.html') - expect(french).toHaveTextContent('French(Français)') - expect(spanish).toHaveAttribute('href', '/watch/jesus.html/spanish.html') - expect(spanish).toHaveTextContent('Spanish(Español)') - }) + const english = await screen.findByRole('option', { name: 'English' }) + const french = await screen.findByRole('option', { name: 'French' }) + const spanish = await screen.findByRole('option', { name: 'Spanish' }) + expect(english).toHaveAttribute('href', '/watch/jesus.html/english.html') + expect(english).toHaveTextContent('English') + expect(french).toHaveAttribute('href', '/watch/jesus.html/french.html') + expect(french).toHaveTextContent('French(Français)') + expect(spanish).toHaveAttribute('href', '/watch/jesus.html/spanish.html') + expect(spanish).toHaveTextContent('Spanish(Español)')apps/watch/src/libs/useLanguages/useLanguages.ts (4)
63-70: Use strict equality and avoid magic literals for the English ID.
==can hide type issues, and'529'is a magic constant. Introduce a named constant and use===.- const englishName = - transformedNames.find((name) => name.id == '529') ?? - (native != null && id === '529' + const englishName = + transformedNames.find((n) => n.id === ENGLISH_LANGUAGE_ID) ?? + (native != null && id === ENGLISH_LANGUAGE_ID ? { - id, + id, primary: true, value: native } : undefined)
7-8: Define a shared, documented English language ID constant.Adds clarity and prevents scattered
'529'usage.const fetcher = (url: string) => fetch(url).then((res) => res.json()) +// ISO 639-3 JESUS Film internal language id for English +export const ENGLISH_LANGUAGE_ID = '529'
46-52: Avoid shadowing the outeridwhen mapping names.Rename the tuple variable to reduce cognitive load.
- }[] = names.map((returnedName) => { - const [id, nameValue] = returnedName.split(':') + }[] = names.map((returnedName) => { + const [nameId, nameValue] = returnedName.split(':') return { - id, + id: nameId, primary: false, value: nameValue } })
93-93: Effect dependency should targeti18n.language, not thei18nobject.Depending on the object can cause unnecessary re-runs; the code only needs the language string.
- }, [data, i18n]) + }, [data, i18n.language])apps/watch/src/components/VideoContentPage/VideoHero/VideoPlayer/VideoControls/VideoControls.spec.tsx (3)
27-34: Mock fscreen to full API shape (add removeEventListener).VideoControls likely removes listeners on unmount. Missing removeEventListener can cause TypeErrors during cleanup.
jest.mock('fscreen', () => ({ __esModule: true, default: { requestFullscreen: jest.fn(), exitFullscreen: jest.fn(), - addEventListener: jest.fn() + addEventListener: jest.fn(), + removeEventListener: jest.fn() } }))
36-41: Normalize mock path to match import path exactly.The double slash can produce brittle mocks in Jest path resolution.
-jest.mock('../../../../..//libs/useLanguages', () => ({ +jest.mock('../../../../../libs/useLanguages', () => ({ useLanguages: jest.fn() }))
167-167: Target the clickable mute control more robustly.Indexing into getAllByTestId makes the test order-dependent. Prefer a role/name query scoped to the visible control, or within() on the controls container.
Example:
await userEvent.click( within(screen.getByTestId('volume-controls')).getByRole('button', { name: /mute/i }) )apps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.spec.tsx (3)
41-48: Align englishName.id with model (English = '529').Hook logic treats '529' as the English language id. Using '496' here is harmless for these tests but drifts from the data contract.
- englishName: { id: '496', value: 'French', primary: false }, + englishName: { id: '529', value: 'French', primary: false },
93-166: Helper text coverage looks good; consider adding a null-state assertion.Optional: add a case asserting correct copy when videoAudioLanguageIds = [] (zero available).
168-199: Use listbox role for MUI Autocomplete options.MUI renders role="listbox" for the options container; role="list" can be brittle across MUI versions.
- expect(screen.getAllByRole('list')[0].children[0]).toHaveTextContent('English') - expect(screen.getAllByRole('list')[0].children[1]).toHaveTextContent('FrenchFrançais') + expect(screen.getAllByRole('listbox')[0].children[0]).toHaveTextContent('English') + expect(screen.getAllByRole('listbox')[0].children[1]).toHaveTextContent('FrenchFrançais') … - expect(screen.getAllByRole('list')[1].children[0]).toHaveTextContent('SpanishEspañol') + expect(screen.getAllByRole('listbox')[1].children[0]).toHaveTextContent('SpanishEspañol')apps/watch/src/components/LanguageSwitchDialog/LanguageSwitchDialog.tsx (2)
18-25: Optional: default props defensively and document optionality.open and handleClose are optional; default open to false and handleClose to a no-op to simplify consumers and avoid conditional checks at call sites.
-export function LanguageSwitchDialog({ - open, - handleClose -}: LanguageSwitchDialogProps): ReactElement { +export function LanguageSwitchDialog({ + open = false, + handleClose = () => {} +}: LanguageSwitchDialogProps): ReactElement {
38-39: Minor: prefer Boolean(open) over open || false.Semantically equivalent, slightly clearer.
- open={open || false} + open={Boolean(open)}apps/watch/pages/watch/[part1]/[part2]/[part3].tsx (2)
53-64: Deduplicate GET_VIDEO_LANGUAGES into a shared module.The same query exists in multiple pages. Extract to a single source (e.g., apps/watch/src/gql/getVideoLanguages.ts) to avoid drift and enable reuse.
Proposed extraction (new file):
// apps/watch/src/gql/getVideoLanguages.ts import { graphql } from '@core/shared/gql' export const GET_VIDEO_LANGUAGES = graphql(` query GetVideoLanguages($id: ID!) { video(id: $id, idType: databaseId) { audioLanguages: variantLanguages { id } subtitleLanguages: subtitles { id: languageId } } } `)Then, replace this block with an import:
-export const GET_VIDEO_LANGUAGES = graphql(` - query GetVideoLanguages($id: ID!) { - video(id: $id, idType: databaseId) { - audioLanguages: variantLanguages { - id - } - subtitleLanguages: subtitles { - id: languageId - } - } - } -`) +// import { GET_VIDEO_LANGUAGES } from '../../../../src/gql/getVideoLanguages'
168-179: Query + mapping LGTM; minor type polish possible.The DB id usage with
idType: databaseIdand mapping to string arrays is correct. Optionally add generics toclient.queryfor stronger typing.-const { data } = await client.query({ +const { data } = await client.query<typeof GET_VIDEO_LANGUAGES>({ query: GET_VIDEO_LANGUAGES, variables: { id: contentData.content.id } })apps/watch/src/libs/watchContext/useLanguageActions/useLanguageActions.ts (1)
6-13: Promote param shapes to named types.Defining
type LanguageWithSlug = { id: string; slug: string }andtype LanguageId = { id: string }improves readability and reuse across the app.-interface UseLanguageActionsHook { - updateAudioLanguage: ( - language: { id: string; slug: string }, - reload?: boolean - ) => void - updateSubtitleLanguage: (language: { id: string }) => void +type LanguageWithSlug = { id: string; slug: string } +type LanguageId = { id: string } + +interface UseLanguageActionsHook { + updateAudioLanguage: (language: LanguageWithSlug, reload?: boolean) => void + updateSubtitleLanguage: (language: LanguageId) => voidapps/watch/pages/watch/[part1]/[part2].tsx (2)
38-50: Avoid duplicating GET_VIDEO_LANGUAGES; import the shared query.This query is identical to the one in the [part3] page. Centralize it to a shared module to prevent divergence.
After creating
apps/watch/src/gql/getVideoLanguages.ts(see suggestion in [part3] comment), replace this block with:-const GET_VIDEO_LANGUAGES = graphql(` - query GetVideoLanguages($id: ID!) { - video(id: $id, idType: databaseId) { - audioLanguages: variantLanguages { - id - } - subtitleLanguages: subtitles { - id: languageId - } - } - } -`) +// import { GET_VIDEO_LANGUAGES } from '../../../src/gql/getVideoLanguages'
153-164: Query + mapping LGTM; optional stronger typing.Same nit as in [part3]: add generics for stricter typing on the query result.
-const { data } = await client.query({ +const { data } = await client.query<typeof GET_VIDEO_LANGUAGES>({ query: GET_VIDEO_LANGUAGES, variables: { id: contentData.content.id } })apps/watch/src/components/LanguageSwitchDialog/SubtitlesSelect/SubtitlesSelect.tsx (1)
150-159: Use MUI Checkbox + ensure boolean valueFollow apps/watch MUI-only guideline and avoid uncontrolled-to-controlled warnings.
+import Checkbox from '@mui/material/Checkbox' +import FormControlLabel from '@mui/material/FormControlLabel' ... - <div className="flex ml-8 my-4 items-center gap-2"> - <input - id="no-subtitles" - type="checkbox" - checked={subtitleOn} - onChange={handleSubtitlesOnChange} - className="accent-[#CB333B] h-4 w-4 rounded border-gray-300 focus:ring-0" - /> - <label htmlFor="no-subtitles" className="text-sm text-gray-500"> - {t('Show subtitles')} - </label> - </div> + <div className="flex ml-8 my-4 items-center gap-2"> + <FormControlLabel + control={ + <Checkbox + checked={Boolean(subtitleOn)} + onChange={handleSubtitlesOnChange} + color="primary" + /> + } + label={t('Show subtitles')} + /> + </div>apps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.tsx (1)
75-92: Null-safe onChange and clarify reload semantics
- Guard for null value from Autocomplete.
- Confirm reload policy: current logic reloads when the selected language is available (and InstantSearch is absent), which may conflict with the PR goal of “apply instantly without client-side redirects.”
- function handleChange(_, language: Language): void { - let reload = instantSearch == null - if (reload) { - const found = videoAudioLanguageIds?.find((id) => id === language.id) - reload = found != null - } - updateAudioLanguage(language, reload) + function handleChange(_, language: Language | null): void { + if (language == null) return + let reload = instantSearch == null && videoAudioLanguageIds?.includes(language.id) === true + updateAudioLanguage(language, reload)Please confirm whether redirects should be entirely avoided (set reload = false), or only when the selected language isn’t available for the current video.
apps/watch/src/libs/watchContext/useLanguageActions/useLanguageActions.spec.tsx (4)
6-6: Remove unused importuseWatch isn’t referenced.
-import { WatchProvider, WatchState, useWatch } from '../WatchContext' +import { WatchProvider, WatchState } from '../WatchContext'
24-26: Reset router between tests to avoid state leakageInitialize mockRouter per test for determinism.
beforeEach(() => { jest.clearAllMocks() + mockRouter.setCurrentUrl('/') })
80-85: Don’t pass non-typed props into WatchProvider initialStaterouter isn’t part of WatchState; casting to WatchState hides a type smell and is unnecessary for these tests.
- <WatchProvider - initialState={ - { - ...defaultInitialState, - router: mockRouter - } as unknown as WatchState - } - > + <WatchProvider initialState={defaultInitialState as WatchState}> {children} </WatchProvider>
102-109: Same as above: remove extraneous router prop- <WatchProvider - initialState={ - { - ...defaultInitialState, - router: mockRouter - } as unknown as WatchState - } - > + <WatchProvider initialState={defaultInitialState as WatchState}> {children} </WatchProvider>apps/watch/src/libs/watchContext/WatchContext.spec.tsx (2)
131-137: Drop MockedProvider to speed up these tests.No GraphQL usage here—wrapping with MockedProvider adds needless complexity and runtime. Replace with plain WatchProvider.
Apply diffs:
- const { rerender } = render( - <MockedProvider mocks={[]} addTypename={false}> - <WatchProvider initialState={defaultInitialState}> - <TestWatchState /> - </WatchProvider> - </MockedProvider> - ) + const { rerender } = render( + <WatchProvider initialState={defaultInitialState}> + <TestWatchState /> + </WatchProvider> + )- rerender( - <MockedProvider mocks={[]} addTypename={false}> - <WatchProvider - initialState={{ - ...defaultInitialState, - videoAudioLanguageIds: ['529'], - videoSubtitleLanguageIds: ['529'] - }} - > - <TestWatchState /> - </WatchProvider> - </MockedProvider> - ) + rerender( + <WatchProvider + initialState={{ + ...defaultInitialState, + videoAudioLanguageIds: ['529'], + videoSubtitleLanguageIds: ['529'] + }} + > + <TestWatchState /> + </WatchProvider> + )- const { result } = renderHook(() => useWatch(), { - wrapper: ({ children }: { children: ReactNode }) => ( - <MockedProvider mocks={[]} addTypename={false}> - <WatchProvider - initialState={{ - audioLanguageId: '496', - subtitleLanguageId: '496' - }} - > - {children} - </WatchProvider> - </MockedProvider> - ) - }) + const { result } = renderHook(() => useWatch(), { + wrapper: ({ children }: { children: ReactNode }) => ( + <WatchProvider + initialState={{ + audioLanguageId: '496', + subtitleLanguageId: '496' + }} + > + {children} + </WatchProvider> + ) + })Also applies to: 147-159, 171-182
139-146: Stabilize assertions with test IDs instead of getByText.String-based matches are brittle. Prefer data-testid on TestWatchState fields.
Suggested change in apps/watch/src/libs/watchContext/TestWatchState.tsx (not in this diff):
- <div>audioLanguageId: {state.audioLanguageId}</div> + <div data-testid="audioLanguageId">{state.audioLanguageId}</div> - <div>subtitleLanguageId: {state.subtitleLanguageId}</div> + <div data-testid="subtitleLanguageId">{state.subtitleLanguageId}</div> - <div>subtitleOn: {state.subtitleOn?.toString()}</div> + <div data-testid="subtitleOn">{state.subtitleOn?.toString()}</div> - <div> - videoAudioLanguageIds: {state.videoAudioLanguageIds?.length || 0} - </div> + <div data-testid="videoAudioLanguageIds"> + {state.videoAudioLanguageIds?.length || 0} + </div> - <div> - videoSubtitleLanguageIds: {state.videoSubtitleLanguageIds?.length || 0} - </div> + <div data-testid="videoSubtitleLanguageIds"> + {state.videoSubtitleLanguageIds?.length || 0} + </div>Then update these tests to use screen.getByTestId(...).
Also applies to: 161-164
apps/watch/src/libs/watchContext/WatchContext.tsx (2)
87-92: Default context state: consider completing defaults and centralizing IDs.Add subtitleOn default (false) and consider centralizing '529' in a constant to avoid magic numbers.
Suggested diff:
state: { - audioLanguageId: '529', - subtitleLanguageId: '529' + audioLanguageId: '529', + subtitleLanguageId: '529', + subtitleOn: false },Optionally export a DEFAULT_LANGUAGE_ID constant from a shared config and reference it here.
105-129: Reducer: add a default branch for runtime safety.Type system ensures exhaustiveness, but a default: return state guards against unexpected actions at runtime.
Apply this diff:
switch (action.type) { case 'SetLanguagePreferences': return { ...state, audioLanguageId: action.audioLanguageId ?? state.audioLanguageId, subtitleLanguageId: action.subtitleLanguageId ?? state.subtitleLanguageId, subtitleOn: action.subtitleOn ?? state.subtitleOn } case 'SetVideoAudioLanguageIds': { return { ...state, videoAudioLanguageIds: action.videoAudioLanguageIds } } case 'SetVideoSubtitleLanguageIds': { return { ...state, videoSubtitleLanguageIds: action.videoSubtitleLanguageIds } } - } + default: + return state + }
apps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.tsx
Outdated
Show resolved
Hide resolved
apps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.tsx
Show resolved
Hide resolved
apps/watch/src/components/LanguageSwitchDialog/SubtitlesSelect/SubtitlesSelect.tsx
Outdated
Show resolved
Hide resolved
apps/watch/src/components/LanguageSwitchDialog/SubtitlesSelect/SubtitlesSelect.tsx
Show resolved
Hide resolved
apps/watch/src/libs/watchContext/useLanguageActions/useLanguageActions.ts
Show resolved
Hide resolved
apps/watch/src/libs/watchContext/useSubtitleUpdate/useSubtitleUpdate.tsx
Show resolved
Hide resolved
…Select components; remove unused getCurrentAudioLanguage utility and related tests
github-actions
bot
temporarily deployed
to
Preview - journeys-admin
github-actions
bot
requested a deployment
to
Preview - journeys-admin
…o-improve-audio-and-subtitle
github-actions
bot
requested a deployment
to
Preview - journeys-admin
github-actions
bot
temporarily deployed
to
Preview - journeys-admin
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/watch/src/components/NewVideoContentPage/NewVideoContentPage.tsx (1)
170-176: Remove tautological self-compare; fix gating to satisfy linter and preserve skeleton behavior.
children.length === children.lengthis always true and triggers Biome’s noSelfCompare error. Use a meaningful condition and keep skeleton rendering when loading.Apply:
- {((container?.childrenCount ?? 0) > 0 || childrenCount > 0) && - (children.length === children.length || children.length > 0) && ( + {((container?.childrenCount ?? 0) > 0 || childrenCount > 0 || loading) && ( <VideoCarousel - videos={children} + videos={children} containerSlug={container?.slug ?? videoSlug} activeVideoId={id} loading={loading} /> )}
🧹 Nitpick comments (3)
apps/watch/src/components/NewVideoContentPage/NewVideoContentHeader/NewVideoContentHeader.spec.tsx (1)
83-83: Confirm mixed totals are intentional ("of 20" vs "3/4").Header shows "Clip X of videos.length" while the compact counter shows "X/container.childrenCount". If this dual-total UX is intentional, consider adding helper text to avoid confusion; otherwise, align both counters to the same total.
apps/watch/src/libs/useVideoChildren/useVideoChildren.ts (1)
30-35: Consider improving fetch policy to reduce network churn
fetchPolicy: 'no-cache'forces a network hit every time. If caching issues stem from variant child normalization, consider an Apollo field policy (keyArgs/merge) and usecache-and-networkto keep UI snappy while still refreshing.apps/watch/src/libs/useVideoChildren/useVideoChildren.spec.tsx (1)
65-75: Type the mocks to strengthen test correctnessUse Apollo’s
MockedResponse[]instead ofany[]to catch shape errors at compile time.-import { MockedProvider } from '@apollo/client/testing' +import { MockedProvider } from '@apollo/client/testing' +import type { MockedResponse } from '@apollo/client/testing' @@ -}: { - children: ReactNode - mocks: any[] -}) => ( +}: { + children: ReactNode + mocks: ReadonlyArray<MockedResponse> +}) => (
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
apps/watch/src/components/NewVideoContentPage/NewVideoContentHeader/NewVideoContentHeader.spec.tsx(2 hunks)apps/watch/src/components/NewVideoContentPage/NewVideoContentHeader/NewVideoContentHeader.tsx(2 hunks)apps/watch/src/components/NewVideoContentPage/NewVideoContentPage.tsx(1 hunks)apps/watch/src/components/VideoContainerPage/VideoContainerPage.tsx(1 hunks)apps/watch/src/components/VideoContentPage/VideoContentPage.tsx(1 hunks)apps/watch/src/libs/useVideoChildren/useVideoChildren.spec.tsx(1 hunks)apps/watch/src/libs/useVideoChildren/useVideoChildren.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/watch/src/components/NewVideoContentPage/NewVideoContentHeader/NewVideoContentHeader.tsx
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/base.mdc)
**/*.{ts,tsx,js,jsx}: Use early returns whenever possible to make the code more readable.
Use descriptive variable and function/const names.
Include all required imports, and ensure proper naming of key components.
Files:
apps/watch/src/libs/useVideoChildren/useVideoChildren.spec.tsxapps/watch/src/libs/useVideoChildren/useVideoChildren.tsapps/watch/src/components/VideoContainerPage/VideoContainerPage.tsxapps/watch/src/components/NewVideoContentPage/NewVideoContentHeader/NewVideoContentHeader.spec.tsxapps/watch/src/components/VideoContentPage/VideoContentPage.tsxapps/watch/src/components/NewVideoContentPage/NewVideoContentPage.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/base.mdc)
Define a type if possible.
Files:
apps/watch/src/libs/useVideoChildren/useVideoChildren.spec.tsxapps/watch/src/libs/useVideoChildren/useVideoChildren.tsapps/watch/src/components/VideoContainerPage/VideoContainerPage.tsxapps/watch/src/components/NewVideoContentPage/NewVideoContentHeader/NewVideoContentHeader.spec.tsxapps/watch/src/components/VideoContentPage/VideoContentPage.tsxapps/watch/src/components/NewVideoContentPage/NewVideoContentPage.tsx
apps/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/apps.mdc)
apps/**/*.{js,jsx,ts,tsx}: Always use MUI over HTML elements; avoid using CSS or tags.
Use descriptive variable and function/const names. Also, event functions should be named with a “handle” prefix, like “handleClick” for onClick and “handleKeyDown” for onKeyDown.
Implement accessibility features on elements. For example, a tag should have a tabindex=“0”, aria-label, on:click, and on:keydown, and similar attributes.
Files:
apps/watch/src/libs/useVideoChildren/useVideoChildren.spec.tsxapps/watch/src/libs/useVideoChildren/useVideoChildren.tsapps/watch/src/components/VideoContainerPage/VideoContainerPage.tsxapps/watch/src/components/NewVideoContentPage/NewVideoContentHeader/NewVideoContentHeader.spec.tsxapps/watch/src/components/VideoContentPage/VideoContentPage.tsxapps/watch/src/components/NewVideoContentPage/NewVideoContentPage.tsx
🧠 Learnings (1)
📚 Learning: 2025-07-27T21:59:10.507Z
Learnt from: CR
PR: JesusFilm/core#0
File: .cursor/rules/watch-modern.mdc:0-0
Timestamp: 2025-07-27T21:59:10.507Z
Learning: Applies to apps/watch-modern/**/*.test.{tsx,jsx} : Use 'testing-library/react' npm package for frontend tests.
Applied to files:
apps/watch/src/libs/useVideoChildren/useVideoChildren.spec.tsx
🧬 Code graph analysis (3)
apps/watch/src/libs/useVideoChildren/useVideoChildren.spec.tsx (2)
apps/watch/src/components/Videos/__generated__/testData.ts (1)
videos(10-1815)apps/watch/src/libs/useVideoChildren/useVideoChildren.ts (2)
GET_VIDEO_CHILDREN(11-21)useVideoChildren(23-47)
apps/watch/src/libs/useVideoChildren/useVideoChildren.ts (1)
apps/watch/__generated__/GetVideoChildren.ts (1)
GetVideoChildren(68-70)
apps/watch/src/components/NewVideoContentPage/NewVideoContentPage.tsx (2)
apps/watch/src/components/NewVideoContentPage/NewVideoContentHeader/NewVideoContentHeader.tsx (1)
NewVideoContentHeader(16-108)apps/watch/src/components/NewVideoContentPage/VideoCarousel/VideoCarousel.tsx (1)
VideoCarousel(20-104)
🪛 Biome (2.1.2)
apps/watch/src/components/VideoContentPage/VideoContentPage.tsx
[error] 116-116: Comparing to itself is potentially pointless.
(lint/suspicious/noSelfCompare)
apps/watch/src/components/NewVideoContentPage/NewVideoContentPage.tsx
[error] 170-170: Comparing to itself is potentially pointless.
(lint/suspicious/noSelfCompare)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Deploy Preview (watch, 7582/merge, pull_request, 22)
- GitHub Check: Deploy Preview (watch-modern, 7582/merge, pull_request, 22)
- GitHub Check: Deploy Preview (videos-admin, 7582/merge, pull_request, 22)
- GitHub Check: Deploy Preview (journeys-admin, 7582/merge, pull_request, 22)
- GitHub Check: Deploy Preview (journeys, 7582/merge, pull_request, 22)
- GitHub Check: test (22, 1/3)
- GitHub Check: test (22, 3/3)
🔇 Additional comments (12)
apps/watch/src/components/NewVideoContentPage/NewVideoContentHeader/NewVideoContentHeader.spec.tsx (1)
65-65: Assertion update matches new header logic (videos.length).The expectation now aligns with NewVideoContentHeader using videos.length when provided.
apps/watch/src/components/NewVideoContentPage/NewVideoContentPage.tsx (2)
168-168: Passing loading and children into header is correct.This matches the component’s new API and avoids reliance on legacy filters.
172-173: VideoCard tolerates missing variant
All references tovideo.variantuse optional chaining (e.g.,video.variant?.slug); no unguarded or forced assertions remain.apps/watch/src/components/VideoContainerPage/VideoContainerPage.tsx (1)
55-59: LGTM: passing hook-filtered children directly to VideoGridThis aligns with the hook’s centralized filtering (variant != null) and removes duplicated filtering here.
apps/watch/src/components/VideoContentPage/VideoContentPage.tsx (2)
108-114: LGTM: VideoHeading now receives the unified children listConsistent with moving filtering into the hook.
119-124: LGTM: VideoCarousel receives hook-filtered childrenMatches the updated data flow; no concerns.
apps/watch/src/libs/useVideoChildren/useVideoChildren.ts (2)
37-41: Good: centralized filtering for playable childrenFiltering to
variant != nullhere de-duplicates UI concerns and keeps consumers simple.
11-21: No changes needed:$languageIdis used inVIDEO_CHILD_FIELDS. Verified that the fragment consumes$languageIdin thetitle,imageAlt,snippet, andvariantfields, so the variable should remain.apps/watch/src/libs/useVideoChildren/useVideoChildren.spec.tsx (4)
82-101: LGTM: initial loading state coverage is solidAsserts both loading=true and empty children; good baseline.
128-152: LGTM: verifies filtering of null variantsConfirms hook behavior against mixed inputs.
226-235: LGTM: skip behavior when slug is undefinedCorrectly expects no query and an idle state.
261-281: LGTM: graceful error handling testEnsures empty list on network error—consistent and predictable.
| {((container?.childrenCount ?? 0) > 0 || childrenCount > 0) && | ||
| (realChildren.length === children.length || | ||
| realChildren.length > 0) && ( | ||
| (children.length === children.length || | ||
| children.length > 0) && ( | ||
| <Box pb={4}> |
There was a problem hiding this comment.
Fix tautology that always renders the carousel when counts > 0
children.length === children.length is always true, defeating the intent to gate on actual children. Replace with a simple length check.
- {((container?.childrenCount ?? 0) > 0 || childrenCount > 0) &&
- (children.length === children.length ||
- children.length > 0) && (
+ {((container?.childrenCount ?? 0) > 0 || childrenCount > 0) &&
+ children.length > 0 && (📝 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.
| {((container?.childrenCount ?? 0) > 0 || childrenCount > 0) && | |
| (realChildren.length === children.length || | |
| realChildren.length > 0) && ( | |
| (children.length === children.length || | |
| children.length > 0) && ( | |
| <Box pb={4}> | |
| {((container?.childrenCount ?? 0) > 0 || childrenCount > 0) && | |
| children.length > 0 && ( | |
| <Box pb={4}> |
🧰 Tools
🪛 Biome (2.1.2)
[error] 116-116: Comparing to itself is potentially pointless.
(lint/suspicious/noSelfCompare)
🤖 Prompt for AI Agents
In apps/watch/src/components/VideoContentPage/VideoContentPage.tsx around lines
115-118, the condition contains a tautology `children.length ===
children.length` which is always true and causes the carousel to render
incorrectly; replace the tautology with a real check such as `children.length >
0` (or simply use the existing children length check) so the combined
conditional only passes when there are actual child elements, keeping the outer
container?.childrenCount check intact.
tataihono
deleted the
tataihono/wat-153-refactor-language-system-to-improve-audio-and-subtitle
branch
2 participants
Overview
This PR implements a comprehensive refactoring of the language handling system to improve performance, user experience, and code maintainability.
Key Changes
1. Redis Caching for Languages
2. Improved Audio and Subtitle Language Handling
3. Simplified State Management
LanguageProvidercontext in favor of a more streamlined approachuseLanguageshookWatchContextby removing unnecessary complexity around language managementWatchInitialStatewith optionalWatchStateproperties4. Enhanced User Experience
5. Code Quality Improvements
useLanguagesSlugQueryand related GraphQL queries6. Performance Optimizations
Technical Details
useLanguageshookTesting
useLanguageshookBreaking Changes
LanguageProvidercontext - components now useuseLanguageshook directlyWatchContextstate structureThis refactoring significantly improves the performance and user experience of language selection while making the codebase more maintainable and easier to understand.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Localization