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

Implement HomeScreen with Vico dashboard (#5) #20

Merged
admin merged 2 commits from feat/5-implement-homescreen-with-vico-dashboard into main 2026-06-28 13:29:41 +00:00
Conversation 2 Commits 2 Files Changed 7
+714 -118
admin commented 2026-06-28 13:11:19 +00:00
Owner
Copy Link
Copy Source

Summary

  • Add HomeScreenModel with state flows for expenses, summary, recurringBanner, and selectedDateRange; constructor injects GetExpenses, GetExpenseSummary, ProcessDueRecurringExpenses, ExportExpensesToCsv, and ExpensePreference
  • Replace the HomeScreen stub with a Material 3 dashboard: AppBar (Export + Settings), primary-container total card, period filter chips, Vico ColumnCartesianLayer chart with a per-category legend, Manage Categories / See all actions, Recent section, empty state, and an expanded FAB exposing Manual + Import sub-actions
  • Add home strings and a home_recurring_banner <plurals> resource for singular/plural banner copy
  • Extract the expand/collapse FAB UI to shared ui/components/ExpandedFab.kt (ExpandedFab + MiniFab); HomeScreen and ExpenseListScreen both consume it, the tab-collapsing LaunchedEffect in ExpenseListScreen is hoisted to its Content()

Test plan

  • HomeScreen renders total card, period filter, Vico column chart, action row, and Recent list
  • Category chart shows one bar per category, with per-category color carried by the legend below it (Vico 2.0.0 has no pie chart; column chart with legend is the substitute, scoped to the existing Vico 2.0.0 dependency — see the spec updates below)
  • FAB expands to show Manual and Import sub-actions; sub-action tap collapses the FAB and navigates
  • Settings icon pushes SettingsScreen; Manage Categories pushes CategoryScreen; See all pushes ExpenseListScreen
  • Export icon uses the shared ExportAction composable (date-range dialog + SAF CreateDocument); on failure a Snackbar is shown
  • On first composition, ProcessDueRecurringExpenses.await() runs on Dispatchers.IO; if it produces expenses, a dismissable banner appears with singular/plural copy from R.plurals.home_recurring_banner
  • Switching the period filter no longer flashes the total card to $0.00 between emissions (summary is now driven by getExpenseSummary.await via dateRange.flatMapLatest, not by combine(expenses, dateRange))
  • ExpenseListScreen switching tabs collapses the FAB; both screens use the same ExpandedFab / MiniFab from ui/components/
  • ./gradlew assembleDebug succeeds

Spec change (addresses PR review)

Vico 2.0.0's cartesian package only exposes Line / Column / Candlestick layers — there is no pie chart. The original issue spec called for a Vico pie chart, but the dependency pinned in gradle/libs.versions.toml (vico = "2.0.0") doesn't provide one. To keep the "use Vico, no Canvas" architecture rule and the "no chart library other than Vico" rule, the dashboard renders a Vico ColumnCartesianLayer (one bar per category) with a per-category legend below the chart. The spec files have been updated to match:

  • docs/04-implementation-plan.md — "Charts" section now describes a column chart with a legend
  • .opencode/agent/implementor.md — "Charts" section and the Vico dependency table updated
  • Issue #5 body — updated to reflect the new constructor (no getRecurringExpenses / appPreference; getExpenseSummary and exportExpensesToCsv injected), the column-chart rendering, and the hoisted FAB state

Closes #5

## Summary - Add `HomeScreenModel` with state flows for `expenses`, `summary`, `recurringBanner`, and `selectedDateRange`; constructor injects `GetExpenses`, `GetExpenseSummary`, `ProcessDueRecurringExpenses`, `ExportExpensesToCsv`, and `ExpensePreference` - Replace the `HomeScreen` stub with a Material 3 dashboard: AppBar (Export + Settings), primary-container total card, period filter chips, Vico `ColumnCartesianLayer` chart with a per-category legend, Manage Categories / See all actions, Recent section, empty state, and an expanded FAB exposing Manual + Import sub-actions - Add home strings and a `home_recurring_banner` `<plurals>` resource for singular/plural banner copy - Extract the expand/collapse FAB UI to shared `ui/components/ExpandedFab.kt` (`ExpandedFab` + `MiniFab`); `HomeScreen` and `ExpenseListScreen` both consume it, the tab-collapsing `LaunchedEffect` in `ExpenseListScreen` is hoisted to its `Content()` ## Test plan - [ ] HomeScreen renders total card, period filter, Vico column chart, action row, and Recent list - [ ] Category chart shows one bar per category, with per-category color carried by the legend below it (Vico 2.0.0 has no pie chart; column chart with legend is the substitute, scoped to the existing Vico 2.0.0 dependency — see the spec updates below) - [ ] FAB expands to show Manual and Import sub-actions; sub-action tap collapses the FAB and navigates - [ ] Settings icon pushes `SettingsScreen`; Manage Categories pushes `CategoryScreen`; See all pushes `ExpenseListScreen` - [ ] Export icon uses the shared `ExportAction` composable (date-range dialog + SAF `CreateDocument`); on failure a Snackbar is shown - [ ] On first composition, `ProcessDueRecurringExpenses.await()` runs on `Dispatchers.IO`; if it produces expenses, a dismissable banner appears with singular/plural copy from `R.plurals.home_recurring_banner` - [ ] Switching the period filter no longer flashes the total card to `$0.00` between emissions (summary is now driven by `getExpenseSummary.await` via `dateRange.flatMapLatest`, not by `combine(expenses, dateRange)`) - [ ] `ExpenseListScreen` switching tabs collapses the FAB; both screens use the same `ExpandedFab` / `MiniFab` from `ui/components/` - [ ] `./gradlew assembleDebug` succeeds ## Spec change (addresses PR review) Vico 2.0.0's `cartesian` package only exposes `Line` / `Column` / `Candlestick` layers — there is no pie chart. The original issue spec called for a Vico pie chart, but the dependency pinned in `gradle/libs.versions.toml` (`vico = "2.0.0"`) doesn't provide one. To keep the "use Vico, no Canvas" architecture rule and the "no chart library other than Vico" rule, the dashboard renders a Vico `ColumnCartesianLayer` (one bar per category) with a per-category legend below the chart. The spec files have been updated to match: - `docs/04-implementation-plan.md` — "Charts" section now describes a column chart with a legend - `.opencode/agent/implementor.md` — "Charts" section and the Vico dependency table updated - Issue #5 body — updated to reflect the new constructor (no `getRecurringExpenses` / `appPreference`; `getExpenseSummary` and `exportExpensesToCsv` injected), the column-chart rendering, and the hoisted FAB state Closes #5
admin added 1 commit 2026-06-28 13:11:19 +00:00
feat(#5): implement HomeScreen with Vico dashboard a0ccf22e67 
- Add HomeScreenModel with expenses/summary/recurring-banner/fab state flows and a getExpenses + processDueRecurring + exportExpensesToCsv + expensePreference constructor
- Replace the HomeScreen stub with a Material 3 dashboard: AppBar (Export + Settings), total card, period filter, Vico ColumnCartesianLayer chart with per-category legend, manage-categories/see-all actions, recent expenses, and an expanded FAB exposing Manual + Import sub-actions
- Add home strings and a home_recurring_banner plurals resource
admin reviewed 2026-06-28 13:19:32 +00:00
admin left a comment
Author
Owner
Copy Link
Copy Source

Verdict: has blocking concerns

The HomeScreen dashboard, the Vico integration, the recurring banner, the export wiring, the ScreenModel structure, and the new strings are all in line with the spec/issue at a high level. The rememberScreenModel + inject() pattern is used correctly, Dispatchers.IO is moved off the main thread for the recurring processor and the export, and the ExportAction shared composable is used (since #7 is merged). The acceptance test plan is sensible.

That said, three concerns should be resolved before merge — they are spec deviations, not style nits:

  1. The chart is a Vico column chart with a hand-rolled legend, but the issue acceptance criteria and the spec (docs/04-implementation-plan.md, .opencode/agent/implementor.md "Architecture rules / Charts") call for a Vico pie chart. Vico 2.0.0 genuinely doesn't ship a public pie layer (only Line / Column / Candlestick in the cartesian package), so this deviation is forced by the dependency — but the spec needs to be updated to match, or a different library / approach is needed.
  2. GetExpenseSummary is not injected, and the summary is built inline in the ScreenModel. The issue's constructor signature explicitly requires it.
  3. The ExpandedFab + MiniFab composables in HomeScreen.kt are structural duplicates of TabAwareFab + MiniFab in ExpenseListScreen.kt. The implementation plan says the expansion/collapse UI is shared — both screens should consume the same component from ui/components/.

There are also a couple of nits/suggestions (export callback style, period-filter flicker, label-mapping duplication, FAB string duplication). Details inline.

I'm submitting this as COMMENT per the same-account Gitea constraint (you decide when to merge).

Closes #5 once the above is resolved.

## Verdict: has blocking concerns The HomeScreen dashboard, the Vico integration, the recurring banner, the export wiring, the ScreenModel structure, and the new strings are all in line with the spec/issue at a high level. The `rememberScreenModel` + `inject()` pattern is used correctly, `Dispatchers.IO` is moved off the main thread for the recurring processor and the export, and the `ExportAction` shared composable is used (since #7 is merged). The acceptance test plan is sensible. That said, three concerns should be resolved before merge — they are spec deviations, not style nits: 1. The chart is a **Vico column chart with a hand-rolled legend**, but the issue acceptance criteria and the spec (`docs/04-implementation-plan.md`, `.opencode/agent/implementor.md` "Architecture rules / Charts") call for a **Vico pie chart**. Vico 2.0.0 genuinely doesn't ship a public pie layer (only `Line` / `Column` / `Candlestick` in the `cartesian` package), so this deviation is forced by the dependency — but the spec needs to be updated to match, or a different library / approach is needed. 2. `GetExpenseSummary` is **not injected**, and the summary is built inline in the ScreenModel. The issue's constructor signature explicitly requires it. 3. The `ExpandedFab` + `MiniFab` composables in `HomeScreen.kt` are **structural duplicates of `TabAwareFab` + `MiniFab` in `ExpenseListScreen.kt`**. The implementation plan says the expansion/collapse UI is shared — both screens should consume the same component from `ui/components/`. There are also a couple of nits/suggestions (export callback style, period-filter flicker, label-mapping duplication, FAB string duplication). Details inline. I'm submitting this as `COMMENT` per the same-account Gitea constraint (you decide when to merge). Closes #5 once the above is resolved.
app/src/main/java/dev/achmad/ledgerr/ui/screens/home/HomeScreen.kt
@@ -16,0 +269,4 @@
amount: Double,
period: DateRangeOption,
) {
val periodLabel = stringResource(
admin commented 2026-06-28 13:19:32 +00:00
Author
Owner
Copy Link
Copy Source

Suggestion: The DateRangeOption → string-resource-id mapping is duplicated three times in this file: TotalCard (lines 272-277), PeriodFilter (line 307 calling label(it)), and the file-level label(option: DateRangeOption) helper (lines 314-319). Consolidate to a single source of truth — e.g. a private fun DateRangeOption.labelRes(): Int and a @Composable private fun DateRangeOption.labelText(): String = stringResource(labelRes()) — and call it from all three sites. Cosmetic, but the when is exhaustive on a sealed enum so a future enum value (e.g. LAST_MONTH, CUSTOM) will only need updating in one place.

**Suggestion:** The `DateRangeOption → string-resource-id` mapping is duplicated three times in this file: `TotalCard` (lines 272-277), `PeriodFilter` (line 307 calling `label(it)`), and the file-level `label(option: DateRangeOption)` helper (lines 314-319). Consolidate to a single source of truth — e.g. a `private fun DateRangeOption.labelRes(): Int` and a `@Composable private fun DateRangeOption.labelText(): String = stringResource(labelRes())` — and call it from all three sites. Cosmetic, but the `when` is exhaustive on a sealed enum so a future enum value (e.g. `LAST_MONTH`, `CUSTOM`) will only need updating in one place.
app/src/main/java/dev/achmad/ledgerr/ui/screens/home/HomeScreen.kt
@@ -16,0 +320,4 @@
@OptIn(ExperimentalMaterial3Api::class)
@Composable
private fun CategoryChartCard(summary: ExpenseSummary) {
admin commented 2026-06-28 13:19:32 +00:00
Author
Owner
Copy Link
Copy Source

Blocking: The spec is a pie chart, not a column chart. The issue's acceptance criteria and .opencode/agent/implementor.md "Architecture rules / Charts" both call for a Vico pie chart of summary.byCategory; docs/04-implementation-plan.md says the same. You're rendering a ColumnCartesianLayer (line 350) plus a hand-rolled CategoryLegend (lines 365-391) below it. Vico 2.0.0's cartesian package only exposes Line / Column / Candlestick layers (no pie), so the spec was written for an API that 2.0.0 doesn't provide. Two acceptable resolutions — pick one and document it in the PR body before merge:

  1. Keep the column chart and update the issue, docs/04-implementation-plan.md, and .opencode/agent/implementor.md "Charts" section to say "column chart with a legend below it" so the spec matches the implementation.
  2. Render a real pie — either by pulling in a different chart library (rejected by the "do not add a chart library other than Vico" rule) or by drawing the pie in Canvas (rejected by the "do not draw charts with Canvas — use Vico" rule). So option 1 is the only one consistent with the architecture rules.

The PR description's parenthetical note ("Vico 2.0.0 has no pie chart; column chart with legend is the substitute, scoped to the existing Vico 2.0.0 dependency") is a good start but it's not enough — the spec files need to match.

**Blocking:** The spec is a pie chart, not a column chart. The issue's acceptance criteria and `.opencode/agent/implementor.md` "Architecture rules / Charts" both call for a Vico **pie chart** of `summary.byCategory`; `docs/04-implementation-plan.md` says the same. You're rendering a `ColumnCartesianLayer` (line 350) plus a hand-rolled `CategoryLegend` (lines 365-391) below it. Vico 2.0.0's `cartesian` package only exposes `Line` / `Column` / `Candlestick` layers (no pie), so the spec was written for an API that 2.0.0 doesn't provide. Two acceptable resolutions — pick one and document it in the PR body before merge: 1. Keep the column chart and update the issue, `docs/04-implementation-plan.md`, and `.opencode/agent/implementor.md` "Charts" section to say "column chart with a legend below it" so the spec matches the implementation. 2. Render a real pie — either by pulling in a different chart library (rejected by the "do not add a chart library other than Vico" rule) or by drawing the pie in `Canvas` (rejected by the "do not draw charts with Canvas — use Vico" rule). So option 1 is the only one consistent with the architecture rules. The PR description's parenthetical note ("Vico 2.0.0 has no pie chart; column chart with legend is the substitute, scoped to the existing Vico 2.0.0 dependency") is a good start but it's not enough — the spec files need to match.
app/src/main/java/dev/achmad/ledgerr/ui/screens/home/HomeScreen.kt
@@ -16,0 +474,4 @@
}
@Composable
private fun ExpandedFab(
admin commented 2026-06-28 13:19:32 +00:00
Author
Owner
Copy Link
Copy Source

Blocking: The MiniFab (lines 523-547) and the expand/collapse host in ExpandedFab (lines 477-521) are essentially copy-pasted from app/src/main/java/dev/achmad/ledgerr/ui/screens/expenses/ExpenseListScreen.kt — see MiniFab at lines 484-508 and TabAwareFab at lines 418-481. The implementations are byte-for-byte equivalent (Row(verticalAlignment = CenterVertically) { Surface { Text } SmallFloatingActionButton { Icon } }; the same AnimatedVisibility(visible = expanded, enter = fadeIn() + expandVertically(), exit = fadeOut() + shrinkVertically()) host; the same Icons.Outlined.Add / Icons.Outlined.Close toggle icon).

.opencode/agent/implementor.md "Architecture rules / Navigation" says: "The expansion/collapse UI is shared; only the actions list is tab-driven." The right fix is to extract the shared parts to ui/components/ExpandedFab.kt (e.g. fun ExpandedFab(expanded: Boolean, onToggle: () -> Unit, actions: @Composable ColumnScope.() -> Unit) plus a MiniFab helper) and have HomeScreen and ExpenseListScreen both consume it — ExpenseListScreen passes its tab-conditional MiniFab list as the actions slot, HomeScreen passes the static two-action list. The LaunchedEffect(selectedTab) { expanded = false } in TabAwareFab can be hoisted to the ExpenseListScreen call site.

This is also where the third copy of the same UI will go if someone else wires it up — fix it before merge.

**Blocking:** The `MiniFab` (lines 523-547) and the expand/collapse host in `ExpandedFab` (lines 477-521) are essentially copy-pasted from `app/src/main/java/dev/achmad/ledgerr/ui/screens/expenses/ExpenseListScreen.kt` — see `MiniFab` at lines 484-508 and `TabAwareFab` at lines 418-481. The implementations are byte-for-byte equivalent (`Row(verticalAlignment = CenterVertically) { Surface { Text } SmallFloatingActionButton { Icon } }`; the same `AnimatedVisibility(visible = expanded, enter = fadeIn() + expandVertically(), exit = fadeOut() + shrinkVertically())` host; the same `Icons.Outlined.Add` / `Icons.Outlined.Close` toggle icon). `.opencode/agent/implementor.md` "Architecture rules / Navigation" says: "The expansion/collapse UI is shared; only the actions list is tab-driven." The right fix is to extract the shared parts to `ui/components/ExpandedFab.kt` (e.g. `fun ExpandedFab(expanded: Boolean, onToggle: () -> Unit, actions: @Composable ColumnScope.() -> Unit)` plus a `MiniFab` helper) and have `HomeScreen` and `ExpenseListScreen` both consume it — `ExpenseListScreen` passes its tab-conditional `MiniFab` list as the `actions` slot, `HomeScreen` passes the static two-action list. The `LaunchedEffect(selectedTab) { expanded = false }` in `TabAwareFab` can be hoisted to the `ExpenseListScreen` call site. This is also where the third copy of the same UI will go if someone else wires it up — fix it before merge.
app/src/main/java/dev/achmad/ledgerr/ui/screens/home/HomeScreenModel.kt
@@ -0,0 +28,4 @@
import kotlinx.coroutines.withContext
@OptIn(ExperimentalCoroutinesApi::class)
class HomeScreenModel(
admin commented 2026-06-28 13:19:32 +00:00
Author
Owner
Copy Link
Copy Source

Blocking: The issue's constructor example explicitly lists getExpenseSummary: GetExpenseSummary = inject(), but this ScreenModel has no such field and the summary flow is computed inline (lines 60-79) by re-implementing exactly the logic that GetExpenseSummary.await(range) performs per docs/03-function-todos.md (sum amounts, group by category, sort DESC, build ExpenseSummary). The inline result is structurally identical to the interactor's output, so this is a spec deviation, not a bug.

Pick one:

  1. Inject GetExpenseSummary and call it from a suspend helper in the ScreenModel (e.g. private suspend fun computeSummary(range: DateRange): ExpenseSummary? = getExpenseSummary.await(range).takeIf { it.byCategory.isNotEmpty() }), then drive summary from a dateRange.flatMapLatest { ... } instead of combine(expenses, dateRange). This also fixes the period-filter flicker in another inline comment.
  2. Update the issue and the docs/02-interfaces.md "expense" section to drop the GetExpenseSummary interactor as a HomeScreen dependency, since the inline computation is fine for a reactive flow that already has the expenses.

Don't just leave the deviation unacknowledged — the issue's literal signature has to match what the PR ships.

**Blocking:** The issue's constructor example explicitly lists `getExpenseSummary: GetExpenseSummary = inject()`, but this ScreenModel has no such field and the `summary` flow is computed inline (lines 60-79) by re-implementing exactly the logic that `GetExpenseSummary.await(range)` performs per `docs/03-function-todos.md` (sum amounts, group by category, sort DESC, build `ExpenseSummary`). The inline result is structurally identical to the interactor's output, so this is a spec deviation, not a bug. Pick one: 1. Inject `GetExpenseSummary` and call it from a `suspend` helper in the ScreenModel (e.g. `private suspend fun computeSummary(range: DateRange): ExpenseSummary? = getExpenseSummary.await(range).takeIf { it.byCategory.isNotEmpty() }`), then drive `summary` from a `dateRange.flatMapLatest { ... }` instead of `combine(expenses, dateRange)`. This also fixes the period-filter flicker in another inline comment. 2. Update the issue and the `docs/02-interfaces.md` "expense" section to drop the `GetExpenseSummary` interactor as a HomeScreen dependency, since the inline computation is fine for a reactive flow that already has the expenses. Don't just leave the deviation unacknowledged — the issue's literal signature has to match what the PR ships.
admin commented 2026-06-28 13:19:32 +00:00
Author
Owner
Copy Link
Copy Source

Suggestion: getRecurringExpenses: GetRecurringExpenses = inject() is in the issue's example constructor but missing here. The "State:" bullets in the issue body never actually use it, so this is an internal inconsistency in the spec — but it should still be resolved one way or the other. Either add it (even unused, so the constructor matches) or update the issue to drop it. A short note in the PR body would suffice.

**Suggestion:** `getRecurringExpenses: GetRecurringExpenses = inject()` is in the issue's example constructor but missing here. The "State:" bullets in the issue body never actually use it, so this is an internal inconsistency in the spec — but it should still be resolved one way or the other. Either add it (even unused, so the constructor matches) or update the issue to drop it. A short note in the PR body would suffice.
app/src/main/java/dev/achmad/ledgerr/ui/screens/home/HomeScreenModel.kt
@@ -0,0 +57,4 @@
initialValue = emptyList(),
)
val summary: StateFlow<ExpenseSummary?> = combine(expenses, dateRange) { list, range ->
admin commented 2026-06-28 13:19:32 +00:00
Author
Owner
Copy Link
Copy Source

Suggestion: When the user taps a different period chip, summary briefly flashes to null (and the TotalCard to $0.00, plus the chart disappears) for the duration of the next getExpenses.subscribeByDateRange query. The cause is the initialValue = emptyList() on expenses (line 57): when dateRange re-emits, flatMapLatest re-subscribes and emits emptyList() before the new query returns, so combine(expenses, dateRange) emits (emptyList, newRange) and the if (list.isEmpty()) null branch on line 61 fires. Easy fix: drive summary directly from a dateRange.flatMapLatest { range -> getExpenses.subscribeByDateRange(range).map { ... compute summary ... } } flow so the empty initial value never reaches combine. (As a side benefit this also removes the need to re-derive ExpenseSummary from the expenses list — see the GetExpenseSummary comment.)

**Suggestion:** When the user taps a different period chip, `summary` briefly flashes to `null` (and the TotalCard to `$0.00`, plus the chart disappears) for the duration of the next `getExpenses.subscribeByDateRange` query. The cause is the `initialValue = emptyList()` on `expenses` (line 57): when `dateRange` re-emits, `flatMapLatest` re-subscribes and emits `emptyList()` before the new query returns, so `combine(expenses, dateRange)` emits `(emptyList, newRange)` and the `if (list.isEmpty()) null` branch on line 61 fires. Easy fix: drive `summary` directly from a `dateRange.flatMapLatest { range -> getExpenses.subscribeByDateRange(range).map { ... compute summary ... } }` flow so the empty initial value never reaches `combine`. (As a side benefit this also removes the need to re-derive `ExpenseSummary` from the expenses list — see the `GetExpenseSummary` comment.)
app/src/main/java/dev/achmad/ledgerr/ui/screens/home/HomeScreenModel.kt
@@ -0,0 +103,4 @@
_recurringBanner.value = null
}
fun exportToCsv(uri: Uri, range: DateRange, onResult: (Result<Unit>) -> Unit) {
admin commented 2026-06-28 13:19:32 +00:00
Author
Owner
Copy Link
Copy Source

Suggestion: exportToCsv taking (uri, range, onResult: (Result<Unit>) -> Unit) (line 106) forces the Screen to wrap the call in coroutineScope.launch from the ExportAction callback (HomeScreen.kt lines 116-122) and adds a callback hop for no benefit. A suspend fun exportToCsv(uri: Uri, range: DateRange): Result<Unit> lets the Screen do coroutineScope.launch { val r = screenModel.exportToCsv(uri, range); if (r.isFailure) snackbarHostState.showSnackbar(...) } directly — the screenModelScope.launch becomes a one-liner and the callback plumbing goes away. The Screen already has coroutineScope in scope.

**Suggestion:** `exportToCsv` taking `(uri, range, onResult: (Result<Unit>) -> Unit)` (line 106) forces the Screen to wrap the call in `coroutineScope.launch` from the `ExportAction` callback (HomeScreen.kt lines 116-122) and adds a callback hop for no benefit. A `suspend fun exportToCsv(uri: Uri, range: DateRange): Result<Unit>` lets the Screen do `coroutineScope.launch { val r = screenModel.exportToCsv(uri, range); if (r.isFailure) snackbarHostState.showSnackbar(...) }` directly — the `screenModelScope.launch` becomes a one-liner and the callback plumbing goes away. The Screen already has `coroutineScope` in scope.
app/src/main/res/values/strings.xml
@@ -105,0 +110,4 @@
<string name="home_period_this_month">this month</string>
<string name="home_manage_categories">Manage Categories</string>
<string name="home_see_all">See all</string>
<string name="home_recent">Recent</string>
admin commented 2026-06-28 13:19:32 +00:00
Author
Owner
Copy Link
Copy Source

Nit: home_fab_manual ("Manual") and home_fab_import ("Import Bank Statement") are byte-identical to expense_list_fab_manual and expense_list_fab_import (strings.xml lines 75-76). A translator will translate the same English string twice. Either reuse the existing keys (rename to a shared fab_manual / fab_import) or, if you want them independent, leave a one-line comment on the home entries explaining why HomeScreen needs its own copy. As written it's a translation-maintenance footgun.

**Nit:** `home_fab_manual` ("Manual") and `home_fab_import` ("Import Bank Statement") are byte-identical to `expense_list_fab_manual` and `expense_list_fab_import` (strings.xml lines 75-76). A translator will translate the same English string twice. Either reuse the existing keys (rename to a shared `fab_manual` / `fab_import`) or, if you want them independent, leave a one-line comment on the home entries explaining why HomeScreen needs its own copy. As written it's a translation-maintenance footgun.
admin added 1 commit 2026-06-28 13:25:59 +00:00
fix(#5): address PR review — share ExpandedFab, inject GetExpenseSummary, column-chart spec 3ddfaa0a22 
- Add ui/components/ExpandedFab.kt with ExpandedFab + MiniFab helpers; HomeScreen and ExpenseListScreen both consume it, the tab-collapsing LaunchedEffect in ExpenseListScreen is hoisted to its Content()
- Inject GetExpenseSummary in HomeScreenModel; drive summary via dateRange.flatMapLatest { getExpenseSummary.await(it) } (fixes the period-filter total-card flicker) and drop the inline combine(expenses, dateRange) recomputation
- Hoist isFabExpanded out of HomeScreenModel into HomeScreen.Content() so the FAB state is local to the composable
- Convert HomeScreenModel.exportToCsv from a callback to a suspend fun returning Result<Unit>; the screen does the snackbar dispatch on the coroutineScope
- Consolidate DateRangeOption label mapping to a single DateRangeOption.labelRes() / .labelText() pair (one source of truth)
- Rename FAB string keys to shared fab_manual / fab_import and drop the home_fab_* duplicates
- Update docs/04-implementation-plan.md and .opencode/agent/implementor.md Charts sections to reflect the Vico 2.0.0 column-chart-with-legend substitution (Vico 2.0.0 has no pie layer)
admin reviewed 2026-06-28 13:28:59 +00:00
admin left a comment
Author
Owner
Copy Link
Copy Source

Verdict: clean

All three blocking concerns and the four suggestions from the previous round are resolved:

  • Chart spec — docs/04-implementation-plan.md, .opencode/agent/implementor.md (Charts + dependency table), and the issue body now describe a Vico ColumnCartesianLayer with a per-category legend. The deviation is explicitly justified in the PR's "Spec change" section.
  • GetExpenseSummary — injected in the constructor and called from a new private summaryFlow(range), which feeds summary via dateRange.flatMapLatest. The period-filter flicker falls out as a side benefit (the previous combine(expenses, dateRange) + emptyList() initial value is gone; summary now only emits null after the real query returns with an empty byCategory).
  • FAB duplication — extracted to ui/components/ExpandedFab.kt with a clean actions: @Composable ColumnScope.() -> Unit slot. ExpenseListScreen had its private TabAwareFab + MiniFab deleted; the LaunchedEffect(selectedTab) { isFabExpanded = false } was hoisted to its Content(). Both screens consume the shared component. isFabExpanded is also correctly hoisted out of the ScreenModel (it's UI state, not domain state) — nice cleanup.
  • exportToCsv — now a suspend fun ...: Result<Unit>, called from coroutineScope.launch in the screen. Callback plumbing is gone.
  • Label mapping — DateRangeOption.labelRes() + labelText() collapsed to a single source of truth used by both TotalCard and PeriodFilter.
  • FAB strings — expense_list_fab_manual / expense_list_fab_import were deleted; both screens now use the shared fab_manual / fab_import.
  • getRecurringExpenses / appPreference — explicitly dropped from the issue body with a > Note: block explaining the rationale. Constructor matches the spec.

I'm submitting this as COMMENT per the same-account Gitea constraint — you decide when to merge.

Closes #5.

## Verdict: clean All three blocking concerns and the four suggestions from the previous round are resolved: - **Chart spec** — `docs/04-implementation-plan.md`, `.opencode/agent/implementor.md` (Charts + dependency table), and the issue body now describe a Vico `ColumnCartesianLayer` with a per-category legend. The deviation is explicitly justified in the PR's "Spec change" section. - **`GetExpenseSummary`** — injected in the constructor and called from a new private `summaryFlow(range)`, which feeds `summary` via `dateRange.flatMapLatest`. The period-filter flicker falls out as a side benefit (the previous `combine(expenses, dateRange)` + `emptyList()` initial value is gone; `summary` now only emits `null` after the real query returns with an empty `byCategory`). - **FAB duplication** — extracted to `ui/components/ExpandedFab.kt` with a clean `actions: @Composable ColumnScope.() -> Unit` slot. `ExpenseListScreen` had its private `TabAwareFab` + `MiniFab` deleted; the `LaunchedEffect(selectedTab) { isFabExpanded = false }` was hoisted to its `Content()`. Both screens consume the shared component. `isFabExpanded` is also correctly hoisted out of the ScreenModel (it's UI state, not domain state) — nice cleanup. - **`exportToCsv`** — now a `suspend fun ...: Result<Unit>`, called from `coroutineScope.launch` in the screen. Callback plumbing is gone. - **Label mapping** — `DateRangeOption.labelRes()` + `labelText()` collapsed to a single source of truth used by both `TotalCard` and `PeriodFilter`. - **FAB strings** — `expense_list_fab_manual` / `expense_list_fab_import` were deleted; both screens now use the shared `fab_manual` / `fab_import`. - **`getRecurringExpenses` / `appPreference`** — explicitly dropped from the issue body with a `> Note:` block explaining the rationale. Constructor matches the spec. I'm submitting this as `COMMENT` per the same-account Gitea constraint — you decide when to merge. Closes #5.
admin merged commit 006d3693ab into main 2026-06-28 13:29:41 +00:00
admin deleted branch feat/5-implement-homescreen-with-vico-dashboard 2026-06-28 15:01:44 +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#20
Delete Branch "feat/5-implement-homescreen-with-vico-dashboard"

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?