From c30ccc2c7c779dfb791002d20a59a227c28ccbe9 Mon Sep 17 00:00:00 2001 From: EtienneLescot Date: Fri, 21 Aug 2026 17:26:46 +0200 Subject: [PATCH 1/2] fix(editor): apply the Edit Clip source range and crop as one document, one save MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/components/ai-edition/Modals.tsx | 2 +- src/components/ai-edition/NewEditorShell.tsx | 10 ++- src/lib/ai-edition/store/useTimeline.test.ts | 59 +++++++++++++- src/lib/ai-edition/store/useTimeline.ts | 81 ++++++++++++-------- 4 files changed, 113 insertions(+), 39 deletions(-) diff --git a/src/components/ai-edition/Modals.tsx b/src/components/ai-edition/Modals.tsx index cc077efd1..9ac9d1079 100644 --- a/src/components/ai-edition/Modals.tsx +++ b/src/components/ai-edition/Modals.tsx @@ -704,7 +704,7 @@ interface EditClipModalProps extends BaseModalProps { // a draggable dual-handle range over the asset's full source duration — // replaces the old numeric-input-only form. Trim range AND crop are both // per-clip and both edited here (see clipSchema.cropRegion / useTimeline's -// updateClipSourceRange + updateClipCrop) — crop used to be a document-wide +// applyClipEdit, which composes the two into one save) — crop used to be a document-wide // setting behind its own facet-rail button; it's a framing choice for one // piece of footage, so it belongs with the rest of this clip's edits. export function EditClipModal({ diff --git a/src/components/ai-edition/NewEditorShell.tsx b/src/components/ai-edition/NewEditorShell.tsx index 8c9ad63b5..4026266cb 100644 --- a/src/components/ai-edition/NewEditorShell.tsx +++ b/src/components/ai-edition/NewEditorShell.tsx @@ -1331,8 +1331,14 @@ export function NewEditorShell() { videoSources={videoSources} onApply={(sStart, sEnd, cropRegion) => { if (!editClipTarget) return; - void tl.updateClipSourceRange(editClipTarget.id, sStart, sEnd); - if (cropRegion !== undefined) void tl.updateClipCrop(editClipTarget.id, cropRegion); + const clipId = editClipTarget.id; + // One user action, one document, one save. This used to be two calls — + // `updateClipSourceRange` then `updateClipCrop` — each building its next + // document from the same pre-Apply one, so the second write clobbered the + // first and one of the two edits vanished silently (#355). It goes on the + // shared write queue for the same reason every other timeline edit does: + // so it can't clobber, or be clobbered by, a save already in flight. + void enqueueTimelineWrite(() => tl.applyClipEdit(clipId, sStart, sEnd, cropRegion)); setEditClipTarget(null); }} /> diff --git a/src/lib/ai-edition/store/useTimeline.test.ts b/src/lib/ai-edition/store/useTimeline.test.ts index cc51b3a01..5eeef2e32 100644 --- a/src/lib/ai-edition/store/useTimeline.test.ts +++ b/src/lib/ai-edition/store/useTimeline.test.ts @@ -341,7 +341,7 @@ describe("useTimeline backfills missing source dimensions on load", () => { }); }); -describe("useTimeline.updateClipSourceRange (Edit-clip modal)", () => { +describe("useTimeline.applyClipEdit (Edit-clip modal)", () => { const anchoredZoom = (id: string, s: number, e: number) => ({ id, startMs: s * 1000, @@ -380,7 +380,7 @@ describe("useTimeline.updateClipSourceRange (Edit-clip modal)", () => { const { result } = renderTimeline(); // Trim the 10s clip down to its first 4s of source. await act(async () => { - await result.current.updateClipSourceRange("clip_a", 0, 4); + await result.current.applyClipEdit("clip_a", 0, 4); }); const clip = useProjectStore.getState().document?.timeline.clips[0]; expect(clip).toMatchObject({ sourceStartSec: 0, sourceEndSec: 4 }); @@ -392,7 +392,7 @@ describe("useTimeline.updateClipSourceRange (Edit-clip modal)", () => { it("drops a pill sitting over the truncated tail and keeps the one that survives", async () => { const { result } = renderTimeline(); await act(async () => { - await result.current.updateClipSourceRange("clip_a", 0, 4); + await result.current.applyClipEdit("clip_a", 0, 4); }); const zooms = useProjectStore.getState().document?.zoomRanges ?? []; // z_keep (source 2-3) stays; z_drop (source 6-8) is entirely past the new 4s end. @@ -414,7 +414,7 @@ describe("useTimeline.updateClipSourceRange (Edit-clip modal)", () => { }); const { result } = renderTimeline(); await act(async () => { - await result.current.updateClipSourceRange("clip_a", 0, 5); + await result.current.applyClipEdit("clip_a", 0, 5); }); const zooms = useProjectStore.getState().document?.zoomRanges ?? []; expect(zooms).toHaveLength(1); @@ -426,6 +426,57 @@ describe("useTimeline.updateClipSourceRange (Edit-clip modal)", () => { endMs: 5000, }); }); + + // #355. Apply used to fire `updateClipSourceRange` and `updateClipCrop` as two + // concurrent saves, each built from the same pre-Apply document — so the second + // write clobbered the first and one of the two edits vanished with no error and no + // toast. Which one survived depended on IPC timing, which is why it read as "the app + // randomly forgets my crop". + it("keeps BOTH the source range and the crop when Apply changes them together", async () => { + const { result } = renderTimeline(); + const crop = { x: 0.1, y: 0.2, width: 0.5, height: 0.5 }; + await act(async () => { + await result.current.applyClipEdit("clip_a", 0, 4, crop); + }); + const clip = useProjectStore.getState().document?.timeline.clips[0]; + expect(clip).toMatchObject({ sourceStartSec: 0, sourceEndSec: 4, cropRegion: crop }); + // The width still followed the range edit — the crop is applied to the + // RESEQUENCED clips, not to a stale copy of them. + expect(clip?.timelineEndSec).toBe(4); + // One user action, one document, one write: two saves is the race itself. + expect(bridgeMocks.save).toHaveBeenCalledTimes(1); + }); + + it("clears the crop on an explicit null and leaves it alone on undefined", async () => { + useProjectStore.setState({ + document: { + ...sampleDoc, + timeline: { + ...sampleDoc.timeline, + clips: [ + { ...sampleDoc.timeline.clips[0], cropRegion: { x: 0, y: 0, width: 0.5, height: 1 } }, + ], + }, + }, + }); + const { result } = renderTimeline(); + // `undefined` is the modal's "crop section untouched" — the stored region stays. + await act(async () => { + await result.current.applyClipEdit("clip_a", 0, 6); + }); + expect(useProjectStore.getState().document?.timeline.clips[0].cropRegion).toEqual({ + x: 0, + y: 0, + width: 0.5, + height: 1, + }); + // `null` is "reset to no crop", stored as an absent field rather than the + // identity region. + await act(async () => { + await result.current.applyClipEdit("clip_a", 0, 6, null); + }); + expect(useProjectStore.getState().document?.timeline.clips[0].cropRegion).toBeUndefined(); + }); }); describe("useTimeline.addAnnotation", () => { diff --git a/src/lib/ai-edition/store/useTimeline.ts b/src/lib/ai-edition/store/useTimeline.ts index 716b4003a..11f3773b0 100644 --- a/src/lib/ai-edition/store/useTimeline.ts +++ b/src/lib/ai-edition/store/useTimeline.ts @@ -858,38 +858,56 @@ export function useTimeline() { setClipSelection(null); }, []); - // Axcut-consistent clip trim: only the source range is user-editable (the - // Edit Clip dialog's draggable track). Changing it changes the clip's - // effective duration, so every clip is resequenced back-to-back afterward — - // same invariant as insertClipAt/moveClip/removeClip — instead of leaving - // downstream clips at their old timeline positions (which would overlap). - // The whole recipe (resequence width + clamp/rederive pills) lives in the one - // pure `setClipSourceRange`, shared with the op dispatcher and the LLM tool. - const updateClipSourceRange = useCallback( - async (clipId: string, sourceStartSec: number, sourceEndSec: number) => { - if (!document) return; - await saveDocument(setClipSourceRange(document, clipId, sourceStartSec, sourceEndSec)); - }, - [document, saveDocument], - ); - - // Crop is a per-clip framing, not a document-wide setting — two clips - // (even from the same asset) can reasonably want different crops. Passing - // `null` clears it back to "no crop" (full frame) instead of storing the - // identity region explicitly. - const updateClipCrop = useCallback( - async (clipId: string, region: AxcutClipCropRegion | null) => { - if (!document) return; - const arr = document.timeline.clips.map((c) => - c.id === clipId ? { ...c, cropRegion: region ?? undefined } : c, - ); - const next: AxcutDocument = { - ...document, - timeline: { ...document.timeline, clips: arr }, - }; + // The Edit Clip dialog's Apply, as ONE document and ONE save. + // + // Source range and crop are two edits made in a single user action, and they used to + // be two independent saves fired back to back. Both built their next document from + // the SAME pre-Apply one — the crop write never saw the source-range change — so + // whichever IPC write landed last silently dropped the other edit, with no error and + // no toast (#355). Composing them means the crop is applied to the *resequenced* + // clips, which is also the only order that can be right. + // + // Axcut-consistent clip trim: only the source range is user-editable (the dialog's + // draggable track). Changing it changes the clip's effective duration, so every clip + // is resequenced back-to-back afterward — same invariant as + // insertClipAt/moveClip/removeClip — instead of leaving downstream clips at their old + // timeline positions (which would overlap). That whole recipe (resequence width + + // clamp/rederive pills) lives in the one pure `setClipSourceRange`, shared with the op + // dispatcher and the LLM tool. + // + // Crop is a per-clip framing, not a document-wide setting — two clips (even from the + // same asset) can reasonably want different crops. `undefined` means the dialog's crop + // section was never touched (leave the stored value alone); `null` clears it back to + // "no crop" (full frame) rather than storing the identity region explicitly. + // + // The document is read from the store, not off the render closure, so this composes + // with `useSequentialTimelineOps`: queued behind another timeline write, it still sees + // what that write committed. Same reason as `setTrimEntries` / `insertClipAt`. + const applyClipEdit = useCallback( + async ( + clipId: string, + sourceStartSec: number, + sourceEndSec: number, + cropRegion?: AxcutClipCropRegion | null, + ) => { + const doc = useProjectStore.getState().document; + if (!doc) return; + const ranged = setClipSourceRange(doc, clipId, sourceStartSec, sourceEndSec); + const next: AxcutDocument = + cropRegion === undefined + ? ranged + : { + ...ranged, + timeline: { + ...ranged.timeline, + clips: ranged.timeline.clips.map((c) => + c.id === clipId ? { ...c, cropRegion: cropRegion ?? undefined } : c, + ), + }, + }; await saveDocument(next); }, - [document, saveDocument], + [saveDocument], ); // Background probe: read the asset's actual duration and patch the @@ -1111,8 +1129,7 @@ export function useTimeline() { removeRegions, selectRegion, clearSelection, - updateClipSourceRange, - updateClipCrop, + applyClipEdit, insertClipAt, moveClip, duplicateClip, From 3bb220ecbcfa5947c9426a529cf304935dbf9f76 Mon Sep 17 00:00:00 2001 From: EtienneLescot Date: Fri, 21 Aug 2026 17:27:04 +0200 Subject: [PATCH 2/2] fix(editor): refuse Full Camera on a project that has no webcam MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../v4/V4Timeline.geometry.test.tsx | 47 ++++++++++++- src/components/ai-edition/v4/V4Timeline.tsx | 20 +++++- src/lib/ai-edition/store/useTimeline.test.ts | 66 +++++++++++++++++++ src/lib/ai-edition/store/useTimeline.ts | 12 ++++ 4 files changed, 143 insertions(+), 2 deletions(-) diff --git a/src/components/ai-edition/v4/V4Timeline.geometry.test.tsx b/src/components/ai-edition/v4/V4Timeline.geometry.test.tsx index 280853bef..f3a067bca 100644 --- a/src/components/ai-edition/v4/V4Timeline.geometry.test.tsx +++ b/src/components/ai-edition/v4/V4Timeline.geometry.test.tsx @@ -63,14 +63,19 @@ function clip(startSec: number, endSec: number) { }; } +/** The asset every clip above points at. No `cameraTrack`: this recording has no webcam, + * which is what the Full Camera button is gated on. */ +const NO_CAMERA_ASSET = { id: "a1", label: "rec", durationSec: TOTAL_SEC }; + /** By default one 30-minute clip carrying a single one-second annotation. */ function renderTimeline( clips = [clip(0, TOTAL_SEC)], annotation = { id: "ann1", startMs: 10_000, endMs: 11_000 }, + assets: Array> = [NO_CAMERA_ASSET], ) { const tl = { clips, - assets: [{ id: "a1", label: "rec", durationSec: TOTAL_SEC }], + assets, annotationRegions: [annotation], speedRegions: [], cameraFullscreenRegions: [], @@ -215,6 +220,46 @@ describe("V4Timeline create-from-toolbar", () => { fireEvent.click(screen.getByTitle("buttons.addZoom")); expect(durationOf(tl)).toBeCloseTo(0.25, 3); }); + + // #353. A camera-fullscreen region grows the webcam overlay, so with no webcam on the + // timeline it renders nothing in the preview and nothing in the export — the region is + // stored and forgotten. `addCameraFullscreen` now refuses to write one; the button says + // so before it is clicked instead of looking like it worked. + it("disables Add Full Camera when no clip on the timeline has a camera", () => { + renderTimeline(); + expect(screen.getByTitle("buttons.addCameraFullscreen")).toBeDisabled(); + }); + + it("enables Add Full Camera as soon as a clip's asset carries one", () => { + renderTimeline(undefined, undefined, [ + { + ...NO_CAMERA_ASSET, + cameraTrack: { sourcePath: "/tmp/cam.webm", startMs: 0, offsetMs: 0, visible: true }, + }, + ]); + expect(screen.getByTitle("buttons.addCameraFullscreen")).toBeEnabled(); + }); + + // The disabled button is only half the promise: an empty lane advertises the shortcut + // that fills it, so on a camera-less project it was still inviting a `C` press that + // `addCameraFullscreen` now refuses. It borrows the Layout pane's "No Webcam" wording + // instead, so the two surfaces agree about the same project. + it("does not advertise the C shortcut on a lane that cannot be filled", () => { + renderTimeline(); + expect(screen.getByText("layout.noWebcam")).toBeInTheDocument(); + expect(screen.queryByText("hints.pressCameraFullscreen")).not.toBeInTheDocument(); + }); + + it("advertises it again once a camera is on the timeline", () => { + renderTimeline(undefined, undefined, [ + { + ...NO_CAMERA_ASSET, + cameraTrack: { sourcePath: "/tmp/cam.webm", startMs: 0, offsetMs: 0, visible: true }, + }, + ]); + expect(screen.getByText("hints.pressCameraFullscreen")).toBeInTheDocument(); + expect(screen.queryByText("layout.noWebcam")).not.toBeInTheDocument(); + }); }); describe("V4Timeline clip row", () => { diff --git a/src/components/ai-edition/v4/V4Timeline.tsx b/src/components/ai-edition/v4/V4Timeline.tsx index 0872d7f87..b6e5f34ea 100644 --- a/src/components/ai-edition/v4/V4Timeline.tsx +++ b/src/components/ai-edition/v4/V4Timeline.tsx @@ -39,6 +39,7 @@ import { useTimelineTranscriptGate } from "@/lib/ai-edition/store/transcriptionS import { useChatPromptBus } from "@/lib/ai-edition/store/useChatPromptBus"; import { useEditorSettings } from "@/lib/ai-edition/store/useEditorSettings"; import type { useTimeline } from "@/lib/ai-edition/store/useTimeline"; +import { hasAnyClipWithCamera } from "@/lib/ai-edition/timeline/camera"; import { formatSec } from "@/lib/ai-edition/timeline/format"; import { newRegionDurationSec, @@ -396,6 +397,9 @@ export function V4Timeline({ onEditClip: (clip: AxcutClip) => void; }) { const t = useScopedT("timeline"); + // The camera lane borrows the Layout pane's "No Webcam" wording when there is no + // camera to grow, so the two surfaces say the same thing about the same project. + const ts = useScopedT("settings"); const tracksRef = useRef(null); // The transformed canvas is the true timeline coordinate frame — clips, pills // and the playhead are all positioned inside it. Time↔x math must measure THIS @@ -465,6 +469,12 @@ export function V4Timeline({ : t("toolbar.smartCutsNeedsTranscript"); const clips = tl.clips; + // A camera-fullscreen region grows the webcam overlay, so on a project with no webcam + // it renders nothing in the preview and nothing in the export. `addCameraFullscreen` + // refuses to write one (see useTimeline) — this makes the control say so before it is + // clicked instead of looking like it worked. Same question, same helper as the Layout + // pane: is a camera attached anywhere on this timeline? + const hasAnyCamera = useMemo(() => hasAnyClipWithCamera(tl.assets, clips), [tl.assets, clips]); const total = useMemo( () => Math.max( @@ -1394,6 +1404,8 @@ export function V4Timeline({ className={styles.tlToolBtn} title={t("buttons.addCameraFullscreen")} aria-label={t("buttons.addCameraFullscreen")} + disabled={!hasAnyCamera} + style={!hasAnyCamera ? { opacity: 0.55, cursor: "not-allowed" } : undefined} onClick={() => void tl.addCameraFullscreen(newRegionDurationSec())} > @@ -1555,7 +1567,13 @@ export function V4Timeline({
{renderPills(trimPills, t("hints.pressTrim"))}
{renderPills(zoomPills, t("hints.pressZoom"))}
- {renderPills(cameraFullscreenPills, t("hints.pressCameraFullscreen"))} + {/* Advertising "Press C" on a project with no webcam invites a keystroke + that `addCameraFullscreen` now refuses (#353). The toolbar button is + already disabled; this keeps the lane from contradicting it. */} + {renderPills( + cameraFullscreenPills, + hasAnyCamera ? t("hints.pressCameraFullscreen") : ts("layout.noWebcam"), + )}
) : null} diff --git a/src/lib/ai-edition/store/useTimeline.test.ts b/src/lib/ai-edition/store/useTimeline.test.ts index 5eeef2e32..939fc7c9d 100644 --- a/src/lib/ai-edition/store/useTimeline.test.ts +++ b/src/lib/ai-edition/store/useTimeline.test.ts @@ -479,6 +479,72 @@ describe("useTimeline.applyClipEdit (Edit-clip modal)", () => { }); }); +// #353. The toolbar button and the `C` shortcut both used to write a region on a +// project with no webcam: it persists into `legacyEditor.cameraFullscreenRegions`, +// renders nothing in the preview (PreviewCanvas short-circuits on a missing +// `webcamRect`) and nothing in the export, forever, with no feedback. The gate lives +// in the shared mutation so both entry points — and any future one — are covered. +describe("useTimeline.addCameraFullscreen (camera gate)", () => { + const cameraAsset = { + ...sampleDoc.assets[0], + cameraTrack: { + sourcePath: "/tmp/camera.webm", + startMs: 0, + offsetMs: 0, + visible: true, + // Dimensions filled in so the hook's backfill probe has nothing to do — an + // unprobed camera would fire its own `saveDocument` alongside this test's. + width: 1280, + height: 720, + }, + }; + + beforeEach(() => { + useProjectStore.getState().clear(); + for (const mock of Object.values(bridgeMocks)) mock.mockReset(); + bridgeMocks.save.mockImplementation(async (doc: typeof sampleDoc) => ({ + success: true, + document: doc, + })); + useProjectStore.setState({ + projectId: "proj_test", + document: sampleDoc, + currentTimeSec: 1, + revision: 1, + status: "ready", + error: null, + }); + }); + + afterEach(() => { + vi.clearAllMocks(); + }); + + it("writes nothing when no clip on the timeline has a camera", async () => { + // sampleDoc's only asset carries `cameraTrack: null`. + const { result } = renderTimeline(); + await act(async () => { + await result.current.addCameraFullscreen(); + }); + expect(bridgeMocks.save).not.toHaveBeenCalled(); + expect(useProjectStore.getState().document?.legacyEditor).toBeNull(); + expect(result.current.cameraFullscreenRegions).toEqual([]); + }); + + it("still writes a region when a clip's asset carries a camera", async () => { + useProjectStore.setState({ document: { ...sampleDoc, assets: [cameraAsset] } }); + const { result } = renderTimeline(); + await act(async () => { + await result.current.addCameraFullscreen(); + }); + const legacy = useProjectStore.getState().document?.legacyEditor as Record; + const regions = legacy.cameraFullscreenRegions as Array<{ startMs: number; endMs: number }>; + expect(regions).toHaveLength(1); + // 2s at the playhead (currentTimeSec = 1), the shared default. + expect(regions[0]).toMatchObject({ startMs: 1000, endMs: 3000 }); + }); +}); + describe("useTimeline.addAnnotation", () => { beforeEach(() => { useProjectStore.getState().clear(); diff --git a/src/lib/ai-edition/store/useTimeline.ts b/src/lib/ai-edition/store/useTimeline.ts index 11f3773b0..788a8a4fc 100644 --- a/src/lib/ai-edition/store/useTimeline.ts +++ b/src/lib/ai-edition/store/useTimeline.ts @@ -20,6 +20,7 @@ import { setClipSourceRange, } from "../document/timeline"; import type { AxcutClipCropRegion, AxcutDocument } from "../schema"; +import { hasAnyClipWithCamera } from "../timeline/camera"; import { probeVideoDimensions, probeVideoDuration } from "../timeline/duration"; import { anchorRegionsWithDerivedMs, @@ -394,9 +395,20 @@ export function useTimeline() { // Full Camera: a plain time span (no value) during which the preview/export // grows the webcam overlay to (almost) fill the canvas and eases it back. + // + // With no webcam anywhere on the timeline there is nothing to grow, so the region + // renders nothing in the preview (`PreviewCanvas.effectiveLayout` short-circuits on + // a missing `webcamRect`) and nothing in the export — it just sits in + // `legacyEditor.cameraFullscreenRegions` forever. The agent's `addCameraFullscreen` + // tool already refuses this and says why (electron/ai-edition/agent-tools.ts, + // `noCameraUnderSpan`); the gate lives HERE rather than at each button so both UI + // entry points — the toolbar and the `C` shortcut — and any future one are covered + // by construction. `hasAnyClipWithCamera` is the consolidated answer to "does this + // project have a camera at all", used the same way by the Layout pane. const addCameraFullscreen = useCallback( async (durationSec = DEFAULT_NEW_REGION_SEC) => { if (!document) return; + if (!hasAnyClipWithCamera(document.assets, document.timeline.clips)) return; const timeMs = Math.round(playheadSec() * 1000); const endMs = timeMs + Math.round(durationSec * 1000); const legacy = (document.legacyEditor as Record) ?? {};