feat(playback): enable instant external text subtitle switching - #532
Closed
RXWatcher wants to merge 7 commits into
Closed
feat(playback): enable instant external text subtitle switching#532RXWatcher wants to merge 7 commits into
RXWatcher wants to merge 7 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds negotiated external SRT/WebVTT subtitle sidecars to Playback V3. The change adds a protocol field and feature flag, validates and persists sidecars during start and replan, serves raw subtitle files, and adds tests and design documentation. ChangesExternal subtitle sidecar feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant PlaybackHandlerV3
participant SubtitleStorage
participant StreamHandler
Client->>PlaybackHandlerV3: Start or replan with feature flags
PlaybackHandlerV3->>SubtitleStorage: Load external SRT/VTT file
SubtitleStorage-->>PlaybackHandlerV3: Subtitle content or error
PlaybackHandlerV3-->>Client: Playback plan with sidecars
Client->>StreamHandler: Request subtitle by sidecar URL
StreamHandler->>SubtitleStorage: Load raw subtitle file
SubtitleStorage-->>StreamHandler: Raw subtitle content
StreamHandler-->>Client: Subtitle content with MIME type
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/plans/2026-08-02-instant-external-srt-switching-server.md`:
- Around line 29-32: Update the V3 sidecar contract and its round-trip test to
explicitly use and assert the JSON keys track_id, index, url, mime_type, format,
and timing_origin_seconds for SubtitleSidecarV3, matching the existing
snake_case conventions in protocol_v3.go. Ensure SubtitleDecisionV3 serializes
sidecars with these exact field names so the Android decoder receives the
metadata.
- Around line 195-205: Update attachExternalTextSidecarsV3 to derive each
external track’s index from the canonical merged subtitle inventory used by
buildSubtitleURLs, preserving that index through filtering instead of using the
local file.ExternalSubtitles position. Use the canonical index for TrackIDV3 and
the subtitle URL, sort sidecars by it, reject duplicate indexes before
serialization, and add coverage for an external track whose merged index is
nonzero.
In `@docs/superpowers/specs/2026-08-02-instant-external-srt-switching-design.md`:
- Around line 91-94: Update the V3-to-session adapter specification to
explicitly state that a singular artifact takes precedence over any sidecar with
the same combined index, regardless of merge order. Add an Android test covering
this collision and asserting that the artifact is retained while the duplicate
sidecar is discarded.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c86e4a51-880a-4af7-8b03-631cb2dc39e3
📒 Files selected for processing (9)
docs/superpowers/plans/2026-08-02-instant-external-srt-switching-server.mddocs/superpowers/specs/2026-08-02-instant-external-srt-switching-design.mdinternal/api/handlers/playback_v3.gointernal/api/handlers/playback_v3_sidecars_test.gointernal/api/handlers/playback_v3_test.gointernal/api/handlers/stream.gointernal/api/handlers/stream_test.gointernal/playback/protocol_v3.gointernal/playback/protocol_v3_test.go
10 tasks
Contributor
|
Closing this implementation rather than rejecting the product goal. The draft parallel sidecar shape was superseded by the platform-neutral protocol-v3 inventory in #567, and this branch now conflicts with the current playback implementation. A defect-first review also found synchronous full-file reads of every eligible SRT/VTT during start and replan, which could block playback or consume unbounded memory on large, numerous, or slow files. The current-contract follow-up, including bounded loading and behavioral acceptance criteria, is tracked in #689. The web app did not negotiate this feature and is not materially affected. Thanks for exploring the original direction. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Part of the coordinated instant external subtitle switching work with silo-android.
Switching between external SRT subtitles currently requires a playback replan and visible rebuffer even though text subtitles can be mounted by the client immediately.
Approach
external_text_sidecar_set_v1playback feature and additive V3 sidecar contract.Companion Android PR: Silo-Server/silo-android#159
Testing
Passing:
go test -count=1 ./internal/api/handlers— PASSgo test ./internal/playback -run 'Test(ServerFeaturesV3ReturnsCompleteIndependentSlices|SubtitleDecisionV3SidecarsRoundTrip|ProtocolV3GoldenWireFixtures)$'— PASSmake verify-local-paths— PASScd web && pnpm run format:check— PASSgit diff --check— PASSKnown baseline limitations, reproduced independently of this diff:
make test-goreaches the repository-wide suite but four macOS NVENC fake-probe timing tests fail identically on an untouchedupstream/mainsnapshot.make lintreports the existing repository lint backlog (308 findings, including stale paths from another checkout).make test-webfails current Node localStorage-dependent tests; this PR changes no web code.AI Disclosure
Checklist
Summary by CodeRabbit
New Features
Bug Fixes