Skip to content

fix(cli): preserve subtitle cues across line endings - #5223

Merged
miguel-heygen merged 5 commits into
heygen-com:mainfrom
user-github-me:fix/cli-subtitle-line-endings
Oct 9, 2026
Merged

miguel-heygen merged 5 commits into
heygen-com:mainfrom
user-github-me:fix/cli-subtitle-line-endings

Conversation

@user-github-me

Copy link
Copy Markdown
Contributor

Importing an SRT or WebVTT file with Windows CRLF line endings merges every cue into the first entry: the next cue's index and timing line become caption text, and its timing is lost. CR-only files import no cues.

Normalize CRLF and CR to LF before scanning SRT cue boundaries or stripping WebVTT headers. This preserves the existing cue text, stable IDs and timestamps across newline forms.

Validation:

  • Five fail-first regression tests cover both formats, multiline/tagged text, round-trip export and mixed newline forms.
  • All 219 transcription/Whisper tests pass (one existing skip), using a canonical macOS temporary path to avoid unrelated /var versus /private/var path assertions.
  • Built CLI imports the same two cues from six LF/CRLF/CR fixtures and exports identical SRT sidecars. Both CRLF formats also patch the two correct entries into caption source, stay byte-stable on repeat import, and export the expected WebVTT.
  • Native Chromium's text-track parser reads the same WebVTT cues and timestamps for all three newline forms.
  • CLI build/typecheck, repository lint, formatting, commit hooks and comment checks pass.

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SRT and WebVTT files with Windows line endings do merge every cue into the first one today, because cue blocks are split on a bare blank line. Normalizing CRLF and CR before parsing fixes that without changing the result for LF files, and the new tests fail on main and pass here. Heads up: this conflicts textually with #5233 on the parseVtt header line and with #5224 in the test file, so whichever lands second needs a small rebase.

— Rames

@jrusso1020
jrusso1020 enabled auto-merge October 8, 2026 19:56
@jrusso1020
jrusso1020 added this pull request to the merge queue Oct 8, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Oct 8, 2026
@miguel-heygen

Copy link
Copy Markdown
Collaborator

Thanks for this fix, it is approved. main has moved since: #5224, #5226, #5233 and #5236 all touched packages/cli/src/whisper/normalize.ts and its test, so the branch now conflicts. main still has no CR/CRLF handling in the SRT/VTT parsers, so the fix is still needed. Could you merge the latest main into this branch? Once CI is green on the new head we will take it through the merge queue.

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve at 693b197f. CRLF and CR line endings in SRT and VTT still split into separate cues after the merge from main, and CI is green.

What changed since my approval at bddc429a

  • Main changed parseVtt underneath this PR. It no longer strips HEADER: value lines (#5233 keeps speaker labels), and it adds timestamp handling before cue settings (#5224).
  • The merge kept main's behaviour and adds only the normalization: content.replace(/\r\n?/g, "\n"), applied before the WEBVTT header strip and before the SRT block split (normalize.ts:193, :230). Doing it at the parser entry means the blank-line split, the header regex and the per-line split all see \n only.
  • The tests are wrapped in a subtitle line endings describe block next to main's new suites. None of main's tests were removed.
  • The diff against main is just normalize.ts (2 lines) and normalize.test.ts.

Tests: I couldn't run normalize.test.ts locally: the borrowed dependencies here are too old for current parsers, which is an environment problem. CI ran it at this head, and the required Test and Windows lanes pass. oxfmt --check is clean on both files.

— Rames

@miguel-heygen
miguel-heygen added this pull request to the merge queue Oct 9, 2026
Merged via the queue into heygen-com:main with commit 9c1d3fb Oct 9, 2026
81 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants