fix(subtitles): interpret short timestamp fields as milliseconds

[Vaishnavi220506]

Summary

Imported SRT or WebVTT cues with a one- or two-digit millisecond field could be shifted by hundreds of milliseconds. Parse the accepted field as a millisecond count.

Problem

00:00:01,5 --> 00:00:02,50 currently imports as 1.500–2.500 s. The parser accepts 1–3 digits and its timestamp helper documents 1.005–2.050 s for these fields. The same error occurs with . separators.

Cause

_ts_to_seconds pads the field on the right before converting it to milliseconds, multiplying one-digit values by 100 and two-digit values by 10.

Changes

• Convert the captured field directly to a millisecond count.
• Cover comma and dot separators in a parser regression test. Three-digit timestamps retain their existing behavior.
• Record the fix in the Unreleased changelog.

Testing

• Before fix on base eef0e230: new regression test failed for both separators (2 failed).
• On 93462fa3: python -m pytest tests/test_srt_parser.py -q -p no:cacheprovider -k 'not paste_endpoint' — 57 passed, 1 deselected.
• python -m pytest tests/test_changelog_style.py -q -p no:cacheprovider — 13 passed.
• python -m compileall -q backend/services/srt_parser.py tests/test_srt_parser.py — passed.
• The endpoint test and adjacent dub integration tests could not run locally: the installed Python 3.14 torchaudio DLL fails to load (WinError 127). CI runs Python 3.11.

The SRT parser now treats one- and two-digit timestamp fields as literal millisecond counts, with regression tests for comma and dot separators. This prevents short fields such as ,5 and .50 from being interpreted as larger durations. No specific merge risk is established by the supplied evidence.

[greptile-apps]

[Medium risk] Changes how subtitle timestamps interpret short millisecond fields.

The PR appears safe to merge; no actionable issue remains.

Summary

The PR interprets short SRT and WebVTT timestamp fields as millisecond counts, adds regression coverage for comma and dot separators, and records the fix in the changelog.

_{Reviews (2) · Last reviewed commit: "Merge main into subtitle timestamp fix"}

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: debpalash/VoiceStudio/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 895f9f13-0892-4907-ab81-2a9a44f91db0

📥 Commits

Reviewing files that changed from the base of the PR and between 93462fa and 573039a.

📒 Files selected for processing (3)

• CHANGELOG.md
• backend/services/srt_parser.py
• tests/test_srt_parser.py

🚧 Files skipped from review as they are similar to previous changes (1)

• CHANGELOG.md

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

---

📝 Walkthrough

Walkthrough

The SRT parser now converts short timestamp millisecond fields directly to milliseconds. Tests cover one- and two-digit fields with comma and dot separators. The changelog records the fix.

Changes

SRT timestamp parsing

Layer / File(s)	Summary
Millisecond field parsing backend/services/srt_parser.py, tests/test_srt_parser.py, CHANGELOG.md	The parser converts the final timestamp field directly to an integer. Tests cover one- and two-digit fields with comma and dot separators. The changelog records the timing fix.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: debpalash

Merge Risk: ⚪ Minimal · up to 57303

Short timestamp fields now retain their intended timing, with regression coverage for both separators. No concrete merge-blocking behavior is evident.

Security Architecture Review

Security architecture risk: ⚪ Minimal · up to 57303

The fix changes timing values for already accepted short timestamp fields without expanding accepted input or bypassing existing validation. No material security risk was found to be introduced or worsened by this change.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The demonstrated changed behavior is limited to timing and timing-dependent processing of already accepted subtitle content. Paste returns parsed segments, while upload can replace segments in the job selected by the existing lookup. No new job-selection authority or cross-service reachability was evidenced.

Trust Boundaries and Controls

• observed — Attacker-controlled subtitle text continues through the same grammar and cue validation. The paste route retains its input-size bound; upload retains job lookup, missing-job rejection, and media-duration clamping. These controls do not establish global authentication or tenant isolation, which were outside the inspected scope.

Resilience and Maintainability Implications

• inferred — The upload workflow retains its existing snapshot persistence and recovery behavior. Concurrent read-modify-write operations are not one atomic transition, and suppressed database-write failures can leave cache state ahead of durable state. These limitations predate this PR; the timestamp arithmetic adds no save operation or failure-state transition that materially worsens them.

🚥 Pre-merge checks | ✅ 8 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 2 files. (1 skipped: 1…	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (8 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title uses Conventional Commit format with the required scope, and issue #2422 is referenced in the pull request body.
Description check	✅ Passed	The description explains the problem, cause, changes, and test results. It omits the template's Type and Checklist sections, but the missing sections do not prevent the description from being mostly c…
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Cross-Platform Default Parity	✅ Passed	The PR changes default subtitle parsing in a shared pure-Python helper. The helper uses only regex captures, integer conversion, and arithmetic; it has no platform-specific branches or APIs. Both defa…
I18n Completeness (21 Locales)	✅ Passed	The pull request changes only CHANGELOG.md, backend/services/srt_parser.py, and tests/test_srt_parser.py. It introduces no frontend code, translation keys, or frontend user-facing strings.
Local-First Guarantee	✅ Passed	The pull request changes only local timestamp arithmetic, tests, and the changelog. The authoritative diff adds no cloud calls, accounts, API keys, telemetry, or network dependencies; _ts_to_seconds…
Backward Compatibility	✅ Passed	PASS — The pull request changes only subtitle timestamp parsing, its tests, and the changelog. It adds no database schema, migration, voice/project/settings, engine, or model files, and it does not al…

Full details: Docstring Coverage

Explanation

Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 2 files. (1 skipped: 1 unsupported.)

• Fix all pre-merge checks with AI

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}