Conversation
…ove middleware handling - Added Upstash Redis integration to cache audio language variants for videos. - Enhanced middleware to handle audio language redirects based on user preferences. - Updated package dependencies for Redis and Zod. - Removed unused components and queries related to audio language selection. - Improved error handling for Redis cache and API fetch scenarios.
…r-middleware-functionality
- Updated Redis cache implementation to use `set` with expiration options. - Refactored middleware tests to align with new Redis methods. - Improved error handling and response headers in API for variant languages. - Added validation for missing language keys in static props.
…e-functionality' of https://github.com/JesusFilm/core into tataihono/wat-151-add-comprehensive-tests-for-middleware-functionality
- Refactored redisClient function to reuse Redis instance for improved performance. - Added condition to create a new Redis client only in test environment or if not already initialized.
- Added `setHeader` mock function to the API response object in variantLanguages tests for improved test coverage and accuracy.
…tPage.spec.tsx - Eliminated the test case that checked for redirection based on cookie language, as it was deemed unnecessary for current test coverage.
- Changed Redis URL and token environment variables to use KV REST API instead of Redis URL and token. - Ensured compatibility with the new configuration for improved functionality.
- Changed the structure of variantLanguages from an array to an object for improved data handling. - Updated middleware and API response to accommodate the new object format. - Enhanced validation and error handling for missing language IDs and slugs in the API. - Adjusted tests to reflect the new variantLanguages structure and ensure accurate response handling.
…rection - Replaced Redis `get` and `set` methods with `exists`, `hset`, and `hget` for improved data management. - Enhanced middleware logic to handle Redis cache checks and API fetches more efficiently. - Updated tests to reflect changes in Redis interactions and ensure accurate behavior for audio language redirection. - Improved error handling for Redis operations and validation of cached data.
…dling - Introduced AUDIO_LANGUAGE_REDIRECT_CACHE_SCHEMA_VERSION for better cache key management in middleware. - Updated Redis cache key structure to include schema version for audio language redirection. - Simplified language handling by removing unnecessary prefixes in variantLanguages. - Enhanced error logging for Redis operations to improve debugging and maintainability. - Adjusted tests to reflect changes in middleware logic and Redis interactions.
…e redirection - Consolidated matcher configuration by merging multiple objects into a single array for cleaner code. - Maintained existing functionality while improving readability and maintainability of the middleware logic.
…meter - Added logic in middleware to skip audio language redirection when the r=0 query parameter is present. - Updated tests to verify that redirection is correctly bypassed under this condition. - Modified VideoCard component to include r=0 in generated links for consistent behavior across the application.
- Introduced a new TUI configuration option in nx.json to manage feature toggling. - Updated the AudioTrackSelect component to improve helper text handling and language selection logic. - Removed unused audio language setter utility and its associated tests to streamline the codebase.
…r-middleware-functionality
WalkthroughRefactors watch app language handling: audio languages flattened to {id, slug}, subtitles represented as languageId string arrays, GET_LANGUAGES_SLUG replaced by GET_VARIANT_LANGUAGES_ID_AND_SLUG, subtitleUpdate replaced by a new useSubtitleUpdate hook, and corresponding context, pages, components, and tests updated to the new shapes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Component as UI Component (Player/Hero)
participant Hook as useSubtitleUpdate
participant Apollo as Apollo Client
participant GQL as GraphQL API
participant Player as Video.js Player
User->>Component: Change subtitle settings
Component->>Hook: subtitleUpdate({ player, subtitleLanguage, subtitleOn, autoSubtitle })
Hook->>Player: Collect textTracks()
alt subtitle off or no language
Hook->>Player: Disable all subtitle tracks
else Fetch and show
Hook->>Apollo: useLazyQuery(GET_SUBTITLES, { id: variant.slug })
Apollo->>GQL: GET_SUBTITLES
GQL-->>Apollo: subtitles data
Apollo-->>Hook: data
Hook->>Player: addRemoteTextTrack(..., default=true)
Hook->>Player: Toggle tracks (show selected, disable others)
end
sequenceDiagram
autonumber
participant Next as Next.js getStaticProps
participant Apollo as Apollo Client
participant GQL as GraphQL API
participant Page as Page component
Next->>Apollo: GET_VARIANT_LANGUAGES_ID_AND_SLUG({ id })
Apollo->>GQL: Query
GQL-->>Apollo: { variantLanguages: [{id, slug}], subtitles: [{languageId}] }
Apollo-->>Next: Data
Next-->>Next: Map -> audio: [{id, slug}], subtitles: [languageId]
Next-->>Page: props { videoAudioLanguagesIdsAndSlugs, videoSubtitleLanguageIds }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 9f38696
☁️ Nx Cloud last updated this comment at |
|
View your CI Pipeline Execution ↗ for commit 207dafb
☁️ Nx Cloud last updated this comment at |
1 similar comment
|
View your CI Pipeline Execution ↗ for commit 207dafb
☁️ Nx Cloud last updated this comment at |
|
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
apps/watch/src/components/LanguageSwitchDialog/utils/audioLanguageSetter/audioLanguageSetter.ts (2)
126-137: Preserve query/hash and normalize slug when updating URL.Current replacement can drop ?query/#hash and mis-compare when “.html” or query is present.
-function updateURLIfNeeded({ router, targetSlug }: UpdateURLParams): void { - if (!targetSlug) return - - const path = router.asPath.split('/') - const currentPathSlug = last(path)?.replace('.html', '') - - if (currentPathSlug !== targetSlug) { - void router.push( - router.asPath.replace(last(router.asPath.split('/')) ?? '', targetSlug) - ) - } -} +function updateURLIfNeeded({ router, targetSlug }: UpdateURLParams): void { + if (!targetSlug) return + const asPath = router.asPath + const qIdx = asPath.indexOf('?') + const hIdx = asPath.indexOf('#') + const cut = qIdx === -1 ? (hIdx === -1 ? asPath.length : hIdx) : (hIdx === -1 ? qIdx : Math.min(qIdx, hIdx)) + const pathname = asPath.slice(0, cut) + const suffix = asPath.slice(cut) // keeps ? and/or # + + const segments = pathname.split('/') + const currentSegment = segments[segments.length - 1] + const currentPathSlug = currentSegment?.replace('.html', '') + + if (currentPathSlug !== targetSlug) { + segments[segments.length - 1] = targetSlug + void router.push(segments.join('/') + suffix) + } +}
148-165: Simplify helper text logic and handle null slug.Avoid double set and show a sensible fallback when slug is null.
-function setHelper({ - isPreferred, - preferredLanguage, - setHelperText, - t -}: SetLanguageAndHelperParams): void { - setHelperText(t('2000 translations')) - - if (isPreferred) { - setHelperText(t('2000 translations')) - } else if (preferredLanguage) { - setHelperText( - t('Not available in {{value}}', { - value: preferredLanguage.slug - }) - ) - } -} +function setHelper({ + isPreferred, + preferredLanguage, + setHelperText, + t +}: SetLanguageAndHelperParams): void { + if (isPreferred) { + setHelperText(t('2000 translations')) + return + } + if (preferredLanguage) { + const value = preferredLanguage.slug ?? preferredLanguage.name + setHelperText(t('Not available in {{value}}', { value })) + return + } + setHelperText(t('2000 translations')) +}apps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.tsx (1)
1-1: Remove unused GraphQL imports.Leftovers from the old fetch path cause lint noise.
-import { useLazyQuery } from '@apollo/client' @@ -import { GetLanguagesSlug } from '../../../../__generated__/GetLanguagesSlug' @@ -import { GET_LANGUAGES_SLUG } from '../../../libs/useLanguagesSlugQuery'Also applies to: 14-16
apps/watch/src/components/LanguageSwitchDialog/utils/getCurrentAudioLanguage/getCurrentAudioLanguage.ts (1)
43-56: Harden path parsing to handle query/hash/trailing slash and only strip a terminal .html
routerAsPathcan include query strings or hashes and sometimes ends with a trailing slash, which will break slug detection. Also,replace('.html', '')may strip mid-path occurrences. Tighten this logic.Apply:
- const currentLanguageSlug = routerAsPath - .split('/') - .pop() - ?.replace('.html', '') + const cleanPath = routerAsPath.split('?')[0].split('#')[0] + const currentLanguageSlug = cleanPath + .split('/') + .filter(Boolean) + .pop() + ?.replace(/\.html$/i, '') + // if encoded, decode it for a fair comparison + const decodedSlug = currentLanguageSlug != null + ? decodeURIComponent(currentLanguageSlug) + : undefinedand update the finder:
- const selectedLanguage = allLanguages.find( - (lang) => lang.slug === currentLanguageSlug - ) + const selectedLanguage = allLanguages.find( + (lang) => lang.slug === decodedSlug + )apps/watch/src/libs/watchContext/WatchContext.tsx (1)
240-258: Fix crash in reducer when videoAudioLanguagesIdsAndSlugs is undefined (unit test failure)Static tests show “TypeError: videoAudioLanguagesIdsAndSlugs is not iterable”. Default to an empty array before iterating.
Apply:
- case 'SetVideoAudioLanguages': { - const videoAudioLanguagesIdsAndSlugs = - action.videoAudioLanguagesIdsAndSlugs + case 'SetVideoAudioLanguages': { + const videoAudioLanguagesIdsAndSlugs = + action.videoAudioLanguagesIdsAndSlugs ?? []Optionally, pull
siteLanguageSlugsout of the loop:- for (const lang of videoAudioLanguagesIdsAndSlugs) { - const siteLanguageMapping = LANGUAGE_MAPPINGS[state.siteLanguage] - const siteLanguageSlugs = siteLanguageMapping?.languageSlugs || [] + const siteLanguageSlugs = + LANGUAGE_MAPPINGS[state.siteLanguage]?.languageSlugs ?? [] + for (const lang of videoAudioLanguagesIdsAndSlugs) {
🧹 Nitpick comments (11)
apps/watch/src/components/NewVideoContentPage/NewVideoContentPage.tsx (1)
231-238: Prefer MUI Button and add accessible labelling per apps/ guidelines.*Replace raw with MUI’s Button, add aria-label.
- <button - onClick={() => setShowShare(true)} - className="inline-flex items-center gap-2 px-4 py-2 rounded-full text-gray-900 font-bold uppercase tracking-wider bg-white hover:bg-[#cb333b] hover:text-white transition-colors duration-200 text-sm cursor-pointer" - > - <LinkExternal className="w-4 h-4" /> - {t('Share')} - </button> + <Button + aria-label={t('Share this video')} + color="inherit" + size="small" + variant="contained" + onClick={() => setShowShare(true)} + startIcon={<LinkExternal className="w-4 h-4" />} + > + {t('Share')} + </Button>Add at top:
import { Button } from '@mui/material'apps/watch/src/components/LanguageSwitchDialog/utils/audioLanguageSetter/audioLanguageSetter.ts (1)
200-205: Align selection with actual availability for the current video.
selectLanguageForVideodoesn’t know which variant languages are available; yet the caller gates onvideoAudioLanguagesIdsAndSlugs. Pass that list in and verifycurrentAudioLanguage.idis available before using it.export interface AudioLanguageSetterParams { /** Current audio language selected by the system based on availability */ currentAudioLanguage?: VariantLanguageIdAndSlug + /** Languages available for the current video (id+slug only) */ + availableVariantLanguages?: VariantLanguageIdAndSlug[] @@ export function selectLanguageForVideo({ currentAudioLanguage, + availableVariantLanguages, allLanguages, audioLanguage, router, setHelperText, t }: AudioLanguageSetterParams): void { @@ - if ( - currentAudioLanguage != null && - preferredAudioLanguage != null && - currentAudioLanguage.id === preferredAudioLanguage.id - ) { + const isCurrentAvailable = + currentAudioLanguage != null && + availableVariantLanguages?.some((v) => v.id === currentAudioLanguage.id) + + if ( + isCurrentAvailable && + preferredAudioLanguage != null && + currentAudioLanguage!.id === preferredAudioLanguage.id + ) { @@ - else if (currentAudioLanguage != null) { + else if (isCurrentAvailable && currentAudioLanguage != null) {Update the caller (AudioTrackSelect) to pass
availableVariantLanguages={videoAudioLanguagesIdsAndSlugs}.I can push a follow-up commit wiring this through if you want.
Also applies to: 206-218, 220-233
apps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.tsx (1)
56-73: Tighten effect gating and fix duplicate dependency.
- Gate in one place; no need to check
loadingtwice.- Remove duplicated
loadingin the deps array.useEffect(() => { // Run automatic selection logic based on current state - if (allLanguages == null || loading) return + if (allLanguages == null || loading) return const params = { currentAudioLanguage, + // pass when you adopt the optional refactor in audioLanguageSetter.ts + // availableVariantLanguages: videoAudioLanguagesIdsAndSlugs, allLanguages, audioLanguage, router, setHelperText, t } - if (videoId != null) { - if (videoAudioLanguagesIdsAndSlugs == null || loading) return - selectLanguageForVideo(params) - } + if (videoId != null && videoAudioLanguagesIdsAndSlugs != null) { + selectLanguageForVideo(params) + } }, [ loading, allLanguages, audioLanguage, currentAudioLanguage, t, router, videoId, - videoAudioLanguagesIdsAndSlugs, - loading + videoAudioLanguagesIdsAndSlugs ])Also applies to: 81-83
apps/watch/src/libs/watchContext/initializeVideoLanguages/initializeVideoLanguages.ts (1)
19-26: Minor: default parameter can simplify nil coalescing.-export function initializeVideoLanguages( - dispatch: Dispatch<WatchAction>, - videoAudioLanguagesIdsAndSlugs: AudioLanguageData[], - videoSubtitleLanguages: SubtitleLanguage[] -): void { +export function initializeVideoLanguages( + dispatch: Dispatch<WatchAction>, + videoAudioLanguagesIdsAndSlugs: AudioLanguageData[] = [], + videoSubtitleLanguages: SubtitleLanguage[] = [] +): void { @@ dispatch({ type: 'SetVideoAudioLanguages', - videoAudioLanguagesIdsAndSlugs: videoAudioLanguagesIdsAndSlugs ?? [] + videoAudioLanguagesIdsAndSlugs }) @@ dispatch({ type: 'SetVideoSubtitleLanguages', - videoSubtitleLanguages: videoSubtitleLanguages ?? [] + videoSubtitleLanguages })apps/watch/src/libs/watchContext/WatchContext.tsx (2)
270-289: Clarify autoSubtitle semantics to a strict booleanUsing
undefinedfor “off” can complicate consumers. Prefer an explicit boolean.Apply:
- return { - ...state, - videoSubtitleLanguages, - autoSubtitle: subtitleAvailable === false ? undefined : true - } + return { + ...state, + videoSubtitleLanguages, + autoSubtitle: subtitleAvailable === true + }If
undefinedis relied upon elsewhere, feel free to skip; otherwise this simplifies downstream checks.
387-397: Effect dependencies: consider stabilizing on the object or memoized valuesAccessing nested props directly in deps arrays can be brittle if
initialStateidentity changes. If feasible, depend oninitialStateor memoize the two arrays before passing toinitializeVideoLanguages.Example:
- }, [ - initialState.videoSubtitleLanguages, - initialState.videoAudioLanguagesIdsAndSlugs - ]) + }, [initialState])Only if this doesn’t fight your ESLint rules; otherwise keep as-is.
apps/watch/pages/watch/[part1]/[part2].tsx (3)
56-65: Avoid defining a query inside a page that other pages import from
[part3].tsximportsGET_VARIANT_LANGUAGES_ID_AND_SLUGfrom this page. Importing from a page couples build chunks and can create surprising bundles. Move the query to a shared module (e.g.,src/libs/queries/getVariantLanguagesIdAndSlug.ts) and import it from both pages.Proposed new shared file:
// src/libs/queries/getVariantLanguagesIdAndSlug.ts import { gql } from '@apollo/client' export const GET_VARIANT_LANGUAGES_ID_AND_SLUG = gql` query GetVariantLanguagesIdAndSlug($id: ID!) { video(id: $id, idType: databaseId) { variantLanguages { id slug } } } `Then update both pages to import from that path.
9-13: Remove stale imports for GetLanguagesSlug/GET_LANGUAGES_SLUGThese appear unused after the refactor and can be dropped to reduce bundle size and noise.
Apply:
-import { - GetLanguagesSlug, - GetLanguagesSlugVariables, - GetLanguagesSlug_video_variantLanguagesWithSlug as VideoAudioLanguage -} from '../../../__generated__/GetLanguagesSlug' -... -import { GET_LANGUAGES_SLUG } from '../../../src/libs/useLanguagesSlugQuery'Also applies to: 25-26
175-193: Prefer nullish coalescing over logical OR when defaulting arraysUse
?? []so empty arrays don’t get overridden (unlikely with.map, but keeps consistency with the other page).- videoAudioLanguagesData = - data?.video?.variantLanguages?.map( + videoAudioLanguagesData = + data?.video?.variantLanguages?.map( ({ id, slug }): AudioLanguageData => ({ id, slug }) - ) || [] + ) ?? []apps/watch/pages/watch/[part1]/[part2]/[part3].tsx (2)
44-53: Don’t import queries from sibling page filesMirror the suggestion in
[part2].tsx: importGET_VARIANT_LANGUAGES_ID_AND_SLUGfrom a shared module, not../[part2].Apply:
-import { GET_VARIANT_LANGUAGES_ID_AND_SLUG } from '../[part2]' +import { GET_VARIANT_LANGUAGES_ID_AND_SLUG } from '../../../../src/libs/queries/getVariantLanguagesIdAndSlug'
11-15: Remove stale and unused imports
GetLanguagesSlug/GET_LANGUAGES_SLUGare unused now; theVariantLanguageIdAndSlugalias also appears unused. Clean them up.Apply:
-import { - GetLanguagesSlug, - GetLanguagesSlugVariables, - GetLanguagesSlug_video_variantLanguagesWithSlug as VideoAudioLanguage -} from '../../../../__generated__/GetLanguagesSlug' -... -import { GET_LANGUAGES_SLUG } from '../../../../src/libs/useLanguagesSlugQuery' -... -import { - GetVariantLanguagesIdAndSlug, - GetVariantLanguagesIdAndSlug_video_variantLanguages as VariantLanguageIdAndSlug, - GetVariantLanguagesIdAndSlugVariables -} from '../../../../__generated__/GetVariantLanguagesIdAndSlug' +import { + GetVariantLanguagesIdAndSlug, + GetVariantLanguagesIdAndSlugVariables +} from '../../../../__generated__/GetVariantLanguagesIdAndSlug'Also applies to: 31-31, 50-52
📜 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 ignored due to path filters (1)
apps/watch/__generated__/GetVariantLanguagesIdAndSlug.tsis excluded by!**/__generated__/**
📒 Files selected for processing (9)
apps/watch/pages/watch/[part1]/[part2].tsx(6 hunks)apps/watch/pages/watch/[part1]/[part2]/[part3].tsx(5 hunks)apps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.tsx(4 hunks)apps/watch/src/components/LanguageSwitchDialog/utils/audioLanguageSetter/audioLanguageSetter.ts(2 hunks)apps/watch/src/components/LanguageSwitchDialog/utils/getCurrentAudioLanguage/getCurrentAudioLanguage.ts(2 hunks)apps/watch/src/components/NewVideoContentPage/NewVideoContentPage.tsx(1 hunks)apps/watch/src/libs/watchContext/TestWatchState.tsx(1 hunks)apps/watch/src/libs/watchContext/WatchContext.tsx(7 hunks)apps/watch/src/libs/watchContext/initializeVideoLanguages/initializeVideoLanguages.ts(2 hunks)
🧰 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/watchContext/TestWatchState.tsxapps/watch/src/components/NewVideoContentPage/NewVideoContentPage.tsxapps/watch/src/components/LanguageSwitchDialog/utils/audioLanguageSetter/audioLanguageSetter.tsapps/watch/src/libs/watchContext/initializeVideoLanguages/initializeVideoLanguages.tsapps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.tsxapps/watch/src/libs/watchContext/WatchContext.tsxapps/watch/pages/watch/[part1]/[part2]/[part3].tsxapps/watch/pages/watch/[part1]/[part2].tsxapps/watch/src/components/LanguageSwitchDialog/utils/getCurrentAudioLanguage/getCurrentAudioLanguage.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/base.mdc)
Define a type if possible.
Files:
apps/watch/src/libs/watchContext/TestWatchState.tsxapps/watch/src/components/NewVideoContentPage/NewVideoContentPage.tsxapps/watch/src/components/LanguageSwitchDialog/utils/audioLanguageSetter/audioLanguageSetter.tsapps/watch/src/libs/watchContext/initializeVideoLanguages/initializeVideoLanguages.tsapps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.tsxapps/watch/src/libs/watchContext/WatchContext.tsxapps/watch/pages/watch/[part1]/[part2]/[part3].tsxapps/watch/pages/watch/[part1]/[part2].tsxapps/watch/src/components/LanguageSwitchDialog/utils/getCurrentAudioLanguage/getCurrentAudioLanguage.ts
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/watchContext/TestWatchState.tsxapps/watch/src/components/NewVideoContentPage/NewVideoContentPage.tsxapps/watch/src/components/LanguageSwitchDialog/utils/audioLanguageSetter/audioLanguageSetter.tsapps/watch/src/libs/watchContext/initializeVideoLanguages/initializeVideoLanguages.tsapps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.tsxapps/watch/src/libs/watchContext/WatchContext.tsxapps/watch/pages/watch/[part1]/[part2]/[part3].tsxapps/watch/pages/watch/[part1]/[part2].tsxapps/watch/src/components/LanguageSwitchDialog/utils/getCurrentAudioLanguage/getCurrentAudioLanguage.ts
🧬 Code graph analysis (5)
apps/watch/src/libs/watchContext/initializeVideoLanguages/initializeVideoLanguages.ts (1)
apps/watch/src/libs/watchContext/WatchContext.tsx (2)
WatchAction(170-182)AudioLanguageData(18-21)
apps/watch/src/libs/watchContext/WatchContext.tsx (1)
apps/watch/src/libs/localeMapping/localeMapping.ts (1)
LANGUAGE_MAPPINGS(22-204)
apps/watch/pages/watch/[part1]/[part2]/[part3].tsx (3)
apps/watch/src/libs/watchContext/WatchContext.tsx (1)
AudioLanguageData(18-21)apps/watch/__generated__/GetVariantLanguagesIdAndSlug.ts (2)
GetVariantLanguagesIdAndSlug(21-23)GetVariantLanguagesIdAndSlugVariables(25-27)apps/watch/pages/watch/[part1]/[part2].tsx (1)
GET_VARIANT_LANGUAGES_ID_AND_SLUG(56-65)
apps/watch/pages/watch/[part1]/[part2].tsx (3)
apps/watch/__generated__/VideoContentFields.ts (1)
VideoContentFields(97-118)apps/watch/src/libs/watchContext/WatchContext.tsx (1)
AudioLanguageData(18-21)apps/watch/__generated__/GetVariantLanguagesIdAndSlug.ts (2)
GetVariantLanguagesIdAndSlug(21-23)GetVariantLanguagesIdAndSlugVariables(25-27)
apps/watch/src/components/LanguageSwitchDialog/utils/getCurrentAudioLanguage/getCurrentAudioLanguage.ts (2)
apps/watch/src/libs/watchContext/WatchContext.tsx (1)
AudioLanguageData(18-21)libs/shared/ui/src/components/LanguageAutocomplete/LanguageAutocomplete.tsx (1)
LanguageOption(33-38)
🪛 GitHub Check: test (22, 1/3)
apps/watch/src/libs/watchContext/WatchContext.tsx
[failure] 247-247: WatchContext › reducer › SetVideoAudioLanguages › should set currentAudioLanguage to undefined when no match found
TypeError: videoAudioLanguagesIdsAndSlugs is not iterable
at videoAudioLanguagesIdsAndSlugs (src/libs/watchContext/WatchContext.tsx:247:26)
at Object.<anonymous> (src/libs/watchContext/WatchContext.spec.tsx:148:31)
[failure] 247-247: WatchContext › reducer › SetVideoAudioLanguages › should set video audio languages and select matching currentAudioLanguage
TypeError: videoAudioLanguagesIdsAndSlugs is not iterable
at videoAudioLanguagesIdsAndSlugs (src/libs/watchContext/WatchContext.tsx:247:26)
at Object.<anonymous> (src/libs/watchContext/WatchContext.spec.tsx:115:31)
⏰ 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). (5)
- GitHub Check: Deploy Preview (watch, 7548/merge, pull_request, 22)
- GitHub Check: test (22, 2/3)
- GitHub Check: build (22)
- GitHub Check: lint (22)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (6)
apps/watch/src/libs/watchContext/TestWatchState.tsx (2)
22-24: LGTM: migrated to videoAudioLanguagesIdsAndSlugs.Display aligns with new AudioLanguageData shape.
30-31: LGTM: currentAudioLanguage now references id directly.Matches flattened state.
apps/watch/src/libs/watchContext/initializeVideoLanguages/initializeVideoLanguages.ts (1)
3-5: LGTM: aligns with new AudioLanguageData shape and action payload.No functional concerns.
Also applies to: 17-33
apps/watch/src/components/LanguageSwitchDialog/utils/getCurrentAudioLanguage/getCurrentAudioLanguage.ts (1)
24-35: Good migration to flat AudioLanguageData; matching by id is correctThe switch to
AudioLanguageDataand id-based match is consistent with the new context shape and avoids nested lookups. LGTM.apps/watch/pages/watch/[part1]/[part2].tsx (1)
37-45: Aligning types looks good; ensure alias imports aren’t left unusedThe
AudioLanguageDataadoption and prop rename are consistent. Double-check that the aliasVariantLanguageIdAndSlugis either used or removed to keep TS happy withnoUnusedLocals.apps/watch/pages/watch/[part1]/[part2]/[part3].tsx (1)
180-198: Consistent defaulting and mapping LGTMUsing
?? []here avoids the empty-array false positive. Mapping directly toAudioLanguageDatais clean.
apps/watch/src/components/NewVideoContentPage/NewVideoContentPage.tsx
Outdated
Show resolved
Hide resolved
…r-middleware-functionality
- Consolidated loading state checks for audio languages and video ID. - Enhanced optional chaining for current audio language. - Added fallback for current language display.
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 (2)
apps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.tsx (2)
1-1: Remove unused imports to satisfy ESLint and trim bundle.These are no longer used after the refactor.
Apply this diff:
-import { useLazyQuery } from '@apollo/client' @@ -import { ReactElement, memo, useEffect, useMemo, useState } from 'react' +import { ReactElement, memo, useMemo } from 'react' @@ -import { GetLanguagesSlug } from '../../../../__generated__/GetLanguagesSlug' -import { GET_LANGUAGES_SLUG } from '../../../libs/useLanguagesSlugQuery'Also applies to: 5-5, 14-15
110-143: Use MUI components instead of raw HTML per apps/ guidelines.*Replace div/label/span+classes with MUI Box/Stack/FormLabel/Typography.
Apply this diff:
- return ( - <div className="mx-6 font-sans"> - <div className="flex items-center justify-between"> - <label - htmlFor="audio-select" - className="block text-xl font-medium text-gray-700 ml-7" - > - {t('Language')} - </label> - <span className="text-sm text-gray-400 opacity-60"> - {currentLanguage?.nativeName} - </span> - </div> - <div className="relative mt-1 flex items-start gap-2"> - <div className="pt-4"> - <SpatialAudioOffOutlinedIcon fontSize="small" /> - </div> - <div className="relative w-full"> - <LanguageAutocomplete - value={{ - id: currentLanguage?.id ?? '', - localName: currentLanguage?.localName, - nativeName: currentLanguage?.nativeName, - slug: currentLanguage?.slug - }} - onChange={handleChange} - languages={languages} - loading={loading} - disabled={loading} - renderInput={renderInput(helperText)} - renderOption={renderOption} - /> - </div> - </div> - </div> - ) + return ( + <Box sx={{ mx: 6, fontFamily: 'sans-serif' }}> + <Stack direction="row" alignItems="center" justifyContent="space-between"> + <FormLabel htmlFor="audio-select" sx={{ typography: 'h6', ml: 7 }}> + {t('Language')} + </FormLabel> + <Typography variant="body2" color="text.secondary" sx={{ opacity: 0.6 }}> + {currentLanguage?.nativeName} + </Typography> + </Stack> + <Stack direction="row" spacing={2} alignItems="flex-start" mt={1}> + <Box pt={4}> + <SpatialAudioOffOutlinedIcon fontSize="small" /> + </Box> + <Box sx={{ position: 'relative', width: '100%' }}> + <LanguageAutocomplete + value={{ + id: currentLanguage?.id ?? '', + localName: currentLanguage?.localName, + nativeName: currentLanguage?.nativeName, + slug: currentLanguage?.slug + }} + onChange={handleChange} + languages={languages} + loading={loading} + disabled={loading} + renderInput={renderInput(helperText)} + renderOption={renderOption} + /> + </Box> + </Stack> + </Box> + )Add imports:
+import { Box, Stack, FormLabel, Typography } from '@mui/material'
♻️ Duplicate comments (1)
apps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.tsx (1)
89-96: Fix availability check to match the message language selection.Compare against
currentLanguage?.id(the language referenced in the message), notcurrentAudioLanguage?.id. Also dropcurrentAudioLanguagefrom the deps if no longer used.Apply this diff:
- const found = videoAudioLanguagesIdsAndSlugs?.find( - ({ id }) => id === currentAudioLanguage?.id - ) + const found = videoAudioLanguagesIdsAndSlugs?.find( + ({ id }) => id === currentLanguage?.id + ) @@ - currentAudioLanguage, videoAudioLanguagesIdsAndSlugs,Also applies to: 103-107
🧹 Nitpick comments (6)
apps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.tsx (1)
29-41: Remove unuseddispatch.It’s destructured but not used; cleans up ESLint warning.
Apply this diff:
const { state: { allLanguages, currentAudioLanguage, audioLanguage, videoId, videoAudioLanguagesIdsAndSlugs, loading - }, - dispatch + } } = useWatch()apps/watch/src/components/LanguageSwitchDialog/utils/getCurrentAudioLanguage/getCurrentAudioLanguage.spec.ts (4)
84-87: DRY and type the currentAudioLanguage shapeDefine a small typed helper and use it here to avoid repeating object literals and to satisfy the “define a type if possible” guideline.
Apply:
+// helper for currentAudioLanguage objects +type AudioRef = { id: string; slug: string } +const audio = (id: string, slug: string): AudioRef => ({ id, slug })And replace inline literal:
- currentAudioLanguage: { id: '496', slug: 'spanish' }, + currentAudioLanguage: audio('496', 'spanish'),
186-191: Reuse the helper for readabilityUse the proposed audio() helper here too.
- currentAudioLanguage: { id: '529', slug: 'english' }, + currentAudioLanguage: audio('529', 'english'),
210-215: Reuse the helper for readabilitySame here—replace inline literal with audio().
- currentAudioLanguage: { id: '529', slug: 'english' }, + currentAudioLanguage: audio('529', 'english'),
156-159: Add test for unmatchedcurrentAudioLanguagefallback
UndergetCurrentAudioLanguage.spec.ts(e.g. after the “Priority order” tests), add a case wherecurrentAudioLanguageis provided but itsid/slugaren’t inmockLanguagesto lock in the existing priority logic (path slug → audioLanguage). For example:it('should ignore unknown currentAudioLanguage and fall back to path slug', () => { const result = getCurrentAudioLanguage({ allLanguages: mockLanguages, currentAudioLanguage: { id: '9999', slug: 'does-not-exist' }, routerAsPath: '/watch/jesus/spanish.html', audioLanguage: '529', }) expect(result).toEqual(expectedSpanishOption) })apps/watch/src/libs/watchContext/WatchContext.spec.tsx (1)
175-175: Nit: test title typoRemove stray closing parenthesis.
-it('should not set autoSubtitle when language preference is met langPrefMet is true)', () => { +it('should not set autoSubtitle when language preference is met (langPrefMet is true)', () => {
📜 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 (5)
apps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.tsx(5 hunks)apps/watch/src/components/LanguageSwitchDialog/utils/getCurrentAudioLanguage/getCurrentAudioLanguage.spec.ts(4 hunks)apps/watch/src/components/LanguageSwitchDialog/utils/getCurrentAudioLanguage/getCurrentAudioLanguage.ts(2 hunks)apps/watch/src/components/NewVideoContentPage/NewVideoContentPage.tsx(0 hunks)apps/watch/src/libs/watchContext/WatchContext.spec.tsx(10 hunks)
💤 Files with no reviewable changes (1)
- apps/watch/src/components/NewVideoContentPage/NewVideoContentPage.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/watch/src/components/LanguageSwitchDialog/utils/getCurrentAudioLanguage/getCurrentAudioLanguage.ts
🧰 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/components/LanguageSwitchDialog/utils/getCurrentAudioLanguage/getCurrentAudioLanguage.spec.tsapps/watch/src/libs/watchContext/WatchContext.spec.tsxapps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/base.mdc)
Define a type if possible.
Files:
apps/watch/src/components/LanguageSwitchDialog/utils/getCurrentAudioLanguage/getCurrentAudioLanguage.spec.tsapps/watch/src/libs/watchContext/WatchContext.spec.tsxapps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.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/components/LanguageSwitchDialog/utils/getCurrentAudioLanguage/getCurrentAudioLanguage.spec.tsapps/watch/src/libs/watchContext/WatchContext.spec.tsxapps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.tsx
🧠 Learnings (1)
📚 Learning: 2025-07-15T03:19:49.322Z
Learnt from: Kneesal
PR: JesusFilm/core#7171
File: apps/watch/src/components/LanguageSwitchDialog/SubtitlesSelect/SubtitlesSelect.spec.tsx:447-451
Timestamp: 2025-07-15T03:19:49.322Z
Learning: In the SubtitlesSelect component tests, the team intentionally uses integration tests that verify interactions work without errors rather than explicitly testing function calls, with explanatory comments to inform future developers of this testing strategy.
Applied to files:
apps/watch/src/libs/watchContext/WatchContext.spec.tsx
🧬 Code graph analysis (1)
apps/watch/src/libs/watchContext/WatchContext.spec.tsx (1)
apps/watch/src/libs/watchContext/WatchContext.tsx (2)
WatchAction(169-181)reducer(223-357)
🪛 GitHub Actions: autofix.ci
apps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.tsx
[warning] 1-1: ESLint: 'useLazyQuery' is defined but never used. (no-unused-vars)
[warning] 5-5: ESLint: 'useEffect' is defined but never used. (no-unused-vars)
[warning] 5-5: ESLint: 'useState' is defined but never used. (no-unused-vars)
[warning] 14-14: ESLint: 'GetLanguagesSlug' is defined but never used. (no-unused-vars)
[warning] 15-15: ESLint: 'GET_LANGUAGES_SLUG' is defined but never used. (no-unused-vars)
[warning] 39-39: ESLint: 'dispatch' is assigned a value but never used. (no-unused-vars)
🔇 Additional comments (15)
apps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.tsx (5)
36-37: LGTM: correct state source (ids+slugs) is used.Switching to
videoAudioLanguagesIdsAndSlugsaligns with the new data shape.
67-72: LGTM: reload gating logic is correct.Computing
reloadonly when InstantSearch is absent and the selected language exists matches the intent.
85-99: LGTM: helperText early returns are clear.The loading and no-video branches are succinct and readable.
112-116: Verify label-input association for accessibility.Ensure the underlying input rendered by
LanguageAutocompleteactually has id="audio-select"; otherwise theFormLabel/htmlFor(or label/htmlFor) won’t bind.If
LanguageAutocompletesupports it, pass an input id prop (e.g.,inputId="audio-select"). If not, updaterenderInputto setidon the TextField, or addaria-labelledbylinking to the label.Also applies to: 127-140
136-138: LGTM: loading/disabled wiring.The
loadingstate is correctly applied to UX and interactivity.apps/watch/src/libs/watchContext/WatchContext.spec.tsx (10)
94-99: Good: verifies id/slug shape and selection of currentAudioLanguageAssertions align with reducer semantics and new payload shape.
Also applies to: 101-106, 108-114
142-147: Good: subtitle IDs shape and autoSubtitle=true when availableMatches reducer branch where subtitleAvailable === true.
Also applies to: 149-155, 151-153
158-169: Good: autoSubtitle disabled when preference not met and subtitle not availableExpectation of undefined aligns with reducer.
Also applies to: 170-173
176-186: Good: respects langPrefMet early return (no autoSubtitle change)Accurately captures the reducer’s early-return behavior.
Also applies to: 187-198, 194-196
200-216: Good: autoSubtitle=true when preference not met and subtitle availableCovers the positive path.
Also applies to: 218-224, 220-221
226-242: Good: autoSubtitle undefined when preference not met and subtitle unavailableCovers the negative path.
Also applies to: 244-250, 246-247
252-266: Good: handles undefined currentAudioLanguageConfirms fallback behavior without currentAudioLanguage.
Also applies to: 267-273, 269-270
467-469: Good: asserts new initial arrays are emptyNew state shape validated.
480-487: Good: initial state with extras wires through and selects currentAudioLanguageMatches reducer’s selection logic and preserved fields.
Also applies to: 490-492, 513-516
520-533: The reducer will setcurrentAudioLanguagewhenever the siteLanguage’s slugs include the audio track’s slug. SincedefaultInitialState.siteLanguagedefaults to"en", and"english"is one of its slugs, your test with siteLanguage"en"and an English track will deterministically pick up that audio as the current audio, makingcurrentAudioLanguagenon-undefined. To force the fallback path you’re testing (no audio match →currentAudioLanguagestaysundefined), inject a siteLanguage whose slug list doesn’t include"english"(e.g."fr").No further changes required—your diff is correct.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.spec.tsx (3)
1369-1390: Migrate empty array case to new keyThis still uses videoAudioLanguages: []. Switch to videoAudioLanguagesIdsAndSlugs: [].
- videoAudioLanguages: [] + videoAudioLanguagesIdsAndSlugs: []
1031-1031: Await userEvent calls to avoid flakinessuserEvent is async; not awaiting can cause racey assertions.
- userEvent.click(screen.getByRole('combobox')) + await userEvent.click(screen.getByRole('combobox')) - userEvent.click(screen.getByRole('option', { name: 'Spanish' })) + await userEvent.click(screen.getByRole('option', { name: 'Spanish' })) - userEvent.click(screen.getByRole('combobox')) + await userEvent.click(screen.getByRole('combobox')) - userEvent.click(screen.getByRole('option', { name: 'French' })) + await userEvent.click(screen.getByRole('option', { name: 'French' })) - userEvent.click(screen.getByRole('combobox')) + await userEvent.click(screen.getByRole('combobox')) - userEvent.click(screen.getByRole('combobox')) + await userEvent.click(screen.getByRole('combobox')) - userEvent.click(screen.getByRole('combobox')) + await userEvent.click(screen.getByRole('combobox')) - userEvent.click(screen.getByRole('option', { name: 'English' })) + await userEvent.click(screen.getByRole('option', { name: 'English' })) - userEvent.click(screen.getByRole('combobox')) + await userEvent.click(screen.getByRole('combobox')) - userEvent.click(screen.getByRole('option', { name: 'English' })) + await userEvent.click(screen.getByRole('option', { name: 'English' }))Also applies to: 1037-1037, 1125-1125, 1131-1131, 1210-1210, 1293-1293, 1352-1352, 1411-1411
513-547: Update all tests to use flattened audio language shapes
Replace legacyvideoAudioLanguagesand nestedcurrentAudioLanguagein tests (lines 513–547, 1308, 1367, 1389) with the new props:- videoAudioLanguages: [ - { __typename: 'LanguageWithSlug' as const, slug: 'spanish', language: { id: '496', slug: 'spanish', … } } - ], - currentAudioLanguage: { __typename: 'LanguageWithSlug' as const, slug: 'spanish', language: { id: '496', slug: 'spanish', … } } + videoAudioLanguagesIdsAndSlugs: [{ id: '496', slug: 'spanish' }], + currentAudioLanguage: { id: '496', slug: 'spanish' }apps/watch/src/components/NewVideoContentPage/NewVideoContentPage.spec.tsx (1)
170-204: Align GraphQL mock to match GET_LANGUAGES_SLUG selection
In NewVideoContentPage.spec.tsx thegetLanguagesSlugMock.result.data.video.variantLanguagesWithSlugitems include extralanguage.slugandlanguage.namefields, but the activeGET_LANGUAGES_SLUGquery only selectsvariantLanguagesWithSlug { slug language { id } }Update the mock to only include
{ __typename: 'LanguageWithSlug', slug: string, language: { __typename: 'Language', id: string } }so tests reflect the real query shape and avoid false positives.
🧹 Nitpick comments (5)
apps/watch/src/libs/watchContext/initializeVideoLanguages/initializeVideoLanguages.spec.ts (1)
21-24: Type and de-duplicate mock audio language objects.Reduce repetition and satisfy “define a type if possible” by typing/hoisting the fixture.
Within tests, annotate inline or hoist a shared const:
- const mockAudioLanguages = [ - { - id: '529', - slug: 'english' - } - ] + const mockAudioLanguages: Array<{ id: string; slug: string }> = [ + { id: '529', slug: 'english' } + ]Additionally (outside selected ranges), consider adding once near the top and reusing:
// near the top of `describe` const englishAudio = { id: '529', slug: 'english' } as const;Then use [englishAudio] in each test.
Also applies to: 46-49, 71-74, 119-122, 176-179
apps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.spec.tsx (1)
767-771: Prefer userEvent over fireEvent for realistic interactionsUse userEvent.type/click for Autocomplete; also remove unnecessary await on fireEvent.change.
- fireEvent.click(screen.getByRole('combobox')) - fireEvent.change(screen.getByRole('combobox'), { + await userEvent.click(screen.getByRole('combobox')) + await userEvent.type(screen.getByRole('combobox'), '496') - fireEvent.click(screen.getByRole('combobox')) - fireEvent.change(screen.getByRole('combobox'), { + await userEvent.click(screen.getByRole('combobox')) + await userEvent.type(screen.getByRole('combobox'), '496') - await fireEvent.change(input, { target: { value: 'Spanish' } }) + await userEvent.type(input, 'Spanish') - fireEvent.click(screen.getByText('Spanish')) + await userEvent.click(screen.getByText('Spanish'))Also applies to: 828-831, 940-948
apps/watch/src/components/NewVideoContentPage/NewVideoContentPage.spec.tsx (3)
25-26: Rename the describe title to match the componentFor clarity and test discovery, align the suite name with the component/file name.
-describe('NewContentPage', () => { +describe('NewVideoContentPage', () => {
149-151: Use jest.spyOn for window.open and restore mocks to avoid leakageDirectly reassigning window.open can leak across tests. Spy + restore is safer.
- const user = userEvent.setup() - window.open = jest.fn() + const user = userEvent.setup() + const openSpy = jest + .spyOn(window, 'open') + .mockImplementation(() => null as unknown as Window) @@ - expect(window.open).toHaveBeenCalledWith( + expect(openSpy).toHaveBeenCalledWith( 'https://join.bsfinternational.org/?utm_source=jesusfilm-watch', '_blank' )Add once per file to auto-restore:
afterEach(() => { jest.restoreAllMocks() })Also applies to: 164-167
219-229: Consider singular/plural handling for “audio language(s)”Current expectation reads “1 audio languages”. Prefer singularization at the source (TestWatchState) and reflect it in the test to improve UX consistency.
Example in the source (pseudo):
const label = `${count} audio ${count === 1 ? 'language' : 'languages'}`Then update this test accordingly:
- screen.getByText('videoAudioLanguagesIdsAndSlugs: 1 audio languages') + screen.getByText('videoAudioLanguagesIdsAndSlugs: 1 audio language')
📜 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 (5)
apps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.spec.tsx(8 hunks)apps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.tsx(7 hunks)apps/watch/src/components/NewVideoContentPage/NewVideoContentPage.spec.tsx(1 hunks)apps/watch/src/components/NewVideoContentPage/NewVideoContentPage.tsx(1 hunks)apps/watch/src/libs/watchContext/initializeVideoLanguages/initializeVideoLanguages.spec.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.tsx
- apps/watch/src/components/NewVideoContentPage/NewVideoContentPage.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/components/NewVideoContentPage/NewVideoContentPage.spec.tsxapps/watch/src/libs/watchContext/initializeVideoLanguages/initializeVideoLanguages.spec.tsapps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.spec.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/base.mdc)
Define a type if possible.
Files:
apps/watch/src/components/NewVideoContentPage/NewVideoContentPage.spec.tsxapps/watch/src/libs/watchContext/initializeVideoLanguages/initializeVideoLanguages.spec.tsapps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.spec.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/components/NewVideoContentPage/NewVideoContentPage.spec.tsxapps/watch/src/libs/watchContext/initializeVideoLanguages/initializeVideoLanguages.spec.tsapps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.spec.tsx
🧠 Learnings (1)
📚 Learning: 2025-07-15T03:19:49.322Z
Learnt from: Kneesal
PR: JesusFilm/core#7171
File: apps/watch/src/components/LanguageSwitchDialog/SubtitlesSelect/SubtitlesSelect.spec.tsx:447-451
Timestamp: 2025-07-15T03:19:49.322Z
Learning: In the SubtitlesSelect component tests, the team intentionally uses integration tests that verify interactions work without errors rather than explicitly testing function calls, with explanatory comments to inform future developers of this testing strategy.
Applied to files:
apps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.spec.tsx
⏰ 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, 7548/merge, pull_request, 22)
- GitHub Check: test (22, 3/3)
- GitHub Check: build (22)
- GitHub Check: test (22, 2/3)
- GitHub Check: test (22, 1/3)
- GitHub Check: lint (22)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (17)
apps/watch/src/libs/watchContext/initializeVideoLanguages/initializeVideoLanguages.spec.ts (6)
21-24: Audio payload shape and key rename look correct — LGTM.Using flat { id, slug } and asserting videoAudioLanguagesIdsAndSlugs matches the refactor intent.
Also applies to: 26-26, 39-39
46-49: Subtitle payload assertion aligns with new API — LGTM.Asserting videoSubtitleLanguageIds: string[] is correct.
Also applies to: 51-51, 64-64
71-74: Order-sensitive dispatch checks are precise — LGTM.First audio (videoAudioLanguagesIdsAndSlugs), then subtitles (videoSubtitleLanguageIds).
Also applies to: 76-76, 89-89, 94-94
99-99: Empty audio array case covered — LGTM.Confirms stable two-dispatch behavior.
Also applies to: 107-107, 112-112
119-122: Empty subtitle array case covered — LGTM.Also applies to: 130-130, 135-135
146-146: Both empty arrays case covered — LGTM.Also applies to: 151-151
apps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.spec.tsx (8)
83-83: LGTM: consistent Apollo test harnessUsing MockedProvider with addTypename={false} is appropriate for these tests.
265-269: LGTM: migrated to videoAudioLanguagesIdsAndSlugsShape { id, slug } aligns with the refactor.
450-459: LGTM: preferred-language available path uses new shapesBoth videoAudioLanguagesIdsAndSlugs and currentAudioLanguage are flattened correctly.
1006-1015: LGTM: availability list migratedCorrect id/slug entries for reload=true scenario.
1100-1109: LGTM: availability list excludes FrenchMatches reload=false expectation.
1183-1188: LGTM: single-availability caseUndefined in list implies reload=false as asserted.
1262-1271: LGTM: instantSearch present pathAvailability list correct; reload=false as expected.
1330-1330: LGTM: undefined availability handledCovers edge case explicitly.
apps/watch/src/components/NewVideoContentPage/NewVideoContentPage.spec.tsx (3)
219-221: LGTM: assertion updated to new audio-languages keyExpectation now matches the renamed payload key. Looks good.
227-229: LGTM: post-fetch assertionThe updated count assertion reflects the new key correctly.
5-10: Retain legacy GET_LANGUAGES_SLUG imports—still in use
The GET_LANGUAGES_SLUG query and its generated types power NewVideoContentPage.tsx via useLanguagesSlugQuery and remain the current implementation; migrate this spec when you adopt GET_VARIANT_LANGUAGES_ID_AND_SLUG.
apps/watch/src/libs/watchContext/initializeVideoLanguages/initializeVideoLanguages.spec.ts
Show resolved
Hide resolved
apps/watch/src/libs/watchContext/initializeVideoLanguages/initializeVideoLanguages.spec.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
apps/watch/pages/watch/[part1]/[part2].tsx (2)
159-163: Fix identifier mismatch: passing Relaycontent.idto adatabaseIdquery will fail.Either pass
content.databaseIdor switch the query toidType: slugand pass the variant slug. Prefer aligning withdatabaseIdto avoid ambiguity.- variables: { - id: contentData.content.id - } + variables: { + id: contentData.content.databaseId + }If
databaseIdisn’t inVideoContentFields, extend the fragment to include it before applying this change:fragment VideoContentFields on Video { id databaseId # ...existing fields }Verification script:
#!/bin/bash # Ensure VideoContentFields selects databaseId rg -nP -C2 'fragment\s+VideoContentFields\b' --type=ts --type=tsx | sed -n '1,200p' # Confirm no remaining call sites pass `.id` into GET_VARIANT_LANGUAGES_ID_AND_SLUG rg -nP -C2 'GET_VARIANT_LANGUAGES_ID_AND_SLUG' apps/watch/pages/watch --type=tsx | sed -n '1,200p'
171-173: EnsurevideoSubtitleLanguageIdsis always an array.Current assignment can be
undefined, violating the declaredstring[]type and causing runtime surprises.- videoSubtitleLanguageIds = data?.video?.subtitles?.map( - ({ languageId }) => languageId - ) + videoSubtitleLanguageIds = + data?.video?.subtitles?.map(({ languageId }) => languageId) ?? []
🧹 Nitpick comments (5)
apps/watch/src/libs/useVariantLanguagesIdAndSlugQuery/useVariantLanguagesIdAndSlugQuery.spec.tsx (2)
71-74: Avoid unnecessary act wrapper; wait on the hook result instead of the mock.This reduces test flakiness and asserts what matters.
- await act( - async () => await waitFor(() => expect(mockResult).toHaveBeenCalled()) - ) + await waitFor(() => expect(result.current.data?.video).toEqual(mockVideo))
11-35: Deduplicate fixtures by reusing the exported mocks.Import from
useVariantLanguagesIdAndSlugQuery.mock.tsand referencemockVariantLanguagesIdAndSlugData.videoto cut duplication and keep tests in sync with other suites.apps/watch/src/libs/useVariantLanguagesIdAndSlugQuery/useVariantLanguagesIdAndSlugQuery.ts (1)
1-1: Optional: adopt TypedDocumentNode to eliminate generics at call sites.This tightens type inference and reduces boilerplate.
-import { QueryHookOptions, QueryResult, gql, useQuery } from '@apollo/client' +import { QueryHookOptions, QueryResult, gql, useQuery } from '@apollo/client' +import type { TypedDocumentNode } from '@graphql-typed-document-node/core' @@ -export const GET_VARIANT_LANGUAGES_ID_AND_SLUG = gql` +export const GET_VARIANT_LANGUAGES_ID_AND_SLUG: TypedDocumentNode< + GetVariantLanguagesIdAndSlug, + GetVariantLanguagesIdAndSlugVariables +> = gql`Also applies to: 8-8, 20-20
apps/watch/pages/watch/[part1]/[part2].tsx (1)
164-170: Use nullish coalescing, not logical OR, for array fallbacks.Avoids surprising fallback on truthy empty arrays (style consistency with Part3 page).
- videoAudioLanguagesData = - data?.video?.variantLanguages?.map( + videoAudioLanguagesData = + data?.video?.variantLanguages?.map( ({ id, slug }): AudioLanguageData => ({ id, slug - }) - ) || [] + }) + ) ?? []apps/watch/pages/watch/[part1]/[part2]/[part3].tsx (1)
51-58: Opportunity to truly condense watch queries: inline languages into the content query.To remove an extra network round-trip in getStaticProps, consider adding
variantLanguages { id slug }andsubtitles { languageId }toGET_VIDEO_CONTENT_PART_3(and similarly for Part2), then map directly fromcontent. This keeps the new hook available for client-side needs but avoids the extra SSR fetch.Also applies to: 170-188
📜 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 (6)
apps/watch/pages/watch/[part1]/[part2].tsx(5 hunks)apps/watch/pages/watch/[part1]/[part2]/[part3].tsx(5 hunks)apps/watch/src/libs/useVariantLanguagesIdAndSlugQuery/index.ts(1 hunks)apps/watch/src/libs/useVariantLanguagesIdAndSlugQuery/useVariantLanguagesIdAndSlugQuery.mock.ts(1 hunks)apps/watch/src/libs/useVariantLanguagesIdAndSlugQuery/useVariantLanguagesIdAndSlugQuery.spec.tsx(1 hunks)apps/watch/src/libs/useVariantLanguagesIdAndSlugQuery/useVariantLanguagesIdAndSlugQuery.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/watch/src/libs/useVariantLanguagesIdAndSlugQuery/index.ts
🧰 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/useVariantLanguagesIdAndSlugQuery/useVariantLanguagesIdAndSlugQuery.spec.tsxapps/watch/src/libs/useVariantLanguagesIdAndSlugQuery/useVariantLanguagesIdAndSlugQuery.mock.tsapps/watch/pages/watch/[part1]/[part2]/[part3].tsxapps/watch/pages/watch/[part1]/[part2].tsxapps/watch/src/libs/useVariantLanguagesIdAndSlugQuery/useVariantLanguagesIdAndSlugQuery.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/base.mdc)
Define a type if possible.
Files:
apps/watch/src/libs/useVariantLanguagesIdAndSlugQuery/useVariantLanguagesIdAndSlugQuery.spec.tsxapps/watch/src/libs/useVariantLanguagesIdAndSlugQuery/useVariantLanguagesIdAndSlugQuery.mock.tsapps/watch/pages/watch/[part1]/[part2]/[part3].tsxapps/watch/pages/watch/[part1]/[part2].tsxapps/watch/src/libs/useVariantLanguagesIdAndSlugQuery/useVariantLanguagesIdAndSlugQuery.ts
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/useVariantLanguagesIdAndSlugQuery/useVariantLanguagesIdAndSlugQuery.spec.tsxapps/watch/src/libs/useVariantLanguagesIdAndSlugQuery/useVariantLanguagesIdAndSlugQuery.mock.tsapps/watch/pages/watch/[part1]/[part2]/[part3].tsxapps/watch/pages/watch/[part1]/[part2].tsxapps/watch/src/libs/useVariantLanguagesIdAndSlugQuery/useVariantLanguagesIdAndSlugQuery.ts
🧬 Code graph analysis (5)
apps/watch/src/libs/useVariantLanguagesIdAndSlugQuery/useVariantLanguagesIdAndSlugQuery.spec.tsx (1)
apps/watch/src/libs/useVariantLanguagesIdAndSlugQuery/useVariantLanguagesIdAndSlugQuery.ts (2)
useVariantLanguagesIdAndSlugQuery(22-37)GET_VARIANT_LANGUAGES_ID_AND_SLUG(8-20)
apps/watch/src/libs/useVariantLanguagesIdAndSlugQuery/useVariantLanguagesIdAndSlugQuery.mock.ts (1)
apps/watch/__generated__/GetVariantLanguagesIdAndSlug.ts (1)
GetVariantLanguagesIdAndSlug(27-29)
apps/watch/pages/watch/[part1]/[part2]/[part3].tsx (3)
apps/watch/src/libs/watchContext/WatchContext.tsx (1)
AudioLanguageData(17-20)apps/watch/__generated__/GetVariantLanguagesIdAndSlug.ts (2)
GetVariantLanguagesIdAndSlug(27-29)GetVariantLanguagesIdAndSlugVariables(31-33)apps/watch/src/libs/useVariantLanguagesIdAndSlugQuery/useVariantLanguagesIdAndSlugQuery.ts (1)
GET_VARIANT_LANGUAGES_ID_AND_SLUG(8-20)
apps/watch/pages/watch/[part1]/[part2].tsx (3)
apps/watch/src/libs/watchContext/WatchContext.tsx (1)
AudioLanguageData(17-20)apps/watch/__generated__/GetVariantLanguagesIdAndSlug.ts (2)
GetVariantLanguagesIdAndSlug(27-29)GetVariantLanguagesIdAndSlugVariables(31-33)apps/watch/src/libs/useVariantLanguagesIdAndSlugQuery/useVariantLanguagesIdAndSlugQuery.ts (1)
GET_VARIANT_LANGUAGES_ID_AND_SLUG(8-20)
apps/watch/src/libs/useVariantLanguagesIdAndSlugQuery/useVariantLanguagesIdAndSlugQuery.ts (2)
apps/watch/src/libs/useVariantLanguagesIdAndSlugQuery/index.ts (2)
GET_VARIANT_LANGUAGES_ID_AND_SLUG(3-3)useVariantLanguagesIdAndSlugQuery(2-2)apps/watch/__generated__/GetVariantLanguagesIdAndSlug.ts (2)
GetVariantLanguagesIdAndSlug(27-29)GetVariantLanguagesIdAndSlugVariables(31-33)
⏰ 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). (2)
- GitHub Check: Deploy Preview (watch, 7548/merge, pull_request, 22)
- GitHub Check: lint (22)
🔇 Additional comments (3)
apps/watch/src/libs/useVariantLanguagesIdAndSlugQuery/useVariantLanguagesIdAndSlugQuery.ts (1)
22-37: LGTM — thin, typed wrapper around Apollo useQuery.Hook shape and type plumbing look correct.
apps/watch/src/libs/useVariantLanguagesIdAndSlugQuery/useVariantLanguagesIdAndSlugQuery.mock.ts (1)
3-38: Mocks look solid and typed; good coverage (full/empty/partial).No changes requested.
apps/watch/pages/watch/[part1]/[part2]/[part3].tsx (1)
186-187: Nice: nullish fallback preserves array type.Consistent, safe mapping.
.../watch/src/libs/useVariantLanguagesIdAndSlugQuery/useVariantLanguagesIdAndSlugQuery.spec.tsx
Show resolved
Hide resolved
apps/watch/src/libs/useVariantLanguagesIdAndSlugQuery/useVariantLanguagesIdAndSlugQuery.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
apps/watch/src/components/VideoContentPage/VideoContentPage.spec.tsx (2)
9-9: Fix ESLint: remove unused mockGetCookie and its import
mockGetCookieis never used, causing the pipeline warning. Drop the alias and the now-unusedgetCookieimport.Apply this diff:
-import { getCookie } from '../../libs/cookieHandler' @@ -const mockGetCookie = getCookie as jest.MockedFunction<typeof getCookie>Also applies to: 32-32
1-167: Remove all remaining legacy language-query usages
The search shows leftoverGET_LANGUAGES_SLUGanduseLanguagesSlugQueryreferences underapps/watch/src/libs/useLanguagesSlugQuery/and in multiple components (e.g. AudioLanguageSelect, NewVideoContentPage, VideoHero.stories). These must be removed or replaced to fully eliminate legacy language queries.apps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.spec.tsx (2)
74-79: Await user-event calls to fix no-floating-promises errors.user-event v14+ returns promises; instantiate a user and await interactions.
Apply these diffs:
@@ beforeEach(() => { useRouterMock.mockReturnValue(mockRouter) useTranslationMock.mockReturnValue({ t: mockT }) useInstantSearchMock.mockReturnValue(undefined) // Default to no instant search jest.clearAllMocks() + user = userEvent.setup() })Add a user variable near the top of the file (after mocks):
+let user: ReturnType<typeof userEvent.setup>Replace direct calls:
- userEvent.click(screen.getByRole('combobox')) + await user.click(screen.getByRole('combobox')) @@ - userEvent.click(screen.getByRole('option', { name: 'Spanish' })) + await user.click(screen.getByRole('option', { name: 'Spanish' })) @@ - userEvent.click(screen.getByRole('combobox')) + await user.click(screen.getByRole('combobox')) @@ - userEvent.click(screen.getByRole('option', { name: 'French' })) + await user.click(screen.getByRole('option', { name: 'French' })) @@ - userEvent.click(screen.getByRole('combobox')) + await user.click(screen.getByRole('combobox')) @@ - userEvent.click(screen.getByRole('option', { name: 'Spanish' })) + await user.click(screen.getByRole('option', { name: 'Spanish' })) @@ - userEvent.click(screen.getByRole('combobox')) + await user.click(screen.getByRole('combobox')) @@ - userEvent.click(screen.getByRole('option', { name: 'English' })) + await user.click(screen.getByRole('option', { name: 'English' })) @@ - userEvent.click(screen.getByRole('combobox')) + await user.click(screen.getByRole('combobox')) @@ - userEvent.click(screen.getByRole('option', { name: 'English' })) + await user.click(screen.getByRole('option', { name: 'English' }))Also applies to: 1031-1031, 1037-1037, 1125-1125, 1131-1131, 1204-1204, 1210-1210, 1287-1287, 1293-1293, 1346-1346, 1352-1352, 1405-1405, 1411-1411
1367-1390: Migrate remaining test state to new shape (IdsAndSlugs).This test still uses legacy videoAudioLanguages: []; use videoAudioLanguagesIdsAndSlugs: [] to align with the public state.
- ], - videoAudioLanguages: [] + ], + videoAudioLanguagesIdsAndSlugs: []apps/watch/src/libs/watchContext/WatchContext.tsx (2)
196-209: Fix createContext default to enable “used outside provider” guard.Currently context is created with a non-undefined default, so the guard never triggers. Initialize with undefined and type accordingly.
-const WatchContext = createContext<{ - state: WatchState - dispatch: Dispatch<WatchAction> -}>({ - state: { - siteLanguage: 'en', - audioLanguage: '529', - subtitleLanguage: '529', - subtitleOn: false, - loading: false - }, - dispatch: () => null -}) +const WatchContext = createContext< + { state: WatchState; dispatch: Dispatch<WatchAction> } | undefined +>(undefined) @@ - const context = useContext(WatchContext) + const context = useContext(WatchContext) - if (context === undefined) { + if (context === undefined) { throw new Error('useWatch must be used within a WatchProvider') } return contextAlso applies to: 426-438
237-262: Correct audio-language selection logic; slug comparison is mismatched.LANGUAGE_MAPPINGS.languageSlugs contains values like 'spanish-latin-american.html', while lang.slug is typically 'spanish'. Comparing them won’t match. Prefer ID-based match (audioLanguage and site language’s languageId), and optionally fall back to videoVariantSlug sans “.html”.
Apply this refactor:
- case 'SetVideoAudioLanguages': { - const videoAudioLanguagesIdsAndSlugs = - action.videoAudioLanguagesIdsAndSlugs - - // Find the best matching audio language for the current user preference - let currentAudioLanguage: AudioLanguageData | undefined - - // Check if user's audio preference is available for this video - for (const lang of videoAudioLanguagesIdsAndSlugs) { - const siteLanguageMapping = LANGUAGE_MAPPINGS[state.siteLanguage] - const siteLanguageSlugs = siteLanguageMapping?.languageSlugs || [] - - if ( - lang.id === state.audioLanguage || - siteLanguageSlugs.includes(lang.slug || '') - ) { - currentAudioLanguage = lang - break - } - } + case 'SetVideoAudioLanguages': { + const videoAudioLanguagesIdsAndSlugs = + action.videoAudioLanguagesIdsAndSlugs + + // Prefer explicit user preference; then site language’s canonical ID; then variant slug match + const preferredSiteLangId = + LANGUAGE_MAPPINGS[state.siteLanguage]?.languageId + const variantSlugNoHtml = state.videoVariantSlug?.replace(/\.html$/, '') + + let currentAudioLanguage: AudioLanguageData | undefined = + videoAudioLanguagesIdsAndSlugs.find( + (l) => l.id === state.audioLanguage + ) ?? + (preferredSiteLangId != null + ? videoAudioLanguagesIdsAndSlugs.find((l) => l.id === preferredSiteLangId) + : undefined) ?? + (variantSlugNoHtml != null + ? videoAudioLanguagesIdsAndSlugs.find( + (l) => (l.slug ?? '') === variantSlugNoHtml + ) + : undefined) // If no matches found, currentAudioLanguage remains undefined return { ...state, videoAudioLanguagesIdsAndSlugs, currentAudioLanguage } }
♻️ Duplicate comments (1)
apps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.tsx (1)
85-101: Fix availability check to compare against currentLanguage, not currentAudioLanguage.Helper text text uses currentLanguage.* but membership checks against currentAudioLanguage?.id. This diverges when currentAudioLanguage is undefined or differs from the displayed language, producing wrong “Not available in …” messaging.
Apply this diff:
- const found = videoAudioLanguagesIdsAndSlugs?.find( - ({ id }) => id === currentAudioLanguage?.id - ) + const found = videoAudioLanguagesIdsAndSlugs?.find( + ({ id }) => id === currentLanguage?.id + ) @@ - currentAudioLanguage, videoAudioLanguagesIdsAndSlugs, currentLanguage
🧹 Nitpick comments (9)
apps/watch/src/components/VideoContentPage/VideoContentPage.spec.tsx (3)
2-2: Prefer userEvent and async finders for realistic interactionsSwitch from
fireEvent+ synchronous assertions touserEvent+findByRoleto reduce flakiness and better emulate user behavior.Apply this diff:
-import { fireEvent, render, waitFor } from '@testing-library/react' +import { render } from '@testing-library/react' +import userEvent from '@testing-library/user-event' @@ - it('should render share button', () => { - const { getByRole } = render( + it('should render share button', async () => { + const { getByRole, findByRole } = render( <MockedProvider> <SnackbarProvider> <VideoProvider value={{ content: videos[0] }}> <VideoContentPage /> </VideoProvider> </SnackbarProvider> </MockedProvider> ) expect(getByRole('button', { name: 'Share' })).toBeInTheDocument() - fireEvent.click(getByRole('button', { name: 'Share' })) - expect( - getByRole('dialog', { name: 'Share this video' }) - ).toBeInTheDocument() + const user = userEvent.setup() + await user.click(getByRole('button', { name: 'Share' })) + expect(await findByRole('dialog', { name: 'Share this video' })).toBeInTheDocument() }) @@ - it('should render download button', () => { - const { getByRole } = render( + it('should render download button', async () => { + const { getByRole, findByRole } = render( <MockedProvider> <SnackbarProvider> <VideoProvider value={{ content: videos[0] }}> <VideoContentPage /> </VideoProvider> </SnackbarProvider> </MockedProvider> ) expect(getByRole('button', { name: 'Download' })).toBeInTheDocument() - fireEvent.click(getByRole('button', { name: 'Download' })) - expect(getByRole('dialog', { name: 'Download Video' })).toBeInTheDocument() + const user = userEvent.setup() + await user.click(getByRole('button', { name: 'Download' })) + expect(await findByRole('dialog', { name: 'Download Video' })).toBeInTheDocument() })Also applies to: 124-138, 140-153
106-114: Simplify async assertion: use findByRole instead of waitFor + getByRoleCleaner and less timing-sensitive.
Apply this diff:
- it('should render children', async () => { - const { getByRole } = render( + it('should render children', async () => { + const { findByRole } = render( <MockedProvider mocks={[getVideoChildrenMock]}> <SnackbarProvider> <VideoProvider value={{ content: videos[1] }}> <VideoContentPage /> </VideoProvider> </SnackbarProvider> </MockedProvider> ) - await waitFor(() => - expect( - getByRole('heading', { name: videos[1].title[0].value }) - ).toBeInTheDocument() - ) + expect( + await findByRole('heading', { name: videos[1].title[0].value }) + ).toBeInTheDocument() })Also applies to: 116-121
36-63: Avoid double cast (unknown as) in test dataIf TS version allows, prefer
satisfiesto validate shape without widening, or narrow the object to the fieldsVideoContentPageactually consumes.Example (if TS ≥ 4.9):
- ] as unknown as CoreVideo[] + ] satisfies CoreVideo[]If not, consider defining a minimal test helper type that matches the component’s usage to avoid
unknown.apps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.tsx (2)
27-39: Remove unused dispatch to satisfy ESLint.dispatch isn’t used; drop it from destructuring.
- const { - state: { - allLanguages, - currentAudioLanguage, - audioLanguage, - videoId, - videoAudioLanguagesIdsAndSlugs, - loading - }, - dispatch - } = useWatch() + const { + state: { + allLanguages, + currentAudioLanguage, + audioLanguage, + videoId, + videoAudioLanguagesIdsAndSlugs, + loading + } + } = useWatch()
104-139: Follow apps/ guideline: prefer MUI components over raw HTML + utility classes.*Replace div/label/spans/classes with MUI Box/Stack/Typography/FormLabel for consistency and built-in a11y.
Example refactor:
- return ( - <div className="mx-6 font-sans"> - <div className="flex items-center justify-between"> - <label - htmlFor="audio-select" - className="block text-xl font-medium text-gray-700 ml-7" - > - {t('Language')} - </label> - <span className="text-sm text-gray-400 opacity-60"> - {currentLanguage?.nativeName} - </span> - </div> - <div className="relative mt-1 flex items-start gap-2"> - <div className="pt-4"> - <SpatialAudioOffOutlinedIcon fontSize="small" /> - </div> - <div className="relative w-full"> + return ( + <Box sx={{ mx: 6, fontFamily: 'sans-serif' }}> + <Stack direction="row" alignItems="center" justifyContent="space-between"> + <FormLabel htmlFor="audio-select"> + <Typography variant="h6">{t('Language')}</Typography> + </FormLabel> + <Typography variant="caption" color="text.secondary"> + {currentLanguage?.nativeName} + </Typography> + </Stack> + <Stack direction="row" spacing={2} mt={1} alignItems="flex-start"> + <Box pt={1}> + <SpatialAudioOffOutlinedIcon fontSize="small" /> + </Box> + <Box sx={{ position: 'relative', width: '100%' }}> <LanguageAutocomplete + id="audio-select" value={{ id: currentLanguage?.id ?? '', localName: currentLanguage?.localName, nativeName: currentLanguage?.nativeName, slug: currentLanguage?.slug }} onChange={handleChange} languages={languages} loading={loading} disabled={loading} renderInput={renderInput(helperText)} renderOption={renderOption} /> - </div> - </div> - </div> + </Box> + </Stack> + </Box>Note: import Box, Stack, Typography, FormLabel from @mui/material.
apps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.spec.tsx (3)
1-1: Remove unused import.MockedResponse is unused.
-import { MockedProvider, MockedResponse } from '@apollo/client/testing' +import { MockedProvider } from '@apollo/client/testing'
610-968: Optional: replace fireEvent with userEvent for realism.Not required, but userEvent better models user intent for typing/clicking and avoids brittle role/DOM coupling.
1-1427: Remove unusedMockedResponseimport
InAudioTrackSelect.spec.tsx, update the Apollo import to only bring inMockedProvider—MockedResponseis never referenced.apps/watch/src/libs/watchContext/WatchContext.tsx (1)
384-395: Effect dependencies: guard against referential changes in initialState.If initialState is re-created by the parent, the effect will re-run. If that’s intentional, OK; otherwise, destructure the arrays before useEffect to stabilize deps.
📜 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 (12)
apps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.spec.tsx(9 hunks)apps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.tsx(6 hunks)apps/watch/src/components/NewVideoContentPage/VideoContentHero/HeroVideo/HeroVideo.tsx(3 hunks)apps/watch/src/components/NewVideoContentPage/VideoContentHero/VideoContentHero.spec.tsx(6 hunks)apps/watch/src/components/VideoContainerPage/AudioLanguageSelect/AudioLanguageSelect.spec.tsx(1 hunks)apps/watch/src/components/VideoContainerPage/AudioLanguageSelect/AudioLanguageSelectContent/AudioLanguageSelectContent.spec.tsx(1 hunks)apps/watch/src/components/VideoContentPage/VideoContentPage.spec.tsx(1 hunks)apps/watch/src/components/VideoContentPage/VideoHero/VideoPlayer/VideoPlayer.tsx(3 hunks)apps/watch/src/libs/watchContext/WatchContext.tsx(8 hunks)apps/watch/src/libs/watchContext/useLanguageActions/useLanguageActions.spec.tsx(1 hunks)apps/watch/src/libs/watchContext/useSubtitleUpdate/useSubtitleUpdate.spec.tsx(1 hunks)apps/watch/src/libs/watchContext/useSubtitleUpdate/useSubtitleUpdate.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/watch/src/components/VideoContainerPage/AudioLanguageSelect/AudioLanguageSelectContent/AudioLanguageSelectContent.spec.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/watch/src/components/NewVideoContentPage/VideoContentHero/VideoContentHero.spec.tsx
- apps/watch/src/libs/watchContext/useSubtitleUpdate/useSubtitleUpdate.tsx
- apps/watch/src/components/VideoContentPage/VideoHero/VideoPlayer/VideoPlayer.tsx
- apps/watch/src/libs/watchContext/useSubtitleUpdate/useSubtitleUpdate.spec.tsx
- apps/watch/src/components/NewVideoContentPage/VideoContentHero/HeroVideo/HeroVideo.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/components/VideoContentPage/VideoContentPage.spec.tsxapps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.tsxapps/watch/src/components/VideoContainerPage/AudioLanguageSelect/AudioLanguageSelect.spec.tsxapps/watch/src/libs/watchContext/useLanguageActions/useLanguageActions.spec.tsxapps/watch/src/libs/watchContext/WatchContext.tsxapps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.spec.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/base.mdc)
Define a type if possible.
Files:
apps/watch/src/components/VideoContentPage/VideoContentPage.spec.tsxapps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.tsxapps/watch/src/components/VideoContainerPage/AudioLanguageSelect/AudioLanguageSelect.spec.tsxapps/watch/src/libs/watchContext/useLanguageActions/useLanguageActions.spec.tsxapps/watch/src/libs/watchContext/WatchContext.tsxapps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.spec.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/components/VideoContentPage/VideoContentPage.spec.tsxapps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.tsxapps/watch/src/components/VideoContainerPage/AudioLanguageSelect/AudioLanguageSelect.spec.tsxapps/watch/src/libs/watchContext/useLanguageActions/useLanguageActions.spec.tsxapps/watch/src/libs/watchContext/WatchContext.tsxapps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.spec.tsx
🧠 Learnings (3)
📚 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/components/VideoContentPage/VideoContentPage.spec.tsxapps/watch/src/libs/watchContext/useLanguageActions/useLanguageActions.spec.tsxapps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.spec.tsx
📚 Learning: 2025-07-27T21:59:04.362Z
Learnt from: CR
PR: JesusFilm/core#0
File: .cursor/rules/apps.mdc:0-0
Timestamp: 2025-07-27T21:59:04.362Z
Learning: Applies to apps/**/*.test.{js,jsx,ts,tsx} : Use 'testing-library/react' npm package for frontend tests.
Applied to files:
apps/watch/src/components/VideoContentPage/VideoContentPage.spec.tsxapps/watch/src/libs/watchContext/useLanguageActions/useLanguageActions.spec.tsx
📚 Learning: 2025-07-15T03:19:49.322Z
Learnt from: Kneesal
PR: JesusFilm/core#7171
File: apps/watch/src/components/LanguageSwitchDialog/SubtitlesSelect/SubtitlesSelect.spec.tsx:447-451
Timestamp: 2025-07-15T03:19:49.322Z
Learning: In the SubtitlesSelect component tests, the team intentionally uses integration tests that verify interactions work without errors rather than explicitly testing function calls, with explanatory comments to inform future developers of this testing strategy.
Applied to files:
apps/watch/src/components/VideoContainerPage/AudioLanguageSelect/AudioLanguageSelect.spec.tsxapps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.spec.tsx
🧬 Code graph analysis (1)
apps/watch/src/libs/watchContext/WatchContext.tsx (2)
apps/watch/src/libs/localeMapping/index.ts (1)
LANGUAGE_MAPPINGS(3-3)apps/watch/src/libs/localeMapping/localeMapping.ts (1)
LANGUAGE_MAPPINGS(22-204)
🪛 GitHub Actions: autofix.ci
apps/watch/src/components/VideoContentPage/VideoContentPage.spec.tsx
[warning] 32-32: ESLint: 'mockGetCookie' is assigned a value but never used. (@typescript-eslint/no-unused-vars)
apps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.tsx
[warning] 36-36: ESLint: 'dispatch' is assigned a value but never used. (@typescript-eslint/no-unused-vars)
apps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.spec.tsx
[warning] 1-1: ESLint: 'MockedResponse' is defined but never used. (@typescript-eslint/no-unused-vars)
[warning] 1-1: ESLint: 'MockedResponse' is defined but never used. (@typescript-eslint/no-unused-vars)
[error] 1031-1031: ESLint: Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with the void operator. (@typescript-eslint/no-floating-promises)
[error] 1037-1037: ESLint: Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with the void operator. (@typescript-eslint/no-floating-promises)
[error] 1125-1125: ESLint: Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with the void operator. (@typescript-eslint/no-floating-promises)
[error] 1131-1131: ESLint: Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with the void operator. (@typescript-eslint/no-floating-promises)
[error] 1204-1204: ESLint: Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with the void operator. (@typescript-eslint/no-floating-promises)
[error] 1210-1210: ESLint: Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with the void operator. (@typescript-eslint/no-floating-promises)
[error] 1287-1287: ESLint: Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with the void operator. (@typescript-eslint/no-floating-promises)
[error] 1293-1293: ESLint: Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with the void operator. (@typescript-eslint/no-floating-promises)
[error] 1346-1346: ESLint: Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with the void operator. (@typescript-eslint/no-floating-promises)
[error] 1352-1352: ESLint: Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with the void operator. (@typescript-eslint/no-floating-promises)
[error] 1405-1405: ESLint: Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with the void operator. (@typescript-eslint/no-floating-promises)
[error] 1411-1411: ESLint: Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with the void operator. (@typescript-eslint/no-floating-promises)
⏰ 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). (2)
- GitHub Check: Deploy Preview (watch, 7548/merge, pull_request, 22)
- GitHub Check: build (22)
🔇 Additional comments (6)
apps/watch/src/components/VideoContainerPage/AudioLanguageSelect/AudioLanguageSelect.spec.tsx (2)
248-254: LGTM: removed unused getByText.Good cleanup—keeps the test focused on test IDs and avoids unused variable warnings.
4-6: Verified query and type imports remain correct after refactor. GET_LANGUAGES_SLUG and GetLanguagesSlug still exist and AudioLanguageSelect continues to use them, not the variant query.apps/watch/src/components/VideoContentPage/VideoContentPage.spec.tsx (2)
1-1: Import cleanup LGTMRemoving unused named imports keeps the test lean. No behavior change.
20-22: Keep the module mock; no change neededThe module-level
jest.mockforcookieHandleris still appropriate even after removing the unused alias.apps/watch/src/libs/watchContext/useLanguageActions/useLanguageActions.spec.tsx (1)
1-1: LGTM: correct test utilities and public API usage.Using renderHook from testing-library/react and consuming WatchProvider/WatchInitialState directly is aligned with our testing guidance.
Also applies to: 6-6
apps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.tsx (1)
107-113: Verify htmlFor/id association for accessibility.If LanguageAutocomplete doesn’t forward the id prop to the input, the label’s htmlFor="audio-select" won’t be associated. Ensure the underlying TextField gets id="audio-select" (either via LanguageAutocomplete’s id prop or within renderInput).
…com/JesusFilm/core into 25-00-NC-fix-condense-watch-queries
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 (5)
apps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.spec.tsx (5)
513-547: Update to new language shapes; this test still uses legacy LanguageWithSlug.The spec should no longer rely on
videoAudioLanguagesor nestedLanguageWithSlug. UsevideoAudioLanguagesIdsAndSlugsand a flat{ id, slug }currentAudioLanguage.- videoAudioLanguages: [ - { - __typename: 'LanguageWithSlug' as const, - slug: 'spanish', // Only Spanish available for this video - language: { - __typename: 'Language' as const, - id: '496', - slug: 'spanish', - name: [ - { - __typename: 'LanguageName' as const, - value: 'Spanish', - primary: true - } - ] - } - } - ], - currentAudioLanguage: { - __typename: 'LanguageWithSlug' as const, - slug: 'spanish', // Video is playing in Spanish (not user's preference) - language: { - __typename: 'Language' as const, - id: '496', - slug: 'spanish', - name: [ - { - __typename: 'LanguageName' as const, - value: 'Spanish', - primary: true - } - ] - } - } + videoAudioLanguagesIdsAndSlugs: [{ id: '496', slug: 'spanish' }], + currentAudioLanguage: { id: '496', slug: 'spanish' }
611-714: Turn this into a real interaction test; remove manual setIndexUiState invocation.Right now the test only exercises the mock, not the component. Select an option via the UI and assert the callback.
- // Test the setIndexUiState callback behavior that would be called when handleChange is triggered - const mockPrevState = { - refinementList: { - existingFilter: ['value'] - } - } - - // Simulate what would happen when handleChange is called with a language that has localName - const languageOption = { - id: '496', - localName: 'Spanish', - nativeName: 'Español', - slug: 'spanish' - } - - // This simulates the logic in handleChange function - if (mockInstantSearch && languageOption.localName) { - mockInstantSearch.setIndexUiState((prev: any) => ({ - ...prev, - refinementList: { - ...prev.refinementList, - languageEnglishName: [languageOption.localName ?? ''] - } - })) - } - - // Verify that setIndexUiState was called - expect(mockSetIndexUiState).toHaveBeenCalled() - - // Verify the function passed to setIndexUiState updates the refinementList correctly - const setIndexUiStateCallback = mockSetIndexUiState.mock.calls[0][0] - const result = setIndexUiStateCallback(mockPrevState) - expect(result).toEqual({ - refinementList: { - existingFilter: ['value'], - languageEnglishName: ['Spanish'] - } - }) - - // Verify that the language actions are still called correctly - // Note: In a real scenario, this would be called when the user selects a language - // Here we're testing that the instant search integration doesn't interfere with core functionality - expect(mockUpdateAudioLanguage).not.toHaveBeenCalled() // Since we didn't actually trigger onChange + await userEvent.click(screen.getByRole('combobox')) + await waitFor(() => { + expect( + screen.getByRole('option', { name: 'Spanish' }) + ).toBeInTheDocument() + }) + await userEvent.click(screen.getByRole('option', { name: 'Spanish' })) + + expect(mockSetIndexUiState).toHaveBeenCalled() + const setIndexUiStateCallback = mockSetIndexUiState.mock.calls[0][0] + const mockPrevState = { refinementList: { existingFilter: ['value'] } } + const result = setIndexUiStateCallback(mockPrevState) + expect(result).toEqual({ + refinementList: { + existingFilter: ['value'], + languageEnglishName: ['Spanish'] + } + })
767-771: This likely never triggers onChange; select an option instead of changing the input value.Using fireEvent.change on the combobox input won’t commit a selection, so the “not called” assertion may be a false negative.
- fireEvent.click(screen.getByRole('combobox')) - fireEvent.change(screen.getByRole('combobox'), { - target: { value: '496' } - }) + await userEvent.click(screen.getByRole('combobox')) + await waitFor(() => { + expect( + screen.getByRole('option', { name: 'Spanish' }) + ).toBeInTheDocument() + }) + await userEvent.click(screen.getByRole('option', { name: 'Spanish' }))
1389-1390: Replace legacy videoAudioLanguages with videoAudioLanguagesIdsAndSlugs.Keeps the spec consistent with new state shape.
- videoAudioLanguages: [] + videoAudioLanguagesIdsAndSlugs: []
828-835: Ensure actual option selection and undefined localName in the failing test
Replace the fireEvent calls in the “should not update instant search when language has no localName” test with userEvent clicks on the combobox and on the “Spanish” option:- fireEvent.click(screen.getByRole('combobox')) - fireEvent.change(screen.getByRole('combobox'), { - target: { value: '496' } - }) + await userEvent.click(screen.getByRole('combobox')) + await waitFor(() => { + expect( + screen.getByRole('option', { name: 'Spanish' }) + ).toBeInTheDocument() + }) + await userEvent.click(screen.getByRole('option', { name: 'Spanish' }))Additionally, update your test fixture for id ‘496’ so it only includes
nativeName(e.g.
{ id: '496', nativeName: 'Español', slug: 'spanish' }), omittinglocalNameto guaranteelanguage.localName === undefined.
♻️ Duplicate comments (2)
apps/watch/pages/watch/[part1]/[part2].tsx (1)
156-163: Fix id mismatch: passing Relay id to a databaseId query.
The query uses idType: databaseId, but variables.id is contentData.content.id (Relay node id). Pass databaseId instead (cast to string if needed), or change the query to accept SLUG. This was noted earlier; still unresolved here.Apply:
>({ - query: GET_VARIANT_LANGUAGES_ID_AND_SLUG, - variables: { - id: contentData.content.id - } + query: GET_VARIANT_LANGUAGES_ID_AND_SLUG, + variables: { + id: String(contentData.content.databaseId) + } })Run to confirm VideoContentFields includes databaseId; if missing, extend the fragment:
#!/bin/bash # Find VideoContentFields fragment and show its fields rg -nP -C2 'fragment\s+VideoContentFields\s+on\s+Video' --type=ts --type=tsx --type=graphqlapps/watch/pages/watch/[part1]/[part2]/[part3].tsx (1)
171-177: Fix id mismatch: using Relay id with idType: databaseId.
Same issue here; pass databaseId (cast) or change query idType.Apply:
>({ - query: GET_VARIANT_LANGUAGES_ID_AND_SLUG, - variables: { - id: contentData.content.id - } + query: GET_VARIANT_LANGUAGES_ID_AND_SLUG, + variables: { + id: String(contentData.content.databaseId) + } })
🧹 Nitpick comments (5)
apps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.spec.tsx (5)
1-1: Remove unused GraphQL testing dependency to speed up tests.MockedProvider is unnecessary here (component no longer issues GraphQL queries). Drop the import.
-import { MockedProvider } from '@apollo/client/testing'
83-88: Inline WatchProvider; drop MockedProvider wrapper (repeat across file).Eliminates needless providers and cuts render overhead.
- <MockedProvider mocks={[]} addTypename={false}> - <WatchProvider initialState={defaultInitialState}> - <AudioTrackSelect /> - </WatchProvider> - </MockedProvider> + <WatchProvider initialState={defaultInitialState}> + <AudioTrackSelect /> + </WatchProvider>Apply the same change to all other render() calls in this spec.
59-59: Use userEvent.setup() for reliable async input and timers.Avoids global state and aligns with Testing Library guidance.
describe('AudioTrackSelect', () => { + let user: ReturnType<typeof userEvent.setup> @@ beforeEach(() => { + user = userEvent.setup() useRouterMock.mockReturnValue(mockRouter) useTranslationMock.mockReturnValue({ t: mockT }) useInstantSearchMock.mockReturnValue(undefined) // Default to no instant search jest.clearAllMocks() })Follow-up: replace
await userEvent.click(...)withawait user.click(...)throughout.Also applies to: 74-79
93-93: Avoid querySelector('svg'); assert via accessible query or test id.Tag selectors are brittle. Prefer a stable handle.
- expect(document.querySelector('svg')).toBeInTheDocument() // SpatialAudioOffOutlinedIcon + expect(screen.getByTestId('audio-track-icon')).toBeInTheDocument()Support in component (outside this file):
// AudioTrackSelect.tsx <SpatialAudioOffOutlinedIcon data-testid="audio-track-icon" />
939-941: Prefer userEvent.type over fireEvent.change for typing.Simulates real user input and opens the listbox reliably.
- await fireEvent.change(input, { target: { value: 'Spanish' } }) + await userEvent.type(input, 'Spanish')
📜 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 (5)
apps/watch/pages/watch/[part1]/[part2].tsx(5 hunks)apps/watch/pages/watch/[part1]/[part2]/[part3].tsx(6 hunks)apps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.spec.tsx(15 hunks)apps/watch/src/components/LanguageSwitchDialog/SubtitlesSelect/SubtitlesSelect.tsx(3 hunks)apps/watch/src/libs/watchContext/useSubtitleUpdate/useSubtitleUpdate.spec.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/watch/src/libs/watchContext/useSubtitleUpdate/useSubtitleUpdate.spec.tsx
- apps/watch/src/components/LanguageSwitchDialog/SubtitlesSelect/SubtitlesSelect.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/pages/watch/[part1]/[part2]/[part3].tsxapps/watch/pages/watch/[part1]/[part2].tsxapps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.spec.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/base.mdc)
Define a type if possible.
Files:
apps/watch/pages/watch/[part1]/[part2]/[part3].tsxapps/watch/pages/watch/[part1]/[part2].tsxapps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.spec.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/pages/watch/[part1]/[part2]/[part3].tsxapps/watch/pages/watch/[part1]/[part2].tsxapps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.spec.tsx
🧠 Learnings (3)
📚 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/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.spec.tsx
📚 Learning: 2025-07-27T21:59:04.362Z
Learnt from: CR
PR: JesusFilm/core#0
File: .cursor/rules/apps.mdc:0-0
Timestamp: 2025-07-27T21:59:04.362Z
Learning: Applies to apps/**/*.test.{js,jsx,ts,tsx} : Use 'testing-library/react' npm package for frontend tests.
Applied to files:
apps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.spec.tsx
📚 Learning: 2025-07-15T03:19:49.322Z
Learnt from: Kneesal
PR: JesusFilm/core#7171
File: apps/watch/src/components/LanguageSwitchDialog/SubtitlesSelect/SubtitlesSelect.spec.tsx:447-451
Timestamp: 2025-07-15T03:19:49.322Z
Learning: In the SubtitlesSelect component tests, the team intentionally uses integration tests that verify interactions work without errors rather than explicitly testing function calls, with explanatory comments to inform future developers of this testing strategy.
Applied to files:
apps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.spec.tsx
🧬 Code graph analysis (2)
apps/watch/pages/watch/[part1]/[part2]/[part3].tsx (3)
apps/watch/src/libs/watchContext/WatchContext.tsx (1)
AudioLanguageData(15-18)apps/watch/__generated__/GetVariantLanguagesIdAndSlug.ts (2)
GetVariantLanguagesIdAndSlug(27-29)GetVariantLanguagesIdAndSlugVariables(31-33)apps/watch/src/libs/useVariantLanguagesIdAndSlugQuery/useVariantLanguagesIdAndSlugQuery.ts (1)
GET_VARIANT_LANGUAGES_ID_AND_SLUG(8-20)
apps/watch/pages/watch/[part1]/[part2].tsx (3)
apps/watch/src/libs/watchContext/WatchContext.tsx (1)
AudioLanguageData(15-18)apps/watch/__generated__/GetVariantLanguagesIdAndSlug.ts (2)
GetVariantLanguagesIdAndSlug(27-29)GetVariantLanguagesIdAndSlugVariables(31-33)apps/watch/src/libs/useVariantLanguagesIdAndSlugQuery/useVariantLanguagesIdAndSlugQuery.ts (1)
GET_VARIANT_LANGUAGES_ID_AND_SLUG(8-20)
⏰ 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). (5)
- GitHub Check: Deploy Preview (watch, 7548/merge, pull_request, 22)
- GitHub Check: test (22, 3/3)
- GitHub Check: build (22)
- GitHub Check: lint (22)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (17)
apps/watch/pages/watch/[part1]/[part2].tsx (7)
10-12: LGTM: correct generated type imports added.
26-26: LGTM: using central GET_VARIANT_LANGUAGES_ID_AND_SLUG.
46-48: Props renamed to new shapes match WatchContext AudioLanguageData/string[] model.
66-70: Component props updated consistently.
80-82: Initial watch state wired to new props.
152-154: Good: pre-initialize arrays to [].
181-182: LGTM: props return matches new names.apps/watch/pages/watch/[part1]/[part2]/[part3].tsx (9)
12-14: LGTM: correct generated type imports added.
33-33: LGTM: central query import.
37-40: LGTM: AudioLanguageData import aligns with WatchContext.
63-65: Props updated to new shapes.
70-72: Component destructuring consistent.
84-86: Initial watch state wired to new fields.
167-168: Good: arrays initialized.
179-188: LGTM: mappings with nullish fallback prevent undefined.
196-197: LGTM: returned props align with new names.apps/watch/src/components/LanguageSwitchDialog/AudioTrackSelect/AudioTrackSelect.spec.tsx (1)
265-269: LGTM: Consistent migration to videoAudioLanguagesIdsAndSlugs.Tests correctly use the new { id, slug } shape in most places and validate reload semantics.
Also applies to: 450-459, 1006-1014, 1183-1188, 1262-1270, 1330-1331
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests