Add support for basic SRT subtitle import to Captions panel#756
Merged
mazeincoding merged 12 commits intoOpenCut-app:mainfrom Apr 14, 2026
Merged
Add support for basic SRT subtitle import to Captions panel#756mazeincoding merged 12 commits intoOpenCut-app:mainfrom
mazeincoding merged 12 commits intoOpenCut-app:mainfrom
Conversation
[Modified from 0bd641b, 2026-03-28 08:46:23 -0400] Parse .srt files into chunks and insert them as text elements on a new text track using the existing caption styling defaults. Displays a warning when malformed cues are skipped during import.
[Modified from eece3bb, 2026-03-28 09:13:18 -0400] Batch caption insertion in a BatchCommand so that undo will batch undo all subtitle insertion from either srt file or ai transcriptions.
[Modified from e84404b, 2026-03-28 09:52:31 -0400] Extracted the srt specific extraction behind a generic shared interface to allow easier adding of other subtitle formats in the future.
[Modified from b5deec4, 2026-03-31 11:49:34 -0400] Shrink font size, aim to layout 5% above bottom, keep subtitle width 80% of video, wrap text on new lines if too long.
Split captions panel into Generate and Import tabs as they are two different approaches for adding captions.
|
@kcfancher is attempting to deploy a commit to the OpenCut OSS Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds subtitle import to the Captions panel (Import action/button and hidden file input), SRT/ASS parsers and types, measured/wrapped subtitle element builder, and batched insertion of parsed cues as a text track via a new insert helper. Also replaces local processing state with a reducer. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CaptionsUI as Captions Panel
participant FileInput as File Input
participant Parser as parseSubtitleFile
participant Editor as Editor Core
participant Builder as buildSubtitleTextElement
User->>CaptionsUI: Open Import / choose file
User->>FileInput: Select .srt/.ass
FileInput->>CaptionsUI: fileName + content
CaptionsUI->>Parser: parseSubtitleFile(fileName, content)
Parser-->>CaptionsUI: {captions, skippedCueCount, warnings}
alt warnings or skipped cues
CaptionsUI->>User: Display warnings/skipped count
end
User->>CaptionsUI: Insert captions
CaptionsUI->>Editor: insertCaptionChunksAsTextTrack(captions)
Editor->>Editor: AddTrackCommand -> create text track (trackId)
loop per caption
Editor->>Builder: buildSubtitleTextElement(index, caption, canvasSize)
Builder-->>Editor: CreateTextElement
Editor->>Editor: InsertElementCommand (trackId)
end
Editor->>Editor: BatchCommand -> execute
Editor-->>CaptionsUI: trackId
CaptionsUI->>User: Confirm insertion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
🧹 Nitpick comments (2)
apps/web/src/components/editor/panels/assets/views/captions.tsx (2)
253-261: Add explicittype="button"here as well.Same as above—explicit
type="button"for consistency and to prevent unintended behavior if this component is ever placed inside a form.Suggested change
<Button + type="button" variant="outline" className="mt-auto w-full" onClick={handleImportClick} disabled={isProcessing} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/editor/panels/assets/views/captions.tsx` around lines 253 - 261, The Button used for importing captions should explicitly set type="button" to avoid accidental form submissions if this component is rendered inside a form; update the Button element that uses onClick={handleImportClick} (and references isProcessing and processingStep) to include type="button" alongside its existing props.
226-233: Add explicittype="button"to Button elements.Per coding guidelines, button elements should always include a
typeattribute to prevent accidental form submissions. While theButtoncomponent may default totype="button", explicitly setting it improves clarity.Suggested change
<Button + type="button" className="mt-auto w-full" onClick={handleGenerateTranscript} disabled={isProcessing} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/editor/panels/assets/views/captions.tsx` around lines 226 - 233, The Button rendering that triggers handleGenerateTranscript (the JSX using <Button className="mt-auto w-full" onClick={handleGenerateTranscript} disabled={isProcessing}>) should include an explicit type attribute to avoid accidental form submissions; update that Button to add type="button" (while preserving className, onClick, disabled and the Spinner/label logic using isProcessing and processingStep).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/web/src/components/editor/panels/assets/views/captions.tsx`:
- Around line 253-261: The Button used for importing captions should explicitly
set type="button" to avoid accidental form submissions if this component is
rendered inside a form; update the Button element that uses
onClick={handleImportClick} (and references isProcessing and processingStep) to
include type="button" alongside its existing props.
- Around line 226-233: The Button rendering that triggers
handleGenerateTranscript (the JSX using <Button className="mt-auto w-full"
onClick={handleGenerateTranscript} disabled={isProcessing}>) should include an
explicit type attribute to avoid accidental form submissions; update that Button
to add type="button" (while preserving className, onClick, disabled and the
Spinner/label logic using isProcessing and processingStep).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 838b65e4-db65-4028-8096-48883624e38b
📒 Files selected for processing (5)
apps/web/src/components/editor/panels/assets/views/captions.tsxapps/web/src/lib/subtitles/build-subtitle-text-element.tsapps/web/src/lib/subtitles/insert.tsapps/web/src/lib/subtitles/parse.tsapps/web/src/lib/subtitles/srt.ts
Extend subtitle import to accept .ass files alongside .srt, map supported .ass styles onto imported text elements, and display warnings when unsupported .ass features are skipped.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
apps/web/src/components/editor/panels/assets/views/captions.tsx (2)
97-106: Consider more specific error message.When
insertCaptionChunksAsTextTrackreturns notrackId, the error "No captions were generated" is slightly misleading for the import flow since captions were parsed but insertion failed. The message is accurate for the generate flow.♻️ Proposed fix with context-aware error
-const insertCaptionChunks = ({ captions }: { captions: CaptionChunk[] }) => { +const insertCaptionChunks = ({ + captions, + errorMessage = "No captions were generated", +}: { + captions: CaptionChunk[]; + errorMessage?: string; +}) => { const trackId = insertCaptionChunksAsTextTrack({ editor, captions, }); if (!trackId) { - throw new Error("No captions were generated"); + throw new Error(errorMessage); } };Then call with appropriate message:
// In handleImportFile: insertCaptionChunks({ captions: result.captions, errorMessage: "Failed to insert imported subtitles" });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/editor/panels/assets/views/captions.tsx` around lines 97 - 106, Update insertCaptionChunks to accept an optional errorMessage string and use that when throwing the Error if insertCaptionChunksAsTextTrack returns no trackId (change signature of insertCaptionChunks to ({ captions, errorMessage }:{ captions: CaptionChunk[]; errorMessage?: string })). Replace the current generic throw new Error("No captions were generated") with throw new Error(errorMessage ?? "No captions were generated") and update callers (e.g., handleImportFile) to pass a contextual message like "Failed to insert imported subtitles" when the failure is in the import flow.
275-283: Potential duplicate key issue in warnings list.Using the warning message string as the
keywill cause React key collisions if the same warning appears multiple times. Consider using the index or a unique identifier.♻️ Proposed fix using index as key
<ul className="text-sm text-amber-700 space-y-1"> - {warnings.map((warning) => ( - <li key={warning}>{warning}</li> + {warnings.map((warning, index) => ( + <li key={index}>{warning}</li> ))} </ul>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/editor/panels/assets/views/captions.tsx` around lines 275 - 283, The list uses the warning string as the React key in warnings.map((warning) => <li key={warning}>…), which can collide when identical messages appear; update the map to include a stable unique key (e.g., use the index or combine the message with the index) such as warnings.map((warning, i) => <li key={`${warning}-${i}`}>…) to guarantee uniqueness and avoid React key collisions in the warnings render block.apps/web/src/lib/subtitles/srt.ts (1)
27-30: Single-line blocks are silently skipped.Blocks with only one line (e.g., just a cue index or just text) are counted as skipped. This is correct behavior, but the user won't know why these blocks were skipped since
warningsis always empty for SRT files. Consider whether SRT parsing should also populate warnings for better user feedback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/subtitles/srt.ts` around lines 27 - 30, The parser currently silently increments skippedCueCount when a block has lines.length < 2; update the SRT parsing path (where lines and skippedCueCount are used) to also append a descriptive warning to the warnings array explaining why the block was skipped (include the offending block text or its cue index if available), so users receive feedback for single-line blocks instead of silent skips.apps/web/src/lib/subtitles/ass.ts (1)
302-316: Type assertion bypasses type safety.The
mapFieldsToRecordfunction usesas Twhich bypasses TypeScript's type checking. While this is acceptable for internal use with known field structures, the return type doesn't guarantee all required properties exist.This is a common pattern for dynamic field mapping and is acceptable here since the caller validates required fields (e.g.,
if (!record.name)at line 141). No change needed, but documenting the assumption would help maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/lib/subtitles/ass.ts` around lines 302 - 316, The function mapFieldsToRecord currently returns a forced type (record as T) which bypasses TypeScript safety; add a short inline comment above mapFieldsToRecord documenting the assumption that callers validate required fields (e.g., checks like if (!record.name) used elsewhere) and that the function intentionally returns a partial record of string|undefined for dynamic ASS field mapping, or alternatively change the return type to Partial<Record<string,string|undefined>>/Record<string,string|undefined> and keep callers validating required properties (refer to mapFieldsToRecord and its callers that perform checks such as the name existence check).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/web/src/components/editor/panels/assets/views/captions.tsx`:
- Around line 97-106: Update insertCaptionChunks to accept an optional
errorMessage string and use that when throwing the Error if
insertCaptionChunksAsTextTrack returns no trackId (change signature of
insertCaptionChunks to ({ captions, errorMessage }:{ captions: CaptionChunk[];
errorMessage?: string })). Replace the current generic throw new Error("No
captions were generated") with throw new Error(errorMessage ?? "No captions were
generated") and update callers (e.g., handleImportFile) to pass a contextual
message like "Failed to insert imported subtitles" when the failure is in the
import flow.
- Around line 275-283: The list uses the warning string as the React key in
warnings.map((warning) => <li key={warning}>…), which can collide when identical
messages appear; update the map to include a stable unique key (e.g., use the
index or combine the message with the index) such as warnings.map((warning, i)
=> <li key={`${warning}-${i}`}>…) to guarantee uniqueness and avoid React key
collisions in the warnings render block.
In `@apps/web/src/lib/subtitles/ass.ts`:
- Around line 302-316: The function mapFieldsToRecord currently returns a forced
type (record as T) which bypasses TypeScript safety; add a short inline comment
above mapFieldsToRecord documenting the assumption that callers validate
required fields (e.g., checks like if (!record.name) used elsewhere) and that
the function intentionally returns a partial record of string|undefined for
dynamic ASS field mapping, or alternatively change the return type to
Partial<Record<string,string|undefined>>/Record<string,string|undefined> and
keep callers validating required properties (refer to mapFieldsToRecord and its
callers that perform checks such as the name existence check).
In `@apps/web/src/lib/subtitles/srt.ts`:
- Around line 27-30: The parser currently silently increments skippedCueCount
when a block has lines.length < 2; update the SRT parsing path (where lines and
skippedCueCount are used) to also append a descriptive warning to the warnings
array explaining why the block was skipped (include the offending block text or
its cue index if available), so users receive feedback for single-line blocks
instead of silent skips.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2e66d122-4f29-4a02-9533-87a3a52f4950
📒 Files selected for processing (7)
apps/web/src/components/editor/panels/assets/views/captions.tsxapps/web/src/lib/subtitles/ass.tsapps/web/src/lib/subtitles/build-subtitle-text-element.tsapps/web/src/lib/subtitles/insert.tsapps/web/src/lib/subtitles/parse.tsapps/web/src/lib/subtitles/srt.tsapps/web/src/lib/subtitles/types.ts
✅ Files skipped from review due to trivial changes (1)
- apps/web/src/lib/subtitles/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/lib/subtitles/insert.ts
# Conflicts: # apps/web/src/components/editor/panels/assets/views/captions.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/web/src/components/editor/panels/assets/views/captions.tsx (1)
86-90: Avoidconsole.errorin these client-side failure paths.These handlers already surface the error through component state, so shipping raw console logging here just adds noisy browser output. If diagnostics are needed, please send them through the app’s logger/telemetry path instead.
As per coding guidelines, "Don't use console".
Also applies to: 143-147
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/editor/panels/assets/views/captions.tsx` around lines 86 - 90, Remove the console.error call in the catch block and route the error through the app's telemetry/logger instead while keeping the existing setError(state) behavior; specifically, in the catch in captions.tsx that currently does console.error("Transcription failed:", error) replace that call with the app logging API your app uses (e.g. appLogger.error or telemetry.trackError) and include the same context string plus the error object, and make the same replacement for the similar catch block around lines 143–147 so all client-side failures use the centralized logger rather than console.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/components/editor/panels/assets/views/captions.tsx`:
- Around line 39-45: The shared transient UI state (isProcessing,
processingStep, error, warnings) currently spans both CaptionsView tabs causing
bleed when switching views; change these to be scoped per-tab (e.g., maintain a
map keyed by view or separate state objects like generateState/importState) and
update usages in handlers (where setIsProcessing, setProcessingStep, setError,
setWarnings are called) to read/write the state for the active view (use
view/setView to choose key), or alternatively disable/lock tab switching while
any of those flags for the active workflow are true; update any other instances
noted around lines 185-193 to follow the same per-tab pattern.
- Around line 197-200: Add unit and integration tests that cover ASS and SRT
parsing and the subtitle import/export flow before enabling those formats in the
UI: write tests exercising parseAss (in apps/web/src/lib/subtitles/ass.ts) and
the SRT parser (parseSrt or equivalent), including valid files, malformed
inputs, edge cases (overlapping timestamps, no events, non-UTF8/encoding), and
error paths, then add integration tests that simulate the file input flow used
by the component (the handler invoked via fileInputRef/File input in
captions.tsx) to assert correct parsing, validation, and round‑trip export
behavior; use the existing test framework (Jest/Testing Library) and assert both
successful parsed output shapes and that failures produce the expected
errors/validation results.
---
Nitpick comments:
In `@apps/web/src/components/editor/panels/assets/views/captions.tsx`:
- Around line 86-90: Remove the console.error call in the catch block and route
the error through the app's telemetry/logger instead while keeping the existing
setError(state) behavior; specifically, in the catch in captions.tsx that
currently does console.error("Transcription failed:", error) replace that call
with the app logging API your app uses (e.g. appLogger.error or
telemetry.trackError) and include the same context string plus the error object,
and make the same replacement for the similar catch block around lines 143–147
so all client-side failures use the centralized logger rather than console.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e3101364-70aa-44b5-82ed-2e260d8c6134
📒 Files selected for processing (1)
apps/web/src/components/editor/panels/assets/views/captions.tsx
Comment on lines
+197
to
+200
| <input | ||
| ref={fileInputRef} | ||
| type="file" | ||
| accept=".srt,.ass" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== ASS parser/import references ==="
rg -n -C2 '\bparseAss\b|\.ass\b' apps/web/src
echo
echo "=== Tests covering subtitle parsing/import ==="
fd -i 'test|spec' apps/web/src | xargs -r rg -n -C2 '\bparseAss\b|parseSubtitleFile|\.ass\b|\.srt\b'Repository: OpenCut-app/OpenCut
Length of output: 2130
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Checking for test files in the repo ==="
fd -i 'test|spec' apps/web/src --type f | head -20
echo
echo "=== Full ASS parser implementation ==="
sed -n '61,120p' apps/web/src/lib/subtitles/ass.ts
echo
echo "=== Check for TODO/FIXME markers in subtitle code ==="
rg -n 'TODO|FIXME|XXX|WIP' apps/web/src/lib/subtitles/ || echo "No markers found"
echo
echo "=== Check if ASS support is explicitly mentioned as incomplete ==="
rg -n 'ass|ASS' apps/web/src/lib/subtitles/ | grep -i 'todo\|fixme\|wip\|incomplete\|phase' || echo "No phase/incomplete markers"Repository: OpenCut-app/OpenCut
Length of output: 3069
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check git log for ASS-related commits ==="
git log --oneline --all --grep='ass\|subtitle' -i | head -20
echo
echo "=== Check if there's a test file specifically for subtitles ==="
fd -type f subtitle apps/web/src
echo
echo "=== Check the full parseSubtitleFile function that routes to parseAss ==="
sed -n '1,30p' apps/web/src/lib/subtitles/parse.ts
echo
echo "=== Check if ASS type/format detection is defined ==="
rg -n 'type.*[Aa]ss|format.*[Aa]ss' apps/web/src/lib/subtitles/
echo
echo "=== Verify parseAss is being exported and used ==="
rg -n 'parseAss' apps/web/src --type ts --type tsxRepository: OpenCut-app/OpenCut
Length of output: 1340
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for subtitle-related test files ==="
find apps/web/src -path '*subtitles*' -o -path '*caption*' | grep -i test || echo "No subtitle tests found"
echo
echo "=== Check when ASS parsing was added to the codebase ==="
git log --all --oneline -- apps/web/src/lib/subtitles/ass.ts | head -5
echo
echo "=== Check the full captions.tsx file context around lines 197-200 and 256-258 ==="
sed -n '190,210p' apps/web/src/components/editor/panels/assets/views/captions.tsx
sed -n '250,260p' apps/web/src/components/editor/panels/assets/views/captions.tsx
echo
echo "=== Check if this is a new file or modified in current changes ==="
git diff HEAD~1 apps/web/src/lib/subtitles/ass.ts 2>/dev/null | head -50 || echo "File not recently changed or no git history available"Repository: OpenCut-app/OpenCut
Length of output: 1320
Add test coverage for ASS subtitle parsing before exposing it to users.
ASS parsing is fully implemented (parseAss in apps/web/src/lib/subtitles/ass.ts), but there are no tests for ASS parsing, or SRT parsing, or subtitle import/export at all. Exposing an untested code path creates risk: validation logic, edge cases, and error handling may not work as expected. Add tests covering both SRT and ASS formats before accepting these formats in the UI.
Also applies to: 256-258
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/editor/panels/assets/views/captions.tsx` around lines
197 - 200, Add unit and integration tests that cover ASS and SRT parsing and the
subtitle import/export flow before enabling those formats in the UI: write tests
exercising parseAss (in apps/web/src/lib/subtitles/ass.ts) and the SRT parser
(parseSrt or equivalent), including valid files, malformed inputs, edge cases
(overlapping timestamps, no events, non-UTF8/encoding), and error paths, then
add integration tests that simulate the file input flow used by the component
(the handler invoked via fileInputRef/File input in captions.tsx) to assert
correct parsing, validation, and round‑trip export behavior; use the existing
test framework (Jest/Testing Library) and assert both successful parsed output
shapes and that failures produce the expected errors/validation results.
Contributor
Author
|
I've fixed the latest merge conflicts and added preliminary support for the |
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
2 participants
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.
Phase one support for #749
Changelog
.srtimport support for generating text elementsNotes
.srtsupport is implemented in this PR, but parsing interface is generic to allow future formats..ass.Summary by CodeRabbit
Update
.asssubtitle support for the styles that the OpenCut editor currently supports.assSupport