Skip to content

Add subtitle metrics library (CPS/WPM, overlaps, gaps) with tests - #57

Open
fasuizu-br wants to merge 3 commits into
laubonghaudoi:mainfrom
fasuizu-br:feat/subtitle-metrics
Open

Add subtitle metrics library (CPS/WPM, overlaps, gaps) with tests#57
fasuizu-br wants to merge 3 commits into
laubonghaudoi:mainfrom
fasuizu-br:feat/subtitle-metrics

Conversation

@fasuizu-br

Copy link
Copy Markdown

As discussed in #34 — starting small, as you asked.

This PR adds only the metrics logic and its tests. No UI, no new dependencies, nothing else touched:

  • lib/subtitle-metrics.ts — pure functions: per-cue CPS/WPM, duration, overlap and gap detection, plus a track-level summary. Thresholds are parameters with documented defaults.
  • tests/subtitle-metrics.test.ts — 9 tests covering known-value CPS/WPM, short duration, overlap, gap above/below threshold, and degenerate input (empty list, end ≤ start, empty text, VTT styling/newlines).

Reuses the project's existing time parsing and follows the conventions in lib/ and tests/. Full suite passes locally (npm test).

Wiring this into the UI can come later in a separate, equally small PR — whatever shape you prefer for the minimal surface.

@laubonghaudoi

Copy link
Copy Markdown
Owner

Confirmed defects:

  • timelineSpanSeconds is wrong for nested cues even when sorted. It uses the final cue’s end time rather than the maximum end time at lib/subtitle-metrics.ts:413. A 0–60s cue followed by a 5–7s cue reports 7s, not 60s.
  • Track averages include text from invalid cues but exclude their duration at lib/subtitle-metrics.ts:405. I reproduced averageCps: 55 while the valid cue’s CPS was only 5.
  • CJK counting is problematic: "佢喺嗰度𠮶𡃁仔" reports charCount: 9 and wordCount: 1. The repository explicitly supports Cantonese via the yue locale, so this needs a stated policy before UI integration.
  • "Infinity" timestamps produce NaN metrics without an invalid-time warning at lib/subtitle-metrics.ts:131. A Number.isFinite guard is needed.
  • Biome formatting fails on both new PR files. The repository-wide check also reports a pre-existing messages/pl.json issue.

Also confirmed:

  • The two new warning types (high-cps, high-wpm) are not tested.
  • Tests do not exercise genuinely non-default options.
  • Track warning aggregation and warning-to-cue identity fields are not tested.
  • The PR says it reuses existing time parsing, but it actually duplicates it.
  • The issue specifies maxWpm: 180; the PR uses 200.
  • npm test, lint, and build pass; TypeScript still has two unrelated pre-existing errors.
  • The module has no application call sites yet, so this is low immediate runtime risk but an important API-contract decision.

Can you fix these issues?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add subtitle statistics and reading-speed warnings

2 participants