Skip to content

fix: render audio blocks in lessons - #2718

Open
prestonzen wants to merge 1 commit into
frappe:mainfrom
prestonzen:fix/audio-block-render
Open

prestonzen wants to merge 1 commit into
frappe:mainfrom
prestonzen:fix/audio-block-render

Conversation

@prestonzen

Copy link
Copy Markdown

Fixes #2717

Problem

An upload block whose file_type is 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. translationPlugin is not installed on the AudioBlock app

frontend/src/utils/upload.js mounts a fresh Vue app per block. The video and PDF branches call app.use(translationPlugin); the audio branch does not:

// video: registerDirectives(app); app.use(translationPlugin); app.mount(...)
// audio: registerDirectives(app);                             app.mount(...)   <-- missing
// pdf:   registerDirectives(app); app.use(translationPlugin); app.mount(...)

translationPlugin assigns __ to app.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. PdfBlock never calls __, and VideoBlock calls it but already gets the plugin, which is why only audio breaks.

2. document.querySelector('audio') returns null during onMounted

onMounted(() => {
	setTimeout(() => {
		audio.value = document.querySelector('audio')
		audio.value.onloadedmetadata = () => { ... }   // throws
		...
	}, 0)
})

EditorJS's compose() calls the tool's render() and mounts the returned wrapper before inserting it into the document, so the wrapper is still detached here and the query matches nothing:

AudioBlock.vue:71 Uncaught TypeError: Cannot set properties of null (setting 'onloadedmetadata')
    mount → renderFile → render → compose (editorjs.mjs:1789)

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

  • Install translationPlugin on the AudioBlock app, matching the video and PDF branches.
  • Bind the media element through a template ref (ref="audioEl") instead of querying the document. This is correct regardless of DOM attachment order and scopes each player to its own element.
  • Guard togglePlay, toggleMute, changeCurrentTime and the isPlaying watcher, which all dereference audio.value unconditionally.

Testing

Verified on frappe 15.120.1 / lms 2.62.1 against a real course:

  • Audio blocks render and play; duration and the seek slider populate correctly.
  • A lesson with three audio blocks gives three independent players (previously they would all have driven the first element).
  • Image, PDF, paragraph, list and header blocks are unaffected.
  • Reproduced the original failure in a clean incognito profile first, ruling out caching and service-worker staleness.

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
@greptile-apps

greptile-apps Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Confidence Score: 4/5

The 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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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!

@raizasafeel

Copy link
Copy Markdown
Contributor

Could you add ss/video showing the implementation?

tirtana888 pushed a commit to tirtana888/lms that referenced this pull request Sep 13, 2026
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants