Fix/audio wrong layer and audio playback#482
Merged
ExtraBinoss merged 25 commits intoMay 9, 2026
Merged
Conversation
…r logic about audio here. reducing further video editor
…dio would not work.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughI can generate the full review stack and walkthrough, but the hidden review artifact must list every provided rangeId exactly once. That’s a large, precise task — I need a bit more time to produce it correctly. Do you want me to proceed and generate the complete artifact now? ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR restructures audio handling across the editor and exporter to address incorrect audio layering and improve playback/export stability. It introduces a dedicated audio routing layer (system/mic/mixed vs embedded), per-clip muting and source-track controls, and worker-based waveform generation for timeline rendering.
Changes:
- Refactors audio routing for playback/export (embedded vs sidecar tracks) and adds per-clip mute + per-track (system/mic/mixed) volume/normalize settings.
- Moves preview-audio synchronization logic out of
VideoEditorinto a dedicated audio hook/module and extends exporter pipelines to honor new audio settings. - Adds waveform generation via Web Workers and extends timeline UI to render source-audio tracks and per-audio-item waveforms.
Reviewed changes
Copilot reviewed 44 out of 44 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/exporter/videoExporter.ts | Passes clip/source-track audio settings into export and triggers audio render when mute/settings require it. |
| src/lib/exporter/modernVideoExporter.ts | Mirrors exporter changes for the modern pipeline (clip mute + source-track settings). |
| src/lib/exporter/audioEncoder.ts | Updates audio processing to use source-track routing policy, clip-mute ranges, and per-track gains/normalization. |
| src/lib/exporter/audioRoutingEngine.ts | Introduces a resolved audio plan abstraction (embedded + sidecar + user regions). |
| src/lib/exporter/sourceTrackRoutingPolicy.ts | Wraps routing engine to produce a policy used by editor/exporter decisions. |
| src/lib/exporter/sourceTrackRoutingPolicy.test.ts | Adds Vitest coverage for routing policy behavior across sidecar combinations. |
| src/components/video-editor/VideoEditor.tsx | Removes inline audio preview sync and delegates to useVideoEditorAudio; threads new audio settings into export + UI. |
| src/components/video-editor/types.ts | Adds audio effect section, clip showSourceAudio, and audio-region normalize; re-exports source track settings types. |
| src/components/video-editor/timeline/TimelineEditor.tsx | Adds source-audio track support and live span previews to drive waveform/row previews during drag/resize. |
| src/components/video-editor/timeline/model/timelineModel.ts | Extends timeline render items with clip mute/showSourceAudio and audio item waveform metadata. |
| src/components/video-editor/timeline/Item.tsx | Renders audio waveforms on items and a muted overlay for source-audio items. |
| src/components/video-editor/timeline/hooks/useTimelineAudioPeaks.ts | Switches waveform extraction to worker-based generator and adds sidecar fallback logic. |
| src/components/video-editor/timeline/core/timelineTypes.ts | Extends render item shape for audio waveform + clip audio flags. |
| src/components/video-editor/timeline/core/constants.ts | Adds SOURCE_AUDIO_ROW_ID and a shared default peak count. |
| src/components/video-editor/timeline/components/wrapper/TimelineWrapper.tsx | Adds live span preview callbacks during drag/resize. |
| src/components/video-editor/timeline/components/waveform/AudioWaveform.tsx | Enhances waveform renderer with segment bounds, gain/normalize shaping, and configurable className. |
| src/components/video-editor/timeline/components/viewport/TimelineCanvas.tsx | Adds dedicated source-audio rows per track (system/mic/mixed) and per-audio-item waveform rendering. |
| src/components/video-editor/SettingsPanel.tsx | Adds an Audio effect section + clip source-audio controls and normalization toggles. |
| src/components/video-editor/projectPersistence.ts | Persists/normalizes new clip/audio fields and source-audio track settings. |
| src/components/video-editor/audio/waveform/WaveformGenerator.ts | Adds worker-backed peak computation with caching and shared generator instance. |
| src/components/video-editor/audio/waveform/waveform.worker.ts | Implements worker peak calculation for waveform bins. |
| src/components/video-editor/audio/useVideoEditorAudio.ts | Centralizes editor audio orchestration (fallback discovery, routing, clip mute, preview sync). |
| src/components/video-editor/audio/useSourceAudioTrackSettings.ts | Manages default/per-clip source track settings and exposes setters for UI. |
| src/components/video-editor/audio/useSourceAudioFallback.ts | Encapsulates fetching companion audio paths + delay metadata via Electron API. |
| src/components/video-editor/audio/useClipAudioSettingsController.ts | Bridges source-track settings into preview gain calculations and UI callbacks. |
| src/components/video-editor/audio/useAudioPreviewSync.ts | Reimplements preview sync for user regions + companion tracks with per-track gain nodes. |
| src/components/video-editor/audio/clipAudio.ts | Adds helpers to resolve active clip and muted state at a given time. |
| src/components/video-editor/audio/audioTypes.ts | Defines source track setting/meta types and shared audio constants/IDs. |
| src/components/video-editor/AnnotationSettingsPanel.tsx | Adjusts panel layout so delete action is pinned in a footer area. |
| electron/ipc/recording/windows.ts | Stops deleting sidecar audio after mux and updates mux result metadata accordingly. |
| electron/ipc/recording/mac.ts | Stops deleting sidecar audio files after mux on macOS. |
| electron/ipc/recording/diagnostics.ts | Expands fallback discovery to include system sidecars (not just mic) when embedded audio exists. |
| electron/ipc/recording/audioFilters.ts | Updates sidecar debug env comment and retains env constant export. |
| electron/ipc/project/manager.ts | Approves editor.audioRegions[].audioPath for local media access policy on project load. |
| electron/ipc/project/manager.test.ts | Adds test ensuring audio region paths are approved during project load. |
| src/i18n/locales/en/settings.json | Adds new clip/audio settings strings (mute state, separate audio, source track labels). |
| src/i18n/locales/es/settings.json | Adds new clip/audio settings strings. |
| src/i18n/locales/fr/settings.json | Adds new clip/audio settings strings. |
| src/i18n/locales/ko/settings.json | Adds new clip/audio settings strings. |
| src/i18n/locales/nl/settings.json | Adds new clip/audio settings strings. |
| src/i18n/locales/pt-BR/settings.json | Adds new clip/audio settings strings. |
| src/i18n/locales/ru/settings.json | Adds new clip/audio settings strings. |
| src/i18n/locales/zh-CN/settings.json | Adds new clip/audio settings strings and updates GitHub CTA wording. |
| src/i18n/locales/zh-TW/settings.json | Adds new clip/audio settings strings. |
![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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
electron/ipc/recording/windows.ts (1)
281-331:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
keptAudioSidecarsis now inconsistent with actual sidecar handling.On Line 483 you always return
keptAudioSidecars: true, but Line 296-Line 298 can still delete sidecars and Line 331 can still returnkeepAudioSidecars(possiblyfalse). This can misreport state and break downstream cleanup/diagnostics logic.Suggested alignment (always-keep policy)
import { - shouldKeepRecordingAudioSidecars, WINDOWS_NATIVE_MIC_PRE_FILTERS, } from "./audioFilters"; @@ - const keepAudioSidecars = shouldKeepRecordingAudioSidecars(); const inputs: string[] = ["-i", videoPath]; @@ if (stat.size <= 0) { console.warn(`[mux-win] Skipping ${label} audio: file is empty (${audioPath})`); - if (!keepAudioSidecars) { - await fs.rm(audioPath, { force: true }).catch(() => undefined); - } continue; } @@ if (audioInputs.length === 0) { return { muxed: false, videoDurationSeconds: videoDuration, muxTimeoutMs, audioInputs, audio, - keptAudioSidecars: keepAudioSidecars, + keptAudioSidecars: true, }; }Also applies to: 483-483
🤖 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 `@electron/ipc/recording/windows.ts` around lines 281 - 331, The returned keptAudioSidecars value is inconsistent: use the policy flag instead of a hardcoded true. Replace any occurrences of a literal true in returned objects with the local variable keepAudioSidecars (from shouldKeepRecordingAudioSidecars()), e.g. update the return in the early-exit after audioInputs.length === 0 and the later return that currently sets keptAudioSidecars: true to keptAudioSidecars: keepAudioSidecars so the returned state matches actual deletion behavior controlled by shouldKeepRecordingAudioSidecars().src/lib/exporter/modernVideoExporter.ts (2)
757-770:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPass
clipRegionsinto the browser audio-processing path.
buildNativeAudioPlan()now switches to edited-track mode for muted clips, but thisAudioProcessor.process(...)call still omitsclipRegions. On the WebCodecs path, muted clip audio can therefore leak back into the export while the native/offline paths mute it correctly.Suggested fix
this.audioProcessor!.process( demuxer, this.muxer!, this.config.videoUrl, this.config.trimRegions, this.config.speedRegions, undefined, this.config.audioRegions, this.config.sourceAudioFallbackPaths, this.config.sourceAudioFallbackStartDelayMsByPath, this.config.sourceAudioTrackSettings, + this.config.clipRegions, ),🤖 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/exporter/modernVideoExporter.ts` around lines 757 - 770, The browser audio-processing path is missing clipRegions when calling AudioProcessor.process, causing muted clips to not be honored by buildNativeAudioPlan; update the call inside measureFinalizationStage / awaitWithFinalizationTimeout to pass this.config.clipRegions as the clipRegions argument to AudioProcessor.process (keeping the same argument order as the method signature), so the browser/WebCodecs path receives clipRegions just like the native/offline paths and muted clips are correctly silenced.
1195-1230:⚠️ Potential issue | 🟠 Major | ⚡ Quick winForce offline rendering when source-track edits are present.
This branch can still pick
filtergraph-fast-pathwhen the only reason we entered"edited-track"issourceAudioTrackSettingsor mutedclipRegions.classifyEditedTrackStrategy(...)never sees those edits, so native/FFmpeg export can silently drop per-track gain/normalize or clip mute state.Suggested fix
+ const requiresRenderedEditedTrack = + hasNonDefaultSourceTrackSettings(this.config.sourceAudioTrackSettings) || + (this.config.clipRegions ?? []).some((clip) => Boolean(clip.muted)); const canUsePrimaryAudioFiltergraph = Boolean(primaryAudioSourcePath) && !hasTimedSourceAudioFallback && (usesEmbeddedPrimaryAudio || sourceAudioFallbackPaths.includes(primaryAudioSourcePath ?? "")) && typeof primaryAudioSourceSampleRate === "number" && Number.isFinite(primaryAudioSourceSampleRate) && primaryAudioSourceSampleRate > 0; - const strategy = canUsePrimaryAudioFiltergraph + const strategy = canUsePrimaryAudioFiltergraph && !requiresRenderedEditedTrack ? classifyEditedTrackStrategy({ primaryAudioSourcePath, sourceDurationMs, trimRegions, speedRegions, audioRegions, sourceAudioFallbackPaths, }) : "offline-render-fallback";🤖 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/exporter/modernVideoExporter.ts` around lines 1195 - 1230, The current strategy selection can still pick "filtergraph-fast-path" even when the only edits are per-track source settings or muted clipRegions, which classifyEditedTrackStrategy doesn't see; update the logic around canUsePrimaryAudioFiltergraph/strategy so that if hasNonDefaultSourceTrackSettings(this.config.sourceAudioTrackSettings) or (this.config.clipRegions ?? []).some(c => Boolean(c.muted)) is true you force strategy = "offline-render-fallback" (skip calling classifyEditedTrackStrategy), otherwise preserve the existing behavior that uses classifyEditedTrackStrategy({ primaryAudioSourcePath, sourceDurationMs, trimRegions, speedRegions, audioRegions, sourceAudioFallbackPaths }) when canUsePrimaryAudioFiltergraph is true.
🧹 Nitpick comments (3)
src/components/video-editor/audio/useSourceAudioTrackSettings.ts (1)
116-128: ⚡ Quick winSkip no-op state writes in selected-track update handlers.
Both handlers always allocate a new state object, even when the incoming value equals current state. This can create avoidable rerenders during rapid slider/toggle interactions.
♻️ Proposed refactor
const onSelectedClipSourceAudioTrackVolumeChange = useCallback( (id: string, volume: number) => { if (!selectedClipId) return; setSourceAudioTrackSettingsByClip((prev) => { const prevClip = prev[selectedClipId] ?? defaultSourceAudioTrackSettings; + const nextVolume = Number.isFinite(volume) + ? Math.max(0, Math.min(2, volume)) + : (prevClip[id]?.volume ?? 1); + const prevNormalize = prevClip[id]?.normalize ?? false; + if (prevClip[id]?.volume === nextVolume && prevNormalize === (prevClip[id]?.normalize ?? false)) { + return prev; + } return { ...prev, [selectedClipId]: { ...prevClip, [id]: { - volume: Math.max(0, Math.min(2, volume)), - normalize: prevClip[id]?.normalize ?? false, + volume: nextVolume, + normalize: prevNormalize, }, }, }; }); }, [defaultSourceAudioTrackSettings, selectedClipId], ); const onSelectedClipSourceAudioTrackNormalizeChange = useCallback( (id: string, normalize: boolean) => { if (!selectedClipId) return; setSourceAudioTrackSettingsByClip((prev) => { const prevClip = prev[selectedClipId] ?? defaultSourceAudioTrackSettings; + const prevVolume = prevClip[id]?.volume ?? 1; + if (prevClip[id]?.normalize === normalize) { + return prev; + } return { ...prev, [selectedClipId]: { ...prevClip, [id]: { - volume: prevClip[id]?.volume ?? 1, + volume: prevVolume, normalize, }, }, }; }); }, [defaultSourceAudioTrackSettings, selectedClipId], );Also applies to: 136-148
🤖 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/video-editor/audio/useSourceAudioTrackSettings.ts` around lines 116 - 128, The selected-track update always creates a new state object even when nothing changes; update setSourceAudioTrackSettingsByClip to first read prevClip = prev[selectedClipId] ?? defaultSourceAudioTrackSettings and compare the new computed entry (for key id using bounded volume and normalize fallback) against prevClip[id]; if identical, return prev to skip the no-op write, otherwise return the updated object. Apply the same early-return check to the analogous handler around lines 136-148 so both the volume slider and normalize toggle avoid reallocating state when values are unchanged (referencing setSourceAudioTrackSettingsByClip, selectedClipId, defaultSourceAudioTrackSettings, id, volume).src/components/video-editor/audio/useVideoEditorAudio.ts (1)
1-1: 💤 Low valueUnused
Reactnamespace import.Same as
useClipAudioSettingsController.ts- onlyuseMemois used from the import.♻️ Suggested fix
-import React, { useMemo } from "react"; +import { useMemo } from "react";🤖 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/video-editor/audio/useVideoEditorAudio.ts` at line 1, The import in useVideoEditorAudio.ts currently brings in the entire React namespace but only useMemo is used; update the import to remove the unused React symbol and import useMemo directly (i.e., change the import that references React and useMemo to just import useMemo) so that the module no longer includes an unused React namespace import.src/components/video-editor/audio/useClipAudioSettingsController.ts (1)
1-1: 💤 Low valueUnused
Reactnamespace import.The
Reactnamespace is imported but onlyuseCallbackanduseMemoare used. The named imports are sufficient.♻️ Suggested fix
-import React, { useCallback, useMemo } from "react"; +import { useCallback, useMemo } from "react";🤖 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/video-editor/audio/useClipAudioSettingsController.ts` at line 1, The module imports the React namespace but only uses hooks; update the import so it only imports the named hooks (remove the unused default React import) — e.g. replace the current import line with one that imports useCallback and useMemo from "react" to eliminate the unused React symbol; check the hook useClipAudioSettingsController to confirm no other React namespace usages remain before committing.
🤖 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/components/video-editor/AnnotationSettingsPanel.tsx`:
- Line 143: The footer (the JSX block that renders the delete action) is
currently inside the scrolling container with class "flex-1 min-h-0 p-4
overflow-y-auto custom-scrollbar", so the delete action scrolls away; fix it by
moving that footer JSX out of that div and making it a sibling footer container
(e.g., a div after the scrollable area) so it stays fixed at the panel bottom,
ensuring the parent container uses a column flex layout (flex flex-col) and the
scrollable area remains "flex-1 overflow-y-auto"; add appropriate padding/border
(e.g., p-4 border-t) to the footer so styling matches the panel.
In `@src/components/video-editor/audio/clipAudio.ts`:
- Around line 1-13: The current getActiveClipIdAtSourceTime function incorrectly
maps source time to timeline time via mapSourceTimeToTimelineTime which can snap
out-of-range source timestamps into the first kept clip; instead check
source-span membership directly on each ClipRegion: for each clip in clipRegions
(use the ClipRegion shape), compare the raw sourceTimeSeconds (converted to ms)
against the clip's source-span fields (e.g., sourceStartMs and sourceEndMs or
whatever the ClipRegion uses for source start/end) and return clip.id only if
sourceMs is >= sourceStartMs and < sourceEndMs; remove the reliance on
mapSourceTimeToTimelineTime for this membership test so out-of-range source
times don’t erroneously mark a clip active.
In `@src/components/video-editor/audio/useSourceAudioTrackSettings.ts`:
- Around line 123-124: The volume clamp expression can still produce NaN when
volume is NaN; update where volume is saved (the volume property currently set
with Math.max(0, Math.min(2, volume))) to first check Number.isFinite(volume)
and fallback to a safe default (e.g., 1.0) before clamping, e.g. use
Number.isFinite(volume) ? Math.max(0, Math.min(2, volume)) : 1; keep the
normalize assignment (prevClip[id]?.normalize ?? false) as-is.
In `@src/components/video-editor/audio/waveform/WaveformGenerator.ts`:
- Around line 28-49: The current computePeaksWithWorker attaches a per-request
"error" listener to the shared Worker which will fire for any worker error and
reject all pending requests; instead have the worker return errors tied to
requestId via postMessage and handle them with a per-request "message" handler
that checks message.requestId and message.error, resolving or rejecting only the
matching promise. Update computePeaksWithWorker to stop adding per-request error
listeners (remove onError usage), add a one-time message listener that inspects
evt.data.requestId and evt.data.error/peaks to call the resolver from
this.workerResolvers.get(requestId) or reject with the provided error, and
ensure you clean up the message listener and this.workerResolvers entry after
resolving/rejecting; keep using this.worker.postMessage({ requestId,
channelData, samples }, [channelData.buffer]) to send the job.
In `@src/components/video-editor/projectPersistence.ts`:
- Around line 994-1003: The persisted fields sourceAudioTrackSettingsByClip and
defaultSourceAudioTrackSettings are being passed through raw (allowing
arrays/malformed objects) — instead, normalize and sanitize their entries before
persisting: ensure the outer value is a plain object (not an array), iterate its
keys and coerce each entry into the expected shape (e.g., { volume: number,
normalize: boolean, ... }), clamp numeric fields (e.g., volume) to valid ranges,
default missing properties, and discard/ignore invalid nested entries; implement
this normalization in projectPersistence.ts where these fields are set (or call
a new helper like normalizeSourceAudioTrackSettings) so loader/clamping logic is
preserved and only valid settings reach runtime.
In `@src/components/video-editor/SettingsPanel.tsx`:
- Around line 3695-3729: The footer renders an extra destructive delete button
for zooms causing a duplicate CTA when activeEffectSection === "zoom" and
selectedZoomId is set; remove one of the duplicates by either deleting the
footer branch that renders the zoom delete (the conditional block checking
activeEffectSection === "zoom" && selectedZoomId which calls onZoomDelete) or by
removing the inline zoom delete inside the zoom editor so only a single delete
remains; keep the handler name onZoomDelete and the selectedZoomId check intact
and ensure only one place invokes onZoomDelete(selectedZoomId).
In `@src/components/video-editor/timeline/components/viewport/TimelineCanvas.tsx`:
- Around line 392-405: The waveformSegmentSpan currently uses the clip's
timeline span which misaligns with track.peaks (source-time); change the prop
passed to Item so waveformSegmentSpan uses the clip's source-time span instead:
replace waveformSegmentSpan={liveSpanPreviewById?.[item.id] ?? item.span} with
waveformSegmentSpan={liveSpanPreviewById?.[item.id] ?? item.sourceSpan ??
item.span} (and ensure the Item/clip object exposes sourceSpan or equivalent),
so the source-audio row (track.peaks) and waveformSegmentSpan are in the same
timebase.
In `@src/components/video-editor/timeline/components/waveform/AudioWaveform.tsx`:
- Around line 52-55: The draw() throttling currently returns early if called
within 33ms and never re-queues a follow-up RAF, causing the waveform to freeze;
update draw() to, when it returns early, schedule a single requestAnimationFrame
if one isn't already pending (use a pendingRafRef or similar flag) so a redraw
will occur after the throttle window, and clear the flag when that RAF runs;
apply the same pattern to the second throttled draw block referenced around the
other draw usage (the same pendingRafRef and enqueue-if-not-pending logic).
In `@src/components/video-editor/timeline/components/wrapper/TimelineWrapper.tsx`:
- Around line 149-152: The live-preview movement test only checks horizontal
motion (Math.abs(event.delta?.x ?? 0) > 0.01), so vertical drags are ignored;
update the moved calculation in TimelineWrapper (where moved is computed and
onLiveSpanPreviewChange is invoked) to consider both axes—for example compute
movement magnitude using both delta.x and delta.y (Math.hypot(event.delta?.x ??
0, event.delta?.y ?? 0) > 0.01) or check Math.abs for either axis—then keep the
existing onLiveSpanPreviewChange call unchanged so vertical drags also trigger
the preview.
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 4419-4424: The export callback handleExport is closing over
clipRegions and audio.activeSourceAudioTrackSettings (and related source-audio
fields) but their values are not included in its dependency array, causing stale
exports; update the useCallback (or whatever creates handleExport) to include
clipRegions, audio.activeSourceAudioTrackSettings,
audio.sourceAudioFallbackPaths, and audio.sourceAudioFallbackStartDelayMsByPath
so the callback is recreated when those settings change, and make the same
dependency additions for the other export-related callback/functionality later
in the file that similarly references these audio/clip symbols.
In `@src/i18n/locales/fr/settings.json`:
- Around line 193-197: Update the French translations for the audio settings
keys to use natural grammar and clearer action wording: change
"sourceTracksTitle" value to "Source audio du clip", adjust "systemLabel" to use
sentence-style capitalization "Son système", keep "micLabel" as "Microphone" (no
change), ensure "mixedLabel" reads "Source" if intended, and change
"deleteRegion" to "Supprimer la zone audio" to reflect region deletion
semantics; locate and edit these keys (sourceTracksTitle, systemLabel, micLabel,
mixedLabel, deleteRegion) in the fr settings JSON and replace their string
values accordingly.
---
Outside diff comments:
In `@electron/ipc/recording/windows.ts`:
- Around line 281-331: The returned keptAudioSidecars value is inconsistent: use
the policy flag instead of a hardcoded true. Replace any occurrences of a
literal true in returned objects with the local variable keepAudioSidecars (from
shouldKeepRecordingAudioSidecars()), e.g. update the return in the early-exit
after audioInputs.length === 0 and the later return that currently sets
keptAudioSidecars: true to keptAudioSidecars: keepAudioSidecars so the returned
state matches actual deletion behavior controlled by
shouldKeepRecordingAudioSidecars().
In `@src/lib/exporter/modernVideoExporter.ts`:
- Around line 757-770: The browser audio-processing path is missing clipRegions
when calling AudioProcessor.process, causing muted clips to not be honored by
buildNativeAudioPlan; update the call inside measureFinalizationStage /
awaitWithFinalizationTimeout to pass this.config.clipRegions as the clipRegions
argument to AudioProcessor.process (keeping the same argument order as the
method signature), so the browser/WebCodecs path receives clipRegions just like
the native/offline paths and muted clips are correctly silenced.
- Around line 1195-1230: The current strategy selection can still pick
"filtergraph-fast-path" even when the only edits are per-track source settings
or muted clipRegions, which classifyEditedTrackStrategy doesn't see; update the
logic around canUsePrimaryAudioFiltergraph/strategy so that if
hasNonDefaultSourceTrackSettings(this.config.sourceAudioTrackSettings) or
(this.config.clipRegions ?? []).some(c => Boolean(c.muted)) is true you force
strategy = "offline-render-fallback" (skip calling classifyEditedTrackStrategy),
otherwise preserve the existing behavior that uses classifyEditedTrackStrategy({
primaryAudioSourcePath, sourceDurationMs, trimRegions, speedRegions,
audioRegions, sourceAudioFallbackPaths }) when canUsePrimaryAudioFiltergraph is
true.
---
Nitpick comments:
In `@src/components/video-editor/audio/useClipAudioSettingsController.ts`:
- Line 1: The module imports the React namespace but only uses hooks; update the
import so it only imports the named hooks (remove the unused default React
import) — e.g. replace the current import line with one that imports useCallback
and useMemo from "react" to eliminate the unused React symbol; check the hook
useClipAudioSettingsController to confirm no other React namespace usages remain
before committing.
In `@src/components/video-editor/audio/useSourceAudioTrackSettings.ts`:
- Around line 116-128: The selected-track update always creates a new state
object even when nothing changes; update setSourceAudioTrackSettingsByClip to
first read prevClip = prev[selectedClipId] ?? defaultSourceAudioTrackSettings
and compare the new computed entry (for key id using bounded volume and
normalize fallback) against prevClip[id]; if identical, return prev to skip the
no-op write, otherwise return the updated object. Apply the same early-return
check to the analogous handler around lines 136-148 so both the volume slider
and normalize toggle avoid reallocating state when values are unchanged
(referencing setSourceAudioTrackSettingsByClip, selectedClipId,
defaultSourceAudioTrackSettings, id, volume).
In `@src/components/video-editor/audio/useVideoEditorAudio.ts`:
- Line 1: The import in useVideoEditorAudio.ts currently brings in the entire
React namespace but only useMemo is used; update the import to remove the unused
React symbol and import useMemo directly (i.e., change the import that
references React and useMemo to just import useMemo) so that the module no
longer includes an unused React namespace import.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5bfb0c0c-23c6-481d-8799-06906ab76aa8
📒 Files selected for processing (44)
electron/ipc/project/manager.test.tselectron/ipc/project/manager.tselectron/ipc/recording/audioFilters.tselectron/ipc/recording/diagnostics.tselectron/ipc/recording/mac.tselectron/ipc/recording/windows.tssrc/components/video-editor/AnnotationSettingsPanel.tsxsrc/components/video-editor/SettingsPanel.tsxsrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/audio/audioTypes.tssrc/components/video-editor/audio/clipAudio.tssrc/components/video-editor/audio/useAudioPreviewSync.tssrc/components/video-editor/audio/useClipAudioSettingsController.tssrc/components/video-editor/audio/useSourceAudioFallback.tssrc/components/video-editor/audio/useSourceAudioTrackSettings.tssrc/components/video-editor/audio/useVideoEditorAudio.tssrc/components/video-editor/audio/waveform/WaveformGenerator.tssrc/components/video-editor/audio/waveform/waveform.worker.tssrc/components/video-editor/projectPersistence.tssrc/components/video-editor/timeline/Item.tsxsrc/components/video-editor/timeline/TimelineEditor.tsxsrc/components/video-editor/timeline/components/viewport/TimelineCanvas.tsxsrc/components/video-editor/timeline/components/waveform/AudioWaveform.tsxsrc/components/video-editor/timeline/components/wrapper/TimelineWrapper.tsxsrc/components/video-editor/timeline/core/constants.tssrc/components/video-editor/timeline/core/timelineTypes.tssrc/components/video-editor/timeline/hooks/useTimelineAudioPeaks.tssrc/components/video-editor/timeline/model/timelineModel.tssrc/components/video-editor/types.tssrc/i18n/locales/en/settings.jsonsrc/i18n/locales/es/settings.jsonsrc/i18n/locales/fr/settings.jsonsrc/i18n/locales/ko/settings.jsonsrc/i18n/locales/nl/settings.jsonsrc/i18n/locales/pt-BR/settings.jsonsrc/i18n/locales/ru/settings.jsonsrc/i18n/locales/zh-CN/settings.jsonsrc/i18n/locales/zh-TW/settings.jsonsrc/lib/exporter/audioEncoder.tssrc/lib/exporter/audioRoutingEngine.tssrc/lib/exporter/modernVideoExporter.tssrc/lib/exporter/sourceTrackRoutingPolicy.test.tssrc/lib/exporter/sourceTrackRoutingPolicy.tssrc/lib/exporter/videoExporter.ts
💤 Files with no reviewable changes (1)
- electron/ipc/recording/mac.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/components/video-editor/audio/waveform/waveform.worker.ts`:
- Around line 16-21: The current bucketization uses a fixed step computed once
which can leave tail samples unscanned and distort peaks; update the loop in
waveform.worker.ts to compute bucket boundaries proportionally per index using
the buffer length and samples (e.g., start = Math.floor(i * firstChannel.length
/ samples) and end = Math.floor((i + 1) * firstChannel.length / samples) so the
last bucket covers the tail), remove reliance on the single step variable, and
then compute the peak/min/max over firstChannel[start..end] to populate
result[i].
- Line 10: The code uses (self as any).postMessage(...) which violates
noExplicitAny; replace the any casts with a typed worker scope (e.g. const
workerScope = self as unknown as DedicatedWorkerGlobalScope) and call
workerScope.postMessage({ requestId, peaks: empty }, [empty.buffer]); do the
same for the other occurrences (the other postMessage calls referencing
requestId and empty around lines 32–34), ensuring you import/declare the correct
DOM lib types or add a local type alias if needed so TypeScript sees
DedicatedWorkerGlobalScope without using any.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0fc17541-a856-4bf4-8e2f-5eb300259cf7
📒 Files selected for processing (6)
src/components/video-editor/AnnotationSettingsPanel.tsxsrc/components/video-editor/audio/clipAudio.tssrc/components/video-editor/audio/useSourceAudioTrackSettings.tssrc/components/video-editor/audio/waveform/WaveformGenerator.tssrc/components/video-editor/audio/waveform/waveform.worker.tssrc/lib/exporter/modernVideoExporter.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/components/video-editor/audio/clipAudio.ts
- src/components/video-editor/audio/waveform/WaveformGenerator.ts
- src/components/video-editor/audio/useSourceAudioTrackSettings.ts
- src/components/video-editor/AnnotationSettingsPanel.tsx
1 task
2 tasks
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
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.
Description
This PR adresses the issue of the audio being at the wrong layer.
While doing that, we also discovered issue with audio playback, audio stuttering, audio exports....
In order to guarantee better code quality, the audio engine is now seperated from the video-editor to avoid bloating even more the video-editor, which is needed for the future refactor
All logic for the audio is inside video-editor/audio
Changes include
Type of Change
Related Issue(s)
#476 Audio on the wrong layer
Video (wherever possible):
Testing Guide
Record a video with only microphone, export with different volumes.. mute parameters...
Record video with Mic + system audio
Record a video with nothing
verify everything is okay both in playback and in export.
Checklist
Thank you for contributing!
Summary by CodeRabbit
New Features
UI/UX Improvements
Bug Fixes
Tests
Localization