perf(accounts): defer the account ledger prop on show (#632)
## Problem `AccountController@show` (added in #631) loaded the **full transaction ledger** for an account into the initial Inertia payload on every visit — `transactions()->with(['category','labels'])->get()`. For accounts with years of movements that's several MB, all on the critical render path, blocking first paint. The `ponytail:` comment flagged the deferred/cursor move as a follow-up; this PR does it. ## Fix Wrap the `transactions` prop in `Inertia::defer(...)` so the page shell (header, chart, actions) paints immediately and the ledger arrives in a follow-up request behind a skeleton. Mirrors how `index()` already defers `accountMetrics` and how `dashboard.tsx` consumes `<Deferred>`. The ledger **stays the whole set on purpose** — search and filtering run client-side over *decrypted* rows (privacy-first design), so the client needs every row. Cursor/offset pagination isn't viable without breaking search over encrypted fields, so deferred is the right lever: it moves the cost off the critical path rather than reducing bytes. Non-transactional account types (`loan`/`investment`/`retirement`/`real_estate`, i.e. `!hasTransactionLedger()`) return a plain `[]` and never render `<Deferred>`, so they take no extra round-trip. ## Testing - `AccountControllerTest`: the "includes transactions" case migrated from `->has('transactions', 2)` to `->missing('transactions')->loadDeferredProps(...)`, proving the prop is deferred yet resolves. - `Show.test.tsx`: added coverage that creating a transaction reloads **only** the deferred prop (`router.reload({ only: ['transactions'] })`) — this refresh became load-bearing on deferred-prop semantics once the remount `key` was dropped in #631. - `php artisan test` (AccountController + PageQueryCount): 45 passed. Vitest Show: 3 passed. Pint + ESLint clean. ## Reviewer notes - **Known minor (not fixed):** brief loading-skeleton shape change on first load — the `<Deferred>` fallback (plain bars) differs from `TransactionList`'s own internal table skeleton, so there's a ~1-frame visual jump. Aligning them would mean either duplicating the table skeleton or adding a prop to the shared component; judged not worth it for a sub-frame cosmetic nit. - **Out of scope (pre-existing, from #631, flagged during review):** since #631 removed the remount `key`, an active filter/search now persists across a create-reload (a new non-matching row can look "not created"); and in-memory search/sort currently operates on still-encrypted rows pending the plaintext migration. Neither is introduced or worsened here.
This commit is contained in:
parent
4fdb7eebd9
commit
9326d8fd2f
|
|
@ -120,20 +120,20 @@ class AccountController extends Controller
|
|||
}
|
||||
}
|
||||
|
||||
// ponytail: load-all transactions for the account (server-side, replaces
|
||||
// the old client-side sync-all-then-filter). Fine for a single account;
|
||||
// switch to a deferred/cursor-paginated prop if accounts grow huge.
|
||||
$transactions = $account->type->hasTransactionLedger()
|
||||
? $account->transactions()
|
||||
->with(['category', 'labels'])
|
||||
->orderBy('transaction_date', 'desc')
|
||||
->orderBy('id', 'desc')
|
||||
->get()
|
||||
: [];
|
||||
|
||||
return Inertia::render('Accounts/Show', [
|
||||
'account' => $data,
|
||||
'transactions' => $transactions,
|
||||
// Deferred so the page shell paints without blocking on the ledger
|
||||
// query/serialization. It stays the whole set because search and
|
||||
// filtering run client-side over decrypted rows. ponytail: window it
|
||||
// server-side only if one account's history gets big enough that the
|
||||
// transfer itself hurts.
|
||||
'transactions' => $account->type->hasTransactionLedger()
|
||||
? Inertia::defer(fn () => $account->transactions()
|
||||
->with(['category', 'labels'])
|
||||
->orderBy('transaction_date', 'desc')
|
||||
->orderBy('id', 'desc')
|
||||
->get())
|
||||
: [],
|
||||
]);
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -1,4 +1,5 @@
|
|||
import { fireEvent, render, screen } from '@testing-library/react';
|
||||
import { router } from '@inertiajs/react';
|
||||
import { act, fireEvent, render, screen } from '@testing-library/react';
|
||||
import { type ReactNode } from 'react';
|
||||
import { describe, expect, it, vi } from 'vitest';
|
||||
|
||||
|
|
@ -7,6 +8,7 @@ import AccountShow from './Show';
|
|||
vi.mock('@inertiajs/react', () => ({
|
||||
Head: () => null,
|
||||
router: { reload: vi.fn() },
|
||||
Deferred: ({ children }: { children: ReactNode }) => <>{children}</>,
|
||||
}));
|
||||
|
||||
vi.mock('@/actions/App/Http/Controllers/AccountController', () => ({
|
||||
|
|
@ -115,6 +117,17 @@ describe('AccountShow', () => {
|
|||
);
|
||||
});
|
||||
|
||||
it('reloads only the deferred transactions prop after a transaction is created', () => {
|
||||
renderPage();
|
||||
|
||||
const { onSuccess } = editTransactionDialog.mock.calls.at(-1)![0] as {
|
||||
onSuccess: () => void;
|
||||
};
|
||||
act(() => onSuccess());
|
||||
|
||||
expect(router.reload).toHaveBeenCalledWith({ only: ['transactions'] });
|
||||
});
|
||||
|
||||
it('hides transaction action for connected accounts', () => {
|
||||
renderPage({
|
||||
...baseAccount,
|
||||
|
|
|
|||
|
|
@ -39,6 +39,7 @@ import {
|
|||
SelectTrigger,
|
||||
SelectValue,
|
||||
} from '@/components/ui/select';
|
||||
import { Skeleton } from '@/components/ui/skeleton';
|
||||
import { Textarea } from '@/components/ui/textarea';
|
||||
import { useChartColors } from '@/hooks/use-chart-color-scheme';
|
||||
import AppSidebarLayout from '@/layouts/app/app-sidebar-layout';
|
||||
|
|
@ -63,7 +64,7 @@ import { Label as LabelType } from '@/types/label';
|
|||
import { type Transaction } from '@/types/transaction';
|
||||
import { formatDateMedium } from '@/utils/date';
|
||||
import { __ } from '@/utils/i18n';
|
||||
import { Head, router } from '@inertiajs/react';
|
||||
import { Deferred, Head, router } from '@inertiajs/react';
|
||||
import { ChevronDown, Pencil, Plus } from 'lucide-react';
|
||||
import { useCallback, useMemo, useState } from 'react';
|
||||
import { Line, LineChart, ResponsiveContainer, Tooltip } from 'recharts';
|
||||
|
|
@ -382,23 +383,34 @@ export default function AccountShow({
|
|||
)}
|
||||
|
||||
{isTransactionalAccount(account) && (
|
||||
<TransactionList
|
||||
categories={categories}
|
||||
accounts={accounts}
|
||||
banks={banks}
|
||||
labels={labels}
|
||||
automationRules={automationRules}
|
||||
accountId={account.id}
|
||||
transactions={transactions}
|
||||
pageSize={50}
|
||||
hideAccountFilter={true}
|
||||
showActionsMenu={false}
|
||||
maxHeight={600}
|
||||
hideColumns={['bank', 'account']}
|
||||
onBalanceUpdated={() =>
|
||||
setChartRefreshKey((key) => key + 1)
|
||||
<Deferred
|
||||
data="transactions"
|
||||
fallback={
|
||||
<div className="space-y-2">
|
||||
{Array.from({ length: 8 }).map((_, i) => (
|
||||
<Skeleton key={i} className="h-12 w-full" />
|
||||
))}
|
||||
</div>
|
||||
}
|
||||
/>
|
||||
>
|
||||
<TransactionList
|
||||
categories={categories}
|
||||
accounts={accounts}
|
||||
banks={banks}
|
||||
labels={labels}
|
||||
automationRules={automationRules}
|
||||
accountId={account.id}
|
||||
transactions={transactions}
|
||||
pageSize={50}
|
||||
hideAccountFilter={true}
|
||||
showActionsMenu={false}
|
||||
maxHeight={600}
|
||||
hideColumns={['bank', 'account']}
|
||||
onBalanceUpdated={() =>
|
||||
setChartRefreshKey((key) => key + 1)
|
||||
}
|
||||
/>
|
||||
</Deferred>
|
||||
)}
|
||||
</div>
|
||||
|
||||
|
|
|
|||
|
|
@ -238,7 +238,10 @@ test('account show includes the account transactions and excludes other accounts
|
|||
->assertOk()
|
||||
->assertInertia(fn ($page) => $page
|
||||
->component('Accounts/Show')
|
||||
->has('transactions', 2)
|
||||
->missing('transactions')
|
||||
->loadDeferredProps(fn ($reload) => $reload
|
||||
->has('transactions', 2)
|
||||
)
|
||||
);
|
||||
});
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue