fix: default preferHls to true on Safari#4236
Conversation
Safari has poor DASH/MSE support, causing error 3016 (VIDEO_ERROR / PIPELINE_ERROR_DECODE) when playing videos via generated DASH manifests. Since Safari natively supports HLS, default preferHls to true on Safari while keeping the existing default (false) for other browsers. Users can still override this in preferences.
|
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)
📝 WalkthroughWalkthroughVideoPlayer now detects Safari via navigator.userAgent and uses that detection as the default for getPreferenceBoolean("preferHls", ...) in both the MSE/audioStreams and DASH fallback branches. CI workflows pin pnpm action to v10 and set actions/setup-node node-version to 22. ChangesSafari-aware HLS preference defaulting
CI Node.js version pinning
Sequence DiagramsequenceDiagram
participant Browser
participant VideoPlayer
participant Preferences
Browser->>VideoPlayer: loadVideo()
VideoPlayer->>Browser: read navigator.userAgent (isSafari)
VideoPlayer->>Preferences: getPreferenceBoolean("preferHls", isSafari)
Preferences-->>VideoPlayer: preferHls value
VideoPlayer->>VideoPlayer: select HLS or DASH playback path
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
pnpm@latest now requires Node.js v22.13+. Without an explicit node-version, the runner uses its default which is too old.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/VideoPlayer.vue (1)
604-604: 💤 Low valueConsider adding a comment explaining the Safari detection logic.
The
isSafariflag is used consistently in both places to intelligently default HLS preference based on browser detection. This is a good solution to the Safari DASH playback issue.For future maintainability, consider adding a brief comment above line 590 explaining why Safari needs this special handling (e.g., "Safari has poor MSE/DASH support, default to native HLS playback").
📝 Optional: Add explanatory comment
const MseSupport = window.MediaSource !== undefined || window.ManagedMediaSource !== undefined; +// Safari has poor MSE/DASH support; detect it to default preferHls to true +// Chrome includes "Safari" in UA for compatibility, so exclude "chrome" and "android" const isSafari = /^((?!chrome|android).)*safari/i.test(navigator.userAgent);Also applies to: 642-642
🤖 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/VideoPlayer.vue` at line 604, Add a short explanatory comment above the Safari detection and HLS preference checks (where isSafari and getPreferenceBoolean("preferHls", isSafari) are used) stating that Safari has limited MSE/DASH support so we default to native HLS playback; update both occurrences that use isSafari (the HLS preference logic and the related conditional at the other location) to include the same comment so future maintainers understand why Safari is handled specially.
🤖 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.
Nitpick comments:
In `@src/components/VideoPlayer.vue`:
- Line 604: Add a short explanatory comment above the Safari detection and HLS
preference checks (where isSafari and getPreferenceBoolean("preferHls",
isSafari) are used) stating that Safari has limited MSE/DASH support so we
default to native HLS playback; update both occurrences that use isSafari (the
HLS preference logic and the related conditional at the other location) to
include the same comment so future maintainers understand why Safari is handled
specially.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b1e0273d-b427-4925-9e79-74013727d632
📒 Files selected for processing (1)
src/components/VideoPlayer.vue
pnpm@latest now enforces onlyBuiltDependencies by default, causing ERR_PNPM_IGNORED_BUILDS. Pin to version 10 for compatibility.
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 `@src/components/VideoPlayer.vue`:
- Line 643: The comment line in VideoPlayer.vue ("// Safari defaults to HLS due
to limited MSE/DASH support (see isSafari above).") is mis-indented and failing
Prettier; fix the indentation/spacing at that line to match project Prettier
rules (use four-space indent or run Prettier) so the line aligns with
surrounding code in the VideoPlayer component and the linter passes.
🪄 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: b3ed73e9-49b9-4e23-94eb-6e714529f2ca
📒 Files selected for processing (3)
.github/workflows/ci.yml.github/workflows/reviewdog.ymlsrc/components/VideoPlayer.vue
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/reviewdog.yml
1 participant
Problem
Safari users get error 3016 (Shaka Player
VIDEO_ERROR/PIPELINE_ERROR_DECODE) when playing videos. This happens because:nullin the API response)Fix
Detect Safari via user agent and default
preferHlstotrueinstead offalse. This makes Safari use HLS (which it supports natively) by default, while keeping DASH as the default for Chrome/Firefox where MSE works well.Users can still override this in Preferences.
Testing
Summary by CodeRabbit
Bug Fixes
Chores