Add ffmpeg wasm #1822
Add ffmpeg wasm #1822
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis change introduces a complete FFmpeg WebAssembly integration into the frontend audio workflow. It adds new dependencies, build tooling, a Vite worker build, and implements a browser-based FFmpeg API and worker. Audio dialog and editor components are refactored to use the new FFmpeg pipeline for audio conversion and processing. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
💻 FW Lite
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
frontend/viewer/src/lib/components/audio/ffmpeg/bundled-ffmpeg-worker.js
Dismissed
Show dismissed
Hide dismissed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (11)
frontend/viewer/src/lib/components/audio/wavesurfer/wavesurfer-utils.ts (2)
58-61: Remove commented sample URLs.The commented sample URLs should be removed from production code.
- // sample audio - // url: 'https://cdn.freesound.org/previews/815/815388_16624953-lq.mp3', // 2.8s - // url: 'https://dl.espressif.com/dl/audio/gs-16b-2c-44100hz.aac', // 16s - // url: 'https://dl.espressif.com/dl/audio/ff-16b-2c-44100hz.aac', // 3m 7s
75-79: Simplify cleanup logic.The cleanup logic is redundant - both
onDestroyandwavesurfer.on('destroy')call the same unsubscribe function.onDestroy(() => { unsub(); wavesurfer.destroy(); }); - wavesurfer.on('destroy', unsub);The
onDestroycallback already handles cleanup when the component is destroyed, making the wavesurfer destroy event listener unnecessary.frontend/viewer/vite.config.ffmpeg-worker.ts (2)
4-5: Consider error handling for module resolution.The
require.resolve()call could fail if the FFmpeg package is not installed or the worker path changes. Consider adding error handling or validation.const require = createRequire(import.meta.url); -const ffmpegWorkerPath = require.resolve('@ffmpeg/ffmpeg/worker'); +let ffmpegWorkerPath: string; +try { + ffmpegWorkerPath = require.resolve('@ffmpeg/ffmpeg/worker'); +} catch (error) { + throw new Error(`Failed to resolve FFmpeg worker: ${error.message}`); +}
19-19: Consider making output directory configurable.The hardcoded output path
'src/lib/components/audio/ffmpeg'makes the configuration less flexible. Consider using a variable or environment-based configuration.+const outputDir = process.env.FFMPEG_WORKER_OUTPUT_DIR || 'src/lib/components/audio/ffmpeg'; + export default defineConfig({ // ... build: { // ... - outDir: 'src/lib/components/audio/ffmpeg', + outDir: outputDir, // ... }, });frontend/viewer/src/lib/components/ui/format/format-duration.ts (1)
16-16: Consider clarifying smallest unit logic.The smallest unit determination based on minutes seems logical, but consider documenting the reasoning or verifying that the
formatDurationfunction properly handles this parameter as expected.frontend/viewer/src/lib/components/audio/audio-editor.svelte (1)
56-57: Enhance audio control robustness.The play/pause logic could be more robust by checking the current state before attempting operations.
- onclick={() => (playing ? audioApi?.pause() : audioApi?.play())} + onclick={() => { + if (!audioApi) return; + if (playing) { + audioApi.pause(); + } else { + audioApi.play().catch(console.error); + } + }}frontend/viewer/src/lib/components/audio/ffmpeg/ffmpeg-api.ts (1)
36-42: Consider using a generic filename for inputThe function accepts any audio Blob but uses 'input.mp3' as the filename, which could be misleading if other formats are provided.
Consider using a generic filename or deriving it from the blob type:
- const inputName = 'input.mp3'; + const inputName = 'input.audio'; // FFmpeg will auto-detect the formatfrontend/viewer/src/lib/components/field-editors/audio-input.svelte (1)
162-170: Address TODO: Fake audio ID implementationThe TODO comment indicates that the audio ID implementation is incomplete. This should be addressed before merging.
Would you like me to help implement the proper audio ID handling or create an issue to track this task?
frontend/viewer/src/lib/components/audio/recorder/recorder.svelte (1)
146-146: Remove unnecessary semicolon- }; + }frontend/viewer/src/lib/components/audio/AudioDialog.svelte (2)
68-74: Replace mock upload implementation with actual audio upload.The
uploadAudiofunction is currently a simulation that generates a fake ID and doesn't actually upload audio. This needs to be replaced with real upload logic.-async function uploadAudio() { - if (!audio) throw new Error('No file selected'); - const name = (selectedFile?.name ?? audio.type); - const id = `audio-${name}-${Date.now()}`; - await delay(1000); - return id; -} +async function uploadAudio() { + if (!audio) throw new Error('No file selected'); + // TODO: Implement actual audio upload to server + const formData = new FormData(); + formData.append('audio', audio, selectedFile?.name || 'recording.wav'); + + const response = await fetch('/api/audio/upload', { + method: 'POST', + body: formData + }); + + if (!response.ok) { + throw new Error(`Upload failed: ${response.statusText}`); + } + + const result = await response.json(); + return result.id; +}Would you like me to implement the actual audio upload functionality or open an issue to track this task?
49-52: Add memory cleanup for audio blobs.Audio blobs can consume significant memory. Consider revoking object URLs and explicitly cleaning up blob references to prevent memory leaks.
function clearAudio() { + // Clean up blob URLs if any were created + if (audio && audio instanceof File === false) { + // If audio is a blob created from recording, ensure cleanup + } audio = selectedFile = undefined; submitting = false; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
frontend/package.json(1 hunks)frontend/viewer/package.json(3 hunks)frontend/viewer/src/lib/DialogsProvider.svelte(1 hunks)frontend/viewer/src/lib/components/audio/AudioDialog.svelte(1 hunks)frontend/viewer/src/lib/components/audio/audio-editor.svelte(1 hunks)frontend/viewer/src/lib/components/audio/audio-provider.svelte(1 hunks)frontend/viewer/src/lib/components/audio/ffmpeg/bundled-ffmpeg-worker.js(1 hunks)frontend/viewer/src/lib/components/audio/ffmpeg/ffmpeg-api.ts(1 hunks)frontend/viewer/src/lib/components/audio/ffmpeg/index.ts(1 hunks)frontend/viewer/src/lib/components/audio/recorder/index.ts(1 hunks)frontend/viewer/src/lib/components/audio/recorder/recorder-trigger.svelte(1 hunks)frontend/viewer/src/lib/components/audio/recorder/recorder.svelte(1 hunks)frontend/viewer/src/lib/components/audio/wavesurfer/waveform.svelte(1 hunks)frontend/viewer/src/lib/components/audio/wavesurfer/wavesurfer-utils.ts(1 hunks)frontend/viewer/src/lib/components/field-editors/audio-input.svelte(4 hunks)frontend/viewer/src/lib/components/ui/button/button.svelte(2 hunks)frontend/viewer/src/lib/components/ui/dialog/dialog-footer.svelte(1 hunks)frontend/viewer/src/lib/components/ui/format/format-duration.ts(1 hunks)frontend/viewer/src/lib/sandbox/Sandbox.svelte(3 hunks)frontend/viewer/src/lib/services/dialogs-service.ts(2 hunks)frontend/viewer/vite.config.ffmpeg-worker.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: myieye
PR: sillsdev/languageforge-lexbox#1817
File: frontend/viewer/src/lib/components/audio/wavesurfer/waveform.svelte:52-66
Timestamp: 2025-07-11T15:28:28.231Z
Learning: In the frontend/viewer audio components using WaveSurfer.js, errors from wavesurfer.load() and wavesurfer.loadBlob() methods are handled automatically by the system's error handling mechanism, so manual try-catch blocks are not needed.
Learnt from: myieye
PR: sillsdev/languageforge-lexbox#1817
File: frontend/viewer/src/lib/components/audio/wavesurfer/waveform.svelte:65-65
Timestamp: 2025-07-11T15:30:24.390Z
Learning: In the frontend/viewer audio components using WaveSurfer.js, the manual call to wavesurfer.play() after loading audio should be removed because WaveSurfer automatically handles playback when configured with the autoplay option, making manual play() calls redundant.
frontend/package.json (1)
Learnt from: rmunn
PR: sillsdev/languageforge-lexbox#1650
File: frontend/src/lib/i18n/locales/en.json:671-681
Timestamp: 2025-05-13T09:50:00.358Z
Learning: The Lexbox project uses the `\n\` pattern at the end of lines in JSON locale files to handle multiline strings, where the `\n` represents a newline character and the trailing backslash indicates line continuation. Standard JSON validators may flag this as invalid, but it's an established convention in the project that their tooling supports.
frontend/viewer/src/lib/DialogsProvider.svelte (1)
Learnt from: hahn-kev
PR: sillsdev/languageforge-lexbox#1710
File: frontend/viewer/src/project/browse/BrowseView.svelte:17-19
Timestamp: 2025-05-27T06:18:33.852Z
Learning: The NewEntryButton component in frontend/viewer/src/project/NewEntryButton.svelte already internally checks features.write permission and conditionally renders based on write access, so external disabled props are not needed.
frontend/viewer/src/lib/components/ui/button/button.svelte (1)
Learnt from: hahn-kev
PR: sillsdev/languageforge-lexbox#1710
File: frontend/viewer/src/project/browse/BrowseView.svelte:17-19
Timestamp: 2025-05-27T06:18:33.852Z
Learning: The NewEntryButton component in frontend/viewer/src/project/NewEntryButton.svelte already internally checks features.write permission and conditionally renders based on write access, so external disabled props are not needed.
frontend/viewer/src/lib/components/audio/wavesurfer/wavesurfer-utils.ts (2)
Learnt from: myieye
PR: sillsdev/languageforge-lexbox#1817
File: frontend/viewer/src/lib/components/audio/wavesurfer/waveform.svelte:65-65
Timestamp: 2025-07-11T15:30:24.390Z
Learning: In the frontend/viewer audio components using WaveSurfer.js, the manual call to wavesurfer.play() after loading audio should be removed because WaveSurfer automatically handles playback when configured with the autoplay option, making manual play() calls redundant.
Learnt from: myieye
PR: sillsdev/languageforge-lexbox#1817
File: frontend/viewer/src/lib/components/audio/wavesurfer/waveform.svelte:52-66
Timestamp: 2025-07-11T15:28:28.231Z
Learning: In the frontend/viewer audio components using WaveSurfer.js, errors from wavesurfer.load() and wavesurfer.loadBlob() methods are handled automatically by the system's error handling mechanism, so manual try-catch blocks are not needed.
frontend/viewer/src/lib/components/audio/audio-editor.svelte (3)
Learnt from: myieye
PR: sillsdev/languageforge-lexbox#1817
File: frontend/viewer/src/lib/components/audio/wavesurfer/waveform.svelte:52-66
Timestamp: 2025-07-11T15:28:28.231Z
Learning: In the frontend/viewer audio components using WaveSurfer.js, errors from wavesurfer.load() and wavesurfer.loadBlob() methods are handled automatically by the system's error handling mechanism, so manual try-catch blocks are not needed.
Learnt from: myieye
PR: sillsdev/languageforge-lexbox#1817
File: frontend/viewer/src/lib/components/audio/wavesurfer/waveform.svelte:65-65
Timestamp: 2025-07-11T15:30:24.390Z
Learning: In the frontend/viewer audio components using WaveSurfer.js, the manual call to wavesurfer.play() after loading audio should be removed because WaveSurfer automatically handles playback when configured with the autoplay option, making manual play() calls redundant.
Learnt from: hahn-kev
PR: sillsdev/languageforge-lexbox#1757
File: frontend/viewer/src/lib/components/field-editors/multi-select.svelte:130-136
Timestamp: 2025-06-18T05:13:00.591Z
Learning: In frontend/viewer/src/lib/components/field-editors/multi-select.svelte, the computeCommandScore function from 'bits-ui' handles empty filter strings appropriately and does not hide all options when the filter is empty, contrary to initial analysis assumptions.
frontend/viewer/src/lib/components/audio/recorder/recorder-trigger.svelte (1)
Learnt from: hahn-kev
PR: sillsdev/languageforge-lexbox#1710
File: frontend/viewer/src/project/browse/BrowseView.svelte:17-19
Timestamp: 2025-05-27T06:18:33.852Z
Learning: The NewEntryButton component in frontend/viewer/src/project/NewEntryButton.svelte already internally checks features.write permission and conditionally renders based on write access, so external disabled props are not needed.
frontend/viewer/src/lib/components/audio/ffmpeg/ffmpeg-api.ts (1)
Learnt from: myieye
PR: sillsdev/languageforge-lexbox#1817
File: frontend/viewer/src/lib/components/audio/wavesurfer/waveform.svelte:52-66
Timestamp: 2025-07-11T15:28:28.231Z
Learning: In the frontend/viewer audio components using WaveSurfer.js, errors from wavesurfer.load() and wavesurfer.loadBlob() methods are handled automatically by the system's error handling mechanism, so manual try-catch blocks are not needed.
frontend/viewer/src/lib/components/field-editors/audio-input.svelte (5)
Learnt from: myieye
PR: sillsdev/languageforge-lexbox#1817
File: frontend/viewer/src/lib/components/audio/wavesurfer/waveform.svelte:65-65
Timestamp: 2025-07-11T15:30:24.390Z
Learning: In the frontend/viewer audio components using WaveSurfer.js, the manual call to wavesurfer.play() after loading audio should be removed because WaveSurfer automatically handles playback when configured with the autoplay option, making manual play() calls redundant.
Learnt from: hahn-kev
PR: sillsdev/languageforge-lexbox#1757
File: frontend/viewer/src/lib/components/field-editors/multi-select.svelte:130-136
Timestamp: 2025-06-18T05:13:00.591Z
Learning: In frontend/viewer/src/lib/components/field-editors/multi-select.svelte, the computeCommandScore function from 'bits-ui' handles empty filter strings appropriately and does not hide all options when the filter is empty, contrary to initial analysis assumptions.
Learnt from: hahn-kev
PR: sillsdev/languageforge-lexbox#1710
File: frontend/viewer/src/project/browse/BrowseView.svelte:17-19
Timestamp: 2025-05-27T06:18:33.852Z
Learning: The NewEntryButton component in frontend/viewer/src/project/NewEntryButton.svelte already internally checks features.write permission and conditionally renders based on write access, so external disabled props are not needed.
Learnt from: myieye
PR: sillsdev/languageforge-lexbox#1802
File: frontend/viewer/src/project/NewEntryButton.svelte:36-36
Timestamp: 2025-07-04T17:00:57.368Z
Learning: In this codebase, `$props.id()` (Svelte rune) automatically returns a unique identifier per component instance, so components using it do not require an explicit `id` prop from parents.
Learnt from: myieye
PR: sillsdev/languageforge-lexbox#1817
File: frontend/viewer/src/lib/components/audio/wavesurfer/waveform.svelte:52-66
Timestamp: 2025-07-11T15:28:28.231Z
Learning: In the frontend/viewer audio components using WaveSurfer.js, errors from wavesurfer.load() and wavesurfer.loadBlob() methods are handled automatically by the system's error handling mechanism, so manual try-catch blocks are not needed.
frontend/viewer/src/lib/components/audio/AudioDialog.svelte (1)
Learnt from: myieye
PR: sillsdev/languageforge-lexbox#1817
File: frontend/viewer/src/lib/components/audio/wavesurfer/waveform.svelte:52-66
Timestamp: 2025-07-11T15:28:28.231Z
Learning: In the frontend/viewer audio components using WaveSurfer.js, errors from wavesurfer.load() and wavesurfer.loadBlob() methods are handled automatically by the system's error handling mechanism, so manual try-catch blocks are not needed.
frontend/viewer/src/lib/components/audio/recorder/recorder.svelte (1)
Learnt from: myieye
PR: sillsdev/languageforge-lexbox#1817
File: frontend/viewer/src/lib/components/audio/wavesurfer/waveform.svelte:52-66
Timestamp: 2025-07-11T15:28:28.231Z
Learning: In the frontend/viewer audio components using WaveSurfer.js, errors from wavesurfer.load() and wavesurfer.loadBlob() methods are handled automatically by the system's error handling mechanism, so manual try-catch blocks are not needed.
frontend/viewer/src/lib/components/audio/wavesurfer/waveform.svelte (2)
Learnt from: myieye
PR: sillsdev/languageforge-lexbox#1817
File: frontend/viewer/src/lib/components/audio/wavesurfer/waveform.svelte:65-65
Timestamp: 2025-07-11T15:30:24.390Z
Learning: In the frontend/viewer audio components using WaveSurfer.js, the manual call to wavesurfer.play() after loading audio should be removed because WaveSurfer automatically handles playback when configured with the autoplay option, making manual play() calls redundant.
Learnt from: myieye
PR: sillsdev/languageforge-lexbox#1817
File: frontend/viewer/src/lib/components/audio/wavesurfer/waveform.svelte:52-66
Timestamp: 2025-07-11T15:28:28.231Z
Learning: In the frontend/viewer audio components using WaveSurfer.js, errors from wavesurfer.load() and wavesurfer.loadBlob() methods are handled automatically by the system's error handling mechanism, so manual try-catch blocks are not needed.
🧬 Code Graph Analysis (2)
frontend/viewer/src/lib/components/ui/format/format-duration.ts (1)
frontend/viewer/src/lib/components/ui/format/index.ts (2)
normalizeDuration(10-10)formatDuration(9-9)
frontend/viewer/src/lib/components/audio/ffmpeg/ffmpeg-api.ts (1)
frontend/viewer/src/lib/notifications/notifications.ts (1)
error(51-66)
🪛 GitHub Check: CodeQL
frontend/viewer/src/lib/components/audio/ffmpeg/bundled-ffmpeg-worker.js
[warning] 13-13: Client-side URL redirect
Untrusted URL redirection depends on a user-provided value.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Build UI / publish-ui
- GitHub Check: Build API / publish-api
- GitHub Check: check-and-lint
- GitHub Check: Build FW Lite and run tests
- GitHub Check: frontend
- GitHub Check: frontend-component-unit-tests
- GitHub Check: Analyze (csharp)
🔇 Additional comments (32)
frontend/package.json (1)
122-122: Good formatting improvement.Adding a newline at the end of the file follows standard conventions.
frontend/viewer/src/lib/DialogsProvider.svelte (2)
4-4: Good integration following established patterns.The AudioDialog import follows the same pattern as the existing dialog imports.
9-9: Proper component integration.Adding the AudioDialog component alongside existing dialogs maintains consistency with the dialogs provider pattern.
frontend/viewer/src/lib/components/audio/ffmpeg/index.ts (1)
1-1: Good module organization.The re-export pattern centralizes access to FFmpeg functionality and follows standard module organization practices.
frontend/viewer/src/lib/sandbox/Sandbox.svelte (3)
36-36: Appropriate import for testing.The FFmpeg import is correctly added for sandbox testing purposes.
129-133: Good testing implementation.The async preload function properly awaits FFmpeg loading and provides useful logging for debugging. This is appropriate for sandbox testing.
145-145: Button integration follows sandbox patterns.The FFmpeg load button follows the established pattern of other testing buttons in the sandbox.
frontend/viewer/src/lib/components/ui/dialog/dialog-footer.svelte (1)
16-16: LGTM! Clean flexbox enhancement for audio dialog support.The addition of
items-endandflex-wrapclasses improves the dialog footer's layout flexibility to better accommodate the new audio dialog components.frontend/viewer/src/lib/components/audio/recorder/index.ts (1)
1-10: LGTM! Clean barrel export pattern for recorder components.The implementation follows standard TypeScript module patterns with clear component exports and meaningful aliases.
frontend/viewer/src/lib/components/ui/button/button.svelte (2)
25-31: LGTM! Improved icon button sizing with overflow protection.The addition of max-height/width constraints prevents icon buttons from growing unexpectedly, and the new
xl-iconvariant provides needed size flexibility for the audio components.
82-84: LGTM! Consistent icon alignment withalign-middle.Adding
align-middleto both loading and regular icons ensures consistent vertical alignment across all button states.frontend/viewer/src/lib/components/ui/format/format-duration.ts (1)
8-24: LGTM! Well-implemented digital duration formatter.The function correctly reuses existing utilities and provides appropriate digital formatting options. The conditional display logic for hours/minutes makes sense for a digital clock-style display.
frontend/viewer/package.json (3)
23-23: FFmpeg worker build configuration verified.The file
frontend/viewer/vite.config.ffmpeg-worker.tsexists and correctly configures both the esbuild banner/include and the library build settings (entry, format, filename, and output directory) for bundling the FFmpeg worker. No further changes needed here.
101-104: FFmpeg dependencies verified and up-to-dateI’ve checked Snyk’s vulnerability database and there are no known security issues in:
- @ffmpeg/ffmpeg 0.12.15
- @ffmpeg/util 0.12.2
- @ffmpeg/core 0.12.10
These are all aligned 0.12.x releases from the ffmpeg.wasm project and are intended to work together. The CVE-2025-0518 out-of-bounds read in ffmpeg 7.1 affects the native binary and does not impact these WebAssembly builds.
Keep monitoring the ffmpeg.wasm changelog and security advisories for any future updates.
123-123: Confirm WaveSurfer.js v7.9.9 SuitabilityWaveSurfer.js 7.9.9 is the latest stable release (published June 28, 2025) and includes a full TypeScript rewrite, improved performance, a typed API, and revamped plugin architecture. Before locking this version, please verify that none of the following known issues impact our use case:
• Spectrogram plugin zoom can lag (issue #4079)
• Incompatibility in certain browsers (e.g., WeChat in-app browser, issue #4069)
• Real-time waveform recording may drop data or resize erratically after ~1 minute (issue #4065)
• Videos without an audio track render no waveform (issue #4059)
• Scrollable waveform flickers in flexbox layouts (issue #4055)
• Chrome on macOS (v12–14.6) may distort audio tracks (issue #4052)
• Firefox rendering anomalies in some scenarios (issue #4051)
• CORS headers required for externally loaded audio
• Large-file decoding can exhaust browser memory—consider pre-decoded peaks
• Reliable streaming demands pre-decoded peaks and duration metadataIf any of these affect our feature set, adjust configuration or defer the upgrade. Otherwise, locking to ^7.9.9 is safe.
frontend/viewer/src/lib/services/dialogs-service.ts (2)
25-28: LGTM! Audio dialog integration follows established pattern.The implementation correctly follows the same pattern as existing dialog methods with proper type safety and error handling.
45-48: AudioDialog integration confirmedThe
AudioDialog.sveltecomponent correctly registers itself with the service by setting:
- File:
frontend/viewer/src/lib/components/audio/AudioDialog.svelte
dialogsService.invokeAudioDialog = getAudio;No further changes are needed.
frontend/viewer/src/lib/components/audio/audio-editor.svelte (1)
49-50: Good use of CSS containment for waveform stability.The CSS containment strategy to prevent WaveSurfer layout issues is well-documented and appropriately applied.
frontend/viewer/src/lib/components/audio/wavesurfer/waveform.svelte (2)
30-46: LGTM! Proper WaveSurfer lifecycle management.The component correctly handles WaveSurfer instance creation, destruction, and recreation when container or audio changes. The event listeners for play/pause synchronization are appropriately set up.
55-67: Audio loading implementation follows best practices.The loadAudio function correctly handles both URL strings and Blobs. Based on retrieved learnings, the error handling for wavesurfer.load() and wavesurfer.loadBlob() is handled automatically by the system, so no manual try-catch blocks are needed here.
frontend/viewer/src/lib/components/audio/recorder/recorder-trigger.svelte (4)
27-46: Good prevention of concurrent recording starts.The implementation correctly prevents multiple concurrent recording starts using the
waitingForRecorderToStartflag and proper async/await handling.
60-68: Proper mouse button filtering for pointer events.The implementation correctly filters for left mouse button (button === 0) to avoid unintended triggers from other mouse buttons.
76-86: Good keyboard accessibility implementation.The component properly handles both Enter and Space keys for recording control, with appropriate
!event.repeatchecks to prevent multiple triggers from held keys.
88-102: Dynamic styling and event handling well-implemented.The derived props correctly handle visual state changes and event binding. The use of
mergePropsprovides good extensibility for consumers.frontend/viewer/src/lib/components/audio/ffmpeg/ffmpeg-api.ts (1)
30-30: Confirm FFmpeg worker availability in production
TheclassWorkerURLis only set whenimport.meta.env.DEVis true, so production builds fall back to createFFmpeg’s default worker loader. Please verify that the Vite-builtbundled-ffmpeg-worker.jsis served correctly and that no CORS or 404 errors occur when loading FFmpeg in a production build.• File: frontend/viewer/src/lib/components/audio/ffmpeg/ffmpeg-api.ts, Line 30
• Test a production build in the browser and inspect the Network/Console to ensure the worker script is fetched and instantiated without errors.frontend/viewer/src/lib/components/audio/ffmpeg/bundled-ffmpeg-worker.js (1)
1-3: Generated file - limited review scopeThis is a Vite-generated bundle file. Consider adding it to
.gitignoreif the build process can regenerate it, or ensure the source files are properly reviewed.frontend/viewer/src/lib/components/audio/audio-provider.svelte (1)
34-46: Well-structured file validationThe file selection validation properly checks for audio files and handles errors gracefully by resetting the input before re-throwing.
frontend/viewer/src/lib/components/audio/recorder/recorder.svelte (2)
50-79: Well-implemented recording state managementThe recording control logic properly validates state before operations and includes comprehensive error handling. The microphone preparation strategy to reduce latency is a good UX consideration.
96-103: Excellent resource cleanup implementationThe reset function properly cleans up all resources including media streams, preventing memory leaks and ensuring proper resource management.
frontend/viewer/src/lib/components/audio/AudioDialog.svelte (3)
13-15: LGTM: Clean integration with services and back handler.The back handler integration and dialogs service setup are well implemented. The service method assignment pattern allows for clean separation of concerns.
34-36: LGTM: Proper cleanup on dialog close.The watch function correctly handles cleanup when the dialog closes, ensuring state is reset properly.
98-118: LGTM: Clean conditional rendering and dialog structure.The template structure is well-organized with appropriate conditional rendering. The dialog footer is correctly placed only when audio is present, and the loading state is properly handled.
frontend/viewer/src/lib/components/audio/wavesurfer/wavesurfer-utils.ts
Outdated
Show resolved
Hide resolved
| function darkenColor(color: string): string { | ||
| return `oklch(from ${color} calc(l * .50) c h)`; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify browser compatibility for modern CSS features.
The oklch() and calc() functions with from syntax are modern CSS features that may not be supported in all browsers.
Please verify browser compatibility requirements for the project and consider adding fallbacks if needed.
🌐 Web query:
What is the browser support for CSS oklch() function and the "from" syntax in calc()?
💡 Result:
Browser support for the CSS oklch() function:
- The oklch() color function is now widely supported in all major browsers:
- Chrome: 111+
- Edge: 111+
- Firefox: 113+
- Safari: 15.4+
- Opera: 97+
These versions support specifying colors using the oklch() syntax directly in CSS, and you don't need any polyfills for modern browsers[1][5][9].
Browser support for "from" syntax in calc() and oklch():
- The "from" syntax (e.g.,
oklch(from var(--origin) l c h)) allows you to derive a new color by referencing and modifying the channels of another color[5]. - This syntax is part of the CSS Color Module Level 5, which is still progressing through standardization and browser implementation[5].
Current support for "from" syntax:
- While all major browsers support standard
oklch()usage, full support for the "from" syntax inoklch()and "from" incalc()remains limited and experimental:- The relative color syntax (the "from" form) is available in some Chromium-based browsers behind a flag or in developer/beta builds, but is not yet enabled by default in stable releases[4][5].
- There is no explicit mention in the search results of stable support for the "from" syntax in any browser as of mid-2025. This means you should not rely on this syntax for production without testing in your project's target browsers.
Summary Table:
| Feature | Chrome | Edge | Firefox | Safari | Opera |
|---|---|---|---|---|---|
| oklch() function | 111+ | 111+ | 113+ | 15.4+ | 97+ |
| oklch() "from" syntax | Experimental | Experimental | Experimental | Experimental | Experimental |
| calc() with standard math | 26+ | 9+ | 16+ | 7+ | 15+ |
Key Takeaways:
- You can confidently use oklch() and calc() in all modern browsers[1][9][10].
- The "from" color syntax in oklch/calc is experimental; check browser release notes and feature flags if you want to experiment, but do not depend on it for cross-browser code yet[4][5].
Citations:
- 1: https://javascript.plainenglish.io/modern-css-for-generating-colors-with-oklch-function-ca1870d73c05
- 2: https://developer.mozilla.org/en-US/docs/Web/CSS/calc
- 3: https://www.lambdatest.com/web-technologies/calc
- 4: https://developer.chrome.com/blog/css-relative-color-syntax
- 5: https://evilmartians.com/chronicles/oklch-in-css-why-quit-rgb-hsl
- 6: https://jdsteinbach.com/css/where-to-use-css-calc-real-world/
- 7: https://pieces.app/blog/introduction-to-building-responsive-ui-with-css-calc
- 8: https://www.w3.org/TR/css-color-4/
- 9: https://www.w3schools.com/cssref/func_oklch.php
- 10: https://www.w3schools.com/cssref/func_calc.php
Add a fallback for the experimental “from” color syntax
Based on browser support data, the plain oklch() function is well supported in all modern browsers (Chrome 111+, Edge 111+, Firefox 113+, Safari 15.4+, Opera 97+), but the relative‐color “from” syntax is still experimental and not enabled by default in stable releases. Relying on it without a fallback may cause styling breakage in unsupported browsers.
Please update the darkenColor util to detect support and provide an alternative:
• File: frontend/viewer/src/lib/components/audio/wavesurfer/wavesurfer-utils.ts (lines 14–16)
– Wrap the “from” call in a CSS.supports('color', …) check
– On unsupported browsers, compute a fallback (e.g., manual LCH or HSL darkening via a helper)
Example diff:
function darkenColor(color: string): string {
- return `oklch(from ${color} calc(l * .50) c h)`;
+ const fromSyntax = `oklch(from ${color} calc(l * .50) c h)`;
+ if (CSS.supports('color', fromSyntax)) {
+ return fromSyntax;
+ }
+ // fallback: manually reduce lightness by 50%
+ return fallbackDarken(color, 0.5);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/viewer/src/lib/components/audio/wavesurfer/wavesurfer-utils.ts
around lines 14 to 16, the darkenColor function uses the experimental "from"
syntax in the oklch color function which lacks broad browser support. To fix
this, add a CSS.supports('color', ...) check to detect if the "from" syntax is
supported. If supported, use the current implementation; otherwise, provide a
fallback by manually darkening the color using a helper function that adjusts
the color in LCH or HSL space without the "from" syntax.
frontend/viewer/src/lib/components/audio/recorder/recorder-trigger.svelte
Show resolved
Hide resolved
frontend/viewer/src/lib/components/field-editors/audio-input.svelte
Outdated
Show resolved
Hide resolved
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
|
I found this npm package that might help us work around this issue: https://github.com/CezaryDanielNowak/CrossOriginWorker |
^ Conflicts: ^ frontend/viewer/src/lib/components/audio/wavesurfer/waveform.svelte
# Conflicts: # frontend/pnpm-lock.yaml # frontend/viewer/package.json
hahn-kev
removed
the
📦 Lexbox
|
Alright, I've got this into a place where I'm happy with it. I tried to get silence trimming to work, but was running into problems with that, I'm not sure what was wrong as the filter looks like it's supported. But that's alright it's a feature for later. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/viewer/src/lib/components/audio/audio-editor.svelte (1)
33-48: Error handling still missing despite previous feedback.The FFmpeg resource pipeline lacks proper error handling, which was flagged in a previous review. If any stage fails (FFmpeg loading, file conversion, or file reading), it could lead to unhandled promise rejections.
The resources need try-catch blocks to handle errors gracefully:
let ffmpegFile = resource(() => audio, async (audio, _, {signal}) => { + try { ffmpegApi ??= await FFmpegApi.create(); return await ffmpegApi.toFFmpegFile(audio, signal); + } catch (error) { + console.error('Failed to process audio file:', error); + throw error; + } }); let flacFile = resource(() => [ffmpegFile.current], async ([file], _, {signal}) => { if (!file) return; + try { ffmpegApi ??= await FFmpegApi.create(); return await ffmpegApi.convertToFlac(file, signal); + } catch (error) { + console.error('Failed to convert to FLAC:', error); + throw error; + } }); let readFile = resource(() => [flacFile.current], async ([file], _, {signal}) => { if (!file) return; + try { ffmpegApi ??= await FFmpegApi.create(); return await ffmpegApi.readFile(file, signal); + } catch (error) { + console.error('Failed to read processed file:', error); + throw error; + } });
🧹 Nitpick comments (1)
frontend/viewer/src/lib/components/audio/audio-editor.svelte (1)
61-65: AbortController not integrated with resources.The
abortControlleris created but not used by the resources, which have their ownsignalparameter. Either remove the unused controller or integrate it properly with the resource pipeline.If you want to use the component-level abort controller:
-let ffmpegFile = resource(() => audio, async (audio, _, {signal}) => { +let ffmpegFile = resource(() => audio, async (audio) => { ffmpegApi ??= await FFmpegApi.create(); - return await ffmpegApi.toFFmpegFile(audio, signal); + return await ffmpegApi.toFFmpegFile(audio, abortController.signal); });Or simply remove the unused controller:
-const abortController = new AbortController(); onDestroy(() => { - abortController.abort(); ffmpegApi?.terminate(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
frontend/viewer/package.json(2 hunks)frontend/viewer/src/lib/components/audio/AudioDialog.svelte(6 hunks)frontend/viewer/src/lib/components/audio/audio-editor.svelte(1 hunks)frontend/viewer/src/lib/components/audio/ffmpeg/bundled-ffmpeg-worker.js(1 hunks)frontend/viewer/src/lib/components/audio/ffmpeg/ffmpeg-api.ts(1 hunks)frontend/viewer/src/lib/components/audio/ffmpeg/index.ts(1 hunks)frontend/viewer/src/lib/sandbox/Sandbox.svelte(3 hunks)frontend/viewer/vite.config.ffmpeg-worker.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- frontend/viewer/src/lib/sandbox/Sandbox.svelte
- frontend/viewer/src/lib/components/audio/ffmpeg/index.ts
- frontend/viewer/vite.config.ffmpeg-worker.ts
- frontend/viewer/package.json
- frontend/viewer/src/lib/components/audio/AudioDialog.svelte
- frontend/viewer/src/lib/components/audio/ffmpeg/ffmpeg-api.ts
🧰 Additional context used
🧠 Learnings (1)
frontend/viewer/src/lib/components/audio/audio-editor.svelte (4)
Learnt from: myieye
PR: #1817
File: frontend/viewer/src/lib/components/audio/wavesurfer/waveform.svelte:52-66
Timestamp: 2025-07-11T15:28:28.240Z
Learning: In the frontend/viewer audio components using WaveSurfer.js, errors from wavesurfer.load() and wavesurfer.loadBlob() methods are handled automatically by the system's error handling mechanism, so manual try-catch blocks are not needed.
Learnt from: myieye
PR: #1817
File: frontend/viewer/src/lib/components/audio/wavesurfer/waveform.svelte:65-65
Timestamp: 2025-07-11T15:30:24.410Z
Learning: In the frontend/viewer audio components using WaveSurfer.js, the manual call to wavesurfer.play() after loading audio should be removed because WaveSurfer automatically handles playback when configured with the autoplay option, making manual play() calls redundant.
Learnt from: rmunn
PR: #1844
File: frontend/viewer/src/lib/entry-editor/ItemListItem.svelte:26-37
Timestamp: 2025-07-30T04:53:41.702Z
Learning: In frontend/viewer/src/lib/entry-editor/ItemListItem.svelte, the TODO comments for unused props index and actions are intentional reminders for future work to be completed in a separate PR, not issues to be resolved immediately. These represent planned functionality that will be implemented later.
Learnt from: hahn-kev
PR: #1757
File: frontend/viewer/src/lib/components/field-editors/multi-select.svelte:130-136
Timestamp: 2025-06-18T05:13:00.591Z
Learning: In frontend/viewer/src/lib/components/field-editors/multi-select.svelte, the computeCommandScore function from 'bits-ui' handles empty filter strings appropriately and does not hide all options when the filter is empty, contrary to initial analysis assumptions.
🪛 GitHub Check: CodeQL
frontend/viewer/src/lib/components/audio/ffmpeg/bundled-ffmpeg-worker.js
[warning] 13-13: Client-side URL redirect
Untrusted URL redirection depends on a user-provided value.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build API / publish-api
- GitHub Check: check-and-lint
- GitHub Check: frontend
- GitHub Check: Build FW Lite and run tests
- GitHub Check: Analyze (csharp)
🔇 Additional comments (9)
frontend/viewer/src/lib/components/audio/audio-editor.svelte (5)
9-12: LGTM - Clean imports for FFmpeg integration.The new imports properly support the FFmpeg WebAssembly integration and reactive resource management.
14-31: Well-structured props and state management.The changes to make
audiorequired andfinalAudiobindable properly support the new processing pipeline. TheffmpegApistate variable enables efficient reuse across multiple resource calls.
50-56: Proper state synchronization with watch effect.The watch effect correctly synchronizes the
finalAudiobindable prop with the resource pipeline state, handling both loading and success scenarios appropriately.
69-104: Well-implemented UI with proper loading and error states.The UI properly handles loading states, displays metadata from the processed audio, and includes helpful CSS comments explaining the containment strategy. The error display provides good user feedback.
93-93: Correct discard button logic.Using the original
audioprop for the discard button's disabled state is appropriate, asfinalAudiomay be temporarily undefined during processing.frontend/viewer/src/lib/components/audio/ffmpeg/bundled-ffmpeg-worker.js (4)
1-3: Generated file - address issues in source code.This is a generated file from the Vite build process. Any significant issues should be addressed in the source TypeScript files rather than this minified output.
49-104: Well-structured worker message handling.The worker properly handles various message types, includes appropriate error handling for edge cases, and efficiently transfers binary data using transferable objects.
30-48: Comprehensive FFmpeg operation support.The worker provides a complete set of FFmpeg operations including command execution, file system manipulation, and mounting operations. The timeout support helps prevent hanging operations.
10-20: No security risk:coreURLis a build-time constantThe
coreURLpassed intoffmpeg.load({ coreURL, … })comes from the static importimport coreURL from '@ffmpeg/core?url';at build time—not from any end-user input—so neither
importScripts(coreURL)nor the dynamic ESM import can be influenced by an attacker.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Haven't found any glaring issues in the code so far, but I want to test this out before I approve it. Did find one minor issue, and I have one concern about the UX of having a recording rejected, see below.
I'll approve this once I've tested it and verified that it works on more than just the original dev's machine. 😄
# Conflicts: # frontend/viewer/src/locales/en.po # frontend/viewer/src/locales/es.po # frontend/viewer/src/locales/fr.po # frontend/viewer/src/locales/id.po # frontend/viewer/src/locales/ko.po
3 participants
WIP/draft because loading ffmpeg was just slapped somewhere awkward to demonstrate that it works.
So, it works, but I'm not very happy with how loading the ffmpeg worker is set up.
In prod, we don't do anything fancy at all and it just works, yay!
But in dev:
I've implemented 1, but my implementation involves:
So.... maybe 2 is better, because it would maybe only mean manually bundling the worker and then dev and prod might be able to be the same?
But then we'd be responsible to make sure browsers don't cache old versions, even in prod. (e.g. add a hash and figure out how to import it anyway).
Or maybe 3 is better, whatever 3 might be.