-
-
Notifications
You must be signed in to change notification settings - Fork 135
support audio embeds #2533
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
support audio embeds #2533
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really shouldn't be considered qualified to review anything related to webapps; however, my comments are provided in the hope that they help reduce the concerns of critics who wish to review the PR.
components/embed.js
Outdated
| whiteSpace: 'nowrap' | ||
| }} | ||
| > | ||
| 🎵 {meta.title} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is "notes" the best graphic for representing audio? lots of audio might not be music, strictly speaking...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While arguable, audio files are historically represented by a music note, so it's okay to use it.
But @pory-gone you should use an SVG instead of an emoji: https://remixicon.com/icon/file-music-line
lib/url.js
Outdated
| return misleading | ||
| } | ||
|
|
||
| // Add after IMG_URL_REGEXP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the above comment is forensically interesting [i.e., "look, Copilot is talking to the operator"], and probably does not belong in the production codebase.
| meta: { | ||
| href, | ||
| audioType: extension, | ||
| title: decodeURIComponent(pathname.split('/').pop().split('.')[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there no better guess for the reasonable text label of uncaptioned audio? I'm thinking, some combination of comment author, context [i.e. SN item ID], and filename only if it's not some hash or blob identifier, otherwise some reasonable timestamp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR focuses on embedding audio from a link, this would just complicate things
lib/constants.js
Outdated
| 'audio/aac', | ||
| 'audio/flac' | ||
| ] | ||
| export const AUDIO_EXTENSIONS = ['mp3', 'wav', 'ogg', 'flac', 'aac', 'm4a', 'opus'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for some folks, the above lines might not be obviously fine; however, line 44 is about file types, while 37-42 are for MIME types, and there isn't any one-to-one mapping between categories of types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it definitely can embed links that ends with an audio extension.
Logic
There are two logics (mmh) and both are flawed:
- via
Embed: link has to end with an extension - via
audio.onload: audio files can be reproduced as video elements
I left a comment on how you could address this.
I want to note that yesterday I opened PR #2599 that would remove the need to load the file to know its type. It could come in handy for this PR.
Style
The style has weird measurements, is inconsistent across browsers and the dark/light switch is pretty painful:
The animation is cool to see, but not really needed and makes thinking about the style harder than it could be.
I left some comments about other things I found, I'd like for you to address them.
In conclusion, focus on using the best logic you can think of. That's what's most important!
components/embed.js
Outdated
| whiteSpace: 'nowrap' | ||
| }} | ||
| > | ||
| 🎵 {meta.title} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While arguable, audio files are historically represented by a music note, so it's okay to use it.
But @pory-gone you should use an SVG instead of an emoji: https://remixicon.com/icon/file-music-line
lib/url.js
Outdated
| // Add after IMG_URL_REGEXP | ||
| export const AUDIO_URL_REGEXP = /^(https?:\/\/.*\.(?:mp3|wav|ogg|flac|aac|m4a))$/i |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused and not necessary, probably you got inspiration from the other regexes at the bottom, but they don't serve a purpose afaik
lib/constants.js
Outdated
| 'audio/aac', | ||
| 'audio/flac' | ||
| ] | ||
| export const AUDIO_EXTENSIONS = ['mp3', 'wav', 'ogg', 'flac', 'aac', 'm4a', 'opus'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused and not necessary
components/media-or-link.js
Outdated
| const video = document.createElement('video') | ||
| video.onloadedmetadata = () => { | ||
| setIsVideo(true) | ||
| setIsImage(false) | ||
| } | ||
| video.onerror = () => { | ||
| // hack | ||
| // if it's not a video it will throw an error, so we can assume it's an image | ||
| const img = new window.Image() | ||
| img.src = src | ||
| img.decode().then(() => { // decoding beforehand to prevent wrong image cropping | ||
| setIsImage(true) | ||
| }).catch((e) => { | ||
| console.warn('Cannot decode image:', src, e) | ||
| }) | ||
| const audio = document.createElement('audio') | ||
| audio.onloadedmetadata = () => { | ||
| setIsAudio(true) | ||
| setIsImage(false) | ||
| setIsVideo(false) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
components/media-or-link.js
Outdated
| {audio | ||
| ? <audio | ||
| ref={ref} | ||
| src={src} | ||
| preload={bestResSrc !== src ? 'metadata' : undefined} | ||
| controls | ||
| poster={bestResSrc !== src ? bestResSrc : undefined} | ||
| preload='metadata' | ||
| width={width} | ||
| height={height} | ||
| onError={onError} | ||
| onLoadedMetadata={handleLoadedMedia} | ||
| /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This way we would have a styled Audio component (parsed from link) and an unstyled audio container (from upload).
You have some choices:
- Don't use
Embedfor audios- you wouldn't need an extension, but you'd need to fix the way you recognize it's an audio in
useMediaHelper
- you wouldn't need an extension, but you'd need to fix the way you recognize it's an audio in
- Upload audio files without the
![]()markdown so it can be parsed as embed- you would also need to append the extension based on the mime type, because your parser checks for extensions, and then remove the extension when the embed... but then the raw link would be broken... mhh...
Audiocomponent shared between embeds and uploads- still two logics for audios
| meta: { | ||
| href, | ||
| audioType: extension, | ||
| title: decodeURIComponent(pathname.split('/').pop().split('.')[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR focuses on embedding audio from a link, this would just complicate things
…ews into support_audio_embeds
|
Thank you for the detailed review. I've implemented all your requests: unused code removed, emoji replaced with SVG, detection logic fixed. You were right about the critical issue! Audio files were being caught as video in the cascade detection. I've fixed it through improved error handling. As you suggested, this PR focuses only on embedding direct audio links. |





Description
fix #2526
this implementation add native audio file embedding, allowing user to embed direct audio files links as playable HTML5 audio players within posts and comments.
Extended
parseEmbedUrl()for audio file detection;Added new "audio" provider to embed framework;
Extended
useMediaHelperwith audio detection cascade;Integrated with existing
MediaOrLinkcomponent;Added CSS styling with theme compatibily.
Screenshots
2025-09-14-20-55-58.mp4
Additional Context
Audio detection is positioned early in
parseEmbedUrl()to catch file links before other providers, uses both embed framework and media helper to garant a fallback if one fails,supports query parameters in URLs (e.g.,
file.mp3?version=1), used!importantCSS override to prevent margin inheritance from general embed rules,implemented audio detection cascade in
useMediaHelperto maintain existing video/image logic, addedpreload='metadata'for performance while avoiding automatic downloadsChecklist
Are your changes backward compatible? Please answer below:
Yes
On a scale of 1-10 how well and how have you QA'd this change and any features it might affect? Please answer below:
8/10
For frontend changes: Tested on mobile, light and dark mode? Please answer below: yes
Did you introduce any new environment variables? If so, call them out explicitly here:
NaN
Did you use AI for this? If so, how much did it assist you?
I used AI to understand the context and identify the files to be modified, I asked for some implementation advice but the changes were all carefully considered and applied manually
Note
Adds first-class audio embedding for direct audio links with detection, playback UI, styling, and allowed MIME types.
audioprovider inparseEmbedUrl(detectsmp3/wav/ogg/flac/aac/m4a/opus/webm, extractsaudioTypeand title).components/embed.jswith optional title and download link; new.audioWrapperstyles incomponents/text.module.css.components/media-or-link.js: supportaudioprop; render<audio>when detected; updateloadedlogic and error handling.useMediaHelper: cascade detection (video → audio → image); exposeaudioflag and include inshowMediagating.UPLOAD_TYPES_ALLOWwith audio MIME types; addAUDIO_EXTENSIONSandAUDIO_URL_REGEXP.pages/settings/index.jsto include audio and list direct audio files as supported embeds.Written by Cursor Bugbot for commit 589a708. This will update automatically on new commits. Configure here.