fix(export): recover video background fallback and smooth preview audio#452
Merged
Conversation
Contributor
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughRefines VideoEditor preview audio-sync parameters and smoke-export backend parsing; implements decoder→media-element fallback with readiness timeout in legacy and modern frame renderers, plus tests that assert decoder teardown and media-element fallback initialization. ChangesVideo Editor Audio Sync & Export Config
Frame Renderer Wallpaper Export Fallback
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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 docstrings
🧪 Generate unit tests (beta)
Comment |
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![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: 3
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/VideoEditor.tsx (1)
4624-4639:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSmoke backend pinning is bypassed when
smokeUseNativeExport=1.In the modern path,
useExperimentalNativeExportflips this ternary to"auto"before the smoke-specificbackendPreferenceoverride/defaults are consulted. That meanssmokeBackendPreferenceis ignored, and the"breeze"fallback here is effectively unreachable, so smoke runs cannot reliably force Breeze vs WebCodecs coverage.Suggested fix
const backendPreference = pipelineModel === "legacy" ? "webcodecs" - : useExperimentalNativeExport - ? "auto" - : smokeExportConfig.enabled - ? (smokeExportConfig.backendPreference ?? - (smokeExportConfig.useNativeExport - ? "breeze" - : "webcodecs")) - : (settings.backendPreference ?? exportBackendPreference); + : smokeExportConfig.enabled + ? (smokeExportConfig.backendPreference ?? + (smokeExportConfig.useNativeExport + ? "breeze" + : "webcodecs")) + : useExperimentalNativeExport + ? "auto" + : (settings.backendPreference ?? exportBackendPreference);🤖 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/VideoEditor.tsx` around lines 4624 - 4639, The ternary logic for backendPreference lets useExperimentalNativeExport force "auto" before honoring smokeExportConfig.backendPreference, so smoke runs can't pin Breeze/WebCodecs. Fix by treating smokeExportConfig.enabled as the highest-priority branch for modern pipeline: when pipelineModel === "modern" and smokeExportConfig.enabled, compute backendPreference from smokeExportConfig.backendPreference ?? (smokeExportConfig.useNativeExport ? "breeze" : "webcodecs"); otherwise (smoke disabled) fall back to the existing useExperimentalNativeExport ? "auto" : (settings.backendPreference ?? exportBackendPreference). Update the backendPreference expression (referencing pipelineModel, useExperimentalNativeExport, and smokeExportConfig) accordingly.
🤖 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/VideoEditor.tsx`:
- Around line 2143-2150: The else (plain-video) branch currently calls
setCurrentVideoPath then still reads getCurrentRecordingSession and applies it,
which can leak previous webcam session UI state; modify the else branch handling
around setCurrentVideoPath so that instead of calling
getCurrentRecordingSession/applySessionPresentation afterwards you explicitly
call applySessionPresentation(null) (i.e., reset session presentation) and only
call getCurrentRecordingSession()/applySessionPresentation(session) when
handling the webcam-backed branch; update code paths around setCurrentVideoPath,
getCurrentRecordingSession, and applySessionPresentation accordingly.
In `@src/lib/exporter/frameRenderer.ts`:
- Around line 906-953: The ready Promise created after calling
resolveMediaElementSource(videoSrc) can hang if loadeddata/canplay/error never
fire; modify the logic in frameRenderer.ts (around resolveMediaElementSource,
cleanupBackgroundSource, and the ready Promise that creates the video element)
to add a bounded timeout (e.g., configurable ms) that rejects/resolves false
after the timeout, ensure the timeout is cleared on success/error, always call
backgroundSource.revoke when the function returns false or on timeout, and
guarantee event listeners on the video element are removed in the timeout
handler and in the existing cleanup function so there is no leaked media source
or dangling listeners. Ensure the same teardown runs for the existing error path
and the new timeout path.
In `@src/lib/exporter/modernFrameRenderer.ts`:
- Around line 2107-2159: Guard the background video readiness with a timeout and
ensure the resolved source/video are revoked on all failure paths like the
webcam fallback: when awaiting resolveMediaElementSource(videoSrc) keep
this.cleanupBackgroundSource assigned, then in the Promise that waits for
"loadeddata"/"canplay"/"error" add a timeout (e.g., setTimeout ->
resolve(false)) and include that timer in the cleanup; on any failure branch
(error handler or timeout) call the revoke function
(this.cleanupBackgroundSource or backgroundSource.revoke) and stop/clear the
video (remove src, call load if needed) before resolving false; ensure the same
cleanup is performed if ready is false so no resolved source/video is left
alive; finally proceed to set this.backgroundVideoElement and call
ensureBackgroundSprite only when the Promise resolves true.
---
Outside diff comments:
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 4624-4639: The ternary logic for backendPreference lets
useExperimentalNativeExport force "auto" before honoring
smokeExportConfig.backendPreference, so smoke runs can't pin Breeze/WebCodecs.
Fix by treating smokeExportConfig.enabled as the highest-priority branch for
modern pipeline: when pipelineModel === "modern" and smokeExportConfig.enabled,
compute backendPreference from smokeExportConfig.backendPreference ??
(smokeExportConfig.useNativeExport ? "breeze" : "webcodecs"); otherwise (smoke
disabled) fall back to the existing useExperimentalNativeExport ? "auto" :
(settings.backendPreference ?? exportBackendPreference). Update the
backendPreference expression (referencing pipelineModel,
useExperimentalNativeExport, and smokeExportConfig) accordingly.
🪄 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: f44f6d4f-20e2-40dc-b9ed-eacad927af91
📒 Files selected for processing (5)
src/components/video-editor/VideoEditor.tsxsrc/lib/exporter/frameRenderer.test.tssrc/lib/exporter/frameRenderer.tssrc/lib/exporter/modernFrameRenderer.test.tssrc/lib/exporter/modernFrameRenderer.ts
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
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.
Summary
Validation
npm test -- src/lib/mediaTiming.test.ts src/lib/exporter/sourceAudioFallback.test.ts src/lib/exporter/modernFrameRenderer.test.ts src/lib/exporter/frameRenderer.test.tsnpx tsc --noEmitnpx biome check src/components/video-editor/VideoEditor.tsx src/lib/exporter/frameRenderer.ts src/lib/exporter/frameRenderer.test.ts src/lib/exporter/modernFrameRenderer.ts src/lib/exporter/modernFrameRenderer.test.tsgit diff --checkSummary by CodeRabbit
Bug Fixes
Tests