Conversation
… enhance cache isolation by host and user, and update cache clearing functionality.
…east one cached chapter.
…ook/file sizes, download progress, individual file deletion, and a polished empty state.
…ted and atomic content caching, including UI and database schema updates.
…d badge with a checkmark icon, and ensure reactive UI updates for caching progress.
…, improved animation, and robust progress calculation.
… strings from multiple locales.
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughBumps Room schema to v19, adds per-host/user scoping and file size, implements migrations, computes disk/volume mappings, rewrites caching to channelFlow with buffered copy and progress, integrates Microsoft Clarity telemetry and consent UI, and updates DAOs, repositories, viewmodels, and many composables. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Downloads UI
participant CCM as ContentCachingManager
participant Repo as LocalCacheRepository
participant DB as RoomDB
participant FS as FileSystem
participant Tracker as ClarityTracker
UI->>CCM: request cache(bookId, downloadOption)
CCM->>Repo: compute fileStartTimes & requested chapters
CCM->>Tracker: trackEvent("download_start")
CCM->>FS: copy file (buffered) with progress callbacks
FS-->>CCM: per-file progress updates
CCM->>DB: persist media progress / metadata
CCM->>UI: emit progress events (per-file / overall)
CCM->>Tracker: trackEvent("download_finished")
alt error
CCM->>UI: emit Error
CCM->>Tracker: trackEvent("download_error", "err")
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
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
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
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/content/cache/persistent/OfflineBookStorageProperties.kt (1)
15-24:⚠️ Potential issue | 🟡 MinorEnsure
baseFolder()only returns a writable directory.
takeIf { it.exists() || it.mkdirs() && it.canWrite() }returns an existing but non-writable directory, which can cause downstream write failures now that this API is public. Consider requiringcanWrite()for both existing and newly created directories, and otherwise fall back tocacheDir.Suggested fix
- fun baseFolder(): File = - context - .getExternalFilesDir(MEDIA_CACHE_FOLDER) - ?.takeIf { it.exists() || it.mkdirs() && it.canWrite() } + fun baseFolder(): File = + context + .getExternalFilesDir(MEDIA_CACHE_FOLDER) + ?.takeIf { (it.exists() || it.mkdirs()) && it.canWrite() } ?: context .cacheDir .resolve(MEDIA_CACHE_FOLDER) .apply { if (exists().not()) { mkdirs() } }app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/api/CachedBookRepository.kt (1)
81-118:⚠️ Potential issue | 🔴 CriticalFilter
fetchBooks()results by current host/username to prevent cross-account data collision.
BookEntityhasidas sole primary key;hostandusernameare regular fields. WhencacheBooks()callsfetchBooks(bookIds)without filtering by the current user's host/username, it can retrieve and then overwrite another account's rows viaupsertBooks()withOnConflictStrategy.REPLACE. The method should extract host/username and filter existing books before attempting updates.app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/dao/CachedBookDao.kt (1)
175-193:⚠️ Potential issue | 🟠 MajorAdd host/username filtering to cached-items queries.
The
fetchCachedItemsandfetchCachedItemsCountmethods return cached items across all users/hosts, breaking per-account isolation. Other methods likecountCachedBooksanddeleteNonDownloadedBookscorrectly apply host/username filtering; these should follow the same pattern.Suggested change
`@Query`( """ SELECT * FROM detailed_books WHERE host = :host AND username = :username AND EXISTS (SELECT 1 FROM book_chapters WHERE bookId = detailed_books.id AND isCached = 1) ORDER BY title ASC, libraryId ASC LIMIT :pageSize OFFSET (:pageNumber * :pageSize) """, ) suspend fun fetchCachedItems( pageSize: Int, pageNumber: Int, + host: String, + username: String, ): List<CachedBookEntity> `@Query`( """ SELECT COUNT(*) FROM detailed_books WHERE host = :host AND username = :username AND EXISTS (SELECT 1 FROM book_chapters WHERE bookId = detailed_books.id AND isCached = 1) """, ) suspend fun fetchCachedItemsCount( + host: String, + username: String, ): IntThen update
CachedBookRepository.fetchCachedItemsandfetchCachedItemsto retrieve and pass the current user's host/username.
🤖 Fix all issues with AI agents
In
`@app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/api/CachedLibraryRepository.kt`:
- Around line 18-31: Both cacheLibraries and fetchLibraries currently use
empty-string fallbacks for missing credentials which can merge data across
sessions; change cacheLibraries and fetchLibraries to validate credentials from
preferences.getHost() and preferences.getUsername() and stop when either is
null/blank (e.g., return early from fetchLibraries with an empty List and skip
dao.updateLibraries in cacheLibraries, or throw a clear exception if you prefer
explicit failure). Update the logic around dao.updateLibraries(libraries, host,
username) and dao.fetchLibraries(host, username) to only call DAO methods when
both host and username are non-blank, and keep converter.apply only in the
successful fetch path.
In
`@app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/api/RecentRequestBuilder.kt`:
- Around line 10-21: The query built by RecentRequestBuilder bypasses
host/username isolation: when downloadedOnly is true host/username filters are
omitted and when false the OR EXISTS clause allows other-host/user books to leak
in; update the SQL construction in RecentRequestBuilder to always apply host and
username filters (use the builder fields host and username) and remove the OR
EXISTS bypass so cached-chapter checks are only applied in conjunction with
matching host/username; ensure this change touches the downloadedOnly-related
branch and the branch using the OR EXISTS clause so both enforce per-host/user
filtering.
In
`@app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/ContentCachingService.kt`:
- Around line 116-131: The current logic skips calling finish() when work is no
longer in progress but errors remain; update the condition around finish() so
finish() is invoked whenever inProgress() returns false (regardless of
hasErrors()), and ensure error notification logic still runs (e.g., call
notificationService.updateCachingNotification(...) for executionStatuses when
errors exist before or after calling finish()). Locate the block using
inProgress(), hasErrors(), finish(), executionStatuses and
notificationService.updateCachingNotification and move or add the finish() call
out of the "!hasErrors()" guard so the service always calls finish() once no
items are in progress.
In
`@app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/dao/CachedLibraryDao.kt`:
- Around line 35-43: The fetchLibrary function currently queries only by ID and
must be scoped to tenant context; update suspend fun fetchLibrary(libraryId:
String): CachedLibraryEntity? to accept host: String and username: String
parameters and change its `@Query` to "SELECT * FROM libraries WHERE id =
:libraryId AND host = :host AND username = :username"; do the same for the other
similar method around lines 48-53 (add host and username params and include them
in the WHERE clause), and then update any DAO call sites to pass host/username
so the DAO enforces multi-tenant scoping (refer to fetchLibraries and
deleteLibrariesExcept for the pattern).
In
`@app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/LocalCacheModule.kt`:
- Around line 48-53: produceMigration15_16 currently injects
preferences.getHost() and preferences.getUsername() directly into SQL DEFAULT
clauses (e.g., DEFAULT '$host'), which risks SQL injection and is fragile since
ALTER TABLE doesn’t accept bind parameters; change the migration to avoid
embedding user values into DDL: create the new/altered tables with neutral
defaults (no interpolated strings) inside produceMigration15_16, then after the
schema change run a separate parameterized INSERT/UPDATE statement (using bound
parameters) to persist host/username into the appropriate rows, using
preferences.getHost()/getUsername() as parameters; update any callers of
produceMigration15_16 and ensure MIGRATION_16_17 is applied after this safe
population step.
In
`@app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/Migrations.kt`:
- Around line 267-281: The migration in produceMigration15_16 interpolates host
and username directly into SQL, risking syntax errors or injection when values
contain single quotes; update produceMigration15_16 (inside its migrate
override) to escape single quotes in host and username (e.g., replace ' with '')
before building the ALTER TABLE ... ADD COLUMN ... DEFAULT 'value' strings and
then use those escaped literals for the three tables (detailed_books,
media_progress, libraries) so the DEFAULT clauses are valid SQLite string
literals.
In `@app/src/main/kotlin/org/grakovne/lissen/content/LissenMediaProvider.kt`:
- Around line 238-241: The current isDetailed predicate treats any zero-sized
file as not detailed even when chapters exist; change the logic in the
isDetailed computation (the val isDetailed using localResult) to short-circuit
on chapters: if localResult?.chapters.isNotEmpty() then isDetailed = true;
otherwise, when chapters are empty, require files.isNotEmpty() and files.all { f
-> f.size > 0 } to mark detailed. Update the expression around localResult?.let
{ ... } (referencing localResult, chapters, files) so file-size checks only
apply when chapters are absent.
In
`@app/src/main/kotlin/org/grakovne/lissen/ui/screens/common/DownloadOptionFormat.kt`:
- Around line 19-22: The first is-check for
org.grakovne.lissen.lib.domain.SpecificFilesDownloadOption duplicates a later
identical branch and uses hard-coded labels; remove the duplicate branch and
replace both with a single SpecificFilesDownloadOption handling that inspects
the instance's fileIds.size to choose between the plural/singular localized
resources (e.g., use context.getString(R.string.download_selected_volume) vs
context.getString(R.string.download_multiple_files) or appropriate
string/plurals), and add the referenced string resources to strings.xml instead
of embedding "Selected Volume"/"Multiple Files".
In
`@app/src/main/kotlin/org/grakovne/lissen/ui/screens/details/BookDetailScreen.kt`:
- Around line 582-586: The local remembers in the downloads sheet use
remember(book.id) so cachingModelView.getVolumes(book) and
getBookStorageType(book) won't update when cacheVersion changes; update the
remember keys to include cacheVersion (e.g., remember(book.id, cacheVersion)) or
simply remove the local volumes and storageType and reference the top-level
volumes (which is already keyed by cacheVersion) and the appropriate storage
lookup so the downloads sheet reflects cache updates; adjust the remember calls
around getBookStorageType/getVolumes or replace them with the existing top-level
volumes variable.
In
`@app/src/main/kotlin/org/grakovne/lissen/ui/screens/migration/MigrationScreen.kt`:
- Around line 33-43: The code currently calls viewModel.startMigration()
unconditionally in LaunchedEffect(Unit), causing re-execution after
configuration changes; change the effects to a single LaunchedEffect keyed on
the observed state (state from viewModel.migrationState.observeAsState) and only
invoke viewModel.startMigration() when the state is the initial/idle state
(e.g., MigrationState.Idle or not MigrationState.Completed), while still calling
onMigrationComplete() when state is MigrationState.Completed; locate usages of
viewModel.startMigration(), MigrationState.Completed, and the state observed via
observeAsState to implement this guard.
In
`@app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/ActionRow.kt`:
- Around line 65-72: The subtextColor is built with a base alpha (alpha = 0.6f)
but later overwritten by copy(alpha = contentAlpha), so change the later copy to
combine alphas instead of replacing them: compute combinedAlpha = 0.6f *
contentAlpha (or read the base alpha from the initial color if extracted) and
use copy(alpha = combinedAlpha) when updating subtextColor; update both
occurrences (the subtextColor initialization block and the later copy at the
other location) so the intended 0.6 opacity is preserved and scaled by
contentAlpha (refer to subtextColor and contentAlpha in ActionRow).
In
`@app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/DownloadsComposable.kt`:
- Around line 127-133: The cached fileStartTimes computed with remember(book.id)
is stale when book.files changes; update the remember call to include book.files
as a key (e.g., remember(book.id, book.files)) so the block that computes
fileStartTimes (using book.files.runningFold(...).dropLast(1).let {
book.files.zip(it) }) recomputes whenever the file list or its metadata change.
In `@app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/PlayerScreen.kt`:
- Line 443: The UI treats only CacheStatus.Caching as an active cache, so queued
downloads don't surface stop/clear actions; update the expression that sets
cachingInProgress (currently using cacheProgress.status) to consider
CacheStatus.Queued as in-progress as well (e.g., check cacheProgress.status is
CacheStatus.Caching || cacheProgress.status is CacheStatus.Queued) so both
queued and caching states enable the stop/clear UI.
- Around line 440-442: The calls to cachingModelView.getBookStorageType(book)
and cachingModelView.getVolumes(book) in PlayerScreen are performing synchronous
filesystem I/O on every recomposition; wrap both calls in remember(book) to
memoize results (as done in BookDetailScreen and NavigationBarComposable) so
getBookStorageType and getVolumes are only computed once per book and avoid
duplicated work (note getVolumes internally calls getBookStorageType, so
memoizing both prevents repeated I/O). Ensure you replace the direct calls with
remember(book) { cachingModelView.getBookStorageType(book) } and remember(book)
{ cachingModelView.getVolumes(book) } usages in the PlayerScreen composable.
In
`@app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/advanced/cache/CachedItemsSettingsScreen.kt`:
- Around line 177-181: The Text composable in CachedItemsSettingsScreen.kt uses
hardcoded "Cancel" and "Edit" strings (tied to selectionMode) which bypass
localization; add two string resources (e.g., R.string.cached_items_cancel and
R.string.cached_items_edit) to strings.xml with the appropriate values, change
the Text call to use stringResource(...) instead of raw literals, and ensure the
file imports androidx.compose.ui.res.stringResource so the composition uses
localized strings.
- Line 638: Replace the unsafe volume.chapters.first() call with a defensive
check: use volume.chapters.firstOrNull(), and if it returns null skip calling
viewModel.dropCache (or call an appropriate overload that doesn't require a
chapter) to avoid NoSuchElementException; update the call site where
viewModel.dropCache(item, ...) is invoked in CachedItemsSettingsScreen (same
area that uses deleteSelectedVolumes) to handle the null chapter case safely.
- Around line 199-208: The haptic call is currently executed after
deleteSelectedVolumes finishes because withHaptic(view) is inside the coroutine;
move the withHaptic invocation to run immediately when the Button is pressed
(before launching the deletion coroutine) so feedback is instantaneous.
Specifically, call withHaptic(view) synchronously at the start of the Button
onClick (or right before scope.launch) and keep selectionMode = false,
selectedVolumes.clear(), and refreshContent(false) inside the coroutine after
deleteSelectedVolumes(viewModel, playerViewModel, cachedItems, selectedVolumes)
as appropriate; update the call sites referencing deleteSelectedVolumes,
withHaptic, scope.launch, selectionMode, selectedVolumes.clear(), and
refreshContent to reflect this change.
In
`@app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/advanced/cache/CacheSettingsScreen.kt`:
- Around line 106-118: Wrap the cache-clear invocation in a try-catch and
surface failures to the user and logs: inside the
AdvancedSettingsNavigationItemComposable onclick lambda, call
viewModel.clearMetadataCache within a try block and on success show the existing
Toast; in the catch block log the exception and show a failure Toast (use
stringResource(R.string.settings_screen_clear_metadata_cache_failure) or a new
failure string), ensuring any exceptions from clearMetadataCache or
cachedCoverProvider.clearCache are handled instead of failing silently;
alternatively, extend viewModel.clearMetadataCache to accept an onError callback
and invoke that when cachedCoverProvider.clearCache throws, then show the
failure Toast from the onclick handler.
In `@app/src/main/kotlin/org/grakovne/lissen/viewmodel/CachingModelView.kt`:
- Around line 161-164: The dropCompletedChapters path updates _cacheVersion but
never refreshes storage stats, so call the ViewModel's storage stats refresh
routine immediately after updating the cache version; specifically, in suspend
fun dropCompletedChapters(item: DetailedItem) (which calls
contentCachingManager.dropCompletedChapters(item) and increments
_cacheVersion.value) invoke the existing method that refreshes storage totals
(e.g., refreshStorageStats() or
loadStorageStats()/storageStatsManager.refreshStats()) right after the
_cacheVersion update so UI totals are updated.
In `@app/src/main/kotlin/org/grakovne/lissen/viewmodel/MigrationViewModel.kt`:
- Around line 48-50: The CURRENT_DATABASE_VERSION constant is out of sync with
the schema/migrations (schema at 17 and MIGRATION_16_17 exists); update
companion object constant CURRENT_DATABASE_VERSION from 16 to 17 so
preferences.setDatabaseVersion(...) writes the correct version and the app's
migration checks match 17.json and MIGRATION_16_17.
🧹 Nitpick comments (10)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/advanced/cache/CachedItemsSettingsScreen.kt (3)
429-429: Unused variableisSingleFileBook.This variable is computed but never referenced anywhere in the composable. It should be removed to avoid confusion.
Suggested fix
- val isSingleFileBook = remember(book) { book.files.size <= 1 } - val bookSize =
609-618: ImportCheckboxinstead of using fully qualified name.Using the fully qualified
androidx.compose.material3.Checkboxis inconsistent with other Material3 components in this file that are imported. Add the import at the top of the file.Suggested fix
Add to imports:
import androidx.compose.material3.CheckboxThen simplify the usage:
if (selectionMode) { - androidx.compose.material3.Checkbox( + Checkbox( checked = isSelected, onCheckedChange = {
666-681: Consider optimizing lookup performance.The current implementation iterates through all cached items for each selected volume (O(n×m) complexity). With many cached items and selections, this could cause UI jank since
calculateReclaimSizeis called during composition.Suggested optimization
private fun calculateReclaimSize( selectedIds: List<VolumeIdentifier>, cachedItems: LazyPagingItems<DetailedItem>, viewModel: CachingModelView, ): Long { + val booksById = (0 until cachedItems.itemCount) + .mapNotNull { cachedItems[it] } + .associateBy { it.id } + var total = 0L selectedIds.forEach { selection -> - val book = (0 until cachedItems.itemCount).mapNotNull { cachedItems[it] }.find { it.id == selection.bookId } + val book = booksById[selection.bookId] book?.let { val volumes = viewModel.getVolumes(it) val volume = volumes.find { v -> v.id == selection.fileId } total += volume?.size ?: 0L } } return total }app/src/main/kotlin/org/grakovne/lissen/persistence/preferences/LissenSharedPreferences.kt (1)
390-394: Log the swallowed exception for debugging purposes.The exception is caught and silently swallowed. While returning
nullis a reasonable fallback for corrupted/outdated JSON data, logging the exception would aid debugging without affecting user experience.♻️ Proposed fix
val adapter = moshi.adapter(DetailedItem::class.java) try { adapter.fromJson(json) } catch (e: Exception) { + android.util.Log.w("LissenSharedPreferences", "Failed to parse playing book", e) null }app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/ContentCachingManager.kt (1)
240-259: Ensure progress reaches 100% when content length is unknown.If
contentLength <= 0,overallProgressnever advances to 1.0, so UI can stay below 100% until completion. Consider emitting a final per‑file update after the loop.♻️ Suggested tweak
while (input.read(buffer).also { bytesRead = it } != -1) { output.write(buffer, 0, bytesRead) totalBytesRead += bytesRead @@ if (overallProgress - lastReportedProgress >= reportThreshold || overallProgress >= 1.0) { onProgress(overallProgress) lastReportedProgress = overallProgress } } + val finalOverallProgress = (index + 1).toDouble() / files.size.toDouble() + if (finalOverallProgress > lastReportedProgress) { + onProgress(finalOverallProgress) + }app/src/main/kotlin/org/grakovne/lissen/channel/audiobookshelf/library/converter/BookResponseConverter.kt (1)
80-89: Guard per‑file Timber logging to avoid production noise.String interpolation runs even when logs are disabled; for large libraries this adds overhead. Consider gating behind a debug flag or removing.
♻️ Example guard
- timber.log.Timber.d("Mapping file ${it.ino} with size ${it.metadata.size}") + if (BuildConfig.DEBUG) { + timber.log.Timber.d("Mapping file ${it.ino} with size ${it.metadata.size}") + }app/src/main/kotlin/org/grakovne/lissen/viewmodel/SettingsViewModel.kt (1)
36-37: Consider using standard import declarations.The fully qualified names are used inline instead of import statements at the top of the file. This is inconsistent with the other imports in this class.
♻️ Suggested refactor
Add these imports at the top of the file with the other imports:
import org.grakovne.lissen.content.BookRepository import org.grakovne.lissen.content.cache.temporary.CachedCoverProviderThen simplify the constructor parameters:
- private val bookRepository: org.grakovne.lissen.content.BookRepository, - private val cachedCoverProvider: org.grakovne.lissen.content.cache.temporary.CachedCoverProvider, + private val bookRepository: BookRepository, + private val cachedCoverProvider: CachedCoverProvider,app/src/main/kotlin/org/grakovne/lissen/viewmodel/MigrationViewModel.kt (1)
41-44: Consider logging the migration failure.The catch block silently proceeds to
Completedstate without logging the exception. While avoiding "bricking" is reasonable, logging the error would help with debugging production issues.♻️ Suggested improvement
+import timber.log.Timber ... } catch (e: Exception) { - // In a real app, we might want to handle this more gracefully + Timber.e(e, "Database migration failed, proceeding anyway") _migrationState.value = MigrationState.Completed // Proceed anyway to avoid bricking }app/src/main/kotlin/org/grakovne/lissen/ui/components/DownloadProgressIcon.kt (1)
40-119: Deduplicate the identical icon rendering branches.
CacheStatus.Completedandelserender the same CloudDone/CloudDownload blocks; consolidating reduces drift and keeps accessibility consistent.♻️ Possible consolidation
- is CacheStatus.Completed -> { - if (isFullyDownloaded) { - Icon( - imageVector = Icons.Filled.CloudDone, - contentDescription = null, - modifier = Modifier.size(size), - tint = colorScheme.primary, - ) - } else { - Icon( - imageVector = Icons.Outlined.CloudDownload, - contentDescription = stringResource(R.string.player_screen_downloads_navigation), - modifier = Modifier.size(size).alpha(0.8f), - tint = color, - ) - } - } - - else -> { - if (isFullyDownloaded) { - Icon( - imageVector = Icons.Filled.CloudDone, - contentDescription = null, - modifier = Modifier.size(size), - tint = colorScheme.primary, - ) - } else { - Icon( - imageVector = Icons.Outlined.CloudDownload, - contentDescription = stringResource(R.string.player_screen_downloads_navigation), - modifier = Modifier.size(size).alpha(0.8f), - tint = color, - ) - } - } + is CacheStatus.Completed, else -> { + val isDone = isFullyDownloaded + val icon = if (isDone) Icons.Filled.CloudDone else Icons.Outlined.CloudDownload + Icon( + imageVector = icon, + contentDescription = if (isDone) null else stringResource(R.string.player_screen_downloads_navigation), + modifier = Modifier.size(size).then(if (isDone) Modifier else Modifier.alpha(0.8f)), + tint = if (isDone) colorScheme.primary else color, + ) + }app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/LocalCacheRepository.kt (1)
166-222: Avoid recalculatingmediaMaptwice for segmented/atomic
getBookStorageTypemay already computemediaMap, andmapChaptersToVolumescomputes it again. Consider a shared helper that returns both storageType and mediaMap to avoid duplicate work.
app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/api/CachedLibraryRepository.kt
Show resolved
Hide resolved
app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/api/RecentRequestBuilder.kt
Outdated
Show resolved
Hide resolved
app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/ContentCachingService.kt
Show resolved
Hide resolved
| @Query("SELECT * FROM libraries WHERE id = :libraryId") | ||
| suspend fun fetchLibrary(libraryId: String): CachedLibraryEntity? | ||
|
|
||
| @Transaction | ||
| @Query("SELECT * FROM libraries") | ||
| suspend fun fetchLibraries(): List<CachedLibraryEntity> | ||
| @Query("SELECT * FROM libraries WHERE host = :host AND username = :username") | ||
| suspend fun fetchLibraries( | ||
| host: String, | ||
| username: String, | ||
| ): List<CachedLibraryEntity> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and read the CachedLibraryDao.kt file
find . -name "CachedLibraryDao.kt" -type fRepository: SurjitSahoo/kahani-android
Length of output: 160
🏁 Script executed:
# Get the size to determine how to read it
wc -l app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/dao/CachedLibraryDao.ktRepository: SurjitSahoo/kahani-android
Length of output: 161
🏁 Script executed:
# Read the entire file to see the context
cat -n app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/dao/CachedLibraryDao.ktRepository: SurjitSahoo/kahani-android
Length of output: 1924
🏁 Script executed:
# Search for all calls to fetchLibrary in the codebase
rg "fetchLibrary" --type kotlin -A 2 -B 2Repository: SurjitSahoo/kahani-android
Length of output: 7563
🏁 Script executed:
# Check the CachedLibraryEntity structure to understand if host/username are part of the entity
find . -name "*CachedLibraryEntity*" -type fRepository: SurjitSahoo/kahani-android
Length of output: 275
🏁 Script executed:
# Read the CachedLibraryEntity to confirm it has host and username fields
cat -n app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/entity/CachedLibraryEntity.ktRepository: SurjitSahoo/kahani-android
Length of output: 751
🏁 Script executed:
# Search for actual call sites of fetchLibrary more carefully - look for calls to the DAO method
rg "\.fetchLibrary\(" --type kotlin -B 3 -A 3Repository: SurjitSahoo/kahani-android
Length of output: 52
🏁 Script executed:
# Also check if there's a repository that wraps this DAO
find . -name "*LibraryRepository*" -o -name "*LibraryDataSource*" | grep -E "\.(kt|java)$"Repository: SurjitSahoo/kahani-android
Length of output: 167
🏁 Script executed:
# Read the CachedLibraryRepository to see how fetchLibrary is used
cat -n app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/api/CachedLibraryRepository.ktRepository: SurjitSahoo/kahani-android
Length of output: 1394
🏁 Script executed:
# Also search for any usage of the DAO method in other files - maybe it's being used indirectly
rg "fetchLibrary" --type kotlin -B 5 -A 5 | grep -A 5 -B 5 "CachedLibraryDao\|libraryDao"Repository: SurjitSahoo/kahani-android
Length of output: 2319
🏁 Script executed:
# Double-check: search for any references to fetchLibrary in the entire codebase including comments and strings
rg "fetchLibrary" --type kotlin --no-headingRepository: SurjitSahoo/kahani-android
Length of output: 1869
🏁 Script executed:
# Also check if this method might be called through reflection or if there are any imports
rg "CachedLibraryDao" --type kotlin -A 10 | head -50Repository: SurjitSahoo/kahani-android
Length of output: 5630
fetchLibrary lacks multi-tenant scoping and risks exposing data across hosts/users.
While other DAO queries filter by host and username, fetchLibrary(libraryId) retrieves by ID alone. This creates a latent cross-tenant data leak risk if the DAO is directly injected. Add host and username parameters to match the scoping pattern of fetchLibraries and deleteLibrariesExcept.
Suggested fix
- `@Query`("SELECT * FROM libraries WHERE id = :libraryId")
- suspend fun fetchLibrary(libraryId: String): CachedLibraryEntity?
+ `@Query`("SELECT * FROM libraries WHERE id = :libraryId AND host = :host AND username = :username")
+ suspend fun fetchLibrary(
+ libraryId: String,
+ host: String,
+ username: String,
+ ): CachedLibraryEntity?Also applies to: 48-53
🤖 Prompt for AI Agents
In
`@app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/dao/CachedLibraryDao.kt`
around lines 35 - 43, The fetchLibrary function currently queries only by ID and
must be scoped to tenant context; update suspend fun fetchLibrary(libraryId:
String): CachedLibraryEntity? to accept host: String and username: String
parameters and change its `@Query` to "SELECT * FROM libraries WHERE id =
:libraryId AND host = :host AND username = :username"; do the same for the other
similar method around lines 48-53 (add host and username params and include them
in the WHERE clause), and then update any DAO call sites to pass host/username
so the DAO enforces multi-tenant scoping (refer to fetchLibraries and
deleteLibrariesExcept for the pattern).
| .addMigrations( | ||
| produceMigration15_16( | ||
| host = preferences.getHost() ?: "", | ||
| username = preferences.getUsername() ?: "", | ||
| ), | ||
| ).addMigrations(MIGRATION_16_17) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the produceMigration15_16 function to understand what it does
rg -n "produceMigration15_16" --type kotlin -A 30Repository: SurjitSahoo/kahani-android
Length of output: 4697
🏁 Script executed:
#!/bin/bash
# Look at the LocalCacheModule.kt file to see the context
cat -n app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/LocalCacheModule.kt | head -60Repository: SurjitSahoo/kahani-android
Length of output: 2374
🏁 Script executed:
#!/bin/bash
# Find DAO files and check how host/username columns are used
fd -e "\.kt$" -path "*/dao/*" app/src/main/kotlin/Repository: SurjitSahoo/kahani-android
Length of output: 238
🏁 Script executed:
#!/bin/bash
# Find DAO files correctly
find app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent -name "*Dao*.kt"Repository: SurjitSahoo/kahani-android
Length of output: 244
🏁 Script executed:
#!/bin/bash
# Search for host and username usage in query methods
rg -n "host|username" app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/ --type kotlin -B 2 -A 2Repository: SurjitSahoo/kahani-android
Length of output: 30749
🏁 Script executed:
#!/bin/bash
# Check if there are any safeguards against SQL injection in preferences or migration
rg -n "getHost|getUsername" --type kotlin app/src/main/kotlin/org/grakovne/lissen/persistence/preferences/ -B 2 -A 2Repository: SurjitSahoo/kahani-android
Length of output: 3500
🏁 Script executed:
#!/bin/bash
# Check if there's any validation or escaping of host/username values
rg -n "class LissenSharedPreferences|fun getHost|fun getUsername" --type kotlin -A 10Repository: SurjitSahoo/kahani-android
Length of output: 4264
Use parameterized queries instead of string interpolation to prevent SQL injection.
The migration uses string interpolation directly in SQL statements (DEFAULT '$host', DEFAULT '$username'), which is a security vulnerability. Although the values come from app-controlled SharedPreferences, best practice requires parameterized or bound queries. Since ALTER TABLE commands don't support bind parameters in SQLite, consider storing these values in the database after table creation instead of embedding them in the schema migration.
The concern about empty-string defaults orphaning data is not valid—all subsequent queries use the same preferences.getHost() ?: "" fallback, so cache scoping remains consistent. This is intentional multi-tenant behavior: when credentials change, the cache scope changes accordingly.
🤖 Prompt for AI Agents
In
`@app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/LocalCacheModule.kt`
around lines 48 - 53, produceMigration15_16 currently injects
preferences.getHost() and preferences.getUsername() directly into SQL DEFAULT
clauses (e.g., DEFAULT '$host'), which risks SQL injection and is fragile since
ALTER TABLE doesn’t accept bind parameters; change the migration to avoid
embedding user values into DDL: create the new/altered tables with neutral
defaults (no interpolated strings) inside produceMigration15_16, then after the
schema change run a separate parameterized INSERT/UPDATE statement (using bound
parameters) to persist host/username into the appropriate rows, using
preferences.getHost()/getUsername() as parameters; update any callers of
produceMigration15_16 and ensure MIGRATION_16_17 is applied after this safe
population step.
...n/kotlin/org/grakovne/lissen/ui/screens/settings/advanced/cache/CachedItemsSettingsScreen.kt
Outdated
Show resolved
Hide resolved
| tint = colorScheme.onSurface, | ||
| modifier = Modifier.size(24.dp), | ||
| ) | ||
| viewModel.dropCache(item, volume.chapters.first()) // dropCache by chapter handles file deletion |
There was a problem hiding this comment.
Potential crash: first() on possibly empty collection.
volume.chapters.first() will throw NoSuchElementException if chapters is empty. While unlikely for downloaded volumes, this should be handled defensively. Note that deleteSelectedVolumes at line 694 correctly uses firstOrNull() for the same operation.
Suggested fix
- viewModel.dropCache(item, volume.chapters.first()) // dropCache by chapter handles file deletion
- onItemRemoved()
+ volume.chapters.firstOrNull()?.let { chapter ->
+ viewModel.dropCache(item, chapter)
+ onItemRemoved()
+ }🤖 Prompt for AI Agents
In
`@app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/advanced/cache/CachedItemsSettingsScreen.kt`
at line 638, Replace the unsafe volume.chapters.first() call with a defensive
check: use volume.chapters.firstOrNull(), and if it returns null skip calling
viewModel.dropCache (or call an appropriate overload that doesn't require a
chapter) to avoid NoSuchElementException; update the call site where
viewModel.dropCache(item, ...) is invoked in CachedItemsSettingsScreen (same
area that uses deleteSelectedVolumes) to handle the null chapter case safely.
| val context = androidx.compose.ui.platform.LocalContext.current | ||
| val successMessage = stringResource(R.string.settings_screen_clear_metadata_cache_success) | ||
|
|
||
| AdvancedSettingsNavigationItemComposable( | ||
| title = stringResource(R.string.settings_screen_clear_metadata_cache_title), | ||
| description = stringResource(R.string.settings_screen_clear_metadata_cache_hint), | ||
| onclick = { | ||
| viewModel.clearMetadataCache { | ||
| android.widget.Toast | ||
| .makeText(context, successMessage, android.widget.Toast.LENGTH_SHORT) | ||
| .show() | ||
| } | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find clearMetadataCache implementation
rg -n "clearMetadataCache" --type kotlinRepository: SurjitSahoo/kahani-android
Length of output: 794
🏁 Script executed:
# Get file context and look for the viewModel definition to understand the ViewModel class
fd -e kt -path "*/settings/advanced/*" | head -20Repository: SurjitSahoo/kahani-android
Length of output: 238
🏁 Script executed:
# Get the clearMetadataCache implementation with context
sed -n '290,310p' app/src/main/kotlin/org/grakovne/lissen/viewmodel/SettingsViewModel.ktRepository: SurjitSahoo/kahani-android
Length of output: 334
🏁 Script executed:
# Look for the SettingsViewModel class definition and coroutine scope setup
sed -n '1,50p' app/src/main/kotlin/org/grakovne/lissen/viewmodel/SettingsViewModel.ktRepository: SurjitSahoo/kahani-android
Length of output: 2305
Add error handling and user feedback for cache clear failures.
The callback is safely dispatched on the main thread via viewModelScope.launch, so the Toast won't crash. However, there's no error handling—if either clearMetadataCache() or cachedCoverProvider.clearCache() throws an exception, the coroutine fails silently and the user receives no feedback. Add a try-catch block to handle and communicate failures to the user.
🤖 Prompt for AI Agents
In
`@app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/advanced/cache/CacheSettingsScreen.kt`
around lines 106 - 118, Wrap the cache-clear invocation in a try-catch and
surface failures to the user and logs: inside the
AdvancedSettingsNavigationItemComposable onclick lambda, call
viewModel.clearMetadataCache within a try block and on success show the existing
Toast; in the catch block log the exception and show a failure Toast (use
stringResource(R.string.settings_screen_clear_metadata_cache_failure) or a new
failure string), ensuring any exceptions from clearMetadataCache or
cachedCoverProvider.clearCache are handled instead of failing silently;
alternatively, extend viewModel.clearMetadataCache to accept an onError callback
and invoke that when cachedCoverProvider.clearCache throws, then show the
failure Toast from the onclick handler.
| companion object { | ||
| const val CURRENT_DATABASE_VERSION = 16 | ||
| } |
There was a problem hiding this comment.
Version mismatch: CURRENT_DATABASE_VERSION is 16 but schema is at version 17.
The database schema (17.json) and migrations (MIGRATION_16_17) indicate the current version is 17, but this constant is set to 16. This will cause preferences.setDatabaseVersion(16) to store an incorrect version, potentially triggering unnecessary migration checks or causing version-tracking inconsistencies.
🐛 Proposed fix
companion object {
- const val CURRENT_DATABASE_VERSION = 16
+ const val CURRENT_DATABASE_VERSION = 17
}📝 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.
| companion object { | |
| const val CURRENT_DATABASE_VERSION = 16 | |
| } | |
| companion object { | |
| const val CURRENT_DATABASE_VERSION = 17 | |
| } |
🤖 Prompt for AI Agents
In `@app/src/main/kotlin/org/grakovne/lissen/viewmodel/MigrationViewModel.kt`
around lines 48 - 50, The CURRENT_DATABASE_VERSION constant is out of sync with
the schema/migrations (schema at 17 and MIGRATION_16_17 exists); update
companion object constant CURRENT_DATABASE_VERSION from 16 to 17 so
preferences.setDatabaseVersion(...) writes the correct version and the app's
migration checks match 17.json and MIGRATION_16_17.
…ng for sensitive UI elements.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/ContentCachingManager.kt (1)
230-238:⚠️ Potential issue | 🟡 MinorPotential resource leak: OkHttp response body not explicitly closed on all paths.
The
response.bodyis consumed inside the try block, but if an exception occurs after getting the response but before the body is fully read (e.g., duringdest.parentFile?.mkdirs()), the response body may not be closed. While OkHttp typically handles this via GC finalization, explicit closure is safer.🛡️ Proposed fix to ensure response body closure
val request = requestBuilder.build() val response = client.newCall(request).execute() if (!response.isSuccessful) { Timber.e("Unable to cache media content: $response") + response.close() return@withContext CacheState(CacheStatus.Error) } val body = response.body val dest = properties.provideMediaCachePath(bookId, file.id) dest.parentFile?.mkdirs() try { - dest.outputStream().use { output -> - body.byteStream().use { input -> + response.use { + dest.outputStream().use { output -> + body.byteStream().use { input ->Alternatively, wrap the entire file processing in
response.use { }.app/src/main/kotlin/org/grakovne/lissen/content/AuthRepository.kt (1)
49-64:⚠️ Potential issue | 🟠 MajorSame privacy concern applies here for user identifier.
The
"${account.username}@$host"identifier is sent to Microsoft Clarity. As noted in theClarityComponentreview, consider hashing this identifier to protect user privacy.The placement of analytics calls after
persistCredentialsis correct - ensuring credentials are saved before tracking the login event.🔒 Proposed fix to hash the identifier
- clarityTracker.setUser("${account.username}@$host") + clarityTracker.setUser("${account.username}@$host".hashCode().toString()) clarityTracker.trackEvent("login_success")
🤖 Fix all issues with AI agents
In `@app/build.gradle.kts`:
- Around line 69-70: The CLARITY_PROJECT_ID is hardcoded via
buildConfigField("String", "CLARITY_PROJECT_ID", "\"vc8bgk8nk9\"") — change it
to read from a build-time property or environment variable (mirroring the
signing credentials pattern used around the other buildConfigField entries) so
each flavor/environment can supply its own project ID; update the
buildConfigField invocation for CLARITY_PROJECT_ID to use project property
lookup (falling back to an env var and then a sensible default) and document the
expected property name so CI/build scripts can set per-environment values.
In `@app/src/main/kotlin/org/grakovne/lissen/analytics/ClarityComponent.kt`:
- Around line 62-74: The reidentifyUser currently sends a plaintext identifier
("$username@$host") to Clarity via clarityTracker.setUser; update reidentifyUser
to first check user consent (e.g., via preferences.hasConsentedToAnalytics or
similar) before calling clarityTracker.setUser, and if consented, compute a
non-reversible hash (use a secure hash like SHA-256 with optional salt/pepper
stored securely) of the combined identifier and pass the hashed string to
clarityTracker.setUser instead of the raw "$username@$host"; when no credentials
exist still respect consent and send only a hashed device id (hash
preferences.getDeviceId()) or skip if no consent.
In `@app/src/main/kotlin/org/grakovne/lissen/analytics/ClarityTracker.kt`:
- Around line 13-40: The success logs in setUser, trackEvent(String) and
trackEvent(String, String) currently print raw userId and value (Timber.d calls)
which can leak PII; change those logging statements to avoid raw sensitive data
by either omitting the userId/value or logging a redacted/hashed form (e.g.,
mask only prefix/suffix or a stable hash), and ensure error logs (Timber.e) also
do not include raw PII—update the Timber.d/Timber.e calls inside setUser,
trackEvent(eventName) and trackEvent(eventName, value) to reference the
redacted/hash variable or only the non-sensitive eventName.
In
`@app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/ContentCachingManager.kt`:
- Around line 266-268: The catch block inside the withContext block in
ContentCachingManager currently swallows exceptions and returns
CacheState(CacheStatus.Error); modify it to log the exception before returning
by calling the class logger (e.g., logger.error or similar) with a clear message
and the exception object (for example: logger.error("Failed to ... while
updating cache", ex)), so that the error and stacktrace are recorded; ensure you
use the existing logging instance on ContentCachingManager and keep the return
CacheState(CacheStatus.Error) afterwards.
- Line 124: The code is calling the suspend function dropCache(mediaItem, it)
inside a non-suspending .map { ... } which does not await the suspend calls;
replace the .map usage with a suspending iteration such as
cachingChapters.forEach { dropCache(mediaItem, it) } (or use a for (chapter in
cachingChapters) { dropCache(mediaItem, chapter) } if you need to handle
exceptions/ordering) so each dropCache call is properly awaited; ensure this
change is made in the method in ContentCachingManager where cachingChapters and
mediaItem are used.
In `@app/src/main/kotlin/org/grakovne/lissen/playback/MediaRepository.kt`:
- Around line 184-187: The onFailure handler in MediaRepository currently sends
raw t.message to clarityTracker.trackEvent which may leak sensitive data;
instead sanitize or categorize the error before sending by mapping known
Throwable types or message patterns to safe error categories (e.g.,
"network_error", "io_error", "authentication_error", "unknown_error") and send
only the category and a non-sensitive code/flag via clarityTracker.trackEvent in
onFailure, while still logging the full throwable locally with Timber.e for
debugging; update the onFailure implementation that calls
clarityTracker.trackEvent and Timber.e to perform this
classification/sanitization.
🧹 Nitpick comments (3)
app/src/main/kotlin/org/grakovne/lissen/viewmodel/LibraryViewModel.kt (1)
237-244: Analytics event fires before operation completes.The
library_refreshevent is tracked before the actual refresh operation starts. If you want to track successful refreshes, consider moving the tracking inside the coroutine afterfetchRecentListening()completes. However, if the intent is to track user intent rather than success, the current placement is acceptable.app/src/main/kotlin/org/grakovne/lissen/analytics/ClarityComponent.kt (2)
45-58: Empty lifecycle callbacks are required by the interface.The detekt warnings about empty function blocks are false positives here since
Application.ActivityLifecycleCallbacksrequires implementing all methods. You can suppress the warnings or use a more explicit style to indicate intentional no-ops.💡 Option to suppress detekt warnings
+ `@Suppress`("EmptyFunctionBlock") override fun onActivityStarted(activity: Activity) {} + `@Suppress`("EmptyFunctionBlock") override fun onActivityResumed(activity: Activity) {} + `@Suppress`("EmptyFunctionBlock") override fun onActivityPaused(activity: Activity) {} + `@Suppress`("EmptyFunctionBlock") override fun onActivityStopped(activity: Activity) {} + `@Suppress`("EmptyFunctionBlock") override fun onActivitySaveInstanceState( activity: Activity, outState: Bundle, ) {} + `@Suppress`("EmptyFunctionBlock") override fun onActivityDestroyed(activity: Activity) {}
26-28: Null-safe cast could silently fail.If
contextis not anApplicationinstance (unlikely but possible in edge cases), the callback registration will silently fail and Clarity won't initialize. Consider logging when this occurs.💡 Proposed fix to add logging
override fun onCreate() { - (context as? Application)?.registerActivityLifecycleCallbacks(this) + (context as? Application)?.registerActivityLifecycleCallbacks(this) + ?: Timber.w("ClarityComponent: Context is not an Application, lifecycle callbacks not registered") }
| buildConfigField("String", "CLARITY_PROJECT_ID", "\"vc8bgk8nk9\"") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
cd app && head -250 build.gradle.kts | tail -200Repository: SurjitSahoo/kahani-android
Length of output: 5795
🌐 Web query:
Microsoft Clarity Android SDK project ID configuration best practices
💡 Result:
Best practices for configuring the Microsoft Clarity Android SDK Project ID
- Treat the Project ID as an environment-specific value (dev/stage/prod). The
projectIdis the routing key that sends sessions to the correct Clarity project, so use separate Clarity projects (and IDs) per environment to avoid mixing data. [1] - Don’t hardcode the Project ID in Activities/fragments. Keep it in build-time configuration (e.g., Gradle
buildConfigField, productFlavors, or CI-injected values) and reference that constant when creatingClarityConfig(projectId = ...). This makes it easy to swap IDs per build variant while keeping initialization code identical. [1] - Initialize Clarity exactly once, as early as practical. Microsoft recommends calling
Clarity.initialize(...)from the startup activityonCreate(); if you have multiple startup activities, initialize in a customApplicationclass or duplicate the call in each startup activity. [1] - Use
Verboselogging only for debugging. SetlogLevel = Verbosetemporarily to validate the integration, then disable (default isNone) for production builds. [1] - Know where to get the correct Project ID. Copy it from your Clarity project’s Settings/Overview in the Clarity dashboard. [1][2]
- If you use a custom WorkManager initializer, initialize Clarity after it. Otherwise Clarity may not function properly. [1]
Reference initialization (Kotlin):
val config = ClarityConfig(
projectId = BuildConfig.CLARITY_PROJECT_ID,
logLevel = LogLevel.Verbose
)
Clarity.initialize(applicationContext, config)(Initialization pattern per Microsoft docs. [1])
Sources: Android SDK installation docs [1], Mobile SDK getting started / Project settings overview [2].
[1] (learn.microsoft.com)
[2] (learn.microsoft.com)
Citations:
- 1: https://learn.microsoft.com/en-us/clarity/mobile-sdk/android-sdk
- 2: https://learn.microsoft.com/de-de/clarity/mobile-sdk/sdk-getting-started?utm_source=openai
🏁 Script executed:
# Check if localProperties is defined/used in build.gradle.kts
grep -n "localProperties" app/build.gradle.kts | head -20Repository: SurjitSahoo/kahani-android
Length of output: 546
🏁 Script executed:
# Examine the specific lines mentioned
sed -n '60,80p' app/build.gradle.ktsRepository: SurjitSahoo/kahani-android
Length of output: 841
🏁 Script executed:
# Check what's at lines 199-200
sed -n '195,205p' app/build.gradle.ktsRepository: SurjitSahoo/kahani-android
Length of output: 324
Make CLARITY_PROJECT_ID configurable to support per-environment project IDs.
Microsoft Clarity best practices recommend storing the project ID in build-time configuration (not hardcoding), with support for environment variables and build flavors. This allows separate Clarity projects for different build variants while keeping initialization code identical.
The codebase already uses this pattern for signing credentials (lines 74, 82–84); apply the same approach here:
♻️ Suggested refactor
- buildConfigField("String", "CLARITY_PROJECT_ID", "\"vc8bgk8nk9\"")
+ val clarityProjectId =
+ System.getenv("CLARITY_PROJECT_ID")
+ ?: localProperties.getProperty("CLARITY_PROJECT_ID")
+ ?: "vc8bgk8nk9"
+ buildConfigField("String", "CLARITY_PROJECT_ID", "\"$clarityProjectId\"")📝 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.
| buildConfigField("String", "CLARITY_PROJECT_ID", "\"vc8bgk8nk9\"") | |
| val clarityProjectId = | |
| System.getenv("CLARITY_PROJECT_ID") | |
| ?: localProperties.getProperty("CLARITY_PROJECT_ID") | |
| ?: "vc8bgk8nk9" | |
| buildConfigField("String", "CLARITY_PROJECT_ID", "\"$clarityProjectId\"") |
🤖 Prompt for AI Agents
In `@app/build.gradle.kts` around lines 69 - 70, The CLARITY_PROJECT_ID is
hardcoded via buildConfigField("String", "CLARITY_PROJECT_ID", "\"vc8bgk8nk9\"")
— change it to read from a build-time property or environment variable
(mirroring the signing credentials pattern used around the other
buildConfigField entries) so each flavor/environment can supply its own project
ID; update the buildConfigField invocation for CLARITY_PROJECT_ID to use project
property lookup (falling back to an env var and then a sensible default) and
document the expected property name so CI/build scripts can set per-environment
values.
| private fun reidentifyUser() { | ||
| if (preferences.hasCredentials()) { | ||
| val username = preferences.getUsername() ?: return | ||
| val host = preferences.getHost() ?: return | ||
|
|
||
| // Combine host and username for a unique identifier across servers | ||
| val identifier = "$username@$host" | ||
| clarityTracker.setUser(identifier) | ||
| } else { | ||
| // Fallback to device ID if not logged in | ||
| clarityTracker.setUser(preferences.getDeviceId()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Privacy consideration: User identifier sent to third-party analytics.
The identifier "$username@$host" is sent to Microsoft Clarity. Depending on your privacy policy and user consent mechanisms, this could be a compliance concern (GDPR/CCPA). Consider:
- Hashing the identifier before sending:
identifier.hashCode().toString()or a proper hash - Ensuring user consent is obtained before analytics initialization
- Documenting this in your privacy policy
🔒 Proposed fix to hash the identifier
private fun reidentifyUser() {
if (preferences.hasCredentials()) {
val username = preferences.getUsername() ?: return
val host = preferences.getHost() ?: return
// Combine host and username for a unique identifier across servers
- val identifier = "$username@$host"
+ val identifier = "$username@$host".hashCode().toString()
clarityTracker.setUser(identifier)
} else {
// Fallback to device ID if not logged in
- clarityTracker.setUser(preferences.getDeviceId())
+ clarityTracker.setUser(preferences.getDeviceId().hashCode().toString())
}
}📝 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 reidentifyUser() { | |
| if (preferences.hasCredentials()) { | |
| val username = preferences.getUsername() ?: return | |
| val host = preferences.getHost() ?: return | |
| // Combine host and username for a unique identifier across servers | |
| val identifier = "$username@$host" | |
| clarityTracker.setUser(identifier) | |
| } else { | |
| // Fallback to device ID if not logged in | |
| clarityTracker.setUser(preferences.getDeviceId()) | |
| } | |
| } | |
| private fun reidentifyUser() { | |
| if (preferences.hasCredentials()) { | |
| val username = preferences.getUsername() ?: return | |
| val host = preferences.getHost() ?: return | |
| // Combine host and username for a unique identifier across servers | |
| val identifier = "$username@$host".hashCode().toString() | |
| clarityTracker.setUser(identifier) | |
| } else { | |
| // Fallback to device ID if not logged in | |
| clarityTracker.setUser(preferences.getDeviceId().hashCode().toString()) | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@app/src/main/kotlin/org/grakovne/lissen/analytics/ClarityComponent.kt` around
lines 62 - 74, The reidentifyUser currently sends a plaintext identifier
("$username@$host") to Clarity via clarityTracker.setUser; update reidentifyUser
to first check user consent (e.g., via preferences.hasConsentedToAnalytics or
similar) before calling clarityTracker.setUser, and if consented, compute a
non-reversible hash (use a secure hash like SHA-256 with optional salt/pepper
stored securely) of the combined identifier and pass the hashed string to
clarityTracker.setUser instead of the raw "$username@$host"; when no credentials
exist still respect consent and send only a hashed device id (hash
preferences.getDeviceId()) or skip if no consent.
app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/ContentCachingManager.kt
Outdated
Show resolved
Hide resolved
| } catch (ex: Exception) { | ||
| return@withContext CacheState(CacheStatus.Error) | ||
| } |
There was a problem hiding this comment.
Exception swallowed without logging.
The caught exception is discarded silently, making it difficult to diagnose caching failures. Consider logging the exception for debugging purposes.
🔧 Proposed fix
} catch (ex: Exception) {
+ Timber.e(ex, "Failed to cache media file: ${file.id}")
return@withContext CacheState(CacheStatus.Error)
}📝 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.
| } catch (ex: Exception) { | |
| return@withContext CacheState(CacheStatus.Error) | |
| } | |
| } catch (ex: Exception) { | |
| Timber.e(ex, "Failed to cache media file: ${file.id}") | |
| return@withContext CacheState(CacheStatus.Error) | |
| } |
🤖 Prompt for AI Agents
In
`@app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/ContentCachingManager.kt`
around lines 266 - 268, The catch block inside the withContext block in
ContentCachingManager currently swallows exceptions and returns
CacheState(CacheStatus.Error); modify it to log the exception before returning
by calling the class logger (e.g., logger.error or similar) with a clear message
and the exception object (for example: logger.error("Failed to ... while
updating cache", ex)), so that the error and stacktrace are recorded; ensure you
use the existing logging instance on ContentCachingManager and keep the return
CacheState(CacheStatus.Error) afterwards.
| override fun onFailure(t: Throwable) { | ||
| Timber.e("Unable to add callback to player") | ||
| clarityTracker.trackEvent("playback_error", t.message ?: "Unknown error") | ||
| } |
There was a problem hiding this comment.
Error message may contain sensitive information.
The error message t.message is passed directly to analytics. Depending on the error source, this could potentially contain sensitive information (file paths, user data, etc.). Consider sanitizing or categorizing errors instead of sending raw messages.
💡 Proposed fix to categorize errors
override fun onFailure(t: Throwable) {
Timber.e("Unable to add callback to player")
- clarityTracker.trackEvent("playback_error", t.message ?: "Unknown error")
+ clarityTracker.trackEvent("playback_error", t.javaClass.simpleName)
}📝 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 onFailure(t: Throwable) { | |
| Timber.e("Unable to add callback to player") | |
| clarityTracker.trackEvent("playback_error", t.message ?: "Unknown error") | |
| } | |
| override fun onFailure(t: Throwable) { | |
| Timber.e("Unable to add callback to player") | |
| clarityTracker.trackEvent("playback_error", t.javaClass.simpleName) | |
| } |
🤖 Prompt for AI Agents
In `@app/src/main/kotlin/org/grakovne/lissen/playback/MediaRepository.kt` around
lines 184 - 187, The onFailure handler in MediaRepository currently sends raw
t.message to clarityTracker.trackEvent which may leak sensitive data; instead
sanitize or categorize the error before sending by mapping known Throwable types
or message patterns to safe error categories (e.g., "network_error", "io_error",
"authentication_error", "unknown_error") and send only the category and a
non-sensitive code/flag via clarityTracker.trackEvent in onFailure, while still
logging the full throwable locally with Timber.e for debugging; update the
onFailure implementation that calls clarityTracker.trackEvent and Timber.e to
perform this classification/sanitization.
…atabase migrations, and update the migration screen UI.
…n the cache is empty, providing a new `onNavigateToLibrary` callback.
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/ContentCachingManager.kt (1)
296-302:⚠️ Potential issue | 🟡 Minor
cacheBookCoverreturnsCompletedeven when cover fetch fails.The
onFailurecallback (line 297-298) is empty, so execution falls through to line 301 which always returnsCacheState(CacheStatus.Completed). This means a cover fetch failure is masked and the overall caching result incacheMediaItem(line 117) will consider cover caching as successful. If the cover is non-critical, consider at least logging the failure.🔧 Proposed fix
onFailure = { + Timber.w("Failed to fetch book cover for ${book.id}: $it") },app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/dao/CachedBookDao.kt (1)
172-193:⚠️ Potential issue | 🟠 Major
fetchCachedItemsandfetchCachedItemsCountleak data across all hosts and users.These queries return cached items and counts without any host/username filtering. The
countCachedBooksmethod demonstrates the established pattern for multi-tenant scoping; both methods must apply similar filtering to prevent data leakage across sessions.
🤖 Fix all issues with AI agents
In `@app/src/main/kotlin/org/grakovne/lissen/content/AuthRepository.kt`:
- Around line 62-66: The calls to clarityTracker.setUser and
clarityTracker.trackEvent inside onPostLogin can throw and currently can abort
the rest of the login flow; wrap those analytics calls in a try/catch so any
exception is swallowed (or logged) and does not propagate, making them
fire-and-forget; locate the calls to clarityTracker.setUser(...) and
clarityTracker.trackEvent("login_success") and enclose them in a small try { ...
} catch (e: Exception) { /* log.debug or ignore */ } block so the subsequent
library-fetch logic in onPostLogin always runs.
In
`@app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/api/FetchRequestBuilder.kt`:
- Around line 44-58: The isolationClause in FetchRequestBuilder.kt weakens
tenant isolation by adding "OR EXISTS(...)" when host and username are present;
change the online branch so that when host and username are non-empty you only
add host and username to args and return "host = ? AND username = ?" (no OR
EXISTS), and keep the downloadedOnly=true branch as-is only if offline should
ignore host/username; update the variable isolationClause construction and any
callers expecting the previous behavior to enforce strict per-tenant scoping
when host/username are supplied.
In
`@app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/api/SearchRequestBuilder.kt`:
- Around line 35-39: The current code builds the SQL LIKE pattern from
searchQuery without escaping '%' and '_' so user input can act as wildcards;
update the logic that constructs pattern (used where searchClause is defined) to
escape '%' and '_' (e.g., replace "%" -> "\\%" and "_" -> "\\_") before wrapping
with "%" for the surrounding contains match, and update searchClause to include
"ESCAPE '\\'" so the backslash escapes are honored; ensure args.add(pattern)
still receives the escaped pattern for title, author, and seriesNames checks.
In
`@app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/ContentCachingService.kt`:
- Around line 121-128: The current guard prevents any notification updates when
any task has errored because of hasErrors().not(); remove that global error
check and instead call notificationService.updateCachingNotification with a
filtered view of executionStatuses so only errored entries are excluded (e.g.,
executionStatuses.entries.filter { (_, status) -> /* status not in error state
*/ } .map { (item, status) -> item to status }). Keep the
shouldUpdateNotification and inProgress() checks and still update
lastNotificationUpdate after calling updateCachingNotification.
- Around line 106-133: The finish() method can be called twice because both the
collect block and the onCompletion handler call finish() when inProgress()
becomes false; modify the flow so finish() is only invoked once—either remove
the finish() call from onCompletion inside executor.run(...).onCompletion or add
a guarded boolean/atomic flag (e.g., a local/field like finishCalled) that
finish() checks/sets to ensure it only runs its stop/notification side-effects
once; update references around executor.run(...).onCompletion, collect { ... },
inProgress(), and
lastNotificationUpdate/notificationService.updateCachingNotification to rely on
the single guarded finish invocation.
In
`@app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/dao/CachedBookDao.kt`:
- Around line 251-266: The two DAO methods deleteNonDownloadedBooks and
fetchNonDownloadedBookIds operate globally; change both to accept host and
username params (e.g., host: String, username: String) and add tenant predicates
to their SQL (restrict detailed_books and the subquery on book_chapters with
WHERE host = :host AND username = :username), then update all call sites to pass
the current host/username from preferences (same pattern used by
cacheBook/fetchCachedItems/fetchRecentBooks) so deletions and fetches are scoped
per tenant.
In
`@app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/dao/CachedLibraryDao.kt`:
- Around line 13-31: The upsert currently uses a single-column PK (id) which
allows cross-tenant overwrites; change the CachedLibraryEntity primary key to a
composite of id, host, and username (e.g., `@PrimaryKey`(value =
["id","host","username"])) and update any DAO upsert logic (upsertLibraries) so
conflict resolution operates on that composite key; keep
deleteLibrariesExcept(host, username) as-is but ensure updateLibraries continues
to build entities with host and username before calling upsertLibraries so each
(id,host,username) tuple is treated uniquely and cannot overwrite other tenants'
rows.
In
`@app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/Migrations.kt`:
- Around line 296-407: MIGRATION_17_18 currently recreates detailed_books,
libraries, media_progress, and book_files but omits book_chapters; add explicit
handling for book_chapters by renaming book_chapters to book_chapters_old,
creating the new book_chapters table with the original columns and a FOREIGN
KEY(bookId) REFERENCES detailed_books(id) ON UPDATE NO ACTION ON DELETE CASCADE
(matching media_progress/book_files style), INSERT ... SELECT to copy rows from
book_chapters_old into book_chapters, DROP TABLE book_chapters_old, and recreate
any needed indexes (e.g., index on bookId) so the migration consistently updates
dependent tables alongside detailed_books.
In
`@app/src/main/kotlin/org/grakovne/lissen/ui/components/AnalyticsConsentBottomSheet.kt`:
- Around line 38-44: The bottom sheet currently still allows swipe-to-dismiss
because the ModalBottomSheet's state (sheetState created by
rememberModalBottomSheetState) has no confirmValueChange guard; update the sheet
state creation to pass confirmValueChange = { newValue -> newValue !=
ModalBottomSheetValue.Hidden } (or simply { false } for stricter behavior) when
calling rememberModalBottomSheetState so drags cannot transition the state to
Hidden while retaining programmatic control used by ModalBottomSheet and its
onDismissRequest.
In `@app/src/main/kotlin/org/grakovne/lissen/viewmodel/MigrationViewModel.kt`:
- Around line 34-36: retryMigration() currently calls executeMigration()
unconditionally, allowing concurrent migrations if tapped multiple times; add
the same guard used in startMigration() to prevent re-entry by checking the
migration-running flag or job before invoking executeMigration() (e.g., early
return if a boolean like isRunning/_isMigrating or an active migration Job
exists), mirror the exact condition and behavior used in startMigration() so
retryMigration() becomes a no-op while a migration is in progress.
In `@app/src/main/res/values/strings.xml`:
- Around line 276-279: The analytics consent text incorrectly claims data is
anonymous while ClarityComponent.reidentifyUser() sends plaintext username@host
to Microsoft Clarity; either stop sending PII or update the copy. Fix option A:
in ClarityComponent.reidentifyUser() (ClarityComponent.kt) hash or redact the
username@host before calling the Clarity SDK so no plaintext identifiers are
transmitted. Fix option B: update the strings (analytics_consent_message and
analytics_consent_accept) to remove claims of anonymity/privacy and explicitly
state that non-personal telemetry and device identifiers are collected; ensure
the UI copy matches the actual behavior.
🧹 Nitpick comments (24)
app/src/main/kotlin/org/grakovne/lissen/common/Moshi.kt (1)
18-28: Debug-only adapter factory: use lazy formatting or guard against release builds.The
"Moshi Breadcrumb"label suggests this is a diagnostic aid. Two concerns:
- Eager string allocation: Kotlin string templates are evaluated before
Timber.dis called, so the interpolation cost is paid even when noTimber.Treeis planted (i.e., release builds). Prefer Timber's format-string overload to defer formatting:Suggested change
- Timber.d("Moshi Breadcrumb: Created adapter for type: $type. Adapter: ${adapter.javaClass.simpleName}") + Timber.d("Moshi Breadcrumb: Created adapter for type: %s. Adapter: %s", type, adapter.javaClass.simpleName)
- Production residue: Since Moshi caches adapters the runtime impact is limited, but wrapping every type lookup for breadcrumb logging is unusual in production code. Consider removing this factory before release, or at minimum guarding it behind
BuildConfig.DEBUG.gradle/libs.versions.toml (1)
88-88: Consider privacy/compliance implications of Microsoft Clarity integration.Microsoft Clarity is a session replay and behavioral analytics tool that captures user interactions (taps, scrolls, navigation). Ensure that:
- Appropriate user consent is obtained before initializing Clarity (GDPR/CCPA).
- Sensitive screens (e.g., login, payment) are masked or excluded from session recording.
- The Clarity project ID is not hardcoded but injected via build config or remote config.
This is broader than this file, but worth validating across the
ClarityTracker,ClarityComponent, andAnalyticsModulewiring mentioned in the PR.app/src/main/kotlin/org/grakovne/lissen/ui/components/DownloadProgressIcon.kt (1)
91-125:Completedandelsebranches are identical — consider merging.The bodies of
CacheStatus.Completed(lines 91–107) andelse(lines 109–125) are copy-pasted. You can collapse them into a single branch to reduce duplication:♻️ Suggested refactor
- is CacheStatus.Completed -> { - if (isFullyDownloaded) { - Icon( - imageVector = Icons.Filled.CloudDone, - contentDescription = stringResource(R.string.accessibility_id_download_complete), - modifier = Modifier.size(size), - tint = colorScheme.primary, - ) - } else { - Icon( - imageVector = Icons.Outlined.CloudDownload, - contentDescription = stringResource(R.string.accessibility_id_download_available), - modifier = Modifier.size(size).alpha(0.8f), - tint = color, - ) - } - } - - else -> { - if (isFullyDownloaded) { - Icon( - imageVector = Icons.Filled.CloudDone, - contentDescription = stringResource(R.string.accessibility_id_download_complete), - modifier = Modifier.size(size), - tint = colorScheme.primary, - ) - } else { - Icon( - imageVector = Icons.Outlined.CloudDownload, - contentDescription = stringResource(R.string.accessibility_id_download_available), - modifier = Modifier.size(size).alpha(0.8f), - tint = color, - ) - } - } + else -> { + if (isFullyDownloaded) { + Icon( + imageVector = Icons.Filled.CloudDone, + contentDescription = stringResource(R.string.accessibility_id_download_complete), + modifier = Modifier.size(size), + tint = colorScheme.primary, + ) + } else { + Icon( + imageVector = Icons.Outlined.CloudDownload, + contentDescription = stringResource(R.string.accessibility_id_download_available), + modifier = Modifier.size(size).alpha(0.8f), + tint = color, + ) + } + }If
Completedis intentionally separated to allow future divergence, a short comment explaining that would be helpful.app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/ActionRow.kt (2)
57-60:translationYuses raw pixels — consider density-aware conversion.
floatYis-2fraw pixels applied viatranslationY. This will appear differently across screen densities. For a consistent look, convert fromdptopxusingLocalDensity:♻️ Suggested approach
+ val density = LocalDensity.current + val floatY by animateFloatAsState( - targetValue = if (isSuggested && !isPressed) -2f else 0f, + targetValue = if (isSuggested && !isPressed) with(density) { -2.dp.toPx() } else 0f, label = "float", )Given the value is only 2 pixels, the visual inconsistency is subtle, so this is purely a polish item.
Also applies to: 78-82
86-90: Redundantif (enabled)guard insideonClick.The
clickablemodifier already receivesenabled = enabled, which prevents the callback from firing when disabled. The inner guard is unnecessary.♻️ Suggested simplification
onClick = { - if (enabled) { - haptic.performHapticFeedback(HapticFeedbackType.TextHandleMove) - onClick() - } + haptic.performHapticFeedback(HapticFeedbackType.TextHandleMove) + onClick() },app/src/main/kotlin/org/grakovne/lissen/common/HashUtils.kt (1)
5-11: Specify UTF-8 charset explicitly intoByteArray().
toByteArray()without arguments uses the platform default charset, which can vary. For consistent hashing across devices, specifyCharsets.UTF_8.♻️ Proposed fix
- .digest(input.toByteArray()) + .digest(input.toByteArray(Charsets.UTF_8))lib/src/main/kotlin/org/grakovne/lissen/lib/domain/DownloadOption.kt (1)
15-17: Consider makingSpecificFilesDownloadOptionadata class.
NumberItemDownloadOptionfollows the same pattern (plainclass), but sinceSpecificFilesDownloadOptionwraps aList<String>, value-based equality could be useful for comparisons and deduplication. This is a consistency note — if plainclassis intentional across the hierarchy, feel free to ignore.app/src/main/kotlin/org/grakovne/lissen/content/AuthRepository.kt (1)
63-64: Use an import instead of fully qualified name.
org.grakovne.lissen.common.sha256(...)can be simplified by adding an import forsha256at the top of the file.♻️ Proposed fix
Add at the top with other imports:
import org.grakovne.lissen.common.sha256Then simplify the call:
clarityTracker.setUser( - org.grakovne.lissen.common - .sha256("${account.username}@$host"), + sha256("${account.username}@$host"), )app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/api/RecentRequestBuilder.kt (1)
52-62:LIMITinterpolated directly instead of being parameterized.Line 61 uses
LIMIT $limitvia string interpolation. While safe (it's always anInt),FetchRequestBuilderparameterizesLIMIT ?— applying the same approach here would be more consistent and defensive.app/schemas/org.grakovne.lissen.content.cache.persistent.LocalCacheStorage/18.json (1)
1-388: Schemas 18 and 19 are byte-for-byte identical (same identity hash).Both
18.jsonand19.jsonshare identity hash58717f77244fed8e0b015049d0164838, andMIGRATION_18_19has an emptymigrate()body. This appears intentional for schema realignment, but should be documented (e.g., in the migration comment or a changelog) so future maintainers understand why two consecutive versions produce the same schema.app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/Migrations.kt (1)
410-415: MIGRATION_18_19 is a no-op — add a brief code comment explaining the reason.The empty migrate body with just
// Schema realignment to fix identity hash mismatchis terse. A follow-up sentence explaining what caused the mismatch (e.g., "MIGRATION_17_18 recreated tables to match Room's expected schema; this version bump aligns the stored version number") would help future maintainers avoid accidentally removing this migration.app/src/main/kotlin/org/grakovne/lissen/viewmodel/CachingModelView.kt (1)
115-119: Unnecessary fully-qualified names forPlayingChapterandBookFile.
PlayingChapteris already imported at line 31.BookFilecould be added to imports as well. Using FQN inline reduces readability.♻️ Proposed fix
Add import at the top:
import org.grakovne.lissen.lib.domain.BookFileThen simplify:
fun calculateChapterSize( bookId: String, - chapter: org.grakovne.lissen.lib.domain.PlayingChapter, - files: List<org.grakovne.lissen.lib.domain.BookFile>, + chapter: PlayingChapter, + files: List<BookFile>, ) = localCacheRepository.calculateChapterSize(bookId, chapter, files)app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/api/CachedBookRepository.kt (1)
61-71: TOCTOU gap between file deletion and DAO deletion indeleteNonDownloadedBooks.
fetchNonDownloadedBookIds()anddeleteNonDownloadedBooks()are two separate queries. If a book becomes "downloaded" between the two calls, its cache directory may still be deleted (Line 64-68 uses the snapshot), but the DAO call on Line 70 won't delete it (uses current state). Conversely,deleteRecursively()silently swallows failures—if a file can't be removed, the DB record is still cleared, orphaning nothing but leaving stale files on disk.For a cache-cleanup path this is probably acceptable, but consider using the fetched
nonDownloadedIdslist for the DAO deletion too, to keep both operations consistent:Suggested fix
suspend fun deleteNonDownloadedBooks() { val nonDownloadedIds = bookDao.fetchNonDownloadedBookIds() nonDownloadedIds.forEach { bookId -> properties .provideBookCache(bookId) .deleteRecursively() } - bookDao.deleteNonDownloadedBooks() + bookDao.deleteBooksByIds(nonDownloadedIds) }This would require a
deleteBooksByIds(ids: List<String>)DAO method. If that's too much churn, the current approach is functionally safe for cache cleanup.app/src/main/kotlin/org/grakovne/lissen/persistence/preferences/LissenSharedPreferences.kt (1)
113-116: Minor style inconsistency insetDatabaseVersion.This uses the raw
edit().putInt().apply()chain, while every other setter in this class uses Kotlin'sedit { ... }DSL extension. Consider aligning for consistency:Suggested fix
- fun setDatabaseVersion(version: Int) = sharedPreferences.edit().putInt(KEY_DATABASE_VERSION, version).apply() + fun setDatabaseVersion(version: Int) { + sharedPreferences.edit { + putInt(KEY_DATABASE_VERSION, version) + } + }app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/GlobalPlayerBottomSheet.kt (1)
448-451: Prefer imports over fully qualified class references.
org.grakovne.lissen.content.cache.persistent.CacheStateandorg.grakovne.lissen.lib.domain.CacheStatusare used inline with their full package paths. Adding them to the import block would improve readability.Suggested fix
Add to imports:
import org.grakovne.lissen.content.cache.persistent.CacheState import org.grakovne.lissen.lib.domain.CacheStatusThen simplify usage:
.collectAsState( initial = - org.grakovne.lissen.content.cache.persistent - .CacheState(org.grakovne.lissen.lib.domain.CacheStatus.Idle), + CacheState(CacheStatus.Idle), ) ... - cachingInProgress = cacheProgress.status is org.grakovne.lissen.lib.domain.CacheStatus.Caching, + cachingInProgress = cacheProgress.status is CacheStatus.Caching,Also applies to: 459-459
app/src/main/kotlin/org/grakovne/lissen/viewmodel/MigrationViewModel.kt (1)
57-57: Use an import forTimberinstead of the fully qualified reference.Suggested fix
Add to imports:
import timber.log.TimberThen:
- timber.log.Timber.e(e, "Migration failed") + Timber.e(e, "Migration failed")app/src/main/kotlin/org/grakovne/lissen/analytics/ClarityComponent.kt (1)
61-74: Empty lifecycle callbacks are interface-mandated — suppress detekt warnings.These empty blocks are required by
Application.ActivityLifecycleCallbacks. Add a@Suppress("EmptyFunctionBlock")annotation on the class or individual overrides to silence detekt.app/src/main/kotlin/org/grakovne/lissen/viewmodel/SettingsViewModel.kt (1)
38-41: Use imports instead of fully-qualified class names.Lines 38–39 use FQNs (
org.grakovne.lissen.content.BookRepository,org.grakovne.lissen.content.cache.temporary.CachedCoverProvider) while the rest of the file uses imports. Same issue on line 315 (kotlinx.coroutines.CancellationException) and line 319 (timber.log.Timber). Add proper imports for consistency.app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/DownloadsComposable.kt (2)
72-88: Derived state computed during composition — considerremember.
currentChapterIndex,currentVolume,remainingVolumes, andcompletedVolumesare recomputed on every recomposition. Wrap them inremember(book, volumes)to avoid redundant work, especially the nested iteration incompletedVolumes.Also, line 88 is extremely long — consider breaking it up for readability.
♻️ Suggested refactor
- val currentChapterIndex = - org.grakovne.lissen.playback.service - .calculateChapterIndex(book, book.progress?.currentTime ?: 0.0) - val currentVolume = - volumes.find { volume -> - volume.chapters.any { it.id == book.chapters.getOrNull(currentChapterIndex)?.id } - } - - val remainingVolumes = - if (currentVolume != null) { - val currentIndex = volumes.indexOf(currentVolume) - volumes.drop(currentIndex + 1).filter { !it.isDownloaded } - } else { - emptyList() - } - - val completedVolumes = volumes.filter { it.isDownloaded && it.chapters.all { ch -> (book.progress?.currentTime ?: 0.0) >= ch.end } } + val currentChapterIndex = remember(book, volumes) { + org.grakovne.lissen.playback.service + .calculateChapterIndex(book, book.progress?.currentTime ?: 0.0) + } + + val currentVolume = remember(book, volumes) { + volumes.find { volume -> + volume.chapters.any { it.id == book.chapters.getOrNull(currentChapterIndex)?.id } + } + } + + val remainingVolumes = remember(currentVolume, volumes) { + if (currentVolume != null) { + val currentIndex = volumes.indexOf(currentVolume) + volumes.drop(currentIndex + 1).filter { !it.isDownloaded } + } else { + emptyList() + } + } + + val currentTime = book.progress?.currentTime ?: 0.0 + val completedVolumes = remember(book, volumes) { + volumes.filter { vol -> + vol.isDownloaded && vol.chapters.all { ch -> currentTime >= ch.end } + } + }
279-324: ATOMIC option size calculations are not memoized.
calculateRequestedChaptersandfindRelatedFilesByStartTimesare called for each of the 5AtomicOptionson every recomposition. While the data set is small, wrapping the entire block in aremember(book, fileStartTimes)that precomputes the sizes would be cleaner and avoid redundant work.app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/advanced/cache/CachedItemsSettingsScreen.kt (4)
699-720:deleteSelectedVolumesiterates all cached items per selection — O(n×m).For each
selection, it scans allcachedItems(line 706) and callsgetVolumes. With small data sets this is fine, but consider grouping bybookIdfirst (ascalculateReclaimSizealready does) to reduce redundant lookups.♻️ Suggested improvement
private suspend fun deleteSelectedVolumes( selectedIds: List<VolumeIdentifier>, cachedItems: LazyPagingItems<DetailedItem>, viewModel: CachingModelView, playerViewModel: PlayerViewModel, ) { - selectedIds.forEach { selection -> - val book = (0 until cachedItems.itemCount).mapNotNull { cachedItems[it] }.find { it.id == selection.bookId } - book?.let { - val volumes = viewModel.getVolumes(it) - val volume = volumes.find { v -> v.id == selection.fileId } - volume?.chapters?.firstOrNull()?.let { chapter -> - playerViewModel.book.value?.let { playingBook -> - if (playingBook.id == it.id) { - playerViewModel.clearPlayingBook() + val books = (0 until cachedItems.itemCount).mapNotNull { cachedItems[it] } + val selectionsByBook = selectedIds.groupBy { it.bookId } + + selectionsByBook.forEach { (bookId, selections) -> + val book = books.find { it.id == bookId } ?: return@forEach + val volumes = viewModel.getVolumes(book) + + playerViewModel.book.value?.let { playingBook -> + if (playingBook.id == bookId) { + playerViewModel.clearPlayingBook() + } + } + + selections.forEach { selection -> + volumes.find { it.id == selection.fileId } + ?.chapters?.firstOrNull()?.let { chapter -> + viewModel.dropCache(book, chapter) - } } - viewModel.dropCache(it, chapter) - } } } }
196-200: Reclaim size recalculation on every selection change may be sluggish.
remember(selectedVolumes.toList(), cachedItems.itemCount)triggerscalculateReclaimSize— which callsgetVolumes()per book — on every checkbox toggle. For a smoother UX, consider precomputing and caching volume sizes when the list loads, so the reclaim total can be summed from a map without I/O.
98-101: Consider placingVolumeIdentifierin a shared domain/model package.It's currently a top-level data class in a UI screen file, but it's used across multiple functions. If other screens need similar selection tracking, this would need to be moved.
675-697:calculateReclaimSizecallsgetVolumes(book)which performs filesystem I/O during composition in arememberblock.
mapChaptersToVolumeschecks file existence via.exists()for every matched book, and this computation recalculates on every selection change (sinceselectedVolumes.toList()is a remember key). Consider caching the volumes per book or moving this computation off the main thread.
| clarityTracker.setUser( | ||
| org.grakovne.lissen.common | ||
| .sha256("${account.username}@$host"), | ||
| ) | ||
| clarityTracker.trackEvent("login_success") |
There was a problem hiding this comment.
Analytics calls should not block the login flow.
If clarityTracker.setUser() or trackEvent() throws an exception, the entire onPostLogin function will fail before the library-fetch logic on lines 69–100 executes. Analytics should be fire-and-forget and never disrupt core functionality.
🛡️ Proposed fix — wrap analytics in try/catch
+ try {
clarityTracker.setUser(
org.grakovne.lissen.common
.sha256("${account.username}@$host"),
)
clarityTracker.trackEvent("login_success")
+ } catch (e: Exception) {
+ Timber.w(e, "Failed to send analytics event")
+ }📝 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.
| clarityTracker.setUser( | |
| org.grakovne.lissen.common | |
| .sha256("${account.username}@$host"), | |
| ) | |
| clarityTracker.trackEvent("login_success") | |
| try { | |
| clarityTracker.setUser( | |
| org.grakovne.lissen.common | |
| .sha256("${account.username}@$host"), | |
| ) | |
| clarityTracker.trackEvent("login_success") | |
| } catch (e: Exception) { | |
| Timber.w(e, "Failed to send analytics event") | |
| } |
🤖 Prompt for AI Agents
In `@app/src/main/kotlin/org/grakovne/lissen/content/AuthRepository.kt` around
lines 62 - 66, The calls to clarityTracker.setUser and clarityTracker.trackEvent
inside onPostLogin can throw and currently can abort the rest of the login flow;
wrap those analytics calls in a try/catch so any exception is swallowed (or
logged) and does not propagate, making them fire-and-forget; locate the calls to
clarityTracker.setUser(...) and clarityTracker.trackEvent("login_success") and
enclose them in a small try { ... } catch (e: Exception) { /* log.debug or
ignore */ } block so the subsequent library-fetch logic in onPostLogin always
runs.
| val isolationClause = | ||
| when (downloadedOnly) { | ||
| true -> "EXISTS (SELECT 1 FROM book_chapters WHERE bookId = detailed_books.id AND isCached = 1)" | ||
| false -> { | ||
| val host = host | ||
| val username = username | ||
| if (!host.isNullOrEmpty() && !username.isNullOrEmpty()) { | ||
| args.add(host) | ||
| args.add(username) | ||
| "((host = ? AND username = ?) OR EXISTS (SELECT 1 FROM book_chapters WHERE bookId = detailed_books.id AND isCached = 1))" | ||
| } else { | ||
| "EXISTS (SELECT 1 FROM book_chapters WHERE bookId = detailed_books.id AND isCached = 1)" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Host/user isolation is weakened by the OR EXISTS fallback in the online path.
When downloadedOnly = false and host/username are provided (lines 50–53), the OR EXISTS(...) clause means books cached by a different host/user will also appear in the current user's list. If this is intentional (to always surface local content regardless of origin), it should be documented. If not, the OR should be removed so that online queries are strictly scoped to the current tenant.
The downloadedOnly = true branch (line 46) similarly ignores host/username — this may be acceptable for offline mode, but should be a deliberate choice.
Suggested tightening for the online branch
if (!host.isNullOrEmpty() && !username.isNullOrEmpty()) {
args.add(host)
args.add(username)
- "((host = ? AND username = ?) OR EXISTS (SELECT 1 FROM book_chapters WHERE bookId = detailed_books.id AND isCached = 1))"
+ "(host = ? AND username = ?)"
} else {🤖 Prompt for AI Agents
In
`@app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/api/FetchRequestBuilder.kt`
around lines 44 - 58, The isolationClause in FetchRequestBuilder.kt weakens
tenant isolation by adding "OR EXISTS(...)" when host and username are present;
change the online branch so that when host and username are non-empty you only
add host and username to args and return "host = ? AND username = ?" (no OR
EXISTS), and keep the downloadedOnly=true branch as-is only if offline should
ignore host/username; update the variable isolationClause construction and any
callers expecting the previous behavior to enforce strict per-tenant scoping
when host/username are supplied.
| val searchClause = "(title LIKE ? OR author LIKE ? OR seriesNames LIKE ?)" | ||
| val pattern = "%$searchQuery%" | ||
| args.add(pattern) | ||
| args.add(pattern) | ||
| args.add(pattern) |
There was a problem hiding this comment.
LIKE wildcards in user input are not escaped.
If searchQuery contains % or _, these characters will act as SQL LIKE wildcards, potentially returning broader results than expected. You could escape them with a custom ESCAPE clause, but this is a minor edge case for a local cache search.
♻️ Optional fix — escape LIKE wildcards
- val pattern = "%$searchQuery%"
+ val escaped = searchQuery.replace("\\", "\\\\").replace("%", "\\%").replace("_", "\\_")
+ val pattern = "%$escaped%"And update the SQL clause to include ESCAPE '\\':
- val searchClause = "(title LIKE ? OR author LIKE ? OR seriesNames LIKE ?)"
+ val searchClause = "(title LIKE ? ESCAPE '\\' OR author LIKE ? ESCAPE '\\' OR seriesNames LIKE ? ESCAPE '\\')"🤖 Prompt for AI Agents
In
`@app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/api/SearchRequestBuilder.kt`
around lines 35 - 39, The current code builds the SQL LIKE pattern from
searchQuery without escaping '%' and '_' so user input can act as wildcards;
update the logic that constructs pattern (used where searchClause is defined) to
escape '%' and '_' (e.g., replace "%" -> "\\%" and "_" -> "\\_") before wrapping
with "%" for the surrounding contains match, and update searchClause to include
"ESCAPE '\\'" so the backslash escapes are honored; ensure args.add(pattern)
still receives the escaped pattern for title, author, and seriesNames checks.
| executor | ||
| .run(mediaProvider.providePreferredChannel()) | ||
| .onCompletion { | ||
| if (executionStatuses.isEmpty()) { | ||
| if (executionStatuses.isEmpty() || !inProgress()) { | ||
| finish() | ||
| } | ||
| }.collect { progress -> | ||
| executionStatuses[item] = progress | ||
| cacheProgressBus.emit(item, progress) | ||
|
|
||
| Timber.d("Caching progress updated: $progress") | ||
| val isTerminalState = | ||
| progress.status is CacheStatus.Completed || progress.status is CacheStatus.Error || progress.status is CacheStatus.Idle | ||
| val currentTime = System.currentTimeMillis() | ||
| val shouldUpdateNotification = isTerminalState || (currentTime - lastNotificationUpdate >= notificationUpdateThrottle) | ||
|
|
||
| if (shouldUpdateNotification && inProgress() && hasErrors().not()) { | ||
| executionStatuses | ||
| .entries | ||
| .map { (item, status) -> item to status } | ||
| .let { notificationService.updateCachingNotification(it) } | ||
|
|
||
| when (inProgress() && hasErrors().not()) { | ||
| true -> | ||
| executionStatuses | ||
| .entries | ||
| .map { (item, status) -> item to status } | ||
| .let { notificationService.updateCachingNotification(it) } | ||
| lastNotificationUpdate = currentTime | ||
| } | ||
|
|
||
| false -> finish() | ||
| if (!inProgress()) { | ||
| finish() | ||
| } | ||
| } |
There was a problem hiding this comment.
finish() can be invoked twice: once from collect and again from onCompletion.
When the last emission makes inProgress() false, finish() fires at line 131. The flow then completes, and onCompletion (line 109) re-evaluates !inProgress() → calls finish() again. While Android tolerates repeated stopSelf()/stopForeground() calls, the double invocation also triggers duplicate updateErrorNotification() or cancel() side-effects.
Consider guarding with a flag or removing the redundant path:
Option: simplify onCompletion
executor
.run(mediaProvider.providePreferredChannel())
.onCompletion {
- if (executionStatuses.isEmpty() || !inProgress()) {
+ if (executionStatuses.isEmpty()) {
finish()
}
}.collect { progress ->🤖 Prompt for AI Agents
In
`@app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/ContentCachingService.kt`
around lines 106 - 133, The finish() method can be called twice because both the
collect block and the onCompletion handler call finish() when inProgress()
becomes false; modify the flow so finish() is only invoked once—either remove
the finish() call from onCompletion inside executor.run(...).onCompletion or add
a guarded boolean/atomic flag (e.g., a local/field like finishCalled) that
finish() checks/sets to ensure it only runs its stop/notification side-effects
once; update references around executor.run(...).onCompletion, collect { ... },
inProgress(), and
lastNotificationUpdate/notificationService.updateCachingNotification to rely on
the single guarded finish invocation.
| if (shouldUpdateNotification && inProgress() && hasErrors().not()) { | ||
| executionStatuses | ||
| .entries | ||
| .map { (item, status) -> item to status } | ||
| .let { notificationService.updateCachingNotification(it) } | ||
|
|
||
| when (inProgress() && hasErrors().not()) { | ||
| true -> | ||
| executionStatuses | ||
| .entries | ||
| .map { (item, status) -> item to status } | ||
| .let { notificationService.updateCachingNotification(it) } | ||
| lastNotificationUpdate = currentTime | ||
| } |
There was a problem hiding this comment.
Notification updates suppressed while any task is in error state.
The hasErrors().not() guard at line 121 means that if task A errors out while task B is still caching, progress notifications for B stop entirely. The user only sees the last notification before the error, with no further progress until everything finishes.
If the intent is to suppress noisy notifications, consider only suppressing for the errored item rather than all items.
🤖 Prompt for AI Agents
In
`@app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/ContentCachingService.kt`
around lines 121 - 128, The current guard prevents any notification updates when
any task has errored because of hasErrors().not(); remove that global error
check and instead call notificationService.updateCachingNotification with a
filtered view of executionStatuses so only errored entries are excluded (e.g.,
executionStatuses.entries.filter { (_, status) -> /* status not in error state
*/ } .map { (item, status) -> item to status }). Keep the
shouldUpdateNotification and inProgress() checks and still update
lastNotificationUpdate after calling updateCachingNotification.
| @Transaction | ||
| suspend fun updateLibraries(libraries: List<Library>) { | ||
| suspend fun updateLibraries( | ||
| libraries: List<Library>, | ||
| host: String?, | ||
| username: String?, | ||
| ) { | ||
| val entities = | ||
| libraries.map { | ||
| CachedLibraryEntity( | ||
| id = it.id, | ||
| title = it.title, | ||
| type = it.type, | ||
| host = host, | ||
| username = username, | ||
| ) | ||
| } | ||
|
|
||
| upsertLibraries(entities) | ||
| deleteLibrariesExcept(entities.map { it.id }) | ||
| deleteLibrariesExcept(entities.map { it.id }, host, username) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check how Library class is defined and how IDs are sourced
rg -n --type kotlin 'data class Library' -C 5
rg -n --type kotlin 'class Library\(' -C 5Repository: SurjitSahoo/kahani-android
Length of output: 17090
🏁 Script executed:
#!/bin/bash
# Find Library instantiation patterns to understand ID source
rg -n --type kotlin 'Library\(' -C 2 -g '!**/test/**' -g '!**/dao/**' | head -80Repository: SurjitSahoo/kahani-android
Length of output: 7690
🏁 Script executed:
#!/bin/bash
# Examine the CachedLibraryEntity to see the actual schema and conflict strategy
rg -n --type kotlin 'CachedLibraryEntity' -B 5 -A 15 | head -100Repository: SurjitSahoo/kahani-android
Length of output: 12030
🏁 Script executed:
#!/bin/bash
# Check the upsertLibraries method implementation to see the OnConflictStrategy
rg -n --type kotlin 'upsertLibraries\|@Insert\|@Upsert' -B 2 -A 2Repository: SurjitSahoo/kahani-android
Length of output: 52
Composite primary key required to prevent cross-tenant library overwrites.
The @PrimaryKey id: String combined with OnConflictStrategy.REPLACE creates a collision risk in this multi-tenant scenario. If libraries from different Audiobookshelf servers share the same ID, upserting for one tenant silently replaces another tenant's library. While deleteLibrariesExcept is scoped to (host, username), the deletion happens after the unsafe upsert, leaving a window where data from one tenant overwrites another's cached data.
Change the PK to composite @PrimaryKey(value = ["id", "host", "username"]) or include both fields in the conflict resolution, ensuring each (id, host, username) tuple is truly unique.
🤖 Prompt for AI Agents
In
`@app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/dao/CachedLibraryDao.kt`
around lines 13 - 31, The upsert currently uses a single-column PK (id) which
allows cross-tenant overwrites; change the CachedLibraryEntity primary key to a
composite of id, host, and username (e.g., `@PrimaryKey`(value =
["id","host","username"])) and update any DAO upsert logic (upsertLibraries) so
conflict resolution operates on that composite key; keep
deleteLibrariesExcept(host, username) as-is but ensure updateLibraries continues
to build entities with host and username before calling upsertLibraries so each
(id,host,username) tuple is treated uniquely and cannot overwrite other tenants'
rows.
| val MIGRATION_17_18 = | ||
| object : Migration(17, 18) { | ||
| override fun migrate(db: SupportSQLiteDatabase) { | ||
| // Recreate detailed_books to ensure host and username are nullable and correctly positioned | ||
| db.execSQL("ALTER TABLE detailed_books RENAME TO detailed_books_old") | ||
| db.execSQL( | ||
| """ | ||
| CREATE TABLE detailed_books ( | ||
| id TEXT NOT NULL PRIMARY KEY, | ||
| title TEXT NOT NULL, | ||
| subtitle TEXT, | ||
| author TEXT, | ||
| narrator TEXT, | ||
| year TEXT, | ||
| abstract TEXT, | ||
| publisher TEXT, | ||
| duration INTEGER NOT NULL, | ||
| libraryId TEXT, | ||
| libraryType TEXT, | ||
| seriesJson TEXT, | ||
| seriesNames TEXT, | ||
| createdAt INTEGER NOT NULL, | ||
| updatedAt INTEGER NOT NULL, | ||
| host TEXT, | ||
| username TEXT | ||
| ) | ||
| """.trimIndent(), | ||
| ) | ||
| db.execSQL( | ||
| """ | ||
| INSERT INTO detailed_books ( | ||
| id, title, subtitle, author, narrator, year, abstract, publisher, | ||
| duration, libraryId, libraryType, seriesJson, seriesNames, createdAt, updatedAt, host, username | ||
| ) | ||
| SELECT | ||
| id, title, subtitle, author, narrator, year, abstract, publisher, | ||
| duration, libraryId, libraryType, seriesJson, seriesNames, createdAt, updatedAt, host, username | ||
| FROM detailed_books_old | ||
| """.trimIndent(), | ||
| ) | ||
| db.execSQL("DROP TABLE detailed_books_old") | ||
|
|
||
| // Recreate libraries | ||
| db.execSQL("ALTER TABLE libraries RENAME TO libraries_old") | ||
| db.execSQL( | ||
| """ | ||
| CREATE TABLE libraries ( | ||
| id TEXT NOT NULL PRIMARY KEY, | ||
| title TEXT NOT NULL, | ||
| type TEXT NOT NULL, | ||
| host TEXT, | ||
| username TEXT | ||
| ) | ||
| """.trimIndent(), | ||
| ) | ||
| db.execSQL( | ||
| """ | ||
| INSERT INTO libraries (id, title, type, host, username) | ||
| SELECT id, title, type, host, username FROM libraries_old | ||
| """.trimIndent(), | ||
| ) | ||
| db.execSQL("DROP TABLE libraries_old") | ||
|
|
||
| // Recreate media_progress | ||
| db.execSQL("ALTER TABLE media_progress RENAME TO media_progress_old") | ||
| db.execSQL( | ||
| """ | ||
| CREATE TABLE media_progress ( | ||
| bookId TEXT NOT NULL PRIMARY KEY, | ||
| currentTime REAL NOT NULL, | ||
| isFinished INTEGER NOT NULL, | ||
| lastUpdate INTEGER NOT NULL, | ||
| host TEXT, | ||
| username TEXT, | ||
| FOREIGN KEY(bookId) REFERENCES detailed_books(id) ON UPDATE NO ACTION ON DELETE CASCADE | ||
| ) | ||
| """.trimIndent(), | ||
| ) | ||
| db.execSQL( | ||
| """ | ||
| INSERT INTO media_progress (bookId, currentTime, isFinished, lastUpdate, host, username) | ||
| SELECT bookId, currentTime, isFinished, lastUpdate, host, username FROM media_progress_old | ||
| """.trimIndent(), | ||
| ) | ||
| db.execSQL("DROP TABLE media_progress_old") | ||
| db.execSQL("CREATE INDEX IF NOT EXISTS index_media_progress_bookId ON media_progress (bookId)") | ||
|
|
||
| // Recreate book_files to move size to the end | ||
| db.execSQL("ALTER TABLE book_files RENAME TO book_files_old") | ||
| db.execSQL( | ||
| """ | ||
| CREATE TABLE book_files ( | ||
| id INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL, | ||
| bookFileId TEXT NOT NULL, | ||
| name TEXT NOT NULL, | ||
| duration REAL NOT NULL, | ||
| mimeType TEXT NOT NULL, | ||
| bookId TEXT NOT NULL, | ||
| size INTEGER NOT NULL DEFAULT 0, | ||
| FOREIGN KEY(bookId) REFERENCES detailed_books(id) ON UPDATE NO ACTION ON DELETE CASCADE | ||
| ) | ||
| """.trimIndent(), | ||
| ) | ||
| db.execSQL( | ||
| """ | ||
| INSERT INTO book_files (id, bookFileId, name, duration, mimeType, bookId, size) | ||
| SELECT id, bookFileId, name, duration, mimeType, bookId, size FROM book_files_old | ||
| """.trimIndent(), | ||
| ) | ||
| db.execSQL("DROP TABLE book_files_old") | ||
| db.execSQL("CREATE INDEX IF NOT EXISTS index_book_files_bookId ON book_files (bookId)") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's view the complete migration context and check for book_chapters
cd app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent || echo "Directory not found"
wc -l Migrations.ktRepository: SurjitSahoo/kahani-android
Length of output: 87
🏁 Script executed:
# Check if book_chapters is mentioned in the migration file
rg -n "book_chapters" app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/Migrations.ktRepository: SurjitSahoo/kahani-android
Length of output: 197
🏁 Script executed:
# Look for the database schema definitions to understand table relationships
rg -n "book_chapters\|CREATE TABLE" app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/ -A 5 -B 1 | head -100Repository: SurjitSahoo/kahani-android
Length of output: 52
🏁 Script executed:
# Check for migration testing setup
rg -n "MigrationTestHelper\|@RunWith\|testMigration" --type kotlin -g '!**/build/**'Repository: SurjitSahoo/kahani-android
Length of output: 52
🏁 Script executed:
# Check for foreign key pragma management
rg -n "foreign_key\|PRAGMA" app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/ -C 2Repository: SurjitSahoo/kahani-android
Length of output: 52
🏁 Script executed:
# Read the full Migrations.kt to understand the complete migration context
cat -n app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/Migrations.kt | head -100Repository: SurjitSahoo/kahani-android
Length of output: 3337
🏁 Script executed:
# Read around lines 296-407 to see MIGRATION_17_18 in context
sed -n '280,420p' app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/Migrations.kt | cat -nRepository: SurjitSahoo/kahani-android
Length of output: 5800
🏁 Script executed:
# Check if there's a subsequent migration that handles book_chapters
grep -n "MIGRATION_18\|MIGRATION_19\|MIGRATION_20\|book_chapters" app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/Migrations.ktRepository: SurjitSahoo/kahani-android
Length of output: 223
🏁 Script executed:
# Look for the database schema/entity definitions to find book_chapters relationship
find app -type f -name "*.kt" | xargs grep -l "book_chapters" | grep -v buildRepository: SurjitSahoo/kahani-android
Length of output: 604
🏁 Script executed:
# Check the Room database configuration
find app -type f -name "*Database.kt" -o -name "*Entities.kt" | head -10Repository: SurjitSahoo/kahani-android
Length of output: 52
🏁 Script executed:
# Find and read the Room database entity definitions to see book_chapters schema
find app -type f -name "*.kt" | xargs grep -l "book_chapters\|BookChapter" | head -10Repository: SurjitSahoo/kahani-android
Length of output: 984
🏁 Script executed:
# Check the database schema - look for `@Entity` or table definitions
rg -n "book_chapters|BookChapter" --type kotlin -B 5 -A 10 app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/ | head -100Repository: SurjitSahoo/kahani-android
Length of output: 11605
🏁 Script executed:
# Look at the complete migration file to see all migrations after 17_18
tail -100 app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/Migrations.ktRepository: SurjitSahoo/kahani-android
Length of output: 3601
🏁 Script executed:
# Check if there are any Room database initialization or fallback creation scripts
find app -type f -name "*.kt" | xargs grep -l "createFromAsset\|createFromFile\|fallback" | head -10Repository: SurjitSahoo/kahani-android
Length of output: 998
🏁 Script executed:
# Check if Room or the Database class explicitly manages foreign key constraints during migrations
rg -n "foreign_key\|PRAGMA\|foreignKeys\|enforceForeignKeys" app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/ -iRepository: SurjitSahoo/kahani-android
Length of output: 52
🏁 Script executed:
# Look for the database class definition and setup
find app -type f -name "*Database.kt" | xargs cat -nRepository: SurjitSahoo/kahani-android
Length of output: 52
🏁 Script executed:
# Check git history or comments about FK handling in migrations
rg -n "foreign\|FK\|constraint" app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/Migrations.kt -iRepository: SurjitSahoo/kahani-android
Length of output: 52
🏁 Script executed:
# Search for the Room database class definition more broadly
find app -type f -name "*.kt" | xargs grep -l "RoomDatabase\|@Database" | head -10Repository: SurjitSahoo/kahani-android
Length of output: 155
🏁 Script executed:
# Look for database initialization or configuration
find app -type f -name "*.kt" | xargs grep -l "\.addMigrations\|Migration\(" | head -15Repository: SurjitSahoo/kahani-android
Length of output: 93
🏁 Script executed:
# Check the LocalCacheStorage file mentioned in earlier results (likely database setup)
cat -n app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/LocalCacheStorage.kt | head -150Repository: SurjitSahoo/kahani-android
Length of output: 1275
🏁 Script executed:
# Look for Room database migration with explicit FK deferral
rg -n "allowMainThreadQueries\|databaseBuilder" app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/ -B 2 -A 5Repository: SurjitSahoo/kahani-android
Length of output: 52
🏁 Script executed:
# Check if there's a database setup/builder in the application or dependency injection
find app -type f -name "*.kt" | xargs grep -l "LocalCacheStorage\|databaseBuilder" | head -10Repository: SurjitSahoo/kahani-android
Length of output: 240
🏁 Script executed:
# Verify that book_chapters has a valid structure matching the FK constraint to detailed_books
cat -n app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/entity/CachedBookEntity.ktRepository: SurjitSahoo/kahani-android
Length of output: 4247
🏁 Script executed:
# Check the LocalCacheModule to see how the database is configured
cat -n app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/LocalCacheModule.ktRepository: SurjitSahoo/kahani-android
Length of output: 2620
🏁 Script executed:
# Verify if book_chapters is ever recreated or handled in any subsequent migration
tail -100 app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/Migrations.kt | cat -nRepository: SurjitSahoo/kahani-android
Length of output: 4301
book_chapters table is not recreated in MIGRATION_17_18 — consider explicit recreation for schema consistency.
The migration recreates detailed_books, libraries, media_progress, and book_files, but skips book_chapters. Since book_chapters has a foreign key referencing detailed_books(id), and detailed_books is dropped and recreated, this inconsistency is safe in practice because Room defers foreign key enforcement during migrations—the new detailed_books table exists before detailed_books_old is dropped.
However, explicitly recreating book_chapters with its foreign key constraint (like media_progress and book_files) would make the schema transformation clearer and consistent across all dependent tables. The empty MIGRATION_18_19 suggests schema issues required post-migration reconciliation, which could have been avoided with explicit handling.
🤖 Prompt for AI Agents
In
`@app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/Migrations.kt`
around lines 296 - 407, MIGRATION_17_18 currently recreates detailed_books,
libraries, media_progress, and book_files but omits book_chapters; add explicit
handling for book_chapters by renaming book_chapters to book_chapters_old,
creating the new book_chapters table with the original columns and a FOREIGN
KEY(bookId) REFERENCES detailed_books(id) ON UPDATE NO ACTION ON DELETE CASCADE
(matching media_progress/book_files style), INSERT ... SELECT to copy rows from
book_chapters_old into book_chapters, DROP TABLE book_chapters_old, and recreate
any needed indexes (e.g., index on bookId) so the migration consistently updates
dependent tables alongside detailed_books.
| val sheetState = rememberModalBottomSheetState(skipPartiallyExpanded = true) | ||
|
|
||
| ModalBottomSheet( | ||
| onDismissRequest = { /* User must make a choice */ }, | ||
| sheetState = sheetState, | ||
| containerColor = colorScheme.background, | ||
| dragHandle = null, |
There was a problem hiding this comment.
Drag-to-dismiss may bypass the forced consent choice.
onDismissRequest = {} blocks outside-tap and back-button dismissal, but the user can still swipe the sheet down to dismiss it. To truly force a choice, pass confirmValueChange = { false } to the sheet state:
Suggested fix
- val sheetState = rememberModalBottomSheetState(skipPartiallyExpanded = true)
+ val sheetState = rememberModalBottomSheetState(
+ skipPartiallyExpanded = true,
+ confirmValueChange = { false },
+ )This prevents the sheet from transitioning to the Hidden state via drag gesture while still allowing the programmatic callbacks to work.
📝 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 sheetState = rememberModalBottomSheetState(skipPartiallyExpanded = true) | |
| ModalBottomSheet( | |
| onDismissRequest = { /* User must make a choice */ }, | |
| sheetState = sheetState, | |
| containerColor = colorScheme.background, | |
| dragHandle = null, | |
| val sheetState = rememberModalBottomSheetState( | |
| skipPartiallyExpanded = true, | |
| confirmValueChange = { false }, | |
| ) | |
| ModalBottomSheet( | |
| onDismissRequest = { /* User must make a choice */ }, | |
| sheetState = sheetState, | |
| containerColor = colorScheme.background, | |
| dragHandle = null, |
🤖 Prompt for AI Agents
In
`@app/src/main/kotlin/org/grakovne/lissen/ui/components/AnalyticsConsentBottomSheet.kt`
around lines 38 - 44, The bottom sheet currently still allows swipe-to-dismiss
because the ModalBottomSheet's state (sheetState created by
rememberModalBottomSheetState) has no confirmValueChange guard; update the sheet
state creation to pass confirmValueChange = { newValue -> newValue !=
ModalBottomSheetValue.Hidden } (or simply { false } for stricter behavior) when
calling rememberModalBottomSheetState so drags cannot transition the state to
Hidden while retaining programmatic control used by ModalBottomSheet and its
onDismissRequest.
| <string name="analytics_consent_title">Help us polish the experience.</string> | ||
| <string name="analytics_consent_message">Help us squash bugs by sharing anonymous technical data. Your library and personal info stay 100% private.</string> | ||
| <string name="analytics_consent_accept">Help Improve Kahani 🚀</string> | ||
| <string name="analytics_consent_decline">Maybe later</string> |
There was a problem hiding this comment.
Analytics consent message claims "anonymous" data, but identifiable data is sent.
Line 277 states "sharing anonymous technical data" and "personal info stay 100% private," but ClarityComponent.reidentifyUser() sends the plaintext username@host to Microsoft Clarity. Either hash the identifier (as flagged in ClarityComponent.kt) or update this copy to accurately reflect what's collected.
🤖 Prompt for AI Agents
In `@app/src/main/res/values/strings.xml` around lines 276 - 279, The analytics
consent text incorrectly claims data is anonymous while
ClarityComponent.reidentifyUser() sends plaintext username@host to Microsoft
Clarity; either stop sending PII or update the copy. Fix option A: in
ClarityComponent.reidentifyUser() (ClarityComponent.kt) hash or redact the
username@host before calling the Clarity SDK so no plaintext identifiers are
transmitted. Fix option B: update the strings (analytics_consent_message and
analytics_consent_accept) to remove claims of anonymity/privacy and explicitly
state that non-personal telemetry and device identifiers are collected; ensure
the UI copy matches the actual behavior.
…data refresh, and refine cache cleanup logic.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/ContentCachingManager.kt (1)
277-308:⚠️ Potential issue | 🟠 Major
cacheBookCoveralways returnsCompletedeven when the cover fetch fails.The
.fold()result on lines 287–304 is never captured. Both theonFailurepath (empty body, Line 302-303) and an exception inonSuccess(returnsCacheState(CacheStatus.Error)at Line 299 — but only from the lambda, not the function) lead to the unconditionalCacheState(CacheStatus.Completed)at Line 306.This means a cover caching failure is reported as success, which can suppress the error rollback in
cacheMediaItem(Lines 112-127).🐛 Proposed fix — capture fold result and return it
return withContext(Dispatchers.IO) { - channel + val result = channel .fetchBookCover(book.id, width = null) .fold( onSuccess = { cover -> try { cover .peek() .withBlur(context) .writeToFile(rawFile) cover .withBlur(context, width = 300) .writeToFile(thumbFile) + CacheState(CacheStatus.Completed) } catch (ex: Exception) { - return@fold CacheState(CacheStatus.Error) + CacheState(CacheStatus.Error) } }, onFailure = { + CacheState(CacheStatus.Error) }, ) - CacheState(CacheStatus.Completed) + result }
🤖 Fix all issues with AI agents
In
`@app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/ContentCachingManager.kt`:
- Around line 128-136: In ContentCachingManager's catch block that currently
checks "if (e !is kotlinx.coroutines.CancellationException)" and calls
send(CacheState(CacheStatus.Error)), remove the unconditional "throw e" so
non-CancellationException errors are signaled via send(...) but not rethrown;
keep rethrowing CancellationException only (i.e., ensure CancellationException
still propagates) so the collector receives the error state without being
crashed by a second thrown exception.
In `@app/src/main/kotlin/org/grakovne/lissen/viewmodel/CachingModelView.kt`:
- Around line 113-123: getBookSize and calculateChapterSize perform synchronous
file I/O and must be moved off the main thread: change these two methods
(getBookSize and calculateChapterSize in CachingModelView) to suspend functions
and wrap the calls to localCacheRepository.calculateBookSize(...) and
localCacheRepository.calculateChapterSize(...) inside
withContext(Dispatchers.IO) (or alternatively make the corresponding
localCacheRepository methods suspend and do the withContext there); update
callers (e.g., BookDetailScreen, CachedItemsSettingsScreen, PlayerScreen,
NavigationBarComposable) to call these from a coroutine or document that they
must be invoked from a background context.
🧹 Nitpick comments (6)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/advanced/cache/CachedItemsSettingsScreen.kt (4)
339-341: Misleading parameter name:onBackactually navigates to library.At the call site (line 260),
onNavigateToLibraryis passed as this parameter. The nameonBackis confusing since it doesn't navigate back — it navigates to the library (potentially clearing the back stack).Suggested rename
-private fun PolishedCachedItemsEmptyState(onBack: () -> Unit) { +private fun PolishedCachedItemsEmptyState(onNavigateToLibrary: () -> Unit) {And update the reference at line 376:
- onClick = onBack, + onClick = onNavigateToLibrary,
660-669: Redundant fully-qualifiedCheckboxreference.
Checkboxfromandroidx.compose.material3is already imported at line 46. The FQN at line 662 is unnecessary and inconsistent with the usage at line 561.Suggested fix
- androidx.compose.material3.Checkbox( + Checkbox(
736-752: Redundant full scan ofcachedItemsper selection entry.
deleteSelectedVolumesiterates the entire paging list for each selected volume (line 739). For n selections and m items, this is O(n·m). Pre-building a lookup map avoids repeated scans.Suggested refactor
private suspend fun deleteSelectedVolumes( selectedIds: List<VolumeIdentifier>, cachedItems: LazyPagingItems<DetailedItem>, viewModel: CachingModelView, playerViewModel: PlayerViewModel, ) { + val bookMap = (0 until cachedItems.itemCount) + .mapNotNull { cachedItems[it] } + .associateBy { it.id } + selectedIds.forEach { selection -> - val book = (0 until cachedItems.itemCount).mapNotNull { cachedItems[it] }.find { it.id == selection.bookId } + val book = bookMap[selection.bookId] book?.let { val volumes = viewModel.getVolumes(it) val volume = volumes.find { v -> v.id == selection.fileId } volume?.chapters?.firstOrNull()?.let { chapter -> playerViewModel.book.value?.let { playingBook -> if (playingBook.id == it.id) { playerViewModel.clearPlayingBook() } } viewModel.dropCache(it, chapter) } } } }
556-572: DuplicatedgetVolumes+ filter call.
viewModel.getVolumes(book).filter { it.isDownloaded }is computed at line 557 (insideCachedItemComposable) and again at line 636 (insideCachedItemVolumeComposable). SinceCachedItemVolumeComposableis a direct child, consider passing the already-computed list as a parameter to avoid the redundant call.app/src/main/kotlin/org/grakovne/lissen/viewmodel/CachingModelView.kt (2)
115-119: Redundant fully-qualified name forPlayingChapter.
PlayingChapteris already imported at Line 31. The FQN on Line 117 is unnecessary.BookFileon Line 118 does appear to need qualification if not imported — consider adding an import for it instead.♻️ Proposed fix
+import org.grakovne.lissen.lib.domain.BookFile ... fun calculateChapterSize( bookId: String, - chapter: org.grakovne.lissen.lib.domain.PlayingChapter, - files: List<org.grakovne.lissen.lib.domain.BookFile>, + chapter: PlayingChapter, + files: List<BookFile>, ) = localCacheRepository.calculateChapterSize(bookId, chapter, files)
208-222:refreshMetadata()loads all cached items into memory at once.Using
Int.MAX_VALUEas the page size (Line 212) fetches the entire cache in a single query. For users with large libraries this could cause memory pressure and long pauses.Consider paginating the fetch or processing items in batches.
| } catch (e: Exception) { | ||
| if (e !is kotlinx.coroutines.CancellationException) { | ||
| Timber.e(e, "Failed to cache media item") | ||
| send(CacheState(CacheStatus.Error)) | ||
| } | ||
| throw e | ||
| } finally { | ||
| // No additional terminal state needed if completed/error already sent | ||
| } |
There was a problem hiding this comment.
Rethrowing after send(CacheStatus.Error) can crash the collector.
For non-CancellationException cases, Line 131 sends the Error state and then Line 133 rethrows the exception. The collector will receive the error emission and then be hit by the unhandled exception from the flow. Typically you either emit a terminal error state or throw — doing both can cause unexpected behavior in the collecting coroutine.
If the intent is a graceful error signal, remove the rethrow for non-cancellation exceptions. If the intent is to propagate exceptions, remove the send.
🐛 Proposed fix — don't rethrow non-cancellation exceptions
} catch (e: Exception) {
if (e !is kotlinx.coroutines.CancellationException) {
Timber.e(e, "Failed to cache media item")
send(CacheState(CacheStatus.Error))
- }
- throw e
+ } else {
+ throw e
+ }
} finally {📝 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.
| } catch (e: Exception) { | |
| if (e !is kotlinx.coroutines.CancellationException) { | |
| Timber.e(e, "Failed to cache media item") | |
| send(CacheState(CacheStatus.Error)) | |
| } | |
| throw e | |
| } finally { | |
| // No additional terminal state needed if completed/error already sent | |
| } | |
| } catch (e: Exception) { | |
| if (e !is kotlinx.coroutines.CancellationException) { | |
| Timber.e(e, "Failed to cache media item") | |
| send(CacheState(CacheStatus.Error)) | |
| } else { | |
| throw e | |
| } | |
| } finally { | |
| // No additional terminal state needed if completed/error already sent | |
| } |
🤖 Prompt for AI Agents
In
`@app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/ContentCachingManager.kt`
around lines 128 - 136, In ContentCachingManager's catch block that currently
checks "if (e !is kotlinx.coroutines.CancellationException)" and calls
send(CacheState(CacheStatus.Error)), remove the unconditional "throw e" so
non-CancellationException errors are signaled via send(...) but not rethrown;
keep rethrowing CancellationException only (i.e., ensure CancellationException
still propagates) so the collector receives the error state without being
crashed by a second thrown exception.
| fun getBookSize(book: DetailedItem) = localCacheRepository.calculateBookSize(book) | ||
|
|
||
| fun calculateChapterSize( | ||
| bookId: String, | ||
| chapter: org.grakovne.lissen.lib.domain.PlayingChapter, | ||
| files: List<org.grakovne.lissen.lib.domain.BookFile>, | ||
| ) = localCacheRepository.calculateChapterSize(bookId, chapter, files) | ||
|
|
||
| fun getBookStorageType(book: DetailedItem) = localCacheRepository.getBookStorageType(book) | ||
|
|
||
| fun getVolumes(book: DetailedItem) = localCacheRepository.mapChaptersToVolumes(book) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the localCacheRepository implementation
find . -type f -name "*.kt" | xargs grep -l "localCacheRepository" | head -20Repository: SurjitSahoo/kahani-android
Length of output: 552
🏁 Script executed:
# Search for the implementation of the methods mentioned
rg -n --type=kt "fun calculateBookSize|fun calculateChapterSize|fun getBookStorageType|fun mapChaptersToVolumes" -g "!*test*"Repository: SurjitSahoo/kahani-android
Length of output: 96
🏁 Script executed:
# Check the localCacheRepository interface/class definition
rg -n --type=kt "class.*LocalCacheRepository|interface.*LocalCacheRepository" -A 5Repository: SurjitSahoo/kahani-android
Length of output: 96
🏁 Script executed:
# Search for calculateBookSize and related methods without type specification
rg "fun calculateBookSize|fun calculateChapterSize|fun getBookStorageType|fun mapChaptersToVolumes" --glob "!*test*" -A 5Repository: SurjitSahoo/kahani-android
Length of output: 4628
🏁 Script executed:
# Find the LocalCacheRepository class definition
rg "class.*LocalCacheRepository|interface.*LocalCacheRepository" -A 20Repository: SurjitSahoo/kahani-android
Length of output: 2592
🏁 Script executed:
# Check the structure of the repository
find . -path ./node_modules -prune -o -type d -name "cache" -print | head -10Repository: SurjitSahoo/kahani-android
Length of output: 202
🏁 Script executed:
# List Kotlin files in the cache directory
find . -path "*/cache*" -name "*.kt" | head -20Repository: SurjitSahoo/kahani-android
Length of output: 1878
🏁 Script executed:
# Find usages of getBookSize, calculateChapterSize, getBookStorageType, getVolumes in the codebase
rg "getBookSize|calculateChapterSize|getBookStorageType|getVolumes" --glob "!*test*" -B 2 -A 2Repository: SurjitSahoo/kahani-android
Length of output: 11403
🏁 Script executed:
# Check the LocalCacheRepository implementation more closely for calculateChapterSize and calculateBookSize
sed -n '1,100p' app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/LocalCacheRepository.kt | cat -nRepository: SurjitSahoo/kahani-android
Length of output: 3965
🏁 Script executed:
# Get more context around the calculateBookSize and calculateChapterSize implementations
rg "fun calculateBookSize|fun calculateChapterSize" -A 15Repository: SurjitSahoo/kahani-android
Length of output: 4585
🏁 Script executed:
# Find calculateMediaMap implementation
rg "fun calculateMediaMap" -A 20Repository: SurjitSahoo/kahani-android
Length of output: 2827
🏁 Script executed:
# Also check the complete getBookStorageType implementation
rg "fun getBookStorageType" -A 25Repository: SurjitSahoo/kahani-android
Length of output: 6112
Only getBookSize and calculateChapterSize perform blocking file I/O on the calling thread.
getBookSize (which delegates to calculateBookSize) and calculateChapterSize call .length() on file paths, which performs synchronous disk I/O. These are invoked from composables in BookDetailScreen, CachedItemsSettingsScreen, PlayerScreen, and NavigationBarComposable, blocking the main thread.
getBookStorageType and getVolumes are CPU-bound (iterating collections and comparisons) and do not perform I/O—no action needed for these.
For the two I/O methods, consider wrapping them in suspend functions with withContext(Dispatchers.IO), or document that callers must invoke them from a background context.
🤖 Prompt for AI Agents
In `@app/src/main/kotlin/org/grakovne/lissen/viewmodel/CachingModelView.kt` around
lines 113 - 123, getBookSize and calculateChapterSize perform synchronous file
I/O and must be moved off the main thread: change these two methods (getBookSize
and calculateChapterSize in CachingModelView) to suspend functions and wrap the
calls to localCacheRepository.calculateBookSize(...) and
localCacheRepository.calculateChapterSize(...) inside
withContext(Dispatchers.IO) (or alternatively make the corresponding
localCacheRepository methods suspend and do the withContext there); update
callers (e.g., BookDetailScreen, CachedItemsSettingsScreen, PlayerScreen,
NavigationBarComposable) to call these from a coroutine or document that they
must be invoked from a background context.
…d dynamic cover art updates for media items, and enable forced library synchronization on pull-to-refresh.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/kotlin/org/grakovne/lissen/viewmodel/LibraryViewModel.kt (1)
246-269:⚠️ Potential issue | 🟡 Minor
refreshLibraryis fire-and-forget — callers usingasync { refreshLibrary(...) }won't await the sync.
refreshLibraryinternally launches viaviewModelScope.launchand returnsUnitimmediately. InLibraryScreen.refreshContent, it's wrapped inasync { ... }.awaitAll(), which gives the illusion of coordination but theawaitAllcompletes instantly while the actual sync (syncLibraryPage,syncRepositories) continues in the background. The pull-refresh spinner will stop before the forced sync finishes.This is a pre-existing pattern for the non-
forceRefreshpath, but it becomes more impactful now sinceforceRefresh = trueadds potentially long-running network I/O that the user expects to complete before the spinner dismisses.If this isn't addressed now, consider marking it as a known limitation.
🤖 Fix all issues with AI agents
In `@app/src/main/kotlin/org/grakovne/lissen/playback/service/PlaybackService.kt`:
- Around line 236-261: The race occurs because updateCover (which calls
fetchCover(book) and immediately updates exoPlayer) can run before prepareQueue
finishes setting media sources; instead, start the cover fetch concurrently but
only apply it after the queue is ready: create a Deferred by calling async {
fetchCover(book) } (e.g., artworkDeferred), then await prepareSession and
prepareQueue (awaitAll), then if artworkDeferred.await() is non-null switch to
withContext(Dispatchers.Main) and loop over exoPlayer.mediaItemCount to build
and replace media items (same logic currently in updateCover); reference
updateCover, fetchCover, prepareQueue, exoPlayer and ensure you remove or
replace the original updateCover async so the apply step happens after
prepareQueue completes.
In
`@app/src/main/kotlin/org/grakovne/lissen/ui/components/AsyncShimmeringImage.kt`:
- Around line 36-37: The state variables isMainLoading and isThumbnailLoaded are
currently remembered without depending on the imageRequest, so they can hold
stale values when AsyncShimmeringImage is reused for a different image; change
their remember calls to key off the imageRequest identity (for example
imageRequest.data or imageRequest.memoryCacheKey / cache key) so the states are
re-initialized when the request changes — update the remember invocations that
create isMainLoading and isThumbnailLoaded inside AsyncShimmeringImage to
remember(imageRequest.data) { mutableStateOf(true) } and
remember(imageRequest.data) { mutableStateOf(false) } (or use the request cache
key) respectively.
🧹 Nitpick comments (7)
app/src/main/kotlin/org/grakovne/lissen/content/BookRepository.kt (2)
382-397: A single exception aborts the entire prefetch loop; add per-item error handling.
fetchBookCoverreturnsOperationResult, but any unexpected exception (e.g., from the network layer or an uncaught IO error) will terminate theforEachand skip all remaining covers. For a best-effort background task, wrap each iteration in atry-catch.Also,
withContext(Dispatchers.IO)on line 390 is redundant —backgroundScopealready dispatches onDispatchers.IO.♻️ Proposed fix
private suspend fun prefetchCovers(books: List<Book>) { if (!networkService.isNetworkAvailable() || preferences.isForceCache()) { return } yield() delay(2000) // Initial delay to prioritize core metadata and thumbnails - withContext(Dispatchers.IO) { - books.forEach { book -> + books.forEach { book -> + try { fetchBookCover(book.id, null) - delay(100) - yield() + } catch (e: Exception) { + Timber.w(e, "Failed to prefetch cover for ${book.id}") } + delay(100) + yield() } }
399-408:libraryIddefaults to empty string — fine for current usage, fragile if reused.
this.libraryId ?: ""is safe here sincetoBook()is only consumed byprefetchCovers(which usesbook.id). If this helper is reused for cache or DB lookups keyed onlibraryId, the empty-string default will silently produce wrong results. Consider documenting the intended scope or using a more explicit sentinel.app/src/main/kotlin/org/grakovne/lissen/viewmodel/LibraryViewModel.kt (2)
219-224: Tracking event fires on every keystroke — consider debouncing.
updateSearchis invoked as the user types (viaonSearchRequestedinLibrarySearchActionComposable). Emitting"search_performed"on every non-empty intermediate value will flood analytics with one event per character. Typically, you'd track this once on search submission or after a debounce window (e.g., 300–500 ms of inactivity).Sketch: debounce approach
You could collect
_searchTokenin theinitblock with adebounceand track there, removing the inline tracking fromupdateSearch:// In init block viewModelScope.launch { _searchToken .debounce(500) .filter { it.isNotEmpty() } .collect { clarityTracker.trackEvent("search_performed") } }fun updateSearch(token: String) { _searchToken.value = token - if (token.isNotEmpty()) { - clarityTracker.trackEvent("search_performed") - } }
237-244: Tracking"library_refresh"on every call is noisy for automatic refreshes.
refreshRecentListening()is called from multiple automatic paths —initcollectors on network/cache-state changes,syncLibrary, andcheckRefreshNeeded. The tracking event will fire for every one of those background triggers, not just user-initiated refreshes. If the intent is to track user pull-to-refresh actions, consider moving the event to the call-site where the user explicitly triggers the refresh, or introducing a parameter to distinguish user-initiated from background refreshes.app/src/main/kotlin/org/grakovne/lissen/ui/screens/details/BookDetailScreen.kt (2)
11-14: Unused animation imports.
animateFloat,infiniteRepeatable,rememberInfiniteTransition, andtweenare imported but not used anywhere in this file. They may be leftovers from an earlier iteration. Consider removing them to keep imports clean.Suggested cleanup
-import androidx.compose.animation.core.animateFloat -import androidx.compose.animation.core.infiniteRepeatable -import androidx.compose.animation.core.rememberInfiniteTransition -import androidx.compose.animation.core.tween
253-254: DuplicatestorageTypecomputation — refactor to eliminate code duplication at lines 253 and 598.
storageTypeis computed identically at line 253 and line 598, both keyed only onbook.id. SincegetBookStorageTypeis a pure function depending only on immutable book metadata (files and chapters structure), compute it once at a higher scope and reuse it in both places.Note:
cacheVersionshould not be added to the dependency key becausegetBookStorageTypedoes not depend on cache state—only on the book's file and chapter structure, which are immutable properties of theDetailedItemdata class.Suggested fix — compute once after bookDetail is confirmed non-null
Move the
storageTypecomputation to line 253 location and reuse the same value at line 598:val book = bookDetail!! val storageType = remember(book.id) { cachingModelView.getBookStorageType(book) }Then remove the duplicate computation at line 598, and pass the single
storageTypetoDownloadsComposable.app/src/main/kotlin/org/grakovne/lissen/ui/components/AsyncShimmeringImage.kt (1)
69-85: Main image layer — consider notifyingonLoadingStateChanged(true)initially.The callback is only invoked with
false(on success/error). If any caller relies on being notified when loading starts, they won't receive it. The default no-op{}means this is benign today, but it's a subtle API contract change.
| val updateCover = | ||
| async { | ||
| val artworkUri = fetchCover(book) ?: return@async | ||
|
|
||
| withContext(Dispatchers.Main) { | ||
| for (i in 0 until exoPlayer.mediaItemCount) { | ||
| val currentItem = exoPlayer.getMediaItemAt(i) | ||
| val updatedMetadata = | ||
| currentItem | ||
| .mediaMetadata | ||
| .buildUpon() | ||
| .setArtworkUri(artworkUri) | ||
| .build() | ||
|
|
||
| val updatedItem = | ||
| currentItem | ||
| .buildUpon() | ||
| .setMediaMetadata(updatedMetadata) | ||
| .build() | ||
|
|
||
| exoPlayer.replaceMediaItem(i, updatedItem) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| awaitAll(prepareSession, prepareQueue) |
There was a problem hiding this comment.
Race condition: updateCover may find zero media items if the cover fetch resolves before prepareQueue sets the media sources.
updateCover and prepareQueue run concurrently. Both switch to Dispatchers.Main for ExoPlayer mutations, but there's no ordering guarantee. If fetchCover completes quickly (e.g., cover is cached), the for (i in 0 until exoPlayer.mediaItemCount) loop on Line 241 can execute before setMediaSources on Line 216, resulting in mediaItemCount == 0 — artwork is silently never applied.
A straightforward fix: fetch the cover concurrently with queue preparation, but apply it to the player only after the queue is ready.
Proposed fix — decouple fetch from apply
- val updateCover =
- async {
- val artworkUri = fetchCover(book) ?: return@async
-
- withContext(Dispatchers.Main) {
- for (i in 0 until exoPlayer.mediaItemCount) {
- val currentItem = exoPlayer.getMediaItemAt(i)
- val updatedMetadata =
- currentItem
- .mediaMetadata
- .buildUpon()
- .setArtworkUri(artworkUri)
- .build()
-
- val updatedItem =
- currentItem
- .buildUpon()
- .setMediaMetadata(updatedMetadata)
- .build()
-
- exoPlayer.replaceMediaItem(i, updatedItem)
- }
- }
- }
-
- awaitAll(prepareSession, prepareQueue)
+ val updateCover =
+ async {
+ fetchCover(book)
+ }
+
+ awaitAll(prepareSession, prepareQueue)
+
+ // Apply artwork after queue is ready
+ val artworkUri = updateCover.await()
+ if (artworkUri != null) {
+ withContext(Dispatchers.Main) {
+ for (i in 0 until exoPlayer.mediaItemCount) {
+ val currentItem = exoPlayer.getMediaItemAt(i)
+ val updatedMetadata =
+ currentItem
+ .mediaMetadata
+ .buildUpon()
+ .setArtworkUri(artworkUri)
+ .build()
+
+ val updatedItem =
+ currentItem
+ .buildUpon()
+ .setMediaMetadata(updatedMetadata)
+ .build()
+
+ exoPlayer.replaceMediaItem(i, updatedItem)
+ }
+ }
+ }This keeps the cover fetch concurrent with queue preparation (no added latency) but guarantees the player has media items before artwork is applied.
🤖 Prompt for AI Agents
In `@app/src/main/kotlin/org/grakovne/lissen/playback/service/PlaybackService.kt`
around lines 236 - 261, The race occurs because updateCover (which calls
fetchCover(book) and immediately updates exoPlayer) can run before prepareQueue
finishes setting media sources; instead, start the cover fetch concurrently but
only apply it after the queue is ready: create a Deferred by calling async {
fetchCover(book) } (e.g., artworkDeferred), then await prepareSession and
prepareQueue (awaitAll), then if artworkDeferred.await() is non-null switch to
withContext(Dispatchers.Main) and loop over exoPlayer.mediaItemCount to build
and replace media items (same logic currently in updateCover); reference
updateCover, fetchCover, prepareQueue, exoPlayer and ensure you remove or
replace the original updateCover async so the apply step happens after
prepareQueue completes.
app/src/main/kotlin/org/grakovne/lissen/ui/components/AsyncShimmeringImage.kt
Outdated
Show resolved
Hide resolved
…or when preparation completes without immediate play.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/src/main/kotlin/org/grakovne/lissen/playback/MediaRepository.kt`:
- Around line 271-273: The "sleep_timer_set" event is being emitted inside
updateTimer which is also invoked by internal flows (adjustTimer,
resetSleepTimer), causing noisy analytics; modify updateTimer(signature) to
accept a userInitiated:Boolean (default false) or similar flag, change calls
from user actions to pass true and internal callers (adjustTimer,
resetSleepTimer) to leave it false, then only call
clarityTracker.trackEvent("sleep_timer_set") when userInitiated is true;
alternatively, remove the tracking from updateTimer and add
clarityTracker.trackEvent("sleep_timer_set") to the explicit user-facing callers
instead (referencing updateTimer, adjustTimer, resetSleepTimer to locate sites).
🧹 Nitpick comments (1)
app/src/main/kotlin/org/grakovne/lissen/playback/MediaRepository.kt (1)
578-586: Consider distinguishing timer-expired pauses from user-initiated pauses.
pause()is called both by user action (togglePlayPause) and by the timer expiry receiver (line 248). Both will emit"playback_pause", which may skew analytics if you later want to understand why users pause. This is minor and can be addressed later if needed.
| if (timerOption != null) { | ||
| clarityTracker.trackEvent("sleep_timer_set") | ||
| } |
There was a problem hiding this comment.
"sleep_timer_set" fires on internal recalculations, not just user actions.
updateTimer is also called from adjustTimer (on every seek/speed change while a timer is active) and resetSleepTimer (on shake). This will emit "sleep_timer_set" events that don't represent a deliberate user timer-set, producing noisy analytics.
Consider either moving the tracking call to the caller sites that represent actual user intent, or gating it with a flag:
Proposed fix — add a parameter to distinguish user-initiated calls
fun updateTimer(
timerOption: TimerOption?,
position: Double? = null,
+ userInitiated: Boolean = false,
) {
_timerOption.postValue(timerOption)
- if (timerOption != null) {
+ if (timerOption != null && userInitiated) {
clarityTracker.trackEvent("sleep_timer_set")
}🤖 Prompt for AI Agents
In `@app/src/main/kotlin/org/grakovne/lissen/playback/MediaRepository.kt` around
lines 271 - 273, The "sleep_timer_set" event is being emitted inside updateTimer
which is also invoked by internal flows (adjustTimer, resetSleepTimer), causing
noisy analytics; modify updateTimer(signature) to accept a userInitiated:Boolean
(default false) or similar flag, change calls from user actions to pass true and
internal callers (adjustTimer, resetSleepTimer) to leave it false, then only
call clarityTracker.trackEvent("sleep_timer_set") when userInitiated is true;
alternatively, remove the tracking from updateTimer and add
clarityTracker.trackEvent("sleep_timer_set") to the explicit user-facing callers
instead (referencing updateTimer, adjustTimer, resetSleepTimer to locate sites).
…hance migration SQL safety, and refine UI/analytics state management with various fixes.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/src/main/kotlin/org/grakovne/lissen/ui/screens/library/LibraryScreen.kt`:
- Line 106: The analyticsConsent LiveData is nullable but the code uses
observeAsState(true), which masks the null "undecided" state; change the
observeAsState call in LibraryScreen (the line creating analyticsConsent from
settingsViewModel.analyticsConsent) to use a null initial value (e.g.,
settingsViewModel.analyticsConsent.observeAsState(null)) so analyticsConsent can
be null and the existing check that shows AnalyticsConsentBottomSheet
(analyticsConsent == null) will work.
🧹 Nitpick comments (6)
app/src/main/kotlin/org/grakovne/lissen/viewmodel/MigrationViewModel.kt (2)
1-14: Missing imports forTimberandCancellationException.Lines 57 and 61 use fully-qualified names (
kotlinx.coroutines.CancellationException,timber.log.Timber) inline instead of importing them at the top like all other dependencies. Add the imports for consistency.♻️ Proposed fix
import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.CancellationException import kotlinx.coroutines.delay import kotlinx.coroutines.launch import kotlinx.coroutines.withContext import org.grakovne.lissen.content.cache.persistent.dao.CachedBookDao import org.grakovne.lissen.persistence.preferences.LissenSharedPreferences +import timber.log.Timber import javax.inject.InjectThen on line 57:
- if (e is kotlinx.coroutines.CancellationException) { + if (e is CancellationException) {And on line 61:
- timber.log.Timber.e(e, "Migration failed") + Timber.e(e, "Migration failed")
42-65: Consider moving the artificial delay insidetry, after the migration query.Currently the 1.5s delay runs before the migration attempt. If the migration fails immediately, the user still waited 1.5s staring at a spinner for no benefit. Moving the delay after the IO block (or using
minOf/parallel timing) would ensure the delay only pads successful fast migrations.♻️ Proposed reordering
viewModelScope.launch { _migrationState.value = MigrationState.Running - // Artificial delay for a better UX if migration is target-fast - delay(1500) try { + // Artificial delay for a better UX if migration is fast + delay(1500) + withContext(Dispatchers.IO) { // Trigger DB initialization and migration by performing a simple query bookDao.countCachedBooks(null, "", "") }app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/Migrations.kt (1)
413-418: Empty migration used as a Room identity-hash workaround — consider a brief explanation.This is a recognized pattern for fixing Room identity-hash mismatches when the actual on-disk schema is already correct. A slightly more descriptive comment (e.g., noting which table or column ordering caused the mismatch) would help future maintainers understand why this no-op migration exists rather than fixing the schema in 17→18 directly.
app/src/main/kotlin/org/grakovne/lissen/analytics/ClarityComponent.kt (2)
35-49: Consider markingisClarityInitializedas@Volatile.
isClarityInitializedis written on the main thread inonActivityCreatedbut read inupdateConsent, which could be called from any thread. Without@Volatile, a non-main thread could see a stalefalseand silently skip the consent update.♻️ Suggested fix
- private var isClarityInitialized = false + `@Volatile` + private var isClarityInitialized = falseAlso applies to: 76-76
61-74: Suppress detekt warnings for required interface stubs.These empty bodies are mandated by
ActivityLifecycleCallbacks. Suppress the detekt noise with a single annotation orUnitexpressions.♻️ Suggested fix (suppress at class level or inline)
+ `@Suppress`("EmptyFunctionBlock") override fun onActivityStarted(activity: Activity) {} + `@Suppress`("EmptyFunctionBlock") override fun onActivityResumed(activity: Activity) {} + `@Suppress`("EmptyFunctionBlock") override fun onActivityPaused(activity: Activity) {} + `@Suppress`("EmptyFunctionBlock") override fun onActivityStopped(activity: Activity) {} + `@Suppress`("EmptyFunctionBlock") override fun onActivitySaveInstanceState( activity: Activity, outState: Bundle, ) {} + `@Suppress`("EmptyFunctionBlock") override fun onActivityDestroyed(activity: Activity) {}Alternatively, a single class-level
@Suppress("EmptyFunctionBlock")keeps it cleaner.app/src/main/kotlin/org/grakovne/lissen/ui/components/AsyncShimmeringImage.kt (1)
44-55: Consider handling thumbnail load failure explicitly.If the thumbnail request fails,
isThumbnailLoadedstaysfalseand the shimmer remains — a reasonable fallback. However, addingonErrorto set a flag could let you remove the thumbnailAsyncImagefrom composition entirely on failure, avoiding a redundant composable in the tree.This is a minor polish; current behavior degrades gracefully.
|
|
||
| val activity = LocalActivity.current | ||
| val recentBooks: List<RecentBook> by libraryViewModel.recentBooks.observeAsState(emptyList()) | ||
| val analyticsConsent by settingsViewModel.analyticsConsent.observeAsState(true) |
There was a problem hiding this comment.
Bug: observeAsState(true) prevents the analytics consent sheet from ever appearing.
analyticsConsent is typed LiveData<Boolean?> where null means "user hasn't decided yet." Passing true as the initial value swallows the null state, so the condition on line 485 (analyticsConsent == null) is never satisfied and the AnalyticsConsentBottomSheet is never shown.
🐛 Proposed fix
- val analyticsConsent by settingsViewModel.analyticsConsent.observeAsState(true)
+ val analyticsConsent by settingsViewModel.analyticsConsent.observeAsState()📝 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 analyticsConsent by settingsViewModel.analyticsConsent.observeAsState(true) | |
| val analyticsConsent by settingsViewModel.analyticsConsent.observeAsState() |
🤖 Prompt for AI Agents
In `@app/src/main/kotlin/org/grakovne/lissen/ui/screens/library/LibraryScreen.kt`
at line 106, The analyticsConsent LiveData is nullable but the code uses
observeAsState(true), which masks the null "undecided" state; change the
observeAsState call in LibraryScreen (the line creating analyticsConsent from
settingsViewModel.analyticsConsent) to use a null initial value (e.g.,
settingsViewModel.analyticsConsent.observeAsState(null)) so analyticsConsent can
be null and the existing check that shows AnalyticsConsentBottomSheet
(analyticsConsent == null) will work.
… listening flicker on pull down to refresh
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 (3)
app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/dao/CachedBookDao.kt (1)
172-193:⚠️ Potential issue | 🟠 Major
fetchCachedItemsandfetchCachedItemsCountlack host/username filtering.
countCachedBooks(line 137–141) applies per-host/per-user filtering, butfetchCachedItemsandfetchCachedItemsCountdon't accept or filter byhost/username. In a multi-tenant setup this returns cached items across all users, which may leak another user's cached library entries.If this is intentional (e.g., a device-level cache view), a clarifying comment would help. Otherwise, add the same nullable filtering:
Suggested fix
`@Transaction` `@Query`( """ SELECT * FROM detailed_books WHERE EXISTS (SELECT 1 FROM book_chapters WHERE bookId = detailed_books.id AND isCached = 1) + AND ((:host IS NULL AND host IS NULL) OR (host = :host)) + AND ((:username IS NULL AND username IS NULL) OR (username = :username)) ORDER BY title ASC, libraryId ASC LIMIT :pageSize OFFSET (:pageNumber * :pageSize) """, ) suspend fun fetchCachedItems( pageSize: Int, pageNumber: Int, + host: String?, + username: String?, ): List<CachedBookEntity> `@Query`( """ SELECT COUNT(*) FROM detailed_books WHERE EXISTS (SELECT 1 FROM book_chapters WHERE bookId = detailed_books.id AND isCached = 1) + AND ((:host IS NULL AND host IS NULL) OR (host = :host)) + AND ((:username IS NULL AND username IS NULL) OR (username = :username)) """, ) - suspend fun fetchCachedItemsCount(): Int + suspend fun fetchCachedItemsCount(host: String?, username: String?): Intapp/src/main/kotlin/org/grakovne/lissen/viewmodel/LibraryViewModel.kt (1)
227-256:⚠️ Potential issue | 🔴 Critical
refreshLibraryis fire-and-forget — callers cannot await its completion.
refreshLibraryinternally launches a new coroutine viaviewModelScope.launchand returnsUnitimmediately. InLibraryScreen.refreshContent, it's wrapped inasync { libraryViewModel.refreshLibrary(...) }.awaitAll(), but since the function isn'tsuspendand doesn't return theJob, theawaitresolves instantly. This meanspullRefreshing = false(line 152 in LibraryScreen) fires before the sync/invalidation work finishes, causing the pull-to-refresh indicator to dismiss prematurely.Either make
refreshLibraryasuspendfunction that does the work directly (the caller already provides a coroutine scope), or return theJobso the caller can join on it.🔧 Option A: make it a suspend function
- fun refreshLibrary(forceRefresh: Boolean = false) { - viewModelScope.launch { - withContext(Dispatchers.IO) { + suspend fun refreshLibrary(forceRefresh: Boolean = false) { + withContext(Dispatchers.IO) { if (forceRefresh) { networkService.refreshServerAvailability() } ... when (searchRequested.value) { true -> searchPagingSource?.invalidate() else -> defaultPagingSource.value?.invalidate() } - } } }app/src/main/kotlin/org/grakovne/lissen/ui/screens/library/LibraryScreen.kt (1)
145-153:⚠️ Potential issue | 🔴 Critical
awaitAll()doesn't actually wait forrefreshLibraryto finish.As noted in the
LibraryViewModelreview,refreshLibraryis fire-and-forget. Theasync { ... }.awaitAll()+withMinimumTimewrapper here gives the illusion of waiting, butpullRefreshing = falseon line 152 fires before the actual refresh work completes. OncerefreshLibraryis converted to asuspendfunction, this call site will work correctly as-is.
🤖 Fix all issues with AI agents
In `@app/src/main/kotlin/org/grakovne/lissen/viewmodel/LibraryViewModel.kt`:
- Around line 209-213: The updateSearch function currently calls
clarityTracker.trackEvent("search_performed") on every keystroke; stop flooding
telemetry by removing that immediate track call and instead either (A) emit the
event from a debounced collector that observes _searchToken (use a
coroutine/Flow with debounce and distinctUntilChanged, then call
clarityTracker.trackEvent("search_performed") only when the debounced value is
collected) or (B) add an explicit public action like searchSubmitted(token:
String) and call clarityTracker.trackEvent("search_performed") only from
searchSubmitted (and wired to the UI submit event); update references to
updateSearch, _searchToken, and clarityTracker.trackEvent("search_performed")
accordingly.
🧹 Nitpick comments (4)
app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/dao/CachedBookDao.kt (1)
243-247:upsertBooksunconditionally updates all books after bulk insert.
updateBooks(books)is called on the entire list, including books that were just freshly inserted. This is functionally correct (Room's@Updateon a just-inserted row is a no-op in terms of data change), but it doubles the write I/O for new entries. For small batches this is fine; if batch sizes grow, consider filtering to only update rows whereinsertBooksIgnorereturned-1L.Potential optimization
`@Transaction` suspend fun upsertBooks(books: List<BookEntity>) { - insertBooksIgnore(books) - updateBooks(books) + val insertResults = insertBooksIgnore(books) + val toUpdate = books.filterIndexed { index, _ -> insertResults[index] == -1L } + if (toUpdate.isNotEmpty()) { + updateBooks(toUpdate) + } }app/src/main/kotlin/org/grakovne/lissen/viewmodel/LibraryViewModel.kt (1)
123-133: DoublesyncRepositoriescall when server becomes available.When
isServerAvailableemitstrue, line 129 callsbookRepository.syncRepositories()and line 130 callsrefreshLibrary(), which internally also callsbookRepository.syncRepositories()(line 247) whenshouldSyncis true. This results in two concurrent sync calls for the same event.Remove the explicit
syncRepositories()here sincerefreshLibraryalready handles it, or vice versa.♻️ Suggested fix
networkService .isServerAvailable .collect { isAvailable -> if (isAvailable) { Timber.d("Server is reachable. Triggering repository sync.") - bookRepository.syncRepositories() refreshLibrary() } }app/src/main/kotlin/org/grakovne/lissen/common/NetworkService.kt (2)
85-121: Race condition oncheckJob— consider synchronization.
checkJobis read/written from multiple threads (network callbacks on the main/handler thread,hostFlowcollection onDispatchers.IO, and now any external caller since the method ispublic). The cancel-and-reassign on lines 88–90 is not atomic, so two concurrent callers can each launch a coroutine while only one reference is retained — the other runs unsupervised.Practical impact is low (both coroutines emit to a
StateFlow, last write wins), but the orphaned coroutine still does unnecessary I/O. A simple fix is to serialize access through the coroutine scope itself:♻️ Proposed fix — use scope serialization
- private var checkJob: Job? = null + private val checkJobLock = Any() + private var checkJob: Job? = null fun refreshServerAvailability() { - checkJob?.cancel() - checkJob = - scope.launch { + synchronized(checkJobLock) { + checkJob?.cancel() + checkJob = scope.launch { delay(500) // ... rest unchanged } + } }Alternatively, confine all calls to a single-threaded dispatcher (e.g.,
Dispatchers.Main.immediateor a dedicated single-thread context) so the mutable state is only touched from one thread.
106-119: DNS resolution not covered by the connect timeout.
InetSocketAddress(url.host, port)on line 109 eagerly resolves the hostname. If DNS is slow or hanging, this blocks theDispatchers.IOthread outside the 2000 mssocket.connect()timeout. UsecreateUnresolvedso resolution happens inside the guardedconnectcall:♻️ Proposed fix
- val address = java.net.InetSocketAddress(url.host, port) + val address = java.net.InetSocketAddress.createUnresolved(url.host, port)
| fun updateSearch(token: String) { | ||
| viewModelScope.launch { _searchToken.emit(token) } | ||
| _searchToken.value = token | ||
| if (token.isNotEmpty()) { | ||
| clarityTracker.trackEvent("search_performed") | ||
| } |
There was a problem hiding this comment.
Analytics event fires on every keystroke.
updateSearch is called on each character typed. Emitting "search_performed" for every non-empty intermediate value will flood telemetry. Consider debouncing or only tracking when the user submits/finishes typing (e.g., via a debounced flow collector or an explicit "search submitted" action).
🤖 Prompt for AI Agents
In `@app/src/main/kotlin/org/grakovne/lissen/viewmodel/LibraryViewModel.kt` around
lines 209 - 213, The updateSearch function currently calls
clarityTracker.trackEvent("search_performed") on every keystroke; stop flooding
telemetry by removing that immediate track call and instead either (A) emit the
event from a debounced collector that observes _searchToken (use a
coroutine/Flow with debounce and distinctUntilChanged, then call
clarityTracker.trackEvent("search_performed") only when the debounced value is
collected) or (B) add an explicit public action like searchSubmitted(token:
String) and call clarityTracker.trackEvent("search_performed") only from
searchSubmitted (and wired to the UI submit event); update references to
updateSearch, _searchToken, and clarityTracker.trackEvent("search_performed")
accordingly.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/PlaybackButtonsComposable.kt (1)
90-92:⚠️ Potential issue | 🟡 MinorPlay/pause button remains clickable during loading state.
When
isLoadingis true, the spinner is shown but theIconButtonon line 90 still invokesviewModel.togglePlayPause()on click. If the player isn't ready yet, this could trigger unintended behavior or a no-op that confuses users (haptic fires but nothing happens). Consider disabling the button while loading:Proposed fix
IconButton( onClick = { withHaptic(view) { viewModel.togglePlayPause() } }, + enabled = preparingBookId == null, modifier = Modifier.size(72.dp), ) {Also applies to: 104-109
🤖 Fix all issues with AI agents
In
`@app/src/main/kotlin/org/grakovne/lissen/ui/screens/library/composables/MiniPlayerComposable.kt`:
- Around line 264-283: preparingBookId is global so the spinner may show for the
wrong item; change the loading guard to check that preparingBookId ==
currentBook.id (or the composable's bookId) before showing
CircularProgressIndicator, remove the unused isError variable (or use it to
switch the icon to an error indicator), and prevent clicks during loading by
disabling the IconButton (enabled = preparingBookId == currentBook.id &&
preparingBookId == null) or make onClick a no-op while that book is preparing;
also replace the fully-qualified
androidx.compose.material3.CircularProgressIndicator with an import at the top
of the file.
In
`@app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/PlaybackButtonsComposable.kt`:
- Around line 44-46: The composable is observing isPlaybackReady and
preparingError but not using them, causing unnecessary recompositions and
leaving preparation failures invisible; either remove the unused observer val
isPlaybackReady by viewModel.isPlaybackReady.observeAsState(false) (and the
local isError) if playback readiness/error are handled elsewhere, or wire
preparingError into the UI by using preparingError (and preparingBookId) to show
error feedback (e.g., replace the spinner with a retry icon/button or trigger a
Snackbar when preparingError is true) and ensure the spinner dismissal logic
respects preparingError vs preparingBookId so users see the failure state and
can retry; update the code paths around preparingBookId, preparingError and the
local isError to reflect the chosen approach.
- Around line 101-102: The local variable isError (val isError = preparingError)
in PlaybackButtonsComposable is dead code and should be removed or used; either
delete that assignment or wire it into the UI branch (e.g., replace places where
preparingError should control UI state with isError or add an error state branch
to show an error icon when preparingError is true). Update the composable logic
that reads preparingBookId/isLoading (preparingBookId != null) to also handle
preparingError (or remove isError and refer directly to preparingError) so the
error state is not unused; adjust any conditional rendering in the
PlaybackButtonsComposable accordingly.
In
`@app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/GlobalPlayerBottomSheet.kt`:
- Around line 454-455: The calls to cachingModelView.getBookStorageType(book)
and cachingModelView.getVolumes(book) allocate new objects on every composition
and should be memoized to avoid unnecessary recompositions of
DownloadsComposable; wrap each call in remember(book) (e.g., create local vals
like storageType = remember(book) { cachingModelView.getBookStorageType(book) }
and volumes = remember(book) { cachingModelView.getVolumes(book) }) and then
pass those vals into DownloadsComposable instead of calling the methods inline.
🧹 Nitpick comments (5)
app/src/main/kotlin/org/grakovne/lissen/playback/service/PlaybackService.kt (1)
63-64: Nit: redundant fully-qualified name fordagger.Lazy.
dagger.Lazyis already imported on Line 15, so the FQN on Line 64 is unnecessary. You can simplify toLazy<Cache>.Suggested fix
`@Inject` - lateinit var mediaCache: dagger.Lazy<Cache> + lateinit var mediaCache: Lazy<Cache>app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/PlaybackButtonsComposable.kt (1)
105-105: Use an import instead of the fully-qualified class name.
androidx.compose.material3.CircularProgressIndicatoris used inline with its FQN. Adding it to the import list keeps the composable body consistent with the rest of the file.Proposed fix
Add the import at the top of the file:
import androidx.compose.material3.CircularProgressIndicatorThen simplify the usage:
- androidx.compose.material3.CircularProgressIndicator( + CircularProgressIndicator( modifier = Modifier.size(32.dp), color = colorScheme.onPrimary, strokeWidth = 3.dp, )app/src/main/kotlin/org/grakovne/lissen/ui/screens/library/composables/MiniPlayerComposable.kt (2)
74-76: Unused observed states trigger unnecessary recompositions.
isPlaybackReady(line 74) is never referenced in this composable.preparingError(line 76) is only assigned to the unusedisErrorlocal on line 267. Both observations subscribe toLiveDataand will cause recompositions for no visible effect. Remove them or wire them into the UI (e.g., show an error state).
279-279: Hardcoded content description strings.
"Pause"and"Play"should usestringResource(…)for localization, consistent with thestringResource(R.string.mini_player_action_close)pattern used elsewhere in this file (line 305).app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/GlobalPlayerBottomSheet.kt (1)
451-481: Redundant.let— name the outerletparameter instead.
iton line 451 is already the non-null book from theplayingBook?.let {on line 405. Wrapping it in another.let { book -> }just to rename it adds unnecessary nesting. Name the parameter at line 405 directly:♻️ Suggested cleanup
At line 405, name the parameter:
- playingBook?.let { + playingBook?.let { book ->Then at line 451, drop the redundant let and un-indent:
- it.let { book -> - DownloadsComposable( - book = book, - ... - ) - } + DownloadsComposable( + book = book, + ... + )This also removes the inconsistency where
itis used directly on lines 406–438 but renamed tobookonly here.
| IconButton( | ||
| onClick = { withHaptic(view) { playerViewModel.togglePlayPause() } }, | ||
| ) { | ||
| Icon( | ||
| imageVector = if (isPlaying) AppIcons.PauseCircleNegative else AppIcons.PlayCircleNegative, | ||
| contentDescription = if (isPlaying) "Pause" else "Play", | ||
| tint = colorScheme.onSurface, | ||
| modifier = Modifier.size(38.dp), | ||
| ) | ||
| val isError = preparingError | ||
| val isLoading = preparingBookId != null | ||
|
|
||
| if (isLoading) { | ||
| androidx.compose.material3.CircularProgressIndicator( | ||
| modifier = Modifier.size(32.dp), | ||
| color = colorScheme.onSurface, | ||
| strokeWidth = 3.dp, | ||
| ) | ||
| } else { | ||
| Icon( | ||
| imageVector = if (isPlaying) AppIcons.PauseCircleNegative else AppIcons.PlayCircleNegative, | ||
| contentDescription = if (isPlaying) "Pause" else "Play", | ||
| tint = colorScheme.onSurface, | ||
| modifier = Modifier.size(38.dp), | ||
| ) | ||
| } |
There was a problem hiding this comment.
Loading indicator is not scoped to the current book — may show a spinner for the wrong item.
preparingBookId is a global ViewModel state. When any book is being prepared, this mini player (which displays the currently playing book) will show a spinner even if the preparation is for a different book. Compare against the current book's ID:
- val isLoading = preparingBookId != null
+ val isLoading = preparingBookId == book.idAdditional observations in this block:
isErroris dead code — assigned on line 267 but never read. Either remove it or use it (e.g., swap the icon to an error indicator).- Button remains clickable during loading — the
IconButtononClickstill callstogglePlayPause()while the spinner is shown. Consider guarding it (enabled = !isLoading) or makingonClicka no-op during preparation to prevent double-taps. - FQN instead of import —
androidx.compose.material3.CircularProgressIndicatorshould be imported at the top of the file for consistency with the other Material 3 imports already present.
🤖 Prompt for AI Agents
In
`@app/src/main/kotlin/org/grakovne/lissen/ui/screens/library/composables/MiniPlayerComposable.kt`
around lines 264 - 283, preparingBookId is global so the spinner may show for
the wrong item; change the loading guard to check that preparingBookId ==
currentBook.id (or the composable's bookId) before showing
CircularProgressIndicator, remove the unused isError variable (or use it to
switch the icon to an error indicator), and prevent clicks during loading by
disabling the IconButton (enabled = preparingBookId == currentBook.id &&
preparingBookId == null) or make onClick a no-op while that book is preparing;
also replace the fully-qualified
androidx.compose.material3.CircularProgressIndicator with an import at the top
of the file.
| val isPlaybackReady by viewModel.isPlaybackReady.observeAsState(false) | ||
| val preparingBookId by viewModel.preparingBookId.observeAsState(null) | ||
| val preparingError by viewModel.preparingError.observeAsState(false) |
There was a problem hiding this comment.
isPlaybackReady is observed but never used; preparingError is observed but the error state is not surfaced.
isPlaybackReady(line 44) is subscribed viaobserveAsStatebut never referenced in the composable, causing unnecessary recompositions on every value change. Remove it if not needed here.preparingError(line 46) is observed and assigned toisErroron line 101, but that local is also unused — the user gets no visual feedback when preparation fails (e.g., the spinner would have already been dismissed sincepreparingBookIdwould be null). Consider either removing these if error handling belongs elsewhere, or wiring the error state into the UI (e.g., show a retry icon or snackbar).
🤖 Prompt for AI Agents
In
`@app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/PlaybackButtonsComposable.kt`
around lines 44 - 46, The composable is observing isPlaybackReady and
preparingError but not using them, causing unnecessary recompositions and
leaving preparation failures invisible; either remove the unused observer val
isPlaybackReady by viewModel.isPlaybackReady.observeAsState(false) (and the
local isError) if playback readiness/error are handled elsewhere, or wire
preparingError into the UI by using preparingError (and preparingBookId) to show
error feedback (e.g., replace the spinner with a retry icon/button or trigger a
Snackbar when preparingError is true) and ensure the spinner dismissal logic
respects preparingError vs preparingBookId so users see the failure state and
can retry; update the code paths around preparingBookId, preparingError and the
local isError to reflect the chosen approach.
| val isError = preparingError | ||
| val isLoading = preparingBookId != null |
There was a problem hiding this comment.
isError is assigned but never read — dead code.
val isError = preparingError on line 101 is unused. Either remove it or implement the error-state branch (e.g., show an error icon when isError is true).
Proposed fix (remove dead variable)
Box(contentAlignment = Alignment.Center) {
- val isError = preparingError
val isLoading = preparingBookId != null📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val isError = preparingError | |
| val isLoading = preparingBookId != null | |
| Box(contentAlignment = Alignment.Center) { | |
| val isLoading = preparingBookId != null |
🤖 Prompt for AI Agents
In
`@app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/PlaybackButtonsComposable.kt`
around lines 101 - 102, The local variable isError (val isError =
preparingError) in PlaybackButtonsComposable is dead code and should be removed
or used; either delete that assignment or wire it into the UI branch (e.g.,
replace places where preparingError should control UI state with isError or add
an error state branch to show an error icon when preparingError is true). Update
the composable logic that reads preparingBookId/isLoading (preparingBookId !=
null) to also handle preparingError (or remove isError and refer directly to
preparingError) so the error state is not unused; adjust any conditional
rendering in the PlaybackButtonsComposable accordingly.
| storageType = cachingModelView.getBookStorageType(book), | ||
| volumes = cachingModelView.getVolumes(book), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*CachingModelView*" | head -20Repository: SurjitSahoo/kahani-android
Length of output: 141
🏁 Script executed:
rg "class CachingModelView" --type kotlinRepository: SurjitSahoo/kahani-android
Length of output: 162
🏁 Script executed:
cat -n app/src/main/kotlin/org/grakovne/lissen/viewmodel/CachingModelView.ktRepository: SurjitSahoo/kahani-android
Length of output: 9118
🏁 Script executed:
find . -type f -name "*LocalCacheRepository*" | head -20Repository: SurjitSahoo/kahani-android
Length of output: 160
🏁 Script executed:
rg "class LocalCacheRepository|interface LocalCacheRepository" --type kotlinRepository: SurjitSahoo/kahani-android
Length of output: 185
🏁 Script executed:
wc -l app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/LocalCacheRepository.ktRepository: SurjitSahoo/kahani-android
Length of output: 162
🏁 Script executed:
cat -n app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/LocalCacheRepository.ktRepository: SurjitSahoo/kahani-android
Length of output: 13871
🏁 Script executed:
sed -n '440,470p' app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/GlobalPlayerBottomSheet.ktRepository: SurjitSahoo/kahani-android
Length of output: 1334
Wrap getBookStorageType and getVolumes calls in remember(book) to prevent unnecessary recompositions.
These methods allocate new objects on every invocation. mapChaptersToVolumes creates a new List and new BookVolume objects each time, while getBookStorageType recalculates by creating a new Map in calculateMediaMap. Calling them directly as composition parameters will cause DownloadsComposable to recompose unnecessarily even when the underlying book data hasn't changed.
Suggested fix
val storageType = remember(book) { cachingModelView.getBookStorageType(book) }
val volumes = remember(book) { cachingModelView.getVolumes(book) }
DownloadsComposable(
book = book,
storageType = storageType,
volumes = volumes,
// ... rest of parameters
)🤖 Prompt for AI Agents
In
`@app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/GlobalPlayerBottomSheet.kt`
around lines 454 - 455, The calls to cachingModelView.getBookStorageType(book)
and cachingModelView.getVolumes(book) allocate new objects on every composition
and should be memoized to avoid unnecessary recompositions of
DownloadsComposable; wrap each call in remember(book) (e.g., create local vals
like storageType = remember(book) { cachingModelView.getBookStorageType(book) }
and volumes = remember(book) { cachingModelView.getVolumes(book) }) and then
pass those vals into DownloadsComposable instead of calling the methods inline.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Improvements