Implement category feature and wire DI foundation (#1) #8
Reference in New Issue
Block a user
Delete Branch "feat/1-category-foundation"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Implements issue #1 — the foundation slice. Category feature is end-to-end functional and DI is fully wired.
What's in this PR
Dependencies (Phase 0):
Room 2.7.1,PDFBox-Android 2.0.27.0,Vico 2.0.0added togradle/libs.versions.tomlandapp/build.gradle.ktscompileSdkbumped 36 → 37 (Koin transitive deps required it)Data layer:
LocalDateConverter(epoch day)CategoryEntity,ExpenseEntity,RecurringExpenseEntity) with proper FKs and indices@Relationrows for the JOIN pathsAppDatabasev1Domain:
expense,category,recurring,bankstatement,preferenceReassignExpenseCategory(full expense interactors in #2)DI:
CoreModule(SharedPreferences + AndroidPreferenceStore)DataModule(AppDatabase + 3 DAOs)PreferenceModule(AppPreference, ExpensePreference)DomainModule(4 category factories + ReassignExpenseCategory)App bootstrap:
MainApplicationregistered in manifest, starts Koin, triggersSeedDefaultCategoriesonDispatchers.IOString resources:
confirm,cancel,ok,general_selected,general_not_selected,disabled)Verification
./gradlew assembleDebugsucceedsMainApplicationOut of scope (belongs to other issues)
ReassignExpenseCategoryis here, as a dependencyNotes
@Transactionannotation on@Relationqueries — non-blocking, can be addressed when the related DAO methods are exercised by #2/#3ui/components/AppBar.ktforrememberPlainTooltipPositionProvider— pre-existing in copied code, not touchedOther→Uncategorizedand the newdata/AddEditRecurringScreendoc items were already committed tomainin51c5474Closes #1
PR review — found a handful of issues to address
Overall the foundation looks solid and the architecture rules from
AGENTS.mdare followed correctly (factories for interactors, no repository layer, no ScreenModel in Koin, SAF in mind, no manifest permissions, etc.). Build green, DI compiles, manifest wired. Good work.A few things to fix before merge:
@Upsertdoes not reliably return the row ID — this is a real bug that will surface in #2/#5 when a caller uses the returnedLongto navigate to the just-created expense/category. See inline comments on the three DAOs and onUpsertCategory.@Transactionon the 3@Relationqueries — these are the exact KSP warnings the PR body mentions. Trivial to fix now.CoroutineScopeinMainApplication— fire-and-forget seed can race with the first screen that needs the default category;GetCategories.awaitDefault()will throwerror(...)and crash.exportSchema = false— fine for v1, but should be flipped totruewith a schema location before any future migration lands.CategoryDao.getDefault()— no DB-level enforcement of the "only one default category" invariant. A partial unique index would be cheap insurance.app/build.gradle.ktsis missing a trailing newline.Also: a small note that
ReassignExpenseCategorylives indomain/expense/interactor/but is only consumed byDeleteCategory(a category interactor). The cross-package dependency is fine, just flagging that the issue's "out of scope" list says expense interactors belong to #2, so this minimal interactor is essentially scope-creep for the foundation slice — minor, but worth a comment in the next PR.@@ -87,1 +90,4 @@implementation(libs.vico.compose)implementation(libs.vico.compose.m3)implementation(libs.vico.core)}File is missing a trailing newline (
\ No newline at end of filein the diff). Most formatters and POSIX text tools assume one. Add a blank line at EOF.@@ -0,0 +18,4 @@RecurringExpenseEntity::class,],version = 1,exportSchema = false,exportSchema = falseis OK for v1, but you should flip it totrueand addroom.schemaLocationtoapp/build.gradle.ktsbefore any future migration. Without exported schemas, migrations can't be auto-tested and you lose the safety netandroidx.room:room-testingprovides. At minimum, open a follow-up issue so it doesn't get forgotten when #2 lands a v2 migration.@@ -0,0 +19,4 @@suspend fun getById(id: Long): CategoryEntity?@Query("SELECT * FROM categories WHERE isDefault = 1 LIMIT 1")suspend fun getDefault(): CategoryEntity?No DB-level enforcement that only one row has
isDefault = 1.SeedDefaultCategoriesenforces it at app start, andUpsertCategoryblocks promoting an existing non-default to default, but if anything ever violates the invariant (manual DB edit, future bug, two seeds racing on first launch)getDefault()will return an arbitrary one.A partial unique index would make this an invariant of the schema:
This is
androidx.room.Index(value = ["isDefault"], unique = true)won't quite work becauseisDefaultis a non-unique column with a0value for most rows. Use the@Indexannotation plus aCREATE INDEXquery in a Room migration, or just add a check via a@Queryintegrity check. Worth at least leaving a comment ongetDefault()documenting the assumption.@@ -0,0 +25,4 @@suspend fun count(): Int@Upsertsuspend fun upsert(category: CategoryEntity): Long@Upsertdoes not return a meaningful row ID on the update path. The generatedupsertruns asINSERT OR IGNOREthenUPDATE; for an existing row the second statement returns the SQLite result code (typically-1), not the entity's ID.UpsertCategory.await()propagates thisLongto its callers, so anyone using the return value to navigate to an updated category will get-1instead ofcategory.id.Two options:
@Insert(onConflict = OnConflictStrategy.REPLACE)(which does return the row ID for both insert and update) and a separate@Update, then haveUpsertCategorycall the right one.UpsertCategory.await(), always returncategory.idafter thedao.upsert(...)call for theid != 0Lbranch and only use thedao.upsert(...)return value for theid == 0Lbranch.Also:
ReassignExpenseCategory(the caller ofreassignCategory) doesn't care about the return, so it's unaffected — this is only a concern forupsert.@@ -0,0 +23,4 @@interface ExpenseDao {@Query("SELECT * FROM expenses ORDER BY date DESC, createdAt DESC")fun subscribeAll(): Flow<List<ExpenseWithCategoryRow>>One of the 3 KSP warnings the PR body mentions.
@Relationqueries need to run inside a transaction; forFlowreturns Room will technically auto-wrap, but KSP still warns unless you annotate the DAO method with@Transaction. Add@Transactionto silence the warning and make the intent explicit.@@ -0,0 +30,4 @@"WHERE date BETWEEN :startDay AND :endDay " +"ORDER BY date DESC, createdAt DESC")fun subscribeByDateRange(startDay: Long, endDay: Long): Flow<List<ExpenseWithCategoryRow>>Same as above — missing
@Transactionon the second@Relationflow. Add it.@@ -0,0 +51,4 @@suspend fun getByDateRange(startDay: Long, endDay: Long): List<ExpenseEntity>@Upsertsuspend fun upsert(expense: ExpenseEntity): LongSame
@Upsertreturn-value issue asCategoryDao.upsert.ReassignExpenseCategoryis fine because it doesn't read the return, butGetExpenses/UpsertExpense(in #2) will want the new row ID — flagging this now so the fix is in one place.@@ -0,0 +22,4 @@interface RecurringExpenseDao {@Query("SELECT * FROM recurring_expenses ORDER BY nextDueDate ASC")fun subscribeAll(): Flow<List<RecurringExpenseWithCategoryRow>>Third KSP warning. Missing
@Transactionon this@Relationflow. Add it.@@ -0,0 +31,4 @@suspend fun getDue(today: Long): List<RecurringExpenseEntity>@Upsertsuspend fun upsert(recurring: RecurringExpenseEntity): LongSame
@Upsertreturn-value issue asCategoryDao.upsert. Flagging for consistency before #3 lands.@@ -0,0 +23,4 @@androidContext(this@MainApplication)modules(coreModule, dataModule, domainModule, preferenceModule)}CoroutineScope(Dispatchers.IO).launch {This
CoroutineScope(Dispatchers.IO)is fire-and-forget with no lifecycle binding. The seed itself is fast (8 row inserts), but it is asynchronous relative to the first frame, so a fast tap on the home screen can land onHomeScreencallingGetCategories.awaitDefault()(directly or via the dashboard) before the seed completes.GetCategories.awaitDefault()throwserror("Default category not found — ...")in that case, crashing the app.Options:
runBlocking(Dispatchers.IO) { inject<SeedDefaultCategories>().await() }— fine for a first-launch one-shot, and the 8 inserts are sub-millisecond.categoriesReady: StateFlow<Boolean>from a startup interactor and have screens observe it.Jobsomewhere and cancel it inonTerminate()for testability (cosmetic).The simplest correct fix is
runBlockinghere.All review comments addressed in commit
5473439(pushed tofeat/1-category-foundation).1.
@Upsertreturn-value bug — fixed by splitting into@Insert(OnConflictStrategy.REPLACE)+@Updatein all 3 DAOs. Interactors now call the right one:UpsertCategory.await()callsdao.insert(...)forid == 0L(returns the new row id) anddao.update(...)then returnscategory.idforid != 0L.ReassignExpenseCategoryis unaffected.2. Missing
@Transaction— added toExpenseDao.subscribeAll,ExpenseDao.subscribeByDateRange, andRecurringExpenseDao.subscribeAll. KSP warnings are gone.3.
MainApplicationrace — switched torunBlocking(Dispatchers.IO) { inject<SeedDefaultCategories>().await() }. 8 row inserts are sub-millisecond so the onCreate cost is negligible, andGetCategories.awaitDefault()can no longer race the seed.4.
exportSchema = false— filed as follow-up issue #9. Doesn't block this PR (no v2 migration exists yet) but is captured so it ships before #2 lands any schema change.5.
CategoryDao.getDefault()invariant — added a KDoc on the method documenting that the "only one isDefault = 1" invariant is maintained by the interactors (partial unique index would be a v2 migration).6. Trailing newline on
app/build.gradle.kts— fixed.On the cross-package
ReassignExpenseCategorynote: agreed it's a soft scope-creep from #1's "out of scope" list. The reason it landed here is theDeleteCategoryconstructor needs it; the alternative wasfactory { DeleteCategory(...) }referencing a non-existent type. I'll leave it where it is (underdomain/expense/interactor/) and #2 will naturally add the rest of the expense interactors next to it. If you'd rather see it indomain/category/interactor/, easy move in a follow-up — just flag.Re-running
./gradlew assembleDebugafter the changes: BUILD SUCCESSFUL, 0 KSP warnings (other than the pre-existing-XXLanguage:+ContextParametersnotice).Re-requesting review.