Scan clever ferret for issues #205
Conversation
Adds support for streaming remote EPUB, CBR, and PDF files by downloading them to a temporary cache. Also, implements placeholder artwork for local media items when embedded artwork is not found. Updates MediaNotificationService to use MediaSessionManager. Cleans up error handling in SettingsScreen. Co-authored-by: kalaeon <kalaeon@gmail.com>
|
Cursor Agent can help with this pull request. Just |
Codoki PR ReviewSummary: Harden stream downloads: parse extensions safely and use timeouts, avoid CBR mis-detection and potential hangs Issues (Critical & High only)
Showing top 2 issues. Critical: 0, High: 2. See inline suggestions for more. Key Feedback (click to expand)
Confidence: 3/5 — Needs work before merge (2 high · status: Requires changes) Sequence DiagramsequenceDiagram
participant Caller
participant ComicReaderEngine
participant Network
Caller->>ComicReaderEngine: open(BookSource)
alt BookSource.Stream
ComicReaderEngine->>Network: downloadToTempFile(url)
Network-->>ComicReaderEngine: temp File
ComicReaderEngine->>ComicReaderEngine: detect format (CBZ/CBR)
ComicReaderEngine-->>Caller: Result.success(Unit) | failure
else BookSource.Local
ComicReaderEngine->>ComicReaderEngine: open from File
ComicReaderEngine-->>Caller: Result.success(Unit) | failure
end
React with 👍 or 👎 if you found this review useful. |
WalkthroughAdds placeholder fallback for missing artwork. Implements remote source handling by downloading to temp files in EPUB, PDF, and comic readers; extends comic reader to support CBR via junrar. Integrates Hilt DI into MediaNotificationService and replaces a playback MediaSessionManager with a deprecated stub. Deprecates web fiction models. Restores SettingsScreen error handling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Reader Client
participant Engine as ReaderEngine (EPUB/PDF/Comic)
participant DL as downloadToTempFile()
participant FS as Cache FS
participant Open as Local Open/Renderer
User->>UI: Open remote book
UI->>Engine: open(BookSource.Stream(url))
Engine->>DL: request temp file for url
DL->>FS: create temp file with inferred ext
DL->>DL: stream URL -> write to temp file
DL-->>Engine: return File
Engine->>Open: open(File)
alt Comic CBR (.cbr/.rar)
note over Engine,Open: Initialize junrar Archive and page headers
Open-->>UI: Ready (CBR pages via RAR extraction)
else CBZ/EPUB/PDF
Open-->>UI: Ready (standard rendering)
end
sequenceDiagram
autonumber
participant OS as Android
participant Svc as MediaNotificationService
participant DI as Hilt DI
participant MSM as MediaSessionManager
participant MS as MediaSession
OS->>Svc: onCreate()
Svc->>DI: request dependencies
DI-->>Svc: inject mediaSessionManager
Svc->>MSM: getMediaSession()
MSM-->>Svc: MediaSession
Svc->>MS: use session for notifications
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
CleverFerret/src/main/java/com/universalmedialibrary/services/media/MediaNotificationService.kt (2)
123-127: UseMediaSessionManager.releaseSession()instead of manual release inonDestroy
Directly callingmediaSession?.release()bypasses the manager’s cleanup and leaves its internal state inconsistent. Replace the manual release with:override fun onDestroy() { mediaSessionManager.releaseSession() super.onDestroy() }
158-182: DeclareserviceScopeand injectartworkLoader
serviceScopeandartworkLoaderare referenced inMediaNotificationServicebut not defined. Follow the established pattern: addclass MediaNotificationService @Singleton @Inject constructor( @ApplicationContext private val context: Context, private val mediaSessionManager: MediaSessionManager, private val artworkLoader: ArtworkLoader ) : MediaSessionService() { private val serviceScope = CoroutineScope(SupervisorJob() + Dispatchers.Main) // … }
♻️ Duplicate comments (2)
CleverFerret/src/main/java/com/universalmedialibrary/services/epub/EpubReaderEngine.kt (1)
112-125: Duplicate code: Extract downloadToTempFile to shared utility.This function is identical to the versions in
ComicReaderEngine.kt(lines 128-141) andPdfReaderEngine.kt(lines 110-123), violating the DRY principle. Additionally, the function has the same critical issues identified in the ComicReaderEngine review:
- No timeout on network operations
- No error handling or context
- SSRF vulnerability (no URL validation)
- Resource leak (temp files never cleaned up)
- Blocking I/O without proper coroutine context
- Poor extension parsing with query parameters
Please refer to the review comment on
ComicReaderEngine.kt(lines 128-141) for the detailed solution and recommended refactoring approach.CleverFerret/src/main/java/com/universalmedialibrary/services/reader/PdfReaderEngine.kt (1)
110-123: Duplicate code: Extract downloadToTempFile to shared utility.This function is identical to the versions in
ComicReaderEngine.kt(lines 128-141) andEpubReaderEngine.kt(lines 112-125), violating the DRY principle. Additionally, the function has the same critical issues identified in the ComicReaderEngine review:
- No timeout on network operations
- No error handling or context
- SSRF vulnerability (no URL validation)
- Resource leak (temp files never cleaned up)
- Blocking I/O without proper coroutine context
- Poor extension parsing with query parameters
Please refer to the review comment on
ComicReaderEngine.kt(lines 128-141) for the detailed solution and recommended refactoring approach.
🧹 Nitpick comments (1)
CleverFerret/src/main/java/com/universalmedialibrary/services/artwork/ArtworkLoader.kt (1)
88-90: Consider extracting the hardcoded resource name to a constant.The placeholder resource name
"placeholder_book_cover"is hardcoded in the method, making it harder to maintain or customize.Extract it to a companion object constant:
companion object { private const val PLACEHOLDER_RESOURCE_NAME = "placeholder_book_cover" }Then use it in the method:
val resId = context.resources.getIdentifier( - "placeholder_book_cover", + PLACEHOLDER_RESOURCE_NAME, "drawable", context.packageName )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CleverFerret/src/main/java/com/universalmedialibrary/services/artwork/ArtworkLoader.kt(1 hunks)CleverFerret/src/main/java/com/universalmedialibrary/services/epub/EpubReaderEngine.kt(3 hunks)CleverFerret/src/main/java/com/universalmedialibrary/services/media/MediaNotificationService.kt(3 hunks)CleverFerret/src/main/java/com/universalmedialibrary/services/reader/ComicReaderEngine.kt(3 hunks)CleverFerret/src/main/java/com/universalmedialibrary/services/reader/PdfReaderEngine.kt(3 hunks)CleverFerret/src/main/java/com/universalmedialibrary/ui/settings/SettingsScreen.kt(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
CleverFerret/src/main/java/com/universalmedialibrary/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place app source under package path com/universalmedialibrary within the CleverFerret module
Files:
CleverFerret/src/main/java/com/universalmedialibrary/services/artwork/ArtworkLoader.ktCleverFerret/src/main/java/com/universalmedialibrary/services/epub/EpubReaderEngine.ktCleverFerret/src/main/java/com/universalmedialibrary/services/reader/ComicReaderEngine.ktCleverFerret/src/main/java/com/universalmedialibrary/ui/settings/SettingsScreen.ktCleverFerret/src/main/java/com/universalmedialibrary/services/reader/PdfReaderEngine.ktCleverFerret/src/main/java/com/universalmedialibrary/services/media/MediaNotificationService.kt
🧬 Code graph analysis (3)
CleverFerret/src/main/java/com/universalmedialibrary/services/epub/EpubReaderEngine.kt (2)
CleverFerret/src/main/java/com/universalmedialibrary/services/reader/ComicReaderEngine.kt (1)
downloadToTempFile(131-141)CleverFerret/src/main/java/com/universalmedialibrary/services/reader/PdfReaderEngine.kt (1)
downloadToTempFile(113-123)
CleverFerret/src/main/java/com/universalmedialibrary/services/reader/ComicReaderEngine.kt (2)
CleverFerret/src/main/java/com/universalmedialibrary/services/epub/EpubReaderEngine.kt (1)
downloadToTempFile(115-125)CleverFerret/src/main/java/com/universalmedialibrary/services/reader/PdfReaderEngine.kt (1)
downloadToTempFile(113-123)
CleverFerret/src/main/java/com/universalmedialibrary/services/reader/PdfReaderEngine.kt (2)
CleverFerret/src/main/java/com/universalmedialibrary/services/epub/EpubReaderEngine.kt (1)
downloadToTempFile(115-125)CleverFerret/src/main/java/com/universalmedialibrary/services/reader/ComicReaderEngine.kt (1)
downloadToTempFile(131-141)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: API 24 (x86)
- GitHub Check: API 30 (x86_64)
- GitHub Check: Codoki PR Review
- GitHub Check: Run Tests
- GitHub Check: Code Quality & Linting
🔇 Additional comments (9)
CleverFerret/src/main/java/com/universalmedialibrary/services/reader/ComicReaderEngine.kt (2)
16-17: LGTM!The imports are necessary to support the network streaming functionality.
74-80: Verify error propagation from downloadToTempFile.The Stream handling now downloads remote CBZ/CBR files to a temporary location before processing. Ensure that any exceptions thrown by
downloadToTempFileare properly caught and handled by the outer try-catch block (lines 120-124).CleverFerret/src/main/java/com/universalmedialibrary/services/epub/EpubReaderEngine.kt (2)
17-18: LGTM!The imports are necessary to support the network streaming functionality.
74-82: Verify error propagation from downloadToTempFile.The Stream handling now downloads remote EPUB files to a temporary location before processing. Ensure that any exceptions thrown by
downloadToTempFileare properly caught and handled by the outer try-catch block (lines 106-108).CleverFerret/src/main/java/com/universalmedialibrary/services/reader/PdfReaderEngine.kt (2)
16-17: LGTM!The imports are necessary to support the network streaming functionality.
72-80: Verify error propagation from downloadToTempFile.The Stream handling now downloads remote PDF files to a temporary location before processing. Ensure that any exceptions thrown by
downloadToTempFileare properly caught and handled by the outer try-catch block (lines 102-106).CleverFerret/src/main/java/com/universalmedialibrary/services/media/MediaNotificationService.kt (3)
42-42: LGTM!The
@AndroidEntryPointannotation correctly enables Dagger Hilt dependency injection for this service.
48-50: LGTM!The
MediaSessionManagerdependency injection is correctly set up using@Injectandlateinit var.
88-89: LGTM!The change to obtain
MediaSessionfrom the central manager is correct and aligns with the centralized session management pattern.
| // Fallback: try a placeholder from resources if available (avoids nulls in UI) | ||
| val placeholder = runCatching { | ||
| val resId = context.resources.getIdentifier( | ||
| "placeholder_book_cover", | ||
| "drawable", | ||
| context.packageName | ||
| ) | ||
| if (resId != 0) BitmapFactory.decodeResource(context.resources, resId) else null | ||
| }.getOrNull() | ||
|
|
||
| Log.d(TAG, "No embedded artwork found for: ${mediaItem.fileName}; returning placeholder=${placeholder != null}") | ||
| placeholder |
There was a problem hiding this comment.
Critical: Missing artwork loading logic before placeholder fallback.
The function now jumps directly from cache lookup (lines 83-84) to placeholder fallback (lines 86-97) without any code path to actually load artwork from the MediaItem. This means:
- If the cache misses, the function never attempts to load the actual artwork
- Users will always see the placeholder (or null) for uncached items
- The core functionality of loading artwork from media files is broken
The artwork loading logic (e.g., extracting cover from EPUB, loading from file path, etc.) should be attempted between the cache check and the placeholder fallback. The placeholder should only be returned when actual artwork loading fails or returns null.
Expected flow:
// Try cache first
val cacheKey = getCacheKey(mediaItem.itemId.toString(), maxWidth, maxHeight)
memoryCache.get(cacheKey)?.let { return@withContext it }
// Try to load actual artwork from MediaItem
val artwork = loadArtworkFromMediaItem(mediaItem, maxWidth, maxHeight)
if (artwork != null) {
memoryCache.put(cacheKey, artwork)
return@withContext artwork
}
// Fallback: try a placeholder from resources if available
val placeholder = runCatching {
// ... existing placeholder logic
}.getOrNull()Cache the placeholder to avoid redundant resource loading.
The placeholder bitmap is loaded from resources on every cache miss without being cached itself. This causes unnecessary resource decoding on each call.
Apply this diff to cache the placeholder:
// Fallback: try a placeholder from resources if available (avoids nulls in UI)
val placeholder = runCatching {
val resId = context.resources.getIdentifier(
"placeholder_book_cover",
"drawable",
context.packageName
)
if (resId != 0) BitmapFactory.decodeResource(context.resources, resId) else null
}.getOrNull()
+// Cache placeholder if loaded
+placeholder?.let { memoryCache.put(cacheKey, it) }
+
Log.d(TAG, "No embedded artwork found for: ${mediaItem.fileName}; returning placeholder=${placeholder != null}")
placeholder🤖 Prompt for AI Agents
In
CleverFerret/src/main/java/com/universalmedialibrary/services/artwork/ArtworkLoader.kt
around lines 86–97, the placeholder bitmap is decoded from resources on every
cache miss; change this to a single cached instance to avoid repeated decoding
and potential GC churn. Add a private nullable cachedPlaceholder in the class
(or a companion object) and on first request (synchronized or using Kotlin's
lazy) resolve the drawable id via
context.applicationContext.resources.getIdentifier and decode the bitmap into
cachedPlaceholder (or leave null if id==0); subsequent calls should return the
cachedPlaceholder without re-decoding; ensure you use applicationContext to
avoid leaking an Activity and handle the null case as currently done.
| if (resId != 0) BitmapFactory.decodeResource(context.resources, resId) else null | ||
| }.getOrNull() | ||
|
|
||
| Log.d(TAG, "No embedded artwork found for: ${mediaItem.fileName}; returning placeholder=${placeholder != null}") |
There was a problem hiding this comment.
Misleading log message about embedded artwork.
The log states "No embedded artwork found" but the code never attempts to load or check for embedded artwork—it jumps directly to the placeholder fallback after a cache miss.
Update the log message to accurately reflect what happened:
-Log.d(TAG, "No embedded artwork found for: ${mediaItem.fileName}; returning placeholder=${placeholder != null}")
+Log.d(TAG, "Cache miss for: ${mediaItem.fileName}; returning placeholder=${placeholder != null}")Or if artwork loading is added (as suggested in the critical issue above), update it to:
-Log.d(TAG, "No embedded artwork found for: ${mediaItem.fileName}; returning placeholder=${placeholder != null}")
+Log.d(TAG, "Artwork loading failed for: ${mediaItem.fileName}; returning placeholder=${placeholder != null}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Log.d(TAG, "No embedded artwork found for: ${mediaItem.fileName}; returning placeholder=${placeholder != null}") | |
| Log.d(TAG, "Cache miss for: ${mediaItem.fileName}; returning placeholder=${placeholder != null}") |
🤖 Prompt for AI Agents
In
CleverFerret/src/main/java/com/universalmedialibrary/services/artwork/ArtworkLoader.kt
around line 96, the log message incorrectly claims "No embedded artwork found"
even though the code never checks for embedded artwork and immediately falls
back to the placeholder after a cache miss; either (A) change the log text to
accurately reflect the actual behavior (e.g., "No artwork in cache for:
${mediaItem.fileName}; returning placeholder=${placeholder != null}") or (B)
implement the missing embedded-artwork load attempt before the placeholder
fallback and update the log to reflect the new flow (e.g., "No artwork in cache;
attempted embedded artwork; returning placeholder=..."). Ensure the chosen fix
updates only the log/message text or inserts a clear embedded-artwork load step
prior to the placeholder fallback so logs match runtime behavior.
| import androidx.media3.common.Player | ||
| import androidx.media3.session.MediaSession | ||
| import dagger.hilt.android.AndroidEntryPoint | ||
| import javax.inject.Inject |
There was a problem hiding this comment.
Remove duplicate import statement.
javax.inject.Inject is imported twice (lines 16 and 26), which will cause a compilation error.
Apply this diff to remove the duplicate import:
import androidx.media3.session.MediaSessionService
import com.universalmedialibrary.MainActivity
import com.universalmedialibrary.R
import com.universalmedialibrary.services.artwork.ArtworkLoader
import com.universalmedialibrary.data.local.entity.MediaItem
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.SupervisorJob
import kotlinx.coroutines.launch
-import javax.inject.InjectAlso applies to: 26-26
🤖 Prompt for AI Agents
In
CleverFerret/src/main/java/com/universalmedialibrary/services/media/MediaNotificationService.kt
around lines 16 and 26, there is a duplicated import of javax.inject.Inject;
remove the redundant import (keep a single javax.inject.Inject import) so the
file has only one import statement for that symbol and recompile to ensure no
other duplicate imports remain.
| /** | ||
| * Download the content at [url] to a temporary file in cache directory and return the file. | ||
| */ | ||
| private fun downloadToTempFile(context: Context, url: String, defaultExtension: String): File { | ||
| val guessedExt = url.substringAfterLast('.', missingDelimiterValue = defaultExtension) | ||
| .lowercase().ifEmpty { defaultExtension } | ||
| val temp = File(context.cacheDir, "stream_${System.currentTimeMillis()}.$guessedExt") | ||
| URL(url).openStream().use { input -> | ||
| FileOutputStream(temp).use { output -> | ||
| input.copyTo(output) | ||
| } | ||
| } | ||
| return temp | ||
| } |
There was a problem hiding this comment.
Address critical issues in downloadToTempFile.
The function has several critical and major issues:
- Code duplication: This function is identically duplicated in
EpubReaderEngine.ktandPdfReaderEngine.kt. Extract to a shared utility class. - No timeout:
URL.openStream()has no timeout, risking indefinite hangs on slow/unresponsive servers. - No error handling: Network I/O can fail in many ways (DNS, connection refused, timeout, incomplete downloads). Exceptions propagate without context.
- SSRF vulnerability: No URL validation allows potential Server-Side Request Forgery attacks (e.g.,
file://,ftp://, internal IPs). - Resource leak: Temp files accumulate in cache and are never cleaned up.
- Blocking I/O: Not wrapped in
withContext(Dispatchers.IO), though the caller is a suspend function. - Extension parsing: Fails with query parameters (e.g.,
file.cbz?token=xyz→ extension becomesxyz).
Recommended solution:
- Extract to a shared utility:
// Create new file: CleverFerret/src/main/java/com/universalmedialibrary/services/reader/NetworkUtils.kt
package com.universalmedialibrary.services.reader
import android.content.Context
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.withContext
import java.io.File
import java.io.FileOutputStream
import java.net.HttpURLConnection
import java.net.URL
object NetworkUtils {
private const val CONNECT_TIMEOUT_MS = 15_000
private const val READ_TIMEOUT_MS = 30_000
private val ALLOWED_SCHEMES = setOf("http", "https")
suspend fun downloadToTempFile(
context: Context,
url: String,
defaultExtension: String
): File = withContext(Dispatchers.IO) {
// Validate URL scheme
val uri = URL(url)
require(uri.protocol in ALLOWED_SCHEMES) {
"Only HTTP(S) URLs are allowed, got: ${uri.protocol}"
}
// Extract extension, ignoring query parameters
val path = uri.path ?: ""
val guessedExt = path.substringAfterLast('/', "")
.substringAfterLast('.', defaultExtension)
.takeIf { it.isNotEmpty() && !it.contains('/') }
?.lowercase()
?: defaultExtension
val temp = File(context.cacheDir, "stream_${System.currentTimeMillis()}_${System.nanoTime()}.$guessedExt")
try {
val connection = uri.openConnection() as HttpURLConnection
connection.connectTimeout = CONNECT_TIMEOUT_MS
connection.readTimeout = READ_TIMEOUT_MS
connection.inputStream.use { input ->
FileOutputStream(temp).use { output ->
input.copyTo(output)
}
}
temp
} catch (e: Exception) {
temp.delete() // Clean up on failure
throw IllegalStateException("Failed to download from $url", e)
}
}
fun cleanupOldTempFiles(context: Context, maxAgeMs: Long = 24 * 60 * 60 * 1000) {
val now = System.currentTimeMillis()
context.cacheDir.listFiles { file ->
file.name.startsWith("stream_") && (now - file.lastModified()) > maxAgeMs
}?.forEach { it.delete() }
}
}- Replace the duplicated functions in all three reader engines:
- private fun downloadToTempFile(context: Context, url: String, defaultExtension: String): File {
- val guessedExt = url.substringAfterLast('.', missingDelimiterValue = defaultExtension)
- .lowercase().ifEmpty { defaultExtension }
- val temp = File(context.cacheDir, "stream_${System.currentTimeMillis()}.$guessedExt")
- URL(url).openStream().use { input ->
- FileOutputStream(temp).use { output ->
- input.copyTo(output)
- }
- }
- return temp
- }- Update the Stream handling to use the shared utility:
is BookSource.Stream -> {
// Download remote CBZ/CBR to a temporary file, support CBZ immediately
- val tempFile = downloadToTempFile(
+ val tempFile = NetworkUtils.downloadToTempFile(
context = context,
url = source.url,
defaultExtension = "cbz"
)
tempFile
}- Add periodic cleanup (e.g., in Application.onCreate() or when opening a book):
NetworkUtils.cleanupOldTempFiles(context)🤖 Prompt for AI Agents
In
CleverFerret/src/main/java/com/universalmedialibrary/services/reader/ComicReaderEngine.kt
around lines 128-141, the downloadToTempFile implementation is duplicated, lacks
timeouts, error handling, URL validation, IO dispatcher usage, safe extension
parsing, and temp-file cleanup; extract this logic to a shared utility (e.g.,
NetworkUtils) and replace duplicates with a suspend wrapper that uses
withContext(Dispatchers.IO), opens an HttpURLConnection with reasonable
connect/read timeouts, validates the URL scheme (allow only http/https and
reject suspicious hosts/addresses), parse the extension from the URL path
(ignore query params), write to a uniquely named temp file, delete the temp on
failure, wrap exceptions with contextual message, and provide a
cleanupOldTempFiles(context, maxAgeMs) method to periodically remove stale
stream_* files (call from Application.onCreate or book-open path).
| uiState.error?.let { error -> | ||
| LaunchedEffect(error) { | ||
| // Show snackbar or toast | ||
| // For now, we'll just clear the error after displaying | ||
| // Placeholder: Could hook into a SnackbarHost in the Scaffold | ||
| viewModel.clearError() | ||
| } | ||
| } |
There was a problem hiding this comment.
Implement user-facing error display before clearing the error.
The error is cleared immediately without showing it to the user, creating a silent failure. Users will have no visibility into what went wrong, defeating the purpose of error state management.
To properly implement error handling, add a SnackbarHostState and display the error before clearing it:
@Composable
fun SettingsScreen(
navController: NavController,
viewModel: SettingsViewModel = hiltViewModel()
) {
val uiState by viewModel.uiState.collectAsState()
+ val snackbarHostState = remember { SnackbarHostState() }
Scaffold(
+ snackbarHost = { SnackbarHost(snackbarHostState) },
topBar = {
TopAppBar(Then update the error handling to display the message:
// Error handling
uiState.error?.let { error ->
LaunchedEffect(error) {
- // Placeholder: Could hook into a SnackbarHost in the Scaffold
+ snackbarHostState.showSnackbar(
+ message = error,
+ duration = SnackbarDuration.Short
+ )
viewModel.clearError()
}
}🤖 Prompt for AI Agents
In
CleverFerret/src/main/java/com/universalmedialibrary/ui/settings/SettingsScreen.kt
around lines 114–119, the code clears uiState.error immediately without
presenting it to the user; instead create a SnackbarHostState (remember to pass
it into Scaffold's snackbarHost), then in the LaunchedEffect(error) call
snackbarHostState.showSnackbar(error.message ?: "An error occurred") and only
after showSnackbar completes invoke viewModel.clearError(); ensure
SnackbarHostState is remembered (e.g., val snackbarHostState = remember {
SnackbarHostState() }) and that you import and use the proper coroutine-friendly
showSnackbar API.
…nager stub Co-authored-by: kalaeon <kalaeon@gmail.com>
![codoki-pr-intelligence[bot]](https://avatars.githubusercontent.com/in/1312414?s=60&v=4){kind=small}
| } | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
|
|
||
| /** | ||
| * Download the content at [url] to a temporary file in cache directory and return the file. | ||
| */ |
There was a problem hiding this comment.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
CleverFerret/src/main/java/com/universalmedialibrary/services/reader/ComicReaderEngine.kt (1)
147-160: ReworkdownloadToTempFilefor security, resilience, and deduplication.This helper still has the previously flagged blockers:
URL.openStream()runs without connect/read timeouts, no scheme validation (SSRF vector), brittle extension parsing (query strings turn into bogus extensions), no cleanup on failure, and the same code is now triplicated across reader engines. Centralize the logic (e.g., sharedNetworkDownloadUtils) with explicit HTTP(S) allow‑list,HttpURLConnectiontimeouts, safe path parsing excluding queries/fragments,withContext(Dispatchers.IO)wrapping, and cache cleanup for orphaned temp files before we can ship.- private fun downloadToTempFile(context: Context, url: String, defaultExtension: String): File { - val guessedExt = url.substringAfterLast('.', missingDelimiterValue = defaultExtension) - .lowercase().ifEmpty { defaultExtension } - val temp = File(context.cacheDir, "stream_${System.currentTimeMillis()}.$guessedExt") - URL(url).openStream().use { input -> - FileOutputStream(temp).use { output -> - input.copyTo(output) - } - } - return temp - } + private suspend fun downloadToTempFile(context: Context, url: String, defaultExtension: String): File = + NetworkDownloadUtils.downloadToTempFile(context, url, defaultExtension)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CleverFerret/src/main/java/com/universalmedialibrary/services/playback/MediaSessionManager.kt(1 hunks)CleverFerret/src/main/java/com/universalmedialibrary/services/reader/ComicReaderEngine.kt(7 hunks)CleverFerret/src/main/java/com/universalmedialibrary/services/webfiction/WebFictionModels.kt(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
CleverFerret/src/main/java/com/universalmedialibrary/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place app source under package path com/universalmedialibrary within the CleverFerret module
Files:
CleverFerret/src/main/java/com/universalmedialibrary/services/playback/MediaSessionManager.ktCleverFerret/src/main/java/com/universalmedialibrary/services/webfiction/WebFictionModels.ktCleverFerret/src/main/java/com/universalmedialibrary/services/reader/ComicReaderEngine.kt
🧬 Code graph analysis (1)
CleverFerret/src/main/java/com/universalmedialibrary/services/reader/ComicReaderEngine.kt (1)
CleverFerret/src/main/java/com/universalmedialibrary/services/reader/PdfReaderEngine.kt (1)
downloadToTempFile(113-123)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: API 34 (x86_64)
- GitHub Check: API 24 (x86)
- GitHub Check: API 30 (x86_64)
- GitHub Check: Codoki PR Review
- GitHub Check: Run Tests
- GitHub Check: Code Quality & Linting
| // Deprecated stub; consolidated into services.media.MediaSessionManager | ||
| @Deprecated("Use com.universalmedialibrary.services.media.MediaSessionManager") | ||
| class MediaSessionManager |
There was a problem hiding this comment.
Don't leave this as a no-op stub
The previous implementation exposed real behavior; replacing it with an empty class means any lingering injections or reflective usages will now succeed yet do nothing, leading to silent breakage. If you need a transitional shim, please delegate to the new services.media.MediaSessionManager (or use a Kotlin typealias) so callers still get working behavior while the deprecation guides them to migrate.
| @Deprecated("Use models defined in WebFictionService.kt") | ||
| object WebFictionModels |
There was a problem hiding this comment.
🧩 Analysis chain
Enhance deprecation message and verify migration completeness.
This breaking change removes all concrete web fiction models and replaces them with a stub. Consider the following improvements:
- Add a
ReplaceWithannotation to specify exact replacements, enabling automated migration - Provide more specific guidance about which models in
WebFictionService.ktreplace each removed model - Consider whether this file should exist at all—if all models have been moved, delete the file entirely rather than keeping a deprecated stub
Apply this diff to improve the deprecation message with a ReplaceWith annotation (adjust the replacement as appropriate):
-@Deprecated("Use models defined in WebFictionService.kt")
+@Deprecated(
+ message = "Use models defined in WebFictionService.kt",
+ replaceWith = ReplaceWith(
+ expression = "WebFictionService",
+ imports = ["com.universalmedialibrary.services.webfiction.WebFictionService"]
+ )
+)
object WebFictionModelsRun the following script to verify that all usages of the removed models have been migrated and that WebFictionService.kt contains the replacement models:
🏁 Script executed:
#!/bin/bash
# Check for any remaining usages of the old model classes
echo "=== Checking for usages of removed models ==="
rg -n --type kotlin -C3 '\b(WebFictionSite|WebFictionStory|WebFictionChapter|WebFictionDownloadProgress|WebFictionResult|StoryStatus|DownloadStatus)\b' \
-g '!**/WebFictionModels.kt'
echo ""
echo "=== Verifying WebFictionService.kt contains replacement models ==="
fd -e kt 'WebFictionService.kt' --exec cat {}Length of output: 18974
Complete deprecation mapping for all models and verify migration.
- Add a
ReplaceWithon each deprecated model inWebFictionModels.ktto point to its replacement inWebFictionService.kt(e.g.WebFictionStory,WebFictionChapter,WebFictionSite) and define or removeWebFictionDownloadProgress,WebFictionResult,StoryStatus,DownloadStatus. - If all models have been migrated, delete
WebFictionModels.ktinstead of keeping an empty stub. - Verify that no code references any of the removed types.
|
@cursoragent read through all reviews. |
|
I tried to locate "clever ferret" by listing files and searching the current directory, but I couldn't find any direct references to it. To help me scan for missing features or broken code, I need more information. Could you please clarify what "clever ferret" refers to? For example, is it a specific file, a directory, a module, or a conceptual part of the application? Once I understand what "clever ferret" is, I can then assist you by listing files, searching for patterns, or analyzing code structure to identify missing features or broken code. Learn more about Cursor Agents |
There was a problem hiding this comment.
Pull Request Overview
Adds remote streaming support for EPUB/PDF/CBZ/CBR, media session/notification integration, placeholder artwork fallback, and reinstates settings error handling while deprecating legacy web fiction and playback session models.
- Implements network-backed BookSource.Stream handling across readers (EPUB, PDF, Comics) with temp file downloads.
- Introduces CBR (RAR) comic support and artwork placeholder logic.
- Deprecates prior MediaSessionManager and WebFictionModels, replacing them with stubs (introduces potential breaking changes).
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| SettingsScreen.kt | Restores previously commented-out error handling block. |
| WebFictionModels.kt | Removes all model definitions and replaces them with a deprecated empty object. |
| PdfReaderEngine.kt | Adds remote streaming (download to temp) for PDFs. |
| ComicReaderEngine.kt | Adds remote streaming and full CBR (RAR) archive support plus page extraction. |
| MediaSessionManager.kt | Replaces prior implementation with deprecated empty stub. |
| MediaNotificationService.kt | Injects MediaSessionManager and attempts to obtain a MediaSession. |
| EpubReaderEngine.kt | Adds remote streaming (download to temp) for EPUBs. |
| ArtworkLoader.kt | Adds placeholder artwork fallback when embedded cover is absent. |
| object Loading : WebFictionResult() | ||
| } | ||
| @Deprecated("Use models defined in WebFictionService.kt") | ||
| object WebFictionModels |
There was a problem hiding this comment.
All previously defined data classes (WebFictionStory, WebFictionChapter, etc.) were removed and replaced with an empty deprecated object, creating a breaking API change for any existing references. Consider retaining the old types with @deprecated annotations or providing typealiases forwarding to the new model locations to preserve binary/source compatibility during a transition period.
| object WebFictionModels | |
| object WebFictionModels | |
| // Restore deprecated data classes for compatibility, forwarding to new models | |
| import com.universalmedialibrary.services.webfiction.WebFictionService.WebFictionStory as NewWebFictionStory | |
| import com.universalmedialibrary.services.webfiction.WebFictionService.WebFictionChapter as NewWebFictionChapter | |
| import com.universalmedialibrary.services.webfiction.WebFictionService.WebFictionAuthor as NewWebFictionAuthor | |
| @Deprecated("Use WebFictionService.WebFictionStory instead", ReplaceWith("WebFictionService.WebFictionStory")) | |
| typealias WebFictionStory = NewWebFictionStory | |
| @Deprecated("Use WebFictionService.WebFictionChapter instead", ReplaceWith("WebFictionService.WebFictionChapter")) | |
| typealias WebFictionChapter = NewWebFictionChapter | |
| @Deprecated("Use WebFictionService.WebFictionAuthor instead", ReplaceWith("WebFictionService.WebFictionAuthor")) | |
| typealias WebFictionAuthor = NewWebFictionAuthor |
Copilot uses AI. Check for mistakes.
| // Deprecated stub; consolidated into services.media.MediaSessionManager | ||
| @Deprecated("Use com.universalmedialibrary.services.media.MediaSessionManager") | ||
| class MediaSessionManager |
There was a problem hiding this comment.
This stub removes the original injectable @singleton implementation and all its public methods (e.g., updatePlaybackState, updateMetadata, getSessionToken). Existing injection points or method calls will now fail to compile. Either keep the original implementation marked @deprecated or supply a delegating shim that preserves the prior API surface.
| // Deprecated stub; consolidated into services.media.MediaSessionManager | |
| @Deprecated("Use com.universalmedialibrary.services.media.MediaSessionManager") | |
| class MediaSessionManager | |
| import javax.inject.Inject | |
| import javax.inject.Singleton | |
| import com.universalmedialibrary.services.media.MediaSessionManager as NewMediaSessionManager | |
| /** | |
| * @deprecated Use com.universalmedialibrary.services.media.MediaSessionManager | |
| */ | |
| @Deprecated("Use com.universalmedialibrary.services.media.MediaSessionManager") | |
| @Singleton | |
| class MediaSessionManager @Inject constructor( | |
| private val delegate: NewMediaSessionManager | |
| ) { | |
| /** | |
| * @deprecated Use com.universalmedialibrary.services.media.MediaSessionManager.updatePlaybackState | |
| */ | |
| @Deprecated("Use com.universalmedialibrary.services.media.MediaSessionManager.updatePlaybackState") | |
| fun updatePlaybackState(state: Any) = delegate.updatePlaybackState(state) | |
| /** | |
| * @deprecated Use com.universalmedialibrary.services.media.MediaSessionManager.updateMetadata | |
| */ | |
| @Deprecated("Use com.universalmedialibrary.services.media.MediaSessionManager.updateMetadata") | |
| fun updateMetadata(metadata: Any) = delegate.updateMetadata(metadata) | |
| /** | |
| * @deprecated Use com.universalmedialibrary.services.media.MediaSessionManager.getSessionToken | |
| */ | |
| @Deprecated("Use com.universalmedialibrary.services.media.MediaSessionManager.getSessionToken") | |
| fun getSessionToken(): Any = delegate.getSessionToken() | |
| } |
Copilot uses AI. Check for mistakes.
| // Obtain MediaSession from the central manager if available | ||
| mediaSession = mediaSessionManager.getMediaSession() |
There was a problem hiding this comment.
mediaSessionManager.getMediaSession() is invoked, but the replaced stub MediaSessionManager no longer defines this method, causing a compilation error. Restore the method (even as a temporary no-op returning null) or refactor this service to obtain the MediaSession from the new consolidated manager.
Copilot uses AI. Check for mistakes.
| private fun downloadToTempFile(context: Context, url: String, defaultExtension: String): File { | ||
| val guessedExt = url.substringAfterLast('.', missingDelimiterValue = defaultExtension) | ||
| .lowercase().ifEmpty { defaultExtension } | ||
| val temp = File(context.cacheDir, "stream_${System.currentTimeMillis()}.$guessedExt") | ||
| URL(url).openStream().use { input -> | ||
| FileOutputStream(temp).use { output -> | ||
| input.copyTo(output) | ||
| } | ||
| } | ||
| return temp | ||
| } |
There was a problem hiding this comment.
Unbounded download: no connection/read timeouts, no size limiting, and no cleanup of the temporary file, which can lead to hanging coroutines and cache bloat. Use a configurable HTTP client (e.g., OkHttp) with timeouts, validate content length before copying, and schedule deletion (e.g., temp.deleteOnExit or managed eviction) after use.
Copilot uses AI. Check for mistakes.
| private fun downloadToTempFile(context: Context, url: String, defaultExtension: String): File { | ||
| val guessedExt = url.substringAfterLast('.', missingDelimiterValue = defaultExtension) | ||
| .lowercase().ifEmpty { defaultExtension } | ||
| val temp = File(context.cacheDir, "stream_${System.currentTimeMillis()}.$guessedExt") | ||
| URL(url).openStream().use { input -> | ||
| FileOutputStream(temp).use { output -> | ||
| input.copyTo(output) | ||
| } | ||
| } | ||
| return temp | ||
| } |
There was a problem hiding this comment.
Same download logic as other reader engines duplicated without timeouts, size checks, or lifecycle cleanup; this risks large file downloads and accumulation of temp files. Refactor into a shared utility with connection/read timeouts and a retention policy for temporary files.
Copilot uses AI. Check for mistakes.
| URL(url).openStream().use { input -> | ||
| FileOutputStream(temp).use { output -> | ||
| input.copyTo(output) | ||
| } | ||
| } | ||
| return temp |
There was a problem hiding this comment.
Duplicate temp download implementation without timeout controls or cleanup strategy can cause hanging downloads and disk growth. Centralize this logic and enforce connection/read timeouts plus a max size guard (e.g., abort if Content-Length exceeds a threshold).
| URL(url).openStream().use { input -> | |
| FileOutputStream(temp).use { output -> | |
| input.copyTo(output) | |
| } | |
| } | |
| return temp | |
| val MAX_SIZE_BYTES = 100 * 1024 * 1024 // 100 MB | |
| var success = false | |
| var connection: java.net.HttpURLConnection? = null | |
| try { | |
| val urlObj = URL(url) | |
| connection = urlObj.openConnection() as java.net.HttpURLConnection | |
| connection.connectTimeout = 10_000 // 10 seconds | |
| connection.readTimeout = 30_000 // 30 seconds | |
| connection.instanceFollowRedirects = true | |
| connection.requestMethod = "GET" | |
| connection.connect() | |
| val contentLength = connection.getHeaderField("Content-Length")?.toLongOrNull() | |
| if (contentLength != null && contentLength > MAX_SIZE_BYTES) { | |
| throw java.io.IOException("File too large: $contentLength bytes") | |
| } | |
| connection.inputStream.use { input -> | |
| FileOutputStream(temp).use { output -> | |
| val buffer = ByteArray(8192) | |
| var totalRead = 0L | |
| while (true) { | |
| val read = input.read(buffer) | |
| if (read == -1) break | |
| totalRead += read | |
| if (totalRead > MAX_SIZE_BYTES) { | |
| throw java.io.IOException("Downloaded file exceeds max size ($MAX_SIZE_BYTES bytes)") | |
| } | |
| output.write(buffer, 0, read) | |
| } | |
| } | |
| } | |
| success = true | |
| return temp | |
| } finally { | |
| connection?.disconnect() | |
| if (!success && temp.exists()) { | |
| temp.delete() | |
| } | |
| } |
Copilot uses AI. Check for mistakes.
| val placeholder = runCatching { | ||
| val resId = context.resources.getIdentifier( | ||
| "placeholder_book_cover", | ||
| "drawable", | ||
| context.packageName | ||
| ) | ||
| if (resId != 0) BitmapFactory.decodeResource(context.resources, resId) else null | ||
| }.getOrNull() | ||
|
|
||
| Log.d(TAG, "No embedded artwork found for: ${mediaItem.fileName}; returning placeholder=${placeholder != null}") | ||
| placeholder |
There was a problem hiding this comment.
The placeholder bitmap is decoded on every miss instead of being cached, causing repeated resource decoding. Cache the decoded placeholder (e.g., store in memoryCache with a constant key) or lazily initialize a singleton reference to avoid redundant allocations.
Copilot uses AI. Check for mistakes.
2 participants
Implement network streaming for EPUB/PDF/CBZ, wire media notifications, add artwork fallbacks, and enable SettingsScreen error handling to address identified missing features and stubs.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores