fix: render audio blocks in lessons - #2718
prestonzen wants to merge 1 commit into
Conversation
Audio blocks never rendered in a lesson. Two separate faults, both in the
audio path only, which is why image, PDF and video blocks were unaffected.
1. upload.js did not install translationPlugin on the AudioBlock app.
AudioBlock's template calls __() for its control labels, so __ was
undefined on that app's globalProperties and the render threw, leaving
the block empty. The video and PDF branches already install it.
2. AudioBlock.vue resolved its media element with
document.querySelector('audio') inside onMounted. EditorJS mounts a
block into a wrapper that is still detached from the document at that
point, so the query returned null and the next line threw:
Uncaught TypeError: Cannot set properties of null
(setting 'onloadedmetadata')
The same call also returned the first <audio> in the document rather
than the component's own, so a lesson holding several audio blocks
pointed every player at one element.
Bind the element through a template ref instead, and guard the handlers
that dereference it.
Fixes frappe#2717
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019KcoYnsWDz8wTq9Z7brEv4
Confidence Score: 4/5The implementation appears correct, but the repository’s regression-test requirement must be satisfied before merging. The media ref and translation setup address the failure paths, but no automated test protects either fix. Files Needing Attention: frontend/src/utils/upload.js, frontend/src/components/AudioBlock.vue Prompt To Fix All With AI### Issue 1
frontend/src/utils/upload.js:77
**Regression Coverage Is Missing**
This fix installs the missing translation plugin and introduces per-instance audio binding, but adds no automated test proving that audio uploads render or that multiple players remain independent. The repository requires every bug fix to include a regression test, so this requirement must be satisfied before merging.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix: render audio blocks in lessons" | Re-trigger Greptile |
| // AudioBlock's template calls __() for its control labels, so without | ||
| // the translation plugin __ is undefined on this app and the render | ||
| // throws. The video and PDF branches already install it. | ||
| app.use(translationPlugin) |
There was a problem hiding this comment.
Regression Coverage Is Missing
This fix installs the missing translation plugin and introduces per-instance audio binding, but adds no automated test proving that audio uploads render or that multiple players remain independent. The repository requires every bug fix to include a regression test, so this requirement must be satisfied before merging.
Context Used: Guidelines for reviewing Frappe Framework applicat... (source)
Knowledge Base Used: Learning content and course materials
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/utils/upload.js
Line: 77
Comment:
**Regression Coverage Is Missing**
This fix installs the missing translation plugin and introduces per-instance audio binding, but adds no automated test proving that audio uploads render or that multiple players remain independent. The repository requires every bug fix to include a regression test, so this requirement must be satisfied before merging.
**Context Used:** Guidelines for reviewing Frappe Framework applicat... ([source](https://github.com/frappe/skills/blob/main/skills/quality-code-review/SKILL.md))
**Knowledge Base Used:** [Learning content and course materials](https://app.greptile.com/frappe/-/custom-context/knowledge-base/frappe/lms/-/docs/learning-content.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Could you add ss/video showing the implementation? |
Cherry-picked from upstream frappe/lms PR frappe#2718 (still open there), adapted cleanly - our AudioBlock.vue/upload.js audio branch matched upstream's pre-fix state exactly, so this is the same fix verbatim: - The audio branch never installed translationPlugin on its Vue app (video/PDF already did), so AudioBlock.vue's __() calls for its control labels threw and nothing rendered at all. - document.querySelector('audio') ran before EditorJS attached the block's wrapper to the document, so it always returned null (or, once attached, the page's FIRST <audio> - a lesson with several audio blocks would have driven them all from one element). Bound through a template ref instead, scoped to this block's own element. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Fixes #2717
Problem
An
uploadblock whosefile_typeis audio (mp3/wav/ogg) renders nothing in a lesson. Image, PDF and video blocks in the same lesson render fine, so the fault is confined to the audio path.There are two separate causes, which is why the symptom changed after fixing only the first.
1.
translationPluginis not installed on the AudioBlock appfrontend/src/utils/upload.jsmounts a fresh Vue app per block. The video and PDF branches callapp.use(translationPlugin); the audio branch does not:translationPluginassigns__toapp.config.globalProperties, which is per-app.AudioBlock.vue's template calls__('Play'),__('Pause'),__('Mute')and__('Seek'), so on this app__is undefined and the render throws — the block mounts nothing at all.PdfBlocknever calls__, andVideoBlockcalls it but already gets the plugin, which is why only audio breaks.2.
document.querySelector('audio')returnsnullduringonMountedEditorJS's
compose()calls the tool'srender()and mounts the returned wrapper before inserting it into the document, so the wrapper is still detached here and the query matches nothing:The
setTimeout(…, 0)looks like it was meant to cover exactly this and does not wait long enough.The same call carries a second, latent bug: it returns the first
<audio>in the document rather than this component's own, so a lesson containing several audio blocks gets several player UIs all driving one element.Fix
translationPluginon the AudioBlock app, matching the video and PDF branches.ref="audioEl") instead of querying the document. This is correct regardless of DOM attachment order and scopes each player to its own element.togglePlay,toggleMute,changeCurrentTimeand theisPlayingwatcher, which all dereferenceaudio.valueunconditionally.Testing
Verified on frappe
15.120.1/ lms2.62.1against a real course: