feat: add ffmpeg-edit task for PATCH /api/content#128
Merged
sweetmantech merged 13 commits intomainfrom Apr 9, 2026
Merged
Conversation
Adds the Trigger.dev task that the API's PATCH /api/content endpoint triggers. Accepts video or audio input with an array of edit operations (trim, crop, resize, overlay_text, mux_audio) and processes them sequentially via ffmpeg. Uploads result to fal.ai storage. - createRenderPayloadSchema: Zod schema matching the API's edit body - createRenderTask: schemaTask with ffmpeg processing pipeline - 12 tests covering schema validation and task configuration All 296 tests pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a modular FFmpeg-based video editing pipeline: builds ffmpeg args, runs ffmpeg, downloads/uploads media, initializes Fal client, and validates edit payloads with Zod. Introduces a Trigger.dev task that orchestrates these steps and returns uploaded output metadata. Changes
Sequence DiagramsequenceDiagram
participant Task as Trigger.dev Task
participant Input as Remote URL
participant FS as File System
participant FFmpeg as FFmpeg
participant Fal as Fal Storage
Task->>Input: fetch media URL
Input-->>Task: media data
Task->>FS: write temp input file
Task->>Task: build ffmpeg args (operations)
Task->>FFmpeg: run ffmpeg with args (input -> output)
FFmpeg->>FS: read input / write output
FFmpeg-->>Task: return success
Task->>Fal: upload output file
Fal-->>Task: { url, mimeType, sizeBytes }
Task->>FS: delete temp files
Task-->>Caller: return { status, url, mimeType, sizeBytes }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/schemas/createRenderSchema.ts`:
- Around line 9-19: The crop/resize schemas allow invalid or no-op ops: tighten
the zod schemas in createRenderSchema.ts by (1) validating aspect (the aspect
field in the type: "crop" object) against an expected format (e.g. "W:H" numeric
pair) using a string refinement, (2) adding a refinement on the crop variant
(type: "crop") to require either a valid aspect OR both width and height present
and positive, and (3) adding a refinement on the resize variant (type: "resize")
to require at least one of width or height present and positive; update any
error messages to be clear so buildFfmpegArgs will never receive
missing/negative dimensions or produce scale=-1:-1.
- Around line 36-42: The createRenderPayloadSchema currently allows zero or both
of video_url and audio_url; update createRenderPayloadSchema to enforce exactly
one input URL by adding a schema-level refinement (e.g., using .refine or
.superRefine) that checks that exactly one of the two optional fields
(video_url, audio_url) is present and non-empty, and return a clear validation
error message when the check fails; keep the existing field validators for
video_url and audio_url and reference createRenderPayloadSchema, video_url, and
audio_url when making the change.
In `@src/tasks/__tests__/createRenderTask.test.ts`:
- Around line 111-118: The test name "rejects empty operations array" mismatches
the assertion for createRenderPayloadSchema.safeParse
(expect(result.success).toBe(true)); either rename the test to reflect that an
empty operations array is valid (e.g., "accepts empty operations array") or
invert the assertion to expect(false) so it truly asserts rejection; update the
spec title referencing createRenderPayloadSchema and the test block name passed
to it() accordingly to keep behavior and intent aligned.
In `@src/tasks/createRenderTask.ts`:
- Around line 133-145: The overlay_text case in createRenderTask.ts builds the
drawtext filter but ignores the op.font field declared in the schema
(createRenderSchema.ts); update the filter construction in the "overlay_text"
branch to include the font by adding a fontfile=${op.font} entry (or
conditionally include it only when op.font is provided) to the filter array so
the drawtext filter uses the requested font; ensure you reference op.font (and
escape it if needed) and keep the rest of the drawtext entries intact.
- Around line 148-166: In the mux_audio case fix the FFmpeg argument ordering
and mapping: ensure all input flags added to extraInputs (including
op.audio_url) are pushed into args before output-side options like -vf and
mappings (reorder where args.push(...extraInputs) occurs relative to
args.push("-vf", ...)); and when op.replace is false, change the amix usage to
produce a labeled filter output and map that label (e.g., build a filter like
combining [0:a][1:a]amix=inputs=2 into a label and then push a -map for that
label) so audioMapping maps the mixed output rather than the original tracks
(update how audioMapping is constructed in the mux_audio branch accordingly,
referencing mux_audio case, extraInputs, audioMapping, videoFilters and args).
- Around line 44-45: The task builds a video-centric pipeline unconditionally
(uses inputPath/outputPath, maps 0:v:0, forces -c:v libx264/-pix_fmt/+faststart
and returns video/*), which breaks audio-only payloads (payload.audio_url);
update createRenderTask to detect audio-only payloads (presence of
payload.audio_url and absence of video sources) and branch: when audio-only, set
outputPath/extension and MIME to an audio type, remove video mapping and video
codec/pix_fmt/faststart options, map the audio stream(s) only (e.g., 0:a:0), and
set appropriate ffmpeg args for audio encoding; when video is present, keep the
existing video pipeline behavior. Ensure any metadata returned reflects audio
MIME/type when audio-only.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: be9df658-83bd-4a0b-a72c-972c35e2e363
📒 Files selected for processing (3)
src/schemas/createRenderSchema.tssrc/tasks/__tests__/createRenderTask.test.tssrc/tasks/createRenderTask.ts
Comment on lines
+9
to
+19
| z.object({ | ||
| type: z.literal("crop"), | ||
| aspect: z.string().optional(), | ||
| width: z.number().int().positive().optional(), | ||
| height: z.number().int().positive().optional(), | ||
| }), | ||
| z.object({ | ||
| type: z.literal("resize"), | ||
| width: z.number().int().positive().optional(), | ||
| height: z.number().int().positive().optional(), | ||
| }), |
There was a problem hiding this comment.
Reject incomplete crop/resize operations up front.
aspect accepts any string, crop can arrive with no usable dimensions, and resize allows both dimensions to be omitted. Those payloads either no-op or generate invalid filters like scale=-1:-1 in buildFfmpegArgs. Tighten this with schema refinements: validate aspect format, require aspect or both crop dimensions, and require at least one resize dimension.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/schemas/createRenderSchema.ts` around lines 9 - 19, The crop/resize
schemas allow invalid or no-op ops: tighten the zod schemas in
createRenderSchema.ts by (1) validating aspect (the aspect field in the type:
"crop" object) against an expected format (e.g. "W:H" numeric pair) using a
string refinement, (2) adding a refinement on the crop variant (type: "crop") to
require either a valid aspect OR both width and height present and positive, and
(3) adding a refinement on the resize variant (type: "resize") to require at
least one of width or height present and positive; update any error messages to
be clear so buildFfmpegArgs will never receive missing/negative dimensions or
produce scale=-1:-1.
src/tasks/createRenderTask.ts
Outdated
Comment on lines
+133
to
+145
| case "overlay_text": { | ||
| const escaped = op.content.replace(/'/g, "'\\''").replace(/:/g, "\\:"); | ||
| const filter = [ | ||
| `drawtext=text='${escaped}'`, | ||
| `fontsize=${op.max_font_size}`, | ||
| `fontcolor=${op.color}`, | ||
| `borderw=${Math.max(2, Math.round(op.max_font_size / 14))}`, | ||
| `bordercolor=${op.stroke_color}`, | ||
| "x=(w-tw)/2", | ||
| op.position === "top" ? "y=180" : op.position === "center" ? "y=(h-th)/2" : `y=h-th-120`, | ||
| ].join(":"); | ||
| videoFilters.push(filter); | ||
| break; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the createRenderTask.ts file
fd -t f "createRenderTask.ts"Repository: recoupable/tasks
Length of output: 89
🏁 Script executed:
# Search for createRenderSchema definition
rg "createRenderSchema" --type ts -B 2 -A 20Repository: recoupable/tasks
Length of output: 2982
🏁 Script executed:
# Look for font-related code in createRenderTask.ts
rg "font" "src/tasks/createRenderTask.ts" -B 2 -A 2Repository: recoupable/tasks
Length of output: 340
🏁 Script executed:
# Find and read the createRenderSchema file
fd -t f "createRenderSchema*" src/Repository: recoupable/tasks
Length of output: 93
🏁 Script executed:
# Read the createRenderSchema file
cat -n "src/schemas/createRenderSchema.ts"Repository: recoupable/tasks
Length of output: 1820
🏁 Script executed:
# Read more context from createRenderTask.ts around the overlay_text case
sed -n '120,160p' "src/tasks/createRenderTask.ts"Repository: recoupable/tasks
Length of output: 1358
Wire the font field into the drawtext filter or remove it from the schema.
The overlay_text operation exposes a font option in the schema (line 23 of createRenderSchema.ts), but the drawtext filter built in the implementation never includes it. Callers can send a font and get silently default-styled output. Either add fontfile=${op.font} to the filter array or remove the field from the public schema.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tasks/createRenderTask.ts` around lines 133 - 145, The overlay_text case
in createRenderTask.ts builds the drawtext filter but ignores the op.font field
declared in the schema (createRenderSchema.ts); update the filter construction
in the "overlay_text" branch to include the font by adding a fontfile=${op.font}
entry (or conditionally include it only when op.font is provided) to the filter
array so the drawtext filter uses the requested font; ensure you reference
op.font (and escape it if needed) and keep the rest of the drawtext entries
intact.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/tasks/createRenderTask.ts">
<violation number="1" location="src/tasks/createRenderTask.ts:96">
P1: Custom agent: **Flag AI Slop and Fabricated Changes**
The `buildFfmpegArgs` function contains the core logic of this PR (5 operation types, ~60 lines of ffmpeg argument construction including aspect-ratio math, text escaping, and audio mapping) but is not exported and has zero test coverage. All 12 tests only check schema parsing and static task config properties — none exercise the actual behavior this PR adds. Export this function and add unit tests for each operation type, especially the crop aspect-ratio branch, the `overlay_text` escaping, and the `mux_audio` replace vs. mix paths.</violation>
<violation number="2" location="src/tasks/createRenderTask.ts:153">
P1: The `mux_audio` with `replace: false` generates incorrect ffmpeg arguments. The `amix` filter output is never mapped — the subsequent `-map 0:a -map 1:a` references the original unmixed streams, so the audio mixing never actually happens. Also, if any video filters are present, the combination of `-vf` and `-filter_complex` will cause ffmpeg to error.
The correct approach labels the filter inputs/outputs and maps only the mixed result.</violation>
</file>
<file name="src/schemas/createRenderSchema.ts">
<violation number="1" location="src/schemas/createRenderSchema.ts:3">
P0: `z.discriminatedUnion()` does not exist in Zod v4 (this project uses zod@4.3.6). This will throw a `TypeError` at runtime. In Zod v4, use `z.union()` instead — it automatically detects discriminated unions by shared literal fields.</violation>
<violation number="2" location="src/schemas/createRenderSchema.ts:11">
P2: All fields in the `crop` operation (`aspect`, `width`, `height`) are optional, so `{ type: "crop" }` passes validation despite being semantically invalid — ffmpeg's `crop=` filter needs at least a dimension or aspect ratio. Consider adding a `.refine()` to require at least one of these fields. The same issue applies to the `resize` operation below.</violation>
</file>
<file name="src/tasks/__tests__/createRenderTask.test.ts">
<violation number="1" location="src/tasks/__tests__/createRenderTask.test.ts:111">
P2: Test name says "rejects empty operations array" but the assertion expects `result.success` to be `true`. Rename to accurately describe the behavior, e.g. `"accepts empty operations array"`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
…nd stripEmoji - Extract ffmpeg arg builder from task into content/buildRenderFfmpegArgs.ts (SRP) - Reuse escapeDrawtext for text escaping (was reimplemented inline) - Reuse stripEmoji for cleaning overlay text (was missing) - Task file now only handles download → delegate → upload (no ffmpeg logic) - 12 new tests for buildRenderFfmpegArgs covering all operation types - Zero changes to existing pipeline code All 308 tests pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Extract 3 shared utilities from renderFinalVideo so both the existing content pipeline and the new create-render task use the same code: - downloadMediaToFile(url, path) — fetch + write to disk - runFfmpeg(args) — execFile wrapper - uploadToFalStorage(path, name, mime) — read + upload to fal.ai renderFinalVideo now composes these instead of inlining the logic. createRenderTask uses the same primitives. Existing pipeline behavior is unchanged — same inputs, same outputs. All 308 tests pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
3 issues found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/content/buildRenderFfmpegArgs.ts">
<violation number="1" location="src/content/buildRenderFfmpegArgs.ts:51">
P2: When both `width` and `height` are omitted (both are optional in the schema), this produces `scale=-1:-1` which is invalid — ffmpeg requires at least one concrete dimension when using `-1`. Add a guard or add a `.refine()` to the schema requiring at least one dimension.</violation>
<violation number="2" location="src/content/buildRenderFfmpegArgs.ts:67">
P2: The `color` and `stroke_color` values are interpolated directly into the colon-delimited drawtext filter string without sanitization. A value containing `:` (e.g., `"white:enable=0"`) would inject additional ffmpeg drawtext parameters. Either validate color format in the schema (e.g., with a regex) or escape colons in these values.</violation>
</file>
<file name="src/tasks/createRenderTask.ts">
<violation number="1" location="src/tasks/createRenderTask.ts:11">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**
Unused imports `escapeDrawtext` and `stripEmoji` — these are only needed inside `buildRenderFfmpegArgs`, which already imports them. Leftover dead imports from the extraction refactor.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/content/__tests__/buildRenderFfmpegArgs.test.ts (1)
113-144: Add a regression test forreplace: false+ filter/input ordering.Line 113 onward only asserts
replace: true. Please add a case that assertsreplace: falseproduces a valid mixed-audio mapping, and a case combining video filters +mux_audiothat verifies all-iinputs appear before output options (like-vf), so ffmpeg argument-order regressions are caught early.✅ Suggested test additions
+ it("builds mux_audio with replace=false using mixed output mapping", () => { + const args = buildRenderFfmpegArgs("in.mp4", "out.mp4", [ + { type: "mux_audio", audio_url: "https://example.com/song.mp3", replace: false }, + ]); + expect(args).toContain("-filter_complex"); + const fcIndex = args.indexOf("-filter_complex"); + expect(args[fcIndex + 1]).toContain("amix=inputs=2"); + const mapIndices = args.reduce((acc: number[], v, i) => (v === "-map" ? [...acc, i] : acc), []); + expect(mapIndices.length).toBeGreaterThanOrEqual(2); + }); + + it("places extra -i inputs before output options when filters are present", () => { + const args = buildRenderFfmpegArgs("in.mp4", "out.mp4", [ + { type: "crop", aspect: "9:16" }, + { type: "mux_audio", audio_url: "https://example.com/song.mp3", replace: true }, + ]); + const lastInputIndex = args.lastIndexOf("-i"); + const vfIndex = args.indexOf("-vf"); + expect(lastInputIndex).toBeGreaterThan(-1); + expect(vfIndex).toBeGreaterThan(-1); + expect(lastInputIndex).toBeLessThan(vfIndex); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/content/__tests__/buildRenderFfmpegArgs.test.ts` around lines 113 - 144, Add two assertions to the buildRenderFfmpegArgs tests: first, add a case calling buildRenderFfmpegArgs with a mux_audio operation where replace: false and assert the mixed-audio mapping is produced (i.e., the original audio input remains mapped and the new audio is appended, checking the positions of "-map" entries similar to the existing replace=true test but expecting original audio map like "0:a:0" and new audio like "1:a:0"); second, add a combined test that includes video filters (e.g., crop and overlay_text) plus a mux_audio and assert that all input URLs/“-i” entries (the audio input string) appear in args before output options like "-vf" (use args.indexOf to compare positions for the audio input and "-vf"), ensuring ffmpeg input ordering is preserved when replace is false.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/content/buildRenderFfmpegArgs.ts`:
- Around line 77-94: The mux_audio branch uses incorrect amix syntax and places
-vf before all inputs; update the "mux_audio" case in buildRenderFfmpegArgs.ts
so that when op.replace is false you build a labeled filter_complex like
"[0:a][1:a]amix=inputs=2[aout]" and map the mixed audio with "-map", "[aout]"
(instead of mapping "0:a" and "1:a" directly), while keeping the replace=true
behavior mapping video and the single audio input ("-map", "0:v:0", "-map",
"1:a:0"). Also change argument ordering so extraInputs (the "-i", op.audio_url"
entries) are added before applying output options like "-vf" (move the
videoFilters/args.push("-vf", ...) after you push extraInputs and audioMapping)
to ensure FFmpeg input-bound options are correctly ordered.
- Around line 39-44: The current crop logic in the "crop" case uses op.aspect to
build a crop string that can produce dimensions larger than the source (e.g.,
`crop=iw:iw*${h}/${w}`) and fail; update the logic in buildRenderFfmpegArgs so
that after parsing op.aspect into w and h you compute the targetAspect = w/h and
sourceAspect = sourceWidth/sourceHeight (or compare using iw and ih
algebraically) and then choose the crop formula based on whether sourceAspect >
targetAspect or not: if source is wider than target, crop width to match target
using width = ih * targetAspect and height = ih (crop by width); otherwise crop
height using width = iw and height = iw / targetAspect (crop by height); push
the resulting safe crop filter string to videoFilters and ensure you only push
when w and h are valid numbers.
---
Nitpick comments:
In `@src/content/__tests__/buildRenderFfmpegArgs.test.ts`:
- Around line 113-144: Add two assertions to the buildRenderFfmpegArgs tests:
first, add a case calling buildRenderFfmpegArgs with a mux_audio operation where
replace: false and assert the mixed-audio mapping is produced (i.e., the
original audio input remains mapped and the new audio is appended, checking the
positions of "-map" entries similar to the existing replace=true test but
expecting original audio map like "0:a:0" and new audio like "1:a:0"); second,
add a combined test that includes video filters (e.g., crop and overlay_text)
plus a mux_audio and assert that all input URLs/“-i” entries (the audio input
string) appear in args before output options like "-vf" (use args.indexOf to
compare positions for the audio input and "-vf"), ensuring ffmpeg input ordering
is preserved when replace is false.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4bde0407-01e9-4f7e-bf20-1be2ffe60365
📒 Files selected for processing (3)
src/content/__tests__/buildRenderFfmpegArgs.test.tssrc/content/buildRenderFfmpegArgs.tssrc/tasks/createRenderTask.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tasks/createRenderTask.ts
downloadMediaToFile now delegates to the existing downloadImageBuffer for fetch + error handling, instead of reimplementing the same logic. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
sweetmantech
commented
Apr 9, 2026
src/tasks/createRenderTask.ts
Outdated
|
|
||
| const falKey = process.env.FAL_KEY; | ||
| if (!falKey) throw new Error("FAL_KEY environment variable is required"); | ||
| fal.config({ credentials: falKey }); |
Contributor
Author
There was a problem hiding this comment.
We need a server config file for fal similar to what we have in the API codebase.
https://github.com/recoupable/api/pull/390/changes#diff-44a25f14cfa1d5d3f40a563c2a803ed9bb6f4ae1f6cbde820fd4da6500ee3ed9
There was a problem hiding this comment.
2 issues found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/content/runFfmpeg.ts">
<violation number="1" location="src/content/runFfmpeg.ts:13">
P1: ffmpeg stderr output can exceed the default 1 MB `maxBuffer`, killing the process with `ERR_CHILD_PROCESS_STDIO_MAXBUFFER`. Increase the limit to accommodate verbose ffmpeg output on long or complex renders.</violation>
</file>
<file name="src/content/downloadMediaToFile.ts">
<violation number="1" location="src/content/downloadMediaToFile.ts:12">
P2: Downloading large video/audio files entirely into an in-memory `Buffer` before writing to disk risks high memory usage or OOM in the task worker. Consider streaming the response body directly to the file using `node:stream/promises` `pipeline` with `fs.createWriteStream`, which avoids holding the full file in memory.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| * @param filePath - Local path to write the downloaded file. | ||
| */ | ||
| export async function downloadMediaToFile(url: string, filePath: string): Promise<void> { | ||
| const { buffer } = await downloadImageBuffer(url); |
There was a problem hiding this comment.
P2: Downloading large video/audio files entirely into an in-memory Buffer before writing to disk risks high memory usage or OOM in the task worker. Consider streaming the response body directly to the file using node:stream/promises pipeline with fs.createWriteStream, which avoids holding the full file in memory.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/content/downloadMediaToFile.ts, line 12:
<comment>Downloading large video/audio files entirely into an in-memory `Buffer` before writing to disk risks high memory usage or OOM in the task worker. Consider streaming the response body directly to the file using `node:stream/promises` `pipeline` with `fs.createWriteStream`, which avoids holding the full file in memory.</comment>
<file context>
@@ -0,0 +1,14 @@
+ * @param filePath - Local path to write the downloaded file.
+ */
+export async function downloadMediaToFile(url: string, filePath: string): Promise<void> {
+ const { buffer } = await downloadImageBuffer(url);
+ await writeFile(filePath, buffer);
+}
</file context>
1. Replace z.discriminatedUnion with z.union (Zod v4 compatibility) 2. Add .refine() to crop/resize requiring at least one dimension 3. Validate color values with regex (prevent ffmpeg injection) 4. Fix mux_audio replace:false — use labeled amix filter outputs 5. Add falServer.ts config (matches API's lib/fal/server.ts pattern) 6. Remove unused imports from createRenderTask 7. Fix test name "rejects" → "accepts" for empty operations 8. Add tests for crop/resize validation and mux_audio mixing 9. uploadToFalStorage uses falServer instead of raw @fal-ai/client All 312 tests pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/schemas/createRenderSchema.ts">
<violation number="1" location="src/schemas/createRenderSchema.ts:28">
P2: Color regex accepts invalid 5- and 7-digit hex values (e.g. `#12345`, `#1234567`). Restrict to the four valid CSS hex lengths: 3, 4, 6, or 8 digits.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
Template operations are incomplete by design — overlay_text has styling but no content (filled dynamically), mux_audio has replace flag but no audio_url (uses top-level audio_url from request). Changes: - Make overlay_text.content optional in schema, skip if missing - Make mux_audio.audio_url optional in schema, fall back to payload audio_url - buildRenderFfmpegArgs accepts fallbackAudioUrl parameter - 3 new tests for template operation handling All 315 tests pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Extra -i inputs (mux_audio URL) must come before -vf filters, otherwise ffmpeg tries to apply the video filter to the audio input. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This reverts commit 2ffb111.
Bug: when crop + mux_audio were combined, -vf was placed before the audio -i input, causing ffmpeg to error with "Option vf cannot be applied to input url". Test reproduces the exact failure from Trigger.dev run run_cmns08hb901730hk6s9vskc4a. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/schemas/createRenderSchema.ts (1)
40-46:⚠️ Potential issue | 🟠 MajorSchema allows both or neither input URLs.
The schema accepts payloads with both
video_urlandaudio_url(silently preferringvideo_urlat runtime) or neither (causing a runtime error). Adding a refinement ensures validation fails early with a clear message.🛡️ Proposed fix: enforce exactly one input URL
-export const createRenderPayloadSchema = z.object({ +export const createRenderPayloadSchema = z.object({ accountId: z.string().min(1, "accountId is required"), video_url: z.string().url().optional(), audio_url: z.string().url().optional(), operations: z.array(editOperationSchema), output_format: z.enum(["mp4", "webm", "mov"]).default("mp4"), -}); +}).refine( + ({ video_url, audio_url }) => Boolean(video_url) !== Boolean(audio_url), + { message: "Provide exactly one of video_url or audio_url", path: ["video_url"] } +);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/schemas/createRenderSchema.ts` around lines 40 - 46, The createRenderPayloadSchema currently allows both or neither of video_url and audio_url; add a zod refinement on createRenderPayloadSchema to require exactly one of video_url or audio_url is present and non-empty so validation fails early with a clear message. Locate createRenderPayloadSchema and attach a .refine(...) that checks (Boolean(video_url) !== Boolean(audio_url)) and returns a custom error message like "exactly one of video_url or audio_url must be provided" so payloads with both or neither are rejected.src/content/buildRenderFfmpegArgs.ts (2)
93-101:⚠️ Potential issue | 🟠 MajorFFmpeg argument ordering:
-vfplaced before audio input-i.Output options like
-vfmust come after all input declarations. Currently,-vfis pushed at line 94, thenextraInputs(containing-i audioUrl) is pushed at line 97. This places-vfbefore the second-i, which can cause ffmpeg errors or unexpected behavior.🐛 Proposed fix: reorder to place inputs before output options
- if (videoFilters.length > 0) { - args.push("-vf", videoFilters.join(",")); - } - args.push(...extraInputs); + if (videoFilters.length > 0) { + args.push("-vf", videoFilters.join(",")); + } + if (audioMapping.length > 0) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/content/buildRenderFfmpegArgs.ts` around lines 93 - 101, The ffmpeg "-vf" output option is being pushed before additional input declarations, which can break ffmpeg; in the buildRenderFfmpegArgs flow move the blocks that push extraInputs and audioMapping (args.push(...extraInputs) and args.push(...audioMapping)) so they occur before the videoFilters handling, and then push the "-vf" (videoFilters.join(",")) after those input-related pushes; update the code around the videoFilters / extraInputs / audioMapping logic (references: videoFilters, extraInputs, audioMapping, args) so all "-i" inputs are added to args prior to any output options like "-vf".
40-48:⚠️ Potential issue | 🔴 CriticalCrop aspect formula can produce dimensions exceeding source bounds.
The formula on line 44 doesn't account for source aspect ratio. For example, a 1920×1080 landscape source with a 9:16 portrait aspect produces
crop=1920:3413(since 1920×16/9≈3413), which exceeds the 1080 source height and causes ffmpeg to fail.The fix must condition on whether the source is wider or taller than the target aspect:
🐛 Proposed fix using ffmpeg conditional expressions
case "crop": if (op.aspect) { const [w, h] = op.aspect.split(":").map(Number); if (w && h) { - videoFilters.push(w > h ? `crop=ih*${w}/${h}:ih` : `crop=iw:iw*${h}/${w}`); + const ratio = `${w}/${h}`; + // Use conditional: if source is wider than target, limit by height; else limit by width + videoFilters.push( + `crop=w='if(gte(iw/ih,${ratio}),ih*${ratio},iw)':h='if(gte(iw/ih,${ratio}),ih,iw/(${ratio}))'` + ); } } else if (op.width || op.height) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/content/buildRenderFfmpegArgs.ts` around lines 40 - 48, The crop aspect handling in buildRenderFfmpegArgs (inside the "crop" case where op.aspect is parsed into [w,h] and videoFilters is pushed) can produce crop dimensions that exceed the source; replace the current w>h ternary expression with a conditional that compares the source aspect (iw/ih) to the target aspect (w/h) and selects a crop that is bounded by the source dimensions (e.g., use an ffmpeg conditional expression or calculate width/height so neither exceeds iw or ih). Ensure you still parse op.aspect into w and h, but build the crop filter using a conditional based on iw/ih vs w/h so the resulting crop width and height never exceed the source frame.
🧹 Nitpick comments (2)
src/schemas/createRenderSchema.ts (1)
10-11: Consider validatingaspectformat.The
aspectfield accepts any string, butbuildRenderFfmpegArgsexpects the"W:H"format (e.g.,"9:16"). Invalid formats like"invalid"or"16-9"would cause silent failures or NaN in the ffmpeg filter.♻️ Proposed: add regex validation
z.object({ type: z.literal("crop"), - aspect: z.string().optional(), + aspect: z.string().regex(/^\d+:\d+$/, "aspect must be in W:H format (e.g., '9:16')").optional(), width: z.number().int().positive().optional(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/schemas/createRenderSchema.ts` around lines 10 - 11, The aspect field in the createRenderSchema (where type: z.literal("crop") and aspect: z.string().optional()) currently allows any string but buildRenderFfmpegArgs expects "W:H" (e.g., "9:16"); update the schema to validate aspect with a regex that enforces two positive integers separated by a colon (e.g., /^\d+:\d+$/) and keep it optional (use aspect: z.string().regex(...).optional()) so invalid values like "invalid" or "16-9" are rejected at validation time.src/content/runFfmpeg.ts (1)
12-14: Consider adding timeout and buffer configuration for robustness.
execFileuses a defaultmaxBufferof ~1MB, which verbose ffmpeg output can exceed, causingENOBUFSerrors. Long-running renders also risk hanging indefinitely. These are optional improvements for production reliability.♻️ Optional: Add timeout and increased buffer
export async function runFfmpeg(args: string[]): Promise<void> { - await execFileAsync("ffmpeg", args); + await execFileAsync("ffmpeg", args, { + maxBuffer: 10 * 1024 * 1024, // 10MB for verbose output + timeout: 10 * 60 * 1000, // 10 min safety timeout + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/content/runFfmpeg.ts` around lines 12 - 14, The runFfmpeg function can overflow the default execFile buffer or hang; update runFfmpeg to accept optional exec options (e.g., timeout and maxBuffer) and pass them into execFileAsync so callers can increase maxBuffer (e.g., 10MB+) and set a sensible timeout. Specifically, modify the runFfmpeg signature (runFfmpeg) to accept an optional options parameter and forward that object to execFileAsync("ffmpeg", args, options) so you can configure timeout and maxBuffer when invoking FFmpeg.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/content/buildRenderFfmpegArgs.ts`:
- Around line 93-101: The ffmpeg "-vf" output option is being pushed before
additional input declarations, which can break ffmpeg; in the
buildRenderFfmpegArgs flow move the blocks that push extraInputs and
audioMapping (args.push(...extraInputs) and args.push(...audioMapping)) so they
occur before the videoFilters handling, and then push the "-vf"
(videoFilters.join(",")) after those input-related pushes; update the code
around the videoFilters / extraInputs / audioMapping logic (references:
videoFilters, extraInputs, audioMapping, args) so all "-i" inputs are added to
args prior to any output options like "-vf".
- Around line 40-48: The crop aspect handling in buildRenderFfmpegArgs (inside
the "crop" case where op.aspect is parsed into [w,h] and videoFilters is pushed)
can produce crop dimensions that exceed the source; replace the current w>h
ternary expression with a conditional that compares the source aspect (iw/ih) to
the target aspect (w/h) and selects a crop that is bounded by the source
dimensions (e.g., use an ffmpeg conditional expression or calculate width/height
so neither exceeds iw or ih). Ensure you still parse op.aspect into w and h, but
build the crop filter using a conditional based on iw/ih vs w/h so the resulting
crop width and height never exceed the source frame.
In `@src/schemas/createRenderSchema.ts`:
- Around line 40-46: The createRenderPayloadSchema currently allows both or
neither of video_url and audio_url; add a zod refinement on
createRenderPayloadSchema to require exactly one of video_url or audio_url is
present and non-empty so validation fails early with a clear message. Locate
createRenderPayloadSchema and attach a .refine(...) that checks
(Boolean(video_url) !== Boolean(audio_url)) and returns a custom error message
like "exactly one of video_url or audio_url must be provided" so payloads with
both or neither are rejected.
---
Nitpick comments:
In `@src/content/runFfmpeg.ts`:
- Around line 12-14: The runFfmpeg function can overflow the default execFile
buffer or hang; update runFfmpeg to accept optional exec options (e.g., timeout
and maxBuffer) and pass them into execFileAsync so callers can increase
maxBuffer (e.g., 10MB+) and set a sensible timeout. Specifically, modify the
runFfmpeg signature (runFfmpeg) to accept an optional options parameter and
forward that object to execFileAsync("ffmpeg", args, options) so you can
configure timeout and maxBuffer when invoking FFmpeg.
In `@src/schemas/createRenderSchema.ts`:
- Around line 10-11: The aspect field in the createRenderSchema (where type:
z.literal("crop") and aspect: z.string().optional()) currently allows any string
but buildRenderFfmpegArgs expects "W:H" (e.g., "9:16"); update the schema to
validate aspect with a regex that enforces two positive integers separated by a
colon (e.g., /^\d+:\d+$/) and keep it optional (use aspect:
z.string().regex(...).optional()) so invalid values like "invalid" or "16-9" are
rejected at validation time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fc7db496-b7be-49b6-ad93-2d1c315a5fb4
📒 Files selected for processing (10)
src/content/__tests__/buildRenderFfmpegArgs.test.tssrc/content/buildRenderFfmpegArgs.tssrc/content/downloadMediaToFile.tssrc/content/falServer.tssrc/content/renderFinalVideo.tssrc/content/runFfmpeg.tssrc/content/uploadToFalStorage.tssrc/schemas/createRenderSchema.tssrc/tasks/__tests__/createRenderTask.test.tssrc/tasks/createRenderTask.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/tasks/createRenderTask.ts
- src/content/tests/buildRenderFfmpegArgs.test.ts
Bug: when only audio_url is provided (no video_url), the input is an audio file. Video operations (crop, resize, overlay_text) and video codec flags (-c:v, -map 0:v:0) fail because there's no video stream. Reproduces Trigger.dev run failure: "Stream map '0:v:0' matches no streams" when template operations include crop on an audio-only input. Changes: - buildRenderFfmpegArgs accepts audioOnly option - Skips crop, resize, overlay_text when audioOnly - mux_audio omits video mapping when audioOnly - Output encoding skips video codec when audioOnly - Task detects audioOnly from payload (no video_url, has audio_url) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/content/buildRenderFfmpegArgs.ts">
<violation number="1" location="src/content/buildRenderFfmpegArgs.ts:86">
P2: In audio-only mode, `mux_audio` fallback can remap to a second input and drop earlier audio edits (e.g., `trim`).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
- createRenderTask.ts → ffmpegEditTask.ts - createRenderSchema.ts → ffmpegEditSchema.ts - Task ID: "create-render" → "ffmpeg-edit" - All exports and test references updated All 318 tests pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
sweetmantech
commented
Apr 9, 2026
src/content/buildRenderFfmpegArgs.ts
Outdated
| break; | ||
| } | ||
|
|
||
| case "mux_audio": { |
Contributor
Author
There was a problem hiding this comment.
Is this case required now that we are video only?
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/schemas/ffmpegEditSchema.ts (1)
11-11:⚠️ Potential issue | 🟡 MinorValidate
aspectstring format to prevent invalid crop filters.The
aspectfield accepts any string, but downstream code inbuildRenderFfmpegArgs.tsexpects a"W:H"format and splits on:. Invalid formats like"foo"or"9-16"will silently produce broken crop filters.🛡️ Proposed fix
- aspect: z.string().optional(), + aspect: z.string().regex(/^\d+:\d+$/, "aspect must be in W:H format (e.g. '9:16')").optional(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/schemas/ffmpegEditSchema.ts` at line 11, The aspect field currently allows any string which can break buildRenderFfmpegArgs.ts that expects "W:H"; change the aspect schema in ffmpegEditSchema (the aspect property) to validate the "W:H" integer pattern (e.g. use z.string().regex(/^\d+:\d+$/, 'aspect must be in "W:H" integer format').optional()) so invalid values like "foo" or "9-16" are rejected with a clear error message.
🧹 Nitpick comments (3)
src/tasks/__tests__/ffmpegEditTask.test.ts (1)
163-177: Dynamic imports may not reset between tests due to module caching.Each test dynamically imports
ffmpegEditTask, but after the first import, subsequent tests receive the cached module. Sincevi.clearAllMocks()only clears mock call history (not module cache), the task object is shared across tests. This works here because you're only reading properties, but could cause issues if tests modified state.Consider importing once at the top of the describe block or using
vi.resetModules()inbeforeEachif isolation is needed.♻️ Alternative approach
describe("ffmpegEditTask", () => { + let ffmpegEditTask: Awaited<typeof import("../ffmpegEditTask")>["ffmpegEditTask"]; + beforeEach(() => { vi.clearAllMocks(); process.env.FAL_KEY = "test-key"; }); - it("exports a task with id ffmpeg-edit", async () => { - const { ffmpegEditTask } = await import("../ffmpegEditTask"); + beforeAll(async () => { + const mod = await import("../ffmpegEditTask"); + ffmpegEditTask = mod.ffmpegEditTask; + }); + + it("exports a task with id ffmpeg-edit", () => { expect(ffmpegEditTask.id).toBe("ffmpeg-edit"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tasks/__tests__/ffmpegEditTask.test.ts` around lines 163 - 177, The tests repeatedly use dynamic imports of ffmpegEditTask which may return a cached module across tests; to ensure test isolation either import ffmpegEditTask once at the top of the describe block and reuse that value for the three tests (so ffmpegEditTask, ffmpegEditTask.id, ffmpegEditTask.schema, ffmpegEditTask.machine, ffmpegEditTask.maxDuration are read from the same stable import), or add vi.resetModules() in a beforeEach to clear the module cache before each test so each dynamic import yields a fresh module instance.src/tasks/ffmpegEditTask.ts (1)
7-7: Useloggerfrom@trigger.dev/sdk/v3instead of customlogStep.Per coding guidelines, tasks should use the SDK's built-in
loggerfor logging. This ensures proper integration with Trigger.dev's logging infrastructure and dashboard.♻️ Proposed change
-import { logStep } from "../sandboxes/logStep"; +import { logger } from "@trigger.dev/sdk/v3"; ... - logStep("ffmpeg-edit task started", true, { + logger.info("ffmpeg-edit task started", { accountId: payload.accountId, ... - logStep("Downloading input media"); + logger.info("Downloading input media"); ... - logStep("Running ffmpeg", true, { args: ffmpegArgs.join(" ") }); + logger.info("Running ffmpeg", { args: ffmpegArgs.join(" ") }); ... - logStep("Uploading rendered output"); + logger.info("Uploading rendered output"); ... - logStep("Render complete", true, { url: result.url, sizeBytes: result.sizeBytes }); + logger.info("Render complete", { url: result.url, sizeBytes: result.sizeBytes });As per coding guidelines: "Use
loggerfrom@trigger.dev/sdk/v3for logging".Also applies to: 30-34, 47-47, 52-52, 55-55, 58-58
src/schemas/ffmpegEditSchema.ts (1)
3-38: Consider usingz.discriminatedUnion()for better performance and error messages.Since all operation objects have a
typeliteral discriminator,z.discriminatedUnion("type", [...])would provide more efficient parsing and clearer validation errors when an operation fails to match.♻️ Suggested refactor
-export const editOperationSchema = z.union([ +export const editOperationSchema = z.discriminatedUnion("type", [ z.object({ type: z.literal("trim"), start: z.number().nonnegative(), duration: z.number().positive(), }), - z.object({ + z.object({ type: z.literal("crop"), ... - }).refine(...), + }), // Note: .refine() cannot be used directly inside discriminatedUnion members. // You may need to validate crop/resize constraints in a wrapper or superRefine.Note: If you keep
z.union()due to the.refine()calls on crop/resize, consider adding a comment explaining this tradeoff.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/schemas/ffmpegEditSchema.ts` around lines 3 - 38, Replace the top-level z.union with z.discriminatedUnion("type", [...]) for editOperationSchema to leverage the literal "type" discriminator (keep each variant object schemas—including the .refine calls on the crop and resize schemas—attached to their respective object definitions); update the definition line from z.union([...]) to z.discriminatedUnion("type", [...]) and ensure the crop and resize objects still call .refine(...) as before so their validation remains intact; if you intentionally keep z.union, add a short comment by editOperationSchema explaining the tradeoff for the refine usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tasks/ffmpegEditTask.ts`:
- Around line 36-37: The temp directory stored in tempDir (created via
join(tmpdir(), `render-${randomUUID()}`) in ffmpegEditTask) is never removed;
update the cleanup in the finally block (the section that currently deletes
individual files around lines 66-70) to also remove the entire tempDir after
deleting files by invoking a recursive delete (e.g., fs.rm or fs.promises.rm
with recursive/force) or equivalent rmdir call, and guard it with try/catch so
failures don't mask other errors; reference tempDir and the existing per-file
cleanup code to add this final directory removal.
- Around line 39-40: The temp filenames and upload MIME are hardcoded for video;
when payload.audioOnly is true, change the inputPath to use an audio extension
(e.g., input.wav or input.<inferred audio ext>) instead of "input.mp4", set
outputPath extension accordingly, and ensure the upload uses an audio MIME like
"audio/${payload.output_format}" rather than "video/${payload.output_format}";
update the logic around inputPath, outputPath and the MIME type construction so
it branches on payload.audioOnly (or inspects payload.output_format) to pick the
correct file extension and "audio/" vs "video/" media type while keeping
existing variable names inputPath, outputPath and payload.output_format.
---
Duplicate comments:
In `@src/schemas/ffmpegEditSchema.ts`:
- Line 11: The aspect field currently allows any string which can break
buildRenderFfmpegArgs.ts that expects "W:H"; change the aspect schema in
ffmpegEditSchema (the aspect property) to validate the "W:H" integer pattern
(e.g. use z.string().regex(/^\d+:\d+$/, 'aspect must be in "W:H" integer
format').optional()) so invalid values like "foo" or "9-16" are rejected with a
clear error message.
---
Nitpick comments:
In `@src/schemas/ffmpegEditSchema.ts`:
- Around line 3-38: Replace the top-level z.union with
z.discriminatedUnion("type", [...]) for editOperationSchema to leverage the
literal "type" discriminator (keep each variant object schemas—including the
.refine calls on the crop and resize schemas—attached to their respective object
definitions); update the definition line from z.union([...]) to
z.discriminatedUnion("type", [...]) and ensure the crop and resize objects still
call .refine(...) as before so their validation remains intact; if you
intentionally keep z.union, add a short comment by editOperationSchema
explaining the tradeoff for the refine usage.
In `@src/tasks/__tests__/ffmpegEditTask.test.ts`:
- Around line 163-177: The tests repeatedly use dynamic imports of
ffmpegEditTask which may return a cached module across tests; to ensure test
isolation either import ffmpegEditTask once at the top of the describe block and
reuse that value for the three tests (so ffmpegEditTask, ffmpegEditTask.id,
ffmpegEditTask.schema, ffmpegEditTask.machine, ffmpegEditTask.maxDuration are
read from the same stable import), or add vi.resetModules() in a beforeEach to
clear the module cache before each test so each dynamic import yields a fresh
module instance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bc2ffb50-09ba-4cad-b212-f90283437d6c
📒 Files selected for processing (5)
src/content/__tests__/buildRenderFfmpegArgs.test.tssrc/content/buildRenderFfmpegArgs.tssrc/schemas/ffmpegEditSchema.tssrc/tasks/__tests__/ffmpegEditTask.test.tssrc/tasks/ffmpegEditTask.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/content/buildRenderFfmpegArgs.ts
Comment on lines
+36
to
+37
| const tempDir = join(tmpdir(), `render-${randomUUID()}`); | ||
| await mkdir(tempDir, { recursive: true }); |
There was a problem hiding this comment.
Temp directory is created but never deleted.
The finally block removes individual files but leaves the temp directory (render-<uuid>) behind. Over time, this could accumulate orphan directories.
🧹 Proposed fix
+import { unlink, mkdir, rm } from "node:fs/promises";
...
} finally {
await Promise.all(
[inputPath, outputPath].map((p) => unlink(p).catch(() => undefined)),
);
+ await rm(tempDir, { recursive: true, force: true }).catch(() => undefined);
}Also applies to: 66-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tasks/ffmpegEditTask.ts` around lines 36 - 37, The temp directory stored
in tempDir (created via join(tmpdir(), `render-${randomUUID()}`) in
ffmpegEditTask) is never removed; update the cleanup in the finally block (the
section that currently deletes individual files around lines 66-70) to also
remove the entire tempDir after deleting files by invoking a recursive delete
(e.g., fs.rm or fs.promises.rm with recursive/force) or equivalent rmdir call,
and guard it with try/catch so failures don't mask other errors; reference
tempDir and the existing per-file cleanup code to add this final directory
removal.
Comment on lines
+39
to
+40
| const inputPath = join(tempDir, "input.mp4"); | ||
| const outputPath = join(tempDir, `output.${payload.output_format}`); |
There was a problem hiding this comment.
Hardcoded video/* MIME type and .mp4 input extension break audio-only workflows.
When audioOnly is true:
- Line 39 names the temp input
input.mp4even for audio files - Line 56 always uploads with
video/${output_format}MIME type
This could cause downstream issues with audio-only renders being mislabeled.
🛡️ Proposed fix
+ const inputExt = audioOnly ? "mp3" : "mp4";
+ const mimePrefix = audioOnly ? "audio" : "video";
- const inputPath = join(tempDir, "input.mp4");
+ const inputPath = join(tempDir, `input.${inputExt}`);
const outputPath = join(tempDir, `output.${payload.output_format}`);
...
- const result = await uploadToFalStorage(outputPath, `rendered.${payload.output_format}`, `video/${payload.output_format}`);
+ const result = await uploadToFalStorage(outputPath, `rendered.${payload.output_format}`, `${mimePrefix}/${payload.output_format}`);Also applies to: 56-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tasks/ffmpegEditTask.ts` around lines 39 - 40, The temp filenames and
upload MIME are hardcoded for video; when payload.audioOnly is true, change the
inputPath to use an audio extension (e.g., input.wav or input.<inferred audio
ext>) instead of "input.mp4", set outputPath extension accordingly, and ensure
the upload uses an audio MIME like "audio/${payload.output_format}" rather than
"video/${payload.output_format}"; update the logic around inputPath, outputPath
and the MIME type construction so it branches on payload.audioOnly (or inspects
payload.output_format) to pick the correct file extension and "audio/" vs
"video/" media type while keeping existing variable names inputPath, outputPath
and payload.output_format.
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/tasks/ffmpegEditTask.ts">
<violation number="1" location="src/tasks/ffmpegEditTask.ts:21">
P1: Custom agent: **Flag AI Slop and Fabricated Changes**
This change violates the “Flag AI Slop and Fabricated Changes” clause for PR/docs claims not implemented: the PR claims a `create-render` task, but the code registers `ffmpeg-edit`. Align the task ID with the stated API contract or correct the PR metadata.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| * order using ffmpeg. Uploads the result to fal.ai storage. | ||
| */ | ||
| export const ffmpegEditTask = schemaTask({ | ||
| id: "ffmpeg-edit", |
There was a problem hiding this comment.
P1: Custom agent: Flag AI Slop and Fabricated Changes
This change violates the “Flag AI Slop and Fabricated Changes” clause for PR/docs claims not implemented: the PR claims a create-render task, but the code registers ffmpeg-edit. Align the task ID with the stated API contract or correct the PR metadata.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/tasks/ffmpegEditTask.ts, line 21:
<comment>This change violates the “Flag AI Slop and Fabricated Changes” clause for PR/docs claims not implemented: the PR claims a `create-render` task, but the code registers `ffmpeg-edit`. Align the task ID with the stated API contract or correct the PR metadata.</comment>
<file context>
@@ -17,8 +17,8 @@ import { buildRenderFfmpegArgs } from "../content/buildRenderFfmpegArgs";
-export const createRenderTask = schemaTask({
- id: "create-render",
+export const ffmpegEditTask = schemaTask({
+ id: "ffmpeg-edit",
schema: createRenderPayloadSchema,
maxDuration: 600,
</file context>
Addresses all 5 PR review comments: 1. Remove mux_audio and audio_url — video-only endpoint 2. Remove audioOnly handling — no longer needed 3. Fix crop aspect math — 9:16 now correctly narrows width (ih*9/16:ih) instead of expanding height. Fixes "Invalid too big size" ffmpeg error. 4. runFfmpeg maxBuffer increased to 10MB for verbose ffmpeg stderr 5. Color regex restricted to valid CSS hex lengths (3/4/6/8 digits) SRP: buildRenderFfmpegArgs split into sub-functions: - buildCropFilter() — aspect ratio and dimension crop logic - buildOverlayTextFilter() — text escaping, positioning, styling Schema renamed: createRenderPayloadSchema → ffmpegEditPayloadSchema (deprecated alias kept for backwards compat) All 316 tests pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/schemas/ffmpegEditSchema.ts (1)
13-18:⚠️ Potential issue | 🟠 Major
cropvalidation is too permissive and allows invalid ffmpeg operations.Current rules accept malformed
aspectvalues and one-sided crop dimensions. That can pass validation but later generate invalid crop filters.Proposed schema tightening
z.object({ type: z.literal("crop"), - aspect: z.string().optional(), + aspect: z + .string() + .regex(/^[1-9]\d*:[1-9]\d*$/, "aspect must be W:H with positive integers") + .optional(), width: z.number().int().positive().optional(), height: z.number().int().positive().optional(), - }).refine(data => data.aspect || data.width || data.height, { - message: "crop requires at least one of: aspect, width, height", + }).refine((data) => Boolean(data.aspect) || (data.width !== undefined && data.height !== undefined), { + message: "crop requires either a valid aspect or both width and height", }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/schemas/ffmpegEditSchema.ts` around lines 13 - 18, The crop schema currently allows invalid aspect strings and one-sided dimensions; tighten validation on the crop object (the crop schema with keys aspect, width, height) by: 1) validating aspect with a strict pattern (e.g., a regex matching "N:M" positive integers) instead of plain z.string(); 2) ensure width and height remain positive ints but enforce that if one of width or height is provided the other must also be present (use a .refine on the crop object to require both or neither); and 3) keep the overall refine that requires at least aspect or both dimensions so invalid combinations (malformed aspect or lone width/height) are rejected.src/content/buildRenderFfmpegArgs.ts (1)
69-74:⚠️ Potential issue | 🔴 Critical
cropaspect math can exceed source bounds and fail ffmpeg.Line 73 chooses crop math only from target orientation, not source-vs-target aspect comparison. This still produces invalid crops for valid inputs (e.g., ultra-wide sources), causing runtime ffmpeg failures.
Proposed fix
function buildCropFilter(op: { aspect?: string; width?: number; height?: number }): string { if (op.aspect) { const [w, h] = op.aspect.split(":").map(Number); if (w && h) { - return w >= h ? `crop=iw:iw*${h}/${w}` : `crop=ih*${w}/${h}:ih`; + const ratio = `${w}/${h}`; + return `crop=if(gte(iw/ih\\,${ratio})\\,ih*${ratio}\\,iw):if(gte(iw/ih\\,${ratio})\\,ih\\,iw/${ratio})`; } } return `crop=${op.width ?? -1}:${op.height ?? -1}`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/content/buildRenderFfmpegArgs.ts` around lines 69 - 74, The crop math in buildCropFilter only looks at target orientation (w >= h) and can produce crop values larger than the source (causing ffmpeg failures); update buildCropFilter to compare source aspect to target aspect (use ffmpeg expressions like iw/ih vs w/h) and emit a conditional crop expression that clamps to source bounds (use ffmpeg min() or if() expressions so the computed crop width/height never exceed iw/ih, e.g. choose cropping by width when source is wider than target and by height otherwise, but expressed with safe ffmpeg arithmetic), referencing buildCropFilter and the local w,h/op.aspect variables so the new expression always yields values within input dimensions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tasks/__tests__/ffmpegEditTask.test.ts`:
- Around line 112-122: The test currently may pass vacuously because it only
checks for absence of audio_url when parse succeeded; explicitly assert the
parse succeeded before further assertions by adding an assertion on
createRenderPayloadSchema.safeParse(...) result (e.g.,
expect(result.success).toBe(true)) and then perform the property check on
result.data; reference the test block using the existing
createRenderPayloadSchema and the "does not accept audio_url param" test to
locate where to add the success assertion.
---
Duplicate comments:
In `@src/content/buildRenderFfmpegArgs.ts`:
- Around line 69-74: The crop math in buildCropFilter only looks at target
orientation (w >= h) and can produce crop values larger than the source (causing
ffmpeg failures); update buildCropFilter to compare source aspect to target
aspect (use ffmpeg expressions like iw/ih vs w/h) and emit a conditional crop
expression that clamps to source bounds (use ffmpeg min() or if() expressions so
the computed crop width/height never exceed iw/ih, e.g. choose cropping by width
when source is wider than target and by height otherwise, but expressed with
safe ffmpeg arithmetic), referencing buildCropFilter and the local w,h/op.aspect
variables so the new expression always yields values within input dimensions.
In `@src/schemas/ffmpegEditSchema.ts`:
- Around line 13-18: The crop schema currently allows invalid aspect strings and
one-sided dimensions; tighten validation on the crop object (the crop schema
with keys aspect, width, height) by: 1) validating aspect with a strict pattern
(e.g., a regex matching "N:M" positive integers) instead of plain z.string(); 2)
ensure width and height remain positive ints but enforce that if one of width or
height is provided the other must also be present (use a .refine on the crop
object to require both or neither); and 3) keep the overall refine that requires
at least aspect or both dimensions so invalid combinations (malformed aspect or
lone width/height) are rejected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3fda437a-2110-4a77-acbb-820b618d495f
📒 Files selected for processing (7)
src/content/__tests__/buildRenderFfmpegArgs.test.tssrc/content/__tests__/runFfmpeg.test.tssrc/content/buildRenderFfmpegArgs.tssrc/content/runFfmpeg.tssrc/schemas/ffmpegEditSchema.tssrc/tasks/__tests__/ffmpegEditTask.test.tssrc/tasks/ffmpegEditTask.ts
✅ Files skipped from review due to trivial changes (1)
- src/content/runFfmpeg.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/tasks/ffmpegEditTask.ts
- src/content/tests/buildRenderFfmpegArgs.test.ts
Comment on lines
+112
to
+122
| it("does not accept audio_url param", () => { | ||
| const result = createRenderPayloadSchema.safeParse({ | ||
| accountId: "acc-123", | ||
| video_url: "https://example.com/video.mp4", | ||
| audio_url: "https://example.com/audio.mp3", | ||
| operations: [{ type: "trim", start: 0, duration: 5 }], | ||
| }); | ||
| if (result.success) { | ||
| expect(result.data).not.toHaveProperty("audio_url"); | ||
| } | ||
| }); |
There was a problem hiding this comment.
This test can pass vacuously without validating behavior.
Please assert result.success explicitly before checking property stripping; otherwise a parse failure still passes this test.
Proposed fix
it("does not accept audio_url param", () => {
const result = createRenderPayloadSchema.safeParse({
accountId: "acc-123",
video_url: "https://example.com/video.mp4",
audio_url: "https://example.com/audio.mp3",
operations: [{ type: "trim", start: 0, duration: 5 }],
});
+ expect(result.success).toBe(true);
if (result.success) {
expect(result.data).not.toHaveProperty("audio_url");
}
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it("does not accept audio_url param", () => { | |
| const result = createRenderPayloadSchema.safeParse({ | |
| accountId: "acc-123", | |
| video_url: "https://example.com/video.mp4", | |
| audio_url: "https://example.com/audio.mp3", | |
| operations: [{ type: "trim", start: 0, duration: 5 }], | |
| }); | |
| if (result.success) { | |
| expect(result.data).not.toHaveProperty("audio_url"); | |
| } | |
| }); | |
| it("does not accept audio_url param", () => { | |
| const result = createRenderPayloadSchema.safeParse({ | |
| accountId: "acc-123", | |
| video_url: "https://example.com/video.mp4", | |
| audio_url: "https://example.com/audio.mp3", | |
| operations: [{ type: "trim", start: 0, duration: 5 }], | |
| }); | |
| expect(result.success).toBe(true); | |
| if (result.success) { | |
| expect(result.data).not.toHaveProperty("audio_url"); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tasks/__tests__/ffmpegEditTask.test.ts` around lines 112 - 122, The test
currently may pass vacuously because it only checks for absence of audio_url
when parse succeeded; explicitly assert the parse succeeded before further
assertions by adding an assertion on createRenderPayloadSchema.safeParse(...)
result (e.g., expect(result.success).toBe(true)) and then perform the property
check on result.data; reference the test block using the existing
createRenderPayloadSchema and the "does not accept audio_url param" test to
locate where to add the success assertion.
sweetmantech
commented
Apr 9, 2026
src/content/buildRenderFfmpegArgs.ts
Outdated
| * - 9:16 (portrait): keep full height, narrow width → crop=ih*9/16:ih | ||
| * - 16:9 (landscape): keep full width, narrow height → crop=iw:iw*9/16 | ||
| */ | ||
| function buildCropFilter(op: { aspect?: string; width?: number; height?: number }): string { |
Contributor
Author
There was a problem hiding this comment.
SRP - new lib file for buildCropFilter
src/content/buildRenderFfmpegArgs.ts
Outdated
| /** | ||
| * Build the ffmpeg drawtext= filter for text overlay. | ||
| */ | ||
| function buildOverlayTextFilter(op: { |
Contributor
Author
There was a problem hiding this comment.
SRP - new lib file for buildOverlayTextFilter
There was a problem hiding this comment.
4 issues found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/schemas/ffmpegEditSchema.ts">
<violation number="1" location="src/schemas/ffmpegEditSchema.ts:3">
P1: Custom agent: **Flag AI Slop and Fabricated Changes**
PR description claims mux_audio/audio-only edits are supported, but this schema change removes the mux_audio operation and makes video_url required, so that behavior isn’t implemented. Update the PR description or restore mux_audio support to avoid shipping misleading documentation.</violation>
</file>
<file name="src/content/__tests__/buildRenderFfmpegArgs.test.ts">
<violation number="1" location="src/content/__tests__/buildRenderFfmpegArgs.test.ts:145">
P1: Custom agent: **Flag AI Slop and Fabricated Changes**
This test claims to verify mux_audio handling, but it never passes a mux_audio operation. As written, it only tests the trim path and doesn’t validate the claimed behavior.</violation>
<violation number="2" location="src/content/__tests__/buildRenderFfmpegArgs.test.ts:157">
P1: Custom agent: **Flag AI Slop and Fabricated Changes**
This test only asserts that `args` is non-empty, which doesn’t verify the claimed signature change at all. It reads like a placeholder rather than a real validation.</violation>
</file>
<file name="src/content/buildRenderFfmpegArgs.ts">
<violation number="1" location="src/content/buildRenderFfmpegArgs.ts:35">
P2: Guard against malformed aspect strings before adding a crop filter. With the current helper, any non-"w:h" aspect that still passes schema validation emits `crop=-1:-1`, which doesn't reflect a valid requested crop. Consider validating the aspect format in the schema or skipping the crop when parsing fails.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| @@ -0,0 +1,49 @@ | |||
| import { z } from "zod"; | |||
There was a problem hiding this comment.
P1: Custom agent: Flag AI Slop and Fabricated Changes
PR description claims mux_audio/audio-only edits are supported, but this schema change removes the mux_audio operation and makes video_url required, so that behavior isn’t implemented. Update the PR description or restore mux_audio support to avoid shipping misleading documentation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/schemas/ffmpegEditSchema.ts, line 3:
<comment>PR description claims mux_audio/audio-only edits are supported, but this schema change removes the mux_audio operation and makes video_url required, so that behavior isn’t implemented. Update the PR description or restore mux_audio support to avoid shipping misleading documentation.</comment>
<file context>
@@ -1,5 +1,7 @@
import { z } from "zod";
+const cssColorRegex = /^[a-zA-Z]+$|^#([0-9a-fA-F]{3}|[0-9a-fA-F]{4}|[0-9a-fA-F]{6}|[0-9a-fA-F]{8})$/;
+
export const editOperationSchema = z.union([
</file context>
… up tests Address PR review comments: 1. buildCropFilter.ts — own file, guards against malformed aspect strings 2. buildOverlayTextFilter.ts — own file with text escaping and positioning 3. buildRenderFfmpegArgs.ts — now under 65 LOC, composes sub-functions 4. Remove weak/placeholder tests, add malformed aspect test 5. Remove all mux_audio and audioOnly references from tests All 316 tests pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
sweetmantech
changed the title
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.
Summary
ffmpeg-editTrigger.dev task that the API'sPATCH /api/contentendpoint triggersContext
Operations Supported
trim-ss/-tcropcrop=resizescale=overlay_textdrawtext=Files
src/schemas/ffmpegEditSchema.tssrc/tasks/ffmpegEditTask.tssrc/content/buildRenderFfmpegArgs.tssrc/content/buildCropFilter.tssrc/content/buildOverlayTextFilter.tssrc/content/downloadMediaToFile.tssrc/content/runFfmpeg.tssrc/content/uploadToFalStorage.tssrc/content/falServer.tsDRY with existing pipeline
Shared utilities extracted from
renderFinalVideoused by both tasks:downloadMediaToFile,runFfmpeg,uploadToFalStorageescapeDrawtext,stripEmojiTest plan
🤖 Generated with Claude Code