Feat: offline cache when server is unreachable#188
Open
dahool wants to merge 9 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds Room entities/DAOs and mappers, Hilt DatabaseModule, Store SourceOfTruth wiring, a Hilt Coroutine SyncWorker with WorkManager configuration, and enqueues an initial one-time sync from SplashViewModel; build/IDE/schema metadata updated elsewhere. ChangesRoom + Sync integration
Sequence DiagramsequenceDiagram
participant SplashVM as SplashViewModel
participant WM as WorkManager
participant Sync as SyncWorker
participant Stores as Stores (RecipePreviewsStore / RecipeStore / CategoriesStore)
participant DB as Room DAOs
SplashVM->>SplashVM: accountRepository.getAccount() -> Resource.Success
SplashVM->>WM: enqueueUniqueWork("sync_initial")
WM->>Sync: start work
Sync->>Stores: refresh previews & categories
Stores-->>Sync: List<PreviewDto>, List<CategoryDto>
loop per preview
Sync->>Stores: recipeStore.fresh(id)
Stores-->>Sync: RecipeDto or error
Sync->>DB: upsert recipe / replace previews / replace categories
DB-->>Sync: ack
end
alt any per-item failures
Sync-->>WM: Result.retry()
else
Sync-->>WM: Result.success()
end
SplashVM->>SplashVM: update state -> Authorized
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 10
🧹 Nitpick comments (4)
app/full/release/output-metadata.json (1)
1-37: Avoid committing generated release output metadataThis JSON is a build artifact, not source code. Keeping it in git will create noisy diffs on every release/version bump and can drift from CI-produced artifacts. Prefer excluding
app/**/release/(or at leastoutput-metadata.json) via.gitignoreand generating it during build/release steps.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/full/release/output-metadata.json` around lines 1 - 37, The committed file output-metadata.json is a generated release build artifact (contains keys like "version", "variantName": "fullRelease", "outputFile": "app-full-release.apk" and "baselineProfiles"), so remove this file from the repo and prevent future commits by adding an ignore rule for generated release metadata (e.g., exclude app/**/release/ or at least app/**/release/output-metadata.json) in .gitignore; ensure your release/CI pipeline recreates output-metadata.json during the build instead of tracking it in source control.app/src/main/java/de/lukasneugebauer/nextcloudcookbook/auth/presentation/splash/SplashViewModel.kt (3)
53-55: Consider usingSyncWorker.WORK_NAMEconstant for consistency.The work is enqueued with
"sync_initial"whileSyncWorker.WORK_NAMEis"sync_recipes". If these are intentionally different (initial one-time vs periodic), consider adding a constant for the initial sync name inSyncWorkerto maintain consistency and avoid magic strings:// In SyncWorker companion object: const val INITIAL_WORK_NAME = "sync_initial"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/de/lukasneugebauer/nextcloudcookbook/auth/presentation/splash/SplashViewModel.kt` around lines 53 - 55, The SplashViewModel enqueues work using the literal "sync_initial" while SyncWorker defines WORK_NAME = "sync_recipes"; add a constant (e.g., INITIAL_WORK_NAME = "sync_initial") to SyncWorker's companion object and replace the hardcoded "sync_initial" in SplashViewModel.enqueueUniqueWork with that constant (or, if both names are intentionally different, add the new constant and use it to avoid magic strings and keep naming consistent).
50-59: Simplify:viewModelScope.launchis unnecessary here.
WorkManager.enqueueUniqueWork()is not a suspend function—it returns synchronously. The coroutine wrapper adds no value.Also consider injecting
WorkManagerdirectly instead ofContextfor better testability:♻️ Proposed refactor
class SplashViewModel `@Inject` constructor( private val accountRepository: AccountRepository, - `@ApplicationContext` private val context: Context, + private val workManager: WorkManager, ) : ViewModel() {private fun triggerInitialSync() { - viewModelScope.launch { - WorkManager.getInstance(context) - .enqueueUniqueWork( - "sync_initial", - ExistingWorkPolicy.KEEP, - SyncWorker.buildOneTimeRequest() - ) - } + workManager.enqueueUniqueWork( + "sync_initial", + ExistingWorkPolicy.KEEP, + SyncWorker.buildOneTimeRequest() + ) }Add to your DI module:
`@Provides` `@Singleton` fun provideWorkManager(`@ApplicationContext` context: Context): WorkManager = WorkManager.getInstance(context)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/de/lukasneugebauer/nextcloudcookbook/auth/presentation/splash/SplashViewModel.kt` around lines 50 - 59, triggerInitialSync() wraps a synchronous WorkManager call in viewModelScope.launch unnecessarily; remove the coroutine and call WorkManager.getInstance(context).enqueueUniqueWork(...) directly, and for testability replace the Context dependency with an injected WorkManager instance (use provideWorkManager in DI) so triggerInitialSync() calls workManager.enqueueUniqueWork("sync_initial", ExistingWorkPolicy.KEEP, SyncWorker.buildOneTimeRequest()) without using viewModelScope.launch.
6-10: Remove unused imports.
Constraints,NetworkType, andOneTimeWorkRequestBuilderare imported but not used—SyncWorker.buildOneTimeRequest()handles the request construction internally.🧹 Proposed fix
import android.content.Context import androidx.lifecycle.ViewModel import androidx.lifecycle.viewModelScope -import androidx.work.Constraints import androidx.work.ExistingWorkPolicy -import androidx.work.NetworkType -import androidx.work.OneTimeWorkRequestBuilder import androidx.work.WorkManager🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/de/lukasneugebauer/nextcloudcookbook/auth/presentation/splash/SplashViewModel.kt` around lines 6 - 10, Remove the unused imports in SplashViewModel.kt—delete Constraints, NetworkType, and OneTimeWorkRequestBuilder from the import list (they’re unused because SyncWorker.buildOneTimeRequest() constructs the request). Keep ExistingWorkPolicy and WorkManager if they’re referenced in SplashViewModel, and run an import/optimize step to clean up any remaining unused imports after removing those three.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/build.gradle`:
- Around line 112-113: SyncWorker uses `@HiltWorker` /
androidx.hilt.work.HiltWorker but the hilt-work library is not declared; add the
androidx.hilt:hilt-work dependency to the module's dependencies (next to the
existing implementation 'androidx.hilt:hilt-common') so `@HiltWorker` and
HiltWorker types are available at build time.
In
`@app/src/main/java/de/lukasneugebauer/nextcloudcookbook/core/data/CookbookDatabase.kt`:
- Around line 12-22: The Room database annotation on CookbookDatabase sets
exportSchema = true but no schema output path is configured; update Gradle KSP
args to provide Room's schemaLocation by adding the ksp block in
app/build.gradle with arg "room.schemaLocation" pointing to a schemas directory
(e.g. "$projectDir/schemas") so Room can export schemas for CookbookDatabase
(and entities RecipePreviewEntity, RecipeEntity, CategoryEntity); ensure the
directory exists or is created as part of the build.
In
`@app/src/main/java/de/lukasneugebauer/nextcloudcookbook/core/worker/SyncWorker.kt`:
- Around line 36-50: The worker currently catches Exception and thus swallows
CancellationException and also always returns Result.success() even if some
recipe syncs failed; update SyncWorker so CancellationException is rethrown (do
not catch/suppress it) both for the outer try/catch around the whole work and
inside the per-item loop, and track per-recipe failures during previews.forEach
when calling recipeStore.fresh(previewDto.id ?: previewDto.recipeId); if any
individual sync fails (except CancellationException) mark that a partial failure
occurred and return Result.retry() instead of Result.success(); keep logging via
Timber.w/e but ensure CancellationException is not logged/converted to success.
In `@app/src/main/java/de/lukasneugebauer/nextcloudcookbook/di/CategoryModule.kt`:
- Around line 43-46: The reader currently maps Room entities to DTOs then uses
takeIf { it.isNotEmpty() } which turns a valid empty list into null; change the
reader in CategoryModule (the reader lambda that calls categoryDao.getAll()) to
return the mapped List<CategoryDto> as-is (allow empty lists) instead of using
takeIf. For the writer, avoid issuing categoryDao.deleteAll() and
categoryDao.upsertAll() as two separate calls; perform both operations inside a
single database transaction (e.g., via your RoomDatabase.runInTransaction or a
`@Transaction` DAO method like replaceAll) so readers never see inconsistent
intermediate state; update the writer to call the transactional
replace/replace-all sequence using categoryDao.deleteAll() and
categoryDao.upsertAll() inside that transaction.
- Around line 48-50: The writer lambda currently calls categoryDao.deleteAll()
and categoryDao.upsertAll(...) separately; wrap both operations in a single Room
transaction to make them atomic by using withTransaction (from room-ktx) on the
database or by adding a DAO helper method that performs both operations inside
`@Transaction`; update the writer to call the transaction wrapper so deleteAll and
upsertAll are executed atomically (reference: categoryDao.deleteAll,
categoryDao.upsertAll, withTransaction, CategoryDto writer lambda).
In `@app/src/main/java/de/lukasneugebauer/nextcloudcookbook/di/DatabaseModule.kt`:
- Around line 29-30: The Room builder in DatabaseModule.kt currently uses
fallbackToDestructiveMigration(true) which will wipe cached recipes/categories
on schema changes; replace this with explicit Room migrations by creating
Migration classes (e.g., Migration_1_2, Migration_2_3, etc.) that implement the
necessary SQL transformations and register them on the builder via
.addMigrations(Migration_1_2(), Migration_2_3(), ...); remove or limit
fallbackToDestructiveMigration to debug-only builds so production uses the added
migrations and preserves offline cache data.
In `@app/src/main/java/de/lukasneugebauer/nextcloudcookbook/di/RecipeModule.kt`:
- Around line 91-95: The delete lambda currently ignores the categoryName and
calls recipePreviewDao.deleteAll(), causing cross-category invalidation; add a
new DAO method deleteByCategory(categoryName: String) on RecipePreviewDao and
change the delete assignment to call that (delete = { categoryName: String ->
recipePreviewDao.deleteByCategory(categoryName) }). Also update the writer
lambda (writer = { _, dtos -> recipePreviewDao.upsertAll(dtos.map {
it.toEntity() }) }) to either clear existing previews for the category before
upserting (e.g., call recipePreviewDao.deleteByCategory(categoryName) prior to
upsert) or ensure the reader (recipePreviewDao.getByCategory) is the sole source
of truth so stale previews aren’t kept.
- Around line 64-66: The current writer lambda calls
recipePreviewDao.deleteAll() before converting DTOs which can clear the cache if
RecipePreviewDto.toEntity() throws; instead map dtos to entities first (calling
RecipePreviewDto.toEntity() for each and validating), then perform the replace
atomically by executing deleteAll() followed by upsertAll(entities) inside a
single transaction (either use the DAO/database runInTransaction utility or add
a `@Transaction` DAO method like replaceAll that calls deleteAll() and
upsertAll(entities)); update the writer to supply the pre-mapped entities to
that transactional replace operation.
- Around line 59-62: The reader lambdas for the RecipePreviewsStore and
RecipePreviewsByCategoryStore are currently calling .takeIf { it.isNotEmpty() }
which turns empty lists into null and causes Store5 to treat them as cache
misses; update the reader in the reader = { _: Any ->
recipePreviewDao.getAll().map { entities -> entities.map { it.toDto() } } } and
the corresponding by-category reader (the lambda using
recipePreviewDao.getByCategory(...) / .toDto()) to stop using .takeIf so that
empty lists are returned and cached as valid data.
In
`@app/src/main/java/de/lukasneugebauer/nextcloudcookbook/recipe/data/dto/mapper/RecipePreviewEntityMapper.kt`:
- Around line 19-22: In RecipePreviewDto.toEntity ensure the fallback recipeId
is validated for not-null AND not-blank before using it for
RecipePreviewEntity.id; change the current null-only Elvis check so that you
first prefer id when not blank, otherwise use recipeId only if not null/blank,
and if both are null/blank throw the IllegalStateException — update the id
assignment in toEntity to explicitly reject blank strings for both id and
recipeId so you never persist an empty primary key.
---
Nitpick comments:
In `@app/full/release/output-metadata.json`:
- Around line 1-37: The committed file output-metadata.json is a generated
release build artifact (contains keys like "version", "variantName":
"fullRelease", "outputFile": "app-full-release.apk" and "baselineProfiles"), so
remove this file from the repo and prevent future commits by adding an ignore
rule for generated release metadata (e.g., exclude app/**/release/ or at least
app/**/release/output-metadata.json) in .gitignore; ensure your release/CI
pipeline recreates output-metadata.json during the build instead of tracking it
in source control.
In
`@app/src/main/java/de/lukasneugebauer/nextcloudcookbook/auth/presentation/splash/SplashViewModel.kt`:
- Around line 53-55: The SplashViewModel enqueues work using the literal
"sync_initial" while SyncWorker defines WORK_NAME = "sync_recipes"; add a
constant (e.g., INITIAL_WORK_NAME = "sync_initial") to SyncWorker's companion
object and replace the hardcoded "sync_initial" in
SplashViewModel.enqueueUniqueWork with that constant (or, if both names are
intentionally different, add the new constant and use it to avoid magic strings
and keep naming consistent).
- Around line 50-59: triggerInitialSync() wraps a synchronous WorkManager call
in viewModelScope.launch unnecessarily; remove the coroutine and call
WorkManager.getInstance(context).enqueueUniqueWork(...) directly, and for
testability replace the Context dependency with an injected WorkManager instance
(use provideWorkManager in DI) so triggerInitialSync() calls
workManager.enqueueUniqueWork("sync_initial", ExistingWorkPolicy.KEEP,
SyncWorker.buildOneTimeRequest()) without using viewModelScope.launch.
- Around line 6-10: Remove the unused imports in SplashViewModel.kt—delete
Constraints, NetworkType, and OneTimeWorkRequestBuilder from the import list
(they’re unused because SyncWorker.buildOneTimeRequest() constructs the
request). Keep ExistingWorkPolicy and WorkManager if they’re referenced in
SplashViewModel, and run an import/optimize step to clean up any remaining
unused imports after removing those three.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff6c0b01-751c-4b18-99bb-93b29267ea32
📒 Files selected for processing (19)
.idea/markdown.xmlapp/build.gradleapp/full/release/baselineProfiles/0/app-full-release.dmapp/full/release/baselineProfiles/1/app-full-release.dmapp/full/release/output-metadata.jsonapp/src/main/java/de/lukasneugebauer/nextcloudcookbook/auth/presentation/splash/SplashViewModel.ktapp/src/main/java/de/lukasneugebauer/nextcloudcookbook/category/data/dto/mapper/CategoryEntityMapper.ktapp/src/main/java/de/lukasneugebauer/nextcloudcookbook/category/domain/dao/CategoryDao.ktapp/src/main/java/de/lukasneugebauer/nextcloudcookbook/category/domain/model/CategoryEntity.ktapp/src/main/java/de/lukasneugebauer/nextcloudcookbook/core/data/CookbookDatabase.ktapp/src/main/java/de/lukasneugebauer/nextcloudcookbook/core/worker/SyncWorker.ktapp/src/main/java/de/lukasneugebauer/nextcloudcookbook/di/CategoryModule.ktapp/src/main/java/de/lukasneugebauer/nextcloudcookbook/di/DatabaseModule.ktapp/src/main/java/de/lukasneugebauer/nextcloudcookbook/di/RecipeModule.ktapp/src/main/java/de/lukasneugebauer/nextcloudcookbook/recipe/data/dto/mapper/RecipePreviewEntityMapper.ktapp/src/main/java/de/lukasneugebauer/nextcloudcookbook/recipe/domain/dao/RecipeDao.ktapp/src/main/java/de/lukasneugebauer/nextcloudcookbook/recipe/domain/dao/RecipePreviewDao.ktapp/src/main/java/de/lukasneugebauer/nextcloudcookbook/recipe/domain/model/RecipeEntity.ktapp/src/main/java/de/lukasneugebauer/nextcloudcookbook/recipe/domain/model/RecipePreviewEntity.kt
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/de/lukasneugebauer/nextcloudcookbook/di/RecipeModule.kt`:
- Around line 53-62: RecipePreviewsStore currently uses a mismatched key type:
Fetcher.of ignores the key (calls apiProvider.getApi()?.getRecipes()) while the
SourceOfTruth reader/writer use categoryName.toString(); fix by choosing one of
two clear options—either make the store key Unit (if you intend to fetch all
recipes) by changing Store<Any,...> to Store<Unit,...> and update Fetcher.of to
accept Unit and keep apiProvider.getApi()?.getRecipes(), or make the store key a
String (category) by changing Store<Any,...> to Store<String,...>, update
Fetcher.of to accept categoryName: String and call
apiProvider.getApi()?.getRecipesByCategory(categoryName) and ensure
writer/reader still use recipePreviewDao.getByCategory(categoryName) so
Fetcher.of, reader, recipePreviewDao.getByCategory, and apiProvider.getApi() are
consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d21dd580-028e-456f-9f12-095090e85e33
📒 Files selected for processing (12)
.gitignoreapp/build.gradleapp/schemas/de.lukasneugebauer.nextcloudcookbook.core.data.CookbookDatabase/1.jsonapp/src/main/AndroidManifest.xmlapp/src/main/java/de/lukasneugebauer/nextcloudcookbook/NextcloudCookbookApplication.ktapp/src/main/java/de/lukasneugebauer/nextcloudcookbook/category/domain/dao/CategoryDao.ktapp/src/main/java/de/lukasneugebauer/nextcloudcookbook/core/worker/SyncWorker.ktapp/src/main/java/de/lukasneugebauer/nextcloudcookbook/di/CategoryModule.ktapp/src/main/java/de/lukasneugebauer/nextcloudcookbook/di/DatabaseModule.ktapp/src/main/java/de/lukasneugebauer/nextcloudcookbook/di/RecipeModule.ktapp/src/main/java/de/lukasneugebauer/nextcloudcookbook/recipe/data/dto/mapper/RecipePreviewEntityMapper.ktapp/src/main/java/de/lukasneugebauer/nextcloudcookbook/recipe/domain/dao/RecipePreviewDao.kt
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- app/schemas/de.lukasneugebauer.nextcloudcookbook.core.data.CookbookDatabase/1.json
🚧 Files skipped from review as they are similar to previous changes (5)
- app/src/main/java/de/lukasneugebauer/nextcloudcookbook/category/domain/dao/CategoryDao.kt
- app/src/main/java/de/lukasneugebauer/nextcloudcookbook/di/DatabaseModule.kt
- app/build.gradle
- app/src/main/java/de/lukasneugebauer/nextcloudcookbook/recipe/data/dto/mapper/RecipePreviewEntityMapper.kt
- app/src/main/java/de/lukasneugebauer/nextcloudcookbook/recipe/domain/dao/RecipePreviewDao.kt
Owner
|
Thank you for your work @dahool! Would be great if you can fix the ktlint issues flagged in CI. |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
app/src/main/java/de/lukasneugebauer/nextcloudcookbook/di/RecipeModule.kt (2)
60-63:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEmpty list converted to
nullprevents legitimate empty-state caching.The
.takeIf { it.isNotEmpty() }transforms an empty list tonull, which Store5 interprets as a cache miss rather than valid cached data. This prevents offline users from seeing "no recipes" when the server genuinely has none, and may trigger unnecessary network fetches.Return the mapped list directly without the
takeIffilter.Proposed fix
reader = { recipePreviewDao.getAll().map { entities -> - entities.map { it.toDto() }.takeIf { it.isNotEmpty() } + entities.map { it.toDto() } } },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/de/lukasneugebauer/nextcloudcookbook/di/RecipeModule.kt` around lines 60 - 63, The reader lambda in RecipeModule using recipePreviewDao.getAll().map { entities -> entities.map { it.toDto() }.takeIf { it.isNotEmpty() } } converts an empty list to null which Store5 treats as a cache miss; remove the takeIf and return the mapped list directly (i.e., return entities.map { it.toDto() }) so an empty list is cached as a valid value; update the reader closure that references recipePreviewDao.getAll() and toDto() accordingly.
89-92:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSame empty-list issue in the by-category store reader.
The
.takeIf { it.isNotEmpty() }here also converts empty categories to cache misses, causing the same offline caching problem.Proposed fix
reader = { categoryName -> recipePreviewDao.getByCategory(categoryName).map { entities -> - entities.map { it.toDto() }.takeIf { it.isNotEmpty() } + entities.map { it.toDto() } } },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/de/lukasneugebauer/nextcloudcookbook/di/RecipeModule.kt` around lines 89 - 92, The reader lambda for by-category currently maps entities to DTOs and then uses .takeIf { it.isNotEmpty() }, which turns empty category lists into cache misses; update the reader (the lambda assigned to reader, the call to recipePreviewDao.getByCategory, and the mapping through .toDto()) to return the mapped list directly (do not use .takeIf) so empty lists are cached instead of treated as missing data.
🧹 Nitpick comments (3)
app/src/main/java/de/lukasneugebauer/nextcloudcookbook/core/worker/SyncWorker.kt (1)
20-20: 💤 Low valuePrefer
kotlinx.coroutines.CancellationExceptionimport.While
java.util.concurrent.CancellationExceptionworks (Kotlin'sCancellationExceptionextends it), usingkotlinx.coroutines.CancellationExceptionis more idiomatic in coroutine code and makes the intent clearer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/de/lukasneugebauer/nextcloudcookbook/core/worker/SyncWorker.kt` at line 20, Replace the java.util.concurrent.CancellationException import with kotlinx.coroutines.CancellationException in SyncWorker.kt so coroutine cancellations are handled idiomatically; update the import statement that references CancellationException and ensure any catches or throws in the SyncWorker class (e.g., inside functions/methods that catch CancellationException) continue to reference the same symbol but from kotlinx.coroutines.app/src/main/java/de/lukasneugebauer/nextcloudcookbook/di/DatabaseModule.kt (1)
22-30: 💤 Low valueConsider adding a migration strategy for future schema changes.
The database builder doesn't specify any migration handling. While this is acceptable for the initial database version, you'll need to add either explicit migrations or
fallbackToDestructiveMigration()before any schema changes in future releases. Without a strategy, Room will throwIllegalStateExceptionon version bumps.For an offline-first cache, explicit migrations are preferred to preserve user data, but
fallbackToDestructiveMigration(true)is acceptable since the data can be re-synced from the server.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/de/lukasneugebauer/nextcloudcookbook/di/DatabaseModule.kt` around lines 22 - 30, The Room database builder in provideDatabase (function provideDatabase) currently calls Room.databaseBuilder(..., CookbookDatabase::class.java, "cookbook.db").build() with no migration strategy; update this builder inside provideDatabase to include either explicit Migration objects via .addMigrations(...) using migrations you create for schema version bumps, or append .fallbackToDestructiveMigration() if destructive fallback is acceptable for this cache, so Room will not throw on version changes.app/src/main/java/de/lukasneugebauer/nextcloudcookbook/auth/presentation/splash/SplashViewModel.kt (1)
47-57: 💤 Low valueUnnecessary coroutine wrapper around synchronous WorkManager call.
WorkManager.getInstance().enqueueUniqueWork()is a synchronous call that returns immediately after enqueueing. TheviewModelScope.launchwrapper adds no value here and can be removed.Proposed simplification
private fun triggerInitialSync() { - viewModelScope.launch { - WorkManager - .getInstance(context) - .enqueueUniqueWork( - "sync_initial", - ExistingWorkPolicy.KEEP, - SyncWorker.buildOneTimeRequest(), - ) - } + WorkManager + .getInstance(context) + .enqueueUniqueWork( + "sync_initial", + ExistingWorkPolicy.KEEP, + SyncWorker.buildOneTimeRequest(), + ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/de/lukasneugebauer/nextcloudcookbook/auth/presentation/splash/SplashViewModel.kt` around lines 47 - 57, The triggerInitialSync() function wraps a synchronous WorkManager call in viewModelScope.launch unnecessarily; remove the coroutine wrapper and directly call WorkManager.getInstance(context).enqueueUniqueWork("sync_initial", ExistingWorkPolicy.KEEP, SyncWorker.buildOneTimeRequest()) inside triggerInitialSync(), keeping the function non-suspending and preserving the same unique work name and policy.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@app/src/main/java/de/lukasneugebauer/nextcloudcookbook/di/RecipeModule.kt`:
- Around line 60-63: The reader lambda in RecipeModule using
recipePreviewDao.getAll().map { entities -> entities.map { it.toDto() }.takeIf {
it.isNotEmpty() } } converts an empty list to null which Store5 treats as a
cache miss; remove the takeIf and return the mapped list directly (i.e., return
entities.map { it.toDto() }) so an empty list is cached as a valid value; update
the reader closure that references recipePreviewDao.getAll() and toDto()
accordingly.
- Around line 89-92: The reader lambda for by-category currently maps entities
to DTOs and then uses .takeIf { it.isNotEmpty() }, which turns empty category
lists into cache misses; update the reader (the lambda assigned to reader, the
call to recipePreviewDao.getByCategory, and the mapping through .toDto()) to
return the mapped list directly (do not use .takeIf) so empty lists are cached
instead of treated as missing data.
---
Nitpick comments:
In
`@app/src/main/java/de/lukasneugebauer/nextcloudcookbook/auth/presentation/splash/SplashViewModel.kt`:
- Around line 47-57: The triggerInitialSync() function wraps a synchronous
WorkManager call in viewModelScope.launch unnecessarily; remove the coroutine
wrapper and directly call
WorkManager.getInstance(context).enqueueUniqueWork("sync_initial",
ExistingWorkPolicy.KEEP, SyncWorker.buildOneTimeRequest()) inside
triggerInitialSync(), keeping the function non-suspending and preserving the
same unique work name and policy.
In
`@app/src/main/java/de/lukasneugebauer/nextcloudcookbook/core/worker/SyncWorker.kt`:
- Line 20: Replace the java.util.concurrent.CancellationException import with
kotlinx.coroutines.CancellationException in SyncWorker.kt so coroutine
cancellations are handled idiomatically; update the import statement that
references CancellationException and ensure any catches or throws in the
SyncWorker class (e.g., inside functions/methods that catch
CancellationException) continue to reference the same symbol but from
kotlinx.coroutines.
In `@app/src/main/java/de/lukasneugebauer/nextcloudcookbook/di/DatabaseModule.kt`:
- Around line 22-30: The Room database builder in provideDatabase (function
provideDatabase) currently calls Room.databaseBuilder(...,
CookbookDatabase::class.java, "cookbook.db").build() with no migration strategy;
update this builder inside provideDatabase to include either explicit Migration
objects via .addMigrations(...) using migrations you create for schema version
bumps, or append .fallbackToDestructiveMigration() if destructive fallback is
acceptable for this cache, so Room will not throw on version changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bd05ebe1-2d36-41af-96bf-759900875fdb
📒 Files selected for processing (15)
app/src/main/java/de/lukasneugebauer/nextcloudcookbook/NextcloudCookbookApplication.ktapp/src/main/java/de/lukasneugebauer/nextcloudcookbook/auth/presentation/splash/SplashViewModel.ktapp/src/main/java/de/lukasneugebauer/nextcloudcookbook/category/data/dto/mapper/CategoryEntityMapper.ktapp/src/main/java/de/lukasneugebauer/nextcloudcookbook/category/domain/dao/CategoryDao.ktapp/src/main/java/de/lukasneugebauer/nextcloudcookbook/category/domain/model/CategoryEntity.ktapp/src/main/java/de/lukasneugebauer/nextcloudcookbook/core/data/CookbookDatabase.ktapp/src/main/java/de/lukasneugebauer/nextcloudcookbook/core/worker/SyncWorker.ktapp/src/main/java/de/lukasneugebauer/nextcloudcookbook/di/CategoryModule.ktapp/src/main/java/de/lukasneugebauer/nextcloudcookbook/di/DatabaseModule.ktapp/src/main/java/de/lukasneugebauer/nextcloudcookbook/di/RecipeModule.ktapp/src/main/java/de/lukasneugebauer/nextcloudcookbook/recipe/data/dto/mapper/RecipePreviewEntityMapper.ktapp/src/main/java/de/lukasneugebauer/nextcloudcookbook/recipe/domain/dao/RecipeDao.ktapp/src/main/java/de/lukasneugebauer/nextcloudcookbook/recipe/domain/dao/RecipePreviewDao.ktapp/src/main/java/de/lukasneugebauer/nextcloudcookbook/recipe/domain/model/RecipeEntity.ktapp/src/main/java/de/lukasneugebauer/nextcloudcookbook/recipe/domain/model/RecipePreviewEntity.kt
dahool
changed the title
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/de/lukasneugebauer/nextcloudcookbook/core/worker/SyncWorker.kt (1)
42-46:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject blank fallback IDs before syncing.
Line 43 filters blank
previewDto.id, but a blankpreviewDto.recipeIdstill passes Line 44 and can callrecipeStore.fresh(""). That turns malformed preview data into a bogus fetch and a worker retry loop.Suggested fix
previews.forEach { previewDto -> - val id = if (!previewDto.id.isNullOrBlank()) previewDto.id else previewDto.recipeId - if (id != null) { + val id = + previewDto.id?.takeUnless { it.isBlank() } + ?: previewDto.recipeId?.takeUnless { it.isBlank() } + if (id != null) { try { recipeStore.fresh(id)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/de/lukasneugebauer/nextcloudcookbook/core/worker/SyncWorker.kt` around lines 42 - 46, In SyncWorker inside the previews.forEach loop, avoid calling recipeStore.fresh with an empty string by rejecting blank fallback IDs: compute the id by preferring previewDto.id only if not null/blank, otherwise use previewDto.recipeId only if not null/blank (e.g. use takeIf / isNullOrBlank checks on both previewDto.id and previewDto.recipeId), and only call recipeStore.fresh(id) when that resulting id is non-null; reference previewDto.id, previewDto.recipeId, the previews.forEach block, and recipeStore.fresh.
🧹 Nitpick comments (1)
app/src/main/java/de/lukasneugebauer/nextcloudcookbook/auth/presentation/splash/SplashViewModel.kt (1)
33-45: ⚡ Quick winMove the account-fetch subscription to
initfor cleaner lifecycle handling.Currently
initialize()is called fromLaunchedEffect(key1 = uiState)in the composable, which relies on state transitions to prevent multiple invocations. While this works in practice, moving the subscription into the ViewModel'sinitblock would eliminate the external dependency and make the flow clearer. The method would no longer need to be called from the UI layer at all.init { accountRepository .getAccount() .distinctUntilChanged() .onEach { account -> if (account is Resource.Success) { triggerInitialSync() _uiState.update { SplashScreenState.Authorized } } else { _uiState.update { SplashScreenState.Unauthorized } } }.launchIn(viewModelScope) }Then remove the
initialize()method and the call fromSplashScreen.kt.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/de/lukasneugebauer/nextcloudcookbook/auth/presentation/splash/SplashViewModel.kt` around lines 33 - 45, The account subscription in SplashViewModel.initialize() should be moved into the ViewModel init block to manage lifecycle inside the ViewModel: copy the accountRepository.getAccount().distinctUntilChanged().onEach { ... }.launchIn(viewModelScope) subscription into init, keep the same handling (triggerInitialSync(), _uiState.update to Authorized/Unauthorized), then remove the initialize() method and delete the call to initialize() from SplashScreen.kt so the UI no longer drives the subscription; ensure triggerInitialSync() and _uiState usage remain unchanged in the new init-based subscription.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@app/src/main/java/de/lukasneugebauer/nextcloudcookbook/core/worker/SyncWorker.kt`:
- Around line 42-46: In SyncWorker inside the previews.forEach loop, avoid
calling recipeStore.fresh with an empty string by rejecting blank fallback IDs:
compute the id by preferring previewDto.id only if not null/blank, otherwise use
previewDto.recipeId only if not null/blank (e.g. use takeIf / isNullOrBlank
checks on both previewDto.id and previewDto.recipeId), and only call
recipeStore.fresh(id) when that resulting id is non-null; reference
previewDto.id, previewDto.recipeId, the previews.forEach block, and
recipeStore.fresh.
---
Nitpick comments:
In
`@app/src/main/java/de/lukasneugebauer/nextcloudcookbook/auth/presentation/splash/SplashViewModel.kt`:
- Around line 33-45: The account subscription in SplashViewModel.initialize()
should be moved into the ViewModel init block to manage lifecycle inside the
ViewModel: copy the accountRepository.getAccount().distinctUntilChanged().onEach
{ ... }.launchIn(viewModelScope) subscription into init, keep the same handling
(triggerInitialSync(), _uiState.update to Authorized/Unauthorized), then remove
the initialize() method and delete the call to initialize() from SplashScreen.kt
so the UI no longer drives the subscription; ensure triggerInitialSync() and
_uiState usage remain unchanged in the new init-based subscription.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: be3d1dc7-a211-4500-b4e0-721bcf9a6d4e
📒 Files selected for processing (2)
app/src/main/java/de/lukasneugebauer/nextcloudcookbook/auth/presentation/splash/SplashViewModel.ktapp/src/main/java/de/lukasneugebauer/nextcloudcookbook/core/worker/SyncWorker.kt
lneugebauer
requested changes
May 24, 2026
Owner
There was a problem hiding this comment.
Please remove the file and add it to .gitignore.
Owner
There was a problem hiding this comment.
Please remove the file. Looks like it has been added before the new .gitignore rule was added.
| fun provideRecipePreviewsStore(apiProvider: NcCookbookApiProvider): RecipePreviewsStore = | ||
| fun provideRecipePreviewsByCategoryStore( | ||
| apiProvider: NcCookbookApiProvider, | ||
| recipePreviewDao: RecipePreviewDao, |
Owner
There was a problem hiding this comment.
It looks like you are writing into the same db table as for the recipe previews without category. I think they should have their own tables as this might introduce unforseeable side effects.
Comment on lines
+27
to
+29
| category = | ||
| categoryOverride ?: category | ||
| ?: throw IllegalStateException("Category is required (either from DTO or override)"), |
Owner
There was a problem hiding this comment.
This errors in my dev build. I suspect this might be due to recipes without categories.
org.mobilenativefoundation.store.store5.SourceOfTruth$WriteException: Failed to write value to Source of Truth. key: kotlin.Unit
at org.mobilenativefoundation.store.store5.impl.SourceOfTruthWithBarrier.write(SourceOfTruthWithBarrier.kt:160)
at org.mobilenativefoundation.store.store5.impl.FetcherController$fetchers$1$4.invokeSuspend(FetcherController.kt:110)
at org.mobilenativefoundation.store.store5.impl.FetcherController$fetchers$1$4.invoke(Unknown Source:8)
at org.mobilenativefoundation.store.store5.impl.FetcherController$fetchers$1$4.invoke(Unknown Source:4)
at org.mobilenativefoundation.store.multicast5.StoreChannelManager$Actor.doDispatchValue(ChannelManager.kt:286)
at org.mobilenativefoundation.store.multicast5.StoreChannelManager$Actor.handle(ChannelManager.kt:218)
at org.mobilenativefoundation.store.multicast5.StoreChannelManager$Actor.handle(ChannelManager.kt:186)
at org.mobilenativefoundation.store.multicast5.StoreRealActor$1.invokeSuspend(StoreRealActor.kt:47)
at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:34)
at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:100)
at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:586)
at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:829)
at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:717)
at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:704)
Caused by: java.lang.IllegalStateException: Category is required (either from DTO or override)
at de.lukasneugebauer.nextcloudcookbook.recipe.data.dto.mapper.RecipePreviewEntityMapperKt.toEntity(RecipePreviewEntityMapper.kt:29)
at de.lukasneugebauer.nextcloudcookbook.recipe.data.dto.mapper.RecipePreviewEntityMapperKt.toEntity$default(RecipePreviewEntityMapper.kt:19)
at de.lukasneugebauer.nextcloudcookbook.di.RecipeModule$provideRecipePreviewsStore$3.invokeSuspend(RecipeModule.kt:66)
at de.lukasneugebauer.nextcloudcookbook.di.RecipeModule$provideRecipePreviewsStore$3.invoke(Unknown Source:11)
at de.lukasneugebauer.nextcloudcookbook.di.RecipeModule$provideRecipePreviewsStore$3.invoke(Unknown Source:6)
at org.mobilenativefoundation.store.store5.impl.PersistentSourceOfTruth.write(RealSourceOfTruth.kt:31)
at org.mobilenativefoundation.store.store5.impl.SourceOfTruthWithBarrier.write(SourceOfTruthWithBarrier.kt:146)
at org.mobilenativefoundation.store.store5.impl.FetcherController$fetchers$1$4.invokeSuspend(FetcherController.kt:110)
at org.mobilenativefoundation.store.store5.impl.FetcherController$fetchers$1$4.invoke(Unknown Source:8)
at org.mobilenativefoundation.store.store5.impl.FetcherController$fetchers$1$4.invoke(Unknown Source:4)
at org.mobilenativefoundation.store.multicast5.StoreChannelManager$Actor.doDispatchValue(ChannelManager.kt:286)
at org.mobilenativefoundation.store.multicast5.StoreChannelManager$Actor.handle(ChannelManager.kt:218)
at org.mobilenativefoundation.store.multicast5.StoreChannelManager$Actor.handle(ChannelManager.kt:186)
at org.mobilenativefoundation.store.multicast5.StoreRealActor$1.invokeSuspend(StoreRealActor.kt:47)
at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:34)
at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:100)
at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:586)
at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:829)
at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:717)
at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:704)
| import java.util.concurrent.TimeUnit | ||
|
|
||
| @HiltWorker | ||
| class SyncWorker |
Owner
There was a problem hiding this comment.
This isn't working in my dev build:
Could not instantiate de.lukasneugebauer.nextcloudcookbook.core.worker.SyncWorker
java.lang.NoSuchMethodException: de.lukasneugebauer.nextcloudcookbook.core.worker.SyncWorker.<init> [class android.content.Context, class androidx.work.WorkerParameters]
at java.lang.Class.getConstructor0(Class.java:3395)
at java.lang.Class.getDeclaredConstructor(Class.java:3077)
at androidx.work.WorkerFactory.createWorkerWithDefaultFallback$fallbackToReflection(WorkerFactory.kt:87)
at androidx.work.WorkerFactory.createWorkerWithDefaultFallback(WorkerFactory.kt:96)
at androidx.work.impl.WorkerWrapper.runWorker(WorkerWrapper.kt:231)
at androidx.work.impl.WorkerWrapper.access$runWorker(WorkerWrapper.kt:67)
at androidx.work.impl.WorkerWrapper$launch$1$resolution$1.invokeSuspend(WorkerWrapper.kt:98)
at androidx.work.impl.WorkerWrapper$launch$1$resolution$1.invoke(Unknown Source:8)
at androidx.work.impl.WorkerWrapper$launch$1$resolution$1.invoke(Unknown Source:4)
at kotlinx.coroutines.intrinsics.UndispatchedKt.startUndspatched(Undispatched.kt:66)
at kotlinx.coroutines.intrinsics.UndispatchedKt.startUndispatchedOrReturn(Undispatched.kt:43)
at kotlinx.coroutines.BuildersKt__Builders_commonKt.withContext(Builders.common.kt:165)
at kotlinx.coroutines.BuildersKt.withContext(Unknown Source:1)
at androidx.work.impl.WorkerWrapper$launch$1.invokeSuspend(WorkerWrapper.kt:98)
at androidx.work.impl.WorkerWrapper$launch$1.invoke(Unknown Source:8)
at androidx.work.impl.WorkerWrapper$launch$1.invoke(Unknown Source:4)
at androidx.work.ListenableFutureKt$launchFuture$1$2.invokeSuspend(ListenableFuture.kt:42)
at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:34)
at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:100)
at androidx.work.impl.utils.SerialExecutorImpl$Task.run(SerialExecutorImpl.java:96)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:644)
at java.lang.Thread.run(Thread.java:1012)
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
2 participants
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.
Description:
This PR introduces functionality to read and access recipes even when the Nextcloud service is not available. Recipes are now cached locally, allowing users to browse and view them without requiring an active connection.
Key changes:
Impact:
Users can now rely on the cookbook app to access their saved recipes seamlessly, regardless of connectivity. This enhances usability in scenarios such as traveling, cooking in areas with poor internet, or temporary server downtime.
Summary by CodeRabbit
New Features
Chores