fix(build): skip-unsupported-streams no longer drops text subtitles#98
Conversation
) `strip_unsupported_subtitle_mappings` was retaining only `SubtitleType::Bitmap` streams, which silently stripped every text-based subtitle track (SRT/ASS/WebVTT) even though Spindle has a fully-working `RenderTextSubtitles` pipeline for them. The filter now retains everything except `SubtitleType::Unknown` — codecs ffprobe identified but Spindle has no handler for. Bitmap passthrough and text rendering are both kept. Rename helper to `strip_unknown_codec_subtitle_mappings` to match the narrower scope. Replace the now-inverted planner test with two regression tests: - text subtitle tracks survive the escape hatch - unknown-codec tracks are still stripped Closes #93. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L3tSFK3P9vGwbNNtb4AYQG
|
@codex review |
Spindle Rust tests222 tests +1 222 ✅ +1 2s ⏱️ ±0s Results for commit e19f8b4. ± Comparison against base commit 759d4d5. This pull request removes 1 and adds 2 tests. Note that renamed tests count towards both. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e19f8b4a3e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| asset.subtitle_streams.iter().any(|s| { | ||
| s.index == sm.source_stream_index && s.subtitle_type == SubtitleType::Bitmap | ||
| s.index == sm.source_stream_index | ||
| && s.subtitle_type != SubtitleType::Unknown |
There was a problem hiding this comment.
Now that this predicate intentionally keeps SubtitleType::Text, the Settings page still says the toggle will “Automatically strip text-based subtitle mappings” and is useful to “author without them” (apps/spindle/src/pages/SettingsPage.tsx:234-235). When a user with only text subtitles enables that setting based on the UI, the planner now retains those mappings and emits RenderTextSubtitles jobs instead, so builds can still fail in the text-subtitle path or produce subtitles contrary to the documented behaviour.
Useful? React with 👍 / 👎.
## Summary Follow-up to #98 / #93, caught in Codex review. The "Skip unsupported streams" toggle label and description said it would strip text-based subtitle mappings. Since #98 changed the behaviour so only unknown-codec streams are stripped, the copy was now actively misleading — a user enabling the toggle based on the description would expect text subtitles to be dropped, but they'd still be rendered. - Label: "Skip unsupported streams" → "Skip unknown-codec streams" - Description: rewritten to accurately describe what the toggle does - Settings test selector updated to match ## Test plan - [x] `pnpm test:js` (233/233 passing) - [x] `pnpm format:check`
1 participant
Summary
strip_unsupported_subtitle_mappingswas retaining onlySubtitleType::Bitmapstreams, silently stripping every SRT/ASS/WebVTT subtitle track even though Spindle has a fully-workingRenderTextSubtitlespipeline for them.The filter now retains everything except
SubtitleType::Unknown— codecs ffprobe identified but Spindle has no handler for. Bitmap passthrough and text rendering are both kept.strip_unknown_codec_subtitle_mappingsto reflect the narrower scopeCloses #93.
Test plan
cargo test -p tauri-plugin-spindle-project(213/213 passing)cargo clippy --all-targets -- -D warningscargo fmt --check