feat(player): discover external subtitles in media directory - #66
Conversation
📝 WalkthroughWalkthroughThe change updates the application version to 2.7.0 and adds best-effort subtitle discovery from indexed and sibling playback paths, including regular and SAF document paths. ChangesPlayback subtitle discovery
Application version update
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PlayerViewModel
participant PlaybackSubtitleResolver
participant MediaSourceFactory
participant PlaybackDirectorySubtitleFiles
PlayerViewModel->>PlaybackSubtitleResolver: resolve subtitles
PlaybackSubtitleResolver->>PlaybackDirectorySubtitleFiles: list sibling playback paths
PlaybackDirectorySubtitleFiles->>MediaSourceFactory: create and list media source
MediaSourceFactory-->>PlaybackDirectorySubtitleFiles: directory entries
PlaybackDirectorySubtitleFiles-->>PlaybackSubtitleResolver: sibling file paths
PlaybackSubtitleResolver-->>PlayerViewModel: merged subtitle tracks
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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: 1
🤖 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
`@ui-tv/src/main/kotlin/com/miruplay/tv/ui/player/PlaybackDirectorySubtitleFiles.kt`:
- Around line 24-31: Update both exception handlers in
PlaybackDirectorySubtitleFiles so they rethrow CancellationException before
converting other failures to an empty list or ignoring close errors. Preserve
the existing best-effort handling for non-cancellation exceptions in the
listFiles and mediaSource.close suspend calls.
🪄 Autofix
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 Plus
Run ID: b96b07b6-097d-445d-a6a4-3d2bb7b31666
📒 Files selected for processing (6)
app/build.gradle.ktsrepository-api/src/main/kotlin/com/miruplay/tv/repository/PlaybackSubtitleResolver.ktrepository-api/src/test/kotlin/com/miruplay/tv/repository/PlaybackSubtitleResolverTest.ktui-tv/src/main/kotlin/com/miruplay/tv/ui/player/PlaybackDirectorySubtitleFiles.ktui-tv/src/main/kotlin/com/miruplay/tv/ui/player/PlayerViewModel.ktui-tv/src/test/kotlin/com/miruplay/tv/ui/player/PlaybackDirectorySubtitleFilesTest.kt
| } catch (_: Exception) { | ||
| emptyList() | ||
| } finally { | ||
| try { | ||
| mediaSource.close() | ||
| } catch (_: Exception) { | ||
| // Directory subtitle discovery is best-effort and must not block playback. | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
In Kotlin coroutines, does catch (Exception)catchCancellationException, and should suspend functions rethrow it?
💡 Result:
Yes, a catch(Exception) or catch(Throwable) block in Kotlin will catch a CancellationException because it is a subclass of Exception [1][2][3]. However, you should always rethrow CancellationException if you catch it [1][2][4]. The reason is that CancellationException is used by the coroutines library to signal that a coroutine has been cancelled [1][2]. If you catch it and do not rethrow it, you suppress this signal, which can prevent the coroutine from terminating correctly, break structured concurrency, and lead to unexpected behavior where a coroutine continues to run after it should have been cancelled [4][5]. If you need to perform cleanup or handle exceptions within a coroutine, the recommended pattern is: 1. Catch the exception. 2. Check if it is a CancellationException. 3. If it is, rethrow it immediately [1][2][4]. Example: try { // some suspending code } catch (e: Exception) { if (e is CancellationException) { throw e // Always rethrow CancellationException } // handle other exceptions } Alternatively, if you are catching a generic exception and want to ensure the coroutine terminates if it has been cancelled, you can call ensureActive within your catch block [6][4]. This checks the cancellation status of the coroutine and throws a CancellationException if the coroutine is cancelled [1][4].
Citations:
- 1: https://kotlinlang.org/docs/coroutines-cancellation.html
- 2: https://kotlinlang.org/docs/exception-handling.html
- 3: https://kotlinlang.org/api/core/kotlin-stdlib/kotlin.coroutines.cancellation/-cancellation-exception/
- 4: https://stackoverflow.com/questions/76259793/is-it-necceary-to-rethrow-the-cancellationexception-in-kotlin
- 5: Provide a
runCatchingthat does not handle aCancellationExceptionbut re-throws it instead. Kotlin/kotlinx.coroutines#1814 - 6: https://kotlinlang.org/api/kotlinx.coroutines/kotlinx-coroutines-core/kotlinx.coroutines/-coroutine-exception-handler/
🏁 Script executed:
#!/bin/bash
set -eu
file="ui-tv/src/main/kotlin/com/miruplay/tv/ui/player/PlaybackDirectorySubtitleFiles.kt"
printf '%s\n' '--- file outline ---'
ast-grep outline "$file" --lang kotlin || true
printf '%s\n' '--- relevant source ---'
cat -n "$file"
printf '%s\n' '--- references ---'
rg -n -C 4 'listPlaybackSiblingPaths|PlaybackDirectorySubtitleFiles' .Repository: ModerRAS/MiruPlay
Length of output: 6779
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- MediaSource declarations and implementations ---'
rg -n -C 5 'interface MediaSource|abstract class .*MediaSource|fun listFiles|fun close' ui-tv/src/main/kotlin
printf '%s\n' '--- resolver and ViewModel coroutine context ---'
cat -n ui-tv/src/main/kotlin/com/miruplay/tv/ui/player/PlaybackSubtitleResolver.kt 2>/dev/null || true
sed -n '160,215p' ui-tv/src/main/kotlin/com/miruplay/tv/ui/player/PlayerViewModel.kt
printf '%s\n' '--- relevant tests ---'
cat -n ui-tv/src/test/kotlin/com/miruplay/tv/ui/player/PlaybackDirectorySubtitleFilesTest.ktRepository: ModerRAS/MiruPlay
Length of output: 9815
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate media-source files ---'
git ls-files | rg '(^|/)(MediaSource|.*MediaSource.*|PlaybackSubtitleResolver)\.(kt|java)$'
printf '%s\n' '--- all relevant declarations ---'
rg -n -C 6 'interface MediaSource|class MediaSource|listFiles\(|close\(\)' --glob '*.kt' --glob '*.java' .
printf '%s\n' '--- resolver source ---'
resolver="$(git ls-files | rg '/PlaybackSubtitleResolver\.kt$' | head -n 1)"
if [ -n "$resolver" ]; then
cat -n "$resolver"
fiRepository: ModerRAS/MiruPlay
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- MediaSource API ---'
cat -n media-source-api/src/main/kotlin/com/miruplay/tv/mediasource/MediaSource.kt
printf '%s\n' '--- MediaSource implementations close/list signatures ---'
rg -n -C 3 'override suspend fun (listFiles|close)|override fun (listFiles|close)|suspend fun (listFiles|close)' media-source media-source-api
printf '%s\n' '--- PlaybackSubtitleResolver ---'
cat -n repository-api/src/main/kotlin/com/miruplay/tv/repository/PlaybackSubtitleResolver.kt
printf '%s\n' '--- dependency versions for coroutine semantics ---'
rg -n -C 2 'kotlinx-coroutines|kotlin\(' gradle/libs.versions.toml build.gradle.kts settings.gradle.kts gradle --glob '*.kts' --glob '*.toml'Repository: ModerRAS/MiruPlay
Length of output: 15341
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- MediaSource API ---'
cat -n media-source-api/src/main/kotlin/com/miruplay/tv/mediasource/MediaSource.kt
printf '%s\n' '--- MediaSource implementations close/list signatures ---'
rg -n -C 3 'override suspend fun (listFiles|close)|override fun (listFiles|close)|suspend fun (listFiles|close)' media-source media-source-api
printf '%s\n' '--- PlaybackSubtitleResolver ---'
cat -n repository-api/src/main/kotlin/com/miruplay/tv/repository/PlaybackSubtitleResolver.kt
printf '%s\n' '--- dependency versions for coroutine semantics ---'
rg -n -C 2 'kotlinx-coroutines|kotlin\(' gradle/libs.versions.toml build.gradle.kts settings.gradle.kts gradle --glob '*.kts' --glob '*.toml'Repository: ModerRAS/MiruPlay
Length of output: 15341
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- project CancellationException usage ---'
rg -n -C 3 'CancellationException|ensureActive|catch \(_?: Exception\)' --glob '*.kt' .
printf '%s\n' '--- standalone JVM exception-hierarchy probe ---'
if command -v jshell >/dev/null 2>&1; then
jshell <<'EOF'
import java.util.concurrent.CancellationException;
System.out.println("CancellationException extends Exception: " +
Exception.class.isAssignableFrom(CancellationException.class));
try {
throw new CancellationException("cancelled");
} catch (Exception error) {
System.out.println("catch(Exception) receives it: " +
(error instanceof CancellationException));
}
EOF
else
printf '%s\n' 'jshell unavailable'
fi
printf '%s\n' '--- source assertions ---'
python3 - <<'PY'
from pathlib import Path
path = Path("ui-tv/src/main/kotlin/com/miruplay/tv/ui/player/PlaybackDirectorySubtitleFiles.kt")
text = path.read_text()
assert "internal suspend fun listPlaybackSiblingPaths" in text
assert text.count("catch (_: Exception)") == 2
assert "mediaSource.listFiles(parentPath)" in text
assert "mediaSource.close()" in text
print("listPlaybackSiblingPaths has two broad Exception handlers around listFiles and close")
PYRepository: ModerRAS/MiruPlay
Length of output: 25567
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- project CancellationException usage ---'
rg -n -C 3 'CancellationException|ensureActive|catch \(_?: Exception\)' --glob '*.kt' .
printf '%s\n' '--- standalone JVM exception-hierarchy probe ---'
if command -v jshell >/dev/null 2>&1; then
jshell <<'EOF'
import java.util.concurrent.CancellationException;
System.out.println("CancellationException extends Exception: " +
Exception.class.isAssignableFrom(CancellationException.class));
try {
throw new CancellationException("cancelled");
} catch (Exception error) {
System.out.println("catch(Exception) receives it: " +
(error instanceof CancellationException));
EOF
else
printf '%s\n' 'jshell unavailable'
fi
printf '%s\n' '--- source assertions ---'
python3 - <<'PY'
from pathlib import Path
path = Path("ui-tv/src/main/kotlin/com/miruplay/tv/ui/player/PlaybackDirectorySubtitleFiles.kt")
text = path.read_text()
assert "internal suspend fun listPlaybackSiblingPaths" in text
assert text.count("catch (_: Exception)") == 2
assert "mediaSource.listFiles(parentPath)" in text
assert "mediaSource.close()" in text
print("listPlaybackSiblingPaths has two broad Exception handlers around listFiles and close")
PYRepository: ModerRAS/MiruPlay
Length of output: 25567
Propagate CancellationException from both suspend calls
listFiles and close are suspend functions. Both catch (Exception) handlers can swallow cancellation. Rethrow CancellationException before handling other exceptions so PlaybackSubtitleResolver cannot continue after cancellation.
🤖 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/main/kotlin/com/miruplay/tv/ui/player/PlaybackDirectorySubtitleFiles.kt`
around lines 24 - 31, Update both exception handlers in
PlaybackDirectorySubtitleFiles so they rethrow CancellationException before
converting other failures to an empty list or ignoring close errors. Preserve
the existing best-effort handling for non-cancellation exceptions in the
listFiles and mediaSource.close suspend calls.
1 participant
Summary
.ass,.ssa,.srt, and.vttsidecarsmedia_subtitlepaths, removing duplicatesMLIP
media_subtitlecontractVerification
./gradlew test./gradlew lint:repository-api:test :ui-tv:testDebugUnitTest :app:assembleDebugafter the version updategit diff --checkVersioning
Minor-level user-visible playback feature.
baseAppVersionNameis2.7.0, based on online stablev2.6.671; CI will generate the release patch number.Summary by CodeRabbit
New Features
Bug Fixes
Chores