fix(editor): preserve Windows microphone audio during preview#677
Open
sin6626 wants to merge 3 commits into
Open
fix(editor): preserve Windows microphone audio during preview#677sin6626 wants to merge 3 commits into
sin6626 wants to merge 3 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughBackend ffprobe probing now returns per-path mediaInfoByPath via IPC; client stores that map, decodes WAVs for peaks/preview when appropriate, uses probed durations in preview timing and sync (WebAudio or media-element), and renders duration-aware waveforms and richer source-audio tracks. ChangesCompanion Audio Probing & Preview Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 5
🧹 Nitpick comments (2)
src/components/video-editor/timeline/components/waveform/AudioWaveform.tsx (1)
79-82: 💤 Low valueOptional: Simplify redundant safety check.
Lines 80-81 check
(audioDurationMs ?? 0) > 0then useMath.max(0, audioDurationMs ?? 0). Since the condition already ensures the value is positive, theMath.max(0, ...)wrapper is redundant.♻️ Simplification
const effectiveDurationMs = - Number.isFinite(audioDurationMs) && (audioDurationMs ?? 0) > 0 - ? Math.max(0, audioDurationMs ?? 0) + Number.isFinite(audioDurationMs) && audioDurationMs! > 0 + ? audioDurationMs! : peaksDurationMs;🤖 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/timeline/components/waveform/AudioWaveform.tsx` around lines 79 - 82, The effectiveDurationMs computation in AudioWaveform redundantly wraps audioDurationMs with Math.max(0, ...) after already checking it's > 0; simplify by returning the audioDurationMs (with the existing nullish fallback) when Number.isFinite(audioDurationMs) && (audioDurationMs ?? 0) > 0, otherwise fall back to peaksDurationMs. Update the effectiveDurationMs assignment to use audioDurationMs ?? 0 directly in the true branch instead of Math.max.src/components/video-editor/timeline/sourceAudioTracks.ts (1)
48-55: 💤 Low valueOptional: Simplify redundant null check.
Line 53's
!Number.isFinite(probedDurationMs) || probedDurationMs === nullcan be simplified to just!Number.isFinite(probedDurationMs)sinceNumber.isFinite(null)already returnsfalse.♻️ Simplification
- if (!Number.isFinite(probedDurationMs) || probedDurationMs === null) return "full"; + if (!Number.isFinite(probedDurationMs)) return "full";🤖 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/timeline/sourceAudioTracks.ts` around lines 48 - 55, The conditional in getWaveformCoverage redundantly checks probedDurationMs === null; replace the combined check `!Number.isFinite(probedDurationMs) || probedDurationMs === null` with just `!Number.isFinite(probedDurationMs)` so the function still returns "full" when probedDurationMs is non-finite (including null), keeping the rest of the logic (peaks null -> "none", duration comparison -> "partial"/"full") unchanged.
🤖 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/useAudioPreviewSync.test.ts`:
- Around line 150-182: The tests import and type-cast internal helpers
(isAudioResourceLoadCurrent, getSourceAudioPreviewVolume) from
useAudioPreviewSync, coupling tests to private implementation; fix by making the
contract explicit: either export these helpers from the module (export function
isAudioResourceLoadCurrent / getSourceAudioPreviewVolume) so tests can import
them directly, or move them into a new utility module (e.g.,
audioResourceUtils.ts) with proper exports and import those in both the
implementation and tests, or alternatively remove direct assertions and verify
the same behaviors via the public API of useAudioPreviewSync; choose one
approach and update imports/exports and test imports accordingly.
- Line 21: The test is asserting an internal key format by expecting "122100";
replace that with a behavioral assertion: remove the literal "122100" check on
the withProbe variable and instead assert behavior using the probe identifier or
outcome produced by the function under test (e.g., assert withProbe contains
probe.id or that the returned key resolves to the expected resource/data or is
non-empty). Locate the test file and update the expectation on the withProbe
variable (from useAudioPreviewSync.test.ts) so it asserts observable behavior
(contains probe.id, resolves to stored data, or similar) rather than the
internal formatted substring "122100".
In `@src/components/video-editor/audio/useAudioPreviewSync.ts`:
- Around line 890-901: The useEffect that calls ensureSourceAudioRunning() then
unconditionally .play()s every element (in useAudioPreviewSync.ts: the effect
watching isPlaying, resolvedSourceTracks.length, ensureSourceAudioRunning) can
restart companion tracks that the main sync logic intentionally paused via
beforeAudioStart/atEnd; modify this effect to honor the same gating: before
calling audio.play(), check the same conditions used by the main sync (e.g.,
beforeAudioStart and atEnd flags or whatever predicate the sync uses) for each
track, or alternatively remove the automatic play here and let the main sync
effect (the primary play/pause logic in useAudioPreviewSync) own all play/pause
transitions; ensure you reference sourceAudioElementsRef.current and only call
.play() on elements allowed by the gating to avoid starting delayed sidecars
early.
In `@src/components/video-editor/audio/waveform/wavDecoder.ts`:
- Around line 75-87: The validation currently allows any bitsPerSample for float
WAVs (audioFormat === 3) so 64-bit floats slip through and decode to silence;
update the checks in the WAV decoding validation (the block that computes
audioFormat, channelCount, sampleRate, bitsPerSample, dataSize and
bytesPerSample) to explicitly reject unsupported float widths by requiring
bitsPerSample === 32 when audioFormat === 3 (and still reject non-integer or <=0
bytesPerSample for PCM paths); make the same explicit check in the secondary
validation near the decodePcmSample/decodeFloatSample call sites (the region
referenced around lines 107-109) so 64-bit float WAVs return null instead of
being passed into decodePcmSample.
In `@src/lib/mediaTiming.test.ts`:
- Around line 96-108: The test call to resolveCompanionAudioPreviewTiming
unnecessarily casts the input object with "as never"; remove that cast so the
literal matches the function's declared param type (which already allows
probedAudioDurationSeconds?: number | null). Update the test in
mediaTiming.test.ts by deleting "as never" from the object passed to
resolveCompanionAudioPreviewTiming (the call with currentTimeSeconds,
timelineDurationSeconds, audioDurationSeconds, recordedStartDelayMs,
probedAudioDurationSeconds) so the test uses the correctly typed literal.
---
Nitpick comments:
In `@src/components/video-editor/timeline/components/waveform/AudioWaveform.tsx`:
- Around line 79-82: The effectiveDurationMs computation in AudioWaveform
redundantly wraps audioDurationMs with Math.max(0, ...) after already checking
it's > 0; simplify by returning the audioDurationMs (with the existing nullish
fallback) when Number.isFinite(audioDurationMs) && (audioDurationMs ?? 0) > 0,
otherwise fall back to peaksDurationMs. Update the effectiveDurationMs
assignment to use audioDurationMs ?? 0 directly in the true branch instead of
Math.max.
In `@src/components/video-editor/timeline/sourceAudioTracks.ts`:
- Around line 48-55: The conditional in getWaveformCoverage redundantly checks
probedDurationMs === null; replace the combined check
`!Number.isFinite(probedDurationMs) || probedDurationMs === null` with just
`!Number.isFinite(probedDurationMs)` so the function still returns "full" when
probedDurationMs is non-finite (including null), keeping the rest of the logic
(peaks null -> "none", duration comparison -> "partial"/"full") unchanged.
🪄 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: 78ec3b2e-3b4d-4bae-9a5a-74b59ae4c934
📒 Files selected for processing (24)
electron/electron-env.d.tselectron/ipc/recording/diagnostics.test.tselectron/ipc/recording/diagnostics.tselectron/ipc/register/recording.test.tselectron/ipc/register/recording.tssrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/audio/audioTypes.tssrc/components/video-editor/audio/useAudioPreviewSync.test.tssrc/components/video-editor/audio/useAudioPreviewSync.tssrc/components/video-editor/audio/useSourceAudioFallback.tssrc/components/video-editor/audio/useVideoEditorAudio.tssrc/components/video-editor/audio/waveform/WaveformGenerator.tssrc/components/video-editor/audio/waveform/wavDecoder.test.tssrc/components/video-editor/audio/waveform/wavDecoder.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/hooks/useTimelineAudioPeaks.test.tssrc/components/video-editor/timeline/hooks/useTimelineAudioPeaks.tssrc/components/video-editor/timeline/sourceAudioTracks.test.tssrc/components/video-editor/timeline/sourceAudioTracks.tssrc/lib/mediaTiming.test.tssrc/lib/mediaTiming.ts
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/video-editor/audio/useAudioPreviewSync.ts (1)
490-497:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFallback when
decodeWavAudioData()returnsnullfor local WAV sidecars
decodeWavAudioData()returnsnullfor unsupported WAV subformats, andsrc/components/video-editor/audio/waveform/WaveformGenerator.ts:64-103already falls back toaudioContext.decodeAudioData(arrayBuffer)in that case. Insrc/components/video-editor/audio/useAudioPreviewSync.ts:490-516, the samenullis treated as a hard error (throw), so preview audio is lost even though the browser decode path could handle it.Suggested fix
const file = await createReadableMediaResourceFile(audioPath); const arrayBuffer = await file.arrayBuffer(); const decoded = decodeWavAudioData(arrayBuffer); - if (!decoded) { - throw new Error("Failed to decode companion WAV audio"); - } if ( !isAudioResourceLoadCurrent( decodedSourceAudioResourcesRef.current, @@ ) { return; } const context = ensureSourceAudioContext(); - const buffer = createAudioBufferFromDecodedWav(context, decoded); + const buffer = decoded + ? createAudioBufferFromDecodedWav(context, decoded) + : await context.decodeAudioData(arrayBuffer); decodedSourceAudioBuffersRef.current.set(audioPath, { resourceKey, buffer, });🤖 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/useAudioPreviewSync.ts` around lines 490 - 497, The code in the async block treats decodeWavAudioData(arrayBuffer) returning null as a hard error and throws, which prevents using the browser decoder fallback used elsewhere; instead, when decodeWavAudioData(...) returns null, call the AudioContext.decodeAudioData(arrayBuffer) fallback (same approach as in WaveformGenerator.ts) and use its result as decoded; only throw if both decodeWavAudioData and audioContext.decodeAudioData fail. Update the block around createReadableMediaResourceFile, decodeWavAudioData, and the async decoding flow to attempt the fallback via audioContext.decodeAudioData(arrayBuffer) and handle the returned AudioBuffer before proceeding.
♻️ Duplicate comments (1)
src/components/video-editor/audio/useAudioPreviewSync.ts (1)
873-876:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid replaying a stale element after the async resume promise settles.
This still queues
audio.play()behindensureSourceAudioRunning(). If the user pauses or seeks out of range before that promise resolves, the older callback can restart a companion element that a newer sync pass already paused. The direct-element path is not routed through Web Audio, so it should not wait on theAudioContextat all here.Suggested fix
if (shouldPlaySourceAudioElement({ isPlaying, beforeAudioStart, atEnd })) { - void ensureSourceAudioRunning().then(() => { - audio.play().catch(() => undefined); - }); + audio.play().catch(() => undefined); } else if (!audio.paused) { audio.pause(); }🤖 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/useAudioPreviewSync.ts` around lines 873 - 876, The current code awaits ensureSourceAudioRunning() then calls audio.play(), which can replay a stale element after the async promise resolves; instead, for the direct-element path (when shouldPlaySourceAudioElement returns true) call audio.play() synchronously without awaiting ensureSourceAudioRunning(), or if you must keep the async call, capture a guard (e.g., a local syncId/compare flag) before awaiting and re-check it after the promise to avoid restarting a newer/paused element; update the block around shouldPlaySourceAudioElement/ensureSourceAudioRunning/audio.play() to either call audio.play() immediately or add a post-await validity check so only the currently intended element is played.
🤖 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.
Outside diff comments:
In `@src/components/video-editor/audio/useAudioPreviewSync.ts`:
- Around line 490-497: The code in the async block treats
decodeWavAudioData(arrayBuffer) returning null as a hard error and throws, which
prevents using the browser decoder fallback used elsewhere; instead, when
decodeWavAudioData(...) returns null, call the
AudioContext.decodeAudioData(arrayBuffer) fallback (same approach as in
WaveformGenerator.ts) and use its result as decoded; only throw if both
decodeWavAudioData and audioContext.decodeAudioData fail. Update the block
around createReadableMediaResourceFile, decodeWavAudioData, and the async
decoding flow to attempt the fallback via
audioContext.decodeAudioData(arrayBuffer) and handle the returned AudioBuffer
before proceeding.
---
Duplicate comments:
In `@src/components/video-editor/audio/useAudioPreviewSync.ts`:
- Around line 873-876: The current code awaits ensureSourceAudioRunning() then
calls audio.play(), which can replay a stale element after the async promise
resolves; instead, for the direct-element path (when
shouldPlaySourceAudioElement returns true) call audio.play() synchronously
without awaiting ensureSourceAudioRunning(), or if you must keep the async call,
capture a guard (e.g., a local syncId/compare flag) before awaiting and re-check
it after the promise to avoid restarting a newer/paused element; update the
block around shouldPlaySourceAudioElement/ensureSourceAudioRunning/audio.play()
to either call audio.play() immediately or add a post-await validity check so
only the currently intended element is played.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5b6afe95-4b3a-4946-a52b-6c7725d5cee7
📒 Files selected for processing (7)
src/components/video-editor/audio/useAudioPreviewSync.test.tssrc/components/video-editor/audio/useAudioPreviewSync.tssrc/components/video-editor/audio/waveform/wavDecoder.test.tssrc/components/video-editor/audio/waveform/wavDecoder.tssrc/components/video-editor/timeline/components/waveform/AudioWaveform.tsxsrc/components/video-editor/timeline/sourceAudioTracks.tssrc/lib/mediaTiming.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/lib/mediaTiming.test.ts
- src/components/video-editor/timeline/components/waveform/AudioWaveform.tsx
- src/components/video-editor/timeline/sourceAudioTracks.ts
- src/components/video-editor/audio/waveform/wavDecoder.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fix Windows editor preview cases where a companion microphone track has an incomplete waveform or becomes silent after the first part of a recording even though the recorded WAV and exported video still contain the audio.
The editor now:
Motivation
On Windows 11, Chromium can expose only part of a long companion WAV through the media-element path. Playback then stops around a fixed timestamp while export remains correct. The timeline can show a full-length microphone track whose waveform only covers the beginning, even though the sidecar contains the complete recording. Playback-state rerenders could also invalidate an in-flight sidecar load, leaving the editor permanently silent.
Type of Change
Related Issue(s)
Fixes #602
Related to #629, #636, and #667.
This is separate from #536: that PR addresses audio loss during recording/mixing, while this PR addresses editor preview and waveform handling when the recorded companion WAV already exists.
This PR is also submitted for consideration for the $50 bounty offered in #629. It fixes the editor-preview variant represented most precisely by duplicate issue #636, and has now been tested with a real affected Windows 11 recording.
Screenshots / Video
Not applicable; this changes audio playback and waveform completeness rather than adding a new UI surface.
Testing Guide
Automated validation:
git diff --checkpassedTypeScript currently reports the same pre-existing upstream error on the untouched
zoomRegionUtils.ts:getConnectedRegionTransitionis declared but never read.Windows 11 manual validation:
npm run devChecklist
Summary by CodeRabbit
New Features
Tests