Implement category feature and wire DI foundation (#1) #8

Merged
admin merged 2 commits from feat/1-category-foundation into main 2026-06-28 09:17:22 +00:00
Owner

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.0 added to gradle/libs.versions.toml and app/build.gradle.kts
  • compileSdk bumped 36 → 37 (Koin transitive deps required it)

Data layer:

  • LocalDateConverter (epoch day)
  • 3 entities (CategoryEntity, ExpenseEntity, RecurringExpenseEntity) with proper FKs and indices
  • 3 DAOs with @Relation rows for the JOIN paths
  • 3 mapper pairs
  • AppDatabase v1

Domain:

  • All models for expense, category, recurring, bankstatement, preference
  • 4 implemented category interactors
  • Minimal ReassignExpenseCategory (full expense interactors in #2)

DI:

  • CoreModule (SharedPreferences + AndroidPreferenceStore)
  • DataModule (AppDatabase + 3 DAOs)
  • PreferenceModule (AppPreference, ExpensePreference)
  • DomainModule (4 category factories + ReassignExpenseCategory)

App bootstrap:

  • MainApplication registered in manifest, starts Koin, triggers SeedDefaultCategories on Dispatchers.IO

String resources:

  • Added 6 missing strings used by pre-existing copied UI components (confirm, cancel, ok, general_selected, general_not_selected, disabled)

Verification

  • ./gradlew assembleDebug succeeds
  • All category interactor TODOs replaced with real code
  • Manifest references MainApplication
  • Koin wiring compiles for all 4 modules

Out of scope (belongs to other issues)

  • Expense interactors (#2) — only ReassignExpenseCategory is here, as a dependency
  • Recurring interactors (#3)
  • Bank statement / export / ClearAllData (#4)
  • All UI screens (#5, #6, #7)

Notes

  • 3 KSP warnings about @Transaction annotation on @Relation queries — non-blocking, can be addressed when the related DAO methods are exercised by #2/#3
  • 4 deprecation warnings in ui/components/AppBar.kt for rememberPlainTooltipPositionProvider — pre-existing in copied code, not touched
  • The rename OtherUncategorized and the new data / AddEditRecurringScreen doc items were already committed to main in 51c5474

Closes #1

## 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.0` added to `gradle/libs.versions.toml` and `app/build.gradle.kts` - `compileSdk` bumped 36 → 37 (Koin transitive deps required it) **Data layer:** - `LocalDateConverter` (epoch day) - 3 entities (`CategoryEntity`, `ExpenseEntity`, `RecurringExpenseEntity`) with proper FKs and indices - 3 DAOs with `@Relation` rows for the JOIN paths - 3 mapper pairs - `AppDatabase` v1 **Domain:** - All models for `expense`, `category`, `recurring`, `bankstatement`, `preference` - 4 implemented category interactors - Minimal `ReassignExpenseCategory` (full expense interactors in #2) **DI:** - `CoreModule` (SharedPreferences + AndroidPreferenceStore) - `DataModule` (AppDatabase + 3 DAOs) - `PreferenceModule` (AppPreference, ExpensePreference) - `DomainModule` (4 category factories + ReassignExpenseCategory) **App bootstrap:** - `MainApplication` registered in manifest, starts Koin, triggers `SeedDefaultCategories` on `Dispatchers.IO` **String resources:** - Added 6 missing strings used by pre-existing copied UI components (`confirm`, `cancel`, `ok`, `general_selected`, `general_not_selected`, `disabled`) ## Verification - [x] `./gradlew assembleDebug` succeeds - [x] All category interactor TODOs replaced with real code - [x] Manifest references `MainApplication` - [x] Koin wiring compiles for all 4 modules ## Out of scope (belongs to other issues) - Expense interactors (#2) — only `ReassignExpenseCategory` is here, as a dependency - Recurring interactors (#3) - Bank statement / export / ClearAllData (#4) - All UI screens (#5, #6, #7) ## Notes - 3 KSP warnings about `@Transaction` annotation on `@Relation` queries — non-blocking, can be addressed when the related DAO methods are exercised by #2/#3 - 4 deprecation warnings in `ui/components/AppBar.kt` for `rememberPlainTooltipPositionProvider` — pre-existing in copied code, not touched - The rename `Other` → `Uncategorized` and the new `data` / `AddEditRecurringScreen` doc items were already committed to `main` in `51c5474` Closes #1
admin added 1 commit 2026-06-28 08:54:11 +00:00
- Add Room 2.7.1, PDFBox-Android 2.0.27.0, Vico 2.0.0 dependencies
- Bump compileSdk 36 -> 37 (transitive deps require it)
- Add LocalDateConverter + 3 entities + 3 DAOs with @Relation rows + mappers + AppDatabase v1
- Add all domain models (expense, category, recurring, bankstatement, preference)
- Implement 4 category interactors: GetCategories, UpsertCategory, DeleteCategory, SeedDefaultCategories
- Add minimal ReassignExpenseCategory (full expense interactors come in #2)
- Wire CoreModule (SharedPreferences + AndroidPreferenceStore), DataModule (Room + 3 DAOs), PreferenceModule, DomainModule (category factories)
- Create MainApplication with Koin start and SeedDefaultCategories trigger
- Register MainApplication in AndroidManifest
- Add missing string resources for pre-existing copied UI components (confirm, cancel, ok, general_selected, general_not_selected, disabled)
admin reviewed 2026-06-28 09:04:20 +00:00
admin left a comment
Author
Owner

PR review — found a handful of issues to address

Overall the foundation looks solid and the architecture rules from AGENTS.md are 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:

  1. @Upsert does not reliably return the row ID — this is a real bug that will surface in #2/#5 when a caller uses the returned Long to navigate to the just-created expense/category. See inline comments on the three DAOs and on UpsertCategory.
  2. Missing @Transaction on the 3 @Relation queries — these are the exact KSP warnings the PR body mentions. Trivial to fix now.
  3. Orphaned CoroutineScope in MainApplication — fire-and-forget seed can race with the first screen that needs the default category; GetCategories.awaitDefault() will throw error(...) and crash.
  4. exportSchema = false — fine for v1, but should be flipped to true with a schema location before any future migration lands.
  5. CategoryDao.getDefault() — no DB-level enforcement of the "only one default category" invariant. A partial unique index would be cheap insurance.
  6. app/build.gradle.kts is missing a trailing newline.

Also: a small note that ReassignExpenseCategory lives in domain/expense/interactor/ but is only consumed by DeleteCategory (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.

## PR review — found a handful of issues to address Overall the foundation looks solid and the architecture rules from `AGENTS.md` are 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: 1. **`@Upsert` does not reliably return the row ID** — this is a real bug that will surface in #2/#5 when a caller uses the returned `Long` to navigate to the just-created expense/category. See inline comments on the three DAOs and on `UpsertCategory`. 2. **Missing `@Transaction` on the 3 `@Relation` queries** — these are the exact KSP warnings the PR body mentions. Trivial to fix now. 3. **Orphaned `CoroutineScope` in `MainApplication`** — fire-and-forget seed can race with the first screen that needs the default category; `GetCategories.awaitDefault()` will throw `error(...)` and crash. 4. **`exportSchema = false`** — fine for v1, but should be flipped to `true` with a schema location before any future migration lands. 5. **`CategoryDao.getDefault()`** — no DB-level enforcement of the "only one default category" invariant. A partial unique index would be cheap insurance. 6. **`app/build.gradle.kts` is missing a trailing newline.** Also: a small note that `ReassignExpenseCategory` lives in `domain/expense/interactor/` but is only consumed by `DeleteCategory` (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)
}
Author
Owner

File is missing a trailing newline (\ No newline at end of file in the diff). Most formatters and POSIX text tools assume one. Add a blank line at EOF.

File is missing a trailing newline (`\ No newline at end of file` in 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,
Author
Owner

exportSchema = false is OK for v1, but you should flip it to true and add room.schemaLocation to app/build.gradle.kts before any future migration. Without exported schemas, migrations can't be auto-tested and you lose the safety net androidx.room:room-testing provides. At minimum, open a follow-up issue so it doesn't get forgotten when #2 lands a v2 migration.

`exportSchema = false` is OK for v1, but you should flip it to `true` and add `room.schemaLocation` to `app/build.gradle.kts` before any future migration. Without exported schemas, migrations can't be auto-tested and you lose the safety net `androidx.room:room-testing` provides. 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?
Author
Owner

No DB-level enforcement that only one row has isDefault = 1. SeedDefaultCategories enforces it at app start, and UpsertCategory blocks 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:

CREATE UNIQUE INDEX idx_categories_only_one_default
    ON categories(isDefault) WHERE isDefault = 1

This is androidx.room.Index(value = ["isDefault"], unique = true) won't quite work because isDefault is a non-unique column with a 0 value for most rows. Use the @Index annotation plus a CREATE INDEX query in a Room migration, or just add a check via a @Query integrity check. Worth at least leaving a comment on getDefault() documenting the assumption.

No DB-level enforcement that only one row has `isDefault = 1`. `SeedDefaultCategories` enforces it at app start, and `UpsertCategory` blocks 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: ```sql CREATE UNIQUE INDEX idx_categories_only_one_default ON categories(isDefault) WHERE isDefault = 1 ``` This is `androidx.room.Index(value = ["isDefault"], unique = true)` won't quite work because `isDefault` is a non-unique column with a `0` value for most rows. Use the `@Index` annotation plus a `CREATE INDEX` query in a Room migration, or just add a check via a `@Query` integrity check. Worth at least leaving a comment on `getDefault()` documenting the assumption.
@@ -0,0 +25,4 @@
suspend fun count(): Int
@Upsert
suspend fun upsert(category: CategoryEntity): Long
Author
Owner

@Upsert does not return a meaningful row ID on the update path. The generated upsert runs as INSERT OR IGNORE then UPDATE; for an existing row the second statement returns the SQLite result code (typically -1), not the entity's ID. UpsertCategory.await() propagates this Long to its callers, so anyone using the return value to navigate to an updated category will get -1 instead of category.id.

Two options:

  • Split into @Insert(onConflict = OnConflictStrategy.REPLACE) (which does return the row ID for both insert and update) and a separate @Update, then have UpsertCategory call the right one.
  • Or, in UpsertCategory.await(), always return category.id after the dao.upsert(...) call for the id != 0L branch and only use the dao.upsert(...) return value for the id == 0L branch.

Also: ReassignExpenseCategory (the caller of reassignCategory) doesn't care about the return, so it's unaffected — this is only a concern for upsert.

`@Upsert` does not return a meaningful row ID on the update path. The generated `upsert` runs as `INSERT OR IGNORE` then `UPDATE`; for an existing row the second statement returns the SQLite result code (typically `-1`), not the entity's ID. `UpsertCategory.await()` propagates this `Long` to its callers, so anyone using the return value to navigate to an updated category will get `-1` instead of `category.id`. Two options: - Split into `@Insert(onConflict = OnConflictStrategy.REPLACE)` (which does return the row ID for both insert and update) and a separate `@Update`, then have `UpsertCategory` call the right one. - Or, in `UpsertCategory.await()`, always return `category.id` after the `dao.upsert(...)` call for the `id != 0L` branch and only use the `dao.upsert(...)` return value for the `id == 0L` branch. Also: `ReassignExpenseCategory` (the caller of `reassignCategory`) doesn't care about the return, so it's unaffected — this is only a concern for `upsert`.
@@ -0,0 +23,4 @@
interface ExpenseDao {
@Query("SELECT * FROM expenses ORDER BY date DESC, createdAt DESC")
fun subscribeAll(): Flow<List<ExpenseWithCategoryRow>>
Author
Owner

One of the 3 KSP warnings the PR body mentions. @Relation queries need to run inside a transaction; for Flow returns Room will technically auto-wrap, but KSP still warns unless you annotate the DAO method with @Transaction. Add @Transaction to silence the warning and make the intent explicit.

One of the 3 KSP warnings the PR body mentions. `@Relation` queries need to run inside a transaction; for `Flow` returns Room will technically auto-wrap, but KSP still warns unless you annotate the DAO method with `@Transaction`. Add `@Transaction` to 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>>
Author
Owner

Same as above — missing @Transaction on the second @Relation flow. Add it.

Same as above — missing `@Transaction` on the second `@Relation` flow. Add it.
@@ -0,0 +51,4 @@
suspend fun getByDateRange(startDay: Long, endDay: Long): List<ExpenseEntity>
@Upsert
suspend fun upsert(expense: ExpenseEntity): Long
Author
Owner

Same @Upsert return-value issue as CategoryDao.upsert. ReassignExpenseCategory is fine because it doesn't read the return, but GetExpenses/UpsertExpense (in #2) will want the new row ID — flagging this now so the fix is in one place.

Same `@Upsert` return-value issue as `CategoryDao.upsert`. `ReassignExpenseCategory` is fine because it doesn't read the return, but `GetExpenses`/`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>>
Author
Owner

Third KSP warning. Missing @Transaction on this @Relation flow. Add it.

Third KSP warning. Missing `@Transaction` on this `@Relation` flow. Add it.
@@ -0,0 +31,4 @@
suspend fun getDue(today: Long): List<RecurringExpenseEntity>
@Upsert
suspend fun upsert(recurring: RecurringExpenseEntity): Long
Author
Owner

Same @Upsert return-value issue as CategoryDao.upsert. Flagging for consistency before #3 lands.

Same `@Upsert` return-value issue as `CategoryDao.upsert`. Flagging for consistency before #3 lands.
@@ -0,0 +23,4 @@
androidContext(this@MainApplication)
modules(coreModule, dataModule, domainModule, preferenceModule)
}
CoroutineScope(Dispatchers.IO).launch {
Author
Owner

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 on HomeScreen calling GetCategories.awaitDefault() (directly or via the dashboard) before the seed completes. GetCategories.awaitDefault() throws error("Default category not found — ...") in that case, crashing the app.

Options:

  • Use runBlocking(Dispatchers.IO) { inject<SeedDefaultCategories>().await() } — fine for a first-launch one-shot, and the 8 inserts are sub-millisecond.
  • Or expose a categoriesReady: StateFlow<Boolean> from a startup interactor and have screens observe it.
  • Also: store the Job somewhere and cancel it in onTerminate() for testability (cosmetic).

The simplest correct fix is runBlocking here.

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 on `HomeScreen` calling `GetCategories.awaitDefault()` (directly or via the dashboard) before the seed completes. `GetCategories.awaitDefault()` throws `error("Default category not found — ...")` in that case, crashing the app. Options: - Use `runBlocking(Dispatchers.IO) { inject<SeedDefaultCategories>().await() }` — fine for a first-launch one-shot, and the 8 inserts are sub-millisecond. - Or expose a `categoriesReady: StateFlow<Boolean>` from a startup interactor and have screens observe it. - Also: store the `Job` somewhere and cancel it in `onTerminate()` for testability (cosmetic). The simplest correct fix is `runBlocking` here.
admin added 1 commit 2026-06-28 09:09:20 +00:00
Per review on PR #8 (#8):

- Split @Upsert into @Insert(OnConflictStrategy.REPLACE) + @Update in all 3 DAOs.
  @Upsert returns -1 on the update path, so callers wanting the row ID would
  get a junk value. Interactors now call insert vs update based on id == 0.
  UpsertCategory returns category.id explicitly for the id != 0 branch.
- Add @Transaction to the 3 @Relation queries (ExpenseDao.subscribeAll,
  ExpenseDao.subscribeByDateRange, RecurringExpenseDao.subscribeAll). This
  silences the KSP warnings the PR body mentioned and makes the intent
  explicit.
- Switch MainApplication seeding from a fire-and-forget CoroutineScope to
  runBlocking(Dispatchers.IO). A fast first-tap on HomeScreen could otherwise
  call GetCategories.awaitDefault() before seeding completed and crash.
- Add documenting comment on CategoryDao.getDefault() noting that the
  'only one isDefault = 1' invariant is maintained at the interactor layer
  (partial unique index would be a v2 migration).
- Add trailing newline to app/build.gradle.kts.
- File follow-up issue #9 for flipping exportSchema to true before any v2
  migration lands.
admin reviewed 2026-06-28 09:09:28 +00:00
admin left a comment
Author
Owner

All review comments addressed in commit 5473439 (pushed to feat/1-category-foundation).

1. @Upsert return-value bug — fixed by splitting into @Insert(OnConflictStrategy.REPLACE) + @Update in all 3 DAOs. Interactors now call the right one:

  • UpsertCategory.await() calls dao.insert(...) for id == 0L (returns the new row id) and dao.update(...) then returns category.id for id != 0L. ReassignExpenseCategory is unaffected.

2. Missing @Transaction — added to ExpenseDao.subscribeAll, ExpenseDao.subscribeByDateRange, and RecurringExpenseDao.subscribeAll. KSP warnings are gone.

3. MainApplication race — switched to runBlocking(Dispatchers.IO) { inject<SeedDefaultCategories>().await() }. 8 row inserts are sub-millisecond so the onCreate cost is negligible, and GetCategories.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 ReassignExpenseCategory note: agreed it's a soft scope-creep from #1's "out of scope" list. The reason it landed here is the DeleteCategory constructor needs it; the alternative was factory { DeleteCategory(...) } referencing a non-existent type. I'll leave it where it is (under domain/expense/interactor/) and #2 will naturally add the rest of the expense interactors next to it. If you'd rather see it in domain/category/interactor/, easy move in a follow-up — just flag.

Re-running ./gradlew assembleDebug after the changes: BUILD SUCCESSFUL, 0 KSP warnings (other than the pre-existing -XXLanguage:+ContextParameters notice).

Re-requesting review.

All review comments addressed in commit `5473439` (pushed to `feat/1-category-foundation`). **1. `@Upsert` return-value bug** — fixed by splitting into `@Insert(OnConflictStrategy.REPLACE)` + `@Update` in all 3 DAOs. Interactors now call the right one: - `UpsertCategory.await()` calls `dao.insert(...)` for `id == 0L` (returns the new row id) and `dao.update(...)` then returns `category.id` for `id != 0L`. `ReassignExpenseCategory` is unaffected. **2. Missing `@Transaction`** — added to `ExpenseDao.subscribeAll`, `ExpenseDao.subscribeByDateRange`, and `RecurringExpenseDao.subscribeAll`. KSP warnings are gone. **3. `MainApplication` race** — switched to `runBlocking(Dispatchers.IO) { inject<SeedDefaultCategories>().await() }`. 8 row inserts are sub-millisecond so the onCreate cost is negligible, and `GetCategories.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 `ReassignExpenseCategory` note: agreed it's a soft scope-creep from #1's "out of scope" list. The reason it landed here is the `DeleteCategory` constructor needs it; the alternative was `factory { DeleteCategory(...) }` referencing a non-existent type. I'll leave it where it is (under `domain/expense/interactor/`) and #2 will naturally add the rest of the expense interactors next to it. If you'd rather see it in `domain/category/interactor/`, easy move in a follow-up — just flag. Re-running `./gradlew assembleDebug` after the changes: **BUILD SUCCESSFUL**, 0 KSP warnings (other than the pre-existing `-XXLanguage:+ContextParameters` notice). Re-requesting review.
admin merged commit c22c33ab7e into main 2026-06-28 09:17:22 +00:00
admin deleted branch feat/1-category-foundation 2026-06-28 09:17:22 +00:00
Sign in to join this conversation.
No Reviewers
No Label
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: admin/ledgerr#8