Repository navigation
refactor: computation-graph IR, render lifecycle, renderers split - #223
Merged
Merged
Conversation
…ixes Stage 0 — P0 fixes: - round musicLengthMs to integer ms in elevenlabs generateMusic (fractional seconds caused ElevenLabs 422s) - warn when providerOptions.varg contains a flat option bag: the varg API only forwards options nested under a provider key (e.g. varg.fal.*), flat options are silently dropped server-side Stage 1 — compile() + IR (read-only, render() untouched): - src/react/ir/: Step/OpKind/Diagnostic types, cycle-detecting toposort, compile(tree) -> CompiledPlan with ordered steps, dependency discovery (prompt.images/audio/video, Captions src, audio parents), validation diagnostics (root-not-render, missing source/model, audio conflicts), pre-resolved elements enter the plan as "skipped" Stage 2a — derived audio nodes: - video.audio / speech.audio: lazy memoized VargElement<"audio"> with transcribe() (reuses native ElevenLabs words when present), silenceSegments(), bounds(); memoized .then (safe to re-await) - audio: "native" sugar on Video -> keepAudio + generate_audio in the correct provider namespace (fal direct, varg.fal for gateway models); conflicts (keepAudio: false / generate_audio: false) rejected by compile() - speech is now part of audio: ResolvedElement.audio returns an AudioNode preserving file/duration/words (destructuring compat kept) - audio elements accepted everywhere speech was: clip child, prompt.audio, TalkingHead audio, Captions src (words shortcut skips transcription), container/render-level audio tracks - pipeline/audio.ts: shared extractAudio (-vn), detectSilence, transcribeAudio primitives (first brick of the shared pipeline layer) - cache keys unchanged: "audio" prop ignored in computeCacheKey, sugar expansion produces identical keys to manual configuration
Contributor
|
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:
📝 WalkthroughWalkthroughthis pr adds first-class derived audio elements, audio analysis, render-plan compilation and execution, caption timeline handling, async jsx support, renderer deduplication, and provider request retries. Changesaudio and render pipeline
provider request handling
estimated code review effort: 5 (critical) | ~120 minutes possibly related issues
possibly related prs
poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
src/ai-sdk/providers/varg.ts (2)
95-97: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuearrow function time.
guidelines prefer arrow functions over standard function declarations. easy swap to keep things consistent! meow 😸
✨ proposed fix
-function checkVargProviderOptions( - opts: Record<string, unknown>, -): Record<string, unknown> { +const checkVargProviderOptions = ( + opts: Record<string, unknown>, +): Record<string, unknown> => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ai-sdk/providers/varg.ts` around lines 95 - 97, Convert checkVargProviderOptions from a standard function declaration to an arrow function while preserving its parameters, return type, and existing behavior.Source: Coding guidelines
81-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuecamelcase needed.
guidelines want camelcase for all variables. check the diffs below for the fix. purrfect 🐈
✨ proposed fix
-const KNOWN_UNDERLYING_PROVIDERS = new Set([ +const knownUnderlyingProviders = new Set([// also update the usage on line 101: knownUnderlyingProviders.has(k) &&🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ai-sdk/providers/varg.ts` at line 81, Rename the constant KNOWN_UNDERLYING_PROVIDERS to knownUnderlyingProviders and update its usage in the provider lookup condition, including the .has(k) call, while preserving the existing set contents and behavior.Source: Coding guidelines
src/ai-sdk/providers/elevenlabs.ts (1)
324-325: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuestrict equality needed.
guidelines say we gotta use strict equality. swap that loose check out to keep the linter happy. meow 🐾
✨ proposed fix
- ...(durationSeconds != null + ...(durationSeconds !== undefined && durationSeconds !== null🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ai-sdk/providers/elevenlabs.ts` around lines 324 - 325, Update the durationSeconds presence check in the musicLengthMs construction to use strict equality semantics instead of the loose null comparison, while preserving the existing behavior of excluding musicLengthMs only when durationSeconds is null or undefined.Source: Coding guidelines
src/react/ir/types.ts (1)
98-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuetiny jsdoc gap on public members
hasErrors,errors, andwarningsare public but skip jsdoc while the class +toJSONhave it. guidelines want jsdoc on all public functions/classes, so worth a one-liner each for consistency. totally optional though, meow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/react/ir/types.ts` around lines 98 - 108, Add one-line JSDoc comments to the public hasErrors method and errors and warnings getters, describing each member’s purpose. Keep their existing implementations and behavior unchanged, matching the surrounding class and toJSON documentation style.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/react/audio-element.ts`:
- Around line 92-100: Update resolveOnce so a rejected resolveAudioElement
promise clears resolvedPromise before propagating the error, allowing subsequent
await, transcribe(), or bounds() calls to retry. Preserve the existing
successful memoization and element.meta synchronization behavior.
In `@src/react/ir/compile.ts`:
- Around line 184-195: Update the providerOptions conflict validation around the
existing Object.entries loop to inspect the nested varg provider path,
specifically providerOptions.varg.fal.generate_audio, in addition to the current
top-level provider option shape. Emit the existing VARG_AUDIO_CONFLICT
diagnostic whenever that nested value is false, so audio: "native" cannot be
silently dropped.
In `@src/react/pipeline/audio.ts`:
- Around line 64-65: Update the fallback audio processing flow around the input
created by file.toTempFile() and the ffmpeg call so any temporary input created
for non-URL files is deleted in a finally block after ffmpeg completes or fails;
preserve URL inputs without attempting to delete them, and apply the same
cleanup behavior in detectSilence.
In `@src/react/resolve.ts`:
- Around line 1195-1217: Make extracted video audio metadata consistent between
the cache-hit and cache-miss paths in the surrounding audio resolution flow.
Update the cached File.fromGenerated path and the newly extracted audioFile path
to use the same appropriate audio metadata type, avoiding the misleading
"speech" label; preserve the existing duration and caching behavior.
In `@src/react/resolved-element.ts`:
- Around line 74-84: Update the audio getter on ResolvedElement so audio-typed
elements return a real AudioNode created through makeAudioNode rather than
casting this directly. Preserve audioNodeCache reuse and ensure the resulting
object provides the AudioNode contract, including transcribe(),
silenceSegments(), and bounds().
---
Nitpick comments:
In `@src/ai-sdk/providers/elevenlabs.ts`:
- Around line 324-325: Update the durationSeconds presence check in the
musicLengthMs construction to use strict equality semantics instead of the loose
null comparison, while preserving the existing behavior of excluding
musicLengthMs only when durationSeconds is null or undefined.
In `@src/ai-sdk/providers/varg.ts`:
- Around line 95-97: Convert checkVargProviderOptions from a standard function
declaration to an arrow function while preserving its parameters, return type,
and existing behavior.
- Line 81: Rename the constant KNOWN_UNDERLYING_PROVIDERS to
knownUnderlyingProviders and update its usage in the provider lookup condition,
including the .has(k) call, while preserving the existing set contents and
behavior.
In `@src/react/ir/types.ts`:
- Around line 98-108: Add one-line JSDoc comments to the public hasErrors method
and errors and warnings getters, describing each member’s purpose. Keep their
existing implementations and behavior unchanged, matching the surrounding class
and toJSON documentation style.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: df802d2c-02d4-47b9-94d5-84c8d386cbac
📒 Files selected for processing (21)
src/ai-sdk/providers/elevenlabs.tssrc/ai-sdk/providers/varg.tssrc/react/audio-element.test.tssrc/react/audio-element.tssrc/react/elements.tssrc/react/index.tssrc/react/ir/compile.test.tssrc/react/ir/compile.tssrc/react/ir/topo-sort.tssrc/react/ir/types.tssrc/react/pipeline/audio.tssrc/react/renderers/audio.tssrc/react/renderers/captions.tssrc/react/renderers/clip.tssrc/react/renderers/render.tssrc/react/renderers/talking-head.tssrc/react/renderers/utils.tssrc/react/renderers/video.tssrc/react/resolve.tssrc/react/resolved-element.tssrc/react/types.ts
Comment on lines
+92
to
+100
| const resolveOnce = () => { | ||
| // Sync element.meta after resolution so the sync getters | ||
| // (.duration, .words) and renderers see the resolved file. | ||
| resolvedPromise ??= resolveAudioElement(element).then((r) => { | ||
| element.meta = r.meta; | ||
| return r; | ||
| }); | ||
| return resolvedPromise; | ||
| }; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
memoized rejection never lets you retry
resolveOnce caches the promise even when it rejects. if resolveAudioElement fails transiently (ffmpeg blip, network), every later await/transcribe()/bounds() on this node replays the same rejection forever. reset on failure so a retry is possible:
🔁 proposed fix
const resolveOnce = () => {
// Sync element.meta after resolution so the sync getters
// (.duration, .words) and renderers see the resolved file.
- resolvedPromise ??= resolveAudioElement(element).then((r) => {
- element.meta = r.meta;
- return r;
- });
+ resolvedPromise ??= resolveAudioElement(element)
+ .then((r) => {
+ element.meta = r.meta;
+ return r;
+ })
+ .catch((err) => {
+ resolvedPromise = undefined;
+ throw err;
+ });
return resolvedPromise;
};meow, small guard, saves a lot of head-scratching.
📝 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
| const resolveOnce = () => { | |
| // Sync element.meta after resolution so the sync getters | |
| // (.duration, .words) and renderers see the resolved file. | |
| resolvedPromise ??= resolveAudioElement(element).then((r) => { | |
| element.meta = r.meta; | |
| return r; | |
| }); | |
| return resolvedPromise; | |
| }; | |
| const resolveOnce = () => { | |
| // Sync element.meta after resolution so the sync getters | |
| // (.duration, .words) and renderers see the resolved file. | |
| resolvedPromise ??= resolveAudioElement(element) | |
| .then((r) => { | |
| element.meta = r.meta; | |
| return r; | |
| }) | |
| .catch((err) => { | |
| resolvedPromise = undefined; | |
| throw err; | |
| }); | |
| return resolvedPromise; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/react/audio-element.ts` around lines 92 - 100, Update resolveOnce so a
rejected resolveAudioElement promise clears resolvedPromise before propagating
the error, allowing subsequent await, transcribe(), or bounds() calls to retry.
Preserve the existing successful memoization and element.meta synchronization
behavior.
Comment on lines
+184
to
+195
| if (providerOptions) { | ||
| for (const [providerKey, opts] of Object.entries(providerOptions)) { | ||
| if (opts && opts.generate_audio === false) { | ||
| addDiagnostic( | ||
| "error", | ||
| "VARG_AUDIO_CONFLICT", | ||
| `audio: "native" conflicts with providerOptions.${providerKey}.generate_audio: false`, | ||
| path, | ||
| ); | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# check how audio:"native" places generate_audio for varg vs fal
rg -nP -C4 'generate_audio|varg\.fal|audio.*native' src/react/elements.ts src/react/audio-element.tsRepository: vargHQ/sdk
Length of output: 2830
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- compile.ts slice ---\n'
sed -n '150,230p' src/react/ir/compile.ts | cat -n
printf '\n--- audio expansion slice ---\n'
sed -n '108,156p' src/react/elements.ts | cat -n
printf '\n--- call sites for expandNativeAudio / compile ---\n'
rg -n "expandNativeAudio|compile\(" src/react -g '!**/*.map'Repository: vargHQ/sdk
Length of output: 7289
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '196,220p' src/react/elements.ts | cat -nRepository: vargHQ/sdk
Length of output: 917
check the nested varg path here
audio: "native" on varg models stores the flag at providerOptions.varg.fal.generate_audio, but this check only looks at top-level providerOptions.*.generate_audio. that lets { varg: { fal: { generate_audio: false } } } slip through and silently drop native audio.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/react/ir/compile.ts` around lines 184 - 195, Update the providerOptions
conflict validation around the existing Object.entries loop to inspect the
nested varg provider path, specifically providerOptions.varg.fal.generate_audio,
in addition to the current top-level provider option shape. Emit the existing
VARG_AUDIO_CONFLICT diagnostic whenever that nested value is false, so audio:
"native" cannot be silently dropped.
Comment on lines
+1195
to
+1217
| if (cached) { | ||
| const file = File.fromGenerated({ | ||
| uint8Array: cached.uint8Array, | ||
| mediaType: cached.mediaType, | ||
| }).withMetadata({ type: "speech" }); | ||
| return new ResolvedElement(element, { | ||
| file, | ||
| duration: cached.duration, | ||
| }); | ||
| } | ||
|
|
||
| const audioFile = await extractAudio(videoMeta.file); | ||
| const duration = | ||
| (await probeDuration(audioFile)) || videoMeta.duration || 0; | ||
|
|
||
| const bytes = await audioFile.arrayBuffer(); | ||
| await cache.set(extractKey, { | ||
| uint8Array: bytes instanceof Uint8Array ? bytes : new Uint8Array(bytes), | ||
| mediaType: "audio/mpeg", | ||
| duration, | ||
| }); | ||
|
|
||
| return new ResolvedElement(element, { file: audioFile, duration }); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
extracted video audio gets mislabeled type: "speech"
on cache-hit you tag the file withMetadata({ type: "speech" }), but the cache-miss path (L1206+) sets no such metadata. so the same audio node has different file metadata depending on whether it hit cache — and calling extracted video audio "speech" is misleading for anything that branches on file type. either drop it or use a consistent audio-ish type on both paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/react/resolve.ts` around lines 1195 - 1217, Make extracted video audio
metadata consistent between the cache-hit and cache-miss paths in the
surrounding audio resolution flow. Update the cached File.fromGenerated path and
the newly extracted audioFile path to use the same appropriate audio metadata
type, avoiding the misleading "speech" label; preserve the existing duration and
caching behavior.
Comment on lines
+74
to
84
| get audio(): AudioNode { | ||
| if (this.type === "audio") return this as unknown as AudioNode; | ||
| let node = audioNodeCache.get(this); | ||
| if (!node) { | ||
| node = makeAudioNode({ | ||
| parent: this as unknown as VargElement<"video"> | VargElement<"speech">, | ||
| }); | ||
| audioNodeCache.set(this, node); | ||
| } | ||
| return node; | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
self-ref audio nodes silently break the AudioNode contract
when this.type === "audio", .audio just returns this cast as AudioNode — but ResolvedElement never implements .transcribe(), .silenceSegments(), or .bounds() (only makeAudioNode's output has those). so (await someAudioNode).audio.transcribe() compiles fine but throws "not a function" at runtime. meow, that's a trap for anyone treating every resolved element uniformly via .audio.
🐛 suggested direction
get audio(): AudioNode {
- if (this.type === "audio") return this as unknown as AudioNode;
let node = audioNodeCache.get(this);
if (!node) {
- node = makeAudioNode({
- parent: this as unknown as VargElement<"video"> | VargElement<"speech">,
- });
+ node =
+ this.type === "audio"
+ ? wrapResolvedAudio(this as unknown as ResolvedElement<"audio">) // delegate to transcribeAudio/detectSilence/computeSoundBounds using this.meta.file
+ : makeAudioNode({
+ parent: this as unknown as VargElement<"video"> | VargElement<"speech">,
+ });
audioNodeCache.set(this, node);
}
return node;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/react/resolved-element.ts` around lines 74 - 84, Update the audio getter
on ResolvedElement so audio-typed elements return a real AudioNode created
through makeAudioNode rather than casting this directly. Preserve audioNodeCache
reuse and ensure the resulting object provides the AudioNode contract, including
transcribe(), silenceSegments(), and bounds().
Stage 4 of the graph refactor — the plan from compile() is now the actual execution path of render(), not just introspection: - ir/execute.ts: executePlan(plan, ctx) walks steps in dependency order with bounded parallelism (ready-set scheduling, not rigid batches), dispatches each step kind to the existing renderers, and writes results back as element.meta. Pre-resolved steps are skipped; shared elements execute once. Emits plan/step-start/step-complete/step-failed/ step-skipped lifecycle events. - render(): resolveLazy -> compile() (throws CompileError with full diagnostics before any generation is paid for) -> executePlan -> renderRoot as the compose phase. One shared RenderContext across both phases (same pendingFiles dedup, progress, generatedFiles). - renderRoot split into createRenderContext + compose phase; calling renderRoot directly still works (legacy recursive behavior preserved for tests and embedders). - Step failures use throwOnError: false inside render(): rejected promises stay in pendingFiles and the compose phase aggregates them with the legacy "N of M clips failed" semantics. - renderStream now yields real per-step events with progress derived from completed/total steps. - makeThenable: memoized resolution instead of .then self-deletion — re-await returns the same ResolvedElement, and resolved meta is mirrored onto the element so the pipeline sees it as pre-resolved. - JSX ElementType now accepts async components; removed the @ts-expect-error workaround from examples.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/react/renderers/render.ts (2)
593-600: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuedry up the type casting
same deal here, cast once and reuse. meow!
♻️ proposed fix
- } else if (audioElement.type === "audio") { - const file = await renderAudio(audioElement as VargElement<"audio">, ctx); - const path = await ctx.backend.resolvePath(file); - audioTracks.push({ - path, - start: offset, - mixVolume: audioMixVolume(audioElement as VargElement<"audio">), - }); + } else if (audioElement.type === "audio") { + const audioEl = audioElement as VargElement<"audio">; + const file = await renderAudio(audioEl, ctx); + const path = await ctx.backend.resolvePath(file); + audioTracks.push({ + path, + start: offset, + mixVolume: audioMixVolume(audioEl), + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/react/renderers/render.ts` around lines 593 - 600, Update the audio branch around renderAudio so the audioElement cast to VargElement<"audio"> is performed once and stored in a local variable, then reuse that variable for renderAudio and audioMixVolume instead of repeating the cast.
427-434: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuedry up the type casting
cast it once to keep things clean. meow!
♻️ proposed fix
- } else if (childElement.type === "audio") { - // Render-level derived audio node — immediate audio track - const file = await renderAudio(childElement as VargElement<"audio">, ctx); - const path = await ctx.backend.resolvePath(file); - audioTracks.push({ - path, - mixVolume: audioMixVolume(childElement as VargElement<"audio">), - }); + } else if (childElement.type === "audio") { + // Render-level derived audio node — immediate audio track + const audioEl = childElement as VargElement<"audio">; + const file = await renderAudio(audioEl, ctx); + const path = await ctx.backend.resolvePath(file); + audioTracks.push({ + path, + mixVolume: audioMixVolume(audioEl), + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/react/renderers/render.ts` around lines 427 - 434, In the audio branch of the render flow, cast childElement once to a VargElement<"audio"> local variable, then reuse that variable for renderAudio and audioMixVolume instead of repeating the type assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/react/elements.ts`:
- Around line 77-86: Update the thenable.then assignment in the element
awaitable setup to use an arrow function and async/await rather than a function
expression and chained .then() calls. Preserve the existing this-based resolver
invocation, resolvedPromise caching, meta mirroring, and resolve/reject
behavior.
In `@src/react/ir/execute.test.ts`:
- Line 177: Update the test assertion in the relevant execute plan test to await
the promise returned by rejects.toThrow(), ensuring the asynchronous matcher
completes before the test finishes.
In `@src/react/runtime/jsx-runtime.ts`:
- Around line 59-60: Replace the props parameter type with Record<string,
unknown> and remove the biome-ignore comment in the JSX factory signature in
src/react/runtime/jsx-runtime.ts lines 59-60. Apply the same change to the
corresponding JSX factory signature in src/react/runtime/jsx-dev-runtime.ts
lines 61-62; no other behavior changes are needed.
---
Nitpick comments:
In `@src/react/renderers/render.ts`:
- Around line 593-600: Update the audio branch around renderAudio so the
audioElement cast to VargElement<"audio"> is performed once and stored in a
local variable, then reuse that variable for renderAudio and audioMixVolume
instead of repeating the cast.
- Around line 427-434: In the audio branch of the render flow, cast childElement
once to a VargElement<"audio"> local variable, then reuse that variable for
renderAudio and audioMixVolume instead of repeating the type assertion.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 98549c1b-216f-4332-8a48-d4e32997e02f
📒 Files selected for processing (10)
src/react/elements.tssrc/react/examples/async/example_we_want_to_test.tsxsrc/react/index.tssrc/react/ir/execute.test.tssrc/react/ir/execute.tssrc/react/ir/types.tssrc/react/render.tssrc/react/renderers/render.tssrc/react/runtime/jsx-dev-runtime.tssrc/react/runtime/jsx-runtime.ts
💤 Files with no reviewable changes (1)
- src/react/examples/async/example_we_want_to_test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/react/ir/types.ts
Comment on lines
77
to
86
| // biome-ignore lint/suspicious/noThenProperty: intentional — makes element awaitable | ||
| thenable.then = function (resolve, reject) { | ||
| // Remove .then to prevent double-resolution | ||
| delete this.then; | ||
| return resolver(this as VargElement<T>).then(resolve, reject); | ||
| resolvedPromise ??= resolver(this as VargElement<T>).then((r) => { | ||
| // Mirror meta onto the original element so the render pipeline | ||
| // treats it as pre-resolved (single generation per element). | ||
| element.meta = r.meta; | ||
| return r; | ||
| }); | ||
| return resolvedPromise.then(resolve, reject); | ||
| }; |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
use arrow functions and async/await instead of function expressions and .then().
hey! the guidelines say we should stick to arrow functions and async/await. let's swap this out so it's chill and compliant. meow!
🐱 proposed fix to use async/await and arrow function
- // biome-ignore lint/suspicious/noThenProperty: intentional — makes element awaitable
- thenable.then = function (resolve, reject) {
- resolvedPromise ??= resolver(this as VargElement<T>).then((r) => {
- // Mirror meta onto the original element so the render pipeline
- // treats it as pre-resolved (single generation per element).
- element.meta = r.meta;
- return r;
- });
- return resolvedPromise.then(resolve, reject);
- };
+ // biome-ignore lint/suspicious/noThenProperty: intentional — makes element awaitable
+ thenable.then = async (resolve, reject) => {
+ resolvedPromise ??= (async () => {
+ const r = await resolver(thenable);
+ // Mirror meta onto the original element so the render pipeline
+ // treats it as pre-resolved (single generation per element).
+ element.meta = r.meta;
+ return r;
+ })();
+ try {
+ const r = await resolvedPromise;
+ return resolve ? resolve(r) : r;
+ } catch (e) {
+ if (reject) return reject(e);
+ throw e;
+ }
+ };📝 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
| // biome-ignore lint/suspicious/noThenProperty: intentional — makes element awaitable | |
| thenable.then = function (resolve, reject) { | |
| // Remove .then to prevent double-resolution | |
| delete this.then; | |
| return resolver(this as VargElement<T>).then(resolve, reject); | |
| resolvedPromise ??= resolver(this as VargElement<T>).then((r) => { | |
| // Mirror meta onto the original element so the render pipeline | |
| // treats it as pre-resolved (single generation per element). | |
| element.meta = r.meta; | |
| return r; | |
| }); | |
| return resolvedPromise.then(resolve, reject); | |
| }; | |
| // biome-ignore lint/suspicious/noThenProperty: intentional — makes element awaitable | |
| thenable.then = async (resolve, reject) => { | |
| resolvedPromise ??= (async () => { | |
| const r = await resolver(thenable); | |
| // Mirror meta onto the original element so the render pipeline | |
| // treats it as pre-resolved (single generation per element). | |
| element.meta = r.meta; | |
| return r; | |
| })(); | |
| try { | |
| const r = await resolvedPromise; | |
| return resolve ? resolve(r) : r; | |
| } catch (e) { | |
| if (reject) return reject(e); | |
| throw e; | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/react/elements.ts` around lines 77 - 86, Update the thenable.then
assignment in the element awaitable setup to use an arrow function and
async/await rather than a function expression and chained .then() calls.
Preserve the existing this-based resolver invocation, resolvedPromise caching,
meta mirroring, and resolve/reject behavior.
Source: Coding guidelines
Comment on lines
+59
to
+60
| // biome-ignore lint/suspicious/noExplicitAny: props are validated by each component's own signature | ||
| (props: any) => VargElement | VargNode | Promise<VargElement | VargNode>; |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
avoid using any type.
both of these files use any for the props type, which violates our typescript coding guidelines to be explicit. let's tighten this up with a strict type and drop the biome ignore comments! meow!
src/react/runtime/jsx-runtime.ts#L59-L60: replaceanywithRecord<string, unknown>and remove the biome ignore comment.src/react/runtime/jsx-dev-runtime.ts#L61-L62: replaceanywithRecord<string, unknown>and remove the biome ignore comment.
📍 Affects 2 files
src/react/runtime/jsx-runtime.ts#L59-L60(this comment)src/react/runtime/jsx-dev-runtime.ts#L61-L62
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/react/runtime/jsx-runtime.ts` around lines 59 - 60, Replace the props
parameter type with Record<string, unknown> and remove the biome-ignore comment
in the JSX factory signature in src/react/runtime/jsx-runtime.ts lines 59-60.
Apply the same change to the corresponding JSX factory signature in
src/react/runtime/jsx-dev-runtime.ts lines 61-62; no other behavior changes are
needed.
Source: Coding guidelines
The strawberry-vs-chocolate example (temporary name from PR #156) is not representative content. Its only unique value — proving async JSX components typecheck without @ts-expect-error — is now covered by jsx-async.test.tsx (compile-time via tsc + runtime __lazy wrapping and resolveLazy materialization). Also dropped its entries from run-all.ts / render-all.ts.
- range() (ex-bounds()): audible-content range from silencedetect — sound,
not speech (ambient/footsteps count). JSDoc now states this explicitly.
- speechRange({ pad? }): { start, end } of actual speech from word-level
transcription timings (native ElevenLabs alignment when present, whisper
otherwise, cached). Returns null when the transcript is empty. Optional
pad widens the range for soft trims, clamped to [0, duration].
bounds() had no external callers (introduced on this branch), so the
rename is not a breaking change.
…t, temp file cleanup - Split compile.ts (386 lines) into: constants.ts, helpers.ts, validate.ts, discover.ts, and a thin compile.ts orchestrator (61 lines) - Split execute.ts: extract runStep dispatch into run-step.ts, remove dead transcribe/silence-detect ops from the switch - Rewrite topo-sort.ts from recursive DFS to iterative Kahn algorithm (prevents stack overflow on large plans) - Add temp file cleanup in extractAudio/detectSilence via try/finally (preserves URL inputs, deletes toTempFile outputs on success and failure) - Validate nested providerOptions.varg.provider.generate_audio in compile (prevents audio native from being silently dropped on gateway models) - Await rejects.toThrow in execute test to fix async matcher Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/react/audio-element.ts (1)
106-114: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winmemoized rejection never lets you retry
resolveOncecaches the promise even when it rejects. ifresolveAudioElementfails transiently (ffmpeg blip, network), every laterawait/transcribe()/bounds()on this node replays the same rejection forever. reset on failure so a retry is possible:🔁 proposed fix
const resolveOnce = () => { // Sync element.meta after resolution so the sync getters // (.duration, .words) and renderers see the resolved file. - resolvedPromise ??= resolveAudioElement(element).then((r) => { - element.meta = r.meta; - return r; - }); + resolvedPromise ??= resolveAudioElement(element) + .then((r) => { + element.meta = r.meta; + return r; + }) + .catch((err) => { + resolvedPromise = undefined; + throw err; + }); return resolvedPromise; };meow, small guard, saves a lot of head-scratching.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/react/audio-element.ts` around lines 106 - 114, Update resolveOnce so a rejected resolveAudioElement promise clears resolvedPromise before propagating the error, allowing later calls to retry resolution while preserving memoization for successful results.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/react/audio-element.ts`:
- Around line 106-114: Update resolveOnce so a rejected resolveAudioElement
promise clears resolvedPromise before propagating the error, allowing later
calls to retry resolution while preserving memoization for successful results.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 27e27f1b-2c00-4432-b6a5-0040c4cc7af9
📒 Files selected for processing (18)
src/react/audio-element.test.tssrc/react/audio-element.tssrc/react/examples/async/example_we_want_to_test.tsxsrc/react/examples/async/render-all.tssrc/react/examples/async/run-all.tssrc/react/ir/compile.tssrc/react/ir/constants.tssrc/react/ir/discover.tssrc/react/ir/execute.tssrc/react/ir/helpers.tssrc/react/ir/run-step.tssrc/react/ir/tests/compile.test.tssrc/react/ir/tests/execute.test.tssrc/react/ir/topo-sort.tssrc/react/ir/validate.tssrc/react/jsx-async.test.tsxsrc/react/pipeline/audio.tssrc/react/resolved-element.ts
💤 Files with no reviewable changes (3)
- src/react/examples/async/run-all.ts
- src/react/examples/async/example_we_want_to_test.tsx
- src/react/examples/async/render-all.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/react/resolved-element.ts
- src/react/audio-element.test.ts
- src/react/pipeline/audio.ts
React-style declarative lifecycle callbacks on the Render element:
- onStep(event) — fires for each plan step lifecycle event
- onComplete(result) — fires after successful render with final video
- onError(error) — fires if render fails at any stage (compile/execute/compose)
Hooks are read from element.props in render(), so they work both in
JSX (<Render onStep={...}>) and programmatic (Render({onStep: ...})) usage.
Generated with [Devin](https://devin.ai)
Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…ribe.ts - Split 300-line audio.ts into three focused modules behind a barrel: - extract.ts: audio extraction via backend or local ffmpeg - silence.ts: silence detection + sound bounds computation - transcribe.ts: Groq Whisper transcription with caching - Extract shared extractWordTimings() helper — eliminates duplicate word extraction logic between transcribeAudio() and captions renderer - Fix temp file cleanup in backend path of extractAudio (was leaking temp inputs created by resolvePath) - Add explicit error in detectSilence when cloud backend is active (silencedetect requires local ffmpeg stderr parsing) - Remove unused groqResponseSchema from captions.ts Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Aligns folder naming with the render lifecycle (bottom-up): primitives/ ← 0. low-level ops (ffmpeg, Groq API) renderers/ ← 1. per-element generation ir/ ← 2. compile → execute (planning) render.ts ← 3. top-level orchestrator 'pipeline' described a programming pattern, not a domain or layer. 'primitives' says what these modules are: the lowest-level operations that renderers build upon. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…cription
Previously transcribeAudio() was called without a model, always falling
back to direct Groq whisper-large-v3 — even when a gateway transcription
model was configured via render(tree, { defaults: { transcription: ... } }).
The captions renderer already read ctx.defaults?.transcription, but
audioNode.transcribe() (used by speechRange(), standalone await) did not.
Fix: thread defaults through ResolveContext so audioNode can read it via
getResolveContext() and pass it to transcribeAudio().
Fallback chain is now consistent everywhere:
1. Native word timings (ElevenLabs alignment) — free, from TTS
2. ctx.defaults?.transcription — gateway whisper via render options
3. groq.transcription('whisper-large-v3') — hardcoded direct Groq
Generated with [Devin](https://devin.ai)
Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
render.ts (795 lines) split into:
context-builder.ts — createRenderContext, cache wrapping, pricing,
preview mode, PreparedRender type
flatten.ts — flattenClips, HoistedCaption, DeferredAudio types
render.ts — thin orchestrator (~400 lines, was 795)
withDedup helper (dedup.ts) eliminates 30 lines of copy-paste from
image/video/speech/music renderers:
- pre-resolved short-circuit
- pendingFiles dedup by cache key
- progress task lifecycle (addTask/startTask/completeTask)
inputs.ts consolidates resolveImageInput, resolveVideoInput,
resolveAudioInput — previously duplicated between image.ts and video.ts.
resolveCacheStorage moved from render.ts (parent) to context-builder.ts,
eliminating the last duplicate.
Generated with [Devin](https://devin.ai)
Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
captions.ts (743 lines) split into:
srt.ts (88 lines) — formatTime, convertToSRT, parseSrt, SrtEntry
ass.ts (306 lines) — SubtitleStyle, STYLE_PRESETS, convertSrtToAss,
convertSrtToAssGrouped, formatAssTime, colorToAss, EntryEmojiData
captions.ts (370 lines) — renderCaptions orchestrator + helpers
(resolveSrtContent, buildEmojiOverlays)
Each file has a single responsibility:
srt.ts — SRT format <-> structured data
ass.ts — SRT -> ASS conversion with styles + emoji data
captions.ts — orchestration: transcription, font resolution, emoji overlays
Generated with [Devin](https://devin.ai)
Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
SecurityQQ
changed the title
SecurityQQ
force-pushed
the
feature/ir-compile-audio
branch
2 times, most recently
from
July 21, 2026 06:14
3e5594d to
c5a1807
Compare
refactor(captions): split into srt.ts + ass.ts + captions.ts
Consolidate all test files touched by this PR into a single src/react/tests/ directory so they don't clutter the source tree: tests/audio-element.test.ts (was src/react/audio-element.test.ts) tests/jsx-async.test.tsx (was src/react/jsx-async.test.tsx) tests/captions.test.ts (was src/react/renderers/captions.test.ts) tests/ir-compile.test.ts (was src/react/ir/tests/compile.test.ts) tests/ir-execute.test.ts (was src/react/ir/tests/execute.test.ts) IR tests renamed with ir- prefix for grouping. Imports updated to reflect the new depth (one level deeper). Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
2 of 3 tasks
…tency keys
Three fixes from the ep5 production incident:
1. Captions in trimmed clips (double lines + timing drift):
- flatten.ts: HoistedCaption carries the leaf clip's trim window
(cutFrom/cutTo/duration)
- merge-ass.ts: transformCue — single shift/drop/clamp primitive.
Effective shift = clipStartOffset - cutFrom (was + offset only, so
captions lagged by cutFrom). Cues outside [cutFrom, cutTo] are
dropped; partially-outside cues are clamped, so a whisper tail can
no longer bleed over the next clip's captions (the double-line bug).
Both the single-caption path and mergeAssFiles use it; emoji
overlays get the same treatment; withAudio caption tracks now carry
cutFrom/cutTo into the mix. Clips without trims behave exactly as
before (covered by tests).
2. In-flight resolve dedup (52 jobs for one image):
- memoizedElementResolve in resolve.ts, WeakMap-keyed by element
identity, wraps all standalone resolvers (image/video/speech/music/
talking-head/audio). Graph invariant: one element resolves at most
once no matter how many concurrent paths lead to it. Previously the
nested path (async component -> speechRange -> resolveVideoElement
-> resolveImageElement) bypassed both makeThenable memoization and
the renderers' pendingFiles dedup, so N components sharing one
dependency image fired N parallel generations. Meta write-back keeps
renderers/executePlan/compile seeing resolved elements; rejected
promises stay memoized (no silent re-spend on re-await).
3. Idempotency-Key on job submission (retry storm multiplier):
- varg.ts submitJob sends a stable Idempotency-Key (hash of
capability + canonicalized params), so 429/network retries reference
the same job instead of creating duplicates. Verified live: the API
stores the key on created jobs.
Also: named element type exports (VideoElement/SpeechElement/
ImageElement), key prop in JSX IntrinsicAttributes, and the native-audio
target-API example that now typechecks against them.
Verified against the real API with a cheap flux-schnell reproducer of
the ep5 dependency shape: 4 unique requests -> exactly 4 jobs, shared
nested image generated once, zero rate-limit warnings.
Refs #225
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
src/react/examples/async/native-audio-scene1.tsx (1)
221-221: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueuse an arrow function and an interface.
just a quick tweak to keep things chill with our guidelines—swap this to an arrow function and toss the props into an interface. 🐱
✨ proposed fix
-async function DialogueClip({ vid }: { vid: VideoElement }) { +interface DialogueClipProps { + vid: VideoElement; +} + +const DialogueClip = async ({ vid }: DialogueClipProps) => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/react/examples/async/native-audio-scene1.tsx` at line 221, Update the DialogueClip component to use an arrow-function declaration instead of the current function declaration, and define its vid prop shape in a named interface. Preserve the existing VideoElement type and component behavior.Source: Coding guidelines
src/react/renderers/context-builder.ts (2)
35-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winmissing jsdoc on exported
resolveCacheStorage.file-level comment covers the module but this exported fn has none. per coding guidelines, public functions need jsdoc.
docs fix
+/** Normalize the `cache` render option into a `CacheStorage`, wrapping a dir path in a fileCache. */ export function resolveCacheStorage( cache: string | CacheStorage | undefined, ): CacheStorage | undefined {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/react/renderers/context-builder.ts` around lines 35 - 43, Add JSDoc for the exported resolveCacheStorage function, documenting its accepted cache input and the CacheStorage or undefined result while preserving the existing implementation.Source: Coding guidelines
116-137: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
anyonresultparam.guidelines say avoid
anyin ts/tsx files. the biome-ignore is there but a narrower type (e.g.{ providerMetadata?: { varg?: {...} } }) would keep type safety without much effort.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/react/renderers/context-builder.ts` around lines 116 - 137, Replace the any-typed result parameter in emitPricing with a narrow structural type containing optional providerMetadata, varg, pricing, and jobId fields, preserving the existing optional access and pricing casts while removing the need for the biome-ignore.Source: Coding guidelines
src/ai-sdk/providers/varg.ts (1)
164-171: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick wincomputeIdempotencyKey deserves real jsdoc, not just "exported for tests"
it's the public surface for a dedup-critical hash — worth documenting params/return/hash-collision assumptions instead of the one-liner. the meaty docstring above is actually attached to
canonicalize(internal helper), not this exported fn. as per coding guidelines, "Ensure all public functions and classes have JSDoc comments."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ai-sdk/providers/varg.ts` around lines 164 - 171, Replace the “Exported for tests” comment above computeIdempotencyKey with complete JSDoc documenting the capability and params inputs, the generated idempotency-key return value, and the hash/collision assumptions relevant to deduplication; keep the existing implementation unchanged and ensure the canonicalize documentation remains attached to that internal helper.Source: Coding guidelines
src/react/primitives/silence.ts (1)
61-67: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueoptional nit:
Bun.file(input).delete?.()—.delete()is a real documented BunFile method, so the?.()optional call is unneeded defensive noise. not blocking.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/react/primitives/silence.ts` around lines 61 - 67, Remove the unnecessary optional call from the BunFile cleanup in the !isUrlInput branch, invoking the documented delete method directly while preserving the existing try/catch and ignored cleanup-error behavior.src/react/renderers/flatten.ts (1)
117-122: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueheads up: this mutates the source element tree in place
childClip.children = [...]reassigns the original child's children, so re-runningflattenClipson the same tree (or reusing it elsewhere) would re-inject the container overlays. leaf clips are cloned ({...clipElement, children: kept}) but container child clips aren't. cheap to make it non-mutating if the tree is ever reused. not blocking meow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/react/renderers/flatten.ts` around lines 117 - 122, The child processing loop in flattenClip mutates source container nodes by assigning childClip.children. Clone each child clip, including its updated children array with containerOverlays, and pass the clone to flattenClip so repeated flattening and reuse of the original tree remain non-mutating.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ai-sdk/providers/varg.ts`:
- Around line 202-213: Update the retry-after parsing in the 429 handling block
so a missing or invalid header does not become a finite zero value. Only use the
header-derived delay when the header is present and valid; otherwise preserve
the exponential fallback using Math.min(60_000, 5_000 * 2 ** attempt).
In `@src/react/examples/async/native-audio-scene1.tsx`:
- Line 1: Remove the /** `@jsxImportSource` vargai */ pragma from
native-audio-scene1.tsx, leaving the rest of the example unchanged.
In `@src/react/renderers/captions.ts`:
- Around line 234-291: Replace timestamp-only temporary filenames in
renderCaptions, covering both SRT branches and the existing assPath generation,
with collision-resistant unique names that remain associated with each rendering
context. Preserve ctx.tempFiles registration and all existing output behavior
while ensuring concurrent clip renders cannot overwrite or clean up another
job’s file.
---
Nitpick comments:
In `@src/ai-sdk/providers/varg.ts`:
- Around line 164-171: Replace the “Exported for tests” comment above
computeIdempotencyKey with complete JSDoc documenting the capability and params
inputs, the generated idempotency-key return value, and the hash/collision
assumptions relevant to deduplication; keep the existing implementation
unchanged and ensure the canonicalize documentation remains attached to that
internal helper.
In `@src/react/examples/async/native-audio-scene1.tsx`:
- Line 221: Update the DialogueClip component to use an arrow-function
declaration instead of the current function declaration, and define its vid prop
shape in a named interface. Preserve the existing VideoElement type and
component behavior.
In `@src/react/primitives/silence.ts`:
- Around line 61-67: Remove the unnecessary optional call from the BunFile
cleanup in the !isUrlInput branch, invoking the documented delete method
directly while preserving the existing try/catch and ignored cleanup-error
behavior.
In `@src/react/renderers/context-builder.ts`:
- Around line 35-43: Add JSDoc for the exported resolveCacheStorage function,
documenting its accepted cache input and the CacheStorage or undefined result
while preserving the existing implementation.
- Around line 116-137: Replace the any-typed result parameter in emitPricing
with a narrow structural type containing optional providerMetadata, varg,
pricing, and jobId fields, preserving the existing optional access and pricing
casts while removing the need for the biome-ignore.
In `@src/react/renderers/flatten.ts`:
- Around line 117-122: The child processing loop in flattenClip mutates source
container nodes by assigning childClip.children. Clone each child clip,
including its updated children array with containerOverlays, and pass the clone
to flattenClip so repeated flattening and reuse of the original tree remain
non-mutating.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b2877d36-6590-4e4b-b6cd-85c2ab69201f
📒 Files selected for processing (38)
src/ai-sdk/providers/varg-idempotency.test.tssrc/ai-sdk/providers/varg.tssrc/react/audio-element.tssrc/react/elements.tssrc/react/examples/async/native-audio-scene1.tsxsrc/react/index.tssrc/react/primitives/audio.tssrc/react/primitives/extract.tssrc/react/primitives/silence.tssrc/react/primitives/transcribe.tssrc/react/render.tssrc/react/renderers/ass.tssrc/react/renderers/audio.tssrc/react/renderers/captions.tssrc/react/renderers/context-builder.tssrc/react/renderers/dedup.tssrc/react/renderers/flatten.tssrc/react/renderers/image.tssrc/react/renderers/index.tssrc/react/renderers/inputs.tssrc/react/renderers/merge-ass.test.tssrc/react/renderers/merge-ass.tssrc/react/renderers/music.tssrc/react/renderers/render.tssrc/react/renderers/speech.tssrc/react/renderers/srt.tssrc/react/renderers/video.tssrc/react/resolve-context.tssrc/react/resolve.tssrc/react/runtime/jsx-dev-runtime.tssrc/react/runtime/jsx-runtime.tssrc/react/tests/audio-element.test.tssrc/react/tests/captions.test.tssrc/react/tests/ir-compile.test.tssrc/react/tests/ir-execute.test.tssrc/react/tests/jsx-async.test.tsxsrc/react/tests/resolve-dedup.test.tssrc/react/types.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/react/audio-element.ts
- src/react/index.ts
- src/react/elements.ts
- src/react/render.ts
- src/react/types.ts
- src/react/renderers/audio.ts
Comment on lines
+202
to
+213
| // Rate limited — back off and retry (60 jobs/min window resets quickly). | ||
| if (response.status === 429 && attempt < maxRetries) { | ||
| const retryAfter = Number(response.headers.get("retry-after")); | ||
| const delayMs = Number.isFinite(retryAfter) | ||
| ? retryAfter * 1000 | ||
| : Math.min(60_000, 5_000 * 2 ** attempt); | ||
| console.warn( | ||
| `[varg] rate limited on ${capability}, retrying in ${Math.round(delayMs / 1000)}s (attempt ${attempt + 1}/${maxRetries})`, | ||
| ); | ||
| await new Promise((r) => setTimeout(r, delayMs)); | ||
| continue; | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
backoff fallback never actually triggers when retry-after header is missing
response.headers.get("retry-after") returns null when absent, and Number(null) is 0, not NaN. so Number.isFinite(retryAfter) is true even with no header, and delayMs becomes 0 — meaning every 429 without a retry-after header retries immediately with zero backoff instead of falling into the exponential Math.min(60_000, 5_000 * 2 ** attempt) path. this basically defeats the whole "back off" comment right above it 😬
fix idea
- if (response.status === 429 && attempt < maxRetries) {
- const retryAfter = Number(response.headers.get("retry-after"));
- const delayMs = Number.isFinite(retryAfter)
- ? retryAfter * 1000
- : Math.min(60_000, 5_000 * 2 ** attempt);
+ if (response.status === 429 && attempt < maxRetries) {
+ const retryAfterHeader = response.headers.get("retry-after");
+ const retryAfter = retryAfterHeader != null ? Number(retryAfterHeader) : NaN;
+ const delayMs = Number.isFinite(retryAfter) && retryAfter > 0
+ ? retryAfter * 1000
+ : Math.min(60_000, 5_000 * 2 ** attempt);📝 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
| // Rate limited — back off and retry (60 jobs/min window resets quickly). | |
| if (response.status === 429 && attempt < maxRetries) { | |
| const retryAfter = Number(response.headers.get("retry-after")); | |
| const delayMs = Number.isFinite(retryAfter) | |
| ? retryAfter * 1000 | |
| : Math.min(60_000, 5_000 * 2 ** attempt); | |
| console.warn( | |
| `[varg] rate limited on ${capability}, retrying in ${Math.round(delayMs / 1000)}s (attempt ${attempt + 1}/${maxRetries})`, | |
| ); | |
| await new Promise((r) => setTimeout(r, delayMs)); | |
| continue; | |
| } | |
| // Rate limited — back off and retry (60 jobs/min window resets quickly). | |
| if (response.status === 429 && attempt < maxRetries) { | |
| const retryAfterHeader = response.headers.get("retry-after"); | |
| const retryAfter = retryAfterHeader != null ? Number(retryAfterHeader) : NaN; | |
| const delayMs = | |
| Number.isFinite(retryAfter) && retryAfter > 0 | |
| ? retryAfter * 1000 | |
| : Math.min(60_000, 5_000 * 2 ** attempt); | |
| console.warn( | |
| `[varg] rate limited on ${capability}, retrying in ${Math.round(delayMs / 1000)}s (attempt ${attempt + 1}/${maxRetries})`, | |
| ); | |
| await new Promise((r) => setTimeout(r, delayMs)); | |
| continue; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ai-sdk/providers/varg.ts` around lines 202 - 213, Update the retry-after
parsing in the 429 handling block so a missing or invalid header does not become
a finite zero value. Only use the header-derived delay when the header is
present and valid; otherwise preserve the exponential fallback using
Math.min(60_000, 5_000 * 2 ** attempt).
| @@ -0,0 +1,301 @@ | |||
| /** @jsxImportSource vargai */ | |||
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
remove the jsx import pragma.
bun's parser gets super confused when it sees the raw pragma in comments and treats it like the real deal. our guidelines say to use alternate wording so it doesn't break things. meow! 🐾
🔧 proposed fix
-/** `@jsxImportSource` vargai */
+/** jsx import source directive: vargai */📝 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
| /** @jsxImportSource vargai */ | |
| /** jsx import source directive: vargai */ |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/react/examples/async/native-audio-scene1.tsx` at line 1, Remove the /**
`@jsxImportSource` vargai */ pragma from native-audio-scene1.tsx, leaving the rest
of the example unchanged.
Source: Coding guidelines
SecurityQQ
force-pushed
the
feature/ir-compile-audio
branch
from
July 21, 2026 18:19
b81e2e2 to
13f737e
Compare
Whisper word timestamps are not guaranteed monotonic — in ep5 the word "can't" started at 3.18s while the previous words ended at 3.56/3.72s. Cue generation trusted these timings, producing overlapping Dialogue events that ASS renders stacked: two caption lines on screen at once INSIDE a single clip (distinct from the cross-clip overlap fixed by transformCue — that one was at clip boundaries, this one is anywhere whisper emits a non-monotonic timestamp). monotonizeEntries() clamps each entry's start to the previous entry's end (and end >= start) before grouping, applied in both convertSrtToAss and convertSrtToAssGrouped. Already-monotonic input passes through unchanged. Verified on ep5 (cached re-render): 221 cues, zero overlapping pairs including intra-clip (previous check only counted cross-clip pairs); frame at 00:03 now shows a single caption line.
Whisper is a seq2seq transcriber, not a frame-level speech detector: its word timestamps come from attention alignment, and leading silence/noise frames have no tokens — their attention mass is absorbed by the FIRST word. In ep5 clip 1, "I" got start=0.000 under ~0.9s of footsteps and birdsong, so (a) captions appeared before anyone spoke and (b) speechRange-based auto-trim computed cutFrom=0 and kept the silent lead-in. Both consumers were consistent with each other and equally wrong about the actual audio. - primitives/silence.ts: refineWordTimings(words, silences, duration) — clamp the first word's start to the end of measured leading silence and the last word's end to the start of trailing silence (ffmpeg silencedetect measures signal energy directly and knows the truth whisper lost). Middle words are never touched. No silence -> no-op. - audio-element.ts: all AudioNode analysis is now memoized per node per option set (transcribe / silenceSegments / range / speechRange), so every consumer — DialogueClip's speechRange, Captions, user code — shares one transcription and one silencedetect run. transcribe() applies refinement by default for whisper words (native ElevenLabs alignment is trusted and never refined); refine: false opts out, noiseDb is tunable (default -35dB). Refinement runs on top of the disk cache — raw transcripts stay the cached source of truth, no retranscription. - captions.ts: an AudioNode src now uses the node's own memoized transcribe() instead of re-running whisper, so captions and auto-trim agree on when speech starts. Verified on ep5 (cached re-render, $0, zero rate-limit): first caption cue moved 0.00 -> 0.30 (clip now actually trimmed by speechRange), frame at 0.15s has no captions, frame at 1.0s shows "I really love" during actual speech; 217 cues, zero overlapping pairs; 99 tests green.
Date.now()-only temp paths have millisecond resolution. Once caption transcripts became memoized (previous commit), all 12 clips' ASS/SRT conversions landed in the same millisecond — 12 paths collapsed into 7 files, neighboring clips overwrote each other, and mergeAssFiles composed clip N's captions over clip M's video (three consecutive clips all showed 'If you can't' while the speech said 'between work, bad weather'). The memoization speedup exposed the latent race. uniqueTempPath(prefix, ext) in renderers/utils.ts appends a process-wide counter; all 6 temp-path sites in captions.ts and merge-ass.ts now use it. Regression test: 100 rapid calls produce 100 unique paths. Verified on ep5 (cached re-render): 12 distinct temp files for 12 clips, each clip's first cue now matches its own dialogue.
fix: caption timing in trimmed clips, in-flight resolve dedup, idempotency keys
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/react/primitives/silence.ts`:
- Around line 141-142: Validate options.minWordDuration before applying the
nullish-default or clamping logic in the word-processing function, rejecting
negative and non-finite values. Preserve the existing default for omitted values
and ensure only validated durations reach the refined-word timing calculations.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ce3b2c00-27db-437e-a8c3-805215dd66c7
📒 Files selected for processing (10)
src/react/audio-element.tssrc/react/primitives/audio.tssrc/react/primitives/silence.tssrc/react/renderers/ass.test.tssrc/react/renderers/ass.tssrc/react/renderers/captions.tssrc/react/renderers/merge-ass.test.tssrc/react/renderers/merge-ass.tssrc/react/renderers/utils.tssrc/react/tests/audio-refine.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/react/primitives/audio.ts
- src/react/renderers/merge-ass.test.ts
- src/react/renderers/merge-ass.ts
- src/react/audio-element.ts
- src/react/renderers/ass.ts
- src/react/renderers/captions.ts
Comment on lines
+141
to
+142
| const minWord = options.minWordDuration ?? 0.05; | ||
| const result = words.map((w) => ({ ...w })); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
validate the duration option
a negative value can make a refined word’s start exceed its end when leading silence covers the word. reject negative or non-finite values before clamping.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/react/primitives/silence.ts` around lines 141 - 142, Validate
options.minWordDuration before applying the nullish-default or clamping logic in
the word-processing function, rejecting negative and non-finite values. Preserve
the existing default for omitted values and ensure only validated durations
reach the refined-word timing calculations.
speechRange was built on two unreliable signals: whisper word timings (attention artifacts at boundaries — ep5 clip 4 gave 'If' a 0.05s duration at 0.75s while the voice starts ~1.65s) and broadband silencedetect (cannot tell footsteps/birdsong from speech — clip 1 onset detected at 0.4s instead of 0.92s). Chaining heuristics on top of the wrong signal was a dead end; this measures the right signal. - primitives/speech-activity.ts: detectSpeechActivity() — bandpassed (200-3400 Hz) ffmpeg silencedetect. Speech energy lives in the telephone band; footsteps are low-frequency, birds sit above. One ffmpeg pass, zero new dependencies. invertSilences + mergeActivity (gap-merge for inter-word pauses, min-duration filter for residual bursts) are pure and unit-tested. - alignWordsToActivity(): words say WHAT, activity says WHEN. Leading words lying wholly inside ambience are PUSHED to the voice onset (a clamp cannot fix a word whisper placed entirely before speech); the last word is clamped to the voice offset; middle words untouched; monotonizeEntries cascades downstream. Empty activity -> words unchanged (never worse than whisper). - AudioNode.transcribe(): activity alignment replaces broadband silence refinement as the default, with fallback chain: activity -> broadband refine (empty activity) -> raw (no ffmpeg). Memoized per node like every other analysis; refine: false opts out. - This module is a provider seam: a real VAD (silero-vad/ONNX) can replace the bandpass heuristic behind the same signature. - varg music model: round duration to integer seconds (fractional timeline sums like 88.35s hit the API's unrounded x1000 conversion and ElevenLabs' int_from_float 422; BGM is timeline-trimmed anyway). Verified on ep5 (cached, $0): all 12 clips now have first-cue vs measured voice-onset delta <= 0.05s (was up to ~1s early on clips 1 and 4); video tightened 90.9s -> 88.35s as silent lead-ins are now actually trimmed; 113 tests green.
Async components generate through the standalone resolve path during resolveLazy, which runs BEFORE executePlan — so the executor's `concurrency` cap (default 3) never applied to them. ep5's 12 DialogueClips each awaiting vid.audio.speechRange() fanned out ~36-42 simultaneous API calls against a 60/min window, then sustained a 429 retry storm. PR #226's dedup removed the DUPLICATE calls (52 jobs for one shared card); the UNIQUE ones were still unbounded, so any graph with >60 unique generations reproduces the storm. resolveLazy now honors the same `concurrency` knob and default as executePlan. The unbounded phase was a bypass, not a feature. - cache.ts: WithCacheOptions.limit, applied around the underlying call only. Cache HITS never queue for a slot — otherwise a fully cached render would serialize its cache reads behind a semaphore that exists to protect API traffic. Omitting `limit` leaves behavior unchanged, so the executor path (context-builder) is untouched: double-limiting the same work would halve effective throughput. - resolve-context.ts: ResolveContext.limit + getActiveLimit(), with a module-level pLimit(3) fallback for top-level `await Image()` outside render() (a script doing Promise.all([...100 images]) has the same fan-out with no RenderOptions to configure). Lives here, not in resolve.ts, so primitives/ can use it without an import cycle. - resolve.ts: limit threaded through the video/music/speech cached wrappers and the raw generateImage call. - primitives/transcribe.ts: transcribeAudio hand-rolls its cache instead of using withCache, so the limit is applied directly. This was the third leg of the ep5 fan-out (12 parallel /v2/transcription). - renderers/utils.ts: resolveConcurrency() extracted so the limiter and the clip-render pMap reject a bad value with the SDK's own message rather than p-limit's TypeError, whichever phase reaches it first. Only leaf network calls are wrapped, never whole resolvers: a resolver holding a slot while awaiting a nested dependency queued behind it would deadlock. Every resolver already materializes its deps before generating; resolve-concurrency.test.ts pins this with a nested image-in-video graph at concurrency 1. Tests: 11 new (6 resolve-concurrency, 5 cache — withCache had no test file). Full src/react + src/ai-sdk run: 433 -> 444 pass, failures unchanged at 118 pre-existing (opentype/font-metrics, unrelated). No new dependency: p-limit@6.2.0 was already direct (storage/retry.ts). Refs #225
fix: bound generation concurrency during the resolveLazy phase
…xtures
opentype.js 1.3.5 (published 2026-04-29 — a v2 codebase republished onto
the 1.x line) replaced load/loadSync with stubs that print a deprecation
notice and return undefined. `^1.3.4` accepted it silently around
2026-05-23.
loadFont() cached that undefined, so getFontMetrics() THREW rather than
degrading:
TypeError: undefined is not an object (evaluating 'font.tables')
That call sits at captions.ts:125 behind `srtHasEmoji`, so every caption
render containing an emoji has been crashing for ~2.5 months. tsc could
not catch it: @types/opentype.js still declares `loadSync(): Font`, and
CI only ran `biome check && tsc --noEmit` — no test step at all.
- text-measure.ts: use opentype.parse() on an explicit buffer, sliced by
byteOffset/byteLength (Node pools small Buffers; passing .buffer raw
hands the parser neighbouring allocations). Added a null guard so a
future stub fails at the source instead of 300 lines downstream.
parse() behaves identically on 1.3.4/1.3.5/2.0.0 — verified
unitsPerEm 1000, usWinAscent 1109, getAdvanceWidth(" ",72) 20.376.
- package.json: pin opentype.js to 1.3.4 (drop caret). 2.0.0 is NOT the
fix — it has the same loadSync stub AND ships no types (@types tops
out at the v1 API).
- Refreshed the safeGetGlyphs comment: Arabic GSUB still throws on 2.0.0
(lookup type 6/substFormat 1 vs 1.3.4's 5/3), so the fallback stays.
Test fixtures: media/ and output/ are gitignored, so ~43 tests could
only pass on a machine that had run the paid example scripts.
- src/tests/fixtures.ts: shared downloader pulling from s3.varg.ai/
test-media/ into /tmp (mirrors the font pattern in text-measure.test).
Uploaded test-red/green/blue.png (1.8 KB total) to unblock it.
- cyberpunk-street.png / kirill-voice.mp3 / duet-mixed.mp4 exist nowhere
(gitignored AND 404). Rather than skip, the tests now use equivalents:
an ffmpeg-generated silent MP3, a frame extracted from the landscape
video, and VIDEO_2 (which also carries audio, what keepSourceAudio
needs). None of those tests asserted on content.
- react.test.ts: the Captions test now writes its own SRT — it is plain
text and never needed a binary fixture.
- warnings.test.ts: assert child exitCode === 0 before content
assertions, so a subprocess crash reports as a crash rather than a
confusing assertion diff.
- Raised timeouts on two landscape-into-portrait zoompan tests: ~20s is
inherent to cover-mode upscaling (measured identical at 640x360,
960x540 and 1280x720), not a fixture artifact.
editly issues #62 and #123 have REGRESSED and are marked test.failing.
These tests died on a missing fixture long before reaching their
assertions, which is why nobody noticed. With fixtures restored: an
otherwise identical 3-clip composition renders 5s when the middle clip
is fill-color but 4s when it holds positioned videos, and a clip whose
only layer is a positioned video still throws "produced no video
output". test.failing (not skip) keeps the correct expectation encoded
and fails loudly once the bug is fixed.
CI: added a `test` job running `bun run test:ci` — 437 hermetic tests in
~1.7s, no API keys or paid generation. Excludes network/live-service
suites, heavy ffmpeg rendering, and pricing.test.ts (3 failures that
predate this change). Note `bun run check` already fails on main from
pre-existing lint errors, unrelated to this commit.
Result: 118 failures -> 0. 433 -> 562 passing, skips unchanged at 34.
fix: repair font loading broken by opentype.js 1.3.5 (emoji captions crash in prod)
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
Comprehensive refactor of the SDK's
src/react/layer — compilation IR, render lifecycle, and renderer modularization.IR / computation graph
ir/compile.tsinto focused modules:constants.ts,helpers.ts,validate.ts,discover.ts,topo-sort.ts,run-step.tstopo-sort.tsto use Kahn's iterative algorithm (avoids stack overflow on large plans)compile.tsis now an orchestrator;execute.tsis a pure schedulerir/tests/subfolderRender lifecycle
render()now executes a compiled plan (render = compile + execute)onStep/onComplete/onErrorlifecycle hooks to<Render>propsspeechRange()to audio nodes; renamedbounds()→range()Audio primitives (pipeline/ → primitives/)
pipeline/audio.tsintoextract.ts,silence.ts,transcribe.tspipeline/→primitives/to reflect its role as low-level opsaudioNode.transcribe()to respectRenderOptions.defaults.transcription(was hardcoded to direct Groq, skipping gateway whisper)Renderers split
render.ts(795 lines) →context-builder.ts(187) +flatten.ts(118) +render.ts(400, thin orchestrator)captions.ts(743 lines) →srt.ts(88) +ass.ts(306) +captions.ts(370)withDeduphelper — eliminates 30 lines of copy-paste from image/video/speech/music renderersresolveImageInput/resolveAudioInput/resolveVideoInputintoinputs.tsTest plan
tsc --noEmitcleanGenerated with Devin