Fix/videoplayer bugs#1339
Conversation
… local source issues in NetflixStylePlayer
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough
ChangesNetflixStylePlayer Bug Fixes and Tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 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.
Actionable comments posted: 2
🤖 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 `@frontend/src/components/VideoPlayer/__tests__/NetflixStylePlayer.test.tsx`:
- Around line 11-21: The Slider mock component in the test file has its props
parameter typed as `any`, which weakens type safety. Replace the `any` type
annotation in the Slider function signature with an explicit interface or type
that properly defines all the props being used: onValueChange (function
accepting an array of numbers), value (array of numbers), min (number), max
(number), and step (number). This will ensure proper contract checking and align
with the coding guidelines requiring explicit types instead of `any`.
In `@frontend/src/components/VideoPlayer/NetflixStylePlayer.tsx`:
- Around line 109-117: The newTime value calculated in the
handleProgressBarClick function is not being validated or clamped to valid media
bounds before being assigned to videoRef.current.currentTime. This can result in
out-of-range seek values (negative or exceeding duration). Before assigning the
newTime to videoRef.current.currentTime, clamp it to a valid range between 0 and
videoRef.current.duration using a Math.min and Math.max approach. Apply the same
clamping logic to the other seek function mentioned in the comment (around lines
129-134) to ensure all seek operations produce valid, bounded time values.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1d9ef488-f1bc-4490-b0d9-6aaa60d29715
📒 Files selected for processing (2)
frontend/src/components/VideoPlayer/NetflixStylePlayer.tsxfrontend/src/components/VideoPlayer/__tests__/NetflixStylePlayer.test.tsx
|
Hey, thanks for picking this up. I went through the diff and tested the asset protocol behavior locally. Most of it looks good, but I think there's one blocking issue with the local file fix, plus a few smaller suggestions. The main thing is that bug 1 (local file loading) needs a CSP change too, not just convertFileSrc. convertFileSrc returns an asset protocol URL ( img-src already lists That is why images load through convertFileSrc today but a video won't. In Tauri each CSP directive has to whitelist the asset protocol separately, and media-src never got it because there was no video feature before. The fix is a one line change in tauri.conf.json, mirroring img-src: One thing worth checking: this is the part the unit tests can't catch, since jsdom doesn't enforce CSP or implement the asset protocol, so a green test suite won't prove the video actually loads. The other four fixes are visible in a normal browser, but bug 1 only shows up in the real webview. Could you run A few smaller, non blocking things:
const togglePlay = () => {
const video = videoRef.current;
if (!video) return;
if (video.paused) {
video.play().catch(() => {});
} else {
video.pause();
}
};
const resolvedSrc = useMemo(() => convertFileSrc(videoSrc), [videoSrc]);
if (!Number.isFinite(timeInSeconds)) {
return '0:00';
}
The only thing I would actually block on is the media-src CSP change. Without it bug 1 isn't really fixed in the running app even though the component code is correct. Everything else is optional polish. Once that line is added and a local file plays in tauri dev, this looks good. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@frontend/src-tauri/tauri.conf.json`:
- Line 69: Add an automated test that reads the tauri.conf.json file and
validates the Content Security Policy configuration. The test should extract the
security.csp value and assert that the media-src directive contains both asset:
and http://asset.localhost to prevent regressions in Tauri media loading
functionality. This test should be included in the Jest test suite alongside
other configuration validation tests to ensure the CSP contract for media
playback remains protected against future changes.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4e6f5bd4-0fe5-40d6-8f48-0ca2c0809261
📒 Files selected for processing (3)
frontend/src-tauri/tauri.conf.jsonfrontend/src/components/VideoPlayer/NetflixStylePlayer.tsxfrontend/src/components/VideoPlayer/__tests__/NetflixStylePlayer.test.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/components/VideoPlayer/NetflixStylePlayer.tsx
- frontend/src/components/VideoPlayer/tests/NetflixStylePlayer.test.tsx
3 participants
Addressed Issues:
Fixes #1329
Screenshots/Recordings:
No changes were made to the overall UI layout or styling. However, functional interface fixes are visible to the user:
0:00 / 0:00(and updates on load) instead ofNaN:NaN.Additional Notes:
This PR resolves all five desync/rendering bugs in the
NetflixStylePlayercomponent:convertFileSrcfor Tauri compatibility.fullscreenchangeevent on the document level, syncingisFullscreenstatus correctly when exited viaEsc.currentTimeanddurationand formatted the output usingformatTimewhich falls back to0:00for non-finite values before video metadata loads. AddedonLoadedMetadataandonDurationChangeto update these states reactively.handleVolumeChangeto correctly unmute the video state and reference (videoRef.current.muted = false) when the volume slider is moved to a value greater than0.onEndedto automatically reset the playing state back tofalsewhen video playback ends.Also added a complete Jest unit test suite covering all 5 scenarios to prevent regressions.
AI Usage Disclosure:
Check one of the checkboxes below:
I have used the following AI models and tools: Gemini 3.5 Flash (via Antigravity IDE)
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Security