fix(recorder): support for "ready to record" signal#2169
fix(recorder): support for "ready to record" signal#2169
Conversation
|
oliverlaz
changed the title
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughEnhanced egress readiness detection by checking video playback state immediately (before the Changes
Sequence Diagram(s)sequenceDiagram
participant Layout as CustomSkoolStreamLayout
participant Hook as useNotifyEgressReady
participant Video as VideoElement
participant Egress as EgressNotifier
Layout->>Hook: invoke hook / register spy
Hook->>Video: query playback state (currentTime, paused, ended)
alt Video already playing
Video-->>Hook: returns playing
Hook->>Egress: notifyReady(true)
else Not yet playing
Hook->>Video: attach 'play' listener
Video-->>Hook: 'play' event fires
Hook->>Egress: notifyReady(true)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
sample-apps/react/egress-composite/src/components/layouts/egressReady.ts (1)
21-30: Good fix for the missed play event scenario.The immediate playback check correctly handles the case where the video is already playing when the effect runs. The conditions
currentTime > 0 && !paused && !endedare appropriate for detecting active playback.Minor optimization: when
isAlreadyPlayingis true, theplayevent listener on lines 28-29 becomes redundant since readiness is already signaled. Consider returning early to avoid unnecessary listener setup.♻️ Optional: Skip listener when already playing
if (isPublishingVideoTrack && videoElement) { // video element for participants with video const isAlreadyPlaying = videoElement.currentTime > 0 && !videoElement.paused && !videoElement.ended; - if (isAlreadyPlaying) notifyEgressReady(true); + if (isAlreadyPlaying) { + notifyEgressReady(true); + return; // No need to listen for 'play' event + } const onPlay = () => notifyEgressReady(true); videoElement.addEventListener('play', onPlay, { once: true }); return () => videoElement.removeEventListener('play', onPlay); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sample-apps/react/egress-composite/src/components/layouts/egressReady.ts` around lines 21 - 30, When the immediate playback check in the effect detects that the video is already playing (using isPublishingVideoTrack, videoElement and the isAlreadyPlaying condition), return early after calling notifyEgressReady(true) so you don't add the redundant 'play' event listener (onPlay) or set up unnecessary cleanup; modify the block around isAlreadyPlaying to call notifyEgressReady(true) and then return before the videoElement.addEventListener(...) line to skip listener registration.sample-apps/react/egress-composite/src/hooks/useNotifyEgress.tsx (1)
33-38: Good use of functional state update pattern.The functional update
setIsReady((current) => ...)correctly avoids stale closure issues and the conditional logging prevents duplicate messages whennotifyEgressReady(true)is called multiple times (e.g., from both the immediate check and theplayevent).One consideration: the
console.logon line 35 will execute in production builds. As per coding guidelines, production logging should be gated behind environment flags.🔧 Optional: Gate logging behind an environment check
const spyIsReady = useCallback((value: boolean) => { setIsReady((current) => { - if (current !== value) console.log('Egress is ready', value); + if (current !== value && process.env.NODE_ENV !== 'production') { + console.log('Egress is ready', value); + } return value; }); }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sample-apps/react/egress-composite/src/hooks/useNotifyEgress.tsx` around lines 33 - 38, The console.log in spyIsReady should be gated so it doesn't run in production; update the spyIsReady callback (which calls setIsReady) to only log when a dev/debug flag is enabled (e.g., process.env.NODE_ENV !== 'production' or a dedicated DEBUG/LOGGING feature flag) before calling console.log('Egress is ready', value); keep the functional state update and conditional compare but wrap the log behind that environment/flag check so notifyEgressReady/play-triggered calls no longer emit production logs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@sample-apps/react/egress-composite/src/components/layouts/egressReady.ts`:
- Around line 21-30: When the immediate playback check in the effect detects
that the video is already playing (using isPublishingVideoTrack, videoElement
and the isAlreadyPlaying condition), return early after calling
notifyEgressReady(true) so you don't add the redundant 'play' event listener
(onPlay) or set up unnecessary cleanup; modify the block around isAlreadyPlaying
to call notifyEgressReady(true) and then return before the
videoElement.addEventListener(...) line to skip listener registration.
In `@sample-apps/react/egress-composite/src/hooks/useNotifyEgress.tsx`:
- Around line 33-38: The console.log in spyIsReady should be gated so it doesn't
run in production; update the spyIsReady callback (which calls setIsReady) to
only log when a dev/debug flag is enabled (e.g., process.env.NODE_ENV !==
'production' or a dedicated DEBUG/LOGGING feature flag) before calling
console.log('Egress is ready', value); keep the functional state update and
conditional compare but wrap the log behind that environment/flag check so
notifyEgressReady/play-triggered calls no longer emit production logs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 42d143df-d71a-457c-a4eb-8d382c2e43b1
📒 Files selected for processing (3)
sample-apps/react/egress-composite/src/components/layouts/egressReady.tssample-apps/react/egress-composite/src/components/layouts/index.tsxsample-apps/react/egress-composite/src/hooks/useNotifyEgress.tsx
1 participant
💡 Overview
Ensure Skool's layout can signal recording readiness.
Ref: https://github.com/skooldev/skool-stream-layout/pull/4
🎫 Ticket: https://linear.app/stream/issue/REACT-907/fix-missed-egress-ready-play-event-in-custom-layouts
Summary by CodeRabbit