Refactor MediaPlayer to composables with VideoPlayer/AudioPlayer split#15035
Refactor MediaPlayer to composables with VideoPlayer/AudioPlayer split#15035rtibbles wants to merge 4 commits into
Conversation
github-actions
Bot
added
DEV: renderers
Build Artifacts
Smoke test screenshot |
🔵 Review postedLast updated: 2026-07-22 22:47 UTC |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15035 — a faithful, clean Vuex→composable migration. registerModule is gone. Captions/progress logic ports well into useCaptions/useMediaProgress. The vueParent injection threading (so video.js-instantiated menu buttons reach injectMediaPlayer()) is correct and consistent. CI passing.
Manual QA ran on both standalone players — desktop + mobile, English + Arabic, axe-core. Players render, autoplay, and play correctly.
Two blocking issues:
- RTL seek-bar fill draws on the opposite side from the thumb and seek math in RTL locales (confirmed against the Arabic route). Inline on AudioPlayerControls.vue.
- Re-init ordering in the new
watch(defaultFile)path constructs the player before disposing the old one, so a source swap returns the already-attached (then disposed) instance. Inline on useMediaPlayer.js.
Non-blocking:
- Scope: issue #14538 asks only for the Vuex→composable migration. This PR also bundles a ~1,400-line net-new audio-player redesign plus new user-visible strings. Worth splitting or calling out explicitly in the description so it gets design + a11y review. New strings are fine on
develop. - Test gap:
VideoPlayer.vue(825 lines) has no spec while the audio path got three. The deletedMediaPlayerIndex.spec.jsremoved the video path's smoke coverage.AudioPlayer.spec.jsshows the pattern to close it. - Sticky/embedded behaviour isn't reproducible on this branch (needs #15037/#15038). Recommend a follow-up QA pass once merged.
See inline suggestions for the IntersectionObserver re-init leak, the audio-poster regression, and the captions language snapshot→live change.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran a phased review pipeline over the pull request diff:
- Classified the diff to select review passes (core, frontend, backend) and whether manual QA was required
- Core review pass checked correctness, design, architecture, testing, completeness, and DRY/SRP/Rule-of-Three principles
- Specialized frontend/backend review passes applied framework-specific lenses where those files changed
- For UI changes: manual QA and an accessibility audit against a live dev server, when available
- Checked CI status and linked issue acceptance criteria
- Synthesized one review from those passes and chose the verdict from the findings, CI status, and QA evidence
| return; | ||
| } | ||
| nextTick(() => { | ||
| const vjsPlayer = videojs(playerRef.value, getPlayerConfig(), handleReadyPlayer); |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: Re-init constructs the player before disposing the old one. initPlayer calls videojs(playerRef.value, ...) here, and only then does setPlayer → resetState() dispose the previous instance (lines 327/354). On the new watch(defaultFile) re-init path (line 374), the old player is still attached to playerRef.value, so videojs() returns that already-attached player (logging "already initialised. Options will not be applied.") and never applies the new source/config; setPlayer then disposes it and assigns player.value = markRaw(<disposed player>). Net result on a source swap: no new source loads and player.value is disposed.
Fix: resetState() at the top of initPlayer (before videojs(...)) so the element is free when it runs. If in practice the content viewer is always re-keyed per node and this watcher never fires, the watcher is dead code — either way worth resolving.
| :style="{ backgroundColor: $themePalette.grey.v_300 }" | ||
| > | ||
| <div | ||
| class="progress-bar-fill" |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: In RTL locales the fill renders on the opposite side from the thumb and the seek math. .progress-bar-fill has no position (CSS line 387), so it is a static block; in an RTL container its inline-start is the right edge, so it grows right→left while the thumb (left: progressPercent%) and clientX - rect.left seek math stay left→right. Confirmed in manual QA on the Arabic audio route: at ~30% the thumb sat left-of-centre while the blue fill hugged the right edge. Affects all audio for RTL learners (Arabic/Hebrew/Persian/Urdu).
Matches the stated LTR-timeline intent in the comment above: add direction: ltr; to .progress-bar-track, or give the fill position: absolute; left: 0; with /* rtl:ignore */ so RTLCSS doesn't flip it to right.
| if (typeof IntersectionObserver === 'undefined') { | ||
| return; | ||
| } | ||
| intersectionObserver = new IntersectionObserver( |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: IntersectionObserver leaks on re-init. setupIntersectionObserver is passed as onReady, which useMediaPlayer invokes on every init (including the watch(defaultFile) re-init path). On a second call this creates a new observer without disconnecting the previous one; onBeforeUnmount only disconnects the last. Guard the top with if (intersectionObserver) intersectionObserver.disconnect(), or reuse a single observer. Same code path as the re-init finding in useMediaPlayer.js.
|
|
||
| <template v-if="!loading"> | ||
| <img | ||
| v-if="posterUrl" |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: Audio content without a thumbnail now renders no poster. posterUrl is thumbnailFiles[0]?.storage_url ?? null, and the img is v-if="posterUrl". The old code fell back to a bundled audio-icon-poster.svg for audio with no thumbnail — a common case. Confirm the empty poster is intended, since it's an easy-to-miss visual regression.
There was a problem hiding this comment.
Intended. The redesigned card no longer needs a filler image where a thumbnail is absent, so the poster is dropped rather than substituted.
| track.addCue = (...args) => { | ||
| const result = addCue(...args); | ||
|
|
||
| if (track.language == language.value) { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: This addCue override reads language.value live on each cue add, whereas the old Vuex module snapshotted language at setTrackList time. Since the override is installed once per track and updateTrackList doesn't re-run setTrackList when track count is unchanged, these differ for cues that load asynchronously after a language switch. Looks like an improvement, but confirm the behavior change is intended.
There was a problem hiding this comment.
Intended, and a fix. Tracks load lazily, so a switch fetches the VTT and cues arrive after the switch — against the old snapshot they were dropped and the transcript stayed short. The reverse case is unchanged: cues for a track switched away from still fail the comparison. Covered by a new useCaptions spec.
571d524 to
6e4c3c4
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15035 — delta re-review on 6e4c3c44. All 5 prior findings resolved or acknowledged, but the re-init fix introduces a new blocking DOM regression (inline).
The dispose-first re-init disposes the video.js player before rebuilding on the same element. Verified against pinned video.js@7.21.7: Player.dispose() → Component.dispose({restoreEl: undefined}) runs this.el_.parentNode.removeChild(this.el_) (video.cjs.js:3936), removing the wrapper and the playerRef <audio> tag it contains from the live DOM. videojs(playerRef.value, …) then re-wraps the now-detached tag, and since the <audio ref="playerRef"> has no :key (only its <source> children do), Vue patches in place and never re-appends it. On a source change the player vanishes from the page. The new "disposes the previous player before constructing the new one" test only asserts call order against a mock whose dispose() is a DOM no-op, so it passes while the real regression goes uncaught. See inline comment for options.
CI pending; manual QA required but did not run — UI not visually verified.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
Prior-finding status
RESOLVED — useMediaPlayer.js:321 — re-init constructs player before disposing
RESOLVED — AudioPlayerControls.vue:62 — RTL seek-bar fill on opposite side
RESOLVED — AudioPlayer.vue:134 — IntersectionObserver leaks on re-init
ACKNOWLEDGED — AudioPlayer.vue:43 — audio without thumbnail renders no poster
ACKNOWLEDGED — useCaptions.js:190 — addCue reads language.value live
| // Dispose any existing player first: on the re-init path the old instance | ||
| // is still attached to playerRef, and videojs() would return that instead | ||
| // of applying the new source. | ||
| resetState(); |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: Dispose-first re-init builds the new player in a detached subtree. resetState() calls player.value.dispose(); verified against pinned video.js@7.21.7, Component.dispose with no restoreEl runs this.el_.parentNode.removeChild(this.el_) (video.cjs.js:3936), which removes the generated wrapper and the playerRef <audio> tag inside it from the live DOM. The next line's videojs(playerRef.value, …) re-wraps the now-orphaned tag, and since <audio ref="playerRef"> has no :key (only its <source> children do), Vue patches in place and never re-appends it — so on a defaultFile.storage_url change the player disappears from the page.
The added "disposes the previous player before constructing the new one" test only asserts call order against videojsMock, whose dispose() never touches the DOM, so it passes while this regression goes uncaught.
Options: (a) swap sources on the existing instance with player.src(newSources) instead of dispose+rebuild — keeps the element attached; (b) :key the media element on defaultFile.storage_url so Vue remounts a fresh tag; or (c) pass restoreEl so video.js restores the tag before you re-wrap. Whichever you pick, add a test that renders into a real container and asserts document.contains(mediaEl) after a source change. If this re-init path never actually fires in production (renderers remount per content node), the watch(defaultFile) + fix + test are dead code and should be removed instead.
6e4c3c4 to
2922a58
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15035 — 6 of 6 prior findings resolved or acknowledged; no new findings. CI is pending and manual QA did not run, so this is a COMMENT rather than an approval.
Prior-finding status
RESOLVED — useMediaPlayer.js:324 — re-init constructs player before disposing old one
RESOLVED — useMediaPlayer.js:323 — dispose-first re-init builds player in detached subtree
RESOLVED — AudioPlayerControls.vue:62 — RTL seek-bar fill draws on wrong side
RESOLVED — AudioPlayer.vue:139 — IntersectionObserver leaks on re-init
ACKNOWLEDGED — AudioPlayer.vue:48 — audio poster dropped when no thumbnail ("Intended")
ACKNOWLEDGED — useCaptions.js:190 — addCue reads language live vs snapshot ("Intended, and a fix")
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
2922a58 to
7d1d9ba
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15035 — All 6 prior findings resolved or acknowledged; no new findings on the newly changed code. registerModule is gone and player/caption/progress state now lives in the useMediaPlayer/useCaptions/useMediaProgress composables. The dispose-first re-init race is cleanly closed via the keyed media-element wrapper (AudioPlayer.vue:12 / VideoPlayer.vue:17), and resource lifecycle (timers, listeners, IntersectionObserver) has matching teardown.
Not approving: CI is still pending and the required manual QA did not run, so the audio-player redesign was not visually verified.
Prior-finding status
RESOLVED — useMediaPlayer.js:314 — re-init constructs player before disposing old one
RESOLVED — useMediaPlayer.js:323 — dispose-first re-init builds player in detached subtree (keyed wrapper on AudioPlayer.vue:12 / VideoPlayer.vue:17)
RESOLVED — AudioPlayerControls.vue:62 — RTL seek-bar fill on wrong side (fill+thumb both keyed off progressPercent)
RESOLVED — AudioPlayer.vue:139 — IntersectionObserver leaks on re-init (disconnect on re-init and onBeforeUnmount)
ACKNOWLEDGED — AudioPlayer.vue:48 — audio poster dropped when no thumbnail (Intended)
ACKNOWLEDGED — useCaptions.js:190 — addCue reads language live vs snapshot (Intended, and a fix)
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
7d1d9ba to
08ddd84
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15035 — delta re-review of the rebased branch (7d1d9ba → 08ddd84). All 6 prior findings resolved or acknowledged; no new findings. The delta since the last round is cleanup-only across three composables and one spec — no behavioural changes.
CI passing. Verdict is COMMENT rather than APPROVE only because manual QA was required for this PR but did not run — the UI was not visually verified in this pass.
Prior-finding status
RESOLVED — useMediaPlayer.js:314 — re-init constructs player before disposing old one
RESOLVED — useMediaPlayer.js:323 — dispose-first re-init builds player in detached subtree
RESOLVED — AudioPlayerControls.vue:62 — RTL seek-bar fill on wrong side
RESOLVED — AudioPlayer.vue:139 — IntersectionObserver leaks on re-init
ACKNOWLEDGED — AudioPlayer.vue:48 — audio poster dropped when no thumbnail (intended)
ACKNOWLEDGED — useCaptions.js:190 — addCue reads language live vs snapshot (intended)
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
Derive source extensions from the MIME type, then the URL suffix, falling back to the container default, so extracted sources never carry a null type. Read src via getAttribute so extraction works on detached nodes. Add VIDEO_PRESETS to classify content as video vs audio, and a mediaStrings module for the extracted components.
Replace the Vuex store module with instance-scoped composables: useMediaPlayer owns player lifecycle and state, composing useCaptions, useMediaProgress, and useScrollContainer; useSeekBar derives scrub-bar state. useMediaProgress isolates the shared progress-tracking logic so video and audio report progress identically. Add a videojs mock and a useMediaPlayer manual mock for testing consumers.
|
Both the new audio and video players work well with Chrome in Linux and Windows, all player commands are keyboard navigable and properly announced through NVDA. Standalone player autoplays, embedded ones do not until interacted with. What does not look optimal (and not exactly as the Figma design) is the position of the transcript: it breaks correctly for display below on narrow screens, but on wider resolutions it creates a weird empty space between the player and itself as it is aligned to the right of the browser page, and does not fill the full available height. 
An issue seems to be impeding video playing in Firefox, as the player is not loading and breaks the embedded article page (audio player works correctly):
|
VideoPlayer wraps the video.js instance previously inlined in MediaPlayerIndex. AudioPlayer is a purpose-built audio experience: AudioPlayerControls provides custom transport controls, AudioStickyPlayer keeps them docked while scrolling, and an inline transcript replaces the video captions UI. Add a centered BigPlayToggle custom button and translate the existing transport buttons at construction time.
08ddd84 to
cf513fa
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15035 — delta re-review. All 6 prior findings resolved or acknowledged; 1 new suggestion below.
Media-player changes since the last baseline are minor cleanup; the rest of the branch delta is an unrelated rebase onto newer develop. CI passing. Manual QA was required but did not run, so the UI was not visually verified — not approving on UI grounds.
- suggestion —
LanguagesMenu.vue:8: droppedKRadioButtonGroupwrapper. See inline.
Prior-finding status
RESOLVED — useMediaPlayer.js:349 — re-init constructs player before disposing old one
RESOLVED — useMediaPlayer.js:348 — dispose-first re-init builds player in detached subtree
RESOLVED — AudioPlayerControls.vue:62 — RTL seek-bar fill on wrong side
RESOLVED — AudioPlayer.vue:139 — IntersectionObserver leaks on re-init
ACKNOWLEDGED — AudioPlayer.vue:48 — audio poster dropped when no thumbnail (intended)
ACKNOWLEDGED — useCaptions.js:189 — addCue reads language live vs snapshot (intended, and a fix)
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| <KRadioButtonGroup> | ||
| <slot></slot> | ||
| </KRadioButtonGroup> | ||
| <slot></slot> |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: This removes the KRadioButtonGroup that wrapped the injected language <slot>. Native radio semantics (single-selection, arrow-key nav) survive because the LanguagesMenuItem KRadioButtons share a constant name, but the group's role="radiogroup" / accessible group label is lost. If the wrapper markup conflicted with video.js's runtime menu DOM this is plausibly deliberate — can you confirm the removal was intentional and that screen-reader group semantics for the language list are still acceptable? Not blocking.
There was a problem hiding this comment.
This was completely deliberate, yes - the KRadioButtonGroup was a cause of a Firefox specific error.
There was a problem hiding this comment.
Thanks for confirming — that fully resolves the question. Dropping the wrapper to avoid the Firefox error is a sensible trade-off, and native radio semantics still cover single-selection and arrow-key nav. Resolved.
MediaPlayerIndex now delegates to VideoPlayer or AudioPlayer and drives state through the media composables instead of the Vuex module. Remove the store module and its injection in videojsVueMixin, and rewire the captions, languages, fullscreen, and transcript views onto the composable API.
cf513fa to
8c69163
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15035 — all 7 prior findings resolved or acknowledged; no new findings on the recomposed diff. CI passing.
Note: manual QA was required for this PR but did not run this round, so the UI was not visually verified — hence COMMENT rather than APPROVE.
Prior-finding status
RESOLVED — useMediaPlayer.js:349 — re-init constructs player before disposing old one
RESOLVED — useMediaPlayer.js:348 — dispose-first re-init builds player in detached subtree
RESOLVED — AudioPlayerControls.vue:62 — RTL seek-bar fill draws on wrong side
RESOLVED — AudioPlayer.vue:139 — IntersectionObserver leaks on re-init
ACKNOWLEDGED — AudioPlayer.vue:48 — audio without thumbnail renders no poster (maintainer: intended, redesigned card drops the filler)
ACKNOWLEDGED — useCaptions.js:189 — addCue reads language live vs old snapshot (maintainer: intended and a fix; covered by new useCaptions spec)
ACKNOWLEDGED — LanguagesMenu.vue:8 — dropped KRadioButtonGroup wrapper (maintainer: deliberate, avoids a Firefox-specific error)
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
|
Issue on Firefox is no more, video player works correctly! 🎉 Transcript is also now occupying the full space on the right side of the browser, and displaying well in the audio player too! 👏🏽 
|
🚀
3 participants
Summary
References
Closes #14538. Contributes to #13824.
#14538 covers the Vuex→composable migration only; the audio-player redesign and its new strings are net-new here and want design and a11y eyes.
Reviewer guidance
useMediaPlayer.js:286— embedded players skip autoplay and play on first interaction.useMediaPlayer.js:354— the video.js player is disposed on scope unmount.fileExtractors.js:82— a<source>extension comes from MIME type, then URL suffix, then a container default.video/null.Screenshots
Pulled from the combined implementation PR #14548, which this PR splits.
The standalone players reproduce on this branch. The embedded ones also need #15037 and #15038.
The audio controls stay LTR in RTL locales, matching the video player (
!!rtl:ignoreover the whole video.js control bar ondevelop) and YouTube.Sticky audio player (recording): https://github.com/user-attachments/assets/95dba36a-07e6-4626-a470-50a0e1f8fa03
Embedded video player (recording): https://github.com/user-attachments/assets/a3f7b970-c608-4564-90c9-aaa667938081
AI usage
Used Claude Code to refactor MediaPlayer from the Vuex module to composables, extract the video and audio players, and split the change into reviewable commits. Verified with the media_player Jest suite and prek.