fix: prevent stale-config frames from appearing in exported video near trim boundaries#1711
fix: prevent stale-config frames from appearing in exported video near trim boundaries#1711MinitJain wants to merge 4 commits intoCapSoftware:mainfrom
Conversation
Wrap trim handle state updates in batch() so project segment and previewTime update atomically before effects fire. Reorder effects in Editor.tsx so the config-update effect is created before the render-frame effect (SolidJS fires effects in creation order). Add skipRenderFrameForConfigUpdate flag so renderFrameEvent is suppressed when updateConfigAndRender already handles the render, eliminating the race condition where a stale-config frame was emitted before the async config update completed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| const emitRenderFrame = (time: number) => { | ||
| if (skipRenderFrameForConfigUpdate) { | ||
| skipRenderFrameForConfigUpdate = false; | ||
| return; | ||
| } |
There was a problem hiding this comment.
Redundant flag reset inside
emitRenderFrame
The skipRenderFrameForConfigUpdate flag is cleared in two places: eagerly inside emitRenderFrame at line 372, and again by the queueMicrotask at line 436. The reset inside emitRenderFrame is redundant since the microtask already owns cleanup.
More importantly, this early reset means that if emitRenderFrame is invoked more than once synchronously within the same reactive flush — e.g., the leading-edge throttled render fires and an isExportMode/isCropMode exit handler calls emitRenderFrame directly before the microtask runs — only the first call is blocked. The flag is cleared prematurely and the second call bypasses the guard before updateProjectConfigInMemory has completed. While the current UI should prevent those callers from coinciding, letting the microtask be the sole owner of flag cleanup is simpler and more defensive:
| const emitRenderFrame = (time: number) => { | |
| if (skipRenderFrameForConfigUpdate) { | |
| skipRenderFrameForConfigUpdate = false; | |
| return; | |
| } | |
| const emitRenderFrame = (time: number) => { | |
| if (skipRenderFrameForConfigUpdate) { | |
| return; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/routes/editor/Editor.tsx
Line: 370-374
Comment:
**Redundant flag reset inside `emitRenderFrame`**
The `skipRenderFrameForConfigUpdate` flag is cleared in two places: eagerly inside `emitRenderFrame` at line 372, and again by the `queueMicrotask` at line 436. The reset inside `emitRenderFrame` is redundant since the microtask already owns cleanup.
More importantly, this early reset means that if `emitRenderFrame` is invoked more than once synchronously within the same reactive flush — e.g., the leading-edge throttled render fires *and* an `isExportMode`/`isCropMode` exit handler calls `emitRenderFrame` directly before the microtask runs — only the first call is blocked. The flag is cleared prematurely and the second call bypasses the guard before `updateProjectConfigInMemory` has completed. While the current UI should prevent those callers from coinciding, letting the microtask be the sole owner of flag cleanup is simpler and more defensive:
```suggestion
const emitRenderFrame = (time: number) => {
if (skipRenderFrameForConfigUpdate) {
return;
}
```
How can I resolve this? If you propose a fix, please make it concise.| createEffect( | ||
| on( | ||
| () => [frameNumberToRender(), previewResolutionBase()], | ||
| ([number]) => { | ||
| if (editorState.playing) return; | ||
| renderFrame(number as number); | ||
| }, | ||
| { defer: false }, | ||
| ), | ||
| ); |
There was a problem hiding this comment.
trailingRenderFrame is not protected by the skip flag
renderFrame() at line 449 enqueues both throttledRenderFrame (leading-edge, synchronously guarded by the flag) and trailingRenderFrame (a debounce with delay 1000/FPS + 16 ms). By the time the trailing debounce fires, the queueMicrotask has already cleared skipRenderFrameForConfigUpdate, so the trailing render is never blocked by the guard.
In practice this is acceptable — throttledConfigUpdate dispatches the IPC on the leading edge and Tauri IPC is typically sub-millisecond, well within the ~49 ms trailing window. However, this is an implicit timing assumption: under system load, if the IPC call takes longer than the debounce window, renderFrameEvent can still arrive at Rust before updateProjectConfigInMemory completes — recreating the same race this PR intends to fix. Consider adding a comment documenting why the trailing window is safe, or coordinating the trailing render with the in-flight config promise.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/routes/editor/Editor.tsx
Line: 444-453
Comment:
**`trailingRenderFrame` is not protected by the skip flag**
`renderFrame()` at line 449 enqueues both `throttledRenderFrame` (leading-edge, synchronously guarded by the flag) and `trailingRenderFrame` (a debounce with delay `1000/FPS + 16` ms). By the time the trailing debounce fires, the `queueMicrotask` has already cleared `skipRenderFrameForConfigUpdate`, so the trailing render is never blocked by the guard.
In practice this is acceptable — `throttledConfigUpdate` dispatches the IPC on the leading edge and Tauri IPC is typically sub-millisecond, well within the ~49 ms trailing window. However, this is an implicit timing assumption: under system load, if the IPC call takes longer than the debounce window, `renderFrameEvent` can still arrive at Rust before `updateProjectConfigInMemory` completes — recreating the same race this PR intends to fix. Consider adding a comment documenting why the trailing window is safe, or coordinating the trailing render with the in-flight config promise.
How can I resolve this? If you propose a fix, please make it concise.…te reset Removing the early reset inside emitRenderFrame prevents a second synchronous call (e.g. export/crop mode exit handlers) from bypassing the guard before updateProjectConfigInMemory completes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Addressed Fix 1 — the early reset inside On Fix 2 (trailing render not covered by the flag): agreed this is an implicit timing assumption. The trailing debounce fires at This codebase has a strict no-comments policy so I haven't added inline documentation, but happy to explore coordinating the trailing render with the config promise if the maintainers feel the timing assumption needs to be made explicit in code rather than prose. |
|
@noeljackson No conflicts, just needs maintainer approval for the Vercel workflow to run. Ready for review! |
|
@MinitJain i am not a maintainer, i just had 1 pr on this project. |
|
@noeljackson Apologies for the confusion! |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Biome uses case-sensitive ASCII sort (uppercase before lowercase), so 'type ComponentProps' (C) must precede 'createEffect' (c). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Problem
When exporting a video with trimmed clips, frames from outside the trimmed region would appear in the exported video at or near trim boundaries — particularly in sections with high motion. This is a race condition in the frontend reactive pipeline between two effects that both fire when the user adjusts a trim handle:
frameNumberToRender()and emits arenderFrameEventTauri event (fast, in-process) directly to the Rust preview rendererproject(the timeline config including trim boundaries) and callsupdateProjectConfigInMemory(async Tauri IPC command) to push the new config to RustBecause Effect A was created before Effect B in the component tree, SolidJS fires it first within the same reactive batch. This means Rust receives a frame render request referencing the old trim boundaries before the config update arrives. The result: a frame from outside the new trimmed region gets rendered and written into the export.
The same issue existed during preview (flickering while dragging trim handles), but its most visible and impactful symptom was in the exported video.
Root Cause
Two compounding issues:
1. Non-atomic state updates in
ClipTrack.tsxThe trim handle
mousemovehandlers were updatingproject.timeline.segments[i].start(or.end) andsetPreviewTimeas two separate signal writes. Between those two writes, SolidJS effects could fire with an inconsistent state —frameNumberToRenderwould reflect the newpreviewTimebefore the segment bounds were updated, causing the wrong frame to be rendered.2. Wrong effect creation order in
Editor.tsxSolidJS fires independent
createEffectcalls in creation order when they are triggered in the same reactive batch. The render-frame effect was created before the config-update effect, so it always fired first — emittingrenderFrameEventwith the stale config beforeupdateProjectConfigInMemorycould run.Fix
ClipTrack.tsx— wrap both state writes insidebatch()sosegment.start/segment.endandpreviewTimeare committed atomically before any effects fire:Editor.tsx— two changes:Move the config-update effect (
trackDeep(project)→updateProjectConfigInMemory) to be created before the render-frame effect so it fires first in any shared reactive batch.Add a
skipRenderFrameForConfigUpdateflag. When the config-update effect fires, it sets this flag synchronously and schedules aqueueMicrotaskto clear it. The render-frame effect checks the flag and short-circuits — preventing a redundantrenderFrameEventfrom racing with the in-flightupdateProjectConfigInMemory.Testing
Manually verified on macOS (Apple Silicon) by:
Before the fix: frames from before the trim-start or after the trim-end were visible in the exported video at cut points.
After the fix: exports are clean with no out-of-bounds frames at trim boundaries.
Files Changed
apps/desktop/src/routes/editor/Timeline/ClipTrack.tsxbatch()apps/desktop/src/routes/editor/Editor.tsxskipRenderFrameForConfigUpdateflagGreptile Summary
This PR fixes a race condition in the desktop video editor where frames from outside the trimmed region appeared in exported videos near trim boundaries. Two root causes are addressed: non-atomic state updates in
ClipTrack.tsx(now wrapped inbatch()) and incorrect effect creation order inEditor.tsx(config-update effect moved before render-frame effect, plus askipRenderFrameForConfigUpdateflag to block the leading-edge render until the updated config reaches Rust).ClipTrack.tsx: Both the start and end trim handlemousemovehandlers now wrapsetProjectandsetPreviewTimeinsidebatch(), ensuring segment boundaries and preview time are committed atomically before any downstream effects fire.Editor.tsx: The config-update effect is created before the render-frame effect, guaranteeing it fires first in any shared reactive batch. AskipRenderFrameForConfigUpdateboolean flag is set synchronously when the config-update effect runs; the render-frame effect checks this flag and skips the leading-edge render. AqueueMicrotaskclears the flag after the current synchronous tick.emitRenderFrameitself (redundant with the microtask), which could prematurely unblock a second synchronousemitRenderFramecaller in the same flush before the Rust config update completes.trailingRenderFrame) fires after the microtask clears the flag and is therefore never blocked by the guard — it implicitly relies on Tauri IPC completing within the ~49 ms debounce window.Confidence Score: 4/5
This PR is safe to merge — the race condition fix is architecturally sound and the observable export artifact bug is resolved.
The effect-reordering and batch() combination correctly prevents stale-config frames from being rendered at trim boundaries. Two minor style concerns remain: the skip flag is cleared redundantly in emitRenderFrame (could weaken the guard in edge cases with multiple synchronous emitRenderFrame callers), and the trailing debounce relies on an implicit timing assumption about Tauri IPC latency.
apps/desktop/src/routes/editor/Editor.tsx — specifically the skipRenderFrameForConfigUpdate flag management (redundant eager reset) and the unguarded trailingRenderFrame path.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A["MouseMove on trim handle"] --> B["batch(setProject + setPreviewTime)\nAtomic signal update"] B --> C["SolidJS reactive batch fires"] C --> D["Effect B fires FIRST\n(config-update, created first)"] D --> E["skipRenderFrameForConfigUpdate = true"] D --> F["queueMicrotask → clear flag"] D --> G["updateConfigAndRender()\nthrottledConfigUpdate IPC dispatched immediately"] C --> H["Effect A fires SECOND\n(render-frame, created second)"] H --> I["renderFrame() → throttledRenderFrame()\n→ emitRenderFrame()"] I --> J{"skipRenderFrameForConfigUpdate?"} J -- "true" --> K["Leading-edge render SKIPPED ✓"] J -- "false" --> L["renderFrameEvent sent to Rust"] K --> M["microtask runs: flag cleared"] M --> N["trailingRenderFrame fires ~49ms later\n→ emitRenderFrame flag=false\n→ renderFrameEvent sent to Rust"] G --> O["updateProjectConfigInMemory IPC\narrives at Rust with NEW config"] O --> P["Rust renders frame with correct\ntrim boundaries ✓"]Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "Fix trim handle flickering in editor" | Re-trigger Greptile
(2/5) Greptile learns from your feedback when you react with thumbs up/down!