admin/ledgerr
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

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
Conversation 2 Commits 2 Files Changed 39
+43 -16
6 changed files with 43 additions and 16 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 547343992a - Show all commits
app/build.gradle.kts
+1 -1
View File
@@ -90,4 +90,4 @@ dependencies {
implementation(libs.vico.compose)
implementation(libs.vico.compose.m3)
implementation(libs.vico.core)
}
}
admin commented 2026-06-28 09:04:20 +00:00
Outdated
Review
Copy Link
Copy Source

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.
app/src/main/java/dev/achmad/ledgerr/data/local/dao/CategoryDao.kt
+17 -4
View File
@@ -1,8 +1,10 @@
package dev.achmad.ledgerr.data.local.dao
import androidx.room.Dao
import androidx.room.Insert
import androidx.room.OnConflictStrategy
import androidx.room.Query
import androidx.room.Upsert
import androidx.room.Update
import dev.achmad.ledgerr.data.local.entity.CategoryEntity
import kotlinx.coroutines.flow.Flow
@@ -18,16 +20,27 @@ interface CategoryDao {
@Query("SELECT * FROM categories WHERE id = :id")
suspend fun getById(id: Long): CategoryEntity?
admin commented 2026-06-28 09:04:20 +00:00
Review
Copy Link
Copy Source

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.
/**
* Returns the single category marked as the system default. The "Uncategorized"
* category is seeded as the only default on first launch. There is no
* DB-level uniqueness constraint on `isDefault = 1` (a partial unique index
* would be a v2 migration); the invariant is maintained by `SeedDefaultCategories`
* and `UpsertCategory` at the interactor layer. If multiple defaults ever
admin commented 2026-06-28 09:04:20 +00:00
Outdated
Review
Copy Link
Copy Source

@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`.
* sneak in, this returns an arbitrary one.
*/
@Query("SELECT * FROM categories WHERE isDefault = 1 LIMIT 1")
suspend fun getDefault(): CategoryEntity?
@Query("SELECT COUNT(*) FROM categories")
suspend fun count(): Int
@Upsert
suspend fun upsert(category: CategoryEntity): Long
@Insert(onConflict = OnConflictStrategy.REPLACE)
suspend fun insert(category: CategoryEntity): Long
@Upsert
@Update
suspend fun update(category: CategoryEntity)
@Insert(onConflict = OnConflictStrategy.REPLACE)
suspend fun upsertAll(categories: List<CategoryEntity>)
@Query("DELETE FROM categories WHERE id = :id")
app/src/main/java/dev/achmad/ledgerr/data/local/dao/ExpenseDao.kt
+10 -3
View File
@@ -3,9 +3,11 @@ package dev.achmad.ledgerr.data.local.dao
import androidx.room.Dao
import androidx.room.Embedded
import androidx.room.Insert
import androidx.room.OnConflictStrategy
import androidx.room.Query
import androidx.room.Relation
import androidx.room.Upsert
import androidx.room.Transaction
import androidx.room.Update
import dev.achmad.ledgerr.data.local.entity.CategoryEntity
import dev.achmad.ledgerr.data.local.entity.ExpenseEntity
import kotlinx.coroutines.flow.Flow
@@ -22,9 +24,11 @@ data class ExpenseWithCategoryRow(
@Dao
interface ExpenseDao {
admin commented 2026-06-28 09:04:20 +00:00
Review
Copy Link
Copy Source

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.
@Transaction
@Query("SELECT * FROM expenses ORDER BY date DESC, createdAt DESC")
fun subscribeAll(): Flow<List<ExpenseWithCategoryRow>>
@Transaction
@Query(
"SELECT * FROM expenses " +
admin commented 2026-06-28 09:04:20 +00:00
Review
Copy Link
Copy Source

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

Same as above — missing `@Transaction` on the second `@Relation` flow. Add it.
"WHERE date BETWEEN :startDay AND :endDay " +
@@ -50,8 +54,11 @@ interface ExpenseDao {
)
admin commented 2026-06-28 09:04:20 +00:00
Review
Copy Link
Copy Source

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.
suspend fun getByDateRange(startDay: Long, endDay: Long): List<ExpenseEntity>
@Upsert
suspend fun upsert(expense: ExpenseEntity): Long
@Insert(onConflict = OnConflictStrategy.REPLACE)
suspend fun insert(expense: ExpenseEntity): Long
@Update
suspend fun update(expense: ExpenseEntity)
@Insert
suspend fun insertAll(expenses: List<ExpenseEntity>)
app/src/main/java/dev/achmad/ledgerr/data/local/dao/RecurringExpenseDao.kt
+10 -3
View File
@@ -2,9 +2,12 @@ package dev.achmad.ledgerr.data.local.dao
import androidx.room.Dao
import androidx.room.Embedded
import androidx.room.Insert
import androidx.room.OnConflictStrategy
import androidx.room.Query
import androidx.room.Relation
import androidx.room.Upsert
import androidx.room.Transaction
import androidx.room.Update
import dev.achmad.ledgerr.data.local.entity.CategoryEntity
import dev.achmad.ledgerr.data.local.entity.RecurringExpenseEntity
import kotlinx.coroutines.flow.Flow
@@ -21,6 +24,7 @@ data class RecurringExpenseWithCategoryRow(
@Dao
interface RecurringExpenseDao {
admin commented 2026-06-28 09:04:20 +00:00
Review
Copy Link
Copy Source

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

Third KSP warning. Missing `@Transaction` on this `@Relation` flow. Add it.
@Transaction
@Query("SELECT * FROM recurring_expenses ORDER BY nextDueDate ASC")
fun subscribeAll(): Flow<List<RecurringExpenseWithCategoryRow>>
@@ -30,8 +34,11 @@ interface RecurringExpenseDao {
@Query("SELECT * FROM recurring_expenses WHERE isActive = 1 AND nextDueDate <= :today")
admin commented 2026-06-28 09:04:20 +00:00
Review
Copy Link
Copy Source

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.
suspend fun getDue(today: Long): List<RecurringExpenseEntity>
@Upsert
suspend fun upsert(recurring: RecurringExpenseEntity): Long
@Insert(onConflict = OnConflictStrategy.REPLACE)
suspend fun insert(recurring: RecurringExpenseEntity): Long
@Update
suspend fun update(recurring: RecurringExpenseEntity)
@Query("DELETE FROM recurring_expenses WHERE id = :id")
suspend fun deleteById(id: Long)
app/src/main/java/dev/achmad/ledgerr/domain/category/interactor/UpsertCategory.kt
+3 -2
View File
@@ -9,7 +9,7 @@ class UpsertCategory(
) {
suspend fun await(category: Category): Long {
if (category.id == 0L) {
return dao.upsert(category.toEntity())
return dao.insert(category.toEntity())
}
val existing = dao.getById(category.id)
val resolved = if (existing != null && category.isDefault && !existing.isDefault) {
@@ -17,6 +17,7 @@ class UpsertCategory(
} else {
category
}
return dao.upsert(resolved.toEntity())
dao.update(resolved.toEntity())
return category.id
}
}
app/src/main/java/dev/achmad/ledgerr/ui/base/MainApplication.kt
+2 -3
View File
@@ -7,9 +7,8 @@ import dev.achmad.ledgerr.di.domainModule
import dev.achmad.ledgerr.di.preferenceModule
import dev.achmad.ledgerr.di.util.inject
import dev.achmad.ledgerr.domain.category.interactor.SeedDefaultCategories
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.launch
import kotlinx.coroutines.runBlocking
import org.koin.android.ext.koin.androidContext
import org.koin.android.ext.koin.androidLogger
import org.koin.core.context.startKoin
@@ -23,7 +22,7 @@ class MainApplication : Application() {
androidContext(this@MainApplication)
modules(coreModule, dataModule, domainModule, preferenceModule)
}
CoroutineScope(Dispatchers.IO).launch {
runBlocking(Dispatchers.IO) {
inject<SeedDefaultCategories>().await()
admin commented 2026-06-28 09:04:20 +00:00
Review
Copy Link
Copy Source

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.
}
}