fix: use hls.js for HLS video playback on share page#76
Conversation
ravirajsinh45
left a comment
There was a problem hiding this comment.
Thanks for tackling cross-browser HLS on the share page — hls.js is the right call. A couple of blockers mean the player won't actually play in the standard split-origin setup, plus some hardening.
Blockers
- Relative stream URL isn't prefixed with the API origin. Backend returns a root-relative URL (
/stream/hls/master.m3u8?token=…,apps/api/routers/share.py:1383). It's passed straight tohls.loadSource(streamUrl)/mediaEl.src, which resolves against the frontend origin (:3000), notNEXT_PUBLIC_API_URL(:8000) → the manifest 404s and the video never plays. The canonical player already handles this (apps/web/components/review/video-player.tsx:185-188:url.startsWith('/') ? ${API_URL}${url} : url), same convention as #46/#49 and #51/#57. Resolve to absolute inside the effect beforeloadSource/src(API_URLis already in scope). - The effect never re-runs after the element mounts. Deps are
[streamUrl, asset.asset_type], but<video>/<audio>are gated behindstreamLoading. The parent setsstreamUrlin.thenandstreamLoading=falsein.finally(separate microtasks, not batched), so by the time the element mounts no dep has changed and hls is never attached. Sincesrc={streamUrl}was removed from the element, the result is a blank player. AddstreamLoadingto the dep array (or use a callback ref).
Should fix
- Playback errors are silently swallowed — no
Hls.Events.ERRORlistener and noonErroron<video>/<audio>. Presigned-S3 expiry/403 leaves a black player; the existing "unavailable" fallback only fires whenstreamUrlis falsy. Surface a fatal-error state.
Nice to have
- The top-level
import Hls from 'hls.js'pulls ~530KB into the public share route's first-load JS for every visitor (including image/audio/folder shares).apps/web/components/share/folder-share-viewer.tsxlazy-loads viaimport('hls.js')— matching that keeps the page light. - The non-Safari-without-MSE path assigns
.m3u8to nativesrcwith nocanPlayTypeguard (siblingHlsVideoguards this).
PR Techiebutler#76 (HLS Playback): - Prefix relative stream URLs with API_URL before hls.loadSource() - Add streamLoading to useEffect dependency array - Lazy-import hls.js (no top-level import) - Add Hls.Events.ERROR handler + video onError for fatal-error state - Add canPlayType guard for native .m3u8 Safari fallback PR Techiebutler#77 (Comment Crash): - Replace guest_name/guest_email with guest_author.name as primary - Fallback chain: guest_author?.name || author?.name || 'Unknown' - Add referrerPolicy=no-referrer on avatar <img> - Add onError fallback to initials on failed avatar load - Use || instead of ?? for empty-string fallthrough PR Techiebutler#78 (No-Audio Transcode): - Add tests asserting constructed argv for has_audio=True/False - ffprobe audio probe uses _run() with fail-fast error handling PR Techiebutler#79 (Stderr in Error Messages): - Add errors='replace' to subprocess.run() for Latin-1/Shift-JIS stderr - Change ffprobe -v quiet to -v error to preserve stderr content PR Techiebutler#80 (Clipboard Fallback): - Extract copyToClipboard() helper into lib/utils.ts - iOS Safari: contentEditable + setSelectionRange for execCommand - Add aria-live region for success/failure screen reader announcements - Add 'Failed' visual state with XCircle icon - Use copyToClipboard in both share-dialog and share-create-dialog
e5e6321 to
7a1ae5f
Compare
|
Thanks for the review @ravirajsinh45. All feedback addressed: Blockers fixed:
Nice-to-haves done:
Frontend build passes. Branch updated with all changes. |
ravirajsinh45
left a comment
There was a problem hiding this comment.
Nice iteration, @rubenxyz — the HLS work from round one is all resolved: the stream URL is now prefixed with API_URL, streamLoading is in the dep array so hls attaches after mount, lazy import('hls.js') keeps the share route light, and the fatal-error surface is a real improvement. The backend transcoder change (only emit -map a:0 / a:{i} when ffprobe actually finds an audio stream) is a genuinely good fix for silent videos, and the test coverage is appreciated. 👍
The remaining issues are all in the changes bundled alongside the HLS fix — one of them is a crash, so flagging as changes-requested.
Blocker
- Rules-of-Hooks violation crashes the comment list. In
GuestCommentList,apps/web/app/share/[token]/page.tsx:234callsconst [imgError, setImgError] = React.useState(false)insidecomments.map((comment) => { … }). Becausecommentsstarts[]with an early return on empty (page.tsx:16,36), the number of hooks rendered changes between renders — 0 when empty, N once comments load, N+1 after each new comment viarefreshKey. React throws "Rendered more hooks than during the previous render" as soon as comments populate, white-screening the panel. Please extract a small<GuestCommentItem comment={…} />subcomponent that owns its ownimgErrorstate, and map over that.
Should fix
copyToClipboardreports success even when the write rejects.apps/web/lib/utils.ts:588firesnavigator.clipboard.writeText(text).catch(() => {})and thenreturn truesynchronously, so a rejected write (document not focused, permission denied, blocked by policy) still returnstrue→ the UI shows "Copied!" for a copy that didn't happen, and the new'failed'state is unreachable on the modern-API path. Since reliable copy + a failed state is the point of this refactor, either make the functionasyncand await the write, or return the promise result (.then(() => true, () => false)).
Scope
- This PR is titled for HLS playback but also ships the guest-comment author/avatar refactor, the
copyToClipboardutility + two share-dialog rewrites, aria-live accessibility, and the backend audio-handling + tests. Each is reasonable on its own, but bundling them is what let a crash-level comment-list bug ride in under a player fix, and it makes the change hard to review or revert. Could you split these — at minimum pull the guest-comment refactor into its own PR where #1 gets fixed? The HLS + transcoder parts look ready to land on their own.
Thanks again for the careful follow-up here — the player itself is in good shape.
|
Heads-up: #77–#80 turned out to be duplicates of this PR (all five had the identical diff), so I've closed them and we'll consolidate here. The two items from my review above — the |
- copyToClipboard: make async and await writeText promise so rejection correctly returns false instead of optimistically returning true (fixes false 'Copied!' state) - GuestCommentList: extract GuestCommentItem subcomponent so imgError useState is per-rendered-item instead of inside .map(), fixing 'Rendered more hooks than during the previous render' crash when comments populate or change count
|
Thanks for the thorough review @ravirajsinh45. Both remaining issues are now fixed: Blockers resolved: 1. 2. Scope is now just the share page + copy utility. The HLS/transcoder parts are unchanged. Ready for re-review. |
Description
The
ShareMediaViewercomponent on the public share page uses a plain<video src={streamUrl}>element. This works in Safari (which has native HLS support) but fails to play HLS (.m3u8) streams in Firefox and Chrome, as these browsers do not support HLS natively.Fix
Hls.isSupported()to detect MSE-capable browsers, loads HLS via hls.js<video src>for Safari and direct media filesTesting
This fix is verified in production at https://www.kamiko.xyz/freeframe/ - HLS video plays correctly in both Firefox and Chrome.
Related
Fixes #68