feat: render audio players for linked note audio - #297
Open
dergigi wants to merge 2 commits into
Open
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughAdds audio playback support to notes: new URL regex constants and exported helpers ( ChangesAudio playback support
Sequence Diagram(s)sequenceDiagram
participant NoteMedia
participant VideoWithBlurhash
participant AudioPlayer
participant Browser as HTML audio/video
NoteMedia->>NoteMedia: extractMediaFromContent → MediaItem[]
alt item.type === 'video'
NoteMedia->>VideoWithBlurhash: src=getTrimmedMediaUrl(item.src)
VideoWithBlurhash->>Browser: render <video> element
Browser-->>VideoWithBlurhash: onLoadedMetadata (videoWidth=0, videoHeight=0 AND allowAudioFallback)
VideoWithBlurhash->>VideoWithBlurhash: setRenderAsAudio(true)
VideoWithBlurhash->>AudioPlayer: render AudioPlayer(src, onClickSearch)
else item.type === 'audio'
NoteMedia->>AudioPlayer: src=getTrimmedMediaUrl(item.src)
end
AudioPlayer->>Browser: render <audio controls preload="metadata">
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/VideoWithBlurhash.tsx (1)
38-47:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear stale dimensions when
dimis absent on source change.Line 44 only updates
dimensionswhendimis valid; otherwise previous dimensions persist acrosssrcchanges and can render an incorrect aspect ratio.Proposed fix
useEffect(() => { setVideoLoaded(false); setVideoError(false); setIsPlaying(false); setShowControls(false); setRenderAsAudio(false); if (dim && dim.width > 0 && dim.height > 0) { setDimensions(dim); + } else { + setDimensions(null); } }, [src, dim]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/VideoWithBlurhash.tsx` around lines 38 - 47, The useEffect hook in VideoWithBlurhash component only updates dimensions when dim is valid, but fails to reset dimensions when dim is absent or invalid on source changes, causing stale dimensions to persist and render incorrect aspect ratios. Add an else clause after the if condition checking dim validity to explicitly clear or reset dimensions by calling setDimensions with a default/null value when dim is not provided or dimensions are invalid.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/utils/textUtils.ts`:
- Around line 23-24: The regex patterns in the replace calls are too broad and
strip any query parameter substring ending with media file extensions, which
corrupts unrelated URLs that contain extension-like strings in their query text.
Tighten both regex patterns to only match explicit filename-style parameters by
requiring the extension to be part of an actual filename context (such as after
an equals sign or in a filename parameter format) rather than matching any
arbitrary text followed by a media extension. This ensures that search queries
or other non-media URLs containing `.mp3` or similar in their query values are
not inadvertently corrupted.
---
Outside diff comments:
In `@src/components/VideoWithBlurhash.tsx`:
- Around line 38-47: The useEffect hook in VideoWithBlurhash component only
updates dimensions when dim is valid, but fails to reset dimensions when dim is
absent or invalid on source changes, causing stale dimensions to persist and
render incorrect aspect ratios. Add an else clause after the if condition
checking dim validity to explicitly clear or reset dimensions by calling
setDimensions with a default/null value when dim is not provided or dimensions
are invalid.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 99c911fa-349c-4f87-864e-518f0e35cbe8
📒 Files selected for processing (6)
src/components/AudioPlayer.tsxsrc/components/NoteMedia.tsxsrc/components/VideoWithBlurhash.tsxsrc/lib/utils/mediaUtils.tssrc/lib/utils/textUtils.tssrc/lib/utils/urlUtils.ts
Comment on lines
+23
to
+24
| .replace(/\?[^\s]*\.(?:png|jpe?g|gif|gifs|apng|webp|avif|svg|m4a|mp3|wav|flac|aac|opus|mp4|webm|ogg|ogv|mov|m4v)[^\s]*/gi, '') | ||
| .replace(/\?name=[^\s]*\.(?:png|jpe?g|gif|gifs|apng|webp|avif|svg|m4a|mp3|wav|flac|aac|opus|mp4|webm|ogg|ogv|mov|m4v)[^\s]*/gi, ''); |
Contributor
There was a problem hiding this comment.
Tighten query-param stripping to avoid mutating non-media URLs.
Line 23 currently removes any query substring that ends with a media-like extension, which can corrupt unrelated links (e.g., search URLs containing .mp3 in query text). Restrict stripping to explicit filename-style params instead of any ?…ext… pattern.
Proposed fix
- .replace(/\?[^\s]*\.(?:png|jpe?g|gif|gifs|apng|webp|avif|svg|m4a|mp3|wav|flac|aac|opus|mp4|webm|ogg|ogv|mov|m4v)[^\s]*/gi, '')
- .replace(/\?name=[^\s]*\.(?:png|jpe?g|gif|gifs|apng|webp|avif|svg|m4a|mp3|wav|flac|aac|opus|mp4|webm|ogg|ogv|mov|m4v)[^\s]*/gi, '');
+ .replace(/\?(?:name|filename)=[^\s]*\.(?:png|jpe?g|gif|gifs|apng|webp|avif|svg|m4a|mp3|wav|flac|aac|opus|mp4|webm|ogg|ogv|mov|m4v)[^\s]*/gi, '');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| .replace(/\?[^\s]*\.(?:png|jpe?g|gif|gifs|apng|webp|avif|svg|m4a|mp3|wav|flac|aac|opus|mp4|webm|ogg|ogv|mov|m4v)[^\s]*/gi, '') | |
| .replace(/\?name=[^\s]*\.(?:png|jpe?g|gif|gifs|apng|webp|avif|svg|m4a|mp3|wav|flac|aac|opus|mp4|webm|ogg|ogv|mov|m4v)[^\s]*/gi, ''); | |
| .replace(/\?(?:name|filename)=[^\s]*\.(?:png|jpe?g|gif|gifs|apng|webp|avif|svg|m4a|mp3|wav|flac|aac|opus|mp4|webm|ogg|ogv|mov|m4v)[^\s]*/gi, ''); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/utils/textUtils.ts` around lines 23 - 24, The regex patterns in the
replace calls are too broad and strip any query parameter substring ending with
media file extensions, which corrupts unrelated URLs that contain extension-like
strings in their query text. Tighten both regex patterns to only match explicit
filename-style parameters by requiring the extension to be part of an actual
filename context (such as after an equals sign or in a filename parameter
format) rather than matching any arbitrary text followed by a media extension.
This ensures that search queries or other non-media URLs containing `.mp3` or
similar in their query values are not inadvertently corrupted.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This renders inline audio players for note links that point to audio files, so notes like the m4a example from issue 235 no longer fall back to a generic URL preview. It also keeps ambiguous
.oggand.webmlinks on the video path and falls back to the audio player when the media has no video track. Build preview: example note on preview. Fixes #235.AudioPlayerplus audio URL detection in note media extraction.oggand.webmvideo previews while falling back to audio for audio-only filesSummary by CodeRabbit