Skip to content

fix: prevent stale-config frames from appearing in exported video near trim boundaries #1711

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MinitJain wants to merge 4 commits into
base: main
Choose a base branch
from
+61 −46
Open

fix: prevent stale-config frames from appearing in exported video near trim boundaries #1711

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4c85528
Fix trim handle flickering in editor
MinitJain Apr 4, 2026
20eafba
fix: let queueMicrotask be sole owner of skipRenderFrameForConfigUpda…
MinitJain Apr 5, 2026
97e2490
fix: sort imports in ClipTrack.tsx for Biome format check
MinitJain Apr 6, 2026
d0f553c
fix: correct import sort order in ClipTrack.tsx per Biome rules
MinitJain Apr 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 37 additions & 27 deletions apps/desktop/src/routes/editor/Editor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,7 +365,12 @@ function Inner() {
setEditorState("playbackTime", payload.playhead_position / FPS);
});

let skipRenderFrameForConfigUpdate = false;

const emitRenderFrame = (time: number) => {
if (skipRenderFrameForConfigUpdate) {
return;
}
Comment on lines 370 to +373
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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:

Suggested change
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.

if (!editorState.playing) {
events.renderFrameEvent.emit({
frame_number: Math.max(Math.floor(time * FPS), 0),
Expand All @@ -390,33 +395,6 @@ function Inner() {
return editorState.playbackTime;
});

createEffect(
on(
() => [frameNumberToRender(), previewResolutionBase()],
([number]) => {
if (editorState.playing) return;
renderFrame(number as number);
},
{ defer: false },
),
);

createEffect(
on(isExportMode, (exportMode, prevExportMode) => {
if (prevExportMode === true && exportMode === false) {
emitRenderFrame(frameNumberToRender());
}
}),
);

createEffect(
on(isCropMode, (cropMode, prevCropMode) => {
if (prevCropMode === true && cropMode === false) {
emitRenderFrame(frameNumberToRender());
}
}),
);

const doConfigUpdate = async (time: number) => {
const config = getPreviewProjectConfig(project, editorState);
const frameNumber = Math.max(Math.floor(time * FPS), 0);
Expand All @@ -441,6 +419,7 @@ function Inner() {
throttledConfigUpdate(time);
trailingConfigUpdate(time);
};

createEffect(
on(
() => {
Expand All @@ -451,12 +430,43 @@ function Inner() {
};
},
() => {
skipRenderFrameForConfigUpdate = true;
queueMicrotask(() => {
skipRenderFrameForConfigUpdate = false;
});
updateConfigAndRender(frameNumberToRender());
},
{ defer: true },
),
);

createEffect(
on(
() => [frameNumberToRender(), previewResolutionBase()],
([number]) => {
if (editorState.playing) return;
renderFrame(number as number);
},
{ defer: false },
),
);
Comment on lines +443 to +452
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.


createEffect(
on(isExportMode, (exportMode, prevExportMode) => {
if (prevExportMode === true && exportMode === false) {
emitRenderFrame(frameNumberToRender());
}
}),
);

createEffect(
on(isCropMode, (cropMode, prevCropMode) => {
if (prevCropMode === true && cropMode === false) {
emitRenderFrame(frameNumberToRender());
}
}),
);

const fullscreenMode = () => {
if (isExportMode()) return "export" as const;
return null;
Expand Down
43 changes: 24 additions & 19 deletions apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
} from "@solid-primitives/event-listener";
import { cx } from "cva";
import {
batch,
type ComponentProps,
createEffect,
createMemo,
Expand Down Expand Up @@ -718,14 +719,16 @@ export function ClipTrack(
initialStart,
});

setProject(
"timeline",
"segments",
i(),
"start",
clampedStart,
);
setPreviewTime(prevDuration());
batch(() => {
setProject(
"timeline",
"segments",
i(),
"start",
clampedStart,
);
setPreviewTime(prevDuration());
});
}

const resumeHistory = projectHistory.pause();
Expand Down Expand Up @@ -822,17 +825,19 @@ export function ClipTrack(
seg.start + minRecordedDuration,
);

setProject(
"timeline",
"segments",
i(),
"end",
clampedEnd,
);
setPreviewTime(
prevDuration() +
(clampedEnd - seg.start) / seg.timescale,
);
batch(() => {
setProject(
"timeline",
"segments",
i(),
"end",
clampedEnd,
);
setPreviewTime(
prevDuration() +
(clampedEnd - seg.start) / seg.timescale,
);
});
}

const resumeHistory = projectHistory.pause();
Expand Down