fix: default preferHls to true on Safari#4236
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 and defaults the HLS preference accordingly. A computed ChangesSafari-aware HLS preference defaulting
🎯 2 (Simple) | ⏱️ ~8 minutes
🚥 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 |
![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
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
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.
johnpc
force-pushed
the
fix/safari-prefer-hls-default
branch
from
91289d5 to
ea61761
Compare
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