PIP, Cast Feature, Event Logic for automatic stopping#8
Conversation
WalkthroughThis PR introduces Picture-in-Picture (PiP) and device casting capabilities, tab visibility monitoring with return ads, and timeline markers support. It adds new UI components (PictureInPictureButton, CastButton, MarkerList), corresponding React hooks (usePictureInPicture, useCast, useTabVisibility, useMarkers), a PlayerEventBus event system, type definitions for new features, and a CDN loader embed script. Integration includes extending VideoPlayer's prop surface, updating ProgressBar with marker visualization, adding Storybook stories for demonstration, and modifying the build configuration to support a new cdn-loader mode. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@src/components/controls/CastButton/CastButton.tsx`:
- Around line 26-41: The CastButton component's JSX button currently lacks an
explicit type, causing it to default to "submit" in forms; update the button
element in CastButton.tsx (the JSX button inside the CastButton component where
onClick, disabled, className, aria-label and title are set) to include
type="button" so clicks won't submit surrounding forms unintentionally.
In `@src/components/controls/PictureInPictureButton/PictureInPictureButton.tsx`:
- Around line 26-41: The button in the PictureInPictureButton component
currently lacks an explicit type, causing it to default to "submit" inside
forms; update the <button> element in PictureInPictureButton (the JSX render for
the component) to include type="button" so it won't trigger form submissions
when clicked, keeping all existing props (onClick, disabled, className,
aria-label, title) intact.
In `@src/components/VideoPlayer/VideoPlayer.stories.tsx`:
- Around line 2598-2601: Update the block comment for the "Tab Visibility with
Return Ad demo" to match the actual configured behavior: either change the
phrase “after 3 seconds” to reflect that returnAdMinHiddenDuration is set to 0
(immediate), or adjust the configuration (returnAdMinHiddenDuration) to 3000 if
you intend a 3-second delay; ensure both the comment near the demo header and
the similar comment at lines around the second occurrence (the other comment at
2655-2659) are made consistent with the chosen behavior.
In `@src/embed/loader.ts`:
- Around line 70-75: The code currently injects user-controlled artwork and
title into innerHTML (in the block that checks artwork and creates d with class
'fl__art'), which allows XSS; replace the innerHTML usage with safe DOM APIs:
create an HTMLImageElement (document.createElement('img')), set its src (img.src
= artwork or img.setAttribute('src', artwork)), set img.alt = title || 'Cover
art' (or img.setAttribute('alt', ...)), set img.loading = 'lazy', and append the
img to the container div (d.appendChild(img)) before w.appendChild(d); this
removes string interpolation and prevents attribute injection. Ensure you update
the code paths that reference artwork and title and remove the inline template
literal usage.
In `@src/hooks/useMarkers.ts`:
- Around line 56-70: nextMarker and previousMarker currently use
activeMarkerIndex directly, causing next to jump to index 0 when
activeMarkerIndex === -1 and previous to do nothing; change both callbacks to
fall back to resolving the target index from currentTime when activeMarkerIndex
=== -1: in nextMarker (function nextMarker) if activeMarkerIndex === -1 find the
first index i in sortedMarkers where sortedMarkers[i].time > currentTime and
call goToMarker(i) (or do nothing if none), otherwise use activeMarkerIndex+1;
in previousMarker (function previousMarker) if activeMarkerIndex === -1 find the
last index i where sortedMarkers[i].time < currentTime and call goToMarker(i)
(or do nothing if none), otherwise use activeMarkerIndex-1; keep references to
sortedMarkers, currentTime, activeMarkerIndex and goToMarker in the hooks'
dependency arrays.
In `@src/hooks/useTabVisibility.ts`:
- Around line 23-46: The effect currently leaves hiddenSinceRef null if the page
is initially hidden, so the first visible transition undercounts hidden time;
inside the useEffect that registers handleVisibilityChange (in
useTabVisibility.ts), initialize hiddenSinceRef.current = Date.now() when the
component mounts and document.hidden is true (guarding for typeof document !==
'undefined' to avoid SSR issues), so handleVisibilityChange computes the full
hiddenDuration; keep the existing handleVisibilityChange, setIsTabVisible, and
onHiddenRef/onVisibleRef usage unchanged—just set hiddenSinceRef.current
appropriately on mount.
In `@src/utils/fairu.ts`:
- Around line 349-360: The milliseconds computation in secondsToFairuTimestamp
can produce 1000 due to Math.round, causing a 4-digit ms field; change the logic
to compute ms using Math.floor((seconds % 1) * 1000) or compute ms with rounding
but clamp to 0..999 and, if you choose rounding and get ms === 1000, set ms = 0
and increment seconds (and propagate the carry into s, m, h using the existing
variables h/m/s in secondsToFairuTimestamp) so the formatted timestamp always
has a three-digit millisecond component.
🧹 Nitpick comments (5)
src/hooks/useCast.ts (1)
54-56: useEffect dependency may not trigger re-evaluation when video element changes.The dependency
[videoRef]tracks the ref object identity (stable across renders), notvideoRef.current. If the underlying video element changes without the ref object changing,setIsSupportedwon't re-run.This is typically fine if the ref is assigned once, but worth noting if dynamic video element swapping is expected.
src/components/controls/ProgressBar/ProgressBar.tsx (1)
177-189: Consider memoizinggetMarkerAtTimeor caching the result.The function iterates over all markers on every hover position update. While typically small arrays, for videos with many markers this could be optimized with
useMemoor binary search if markers are sorted by time.That said, for typical use cases (< 50 markers), this is acceptable.
src/types/video.ts (1)
140-156:OverlayAdtype is used before it's defined.
TabVisibilityConfigreferencesOverlayAdon line 155, butOverlayAdis defined later in this file (line 256). While TypeScript allows this within the same file, consider reordering the interface definitions or adding a forward reference comment for clarity. This is a minor readability concern.src/hooks/useVideo.ts (2)
173-185: Mute toggle logic may not restore original mute state correctly.On line 180, when
muteOnHiddenis true and the video is not already muted,toggleMute()is called. However,wasMutedBeforeHiddenRefis set totrueunconditionally before toggling. This ref should track whether the video was not muted before hiding (to know if we need to unmute on return), but the naming suggests it stores whether it was muted.The current logic:
- If not muted → set
wasMutedBeforeHiddenRef = true→ mute- On return → if
wasMutedBeforeHiddenRefis true → toggle (unmute)This works correctly but the variable name is misleading. Consider renaming to
shouldUnmuteOnVisibleReffor clarity.Suggested naming improvement
- const wasMutedBeforeHiddenRef = useRef(false); + const shouldUnmuteOnVisibleRef = useRef(false);And update usages accordingly at lines 179, 192-194.
187-215: Return-ad trigger emits events even whenplayerEventBusis undefined.On line 212,
playerEventBusRef.current?.emit('triggerReturnAd', ...)is called inside the condition that already checksadEventBusRef.currentexists. However, ifplayerEventBusis not provided butadEventBusis, thetriggerReturnAdevent won't be emitted but the overlay ad will still show. This might be intentional, but consider whether thetriggerReturnAdevent should be emitted unconditionally when showing a return ad (for observability).Otherwise, the tab visibility restoration logic looks correct.
| <button | ||
| onClick={onClick} | ||
| disabled={disabled} | ||
| className={cn( | ||
| 'fp-cast-button', | ||
| 'p-2 rounded-lg', | ||
| 'text-[var(--fp-color-text-secondary)] hover:text-[var(--fp-color-text-primary)]', | ||
| 'hover:bg-[var(--fp-color-surface-hover)]', | ||
| 'transition-colors duration-[var(--fp-transition-fast)]', | ||
| 'focus:outline-none focus:ring-2 focus:ring-[var(--fp-color-accent)]/50', | ||
| disabled && 'opacity-50 cursor-not-allowed', | ||
| className | ||
| )} | ||
| aria-label={isCasting ? labels.stopCast : labels.startCast} | ||
| title={isCasting ? labels.stopCast : labels.startCast} | ||
| > |
There was a problem hiding this comment.
Add type="button" to prevent unintended form submission.
The button element defaults to type="submit". If this component is ever used within a form context, clicking it would submit the form. Explicitly setting type="button" prevents this behavior.
Proposed fix
<button
+ type="button"
onClick={onClick}
disabled={disabled}📝 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.
| <button | |
| onClick={onClick} | |
| disabled={disabled} | |
| className={cn( | |
| 'fp-cast-button', | |
| 'p-2 rounded-lg', | |
| 'text-[var(--fp-color-text-secondary)] hover:text-[var(--fp-color-text-primary)]', | |
| 'hover:bg-[var(--fp-color-surface-hover)]', | |
| 'transition-colors duration-[var(--fp-transition-fast)]', | |
| 'focus:outline-none focus:ring-2 focus:ring-[var(--fp-color-accent)]/50', | |
| disabled && 'opacity-50 cursor-not-allowed', | |
| className | |
| )} | |
| aria-label={isCasting ? labels.stopCast : labels.startCast} | |
| title={isCasting ? labels.stopCast : labels.startCast} | |
| > | |
| <button | |
| type="button" | |
| onClick={onClick} | |
| disabled={disabled} | |
| className={cn( | |
| 'fp-cast-button', | |
| 'p-2 rounded-lg', | |
| 'text-[var(--fp-color-text-secondary)] hover:text-[var(--fp-color-text-primary)]', | |
| 'hover:bg-[var(--fp-color-surface-hover)]', | |
| 'transition-colors duration-[var(--fp-transition-fast)]', | |
| 'focus:outline-none focus:ring-2 focus:ring-[var(--fp-color-accent)]/50', | |
| disabled && 'opacity-50 cursor-not-allowed', | |
| className | |
| )} | |
| aria-label={isCasting ? labels.stopCast : labels.startCast} | |
| title={isCasting ? labels.stopCast : labels.startCast} | |
| > |
🤖 Prompt for AI Agents
In `@src/components/controls/CastButton/CastButton.tsx` around lines 26 - 41, The
CastButton component's JSX button currently lacks an explicit type, causing it
to default to "submit" in forms; update the button element in CastButton.tsx
(the JSX button inside the CastButton component where onClick, disabled,
className, aria-label and title are set) to include type="button" so clicks
won't submit surrounding forms unintentionally.
| <button | ||
| onClick={onClick} | ||
| disabled={disabled} | ||
| className={cn( | ||
| 'fp-pip-button', | ||
| 'p-2 rounded-lg', | ||
| 'text-[var(--fp-color-text-secondary)] hover:text-[var(--fp-color-text-primary)]', | ||
| 'hover:bg-[var(--fp-color-surface-hover)]', | ||
| 'transition-colors duration-[var(--fp-transition-fast)]', | ||
| 'focus:outline-none focus:ring-2 focus:ring-[var(--fp-color-accent)]/50', | ||
| disabled && 'opacity-50 cursor-not-allowed', | ||
| className | ||
| )} | ||
| aria-label={isPictureInPicture ? labels.exitPictureInPicture : labels.enterPictureInPicture} | ||
| title={isPictureInPicture ? labels.exitPictureInPicture : labels.enterPictureInPicture} | ||
| > |
There was a problem hiding this comment.
Add type="button" to prevent unintended form submission.
Same issue as CastButton—the button element defaults to type="submit". Explicitly setting type="button" prevents unintended form submission if this component is used within a form.
Proposed fix
<button
+ type="button"
onClick={onClick}
disabled={disabled}📝 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.
| <button | |
| onClick={onClick} | |
| disabled={disabled} | |
| className={cn( | |
| 'fp-pip-button', | |
| 'p-2 rounded-lg', | |
| 'text-[var(--fp-color-text-secondary)] hover:text-[var(--fp-color-text-primary)]', | |
| 'hover:bg-[var(--fp-color-surface-hover)]', | |
| 'transition-colors duration-[var(--fp-transition-fast)]', | |
| 'focus:outline-none focus:ring-2 focus:ring-[var(--fp-color-accent)]/50', | |
| disabled && 'opacity-50 cursor-not-allowed', | |
| className | |
| )} | |
| aria-label={isPictureInPicture ? labels.exitPictureInPicture : labels.enterPictureInPicture} | |
| title={isPictureInPicture ? labels.exitPictureInPicture : labels.enterPictureInPicture} | |
| > | |
| <button | |
| type="button" | |
| onClick={onClick} | |
| disabled={disabled} | |
| className={cn( | |
| 'fp-pip-button', | |
| 'p-2 rounded-lg', | |
| 'text-[var(--fp-color-text-secondary)] hover:text-[var(--fp-color-text-primary)]', | |
| 'hover:bg-[var(--fp-color-surface-hover)]', | |
| 'transition-colors duration-[var(--fp-transition-fast)]', | |
| 'focus:outline-none focus:ring-2 focus:ring-[var(--fp-color-accent)]/50', | |
| disabled && 'opacity-50 cursor-not-allowed', | |
| className | |
| )} | |
| aria-label={isPictureInPicture ? labels.exitPictureInPicture : labels.enterPictureInPicture} | |
| title={isPictureInPicture ? labels.exitPictureInPicture : labels.enterPictureInPicture} | |
| > |
🤖 Prompt for AI Agents
In `@src/components/controls/PictureInPictureButton/PictureInPictureButton.tsx`
around lines 26 - 41, The button in the PictureInPictureButton component
currently lacks an explicit type, causing it to default to "submit" inside
forms; update the <button> element in PictureInPictureButton (the JSX render for
the component) to include type="button" so it won't trigger form submissions
when clicked, keeping all existing props (onClick, disabled, className,
aria-label, title) intact.
| /** | ||
| * Tab Visibility with Return Ad demo | ||
| * Pause on hidden, resume on visible, show return ad after 3 seconds away | ||
| */ |
There was a problem hiding this comment.
Align the comment with the configured return-ad delay.
Line 2598-2601 vs. Line 2658: The comment says “after 3 seconds,” but returnAdMinHiddenDuration is set to 0 (immediate). Please make the comment match the behavior (or adjust the config).
🛠️ Suggested tweak
- * Pause on hidden, resume on visible, show return ad after 3 seconds away
+ * Pause on hidden, resume on visible, show return ad immediately when returning (0s away)Also applies to: 2655-2659
🤖 Prompt for AI Agents
In `@src/components/VideoPlayer/VideoPlayer.stories.tsx` around lines 2598 - 2601,
Update the block comment for the "Tab Visibility with Return Ad demo" to match
the actual configured behavior: either change the phrase “after 3 seconds” to
reflect that returnAdMinHiddenDuration is set to 0 (immediate), or adjust the
configuration (returnAdMinHiddenDuration) to 3000 if you intend a 3-second
delay; ensure both the comment near the demo header and the similar comment at
lines around the second occurrence (the other comment at 2655-2659) are made
consistent with the chosen behavior.
| if (artwork) { | ||
| const d = document.createElement('div'); | ||
| d.className = 'fl__art'; | ||
| d.innerHTML = `<img src="${artwork}" alt="${title || 'Cover art'}" loading="lazy">`; | ||
| w.appendChild(d); | ||
| } |
There was a problem hiding this comment.
Sanitize user-provided data attributes to prevent XSS.
The artwork and title values are read from data-* attributes and interpolated directly into HTML. If a page allows user-controlled data attributes, an attacker could inject malicious payloads (e.g., " onerror="alert(1)).
Proposed fix using safe DOM APIs
if (artwork) {
const d = document.createElement('div');
d.className = 'fl__art';
- d.innerHTML = `<img src="${artwork}" alt="${title || 'Cover art'}" loading="lazy">`;
+ const img = document.createElement('img');
+ img.src = artwork;
+ img.alt = title || 'Cover art';
+ img.loading = 'lazy';
+ d.appendChild(img);
w.appendChild(d);
}📝 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 (artwork) { | |
| const d = document.createElement('div'); | |
| d.className = 'fl__art'; | |
| d.innerHTML = `<img src="${artwork}" alt="${title || 'Cover art'}" loading="lazy">`; | |
| w.appendChild(d); | |
| } | |
| if (artwork) { | |
| const d = document.createElement('div'); | |
| d.className = 'fl__art'; | |
| const img = document.createElement('img'); | |
| img.src = artwork; | |
| img.alt = title || 'Cover art'; | |
| img.loading = 'lazy'; | |
| d.appendChild(img); | |
| w.appendChild(d); | |
| } |
🧰 Tools
🪛 ast-grep (0.40.5)
[warning] 72-72: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: d.innerHTML = <img src="${artwork}" alt="${title || 'Cover art'}" loading="lazy">
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 72-72: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: d.innerHTML = <img src="${artwork}" alt="${title || 'Cover art'}" loading="lazy">
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
🤖 Prompt for AI Agents
In `@src/embed/loader.ts` around lines 70 - 75, The code currently injects
user-controlled artwork and title into innerHTML (in the block that checks
artwork and creates d with class 'fl__art'), which allows XSS; replace the
innerHTML usage with safe DOM APIs: create an HTMLImageElement
(document.createElement('img')), set its src (img.src = artwork or
img.setAttribute('src', artwork)), set img.alt = title || 'Cover art' (or
img.setAttribute('alt', ...)), set img.loading = 'lazy', and append the img to
the container div (d.appendChild(img)) before w.appendChild(d); this removes
string interpolation and prevents attribute injection. Ensure you update the
code paths that reference artwork and title and remove the inline template
literal usage.
| // Next marker | ||
| const nextMarker = useCallback(() => { | ||
| const nextIndex = activeMarkerIndex + 1; | ||
| if (nextIndex < sortedMarkers.length) { | ||
| goToMarker(nextIndex); | ||
| } | ||
| }, [activeMarkerIndex, sortedMarkers.length, goToMarker]); | ||
|
|
||
| // Previous marker | ||
| const previousMarker = useCallback(() => { | ||
| const prevIndex = activeMarkerIndex - 1; | ||
| if (prevIndex >= 0) { | ||
| goToMarker(prevIndex); | ||
| } | ||
| }, [activeMarkerIndex, goToMarker]); |
There was a problem hiding this comment.
Next/previous jump to the first marker when none is “active.”
When activeMarkerIndex is -1 (e.g., currentTime is mid/late but outside the threshold), nextMarker() goes to index 0 and previousMarker() does nothing. That’s a surprising UX for “next/previous.” Consider resolving the next/previous index from currentTime when there’s no active marker.
🛠️ Proposed fix
// Next marker
const nextMarker = useCallback(() => {
- const nextIndex = activeMarkerIndex + 1;
- if (nextIndex < sortedMarkers.length) {
+ const nextIndex =
+ activeMarkerIndex >= 0
+ ? activeMarkerIndex + 1
+ : sortedMarkers.findIndex((m) => m.time > currentTime);
+ if (nextIndex >= 0 && nextIndex < sortedMarkers.length) {
goToMarker(nextIndex);
}
- }, [activeMarkerIndex, sortedMarkers.length, goToMarker]);
+ }, [activeMarkerIndex, sortedMarkers, currentTime, goToMarker]);
// Previous marker
const previousMarker = useCallback(() => {
- const prevIndex = activeMarkerIndex - 1;
+ let prevIndex = activeMarkerIndex - 1;
+ if (activeMarkerIndex < 0) {
+ for (let i = sortedMarkers.length - 1; i >= 0; i--) {
+ if (sortedMarkers[i].time < currentTime) {
+ prevIndex = i;
+ break;
+ }
+ }
+ }
if (prevIndex >= 0) {
goToMarker(prevIndex);
}
- }, [activeMarkerIndex, goToMarker]);
+ }, [activeMarkerIndex, sortedMarkers, currentTime, goToMarker]);📝 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.
| // Next marker | |
| const nextMarker = useCallback(() => { | |
| const nextIndex = activeMarkerIndex + 1; | |
| if (nextIndex < sortedMarkers.length) { | |
| goToMarker(nextIndex); | |
| } | |
| }, [activeMarkerIndex, sortedMarkers.length, goToMarker]); | |
| // Previous marker | |
| const previousMarker = useCallback(() => { | |
| const prevIndex = activeMarkerIndex - 1; | |
| if (prevIndex >= 0) { | |
| goToMarker(prevIndex); | |
| } | |
| }, [activeMarkerIndex, goToMarker]); | |
| // Next marker | |
| const nextMarker = useCallback(() => { | |
| const nextIndex = | |
| activeMarkerIndex >= 0 | |
| ? activeMarkerIndex + 1 | |
| : sortedMarkers.findIndex((m) => m.time > currentTime); | |
| if (nextIndex >= 0 && nextIndex < sortedMarkers.length) { | |
| goToMarker(nextIndex); | |
| } | |
| }, [activeMarkerIndex, sortedMarkers, currentTime, goToMarker]); | |
| // Previous marker | |
| const previousMarker = useCallback(() => { | |
| let prevIndex = activeMarkerIndex - 1; | |
| if (activeMarkerIndex < 0) { | |
| for (let i = sortedMarkers.length - 1; i >= 0; i--) { | |
| if (sortedMarkers[i].time < currentTime) { | |
| prevIndex = i; | |
| break; | |
| } | |
| } | |
| } | |
| if (prevIndex >= 0) { | |
| goToMarker(prevIndex); | |
| } | |
| }, [activeMarkerIndex, sortedMarkers, currentTime, goToMarker]); |
🤖 Prompt for AI Agents
In `@src/hooks/useMarkers.ts` around lines 56 - 70, nextMarker and previousMarker
currently use activeMarkerIndex directly, causing next to jump to index 0 when
activeMarkerIndex === -1 and previous to do nothing; change both callbacks to
fall back to resolving the target index from currentTime when activeMarkerIndex
=== -1: in nextMarker (function nextMarker) if activeMarkerIndex === -1 find the
first index i in sortedMarkers where sortedMarkers[i].time > currentTime and
call goToMarker(i) (or do nothing if none), otherwise use activeMarkerIndex+1;
in previousMarker (function previousMarker) if activeMarkerIndex === -1 find the
last index i where sortedMarkers[i].time < currentTime and call goToMarker(i)
(or do nothing if none), otherwise use activeMarkerIndex-1; keep references to
sortedMarkers, currentTime, activeMarkerIndex and goToMarker in the hooks'
dependency arrays.
| const [isTabVisible, setIsTabVisible] = useState( | ||
| typeof document !== 'undefined' ? !document.hidden : true | ||
| ); | ||
| const hiddenSinceRef = useRef<number | null>(null); | ||
|
|
||
| // Store callbacks in refs so the event listener always calls the latest version | ||
| const onHiddenRef = useRef(options.onHidden); | ||
| const onVisibleRef = useRef(options.onVisible); | ||
| onHiddenRef.current = options.onHidden; | ||
| onVisibleRef.current = options.onVisible; | ||
|
|
||
| useEffect(() => { | ||
| const handleVisibilityChange = () => { | ||
| if (document.hidden) { | ||
| hiddenSinceRef.current = Date.now(); | ||
| setIsTabVisible(false); | ||
| onHiddenRef.current?.(); | ||
| } else { | ||
| const hiddenDuration = hiddenSinceRef.current | ||
| ? (Date.now() - hiddenSinceRef.current) / 1000 | ||
| : 0; | ||
| hiddenSinceRef.current = null; | ||
| setIsTabVisible(true); | ||
| onVisibleRef.current?.(hiddenDuration); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/hooks/useTabVisibility.tsRepository: sushidev-team/fairu-player
Length of output: 2305
Initialize hiddenSinceRef when tab starts hidden.
If the page loads hidden (background tab or SSR hydration), hiddenSinceRef remains null and hiddenDuration computes as 0 on the first visibility change, undercounting actual hidden time. Initialize hiddenSinceRef in the effect based on the initial visibility state to capture the full hidden duration from page load.
🛠️ Proposed fix
useEffect(() => {
+ const initiallyHidden = document.hidden;
+ hiddenSinceRef.current = initiallyHidden ? Date.now() : null;
+ setIsTabVisible(!initiallyHidden);
+
const handleVisibilityChange = () => {
if (document.hidden) {
hiddenSinceRef.current = Date.now();🤖 Prompt for AI Agents
In `@src/hooks/useTabVisibility.ts` around lines 23 - 46, The effect currently
leaves hiddenSinceRef null if the page is initially hidden, so the first visible
transition undercounts hidden time; inside the useEffect that registers
handleVisibilityChange (in useTabVisibility.ts), initialize
hiddenSinceRef.current = Date.now() when the component mounts and
document.hidden is true (guarding for typeof document !== 'undefined' to avoid
SSR issues), so handleVisibilityChange computes the full hiddenDuration; keep
the existing handleVisibilityChange, setIsTabVisible, and
onHiddenRef/onVisibleRef usage unchanged—just set hiddenSinceRef.current
appropriately on mount.
| export function secondsToFairuTimestamp(seconds: number): string { | ||
| const h = Math.floor(seconds / 3600); | ||
| const m = Math.floor((seconds % 3600) / 60); | ||
| const s = Math.floor(seconds % 60); | ||
| const ms = Math.round((seconds % 1) * 1000); | ||
|
|
||
| return [ | ||
| String(h).padStart(2, '0'), | ||
| String(m).padStart(2, '0'), | ||
| String(s).padStart(2, '0'), | ||
| ].join(':') + '.' + String(ms).padStart(3, '0'); | ||
| } |
There was a problem hiding this comment.
Potential off-by-one in milliseconds calculation.
Math.round((seconds % 1) * 1000) can produce 1000 when seconds has a fractional part very close to 1 (e.g., 0.9999), resulting in a 4-digit millisecond string like "00:00:00.1000".
Proposed fix
- const ms = Math.round((seconds % 1) * 1000);
+ const ms = Math.min(999, Math.floor((seconds % 1) * 1000));🤖 Prompt for AI Agents
In `@src/utils/fairu.ts` around lines 349 - 360, The milliseconds computation in
secondsToFairuTimestamp can produce 1000 due to Math.round, causing a 4-digit ms
field; change the logic to compute ms using Math.floor((seconds % 1) * 1000) or
compute ms with rounding but clamp to 0..999 and, if you choose rounding and get
ms === 1000, set ms = 0 and increment seconds (and propagate the carry into s,
m, h using the existing variables h/m/s in secondsToFairuTimestamp) so the
formatted timestamp always has a three-digit millisecond component.
1 participant
PiP, Event Tracker & Cast Support
Summary
Cast: Browser Compatibility
webkitShowPlaybackTargetPickerremote.prompt)New Files
src/hooks/usePictureInPicture.ts— PiP hooksrc/hooks/useTabVisibility.ts— Tab visibility hooksrc/hooks/useMarkers.ts— Timeline markers hooksrc/hooks/useCast.ts— Cast hook (AirPlay + Chromecast)src/utils/PlayerEventBus.ts— Typed event bussrc/components/controls/PictureInPictureButton/— PiP buttonsrc/components/controls/CastButton/— Cast buttonsrc/components/markers/MarkerList/— Marker componentChanged Files
src/types/video.ts—castinVideoFeatures,isCastinginVideoState,toggleCastinVideoControlssrc/types/labels.ts—startCast/stopCastlabelssrc/hooks/useVideo.ts— Integration of PiP, cast, tab visibility, markerssrc/components/VideoPlayer/VideoControls/VideoControls.tsx— PiP & cast buttonssrc/components/VideoPlayer/VideoPlayer.tsx— Event bus callbacks, cast change handlersrc/context/VideoContext.tsx— New callbacks passed through touseVideosrc/index.ts— Barrel exports for all new modulessrc/hooks/index.ts,src/components/controls/index.tsStorybook
New stories under
Components/VideoPlayer:features: { cast: true }