Closed
Conversation
added 5 commits
February 3, 2026 22:33
… 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.
📝 WalkthroughWalkthroughThis pull request implements comprehensive database migration infrastructure (versions 15-17), adds file size tracking, introduces per-user/host-scoped caching, establishes storage type classification for downloads (MONOLITH, SEGMENTED, ATOMIC), refactors cache management with granular sizing and cleanup, and redesigns the UI for cached items with selection-based volume management and a new migration screen. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as UI<br/>(DownloadsComposable)
participant VM as ViewModel<br/>(CachingModelView)
participant Repo as Repository<br/>(LocalCacheRepository)
participant DAO as DAO<br/>(CachedBookDao)
participant DB as Database
User->>UI: Initiates download<br/>with SpecificFilesDownloadOption
UI->>VM: onRequestedDownload(option)
VM->>Repo: cacheBook(book, chapters)
Repo->>Repo: calculateFileStartTimes(files)
Repo->>Repo: findRelatedFilesByStartTimes(chapter)
Repo->>DAO: upsertCachedBook(book,<br/>host, username)
DAO->>DB: INSERT BookEntity<br/>with host/username
DAO->>DB: INSERT BookFileEntity<br/>with size
DB-->>DAO: Success
DAO-->>Repo: Result
Repo->>Repo: refreshStorageStats()
Repo-->>VM: Completion
VM->>UI: Update storageStats Flow
UI-->>User: Display updated<br/>cache & storage state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
lib/src/main/kotlin/org/grakovne/lissen/lib/domain/DetailedItem.kt (1)
31-37:⚠️ Potential issue | 🟠 MajorAdd backward compatibility for JSON deserialization of the
sizefield.The non-nullable
size: Longfield added toBookFilewill cause deserialization failures when older cached JSON from SharedPreferences (which lacks this field) is read back. Moshi's code-gen adapters with@JsonClass(generateAdapter = true)are strict and require all non-nullable fields to be present.Users upgrading the app with a cached
DetailedItemin SharedPreferences will encounter a crash inAppNavHost(line 54) orPlayerViewModel.recoverMiniPlayer()when callinggetPlayingBook(). The database migration that addsDEFAULT 0only affects the Room database, not the SharedPreferences JSON cache.Consider either:
- Making
sizenullable with a default value, or- Adding a custom Moshi adapter for
BookFileto handle missingsizegracefully, or- Clearing the SharedPreferences cache during the database migration.
app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/api/CachedBookRepository.kt (1)
95-118:⚠️ Potential issue | 🟠 MajorExisting cache entries keep old host/username when re‑caching.
existing?.copy(...)does not override host/username, so after switching accounts the updated rows remain bound to the previous user and won’t show up in host/username‑scoped queries. Either scopeexistingBooksby host/username or update those fields on copy.🔧 Example: update host/username in the copy path
- existing?.copy( + existing?.copy( title = book.title, author = book.author, subtitle = book.subtitle, seriesNames = book.series, duration = book.duration.toInt(), libraryType = null, // currently not available here + host = preferences.getHost() ?: "", + username = preferences.getUsername() ?: "", ) ?: BookEntity(app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/api/FetchRequestBuilder.kt (1)
35-53:⚠️ Potential issue | 🟠 Major
downloadedOnly=truebypasses host/username isolation.When
downloadedOnly=true, the isolation clause omits the host/username filter that is enforced in thefalsebranch. This allows cached items from other users/hosts to appear in results, violating the per-user/host isolation intended elsewhere in the code.🔧 Example: Apply host/username filtering in both branches
val isolationClause = when (downloadedOnly) { true -> { args.add(host) args.add(username) "(host = ? AND username = ?) AND EXISTS (SELECT 1 FROM book_chapters WHERE bookId = detailed_books.id AND isCached = 1)" } false -> { args.add(host) args.add(username) "((host = ? AND username = ?) OR EXISTS (SELECT 1 FROM book_chapters WHERE bookId = detailed_books.id AND isCached = 1))" } }
🤖 Fix all issues with AI agents
In
`@app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/api/SearchRequestBuilder.kt`:
- Around line 11-12: The host and username fields (and the similar fields at
lines 22-24) must not default to empty strings because that yields unintended
SQL matches; change their declarations in SearchRequestBuilder to nullable types
(e.g., host: String? and username: String?) and update the build() method so it
only appends the corresponding SQL WHERE clauses when those properties are
non-null (or non-empty if you keep validation), otherwise omit them;
alternatively, if these fields are required for a valid search, make the setters
enforce presence and have build() throw an IllegalStateException when host or
username is unset (refer to the host, username properties and the build()
function to implement the null-check/conditional clause logic).
In
`@app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/dao/CachedLibraryDao.kt`:
- Around line 14-32: The updateLibraries function currently calls
deleteLibrariesExcept with an empty ids list when libraries is empty, which
causes SQLite to evaluate `id NOT IN ()` and delete all rows for that
host/username; add a guard in updateLibraries (in
CachedLibraryDao::updateLibraries) to return early or skip calling
deleteLibrariesExcept when libraries.isEmpty() (i.e., only build entities, call
upsertLibraries(entities) and deleteLibrariesExcept(entities.map { it.id },
host, username) if entities is not empty).
In
`@app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/LocalCacheModule.kt`:
- Around line 48-53: The migration call to produceMigration15_16 in
LocalCacheModule.kt passes raw host/username into SQL defaults which can break
migrations when values contain single quotes; update produceMigration15_16 (in
Migrations.kt) to either parameterize the UPDATE/INSERT statements using bind
args or properly escape values (e.g., replace single quote with two single
quotes or use DatabaseUtils.sqlEscapeString) before interpolating, and ensure
the invocation in LocalCacheModule.kt still supplies
preferences.getHost()/getUsername() but no longer allows raw unescaped strings
to be placed directly into SQL.
In
`@app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/Migrations.kt`:
- Around line 267-280: The migration embeds host and username directly into SQL
which breaks/unsafe if they contain single quotes; in produceMigration15_16
compute escapedHost and escapedUsername (replace single quote ' with two single
quotes '') and use those escaped values when building the ALTER TABLE ...
DEFAULT '...' strings in migrate; reference produceMigration15_16 and the
Migration(15, 16) override to locate where to sanitize host and username before
calling db.execSQL.
In
`@app/src/main/kotlin/org/grakovne/lissen/ui/screens/common/DownloadOptionFormat.kt`:
- Around line 19-21: The branch handling
org.grakovne.lissen.lib.domain.SpecificFilesDownloadOption in
DownloadOptionFormat.kt returns a hardcoded "Selected Volume"; replace it with a
string resource lookup (add a new entry like selected_volume or
download_option_selected_volume to strings.xml) and use the same retrieval
approach used by the other branches in this file (e.g.,
context.getString(R.string.selected_volume) or
stringResource(R.string.selected_volume) if this is a `@Composable`) so the label
is localized consistently.
- Around line 19-21: The when expression in DownloadOptionFormat.kt contains a
duplicate branch for org.grakovne.lissen.lib.domain.SpecificFilesDownloadOption
(one returning "Selected Volume" and a later one returning "Multiple Files"),
causing the latter to be unreachable; fix by removing the unintended duplicate
branch (or, if the later branch was meant for a different type, change its
pattern to the correct download option class) so each download option case in
the when (in the DownloadOptionFormat formatting logic) is unique.
In
`@app/src/main/kotlin/org/grakovne/lissen/ui/screens/details/BookDetailScreen.kt`:
- Around line 243-246: The remember keys for storageType and volumes (where you
call remember(book.id) around cachingModelView.getBookStorageType(book) and
cachingModelView.getVolumes(book)) must include the caching progress/state so
they recompute as files are written; update the remember keys to also depend on
the cache progress observable (e.g., cacheProgress.status or the
cachingModelView-provided progress key) used by your cachingModelView so
storageType/volumes update when download state changes, and change the badge
condition that uses volumes.all { it.isDownloaded } to first check
volumes.isNotEmpty() && volumes.all { it.isDownloaded } to avoid vacuous truth
when the list is empty; apply the same remember-key change at the other
locations mentioned (the downloads sheet and the badge check).
In
`@app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/composable/DownloadsComposable.kt`:
- Around line 127-133: The cached computation for fileStartTimes uses
remember(book.id) but also depends on book.files, so add book.files to the
remember key to avoid stale values; update the remember call for fileStartTimes
(the androidx.compose.runtime.remember invocation) to include both book.id and
book.files (e.g., remember(book.id, book.files) or an equivalent key) so the
runningFold/zip computation recomputes when files change.
In `@app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/PlayerScreen.kt`:
- Line 435: Remove the unused observation variable by deleting the line that
declares hasDownloadedChapters (the call to
cachingModelView.hasDownloadedChapters(playingBook?.id.orEmpty()).observeAsState(false));
this eliminates the dead local variable and its unused observe
side-effect—ensure no other code references hasDownloadedChapters before
removing, and keep the rest of PlayerScreen.kt unchanged.
In
`@app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/advanced/cache/CachedItemsSettingsScreen.kt`:
- Around line 163-170: The back IconButton's onClick currently only toggles
selectionMode to false without clearing selectedVolumes, which leaves stale
selections; modify the onClick handler in CachedItemsSettingsScreen so that when
it sets selectionMode = false it also clears selectedVolumes (e.g.,
selectedVolumes.clear() or assign an empty list) before returning, while
preserving the existing onBack() call when not in selection mode. Ensure you
update the onClick block that references selectionMode, selectedVolumes, and
onBack() accordingly.
- Around line 633-639: Guard against empty chapter lists before calling
volume.chapters.first(): replace the unsafe first() call in
CachedItemsSettingsScreen (the block that calls viewModel.dropCache(item,
volume.chapters.first())) with a null-safe check (e.g., use firstOrNull() or
check isNotEmpty()) and only call viewModel.dropCache when a chapter exists,
mirroring the multi-delete path; ensure onItemRemoved() and
playerViewModel.clearPlayingBook() behavior is preserved when there is no
chapter to pass.
- Around line 173-180: The Text composable in CachedItemsSettingsScreen.kt
currently hardcodes "Cancel" and "Edit" (inside the IconButton toggling
selectionMode/clearing selectedVolumes); replace those literals with string
resources (e.g., use stringResource(R.string.edit) and
stringResource(R.string.cancel)) and add corresponding entries in your
strings.xml (keys like edit and cancel or cached_items_edit/cached_items_cancel)
so the UI is localizable.
In `@app/src/main/kotlin/org/grakovne/lissen/viewmodel/CachingModelView.kt`:
- Around line 92-103: refreshStorageStats() is only invoked after dropCache(),
causing storage UI to go stale when other cache-mutating methods run; locate
other cache mutation functions such as dropCompletedChapters(), any short-term
cache clear methods, and invoke refreshStorageStats() after their repository
operations (using viewModelScope.launch/withContext as you already do) so the
_storageStats LiveData is updated whenever cache state changes.
- Around line 106-116: The four helper methods (getBookSize,
calculateChapterSize, getBookStorageType, getVolumes) call filesystem APIs
synchronously; change each to a suspend function that delegates to
Dispatchers.IO by wrapping the repository call in withContext(Dispatchers.IO) {
... } (e.g., suspend fun getBookSize(book: DetailedItem) =
withContext(Dispatchers.IO) { localCacheRepository.calculateBookSize(book) })
and similarly for calculateChapterSize, getBookStorageType, and getVolumes;
update any callers (e.g., places that use these in UI remember blocks or
PlayerScreen/BookDetailScreen) to call them from a coroutine or suspend context.
In `@app/src/main/kotlin/org/grakovne/lissen/viewmodel/MigrationViewModel.kt`:
- Around line 41-44: The catch block in MigrationViewModel is swallowing
exceptions silently; update the catch for Exception in the migration routine to
log the caught exception before setting _migrationState.value =
MigrationState.Completed — use the project's logging facility (e.g., Logger,
Timber, or Android Log) or add one if missing, and include the exception message
and stacktrace along with context (e.g., "Migration failed") so failures in the
migration flow inside MigrationViewModel are recorded for debugging.
- Around line 48-50: The companion constant CURRENT_DATABASE_VERSION in
MigrationViewModel is out of sync with the Room schema; update
CURRENT_DATABASE_VERSION from 16 to 17 to match the `@Database`(version = 17)
annotation in LocalCacheStorage (and the existing 17.json schema) so migrations
and prompts align; locate the companion object in MigrationViewModel (symbol:
CURRENT_DATABASE_VERSION) and change its value to 17.
In `@lib/src/main/kotlin/org/grakovne/lissen/lib/domain/DownloadOption.kt`:
- Line 28: The serialization for SpecificFilesDownloadOption using
fileIds.joinToString(",") and corresponding split is unsafe: use a robust
encoding (e.g., JSON array via kotlinx.serialization or URLEncoder/URLDecoder
for each id) instead of a raw comma delimiter, and update the deserialization
logic that reconstructs SpecificFilesDownloadOption to parse the chosen format
and treat an empty list correctly (avoid treating an empty substring as a single
empty id). Locate the SpecificFilesDownloadOption class/enum case and replace
joinToString(",") and the split(",") usage with the safe encoder/decoder and add
explicit handling for an empty collection.
- Line 37: The branch that constructs NumberItemDownloadOption using
substringAfter("number_items_").toInt() can throw NumberFormatException on
malformed input; update the parsing in the DownloadOption mapping (the
startsWith("number_items_") -> NumberItemDownloadOption(...) case) to use
toIntOrNull() and handle null by either returning null (or a safe fallback
option) instead of throwing, so invalid or corrupted strings are handled
gracefully.
🧹 Nitpick comments (8)
app/src/main/kotlin/org/grakovne/lissen/content/LissenMediaProvider.kt (1)
238-241: Logic is correct and aligns with the file size tracking feature.The updated condition properly ensures that local results are only considered "detailed" when file sizes are actually populated (> 0). This triggers a remote fetch when size metadata is missing, which is necessary for the new caching/storage classification features.
Consider extracting the condition for improved readability:
♻️ Optional: extract for clarity
val localResult = localCacheRepository.fetchBook(bookId) + val hasContent = { item: DetailedItem -> item.chapters.isNotEmpty() || item.files.isNotEmpty() } + val hasValidFileSizes = { item: DetailedItem -> item.files.isEmpty() || item.files.all { it.size > 0 } } val isDetailed = localResult - ?.let { (it.chapters.isNotEmpty() || it.files.isNotEmpty()) && (it.files.isEmpty() || it.files.all { f -> f.size > 0 }) } + ?.let { hasContent(it) && hasValidFileSizes(it) } ?: falseapp/src/main/kotlin/org/grakovne/lissen/persistence/preferences/LissenSharedPreferences.kt (1)
113-116: Minor style inconsistency insetDatabaseVersion.The
setDatabaseVersionmethod uses the chainededit().putInt().apply()pattern, while other similar methods in this class use theedit { }block syntax (e.g.,saveSslBypass,saveAcraEnabled). Consider using the consistent block style for uniformity.Suggested refactor for consistency
- fun setDatabaseVersion(version: Int) = sharedPreferences.edit().putInt(KEY_DATABASE_VERSION, version).apply() + fun setDatabaseVersion(version: Int) { + sharedPreferences.edit { + putInt(KEY_DATABASE_VERSION, version) + } + }lib/src/main/kotlin/org/grakovne/lissen/lib/domain/DownloadOption.kt (1)
13-15: Consider usingdata classforSpecificFilesDownloadOption.Unlike the object declarations and the other class
NumberItemDownloadOption, this class holds state (fileIds) but isn't adata class. If equality comparisons or debugging output are needed, consider making it a data class for automaticequals/hashCode/toStringimplementations.Suggested change
-class SpecificFilesDownloadOption( +data class SpecificFilesDownloadOption( val fileIds: List<String>, ) : DownloadOptionNote: The same consideration applies to
NumberItemDownloadOptionon lines 9-11.app/src/main/kotlin/org/grakovne/lissen/ui/screens/settings/advanced/cache/CacheSettingsScreen.kt (1)
106-116: Prefer imports over fully qualified names for consistency.The code uses fully qualified names (
androidx.compose.ui.platform.LocalContextandandroid.widget.Toast) while the rest of the file uses imports. Consider adding these to the import section for consistency.♻️ Suggested refactor
Add to imports:
import android.widget.Toast import androidx.compose.ui.platform.LocalContextThen simplify the code:
- 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 + Toast .makeText(context, successMessage, android.widget.Toast.LENGTH_SHORT) .show() } }, )app/src/main/kotlin/org/grakovne/lissen/ui/screens/player/GlobalPlayerBottomSheet.kt (1)
454-484: Simplify redundantletblock.The
it.let { book -> ... }pattern is redundant sinceitalready refers to the non-null book from the outerplayingBook?.letat line 407. You can useitdirectly or rename the outer lambda parameter.♻️ Suggested simplification
if (downloadsExpanded) { // ... cacheProgress and hasDownloadedChapters ... - it.let { book -> - DownloadsComposable( - book = book, - storageType = cachingModelView.getBookStorageType(book), - volumes = cachingModelView.getVolumes(book), + DownloadsComposable( + book = it, + storageType = cachingModelView.getBookStorageType(it), + volumes = cachingModelView.getVolumes(it), // ... rest of parameters using `it` instead of `book` ... - ) - } + ) }app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/CalculateRequestedChapters.kt (1)
50-55: Consider using imports instead of fully qualified names.The fully qualified names for
SpecificFilesDownloadOptionandfindRelatedFilesByStartTimesreduce readability. Consider adding imports at the top of the file.♻️ Suggested imports
import org.grakovne.lissen.lib.domain.AllItemsDownloadOption import org.grakovne.lissen.lib.domain.BookFile import org.grakovne.lissen.lib.domain.CurrentItemDownloadOption import org.grakovne.lissen.lib.domain.DetailedItem import org.grakovne.lissen.lib.domain.DownloadOption import org.grakovne.lissen.lib.domain.NumberItemDownloadOption import org.grakovne.lissen.lib.domain.PlayingChapter import org.grakovne.lissen.lib.domain.RemainingItemsDownloadOption +import org.grakovne.lissen.lib.domain.SpecificFilesDownloadOption +import org.grakovne.lissen.content.cache.common.findRelatedFilesByStartTimes import org.grakovne.lissen.playback.service.calculateChapterIndexThen simplify the usage:
- is org.grakovne.lissen.lib.domain.SpecificFilesDownloadOption -> + is SpecificFilesDownloadOption -> book.chapters.filter { chapter -> - org.grakovne.lissen.content.cache.common - .findRelatedFilesByStartTimes(chapter, fileStartTimes) + findRelatedFilesByStartTimes(chapter, fileStartTimes) .any { it.id in option.fileIds } }app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/api/CachedLibraryRepository.kt (1)
18-32: Consider extracting host/username retrieval to reduce duplication.The pattern
preferences.getHost() ?: ""andpreferences.getUsername() ?: ""is duplicated in both methods. Consider extracting this to private helper properties or a method.♻️ Suggested refactor
class CachedLibraryRepository `@Inject` constructor( private val dao: CachedLibraryDao, private val converter: CachedLibraryEntityConverter, private val preferences: LissenSharedPreferences, ) { + private val currentHost: String + get() = preferences.getHost() ?: "" + + private val currentUsername: String + get() = preferences.getUsername() ?: "" + suspend fun cacheLibraries(libraries: List<Library>) { - val host = preferences.getHost() ?: "" - val username = preferences.getUsername() ?: "" - - dao.updateLibraries(libraries, host, username) + dao.updateLibraries(libraries, currentHost, currentUsername) } suspend fun fetchLibraries(): List<Library> { - val host = preferences.getHost() ?: "" - val username = preferences.getUsername() ?: "" - return dao - .fetchLibraries(host, username) + .fetchLibraries(currentHost, currentUsername) .map { converter.apply(it) } } }app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/LocalCacheRepository.kt (1)
186-223: Avoid hardcoded volume labels in domain data.
BookVolume.nameuses English literals ("Full Archive","Volume X","Part X"). Since UI strings exist, consider storing neutral identifiers (type/index) and format in the UI layer.
Comment on lines
+11
to
+12
| private var host: String = "" | ||
| private var username: String = "" |
There was a problem hiding this comment.
Empty string defaults for host and username may cause unintended query matches.
The fields default to empty strings, and if build() is called without setting them, the query will match rows where host = '' and username = ''. Consider either:
- Making these required (throwing if not set), or
- Using nullable types and adjusting the SQL clause accordingly.
Also applies to: 22-24
🤖 Prompt for AI Agents
In
`@app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/api/SearchRequestBuilder.kt`
around lines 11 - 12, The host and username fields (and the similar fields at
lines 22-24) must not default to empty strings because that yields unintended
SQL matches; change their declarations in SearchRequestBuilder to nullable types
(e.g., host: String? and username: String?) and update the build() method so it
only appends the corresponding SQL WHERE clauses when those properties are
non-null (or non-empty if you keep validation), otherwise omit them;
alternatively, if these fields are required for a valid search, make the setters
enforce presence and have build() throw an IllegalStateException when host or
username is unset (refer to the host, username properties and the build()
function to implement the null-check/conditional clause logic).
Comment on lines
+14
to
32
| suspend fun updateLibraries( | ||
| libraries: List<Library>, | ||
| host: String, | ||
| username: String, | ||
| ) { | ||
| val entities = | ||
| libraries.map { | ||
| CachedLibraryEntity( | ||
| id = it.id, | ||
| title = it.title, | ||
| type = it.type, | ||
| host = host, | ||
| username = username, | ||
| ) | ||
| } | ||
|
|
||
| upsertLibraries(entities) | ||
| deleteLibrariesExcept(entities.map { it.id }) | ||
| deleteLibrariesExcept(entities.map { it.id }, host, username) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the CachedLibraryDao file and examine its contents
find . -name "CachedLibraryDao.kt" -type f | head -5Repository: SurjitSahoo/kahani-android
Length of output: 160
🏁 Script executed:
#!/bin/bash
# Search for deleteLibrariesExcept implementation and its usage
rg -n "deleteLibrariesExcept|updateLibraries" --type=kt -B2 -A5Repository: SurjitSahoo/kahani-android
Length of output: 96
🏁 Script executed:
#!/bin/bash
# Read the CachedLibraryDao.kt file
cat -n ./app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/dao/CachedLibraryDao.ktRepository: SurjitSahoo/kahani-android
Length of output: 1924
🏁 Script executed:
#!/bin/bash
# Search for deleteLibrariesExcept and updateLibraries across kotlin files
rg -n "deleteLibrariesExcept|updateLibraries" -A5 -B2Repository: SurjitSahoo/kahani-android
Length of output: 6482
🏁 Script executed:
#!/bin/bash
# Read CachedLibraryRepository to see if it guards against empty lists
cat -n ./app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/api/CachedLibraryRepository.ktRepository: SurjitSahoo/kahani-android
Length of output: 1394
🏁 Script executed:
#!/bin/bash
# Search for how cacheLibraries is called upstream
rg -n "cacheLibraries" -B3 -A3Repository: SurjitSahoo/kahani-android
Length of output: 4323
🏁 Script executed:
#!/bin/bash
# Search for any documentation or handling of empty lists in the codebase
rg -n "NOT IN|empty" app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/ -B2 -A2Repository: SurjitSahoo/kahani-android
Length of output: 8350
🏁 Script executed:
#!/bin/bash
# Check if there's any guard clause or if callers ever intentionally pass empty lists
# Let's examine the ContentCachingManager more closely
cat -n app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/ContentCachingManager.kt | sed -n '259,270p'Repository: SurjitSahoo/kahani-android
Length of output: 510
🌐 Web query:
Room Android NOT IN empty list parameter behavior
💡 Result:
In Room, when you use a collection parameter in an IN/NOT IN clause, Room expands it to the matching number of placeholders. If the collection is empty, Room effectively generates SQL like:
... IN ()... NOT IN ()(developer.android.com)
In SQLite, an empty RHS set is allowed and has defined semantics:
x IN ()→ falsex NOT IN ()→ true (so theWHEREpredicate matches all rows, regardless ofx, even ifxisNULL) (system.data.sqlite.org)
Practical impact
WHERE id NOT IN (:ids)withids = emptyList()will return all rows (i.e., “exclude nothing”). (system.data.sqlite.org)
Common pitfall (NULLs)
If the list/subquery for NOT IN can contain NULL, SQLite’s NOT IN can evaluate to NULL (and then the row is filtered out by WHERE), often producing “unexpectedly empty” results; NOT EXISTS is usually safer. (stackoverflow.com)
If you want explicit/portable behavior
Gate the predicate yourself:
WHERE (:excludeCount = 0) OR id NOT IN (:excludeIds)(pass both excludeIds and excludeIds.size).
Citations:
- 1: https://developer.android.com/reference/androidx/room/Query?utm_source=openai
- 2: https://system.data.sqlite.org/home/doc/8e13c43294410407/Doc/Extra/Core/lang_expr.html?utm_source=openai
- 3: https://system.data.sqlite.org/home/doc/8e13c43294410407/Doc/Extra/Core/lang_expr.html?utm_source=openai
- 4: https://stackoverflow.com/questions/24924431/sql-not-in-subquery-is-not-working-as-expected?utm_source=openai
Add guard clause to prevent data loss when libraries list is empty.
When libraries is empty, deleteLibrariesExcept is called with an empty ids list. SQLite evaluates id NOT IN () as true, causing the query to delete all libraries for the given host/username combination instead of preserving them. This can silently wipe cached libraries if an API call returns an empty list.
Add an early return or conditional check:
suspend fun updateLibraries(
libraries: List<Library>,
host: String,
username: String,
) {
if (libraries.isEmpty()) return // or: deleteLibrariesExcept(...) only if entities is not empty
val entities = libraries.map { /* ... */ }
upsertLibraries(entities)
deleteLibrariesExcept(entities.map { it.id }, host, username)
}🤖 Prompt for AI Agents
In
`@app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/dao/CachedLibraryDao.kt`
around lines 14 - 32, The updateLibraries function currently calls
deleteLibrariesExcept with an empty ids list when libraries is empty, which
causes SQLite to evaluate `id NOT IN ()` and delete all rows for that
host/username; add a guard in updateLibraries (in
CachedLibraryDao::updateLibraries) to return early or skip calling
deleteLibrariesExcept when libraries.isEmpty() (i.e., only build entities, call
upsertLibraries(entities) and deleteLibrariesExcept(entities.map { it.id },
host, username) if entities is not empty).
Comment on lines
+48
to
+53
| .addMigrations( | ||
| produceMigration15_16( | ||
| host = preferences.getHost() ?: "", | ||
| username = preferences.getUsername() ?: "", | ||
| ), | ||
| ).addMigrations(MIGRATION_16_17) |
There was a problem hiding this comment.
Escape host/username in migration defaults to avoid SQL errors.
produceMigration15_16 interpolates raw host/username into SQL defaults; values containing ' will break the migration. Please escape or switch to parameterized updates in the migration (see Migrations.kt lines 266‑280).
🛠️ Example fix in Migrations.kt
+import android.database.DatabaseUtils
...
fun produceMigration15_16(
host: String,
username: String,
) = object : Migration(15, 16) {
override fun migrate(db: SupportSQLiteDatabase) {
- db.execSQL("ALTER TABLE detailed_books ADD COLUMN host TEXT NOT NULL DEFAULT '$host'")
- db.execSQL("ALTER TABLE detailed_books ADD COLUMN username TEXT NOT NULL DEFAULT '$username'")
+ val escapedHost = DatabaseUtils.sqlEscapeString(host)
+ val escapedUsername = DatabaseUtils.sqlEscapeString(username)
+ db.execSQL("ALTER TABLE detailed_books ADD COLUMN host TEXT NOT NULL DEFAULT $escapedHost")
+ db.execSQL("ALTER TABLE detailed_books ADD COLUMN username TEXT NOT NULL DEFAULT $escapedUsername")
- db.execSQL("ALTER TABLE media_progress ADD COLUMN host TEXT NOT NULL DEFAULT '$host'")
- db.execSQL("ALTER TABLE media_progress ADD COLUMN username TEXT NOT NULL DEFAULT '$username'")
+ db.execSQL("ALTER TABLE media_progress ADD COLUMN host TEXT NOT NULL DEFAULT $escapedHost")
+ db.execSQL("ALTER TABLE media_progress ADD COLUMN username TEXT NOT NULL DEFAULT $escapedUsername")
- db.execSQL("ALTER TABLE libraries ADD COLUMN host TEXT NOT NULL DEFAULT '$host'")
- db.execSQL("ALTER TABLE libraries ADD COLUMN username TEXT NOT NULL DEFAULT '$username'")
+ db.execSQL("ALTER TABLE libraries ADD COLUMN host TEXT NOT NULL DEFAULT $escapedHost")
+ db.execSQL("ALTER TABLE libraries ADD COLUMN username TEXT NOT NULL DEFAULT $escapedUsername")
}
}🤖 Prompt for AI Agents
In
`@app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/LocalCacheModule.kt`
around lines 48 - 53, The migration call to produceMigration15_16 in
LocalCacheModule.kt passes raw host/username into SQL defaults which can break
migrations when values contain single quotes; update produceMigration15_16 (in
Migrations.kt) to either parameterize the UPDATE/INSERT statements using bind
args or properly escape values (e.g., replace single quote with two single
quotes or use DatabaseUtils.sqlEscapeString) before interpolating, and ensure
the invocation in LocalCacheModule.kt still supplies
preferences.getHost()/getUsername() but no longer allows raw unescaped strings
to be placed directly into SQL.
Comment on lines
+267
to
+280
| fun produceMigration15_16( | ||
| host: String, | ||
| username: String, | ||
| ) = object : Migration(15, 16) { | ||
| override fun migrate(db: SupportSQLiteDatabase) { | ||
| db.execSQL("ALTER TABLE detailed_books ADD COLUMN host TEXT NOT NULL DEFAULT '$host'") | ||
| db.execSQL("ALTER TABLE detailed_books ADD COLUMN username TEXT NOT NULL DEFAULT '$username'") | ||
|
|
||
| db.execSQL("ALTER TABLE media_progress ADD COLUMN host TEXT NOT NULL DEFAULT '$host'") | ||
| db.execSQL("ALTER TABLE media_progress ADD COLUMN username TEXT NOT NULL DEFAULT '$username'") | ||
|
|
||
| db.execSQL("ALTER TABLE libraries ADD COLUMN host TEXT NOT NULL DEFAULT '$host'") | ||
| db.execSQL("ALTER TABLE libraries ADD COLUMN username TEXT NOT NULL DEFAULT '$username'") | ||
| } |
There was a problem hiding this comment.
Escape host/username in migration SQL to prevent syntax errors.
Directly interpolating host/username into SQL breaks migrations if values contain ' and is unsafe. Please escape or sanitize before embedding.
✅ Suggested fix (escape quotes before embedding)
fun produceMigration15_16(
host: String,
username: String,
) = object : Migration(15, 16) {
override fun migrate(db: SupportSQLiteDatabase) {
- db.execSQL("ALTER TABLE detailed_books ADD COLUMN host TEXT NOT NULL DEFAULT '$host'")
- db.execSQL("ALTER TABLE detailed_books ADD COLUMN username TEXT NOT NULL DEFAULT '$username'")
+ val safeHost = host.replace("'", "''")
+ val safeUsername = username.replace("'", "''")
+ db.execSQL("ALTER TABLE detailed_books ADD COLUMN host TEXT NOT NULL DEFAULT '$safeHost'")
+ db.execSQL("ALTER TABLE detailed_books ADD COLUMN username TEXT NOT NULL DEFAULT '$safeUsername'")
- db.execSQL("ALTER TABLE media_progress ADD COLUMN host TEXT NOT NULL DEFAULT '$host'")
- db.execSQL("ALTER TABLE media_progress ADD COLUMN username TEXT NOT NULL DEFAULT '$username'")
+ db.execSQL("ALTER TABLE media_progress ADD COLUMN host TEXT NOT NULL DEFAULT '$safeHost'")
+ db.execSQL("ALTER TABLE media_progress ADD COLUMN username TEXT NOT NULL DEFAULT '$safeUsername'")
- db.execSQL("ALTER TABLE libraries ADD COLUMN host TEXT NOT NULL DEFAULT '$host'")
- db.execSQL("ALTER TABLE libraries ADD COLUMN username TEXT NOT NULL DEFAULT '$username'")
+ db.execSQL("ALTER TABLE libraries ADD COLUMN host TEXT NOT NULL DEFAULT '$safeHost'")
+ db.execSQL("ALTER TABLE libraries ADD COLUMN username TEXT NOT NULL DEFAULT '$safeUsername'")
}
}📝 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.
Suggested change
| fun produceMigration15_16( | |
| host: String, | |
| username: String, | |
| ) = object : Migration(15, 16) { | |
| override fun migrate(db: SupportSQLiteDatabase) { | |
| db.execSQL("ALTER TABLE detailed_books ADD COLUMN host TEXT NOT NULL DEFAULT '$host'") | |
| db.execSQL("ALTER TABLE detailed_books ADD COLUMN username TEXT NOT NULL DEFAULT '$username'") | |
| db.execSQL("ALTER TABLE media_progress ADD COLUMN host TEXT NOT NULL DEFAULT '$host'") | |
| db.execSQL("ALTER TABLE media_progress ADD COLUMN username TEXT NOT NULL DEFAULT '$username'") | |
| db.execSQL("ALTER TABLE libraries ADD COLUMN host TEXT NOT NULL DEFAULT '$host'") | |
| db.execSQL("ALTER TABLE libraries ADD COLUMN username TEXT NOT NULL DEFAULT '$username'") | |
| } | |
| fun produceMigration15_16( | |
| host: String, | |
| username: String, | |
| ) = object : Migration(15, 16) { | |
| override fun migrate(db: SupportSQLiteDatabase) { | |
| val safeHost = host.replace("'", "''") | |
| val safeUsername = username.replace("'", "''") | |
| db.execSQL("ALTER TABLE detailed_books ADD COLUMN host TEXT NOT NULL DEFAULT '$safeHost'") | |
| db.execSQL("ALTER TABLE detailed_books ADD COLUMN username TEXT NOT NULL DEFAULT '$safeUsername'") | |
| db.execSQL("ALTER TABLE media_progress ADD COLUMN host TEXT NOT NULL DEFAULT '$safeHost'") | |
| db.execSQL("ALTER TABLE media_progress ADD COLUMN username TEXT NOT NULL DEFAULT '$safeUsername'") | |
| db.execSQL("ALTER TABLE libraries ADD COLUMN host TEXT NOT NULL DEFAULT '$safeHost'") | |
| db.execSQL("ALTER TABLE libraries ADD COLUMN username TEXT NOT NULL DEFAULT '$safeUsername'") | |
| } | |
| } |
🤖 Prompt for AI Agents
In
`@app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/Migrations.kt`
around lines 267 - 280, The migration embeds host and username directly into SQL
which breaks/unsafe if they contain single quotes; in produceMigration15_16
compute escapedHost and escapedUsername (replace single quote ' with two single
quotes '') and use those escaped values when building the ALTER TABLE ...
DEFAULT '...' strings in migrate; reference produceMigration15_16 and the
Migration(15, 16) override to locate where to sanitize host and username before
calling db.execSQL.
Comment on lines
+19
to
+21
| is org.grakovne.lissen.lib.domain.SpecificFilesDownloadOption -> { | ||
| "Selected Volume" | ||
| } |
There was a problem hiding this comment.
Hardcoded string should use string resources for i18n.
The string "Selected Volume" is hardcoded rather than using a string resource like other options in this function. This breaks internationalization support.
🤖 Prompt for AI Agents
In
`@app/src/main/kotlin/org/grakovne/lissen/ui/screens/common/DownloadOptionFormat.kt`
around lines 19 - 21, The branch handling
org.grakovne.lissen.lib.domain.SpecificFilesDownloadOption in
DownloadOptionFormat.kt returns a hardcoded "Selected Volume"; replace it with a
string resource lookup (add a new entry like selected_volume or
download_option_selected_volume to strings.xml) and use the same retrieval
approach used by the other branches in this file (e.g.,
context.getString(R.string.selected_volume) or
stringResource(R.string.selected_volume) if this is a `@Composable`) so the label
is localized consistently.
Duplicate when branch causes unreachable code.
SpecificFilesDownloadOption appears twice in the when expression. The first branch (lines 19-21, returning "Selected Volume") will always match, making the second branch (lines 68-71, returning "Multiple Files") unreachable dead code. This appears to be an unintended duplication.
Suggested fix: Remove duplicate branch
null -> context.getString(R.string.downloads_menu_download_option_disable)
- is org.grakovne.lissen.lib.domain.SpecificFilesDownloadOption -> {
- "Selected Volume"
- }
-
+ is org.grakovne.lissen.lib.domain.SpecificFilesDownloadOption ->
+ context.getString(R.string.downloads_menu_download_option_specific_files) // Add string resource
+
CurrentItemDownloadOption -> {And remove lines 68-71 entirely.
Also applies to: 68-71
🤖 Prompt for AI Agents
In
`@app/src/main/kotlin/org/grakovne/lissen/ui/screens/common/DownloadOptionFormat.kt`
around lines 19 - 21, The when expression in DownloadOptionFormat.kt contains a
duplicate branch for org.grakovne.lissen.lib.domain.SpecificFilesDownloadOption
(one returning "Selected Volume" and a later one returning "Multiple Files"),
causing the latter to be unreachable; fix by removing the unintended duplicate
branch (or, if the later branch was meant for a different type, change its
pattern to the correct download option class) so each download option case in
the when (in the DownloadOptionFormat formatting logic) is unique.
Comment on lines
+106
to
+116
| fun getBookSize(book: DetailedItem) = localCacheRepository.calculateBookSize(book) | ||
|
|
||
| fun calculateChapterSize( | ||
| bookId: String, | ||
| chapter: org.grakovne.lissen.lib.domain.PlayingChapter, | ||
| files: List<org.grakovne.lissen.lib.domain.BookFile>, | ||
| ) = localCacheRepository.calculateChapterSize(bookId, chapter, files) | ||
|
|
||
| fun getBookStorageType(book: DetailedItem) = localCacheRepository.getBookStorageType(book) | ||
|
|
||
| fun getVolumes(book: DetailedItem) = localCacheRepository.mapChaptersToVolumes(book) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the file and understand the structure
find . -name "CachingModelView.kt" -type fRepository: SurjitSahoo/kahani-android
Length of output: 141
🏁 Script executed:
# Also search for localCacheRepository to understand the implementation
find . -name "*Repository*" -type f | grep -i cacheRepository: SurjitSahoo/kahani-android
Length of output: 353
🏁 Script executed:
# Search for the methods being called to understand call patterns
rg "calculateBookSize|calculateChapterSize|getBookStorageType|mapChaptersToVolumes" -t ktRepository: SurjitSahoo/kahani-android
Length of output: 96
🏁 Script executed:
# Read the LocalCacheRepository to see actual implementations
cat ./app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/LocalCacheRepository.ktRepository: SurjitSahoo/kahani-android
Length of output: 11477
🏁 Script executed:
# Search for method calls without type filter
rg "getBookSize|calculateChapterSize|getBookStorageType|mapChaptersToVolumes"Repository: SurjitSahoo/kahani-android
Length of output: 2234
🏁 Script executed:
# Read the CachingModelView.kt file
cat ./app/src/main/kotlin/org/grakovne/lissen/viewmodel/CachingModelView.ktRepository: SurjitSahoo/kahani-android
Length of output: 6934
Wrap size/storage-type helpers with IO dispatcher to avoid main-thread filesystem blocking.
These methods perform synchronous File.length() and File.exists() operations that can trigger StrictMode violations when called from the UI layer (e.g., BookDetailScreen.kt within remember blocks, PlayerScreen.kt). Dispatching to Dispatchers.IO is necessary and aligns with the existing pattern in refreshStorageStats().
💡 Example: dispatch size helpers on IO
- fun getBookSize(book: DetailedItem) = localCacheRepository.calculateBookSize(book)
+ suspend fun getBookSize(book: DetailedItem) =
+ withContext(Dispatchers.IO) { localCacheRepository.calculateBookSize(book) }
- fun calculateChapterSize(
+ suspend fun calculateChapterSize(
bookId: String,
chapter: org.grakovne.lissen.lib.domain.PlayingChapter,
files: List<org.grakovne.lissen.lib.domain.BookFile>,
- ) = localCacheRepository.calculateChapterSize(bookId, chapter, files)
+ ) =
+ withContext(Dispatchers.IO) {
+ localCacheRepository.calculateChapterSize(bookId, chapter, files)
+ }
- fun getBookStorageType(book: DetailedItem) = localCacheRepository.getBookStorageType(book)
+ suspend fun getBookStorageType(book: DetailedItem) =
+ withContext(Dispatchers.IO) { localCacheRepository.getBookStorageType(book) }
- fun getVolumes(book: DetailedItem) = localCacheRepository.mapChaptersToVolumes(book)
+ suspend fun getVolumes(book: DetailedItem) =
+ withContext(Dispatchers.IO) { localCacheRepository.mapChaptersToVolumes(book) }🤖 Prompt for AI Agents
In `@app/src/main/kotlin/org/grakovne/lissen/viewmodel/CachingModelView.kt` around
lines 106 - 116, The four helper methods (getBookSize, calculateChapterSize,
getBookStorageType, getVolumes) call filesystem APIs synchronously; change each
to a suspend function that delegates to Dispatchers.IO by wrapping the
repository call in withContext(Dispatchers.IO) { ... } (e.g., suspend fun
getBookSize(book: DetailedItem) = withContext(Dispatchers.IO) {
localCacheRepository.calculateBookSize(book) }) and similarly for
calculateChapterSize, getBookStorageType, and getVolumes; update any callers
(e.g., places that use these in UI remember blocks or
PlayerScreen/BookDetailScreen) to call them from a coroutine or suspend context.
Comment on lines
+41
to
+44
| } catch (e: Exception) { | ||
| // In a real app, we might want to handle this more gracefully | ||
| _migrationState.value = MigrationState.Completed // Proceed anyway to avoid bricking | ||
| } |
There was a problem hiding this comment.
Log the exception for debugging purposes.
The exception is silently swallowed, making it difficult to diagnose migration failures. Consider logging the exception before proceeding.
🛠️ Suggested fix
} catch (e: Exception) {
- // In a real app, we might want to handle this more gracefully
+ timber.log.Timber.e(e, "Database migration failed")
_migrationState.value = MigrationState.Completed // Proceed anyway to avoid bricking
}📝 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.
Suggested change
| } catch (e: Exception) { | |
| // In a real app, we might want to handle this more gracefully | |
| _migrationState.value = MigrationState.Completed // Proceed anyway to avoid bricking | |
| } | |
| } catch (e: Exception) { | |
| timber.log.Timber.e(e, "Database migration failed") | |
| _migrationState.value = MigrationState.Completed // Proceed anyway to avoid bricking | |
| } |
🤖 Prompt for AI Agents
In `@app/src/main/kotlin/org/grakovne/lissen/viewmodel/MigrationViewModel.kt`
around lines 41 - 44, The catch block in MigrationViewModel is swallowing
exceptions silently; update the catch for Exception in the migration routine to
log the caught exception before setting _migrationState.value =
MigrationState.Completed — use the project's logging facility (e.g., Logger,
Timber, or Android Log) or add one if missing, and include the exception message
and stacktrace along with context (e.g., "Migration failed") so failures in the
migration flow inside MigrationViewModel are recorded for debugging.
Comment on lines
+48
to
+50
| companion object { | ||
| const val CURRENT_DATABASE_VERSION = 16 | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for database version declarations and schema version
rg -n "CURRENT_DATABASE_VERSION|version\s*=" --type=kt -C2 | head -50Repository: SurjitSahoo/kahani-android
Length of output: 96
🏁 Script executed:
#!/bin/bash
# Look for LocalCacheStorage specifically
find . -name "*LocalCacheStorage*" -o -name "*LocalCacheModule*" | head -20Repository: SurjitSahoo/kahani-android
Length of output: 321
🏁 Script executed:
#!/bin/bash
# Search for Room database schema or migration files
find . -name "*.kt" | xargs rg -l "RoomDatabase|@Database" | head -20Repository: SurjitSahoo/kahani-android
Length of output: 157
🏁 Script executed:
#!/bin/bash
# Read LocalCacheStorage.kt to find the `@Database` annotation
cat -n ./app/src/main/kotlin/org/grakovne/lissen/content/cache/persistent/LocalCacheStorage.kt | head -40Repository: SurjitSahoo/kahani-android
Length of output: 1275
🏁 Script executed:
#!/bin/bash
# Check the schema directory - latest version is the highest numbered file
ls -1 ./app/schemas/org.grakovne.lissen.content.cache.persistent.LocalCacheStorage/ | sort -V | tail -5Repository: SurjitSahoo/kahani-android
Length of output: 101
Update CURRENT_DATABASE_VERSION to match the database schema version.
CURRENT_DATABASE_VERSION = 16 is out of sync with @Database(version = 17) in LocalCacheStorage.kt. The schema has been bumped to version 17 (schema file 17.json exists), but the ViewModel constant was not updated. This will cause Room migration failures or repeated migration prompts.
🤖 Prompt for AI Agents
In `@app/src/main/kotlin/org/grakovne/lissen/viewmodel/MigrationViewModel.kt`
around lines 48 - 50, The companion constant CURRENT_DATABASE_VERSION in
MigrationViewModel is out of sync with the Room schema; update
CURRENT_DATABASE_VERSION from 16 to 17 to match the `@Database`(version = 17)
annotation in LocalCacheStorage (and the existing 17.json schema) so migrations
and prompts align; locate the companion object in MigrationViewModel (symbol:
CURRENT_DATABASE_VERSION) and change its value to 17.
| AllItemsDownloadOption -> "all_items" | ||
| CurrentItemDownloadOption -> "current_item" | ||
| is NumberItemDownloadOption -> "number_items_$itemsNumber" | ||
| is SpecificFilesDownloadOption -> "specific_files_${fileIds.joinToString(",")}" |
There was a problem hiding this comment.
Potential issue: fileIds containing commas will break serialization roundtrip.
Using comma as a delimiter in joinToString(",") and split(",") will cause incorrect parsing if any fileId contains a comma character. If file IDs can contain arbitrary characters, consider using a different serialization approach (e.g., JSON array, URL encoding, or a delimiter unlikely to appear in IDs).
Additionally, split(",") on an empty string (when the stored value is "specific_files_") returns [""] rather than an empty list, which may cause unexpected behavior.
Safer parsing example
- startsWith("specific_files_") -> SpecificFilesDownloadOption(substringAfter("specific_files_").split(","))
+ startsWith("specific_files_") -> {
+ val ids = substringAfter("specific_files_")
+ SpecificFilesDownloadOption(if (ids.isEmpty()) emptyList() else ids.split(","))
+ }Also applies to: 38-38
🤖 Prompt for AI Agents
In `@lib/src/main/kotlin/org/grakovne/lissen/lib/domain/DownloadOption.kt` at line
28, The serialization for SpecificFilesDownloadOption using
fileIds.joinToString(",") and corresponding split is unsafe: use a robust
encoding (e.g., JSON array via kotlinx.serialization or URLEncoder/URLDecoder
for each id) instead of a raw comma delimiter, and update the deserialization
logic that reconstructs SpecificFilesDownloadOption to parse the chosen format
and treat an empty list correctly (avoid treating an empty substring as a single
empty id). Locate the SpecificFilesDownloadOption class/enum case and replace
joinToString(",") and the split(",") usage with the safe encoder/decoder and add
explicit handling for an empty collection.
| this == "all_items" -> AllItemsDownloadOption | ||
| this == "current_item" -> CurrentItemDownloadOption | ||
| this == "remaining_items" -> RemainingItemsDownloadOption | ||
| startsWith("number_items_") -> NumberItemDownloadOption(substringAfter("number_items_").toInt()) |
There was a problem hiding this comment.
Potential NumberFormatException on malformed input.
substringAfter("number_items_").toInt() will throw if the stored string is corrupted or malformed. Consider using toIntOrNull() with a fallback or returning null for invalid input.
Suggested defensive parsing
- startsWith("number_items_") -> NumberItemDownloadOption(substringAfter("number_items_").toInt())
+ startsWith("number_items_") -> substringAfter("number_items_").toIntOrNull()?.let { NumberItemDownloadOption(it) }🤖 Prompt for AI Agents
In `@lib/src/main/kotlin/org/grakovne/lissen/lib/domain/DownloadOption.kt` at line
37, The branch that constructs NumberItemDownloadOption using
substringAfter("number_items_").toInt() can throw NumberFormatException on
malformed input; update the parsing in the DownloadOption mapping (the
startsWith("number_items_") -> NumberItemDownloadOption(...) case) to use
toIntOrNull() and handle null by either returning null (or a safe fallback
option) instead of throwing, so invalid or corrupted strings are handled
gracefully.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes