Download Enhancements, Integrate Clarity analytics, database migrations, and cache redesign#8
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.
…ng for sensitive UI elements.
…atabase migrations, and update the migration screen UI.
…n the cache is empty, providing a new `onNavigateToLibrary` callback.
…data refresh, and refine cache cleanup logic.
…d dynamic cover art updates for media items, and enable forced library synchronization on pull-to-refresh.
…or when preparation completes without immediate play.
…hance migration SQL safety, and refine UI/analytics state management with various fixes.
… listening flicker on pull down to refresh
…zation out of the main thread
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughIntegrates Microsoft Clarity analytics and Firebase Crashlytics, adds Google Services config, extensive Room schema migrations (→19) with host/username/size plumbing, refactors caching/downloads to be volume-aware, adds migration UI and analytics consent flows, and updates many DAOs, repositories, viewmodels, and UI components. Changes
Sequence Diagram(s)[skipped] 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 review |
✅ Actions performedReview triggered.
|
coderabbitai
bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
app/src/main/kotlin/org/grakovne/lissen/playback/MediaRepository.kt (1)
266-273:⚠️ Potential issue | 🟡 Minor
sleep_timer_setfires on every timer update, including resets and adjustments.
updateTimeris also called fromresetSleepTimer()(shake-to-reset) andadjustTimer()(seek adjustments for episode-based timers). This meanssleep_timer_setwill fire repeatedly during normal usage, inflating the metric beyond actual user-initiated timer sets.Consider moving the tracking to the call sites where the user explicitly sets the timer, or adding a flag to distinguish user-initiated from programmatic calls.
app/src/main/res/values/strings.xml (1)
97-97:⚠️ Potential issue | 🟡 MinorPre-existing typo: "No podcast has no episodes".
This string wasn't changed in this PR, but it reads as a double negative. The intended text is likely "This podcast has no episodes" or "No episodes available."
app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/ContentCachingManager.kt (1)
277-308:⚠️ Potential issue | 🟠 Major
cacheBookCoveralways returnsCompleted, even when cover fetching or writing fails.The
foldreturn value is discarded (not assigned), and the function always falls through toCacheState(CacheStatus.Completed)on Line 306. This means cover-caching failures are invisible to the caller at Lines 108-127, and the overallwhenblock will never see a cover error.🐛 Proposed fix to propagate the fold result
return withContext(Dispatchers.IO) { - channel + val coverResult = channel .fetchBookCover(book.id, width = null) .fold( onSuccess = { cover -> try { cover .peek() .withBlur(context) .writeToFile(rawFile) cover .withBlur(context, width = 300) // Trigger thumbnail transformation .writeToFile(thumbFile) + CacheState(CacheStatus.Completed) } catch (ex: Exception) { - return@fold CacheState(CacheStatus.Error) + Timber.e(ex, "Failed to write book cover for ${book.id}") + CacheState(CacheStatus.Error) } }, onFailure = { + Timber.w("Failed to fetch book cover for ${book.id}") + CacheState(CacheStatus.Error) }, ) - CacheState(CacheStatus.Completed) + coverResult }app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/api/CachedBookRepository.kt (2)
96-129:⚠️ Potential issue | 🟡 MinorExisting books are not updated with the current
host/usernameduringcacheBooks.When an existing
BookEntityis found (Line 100), thecopy()call omitshostandusername. Since the@UpdateDAO method updates all fields of the entity, these fields retain stale values from the previous caching session. If a user switches servers or accounts, the book entry keeps the old host/username and becomes invisible to host/username-filtered queries (Lines 163-164), effectively orphaning it.Update the
copy()call to include current host/username values from preferences:existing?.copy( title = book.title, author = book.author, subtitle = book.subtitle, seriesNames = book.series, duration = book.duration.toInt(), libraryType = null, host = preferences.getHost(), username = preferences.getUsername(), )
199-221:⚠️ Potential issue | 🔴 Critical
fetchMediaProgressat line 216 lacks host/username filtering, creating potential data isolation issues.The request includes host and username from preferences (lines 207-208), but
fetchMediaProgress(it)filters only bybookId. SinceMediaProgressEntityhasbookIdas the sole@PrimaryKey, it stores only one progress record per book across all users/hosts. If progress data contains host/username values that don't match the current request context, the wrong user's progress could be returned.
🤖 Fix all issues with AI agents
In @.github/workflows/release.yml:
- Line 73: The workflow uses the deprecated softprops/action-gh-release@v1 which
relies on an unsupported Node runtime; update the action reference in the
release workflow by changing the uses entry from softprops/action-gh-release@v1
to softprops/action-gh-release@v2 so the job uses the supported v2 release
action; ensure the rest of the job (inputs/outputs, environment variables)
remain compatible after the upgrade and run the workflow to verify no other
adjustments are required.
In `@app/src/main/kotlin/org/grakovne/lissen/analytics/ClarityComponent.kt`:
- Around line 78-90: The reidentifyUser method is sending raw PII
("$username@$host") to Clarity; change it to compute a strong hash (e.g.,
SHA-256) of the combined identifier and pass the hex/base64 digest to
clarityTracker.setUser instead of the plain string; update the branch that
currently builds identifier in reidentifyUser (using preferences.getUsername()
and preferences.getHost()) to produce and use the hashed identifier, while
leaving the fallback to preferences.getDeviceId() unchanged if device ID is
non-PII or already acceptable.
In
`@app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/entity/CachedBookEntity.kt`:
- Around line 127-128: The username field (CachedBookEntity.username) is being
sent and logged in plaintext; update the code that calls Clarity.setCustomUserId
(ClarityComponent) and the Timber log in LissenMediaProvider to send only an
irreversible pseudonymized identifier (e.g., SHA-256/HMAC of "$username@$host"
or a session/device ID) instead of the raw username, and stop logging the
plaintext username (log only masked or the hashed ID); ensure the hashing is
applied before any call to Clarity.setCustomUserId and replace the Timber
statement that prints username with one that prints the hashed/masked value, and
update privacy statement strings to reflect the new behavior.
In
`@app/src/main/kotlin/org/grakovne/lissen/domain/update/UpdateCheckerService.kt`:
- Around line 48-54: In UpdateCheckerService (the method that calls
client.newCall(request).execute()), ensure the OkHttp Response is always closed
to avoid resource leaks: wrap the response handling in response.use { ... } (or
at minimum response.body?.use { ... }) and move the call to
response.body?.string() and subsequent processing (including the isSuccessful
check) inside that block so the Response/ResponseBody are closed even if
string() throws or processing fails; update references to response and json
accordingly.
In `@app/src/main/kotlin/org/grakovne/lissen/playback/service/PlaybackService.kt`:
- Around line 237-260: The launched coroutine that updates artwork can overwrite
metadata after a new preparePlayback run; to fix, add a cancellable Job field
(e.g., artworkUpdateJob: Job?) on PlaybackService, cancel() and null it at the
start of preparePlayback, then assign artworkUpdateJob = launch { ... } when
starting the background update (the existing launch block containing fetchCover
and exoPlayer.replaceMediaItem). Additionally, capture the current book
identifier (e.g., book.id) into a local val before calling fetchCover and,
inside the withContext(Dispatchers.Main) loop, verify the queue still belongs to
that book by checking a media item tag/metadata value (e.g., mediaMetadata.tag
or a custom identifier you set when building MediaItems) before calling
exoPlayer.replaceMediaItem, to prevent applying stale artwork.
In
`@app/src/main/kotlin/org/grakovne/lissen/ui/components/AnalyticsConsentBottomSheet.kt`:
- Around line 38-41: The ModalBottomSheet can still be dismissed via swipe
because rememberModalBottomSheetState(skipPartiallyExpanded = true) alone
doesn't block transitions to Hidden; update the sheet state creation
(rememberModalBottomSheetState) to supply confirmValueChange = { it !=
SheetValue.Hidden } so the sheet cannot transition to Hidden by drag, and add
the import for androidx.compose.material3.SheetValue; keep the existing
onDismissRequest no-op as-is and reference rememberModalBottomSheetState,
ModalBottomSheet, and SheetValue.Hidden when making this change.
In
`@app/src/main/kotlin/org/grakovne/lissen/ui/screens/migration/MigrationScreen.kt`:
- Around line 93-101: The dismiss TextButton in MigrationScreen (the TextButton
whose onClick calls onMigrationComplete and label
stringResource(R.string.migration_screen_dismiss_button)) allows users to bypass
a failed migration and can cause runtime crashes; update the flow so the dismiss
action is not allowed on migration failure: replace or gate the TextButton
behavior to require a successful migration or explicit destructive fallback
(e.g., show a Retry migration action that calls the migration retry helper and a
Clear App Data confirmation that wipes DB and then calls onMigrationComplete),
disable or hide the dismiss button when migration state indicates failure, and
surface clear retry UI with explicit confirmation instead of immediately calling
onMigrationComplete. Ensure the UI hooks reference the migration state checks
used by MigrationScreen and call the existing migration retry / clear methods
rather than directly proceeding.
In
`@app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/DownloadsComposable.kt`:
- Around line 54-59: The file uses fully-qualified domain type names everywhere
(e.g., org.grakovne.lissen.lib.domain.DetailedItem, BookStorageType, BookVolume,
DownloadOption) which hurts readability; update DownloadsComposable (and any
other top-level composables in DownloadsComposable.kt) to import the domain
types at the top of the file and replace all fully-qualified references with the
short type names (DetailedItem, BookStorageType, BookVolume, DownloadOption,
etc.) in the function signature and body, keeping the exact parameter names and
lambda signatures (e.g., onRequestedDownload: (DownloadOption) -> Unit)
unchanged.
In
`@app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/advanced/cache/CachedItemsSettingsScreen.kt`:
- Around line 732-753: deleteSelectedVolumes currently calls
viewModel.dropCache(it, chapter) only for volume.chapters.firstOrNull(), so
SEGMENTED/MONOLITH files shared by other chapters are not removed; change the
logic in deleteSelectedVolumes to iterate all chapters in the found volume
(e.g., volume.chapters.forEach { chapter -> viewModel.dropCache(book, chapter)
}) so each chapter reference is dropped, and apply the same fix in
CachedItemVolumeComposable where volume.chapters.firstOrNull() is used (replace
with iterating all chapters and calling dropCache for each) to ensure underlying
files are actually deleted.
🟡 Minor comments (12)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/library/composables/MiniPlayerComposable.kt-267-283 (1)
267-283:⚠️ Potential issue | 🟡 Minor
isErroris assigned but never used; no error state is surfaced to the user.Line 267 assigns
preparingErrortoisErrorbut nothing reads it. If preparation fails, the user sees neither the spinner nor an error indication — they just get the normal play button with no feedback.Additionally, the loading state checks
preparingBookId != nullglobally rather thanpreparingBookId == book.id. If a different book is being prepared, this mini player will incorrectly show a spinner.Proposed fix
- val isError = preparingError - val isLoading = preparingBookId != null + val isLoading = preparingBookId == book.id if (isLoading) {For the error case, consider showing an error icon or snackbar when
preparingError == true, or remove the unused observer entirely to avoid confusion.app/src/main/kotlin/org/grakovne/lissen/domain/update/UpdateCheckerService.kt-68-83 (1)
68-83:⚠️ Potential issue | 🟡 Minor
mapNotNullsilently drops non-numeric version segments, which can corrupt the comparison.If a tag contains pre-release suffixes (e.g.,
1.2.3-rc.1),"3-rc".toIntOrNull()returnsnulland is dropped, collapsing the version to[1, 2, 1]. This misrepresents the actual version. Consider splitting on-first to strip pre-release suffixes, or at minimum usingsubstringBefore("-")before parsing.Proposed fix
- val remote = remoteTag.removePrefix("v").split(".").mapNotNull { it.toIntOrNull() } - val local = localVersion.removePrefix("v").split(".").mapNotNull { it.toIntOrNull() } + val remote = remoteTag.removePrefix("v").substringBefore("-").split(".").mapNotNull { it.toIntOrNull() } + val local = localVersion.removePrefix("v").substringBefore("-").split(".").mapNotNull { it.toIntOrNull() }app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/PlaybackButtonsComposable.kt-44-46 (1)
44-46:⚠️ Potential issue | 🟡 Minor
isPlaybackReadyandisErrorare observed/assigned but never used.
isPlaybackReady(line 44) is subscribed as a LiveData observer but not referenced anywhere in this composable — it creates an unnecessary recomposition trigger.isError(line 101) is assigned frompreparingErrorbut also unused. Either wire these into the UI (e.g., disable buttons when not ready, show error state) or remove them to avoid unnecessary recompositions.Proposed cleanup if these are not needed yet
- val isPlaybackReady by viewModel.isPlaybackReady.observeAsState(false) val preparingBookId by viewModel.preparingBookId.observeAsState(null) - val preparingError by viewModel.preparingError.observeAsState(false) ... - val isError = preparingError val isLoading = preparingBookId != nullAlso applies to: 101-102
lib/src/main/kotlin/org/grakovne/lissen/lib/domain/DownloadOption.kt-40-60 (1)
40-60:⚠️ Potential issue | 🟡 Minor
makeDownloadOption()can throw uncaught exceptions on malformed input.
substringAfter("number_items_").toInt()(line 45) throwsNumberFormatExceptionon non-numeric input, andBase64.getUrlDecoder().decode(it)(line 53) throwsIllegalArgumentExceptionon invalid Base64. Since this parses persisted/serialized strings, corrupted data could crash the app.Consider wrapping in
runCatchingortoIntOrNull()with a fallback tonull.Proposed defensive parsing
- startsWith("number_items_") -> NumberItemDownloadOption(substringAfter("number_items_").toInt()) + startsWith("number_items_") -> substringAfter("number_items_").toIntOrNull()?.let { NumberItemDownloadOption(it) } startsWith("specific_files_") -> { val payload = substringAfter("specific_files_") - val fileIds = when { - payload.isEmpty() -> emptyList() - else -> payload - .split(",") - .map { String(Base64.getUrlDecoder().decode(it)) } + val fileIds = runCatching { + when { + payload.isEmpty() -> emptyList() + else -> payload + .split(",") + .map { String(Base64.getUrlDecoder().decode(it), Charsets.UTF_8) } + } + }.getOrNull() ?: return null } SpecificFilesDownloadOption(fileIds) }app/src/main/kotlin/org/grakovne/lissen/playback/MediaRepository.kt-185-188 (1)
185-188:⚠️ Potential issue | 🟡 MinorError messages sent to analytics may contain internal details.
t.message(e.g., from network errors) could include server URLs, file paths, or other internal info being sent to Clarity. Consider sanitizing or categorizing the error type instead of forwarding the raw message.app/src/main/kotlin/org/grakovne/lissen/viewmodel/MigrationViewModel.kt-67-69 (1)
67-69:⚠️ Potential issue | 🟡 MinorHardcoded
CURRENT_DATABASE_VERSIONrisks going out of sync with Room's schema version.This constant at line 68 must always match the
versionparameter in@Database(version = 19)onLocalCacheStorage. Currently both are 19, but they are two separate hardcoded values with no automated verification. If one is updated without the other, migrations may silently malfunction or the migration screen may not appear when needed. Consider deriving this from a single source of truth, or at minimum add a test linking these values.app/schemas/org.grakovne.lissen.content.cache.persistent.LocalCacheStorage/16.json-91-102 (1)
91-102:⚠️ Potential issue | 🟡 MinorSchema 16 declares
host/usernameas NOT NULL, but migration 15→16 adds them as nullable.In
produceMigration15_16, the columns are added viaALTER TABLE … ADD COLUMN host TEXTandALTER TABLE … ADD COLUMN username TEXT—both nullable, with optional DEFAULT values. However, schema 16.json declares these fields as"notNull": truefordetailed_books,media_progress, andlibraries. This mismatch means the exported v16 schema does not reflect what the migration produces, which would cause schema validation issues in migration tests.This is corrected in migration 17→18 (which recreates tables with nullable columns) and schema 18.json (which correctly omits
notNull), so runtime behavior is unaffected for users upgrading through all versions. However, the v16 snapshot is inaccurate for intermediate validation.app/src/main/kotlin/org/grakovne/lissen/content/BookRepository.kt-85-87 (1)
85-87:⚠️ Potential issue | 🟡 Minor
trackEvent("sync_progress")fires on every progress sync — could be very noisy.During playback,
syncProgressis called frequently (typically every few seconds). Emitting an analytics event on every call may flood the analytics backend and skew event counts. Consider throttling or sampling this event, or tracking it only at meaningful boundaries (e.g., chapter completion).app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/GlobalPlayerBottomSheet.kt-451-481 (1)
451-481:⚠️ Potential issue | 🟡 Minor
getBookStorageType/getVolumesperform file-existence checks on the main thread during composition.Both delegates ultimately call
storageProperties.provideMediaCachePath(...).exists()(visible inLocalCacheRepository.mapChaptersToVolumes). Since this composable runs on the main thread, these synchronous I/O calls could cause frame drops, especially for books with many files.Consider hoisting these into a
LaunchedEffectorproduceStatewith a background dispatcher.app/src/main/kotlin/org/grakovne/lissen/viewmodel/CachingModelView.kt-113-123 (1)
113-123:⚠️ Potential issue | 🟡 Minor
getBookSize,calculateChapterSize,getBookStorageType, andgetVolumesperform blocking I/O but are exposed as non-suspend functions.These delegate to
LocalCacheRepositorymethods that callFile.length()andFile.exists(). Callers (composables) invoke them on the main thread. Making themsuspend(or computing results in the background and exposing viaStateFlow) would prevent potential jank.This is the root cause of the main-thread I/O concern noted in
GlobalPlayerBottomSheet.ktandCachedItemsSettingsScreen.kt.app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/api/CachedBookRepository.kt-61-71 (1)
61-71:⚠️ Potential issue | 🟡 Minor
deleteNonDownloadedBookshas no error handling for file deletion failures.
deleteRecursively()at Line 67 can returnfalseor throw on I/O errors. If file deletion fails but the DB records are deleted at Line 70, the orphaned files will never be cleaned up. Consider logging failures or continuing with best-effort cleanup.Proposed improvement
nonDownloadedIds.forEach { bookId -> - properties - .provideBookCache(bookId) - .deleteRecursively() + val deleted = properties + .provideBookCache(bookId) + .deleteRecursively() + if (!deleted) { + Timber.w("Failed to fully delete cache for book $bookId") + } }app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/ContentCachingManager.kt-269-271 (1)
269-271:⚠️ Potential issue | 🟡 MinorException is caught and swallowed without logging.
The
catchreturnsCacheState(CacheStatus.Error)but discardsex, making download failures hard to diagnose. This aligns with the detektSwallowedExceptionwarning.Proposed fix
} catch (ex: Exception) { + Timber.e(ex, "Failed to write cached media file for $bookId") return@withContext CacheState(CacheStatus.Error) }
🧹 Nitpick comments (48)
app/src/main/kotlin/org/grakovne/lissen/content/LissenMediaProvider.kt (1)
238-241: Consider extracting theisDetailedpredicate for readability.The compound boolean on line 240 is dense and uses nested lambdas (
itshadowing withinit.files.all { f -> … }). Extracting it into a small named helper or local variables would make the intent clearer at a glance.♻️ Optional: break the condition into named parts
val isDetailed = localResult - ?.let { (it.chapters.isNotEmpty() || it.files.isNotEmpty()) && (it.files.isEmpty() || it.files.all { f -> f.size > 0 }) } + ?.let { book -> + val hasContent = book.chapters.isNotEmpty() || book.files.isNotEmpty() + val allFilesComplete = book.files.isEmpty() || book.files.all { it.size > 0 } + hasContent && allFilesComplete + } ?: falseapp/src/main/kotlin/org/grakovne/lissen/common/Moshi.kt (1)
18-28: Debug logging factory likely shouldn't ship in production.This
JsonAdapter.Factoryexists solely to log adapter creation. Even when no Timber debug tree is planted in release builds, the string interpolation on line 25 ("$type","${adapter.javaClass.simpleName}") still executes for every unique type Moshi resolves. Consider removing this before merging, or wrapping it behind aBuildConfig.DEBUGcheck if you want to keep it for development.♻️ Suggested removal
val moshi: Moshi = Moshi .Builder() - .add( - object : JsonAdapter.Factory { - override fun create( - type: Type, - annotations: Set<Annotation>, - moshi: Moshi, - ): JsonAdapter<*>? { - val adapter = moshi.nextAdapter<Any>(this, type, annotations) - Timber.d("Moshi Breadcrumb: Created adapter for type: $type. Adapter: ${adapter.javaClass.simpleName}") - return adapter - } - }, - ).add( + .add(app/src/main/kotlin/org/grakovne/lissen/common/NetworkService.kt (1)
85-121: Minor thread-safety note oncheckJobnow that the method is public.
checkJobis read/written from network-callback threads, thehostFlowcollector, and now any external caller. The cancel-and-reassign sequence on lines 88-89 isn't atomic, so two concurrent invocations could each launch a coroutine. The practical impact is just a redundant server check (theStateFlowemits are safe), so this is benign today, but worth noting.If you ever want to tighten this up, wrapping the cancel+launch in a
synchronizedblock or usingMutexwould eliminate the window.app/src/main/kotlin/org/grakovne/lissen/ui/screens/library/composables/MiniPlayerComposable.kt (2)
74-76:isPlaybackReadyis observed but never used in this composable.This creates an unnecessary recomposition trigger. Either use it (e.g., to disable the play/pause button when playback isn't ready) or remove the observer.
271-275: Use a short import instead of the fully-qualifiedandroidx.compose.material3.CircularProgressIndicator.Other Material 3 components (e.g.,
Icon,IconButton,Text) are already imported at the top of the file. Add an import forCircularProgressIndicatorand use the short name here.Proposed fix
Add to imports:
import androidx.compose.material3.CircularProgressIndicatorThen simplify:
- androidx.compose.material3.CircularProgressIndicator( + CircularProgressIndicator( modifier = Modifier.size(32.dp), color = colorScheme.onSurface, strokeWidth = 3.dp, )app/src/main/kotlin/org/grakovne/lissen/domain/update/UpdateCheckerService.kt (1)
126-130: Use top-level imports instead of fully-qualified names.Lines 126–130 use FQN references (
android.os.Build,androidx.core.content.ContextCompat,android.Manifest,android.content.pm.PackageManager) instead of importing them at the top of the file, unlike the rest of the code.app/src/main/kotlin/org/grakovne/lissen/playback/service/PlaybackService.kt (1)
64-64: Redundant fully-qualified name: useLazy<Cache>sincedagger.Lazyis already imported on Line 15.- lateinit var mediaCache: dagger.Lazy<Cache> + lateinit var mediaCache: Lazy<Cache>app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/ContentCachingNotificationService.kt (1)
29-44: Consider usingwhenor aSetinstead of creating a list for each status check.Lines 31 and 87 both create a new
listOf(...)on each invocation just for a membership test. Awhenexpression (or a pre-allocatedSet) is more idiomatic and avoids unnecessary allocations.♻️ Suggested refactor
val cachingItems = items - .filter { (_, state) -> listOf(CacheStatus.Caching, CacheStatus.Completed, CacheStatus.Queued).contains(state.status) } + .filter { (_, state) -> + state.status is CacheStatus.Caching || + state.status is CacheStatus.Completed || + state.status is CacheStatus.Queued + }Same pattern applies to line 87.
gradle/libs.versions.toml (1)
35-35: Consider upgrading to Microsoft Clarity Compose version 3.8.1 (released January 26, 2026), which fixes a Compose rendering edge case. Version 3.8.0 is valid and published, but 3.8.1 is the current release.app/build.gradle.kts (1)
69-69: Clarity project ID is hardcoded in the build file.This is comparable to a Google Analytics tracking ID — not a secret, but hardcoding it here means different environments (debug/release/staging) share the same project. Consider whether you want separate Clarity projects per build type. If so, you could define different values per
buildTypeblock.app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/PlaybackButtonsComposable.kt (1)
105-109: Use an import instead of the fully qualifiedandroidx.compose.material3.CircularProgressIndicator.There's already a mix of Material3 imports at the top of the file. Add
CircularProgressIndicatorto the imports and use the short name here.Proposed fix
Add to imports:
import androidx.compose.material3.CircularProgressIndicator- androidx.compose.material3.CircularProgressIndicator( + CircularProgressIndicator( modifier = Modifier.size(32.dp), color = colorScheme.onPrimary, strokeWidth = 3.dp, )app/src/main/kotlin/org/grakovne/lissen/common/HashUtils.kt (1)
5-11: Specify charset explicitly intoByteArray().
input.toByteArray()uses the platform default charset, which can vary. For deterministic hashing, useCharsets.UTF_8explicitly.Proposed fix
- .digest(input.toByteArray()) + .digest(input.toByteArray(Charsets.UTF_8))lib/src/main/kotlin/org/grakovne/lissen/lib/domain/DetailedItem.kt (1)
3-5: Unused import:com.squareup.moshi.JsonClassis still imported but no longer referenced.The
@JsonClass(generateAdapter = true)annotations were removed from all classes, but the import on line 4 remains. This is a minor leftover.🧹 Remove unused import
import androidx.annotation.Keep -import com.squareup.moshi.JsonClass import java.io.Serializableapp/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/ActionRow.kt (1)
82-91: Redundantenabledguard insideonClick.The
clickablemodifier already usesenabled = enabled, so theonClicklambda will never be invoked whenenabledisfalse. The innerif (enabled)check on line 87 is redundant.Simplify
onClick = { - if (enabled) { - haptic.performHapticFeedback(HapticFeedbackType.TextHandleMove) - onClick() - } + haptic.performHapticFeedback(HapticFeedbackType.TextHandleMove) + onClick() },app/src/main/kotlin/org/grakovne/lissen/analytics/ClarityTracker.kt (1)
41-54: Event name concatenation loses structured data and risks exceeding Clarity's event name length limit.
"$eventName: $value"merges the event name and payload into one string. This makes it harder to filter/aggregate in the Clarity dashboard and could silently truncate if Clarity imposes a character limit on event names.If Clarity's
sendCustomEventonly accepts a single string, consider at minimum truncating or sanitizing the combined value to a safe max length.app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/api/RecentRequestBuilder.kt (1)
35-50: WhendownloadedOnly = falsebuthost/usernameare not set, the query behaves identically todownloadedOnly = true.Lines 46–48: the fallback clause for missing host/username is the same EXISTS subquery used for
downloadedOnly = true(line 37). This means callers that setdownloadedOnly(false)without providing host/username will silently get download-filtered results. If this is intentional as a safety measure for cache isolation, a code comment would help clarify the intent.app/src/main/kotlin/org/grakovne/lissen/ui/components/DownloadProgressIcon.kt (1)
91-125: Duplicated rendering logic betweenCompletedandelsebranches.Lines 91–107 (
Completed) and 109–125 (else) are identical. Combining them would eliminate the duplication.♻️ Merge the two branches
- 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 -> { + 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, ) } }app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/advanced/cache/CacheSettingsScreen.kt (1)
106-118: Use proper imports instead of fully-qualified class names.
androidx.compose.ui.platform.LocalContext(line 106) andandroid.widget.Toast(line 114) should be imported at the top of the file, consistent with the rest of the file's style.♻️ Add imports and simplify usage
Add to the import block at the top of the file:
import android.widget.Toast import androidx.compose.ui.platform.LocalContextThen simplify the inline usages:
- val context = androidx.compose.ui.platform.LocalContext.current + val context = 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) + Toast + .makeText(context, successMessage, Toast.LENGTH_SHORT) .show() } }, )app/src/main/kotlin/org/grakovne/lissen/channel/audiobookshelf/library/converter/BookResponseConverter.kt (1)
80-80: Leftover debug log — consider removing or using parameterized format.This
Timber.dfires for every audio file on every book fetch, which is noisy. If it's still needed, importTimberand use format specifiers (Timber.d("Mapping file %s with size %s", it.ino, it.metadata.size)) to avoid string allocation when debug logging is disabled.♻️ Remove or clean up the debug log
If the log should be removed:
- timber.log.Timber.d("Mapping file ${it.ino} with size ${it.metadata.size}")If keeping it, add
import timber.log.Timberto the imports and use:- timber.log.Timber.d("Mapping file ${it.ino} with size ${it.metadata.size}") + Timber.d("Mapping file %s with size %s", it.ino, it.metadata.size)app/src/main/kotlin/org/grakovne/lissen/ui/screens/migration/MigrationScreen.kt (1)
42-50:LaunchedEffectordering:startMigration()should be invoked before observing forCompleted.Currently, the
LaunchedEffect(state)block (line 42) that checks forCompletedis declared before theLaunchedEffect(Unit)block (line 48) that starts the migration. While this works becauseLaunchedEffect(state)re-triggers on state changes, swapping the order would make the intent clearer: start first, then react.Suggested reorder
- LaunchedEffect(state) { - if (state is MigrationState.Completed) { - onMigrationComplete() - } - } - LaunchedEffect(Unit) { viewModel.startMigration() } + + LaunchedEffect(state) { + if (state is MigrationState.Completed) { + onMigrationComplete() + } + }app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/LocalCacheModule.kt (1)
53-53: Nit: inconsistent chain formatting.Line 53 chains
.addMigrations(MIGRATION_16_17)directly after the closing paren of the previous call, breaking the one-call-per-line pattern used elsewhere.Suggested format
.addMigrations( produceMigration15_16( host = preferences.getHost(), username = preferences.getUsername(), ), - ).addMigrations(MIGRATION_16_17) + ) + .addMigrations(MIGRATION_16_17) .addMigrations(MIGRATION_17_18) .addMigrations(MIGRATION_18_19)app/src/main/kotlin/org/grakovne/lissen/ui/navigation/AppNavHost.kt (1)
107-117: Consider importingMigrationScreenandMigrationViewModelinstead of using fully qualified names.Lines 58 and 112 use fully qualified references (
org.grakovne.lissen.viewmodel.MigrationViewModelandorg.grakovne.lissen.ui.screens.migration.MigrationScreen) while all other composables in this file are imported at the top. This is a minor inconsistency.Add imports
import org.grakovne.lissen.ui.screens.settings.advanced.cache.CachedItemsSettingsScreen import org.grakovne.lissen.ui.screens.settings.playback.PlaybackSettingsScreen +import org.grakovne.lissen.ui.screens.migration.MigrationScreen +import org.grakovne.lissen.viewmodel.MigrationViewModelThen replace FQN usages with short names.
app/src/main/kotlin/org/grakovne/lissen/persistence/preferences/LissenSharedPreferences.kt (2)
390-394: Swallowed exception loses diagnostic information.The
Throwableis caught and discarded. While returningnullfor corrupted JSON is the right fallback, silently swallowing the exception makes it harder to diagnose data corruption issues. Consider logging it.Log the exception
try { adapter.fromJson(json) } catch (e: Throwable) { + timber.log.Timber.w(e, "Failed to deserialize playing book") null }
113-115: Minor style inconsistency insetDatabaseVersion.This method uses
sharedPreferences.edit().putInt(...).apply()while most other setters in this class use theedit { }Kotlin extension (e.g., line 101, 121, etc.). Consider aligning for consistency.Use edit block
- 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/content/cache/persistent/CalculateRequestedChapters.kt (1)
50-55: Prefer imports over fully-qualified names.
SpecificFilesDownloadOptionandfindRelatedFilesByStartTimesare referenced with full package paths inline, reducing readability. Add them as imports at the top of the file.♻️ Proposed refactor
Add imports:
+import org.grakovne.lissen.content.cache.common.findRelatedFilesByStartTimes +import org.grakovne.lissen.lib.domain.SpecificFilesDownloadOptionThen simplify the branch:
- is org.grakovne.lissen.lib.domain.SpecificFilesDownloadOption -> - book.chapters.filter { chapter -> - org.grakovne.lissen.content.cache.common - .findRelatedFilesByStartTimes(chapter, fileStartTimes) - .any { it.id in option.fileIds } - } + is SpecificFilesDownloadOption -> + book.chapters.filter { chapter -> + findRelatedFilesByStartTimes(chapter, fileStartTimes) + .any { it.id in option.fileIds } + }app/src/main/kotlin/org/grakovne/lissen/viewmodel/LibraryViewModel.kt (1)
209-213: Analytics event fired on every keystroke.
updateSearchis called on each character the user types in the search field. Since there's no debounce,trackEvent("search_performed")will fire for every non-empty intermediate input (e.g., typing "Harry" triggers 5 events). Consider debouncing or tracking only on search submission to reduce noise.app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/Migrations.kt (1)
413-418: Empty migration for identity hash realignment.MIGRATION_18_19 has no SQL statements. While the comment explains the intent, this suggests the v18 schema's identity hash didn't match Room's expected hash, requiring a version bump to resync. This is a known Room workaround, but worth a brief doc comment to future maintainers explaining why the hash drifted (e.g., nullable vs NOT NULL mismatch in earlier versions).
app/src/main/kotlin/org/grakovne/lissen/viewmodel/SettingsViewModel.kt (2)
38-41: Use imports instead of fully-qualified class names in the constructor.
BookRepositoryandCachedCoverProviderare referenced by their fully-qualified names in the constructor signature. This is atypical for Kotlin and reduces readability. Add proper imports instead.♻️ Proposed refactor
Add imports:
+import org.grakovne.lissen.content.BookRepository +import org.grakovne.lissen.content.cache.temporary.CachedCoverProviderThen update the constructor:
- 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,
309-324:clearMetadataCache: CancellationException handling and FQN usage.The CancellationException rethrow pattern is correct for structured concurrency. However,
kotlinx.coroutines.CancellationExceptionandtimber.log.Timberare referenced by FQN inline — add them as imports for consistency.Also, the
onComplete()callback infinallyensures the UI is always notified regardless of success/failure, which is a good pattern.♻️ Proposed refactor for cleaner imports
Add imports at the top:
+import kotlinx.coroutines.CancellationException +import timber.log.TimberThen simplify the catch block:
- } catch (e: Exception) { - if (e is kotlinx.coroutines.CancellationException) { - throw e - } - - timber.log.Timber.e(e, "Failed to clear metadata cache") + } catch (e: Exception) { + if (e is CancellationException) throw e + Timber.e(e, "Failed to clear metadata cache")app/src/main/kotlin/org/grakovne/lissen/viewmodel/MigrationViewModel.kt (2)
34-40:retryMigrationallows re-execution fromCompletedstate.The guard only blocks
Running, so callingretryMigration()after a successful migration would re-execute it (re-running the DB query and re-setting the version). While not harmful, this is likely unintended. Consider also blocking theCompletedstate.♻️ Proposed fix
fun retryMigration() { - if (_migrationState.value == MigrationState.Running) { + if (_migrationState.value == MigrationState.Running || _migrationState.value == MigrationState.Completed) { return } executeMigration() }
56-62: Use imports forCancellationExceptionandTimberinstead of FQNs.Same FQN pattern seen in
SettingsViewModel. For consistency across the codebase, add proper imports.♻️ Proposed refactor
Add imports:
+import kotlinx.coroutines.CancellationException +import timber.log.TimberThen simplify:
- } catch (e: Exception) { - if (e is kotlinx.coroutines.CancellationException) { - throw e - } - - timber.log.Timber.e(e, "Migration failed") + } catch (e: Exception) { + if (e is CancellationException) throw e + Timber.e(e, "Migration failed")app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/ContentCachingManager.kt (1)
128-136: CancellationException handling and emptyfinallylook correct but thefinallyis dead weight.The CancellationException check and re-throw is the right pattern. The empty
finallyblock with just a comment can be removed for clarity.Proposed cleanup
} 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 }app/src/main/kotlin/org/grakovne/lissen/content/BookRepository.kt (2)
382-397:withContext(Dispatchers.IO)is redundant inside abackgroundScopealready onDispatchers.IO.
backgroundScopeat Line 47 is defined withDispatchers.IO. ThewithContext(Dispatchers.IO)at Line 390 adds unnecessary context switching overhead.Proposed simplification
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 -> - fetchBookCover(book.id, null) - delay(100) - yield() - } + books.forEach { book -> + fetchBookCover(book.id, null) + delay(100) + yield() } }
47-47: UnscopedCoroutineScopein a@Singleton— acceptable but worth documenting.Since
BookRepositoryis a@Singleton,backgroundScopelives for the entire process lifetime and is never cancelled. This is a well-known pattern for singletons, but a brief comment noting this intentional design would help future maintainers.app/src/main/kotlin/org/grakovne/lissen/viewmodel/CachingModelView.kt (1)
208-222:refreshMetadatafetches all cached items withInt.MAX_VALUEpage size.For users with large caches, this loads every
DetailedIteminto memory at once and re-caches each sequentially. Consider paginating or batching this work to limit memory pressure.app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/LocalCacheRepository.kt (3)
284-292:File.freeSpacemay overstate available space; preferFile.usableSpace.
freeSpaceincludes blocks reserved for the OS/root, which aren't usable by the app.usableSpacegives a more accurate number on Android devices.Proposed fix
fun getAvailableDiskSpace(): Long { val mediaFolder = storageProperties.baseFolder() - return mediaFolder.freeSpace + return mediaFolder.usableSpace }
292-292:StorageHeader(inCachedItemsSettingsScreen.kt) usesusedBytes / (usedBytes + freeBytes)for the progress bar.Since
usedBytesis only the app's cache size (not total disk usage by all apps), the bar will appear nearly empty even when the disk is almost full from other apps. If the intent is to show the disk usage proportion, consider using(totalBytes - freeBytes)for overall used and showing the cache portion within that.
166-184: Storage-type heuristic is reasonable but thefiles.size == chapters.sizeshortcut could misclassify.If a book has N files and N chapters but they don't have a 1:1 mapping (e.g., a chapter spans two files while two short chapters share another file), this would incorrectly return
ATOMIC. This is acknowledged as a heuristic, but worth a comment in code.app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/advanced/cache/CachedItemsSettingsScreen.kt (2)
339-391: Empty state composable is clean and provides a clear call-to-action.The
PolishedCachedItemsEmptyStatewith navigation to the library is well-designed. Minor note: the parameter is namedonBackbut it navigates to the library, not back.Suggested rename for clarity
-private fun PolishedCachedItemsEmptyState(onBack: () -> Unit) { +private fun PolishedCachedItemsEmptyState(onNavigateToLibrary: () -> Unit) { ... - onClick = onBack, + onClick = onNavigateToLibrary,
107-148:refreshContentcallsviewModel.refreshMetadata()on every pull-to-refresh, which re-caches all items.As noted in
CachingModelView.kt,refreshMetadatafetches every cached item (Int.MAX_VALUE) and re-processes them. On pull-to-refresh, this could cause noticeable delay for large caches. Consider whether a full metadata refresh is needed on every pull or only on explicit user action.app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/PlayerScreen.kt (2)
436-466:storageTypeandvolumesare not wrapped inremember, unlike inBookDetailScreen.In
BookDetailScreen.kt(lines 152–156, 253), bothgetVolumes(book)andgetBookStorageType(book)are wrapped inremember(...)to avoid recomputation on every recomposition. Here they're called directly, which may cause unnecessary work on each recompose.♻️ Suggested fix
playingBook?.let { book -> + val storageType = remember(book.id) { cachingModelView.getBookStorageType(book) } + val bookVolumes = remember(book, cacheProgress.status) { cachingModelView.getVolumes(book) } + DownloadsComposable( book = book, - storageType = cachingModelView.getBookStorageType(book), - volumes = cachingModelView.getVolumes(book), + storageType = storageType, + volumes = bookVolumes, isOnline = isOnline,
426-434:cacheProgressFlow is subscribed even whenplayingBookis null.When
downloadsExpandedis true butplayingBookis null,playingBook?.id.orEmpty()resolves to"", creating an unnecessary Flow subscription for an empty book ID. Consider moving this collection inside theplayingBook?.letblock to avoid wasted work.♻️ Suggested restructuring
if (downloadsExpanded) { - val cacheProgress by cachingModelView - .getProgress( - playingBook?.id.orEmpty(), - ).collectAsState( - initial = - org.grakovne.lissen.content.cache.persistent - .CacheState(org.grakovne.lissen.lib.domain.CacheStatus.Idle), - ) - playingBook?.let { book -> + val cacheProgress by cachingModelView + .getProgress(book.id) + .collectAsState( + initial = org.grakovne.lissen.content.cache.persistent + .CacheState(org.grakovne.lissen.lib.domain.CacheStatus.Idle), + ) + DownloadsComposable(app/src/main/kotlin/org/grakovne/lissen/analytics/ClarityComponent.kt (2)
61-74: Consider suppressing detekt'sEmptyFunctionBlockfor required interface stubs.These empty bodies are required by the
Application.ActivityLifecycleCallbacksinterface. You can silence the detekt warnings with a targeted suppression or by addingUnitas the body expression.♻️ Example using Unit bodies
- override fun onActivityStarted(activity: Activity) {} + override fun onActivityStarted(activity: Activity) = Unit - override fun onActivityResumed(activity: Activity) {} + override fun onActivityResumed(activity: Activity) = Unit - override fun onActivityPaused(activity: Activity) {} + override fun onActivityPaused(activity: Activity) = Unit - override fun onActivityStopped(activity: Activity) {} + override fun onActivityStopped(activity: Activity) = Unit override fun onActivitySaveInstanceState( activity: Activity, outState: Bundle, - ) {} + ) = Unit - override fun onActivityDestroyed(activity: Activity) {} + override fun onActivityDestroyed(activity: Activity) = Unit
26-33: Silent failure ifcontextis not anApplicationinstance.The
as?safe cast at line 32 silently skips registration ifcontextisn't anApplication. Since@ApplicationContextshould always injectApplication, this is defensive — but a log warning would help diagnose if it ever fails.♻️ Suggested improvement
override fun onCreate() { if (BuildConfig.DEBUG) { Timber.d("Skip Microsoft Clarity initialization for debug build") return } - (context as? Application)?.registerActivityLifecycleCallbacks(this) + val app = context as? Application + if (app == null) { + Timber.w("ClarityComponent: context is not Application, skipping lifecycle registration") + return + } + app.registerActivityLifecycleCallbacks(this) }app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/DownloadsComposable.kt (1)
88-88: Dense one-liner forcompletedVolumes— consider extracting for clarity.This line combines volume filtering, nested chapter iteration, and progress comparison in a single expression. Extracting it to a local helper or splitting across multiple lines would improve readability and make the completion logic easier to verify.
♻️ Suggested extraction
- val completedVolumes = volumes.filter { it.isDownloaded && it.chapters.all { ch -> (book.progress?.currentTime ?: 0.0) >= ch.end } } + val currentTime = book.progress?.currentTime ?: 0.0 + val completedVolumes = volumes.filter { volume -> + volume.isDownloaded && volume.chapters.all { ch -> currentTime >= ch.end } + }app/src/main/kotlin/org/grakovne/lissen/ui/screens/details/BookDetailScreen.kt (1)
596-629: RedundantstorageTypecomputation and shadowedbookvariable.
storageTypeis already computed at line 253 andbookis already assigned at line 252. Both are in scope here, so re-declaring them at lines 597–598 is redundant. The shadowedbookvariable is identical to the outer one (bookDetail!!), andstorageTypeuses the sameremember(book.id)key.♻️ Suggested simplification
- if (downloadsExpanded && bookDetail != null) { - val book = bookDetail!! - val storageType = remember(book.id) { cachingModelView.getBookStorageType(book) } - + if (downloadsExpanded && bookDetail != null) { DownloadsComposable( - book = book, + book = bookDetail!!, storageType = storageType, volumes = volumes,Or even simpler, since both
bookandstorageTypefrom the outer scope (line 252–253) are already available whenbookDetail != null.app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/dao/CachedBookDao.kt (2)
246-250:upsertBooksalways callsupdateBookson the full list, including rows just inserted.Unlike
safeUpsertBook(which conditionally updates only when insert fails), this batch version blindly updates all rows after inserting. For newly inserted rows, the subsequentUPDATEis a no-op but wastes I/O. Consider filtering to only update rows whose insert returned-1L.♻️ Suggested optimization
`@Transaction` suspend fun upsertBooks(books: List<BookEntity>) { - insertBooksIgnore(books) - updateBooks(books) + val insertResults = insertBooksIgnore(books) + val toUpdate = books.zip(insertResults) + .filter { (_, result) -> result == -1L } + .map { (book, _) -> book } + if (toUpdate.isNotEmpty()) { + updateBooks(toUpdate) + } }
44-44: Duration truncated fromDoubletoInt.
book.chapters.sumOf { it.duration }.toInt()drops fractional seconds. If chapter durations have meaningful sub-second precision (e.g.,12.5seconds), the total will accumulate truncation error. If theBookEntity.durationfield is intentionally integer seconds, this is fine — but worth confirming.
| val host: String? = null, | ||
| val username: String? = null, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's explore the file structure and understand the context
find . -name "CachedBookEntity.kt" -type fRepository: SurjitSahoo/kahani-android
Length of output: 163
🏁 Script executed:
#!/bin/bash
# Check how username is populated and whether it's logged anywhere
rg -n --type=kt 'username' -C2 -g '!*.json' | head -100Repository: SurjitSahoo/kahani-android
Length of output: 96
🏁 Script executed:
#!/bin/bash
# Look for CachedBookEntity definition and its full structure
fd CachedBookEntity.kt -x cat -n {}Repository: SurjitSahoo/kahani-android
Length of output: 4247
🏁 Script executed:
#!/bin/bash
# Search for where username is assigned or populated in Kotlin files
rg -n 'username\s*=' --type-list | head -5Repository: SurjitSahoo/kahani-android
Length of output: 459
🏁 Script executed:
#!/bin/bash
# Use a different approach - search for username assignments
rg -n 'username\s*=' -A2 -B2 2>/dev/null | head -100Repository: SurjitSahoo/kahani-android
Length of output: 10886
🏁 Script executed:
#!/bin/bash
# Search for logging patterns that might include username
rg -n '(Log\.|log|Timber|println).*username' -i 2>/dev/null | head -50Repository: SurjitSahoo/kahani-android
Length of output: 3345
🏁 Script executed:
#!/bin/bash
# Search for API calls or network operations involving these entities
rg -n 'BookEntity|MediaProgressEntity' -A3 -B1 | head -100Repository: SurjitSahoo/kahani-android
Length of output: 11489
🏁 Script executed:
#!/bin/bash
# Look for host and username usage context
rg -n 'host.*username|username.*host' -B3 -A3 2>/dev/null | head -80Repository: SurjitSahoo/kahani-android
Length of output: 10197
🏁 Script executed:
#!/bin/bash
# Search for ClarityComponent to see how username is used in analytics
cat -n app/src/main/kotlin/org/grakovne/lissen/analytics/ClarityComponent.kt | head -100Repository: SurjitSahoo/kahani-android
Length of output: 3486
🏁 Script executed:
#!/bin/bash
# Check how BookEntity and MediaProgressEntity are serialized/exposed
rg -n '@JsonClass|toJson|fromJson' -B2 -A2 | grep -A5 -B5 'BookEntity\|MediaProgressEntity' | head -50Repository: SurjitSahoo/kahani-android
Length of output: 5373
🏁 Script executed:
#!/bin/bash
# Look for data deletion or cleanup routines
rg -n 'clearData|deleteData|logout|resetUser' -B2 -A4 | head -80Repository: SurjitSahoo/kahani-android
Length of output: 1581
🏁 Script executed:
#!/bin/bash
# Check if there are any data retention policies or comments about PII
rg -n 'GDPR|PII|sensitive|credential|personal' -B1 -A1 | head -60Repository: SurjitSahoo/kahani-android
Length of output: 6165
🏁 Script executed:
#!/bin/bash
# Check the ClarityTracker implementation to see if username is hashed/masked
fd ClarityTracker.kt -x cat -n {}Repository: SurjitSahoo/kahani-android
Length of output: 1944
Username is being sent unmasked to Microsoft Clarity analytics, contradicting privacy claims.
The username field is indeed PII and requires special handling. While cascade delete is properly configured and credentials are cleared on logout, there's a more critical issue: the username (combined as $username@$host) is being sent unmasked to Microsoft Clarity's analytics service (ClarityComponent.kt:84-85). Although logs mask the identifier for display, the actual Clarity.setCustomUserId() call receives the plaintext username. This contradicts the analytics consent message stating "Your library and personal info stay 100% private." Additionally, username is logged plaintext via Timber (LissenMediaProvider.kt:292).
To address privacy and compliance concerns:
- Hash or anonymize the username before sending to Clarity, or use only device/session IDs
- Remove plaintext username logging
- Update privacy statements to accurately reflect what data is shared with analytics providers
🤖 Prompt for AI Agents
In
`@app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/entity/CachedBookEntity.kt`
around lines 127 - 128, The username field (CachedBookEntity.username) is being
sent and logged in plaintext; update the code that calls Clarity.setCustomUserId
(ClarityComponent) and the Timber log in LissenMediaProvider to send only an
irreversible pseudonymized identifier (e.g., SHA-256/HMAC of "$username@$host"
or a session/device ID) instead of the raw username, and stop logging the
plaintext username (log only masked or the hashed ID); ensure the hashing is
applied before any call to Clarity.setCustomUserId and replace the Timber
statement that prints username with one that prints the hashed/masked value, and
update privacy statement strings to reflect the new behavior.
app/src/main/kotlin/org/grakovne/lissen/domain/update/UpdateCheckerService.kt
Outdated
Show resolved
Hide resolved
app/src/main/kotlin/org/grakovne/lissen/playback/service/PlaybackService.kt
Outdated
Show resolved
Hide resolved
app/src/main/kotlin/org/grakovne/lissen/ui/components/AnalyticsConsentBottomSheet.kt
Outdated
Show resolved
Hide resolved
app/src/main/kotlin/org/grakovne/lissen/ui/screens/migration/MigrationScreen.kt
Outdated
Show resolved
Hide resolved
app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/DownloadsComposable.kt
Outdated
Show resolved
Hide resolved
...n/kotlin/org/grakovne/lissen/ui/screens/settings/advanced/cache/CachedItemsSettingsScreen.kt
Show resolved
Hide resolved
…efactor download option type references in player downloads.
…fine smart rewind logic in `PlaybackService`, and adjust coroutine execution in `PlaybackSynchronizationService`.
SurjitSahoo
changed the title
…nline success indicators.
…dProgressIcon` and remove the redundant inline checkmark from `BookDetailScreen`.
…a `Set` of ignored items, improve `LazyColumn` keying for better performance, refine scroll behavior, and update project context documentation.
… cached items are being removed.
…ils are being fetched by introducing an `isFetchingDetails` state in PlayerViewModel.
…d remove the now redundant `cacheBookInfo` function.
…ntegrate Firebase Analytics.
…th varying delays and enhance related logging for server status and library state.
…imestamps and minified book fetches to efficiently update local book data, and add `addedAt`/`updatedAt` fields to the `Book` model.
…fety, correct UI text, and refine mini-player state management.**
…iple language string resources
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/PlaybackButtonsComposable.kt (1)
88-119:⚠️ Potential issue | 🟡 MinorPlay button remains clickable during the loading state.
When
preparingBookId != null, theCircularProgressIndicatoris shown, but theIconButton(Line 88) is still enabled and itsonClickcallstogglePlayPause. Consider disabling the button while loading to prevent the user from toggling playback mid-preparation.Also,
CircularProgressIndicatoron Line 102 uses a fully-qualified name instead of an import — add a top-level import for consistency with the rest of the file.Suggested fix
Add import at the top of the file:
import androidx.compose.material3.CircularProgressIndicatorIconButton( onClick = { withHaptic(view) { viewModel.togglePlayPause() } }, modifier = Modifier.size(72.dp), + enabled = preparingBookId == null, ) { Surface( ... Box(contentAlignment = Alignment.Center) { val isLoading = preparingBookId != null if (isLoading) { - androidx.compose.material3.CircularProgressIndicator( + CircularProgressIndicator( modifier = Modifier.size(32.dp), color = colorScheme.onPrimary, strokeWidth = 3.dp, )app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/api/CachedBookRepository.kt (1)
223-243:⚠️ Potential issue | 🟡 Minor
fetchRecentBooksFlowcaptureshost/usernameeagerly at flow-creation time.Since this is a non-suspend function returning a
Flow,preferences.getHost()andpreferences.getUsername()are evaluated once when the flow is built (line 231–232), not when it's collected. If the user's session context changes while the flow is active, the query will use stale credentials.The other
fetchRecentBooks(suspend, line 199) has the same pattern but runs once per call, so it's fine. For the flow variant, consider building the request inside themapor using aflow { }builder that re-evaluates on each collection.app/src/main/kotlin/org/grakovne/lissen/playback/MediaRepository.kt (1)
574-592:⚠️ Potential issue | 🟡 MinorAnalytics calls in
play()/pause()lack error handling, unlikeAuthRepository.In
AuthRepository.onPostLogin, analytics calls are wrapped in try-catch to prevent analytics failures from disrupting core functionality. Here,clarityTracker.trackEvent(...)is called without protection. IfClarityTrackerthrows (e.g., Clarity not initialized in debug builds), it could prevent the playback intent from being dispatched or cause an unhandled exception.🛡️ Suggested fix for play()
context.startForegroundService(intent) - clarityTracker.trackEvent("playback_start") + try { clarityTracker.trackEvent("playback_start") } catch (_: Exception) {}Apply the same pattern for
pause()(line 591) andupdateTimer()(line 275).#!/bin/bash # Check if ClarityTracker methods have internal error handling ast-grep --pattern 'class ClarityTracker { $$$ }'app/src/main/res/values/strings.xml (1)
97-97:⚠️ Potential issue | 🟡 MinorPre-existing grammatical error: "No podcast has no episodes" (double negative).
This is likely intended to say something like "This podcast has no episodes" or "No episodes available."
✏️ Suggested fix
- <string name="episodes_list_empty">No podcast has no episodes</string> + <string name="episodes_list_empty">This podcast has no episodes</string>app/src/main/kotlin/org/grakovne/lissen/viewmodel/LibraryViewModel.kt (1)
127-139:⚠️ Potential issue | 🟡 MinorMultiple init-time collectors can trigger concurrent overlapping syncs.
This
isServerAvailablecollector (Line 130) triggerssyncRepositories()+refreshLibrary()when the server becomes reachable. Meanwhile, thelibraryPagerflow (Line 178-181) also callssyncLibrary(libraryId)whendownloadedOnlychanges (which depends onisServerAvailable). Both fire near-simultaneously on server availability change, leading to redundant concurrent sync operations.app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/ContentCachingManager.kt (1)
281-313:⚠️ Potential issue | 🔴 Critical
cacheBookCoveralways returnsCompleted, even when cover fetch fails.The
onFailureblock on Line 307-308 is empty, and execution falls through to Line 311 which unconditionally returnsCacheState(CacheStatus.Completed). This means:
- If
fetchBookCoverreturns an error →Completedis returned (bug).- If the
fold.onSuccessblock throws and catches via Line 302-304 →CacheState(CacheStatus.Error)is returned fromfold, but it's discarded because thefoldcall's return value isn't used as the function's return value — Line 311 still returnsCompleted.The
foldresult is never assigned or returned.🐛 Proposed fix
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) { Firebase.crashlytics.recordException(ex) return@fold CacheState(CacheStatus.Error) } }, onFailure = { + CacheState(CacheStatus.Error) }, ) - CacheState(CacheStatus.Completed) + result }
🤖 Fix all issues with AI agents
In `@app/google-services.json`:
- Around line 16-19: The committed Firebase/GCP API key is exposed in
google-services.json under the "api_key" -> "current_key" field; restrict that
key in the Google Cloud Console (Credentials) by applying Android app
restrictions (package name + SHA-1) and API restrictions to only needed Firebase
APIs, and if this repo is public remove or stop committing google-services.json
(or serve it via CI secrets) and consider enabling Firebase App Check for
additional protection; also review .gitignore to prevent accidental commits of
google-services.json going forward.
In `@app/src/main/kotlin/org/grakovne/lissen/analytics/ClarityTracker.kt`:
- Around line 41-54: The trackEvent function accepts a value parameter but never
uses it and appends a redacted suffix to the event name, which misleads callers
and pollutes Clarity event names; fix by removing the unused value parameter
from the signature of org.grakovne.lissen.analytics.ClarityTracker.trackEvent
and implement a single-argument overload that preserves existing behavior (i.e.,
call Clarity.sendCustomEvent(eventName) and Timber.d with eventName) and update
all callers to use the single-arg overload (or provide a two-arg overload that
delegates to the single-arg one if you need backward compatibility);
alternatively, if you intend to keep the parameter, explicitly use or sanitize
value before sending it via Clarity.sendCustomEvent and add a KDoc or
`@Suppress`("UNUSED_PARAMETER") if it must remain unused.
In `@app/src/main/kotlin/org/grakovne/lissen/common/CrashHandler.kt`:
- Around line 20-22: The current CrashHandler use of
FirebaseCrashlytics.getInstance().recordException(ex) records the throwable as a
non-fatal event and can duplicate the same uncaught crash that Crashlytics
already captures; update the CrashHandler (the code in CrashHandler.kt where
recordException is called) to either remove the recordException call and rely on
Crashlytics' default uncaught exception handler, or replace it with
FirebaseCrashlytics.getInstance().sendUnsentReports() (or another appropriate
send/flush method) to ensure delivery without creating a duplicate non-fatal
record, and keep any existing toast/delay logic intact so the crash is not
double-reported.
In `@app/src/main/kotlin/org/grakovne/lissen/common/NetworkService.kt`:
- Around line 96-135: The refreshServerAvailabilitySync() retry logic wrongly
re-applies the 500ms debounce on each recursive retry and uses an
instance-scoped initialRetryCount that is never reset, causing compounding
delays and one-shot retries; replace the recursion with a local-loop retry
inside refreshServerAvailabilitySync(): perform the initial
delay/isNetworkAvailable()/host null check once at the start, then attempt the
socket connect in a for/while loop that uses a local retry counter (reset on
each invocation) and delays 300ms between attempts, emit _isServerAvailable and
_networkStatus appropriately on success/failure, and remove the instance field
initialRetryCount entirely so each call gets its own retry budget.
In `@app/src/main/kotlin/org/grakovne/lissen/content/BookRepository.kt`:
- Around line 221-227: The current use of Int.MAX_VALUE in
localCacheRepository.fetchBooks (the knownBooks construction) can OOM for large
libraries; replace the single massive call with a paginated or streaming
approach: implement a bounded PAGE_SIZE constant and loop calling
localCacheRepository.fetchBooks(libraryId, PAGE_SIZE, offset, false) (or add/use
a dedicated repository method like
fetchAllBookIdsWithUpdatedAt/fetchBookIdsPage) accumulating items until no more
pages, then build the associateBy { it.id } map; ensure the loop/stream handles
failures (fold onFailure -> emptyList or propagate) and avoids loading all items
at once.
- Around line 177-192: The current syncLibraryPage function ignores its
parameters and always calls syncAllLibraries; change it to perform a targeted
page sync using the provided libraryId, pageSize, and pageNumber instead of
calling syncAllLibraries. Replace the backgroundScope.launch block to call a
suspend worker like syncLibraryPageTask(libraryId, pageSize, pageNumber) (or
create such a helper) and handle exceptions with Timber.e(e, "...") as before;
keep returning OperationResult.Success(Unit) immediately if you want
fire-and-forget semantics, or await the task and return a success/failure based
on the result if you want synchronous behavior. Ensure you reference the
existing function name syncLibraryPage and the launched coroutine
(backgroundScope.launch) when making the change so callers like
LibraryViewModel.refreshLibrary receive the intended behavior.
In
`@app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/LocalCacheModule.kt`:
- Around line 48-55: The migration call is passing empty-string fallbacks which
defeat the migration's null-handling; change the arguments to
produceMigration15_16 to pass the nullable results from preferences.getHost()
and preferences.getUsername() (i.e., remove the ?: "" fallbacks) so
produceMigration15_16 receives null when the user is logged out and can emit
NULL defaults as intended.
In `@app/src/main/kotlin/org/grakovne/lissen/playback/service/PlaybackService.kt`:
- Around line 69-73: The artworkJob field is written on the IO dispatcher inside
withContext(Dispatchers.IO) but read/cancelled on the Main thread, risking a
visibility race; fix by making the field volatile or by ensuring assignment
happens on the Main dispatcher: either annotate artworkJob with `@Volatile` var
artworkJob: kotlinx.coroutines.Job? = null, or move the assignment out of the IO
block so you set artworkJob on Dispatchers.Main (or wrap the assignment in
withContext(Dispatchers.Main)) before entering withContext(Dispatchers.IO);
update references to artworkJob (the cancel/read site and the assignment site
inside withContext(Dispatchers.IO)) accordingly.
- Around line 251-277: The artwork coroutine launched inside the
withContext(Dispatchers.IO) is still a structured-child and blocks
preparePlayback/playsbackJob; instead, run the fire-and-forget task on the
service-wide scope: call playerServiceScope.launch { val artworkUri =
withContext(Dispatchers.IO) { fetchCover(book) } ?: return@launch;
withContext(Dispatchers.Main) { /* update exoPlayer metadata loop */ } } and
assign that to artworkJob so you keep explicit cancellation, replacing the
current launch that is nested under withContext and keeping UI updates in
withContext(Dispatchers.Main); ensure you reference artworkJob,
fetchCover(book), playerServiceScope, and the existing loop that updates
exoPlayer.getMediaItemAt(i)/replaceMediaItem.
In
`@app/src/main/kotlin/org/grakovne/lissen/ui/components/AsyncShimmeringImage.kt`:
- Around line 37-43: The LaunchedEffect that sets isMainLoading and calls
onLoadingStateChanged races with Coil’s onSuccess; remove the LaunchedEffect and
keep isMainLoading initialized by remember(imageRequest) only, then add a
SideEffect (or snapshotFlow observing isMainLoading) that invokes
onLoadingStateChanged(true) only when isMainLoading transitions to true so a
later onSuccess handler (in onSuccess) can reliably set isMainLoading = false
and call onLoadingStateChanged(false) without being overwritten; reference the
variables and callbacks isMainLoading, remember(imageRequest), LaunchedEffect,
SideEffect/snapshotFlow, and the onSuccess block to locate and update the code.
In
`@app/src/main/kotlin/org/grakovne/lissen/ui/screens/library/composables/MiniPlayerComposable.kt`:
- Around line 76-77: The loading/error states are not gated to the current book;
compute local flags using the observed values so the spinner/error show only for
this book: derive val isLoading = preparingBookId == book.id and val isError =
preparingError && preparingBookId == book.id (observed from
playerViewModel.preparingBookId and preparingError) and replace usages that
currently check preparingBookId or preparingError directly with isLoading and
isError inside MiniPlayerComposable (where the spinner and error icon rendering
occur).
In
`@app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/advanced/cache/CachedItemsSettingsScreen.kt`:
- Around line 741-749: calculateReclaimSize and deleteSelectedVolumes currently
iterate 0 until cachedItems.itemCount and skip null entries, which silently
ignores selections whose pages aren't loaded; fix by maintaining a persistent
lookup (e.g., MutableMap<String, DetailedItem> renderedItemCache) that is
populated when items are bound/rendered (update in the composable or adapter
where items are accessed) and then use renderedItemCache.lookup(book.id) and
renderedItemCache.lookup(volumeId) inside calculateReclaimSize and
deleteSelectedVolumes (fall back to loading from viewModel.getVolumes(book) only
if the cache misses), or alternatively trigger loading of the necessary paging
pages before computing/deleting to guarantee presence — update the code paths
that reference cachedItems[i], calculateReclaimSize, and deleteSelectedVolumes
to use this cache/fallback so selections aren't skipped.
In `@app/src/main/kotlin/org/grakovne/lissen/viewmodel/LibraryViewModel.kt`:
- Around line 233-257: refreshLibrary currently triggers duplicate work by
calling bookRepository.syncLibraryPage(...) and then
bookRepository.syncRepositories(), which results in syncFullLibrary running
twice for the preferred library (via syncAllLibraries inside syncLibraryPage and
directly in syncRepositories). Fix by ensuring only one path performs a full
sync for the preferred library: either remove the call to
bookRepository.syncRepositories() when preferences.getPreferredLibrary() is
non-null (i.e., when syncLibraryPage is invoked for that library), or change
BookRepository.syncRepositories() to skip libraries already synced by
syncLibraryPage (e.g., accept an optional excludedLibraryId and avoid calling
syncFullLibrary for that id). Update refreshLibrary (function refreshLibrary)
and/or BookRepository.syncRepositories/syncAllLibraries/syncFullLibrary
accordingly so the preferred library is not synced twice.
In `@app/src/main/res/values/strings.xml`:
- Around line 277-278: The consent copy in resources (strings
analytics_consent_title and analytics_consent_message) incorrectly states
"anonymous" while ClarityComponent sends a SHA-256 hash of username@host
(pseudonymous identifier); update the strings to accurately reflect pseudonymous
or non‑personally‑identifiable data (e.g., replace "anonymous" with
"pseudonymous" or "non-personally-identifiable") and ensure messaging aligns
with ClarityComponent's hashing behavior and any privacy docs.
🟡 Minor comments (18)
app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/advanced/cache/CachedItemsSettingsScreen.kt-203-261 (1)
203-261:⚠️ Potential issue | 🟡 MinorBottom bar reclaim-size calculation is eagerly remembered — verify reactivity with paging.
totalSizeToReclaimat line 206 is wrapped inremember(selectedVolumes.toList(), cachedItems.itemCount). SincecalculateReclaimSizereads fromcachedItems[i](subject to the paging window concern noted above), the remembered value could be stale or incorrect if paging state changes withoutitemCountchanging. This is a downstream consequence of the paging-iteration pattern.Also,
Formatter.formatFileSizeat line 209 will display"0 B"for any volumes that couldn't be resolved, which may confuse users into thinking nothing will be freed.app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/advanced/cache/CachedItemsSettingsScreen.kt-574-589 (1)
574-589:⚠️ Potential issue | 🟡 MinorBook-level checkbox:
isFullySelectedis vacuously true when a book has zero downloaded volumes.If
downloadedVolumesis empty,bookVolumesis empty, andselectedVolumes.containsAll(emptyList())always returnstrue— rendering the checkbox as checked. This is a cosmetic edge case since the volumes section would also be empty, but it's worth a guard:- val isFullySelected = selectedVolumes.containsAll(bookVolumes) + val isFullySelected = bookVolumes.isNotEmpty() && selectedVolumes.containsAll(bookVolumes)app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/advanced/cache/CachedItemsSettingsScreen.kt-680-687 (1)
680-687:⚠️ Potential issue | 🟡 MinorFully-qualified
Checkboxis redundant; missing duplicate guard on add.Two small issues here:
androidx.compose.material3.Checkboxis already imported at line 46, so the FQN is unnecessary.selectedVolumes.add(identifier)on line 684 does not check for duplicates before adding, unlike the book-level checkbox at line 583 which doesif (!selectedVolumes.contains(it)) selectedVolumes.add(it). Rapid or repeatedonCheckedChange(true)calls could insert duplicate entries, inflating the reclaim-size calculation.Proposed fix
- if (selectionMode) { - androidx.compose.material3.Checkbox( - checked = isSelected, - onCheckedChange = { - val identifier = VolumeIdentifier(item.id, volume.id) - if (it) selectedVolumes.add(identifier) else selectedVolumes.remove(identifier) - }, + if (selectionMode) { + Checkbox( + checked = isSelected, + onCheckedChange = { + val identifier = VolumeIdentifier(item.id, volume.id) + if (it) { + if (!selectedVolumes.contains(identifier)) selectedVolumes.add(identifier) + } else { + selectedVolumes.remove(identifier) + } + },app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/advanced/cache/CachedItemsSettingsScreen.kt-226-237 (1)
226-237:⚠️ Potential issue | 🟡 MinorDelete button
onClicklaunches a coroutine but doesn't guard against double-tap.
isDeletingprevents a second press viaenabled = !isDeleting, butisDeletingis set inside the launched coroutine (line 230). Between the click and the coroutine dispatching, a rapid second tap could launch a duplicate deletion. Consider settingisDeleting = truebeforescope.launchor using aLaunchedEffect-style guard.Proposed fix
onClick = { withHaptic(view) { + if (isDeleting) return@withHaptic + isDeleting = true scope.launch { - isDeleting = true deleteSelectedVolumes(selectedVolumes, cachedItems, viewModel, playerViewModel) selectionMode = false selectedVolumes.clear() refreshContent(false) isDeleting = false } } },app/src/main/kotlin/org/grakovne/lissen/playback/service/PlaybackSynchronizationService.kt-94-131 (1)
94-131:⚠️ Potential issue | 🟡 MinorStale state captured outside mutex can cause redundant session creation.
Lines 98–100 snapshot
currentItem,playbackSession, andcurrentChapterIndexon the Main thread, but the mutex is only acquired later on the IO coroutine (line 105). Between the snapshot and the lock acquisition, anotherrunSyncinvocation may have already opened a new session and updatedplaybackSession. The second invocation still sees the stalenullsnapshot and opens a duplicate session.Consider re-reading
playbackSessionandcurrentChapterIndexinside the mutex-guarded section (e.g., viawithContext(Dispatchers.Main)) or marking them@Volatileso the IO coroutine sees the latest value:Suggested approach: re-read state after acquiring lock
try { val currentIndex = calculateChapterIndex(item, overallProgress.currentTotalTime) - var activeSession = session - if (activeSession == null || activeSession.itemId != item.id || currentIndex != lastChapterIndex) { + var activeSession = withContext(Dispatchers.Main) { playbackSession } + val lastIdx = withContext(Dispatchers.Main) { currentChapterIndex } + if (activeSession == null || activeSession.itemId != item.id || currentIndex != lastIdx) { activeSession = openPlaybackSession(item, overallProgress) withContext(Dispatchers.Main) { playbackSession = activeSession currentChapterIndex = currentIndex } }app/src/main/kotlin/org/grakovne/lissen/common/CrashHandler.kt-18-44 (1)
18-44:⚠️ Potential issue | 🟡 MinorTiming race: toast may not be visible before the process is killed.
Thread.sleep(TOAST_TIMEOUT)sleeps 1000ms, thenfinallydelegates todefaultHandlerwhich typically terminates the process. Meanwhile, the toast thread's looper is scheduled to quit after 1500ms. The toast may barely flash before the process dies.Consider increasing
TOAST_TIMEOUTto at least match the toast visibility window (e.g., 2000–2500ms), or switch toToast.LENGTH_SHORTwhose actual duration (~2s) is closer to the sleep budget.app/src/main/kotlin/org/grakovne/lissen/ui/screens/library/composables/MiniPlayerComposable.kt-265-292 (1)
265-292:⚠️ Potential issue | 🟡 MinorButton remains interactive during loading and error states; hardcoded content description.
The
IconButtononClickstill firestogglePlayPausewhile the spinner or error icon is displayed. During loading, tapping should be a no-op (or disabled). During the error state, you may want a different action (e.g., retry) rather thantogglePlayPause.Line 280:
"Error"is a hardcoded English string. Use a string resource for accessibility and localization.Suggested fix
IconButton( - onClick = { withHaptic(view) { playerViewModel.togglePlayPause() } }, + onClick = { + if (preparingBookId == null && !preparingError) { + withHaptic(view) { playerViewModel.togglePlayPause() } + } + }, + enabled = preparingBookId == null && !preparingError, ) {Icon( imageVector = Icons.Rounded.Error, - contentDescription = "Error", + contentDescription = stringResource(R.string.playback_error), tint = colorScheme.error, modifier = Modifier.size(32.dp), )app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/PlaybackButtonsComposable.kt-44-45 (1)
44-45:⚠️ Potential issue | 🟡 MinorCompare
preparingBookIdagainstbook?.idto scope the loading state to the current book.The loading indicator should only display when preparing the current book shown in this composable. Since
bookis already observed (line 41), usepreparingBookId == book?.idinstead ofpreparingBookId != null. This matches howBookDetailScreen.ktcorrectly handles the same state, showing the spinner only when preparing that specific book.gradle/libs.versions.toml-34-37 (2)
34-37:⚠️ Potential issue | 🟡 MinorUpdate Firebase BOM and Crashlytics plugin to latest versions.
firebaseBomshould be34.9.0(current:33.9.0, released February 5, 2026)firebaseCrashlyticsPluginshould be3.0.6(current:3.0.3)
34-37:⚠️ Potential issue | 🟡 MinorUpdate dependency versions to latest available releases.
All specified versions are published and valid, but each has a newer version available:
- clarity: 3.8.0 → 3.8.1 (Jan 26, 2026)
- googleServices: 4.4.2 → 4.4.4 (Oct 9, 2025)
- firebaseCrashlyticsPlugin: 3.0.3 → 3.0.6
- firebaseBom: 33.9.0 → 34.7.0
Update to these current versions to gain latest bug fixes and feature support.
app/src/main/kotlin/org/grakovne/lissen/channel/audiobookshelf/common/api/AudioBookshelfRepository.kt-88-98 (1)
88-98:⚠️ Potential issue | 🟡 MinorHardcoded
pageSize = 10000will silently truncate results for large libraries.If a library contains more than 10,000 items, the excess will be silently dropped. Consider either paginating until all results are fetched, or at minimum extracting this into a named constant and documenting the assumption. The same applies to
fetchPodcastMinifiedon Line 123.Suggested constant extraction
+ companion object { + /** Upper bound for minified single-page fetches. Libraries exceeding this size will be truncated. */ + private const val MAX_MINIFIED_PAGE_SIZE = 10_000 + } + suspend fun fetchLibraryMinified(libraryId: String): OperationResult<LibraryItemsResponse> = audioBookShelfApiService.makeRequest { it.fetchLibraryItems( libraryId = libraryId, - pageSize = 10000, + pageSize = MAX_MINIFIED_PAGE_SIZE, pageNumber = 0, sort = "addedAt", desc = "1", minified = "1", ) }app/src/main/kotlin/org/grakovne/lissen/ui/components/DownloadProgressIcon.kt-62-71 (1)
62-71:⚠️ Potential issue | 🟡 Minor
wasCachingis never reset when the download is completed butisFullyDownloadedis false.If the status transitions from
Caching→CompletedwhileisFullyDownloadedisfalse(partial download),wasCachingstaystrueindefinitely. IfisFullyDownloadedlater flips totruewithout passing throughCachingagain, theLaunchedEffectwon't re-fire (it keys oncacheState.status, notisFullyDownloaded), so the shine won't trigger. This is likely harmless but meanswasCachingleaks state across download cycles.Consider resetting
wasCachingin theCompleted/Idlepath regardless ofisFullyDownloaded:if (wasCaching && (cacheState.status is CacheStatus.Completed || cacheState.status is CacheStatus.Idle) && isFullyDownloaded) { triggerShine = true - wasCaching = false } + if (wasCaching && (cacheState.status is CacheStatus.Completed || cacheState.status is CacheStatus.Idle)) { + wasCaching = false + }app/src/main/kotlin/org/grakovne/lissen/persistence/preferences/LissenSharedPreferences.kt-392-396 (1)
392-396:⚠️ Potential issue | 🟡 MinorSwallowed exception — consider logging or reporting the deserialization failure.
The
catch (e: Throwable)silently discards the error. Sincedecrypt()already reports exceptions to Crashlytics, applying the same pattern here would help diagnose corrupted preference data in production.🐛 Suggested fix
try { adapter.fromJson(json) } catch (e: Throwable) { + Firebase.crashlytics.recordException(e) null }app/src/main/kotlin/org/grakovne/lissen/LissenApplication.kt-44-54 (1)
44-54:⚠️ Potential issue | 🟡 MinorComponent error handler may fail silently if core services initialization failed.
If the try-catch on lines 23–42 catches an exception (e.g., Crashlytics not available), then the error handling in lines 49–52 relies on both
Timber(which has no tree planted in release, and none at all if DEBUG init failed) andFirebaseCrashlytics.getInstance()(which could also throw). Consider wrapping lines 50–52 in their own try-catch to avoid masking component failures.🛡️ Suggested defensive fix
try { it.onCreate() } catch (ex: Exception) { Timber.e(ex, "Unable to register Running component: ${ex.message}") - com.google.firebase.crashlytics.FirebaseCrashlytics - .getInstance() - .recordException(ex) + try { + com.google.firebase.crashlytics.FirebaseCrashlytics + .getInstance() + .recordException(ex) + } catch (crashEx: Exception) { + android.util.Log.e("LissenApplication", "Failed to record exception to Crashlytics", crashEx) + } }app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/ContentCachingService.kt-121-128 (1)
121-128:⚠️ Potential issue | 🟡 MinorNotification updates are suppressed while any prior item has errored, even if other items are still caching.
When one item errors and another is still in progress,
hasErrors()returnstrue, making the guardinProgress() && hasErrors().not()evaluate tofalse. Non-terminal progress emissions will skip the notification update, leaving the user with a stale notification until the remaining items reach a terminal state.If the intent is to keep updating the notification for actively-caching items regardless of prior errors, drop the
hasErrors()guard from the in-progress branch:🔧 Proposed fix
- if (shouldUpdateNotification && (isTerminalState || (inProgress() && hasErrors().not()))) { + if (shouldUpdateNotification && (isTerminalState || inProgress())) {app/src/main/kotlin/org/grakovne/lissen/content/BookRepository.kt-245-270 (1)
245-270:⚠️ Potential issue | 🟡 MinorUnbounded fire-and-forget cover prefetch inside chunked loop.
Each chunk spawns a
backgroundScope.launch { prefetchCovers(...) }that is completely detached. For a library with many updated books, this creates multiple concurrent cover-prefetch tasks that all independently delay 2 seconds and then fetch sequentially. There is no concurrency limit, no cancellation propagation from the parent sync, and no backpressure. IfsyncFullLibraryis called again before the previous prefetches finish, they pile up.Consider collecting all books needing covers and launching a single prefetch at the end of
syncFullLibrary, or use a bounded dispatcher/semaphore.app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/ContentCachingManager.kt-115-130 (1)
115-130:⚠️ Potential issue | 🟡 MinorAll-or-nothing rollback on partial failure may discard successfully downloaded media.
If the cover or library cache fails (e.g., transient network issue for a small cover image), the entire media download is rolled back (Line 127), even though
cacheBookMediamay have successfully downloaded large audio files. Consider treating cover/library failures as non-fatal — the media is the primary artifact users care about.app/src/main/kotlin/org/grakovne/lissen/content/BookRepository.kt-471-480 (1)
471-480:⚠️ Potential issue | 🟡 Minor
toBook()omitsaddedAtandupdatedAtfields from the mappedBook.The
Bookmodel reportedly hasaddedAt/updatedAtfields (per commit messages and the incremental sync logic on Line 235), but this mapper doesn't set them. This means books created via this mapper will have default values for these timestamp fields, which could cause issues if they're subsequently used in comparisons likeremote.updatedAt > local.updatedAt.#!/bin/bash # Check the Book data class to see if addedAt/updatedAt have defaults ast-grep --pattern $'data class Book( $$$ )'
🧹 Nitpick comments (33)
app/src/main/kotlin/org/grakovne/lissen/viewmodel/PlayerViewModel.kt (1)
158-167: Consider catching exceptions fromfetchBookto avoid unhandled coroutine failures.The
finallyblock correctly resets the loading flag, which is good. However, ifmediaRepository.fetchBook(bookId)throws, the exception propagates unhandled inviewModelScope.launch, which could surface as a crash. This applies to otherlaunchcalls in this file too, but since this one drives a user-visible loading state, handling the error (e.g., exposing a fetch-error state) would improve UX.♻️ Optional: add error handling
fun fetchBook(bookId: String) { viewModelScope.launch { try { _isFetchingDetails.postValue(true) mediaRepository.fetchBook(bookId) + } catch (e: Exception) { + // Consider exposing an error state to the UI + timber.e(e, "Failed to fetch book details") } finally { _isFetchingDetails.postValue(false) } } }app/src/main/kotlin/org/grakovne/lissen/common/Moshi.kt (1)
31-42: Debug logging factory should not ship in production code.This
JsonAdapter.Factoryis a diagnostic/breadcrumb tool that intercepts every adapter creation. Two concerns:
- String template is always evaluated —
"Moshi Breadcrumb: Created adapter for type: $type. Adapter: ${adapter.javaClass.simpleName}"performstoString()and reflection even when no Timber tree is planted in release builds.- Debug artifact in the adapter chain — it adds unnecessary complexity to the Moshi builder for no functional benefit.
If this was added to diagnose a specific issue, consider removing it now that the issue is resolved, or at minimum guard it behind a
BuildConfig.DEBUGcheck.Suggested fix: remove the debug factory
- ).add( - object : JsonAdapter.Factory { - override fun create( - type: Type, - annotations: Set<Annotation>, - moshi: Moshi, - ): JsonAdapter<*>? { - val adapter = moshi.nextAdapter<Any>(this, type, annotations) - Timber.d("Moshi Breadcrumb: Created adapter for type: $type. Adapter: ${adapter.javaClass.simpleName}") - return adapter - } - }, - ).addLast(KotlinJsonAdapterFactory()) + ).addLast(KotlinJsonAdapterFactory())Also remove the unused imports if this factory is dropped:
-import timber.log.Timber -import java.lang.reflect.Typeapp/src/main/kotlin/org/grakovne/lissen/ui/components/VerticalScrollbar.kt (1)
98-102: Thetry-catchdoesn't protect the draw-phase lambda.
baseScrollbarreturns aModifierviacomposed { ... drawWithContent { ... } }. The lambda body (lines 61–96) executes during the draw phase, well afterwithScrollbarreturns — so thistry-catchnever catches exceptions from the actual scrollbar computation. The Crashlytics call on line 100 will only fire for errors during Modifier chain assembly, which is unlikely to fail.This is pre-existing, but worth noting: if you want crash reports from the draw-time logic, the
try-catch(andrecordException) should be moved inside theonDrawlambda passed tobaseScrollbar.♻️ Suggested: move error handling inside the draw lambda
fun Modifier.withScrollbar( state: LazyListState, color: () -> Color, totalItems: Int?, ignoreItems: Set<String> = emptySet(), ): Modifier { - try { return baseScrollbar { atEnd -> + try { val layoutInfo = state.layoutInfo ... drawScrollbarThumb(atEnd, thumbSize, startOffset, color()) + } catch (ex: Exception) { + Timber.w("Unable to draw scrollbar due to ${ex.message}") + Firebase.crashlytics.recordException(ex) } - } catch (ex: Exception) { - Timber.w("Unable to apply scrollbar due to ${ex.message}") - Firebase.crashlytics.recordException(ex) - return this } }app/src/main/kotlin/org/grakovne/lissen/domain/update/UpdateCheckerService.kt (2)
31-36: Consider injecting a sharedOkHttpClientinstead of creating a dedicated instance.If OkHttp is already used elsewhere in the app (e.g., for API calls to the Audiobookshelf server), each standalone
OkHttpClientmaintains its own connection pool and dispatcher threads. Injecting a shared instance via Hilt would reduce resource overhead.
136-141: Nit: Use imports instead of fully-qualified names for consistency.
Buildis already imported on Line 9, yet Line 136 usesandroid.os.Build. Similarly,ContextCompat,Manifest, andPackageManagercould be imported at the top of the file to reduce visual noise here.Proposed fix
Add to imports:
import android.Manifest import android.content.pm.PackageManager import androidx.core.content.ContextCompatThen simplify the check:
- if (android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.TIRAMISU) { - if (androidx.core.content.ContextCompat.checkSelfPermission( - context, - android.Manifest.permission.POST_NOTIFICATIONS, - ) == android.content.pm.PackageManager.PERMISSION_GRANTED - ) { + if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) { + if (ContextCompat.checkSelfPermission( + context, + Manifest.permission.POST_NOTIFICATIONS, + ) == PackageManager.PERMISSION_GRANTED + ) {app/src/main/kotlin/org/grakovne/lissen/ui/screens/migration/MigrationScreen.kt (1)
97-111: AddBackHandlerto prevent back-press interruption during migration.The migration screen currently lacks back-press handling. While the dismiss button was intentionally removed to prevent bypassing migration (lines 93-94), a user can still press the system back button during the
Idle,Running, orCompletedstates to escape the screen prematurely. TheclearHistory = truecall inAppNavHost(line 114) only takes effect after migration completes, offering no protection during the process.Other screens in the codebase already use
BackHandler(PlayerScreen, GlobalPlayerBottomSheet, LibraryScreen), establishing the pattern. Add it here to suppress back navigation until migration completes:Suggested implementation
+import androidx.activity.compose.BackHandler + `@Composable` fun MigrationScreen( onMigrationComplete: () -> Unit, viewModel: MigrationViewModel = hiltViewModel(), ) { val state by viewModel.migrationState.observeAsState(MigrationState.Idle) + + BackHandler(enabled = state !is MigrationState.Completed) { + // Intentionally no-op: prevent navigating away during migration + }app/src/main/kotlin/org/grakovne/lissen/common/NetworkService.kt (1)
88-94:refreshServerAvailabilitySync()is public but not safe for concurrent callers.
refreshServerAvailability()cancels the previouscheckJobto ensure single-flight execution. But sincerefreshServerAvailabilitySync()is also public, an external caller can invoke it concurrently, causing two coroutines to race on_isServerAvailableemissions (andinitialRetryCountif it remains an instance field).If external callers need a synchronous check, consider either:
- Making
refreshServerAvailabilitySync()internal/privateand having the public path go throughrefreshServerAvailability(), or- Using a
Mutexto serialize concurrent executions.Also applies to: 96-96
app/src/main/kotlin/org/grakovne/lissen/playback/service/PlaybackSynchronizationService.kt (2)
143-154:currentChapterIndexnot updated on NotFoundError session recovery.When a
NotFoundErrortriggers session re-creation (line 147–150),playbackSessionis updated on Main butcurrentChapterIndexis not, unlike the session-creation path inrunSync(lines 116–119). While this doesn't cause a functional bug (the nextrunSyncwill reconcile), it is inconsistent and could cause one redundant session re-open on the following sync tick if the chapter index has shifted.Suggested fix for consistency
OperationError.NotFoundError -> { val newSession = openPlaybackSession(item, overallProgress) withContext(Dispatchers.Main) { playbackSession = newSession + currentChapterIndex = calculateChapterIndex(item, overallProgress.currentTotalTime) } }
94-108: Nestedlaunch(Dispatchers.IO)insideserviceScope.launch: clarify intent with a comment.The two-layer coroutine structure — outer launch on Main (to safely read ExoPlayer state) and inner launch on IO (for network/sync) — is correct but non-obvious to future readers. A brief inline comment on the outer launch (e.g., "// Capture player state on Main, then sync on IO") would help readability.
app/src/main/kotlin/org/grakovne/lissen/playback/service/PlaybackService.kt (1)
64-65: Nit: Redundant fully-qualified type onmediaCachedeclaration.Line 15 already imports
dagger.Lazy, so the field can useLazy<Cache>directly.- lateinit var mediaCache: dagger.Lazy<Cache> + lateinit var mediaCache: Lazy<Cache>app/src/main/kotlin/org/grakovne/lissen/common/CrashHandler.kt (1)
40-41: Swallowed exception in crash handler (detekt: SwallowedException).While silencing exceptions here is defensively reasonable, consider logging to
TimberorLog.eso crash-handler failures are diagnosable from logcat during development. This won't affect production safety.Suggested fix
} catch (e: Exception) { - // Ignore errors in the crash handler itself to avoid infinite loops + // Log but don't rethrow to avoid infinite loops + android.util.Log.e("CrashHandler", "Error in crash handler", e) } finally {lib/src/main/kotlin/org/grakovne/lissen/lib/domain/DownloadOption.kt (1)
15-17:SpecificFilesDownloadOptionis a plain class — no structural equality.This is consistent with
NumberItemDownloadOption, but worth noting: two instances with the samefileIdswon't be==. If this type is ever compared, stored in a set, or used as a map key, consider making it adata class. Not blocking if the current usage doesn't require equality.app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/entity/CachedBookEntity.kt (1)
108-128:@Keepannotation removed fromMediaProgressEntitybut retained on all other entities.All other entity classes (
BookEntity,BookFileEntity,BookChapterEntity,BookSeriesDto, and the wrapperCachedBookEntity) still carry@Keep. While@JsonClass(generateAdapter = true)uses codegen rather than reflection (so@Keepisn't strictly required), the inconsistency could cause confusion or subtle issues if ProGuard/R8 rules change. Consider either removing@Keepfrom all entities or restoring it here for consistency.app/src/main/kotlin/org/grakovne/lissen/analytics/ClarityTracker.kt (1)
4-4: Unused import:ClarityConfig.
ClarityConfigis imported but never referenced in this file.app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/LocalCacheRepository.kt (2)
158-231: Excessive fully-qualified names hurt readability throughout the new methods.Types like
BookStorageType,BookVolume,BookFile, and the already-importedfindRelatedFiles(Line 9) are referenced with their full package paths repeatedly (e.g., Lines 168, 172–190, 195–231, 256–257). Adding the missing imports at the top of the file would make this code much cleaner.Add imports and simplify references
Add these imports at the top:
import org.grakovne.lissen.lib.domain.BookFile import org.grakovne.lissen.lib.domain.BookStorageType import org.grakovne.lissen.lib.domain.BookVolumeThen replace all fully-qualified references, e.g.:
- fun getBookStorageType(book: DetailedItem): org.grakovne.lissen.lib.domain.BookStorageType { - if (book.files.size <= 1) return org.grakovne.lissen.lib.domain.BookStorageType.MONOLITH + fun getBookStorageType(book: DetailedItem): BookStorageType { + if (book.files.size <= 1) return BookStorageType.MONOLITHAnd on Line 168, use the already-imported
findRelatedFileswithout FQN.
293-301:File.freeSpace/totalSpacemay be inaccurate on modern Android with scoped storage.On API 26+,
StorageStatsManagerorStatFsprovide more reliable disk space metrics that account for storage quotas.File.freeSpacecan report misleading values on some devices. Worth noting if accuracy matters for UX decisions.app/src/main/kotlin/org/grakovne/lissen/ui/components/DownloadProgressIcon.kt (1)
156-190:Completedandelsebranches are identical — extract to a shared helper.The icon-rendering logic for
CacheStatus.Completed(lines 156–172) and theelsebranch (lines 174–190) are exact duplicates. Consider extracting a local composable or inline lambda to avoid the 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 = successColor, - ) - } 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 = successColor, - ) - } else { - Icon( - imageVector = Icons.Outlined.CloudDownload, - contentDescription = stringResource(R.string.accessibility_id_download_available), - modifier = Modifier.size(size).alpha(0.8f), - tint = color, - ) - } - } + is CacheStatus.Completed, else -> { + if (isFullyDownloaded) { + Icon( + imageVector = Icons.Filled.CloudDone, + contentDescription = stringResource(R.string.accessibility_id_download_complete), + modifier = Modifier.size(size), + tint = successColor, + ) + } else { + Icon( + imageVector = Icons.Outlined.CloudDownload, + contentDescription = stringResource(R.string.accessibility_id_download_available), + modifier = Modifier.size(size).alpha(0.8f), + tint = color, + ) + } + }app/build.gradle.kts (1)
65-65: Consider externalizing the Clarity project ID.The Clarity project ID
"vc8bgk8nk9"is hardcoded directly in the build file. While analytics project IDs aren't secret, consider moving it tolocal.propertiesorgradle.propertiesfor easier configuration across environments (e.g., separate IDs for debug/release builds or different flavors).app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/api/CachedBookRepository.kt (1)
61-71: Consider wrappingdeleteNonDownloadedBooksin a transaction.The file deletion (lines 64–68) and DB deletion (line 70) are not atomic. If the process is interrupted between them, orphaned DB records (pointing to deleted files) could remain. Wrapping the DAO call and file I/O, or at minimum the two DAO operations, in a coordinated block would improve resilience.
app/src/main/kotlin/org/grakovne/lissen/analytics/ClarityComponent.kt (2)
94-98: Duplicate SHA-256 hashing logic — reuseHashUtils.sha256.
AuthRepositoryalready usesorg.grakovne.lissen.common.sha256(...)for the same purpose (hashingusername@host). This localhashIdentifiermethod duplicates that utility.♻️ Suggested fix
- 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 rawIdentifier = "$username@$host" - val hashedIdentifier = hashIdentifier(rawIdentifier) - - clarityTracker.setUser(hashedIdentifier) + private fun reidentifyUser() { + if (preferences.hasCredentials()) { + val username = preferences.getUsername() ?: return + val host = preferences.getHost() ?: return + + val hashedIdentifier = org.grakovne.lissen.common.sha256("$username@$host") + clarityTracker.setUser(hashedIdentifier) } else { // Fallback to device ID if not logged in clarityTracker.setUser(preferences.getDeviceId()) } } - - private fun hashIdentifier(input: String): String { - val digest = java.security.MessageDigest.getInstance("SHA-256") - val hashBytes = digest.digest(input.toByteArray(Charsets.UTF_8)) - return hashBytes.joinToString("") { "%02x".format(it) } - }#!/bin/bash # Verify HashUtils.sha256 exists and its signature ast-grep --pattern 'fun sha256($$$) : String { $$$ }'
61-74: Empty lifecycle callbacks are interface-mandated — detekt warnings are benign here.These no-op overrides are required by
Application.ActivityLifecycleCallbacks. Consider suppressing the detekt warning at the class level to avoid noise.♻️ Optional suppression
Add above the class declaration:
`@Suppress`("EmptyFunctionBlock")app/src/main/kotlin/org/grakovne/lissen/content/LissenMediaProvider.kt (1)
238-241: Dense predicate — consider extracting to a named function for readability.The compound condition on line 240 correctly ensures files (if present) all have a populated size, but it's hard to parse at a glance. A small helper like
DetailedItem.hasCompleteMetadata()would make the intent self-documenting.♻️ Suggested extraction
+ private fun DetailedItem.hasCompleteMetadata(): Boolean { + val hasContent = chapters.isNotEmpty() || files.isNotEmpty() + val filesValid = files.isEmpty() || files.all { it.size > 0 } + return hasContent && filesValid + } + suspend fun fetchBook(bookId: String): OperationResult<DetailedItem> { Timber.d("Fetching Detailed book info for $bookId") val localResult = localCacheRepository.fetchBook(bookId) - val isDetailed = - localResult - ?.let { (it.chapters.isNotEmpty() || it.files.isNotEmpty()) && (it.files.isEmpty() || it.files.all { f -> f.size > 0 }) } - ?: false + val isDetailed = localResult?.hasCompleteMetadata() ?: falseapp/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/ContentCachingService.kt (1)
37-38: Prefer top-level imports over fully qualified inline references.
java.util.concurrent.ConcurrentHashMapandjava.util.concurrent.atomic.AtomicLongare used inline. Importing them at the top keeps the code consistent with the rest of the file's style.♻️ Suggested cleanup
Add to the import block:
import java.util.concurrent.ConcurrentHashMap import java.util.concurrent.atomic.AtomicLongThen simplify the declarations:
- private val executionStatuses = java.util.concurrent.ConcurrentHashMap<DetailedItem, CacheState>() - private val executingCaching = java.util.concurrent.ConcurrentHashMap<DetailedItem, Job>() - - private val lastNotificationUpdate = - java.util.concurrent.atomic - .AtomicLong(0L) + private val executionStatuses = ConcurrentHashMap<DetailedItem, CacheState>() + private val executingCaching = ConcurrentHashMap<DetailedItem, Job>() + private val lastNotificationUpdate = AtomicLong(0L)app/src/main/kotlin/org/grakovne/lissen/viewmodel/SettingsViewModel.kt (2)
38-39: Use imports instead of fully qualified names in constructor parameters.
org.grakovne.lissen.content.BookRepositoryandorg.grakovne.lissen.content.cache.temporary.CachedCoverProviderare written fully qualified in the constructor while every other dependency uses a short name via imports.♻️ Suggested cleanup
Add to the import block:
import org.grakovne.lissen.content.BookRepository import org.grakovne.lissen.content.cache.temporary.CachedCoverProviderThen simplify:
- 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,
309-329: FQN references totimber.log.Timberandkotlinx.coroutines.CancellationException— use imports for consistency.The rest of the codebase (and this file's sibling
LissenMediaProvider) uses importedTimber. Same applies tokotlinx.coroutines.CancellationException. The CancellationException re-throw logic itself is correct and important for structured concurrency.♻️ Suggested cleanup
Add to the import block:
import kotlinx.coroutines.CancellationException import timber.log.TimberThen simplify inline references:
} catch (e: Exception) { - if (e is kotlinx.coroutines.CancellationException) throw e - timber.log.Timber.e(e, "Failed to clear book metadata cache") + if (e is CancellationException) throw e + Timber.e(e, "Failed to clear book metadata cache") } try { cachedCoverProvider.clearCache() } catch (e: Exception) { - if (e is kotlinx.coroutines.CancellationException) throw e - timber.log.Timber.e(e, "Failed to clear cover cache") + if (e is CancellationException) throw e + Timber.e(e, "Failed to clear cover cache") }app/src/main/kotlin/org/grakovne/lissen/content/BookRepository.kt (2)
47-47:backgroundScopeis never cancelled.Since
BookRepositoryis a@Singleton, this scope lives for the entire app lifetime, which is generally acceptable on Android. However, it has no structured relationship with any lifecycle owner, so leaked coroutines fromsyncAllLibrariesorprefetchCoverswill silently run until completion or app death. Consider whether aProcessLifecycleOwner-scoped approach or at least documenting this design choice would be beneficial.
454-469:prefetchCoversusesyield()anddelay()but runs insidebackgroundScopewhich ignores structured cancellation.When called from
backgroundScope.launch(Lines 265, 422),yield()anddelay()are cancellation points — but nothing ever cancels these jobs. The 2-second initial delay and per-book 100ms delay are reasonable throttling, but thewithContext(Dispatchers.IO)on Line 462 is redundant sincebackgroundScopealready usesDispatchers.IO.app/src/main/kotlin/org/grakovne/lissen/viewmodel/LibraryViewModel.kt (1)
215-219: Analytics event fires on every keystroke during search.
updateSearchis called on each text change. TheisNotEmpty()guard prevents tracking the initial empty state, but every subsequent character typed fires asearch_performedevent. Consider debouncing or only tracking on search submission.app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/DownloadsComposable.kt (2)
137-143:fileStartTimesis computed unconditionally but only used in the ATOMIC branch.The
fileStartTimescomputation runs for all storage types, including MONOLITH and SEGMENTED where it's never referenced. Consider moving it inside the ATOMIC block or wrapping it in a lazy computation.
83-98: Derived state computations in the composable body are not wrapped inremember/derivedStateOf.
currentChapterIndex,currentVolume,remainingVolumes, andcompletedVolumesare recomputed on every recomposition. Since these depend onbookandvolumes, consider wrapping them inrememberwith appropriate keys to avoid redundant computation, especiallycalculateChapterIndexwhich iterates through chapters.app/src/main/kotlin/org/grakovne/lissen/viewmodel/CachingModelView.kt (2)
116-142: Fully-qualified type names hurt readability.Several parameter types and references use full package paths instead of imports (e.g.,
org.grakovne.lissen.lib.domain.PlayingChapter,org.grakovne.lissen.lib.domain.BookFile,org.grakovne.lissen.content.cache.persistent.VolumeLabelType,org.grakovne.lissen.R.string.*). Add imports and use short names.♻️ Add imports at the top of the file
+import org.grakovne.lissen.R +import org.grakovne.lissen.lib.domain.BookFile +import org.grakovne.lissen.content.cache.persistent.VolumeLabelTypeThen use short names throughout.
227-255:refreshMetadatahas no cancellation check or progress feedback in its loop.The
while (hasMore)loop re-fetches and re-caches every detailed item across all pages. For large libraries, this could run for an extended period with no way for the user to observe progress or cancel. Consider addingyield()calls between pages for cooperative cancellation, and optionally emitting progress.♻️ Add cooperative cancellation
while (hasMore) { + kotlinx.coroutines.yield() val items = localCacheRepository .fetchDetailedItems(BATCH_SIZE, page)app/src/main/kotlin/org/grakovne/lissen/ui/screens/details/BookDetailScreen.kt (1)
589-591:storageTypeis computed twice in separate composition scopes.
cachingModelView.getBookStorageType(book)is called at Line 259 (content block) and again at Line 591 (downloads block). SincegetBookStorageTypeis a synchronous call and the book is the same, consider hoisting this computation alongside thevolumesderivation (around Line 156) so both blocks can share it.♻️ Hoist storageType alongside volumes
+ val storageType = + remember(bookDetail?.id) { + bookDetail?.let { cachingModelView.getBookStorageType(it) } + } + val volumes = remember(bookDetail, cacheProgress.status, cacheVersion) { bookDetail?.let { cachingModelView.getVolumes(it) } ?: emptyList() }Then remove the two
remember(book.id) { cachingModelView.getBookStorageType(book) }calls at Lines 259 and 591, and use the hoisted value (with a null fallback if needed).
| "api_key": [ | ||
| { | ||
| "current_key": "AIzaSyAB8o-65tB6TzUbdaqVAP9XnTl1wgC6dks" | ||
| } |
There was a problem hiding this comment.
GCP API key committed to repository — ensure it is restricted in Cloud Console.
While google-services.json is routinely committed to Android projects (and Google documents it as safe to include in source control), the Firebase/GCP API key on Lines 18 and 37 is nonetheless visible to anyone with repo access. Static analysis (Gitleaks) correctly flags this.
Mitigations to apply:
- Restrict the API key in the Google Cloud Console → Credentials: limit it by Android app (package name + SHA-1 fingerprint) and by API (only the specific Firebase APIs you use).
- If this repository is public, also consider whether the project number and app IDs leak more surface area than intended — Firebase App Check can further lock down backend access.
- Ensure
.gitignorepatterns are intentional — some teams choose to distribute this file via CI secrets instead of checking it in for public repos.
No code change needed if the key is properly restricted, but leaving it unrestricted in a public repo is a security posture gap.
Also applies to: 35-38
🧰 Tools
🪛 Gitleaks (8.30.0)
[high] 18-18: Uncovered a GCP API key, which could lead to unauthorized access to Google Cloud services and data breaches.
(gcp-api-key)
🤖 Prompt for AI Agents
In `@app/google-services.json` around lines 16 - 19, The committed Firebase/GCP
API key is exposed in google-services.json under the "api_key" -> "current_key"
field; restrict that key in the Google Cloud Console (Credentials) by applying
Android app restrictions (package name + SHA-1) and API restrictions to only
needed Firebase APIs, and if this repo is public remove or stop committing
google-services.json (or serve it via CI secrets) and consider enabling Firebase
App Check for additional protection; also review .gitignore to prevent
accidental commits of google-services.json going forward.
| fun trackEvent( | ||
| eventName: String, | ||
| value: String, | ||
| ) { | ||
| if (BuildConfig.DEBUG) return | ||
|
|
||
| try { | ||
| // Clarity sendCustomEvent takes a string value for the event name/payload | ||
| Clarity.sendCustomEvent("$eventName: [REDACTED]") | ||
| Timber.d("Clarity Event tracked: $eventName with value: [REDACTED]") | ||
| } catch (e: Exception) { | ||
| Timber.e(e, "Failed to track Clarity event: $eventName with value: [REDACTED]") | ||
| } | ||
| } |
There was a problem hiding this comment.
value parameter is accepted but silently discarded — misleading API.
The value parameter is never used; Line 49 sends "$eventName: [REDACTED]" to Clarity regardless of what value is passed. Callers passing a value will expect it to be tracked. Additionally, appending ": [REDACTED]" to the event name pollutes Clarity's event namespace (e.g., "sync_progress: [REDACTED]" instead of "sync_progress").
Either remove the value parameter entirely (and have callers use the single-arg overload), or send the actual value if it contains no PII. If intentional, add a @Suppress("UNUSED_PARAMETER") and a KDoc note.
Option A: Remove unused parameter, delegate to single-arg overload
fun trackEvent(
eventName: String,
- value: String,
) {
if (BuildConfig.DEBUG) return
try {
- // Clarity sendCustomEvent takes a string value for the event name/payload
- Clarity.sendCustomEvent("$eventName: [REDACTED]")
- Timber.d("Clarity Event tracked: $eventName with value: [REDACTED]")
+ Clarity.sendCustomEvent(eventName)
+ Timber.d("Clarity Event tracked: $eventName")
} catch (e: Exception) {
- Timber.e(e, "Failed to track Clarity event: $eventName with value: [REDACTED]")
+ Timber.e(e, "Failed to track Clarity event: $eventName")
}
}📝 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.
| fun trackEvent( | |
| eventName: String, | |
| value: String, | |
| ) { | |
| if (BuildConfig.DEBUG) return | |
| try { | |
| // Clarity sendCustomEvent takes a string value for the event name/payload | |
| Clarity.sendCustomEvent("$eventName: [REDACTED]") | |
| Timber.d("Clarity Event tracked: $eventName with value: [REDACTED]") | |
| } catch (e: Exception) { | |
| Timber.e(e, "Failed to track Clarity event: $eventName with value: [REDACTED]") | |
| } | |
| } | |
| fun trackEvent( | |
| eventName: String, | |
| ) { | |
| if (BuildConfig.DEBUG) return | |
| try { | |
| Clarity.sendCustomEvent(eventName) | |
| Timber.d("Clarity Event tracked: $eventName") | |
| } catch (e: Exception) { | |
| Timber.e(e, "Failed to track Clarity event: $eventName") | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@app/src/main/kotlin/org/grakovne/lissen/analytics/ClarityTracker.kt` around
lines 41 - 54, The trackEvent function accepts a value parameter but never uses
it and appends a redacted suffix to the event name, which misleads callers and
pollutes Clarity event names; fix by removing the unused value parameter from
the signature of org.grakovne.lissen.analytics.ClarityTracker.trackEvent and
implement a single-argument overload that preserves existing behavior (i.e.,
call Clarity.sendCustomEvent(eventName) and Timber.d with eventName) and update
all callers to use the single-arg overload (or provide a two-arg overload that
delegates to the single-arg one if you need backward compatibility);
alternatively, if you intend to keep the parameter, explicitly use or sanitize
value before sending it via Clarity.sendCustomEvent and add a KDoc or
`@Suppress`("UNUSED_PARAMETER") if it must remain unused.
| com.google.firebase.crashlytics.FirebaseCrashlytics | ||
| .getInstance() | ||
| .recordException(ex) |
There was a problem hiding this comment.
recordException reports as non-fatal — potential duplicate with Crashlytics' own fatal handler.
Firebase Crashlytics automatically captures uncaught exceptions when it's initialized. Calling recordException here records it as a non-fatal, so the same crash may appear twice in the dashboard — once as fatal (auto-captured) and once as non-fatal.
If the intent is just to ensure the crash reaches Crashlytics before the toast delay, consider calling FirebaseCrashlytics.getInstance().sendUnsentReports() instead, or simply rely on Crashlytics' built-in handler (which typically chains with the default handler).
🤖 Prompt for AI Agents
In `@app/src/main/kotlin/org/grakovne/lissen/common/CrashHandler.kt` around lines
20 - 22, The current CrashHandler use of
FirebaseCrashlytics.getInstance().recordException(ex) records the throwable as a
non-fatal event and can duplicate the same uncaught crash that Crashlytics
already captures; update the CrashHandler (the code in CrashHandler.kt where
recordException is called) to either remove the recordException call and rely on
Crashlytics' default uncaught exception handler, or replace it with
FirebaseCrashlytics.getInstance().sendUnsentReports() (or another appropriate
send/flush method) to ensure delivery without creating a duplicate non-fatal
record, and keep any existing toast/delay logic intact so the crash is not
double-reported.
| suspend fun refreshServerAvailabilitySync(): Boolean { | ||
| delay(500) | ||
| val isConnectedToInternet = isNetworkAvailable() | ||
| _networkStatus.emit(isConnectedToInternet) | ||
|
|
||
| val hostUrl = preferences.getHost() | ||
| if (hostUrl.isNullOrBlank()) { | ||
| _isServerAvailable.emit(false) | ||
| return@launch | ||
| } | ||
| if (!isConnectedToInternet) { | ||
| _isServerAvailable.emit(false) | ||
| return false | ||
| } | ||
|
|
||
| try { | ||
| val url = java.net.URL(hostUrl) | ||
| val port = if (url.port == -1) url.defaultPort else url.port | ||
| val address = java.net.InetSocketAddress(url.host, port) | ||
| val hostUrl = preferences.getHost() | ||
| if (hostUrl.isNullOrBlank()) { | ||
| _isServerAvailable.emit(false) | ||
| return false | ||
| } | ||
|
|
||
| java.net.Socket().use { socket -> | ||
| socket.connect(address, 2000) | ||
| } | ||
| return try { | ||
| val url = java.net.URL(hostUrl) | ||
| val port = if (url.port == -1) url.defaultPort else url.port | ||
| val address = java.net.InetSocketAddress(url.host, port) | ||
|
|
||
| _isServerAvailable.emit(true) | ||
| } catch (e: Exception) { | ||
| Timber.e(e, "Server reachability check failed for $hostUrl") | ||
| _isServerAvailable.emit(false) | ||
| } | ||
| java.net.Socket().use { socket -> | ||
| socket.connect(address, 2000) | ||
| } | ||
|
|
||
| _isServerAvailable.emit(true) | ||
| initialRetryCount = MAX_INITIAL_RETRIES // Stop retries once connected | ||
| true | ||
| } catch (e: Exception) { | ||
| Timber.e(e, "Server reachability check failed for $hostUrl (Attempt ${initialRetryCount + 1})") | ||
| _isServerAvailable.emit(false) | ||
|
|
||
| if (initialRetryCount < MAX_INITIAL_RETRIES) { | ||
| initialRetryCount++ | ||
| delay(300L) | ||
| refreshServerAvailabilitySync() | ||
| } else { | ||
| false | ||
| } | ||
| } |
There was a problem hiding this comment.
Retry logic has two defects: delay compounding and one-shot retry counter.
1. Delay compounding in retries: The 500ms debounce delay at Line 97 is hit on every recursive call (Line 131), so each retry actually waits 800ms (500 + 300) instead of the intended 300ms.
2. initialRetryCount is never reset: Once it reaches MAX_INITIAL_RETRIES (via success at Line 122 or exhaustion), all future calls from network events or host changes get zero retries. This means a host change (via hostFlow) or a network reconnect will do a single attempt with no retry, which likely isn't the intent.
3. Minor: Recursive approach is harder to follow than a simple loop and risks the compounding issue above.
Proposed fix: use a loop and reset retry count per invocation
suspend fun refreshServerAvailabilitySync(): Boolean {
delay(500)
val isConnectedToInternet = isNetworkAvailable()
_networkStatus.emit(isConnectedToInternet)
if (!isConnectedToInternet) {
_isServerAvailable.emit(false)
return false
}
val hostUrl = preferences.getHost()
if (hostUrl.isNullOrBlank()) {
_isServerAvailable.emit(false)
return false
}
- return try {
- val url = java.net.URL(hostUrl)
- val port = if (url.port == -1) url.defaultPort else url.port
- val address = java.net.InetSocketAddress(url.host, port)
-
- java.net.Socket().use { socket ->
- socket.connect(address, 2000)
- }
-
- _isServerAvailable.emit(true)
- initialRetryCount = MAX_INITIAL_RETRIES // Stop retries once connected
- true
- } catch (e: Exception) {
- Timber.e(e, "Server reachability check failed for $hostUrl (Attempt ${initialRetryCount + 1})")
- _isServerAvailable.emit(false)
-
- if (initialRetryCount < MAX_INITIAL_RETRIES) {
- initialRetryCount++
- delay(300L)
- refreshServerAvailabilitySync()
- } else {
- false
- }
- }
+ val url = java.net.URL(hostUrl)
+ val port = if (url.port == -1) url.defaultPort else url.port
+ val address = java.net.InetSocketAddress(url.host, port)
+
+ repeat(MAX_INITIAL_RETRIES) { attempt ->
+ try {
+ java.net.Socket().use { socket ->
+ socket.connect(address, 2000)
+ }
+ _isServerAvailable.emit(true)
+ return true
+ } catch (e: Exception) {
+ Timber.e(e, "Server reachability check failed for $hostUrl (Attempt ${attempt + 1})")
+ _isServerAvailable.emit(false)
+ if (attempt < MAX_INITIAL_RETRIES - 1) {
+ delay(300L)
+ }
+ }
+ }
+ return false
}This also eliminates the initialRetryCount instance field entirely, removing the thread-safety concern (see next comment).
📝 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.
| suspend fun refreshServerAvailabilitySync(): Boolean { | |
| delay(500) | |
| val isConnectedToInternet = isNetworkAvailable() | |
| _networkStatus.emit(isConnectedToInternet) | |
| val hostUrl = preferences.getHost() | |
| if (hostUrl.isNullOrBlank()) { | |
| _isServerAvailable.emit(false) | |
| return@launch | |
| } | |
| if (!isConnectedToInternet) { | |
| _isServerAvailable.emit(false) | |
| return false | |
| } | |
| try { | |
| val url = java.net.URL(hostUrl) | |
| val port = if (url.port == -1) url.defaultPort else url.port | |
| val address = java.net.InetSocketAddress(url.host, port) | |
| val hostUrl = preferences.getHost() | |
| if (hostUrl.isNullOrBlank()) { | |
| _isServerAvailable.emit(false) | |
| return false | |
| } | |
| java.net.Socket().use { socket -> | |
| socket.connect(address, 2000) | |
| } | |
| return try { | |
| val url = java.net.URL(hostUrl) | |
| val port = if (url.port == -1) url.defaultPort else url.port | |
| val address = java.net.InetSocketAddress(url.host, port) | |
| _isServerAvailable.emit(true) | |
| } catch (e: Exception) { | |
| Timber.e(e, "Server reachability check failed for $hostUrl") | |
| _isServerAvailable.emit(false) | |
| } | |
| java.net.Socket().use { socket -> | |
| socket.connect(address, 2000) | |
| } | |
| _isServerAvailable.emit(true) | |
| initialRetryCount = MAX_INITIAL_RETRIES // Stop retries once connected | |
| true | |
| } catch (e: Exception) { | |
| Timber.e(e, "Server reachability check failed for $hostUrl (Attempt ${initialRetryCount + 1})") | |
| _isServerAvailable.emit(false) | |
| if (initialRetryCount < MAX_INITIAL_RETRIES) { | |
| initialRetryCount++ | |
| delay(300L) | |
| refreshServerAvailabilitySync() | |
| } else { | |
| false | |
| } | |
| } | |
| suspend fun refreshServerAvailabilitySync(): Boolean { | |
| delay(500) | |
| val isConnectedToInternet = isNetworkAvailable() | |
| _networkStatus.emit(isConnectedToInternet) | |
| if (!isConnectedToInternet) { | |
| _isServerAvailable.emit(false) | |
| return false | |
| } | |
| val hostUrl = preferences.getHost() | |
| if (hostUrl.isNullOrBlank()) { | |
| _isServerAvailable.emit(false) | |
| return false | |
| } | |
| val url = java.net.URL(hostUrl) | |
| val port = if (url.port == -1) url.defaultPort else url.port | |
| val address = java.net.InetSocketAddress(url.host, port) | |
| repeat(MAX_INITIAL_RETRIES) { attempt -> | |
| try { | |
| java.net.Socket().use { socket -> | |
| socket.connect(address, 2000) | |
| } | |
| _isServerAvailable.emit(true) | |
| return true | |
| } catch (e: Exception) { | |
| Timber.e(e, "Server reachability check failed for $hostUrl (Attempt ${attempt + 1})") | |
| _isServerAvailable.emit(false) | |
| if (attempt < MAX_INITIAL_RETRIES - 1) { | |
| delay(300L) | |
| } | |
| } | |
| } | |
| return false | |
| } |
🤖 Prompt for AI Agents
In `@app/src/main/kotlin/org/grakovne/lissen/common/NetworkService.kt` around
lines 96 - 135, The refreshServerAvailabilitySync() retry logic wrongly
re-applies the 500ms debounce on each recursive retry and uses an
instance-scoped initialRetryCount that is never reset, causing compounding
delays and one-shot retries; replace the recursion with a local-loop retry
inside refreshServerAvailabilitySync(): perform the initial
delay/isNetworkAvailable()/host null check once at the start, then attempt the
socket connect in a for/while loop that uses a local retry counter (reset on
each invocation) and delays 300ms between attempts, emit _isServerAvailable and
_networkStatus appropriately on success/failure, and remove the instance field
initialRetryCount entirely so each call gets its own retry budget.
| suspend fun syncLibraryPage( | ||
| libraryId: String, | ||
| pageSize: Int, | ||
| pageNumber: Int, | ||
| ): OperationResult<Unit> = | ||
| providePreferredChannel() | ||
| .fetchBooks(libraryId, pageSize, pageNumber) | ||
| .map { localCacheRepository.cacheBooks(it.items) } | ||
| OperationResult | ||
| .Success(Unit) | ||
| .also { | ||
| backgroundScope.launch { | ||
| try { | ||
| syncAllLibraries() | ||
| } catch (e: Exception) { | ||
| Timber.e(e, "Failed to sync all libraries") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
syncLibraryPage ignores its libraryId, pageSize, and pageNumber parameters.
The method signature suggests a targeted page sync, but the implementation returns Success immediately and fires off syncAllLibraries() in the background, ignoring all three parameters. This is misleading for callers (e.g., LibraryViewModel.refreshLibrary at Line 249 passes specific values). Either align the implementation with the contract or simplify the signature.
♻️ Suggested: remove unused parameters or use them
If the intent is truly to sync all libraries in the background regardless:
- suspend fun syncLibraryPage(
- libraryId: String,
- pageSize: Int,
- pageNumber: Int,
- ): OperationResult<Unit> =
+ fun triggerBackgroundSync(): OperationResult<Unit> =
OperationResult
.Success(Unit)
.also {
backgroundScope.launch {
try {
syncAllLibraries()
} catch (e: Exception) {
Timber.e(e, "Failed to sync all libraries")
}
}
}🤖 Prompt for AI Agents
In `@app/src/main/kotlin/org/grakovne/lissen/content/BookRepository.kt` around
lines 177 - 192, The current syncLibraryPage function ignores its parameters and
always calls syncAllLibraries; change it to perform a targeted page sync using
the provided libraryId, pageSize, and pageNumber instead of calling
syncAllLibraries. Replace the backgroundScope.launch block to call a suspend
worker like syncLibraryPageTask(libraryId, pageSize, pageNumber) (or create such
a helper) and handle exceptions with Timber.e(e, "...") as before; keep
returning OperationResult.Success(Unit) immediately if you want fire-and-forget
semantics, or await the task and return a success/failure based on the result if
you want synchronous behavior. Ensure you reference the existing function name
syncLibraryPage and the launched coroutine (backgroundScope.launch) when making
the change so callers like LibraryViewModel.refreshLibrary receive the intended
behavior.
| var isMainLoading by remember(imageRequest) { mutableStateOf(true) } | ||
| var isThumbnailLoaded by remember(thumbnailRequest) { mutableStateOf(false) } | ||
|
|
||
| LaunchedEffect(imageRequest) { | ||
| isMainLoading = true | ||
| onLoadingStateChanged(true) | ||
| } |
There was a problem hiding this comment.
Potential race between LaunchedEffect and onSuccess for cache-hit images.
When imageRequest changes, remember already initializes isMainLoading = true. The LaunchedEffect then also sets it to true and fires onLoadingStateChanged(true), but it runs as a coroutine launched after composition is committed. If Coil resolves the image from memory/disk cache very quickly, onSuccess (line 82-85) could set isMainLoading = false and call onLoadingStateChanged(false) before or around the same time as the LaunchedEffect body executes, which would then overwrite isMainLoading back to true — leaving the component stuck in a loading state.
Consider moving the onLoadingStateChanged(true) callback into a SideEffect or invoking it conditionally, or removing the LaunchedEffect entirely since remember(imageRequest) already handles the state reset:
Proposed fix
- var isMainLoading by remember(imageRequest) { mutableStateOf(true) }
+ var isMainLoading by remember(imageRequest) { mutableStateOf(true).also { onLoadingStateChanged(true) } }
var isThumbnailLoaded by remember(thumbnailRequest) { mutableStateOf(false) }
- LaunchedEffect(imageRequest) {
- isMainLoading = true
- onLoadingStateChanged(true)
- }Note: Calling onLoadingStateChanged inside remember's initializer may not be ideal either (side-effects in composition). A cleaner alternative is a SideEffect that fires only when isMainLoading transitions to true, or use snapshotFlow.
🤖 Prompt for AI Agents
In
`@app/src/main/kotlin/org/grakovne/lissen/ui/components/AsyncShimmeringImage.kt`
around lines 37 - 43, The LaunchedEffect that sets isMainLoading and calls
onLoadingStateChanged races with Coil’s onSuccess; remove the LaunchedEffect and
keep isMainLoading initialized by remember(imageRequest) only, then add a
SideEffect (or snapshotFlow observing isMainLoading) that invokes
onLoadingStateChanged(true) only when isMainLoading transitions to true so a
later onSuccess handler (in onSuccess) can reliably set isMainLoading = false
and call onLoadingStateChanged(false) without being overwritten; reference the
variables and callbacks isMainLoading, remember(imageRequest), LaunchedEffect,
SideEffect/snapshotFlow, and the onSuccess block to locate and update the code.
| val preparingBookId by playerViewModel.preparingBookId.observeAsState(null) | ||
| val preparingError by playerViewModel.preparingError.observeAsState(false) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "preparingError" --type=kt -C 3Repository: SurjitSahoo/kahani-android
Length of output: 96
🏁 Script executed:
rg -n "preparingBookId" --type=kt -C 3Repository: SurjitSahoo/kahani-android
Length of output: 96
🏁 Script executed:
cat -n app/src/main/kotlin/org/grakovne/lissen/ui/screens/library/composables/MiniPlayerComposable.kt | head -100Repository: SurjitSahoo/kahani-android
Length of output: 4785
🏁 Script executed:
cat -n app/src/main/kotlin/org/grakovne/lissen/ui/screens/library/composables/MiniPlayerComposable.kt | sed -n '100,250p'Repository: SurjitSahoo/kahani-android
Length of output: 5601
🏁 Script executed:
rg "preparingError" --type=kotlin -C 3Repository: SurjitSahoo/kahani-android
Length of output: 4476
🏁 Script executed:
rg "preparingBookId" --type=kotlin -C 3Repository: SurjitSahoo/kahani-android
Length of output: 11463
🏁 Script executed:
cat -n app/src/main/kotlin/org/grakovne/lissen/ui/screens/library/composables/MiniPlayerComposable.kt | sed -n '275,305p'Repository: SurjitSahoo/kahani-android
Length of output: 1244
🏁 Script executed:
cat -n app/src/main/kotlin/org/grakovne/lissen/ui/screens/details/BookDetailScreen.kt | sed -n '140,165p'Repository: SurjitSahoo/kahani-android
Length of output: 1731
Gate loading/error state with preparingBookId == book.id to avoid showing incorrect states for other books.
The mini player displays a specific book (book: DetailedItem), but currently the loading and error states trigger whenever preparingBookId is non-null, regardless of which book is being prepared. This causes the spinner and error icon to display incorrectly when a different book is being loaded.
Compare with BookDetailScreen, which correctly uses preparingBookId == bookId to gate the loading indicator. Apply the same pattern here:
val isLoading = preparingBookId == book.id
val isError = preparingError && preparingBookId == book.id🤖 Prompt for AI Agents
In
`@app/src/main/kotlin/org/grakovne/lissen/ui/screens/library/composables/MiniPlayerComposable.kt`
around lines 76 - 77, The loading/error states are not gated to the current
book; compute local flags using the observed values so the spinner/error show
only for this book: derive val isLoading = preparingBookId == book.id and val
isError = preparingError && preparingBookId == book.id (observed from
playerViewModel.preparingBookId and preparingError) and replace usages that
currently check preparingBookId or preparingError directly with isLoading and
isError inside MiniPlayerComposable (where the spinner and error icon rendering
occur).
| for (i in 0 until cachedItems.itemCount) { | ||
| val book = cachedItems[i] ?: continue | ||
| val selectionsForBook = selectedByBook[book.id] ?: continue | ||
|
|
||
| val volumes = viewModel.getVolumes(book) | ||
| selectionsForBook.forEach { selection -> | ||
| val volume = volumes.find { it.id == selection.fileId } | ||
| total += volume?.size ?: 0L | ||
| } |
There was a problem hiding this comment.
Iterating all paging items may silently skip pages that haven't been loaded yet.
cachedItems[i] returns null for items whose page hasn't been fetched. Both calculateReclaimSize (line 742) and deleteSelectedVolumes (line 763) iterate 0 until cachedItems.itemCount and skip nulls with ?: continue / mapNotNull. If the user selects volumes from items that have scrolled out of the paging window, those items may not be resident and will be silently skipped — the reclaim size will be under-reported, and more critically, deleteSelectedVolumes will silently fail to delete those volumes.
Consider maintaining a separate lookup (e.g. a Map<String, DetailedItem> populated as items are rendered) rather than re-reading from the paging source at deletion time, or ensure all relevant pages are loaded before these operations run.
Also applies to: 761-764
🤖 Prompt for AI Agents
In
`@app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/advanced/cache/CachedItemsSettingsScreen.kt`
around lines 741 - 749, calculateReclaimSize and deleteSelectedVolumes currently
iterate 0 until cachedItems.itemCount and skip null entries, which silently
ignores selections whose pages aren't loaded; fix by maintaining a persistent
lookup (e.g., MutableMap<String, DetailedItem> renderedItemCache) that is
populated when items are bound/rendered (update in the composable or adapter
where items are accessed) and then use renderedItemCache.lookup(book.id) and
renderedItemCache.lookup(volumeId) inside calculateReclaimSize and
deleteSelectedVolumes (fall back to loading from viewModel.getVolumes(book) only
if the cache misses), or alternatively trigger loading of the necessary paging
pages before computing/deleting to guarantee presence — update the code paths
that reference cachedItems[i], calculateReclaimSize, and deleteSelectedVolumes
to use this cache/fallback so selections aren't skipped.
| fun refreshLibrary(forceRefresh: Boolean = false) { | ||
| viewModelScope.launch { | ||
| withContext(Dispatchers.IO) { | ||
| fetchRecentListening() | ||
| } | ||
| } | ||
| } | ||
| val isAvailable = | ||
| if (forceRefresh) { | ||
| networkService.refreshServerAvailabilitySync() | ||
| } else { | ||
| networkService.isServerAvailable.value | ||
| } | ||
|
|
||
| val shouldSync = (forceRefresh || isAvailable) && !preferences.isForceCache() | ||
|
|
||
| if (shouldSync) { | ||
| val libraryId = preferences.getPreferredLibrary()?.id | ||
|
|
||
| if (libraryId != null) { | ||
| bookRepository.syncLibraryPage( | ||
| libraryId = libraryId, | ||
| pageSize = PAGE_SIZE, | ||
| pageNumber = 0, | ||
| ) | ||
| } | ||
|
|
||
| bookRepository.syncRepositories() | ||
| } |
There was a problem hiding this comment.
refreshLibrary triggers duplicate sync work.
When shouldSync is true, the method calls both syncLibraryPage (Line 249) and syncRepositories (Line 256). Looking at their implementations:
syncLibraryPagelaunchessyncAllLibraries()in the background, which callssyncFullLibraryfor every library.syncRepositories(inBookRepository) also callssyncFullLibrary(libraryId)directly.
This means syncFullLibrary runs at least twice for the preferred library — once from each path — potentially causing redundant network calls for minified/detailed book fetches, duplicate cover prefetches, and wasted bandwidth.
Consider removing one of the two calls or coordinating them to avoid overlapping work.
🤖 Prompt for AI Agents
In `@app/src/main/kotlin/org/grakovne/lissen/viewmodel/LibraryViewModel.kt` around
lines 233 - 257, refreshLibrary currently triggers duplicate work by calling
bookRepository.syncLibraryPage(...) and then bookRepository.syncRepositories(),
which results in syncFullLibrary running twice for the preferred library (via
syncAllLibraries inside syncLibraryPage and directly in syncRepositories). Fix
by ensuring only one path performs a full sync for the preferred library: either
remove the call to bookRepository.syncRepositories() when
preferences.getPreferredLibrary() is non-null (i.e., when syncLibraryPage is
invoked for that library), or change BookRepository.syncRepositories() to skip
libraries already synced by syncLibraryPage (e.g., accept an optional
excludedLibraryId and avoid calling syncFullLibrary for that id). Update
refreshLibrary (function refreshLibrary) and/or
BookRepository.syncRepositories/syncAllLibraries/syncFullLibrary accordingly so
the preferred library is not synced twice.
| <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> |
There was a problem hiding this comment.
Consent message claims "anonymous" data but hashed user identifiers are pseudonymous, not anonymous.
The analytics consent message states "sharing anonymous technical data," but ClarityComponent sends a SHA-256 hash of username@host to Microsoft Clarity for user re-identification. Under GDPR, pseudonymized data (hashed identifiers that can correlate sessions) is not anonymous. Consider rewording to "pseudonymous" or "non-personally-identifiable" to accurately represent what is collected.
🤖 Prompt for AI Agents
In `@app/src/main/res/values/strings.xml` around lines 277 - 278, The consent copy
in resources (strings analytics_consent_title and analytics_consent_message)
incorrectly states "anonymous" while ClarityComponent sends a SHA-256 hash of
username@host (pseudonymous identifier); update the strings to accurately
reflect pseudonymous or non‑personally‑identifiable data (e.g., replace
"anonymous" with "pseudonymous" or "non-personally-identifiable") and ensure
messaging aligns with ClarityComponent's hashing behavior and any privacy docs.
1 participant
Summary by CodeRabbit
New Features
Improvements
Chores