Tests + subtitle enhancements#16
Conversation
WalkthroughAdds many new UI components, hooks, contexts, tests, and Storybook stories across the player codebase. Notable additions: PlayerErrorBoundary; GestureOverlay and gesture wiring in VideoPlayer; SubtitleDisplay and subtitle-related controls/hooks; PauseAd, RewardedAdOverlay, and ad-related controls; Equalizer, SleepTimer, ShareButton, SubtitleSettings, and related hooks (useABLoop, useEqualizer, useAutoplayDetection, useFocusTrap, etc.); ThumbnailPreview and ProgressBar feature extensions; FairuProvider composition; TrackingProvider refactor to use TrackingService; extensive Vitest + RTL test suites; story/demo updates; and package.json bundle-size CI config and devDependencies. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (23)
src/components/VideoPlayer/LogoOverlay/LogoOverlay.test.tsx-192-200 (1)
192-200:⚠️ Potential issue | 🟡 MinorAssert default prevention explicitly for the link-click test.
On Line 192, the test name says default is prevented, but the body only asserts
onClickwas called.✅ Suggested test fix
it('calls onClick and prevents default when both href and onClick are set', () => { const onClick = vi.fn(); const { container } = render( <LogoOverlay config={createLogoConfig({ href: 'https://example.com', onClick })} /> ); const link = container.querySelector('a')!; - fireEvent.click(link); + const defaultNotPrevented = fireEvent.click(link); expect(onClick).toHaveBeenCalledTimes(1); + expect(defaultNotPrevented).toBe(false); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/VideoPlayer/LogoOverlay/LogoOverlay.test.tsx` around lines 192 - 200, Update the test in LogoOverlay.test.tsx that uses LogoOverlay and createLogoConfig so it actually verifies default prevention: either spy on Event.prototype.preventDefault before calling fireEvent.click(link) (e.g., vi.spyOn(Event.prototype, 'preventDefault') and assert it was called) or have the onClick mock call e.preventDefault() and then assert the received event has defaultPrevented true (inspect onClick.mock.calls[0][0].defaultPrevented); ensure the test still asserts onClick was called and add the explicit expectation that default prevention occurred.src/components/markers/MarkerList/MarkerList.test.tsx-98-117 (1)
98-117:⚠️ Potential issue | 🟡 MinorAssert single invocation in click-handler tests.
At Line 102, Line 109, and Line 116 you only check arguments; these tests still pass if
onMarkerClickfires multiple times per click. AddtoHaveBeenCalledTimes(1)to catch duplicate-callback regressions.Proposed test hardening
it('calls onMarkerClick with marker and index when clicked', () => { const onMarkerClick = vi.fn(); renderMarkerList({ onMarkerClick }); fireEvent.click(screen.getByText('Highlight 2')); + expect(onMarkerClick).toHaveBeenCalledTimes(1); expect(onMarkerClick).toHaveBeenCalledWith(markers[1], 1); }); it('calls onMarkerClick for first marker', () => { const onMarkerClick = vi.fn(); renderMarkerList({ onMarkerClick }); fireEvent.click(screen.getByText('Highlight 1')); + expect(onMarkerClick).toHaveBeenCalledTimes(1); expect(onMarkerClick).toHaveBeenCalledWith(markers[0], 0); }); it('calls onMarkerClick for last marker', () => { const onMarkerClick = vi.fn(); renderMarkerList({ onMarkerClick }); fireEvent.click(screen.getByText('Highlight 3')); + expect(onMarkerClick).toHaveBeenCalledTimes(1); expect(onMarkerClick).toHaveBeenCalledWith(markers[2], 2); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/markers/MarkerList/MarkerList.test.tsx` around lines 98 - 117, The click-handler tests for MarkerList (tests named "calls onMarkerClick with marker and index when clicked", "calls onMarkerClick for first marker", and "calls onMarkerClick for last marker") only assert the arguments passed to the mock onMarkerClick and not the invocation count; update each test that calls renderMarkerList({ onMarkerClick }) and fireEvent.click(screen.getByText(...)) to also assert onMarkerClick has been called exactly once by adding expect(onMarkerClick).toHaveBeenCalledTimes(1) after the existing toHaveBeenCalledWith checks to prevent regressions where the handler might be invoked multiple times.src/components/a11y/ScreenReaderAnnouncer.tsx-21-23 (1)
21-23:⚠️ Potential issue | 🟡 MinorClear stale announcement when message becomes empty.
Line [22] returns early for empty messages, so previous announcement text can remain rendered.
💡 Proposed fix
useEffect(() => { - if (!message) return; + if (!message) { + setAnnouncement(''); + return; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/a11y/ScreenReaderAnnouncer.tsx` around lines 21 - 23, The useEffect currently returns early when message is falsy, leaving the previous announcement rendered; update the effect in ScreenReaderAnnouncer so it explicitly clears the rendered announcement when message becomes empty (e.g., in the useEffect watching message, handle the falsy branch by calling the component's state setter or clearing the announcementRef/textContent — reference useEffect, message, and whichever state/ref you use such as setAnnouncement or announcementRef — so the aria-live node is cleared when message is empty).src/components/VideoPlayer/EndScreen/EndScreen.test.tsx-283-294 (1)
283-294:⚠️ Potential issue | 🟡 MinorUse a countdown-specific assertion after cancel.
"Up next"is shared UI copy here, so its absence doesn’t uniquely prove the countdown was removed. Assert on the countdown control/root instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/VideoPlayer/EndScreen/EndScreen.test.tsx` around lines 283 - 294, The test currently asserts the absence of the shared "Up next" copy which is ambiguous; after clicking Cancel in the EndScreen render (created via createEndScreenConfig and rendered by EndScreen), replace the final assertion so it specifically targets the autoplay countdown element/root (e.g., use screen.queryByRole('timer') or screen.queryByTestId('autoplay-countdown' or 'countdown-root') depending on what the component exposes) and assert that that countdown node is not in the document instead of checking for "Up next".src/components/VideoPlayer/EndScreen/EndScreen.test.tsx-2-2 (1)
2-2:⚠️ Potential issue | 🟡 MinorDrop the unused
waitForimport.It isn’t used anywhere in this file, and it’s already failing the test job.
🧹 Suggested cleanup
-import { render, screen, fireEvent, act, waitFor } from '@testing-library/react'; +import { render, screen, fireEvent, act } from '@testing-library/react';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/VideoPlayer/EndScreen/EndScreen.test.tsx` at line 2, The import list in the test file includes an unused symbol `waitFor`; remove `waitFor` from the import statement that currently imports from '@testing-library/react' (the line importing render, screen, fireEvent, act, waitFor) so the file only imports the utilities actually used (render, screen, fireEvent, act) to resolve the failing test job due to the unused import.src/components/VideoPlayer/EndScreen/EndScreen.test.tsx-110-137 (1)
110-137:⚠️ Potential issue | 🟡 MinorSet
showReplay: trueexplicitly in the replay tests.Right now these cases depend on the helper default, so they can still pass if the
showReplaygate is accidentally ignored.✅ Suggested change
<EndScreen - config={createEndScreenConfig()} + config={createEndScreenConfig({ showReplay: true })} currentTime={300} duration={300} isEnded={true} onReplay={onReplay} /> @@ <EndScreen - config={createEndScreenConfig()} + config={createEndScreenConfig({ showReplay: true })} currentTime={300} duration={300} isEnded={true} onReplay={onReplay} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/VideoPlayer/EndScreen/EndScreen.test.tsx` around lines 110 - 137, The replay tests rely on the helper's default; explicitly set showReplay: true in the config passed to EndScreen in both test cases to ensure the replay gate is tested: update the render calls in the "renders replay button when showReplay is true and onReplay is provided" and "calls onReplay when replay button is clicked" tests to pass createEndScreenConfig({ showReplay: true }) (or otherwise set showReplay: true on the config object) so the component's showReplay guard is actually exercised when rendering and when clicking the Replay button.src/components/controls/Equalizer/Equalizer.tsx-54-69 (1)
54-69:⚠️ Potential issue | 🟡 MinorAdd semantic attributes to the toggle switch for better accessibility.
The toggle button should include
role="switch"andaria-checkedto properly communicate its state to assistive technologies.♿ Proposed fix for accessibility
<button type="button" onClick={() => onEnabledChange(!enabled)} + role="switch" + aria-checked={enabled} className={cn( 'relative w-9 h-5 rounded-full transition-colors duration-200', enabled ? 'bg-[var(--fp-color-accent)]' : 'bg-[var(--fp-progress-bg)]' )} aria-label={enabled ? 'Disable equalizer' : 'Enable equalizer'} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/controls/Equalizer/Equalizer.tsx` around lines 54 - 69, The toggle button in the Equalizer component lacks semantic switch attributes; update the button element (the one using onEnabledChange and the enabled prop) to include role="switch" and aria-checked={enabled} so assistive tech can detect its state (keep the existing aria-label and onClick behavior intact).src/hooks/useABLoop.ts-81-96 (1)
81-96:⚠️ Potential issue | 🟡 MinorVery short loops only run once.
After seeking back to A, the guard only resets when
currentTime < loopEnd - 0.15. IfloopStartis within 150 ms ofloopEnd, that condition is impossible after the seek, so the loop never re-arms. Re-arm when playback lands back near the seek target instead of comparing againstloopEnd - 0.15.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useABLoop.ts` around lines 81 - 96, The justSeekedRef guard currently waits for currentTime < loopEnd - 0.15 to clear (inside the useEffect handling loop re-arming), which fails when loopStart is within 150ms of loopEnd; change the re-arm condition to detect when playback has returned near the seek target instead (e.g., check currentTime is close to loopStart within a small tolerance like 0.15s) so justSeekedRef.current is set to false when currentTime <= loopStart + tolerance (or otherwise within tolerance of loopStart) and keep the rest of the logic (onSeek(loopStart), currentTime >= loopEnd) intact.src/components/controls/ShareButton/ShareButton.stories.tsx-47-58 (1)
47-58:⚠️ Potential issue | 🟡 MinorDon't report clipboard failures as success.
The interactive story returns
trueeven whenwriteText()throws or the Clipboard API is unavailable. That hides the failure path and documents the wrong contract forcopyShareUrl.💡 Suggested change
const copyShareUrl = async (time?: number) => { const url = getShareUrl(time); if (typeof navigator !== 'undefined' && navigator.clipboard) { try { await navigator.clipboard.writeText(url); return true; } catch { - return true; + return false; } } - return true; + return false; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/controls/ShareButton/ShareButton.stories.tsx` around lines 47 - 58, The helper copyShareUrl currently always returns true even when the Clipboard API is missing or navigator.clipboard.writeText() throws; update copyShareUrl to return true only on a successful write and return false (or propagate a rejection) when clipboard is unavailable or writeText fails. Locate the copyShareUrl function (and its call to getShareUrl) and modify the try/catch and the navigator.clipboard availability check so that failures return false (or rethrow) instead of returning true, ensuring the promise resolves to false on failure.src/hooks/useABLoop.ts-46-72 (1)
46-72:⚠️ Potential issue | 🟡 MinorAvoid calling state setters inside updater functions.
Lines 53 and 67 call state setters (
setLoopStartandsetLoopEnd) from within updater functions. Updater functions should be pure—React invokes them twice in Strict Mode to detect impurities, so these nested setter calls execute twice per state update, potentially enqueueing duplicate swaps. Instead, compute both the new A and B values based on the previous state within a single updater, then apply both changes atomically using a reducer or by restructuring the state updates to avoid nested side effects.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useABLoop.ts` around lines 46 - 72, The updater functions setA and setB call other state setters inside their updater callbacks (setLoopStart/setLoopEnd), which is impure and can run twice in Strict Mode; fix by updating both start and end atomically instead of nesting setters: combine loopStart and loopEnd into a single piece of state (or switch to useReducer) and implement a single updater (e.g., setLoops or reducer action) that receives prev {start,end}, computes newStart/newEnd (perform the swap logic there when t < prev.start or prev.end < t) and returns the new state so both values are set together without calling one setter from inside another; update references in useABLoop for functions setA and setB to dispatch the new atomic updater.src/hooks/useEqualizer.ts-100-104 (1)
100-104:⚠️ Potential issue | 🟡 MinorStale source node if media element changes.
MediaElementAudioSourceNodecan only be created once perHTMLMediaElement. IfmediaRef.currentchanges to a different element,sourceRef.currentstill references the old source node, causing the new element to have no EQ applied.🛡️ Suggested fix: track the connected element
+ const connectedElementRef = useRef<HTMLMediaElement | null>(null); // Create source (only once per element) - if (!sourceRef.current) { + if (!sourceRef.current || connectedElementRef.current !== media) { + if (sourceRef.current) { + sourceRef.current.disconnect(); + } sourceRef.current = ctx.createMediaElementSource(media); + connectedElementRef.current = media; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useEqualizer.ts` around lines 100 - 104, The sourceRef.current is stale when the media element changes; update useEqualizer to track which HTMLMediaElement is connected (e.g., store a connectedElementRef) and when media (media or mediaRef.current) differs, disconnect and/or release the old MediaElementAudioSourceNode and create a new one via ctx.createMediaElementSource(media), setting sourceRef.current to the new node and updating the connected element ref; ensure you call sourceRef.current.disconnect() (if present) before replacing it so the old node is not left connected.src/components/playlist/TrackItem/TrackItem.test.tsx-7-7 (1)
7-7:⚠️ Potential issue | 🟡 MinorUnused import:
Tracktype.The
Tracktype is imported but never used in this file. This causes TypeScript compilation errors in CI.🔧 Remove unused import
import { LabelsProvider } from '@/context/LabelsContext'; import type { ReactNode } from 'react'; -import type { Track } from '@/types/player';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/playlist/TrackItem/TrackItem.test.tsx` at line 7, Remove the unused type import by deleting the line that imports Track (the "import type { Track } from '@/types/player';" statement) from TrackItem.test.tsx so the file no longer references the unused symbol Track and TypeScript CI errors are resolved; alternatively, if the test should use the type, update the test to reference Track where appropriate (e.g., annotate mock data) instead of leaving the import unused.src/hooks/useCast.test.ts-314-329 (1)
314-329:⚠️ Potential issue | 🟡 MinorAssert that unmount removes the same listener instance.
Using
expect.any(Function)here allows a broken cleanup to pass even ifremoveEventListenerreceives a different callback than the one that was registered. Capture the handler fromaddEventListenerand assert that exact reference is removed.🔍 Stronger cleanup assertions
it('should remove remote event listeners on unmount', () => { const { video, remote } = createVideoWithRemote(); const ref = { current: video }; const { unmount } = renderHook(() => useCast(ref)); + const connectHandler = remote.addEventListener.mock.calls.find( + ([event]: [string, Function]) => event === 'connect', + )?.[1]; + const disconnectHandler = remote.addEventListener.mock.calls.find( + ([event]: [string, Function]) => event === 'disconnect', + )?.[1]; unmount(); - expect(remote.removeEventListener).toHaveBeenCalledWith( - 'connect', - expect.any(Function), - ); - expect(remote.removeEventListener).toHaveBeenCalledWith( - 'disconnect', - expect.any(Function), - ); + expect(remote.removeEventListener).toHaveBeenCalledWith('connect', connectHandler); + expect(remote.removeEventListener).toHaveBeenCalledWith('disconnect', disconnectHandler); }); it('should remove AirPlay listener on unmount', () => { const video = createVideoWithAirPlay(); + const addSpy = vi.spyOn(video, 'addEventListener'); const removeSpy = vi.spyOn(video, 'removeEventListener'); const ref = { current: video }; const { unmount } = renderHook(() => useCast(ref)); + const airPlayHandler = addSpy.mock.calls.find( + ([event]: [string, EventListener]) => + event === 'webkitcurrentplaybacktargetiswirelesschanged', + )?.[1]; unmount(); - expect(removeSpy).toHaveBeenCalledWith( - 'webkitcurrentplaybacktargetiswirelesschanged', - expect.any(Function), - ); + expect(removeSpy).toHaveBeenCalledWith( + 'webkitcurrentplaybacktargetiswirelesschanged', + airPlayHandler, + ); });Also applies to: 388-400
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useCast.test.ts` around lines 314 - 329, The test currently asserts remote.removeEventListener was called with expect.any(Function), which can miss mismatched handler instances; modify the test in useCast.test.ts to capture the actual handler passed to remote.addEventListener (e.g., spy on remote.addEventListener and save the callback for the 'connect' and 'disconnect' events from createVideoWithRemote), then assert remote.removeEventListener was called with the exact same handler reference for both events (use strict equality checks), and apply the same change to the similar assertions around the other block referenced (lines ~388-400) so both 'connect' and 'disconnect' removals validate the exact callback instance.src/hooks/useCast.test.ts-61-87 (1)
61-87:⚠️ Potential issue | 🟡 MinorProtect the prototype mutation with
try/finally.This test mutates
HTMLVideoElement.prototype.remoteand restores it only on the happy path. IfrenderHook,expect, orunmountthrows, the polluted prototype leaks into later tests.🧪 Safer cleanup pattern
const ref = { current: document.createElement('video') }; - const { result, unmount } = renderHook(() => useCast(ref)); - expect(result.current.isSupported).toBe(true); - - // Unmount before restoring, so the cleanup can still access video.remote - unmount(); - - // Restore to avoid polluting other tests - if (originalDescriptor) { - Object.defineProperty(HTMLVideoElement.prototype, 'remote', originalDescriptor); - } else { - delete (HTMLVideoElement.prototype as Record<string, unknown>).remote; - } + const { result, unmount } = renderHook(() => useCast(ref)); + + try { + expect(result.current.isSupported).toBe(true); + } finally { + unmount(); + if (originalDescriptor) { + Object.defineProperty(HTMLVideoElement.prototype, 'remote', originalDescriptor); + } else { + delete (HTMLVideoElement.prototype as Record<string, unknown>).remote; + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useCast.test.ts` around lines 61 - 87, The test mutates HTMLVideoElement.prototype.remote without guaranteeing restoration on failures; wrap the prototype override and the test operations (creating ref, calling renderHook/useCast, assertions, and unmount) inside a try/finally so that in finally you always restore the originalDescriptor (or delete the property) using the same logic currently present; reference the test block that uses createRemoteMock(), Object.defineProperty(HTMLVideoElement.prototype, 'remote', ...), renderHook(() => useCast(ref)), expect(result.current.isSupported) and unmount() and move the restoration into the finally to ensure cleanup runs even if renderHook/expect/unmount throws.src/components/controls/VolumeControl/VolumeControl.test.tsx-412-425 (1)
412-425:⚠️ Potential issue | 🟡 MinorDrop the unused
containerbindings.Line 413 and Line 421 destructure
containerbut never read it, and that is already failing thetestcheck.🧹 Minimal fix
- const { container } = renderVolumeControl({ orientation: 'horizontal', volume: 0.6, muted: false }); + renderVolumeControl({ orientation: 'horizontal', volume: 0.6, muted: false }); const slider = screen.getByRole('slider'); @@ - const { container } = renderVolumeControl({ orientation: 'horizontal', volume: 0.6, muted: true }); + renderVolumeControl({ orientation: 'horizontal', volume: 0.6, muted: true }); const slider = screen.getByRole('slider');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/controls/VolumeControl/VolumeControl.test.tsx` around lines 412 - 425, The tests renderVolumeControl(...) are destructuring an unused container variable which fails lint/tests; update both tests ("shows correct volume fill width (horizontal)" and "shows 0% fill when muted (horizontal)") to call renderVolumeControl({ orientation: 'horizontal', volume: ..., muted: ... }) without destructuring container (or only destructure what you actually use), so remove "const { container } =" and keep the rest of the test logic using screen.getByRole and slider.querySelector as-is.src/components/controls/ProgressBar/ProgressBar.tsx-337-344 (1)
337-344:⚠️ Potential issue | 🟡 MinorNormalize the A/B bounds before computing the overlay.
If callers pass
loopStart > loopEnd, the computed width goes negative and the highlight disappears. The new interactive story can get into that state by setting B before A, so this branch should sort/clamp the endpoints before converting them to percentages.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/controls/ProgressBar/ProgressBar.tsx` around lines 337 - 344, The overlay calculation in ProgressBar (using loopStart, loopEnd, and duration) can produce negative widths when loopStart > loopEnd; normalize and clamp the endpoints first (e.g., compute start = Math.max(0, Math.min(loopStart, loopEnd)), end = Math.min(duration, Math.max(loopStart, loopEnd))) or equivalent, then compute left and width from those normalized values so left = (start/duration)*100% and width = ((end-start)/duration)*100%, ensuring width is never negative or extends beyond bounds.src/components/controls/ProgressBar/ProgressBar.tsx-369-375 (1)
369-375:⚠️ Potential issue | 🟡 MinorRoute the new chapter strings through the labels system.
Go to chapter: …andChapter {n}are hardcoded English strings even though this component already resolves labels from context. That makes the new marker buttons and tooltip content non-localizable in non-English embeds.Also applies to: 475-485
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/controls/ProgressBar/ProgressBar.tsx` around lines 369 - 375, The hardcoded strings "Go to chapter: …" and "Chapter {n}" in ProgressBar's marker buttons and tooltips bypass the app's labels system; update the JSX in the ProgressBar component (the button with key={chapter.id} and the related tooltip code around the 475-485 area) to retrieve localized strings from the existing labels context/resolver used elsewhere in this component (e.g., call the same labels.resolve/labels.get helper or prop the component uses) and format the chapter title/number into that localized template for aria-label, title and visible label text instead of concatenating English literals.src/components/controls/ProgressBar/ProgressBar.tsx-347-362 (1)
347-362:⚠️ Potential issue | 🟡 MinorRender the chapter segments behind the active fills.
These are labeled as background segments, but they’re appended after buffered progress, current progress, and the A/B overlay, so they actually paint on top of those states and mute them. Moving them earlier in the stack or giving them a lower z-index keeps them decorative.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/controls/ProgressBar/ProgressBar.tsx` around lines 347 - 362, The chapter segment divs rendered in the chapterSegments.map are currently painted on top of buffered/current fills and the A/B overlay; move this block so it renders before the buffered/current progress and AB overlay elements in ProgressBar.tsx or change the segment divs to a lower stacking context (e.g., add a class like 'z-0 pointer-events-none') while ensuring the progress fill elements (buffered fill, current progress, AB overlay) have higher z-index (e.g., 'z-10' or greater). Locate the chapterSegments mapping in ProgressBar.tsx and either reposition it above the buffered/current fill and AB overlay JSX or adjust the classes on both the segment divs and the fill/overlay elements to guarantee the segments stay visually behind.src/components/Player/Player.stories.tsx-383-383 (1)
383-383:⚠️ Potential issue | 🟡 MinorLanguage consistency is still mixed in this section.
Line 383 is now English, but nearby user-facing labels remain German. Consider fully aligning the block to one language for docs/story clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Player/Player.stories.tsx` at line 383, The header "Available Features:" in Player.stories.tsx is in English while surrounding user-facing labels remain German; pick one language and make the entire block consistent—either translate this h4 text ("Available Features:") to the German equivalent used elsewhere or translate neighboring German labels to English; locate the string "Available Features:" in the Player.stories.tsx story block and update it so all user-facing labels within that story (the h4 and nearby labels) use the same language.src/components/VideoPlayer/VideoPlayer.tsx-742-767 (1)
742-767:⚠️ Potential issue | 🟡 MinorStale
wrappedControlsreference in gesture callbacks.
wrappedControls(defined at lines 706-722) is recreated on every render since it's an object literal. This causes all gesture callbacks that depend on it to be recreated, potentially causing issues withuseGesturesif it uses referential equality checks.🔧 Memoize wrappedControls
- // Wrapped controls for ad integration - const wrappedControls = { + // Wrapped controls for ad integration + const wrappedControls = useMemo(() => ({ ...controls, play: async () => { if (onPlayWithAds) { onPlayWithAds(); } else { await controls.play(); } }, toggle: async () => { if (!state.isPlaying && onPlayWithAds) { onPlayWithAds(); } else { await controls.toggle(); } }, - }; + }), [controls, onPlayWithAds, state.isPlaying]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/VideoPlayer/VideoPlayer.tsx` around lines 742 - 767, The gesture callbacks (onDoubleTapLeft, onDoubleTapRight, onSwipeUp, onSwipeDown, onSwipeLeft, onSwipeRight) capture a freshly created wrappedControls object each render because wrappedControls is created as an object literal; wrap wrappedControls creation in useMemo so its identity is stable (e.g., const wrappedControls = useMemo(() => ({ skipForward: ..., skipBackward: ..., setVolume: ... }), [the actual underlying control function refs]), and then update the useCallback dependency arrays to depend on the memoized wrappedControls (or better, on the individual stable functions inside it) so the gesture handlers do not recreate unnecessarily; ensure you import/use useMemo and include only the true changing dependencies (underlying control function refs and state.volume where needed).src/components/ads/PauseAd/PauseAd.stories.tsx-95-110 (1)
95-110:⚠️ Potential issue | 🟡 MinorClear the delayed show timer when playback resumes.
The timeout id is scoped to
handlePause, sohandlePlayhas no way to cancel it. A quick Pause → Play will still fire the 500ms callback and reopen the ad over active playback. Keep the timer in a ref and clear it when playback resumes and on unmount.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ads/PauseAd/PauseAd.stories.tsx` around lines 95 - 110, The pause timer in handlePause is local and cannot be cancelled by handlePlay; move the timeout id into a ref (e.g., pauseTimerRef) so handlePause assigns pauseTimerRef.current = setTimeout(...), have handlePlay clearTimeout(pauseTimerRef.current) and set pauseTimerRef.current = null before hiding the ad, and add a useEffect cleanup that clears pauseTimerRef.current on unmount to avoid the delayed callback reopening the ad after resume.src/components/VideoPlayer/VideoPlayer.stories.tsx-588-589 (1)
588-589:⚠️ Potential issue | 🟡 MinorThe compliance subtitle mode state doesn't match what this story can render.
subtitleModestarts as'overlay'and the copy says overlay/below are supported, but this story always rendersmode="below"and the toggle only exposes below/off. On first render the subtitles are visible with no active control. Either add the missing overlay branch or narrow the state/copy to the two modes this story actually supports.Also applies to: 635-635, 660-683
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/VideoPlayer/VideoPlayer.stories.tsx` around lines 588 - 589, The story's subtitleMode state and copy are inconsistent with the UI (the story only renders mode="below" and a toggle for below/off), so update the state and copy to match or add the missing overlay rendering: either (A) restrict subtitleMode's type and default to only 'below'|'off' by changing the useState declaration (subtitleMode, setSubtitleMode) and DEFAULT_SUBTITLE_STYLE usage to default to 'below', and update any copy/text that mentions "overlay" to only mention "below/off"; or (B) implement the overlay branch where the component is rendered with mode="overlay" and ensure the toggle/control exposes overlay/below/off and the rendering logic (mode prop and subtitle rendering section referenced in this story) supports overlay styling—make corresponding updates to the subtitleMode state, the toggle handlers (setSubtitleMode), and the displayed helper copy so they stay consistent across the other occurrences noted.src/components/ErrorBoundary/PlayerErrorBoundary.tsx-84-85 (1)
84-85:⚠️ Potential issue | 🟡 MinorSet explicit button type in fallback actions
Line 84 and Line 151 should set
type="button"to avoid accidental form submission when this boundary is used inside a<form>.Suggested fix
<button + type="button" onClick={this.reset} className={cn( @@ <button + type="button" onClick={this.reset} className={cn(Also applies to: 151-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ErrorBoundary/PlayerErrorBoundary.tsx` around lines 84 - 85, The fallback buttons in the PlayerErrorBoundary component can submit enclosing forms because they lack an explicit type; update the button elements rendered in the fallback UI (the one that calls this.reset and the other fallback action button) to include type="button" so clicks don't trigger form submissions—locate these in the PlayerErrorBoundary render fallback block and add type="button" to both buttons that invoke this.reset and the secondary action.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 876b280d-7d21-4e56-8e42-f82c69689f84
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (142)
package.jsonsrc/components/ErrorBoundary/PlayerErrorBoundary.stories.tsxsrc/components/ErrorBoundary/PlayerErrorBoundary.tsxsrc/components/ErrorBoundary/index.tssrc/components/Player/EpisodeView/EpisodeView.stories.tsxsrc/components/Player/NowPlayingView/NowPlayingView.stories.tsxsrc/components/Player/Player.stories.tsxsrc/components/Player/Player.tsxsrc/components/VideoPlayer/EndScreen/EndScreen.test.tsxsrc/components/VideoPlayer/GestureOverlay/GestureOverlay.tsxsrc/components/VideoPlayer/GestureOverlay/index.tssrc/components/VideoPlayer/LogoOverlay/LogoOverlay.test.tsxsrc/components/VideoPlayer/SubtitleDisplay/SubtitleDisplay.stories.tsxsrc/components/VideoPlayer/SubtitleDisplay/SubtitleDisplay.test.tsxsrc/components/VideoPlayer/SubtitleDisplay/SubtitleDisplay.tsxsrc/components/VideoPlayer/SubtitleDisplay/index.tssrc/components/VideoPlayer/VideoPlayer.stories.tsxsrc/components/VideoPlayer/VideoPlayer.tsxsrc/components/VideoPlayer/index.tssrc/components/a11y/ScreenReaderAnnouncer.tsxsrc/components/a11y/index.tssrc/components/ads/AdOverlay/AdOverlay.stories.tsxsrc/components/ads/AdOverlay/AdOverlay.test.tsxsrc/components/ads/AdSkipButton/AdSkipButton.test.tsxsrc/components/ads/PauseAd/PauseAd.stories.tsxsrc/components/ads/PauseAd/PauseAd.test.tsxsrc/components/ads/PauseAd/PauseAd.tsxsrc/components/ads/PauseAd/index.tssrc/components/ads/RewardedAd/RewardedAd.stories.tsxsrc/components/ads/RewardedAd/RewardedAd.test.tsxsrc/components/ads/RewardedAd/RewardedAd.tsxsrc/components/ads/RewardedAd/index.tssrc/components/ads/index.tssrc/components/chapters/ChapterList/ChapterList.test.tsxsrc/components/chapters/ChapterMarker/ChapterMarker.test.tsxsrc/components/controls/CastButton/CastButton.test.tsxsrc/components/controls/Equalizer/Equalizer.stories.tsxsrc/components/controls/Equalizer/Equalizer.tsxsrc/components/controls/Equalizer/index.tssrc/components/controls/FullscreenButton/FullscreenButton.test.tsxsrc/components/controls/NowPlayingIndicator/NowPlayingIndicator.test.tsxsrc/components/controls/PictureInPictureButton/PictureInPictureButton.test.tsxsrc/components/controls/PlayButton/PlayButton.test.tsxsrc/components/controls/PlaybackSpeed/PlaybackSpeed.test.tsxsrc/components/controls/ProgressBar/ProgressBar.stories.tsxsrc/components/controls/ProgressBar/ProgressBar.test.tsxsrc/components/controls/ProgressBar/ProgressBar.tsxsrc/components/controls/ProgressBar/ThumbnailPreview.tsxsrc/components/controls/ShareButton/ShareButton.stories.tsxsrc/components/controls/ShareButton/ShareButton.tsxsrc/components/controls/ShareButton/index.tssrc/components/controls/SkipButtons/SkipButtons.test.tsxsrc/components/controls/SleepTimer/SleepTimer.tsxsrc/components/controls/SleepTimer/index.tssrc/components/controls/SubtitleSelector/SubtitleSelector.test.tsxsrc/components/controls/SubtitleSettings/SubtitleSettings.stories.tsxsrc/components/controls/SubtitleSettings/SubtitleSettings.tsxsrc/components/controls/SubtitleSettings/index.tssrc/components/controls/TimeDisplay/TimeDisplay.test.tsxsrc/components/controls/VolumeControl/VolumeControl.test.tsxsrc/components/controls/index.tssrc/components/markers/MarkerList/MarkerList.test.tsxsrc/components/playlist/PlaylistView/PlaylistView.stories.tsxsrc/components/playlist/PlaylistView/PlaylistView.test.tsxsrc/components/playlist/TrackItem/TrackItem.test.tsxsrc/components/podcast/PodcastPage.stories.tsxsrc/components/stats/Rating.stories.tsxsrc/components/stats/Rating.test.tsxsrc/components/stats/Stats.stories.tsxsrc/components/stats/Stats.test.tsxsrc/context/AdContext.test.tsxsrc/context/FairuProvider.tsxsrc/context/LabelsContext.test.tsxsrc/context/PlayerContext.test.tsxsrc/context/TrackingContext.test.tsxsrc/context/TrackingContext.tsxsrc/context/VideoAdContext.test.tsxsrc/context/VideoContext.test.tsxsrc/context/index.tssrc/embed/parseConfig.test.tssrc/examples/NewFeaturesDemo.stories.tsxsrc/hooks/index.tssrc/hooks/useABLoop.tssrc/hooks/useAudio.test.tssrc/hooks/useAutoplayDetection.tssrc/hooks/useCast.test.tssrc/hooks/useChapters.test.tssrc/hooks/useEqualizer.tssrc/hooks/useFocusTrap.tssrc/hooks/useFullscreen.test.tssrc/hooks/useGestures.tssrc/hooks/useHLS.test.tssrc/hooks/useKeyboardControls.test.tssrc/hooks/useKeyboardControls.tssrc/hooks/useMarkers.test.tssrc/hooks/useMedia.test.tssrc/hooks/useMedia.tssrc/hooks/usePauseAd.test.tssrc/hooks/usePauseAd.tssrc/hooks/usePictureInPicture.test.tssrc/hooks/usePlaybackHistory.tssrc/hooks/usePlayer.test.tsxsrc/hooks/usePlaylist.test.tssrc/hooks/usePlaylistPersistence.tssrc/hooks/useResumePosition.tssrc/hooks/useRewardedAd.test.tssrc/hooks/useRewardedAd.tssrc/hooks/useShareableTimestamp.tssrc/hooks/useSleepTimer.tssrc/hooks/useSubtitleParser.test.tssrc/hooks/useSubtitleParser.tssrc/hooks/useSubtitleStyling.tssrc/hooks/useSyncPlayback.tssrc/hooks/useTabVisibility.test.tssrc/hooks/useVideo.test.tssrc/index.tssrc/services/AdService.test.tssrc/services/SyncService.tssrc/services/TrackingService.test.tssrc/services/TrackingService.tssrc/services/index.tssrc/test/helpers.tsxsrc/test/setup.tssrc/types/abloop.tssrc/types/equalizer.tssrc/types/history.tssrc/types/index.tssrc/types/media.tssrc/types/pauseAd.tssrc/types/playlistPersistence.tssrc/types/resume.tssrc/types/rewardedAd.tssrc/types/sleepTimer.tssrc/types/subtitleStyling.tssrc/types/sync.tssrc/types/tracking.tssrc/types/video.tssrc/utils/PlayerEventBus.test.tssrc/utils/fairu.test.tssrc/utils/index.tssrc/utils/thumbnails.tsvite.config.ts
| politeness = 'polite', | ||
| }: ScreenReaderAnnouncerProps) { | ||
| const [announcement, setAnnouncement] = useState(''); | ||
| const timeoutRef = useRef<ReturnType<typeof setTimeout>>(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== React package versions =="
fd -i package.json --exec sh -c '
echo "-- $1"
rg -n "\"(react|react-dom|@types/react|@types/react-dom)\"\\s*:" "$1" || true
' sh {}
echo
echo "== useRef calls without initializer =="
rg -nP --type ts --type tsx '\buseRef(?:<[^>]+>)?\s*\(\s*\)'Repository: sushidev-team/fairu-player
Length of output: 411
🏁 Script executed:
cat -n src/components/a11y/ScreenReaderAnnouncer.tsx | head -30Repository: sushidev-team/fairu-player
Length of output: 1175
🏁 Script executed:
# Try different patterns to find useRef without initializer
rg -n "useRef" src/components/a11y/ScreenReaderAnnouncer.tsxRepository: sushidev-team/fairu-player
Length of output: 189
🏁 Script executed:
# Check tsconfig for React 19 JSX handling and type strictness
cat tsconfig.jsonRepository: sushidev-team/fairu-player
Length of output: 784
🏁 Script executed:
# Verify `@types/react` version requirement details for useRef
web_search
React 19 useRef type definition initializer requirement `@types/react`Repository: sushidev-team/fairu-player
Length of output: 164
🌐 Web query:
React 19 useRef type definition initializer requirement
💡 Result:
In React 19 with TypeScript (via @types/react ^19.0.0), the useRef hook requires an initializer argument. Previously in React 18, useRef without arguments was allowed (implicitly initializing to {current: undefined}), but this is no longer permitted. useRef(initialValue) returns RefObject where RefObject = { current: T }, and all refs are now mutable by default (no more readonly current when initialized with null). For DOM refs, use useRef(null), which has a convenience overload returning RefObject<ElementType | null>. Examples: - const inputRef = useRef(null); // RefObject<HTMLInputElement | null> - const valueRef = useRef(0); // RefObject - const objRef = useRef({}); // RefObject<{}> - const undefinedRef = useRef(undefined); // Allowed for migration, RefObject Attempting useRef without argument results in a TypeScript error: "Expected 1 argument, but got 0." This change simplifies the type signature, aligns useRef with hooks like useState/createContext, eliminates MutableRefObject vs RefObject confusion, and makes all refs mutable to avoid common "Cannot assign to read-only current" errors when initializing with null. Official docs confirm useRef requires initialValue: https://react.dev/reference/react/useRef. Upgrade codemod available: npx types-react-codemod@latest preset-19 (includes useRef-required-initial transformation).
Citations:
- 1: https://react.dev/reference/react/useRef
- 2: [react] Types for React 19 DefinitelyTyped/DefinitelyTyped#69022
- 3: Change the type of refs for react 19 juliencrn/usehooks-ts#680
- 4: React.js 19 nullable RefObject type is not supported adobe/react-spectrum#7549
- 5: https://github.com/facebook/react/releases/tag/v19.0.0
- 6: https://www.react.dev
- 7: https://www.jsdocs.io/package/@types/react
- 8: https://ar.react.dev/blog/2024/04/25/react-19-upgrade-guide
🏁 Script executed:
# Check TypeScript and React versions in package.json
rg -A2 '"typescript"|"react"' package.jsonRepository: sushidev-team/fairu-player
Length of output: 458
🏁 Script executed:
# Check if there's a cleanup function that handles the timeout
cat -n src/components/a11y/ScreenReaderAnnouncer.tsx | sed -n '21,50p'Repository: sushidev-team/fairu-player
Length of output: 975
Initialize useRef with null for React 19 type compatibility.
Line 19 uses useRef without an initial value. While this works with @types/react 18, the project's peer dependency supports React 19 (^18.0.0 || ^19.0.0). React 19 requires an initializer argument for useRef, and upgrading to @types/react 19 will cause a TypeScript error: "Expected 1 argument, but got 0."
Proposed fix
- const timeoutRef = useRef<ReturnType<typeof setTimeout>>();
+ const timeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const timeoutRef = useRef<ReturnType<typeof setTimeout>>(); | |
| const timeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/a11y/ScreenReaderAnnouncer.tsx` at line 19, The timeoutRef in
ScreenReaderAnnouncer is created without an initial value which breaks React 19
typings; change the declaration of timeoutRef to include a null initializer
(e.g., useRef<ReturnType<typeof setTimeout> | null>(null)) and update any usages
(clearTimeout(timeoutRef.current) / timeoutRef.current = ...) to handle the
nullable type (use optional chaining or null checks) so TypeScript compiles
under React 19.
| <div | ||
| className={cn( | ||
| 'absolute inset-0 z-40 flex items-center justify-center', | ||
| 'bg-black/60 backdrop-blur-sm', | ||
| 'animate-in fade-in duration-300', | ||
| className | ||
| )} | ||
| data-testid="pause-ad" | ||
| > |
There was a problem hiding this comment.
Expose the pause ad as a modal dialog.
This overlay is rendered as a plain <div>, so assistive tech gets no dialog announcement and focus can stay behind the ad. Add role="dialog", aria-modal, an accessible name, and move focus into the overlay when it opens.
💡 Suggested starting point
<div
+ role="dialog"
+ aria-modal="true"
+ aria-label={ad.title || 'Advertisement'}
className={cn(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div | |
| className={cn( | |
| 'absolute inset-0 z-40 flex items-center justify-center', | |
| 'bg-black/60 backdrop-blur-sm', | |
| 'animate-in fade-in duration-300', | |
| className | |
| )} | |
| data-testid="pause-ad" | |
| > | |
| <div | |
| role="dialog" | |
| aria-modal="true" | |
| aria-label={ad.title || 'Advertisement'} | |
| className={cn( | |
| 'absolute inset-0 z-40 flex items-center justify-center', | |
| 'bg-black/60 backdrop-blur-sm', | |
| 'animate-in fade-in duration-300', | |
| className | |
| )} | |
| data-testid="pause-ad" | |
| > |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ads/PauseAd/PauseAd.tsx` around lines 35 - 43, The overlay div
in the PauseAd component is not accessible as a modal dialog; update the element
rendered in PauseAd (the div with data-testid="pause-ad") to include
role="dialog", aria-modal="true" and an accessible name (either aria-label or
aria-labelledby pointing to a title element inside the dialog), add
tabIndex={-1} and a ref (e.g., containerRef), then in PauseAd use a useEffect
that, when the overlay opens, saves document.activeElement, moves focus into
containerRef.current (or the first focusable element), and on close restores
focus to the previously focused element; ensure the title element has an id if
using aria-labelledby and keep these changes scoped to the PauseAd component.
| useEffect(() => { | ||
| if (visible && videoRef.current) { | ||
| videoRef.current.play().catch(() => { | ||
| // Autoplay blocked - user needs to interact | ||
| }); | ||
| } | ||
| }, [visible]); |
There was a problem hiding this comment.
Autoplay failure can dead-end the user behind the overlay
At Line 49, autoplay rejection is ignored, but the UI has no guaranteed manual play path while close stays gated behind completion. Downstream impact: user can get stuck and never reach onClose.
Suggested fix (track autoplay block + provide manual start CTA)
const [isPlaying, setIsPlaying] = useState(false);
const [isCompleted, setIsCompleted] = useState(false);
+ const [autoplayBlocked, setAutoplayBlocked] = useState(false);
const rewardedRef = useRef(false);
// Auto-play when visible
useEffect(() => {
if (visible && videoRef.current) {
- videoRef.current.play().catch(() => {
- // Autoplay blocked - user needs to interact
- });
+ videoRef.current
+ .play()
+ .then(() => setAutoplayBlocked(false))
+ .catch(() => setAutoplayBlocked(true));
}
}, [visible]); <div className="relative w-full max-w-2xl aspect-video rounded-xl overflow-hidden">
<video
@@
data-testid="rewarded-ad-video"
/>
+
+ {autoplayBlocked && !isCompleted && (
+ <button
+ type="button"
+ onClick={() => videoRef.current?.play()}
+ className="absolute inset-0 m-auto h-fit w-fit px-4 py-2 rounded-md bg-black/70 text-white text-sm"
+ >
+ Tap to play ad
+ </button>
+ )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ads/RewardedAd/RewardedAd.tsx` around lines 47 - 53, The
useEffect in RewardedAd currently swallows autoplay rejections causing users to
be stuck; update the component (RewardedAd) to catch autoplay rejection from
videoRef.play(), set a local state flag (e.g., autoplayBlocked) when
play().catch triggers, and render a visible manual play CTA and an enabled close
path (so onClose can be invoked) whenever autoplayBlocked or video is paused;
ensure the play CTA calls videoRef.current.play() and clears autoplayBlocked on
success, and that the close control is not gated only by video completion.
| if (ad.clickThroughUrl) { | ||
| window.open(ad.clickThroughUrl, '_blank', 'noopener,noreferrer'); | ||
| } |
There was a problem hiding this comment.
Validate click-through URL scheme before window.open
Line 79 opens ad.clickThroughUrl directly. If ad payloads are external/untrusted, unsafe schemes (e.g., non-http(s)) should be rejected.
Suggested fix
const handleClick = useCallback(() => {
- if (ad.clickThroughUrl) {
- window.open(ad.clickThroughUrl, '_blank', 'noopener,noreferrer');
- }
+ if (ad.clickThroughUrl) {
+ try {
+ const url = new URL(ad.clickThroughUrl, window.location.href);
+ if (url.protocol === 'http:' || url.protocol === 'https:') {
+ window.open(url.toString(), '_blank', 'noopener,noreferrer');
+ }
+ } catch {
+ // Ignore invalid click-through URL
+ }
+ }
onClick?.(ad);
}, [ad, onClick]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (ad.clickThroughUrl) { | |
| window.open(ad.clickThroughUrl, '_blank', 'noopener,noreferrer'); | |
| } | |
| if (ad.clickThroughUrl) { | |
| try { | |
| const url = new URL(ad.clickThroughUrl, window.location.href); | |
| if (url.protocol === 'http:' || url.protocol === 'https:') { | |
| window.open(url.toString(), '_blank', 'noopener,noreferrer'); | |
| } | |
| } catch { | |
| // Ignore invalid click-through URL | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ads/RewardedAd/RewardedAd.tsx` around lines 78 - 80, The
click-through handler in the RewardedAd component currently calls
window.open(ad.clickThroughUrl) without validating the URL scheme; update the
handler (where ad.clickThroughUrl is used) to parse the URL (e.g., using the URL
constructor) and only call window.open if the protocol is "http:" or "https:",
otherwise reject the value (no-op or log) to prevent unsafe schemes; also guard
the parse in a try/catch to handle malformed URLs and avoid throwing.
| // Optionally wrap with AdProvider | ||
| if (ads) { | ||
| content = <AdProvider config={ads}>{content}</AdProvider>; | ||
| } | ||
|
|
||
| // Optionally wrap with TrackingProvider | ||
| if (tracking) { | ||
| content = <TrackingProvider config={tracking}>{content}</TrackingProvider>; | ||
| } |
There was a problem hiding this comment.
Keep the provider tree stable across config changes.
Conditionally inserting AdProvider or TrackingProvider changes the root element type when those props appear or disappear. That remounts PlayerProvider and resets playback/context state if ads or tracking config loads asynchronously.
Prefer always-rendered wrappers that no-op when config is absent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/context/FairuProvider.tsx` around lines 68 - 76, The conditional
rendering of AdProvider and TrackingProvider causes the root element type to
change and remount PlayerProvider when configs appear/disappear; always render
the wrappers instead and make them no-op when config is absent: replace the
if-blocks in FairuProvider.tsx so AdProvider and TrackingProvider are always
present around content (wrapping PlayerProvider) and update their
props/implementation to accept a nullable config (or introduce
NullAdProvider/NullTrackingProvider that forwards children unchanged) so
PlayerProvider is not unmounted when ads or tracking load asynchronously.
| const [enabled, setEnabledState] = useState(config.enabled); | ||
| const serviceRef = useRef<TrackingService | null>(null); | ||
|
|
||
| // Flush event queue | ||
| const flush = useCallback(async () => { | ||
| if (eventQueue.current.length === 0) return; | ||
| const events = [...eventQueue.current]; | ||
| eventQueue.current = []; | ||
| await sendEvents(events); | ||
| }, [sendEvents]); | ||
| // Initialize / recreate the tracking service when config changes | ||
| useEffect(() => { | ||
| const service = new TrackingService({ ...config, enabled }); | ||
| serviceRef.current = service; | ||
|
|
||
| return () => { | ||
| service.destroy(); | ||
| serviceRef.current = null; | ||
| }; | ||
| }, [config, enabled]); |
There was a problem hiding this comment.
Keep enabled in sync with prop updates.
useState(config.enabled) only reads the prop on the first render. If a parent later flips tracking.enabled, this provider keeps the old value and recreates TrackingService with stale settings, so external config changes never take effect.
💡 Suggested fix
const [enabled, setEnabledState] = useState(config.enabled);
+
+ useEffect(() => {
+ setEnabledState(config.enabled);
+ }, [config.enabled]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [enabled, setEnabledState] = useState(config.enabled); | |
| const serviceRef = useRef<TrackingService | null>(null); | |
| // Flush event queue | |
| const flush = useCallback(async () => { | |
| if (eventQueue.current.length === 0) return; | |
| const events = [...eventQueue.current]; | |
| eventQueue.current = []; | |
| await sendEvents(events); | |
| }, [sendEvents]); | |
| // Initialize / recreate the tracking service when config changes | |
| useEffect(() => { | |
| const service = new TrackingService({ ...config, enabled }); | |
| serviceRef.current = service; | |
| return () => { | |
| service.destroy(); | |
| serviceRef.current = null; | |
| }; | |
| }, [config, enabled]); | |
| const [enabled, setEnabledState] = useState(config.enabled); | |
| useEffect(() => { | |
| setEnabledState(config.enabled); | |
| }, [config.enabled]); | |
| const serviceRef = useRef<TrackingService | null>(null); | |
| // Initialize / recreate the tracking service when config changes | |
| useEffect(() => { | |
| const service = new TrackingService({ ...config, enabled }); | |
| serviceRef.current = service; | |
| return () => { | |
| service.destroy(); | |
| serviceRef.current = null; | |
| }; | |
| }, [config, enabled]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/context/TrackingContext.tsx` around lines 43 - 55, The local state
"enabled" (created via useState(config.enabled)) can become stale when the
parent updates config.enabled; add an effect to sync it from props by calling
setEnabledState(config.enabled) whenever config.enabled changes (so that the
existing useEffect which constructs TrackingService with { ...config, enabled }
gets the updated value), or alternatively remove the local enabled state and use
config.enabled directly when instantiating TrackingService; update code
referencing enabled, setEnabledState, serviceRef, and the useEffect that creates
TrackingService accordingly.
| beforeEach(() => { | ||
| vi.useFakeTimers(); | ||
| globalThis.fetch = mockFetch as unknown as typeof fetch; | ||
| mockFetch.mockClear(); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| vi.useRealTimers(); | ||
| vi.restoreAllMocks(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and read the relevant section
find . -name "VideoAdContext.test.tsx" -type fRepository: sushidev-team/fairu-player
Length of output: 107
🏁 Script executed:
# Also search for the file more broadly
fd -i "videoadcontext" --type fRepository: sushidev-team/fairu-player
Length of output: 136
🏁 Script executed:
# Read the test file to verify the code snippet
cat -n src/context/VideoAdContext.test.tsx | head -60Repository: sushidev-team/fairu-player
Length of output: 2155
🏁 Script executed:
# Check if there are other tests in the repo that handle global fetch or similar patterns
rg -t js -t tsx "globalThis\." src/ --max-count=20Repository: sushidev-team/fairu-player
Length of output: 97
🏁 Script executed:
# Check if there are other afterEach patterns that restore globals
rg -t js -t tsx "afterEach" src/ -A 3 --max-count=20Repository: sushidev-team/fairu-player
Length of output: 97
🏁 Script executed:
# Search for globalThis patterns without file type restriction
rg "globalThis\." src/ -A 2 -B 2Repository: sushidev-team/fairu-player
Length of output: 1681
🏁 Script executed:
# Search for fetch restoration patterns in test files
rg "originalFetch\|globalThis.fetch" --no-headingRepository: sushidev-team/fairu-player
Length of output: 52
🌐 Web query:
Vitest vi.restoreAllMocks() global reassignment fetch globalThis
💡 Result:
In Vitest, vi.restoreAllMocks restores spies created with vi.spyOn or vi.fn, but does not automatically restore direct global reassignments like globalThis.fetch = vi.fn or vi.stubGlobal('fetch', ...). For mocking the global fetch (often via globalThis.fetch), common patterns include: 1. Direct assignment: globalThis.fetch = vi.fn( => Promise.resolve(mockResponse)). This fully replaces fetch. vi.restoreAllMocks does not restore it; use vi.unstubAllGlobals if using vi.stubGlobal, or manual restoration like saving/restoring the original in beforeEach/afterEach. 2. vi.spyOn(globalThis, 'fetch'): Creates a spy on the existing fetch. This works with vi.restoreAllMocks to restore the original implementation and clear history. vi.restoreAllMocks calls .mockRestore on all spies, preserving original behavior for spied globals but not handling non-spied reassignments. Official docs recommend vi.stubGlobal for globals with unstubGlobals: true in config for auto-reset, or vi.unstubAllGlobals manually. For fetch specifically, libraries like vitest-fetch-mock or MSW (@mswjs/interceptors in Node) handle global overriding with their own reset methods integrated with Vitest hooks. Example for spyOn (restorable with vi.restoreAllMocks): const fetchSpy = vi.spyOn(globalThis, 'fetch').mockResolvedValue(mockResponse); afterEach( => { vi.restoreAllMocks; }); Example for direct assignment (requires unstubAllGlobals or manual reset): const originalFetch = globalThis.fetch; globalThis.fetch = vi.fn; afterEach( => { globalThis.fetch = originalFetch; }); Or with vi.stubGlobal('fetch', vi.fn): afterEach(vi.unstubAllGlobals); Direct global reassignments persist across tests unless explicitly reset, leading to leakage if not handled in hooks.
Citations:
- 1: Mocks are not restored using resetAllMocks() or restoreAllMocks() vitest-dev/vitest#2536
- 2: https://runebook.dev/en/articles/vitest/api/vi/vi-restoreallmocks
- 3: https://vitest.dev/guide/mocking
- 4: restoreAllMocks doesn't restore mocks vitest-dev/vitest#5748
- 5: https://v4.vitest.dev/guide/mocking/globals
- 6: https://blog.logrocket.com/advanced-guide-vitest-testing-mocking/
- 7: https://runthatline.com/how-to-mock-fetch-api-with-vitest/
- 8: https://v3.vitest.dev/api/vi
- 9: https://tannerhodges.com/blog/mocking-fetch-with-vitest/
Restore the original fetch after each test.
globalThis.fetch is reassigned directly in beforeEach, and vi.restoreAllMocks() does not undo direct reassignments to global objects. The mock will leak into subsequent tests, potentially causing unrelated assertions to pass or fail for the wrong reason.
Suggested fix
+const originalFetch = globalThis.fetch;
+
beforeEach(() => {
vi.useFakeTimers();
globalThis.fetch = mockFetch as unknown as typeof fetch;
mockFetch.mockClear();
});
afterEach(() => {
vi.useRealTimers();
vi.restoreAllMocks();
+ globalThis.fetch = originalFetch;
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| beforeEach(() => { | |
| vi.useFakeTimers(); | |
| globalThis.fetch = mockFetch as unknown as typeof fetch; | |
| mockFetch.mockClear(); | |
| }); | |
| afterEach(() => { | |
| vi.useRealTimers(); | |
| vi.restoreAllMocks(); | |
| }); | |
| const originalFetch = globalThis.fetch; | |
| beforeEach(() => { | |
| vi.useFakeTimers(); | |
| globalThis.fetch = mockFetch as unknown as typeof fetch; | |
| mockFetch.mockClear(); | |
| }); | |
| afterEach(() => { | |
| vi.useRealTimers(); | |
| vi.restoreAllMocks(); | |
| globalThis.fetch = originalFetch; | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/context/VideoAdContext.test.tsx` around lines 37 - 46, Tests reassign
globalThis.fetch in the beforeEach block causing leakage; save the original
fetch (e.g., const originalFetch = globalThis.fetch) before assigning mockFetch
in beforeEach, and then restore it in afterEach by setting globalThis.fetch =
originalFetch; keep existing vi.restoreAllMocks() and vi.useRealTimers() but
ensure globalThis.fetch is explicitly reverted to the saved original so the
mockFetch does not leak across tests (refer to beforeEach, afterEach,
globalThis.fetch, mockFetch, vi.useFakeTimers, vi.restoreAllMocks).
| const onPlay = vi.fn(); | ||
| const onPause = vi.fn(); | ||
| const onEnded = vi.fn(); | ||
| const onError = vi.fn(); | ||
| const onTimeUpdate = vi.fn(); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Vitest config files =="
fd -i 'vitest.config.*'
echo "== globals setting in test configs =="
rg -n --iglob '*vitest*.*' 'globals\s*:'
echo "== vi usage in tests =="
rg -nP --type=ts --type=tsx '\bvi\.' -g '!**/node_modules/**'
echo "== explicit vitest imports =="
rg -nP --type=ts --type=tsx "from ['\"]vitest['\"]"Repository: sushidev-team/fairu-player
Length of output: 299
🏁 Script executed:
# Read the test file to see imports and vi usage
cat -n src/hooks/useAudio.test.ts | head -30Repository: sushidev-team/fairu-player
Length of output: 1432
🏁 Script executed:
# Check the entire beginning of the file including imports
head -200 src/hooks/useAudio.test.ts | tail -50Repository: sushidev-team/fairu-player
Length of output: 1256
🏁 Script executed:
# Look for vitest config files more broadly
find . -maxdepth 2 -name "*vitest*" -o -name "vite.config.*" 2>/dev/null | head -20Repository: sushidev-team/fairu-player
Length of output: 86
🏁 Script executed:
# Check for package.json vitest config
cat package.json 2>/dev/null | grep -A 10 '"vitest"'Repository: sushidev-team/fairu-player
Length of output: 609
🏁 Script executed:
# Look for vi imports across test files
rg "from ['\"]vitest['\"]" src/ 2>/dev/null | head -20Repository: sushidev-team/fairu-player
Length of output: 1937
🏁 Script executed:
cat vite.config.tsRepository: sushidev-team/fairu-player
Length of output: 4377
🏁 Script executed:
sed -n '195,210p' src/hooks/useAudio.test.tsRepository: sushidev-team/fairu-player
Length of output: 445
Import vi explicitly to match codebase convention.
vi.fn() is used at lines 199-204, but vi is not imported. While globals: true in vite.config.ts enables global vi access, all other test files in the codebase explicitly import vi from vitest. Add the import to maintain consistency:
import { renderHook, act } from '@testing-library/react';
+import { vi } from 'vitest';
import { useAudio } from './useAudio';
import { fireMediaEvent, simulateTimeUpdate, simulateLoadedMetadata } from '@/test/helpers';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const onPlay = vi.fn(); | |
| const onPause = vi.fn(); | |
| const onEnded = vi.fn(); | |
| const onError = vi.fn(); | |
| const onTimeUpdate = vi.fn(); | |
| import { renderHook, act } from '@testing-library/react'; | |
| import { vi } from 'vitest'; | |
| import { useAudio } from './useAudio'; | |
| import { fireMediaEvent, simulateTimeUpdate, simulateLoadedMetadata } from '@/test/helpers'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/useAudio.test.ts` around lines 199 - 204, The test file uses
vi.fn() for mocks (onPlay, onPause, onEnded, onError, onTimeUpdate) but does not
import vi; add an explicit import "import { vi } from 'vitest'" at the top of
src/hooks/useAudio.test.ts to follow project convention and ensure the vi symbol
is resolved consistently across tests.
| @@ -0,0 +1,118 @@ | |||
| import { useState, useEffect, useCallback, useRef } from 'react'; | |||
There was a problem hiding this comment.
requestPlay is part of the public contract, but it currently does nothing.
Consumers calling the returned method after a user gesture will never recover blocked autoplay, even though the API says they can. Either implement it against a supplied media ref/callback, or remove it until that path exists.
🧩 One safe option is to remove the placeholder API for now
-import { useState, useEffect, useCallback, useRef } from 'react';
+import { useState, useEffect, useRef } from 'react';
@@
export interface AutoplayPolicy {
/** Whether autoplay with sound is allowed */
canAutoplay: boolean;
/** Whether autoplay is allowed when muted */
canAutoplayMuted: boolean;
/** Whether detection has completed */
detected: boolean;
- /** If autoplay was blocked, call this to attempt playing after user gesture */
- requestPlay: () => void;
}
@@
- const requestPlay = useCallback(() => {
- // No-op placeholder — consumers can replace this to trigger play after a user gesture
- }, []);
-
useEffect(() => {
@@
return {
canAutoplay,
canAutoplayMuted,
detected,
- requestPlay,
};
}Also applies to: 10-11, 49-51, 112-117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/useAutoplayDetection.ts` at line 1, The exported requestPlay
function in useAutoplayDetection.ts is currently a no-op and must either be
implemented or removed; implement it by accepting the media element
reference/callback used by the hook (e.g., the same ref passed into the hook or
a setMediaRef callback) and attempt to call mediaRef.current.play(), handling
the returned Promise and caught DOMException by setting the hook's state
(isAutoplayBlocked/isAutoplayAllowed) and resolving/rejecting appropriately;
update the hook's returned API to include this functional requestPlay and ensure
it uses the existing internal state setters (the same ones referenced around
lines where requestPlay is declared/returned) so consumers can call
requestPlay() after a user gesture to recover from blocked autoplay.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
src/components/controls/VolumeControl/VolumeControl.test.tsx (3)
175-187: Close the drag cycle in this test to prevent lingering global listeners.At Line 181, drag starts but the test never sends
mouseUp. If cleanup depends on mouseup, this can make later tests flaky.Suggested change
fireEvent.mouseMove(document, { clientX: 72, clientY: 12 }); expect(onVolumeChange).toHaveBeenCalled(); + fireEvent.mouseUp(document);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/controls/VolumeControl/VolumeControl.test.tsx` around lines 175 - 187, The test "calls onVolumeChange on drag (horizontal)" starts a drag with fireEvent.mouseDown but never ends it, leaving global listeners active; update the test in VolumeControl.test.tsx (the one that uses renderVolumeControl, mockSliderRect, fireEvent.mouseDown, and fireEvent.mouseMove) to dispatch a corresponding mouseUp (e.g., fireEvent.mouseUp(document)) after the mouseMove assertion to close the drag cycle and ensure global listeners are removed/cleaned up.
1-1: Improve test isolation by restoring spies after each test.
getBoundingClientRectspies are created repeatedly; adding teardown avoids hidden coupling between tests and keeps failures easier to diagnose.Suggested change
-import { describe, it, expect, vi } from 'vitest'; +import { describe, it, expect, vi, afterEach } from 'vitest'; ... describe('VolumeControl', () => { + afterEach(() => { + vi.restoreAllMocks(); + });Also applies to: 53-53, 25-51
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/controls/VolumeControl/VolumeControl.test.tsx` at line 1, Tests in VolumeControl.test.tsx create spies for getBoundingClientRect repeatedly without teardown; add an afterEach that calls vi.restoreAllMocks() (or explicitly calls mockRestore() on any vi.spyOn instances) to restore getBoundingClientRect spies between tests so they don't leak state across cases; ensure the afterEach is placed alongside the existing describe/it blocks so vi.restoreAllMocks() runs after each test.
257-267: Consider reducing Tailwind-implementation coupling in selectors.These assertions depend on specific utility-class chains and internal DOM structure. Prefer stable selectors (
data-testid) for overlay/fill elements and keep class checks for cases where style class itself is the behavior under test.Also applies to: 384-394, 412-425
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/controls/VolumeControl/VolumeControl.test.tsx` around lines 257 - 267, The tests rely on fragile Tailwind class selectors and DOM structure; update the VolumeControl component to add stable data-testid attributes for the overlay/fill elements (e.g., data-testid="volume-overlay" and data-testid="volume-fill") and change the tests (e.g., the 'hides vertical slider on mouse leave (when not dragging)' spec) to query those elements via getByTestId (or screen.getByTestId) instead of container.querySelector('.absolute.bottom-full'); continue to assert the opacity class only when the class itself is the behavior under test (otherwise assert visibility via computed styles or aria attributes), and apply the same update pattern to the other tests referenced (lines ~384-394 and ~412-425) that currently rely on utility-class chains.src/examples/NewFeaturesDemo.stories.tsx (1)
21-21: Use a single source of truth for demo duration.
ProgressBaruses a hardcoded596while the track already definesduration. UsingsampleVideo.durationavoids silent drift.♻️ Proposed fix
- duration={596} + duration={sampleVideo.duration}Also applies to: 388-388
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/examples/NewFeaturesDemo.stories.tsx` at line 21, Replace the hardcoded duration value used in ProgressBar with the single source of truth (the demo track's duration) — find the ProgressBar prop currently set to 596 and change it to use sampleVideo.duration (or the existing track.duration variable) and do the same for the other occurrence around line 388; ensure the value passed is a number and matches the shape expected by ProgressBar so both instances reference the track's duration instead of the literal 596.src/embed/parseConfig.test.ts (2)
118-197: Consider table-driven tests to reduce duplication.Large repeated true/false and default/override assertions are good coverage, but this can be compressed with
it.each(...)to keep future updates safer and easier.Also applies to: 540-604, 606-708
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/embed/parseConfig.test.ts` around lines 118 - 197, The tests under describe('features parsing') are repetitive; refactor them to a data-driven table using Jest's it.each to reduce duplication: create a matrix of input attribute keys/values and expected feature keys (e.g., rows like {attr: 'chapters', value: 'false', feature: 'chapters', expected: false} and default rows for true cases), then replace individual it blocks with a single parametrized test that calls createElement and parseDataAttributes and asserts config.player.features[feature] === expected; also consolidate the "defaults all features to true" as one smoke test and keep the mixed-values case as a separate table-driven row set to verify non-"false" values remain true. Ensure tests reference the parseDataAttributes and createElement helpers so behavior stays identical.
82-87: Codify malformed JSON behavior with explicit tests.These sections only cover valid JSON. Add tests for malformed
chapters/playlist/speedsso parser behavior (throw vs fallback) is intentional and protected from regressions.Also applies to: 99-108, 284-290, 522-531
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/embed/parseConfig.test.ts` around lines 82 - 87, Add explicit tests in src/embed/parseConfig.test.ts that cover malformed JSON inputs for the data attributes handled by parseDataAttributes (specifically "chapters", "playlist", and "speeds") so the parser's current behavior is codified and protected from regressions; use the same test patterns as the existing valid-JSON tests (createElement helper, call parseDataAttributes) but pass invalid JSON strings and assert the current observed outcome (either expect the call to throw or expect a safe fallback value on config.player.track!/config.playlist!/config.speeds), and replicate this for the other locations referenced in the comment to ensure consistent behavior across parseDataAttributes and any helper parsing functions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/playlist/TrackItem/TrackItem.test.tsx`:
- Around line 65-74: The tests in TrackItem.test.tsx use getByRole('img') /
queryByRole('img') which can match the artwork <img> in TrackItem.tsx as well as
the NowPlayingIndicator; update the assertions to target the NowPlayingIndicator
by its accessible name instead of any img role (e.g., use getByRole('img', {
name: /now playing/i }) and queryByRole('img', { name: /now playing/i })) so
renderTrackItem(...) still renders the artwork but the test only checks the
specific indicator.
In `@src/embed/parseConfig.test.ts`:
- Around line 134-139: Add a regression test to cover the interaction where an
element has a src and chapters="false": create an element with createElement({
src: 'some.mp4', chapters: 'false' }), call parseDataAttributes(el) and assert
that config.player.features!.chapters is false (and that no chapters track array
was parsed); this ensures parseDataAttributes handles the chapters="false"
string correctly when src is present.
In `@src/examples/NewFeaturesDemo.stories.tsx`:
- Around line 333-350: Guard the loop bounds before passing them to the
ProgressBar: ensure loopStart and loopEnd are non-null and that loopStart <=
loopEnd (normalize by swapping or clamping if inverted) wherever you compute the
preview or render ProgressBar (e.g., where loopStart, loopEnd are used after
setLoopStart/setLoopEnd and in the second block around lines 370-393); update
the code that supplies the loop region to compute safeStart =
Math.min(loopStart, loopEnd) and safeEnd = Math.max(loopStart, loopEnd) (or skip
rendering the preview when either is null) and pass safeStart/safeEnd to
ProgressBar to avoid sending an inverted range.
---
Nitpick comments:
In `@src/components/controls/VolumeControl/VolumeControl.test.tsx`:
- Around line 175-187: The test "calls onVolumeChange on drag (horizontal)"
starts a drag with fireEvent.mouseDown but never ends it, leaving global
listeners active; update the test in VolumeControl.test.tsx (the one that uses
renderVolumeControl, mockSliderRect, fireEvent.mouseDown, and
fireEvent.mouseMove) to dispatch a corresponding mouseUp (e.g.,
fireEvent.mouseUp(document)) after the mouseMove assertion to close the drag
cycle and ensure global listeners are removed/cleaned up.
- Line 1: Tests in VolumeControl.test.tsx create spies for getBoundingClientRect
repeatedly without teardown; add an afterEach that calls vi.restoreAllMocks()
(or explicitly calls mockRestore() on any vi.spyOn instances) to restore
getBoundingClientRect spies between tests so they don't leak state across cases;
ensure the afterEach is placed alongside the existing describe/it blocks so
vi.restoreAllMocks() runs after each test.
- Around line 257-267: The tests rely on fragile Tailwind class selectors and
DOM structure; update the VolumeControl component to add stable data-testid
attributes for the overlay/fill elements (e.g., data-testid="volume-overlay" and
data-testid="volume-fill") and change the tests (e.g., the 'hides vertical
slider on mouse leave (when not dragging)' spec) to query those elements via
getByTestId (or screen.getByTestId) instead of
container.querySelector('.absolute.bottom-full'); continue to assert the opacity
class only when the class itself is the behavior under test (otherwise assert
visibility via computed styles or aria attributes), and apply the same update
pattern to the other tests referenced (lines ~384-394 and ~412-425) that
currently rely on utility-class chains.
In `@src/embed/parseConfig.test.ts`:
- Around line 118-197: The tests under describe('features parsing') are
repetitive; refactor them to a data-driven table using Jest's it.each to reduce
duplication: create a matrix of input attribute keys/values and expected feature
keys (e.g., rows like {attr: 'chapters', value: 'false', feature: 'chapters',
expected: false} and default rows for true cases), then replace individual it
blocks with a single parametrized test that calls createElement and
parseDataAttributes and asserts config.player.features[feature] === expected;
also consolidate the "defaults all features to true" as one smoke test and keep
the mixed-values case as a separate table-driven row set to verify non-"false"
values remain true. Ensure tests reference the parseDataAttributes and
createElement helpers so behavior stays identical.
- Around line 82-87: Add explicit tests in src/embed/parseConfig.test.ts that
cover malformed JSON inputs for the data attributes handled by
parseDataAttributes (specifically "chapters", "playlist", and "speeds") so the
parser's current behavior is codified and protected from regressions; use the
same test patterns as the existing valid-JSON tests (createElement helper, call
parseDataAttributes) but pass invalid JSON strings and assert the current
observed outcome (either expect the call to throw or expect a safe fallback
value on config.player.track!/config.playlist!/config.speeds), and replicate
this for the other locations referenced in the comment to ensure consistent
behavior across parseDataAttributes and any helper parsing functions.
In `@src/examples/NewFeaturesDemo.stories.tsx`:
- Line 21: Replace the hardcoded duration value used in ProgressBar with the
single source of truth (the demo track's duration) — find the ProgressBar prop
currently set to 596 and change it to use sampleVideo.duration (or the existing
track.duration variable) and do the same for the other occurrence around line
388; ensure the value passed is a number and matches the shape expected by
ProgressBar so both instances reference the track's duration instead of the
literal 596.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 90b62af8-01ae-42a1-b773-2517c261d4b8
📒 Files selected for processing (20)
package.jsonsrc/components/VideoPlayer/EndScreen/EndScreen.test.tsxsrc/components/controls/ProgressBar/ProgressBar.test.tsxsrc/components/controls/VolumeControl/VolumeControl.test.tsxsrc/components/playlist/PlaylistView/PlaylistView.test.tsxsrc/components/playlist/TrackItem/TrackItem.test.tsxsrc/context/AdContext.test.tsxsrc/context/TrackingContext.test.tsxsrc/embed/parseConfig.test.tssrc/examples/NewFeaturesDemo.stories.tsxsrc/hooks/useAudio.test.tssrc/hooks/useCast.test.tssrc/hooks/usePauseAd.tssrc/hooks/useRewardedAd.tssrc/hooks/useSubtitleParser.test.tssrc/hooks/useVideo.test.tssrc/index.tssrc/services/AdService.test.tssrc/services/TrackingService.test.tssrc/test/helpers.tsx
✅ Files skipped from review due to trivial changes (3)
- src/context/AdContext.test.tsx
- src/hooks/useCast.test.ts
- src/components/controls/ProgressBar/ProgressBar.test.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- package.json
- src/hooks/useAudio.test.ts
- src/components/playlist/PlaylistView/PlaylistView.test.tsx
- src/context/TrackingContext.test.tsx
- src/components/VideoPlayer/EndScreen/EndScreen.test.tsx
| it('shows NowPlayingIndicator when active and playing', () => { | ||
| renderTrackItem({ isActive: true, isPlaying: true }); | ||
| // The NowPlayingIndicator renders with role="img" | ||
| expect(screen.getByRole('img')).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('does not show NowPlayingIndicator when not active', () => { | ||
| renderTrackItem({ isActive: false, isPlaying: false }); | ||
| expect(screen.queryByRole('img')).not.toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether TrackItem can render additional img-role elements (e.g., artwork),
# which would make generic getByRole('img') ambiguous.
rg -n -C3 'NowPlayingIndicator|<img|role=|aria-label|alt=' \
src/components/playlist/TrackItem/TrackItem.tsx \
src/components/controls/NowPlayingIndicator/NowPlayingIndicator.tsxRepository: sushidev-team/fairu-player
Length of output: 4563
Disambiguate NowPlayingIndicator queries to avoid matching artwork.
At lines 68 and 73, getByRole('img') and queryByRole('img') can match the track artwork <img> element (lines 48–52 of TrackItem.tsx) in addition to the NowPlayingIndicator, causing test brittleness. Narrow the selector to the specific indicator using its accessible name.
Proposed test fix
it('shows NowPlayingIndicator when active and playing', () => {
renderTrackItem({ isActive: true, isPlaying: true });
- // The NowPlayingIndicator renders with role="img"
- expect(screen.getByRole('img')).toBeInTheDocument();
+ expect(screen.getByRole('img', { name: /now playing/i })).toBeInTheDocument();
});
it('does not show NowPlayingIndicator when not active', () => {
renderTrackItem({ isActive: false, isPlaying: false });
- expect(screen.queryByRole('img')).not.toBeInTheDocument();
+ expect(
+ screen.queryByRole('img', { name: /now playing/i })
+ ).not.toBeInTheDocument();
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('shows NowPlayingIndicator when active and playing', () => { | |
| renderTrackItem({ isActive: true, isPlaying: true }); | |
| // The NowPlayingIndicator renders with role="img" | |
| expect(screen.getByRole('img')).toBeInTheDocument(); | |
| }); | |
| it('does not show NowPlayingIndicator when not active', () => { | |
| renderTrackItem({ isActive: false, isPlaying: false }); | |
| expect(screen.queryByRole('img')).not.toBeInTheDocument(); | |
| }); | |
| it('shows NowPlayingIndicator when active and playing', () => { | |
| renderTrackItem({ isActive: true, isPlaying: true }); | |
| expect(screen.getByRole('img', { name: /now playing/i })).toBeInTheDocument(); | |
| }); | |
| it('does not show NowPlayingIndicator when not active', () => { | |
| renderTrackItem({ isActive: false, isPlaying: false }); | |
| expect( | |
| screen.queryByRole('img', { name: /now playing/i }) | |
| ).not.toBeInTheDocument(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/playlist/TrackItem/TrackItem.test.tsx` around lines 65 - 74,
The tests in TrackItem.test.tsx use getByRole('img') / queryByRole('img') which
can match the artwork <img> in TrackItem.tsx as well as the NowPlayingIndicator;
update the assertions to target the NowPlayingIndicator by its accessible name
instead of any img role (e.g., use getByRole('img', { name: /now playing/i })
and queryByRole('img', { name: /now playing/i })) so renderTrackItem(...) still
renders the artwork but the test only checks the specific indicator.
| it('disables chapters when data-chapters is "false"', () => { | ||
| const el = createElement({ chapters: 'false' }); | ||
| const config = parseDataAttributes(el); | ||
|
|
||
| expect(config.player.features!.chapters).toBe(false); | ||
| }); |
There was a problem hiding this comment.
Add a regression test for src + chapters="false" interaction.
Line 134 currently validates only the feature flag path. With src present, parseDataAttributes also parses chapters as track JSON, so "false" can become a non-array track value and slip through untested.
Proposed test addition
+ it('disables chapter feature without populating track chapters when chapters="false"', () => {
+ const el = createElement({ src: 'audio.mp3', chapters: 'false' });
+ const config = parseDataAttributes(el);
+
+ expect(config.player.features!.chapters).toBe(false);
+ expect(config.player.track!.chapters).toBeUndefined();
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('disables chapters when data-chapters is "false"', () => { | |
| const el = createElement({ chapters: 'false' }); | |
| const config = parseDataAttributes(el); | |
| expect(config.player.features!.chapters).toBe(false); | |
| }); | |
| it('disables chapters when data-chapters is "false"', () => { | |
| const el = createElement({ chapters: 'false' }); | |
| const config = parseDataAttributes(el); | |
| expect(config.player.features!.chapters).toBe(false); | |
| }); | |
| it('disables chapter feature without populating track chapters when chapters="false"', () => { | |
| const el = createElement({ src: 'audio.mp3', chapters: 'false' }); | |
| const config = parseDataAttributes(el); | |
| expect(config.player.features!.chapters).toBe(false); | |
| expect(config.player.track!.chapters).toBeUndefined(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/embed/parseConfig.test.ts` around lines 134 - 139, Add a regression test
to cover the interaction where an element has a src and chapters="false": create
an element with createElement({ src: 'some.mp4', chapters: 'false' }), call
parseDataAttributes(el) and assert that config.player.features!.chapters is
false (and that no chapters track array was parsed); this ensures
parseDataAttributes handles the chapters="false" string correctly when src is
present.
| onClick={() => { | ||
| setLoopStart(currentTime); | ||
| addEvent(`Loop A set: ${Math.floor(currentTime)}s`); | ||
| }} | ||
| className={`flex-1 px-2 py-1 rounded text-xs border transition-colors ${ | ||
| loopStart !== null | ||
| ? 'border-blue-400 text-blue-400' | ||
| : 'border-gray-600 text-gray-400' | ||
| }`} | ||
| > | ||
| Set A{' '} | ||
| {loopStart !== null ? `(${Math.floor(loopStart)}s)` : ''} | ||
| </button> | ||
| <button | ||
| onClick={() => { | ||
| setLoopEnd(currentTime); | ||
| addEvent(`Loop B set: ${Math.floor(currentTime)}s`); | ||
| }} |
There was a problem hiding this comment.
Guard against inverted A/B loop bounds before rendering preview.
If users set B before A (or move A past B), loopStart > loopEnd can be sent to ProgressBar, producing an invalid loop region.
💡 Proposed fix
+ const normalizedLoop = useMemo(() => {
+ if (loopStart === null || loopEnd === null) return null;
+ return {
+ start: Math.min(loopStart, loopEnd),
+ end: Math.max(loopStart, loopEnd),
+ };
+ }, [loopStart, loopEnd]);
+
// Convert subtitle style to CSS
const subtitleCss = useMemo(
() => styleToCss(subtitleStyle),
[subtitleStyle],
);
@@
- {loopStart !== null && loopEnd !== null && (
+ {normalizedLoop && (
<div className="text-xs text-blue-400">
- Looping: {Math.floor(loopStart)}s →{' '}
- {Math.floor(loopEnd)}s
+ Looping: {Math.floor(normalizedLoop.start)}s →{' '}
+ {Math.floor(normalizedLoop.end)}s
</div>
)}
@@
- {loopStart !== null && loopEnd !== null && (
+ {normalizedLoop && (
<div className="bg-gray-800 rounded-lg p-4">
@@
<ProgressBar
currentTime={currentTime}
duration={596}
- loopStart={loopStart}
- loopEnd={loopEnd}
+ loopStart={normalizedLoop.start}
+ loopEnd={normalizedLoop.end}
disabled
/>
</div>
)}Also applies to: 370-393
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/examples/NewFeaturesDemo.stories.tsx` around lines 333 - 350, Guard the
loop bounds before passing them to the ProgressBar: ensure loopStart and loopEnd
are non-null and that loopStart <= loopEnd (normalize by swapping or clamping if
inverted) wherever you compute the preview or render ProgressBar (e.g., where
loopStart, loopEnd are used after setLoopStart/setLoopEnd and in the second
block around lines 370-393); update the code that supplies the loop region to
compute safeStart = Math.min(loopStart, loopEnd) and safeEnd =
Math.max(loopStart, loopEnd) (or skip rendering the preview when either is null)
and pass safeStart/safeEnd to ProgressBar to avoid sending an inverted range.
1 participant
No description provided.