Conversation
Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
…ording Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
…target select Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
|
Paragon Review Skipped Hi @richiemcilroy! Your Polarity credit balance is insufficient to complete this review. Please visit https://home.polarity.cc to add more credits and continue using Paragon reviews. |
| setHlsInitFailed(true); | ||
| } | ||
| }, [playbackSrc, isLiveSegments, videoRef.current]); | ||
| }, [playbackSrc, isLiveSegments, reloadPlayback, router, videoLoaded]); |
There was a problem hiding this comment.
HLS player destroyed and recreated when
videoLoaded becomes true
Adding videoLoaded to the dependency array means the entire effect — which creates, configures, and attaches the HLS instance — re-runs every time videoLoaded flips from false to true. This causes the cleanup to destroy the active HLS instance and immediately create a new one.
For live segments (isLiveSegments=true), this happens inside Hls.Events.FRAG_LOADED: the handler sets videoLoaded = true, which triggers the re-run mid-stream. The HLS connection is torn down and restarted exactly once, producing a visible interruption when viewing a recording that is still in progress.
The intent appears to be giving the FRAG_LOADED closure a fresh value of videoLoaded, but a ref avoids the re-init:
const videoLoadedRef = useRef(videoLoaded);
useEffect(() => { videoLoadedRef.current = videoLoaded; }, [videoLoaded]);
// Inside the HLS effect:
hls.on(Hls.Events.FRAG_LOADED, () => {
if (isLiveSegments && !videoLoadedRef.current) {
setVideoLoaded(true);
...
}
});Then remove videoLoaded from the dependency array.
| }, [playbackSrc, isLiveSegments, reloadPlayback, router, videoLoaded]); | |
| }, [playbackSrc, isLiveSegments, reloadPlayback, router]); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/app/s/[videoId]/_components/HLSVideoPlayer.tsx
Line: 354
Comment:
**HLS player destroyed and recreated when `videoLoaded` becomes `true`**
Adding `videoLoaded` to the dependency array means the entire effect — which creates, configures, and attaches the HLS instance — re-runs every time `videoLoaded` flips from `false` to `true`. This causes the cleanup to destroy the active HLS instance and immediately create a new one.
For live segments (`isLiveSegments=true`), this happens inside `Hls.Events.FRAG_LOADED`: the handler sets `videoLoaded = true`, which triggers the re-run mid-stream. The HLS connection is torn down and restarted exactly once, producing a visible interruption when viewing a recording that is still in progress.
The intent appears to be giving the `FRAG_LOADED` closure a fresh value of `videoLoaded`, but a ref avoids the re-init:
```typescript
const videoLoadedRef = useRef(videoLoaded);
useEffect(() => { videoLoadedRef.current = videoLoaded; }, [videoLoaded]);
// Inside the HLS effect:
hls.on(Hls.Events.FRAG_LOADED, () => {
if (isLiveSegments && !videoLoadedRef.current) {
setVideoLoaded(true);
...
}
});
```
Then remove `videoLoaded` from the dependency array.
```suggestion
}, [playbackSrc, isLiveSegments, reloadPlayback, router]);
```
How can I resolve this? If you propose a fix, please make it concise.| onClick={() => { | ||
| if (!selectedWindow()) { | ||
| setOriginalCameraBounds(null); | ||
| setLockedIcon(windowIcon.data ?? null); |
There was a problem hiding this comment.
windowIcon.data is an accessor; storing it directly means lockedIcon() becomes a function and src ends up receiving a function. Probably want the resolved value here.
| setLockedIcon(windowIcon.data ?? null); | |
| setLockedIcon(windowIcon.data?.() ?? null); |
| } | ||
| }); | ||
|
|
||
| hls.on(Hls.Events.FRAG_LOADED, () => { |
There was a problem hiding this comment.
Avoid reading videoLoaded inside the HLS setup effect; it forces videoLoaded into deps and can cause the whole Hls instance to be torn down/recreated when the first fragment arrives. Since setVideoLoaded(true) is idempotent, you can simplify this handler.
| hls.on(Hls.Events.FRAG_LOADED, () => { | |
| hls.on(Hls.Events.FRAG_LOADED, () => { | |
| if (isLiveSegments) { | |
| setVideoLoaded(true); | |
| if (!hasPlayedOnceRef.current) { | |
| setShowPlayButton(true); | |
| } | |
| } | |
| }); |
| setHlsInitFailed(true); | ||
| } | ||
| }, [playbackSrc, isLiveSegments, videoRef.current]); | ||
| }, [playbackSrc, isLiveSegments, reloadPlayback, router, videoLoaded]); |
There was a problem hiding this comment.
videoLoaded in this dependency array will re-run the HLS init effect when setVideoLoaded(true) fires (live segments: FRAG_LOADED), which looks like it would restart playback.
| }, [playbackSrc, isLiveSegments, reloadPlayback, router, videoLoaded]); | |
| }, [playbackSrc, isLiveSegments, reloadPlayback, router]); |
| return; | ||
| } | ||
|
|
||
| if (isMounted) setPlatform("macos"); |
There was a problem hiding this comment.
This sets platform to macos for any non-Windows/Linux UA (Android/iOS/ChromeOS, etc). If this is only meant to distinguish desktop platforms, it might be safer to only set macos when the UA actually contains Mac, otherwise keep platform as null.
| if (isMounted) setPlatform("macos"); | |
| if (userAgent.includes("Mac")) { | |
| if (isMounted) setPlatform("macos"); | |
| } else { | |
| if (isMounted) setPlatform(null); | |
| } |
Greptile Summary
This PR is a patch release (0.4.84) with a collection of bug fixes across the desktop app and web player. Key improvements include: camera preview state handling after resume, correct recording controls positioning for window/area capture targets, camera-only mode upload and screenshot fixes, HLS live segments retry logic, and macOS dock visibility synchronization.
Confidence Score: 4/5
Safe to merge after addressing the HLS re-initialization bug; all other changes are well-scoped fixes.
One P1 finding: adding
videoLoadedto the HLS setup effect dependency array causes the HLS player to be destroyed and recreated when the first fragment loads during live-recording playback, producing a noticeable interruption. All other changes (camera preview state machine, recording controls positioning, camera-only mode, force_exit, dock visibility) are correct bug fixes.apps/web/app/s/[videoId]/_components/HLSVideoPlayer.tsx — the
videoLoadeddependency in the HLS setup effectVulnerabilities
No security concerns identified. The
unsafeblock inforce_exitis a deliberate POSIX_exitcall to bypass atexit handlers on shutdown — it does not introduce a security vulnerability.Important Files Changed
videoLoadedin HLS setup deps causes the HLS instance to be torn down and restarted when the video first loads, breaking live-segment playback continuity.needs_full_reconfigureflag and GPU surface reconfiguration.std::process::exitwith a POSIX_exit-backedforce_exitto avoid macOS shutdown hangs; guards camera cleanup/restore behindmain_window_visibleandcamera_cleanup_doneto prevent spurious camera feed preservation.calculate_recording_controls_position_for_targetto position recording controls relative to the captured window or area bounds rather than always using cursor position.capture_targetfield toInProgressRecording, uses it for smart controls positioning, and callssync_macos_dock_visibilityat panel creation/error paths for correct macOS dock state.desktopMP4upload mode for camera-only recordings, passescapture_targettoInProgressRecording, and switches screenshot source for camera-only to the output MP4 path.output.mp4as healthy, covering camera-only recordings that produce an MP4 instead of segments.Sequence Diagram
sequenceDiagram participant UI as Desktop UI participant Rec as recording.rs participant Win as windows.rs participant FW as fake_window.rs participant Cam as camera.rs UI->>Rec: start_recording(inputs) Rec->>Rec: Determine upload_mode Rec->>Win: ShowCapWindow::InProgressRecording Win->>FW: calculate_recording_controls_position_for_target FW-->>Win: pos_x, pos_y Win->>Win: set_position + sync_macos_dock_visibility Note over Cam: On resume from pause Cam->>Cam: drain camera_rx buffer Cam->>Cam: needs_full_reconfigure=true Cam->>Cam: update_state_uniforms Cam->>Cam: resize_window + reconfigure_gpu_surfacePrompt To Fix All With AI
Reviews (1): Last reviewed commit: "style(desktop): apply clippy suggestion ..." | Re-trigger Greptile