Stabilize video export on Windows#210
Conversation
|
✅ Actions performedReview triggered.
|
📝 Walkthrough🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip CodeRabbit can use oxc to improve the quality of JavaScript and TypeScript code reviews.Add a configuration file to your project to customize how CodeRabbit runs oxc. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/exporter/videoExporter.ts (1)
413-427: Consider extractingwithTimeoutto a shared utility.This implementation is identical to the one in
streamingDecoder.ts. Extracting to a shared utility (e.g.,src/lib/utils/withTimeout.ts) would reduce duplication and ensure consistent behavior.♻️ Proposed shared utility
Create a new file
src/lib/utils/withTimeout.ts:export function withTimeout<T>( promise: Promise<T>, timeoutMs: number, message: string, ): Promise<T> { return new Promise<T>((resolve, reject) => { const timer = window.setTimeout(() => reject(new Error(message)), timeoutMs); promise.then( (value) => { window.clearTimeout(timer); resolve(value); }, (error) => { window.clearTimeout(timer); reject(error); }, ); }); }Then import and use in both files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/videoExporter.ts` around lines 413 - 427, Extract the duplicate withTimeout<T> implementation into a single shared utility function named withTimeout and export it from a new module, then replace the local implementations in videoExporter.ts (method withTimeout) and streamingDecoder.ts with imports of that shared withTimeout to remove duplication and ensure consistent behavior; keep the same signature (promise: Promise<T>, timeoutMs: number, message: string) and the same timer/clear logic so callers (e.g., the withTimeout call sites in VideoExporter and StreamingDecoder) continue to work unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/lib/exporter/videoExporter.ts`:
- Around line 413-427: Extract the duplicate withTimeout<T> implementation into
a single shared utility function named withTimeout and export it from a new
module, then replace the local implementations in videoExporter.ts (method
withTimeout) and streamingDecoder.ts with imports of that shared withTimeout to
remove duplication and ensure consistent behavior; keep the same signature
(promise: Promise<T>, timeoutMs: number, message: string) and the same
timer/clear logic so callers (e.g., the withTimeout call sites in VideoExporter
and StreamingDecoder) continue to work unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 851ef0a9-eb04-4211-a61a-f48f3295f45e
📒 Files selected for processing (2)
src/lib/exporter/streamingDecoder.tssrc/lib/exporter/videoExporter.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3726ed73c7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| import type { ExportConfig, ExportProgress, ExportResult } from "./types"; | ||
|
|
||
| const ENCODER_STALL_TIMEOUT_MS = 15_000; | ||
| const ENCODER_FLUSH_TIMEOUT_MS = 20_000; |
There was a problem hiding this comment.
Scale encoder flush timeout with pending frame backlog
A fixed ENCODER_FLUSH_TIMEOUT_MS = 20_000 can make healthy exports fail on slower machines because VideoEncoder.flush() must wait for all queued frames to finish encoding, and this pipeline can still have a sizable backlog at finalize time. With up to 32 queued frames in software mode (and 120 in hardware mode), valid exports can legitimately take longer than 20s to drain, so this introduces false "encoder stopped responding" failures and unnecessary fallback/final failure paths.
Useful? React with 👍 / 👎.
|
Looks reasonable to me. Have you tested it thoroughly with exporting? |
Yes, I have. Initially, I found that it would get stuck on the export screen when running on Windows 11, making it unusable. After fixing the bug, I've verified that the export process now works correctly. |
|
can we have the conflicts resolved? |
What changed
This PR improves export stability, especially on Windows, by adding timeout protection and safer encoder fallback behavior.
Key updates
add source-loading timeouts in
streamingDecoderharden the video export pipeline in
videoExporterimprove Windows export behavior
small cleanup/refactor
Files changed
src/lib/exporter/streamingDecoder.tssrc/lib/exporter/videoExporter.tsWhy
This round mainly addresses export hangs, long waits, and encoder stall issues during video export, with extra attention to Windows stability and clearer failure paths.
Validation
git diff --checkpassednpm run lintandnpm testcould not be completed locally because dependencies were not in a clean usable statenpm ci --cache .npm-cache --foreground-scriptshitEBUSYonnode_modules/electron, and the current environment also showed Node engine warningsMerge request
@author This change is ready for review. If it looks good, please help merge it so the Windows export stability fixes can ship soon. Thanks!
Summary by CodeRabbit
Bug Fixes
New Features