From 0e17ec5e7c1e5a1b78020fd44bbce66fe57d998b Mon Sep 17 00:00:00 2001 From: dazzatronus Date: Fri, 3 Jul 2026 18:03:36 +1000 Subject: [PATCH 1/3] feat: surface command outcomes (success/noop) from Edit mutators --- src/core/edit-session.ts | 161 ++++++++++++++++++----------- src/core/shotstack-edit.ts | 7 +- src/index.ts | 1 + tests/edit-clip-operations.test.ts | 66 ++++++++++++ 4 files changed, 174 insertions(+), 61 deletions(-) diff --git a/src/core/edit-session.ts b/src/core/edit-session.ts index 10930be9..3b7a4e86 100644 --- a/src/core/edit-session.ts +++ b/src/core/edit-session.ts @@ -55,7 +55,7 @@ import { calculateOverlap } from "@timeline/interaction/interaction-calculations import * as pixi from "pixi.js"; import { CommandQueue } from "./commands/command-queue"; -import type { EditCommand, CommandContext, CommandResult } from "./commands/types"; +import { CommandNoop, type EditCommand, type CommandContext, type CommandResult } from "./commands/types"; import { EditDocument } from "./edit-document"; import { PlayerReconciler } from "./player-reconciler"; import { resolve as resolveDocument, resolveClip as resolveClipById, type SingleClipContext } from "./resolver"; @@ -419,36 +419,46 @@ export class Edit { /** * Patch fields on a clip identified by stable ID. + * + * Resolves with the command outcome: `success` when the clip was updated, or `noop` + * (with a `message`) when no clip matches `clipId`. */ - public updateClipById(clipId: string, updates: Partial): Promise { + public updateClipById(clipId: string, updates: Partial): Promise { const found = this.document.getClipById(clipId); if (!found) { console.warn(`updateClipById: no clip with id ${clipId}`); - return Promise.resolve(); + return Promise.resolve(CommandNoop(`No clip with id ${clipId}`)); } return this.updateClip(found.trackIndex, found.clipIndex, updates); } /** * Remove a clip identified by stable ID. + * + * Resolves with the command outcome: `success` when the clip was removed, or `noop` + * (with a `message`) when no clip matches `clipId` or the deletion was refused — + * the timeline always keeps at least one clip. */ - public deleteClipById(clipId: string): Promise { + public deleteClipById(clipId: string): Promise { const found = this.document.getClipById(clipId); if (!found) { console.warn(`deleteClipById: no clip with id ${clipId}`); - return Promise.resolve(); + return Promise.resolve(CommandNoop(`No clip with id ${clipId}`)); } return this.deleteClip(found.trackIndex, found.clipIndex); } /** * Move a clip identified by stable ID to a different track and/or start time. + * + * Resolves with the command outcome: `success` when the clip was moved, or `noop` + * (with a `message`) when no clip matches `clipId`. */ - public moveClipById(clipId: string, toTrackIndex: number, newStart?: Seconds): Promise { + public moveClipById(clipId: string, toTrackIndex: number, newStart?: Seconds): Promise { const found = this.document.getClipById(clipId); if (!found) { console.warn(`moveClipById: no clip with id ${clipId}`); - return Promise.resolve(); + return Promise.resolve(CommandNoop(`No clip with id ${clipId}`)); } const currentStart = found.clip.start; const start = newStart ?? (typeof currentStart === "number" ? (currentStart as Seconds) : null); @@ -460,7 +470,7 @@ export class Edit { ); } const command = new MoveClipCommand(found.trackIndex, found.clipIndex, toTrackIndex, start); - return Promise.resolve(this.executeCommand(command)); + return this.executeCommand(command); } /** @@ -614,7 +624,7 @@ export class Edit { return updated !== false; } - public async addClip(trackIdx: number, clip: Clip): Promise { + public async addClip(trackIdx: number, clip: Clip): Promise { ClipSchema.parse(clip); await this.preflightAssetUrls(extractClipUrls(clip)); // Cast to ResolvedClip - the Player and timing resolver handle "auto"/"end" at runtime @@ -909,13 +919,20 @@ export class Edit { return this.document.getClip(trackIdx, clipIdx) !== null; } - public async deleteClip(trackIdx: number, clipIdx: number): Promise { + /** + * Delete the clip at `(trackIdx, clipIdx)`, along with any luma matte attached to it. + * + * Resolves with the command outcome for the requested clip: `success` when it was + * removed, or `noop` (with a `message`) when there is no clip at that position or the + * deletion was refused — the timeline always keeps at least one clip. + */ + public async deleteClip(trackIdx: number, clipIdx: number): Promise { const track = this.tracks[trackIdx]; - if (!track) return; + if (!track) return CommandNoop(`No track at index ${trackIdx}`); // Get the clip being deleted const clipToDelete = track[clipIdx]; - if (!clipToDelete) return; + if (!clipToDelete) return CommandNoop(`No clip at track ${trackIdx}, index ${clipIdx}`); // Check if this is a content clip (not a luma) const isContentClip = clipToDelete.playerType !== PlayerType.Luma; @@ -932,27 +949,29 @@ export class Edit { const lumaCommand = new DeleteClipCommand(trackIdx, lumaIndex); await this.executeCommand(lumaCommand); - // Now delete content clip with adjusted index + // Now delete content clip with adjusted index — this outcome is the one the + // caller asked about, so it is the one returned const contentCommand = new DeleteClipCommand(trackIdx, adjustedContentIdx); - await this.executeCommand(contentCommand); - return; + return this.executeCommand(contentCommand); } } // No luma attachment or deleting a luma directly - just delete the clip const command = new DeleteClipCommand(trackIdx, clipIdx); - await this.executeCommand(command); + return this.executeCommand(command); } - public async addTrack(trackIdx: number, track: Track): Promise { + public async addTrack(trackIdx: number, track: Track): Promise { TrackSchema.parse(track); await this.preflightAssetUrls(extractTrackUrls(track)); // Single atomic command — track + all its clips in one undo step. - await this.executeCommand(new AddTrackCommand(trackIdx, track)); + const result = await this.executeCommand(new AddTrackCommand(trackIdx, track)); // Auto-link caption clips with unresolved alias sources await this.autoLinkCaptionSources(trackIdx, track.clips); + + return result; } /** @@ -1006,43 +1025,62 @@ export class Edit { }; } - public deleteTrack(trackIdx: number): void { + /** + * Delete the track at `trackIdx` and all of its clips. + * + * Resolves with the command outcome: `success` when the track was removed, or `noop` + * (with a `message`) when the deletion was refused — the timeline always keeps at + * least one track. + */ + public deleteTrack(trackIdx: number): Promise { const command = new DeleteTrackCommand(trackIdx); - this.executeCommand(command); + return this.executeCommand(command); } - public undo(): Promise { + /** + * Undo the most recent command. + * + * Resolves with the outcome: `success` when a command was undone, or `noop` (with a + * `message`) when the history is empty or the command cannot be undone. + */ + public undo(): Promise { return this.commandQueue.enqueue(async () => { - if (this.commandIndex >= 0) { - const command = this.commandHistory[this.commandIndex]; - if (command.undo) { - const context = this.createCommandContext(); - // Always await - harmless on sync results, works across realms - await Promise.resolve(command.undo(context)); - // Only decrement after successful completion - this.commandIndex -= 1; - - this.internalEvents.emit(EditEvent.EditUndo, { command: command.name }); - this.emitEditChanged(`undo:${command.name}`); - } - } + if (this.commandIndex < 0) return CommandNoop("Nothing to undo"); + const command = this.commandHistory[this.commandIndex]; + if (!command.undo) return CommandNoop("Command cannot be undone"); + + const context = this.createCommandContext(); + // Always await - harmless on sync results, works across realms + const result = await Promise.resolve(command.undo(context)); + // Only decrement after successful completion + this.commandIndex -= 1; + + this.internalEvents.emit(EditEvent.EditUndo, { command: command.name }); + this.emitEditChanged(`undo:${command.name}`); + return result; }); } - public redo(): Promise { + /** + * Re-apply the most recently undone command. + * + * Resolves with the outcome: `success` when a command was re-applied, or `noop` (with + * a `message`) when there is nothing to redo. + */ + public redo(): Promise { return this.commandQueue.enqueue(async () => { - if (this.commandIndex < this.commandHistory.length - 1) { - const nextIndex = this.commandIndex + 1; - const command = this.commandHistory[nextIndex]; - const context = this.createCommandContext(); - // Always await - harmless on sync results, works across realms - await Promise.resolve(command.execute(context)); - // Only increment after successful completion - this.commandIndex = nextIndex; - - this.internalEvents.emit(EditEvent.EditRedo, { command: command.name }); - this.emitEditChanged(`redo:${command.name}`); - } + if (this.commandIndex >= this.commandHistory.length - 1) return CommandNoop("Nothing to redo"); + const nextIndex = this.commandIndex + 1; + const command = this.commandHistory[nextIndex]; + const context = this.createCommandContext(); + // Always await - harmless on sync results, works across realms + const result = await Promise.resolve(command.execute(context)); + // Only increment after successful completion + this.commandIndex = nextIndex; + + this.internalEvents.emit(EditEvent.EditRedo, { command: command.name }); + this.emitEditChanged(`redo:${command.name}`); + return result; }); } /** True when there is a command to undo (the history pointer is not before the first command). */ @@ -1142,11 +1180,17 @@ export class Edit { this.emitEditChanged(`commit:${command.name}`); } - public updateClip(trackIdx: number, clipIdx: number, updates: Partial): Promise { + /** + * Merge `updates` into the clip at `(trackIdx, clipIdx)`. + * + * Resolves with the command outcome: `success` when the clip was updated, or `noop` + * (with a `message`) when there is no clip at that position. + */ + public updateClip(trackIdx: number, clipIdx: number, updates: Partial): Promise { const clip = this.getPlayerClip(trackIdx, clipIdx); if (!clip) { console.warn(`Clip not found at track ${trackIdx}, index ${clipIdx}`); - return Promise.resolve(); + return Promise.resolve(CommandNoop(`No clip at track ${trackIdx}, index ${clipIdx}`)); } const documentClip = this.document?.getClip(trackIdx, clipIdx); @@ -1193,7 +1237,7 @@ export class Edit { } /** @internal */ - public executeEditCommand(command: EditCommand): void | Promise { + public executeEditCommand(command: EditCommand): Promise { return this.executeCommand(command); } @@ -1221,12 +1265,13 @@ export class Edit { } /** @internal */ - protected executeCommand(command: EditCommand): Promise { + protected executeCommand(command: EditCommand): Promise { return this.commandQueue.enqueue(async () => { const context = this.createCommandContext(); // Always await - harmless on sync results, works across realms const result = await Promise.resolve(command.execute(context)); this.handleCommandResult(command, result); + return result; }); } @@ -2074,12 +2119,12 @@ export class Edit { // ─── Output Settings (delegated to OutputSettingsManager) ──────────────────── - public setOutputSize(width: number, height: number): Promise { + public setOutputSize(width: number, height: number): Promise { const command = new SetOutputSizeCommand(width, height); return this.executeCommand(command); } - public setOutputFps(fps: number): Promise { + public setOutputFps(fps: number): Promise { const command = new SetOutputFpsCommand(fps); return this.executeCommand(command); } @@ -2088,7 +2133,7 @@ export class Edit { return this.outputSettings.getFps(); } - public setOutputFormat(format: string): Promise { + public setOutputFormat(format: string): Promise { const command = new SetOutputFormatCommand(format); return this.executeCommand(command); } @@ -2097,7 +2142,7 @@ export class Edit { return this.outputSettings.getFormat(); } - public setOutputDestinations(destinations: Destination[]): Promise { + public setOutputDestinations(destinations: Destination[]): Promise { const command = new SetOutputDestinationsCommand(destinations); return this.executeCommand(command); } @@ -2106,7 +2151,7 @@ export class Edit { return this.outputSettings.getDestinations(); } - public setOutputResolution(resolution: string): Promise { + public setOutputResolution(resolution: string): Promise { const command = new SetOutputResolutionCommand(resolution); return this.executeCommand(command); } @@ -2115,7 +2160,7 @@ export class Edit { return this.outputSettings.getResolution(); } - public setOutputAspectRatio(aspectRatio: string): Promise { + public setOutputAspectRatio(aspectRatio: string): Promise { const command = new SetOutputAspectRatioCommand(aspectRatio); return this.executeCommand(command); } @@ -2262,7 +2307,7 @@ export class Edit { this.cleanupUnusedFonts(); } - public setTimelineBackground(color: string): Promise { + public setTimelineBackground(color: string): Promise { const command = new SetTimelineBackgroundCommand(color); return this.executeCommand(command); } diff --git a/src/core/shotstack-edit.ts b/src/core/shotstack-edit.ts index 55ece3a2..cc1f57c7 100644 --- a/src/core/shotstack-edit.ts +++ b/src/core/shotstack-edit.ts @@ -1,4 +1,5 @@ import { SetMergeFieldCommand } from "./commands/set-merge-field-command"; +import { CommandNoop, type CommandResult } from "./commands/types"; import { Edit } from "./edit-session"; import { EditEvent } from "./events/edit-events"; import { parseFontFamily } from "./fonts/font-config"; @@ -101,7 +102,7 @@ export class ShotstackEdit extends Edit { /** * Apply a merge field to a clip property. */ - public applyMergeField(clipId: string, propertyPath: string, fieldName: string, value: string, originalValue?: string): Promise { + public applyMergeField(clipId: string, propertyPath: string, fieldName: string, value: string, originalValue?: string): Promise { const resolvedClip = this.getResolvedClipById(clipId); const currentValue = resolvedClip ? getNestedValue(resolvedClip, propertyPath) : null; const previousValue = originalValue ?? (currentValue != null ? String(currentValue) : ""); @@ -118,9 +119,9 @@ export class ShotstackEdit extends Edit { /** * Remove a merge field from a clip property, restoring the original value. */ - public removeMergeField(clipId: string, propertyPath: string, restoreValue: string): Promise { + public removeMergeField(clipId: string, propertyPath: string, restoreValue: string): Promise { const currentFieldName = this.getMergeFieldForProperty(clipId, propertyPath); - if (!currentFieldName) return Promise.resolve(); + if (!currentFieldName) return Promise.resolve(CommandNoop("No merge field on this property")); const command = new SetMergeFieldCommand(clipId, propertyPath, null, currentFieldName, restoreValue, restoreValue); return this.executeCommand(command); diff --git a/src/index.ts b/src/index.ts index 6922a95c..1fe2573b 100644 --- a/src/index.ts +++ b/src/index.ts @@ -9,5 +9,6 @@ export { UIController } from "@core/ui/ui-controller"; export type { UIControllerOptions, ToolbarButtonConfig } from "@core/ui/ui-controller"; export type { EditConfig } from "@core/schemas"; +export type { CommandResult } from "@core/commands/types"; export const VERSION = pkg.version; diff --git a/tests/edit-clip-operations.test.ts b/tests/edit-clip-operations.test.ts index 27230945..5ad58f35 100644 --- a/tests/edit-clip-operations.test.ts +++ b/tests/edit-clip-operations.test.ts @@ -483,6 +483,72 @@ describe("Edit Clip Operations", () => { }); }); + describe("command results", () => { + it("deleteClip resolves noop when deleting the only clip", async () => { + // Only the initial image clip exists at this point + const result = await edit.deleteClip(0, 0); + + expect(result).toEqual({ status: "noop", message: "Cannot delete the last clip" }); + const { tracks } = getEditState(edit); + expect(tracks[0].length).toBe(1); + }); + + it("deleteClip resolves success when another clip remains", async () => { + await edit.addClip(0, createVideoClip(0, 5)); + + const result = await edit.deleteClip(0, 0); + + expect(result.status).toBe("success"); + }); + + it("deleteClip resolves noop for a missing position", async () => { + expect(await edit.deleteClip(99, 0)).toMatchObject({ status: "noop" }); + expect(await edit.deleteClip(0, 99)).toMatchObject({ status: "noop" }); + }); + + it("deleteClipById resolves noop for an unknown id", async () => { + const result = await edit.deleteClipById("not-a-real-clip-id"); + + expect(result).toEqual({ status: "noop", message: "No clip with id not-a-real-clip-id" }); + }); + + it("updateClipById resolves noop for an unknown id", async () => { + const result = await edit.updateClipById("not-a-real-clip-id", {}); + + expect(result).toMatchObject({ status: "noop" }); + }); + + it("addClip resolves success", async () => { + const result = await edit.addClip(0, createVideoClip(0, 5)); + + expect(result.status).toBe("success"); + }); + + it("deleteTrack resolves noop when deleting the last track", async () => { + const result = await edit.deleteTrack(0); + + expect(result).toEqual({ status: "noop", message: "Cannot delete the last track" }); + }); + + it("undo resolves noop when the history is empty", async () => { + // load() builds the initial timeline without going through the command queue + expect(await edit.undo()).toEqual({ status: "noop", message: "Nothing to undo" }); + }); + + it("undo resolves the undone command's outcome", async () => { + await edit.addClip(0, createVideoClip(0, 5)); + + expect((await edit.undo()).status).toBe("success"); + expect(await edit.undo()).toEqual({ status: "noop", message: "Nothing to undo" }); + }); + + it("redo resolves noop when there is nothing to redo", async () => { + const result = await edit.redo(); + + expect(result).toEqual({ status: "noop", message: "Nothing to redo" }); + }); + }); + describe("updateClip()", () => { beforeEach(async () => { await edit.addClip(0, createTextClip(0, 5, "Original")); From 4ad4ec78384b687e9b4797532351c7d1780ae84f Mon Sep 17 00:00:00 2001 From: dazzatronus Date: Fri, 3 Jul 2026 18:14:50 +1000 Subject: [PATCH 2/3] fix: refuse luma-attached clip deletion atomically at the last-clip boundary --- src/core/edit-session.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/core/edit-session.ts b/src/core/edit-session.ts index 3b7a4e86..b880e689 100644 --- a/src/core/edit-session.ts +++ b/src/core/edit-session.ts @@ -942,6 +942,12 @@ export class Edit { const lumaIndex = track.findIndex(clip => clip.playerType === PlayerType.Luma); if (lumaIndex !== -1) { + // Refuse atomically up front: once the luma is gone the content deletion below + // would hit the last-clip rule, and a refusal must not half-apply by deleting the luma + if (this.document.getClipCount() <= 2) { + return CommandNoop("Cannot delete the last clip"); + } + // Delete luma first (handles index shifting correctly) // If luma comes before content clip, content clip index shifts after luma deletion const adjustedContentIdx = lumaIndex < clipIdx ? clipIdx - 1 : clipIdx; From eea1b80e23b46aeca784f9b465f369fe6921297c Mon Sep 17 00:00:00 2001 From: dazzatronus Date: Fri, 3 Jul 2026 18:14:50 +1000 Subject: [PATCH 3/3] test: assert command outcomes across mutators, undo/redo and merge fields --- tests/edit-clip-operations.test.ts | 65 ++++++++++++++++++++++++++++++ tests/edit-merge-fields.test.ts | 15 +++++++ 2 files changed, 80 insertions(+) diff --git a/tests/edit-clip-operations.test.ts b/tests/edit-clip-operations.test.ts index 5ad58f35..7d926dbc 100644 --- a/tests/edit-clip-operations.test.ts +++ b/tests/edit-clip-operations.test.ts @@ -547,6 +547,71 @@ describe("Edit Clip Operations", () => { expect(result).toEqual({ status: "noop", message: "Nothing to redo" }); }); + + it("redo resolves the re-applied command's outcome", async () => { + await edit.addClip(0, createVideoClip(0, 5)); + await edit.undo(); + + expect((await edit.redo()).status).toBe("success"); + expect(await edit.redo()).toEqual({ status: "noop", message: "Nothing to redo" }); + }); + + it("moveClipById resolves noop for an unknown id", async () => { + const result = await edit.moveClipById("not-a-real-clip-id", 0); + + expect(result).toEqual({ status: "noop", message: "No clip with id not-a-real-clip-id" }); + }); + + it("deleteClipById resolves success for a real clip", async () => { + await edit.addClip(0, createVideoClip(0, 5)); + const doc = (edit as unknown as { document: { getClipId(t: number, c: number): string | null } }).document; + const id = doc.getClipId(0, 1); + expect(id).toBeTruthy(); + + expect((await edit.deleteClipById(id as string)).status).toBe("success"); + }); + + it("updateClip resolves success and noop by position", async () => { + expect((await edit.updateClip(0, 0, { fit: "contain" })).status).toBe("success"); + expect(await edit.updateClip(0, 99, {})).toMatchObject({ status: "noop" }); + }); + + it("deleteTrack resolves success when another track remains", async () => { + await edit.addTrack(1, { clips: [createVideoClip(0, 5)] }); + + expect((await edit.deleteTrack(1)).status).toBe("success"); + }); + + it("setOutputSize resolves a command outcome", async () => { + expect((await edit.setOutputSize(1280, 720)).status).toBe("success"); + }); + + it("deleteClip returns the requested clip's outcome when a luma matte shares the track", async () => { + // Track 0: [image (initial), luma, video] + await edit.addClip(0, { asset: { type: "luma", src: "https://example.com/matte.mp4" }, start: 0, length: 5 }); + await edit.addClip(0, createVideoClip(0, 5)); + + // Deleting the video (a content clip) removes the track's luma first, then the video — + // the resolved outcome is the video's + const result = await edit.deleteClip(0, 2); + + expect(result.status).toBe("success"); + const { tracks } = getEditState(edit); + expect(tracks[0].length).toBe(1); + }); + + it("deleteClip refuses atomically when the content clip with a luma is the last one", async () => { + // Track 0: [image (initial), luma] — the image is the last content clip + await edit.addClip(0, { asset: { type: "luma", src: "https://example.com/matte.mp4" }, start: 0, length: 5 }); + + // The deletion is refused up front: the luma must NOT be deleted as a side + // effect of a refused operation + const result = await edit.deleteClip(0, 0); + + expect(result).toEqual({ status: "noop", message: "Cannot delete the last clip" }); + const { tracks } = getEditState(edit); + expect(tracks[0].length).toBe(2); + }); }); describe("updateClip()", () => { diff --git a/tests/edit-merge-fields.test.ts b/tests/edit-merge-fields.test.ts index fe01a419..526009a7 100644 --- a/tests/edit-merge-fields.test.ts +++ b/tests/edit-merge-fields.test.ts @@ -338,6 +338,21 @@ describe("Edit Merge Fields", () => { }); describe("applyMergeField()", () => { + it("resolves a command outcome, and removeMergeField resolves noop when no field exists", async () => { + const clip = createImageClip(0, 3); + await edit.addClip(0, clip); + const clipId = getClipIdOrFail(edit, 0, 0); + + // No merge field on the property yet — removal reports it did nothing + expect(await edit.removeMergeField(clipId, "asset.src", "restore")).toEqual({ + status: "noop", + message: "No merge field on this property" + }); + + const applied = await edit.applyMergeField(clipId, "asset.src", "MEDIA_URL", "https://cdn.example.com/new.jpg"); + expect(applied.status).toBe("success"); + }); + it("stores {{ FIELD }} template in document bindings", async () => { // Add a clip first const clip = createImageClip(0, 3);