feat: Introduce book detail screen, global player, smart rewind settings, and enhance content caching and player UI.#2
Conversation
…ngs, and enhance content caching and player UI.
📝 WalkthroughWalkthroughAdds offline-first local-first caching, network/server availability monitoring and sync, new repositories/DAOs/flows, smart-rewind and shake-to-reset features, extensive player/settings UI and theme/branding rename to "Kahani", plus CI/versioning and signing updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as UI / ViewModel
participant Repo as BookRepository
participant Cache as LocalCache/DAO
participant Remote as AudiobookshelfChannel
participant Net as NetworkService
Note over Net,Repo: NetworkService publishes network & server availability
Net->>Repo: isServerAvailable updates
User->>UI: request fetchBooks(libraryId,page,downloadedOnly)
UI->>Repo: fetchBooks(libraryId,page,downloadedOnly)
Repo->>Cache: query local cache (downloadedOnly?)
alt local hit
Cache-->>Repo: cached page
Repo-->>UI: return cached page
else local miss and downloadedOnly = false
Repo->>Remote: fetch page
Remote-->>Repo: results
Repo->>Cache: cacheBooks(results)
Repo-->>UI: return merged page
end
Note over Net,Repo: when server becomes available
Net->>Repo: availability true
Repo->>Remote: syncLibraryPage / push local progress
Remote-->>Repo: ack
Repo->>Cache: update local cache
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 9
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/src/main/kotlin/org/grakovne/lissen/playback/service/LissenDataSourceFactory.kt (1)
72-81: Silent failure when URI parsing fails.Returning
0on line 73 whenunapply()returnsnullwill silently signal end-of-content rather than an error, causing playback to fail without diagnostics. Additionally, the fallback on line 80 retains the originallissen://scheme URI, which neither underlying source can handle.Consider throwing an appropriate exception or logging an error to aid debugging:
🔎 Proposed fix
override fun open(dataSpec: DataSpec): Long { - val (itemId, fileId) = unapply(dataSpec.uri) ?: return 0 + val (itemId, fileId) = unapply(dataSpec.uri) + ?: throw IOException("Invalid lissen URI: ${dataSpec.uri}") val resolvedUri = mediaProvider .provideFileUri(itemId, fileId) .fold( onSuccess = { it }, - onFailure = { dataSpec.uri }, + onFailure = { throw IOException("Failed to resolve URI for itemId=$itemId, fileId=$fileId", it) }, )You'll need to add
import java.io.IOExceptionat the top of the file.app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/LocalCacheRepository.kt (1)
144-160: Orphaned documentation comment.The docstring at lines 144-155 describes playback position adjustment for
fetchBook, but it now precedescacheBooks. The docstring should either be moved tofetchBookat line 160 or removed if the described behavior is no longer implemented.🔎 Proposed fix
suspend fun fetchLatestUpdate(libraryId: String) = cachedBookRepository.fetchLatestUpdate(libraryId) - /** - * Fetches a detailed book item by its ID from the cached repository. - * If the book is not found in the cache, returns `null`. - * - * The method ensures that the book's playback position points to an available chapter: - * - If the current chapter is available, the cached book is returned as is. - * - If the current chapter is unavailable, the playback progress is adjusted to the first available chapter. - * - * @param bookId the unique identifier of the book to fetch. - * @return the detailed book item with updated playback progress if necessary, - * or `null` if the book is not found in the cache. - */ suspend fun cacheBooks(books: List<Book>) { cachedBookRepository.cacheBooks(books) } suspend fun fetchBook(bookId: String): DetailedItem? = cachedBookRepository.fetchBook(bookId)app/src/main/kotlin/org/grakovne/lissen/ui/navigation/AppNavHost.kt (1)
58-64: Use theROUTE_SETTINGSconstant on line 101 instead of hardcoding the route string.Line 58 correctly uses
$ROUTE_SETTINGS/cached_items, but line 101 hardcodes"settings_screen/cached_items". Both currently resolve to the same value, but hardcoding creates a maintainability issue: ifROUTE_SETTINGSis updated in the future, the hardcoded string on line 101 will be missed, causing a route mismatch. Use"$ROUTE_SETTINGS/cached_items"on line 101 for consistency.
🟡 Minor comments (13)
app/src/main/kotlin/org/grakovne/lissen/playback/service/LissenDataSourceFactory.kt-101-105 (1)
101-105: Returning 0 fromread()when no data source is open may cause issues.Returning
0whencurrentDataSourceisnullindicates zero bytes read but more may be available, which could cause callers to spin. Ifread()is called beforeopen(), this masks a programming error. Consider throwing an exception or returningC.RESULT_END_OF_INPUT(-1):🔎 Proposed fix
override fun read( buffer: ByteArray, offset: Int, length: Int, -): Int = currentDataSource?.read(buffer, offset, length) ?: 0 +): Int = currentDataSource?.read(buffer, offset, length) + ?: throw IllegalStateException("DataSource not opened").agent/rules/project-context.md-9-9 (1)
9-9: Fix: Use hyphenated "offline-first".The phrase should be hyphenated: "offline-first Android client app".
🔎 Proposed fix
-This is an offline first Android client app for the [audiobookshelf](https://github.com/advplyr/audiobookshelf) server. +This is an offline-first Android client app for the [audiobookshelf](https://github.com/advplyr/audiobookshelf) server..agent/rules/project-context.md-31-31 (1)
31-31: Fix typo: "cahce" should be "cache".🔎 Proposed fix
-- When the server becomes reachable, it should update the books list, as now all the books can be played from local cahce or online from the server. +- When the server becomes reachable, it should update the books list, as now all the books can be played from local cache or online from the server..agent/rules/project-context.md-32-32 (1)
32-32: Fix: Use hyphens for compound adjective "offline-available".The phrase "offline available ready to play books" needs hyphens for the compound adjectives.
🔎 Proposed fix
-- When the network is switched, the app should trigger checking whether the server is still reachable or not, if not reachable, it should update the UI to only show offline available ready to play books. +- When the network is switched, the app should trigger checking whether the server is still reachable or not; if not reachable, it should update the UI to only show offline-available, ready-to-play books..agent/rules/project-context.md-15-15 (1)
15-15: Fix multiple typos and style issues in network monitoring description.This line has several issues:
- "lan network" is redundant (LAN = Local Area Network)
- "etc" should be "etc." in American English
- "reachout" should be "reach out" (two words)
🔎 Proposed fixes
-- The app should watch the network changes, such as connecting to a new wifi network, or lan network, or disconnecting from a network and connecting to a new network, disconnecting from wifi and connecting to celular network etc, and try to ping or reachout the audiobook server to check whether the server is reachable. +- The app should watch the network changes, such as connecting to a new wifi network, or LAN, or disconnecting from a network and connecting to a new network, disconnecting from wifi and connecting to cellular network, etc., and try to ping or reach out to the audiobook server to check whether the server is reachable.app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/TimerComposable.kt-47-51 (1)
47-51: ReplaceSideEffectwithDisposableEffectto reset blur on dismiss.
WindowBlurEffect()usesSideEffectto setwindow.setBackgroundBlurRadius(30), but provides no cleanup logic. When the bottom sheet is dismissed and the composable leaves composition, the blur effect persists. ReplaceSideEffectwithDisposableEffectto reset the blur radius to0in the cleanup block.app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/SettingsScreen.kt-103-107 (1)
103-107: Hardcoded strings break i18n support.The title and description are hardcoded, while other navigation items in this file use
stringResource()(e.g., lines 110-111, 116-117). This breaks localization support.🔎 Suggested fix to use string resources
Add the following strings to your
strings.xml:<string name="playback_settings_title">Playback preferences</string> <string name="playback_settings_description">Player controls and sleep timer settings</string>Then update the code:
AdvancedSettingsNavigationItemComposable( - title = "Playback preferences", - description = "Player controls and sleep timer settings", + title = stringResource(R.string.playback_settings_title), + description = stringResource(R.string.playback_settings_description), onclick = { navController.showPlaybackSettings() }, )Committable suggestion skipped: line range outside the PR's diff.
app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/playback/PlaybackSettingsScreen.kt-52-52 (1)
52-52: Use stringResource for i18n compliance.The screen title and toggle item labels are hardcoded strings, which breaks internationalization support. These should be moved to string resources for consistency with other UI text in the application (Lines 103, 111 already use
stringResource).🔎 Suggested fix
Add to your strings.xml:
<string name="playback_preferences_title">Playback preferences</string> <string name="show_nav_buttons_title">Next/previous buttons</string> <string name="show_nav_buttons_description">Show next and previous track buttons in the player</string> <string name="shake_to_reset_timer_title">Shake to reset sleep timer</string> <string name="shake_to_reset_timer_description">Shake device to reset the sleep timer</string>Then update the code:
- text = "Playback preferences", + text = stringResource(R.string.playback_preferences_title), - title = "Next/previous buttons", - description = "Show next and previous track buttons in the player", + title = stringResource(R.string.show_nav_buttons_title), + description = stringResource(R.string.show_nav_buttons_description), - title = "Shake to reset sleep timer", - description = "Shake device to reset the sleep timer", + title = stringResource(R.string.shake_to_reset_timer_title), + description = stringResource(R.string.shake_to_reset_timer_description),Also applies to: 89-90, 96-97
app/src/main/kotlin/org/grakovne/lissen/common/NetworkService.kt-99-111 (1)
99-111: Socket may leak ifconnect()throws.If
socket.connect()throws an exception,socket.close()is never called. Useuse {}or a try-finally block to ensure the socket is closed.🔎 Proposed fix
try { val url = java.net.URL(hostUrl) val port = if (url.port == -1) url.defaultPort else url.port - val socket = java.net.Socket() val address = java.net.InetSocketAddress(url.host, port) - socket.connect(address, 2000) - socket.close() + java.net.Socket().use { socket -> + socket.connect(address, 2000) + } _isServerAvailable.emit(true) } catch (e: Exception) { Timber.e(e, "Server reachability check failed for $hostUrl") _isServerAvailable.emit(false) }app/src/main/kotlin/org/grakovne/lissen/playback/service/PlaybackService.kt-265-283 (1)
265-283: Edge case:lastUpdate = 0triggers unintended smart rewind.If
book.progress?.lastUpdateis0L(default/never set), the conditioncurrentTime - lastUpdate > thresholdwill always be true, causing smart rewind on every initial play. Consider treatinglastUpdate = 0as "never updated" and skipping the rewind in that case.🔎 Proposed fix
private fun calculateSmartRewindPosition(book: DetailedItem): Double = when (sharedPreferences.getSmartRewindEnabled()) { true -> { val lastUpdate = book.progress?.lastUpdate ?: 0L + if (lastUpdate == 0L) { + return book.progress?.currentTime ?: 0.0 + } + val currentTime = System.currentTimeMillis() val threshold = sharedPreferences.getSmartRewindThreshold().durationMillis val rewindDuration = sharedPreferences.getSmartRewindDuration().durationSeconds.toDouble() val currentPosition = book.progress?.currentTime ?: 0.0 if (currentTime - lastUpdate > threshold) { (currentPosition - rewindDuration).coerceAtLeast(0.0) } else { currentPosition } } false -> book.progress?.currentTime ?: 0.0 }app/src/main/kotlin/org/grakovne/lissen/content/AuthRepository.kt-47-93 (1)
47-93: Extract duplicated login logic to a shared helper function.Both
AuthRepository.onPostLoginandLissenMediaProvider.onPostLogincontain nearly identical implementations for persisting credentials and handling library preferences. Extract the common library-fetching and preference-saving logic to avoid maintenance issues and potential divergence. Additionally, replace the hardcoded"Default Library"string in both implementations with a constant.app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/api/CachedBookRepository.kt-64-95 (1)
64-95: MissinglibraryIdupdate when modifying existing books.When updating an existing book entity (lines 69-72), only
title,author, anddurationare updated. If a book'slibraryIdchanges, this won't be reflected. Additionally, consider batch operations to reduce N+1 query overhead.🔎 Proposed fix for libraryId
val entity = existing?.copy( title = book.title, author = book.author, duration = book.duration.toInt(), + libraryId = book.libraryId, ) ?: BookEntity(app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/GlobalPlayerBottomSheet.kt-154-166 (1)
154-166: Creating a dummyNavHostControlleris fragile and may cause runtime issues.The
NavHostControllercreated at line 157 is disconnected from the actual navigation graph and lifecycle. While the comment says it "won't be used for the click action," ifMiniPlayerComposableinternally calls navigation methods, it will fail silently or crash.Consider either:
- Refactoring
MiniPlayerComposableto makenavControlleroptional/nullable whenonContentClickis provided- Passing the real
navControllerifMiniPlayerComposableneeds it for other navigation
🧹 Nitpick comments (36)
app/src/main/kotlin/org/grakovne/lissen/ui/theme/Spacing.kt (1)
5-11: LGTM! Clean spacing system implementation.The spacing constants are well-defined and follow Compose best practices. The singleton object pattern is appropriate for theme constants.
Optional: Consider adding documentation and reviewing the spacing scale
Documentation: Adding KDoc comments would help developers understand when to use each spacing value:
+/** + * Standard spacing values for consistent layout across the app. + * Use these constants instead of hardcoded dp values. + */ object Spacing { + /** Extra small spacing - Use for tight component spacing */ val xs = 4.dp + /** Small spacing - Use for compact layouts */ val sm = 8.dp + /** Medium spacing - Use for standard component spacing */ val md = 16.dp + /** Large spacing - Use for section spacing */ val lg = 20.dp + /** Extra large spacing - Use for major layout divisions */ val xl = 32.dp }Spacing progression: The current scale (4, 8, 16, 20, 32) has an irregular progression from
mdonward. If this is intentional based on your design requirements, that's fine. Otherwise, consider a more geometric progression like 4, 8, 16, 24, 32 or 4, 8, 16, 32, 48 for consistency.app/src/main/kotlin/org/grakovne/lissen/ui/screens/library/composables/MiniPlayerComposable.kt (2)
140-142: Optimize the remember key for totalDuration calculation.The
remember(book)will recalculate the sum whenever any property ofbookchanges (including progress updates). Consider using more specific keys to avoid unnecessary recalculations when only the progress changes.🔎 Proposed optimization
-val totalDuration = remember(book) { book.chapters.sumOf { it.duration } } -val currentTime = book.progress?.currentTime ?: 0.0 -val progress = if (totalDuration > 0) (currentTime / totalDuration).toFloat() else 0f +val totalDuration = remember(book.id, book.chapters) { + book.chapters.sumOf { it.duration } +} +val progress = remember(totalDuration, book.progress?.currentTime) { + val currentTime = book.progress?.currentTime ?: 0.0 + if (totalDuration > 0) (currentTime / totalDuration).toFloat() else 0f +}
218-233: Simplify unnecessary nesting.The
Columnwrapping aRowthat contains a singleIconButtonis unnecessary. The button can be placed directly in the parentRow, reducing nesting complexity.🔎 Proposed simplification
- Column( - horizontalAlignment = Alignment.CenterHorizontally, - verticalArrangement = Arrangement.Center, - ) { - Row { - IconButton( - onClick = { withHaptic(view) { playerViewModel.togglePlayPause() } }, - ) { - Icon( - imageVector = if (isPlaying) Icons.Outlined.PauseCircleOutline else Icons.Outlined.PlayCircle, - contentDescription = if (isPlaying) "Pause" else "Play", - modifier = Modifier.size(34.dp), - ) - } - } - } + IconButton( + onClick = { withHaptic(view) { playerViewModel.togglePlayPause() } }, + ) { + Icon( + imageVector = if (isPlaying) Icons.Outlined.PauseCircleOutline else Icons.Outlined.PlayCircle, + contentDescription = if (isPlaying) "Pause" else "Play", + modifier = Modifier.size(34.dp), + ) + }app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/common/SeekButton.kt (2)
15-15: Remove unused import.The
scalemodifier is imported but never used in this file.🔎 Proposed fix
-import androidx.compose.ui.draw.scale
36-43: Simplify the conditional transformation.The
graphicsLayerblock can be streamlined by computingscaleXdirectly.🔎 Proposed refactor
modifier = Modifier .size(48.dp) - .graphicsLayer { - if (isForward) { - scaleX = -1f - } - }, + .graphicsLayer { scaleX = if (isForward) -1f else 1f },app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/TrackControlComposable.kt (1)
142-156: Consider disabling Previous button at first track.The Previous button is always enabled, but it should likely be disabled when
currentTrackIndex == 0for consistency with how the Next button is disabled at the last track.🔎 Proposed enhancement
if (showNavButtons) { IconButton( onClick = { withHaptic(view) { viewModel.previousTrack() } }, - enabled = true, + enabled = currentTrackIndex > 0, ) { Icon( imageVector = Icons.Rounded.SkipPrevious, contentDescription = "Previous Track", - tint = colorScheme.onBackground, + tint = + if (currentTrackIndex > 0) { + colorScheme.onBackground + } else { + colorScheme.onBackground.copy(alpha = 0.3f) + }, modifier = Modifier.size(36.dp), ) } }app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/PlaylistItemComposable.kt (1)
119-132: Consider dimming the offline icon when canPlay is false for visual consistency.While the current implementation is functionally correct (since cached items should always be playable), applying the same
canPlay-based dimming to the offline icon would ensure complete visual consistency across all UI elements.🔎 Proposed enhancement for visual consistency
tint = colorScheme.onBackground.copy( - alpha = if (isSelected) 0.6f else 0.4f, + alpha = when { + !canPlay -> 0.4f + isSelected -> 0.6f + else -> 0.4f + }, ),app/src/main/kotlin/org/grakovne/lissen/playback/service/PlaybackTimer.kt (1)
92-112: Review volume fade implementation for potential refinements.The volume fade feature is well-structured, but consider these potential improvements:
Hardcoded fade window (line 96): The 60-second fade is fixed. Consider making this configurable via preferences if users find it too long/short.
Short timer behavior (lines 105-106): The comment mentions handling short durations, but for timers < 60s, the fade will span the entire duration. This might be jarring for very short timers (e.g., 10s). Consider a minimum fade window or proportional fade.
Perceptual fade curve (line 111): The comment suggests "squared for smoother perception," but the implementation uses linear fade. Human perception of volume is logarithmic, so
progress * progressorsqrt(progress)might provide a more natural fade experience.Volume check optimization (line 100): The condition
if (exoPlayer.volume != 1f)could cause issues if volume is manually changed elsewhere. Consider always setting to1ffor consistency.🔎 Suggested refinements
1. Make fade window configurable:
private fun handleVolumeFade( remainingSeconds: Long, totalSeconds: Double, ) { val fadeWindow = minOf(60.0, totalSeconds * 0.5) // Fade over last 60s or half duration, whichever is smaller if (remainingSeconds > fadeWindow) { exoPlayer.volume = 1f return } val progress = (remainingSeconds / fadeWindow).toFloat().coerceIn(0f, 1f) exoPlayer.volume = progress }2. Use perceptual fade curve:
// For more natural perception, use squared fade val progress = (remainingSeconds / fadeWindow).toFloat().coerceIn(0f, 1f) exoPlayer.volume = progress * progress // Squared for smoother perceptual fadeapp/src/main/kotlin/org/grakovne/lissen/playback/service/ShakeDetector.kt (2)
31-34: Remove unused threshold configuration fields.
thresholdanddebounceTimeare declared but never referenced. OnlytimeThresholdandSHAKE_THRESHOLDare actually used for shake detection.🔎 Proposed cleanup
- // Configurable Thresholds - private val threshold = 12.0f // Accelerometer sensitivity private val timeThreshold = 1000L // Minimum time between shakes in ms - private val debounceTime = 500L // Time buffer to avoid duplicate triggers
65-77: Remove dead gForce calculation.The
gX,gY,gZ, andgForcevariables are computed but never used. The comments indicate this was an alternative approach that was abandoned in favor of the "classic implementation" on line 78.🔎 Proposed cleanup
- // Simple shake detection logic based on g-force change - // We're looking for significant acceleration in any direction - val gX = x / SensorManager.GRAVITY_EARTH - val gY = y / SensorManager.GRAVITY_EARTH - val gZ = z / SensorManager.GRAVITY_EARTH - - // Calculate g-force - val gForce = Math.sqrt((gX * gX + gY * gY + gZ * gZ).toDouble()).toFloat() - - // 1.0 is minimal gravity. Anything significantly above suggests movement. - // Using raw values might be simpler if gForce logic is too sensitive or not enough. - // Let's stick to the classic implementation: - val speed = Math.abs(x + y + z - lastX - lastY - lastZ) / diffTime * 10000app/src/main/kotlin/org/grakovne/lissen/channel/audiobookshelf/library/converter/BookResponseConverter.kt (1)
11-13: Unused import:DateTimeFormatter.
DateTimeFormatteris imported but not used sinceZonedDateTime.parse()andLocalDate.parse()use ISO format by default.Proposed fix
import java.time.LocalDate import java.time.ZonedDateTime -import java.time.format.DateTimeFormatterapp/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/DownloadsComposable.kt (1)
85-99: Consider adding user feedback for offline state.The download options are visually faded and clicks are ignored when offline, but users receive no explicit feedback. Consider adding a brief toast or snackbar when tapped while offline to clarify why the action is unavailable.
Example enhancement
.clickable { if (isOnline) { onRequestedDownload(item) onDismissRequest() + } else { + // Show a toast or snackbar explaining offline state } },app/src/main/kotlin/org/grakovne/lissen/playback/MediaRepository.kt (1)
281-291:ToneGeneratorcleanup could fail if handler is cleared.The
ToneGeneratoris released viaHandler.postDelayed, but if the handler's callbacks are cleared (viaremoveCallbacksAndMessages) before 200ms elapse, theToneGeneratorwon't be released. Additionally, volume 100 is quite loud and may be jarring.Safer cleanup approach
private fun playResetSound() { try { - val toneGen = ToneGenerator(AudioManager.STREAM_MUSIC, 100) - toneGen.startTone(ToneGenerator.TONE_PROP_BEEP) - Handler(Looper.getMainLooper()).postDelayed({ - toneGen.release() - }, 200) + val toneGen = ToneGenerator(AudioManager.STREAM_NOTIFICATION, 50) + toneGen.startTone(ToneGenerator.TONE_PROP_BEEP, 150) + // ToneGenerator auto-releases after duration when using the overload with durationMs + toneGen.release() } catch (e: Exception) { Timber.e(e, "Failed to play reset sound") } }Note: Using
STREAM_NOTIFICATIONand lower volume (50) provides a less intrusive feedback. ThestartTone(int, int)overload accepts duration in ms.app/src/main/kotlin/org/grakovne/lissen/ui/screens/details/BookDetailScreen.kt (3)
119-125: Use imported Timber instead of fully qualified name.Import
timber.log.Timberat the top (or use the existing import pattern) rather than using the fully qualified name inline.🔎 Proposed fix
-import kotlinx.coroutines.launch +import timber.log.TimberThen update usage:
- timber.log.Timber.d("BookDetailScreen: Launched with bookId $bookId") + Timber.d("BookDetailScreen: Launched with bookId $bookId")- timber.log.Timber.d("BookDetailScreen: bookDetail changed to ${bookDetail?.id}, title: ${bookDetail?.title}") + Timber.d("BookDetailScreen: bookDetail changed to ${bookDetail?.id}, title: ${bookDetail?.title}")
484-505: Use importedmutableStateOffor consistency.Lines 486-487 use the fully qualified
androidx.compose.runtime.mutableStateOfwhilemutableStateOfis already used via delegation elsewhere. Use the shorter form for consistency.🔎 Proposed fix
@Composable fun ExpandableDescription(description: String) { - var expanded by remember { androidx.compose.runtime.mutableStateOf(false) } - var overflowDetected by remember { androidx.compose.runtime.mutableStateOf(false) } + var expanded by remember { mutableStateOf(false) } + var overflowDetected by remember { mutableStateOf(false) }
467-482: Remove the unusedDetailRowcomposable unless it's reserved for future use.The function is defined but never called anywhere in the codebase.
app/src/main/kotlin/org/grakovne/lissen/content/AuthRepository.kt (1)
23-45: Consider logging and lambda simplification.
- Line 28 logs the username, which could be PII. Consider redacting or removing in production builds.
- Line 43
onFailure = { onFailure(it) }can be simplified toonFailure = onFailure.🔎 Proposed simplification
return provideAuthService() .startOAuth( host = host, onSuccess = onSuccess, - onFailure = { onFailure(it) }, + onFailure = onFailure, )app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/composable/PlaybackSmartRewindSettingsComposable.kt (2)
67-77: Use.entriesinstead of deprecated.values()for enum lookup.
SmartRewindInactivityThreshold.values()is deprecated in Kotlin. Since you're already using.entrieson line 67 for mapping, use it consistently for the lookup as well.🔎 Proposed fix
onItemSelected = { item -> - SmartRewindInactivityThreshold.values().find { it.name == item.id }?.let { + SmartRewindInactivityThreshold.entries.find { it.name == item.id }?.let { viewModel.preferSmartRewindThreshold(it) } thresholdExpanded = false },
86-96: Use.entriesinstead of deprecated.values()for enum lookup.Same issue as the threshold selection - use
.entriesfor consistency and to avoid deprecated API.🔎 Proposed fix
onItemSelected = { item -> - SmartRewindDuration.values().find { it.name == item.id }?.let { + SmartRewindDuration.entries.find { it.name == item.id }?.let { viewModel.preferSmartRewindDuration(it) } durationExpanded = false },app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/composable/CommonSettingsItemComposable.kt (1)
60-68: Simplify title/spacer rendering by extracting common spacer.Both branches add a
Spacer(modifier = Modifier.height(8.dp)), so it can be placed after the conditional block.🔎 Proposed fix
if (title != null) { Text( text = title, style = MaterialTheme.typography.bodyLarge, ) - Spacer(modifier = Modifier.height(8.dp)) - } else { - Spacer(modifier = Modifier.height(8.dp)) } + Spacer(modifier = Modifier.height(8.dp))app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/api/RecentRequestBuilder.kt (1)
35-45: Consider parameterizing the LIMIT clause for consistency.While
limitis anIntand safe from SQL injection, using parameterized queries throughout maintains consistency with howlibraryIdis handled and follows best practices.🔎 Proposed fix
val sql = """ SELECT DISTINCT detailed_books.* FROM detailed_books INNER JOIN media_progress ON detailed_books.id = media_progress.bookId $joinClause WHERE $whereClause AND media_progress.currentTime > 1.0 AND media_progress.isFinished = 0 ORDER BY media_progress.lastUpdate DESC - LIMIT $limit + LIMIT ? """.trimIndent() + args.add(limit) return SimpleSQLiteQuery(sql, args.toTypedArray()) }app/src/main/kotlin/org/grakovne/lissen/ui/navigation/AppNavHost.kt (2)
92-95: Consider adding explicit imports instead of using fully-qualified names.Several composables are referenced via FQN (
org.grakovne.lissen.ui.screens.player.GlobalPlayerBottomSheet,org.grakovne.lissen.ui.screens.details.BookDetailScreen,org.grakovne.lissen.ui.screens.settings.playback.SmartRewindSettingsScreen), whilePlaybackSettingsScreenis properly imported. Adding imports improves readability and consistency.Also applies to: 174-179, 298-304
108-114: Extract repeatedonBacklambda to reduce duplication.The same
onBackpattern is repeated across 9+ composables. Consider extracting a helper function.🔎 Proposed refactor
// Add at the top of the AppNavHost function, after navController parameter val onBack: () -> Unit = { if (navController.previousBackStackEntry != null) { navController.popBackStack() } } // Then use it in composables: CachedItemsSettingsScreen( onBack = onBack, imageLoader = imageLoader, )Also applies to: 125-130, 200-206, 217-222, 233-238, 250-255, 266-272, 283-288, 299-304
app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/NavigationBarComposable.kt (1)
88-88: Add missing imports instead of using fully-qualified names.
LocalContext.current,CircleShape, andAlignment.CenterHorizontallyare referenced via FQN. Add these imports for consistency with the rest of the file.🔎 Proposed imports
+import androidx.compose.foundation.shape.CircleShape +import androidx.compose.ui.platform.LocalContext import androidx.compose.ui.AlignmentAlso applies to: 97-97, 267-267
app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/GlobalPlayerBottomSheet.kt (1)
238-249: Simplify redundant null check pattern.Line 238 checks
playingBook != null, then line 239 usesplayingBook?.let. The innerletis redundant since null was already excluded.🔎 Proposed simplification
if (playingBook != null && isPlaybackReady) { - playingBook?.let { - NavigationBarComposable( - book = it, - ... - ) - } + NavigationBarComposable( + book = playingBook, + playerViewModel = playerViewModel, + contentCachingModelView = cachingModelView, + settingsViewModel = settingsViewModel, + navController = navController, + libraryType = libraryViewModel.fetchPreferredLibraryType(), + ) }app/src/main/kotlin/org/grakovne/lissen/viewmodel/LibraryViewModel.kt (1)
99-103: Potential redundant refresh ondownloadedOnlyFlowcollection.Every emission triggers
refreshRecentListening(), butdownloadedOnlyFlowalready usescombinewhich may emit even if the resulting boolean doesn't change. Consider addingdistinctUntilChanged()to avoid redundant refreshes.🔎 Proposed fix
init { viewModelScope.launch { - downloadedOnlyFlow.collect { refreshRecentListening() } + downloadedOnlyFlow + .distinctUntilChanged() + .collect { refreshRecentListening() } }app/src/main/kotlin/org/grakovne/lissen/content/LissenMediaProvider.kt (2)
364-377: Sequential book fetches insyncRepositoriesmay be slow with many items.The loop fetches each book one-by-one when remote is newer. For large libraries, this could be very slow. Consider batching or parallel fetches with controlled concurrency.
🔎 Example parallel approach
suspend fun syncRepositories() { // ... existing setup code ... val idsToSync = allIds.filter { id -> val remoteTime = remoteMap[id]?.listenedLastUpdate ?: 0L val localTime = localMap[id]?.listenedLastUpdate ?: 0L remoteTime > localTime } // Fetch in parallel with limited concurrency idsToSync.chunked(5).forEach { batch -> batch.map { id -> async { providePreferredChannel().fetchBook(id).foldAsync( onSuccess = { localCacheRepository.cacheBookMetadata(it) }, onFailure = {}, ) } }.awaitAll() } }
266-278: Minor inconsistency:fetchBookFlowusesnetworkStatusflow butmakeAvailableIfOnlineusesisNetworkAvailable().The reactive
fetchBookFlowcombines withnetworkService.networkStatus, whilemakeAvailableIfOnlineat line 252 checksnetworkService.isNetworkAvailable(). This is functionally similar but the different APIs may diverge in behavior.app/src/main/kotlin/org/grakovne/lissen/persistence/preferences/LissenSharedPreferences.kt (1)
479-489: Exception is swallowed without logging; consider adding debug logging.The static analysis tool flagged that the caught exception at line 486 is swallowed silently. While returning a default value is correct behavior, logging the exception would aid debugging when unexpected preference values occur.
Based on static analysis hint.
🔎 Proposed fix
private inline fun <reified T : Enum<T>> safeEnumValueOf( value: String?, default: T, ): T { if (value == null) return default return try { enumValueOf<T>(value) } catch (e: Exception) { + timber.log.Timber.w(e, "Failed to parse enum value '$value' for ${T::class.simpleName}, using default: $default") default } }app/src/main/kotlin/org/grakovne/lissen/content/BookRepository.kt (7)
64-64: Local sync result is ignored.
localCacheRepository.syncProgress()return value is discarded. If local caching fails, there's no indication to the caller. Consider logging the failure or returning a combined result if local persistence is critical for offline support.
99-101: Local search ignoreslimitparameter.The
limitparameter is passed to the remote channel but not tolocalCacheRepository.searchBooks(). This could return more results than expected when serving from cache.🔎 Proposed fix
-val localResult = localCacheRepository.searchBooks(libraryId = libraryId, query = query) +val localResult = localCacheRepository.searchBooks(libraryId = libraryId, query = query, limit = limit)Alternatively, if the local repository doesn't support limiting, apply the limit after fetching:
val localResult = localCacheRepository.searchBooks(libraryId = libraryId, query = query) .map { it.take(limit) }
138-150: Last page may trigger unnecessary remote fetch.When fetching the final page of a library,
localItems.size < pageSizewill be true (e.g., 3 items on a 20-item page). This causes an unnecessary remote call even when local cache is complete for that page. Consider tracking total item count or implementing a "complete" flag for cached pages.
214-216: Recent books may become stale when online.When online, if local cache has any results, the method returns local data without checking the remote server. Unlike
fetchBookwhich validates cache completeness, this could show outdated "recently listened" data. Consider adding a TTL check or background refresh.
223-230: Offline status captured once, not reactively.
isOfflineis computed when the Flow is created, so thedownloadedOnlyfilter won't update if network status changes. Consider combining withnetworkService.isServerAvailableFlow to reactively update filtering:🔎 Proposed reactive approach
fun fetchRecentListenedBooksFlow(libraryId: String): Flow<List<RecentBook>> = networkService.isServerAvailable.flatMapLatest { isAvailable -> val isOffline = !isAvailable || preferences.isForceCache() localCacheRepository.fetchRecentListenedBooksFlow( libraryId = libraryId, downloadedOnly = isOffline, ) }
256-269: Duplicate logic withLissenMediaProvider.makeAvailableIfOnline.This method is identical to the one in
LissenMediaProvider.kt(lines 250-263 in the relevant snippets). Consider extracting to a shared utility or extension function to maintain DRY principles.
321-377: Duplicate sync logic withLissenMediaProvider.Both
syncFromLocalProgressoverloads mirror the implementations inLissenMediaProvider.kt(lines 385-449 in the relevant snippets). Consider extracting these synchronization strategies to a shared module or service to avoid maintaining duplicate logic.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (72)
.agent/rules/project-context.md.gitignoreapp/src/main/kotlin/org/grakovne/lissen/channel/audiobookshelf/common/converter/LibraryPageResponseConverter.ktapp/src/main/kotlin/org/grakovne/lissen/channel/audiobookshelf/library/LibraryAudiobookshelfChannel.ktapp/src/main/kotlin/org/grakovne/lissen/channel/audiobookshelf/library/converter/BookResponseConverter.ktapp/src/main/kotlin/org/grakovne/lissen/channel/audiobookshelf/library/converter/LibrarySearchItemsConverter.ktapp/src/main/kotlin/org/grakovne/lissen/channel/audiobookshelf/podcast/PodcastAudiobookshelfChannel.ktapp/src/main/kotlin/org/grakovne/lissen/channel/audiobookshelf/podcast/converter/PodcastPageResponseConverter.ktapp/src/main/kotlin/org/grakovne/lissen/channel/audiobookshelf/podcast/converter/PodcastSearchItemsConverter.ktapp/src/main/kotlin/org/grakovne/lissen/common/NetworkService.ktapp/src/main/kotlin/org/grakovne/lissen/content/AuthRepository.ktapp/src/main/kotlin/org/grakovne/lissen/content/BookRepository.ktapp/src/main/kotlin/org/grakovne/lissen/content/LissenMediaProvider.ktapp/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/ContentCachingManager.ktapp/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/LocalCacheRepository.ktapp/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/api/CachedBookRepository.ktapp/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/api/FetchRequestBuilder.ktapp/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/api/RecentRequestBuilder.ktapp/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/converter/CachedBookEntityConverter.ktapp/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/converter/CachedBookEntityRecentConverter.ktapp/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/dao/CachedBookDao.ktapp/src/main/kotlin/org/grakovne/lissen/content/cache/temporary/CachedCoverProvider.ktapp/src/main/kotlin/org/grakovne/lissen/persistence/preferences/LissenSharedPreferences.ktapp/src/main/kotlin/org/grakovne/lissen/playback/MediaRepository.ktapp/src/main/kotlin/org/grakovne/lissen/playback/service/LissenDataSourceFactory.ktapp/src/main/kotlin/org/grakovne/lissen/playback/service/PlaybackService.ktapp/src/main/kotlin/org/grakovne/lissen/playback/service/PlaybackSynchronizationService.ktapp/src/main/kotlin/org/grakovne/lissen/playback/service/PlaybackTimer.ktapp/src/main/kotlin/org/grakovne/lissen/playback/service/ShakeDetector.ktapp/src/main/kotlin/org/grakovne/lissen/ui/components/AsyncShimmeringImage.ktapp/src/main/kotlin/org/grakovne/lissen/ui/components/DownloadProgressIcon.ktapp/src/main/kotlin/org/grakovne/lissen/ui/effects/WindowBlurEffect.ktapp/src/main/kotlin/org/grakovne/lissen/ui/navigation/AppNavHost.ktapp/src/main/kotlin/org/grakovne/lissen/ui/navigation/AppNavigationService.ktapp/src/main/kotlin/org/grakovne/lissen/ui/screens/details/BookDetailScreen.ktapp/src/main/kotlin/org/grakovne/lissen/ui/screens/library/LibraryScreen.ktapp/src/main/kotlin/org/grakovne/lissen/ui/screens/library/composables/MiniPlayerComposable.ktapp/src/main/kotlin/org/grakovne/lissen/ui/screens/library/composables/fallback/LibraryFallbackComposable.ktapp/src/main/kotlin/org/grakovne/lissen/ui/screens/library/composables/placeholder/LibraryPlaceholderComposable.ktapp/src/main/kotlin/org/grakovne/lissen/ui/screens/library/composables/placeholder/RecentBooksPlaceholderComposable.ktapp/src/main/kotlin/org/grakovne/lissen/ui/screens/library/paging/LibraryDefaultPagingSource.ktapp/src/main/kotlin/org/grakovne/lissen/ui/screens/library/paging/LibrarySearchPagingSource.ktapp/src/main/kotlin/org/grakovne/lissen/ui/screens/player/GlobalPlayerBottomSheet.ktapp/src/main/kotlin/org/grakovne/lissen/ui/screens/player/PlayerScreen.ktapp/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/DownloadsComposable.ktapp/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/NavigationBarComposable.ktapp/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/PlaybackSpeedComposable.ktapp/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/PlayingQueueComposable.ktapp/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/PlaylistItemComposable.ktapp/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/TimerComposable.ktapp/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/TrackControlComposable.ktapp/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/TrackDetailsComposable.ktapp/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/common/SeekButton.ktapp/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/placeholder/NavigationBarPlaceholderComposable.ktapp/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/placeholder/TrackDetailsPlaceholderComposable.ktapp/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/SettingsScreen.ktapp/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/advanced/AdvancedSettingsComposable.ktapp/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/composable/CommonSettingsItemComposable.ktapp/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/composable/PlaybackSmartRewindSettingsComposable.ktapp/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/playback/PlaybackSettingsScreen.ktapp/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/playback/SmartRewindSettingsScreen.ktapp/src/main/kotlin/org/grakovne/lissen/ui/theme/Color.ktapp/src/main/kotlin/org/grakovne/lissen/ui/theme/Spacing.ktapp/src/main/kotlin/org/grakovne/lissen/ui/theme/Theme.ktapp/src/main/kotlin/org/grakovne/lissen/viewmodel/CachingModelView.ktapp/src/main/kotlin/org/grakovne/lissen/viewmodel/LibraryViewModel.ktapp/src/main/kotlin/org/grakovne/lissen/viewmodel/PlayerViewModel.ktapp/src/main/kotlin/org/grakovne/lissen/viewmodel/SettingsViewModel.ktapp/src/main/res/values/strings.xmllib/src/main/kotlin/org/grakovne/lissen/lib/domain/Book.ktlib/src/main/kotlin/org/grakovne/lissen/lib/domain/SeekTimeOption.ktlib/src/main/kotlin/org/grakovne/lissen/lib/domain/SmartRewindOptions.kt
💤 Files with no reviewable changes (2)
- app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/advanced/AdvancedSettingsComposable.kt
- app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/placeholder/NavigationBarPlaceholderComposable.kt
🧰 Additional context used
🧬 Code graph analysis (18)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/PlaybackSpeedComposable.kt (1)
app/src/main/kotlin/org/grakovne/lissen/ui/effects/WindowBlurEffect.kt (1)
WindowBlurEffect(10-19)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/PlayerScreen.kt (1)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/NavigationBarComposable.kt (1)
NavigationBarComposable(61-257)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/DownloadsComposable.kt (1)
app/src/main/kotlin/org/grakovne/lissen/ui/effects/WindowBlurEffect.kt (1)
WindowBlurEffect(10-19)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/playback/PlaybackSettingsScreen.kt (3)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/composable/SettingsToggleItem.kt (1)
SettingsToggleItem(19-76)app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/advanced/AdvancedSettingsNavigationItemComposable.kt (1)
AdvancedSettingsNavigationItemComposable(22-61)app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/composable/PlaybackVolumeBoostSettingsComposable.kt (1)
PlaybackVolumeBoostSettingsComposable(27-76)
app/src/main/kotlin/org/grakovne/lissen/playback/service/LissenDataSourceFactory.kt (1)
app/src/main/kotlin/org/grakovne/lissen/playback/service/LissenMediaSchemeConverter.kt (1)
unapply(16-26)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/composable/PlaybackSmartRewindSettingsComposable.kt (2)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/composable/SettingsToggleItem.kt (1)
SettingsToggleItem(19-76)app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/composable/CommonSettingsItemComposable.kt (1)
CommonSettingsItemComposable(33-113)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/playback/SmartRewindSettingsScreen.kt (1)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/composable/PlaybackSmartRewindSettingsComposable.kt (1)
PlaybackSmartRewindSettingsComposable(27-99)
app/src/main/kotlin/org/grakovne/lissen/ui/navigation/AppNavHost.kt (3)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/advanced/cache/CachedItemsSettingsScreen.kt (1)
CachedItemsSettingsScreen(80-174)app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/advanced/cache/CacheSettingsScreen.kt (1)
CacheSettingsScreen(36-109)app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/playback/PlaybackSettingsScreen.kt (1)
PlaybackSettingsScreen(37-119)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/TrackControlComposable.kt (2)
app/src/main/kotlin/org/grakovne/lissen/common/HapticAction.kt (1)
withHaptic(6-12)app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/common/SeekButton.kt (1)
SeekButton(21-55)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/SettingsScreen.kt (1)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/advanced/AdvancedSettingsNavigationItemComposable.kt (1)
AdvancedSettingsNavigationItemComposable(22-61)
app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/api/RecentRequestBuilder.kt (2)
app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/converter/CachedBookEntityConverter.kt (1)
apply(16-39)app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/converter/CachedBookEntityRecentConverter.kt (1)
apply(12-28)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/details/BookDetailScreen.kt (5)
app/src/main/kotlin/org/grakovne/lissen/ui/components/DownloadProgressIcon.kt (1)
DownloadProgressIcon(21-46)app/src/main/kotlin/org/grakovne/lissen/ui/components/AsyncShimmeringImage.kt (1)
AsyncShimmeringImage(21-65)app/src/main/kotlin/org/grakovne/lissen/common/HapticAction.kt (1)
withHaptic(6-12)app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/PlaylistItemComposable.kt (1)
PlaylistItemComposable(36-148)app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/DownloadsComposable.kt (1)
DownloadsComposable(35-176)
app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/LocalCacheRepository.kt (1)
app/src/main/kotlin/org/grakovne/lissen/content/cache/common/FindRelatedFiles.kt (1)
findRelatedFiles(6-27)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/TimerComposable.kt (1)
app/src/main/kotlin/org/grakovne/lissen/ui/effects/WindowBlurEffect.kt (1)
WindowBlurEffect(10-19)
app/src/main/kotlin/org/grakovne/lissen/content/AuthRepository.kt (1)
app/src/main/kotlin/org/grakovne/lissen/content/LissenMediaProvider.kt (2)
provideAuthService(456-456)onPostLogin(304-348)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/library/composables/MiniPlayerComposable.kt (2)
app/src/main/kotlin/org/grakovne/lissen/ui/components/AsyncShimmeringImage.kt (1)
AsyncShimmeringImage(21-65)app/src/main/kotlin/org/grakovne/lissen/common/HapticAction.kt (1)
withHaptic(6-12)
app/src/main/kotlin/org/grakovne/lissen/content/BookRepository.kt (1)
app/src/main/kotlin/org/grakovne/lissen/content/LissenMediaProvider.kt (4)
providePreferredChannel(458-458)syncFromLocalProgress(386-424)syncFromLocalProgress(426-450)makeAvailableIfOnline(251-264)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/composable/CommonSettingsItemComposable.kt (1)
app/src/main/kotlin/org/grakovne/lissen/ui/effects/WindowBlurEffect.kt (1)
WindowBlurEffect(10-19)
🪛 detekt (1.23.8)
app/src/main/kotlin/org/grakovne/lissen/persistence/preferences/LissenSharedPreferences.kt
[warning] 486-486: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🪛 LanguageTool
.agent/rules/project-context.md
[grammar] ~9-~9: Use a hyphen to join words.
Context: ...es ## About Project This is an offline first Android client app for the [audiob...
(QB_NEW_EN_HYPHEN)
[style] ~15-~15: This phrase is redundant (‘N’ stands for ‘network’). Use simply “lan”.
Context: ...as connecting to a new wifi network, or lan network, or disconnecting from a network and co...
(ACRONYM_TAUTOLOGY)
[style] ~15-~15: In American English, abbreviations like “etc.” require a period.
Context: ... wifi and connecting to celular network etc, and try to ping or reachout the audiob...
(ETC_PERIOD)
[grammar] ~15-~15: Ensure spelling is correct
Context: ...celular network etc, and try to ping or reachout the audiobook server to check whether t...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~18-~18: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...hould be given priority for playback. - If the offline track is deleted / cleared ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[grammar] ~31-~31: Ensure spelling is correct
Context: ... all the books can be played from local cahce or online from the server. - When the n...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~32-~32: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ocal cahce or online from the server. - When the network is switched, the app should...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[grammar] ~32-~32: Use a hyphen to join words.
Context: ...hould update the UI to only show offline available ready to play books. ## Gener...
(QB_NEW_EN_HYPHEN)
[grammar] ~32-~32: Use a hyphen to join words.
Context: ... UI to only show offline available ready to play books. ## General Guidelines an...
(QB_NEW_EN_HYPHEN)
[grammar] ~32-~32: Use a hyphen to join words.
Context: ... to only show offline available ready to play books. ## General Guidelines and S...
(QB_NEW_EN_HYPHEN)
app/src/main/kotlin/org/grakovne/lissen/content/LissenMediaProvider.kt
Outdated
Show resolved
Hide resolved
| onTickSeconds = { seconds -> | ||
| broadcastRemaining(seconds) | ||
| handleVolumeFade(seconds, delayInSeconds) | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if there are other places that modify exoPlayer.volume
rg -nP --type=kt 'exoPlayer\.volume\s*=' -C3Repository: SurjitSahoo/lissen-up
Length of output: 91
🏁 Script executed:
# Check for volume-related user interactions or settings
rg -nP --type=kt 'volume' -g '!PlaybackTimer.kt' | head -50Repository: SurjitSahoo/lissen-up
Length of output: 91
🏁 Script executed:
# Find and examine the PlaybackTimer.kt file to understand the full implementation
fd -t f 'PlaybackTimer.kt' --exec wc -l {} \;Repository: SurjitSahoo/lissen-up
Length of output: 144
🏁 Script executed:
# Read the PlaybackTimer.kt file
cat -n ./app/src/main/kotlin/org/grakovne/lissen/playback/service/PlaybackTimer.ktRepository: SurjitSahoo/lissen-up
Length of output: 4156
🏁 Script executed:
# Check for exoPlayer.volume modifications - corrected search
rg 'exoPlayer\.volume' --type-add='kt:*.kt' -C3Repository: SurjitSahoo/lissen-up
Length of output: 2754
🏁 Script executed:
# Check for volume-related code in other files
rg 'exoPlayer\.volume' --type-add='kt:*.kt'Repository: SurjitSahoo/lissen-up
Length of output: 508
Store and restore original volume instead of resetting to hardcoded 1f.
The volume fade implementation has three issues:
-
Volume fade overrides user input:
handleVolumeFade()is called every 500ms and continuously modifiesexoPlayer.volume, so any manual user adjustment during countdown will be immediately overwritten on the next tick. -
Original volume not preserved:
startTimer()(line 46) setsexoPlayer.volume = 1fwithout storing the previous value, andstopTimer()(line 87) always resets to1f. If a user had volume at 0.5f before starting the timer, it gets reset to 1f instead of restored. -
No safeguard for timer cancellation: Mid-fade cancellation resets to
1frather than the pre-timer volume.
Store the original volume in a property before modifying it, and restore it in stopTimer(). Consider pausing fade updates during manual user adjustments if the platform provides volume change callbacks.
🤖 Prompt for AI Agents
In app/src/main/kotlin/org/grakovne/lissen/playback/service/PlaybackTimer.kt
around lines 57-60 (see related startTimer at ~46 and stopTimer at ~87): the
timer currently forces exoPlayer.volume to 1f and repeatedly overwrites user
adjustments during fade; capture and preserve the player's pre-timer volume in a
private property (e.g. originalVolume) before changing it, compute fade steps
relative to that originalVolume, set the player volume from the fade only when
the player’s current volume still matches the last fade value (or within a small
epsilon) to avoid clobbering manual changes, and on stopTimer restore
originalVolume (and clear the saved value); if platform supports a volume change
listener use it to detect manual changes and suspend fade updates while manual
adjustments occur.
app/src/main/kotlin/org/grakovne/lissen/ui/screens/details/BookDetailScreen.kt
Outdated
Show resolved
Hide resolved
app/src/main/kotlin/org/grakovne/lissen/ui/screens/details/BookDetailScreen.kt
Outdated
Show resolved
Hide resolved
app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/GlobalPlayerBottomSheet.kt
Outdated
Show resolved
Hide resolved
| navigationIcon = { | ||
| IconButton(onClick = { onBack() }) { | ||
| Icon( | ||
| imageVector = Icons.AutoMirrored.Outlined.ArrowBack, | ||
| contentDescription = "Back", | ||
| tint = MaterialTheme.colorScheme.onSurface, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract hardcoded string to resources.
The content description "Back" is hardcoded. Extract it to strings.xml for proper localization and consistency with the rest of the codebase.
🔎 Proposed fix
Add to strings.xml:
<string name="back_content_description">Back</string>Then update the code:
navigationIcon = {
IconButton(onClick = { onBack() }) {
Icon(
imageVector = Icons.AutoMirrored.Outlined.ArrowBack,
- contentDescription = "Back",
+ contentDescription = stringResource(R.string.back_content_description),
tint = MaterialTheme.colorScheme.onSurface,
)
}
},📝 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.
| navigationIcon = { | |
| IconButton(onClick = { onBack() }) { | |
| Icon( | |
| imageVector = Icons.AutoMirrored.Outlined.ArrowBack, | |
| contentDescription = "Back", | |
| tint = MaterialTheme.colorScheme.onSurface, | |
| ) | |
| } | |
| navigationIcon = { | |
| IconButton(onClick = { onBack() }) { | |
| Icon( | |
| imageVector = Icons.AutoMirrored.Outlined.ArrowBack, | |
| contentDescription = stringResource(R.string.back_content_description), | |
| tint = MaterialTheme.colorScheme.onSurface, | |
| ) | |
| } | |
| }, |
🤖 Prompt for AI Agents
In
app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/playback/SmartRewindSettingsScreen.kt
around lines 46 to 53, the contentDescription "Back" is hardcoded; extract it to
resources by adding a new entry in res/values/strings.xml (e.g. <string
name="back_content_description">Back</string>) and replace the hardcoded literal
with a string resource lookup using
stringResource(R.string.back_content_description) for the Icon's
contentDescription.
There was a problem hiding this comment.
Actionable comments posted: 13
♻️ Duplicate comments (2)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/details/BookDetailScreen.kt (2)
248-258: Play button logic may not handle switching books correctly.The current logic has issues:
- If this book is playing → toggles (pauses) ✓
- If nothing is playing → toggles (plays) ✓
- If a different book is playing →
isPlayingis true, so the innerif (!isPlaying)is false, and nothing happensThis means users cannot switch playback to a different book by clicking the play button.
🔎 Proposed fix
onClick = { withHaptic(view) { if (playingBook?.id == bookId && isPlaying) { playerViewModel.togglePlayPause() } else { - if (!isPlaying) { - playerViewModel.togglePlayPause() - } + // Switch to this book and start/resume playback + if (playingBook?.id != bookId) { + playerViewModel.preparePlayback(bookId) + } + if (!isPlaying) { + playerViewModel.togglePlayPause() + } } } },
454-461:currentPositionmay still reference the wrong book's playback position.While the callback now correctly uses
bookDetailinstead ofplayingBook, line 458 still usesplayerViewModel.totalPosition.valuewhich represents the currently playing book's position. If the user is viewing a book that isn't playing, this passes an incorrect position.🔎 Proposed fix
onRequestedDownload = { option -> bookDetail?.let { cachingModelView.cache( mediaItem = it, - currentPosition = playerViewModel.totalPosition.value ?: 0.0, + currentPosition = if (playingBook?.id == bookId) { + playerViewModel.totalPosition.value ?: 0.0 + } else { + it.progress?.currentTime ?: 0.0 + }, option = option, ) } },
🧹 Nitpick comments (15)
.agent/rules/project-context.md (1)
18-18: Improve clarity of complex requirement.Line 18 is dense and difficult to parse. Consider breaking it into smaller, more focused bullet points or reformatting for clarity. The requirement spans offline content removal, fallback logic, playback pausing, and atomic persistence—each deserving clearer exposition.
app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/playback/PlaybackSettingsScreen.kt (1)
73-87: Simplify the nested Column structure.The outer Column (lines 73-80) only wraps a single inner Column (lines 81-87), which is redundant. The outer Column doesn't add meaningful layout behavior and can be removed for cleaner code.
🔎 Proposed refactor to remove nested Column
content = { innerPadding -> - Column( - modifier = - Modifier - .fillMaxSize() - .padding(innerPadding), - verticalArrangement = Arrangement.Top, - horizontalAlignment = Alignment.CenterHorizontally, - ) { - Column( - modifier = - Modifier - .fillMaxWidth() - .verticalScroll(rememberScrollState()), - horizontalAlignment = Alignment.CenterHorizontally, - ) { + Column( + modifier = + Modifier + .fillMaxSize() + .padding(innerPadding) + .verticalScroll(rememberScrollState()), + verticalArrangement = Arrangement.Top, + horizontalAlignment = Alignment.CenterHorizontally, + ) { SettingsToggleItem(app/src/main/kotlin/org/grakovne/lissen/ui/components/DownloadProgressIcon.kt (1)
25-50: Well-implemented progress indicator with good accessibility support.The component correctly handles:
- Progress clamping to valid range
- Proper semantics for screen readers (progressBarRangeInfo and contentDescription)
- Material3 CircularProgressIndicator API usage
One minor edge case: if
sizeis passed as less than 2.dp,iconSizebecomes negative (line 36), which could cause unexpected rendering. Consider adding a guard or usingcoerceAtLeast(0.dp).Optional: Guard against negative iconSize
- val iconSize = size - 2.dp + val iconSize = (size - 2.dp).coerceAtLeast(0.dp)app/src/main/kotlin/org/grakovne/lissen/ui/screens/details/BookDetailScreen.kt (4)
134-140: Consider importing Timber instead of using fully-qualified calls.The inline
timber.log.Timber.d()calls work but are inconsistent with typical Kotlin style. Consider addingimport timber.log.Timberand usingTimber.d()directly.🔎 Proposed fix
Add to imports:
import timber.log.TimberThen update usages:
LaunchedEffect(bookId) { - timber.log.Timber.d("BookDetailScreen: Launched with bookId $bookId") + Timber.d("BookDetailScreen: Launched with bookId $bookId") } LaunchedEffect(bookDetail) { - timber.log.Timber.d("BookDetailScreen: bookDetail changed to ${bookDetail?.id}, title: ${bookDetail?.title}") + Timber.d("BookDetailScreen: bookDetail changed to ${bookDetail?.id}, title: ${bookDetail?.title}") }
330-335: Usevalinstead ofvarfor immutable variable.
durationSecondsis never reassigned after initialization, so it should be declared withval.🔎 Proposed fix
- var durationSeconds = + val durationSeconds = if (isStarted) { (totalDuration - currentPosition).toLong() } else { totalDuration.toLong() }
413-424: Address incomplete chapter click implementation.The inline comments indicate uncertainty about the intended behavior when clicking a chapter for a book that isn't currently playing. Currently, if
playingBook?.id != bookIdand!isPlaybackReady, the click is ignored.Consider either:
- Starting playback from the clicked chapter (may need a new ViewModel method)
- Preparing playback and then seeking to the chapter
- Documenting that chapters are view-only when the book isn't active
Would you like me to propose a more complete implementation for starting playback from a specific chapter?
501-502: Inconsistent use of fully-qualifiedmutableStateOf.
mutableStateOfis used with its fully-qualified name whilerememberis imported. For consistency, either importmutableStateOfor use the existing import at line 61.🔎 Proposed fix
@Composable fun ExpandableDescription(description: String) { - var expanded by remember { androidx.compose.runtime.mutableStateOf(false) } - var overflowDetected by remember { androidx.compose.runtime.mutableStateOf(false) } + var expanded by remember { mutableStateOf(false) } + var overflowDetected by remember { mutableStateOf(false) }Also fix line 510:
- var layoutResult by remember { androidx.compose.runtime.mutableStateOf<TextLayoutResult?>(null) } + var layoutResult by remember { mutableStateOf<TextLayoutResult?>(null) }app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/GlobalPlayerBottomSheet.kt (2)
153-153: Remove unused variable.The
contextvariable is declared but never used. This appears to be leftover from refactoring.🔎 Proposed fix
fun GlobalMiniPlayer( book: DetailedItem, imageLoader: ImageLoader, playerViewModel: PlayerViewModel, onOpenPlayer: () -> Unit, ) { - // We pass a dummy NavController or null since we use onContentClick - // However, MiniPlayerComposable still requires a AppNavigationService in the signature. - // We can construct a dummy one safely because it won't be used for the click action. - val context = androidx.compose.ui.platform.LocalContext.current - org.grakovne.lissen.ui.screens.library.composables.MiniPlayerComposable(
225-236: Simplify redundant null check.The
playingBook != nullcheck at line 225 makes theplayingBook?.letat line 226 redundant. Consider using smart-cast directly.🔎 Proposed fix
- if (playingBook != null && isPlaybackReady) { - playingBook?.let { - NavigationBarComposable( - book = it, - playerViewModel = playerViewModel, - contentCachingModelView = cachingModelView, - settingsViewModel = settingsViewModel, - navController = navController, - libraryType = libraryViewModel.fetchPreferredLibraryType(), - ) - } + if (playingBook != null && isPlaybackReady) { + NavigationBarComposable( + book = playingBook, + playerViewModel = playerViewModel, + contentCachingModelView = cachingModelView, + settingsViewModel = settingsViewModel, + navController = navController, + libraryType = libraryViewModel.fetchPreferredLibraryType(), + ) }app/src/main/kotlin/org/grakovne/lissen/content/BookRepository.kt (1)
1-393: Significant code duplication withLissenMediaProvider.
BookRepositorylargely duplicates the logic fromLissenMediaProvider.kt(same methods likeprovideFileUri,fetchBook,syncFromLocalProgress,makeAvailableIfOnline, etc.). Consider whether both classes are needed or if one should delegate to the other to avoid maintaining parallel implementations.#!/bin/bash # Check how both classes are used to understand if they serve different purposes rg -nP "LissenMediaProvider|BookRepository" --type=kotlin -l | head -20app/src/main/kotlin/org/grakovne/lissen/viewmodel/LibraryViewModel.kt (5)
71-77: Consider adding distinctUntilChanged to prevent redundant downstream processing.The
downloadedOnlyFlowcombines two sources without deduplication. If the combined boolean value doesn't change, downstream collectors (like the init block at line 105 and libraryPager at line 162) will still be notified, potentially triggering unnecessary refreshes and syncs.🔎 Proposed enhancement
private val downloadedOnlyFlow = combine( networkService.isServerAvailable, preferences.forceCacheFlow, ) { isServerAvailable, isForceCache -> !isServerAvailable || isForceCache - } + }.distinctUntilChanged()
83-101: Consider edge case: Genuinely empty libraries.The
emptyContentcheck will trigger a refresh wheneveritemCount == 0. For libraries that are legitimately empty or when all books are filtered out, this could cause repeated refresh attempts. Consider tracking whether a refresh has already been attempted or adding a cooldown mechanism.
108-126: Comment indicates incomplete reactive implementation.The detailed comment (lines 115-121) reveals that
BookRepository.fetchRecentListenedBooksFlowdoesn't reactively handleisOfflinechanges internally. The current workaround—re-collecting whendownloadedOnlyFlowemits—forces re-evaluation, but this is a temporary solution.Consider filing a technical debt issue to make
BookRepositoryfully reactive to network state changes, eliminating the need for manual re-collection.Would you like me to create an issue to track this repository-layer reactive enhancement?
186-192: Prevent overlapping sync operations.
syncLibrary()is launched fromonEachwithout tracking or canceling previous instances. Rapid flow emissions could start multiple concurrent sync operations, leading to:
- Wasted resources (duplicate network calls, database operations)
- Race conditions in state updates
- Unpredictable ordering of
refreshRecentListening()resultsConsider using a
Mutexor tracking the sync job to ensure only one sync runs at a time.🔎 Proposed fix with mutex
+private val syncMutex = kotlinx.coroutines.sync.Mutex() + private fun syncLibrary(libraryId: String) { viewModelScope.launch(Dispatchers.IO) { + syncMutex.withLock { bookRepository.syncRepositories() refreshRecentListening() defaultPagingSource.value?.invalidate() + } } }
237-250: Consider adding error handling.The migration to
bookRepository.fetchRecentListenedBooks()looks correct, but there's no try-catch block. If the repository throws an exception,_recentBookUpdatingwill remaintrue, potentially showing an infinite loading state in the UI.🔎 Proposed enhancement with error handling
viewModelScope.launch { - bookRepository.fetchRecentListenedBooks(preferredLibrary) - _recentBookUpdating.postValue(false) + try { + bookRepository.fetchRecentListenedBooks(preferredLibrary) + } finally { + _recentBookUpdating.postValue(false) + } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
.agent/rules/project-context.mdapp/src/main/kotlin/org/grakovne/lissen/channel/audiobookshelf/podcast/converter/PodcastPageResponseConverter.ktapp/src/main/kotlin/org/grakovne/lissen/common/NetworkService.ktapp/src/main/kotlin/org/grakovne/lissen/common/RunningComponent.ktapp/src/main/kotlin/org/grakovne/lissen/content/AuthRepository.ktapp/src/main/kotlin/org/grakovne/lissen/content/BookRepository.ktapp/src/main/kotlin/org/grakovne/lissen/content/LissenMediaProvider.ktapp/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/ContentCachingManager.ktapp/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/LocalCacheRepository.ktapp/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/OfflineBookStorageProperties.ktapp/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/api/CachedBookRepository.ktapp/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/dao/CachedBookDao.ktapp/src/main/kotlin/org/grakovne/lissen/playback/service/LissenDataSourceFactory.ktapp/src/main/kotlin/org/grakovne/lissen/playback/service/ShakeDetector.ktapp/src/main/kotlin/org/grakovne/lissen/ui/components/DownloadProgressIcon.ktapp/src/main/kotlin/org/grakovne/lissen/ui/effects/WindowBlurEffect.ktapp/src/main/kotlin/org/grakovne/lissen/ui/screens/details/BookDetailScreen.ktapp/src/main/kotlin/org/grakovne/lissen/ui/screens/library/composables/MiniPlayerComposable.ktapp/src/main/kotlin/org/grakovne/lissen/ui/screens/player/GlobalPlayerBottomSheet.ktapp/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/common/SeekButton.ktapp/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/SettingsScreen.ktapp/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/playback/PlaybackSettingsScreen.ktapp/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/playback/SmartRewindSettingsScreen.ktapp/src/main/kotlin/org/grakovne/lissen/viewmodel/LibraryViewModel.ktapp/src/main/res/values/strings.xmllib/src/main/kotlin/org/grakovne/lissen/lib/domain/Book.kt
🚧 Files skipped from review as they are similar to previous changes (7)
- app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/SettingsScreen.kt
- app/src/main/kotlin/org/grakovne/lissen/content/AuthRepository.kt
- lib/src/main/kotlin/org/grakovne/lissen/lib/domain/Book.kt
- app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/playback/SmartRewindSettingsScreen.kt
- app/src/main/kotlin/org/grakovne/lissen/playback/service/ShakeDetector.kt
- app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/common/SeekButton.kt
- app/src/main/kotlin/org/grakovne/lissen/ui/effects/WindowBlurEffect.kt
🧰 Additional context used
🧬 Code graph analysis (7)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/playback/PlaybackSettingsScreen.kt (3)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/composable/SettingsToggleItem.kt (1)
SettingsToggleItem(19-76)app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/advanced/AdvancedSettingsNavigationItemComposable.kt (1)
AdvancedSettingsNavigationItemComposable(22-61)app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/composable/PlaybackVolumeBoostSettingsComposable.kt (1)
PlaybackVolumeBoostSettingsComposable(27-76)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/GlobalPlayerBottomSheet.kt (6)
app/src/main/kotlin/org/grakovne/lissen/ui/effects/WindowBlurEffect.kt (1)
WindowBlurEffect(11-21)app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/placeholder/TrackDetailsPlaceholderComposable.kt (1)
TrackDetailsPlaceholderComposable(30-100)app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/TrackDetailsComposable.kt (1)
TrackDetailsComposable(39-133)app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/placeholder/TrackControlPlaceholderComposable.kt (1)
TrackControlPlaceholderComposable(35-155)app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/TrackControlComposable.kt (1)
TrackControlComposable(48-217)app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/NavigationBarComposable.kt (1)
NavigationBarComposable(61-257)
app/src/main/kotlin/org/grakovne/lissen/playback/service/LissenDataSourceFactory.kt (1)
app/src/main/kotlin/org/grakovne/lissen/playback/service/LissenMediaSchemeConverter.kt (1)
unapply(16-26)
app/src/main/kotlin/org/grakovne/lissen/content/LissenMediaProvider.kt (1)
app/src/main/kotlin/org/grakovne/lissen/content/BookRepository.kt (4)
providePreferredChannel(392-392)syncFromLocalProgress(330-368)syncFromLocalProgress(370-386)makeAvailableIfOnline(265-278)
app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/LocalCacheRepository.kt (1)
app/src/main/kotlin/org/grakovne/lissen/content/cache/common/FindRelatedFiles.kt (1)
findRelatedFiles(6-27)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/library/composables/MiniPlayerComposable.kt (2)
app/src/main/kotlin/org/grakovne/lissen/ui/components/AsyncShimmeringImage.kt (1)
AsyncShimmeringImage(21-65)app/src/main/kotlin/org/grakovne/lissen/common/HapticAction.kt (1)
withHaptic(6-12)
app/src/main/kotlin/org/grakovne/lissen/content/BookRepository.kt (1)
app/src/main/kotlin/org/grakovne/lissen/content/LissenMediaProvider.kt (4)
providePreferredChannel(460-460)syncFromLocalProgress(388-426)syncFromLocalProgress(428-452)makeAvailableIfOnline(253-266)
🪛 LanguageTool
.agent/rules/project-context.md
[grammar] ~9-~9: Use a hyphen to join words.
Context: ...es ## About Project This is an offline first Android client app for the [audiob...
(QB_NEW_EN_HYPHEN)
[style] ~15-~15: This phrase is redundant (‘N’ stands for ‘network’). Use simply “lan”.
Context: ...as connecting to a new wifi network, or lan network, or disconnecting from a network and co...
(ACRONYM_TAUTOLOGY)
[style] ~15-~15: In American English, abbreviations like “etc.” require a period.
Context: ... wifi and connecting to celular network etc, and try to ping or reachout the audiob...
(ETC_PERIOD)
[grammar] ~15-~15: Ensure spelling is correct
Context: ...celular network etc, and try to ping or reachout the audiobook server to check whether t...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~18-~18: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...hould be given priority for playback. - If the offline track is deleted / cleared ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~32-~32: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ocal cache or online from the server. - When the network is switched, the app should...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[grammar] ~32-~32: Use a hyphen to join words.
Context: ...hould update the UI to only show offline available ready to play books. ## Gener...
(QB_NEW_EN_HYPHEN)
[grammar] ~32-~32: Use a hyphen to join words.
Context: ... UI to only show offline available ready to play books. ## General Guidelines an...
(QB_NEW_EN_HYPHEN)
[grammar] ~32-~32: Use a hyphen to join words.
Context: ... to only show offline available ready to play books. ## General Guidelines and S...
(QB_NEW_EN_HYPHEN)
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (41)
.agent/rules/project-context.md (1)
1-42: Overall documentation structure and guidelines are well-aligned with PR objectives.This file effectively establishes the offline-first design philosophy and coding standards for the project. The feature requirements, stability guidelines, and general standards are actionable and align with the PR's focus on offline-first data flow, caching enhancements, and player UI improvements. Once the grammar and clarity issues are addressed, this will be a solid reference document for contributors.
app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/playback/PlaybackSettingsScreen.kt (5)
1-36: LGTM! Clean imports.The imports are well-organized and include all necessary dependencies for the Compose UI, Material3 components, Hilt integration, and internal project components.
37-42: LGTM! Clear function signature.The composable function signature is well-defined with appropriate parameters for navigation handling.
43-45: LGTM! Proper state management.The ViewModel is correctly injected via Hilt, and LiveData states are properly observed with sensible default values.
47-67: LGTM! Well-structured TopAppBar.The Scaffold and TopAppBar are properly configured with correct Material3 patterns, back navigation, and internationalized strings.
88-115: LGTM! Settings items properly configured.All settings composables are correctly wired with appropriate callbacks, state bindings, and internationalized strings. The integration follows the established patterns from the referenced composables.
app/src/main/kotlin/org/grakovne/lissen/ui/components/DownloadProgressIcon.kt (1)
51-58: LGTM!The fallback icon branch is clean and accessible with proper content description.
app/src/main/res/values/strings.xml (4)
2-2: LGTM!The
translatable="false"attribute is correctly applied for the app name.
160-180: LGTM!New string resources are well-structured with consistent naming conventions and proper format specifiers. Action labels use appropriate title case for UI buttons.
181-188: Plurals are correctly structured for English.The "one" and "other" quantities are properly defined. Note that if the app supports other languages in the future, some locales (e.g., Arabic, Polish, Russian) require additional quantity forms (
zero,two,few,many). This would be handled in translatedstrings.xmlfiles.
189-199: LGTM!The new strings for download progress, seek operations, and playback preferences are well-defined. The
download_progress_descriptioncorrectly corresponds with its usage inDownloadProgressIcon.kt.app/src/main/kotlin/org/grakovne/lissen/playback/service/LissenDataSourceFactory.kt (2)
67-70: LGTM: Transfer listener propagation is correct.Propagating the listener to both underlying data sources ensures events are captured regardless of which source becomes active during playback.
101-118: LGTM: Excellent defensive programming and resource management.The implementations demonstrate good practices:
read()fails fast with a clear error if called beforeopen()close()properly releases resources and resets state- All methods correctly delegate to
currentDataSourceapp/src/main/kotlin/org/grakovne/lissen/ui/screens/details/BookDetailScreen.kt (2)
560-588: LGTM!The
toAnnotatedStringextension function correctly converts AndroidSpannedHTML content to ComposeAnnotatedString, handling common span types (bold, italic, underline, strikethrough, color, and URLs). Unhandled span types are gracefully ignored.
590-615: LGTM!
BookInfoTileis a clean, reusable composable with appropriate styling and layout.app/src/main/kotlin/org/grakovne/lissen/channel/audiobookshelf/podcast/converter/PodcastPageResponseConverter.kt (1)
13-31: LGTM!The method signature change to include
libraryIdand the population of theBookobject with this parameter aligns well with the broader codebase changes. The hardcodedduration = 0.0is appropriately documented as an API limitation.app/src/main/kotlin/org/grakovne/lissen/ui/screens/library/composables/MiniPlayerComposable.kt (2)
65-66: LGTM - Good API flexibility.The optional
navControllerandonContentClickparameters provide flexibility for different usage contexts (library screen vs. global player sheet). The null-check pattern at lines 132-138 correctly prioritizesonContentClickover navigation.
140-161: LGTM - Progress bar implementation is correct.The progress calculation handles the edge case of zero duration properly with the ternary check
if (totalDuration > 0). The use ofremember(book)fortotalDurationis appropriate for memoization.app/src/main/kotlin/org/grakovne/lissen/content/LissenMediaProvider.kt (3)
53-60: Good fix for exception handling.The try-catch around the remote URI fetch properly addresses the previous review feedback about unhandled exceptions. Errors are now correctly wrapped in
OperationResult.Error.
268-280: LGTM - Reactive book flow with network-aware availability.The
fetchBookFlowcorrectly combines the local cache flow with network status to dynamically mark chapters as available when online. The logic mirrorsmakeAvailableIfOnlinebut in a reactive context.
352-380: Verify sync behavior doesn't cause excessive network calls.The
syncRepositoriesfunction fetches book details for every item where remote is newer. For users with many books, this could trigger many sequential network calls within the loop (lines 374-378).Consider whether batching or rate-limiting these fetches might be beneficial for users with large libraries, or if this sync is called infrequently enough that sequential calls are acceptable.
app/src/main/kotlin/org/grakovne/lissen/common/RunningComponent.kt (1)
5-6: LGTM!Adding
onDestroy()with a default empty implementation is a clean, non-breaking extension to the interface. This allows components likeNetworkServiceto implement cleanup logic without forcing all implementors to override.app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/ContentCachingManager.kt (3)
113-113: Good typo fix.Renaming
provideMediaCachePatchtoprovideMediaCachePathcorrects the naming and improves code clarity.
121-135: Verify behavior change indropCache(itemId).The behavior has changed from removing the book outright to preserving the book entity while marking all chapters as dropped. Ensure this aligns with the expected cache-clearing behavior - the book metadata will remain in the database even after dropping all cached content.
Is preserving the book metadata intentional? If users expect "drop cache" to fully remove the item from the local database, this might cause confusion or leave orphaned records.
144-144: LGTM - New utility method.The
hasDownloadedChaptersmethod is a clean delegation to the repository, supporting the offline-first functionality in the UI layer.app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/OfflineBookStorageProperties.kt (1)
30-36: LGTM! Method rename corrects typo.The rename from
provideMediaCachePatchtoprovideMediaCachePathfixes a typo and aligns with the path-based naming conventions used throughout the caching layer.app/src/main/kotlin/org/grakovne/lissen/common/NetworkService.kt (3)
37-43: LGTM! Well-structured reactive state management.The StateFlow properties for
networkStatusandisServerAvailableprovide a clean reactive interface for components to observe connectivity state. The use ofSupervisorJobensures that individual failures don't cancel the entire scope.
50-51: Good enhancement to network monitoring coverage.Adding
CELLULARandETHERNETtransport types ensures comprehensive network monitoring across different connectivity scenarios.
162-164: LGTM! Proper lifecycle management.The
onDestroymethod correctly cancels the coroutine scope, preventing leaks and ensuring clean shutdown of ongoing operations.app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/LocalCacheRepository.kt (3)
92-101: LGTM! Offline-first filtering support added.The
downloadedOnlyparameter enables filtering books by their download status, which is essential for offline-first functionality. The parameter is properly passed through to the repository layer.
132-140: LGTM! Flow-based API enables reactive UI updates.The addition of
fetchRecentListenedBooksFlowallows UI components to reactively observe changes to recent books, improving user experience with real-time updates.
162-187: LGTM! Robust metadata caching with file verification.The
cacheBookMetadatamethod correctly partitions chapters based on whether their related files exist in storage. The logic properly:
- Uses
findRelatedFilesto identify dependencies- Verifies all related files exist before marking a chapter as available
- Handles errors gracefully with Timber logging
The complexity is justified given the requirement to maintain cache consistency.
app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/api/CachedBookRepository.kt (2)
43-44: LGTM! Typo fix applied.The update from
provideMediaCachePatchtoprovideMediaCachePathcorrects the method name.
189-207: LGTM! Flow-based API with progress mapping.The
fetchRecentBooksFlowcorrectly streams recent books and maps progress data using the converter. The reactive pattern enables UI components to receive real-time updates when cache state changes.app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/dao/CachedBookDao.kt (4)
141-146: LGTM! Flow-based queries with proper reactive annotations.The addition of both Flow and suspend variants for fetching recently listened books provides flexibility:
fetchRecentlyListenedCachedBooksFlowenables reactive UI updates with properobservedEntitiesannotationfetchRecentlyListenedCachedBooksprovides direct suspend access for one-time queriesThe
observedEntitiesannotation correctly includes bothBookEntityandMediaProgressEntity, ensuring the Flow emits when either changes.
152-154: LGTM! Reactive book detail streaming.The
fetchBookFlowenables reactive observation of individual book details, supporting real-time UI updates when book metadata or cache state changes.
190-198: LGTM! Downloaded chapters check added.The
hasDownloadedChaptersquery correctly checks if any chapters for a book are cached, which is essential for determining partial download states. The LiveData return type enables UI components to observe changes.
217-224: LGTM! Batch operations for efficient bulk updates.The addition of batch operations (
upsertBooks,fetchBooks,updateBook) enables efficient bulk data manipulation, which is essential for synchronization and caching workflows.app/src/main/kotlin/org/grakovne/lissen/viewmodel/LibraryViewModel.kt (3)
17-29: LGTM - Imports align with new functionality.The new imports support the repository pattern migration, network monitoring, and reactive flow operations.
135-157: LGTM - Search pager updated for repository pattern.The migration from media provider to
bookRepositoryis consistent with the architectural shift to repository-driven data access.
226-235: LGTM - Safe nullable paging source access.The migration to
defaultPagingSource.value?.invalidate()correctly handles the nullable state with a safe call operator.
| suspend fun cacheBooks(books: List<Book>) { | ||
| if (books.isEmpty()) return | ||
|
|
||
| val bookIds = books.map { it.id } | ||
| val existingBooks = | ||
| bookDao | ||
| .fetchBooks(bookIds) | ||
| .associateBy { it.id } | ||
|
|
||
| val entities = | ||
| books | ||
| .map { book -> | ||
| val existing = existingBooks[book.id] | ||
|
|
||
| existing?.copy( | ||
| title = book.title, | ||
| author = book.author, | ||
| subtitle = book.subtitle, | ||
| seriesNames = book.series, | ||
| duration = book.duration.toInt(), | ||
| ) ?: BookEntity( | ||
| id = book.id, | ||
| title = book.title, | ||
| author = book.author, | ||
| subtitle = book.subtitle, | ||
| narrator = null, | ||
| publisher = null, | ||
| year = null, | ||
| abstract = null, | ||
| duration = book.duration.toInt(), | ||
| libraryId = book.libraryId, | ||
| createdAt = 0, | ||
| updatedAt = 0, | ||
| seriesNames = book.series, | ||
| seriesJson = "[]", | ||
| ) | ||
| } | ||
|
|
||
| bookDao.upsertBooks(entities) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find BookEntity class definition
rg -n "class BookEntity" --type=kt -A 30Repository: SurjitSahoo/lissen-up
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Check how the nullable and defaulted fields are used in the codebase
rg -n "\.narrator|\.publisher|\.year|\.abstract|\.createdAt|\.updatedAt|\.seriesJson" --type=kt -C 2 | head -100Repository: SurjitSahoo/lissen-up
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Look for null checks or safe navigation on these fields
rg -n "narrator\?|publisher\?|year\?|abstract\?|createdAt\?|updatedAt\?" --type=ktRepository: SurjitSahoo/lissen-up
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Find where BookEntity is instantiated in the repository
rg -n "BookEntity(" --type=kt -C 3 | head -150Repository: SurjitSahoo/lissen-up
Length of output: 137
🏁 Script executed:
#!/bin/bash
# Find BookEntity class definition in Kotlin files
find . -name "BookEntity.kt" -o -name "*BookEntity*.kt" | head -10Repository: SurjitSahoo/lissen-up
Length of output: 490
🏁 Script executed:
#!/bin/bash
# Search for BookEntity class definition
rg -n "class BookEntity" --type-add 'kt:*.kt' -t kt -A 30 2>/dev/null || rg -n "class BookEntity" -A 30 | head -100Repository: SurjitSahoo/lissen-up
Length of output: 3723
🏁 Script executed:
#!/bin/bash
# Find usage of nullable fields - use simpler regex
rg "narrator|publisher|abstract" -g "*.kt" -C 2 | head -150Repository: SurjitSahoo/lissen-up
Length of output: 15669
🏁 Script executed:
#!/bin/bash
# Check for BookEntity instantiation patterns
rg "BookEntity\(" -g "*.kt" -C 2 | head -200Repository: SurjitSahoo/lissen-up
Length of output: 2435
🏁 Script executed:
#!/bin/bash
# Check how createdAt and updatedAt are used
rg "createdAt|updatedAt" -g "*.kt" -C 2 | head -200Repository: SurjitSahoo/lissen-up
Length of output: 8887
🏁 Script executed:
#!/bin/bash
# Check the actual file where BookEntity is used in the cacheBooks function
cat -n app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/api/CachedBookRepository.kt | sed -n '64,103p'Repository: SurjitSahoo/lissen-up
Length of output: 1491
🏁 Script executed:
#!/bin/bash
# Check if there's any validation or initialization logic for these fields
rg "createdAt|updatedAt" -g "*.kt" app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/ -C 1Repository: SurjitSahoo/lissen-up
Length of output: 4647
Set createdAt and updatedAt to current timestamp instead of hardcoded 0.
The nullable fields (narrator, publisher, year, abstract, seriesJson) are appropriate—they're properly null-checked downstream using safe navigation and conditional rendering. However, initializing createdAt and updatedAt to 0 is incorrect and will cause issues:
- These fields are used in sorting queries (LibraryOrderingOption.CREATED_AT and UPDATED_AT), so books with zero timestamps will sort to epoch time (1970-01-01), distorting library ordering.
- Other converters (BookResponseConverter, PodcastResponseConverter) use actual timestamps (
item.addedAt,item.ctimeMs), making this inconsistent. - A zero timestamp is semantically misleading for cache timestamps.
Replace with System.currentTimeMillis() for both fields to match the behavior of other data sources and maintain correct sorting behavior.
🤖 Prompt for AI Agents
In
app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/api/CachedBookRepository.kt
around lines 64 to 103, the code sets createdAt and updatedAt to 0 when creating
new BookEntity which causes incorrect sorting and inconsistency with other
converters; change those assignments to use the current timestamp
(System.currentTimeMillis()) so new cached books have proper createdAt and
updatedAt values consistent with other data sources.
| override fun open(dataSpec: DataSpec): Long { | ||
| val (itemId, fileId) = unapply(dataSpec.uri) ?: return 0 |
There was a problem hiding this comment.
Returning 0 on invalid URI may cause silent failures.
When unapply() returns null (indicating an invalid URI scheme or format), the method returns 0, which callers may interpret as a successful open with zero bytes available. This could lead to confusing playback behavior where invalid URIs are silently ignored rather than failing explicitly.
Consider throwing an exception to fail fast on invalid URIs.
🔎 Proposed fix
override fun open(dataSpec: DataSpec): Long {
- val (itemId, fileId) = unapply(dataSpec.uri) ?: return 0
+ val (itemId, fileId) = unapply(dataSpec.uri)
+ ?: throw IllegalArgumentException("Invalid lissen:// URI format: ${dataSpec.uri}")📝 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.
| override fun open(dataSpec: DataSpec): Long { | |
| val (itemId, fileId) = unapply(dataSpec.uri) ?: return 0 | |
| override fun open(dataSpec: DataSpec): Long { | |
| val (itemId, fileId) = unapply(dataSpec.uri) | |
| ?: throw IllegalArgumentException("Invalid lissen:// URI format: ${dataSpec.uri}") |
🤖 Prompt for AI Agents
In
app/src/main/kotlin/org/grakovne/lissen/playback/service/LissenDataSourceFactory.kt
around lines 72-73, the open(dataSpec: DataSpec) currently returns 0 when
unapply(dataSpec.uri) is null which can silently succeed; instead detect the
null and throw an appropriate exception (e.g., an IOException with a clear
message including the invalid URI) so callers fail fast; ensure the method
signature still satisfies the DataSource contract and replace the return 0 with
throwing the exception to signal an invalid URI.
| val source = | ||
| when (resolvedUri.scheme) { | ||
| "file" -> defaultDataSource | ||
| else -> cacheDataSource | ||
| } |
There was a problem hiding this comment.
Consider case-insensitive scheme matching.
URI schemes are case-insensitive per RFC 3986, but the current check only matches lowercase "file". While Android's Uri.scheme typically returns lowercase, URIs from external sources or manual construction might use different casing (e.g., "FILE" or "File"), causing incorrect routing.
🔎 Proposed fix
val source =
when (resolvedUri.scheme) {
- "file" -> defaultDataSource
+ "file" -> defaultDataSource
+ null -> defaultDataSource // Also handle null scheme as file path
- else -> cacheDataSource
+ else -> if (resolvedUri.scheme.equals("file", ignoreCase = true)) {
+ defaultDataSource
+ } else {
+ cacheDataSource
+ }
}Or more concisely:
val source =
- when (resolvedUri.scheme) {
- "file" -> defaultDataSource
- else -> cacheDataSource
- }
+ if (resolvedUri.scheme.equals("file", ignoreCase = true)) {
+ defaultDataSource
+ } else {
+ cacheDataSource
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
app/src/main/kotlin/org/grakovne/lissen/playback/service/LissenDataSourceFactory.kt
around lines 91 to 95, the scheme check only matches the lowercase "file" which
can misclassify URIs with different casing; update the branch to perform a
case-insensitive match (e.g., check resolvedUri.scheme?.equals("file",
ignoreCase = true) or normalize with .lowercase(Locale.US) before comparing) and
handle a possible null scheme by treating it as non-file so the default routing
to cacheDataSource remains correct.
| currentDataSource = source | ||
| return source.open(newSpec) |
There was a problem hiding this comment.
Potential resource leak if open() is called multiple times.
If open() is invoked again without calling close(), the previous currentDataSource won't be closed, potentially leaking file handles or network connections. While the media3 framework typically manages the lifecycle correctly, defensive programming suggests closing any existing source before opening a new one.
🔎 Proposed fix
+currentDataSource?.close()
currentDataSource = source
return source.open(newSpec)📝 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.
| currentDataSource = source | |
| return source.open(newSpec) | |
| currentDataSource?.close() | |
| currentDataSource = source | |
| return source.open(newSpec) |
🤖 Prompt for AI Agents
In
app/src/main/kotlin/org/grakovne/lissen/playback/service/LissenDataSourceFactory.kt
around lines 97 to 98, the factory assigns currentDataSource = source and
immediately returns source.open(newSpec) which can leak the previously held data
source if open() is called again; before assigning and opening the new source,
defensively close any existing currentDataSource (check for null and call its
close/shutdown method) and handle/ignore exceptions from that close, then set
currentDataSource = source and call source.open(newSpec).
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
.agent/rules/project-context.md (1)
32-32: Fix hyphenation in compound adjectives.Use hyphens to join compound adjectives: "offline-available" and "ready-to-play".
🔎 Proposed fix
-- When the network is switched, the app should trigger checking whether the server is still reachable or not, if not reachable, it should update the UI to only show offline available ready to play books. +- When the network is switched, the app should trigger checking whether the server is still reachable; if not reachable, update the UI to show only offline-available, ready-to-play books.
🧹 Nitpick comments (3)
.agent/rules/project-context.md (1)
16-18: Consider refactoring sentence structure to reduce repetition.Lines 16–18 begin with consecutive "If" statements. While technically correct, rewording some of these for variety would improve readability and flow.
🔎 Example refactor
- If the server is reachable, the app should sync the local progress to the server and pull the latest progress updates from the server to the local database; it should merge the updates from both. - If the server is reachable, and some chapters of any audiobook are downloaded, i.e., available offline, then the offline track should be given priority for playback. - If the offline track is deleted/cleared while the book is being played, the player should attempt to fallback to the online URL if the server is reachable, otherwise pause playback and persist the last playback state (track ID, playback position/timestamp, current chapter/index, and playback status) plus a flag indicating offline content was removed; ensure the rule notes that these persisted fields are used to resume or report playback state and are written atomically to the player state store. + When the server is reachable, the app should sync local progress to the server and pull the latest progress updates; it should merge updates from both sources. + For downloaded chapters (available offline), prioritize the offline track for playback when the server is reachable. + Should offline content be deleted while playback is in progress, attempt fallback to the online URL (if the server is reachable); otherwise, pause playback and atomically persist playback state (track ID, position, chapter index, status) plus a flag indicating offline content was removed.app/src/main/kotlin/org/grakovne/lissen/persistence/preferences/LissenSharedPreferences.kt (1)
479-489: Good defensive pattern for enum deserialization.The helper correctly handles both null and invalid enum values by returning the default. The static analysis warning about swallowing the exception is a false positive—graceful fallback to defaults is the intended behavior for corrupted or migrated preferences.
💡 Optional: Add debug logging for preference corruption visibility
If you want visibility into preference corruption during development/testing:
private inline fun <reified T : Enum<T>> safeEnumValueOf( value: String?, default: T, ): T { if (value == null) return default return try { enumValueOf<T>(value) } catch (e: Exception) { + Timber.d("Invalid enum value '$value' for ${T::class.simpleName}, using default: $default") default } }app/src/main/kotlin/org/grakovne/lissen/common/NetworkService.kt (1)
87-120: Server reachability check works but consider refactoring to HTTP-based approach.The socket-based implementation correctly checks server availability. However, as noted in previous reviews, using an OkHttpClient HEAD request would be more consistent with the project's HTTP-based architecture and allow reuse of existing client infrastructure.
Additionally, the hard-coded timeouts (2000ms socket timeout, 500ms delay) could be made configurable via preferences for operational flexibility.
The current implementation is functional, but the HTTP-based approach would be more maintainable. See the previous review comment for detailed guidance on the migration approach.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.agent/rules/project-context.md.vscode/tasks.jsonapp/src/main/kotlin/org/grakovne/lissen/common/NetworkService.ktapp/src/main/kotlin/org/grakovne/lissen/persistence/preferences/LissenSharedPreferences.kt
🧰 Additional context used
🪛 detekt (1.23.8)
app/src/main/kotlin/org/grakovne/lissen/persistence/preferences/LissenSharedPreferences.kt
[warning] 486-486: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🪛 LanguageTool
.agent/rules/project-context.md
[style] ~18-~18: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...hould be given priority for playback. - If the offline track is deleted/cleared wh...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~32-~32: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ocal cache or online from the server. - When the network is switched, the app should...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[grammar] ~32-~32: Use a hyphen to join words.
Context: ...hould update the UI to only show offline available ready to play books. ## Gener...
(QB_NEW_EN_HYPHEN)
[grammar] ~32-~32: Use a hyphen to join words.
Context: ... UI to only show offline available ready to play books. ## General Guidelines an...
(QB_NEW_EN_HYPHEN)
[grammar] ~32-~32: Use a hyphen to join words.
Context: ... to only show offline available ready to play books. ## General Guidelines and S...
(QB_NEW_EN_HYPHEN)
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (11)
app/src/main/kotlin/org/grakovne/lissen/persistence/preferences/LissenSharedPreferences.kt (5)
21-21: LGTM!The new domain type imports are properly organized and necessary for the smart rewind and timer functionality added below.
Also applies to: 24-24, 28-30
438-450: LGTM!The boolean preference methods follow the established pattern. The default values (false for nav buttons, true for shake-to-reset) are sensible for opt-in and opt-out features respectively.
452-477: LGTM!The smart rewind preference methods correctly use enum serialization with safe defaults. The use of
safeEnumValueOfprovides robust deserialization with proper fallback behavior.
491-593: LGTM!All Flow declarations consistently follow the established reactive pattern: listener registration, initial emission, proper cleanup, and deduplication. This enables reactive observation of preference changes across the app.
604-606: LGTM!The new preference key constants follow the established naming convention and are properly organized in the companion object.
Also applies to: 630-631
app/src/main/kotlin/org/grakovne/lissen/common/NetworkService.kt (6)
10-18: LGTM!The coroutine setup is well-structured:
Dispatchers.IOfor network operations,SupervisorJobfor isolated failure handling, and proper lifecycle management with cancellation inonDestroy.Also applies to: 30-30, 43-43
37-41: LGTM!The StateFlow pattern correctly exposes read-only public flows backed by private mutable state. The pessimistic default (false) ensures consumers wait for confirmed connectivity before proceeding.
50-51: LGTM!Expanding network monitoring to cellular and ethernet supports true offline-first behavior. Reactive server checks on all network events ensure consumers receive timely status updates.
Also applies to: 56-58, 64-72, 76-76
78-82: LGTM!Observing host changes via flow ensures immediate server reachability checks when users update their server URL, maintaining accurate connection state.
85-90: LGTM!The job cancellation pattern provides effective debouncing for rapid network events, preventing redundant server checks and resource waste.
168-170: LGTM!The
onDestroyimplementation properly cancels the coroutine scope, preventing memory leaks and ensuring all network monitoring coroutines terminate cleanly.
|
|
||
| ## Overall Functionality | ||
|
|
||
| - When the app loads, it should load the offline available book content immediately, then in the background reach out to the server (if the server is reachable) and fetch the full list of the books, continue listening section books and update the UI seemlessly. |
There was a problem hiding this comment.
Fix spelling error: "seamlessly".
The word "seemlessly" is misspelled; it should be "seamlessly".
🔎 Proposed fix
-- When the app loads, it should load the offline available book content immediately, then in the background reach out to the server (if the server is reachable) and fetch the full list of the books, continue listening section books and update the UI seemlessly.
+- When the app loads, it should load the offline available book content immediately, then in the background reach out to the server (if the server is reachable) and fetch the full list of the books, continue listening section books and update the UI seamlessly.🤖 Prompt for AI Agents
In .agent/rules/project-context.md around line 28, the word "seemlessly" is
misspelled; replace it with "seamlessly" so the sentence reads "...update the UI
seamlessly."
|
|
||
| - When the app loads, it should load the offline available book content immediately, then in the background reach out to the server (if the server is reachable) and fetch the full list of the books, continue listening section books and update the UI seemlessly. | ||
| - The app should cache all the book's metadata in local database to optimise the app load time, Only the chapters / audio tracks should not be cached automatically / by default. Chapters should be downloaded and cached on demand by the user, using the download chapters/book functionality. | ||
| - When the app loads, if the server is not reachable, it shouldn't show long loading screen, trying to fetch the books from the server, it should load the book's list from the local database, however it should only show the books whos' chapters are downloaded and available offline can be played. |
There was a problem hiding this comment.
Fix grammar error: "whose" (no apostrophe).
The possessive form should be "whose", not "whos'".
🔎 Proposed fix
-- When the app loads, if the server is not reachable, it shouldn't show long loading screen, trying to fetch the books from the server, it should load the book's list from the local database, however it should only show the books whos' chapters are downloaded and available offline can be played.
+- When the app loads, if the server is not reachable, it shouldn't show long loading screen, trying to fetch the books from the server, it should load the book's list from the local database, however it should only show the books whose chapters are downloaded and available offline can be played.📝 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.
| - When the app loads, if the server is not reachable, it shouldn't show long loading screen, trying to fetch the books from the server, it should load the book's list from the local database, however it should only show the books whos' chapters are downloaded and available offline can be played. | |
| - When the app loads, if the server is not reachable, it shouldn't show long loading screen, trying to fetch the books from the server, it should load the book's list from the local database, however it should only show the books whose chapters are downloaded and available offline can be played. |
🤖 Prompt for AI Agents
In .agent/rules/project-context.md around line 30, fix the grammar by replacing
the incorrect possessive "whos'" with "whose" and adjust surrounding wording for
clarity; update the sentence to read something like: "When the app loads, if the
server is not reachable, it shouldn't show a long loading screen trying to fetch
books from the server; it should load the book list from the local database, and
only show books whose chapters are downloaded and available offline for
playback."
| "tasks": [ | ||
| { | ||
| "type": "gradle", | ||
| "id": "${workspaceFolder}app:installDebugapp", |
There was a problem hiding this comment.
Fix the malformed task ID.
The task ID appears to have a concatenation issue: "${workspaceFolder}app:installDebugapp" contains installDebugapp without proper separation. This is inconsistent with the script name app:installDebug on line 7.
🔎 Proposed fix
- "id": "${workspaceFolder}app:installDebugapp",
+ "id": "${workspaceFolder}/app:installDebug:app",📝 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.
| "id": "${workspaceFolder}app:installDebugapp", | |
| "id": "${workspaceFolder}/app:installDebug:app", |
🤖 Prompt for AI Agents
In .vscode/tasks.json around line 6, the task ID is malformed as
"${workspaceFolder}app:installDebugapp" (it mistakenly concatenates "app"
twice); update the id to match the script name on line 7 by changing it to
"${workspaceFolder}app:installDebug" so the task ID exactly matches the actual
script/task name and removes the extra "app" suffix.
…on and playback control.
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Fix all issues with AI agents
In @app/build.gradle.kts:
- Line 53: The change introduced both an applicationId and versionCode
regression: restore the original applicationId value (set applicationId back to
"org.grakovne.lissen") and bump the versionCode to at least 10801 to preserve
upgrade path; alternatively, if this is an intentional new app identity/fork,
keep the new applicationId but reset versionCode appropriately (e.g., 1 or
10000) and add clear documentation in the PR/release notes stating this is a
breaking change requiring users to uninstall the old app and explaining data
migration guidance.
- Line 86: The release build is incorrectly hardcoded to use the debug keystore;
change the release signing to use the proper release signingConfig (or
conditional logic) instead of signingConfigs.getByName("debug"): locate the
buildTypes.release block and replace the debug reference with the correct
release signingConfig name (or add a conditional that selects
signingConfigs.getByName("release") when available, falling back to a
CI/keystore-provided config for local debug builds), ensuring the symbol
signingConfig in the release block points to the release keystore and not the
debug keystore.
In @app/src/main/kotlin/org/grakovne/lissen/playback/MediaRepository.kt:
- Around line 180-184: The anonymous CoroutineScope(Dispatchers.Main).launch
used to collect preferences.shakeToResetTimerFlow should be replaced with a
class-scoped SupervisorJob-backed CoroutineScope (e.g., repositoryScope) so
collections can be cancelled; create repositoryScope =
CoroutineScope(SupervisorJob() + Dispatchers.Main) and use
repositoryScope.launch to collect and call updateShakeDetectorState, and add a
public dispose() (or close()) method that cancels repositoryScope and stops the
shake detector (e.g., repositoryScope.cancel(); shakeDetector.stop()) to ensure
proper lifecycle management.
In
@app/src/main/kotlin/org/grakovne/lissen/ui/screens/details/BookDetailScreen.kt:
- Around line 395-403: The calculation of durationSeconds in BookDetailScreen
(variables: isStarted, totalDuration, currentPosition, durationSeconds) can go
negative if currentPosition > totalDuration; change the assignment to clamp to
non-negative (e.g., use maxOf(0L, totalDuration.toLong() -
currentPosition.toLong()) when isStarted) so hours and minutes are derived from
a non-negative duration; ensure the else branch also produces a non-negative
long (totalDuration.toLong()) and update any subsequent uses to rely on the
clamped durationSeconds.
- Around line 482-491: The progress calculation in BookDetailScreen (variables
chapterStart, chapterEnd, progressRaw, progress) can divide by zero when a
chapter has zero duration; guard against this by checking if chapterEnd ==
chapterStart (or duration <= 0) before computing progressRaw and set progress to
0f (or 1f if overallPosition >= chapterEnd) accordingly; update the when/if
logic to handle the zero-duration case explicitly so no division is performed
and progress is a stable float.
In
@app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/ChaptersBottomSheet.kt:
- Around line 84-100: The progress calculation can divide by zero when
chapter.duration == 0.0; update the computation around progressRaw/progress
(inside the itemsIndexed lambda where chapterStart, chapterEnd, progressRaw and
progress are calculated) to guard the denominator (chapterEnd - chapterStart)
and treat zero-duration chapters explicitly (e.g., set progressRaw to 0.0 or 1.0
based on currentPosition relative to chapter bounds) before applying the
existing when expression so no division by zero occurs.
In
@app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/NavigationBarComposable.kt:
- Around line 71-72: The file collects an unused state via
playerViewModel.isOnline.collectAsState into isOnline, which causes unnecessary
recompositions; remove the line "val isOnline by
playerViewModel.isOnline.collectAsState(initial = false)" from
NavigationBarComposable (and any now-unused imports) and ensure no remaining
references to isOnline exist in the composable body.
In
@app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/TrackDetailsComposable.kt:
- Around line 210-241: The function provideChapterNumberTitle is dead code and
should be removed: delete the entire private fun provideChapterNumberTitle(...)
declaration (including its when branches) from TrackDetailsComposable so there’s
no unused helper left; ensure no remaining references to
provideChapterNumberTitle exist and run a build to verify no usages or imports
depended on it.
- Around line 86-87: The shadow is being clipped because
.clip(RoundedCornerShape(8.dp)) is applied after .shadow(...); in the composable
where the modifier chain includes .clip(RoundedCornerShape(8.dp)) and
.shadow(12.dp, RoundedCornerShape(8.dp)) (in TrackDetailsComposable), swap the
order so .shadow(...) comes before .clip(...) using the same
RoundedCornerShape(8.dp) to allow the shadow to render outside the clipped
bounds.
In @app/src/main/res/values/strings.xml:
- Line 13: Remove the three dead string resources
player_screen_chapter_list_navigation_library,
player_screen_chapter_list_navigation_podcast, and
player_screen_chapter_list_navigation_items from the main strings.xml and all
localization files (values-*/ folders); before deleting, run a global search to
confirm they have no usages in code (the UI now uses
player_screen_chapter_list_title in BookDetailScreen.kt and
ChaptersBottomSheet.kt), then delete those entries from every localized
strings.xml to keep translations consistent and avoid unused resource warnings.
- Around line 174-177: The three string resources
player_screen_chapter_list_navigation_library,
player_screen_chapter_list_navigation_items, and
player_screen_chapter_list_navigation_podcast are dead code (no references) and
should be removed or justified; remove these three entries from strings.xml if
the app now uses player_screen_chapter_list_title (used in BookDetailScreen.kt
and ChaptersBottomSheet.kt), or if they must exist for a lint/build reason, add
a comment in strings.xml and a short note in the build config or a TODO linking
to that rationale so their presence is documented.
🧹 Nitpick comments (12)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/TrackControlComposable.kt (2)
39-39: Unused import.The
Colorimport appears unused in the current code.🧹 Proposed fix
-import androidx.compose.ui.graphics.Color
150-164: Consider disabling Previous button at first track.Unlike the Next button (lines 205-220) which properly disables and tints differently when at the last track, the Previous button remains always enabled even at the first track. For consistency, consider adding similar logic.
♻️ Proposed enhancement
if (showNavButtons) { IconButton( onClick = { - withHaptic(view) { viewModel.previousTrack() } + if (currentTrackIndex > 0) { + withHaptic(view) { viewModel.previousTrack() } + } }, - enabled = true, + enabled = currentTrackIndex > 0, ) { Icon( imageVector = Icons.Rounded.SkipPrevious, contentDescription = "Previous Track", - tint = colorScheme.onBackground, + tint = + if (currentTrackIndex > 0) { + colorScheme.onBackground + } else { + colorScheme.onBackground.copy(alpha = 0.3f) + }, modifier = Modifier.size(36.dp), ) } }app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/TrackDetailsComposable.kt (3)
4-6: Remove unused imports.The imports for
basicMarquee(line 5) andSurface(line 17) are not used anywhere in this file. TheExperimentalFoundationApiopt-in (line 45) is also unnecessary sincebasicMarqueeis never used.🧹 Proposed cleanup
-import androidx.compose.foundation.ExperimentalFoundationApi -import androidx.compose.foundation.basicMarquee import androidx.compose.foundation.clickable-import androidx.compose.material3.Surface import androidx.compose.material3.Text-@OptIn(ExperimentalFoundationApi::class) @Composable fun TrackDetailsComposable(Also applies to: 17-17
93-107: Consider adding visual feedback for clickable title.The title is clickable but provides no visual indication to users. Consider using
Modifier.clickable(indication = ...)with a ripple effect or another visual cue to improve discoverability.💡 Optional enhancement
Modifier .fillMaxWidth() .padding(horizontal = 24.dp) - .clickable(onClick = onTitleClick), + .clickable( + onClick = onTitleClick, + indication = androidx.compose.material.ripple.rememberRipple(), + interactionSource = remember { MutableInteractionSource() } + ),Note: You'll need to add imports for
rememberRippleandMutableInteractionSource.
138-177: Consider adding visual feedback for clickable chapter area.Similar to the title, the chapter area is clickable but lacks visual feedback. Consider adding ripple or another indication to improve discoverability.
app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/GlobalPlayerBottomSheet.kt (4)
94-94: Consider making the mini player height dynamic or using a constant.The hardcoded
66.dppadding might not accurately match the actual mini player height, especially with different screen densities or if the mini player design changes. This could cause layout inconsistencies.♻️ Suggested approach
Define a constant or measure the actual mini player height:
// At file level private val MINI_PLAYER_HEIGHT = 66.dpOr use
onSizeChangedmodifier on the mini player to dynamically measure its height and store it in state.
168-168: Remove unused context variable.The
contextvariable is retrieved but never used, as indicated by the comment above it.♻️ Proposed fix
- val context = androidx.compose.ui.platform.LocalContext.current - org.grakovne.lissen.ui.screens.library.composables.MiniPlayerComposable(
330-335: UseobserveAsStatefor LiveData instead of direct.valueaccess.Lines 330, 331, and 335 access LiveData using
.valuedirectly, which bypasses Compose's state observation system. This means the UI won't recompose when these values change, potentially showing stale chapter position or index.♻️ Proposed fix
At the top of the
PlayerContentfunction, add state observations:val playingBook by playerViewModel.book.observeAsState() val isPlaybackReady by playerViewModel.isPlaybackReady.observeAsState(false) val playingQueueExpanded by playerViewModel.playingQueueExpanded.observeAsState(false) + val totalPosition by playerViewModel.totalPosition.observeAsState(0.0) + val currentChapterIndex by playerViewModel.currentChapterIndex.observeAsState(0)Then update the usage:
ChaptersBottomSheet( book = it, - currentPosition = playerViewModel.totalPosition.value ?: 0.0, - currentChapterIndex = playerViewModel.currentChapterIndex.value ?: 0, + currentPosition = totalPosition, + currentChapterIndex = currentChapterIndex, isOnline = isOnline, cachingModelView = cachingModelView, onChapterSelected = { chapter -> - val currentChapterIndex = playerViewModel.currentChapterIndex.value + val chapterIndex = currentChapterIndex - val index = it.chapters.indexOf(chapter) + val index = it.chapters.indexOf(chapter) - if (index == currentChapterIndex) { + if (index == chapterIndex) { playerViewModel.togglePlayPause()
314-315: Remove redundant null check.
playingBookis already verified as non-null in the condition on line 314, making theplayingBook?.leton line 315 redundant.♻️ Proposed fix
if (playingBook != null && isPlaybackReady) { - playingBook?.let { + playingBook.let { NavigationBarComposable(Or more idiomatically, restructure to use a single
playingBook?.let:- if (playingBook != null && isPlaybackReady) { - playingBook?.let { + if (isPlaybackReady) { + playingBook?.let { NavigationBarComposable(app/src/main/res/values/strings.xml (1)
193-203: LGTM! Clear and accessible strings.All format specifiers are correct, and the strings provide clear descriptions for accessibility. The use of "percent" instead of "%" in line 193 is good for screen reader accessibility.
One minor style observation: Line 199 uses "Next/previous" with lowercase "p". While consistent with sentence case, "Next/Previous" with capital "P" is also common for UI labels. This is purely stylistic.
app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/NavigationBarComposable.kt (2)
197-216: LGTM: PlayerActionItem is well-implemented.The composable correctly handles clickable interactions with proper state management. If this pattern is reused in other player screens, consider extracting it to a separate file for consistency.
218-230: Consider relocating the extension function.The
PlaybackVolumeBoost.toItemextension is correctly implemented but is defined in a specific composable file. If volume boost UI is used in other screens (e.g., settings), consider moving this extension to a shared UI utilities file for better reusability.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
app/build.gradle.ktsapp/src/main/kotlin/org/grakovne/lissen/playback/MediaRepository.ktapp/src/main/kotlin/org/grakovne/lissen/ui/screens/details/BookDetailScreen.ktapp/src/main/kotlin/org/grakovne/lissen/ui/screens/library/composables/MiniPlayerComposable.ktapp/src/main/kotlin/org/grakovne/lissen/ui/screens/player/GlobalPlayerBottomSheet.ktapp/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/ChaptersBottomSheet.ktapp/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/NavigationBarComposable.ktapp/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/PlaylistItemComposable.ktapp/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/TrackControlComposable.ktapp/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/TrackDetailsComposable.ktapp/src/main/kotlin/org/grakovne/lissen/viewmodel/PlayerViewModel.ktapp/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/PlaylistItemComposable.kt
🧰 Additional context used
🧬 Code graph analysis (7)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/ChaptersBottomSheet.kt (1)
app/src/main/kotlin/org/grakovne/lissen/ui/effects/WindowBlurEffect.kt (1)
WindowBlurEffect(11-21)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/GlobalPlayerBottomSheet.kt (4)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/TrackDetailsComposable.kt (1)
TrackDetailsComposable(45-179)app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/TrackControlComposable.kt (1)
TrackControlComposable(49-225)app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/NavigationBarComposable.kt (1)
NavigationBarComposable(56-194)app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/ChaptersBottomSheet.kt (1)
ChaptersBottomSheet(51-125)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/TrackControlComposable.kt (2)
app/src/main/kotlin/org/grakovne/lissen/common/HapticAction.kt (1)
withHaptic(6-12)app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/common/SeekButton.kt (1)
SeekButton(23-61)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/details/BookDetailScreen.kt (5)
app/src/main/kotlin/org/grakovne/lissen/ui/components/DownloadProgressIcon.kt (1)
DownloadProgressIcon(25-59)app/src/main/kotlin/org/grakovne/lissen/ui/components/AsyncShimmeringImage.kt (1)
AsyncShimmeringImage(21-65)app/src/main/kotlin/org/grakovne/lissen/common/HapticAction.kt (1)
withHaptic(6-12)app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/PlaylistItemComposable.kt (1)
PlaylistItemComposable(37-156)app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/DownloadsComposable.kt (1)
DownloadsComposable(35-176)
app/src/main/kotlin/org/grakovne/lissen/playback/MediaRepository.kt (1)
app/src/main/kotlin/org/grakovne/lissen/viewmodel/PlayerViewModel.kt (1)
setChapter(125-131)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/NavigationBarComposable.kt (3)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/PlaybackSpeedComposable.kt (1)
PlaybackSpeedComposable(37-132)app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/TimerComposable.kt (1)
TimerComposable(37-134)app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/composable/CommonSettingsItemComposable.kt (1)
CommonSettingsItemComposable(33-113)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/library/composables/MiniPlayerComposable.kt (2)
app/src/main/kotlin/org/grakovne/lissen/ui/components/AsyncShimmeringImage.kt (1)
AsyncShimmeringImage(21-65)app/src/main/kotlin/org/grakovne/lissen/common/HapticAction.kt (1)
withHaptic(6-12)
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (26)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/TrackControlComposable.kt (7)
61-61: LGTM!The new state variables are properly integrated. The
showRemainingTimetoggle provides a useful UX pattern for media players, andshowNavButtonseffectively controls navigation button visibility based on user settings.Also applies to: 70-70
126-137: LGTM!The time display toggle implementation is well done. Using
maxOf(0.0, ...)prevents negative values, and the clickable interaction provides an intuitive way to switch between viewing modes.
145-145: Verify the large padding increase.The top padding increased from 6.dp to 48.dp (8x increase). This significant change likely accommodates the clickable time display above, but please verify the visual layout meets design expectations.
166-170: LGTM!The SeekButton integration for rewind is properly implemented, with correct parameters and callback handling.
176-190: LGTM!The Play/Pause button redesign using Surface with proper primary/onPrimary color pairing follows Material Design patterns and provides better visual separation between the background and icon.
192-196: LGTM!The SeekButton integration for forward seek is correctly implemented, mirroring the rewind button with appropriate parameters.
198-221: LGTM!The Next button implementation properly handles boundary conditions with appropriate enabling/disabling logic and visual feedback through conditional tinting.
app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/TrackDetailsComposable.kt (2)
52-53: LGTM: Well-designed click handler API.The addition of optional click handlers with sensible defaults enables parent composables to handle user interactions while keeping the component flexible.
112-126: LGTM: Well-implemented author display.The author section properly handles null/blank cases and uses string resources for localization. The styling and spacing are appropriate.
app/src/main/kotlin/org/grakovne/lissen/viewmodel/PlayerViewModel.kt (2)
28-36: LGTM!The
isOnlineflow correctly combines server availability with the force-cache preference, yieldingfalsewhen force-cache is enabled or the server is unavailable. This provides a clean, reactive way for UI components to adapt to connectivity state.
148-159: LGTM!The new
playBookandfetchBookfunctions provide clear, concise entry points for initiating playback and fetching book data. The optionalchapterIndexparameter inplayBookis a nice touch for flexible navigation.app/src/main/kotlin/org/grakovne/lissen/ui/screens/library/composables/MiniPlayerComposable.kt (2)
69-70: LGTM!The addition of optional
navControllerandonContentClickparameters provides flexible navigation options. The click handler correctly prefersonContentClickwhen provided, otherwise falling back tonavController.showPlayer(). This design allows the mini player to be embedded in different contexts without breaking existing usage.Also applies to: 150-156
186-188: Guard against division by zero in progress calculation.If
totalDurationis zero (e.g., for malformed or empty books), line 188 performs division by zero, resulting inNaN. This should be guarded to prevent rendering issues with the progress indicator.🛡️ Proposed fix
val totalDuration = remember(book) { book.chapters.sumOf { it.duration } } val currentTime = book.progress?.currentTime ?: 0.0 - val progress = if (totalDuration > 0) (currentTime / totalDuration).toFloat() else 0f + val progress = if (totalDuration > 0.0) (currentTime / totalDuration).toFloat() else 0fActually, the code already has a guard! However, the comparison should be
> 0.0(double) instead of> 0(int) for consistency with the double type oftotalDuration. This is a minor nitpick and the current code is functionally correct.Actually, reviewing more carefully, the existing code already checks
if (totalDuration > 0)before dividing. This is correct and safe.app/src/main/kotlin/org/grakovne/lissen/playback/MediaRepository.kt (3)
206-211: LGTM!The
pendingChapterIndexmechanism elegantly handles the case where a specific chapter is requested before playback is ready. Once playback becomes ready, the pending chapter is applied and cleared. This ensures the correct chapter starts playing even when preparation takes time.
287-318: LGTM!The shake-to-reset-timer implementation is well-structured:
resetSleepTimer()reapplies the current timer and plays feedbackplayResetSound()safely handlesToneGeneratorexceptionsupdateShakeDetectorState()correctly starts/stops the detector based on playback stateThe integration is clean and defensive.
374-391: LGTM!The
prepareAndPlaylogic correctly distinguishes between:
- Same book already ready → immediately apply chapter and play
- Different book or not ready → prepare and queue chapter for later
The
pendingChapterIndexfield bridges the gap between preparation and playback readiness.app/src/main/res/values/strings.xml (7)
2-2: LGTM! App name rebranding.The app name has been changed from "Lissen" to "Kahani" with the correct
translatable="false"attribute for proper nouns.
129-129: LGTM! Improved action-oriented text.The change from "Boosted volume" to "Boost volume" is more concise and action-oriented, improving clarity.
158-159: LGTM! Well-formatted pattern strings.The author and narrator pattern strings use correct format specifiers and provide clear, natural-language presentation.
160-169: LGTM! Comprehensive smart rewind feature strings.The smart rewind strings are well-organized with clear titles, descriptions, and sensible threshold/duration options.
170-173: LGTM! Clear action strings.The book detail action strings clearly communicate different playback states with appropriate action verbs.
178-184: LGTM! Well-crafted book detail strings.The book detail strings provide clear labels and appropriate fallbacks ("Unknown") for missing data. The "Read more"/"See less" pattern is standard and user-friendly.
185-192: LGTM! Proper plural resources.The duration plurals follow correct Android plural resource format with appropriate abbreviations. Note that when localizing to other languages, some may require additional plural forms (zero, two, few, many) beyond "one" and "other".
app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/NavigationBarComposable.kt (3)
3-54: LGTM: Imports are clean and necessary.All new imports support the volume boost feature, refactored UI layout, and settings integration.
57-65: LGTM: Signature updated for settings integration.The addition of
settingsViewModelparameter enables volume boost preferences. Ensure all call sites pass the new parameter.
82-194: LGTM: UI refactor is well-structured.The new action-based layout provides a cleaner interface with:
- Dynamic content (showing values when non-default)
- Proper null safety for volume boost preferences
- Consistent modal patterns for all three actions
- Safe enum reverse-lookup using name matching
app/build.gradle.kts
Outdated
| val commitHash = gitCommitHash() | ||
|
|
||
| applicationId = "org.grakovne.lissen" | ||
| applicationId = "org.surjit.lissen_up" |
There was a problem hiding this comment.
Critical: Version code regression and application ID change break upgrade path.
This PR introduces two critical issues:
-
Version code downgrade: Changing from 10800 to 10000 violates Android's requirement that version codes must always increase. Users with version 10800 installed cannot "upgrade" to 10000 - the installation will fail.
-
Application ID change: Changing from
org.grakovne.lissentoorg.surjit.lissen_upcreates an entirely different app identity. This means:- Existing users cannot upgrade; they must uninstall the old app and install this as a new app
- All user data (preferences, progress, downloads) will be lost
- This will appear as a completely separate app in app stores
If this is an intentional fork/rebrand:
- Keep the version code at 10000 (or reset to 1) for the new app identity
- Document this as a breaking change requiring fresh installation
- Consider providing data migration guidance for users
If this is unintentional:
- Revert
applicationIdtoorg.grakovne.lissen - Set
versionCodeto at least 10801 to allow upgrades
Also applies to: 56-57
🤖 Prompt for AI Agents
In @app/build.gradle.kts at line 53, The change introduced both an applicationId
and versionCode regression: restore the original applicationId value (set
applicationId back to "org.grakovne.lissen") and bump the versionCode to at
least 10801 to preserve upgrade path; alternatively, if this is an intentional
new app identity/fork, keep the new applicationId but reset versionCode
appropriately (e.g., 1 or 10000) and add clear documentation in the PR/release
notes stating this is a breaking change requiring users to uninstall the old app
and explaining data migration guidance.
| CoroutineScope(Dispatchers.Main).launch { | ||
| preferences.shakeToResetTimerFlow.collect { enabled -> | ||
| updateShakeDetectorState(enabled, isPlaying.value == true) | ||
| } | ||
| } |
There was a problem hiding this comment.
Unmanaged CoroutineScope remains unresolved.
This issue was previously flagged: the CoroutineScope(Dispatchers.Main).launch created in init lacks lifecycle management. While acceptable for an app-lifetime singleton, the pattern is fragile and the collector will never be cancelled. The previous review recommended using a SupervisorJob-backed scope with explicit cancellation support.
🔧 Recommended fix (from previous review)
+ private val repositoryScope = CoroutineScope(Dispatchers.Main + SupervisorJob())
+
init {
// ... existing init code ...
- CoroutineScope(Dispatchers.Main).launch {
+ repositoryScope.launch {
preferences.shakeToResetTimerFlow.collect { enabled ->
updateShakeDetectorState(enabled, isPlaying.value == true)
}
}
}Additionally, consider exposing a cleanup method:
fun dispose() {
repositoryScope.cancel()
shakeDetector.stop()
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @app/src/main/kotlin/org/grakovne/lissen/playback/MediaRepository.kt around
lines 180 - 184, The anonymous CoroutineScope(Dispatchers.Main).launch used to
collect preferences.shakeToResetTimerFlow should be replaced with a class-scoped
SupervisorJob-backed CoroutineScope (e.g., repositoryScope) so collections can
be cancelled; create repositoryScope = CoroutineScope(SupervisorJob() +
Dispatchers.Main) and use repositoryScope.launch to collect and call
updateShakeDetectorState, and add a public dispose() (or close()) method that
cancels repositoryScope and stops the shake detector (e.g.,
repositoryScope.cancel(); shakeDetector.stop()) to ensure proper lifecycle
management.
| var durationSeconds = | ||
| if (isStarted) { | ||
| (totalDuration - currentPosition).toLong() | ||
| } else { | ||
| totalDuration.toLong() | ||
| } | ||
|
|
||
| val hours = durationSeconds / 3600 | ||
| val minutes = (durationSeconds % 3600) / 60 |
There was a problem hiding this comment.
Prevent negative duration when currentPosition exceeds totalDuration.
If currentPosition exceeds totalDuration (e.g., due to progress sync issues or data inconsistencies), durationSeconds becomes negative, which will cause incorrect plurals and potentially confusing UI text.
🛡️ Proposed fix
var durationSeconds =
if (isStarted) {
- (totalDuration - currentPosition).toLong()
+ maxOf(0L, (totalDuration - currentPosition).toLong())
} else {
totalDuration.toLong()
}📝 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.
| var durationSeconds = | |
| if (isStarted) { | |
| (totalDuration - currentPosition).toLong() | |
| } else { | |
| totalDuration.toLong() | |
| } | |
| val hours = durationSeconds / 3600 | |
| val minutes = (durationSeconds % 3600) / 60 | |
| var durationSeconds = | |
| if (isStarted) { | |
| maxOf(0L, (totalDuration - currentPosition).toLong()) | |
| } else { | |
| totalDuration.toLong() | |
| } | |
| val hours = durationSeconds / 3600 | |
| val minutes = (durationSeconds % 3600) / 60 |
🤖 Prompt for AI Agents
In
@app/src/main/kotlin/org/grakovne/lissen/ui/screens/details/BookDetailScreen.kt
around lines 395 - 403, The calculation of durationSeconds in BookDetailScreen
(variables: isStarted, totalDuration, currentPosition, durationSeconds) can go
negative if currentPosition > totalDuration; change the assignment to clamp to
non-negative (e.g., use maxOf(0L, totalDuration.toLong() -
currentPosition.toLong()) when isStarted) so hours and minutes are derived from
a non-negative duration; ensure the else branch also produces a non-negative
long (totalDuration.toLong()) and update any subsequent uses to rely on the
clamped durationSeconds.
| val chapterStart = chapter.start | ||
| val chapterEnd = chapterStart + chapter.duration | ||
|
|
||
| val progressRaw = (overallPosition - chapterStart) / (chapterEnd - chapterStart) | ||
| val progress = | ||
| when { | ||
| overallPosition >= chapterEnd -> 1f | ||
| overallPosition <= chapterStart -> 0f | ||
| else -> progressRaw.toFloat() | ||
| } |
There was a problem hiding this comment.
Guard against division by zero in chapter progress calculation.
Similar to the issue in ChaptersBottomSheet.kt, if a chapter has zero duration, line 485 performs division by zero. This will result in NaN or Infinity for progressRaw.
🛡️ Proposed fix
val chapterStart = chapter.start
val chapterEnd = chapterStart + chapter.duration
- val progressRaw = (overallPosition - chapterStart) / (chapterEnd - chapterStart)
+ val chapterDuration = chapterEnd - chapterStart
+ val progressRaw = if (chapterDuration > 0.0) {
+ (overallPosition - chapterStart) / chapterDuration
+ } else {
+ 0f
+ }
val progress =
when {
overallPosition >= chapterEnd -> 1f
overallPosition <= chapterStart -> 0f
else -> progressRaw.toFloat()
}📝 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.
| val chapterStart = chapter.start | |
| val chapterEnd = chapterStart + chapter.duration | |
| val progressRaw = (overallPosition - chapterStart) / (chapterEnd - chapterStart) | |
| val progress = | |
| when { | |
| overallPosition >= chapterEnd -> 1f | |
| overallPosition <= chapterStart -> 0f | |
| else -> progressRaw.toFloat() | |
| } | |
| val chapterStart = chapter.start | |
| val chapterEnd = chapterStart + chapter.duration | |
| val chapterDuration = chapterEnd - chapterStart | |
| val progressRaw = if (chapterDuration > 0.0) { | |
| (overallPosition - chapterStart) / chapterDuration | |
| } else { | |
| 0.0 | |
| } | |
| val progress = | |
| when { | |
| overallPosition >= chapterEnd -> 1f | |
| overallPosition <= chapterStart -> 0f | |
| else -> progressRaw.toFloat() | |
| } |
🤖 Prompt for AI Agents
In
@app/src/main/kotlin/org/grakovne/lissen/ui/screens/details/BookDetailScreen.kt
around lines 482 - 491, The progress calculation in BookDetailScreen (variables
chapterStart, chapterEnd, progressRaw, progress) can divide by zero when a
chapter has zero duration; guard against this by checking if chapterEnd ==
chapterStart (or duration <= 0) before computing progressRaw and set progress to
0f (or 1f if overallPosition >= chapterEnd) accordingly; update the when/if
logic to handle the zero-duration case explicitly so no division is performed
and progress is a stable float.
| val preferredPlaybackVolumeBoost by settingsViewModel.preferredPlaybackVolumeBoost.observeAsState() | ||
| val isOnline by playerViewModel.isOnline.collectAsState(initial = false) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for isOnline usage in player composables to verify if it's actually unused
rg -n --type=kt 'isOnline' app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/Repository: SurjitSahoo/lissen-up
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Search for isOnline usage in player composables without file type restriction
rg -n 'isOnline' app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/Repository: SurjitSahoo/lissen-up
Length of output: 1483
🏁 Script executed:
#!/bin/bash
# Examine NavigationBarComposable.kt to see the function signature and usage of isOnline
head -100 app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/NavigationBarComposable.kt | cat -nRepository: SurjitSahoo/lissen-up
Length of output: 5182
🏁 Script executed:
#!/bin/bash
# Check the entire NavigationBarComposable.kt to see if isOnline is used anywhere in the function body
wc -l app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/NavigationBarComposable.ktRepository: SurjitSahoo/lissen-up
Length of output: 164
🏁 Script executed:
#!/bin/bash
# Read the entire NavigationBarComposable.kt file
cat -n app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/NavigationBarComposable.ktRepository: SurjitSahoo/lissen-up
Length of output: 10164
Remove the unused isOnline state collection from line 72.
The isOnline state is collected from playerViewModel but never referenced in the composable body, causing unnecessary recomposition on state changes. This appears to be leftover from refactoring or an incomplete feature.
🤖 Prompt for AI Agents
In
@app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/NavigationBarComposable.kt
around lines 71 - 72, The file collects an unused state via
playerViewModel.isOnline.collectAsState into isOnline, which causes unnecessary
recompositions; remove the line "val isOnline by
playerViewModel.isOnline.collectAsState(initial = false)" from
NavigationBarComposable (and any now-unused imports) and ensure no remaining
references to isOnline exist in the composable body.
| .clip(RoundedCornerShape(8.dp)) | ||
| .shadow(12.dp, RoundedCornerShape(8.dp)), |
There was a problem hiding this comment.
Fix modifier order for proper shadow rendering.
The shadow modifier should be applied before clip for the shadow to render correctly. Currently, the shadow is being clipped, which defeats its purpose.
🎨 Proposed fix
Modifier
.heightIn(max = maxImageHeight)
.aspectRatio(1f)
- .clip(RoundedCornerShape(8.dp))
- .shadow(12.dp, RoundedCornerShape(8.dp)),
+ .shadow(12.dp, RoundedCornerShape(8.dp))
+ .clip(RoundedCornerShape(8.dp)),📝 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.
| .clip(RoundedCornerShape(8.dp)) | |
| .shadow(12.dp, RoundedCornerShape(8.dp)), | |
| .shadow(12.dp, RoundedCornerShape(8.dp)) | |
| .clip(RoundedCornerShape(8.dp)), |
🤖 Prompt for AI Agents
In
@app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/TrackDetailsComposable.kt
around lines 86 - 87, The shadow is being clipped because
.clip(RoundedCornerShape(8.dp)) is applied after .shadow(...); in the composable
where the modifier chain includes .clip(RoundedCornerShape(8.dp)) and
.shadow(12.dp, RoundedCornerShape(8.dp)) (in TrackDetailsComposable), swap the
order so .shadow(...) comes before .clip(...) using the same
RoundedCornerShape(8.dp) to allow the shadow to render outside the clipped
bounds.
| private fun provideChapterNumberTitle( | ||
| currentTrackIndex: Int, | ||
| book: DetailedItem?, | ||
| libraryType: LibraryType, | ||
| context: Context, | ||
| ): String = | ||
| when (libraryType) { | ||
| LibraryType.LIBRARY -> { | ||
| val part = | ||
| context.getString( | ||
| R.string.player_screen_now_playing_title_chapter_of, | ||
| currentTrackIndex + 1, | ||
| book?.chapters?.size ?: "?", | ||
| ) | ||
| val chapterTitle = book?.chapters?.getOrNull(currentTrackIndex)?.title | ||
| if (chapterTitle.isNullOrBlank()) part else "$chapterTitle ($part)" | ||
| } | ||
|
|
||
| LibraryType.PODCAST -> | ||
| context.getString( | ||
| R.string.player_screen_now_playing_title_podcast_of, | ||
| currentTrackIndex + 1, | ||
| book?.chapters?.size ?: "?", | ||
| ) | ||
|
|
||
| LibraryType.UNKNOWN -> | ||
| context.getString( | ||
| R.string.player_screen_now_playing_title_item_of, | ||
| currentTrackIndex + 1, | ||
| book?.chapters?.size ?: "?", | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused dead code.
The function provideChapterNumberTitle is defined but never called. This appears to be leftover code from refactoring. Removing it will improve maintainability and reduce confusion.
🗑️ Proposed cleanup
-
-private fun provideChapterNumberTitle(
- currentTrackIndex: Int,
- book: DetailedItem?,
- libraryType: LibraryType,
- context: Context,
-): String =
- when (libraryType) {
- LibraryType.LIBRARY -> {
- val part =
- context.getString(
- R.string.player_screen_now_playing_title_chapter_of,
- currentTrackIndex + 1,
- book?.chapters?.size ?: "?",
- )
- val chapterTitle = book?.chapters?.getOrNull(currentTrackIndex)?.title
- if (chapterTitle.isNullOrBlank()) part else "$chapterTitle ($part)"
- }
-
- LibraryType.PODCAST ->
- context.getString(
- R.string.player_screen_now_playing_title_podcast_of,
- currentTrackIndex + 1,
- book?.chapters?.size ?: "?",
- )
-
- LibraryType.UNKNOWN ->
- context.getString(
- R.string.player_screen_now_playing_title_item_of,
- currentTrackIndex + 1,
- book?.chapters?.size ?: "?",
- )
- }📝 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.
| private fun provideChapterNumberTitle( | |
| currentTrackIndex: Int, | |
| book: DetailedItem?, | |
| libraryType: LibraryType, | |
| context: Context, | |
| ): String = | |
| when (libraryType) { | |
| LibraryType.LIBRARY -> { | |
| val part = | |
| context.getString( | |
| R.string.player_screen_now_playing_title_chapter_of, | |
| currentTrackIndex + 1, | |
| book?.chapters?.size ?: "?", | |
| ) | |
| val chapterTitle = book?.chapters?.getOrNull(currentTrackIndex)?.title | |
| if (chapterTitle.isNullOrBlank()) part else "$chapterTitle ($part)" | |
| } | |
| LibraryType.PODCAST -> | |
| context.getString( | |
| R.string.player_screen_now_playing_title_podcast_of, | |
| currentTrackIndex + 1, | |
| book?.chapters?.size ?: "?", | |
| ) | |
| LibraryType.UNKNOWN -> | |
| context.getString( | |
| R.string.player_screen_now_playing_title_item_of, | |
| currentTrackIndex + 1, | |
| book?.chapters?.size ?: "?", | |
| ) | |
| } |
🤖 Prompt for AI Agents
In
@app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/TrackDetailsComposable.kt
around lines 210 - 241, The function provideChapterNumberTitle is dead code and
should be removed: delete the entire private fun provideChapterNumberTitle(...)
declaration (including its when branches) from TrackDetailsComposable so there’s
no unused helper left; ensure no remaining references to
provideChapterNumberTitle exist and run a build to verify no usages or imports
depended on it.
| <string name="player_screen_chapter_list_navigation_library">Chapters</string> | ||
| <string name="player_screen_chapter_list_navigation_podcast">Episodes</string> | ||
| <string name="player_screen_chapter_list_navigation_items">Items</string> | ||
| <string name="player_screen_chapter_list_title">Chapters</string> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify usage of player_screen_chapter_list_title vs the old navigation strings
echo "=== Searching for new unified title usage ==="
rg -n 'player_screen_chapter_list_title' --type xml --type kotlin --type java
echo -e "\n=== Searching for old navigation string references ==="
rg -n 'player_screen_chapter_list_navigation_(library|podcast|items)' --type xml --type kotlin --type java
echo -e "\n=== Also checking for the old string names in broader context ==="
rg -n 'player_screen_chapter_list_navigation' --type xml --type kotlin --type javaRepository: SurjitSahoo/lissen-up
Length of output: 13796
🏁 Script executed:
#!/bin/bash
# More comprehensive search for any references to the old strings
echo "=== Searching for ANY reference to navigation_library ==="
rg -n 'navigation_library'
echo -e "\n=== Searching for ANY reference to navigation_podcast ==="
rg -n 'navigation_podcast'
echo -e "\n=== Searching for ANY reference to navigation_items ==="
rg -n 'navigation_items'Repository: SurjitSahoo/lissen-up
Length of output: 6798
Remove unused dead code strings from all localization files.
The new unified title player_screen_chapter_list_title is correctly used in BookDetailScreen.kt and ChaptersBottomSheet.kt. However, the three old context-specific navigation strings (player_screen_chapter_list_navigation_library, player_screen_chapter_list_navigation_podcast, player_screen_chapter_list_navigation_items) at lines 175-177 in strings.xml are completely unused in code and should be removed from the main strings.xml and all 22+ localization files (values-ko, values-fr, values-de, values-zh-rCN, etc.).
🤖 Prompt for AI Agents
In @app/src/main/res/values/strings.xml at line 13, Remove the three dead string
resources player_screen_chapter_list_navigation_library,
player_screen_chapter_list_navigation_podcast, and
player_screen_chapter_list_navigation_items from the main strings.xml and all
localization files (values-*/ folders); before deleting, run a global search to
confirm they have no usages in code (the UI now uses
player_screen_chapter_list_title in BookDetailScreen.kt and
ChaptersBottomSheet.kt), then delete those entries from every localized
strings.xml to keep translations consistent and avoid unused resource warnings.
| <!-- Missing strings required for release build lint check --> | ||
| <string name="player_screen_chapter_list_navigation_library">Chapters</string> | ||
| <string name="player_screen_chapter_list_navigation_items">Items</string> | ||
| <string name="player_screen_chapter_list_navigation_podcast">Episodes</string> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if the "deprecated" strings are actually referenced in code
echo "=== Searching for player_screen_chapter_list_navigation_library usage ==="
rg -n 'player_screen_chapter_list_navigation_library' -C2
echo -e "\n=== Searching for player_screen_chapter_list_navigation_podcast usage ==="
rg -n 'player_screen_chapter_list_navigation_podcast' -C2
echo -e "\n=== Searching for player_screen_chapter_list_navigation_items usage ==="
rg -n 'player_screen_chapter_list_navigation_items' -C2
echo -e "\n=== Searching for player_screen_chapter_list_title usage ==="
rg -n 'player_screen_chapter_list_title' -C2Repository: SurjitSahoo/lissen-up
Length of output: 34484
🏁 Script executed:
#!/bin/bash
echo "=== Searching for player_screen_chapter_list_navigation_library in layout files ==="
rg -n 'player_screen_chapter_list_navigation_library' --type xml
echo -e "\n=== Searching for player_screen_chapter_list_navigation_podcast in layout files ==="
rg -n 'player_screen_chapter_list_navigation_podcast' --type xml
echo -e "\n=== Searching for player_screen_chapter_list_navigation_items in layout files ==="
rg -n 'player_screen_chapter_list_navigation_items' --type xml
echo -e "\n=== Checking for lint configuration files ==="
fd -e 'lint.xml' -o -e 'lint-baseline.xml' -o -e 'lintOptions'
echo -e "\n=== Checking for any build config that mentions release lint ==="
rg -n 'lint' build.gradle* 2>/dev/null | head -20Repository: SurjitSahoo/lissen-up
Length of output: 7123
These strings appear unused in code and should be removed or justified.
The three strings (player_screen_chapter_list_navigation_library, player_screen_chapter_list_navigation_podcast, player_screen_chapter_list_navigation_items) are defined in the main strings.xml and all localization files but have no references in any Kotlin/Java code or layout files. Meanwhile, player_screen_chapter_list_title (line 13) is actively used in BookDetailScreen.kt and ChaptersBottomSheet.kt.
If these strings were genuinely replaced by player_screen_chapter_list_title, they should be removed rather than kept as dead code. If they're truly required for release build lint checks, that requirement should be documented in code or build configuration.
🤖 Prompt for AI Agents
In @app/src/main/res/values/strings.xml around lines 174 - 177, The three string
resources player_screen_chapter_list_navigation_library,
player_screen_chapter_list_navigation_items, and
player_screen_chapter_list_navigation_podcast are dead code (no references) and
should be removed or justified; remove these three entries from strings.xml if
the app now uses player_screen_chapter_list_title (used in BookDetailScreen.kt
and ChaptersBottomSheet.kt), or if they must exist for a lint/build reason, add
a comment in strings.xml and a short note in the build config or a TODO linking
to that rationale so their presence is documented.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/library/LibraryScreen.kt (1)
226-231: Consider refining the auto-scroll trigger condition.The auto-scroll feature triggers when
recentBookstransitions from empty to non-empty and the user is near the top (firstVisibleItemIndex <= 1). This might scroll the user unexpectedly if they're viewing item 1 when recent books update.Consider using
firstVisibleItemIndex == 0for a stricter "at the very top" check, or adding a small delay to avoid jarring scrolls during rapid data updates.♻️ Stricter scroll condition
LaunchedEffect(recentBooks.isNotEmpty()) { - val needsScroll = libraryListState.firstVisibleItemIndex <= 1 + val needsScroll = libraryListState.firstVisibleItemIndex == 0 if (recentBooks.isNotEmpty() && needsScroll) { libraryListState.animateScrollToItem(0) } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/library/LibraryScreen.kt
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (7)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/library/LibraryScreen.kt (7)
81-81: LGTM: Theme-based spacing support.The Spacing import enables consistent use of theme-based spacing tokens throughout the file, improving maintainability.
162-165: LGTM: Simplified and clearer loading logic.The refactored placeholder logic is more straightforward—showing the placeholder only when loading with no content present. This improves code readability while maintaining correct behavior.
189-193: LGTM: Cleaner visibility logic.The simplified
isRecentVisible()function removes unnecessary coupling to network/cache state, making the logic clearer and more maintainable.
352-352: LGTM: Theme-based spacing improves consistency.Replacing hardcoded
16.dpwithSpacing.mdimproves maintainability and ensures consistent spacing across the app.
372-372: LGTM: Consistent spacing token usage.Replacing
20.dpwithSpacing.lgaligns with the theme-based spacing approach.
429-429: LGTM: Theme token alignment.Using
Spacing.sminstead of8.dpmaintains consistency with the spacing system.
207-217: No action needed —checkRefreshNeeded()correctly handlesitemCount = 0.The code explicitly checks for empty content and intentionally uses it to trigger library refresh at startup, which is the correct behavior. When the library is empty, the refresh is necessary to load data.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In @app/src/main/AndroidManifest.xml:
- Line 71: Update the AuthScheme constant in OAuthScheme.kt from the old value
to match the AndroidManifest change: change the const val AuthScheme declaration
to use "kahani" instead of "lissen" so redirect_uri uses kahani://oauth; ensure
any places that read AuthScheme (e.g., code that builds redirect_uri or
registers intent filters) will pick up the new value and note to inform users
that they must re-authenticate after this scheme change.
In @app/src/main/kotlin/org/grakovne/lissen/content/BookRepository.kt:
- Around line 241-248: fetchRecentListenedBooksFlow captures isOffline once, so
downloadedOnly never updates when networkService.isServerAvailable or
preferences.isForceCache change; make it reactive by returning a Flow that
observes those sources (e.g., use networkService.isServerAvailable.asFlow()/map
or combine with preferences.isForceCache as a Flow) and then flatMapLatest to
call localCacheRepository.fetchRecentListenedBooksFlow(libraryId, downloadedOnly
= reactiveValue), keeping the function signature fetchRecentListenedBooksFlow
and using localCacheRepository.fetchRecentListenedBooksFlow as the inner
provider so downloadedOnly updates reactively.
- Around line 54-59: The null check around the Uri returned from
MediaChannel.provideFileUri() is dead because provideFileUri() returns a
non-nullable Uri; in BookRepository (the code block handling
libraryItemId/chapterId) remove the if (uri == null) branch and directly log and
return OperationResult.Success(uri) after calling provideFileUri(), cleaning up
the unreachable OperationResult.Error path and any related dead variables.
- Around line 303-356: syncRepositories has three issues: it passes emptyList()
to providePreferredChannel().startPlayback, sets
PlaybackProgress.currentChapterTime = 0.0, and swallows errors via getOrNull()
and empty onFailure blocks. Fix by: 1) replacing emptyList() with
MimeTypeProvider.getSupportedMimeTypes() when calling startPlayback; 2) compute
currentChapterTime the same way PlaybackSynchronizationService does (use
calculateChapterPosition(book, progress) or equivalent) instead of hardcoding
0.0, or explicitly handle/validate server behavior if recalculation is required;
3) replace getOrNull()/empty onFailure with proper fold/failure handlers that
log issues (Timber.w/e) and handle errors (e.g., skip on fatal failures but log
context including book id, session errors and fetchBook failures) for functions
like providePreferredChannel().fetchBook(id), startPlayback(...).getOrNull(),
and providePreferredChannel().syncProgress(...).
In @app/src/main/kotlin/org/grakovne/lissen/content/LissenMediaProvider.kt:
- Around line 273-285: fetchBookFlow is combining on
networkService.networkStatus (connectivity) but should use server reachability
like fetchRecentListenedBooks; replace networkService.networkStatus with
networkService.isServerAvailable in fetchBookFlow and update the combine lambda
parameter name from isOnline to isServerAvailable so the early-return uses
server availability; also make makeAvailableIfOnline consistent—either switch it
to use networkService.isServerAvailable or change both usages
(fetchRecentListenedBooks and fetchBookFlow) to use
networkService.isNetworkAvailable() so all three methods (fetchBookFlow,
fetchRecentListenedBooks, makeAvailableIfOnline) use the same server/network
availability check.
In
@app/src/main/kotlin/org/grakovne/lissen/playback/service/PlaybackSynchronizationService.kt:
- Around line 185-186: SYNC_INTERVAL_LONG was shortened from 30_000L to 10_000L
which triples sync attempts; revert or make it configurable and validate limits:
change SYNC_INTERVAL_LONG back to 30_000L or expose it as a config/setting (and
update SHORT_SYNC_WINDOW accordingly) so you can test 10/15/20s values without
code changes; keep the existing syncMutex.tryLock() guard and continue calling
syncProgress(), but run extended playback battery tests and backend rate-limit
checks for the chosen interval and document acceptable tradeoffs before
shipping.
In @app/src/main/kotlin/org/grakovne/lissen/viewmodel/LibraryViewModel.kt:
- Around line 80-82: The three mutable fields currentLibraryId, currentOrdering,
and localCacheUpdatedAt are read and updated in checkRefreshNeeded() without
synchronization; introduce a private val refreshCheckMutex =
kotlinx.coroutines.sync.Mutex() and wrap the body of checkRefreshNeeded(...)
with refreshCheckMutex.withLock { ... } so the check-then-update sequence is
atomic across all three variables; update any other suspend callers to respect
the mutex usage and keep the lock scope minimal to only the critical read/update
section.
- Around line 179-182: The onEach block is causing redundant syncLibrary calls
because emissions aren’t deduplicated; add a Flow operator (e.g.,
distinctUntilChanged() or distinctUntilChangedBy { it }) before onEach so the
downstream onEach runs only when the pair (libraryId, downloadedOnly) actually
changes, and keep the existing guard (if (!downloadedOnly && libraryId != null)
syncLibrary(libraryId)) inside onEach; reference the lambda receiving
(libraryId, downloadedOnly) and the syncLibrary(libraryId) call when making this
change.
- Around line 105-107: The coroutine that launches viewModelScope.launch to
collect downloadedOnlyFlow and call refreshRecentListening() is redundant and
causes duplicate updates with the existing collector that subscribes to
bookRepository.fetchRecentListenedBooksFlow() and updates _recentBooks; remove
the viewModelScope.launch { downloadedOnlyFlow.collect {
refreshRecentListening() } } block (or disable its collection) so only the
reactive collection from fetchRecentListenedBooksFlow() drives updates to
_recentBooks and avoid duplicate queries/races.
🧹 Nitpick comments (7)
app/src/main/kotlin/org/grakovne/lissen/viewmodel/LibraryViewModel.kt (2)
200-206: Consider error handling for sync failures.The
syncLibrarymethod performs critical sync operations but doesn't handle failures. IfsyncRepositories()fails, the user won't be notified and data may remain out of sync. Consider logging errors or updating UI state to indicate sync issues.♻️ Proposed enhancement
private fun syncLibrary(libraryId: String) { viewModelScope.launch(Dispatchers.IO) { - bookRepository.syncRepositories() - refreshRecentListening() - defaultPagingSource.value?.invalidate() + try { + bookRepository.syncRepositories() + refreshRecentListening() + defaultPagingSource.value?.invalidate() + } catch (e: Exception) { + Timber.e(e, "Failed to sync library: $libraryId") + // Optionally: update UI state to show sync error + } } }
129-140: Multiple overlapping refresh mechanisms.When the server becomes available, this coroutine triggers sync and refresh operations that overlap with:
- The first init coroutine responding to
downloadedOnlyFlowchanges- The
libraryPager'sonEachblock callingsyncLibraryThis creates redundant work and potential race conditions. Consider consolidating these refresh triggers into a single coordinated mechanism.
app/src/main/kotlin/org/grakovne/lissen/content/LissenMediaProvider.kt (5)
103-121: Consider caching remote search results for consistency.The method correctly implements local-first search, but unlike
fetchBooks,fetchLibraries, and other methods, it doesn't cache remote search results back to the local repository. This means repeated searches for the same query will always hit the remote server if the local cache was initially empty.♻️ Proposed enhancement to cache remote results
return providePreferredChannel() .searchBooks( libraryId = libraryId, query = query, limit = limit, ) + .also { + it.foldAsync( + onSuccess = { books -> localCacheRepository.cacheBooks(books) }, + onFailure = {}, + ) + }
258-271: Consider usingisServerAvailableinstead ofisNetworkAvailablefor consistency.Line 259 checks
networkService.isNetworkAvailable(), but line 213 usesnetworkService.isServerAvailable.value. For marking chapters as streamable, verifying server reachability (not just network connectivity) would prevent marking chapters as available when the server is unreachable.♻️ Proposed enhancement for consistency
private fun makeAvailableIfOnline(book: DetailedItem): DetailedItem { - if (!networkService.isNetworkAvailable()) { + if (!networkService.isServerAvailable.value) { return book }
357-385: Consider error handling and API call volume for sync operation.The sync logic correctly compares timestamps and syncs newer remote data, but:
- Remote fetch errors (line 359) are silently converted to empty list via
getOrNull(), with no logging or error handling- The method fetches full book details for every book with newer remote data (line 379), which could generate many API calls if numerous books need syncing
♻️ Proposed enhancements
Add error logging for remote fetch:
val libraryId = preferences.getPreferredLibrary()?.id ?: return - val remoteRecents = providePreferredChannel().fetchRecentListenedBooks(libraryId).getOrNull() ?: emptyList() + val remoteRecents = providePreferredChannel().fetchRecentListenedBooks(libraryId).getOrNull() + ?: run { + Timber.w("Failed to fetch remote recents for sync, using local only") + emptyList() + }Consider batching book fetches:
for (id in allIds) { val remote = remoteMap[id] val local = localMap[id] val remoteTime = remote?.listenedLastUpdate ?: 0L val localTime = local?.listenedLastUpdate ?: 0L if (remoteTime > localTime) { + Timber.d("Syncing book $id from remote (remote: $remoteTime > local: $localTime)") providePreferredChannel().fetchBook(id).foldAsync( onSuccess = { localCacheRepository.cacheBookMetadata(it) }, - onFailure = {}, + onFailure = { Timber.w("Failed to sync book $id: ${it.error}") }, ) } }
436-441: Clean up exploratory comments for production code.Lines 436-441 contain exploratory thinking ("Always prefer local?", "Or if we just fetched...") rather than clear documentation. For production code, comments should explain what the code does and why, not the thought process during development.
♻️ Proposed comment cleanup
- // Always prefer local progress if available and newer? - // Or if we just fetched from remote (in caller), and remote is newer, we use remote? - // But here we are "syncing FROM local". - // If we seeked offline, local is newer. - // If we seeked online on other device, remote is newer. - // We need timestamp check. - val cachedProgress = cachedBook.progress ?: return detailedItem val channelProgress = detailedItem.progress - // If channel progress is null, use cached. + // Use cached progress if channel has none if (channelProgress == null) return detailedItem.copy(progress = cachedProgress) + // Prefer the progress with the most recent timestamp val updatedProgress =
393-431: LGTM! Progress synchronization logic correctly merges local and remote states.The method properly:
- Fetches local progress data
- Matches items by ID
- Compares timestamps to prefer newer progress
- Preserves remote items while updating with local when appropriate
Minor optimization: Line 427 could use
syncedMap[item.id]instead ofsyncedMap.get(item.id)for more idiomatic Kotlin.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/src/debug/res/values/strings.xmlapp/src/main/AndroidManifest.xmlapp/src/main/kotlin/org/grakovne/lissen/content/BookRepository.ktapp/src/main/kotlin/org/grakovne/lissen/content/LissenMediaProvider.ktapp/src/main/kotlin/org/grakovne/lissen/playback/service/PlaybackSynchronizationService.ktapp/src/main/kotlin/org/grakovne/lissen/viewmodel/LibraryViewModel.ktapp/src/main/res/values/styles.xml
✅ Files skipped from review due to trivial changes (1)
- app/src/debug/res/values/strings.xml
🧰 Additional context used
🧬 Code graph analysis (2)
app/src/main/kotlin/org/grakovne/lissen/content/LissenMediaProvider.kt (1)
app/src/main/kotlin/org/grakovne/lissen/content/BookRepository.kt (3)
providePreferredChannel(426-426)syncFromLocalProgress(364-402)syncFromLocalProgress(404-420)
app/src/main/kotlin/org/grakovne/lissen/content/BookRepository.kt (1)
app/src/main/kotlin/org/grakovne/lissen/content/LissenMediaProvider.kt (4)
providePreferredChannel(465-465)syncFromLocalProgress(393-431)syncFromLocalProgress(433-457)makeAvailableIfOnline(258-271)
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (24)
app/src/main/res/values/styles.xml (1)
3-3: LGTM! Theme rename is clean.The rebranding from Theme.Lissen to Theme.Kahani is straightforward and consistent with the manifest updates.
app/src/main/AndroidManifest.xml (1)
29-29: LGTM! Theme references updated correctly.Both the application and activity themes now consistently reference Theme.Kahani, aligning with the styles.xml changes.
Also applies to: 44-44
app/src/main/kotlin/org/grakovne/lissen/playback/service/PlaybackSynchronizationService.kt (3)
57-60: Clarify the intent behind synchronizing when canceling.The function
cancelSynchronization()now triggers an immediate sync run after canceling the periodic sync job. This seems counterintuitive given the function name. If the intent is to perform a final sync before stopping (e.g., to flush progress to local storage), consider:
- Renaming to something like
stopSynchronization()orcancelPeriodicSync()- Adding a comment explaining why a final sync is performed
- Ensuring this is the desired behavior when canceling
Could you clarify the use case for this change? Is this intended to ensure local progress is saved before stopping playback?
115-115: LGTM: Local-first progress sync.The addition of
syncLocalProgressbefore the server sync aligns well with the offline-first approach mentioned in the PR objectives. This ensures playback progress is persisted locally even if the server sync fails.
190-196: LGTM: Position discontinuity event enhances sync coverage.Adding
Player.EVENT_POSITION_DISCONTINUITYto the sync events ensures that progress is synchronized when the playback position jumps, such as during:
- User seeking (forward/backward)
- Smart rewind activation (mentioned in PR title)
- Programmatic position changes
The
tryLock()mechanism at line 99 appropriately prevents sync storms if the user rapidly seeks or scrubs through the content.app/src/main/kotlin/org/grakovne/lissen/content/BookRepository.kt (5)
66-83: LGTM! Offline-first progress sync correctly prioritizes local cache.The implementation correctly updates local cache first, then syncs to remote. The force-cache mode appropriately allows offline operation by returning success even if remote sync fails.
130-172: LGTM! Cache-first pattern with proper fallback strategy.The implementation correctly prioritizes local cache, falls back to remote on cache miss, and asynchronously caches remote results without blocking the response. The fire-and-forget caching approach is appropriate for performance.
289-301: LGTM! Excellent reactive pattern for network-aware data flow.This method correctly combines local cache flow with network status to reactively update chapter availability. This is the pattern that should be used in
fetchRecentListenedBooksFlowfor consistency.
364-402: LGTM! Timestamp-based progress reconciliation is sound.The implementation correctly reconciles local and remote progress using timestamps, falling back to remote values when timestamps are missing. The logic preserves unsynced remote items while updating those with local changes.
85-92: LGTM! Consistent cache-first patterns across all methods.All methods correctly implement the offline-first strategy with appropriate cache-first patterns and fallback behavior. The implementations are consistent and well-structured.
Also applies to: 94-108, 110-128, 183-199, 201-215, 274-287, 422-426
app/src/main/kotlin/org/grakovne/lissen/viewmodel/LibraryViewModel.kt (3)
44-44: LGTM! Clean dependency injection and reactive offline state.The constructor properly injects
BookRepositoryandNetworkService, anddownloadedOnlyFlowcorrectly combines network availability with force-cache preference to derive offline state reactively.Also applies to: 46-46, 72-78
251-264: LGTM! Clean migration to repository pattern.The method correctly delegates to
BookRepositoryfor data fetching, maintaining proper loading state management.
162-162: LGTM! Consistent repository integration across paging sources.All paging sources correctly use
BookRepositoryand properly thread through thedownloadedOnlyparameter for offline-first behavior.Also applies to: 188-192, 245-245
app/src/main/kotlin/org/grakovne/lissen/content/LissenMediaProvider.kt (11)
4-5: LGTM! Imports support new reactive and network-aware functionality.The Flow/combine imports enable the new
fetchBookFlowmethod, and NetworkService supports offline-first behavior throughout the provider.Also applies to: 11-11
37-37: LGTM! NetworkService dependency enables offline-first behavior.The injected NetworkService is appropriately used throughout the file for network availability checks and reactive network status monitoring.
45-60: LGTM! Local-first approach with proper error handling.The implementation correctly:
- Checks local cache first before remote fallback
- Wraps remote calls in try-catch to handle exceptions
- Returns appropriate error results instead of swallowing errors
This properly addresses the previous review concern about error handling.
82-85: LGTM! New API for local-only progress syncing.This method enables progress synchronization in offline scenarios by bypassing remote sync, which is appropriate for offline-first architecture.
87-101: LGTM! Local-first cover fetching.The method correctly checks the local cache before falling back to the remote provider, consistent with the offline-first pattern.
167-174: LGTM! New sync method for explicit page synchronization.This method appropriately performs remote fetch and local caching. As an explicit sync operation, it correctly requires network availability, with error handling delegated to the caller.
176-192: LGTM! Local-first library fetching with proper caching.The method correctly checks local cache before remote fetch and asynchronously caches remote results, consistent with the offline-first pattern.
210-232: LGTM! Offline-aware recent books fetching.The method correctly implements offline-first behavior:
- Returns local data when offline
- Uses local cache when available online
- Falls back to remote only on cache miss
- Syncs progress from local state
This pattern works well with the separate
syncRepositoriesmethod for background synchronization.
234-256: LGTM! Local-first book fetching with intelligent fallback.The method correctly:
- Uses locally cached detailed data when available
- Fetches fresh data only when local is incomplete
- Syncs progress and caches metadata
- Adjusts chapter availability based on network status
Verify the network availability check consistency in the
makeAvailableIfOnlinemethod - see comment on lines 258-271.
387-391: LGTM! Useful extension for nullable result extraction.The
getOrNullextension simplifies result handling by providing a concise way to extract data or null, improving code readability.
123-165: Critical bug: Remote fetch result is not returned when local cache is empty.When
downloadedOnly=falseand the local cache is empty, lines 151-162 fetch from remote and cache the results, but line 164 still returns the old (empty)localResultinstead of the remote fetch result. This means callers will receive empty data even after a successful remote fetch.🐛 Proposed fix to return remote result
- if (localItems.isEmpty()) { - Timber.d("Local cache miss for page $pageNumber. Fetching from remote.") - return providePreferredChannel() - .fetchBooks( - libraryId = libraryId, - pageSize = pageSize, - pageNumber = pageNumber, - ).also { - it.foldAsync( - onSuccess = { result -> localCacheRepository.cacheBooks(result.items) }, - onFailure = {}, - ) - } - } - - return localResult + if (localItems.isNotEmpty()) { + return localResult + } + + Timber.d("Local cache miss for page $pageNumber. Fetching from remote.") + return providePreferredChannel() + .fetchBooks( + libraryId = libraryId, + pageSize = pageSize, + pageNumber = pageNumber, + ).also { + it.foldAsync( + onSuccess = { result -> localCacheRepository.cacheBooks(result.items) }, + onFailure = {}, + ) + }Likely an incorrect or invalid review comment.
|
|
||
| <data | ||
| android:scheme="lissen" | ||
| android:scheme="kahani" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for any remaining references to "lissen" OAuth scheme
rg -in 'lissen' --type xml --type kt --type java --type md --type gradleRepository: SurjitSahoo/lissen-up
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Check the context in AndroidManifest.xml around the kahani scheme
cat -n app/src/main/AndroidManifest.xml | grep -A 5 -B 5 'kahani'Repository: SurjitSahoo/lissen-up
Length of output: 474
🏁 Script executed:
#!/bin/bash
# Look for OAuth-related configurations or documentation
find . -type f \( -name "*.md" -o -name "*.txt" -o -name "README*" \) | head -20 | xargs grep -l -i 'oauth\|redirect' 2>/dev/null | head -10Repository: SurjitSahoo/lissen-up
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Search for any remaining references to "lissen" with correct file types
rg -in 'lissen' --type xml --type kotlin --type java --type mdRepository: SurjitSahoo/lissen-up
Length of output: 50378
🏁 Script executed:
#!/bin/bash
# Look for OAuth-related code and configuration
rg -in 'oauth|redirect.*uri' --type xml --type kotlin --type java --type md -A 2 -B 2Repository: SurjitSahoo/lissen-up
Length of output: 23846
🏁 Script executed:
#!/bin/bash
# Check build.gradle files and other configuration for OAuth setup
find . -type f -name "*.gradle" -o -name "build.gradle.kts" | xargs grep -i 'oauth\|audiobookshelf' 2>/dev/nullRepository: SurjitSahoo/lissen-up
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Search for any migration-related code or version handling
rg -in 'migration|migrate|scheme.*change' --type kotlin --type java --type xmlRepository: SurjitSahoo/lissen-up
Length of output: 6395
🏁 Script executed:
#!/bin/bash
# Verify the AuthScheme constant value
cat -n app/src/main/kotlin/org/grakovne/lissen/channel/audiobookshelf/common/oauth/OAuthScheme.ktRepository: SurjitSahoo/lissen-up
Length of output: 261
🏁 Script executed:
#!/bin/bash
# Check how AuthScheme is used in the OAuth flow
rg -A 2 -B 2 'AuthScheme.*://' app/src/main/kotlin/org/grakovne/lissen/channel/audiobookshelf/common/api/AudiobookshelfAuthService.ktRepository: SurjitSahoo/lissen-up
Length of output: 384
Update AuthScheme constant to match the AndroidManifest.xml change.
The OAuth scheme was updated in AndroidManifest.xml to kahani://oauth, but the AuthScheme constant in OAuthScheme.kt remains set to "lissen". This causes the OAuth redirect_uri sent to the Audiobookshelf server to be lissen://oauth instead of kahani://oauth, breaking authentication if the server is configured to accept only the new scheme.
Update line 3 in app/src/main/kotlin/org/grakovne/lissen/channel/audiobookshelf/common/oauth/OAuthScheme.kt:
const val AuthScheme = "kahani"Additionally, confirm with users that re-authentication is required after this change.
🤖 Prompt for AI Agents
In @app/src/main/AndroidManifest.xml at line 71, Update the AuthScheme constant
in OAuthScheme.kt from the old value to match the AndroidManifest change: change
the const val AuthScheme declaration to use "kahani" instead of "lissen" so
redirect_uri uses kahani://oauth; ensure any places that read AuthScheme (e.g.,
code that builds redirect_uri or registers intent filters) will pick up the new
value and note to inform users that they must re-authenticate after this scheme
change.
| if (uri == null) { | ||
| OperationResult.Error(OperationError.InternalError, "Remote URI is null") | ||
| } else { | ||
| Timber.d("Providing REMOTE URI for $libraryItemId / $chapterId: $uri") | ||
| OperationResult.Success(uri) | ||
| } |
There was a problem hiding this comment.
Remove dead null check; provideFileUri returns non-nullable Uri.
The null check is unnecessary since MediaChannel.provideFileUri() returns a non-nullable Uri. This was flagged in a previous review but remains unaddressed.
♻️ Proposed fix
return try {
val uri = providePreferredChannel().provideFileUri(libraryItemId, chapterId)
-
- if (uri == null) {
- OperationResult.Error(OperationError.InternalError, "Remote URI is null")
- } else {
- Timber.d("Providing REMOTE URI for $libraryItemId / $chapterId: $uri")
- OperationResult.Success(uri)
- }
+ Timber.d("Providing REMOTE URI for $libraryItemId / $chapterId: $uri")
+ OperationResult.Success(uri)
} catch (e: Exception) {📝 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.
| if (uri == null) { | |
| OperationResult.Error(OperationError.InternalError, "Remote URI is null") | |
| } else { | |
| Timber.d("Providing REMOTE URI for $libraryItemId / $chapterId: $uri") | |
| OperationResult.Success(uri) | |
| } | |
| return try { | |
| val uri = providePreferredChannel().provideFileUri(libraryItemId, chapterId) | |
| Timber.d("Providing REMOTE URI for $libraryItemId / $chapterId: $uri") | |
| OperationResult.Success(uri) | |
| } catch (e: Exception) { |
🤖 Prompt for AI Agents
In @app/src/main/kotlin/org/grakovne/lissen/content/BookRepository.kt around
lines 54 - 59, The null check around the Uri returned from
MediaChannel.provideFileUri() is dead because provideFileUri() returns a
non-nullable Uri; in BookRepository (the code block handling
libraryItemId/chapterId) remove the if (uri == null) branch and directly log and
return OperationResult.Success(uri) after calling provideFileUri(), cleaning up
the unreachable OperationResult.Error path and any related dead variables.
| fun fetchRecentListenedBooksFlow(libraryId: String): Flow<List<RecentBook>> { | ||
| val isOffline = !networkService.isServerAvailable.value || preferences.isForceCache() | ||
|
|
||
| return localCacheRepository.fetchRecentListenedBooksFlow( | ||
| libraryId = libraryId, | ||
| downloadedOnly = isOffline, | ||
| ) | ||
| } |
There was a problem hiding this comment.
isOffline is captured once, preventing reactive updates.
The isOffline variable is evaluated at call time, not reactively. If network status or force-cache settings change after the flow is created, downloadedOnly won't update. This was flagged in a previous review but remains unaddressed. Consider making it reactive similar to fetchBookFlow at lines 289-301.
♻️ Proposed fix for reactive offline status
fun fetchRecentListenedBooksFlow(libraryId: String): Flow<List<RecentBook>> {
- val isOffline = !networkService.isServerAvailable.value || preferences.isForceCache()
-
- return localCacheRepository.fetchRecentListenedBooksFlow(
- libraryId = libraryId,
- downloadedOnly = isOffline,
- )
+ return combine(
+ networkService.isServerAvailable,
+ preferences.forceCacheFlow,
+ ) { isServerAvailable, isForceCache ->
+ !isServerAvailable || isForceCache
+ }.flatMapLatest { downloadedOnly ->
+ localCacheRepository.fetchRecentListenedBooksFlow(
+ libraryId = libraryId,
+ downloadedOnly = downloadedOnly,
+ )
+ }
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @app/src/main/kotlin/org/grakovne/lissen/content/BookRepository.kt around
lines 241 - 248, fetchRecentListenedBooksFlow captures isOffline once, so
downloadedOnly never updates when networkService.isServerAvailable or
preferences.isForceCache change; make it reactive by returning a Flow that
observes those sources (e.g., use networkService.isServerAvailable.asFlow()/map
or combine with preferences.isForceCache as a Flow) and then flatMapLatest to
call localCacheRepository.fetchRecentListenedBooksFlow(libraryId, downloadedOnly
= reactiveValue), keeping the function signature fetchRecentListenedBooksFlow
and using localCacheRepository.fetchRecentListenedBooksFlow as the inner
provider so downloadedOnly updates reactively.
| suspend fun syncRepositories() { | ||
| val libraryId = preferences.getPreferredLibrary()?.id ?: return | ||
| val remoteRecents = providePreferredChannel().fetchRecentListenedBooks(libraryId).getOrNull() ?: emptyList() | ||
| val localRecents = | ||
| localCacheRepository | ||
| .fetchRecentListenedBooks( | ||
| libraryId = libraryId, | ||
| downloadedOnly = false, | ||
| ).getOrNull() ?: emptyList() | ||
|
|
||
| val remoteMap = remoteRecents.associateBy { it.id } | ||
| val localMap = localRecents.associateBy { it.id } | ||
| val allIds = (remoteMap.keys + localMap.keys).toSet() | ||
|
|
||
| for (id in allIds) { | ||
| val remote = remoteMap[id] | ||
| val local = localMap[id] | ||
|
|
||
| val remoteTime = remote?.listenedLastUpdate ?: 0L | ||
| val localTime = local?.listenedLastUpdate ?: 0L | ||
|
|
||
| when { | ||
| remoteTime > localTime -> { | ||
| providePreferredChannel().fetchBook(id).foldAsync( | ||
| onSuccess = { localCacheRepository.cacheBookMetadata(it) }, | ||
| onFailure = {}, | ||
| ) | ||
| } | ||
|
|
||
| localTime > remoteTime -> { | ||
| val book = localCacheRepository.fetchBook(id) ?: continue | ||
| val progress = book.progress ?: continue | ||
|
|
||
| val session = | ||
| providePreferredChannel() | ||
| .startPlayback( | ||
| bookId = id, | ||
| episodeId = book.chapters.firstOrNull()?.id ?: continue, | ||
| supportedMimeTypes = emptyList(), | ||
| deviceId = preferences.getDeviceId(), | ||
| ).getOrNull() ?: continue | ||
|
|
||
| providePreferredChannel().syncProgress( | ||
| sessionId = session.sessionId, | ||
| progress = | ||
| PlaybackProgress( | ||
| currentTotalTime = progress.currentTime, | ||
| currentChapterTime = 0.0, // Server will recalculate based on total time | ||
| ), | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for startPlayback function definition and its usage
rg -n "fun startPlayback" -A 20 --type=kotlinRepository: SurjitSahoo/lissen-up
Length of output: 17227
🏁 Script executed:
# Search for any reference to currentChapterTime to understand its semantics
rg -n "currentChapterTime" --type=kotlin -B 2 -A 2Repository: SurjitSahoo/lissen-up
Length of output: 4591
🏁 Script executed:
# Check error handling patterns in the codebase - look for Timber usage
rg -n "Timber\.(w|e|d)" --type=kotlin | head -20Repository: SurjitSahoo/lissen-up
Length of output: 2998
🏁 Script executed:
# Search for playback progress sync patterns to understand if currentChapterTime = 0.0 is a known pattern
rg -n "PlaybackProgress" --type=kotlin -B 2 -A 2Repository: SurjitSahoo/lissen-up
Length of output: 18167
🏁 Script executed:
# Check if startPlayback validation or logging happens elsewhere
rg -n "startPlayback" --type=kotlin -B 2 -A 5Repository: SurjitSahoo/lissen-up
Length of output: 12223
Fix sync error handling and MIME type handling in syncRepositories().
The bidirectional sync implementation has three confirmed issues:
-
Empty MIME types (line 341): Passing
emptyList()tostartPlaybackis inconsistent with other callers likePlaybackSynchronizationServicewhich useMimeTypeProvider.getSupportedMimeTypes(). Empty MIME types may cause playback session creation to fail or return an invalid session. -
Hardcoded chapter time (line 350): Setting
currentChapterTime = 0.0assumes server-side recalculation, but the normal playback flow inPlaybackSynchronizationServicecalculates this properly withcalculateChapterPosition(). This pattern is inconsistent and risky. -
Silent failure handling (lines 326–327, 343): Using
getOrNull()withcontinueand emptyonFailurehandlers suppresses errors without logging. Add logging for failures and handle them explicitly:
- Use
Timber.w()orTimber.e()for fetch/sync failures - Use actual MIME types from
MimeTypeProvider.getSupportedMimeTypes() - Calculate
currentChapterTimeproperly or handle the server-side dependency explicitly
🤖 Prompt for AI Agents
In @app/src/main/kotlin/org/grakovne/lissen/content/BookRepository.kt around
lines 303 - 356, syncRepositories has three issues: it passes emptyList() to
providePreferredChannel().startPlayback, sets
PlaybackProgress.currentChapterTime = 0.0, and swallows errors via getOrNull()
and empty onFailure blocks. Fix by: 1) replacing emptyList() with
MimeTypeProvider.getSupportedMimeTypes() when calling startPlayback; 2) compute
currentChapterTime the same way PlaybackSynchronizationService does (use
calculateChapterPosition(book, progress) or equivalent) instead of hardcoding
0.0, or explicitly handle/validate server behavior if recalculation is required;
3) replace getOrNull()/empty onFailure with proper fold/failure handlers that
log issues (Timber.w/e) and handle errors (e.g., skip on fatal failures but log
context including book id, session errors and fetchBook failures) for functions
like providePreferredChannel().fetchBook(id), startPlayback(...).getOrNull(),
and providePreferredChannel().syncProgress(...).
| fun fetchBookFlow(bookId: String): Flow<DetailedItem?> = | ||
| localCacheRepository | ||
| .fetchBookFlow(bookId) | ||
| .combine(networkService.networkStatus) { book: DetailedItem?, isOnline: Boolean -> | ||
| if (book == null) return@combine null | ||
|
|
||
| val isAllCached = book.chapters.all { it.available } | ||
| if (!isOnline || isAllCached) return@combine book | ||
|
|
||
| book.copy( | ||
| chapters = book.chapters.map { it.copy(available = true) }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find NetworkService class definition and networkStatus property
find . -type f -name "*.kt" | xargs grep -l "class NetworkService"Repository: SurjitSahoo/lissen-up
Length of output: 131
🏁 Script executed:
# Get the NetworkService file location and examine it
fd -e kt "NetworkService" --type fRepository: SurjitSahoo/lissen-up
Length of output: 129
🏁 Script executed:
# Check lines around 213 in LissenMediaProvider.kt to see isServerAvailable usage
fd -e kt "LissenMediaProvider" --type fRepository: SurjitSahoo/lissen-up
Length of output: 135
🏁 Script executed:
cat -n app/src/main/kotlin/org/grakovne/lissen/common/NetworkService.ktRepository: SurjitSahoo/lissen-up
Length of output: 6793
🏁 Script executed:
sed -n '210,220p' app/src/main/kotlin/org/grakovne/lissen/content/LissenMediaProvider.ktRepository: SurjitSahoo/lissen-up
Length of output: 478
🏁 Script executed:
# Also look for makeAvailableIfOnline method for comparison
rg -A 15 "fun makeAvailableIfOnline" app/src/main/kotlin/org/grakovne/lissen/content/LissenMediaProvider.ktRepository: SurjitSahoo/lissen-up
Length of output: 498
Use consistent server availability checks across the class.
The fetchBookFlow method uses networkService.networkStatus (network connectivity), but this differs from fetchRecentListenedBooks (line 213), which correctly checks isServerAvailable (server reachability). The networkStatus reflects only whether the device has internet—not whether the server is actually reachable. If the server is down but network is available, this method will mark chapters as available but subsequent operations will fail.
For consistency and correctness, use isServerAvailable instead of networkStatus, aligning with the approach at line 213:
fun fetchBookFlow(bookId: String): Flow<DetailedItem?> =
localCacheRepository
.fetchBookFlow(bookId)
.combine(networkService.isServerAvailable) { book: DetailedItem?, isServerAvailable: Boolean ->
if (book == null) return@combine null
val isAllCached = book.chapters.all { it.available }
if (!isServerAvailable || isAllCached) return@combine book
book.copy(
chapters = book.chapters.map { it.copy(available = true) },
)
}Also align makeAvailableIfOnline to use isNetworkAvailable() consistently or update both methods to use the same availability check.
🤖 Prompt for AI Agents
In @app/src/main/kotlin/org/grakovne/lissen/content/LissenMediaProvider.kt
around lines 273 - 285, fetchBookFlow is combining on
networkService.networkStatus (connectivity) but should use server reachability
like fetchRecentListenedBooks; replace networkService.networkStatus with
networkService.isServerAvailable in fetchBookFlow and update the combine lambda
parameter name from isOnline to isServerAvailable so the early-return uses
server availability; also make makeAvailableIfOnline consistent—either switch it
to use networkService.isServerAvailable or change both usages
(fetchRecentListenedBooks and fetchBookFlow) to use
networkService.isNetworkAvailable() so all three methods (fetchBookFlow,
fetchRecentListenedBooks, makeAvailableIfOnline) use the same server/network
availability check.
| private const val SYNC_INTERVAL_LONG = 10_000L | ||
| private const val SHORT_SYNC_WINDOW = SYNC_INTERVAL_LONG * 2 - 1 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "PlaybackSynchronizationService.kt"Repository: SurjitSahoo/lissen-up
Length of output: 157
🏁 Script executed:
wc -l app/src/main/kotlin/org/grakovne/lissen/playback/service/PlaybackSynchronizationService.ktRepository: SurjitSahoo/lissen-up
Length of output: 159
🏁 Script executed:
cat -n app/src/main/kotlin/org/grakovne/lissen/playback/service/PlaybackSynchronizationService.kt | sed -n '1,50p'Repository: SurjitSahoo/lissen-up
Length of output: 2038
🏁 Script executed:
cat -n app/src/main/kotlin/org/grakovne/lissen/playback/service/PlaybackSynchronizationService.kt | sed -n '60,75p'Repository: SurjitSahoo/lissen-up
Length of output: 615
🏁 Script executed:
cat -n app/src/main/kotlin/org/grakovne/lissen/playback/service/PlaybackSynchronizationService.kt | sed -n '180,220p'Repository: SurjitSahoo/lissen-up
Length of output: 713
🏁 Script executed:
git log --oneline -n 20 -- app/src/main/kotlin/org/grakovne/lissen/playback/service/PlaybackSynchronizationService.ktRepository: SurjitSahoo/lissen-up
Length of output: 113
🏁 Script executed:
git show HEAD:app/src/main/kotlin/org/grakovne/lissen/playback/service/PlaybackSynchronizationService.kt | grep -n "SYNC_INTERVAL_LONG\|SHORT_SYNC_WINDOW" | head -10Repository: SurjitSahoo/lissen-up
Length of output: 427
🏁 Script executed:
git diff HEAD~1 HEAD -- app/src/main/kotlin/org/grakovne/lissen/playback/service/PlaybackSynchronizationService.ktRepository: SurjitSahoo/lissen-up
Length of output: 95
🏁 Script executed:
cat -n app/src/main/kotlin/org/grakovne/lissen/playback/service/PlaybackSynchronizationService.kt | sed -n '62,90p'Repository: SurjitSahoo/lissen-up
Length of output: 1099
🏁 Script executed:
git log --all --oneline -- app/src/main/kotlin/org/grakovne/lissen/playback/service/PlaybackSynchronizationService.kt | head -10Repository: SurjitSahoo/lissen-up
Length of output: 292
🏁 Script executed:
git show HEAD~1:app/src/main/kotlin/org/grakovne/lissen/playback/service/PlaybackSynchronizationService.kt 2>/dev/null | grep -n "SYNC_INTERVAL_LONG\|SHORT_SYNC_WINDOW\|SYNC_INTERVAL_SHORT" || echo "Previous version not found or file did not exist"Repository: SurjitSahoo/lissen-up
Length of output: 113
🏁 Script executed:
git show 8de8332:app/src/main/kotlin/org/grakovne/lissen/playback/service/PlaybackSynchronizationService.kt 2>/dev/null | grep -n "SYNC_INTERVAL_LONG\|SHORT_SYNC_WINDOW\|SYNC_INTERVAL_SHORT" || echo "Not found in commit 8de8332"Repository: SurjitSahoo/lissen-up
Length of output: 538
🏁 Script executed:
git show ac0829a:app/src/main/kotlin/org/grakovne/lissen/playback/service/PlaybackSynchronizationService.kt 2>/dev/null | grep -n "SYNC_INTERVAL_LONG\|SHORT_SYNC_WINDOW\|SYNC_INTERVAL_SHORT" || echo "Not found in commit ac0829a"Repository: SurjitSahoo/lissen-up
Length of output: 538
🏁 Script executed:
cat -n app/src/main/kotlin/org/grakovne/lissen/playback/service/PlaybackSynchronizationService.kt | sed -n '90,150p'Repository: SurjitSahoo/lissen-up
Length of output: 2415
🏁 Script executed:
cat -n app/src/main/kotlin/org/grakovne/lissen/playback/service/PlaybackSynchronizationService.kt | sed -n '100,145p'Repository: SurjitSahoo/lissen-up
Length of output: 1808
Verify the performance impact of tripling sync frequency.
The SYNC_INTERVAL_LONG has been reduced from 30 seconds to 10 seconds, which triples the background sync frequency during playback. Combined with the immediate sync triggered on each player event, this creates a more aggressive sync schedule.
While the syncMutex.tryLock() pattern prevents sync operations from queueing up (overlapping syncs are skipped rather than accumulated), this still results in 3× more frequent sync attempts and I/O operations:
- Local storage: More frequent progress writes
- Network: 3× more API calls to
syncProgress() - Battery: Increased power consumption from frequent I/O and network activity
Immediate event-based syncing already handles most critical scenarios (seeks, play/pause, chapter transitions), so verify whether the reduced interval is necessary.
Please confirm:
- Has this been tested for battery impact during extended playback sessions?
- Is 10-second granularity necessary, or would a longer interval (15–20 seconds) provide an acceptable balance?
- Have backend rate limits or quota considerations been validated for this sync frequency?
🤖 Prompt for AI Agents
In
@app/src/main/kotlin/org/grakovne/lissen/playback/service/PlaybackSynchronizationService.kt
around lines 185 - 186, SYNC_INTERVAL_LONG was shortened from 30_000L to 10_000L
which triples sync attempts; revert or make it configurable and validate limits:
change SYNC_INTERVAL_LONG back to 30_000L or expose it as a config/setting (and
update SHORT_SYNC_WINDOW accordingly) so you can test 10/15/20s values without
code changes; keep the existing syncMutex.tryLock() guard and continue calling
syncProgress(), but run extended playback battery tests and backend rate-limit
checks for the chosen interval and document acceptable tradeoffs before
shipping.
| private var currentLibraryId = "" | ||
| private var currentOrdering = LibraryOrderingConfiguration.default | ||
| private var localCacheUpdatedAt = 0L |
There was a problem hiding this comment.
Race condition: Unsynchronized access to mutable state.
The variables currentLibraryId, currentOrdering, and localCacheUpdatedAt are accessed with a check-then-update pattern in checkRefreshNeeded() without synchronization. This was flagged in a previous review but remains unaddressed. Concurrent calls could lead to lost updates and inconsistent state.
🔒 Recommended fix
+@Volatile private var currentLibraryId = ""
+@Volatile private var currentOrdering = LibraryOrderingConfiguration.default
+@Volatile private var localCacheUpdatedAt = 0L
-private var currentLibraryId = ""
-private var currentOrdering = LibraryOrderingConfiguration.default
-private var localCacheUpdatedAt = 0LFor atomic consistency across all three variables, also add:
private val refreshCheckMutex = kotlinx.coroutines.sync.Mutex()
suspend fun checkRefreshNeeded(...) {
refreshCheckMutex.withLock {
// existing logic
}
}Also applies to: 84-102
🤖 Prompt for AI Agents
In @app/src/main/kotlin/org/grakovne/lissen/viewmodel/LibraryViewModel.kt around
lines 80 - 82, The three mutable fields currentLibraryId, currentOrdering, and
localCacheUpdatedAt are read and updated in checkRefreshNeeded() without
synchronization; introduce a private val refreshCheckMutex =
kotlinx.coroutines.sync.Mutex() and wrap the body of checkRefreshNeeded(...)
with refreshCheckMutex.withLock { ... } so the check-then-update sequence is
atomic across all three variables; update any other suspend callers to respect
the mutex usage and keep the lock scope minimal to only the critical read/update
section.
| viewModelScope.launch { | ||
| downloadedOnlyFlow.collect { refreshRecentListening() } | ||
| } |
There was a problem hiding this comment.
Redundant refresh path creates duplicate updates.
This coroutine triggers refreshRecentListening() on every downloadedOnlyFlow emission, while the second init coroutine (lines 109-127) already reactively collects from bookRepository.fetchRecentListenedBooksFlow(). Both update _recentBooks, causing redundant queries and potential race conditions. This was flagged in a previous review but remains unaddressed.
♻️ Recommended fix
- viewModelScope.launch {
- downloadedOnlyFlow.collect { refreshRecentListening() }
- }
-
viewModelScope.launch {
combine(
preferences.preferredLibraryIdFlow,The second coroutine already handles reactive updates when downloadedOnly changes.
📝 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.
| viewModelScope.launch { | |
| downloadedOnlyFlow.collect { refreshRecentListening() } | |
| } | |
| viewModelScope.launch { | |
| combine( | |
| preferences.preferredLibraryIdFlow, |
🤖 Prompt for AI Agents
In @app/src/main/kotlin/org/grakovne/lissen/viewmodel/LibraryViewModel.kt around
lines 105 - 107, The coroutine that launches viewModelScope.launch to collect
downloadedOnlyFlow and call refreshRecentListening() is redundant and causes
duplicate updates with the existing collector that subscribes to
bookRepository.fetchRecentListenedBooksFlow() and updates _recentBooks; remove
the viewModelScope.launch { downloadedOnlyFlow.collect {
refreshRecentListening() } } block (or disable its collection) so only the
reactive collection from fetchRecentListenedBooksFlow() drives updates to
_recentBooks and avoid duplicate queries/races.
| }.onEach { (libraryId, downloadedOnly) -> | ||
| if (!downloadedOnly && libraryId != null) { | ||
| syncLibrary(libraryId) | ||
| } |
There was a problem hiding this comment.
Excessive sync operations without deduplication.
The onEach block calls syncLibrary() on every emission without checking if values changed. Combined with the lack of distinctUntilChanged, this causes redundant syncs and potential network thrashing. This was flagged in a previous review but remains unaddressed.
♻️ Proposed fix
private val libraryPager: Flow<PagingData<Book>> =
combine(
preferences.preferredLibraryIdFlow,
downloadedOnlyFlow,
) { libraryId, downloadedOnly ->
Pair(libraryId, downloadedOnly)
- }.onEach { (libraryId, downloadedOnly) ->
+ }.distinctUntilChanged()
+ .onEach { (libraryId, downloadedOnly) ->
if (!downloadedOnly && libraryId != null) {
syncLibrary(libraryId)
}📝 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.
| }.onEach { (libraryId, downloadedOnly) -> | |
| if (!downloadedOnly && libraryId != null) { | |
| syncLibrary(libraryId) | |
| } | |
| }.distinctUntilChanged() | |
| .onEach { (libraryId, downloadedOnly) -> | |
| if (!downloadedOnly && libraryId != null) { | |
| syncLibrary(libraryId) | |
| } |
🤖 Prompt for AI Agents
In @app/src/main/kotlin/org/grakovne/lissen/viewmodel/LibraryViewModel.kt around
lines 179 - 182, The onEach block is causing redundant syncLibrary calls because
emissions aren’t deduplicated; add a Flow operator (e.g., distinctUntilChanged()
or distinctUntilChangedBy { it }) before onEach so the downstream onEach runs
only when the pair (libraryId, downloadedOnly) actually changes, and keep the
existing guard (if (!downloadedOnly && libraryId != null)
syncLibrary(libraryId)) inside onEach; reference the lambda receiving
(libraryId, downloadedOnly) and the syncLibrary(libraryId) call when making this
change.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/composable/LicenseFooterComposable.kt (1)
53-71: String concatenation is acceptable but consider a single format pattern.The copyright footer correctly uses string resources and the bullet-separated concatenation is readable. However, for improved localization flexibility, consider using a single format string pattern instead.
This would allow translators to reorder components if needed for different locales.
♻️ Alternative approach using a single format pattern
In
strings.xml, add:<string name="settings_screen_footer_full">%1$s • %2$s • %3$s</string>Then simplify the Text:
Text( modifier = Modifier .fillMaxWidth() .padding(top = 4.dp), - text = - "${stringResource(R.string.settings_screen_footer_copyright_original)} • ${ - stringResource( - R.string.settings_screen_footer_copyright_fork, - ) - } • ${stringResource(R.string.settings_screen_footer_license)}", + text = + stringResource( + R.string.settings_screen_footer_full, + stringResource(R.string.settings_screen_footer_copyright_original), + stringResource(R.string.settings_screen_footer_copyright_fork), + stringResource(R.string.settings_screen_footer_license), + ), style = TextStyle( fontFamily = FontFamily.Monospace, textAlign = TextAlign.Center, fontSize = 10.sp, color = colorScheme.onSurface.copy(alpha = 0.4f), ), )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/debug/res/values/strings.xmlapp/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/composable/LicenseFooterComposable.ktapp/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/debug/res/values/strings.xml
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (4)
app/src/main/res/values/strings.xml (3)
2-3: LGTM! App rebranding properly implemented.The app name and branding updates from "Lissen" to "Kahani" are correctly defined with
translatable="false"attributes.
159-208: LGTM! New feature strings are well-structured.The new strings for book details, smart rewind settings, playback preferences, and footer information follow proper Android resource conventions with correct placeholder syntax and plural forms.
176-178: These navigation strings are unused in code but marked as lint check requirements—verify if they're still needed.These strings (
player_screen_chapter_list_navigation_library,player_screen_chapter_list_navigation_items,player_screen_chapter_list_navigation_podcast) are not referenced anywhere in the codebase and are explicitly marked as "Missing strings required for release build lint check" (line 175). Unlikeplayer_screen_chapter_list_title(line 14), which is actively used in BookDetailScreen.kt and ChaptersBottomSheet.kt, these navigation strings serve a different purpose and are genuinely unused.They are maintained across 17+ localization files, indicating they're part of ongoing translation efforts. If the lint check requirement is no longer valid, removing these strings (from all locales) would clean up unused string resources. Otherwise, consider documenting why these strings are necessary.
app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/composable/LicenseFooterComposable.kt (1)
37-50: LGTM! Proper localization implemented for app name and version.The first Text block correctly uses
stringResourcewith the pattern format, making the app name and version localizable and maintainable.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @.github/workflows/release.yml:
- Around line 3-10: The workflow is currently triggered on pull_request which
causes releases for PRs; remove the "pull_request" entry under the "on:" block
in release.yml (or alternatively create a separate PR-build workflow) and ensure
the release creation step is gated with a condition such as using if:
github.event_name != 'pull_request' so releases only run on actual
pushes/dispatches; you can also switch PR builds to a separate workflow that
uploads artifacts instead of creating GitHub releases.
- Line 42: Replace the deprecated softprops/action-gh-release@v1 reference with
a pinned, supported ref: either update to the current maintained major (e.g.,
softprops/action-gh-release@v2 if available) or pin to a specific commit SHA by
replacing uses: softprops/action-gh-release@v1 with uses:
softprops/action-gh-release@<commit-sha> (fetch the latest stable commit from
the action repo), ensuring the workflow uses a fixed, non-deprecated ref for
security and stability.
In @app/build.gradle.kts:
- Around line 53-57: The PR lowers versionCode and changes applicationId, which
will break updates; restore versionCode to a value >= the previous production
value (e.g., set versionCode to at least 10800 or bump to a new higher integer)
in the build script where versionCode is set, and only change applicationId
(org.surjit.kahani) if you intend this to be a new app—if so, document that this
is a deliberate rebranding and verify users must reinstall and a data migration
plan exists; otherwise revert applicationId to the original
(org.grakovne.lissen) to preserve update continuity.
🧹 Nitpick comments (1)
.github/workflows/release.yml (1)
45-46: Consider semantic versioning for releases.Using
github.run_numberfor release tags (e.g.,v123) doesn't follow semantic versioning and makes it difficult to understand the significance of each release. Consider using the actual app version frombuild.gradle.kts.📋 Proposed improvement
Option 1: Extract version from gradle.properties
Create
gradle.properties:versionName=1.0.0 versionCode=10000Update workflow:
+ - name: Extract Version + id: version + run: | + VERSION=$(grep "versionName=" gradle.properties | cut -d'=' -f2) + echo "version=$VERSION" >> $GITHUB_OUTPUT - name: Create Release uses: softprops/action-gh-release@v2 with: files: app/build/outputs/apk/release/app-release.apk - tag_name: v${{ github.run_number }} - name: Release v${{ github.run_number }} + tag_name: v${{ steps.version.outputs.version }} + name: Release v${{ steps.version.outputs.version }} body: | Automated Release for Kahani. Commit: ${{ github.sha }}Option 2: Use git tags
Only trigger releases when a version tag is pushed:
on: push: - branches: - - main + tags: + - 'v*.*.*' workflow_dispatch:
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/release.ymlapp/build.gradle.kts
🧰 Additional context used
🪛 actionlint (1.7.10)
.github/workflows/release.yml
42-42: the runner of "softprops/action-gh-release@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ 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). (2)
- GitHub Check: Build Signed APK
- GitHub Check: build
🔇 Additional comments (2)
app/build.gradle.kts (1)
93-93: Good practice: minification enabled for release builds.Enabling
isMinifyEnabled = trueis a best practice that reduces APK size and adds code obfuscation. The ProGuard rules are properly configured for Gson. Make sure to test the release build thoroughly to catch any ProGuard-related issues..github/workflows/release.yml (1)
29-39: No issue to address. The keystore path resolution is correct as-is.The keystore decoded to
app/kahani-release.jksis correctly located by Gradle'sfile()function inapp/build.gradle.kts. When Gradle callsfile("kahani-release.jks")from a module build script, it resolves relative to that module's directory (app/), resulting in the correct pathapp/kahani-release.jksfrom the repository root. The current configuration matches the keystore location without requiring changes.Likely an incorrect or invalid review comment.
| RELEASE_KEY_PASSWORD: ${{ secrets.RELEASE_KEY_PASSWORD }} | ||
|
|
||
| - name: Create Release | ||
| uses: softprops/action-gh-release@v1 |
There was a problem hiding this comment.
Update to latest action version.
The softprops/action-gh-release@v1 action is outdated. The v1 runner is deprecated on GitHub Actions.
⬆️ Proposed fix
- name: Create Release
- uses: softprops/action-gh-release@v1
+ uses: softprops/action-gh-release@v2
with:
files: app/build/outputs/apk/release/app-release.apkFor better security and stability, consider pinning to a specific commit SHA:
- uses: softprops/action-gh-release@v1
+ uses: softprops/action-gh-release@c062e08bd532815e2082a85e87e3ef29c3e6d191 # v2.0.4Based on static analysis hints.
📝 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.
| uses: softprops/action-gh-release@v1 | |
| - name: Create Release | |
| uses: softprops/action-gh-release@v2 | |
| with: | |
| files: app/build/outputs/apk/release/app-release.apk |
🧰 Tools
🪛 actionlint (1.7.10)
42-42: the runner of "softprops/action-gh-release@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
In @.github/workflows/release.yml at line 42, Replace the deprecated
softprops/action-gh-release@v1 reference with a pinned, supported ref: either
update to the current maintained major (e.g., softprops/action-gh-release@v2 if
available) or pin to a specific commit SHA by replacing uses:
softprops/action-gh-release@v1 with uses:
softprops/action-gh-release@<commit-sha> (fetch the latest stable commit from
the action repo), ensuring the workflow uses a fixed, non-deprecated ref for
security and stability.
app/build.gradle.kts
Outdated
| applicationId = "org.surjit.kahani" | ||
| minSdk = 28 | ||
| targetSdk = 36 | ||
| versionCode = 10800 | ||
| versionName = "1.8.0-$commitHash" | ||
| versionCode = 10000 | ||
| versionName = "1.0.0-${commitHash}" |
There was a problem hiding this comment.
Critical: versionCode regression will break app updates.
The versionCode has been decreased from 10800 to 10000. Android requires versionCode to monotonically increase for app updates to work. Users with version 10800 installed will not be able to update to 10000.
Additionally, the applicationId change from org.grakovne.lissen to org.surjit.kahani means this will be treated as a completely new app by app stores, not an update to the existing app. Existing users will need to uninstall and reinstall.
🔧 Proposed fix
- versionCode = 10000
+ versionCode = 10900If this is intentional rebranding as a new app, please confirm that:
- This is not meant to be an update to an existing app in production
- Users understand they need to reinstall
- Data migration strategy is in place
📝 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.
| applicationId = "org.surjit.kahani" | |
| minSdk = 28 | |
| targetSdk = 36 | |
| versionCode = 10800 | |
| versionName = "1.8.0-$commitHash" | |
| versionCode = 10000 | |
| versionName = "1.0.0-${commitHash}" | |
| applicationId = "org.surjit.kahani" | |
| minSdk = 28 | |
| targetSdk = 36 | |
| versionCode = 10900 | |
| versionName = "1.0.0-${commitHash}" |
🤖 Prompt for AI Agents
In @app/build.gradle.kts around lines 53 - 57, The PR lowers versionCode and
changes applicationId, which will break updates; restore versionCode to a value
>= the previous production value (e.g., set versionCode to at least 10800 or
bump to a new higher integer) in the build script where versionCode is set, and
only change applicationId (org.surjit.kahani) if you intend this to be a new
app—if so, document that this is a deliberate rebranding and verify users must
reinstall and a data migration plan exists; otherwise revert applicationId to
the original (org.grakovne.lissen) to preserve update continuity.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/build.gradle.kts (1)
63-67: Hardcoded ACRA credentials pose a security risk.The ACRA report credentials are hardcoded in plain text and will be visible in the repository history. Move these to
local.propertiesor use environment variables/secrets.🔐 Proposed fix
- val acraReportLogin = "8yJ59n0UToCja8LR" - val acraReportPassword = "kuW9TV7BbJByuIAc" + val acraReportLogin = System.getenv("ACRA_REPORT_LOGIN") + ?: localProperties.getProperty("ACRA_REPORT_LOGIN") + ?: "" + val acraReportPassword = System.getenv("ACRA_REPORT_PASSWORD") + ?: localProperties.getProperty("ACRA_REPORT_PASSWORD") + ?: ""
🤖 Fix all issues with AI agents
In @.github/workflows/auto-version-bump.yml:
- Around line 42-43: The workflow references a missing script
scripts/bump_version.py which causes NEW_VERSION=$(python3
scripts/bump_version.py ${{ steps.get_version.outputs.current_version }}
$BUMP_TYPE) to fail; either add a new executable scripts/bump_version.py that
accepts (current_version, bump_type) and prints the bumped semantic version to
stdout (so NEW_VERSION captures it) or update the workflow to call the correct
existing script path/name; ensure the script returns non-zero on errors and
prints only the new version string for the NEW_VERSION assignment to work.
♻️ Duplicate comments (2)
.github/workflows/release.yml (1)
38-39: Update to softprops/action-gh-release@v2.The
softprops/action-gh-release@v1runner is deprecated. Static analysis confirms: "the runner of 'softprops/action-gh-release@v1' action is too old to run on GitHub Actions."🔧 Proposed fix
- name: Create Release - uses: softprops/action-gh-release@v1 + uses: softprops/action-gh-release@v2 with:app/build.gradle.kts (1)
69-92: Release builds will fail when signing credentials are missing.The signing configuration can have
storeFile = null(line 77), but line 92 unconditionally applies this config. Local development builds without keystore setup will fail with cryptic errors.🛡️ Proposed fix with conditional signing
buildTypes { release { - signingConfig = signingConfigs.getByName("release") + signingConfig = signingConfigs.findByName("release")?.takeIf { + it.storeFile != null + } isMinifyEnabled = true
| NEW_VERSION=$(python3 scripts/bump_version.py ${{ steps.get_version.outputs.current_version }} $BUMP_TYPE) | ||
| echo "new_version=$NEW_VERSION" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the bump_version.py script exists
if [ -f "scripts/bump_version.py" ]; then
echo "Script found. Contents:"
cat scripts/bump_version.py
else
echo "ERROR: scripts/bump_version.py not found"
fd -t f "bump_version"
fiRepository: SurjitSahoo/kahani-android
Length of output: 110
🏁 Script executed:
#!/bin/bash
# Broader search for any bump_version related files
fd -i "bump" || echo "No files with 'bump' in name found"
echo "---"
# Also search in the workflow file to understand context
cat .github/workflows/auto-version-bump.yml | head -50Repository: SurjitSahoo/kahani-android
Length of output: 1579
Create the missing scripts/bump_version.py script or correct the script path.
The workflow fails at line 42 because scripts/bump_version.py does not exist in the repository. This script is essential for the version bumping logic and must be implemented or the path must be corrected before this workflow can function.
🤖 Prompt for AI Agents
In @.github/workflows/auto-version-bump.yml around lines 42 - 43, The workflow
references a missing script scripts/bump_version.py which causes
NEW_VERSION=$(python3 scripts/bump_version.py ${{
steps.get_version.outputs.current_version }} $BUMP_TYPE) to fail; either add a
new executable scripts/bump_version.py that accepts (current_version, bump_type)
and prints the bumped semantic version to stdout (so NEW_VERSION captures it) or
update the workflow to call the correct existing script path/name; ensure the
script returns non-zero on errors and prints only the new version string for the
NEW_VERSION assignment to work.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @.github/workflows/release.yml:
- Around line 3-8: Add an explicit workflow-level permissions block granting
write access to repository contents: add a top-level permissions entry with
contents: write so the release creation action (softprops/action-gh-release@v1)
has the required token scope; update the workflow YAML that currently starts
with "on:" to include permissions: { contents: write } at the top level.
In
`@app/schemas/org.grakovne.lissen.content.cache.persistent.LocalCacheStorage/14.json`:
- Around line 53-58: The detailed_books table and BookEntity declare duration as
INTEGER/Int while book_files and book_chapters use REAL/Double, causing schema
inconsistency; update BookEntity.duration from Int to Double (and any
corresponding DTO/mapper/types) and change the detailed_books schema/column
definition for "duration" (fieldPath "duration", columnName "duration") from
INTEGER to REAL so all three entities (BookEntity, BookFileEntity,
BookChapterEntity) consistently use Double, and adjust any affected
serialization/migration code accordingly.
In `@scripts/bump_version.py`:
- Around line 18-34: The update_gradle_properties function currently silently
does nothing if appVersionName or appVersionCode are missing; change it to
detect and fail-fast (raise an exception or exit non-zero) when the regexes
don't match, or explicitly insert the missing keys before writing. Specifically,
after reading content, verify presence of 'appVersionName=' (use the same regex
r'appVersionName=.*') and 'appVersionCode=(\d+)' (used for version_code_match);
if either check fails, raise a clear error (including which key is missing) or
append the appropriate key(s) with sensible defaults, then proceed to perform
the substitution/increment and write back to path. Ensure
update_gradle_properties reports failures instead of exiting silently.
- Around line 5-16: In bump_version ensure inputs are validated: check
current_version matches a three-part dot-separated integer pattern before
calling map(int, ...), and raise a clear ValueError (e.g., "Invalid version
format: ...") if not; also validate bump_type is one of 'major', 'minor', or
'patch' and raise a ValueError (e.g., "Invalid bump_type: ...") for any other
value so the function doesn't silently return an unchanged version; keep the
existing increment logic for major/minor/patch inside bump_version and reference
the variables major, minor, patch when performing the checks and raises.
♻️ Duplicate comments (1)
.github/workflows/release.yml (1)
42-42: Update deprecatedsoftprops/action-gh-release@v1.Line 42 still uses the deprecated v1 release action; please upgrade to v2 or pin to a stable SHA.
What is the latest stable version (or commit SHA) of softprops/action-gh-release, and is v1 deprecated on GitHub Actions?⬆️ Proposed update
- uses: softprops/action-gh-release@v1 + uses: softprops/action-gh-release@v2
🧹 Nitpick comments (2)
app/schemas/org.grakovne.lissen.content.cache.persistent.LocalCacheStorage/14.json (2)
285-294: Redundant index on primary key column.The
index_media_progress_bookIdindex is unnecessary becausebookIdis already the primary key (line 279-284). Room automatically creates a unique index on primary key columns, so this additional non-unique index adds overhead without benefit.This should be removed from the corresponding entity annotation in your Kotlin code.
59-63: Consider adding foreign key constraint forlibraryId.The
detailed_books.libraryIdcolumn (lines 59-63) references thelibrariestable conceptually, but lacks a foreign key constraint. Other relationships (book_files,book_chapters,media_progress) properly enforce referential integrity with CASCADE deletes.If a library is deleted, books referencing that
libraryIdcould become orphaned. If this is intentional (soft reference for offline scenarios), consider documenting it. Otherwise, add a foreign key with appropriateON DELETEbehavior (e.g.,SET NULLorCASCADE).Also applies to: 310-339
| on: | ||
| push: | ||
| tags: | ||
| - 'v*' | ||
| workflow_dispatch: | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat .github/workflows/release.ymlRepository: SurjitSahoo/kahani-android
Length of output: 1511
Add explicit permissions: contents: write for release creation robustness.
The workflow uses softprops/action-gh-release@v1 to create releases, which requires write access to repository contents. Without explicit permissions configured at the workflow level, the action may fail with 403 errors if the repository's default token permissions are read-only. Adding this permission ensures the workflow succeeds in restrictive environments.
🔧 Proposed fix
on:
push:
tags:
- 'v*'
workflow_dispatch:
+
+permissions:
+ contents: write📝 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.
| on: | |
| push: | |
| tags: | |
| - 'v*' | |
| workflow_dispatch: | |
| on: | |
| push: | |
| tags: | |
| - 'v*' | |
| workflow_dispatch: | |
| permissions: | |
| contents: write |
🤖 Prompt for AI Agents
In @.github/workflows/release.yml around lines 3 - 8, Add an explicit
workflow-level permissions block granting write access to repository contents:
add a top-level permissions entry with contents: write so the release creation
action (softprops/action-gh-release@v1) has the required token scope; update the
workflow YAML that currently starts with "on:" to include permissions: {
contents: write } at the top level.
| { | ||
| "fieldPath": "duration", | ||
| "columnName": "duration", | ||
| "affinity": "INTEGER", | ||
| "notNull": true | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the schema file to verify the column types at the specified lines
echo "=== Schema file: detailed_books duration (around line 56) ==="
cat -n app/schemas/org.grakovne.lissen.content.cache.persistent.LocalCacheStorage/14.json | sed -n '50,65p'
echo ""
echo "=== Schema file: book_files duration (around line 119) ==="
cat -n app/schemas/org.grakovne.lissen.content.cache.persistent.LocalCacheStorage/14.json | sed -n '115,125p'
echo ""
echo "=== Schema file: book_chapters duration (around line 185) ==="
cat -n app/schemas/org.grakovne.lissen.content.cache.persistent.LocalCacheStorage/14.json | sed -n '181,190p'Repository: SurjitSahoo/kahani-android
Length of output: 1489
🏁 Script executed:
# Now check entity class definitions for duration field types
echo "=== Checking entity class duration field definitions ==="
find . -type f -name "*Entity.kt" | xargs grep -l "duration" 2>/dev/null | head -5Repository: SurjitSahoo/kahani-android
Length of output: 220
🏁 Script executed:
# Search for Book class and related entity definitions
echo "=== Searching for Book and related entity definitions ==="
rg -n "class.*Book.*Entity" --type kotlin -A 10 | head -50Repository: SurjitSahoo/kahani-android
Length of output: 5601
🏁 Script executed:
# Check the actual duration field type in entities
echo "=== Searching for duration field declarations ==="
rg -n "duration\s*:" --type kotlin -B 2 -A 1Repository: SurjitSahoo/kahani-android
Length of output: 5636
Inconsistent duration column types across database tables.
The schema shows:
detailed_books.duration:INTEGERbook_files.duration:REALbook_chapters.duration:REAL
The entity classes reveal the source: BookEntity (detailed_books) declares duration: Int, while BookFileEntity and BookChapterEntity both declare duration: Double. Align BookEntity.duration to Double to match the other entities and maintain schema consistency.
🤖 Prompt for AI Agents
In
`@app/schemas/org.grakovne.lissen.content.cache.persistent.LocalCacheStorage/14.json`
around lines 53 - 58, The detailed_books table and BookEntity declare duration
as INTEGER/Int while book_files and book_chapters use REAL/Double, causing
schema inconsistency; update BookEntity.duration from Int to Double (and any
corresponding DTO/mapper/types) and change the detailed_books schema/column
definition for "duration" (fieldPath "duration", columnName "duration") from
INTEGER to REAL so all three entities (BookEntity, BookFileEntity,
BookChapterEntity) consistently use Double, and adjust any affected
serialization/migration code accordingly.
| def bump_version(current_version, bump_type): | ||
| major, minor, patch = map(int, current_version.split('.')) | ||
| if bump_type == 'major': | ||
| major += 1 | ||
| minor = 0 | ||
| patch = 0 | ||
| elif bump_type == 'minor': | ||
| minor += 1 | ||
| patch = 0 | ||
| elif bump_type == 'patch': | ||
| patch += 1 | ||
| return f"{major}.{minor}.{patch}" |
There was a problem hiding this comment.
Validate bump type and version format before computing.
Right now an invalid bump_type silently keeps the version unchanged, and malformed versions crash with an unhelpful stack trace. That can lead to tagging the wrong version in CI. Add explicit validation and raise a clear error.
✅ Suggested fix (validation inside bump_version)
def bump_version(current_version, bump_type):
- major, minor, patch = map(int, current_version.split('.'))
+ match = re.fullmatch(r'(\d+)\.(\d+)\.(\d+)', current_version)
+ if not match:
+ raise ValueError(f"Invalid version format: {current_version}. अपेक्षित format: X.Y.Z")
+ if bump_type not in {"major", "minor", "patch"}:
+ raise ValueError(f"Invalid bump type: {bump_type}. Use major|minor|patch.")
+ major, minor, patch = map(int, match.groups())
if bump_type == 'major':
major += 1
minor = 0
patch = 0
elif bump_type == 'minor':
minor += 1
patch = 0
elif bump_type == 'patch':
patch += 1
return f"{major}.{minor}.{patch}"🤖 Prompt for AI Agents
In `@scripts/bump_version.py` around lines 5 - 16, In bump_version ensure inputs
are validated: check current_version matches a three-part dot-separated integer
pattern before calling map(int, ...), and raise a clear ValueError (e.g.,
"Invalid version format: ...") if not; also validate bump_type is one of
'major', 'minor', or 'patch' and raise a ValueError (e.g., "Invalid bump_type:
...") for any other value so the function doesn't silently return an unchanged
version; keep the existing increment logic for major/minor/patch inside
bump_version and reference the variables major, minor, patch when performing the
checks and raises.
| def update_gradle_properties(new_version): | ||
| path = 'gradle.properties' | ||
| with open(path, 'r') as f: | ||
| content = f.read() | ||
|
|
||
| # Update appVersionName | ||
| content = re.sub(r'appVersionName=.*', f'appVersionName={new_version}', content) | ||
|
|
||
| # Update appVersionCode (increment by 1) | ||
| version_code_match = re.search(r'appVersionCode=(\d+)', content) | ||
| if version_code_match: | ||
| old_code = int(version_code_match.group(1)) | ||
| new_code = old_code + 1 | ||
| content = re.sub(r'appVersionCode=\d+', f'appVersionCode={new_code}', content) | ||
|
|
||
| with open(path, 'w') as f: | ||
| f.write(content) |
There was a problem hiding this comment.
Fail fast if appVersionName / appVersionCode keys are missing.
If those keys are absent or renamed, the script will still print a new version and exit successfully, leaving gradle.properties unchanged. Consider enforcing replacements (or explicitly adding keys) to avoid silent desync.
✅ Suggested fix (anchor + validate replacements)
def update_gradle_properties(new_version):
path = 'gradle.properties'
with open(path, 'r') as f:
content = f.read()
# Update appVersionName
- content = re.sub(r'appVersionName=.*', f'appVersionName={new_version}', content)
+ name_re = re.compile(r'^appVersionName=.*$', re.MULTILINE)
+ content, name_count = name_re.subn(f'appVersionName={new_version}', content)
+ if name_count == 0:
+ raise ValueError("appVersionName not found in gradle.properties")
# Update appVersionCode (increment by 1)
- version_code_match = re.search(r'appVersionCode=(\d+)', content)
+ version_code_match = re.search(r'^appVersionCode=(\d+)$', content, re.MULTILINE)
if version_code_match:
old_code = int(version_code_match.group(1))
new_code = old_code + 1
- content = re.sub(r'appVersionCode=\d+', f'appVersionCode={new_code}', content)
+ content = re.sub(r'^appVersionCode=\d+$', f'appVersionCode={new_code}', content, flags=re.MULTILINE)
+ else:
+ raise ValueError("appVersionCode not found in gradle.properties")🤖 Prompt for AI Agents
In `@scripts/bump_version.py` around lines 18 - 34, The update_gradle_properties
function currently silently does nothing if appVersionName or appVersionCode are
missing; change it to detect and fail-fast (raise an exception or exit non-zero)
when the regexes don't match, or explicitly insert the missing keys before
writing. Specifically, after reading content, verify presence of
'appVersionName=' (use the same regex r'appVersionName=.*') and
'appVersionCode=(\d+)' (used for version_code_match); if either check fails,
raise a clear error (including which key is missing) or append the appropriate
key(s) with sensible defaults, then proceed to perform the
substitution/increment and write back to path. Ensure update_gradle_properties
reports failures instead of exiting silently.
1 participant
Summary by CodeRabbit
New Features
Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.