fix(editor): apply Edit Clip as one save, and refuse Full Camera with no webcam - #444
Merged
EtienneLescot merged 2 commits intoAug 21, 2026
Merged
Conversation
…t, one save Changing both the source range and the crop and clicking Apply fired two independent saves built from the SAME pre-Apply document: the crop write never saw the source-range change, so whichever IPC write landed last silently dropped the other edit. No error, no toast, and the loser depends on timing — which is what reads as "the app randomly forgets my crop". Replace the two hook methods with a single `applyClipEdit` that runs the shared `setClipSourceRange` recipe and then maps the crop onto the *resequenced* clips of that same document before saving once. That order is also the only one that can be right: resequencing changes which clips exist to be cropped. Two supporting details. The document is read from the store rather than the render closure, the idiom `setTrimEntries` and `insertClipAt` already use, so the call is safe to queue; and Apply now goes through `enqueueTimelineWrite`, the serialisation this race is exactly what `useSequentialTimelineOps` exists to prevent. Fixes #355
The toolbar button and the `C` shortcut wrote a camera-fullscreen region on projects with no webcam at all. The region persisted into `legacyEditor.cameraFullscreenRegions`, rendered nothing in the preview and nothing in the export — `effectiveLayout` short-circuits with no `webcamRect` to grow — and the user got no feedback, ever. The agent's own tool already refused this exact action and said why, so the app contradicted itself depending on which entry point you used. Gate the shared mutation on `hasAnyClipWithCamera`, the consolidated answer the Layout pane already uses, so both UI entry points and any future one are covered by construction rather than one guard per button. Then make the surfaces honest before they are clicked: the toolbar button is disabled and dimmed, and the empty lane stops advertising "Press C to add a Full Camera segment" — it borrows the Layout pane's existing "No Webcam" wording, so the two surfaces agree about the same project and no new locale keys are needed. The lane was found still inviting the keystroke during a visual pass, after the button was already correct. Fixes #353
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe timeline store now applies source-range and crop edits in one save. Camera fullscreen actions are blocked when no camera exists. The timeline UI shows disabled controls and a no-webcam message for camera-less assets. ChangesTimeline editing and camera availability
Estimated code review effort: 3 (Moderate) | ~30 minutes Merge Risk: ⚪ Minimal · up to The PR makes clip edits save atomically and prevents Full Camera actions when no webcam exists; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
EtienneLescot
deleted the
claude/fix-355-353-clip-apply-and-camera-gate
branch
August 21, 2026 17:34
This was referenced Aug 21, 2026
Closed
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.
Two independent bugs, one PR: both fixes land in
useTimeline.tsand their tests interleave inuseTimeline.test.ts. Splitting them into parallel PRs would guarantee a rebase for whichever merged second, for no reviewer benefit — each is its own commit, review them separately.#355 — Apply loses one edit when source range and crop change together
Changing both and clicking Apply fired two independent saves built from the SAME pre-Apply document: the crop write never saw the source-range change, so whichever IPC write landed last silently dropped the other edit. No error, no toast, and the loser depends on timing — which is what reads as "the app randomly forgets my crop".
updateClipSourceRange+updateClipCropare replaced by a singleapplyClipEditthat runs the sharedsetClipSourceRangerecipe and then maps the crop onto the resequenced clips of that same document before saving once. That order is also the only one that can be right: resequencing changes which clips exist to be cropped.Two supporting details. The document is read from the store rather than the render closure — the idiom
setTrimEntriesandinsertClipAtalready use — so the call is safe to queue; and Apply now goes throughenqueueTimelineWrite, since this race is exactly whatuseSequentialTimelineOpsexists to prevent.The test asserts both edits survive and that
savewas called exactly once; a second pins the modal's tri-state crop contract (undefined= untouched,null= cleared).#353 — Full Camera on a project with no webcam
The toolbar button and the
Cshortcut wrote a camera-fullscreen region on projects with no webcam at all. It persisted, rendered nothing in preview and nothing in export —effectiveLayoutshort-circuits with nowebcamRectto grow — and the user got no feedback, ever. The agent's own tool already refused this exact action and said why, so the app contradicted itself depending on which entry point you used.The gate goes on the shared mutation, keyed on
hasAnyClipWithCamera— the consolidated answer the Layout pane already uses — so both entry points and any future one are covered by construction rather than one guard per button.Then the surfaces are made honest before they are clicked: the toolbar button is disabled and dimmed, and the empty lane stops advertising "Press C to add a Full Camera segment".
That lane was found during a visual pass, after the button was already correct — the fix was half-honest and no unit test would have caught it. It borrows the Layout pane's existing "No Webcam" wording, so the two surfaces agree about the same project and no new locale keys are needed. Verified on screen: the camera lane reads "No Webcam" while the others keep their shortcut hints.
A sweep for callers that depended on the mutation always writing turned up none. The agent path (
electron/ai-edition/agent-tools.ts) has its own implementation and its own gate, and does not go through this hook, so its tests are unaffected.Fixes #355
Fixes #353
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation