Add live RTMP streaming (e.g. Youtube Live)#662
Closed
marcusschiesser wants to merge 3 commits into
Closed
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds end-to-end live streaming: domain presets and validation, renderer capture/composite/recorder hook, Electron IPC that spawns a bundled FFmpeg process and accepts binary chunks, HUD expansion control and preview refresh, and UI to configure/start/stop streams. ChangesLive Streaming Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 853a6553c3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
src/components/launch/LaunchWindow.tsx (1)
330-336: 💤 Low valuecleanup always resets expanded to false even when dialog is still open
this effect's cleanup runs on every re-render when deps change, not just unmount. if
isLanguageMenuOpenchanges whileliveSetupOpenis still true, the cleanup will fire and set expanded to false before the new effect runs. this could cause a brief flicker where the HUD collapses and re-expands. lowkey might not be noticeable in practice, but a guard in cleanup would be cleaner:useEffect(() => { window.electronAPI?.setHudOverlayExpanded?.(liveSetupOpen); setHudMouseEventsEnabled(liveSetupOpen || isLanguageMenuOpen); - return () => { - window.electronAPI?.setHudOverlayExpanded?.(false); - }; }, [isLanguageMenuOpen, liveSetupOpen, setHudMouseEventsEnabled]); + + useEffect(() => { + return () => { + window.electronAPI?.setHudOverlayExpanded?.(false); + }; + }, []);or just remove the cleanup entirely if the expanded state is always explicitly set.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/launch/LaunchWindow.tsx` around lines 330 - 336, The cleanup in the useEffect always calls window.electronAPI?.setHudOverlayExpanded?.(false) which can run on dependency changes and briefly collapse the HUD; update the cleanup to only call setHudOverlayExpanded(false) when both liveSetupOpen and isLanguageMenuOpen are false (i.e., guard against collapsing when a dialog is still open), or remove the cleanup entirely if expanded is always explicitly set elsewhere; locate the effect using useEffect and the call to window.electronAPI?.setHudOverlayExpanded and adjust the cleanup logic around liveSetupOpen and isLanguageMenuOpen.src/components/launch/LiveStreamSetupDialog.tsx (2)
138-191: 💤 Low valuewebcam cleanup looks solid but video.play() error is swallowed
the cleanup logic for stopping tracks and clearing srcObject is good. one small thing - line 174 catches play errors with
() => undefinedwhich is fine for autoplay policy failures, but you might want to at least log them during dev for debugging:- video.play().catch(() => undefined); + video.play().catch((err) => { + if (import.meta.env.DEV) { + console.debug("Webcam preview autoplay blocked:", err); + } + });not a big deal since it's just a preview, but could help diagnose issues.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/launch/LiveStreamSetupDialog.tsx` around lines 138 - 191, The video.play() catch currently swallows errors in the useEffect acquire flow (see acquire, webcamVideoRef, and video.play), so change the noop catch (() => undefined) to log the error in non-production/dev builds—e.g., catch((err) => { if (process.env.NODE_ENV !== 'production') console.debug('webcam preview play failed:', err); })—so autoplay-related failures are visible during development while remaining silent in production; ensure this logging is only added where video.play() is called (inside the onloadedmetadata handler).
222-238: ⚖️ Poor tradeoffclosing dialog before stream starts could feel janky on slow networks
the flow here is: validate → close dialog → wait 220ms → call onStart → if failed, reopen dialog. on a slow FFmpeg spawn or network hiccup, the user sees the dialog close, then potentially reopen with an error. the UX might feel more polished if you kept the dialog open with a "starting" state until confirmed, then closed on success.
that said, i understand the 220ms delay is probably to let the dialog animation complete before capturing. if that's intentional for visual reasons, this is fine as-is.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/launch/LiveStreamSetupDialog.tsx` around lines 222 - 238, The current handleStart closes the dialog (onOpenChange(false)) before awaiting delay/ onStart which can feel janky on slow networks; change handleStart so it validates, setStarting(true) (to disable inputs/show spinner), await delay(START_CAPTURE_DELAY_MS) and then call onStart({ serverUrl, streamKey, layout}) and only call onOpenChange(false) if onStart returns true; if onStart fails, leave the dialog open and setError appropriately and setStarting(false). Keep validateLiveStreamDestination, setError, setStarting, delay, START_CAPTURE_DELAY_MS and onStart usage but move the onOpenChange(false) to run only after a successful start and ensure the UI shows the "starting" state while awaiting.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@electron/ipc/handlers.ts`:
- Around line 200-224: The current timeoutPromise sends SIGTERM and immediately
causes the handler to return success when Promise.race picks the timeout path;
instead, after sending SIGTERM in timeoutPromise (or when result.signal ===
"SIGTERM"), wait for the actual process exit by awaiting the existing
exitPromise (or chain a short secondary timeout) before considering the stop
successful. Update the logic around timeoutPromise/exitPromise and the result
check in the handler so that proc.kill("SIGTERM") is issued but the function
only returns { success: true } once exitPromise has resolved with code 0 or a
terminating signal; keep existing cleanup of liveStreamProcess,
liveStreamStopping, and liveStreamStderr intact and use those symbols
(timeoutPromise, exitPromise, proc, liveStreamProcess, liveStreamStopping,
liveStreamStderr) to locate and modify the code.
In `@electron/windows.ts`:
- Line 58: The code currently uses screen.getPrimaryDisplay() (const { workArea
} = screen.getPrimaryDisplay()) which forces the expanded HUD to recenter on the
primary monitor; instead retrieve the display that actually contains the HUD
window by getting the HUD BrowserWindow's bounds (e.g., hudWindow or
mainWindow). Call screen.getDisplayMatching(hudWindow.getBounds()) (or
screen.getDisplayNearestPoint) and use that display.workArea for positioning;
update both the occurrence at the shown line and the similar block around lines
69–73 so the HUD stays on the current monitor when expanded.
In `@src/components/launch/LaunchWindow.tsx`:
- Around line 381-407: The call to startLiveStream can throw and is currently
unhandled; update handleStartLiveStream to catch any exceptions from
startLiveStream (e.g., wrap the await startLiveStream(config) in try/catch), log
the error (console.warn/processLogger) and return a safe failure value (false or
an error object) so it never throws; also update the caller(s) that invoke
handleStartLiveStream (the async fire-and-forget invocation in the UI) to either
await the returned result and handle failure or add its own try/catch to
surface/log errors to the user. Ensure you reference the functions
handleStartLiveStream and startLiveStream when making these changes.
In `@src/components/launch/LiveStreamSetupDialog.tsx`:
- Around line 371-395: The onPointerLeave handler on the video element can
prematurely call handleWebcamPointerUp and end drags when the pointer briefly
exits the element; update the event bindings on the video (where webcamVideoRef
is used and handlers
handleWebcamPointerDown/handleWebcamPointerMove/handleWebcamPointerUp are
referenced) to stop using onPointerLeave and instead rely on onPointerUp and
onPointerCancel (or add an onPointerCancel that calls handleWebcamPointerUp) so
dragging only ends on pointerup/pointercancel while keeping pointer capture in
handleWebcamPointerDown.
In `@src/hooks/useLiveStreamer.ts`:
- Around line 474-485: recorder.ondataavailable handler can throw rejected
promises from event.data.arrayBuffer() or
window.electronAPI.writeLiveStreamChunk causing unhandled rejections; wrap the
async work in a try/catch (or append .catch) inside the handler used by
recorder.ondataavailable to catch errors, log/show them via toast.error (similar
messaging as existing failure path), set/inspect stopping.current and call await
stopLiveStream() on error, and ensure any thrown error from arrayBuffer() or
writeLiveStreamChunk is handled to avoid leaving the stream running.
- Around line 378-381: The screen video track created in useLiveStreamer (the
screenTrack variable obtained from screenStream.getVideoTracks()[0]) can end
externally; attach an onended/ended event handler to that track to trigger the
same shutdown/cleanup used when the user stops streaming (call the hook's
existing stop/cleanup function such as stopLiveStream or whatever cleans up
streams, state and timers), remove the handler when replacing tracks, and ensure
you also handle cases where screenTrack is null by early-returning; this ensures
the hook auto-stops and clears state when the browser or user revokes screen
share.
---
Nitpick comments:
In `@src/components/launch/LaunchWindow.tsx`:
- Around line 330-336: The cleanup in the useEffect always calls
window.electronAPI?.setHudOverlayExpanded?.(false) which can run on dependency
changes and briefly collapse the HUD; update the cleanup to only call
setHudOverlayExpanded(false) when both liveSetupOpen and isLanguageMenuOpen are
false (i.e., guard against collapsing when a dialog is still open), or remove
the cleanup entirely if expanded is always explicitly set elsewhere; locate the
effect using useEffect and the call to window.electronAPI?.setHudOverlayExpanded
and adjust the cleanup logic around liveSetupOpen and isLanguageMenuOpen.
In `@src/components/launch/LiveStreamSetupDialog.tsx`:
- Around line 138-191: The video.play() catch currently swallows errors in the
useEffect acquire flow (see acquire, webcamVideoRef, and video.play), so change
the noop catch (() => undefined) to log the error in non-production/dev
builds—e.g., catch((err) => { if (process.env.NODE_ENV !== 'production')
console.debug('webcam preview play failed:', err); })—so autoplay-related
failures are visible during development while remaining silent in production;
ensure this logging is only added where video.play() is called (inside the
onloadedmetadata handler).
- Around line 222-238: The current handleStart closes the dialog
(onOpenChange(false)) before awaiting delay/ onStart which can feel janky on
slow networks; change handleStart so it validates, setStarting(true) (to disable
inputs/show spinner), await delay(START_CAPTURE_DELAY_MS) and then call
onStart({ serverUrl, streamKey, layout}) and only call onOpenChange(false) if
onStart returns true; if onStart fails, leave the dialog open and setError
appropriately and setStarting(false). Keep validateLiveStreamDestination,
setError, setStarting, delay, START_CAPTURE_DELAY_MS and onStart usage but move
the onOpenChange(false) to run only after a successful start and ensure the UI
shows the "starting" state while awaiting.
🪄 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: 26b2c23d-a123-4ad1-85b3-a82cd60c5774
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
electron/electron-env.d.tselectron/ipc/handlers.tselectron/preload.tselectron/windows.tspackage.jsonsrc/components/launch/LaunchWindow.tsxsrc/components/launch/LiveStreamSetupDialog.tsxsrc/hooks/useLiveStreamer.tssrc/lib/liveStream.test.tssrc/lib/liveStream.ts
marcusschiesser
force-pushed
the
codex/live-rtmp-setup
branch
from
853a655 to
05c4154
Compare
Contributor
Author
|
@siddharthvaddem here's a screenshot how this looks like: |
marcusschiesser
changed the title
Contributor
Author
|
closing - made a fork now for streaming: https://github.com/marcusschiesser/openstream |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
Summary by CodeRabbit
New Features
Tests