Add ffmpeg wasm #1822
Add ffmpeg wasm #1822
Conversation
📝 WalkthroughWalkthroughThis update introduces a comprehensive audio upload, recording, and editing workflow to the frontend viewer. It adds new Svelte components for audio dialogs, waveform visualization, and recording, integrates FFmpeg and WaveSurfer.js, and extends the dialogs service. Supporting build scripts, dependencies, and utility modules are also added or updated to enable browser-based audio processing and UI enhancements. Changes
Suggested labels
Poem
✨ Finishing Touches
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}
| const l = async ({ coreURL: t, wasmURL: n, workerURL: e }) => { | ||
| const o = !r; | ||
| try { | ||
| t || (t = R), importScripts(t); |
Check warning
Code scanning / CodeQL
Client-side URL redirect Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
To fix the issue, we need to validate the coreURL parameter (t) before using it in the importScripts function. A whitelist-based approach is recommended, where only URLs from a predefined list of trusted sources are allowed. If the provided URL does not match any trusted source, the code should either reject it or fall back to the default URL (R).
Steps to implement the fix:
- Define a list of trusted base URLs.
- Validate the
coreURLparameter against this list. - If the URL is not trusted, either throw an error or use the default URL (
R).
| @@ -12,3 +12,8 @@ | ||
| try { | ||
| t || (t = R), importScripts(t); | ||
| const trustedSources = ["https://unpkg.com/@ffmpeg/core"]; // Add trusted base URLs here | ||
| const isTrusted = trustedSources.some((base) => t && t.startsWith(base)); | ||
| if (!isTrusted) { | ||
| t = R; // Fallback to the default URL if the provided URL is not trusted | ||
| } | ||
| importScripts(t); | ||
| } catch { |
C# Unit Tests126 tests 126 ✅ 12s ⏱️ Results for commit 756df89. |
![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.
| function getPrimaryColor() { | ||
| return `hsl(${getComputedStyle(document.documentElement).getPropertyValue('--primary').trim()})`; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add SSR safety check for DOM access.
Direct access to document.documentElement will fail during server-side rendering.
Add a browser check:
function getPrimaryColor() {
+ if (typeof document === 'undefined') return 'hsl(var(--primary))';
return `hsl(${getComputedStyle(document.documentElement).getPropertyValue('--primary').trim()})`;
};📝 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.
| function getPrimaryColor() { | |
| return `hsl(${getComputedStyle(document.documentElement).getPropertyValue('--primary').trim()})`; | |
| }; | |
| function getPrimaryColor() { | |
| if (typeof document === 'undefined') return 'hsl(var(--primary))'; | |
| return `hsl(${getComputedStyle(document.documentElement).getPropertyValue('--primary').trim()})`; | |
| }; |
🤖 Prompt for AI Agents
In frontend/viewer/src/lib/components/audio/wavesurfer/wavesurfer-utils.ts
around lines 10 to 12, the function getPrimaryColor directly accesses
document.documentElement, which will cause errors during server-side rendering.
Add a check to ensure the code only runs in a browser environment by verifying
if window and document are defined before accessing document.documentElement. If
not in a browser, return a safe default value or handle accordingly to prevent
SSR failures.
| 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.
| void loadFFmpeg().then(async (ffmpeg) => { | ||
| if (!audio) return; | ||
| audio = await convertToWav(ffmpeg, audio); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for FFmpeg loading and conversion.
The FFmpeg loading and audio conversion lacks error handling, which could lead to unhandled promise rejections if FFmpeg fails to load or conversion fails.
- void loadFFmpeg().then(async (ffmpeg) => {
- if (!audio) return;
- audio = await convertToWav(ffmpeg, audio);
- });
+ let conversionError = $state<string | null>(null);
+
+ void loadFFmpeg().then(async (ffmpeg) => {
+ try {
+ if (!audio) return;
+ audio = await convertToWav(ffmpeg, audio);
+ conversionError = null;
+ } catch (error) {
+ console.error('Failed to convert audio:', error);
+ conversionError = error instanceof Error ? error.message : 'Audio conversion failed';
+ }
+ }).catch(error => {
+ console.error('Failed to load FFmpeg:', error);
+ conversionError = 'Failed to load audio processor';
+ });Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In frontend/viewer/src/lib/components/audio/audio-editor.svelte around lines 25
to 28, the code loading FFmpeg and converting audio lacks error handling,
risking unhandled promise rejections. Wrap the async operations in a try-catch
block inside the then callback to catch and handle any errors during FFmpeg
loading or audio conversion, and log or handle the errors appropriately.
| // 400 ms to opt out of walkie-talkie mode i.e. prevent pausing on release/up | ||
| releaseToStop = false; | ||
| walkedTalkieTimeout = setTimeout(() => releaseToStop = true, 400); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider clearing timeout on component cleanup.
The walkedTalkieTimeout should be cleared when the component is destroyed to prevent potential memory leaks.
Add cleanup in an onDestroy lifecycle:
+ import { onDestroy } from 'svelte';
+
+ onDestroy(() => {
+ clearTimeout(walkedTalkieTimeout);
+ });📝 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.
| // 400 ms to opt out of walkie-talkie mode i.e. prevent pausing on release/up | |
| releaseToStop = false; | |
| walkedTalkieTimeout = setTimeout(() => releaseToStop = true, 400); | |
| import { onDestroy } from 'svelte'; | |
| // 400 ms to opt out of walkie-talkie mode i.e. prevent pausing on release/up | |
| releaseToStop = false; | |
| walkedTalkieTimeout = setTimeout(() => releaseToStop = true, 400); | |
| onDestroy(() => { | |
| clearTimeout(walkedTalkieTimeout); | |
| }); |
🤖 Prompt for AI Agents
In frontend/viewer/src/lib/components/audio/recorder/recorder-trigger.svelte
around lines 42 to 44, the timeout stored in walkedTalkieTimeout is not cleared
on component destruction, which can cause memory leaks. Add an onDestroy
lifecycle function to clear the walkedTalkieTimeout using clearTimeout to ensure
the timeout is properly cleaned up when the component is destroyed.
| ]); | ||
|
|
||
| // Read output file | ||
| const data = await ffmpeg.readFile(outputName) as Uint8Array;; |
There was a problem hiding this comment.
Remove duplicate semicolon
- const data = await ffmpeg.readFile(outputName) as Uint8Array;;
+ const data = await ffmpeg.readFile(outputName) as Uint8Array;📝 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.
| const data = await ffmpeg.readFile(outputName) as Uint8Array;; | |
| const data = await ffmpeg.readFile(outputName) as Uint8Array; |
🤖 Prompt for AI Agents
In frontend/viewer/src/lib/components/audio/ffmpeg/ffmpeg-api.ts at line 54,
there is a duplicate semicolon at the end of the statement. Remove the extra
semicolon so that the line ends with a single semicolon after the Uint8Array
type assertion.
| {#if $isDev} | ||
| <Button variant="secondary" icon="i-mdi-microphone-plus" size="sm" iconProps={{class: 'size-5'}} onclick={onGetAudioClick}> | ||
| {$t`Add audio`} | ||
| </Button> | ||
| {:else} | ||
| <div class="text-muted-foreground p-1"> | ||
| {$t`No audio`} | ||
| </div> | ||
| {/if} |
There was a problem hiding this comment.
Production users cannot add audio
The "Add audio" button is only visible in development mode. Production users will only see "No audio" with no way to add audio content.
Is this intentional for the current draft, or should the button be available in production once the feature is complete?
🤖 Prompt for AI Agents
In frontend/viewer/src/lib/components/field-editors/audio-input.svelte around
lines 182 to 190, the "Add audio" button is conditionally rendered only for
development mode users, hiding it from production users who see only "No audio."
Clarify whether this is intentional for the current draft; if not, remove or
adjust the condition so the "Add audio" button is available to production users
once the feature is complete, ensuring all users can add audio content as
intended.
|
|
||
| let recorderContainerElem = $state<HTMLElement>(); | ||
| let recording = $state(false); | ||
| let walkieTalkieMode = $state<boolean>(null!); |
There was a problem hiding this comment.
Avoid TypeScript non-null assertion with null
Using null! is a TypeScript anti-pattern that bypasses type safety.
Initialize with a proper boolean value:
- let walkieTalkieMode = $state<boolean>(null!);
+ let walkieTalkieMode = $state<boolean>(false);🤖 Prompt for AI Agents
In frontend/viewer/src/lib/components/audio/audio-provider.svelte at line 24,
the variable walkieTalkieMode is initialized using the TypeScript non-null
assertion with null (null!), which bypasses type safety. Replace the
initialization with a proper boolean value such as false or true to ensure type
safety and avoid using the non-null assertion operator.
| import AudioProvider from './audio-provider.svelte'; | ||
| import AudioEditor from './audio-editor.svelte'; | ||
|
|
||
| let open = $state(true); |
There was a problem hiding this comment.
Fix initial dialog state to prevent unintended display.
The dialog opens immediately on component mount due to open being initialized to true. This could cause the dialog to appear unexpectedly when the component is first rendered.
-let open = $state(true);
+let open = $state(false);📝 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.
| let open = $state(true); | |
| let open = $state(false); |
🤖 Prompt for AI Agents
In frontend/viewer/src/lib/components/audio/AudioDialog.svelte at line 12, the
variable `open` is initialized to `true`, causing the dialog to open immediately
on component mount. Change the initial value of `open` to `false` to prevent the
dialog from displaying unexpectedly when the component first renders.
| async function processAudio(blob: Blob): Promise<Blob> { | ||
| await delay(1000); // Simulate processing delay | ||
| return blob; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement actual audio processing or remove placeholder.
The processAudio function currently only simulates processing with a delay but doesn't perform any actual audio processing. Consider implementing format conversion, compression, or validation logic.
Based on the PR objectives mentioning FFmpeg integration, consider:
-async function processAudio(blob: Blob): Promise<Blob> {
- await delay(1000); // Simulate processing delay
- return blob;
-}
+async function processAudio(blob: Blob): Promise<Blob> {
+ // TODO: Integrate with FFmpeg for audio processing
+ // Example: Convert to standardized format, apply compression, etc.
+ try {
+ // Add actual audio processing logic here
+ return blob; // Return processed blob
+ } catch (error) {
+ console.error('Audio processing failed:', error);
+ throw new Error('Failed to process audio');
+ }
+}📝 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.
| async function processAudio(blob: Blob): Promise<Blob> { | |
| await delay(1000); // Simulate processing delay | |
| return blob; | |
| } | |
| async function processAudio(blob: Blob): Promise<Blob> { | |
| // TODO: Integrate with FFmpeg for audio processing | |
| // Example: Convert to standardized format, apply compression, etc. | |
| try { | |
| // Add actual audio processing logic here | |
| return blob; // Return processed blob | |
| } catch (error) { | |
| console.error('Audio processing failed:', error); | |
| throw new Error('Failed to process audio'); | |
| } | |
| } |
🤖 Prompt for AI Agents
In frontend/viewer/src/lib/components/audio/AudioDialog.svelte around lines 92
to 95, the processAudio function currently only simulates a delay without
performing any real audio processing. To fix this, replace the placeholder delay
with actual audio processing logic such as format conversion, compression, or
validation, potentially using FFmpeg or another audio processing library as
indicated by the PR objectives.
| async function submitAudio() { | ||
| if (!audio) throw new Error('No audio to upload'); | ||
| if (!requester) throw new Error('No requester'); | ||
|
|
||
| submitting = true; | ||
| try { | ||
| const audioId = await uploadAudio(); | ||
| requester.resolve(audioId); | ||
| close(); | ||
| } finally { | ||
| submitting = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in submitAudio.
The function handles the finally block for loading state but doesn't handle errors properly. Consider adding user-friendly error handling.
async function submitAudio() {
if (!audio) throw new Error('No audio to upload');
if (!requester) throw new Error('No requester');
submitting = true;
try {
const audioId = await uploadAudio();
requester.resolve(audioId);
close();
+ } catch (error) {
+ console.error('Failed to submit audio:', error);
+ // TODO: Show user-friendly error message
+ // Consider adding toast notification or error state
} finally {
submitting = false;
}
}📝 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.
| async function submitAudio() { | |
| if (!audio) throw new Error('No audio to upload'); | |
| if (!requester) throw new Error('No requester'); | |
| submitting = true; | |
| try { | |
| const audioId = await uploadAudio(); | |
| requester.resolve(audioId); | |
| close(); | |
| } finally { | |
| submitting = false; | |
| } | |
| } | |
| async function submitAudio() { | |
| if (!audio) throw new Error('No audio to upload'); | |
| if (!requester) throw new Error('No requester'); | |
| submitting = true; | |
| try { | |
| const audioId = await uploadAudio(); | |
| requester.resolve(audioId); | |
| close(); | |
| } catch (error) { | |
| console.error('Failed to submit audio:', error); | |
| // TODO: Show user-friendly error message | |
| // Consider adding toast notification or error state | |
| } finally { | |
| submitting = false; | |
| } | |
| } |
🤖 Prompt for AI Agents
In frontend/viewer/src/lib/components/audio/AudioDialog.svelte around lines 54
to 66, the submitAudio function lacks proper error handling for the uploadAudio
call. Enhance the function by adding a try-catch block around the await
uploadAudio() call to catch any errors, then display a user-friendly error
message or notification instead of letting the error propagate silently. Ensure
the submitting state is still reset in the finally block.
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
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.