Add preferred subtitle language#46
Merged
Merged
Conversation
📝 WalkthroughWalkthroughAdds a preferred subtitle language model with detection and prioritization, persists it across repositories, exposes it through web and TV/desktop settings, and applies it automatically in Exo and embedded mpv playback unless the user manually selects a subtitle. ChangesPreferred subtitle language
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SettingsUI
participant WebControlService
participant PlaybackPreferencesRepository
participant ExoPlaybackController
participant SubtitleLanguagePreference
SettingsUI->>WebControlService: save preferred subtitle language
WebControlService->>PlaybackPreferencesRepository: persist preference
ExoPlaybackController->>PlaybackPreferencesRepository: load preference on play
ExoPlaybackController->>SubtitleLanguagePreference: resolve preferred track
SubtitleLanguagePreference-->>ExoPlaybackController: return matching track index
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 2
🧹 Nitpick comments (3)
ui-tv/src/test/kotlin/com/miruplay/tv/ui/settings/SettingsViewModelTokenTest.kt (1)
91-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test coverage for
setPreferredSubtitleLanguage.The mock stub is added, but no test exercises
SettingsViewModel.setPreferredSubtitleLanguage(...). Consider mirroring the existingsetCurrentAppMode persists next launch mode and updates state flowtest.✅ Suggested test
`@Test` fun `setPreferredSubtitleLanguage persists preference and updates state flow`() = runTest { every { playbackPreferences.preferredSubtitleLanguage = any() } just Runs val viewModel = createViewModel() advanceUntilIdle() viewModel.setPreferredSubtitleLanguage(SubtitleLanguagePreference.CHINESE_SIMPLIFIED) verify(exactly = 1) { playbackPreferences.preferredSubtitleLanguage = SubtitleLanguagePreference.CHINESE_SIMPLIFIED } assertEquals(SubtitleLanguagePreference.CHINESE_SIMPLIFIED, viewModel.preferredSubtitleLanguage.value) }As per path instructions,
**/src/test/**/*.ktshould "Write test methods with backtick-quoted, descriptive names" and "use JUnit 4, MockK ... for Flow testing".🤖 Prompt for 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. In `@ui-tv/src/test/kotlin/com/miruplay/tv/ui/settings/SettingsViewModelTokenTest.kt` around lines 91 - 93, Add a JUnit 4 test in SettingsViewModelTokenTest for SettingsViewModel.setPreferredSubtitleLanguage, using a backtick-quoted descriptive name. Stub playbackPreferences.preferredSubtitleLanguage assignment, create the view model, invoke the method with CHINESE_SIMPLIFIED, verify the preference is persisted exactly once, and assert the preferredSubtitleLanguage state flow updates accordingly using the existing MockK and runTest patterns.Source: Path instructions
desktop-app/src/main/kotlin/com/miruplay/tv/desktop/MiruPlayDesktopComposeApp.kt (1)
1049-1084: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPrefer the local
preferredSubtitleLanguagestate over re-reading the repository on every launch.
launchSubtitlePreferenceis fetched viarepositories.playbackPreferences.getPreferredSubtitleLanguage()on everyplaybackLaunchRequestForcall, even though the already-loaded Compose statepreferredSubtitleLanguageholds the same value and is kept in sync at startup and on every change. This adds an unnecessary suspend/I-O read on the playback hot path (unused entirely whenhasSubtitleOverrideis true), and diverges from the siblingplaybackEndAction, which is read directly from local state at its use site (line 1287) rather than re-fetched from the repository. SinceonPreferredSubtitleLanguageChangepersists asynchronously insidescope.launchbefore updating the local var, reading straight from the repository also opens a narrow staleness window between a just-changed setting and its persisted value.♻️ Suggested fix
- val launchSubtitlePreference = repositories.playbackPreferences.getPreferredSubtitleLanguage() + val launchSubtitlePreference = preferredSubtitleLanguage🤖 Prompt for 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. In `@desktop-app/src/main/kotlin/com/miruplay/tv/desktop/MiruPlayDesktopComposeApp.kt` around lines 1049 - 1084, Update playbackLaunchRequestFor to use the existing preferredSubtitleLanguage Compose state instead of calling repositories.playbackPreferences.getPreferredSubtitleLanguage(). Keep the local value in the existing subtitle prioritization flow, preserving manual override behavior and avoiding the suspend/I/O repository read on each launch.web-control-core/src/main/kotlin/com/miruplay/tv/webcontrol/WebControlModels.kt (1)
337-342: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded subtitle-language option list duplicates the
SubtitleLanguagePreferenceenum in two places.The allowed values (
auto,zh_hans,zh_hant,zh,en,ja) already exist asSubtitleLanguagePreference.entries(withstorageValue), but are re-declared as literal string lists here and on the frontend. Since no call site overrides the DTO default, this literal is what production responses actually return — a future enum addition (e.g. Korean) would silently be missing from the API/UI unless both copies are remembered and updated together.
web-control-core/src/main/kotlin/com/miruplay/tv/webcontrol/WebControlModels.kt#L337-L342: derive the default fromSubtitleLanguagePreference.entries.map { it.storageValue }instead of a hardcoded list.web-control/frontend/src/App.vue#L1638-L1649: update thepreferredSubtitleLanguageOptionsfallback array (or drop it in favor of always trusting the server payload) to avoid a second copy of the same literal.♻️ Proposed fix
+import com.miruplay.tv.model.SubtitleLanguagePreference + `@Serializable` data class PlaybackSettingsDto( val endAction: String, val preferredSubtitleLanguage: String, val formatAwareToneMapping: FormatAwareToneMappingPreferences, val endActionOptions: List<String> = listOf("return_to_detail", "play_next_episode"), val preferredSubtitleLanguageOptions: List<String> = - listOf("auto", "zh_hans", "zh_hant", "zh", "en", "ja"), + SubtitleLanguagePreference.entries.map { it.storageValue }, )Note: this mirrors the existing
endActionOptionshardcoding pattern already present in the codebase, so it's a pre-existing style choice rather than a new regression — flagging for awareness rather than as a blocker.🤖 Prompt for 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. In `@web-control-core/src/main/kotlin/com/miruplay/tv/webcontrol/WebControlModels.kt` around lines 337 - 342, The subtitle-language options are duplicated as hardcoded literals instead of deriving from SubtitleLanguagePreference. In web-control-core/src/main/kotlin/com/miruplay/tv/webcontrol/WebControlModels.kt lines 337-342, derive preferredSubtitleLanguageOptions from SubtitleLanguagePreference.entries using each entry’s storageValue; in web-control/frontend/src/App.vue lines 1638-1649, update or remove the fallback array so it does not maintain a second literal copy and continues relying on the server-provided options.
🤖 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 `@core/model/src/main/kotlin/com/miruplay/tv/model/SubtitleTracks.kt`:
- Around line 78-84: Update subtitleFileName() so URL decoding is performed only
for URL-like paths, preserving local filenames exactly; retain URL suffix
stripping for remote inputs. Tighten SUBTITLE_LANGUAGE_SUFFIX validation so
ordinary filename endings such as “Part One.srt” and “The End.srt” are rejected
and resolve to the existing und fallback.
- Around line 57-66: Update subtitleLanguageFromPath and
SUBTITLE_LANGUAGE_SUFFIX handling so trailing 2–3 character suffixes are
accepted only when they are recognized language tags. Preserve the existing
Chinese mappings and return "und" for unrecognized suffixes such as "one" or
"end", preventing them from being used as subtitle labels.
---
Nitpick comments:
In
`@desktop-app/src/main/kotlin/com/miruplay/tv/desktop/MiruPlayDesktopComposeApp.kt`:
- Around line 1049-1084: Update playbackLaunchRequestFor to use the existing
preferredSubtitleLanguage Compose state instead of calling
repositories.playbackPreferences.getPreferredSubtitleLanguage(). Keep the local
value in the existing subtitle prioritization flow, preserving manual override
behavior and avoiding the suspend/I/O repository read on each launch.
In
`@ui-tv/src/test/kotlin/com/miruplay/tv/ui/settings/SettingsViewModelTokenTest.kt`:
- Around line 91-93: Add a JUnit 4 test in SettingsViewModelTokenTest for
SettingsViewModel.setPreferredSubtitleLanguage, using a backtick-quoted
descriptive name. Stub playbackPreferences.preferredSubtitleLanguage assignment,
create the view model, invoke the method with CHINESE_SIMPLIFIED, verify the
preference is persisted exactly once, and assert the preferredSubtitleLanguage
state flow updates accordingly using the existing MockK and runTest patterns.
In
`@web-control-core/src/main/kotlin/com/miruplay/tv/webcontrol/WebControlModels.kt`:
- Around line 337-342: The subtitle-language options are duplicated as hardcoded
literals instead of deriving from SubtitleLanguagePreference. In
web-control-core/src/main/kotlin/com/miruplay/tv/webcontrol/WebControlModels.kt
lines 337-342, derive preferredSubtitleLanguageOptions from
SubtitleLanguagePreference.entries using each entry’s storageValue; in
web-control/frontend/src/App.vue lines 1638-1649, update or remove the fallback
array so it does not maintain a second literal copy and continues relying on the
server-provided options.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1147097f-12e8-4f3c-a784-921ea1014f2b
📒 Files selected for processing (25)
core/model/src/main/kotlin/com/miruplay/tv/model/PlaybackUiConventions.ktcore/model/src/main/kotlin/com/miruplay/tv/model/SubtitleLanguagePreference.ktcore/model/src/main/kotlin/com/miruplay/tv/model/SubtitleTracks.ktcore/model/src/test/kotlin/com/miruplay/tv/model/SubtitleTracksTest.ktdata/src/main/kotlin/com/miruplay/tv/data/preferences/PlaybackPreferencesManager.ktdata/src/test/kotlin/com/miruplay/tv/data/preferences/PlaybackPreferencesManagerTest.ktdesktop-app/src/main/kotlin/com/miruplay/tv/desktop/DesktopPlaybackPanels.ktdesktop-app/src/main/kotlin/com/miruplay/tv/desktop/DesktopWebControlService.ktdesktop-app/src/main/kotlin/com/miruplay/tv/desktop/MiruPlayDesktopComposeApp.ktdesktop-app/src/test/kotlin/com/miruplay/tv/desktop/DesktopPlaybackPanelTest.ktdesktop-app/src/test/kotlin/com/miruplay/tv/desktop/DesktopWebControlServerTest.ktplayer-core/src/main/kotlin/com/miruplay/tv/player/ExoPlaybackController.ktrepository-api/src/main/kotlin/com/miruplay/tv/repository/PlaybackPreferencesRepository.ktrepository-api/src/main/kotlin/com/miruplay/tv/repository/SettingsPreferenceActionCoordinator.ktrepository-api/src/test/kotlin/com/miruplay/tv/repository/SettingsPreferenceActionCoordinatorTest.ktrepository-desktop/src/main/kotlin/com/miruplay/tv/repository/desktop/DesktopRepositoryStore.ktrepository-desktop/src/main/kotlin/com/miruplay/tv/repository/desktop/FileBackedPlaybackPreferencesRepository.ktrepository-desktop/src/test/kotlin/com/miruplay/tv/repository/desktop/DesktopRepositoriesTest.ktui-tv/src/main/kotlin/com/miruplay/tv/ui/settings/AddSourceScreen.ktui-tv/src/main/kotlin/com/miruplay/tv/ui/settings/SettingsViewModel.ktui-tv/src/test/kotlin/com/miruplay/tv/ui/settings/SettingsViewModelTokenTest.ktweb-control-core/src/main/kotlin/com/miruplay/tv/webcontrol/WebControlModels.ktweb-control-core/src/test/kotlin/com/miruplay/tv/webcontrol/WebControlSettingsRouteTest.ktweb-control/frontend/src/App.vueweb-control/src/main/kotlin/com/miruplay/tv/webcontrol/WebControlService.kt
Comment on lines
57
to
+66
| private fun subtitleLanguageFromPath(path: String): String { | ||
| val stem = MediaPathConventions.stem(MediaPathConventions.fileName(path)) | ||
| val candidate = listOf('.', '_') | ||
| .map { separator -> stem.substringAfterLast(separator, "") } | ||
| .firstOrNull(::isSubtitleLanguageCode) | ||
| val stem = MediaPathConventions.stem(subtitleFileName(path)).replace('_', '-') | ||
| val candidate = SUBTITLE_LANGUAGE_SUFFIX.find(stem) | ||
| ?.groupValues | ||
| ?.get(1) | ||
| ?.lowercase() | ||
| ?: return "und" | ||
| return when (candidate) { | ||
| "chs" -> "zh-Hans" | ||
| "cht" -> "zh-Hant" | ||
| "chs", "sc" -> "zh-Hans" | ||
| "cht", "tc" -> "zh-Hant" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- SubtitleTracks.kt outline ---'
ast-grep outline core/model/src/main/kotlin/com/miruplay/tv/model/SubtitleTracks.kt --view expanded || true
echo '--- Relevant file excerpt ---'
sed -n '1,220p' core/model/src/main/kotlin/com/miruplay/tv/model/SubtitleTracks.kt
echo '--- Search usages of SubtitleTrack.language ---'
rg -n "SubtitleTrack\.language|language\s*[:=]" core -g '*.kt' || trueRepository: ModerRAS/MiruPlay
Length of output: 5969
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('core/model/src/main/kotlin/com/miruplay/tv/model/SubtitleTracks.kt')
print(p.exists(), p.stat().st_size)
print(p.read_text()[:5000])
PYRepository: ModerRAS/MiruPlay
Length of output: 3659
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact function and related constants.
nl -ba core/model/src/main/kotlin/com/miruplay/tv/model/SubtitleTracks.kt | sed -n '1,180p'
echo '--- occurrences of subtitleLanguageFromPath and SubtitleTrack.language ---'
rg -n "subtitleLanguageFromPath|SubtitleTrack\\(|language" core/model/src/main/kotlin/com/miruplay/tv/model -g '*.kt'Repository: ModerRAS/MiruPlay
Length of output: 195
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,140p' core/model/src/main/kotlin/com/miruplay/tv/model/PlaybackUiConventions.kt
echo '---'
sed -n '1,220p' core/model/src/test/kotlin/com/miruplay/tv/model/PlaybackUiConventionsTest.ktRepository: ModerRAS/MiruPlay
Length of output: 12698
Restrict trailing filename suffixes to real language tags
Trailing 2–3 letter suffixes are treated as language codes without validation, so Part One.srt / The End.srt can produce language = "one" / "end" and surface in subtitle picker labels when the track title is blank.
🤖 Prompt for 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.
In `@core/model/src/main/kotlin/com/miruplay/tv/model/SubtitleTracks.kt` around
lines 57 - 66, Update subtitleLanguageFromPath and SUBTITLE_LANGUAGE_SUFFIX
handling so trailing 2–3 character suffixes are accepted only when they are
recognized language tags. Preserve the existing Chinese mappings and return
"und" for unrecognized suffixes such as "one" or "end", preventing them from
being used as subtitle labels.
Comment on lines
+78
to
84
| private fun subtitleFileName(path: String): String { | ||
| val pathWithoutUrlSuffix = if ("://" in path) path.substringBefore('?').substringBefore('#') else path | ||
| val encodedName = pathWithoutUrlSuffix.substringAfterLast('/').substringAfterLast('\\') | ||
| return runCatching { | ||
| URLDecoder.decode(encodedName.replace("+", "%2B"), StandardCharsets.UTF_8) | ||
| }.getOrDefault(encodedName) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and nearby logic.
wc -l core/model/src/main/kotlin/com/miruplay/tv/model/SubtitleTracks.kt
cat -n core/model/src/main/kotlin/com/miruplay/tv/model/SubtitleTracks.kt | sed -n '1,220p'
# Find related subtitle language detection code and usages.
rg -n "subtitleLanguageFromPath|detectSubtitleLanguage|detectedSubtitleLanguage|SubtitleTrack.language|SUBTITLE_LANGUAGE_SUFFIX|subtitleFileName" core -SRepository: ModerRAS/MiruPlay
Length of output: 6842
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the subtitle language preference logic.
cat -n core/model/src/main/kotlin/com/miruplay/tv/model/SubtitleLanguagePreference.kt | sed -n '1,220p'
# Inspect any tests covering subtitle filename/language parsing.
rg -n "subtitleFileName|subtitleLanguageFromPath|SUBTITLE_LANGUAGE_SUFFIX|buildExternalSubtitleTracks|externalSubtitleTrackFromPath|SubtitleTrack\\(" core/model/src/test -SRepository: ModerRAS/MiruPlay
Length of output: 8676
🏁 Script executed:
#!/bin/bash
set -euo pipefail
wc -l core/model/src/test/kotlin/com/miruplay/tv/model/SubtitleTracksTest.kt
cat -n core/model/src/test/kotlin/com/miruplay/tv/model/SubtitleTracksTest.kt | sed -n '1,240p'Repository: ModerRAS/MiruPlay
Length of output: 7447
Scope remote decoding and tighten suffix parsing.
subtitleFileName()decodes local paths too, so valid%XXsequences in filenames can be rewritten unexpectedly. Gate decoding to URL-like inputs.SUBTITLE_LANGUAGE_SUFFIXis too permissive; ordinary endings likePart One.srtorThe End.srtcan be treated as language codes instead of falling back tound.
🤖 Prompt for 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.
In `@core/model/src/main/kotlin/com/miruplay/tv/model/SubtitleTracks.kt` around
lines 78 - 84, Update subtitleFileName() so URL decoding is performed only for
URL-like paths, preserving local filenames exactly; retain URL suffix stripping
for remote inputs. Tighten SUBTITLE_LANGUAGE_SUFFIX validation so ordinary
filename endings such as “Part One.srt” and “The End.srt” are rejected and
resolve to the existing und fallback.
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
1 participant
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.
Summary
Validation
npm run buildinweb-control/frontend./gradlew :core:model:test :repository-api:test :repository-desktop:test :data:testDebugUnitTest :player-core:testDebugUnitTest :ui-tv:testDebugUnitTest :web-control-core:test :web-control:testDebugUnitTest :desktop-app:test :app:assembleDebugadb install -rand startup health checkzh-CN.srttracksVersion
baseAppVersionNameat1.1.0; this completes the subtitle feature line already released asv1.1.xSummary by CodeRabbit
New Features
Bug Fixes