Implement ExpenseListScreen, AddEditExpenseScreen, AddEditRecurringScreen (#6) #18
Reference in New Issue
Block a user
Delete Branch "feat/6-implement-expense-list-add-edit-screens"
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
ExpenseListScreenwith 2 tabs (Expenses | Recurring) usingPrimaryTabRow+ foundationHorizontalPager, tab-awareFloatingActionButton(Manual + Import Bank Statement on Expenses tab, Add Recurring on Recurring tab, collapses on tab change), search field with date-range filter chips, long-press delete with confirmation dialog, andExportActionwired toExportExpensesToCsvwith a snackbar on failure.AddEditExpenseScreenandAddEditRecurringScreenas shared add/edit forms; edit mode prefills viaGetExpenses.awaitOne/GetRecurringExpenses.awaitOne, the Save action is disabled untilamount > 0 && categoryId != null, recurring edit preserves the loadednextDueDateperdocs/03-function-todos.md:116, and the expense form's "Manage Categories" button pushesCategoryScreen.DateFieldcomposable toui/components/DateField.kt(UTC roundtrip, matches the pattern from #7'sExportAction).Test plan
./gradlew assembleDebugsucceeds (37 tasks, 0 errors)AddEditExpenseScreenwith an empty form; fill amount + category → Save pops back to the list with the new rowisActiveflips and persistsnextDueDateExpenseListScreen→ date range dialog → SAFCreateDocument("text/csv")→ file written with UTF-8 BOM, ISO 8601 dates, category names, and quoted notesCloses #6
Summary
Verdict: clean — no blocking concerns.
This PR implements issue #6 end-to-end. The three new screens, their ScreenModels, and the shared
DateFieldcomposable all match the issue's acceptance criteria:ExpenseListScreenhas 2 tabs onPrimaryTabRow(the Material3 1.2+ successor ofTabRow, which is the correct modern choice) + foundationHorizontalPagerTabAwareFabtakesselectedTab: Intand renders different sub-actions per tab, andLaunchedEffect(selectedTab) { expanded = false }collapses the menu on tab changeAddEditExpenseScreenandAddEditRecurringScreenboth handle add and edit; prefill goes throughgetExpenses.awaitOne/getRecurringExpenses.awaitOneamount > 0 && categoryId != null(line 78 of each screen)navigator.pop()on successnextDueDatepreservation on recurring edit is implemented perdocs/03-function-todos.md:116—loadedNextDueDateis captured ininit { }and used as the fallback at save time, so astartDatechange in edit mode does not resetnextDueDate. New records correctly fall back tostartDateDateFielduses UTC midnight roundtrip, which is the right call given M3'sDatePickeroperates in UTCExportAction(from #7) is reused correctly, with a snackbar wired toR.string.settings_export_failureonResult.failurenextDueDateand the "Manage Categories" button on the expense form pushes theCategoryScreensingletonLocalDateflows cleanly through the screen model and is stored as epoch day by the existingLocalDateConverter— the UI never sees epoch longsArchitecture rules are all respected: ScreenModels are NOT in Koin (they use
inject()default parameters via theKoinExtensionshelpers); interactors arefactory-scoped perDomainModule; no repository layer; noTabNavigator; no Canvas charts; SAF only (no manifest permissions).Non-blocking notes (not in inline comments)
RecurringRowdoes not shownote(ExpenseListScreen.kt:367-414).ExpenseRowrenders the note as a second line (ExpenseListScreen.kt:343-351) andRecurringExpense.noteexists in the data model (docs/01-data-model.md:104). For parity, consider renderingitem.recurring.notein the same spot. Issue #6 doesn't specify row layout, so this is a design call, not a bug.setDate/setNote/setStartDate/setIntervaldon't callrefreshIsValid()— flagged by the explore agent but not a real concern: those fields aren't part of the validity check, so omitting the call is correct (onlysetAmountandsetCategoryneed it).save()early-returns silently on invalid input (?: returnatAddEditExpenseScreenModel.kt:89-90andAddEditRecurringScreenModel.kt:109-110) — defensive but the button isenabled = isValid && !isLoading, so the path is unreachable from the UI. Arequire(...)would catch bugs earlier if you prefer fail-fast.All test plan checkboxes are addressed by the implementation; the
./gradlew assembleDebugone is the only one the agent could mark itself. The seven manual checkboxes will be exercised in a follow-up round.@@ -0,0 +138,4 @@@OptIn(ExperimentalMaterial3Api::class)@Composableprivate fun CategoryDropdownField(Nit:
CategoryDropdownFieldis duplicated almost verbatim betweenAddEditExpenseScreen.kt(lines 141–180) andAddEditRecurringScreen.kt(lines 153–192) — only the label string resource differs. Extract it toui/components/CategoryDropdownField.kt, taking thelabel: Stringas a parameter, so both screens share one implementation. The component already only depends on public models and strings.@@ -0,0 +19,4 @@import androidx.compose.material3.MaterialThemeimport androidx.compose.material3.OutlinedTextFieldimport androidx.compose.material3.Scaffoldimport androidx.compose.material3.SwitchNit: The
import androidx.compose.material3.Switchis unused. The active toggle on the form is rendered viaToggleItem(fromui/components/CardSection.kt), which wrapsSwitchinternally. Remove this import.@@ -0,0 +150,4 @@@OptIn(ExperimentalMaterial3Api::class)@Composableprivate fun CategoryDropdownField(Nit: The matching duplicate in the recurring screen. See the comment on
AddEditExpenseScreen.kt:141— the twoCategoryDropdownFieldimplementations should be consolidated into a single shared component inui/components/.@@ -0,0 +414,4 @@}}@ComposableSuggestion:
TabAwareFab(and the innerMiniFab) is defined as a private helper insideExpenseListScreen.kt. The design doc and the implementor rules (docs/04-implementation-plan.md:288,.opencode/agent/implementor.md:156) describe the expanded mini-FAB as a shared component used by bothHomeScreenandExpenseListScreen.HomeScreenis currently a stub (ui/screens/home/HomeScreen.kt), so duplication isn't a problem yet — but once the home screen grows its FAB, moveTabAwareFab+MiniFabintoui/components/TabAwareFab.kt(taking the action list as a parameter) so both screens share the same expansion UI and only differ in which actions they pass in.@@ -0,0 +106,4 @@private fun matchesQuery(item: ExpenseWithCategory, query: String): Boolean {if (query.isBlank()) return trueval note = item.expense.note.orEmpty()val amountStr = item.expense.amount.toString()Suggestion:
item.expense.amount.toString()uses Java's defaultDouble.toString, which is locale-naive and uses scientific notation for very large or very small values. A user searching for"1000000"won't find an amount of1_000_000.0(which prints as"1.0E6"), and a user searching for"12.34"won't find12.345(which prints as"12.345", not the"12.35"they see in the row). For consistency with the display format inExpenseRow("%.2f".format(item.expense.amount)atExpenseListScreen.kt:359), use the same formatter here — or strip scientific notation explicitly.@@ -64,0 +79,4 @@<string name="expense_list_delete_message">This will permanently delete the expense.</string><string name="expense_list_empty">No expenses yet</string><string name="recurring_list_empty">No recurring expenses</string><string name="recurring_list_active">Active</string>Nit:
recurring_list_activeis defined here but never referenced anywhere in the new code (I searched the whole tree forR.string.recurring_list_activeand there are zero call sites). Either remove it or wire it up — e.g., as a label next to theSwitchinRecurringRow(ExpenseListScreen.kt:410-413).Summary
Verdict: still clean — all blocking concerns were already addressed; all 4 of the inline comments you addressed in
e8c7a14are correctly applied.Verified each of the 4 fixes against the new commit:
matchesQueryformat (ExpenseListScreenModel.kt:109) —"%.2f".format(item.expense.amount)now matches the row display.1_000_000.0will still render as"1000000.00"rather than"1.0E6". The single-line hunk is minimal and doesn't touch anything else.Switchimport removed (AddEditRecurringScreen.kt) — theimport androidx.compose.material3.Switchline is gone; the active toggle continues to useToggleItem(which wrapsSwitchinternally viaCardSection.kt). Grep confirms no otherSwitchreference in the file.CategoryDropdownFieldextracted toui/components/CategoryDropdownField.kt— new file is 61 lines, takeslabel: Stringas a parameter (4th positional), and both screens now call it with their ownstringResource(R.string.add_edit_expense_field_category)/stringResource(R.string.add_edit_recurring_field_category). The two old inlined definitions were removed, the now-unusedDropdownMenuItem/ExposedDropdownMenu*/mutableStateOfimports in both screens were dropped. No duplication remains.recurring_list_activestring removed fromstrings.xml— confirmed absent from the file.Scope discipline: 5 files, 66 insertions, 106 deletions — every changed line maps to one of the 4 fixes, no drive-by edits, no other files modified.
The two items you left unaddressed are fine to leave as-is:
ui/components/— the design doc hasHomeScreenandExpenseListScreensharing the same expanded mini-FAB, butHomeScreenis still a stub (ui/screens/home/HomeScreen.kt:6), so there's no current duplication. Extract this whenHomeScreenactually grows its FAB; for now, leaving it local is the right call.RecurringRownot showingnote— I flagged this as a parity/design call, not a bug, and issue #6 doesn't specify row layout. Defer to the design pass.The previous review is now stale; this one supersedes it. No new inline comments. The PR is ready from a review perspective —
./gradlew assembleDebugpasses per the test plan, and the 7 manual checkboxes will be exercised on a follow-up round.