Conversation
There was a problem hiding this comment.
Pull request overview
This PR renames the PWA "Expenses" section to "Transactions" and extends the monthly ledger drill-in feature so that Income, Expense, and Transfer rows all navigate to month-scoped category detail screens. It also refactors the category detail flow using three focused hooks (useCategoryTransactionsRoute, useCategoryTransactions, useCategoryTransactionsShell) and introduces a ShellChromeContext that lets deep page components override the app bar title, active tab, and back-button behavior from the shell.
Changes:
- Route rename from
/expensesto/transactionswith legacy redirect support, and a newShellChromeContextfor shell overrides from page components - Income and Transfer ledger rows made tappable (previously only Expense rows were), with transfer drill-ins carrying a
direction=in|outURL param and client-side filtering - Category detail page refactored into three single-responsibility hooks; Playwright tests extended to cover all three drill-in scenarios and legacy redirect behavior
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
apps/pwa/src/App.tsx |
Adds ShellChromeContext, legacy /expenses* → /transactions* redirect routes, and reads shell overrides for title/tab/back |
apps/pwa/src/components/MobileShell.tsx |
Renames "Expenses" tab to "Transactions", adds activeTab override prop |
apps/pwa/src/pages/HomePage.tsx |
Replaces handleOpenExpenseCategory with handleOpenCategory supporting optional direction |
apps/pwa/src/pages/ExpensesPage.tsx |
Extracts query logic to useTransactionsPage hook |
apps/pwa/src/pages/ExpenseCategoryDetailPage.tsx |
Refactored to compose three focused hooks for route, data, and shell chrome |
apps/pwa/src/lib/shell-chrome.tsx |
New context + useShellChrome hook for page-level shell overrides |
apps/pwa/src/features/home/components/MonthlyLedgerCard.tsx |
Makes income/transfer rows tappable; extracts LedgerTransferRow component |
apps/pwa/src/features/expenses/hooks/useTransactionsPage.ts |
New hook consolidating categories+expenses queries for the Transactions page |
apps/pwa/src/features/expenses/hooks/useCategoryTransactions.ts |
New hook for category detail data fetching with client-side direction filtering |
apps/pwa/src/features/expenses/hooks/useCategoryTransactionsRoute.ts |
New hook parsing route params for category detail |
apps/pwa/src/features/expenses/hooks/useCategoryTransactionsShell.ts |
New hook configuring shell chrome for category detail page |
apps/pwa/src/features/expenses/components/ExpensesList.tsx |
Updates error message to "transactions" |
tests/pwa/mobile.spec.ts |
Adds legacy redirect test and extends ledger drill-in test to cover income, expense, and transfer |
README.md |
Updates feature descriptions to reflect route rename and new drill-in behavior |
AGENTS.md |
Updates guidance for route names and hook decomposition conventions |
.agents/notes.md |
Adds notes about sandbox port-binding and monzo sync behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const categories = view.categories; | ||
|
|
||
| return ( | ||
| <Box> | ||
| <ExpensesList | ||
| categories={categories} | ||
| expenses={expensesQuery.data ?? []} | ||
| isLoading={categoriesQuery.isLoading || expensesQuery.isLoading} | ||
| isError={categoriesQuery.isError || expensesQuery.isError} | ||
| emptyLabel="No expenses logged yet." | ||
| categories={view.categories} |
There was a problem hiding this comment.
The categories local variable (alias for view.categories) is used later in the dropdown at line 105, but view.categories is also passed directly to ExpensesList at line 65 instead of using the categories alias. For consistency, ExpensesList's categories prop should use the categories alias (or conversely, the alias should be removed and view.categories used throughout). As written, this mixed usage is slightly confusing.
| onRowClick?: (input: LedgerTransferClickInput) => void; | ||
| }) => ( | ||
| <ListItem | ||
| key={`transfer-${item.categoryId}-${item.direction}`} |
There was a problem hiding this comment.
The key prop at line 210 inside the LedgerTransferRow component body is a no-op and should be removed. React only uses key on elements returned from a .map() at the call site to reconcile sibling lists — placing key on the root element returned by a functional component (where it's not inside a sibling array) has no effect. The actual key that React uses is correctly set on the <LedgerTransferRow> call site at line 262.
Summary
/transactionswith legacy/expensesredirectsTesting