Untitled

# PR Review — PD-12627: Fix Subtitle Encoding and Parsing for VTT Files

**Branch:** `fix/PD-12627_fix_subtitles_encoding_and_parsing_for_vtt`  
**Target:** `develop`  
**Commit:** `be1ba83b69`  
**Author:** rajesh-fuseuniversal  
**Files changed:** `app/lib/video/captions.rb`, `spec/lib/video/captions_spec.rb`

---

## Summary

The PR fixes subtitle upload failures caused by the caption parsing pipeline only supporting standard SRT input. It adds:

- UTF-8 encoding normalisation before any parsing
- VTT format detection and a dedicated `parse_vtt` path
- Plain-text offset format detection (`HH:MM:SS` + prose) and a dedicated `parse_offset_txt` path
- Unit tests for all three new paths plus the encoding fix

---

## What's Good

- **Encoding normalisation is correct and well-placed.** Calling `force_encoding` then `encode` with replacement handles the full range of binary-encoded uploads (ASCII-8BIT, Latin-1, etc.) without raising. Doing this before format detection means all three parsers receive clean UTF-8.
- **VTT is parsed directly into `SRT::File` objects** rather than converting to an SRT string first. This avoids the `SRT::File.parse_timecode` incompatibility with dot-millisecond separators — a sound architectural choice.
- **`parse_offset_txt` end-time derivation** (using the next block's start time as the current block's end time) is a reasonable heuristic for a format that carries no explicit end times.
- **Trailing cue flush** in `parse_vtt` correctly handles files that don't end with a blank line.
- **`private_class_method`** correctly hides the format-specific parsers from external callers.
- **Tests cover** the happy path for each new format, the encoding fix, and the 30-second fallback for the last offset-txt cue.

---

## Issues Requiring Changes

### 🔴 Critical — Silent data loss for VTT files with `MM:SS.mmm` timestamps

**File:** `app/lib/video/captions.rb`

**The problem:**  
The WebVTT spec allows timestamps in both `HH:MM:SS.mmm` and `MM:SS.mmm` form. The shorter form is the default output of YouTube caption exports, Adobe Premiere, DaVinci Resolve, and most subtitle editors for any video under one hour — making it the most common format in practice.

The timestamp regex in `parse_vtt` only matches `HH:MM:SS.mmm`:

```ruby
# current — rejects MM:SS.mmm
str.match(/^(\d{2}:\d{2}:\d{2})\.(\d{3})\s+-->\s+(\d{2}:\d{2}:\d{2})\.(\d{3})/)
```

A file starting with `WEBVTT` is correctly routed to `parse_vtt`. Inside that method, every cue whose timestamp uses `MM:SS.mmm` fails the regex match. Because `current_line` is `nil` at that point, the `elsif current_line` branch is also skipped. The cue is silently discarded. The result is an empty `SRT::File`, and `[]` is written to `subtitles_files.transcript` with no error raised and no feedback to the user.

`vtt_timecode_to_seconds` has a compounding bug: it destructures with `h, m, s = timecode.split(':').map(&:to_i)`. For a two-part timecode like `00:03`, `s` receives `nil`, which would cause a `NoMethodError` on `nil * 1` even if the regex were relaxed independently.

**Trigger:**  
Upload any `.vtt` file exported from YouTube, Premiere, or DaVinci for a video under one hour.

**What needs to change:**

The regex doesn't need to enumerate every valid timestamp shape — it just needs to capture the digit-colon portion before the dot and let `vtt_timecode_to_seconds` handle the rest:

```ruby
str.match(/^([\.\d:]+)\.(\d{3})\s+-->\s+([\d:]+)\.(\d{3})/)
```

Wait — even simpler, just match any `\d` and `:` sequence before the dot:

```ruby
str.match(/^([\d:]+)\.(\d{3})\s+-->\s+([\d:]+)\.(\d{3})/)
```

This matches both `00:01.000` and `00:00:01.000` without any alternation. `vtt_timecode_to_seconds` then needs to handle a two-part timecode without crashing — replace the fixed three-way destructure with `pop` so hours defaults to zero when absent:

```ruby
def self.vtt_timecode_to_seconds timecode, milliseconds
  parts = timecode.split(':').map(&:to_i)
  s = parts.pop
  m = parts.pop || 0
  h = parts.pop || 0
  h * 3600 + m * 60 + s + "0.#{milliseconds}".to_f
end
```

Add a regression test with a `MM:SS.mmm` VTT string asserting the correct cue count and timecode values.

---

## Issues to Discuss (Non-blocking)

### 🟡 Sequence number gaps when `parse_offset_txt` skips empty blocks

**File:** `app/lib/video/captions.rb`, `parse_offset_txt`

```ruby
blocks.each_with_index do |block, i|
  next if block[:text].empty?
  line.sequence = i + 1   # uses raw array index, not a counter
  ...
end
```

If any block has no text (e.g. two consecutive timestamps with nothing between them), that block is skipped but the index advances. The resulting sequence numbers have gaps (1, 3, 4 instead of 1, 2, 3). The `srt` gem and downstream consumers (`as_json`, `to_html`) don't validate sequence continuity, so this won't crash — but it's worth a quick fix:

```ruby
srt_file.lines.size + 1  # or a local counter
```

### 🟡 `end_time` for a non-empty block followed by an empty block is incorrect

**File:** `app/lib/video/captions.rb`, `parse_offset_txt`

```ruby
end_time = blocks[i + 1] ? blocks[i + 1][:start_time] : block[:start_time] + 30
```

If `blocks[i + 1]` exists but has empty text (and will be skipped), the current cue's `end_time` is set to the empty block's `start_time` rather than the next *visible* cue's `start_time`. In practice this produces a shorter-than-expected cue followed by a gap. Low impact given the format is already a best-effort heuristic, but worth noting.

### 🟡 VTT cue identifiers are treated as cue text if they appear mid-file

**File:** `app/lib/video/captions.rb`, `parse_vtt`

The WebVTT spec allows optional cue identifiers on the line immediately before a timestamp:

```
WEBVTT

intro
00:00:01.000 --> 00:00:03.000
Hello.
```

When `current_line` is `nil` (before the first cue), the identifier line falls through silently — correct. But if a cue identifier appears between two cues where `current_line` has already been flushed and reset to `nil`, it also falls through silently — also correct. The only problematic case would be a cue identifier that appears *before the blank line* that separates two cues (i.e. immediately after cue text), which is not valid VTT. Real-world files are safe. Flagging for awareness only.

---

## Test Coverage Assessment

| Scenario | Covered |
|---|---|
| VTT `HH:MM:SS.mmm` timestamps | ✅ |
| VTT `MM:SS.mmm` timestamps | ❌ not tested |
| VTT trailing cue (no final blank line) | ❌ not tested |
| Offset-txt happy path | ✅ |
| Offset-txt 30s fallback on last cue | ✅ |
| ASCII-8BIT encoding | ✅ |
| Standard SRT still works | ✅ (existing `#to_html` / `#as_json` tests) |
| Empty VTT file | ❌ not tested |
| VTT with cue identifiers | ❌ not tested |

The missing cases are edge cases rather than critical paths. The existing coverage is sufficient to merge, but the trailing-cue and cue-identifier cases would be worth adding before this code handles production volume.

---

## Checklist

- [x] Encoding normalisation handles binary-encoded uploads without raising
- [x] VTT detection is correct (`lstrip` handles BOM/leading whitespace)
- [x] `parse_vtt` handles trailing cue without final blank line
- [x] `parse_offset_txt` derives end times from next block
- [x] All new parsers are `private_class_method`
- [ ] `MM:SS.mmm` timestamp format handled
- [ ] Sequence number gaps in `parse_offset_txt` when empty blocks are skipped
- [ ] `end_time` correctness when next block is empty in `parse_offset_txt`
- [ ] Test: VTT trailing cue without final blank line
- [ ] Test: empty VTT file

---

## Verdict

**Request changes** on the `MM:SS.mmm` issue — it's a silent data-loss bug that will affect the majority of real-world VTT uploads. The two non-blocking issues are low-impact given the offset-txt format is already a best-effort parse, and can be followed up in a separate ticket if the format sees significant usage.