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

Implement CategoryScreen, ImportBankStatementScreen, SettingsScreen, and ExportAction helper (#7) #17

Merged
admin merged 5 commits from feat/7-implement-screens-export into main 2026-06-28 12:06:48 +00:00
Conversation 1 Commits 5 Files Changed 8
+1410
admin commented 2026-06-28 11:16:13 +00:00
Owner
Copy Link
Copy Source

Summary

  • Add ui/components/ExportAction.kt — shared date-range dialog + SAF CreateDocument launcher, with ExportAction (icon button) and ExportPreferenceAction (text row) variants backed by the same controller
  • Add CategoryScreen (add/edit/delete with color picker; default categories are locked) + CategoryScreenModel
  • Add ImportBankStatementScreen (1 Voyager Screen + 2 composables) + ImportBankStatementScreenModel with sealed ImportState; NotImplementedError from bank stubs surfaces as a snackbar and resets the picker
  • Add SettingsScreen (theme + export + clear-data) using PreferenceScreen, and SettingsScreenModel that chains ClearAllData + re-seeds SeedDefaultCategories
  • Fix thread context: ScreenModels now do DB/IO on Dispatchers.IO instead of Dispatchers.Main.immediate (flow sources use .flowOn(Dispatchers.IO); suspend calls use withContext(Dispatchers.IO))

Test plan

  • ./gradlew :app:assembleDebug succeeds
  • ./gradlew :app:testDebugUnitTest succeeds
  • Manual: open Categories — add, edit (name + color), delete (default protected), confirm Uncategorized reaps orphan expenses
  • Manual: Import Bank Statement — pick bank, select PDF, see processing overlay, review rows, toggle selection, change category, edit amount/date/description, confirm import
  • Manual: Settings — change theme, export CSV (date range dialog → SAF picker → file written with UTF-8 BOM), clear all data (confirmation dialog → default categories re-seeded)

Closes #7

## Summary - Add `ui/components/ExportAction.kt` — shared date-range dialog + SAF `CreateDocument` launcher, with `ExportAction` (icon button) and `ExportPreferenceAction` (text row) variants backed by the same controller - Add `CategoryScreen` (add/edit/delete with color picker; default categories are locked) + `CategoryScreenModel` - Add `ImportBankStatementScreen` (1 Voyager Screen + 2 composables) + `ImportBankStatementScreenModel` with sealed `ImportState`; `NotImplementedError` from bank stubs surfaces as a snackbar and resets the picker - Add `SettingsScreen` (theme + export + clear-data) using `PreferenceScreen`, and `SettingsScreenModel` that chains `ClearAllData` + re-seeds `SeedDefaultCategories` - Fix thread context: ScreenModels now do DB/IO on `Dispatchers.IO` instead of `Dispatchers.Main.immediate` (flow sources use `.flowOn(Dispatchers.IO)`; suspend calls use `withContext(Dispatchers.IO)`) ## Test plan - [x] `./gradlew :app:assembleDebug` succeeds - [x] `./gradlew :app:testDebugUnitTest` succeeds - [ ] Manual: open Categories — add, edit (name + color), delete (default protected), confirm Uncategorized reaps orphan expenses - [ ] Manual: Import Bank Statement — pick bank, select PDF, see processing overlay, review rows, toggle selection, change category, edit amount/date/description, confirm import - [ ] Manual: Settings — change theme, export CSV (date range dialog → SAF picker → file written with UTF-8 BOM), clear all data (confirmation dialog → default categories re-seeded) Closes #7
admin added 2 commits 2026-06-28 11:16:14 +00:00
feat(#7): implement CategoryScreen, ImportBankStatementScreen, SettingsScreen, and ExportAction helper f6860544e4
fix(#7): move DB/IO off main thread in ScreenModels 7782df8b36 
screenModelScope is backed by PlatformMainDispatcher (Main.immediate),
so direct interactor calls run DB queries and file I/O on the UI
thread. Switch reactive flows with .flowOn(Dispatchers.IO) and wrap
suspend calls in withContext(Dispatchers.IO).
admin reviewed 2026-06-28 11:25:01 +00:00
admin left a comment
Author
Owner
Copy Link
Copy Source

Summary

Verdict: has one blocking concern, plus non-blocking notes.

The PR is well-structured and follows the spec from issue #7: the four pieces (ExportAction, CategoryScreen, ImportBankStatementScreen, SettingsScreen) are all in place, the Voyager + Koin ScreenModel pattern is used correctly, no ScreenModel is registered in DI, no repository layer is introduced, no manifest permissions are added, and no chart library is used. The LazyVerticalGrid + LazyColumn co-existence in CategoryScreen is handled with an aliased import, which is the right call. ./gradlew assembleDebug and :app:testDebugUnitTest are reported green. All cross-references (Category.DEFAULT_COLOR_*, ui.util.onClickInput, Preference.PreferenceItem.CustomPreference / AlertDialogPreference, AppBar, DateRange.thisMonth()) resolve to existing code.

Blocking

  • ExportAction.kt:199 — DateField mixes ZoneId.systemDefault() (line 188, initial seed) and ZoneId.of("UTC") (line 199, pick → LocalDate). For any user in a non-UTC zone, the picker and the field display different days, and confirming without re-picking silently shifts the export range. See the inline comment for the fix.

Non-blocking notes (out of scope for #7, not blocking merge)

  • ImportBankStatementScreenModel.kt:69, 77, 116 — three snackbar strings are hardcoded English ("No transactions found in PDF", the error::class.simpleName fallback, "No items selected") while the rest of the file uses stringResource. Worth aligning for localization parity.
  • ImportBankStatementScreenModel.kt:77 — falling back to error::class.simpleName can surface Kotlin class names (e.g. NotImplementedError) to the user. A generic "Import failed" fallback is friendlier.
  • SettingsScreenModel.kt:21, 30 — exportToCsv and clearData take a (Result<Unit>) -> Unit callback rather than returning a suspend Result<Unit> or a SharedFlow<Result<Unit>>. It works, but it's the only callback-style ScreenModel API in the codebase. If a second screen starts needing the same pattern, a Flow-based result channel would be more consistent.

Test plan

The PR body correctly notes that the three manual test items (Categories CRUD, Import flow, Settings flow) are left unchecked — the implementor has no device available, and the user triages those at review/merge time. The assembleDebug + testDebugUnitTest checks being pre-ticked is the right call given the diff is mostly Compose UI + state plumbing.

## Summary **Verdict: has one blocking concern, plus non-blocking notes.** The PR is well-structured and follows the spec from issue #7: the four pieces (`ExportAction`, `CategoryScreen`, `ImportBankStatementScreen`, `SettingsScreen`) are all in place, the Voyager + Koin ScreenModel pattern is used correctly, no ScreenModel is registered in DI, no repository layer is introduced, no manifest permissions are added, and no chart library is used. The `LazyVerticalGrid` + `LazyColumn` co-existence in `CategoryScreen` is handled with an aliased import, which is the right call. `./gradlew assembleDebug` and `:app:testDebugUnitTest` are reported green. All cross-references (`Category.DEFAULT_COLOR_*`, `ui.util.onClickInput`, `Preference.PreferenceItem.CustomPreference` / `AlertDialogPreference`, `AppBar`, `DateRange.thisMonth()`) resolve to existing code. ### Blocking - `ExportAction.kt:199` — `DateField` mixes `ZoneId.systemDefault()` (line 188, initial seed) and `ZoneId.of("UTC")` (line 199, pick → `LocalDate`). For any user in a non-UTC zone, the picker and the field display different days, and confirming without re-picking silently shifts the export range. See the inline comment for the fix. ### Non-blocking notes (out of scope for #7, not blocking merge) - `ImportBankStatementScreenModel.kt:69, 77, 116` — three snackbar strings are hardcoded English ("No transactions found in PDF", the `error::class.simpleName` fallback, "No items selected") while the rest of the file uses `stringResource`. Worth aligning for localization parity. - `ImportBankStatementScreenModel.kt:77` — falling back to `error::class.simpleName` can surface Kotlin class names (e.g. `NotImplementedError`) to the user. A generic "Import failed" fallback is friendlier. - `SettingsScreenModel.kt:21, 30` — `exportToCsv` and `clearData` take a `(Result<Unit>) -> Unit` callback rather than returning a suspend `Result<Unit>` or a `SharedFlow<Result<Unit>>`. It works, but it's the only callback-style ScreenModel API in the codebase. If a second screen starts needing the same pattern, a `Flow`-based result channel would be more consistent. ### Test plan The PR body correctly notes that the three manual test items (Categories CRUD, Import flow, Settings flow) are left unchecked — the implementor has no device available, and the user triages those at review/merge time. The `assembleDebug` + `testDebugUnitTest` checks being pre-ticked is the right call given the diff is mostly Compose UI + state plumbing.
app/src/main/java/dev/achmad/ledgerr/ui/components/ExportAction.kt
@@ -0,0 +196,4 @@
onClick = {
datePickerState.selectedDateMillis?.let { millis ->
val picked = Instant.ofEpochMilli(millis)
.atZone(ZoneId.of("UTC"))
admin commented 2026-06-28 11:25:01 +00:00
Author
Owner
Copy Link
Copy Source

Blocking: DateField is inconsistent about time zones. Line 188 seeds initialMillis with ZoneId.systemDefault() (so the OutlinedTextField shows the date in the user's zone), but line 199 converts the pick back with ZoneId.of("UTC"). The Compose DatePickerState.selectedDateMillis is documented as "UTC milliseconds at the start of the day for the date in the user's time zone" — i.e. the picker encodes the picked day as midnight UTC of that day. Concretely, for a user in UTC+7 with the field seeded to 2025-01-15:

  • initialMillis = 2025-01-15T00:00:00+07:00 = 2025-01-14T17:00:00Z
  • The DatePicker reads that as UTC midnight → displays 2025-01-14
  • User confirms without re-picking → atZone(UTC).toLocalDate() returns 2025-01-14
  • startDate/endDate are written as 2025-01-14, silently shifting the export range by a day in the common case.

Fix: pick one zone and apply it on both sides. The simpler change is ZoneId.of("UTC") on line 188 instead of ZoneId.systemDefault() — that way both seed and read agree on UTC and the picker/field always match. (Using systemDefault() on the read side would work too, but introduces a second zone dependency that's harder to reason about.)

**Blocking:** `DateField` is inconsistent about time zones. Line 188 seeds `initialMillis` with `ZoneId.systemDefault()` (so the OutlinedTextField shows the date in the user's zone), but line 199 converts the pick back with `ZoneId.of("UTC")`. The Compose `DatePickerState.selectedDateMillis` is documented as "UTC milliseconds at the start of the day for the date in the user's time zone" — i.e. the picker encodes the picked day as midnight UTC of *that* day. Concretely, for a user in UTC+7 with the field seeded to `2025-01-15`: - `initialMillis = 2025-01-15T00:00:00+07:00 = 2025-01-14T17:00:00Z` - The DatePicker reads that as UTC midnight → displays **2025-01-14** - User confirms without re-picking → `atZone(UTC).toLocalDate()` returns **2025-01-14** - `startDate`/`endDate` are written as 2025-01-14, silently shifting the export range by a day in the common case. Fix: pick one zone and apply it on both sides. The simpler change is `ZoneId.of("UTC")` on line 188 instead of `ZoneId.systemDefault()` — that way both seed and read agree on UTC and the picker/field always match. (Using `systemDefault()` on the read side would work too, but introduces a second zone dependency that's harder to reason about.)
admin added 1 commit 2026-06-28 11:27:52 +00:00
fix(#7): use UTC for DateField initial seed to match pick conversion ce01c175df 
DateField seeded the picker with ZoneId.systemDefault() but converted
the selectedDateMillis back with ZoneId.of("UTC"). For any non-UTC
user, the field and the picker displayed different days, and
confirming without re-picking silently shifted the export range by a
day. Use UTC on both sides (the DatePickerState contract is that
selectedDateMillis is UTC midnight of the picked day).
admin added 1 commit 2026-06-28 11:33:00 +00:00
fix(#7): localize import snackbar strings and drop class.simpleName fallback 5893ffc955 
The three snackbar strings in ImportBankStatementScreenModel were
hardcoded English while the rest of the app uses stringResource. Move
them to res/values/strings.xml. Also drop the
error::class.simpleName fallback in the failure branch — it surfaced
Kotlin class names like NotImplementedError to end users. Now falls
back to the localized 'Import failed' string.
admin added 1 commit 2026-06-28 11:46:20 +00:00
fix(#7): emit snackbar as resource id, resolve with stringResource in UI 22863bfcd6 
Drop the Context injection from the ScreenModel. Emit a sealed
ImportSnackbarMessage carrying either a @StringRes id or a dynamic
text (for the error.message case where the bank importer surfaces a
useful 'BRI import not yet implemented' string). The screen resolves
resource ids via stringResource() in Composable scope and shows the
snackbar; the original error.message info is preserved via the
Dynamic variant instead of being dropped.
admin merged commit 6f7d24a303 into main 2026-06-28 12:06:48 +00:00
admin deleted branch feat/7-implement-screens-export 2026-06-28 15:02:01 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
admin
Labels
No items
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
admin
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: admin/ledgerr#17
Delete Branch "feat/7-implement-screens-export"

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?