Repository navigation
refactor(captions): split into srt.ts + ass.ts + captions.ts - #224
Conversation
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>
📝 WalkthroughWalkthroughadds derived, memoized audio nodes and audio analysis; introduces a compiled render-plan ir with dependency-aware execution; integrates audio into composition and captions; expands public exports and async jsx support; and hardens provider request handling. Changesreact audio and render pipeline
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
3e5594d to
c5a1807
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/react/index.ts (1)
6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuegroup audio exports.
might be nice to move this down near the other audio primitive exports so they hang out together. meow! just a tiny thought.
🤖 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/index.ts` at line 6, Move the AudioNode type export in the module’s export list so it is adjacent to the other audio primitive exports, keeping the export unchanged and grouping related audio symbols together.src/react/renderers/context-builder.ts (1)
35-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winmissing jsdoc on exported
resolveCacheStoragemeow, small one — this exported fn has no doc comment, unlike
createRenderContextright below it.as per coding guidelines, "Ensure all public functions and classes have JSDoc comments."
✏️ suggested fix
+/** Resolve the render option's `cache` value into a `CacheStorage`, or `undefined` if caching is disabled. */ 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 a JSDoc comment directly above the exported resolveCacheStorage function, documenting its purpose, accepted cache argument forms, and returned CacheStorage behavior, consistent with the existing documentation style for createRenderContext.Source: Coding guidelines
src/react/elements.ts (1)
96-114: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuecast drops talking-head from the union, harmless but sloppy
attachAudioGetter's generic acceptsvideo | speech | talking-head, but themakeAudioNode({ parent: element as VargElement<"video"> | VargElement<"speech"> })cast silently narrows it back out. no current caller passes talking-head here so it's not live, but it's misleading givenAudioElementProps.parentexplicitly supports talking-head too. see companion note onresolved-element.ts— same pattern there.🤖 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 96 - 114, The parent cast inside attachAudioGetter currently omits talking-head; update it to preserve the full T-supported element union, including VargElement<"talking-head">, when passing element to makeAudioNode. Keep the existing getter behavior and generic return type unchanged.
🤖 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 81-104: Rename the KNOWN_UNDERLYING_PROVIDERS constant to
knownunderlyingproviders and update its reference within
checkVargProviderOptions, preserving the existing provider-option detection
behavior.
In `@src/react/renderers/captions.ts`:
- Around line 236-289: Update the temporary filename generation for both SRT
writes and the assPath construction in the caption rendering flow to append a
per-file random suffix alongside Date.now(). Follow the existing
Date.now()+random approach used by extract.ts so parallel renders cannot share
or overwrite temporary paths, while preserving the existing extensions and
tempFiles tracking.
In `@src/react/renderers/context-builder.ts`:
- Around line 139-181: Move preview placeholder tracking in wrapGenerateImage
and wrapGenerateVideo so trackPlaceholder runs only when the cached generation
actually uses the placeholder fallback, not before cachedGenerateImage or
cachedGenerateVideo can return a cache hit. Preserve the existing preview
middleware behavior and ensure cache hits do not increment placeholderCount.
---
Nitpick comments:
In `@src/react/elements.ts`:
- Around line 96-114: The parent cast inside attachAudioGetter currently omits
talking-head; update it to preserve the full T-supported element union,
including VargElement<"talking-head">, when passing element to makeAudioNode.
Keep the existing getter behavior and generic return type unchanged.
In `@src/react/index.ts`:
- Line 6: Move the AudioNode type export in the module’s export list so it is
adjacent to the other audio primitive exports, keeping the export unchanged and
grouping related audio symbols together.
In `@src/react/renderers/context-builder.ts`:
- Around line 35-43: Add a JSDoc comment directly above the exported
resolveCacheStorage function, documenting its purpose, accepted cache argument
forms, and returned CacheStorage behavior, consistent with the existing
documentation style for createRenderContext.
🪄 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: bd56b3d2-2d30-46e4-8f53-564ea421f3be
📒 Files selected for processing (50)
src/ai-sdk/providers/elevenlabs.tssrc/ai-sdk/providers/varg.tssrc/react/audio-element.test.tssrc/react/audio-element.tssrc/react/elements.tssrc/react/examples/async/example_we_want_to_test.tsxsrc/react/examples/async/render-all.tssrc/react/examples/async/run-all.tssrc/react/index.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/types.tssrc/react/ir/validate.tssrc/react/jsx-async.test.tsxsrc/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.test.tssrc/react/renderers/captions.tssrc/react/renderers/clip.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/music.tssrc/react/renderers/render.tssrc/react/renderers/speech.tssrc/react/renderers/srt.tssrc/react/renderers/talking-head.tssrc/react/renderers/utils.tssrc/react/renderers/video.tssrc/react/resolve-context.tssrc/react/resolve.tssrc/react/resolved-element.tssrc/react/runtime/jsx-dev-runtime.tssrc/react/runtime/jsx-runtime.tssrc/react/types.ts
💤 Files with no reviewable changes (3)
- src/react/examples/async/example_we_want_to_test.tsx
- src/react/examples/async/run-all.ts
- src/react/examples/async/render-all.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/react/index.ts (1)
6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuegroup audio exports.
might be nice to move this down near the other audio primitive exports so they hang out together. meow! just a tiny thought.
🤖 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/index.ts` at line 6, Move the AudioNode type export in the module’s export list so it is adjacent to the other audio primitive exports, keeping the export unchanged and grouping related audio symbols together.src/react/renderers/context-builder.ts (1)
35-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winmissing jsdoc on exported
resolveCacheStoragemeow, small one — this exported fn has no doc comment, unlike
createRenderContextright below it.as per coding guidelines, "Ensure all public functions and classes have JSDoc comments."
✏️ suggested fix
+/** Resolve the render option's `cache` value into a `CacheStorage`, or `undefined` if caching is disabled. */ 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 a JSDoc comment directly above the exported resolveCacheStorage function, documenting its purpose, accepted cache argument forms, and returned CacheStorage behavior, consistent with the existing documentation style for createRenderContext.Source: Coding guidelines
src/react/elements.ts (1)
96-114: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuecast drops talking-head from the union, harmless but sloppy
attachAudioGetter's generic acceptsvideo | speech | talking-head, but themakeAudioNode({ parent: element as VargElement<"video"> | VargElement<"speech"> })cast silently narrows it back out. no current caller passes talking-head here so it's not live, but it's misleading givenAudioElementProps.parentexplicitly supports talking-head too. see companion note onresolved-element.ts— same pattern there.🤖 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 96 - 114, The parent cast inside attachAudioGetter currently omits talking-head; update it to preserve the full T-supported element union, including VargElement<"talking-head">, when passing element to makeAudioNode. Keep the existing getter behavior and generic return type unchanged.
🤖 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 81-104: Rename the KNOWN_UNDERLYING_PROVIDERS constant to
knownunderlyingproviders and update its reference within
checkVargProviderOptions, preserving the existing provider-option detection
behavior.
In `@src/react/renderers/captions.ts`:
- Around line 236-289: Update the temporary filename generation for both SRT
writes and the assPath construction in the caption rendering flow to append a
per-file random suffix alongside Date.now(). Follow the existing
Date.now()+random approach used by extract.ts so parallel renders cannot share
or overwrite temporary paths, while preserving the existing extensions and
tempFiles tracking.
In `@src/react/renderers/context-builder.ts`:
- Around line 139-181: Move preview placeholder tracking in wrapGenerateImage
and wrapGenerateVideo so trackPlaceholder runs only when the cached generation
actually uses the placeholder fallback, not before cachedGenerateImage or
cachedGenerateVideo can return a cache hit. Preserve the existing preview
middleware behavior and ensure cache hits do not increment placeholderCount.
---
Nitpick comments:
In `@src/react/elements.ts`:
- Around line 96-114: The parent cast inside attachAudioGetter currently omits
talking-head; update it to preserve the full T-supported element union,
including VargElement<"talking-head">, when passing element to makeAudioNode.
Keep the existing getter behavior and generic return type unchanged.
In `@src/react/index.ts`:
- Line 6: Move the AudioNode type export in the module’s export list so it is
adjacent to the other audio primitive exports, keeping the export unchanged and
grouping related audio symbols together.
In `@src/react/renderers/context-builder.ts`:
- Around line 35-43: Add a JSDoc comment directly above the exported
resolveCacheStorage function, documenting its purpose, accepted cache argument
forms, and returned CacheStorage behavior, consistent with the existing
documentation style for createRenderContext.
🪄 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: bd56b3d2-2d30-46e4-8f53-564ea421f3be
📒 Files selected for processing (50)
src/ai-sdk/providers/elevenlabs.tssrc/ai-sdk/providers/varg.tssrc/react/audio-element.test.tssrc/react/audio-element.tssrc/react/elements.tssrc/react/examples/async/example_we_want_to_test.tsxsrc/react/examples/async/render-all.tssrc/react/examples/async/run-all.tssrc/react/index.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/types.tssrc/react/ir/validate.tssrc/react/jsx-async.test.tsxsrc/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.test.tssrc/react/renderers/captions.tssrc/react/renderers/clip.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/music.tssrc/react/renderers/render.tssrc/react/renderers/speech.tssrc/react/renderers/srt.tssrc/react/renderers/talking-head.tssrc/react/renderers/utils.tssrc/react/renderers/video.tssrc/react/resolve-context.tssrc/react/resolve.tssrc/react/resolved-element.tssrc/react/runtime/jsx-dev-runtime.tssrc/react/runtime/jsx-runtime.tssrc/react/types.ts
💤 Files with no reviewable changes (3)
- src/react/examples/async/example_we_want_to_test.tsx
- src/react/examples/async/run-all.ts
- src/react/examples/async/render-all.ts
🛑 Comments failed to post (3)
src/ai-sdk/providers/varg.ts (1)
81-104: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
use camelcase for variables.
hey! as per coding guidelines, we gotta use camelcase for variable names. let's swap this over to
knownunderlyingprovidersto keep things super chill and consistent. meow 🐱🐾 proposed fix
-const KNOWN_UNDERLYING_PROVIDERS = new Set([ +const knownUnderlyingProviders = new Set([ "fal", "together", "rendi", "groq", "elevenlabs", "higgsfield", "piapi", "heygen", "replicate", ]); let warnedFlatProviderOptions = false; function checkVargProviderOptions( opts: Record<string, unknown>, ): Record<string, unknown> { const keys = Object.keys(opts); const hasProviderKey = keys.some( (k) => - KNOWN_UNDERLYING_PROVIDERS.has(k) && + knownUnderlyingProviders.has(k) && typeof opts[k] === "object" && opts[k] !== null, );📝 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.const knownUnderlyingProviders = new Set([ "fal", "together", "rendi", "groq", "elevenlabs", "higgsfield", "piapi", "heygen", "replicate", ]); let warnedFlatProviderOptions = false; function checkVargProviderOptions( opts: Record<string, unknown>, ): Record<string, unknown> { const keys = Object.keys(opts); const hasProviderKey = keys.some( (k) => knownUnderlyingProviders.has(k) && typeof opts[k] === "object" && opts[k] !== 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/varg.ts` around lines 81 - 104, Rename the KNOWN_UNDERLYING_PROVIDERS constant to knownunderlyingproviders and update its reference within checkVargProviderOptions, preserving the existing provider-option detection behavior.Source: Coding guidelines
src/react/renderers/captions.ts (1)
236-289: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
temp filenames only use Date.now() — collision risk under parallel render
both srt writes here (lines 236-237 and 287-288), and
assPathup at line 157, key the temp path purely onDate.now(). clips render in parallel (default concurrency 3), so two captions in the same millisecond write to the same/tmp/varg-captions-<ts>.srtand clobber each other → wrong captions burned in.extract.tsalready dodges this with aDate.now()+randomsuffix — do the same here.proposed fix
- const srtPath = `/tmp/varg-captions-${Date.now()}.srt`; + const srtPath = `/tmp/varg-captions-${Date.now()}-${Math.random().toString(36).slice(2)}.srt`;(apply to both srt writes + the assPath at line 157)
🤖 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/captions.ts` around lines 236 - 289, Update the temporary filename generation for both SRT writes and the assPath construction in the caption rendering flow to append a per-file random suffix alongside Date.now(). Follow the existing Date.now()+random approach used by extract.ts so parallel renders cannot share or overwrite temporary paths, while preserving the existing extensions and tempFiles tracking.src/react/renderers/context-builder.ts (1)
139-181: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash rg -n 'placeholderCount' -C3 src/reactRepository: vargHQ/sdk
Length of output: 2886
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '== outlines ==' ast-grep outline src/react/renderers/context-builder.ts --view expanded | sed -n '1,220p' echo ast-grep outline src/react/renderers/render.ts --view expanded | sed -n '1,220p' echo rg -n 'withCache|cacheKey|skipCacheWrite|placeholderFallbackMiddleware|imagePlaceholderFallbackMiddleware|trackPlaceholder|placeholderCount' src/react -C 2Repository: vargHQ/sdk
Length of output: 23700
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' from pathlib import Path for path in [ Path("src/react/renderers/context-builder.ts"), Path("src/react/renderers/render.ts"), ]: print(f"\n== {path} ==") text = path.read_text().splitlines() for i, line in enumerate(text, 1): if any(k in line for k in ["withCache", "cacheKey", "skipCacheWrite", "placeholderFallbackMiddleware", "imagePlaceholderFallbackMiddleware", "trackPlaceholder", "placeholderCount"]): start = max(1, i-6) end = min(len(text), i+8) for j in range(start, end + 1): print(f"{j:4}: {text[j-1]}") print() PYRepository: vargHQ/sdk
Length of output: 12521
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n 'function withCache|const withCache|withCache\(' src -C 3Repository: vargHQ/sdk
Length of output: 14514
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '1,260p' src/ai-sdk/cache.ts | cat -nRepository: vargHQ/sdk
Length of output: 5256
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '1,220p' src/ai-sdk/cache.ts | cat -nRepository: vargHQ/sdk
Length of output: 5256
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' from pathlib import Path text = Path("src/ai-sdk/cache.ts").read_text().splitlines() for i, line in enumerate(text, 1): if 1 <= i <= 220: print(f"{i:4}: {line}") PYRepository: vargHQ/sdk
Length of output: 5110
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '1,260p' src/ai-sdk/middleware.ts | cat -nRepository: vargHQ/sdk
Length of output: 216
preview placeholder count overcounts cache hits
trackPlaceholder(...)runs beforecachedGenerateImage/cachedGenerateVideo, andwithCache(...)returns cached entries without calling the fallback path. in preview mode, a cache hit still bumpsplaceholderCount, so thepreview mode: n placeholders usedbanner can be wrong, 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/context-builder.ts` around lines 139 - 181, Move preview placeholder tracking in wrapGenerateImage and wrapGenerateVideo so trackPlaceholder runs only when the cached generation actually uses the placeholder fallback, not before cachedGenerateImage or cachedGenerateVideo can return a cache hit. Preserve the existing preview middleware behavior and ensure cache hits do not increment placeholderCount.
Summary
Split
captions.ts(743 lines) into three focused modules:srt.ts(88 lines) —formatTime,convertToSRT,parseSrt,SrtEntryass.ts(306 lines) —SubtitleStyle,STYLE_PRESETS,convertSrtToAss,convertSrtToAssGrouped,formatAssTime,colorToAss,EntryEmojiDatacaptions.ts(370 lines) —renderCaptionsorchestrator + helpers (resolveSrtContent,buildEmojiOverlays)Each file now has a single responsibility:
srt.ts— SRT format ↔ structured dataass.ts— SRT → ASS conversion with styles + emoji datacaptions.ts— orchestration: transcription, font resolution, emoji overlaysTest plan
tsc --noEmitpassesbun test src/react/renderers/captions.test.ts— 3 pass, 0 failGenerated with Devin